[PATCH v2] drm: document how user-space should use link-status

[PATCH v2] drm: document how user-space should use link-status

[Simon Ser]

Describe what a "BAD" link-status means for user-space and how it should
handle it. The logic described has been implemented in igt [1].

v2:

- Change wording to avoid "enabled" (Daniel)
- Add paragraph about multiple connectors sharing the same CRTC (Pekka)
- Add paragraph about performing an atomic commit on a connector without
  updating the link-status property (Daniel)

[1]: https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/commit/fbe61f529737191d0920521946a575bd55f00fbe

Signed-off-by: Simon Ser <contact@emersion.fr>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Manasi Navare <manasi.d.navare@intel.com>
Cc: Pekka Paalanen <ppaalanen@gmail.com>
---
 drivers/gpu/drm/drm_connector.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index f2b20fd66319..829b21124048 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -994,6 +994,21 @@ static const struct drm_prop_enum_list dp_colorspaces[] = {
  *      after modeset, the kernel driver may set this to "BAD" and issue a
  *      hotplug uevent. Drivers should update this value using
  *      drm_connector_set_link_status_property().
+ *
+ *      When user-space receives the hotplug uevent and detects a "BAD"
+ *      link-status, the sink doesn't receive pixels anymore. The list of
+ *      available modes may have changed. User-space is expected to pick a new
+ *      mode if the current one has disappeared and perform a new modeset with
+ *      link-status set to "GOOD" to re-enable the connector.
+ *
+ *      If multiple connectors share the same CRTC and one of them gets a "BAD"
+ *      link-status, the other are unaffected (ie. the sinks still continue to
+ *      receive pixels).
+ *
+ *      When user-space performs an atomic commit on a connector with a "BAD"
+ *      link-status without resetting the property to "GOOD", it gets
+ *      implicitly reset. This might make the atomic commit fail if the modeset
+ *      is unsuccessful.
  * non_desktop:
  * 	Indicates the output should be ignored for purposes of displaying a
  * 	standard desktop environment or console. This is most likely because
-- 
2.26.2


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

Re: [PATCH v2] drm: document how user-space should use link-status

[Pekka Paalanen]

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

On Tue, 02 Jun 2020 19:18:15 +0000
Simon Ser <contact@emersion.fr> wrote:

> Describe what a "BAD" link-status means for user-space and how it should
> handle it. The logic described has been implemented in igt [1].
> 
> v2:
> 
> - Change wording to avoid "enabled" (Daniel)
> - Add paragraph about multiple connectors sharing the same CRTC (Pekka)
> - Add paragraph about performing an atomic commit on a connector without
>   updating the link-status property (Daniel)
> 
> [1]: https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/commit/fbe61f529737191d0920521946a575bd55f00fbe
> 
> Signed-off-by: Simon Ser <contact@emersion.fr>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Manasi Navare <manasi.d.navare@intel.com>
> Cc: Pekka Paalanen <ppaalanen@gmail.com>
> ---
>  drivers/gpu/drm/drm_connector.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index f2b20fd66319..829b21124048 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -994,6 +994,21 @@ static const struct drm_prop_enum_list dp_colorspaces[] = {
>   *      after modeset, the kernel driver may set this to "BAD" and issue a
>   *      hotplug uevent. Drivers should update this value using
>   *      drm_connector_set_link_status_property().
> + *
> + *      When user-space receives the hotplug uevent and detects a "BAD"
> + *      link-status, the sink doesn't receive pixels anymore. The list of
> + *      available modes may have changed. User-space is expected to pick a new
> + *      mode if the current one has disappeared and perform a new modeset with
> + *      link-status set to "GOOD" to re-enable the connector.
> + *
> + *      If multiple connectors share the same CRTC and one of them gets a "BAD"
> + *      link-status, the other are unaffected (ie. the sinks still continue to
> + *      receive pixels).
> + *

Hi,

looks good up to here.

> + *      When user-space performs an atomic commit on a connector with a "BAD"
> + *      link-status without resetting the property to "GOOD", it gets
> + *      implicitly reset. This might make the atomic commit fail if the modeset
> + *      is unsuccessful.

I think this was what Daniel was saying that the kernel should require
ALLOW_MODESET to be set for the automatic reset, right?

I'm fine with how the doc is written now. But if ALLOW_MODESET becomes
a requirement for the automatic reset, I suspect there is a risk to
regress Weston, assuming the automatic reset used to be successful.

I understand this doc describes the current situation and it answers my
questions, so:

Acked-by: Pekka Paalanen <pekka.paalanen@collabora.com>

Thanks,
pq

>   * non_desktop:
>   * 	Indicates the output should be ignored for purposes of displaying a
>   * 	standard desktop environment or console. This is most likely because


[-- 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 v2] drm: document how user-space should use link-status

[Simon Ser]

> > + *      When user-space performs an atomic commit on a connector with a "BAD"
> > + *      link-status without resetting the property to "GOOD", it gets
> > + *      implicitly reset. This might make the atomic commit fail if the modeset
> > + *      is unsuccessful.
>
> I think this was what Daniel was saying that the kernel should require
> ALLOW_MODESET to be set for the automatic reset, right?

Actually this paragraph isn't true. link-status is only reset to GOOD for
legacy modeset.

But right now this doesn't matter since no driver reads the link-status
property value as far as I can tell. Note, only i915 sets link-status
to BAD.

> I'm fine with how the doc is written now. But if ALLOW_MODESET becomes
> a requirement for the automatic reset, I suspect there is a risk to
> regress Weston, assuming the automatic reset used to be successful.

Right now a commit without ALLOW_MODESET won't reset link-status to GOOD,
but also won't re-train the link on i915. So I think it's fine to require
ALLOW_MODESET.

Should drivers read the value of the link-status property? Or should we
ignore user-space writes to the property and only require ALLOW_MODESET
to re-train the link?

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

Re: [PATCH v2] drm: document how user-space should use link-status

[Daniel Vetter]

On Wed, Jun 03, 2020 at 01:27:55PM +0000, Simon Ser wrote:
> > > + *      When user-space performs an atomic commit on a connector with a "BAD"
> > > + *      link-status without resetting the property to "GOOD", it gets
> > > + *      implicitly reset. This might make the atomic commit fail if the modeset
> > > + *      is unsuccessful.
> >
> > I think this was what Daniel was saying that the kernel should require
> > ALLOW_MODESET to be set for the automatic reset, right?
> 
> Actually this paragraph isn't true. link-status is only reset to GOOD for
> legacy modeset.
> 
> But right now this doesn't matter since no driver reads the link-status
> property value as far as I can tell. Note, only i915 sets link-status
> to BAD.

It's magic ... change in link status results in connectors_change (or
something like that), which forces the modeset (and first recomputation of
mode state) that fixes everything up.

But yeah I entirely missed that the autoreset to GOOD only happens in
legacy modeset.

I guess we might have some atomic compositors with slightly suboptimal
handling of link status failure :-/

> > I'm fine with how the doc is written now. But if ALLOW_MODESET becomes
> > a requirement for the automatic reset, I suspect there is a risk to
> > regress Weston, assuming the automatic reset used to be successful.
> 
> Right now a commit without ALLOW_MODESET won't reset link-status to GOOD,
> but also won't re-train the link on i915. So I think it's fine to require
> ALLOW_MODESET.
> 
> Should drivers read the value of the link-status property? Or should we
> ignore user-space writes to the property and only require ALLOW_MODESET
> to re-train the link?

Well without allow_modeset it'll fail, that's the problem. But since we
dont automatically restore, I think the only problem is that existing
atomic userspace might be stuck on a bad link for a while ...
-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