dri-devel.lists.freedesktop.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] drm: dp_mst_topology: Ensure the proposed pbn does not exceed available bandwidth
@ 2019-08-27 20:35 Sean Paul
  2019-08-28  0:31 ` Lyude Paul
  0 siblings, 1 reply; 4+ messages in thread
From: Sean Paul @ 2019-08-27 20:35 UTC (permalink / raw)
  To: dri-devel; +Cc: Maxime Ripard, David Airlie, Sean Paul, Sean Paul

From: Sean Paul <seanpaul@chromium.org>

The PBN is calculated by the DP helpers during atomic check using the
adjusted mode. In some cases, the EDID may contain modes which are not
possible given the available PBN. One such example of this is if you
downgrade the DP version of a 4k display from 1.2 to 1.1. The EDID would
still contain the 4k mode, but it is not possible without HBR2. Another
case might be if the branch device and sink have to downgrade their link
speed during training.

This patch checks that the proposed pbn does not exceed a port's
available pbn before allocating vcpi slots.

Signed-off-by: Sean Paul <seanpaul@chromium.org>
---

This is my first dip into MST, so it's possible (probable?) that I'm
doing something wrong. However this seems to fix the issue I'm
experiencing, so at least we have a base to work with.

I'm more than a bit confused when available_pbn is 0, and I've tried
retrying enum_path_resources and even doing a phy powerup before epr,
but it still insists available_pbn=0. I've been looking at the DP 1.3
spec and it isn't too clear on why this might be. If there are better
resources, I'm interested :)

 drivers/gpu/drm/drm_dp_mst_topology.c | 31 ++++++++++++++++++++++++++-
 1 file changed, 30 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
index 82add736e17d..16a88230091a 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -2182,7 +2182,26 @@ static int drm_dp_send_enum_path_resources(struct drm_dp_mst_topology_mgr *mgr,
 				DRM_ERROR("got incorrect port in response\n");
 			DRM_DEBUG_KMS("enum path resources %d: %d %d\n", txmsg->reply.u.path_resources.port_number, txmsg->reply.u.path_resources.full_payload_bw_number,
 			       txmsg->reply.u.path_resources.avail_payload_bw_number);
-			port->available_pbn = txmsg->reply.u.path_resources.avail_payload_bw_number;
+
+			/*
+			 * Some monitors (Samsung U28D590 at least) return 0 for
+			 * available pbn while in low power mode.
+			 *
+			 * If we were to trust this value, we'd end up failing
+			 * all vcpi allocations, since the requested bandwidth
+			 * will be compared to 0. So use the full pbn as
+			 * available. Doing this will cap the vcpi allocations
+			 * at the trained link rate and will at least prevent
+			 * us from trying to allocate payloads larger than our
+			 * full pbn.
+			 *
+			 * If there is actually no bandwidth left, we'll fail
+			 * on ALLOCATE_PAYLOAD anyways.
+			 */
+			if (txmsg->reply.u.path_resources.avail_payload_bw_number)
+				port->available_pbn = txmsg->reply.u.path_resources.avail_payload_bw_number;
+			else
+				port->available_pbn = txmsg->reply.u.path_resources.full_payload_bw_number;
 		}
 	}
 
@@ -3239,6 +3258,16 @@ int drm_dp_atomic_find_vcpi_slots(struct drm_atomic_state *state,
 	struct drm_dp_vcpi_allocation *pos, *vcpi = NULL;
 	int prev_slots, req_slots, ret;
 
+	/*
+	 * Sanity check that the pbn proposed doesn't exceed the maximum
+	 * available pbn for the port. This could happen if the EDID is
+	 * advertising a mode which needs a faster link rate than has been
+	 * trained between the sink and the branch device (easy to repro by
+	 * using "compatibility" mode on a 4k display and downgrading to DP 1.1)
+	 */
+	if (pbn > port->available_pbn)
+		return -EINVAL;
+
 	topology_state = drm_atomic_get_mst_topology_state(state, mgr);
 	if (IS_ERR(topology_state))
 		return PTR_ERR(topology_state);
-- 
Sean Paul, Software Engineer, Google / Chromium OS

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

^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH] drm: dp_mst_topology: Ensure the proposed pbn does not exceed available bandwidth
  2019-08-27 20:35 [PATCH] drm: dp_mst_topology: Ensure the proposed pbn does not exceed available bandwidth Sean Paul
@ 2019-08-28  0:31 ` Lyude Paul
  2019-08-28 14:29   ` Sean Paul
  0 siblings, 1 reply; 4+ messages in thread
From: Lyude Paul @ 2019-08-28  0:31 UTC (permalink / raw)
  To: Sean Paul, dri-devel; +Cc: Maxime Ripard, David Airlie, Sean Paul

Hi! So, I don't have access to the DP 1.3 spec or anything later than 1.3.
However, I'm fairly sure this is very much againt spec since there's no way
for us to determine the available PBN otherwise...
Honestly though, being against spec might not surprise me here.

I kinda want to look into this more before giving an r-b on this, although
I've got some comments down below on the patch itself. Feel free to poke me
tommorrow so we can take a closer look and maybe figure out more about what's
going on here, I'll try to remember to poke you as well.

On Tue, 2019-08-27 at 16:35 -0400, Sean Paul wrote:
> From: Sean Paul <seanpaul@chromium.org>
> 
> The PBN is calculated by the DP helpers during atomic check using the
> adjusted mode. In some cases, the EDID may contain modes which are not
> possible given the available PBN. One such example of this is if you
> downgrade the DP version of a 4k display from 1.2 to 1.1. The EDID would
> still contain the 4k mode, but it is not possible without HBR2. Another
> case might be if the branch device and sink have to downgrade their link
> speed during training.
> 
> This patch checks that the proposed pbn does not exceed a port's
> available pbn before allocating vcpi slots.
> 
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> ---
> 
> This is my first dip into MST, so it's possible (probable?) that I'm
> doing something wrong. However this seems to fix the issue I'm
> experiencing, so at least we have a base to work with.
> 
> I'm more than a bit confused when available_pbn is 0, and I've tried
> retrying enum_path_resources and even doing a phy powerup before epr,
> but it still insists available_pbn=0. I've been looking at the DP 1.3
> spec and it isn't too clear on why this might be. If there are better
> resources, I'm interested :)
> 
>  drivers/gpu/drm/drm_dp_mst_topology.c | 31 ++++++++++++++++++++++++++-
>  1 file changed, 30 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 82add736e17d..16a88230091a 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -2182,7 +2182,26 @@ static int drm_dp_send_enum_path_resources(struct
> drm_dp_mst_topology_mgr *mgr,
>  				DRM_ERROR("got incorrect port in response\n");
>  			DRM_DEBUG_KMS("enum path resources %d: %d %d\n",
> txmsg->reply.u.path_resources.port_number, txmsg-
> >reply.u.path_resources.full_payload_bw_number,
>  			       txmsg-
> >reply.u.path_resources.avail_payload_bw_number);
> -			port->available_pbn = txmsg-
> >reply.u.path_resources.avail_payload_bw_number;
> +
> +			/*
> +			 * Some monitors (Samsung U28D590 at least) return 0
> for
> +			 * available pbn while in low power mode.
> +			 *
> +			 * If we were to trust this value, we'd end up failing
> +			 * all vcpi allocations, since the requested bandwidth
> +			 * will be compared to 0. So use the full pbn as
> +			 * available. Doing this will cap the vcpi allocations
> +			 * at the trained link rate and will at least prevent
> +			 * us from trying to allocate payloads larger than our
> +			 * full pbn.
> +			 *
> +			 * If there is actually no bandwidth left, we'll fail
> +			 * on ALLOCATE_PAYLOAD anyways.

I would hope this would be the case, but I've seen a lot of situations where
MST hubs will just stop responding in situations like this instead of
providing an actual error. So it's probably safer to validate this as much as
possible beforehand without relying on the sink to tell us when we've done
something wrong.

> +			 */
> +			if (txmsg-
> >reply.u.path_resources.avail_payload_bw_number)
> +				port->available_pbn = txmsg-
> >reply.u.path_resources.avail_payload_bw_number;
> +			else
> +				port->available_pbn = txmsg-
> >reply.u.path_resources.full_payload_bw_number;

I think we should use a DP quirk for this so that we only follow this behavior
for this monitor, and not any others. It's possible that other things can
cause bandwidth restrictions, and while I haven't had a chance to look further
into it I've noticed that sometimes sinks even end up allocating more
handwidth then we actually asked for.

>  		}
>  	}
>  
> @@ -3239,6 +3258,16 @@ int drm_dp_atomic_find_vcpi_slots(struct
> drm_atomic_state *state,
>  	struct drm_dp_vcpi_allocation *pos, *vcpi = NULL;
>  	int prev_slots, req_slots, ret;
>  
> +	/*
> +	 * Sanity check that the pbn proposed doesn't exceed the maximum
> +	 * available pbn for the port. This could happen if the EDID is
> +	 * advertising a mode which needs a faster link rate than has been
> +	 * trained between the sink and the branch device (easy to repro by
> +	 * using "compatibility" mode on a 4k display and downgrading to DP
> 1.1)
> +	 */
> +	if (pbn > port->available_pbn)
> +		return -EINVAL;
> +

port->available_pbn isn't really protected by any actual locking yet
unfortunately :(. See

https://patchwork.freedesktop.org/patch/318683/?series=63847&rev=1

so I'm not sure we should be validating the PBN during the atomic check until
that's landed (we already don't do this, so dropping this wouldn't make things
any worse then they are right now). Additionally, we also don't have a handler
for DP_RESOURCE_STATUS_NOTIFY UP messages yet either.

>  	topology_state = drm_atomic_get_mst_topology_state(state, mgr);
>  	if (IS_ERR(topology_state))
>  		return PTR_ERR(topology_state);
-- 
Cheers,
	Lyude Paul

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] drm: dp_mst_topology: Ensure the proposed pbn does not exceed available bandwidth
  2019-08-28  0:31 ` Lyude Paul
@ 2019-08-28 14:29   ` Sean Paul
  2019-08-28 16:43     ` Sean Paul
  0 siblings, 1 reply; 4+ messages in thread
From: Sean Paul @ 2019-08-28 14:29 UTC (permalink / raw)
  To: Lyude Paul; +Cc: Maxime Ripard, dri-devel, David Airlie, Sean Paul, Sean Paul

On Tue, Aug 27, 2019 at 08:31:29PM -0400, Lyude Paul wrote:
> Hi! So, I don't have access to the DP 1.3 spec or anything later than 1.3.

1.3 was just what I was looking at, I checked 1.2 and it looks the same (aside
from a typo fix).

> However, I'm fairly sure this is very much againt spec since there's no way
> for us to determine the available PBN otherwise...
> Honestly though, being against spec might not surprise me here.

Yeah, I was pretty surprised by this behavior too. Everything in the spec states
that Available_Payload_Bandwidth_Number is what we should be using to determine
maximum PBN.

> 
> I kinda want to look into this more before giving an r-b on this, although
> I've got some comments down below on the patch itself. Feel free to poke me
> tommorrow so we can take a closer look and maybe figure out more about what's
> going on here, I'll try to remember to poke you as well.

Help would be very much appreciated, thanks!

> 
> On Tue, 2019-08-27 at 16:35 -0400, Sean Paul wrote:
> > From: Sean Paul <seanpaul@chromium.org>
> > 
> > The PBN is calculated by the DP helpers during atomic check using the
> > adjusted mode. In some cases, the EDID may contain modes which are not
> > possible given the available PBN. One such example of this is if you
> > downgrade the DP version of a 4k display from 1.2 to 1.1. The EDID would
> > still contain the 4k mode, but it is not possible without HBR2. Another
> > case might be if the branch device and sink have to downgrade their link
> > speed during training.
> > 
> > This patch checks that the proposed pbn does not exceed a port's
> > available pbn before allocating vcpi slots.
> > 
> > Signed-off-by: Sean Paul <seanpaul@chromium.org>
> > ---
> > 
> > This is my first dip into MST, so it's possible (probable?) that I'm
> > doing something wrong. However this seems to fix the issue I'm
> > experiencing, so at least we have a base to work with.
> > 
> > I'm more than a bit confused when available_pbn is 0, and I've tried
> > retrying enum_path_resources and even doing a phy powerup before epr,
> > but it still insists available_pbn=0. I've been looking at the DP 1.3
> > spec and it isn't too clear on why this might be. If there are better
> > resources, I'm interested :)
> > 
> >  drivers/gpu/drm/drm_dp_mst_topology.c | 31 ++++++++++++++++++++++++++-
> >  1 file changed, 30 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> > b/drivers/gpu/drm/drm_dp_mst_topology.c
> > index 82add736e17d..16a88230091a 100644
> > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > @@ -2182,7 +2182,26 @@ static int drm_dp_send_enum_path_resources(struct
> > drm_dp_mst_topology_mgr *mgr,
> >  				DRM_ERROR("got incorrect port in response\n");
> >  			DRM_DEBUG_KMS("enum path resources %d: %d %d\n",
> > txmsg->reply.u.path_resources.port_number, txmsg-
> > >reply.u.path_resources.full_payload_bw_number,
> >  			       txmsg-
> > >reply.u.path_resources.avail_payload_bw_number);
> > -			port->available_pbn = txmsg-
> > >reply.u.path_resources.avail_payload_bw_number;
> > +
> > +			/*
> > +			 * Some monitors (Samsung U28D590 at least) return 0
> > for
> > +			 * available pbn while in low power mode.
> > +			 *
> > +			 * If we were to trust this value, we'd end up failing
> > +			 * all vcpi allocations, since the requested bandwidth
> > +			 * will be compared to 0. So use the full pbn as
> > +			 * available. Doing this will cap the vcpi allocations
> > +			 * at the trained link rate and will at least prevent
> > +			 * us from trying to allocate payloads larger than our
> > +			 * full pbn.
> > +			 *
> > +			 * If there is actually no bandwidth left, we'll fail
> > +			 * on ALLOCATE_PAYLOAD anyways.
> 
> I would hope this would be the case, but I've seen a lot of situations where
> MST hubs will just stop responding in situations like this instead of
> providing an actual error. So it's probably safer to validate this as much as
> possible beforehand without relying on the sink to tell us when we've done
> something wrong.
> 

thismakesmesad.gif

> > +			 */
> > +			if (txmsg-
> > >reply.u.path_resources.avail_payload_bw_number)
> > +				port->available_pbn = txmsg-
> > >reply.u.path_resources.avail_payload_bw_number;
> > +			else
> > +				port->available_pbn = txmsg-
> > >reply.u.path_resources.full_payload_bw_number;
> 
> I think we should use a DP quirk for this so that we only follow this behavior
> for this monitor, and not any others. It's possible that other things can
> cause bandwidth restrictions, and while I haven't had a chance to look further
> into it I've noticed that sometimes sinks even end up allocating more
> handwidth then we actually asked for.
> 

After reading your reply, I tested a few other monitors I had laying around and
all are behaving the same way -- even without the "compatibility" mode enabled.
The common theme seems to be that when I reboot my source the available_pbn on
boot is 0. If I hotplug after that, available_pbn is correct.

I'm wondering whether the branch device (CableMatters USB-C 2x DP hub) is
holding onto that allocation across reboot. That said, the payload allocation
I'm making doesn't use the full available PBN, so I would kind of expect the
available pbn to be non-zero if this were the case, but ¯\_(ツ)_/¯

> >  		}
> >  	}
> >  
> > @@ -3239,6 +3258,16 @@ int drm_dp_atomic_find_vcpi_slots(struct
> > drm_atomic_state *state,
> >  	struct drm_dp_vcpi_allocation *pos, *vcpi = NULL;
> >  	int prev_slots, req_slots, ret;
> >  
> > +	/*
> > +	 * Sanity check that the pbn proposed doesn't exceed the maximum
> > +	 * available pbn for the port. This could happen if the EDID is
> > +	 * advertising a mode which needs a faster link rate than has been
> > +	 * trained between the sink and the branch device (easy to repro by
> > +	 * using "compatibility" mode on a 4k display and downgrading to DP
> > 1.1)
> > +	 */
> > +	if (pbn > port->available_pbn)
> > +		return -EINVAL;
> > +
> 
> port->available_pbn isn't really protected by any actual locking yet
> unfortunately :(. See
> 
> https://patchwork.freedesktop.org/patch/318683/?series=63847&rev=1
> 
> so I'm not sure we should be validating the PBN during the atomic check until
> that's landed (we already don't do this, so dropping this wouldn't make things
> any worse then they are right now). Additionally, we also don't have a handler
> for DP_RESOURCE_STATUS_NOTIFY UP messages yet either.

Yep, that's fine with me.

Sean

> 
> >  	topology_state = drm_atomic_get_mst_topology_state(state, mgr);
> >  	if (IS_ERR(topology_state))
> >  		return PTR_ERR(topology_state);
> -- 
> Cheers,
> 	Lyude Paul
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] drm: dp_mst_topology: Ensure the proposed pbn does not exceed available bandwidth
  2019-08-28 14:29   ` Sean Paul
@ 2019-08-28 16:43     ` Sean Paul
  0 siblings, 0 replies; 4+ messages in thread
From: Sean Paul @ 2019-08-28 16:43 UTC (permalink / raw)
  To: Lyude Paul; +Cc: Maxime Ripard, dri-devel, David Airlie, Sean Paul, Sean Paul

On Wed, Aug 28, 2019 at 10:29:32AM -0400, Sean Paul wrote:
> On Tue, Aug 27, 2019 at 08:31:29PM -0400, Lyude Paul wrote:
> > Hi! So, I don't have access to the DP 1.3 spec or anything later than 1.3.
> 
> 1.3 was just what I was looking at, I checked 1.2 and it looks the same (aside
> from a typo fix).
> 
> > However, I'm fairly sure this is very much againt spec since there's no way
> > for us to determine the available PBN otherwise...
> > Honestly though, being against spec might not surprise me here.
> 
> Yeah, I was pretty surprised by this behavior too. Everything in the spec states
> that Available_Payload_Bandwidth_Number is what we should be using to determine
> maximum PBN.
> 
> > 
> > I kinda want to look into this more before giving an r-b on this, although
> > I've got some comments down below on the patch itself. Feel free to poke me
> > tommorrow so we can take a closer look and maybe figure out more about what's
> > going on here, I'll try to remember to poke you as well.
> 
> Help would be very much appreciated, thanks!
> 
> > 
> > On Tue, 2019-08-27 at 16:35 -0400, Sean Paul wrote:
> > > From: Sean Paul <seanpaul@chromium.org>
> > > 
> > > The PBN is calculated by the DP helpers during atomic check using the
> > > adjusted mode. In some cases, the EDID may contain modes which are not
> > > possible given the available PBN. One such example of this is if you
> > > downgrade the DP version of a 4k display from 1.2 to 1.1. The EDID would
> > > still contain the 4k mode, but it is not possible without HBR2. Another
> > > case might be if the branch device and sink have to downgrade their link
> > > speed during training.
> > > 
> > > This patch checks that the proposed pbn does not exceed a port's
> > > available pbn before allocating vcpi slots.
> > > 
> > > Signed-off-by: Sean Paul <seanpaul@chromium.org>
> > > ---
> > > 
> > > This is my first dip into MST, so it's possible (probable?) that I'm
> > > doing something wrong. However this seems to fix the issue I'm
> > > experiencing, so at least we have a base to work with.
> > > 
> > > I'm more than a bit confused when available_pbn is 0, and I've tried
> > > retrying enum_path_resources and even doing a phy powerup before epr,
> > > but it still insists available_pbn=0. I've been looking at the DP 1.3
> > > spec and it isn't too clear on why this might be. If there are better
> > > resources, I'm interested :)
> > > 
> > >  drivers/gpu/drm/drm_dp_mst_topology.c | 31 ++++++++++++++++++++++++++-
> > >  1 file changed, 30 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> > > b/drivers/gpu/drm/drm_dp_mst_topology.c
> > > index 82add736e17d..16a88230091a 100644
> > > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > > @@ -2182,7 +2182,26 @@ static int drm_dp_send_enum_path_resources(struct
> > > drm_dp_mst_topology_mgr *mgr,
> > >  				DRM_ERROR("got incorrect port in response\n");
> > >  			DRM_DEBUG_KMS("enum path resources %d: %d %d\n",
> > > txmsg->reply.u.path_resources.port_number, txmsg-
> > > >reply.u.path_resources.full_payload_bw_number,
> > >  			       txmsg-
> > > >reply.u.path_resources.avail_payload_bw_number);
> > > -			port->available_pbn = txmsg-
> > > >reply.u.path_resources.avail_payload_bw_number;
> > > +
> > > +			/*
> > > +			 * Some monitors (Samsung U28D590 at least) return 0
> > > for
> > > +			 * available pbn while in low power mode.
> > > +			 *
> > > +			 * If we were to trust this value, we'd end up failing
> > > +			 * all vcpi allocations, since the requested bandwidth
> > > +			 * will be compared to 0. So use the full pbn as
> > > +			 * available. Doing this will cap the vcpi allocations
> > > +			 * at the trained link rate and will at least prevent
> > > +			 * us from trying to allocate payloads larger than our
> > > +			 * full pbn.
> > > +			 *
> > > +			 * If there is actually no bandwidth left, we'll fail
> > > +			 * on ALLOCATE_PAYLOAD anyways.
> > 
> > I would hope this would be the case, but I've seen a lot of situations where
> > MST hubs will just stop responding in situations like this instead of
> > providing an actual error. So it's probably safer to validate this as much as
> > possible beforehand without relying on the sink to tell us when we've done
> > something wrong.
> > 
> 
> thismakesmesad.gif
> 
> > > +			 */
> > > +			if (txmsg-
> > > >reply.u.path_resources.avail_payload_bw_number)
> > > +				port->available_pbn = txmsg-
> > > >reply.u.path_resources.avail_payload_bw_number;
> > > +			else
> > > +				port->available_pbn = txmsg-
> > > >reply.u.path_resources.full_payload_bw_number;
> > 
> > I think we should use a DP quirk for this so that we only follow this behavior
> > for this monitor, and not any others. It's possible that other things can
> > cause bandwidth restrictions, and while I haven't had a chance to look further
> > into it I've noticed that sometimes sinks even end up allocating more
> > handwidth then we actually asked for.
> > 
> 
> After reading your reply, I tested a few other monitors I had laying around and
> all are behaving the same way -- even without the "compatibility" mode enabled.
> The common theme seems to be that when I reboot my source the available_pbn on
> boot is 0. If I hotplug after that, available_pbn is correct.
> 
> I'm wondering whether the branch device (CableMatters USB-C 2x DP hub) is
> holding onto that allocation across reboot. That said, the payload allocation
> I'm making doesn't use the full available PBN, so I would kind of expect the
> available pbn to be non-zero if this were the case, but ¯\_(ツ)_/¯
> 

After playing around with this theory, it might hold some water. I added a
CLEAR_PAYLOAD_ID_TABLE broadcast before ENUM_PATH_RESOURCES when a port is
created. Now I'm getting the expected value in available_pbn reliably.

I'm not really sure why this works, since I'd expect the DPCD writes in
drm_dp_mst_topology_mgr_set_mst() to the PAYLOAD_ALLOCATE_* registers to be 
sufficient.

thoughts?

Sean

> > >  		}
> > >  	}
> > >  
> > > @@ -3239,6 +3258,16 @@ int drm_dp_atomic_find_vcpi_slots(struct
> > > drm_atomic_state *state,
> > >  	struct drm_dp_vcpi_allocation *pos, *vcpi = NULL;
> > >  	int prev_slots, req_slots, ret;
> > >  
> > > +	/*
> > > +	 * Sanity check that the pbn proposed doesn't exceed the maximum
> > > +	 * available pbn for the port. This could happen if the EDID is
> > > +	 * advertising a mode which needs a faster link rate than has been
> > > +	 * trained between the sink and the branch device (easy to repro by
> > > +	 * using "compatibility" mode on a 4k display and downgrading to DP
> > > 1.1)
> > > +	 */
> > > +	if (pbn > port->available_pbn)
> > > +		return -EINVAL;
> > > +
> > 
> > port->available_pbn isn't really protected by any actual locking yet
> > unfortunately :(. See
> > 
> > https://patchwork.freedesktop.org/patch/318683/?series=63847&rev=1
> > 
> > so I'm not sure we should be validating the PBN during the atomic check until
> > that's landed (we already don't do this, so dropping this wouldn't make things
> > any worse then they are right now). Additionally, we also don't have a handler
> > for DP_RESOURCE_STATUS_NOTIFY UP messages yet either.
> 
> Yep, that's fine with me.
> 
> Sean
> 
> > 
> > >  	topology_state = drm_atomic_get_mst_topology_state(state, mgr);
> > >  	if (IS_ERR(topology_state))
> > >  		return PTR_ERR(topology_state);
> > -- 
> > Cheers,
> > 	Lyude Paul
> > 
> 
> -- 
> Sean Paul, Software Engineer, Google / Chromium OS

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2019-08-28 16:43 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-27 20:35 [PATCH] drm: dp_mst_topology: Ensure the proposed pbn does not exceed available bandwidth Sean Paul
2019-08-28  0:31 ` Lyude Paul
2019-08-28 14:29   ` Sean Paul
2019-08-28 16:43     ` Sean Paul

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).