Re: [PATCH 1/2] drm/i915/dp: Print full branch/sink descriptor for all outputs

Imre Deak
2016-10-20T18:58:14Z

On Thu, 2016-10-20 at 21:06 +0300, Jani Nikula wrote:
> On Thu, 20 Oct 2016, Imre Deak <imre.deak@intel.com> wrote:
> > Extend the branch/sink descriptor info with the missing device ID
> > field and print this info for eDP and LSPCON connectors too.
> >
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c     | 83 +++++++++++++++----------------------
> >  drivers/gpu/drm/i915/intel_drv.h    | 13 ++++++
> >  drivers/gpu/drm/i915/intel_lspcon.c |  7 ++++
> >  3 files changed, 53 insertions(+), 50 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 88f3b74..e90211e 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -1442,42 +1442,34 @@ 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_is_branch(struct intel_dp *intel_dp)
>
> Belongs in drm dp helpers.

Ok.

>
> >  {
> > - uint8_t rev;
> > - int len;
> > -
> > - if ((drm_debug & DRM_UT_KMS) == 0)
> > - return;
> > -
> > - if (!(intel_dp->dpcd[DP_DOWNSTREAMPORT_PRESENT] &
> > -       DP_DWN_STRM_PORT_PRESENT))
> > - 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 intel_dp->dpcd[DP_DOWNSTREAMPORT_PRESENT] &
> > +        DP_DWN_STRM_PORT_PRESENT;
> >  }
> >
> > -static void intel_dp_print_sw_revision(struct intel_dp *intel_dp)
> > +bool
> > +intel_dp_read_desc(struct intel_dp *intel_dp, struct intel_dp_desc *desc)
> >  {
> > - uint8_t rev[2];
> > - int len;
> > + u32 base = intel_dp_is_branch(intel_dp) ? DP_BRANCH_OUI : DP_SINK_OUI;
> >
> > - if ((drm_debug & DRM_UT_KMS) == 0)
> > - return;
> > + return drm_dp_dpcd_read(&intel_dp->aux, base, desc, sizeof(*desc)) ==
> > +        sizeof(*desc);
>
> Starting to read either branch or sink oui should be a standalone prep
> change. I guess this should be done, although I've seen crappy devices
> that report oui in wrong place...

Ok, can make that a separate change.

>
> > +}
> >
> > - if (!(intel_dp->dpcd[DP_DOWNSTREAMPORT_PRESENT] &
> > -       DP_DWN_STRM_PORT_PRESENT))
> > - return;
> > +void
> > +intel_dp_print_desc(struct intel_dp *intel_dp, struct intel_dp_desc *desc)
> > +{
> > + const char *dev_type = intel_dp_is_branch(intel_dp) ? "branch" : "sink";
> > + bool oui_sup = intel_dp->dpcd[DP_DOWN_STREAM_PORT_COUNT] &
> > +        DP_OUI_SUPPORT;
> >
> > - len = drm_dp_dpcd_read(&intel_dp->aux, DP_BRANCH_SW_REV, &rev, 2);
> > - if (len < 0)
> > - return;
> > -
> > - DRM_DEBUG_KMS("sink sw revision: %d.%d\n", rev[0], rev[1]);
> > + DRM_DEBUG_KMS("DP %s: OUI %*phD%s dev-ID %.*s HW-rev %d.%d SW-rev %d.%d\n",
> > +       dev_type,
> > +       (int)sizeof(desc->oui), desc->oui, oui_sup ? "" : "(NS)",
> > +       (int)sizeof(desc->device_id), desc->device_id,
> > +       (desc->hw_rev & 0xf0) >> 4, (desc->hw_rev & 0xf),
> > +       desc->sw_major_rev, desc->sw_minor_rev);
>
> I've been thinking about starting to print this stuff too. But again,
> could be a standalone change.
>
> >  }
> >
> >  static int rate_to_index(int find, const int *rates)
> > @@ -3519,6 +3511,13 @@ intel_edp_init_dpcd(struct intel_dp *intel_dp)
> >   if (!intel_dp_read_dpcd(intel_dp))
> >   return false;
> >
> > + if (drm_debug & DRM_UT_KMS) {
> > + struct intel_dp_desc desc;
> > +
> > + if (intel_dp_read_desc(intel_dp, &desc))
> > + intel_dp_print_desc(intel_dp, &desc);
> > + }
>
> I *really* don't think we should do dpcd access conditional to drm
> debugs. I smell heisenbugs just thinking about it.

It's already conditional,
see intel_dp_print_hw_revision(), intel_dp_print_sw_revision(). But I
can make it unconditional.

>
> > +
> >   if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11)
> >   dev_priv->no_aux_handshake = intel_dp->dpcd[DP_MAX_DOWNSPREAD] &
> >   DP_NO_AUX_HANDSHAKE_LINK_TRAINING;
> > @@ -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)
> > -{
> > - u8 buf[3];
> > -
> > - if (!(intel_dp->dpcd[DP_DOWN_STREAM_PORT_COUNT] & DP_OUI_SUPPORT))
> > - return;
> > -
> > - if (drm_dp_dpcd_read(&intel_dp->aux, DP_SINK_OUI, buf, 3) == 3)
> > - DRM_DEBUG_KMS("Sink OUI: %02hx%02hx%02hx\n",
> > -       buf[0], buf[1], buf[2]);
> > -
> > - if (drm_dp_dpcd_read(&intel_dp->aux, DP_BRANCH_OUI, buf, 3) == 3)
> > - DRM_DEBUG_KMS("Branch OUI: %02hx%02hx%02hx\n",
> > -       buf[0], buf[1], buf[2]);
> > -}
> > -
> >  static bool
> >  intel_dp_can_mst(struct intel_dp *intel_dp)
> >  {
> > @@ -4410,11 +4392,12 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
> >         yesno(drm_dp_tps3_supported(intel_dp->dpcd)));
> >
> >   intel_dp_print_rates(intel_dp);
> > + if (drm_debug & DRM_UT_KMS) {
> > + struct intel_dp_desc desc;
> >
> > - intel_dp_probe_oui(intel_dp);
> > -
> > - intel_dp_print_hw_revision(intel_dp);
> > - intel_dp_print_sw_revision(intel_dp);
> > + if (intel_dp_read_desc(intel_dp, &desc))
> > + intel_dp_print_desc(intel_dp, &desc);
> > + }
>
> Ditto. I do like putting the desc reading into one place though.
>
> >
> >   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 c06a33e..a35e241 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;
> > +
>
> I understand this, but makes me wonder about dp helpers vs. i915. I
> guess we could add this for now.
>
> >  struct intel_dp {
> >   i915_reg_t output_reg;
> >   i915_reg_t aux_ch_ctl_reg;
> > @@ -1460,6 +1468,11 @@ static inline unsigned int intel_dp_unused_lane_mask(int lane_count)
> >   return ~((1 << lane_count) - 1) & 0xf;
> >  }
> >
> > +bool
> > +intel_dp_read_desc(struct intel_dp *intel_dp, struct intel_dp_desc *desc);
> > +void
> > +intel_dp_print_desc(struct intel_dp *intel_dp, struct intel_dp_desc *desc);
> > +
> >  /* intel_dp_aux_backlight.c */
> >  int intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector);
> >
> > diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c
> > index 632149c..d2c8cb2 100644
> > --- a/drivers/gpu/drm/i915/intel_lspcon.c
> > +++ b/drivers/gpu/drm/i915/intel_lspcon.c
> > @@ -131,6 +131,13 @@ bool lspcon_init(struct intel_digital_port *intel_dig_port)
> >   }
> >   }
> >
> > + if (drm_debug & DRM_UT_KMS) {
> > + struct intel_dp_desc desc;
> > +
> > + if (intel_dp_read_desc(dp, &desc))
> > + intel_dp_print_desc(dp, &desc);
> > + }
>
> Again, reads conditional to debugs is scary.
>
> > +
> >   DRM_DEBUG_KMS("Success: LSPCON init\n");
> >   return true;
> >  }