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