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

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

[Thomas Zimmermann]

The malidp driver uses GEM object functions for callbacks. Fix it to
use them internally as well.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Fixes: ecdd6474644f ("drm/malidp: Use GEM CMA object functions")
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Emil Velikov <emil.velikov@collabora.com>
Cc: Liviu Dudau <liviu.dudau@arm.com>
Cc: Brian Starkey <brian.starkey@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);
 
 		if (!sgt)
 			return false;
-- 
2.28.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[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@suse.de>
> Fixes: ecdd6474644f ("drm/malidp: Use GEM CMA object functions")
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Emil Velikov <emil.velikov@collabora.com>
> Cc: Liviu Dudau <liviu.dudau@arm.com>
> Cc: Brian Starkey <brian.starkey@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 ...
-Daniel

>  
>  		if (!sgt)
>  			return false;
> -- 
> 2.28.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 1881 bytes --]

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@suse.de>
>> Fixes: ecdd6474644f ("drm/malidp: Use GEM CMA object functions")
>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
>> Cc: Emil Velikov <emil.velikov@collabora.com>
>> Cc: Liviu Dudau <liviu.dudau@arm.com>
>> Cc: Brian Starkey <brian.starkey@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.

Best regards
Thomas

> -Daniel
> 
>>  
>>  		if (!sgt)
>>  			return false;
>> -- 
>> 2.28.0
>>
> 

-- 
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 #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 516 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Vetter]

On Fri, Aug 7, 2020 at 4:02 PM Thomas Zimmermann <tzimmermann@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@suse.de>
> >> Fixes: ecdd6474644f ("drm/malidp: Use GEM CMA object functions")
> >> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> >> Cc: Emil Velikov <emil.velikov@collabora.com>
> >> Cc: Liviu Dudau <liviu.dudau@arm.com>
> >> Cc: Brian Starkey <brian.starkey@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.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 2715 bytes --]

Hi

Am 07.08.20 um 18:10 schrieb Daniel Vetter:
> On Fri, Aug 7, 2020 at 4:02 PM Thomas Zimmermann <tzimmermann@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@suse.de>
>>>> Fixes: ecdd6474644f ("drm/malidp: Use GEM CMA object functions")
>>>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
>>>> Cc: Emil Velikov <emil.velikov@collabora.com>
>>>> Cc: Liviu Dudau <liviu.dudau@arm.com>
>>>> Cc: Brian Starkey <brian.starkey@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.

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


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 516 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[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@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@suse.de>
> >>>> Fixes: ecdd6474644f ("drm/malidp: Use GEM CMA object functions")
> >>>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> >>>> Cc: Emil Velikov <emil.velikov@collabora.com>
> >>>> Cc: Liviu Dudau <liviu.dudau@arm.com>
> >>>> Cc: Brian Starkey <brian.starkey@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?
-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
> 




-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 4285 bytes --]



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@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@suse.de>
>>>>>> Fixes: ecdd6474644f ("drm/malidp: Use GEM CMA object functions")
>>>>>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
>>>>>> Cc: Emil Velikov <emil.velikov@collabora.com>
>>>>>> Cc: Liviu Dudau <liviu.dudau@arm.com>
>>>>>> Cc: Brian Starkey <brian.starkey@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


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 516 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[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@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@suse.de>
> >>>>>> Fixes: ecdd6474644f ("drm/malidp: Use GEM CMA object functions")
> >>>>>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> >>>>>> Cc: Emil Velikov <emil.velikov@collabora.com>
> >>>>>> Cc: Liviu Dudau <liviu.dudau@arm.com>
> >>>>>> Cc: Brian Starkey <brian.starkey@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.

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
> 




-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 5459 bytes --]

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@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@suse.de>
>>>>>>>> Fixes: ecdd6474644f ("drm/malidp: Use GEM CMA object functions")
>>>>>>>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
>>>>>>>> Cc: Emil Velikov <emil.velikov@collabora.com>
>>>>>>>> Cc: Liviu Dudau <liviu.dudau@arm.com>
>>>>>>>> Cc: Brian Starkey <brian.starkey@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


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 516 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Vetter]

On Thu, Aug 13, 2020 at 12:39 PM Thomas Zimmermann <tzimmermann@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@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@suse.de>
> >>>>>>>> Fixes: ecdd6474644f ("drm/malidp: Use GEM CMA object functions")
> >>>>>>>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> >>>>>>>> Cc: Emil Velikov <emil.velikov@collabora.com>
> >>>>>>>> Cc: Liviu Dudau <liviu.dudau@arm.com>
> >>>>>>>> Cc: Brian Starkey <brian.starkey@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...

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@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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[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@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@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@suse.de>
> > >>>>>>>> Fixes: ecdd6474644f ("drm/malidp: Use GEM CMA object functions")
> > >>>>>>>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> > >>>>>>>> Cc: Emil Velikov <emil.velikov@collabora.com>
> > >>>>>>>> Cc: Liviu Dudau <liviu.dudau@arm.com>
> > >>>>>>>> Cc: Brian Starkey <brian.starkey@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@arm.com>

I will push the patch into drm-misc-next if Daniel has no other comments.

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

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 7616 bytes --]

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@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@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@suse.de>
>>>>>>>>>>> Fixes: ecdd6474644f ("drm/malidp: Use GEM CMA object functions")
>>>>>>>>>>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
>>>>>>>>>>> Cc: Emil Velikov <emil.velikov@collabora.com>
>>>>>>>>>>> Cc: Liviu Dudau <liviu.dudau@arm.com>
>>>>>>>>>>> Cc: Brian Starkey <brian.starkey@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@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@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


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 516 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel