[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v2 17/21] drm/fb-helper: Perform all fbdev I/O with the same implementation



Hi

Am 02.11.22 um 10:32 schrieb Javier Martinez Canillas:
On 10/24/22 13:19, Thomas Zimmermann wrote:
Implement the fbdev's read/write helpers with the same functions. Use
the generic fbdev's code as template. Convert all drivers.

DRM's fb helpers must implement regular I/O functionality in struct
fb_ops and possibly perform a damage update. Handle all this in the
same functions and convert drivers. The functionality has been used
as part of the generic fbdev code for some time. The drivers don't
set struct drm_fb_helper.fb_dirty, so they will not be affected by
damage handling.

For I/O memory, fb helpers now provide drm_fb_helper_cfb_read() and
drm_fb_helper_cfb_write(). Several drivers require these. Until now
tegra used I/O read and write, although the memory buffer appears to
be in system memory. So use _sys_ helpers now.

Signed-off-by: Thomas Zimmermann <tzimmermann@xxxxxxx>
---

[...]

+static ssize_t __drm_fb_helper_write(struct fb_info *info, const char __user 
*buf, size_t count,
+                                    loff_t *ppos, drm_fb_helper_write_screen 
write_screen)
+{

[...]

+       /*
+        * Copy to framebuffer even if we already logged an error. Emulates
+        * the behavior of the original fbdev implementation.
+        */
+       ret = write_screen(info, buf, count, pos);
+       if (ret < 0)
+               return ret; /* return last error, if any */
+       else if (!ret)
+               return err; /* return previous error, if any */
+
+       *ppos += ret;
+

Should *ppos be incremented even if the previous error is returned?

Yes. It emulates the original fbdev code at [1]. Further down in that function, the position is being updated even if an error occured. We only return the initial error if no bytes got written.

It could happen that some userspace program hits to error, but still relies on the output and position being updated. IIRC I even added validation of this behavior to the IGT fbdev tests. I agree that this is somewhat bogus behavior, but changing it would change long-standing userspace semantics.

[1] https://elixir.bootlin.com/linux/v6.0.6/source/drivers/video/fbdev/core/fbmem.c#L825


The write_screen() succeeded anyways, even when the count written was
smaller than what the caller asked for.

  /**
- * drm_fb_helper_sys_read - wrapper around fb_sys_read
+ * drm_fb_helper_sys_read - Implements struct &fb_ops.fb_read for system memory
   * @info: fb_info struct pointer
   * @buf: userspace buffer to read from framebuffer memory
   * @count: number of bytes to read from framebuffer memory
   * @ppos: read offset within framebuffer memory
   *
- * A wrapper around fb_sys_read implemented by fbdev core
+ * Returns:
+ * The number of read bytes on success, or an error code otherwise.
   */

This sentence sounds a little bit off to me. Shouldn't be "number of bytes read"
instead? I'm not a native English speaker though, so feel free to just ignore 
me.

You're right.


[...]

+static ssize_t fb_read_screen_base(struct fb_info *info, char __user *buf, size_t count,
+                                  loff_t pos)
+{
+       const char __iomem *src = info->screen_base + pos;
+       size_t alloc_size = min_t(size_t, count, PAGE_SIZE);
+       ssize_t ret = 0;
+       int err = 0;

Do you really need these two? AFAIK ssize_t is a signed type

I think so. We'll go through the while loop multiple times. If we fail on the initial iteration, we return the error in err. If we fail on any later iteration, we return the number of processed bytes. Having this in two variables simplifies the logic AFAICT.

Best regards
Thomas

so you can just use the ret variable to store and return the
errno value.

[...]

+static ssize_t fb_write_screen_base(struct fb_info *info, const char __user 
*buf, size_t count,
+                                   loff_t pos)
+{
+       char __iomem *dst = info->screen_base + pos;
+       size_t alloc_size = min_t(size_t, count, PAGE_SIZE);
+       ssize_t ret = 0;
+       int err = 0;

Same here.


--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

Attachment: OpenPGP_signature
Description: OpenPGP digital signature


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.