Re: [PATCH] Revert "drm/amd/display: Fix overlay validation by considering cursors"

[Harry Wentland <harry.wentland@amd.com>]

On 2021-06-16 1:14 p.m., Sean Paul wrote:
> On Wed, Jun 16, 2021 at 12:21 PM Rodrigo Siqueira
> <Rodrigo.Siqueira@amd.com> wrote:
>>
>> This reverts commit 04cc17a951f73f9a9092ca572b063e6292aeb085.
>>
>> The patch that we are reverting here was originally applied because it
>> fixes multiple IGT issues and flickering in Android. However, after a
>> discussion with Sean Paul and Mark, it looks like that this patch might
>> cause problems on ChromeOS. For this reason, we decided to revert this
>> patch.
> 
> Thanks for sending this, Siqueira!
> 
> To be clear for those unfamiliar, the issue extends beyond ChromeOS
> (we're not just pushing our compositor problems on the rest of the
> community).
> 
> Relying on cursor enable/disable for atomic creates non-deterministic
> behavior which would be very hard for any compositor to reason out
> without knowing the hardware-specific limitations. The case I'm
> worried about is that the compositor has an overlay active without the
> cursor and at some point the compositor enables the cursor which will
> fail because of the overlay.
> 

Previous discussion highlighted that the cursor IOCTL should never
fail and that userspace generally is not well equipped to deal with
it if it does.

https://patchwork.freedesktop.org/patch/387230/
https://patchwork.freedesktop.org/patch/389084/

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

Harry

> Reviewed-by: Sean Paul <seanpaul@chromium.org>
> 
>>
>> Cc: Nicholas Kazlauskas <Nicholas.Kazlauskas@amd.com>
>> Cc: Harry Wentland <Harry.Wentland@amd.com>
>> Cc: Hersen Wu <hersenxs.wu@amd.com>
>> Cc: Sean Paul <seanpaul@chromium.org>
>> Cc: Mark Yacoub <markyacoub@chromium.org>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Signed-off-by: Rodrigo Siqueira <Rodrigo.Siqueira@amd.com>
>> ---
>>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 12 ++----------
>>  1 file changed, 2 insertions(+), 10 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 8358112b5822..3fd41e098c90 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> @@ -10200,8 +10200,8 @@ static int validate_overlay(struct drm_atomic_state *state)
>>  {
>>         int i;
>>         struct drm_plane *plane;
>> -       struct drm_plane_state *new_plane_state;
>> -       struct drm_plane_state *primary_state, *cursor_state, *overlay_state = NULL;
>> +       struct drm_plane_state *old_plane_state, *new_plane_state;
>> +       struct drm_plane_state *primary_state, *overlay_state = NULL;
>>
>>         /* Check if primary plane is contained inside overlay */
>>         for_each_new_plane_in_state_reverse(state, plane, new_plane_state, i) {
>> @@ -10231,14 +10231,6 @@ static int validate_overlay(struct drm_atomic_state *state)
>>         if (!primary_state->crtc)
>>                 return 0;
>>
>> -       /* check if cursor plane is enabled */
>> -       cursor_state = drm_atomic_get_plane_state(state, overlay_state->crtc->cursor);
>> -       if (IS_ERR(cursor_state))
>> -               return PTR_ERR(cursor_state);
>> -
>> -       if (drm_atomic_plane_disabling(plane->state, cursor_state))
>> -               return 0;
>> -
>>         /* Perform the bounds check to ensure the overlay plane covers the primary */
>>         if (primary_state->crtc_x < overlay_state->crtc_x ||
>>             primary_state->crtc_y < overlay_state->crtc_y ||
>> --
>> 2.25.1
>>

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