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

["Kulkarni, Vandita" <vandita.kulkarni@intel.com>]

> -----Original Message-----
> From: Nikula, Jani <jani.nikula@intel.com>
> Sent: Tuesday, November 2, 2021 3:13 PM
> To: Kulkarni, Vandita <vandita.kulkarni@intel.com>; intel-
> gfx@lists.freedesktop.org
> Cc: Deak, Imre <imre.deak@intel.com>; Roper, Matthew D
> <matthew.d.roper@intel.com>; ville.syrjala@linux.intel.com
> Subject: RE: [V2 4/4] drm/i915/dsi: Ungate clock before enabling the phy
> 
> On Tue, 02 Nov 2021, "Kulkarni, Vandita" <vandita.kulkarni@intel.com>
> wrote:
> >> -----Original Message-----
> >> From: Nikula, Jani <jani.nikula@intel.com>
> >> Sent: Monday, November 1, 2021 5:37 PM
> >> To: Kulkarni, Vandita <vandita.kulkarni@intel.com>; intel-
> >> gfx@lists.freedesktop.org
> >> Cc: Deak, Imre <imre.deak@intel.com>; Roper, Matthew D
> >> <matthew.d.roper@intel.com>; ville.syrjala@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@intel.com>
> >> wrote:
> >> >> -----Original Message-----
> >> >> From: Nikula, Jani <jani.nikula@intel.com>
> >> >> Sent: Thursday, October 28, 2021 8:06 PM
> >> >> To: Kulkarni, Vandita <vandita.kulkarni@intel.com>; intel-
> >> >> gfx@lists.freedesktop.org
> >> >> Cc: Deak, Imre <imre.deak@intel.com>; Roper, Matthew D
> >> >> <matthew.d.roper@intel.com>; ville.syrjala@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@intel.com>
> >> >> wrote:
> >> >> >> -----Original Message-----
> >> >> >> From: Nikula, Jani <jani.nikula@intel.com>
> >> >> >> Sent: Thursday, October 28, 2021 5:13 PM
> >> >> >> To: Kulkarni, Vandita <vandita.kulkarni@intel.com>; intel-
> >> >> >> gfx@lists.freedesktop.org
> >> >> >> Cc: Deak, Imre <imre.deak@intel.com>; Roper, Matthew D
> >> >> >> <matthew.d.roper@intel.com>; ville.syrjala@linux.intel.com;
> >> >> >> Kulkarni, Vandita <vandita.kulkarni@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@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@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")
> 
> I think if you remove the call to gen11_dsi_ungate_clocks(encoder) from this
> patch, the sequence matches bspec.
> 
> But this means the sequence is different between display 11 and 12+, and the
> clock will be gated for the entire enabling sequence on 12+. That's my
> reading of bspec, anyway.

Right the current bspec doesn't show us where to enable the clocks.
Clock ungating is not mentioned anywhere, and we need to enable clocks before enabling
DDI_BUF_CTL , have requested for sequence update in the bspec.

Thanks,
Vandita
> 
> BR,
> Jani.
> 
> 
> 
> >
> > 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
> 
> --
> Jani Nikula, Intel Open Source Graphics Center