[Intel-gfx] [PATCH] drm/i915/mtl: Reset only one lane in case of MFD

[Kahola, Mika]

> -----Original Message-----
> From: Luca Coelho <luca at coelho.fi>
> Sent: Tuesday, May 30, 2023 11:38 AM
> To: Kahola, Mika <mika.kahola at intel.com>; intel-gfx at lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH] drm/i915/mtl: Reset only one lane in case of MFD
> 
> Looks good, I only have some nitpicks.
> 
> On Wed, 2023-05-24 at 18:01 +0300, Mika Kahola wrote:
> > In case when only two or less lanes are owned such as MFD (DP-alt with
> > x2 lanes) we need to reset only one lane (lane0). With only x2 lanes
> > we don't need to poll for the phy current status on both lanes since
> > only the owned lane will respond.
> 
> It would be nice to explain why it is so.
> 
> 
> > Signed-off-by: Mika Kahola <mika.kahola at intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_cx0_phy.c | 39
> > ++++++++++++--------
> >  1 file changed, 23 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> > b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> > index ee6902118860..b8c812c5b33f 100644
> > --- a/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> > +++ b/drivers/gpu/drm/i915/display/intel_cx0_phy.c
> > @@ -2528,13 +2528,23 @@ static u32 intel_cx0_get_pclk_refclk_ack(u8 lane_mask)
> >  	return val;
> >  }
> >
> > -/* FIXME: Some Type-C cases need not reset both the lanes. Handle
> > those cases. */ -static void intel_cx0_phy_lane_reset(struct
> > drm_i915_private *i915, enum port port,
> > +static void intel_cx0_phy_lane_reset(struct drm_i915_private *i915,
> > +				     struct intel_encoder *encoder,
> >  				     bool lane_reversal)
> >  {
> > +	enum port port = encoder->port;
> >  	enum phy phy = intel_port_to_phy(i915, port);
> > +	u8 fia_max =
> > +intel_tc_port_fia_max_lane_count(enc_to_dig_port(encoder));
> 
> Logically, we don't care about "fia_max" in this function, we only care whether one or two lanes should be handled.  In all places
> we use fia_max, we only check if it is > 2.  So I think it would be clearer to have the > 2 here already and call the variable
> something else.
> 
> Additionally, "> 2" looks slightly magic (without knowing the reason, as stated in my first comment).  Is there any more self-
> explanatory symbol we could used?
I admit, it's not that clear when you look at it first time. It only means that all lanes are in use and we should in that case reset all lanes. Maybe switching to Boolean instead something like this

bool all_lanes = intel_tc_port_fia_max_lane_count(enc_to_dig_port(encoder)) > 2;

And do the reset routines based on this? 

> 
> >  	u8 lane_mask = lane_reversal ? INTEL_CX0_LANE1 :
> >  				  INTEL_CX0_LANE0;
> > +	u32 lane_pipe_reset = fia_max > 2 ?
> > +			      XELPDP_LANE_PIPE_RESET(0) |
> > +			      XELPDP_LANE_PIPE_RESET(1) :
> > +			      XELPDP_LANE_PIPE_RESET(0);
> > +	u32 lane_phy_current_status = fia_max > 2 ?
> > +				      XELPDP_LANE_PHY_CURRENT_STATUS(0) |
> > +				      XELPDP_LANE_PHY_CURRENT_STATUS(1) :
> > +				      XELPDP_LANE_PHY_CURRENT_STATUS(0);
> 
> It was already logically like this in the code, so not directly related to this patch, but is there any reason why we don't need to
> include more lanes in the reset? I mean, we're only handling lanes 0 and 1.  If we have 4 lanes, the other two never need to be
> reset?
There are lanes and then there are lanes. FIA has 4 main lanes with are muxed into 2 data lanes and here we only reset these data lanes. It's confusing as we have lanes defined for two different meanings.

Thanks for the review and comments!

-Mika-

> 
> --
> Cheers,
> Luca.