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.
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
Attachment:
signature.asc
Description: OpenPGP digital signature
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
