[PATCH] drm/malidp: Use struct drm_gem_object_funcs.get_sg_table internally

Liviu Dudau liviu.dudau at arm.com
Tue Aug 18 17:15:30 UTC 2020 

On Thu, Aug 13, 2020 at 02:01:19PM +0200, Daniel Vetter wrote:
> On Thu, Aug 13, 2020 at 12:39 PM Thomas Zimmermann <tzimmermann at suse.de> wrote:
> >
> > Hi
> >
> > Am 13.08.20 um 12:31 schrieb Daniel Vetter:
> > > On Thu, Aug 13, 2020 at 12:28:55PM +0200, Thomas Zimmermann wrote:
> > >>
> > >>
> > >> Am 13.08.20 um 11:48 schrieb Daniel Vetter:
> > >>> On Thu, Aug 13, 2020 at 11:19:31AM +0200, Thomas Zimmermann wrote:
> > >>>> Hi
> > >>>>
> > >>>> Am 07.08.20 um 18:10 schrieb Daniel Vetter:
> > >>>>> On Fri, Aug 7, 2020 at 4:02 PM Thomas Zimmermann <tzimmermann at suse.de> wrote:
> > >>>>>>
> > >>>>>> Hi
> > >>>>>>
> > >>>>>> Am 07.08.20 um 15:12 schrieb Daniel Vetter:
> > >>>>>>> On Fri, Aug 07, 2020 at 01:10:22PM +0200, Thomas Zimmermann wrote:
> > >>>>>>>> The malidp driver uses GEM object functions for callbacks. Fix it to
> > >>>>>>>> use them internally as well.
> > >>>>>>>>
> > >>>>>>>> Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de>
> > >>>>>>>> Fixes: ecdd6474644f ("drm/malidp: Use GEM CMA object functions")
> > >>>>>>>> Cc: Thomas Zimmermann <tzimmermann at suse.de>
> > >>>>>>>> Cc: Emil Velikov <emil.velikov at collabora.com>
> > >>>>>>>> Cc: Liviu Dudau <liviu.dudau at arm.com>
> > >>>>>>>> Cc: Brian Starkey <brian.starkey at arm.com>
> > >>>>>>>> ---
> > >>>>>>>>  drivers/gpu/drm/arm/malidp_planes.c | 2 +-
> > >>>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> > >>>>>>>>
> > >>>>>>>> diff --git a/drivers/gpu/drm/arm/malidp_planes.c b/drivers/gpu/drm/arm/malidp_planes.c
> > >>>>>>>> index ab45ac445045..351a85088d0e 100644
> > >>>>>>>> --- a/drivers/gpu/drm/arm/malidp_planes.c
> > >>>>>>>> +++ b/drivers/gpu/drm/arm/malidp_planes.c
> > >>>>>>>> @@ -346,7 +346,7 @@ static bool malidp_check_pages_threshold(struct malidp_plane_state *ms,
> > >>>>>>>>              if (cma_obj->sgt)
> > >>>>>>>>                      sgt = cma_obj->sgt;
> > >>>>>>>>              else
> > >>>>>>>> -                    sgt = obj->dev->driver->gem_prime_get_sg_table(obj);
> > >>>>>>>> +                    sgt = obj->funcs->get_sg_table(obj);
> > >>>>>>>
> > >>>>>>> Uh if there's not a switch somewhere I'd just call the right function
> > >>>>>>> directly. Or call the right wrapper for this, this feels a bit fishy ...
> > >>>>>>
> > >>>>>> The driver initializes the pointer via CMA helper macro to an
> > >>>>>> CMA-internal default. Calling the actual function here is fragile if the
> > >>>>>> CMA-internal default ever changes.
> > >>>>>>
> > >>>>>> But I have no strong feelings. I'll go with whatever the driver's
> > >>>>>> maintainer prefers.

Hi,

Sorry for the silence, I was on holiday.

> > >>>>>
> > >>>>> What I meant is: There should be an exported helper to get at the sgt.
> > >>>>> Drivers using helpers shouldn't need to do this kind of stuff here.
> > >>>>>
> > >>>>> Also the entire code is fairly suspect, getting at the sgt from
> > >>>>> plane_check is a bit iffy. But that's a different kind of problem.
> > >>>>
> > >>>> I tried to somehow move the code to CMA, but it's not easy. There's no
> > >>>> good place to put the look-up code of sgt. And sgt is later being freed
> > >>>> iff it came from the callback (and not freed if it was stored in the
> > >>>> object). AFIACT the best options are to either keep the code here or
> > >>>> move the entire function to CMA helpers.
> > >>>
> > >>> Ok I read some code ... I'm confused. From the control flow it looks like
> > >>> malidp is using cma helpers. Otherwise why does the upcasting not blow up
> > >>> sometimes.
> > >>>
> > >>> But cma helpers already check at import time that any imported dma-buf is
> > >>> contiguous, and they guarantee to fill out the cma_obj->sgt.
> > >>>
> > >>> So really no idea what this code is doing here.
> > >>>
> > >>> It's also not correct, since it doesn't coalesce sgt entries, since a
> > >>> range might be split up, but still mapped into a contiguous dma_addr_t
> > >>> range when you take it all together. The code in
> > >>> drm_gem_cma_prime_import_sg_table() gets this more right.
> > >>>
> > >>> So maybe right fix is to just ditch this all, and use cma helpers fully?
> > >>
> > >> The driver already does use CMA, including
> > >> drm_gem_cma_prime_import_sg_table().
> > >>
> > >> The patched code is not about importing/exporting sg tables. It
> > >> configures the MMU's prefetching pattern by looking at some of the page
> > >> sizes. I don't feel confident enough with this code to alter it. I'd
> > >> expect to break the heuristics.

That's right, this piece of code has nothing to do with importing dma-buf or
checking if things are contiguous. Mali DP hardware has a prefetcher block that
can generate some fake requests during vblank to get the MMU pagetables cached
so that reads from the buffers can be done without waiting for page walks. The
block supports two modes, a full page or partial page request (to cater for the
ends of the buffer that might not be a full page).

The patch proposed looks good to me, so:

Acked-by: Liviu Dudau <liviu.dudau at arm.com>

I will push the patch into drm-misc-next if Daniel has no other comments.

Best regards,
Liviu

> > >
> > > Hm ok, no idea either.
> > >
> > > But then we can just assume that cma_obj->sgt is always set, and we don't
> > > have to call anything. If a driver uses cma helpers, and cma doesn't set
> > > ->sgt over the lifetime of the buffer, that breaks a cma helper assumption
> > > since cma doens't support swap-out.
> >
> > Really? I just looked at drm_gem_cma_helper.c and ->sgt is only ever set
> > on imports, and only freed for imported memory. I'm confused now...
> 
> Hm right this works differently than I thought, for native cma objects
> we just store padd/vaddr and that's it. Still feels wrong that malidp
> digs around in an internal helper that's meant for exporting as a
> dma-buf.
> 
> Oh well I guess this is just a very special special case.
> 
> Reviewed-by: Daniel Vetter <daniel at ffwll.ch>
> 
> >
> > Best regards
> > Thomas
> >
> > >
> > > So converting the if to a WARN_ON and failing with an error, and then
> > > remove the else should work.
> > > -Daniel
> > >
> > >>
> > >> Best regards
> > >> Thomas
> > >>
> > >>> -Daniel
> > >>>
> > >>>>
> > >>>> Best regards
> > >>>> Thomas
> > >>>>
> > >>>>> -Daniel
> > >>>>>
> > >>>>
> > >>>> --
> > >>>> 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
> > >>>>
> > >>>
> > >>>
> > >>>
> > >>>
> > >>
> > >> --
> > >> 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
> > >>
> > >
> > >
> > >
> > >
> >
> > --
> > 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
> >
> 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

More information about the dri-devel mailing list