On Mon, 2016-10-24 at 22:10 +0300, Jani Nikula wrote:
> On Mon, 24 Oct 2016, Imre Deak <imre.deak@xxxxxxxxx> wrote:
> > On Mon, 2016-10-24 at 21:14 +0300, Jani Nikula wrote:
> > > On Mon, 24 Oct 2016, Imre Deak <imre.deak@xxxxxxxxx> 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@xxxxxxxxx>
> > > > Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx>
> > > > ---
> > > > 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.
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"?
>
> > 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;
> > >
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
