Re: [PATCH v2] drm/amdgpu/dc: Require primary plane to be enabled whenever the CRTC is
Applied. Thanks! Alex On Mon, Sep 14, 2020 at 10:37 AM 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 > >>> > >>> 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 > >>> * _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 > >>> Signed-off-by: Michel Dänzer > >> > >> Commit message agrees with my understand of this maze now, so: > >> > >> Acked-by: Daniel Vetter > > > > 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. > > 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 since the > blank plane hack is going to add a significant amount of complexity to DM. > > Reviewed-by: Nicholas Kazlauskas > > Regards, > Nicholas Kazlauskas > ___ > amd-gfx mailing list > amd-...@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2] drm/amdgpu/dc: Require primary plane to be enabled whenever the CRTC is
On 2020-09-14 5:33 p.m., Kazlauskas, Nicholas wrote: 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: 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. Interesting, so if we're lucky this really won't affect any real-world apps. 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. As Daniel pointed out in <20200901075432.GW2352366@phenom.ffwll.local> in the v1 patch thread, that won't fly, since atomic userspace can legitimately expect the cursor plane to be visible in that case. -- Earthling Michel Dänzer | https://redhat.com Libre software enthusiast | Mesa and X developer ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2] drm/amdgpu/dc: Require primary plane to be enabled whenever the CRTC is
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 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 * _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 Signed-off-by: Michel Dänzer Commit message agrees with my understand of this maze now, so: Acked-by: Daniel Vetter 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 Thanks! ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2] drm/amdgpu/dc: Require primary plane to be enabled whenever the CRTC is
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 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 * _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 Signed-off-by: Michel Dänzer Commit message agrees with my understand of this maze now, so: Acked-by: Daniel Vetter 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 Thanks! -- Earthling Michel Dänzer | https://redhat.com Libre software enthusiast | Mesa and X developer ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2] drm/amdgpu/dc: Require primary plane to be enabled whenever the CRTC is
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 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 * _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 Signed-off-by: Michel Dänzer Commit message agrees with my understand of this maze now, so: Acked-by: Daniel Vetter 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. 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 since the blank plane hack is going to add a significant amount of complexity to DM. Reviewed-by: Nicholas Kazlauskas Regards, Nicholas Kazlauskas ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2] drm/amdgpu/dc: Require primary plane to be enabled whenever the CRTC is
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 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 * _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 Signed-off-by: Michel Dänzer Commit message agrees with my understand of this maze now, so: Acked-by: Daniel Vetter 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". -- Earthling Michel Dänzer | https://redhat.com Libre software enthusiast | Mesa and X developer ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2] drm/amdgpu/dc: Require primary plane to be enabled whenever the CRTC is
On Fri, Sep 04, 2020 at 12:43:04PM +0200, Michel Dänzer wrote:
> From: Michel Dänzer
>
> 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
> * _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
> Signed-off-by: Michel Dänzer
Commit message agrees with my understand of this maze now, so:
Acked-by: Daniel Vetter
> ---
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 32 ++-
> 1 file changed, 10 insertions(+), 22 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 45f5f4b7024d..c5f9452490d6 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -5269,19 +5269,6 @@ static void dm_crtc_helper_disable(struct drm_crtc
> *crtc)
> {
> }
>
> -static bool does_crtc_have_active_cursor(struct drm_crtc_state
> *new_crtc_state)
> -{
> - struct drm_device *dev = new_crtc_state->crtc->dev;
> - struct drm_plane *plane;
> -
> - drm_for_each_plane_mask(plane, dev, new_crtc_state->plane_mask) {
> - if (plane->type == DRM_PLANE_TYPE_CURSOR)
> - return true;
> - }
> -
> - return false;
> -}
> -
> static int count_crtc_active_planes(struct drm_crtc_state *new_crtc_state)
> {
> struct drm_atomic_state *state = new_crtc_state->state;
> @@ -5345,19 +5332,20 @@ static int dm_crtc_helper_atomic_check(struct
> drm_crtc *crtc,
> return ret;
> }
>
> - /* In some use cases, like reset, no stream is attached */
> - if (!dm_crtc_state->stream)
> - return 0;
> -
> /*
> - * We want at least one hardware plane enabled to use
> - * the stream with a cursor enabled.
> + * We require the primary plane to be enabled whenever the CRTC is,
> otherwise
> + * drm_mode_cursor_universal may end up trying to enable the cursor
> plane while all other
> + * planes are disabled, which is not supported by the hardware. And
> there is legacy
> + * userspace which stops using the HW cursor altogether in response to
> the resulting EINVAL.
>*/
> - if (state->enable && state->active &&
> - does_crtc_have_active_cursor(state) &&
> - dm_crtc_state->active_planes == 0)
> + if (state->enable &&
> + !(state->plane_mask & drm_plane_mask(crtc->primary)))
> return -EINVAL;
>
> + /* In some use cases, like reset, no stream is attached */
> + if (!dm_crtc_state->stream)
> + return 0;
> +
> if (dc_validate_stream(dc, dm_crtc_state->stream) == DC_OK)
> return 0;
>
> --
> 2.28.0
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2] drm/amdgpu/dc: Require primary plane to be enabled whenever the CRTC is
From: Michel Dänzer
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
* _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
Signed-off-by: Michel Dänzer
---
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 32 ++-
1 file changed, 10 insertions(+), 22 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 45f5f4b7024d..c5f9452490d6 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -5269,19 +5269,6 @@ static void dm_crtc_helper_disable(struct drm_crtc *crtc)
{
}
-static bool does_crtc_have_active_cursor(struct drm_crtc_state *new_crtc_state)
-{
- struct drm_device *dev = new_crtc_state->crtc->dev;
- struct drm_plane *plane;
-
- drm_for_each_plane_mask(plane, dev, new_crtc_state->plane_mask) {
- if (plane->type == DRM_PLANE_TYPE_CURSOR)
- return true;
- }
-
- return false;
-}
-
static int count_crtc_active_planes(struct drm_crtc_state *new_crtc_state)
{
struct drm_atomic_state *state = new_crtc_state->state;
@@ -5345,19 +5332,20 @@ static int dm_crtc_helper_atomic_check(struct drm_crtc
*crtc,
return ret;
}
- /* In some use cases, like reset, no stream is attached */
- if (!dm_crtc_state->stream)
- return 0;
-
/*
-* We want at least one hardware plane enabled to use
-* the stream with a cursor enabled.
+* We require the primary plane to be enabled whenever the CRTC is,
otherwise
+* drm_mode_cursor_universal may end up trying to enable the cursor
plane while all other
+* planes are disabled, which is not supported by the hardware. And
there is legacy
+* userspace which stops using the HW cursor altogether in response to
the resulting EINVAL.
*/
- if (state->enable && state->active &&
- does_crtc_have_active_cursor(state) &&
- dm_crtc_state->active_planes == 0)
+ if (state->enable &&
+ !(state->plane_mask & drm_plane_mask(crtc->primary)))
return -EINVAL;
+ /* In some use cases, like reset, no stream is attached */
+ if (!dm_crtc_state->stream)
+ return 0;
+
if (dc_validate_stream(dc, dm_crtc_state->stream) == DC_OK)
return 0;
--
2.28.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
