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

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

On Tue, 25 Oct 2016, Imre Deak <imre.deak@intel.com> wrote:
> On ti, 2016-10-25 at 12:28 +0300, Jani Nikula wrote:
>> On Mon, 24 Oct 2016, Imre Deak <imre.deak@intel.com> wrote:
>> > On Mon, 2016-10-24 at 22:10 +0300, Jani Nikula wrote:
>> > > 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?
>> > 
>> > As a reference to what 'Device Identification' above means? In any case
>> > if 'OUI Support' applies to the whole descriptor referring separately
>> > to OUI and Device Identification right afterwards is confusing to me.
>> 
>> What you call "desc" here means DPCD 0x400..0x40b for sinks, or
>> 0x500..0x50b for branches. That's OUI, an ascii device identification
>> string, and hw/sw revisions.
>
> Yes, I was talking about the specification.
>
>> What's the DPCD revision that LSPCON reports? OUI support is mandatory
>> for DPCD 1.2 and 1.4.
>
> 1.2
>
>> > Could we just read it out regardless of the flag and depend on the read
>> > failing or the values being zeroed if it's not "supported"?
>> 
>> Please tell me again why we use the OUI for this purpose instead of just
>> intel_dp->dpcd?
>
> For identifying if correct AUX data is returned in patch 8? Because imo
> an ID will mismatch with bigger likelihood than the start of DPCD, if
> for example an incorrect read will result in zeroes.

If the sink doesn't support some DPCD stuff, such as the OUI, it's
supposed to return all zeros... And this bugger is supposed to support
OUI because it's DP 1.2 but it doesn't, except it does anyway. *rolls
eyes*.

J.


>
> --Imre

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