On Tue, 2023-05-30 at 09:30 +0000, Kahola, Mika wrote:
> > -----Original Message-----
> > From: Luca Coelho <luca@xxxxxxxxx>
> > Sent: Tuesday, May 30, 2023 11:38 AM
> > To: Kahola, Mika <mika.kahola@xxxxxxxxx>; intel-
> > gfx@xxxxxxxxxxxxxxxxxxxxx
> > Subject: Re: [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@xxxxxxxxx>
> > > ---
> > > 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.
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.
