Re: [PATCH v2 4/8] drm/i915/dp: Print full branch/sink descriptor

[Imre Deak <imre.deak@intel.com>]

On Mon, 2016-10-24 at 21:14 +0300, Jani Nikula wrote:
> On Mon, 24 Oct 2016, Imre Deak <imre.deak@intel.com> wrote:
> > Extend the branch/sink descriptor info with the missing device ID
> > field. While at it also read out all the descriptor registers in one
> > transfer and make the debug print more compact.
> > 
> > v2: (Jani)
> > - Cache the descriptor in intel_dp.
> > - Split out this change into a separate patch.
> > 
> > Cc: Jani Nikula <jani.nikula@intel.com>
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c  | 61 +++++++++++++---------------------------
> >  drivers/gpu/drm/i915/intel_drv.h | 10 +++++++
> >  2 files changed, 30 insertions(+), 41 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 1991250..726fdf2 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -1451,34 +1451,33 @@ static void intel_dp_print_rates(struct intel_dp *intel_dp)
> >  	DRM_DEBUG_KMS("common rates: %s\n", str);
> >  }
> >  
> > -static void intel_dp_print_hw_revision(struct intel_dp *intel_dp)
> > +static bool
> > +__intel_dp_read_desc(struct intel_dp *intel_dp, struct intel_dp_desc *desc)
> >  {
> > -	uint8_t rev;
> > -	int len;
> > +	u32 base = drm_dp_is_branch(intel_dp->dpcd) ? DP_BRANCH_OUI :
> > +						      DP_SINK_OUI;
> >  
> > -	if (!drm_dp_is_branch(intel_dp->dpcd))
> > -		return;
> > -
> > -	len = drm_dp_dpcd_read(&intel_dp->aux, DP_BRANCH_HW_REV, &rev, 1);
> > -	if (len < 0)
> > -		return;
> > -
> > -	DRM_DEBUG_KMS("sink hw revision: %d.%d\n", (rev & 0xf0) >> 4, rev & 0xf);
> > +	return drm_dp_dpcd_read(&intel_dp->aux, base, desc, sizeof(*desc)) ==
> > +	       sizeof(*desc);
> >  }
> >  
> > -static void intel_dp_print_sw_revision(struct intel_dp *intel_dp)
> > +static bool intel_dp_read_desc(struct intel_dp *intel_dp)
> >  {
> > -	uint8_t rev[2];
> > -	int len;
> > +	struct intel_dp_desc *desc = &intel_dp->desc;
> > +	bool oui_sup = intel_dp->dpcd[DP_DOWN_STREAM_PORT_COUNT] &
> > +		       DP_OUI_SUPPORT;
> >  
> > -	if (!drm_dp_is_branch(intel_dp->dpcd))
> > -		return;
> > +	if (__intel_dp_read_desc(intel_dp, desc) < 0)
> 
> The bool returned from __intel_dp_read_desc will never be less than 0...

Yep, that's a typo, will fix it.

> There's no point in reading the desc if oui_sup is false, is there? All of desc should read all zeros in that case, not just OUI.

The spec is not too clear about this yes, but as I read it oui_sup
applies only to the OUI reg, device ID and the revisions can be
still valid. The LSPCON FW has oui_sup cleared, but returns valid
device ID and revision values.

> I guess the desc should be reset to zeros at detect or somewhere. I'm
> sure we have other stuff we fail to reset too.

Yes, like the dpcd array. desc will be used only for debug printing atm
where we always re-read it first, and for LSPCON where it's constant,
so if that's fine for you I'd leave this for later.

> 
> > +		return false;
> >  
> > -	len = drm_dp_dpcd_read(&intel_dp->aux, DP_BRANCH_SW_REV, &rev, 2);
> > -	if (len < 0)
> > -		return;
> > +	DRM_DEBUG_KMS("DP %s: OUI %*phD%s dev-ID %.*s HW-rev %d.%d SW-rev %d.%d\n",
> 
> Maybe some form of %*pE would be more defensive for device id? See
> Documentation/printk-formats.txt.

Yes, can change that to '%*pE'.

> > +		      drm_dp_is_branch(intel_dp->dpcd) ? "branch" : "sink",
> > +		      (int)sizeof(desc->oui), desc->oui, oui_sup ? "" : "(NS)",
> > +		      (int)sizeof(desc->device_id), desc->device_id,
> > +		      desc->hw_rev >> 4, desc->hw_rev & 0xf,
> > +		      desc->sw_major_rev, desc->sw_minor_rev);
> >  
> > -	DRM_DEBUG_KMS("sink sw revision: %d.%d\n", rev[0], rev[1]);
> > +	return true;
> >  }
> >  
> >  static int rate_to_index(int find, const int *rates)
> > @@ -3621,23 +3620,6 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
> >  	return true;
> >  }
> >  
> > -static void
> > -intel_dp_probe_oui(struct intel_dp *intel_dp)
> > -{
> > -	bool is_branch = drm_dp_is_branch(intel_dp->dpcd);
> > -	u8 buf[3];
> > -
> > -	if (!(intel_dp->dpcd[DP_DOWN_STREAM_PORT_COUNT] & DP_OUI_SUPPORT))
> > -		return;
> > -
> > -	if (drm_dp_dpcd_read(&intel_dp->aux,
> > -			     is_branch ? DP_BRANCH_OUI : DP_SINK_OUI,
> > -			     buf, 3) == 3)
> > -		DRM_DEBUG_KMS("%s OUI: %02hx%02hx%02hx\n",
> > -			      is_branch ? "Branch" : "Sink",
> > -			      buf[0], buf[1], buf[2]);
> > -}
> > -
> >  static bool
> >  intel_dp_can_mst(struct intel_dp *intel_dp)
> >  {
> > @@ -4416,10 +4398,7 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
> >  
> >  	intel_dp_print_rates(intel_dp);
> >  
> > -	intel_dp_probe_oui(intel_dp);
> > -
> > -	intel_dp_print_hw_revision(intel_dp);
> > -	intel_dp_print_sw_revision(intel_dp);
> > +	intel_dp_read_desc(intel_dp);
> >  
> >  	intel_dp_configure_mst(intel_dp);
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 16b33f5..4c9f953 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -883,6 +883,14 @@ enum link_m_n_set {
> >  	M2_N2
> >  };
> >  
> > +struct intel_dp_desc {
> > +	u8 oui[3];
> > +	u8 device_id[6];
> > +	u8 hw_rev;
> > +	u8 sw_major_rev;
> > +	u8 sw_minor_rev;
> > +} __packed;
> > +
> >  struct intel_dp {
> >  	i915_reg_t output_reg;
> >  	i915_reg_t aux_ch_ctl_reg;
> > @@ -905,6 +913,8 @@ struct intel_dp {
> >  	/* sink rates as reported by DP_SUPPORTED_LINK_RATES */
> >  	uint8_t num_sink_rates;
> >  	int sink_rates[DP_MAX_SUPPORTED_RATES];
> > +	/* sink or branch descriptor */
> > +	struct intel_dp_desc desc;
> >  	struct drm_dp_aux aux;
> >  	uint8_t train_set[4];
> >  	int panel_power_up_delay;
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx