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

[Kazlauskas, Nicholas]

On 2020-09-14 11:22 a.m., Michel Dänzer wrote:
> 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).

We only expose support for NV12 on the primary plane so we wouldn't be 
hitting this case at least.

> 
> 
>> 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.

Dropping the check you added in this patch:

+	if (state->enable &&
+	    !(state->plane_mask & drm_plane_mask(crtc->primary)))
  		return -EINVAL;

That way we can still allow the cursor plane to be enabled while 
primary/overlay are not, it's just not going to be actually visible on 
the screen. It'll fail some IGT tests but nothing really uses this 
configuration as more than just a temporary state.

Regards,
Nicholas Kazlauskas

> 
> 
>> Reviewed-by: Nicholas Kazlauskas <nicholas.kazlauskas at amd.com>
> 
> Thanks!
> 
>