On Wed, Mar 27, 2024 at 01:40:58PM +0530, Nautiyal, Ankit K wrote:
>
> On 3/21/2024 1:41 AM, Imre Deak wrote:
> > The DPCD OUI of the logical port on a Dell UHBR monitor - on which the
> > AUX device is used to enable DSC - is all 0. To detect if the HBLANK
> > expansion quirk is required for this monitor use the OUI of the port's
> > parent instead.
> >
> > Since in the above case the DPCD of both the logical port and the parent
> > port reports being a sink device (vs. branch device) type, read the
> > proper sink/branch OUI based on the DPCD device type.
> >
> > This is required by a follow-up patch enabling the quirk for the above
> > Dell monitor.
> >
> > Signed-off-by: Imre Deak <imre.d...@intel.com>
> > ---
> > drivers/gpu/drm/i915/display/intel_dp_mst.c | 22 +++++++++++++++------
> > 1 file changed, 16 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > index 516b00f791420..76a8fb21b8e52 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > @@ -1512,23 +1512,33 @@
> > intel_dp_mst_read_decompression_port_dsc_caps(struct intel_dp *intel_dp,
> > static bool detect_dsc_hblank_expansion_quirk(const struct
> > intel_connector *connector)
> > {
> > struct drm_i915_private *i915 = to_i915(connector->base.dev);
> > + struct drm_dp_aux *aux = connector->dp.dsc_decompression_aux;
> > struct drm_dp_desc desc;
> > u8 dpcd[DP_RECEIVER_CAP_SIZE];
> > - if (!connector->dp.dsc_decompression_aux)
> > + if (!aux)
> > return false;
> > - if (drm_dp_read_desc(connector->dp.dsc_decompression_aux,
> > - &desc, true) < 0)
> > + /*
> > + * A logical port's OUI (at least for affected sinks) is all 0, so
> > + * instead of that the parent port's OUI is used for identification.
> > + */
> > + if (drm_dp_mst_port_is_logical(connector->port)) {
> > + aux = drm_dp_mst_aux_for_parent(connector->port);
> > + if (!aux)
> > + aux = &connector->mst_port->aux;
>
> As I understand, we are setting connector->mst_port as intel_dp, in the
> caller intel_dp_add_mst_connector so its unlikely that aux can be NULL, but
> do you see if we need to check for aux?
Yes, intel_connector::mst_port (always) points to the intel_dp of the
MST root port, and aux will be always initialized for all the registered
DP encoders/intel_dps; so mst_port->aux will always point to a
valid/non-NULL AUX device. (In any case above we take the address of
intel_dp::aux, which can't be NULL.)
> Regards,
>
> Ankit
>
> > + }
> > +
> > + if (drm_dp_read_dpcd_caps(aux, dpcd) < 0)
> > + return false;
> > +
> > + if (drm_dp_read_desc(aux, &desc, drm_dp_is_branch(dpcd)) < 0)
> > return false;
> > if (!drm_dp_has_quirk(&desc,
> > DP_DPCD_QUIRK_HBLANK_EXPANSION_REQUIRES_DSC))
> > return false;
> > - if (drm_dp_read_dpcd_caps(connector->dp.dsc_decompression_aux, dpcd) <
> > 0)
> > - return false;
> > -
> > if (!(dpcd[DP_RECEIVE_PORT_0_CAP_0] & DP_HBLANK_EXPANSION_CAPABLE))
> > return false;
