[PATCH 11/12] drm/dp: Read AUX read interval from DPCD

[Daniel Vetter]

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 
> > > 
> > > 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 
> > > ---
> > >  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.

[PATCH 11/12] drm/dp: Read AUX read interval from DPCD

[Thierry Reding]

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 
> > 
> > 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 
> > ---
> >  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.

> > +
> > 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
-- next part --
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: 

[PATCH 11/12] drm/dp: Read AUX read interval from DPCD

[Daniel Vetter]

On Mon, Dec 14, 2015 at 01:56:03PM +0100, Thierry Reding wrote:
> From: Thierry Reding 
> 
> 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 
> ---
>  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.

> +
>   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.
-Daniel

> +{
> + return dpcd[DP_TRAINING_AUX_RD_INTERVAL] * 4000;
> +}
> +
>  /*
>   * DisplayPort AUX channel
>   */
> @@ -805,6 +821,7 @@ struct drm_dp_link {
>   unsigned int max_lanes;
>  
>   struct drm_dp_link_caps caps;
> + unsigned int aux_rd_interval;
>   unsigned char edp;
>  
>   unsigned int rate;
> -- 
> 2.5.0
> 
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[PATCH 11/12] drm/dp: Read AUX read interval from DPCD

[Thierry Reding]

From: Thierry Reding 

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 
---
 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);
+
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])
+{
+   return dpcd[DP_TRAINING_AUX_RD_INTERVAL] * 4000;
+}
+
 /*
  * DisplayPort AUX channel
  */
@@ -805,6 +821,7 @@ struct drm_dp_link {
unsigned int max_lanes;

struct drm_dp_link_caps caps;
+   unsigned int aux_rd_interval;
unsigned char edp;

unsigned int rate;
-- 
2.5.0