[Intel-gfx] [V2 4/4] drm/i915/dsi: Ungate clock before enabling the phy

Tue Nov 2 06:14:52 UTC 2021

> -----Original Message-----
> From: Nikula, Jani <jani.nikula at intel.com>
> Sent: Monday, November 1, 2021 5:37 PM
> To: Kulkarni, Vandita <vandita.kulkarni at intel.com>; intel-
> gfx at lists.freedesktop.org
> Cc: Deak, Imre <imre.deak at intel.com>; Roper, Matthew D
> <matthew.d.roper at intel.com>; ville.syrjala at linux.intel.com
> Subject: RE: [V2 4/4] drm/i915/dsi: Ungate clock before enabling the phy
> 
> On Thu, 28 Oct 2021, "Kulkarni, Vandita" <vandita.kulkarni at intel.com>
> wrote:
> >> -----Original Message-----
> >> From: Nikula, Jani <jani.nikula at intel.com>
> >> Sent: Thursday, October 28, 2021 8:06 PM
> >> To: Kulkarni, Vandita <vandita.kulkarni at intel.com>; intel-
> >> gfx at lists.freedesktop.org
> >> Cc: Deak, Imre <imre.deak at intel.com>; Roper, Matthew D
> >> <matthew.d.roper at intel.com>; ville.syrjala at linux.intel.com
> >> Subject: RE: [V2 4/4] drm/i915/dsi: Ungate clock before enabling the
> >> phy
> >>
> >> On Thu, 28 Oct 2021, "Kulkarni, Vandita" <vandita.kulkarni at intel.com>
> >> wrote:
> >> >> -----Original Message-----
> >> >> From: Nikula, Jani <jani.nikula at intel.com>
> >> >> Sent: Thursday, October 28, 2021 5:13 PM
> >> >> To: Kulkarni, Vandita <vandita.kulkarni at intel.com>; intel-
> >> >> gfx at lists.freedesktop.org
> >> >> Cc: Deak, Imre <imre.deak at intel.com>; Roper, Matthew D
> >> >> <matthew.d.roper at intel.com>; ville.syrjala at linux.intel.com;
> >> >> Kulkarni, Vandita <vandita.kulkarni at intel.com>
> >> >> Subject: Re: [V2 4/4] drm/i915/dsi: Ungate clock before enabling
> >> >> the phy
> >> >>
> >> >> On Tue, 19 Oct 2021, Vandita Kulkarni <vandita.kulkarni at intel.com>
> >> wrote:
> >> >> > For the PHY enable/disable signalling to propagate between
> >> >> > Dispaly and PHY, DDI clocks need to be running when enabling the
> PHY.
> >> >> >
> >> >> > Bspec: 49188 says gate the clocks after enabling the
> >> >> >        DDI Buffer.
> >> >> >        We also have a commit 991d9557b0c4 ("drm/i915/tgl/dsi:
> >> >> > Gate the
> >> ddi
> >> >> >        clocks after pll mapping") which gates the clocks much before,
> >> >> >        as per the older spec. This commit nullifies its effect and makes
> >> >> >        sure that the clocks are not gated while we enable the DDI
> >> >> >        buffer.
> >> >> > v2: Bspec ref, add a comment wrt earlier clock gating sequence
> >> >> > (Jani)
> >> >> >
> >> >> > Signed-off-by: Vandita Kulkarni <vandita.kulkarni at intel.com>
> >> >> > ---
> >> >> >  drivers/gpu/drm/i915/display/icl_dsi.c | 8 +++-----
> >> >> >  1 file changed, 3 insertions(+), 5 deletions(-)
> >> >> >
> >> >> > diff --git a/drivers/gpu/drm/i915/display/icl_dsi.c
> >> >> > b/drivers/gpu/drm/i915/display/icl_dsi.c
> >> >> > index 63dd75c6448a..e5ef5c4a32d7 100644
> >> >> > --- a/drivers/gpu/drm/i915/display/icl_dsi.c
> >> >> > +++ b/drivers/gpu/drm/i915/display/icl_dsi.c
> >> >> > @@ -1135,8 +1135,6 @@ static void
> >> >> > gen11_dsi_enable_port_and_phy(struct intel_encoder *encoder,
> >> >> >  			      const struct intel_crtc_state *crtc_state)
> {
> >> >> > -	struct drm_i915_private *dev_priv = to_i915(encoder-
> >base.dev);
> >> >> > -
> >> >> >  	/* step 4a: power up all lanes of the DDI used by DSI */
> >> >> >  	gen11_dsi_power_up_lanes(encoder);
> >> >> >
> >> >> > @@ -1146,6 +1144,8 @@ gen11_dsi_enable_port_and_phy(struct
> >> >> intel_encoder *encoder,
> >> >> >  	/* step 4c: configure voltage swing and skew */
> >> >> >  	gen11_dsi_voltage_swing_program_seq(encoder);
> >> >> >
> >> >> > +	gen11_dsi_ungate_clocks(encoder);
> >> >> > +
> >> >>
> >> >> What about the changes to gen11_dsi_map_pll() in commit
> >> >> 991d9557b0c4
> >> >> ("drm/i915/tgl/dsi: Gate the ddi clocks after pll mapping")? It
> >> >> starts off with clocks gated for gen12+, ungated otherwise.
> >> >
> >> > Now the same spec is updated with the gate step after ddi buffer
> enable.
> >> > And the one before is marked with remove tag.
> >> > That makes all gen12+ align with gen 11.
> >> > You suggested to update the same in the commit message on v1.
> >> > Should I still consider just reverting that commit?
> >>
> >> I'm just royally confused about the sequence myself, so I'm asking
> >> you. ;)
> >>
> >> It doesn't help that the code has step references to gen 11 mode set
> >> that are completely different from the steps in gen 12 sequence.
> >
> > Right, they have lot of different steps coming in.
> > As per gen11 sequence, we were gating pll after enabling ddi buffer.
> >
> > Initially when there was only gen12 in the bspec, it stated to gate the pll
> after mapping.
> > Hence we had that commit  991d9557b0c4.
> > Then Gen12's mipi dsi sequence was carried fwd for all later platforms as
> well.
> >  with the modification saying that
> > Do not gate the pll until we enable the ddi buffer.
> > And this applies to gen 12 as well because they have marked the
> > earlier mentioned step of gating pll after pll mapping as removed on all
> gen12 and later platforms.
> >
> > This patch now is keeping the older step as is, but making sure that clocks
> are ungated while enabling ddi buffer.
> 
> Where does it say for gen12+ that clocks should be ungated at any point?
> 
> My reading of the spec:
> 
> Gen11, bspec 20845 and 20597:
> - start with clocks ungated at mapping
> - gate after port/phy enabling (step 4m in gen11 mode set sequence)
> 
> Gen12, bspec 49204, 49188 and 55316:
> - start with clocks gated at mapping
> - gate *if* not already gated (steps 4c and 4i in gen12 mode set sequence)

Right the ungate step is not mentioned in the bspec.
Instead the step 4.c is marked as Removed.
I had added ungate just to make sure we are addressing the issue mentioned in front of removed tag while
Retaining the old sequence of 4.c

In order to completely adhere to the current bspec, I can
1. submit a patch removing 4.c
or
2.  submit a revert of the patch which was adding 4.c
("drm/i915/tgl/dsi: Gate the ddi clocks after pll mapping")

Thanks,
Vandita
> 
> It may be that your patch is correct, but IMO it does not match bspec.
> 
> 
> BR,
> Jani.
> 
> 
> 
> >
> > Thanks
> > Vandita
> >>
> >> BR,
> >> Jani.
> >>
> >>
> >>
> >> >
> >> > Thanks,
> >> > Vandita
> >> >
> >> >>
> >> >> BR,
> >> >> Jani.
> >> >>
> >> >>
> >> >> >  	/* enable DDI buffer */
> >> >> >  	gen11_dsi_enable_ddi_buffer(encoder);
> >> >> >
> >> >> > @@ -1161,9 +1161,7 @@ gen11_dsi_enable_port_and_phy(struct
> >> >> intel_encoder *encoder,
> >> >> >  	/* Step (4h, 4i, 4j, 4k): Configure transcoder */
> >> >> >  	gen11_dsi_configure_transcoder(encoder, crtc_state);
> >> >> >
> >> >> > -	/* Step 4l: Gate DDI clocks */
> >> >> > -	if (DISPLAY_VER(dev_priv) == 11)
> >> >> > -		gen11_dsi_gate_clocks(encoder);
> >> >> > +	gen11_dsi_gate_clocks(encoder);
> >> >> >  }
> >> >> >
> >> >> >  static void gen11_dsi_powerup_panel(struct intel_encoder
> >> >> > *encoder)
> >> >>
> >> >> --
> >> >> Jani Nikula, Intel Open Source Graphics Center
> >>
> >> --
> >> Jani Nikula, Intel Open Source Graphics Center
> 
> --
> Jani Nikula, Intel Open Source Graphics Center