On Thu, Aug 13, 2020 at 12:39 PM Thomas Zimmermann <tzimmermann@xxxxxxx> 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@xxxxxxx> 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@xxxxxxx>
> >>>>>>>> Fixes: ecdd6474644f ("drm/malidp: Use GEM CMA object functions")
> >>>>>>>> Cc: Thomas Zimmermann <tzimmermann@xxxxxxx>
> >>>>>>>> Cc: Emil Velikov <emil.velikov@xxxxxxxxxxxxx>
> >>>>>>>> Cc: Liviu Dudau <liviu.dudau@xxxxxxx>
> >>>>>>>> Cc: Brian Starkey <brian.starkey@xxxxxxx>
> >>>>>>>> ---
> >>>>>>>> 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.
>
> 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@xxxxxxxx>
>
> 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
