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

[Jani Nikula <jani.nikula@intel.com>]

On Mon, 24 Oct 2016, Imre Deak <imre.deak@intel.com> wrote:
> 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.

For address 00007h:

"""
Bit 7 = OUI Support
0 = OUI not supported
1 = OUI supported (OUI and Device Identification mandatory for DP 1.2)
00400h to 00402h for a Sink device, plus 00403h-0040Bh for Device
Identification
00500h to 00502h for a Branch device, plus 00503h-0050Bh for Device
Identification
"""

Based on that I'd say the bit covers device id and revisions too. Why
would the device id and revision offset be mentioned here otherwise?

> The LSPCON FW has oui_sup cleared, but returns valid device ID and
> revision values.

I think I'll cry a little.

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

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx