[PATCH 10/11] drm/i915/dp_mst: Make HBLANK expansion quirk work for logical ports

[Imre Deak]

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.deak at 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;