[PATCH] drm/amd/display: Disallow enabling CRTC without primary plane with FB

[PATCH] drm/amd/display: Disallow enabling CRTC without primary plane with FB

[Harry Wentland]

The below commit

    "drm/atomic: Try to preserve the crtc enabled state in drm_atomic_remove_fb, v2"

introduces a slight behavioral change to rmfb. Instead of disabling a crtc
when the primary plane is disabled, it now preserves it.

Since DC is currently not equipped to handle this we need to fail such
a commit, otherwise we might see a corrupted screen.

This is based on Shirish's previous approach but avoids adding all
planes to the new atomic state which leads to a full update in DC for
any commit, and is not what we intend.

Theoretically DM should be able to deal with states with fully populated planes,
even for simple updates, such as cursor updates. This should still be
addressed in the future.

Signed-off-by: Harry Wentland <harry.wentland@amd.com>
Cc: stable@vger.kernel.org
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

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 6f92a19bebd6..0bdc6b484bad 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -4683,6 +4683,7 @@ static int dm_update_crtcs_state(struct amdgpu_display_manager *dm,
 		struct amdgpu_dm_connector *aconnector = NULL;
 		struct drm_connector_state *new_con_state = NULL;
 		struct dm_connector_state *dm_conn_state = NULL;
+		struct drm_plane_state *new_plane_state = NULL;
 
 		new_stream = NULL;
 
@@ -4690,6 +4691,13 @@ static int dm_update_crtcs_state(struct amdgpu_display_manager *dm,
 		dm_new_crtc_state = to_dm_crtc_state(new_crtc_state);
 		acrtc = to_amdgpu_crtc(crtc);
 
+		new_plane_state = drm_atomic_get_new_plane_state(state, new_crtc_state->crtc->primary);
+
+		if (new_crtc_state->enable && new_plane_state && !new_plane_state->fb) {
+			ret = -EINVAL;
+			goto fail;
+		}
+
 		aconnector = amdgpu_dm_find_first_crtc_matching_connector(state, crtc);
 
 		/* TODO This hack should go away */
@@ -4894,7 +4902,7 @@ static int dm_update_planes_state(struct dc *dc,
 			if (!dm_old_crtc_state->stream)
 				continue;
 
-			DRM_DEBUG_DRIVER("Disabling DRM plane: %d on DRM crtc %d\n",
+			DRM_DEBUG_ATOMIC("Disabling DRM plane: %d on DRM crtc %d\n",
 					plane->base.id, old_plane_crtc->base.id);
 
 			if (!dc_remove_plane_from_context(
-- 
2.17.0

[PATCH] drm/amd/display: Disallow enabling CRTC without primary plane with FB

[Harry Wentland]

The below commit

    "drm/atomic: Try to preserve the crtc enabled state in drm_atomic_remove_fb, v2"

introduces a slight behavioral change to rmfb. Instead of disabling a crtc
when the primary plane is disabled, it now preserves it.

Since DC is currently not equipped to handle this we need to fail such
a commit, otherwise we might see a corrupted screen.

This is based on Shirish's previous approach but avoids adding all
planes to the new atomic state which leads to a full update in DC for
any commit, and is not what we intend.

Theoretically DM should be able to deal with states with fully populated planes,
even for simple updates, such as cursor updates. This should still be
addressed in the future.

Signed-off-by: Harry Wentland <harry.wentland@amd.com>
Cc: stable@vger.kernel.org
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

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 6f92a19bebd6..0bdc6b484bad 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -4683,6 +4683,7 @@ static int dm_update_crtcs_state(struct amdgpu_display_manager *dm,
 		struct amdgpu_dm_connector *aconnector = NULL;
 		struct drm_connector_state *new_con_state = NULL;
 		struct dm_connector_state *dm_conn_state = NULL;
+		struct drm_plane_state *new_plane_state = NULL;
 
 		new_stream = NULL;
 
@@ -4690,6 +4691,13 @@ static int dm_update_crtcs_state(struct amdgpu_display_manager *dm,
 		dm_new_crtc_state = to_dm_crtc_state(new_crtc_state);
 		acrtc = to_amdgpu_crtc(crtc);
 
+		new_plane_state = drm_atomic_get_new_plane_state(state, new_crtc_state->crtc->primary);
+
+		if (new_crtc_state->enable && new_plane_state && !new_plane_state->fb) {
+			ret = -EINVAL;
+			goto fail;
+		}
+
 		aconnector = amdgpu_dm_find_first_crtc_matching_connector(state, crtc);
 
 		/* TODO This hack should go away */
@@ -4894,7 +4902,7 @@ static int dm_update_planes_state(struct dc *dc,
 			if (!dm_old_crtc_state->stream)
 				continue;
 
-			DRM_DEBUG_DRIVER("Disabling DRM plane: %d on DRM crtc %d\n",
+			DRM_DEBUG_ATOMIC("Disabling DRM plane: %d on DRM crtc %d\n",
 					plane->base.id, old_plane_crtc->base.id);
 
 			if (!dc_remove_plane_from_context(
-- 
2.17.0

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

Re: [PATCH] drm/amd/display: Disallow enabling CRTC without primary plane with FB

[Michel Dänzer]

[ Dropping stable@ (fixes with Cc: stable are picked up for stable
branches once they land in Linus' tree, there's no point sending them to
stable@ during review), adding dri-devel ]

On 2018-04-18 10:26 PM, Harry Wentland wrote:
> The below commit
> 
>     "drm/atomic: Try to preserve the crtc enabled state in drm_atomic_remove_fb, v2"
> 
> introduces a slight behavioral change to rmfb. Instead of disabling a crtc
> when the primary plane is disabled, it now preserves it.
> 
> Since DC is currently not equipped to handle this we need to fail such
> a commit, otherwise we might see a corrupted screen.

How does the caller react to failing such a commit?


> This is based on Shirish's previous approach but avoids adding all
> planes to the new atomic state which leads to a full update in DC for
> any commit, and is not what we intend.
> 
> Theoretically DM should be able to deal with states with fully populated planes,
> even for simple updates, such as cursor updates. This should still be
> addressed in the future.
> 
> Signed-off-by: Harry Wentland <harry.wentland@amd.com>
> Cc: stable@vger.kernel.org
> ---
>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> 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 6f92a19bebd6..0bdc6b484bad 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -4683,6 +4683,7 @@ static int dm_update_crtcs_state(struct amdgpu_display_manager *dm,
>  		struct amdgpu_dm_connector *aconnector = NULL;
>  		struct drm_connector_state *new_con_state = NULL;
>  		struct dm_connector_state *dm_conn_state = NULL;
> +		struct drm_plane_state *new_plane_state = NULL;
>  
>  		new_stream = NULL;
>  
> @@ -4690,6 +4691,13 @@ static int dm_update_crtcs_state(struct amdgpu_display_manager *dm,
>  		dm_new_crtc_state = to_dm_crtc_state(new_crtc_state);
>  		acrtc = to_amdgpu_crtc(crtc);
>  
> +		new_plane_state = drm_atomic_get_new_plane_state(state, new_crtc_state->crtc->primary);
> +
> +		if (new_crtc_state->enable && new_plane_state && !new_plane_state->fb) {
> +			ret = -EINVAL;
> +			goto fail;
> +		}
> +
>  		aconnector = amdgpu_dm_find_first_crtc_matching_connector(state, crtc);
>  
>  		/* TODO This hack should go away */
> @@ -4894,7 +4902,7 @@ static int dm_update_planes_state(struct dc *dc,
>  			if (!dm_old_crtc_state->stream)
>  				continue;
>  
> -			DRM_DEBUG_DRIVER("Disabling DRM plane: %d on DRM crtc %d\n",
> +			DRM_DEBUG_ATOMIC("Disabling DRM plane: %d on DRM crtc %d\n",
>  					plane->base.id, old_plane_crtc->base.id);
>  
>  			if (!dc_remove_plane_from_context(
> 


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amd/display: Disallow enabling CRTC without primary plane with FB

[Harry Wentland]

On 2018-04-19 03:43 AM, Michel Dänzer wrote:
> 
> [ Dropping stable@ (fixes with Cc: stable are picked up for stable
> branches once they land in Linus' tree, there's no point sending them to
> stable@ during review), adding dri-devel ]
> 
> On 2018-04-18 10:26 PM, Harry Wentland wrote:
>> The below commit
>>
>>     "drm/atomic: Try to preserve the crtc enabled state in drm_atomic_remove_fb, v2"
>>
>> introduces a slight behavioral change to rmfb. Instead of disabling a crtc
>> when the primary plane is disabled, it now preserves it.
>>
>> Since DC is currently not equipped to handle this we need to fail such
>> a commit, otherwise we might see a corrupted screen.
> 
> How does the caller react to failing such a commit?

The caller (drm_atomic_remove_fb in this case) will retry with the old behavior and disable the CRTC.

Harry

> 
> 
>> This is based on Shirish's previous approach but avoids adding all
>> planes to the new atomic state which leads to a full update in DC for
>> any commit, and is not what we intend.
>>
>> Theoretically DM should be able to deal with states with fully populated planes,
>> even for simple updates, such as cursor updates. This should still be
>> addressed in the future.
>>
>> Signed-off-by: Harry Wentland <harry.wentland@amd.com>
>> Cc: stable@vger.kernel.org
>> ---
>>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 10 +++++++++-
>>  1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> 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 6f92a19bebd6..0bdc6b484bad 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> @@ -4683,6 +4683,7 @@ static int dm_update_crtcs_state(struct amdgpu_display_manager *dm,
>>  		struct amdgpu_dm_connector *aconnector = NULL;
>>  		struct drm_connector_state *new_con_state = NULL;
>>  		struct dm_connector_state *dm_conn_state = NULL;
>> +		struct drm_plane_state *new_plane_state = NULL;
>>  
>>  		new_stream = NULL;
>>  
>> @@ -4690,6 +4691,13 @@ static int dm_update_crtcs_state(struct amdgpu_display_manager *dm,
>>  		dm_new_crtc_state = to_dm_crtc_state(new_crtc_state);
>>  		acrtc = to_amdgpu_crtc(crtc);
>>  
>> +		new_plane_state = drm_atomic_get_new_plane_state(state, new_crtc_state->crtc->primary);
>> +
>> +		if (new_crtc_state->enable && new_plane_state && !new_plane_state->fb) {
>> +			ret = -EINVAL;
>> +			goto fail;
>> +		}
>> +
>>  		aconnector = amdgpu_dm_find_first_crtc_matching_connector(state, crtc);
>>  
>>  		/* TODO This hack should go away */
>> @@ -4894,7 +4902,7 @@ static int dm_update_planes_state(struct dc *dc,
>>  			if (!dm_old_crtc_state->stream)
>>  				continue;
>>  
>> -			DRM_DEBUG_DRIVER("Disabling DRM plane: %d on DRM crtc %d\n",
>> +			DRM_DEBUG_ATOMIC("Disabling DRM plane: %d on DRM crtc %d\n",
>>  					plane->base.id, old_plane_crtc->base.id);
>>  
>>  			if (!dc_remove_plane_from_context(
>>
> 
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/amd/display: Disallow enabling CRTC without primary plane with FB

[Michel Dänzer]

On 2018-04-19 04:38 PM, Harry Wentland wrote:
> On 2018-04-19 03:43 AM, Michel Dänzer wrote:
>> On 2018-04-18 10:26 PM, Harry Wentland wrote:
>>> The below commit
>>>
>>>     "drm/atomic: Try to preserve the crtc enabled state in drm_atomic_remove_fb, v2"
>>>
>>> introduces a slight behavioral change to rmfb. Instead of disabling a crtc
>>> when the primary plane is disabled, it now preserves it.
>>>
>>> Since DC is currently not equipped to handle this we need to fail such
>>> a commit, otherwise we might see a corrupted screen.
>>
>> How does the caller react to failing such a commit?
> 
> The caller (drm_atomic_remove_fb in this case) will retry with the old behavior and disable the CRTC.

I see, thanks.

FWIW, stopping gdm3 works fine with this patch as well.

Tested-by: Michel Dänzer <michel.daenzer@amd.com>


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amd/display: Disallow enabling CRTC without primary plane with FB

[S, Shirish]

[-- Attachment #1.1: Type: text/plain, Size: 3312 bytes --]



On 4/19/2018 8:08 PM, Harry Wentland wrote:
> On 2018-04-19 03:43 AM, Michel Dänzer wrote:
>> [ Dropping stable@ (fixes with Cc: stable are picked up for stable
>> branches once they land in Linus' tree, there's no point sending them to
>> stable@ during review), adding dri-devel ]
>>
>> On 2018-04-18 10:26 PM, Harry Wentland wrote:
>>> The below commit
>>>
>>>      "drm/atomic: Try to preserve the crtc enabled state in drm_atomic_remove_fb, v2"
>>>
>>> introduces a slight behavioral change to rmfb. Instead of disabling a crtc
>>> when the primary plane is disabled, it now preserves it.
>>>
>>> Since DC is currently not equipped to handle this we need to fail such
>>> a commit, otherwise we might see a corrupted screen.
>> How does the caller react to failing such a commit?
> The caller (drm_atomic_remove_fb in this case) will retry with the old behavior and disable the CRTC.
>
> Harry
That's the fall back logic suggested in the patch that caused this issue.

This patch is  Reviewed-by: Shirish S <shirish.s@amd.com 
<mailto:harry.wentland@amd.com>>

Regards,

Shirish S

>>
>>> This is based on Shirish's previous approach but avoids adding all
>>> planes to the new atomic state which leads to a full update in DC for
>>> any commit, and is not what we intend.
>>>
>>> Theoretically DM should be able to deal with states with fully populated planes,
>>> even for simple updates, such as cursor updates. This should still be
>>> addressed in the future.
>>>
>>> Signed-off-by: Harry Wentland <harry.wentland@amd.com>
>>> Cc: stable@vger.kernel.org
>>> ---
>>>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 10 +++++++++-
>>>   1 file changed, 9 insertions(+), 1 deletion(-)
>>>
>>> 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 6f92a19bebd6..0bdc6b484bad 100644
>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> @@ -4683,6 +4683,7 @@ static int dm_update_crtcs_state(struct amdgpu_display_manager *dm,
>>>   		struct amdgpu_dm_connector *aconnector = NULL;
>>>   		struct drm_connector_state *new_con_state = NULL;
>>>   		struct dm_connector_state *dm_conn_state = NULL;
>>> +		struct drm_plane_state *new_plane_state = NULL;
>>>   
>>>   		new_stream = NULL;
>>>   
>>> @@ -4690,6 +4691,13 @@ static int dm_update_crtcs_state(struct amdgpu_display_manager *dm,
>>>   		dm_new_crtc_state = to_dm_crtc_state(new_crtc_state);
>>>   		acrtc = to_amdgpu_crtc(crtc);
>>>   
>>> +		new_plane_state = drm_atomic_get_new_plane_state(state, new_crtc_state->crtc->primary);
>>> +
>>> +		if (new_crtc_state->enable && new_plane_state && !new_plane_state->fb) {
>>> +			ret = -EINVAL;
>>> +			goto fail;
>>> +		}
>>> +
>>>   		aconnector = amdgpu_dm_find_first_crtc_matching_connector(state, crtc);
>>>   
>>>   		/* TODO This hack should go away */
>>> @@ -4894,7 +4902,7 @@ static int dm_update_planes_state(struct dc *dc,
>>>   			if (!dm_old_crtc_state->stream)
>>>   				continue;
>>>   
>>> -			DRM_DEBUG_DRIVER("Disabling DRM plane: %d on DRM crtc %d\n",
>>> +			DRM_DEBUG_ATOMIC("Disabling DRM plane: %d on DRM crtc %d\n",
>>>   					plane->base.id, old_plane_crtc->base.id);
>>>   
>>>   			if (!dc_remove_plane_from_context(
>>>
>>


[-- Attachment #1.2: Type: text/html, Size: 4394 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