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. 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
