[Intel-gfx] [PATCH] drm/i915/skl: Buffer translation improvements

[David Weinehall]

On Thu, Jun 18, 2015 at 11:10:13AM +0100, Chris Wilson wrote:
> On Thu, Jun 18, 2015 at 12:50:33PM +0300, David Weinehall wrote:
> > +static const struct ddi_buf_trans *skl_get_buf_trans_dp(struct drm_device *dev,
> 
> struct drm_i915_private not struct drm_device!

The device uses both dev and dev_priv; only passing in
drm_i915_private wouldn't provide access to dev,
or am I missing something?

> > +							int *n_entries)
> > +{
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	const struct ddi_buf_trans *ddi_translations;
> > +	static int is_095v = -1;
> > +
> > +	if (is_095v == -1) {
> > +		u32 spr1 = I915_READ(UAIMI_SPR1);
> > +		is_095v = spr1 & SKL_VCCIO_MASK;
> > +	}
> > +
> > +	if (IS_SKL_ULX(dev) && !is_095v) {
> > +		ddi_translations = skl_y_085v_ddi_translations_dp;
> > +		*n_entries = ARRAY_SIZE(skl_y_085v_ddi_translations_dp);
> > +	} else if (IS_SKL_ULT(dev)) {
> > +		ddi_translations = skl_u_ddi_translations_dp;
> > +		*n_entries = ARRAY_SIZE(skl_u_ddi_translations_dp);
> > +	} else {
> > +		ddi_translations = skl_ddi_translations_dp;
> > +		*n_entries = ARRAY_SIZE(skl_ddi_translations_dp);
> > +	}
> 
> These are static routing, but called fairly often. (Often enough that
> you care to only read the reigster once.) Any reason not to preserve
> these routing tables in dev_priv or, slightly more preferrable, intel_dp?
> -Chris

No particularly good reason, so yes, it's a good proposal.
The reason I cached the register read was mostly because the less reads
from hardware the better (I was thinking of whether having the value
read more than once was too much already, but I figured that having
a global variable to hold a Skylake-specific value would be ugly).

I'll fix it on Monday (tomorrow is a public holiday here, so I'll
be traveling this weekend).


Kind regards, David