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

Thu Aug 13 10:38:56 UTC 2020

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

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

-------------- 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/19da1187/attachment-0001.sig>