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

[Thomas Zimmermann]

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.

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 516 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20200813/af579e04/attachment.sig>