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