diff mbox series

drm/dp_mst: Tune down the WARN modesetting a port with full_pbn=0

Message ID 20210216123448.410545-1-imre.deak@intel.com (mailing list archive)
State New, archived
Headers show
Series drm/dp_mst: Tune down the WARN modesetting a port with full_pbn=0 | expand

Commit Message

Imre Deak Feb. 16, 2021, 12:34 p.m. UTC
It's possible to modeset a connector/mst port that has a 0 full_pbn
value: if the sink on the port deasserts its HPD and a branch device
reports this via a CSN with the port's ddps=0 and pdt!=NONE the driver
clears full_pbn, but the corresponding connector can be still
modesetted.

This happened on a DELL U2719D monitor as the branch device and an LG
27UL650-W daisy-chained to it, the LG monitor generating a long HPD
pulse (doing this for some reason always when waking up from some power
saving state).

Tune down the WARN about this scenario to a debug message.

References: https://gitlab.freedesktop.org/drm/intel/-/issues/1917
Cc: Lyude Paul <lyude@redhat.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/drm_dp_mst_topology.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

Comments

Lyude Paul Feb. 17, 2021, 6:07 p.m. UTC | #1
On Tue, 2021-02-16 at 14:34 +0200, Imre Deak wrote:
> It's possible to modeset a connector/mst port that has a 0 full_pbn
> value: if the sink on the port deasserts its HPD and a branch device
> reports this via a CSN with the port's ddps=0 and pdt!=NONE the driver
> clears full_pbn, but the corresponding connector can be still
> modesetted.
> 
> This happened on a DELL U2719D monitor as the branch device and an LG
> 27UL650-W daisy-chained to it, the LG monitor generating a long HPD
> pulse (doing this for some reason always when waking up from some power
> saving state).
> 
> Tune down the WARN about this scenario to a debug message.
> 
> References: https://gitlab.freedesktop.org/drm/intel/-/issues/1917
> Cc: Lyude Paul <lyude@redhat.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/drm_dp_mst_topology.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 0a54506c27738..dcaf4bf9b62f6 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -5118,11 +5118,15 @@ drm_dp_mst_atomic_check_port_bw_limit(struct
> drm_dp_mst_port *port,
>                 if (!found)
>                         return 0;
>  
> -               /* This should never happen, as it means we tried to
> -                * set a mode before querying the full_pbn
> +               /*
> +                * This could happen if the sink deasserted its HPD line, but
> +                * the branch device still reports it as attached (PDT !=
> NONE).
>                  */
> -               if (WARN_ON(!port->full_pbn))
> +               if (!port->full_pbn) {
> +                       drm_dbg_kms(port->mgr->dev, "[MSTB:%p] [MST PORT:%p]
> no BW available for the port\n",
> +                                   port->parent, port);

Shouldn't this use the atomic debugging level? With that fixed, this is:

Reviewed-by: Lyude Paul <lyude@redhat.com>

>                         return -EINVAL;
> +               }
>  
>                 pbn_used = vcpi->pbn;
>         } else {
Imre Deak Feb. 17, 2021, 6:27 p.m. UTC | #2
On Wed, Feb 17, 2021 at 01:07:11PM -0500, Lyude Paul wrote:
> On Tue, 2021-02-16 at 14:34 +0200, Imre Deak wrote:
> > It's possible to modeset a connector/mst port that has a 0 full_pbn
> > value: if the sink on the port deasserts its HPD and a branch device
> > reports this via a CSN with the port's ddps=0 and pdt!=NONE the driver
> > clears full_pbn, but the corresponding connector can be still
> > modesetted.
> > 
> > This happened on a DELL U2719D monitor as the branch device and an LG
> > 27UL650-W daisy-chained to it, the LG monitor generating a long HPD
> > pulse (doing this for some reason always when waking up from some power
> > saving state).
> > 
> > Tune down the WARN about this scenario to a debug message.
> > 
> > References: https://gitlab.freedesktop.org/drm/intel/-/issues/1917
> > Cc: Lyude Paul <lyude@redhat.com>
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/drm_dp_mst_topology.c | 10 +++++++---
> >  1 file changed, 7 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> > b/drivers/gpu/drm/drm_dp_mst_topology.c
> > index 0a54506c27738..dcaf4bf9b62f6 100644
> > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > @@ -5118,11 +5118,15 @@ drm_dp_mst_atomic_check_port_bw_limit(struct
> > drm_dp_mst_port *port,
> >                 if (!found)
> >                         return 0;
> >  
> > -               /* This should never happen, as it means we tried to
> > -                * set a mode before querying the full_pbn
> > +               /*
> > +                * This could happen if the sink deasserted its HPD line, but
> > +                * the branch device still reports it as attached (PDT != NONE).
> >                  */
> > -               if (WARN_ON(!port->full_pbn))
> > +               if (!port->full_pbn) {
> > +                       drm_dbg_kms(port->mgr->dev, "[MSTB:%p] [MST PORT:%p] no BW available for the port\n",
> > +                                   port->parent, port);
> 
> Shouldn't this use the atomic debugging level? With that fixed, this is:
> 
> Reviewed-by: Lyude Paul <lyude@redhat.com>

Thanks, yes mixed up the debug level, will fix that.

> >                         return -EINVAL;
> > +               }
> >  
> >                 pbn_used = vcpi->pbn;
> >         } else {
> 
> -- 
> Sincerely,
>    Lyude Paul (she/her)
>    Software Engineer at Red Hat
>    
> Note: I deal with a lot of emails and have a lot of bugs on my plate. If you've
> asked me a question, are waiting for a review/merge on a patch, etc. and I
> haven't responded in a while, please feel free to send me another email to check
> on my status. I don't bite!
>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index 0a54506c27738..dcaf4bf9b62f6 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -5118,11 +5118,15 @@  drm_dp_mst_atomic_check_port_bw_limit(struct drm_dp_mst_port *port,
 		if (!found)
 			return 0;
 
-		/* This should never happen, as it means we tried to
-		 * set a mode before querying the full_pbn
+		/*
+		 * This could happen if the sink deasserted its HPD line, but
+		 * the branch device still reports it as attached (PDT != NONE).
 		 */
-		if (WARN_ON(!port->full_pbn))
+		if (!port->full_pbn) {
+			drm_dbg_kms(port->mgr->dev, "[MSTB:%p] [MST PORT:%p] no BW available for the port\n",
+				    port->parent, port);
 			return -EINVAL;
+		}
 
 		pbn_used = vcpi->pbn;
 	} else {