[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 1:08 PM
> 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
> 
> On Tue, 2023-05-30 at 09:30 +0000, Kahola, Mika wrote:
> > > -----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?
> 
> Sounds good.
> 
> 
> > > > 	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.
> 
> Okay, that clarifies it! We should probably have been calling them fia_lanes and data_lanes to distinguish, but make these
> changes now.
I will try to clarify these naming conventions. I spin another round of this patch which hopefully is more self-explanatory. 

-Mika-
> 
> This also clarifies why we reset only one or both data lanes.  A small paragraph explaining this would be nice in the commit log
> and/or as a comment in the function.
> 
> 
> > Thanks for the review and comments!
> 
> You're welcome! You're helping me learn. 😉
> 
> --
> Cheers,
> Luca.