Re: [PATCH v2] drm/amdgpu/dc: Require primary plane to be enabled whenever the CRTC is

From: "Michel Dänzer" <michel@daenzer.net>
To: "Kazlauskas, Nicholas" <nicholas.kazlauskas@amd.com>,
	Daniel Vetter <daniel@ffwll.ch>,
	Harry Wentland <hwentlan@amd.com>
Cc: dri-devel@lists.freedesktop.org, amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH v2] drm/amdgpu/dc: Require primary plane to be enabled whenever the CRTC is
Date: Mon, 14 Sep 2020 17:22:13 +0200	[thread overview]
Message-ID: <2c090b33-2709-1f83-41ec-56fc5cc9e2ce@daenzer.net> (raw)
In-Reply-To: <c64d591b-8f3c-5384-f103-3283bcf15c9d@amd.com>

On 2020-09-14 4:37 p.m., Kazlauskas, Nicholas wrote:
> On 2020-09-14 3:52 a.m., Michel Dänzer wrote:
>> On 2020-09-07 9:57 a.m., Daniel Vetter wrote:
>>> On Fri, Sep 04, 2020 at 12:43:04PM +0200, Michel Dänzer wrote:
>>>> From: Michel Dänzer <mdaenzer@redhat.com>
>>>>
>>>> Don't check drm_crtc_state::active for this either, per its
>>>> documentation in include/drm/drm_crtc.h:
>>>>
>>>>   * Hence drivers must not consult @active in their various
>>>>   * &drm_mode_config_funcs.atomic_check callback to reject an atomic
>>>>   * commit.
>>>>
>>>> atomic_remove_fb disables the CRTC as needed for disabling the primary
>>>> plane.
>>>>
>>>> This prevents at least the following problems if the primary plane gets
>>>> disabled (e.g. due to destroying the FB assigned to the primary plane,
>>>> as happens e.g. with mutter in Wayland mode):
>>>>
>>>> * The legacy cursor ioctl returned EINVAL for a non-0 cursor FB ID
>>>>    (which enables the cursor plane).
>>>> * If the cursor plane was enabled, changing the legacy DPMS property
>>>>    value from off to on returned EINVAL.
>>>>
>>>> v2:
>>>> * Minor changes to code comment and commit log, per review feedback.
>>>>
>>>> GitLab: https://gitlab.gnome.org/GNOME/mutter/-/issues/1108
>>>> GitLab: https://gitlab.gnome.org/GNOME/mutter/-/issues/1165
>>>> GitLab: https://gitlab.gnome.org/GNOME/mutter/-/issues/1344
>>>> Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>> Signed-off-by: Michel Dänzer <mdaenzer@redhat.com>
>>>
>>> Commit message agrees with my understand of this maze now, so:
>>>
>>> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>
>> Thanks Daniel!
>>
>>
>> Nick / Harry, any more feedback? If not, can you apply this?
>>
>>
>> P.S. Since DCN doesn't make a distinction between primary or overlay 
>> planes in hardware, could ChromiumOS achieve the same effect with only 
>> the primary plane instead of only an overlay plane? If so, maybe 
>> there's no need for the "black plane hack".
>>
>>
> 
> I only know that atomictest tries to enable this configuration. Not sure 
> if ChromiumOS or other "real" userspace tries this configuration.

Someone mentioned that ChromiumOS uses this for video playback with 
black bars (when the video aspect ratio doesn't match the display's).

> Maybe for now this can go in and if something breaks we can deal with 
> the fallout then. We can always go back to lying to userspace about the 
> cursor being visible if the commit fails in that case I guess [...]

I'm not sure what you mean by that / how it could work.

> Reviewed-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>

Thanks!

-- 
Earthling Michel Dänzer               |               https://redhat.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