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

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