[PATCH] drm/amd/display: remove need of modeset flag for overlay planes

[PATCH] drm/amd/display: remove need of modeset flag for overlay planes

[Shirish S]

This patch is in continuation to the
"843e3c7 drm/amd/display: defer modeset check in dm_update_planes_state"
where we started to eliminate the dependency on
DRM_MODE_ATOMIC_ALLOW_MODESET to be set by the user space,
which as such is not mandatory.

After deferring, this patch eliminates the dependency on the flag
for overlay planes.

This has to be done in stages as its a pretty complex and requires thorough
testing before we free primary planes as well from dependency on modeset
flag.

Signed-off-by: Shirish S <shirish.s@amd.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 1a63c04..87b661d 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -4174,7 +4174,7 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
 		}
 		spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
 
-		if (!pflip_needed) {
+		if (!pflip_needed || plane->type == DRM_PLANE_TYPE_OVERLAY) {
 			WARN_ON(!dm_new_plane_state->dc_state);
 
 			plane_states_constructed[planes_count] = dm_new_plane_state->dc_state;
@@ -4884,7 +4884,8 @@ static int dm_update_planes_state(struct dc *dc,
 
 		/* Remove any changed/removed planes */
 		if (!enable) {
-			if (pflip_needed)
+			if (pflip_needed &&
+			    plane && plane->type != DRM_PLANE_TYPE_OVERLAY)
 				continue;
 
 			if (!old_plane_crtc)
@@ -4931,7 +4932,8 @@ static int dm_update_planes_state(struct dc *dc,
 			if (!dm_new_crtc_state->stream)
 				continue;
 
-			if (pflip_needed)
+			if (pflip_needed &&
+			    plane && plane->type != DRM_PLANE_TYPE_OVERLAY)
 				continue;
 
 			WARN_ON(dm_new_plane_state->dc_state);
-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amd/display: remove need of modeset flag for overlay planes

[Stéphane Marchesin]

On Fri, Apr 27, 2018 at 3:27 AM Shirish S <shirish.s@amd.com> wrote:

> This patch is in continuation to the
> "843e3c7 drm/amd/display: defer modeset check in dm_update_planes_state"
> where we started to eliminate the dependency on
> DRM_MODE_ATOMIC_ALLOW_MODESET to be set by the user space,
> which as such is not mandatory.

> After deferring, this patch eliminates the dependency on the flag
> for overlay planes.

> This has to be done in stages as its a pretty complex and requires
thorough
> testing before we free primary planes as well from dependency on modeset
> flag.

> Signed-off-by: Shirish S <shirish.s@amd.com>
> ---
>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)

> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 1a63c04..87b661d 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -4174,7 +4174,7 @@ static void amdgpu_dm_commit_planes(struct
drm_atomic_state *state,
>                  }
>                  spin_unlock_irqrestore(&crtc->dev->event_lock, flags);

> -               if (!pflip_needed) {
> +               if (!pflip_needed || plane->type ==
DRM_PLANE_TYPE_OVERLAY) {
>                          WARN_ON(!dm_new_plane_state->dc_state);

>                          plane_states_constructed[planes_count] =
dm_new_plane_state->dc_state;
> @@ -4884,7 +4884,8 @@ static int dm_update_planes_state(struct dc *dc,

>                  /* Remove any changed/removed planes */
>                  if (!enable) {
> -                       if (pflip_needed)
> +                       if (pflip_needed &&
> +                           plane && plane->type !=
DRM_PLANE_TYPE_OVERLAY)

nit: I don't think we need to check that plane is non-NULL

Stéphane

>                                  continue;

>                          if (!old_plane_crtc)
> @@ -4931,7 +4932,8 @@ static int dm_update_planes_state(struct dc *dc,
>                          if (!dm_new_crtc_state->stream)
>                                  continue;

> -                       if (pflip_needed)
> +                       if (pflip_needed &&
> +                           plane && plane->type !=
DRM_PLANE_TYPE_OVERLAY)
>                                  continue;

>                          WARN_ON(dm_new_plane_state->dc_state);
> --
> 2.7.4

> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amd/display: remove need of modeset flag for overlay planes

[S, Shirish]

On 5/2/2018 12:53 AM, Stéphane Marchesin wrote:
> On Fri, Apr 27, 2018 at 3:27 AM Shirish S <shirish.s@amd.com> wrote:
>
>> This patch is in continuation to the
>> "843e3c7 drm/amd/display: defer modeset check in dm_update_planes_state"
>> where we started to eliminate the dependency on
>> DRM_MODE_ATOMIC_ALLOW_MODESET to be set by the user space,
>> which as such is not mandatory.
>> After deferring, this patch eliminates the dependency on the flag
>> for overlay planes.
>> This has to be done in stages as its a pretty complex and requires
> thorough
>> testing before we free primary planes as well from dependency on modeset
>> flag.
>> Signed-off-by: Shirish S <shirish.s@amd.com>
>> ---
>>    drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 8 +++++---
>>    1 file changed, 5 insertions(+), 3 deletions(-)
>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> index 1a63c04..87b661d 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> @@ -4174,7 +4174,7 @@ static void amdgpu_dm_commit_planes(struct
> drm_atomic_state *state,
>>                   }
>>                   spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
>> -               if (!pflip_needed) {
>> +               if (!pflip_needed || plane->type ==
> DRM_PLANE_TYPE_OVERLAY) {
>>                           WARN_ON(!dm_new_plane_state->dc_state);
>>                           plane_states_constructed[planes_count] =
> dm_new_plane_state->dc_state;
>> @@ -4884,7 +4884,8 @@ static int dm_update_planes_state(struct dc *dc,
>>                   /* Remove any changed/removed planes */
>>                   if (!enable) {
>> -                       if (pflip_needed)
>> +                       if (pflip_needed &&
>> +                           plane && plane->type !=
> DRM_PLANE_TYPE_OVERLAY)
>
> nit: I don't think we need to check that plane is non-NULL
Agree, was a bit over cautious.
Have removed it in V2.
Thanks.
Regards,
Shirish S
> Stéphane
>
>>                                   continue;
>>                           if (!old_plane_crtc)
>> @@ -4931,7 +4932,8 @@ static int dm_update_planes_state(struct dc *dc,
>>                           if (!dm_new_crtc_state->stream)
>>                                   continue;
>> -                       if (pflip_needed)
>> +                       if (pflip_needed &&
>> +                           plane && plane->type !=
> DRM_PLANE_TYPE_OVERLAY)
>>                                   continue;
>>                           WARN_ON(dm_new_plane_state->dc_state);
>> --
>> 2.7.4
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amd/display: remove need of modeset flag for overlay planes

[Harry Wentland]

On 2018-04-27 06:27 AM, Shirish S wrote:
> This patch is in continuation to the
> "843e3c7 drm/amd/display: defer modeset check in dm_update_planes_state"
> where we started to eliminate the dependency on
> DRM_MODE_ATOMIC_ALLOW_MODESET to be set by the user space,
> which as such is not mandatory.
> 
> After deferring, this patch eliminates the dependency on the flag
> for overlay planes.
> 

Apologies for the late response. I had to think about this patch for a long time since I'm not quite comfortable with it.

> This has to be done in stages as its a pretty complex and requires thorough
> testing before we free primary planes as well from dependency on modeset
> flag.
> 
> Signed-off-by: Shirish S <shirish.s@amd.com>
> ---
>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 1a63c04..87b661d 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -4174,7 +4174,7 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
>  		}
>  		spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
>  
> -		if (!pflip_needed) {
> +		if (!pflip_needed || plane->type == DRM_PLANE_TYPE_OVERLAY) {

Does this mean that whenever we have an overlay plane we won't do amdgpu_dm_do_flip but commit_planes_to_stream instead? Is this really the behavior we want?

commit_planes_to_stream was intended to program a new surface on a modeset whereas amdgpu_dm_do_flip was intended for pageflips. If we use commit_planes_to_stream we end up losing things like the immediate_flip flag, as well as the wait for the right moment to program the flip that amdgpu_dm_do_flip does. Even more importantly we won't wait for fences (reservation_object_wait_timeout_rcu).

Harry

>  			WARN_ON(!dm_new_plane_state->dc_state);
>  
>  			plane_states_constructed[planes_count] = dm_new_plane_state->dc_state;
> @@ -4884,7 +4884,8 @@ static int dm_update_planes_state(struct dc *dc,
>  
>  		/* Remove any changed/removed planes */
>  		if (!enable) {
> -			if (pflip_needed)
> +			if (pflip_needed &&
> +			    plane && plane->type != DRM_PLANE_TYPE_OVERLAY)
>  				continue;
>  
>  			if (!old_plane_crtc)
> @@ -4931,7 +4932,8 @@ static int dm_update_planes_state(struct dc *dc,
>  			if (!dm_new_crtc_state->stream)
>  				continue;
>  
> -			if (pflip_needed)
> +			if (pflip_needed &&
> +			    plane && plane->type != DRM_PLANE_TYPE_OVERLAY)
>  				continue;
>  
>  			WARN_ON(dm_new_plane_state->dc_state);
> 
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amd/display: remove need of modeset flag for overlay planes

[S, Shirish]

On 5/2/2018 7:21 PM, Harry Wentland wrote:
> On 2018-04-27 06:27 AM, Shirish S wrote:
>> This patch is in continuation to the
>> "843e3c7 drm/amd/display: defer modeset check in dm_update_planes_state"
>> where we started to eliminate the dependency on
>> DRM_MODE_ATOMIC_ALLOW_MODESET to be set by the user space,
>> which as such is not mandatory.
>>
>> After deferring, this patch eliminates the dependency on the flag
>> for overlay planes.
>>
> Apologies for the late response. I had to think about this patch for a long time since I'm not quite comfortable with it.
>
>> This has to be done in stages as its a pretty complex and requires thorough
>> testing before we free primary planes as well from dependency on modeset
>> flag.
>>
>> Signed-off-by: Shirish S <shirish.s@amd.com>
>> ---
>>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 8 +++++---
>>   1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> index 1a63c04..87b661d 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> @@ -4174,7 +4174,7 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
>>   		}
>>   		spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
>>   
>> -		if (!pflip_needed) {
>> +		if (!pflip_needed || plane->type == DRM_PLANE_TYPE_OVERLAY) {
> Does this mean that whenever we have an overlay plane we won't do amdgpu_dm_do_flip but commit_planes_to_stream instead? Is this really the behavior we want?
>
> commit_planes_to_stream was intended to program a new surface on a modeset whereas amdgpu_dm_do_flip was intended for pageflips.
Need of "modeset" flag to program new surface is what we want to fix in 
this patch for underlay plane and in next stages, fix manifestations 
caused by this approach as and when seen.
Since the user space doesn't send modeset flag for new surface, hence to 
program it, this patch checks the plane type to construct planes_count 
before calling commit_planes_to_stream().

  If we use commit_planes_to_stream we end up losing things like the immediate_flip flag, as well as the wait for the right moment to program the flip that amdgpu_dm_do_flip does.

 From the code, amdgpu_dm_do_flip does what you mentioned only for 
primary plane and hence either way its not set for underlay.

Regards,
Shirish S
>   Even more importantly we won't wait for fences (reservation_object_wait_timeout_rcu).
>
> Harry
>
>>   			WARN_ON(!dm_new_plane_state->dc_state);
>>   
>>   			plane_states_constructed[planes_count] = dm_new_plane_state->dc_state;
>> @@ -4884,7 +4884,8 @@ static int dm_update_planes_state(struct dc *dc,
>>   
>>   		/* Remove any changed/removed planes */
>>   		if (!enable) {
>> -			if (pflip_needed)
>> +			if (pflip_needed &&
>> +			    plane && plane->type != DRM_PLANE_TYPE_OVERLAY)
>>   				continue;
>>   
>>   			if (!old_plane_crtc)
>> @@ -4931,7 +4932,8 @@ static int dm_update_planes_state(struct dc *dc,
>>   			if (!dm_new_crtc_state->stream)
>>   				continue;
>>   
>> -			if (pflip_needed)
>> +			if (pflip_needed &&
>> +			    plane && plane->type != DRM_PLANE_TYPE_OVERLAY)
>>   				continue;
>>   
>>   			WARN_ON(dm_new_plane_state->dc_state);
>>

-- 
Regards,
Shirish S

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amd/display: remove need of modeset flag for overlay planes

[Harry Wentland]

On 2018-05-03 04:00 AM, S, Shirish wrote:
> 
> 
> On 5/2/2018 7:21 PM, Harry Wentland wrote:
>> On 2018-04-27 06:27 AM, Shirish S wrote:
>>> This patch is in continuation to the
>>> "843e3c7 drm/amd/display: defer modeset check in dm_update_planes_state"
>>> where we started to eliminate the dependency on
>>> DRM_MODE_ATOMIC_ALLOW_MODESET to be set by the user space,
>>> which as such is not mandatory.
>>>
>>> After deferring, this patch eliminates the dependency on the flag
>>> for overlay planes.
>>>
>> Apologies for the late response. I had to think about this patch for a long time since I'm not quite comfortable with it.
>>
>>> This has to be done in stages as its a pretty complex and requires thorough
>>> testing before we free primary planes as well from dependency on modeset
>>> flag.
>>>
>>> Signed-off-by: Shirish S <shirish.s@amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 8 +++++---
>>>   1 file changed, 5 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> index 1a63c04..87b661d 100644
>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> @@ -4174,7 +4174,7 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
>>>           }
>>>           spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
>>>   -        if (!pflip_needed) {
>>> +        if (!pflip_needed || plane->type == DRM_PLANE_TYPE_OVERLAY) {
>> Does this mean that whenever we have an overlay plane we won't do amdgpu_dm_do_flip but commit_planes_to_stream instead? Is this really the behavior we want?
>>
>> commit_planes_to_stream was intended to program a new surface on a modeset whereas amdgpu_dm_do_flip was intended for pageflips.
> Need of "modeset" flag to program new surface is what we want to fix in this patch for underlay plane and in next stages, fix manifestations caused by this approach as and when seen.
> Since the user space doesn't send modeset flag for new surface, hence to program it, this patch checks the plane type to construct planes_count before calling commit_planes_to_stream().
> 

Looking at the allow_modeset flag was never quite right and we anticipated having to rework this when having to deal with things like multiple planes. What really has to happen is that we determine the surface_update_type in atomic_check and then use that in atomic_commit to either program the surface only (UPDATE_TYPE_FAST) without having to lock all pipes or to lock all pipes (see lock_and_validation_needed in amdgpu_dm_atomic_check) if we need to reprogram mode (UPDATE_TYPE_FULL). I don't remember exactly what UPDATE_TYPE_MED is used for.

I don't feel comfortable taking a shortcut for DRM_PLANE_TYPE_OVERLAY without first having a plan and patches for how to deal with the above-mentioned.

Bhawan and Andrey had a look at this before but it was never quite ready. The work was non-trivial and potentially impacts lots of configurations and scenarios if we don't get it right. If you're curious you can look at this change (apologies to everyone else for posting AMD-internal link): http://git.amd.com:8080/#/c/103931/11

>  If we use commit_planes_to_stream we end up losing things like the immediate_flip flag, as well as the wait for the right moment to program the flip that amdgpu_dm_do_flip does.
> 
> From the code, amdgpu_dm_do_flip does what you mentioned only for primary plane and hence either way its not set for underlay.

The code wasn't designed with underlay in mind, so it will need work.

Harry

> 
> Regards,
> Shirish S
>>   Even more importantly we won't wait for fences (reservation_object_wait_timeout_rcu).
>>
>> Harry
>>
>>>               WARN_ON(!dm_new_plane_state->dc_state);
>>>                 plane_states_constructed[planes_count] = dm_new_plane_state->dc_state;
>>> @@ -4884,7 +4884,8 @@ static int dm_update_planes_state(struct dc *dc,
>>>             /* Remove any changed/removed planes */
>>>           if (!enable) {
>>> -            if (pflip_needed)
>>> +            if (pflip_needed &&
>>> +                plane && plane->type != DRM_PLANE_TYPE_OVERLAY)
>>>                   continue;
>>>                 if (!old_plane_crtc)
>>> @@ -4931,7 +4932,8 @@ static int dm_update_planes_state(struct dc *dc,
>>>               if (!dm_new_crtc_state->stream)
>>>                   continue;
>>>   -            if (pflip_needed)
>>> +            if (pflip_needed &&
>>> +                plane && plane->type != DRM_PLANE_TYPE_OVERLAY)
>>>                   continue;
>>>                 WARN_ON(dm_new_plane_state->dc_state);
>>>
> 
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amd/display: remove need of modeset flag for overlay planes

[Andrey Grodzovsky]

On 05/03/2018 02:11 PM, Harry Wentland wrote:
> On 2018-05-03 04:00 AM, S, Shirish wrote:
>>
>> On 5/2/2018 7:21 PM, Harry Wentland wrote:
>>> On 2018-04-27 06:27 AM, Shirish S wrote:
>>>> This patch is in continuation to the
>>>> "843e3c7 drm/amd/display: defer modeset check in dm_update_planes_state"
>>>> where we started to eliminate the dependency on
>>>> DRM_MODE_ATOMIC_ALLOW_MODESET to be set by the user space,
>>>> which as such is not mandatory.
>>>>
>>>> After deferring, this patch eliminates the dependency on the flag
>>>> for overlay planes.
>>>>
>>> Apologies for the late response. I had to think about this patch for a long time since I'm not quite comfortable with it.
>>>
>>>> This has to be done in stages as its a pretty complex and requires thorough
>>>> testing before we free primary planes as well from dependency on modeset
>>>> flag.
>>>>
>>>> Signed-off-by: Shirish S <shirish.s@amd.com>
>>>> ---
>>>>    drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 8 +++++---
>>>>    1 file changed, 5 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>> index 1a63c04..87b661d 100644
>>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>> @@ -4174,7 +4174,7 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
>>>>            }
>>>>            spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
>>>>    -        if (!pflip_needed) {
>>>> +        if (!pflip_needed || plane->type == DRM_PLANE_TYPE_OVERLAY) {
>>> Does this mean that whenever we have an overlay plane we won't do amdgpu_dm_do_flip but commit_planes_to_stream instead? Is this really the behavior we want?
>>>
>>> commit_planes_to_stream was intended to program a new surface on a modeset whereas amdgpu_dm_do_flip was intended for pageflips.
>> Need of "modeset" flag to program new surface is what we want to fix in this patch for underlay plane and in next stages, fix manifestations caused by this approach as and when seen.
>> Since the user space doesn't send modeset flag for new surface, hence to program it, this patch checks the plane type to construct planes_count before calling commit_planes_to_stream().
>>
> Looking at the allow_modeset flag was never quite right and we anticipated having to rework this when having to deal with things like multiple planes. What really has to happen is that we determine the surface_update_type in atomic_check and then use that in atomic_commit to either program the surface only (UPDATE_TYPE_FAST) without having to lock all pipes or to lock all pipes (see lock_and_validation_needed in amdgpu_dm_atomic_check) if we need to reprogram mode (UPDATE_TYPE_FULL). I don't remember exactly what UPDATE_TYPE_MED is used for.
>
> I don't feel comfortable taking a shortcut for DRM_PLANE_TYPE_OVERLAY without first having a plan and patches for how to deal with the above-mentioned.
>
> Bhawan and Andrey had a look at this before but it was never quite ready. The work was non-trivial and potentially impacts lots of configurations and scenarios if we don't get it right. If you're curious you can look at this change (apologies to everyone else for posting AMD-internal link): http://git.amd.com:8080/#/c/103931/11
>
>>   If we use commit_planes_to_stream we end up losing things like the immediate_flip flag, as well as the wait for the right moment to program the flip that amdgpu_dm_do_flip does.
>>
>>  From the code, amdgpu_dm_do_flip does what you mentioned only for primary plane and hence either way its not set for underlay.
> The code wasn't designed with underlay in mind, so it will need work.
>
> Harry

I support Harry's comments, we definitely need to strive to remove 
dependency on page_fleep needed flag, AFAIK we are the only ATOMIC KMS 
driver which makes a distinction between page fleep and other
surface updates, but it's better to sit and create a general plan of how 
to address it for all type of planes instead of patching for overlay only.

Andrey

>
>> Regards,
>> Shirish S
>>>    Even more importantly we won't wait for fences (reservation_object_wait_timeout_rcu).
>>>
>>> Harry
>>>
>>>>                WARN_ON(!dm_new_plane_state->dc_state);
>>>>                  plane_states_constructed[planes_count] = dm_new_plane_state->dc_state;
>>>> @@ -4884,7 +4884,8 @@ static int dm_update_planes_state(struct dc *dc,
>>>>              /* Remove any changed/removed planes */
>>>>            if (!enable) {
>>>> -            if (pflip_needed)
>>>> +            if (pflip_needed &&
>>>> +                plane && plane->type != DRM_PLANE_TYPE_OVERLAY)
>>>>                    continue;
>>>>                  if (!old_plane_crtc)
>>>> @@ -4931,7 +4932,8 @@ static int dm_update_planes_state(struct dc *dc,
>>>>                if (!dm_new_crtc_state->stream)
>>>>                    continue;
>>>>    -            if (pflip_needed)
>>>> +            if (pflip_needed &&
>>>> +                plane && plane->type != DRM_PLANE_TYPE_OVERLAY)
>>>>                    continue;
>>>>                  WARN_ON(dm_new_plane_state->dc_state);
>>>>

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amd/display: remove need of modeset flag for overlay planes

[S, Shirish]

On 5/4/2018 6:27 PM, Andrey Grodzovsky wrote:
>
>
> On 05/03/2018 02:11 PM, Harry Wentland wrote:
>> On 2018-05-03 04:00 AM, S, Shirish wrote:
>>>
>>> On 5/2/2018 7:21 PM, Harry Wentland wrote:
>>>> On 2018-04-27 06:27 AM, Shirish S wrote:
>>>>> This patch is in continuation to the
>>>>> "843e3c7 drm/amd/display: defer modeset check in 
>>>>> dm_update_planes_state"
>>>>> where we started to eliminate the dependency on
>>>>> DRM_MODE_ATOMIC_ALLOW_MODESET to be set by the user space,
>>>>> which as such is not mandatory.
>>>>>
>>>>> After deferring, this patch eliminates the dependency on the flag
>>>>> for overlay planes.
>>>>>
>>>> Apologies for the late response. I had to think about this patch 
>>>> for a long time since I'm not quite comfortable with it.
>>>>
>>>>> This has to be done in stages as its a pretty complex and requires 
>>>>> thorough
>>>>> testing before we free primary planes as well from dependency on 
>>>>> modeset
>>>>> flag.
>>>>>
>>>>> Signed-off-by: Shirish S <shirish.s@amd.com>
>>>>> ---
>>>>>    drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 8 +++++---
>>>>>    1 file changed, 5 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c 
>>>>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>>> index 1a63c04..87b661d 100644
>>>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>>> @@ -4174,7 +4174,7 @@ static void amdgpu_dm_commit_planes(struct 
>>>>> drm_atomic_state *state,
>>>>>            }
>>>>> spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
>>>>>    -        if (!pflip_needed) {
>>>>> +        if (!pflip_needed || plane->type == 
>>>>> DRM_PLANE_TYPE_OVERLAY) {
>>>> Does this mean that whenever we have an overlay plane we won't do 
>>>> amdgpu_dm_do_flip but commit_planes_to_stream instead? Is this 
>>>> really the behavior we want?
>>>>
>>>> commit_planes_to_stream was intended to program a new surface on a 
>>>> modeset whereas amdgpu_dm_do_flip was intended for pageflips.
>>> Need of "modeset" flag to program new surface is what we want to fix 
>>> in this patch for underlay plane and in next stages, fix 
>>> manifestations caused by this approach as and when seen.
>>> Since the user space doesn't send modeset flag for new surface, 
>>> hence to program it, this patch checks the plane type to construct 
>>> planes_count before calling commit_planes_to_stream().
>>>
>> Looking at the allow_modeset flag was never quite right and we 
>> anticipated having to rework this when having to deal with things 
>> like multiple planes. What really has to happen is that we determine 
>> the surface_update_type in atomic_check and then use that in 
>> atomic_commit to either program the surface only (UPDATE_TYPE_FAST) 
>> without having to lock all pipes or to lock all pipes (see 
>> lock_and_validation_needed in amdgpu_dm_atomic_check) if we need to 
>> reprogram mode (UPDATE_TYPE_FULL). I don't remember exactly what 
>> UPDATE_TYPE_MED is used for.
>>
True, any suggestions on what needs to be check to decide if the surface 
update has  to be FAST?
Like plane_count, format or dc_state etc?
>> I don't feel comfortable taking a shortcut for DRM_PLANE_TYPE_OVERLAY 
>> without first having a plan and patches for how to deal with the 
>> above-mentioned.
>>
>> Bhawan and Andrey had a look at this before but it was never quite 
>> ready. The work was non-trivial and potentially impacts lots of 
>> configurations and scenarios if we don't get it right. If you're 
>> curious you can look at this change (apologies to everyone else for 
>> posting AMD-internal link): http://git.amd.com:8080/#/c/103931/11
>>
>>>   If we use commit_planes_to_stream we end up losing things like the 
>>> immediate_flip flag, as well as the wait for the right moment to 
>>> program the flip that amdgpu_dm_do_flip does.
>>>
>>>  From the code, amdgpu_dm_do_flip does what you mentioned only for 
>>> primary plane and hence either way its not set for underlay.
>> The code wasn't designed with underlay in mind, so it will need work.
>>
>> Harry
>
> I support Harry's comments, we definitely need to strive to remove 
> dependency on page_fleep needed flag, AFAIK we are the only ATOMIC KMS 
> driver which makes a distinction between page fleep and other
> surface updates, but it's better to sit and create a general plan of 
> how to address it for all type of planes instead of patching for 
> overlay only.
>
> Andrey
>
Is anybody working on removing the dependency on page_flip_needed flag ?
Since am working on stoney for chrome os, i have limited visibility or 
knowledge of its implications on other asics,
hence to avoid regressions to other users i patched only the overlay 
path of it.

Some one who knows its inter-dependency on dc would be right to do it.
If you think its a long shot please re-consider my proposal of doing it 
in stages, i.e., removing overlay planes from dependency and then the 
primary since cursor updates is a but tricky and tied to primary plane.
Regards,
Shirish S
>>
>>> Regards,
>>> Shirish S
>>>>    Even more importantly we won't wait for fences 
>>>> (reservation_object_wait_timeout_rcu).
>>>>
>>>> Harry
>>>>
>>>>> WARN_ON(!dm_new_plane_state->dc_state);
>>>>>                  plane_states_constructed[planes_count] = 
>>>>> dm_new_plane_state->dc_state;
>>>>> @@ -4884,7 +4884,8 @@ static int dm_update_planes_state(struct dc 
>>>>> *dc,
>>>>>              /* Remove any changed/removed planes */
>>>>>            if (!enable) {
>>>>> -            if (pflip_needed)
>>>>> +            if (pflip_needed &&
>>>>> +                plane && plane->type != DRM_PLANE_TYPE_OVERLAY)
>>>>>                    continue;
>>>>>                  if (!old_plane_crtc)
>>>>> @@ -4931,7 +4932,8 @@ static int dm_update_planes_state(struct dc 
>>>>> *dc,
>>>>>                if (!dm_new_crtc_state->stream)
>>>>>                    continue;
>>>>>    -            if (pflip_needed)
>>>>> +            if (pflip_needed &&
>>>>> +                plane && plane->type != DRM_PLANE_TYPE_OVERLAY)
>>>>>                    continue;
>>>>>                  WARN_ON(dm_new_plane_state->dc_state);
>>>>>
>

-- 
Regards,
Shirish S

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amd/display: remove need of modeset flag for overlay planes

[Harry Wentland]

On 2018-04-27 06:27 AM, Shirish S wrote:
> This patch is in continuation to the
> "843e3c7 drm/amd/display: defer modeset check in dm_update_planes_state"
> where we started to eliminate the dependency on
> DRM_MODE_ATOMIC_ALLOW_MODESET to be set by the user space,
> which as such is not mandatory.
> 
> After deferring, this patch eliminates the dependency on the flag
> for overlay planes.
> 
> This has to be done in stages as its a pretty complex and requires thorough
> testing before we free primary planes as well from dependency on modeset
> flag.
> 
> Signed-off-by: Shirish S <shirish.s@amd.com>

After the offline discussion I'm okay to go ahead with this as it should be relatively self-contained and currently unblocks one use-case. We'll definitely need to address this code soon and find a good way to stop abusing the allow_modeset flag.

Reviewed-by: Harry Wentland <harry.wentland@amd.com>

If this does cause unexpected fallout we can always revert and revisit.

Harry

> ---
>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 1a63c04..87b661d 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -4174,7 +4174,7 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
>  		}
>  		spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
>  
> -		if (!pflip_needed) {
> +		if (!pflip_needed || plane->type == DRM_PLANE_TYPE_OVERLAY) {
>  			WARN_ON(!dm_new_plane_state->dc_state);
>  
>  			plane_states_constructed[planes_count] = dm_new_plane_state->dc_state;
> @@ -4884,7 +4884,8 @@ static int dm_update_planes_state(struct dc *dc,
>  
>  		/* Remove any changed/removed planes */
>  		if (!enable) {
> -			if (pflip_needed)
> +			if (pflip_needed &&
> +			    plane && plane->type != DRM_PLANE_TYPE_OVERLAY)
>  				continue;
>  
>  			if (!old_plane_crtc)
> @@ -4931,7 +4932,8 @@ static int dm_update_planes_state(struct dc *dc,
>  			if (!dm_new_crtc_state->stream)
>  				continue;
>  
> -			if (pflip_needed)
> +			if (pflip_needed &&
> +			    plane && plane->type != DRM_PLANE_TYPE_OVERLAY)
>  				continue;
>  
>  			WARN_ON(dm_new_plane_state->dc_state);
> 
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx