| Message ID | 20191024144237.8898-4-tzimmermann@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/udl: Convert to generic fbdev emulation | expand |
On Thu, Oct 24, 2019 at 04:42:35PM +0200, Thomas Zimmermann wrote: > Implementing vmap() and vunmap() of struct drm_gem_object_funcs is > required by generic fbdev emulation. Supporting free() is easy as > well. More udl-specific interfaces can probably be replaced by GEM > object functions in later patches. > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > --- > drivers/gpu/drm/udl/udl_gem.c | 34 ++++++++++++++++++++++++++++++++++ > 1 file changed, 34 insertions(+) > > diff --git a/drivers/gpu/drm/udl/udl_gem.c b/drivers/gpu/drm/udl/udl_gem.c > index 3ea0cd9ae2d6..6ca097c270d6 100644 > --- a/drivers/gpu/drm/udl/udl_gem.c > +++ b/drivers/gpu/drm/udl/udl_gem.c > @@ -11,6 +11,8 @@ > > #include "udl_drv.h" > > +static const struct drm_gem_object_funcs udl_gem_object_funcs; > + > struct udl_gem_object *udl_gem_alloc_object(struct drm_device *dev, > size_t size) > { > @@ -25,6 +27,7 @@ struct udl_gem_object *udl_gem_alloc_object(struct drm_device *dev, > return NULL; > } > > + obj->base.funcs = &udl_gem_object_funcs; > obj->flags = UDL_BO_CACHEABLE; > return obj; > } > @@ -230,3 +233,34 @@ int udl_gem_mmap(struct drm_file *file, struct drm_device *dev, > mutex_unlock(&udl->gem_lock); > return ret; > } > + > +/* > + * Helpers for struct drm_gem_object_funcs > + */ > + > +static void udl_gem_object_free(struct drm_gem_object *obj) > +{ > + udl_gem_free_object(obj); > +} > + > +static void *udl_gem_object_vmap(struct drm_gem_object *obj) > +{ > + struct udl_gem_object *uobj = to_udl_bo(obj); > + int ret; > + > + ret = udl_gem_vmap(uobj); > + if (ret) > + return ERR_PTR(ret); > + return uobj->vmapping; > +} > + > +static void udl_gem_object_vunmap(struct drm_gem_object *obj, void *vaddr) > +{ > + return udl_gem_vunmap(to_udl_bo(obj)); > +} > + > +static const struct drm_gem_object_funcs udl_gem_object_funcs = { > + .free = udl_gem_object_free, > + .vmap = udl_gem_object_vmap, > + .vunmap = udl_gem_object_vunmap, Yeah this doesn't work, you need to refcount the vmap here I think. I'd say simpler to first cut over to shmem helpers. -Daniel
Hi Am 25.10.19 um 09:40 schrieb Daniel Vetter: > On Thu, Oct 24, 2019 at 04:42:35PM +0200, Thomas Zimmermann wrote: >> Implementing vmap() and vunmap() of struct drm_gem_object_funcs is >> required by generic fbdev emulation. Supporting free() is easy as >> well. More udl-specific interfaces can probably be replaced by GEM >> object functions in later patches. >> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >> --- >> drivers/gpu/drm/udl/udl_gem.c | 34 ++++++++++++++++++++++++++++++++++ >> 1 file changed, 34 insertions(+) >> >> diff --git a/drivers/gpu/drm/udl/udl_gem.c b/drivers/gpu/drm/udl/udl_gem.c >> index 3ea0cd9ae2d6..6ca097c270d6 100644 >> --- a/drivers/gpu/drm/udl/udl_gem.c >> +++ b/drivers/gpu/drm/udl/udl_gem.c >> @@ -11,6 +11,8 @@ >> >> #include "udl_drv.h" >> >> +static const struct drm_gem_object_funcs udl_gem_object_funcs; >> + >> struct udl_gem_object *udl_gem_alloc_object(struct drm_device *dev, >> size_t size) >> { >> @@ -25,6 +27,7 @@ struct udl_gem_object *udl_gem_alloc_object(struct drm_device *dev, >> return NULL; >> } >> >> + obj->base.funcs = &udl_gem_object_funcs; >> obj->flags = UDL_BO_CACHEABLE; >> return obj; >> } >> @@ -230,3 +233,34 @@ int udl_gem_mmap(struct drm_file *file, struct drm_device *dev, >> mutex_unlock(&udl->gem_lock); >> return ret; >> } >> + >> +/* >> + * Helpers for struct drm_gem_object_funcs >> + */ >> + >> +static void udl_gem_object_free(struct drm_gem_object *obj) >> +{ >> + udl_gem_free_object(obj); >> +} >> + >> +static void *udl_gem_object_vmap(struct drm_gem_object *obj) >> +{ >> + struct udl_gem_object *uobj = to_udl_bo(obj); >> + int ret; >> + >> + ret = udl_gem_vmap(uobj); >> + if (ret) >> + return ERR_PTR(ret); >> + return uobj->vmapping; >> +} >> + >> +static void udl_gem_object_vunmap(struct drm_gem_object *obj, void *vaddr) >> +{ >> + return udl_gem_vunmap(to_udl_bo(obj)); >> +} >> + >> +static const struct drm_gem_object_funcs udl_gem_object_funcs = { >> + .free = udl_gem_object_free, >> + .vmap = udl_gem_object_vmap, >> + .vunmap = udl_gem_object_vunmap, > > Yeah this doesn't work, you need to refcount the vmap here I think. I'd I see. Should have thought of that... > say simpler to first cut over to shmem helpers. Right. I was already attempting the conversion and the udl gem is more or less the same as shmem, except for the flags field. [1] The flag signals page attributes for mmap-ing [2]. For prime-imported BOs its is set to writecombining, and for local BOs it is set to cached. Shmem always maps with writecombining. I don't see why udl BOs are special wrt to mmap-ing. It seems to be some kind of optimization. Do you have an idea? Best regards Thomas [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/udl/udl_drv.h?h=v5.3#n78 [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/udl/udl_gem.c?h=v5.3#n57 > -Daniel >
On Fri, Oct 25, 2019 at 9:59 AM Thomas Zimmermann <tzimmermann@suse.de> wrote > > Hi > > Am 25.10.19 um 09:40 schrieb Daniel Vetter: > > On Thu, Oct 24, 2019 at 04:42:35PM +0200, Thomas Zimmermann wrote: > >> Implementing vmap() and vunmap() of struct drm_gem_object_funcs is > >> required by generic fbdev emulation. Supporting free() is easy as > >> well. More udl-specific interfaces can probably be replaced by GEM > >> object functions in later patches. > >> > >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > >> --- > >> drivers/gpu/drm/udl/udl_gem.c | 34 ++++++++++++++++++++++++++++++++++ > >> 1 file changed, 34 insertions(+) > >> > >> diff --git a/drivers/gpu/drm/udl/udl_gem.c b/drivers/gpu/drm/udl/udl_gem.c > >> index 3ea0cd9ae2d6..6ca097c270d6 100644 > >> --- a/drivers/gpu/drm/udl/udl_gem.c > >> +++ b/drivers/gpu/drm/udl/udl_gem.c > >> @@ -11,6 +11,8 @@ > >> > >> #include "udl_drv.h" > >> > >> +static const struct drm_gem_object_funcs udl_gem_object_funcs; > >> + > >> struct udl_gem_object *udl_gem_alloc_object(struct drm_device *dev, > >> size_t size) > >> { > >> @@ -25,6 +27,7 @@ struct udl_gem_object *udl_gem_alloc_object(struct drm_device *dev, > >> return NULL; > >> } > >> > >> + obj->base.funcs = &udl_gem_object_funcs; > >> obj->flags = UDL_BO_CACHEABLE; > >> return obj; > >> } > >> @@ -230,3 +233,34 @@ int udl_gem_mmap(struct drm_file *file, struct drm_device *dev, > >> mutex_unlock(&udl->gem_lock); > >> return ret; > >> } > >> + > >> +/* > >> + * Helpers for struct drm_gem_object_funcs > >> + */ > >> + > >> +static void udl_gem_object_free(struct drm_gem_object *obj) > >> +{ > >> + udl_gem_free_object(obj); > >> +} > >> + > >> +static void *udl_gem_object_vmap(struct drm_gem_object *obj) > >> +{ > >> + struct udl_gem_object *uobj = to_udl_bo(obj); > >> + int ret; > >> + > >> + ret = udl_gem_vmap(uobj); > >> + if (ret) > >> + return ERR_PTR(ret); > >> + return uobj->vmapping; > >> +} > >> + > >> +static void udl_gem_object_vunmap(struct drm_gem_object *obj, void *vaddr) > >> +{ > >> + return udl_gem_vunmap(to_udl_bo(obj)); > >> +} > >> + > >> +static const struct drm_gem_object_funcs udl_gem_object_funcs = { > >> + .free = udl_gem_object_free, > >> + .vmap = udl_gem_object_vmap, > >> + .vunmap = udl_gem_object_vunmap, > > > > Yeah this doesn't work, you need to refcount the vmap here I think. I'd > > I see. Should have thought of that... > > > say simpler to first cut over to shmem helpers. > > Right. I was already attempting the conversion and the udl gem is more > or less the same as shmem, except for the flags field. [1] The flag > signals page attributes for mmap-ing [2]. For prime-imported BOs its is > set to writecombining, and for local BOs it is set to cached. Shmem > always maps with writecombining. > > I don't see why udl BOs are special wrt to mmap-ing. It seems to be some > kind of optimization. Do you have an idea? > > Best regards > Thomas > > [1] > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/udl/udl_drv.h?h=v5.3#n78 > [2] > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/udl/udl_gem.c?h=v5.3#n57 udl doesn't set prefer_shadow = 1, which means compositors will render directly into the buffer. And for that you want caching. For imported dma-buf otoh you want wc to make sure it's coherent. Otherwise I guess udl would need to get the dma_buf_begin/end_cpu_access calls added (and that would probably be faster than going with wc for everything). So we do want cachable, it's going to suck otherwise. But that should be fairly easy to do by overwriting the obj->mmap callback and wrapping it around the shmem one. What surprises me more is that udl supports mmap on imported dma-buf, that's some real quirky stuff. Not sure there's really other drivers doing that. Might be easier to rip that out :-) -Daniel
Hi Am 25.10.19 um 11:28 schrieb Daniel Vetter: > On Fri, Oct 25, 2019 at 9:59 AM Thomas Zimmermann <tzimmermann@suse.de> wrote >> >> Hi >> >> Am 25.10.19 um 09:40 schrieb Daniel Vetter: >>> On Thu, Oct 24, 2019 at 04:42:35PM +0200, Thomas Zimmermann wrote: >>>> Implementing vmap() and vunmap() of struct drm_gem_object_funcs is >>>> required by generic fbdev emulation. Supporting free() is easy as >>>> well. More udl-specific interfaces can probably be replaced by GEM >>>> object functions in later patches. >>>> >>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >>>> --- >>>> drivers/gpu/drm/udl/udl_gem.c | 34 ++++++++++++++++++++++++++++++++++ >>>> 1 file changed, 34 insertions(+) >>>> >>>> diff --git a/drivers/gpu/drm/udl/udl_gem.c b/drivers/gpu/drm/udl/udl_gem.c >>>> index 3ea0cd9ae2d6..6ca097c270d6 100644 >>>> --- a/drivers/gpu/drm/udl/udl_gem.c >>>> +++ b/drivers/gpu/drm/udl/udl_gem.c >>>> @@ -11,6 +11,8 @@ >>>> >>>> #include "udl_drv.h" >>>> >>>> +static const struct drm_gem_object_funcs udl_gem_object_funcs; >>>> + >>>> struct udl_gem_object *udl_gem_alloc_object(struct drm_device *dev, >>>> size_t size) >>>> { >>>> @@ -25,6 +27,7 @@ struct udl_gem_object *udl_gem_alloc_object(struct drm_device *dev, >>>> return NULL; >>>> } >>>> >>>> + obj->base.funcs = &udl_gem_object_funcs; >>>> obj->flags = UDL_BO_CACHEABLE; >>>> return obj; >>>> } >>>> @@ -230,3 +233,34 @@ int udl_gem_mmap(struct drm_file *file, struct drm_device *dev, >>>> mutex_unlock(&udl->gem_lock); >>>> return ret; >>>> } >>>> + >>>> +/* >>>> + * Helpers for struct drm_gem_object_funcs >>>> + */ >>>> + >>>> +static void udl_gem_object_free(struct drm_gem_object *obj) >>>> +{ >>>> + udl_gem_free_object(obj); >>>> +} >>>> + >>>> +static void *udl_gem_object_vmap(struct drm_gem_object *obj) >>>> +{ >>>> + struct udl_gem_object *uobj = to_udl_bo(obj); >>>> + int ret; >>>> + >>>> + ret = udl_gem_vmap(uobj); >>>> + if (ret) >>>> + return ERR_PTR(ret); >>>> + return uobj->vmapping; >>>> +} >>>> + >>>> +static void udl_gem_object_vunmap(struct drm_gem_object *obj, void *vaddr) >>>> +{ >>>> + return udl_gem_vunmap(to_udl_bo(obj)); >>>> +} >>>> + >>>> +static const struct drm_gem_object_funcs udl_gem_object_funcs = { >>>> + .free = udl_gem_object_free, >>>> + .vmap = udl_gem_object_vmap, >>>> + .vunmap = udl_gem_object_vunmap, >>> >>> Yeah this doesn't work, you need to refcount the vmap here I think. I'd >> >> I see. Should have thought of that... >> >>> say simpler to first cut over to shmem helpers. >> >> Right. I was already attempting the conversion and the udl gem is more >> or less the same as shmem, except for the flags field. [1] The flag >> signals page attributes for mmap-ing [2]. For prime-imported BOs its is >> set to writecombining, and for local BOs it is set to cached. Shmem >> always maps with writecombining. >> >> I don't see why udl BOs are special wrt to mmap-ing. It seems to be some >> kind of optimization. Do you have an idea? >> >> Best regards >> Thomas >> >> [1] >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/udl/udl_drv.h?h=v5.3#n78 >> [2] >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/udl/udl_gem.c?h=v5.3#n57 > > udl doesn't set prefer_shadow = 1, which means compositors will render > directly into the buffer. And for that you want caching. For imported > dma-buf otoh you want wc to make sure it's coherent. Otherwise I guess > udl would need to get the dma_buf_begin/end_cpu_access calls added > (and that would probably be faster than going with wc for everything). > So we do want cachable, it's going to suck otherwise. Thanks for clarifying. Actually, it does have these calls in its dirty handler. [1] > > But that should be fairly easy to do by overwriting the obj->mmap > callback and wrapping it around the shmem one. Is there a reason why shmem doesn't implement the wc-vs.cached logic? Could this be added? (I'm just trying to understand the available options here before attempting to do a conversion.) Best regards Thomas > > What surprises me more is that udl supports mmap on imported dma-buf, > that's some real quirky stuff. Not sure there's really other drivers > doing that. Might be easier to rip that out :-) > -Daniel > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/udl/udl_fb.c?h=v5.3#n293
Den 25.10.2019 12.12, skrev Thomas Zimmermann: > Hi > > Am 25.10.19 um 11:28 schrieb Daniel Vetter: >> On Fri, Oct 25, 2019 at 9:59 AM Thomas Zimmermann <tzimmermann@suse.de> wrote >>> >>> Hi >>> >>> Am 25.10.19 um 09:40 schrieb Daniel Vetter: >>>> On Thu, Oct 24, 2019 at 04:42:35PM +0200, Thomas Zimmermann wrote: >>>>> Implementing vmap() and vunmap() of struct drm_gem_object_funcs is >>>>> required by generic fbdev emulation. Supporting free() is easy as >>>>> well. More udl-specific interfaces can probably be replaced by GEM >>>>> object functions in later patches. >>>>> >>>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >>>>> --- >>>>> drivers/gpu/drm/udl/udl_gem.c | 34 ++++++++++++++++++++++++++++++++++ >>>>> 1 file changed, 34 insertions(+) >>>>> >>>>> diff --git a/drivers/gpu/drm/udl/udl_gem.c b/drivers/gpu/drm/udl/udl_gem.c >>>>> index 3ea0cd9ae2d6..6ca097c270d6 100644 >>>>> --- a/drivers/gpu/drm/udl/udl_gem.c >>>>> +++ b/drivers/gpu/drm/udl/udl_gem.c >>>>> @@ -11,6 +11,8 @@ >>>>> >>>>> #include "udl_drv.h" >>>>> >>>>> +static const struct drm_gem_object_funcs udl_gem_object_funcs; >>>>> + >>>>> struct udl_gem_object *udl_gem_alloc_object(struct drm_device *dev, >>>>> size_t size) >>>>> { >>>>> @@ -25,6 +27,7 @@ struct udl_gem_object *udl_gem_alloc_object(struct drm_device *dev, >>>>> return NULL; >>>>> } >>>>> >>>>> + obj->base.funcs = &udl_gem_object_funcs; >>>>> obj->flags = UDL_BO_CACHEABLE; >>>>> return obj; >>>>> } >>>>> @@ -230,3 +233,34 @@ int udl_gem_mmap(struct drm_file *file, struct drm_device *dev, >>>>> mutex_unlock(&udl->gem_lock); >>>>> return ret; >>>>> } >>>>> + >>>>> +/* >>>>> + * Helpers for struct drm_gem_object_funcs >>>>> + */ >>>>> + >>>>> +static void udl_gem_object_free(struct drm_gem_object *obj) >>>>> +{ >>>>> + udl_gem_free_object(obj); >>>>> +} >>>>> + >>>>> +static void *udl_gem_object_vmap(struct drm_gem_object *obj) >>>>> +{ >>>>> + struct udl_gem_object *uobj = to_udl_bo(obj); >>>>> + int ret; >>>>> + >>>>> + ret = udl_gem_vmap(uobj); >>>>> + if (ret) >>>>> + return ERR_PTR(ret); >>>>> + return uobj->vmapping; >>>>> +} >>>>> + >>>>> +static void udl_gem_object_vunmap(struct drm_gem_object *obj, void *vaddr) >>>>> +{ >>>>> + return udl_gem_vunmap(to_udl_bo(obj)); >>>>> +} >>>>> + >>>>> +static const struct drm_gem_object_funcs udl_gem_object_funcs = { >>>>> + .free = udl_gem_object_free, >>>>> + .vmap = udl_gem_object_vmap, >>>>> + .vunmap = udl_gem_object_vunmap, >>>> >>>> Yeah this doesn't work, you need to refcount the vmap here I think. I'd >>> >>> I see. Should have thought of that... >>> >>>> say simpler to first cut over to shmem helpers. >>> >>> Right. I was already attempting the conversion and the udl gem is more >>> or less the same as shmem, except for the flags field. [1] The flag >>> signals page attributes for mmap-ing [2]. For prime-imported BOs its is >>> set to writecombining, and for local BOs it is set to cached. Shmem >>> always maps with writecombining. >>> >>> I don't see why udl BOs are special wrt to mmap-ing. It seems to be some >>> kind of optimization. Do you have an idea? >>> >>> Best regards >>> Thomas >>> >>> [1] >>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/udl/udl_drv.h?h=v5.3#n78 >>> [2] >>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/udl/udl_gem.c?h=v5.3#n57 >> >> udl doesn't set prefer_shadow = 1, which means compositors will render >> directly into the buffer. And for that you want caching. For imported >> dma-buf otoh you want wc to make sure it's coherent. Otherwise I guess >> udl would need to get the dma_buf_begin/end_cpu_access calls added >> (and that would probably be faster than going with wc for everything). >> So we do want cachable, it's going to suck otherwise. > > Thanks for clarifying. Actually, it does have these calls in its dirty > handler. [1] > >> >> But that should be fairly easy to do by overwriting the obj->mmap >> callback and wrapping it around the shmem one. > > Is there a reason why shmem doesn't implement the wc-vs.cached logic? > Could this be added? > I had a flag to set this in the initial version of the shmem helper modeled after udl, but Thomas Hellstrom brought up a question and it was dropped. The issue was beyond my understanding: [PATCH v3 0/2] drm: Add shmem GEM library https://lists.freedesktop.org/archives/dri-devel/2018-September/190001.html In tinydrm I have sped up uncached writes on arm by copying one pixel line at a time into a temporary buffer before accessing the individual bytes. See drm_fb_swab16(). Noralf. > (I'm just trying to understand the available options here before > attempting to do a conversion.) > > Best regards > Thomas > >> >> What surprises me more is that udl supports mmap on imported dma-buf, >> that's some real quirky stuff. Not sure there's really other drivers >> doing that. Might be easier to rip that out :-) >> -Daniel >> > > [1] > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/udl/udl_fb.c?h=v5.3#n293 > > > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel >
Den 25.10.2019 13.44, skrev Noralf Trønnes: > > > Den 25.10.2019 12.12, skrev Thomas Zimmermann: >> Hi >> >> Am 25.10.19 um 11:28 schrieb Daniel Vetter: >>> On Fri, Oct 25, 2019 at 9:59 AM Thomas Zimmermann <tzimmermann@suse.de> wrote >>>> >>>> Hi >>>> >>>> Am 25.10.19 um 09:40 schrieb Daniel Vetter: >>>>> On Thu, Oct 24, 2019 at 04:42:35PM +0200, Thomas Zimmermann wrote: >>>>>> Implementing vmap() and vunmap() of struct drm_gem_object_funcs is >>>>>> required by generic fbdev emulation. Supporting free() is easy as >>>>>> well. More udl-specific interfaces can probably be replaced by GEM >>>>>> object functions in later patches. >>>>>> >>>>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >>>>>> --- >>>>>> drivers/gpu/drm/udl/udl_gem.c | 34 ++++++++++++++++++++++++++++++++++ >>>>>> 1 file changed, 34 insertions(+) >>>>>> >>>>>> diff --git a/drivers/gpu/drm/udl/udl_gem.c b/drivers/gpu/drm/udl/udl_gem.c >>>>>> index 3ea0cd9ae2d6..6ca097c270d6 100644 >>>>>> --- a/drivers/gpu/drm/udl/udl_gem.c >>>>>> +++ b/drivers/gpu/drm/udl/udl_gem.c >>>>>> @@ -11,6 +11,8 @@ >>>>>> >>>>>> #include "udl_drv.h" >>>>>> >>>>>> +static const struct drm_gem_object_funcs udl_gem_object_funcs; >>>>>> + >>>>>> struct udl_gem_object *udl_gem_alloc_object(struct drm_device *dev, >>>>>> size_t size) >>>>>> { >>>>>> @@ -25,6 +27,7 @@ struct udl_gem_object *udl_gem_alloc_object(struct drm_device *dev, >>>>>> return NULL; >>>>>> } >>>>>> >>>>>> + obj->base.funcs = &udl_gem_object_funcs; >>>>>> obj->flags = UDL_BO_CACHEABLE; >>>>>> return obj; >>>>>> } >>>>>> @@ -230,3 +233,34 @@ int udl_gem_mmap(struct drm_file *file, struct drm_device *dev, >>>>>> mutex_unlock(&udl->gem_lock); >>>>>> return ret; >>>>>> } >>>>>> + >>>>>> +/* >>>>>> + * Helpers for struct drm_gem_object_funcs >>>>>> + */ >>>>>> + >>>>>> +static void udl_gem_object_free(struct drm_gem_object *obj) >>>>>> +{ >>>>>> + udl_gem_free_object(obj); >>>>>> +} >>>>>> + >>>>>> +static void *udl_gem_object_vmap(struct drm_gem_object *obj) >>>>>> +{ >>>>>> + struct udl_gem_object *uobj = to_udl_bo(obj); >>>>>> + int ret; >>>>>> + >>>>>> + ret = udl_gem_vmap(uobj); >>>>>> + if (ret) >>>>>> + return ERR_PTR(ret); >>>>>> + return uobj->vmapping; >>>>>> +} >>>>>> + >>>>>> +static void udl_gem_object_vunmap(struct drm_gem_object *obj, void *vaddr) >>>>>> +{ >>>>>> + return udl_gem_vunmap(to_udl_bo(obj)); >>>>>> +} >>>>>> + >>>>>> +static const struct drm_gem_object_funcs udl_gem_object_funcs = { >>>>>> + .free = udl_gem_object_free, >>>>>> + .vmap = udl_gem_object_vmap, >>>>>> + .vunmap = udl_gem_object_vunmap, >>>>> >>>>> Yeah this doesn't work, you need to refcount the vmap here I think. I'd >>>> >>>> I see. Should have thought of that... >>>> >>>>> say simpler to first cut over to shmem helpers. >>>> >>>> Right. I was already attempting the conversion and the udl gem is more >>>> or less the same as shmem, except for the flags field. [1] The flag >>>> signals page attributes for mmap-ing [2]. For prime-imported BOs its is >>>> set to writecombining, and for local BOs it is set to cached. Shmem >>>> always maps with writecombining. >>>> >>>> I don't see why udl BOs are special wrt to mmap-ing. It seems to be some >>>> kind of optimization. Do you have an idea? >>>> >>>> Best regards >>>> Thomas >>>> >>>> [1] >>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/udl/udl_drv.h?h=v5.3#n78 >>>> [2] >>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/udl/udl_gem.c?h=v5.3#n57 >>> >>> udl doesn't set prefer_shadow = 1, which means compositors will render >>> directly into the buffer. And for that you want caching. For imported >>> dma-buf otoh you want wc to make sure it's coherent. Otherwise I guess >>> udl would need to get the dma_buf_begin/end_cpu_access calls added >>> (and that would probably be faster than going with wc for everything). >>> So we do want cachable, it's going to suck otherwise. >> >> Thanks for clarifying. Actually, it does have these calls in its dirty >> handler. [1] >> >>> >>> But that should be fairly easy to do by overwriting the obj->mmap >>> callback and wrapping it around the shmem one. >> >> Is there a reason why shmem doesn't implement the wc-vs.cached logic? >> Could this be added? >> > > I had a flag to set this in the initial version of the shmem helper > modeled after udl, but Thomas Hellstrom brought up a question and it was > dropped. The issue was beyond my understanding: > > [PATCH v3 0/2] drm: Add shmem GEM library > https://lists.freedesktop.org/archives/dri-devel/2018-September/190001.html > > In tinydrm I have sped up uncached writes on arm by copying one pixel s/writes/reads/ > line at a time into a temporary buffer before accessing the individual > bytes. See drm_fb_swab16(). > > Noralf. > >> (I'm just trying to understand the available options here before >> attempting to do a conversion.) >> >> Best regards >> Thomas >> >>> >>> What surprises me more is that udl supports mmap on imported dma-buf, >>> that's some real quirky stuff. Not sure there's really other drivers >>> doing that. Might be easier to rip that out :-) >>> -Daniel >>> >> >> [1] >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/udl/udl_fb.c?h=v5.3#n293 >> >> >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel >> > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel >
(cc: Gerd) Hi Am 25.10.19 um 13:44 schrieb Noralf Trønnes: > > > Den 25.10.2019 12.12, skrev Thomas Zimmermann: >> Hi >> >> Am 25.10.19 um 11:28 schrieb Daniel Vetter: >>> On Fri, Oct 25, 2019 at 9:59 AM Thomas Zimmermann <tzimmermann@suse.de> wrote >>>> >>>> Hi >>>> >>>> Am 25.10.19 um 09:40 schrieb Daniel Vetter: >>>>> On Thu, Oct 24, 2019 at 04:42:35PM +0200, Thomas Zimmermann wrote: >>>>>> Implementing vmap() and vunmap() of struct drm_gem_object_funcs is >>>>>> required by generic fbdev emulation. Supporting free() is easy as >>>>>> well. More udl-specific interfaces can probably be replaced by GEM >>>>>> object functions in later patches. >>>>>> >>>>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >>>>>> --- >>>>>> drivers/gpu/drm/udl/udl_gem.c | 34 ++++++++++++++++++++++++++++++++++ >>>>>> 1 file changed, 34 insertions(+) >>>>>> >>>>>> diff --git a/drivers/gpu/drm/udl/udl_gem.c b/drivers/gpu/drm/udl/udl_gem.c >>>>>> index 3ea0cd9ae2d6..6ca097c270d6 100644 >>>>>> --- a/drivers/gpu/drm/udl/udl_gem.c >>>>>> +++ b/drivers/gpu/drm/udl/udl_gem.c >>>>>> @@ -11,6 +11,8 @@ >>>>>> >>>>>> #include "udl_drv.h" >>>>>> >>>>>> +static const struct drm_gem_object_funcs udl_gem_object_funcs; >>>>>> + >>>>>> struct udl_gem_object *udl_gem_alloc_object(struct drm_device *dev, >>>>>> size_t size) >>>>>> { >>>>>> @@ -25,6 +27,7 @@ struct udl_gem_object *udl_gem_alloc_object(struct drm_device *dev, >>>>>> return NULL; >>>>>> } >>>>>> >>>>>> + obj->base.funcs = &udl_gem_object_funcs; >>>>>> obj->flags = UDL_BO_CACHEABLE; >>>>>> return obj; >>>>>> } >>>>>> @@ -230,3 +233,34 @@ int udl_gem_mmap(struct drm_file *file, struct drm_device *dev, >>>>>> mutex_unlock(&udl->gem_lock); >>>>>> return ret; >>>>>> } >>>>>> + >>>>>> +/* >>>>>> + * Helpers for struct drm_gem_object_funcs >>>>>> + */ >>>>>> + >>>>>> +static void udl_gem_object_free(struct drm_gem_object *obj) >>>>>> +{ >>>>>> + udl_gem_free_object(obj); >>>>>> +} >>>>>> + >>>>>> +static void *udl_gem_object_vmap(struct drm_gem_object *obj) >>>>>> +{ >>>>>> + struct udl_gem_object *uobj = to_udl_bo(obj); >>>>>> + int ret; >>>>>> + >>>>>> + ret = udl_gem_vmap(uobj); >>>>>> + if (ret) >>>>>> + return ERR_PTR(ret); >>>>>> + return uobj->vmapping; >>>>>> +} >>>>>> + >>>>>> +static void udl_gem_object_vunmap(struct drm_gem_object *obj, void *vaddr) >>>>>> +{ >>>>>> + return udl_gem_vunmap(to_udl_bo(obj)); >>>>>> +} >>>>>> + >>>>>> +static const struct drm_gem_object_funcs udl_gem_object_funcs = { >>>>>> + .free = udl_gem_object_free, >>>>>> + .vmap = udl_gem_object_vmap, >>>>>> + .vunmap = udl_gem_object_vunmap, >>>>> >>>>> Yeah this doesn't work, you need to refcount the vmap here I think. I'd >>>> >>>> I see. Should have thought of that... >>>> >>>>> say simpler to first cut over to shmem helpers. >>>> >>>> Right. I was already attempting the conversion and the udl gem is more >>>> or less the same as shmem, except for the flags field. [1] The flag >>>> signals page attributes for mmap-ing [2]. For prime-imported BOs its is >>>> set to writecombining, and for local BOs it is set to cached. Shmem >>>> always maps with writecombining. >>>> >>>> I don't see why udl BOs are special wrt to mmap-ing. It seems to be some >>>> kind of optimization. Do you have an idea? >>>> >>>> Best regards >>>> Thomas >>>> >>>> [1] >>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/udl/udl_drv.h?h=v5.3#n78 >>>> [2] >>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/udl/udl_gem.c?h=v5.3#n57 >>> >>> udl doesn't set prefer_shadow = 1, which means compositors will render >>> directly into the buffer. And for that you want caching. For imported >>> dma-buf otoh you want wc to make sure it's coherent. Otherwise I guess >>> udl would need to get the dma_buf_begin/end_cpu_access calls added >>> (and that would probably be faster than going with wc for everything). >>> So we do want cachable, it's going to suck otherwise. >> >> Thanks for clarifying. Actually, it does have these calls in its dirty >> handler. [1] >> >>> >>> But that should be fairly easy to do by overwriting the obj->mmap >>> callback and wrapping it around the shmem one. >> >> Is there a reason why shmem doesn't implement the wc-vs.cached logic? >> Could this be added? >> > > I had a flag to set this in the initial version of the shmem helper > modeled after udl, but Thomas Hellstrom brought up a question and it was > dropped. The issue was beyond my understanding: > > [PATCH v3 0/2] drm: Add shmem GEM library > https://lists.freedesktop.org/archives/dri-devel/2018-September/190001.html If I understand that discussion correctly, the concern was that write combining and shared memory would not work well together. So you went with always-cached? Just recently, Gerd added unconditional write combining in rev 0be8958936. Best regards Thomas > > In tinydrm I have sped up uncached writes on arm by copying one pixel > line at a time into a temporary buffer before accessing the individual > bytes. See drm_fb_swab16(). > > Noralf. > >> (I'm just trying to understand the available options here before >> attempting to do a conversion.) >> >> Best regards >> Thomas >> >>> >>> What surprises me more is that udl supports mmap on imported dma-buf, >>> that's some real quirky stuff. Not sure there's really other drivers >>> doing that. Might be easier to rip that out :-) >>> -Daniel >>> >> >> [1] >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/udl/udl_fb.c?h=v5.3#n293 >> >> >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel >> > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel >
Hi, > > I had a flag to set this in the initial version of the shmem helper > > modeled after udl, but Thomas Hellstrom brought up a question and it was > > dropped. The issue was beyond my understanding: > > > > [PATCH v3 0/2] drm: Add shmem GEM library > > https://lists.freedesktop.org/archives/dri-devel/2018-September/190001.html > > If I understand that discussion correctly, the concern was that write > combining and shared memory would not work well together. So you went > with always-cached? > > Just recently, Gerd added unconditional write combining in rev 0be8958936. Well, it's not really added. It's the same thing drm_gem_mmap_obj() does for you when you don't have a drm_gem_object_funcs.mmap callback. But, yes, the reason this is done in the driver's mmap() callback with the new mmap code path is to give drivers the option to override this by supplying their own mmap() handler. So going with shmem helpers + custom mmap callback is a reasonable approach. HTH, Gerd
Den 25.10.2019 14.20, skrev Thomas Zimmermann: > (cc: Gerd) > > Hi > > Am 25.10.19 um 13:44 schrieb Noralf Trønnes: >> >> >> Den 25.10.2019 12.12, skrev Thomas Zimmermann: >>> Hi >>> >>> Am 25.10.19 um 11:28 schrieb Daniel Vetter: >>>> On Fri, Oct 25, 2019 at 9:59 AM Thomas Zimmermann <tzimmermann@suse.de> wrote >>>>> >>>>> Hi >>>>> >>>>> Am 25.10.19 um 09:40 schrieb Daniel Vetter: >>>>>> On Thu, Oct 24, 2019 at 04:42:35PM +0200, Thomas Zimmermann wrote: >>>>>>> Implementing vmap() and vunmap() of struct drm_gem_object_funcs is >>>>>>> required by generic fbdev emulation. Supporting free() is easy as >>>>>>> well. More udl-specific interfaces can probably be replaced by GEM >>>>>>> object functions in later patches. >>>>>>> >>>>>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >>>>>>> --- >>>>>>> drivers/gpu/drm/udl/udl_gem.c | 34 ++++++++++++++++++++++++++++++++++ >>>>>>> 1 file changed, 34 insertions(+) >>>>>>> >>>>>>> diff --git a/drivers/gpu/drm/udl/udl_gem.c b/drivers/gpu/drm/udl/udl_gem.c >>>>>>> index 3ea0cd9ae2d6..6ca097c270d6 100644 >>>>>>> --- a/drivers/gpu/drm/udl/udl_gem.c >>>>>>> +++ b/drivers/gpu/drm/udl/udl_gem.c >>>>>>> @@ -11,6 +11,8 @@ >>>>>>> >>>>>>> #include "udl_drv.h" >>>>>>> >>>>>>> +static const struct drm_gem_object_funcs udl_gem_object_funcs; >>>>>>> + >>>>>>> struct udl_gem_object *udl_gem_alloc_object(struct drm_device *dev, >>>>>>> size_t size) >>>>>>> { >>>>>>> @@ -25,6 +27,7 @@ struct udl_gem_object *udl_gem_alloc_object(struct drm_device *dev, >>>>>>> return NULL; >>>>>>> } >>>>>>> >>>>>>> + obj->base.funcs = &udl_gem_object_funcs; >>>>>>> obj->flags = UDL_BO_CACHEABLE; >>>>>>> return obj; >>>>>>> } >>>>>>> @@ -230,3 +233,34 @@ int udl_gem_mmap(struct drm_file *file, struct drm_device *dev, >>>>>>> mutex_unlock(&udl->gem_lock); >>>>>>> return ret; >>>>>>> } >>>>>>> + >>>>>>> +/* >>>>>>> + * Helpers for struct drm_gem_object_funcs >>>>>>> + */ >>>>>>> + >>>>>>> +static void udl_gem_object_free(struct drm_gem_object *obj) >>>>>>> +{ >>>>>>> + udl_gem_free_object(obj); >>>>>>> +} >>>>>>> + >>>>>>> +static void *udl_gem_object_vmap(struct drm_gem_object *obj) >>>>>>> +{ >>>>>>> + struct udl_gem_object *uobj = to_udl_bo(obj); >>>>>>> + int ret; >>>>>>> + >>>>>>> + ret = udl_gem_vmap(uobj); >>>>>>> + if (ret) >>>>>>> + return ERR_PTR(ret); >>>>>>> + return uobj->vmapping; >>>>>>> +} >>>>>>> + >>>>>>> +static void udl_gem_object_vunmap(struct drm_gem_object *obj, void *vaddr) >>>>>>> +{ >>>>>>> + return udl_gem_vunmap(to_udl_bo(obj)); >>>>>>> +} >>>>>>> + >>>>>>> +static const struct drm_gem_object_funcs udl_gem_object_funcs = { >>>>>>> + .free = udl_gem_object_free, >>>>>>> + .vmap = udl_gem_object_vmap, >>>>>>> + .vunmap = udl_gem_object_vunmap, >>>>>> >>>>>> Yeah this doesn't work, you need to refcount the vmap here I think. I'd >>>>> >>>>> I see. Should have thought of that... >>>>> >>>>>> say simpler to first cut over to shmem helpers. >>>>> >>>>> Right. I was already attempting the conversion and the udl gem is more >>>>> or less the same as shmem, except for the flags field. [1] The flag >>>>> signals page attributes for mmap-ing [2]. For prime-imported BOs its is >>>>> set to writecombining, and for local BOs it is set to cached. Shmem >>>>> always maps with writecombining. >>>>> >>>>> I don't see why udl BOs are special wrt to mmap-ing. It seems to be some >>>>> kind of optimization. Do you have an idea? >>>>> >>>>> Best regards >>>>> Thomas >>>>> >>>>> [1] >>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/udl/udl_drv.h?h=v5.3#n78 >>>>> [2] >>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/udl/udl_gem.c?h=v5.3#n57 >>>> >>>> udl doesn't set prefer_shadow = 1, which means compositors will render >>>> directly into the buffer. And for that you want caching. For imported >>>> dma-buf otoh you want wc to make sure it's coherent. Otherwise I guess >>>> udl would need to get the dma_buf_begin/end_cpu_access calls added >>>> (and that would probably be faster than going with wc for everything). >>>> So we do want cachable, it's going to suck otherwise. >>> >>> Thanks for clarifying. Actually, it does have these calls in its dirty >>> handler. [1] >>> >>>> >>>> But that should be fairly easy to do by overwriting the obj->mmap >>>> callback and wrapping it around the shmem one. >>> >>> Is there a reason why shmem doesn't implement the wc-vs.cached logic? >>> Could this be added? >>> >> >> I had a flag to set this in the initial version of the shmem helper >> modeled after udl, but Thomas Hellstrom brought up a question and it was >> dropped. The issue was beyond my understanding: >> >> [PATCH v3 0/2] drm: Add shmem GEM library >> https://lists.freedesktop.org/archives/dri-devel/2018-September/190001.html > > If I understand that discussion correctly, the concern was that write > combining and shared memory would not work well together. So you went > with always-cached? > > Just recently, Gerd added unconditional write combining in rev 0be8958936. > I don't remember, Rob picked up the patch since I couldn't use shmem after all for tinydrm. I see in the commit message that he made this change: v7: - Use write-combine for mmap instead. This is the more common case. (robher) Then we have this: drm/gem_shmem: Use a writecombine mapping for ->vaddr (be7d9f05c53e) So it looks like my initial work has been fixed up piece by piece to use writecombine. Noralf. > Best regards > Thomas > >> >> In tinydrm I have sped up uncached writes on arm by copying one pixel >> line at a time into a temporary buffer before accessing the individual >> bytes. See drm_fb_swab16(). >> >> Noralf. >> >>> (I'm just trying to understand the available options here before >>> attempting to do a conversion.) >>> >>> Best regards >>> Thomas >>> >>>> >>>> What surprises me more is that udl supports mmap on imported dma-buf, >>>> that's some real quirky stuff. Not sure there's really other drivers >>>> doing that. Might be easier to rip that out :-) >>>> -Daniel >>>> >>> >>> [1] >>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/udl/udl_fb.c?h=v5.3#n293 >>> >>> >>> >>> _______________________________________________ >>> dri-devel mailing list >>> dri-devel@lists.freedesktop.org >>> https://lists.freedesktop.org/mailman/listinfo/dri-devel >>> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel >> >
Hi Am 25.10.19 um 15:32 schrieb Gerd Hoffmann: > Hi, > >>> I had a flag to set this in the initial version of the shmem helper >>> modeled after udl, but Thomas Hellstrom brought up a question and it was >>> dropped. The issue was beyond my understanding: >>> >>> [PATCH v3 0/2] drm: Add shmem GEM library >>> https://lists.freedesktop.org/archives/dri-devel/2018-September/190001.html >> >> If I understand that discussion correctly, the concern was that write >> combining and shared memory would not work well together. So you went >> with always-cached? >> >> Just recently, Gerd added unconditional write combining in rev 0be8958936. > > Well, it's not really added. It's the same thing drm_gem_mmap_obj() > does for you when you don't have a drm_gem_object_funcs.mmap callback. > > But, yes, the reason this is done in the driver's mmap() callback with > the new mmap code path is to give drivers the option to override this > by supplying their own mmap() handler. So going with shmem helpers + > custom mmap callback is a reasonable approach. > > HTH, Absolutely. Thanks everyone for the feedback. Best regards Thomas > Gerd > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel >
diff --git a/drivers/gpu/drm/udl/udl_gem.c b/drivers/gpu/drm/udl/udl_gem.c index 3ea0cd9ae2d6..6ca097c270d6 100644 --- a/drivers/gpu/drm/udl/udl_gem.c +++ b/drivers/gpu/drm/udl/udl_gem.c @@ -11,6 +11,8 @@ #include "udl_drv.h" +static const struct drm_gem_object_funcs udl_gem_object_funcs; + struct udl_gem_object *udl_gem_alloc_object(struct drm_device *dev, size_t size) { @@ -25,6 +27,7 @@ struct udl_gem_object *udl_gem_alloc_object(struct drm_device *dev, return NULL; } + obj->base.funcs = &udl_gem_object_funcs; obj->flags = UDL_BO_CACHEABLE; return obj; } @@ -230,3 +233,34 @@ int udl_gem_mmap(struct drm_file *file, struct drm_device *dev, mutex_unlock(&udl->gem_lock); return ret; } + +/* + * Helpers for struct drm_gem_object_funcs + */ + +static void udl_gem_object_free(struct drm_gem_object *obj) +{ + udl_gem_free_object(obj); +} + +static void *udl_gem_object_vmap(struct drm_gem_object *obj) +{ + struct udl_gem_object *uobj = to_udl_bo(obj); + int ret; + + ret = udl_gem_vmap(uobj); + if (ret) + return ERR_PTR(ret); + return uobj->vmapping; +} + +static void udl_gem_object_vunmap(struct drm_gem_object *obj, void *vaddr) +{ + return udl_gem_vunmap(to_udl_bo(obj)); +} + +static const struct drm_gem_object_funcs udl_gem_object_funcs = { + .free = udl_gem_object_free, + .vmap = udl_gem_object_vmap, + .vunmap = udl_gem_object_vunmap, +};
Implementing vmap() and vunmap() of struct drm_gem_object_funcs is required by generic fbdev emulation. Supporting free() is easy as well. More udl-specific interfaces can probably be replaced by GEM object functions in later patches. Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> --- drivers/gpu/drm/udl/udl_gem.c | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+)