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