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

[Michel Dänzer]

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 at 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 at ffwll.ch>
>>>> Signed-off-by: Michel Dänzer <mdaenzer at redhat.com>
>>>
>>> Commit message agrees with my understand of this maze now, so:
>>>
>>> Acked-by: Daniel Vetter <daniel.vetter at 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 at amd.com>

Thanks!


-- 
Earthling Michel Dänzer               |               https://redhat.com
Libre software enthusiast             |             Mesa and X developer