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

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

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.

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
> 




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