[PATCH] drm/malidp: Use struct drm_gem_object_funcs.get_sg_table internally
Thomas Zimmermann
tzimmermann at suse.de
Wed Aug 19 06:58:27 UTC 2020
Hi
Am 18.08.20 um 19:15 schrieb Liviu Dudau:
> On Thu, Aug 13, 2020 at 02:01:19PM +0200, Daniel Vetter wrote:
>> On Thu, Aug 13, 2020 at 12:39 PM Thomas Zimmermann <tzimmermann at suse.de> 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 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.
>
> Hi,
>
> Sorry for the silence, I was on holiday.
>
>>>>>>>>
>>>>>>>> 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.
>
> That's right, this piece of code has nothing to do with importing dma-buf or
> checking if things are contiguous. Mali DP hardware has a prefetcher block that
> can generate some fake requests during vblank to get the MMU pagetables cached
> so that reads from the buffers can be done without waiting for page walks. The
> block supports two modes, a full page or partial page request (to cater for the
> ends of the buffer that might not be a full page).
>
> The patch proposed looks good to me, so:
>
> Acked-by: Liviu Dudau <liviu.dudau at arm.com>
>
> I will push the patch into drm-misc-next if Daniel has no other comments.
Thanks for explaining. The patch has already been merged.
Best regards
Thomas
>
> Best regards,
> Liviu
>
>>>>
>>>> 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 at ffwll.ch>
>>
>>>
>>> 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
>
--
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/20200819/892b9b49/attachment.sig>
More information about the dri-devel
mailing list