| Message ID | 20200806085239.4606-3-tzimmermann@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
On Thu, Aug 06, 2020 at 10:52:37AM +0200, Thomas Zimmermann wrote: > GEM's vmap interface now wraps memory pointers in struct drm_gem_membuf. > The structure represents a pointer into the framebuffer, which is either > in I/O memory or in system memory. The structure contains a flag that > distinguishes these cases. > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > --- > drivers/gpu/drm/drm_client.c | 25 ++++++++++++------------- > drivers/gpu/drm/drm_fb_helper.c | 18 +++++++++--------- > drivers/gpu/drm/drm_gem.c | 19 +++++++++++-------- > drivers/gpu/drm/drm_internal.h | 5 +++-- > drivers/gpu/drm/drm_prime.c | 16 ++++++++++------ > include/drm/drm_client.h | 7 ++++--- > include/drm/drm_device.h | 26 ++++++++++++++++++++++++++ > 7 files changed, 75 insertions(+), 41 deletions(-) > > diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c > index 495f47d23d87..0359b82928c1 100644 > --- a/drivers/gpu/drm/drm_client.c > +++ b/drivers/gpu/drm/drm_client.c > @@ -234,7 +234,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->membuf); > > if (buffer->gem) > drm_gem_object_put(buffer->gem); > @@ -302,12 +302,13 @@ drm_client_buffer_create(struct drm_client_dev *client, u32 width, u32 height, u > * Returns: > * The mapped memory's address > */ > -void *drm_client_buffer_vmap(struct drm_client_buffer *buffer) > +const struct drm_gem_membuf * > +drm_client_buffer_vmap(struct drm_client_buffer *buffer) > { > - void *vaddr; > + int ret; > > - if (buffer->vaddr) > - return buffer->vaddr; > + if (buffer->membuf.vaddr) > + return &buffer->membuf; > > /* > * FIXME: The dependency on GEM here isn't required, we could > @@ -317,13 +318,11 @@ 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. > */ > - vaddr = drm_gem_vmap(buffer->gem); > - if (IS_ERR(vaddr)) > - return vaddr; > - > - buffer->vaddr = vaddr; > + ret = drm_gem_vmap(buffer->gem, &buffer->membuf); > + if (ret) > + return ERR_PTR(ret); > > - return vaddr; > + return &buffer->membuf; > } > EXPORT_SYMBOL(drm_client_buffer_vmap); > > @@ -337,8 +336,8 @@ EXPORT_SYMBOL(drm_client_buffer_vmap); > */ > void drm_client_buffer_vunmap(struct drm_client_buffer *buffer) > { > - drm_gem_vunmap(buffer->gem, buffer->vaddr); > - buffer->vaddr = NULL; > + drm_gem_vunmap(buffer->gem, &buffer->membuf); > + buffer->membuf.vaddr = NULL; > } > 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 8697554ccd41..da24874247e7 100644 > --- a/drivers/gpu/drm/drm_fb_helper.c > +++ b/drivers/gpu/drm/drm_fb_helper.c > @@ -394,7 +394,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->membuf.vaddr + offset; > size_t len = (clip->x2 - clip->x1) * cpp; > unsigned int y; > > @@ -416,7 +416,7 @@ 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; > + const struct drm_gem_membuf *buf; > > spin_lock_irqsave(&helper->dirty_lock, flags); > clip_copy = *clip; > @@ -429,8 +429,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)) > + buf = drm_client_buffer_vmap(helper->buffer); > + if (IS_ERR(buf)) > return; > drm_fb_helper_dirty_blit_real(helper, &clip_copy); > } > @@ -2076,7 +2076,7 @@ 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; > + const struct drm_gem_membuf *membuf; > > drm_dbg_kms(dev, "surface width(%d), height(%d) and bpp(%d)\n", > sizes->surface_width, sizes->surface_height, > @@ -2112,11 +2112,11 @@ 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); > + membuf = drm_client_buffer_vmap(fb_helper->buffer); > + if (IS_ERR(membuf)) > + return PTR_ERR(membuf); > > - fbi->screen_buffer = vaddr; > + fbi->screen_buffer = membuf->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) > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c > index 19d73868490e..36ded6a56fb2 100644 > --- a/drivers/gpu/drm/drm_gem.c > +++ b/drivers/gpu/drm/drm_gem.c > @@ -1220,7 +1220,7 @@ void drm_gem_unpin(struct drm_gem_object *obj) > obj->dev->driver->gem_prime_unpin(obj); > } > > -void *drm_gem_vmap(struct drm_gem_object *obj) > +int drm_gem_vmap(struct drm_gem_object *obj, struct drm_gem_membuf *buf) > { > void *vaddr; > > @@ -1229,23 +1229,26 @@ void *drm_gem_vmap(struct drm_gem_object *obj) > else if (obj->dev->driver->gem_prime_vmap) > vaddr = obj->dev->driver->gem_prime_vmap(obj); > else > - vaddr = ERR_PTR(-EOPNOTSUPP); > + return -EOPNOTSUPP; > > if (!vaddr) > - vaddr = ERR_PTR(-ENOMEM); > + return -ENOMEM; > + > + buf->vaddr = vaddr; > + buf->is_iomem = false; > > - return vaddr; > + return 0; > } > > -void drm_gem_vunmap(struct drm_gem_object *obj, void *vaddr) > +void drm_gem_vunmap(struct drm_gem_object *obj, const struct drm_gem_membuf *buf) > { > - if (!vaddr) > + if (!buf || !buf->vaddr) > return; > > if (obj->funcs && obj->funcs->vunmap) > - obj->funcs->vunmap(obj, vaddr); > + obj->funcs->vunmap(obj, buf->vaddr); > else if (obj->dev->driver->gem_prime_vunmap) > - obj->dev->driver->gem_prime_vunmap(obj, vaddr); > + obj->dev->driver->gem_prime_vunmap(obj, buf->vaddr); > } > > /** > diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h > index 8e01caaf95cc..201d71249954 100644 > --- a/drivers/gpu/drm/drm_internal.h > +++ b/drivers/gpu/drm/drm_internal.h > @@ -36,6 +36,7 @@ struct dma_buf; > struct drm_connector; > struct drm_crtc; > struct drm_framebuffer; > +struct drm_gem_membuf; > struct drm_gem_object; > struct drm_master; > struct drm_minor; > @@ -186,8 +187,8 @@ void drm_gem_print_info(struct drm_printer *p, unsigned int indent, > > int drm_gem_pin(struct drm_gem_object *obj); > void drm_gem_unpin(struct drm_gem_object *obj); > -void *drm_gem_vmap(struct drm_gem_object *obj); > -void drm_gem_vunmap(struct drm_gem_object *obj, void *vaddr); > +int drm_gem_vmap(struct drm_gem_object *obj, struct drm_gem_membuf *buf); > +void drm_gem_vunmap(struct drm_gem_object *obj, const struct drm_gem_membuf *buf); > > /* drm_debugfs.c drm_debugfs_crc.c */ > #if defined(CONFIG_DEBUG_FS) > diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c > index 1693aa7c14b5..d95a39030a93 100644 > --- a/drivers/gpu/drm/drm_prime.c > +++ b/drivers/gpu/drm/drm_prime.c > @@ -671,13 +671,14 @@ EXPORT_SYMBOL(drm_gem_unmap_dma_buf); > void *drm_gem_dmabuf_vmap(struct dma_buf *dma_buf) > { > struct drm_gem_object *obj = dma_buf->priv; > - void *vaddr; > + struct drm_gem_membuf buf; > + int ret; > > - vaddr = drm_gem_vmap(obj); > - if (IS_ERR(vaddr)) > - vaddr = NULL; > + ret = drm_gem_vmap(obj, &buf); > + if (ret) > + buf.vaddr = NULL; > > - return vaddr; > + return buf.vaddr; > } > EXPORT_SYMBOL(drm_gem_dmabuf_vmap); > > @@ -692,8 +693,11 @@ EXPORT_SYMBOL(drm_gem_dmabuf_vmap); > void drm_gem_dmabuf_vunmap(struct dma_buf *dma_buf, void *vaddr) > { > struct drm_gem_object *obj = dma_buf->priv; > + struct drm_gem_membuf buf; > > - drm_gem_vunmap(obj, vaddr); > + buf.vaddr = vaddr; > + buf.is_iomem = false; > + drm_gem_vunmap(obj, &buf); > } > EXPORT_SYMBOL(drm_gem_dmabuf_vunmap); > > diff --git a/include/drm/drm_client.h b/include/drm/drm_client.h > index 7aaea665bfc2..5ed73c390619 100644 > --- a/include/drm/drm_client.h > +++ b/include/drm/drm_client.h > @@ -14,6 +14,7 @@ struct drm_client_dev; > struct drm_device; > struct drm_file; > struct drm_framebuffer; > +struct drm_gem_membuf; > struct drm_gem_object; > struct drm_minor; > struct module; > @@ -141,9 +142,9 @@ struct drm_client_buffer { > struct drm_gem_object *gem; > > /** > - * @vaddr: Virtual address for the buffer > + * @membuf: Virtual address for the buffer > */ > - void *vaddr; > + struct drm_gem_membuf membuf; > > /** > * @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); > +const struct drm_gem_membuf *drm_client_buffer_vmap(struct drm_client_buffer *buffer); > void drm_client_buffer_vunmap(struct drm_client_buffer *buffer); > > int drm_client_modeset_create(struct drm_client_dev *client); > diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h > index 0988351d743c..6ecf03601c36 100644 > --- a/include/drm/drm_device.h > +++ b/include/drm/drm_device.h > @@ -381,4 +381,30 @@ struct drm_device { > #endif > }; > > +/** > + * struct drm_gem_membuf - GEM memory buffer This isn't gem specific, and membuf feels a bit strange - it's not a memory buffer, but more a buffer mapping. I'd call these struct drm_bufmap. > + */ > +struct drm_gem_membuf { > + union { > + /** > + * @vaddr: > + * > + * The virtual address for the buffer in system memory. > + */ > + void *vaddr; > + /** > + * @vaddr_iomem: > + * > + * The virtual address for the buffer in I/O memory. > + */ > + void __iomem *vaddr_iomem; > + }; > + /** > + * @is_iomem: > + * > + * True if the memory is located in I/O memory, false otherwise. > + */ > + bool is_iomem; > +}; I think if we do this we should go all in, i.e. create a new header, and then start to add all kinds of helper functions to it, like: static inline drm_memcpy_to_bufmap(bufmap, ...) { if (bufmap->is_iomem) memcpy_toio(bufmap->vaddr_iomem, ...); else memcpy(bufmap->vaddr_iomem, ...); } totally wrong code of course, but I think you get the idea. If we add helpers like this I think a new header file is also warranted, so maybe put it all in drm_bufmap.h and then wire up the kerneldoc. Also I think cc: ttm people for an ack would be good, maybe long-term we want to roll this out through ttm too. Cheers, Daniel > + > #endif > -- > 2.28.0 >
Hi Am 13.08.20 um 12:26 schrieb Daniel Vetter: > On Thu, Aug 06, 2020 at 10:52:37AM +0200, Thomas Zimmermann wrote: >> GEM's vmap interface now wraps memory pointers in struct drm_gem_membuf. >> The structure represents a pointer into the framebuffer, which is either >> in I/O memory or in system memory. The structure contains a flag that >> distinguishes these cases. >> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >> --- >> drivers/gpu/drm/drm_client.c | 25 ++++++++++++------------- >> drivers/gpu/drm/drm_fb_helper.c | 18 +++++++++--------- >> drivers/gpu/drm/drm_gem.c | 19 +++++++++++-------- >> drivers/gpu/drm/drm_internal.h | 5 +++-- >> drivers/gpu/drm/drm_prime.c | 16 ++++++++++------ >> include/drm/drm_client.h | 7 ++++--- >> include/drm/drm_device.h | 26 ++++++++++++++++++++++++++ >> 7 files changed, 75 insertions(+), 41 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c >> index 495f47d23d87..0359b82928c1 100644 >> --- a/drivers/gpu/drm/drm_client.c >> +++ b/drivers/gpu/drm/drm_client.c >> @@ -234,7 +234,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->membuf); >> >> if (buffer->gem) >> drm_gem_object_put(buffer->gem); >> @@ -302,12 +302,13 @@ drm_client_buffer_create(struct drm_client_dev *client, u32 width, u32 height, u >> * Returns: >> * The mapped memory's address >> */ >> -void *drm_client_buffer_vmap(struct drm_client_buffer *buffer) >> +const struct drm_gem_membuf * >> +drm_client_buffer_vmap(struct drm_client_buffer *buffer) >> { >> - void *vaddr; >> + int ret; >> >> - if (buffer->vaddr) >> - return buffer->vaddr; >> + if (buffer->membuf.vaddr) >> + return &buffer->membuf; >> >> /* >> * FIXME: The dependency on GEM here isn't required, we could >> @@ -317,13 +318,11 @@ 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. >> */ >> - vaddr = drm_gem_vmap(buffer->gem); >> - if (IS_ERR(vaddr)) >> - return vaddr; >> - >> - buffer->vaddr = vaddr; >> + ret = drm_gem_vmap(buffer->gem, &buffer->membuf); >> + if (ret) >> + return ERR_PTR(ret); >> >> - return vaddr; >> + return &buffer->membuf; >> } >> EXPORT_SYMBOL(drm_client_buffer_vmap); >> >> @@ -337,8 +336,8 @@ EXPORT_SYMBOL(drm_client_buffer_vmap); >> */ >> void drm_client_buffer_vunmap(struct drm_client_buffer *buffer) >> { >> - drm_gem_vunmap(buffer->gem, buffer->vaddr); >> - buffer->vaddr = NULL; >> + drm_gem_vunmap(buffer->gem, &buffer->membuf); >> + buffer->membuf.vaddr = NULL; >> } >> 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 8697554ccd41..da24874247e7 100644 >> --- a/drivers/gpu/drm/drm_fb_helper.c >> +++ b/drivers/gpu/drm/drm_fb_helper.c >> @@ -394,7 +394,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->membuf.vaddr + offset; >> size_t len = (clip->x2 - clip->x1) * cpp; >> unsigned int y; >> >> @@ -416,7 +416,7 @@ 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; >> + const struct drm_gem_membuf *buf; >> >> spin_lock_irqsave(&helper->dirty_lock, flags); >> clip_copy = *clip; >> @@ -429,8 +429,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)) >> + buf = drm_client_buffer_vmap(helper->buffer); >> + if (IS_ERR(buf)) >> return; >> drm_fb_helper_dirty_blit_real(helper, &clip_copy); >> } >> @@ -2076,7 +2076,7 @@ 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; >> + const struct drm_gem_membuf *membuf; >> >> drm_dbg_kms(dev, "surface width(%d), height(%d) and bpp(%d)\n", >> sizes->surface_width, sizes->surface_height, >> @@ -2112,11 +2112,11 @@ 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); >> + membuf = drm_client_buffer_vmap(fb_helper->buffer); >> + if (IS_ERR(membuf)) >> + return PTR_ERR(membuf); >> >> - fbi->screen_buffer = vaddr; >> + fbi->screen_buffer = membuf->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) >> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c >> index 19d73868490e..36ded6a56fb2 100644 >> --- a/drivers/gpu/drm/drm_gem.c >> +++ b/drivers/gpu/drm/drm_gem.c >> @@ -1220,7 +1220,7 @@ void drm_gem_unpin(struct drm_gem_object *obj) >> obj->dev->driver->gem_prime_unpin(obj); >> } >> >> -void *drm_gem_vmap(struct drm_gem_object *obj) >> +int drm_gem_vmap(struct drm_gem_object *obj, struct drm_gem_membuf *buf) >> { >> void *vaddr; >> >> @@ -1229,23 +1229,26 @@ void *drm_gem_vmap(struct drm_gem_object *obj) >> else if (obj->dev->driver->gem_prime_vmap) >> vaddr = obj->dev->driver->gem_prime_vmap(obj); >> else >> - vaddr = ERR_PTR(-EOPNOTSUPP); >> + return -EOPNOTSUPP; >> >> if (!vaddr) >> - vaddr = ERR_PTR(-ENOMEM); >> + return -ENOMEM; >> + >> + buf->vaddr = vaddr; >> + buf->is_iomem = false; >> >> - return vaddr; >> + return 0; >> } >> >> -void drm_gem_vunmap(struct drm_gem_object *obj, void *vaddr) >> +void drm_gem_vunmap(struct drm_gem_object *obj, const struct drm_gem_membuf *buf) >> { >> - if (!vaddr) >> + if (!buf || !buf->vaddr) >> return; >> >> if (obj->funcs && obj->funcs->vunmap) >> - obj->funcs->vunmap(obj, vaddr); >> + obj->funcs->vunmap(obj, buf->vaddr); >> else if (obj->dev->driver->gem_prime_vunmap) >> - obj->dev->driver->gem_prime_vunmap(obj, vaddr); >> + obj->dev->driver->gem_prime_vunmap(obj, buf->vaddr); >> } >> >> /** >> diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h >> index 8e01caaf95cc..201d71249954 100644 >> --- a/drivers/gpu/drm/drm_internal.h >> +++ b/drivers/gpu/drm/drm_internal.h >> @@ -36,6 +36,7 @@ struct dma_buf; >> struct drm_connector; >> struct drm_crtc; >> struct drm_framebuffer; >> +struct drm_gem_membuf; >> struct drm_gem_object; >> struct drm_master; >> struct drm_minor; >> @@ -186,8 +187,8 @@ void drm_gem_print_info(struct drm_printer *p, unsigned int indent, >> >> int drm_gem_pin(struct drm_gem_object *obj); >> void drm_gem_unpin(struct drm_gem_object *obj); >> -void *drm_gem_vmap(struct drm_gem_object *obj); >> -void drm_gem_vunmap(struct drm_gem_object *obj, void *vaddr); >> +int drm_gem_vmap(struct drm_gem_object *obj, struct drm_gem_membuf *buf); >> +void drm_gem_vunmap(struct drm_gem_object *obj, const struct drm_gem_membuf *buf); >> >> /* drm_debugfs.c drm_debugfs_crc.c */ >> #if defined(CONFIG_DEBUG_FS) >> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c >> index 1693aa7c14b5..d95a39030a93 100644 >> --- a/drivers/gpu/drm/drm_prime.c >> +++ b/drivers/gpu/drm/drm_prime.c >> @@ -671,13 +671,14 @@ EXPORT_SYMBOL(drm_gem_unmap_dma_buf); >> void *drm_gem_dmabuf_vmap(struct dma_buf *dma_buf) >> { >> struct drm_gem_object *obj = dma_buf->priv; >> - void *vaddr; >> + struct drm_gem_membuf buf; >> + int ret; >> >> - vaddr = drm_gem_vmap(obj); >> - if (IS_ERR(vaddr)) >> - vaddr = NULL; >> + ret = drm_gem_vmap(obj, &buf); >> + if (ret) >> + buf.vaddr = NULL; >> >> - return vaddr; >> + return buf.vaddr; >> } >> EXPORT_SYMBOL(drm_gem_dmabuf_vmap); >> >> @@ -692,8 +693,11 @@ EXPORT_SYMBOL(drm_gem_dmabuf_vmap); >> void drm_gem_dmabuf_vunmap(struct dma_buf *dma_buf, void *vaddr) >> { >> struct drm_gem_object *obj = dma_buf->priv; >> + struct drm_gem_membuf buf; >> >> - drm_gem_vunmap(obj, vaddr); >> + buf.vaddr = vaddr; >> + buf.is_iomem = false; >> + drm_gem_vunmap(obj, &buf); >> } >> EXPORT_SYMBOL(drm_gem_dmabuf_vunmap); >> >> diff --git a/include/drm/drm_client.h b/include/drm/drm_client.h >> index 7aaea665bfc2..5ed73c390619 100644 >> --- a/include/drm/drm_client.h >> +++ b/include/drm/drm_client.h >> @@ -14,6 +14,7 @@ struct drm_client_dev; >> struct drm_device; >> struct drm_file; >> struct drm_framebuffer; >> +struct drm_gem_membuf; >> struct drm_gem_object; >> struct drm_minor; >> struct module; >> @@ -141,9 +142,9 @@ struct drm_client_buffer { >> struct drm_gem_object *gem; >> >> /** >> - * @vaddr: Virtual address for the buffer >> + * @membuf: Virtual address for the buffer >> */ >> - void *vaddr; >> + struct drm_gem_membuf membuf; >> >> /** >> * @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); >> +const struct drm_gem_membuf *drm_client_buffer_vmap(struct drm_client_buffer *buffer); >> void drm_client_buffer_vunmap(struct drm_client_buffer *buffer); >> >> int drm_client_modeset_create(struct drm_client_dev *client); >> diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h >> index 0988351d743c..6ecf03601c36 100644 >> --- a/include/drm/drm_device.h >> +++ b/include/drm/drm_device.h >> @@ -381,4 +381,30 @@ struct drm_device { >> #endif >> }; >> >> +/** >> + * struct drm_gem_membuf - GEM memory buffer > > This isn't gem specific, and membuf feels a bit strange - it's not a > memory buffer, but more a buffer mapping. I'd call these struct > drm_bufmap. Yep, I didn't like the name either. struct drm_bufmap sounds good. As i mentioned on irc, I'd also want to propose this as an update to dma-buf's vmap. It's not that much more work and should fix the issue once and for all. If they are interested, struct dma_buf_addr might be a candidate. > >> + */ >> +struct drm_gem_membuf { >> + union { >> + /** >> + * @vaddr: >> + * >> + * The virtual address for the buffer in system memory. >> + */ >> + void *vaddr; >> + /** >> + * @vaddr_iomem: >> + * >> + * The virtual address for the buffer in I/O memory. >> + */ >> + void __iomem *vaddr_iomem; >> + }; >> + /** >> + * @is_iomem: >> + * >> + * True if the memory is located in I/O memory, false otherwise. >> + */ >> + bool is_iomem; >> +}; > > I think if we do this we should go all in, i.e. create a new header, and > then start to add all kinds of helper functions to it, like: > > static inline drm_memcpy_to_bufmap(bufmap, ...) > { > if (bufmap->is_iomem) > memcpy_toio(bufmap->vaddr_iomem, ...); > else > memcpy(bufmap->vaddr_iomem, ...); > > } > > totally wrong code of course, but I think you get the idea. If we add > helpers like this I think a new header file is also warranted, so maybe > put it all in drm_bufmap.h and then wire up the kerneldoc. > > Also I think cc: ttm people for an ack would be good, maybe long-term we > want to roll this out through ttm too. Sure. Best regards Thomas > > Cheers, Daniel > > >> + >> #endif >> -- >> 2.28.0 >> >
On Thu, Aug 13, 2020 at 12:46 PM Thomas Zimmermann <tzimmermann@suse.de> wrote: > > Hi > > Am 13.08.20 um 12:26 schrieb Daniel Vetter: > > On Thu, Aug 06, 2020 at 10:52:37AM +0200, Thomas Zimmermann wrote: > >> GEM's vmap interface now wraps memory pointers in struct drm_gem_membuf. > >> The structure represents a pointer into the framebuffer, which is either > >> in I/O memory or in system memory. The structure contains a flag that > >> distinguishes these cases. > >> > >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > >> --- > >> drivers/gpu/drm/drm_client.c | 25 ++++++++++++------------- > >> drivers/gpu/drm/drm_fb_helper.c | 18 +++++++++--------- > >> drivers/gpu/drm/drm_gem.c | 19 +++++++++++-------- > >> drivers/gpu/drm/drm_internal.h | 5 +++-- > >> drivers/gpu/drm/drm_prime.c | 16 ++++++++++------ > >> include/drm/drm_client.h | 7 ++++--- > >> include/drm/drm_device.h | 26 ++++++++++++++++++++++++++ > >> 7 files changed, 75 insertions(+), 41 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c > >> index 495f47d23d87..0359b82928c1 100644 > >> --- a/drivers/gpu/drm/drm_client.c > >> +++ b/drivers/gpu/drm/drm_client.c > >> @@ -234,7 +234,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->membuf); > >> > >> if (buffer->gem) > >> drm_gem_object_put(buffer->gem); > >> @@ -302,12 +302,13 @@ drm_client_buffer_create(struct drm_client_dev *client, u32 width, u32 height, u > >> * Returns: > >> * The mapped memory's address > >> */ > >> -void *drm_client_buffer_vmap(struct drm_client_buffer *buffer) > >> +const struct drm_gem_membuf * > >> +drm_client_buffer_vmap(struct drm_client_buffer *buffer) > >> { > >> - void *vaddr; > >> + int ret; > >> > >> - if (buffer->vaddr) > >> - return buffer->vaddr; > >> + if (buffer->membuf.vaddr) > >> + return &buffer->membuf; > >> > >> /* > >> * FIXME: The dependency on GEM here isn't required, we could > >> @@ -317,13 +318,11 @@ 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. > >> */ > >> - vaddr = drm_gem_vmap(buffer->gem); > >> - if (IS_ERR(vaddr)) > >> - return vaddr; > >> - > >> - buffer->vaddr = vaddr; > >> + ret = drm_gem_vmap(buffer->gem, &buffer->membuf); > >> + if (ret) > >> + return ERR_PTR(ret); > >> > >> - return vaddr; > >> + return &buffer->membuf; > >> } > >> EXPORT_SYMBOL(drm_client_buffer_vmap); > >> > >> @@ -337,8 +336,8 @@ EXPORT_SYMBOL(drm_client_buffer_vmap); > >> */ > >> void drm_client_buffer_vunmap(struct drm_client_buffer *buffer) > >> { > >> - drm_gem_vunmap(buffer->gem, buffer->vaddr); > >> - buffer->vaddr = NULL; > >> + drm_gem_vunmap(buffer->gem, &buffer->membuf); > >> + buffer->membuf.vaddr = NULL; > >> } > >> 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 8697554ccd41..da24874247e7 100644 > >> --- a/drivers/gpu/drm/drm_fb_helper.c > >> +++ b/drivers/gpu/drm/drm_fb_helper.c > >> @@ -394,7 +394,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->membuf.vaddr + offset; > >> size_t len = (clip->x2 - clip->x1) * cpp; > >> unsigned int y; > >> > >> @@ -416,7 +416,7 @@ 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; > >> + const struct drm_gem_membuf *buf; > >> > >> spin_lock_irqsave(&helper->dirty_lock, flags); > >> clip_copy = *clip; > >> @@ -429,8 +429,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)) > >> + buf = drm_client_buffer_vmap(helper->buffer); > >> + if (IS_ERR(buf)) > >> return; > >> drm_fb_helper_dirty_blit_real(helper, &clip_copy); > >> } > >> @@ -2076,7 +2076,7 @@ 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; > >> + const struct drm_gem_membuf *membuf; > >> > >> drm_dbg_kms(dev, "surface width(%d), height(%d) and bpp(%d)\n", > >> sizes->surface_width, sizes->surface_height, > >> @@ -2112,11 +2112,11 @@ 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); > >> + membuf = drm_client_buffer_vmap(fb_helper->buffer); > >> + if (IS_ERR(membuf)) > >> + return PTR_ERR(membuf); > >> > >> - fbi->screen_buffer = vaddr; > >> + fbi->screen_buffer = membuf->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) > >> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c > >> index 19d73868490e..36ded6a56fb2 100644 > >> --- a/drivers/gpu/drm/drm_gem.c > >> +++ b/drivers/gpu/drm/drm_gem.c > >> @@ -1220,7 +1220,7 @@ void drm_gem_unpin(struct drm_gem_object *obj) > >> obj->dev->driver->gem_prime_unpin(obj); > >> } > >> > >> -void *drm_gem_vmap(struct drm_gem_object *obj) > >> +int drm_gem_vmap(struct drm_gem_object *obj, struct drm_gem_membuf *buf) > >> { > >> void *vaddr; > >> > >> @@ -1229,23 +1229,26 @@ void *drm_gem_vmap(struct drm_gem_object *obj) > >> else if (obj->dev->driver->gem_prime_vmap) > >> vaddr = obj->dev->driver->gem_prime_vmap(obj); > >> else > >> - vaddr = ERR_PTR(-EOPNOTSUPP); > >> + return -EOPNOTSUPP; > >> > >> if (!vaddr) > >> - vaddr = ERR_PTR(-ENOMEM); > >> + return -ENOMEM; > >> + > >> + buf->vaddr = vaddr; > >> + buf->is_iomem = false; > >> > >> - return vaddr; > >> + return 0; > >> } > >> > >> -void drm_gem_vunmap(struct drm_gem_object *obj, void *vaddr) > >> +void drm_gem_vunmap(struct drm_gem_object *obj, const struct drm_gem_membuf *buf) > >> { > >> - if (!vaddr) > >> + if (!buf || !buf->vaddr) > >> return; > >> > >> if (obj->funcs && obj->funcs->vunmap) > >> - obj->funcs->vunmap(obj, vaddr); > >> + obj->funcs->vunmap(obj, buf->vaddr); > >> else if (obj->dev->driver->gem_prime_vunmap) > >> - obj->dev->driver->gem_prime_vunmap(obj, vaddr); > >> + obj->dev->driver->gem_prime_vunmap(obj, buf->vaddr); > >> } > >> > >> /** > >> diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h > >> index 8e01caaf95cc..201d71249954 100644 > >> --- a/drivers/gpu/drm/drm_internal.h > >> +++ b/drivers/gpu/drm/drm_internal.h > >> @@ -36,6 +36,7 @@ struct dma_buf; > >> struct drm_connector; > >> struct drm_crtc; > >> struct drm_framebuffer; > >> +struct drm_gem_membuf; > >> struct drm_gem_object; > >> struct drm_master; > >> struct drm_minor; > >> @@ -186,8 +187,8 @@ void drm_gem_print_info(struct drm_printer *p, unsigned int indent, > >> > >> int drm_gem_pin(struct drm_gem_object *obj); > >> void drm_gem_unpin(struct drm_gem_object *obj); > >> -void *drm_gem_vmap(struct drm_gem_object *obj); > >> -void drm_gem_vunmap(struct drm_gem_object *obj, void *vaddr); > >> +int drm_gem_vmap(struct drm_gem_object *obj, struct drm_gem_membuf *buf); > >> +void drm_gem_vunmap(struct drm_gem_object *obj, const struct drm_gem_membuf *buf); > >> > >> /* drm_debugfs.c drm_debugfs_crc.c */ > >> #if defined(CONFIG_DEBUG_FS) > >> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c > >> index 1693aa7c14b5..d95a39030a93 100644 > >> --- a/drivers/gpu/drm/drm_prime.c > >> +++ b/drivers/gpu/drm/drm_prime.c > >> @@ -671,13 +671,14 @@ EXPORT_SYMBOL(drm_gem_unmap_dma_buf); > >> void *drm_gem_dmabuf_vmap(struct dma_buf *dma_buf) > >> { > >> struct drm_gem_object *obj = dma_buf->priv; > >> - void *vaddr; > >> + struct drm_gem_membuf buf; > >> + int ret; > >> > >> - vaddr = drm_gem_vmap(obj); > >> - if (IS_ERR(vaddr)) > >> - vaddr = NULL; > >> + ret = drm_gem_vmap(obj, &buf); > >> + if (ret) > >> + buf.vaddr = NULL; > >> > >> - return vaddr; > >> + return buf.vaddr; > >> } > >> EXPORT_SYMBOL(drm_gem_dmabuf_vmap); > >> > >> @@ -692,8 +693,11 @@ EXPORT_SYMBOL(drm_gem_dmabuf_vmap); > >> void drm_gem_dmabuf_vunmap(struct dma_buf *dma_buf, void *vaddr) > >> { > >> struct drm_gem_object *obj = dma_buf->priv; > >> + struct drm_gem_membuf buf; > >> > >> - drm_gem_vunmap(obj, vaddr); > >> + buf.vaddr = vaddr; > >> + buf.is_iomem = false; > >> + drm_gem_vunmap(obj, &buf); > >> } > >> EXPORT_SYMBOL(drm_gem_dmabuf_vunmap); > >> > >> diff --git a/include/drm/drm_client.h b/include/drm/drm_client.h > >> index 7aaea665bfc2..5ed73c390619 100644 > >> --- a/include/drm/drm_client.h > >> +++ b/include/drm/drm_client.h > >> @@ -14,6 +14,7 @@ struct drm_client_dev; > >> struct drm_device; > >> struct drm_file; > >> struct drm_framebuffer; > >> +struct drm_gem_membuf; > >> struct drm_gem_object; > >> struct drm_minor; > >> struct module; > >> @@ -141,9 +142,9 @@ struct drm_client_buffer { > >> struct drm_gem_object *gem; > >> > >> /** > >> - * @vaddr: Virtual address for the buffer > >> + * @membuf: Virtual address for the buffer > >> */ > >> - void *vaddr; > >> + struct drm_gem_membuf membuf; > >> > >> /** > >> * @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); > >> +const struct drm_gem_membuf *drm_client_buffer_vmap(struct drm_client_buffer *buffer); > >> void drm_client_buffer_vunmap(struct drm_client_buffer *buffer); > >> > >> int drm_client_modeset_create(struct drm_client_dev *client); > >> diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h > >> index 0988351d743c..6ecf03601c36 100644 > >> --- a/include/drm/drm_device.h > >> +++ b/include/drm/drm_device.h > >> @@ -381,4 +381,30 @@ struct drm_device { > >> #endif > >> }; > >> > >> +/** > >> + * struct drm_gem_membuf - GEM memory buffer > > > > This isn't gem specific, and membuf feels a bit strange - it's not a > > memory buffer, but more a buffer mapping. I'd call these struct > > drm_bufmap. > > Yep, I didn't like the name either. struct drm_bufmap sounds good. > > As i mentioned on irc, I'd also want to propose this as an update to > dma-buf's vmap. It's not that much more work and should fix the issue > once and for all. If they are interested, struct dma_buf_addr might be a > candidate. Ah if that's the goal, I'd say call it dma_buf_map from the start and have it under the dma-buf documentation section. -Daniel > > > > >> + */ > >> +struct drm_gem_membuf { > >> + union { > >> + /** > >> + * @vaddr: > >> + * > >> + * The virtual address for the buffer in system memory. > >> + */ > >> + void *vaddr; > >> + /** > >> + * @vaddr_iomem: > >> + * > >> + * The virtual address for the buffer in I/O memory. > >> + */ > >> + void __iomem *vaddr_iomem; > >> + }; > >> + /** > >> + * @is_iomem: > >> + * > >> + * True if the memory is located in I/O memory, false otherwise. > >> + */ > >> + bool is_iomem; > >> +}; > > > > I think if we do this we should go all in, i.e. create a new header, and > > then start to add all kinds of helper functions to it, like: > > > > static inline drm_memcpy_to_bufmap(bufmap, ...) > > { > > if (bufmap->is_iomem) > > memcpy_toio(bufmap->vaddr_iomem, ...); > > else > > memcpy(bufmap->vaddr_iomem, ...); > > > > } > > > > totally wrong code of course, but I think you get the idea. If we add > > helpers like this I think a new header file is also warranted, so maybe > > put it all in drm_bufmap.h and then wire up the kerneldoc. > > > > Also I think cc: ttm people for an ack would be good, maybe long-term we > > want to roll this out through ttm too. > > Sure. > > Best regards > Thomas > > > > > Cheers, Daniel > > > > > >> + > >> #endif > >> -- > >> 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 Am 13.08.20 um 12:26 schrieb Daniel Vetter: > On Thu, Aug 06, 2020 at 10:52:37AM +0200, Thomas Zimmermann wrote: >> GEM's vmap interface now wraps memory pointers in struct drm_gem_membuf. >> The structure represents a pointer into the framebuffer, which is either >> in I/O memory or in system memory. The structure contains a flag that >> distinguishes these cases. >> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >> --- >> drivers/gpu/drm/drm_client.c | 25 ++++++++++++------------- >> drivers/gpu/drm/drm_fb_helper.c | 18 +++++++++--------- >> drivers/gpu/drm/drm_gem.c | 19 +++++++++++-------- >> drivers/gpu/drm/drm_internal.h | 5 +++-- >> drivers/gpu/drm/drm_prime.c | 16 ++++++++++------ >> include/drm/drm_client.h | 7 ++++--- >> include/drm/drm_device.h | 26 ++++++++++++++++++++++++++ >> 7 files changed, 75 insertions(+), 41 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c >> index 495f47d23d87..0359b82928c1 100644 >> --- a/drivers/gpu/drm/drm_client.c >> +++ b/drivers/gpu/drm/drm_client.c >> @@ -234,7 +234,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->membuf); >> >> if (buffer->gem) >> drm_gem_object_put(buffer->gem); >> @@ -302,12 +302,13 @@ drm_client_buffer_create(struct drm_client_dev *client, u32 width, u32 height, u >> * Returns: >> * The mapped memory's address >> */ >> -void *drm_client_buffer_vmap(struct drm_client_buffer *buffer) >> +const struct drm_gem_membuf * >> +drm_client_buffer_vmap(struct drm_client_buffer *buffer) >> { >> - void *vaddr; >> + int ret; >> >> - if (buffer->vaddr) >> - return buffer->vaddr; >> + if (buffer->membuf.vaddr) >> + return &buffer->membuf; >> >> /* >> * FIXME: The dependency on GEM here isn't required, we could >> @@ -317,13 +318,11 @@ 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. >> */ >> - vaddr = drm_gem_vmap(buffer->gem); >> - if (IS_ERR(vaddr)) >> - return vaddr; >> - >> - buffer->vaddr = vaddr; >> + ret = drm_gem_vmap(buffer->gem, &buffer->membuf); >> + if (ret) >> + return ERR_PTR(ret); >> >> - return vaddr; >> + return &buffer->membuf; >> } >> EXPORT_SYMBOL(drm_client_buffer_vmap); >> >> @@ -337,8 +336,8 @@ EXPORT_SYMBOL(drm_client_buffer_vmap); >> */ >> void drm_client_buffer_vunmap(struct drm_client_buffer *buffer) >> { >> - drm_gem_vunmap(buffer->gem, buffer->vaddr); >> - buffer->vaddr = NULL; >> + drm_gem_vunmap(buffer->gem, &buffer->membuf); >> + buffer->membuf.vaddr = NULL; >> } >> 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 8697554ccd41..da24874247e7 100644 >> --- a/drivers/gpu/drm/drm_fb_helper.c >> +++ b/drivers/gpu/drm/drm_fb_helper.c >> @@ -394,7 +394,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->membuf.vaddr + offset; >> size_t len = (clip->x2 - clip->x1) * cpp; >> unsigned int y; >> >> @@ -416,7 +416,7 @@ 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; >> + const struct drm_gem_membuf *buf; >> >> spin_lock_irqsave(&helper->dirty_lock, flags); >> clip_copy = *clip; >> @@ -429,8 +429,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)) >> + buf = drm_client_buffer_vmap(helper->buffer); >> + if (IS_ERR(buf)) >> return; >> drm_fb_helper_dirty_blit_real(helper, &clip_copy); >> } >> @@ -2076,7 +2076,7 @@ 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; >> + const struct drm_gem_membuf *membuf; >> >> drm_dbg_kms(dev, "surface width(%d), height(%d) and bpp(%d)\n", >> sizes->surface_width, sizes->surface_height, >> @@ -2112,11 +2112,11 @@ 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); >> + membuf = drm_client_buffer_vmap(fb_helper->buffer); >> + if (IS_ERR(membuf)) >> + return PTR_ERR(membuf); >> >> - fbi->screen_buffer = vaddr; >> + fbi->screen_buffer = membuf->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) >> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c >> index 19d73868490e..36ded6a56fb2 100644 >> --- a/drivers/gpu/drm/drm_gem.c >> +++ b/drivers/gpu/drm/drm_gem.c >> @@ -1220,7 +1220,7 @@ void drm_gem_unpin(struct drm_gem_object *obj) >> obj->dev->driver->gem_prime_unpin(obj); >> } >> >> -void *drm_gem_vmap(struct drm_gem_object *obj) >> +int drm_gem_vmap(struct drm_gem_object *obj, struct drm_gem_membuf *buf) >> { >> void *vaddr; >> >> @@ -1229,23 +1229,26 @@ void *drm_gem_vmap(struct drm_gem_object *obj) >> else if (obj->dev->driver->gem_prime_vmap) >> vaddr = obj->dev->driver->gem_prime_vmap(obj); >> else >> - vaddr = ERR_PTR(-EOPNOTSUPP); >> + return -EOPNOTSUPP; >> >> if (!vaddr) >> - vaddr = ERR_PTR(-ENOMEM); >> + return -ENOMEM; >> + >> + buf->vaddr = vaddr; >> + buf->is_iomem = false; >> >> - return vaddr; >> + return 0; >> } >> >> -void drm_gem_vunmap(struct drm_gem_object *obj, void *vaddr) >> +void drm_gem_vunmap(struct drm_gem_object *obj, const struct drm_gem_membuf *buf) >> { >> - if (!vaddr) >> + if (!buf || !buf->vaddr) >> return; >> >> if (obj->funcs && obj->funcs->vunmap) >> - obj->funcs->vunmap(obj, vaddr); >> + obj->funcs->vunmap(obj, buf->vaddr); >> else if (obj->dev->driver->gem_prime_vunmap) >> - obj->dev->driver->gem_prime_vunmap(obj, vaddr); >> + obj->dev->driver->gem_prime_vunmap(obj, buf->vaddr); >> } >> >> /** >> diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h >> index 8e01caaf95cc..201d71249954 100644 >> --- a/drivers/gpu/drm/drm_internal.h >> +++ b/drivers/gpu/drm/drm_internal.h >> @@ -36,6 +36,7 @@ struct dma_buf; >> struct drm_connector; >> struct drm_crtc; >> struct drm_framebuffer; >> +struct drm_gem_membuf; >> struct drm_gem_object; >> struct drm_master; >> struct drm_minor; >> @@ -186,8 +187,8 @@ void drm_gem_print_info(struct drm_printer *p, unsigned int indent, >> >> int drm_gem_pin(struct drm_gem_object *obj); >> void drm_gem_unpin(struct drm_gem_object *obj); >> -void *drm_gem_vmap(struct drm_gem_object *obj); >> -void drm_gem_vunmap(struct drm_gem_object *obj, void *vaddr); >> +int drm_gem_vmap(struct drm_gem_object *obj, struct drm_gem_membuf *buf); >> +void drm_gem_vunmap(struct drm_gem_object *obj, const struct drm_gem_membuf *buf); >> >> /* drm_debugfs.c drm_debugfs_crc.c */ >> #if defined(CONFIG_DEBUG_FS) >> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c >> index 1693aa7c14b5..d95a39030a93 100644 >> --- a/drivers/gpu/drm/drm_prime.c >> +++ b/drivers/gpu/drm/drm_prime.c >> @@ -671,13 +671,14 @@ EXPORT_SYMBOL(drm_gem_unmap_dma_buf); >> void *drm_gem_dmabuf_vmap(struct dma_buf *dma_buf) >> { >> struct drm_gem_object *obj = dma_buf->priv; >> - void *vaddr; >> + struct drm_gem_membuf buf; >> + int ret; >> >> - vaddr = drm_gem_vmap(obj); >> - if (IS_ERR(vaddr)) >> - vaddr = NULL; >> + ret = drm_gem_vmap(obj, &buf); >> + if (ret) >> + buf.vaddr = NULL; >> >> - return vaddr; >> + return buf.vaddr; >> } >> EXPORT_SYMBOL(drm_gem_dmabuf_vmap); >> >> @@ -692,8 +693,11 @@ EXPORT_SYMBOL(drm_gem_dmabuf_vmap); >> void drm_gem_dmabuf_vunmap(struct dma_buf *dma_buf, void *vaddr) >> { >> struct drm_gem_object *obj = dma_buf->priv; >> + struct drm_gem_membuf buf; >> >> - drm_gem_vunmap(obj, vaddr); >> + buf.vaddr = vaddr; >> + buf.is_iomem = false; >> + drm_gem_vunmap(obj, &buf); >> } >> EXPORT_SYMBOL(drm_gem_dmabuf_vunmap); >> >> diff --git a/include/drm/drm_client.h b/include/drm/drm_client.h >> index 7aaea665bfc2..5ed73c390619 100644 >> --- a/include/drm/drm_client.h >> +++ b/include/drm/drm_client.h >> @@ -14,6 +14,7 @@ struct drm_client_dev; >> struct drm_device; >> struct drm_file; >> struct drm_framebuffer; >> +struct drm_gem_membuf; >> struct drm_gem_object; >> struct drm_minor; >> struct module; >> @@ -141,9 +142,9 @@ struct drm_client_buffer { >> struct drm_gem_object *gem; >> >> /** >> - * @vaddr: Virtual address for the buffer >> + * @membuf: Virtual address for the buffer >> */ >> - void *vaddr; >> + struct drm_gem_membuf membuf; >> >> /** >> * @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); >> +const struct drm_gem_membuf *drm_client_buffer_vmap(struct drm_client_buffer *buffer); >> void drm_client_buffer_vunmap(struct drm_client_buffer *buffer); >> >> int drm_client_modeset_create(struct drm_client_dev *client); >> diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h >> index 0988351d743c..6ecf03601c36 100644 >> --- a/include/drm/drm_device.h >> +++ b/include/drm/drm_device.h >> @@ -381,4 +381,30 @@ struct drm_device { >> #endif >> }; >> >> +/** >> + * struct drm_gem_membuf - GEM memory buffer > > This isn't gem specific, and membuf feels a bit strange - it's not a > memory buffer, but more a buffer mapping. I'd call these struct > drm_bufmap. > >> + */ >> +struct drm_gem_membuf { >> + union { >> + /** >> + * @vaddr: >> + * >> + * The virtual address for the buffer in system memory. >> + */ >> + void *vaddr; >> + /** >> + * @vaddr_iomem: >> + * >> + * The virtual address for the buffer in I/O memory. >> + */ >> + void __iomem *vaddr_iomem; >> + }; >> + /** >> + * @is_iomem: >> + * >> + * True if the memory is located in I/O memory, false otherwise. >> + */ >> + bool is_iomem; >> +}; > > I think if we do this we should go all in, i.e. create a new header, and > then start to add all kinds of helper functions to it, like: > > static inline drm_memcpy_to_bufmap(bufmap, ...) > { > if (bufmap->is_iomem) > memcpy_toio(bufmap->vaddr_iomem, ...); > else > memcpy(bufmap->vaddr_iomem, ...); > > } > > totally wrong code of course, but I think you get the idea. If we add > helpers like this I think a new header file is also warranted, so maybe > put it all in drm_bufmap.h and then wire up the kerneldoc. Just a note: while converting the fbdev blit code, I found it easier to open-code the test for is_iomem and have two separate loops. Doing the same with drm_memcpy_to_bufmap would also require helpers for pointer arithmetics and array indexing to make it comfortable to use. Best regards Thomas > > Also I think cc: ttm people for an ack would be good, maybe long-term we > want to roll this out through ttm too. > > Cheers, Daniel > > >> + >> #endif >> -- >> 2.28.0 >> >
diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c index 495f47d23d87..0359b82928c1 100644 --- a/drivers/gpu/drm/drm_client.c +++ b/drivers/gpu/drm/drm_client.c @@ -234,7 +234,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->membuf); if (buffer->gem) drm_gem_object_put(buffer->gem); @@ -302,12 +302,13 @@ drm_client_buffer_create(struct drm_client_dev *client, u32 width, u32 height, u * Returns: * The mapped memory's address */ -void *drm_client_buffer_vmap(struct drm_client_buffer *buffer) +const struct drm_gem_membuf * +drm_client_buffer_vmap(struct drm_client_buffer *buffer) { - void *vaddr; + int ret; - if (buffer->vaddr) - return buffer->vaddr; + if (buffer->membuf.vaddr) + return &buffer->membuf; /* * FIXME: The dependency on GEM here isn't required, we could @@ -317,13 +318,11 @@ 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. */ - vaddr = drm_gem_vmap(buffer->gem); - if (IS_ERR(vaddr)) - return vaddr; - - buffer->vaddr = vaddr; + ret = drm_gem_vmap(buffer->gem, &buffer->membuf); + if (ret) + return ERR_PTR(ret); - return vaddr; + return &buffer->membuf; } EXPORT_SYMBOL(drm_client_buffer_vmap); @@ -337,8 +336,8 @@ EXPORT_SYMBOL(drm_client_buffer_vmap); */ void drm_client_buffer_vunmap(struct drm_client_buffer *buffer) { - drm_gem_vunmap(buffer->gem, buffer->vaddr); - buffer->vaddr = NULL; + drm_gem_vunmap(buffer->gem, &buffer->membuf); + buffer->membuf.vaddr = NULL; } 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 8697554ccd41..da24874247e7 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -394,7 +394,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->membuf.vaddr + offset; size_t len = (clip->x2 - clip->x1) * cpp; unsigned int y; @@ -416,7 +416,7 @@ 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; + const struct drm_gem_membuf *buf; spin_lock_irqsave(&helper->dirty_lock, flags); clip_copy = *clip; @@ -429,8 +429,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)) + buf = drm_client_buffer_vmap(helper->buffer); + if (IS_ERR(buf)) return; drm_fb_helper_dirty_blit_real(helper, &clip_copy); } @@ -2076,7 +2076,7 @@ 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; + const struct drm_gem_membuf *membuf; drm_dbg_kms(dev, "surface width(%d), height(%d) and bpp(%d)\n", sizes->surface_width, sizes->surface_height, @@ -2112,11 +2112,11 @@ 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); + membuf = drm_client_buffer_vmap(fb_helper->buffer); + if (IS_ERR(membuf)) + return PTR_ERR(membuf); - fbi->screen_buffer = vaddr; + fbi->screen_buffer = membuf->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) diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 19d73868490e..36ded6a56fb2 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -1220,7 +1220,7 @@ void drm_gem_unpin(struct drm_gem_object *obj) obj->dev->driver->gem_prime_unpin(obj); } -void *drm_gem_vmap(struct drm_gem_object *obj) +int drm_gem_vmap(struct drm_gem_object *obj, struct drm_gem_membuf *buf) { void *vaddr; @@ -1229,23 +1229,26 @@ void *drm_gem_vmap(struct drm_gem_object *obj) else if (obj->dev->driver->gem_prime_vmap) vaddr = obj->dev->driver->gem_prime_vmap(obj); else - vaddr = ERR_PTR(-EOPNOTSUPP); + return -EOPNOTSUPP; if (!vaddr) - vaddr = ERR_PTR(-ENOMEM); + return -ENOMEM; + + buf->vaddr = vaddr; + buf->is_iomem = false; - return vaddr; + return 0; } -void drm_gem_vunmap(struct drm_gem_object *obj, void *vaddr) +void drm_gem_vunmap(struct drm_gem_object *obj, const struct drm_gem_membuf *buf) { - if (!vaddr) + if (!buf || !buf->vaddr) return; if (obj->funcs && obj->funcs->vunmap) - obj->funcs->vunmap(obj, vaddr); + obj->funcs->vunmap(obj, buf->vaddr); else if (obj->dev->driver->gem_prime_vunmap) - obj->dev->driver->gem_prime_vunmap(obj, vaddr); + obj->dev->driver->gem_prime_vunmap(obj, buf->vaddr); } /** diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h index 8e01caaf95cc..201d71249954 100644 --- a/drivers/gpu/drm/drm_internal.h +++ b/drivers/gpu/drm/drm_internal.h @@ -36,6 +36,7 @@ struct dma_buf; struct drm_connector; struct drm_crtc; struct drm_framebuffer; +struct drm_gem_membuf; struct drm_gem_object; struct drm_master; struct drm_minor; @@ -186,8 +187,8 @@ void drm_gem_print_info(struct drm_printer *p, unsigned int indent, int drm_gem_pin(struct drm_gem_object *obj); void drm_gem_unpin(struct drm_gem_object *obj); -void *drm_gem_vmap(struct drm_gem_object *obj); -void drm_gem_vunmap(struct drm_gem_object *obj, void *vaddr); +int drm_gem_vmap(struct drm_gem_object *obj, struct drm_gem_membuf *buf); +void drm_gem_vunmap(struct drm_gem_object *obj, const struct drm_gem_membuf *buf); /* drm_debugfs.c drm_debugfs_crc.c */ #if defined(CONFIG_DEBUG_FS) diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c index 1693aa7c14b5..d95a39030a93 100644 --- a/drivers/gpu/drm/drm_prime.c +++ b/drivers/gpu/drm/drm_prime.c @@ -671,13 +671,14 @@ EXPORT_SYMBOL(drm_gem_unmap_dma_buf); void *drm_gem_dmabuf_vmap(struct dma_buf *dma_buf) { struct drm_gem_object *obj = dma_buf->priv; - void *vaddr; + struct drm_gem_membuf buf; + int ret; - vaddr = drm_gem_vmap(obj); - if (IS_ERR(vaddr)) - vaddr = NULL; + ret = drm_gem_vmap(obj, &buf); + if (ret) + buf.vaddr = NULL; - return vaddr; + return buf.vaddr; } EXPORT_SYMBOL(drm_gem_dmabuf_vmap); @@ -692,8 +693,11 @@ EXPORT_SYMBOL(drm_gem_dmabuf_vmap); void drm_gem_dmabuf_vunmap(struct dma_buf *dma_buf, void *vaddr) { struct drm_gem_object *obj = dma_buf->priv; + struct drm_gem_membuf buf; - drm_gem_vunmap(obj, vaddr); + buf.vaddr = vaddr; + buf.is_iomem = false; + drm_gem_vunmap(obj, &buf); } EXPORT_SYMBOL(drm_gem_dmabuf_vunmap); diff --git a/include/drm/drm_client.h b/include/drm/drm_client.h index 7aaea665bfc2..5ed73c390619 100644 --- a/include/drm/drm_client.h +++ b/include/drm/drm_client.h @@ -14,6 +14,7 @@ struct drm_client_dev; struct drm_device; struct drm_file; struct drm_framebuffer; +struct drm_gem_membuf; struct drm_gem_object; struct drm_minor; struct module; @@ -141,9 +142,9 @@ struct drm_client_buffer { struct drm_gem_object *gem; /** - * @vaddr: Virtual address for the buffer + * @membuf: Virtual address for the buffer */ - void *vaddr; + struct drm_gem_membuf membuf; /** * @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); +const struct drm_gem_membuf *drm_client_buffer_vmap(struct drm_client_buffer *buffer); void drm_client_buffer_vunmap(struct drm_client_buffer *buffer); int drm_client_modeset_create(struct drm_client_dev *client); diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h index 0988351d743c..6ecf03601c36 100644 --- a/include/drm/drm_device.h +++ b/include/drm/drm_device.h @@ -381,4 +381,30 @@ struct drm_device { #endif }; +/** + * struct drm_gem_membuf - GEM memory buffer + */ +struct drm_gem_membuf { + union { + /** + * @vaddr: + * + * The virtual address for the buffer in system memory. + */ + void *vaddr; + /** + * @vaddr_iomem: + * + * The virtual address for the buffer in I/O memory. + */ + void __iomem *vaddr_iomem; + }; + /** + * @is_iomem: + * + * True if the memory is located in I/O memory, false otherwise. + */ + bool is_iomem; +}; + #endif
GEM's vmap interface now wraps memory pointers in struct drm_gem_membuf. The structure represents a pointer into the framebuffer, which is either in I/O memory or in system memory. The structure contains a flag that distinguishes these cases. Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> --- drivers/gpu/drm/drm_client.c | 25 ++++++++++++------------- drivers/gpu/drm/drm_fb_helper.c | 18 +++++++++--------- drivers/gpu/drm/drm_gem.c | 19 +++++++++++-------- drivers/gpu/drm/drm_internal.h | 5 +++-- drivers/gpu/drm/drm_prime.c | 16 ++++++++++------ include/drm/drm_client.h | 7 ++++--- include/drm/drm_device.h | 26 ++++++++++++++++++++++++++ 7 files changed, 75 insertions(+), 41 deletions(-)