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

[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.
> > 
> > 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?
-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
> 




-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch