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

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

[Michel Dänzer]

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.

The atomic helpers disable 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):

* Toggling CRTC active to 1 failed if the cursor plane was enabled
  (e.g. via legacy DPMS property & cursor ioctl).
* Enabling the cursor plane failed, e.g. via the legacy cursor ioctl.

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

Note that this will cause some IGT tests to fail without
https://patchwork.freedesktop.org/series/80904/ .

 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 33 +++++++------------
 1 file changed, 11 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 897d60ade1e4..33c5739e221b 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -5262,19 +5262,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;
@@ -5338,19 +5325,21 @@ 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 the legacy cursor ioctl helper may end up trying to enable
+	 * the cursor plane while the primary plane is 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

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

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

[Michel Dänzer]

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.

The atomic helpers disable 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):

* Toggling CRTC active to 1 failed if the cursor plane was enabled
  (e.g. via legacy DPMS property & cursor ioctl).
* Enabling the cursor plane failed, e.g. via the legacy cursor ioctl.

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

Note that this will cause some IGT tests to fail without
https://patchwork.freedesktop.org/series/80904/ .

 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 33 +++++++------------
 1 file changed, 11 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 897d60ade1e4..33c5739e221b 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -5262,19 +5262,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;
@@ -5338,19 +5325,21 @@ 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 the legacy cursor ioctl helper may end up trying to enable
+	 * the cursor plane while the primary plane is 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

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

[Kazlauskas, Nicholas]

On 2020-08-21 12:57 p.m., 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.
> 
> The atomic helpers disable 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):
> 
> * Toggling CRTC active to 1 failed if the cursor plane was enabled
>    (e.g. via legacy DPMS property & cursor ioctl).
> * Enabling the cursor plane failed, e.g. via the legacy cursor ioctl.

We previously had the requirement that the primary plane must be enabled 
but some userspace expects that they can enable just the overlay plane 
without anything else.

I think the chromuiumos atomictest validates that this works as well:

So is DRM going forward then with the expectation that this is wrong 
behavior from userspace?

We require at least one plane to be enabled to display a cursor, but it 
doesn't necessarily need to be the primary.

Regards,
Nicholas Kazlauskas

> 
> 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>
> ---
> 
> Note that this will cause some IGT tests to fail without
> https://patchwork.freedesktop.org/series/80904/ .
> 
>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 33 +++++++------------
>   1 file changed, 11 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 897d60ade1e4..33c5739e221b 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -5262,19 +5262,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;
> @@ -5338,19 +5325,21 @@ 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 the legacy cursor ioctl helper may end up trying to enable
> +	 * the cursor plane while the primary plane is 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;
>   
> 

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

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

[Kazlauskas, Nicholas]

On 2020-08-21 12:57 p.m., 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.
> 
> The atomic helpers disable 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):
> 
> * Toggling CRTC active to 1 failed if the cursor plane was enabled
>    (e.g. via legacy DPMS property & cursor ioctl).
> * Enabling the cursor plane failed, e.g. via the legacy cursor ioctl.

We previously had the requirement that the primary plane must be enabled 
but some userspace expects that they can enable just the overlay plane 
without anything else.

I think the chromuiumos atomictest validates that this works as well:

So is DRM going forward then with the expectation that this is wrong 
behavior from userspace?

We require at least one plane to be enabled to display a cursor, but it 
doesn't necessarily need to be the primary.

Regards,
Nicholas Kazlauskas

> 
> 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>
> ---
> 
> Note that this will cause some IGT tests to fail without
> https://patchwork.freedesktop.org/series/80904/ .
> 
>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 33 +++++++------------
>   1 file changed, 11 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 897d60ade1e4..33c5739e221b 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -5262,19 +5262,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;
> @@ -5338,19 +5325,21 @@ 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 the legacy cursor ioctl helper may end up trying to enable
> +	 * the cursor plane while the primary plane is 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;
>   
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Michel Dänzer]

On 2020-08-21 8:07 p.m., Kazlauskas, Nicholas wrote:
> On 2020-08-21 12:57 p.m., 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.
>>
>> The atomic helpers disable 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):
>>
>> * Toggling CRTC active to 1 failed if the cursor plane was enabled
>>    (e.g. via legacy DPMS property & cursor ioctl).
>> * Enabling the cursor plane failed, e.g. via the legacy cursor ioctl.
> 
> We previously had the requirement that the primary plane must be enabled
> but some userspace expects that they can enable just the overlay plane
> without anything else.
> 
> I think the chromuiumos atomictest validates that this works as well:
> 
> So is DRM going forward then with the expectation that this is wrong
> behavior from userspace?
> 
> We require at least one plane to be enabled to display a cursor, but it
> doesn't necessarily need to be the primary.

It's a "pick your poison" situation:

1) Currently the checks are invalid (atomic_check must not decide based
on drm_crtc_state::active), and it's easy for legacy KMS userspace to
accidentally hit errors trying to enable/move the cursor or switch DPMS
off → on.

2) Accurately rejecting only atomic states where the cursor plane is
enabled but all other planes are off would break the KMS helper code,
which can only deal with the "CRTC on & primary plane off is not
allowed" case specifically.

3) This patch addresses 1) & 2) but may break existing atomic userspace
which wants to enable an overlay plane while disabling the primary plane.


I do think in principle atomic userspace is expected to handle case 3)
and leave the primary plane enabled. However, this is not ideal from an
energy consumption PoV. Therefore, here's another idea for a possible
way out of this quagmire:

amdgpu_dm does not reject any atomic states based on which planes are
enabled in it. If the cursor plane is enabled but all other planes are
off, amdgpu_dm internally either:

a) Enables an overlay plane and makes it invisible, e.g. by assigning a
minimum size FB with alpha = 0.

b) Enables the primary plane and assigns a minimum size FB (scaled up to
the required size) containing all black, possibly using compression.
(Trying to minimize the memory bandwidth)


Does either of these seem feasible? If both do, which one would be
preferable?


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

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

[Michel Dänzer]

On 2020-08-21 8:07 p.m., Kazlauskas, Nicholas wrote:
> On 2020-08-21 12:57 p.m., 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.
>>
>> The atomic helpers disable 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):
>>
>> * Toggling CRTC active to 1 failed if the cursor plane was enabled
>>    (e.g. via legacy DPMS property & cursor ioctl).
>> * Enabling the cursor plane failed, e.g. via the legacy cursor ioctl.
> 
> We previously had the requirement that the primary plane must be enabled
> but some userspace expects that they can enable just the overlay plane
> without anything else.
> 
> I think the chromuiumos atomictest validates that this works as well:
> 
> So is DRM going forward then with the expectation that this is wrong
> behavior from userspace?
> 
> We require at least one plane to be enabled to display a cursor, but it
> doesn't necessarily need to be the primary.

It's a "pick your poison" situation:

1) Currently the checks are invalid (atomic_check must not decide based
on drm_crtc_state::active), and it's easy for legacy KMS userspace to
accidentally hit errors trying to enable/move the cursor or switch DPMS
off → on.

2) Accurately rejecting only atomic states where the cursor plane is
enabled but all other planes are off would break the KMS helper code,
which can only deal with the "CRTC on & primary plane off is not
allowed" case specifically.

3) This patch addresses 1) & 2) but may break existing atomic userspace
which wants to enable an overlay plane while disabling the primary plane.


I do think in principle atomic userspace is expected to handle case 3)
and leave the primary plane enabled. However, this is not ideal from an
energy consumption PoV. Therefore, here's another idea for a possible
way out of this quagmire:

amdgpu_dm does not reject any atomic states based on which planes are
enabled in it. If the cursor plane is enabled but all other planes are
off, amdgpu_dm internally either:

a) Enables an overlay plane and makes it invisible, e.g. by assigning a
minimum size FB with alpha = 0.

b) Enables the primary plane and assigns a minimum size FB (scaled up to
the required size) containing all black, possibly using compression.
(Trying to minimize the memory bandwidth)


Does either of these seem feasible? If both do, which one would be
preferable?


-- 
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] drm/amdgpu/dc: Require primary plane to be enabled whenever the CRTC is

[Pekka Paalanen]

[-- Attachment #1.1: Type: text/plain, Size: 3761 bytes --]

On Sat, 22 Aug 2020 11:59:26 +0200
Michel Dänzer <michel@daenzer.net> wrote:

> On 2020-08-21 8:07 p.m., Kazlauskas, Nicholas wrote:
> > On 2020-08-21 12:57 p.m., 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.
> >>
> >> The atomic helpers disable 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):
> >>
> >> * Toggling CRTC active to 1 failed if the cursor plane was enabled
> >>    (e.g. via legacy DPMS property & cursor ioctl).
> >> * Enabling the cursor plane failed, e.g. via the legacy cursor ioctl.  
> > 
> > We previously had the requirement that the primary plane must be enabled
> > but some userspace expects that they can enable just the overlay plane
> > without anything else.
> > 
> > I think the chromuiumos atomictest validates that this works as well:
> > 
> > So is DRM going forward then with the expectation that this is wrong
> > behavior from userspace?
> > 
> > We require at least one plane to be enabled to display a cursor, but it
> > doesn't necessarily need to be the primary.  
> 
> It's a "pick your poison" situation:
> 
> 1) Currently the checks are invalid (atomic_check must not decide based
> on drm_crtc_state::active), and it's easy for legacy KMS userspace to
> accidentally hit errors trying to enable/move the cursor or switch DPMS
> off → on.
> 
> 2) Accurately rejecting only atomic states where the cursor plane is
> enabled but all other planes are off would break the KMS helper code,
> which can only deal with the "CRTC on & primary plane off is not
> allowed" case specifically.
> 
> 3) This patch addresses 1) & 2) but may break existing atomic userspace
> which wants to enable an overlay plane while disabling the primary plane.
> 
> 
> I do think in principle atomic userspace is expected to handle case 3)
> and leave the primary plane enabled.

Hi,

my opinion as a userspace developer is that enabling a CRTC without a
primary plane has traditionally not worked, so userspace cannot *rely*
on it ever working. I think legacy KMS API does not even allow to
express that really, or has built-in assumptions that it doesn't work -
you can call the legacy cursor ioctls, they don't fail, but also the
CRTC remains off. Correct me if this is not true.

Atomic userspace OTOH is required to test the strange (and all!) cases
like this to see if they work or not, and carry on with a fallback if
they don't. Userspace that wants to use a CRTC without a primary plane
likely needs other CRTC properties as well, like background color.

I would guess that when two regressions conflict, the older userspace
would win. Hence legacy KMS using Mutter should keep working, while
atomic userspace is left with increased power consumption. Not using a
hardware cursor (Mutter) is much more easily an end-user visible
regression than slightly shorter battery life.

Atomic test commits are allowed to fail at any time for any reason and
something that previously worked can fail on the next frame or on the
next kernel, as far as I understand.

Sounds like the helpers you refer to are inadequate for your case.
Can't you fix the helpers in the long run and land this patch as an
immediate fix?


Thanks,
pq

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

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

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

[Pekka Paalanen]

[-- Attachment #1.1: Type: text/plain, Size: 3761 bytes --]

On Sat, 22 Aug 2020 11:59:26 +0200
Michel Dänzer <michel@daenzer.net> wrote:

> On 2020-08-21 8:07 p.m., Kazlauskas, Nicholas wrote:
> > On 2020-08-21 12:57 p.m., 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.
> >>
> >> The atomic helpers disable 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):
> >>
> >> * Toggling CRTC active to 1 failed if the cursor plane was enabled
> >>    (e.g. via legacy DPMS property & cursor ioctl).
> >> * Enabling the cursor plane failed, e.g. via the legacy cursor ioctl.  
> > 
> > We previously had the requirement that the primary plane must be enabled
> > but some userspace expects that they can enable just the overlay plane
> > without anything else.
> > 
> > I think the chromuiumos atomictest validates that this works as well:
> > 
> > So is DRM going forward then with the expectation that this is wrong
> > behavior from userspace?
> > 
> > We require at least one plane to be enabled to display a cursor, but it
> > doesn't necessarily need to be the primary.  
> 
> It's a "pick your poison" situation:
> 
> 1) Currently the checks are invalid (atomic_check must not decide based
> on drm_crtc_state::active), and it's easy for legacy KMS userspace to
> accidentally hit errors trying to enable/move the cursor or switch DPMS
> off → on.
> 
> 2) Accurately rejecting only atomic states where the cursor plane is
> enabled but all other planes are off would break the KMS helper code,
> which can only deal with the "CRTC on & primary plane off is not
> allowed" case specifically.
> 
> 3) This patch addresses 1) & 2) but may break existing atomic userspace
> which wants to enable an overlay plane while disabling the primary plane.
> 
> 
> I do think in principle atomic userspace is expected to handle case 3)
> and leave the primary plane enabled.

Hi,

my opinion as a userspace developer is that enabling a CRTC without a
primary plane has traditionally not worked, so userspace cannot *rely*
on it ever working. I think legacy KMS API does not even allow to
express that really, or has built-in assumptions that it doesn't work -
you can call the legacy cursor ioctls, they don't fail, but also the
CRTC remains off. Correct me if this is not true.

Atomic userspace OTOH is required to test the strange (and all!) cases
like this to see if they work or not, and carry on with a fallback if
they don't. Userspace that wants to use a CRTC without a primary plane
likely needs other CRTC properties as well, like background color.

I would guess that when two regressions conflict, the older userspace
would win. Hence legacy KMS using Mutter should keep working, while
atomic userspace is left with increased power consumption. Not using a
hardware cursor (Mutter) is much more easily an end-user visible
regression than slightly shorter battery life.

Atomic test commits are allowed to fail at any time for any reason and
something that previously worked can fail on the next frame or on the
next kernel, as far as I understand.

Sounds like the helpers you refer to are inadequate for your case.
Can't you fix the helpers in the long run and land this patch as an
immediate fix?


Thanks,
pq

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Michel Dänzer]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 2020-08-24 9:43 a.m., Pekka Paalanen wrote:
> On Sat, 22 Aug 2020 11:59:26 +0200 Michel Dänzer
> <michel@daenzer.net> wrote:
>> On 2020-08-21 8:07 p.m., Kazlauskas, Nicholas wrote:
>>> On 2020-08-21 12:57 p.m., 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.
>>>>
>>>> The atomic helpers disable 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):
>>>>
>>>> * Toggling CRTC active to 1 failed if the cursor plane was
>>>> enabled (e.g. via legacy DPMS property & cursor ioctl). *
>>>> Enabling the cursor plane failed, e.g. via the legacy cursor
>>>> ioctl.
>>>
>>> We previously had the requirement that the primary plane must
>>> be enabled but some userspace expects that they can enable
>>> just the overlay plane without anything else.
>>>
>>> I think the chromuiumos atomictest validates that this works
>>> as well:
>>>
>>> So is DRM going forward then with the expectation that this is
>>> wrong behavior from userspace?
>>>
>>> We require at least one plane to be enabled to display a
>>> cursor, but it doesn't necessarily need to be the primary.
>>
>> It's a "pick your poison" situation:
>>
>> 1) Currently the checks are invalid (atomic_check must not
>> decide based on drm_crtc_state::active), and it's easy for legacy
>> KMS userspace to accidentally hit errors trying to enable/move
>> the cursor or switch DPMS off → on.
>>
>> 2) Accurately rejecting only atomic states where the cursor
>> plane is enabled but all other planes are off would break the
>> KMS helper code, which can only deal with the "CRTC on & primary
>> plane off is not allowed" case specifically.
>>
>> 3) This patch addresses 1) & 2) but may break existing atomic
>> userspace which wants to enable an overlay plane while disabling
>> the primary plane.
>>
>>
>> I do think in principle atomic userspace is expected to handle
>> case 3) and leave the primary plane enabled.
>
> Hi,
>
> my opinion as a userspace developer is that enabling a CRTC
> without a primary plane has traditionally not worked, so userspace
> cannot *rely* on it ever working. I think legacy KMS API does not
> even allow to express that really, or has built-in assumptions that
> it doesn't work - you can call the legacy cursor ioctls, they
> don't fail, but also the CRTC remains off. Correct me if this is
> not true.
>
> Atomic userspace OTOH is required to test the strange (and all!)
> cases like this to see if they work or not, and carry on with a
> fallback if they don't. Userspace that wants to use a CRTC without
> a primary plane likely needs other CRTC properties as well, like
> background color.
>
> I would guess that when two regressions conflict, the older
> userspace would win. Hence legacy KMS using Mutter should keep
> working, while atomic userspace is left with increased power
> consumption. Not using a hardware cursor (Mutter) is much more
> easily an end-user visible regression than slightly shorter
> battery life.
>
> Atomic test commits are allowed to fail at any time for any reason
> and something that previously worked can fail on the next frame or
> on the next kernel, as far as I understand.

All of this basically matches my understanding.


> Sounds like the helpers you refer to are inadequate for your case.
> Can't you fix the helpers in the long run and land this patch as an
> immediate fix?

I'd rather leave working on those helpers to KMS developers. :)


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

iF0EARECAB0WIQSwn681vpFFIZgJURRaga+OatuyAAUCX0UmXAAKCRBaga+Oatuy
AIeNAJoC9UgOrF+qBq08uOyjaV7Vfp+PgACfSp3nXB3un3LUZQxrvaxMAON+eIs=
=Pbd2
-----END PGP SIGNATURE-----
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Michel Dänzer]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 2020-08-24 9:43 a.m., Pekka Paalanen wrote:
> On Sat, 22 Aug 2020 11:59:26 +0200 Michel Dänzer
> <michel@daenzer.net> wrote:
>> On 2020-08-21 8:07 p.m., Kazlauskas, Nicholas wrote:
>>> On 2020-08-21 12:57 p.m., 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.
>>>>
>>>> The atomic helpers disable 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):
>>>>
>>>> * Toggling CRTC active to 1 failed if the cursor plane was
>>>> enabled (e.g. via legacy DPMS property & cursor ioctl). *
>>>> Enabling the cursor plane failed, e.g. via the legacy cursor
>>>> ioctl.
>>>
>>> We previously had the requirement that the primary plane must
>>> be enabled but some userspace expects that they can enable
>>> just the overlay plane without anything else.
>>>
>>> I think the chromuiumos atomictest validates that this works
>>> as well:
>>>
>>> So is DRM going forward then with the expectation that this is
>>> wrong behavior from userspace?
>>>
>>> We require at least one plane to be enabled to display a
>>> cursor, but it doesn't necessarily need to be the primary.
>>
>> It's a "pick your poison" situation:
>>
>> 1) Currently the checks are invalid (atomic_check must not
>> decide based on drm_crtc_state::active), and it's easy for legacy
>> KMS userspace to accidentally hit errors trying to enable/move
>> the cursor or switch DPMS off → on.
>>
>> 2) Accurately rejecting only atomic states where the cursor
>> plane is enabled but all other planes are off would break the
>> KMS helper code, which can only deal with the "CRTC on & primary
>> plane off is not allowed" case specifically.
>>
>> 3) This patch addresses 1) & 2) but may break existing atomic
>> userspace which wants to enable an overlay plane while disabling
>> the primary plane.
>>
>>
>> I do think in principle atomic userspace is expected to handle
>> case 3) and leave the primary plane enabled.
>
> Hi,
>
> my opinion as a userspace developer is that enabling a CRTC
> without a primary plane has traditionally not worked, so userspace
> cannot *rely* on it ever working. I think legacy KMS API does not
> even allow to express that really, or has built-in assumptions that
> it doesn't work - you can call the legacy cursor ioctls, they
> don't fail, but also the CRTC remains off. Correct me if this is
> not true.
>
> Atomic userspace OTOH is required to test the strange (and all!)
> cases like this to see if they work or not, and carry on with a
> fallback if they don't. Userspace that wants to use a CRTC without
> a primary plane likely needs other CRTC properties as well, like
> background color.
>
> I would guess that when two regressions conflict, the older
> userspace would win. Hence legacy KMS using Mutter should keep
> working, while atomic userspace is left with increased power
> consumption. Not using a hardware cursor (Mutter) is much more
> easily an end-user visible regression than slightly shorter
> battery life.
>
> Atomic test commits are allowed to fail at any time for any reason
> and something that previously worked can fail on the next frame or
> on the next kernel, as far as I understand.

All of this basically matches my understanding.


> Sounds like the helpers you refer to are inadequate for your case.
> Can't you fix the helpers in the long run and land this patch as an
> immediate fix?

I'd rather leave working on those helpers to KMS developers. :)


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

iF0EARECAB0WIQSwn681vpFFIZgJURRaga+OatuyAAUCX0UmXAAKCRBaga+Oatuy
AIeNAJoC9UgOrF+qBq08uOyjaV7Vfp+PgACfSp3nXB3un3LUZQxrvaxMAON+eIs=
=Pbd2
-----END PGP SIGNATURE-----
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Vetter]

On Tue, Aug 25, 2020 at 04:55:28PM +0200, Michel Dänzer wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> On 2020-08-24 9:43 a.m., Pekka Paalanen wrote:
> > On Sat, 22 Aug 2020 11:59:26 +0200 Michel Dänzer
> > <michel@daenzer.net> wrote:
> >> On 2020-08-21 8:07 p.m., Kazlauskas, Nicholas wrote:
> >>> On 2020-08-21 12:57 p.m., 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.
> >>>>
> >>>> The atomic helpers disable 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):
> >>>>
> >>>> * Toggling CRTC active to 1 failed if the cursor plane was
> >>>> enabled (e.g. via legacy DPMS property & cursor ioctl). *
> >>>> Enabling the cursor plane failed, e.g. via the legacy cursor
> >>>> ioctl.
> >>>
> >>> We previously had the requirement that the primary plane must
> >>> be enabled but some userspace expects that they can enable
> >>> just the overlay plane without anything else.
> >>>
> >>> I think the chromuiumos atomictest validates that this works
> >>> as well:
> >>>
> >>> So is DRM going forward then with the expectation that this is
> >>> wrong behavior from userspace?
> >>>
> >>> We require at least one plane to be enabled to display a
> >>> cursor, but it doesn't necessarily need to be the primary.
> >>
> >> It's a "pick your poison" situation:
> >>
> >> 1) Currently the checks are invalid (atomic_check must not
> >> decide based on drm_crtc_state::active), and it's easy for legacy
> >> KMS userspace to accidentally hit errors trying to enable/move
> >> the cursor or switch DPMS off → on.
> >>
> >> 2) Accurately rejecting only atomic states where the cursor
> >> plane is enabled but all other planes are off would break the
> >> KMS helper code, which can only deal with the "CRTC on & primary
> >> plane off is not allowed" case specifically.
> >>
> >> 3) This patch addresses 1) & 2) but may break existing atomic
> >> userspace which wants to enable an overlay plane while disabling
> >> the primary plane.
> >>
> >>
> >> I do think in principle atomic userspace is expected to handle
> >> case 3) and leave the primary plane enabled.
> >
> > Hi,
> >
> > my opinion as a userspace developer is that enabling a CRTC
> > without a primary plane has traditionally not worked, so userspace
> > cannot *rely* on it ever working. I think legacy KMS API does not
> > even allow to express that really, or has built-in assumptions that
> > it doesn't work - you can call the legacy cursor ioctls, they
> > don't fail, but also the CRTC remains off. Correct me if this is
> > not true.
> >
> > Atomic userspace OTOH is required to test the strange (and all!)
> > cases like this to see if they work or not, and carry on with a
> > fallback if they don't. Userspace that wants to use a CRTC without
> > a primary plane likely needs other CRTC properties as well, like
> > background color.
> >
> > I would guess that when two regressions conflict, the older
> > userspace would win. Hence legacy KMS using Mutter should keep
> > working, while atomic userspace is left with increased power
> > consumption. Not using a hardware cursor (Mutter) is much more
> > easily an end-user visible regression than slightly shorter
> > battery life.
> >
> > Atomic test commits are allowed to fail at any time for any reason
> > and something that previously worked can fail on the next frame or
> > on the next kernel, as far as I understand.
> 
> All of this basically matches my understanding.
> 
> 
> > Sounds like the helpers you refer to are inadequate for your case.
> > Can't you fix the helpers in the long run and land this patch as an
> > immediate fix?
> 
> I'd rather leave working on those helpers to KMS developers. :)

I thought the issue is the rmfb ioctl trickery, which has this assumption
fully backed in. The wiggle room in there for semantic changes is iirc
pretty much nil, it took us a pile of patches to just get to the current
state. So it's not helper code, it's core legacy ioctl code for atomic
drivers.
-Daniel

> 
> 
> - -- 
> Earthling Michel Dänzer               |               https://redhat.com
> Libre software enthusiast             |             Mesa and X developer
> -----BEGIN PGP SIGNATURE-----
> 
> iF0EARECAB0WIQSwn681vpFFIZgJURRaga+OatuyAAUCX0UmXAAKCRBaga+Oatuy
> AIeNAJoC9UgOrF+qBq08uOyjaV7Vfp+PgACfSp3nXB3un3LUZQxrvaxMAON+eIs=
> =Pbd2
> -----END PGP SIGNATURE-----
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Daniel Vetter]

On Tue, Aug 25, 2020 at 04:55:28PM +0200, Michel Dänzer wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> On 2020-08-24 9:43 a.m., Pekka Paalanen wrote:
> > On Sat, 22 Aug 2020 11:59:26 +0200 Michel Dänzer
> > <michel@daenzer.net> wrote:
> >> On 2020-08-21 8:07 p.m., Kazlauskas, Nicholas wrote:
> >>> On 2020-08-21 12:57 p.m., 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.
> >>>>
> >>>> The atomic helpers disable 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):
> >>>>
> >>>> * Toggling CRTC active to 1 failed if the cursor plane was
> >>>> enabled (e.g. via legacy DPMS property & cursor ioctl). *
> >>>> Enabling the cursor plane failed, e.g. via the legacy cursor
> >>>> ioctl.
> >>>
> >>> We previously had the requirement that the primary plane must
> >>> be enabled but some userspace expects that they can enable
> >>> just the overlay plane without anything else.
> >>>
> >>> I think the chromuiumos atomictest validates that this works
> >>> as well:
> >>>
> >>> So is DRM going forward then with the expectation that this is
> >>> wrong behavior from userspace?
> >>>
> >>> We require at least one plane to be enabled to display a
> >>> cursor, but it doesn't necessarily need to be the primary.
> >>
> >> It's a "pick your poison" situation:
> >>
> >> 1) Currently the checks are invalid (atomic_check must not
> >> decide based on drm_crtc_state::active), and it's easy for legacy
> >> KMS userspace to accidentally hit errors trying to enable/move
> >> the cursor or switch DPMS off → on.
> >>
> >> 2) Accurately rejecting only atomic states where the cursor
> >> plane is enabled but all other planes are off would break the
> >> KMS helper code, which can only deal with the "CRTC on & primary
> >> plane off is not allowed" case specifically.
> >>
> >> 3) This patch addresses 1) & 2) but may break existing atomic
> >> userspace which wants to enable an overlay plane while disabling
> >> the primary plane.
> >>
> >>
> >> I do think in principle atomic userspace is expected to handle
> >> case 3) and leave the primary plane enabled.
> >
> > Hi,
> >
> > my opinion as a userspace developer is that enabling a CRTC
> > without a primary plane has traditionally not worked, so userspace
> > cannot *rely* on it ever working. I think legacy KMS API does not
> > even allow to express that really, or has built-in assumptions that
> > it doesn't work - you can call the legacy cursor ioctls, they
> > don't fail, but also the CRTC remains off. Correct me if this is
> > not true.
> >
> > Atomic userspace OTOH is required to test the strange (and all!)
> > cases like this to see if they work or not, and carry on with a
> > fallback if they don't. Userspace that wants to use a CRTC without
> > a primary plane likely needs other CRTC properties as well, like
> > background color.
> >
> > I would guess that when two regressions conflict, the older
> > userspace would win. Hence legacy KMS using Mutter should keep
> > working, while atomic userspace is left with increased power
> > consumption. Not using a hardware cursor (Mutter) is much more
> > easily an end-user visible regression than slightly shorter
> > battery life.
> >
> > Atomic test commits are allowed to fail at any time for any reason
> > and something that previously worked can fail on the next frame or
> > on the next kernel, as far as I understand.
> 
> All of this basically matches my understanding.
> 
> 
> > Sounds like the helpers you refer to are inadequate for your case.
> > Can't you fix the helpers in the long run and land this patch as an
> > immediate fix?
> 
> I'd rather leave working on those helpers to KMS developers. :)

I thought the issue is the rmfb ioctl trickery, which has this assumption
fully backed in. The wiggle room in there for semantic changes is iirc
pretty much nil, it took us a pile of patches to just get to the current
state. So it's not helper code, it's core legacy ioctl code for atomic
drivers.
-Daniel

> 
> 
> - -- 
> Earthling Michel Dänzer               |               https://redhat.com
> Libre software enthusiast             |             Mesa and X developer
> -----BEGIN PGP SIGNATURE-----
> 
> iF0EARECAB0WIQSwn681vpFFIZgJURRaga+OatuyAAUCX0UmXAAKCRBaga+Oatuy
> AIeNAJoC9UgOrF+qBq08uOyjaV7Vfp+PgACfSp3nXB3un3LUZQxrvaxMAON+eIs=
> =Pbd2
> -----END PGP SIGNATURE-----
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

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

[Michel Dänzer]

On 2020-09-01 9:57 a.m., Daniel Vetter wrote:
> On Tue, Aug 25, 2020 at 04:55:28PM +0200, Michel Dänzer wrote:
>> On 2020-08-24 9:43 a.m., Pekka Paalanen wrote:
>> 
>>> Sounds like the helpers you refer to are inadequate for your case.
>>> Can't you fix the helpers in the long run and land this patch as an
>>> immediate fix?
>> 
>> I'd rather leave working on those helpers to KMS developers. :)
> 
> I thought the issue is the rmfb ioctl trickery, which has this assumption
> fully backed in. The wiggle room in there for semantic changes is iirc
> pretty much nil, it took us a pile of patches to just get to the current
> state. So it's not helper code, it's core legacy ioctl code for atomic
> drivers.

My bad. Should I respin with an amended commit log?


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

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

[Michel Dänzer]

On 2020-09-01 9:57 a.m., Daniel Vetter wrote:
> On Tue, Aug 25, 2020 at 04:55:28PM +0200, Michel Dänzer wrote:
>> On 2020-08-24 9:43 a.m., Pekka Paalanen wrote:
>> 
>>> Sounds like the helpers you refer to are inadequate for your case.
>>> Can't you fix the helpers in the long run and land this patch as an
>>> immediate fix?
>> 
>> I'd rather leave working on those helpers to KMS developers. :)
> 
> I thought the issue is the rmfb ioctl trickery, which has this assumption
> fully backed in. The wiggle room in there for semantic changes is iirc
> pretty much nil, it took us a pile of patches to just get to the current
> state. So it's not helper code, it's core legacy ioctl code for atomic
> drivers.

My bad. Should I respin with an amended commit log?


-- 
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] drm/amdgpu/dc: Require primary plane to be enabled whenever the CRTC is

[Daniel Vetter]

On Tue, Sep 01, 2020 at 10:56:42AM +0200, Michel Dänzer wrote:
> On 2020-09-01 9:57 a.m., Daniel Vetter wrote:
> > On Tue, Aug 25, 2020 at 04:55:28PM +0200, Michel Dänzer wrote:
> >> On 2020-08-24 9:43 a.m., Pekka Paalanen wrote:
> >> 
> >>> Sounds like the helpers you refer to are inadequate for your case.
> >>> Can't you fix the helpers in the long run and land this patch as an
> >>> immediate fix?
> >> 
> >> I'd rather leave working on those helpers to KMS developers. :)
> > 
> > I thought the issue is the rmfb ioctl trickery, which has this assumption
> > fully backed in. The wiggle room in there for semantic changes is iirc
> > pretty much nil, it took us a pile of patches to just get to the current
> > state. So it's not helper code, it's core legacy ioctl code for atomic
> > drivers.
> 
> My bad. Should I respin with an amended commit log?

Yeah if it's not yet merged then I think would be good to clarify that
this assumption is baked into rmfb, and that together with the assumption
the the legacy cursor ioctls just work we have fallout.

If (and hey I've been on vacations for 2 weeks, so good chances I remebers
this all wrong) this is indeed what we discussed a few weeks ago.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Daniel Vetter]

On Tue, Sep 01, 2020 at 10:56:42AM +0200, Michel Dänzer wrote:
> On 2020-09-01 9:57 a.m., Daniel Vetter wrote:
> > On Tue, Aug 25, 2020 at 04:55:28PM +0200, Michel Dänzer wrote:
> >> On 2020-08-24 9:43 a.m., Pekka Paalanen wrote:
> >> 
> >>> Sounds like the helpers you refer to are inadequate for your case.
> >>> Can't you fix the helpers in the long run and land this patch as an
> >>> immediate fix?
> >> 
> >> I'd rather leave working on those helpers to KMS developers. :)
> > 
> > I thought the issue is the rmfb ioctl trickery, which has this assumption
> > fully backed in. The wiggle room in there for semantic changes is iirc
> > pretty much nil, it took us a pile of patches to just get to the current
> > state. So it's not helper code, it's core legacy ioctl code for atomic
> > drivers.
> 
> My bad. Should I respin with an amended commit log?

Yeah if it's not yet merged then I think would be good to clarify that
this assumption is baked into rmfb, and that together with the assumption
the the legacy cursor ioctls just work we have fallout.

If (and hey I've been on vacations for 2 weeks, so good chances I remebers
this all wrong) this is indeed what we discussed a few weeks ago.

Cheers, Daniel
-- 
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

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

[Kazlauskas, Nicholas]

On 2020-08-22 5:59 a.m., Michel Dänzer wrote:
> On 2020-08-21 8:07 p.m., Kazlauskas, Nicholas wrote:
>> On 2020-08-21 12:57 p.m., 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.
>>>
>>> The atomic helpers disable 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):
>>>
>>> * Toggling CRTC active to 1 failed if the cursor plane was enabled
>>>     (e.g. via legacy DPMS property & cursor ioctl).
>>> * Enabling the cursor plane failed, e.g. via the legacy cursor ioctl.
>>
>> We previously had the requirement that the primary plane must be enabled
>> but some userspace expects that they can enable just the overlay plane
>> without anything else.
>>
>> I think the chromuiumos atomictest validates that this works as well:
>>
>> So is DRM going forward then with the expectation that this is wrong
>> behavior from userspace?
>>
>> We require at least one plane to be enabled to display a cursor, but it
>> doesn't necessarily need to be the primary.
> 
> It's a "pick your poison" situation:
> 
> 1) Currently the checks are invalid (atomic_check must not decide based
> on drm_crtc_state::active), and it's easy for legacy KMS userspace to
> accidentally hit errors trying to enable/move the cursor or switch DPMS
> off → on.
> 
> 2) Accurately rejecting only atomic states where the cursor plane is
> enabled but all other planes are off would break the KMS helper code,
> which can only deal with the "CRTC on & primary plane off is not
> allowed" case specifically.
> 
> 3) This patch addresses 1) & 2) but may break existing atomic userspace
> which wants to enable an overlay plane while disabling the primary plane.
> 
> 
> I do think in principle atomic userspace is expected to handle case 3)
> and leave the primary plane enabled. However, this is not ideal from an
> energy consumption PoV. Therefore, here's another idea for a possible
> way out of this quagmire:
> 
> amdgpu_dm does not reject any atomic states based on which planes are
> enabled in it. If the cursor plane is enabled but all other planes are
> off, amdgpu_dm internally either:
> 
> a) Enables an overlay plane and makes it invisible, e.g. by assigning a
> minimum size FB with alpha = 0.
> 
> b) Enables the primary plane and assigns a minimum size FB (scaled up to
> the required size) containing all black, possibly using compression.
> (Trying to minimize the memory bandwidth)
> 
> 
> Does either of these seem feasible? If both do, which one would be
> preferable?
> 
> 

It's really the same solution since DCN doesn't make a distinction 
between primary or overlay planes in hardware. DCE doesn't have overlay 
planes enabled so this is not relevant there.

The old behavior (pre 5.1?) was to silently accept the commit even 
though the screen would be completely black instead of outright 
rejecting the commit.

I almost wonder if that makes more sense in the short term here since 
the only "userspace" affected here is IGT. We'll fail the CRC checks, 
but no userspace actually tries to actively use a cursor with no primary 
plane enabled from my understanding.

In the long term I think we can work on getting cursor actually on the 
screen in this case, though I can't say I really like having to reserve 
some small buffer (eg. 16x16) for allowing lightup on this corner case.

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

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

[Kazlauskas, Nicholas]

On 2020-08-22 5:59 a.m., Michel Dänzer wrote:
> On 2020-08-21 8:07 p.m., Kazlauskas, Nicholas wrote:
>> On 2020-08-21 12:57 p.m., 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.
>>>
>>> The atomic helpers disable 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):
>>>
>>> * Toggling CRTC active to 1 failed if the cursor plane was enabled
>>>     (e.g. via legacy DPMS property & cursor ioctl).
>>> * Enabling the cursor plane failed, e.g. via the legacy cursor ioctl.
>>
>> We previously had the requirement that the primary plane must be enabled
>> but some userspace expects that they can enable just the overlay plane
>> without anything else.
>>
>> I think the chromuiumos atomictest validates that this works as well:
>>
>> So is DRM going forward then with the expectation that this is wrong
>> behavior from userspace?
>>
>> We require at least one plane to be enabled to display a cursor, but it
>> doesn't necessarily need to be the primary.
> 
> It's a "pick your poison" situation:
> 
> 1) Currently the checks are invalid (atomic_check must not decide based
> on drm_crtc_state::active), and it's easy for legacy KMS userspace to
> accidentally hit errors trying to enable/move the cursor or switch DPMS
> off → on.
> 
> 2) Accurately rejecting only atomic states where the cursor plane is
> enabled but all other planes are off would break the KMS helper code,
> which can only deal with the "CRTC on & primary plane off is not
> allowed" case specifically.
> 
> 3) This patch addresses 1) & 2) but may break existing atomic userspace
> which wants to enable an overlay plane while disabling the primary plane.
> 
> 
> I do think in principle atomic userspace is expected to handle case 3)
> and leave the primary plane enabled. However, this is not ideal from an
> energy consumption PoV. Therefore, here's another idea for a possible
> way out of this quagmire:
> 
> amdgpu_dm does not reject any atomic states based on which planes are
> enabled in it. If the cursor plane is enabled but all other planes are
> off, amdgpu_dm internally either:
> 
> a) Enables an overlay plane and makes it invisible, e.g. by assigning a
> minimum size FB with alpha = 0.
> 
> b) Enables the primary plane and assigns a minimum size FB (scaled up to
> the required size) containing all black, possibly using compression.
> (Trying to minimize the memory bandwidth)
> 
> 
> Does either of these seem feasible? If both do, which one would be
> preferable?
> 
> 

It's really the same solution since DCN doesn't make a distinction 
between primary or overlay planes in hardware. DCE doesn't have overlay 
planes enabled so this is not relevant there.

The old behavior (pre 5.1?) was to silently accept the commit even 
though the screen would be completely black instead of outright 
rejecting the commit.

I almost wonder if that makes more sense in the short term here since 
the only "userspace" affected here is IGT. We'll fail the CRC checks, 
but no userspace actually tries to actively use a cursor with no primary 
plane enabled from my understanding.

In the long term I think we can work on getting cursor actually on the 
screen in this case, though I can't say I really like having to reserve 
some small buffer (eg. 16x16) for allowing lightup on this corner case.

Regards,
Nicholas Kazlauskas
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Pekka Paalanen]

[-- Attachment #1.1: Type: text/plain, Size: 4945 bytes --]

On Tue, 25 Aug 2020 12:58:19 -0400
"Kazlauskas, Nicholas" <nicholas.kazlauskas@amd.com> wrote:

> On 2020-08-22 5:59 a.m., Michel Dänzer wrote:
> > On 2020-08-21 8:07 p.m., Kazlauskas, Nicholas wrote:  
> >> On 2020-08-21 12:57 p.m., 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.
> >>>
> >>> The atomic helpers disable 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):
> >>>
> >>> * Toggling CRTC active to 1 failed if the cursor plane was enabled
> >>>     (e.g. via legacy DPMS property & cursor ioctl).
> >>> * Enabling the cursor plane failed, e.g. via the legacy cursor ioctl.  
> >>
> >> We previously had the requirement that the primary plane must be enabled
> >> but some userspace expects that they can enable just the overlay plane
> >> without anything else.
> >>
> >> I think the chromuiumos atomictest validates that this works as well:
> >>
> >> So is DRM going forward then with the expectation that this is wrong
> >> behavior from userspace?
> >>
> >> We require at least one plane to be enabled to display a cursor, but it
> >> doesn't necessarily need to be the primary.  
> > 
> > It's a "pick your poison" situation:
> > 
> > 1) Currently the checks are invalid (atomic_check must not decide based
> > on drm_crtc_state::active), and it's easy for legacy KMS userspace to
> > accidentally hit errors trying to enable/move the cursor or switch DPMS
> > off → on.
> > 
> > 2) Accurately rejecting only atomic states where the cursor plane is
> > enabled but all other planes are off would break the KMS helper code,
> > which can only deal with the "CRTC on & primary plane off is not
> > allowed" case specifically.
> > 
> > 3) This patch addresses 1) & 2) but may break existing atomic userspace
> > which wants to enable an overlay plane while disabling the primary plane.
> > 
> > 
> > I do think in principle atomic userspace is expected to handle case 3)
> > and leave the primary plane enabled. However, this is not ideal from an
> > energy consumption PoV. Therefore, here's another idea for a possible
> > way out of this quagmire:
> > 
> > amdgpu_dm does not reject any atomic states based on which planes are
> > enabled in it. If the cursor plane is enabled but all other planes are
> > off, amdgpu_dm internally either:
> > 
> > a) Enables an overlay plane and makes it invisible, e.g. by assigning a
> > minimum size FB with alpha = 0.
> > 
> > b) Enables the primary plane and assigns a minimum size FB (scaled up to
> > the required size) containing all black, possibly using compression.
> > (Trying to minimize the memory bandwidth)
> > 
> > 
> > Does either of these seem feasible? If both do, which one would be
> > preferable?
> > 
> >   
> 
> It's really the same solution since DCN doesn't make a distinction 
> between primary or overlay planes in hardware. DCE doesn't have overlay 
> planes enabled so this is not relevant there.
> 
> The old behavior (pre 5.1?) was to silently accept the commit even 
> though the screen would be completely black instead of outright 
> rejecting the commit.
> 
> I almost wonder if that makes more sense in the short term here since 
> the only "userspace" affected here is IGT. We'll fail the CRC checks, 
> but no userspace actually tries to actively use a cursor with no primary 
> plane enabled from my understanding.

Hi,

I believe that there exists userspace that will *accidentally* attempt
to update the cursor plane while primary plane or whole CRTC is off.
Some versions of Mutter might do that on racy conditions, I suspect.
These are legacy KMS users, not atomic KMS.

However, I do not believe there exists any userspace that would
actually expect the display to show the cursor plane alone without a
primary plane. Therefore I'd be ok with legacy cursor ioctls silently
succeeding. Atomic commits not. So the difference has to be in the
translation from legacy UAPI to kernel internal atomic interface.

> In the long term I think we can work on getting cursor actually on the 
> screen in this case, though I can't say I really like having to reserve 
> some small buffer (eg. 16x16) for allowing lightup on this corner case.

Why would you bother implementing that?

Is there really an IGT test that unconditionally demands cursor plane
to be usable without any other planes?


Thanks,
pq

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

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

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

[Pekka Paalanen]

[-- Attachment #1.1: Type: text/plain, Size: 4945 bytes --]

On Tue, 25 Aug 2020 12:58:19 -0400
"Kazlauskas, Nicholas" <nicholas.kazlauskas@amd.com> wrote:

> On 2020-08-22 5:59 a.m., Michel Dänzer wrote:
> > On 2020-08-21 8:07 p.m., Kazlauskas, Nicholas wrote:  
> >> On 2020-08-21 12:57 p.m., 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.
> >>>
> >>> The atomic helpers disable 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):
> >>>
> >>> * Toggling CRTC active to 1 failed if the cursor plane was enabled
> >>>     (e.g. via legacy DPMS property & cursor ioctl).
> >>> * Enabling the cursor plane failed, e.g. via the legacy cursor ioctl.  
> >>
> >> We previously had the requirement that the primary plane must be enabled
> >> but some userspace expects that they can enable just the overlay plane
> >> without anything else.
> >>
> >> I think the chromuiumos atomictest validates that this works as well:
> >>
> >> So is DRM going forward then with the expectation that this is wrong
> >> behavior from userspace?
> >>
> >> We require at least one plane to be enabled to display a cursor, but it
> >> doesn't necessarily need to be the primary.  
> > 
> > It's a "pick your poison" situation:
> > 
> > 1) Currently the checks are invalid (atomic_check must not decide based
> > on drm_crtc_state::active), and it's easy for legacy KMS userspace to
> > accidentally hit errors trying to enable/move the cursor or switch DPMS
> > off → on.
> > 
> > 2) Accurately rejecting only atomic states where the cursor plane is
> > enabled but all other planes are off would break the KMS helper code,
> > which can only deal with the "CRTC on & primary plane off is not
> > allowed" case specifically.
> > 
> > 3) This patch addresses 1) & 2) but may break existing atomic userspace
> > which wants to enable an overlay plane while disabling the primary plane.
> > 
> > 
> > I do think in principle atomic userspace is expected to handle case 3)
> > and leave the primary plane enabled. However, this is not ideal from an
> > energy consumption PoV. Therefore, here's another idea for a possible
> > way out of this quagmire:
> > 
> > amdgpu_dm does not reject any atomic states based on which planes are
> > enabled in it. If the cursor plane is enabled but all other planes are
> > off, amdgpu_dm internally either:
> > 
> > a) Enables an overlay plane and makes it invisible, e.g. by assigning a
> > minimum size FB with alpha = 0.
> > 
> > b) Enables the primary plane and assigns a minimum size FB (scaled up to
> > the required size) containing all black, possibly using compression.
> > (Trying to minimize the memory bandwidth)
> > 
> > 
> > Does either of these seem feasible? If both do, which one would be
> > preferable?
> > 
> >   
> 
> It's really the same solution since DCN doesn't make a distinction 
> between primary or overlay planes in hardware. DCE doesn't have overlay 
> planes enabled so this is not relevant there.
> 
> The old behavior (pre 5.1?) was to silently accept the commit even 
> though the screen would be completely black instead of outright 
> rejecting the commit.
> 
> I almost wonder if that makes more sense in the short term here since 
> the only "userspace" affected here is IGT. We'll fail the CRC checks, 
> but no userspace actually tries to actively use a cursor with no primary 
> plane enabled from my understanding.

Hi,

I believe that there exists userspace that will *accidentally* attempt
to update the cursor plane while primary plane or whole CRTC is off.
Some versions of Mutter might do that on racy conditions, I suspect.
These are legacy KMS users, not atomic KMS.

However, I do not believe there exists any userspace that would
actually expect the display to show the cursor plane alone without a
primary plane. Therefore I'd be ok with legacy cursor ioctls silently
succeeding. Atomic commits not. So the difference has to be in the
translation from legacy UAPI to kernel internal atomic interface.

> In the long term I think we can work on getting cursor actually on the 
> screen in this case, though I can't say I really like having to reserve 
> some small buffer (eg. 16x16) for allowing lightup on this corner case.

Why would you bother implementing that?

Is there really an IGT test that unconditionally demands cursor plane
to be usable without any other planes?


Thanks,
pq

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Michel Dänzer]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 2020-08-26 10:24 a.m., Pekka Paalanen wrote:
> On Tue, 25 Aug 2020 12:58:19 -0400 "Kazlauskas, Nicholas"
> <nicholas.kazlauskas@amd.com> wrote:
>
>> On 2020-08-22 5:59 a.m., Michel Dänzer wrote:
>>> On 2020-08-21 8:07 p.m., Kazlauskas, Nicholas wrote:
>>>> On 2020-08-21 12:57 p.m., 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.
>>>>>
>>>>> The atomic helpers disable 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):
>>>>>
>>>>> * Toggling CRTC active to 1 failed if the cursor plane was
>>>>> enabled (e.g. via legacy DPMS property & cursor ioctl). *
>>>>> Enabling the cursor plane failed, e.g. via the legacy
>>>>> cursor ioctl.
>>>>
>>>> We previously had the requirement that the primary plane must
>>>> be enabled but some userspace expects that they can enable
>>>> just the overlay plane without anything else.
>>>>
>>>> I think the chromuiumos atomictest validates that this works
>>>> as well:
>>>>
>>>> So is DRM going forward then with the expectation that this
>>>> is wrong behavior from userspace?
>>>>
>>>> We require at least one plane to be enabled to display a
>>>> cursor, but it doesn't necessarily need to be the primary.
>>>
>>> It's a "pick your poison" situation:
>>>
>>> 1) Currently the checks are invalid (atomic_check must not
>>> decide based on drm_crtc_state::active), and it's easy for
>>> legacy KMS userspace to accidentally hit errors trying to
>>> enable/move the cursor or switch DPMS off → on.
>>>
>>> 2) Accurately rejecting only atomic states where the cursor
>>> plane is enabled but all other planes are off would break the
>>> KMS helper code, which can only deal with the "CRTC on &
>>> primary plane off is not allowed" case specifically.
>>>
>>> 3) This patch addresses 1) & 2) but may break existing atomic
>>> userspace which wants to enable an overlay plane while
>>> disabling the primary plane.
>>>
>>>
>>> I do think in principle atomic userspace is expected to handle
>>> case 3) and leave the primary plane enabled. However, this is
>>> not ideal from an energy consumption PoV. Therefore, here's
>>> another idea for a possible way out of this quagmire:
>>>
>>> amdgpu_dm does not reject any atomic states based on which
>>> planes are enabled in it. If the cursor plane is enabled but
>>> all other planes are off, amdgpu_dm internally either:
>>>
>>> a) Enables an overlay plane and makes it invisible, e.g. by
>>> assigning a minimum size FB with alpha = 0.
>>>
>>> b) Enables the primary plane and assigns a minimum size FB
>>> (scaled up to the required size) containing all black, possibly
>>> using compression. (Trying to minimize the memory bandwidth)
>>>
>>>
>>> Does either of these seem feasible? If both do, which one would
>>> be preferable?
>>
>> It's really the same solution since DCN doesn't make a
>> distinction between primary or overlay planes in hardware. DCE
>> doesn't have overlay planes enabled so this is not relevant
>> there.
>>
>> The old behavior (pre 5.1?) was to silently accept the commit
>> even though the screen would be completely black instead of
>> outright rejecting the commit.
>>
>> I almost wonder if that makes more sense in the short term here
>> since the only "userspace" affected here is IGT. We'll fail the
>> CRC checks, but no userspace actually tries to actively use a
>> cursor with no primary plane enabled from my understanding.
>
> Hi,
>
> I believe that there exists userspace that will *accidentally*
> attempt to update the cursor plane while primary plane or whole
> CRTC is off. Some versions of Mutter might do that on racy
> conditions, I suspect. These are legacy KMS users, not atomic KMS.
>
> However, I do not believe there exists any userspace that would
> actually expect the display to show the cursor plane alone without
> a primary plane. Therefore I'd be ok with legacy cursor ioctls
> silently succeeding. Atomic commits not. So the difference has to
> be in the translation from legacy UAPI to kernel internal atomic
> interface.

This would be my case 2) above, so still requires figuring out how the
atomic KMS helpers should deal with corner cases such as:

* CRTC on, primary plane off, overlay & cursor planes on
* RmFB of FB assigned to overlay plane → need to disable overlay
plane, but driver rejects that (because it would leave only the cursor
plane on)


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

iF0EARECAB0WIQSwn681vpFFIZgJURRaga+OatuyAAUCX0YkIwAKCRBaga+Oatuy
AMWrAJ9IDMIj2eGXERYDZfwraOCHebwE9QCfR/nOaoLGfIOii6r1H4JLn9sraFI=
=D/LX
-----END PGP SIGNATURE-----
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Michel Dänzer]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 2020-08-26 10:24 a.m., Pekka Paalanen wrote:
> On Tue, 25 Aug 2020 12:58:19 -0400 "Kazlauskas, Nicholas"
> <nicholas.kazlauskas@amd.com> wrote:
>
>> On 2020-08-22 5:59 a.m., Michel Dänzer wrote:
>>> On 2020-08-21 8:07 p.m., Kazlauskas, Nicholas wrote:
>>>> On 2020-08-21 12:57 p.m., 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.
>>>>>
>>>>> The atomic helpers disable 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):
>>>>>
>>>>> * Toggling CRTC active to 1 failed if the cursor plane was
>>>>> enabled (e.g. via legacy DPMS property & cursor ioctl). *
>>>>> Enabling the cursor plane failed, e.g. via the legacy
>>>>> cursor ioctl.
>>>>
>>>> We previously had the requirement that the primary plane must
>>>> be enabled but some userspace expects that they can enable
>>>> just the overlay plane without anything else.
>>>>
>>>> I think the chromuiumos atomictest validates that this works
>>>> as well:
>>>>
>>>> So is DRM going forward then with the expectation that this
>>>> is wrong behavior from userspace?
>>>>
>>>> We require at least one plane to be enabled to display a
>>>> cursor, but it doesn't necessarily need to be the primary.
>>>
>>> It's a "pick your poison" situation:
>>>
>>> 1) Currently the checks are invalid (atomic_check must not
>>> decide based on drm_crtc_state::active), and it's easy for
>>> legacy KMS userspace to accidentally hit errors trying to
>>> enable/move the cursor or switch DPMS off → on.
>>>
>>> 2) Accurately rejecting only atomic states where the cursor
>>> plane is enabled but all other planes are off would break the
>>> KMS helper code, which can only deal with the "CRTC on &
>>> primary plane off is not allowed" case specifically.
>>>
>>> 3) This patch addresses 1) & 2) but may break existing atomic
>>> userspace which wants to enable an overlay plane while
>>> disabling the primary plane.
>>>
>>>
>>> I do think in principle atomic userspace is expected to handle
>>> case 3) and leave the primary plane enabled. However, this is
>>> not ideal from an energy consumption PoV. Therefore, here's
>>> another idea for a possible way out of this quagmire:
>>>
>>> amdgpu_dm does not reject any atomic states based on which
>>> planes are enabled in it. If the cursor plane is enabled but
>>> all other planes are off, amdgpu_dm internally either:
>>>
>>> a) Enables an overlay plane and makes it invisible, e.g. by
>>> assigning a minimum size FB with alpha = 0.
>>>
>>> b) Enables the primary plane and assigns a minimum size FB
>>> (scaled up to the required size) containing all black, possibly
>>> using compression. (Trying to minimize the memory bandwidth)
>>>
>>>
>>> Does either of these seem feasible? If both do, which one would
>>> be preferable?
>>
>> It's really the same solution since DCN doesn't make a
>> distinction between primary or overlay planes in hardware. DCE
>> doesn't have overlay planes enabled so this is not relevant
>> there.
>>
>> The old behavior (pre 5.1?) was to silently accept the commit
>> even though the screen would be completely black instead of
>> outright rejecting the commit.
>>
>> I almost wonder if that makes more sense in the short term here
>> since the only "userspace" affected here is IGT. We'll fail the
>> CRC checks, but no userspace actually tries to actively use a
>> cursor with no primary plane enabled from my understanding.
>
> Hi,
>
> I believe that there exists userspace that will *accidentally*
> attempt to update the cursor plane while primary plane or whole
> CRTC is off. Some versions of Mutter might do that on racy
> conditions, I suspect. These are legacy KMS users, not atomic KMS.
>
> However, I do not believe there exists any userspace that would
> actually expect the display to show the cursor plane alone without
> a primary plane. Therefore I'd be ok with legacy cursor ioctls
> silently succeeding. Atomic commits not. So the difference has to
> be in the translation from legacy UAPI to kernel internal atomic
> interface.

This would be my case 2) above, so still requires figuring out how the
atomic KMS helpers should deal with corner cases such as:

* CRTC on, primary plane off, overlay & cursor planes on
* RmFB of FB assigned to overlay plane → need to disable overlay
plane, but driver rejects that (because it would leave only the cursor
plane on)


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

iF0EARECAB0WIQSwn681vpFFIZgJURRaga+OatuyAAUCX0YkIwAKCRBaga+Oatuy
AMWrAJ9IDMIj2eGXERYDZfwraOCHebwE9QCfR/nOaoLGfIOii6r1H4JLn9sraFI=
=D/LX
-----END PGP SIGNATURE-----
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Vetter]

On Wed, Aug 26, 2020 at 11:24:23AM +0300, Pekka Paalanen wrote:
> On Tue, 25 Aug 2020 12:58:19 -0400
> "Kazlauskas, Nicholas" <nicholas.kazlauskas@amd.com> wrote:
> 
> > On 2020-08-22 5:59 a.m., Michel Dänzer wrote:
> > > On 2020-08-21 8:07 p.m., Kazlauskas, Nicholas wrote:  
> > >> On 2020-08-21 12:57 p.m., 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.
> > >>>
> > >>> The atomic helpers disable 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):
> > >>>
> > >>> * Toggling CRTC active to 1 failed if the cursor plane was enabled
> > >>>     (e.g. via legacy DPMS property & cursor ioctl).
> > >>> * Enabling the cursor plane failed, e.g. via the legacy cursor ioctl.  
> > >>
> > >> We previously had the requirement that the primary plane must be enabled
> > >> but some userspace expects that they can enable just the overlay plane
> > >> without anything else.
> > >>
> > >> I think the chromuiumos atomictest validates that this works as well:
> > >>
> > >> So is DRM going forward then with the expectation that this is wrong
> > >> behavior from userspace?
> > >>
> > >> We require at least one plane to be enabled to display a cursor, but it
> > >> doesn't necessarily need to be the primary.  
> > > 
> > > It's a "pick your poison" situation:
> > > 
> > > 1) Currently the checks are invalid (atomic_check must not decide based
> > > on drm_crtc_state::active), and it's easy for legacy KMS userspace to
> > > accidentally hit errors trying to enable/move the cursor or switch DPMS
> > > off → on.
> > > 
> > > 2) Accurately rejecting only atomic states where the cursor plane is
> > > enabled but all other planes are off would break the KMS helper code,
> > > which can only deal with the "CRTC on & primary plane off is not
> > > allowed" case specifically.
> > > 
> > > 3) This patch addresses 1) & 2) but may break existing atomic userspace
> > > which wants to enable an overlay plane while disabling the primary plane.
> > > 
> > > 
> > > I do think in principle atomic userspace is expected to handle case 3)
> > > and leave the primary plane enabled. However, this is not ideal from an
> > > energy consumption PoV. Therefore, here's another idea for a possible
> > > way out of this quagmire:
> > > 
> > > amdgpu_dm does not reject any atomic states based on which planes are
> > > enabled in it. If the cursor plane is enabled but all other planes are
> > > off, amdgpu_dm internally either:
> > > 
> > > a) Enables an overlay plane and makes it invisible, e.g. by assigning a
> > > minimum size FB with alpha = 0.
> > > 
> > > b) Enables the primary plane and assigns a minimum size FB (scaled up to
> > > the required size) containing all black, possibly using compression.
> > > (Trying to minimize the memory bandwidth)
> > > 
> > > 
> > > Does either of these seem feasible? If both do, which one would be
> > > preferable?
> > > 
> > >   
> > 
> > It's really the same solution since DCN doesn't make a distinction 
> > between primary or overlay planes in hardware. DCE doesn't have overlay 
> > planes enabled so this is not relevant there.
> > 
> > The old behavior (pre 5.1?) was to silently accept the commit even 
> > though the screen would be completely black instead of outright 
> > rejecting the commit.
> > 
> > I almost wonder if that makes more sense in the short term here since 
> > the only "userspace" affected here is IGT. We'll fail the CRC checks, 
> > but no userspace actually tries to actively use a cursor with no primary 
> > plane enabled from my understanding.
> 
> Hi,
> 
> I believe that there exists userspace that will *accidentally* attempt
> to update the cursor plane while primary plane or whole CRTC is off.
> Some versions of Mutter might do that on racy conditions, I suspect.
> These are legacy KMS users, not atomic KMS.
> 
> However, I do not believe there exists any userspace that would
> actually expect the display to show the cursor plane alone without a
> primary plane. Therefore I'd be ok with legacy cursor ioctls silently
> succeeding. Atomic commits not. So the difference has to be in the
> translation from legacy UAPI to kernel internal atomic interface.
> 
> > In the long term I think we can work on getting cursor actually on the 
> > screen in this case, though I can't say I really like having to reserve 
> > some small buffer (eg. 16x16) for allowing lightup on this corner case.
> 
> Why would you bother implementing that?
> 
> Is there really an IGT test that unconditionally demands cursor plane
> to be usable without any other planes?

The cursor plane isn't anything else than any other plane, aside from the
legacy uapi implication that it's used for the legacy cursor ioctls.

Which means the cursor plane could actually be a full-featured plane, and
it's totally legit to use just that without anything else enabled.

So yeah if you allow that, it better show something :-)

Personally I'd lean towards merging this patch to close the gap (oldest
regressions wins and all that) and then implement the black plane hack on
top.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Daniel Vetter]

On Wed, Aug 26, 2020 at 11:24:23AM +0300, Pekka Paalanen wrote:
> On Tue, 25 Aug 2020 12:58:19 -0400
> "Kazlauskas, Nicholas" <nicholas.kazlauskas@amd.com> wrote:
> 
> > On 2020-08-22 5:59 a.m., Michel Dänzer wrote:
> > > On 2020-08-21 8:07 p.m., Kazlauskas, Nicholas wrote:  
> > >> On 2020-08-21 12:57 p.m., 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.
> > >>>
> > >>> The atomic helpers disable 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):
> > >>>
> > >>> * Toggling CRTC active to 1 failed if the cursor plane was enabled
> > >>>     (e.g. via legacy DPMS property & cursor ioctl).
> > >>> * Enabling the cursor plane failed, e.g. via the legacy cursor ioctl.  
> > >>
> > >> We previously had the requirement that the primary plane must be enabled
> > >> but some userspace expects that they can enable just the overlay plane
> > >> without anything else.
> > >>
> > >> I think the chromuiumos atomictest validates that this works as well:
> > >>
> > >> So is DRM going forward then with the expectation that this is wrong
> > >> behavior from userspace?
> > >>
> > >> We require at least one plane to be enabled to display a cursor, but it
> > >> doesn't necessarily need to be the primary.  
> > > 
> > > It's a "pick your poison" situation:
> > > 
> > > 1) Currently the checks are invalid (atomic_check must not decide based
> > > on drm_crtc_state::active), and it's easy for legacy KMS userspace to
> > > accidentally hit errors trying to enable/move the cursor or switch DPMS
> > > off → on.
> > > 
> > > 2) Accurately rejecting only atomic states where the cursor plane is
> > > enabled but all other planes are off would break the KMS helper code,
> > > which can only deal with the "CRTC on & primary plane off is not
> > > allowed" case specifically.
> > > 
> > > 3) This patch addresses 1) & 2) but may break existing atomic userspace
> > > which wants to enable an overlay plane while disabling the primary plane.
> > > 
> > > 
> > > I do think in principle atomic userspace is expected to handle case 3)
> > > and leave the primary plane enabled. However, this is not ideal from an
> > > energy consumption PoV. Therefore, here's another idea for a possible
> > > way out of this quagmire:
> > > 
> > > amdgpu_dm does not reject any atomic states based on which planes are
> > > enabled in it. If the cursor plane is enabled but all other planes are
> > > off, amdgpu_dm internally either:
> > > 
> > > a) Enables an overlay plane and makes it invisible, e.g. by assigning a
> > > minimum size FB with alpha = 0.
> > > 
> > > b) Enables the primary plane and assigns a minimum size FB (scaled up to
> > > the required size) containing all black, possibly using compression.
> > > (Trying to minimize the memory bandwidth)
> > > 
> > > 
> > > Does either of these seem feasible? If both do, which one would be
> > > preferable?
> > > 
> > >   
> > 
> > It's really the same solution since DCN doesn't make a distinction 
> > between primary or overlay planes in hardware. DCE doesn't have overlay 
> > planes enabled so this is not relevant there.
> > 
> > The old behavior (pre 5.1?) was to silently accept the commit even 
> > though the screen would be completely black instead of outright 
> > rejecting the commit.
> > 
> > I almost wonder if that makes more sense in the short term here since 
> > the only "userspace" affected here is IGT. We'll fail the CRC checks, 
> > but no userspace actually tries to actively use a cursor with no primary 
> > plane enabled from my understanding.
> 
> Hi,
> 
> I believe that there exists userspace that will *accidentally* attempt
> to update the cursor plane while primary plane or whole CRTC is off.
> Some versions of Mutter might do that on racy conditions, I suspect.
> These are legacy KMS users, not atomic KMS.
> 
> However, I do not believe there exists any userspace that would
> actually expect the display to show the cursor plane alone without a
> primary plane. Therefore I'd be ok with legacy cursor ioctls silently
> succeeding. Atomic commits not. So the difference has to be in the
> translation from legacy UAPI to kernel internal atomic interface.
> 
> > In the long term I think we can work on getting cursor actually on the 
> > screen in this case, though I can't say I really like having to reserve 
> > some small buffer (eg. 16x16) for allowing lightup on this corner case.
> 
> Why would you bother implementing that?
> 
> Is there really an IGT test that unconditionally demands cursor plane
> to be usable without any other planes?

The cursor plane isn't anything else than any other plane, aside from the
legacy uapi implication that it's used for the legacy cursor ioctls.

Which means the cursor plane could actually be a full-featured plane, and
it's totally legit to use just that without anything else enabled.

So yeah if you allow that, it better show something :-)

Personally I'd lean towards merging this patch to close the gap (oldest
regressions wins and all that) and then implement the black plane hack on
top.
-Daniel
-- 
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

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

[Harry Wentland]

On 2020-09-01 3:54 a.m., Daniel Vetter wrote:
> On Wed, Aug 26, 2020 at 11:24:23AM +0300, Pekka Paalanen wrote:
>> On Tue, 25 Aug 2020 12:58:19 -0400
>> "Kazlauskas, Nicholas" <nicholas.kazlauskas@amd.com> wrote:
>>
>>> On 2020-08-22 5:59 a.m., Michel Dänzer wrote:
>>>> On 2020-08-21 8:07 p.m., Kazlauskas, Nicholas wrote:  
>>>>> On 2020-08-21 12:57 p.m., 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.
>>>>>>
>>>>>> The atomic helpers disable 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):
>>>>>>
>>>>>> * Toggling CRTC active to 1 failed if the cursor plane was enabled
>>>>>>     (e.g. via legacy DPMS property & cursor ioctl).
>>>>>> * Enabling the cursor plane failed, e.g. via the legacy cursor ioctl.  
>>>>>
>>>>> We previously had the requirement that the primary plane must be enabled
>>>>> but some userspace expects that they can enable just the overlay plane
>>>>> without anything else.
>>>>>
>>>>> I think the chromuiumos atomictest validates that this works as well:
>>>>>
>>>>> So is DRM going forward then with the expectation that this is wrong
>>>>> behavior from userspace?
>>>>>
>>>>> We require at least one plane to be enabled to display a cursor, but it
>>>>> doesn't necessarily need to be the primary.  
>>>>
>>>> It's a "pick your poison" situation:
>>>>
>>>> 1) Currently the checks are invalid (atomic_check must not decide based
>>>> on drm_crtc_state::active), and it's easy for legacy KMS userspace to
>>>> accidentally hit errors trying to enable/move the cursor or switch DPMS
>>>> off → on.
>>>>
>>>> 2) Accurately rejecting only atomic states where the cursor plane is
>>>> enabled but all other planes are off would break the KMS helper code,
>>>> which can only deal with the "CRTC on & primary plane off is not
>>>> allowed" case specifically.
>>>>
>>>> 3) This patch addresses 1) & 2) but may break existing atomic userspace
>>>> which wants to enable an overlay plane while disabling the primary plane.
>>>>
>>>>
>>>> I do think in principle atomic userspace is expected to handle case 3)
>>>> and leave the primary plane enabled. However, this is not ideal from an
>>>> energy consumption PoV. Therefore, here's another idea for a possible
>>>> way out of this quagmire:
>>>>
>>>> amdgpu_dm does not reject any atomic states based on which planes are
>>>> enabled in it. If the cursor plane is enabled but all other planes are
>>>> off, amdgpu_dm internally either:
>>>>
>>>> a) Enables an overlay plane and makes it invisible, e.g. by assigning a
>>>> minimum size FB with alpha = 0.
>>>>
>>>> b) Enables the primary plane and assigns a minimum size FB (scaled up to
>>>> the required size) containing all black, possibly using compression.
>>>> (Trying to minimize the memory bandwidth)
>>>>
>>>>
>>>> Does either of these seem feasible? If both do, which one would be
>>>> preferable?
>>>>
>>>>   
>>>
>>> It's really the same solution since DCN doesn't make a distinction 
>>> between primary or overlay planes in hardware. DCE doesn't have overlay 
>>> planes enabled so this is not relevant there.
>>>
>>> The old behavior (pre 5.1?) was to silently accept the commit even 
>>> though the screen would be completely black instead of outright 
>>> rejecting the commit.
>>>
>>> I almost wonder if that makes more sense in the short term here since 
>>> the only "userspace" affected here is IGT. We'll fail the CRC checks, 
>>> but no userspace actually tries to actively use a cursor with no primary 
>>> plane enabled from my understanding.
>>
>> Hi,
>>
>> I believe that there exists userspace that will *accidentally* attempt
>> to update the cursor plane while primary plane or whole CRTC is off.
>> Some versions of Mutter might do that on racy conditions, I suspect.
>> These are legacy KMS users, not atomic KMS.
>>
>> However, I do not believe there exists any userspace that would
>> actually expect the display to show the cursor plane alone without a
>> primary plane. Therefore I'd be ok with legacy cursor ioctls silently
>> succeeding. Atomic commits not. So the difference has to be in the
>> translation from legacy UAPI to kernel internal atomic interface.
>>
>>> In the long term I think we can work on getting cursor actually on the 
>>> screen in this case, though I can't say I really like having to reserve 
>>> some small buffer (eg. 16x16) for allowing lightup on this corner case.
>>
>> Why would you bother implementing that?
>>
>> Is there really an IGT test that unconditionally demands cursor plane
>> to be usable without any other planes?
> 
> The cursor plane isn't anything else than any other plane, aside from the
> legacy uapi implication that it's used for the legacy cursor ioctls.
> 
> Which means the cursor plane could actually be a full-featured plane, and
> it's totally legit to use just that without anything else enabled.
> 
> So yeah if you allow that, it better show something :-)
> 
> Personally I'd lean towards merging this patch to close the gap (oldest
> regressions wins and all that) and then implement the black plane hack on
> top.

Not sure I'm a big fan of the black plane hack. Is there any way we
could allow the (non-displayed) cursor for the legacy IOCTL but not for
the atomic IOCTL? I assume that would require a change to core code in
the atomic helpers that convert legacy IOCTLs to atomic for drivers.

Harry

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

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

[Harry Wentland]

On 2020-09-01 3:54 a.m., Daniel Vetter wrote:
> On Wed, Aug 26, 2020 at 11:24:23AM +0300, Pekka Paalanen wrote:
>> On Tue, 25 Aug 2020 12:58:19 -0400
>> "Kazlauskas, Nicholas" <nicholas.kazlauskas@amd.com> wrote:
>>
>>> On 2020-08-22 5:59 a.m., Michel Dänzer wrote:
>>>> On 2020-08-21 8:07 p.m., Kazlauskas, Nicholas wrote:  
>>>>> On 2020-08-21 12:57 p.m., 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.
>>>>>>
>>>>>> The atomic helpers disable 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):
>>>>>>
>>>>>> * Toggling CRTC active to 1 failed if the cursor plane was enabled
>>>>>>     (e.g. via legacy DPMS property & cursor ioctl).
>>>>>> * Enabling the cursor plane failed, e.g. via the legacy cursor ioctl.  
>>>>>
>>>>> We previously had the requirement that the primary plane must be enabled
>>>>> but some userspace expects that they can enable just the overlay plane
>>>>> without anything else.
>>>>>
>>>>> I think the chromuiumos atomictest validates that this works as well:
>>>>>
>>>>> So is DRM going forward then with the expectation that this is wrong
>>>>> behavior from userspace?
>>>>>
>>>>> We require at least one plane to be enabled to display a cursor, but it
>>>>> doesn't necessarily need to be the primary.  
>>>>
>>>> It's a "pick your poison" situation:
>>>>
>>>> 1) Currently the checks are invalid (atomic_check must not decide based
>>>> on drm_crtc_state::active), and it's easy for legacy KMS userspace to
>>>> accidentally hit errors trying to enable/move the cursor or switch DPMS
>>>> off → on.
>>>>
>>>> 2) Accurately rejecting only atomic states where the cursor plane is
>>>> enabled but all other planes are off would break the KMS helper code,
>>>> which can only deal with the "CRTC on & primary plane off is not
>>>> allowed" case specifically.
>>>>
>>>> 3) This patch addresses 1) & 2) but may break existing atomic userspace
>>>> which wants to enable an overlay plane while disabling the primary plane.
>>>>
>>>>
>>>> I do think in principle atomic userspace is expected to handle case 3)
>>>> and leave the primary plane enabled. However, this is not ideal from an
>>>> energy consumption PoV. Therefore, here's another idea for a possible
>>>> way out of this quagmire:
>>>>
>>>> amdgpu_dm does not reject any atomic states based on which planes are
>>>> enabled in it. If the cursor plane is enabled but all other planes are
>>>> off, amdgpu_dm internally either:
>>>>
>>>> a) Enables an overlay plane and makes it invisible, e.g. by assigning a
>>>> minimum size FB with alpha = 0.
>>>>
>>>> b) Enables the primary plane and assigns a minimum size FB (scaled up to
>>>> the required size) containing all black, possibly using compression.
>>>> (Trying to minimize the memory bandwidth)
>>>>
>>>>
>>>> Does either of these seem feasible? If both do, which one would be
>>>> preferable?
>>>>
>>>>   
>>>
>>> It's really the same solution since DCN doesn't make a distinction 
>>> between primary or overlay planes in hardware. DCE doesn't have overlay 
>>> planes enabled so this is not relevant there.
>>>
>>> The old behavior (pre 5.1?) was to silently accept the commit even 
>>> though the screen would be completely black instead of outright 
>>> rejecting the commit.
>>>
>>> I almost wonder if that makes more sense in the short term here since 
>>> the only "userspace" affected here is IGT. We'll fail the CRC checks, 
>>> but no userspace actually tries to actively use a cursor with no primary 
>>> plane enabled from my understanding.
>>
>> Hi,
>>
>> I believe that there exists userspace that will *accidentally* attempt
>> to update the cursor plane while primary plane or whole CRTC is off.
>> Some versions of Mutter might do that on racy conditions, I suspect.
>> These are legacy KMS users, not atomic KMS.
>>
>> However, I do not believe there exists any userspace that would
>> actually expect the display to show the cursor plane alone without a
>> primary plane. Therefore I'd be ok with legacy cursor ioctls silently
>> succeeding. Atomic commits not. So the difference has to be in the
>> translation from legacy UAPI to kernel internal atomic interface.
>>
>>> In the long term I think we can work on getting cursor actually on the 
>>> screen in this case, though I can't say I really like having to reserve 
>>> some small buffer (eg. 16x16) for allowing lightup on this corner case.
>>
>> Why would you bother implementing that?
>>
>> Is there really an IGT test that unconditionally demands cursor plane
>> to be usable without any other planes?
> 
> The cursor plane isn't anything else than any other plane, aside from the
> legacy uapi implication that it's used for the legacy cursor ioctls.
> 
> Which means the cursor plane could actually be a full-featured plane, and
> it's totally legit to use just that without anything else enabled.
> 
> So yeah if you allow that, it better show something :-)
> 
> Personally I'd lean towards merging this patch to close the gap (oldest
> regressions wins and all that) and then implement the black plane hack on
> top.

Not sure I'm a big fan of the black plane hack. Is there any way we
could allow the (non-displayed) cursor for the legacy IOCTL but not for
the atomic IOCTL? I assume that would require a change to core code in
the atomic helpers that convert legacy IOCTLs to atomic for drivers.

Harry

> -Daniel
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Vetter]

On Tue, Sep 01, 2020 at 09:58:43AM -0400, Harry Wentland wrote:
> 
> 
> On 2020-09-01 3:54 a.m., Daniel Vetter wrote:
> > On Wed, Aug 26, 2020 at 11:24:23AM +0300, Pekka Paalanen wrote:
> >> On Tue, 25 Aug 2020 12:58:19 -0400
> >> "Kazlauskas, Nicholas" <nicholas.kazlauskas@amd.com> wrote:
> >>
> >>> On 2020-08-22 5:59 a.m., Michel Dänzer wrote:
> >>>> On 2020-08-21 8:07 p.m., Kazlauskas, Nicholas wrote:  
> >>>>> On 2020-08-21 12:57 p.m., 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.
> >>>>>>
> >>>>>> The atomic helpers disable 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):
> >>>>>>
> >>>>>> * Toggling CRTC active to 1 failed if the cursor plane was enabled
> >>>>>>     (e.g. via legacy DPMS property & cursor ioctl).
> >>>>>> * Enabling the cursor plane failed, e.g. via the legacy cursor ioctl.  
> >>>>>
> >>>>> We previously had the requirement that the primary plane must be enabled
> >>>>> but some userspace expects that they can enable just the overlay plane
> >>>>> without anything else.
> >>>>>
> >>>>> I think the chromuiumos atomictest validates that this works as well:
> >>>>>
> >>>>> So is DRM going forward then with the expectation that this is wrong
> >>>>> behavior from userspace?
> >>>>>
> >>>>> We require at least one plane to be enabled to display a cursor, but it
> >>>>> doesn't necessarily need to be the primary.  
> >>>>
> >>>> It's a "pick your poison" situation:
> >>>>
> >>>> 1) Currently the checks are invalid (atomic_check must not decide based
> >>>> on drm_crtc_state::active), and it's easy for legacy KMS userspace to
> >>>> accidentally hit errors trying to enable/move the cursor or switch DPMS
> >>>> off → on.
> >>>>
> >>>> 2) Accurately rejecting only atomic states where the cursor plane is
> >>>> enabled but all other planes are off would break the KMS helper code,
> >>>> which can only deal with the "CRTC on & primary plane off is not
> >>>> allowed" case specifically.
> >>>>
> >>>> 3) This patch addresses 1) & 2) but may break existing atomic userspace
> >>>> which wants to enable an overlay plane while disabling the primary plane.
> >>>>
> >>>>
> >>>> I do think in principle atomic userspace is expected to handle case 3)
> >>>> and leave the primary plane enabled. However, this is not ideal from an
> >>>> energy consumption PoV. Therefore, here's another idea for a possible
> >>>> way out of this quagmire:
> >>>>
> >>>> amdgpu_dm does not reject any atomic states based on which planes are
> >>>> enabled in it. If the cursor plane is enabled but all other planes are
> >>>> off, amdgpu_dm internally either:
> >>>>
> >>>> a) Enables an overlay plane and makes it invisible, e.g. by assigning a
> >>>> minimum size FB with alpha = 0.
> >>>>
> >>>> b) Enables the primary plane and assigns a minimum size FB (scaled up to
> >>>> the required size) containing all black, possibly using compression.
> >>>> (Trying to minimize the memory bandwidth)
> >>>>
> >>>>
> >>>> Does either of these seem feasible? If both do, which one would be
> >>>> preferable?
> >>>>
> >>>>   
> >>>
> >>> It's really the same solution since DCN doesn't make a distinction 
> >>> between primary or overlay planes in hardware. DCE doesn't have overlay 
> >>> planes enabled so this is not relevant there.
> >>>
> >>> The old behavior (pre 5.1?) was to silently accept the commit even 
> >>> though the screen would be completely black instead of outright 
> >>> rejecting the commit.
> >>>
> >>> I almost wonder if that makes more sense in the short term here since 
> >>> the only "userspace" affected here is IGT. We'll fail the CRC checks, 
> >>> but no userspace actually tries to actively use a cursor with no primary 
> >>> plane enabled from my understanding.
> >>
> >> Hi,
> >>
> >> I believe that there exists userspace that will *accidentally* attempt
> >> to update the cursor plane while primary plane or whole CRTC is off.
> >> Some versions of Mutter might do that on racy conditions, I suspect.
> >> These are legacy KMS users, not atomic KMS.
> >>
> >> However, I do not believe there exists any userspace that would
> >> actually expect the display to show the cursor plane alone without a
> >> primary plane. Therefore I'd be ok with legacy cursor ioctls silently
> >> succeeding. Atomic commits not. So the difference has to be in the
> >> translation from legacy UAPI to kernel internal atomic interface.
> >>
> >>> In the long term I think we can work on getting cursor actually on the 
> >>> screen in this case, though I can't say I really like having to reserve 
> >>> some small buffer (eg. 16x16) for allowing lightup on this corner case.
> >>
> >> Why would you bother implementing that?
> >>
> >> Is there really an IGT test that unconditionally demands cursor plane
> >> to be usable without any other planes?
> > 
> > The cursor plane isn't anything else than any other plane, aside from the
> > legacy uapi implication that it's used for the legacy cursor ioctls.
> > 
> > Which means the cursor plane could actually be a full-featured plane, and
> > it's totally legit to use just that without anything else enabled.
> > 
> > So yeah if you allow that, it better show something :-)
> > 
> > Personally I'd lean towards merging this patch to close the gap (oldest
> > regressions wins and all that) and then implement the black plane hack on
> > top.
> 
> Not sure I'm a big fan of the black plane hack. Is there any way we
> could allow the (non-displayed) cursor for the legacy IOCTL but not for
> the atomic IOCTL? I assume that would require a change to core code in
> the atomic helpers that convert legacy IOCTLs to atomic for drivers.

That's the "just dont show the cursor when it's not possible" hack, which
is also rather iffy imo.

The other side is that this is all kinda uapi, or at least we've spent a
lot of attempts trying to needle all this through rmfb and cursor ioctls,
and I'm not sure what exactly you can change without breaking something.
Yeah it's not helper stuff as in the commit message, it's core ioctl code
unfortunately.
-Daniel

> 
> Harry
> 
> > -Daniel
> > 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Daniel Vetter]

On Tue, Sep 01, 2020 at 09:58:43AM -0400, Harry Wentland wrote:
> 
> 
> On 2020-09-01 3:54 a.m., Daniel Vetter wrote:
> > On Wed, Aug 26, 2020 at 11:24:23AM +0300, Pekka Paalanen wrote:
> >> On Tue, 25 Aug 2020 12:58:19 -0400
> >> "Kazlauskas, Nicholas" <nicholas.kazlauskas@amd.com> wrote:
> >>
> >>> On 2020-08-22 5:59 a.m., Michel Dänzer wrote:
> >>>> On 2020-08-21 8:07 p.m., Kazlauskas, Nicholas wrote:  
> >>>>> On 2020-08-21 12:57 p.m., 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.
> >>>>>>
> >>>>>> The atomic helpers disable 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):
> >>>>>>
> >>>>>> * Toggling CRTC active to 1 failed if the cursor plane was enabled
> >>>>>>     (e.g. via legacy DPMS property & cursor ioctl).
> >>>>>> * Enabling the cursor plane failed, e.g. via the legacy cursor ioctl.  
> >>>>>
> >>>>> We previously had the requirement that the primary plane must be enabled
> >>>>> but some userspace expects that they can enable just the overlay plane
> >>>>> without anything else.
> >>>>>
> >>>>> I think the chromuiumos atomictest validates that this works as well:
> >>>>>
> >>>>> So is DRM going forward then with the expectation that this is wrong
> >>>>> behavior from userspace?
> >>>>>
> >>>>> We require at least one plane to be enabled to display a cursor, but it
> >>>>> doesn't necessarily need to be the primary.  
> >>>>
> >>>> It's a "pick your poison" situation:
> >>>>
> >>>> 1) Currently the checks are invalid (atomic_check must not decide based
> >>>> on drm_crtc_state::active), and it's easy for legacy KMS userspace to
> >>>> accidentally hit errors trying to enable/move the cursor or switch DPMS
> >>>> off → on.
> >>>>
> >>>> 2) Accurately rejecting only atomic states where the cursor plane is
> >>>> enabled but all other planes are off would break the KMS helper code,
> >>>> which can only deal with the "CRTC on & primary plane off is not
> >>>> allowed" case specifically.
> >>>>
> >>>> 3) This patch addresses 1) & 2) but may break existing atomic userspace
> >>>> which wants to enable an overlay plane while disabling the primary plane.
> >>>>
> >>>>
> >>>> I do think in principle atomic userspace is expected to handle case 3)
> >>>> and leave the primary plane enabled. However, this is not ideal from an
> >>>> energy consumption PoV. Therefore, here's another idea for a possible
> >>>> way out of this quagmire:
> >>>>
> >>>> amdgpu_dm does not reject any atomic states based on which planes are
> >>>> enabled in it. If the cursor plane is enabled but all other planes are
> >>>> off, amdgpu_dm internally either:
> >>>>
> >>>> a) Enables an overlay plane and makes it invisible, e.g. by assigning a
> >>>> minimum size FB with alpha = 0.
> >>>>
> >>>> b) Enables the primary plane and assigns a minimum size FB (scaled up to
> >>>> the required size) containing all black, possibly using compression.
> >>>> (Trying to minimize the memory bandwidth)
> >>>>
> >>>>
> >>>> Does either of these seem feasible? If both do, which one would be
> >>>> preferable?
> >>>>
> >>>>   
> >>>
> >>> It's really the same solution since DCN doesn't make a distinction 
> >>> between primary or overlay planes in hardware. DCE doesn't have overlay 
> >>> planes enabled so this is not relevant there.
> >>>
> >>> The old behavior (pre 5.1?) was to silently accept the commit even 
> >>> though the screen would be completely black instead of outright 
> >>> rejecting the commit.
> >>>
> >>> I almost wonder if that makes more sense in the short term here since 
> >>> the only "userspace" affected here is IGT. We'll fail the CRC checks, 
> >>> but no userspace actually tries to actively use a cursor with no primary 
> >>> plane enabled from my understanding.
> >>
> >> Hi,
> >>
> >> I believe that there exists userspace that will *accidentally* attempt
> >> to update the cursor plane while primary plane or whole CRTC is off.
> >> Some versions of Mutter might do that on racy conditions, I suspect.
> >> These are legacy KMS users, not atomic KMS.
> >>
> >> However, I do not believe there exists any userspace that would
> >> actually expect the display to show the cursor plane alone without a
> >> primary plane. Therefore I'd be ok with legacy cursor ioctls silently
> >> succeeding. Atomic commits not. So the difference has to be in the
> >> translation from legacy UAPI to kernel internal atomic interface.
> >>
> >>> In the long term I think we can work on getting cursor actually on the 
> >>> screen in this case, though I can't say I really like having to reserve 
> >>> some small buffer (eg. 16x16) for allowing lightup on this corner case.
> >>
> >> Why would you bother implementing that?
> >>
> >> Is there really an IGT test that unconditionally demands cursor plane
> >> to be usable without any other planes?
> > 
> > The cursor plane isn't anything else than any other plane, aside from the
> > legacy uapi implication that it's used for the legacy cursor ioctls.
> > 
> > Which means the cursor plane could actually be a full-featured plane, and
> > it's totally legit to use just that without anything else enabled.
> > 
> > So yeah if you allow that, it better show something :-)
> > 
> > Personally I'd lean towards merging this patch to close the gap (oldest
> > regressions wins and all that) and then implement the black plane hack on
> > top.
> 
> Not sure I'm a big fan of the black plane hack. Is there any way we
> could allow the (non-displayed) cursor for the legacy IOCTL but not for
> the atomic IOCTL? I assume that would require a change to core code in
> the atomic helpers that convert legacy IOCTLs to atomic for drivers.

That's the "just dont show the cursor when it's not possible" hack, which
is also rather iffy imo.

The other side is that this is all kinda uapi, or at least we've spent a
lot of attempts trying to needle all this through rmfb and cursor ioctls,
and I'm not sure what exactly you can change without breaking something.
Yeah it's not helper stuff as in the commit message, it's core ioctl code
unfortunately.
-Daniel

> 
> Harry
> 
> > -Daniel
> > 

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

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

[Michel Dänzer]

On 2020-09-02 9:02 a.m., Daniel Vetter wrote:
> On Tue, Sep 01, 2020 at 09:58:43AM -0400, Harry Wentland wrote:
>> On 2020-09-01 3:54 a.m., Daniel Vetter wrote:
>>> On Wed, Aug 26, 2020 at 11:24:23AM +0300, Pekka Paalanen wrote:
>>>> On Tue, 25 Aug 2020 12:58:19 -0400
>>>> "Kazlauskas, Nicholas" <nicholas.kazlauskas@amd.com> wrote:
>>>>
>>>>> On 2020-08-22 5:59 a.m., Michel Dänzer wrote:
>>>>>>
>>>>>> It's a "pick your poison" situation:
>>>>>>
>>>>>> 1) Currently the checks are invalid (atomic_check must not decide based
>>>>>> on drm_crtc_state::active), and it's easy for legacy KMS userspace to
>>>>>> accidentally hit errors trying to enable/move the cursor or switch DPMS
>>>>>> off → on.
>>>>>>
>>>>>> 2) Accurately rejecting only atomic states where the cursor plane is
>>>>>> enabled but all other planes are off would break the KMS helper code,
>>>>>> which can only deal with the "CRTC on & primary plane off is not
>>>>>> allowed" case specifically.
>>>>>>
>>>>>> 3) This patch addresses 1) & 2) but may break existing atomic userspace
>>>>>> which wants to enable an overlay plane while disabling the primary plane.
>>>>>>
>>>>>>
>>>>>> I do think in principle atomic userspace is expected to handle case 3)
>>>>>> and leave the primary plane enabled. However, this is not ideal from an
>>>>>> energy consumption PoV. Therefore, here's another idea for a possible
>>>>>> way out of this quagmire:
>>>>>>
>>>>>> amdgpu_dm does not reject any atomic states based on which planes are
>>>>>> enabled in it. If the cursor plane is enabled but all other planes are
>>>>>> off, amdgpu_dm internally either:
>>>>>>
>>>>>> a) Enables an overlay plane and makes it invisible, e.g. by assigning a
>>>>>> minimum size FB with alpha = 0.
>>>>>>
>>>>>> b) Enables the primary plane and assigns a minimum size FB (scaled up to
>>>>>> the required size) containing all black, possibly using compression.
>>>>>> (Trying to minimize the memory bandwidth)
>>>>>>
>>>>>>
>>>>>> Does either of these seem feasible? If both do, which one would be
>>>>>> preferable?
>>>>>>
>>>>>>   
>>>>>
>>>>> It's really the same solution since DCN doesn't make a distinction 
>>>>> between primary or overlay planes in hardware. DCE doesn't have overlay 
>>>>> planes enabled so this is not relevant there.
>>>>>
>>>>> The old behavior (pre 5.1?) was to silently accept the commit even 
>>>>> though the screen would be completely black instead of outright 
>>>>> rejecting the commit.
>>>>>
>>>>> I almost wonder if that makes more sense in the short term here since 
>>>>> the only "userspace" affected here is IGT. We'll fail the CRC checks, 
>>>>> but no userspace actually tries to actively use a cursor with no primary 
>>>>> plane enabled from my understanding.
>>>>
>>>> Hi,
>>>>
>>>> I believe that there exists userspace that will *accidentally* attempt
>>>> to update the cursor plane while primary plane or whole CRTC is off.
>>>> Some versions of Mutter might do that on racy conditions, I suspect.
>>>> These are legacy KMS users, not atomic KMS.
>>>>
>>>> However, I do not believe there exists any userspace that would
>>>> actually expect the display to show the cursor plane alone without a
>>>> primary plane. Therefore I'd be ok with legacy cursor ioctls silently
>>>> succeeding. Atomic commits not. So the difference has to be in the
>>>> translation from legacy UAPI to kernel internal atomic interface.
>>>>
>>>>> In the long term I think we can work on getting cursor actually on the 
>>>>> screen in this case, though I can't say I really like having to reserve 
>>>>> some small buffer (eg. 16x16) for allowing lightup on this corner case.
>>>>
>>>> Why would you bother implementing that?
>>>>
>>>> Is there really an IGT test that unconditionally demands cursor plane
>>>> to be usable without any other planes?
>>>
>>> The cursor plane isn't anything else than any other plane, aside from the
>>> legacy uapi implication that it's used for the legacy cursor ioctls.
>>>
>>> Which means the cursor plane could actually be a full-featured plane, and
>>> it's totally legit to use just that without anything else enabled.
>>>
>>> So yeah if you allow that, it better show something :-)
>>>
>>> Personally I'd lean towards merging this patch to close the gap (oldest
>>> regressions wins and all that) and then implement the black plane hack on
>>> top.
>>
>> Not sure I'm a big fan of the black plane hack. Is there any way we
>> could allow the (non-displayed) cursor for the legacy IOCTL but not for
>> the atomic IOCTL? I assume that would require a change to core code in
>> the atomic helpers that convert legacy IOCTLs to atomic for drivers.
> 
> That's the "just dont show the cursor when it's not possible" hack, which
> is also rather iffy imo.
> 
> The other side is that this is all kinda uapi, or at least we've spent a
> lot of attempts trying to needle all this through rmfb and cursor ioctls,
> and I'm not sure what exactly you can change without breaking something.
> Yeah it's not helper stuff as in the commit message, it's core ioctl code
> unfortunately.

Yeah, and I'm not sure how it could work.

E.g. I don't think the core code for the legacy cursor ioctl can just
ignore an error from the driver when trying to enable the cursor plane,
as there can be genuine errors for other reasons? So instead the driver
code would have to special-case somehow for certain legacy ioctls, which
sounds very iffy, if it's possible at all.

Also, what should happen if atomic user-space destroys the FB assigned
to an overlay plane, and disabling that plane leaves only the cursor
plane enabled?

There would need to be a more specific / less hand-wavy proposal how
exactly this is envisioned to work.


Anyway, this sub-thread is now about the next steps after this patch,
not about alternatives to it, right?


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

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

[Michel Dänzer]

On 2020-09-02 9:02 a.m., Daniel Vetter wrote:
> On Tue, Sep 01, 2020 at 09:58:43AM -0400, Harry Wentland wrote:
>> On 2020-09-01 3:54 a.m., Daniel Vetter wrote:
>>> On Wed, Aug 26, 2020 at 11:24:23AM +0300, Pekka Paalanen wrote:
>>>> On Tue, 25 Aug 2020 12:58:19 -0400
>>>> "Kazlauskas, Nicholas" <nicholas.kazlauskas@amd.com> wrote:
>>>>
>>>>> On 2020-08-22 5:59 a.m., Michel Dänzer wrote:
>>>>>>
>>>>>> It's a "pick your poison" situation:
>>>>>>
>>>>>> 1) Currently the checks are invalid (atomic_check must not decide based
>>>>>> on drm_crtc_state::active), and it's easy for legacy KMS userspace to
>>>>>> accidentally hit errors trying to enable/move the cursor or switch DPMS
>>>>>> off → on.
>>>>>>
>>>>>> 2) Accurately rejecting only atomic states where the cursor plane is
>>>>>> enabled but all other planes are off would break the KMS helper code,
>>>>>> which can only deal with the "CRTC on & primary plane off is not
>>>>>> allowed" case specifically.
>>>>>>
>>>>>> 3) This patch addresses 1) & 2) but may break existing atomic userspace
>>>>>> which wants to enable an overlay plane while disabling the primary plane.
>>>>>>
>>>>>>
>>>>>> I do think in principle atomic userspace is expected to handle case 3)
>>>>>> and leave the primary plane enabled. However, this is not ideal from an
>>>>>> energy consumption PoV. Therefore, here's another idea for a possible
>>>>>> way out of this quagmire:
>>>>>>
>>>>>> amdgpu_dm does not reject any atomic states based on which planes are
>>>>>> enabled in it. If the cursor plane is enabled but all other planes are
>>>>>> off, amdgpu_dm internally either:
>>>>>>
>>>>>> a) Enables an overlay plane and makes it invisible, e.g. by assigning a
>>>>>> minimum size FB with alpha = 0.
>>>>>>
>>>>>> b) Enables the primary plane and assigns a minimum size FB (scaled up to
>>>>>> the required size) containing all black, possibly using compression.
>>>>>> (Trying to minimize the memory bandwidth)
>>>>>>
>>>>>>
>>>>>> Does either of these seem feasible? If both do, which one would be
>>>>>> preferable?
>>>>>>
>>>>>>   
>>>>>
>>>>> It's really the same solution since DCN doesn't make a distinction 
>>>>> between primary or overlay planes in hardware. DCE doesn't have overlay 
>>>>> planes enabled so this is not relevant there.
>>>>>
>>>>> The old behavior (pre 5.1?) was to silently accept the commit even 
>>>>> though the screen would be completely black instead of outright 
>>>>> rejecting the commit.
>>>>>
>>>>> I almost wonder if that makes more sense in the short term here since 
>>>>> the only "userspace" affected here is IGT. We'll fail the CRC checks, 
>>>>> but no userspace actually tries to actively use a cursor with no primary 
>>>>> plane enabled from my understanding.
>>>>
>>>> Hi,
>>>>
>>>> I believe that there exists userspace that will *accidentally* attempt
>>>> to update the cursor plane while primary plane or whole CRTC is off.
>>>> Some versions of Mutter might do that on racy conditions, I suspect.
>>>> These are legacy KMS users, not atomic KMS.
>>>>
>>>> However, I do not believe there exists any userspace that would
>>>> actually expect the display to show the cursor plane alone without a
>>>> primary plane. Therefore I'd be ok with legacy cursor ioctls silently
>>>> succeeding. Atomic commits not. So the difference has to be in the
>>>> translation from legacy UAPI to kernel internal atomic interface.
>>>>
>>>>> In the long term I think we can work on getting cursor actually on the 
>>>>> screen in this case, though I can't say I really like having to reserve 
>>>>> some small buffer (eg. 16x16) for allowing lightup on this corner case.
>>>>
>>>> Why would you bother implementing that?
>>>>
>>>> Is there really an IGT test that unconditionally demands cursor plane
>>>> to be usable without any other planes?
>>>
>>> The cursor plane isn't anything else than any other plane, aside from the
>>> legacy uapi implication that it's used for the legacy cursor ioctls.
>>>
>>> Which means the cursor plane could actually be a full-featured plane, and
>>> it's totally legit to use just that without anything else enabled.
>>>
>>> So yeah if you allow that, it better show something :-)
>>>
>>> Personally I'd lean towards merging this patch to close the gap (oldest
>>> regressions wins and all that) and then implement the black plane hack on
>>> top.
>>
>> Not sure I'm a big fan of the black plane hack. Is there any way we
>> could allow the (non-displayed) cursor for the legacy IOCTL but not for
>> the atomic IOCTL? I assume that would require a change to core code in
>> the atomic helpers that convert legacy IOCTLs to atomic for drivers.
> 
> That's the "just dont show the cursor when it's not possible" hack, which
> is also rather iffy imo.
> 
> The other side is that this is all kinda uapi, or at least we've spent a
> lot of attempts trying to needle all this through rmfb and cursor ioctls,
> and I'm not sure what exactly you can change without breaking something.
> Yeah it's not helper stuff as in the commit message, it's core ioctl code
> unfortunately.

Yeah, and I'm not sure how it could work.

E.g. I don't think the core code for the legacy cursor ioctl can just
ignore an error from the driver when trying to enable the cursor plane,
as there can be genuine errors for other reasons? So instead the driver
code would have to special-case somehow for certain legacy ioctls, which
sounds very iffy, if it's possible at all.

Also, what should happen if atomic user-space destroys the FB assigned
to an overlay plane, and disabling that plane leaves only the cursor
plane enabled?

There would need to be a more specific / less hand-wavy proposal how
exactly this is envisioned to work.


Anyway, this sub-thread is now about the next steps after this patch,
not about alternatives to it, right?


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

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

[Michel Dänzer]

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

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

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

[Michel Dänzer]

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

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

[Daniel Vetter]

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>
> ---
>  .../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
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Daniel Vetter]

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

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

[Michel Dänzer]

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


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

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

[Michel Dänzer]

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


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

[Kazlauskas, Nicholas]

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.

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 <nicholas.kazlauskas@amd.com>

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

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

[Kazlauskas, Nicholas]

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.

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 <nicholas.kazlauskas@amd.com>

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

[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@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

Re: [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@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
_______________________________________________
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

[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@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).

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@amd.com>
> 
> Thanks!
> 
> 

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

Re: [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@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).

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@amd.com>
> 
> 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

[Michel Dänzer]

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
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Michel Dänzer]

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

[Alex Deucher]

Applied.  Thanks!

Alex

On Mon, Sep 14, 2020 at 10:37 AM Kazlauskas, Nicholas
<nicholas.kazlauskas@amd.com> 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.
>
> 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 <nicholas.kazlauskas@amd.com>
>
> Regards,
> Nicholas Kazlauskas
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Alex Deucher]

Applied.  Thanks!

Alex

On Mon, Sep 14, 2020 at 10:37 AM Kazlauskas, Nicholas
<nicholas.kazlauskas@amd.com> 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.
>
> 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 <nicholas.kazlauskas@amd.com>
>
> Regards,
> Nicholas Kazlauskas
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@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