On Mon, Feb 29, 2016 at 07:38:21AM +0100, Thierry Reding wrote: > On Tue, Dec 15, 2015 at 11:38:10AM +0100, Daniel Vetter wrote: > > On Mon, Dec 14, 2015 at 01:56:03PM +0100, Thierry Reding wrote: > > > From: Thierry Reding <treding@xxxxxxxxxx> > > > > > > Store the AUX read interval from DPCD, so that it can be used to wait > > > for the durations given in the specification during link training. > > > > > > Signed-off-by: Thierry Reding <treding@xxxxxxxxxx> > > > --- > > > drivers/gpu/drm/drm_dp_helper.c | 4 ++++ > > > include/drm/drm_dp_helper.h | 17 +++++++++++++++++ > > > 2 files changed, 21 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c > > > index 76ac68bc1042..da519acfeba7 100644 > > > --- a/drivers/gpu/drm/drm_dp_helper.c > > > +++ b/drivers/gpu/drm/drm_dp_helper.c > > > @@ -339,6 +339,7 @@ static void drm_dp_link_reset(struct drm_dp_link *link) > > > link->max_lanes = 0; > > > > > > drm_dp_link_caps_reset(&link->caps); > > > + link->aux_rd_interval = 0; > > > link->edp = 0; > > > > > > link->rate = 0; > > > @@ -392,6 +393,9 @@ int drm_dp_link_probe(struct drm_dp_aux *aux, struct drm_dp_link *link) > > > link->edp = edp_revs[value]; > > > } > > > > > > + /* DP_TRAINING_AUX_RD_INTERVAL is in units of 4 milliseconds */ > > > + link->aux_rd_interval = drm_dp_aux_rd_interval(values); > > > > Hm, just wondering a bit of the relationship between link and cap. Is link > > all about sink really, and not the source? At least in my experience it > > makes a lot of sense to strictly keep these two separate, since otherwise > > you'll have lots of fun aligning things in generic code. Anyway, just a > > thougth. > > The idea is that the link is the intersection between sink and source > capabilities. Drivers are supposed to call drm_dp_link_probe() to obtain > the capabilities of the sink and then adjust the struct drm_dp_link > according to their limitations (e.g. decrease the maximum rate if they > don't support 5.4 GHz, reduce the number of lanes if they only support > two, ...). > > Once that's done the drivers can call drm_dp_link_choose() to select the > "best" set of configuration parameters given the link capabilities. > > Note that this is strictly deriven from reading the specification under > the assumption that this is how things work in real life. My, arguably > limited, experience with Tegra shows that this is true. But perhaps that > is overly naive. > > But I'd like to better understand what other drivers require so that > these helpers can be improved and be useful by more than a single > driver. Currently every driver implements their own DP stack, which I > think is rather unfortunate because we end up with vastly different > behaviour depending on which driver is in use. > > Of course if that's what's desired, I'm more than happy to move this > code into the Tegra driver. I might have to duplicate the code that's > shared with MSM, but it's really not a lot compared to what's coming > up. Ah, that makes sense to have a link structure for the negotioted values that fit both sink and source. In that case I think making it a bit leaner would be good, and only include parameters which actually need to be negiotated (link rate, lanes, ...). aux retry intervals and stuff like that should be agnostic, or at least only depend upon the sink (I hope). Given that I don't think we should store such values, but instead recompute them from the read/cached dcpd blocks. That would be more in-line with the style of the helpers used by i915/radeon. This way the actually negotiated stuff sticks out more. And like you say we need that in some struct so that drivers can apply quirks/tricks (i.e. simply to avoid the midlayer mistake). E.g. one thing i915 does is up-front link training, to be able to handle usb type C correctly. Another reason why I think we should charge ahead with caching source/sink invariant values too much is that I still believe extracting SST helpers from MST helpers is a good long-term plan. Keeping is simple (and accepting more copypasting due to that) will make the inevitable refactoring easier I hope. Cheers, Daniel > > > + > > > link->rate = link->max_rate; > > > link->lanes = link->max_lanes; > > > > > > diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h > > > index 825aaf4e8c71..20ae0e413b64 100644 > > > --- a/include/drm/drm_dp_helper.h > > > +++ b/include/drm/drm_dp_helper.h > > > @@ -678,6 +678,22 @@ drm_dp_alternate_scrambler_reset_cap(const u8 dpcd[DP_RECEIVER_CAP_SIZE]) > > > DP_ALTERNATE_SCRAMBLER_RESET_CAP; > > > } > > > > > > +/** > > > + * drm_dp_read_aux_interval() - read the AUX read interval from the DPCD > > > + * @dpcd: receiver capacity buffer > > > + * > > > + * Reads the AUX read interval (in microseconds) from the DPCD. Note that the > > > + * TRAINING_AUX_RD_INTERVAL stores the value in units of 4 milliseconds. > > > + * > > > + * Returns: > > > + * The read AUX interval in microseconds. > > > + */ > > > +static inline unsigned int > > > +drm_dp_aux_rd_interval(const u8 dpcd[DP_RECEIVER_CAP_SIZE]) > > > > We should use this one here in the 2 delay helpers for channel_eq and > > clock_recovery imo. > > Agreed. > > Thierry -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
