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 <[email protected]> > > > > > > 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 <[email protected]> > > > --- > > > > > > 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 [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
