| Message ID | 20201020122046.31167-1-tzimmermann@suse.de |
|---|---|
| Headers | show |
| Series | Support GEM object mappings from I/O memory | expand |
On Tue, Oct 20, 2020 at 02:20:46PM +0200, Thomas Zimmermann wrote: > At least sparc64 requires I/O-specific access to framebuffers. This > patch updates the fbdev console accordingly. > > For drivers with direct access to the framebuffer memory, the callback > functions in struct fb_ops test for the type of memory and call the rsp > fb_sys_ of fb_cfb_ functions. Read and write operations are implemented > internally by DRM's fbdev helper. > > For drivers that employ a shadow buffer, fbdev's blit function retrieves > the framebuffer address as struct dma_buf_map, and uses dma_buf_map > interfaces to access the buffer. > > The bochs driver on sparc64 uses a workaround to flag the framebuffer as > I/O memory and avoid a HW exception. With the introduction of struct > dma_buf_map, this is not required any longer. The patch removes the rsp > code from both, bochs and fbdev. > > v5: > * implement fb_read/fb_write internally (Daniel, Sam) > v4: > * move dma_buf_map changes into separate patch (Daniel) > * TODO list: comment on fbdev updates (Daniel) > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > Tested-by: Sam Ravnborg <sam@ravnborg.org> > --- > Documentation/gpu/todo.rst | 19 ++- > drivers/gpu/drm/bochs/bochs_kms.c | 1 - > drivers/gpu/drm/drm_fb_helper.c | 227 ++++++++++++++++++++++++++++-- > include/drm/drm_mode_config.h | 12 -- > 4 files changed, 230 insertions(+), 29 deletions(-) > > diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst > index 7e6fc3c04add..638b7f704339 100644 > --- a/Documentation/gpu/todo.rst > +++ b/Documentation/gpu/todo.rst > @@ -197,13 +197,28 @@ Convert drivers to use drm_fbdev_generic_setup() > ------------------------------------------------ > > Most drivers can use drm_fbdev_generic_setup(). Driver have to implement > -atomic modesetting and GEM vmap support. Current generic fbdev emulation > -expects the framebuffer in system memory (or system-like memory). > +atomic modesetting and GEM vmap support. Historically, generic fbdev emulation > +expected the framebuffer in system memory or system-like memory. By employing > +struct dma_buf_map, drivers with frambuffers in I/O memory can be supported > +as well. > > Contact: Maintainer of the driver you plan to convert > > Level: Intermediate > > +Reimplement functions in drm_fbdev_fb_ops without fbdev > +------------------------------------------------------- > + > +A number of callback functions in drm_fbdev_fb_ops could benefit from > +being rewritten without dependencies on the fbdev module. Some of the > +helpers could further benefit from using struct dma_buf_map instead of > +raw pointers. > + > +Contact: Thomas Zimmermann <tzimmermann@suse.de>, Daniel Vetter > + > +Level: Advanced > + > + > drm_framebuffer_funcs and drm_mode_config_funcs.fb_create cleanup > ----------------------------------------------------------------- > > diff --git a/drivers/gpu/drm/bochs/bochs_kms.c b/drivers/gpu/drm/bochs/bochs_kms.c > index 13d0d04c4457..853081d186d5 100644 > --- a/drivers/gpu/drm/bochs/bochs_kms.c > +++ b/drivers/gpu/drm/bochs/bochs_kms.c > @@ -151,7 +151,6 @@ int bochs_kms_init(struct bochs_device *bochs) > bochs->dev->mode_config.preferred_depth = 24; > bochs->dev->mode_config.prefer_shadow = 0; > bochs->dev->mode_config.prefer_shadow_fbdev = 1; > - bochs->dev->mode_config.fbdev_use_iomem = true; > bochs->dev->mode_config.quirk_addfb_prefer_host_byte_order = true; > > bochs->dev->mode_config.funcs = &bochs_mode_funcs; > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > index 6212cd7cde1d..1d3180841778 100644 > --- a/drivers/gpu/drm/drm_fb_helper.c > +++ b/drivers/gpu/drm/drm_fb_helper.c > @@ -372,24 +372,22 @@ static void drm_fb_helper_resume_worker(struct work_struct *work) > } > > static void drm_fb_helper_dirty_blit_real(struct drm_fb_helper *fb_helper, > - struct drm_clip_rect *clip) > + struct drm_clip_rect *clip, > + struct dma_buf_map *dst) > { > struct drm_framebuffer *fb = fb_helper->fb; > unsigned int cpp = fb->format->cpp[0]; > size_t offset = clip->y1 * fb->pitches[0] + clip->x1 * cpp; > void *src = fb_helper->fbdev->screen_buffer + offset; > - void *dst = fb_helper->buffer->map.vaddr + offset; > size_t len = (clip->x2 - clip->x1) * cpp; > unsigned int y; > > - for (y = clip->y1; y < clip->y2; y++) { > - if (!fb_helper->dev->mode_config.fbdev_use_iomem) > - memcpy(dst, src, len); > - else > - memcpy_toio((void __iomem *)dst, src, len); > + dma_buf_map_incr(dst, offset); /* go to first pixel within clip rect */ > > + for (y = clip->y1; y < clip->y2; y++) { > + dma_buf_map_memcpy_to(dst, src, len); > + dma_buf_map_incr(dst, fb->pitches[0]); > src += fb->pitches[0]; > - dst += fb->pitches[0]; > } > } > > @@ -417,8 +415,9 @@ static void drm_fb_helper_dirty_work(struct work_struct *work) > ret = drm_client_buffer_vmap(helper->buffer, &map); > if (ret) > return; > - drm_fb_helper_dirty_blit_real(helper, &clip_copy); > + drm_fb_helper_dirty_blit_real(helper, &clip_copy, &map); > } > + > if (helper->fb->funcs->dirty) > helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, > &clip_copy, 1); > @@ -2027,6 +2026,206 @@ static int drm_fbdev_fb_mmap(struct fb_info *info, struct vm_area_struct *vma) > return -ENODEV; > } > > +static bool drm_fbdev_use_iomem(struct fb_info *info) > +{ > + struct drm_fb_helper *fb_helper = info->par; > + struct drm_client_buffer *buffer = fb_helper->buffer; > + > + return !drm_fbdev_use_shadow_fb(fb_helper) && buffer->map.is_iomem; > +} > + > +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; Maybe a bit much a bikeshed, but I'd write this in terms of drm objects, like the dirty_blit function, using the dma_buf_map (instead of the fb_info parameter). And then instead of screen_base and screen_buffer suffixes give them _mem and _iomem suffixes. Same for write below. Or I'm not quite understanding why we do it like this here - I don't think this code will be used outside of the generic fbdev code, so we can always assume that drm_fb_helper->buffer is set up. The other thing I think we need is some minimal testcases to make sure. The fbtest tool used way back seems to have disappeared, I couldn't find a copy of the source anywhere anymore. With all that: Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> Cheers, Daniel > + size_t alloc_size = min(count, PAGE_SIZE); > + ssize_t ret = 0; > + char *tmp; > + > + tmp = kmalloc(alloc_size, GFP_KERNEL); > + if (!tmp) > + return -ENOMEM; > + > + while (count) { > + size_t c = min(count, alloc_size); > + > + memcpy_fromio(tmp, src, c); > + if (copy_to_user(buf, tmp, c)) { > + ret = -EFAULT; > + break; > + } > + > + src += c; > + buf += c; > + ret += c; > + count -= c; > + } > + > + kfree(tmp); > + > + return ret; > +} > + > +static ssize_t fb_read_screen_buffer(struct fb_info *info, char __user *buf, size_t count, > + loff_t pos) > +{ > + const char *src = info->screen_buffer + pos; > + > + if (copy_to_user(buf, src, count)) > + return -EFAULT; > + > + return count; > +} > + > +static ssize_t drm_fbdev_fb_read(struct fb_info *info, char __user *buf, > + size_t count, loff_t *ppos) > +{ > + loff_t pos = *ppos; > + size_t total_size; > + ssize_t ret; > + > + if (info->state != FBINFO_STATE_RUNNING) > + return -EPERM; > + > + if (info->screen_size) > + total_size = info->screen_size; > + else > + total_size = info->fix.smem_len; > + > + if (pos >= total_size) > + return 0; > + if (count >= total_size) > + count = total_size; > + if (total_size - count < pos) > + count = total_size - pos; > + > + if (drm_fbdev_use_iomem(info)) > + ret = fb_read_screen_base(info, buf, count, pos); > + else > + ret = fb_read_screen_buffer(info, buf, count, pos); > + > + if (ret > 0) > + *ppos = ret; > + > + return ret; > +} > + > +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(count, PAGE_SIZE); > + ssize_t ret = 0; > + u8 *tmp; > + > + tmp = kmalloc(alloc_size, GFP_KERNEL); > + if (!tmp) > + return -ENOMEM; > + > + while (count) { > + size_t c = min(count, alloc_size); > + > + if (copy_from_user(tmp, buf, c)) { > + ret = -EFAULT; > + break; > + } > + memcpy_toio(dst, tmp, c); > + > + dst += c; > + buf += c; > + ret += c; > + count -= c; > + } > + > + kfree(tmp); > + > + return ret; > +} > + > +static ssize_t fb_write_screen_buffer(struct fb_info *info, const char __user *buf, size_t count, > + loff_t pos) > +{ > + char *dst = info->screen_buffer + pos; > + > + if (copy_from_user(dst, buf, count)) > + return -EFAULT; > + > + return count; > +} > + > +static ssize_t drm_fbdev_fb_write(struct fb_info *info, const char __user *buf, > + size_t count, loff_t *ppos) > +{ > + loff_t pos = *ppos; > + size_t total_size; > + ssize_t ret; > + int err; > + > + if (info->state != FBINFO_STATE_RUNNING) > + return -EPERM; > + > + if (info->screen_size) > + total_size = info->screen_size; > + else > + total_size = info->fix.smem_len; > + > + if (pos > total_size) > + return -EFBIG; > + if (count > total_size) { > + err = -EFBIG; > + count = total_size; > + } > + if (total_size - count < pos) { > + if (!err) > + err = -ENOSPC; > + count = total_size - pos; > + } > + > + /* > + * Copy to framebuffer even if we already logged an error. Emulates > + * the behavior of the original fbdev implementation. > + */ > + if (drm_fbdev_use_iomem(info)) > + ret = fb_write_screen_base(info, buf, count, pos); > + else > + ret = fb_write_screen_buffer(info, buf, count, pos); > + > + if (ret > 0) > + *ppos = ret; > + > + if (err) > + return err; > + > + return ret; > +} > + > +static void drm_fbdev_fb_fillrect(struct fb_info *info, > + const struct fb_fillrect *rect) > +{ > + if (drm_fbdev_use_iomem(info)) > + drm_fb_helper_cfb_fillrect(info, rect); > + else > + drm_fb_helper_sys_fillrect(info, rect); > +} > + > +static void drm_fbdev_fb_copyarea(struct fb_info *info, > + const struct fb_copyarea *area) > +{ > + if (drm_fbdev_use_iomem(info)) > + drm_fb_helper_cfb_copyarea(info, area); > + else > + drm_fb_helper_sys_copyarea(info, area); > +} > + > +static void drm_fbdev_fb_imageblit(struct fb_info *info, > + const struct fb_image *image) > +{ > + if (drm_fbdev_use_iomem(info)) > + drm_fb_helper_cfb_imageblit(info, image); > + else > + drm_fb_helper_sys_imageblit(info, image); > +} > + > static const struct fb_ops drm_fbdev_fb_ops = { > .owner = THIS_MODULE, > DRM_FB_HELPER_DEFAULT_OPS, > @@ -2034,11 +2233,11 @@ static const struct fb_ops drm_fbdev_fb_ops = { > .fb_release = drm_fbdev_fb_release, > .fb_destroy = drm_fbdev_fb_destroy, > .fb_mmap = drm_fbdev_fb_mmap, > - .fb_read = drm_fb_helper_sys_read, > - .fb_write = drm_fb_helper_sys_write, > - .fb_fillrect = drm_fb_helper_sys_fillrect, > - .fb_copyarea = drm_fb_helper_sys_copyarea, > - .fb_imageblit = drm_fb_helper_sys_imageblit, > + .fb_read = drm_fbdev_fb_read, > + .fb_write = drm_fbdev_fb_write, > + .fb_fillrect = drm_fbdev_fb_fillrect, > + .fb_copyarea = drm_fbdev_fb_copyarea, > + .fb_imageblit = drm_fbdev_fb_imageblit, > }; > > static struct fb_deferred_io drm_fbdev_defio = { > diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h > index 5ffbb4ed5b35..ab424ddd7665 100644 > --- a/include/drm/drm_mode_config.h > +++ b/include/drm/drm_mode_config.h > @@ -877,18 +877,6 @@ struct drm_mode_config { > */ > bool prefer_shadow_fbdev; > > - /** > - * @fbdev_use_iomem: > - * > - * Set to true if framebuffer reside in iomem. > - * When set to true memcpy_toio() is used when copying the framebuffer in > - * drm_fb_helper.drm_fb_helper_dirty_blit_real(). > - * > - * FIXME: This should be replaced with a per-mapping is_iomem > - * flag (like ttm does), and then used everywhere in fbdev code. > - */ > - bool fbdev_use_iomem; > - > /** > * @quirk_addfb_prefer_xbgr_30bpp: > * > -- > 2.28.0 >
Hi On 22.10.20 10:05, Daniel Vetter wrote: > On Tue, Oct 20, 2020 at 02:20:46PM +0200, Thomas Zimmermann wrote: >> At least sparc64 requires I/O-specific access to framebuffers. This >> patch updates the fbdev console accordingly. >> >> For drivers with direct access to the framebuffer memory, the callback >> functions in struct fb_ops test for the type of memory and call the rsp >> fb_sys_ of fb_cfb_ functions. Read and write operations are implemented >> internally by DRM's fbdev helper. >> >> For drivers that employ a shadow buffer, fbdev's blit function retrieves >> the framebuffer address as struct dma_buf_map, and uses dma_buf_map >> interfaces to access the buffer. >> >> The bochs driver on sparc64 uses a workaround to flag the framebuffer as >> I/O memory and avoid a HW exception. With the introduction of struct >> dma_buf_map, this is not required any longer. The patch removes the rsp >> code from both, bochs and fbdev. >> >> v5: >> * implement fb_read/fb_write internally (Daniel, Sam) >> v4: >> * move dma_buf_map changes into separate patch (Daniel) >> * TODO list: comment on fbdev updates (Daniel) >> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >> Tested-by: Sam Ravnborg <sam@ravnborg.org> >> --- >> Documentation/gpu/todo.rst | 19 ++- >> drivers/gpu/drm/bochs/bochs_kms.c | 1 - >> drivers/gpu/drm/drm_fb_helper.c | 227 ++++++++++++++++++++++++++++-- >> include/drm/drm_mode_config.h | 12 -- >> 4 files changed, 230 insertions(+), 29 deletions(-) >> >> diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst >> index 7e6fc3c04add..638b7f704339 100644 >> --- a/Documentation/gpu/todo.rst >> +++ b/Documentation/gpu/todo.rst >> @@ -197,13 +197,28 @@ Convert drivers to use drm_fbdev_generic_setup() >> ------------------------------------------------ >> >> Most drivers can use drm_fbdev_generic_setup(). Driver have to implement >> -atomic modesetting and GEM vmap support. Current generic fbdev emulation >> -expects the framebuffer in system memory (or system-like memory). >> +atomic modesetting and GEM vmap support. Historically, generic fbdev emulation >> +expected the framebuffer in system memory or system-like memory. By employing >> +struct dma_buf_map, drivers with frambuffers in I/O memory can be supported >> +as well. >> >> Contact: Maintainer of the driver you plan to convert >> >> Level: Intermediate >> >> +Reimplement functions in drm_fbdev_fb_ops without fbdev >> +------------------------------------------------------- >> + >> +A number of callback functions in drm_fbdev_fb_ops could benefit from >> +being rewritten without dependencies on the fbdev module. Some of the >> +helpers could further benefit from using struct dma_buf_map instead of >> +raw pointers. >> + >> +Contact: Thomas Zimmermann <tzimmermann@suse.de>, Daniel Vetter >> + >> +Level: Advanced >> + >> + >> drm_framebuffer_funcs and drm_mode_config_funcs.fb_create cleanup >> ----------------------------------------------------------------- >> >> diff --git a/drivers/gpu/drm/bochs/bochs_kms.c b/drivers/gpu/drm/bochs/bochs_kms.c >> index 13d0d04c4457..853081d186d5 100644 >> --- a/drivers/gpu/drm/bochs/bochs_kms.c >> +++ b/drivers/gpu/drm/bochs/bochs_kms.c >> @@ -151,7 +151,6 @@ int bochs_kms_init(struct bochs_device *bochs) >> bochs->dev->mode_config.preferred_depth = 24; >> bochs->dev->mode_config.prefer_shadow = 0; >> bochs->dev->mode_config.prefer_shadow_fbdev = 1; >> - bochs->dev->mode_config.fbdev_use_iomem = true; >> bochs->dev->mode_config.quirk_addfb_prefer_host_byte_order = true; >> >> bochs->dev->mode_config.funcs = &bochs_mode_funcs; >> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c >> index 6212cd7cde1d..1d3180841778 100644 >> --- a/drivers/gpu/drm/drm_fb_helper.c >> +++ b/drivers/gpu/drm/drm_fb_helper.c >> @@ -372,24 +372,22 @@ static void drm_fb_helper_resume_worker(struct work_struct *work) >> } >> >> static void drm_fb_helper_dirty_blit_real(struct drm_fb_helper *fb_helper, >> - struct drm_clip_rect *clip) >> + struct drm_clip_rect *clip, >> + struct dma_buf_map *dst) >> { >> struct drm_framebuffer *fb = fb_helper->fb; >> unsigned int cpp = fb->format->cpp[0]; >> size_t offset = clip->y1 * fb->pitches[0] + clip->x1 * cpp; >> void *src = fb_helper->fbdev->screen_buffer + offset; >> - void *dst = fb_helper->buffer->map.vaddr + offset; >> size_t len = (clip->x2 - clip->x1) * cpp; >> unsigned int y; >> >> - for (y = clip->y1; y < clip->y2; y++) { >> - if (!fb_helper->dev->mode_config.fbdev_use_iomem) >> - memcpy(dst, src, len); >> - else >> - memcpy_toio((void __iomem *)dst, src, len); >> + dma_buf_map_incr(dst, offset); /* go to first pixel within clip rect */ >> >> + for (y = clip->y1; y < clip->y2; y++) { >> + dma_buf_map_memcpy_to(dst, src, len); >> + dma_buf_map_incr(dst, fb->pitches[0]); >> src += fb->pitches[0]; >> - dst += fb->pitches[0]; >> } >> } >> >> @@ -417,8 +415,9 @@ static void drm_fb_helper_dirty_work(struct work_struct *work) >> ret = drm_client_buffer_vmap(helper->buffer, &map); >> if (ret) >> return; >> - drm_fb_helper_dirty_blit_real(helper, &clip_copy); >> + drm_fb_helper_dirty_blit_real(helper, &clip_copy, &map); >> } >> + >> if (helper->fb->funcs->dirty) >> helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, >> &clip_copy, 1); >> @@ -2027,6 +2026,206 @@ static int drm_fbdev_fb_mmap(struct fb_info *info, struct vm_area_struct *vma) >> return -ENODEV; >> } >> >> +static bool drm_fbdev_use_iomem(struct fb_info *info) >> +{ >> + struct drm_fb_helper *fb_helper = info->par; >> + struct drm_client_buffer *buffer = fb_helper->buffer; >> + >> + return !drm_fbdev_use_shadow_fb(fb_helper) && buffer->map.is_iomem; >> +} >> + >> +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; > > Maybe a bit much a bikeshed, but I'd write this in terms of drm objects, > like the dirty_blit function, using the dma_buf_map (instead of the > fb_info parameter). And then instead of > screen_base and screen_buffer suffixes give them _mem and _iomem suffixes. Screen_buffer can be a shadow buffer. Until the blit worker (see drm_fb_helper_dirty_work() ) completes, it might be more up to date than the real buffer that's stored in the client. The orignal fbdev code supported an fb_sync callback to synchronize with outstanding screen updates (e.g., HW blit ops), but fb_sync is just overhead here. Copying from screen_buffer or screen_base always returns the most up-to-date image. > > Same for write below. Or I'm not quite understanding why we do it like > this here - I don't think this code will be used outside of the generic > fbdev code, so we can always assume that drm_fb_helper->buffer is set up. It's similar as in the read case. If we write to the client's buffer, an outstanding blit worker could write the now-outdated shadow buffer over the user's newly written framebuffer data. Thinking about it, we might want to schedule the blit worker at the end of each fb_write, so that the data makes it into the HW buffer in time. > > The other thing I think we need is some minimal testcases to make sure. > The fbtest tool used way back seems to have disappeared, I couldn't find > a copy of the source anywhere anymore. As discussed on IRC, I'll add some testcase to the igt test. I'll share the link here when done. Best regards Thomas > > With all that: Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > Cheers, Daniel > >> + size_t alloc_size = min(count, PAGE_SIZE); >> + ssize_t ret = 0; >> + char *tmp; >> + >> + tmp = kmalloc(alloc_size, GFP_KERNEL); >> + if (!tmp) >> + return -ENOMEM; >> + >> + while (count) { >> + size_t c = min(count, alloc_size); >> + >> + memcpy_fromio(tmp, src, c); >> + if (copy_to_user(buf, tmp, c)) { >> + ret = -EFAULT; >> + break; >> + } >> + >> + src += c; >> + buf += c; >> + ret += c; >> + count -= c; >> + } >> + >> + kfree(tmp); >> + >> + return ret; >> +} >> + >> +static ssize_t fb_read_screen_buffer(struct fb_info *info, char __user *buf, size_t count, >> + loff_t pos) >> +{ >> + const char *src = info->screen_buffer + pos; >> + >> + if (copy_to_user(buf, src, count)) >> + return -EFAULT; >> + >> + return count; >> +} >> + >> +static ssize_t drm_fbdev_fb_read(struct fb_info *info, char __user *buf, >> + size_t count, loff_t *ppos) >> +{ >> + loff_t pos = *ppos; >> + size_t total_size; >> + ssize_t ret; >> + >> + if (info->state != FBINFO_STATE_RUNNING) >> + return -EPERM; >> + >> + if (info->screen_size) >> + total_size = info->screen_size; >> + else >> + total_size = info->fix.smem_len; >> + >> + if (pos >= total_size) >> + return 0; >> + if (count >= total_size) >> + count = total_size; >> + if (total_size - count < pos) >> + count = total_size - pos; >> + >> + if (drm_fbdev_use_iomem(info)) >> + ret = fb_read_screen_base(info, buf, count, pos); >> + else >> + ret = fb_read_screen_buffer(info, buf, count, pos); >> + >> + if (ret > 0) >> + *ppos = ret; >> + >> + return ret; >> +} >> + >> +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(count, PAGE_SIZE); >> + ssize_t ret = 0; >> + u8 *tmp; >> + >> + tmp = kmalloc(alloc_size, GFP_KERNEL); >> + if (!tmp) >> + return -ENOMEM; >> + >> + while (count) { >> + size_t c = min(count, alloc_size); >> + >> + if (copy_from_user(tmp, buf, c)) { >> + ret = -EFAULT; >> + break; >> + } >> + memcpy_toio(dst, tmp, c); >> + >> + dst += c; >> + buf += c; >> + ret += c; >> + count -= c; >> + } >> + >> + kfree(tmp); >> + >> + return ret; >> +} >> + >> +static ssize_t fb_write_screen_buffer(struct fb_info *info, const char __user *buf, size_t count, >> + loff_t pos) >> +{ >> + char *dst = info->screen_buffer + pos; >> + >> + if (copy_from_user(dst, buf, count)) >> + return -EFAULT; >> + >> + return count; >> +} >> + >> +static ssize_t drm_fbdev_fb_write(struct fb_info *info, const char __user *buf, >> + size_t count, loff_t *ppos) >> +{ >> + loff_t pos = *ppos; >> + size_t total_size; >> + ssize_t ret; >> + int err; >> + >> + if (info->state != FBINFO_STATE_RUNNING) >> + return -EPERM; >> + >> + if (info->screen_size) >> + total_size = info->screen_size; >> + else >> + total_size = info->fix.smem_len; >> + >> + if (pos > total_size) >> + return -EFBIG; >> + if (count > total_size) { >> + err = -EFBIG; >> + count = total_size; >> + } >> + if (total_size - count < pos) { >> + if (!err) >> + err = -ENOSPC; >> + count = total_size - pos; >> + } >> + >> + /* >> + * Copy to framebuffer even if we already logged an error. Emulates >> + * the behavior of the original fbdev implementation. >> + */ >> + if (drm_fbdev_use_iomem(info)) >> + ret = fb_write_screen_base(info, buf, count, pos); >> + else >> + ret = fb_write_screen_buffer(info, buf, count, pos); >> + >> + if (ret > 0) >> + *ppos = ret; >> + >> + if (err) >> + return err; >> + >> + return ret; >> +} >> + >> +static void drm_fbdev_fb_fillrect(struct fb_info *info, >> + const struct fb_fillrect *rect) >> +{ >> + if (drm_fbdev_use_iomem(info)) >> + drm_fb_helper_cfb_fillrect(info, rect); >> + else >> + drm_fb_helper_sys_fillrect(info, rect); >> +} >> + >> +static void drm_fbdev_fb_copyarea(struct fb_info *info, >> + const struct fb_copyarea *area) >> +{ >> + if (drm_fbdev_use_iomem(info)) >> + drm_fb_helper_cfb_copyarea(info, area); >> + else >> + drm_fb_helper_sys_copyarea(info, area); >> +} >> + >> +static void drm_fbdev_fb_imageblit(struct fb_info *info, >> + const struct fb_image *image) >> +{ >> + if (drm_fbdev_use_iomem(info)) >> + drm_fb_helper_cfb_imageblit(info, image); >> + else >> + drm_fb_helper_sys_imageblit(info, image); >> +} >> + >> static const struct fb_ops drm_fbdev_fb_ops = { >> .owner = THIS_MODULE, >> DRM_FB_HELPER_DEFAULT_OPS, >> @@ -2034,11 +2233,11 @@ static const struct fb_ops drm_fbdev_fb_ops = { >> .fb_release = drm_fbdev_fb_release, >> .fb_destroy = drm_fbdev_fb_destroy, >> .fb_mmap = drm_fbdev_fb_mmap, >> - .fb_read = drm_fb_helper_sys_read, >> - .fb_write = drm_fb_helper_sys_write, >> - .fb_fillrect = drm_fb_helper_sys_fillrect, >> - .fb_copyarea = drm_fb_helper_sys_copyarea, >> - .fb_imageblit = drm_fb_helper_sys_imageblit, >> + .fb_read = drm_fbdev_fb_read, >> + .fb_write = drm_fbdev_fb_write, >> + .fb_fillrect = drm_fbdev_fb_fillrect, >> + .fb_copyarea = drm_fbdev_fb_copyarea, >> + .fb_imageblit = drm_fbdev_fb_imageblit, >> }; >> >> static struct fb_deferred_io drm_fbdev_defio = { >> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h >> index 5ffbb4ed5b35..ab424ddd7665 100644 >> --- a/include/drm/drm_mode_config.h >> +++ b/include/drm/drm_mode_config.h >> @@ -877,18 +877,6 @@ struct drm_mode_config { >> */ >> bool prefer_shadow_fbdev; >> >> - /** >> - * @fbdev_use_iomem: >> - * >> - * Set to true if framebuffer reside in iomem. >> - * When set to true memcpy_toio() is used when copying the framebuffer in >> - * drm_fb_helper.drm_fb_helper_dirty_blit_real(). >> - * >> - * FIXME: This should be replaced with a per-mapping is_iomem >> - * flag (like ttm does), and then used everywhere in fbdev code. >> - */ >> - bool fbdev_use_iomem; >> - >> /** >> * @quirk_addfb_prefer_xbgr_30bpp: >> * >> -- >> 2.28.0 >> >
On Thu, Oct 22, 2020 at 10:37:56AM +0200, Thomas Zimmermann wrote: > Hi > > On 22.10.20 10:05, Daniel Vetter wrote: > > On Tue, Oct 20, 2020 at 02:20:46PM +0200, Thomas Zimmermann wrote: > >> At least sparc64 requires I/O-specific access to framebuffers. This > >> patch updates the fbdev console accordingly. > >> > >> For drivers with direct access to the framebuffer memory, the callback > >> functions in struct fb_ops test for the type of memory and call the rsp > >> fb_sys_ of fb_cfb_ functions. Read and write operations are implemented > >> internally by DRM's fbdev helper. > >> > >> For drivers that employ a shadow buffer, fbdev's blit function retrieves > >> the framebuffer address as struct dma_buf_map, and uses dma_buf_map > >> interfaces to access the buffer. > >> > >> The bochs driver on sparc64 uses a workaround to flag the framebuffer as > >> I/O memory and avoid a HW exception. With the introduction of struct > >> dma_buf_map, this is not required any longer. The patch removes the rsp > >> code from both, bochs and fbdev. > >> > >> v5: > >> * implement fb_read/fb_write internally (Daniel, Sam) > >> v4: > >> * move dma_buf_map changes into separate patch (Daniel) > >> * TODO list: comment on fbdev updates (Daniel) > >> > >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > >> Tested-by: Sam Ravnborg <sam@ravnborg.org> > >> --- > >> Documentation/gpu/todo.rst | 19 ++- > >> drivers/gpu/drm/bochs/bochs_kms.c | 1 - > >> drivers/gpu/drm/drm_fb_helper.c | 227 ++++++++++++++++++++++++++++-- > >> include/drm/drm_mode_config.h | 12 -- > >> 4 files changed, 230 insertions(+), 29 deletions(-) > >> > >> diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst > >> index 7e6fc3c04add..638b7f704339 100644 > >> --- a/Documentation/gpu/todo.rst > >> +++ b/Documentation/gpu/todo.rst > >> @@ -197,13 +197,28 @@ Convert drivers to use drm_fbdev_generic_setup() > >> ------------------------------------------------ > >> > >> Most drivers can use drm_fbdev_generic_setup(). Driver have to implement > >> -atomic modesetting and GEM vmap support. Current generic fbdev emulation > >> -expects the framebuffer in system memory (or system-like memory). > >> +atomic modesetting and GEM vmap support. Historically, generic fbdev emulation > >> +expected the framebuffer in system memory or system-like memory. By employing > >> +struct dma_buf_map, drivers with frambuffers in I/O memory can be supported > >> +as well. > >> > >> Contact: Maintainer of the driver you plan to convert > >> > >> Level: Intermediate > >> > >> +Reimplement functions in drm_fbdev_fb_ops without fbdev > >> +------------------------------------------------------- > >> + > >> +A number of callback functions in drm_fbdev_fb_ops could benefit from > >> +being rewritten without dependencies on the fbdev module. Some of the > >> +helpers could further benefit from using struct dma_buf_map instead of > >> +raw pointers. > >> + > >> +Contact: Thomas Zimmermann <tzimmermann@suse.de>, Daniel Vetter > >> + > >> +Level: Advanced > >> + > >> + > >> drm_framebuffer_funcs and drm_mode_config_funcs.fb_create cleanup > >> ----------------------------------------------------------------- > >> > >> diff --git a/drivers/gpu/drm/bochs/bochs_kms.c b/drivers/gpu/drm/bochs/bochs_kms.c > >> index 13d0d04c4457..853081d186d5 100644 > >> --- a/drivers/gpu/drm/bochs/bochs_kms.c > >> +++ b/drivers/gpu/drm/bochs/bochs_kms.c > >> @@ -151,7 +151,6 @@ int bochs_kms_init(struct bochs_device *bochs) > >> bochs->dev->mode_config.preferred_depth = 24; > >> bochs->dev->mode_config.prefer_shadow = 0; > >> bochs->dev->mode_config.prefer_shadow_fbdev = 1; > >> - bochs->dev->mode_config.fbdev_use_iomem = true; > >> bochs->dev->mode_config.quirk_addfb_prefer_host_byte_order = true; > >> > >> bochs->dev->mode_config.funcs = &bochs_mode_funcs; > >> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > >> index 6212cd7cde1d..1d3180841778 100644 > >> --- a/drivers/gpu/drm/drm_fb_helper.c > >> +++ b/drivers/gpu/drm/drm_fb_helper.c > >> @@ -372,24 +372,22 @@ static void drm_fb_helper_resume_worker(struct work_struct *work) > >> } > >> > >> static void drm_fb_helper_dirty_blit_real(struct drm_fb_helper *fb_helper, > >> - struct drm_clip_rect *clip) > >> + struct drm_clip_rect *clip, > >> + struct dma_buf_map *dst) > >> { > >> struct drm_framebuffer *fb = fb_helper->fb; > >> unsigned int cpp = fb->format->cpp[0]; > >> size_t offset = clip->y1 * fb->pitches[0] + clip->x1 * cpp; > >> void *src = fb_helper->fbdev->screen_buffer + offset; > >> - void *dst = fb_helper->buffer->map.vaddr + offset; > >> size_t len = (clip->x2 - clip->x1) * cpp; > >> unsigned int y; > >> > >> - for (y = clip->y1; y < clip->y2; y++) { > >> - if (!fb_helper->dev->mode_config.fbdev_use_iomem) > >> - memcpy(dst, src, len); > >> - else > >> - memcpy_toio((void __iomem *)dst, src, len); > >> + dma_buf_map_incr(dst, offset); /* go to first pixel within clip rect */ > >> > >> + for (y = clip->y1; y < clip->y2; y++) { > >> + dma_buf_map_memcpy_to(dst, src, len); > >> + dma_buf_map_incr(dst, fb->pitches[0]); > >> src += fb->pitches[0]; > >> - dst += fb->pitches[0]; > >> } > >> } > >> > >> @@ -417,8 +415,9 @@ static void drm_fb_helper_dirty_work(struct work_struct *work) > >> ret = drm_client_buffer_vmap(helper->buffer, &map); > >> if (ret) > >> return; > >> - drm_fb_helper_dirty_blit_real(helper, &clip_copy); > >> + drm_fb_helper_dirty_blit_real(helper, &clip_copy, &map); > >> } > >> + > >> if (helper->fb->funcs->dirty) > >> helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, > >> &clip_copy, 1); > >> @@ -2027,6 +2026,206 @@ static int drm_fbdev_fb_mmap(struct fb_info *info, struct vm_area_struct *vma) > >> return -ENODEV; > >> } > >> > >> +static bool drm_fbdev_use_iomem(struct fb_info *info) > >> +{ > >> + struct drm_fb_helper *fb_helper = info->par; > >> + struct drm_client_buffer *buffer = fb_helper->buffer; > >> + > >> + return !drm_fbdev_use_shadow_fb(fb_helper) && buffer->map.is_iomem; > >> +} > >> + > >> +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; > > > > Maybe a bit much a bikeshed, but I'd write this in terms of drm objects, > > like the dirty_blit function, using the dma_buf_map (instead of the > > fb_info parameter). And then instead of > > screen_base and screen_buffer suffixes give them _mem and _iomem suffixes. > > Screen_buffer can be a shadow buffer. Until the blit worker (see > drm_fb_helper_dirty_work() ) completes, it might be more up to date than > the real buffer that's stored in the client. > > The orignal fbdev code supported an fb_sync callback to synchronize with > outstanding screen updates (e.g., HW blit ops), but fb_sync is just > overhead here. Copying from screen_buffer or screen_base always returns > the most up-to-date image. > > > > > Same for write below. Or I'm not quite understanding why we do it like > > this here - I don't think this code will be used outside of the generic > > fbdev code, so we can always assume that drm_fb_helper->buffer is set up. > > It's similar as in the read case. If we write to the client's buffer, an > outstanding blit worker could write the now-outdated shadow buffer over > the user's newly written framebuffer data. > > Thinking about it, we might want to schedule the blit worker at the end > of each fb_write, so that the data makes it into the HW buffer in time. Hm ok, makes some sense. I think there's some potential for cleanup if we add a dma_buf_map drm_fb_helper->uapi_map which points at the right thing always. That could then also the drm_fbdev_use_iomem() helper and make this all look really neat. But maybe a follow up clean up patch, if you're bored. As-is: Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> While looking at this I also noticed a potential small issue in an earlier patch. > > The other thing I think we need is some minimal testcases to make sure. > > The fbtest tool used way back seems to have disappeared, I couldn't find > > a copy of the source anywhere anymore. > > As discussed on IRC, I'll add some testcase to the igt test. I'll share > the link here when done. > > Best regards > Thomas > > > > > With all that: Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > > > Cheers, Daniel > > > >> + size_t alloc_size = min(count, PAGE_SIZE); > >> + ssize_t ret = 0; > >> + char *tmp; > >> + > >> + tmp = kmalloc(alloc_size, GFP_KERNEL); > >> + if (!tmp) > >> + return -ENOMEM; > >> + > >> + while (count) { > >> + size_t c = min(count, alloc_size); > >> + > >> + memcpy_fromio(tmp, src, c); > >> + if (copy_to_user(buf, tmp, c)) { > >> + ret = -EFAULT; > >> + break; > >> + } > >> + > >> + src += c; > >> + buf += c; > >> + ret += c; > >> + count -= c; > >> + } > >> + > >> + kfree(tmp); > >> + > >> + return ret; > >> +} > >> + > >> +static ssize_t fb_read_screen_buffer(struct fb_info *info, char __user *buf, size_t count, > >> + loff_t pos) > >> +{ > >> + const char *src = info->screen_buffer + pos; > >> + > >> + if (copy_to_user(buf, src, count)) > >> + return -EFAULT; > >> + > >> + return count; > >> +} > >> + > >> +static ssize_t drm_fbdev_fb_read(struct fb_info *info, char __user *buf, > >> + size_t count, loff_t *ppos) > >> +{ > >> + loff_t pos = *ppos; > >> + size_t total_size; > >> + ssize_t ret; > >> + > >> + if (info->state != FBINFO_STATE_RUNNING) > >> + return -EPERM; > >> + > >> + if (info->screen_size) > >> + total_size = info->screen_size; > >> + else > >> + total_size = info->fix.smem_len; > >> + > >> + if (pos >= total_size) > >> + return 0; > >> + if (count >= total_size) > >> + count = total_size; > >> + if (total_size - count < pos) > >> + count = total_size - pos; > >> + > >> + if (drm_fbdev_use_iomem(info)) > >> + ret = fb_read_screen_base(info, buf, count, pos); > >> + else > >> + ret = fb_read_screen_buffer(info, buf, count, pos); > >> + > >> + if (ret > 0) > >> + *ppos = ret; > >> + > >> + return ret; > >> +} > >> + > >> +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(count, PAGE_SIZE); > >> + ssize_t ret = 0; > >> + u8 *tmp; > >> + > >> + tmp = kmalloc(alloc_size, GFP_KERNEL); > >> + if (!tmp) > >> + return -ENOMEM; > >> + > >> + while (count) { > >> + size_t c = min(count, alloc_size); > >> + > >> + if (copy_from_user(tmp, buf, c)) { > >> + ret = -EFAULT; > >> + break; > >> + } > >> + memcpy_toio(dst, tmp, c); > >> + > >> + dst += c; > >> + buf += c; > >> + ret += c; > >> + count -= c; > >> + } > >> + > >> + kfree(tmp); > >> + > >> + return ret; > >> +} > >> + > >> +static ssize_t fb_write_screen_buffer(struct fb_info *info, const char __user *buf, size_t count, > >> + loff_t pos) > >> +{ > >> + char *dst = info->screen_buffer + pos; > >> + > >> + if (copy_from_user(dst, buf, count)) > >> + return -EFAULT; > >> + > >> + return count; > >> +} > >> + > >> +static ssize_t drm_fbdev_fb_write(struct fb_info *info, const char __user *buf, > >> + size_t count, loff_t *ppos) > >> +{ > >> + loff_t pos = *ppos; > >> + size_t total_size; > >> + ssize_t ret; > >> + int err; > >> + > >> + if (info->state != FBINFO_STATE_RUNNING) > >> + return -EPERM; > >> + > >> + if (info->screen_size) > >> + total_size = info->screen_size; > >> + else > >> + total_size = info->fix.smem_len; > >> + > >> + if (pos > total_size) > >> + return -EFBIG; > >> + if (count > total_size) { > >> + err = -EFBIG; > >> + count = total_size; > >> + } > >> + if (total_size - count < pos) { > >> + if (!err) > >> + err = -ENOSPC; > >> + count = total_size - pos; > >> + } > >> + > >> + /* > >> + * Copy to framebuffer even if we already logged an error. Emulates > >> + * the behavior of the original fbdev implementation. > >> + */ > >> + if (drm_fbdev_use_iomem(info)) > >> + ret = fb_write_screen_base(info, buf, count, pos); > >> + else > >> + ret = fb_write_screen_buffer(info, buf, count, pos); > >> + > >> + if (ret > 0) > >> + *ppos = ret; > >> + > >> + if (err) > >> + return err; > >> + > >> + return ret; > >> +} > >> + > >> +static void drm_fbdev_fb_fillrect(struct fb_info *info, > >> + const struct fb_fillrect *rect) > >> +{ > >> + if (drm_fbdev_use_iomem(info)) > >> + drm_fb_helper_cfb_fillrect(info, rect); > >> + else > >> + drm_fb_helper_sys_fillrect(info, rect); > >> +} > >> + > >> +static void drm_fbdev_fb_copyarea(struct fb_info *info, > >> + const struct fb_copyarea *area) > >> +{ > >> + if (drm_fbdev_use_iomem(info)) > >> + drm_fb_helper_cfb_copyarea(info, area); > >> + else > >> + drm_fb_helper_sys_copyarea(info, area); > >> +} > >> + > >> +static void drm_fbdev_fb_imageblit(struct fb_info *info, > >> + const struct fb_image *image) > >> +{ > >> + if (drm_fbdev_use_iomem(info)) > >> + drm_fb_helper_cfb_imageblit(info, image); > >> + else > >> + drm_fb_helper_sys_imageblit(info, image); > >> +} > >> + > >> static const struct fb_ops drm_fbdev_fb_ops = { > >> .owner = THIS_MODULE, > >> DRM_FB_HELPER_DEFAULT_OPS, > >> @@ -2034,11 +2233,11 @@ static const struct fb_ops drm_fbdev_fb_ops = { > >> .fb_release = drm_fbdev_fb_release, > >> .fb_destroy = drm_fbdev_fb_destroy, > >> .fb_mmap = drm_fbdev_fb_mmap, > >> - .fb_read = drm_fb_helper_sys_read, > >> - .fb_write = drm_fb_helper_sys_write, > >> - .fb_fillrect = drm_fb_helper_sys_fillrect, > >> - .fb_copyarea = drm_fb_helper_sys_copyarea, > >> - .fb_imageblit = drm_fb_helper_sys_imageblit, > >> + .fb_read = drm_fbdev_fb_read, > >> + .fb_write = drm_fbdev_fb_write, > >> + .fb_fillrect = drm_fbdev_fb_fillrect, > >> + .fb_copyarea = drm_fbdev_fb_copyarea, > >> + .fb_imageblit = drm_fbdev_fb_imageblit, > >> }; > >> > >> static struct fb_deferred_io drm_fbdev_defio = { > >> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h > >> index 5ffbb4ed5b35..ab424ddd7665 100644 > >> --- a/include/drm/drm_mode_config.h > >> +++ b/include/drm/drm_mode_config.h > >> @@ -877,18 +877,6 @@ struct drm_mode_config { > >> */ > >> bool prefer_shadow_fbdev; > >> > >> - /** > >> - * @fbdev_use_iomem: > >> - * > >> - * Set to true if framebuffer reside in iomem. > >> - * When set to true memcpy_toio() is used when copying the framebuffer in > >> - * drm_fb_helper.drm_fb_helper_dirty_blit_real(). > >> - * > >> - * FIXME: This should be replaced with a per-mapping is_iomem > >> - * flag (like ttm does), and then used everywhere in fbdev code. > >> - */ > >> - bool fbdev_use_iomem; > >> - > >> /** > >> * @quirk_addfb_prefer_xbgr_30bpp: > >> * > >> -- > >> 2.28.0 > >> > > > > -- > Thomas Zimmermann > Graphics Driver Developer > SUSE Software Solutions Germany GmbH > Maxfeldstr. 5, 90409 Nürnberg, Germany > (HRB 36809, AG Nürnberg) > Geschäftsführer: Felix Imendörffer
Hi On 22.10.20 10:49, Daniel Vetter wrote: > On Tue, Oct 20, 2020 at 02:20:44PM +0200, Thomas Zimmermann wrote: >> Kernel DRM clients now store their framebuffer address in an instance >> of struct dma_buf_map. Depending on the buffer's location, the address >> refers to system or I/O memory. >> >> Callers of drm_client_buffer_vmap() receive a copy of the value in >> the call's supplied arguments. It can be accessed and modified with >> dma_buf_map interfaces. >> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> >> Tested-by: Sam Ravnborg <sam@ravnborg.org> >> --- >> drivers/gpu/drm/drm_client.c | 34 +++++++++++++++++++-------------- >> drivers/gpu/drm/drm_fb_helper.c | 23 +++++++++++++--------- >> include/drm/drm_client.h | 7 ++++--- >> 3 files changed, 38 insertions(+), 26 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c >> index ac0082bed966..fe573acf1067 100644 >> --- a/drivers/gpu/drm/drm_client.c >> +++ b/drivers/gpu/drm/drm_client.c >> @@ -235,7 +235,7 @@ static void drm_client_buffer_delete(struct drm_client_buffer *buffer) >> { >> struct drm_device *dev = buffer->client->dev; >> >> - drm_gem_vunmap(buffer->gem, buffer->vaddr); >> + drm_gem_vunmap(buffer->gem, &buffer->map); >> >> if (buffer->gem) >> drm_gem_object_put(buffer->gem); >> @@ -291,25 +291,31 @@ drm_client_buffer_create(struct drm_client_dev *client, u32 width, u32 height, u >> /** >> * drm_client_buffer_vmap - Map DRM client buffer into address space >> * @buffer: DRM client buffer >> + * @map_copy: Returns the mapped memory's address >> * >> * This function maps a client buffer into kernel address space. If the >> - * buffer is already mapped, it returns the mapping's address. >> + * buffer is already mapped, it returns the existing mapping's address. >> * >> * Client buffer mappings are not ref'counted. Each call to >> * drm_client_buffer_vmap() should be followed by a call to >> * drm_client_buffer_vunmap(); or the client buffer should be mapped >> * throughout its lifetime. >> * >> + * The returned address is a copy of the internal value. In contrast to >> + * other vmap interfaces, you don't need it for the client's vunmap >> + * function. So you can modify it at will during blit and draw operations. >> + * >> * Returns: >> - * The mapped memory's address >> + * 0 on success, or a negative errno code otherwise. >> */ >> -void *drm_client_buffer_vmap(struct drm_client_buffer *buffer) >> +int >> +drm_client_buffer_vmap(struct drm_client_buffer *buffer, struct dma_buf_map *map_copy) >> { >> - struct dma_buf_map map; >> + struct dma_buf_map *map = &buffer->map; >> int ret; >> >> - if (buffer->vaddr) >> - return buffer->vaddr; >> + if (dma_buf_map_is_set(map)) >> + goto out; >> >> /* >> * FIXME: The dependency on GEM here isn't required, we could >> @@ -319,13 +325,14 @@ void *drm_client_buffer_vmap(struct drm_client_buffer *buffer) >> * fd_install step out of the driver backend hooks, to make that >> * final step optional for internal users. >> */ >> - ret = drm_gem_vmap(buffer->gem, &map); >> + ret = drm_gem_vmap(buffer->gem, map); >> if (ret) >> - return ERR_PTR(ret); >> + return ret; >> >> - buffer->vaddr = map.vaddr; >> +out: >> + *map_copy = *map; >> >> - return map.vaddr; >> + return 0; >> } >> EXPORT_SYMBOL(drm_client_buffer_vmap); >> >> @@ -339,10 +346,9 @@ EXPORT_SYMBOL(drm_client_buffer_vmap); >> */ >> void drm_client_buffer_vunmap(struct drm_client_buffer *buffer) >> { >> - struct dma_buf_map map = DMA_BUF_MAP_INIT_VADDR(buffer->vaddr); >> + struct dma_buf_map *map = &buffer->map; >> >> - drm_gem_vunmap(buffer->gem, &map); >> - buffer->vaddr = NULL; >> + drm_gem_vunmap(buffer->gem, map); >> } >> EXPORT_SYMBOL(drm_client_buffer_vunmap); >> >> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c >> index c2f72bb6afb1..6212cd7cde1d 100644 >> --- a/drivers/gpu/drm/drm_fb_helper.c >> +++ b/drivers/gpu/drm/drm_fb_helper.c >> @@ -378,7 +378,7 @@ static void drm_fb_helper_dirty_blit_real(struct drm_fb_helper *fb_helper, >> unsigned int cpp = fb->format->cpp[0]; >> size_t offset = clip->y1 * fb->pitches[0] + clip->x1 * cpp; >> void *src = fb_helper->fbdev->screen_buffer + offset; >> - void *dst = fb_helper->buffer->vaddr + offset; >> + void *dst = fb_helper->buffer->map.vaddr + offset; >> size_t len = (clip->x2 - clip->x1) * cpp; >> unsigned int y; >> >> @@ -400,7 +400,8 @@ static void drm_fb_helper_dirty_work(struct work_struct *work) >> struct drm_clip_rect *clip = &helper->dirty_clip; >> struct drm_clip_rect clip_copy; >> unsigned long flags; >> - void *vaddr; >> + struct dma_buf_map map; >> + int ret; >> >> spin_lock_irqsave(&helper->dirty_lock, flags); >> clip_copy = *clip; >> @@ -413,8 +414,8 @@ static void drm_fb_helper_dirty_work(struct work_struct *work) >> >> /* Generic fbdev uses a shadow buffer */ >> if (helper->buffer) { >> - vaddr = drm_client_buffer_vmap(helper->buffer); >> - if (IS_ERR(vaddr)) >> + ret = drm_client_buffer_vmap(helper->buffer, &map); >> + if (ret) >> return; >> drm_fb_helper_dirty_blit_real(helper, &clip_copy); >> } >> @@ -2060,7 +2061,8 @@ static int drm_fb_helper_generic_probe(struct drm_fb_helper *fb_helper, >> struct drm_framebuffer *fb; >> struct fb_info *fbi; >> u32 format; >> - void *vaddr; >> + struct dma_buf_map map; >> + int ret; >> >> drm_dbg_kms(dev, "surface width(%d), height(%d) and bpp(%d)\n", >> sizes->surface_width, sizes->surface_height, >> @@ -2096,11 +2098,14 @@ static int drm_fb_helper_generic_probe(struct drm_fb_helper *fb_helper, >> fb_deferred_io_init(fbi); >> } else { >> /* buffer is mapped for HW framebuffer */ >> - vaddr = drm_client_buffer_vmap(fb_helper->buffer); >> - if (IS_ERR(vaddr)) >> - return PTR_ERR(vaddr); >> + ret = drm_client_buffer_vmap(fb_helper->buffer, &map); >> + if (ret) >> + return ret; >> + if (map.is_iomem) >> + fbi->screen_base = map.vaddr_iomem; >> + else >> + fbi->screen_buffer = map.vaddr; >> >> - fbi->screen_buffer = vaddr; >> /* Shamelessly leak the physical address to user-space */ >> #if IS_ENABLED(CONFIG_DRM_FBDEV_LEAK_PHYS_SMEM) >> if (drm_leak_fbdev_smem && fbi->fix.smem_start == 0) > > Just noticed a tiny thing here: I think this needs to be patched to only > set smem_start when the map is _not_ iomem. Since virt_to_page isn't > defined on iomem at all. > > I guess it'd be neat if we can set this for iomem too, but I have no idea > how to convert an iomem pointer back to a bus_addr_t ... Not that I disagree, but that should be reviewed by the right people. The commit at 4be9bd10e22d ("drm/fb_helper: Allow leaking fbdev smem_start") appears to work around specific userspace drivers. Best regards Thomas > > Cheers, Daniel > >> diff --git a/include/drm/drm_client.h b/include/drm/drm_client.h >> index 7aaea665bfc2..f07f2fb02e75 100644 >> --- a/include/drm/drm_client.h >> +++ b/include/drm/drm_client.h >> @@ -3,6 +3,7 @@ >> #ifndef _DRM_CLIENT_H_ >> #define _DRM_CLIENT_H_ >> >> +#include <linux/dma-buf-map.h> >> #include <linux/lockdep.h> >> #include <linux/mutex.h> >> #include <linux/types.h> >> @@ -141,9 +142,9 @@ struct drm_client_buffer { >> struct drm_gem_object *gem; >> >> /** >> - * @vaddr: Virtual address for the buffer >> + * @map: Virtual address for the buffer >> */ >> - void *vaddr; >> + struct dma_buf_map map; >> >> /** >> * @fb: DRM framebuffer >> @@ -155,7 +156,7 @@ struct drm_client_buffer * >> drm_client_framebuffer_create(struct drm_client_dev *client, u32 width, u32 height, u32 format); >> void drm_client_framebuffer_delete(struct drm_client_buffer *buffer); >> int drm_client_framebuffer_flush(struct drm_client_buffer *buffer, struct drm_rect *rect); >> -void *drm_client_buffer_vmap(struct drm_client_buffer *buffer); >> +int drm_client_buffer_vmap(struct drm_client_buffer *buffer, struct dma_buf_map *map); >> void drm_client_buffer_vunmap(struct drm_client_buffer *buffer); >> >> int drm_client_modeset_create(struct drm_client_dev *client); >> -- >> 2.28.0 >> > -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Felix Imendörffer
On Thu, Oct 22, 2020 at 11:18 AM Thomas Zimmermann <tzimmermann@suse.de> wrote: > > Hi > > On 22.10.20 10:49, Daniel Vetter wrote: > > On Tue, Oct 20, 2020 at 02:20:44PM +0200, Thomas Zimmermann wrote: > >> Kernel DRM clients now store their framebuffer address in an instance > >> of struct dma_buf_map. Depending on the buffer's location, the address > >> refers to system or I/O memory. > >> > >> Callers of drm_client_buffer_vmap() receive a copy of the value in > >> the call's supplied arguments. It can be accessed and modified with > >> dma_buf_map interfaces. > >> > >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > >> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > >> Tested-by: Sam Ravnborg <sam@ravnborg.org> > >> --- > >> drivers/gpu/drm/drm_client.c | 34 +++++++++++++++++++-------------- > >> drivers/gpu/drm/drm_fb_helper.c | 23 +++++++++++++--------- > >> include/drm/drm_client.h | 7 ++++--- > >> 3 files changed, 38 insertions(+), 26 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c > >> index ac0082bed966..fe573acf1067 100644 > >> --- a/drivers/gpu/drm/drm_client.c > >> +++ b/drivers/gpu/drm/drm_client.c > >> @@ -235,7 +235,7 @@ static void drm_client_buffer_delete(struct drm_client_buffer *buffer) > >> { > >> struct drm_device *dev = buffer->client->dev; > >> > >> - drm_gem_vunmap(buffer->gem, buffer->vaddr); > >> + drm_gem_vunmap(buffer->gem, &buffer->map); > >> > >> if (buffer->gem) > >> drm_gem_object_put(buffer->gem); > >> @@ -291,25 +291,31 @@ drm_client_buffer_create(struct drm_client_dev *client, u32 width, u32 height, u > >> /** > >> * drm_client_buffer_vmap - Map DRM client buffer into address space > >> * @buffer: DRM client buffer > >> + * @map_copy: Returns the mapped memory's address > >> * > >> * This function maps a client buffer into kernel address space. If the > >> - * buffer is already mapped, it returns the mapping's address. > >> + * buffer is already mapped, it returns the existing mapping's address. > >> * > >> * Client buffer mappings are not ref'counted. Each call to > >> * drm_client_buffer_vmap() should be followed by a call to > >> * drm_client_buffer_vunmap(); or the client buffer should be mapped > >> * throughout its lifetime. > >> * > >> + * The returned address is a copy of the internal value. In contrast to > >> + * other vmap interfaces, you don't need it for the client's vunmap > >> + * function. So you can modify it at will during blit and draw operations. > >> + * > >> * Returns: > >> - * The mapped memory's address > >> + * 0 on success, or a negative errno code otherwise. > >> */ > >> -void *drm_client_buffer_vmap(struct drm_client_buffer *buffer) > >> +int > >> +drm_client_buffer_vmap(struct drm_client_buffer *buffer, struct dma_buf_map *map_copy) > >> { > >> - struct dma_buf_map map; > >> + struct dma_buf_map *map = &buffer->map; > >> int ret; > >> > >> - if (buffer->vaddr) > >> - return buffer->vaddr; > >> + if (dma_buf_map_is_set(map)) > >> + goto out; > >> > >> /* > >> * FIXME: The dependency on GEM here isn't required, we could > >> @@ -319,13 +325,14 @@ void *drm_client_buffer_vmap(struct drm_client_buffer *buffer) > >> * fd_install step out of the driver backend hooks, to make that > >> * final step optional for internal users. > >> */ > >> - ret = drm_gem_vmap(buffer->gem, &map); > >> + ret = drm_gem_vmap(buffer->gem, map); > >> if (ret) > >> - return ERR_PTR(ret); > >> + return ret; > >> > >> - buffer->vaddr = map.vaddr; > >> +out: > >> + *map_copy = *map; > >> > >> - return map.vaddr; > >> + return 0; > >> } > >> EXPORT_SYMBOL(drm_client_buffer_vmap); > >> > >> @@ -339,10 +346,9 @@ EXPORT_SYMBOL(drm_client_buffer_vmap); > >> */ > >> void drm_client_buffer_vunmap(struct drm_client_buffer *buffer) > >> { > >> - struct dma_buf_map map = DMA_BUF_MAP_INIT_VADDR(buffer->vaddr); > >> + struct dma_buf_map *map = &buffer->map; > >> > >> - drm_gem_vunmap(buffer->gem, &map); > >> - buffer->vaddr = NULL; > >> + drm_gem_vunmap(buffer->gem, map); > >> } > >> EXPORT_SYMBOL(drm_client_buffer_vunmap); > >> > >> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > >> index c2f72bb6afb1..6212cd7cde1d 100644 > >> --- a/drivers/gpu/drm/drm_fb_helper.c > >> +++ b/drivers/gpu/drm/drm_fb_helper.c > >> @@ -378,7 +378,7 @@ static void drm_fb_helper_dirty_blit_real(struct drm_fb_helper *fb_helper, > >> unsigned int cpp = fb->format->cpp[0]; > >> size_t offset = clip->y1 * fb->pitches[0] + clip->x1 * cpp; > >> void *src = fb_helper->fbdev->screen_buffer + offset; > >> - void *dst = fb_helper->buffer->vaddr + offset; > >> + void *dst = fb_helper->buffer->map.vaddr + offset; > >> size_t len = (clip->x2 - clip->x1) * cpp; > >> unsigned int y; > >> > >> @@ -400,7 +400,8 @@ static void drm_fb_helper_dirty_work(struct work_struct *work) > >> struct drm_clip_rect *clip = &helper->dirty_clip; > >> struct drm_clip_rect clip_copy; > >> unsigned long flags; > >> - void *vaddr; > >> + struct dma_buf_map map; > >> + int ret; > >> > >> spin_lock_irqsave(&helper->dirty_lock, flags); > >> clip_copy = *clip; > >> @@ -413,8 +414,8 @@ static void drm_fb_helper_dirty_work(struct work_struct *work) > >> > >> /* Generic fbdev uses a shadow buffer */ > >> if (helper->buffer) { > >> - vaddr = drm_client_buffer_vmap(helper->buffer); > >> - if (IS_ERR(vaddr)) > >> + ret = drm_client_buffer_vmap(helper->buffer, &map); > >> + if (ret) > >> return; > >> drm_fb_helper_dirty_blit_real(helper, &clip_copy); > >> } > >> @@ -2060,7 +2061,8 @@ static int drm_fb_helper_generic_probe(struct drm_fb_helper *fb_helper, > >> struct drm_framebuffer *fb; > >> struct fb_info *fbi; > >> u32 format; > >> - void *vaddr; > >> + struct dma_buf_map map; > >> + int ret; > >> > >> drm_dbg_kms(dev, "surface width(%d), height(%d) and bpp(%d)\n", > >> sizes->surface_width, sizes->surface_height, > >> @@ -2096,11 +2098,14 @@ static int drm_fb_helper_generic_probe(struct drm_fb_helper *fb_helper, > >> fb_deferred_io_init(fbi); > >> } else { > >> /* buffer is mapped for HW framebuffer */ > >> - vaddr = drm_client_buffer_vmap(fb_helper->buffer); > >> - if (IS_ERR(vaddr)) > >> - return PTR_ERR(vaddr); > >> + ret = drm_client_buffer_vmap(fb_helper->buffer, &map); > >> + if (ret) > >> + return ret; > >> + if (map.is_iomem) > >> + fbi->screen_base = map.vaddr_iomem; > >> + else > >> + fbi->screen_buffer = map.vaddr; > >> > >> - fbi->screen_buffer = vaddr; > >> /* Shamelessly leak the physical address to user-space */ > >> #if IS_ENABLED(CONFIG_DRM_FBDEV_LEAK_PHYS_SMEM) > >> if (drm_leak_fbdev_smem && fbi->fix.smem_start == 0) > > > > Just noticed a tiny thing here: I think this needs to be patched to only > > set smem_start when the map is _not_ iomem. Since virt_to_page isn't > > defined on iomem at all. > > > > I guess it'd be neat if we can set this for iomem too, but I have no idea > > how to convert an iomem pointer back to a bus_addr_t ... > > Not that I disagree, but that should be reviewed by the right people. > The commit at 4be9bd10e22d ("drm/fb_helper: Allow leaking fbdev > smem_start") appears to work around specific userspace drivers. It's for soc drivers, which all use either shmem or cma helpers, so all system memory. Which means your patch here doesn't break anything. But we need to make sure that if someone enables this it doesn't blow up at least when used on a device where we map iomem. -Daniel > Best regards > Thomas > > > > > Cheers, Daniel > > > >> diff --git a/include/drm/drm_client.h b/include/drm/drm_client.h > >> index 7aaea665bfc2..f07f2fb02e75 100644 > >> --- a/include/drm/drm_client.h > >> +++ b/include/drm/drm_client.h > >> @@ -3,6 +3,7 @@ > >> #ifndef _DRM_CLIENT_H_ > >> #define _DRM_CLIENT_H_ > >> > >> +#include <linux/dma-buf-map.h> > >> #include <linux/lockdep.h> > >> #include <linux/mutex.h> > >> #include <linux/types.h> > >> @@ -141,9 +142,9 @@ struct drm_client_buffer { > >> struct drm_gem_object *gem; > >> > >> /** > >> - * @vaddr: Virtual address for the buffer > >> + * @map: Virtual address for the buffer > >> */ > >> - void *vaddr; > >> + struct dma_buf_map map; > >> > >> /** > >> * @fb: DRM framebuffer > >> @@ -155,7 +156,7 @@ struct drm_client_buffer * > >> drm_client_framebuffer_create(struct drm_client_dev *client, u32 width, u32 height, u32 format); > >> void drm_client_framebuffer_delete(struct drm_client_buffer *buffer); > >> int drm_client_framebuffer_flush(struct drm_client_buffer *buffer, struct drm_rect *rect); > >> -void *drm_client_buffer_vmap(struct drm_client_buffer *buffer); > >> +int drm_client_buffer_vmap(struct drm_client_buffer *buffer, struct dma_buf_map *map); > >> void drm_client_buffer_vunmap(struct drm_client_buffer *buffer); > >> > >> int drm_client_modeset_create(struct drm_client_dev *client); > >> -- > >> 2.28.0 > >> > > > > -- > Thomas Zimmermann > Graphics Driver Developer > SUSE Software Solutions Germany GmbH > Maxfeldstr. 5, 90409 Nürnberg, Germany > (HRB 36809, AG Nürnberg) > Geschäftsführer: Felix Imendörffer
Hi Thomas. On Tue, Oct 20, 2020 at 02:20:46PM +0200, Thomas Zimmermann wrote: > At least sparc64 requires I/O-specific access to framebuffers. This > patch updates the fbdev console accordingly. > > For drivers with direct access to the framebuffer memory, the callback > functions in struct fb_ops test for the type of memory and call the rsp > fb_sys_ of fb_cfb_ functions. Read and write operations are implemented > internally by DRM's fbdev helper. > > For drivers that employ a shadow buffer, fbdev's blit function retrieves > the framebuffer address as struct dma_buf_map, and uses dma_buf_map > interfaces to access the buffer. > > The bochs driver on sparc64 uses a workaround to flag the framebuffer as > I/O memory and avoid a HW exception. With the introduction of struct > dma_buf_map, this is not required any longer. The patch removes the rsp > code from both, bochs and fbdev. > > v5: > * implement fb_read/fb_write internally (Daniel, Sam) > v4: > * move dma_buf_map changes into separate patch (Daniel) > * TODO list: comment on fbdev updates (Daniel) > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > Tested-by: Sam Ravnborg <sam@ravnborg.org> Reviewed-by: Sam Ravnborg <sam@ravnborg.org> But see a few comments below on naming for you to consider. Sam > --- > Documentation/gpu/todo.rst | 19 ++- > drivers/gpu/drm/bochs/bochs_kms.c | 1 - > drivers/gpu/drm/drm_fb_helper.c | 227 ++++++++++++++++++++++++++++-- > include/drm/drm_mode_config.h | 12 -- > 4 files changed, 230 insertions(+), 29 deletions(-) > > diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst > index 7e6fc3c04add..638b7f704339 100644 > --- a/Documentation/gpu/todo.rst > +++ b/Documentation/gpu/todo.rst > @@ -197,13 +197,28 @@ Convert drivers to use drm_fbdev_generic_setup() > ------------------------------------------------ > > Most drivers can use drm_fbdev_generic_setup(). Driver have to implement > -atomic modesetting and GEM vmap support. Current generic fbdev emulation > -expects the framebuffer in system memory (or system-like memory). > +atomic modesetting and GEM vmap support. Historically, generic fbdev emulation > +expected the framebuffer in system memory or system-like memory. By employing > +struct dma_buf_map, drivers with frambuffers in I/O memory can be supported > +as well. > > Contact: Maintainer of the driver you plan to convert > > Level: Intermediate > > +Reimplement functions in drm_fbdev_fb_ops without fbdev > +------------------------------------------------------- > + > +A number of callback functions in drm_fbdev_fb_ops could benefit from > +being rewritten without dependencies on the fbdev module. Some of the > +helpers could further benefit from using struct dma_buf_map instead of > +raw pointers. > + > +Contact: Thomas Zimmermann <tzimmermann@suse.de>, Daniel Vetter > + > +Level: Advanced > + > + > drm_framebuffer_funcs and drm_mode_config_funcs.fb_create cleanup > ----------------------------------------------------------------- > > diff --git a/drivers/gpu/drm/bochs/bochs_kms.c b/drivers/gpu/drm/bochs/bochs_kms.c > index 13d0d04c4457..853081d186d5 100644 > --- a/drivers/gpu/drm/bochs/bochs_kms.c > +++ b/drivers/gpu/drm/bochs/bochs_kms.c > @@ -151,7 +151,6 @@ int bochs_kms_init(struct bochs_device *bochs) > bochs->dev->mode_config.preferred_depth = 24; > bochs->dev->mode_config.prefer_shadow = 0; > bochs->dev->mode_config.prefer_shadow_fbdev = 1; > - bochs->dev->mode_config.fbdev_use_iomem = true; > bochs->dev->mode_config.quirk_addfb_prefer_host_byte_order = true; > > bochs->dev->mode_config.funcs = &bochs_mode_funcs; > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > index 6212cd7cde1d..1d3180841778 100644 > --- a/drivers/gpu/drm/drm_fb_helper.c > +++ b/drivers/gpu/drm/drm_fb_helper.c > @@ -372,24 +372,22 @@ static void drm_fb_helper_resume_worker(struct work_struct *work) > } > > static void drm_fb_helper_dirty_blit_real(struct drm_fb_helper *fb_helper, > - struct drm_clip_rect *clip) > + struct drm_clip_rect *clip, > + struct dma_buf_map *dst) > { > struct drm_framebuffer *fb = fb_helper->fb; > unsigned int cpp = fb->format->cpp[0]; > size_t offset = clip->y1 * fb->pitches[0] + clip->x1 * cpp; > void *src = fb_helper->fbdev->screen_buffer + offset; > - void *dst = fb_helper->buffer->map.vaddr + offset; > size_t len = (clip->x2 - clip->x1) * cpp; > unsigned int y; > > - for (y = clip->y1; y < clip->y2; y++) { > - if (!fb_helper->dev->mode_config.fbdev_use_iomem) > - memcpy(dst, src, len); > - else > - memcpy_toio((void __iomem *)dst, src, len); > + dma_buf_map_incr(dst, offset); /* go to first pixel within clip rect */ > > + for (y = clip->y1; y < clip->y2; y++) { > + dma_buf_map_memcpy_to(dst, src, len); > + dma_buf_map_incr(dst, fb->pitches[0]); > src += fb->pitches[0]; > - dst += fb->pitches[0]; > } > } > > @@ -417,8 +415,9 @@ static void drm_fb_helper_dirty_work(struct work_struct *work) > ret = drm_client_buffer_vmap(helper->buffer, &map); > if (ret) > return; > - drm_fb_helper_dirty_blit_real(helper, &clip_copy); > + drm_fb_helper_dirty_blit_real(helper, &clip_copy, &map); > } > + > if (helper->fb->funcs->dirty) > helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, > &clip_copy, 1); > @@ -2027,6 +2026,206 @@ static int drm_fbdev_fb_mmap(struct fb_info *info, struct vm_area_struct *vma) > return -ENODEV; > } > > +static bool drm_fbdev_use_iomem(struct fb_info *info) > +{ > + struct drm_fb_helper *fb_helper = info->par; > + struct drm_client_buffer *buffer = fb_helper->buffer; > + > + return !drm_fbdev_use_shadow_fb(fb_helper) && buffer->map.is_iomem; > +} > + > +static ssize_t fb_read_screen_base(struct fb_info *info, char __user *buf, size_t count, > + loff_t pos) The naming here confused me - a name like: fb_read_iomem() would have helped me more. With the current naming I shall remember that the screen_base member is the iomem pointer. > +{ > + const char __iomem *src = info->screen_base + pos; > + size_t alloc_size = min(count, PAGE_SIZE); > + ssize_t ret = 0; > + char *tmp; > + > + tmp = kmalloc(alloc_size, GFP_KERNEL); > + if (!tmp) > + return -ENOMEM; > + I looked around and could not find other places where we copy from iomem to mem to usermem in chunks of PAGE_SIZE. > + while (count) { > + size_t c = min(count, alloc_size); > + > + memcpy_fromio(tmp, src, c); > + if (copy_to_user(buf, tmp, c)) { > + ret = -EFAULT; > + break; > + } > + > + src += c; > + buf += c; > + ret += c; > + count -= c; > + } > + > + kfree(tmp); > + > + return ret; > +} > + > +static ssize_t fb_read_screen_buffer(struct fb_info *info, char __user *buf, size_t count, > + loff_t pos) And fb_read_sysmem() here. > +{ > + const char *src = info->screen_buffer + pos; > + > + if (copy_to_user(buf, src, count)) > + return -EFAULT; > + > + return count; > +} > + > +static ssize_t drm_fbdev_fb_read(struct fb_info *info, char __user *buf, > + size_t count, loff_t *ppos) > +{ > + loff_t pos = *ppos; > + size_t total_size; > + ssize_t ret; > + > + if (info->state != FBINFO_STATE_RUNNING) > + return -EPERM; > + > + if (info->screen_size) > + total_size = info->screen_size; > + else > + total_size = info->fix.smem_len; > + > + if (pos >= total_size) > + return 0; > + if (count >= total_size) > + count = total_size; > + if (total_size - count < pos) > + count = total_size - pos; > + > + if (drm_fbdev_use_iomem(info)) > + ret = fb_read_screen_base(info, buf, count, pos); > + else > + ret = fb_read_screen_buffer(info, buf, count, pos); > + > + if (ret > 0) > + *ppos = ret; > + > + return ret; > +} > + > +static ssize_t fb_write_screen_base(struct fb_info *info, const char __user *buf, size_t count, > + loff_t pos) fb_write_iomem() > +{ > + char __iomem *dst = info->screen_base + pos; > + size_t alloc_size = min(count, PAGE_SIZE); > + ssize_t ret = 0; > + u8 *tmp; > + > + tmp = kmalloc(alloc_size, GFP_KERNEL); > + if (!tmp) > + return -ENOMEM; > + > + while (count) { > + size_t c = min(count, alloc_size); > + > + if (copy_from_user(tmp, buf, c)) { > + ret = -EFAULT; > + break; > + } > + memcpy_toio(dst, tmp, c); > + > + dst += c; > + buf += c; > + ret += c; > + count -= c; > + } > + > + kfree(tmp); > + > + return ret; > +} > + > +static ssize_t fb_write_screen_buffer(struct fb_info *info, const char __user *buf, size_t count, > + loff_t pos) fb_write_sysmem() > +{ > + char *dst = info->screen_buffer + pos; > + > + if (copy_from_user(dst, buf, count)) > + return -EFAULT; > + > + return count; > +} > + > +static ssize_t drm_fbdev_fb_write(struct fb_info *info, const char __user *buf, > + size_t count, loff_t *ppos) > +{ > + loff_t pos = *ppos; > + size_t total_size; > + ssize_t ret; > + int err; > + > + if (info->state != FBINFO_STATE_RUNNING) > + return -EPERM; > + > + if (info->screen_size) > + total_size = info->screen_size; > + else > + total_size = info->fix.smem_len; > + > + if (pos > total_size) > + return -EFBIG; > + if (count > total_size) { > + err = -EFBIG; > + count = total_size; > + } > + if (total_size - count < pos) { > + if (!err) > + err = -ENOSPC; > + count = total_size - pos; > + } > + > + /* > + * Copy to framebuffer even if we already logged an error. Emulates > + * the behavior of the original fbdev implementation. > + */ > + if (drm_fbdev_use_iomem(info)) > + ret = fb_write_screen_base(info, buf, count, pos); > + else > + ret = fb_write_screen_buffer(info, buf, count, pos); > + > + if (ret > 0) > + *ppos = ret; > + > + if (err) > + return err; > + > + return ret; > +} > + > +static void drm_fbdev_fb_fillrect(struct fb_info *info, > + const struct fb_fillrect *rect) > +{ > + if (drm_fbdev_use_iomem(info)) > + drm_fb_helper_cfb_fillrect(info, rect); > + else > + drm_fb_helper_sys_fillrect(info, rect); > +} > + > +static void drm_fbdev_fb_copyarea(struct fb_info *info, > + const struct fb_copyarea *area) > +{ > + if (drm_fbdev_use_iomem(info)) > + drm_fb_helper_cfb_copyarea(info, area); > + else > + drm_fb_helper_sys_copyarea(info, area); > +} > + > +static void drm_fbdev_fb_imageblit(struct fb_info *info, > + const struct fb_image *image) > +{ > + if (drm_fbdev_use_iomem(info)) > + drm_fb_helper_cfb_imageblit(info, image); > + else > + drm_fb_helper_sys_imageblit(info, image); > +} > + > static const struct fb_ops drm_fbdev_fb_ops = { > .owner = THIS_MODULE, > DRM_FB_HELPER_DEFAULT_OPS, > @@ -2034,11 +2233,11 @@ static const struct fb_ops drm_fbdev_fb_ops = { > .fb_release = drm_fbdev_fb_release, > .fb_destroy = drm_fbdev_fb_destroy, > .fb_mmap = drm_fbdev_fb_mmap, > - .fb_read = drm_fb_helper_sys_read, > - .fb_write = drm_fb_helper_sys_write, > - .fb_fillrect = drm_fb_helper_sys_fillrect, > - .fb_copyarea = drm_fb_helper_sys_copyarea, > - .fb_imageblit = drm_fb_helper_sys_imageblit, > + .fb_read = drm_fbdev_fb_read, > + .fb_write = drm_fbdev_fb_write, > + .fb_fillrect = drm_fbdev_fb_fillrect, > + .fb_copyarea = drm_fbdev_fb_copyarea, > + .fb_imageblit = drm_fbdev_fb_imageblit, > }; > > static struct fb_deferred_io drm_fbdev_defio = { > diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h > index 5ffbb4ed5b35..ab424ddd7665 100644 > --- a/include/drm/drm_mode_config.h > +++ b/include/drm/drm_mode_config.h > @@ -877,18 +877,6 @@ struct drm_mode_config { > */ > bool prefer_shadow_fbdev; > > - /** > - * @fbdev_use_iomem: > - * > - * Set to true if framebuffer reside in iomem. > - * When set to true memcpy_toio() is used when copying the framebuffer in > - * drm_fb_helper.drm_fb_helper_dirty_blit_real(). > - * > - * FIXME: This should be replaced with a per-mapping is_iomem > - * flag (like ttm does), and then used everywhere in fbdev code. > - */ > - bool fbdev_use_iomem; > - > /** > * @quirk_addfb_prefer_xbgr_30bpp: > * > -- > 2.28.0
DRM's fbdev console uses regular load and store operations to update framebuffer memory. The bochs driver on sparc64 requires the use of I/O-specific load and store operations. We have a workaround, but need a long-term solution to the problem. This patchset changes GEM's vmap/vunmap interfaces to forward pointers of type struct dma_buf_map and updates the generic fbdev emulation to use them correctly. This enables I/O-memory operations on all framebuffers that require and support them. Patches #1 to #4 prepare VRAM helpers and drivers. Next is the update of the GEM vmap functions. Patch #5 adds vmap and vunmap that is usable with TTM-based GEM drivers, and patch #6 updates GEM's vmap/vunmap callback to forward instances of type struct dma_buf_map. While the patch touches many files throughout the DRM modules, the applied changes are mostly trivial interface fixes. Several TTM-based GEM drivers now use the new vmap code. Patch #7 updates GEM's internal vmap/vunmap functions to forward struct dma_buf_map. With struct dma_buf_map propagated through the layers, patches #8 to #10 convert DRM clients and generic fbdev emulation to use it. Updating the fbdev framebuffer will select the correct functions, either for system or I/O memory. v5: * rebase onto latest TTM changes (Chrsitian) * support TTM premapped memory correctly (Christian) * implement fb_read/fb_write internally (Sam, Daniel) * cleanups v4: * provide TTM vmap/vunmap plus GEM helpers and convert drivers over (Christian, Daniel) * remove several empty functions * more TODOs and documentation (Daniel) v3: * recreate the whole patchset on top of struct dma_buf_map v2: * RFC patchset Thomas Zimmermann (10): drm/vram-helper: Remove invariant parameters from internal kmap function drm/cma-helper: Remove empty drm_gem_cma_prime_vunmap() drm/etnaviv: Remove empty etnaviv_gem_prime_vunmap() drm/exynos: Remove empty exynos_drm_gem_prime_{vmap,vunmap}() drm/ttm: Add vmap/vunmap to TTM and TTM GEM helpers drm/gem: Use struct dma_buf_map in GEM vmap ops and convert GEM backends drm/gem: Update internal GEM vmap/vunmap interfaces to use struct dma_buf_map drm/gem: Store client buffer mappings as struct dma_buf_map dma-buf-map: Add memcpy and pointer-increment interfaces drm/fb_helper: Support framebuffers in I/O memory Documentation/gpu/todo.rst | 37 ++- drivers/gpu/drm/Kconfig | 2 + drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c | 36 --- drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.h | 2 - drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 5 +- drivers/gpu/drm/amd/amdgpu/amdgpu_object.h | 1 - drivers/gpu/drm/ast/ast_cursor.c | 27 +-- drivers/gpu/drm/ast/ast_drv.h | 7 +- drivers/gpu/drm/bochs/bochs_kms.c | 1 - drivers/gpu/drm/drm_client.c | 38 +-- drivers/gpu/drm/drm_fb_helper.c | 248 ++++++++++++++++++-- drivers/gpu/drm/drm_gem.c | 29 ++- drivers/gpu/drm/drm_gem_cma_helper.c | 27 +-- drivers/gpu/drm/drm_gem_shmem_helper.c | 48 ++-- drivers/gpu/drm/drm_gem_ttm_helper.c | 38 +++ drivers/gpu/drm/drm_gem_vram_helper.c | 117 +++++---- drivers/gpu/drm/drm_internal.h | 5 +- drivers/gpu/drm/drm_prime.c | 14 +- drivers/gpu/drm/etnaviv/etnaviv_drv.h | 3 +- drivers/gpu/drm/etnaviv/etnaviv_gem.c | 1 - drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c | 12 +- drivers/gpu/drm/exynos/exynos_drm_gem.c | 12 - drivers/gpu/drm/exynos/exynos_drm_gem.h | 2 - drivers/gpu/drm/lima/lima_gem.c | 6 +- drivers/gpu/drm/lima/lima_sched.c | 11 +- drivers/gpu/drm/mgag200/mgag200_mode.c | 10 +- drivers/gpu/drm/nouveau/Kconfig | 1 + drivers/gpu/drm/nouveau/nouveau_bo.h | 2 - drivers/gpu/drm/nouveau/nouveau_gem.c | 6 +- drivers/gpu/drm/nouveau/nouveau_gem.h | 2 - drivers/gpu/drm/nouveau/nouveau_prime.c | 20 -- drivers/gpu/drm/panfrost/panfrost_perfcnt.c | 14 +- drivers/gpu/drm/qxl/qxl_display.c | 11 +- drivers/gpu/drm/qxl/qxl_draw.c | 14 +- drivers/gpu/drm/qxl/qxl_drv.h | 11 +- drivers/gpu/drm/qxl/qxl_object.c | 31 ++- drivers/gpu/drm/qxl/qxl_object.h | 2 +- drivers/gpu/drm/qxl/qxl_prime.c | 12 +- drivers/gpu/drm/radeon/radeon.h | 1 - drivers/gpu/drm/radeon/radeon_gem.c | 7 +- drivers/gpu/drm/radeon/radeon_prime.c | 20 -- drivers/gpu/drm/rockchip/rockchip_drm_gem.c | 22 +- drivers/gpu/drm/rockchip/rockchip_drm_gem.h | 4 +- drivers/gpu/drm/tiny/cirrus.c | 10 +- drivers/gpu/drm/tiny/gm12u320.c | 10 +- drivers/gpu/drm/ttm/ttm_bo_util.c | 72 ++++++ drivers/gpu/drm/udl/udl_modeset.c | 8 +- drivers/gpu/drm/vboxvideo/vbox_mode.c | 11 +- drivers/gpu/drm/vc4/vc4_bo.c | 7 +- drivers/gpu/drm/vc4/vc4_drv.h | 2 +- drivers/gpu/drm/vgem/vgem_drv.c | 16 +- drivers/gpu/drm/vkms/vkms_plane.c | 15 +- drivers/gpu/drm/vkms/vkms_writeback.c | 22 +- drivers/gpu/drm/xen/xen_drm_front_gem.c | 18 +- drivers/gpu/drm/xen/xen_drm_front_gem.h | 6 +- include/drm/drm_client.h | 7 +- include/drm/drm_gem.h | 5 +- include/drm/drm_gem_cma_helper.h | 3 +- include/drm/drm_gem_shmem_helper.h | 4 +- include/drm/drm_gem_ttm_helper.h | 6 + include/drm/drm_gem_vram_helper.h | 14 +- include/drm/drm_mode_config.h | 12 - include/drm/ttm/ttm_bo_api.h | 28 +++ include/linux/dma-buf-map.h | 93 +++++++- 64 files changed, 852 insertions(+), 436 deletions(-)