Re: [Intel-gfx] [PATCH] drm/i915/display/: Refactor hsw_crtc_enable for bigjoiner cleanup
On Tue, Mar 29, 2022 at 05:00:39PM -0700, Navare, Manasi wrote: > Hi Ville, > > I was looking at your suggestion of extracting the per pipe stuff out. > Currently in hsw_crtc_enable: the Only non per pipe stuff which gets enabled > for the encoders is : > encoder specific is pre_pll_enable(), enable_shared_dpll, encoders_pre_enable > and configure_cpu_transcoder() - All of this > can be put in a function hsw_encoder_configure() or something that can still > be called from with hsw_crtc_enable I don't see a need to move that stuff anywhere. Just leave it in hsw_crtc_enable() IMO. > > Then the remaining can go into a per pipe function that can be called for > each slave pipe This is what I have been suggesting. But I think there's also some per-pipe stuff currently in the the encoder hooks, and some of that is only done for the master there whereaas the slave part I think are somewhere else. We should attempt to fix that as well so that every step is done in a consistent manner for every pipe be it master or slave. -- Ville Syrjälä Intel
Re: [Intel-gfx] [PATCH] drm/i915/display/: Refactor hsw_crtc_enable for bigjoiner cleanup
Hi Ville,
I was looking at your suggestion of extracting the per pipe stuff out.
Currently in hsw_crtc_enable: the Only non per pipe stuff which gets enabled
for the encoders is :
encoder specific is pre_pll_enable(), enable_shared_dpll, encoders_pre_enable
and configure_cpu_transcoder() - All of this
can be put in a function hsw_encoder_configure() or something that can still be
called from with hsw_crtc_enable
Then the remaining can go into a per pipe function that can be called for each
slave pipe
But it means still pretty much splitting the current hsw_crtc_enable into 2
separate functions
Does this refactoring make sense?
Manasi
On Thu, Mar 17, 2022 at 09:14:16PM +0200, Ville Syrjälä wrote:
> On Thu, Mar 17, 2022 at 12:05:47PM -0700, Navare, Manasi wrote:
> > On Thu, Mar 17, 2022 at 08:52:52PM +0200, Ville Syrjälä wrote:
> > > On Tue, Mar 15, 2022 at 04:38:56PM -0700, Manasi Navare wrote:
> > > > This patch abstracts pieces of hsw_crtc_enable corresponding to
> > > > different
> > > > Bspec enable sequence steps into separate functions.
> > > > This helps to call them in a specific order for bigjoiner master/slave
> > > > in a cleaner fashion.
> > > >
> > > > Cc: Ville Syrjälä
> > > > Cc: Animesh Manna
> > > > Signed-off-by: Manasi Navare
> > > > ---
> > > > drivers/gpu/drm/i915/display/intel_display.c | 125 ++-
> > > > 1 file changed, 66 insertions(+), 59 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > > > b/drivers/gpu/drm/i915/display/intel_display.c
> > > > index eb49973621f0..d8e6466c9fa0 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > > @@ -1865,24 +1865,6 @@ static void hsw_set_frame_start_delay(const
> > > > struct intel_crtc_state *crtc_state)
> > > > intel_de_write(dev_priv, reg, val);
> > > > }
> > > >
> > > > -static void icl_ddi_bigjoiner_pre_enable(struct intel_atomic_state
> > > > *state,
> > > > -const struct intel_crtc_state
> > > > *crtc_state)
> > > > -{
> > > > - struct intel_crtc *master_crtc = intel_master_crtc(crtc_state);
> > > > -
> > > > - /*
> > > > -* Enable sequence steps 1-7 on bigjoiner master
> > > > -*/
> > > > - if (intel_crtc_is_bigjoiner_slave(crtc_state))
> > > > - intel_encoders_pre_pll_enable(state, master_crtc);
> > > > -
> > > > - if (crtc_state->shared_dpll)
> > > > - intel_enable_shared_dpll(crtc_state);
> > > > -
> > > > - if (intel_crtc_is_bigjoiner_slave(crtc_state))
> > > > - intel_encoders_pre_enable(state, master_crtc);
> > > > -}
> > > > -
> > > > static void hsw_configure_cpu_transcoder(const struct intel_crtc_state
> > > > *crtc_state)
> > > > {
> > > > struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > > > @@ -1910,70 +1892,73 @@ static void hsw_configure_cpu_transcoder(const
> > > > struct intel_crtc_state *crtc_sta
> > > > hsw_set_transconf(crtc_state);
> > > > }
> > > >
> > > > -static void hsw_crtc_enable(struct intel_atomic_state *state,
> > > > - struct intel_crtc *crtc)
> > > > +static void hsw_crtc_pre_pll_enable(struct intel_atomic_state *state,
> > > > + const struct intel_crtc_state
> > > > *crtc_state)
> > > > {
> > > > - const struct intel_crtc_state *new_crtc_state =
> > > > - intel_atomic_get_new_crtc_state(state, crtc);
> > > > - struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > > > - enum pipe pipe = crtc->pipe, hsw_workaround_pipe;
> > > > - enum transcoder cpu_transcoder = new_crtc_state->cpu_transcoder;
> > > > - bool psl_clkgate_wa;
> > > > -
> > > > - if (drm_WARN_ON(_priv->drm, crtc->active))
> > > > - return;
> > > > + struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > > >
> > > > - if (!new_crtc_state->bigjoiner_pipes) {
> > > > - intel_encoders_pre_pll_enable(state, crtc);
> > > > + /*
> > > > +* Enable sequence steps 1 - 7 on all pipes
> > > > +*/
> > > > + intel_encoders_pre_pll_enable(state, crtc);
> > > > + if (crtc_state->shared_dpll)
> > > > + intel_enable_shared_dpll(crtc_state);
> > > >
> > > > - if (new_crtc_state->shared_dpll)
> > > > - intel_enable_shared_dpll(new_crtc_state);
> > > > + intel_encoders_pre_enable(state, crtc);
> > > > +}
> > > >
> > > > - intel_encoders_pre_enable(state, crtc);
> > > > - } else {
> > > > - icl_ddi_bigjoiner_pre_enable(state, new_crtc_state);
> > > > - }
> > > > +static void hsw_crtc_post_pll_enable(struct intel_atomic_state *state,
> > > > +const struct intel_crtc_state
> > > > *crtc_state)
Re: [Intel-gfx] [PATCH] drm/i915/display/: Refactor hsw_crtc_enable for bigjoiner cleanup
On Thu, 17 Mar 2022, "Navare, Manasi" wrote:
> On Thu, Mar 17, 2022 at 11:28:03AM -0700, Navare, Manasi wrote:
>> On Thu, Mar 17, 2022 at 05:35:43PM +0200, Jani Nikula wrote:
>> > On Wed, 16 Mar 2022, "Navare, Manasi" wrote:
>> > > On Wed, Mar 16, 2022 at 09:48:17AM +0200, Jani Nikula wrote:
>> > >> On Tue, 15 Mar 2022, Manasi Navare wrote:
>> > >> > This patch abstracts pieces of hsw_crtc_enable corresponding to
>> > >> > different
>> > >> > Bspec enable sequence steps into separate functions.
>> > >> > This helps to call them in a specific order for bigjoiner master/slave
>> > >> > in a cleaner fashion.
>> > >>
>> > >> So does this contain only refactoring or functional changes also? One or
>> > >> the other at a time, please, not both.
>> > >
>> > > No this is only refactor, no functional changes here
>> > >
>> > >>
>> > >> Also looks like hsw_crtc_* have accumulated just way too many checks for
>> > >> platforms instead of having a clean break at e.g. display 9 and/or 11.
>> > >
>> > > These checks were already part of hsw_crtc_enable() function that I have
>> > > just moved to a separate
>> > > function. How do you recommend removing these checks?
>> >
>> > We use hsw_crtc_enable for all DDI platforms and later. We do have the
>> > difference between skl_display_funcs and ddi_display_funcs, but they
>> > both point to hsw_crtc_enable. Maybe it's time for them to have their
>> > own functions that don't have to take so many platform differences into
>> > account.
>>
>> Yes I could perhaps have a separate skl_crtc_enable() function which would
>> then have all parts that apply to DISPLAy >=9
>> everything else can stay in hsw_crtc_enable
>>
>> That makes sense?
>
> Or actually we could have a separate one for >=11 starting ICL, since there
> are a lot of
> checks for >=11. Also bigjoiner support only from ICL, so then this bigjoiner
> sequence then only becomes part
> of icl_crtc_enable()
> Which one of the above 2 do you recommend?
Could be both! Depends on the improved clarity vs. amount of code
duplication.
BR,
Jani.
>
> Manasi
>
>>
>> Manasi
>>
>> >
>> > BR,
>> > Jani.
>> >
>> >
>> >
>> >
>> > >
>> > >>
>> > >> Adding references to "enable sequence step 8" is not helpful because
>> > >> it's platform specific. (Yeah, I know there are existing references like
>> > >> this, but they're also suspect.)
>> > >
>> > > Yes agree, may be I will add the comment for what actually the step 7/8
>> > > does?
>> > >
>> > > Manasi
>> > >
>> > >>
>> > >>
>> > >> BR,
>> > >> Jani.
>> > >>
>> > >>
>> > >> >
>> > >> > Cc: Ville Syrjälä
>> > >> > Cc: Animesh Manna
>> > >> > Signed-off-by: Manasi Navare
>> > >> > ---
>> > >> > drivers/gpu/drm/i915/display/intel_display.c | 125
>> > >> > ++-
>> > >> > 1 file changed, 66 insertions(+), 59 deletions(-)
>> > >> >
>> > >> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
>> > >> > b/drivers/gpu/drm/i915/display/intel_display.c
>> > >> > index eb49973621f0..d8e6466c9fa0 100644
>> > >> > --- a/drivers/gpu/drm/i915/display/intel_display.c
>> > >> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
>> > >> > @@ -1865,24 +1865,6 @@ static void hsw_set_frame_start_delay(const
>> > >> > struct intel_crtc_state *crtc_state)
>> > >> > intel_de_write(dev_priv, reg, val);
>> > >> > }
>> > >> >
>> > >> > -static void icl_ddi_bigjoiner_pre_enable(struct intel_atomic_state
>> > >> > *state,
>> > >> > - const struct intel_crtc_state
>> > >> > *crtc_state)
>> > >> > -{
>> > >> > - struct intel_crtc *master_crtc = intel_master_crtc(crtc_state);
>> > >> > -
>> > >> > - /*
>> > >> > - * Enable sequence steps 1-7 on bigjoiner master
>> > >> > - */
>> > >> > - if (intel_crtc_is_bigjoiner_slave(crtc_state))
>> > >> > - intel_encoders_pre_pll_enable(state, master_crtc);
>> > >> > -
>> > >> > - if (crtc_state->shared_dpll)
>> > >> > - intel_enable_shared_dpll(crtc_state);
>> > >> > -
>> > >> > - if (intel_crtc_is_bigjoiner_slave(crtc_state))
>> > >> > - intel_encoders_pre_enable(state, master_crtc);
>> > >> > -}
>> > >> > -
>> > >> > static void hsw_configure_cpu_transcoder(const struct
>> > >> > intel_crtc_state *crtc_state)
>> > >> > {
>> > >> > struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>> > >> > @@ -1910,70 +1892,73 @@ static void
>> > >> > hsw_configure_cpu_transcoder(const struct intel_crtc_state *crtc_sta
>> > >> > hsw_set_transconf(crtc_state);
>> > >> > }
>> > >> >
>> > >> > -static void hsw_crtc_enable(struct intel_atomic_state *state,
>> > >> > - struct intel_crtc *crtc)
>> > >> > +static void hsw_crtc_pre_pll_enable(struct intel_atomic_state *state,
>> > >> > + const struct intel_crtc_state
>> > >> > *crtc_state)
>> > >> > {
>> > >> > - const struct intel_crtc_state *new_crtc_state =
>> > >> > -
Re: [Intel-gfx] [PATCH] drm/i915/display/: Refactor hsw_crtc_enable for bigjoiner cleanup
On Thu, Mar 17, 2022 at 12:05:47PM -0700, Navare, Manasi wrote:
> On Thu, Mar 17, 2022 at 08:52:52PM +0200, Ville Syrjälä wrote:
> > On Tue, Mar 15, 2022 at 04:38:56PM -0700, Manasi Navare wrote:
> > > This patch abstracts pieces of hsw_crtc_enable corresponding to different
> > > Bspec enable sequence steps into separate functions.
> > > This helps to call them in a specific order for bigjoiner master/slave
> > > in a cleaner fashion.
> > >
> > > Cc: Ville Syrjälä
> > > Cc: Animesh Manna
> > > Signed-off-by: Manasi Navare
> > > ---
> > > drivers/gpu/drm/i915/display/intel_display.c | 125 ++-
> > > 1 file changed, 66 insertions(+), 59 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > > b/drivers/gpu/drm/i915/display/intel_display.c
> > > index eb49973621f0..d8e6466c9fa0 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > @@ -1865,24 +1865,6 @@ static void hsw_set_frame_start_delay(const struct
> > > intel_crtc_state *crtc_state)
> > > intel_de_write(dev_priv, reg, val);
> > > }
> > >
> > > -static void icl_ddi_bigjoiner_pre_enable(struct intel_atomic_state
> > > *state,
> > > - const struct intel_crtc_state
> > > *crtc_state)
> > > -{
> > > - struct intel_crtc *master_crtc = intel_master_crtc(crtc_state);
> > > -
> > > - /*
> > > - * Enable sequence steps 1-7 on bigjoiner master
> > > - */
> > > - if (intel_crtc_is_bigjoiner_slave(crtc_state))
> > > - intel_encoders_pre_pll_enable(state, master_crtc);
> > > -
> > > - if (crtc_state->shared_dpll)
> > > - intel_enable_shared_dpll(crtc_state);
> > > -
> > > - if (intel_crtc_is_bigjoiner_slave(crtc_state))
> > > - intel_encoders_pre_enable(state, master_crtc);
> > > -}
> > > -
> > > static void hsw_configure_cpu_transcoder(const struct intel_crtc_state
> > > *crtc_state)
> > > {
> > > struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > > @@ -1910,70 +1892,73 @@ static void hsw_configure_cpu_transcoder(const
> > > struct intel_crtc_state *crtc_sta
> > > hsw_set_transconf(crtc_state);
> > > }
> > >
> > > -static void hsw_crtc_enable(struct intel_atomic_state *state,
> > > - struct intel_crtc *crtc)
> > > +static void hsw_crtc_pre_pll_enable(struct intel_atomic_state *state,
> > > + const struct intel_crtc_state *crtc_state)
> > > {
> > > - const struct intel_crtc_state *new_crtc_state =
> > > - intel_atomic_get_new_crtc_state(state, crtc);
> > > - struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > > - enum pipe pipe = crtc->pipe, hsw_workaround_pipe;
> > > - enum transcoder cpu_transcoder = new_crtc_state->cpu_transcoder;
> > > - bool psl_clkgate_wa;
> > > -
> > > - if (drm_WARN_ON(_priv->drm, crtc->active))
> > > - return;
> > > + struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > >
> > > - if (!new_crtc_state->bigjoiner_pipes) {
> > > - intel_encoders_pre_pll_enable(state, crtc);
> > > + /*
> > > + * Enable sequence steps 1 - 7 on all pipes
> > > + */
> > > + intel_encoders_pre_pll_enable(state, crtc);
> > > + if (crtc_state->shared_dpll)
> > > + intel_enable_shared_dpll(crtc_state);
> > >
> > > - if (new_crtc_state->shared_dpll)
> > > - intel_enable_shared_dpll(new_crtc_state);
> > > + intel_encoders_pre_enable(state, crtc);
> > > +}
> > >
> > > - intel_encoders_pre_enable(state, crtc);
> > > - } else {
> > > - icl_ddi_bigjoiner_pre_enable(state, new_crtc_state);
> > > - }
> > > +static void hsw_crtc_post_pll_enable(struct intel_atomic_state *state,
> > > + const struct intel_crtc_state *crtc_state)
> > > +{
> > > + struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > > + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > > + enum pipe pipe = crtc->pipe, hsw_workaround_pipe;
> > > + enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
> > > + bool psl_clkgate_wa;
> > >
> > > - intel_dsc_enable(new_crtc_state);
> > > + /*
> > > + * Enable sequence step 8
> > > + */
> > > + intel_dsc_enable(crtc_state);
> > >
> > > if (DISPLAY_VER(dev_priv) >= 13)
> > > - intel_uncompressed_joiner_enable(new_crtc_state);
> > > + intel_uncompressed_joiner_enable(crtc_state);
> > >
> > > - intel_set_pipe_src_size(new_crtc_state);
> > > + intel_set_pipe_src_size(crtc_state);
> > > if (DISPLAY_VER(dev_priv) >= 9 || IS_BROADWELL(dev_priv))
> > > - bdw_set_pipemisc(new_crtc_state);
> > > + bdw_set_pipemisc(crtc_state);
> > >
> > > - if (!intel_crtc_is_bigjoiner_slave(new_crtc_state) &&
> > > + if (!intel_crtc_is_bigjoiner_slave(crtc_state) &&
> > > !transcoder_is_dsi(cpu_transcoder))
> > > - hsw_configure_cpu_transcoder(new_crtc_state);
> > > +
Re: [Intel-gfx] [PATCH] drm/i915/display/: Refactor hsw_crtc_enable for bigjoiner cleanup
On Thu, Mar 17, 2022 at 08:52:52PM +0200, Ville Syrjälä wrote:
> On Tue, Mar 15, 2022 at 04:38:56PM -0700, Manasi Navare wrote:
> > This patch abstracts pieces of hsw_crtc_enable corresponding to different
> > Bspec enable sequence steps into separate functions.
> > This helps to call them in a specific order for bigjoiner master/slave
> > in a cleaner fashion.
> >
> > Cc: Ville Syrjälä
> > Cc: Animesh Manna
> > Signed-off-by: Manasi Navare
> > ---
> > drivers/gpu/drm/i915/display/intel_display.c | 125 ++-
> > 1 file changed, 66 insertions(+), 59 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index eb49973621f0..d8e6466c9fa0 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -1865,24 +1865,6 @@ static void hsw_set_frame_start_delay(const struct
> > intel_crtc_state *crtc_state)
> > intel_de_write(dev_priv, reg, val);
> > }
> >
> > -static void icl_ddi_bigjoiner_pre_enable(struct intel_atomic_state *state,
> > -const struct intel_crtc_state
> > *crtc_state)
> > -{
> > - struct intel_crtc *master_crtc = intel_master_crtc(crtc_state);
> > -
> > - /*
> > -* Enable sequence steps 1-7 on bigjoiner master
> > -*/
> > - if (intel_crtc_is_bigjoiner_slave(crtc_state))
> > - intel_encoders_pre_pll_enable(state, master_crtc);
> > -
> > - if (crtc_state->shared_dpll)
> > - intel_enable_shared_dpll(crtc_state);
> > -
> > - if (intel_crtc_is_bigjoiner_slave(crtc_state))
> > - intel_encoders_pre_enable(state, master_crtc);
> > -}
> > -
> > static void hsw_configure_cpu_transcoder(const struct intel_crtc_state
> > *crtc_state)
> > {
> > struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > @@ -1910,70 +1892,73 @@ static void hsw_configure_cpu_transcoder(const
> > struct intel_crtc_state *crtc_sta
> > hsw_set_transconf(crtc_state);
> > }
> >
> > -static void hsw_crtc_enable(struct intel_atomic_state *state,
> > - struct intel_crtc *crtc)
> > +static void hsw_crtc_pre_pll_enable(struct intel_atomic_state *state,
> > + const struct intel_crtc_state *crtc_state)
> > {
> > - const struct intel_crtc_state *new_crtc_state =
> > - intel_atomic_get_new_crtc_state(state, crtc);
> > - struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > - enum pipe pipe = crtc->pipe, hsw_workaround_pipe;
> > - enum transcoder cpu_transcoder = new_crtc_state->cpu_transcoder;
> > - bool psl_clkgate_wa;
> > -
> > - if (drm_WARN_ON(_priv->drm, crtc->active))
> > - return;
> > + struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> >
> > - if (!new_crtc_state->bigjoiner_pipes) {
> > - intel_encoders_pre_pll_enable(state, crtc);
> > + /*
> > +* Enable sequence steps 1 - 7 on all pipes
> > +*/
> > + intel_encoders_pre_pll_enable(state, crtc);
> > + if (crtc_state->shared_dpll)
> > + intel_enable_shared_dpll(crtc_state);
> >
> > - if (new_crtc_state->shared_dpll)
> > - intel_enable_shared_dpll(new_crtc_state);
> > + intel_encoders_pre_enable(state, crtc);
> > +}
> >
> > - intel_encoders_pre_enable(state, crtc);
> > - } else {
> > - icl_ddi_bigjoiner_pre_enable(state, new_crtc_state);
> > - }
> > +static void hsw_crtc_post_pll_enable(struct intel_atomic_state *state,
> > +const struct intel_crtc_state *crtc_state)
> > +{
> > + struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > + enum pipe pipe = crtc->pipe, hsw_workaround_pipe;
> > + enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
> > + bool psl_clkgate_wa;
> >
> > - intel_dsc_enable(new_crtc_state);
> > + /*
> > +* Enable sequence step 8
> > +*/
> > + intel_dsc_enable(crtc_state);
> >
> > if (DISPLAY_VER(dev_priv) >= 13)
> > - intel_uncompressed_joiner_enable(new_crtc_state);
> > + intel_uncompressed_joiner_enable(crtc_state);
> >
> > - intel_set_pipe_src_size(new_crtc_state);
> > + intel_set_pipe_src_size(crtc_state);
> > if (DISPLAY_VER(dev_priv) >= 9 || IS_BROADWELL(dev_priv))
> > - bdw_set_pipemisc(new_crtc_state);
> > + bdw_set_pipemisc(crtc_state);
> >
> > - if (!intel_crtc_is_bigjoiner_slave(new_crtc_state) &&
> > + if (!intel_crtc_is_bigjoiner_slave(crtc_state) &&
> > !transcoder_is_dsi(cpu_transcoder))
> > - hsw_configure_cpu_transcoder(new_crtc_state);
> > + hsw_configure_cpu_transcoder(crtc_state);
> >
> > crtc->active = true;
> >
> > /* Display WA #1180: WaDisableScalarClockGating: glk */
> > psl_clkgate_wa = DISPLAY_VER(dev_priv) == 10
Re: [Intel-gfx] [PATCH] drm/i915/display/: Refactor hsw_crtc_enable for bigjoiner cleanup
On Tue, Mar 15, 2022 at 04:38:56PM -0700, Manasi Navare wrote:
> This patch abstracts pieces of hsw_crtc_enable corresponding to different
> Bspec enable sequence steps into separate functions.
> This helps to call them in a specific order for bigjoiner master/slave
> in a cleaner fashion.
>
> Cc: Ville Syrjälä
> Cc: Animesh Manna
> Signed-off-by: Manasi Navare
> ---
> drivers/gpu/drm/i915/display/intel_display.c | 125 ++-
> 1 file changed, 66 insertions(+), 59 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> b/drivers/gpu/drm/i915/display/intel_display.c
> index eb49973621f0..d8e6466c9fa0 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -1865,24 +1865,6 @@ static void hsw_set_frame_start_delay(const struct
> intel_crtc_state *crtc_state)
> intel_de_write(dev_priv, reg, val);
> }
>
> -static void icl_ddi_bigjoiner_pre_enable(struct intel_atomic_state *state,
> - const struct intel_crtc_state
> *crtc_state)
> -{
> - struct intel_crtc *master_crtc = intel_master_crtc(crtc_state);
> -
> - /*
> - * Enable sequence steps 1-7 on bigjoiner master
> - */
> - if (intel_crtc_is_bigjoiner_slave(crtc_state))
> - intel_encoders_pre_pll_enable(state, master_crtc);
> -
> - if (crtc_state->shared_dpll)
> - intel_enable_shared_dpll(crtc_state);
> -
> - if (intel_crtc_is_bigjoiner_slave(crtc_state))
> - intel_encoders_pre_enable(state, master_crtc);
> -}
> -
> static void hsw_configure_cpu_transcoder(const struct intel_crtc_state
> *crtc_state)
> {
> struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> @@ -1910,70 +1892,73 @@ static void hsw_configure_cpu_transcoder(const struct
> intel_crtc_state *crtc_sta
> hsw_set_transconf(crtc_state);
> }
>
> -static void hsw_crtc_enable(struct intel_atomic_state *state,
> - struct intel_crtc *crtc)
> +static void hsw_crtc_pre_pll_enable(struct intel_atomic_state *state,
> + const struct intel_crtc_state *crtc_state)
> {
> - const struct intel_crtc_state *new_crtc_state =
> - intel_atomic_get_new_crtc_state(state, crtc);
> - struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> - enum pipe pipe = crtc->pipe, hsw_workaround_pipe;
> - enum transcoder cpu_transcoder = new_crtc_state->cpu_transcoder;
> - bool psl_clkgate_wa;
> -
> - if (drm_WARN_ON(_priv->drm, crtc->active))
> - return;
> + struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>
> - if (!new_crtc_state->bigjoiner_pipes) {
> - intel_encoders_pre_pll_enable(state, crtc);
> + /*
> + * Enable sequence steps 1 - 7 on all pipes
> + */
> + intel_encoders_pre_pll_enable(state, crtc);
> + if (crtc_state->shared_dpll)
> + intel_enable_shared_dpll(crtc_state);
>
> - if (new_crtc_state->shared_dpll)
> - intel_enable_shared_dpll(new_crtc_state);
> + intel_encoders_pre_enable(state, crtc);
> +}
>
> - intel_encoders_pre_enable(state, crtc);
> - } else {
> - icl_ddi_bigjoiner_pre_enable(state, new_crtc_state);
> - }
> +static void hsw_crtc_post_pll_enable(struct intel_atomic_state *state,
> + const struct intel_crtc_state *crtc_state)
> +{
> + struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> + enum pipe pipe = crtc->pipe, hsw_workaround_pipe;
> + enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
> + bool psl_clkgate_wa;
>
> - intel_dsc_enable(new_crtc_state);
> + /*
> + * Enable sequence step 8
> + */
> + intel_dsc_enable(crtc_state);
>
> if (DISPLAY_VER(dev_priv) >= 13)
> - intel_uncompressed_joiner_enable(new_crtc_state);
> + intel_uncompressed_joiner_enable(crtc_state);
>
> - intel_set_pipe_src_size(new_crtc_state);
> + intel_set_pipe_src_size(crtc_state);
> if (DISPLAY_VER(dev_priv) >= 9 || IS_BROADWELL(dev_priv))
> - bdw_set_pipemisc(new_crtc_state);
> + bdw_set_pipemisc(crtc_state);
>
> - if (!intel_crtc_is_bigjoiner_slave(new_crtc_state) &&
> + if (!intel_crtc_is_bigjoiner_slave(crtc_state) &&
> !transcoder_is_dsi(cpu_transcoder))
> - hsw_configure_cpu_transcoder(new_crtc_state);
> + hsw_configure_cpu_transcoder(crtc_state);
>
> crtc->active = true;
>
> /* Display WA #1180: WaDisableScalarClockGating: glk */
> psl_clkgate_wa = DISPLAY_VER(dev_priv) == 10 &&
> - new_crtc_state->pch_pfit.enabled;
> + crtc_state->pch_pfit.enabled;
> if (psl_clkgate_wa)
>
Re: [Intel-gfx] [PATCH] drm/i915/display/: Refactor hsw_crtc_enable for bigjoiner cleanup
On Thu, Mar 17, 2022 at 11:28:03AM -0700, Navare, Manasi wrote:
> On Thu, Mar 17, 2022 at 05:35:43PM +0200, Jani Nikula wrote:
> > On Wed, 16 Mar 2022, "Navare, Manasi" wrote:
> > > On Wed, Mar 16, 2022 at 09:48:17AM +0200, Jani Nikula wrote:
> > >> On Tue, 15 Mar 2022, Manasi Navare wrote:
> > >> > This patch abstracts pieces of hsw_crtc_enable corresponding to
> > >> > different
> > >> > Bspec enable sequence steps into separate functions.
> > >> > This helps to call them in a specific order for bigjoiner master/slave
> > >> > in a cleaner fashion.
> > >>
> > >> So does this contain only refactoring or functional changes also? One or
> > >> the other at a time, please, not both.
> > >
> > > No this is only refactor, no functional changes here
> > >
> > >>
> > >> Also looks like hsw_crtc_* have accumulated just way too many checks for
> > >> platforms instead of having a clean break at e.g. display 9 and/or 11.
> > >
> > > These checks were already part of hsw_crtc_enable() function that I have
> > > just moved to a separate
> > > function. How do you recommend removing these checks?
> >
> > We use hsw_crtc_enable for all DDI platforms and later. We do have the
> > difference between skl_display_funcs and ddi_display_funcs, but they
> > both point to hsw_crtc_enable. Maybe it's time for them to have their
> > own functions that don't have to take so many platform differences into
> > account.
>
> Yes I could perhaps have a separate skl_crtc_enable() function which would
> then have all parts that apply to DISPLAy >=9
> everything else can stay in hsw_crtc_enable
>
> That makes sense?
Or actually we could have a separate one for >=11 starting ICL, since there are
a lot of
checks for >=11. Also bigjoiner support only from ICL, so then this bigjoiner
sequence then only becomes part
of icl_crtc_enable()
Which one of the above 2 do you recommend?
Manasi
>
> Manasi
>
> >
> > BR,
> > Jani.
> >
> >
> >
> >
> > >
> > >>
> > >> Adding references to "enable sequence step 8" is not helpful because
> > >> it's platform specific. (Yeah, I know there are existing references like
> > >> this, but they're also suspect.)
> > >
> > > Yes agree, may be I will add the comment for what actually the step 7/8
> > > does?
> > >
> > > Manasi
> > >
> > >>
> > >>
> > >> BR,
> > >> Jani.
> > >>
> > >>
> > >> >
> > >> > Cc: Ville Syrjälä
> > >> > Cc: Animesh Manna
> > >> > Signed-off-by: Manasi Navare
> > >> > ---
> > >> > drivers/gpu/drm/i915/display/intel_display.c | 125 ++-
> > >> > 1 file changed, 66 insertions(+), 59 deletions(-)
> > >> >
> > >> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > >> > b/drivers/gpu/drm/i915/display/intel_display.c
> > >> > index eb49973621f0..d8e6466c9fa0 100644
> > >> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > >> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > >> > @@ -1865,24 +1865,6 @@ static void hsw_set_frame_start_delay(const
> > >> > struct intel_crtc_state *crtc_state)
> > >> >intel_de_write(dev_priv, reg, val);
> > >> > }
> > >> >
> > >> > -static void icl_ddi_bigjoiner_pre_enable(struct intel_atomic_state
> > >> > *state,
> > >> > - const struct intel_crtc_state
> > >> > *crtc_state)
> > >> > -{
> > >> > - struct intel_crtc *master_crtc = intel_master_crtc(crtc_state);
> > >> > -
> > >> > - /*
> > >> > - * Enable sequence steps 1-7 on bigjoiner master
> > >> > - */
> > >> > - if (intel_crtc_is_bigjoiner_slave(crtc_state))
> > >> > - intel_encoders_pre_pll_enable(state, master_crtc);
> > >> > -
> > >> > - if (crtc_state->shared_dpll)
> > >> > - intel_enable_shared_dpll(crtc_state);
> > >> > -
> > >> > - if (intel_crtc_is_bigjoiner_slave(crtc_state))
> > >> > - intel_encoders_pre_enable(state, master_crtc);
> > >> > -}
> > >> > -
> > >> > static void hsw_configure_cpu_transcoder(const struct
> > >> > intel_crtc_state *crtc_state)
> > >> > {
> > >> >struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > >> > @@ -1910,70 +1892,73 @@ static void hsw_configure_cpu_transcoder(const
> > >> > struct intel_crtc_state *crtc_sta
> > >> >hsw_set_transconf(crtc_state);
> > >> > }
> > >> >
> > >> > -static void hsw_crtc_enable(struct intel_atomic_state *state,
> > >> > - struct intel_crtc *crtc)
> > >> > +static void hsw_crtc_pre_pll_enable(struct intel_atomic_state *state,
> > >> > + const struct intel_crtc_state
> > >> > *crtc_state)
> > >> > {
> > >> > - const struct intel_crtc_state *new_crtc_state =
> > >> > - intel_atomic_get_new_crtc_state(state, crtc);
> > >> > - struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > >> > - enum pipe pipe = crtc->pipe, hsw_workaround_pipe;
> > >> > - enum transcoder cpu_transcoder = new_crtc_state->cpu_transcoder;
Re: [Intel-gfx] [PATCH] drm/i915/display/: Refactor hsw_crtc_enable for bigjoiner cleanup
On Thu, Mar 17, 2022 at 05:35:43PM +0200, Jani Nikula wrote:
> On Wed, 16 Mar 2022, "Navare, Manasi" wrote:
> > On Wed, Mar 16, 2022 at 09:48:17AM +0200, Jani Nikula wrote:
> >> On Tue, 15 Mar 2022, Manasi Navare wrote:
> >> > This patch abstracts pieces of hsw_crtc_enable corresponding to different
> >> > Bspec enable sequence steps into separate functions.
> >> > This helps to call them in a specific order for bigjoiner master/slave
> >> > in a cleaner fashion.
> >>
> >> So does this contain only refactoring or functional changes also? One or
> >> the other at a time, please, not both.
> >
> > No this is only refactor, no functional changes here
> >
> >>
> >> Also looks like hsw_crtc_* have accumulated just way too many checks for
> >> platforms instead of having a clean break at e.g. display 9 and/or 11.
> >
> > These checks were already part of hsw_crtc_enable() function that I have
> > just moved to a separate
> > function. How do you recommend removing these checks?
>
> We use hsw_crtc_enable for all DDI platforms and later. We do have the
> difference between skl_display_funcs and ddi_display_funcs, but they
> both point to hsw_crtc_enable. Maybe it's time for them to have their
> own functions that don't have to take so many platform differences into
> account.
Yes I could perhaps have a separate skl_crtc_enable() function which would then
have all parts that apply to DISPLAy >=9
everything else can stay in hsw_crtc_enable
That makes sense?
Manasi
>
> BR,
> Jani.
>
>
>
>
> >
> >>
> >> Adding references to "enable sequence step 8" is not helpful because
> >> it's platform specific. (Yeah, I know there are existing references like
> >> this, but they're also suspect.)
> >
> > Yes agree, may be I will add the comment for what actually the step 7/8
> > does?
> >
> > Manasi
> >
> >>
> >>
> >> BR,
> >> Jani.
> >>
> >>
> >> >
> >> > Cc: Ville Syrjälä
> >> > Cc: Animesh Manna
> >> > Signed-off-by: Manasi Navare
> >> > ---
> >> > drivers/gpu/drm/i915/display/intel_display.c | 125 ++-
> >> > 1 file changed, 66 insertions(+), 59 deletions(-)
> >> >
> >> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> >> > b/drivers/gpu/drm/i915/display/intel_display.c
> >> > index eb49973621f0..d8e6466c9fa0 100644
> >> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> >> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> >> > @@ -1865,24 +1865,6 @@ static void hsw_set_frame_start_delay(const
> >> > struct intel_crtc_state *crtc_state)
> >> > intel_de_write(dev_priv, reg, val);
> >> > }
> >> >
> >> > -static void icl_ddi_bigjoiner_pre_enable(struct intel_atomic_state
> >> > *state,
> >> > - const struct intel_crtc_state
> >> > *crtc_state)
> >> > -{
> >> > -struct intel_crtc *master_crtc = intel_master_crtc(crtc_state);
> >> > -
> >> > -/*
> >> > - * Enable sequence steps 1-7 on bigjoiner master
> >> > - */
> >> > -if (intel_crtc_is_bigjoiner_slave(crtc_state))
> >> > -intel_encoders_pre_pll_enable(state, master_crtc);
> >> > -
> >> > -if (crtc_state->shared_dpll)
> >> > -intel_enable_shared_dpll(crtc_state);
> >> > -
> >> > -if (intel_crtc_is_bigjoiner_slave(crtc_state))
> >> > -intel_encoders_pre_enable(state, master_crtc);
> >> > -}
> >> > -
> >> > static void hsw_configure_cpu_transcoder(const struct intel_crtc_state
> >> > *crtc_state)
> >> > {
> >> > struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> >> > @@ -1910,70 +1892,73 @@ static void hsw_configure_cpu_transcoder(const
> >> > struct intel_crtc_state *crtc_sta
> >> > hsw_set_transconf(crtc_state);
> >> > }
> >> >
> >> > -static void hsw_crtc_enable(struct intel_atomic_state *state,
> >> > -struct intel_crtc *crtc)
> >> > +static void hsw_crtc_pre_pll_enable(struct intel_atomic_state *state,
> >> > +const struct intel_crtc_state
> >> > *crtc_state)
> >> > {
> >> > -const struct intel_crtc_state *new_crtc_state =
> >> > -intel_atomic_get_new_crtc_state(state, crtc);
> >> > -struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> >> > -enum pipe pipe = crtc->pipe, hsw_workaround_pipe;
> >> > -enum transcoder cpu_transcoder = new_crtc_state->cpu_transcoder;
> >> > -bool psl_clkgate_wa;
> >> > -
> >> > -if (drm_WARN_ON(_priv->drm, crtc->active))
> >> > -return;
> >> > +struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> >> >
> >> > -if (!new_crtc_state->bigjoiner_pipes) {
> >> > -intel_encoders_pre_pll_enable(state, crtc);
> >> > +/*
> >> > + * Enable sequence steps 1 - 7 on all pipes
> >> > + */
> >> > +intel_encoders_pre_pll_enable(state, crtc);
> >> > +if
Re: [Intel-gfx] [PATCH] drm/i915/display/: Refactor hsw_crtc_enable for bigjoiner cleanup
On Wed, 16 Mar 2022, "Navare, Manasi" wrote:
> On Wed, Mar 16, 2022 at 09:48:17AM +0200, Jani Nikula wrote:
>> On Tue, 15 Mar 2022, Manasi Navare wrote:
>> > This patch abstracts pieces of hsw_crtc_enable corresponding to different
>> > Bspec enable sequence steps into separate functions.
>> > This helps to call them in a specific order for bigjoiner master/slave
>> > in a cleaner fashion.
>>
>> So does this contain only refactoring or functional changes also? One or
>> the other at a time, please, not both.
>
> No this is only refactor, no functional changes here
>
>>
>> Also looks like hsw_crtc_* have accumulated just way too many checks for
>> platforms instead of having a clean break at e.g. display 9 and/or 11.
>
> These checks were already part of hsw_crtc_enable() function that I have just
> moved to a separate
> function. How do you recommend removing these checks?
We use hsw_crtc_enable for all DDI platforms and later. We do have the
difference between skl_display_funcs and ddi_display_funcs, but they
both point to hsw_crtc_enable. Maybe it's time for them to have their
own functions that don't have to take so many platform differences into
account.
BR,
Jani.
>
>>
>> Adding references to "enable sequence step 8" is not helpful because
>> it's platform specific. (Yeah, I know there are existing references like
>> this, but they're also suspect.)
>
> Yes agree, may be I will add the comment for what actually the step 7/8 does?
>
> Manasi
>
>>
>>
>> BR,
>> Jani.
>>
>>
>> >
>> > Cc: Ville Syrjälä
>> > Cc: Animesh Manna
>> > Signed-off-by: Manasi Navare
>> > ---
>> > drivers/gpu/drm/i915/display/intel_display.c | 125 ++-
>> > 1 file changed, 66 insertions(+), 59 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
>> > b/drivers/gpu/drm/i915/display/intel_display.c
>> > index eb49973621f0..d8e6466c9fa0 100644
>> > --- a/drivers/gpu/drm/i915/display/intel_display.c
>> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
>> > @@ -1865,24 +1865,6 @@ static void hsw_set_frame_start_delay(const struct
>> > intel_crtc_state *crtc_state)
>> >intel_de_write(dev_priv, reg, val);
>> > }
>> >
>> > -static void icl_ddi_bigjoiner_pre_enable(struct intel_atomic_state *state,
>> > - const struct intel_crtc_state
>> > *crtc_state)
>> > -{
>> > - struct intel_crtc *master_crtc = intel_master_crtc(crtc_state);
>> > -
>> > - /*
>> > - * Enable sequence steps 1-7 on bigjoiner master
>> > - */
>> > - if (intel_crtc_is_bigjoiner_slave(crtc_state))
>> > - intel_encoders_pre_pll_enable(state, master_crtc);
>> > -
>> > - if (crtc_state->shared_dpll)
>> > - intel_enable_shared_dpll(crtc_state);
>> > -
>> > - if (intel_crtc_is_bigjoiner_slave(crtc_state))
>> > - intel_encoders_pre_enable(state, master_crtc);
>> > -}
>> > -
>> > static void hsw_configure_cpu_transcoder(const struct intel_crtc_state
>> > *crtc_state)
>> > {
>> >struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>> > @@ -1910,70 +1892,73 @@ static void hsw_configure_cpu_transcoder(const
>> > struct intel_crtc_state *crtc_sta
>> >hsw_set_transconf(crtc_state);
>> > }
>> >
>> > -static void hsw_crtc_enable(struct intel_atomic_state *state,
>> > - struct intel_crtc *crtc)
>> > +static void hsw_crtc_pre_pll_enable(struct intel_atomic_state *state,
>> > + const struct intel_crtc_state *crtc_state)
>> > {
>> > - const struct intel_crtc_state *new_crtc_state =
>> > - intel_atomic_get_new_crtc_state(state, crtc);
>> > - struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>> > - enum pipe pipe = crtc->pipe, hsw_workaround_pipe;
>> > - enum transcoder cpu_transcoder = new_crtc_state->cpu_transcoder;
>> > - bool psl_clkgate_wa;
>> > -
>> > - if (drm_WARN_ON(_priv->drm, crtc->active))
>> > - return;
>> > + struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>> >
>> > - if (!new_crtc_state->bigjoiner_pipes) {
>> > - intel_encoders_pre_pll_enable(state, crtc);
>> > + /*
>> > + * Enable sequence steps 1 - 7 on all pipes
>> > + */
>> > + intel_encoders_pre_pll_enable(state, crtc);
>> > + if (crtc_state->shared_dpll)
>> > + intel_enable_shared_dpll(crtc_state);
>> >
>> > - if (new_crtc_state->shared_dpll)
>> > - intel_enable_shared_dpll(new_crtc_state);
>> > + intel_encoders_pre_enable(state, crtc);
>> > +}
>> >
>> > - intel_encoders_pre_enable(state, crtc);
>> > - } else {
>> > - icl_ddi_bigjoiner_pre_enable(state, new_crtc_state);
>> > - }
>> > +static void hsw_crtc_post_pll_enable(struct intel_atomic_state *state,
>> > + const struct intel_crtc_state *crtc_state)
>> > +{
>> > + struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>> > + struct drm_i915_private *dev_priv =
Re: [Intel-gfx] [PATCH] drm/i915/display/: Refactor hsw_crtc_enable for bigjoiner cleanup
On Wed, Mar 16, 2022 at 09:48:17AM +0200, Jani Nikula wrote:
> On Tue, 15 Mar 2022, Manasi Navare wrote:
> > This patch abstracts pieces of hsw_crtc_enable corresponding to different
> > Bspec enable sequence steps into separate functions.
> > This helps to call them in a specific order for bigjoiner master/slave
> > in a cleaner fashion.
>
> So does this contain only refactoring or functional changes also? One or
> the other at a time, please, not both.
No this is only refactor, no functional changes here
>
> Also looks like hsw_crtc_* have accumulated just way too many checks for
> platforms instead of having a clean break at e.g. display 9 and/or 11.
These checks were already part of hsw_crtc_enable() function that I have just
moved to a separate
function. How do you recommend removing these checks?
>
> Adding references to "enable sequence step 8" is not helpful because
> it's platform specific. (Yeah, I know there are existing references like
> this, but they're also suspect.)
Yes agree, may be I will add the comment for what actually the step 7/8 does?
Manasi
>
>
> BR,
> Jani.
>
>
> >
> > Cc: Ville Syrjälä
> > Cc: Animesh Manna
> > Signed-off-by: Manasi Navare
> > ---
> > drivers/gpu/drm/i915/display/intel_display.c | 125 ++-
> > 1 file changed, 66 insertions(+), 59 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index eb49973621f0..d8e6466c9fa0 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -1865,24 +1865,6 @@ static void hsw_set_frame_start_delay(const struct
> > intel_crtc_state *crtc_state)
> > intel_de_write(dev_priv, reg, val);
> > }
> >
> > -static void icl_ddi_bigjoiner_pre_enable(struct intel_atomic_state *state,
> > -const struct intel_crtc_state
> > *crtc_state)
> > -{
> > - struct intel_crtc *master_crtc = intel_master_crtc(crtc_state);
> > -
> > - /*
> > -* Enable sequence steps 1-7 on bigjoiner master
> > -*/
> > - if (intel_crtc_is_bigjoiner_slave(crtc_state))
> > - intel_encoders_pre_pll_enable(state, master_crtc);
> > -
> > - if (crtc_state->shared_dpll)
> > - intel_enable_shared_dpll(crtc_state);
> > -
> > - if (intel_crtc_is_bigjoiner_slave(crtc_state))
> > - intel_encoders_pre_enable(state, master_crtc);
> > -}
> > -
> > static void hsw_configure_cpu_transcoder(const struct intel_crtc_state
> > *crtc_state)
> > {
> > struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > @@ -1910,70 +1892,73 @@ static void hsw_configure_cpu_transcoder(const
> > struct intel_crtc_state *crtc_sta
> > hsw_set_transconf(crtc_state);
> > }
> >
> > -static void hsw_crtc_enable(struct intel_atomic_state *state,
> > - struct intel_crtc *crtc)
> > +static void hsw_crtc_pre_pll_enable(struct intel_atomic_state *state,
> > + const struct intel_crtc_state *crtc_state)
> > {
> > - const struct intel_crtc_state *new_crtc_state =
> > - intel_atomic_get_new_crtc_state(state, crtc);
> > - struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > - enum pipe pipe = crtc->pipe, hsw_workaround_pipe;
> > - enum transcoder cpu_transcoder = new_crtc_state->cpu_transcoder;
> > - bool psl_clkgate_wa;
> > -
> > - if (drm_WARN_ON(_priv->drm, crtc->active))
> > - return;
> > + struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> >
> > - if (!new_crtc_state->bigjoiner_pipes) {
> > - intel_encoders_pre_pll_enable(state, crtc);
> > + /*
> > +* Enable sequence steps 1 - 7 on all pipes
> > +*/
> > + intel_encoders_pre_pll_enable(state, crtc);
> > + if (crtc_state->shared_dpll)
> > + intel_enable_shared_dpll(crtc_state);
> >
> > - if (new_crtc_state->shared_dpll)
> > - intel_enable_shared_dpll(new_crtc_state);
> > + intel_encoders_pre_enable(state, crtc);
> > +}
> >
> > - intel_encoders_pre_enable(state, crtc);
> > - } else {
> > - icl_ddi_bigjoiner_pre_enable(state, new_crtc_state);
> > - }
> > +static void hsw_crtc_post_pll_enable(struct intel_atomic_state *state,
> > +const struct intel_crtc_state *crtc_state)
> > +{
> > + struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > + enum pipe pipe = crtc->pipe, hsw_workaround_pipe;
> > + enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
> > + bool psl_clkgate_wa;
> >
> > - intel_dsc_enable(new_crtc_state);
> > + /*
> > +* Enable sequence step 8
> > +*/
> > + intel_dsc_enable(crtc_state);
> >
> > if (DISPLAY_VER(dev_priv) >= 13)
> > - intel_uncompressed_joiner_enable(new_crtc_state);
> > +
Re: [Intel-gfx] [PATCH] drm/i915/display/: Refactor hsw_crtc_enable for bigjoiner cleanup
On Tue, 15 Mar 2022, Manasi Navare wrote:
> This patch abstracts pieces of hsw_crtc_enable corresponding to different
> Bspec enable sequence steps into separate functions.
> This helps to call them in a specific order for bigjoiner master/slave
> in a cleaner fashion.
So does this contain only refactoring or functional changes also? One or
the other at a time, please, not both.
Also looks like hsw_crtc_* have accumulated just way too many checks for
platforms instead of having a clean break at e.g. display 9 and/or 11.
Adding references to "enable sequence step 8" is not helpful because
it's platform specific. (Yeah, I know there are existing references like
this, but they're also suspect.)
BR,
Jani.
>
> Cc: Ville Syrjälä
> Cc: Animesh Manna
> Signed-off-by: Manasi Navare
> ---
> drivers/gpu/drm/i915/display/intel_display.c | 125 ++-
> 1 file changed, 66 insertions(+), 59 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> b/drivers/gpu/drm/i915/display/intel_display.c
> index eb49973621f0..d8e6466c9fa0 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -1865,24 +1865,6 @@ static void hsw_set_frame_start_delay(const struct
> intel_crtc_state *crtc_state)
> intel_de_write(dev_priv, reg, val);
> }
>
> -static void icl_ddi_bigjoiner_pre_enable(struct intel_atomic_state *state,
> - const struct intel_crtc_state
> *crtc_state)
> -{
> - struct intel_crtc *master_crtc = intel_master_crtc(crtc_state);
> -
> - /*
> - * Enable sequence steps 1-7 on bigjoiner master
> - */
> - if (intel_crtc_is_bigjoiner_slave(crtc_state))
> - intel_encoders_pre_pll_enable(state, master_crtc);
> -
> - if (crtc_state->shared_dpll)
> - intel_enable_shared_dpll(crtc_state);
> -
> - if (intel_crtc_is_bigjoiner_slave(crtc_state))
> - intel_encoders_pre_enable(state, master_crtc);
> -}
> -
> static void hsw_configure_cpu_transcoder(const struct intel_crtc_state
> *crtc_state)
> {
> struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> @@ -1910,70 +1892,73 @@ static void hsw_configure_cpu_transcoder(const struct
> intel_crtc_state *crtc_sta
> hsw_set_transconf(crtc_state);
> }
>
> -static void hsw_crtc_enable(struct intel_atomic_state *state,
> - struct intel_crtc *crtc)
> +static void hsw_crtc_pre_pll_enable(struct intel_atomic_state *state,
> + const struct intel_crtc_state *crtc_state)
> {
> - const struct intel_crtc_state *new_crtc_state =
> - intel_atomic_get_new_crtc_state(state, crtc);
> - struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> - enum pipe pipe = crtc->pipe, hsw_workaround_pipe;
> - enum transcoder cpu_transcoder = new_crtc_state->cpu_transcoder;
> - bool psl_clkgate_wa;
> -
> - if (drm_WARN_ON(_priv->drm, crtc->active))
> - return;
> + struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>
> - if (!new_crtc_state->bigjoiner_pipes) {
> - intel_encoders_pre_pll_enable(state, crtc);
> + /*
> + * Enable sequence steps 1 - 7 on all pipes
> + */
> + intel_encoders_pre_pll_enable(state, crtc);
> + if (crtc_state->shared_dpll)
> + intel_enable_shared_dpll(crtc_state);
>
> - if (new_crtc_state->shared_dpll)
> - intel_enable_shared_dpll(new_crtc_state);
> + intel_encoders_pre_enable(state, crtc);
> +}
>
> - intel_encoders_pre_enable(state, crtc);
> - } else {
> - icl_ddi_bigjoiner_pre_enable(state, new_crtc_state);
> - }
> +static void hsw_crtc_post_pll_enable(struct intel_atomic_state *state,
> + const struct intel_crtc_state *crtc_state)
> +{
> + struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> + enum pipe pipe = crtc->pipe, hsw_workaround_pipe;
> + enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
> + bool psl_clkgate_wa;
>
> - intel_dsc_enable(new_crtc_state);
> + /*
> + * Enable sequence step 8
> + */
> + intel_dsc_enable(crtc_state);
>
> if (DISPLAY_VER(dev_priv) >= 13)
> - intel_uncompressed_joiner_enable(new_crtc_state);
> + intel_uncompressed_joiner_enable(crtc_state);
>
> - intel_set_pipe_src_size(new_crtc_state);
> + intel_set_pipe_src_size(crtc_state);
> if (DISPLAY_VER(dev_priv) >= 9 || IS_BROADWELL(dev_priv))
> - bdw_set_pipemisc(new_crtc_state);
> + bdw_set_pipemisc(crtc_state);
>
> - if (!intel_crtc_is_bigjoiner_slave(new_crtc_state) &&
> + if (!intel_crtc_is_bigjoiner_slave(crtc_state) &&
> !transcoder_is_dsi(cpu_transcoder))
> -
[Intel-gfx] [PATCH] drm/i915/display/: Refactor hsw_crtc_enable for bigjoiner cleanup
This patch abstracts pieces of hsw_crtc_enable corresponding to different
Bspec enable sequence steps into separate functions.
This helps to call them in a specific order for bigjoiner master/slave
in a cleaner fashion.
Cc: Ville Syrjälä
Cc: Animesh Manna
Signed-off-by: Manasi Navare
---
drivers/gpu/drm/i915/display/intel_display.c | 125 ++-
1 file changed, 66 insertions(+), 59 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_display.c
b/drivers/gpu/drm/i915/display/intel_display.c
index eb49973621f0..d8e6466c9fa0 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -1865,24 +1865,6 @@ static void hsw_set_frame_start_delay(const struct
intel_crtc_state *crtc_state)
intel_de_write(dev_priv, reg, val);
}
-static void icl_ddi_bigjoiner_pre_enable(struct intel_atomic_state *state,
-const struct intel_crtc_state
*crtc_state)
-{
- struct intel_crtc *master_crtc = intel_master_crtc(crtc_state);
-
- /*
-* Enable sequence steps 1-7 on bigjoiner master
-*/
- if (intel_crtc_is_bigjoiner_slave(crtc_state))
- intel_encoders_pre_pll_enable(state, master_crtc);
-
- if (crtc_state->shared_dpll)
- intel_enable_shared_dpll(crtc_state);
-
- if (intel_crtc_is_bigjoiner_slave(crtc_state))
- intel_encoders_pre_enable(state, master_crtc);
-}
-
static void hsw_configure_cpu_transcoder(const struct intel_crtc_state
*crtc_state)
{
struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
@@ -1910,70 +1892,73 @@ static void hsw_configure_cpu_transcoder(const struct
intel_crtc_state *crtc_sta
hsw_set_transconf(crtc_state);
}
-static void hsw_crtc_enable(struct intel_atomic_state *state,
- struct intel_crtc *crtc)
+static void hsw_crtc_pre_pll_enable(struct intel_atomic_state *state,
+ const struct intel_crtc_state *crtc_state)
{
- const struct intel_crtc_state *new_crtc_state =
- intel_atomic_get_new_crtc_state(state, crtc);
- struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
- enum pipe pipe = crtc->pipe, hsw_workaround_pipe;
- enum transcoder cpu_transcoder = new_crtc_state->cpu_transcoder;
- bool psl_clkgate_wa;
-
- if (drm_WARN_ON(_priv->drm, crtc->active))
- return;
+ struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
- if (!new_crtc_state->bigjoiner_pipes) {
- intel_encoders_pre_pll_enable(state, crtc);
+ /*
+* Enable sequence steps 1 - 7 on all pipes
+*/
+ intel_encoders_pre_pll_enable(state, crtc);
+ if (crtc_state->shared_dpll)
+ intel_enable_shared_dpll(crtc_state);
- if (new_crtc_state->shared_dpll)
- intel_enable_shared_dpll(new_crtc_state);
+ intel_encoders_pre_enable(state, crtc);
+}
- intel_encoders_pre_enable(state, crtc);
- } else {
- icl_ddi_bigjoiner_pre_enable(state, new_crtc_state);
- }
+static void hsw_crtc_post_pll_enable(struct intel_atomic_state *state,
+const struct intel_crtc_state *crtc_state)
+{
+ struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
+ struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
+ enum pipe pipe = crtc->pipe, hsw_workaround_pipe;
+ enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
+ bool psl_clkgate_wa;
- intel_dsc_enable(new_crtc_state);
+ /*
+* Enable sequence step 8
+*/
+ intel_dsc_enable(crtc_state);
if (DISPLAY_VER(dev_priv) >= 13)
- intel_uncompressed_joiner_enable(new_crtc_state);
+ intel_uncompressed_joiner_enable(crtc_state);
- intel_set_pipe_src_size(new_crtc_state);
+ intel_set_pipe_src_size(crtc_state);
if (DISPLAY_VER(dev_priv) >= 9 || IS_BROADWELL(dev_priv))
- bdw_set_pipemisc(new_crtc_state);
+ bdw_set_pipemisc(crtc_state);
- if (!intel_crtc_is_bigjoiner_slave(new_crtc_state) &&
+ if (!intel_crtc_is_bigjoiner_slave(crtc_state) &&
!transcoder_is_dsi(cpu_transcoder))
- hsw_configure_cpu_transcoder(new_crtc_state);
+ hsw_configure_cpu_transcoder(crtc_state);
crtc->active = true;
/* Display WA #1180: WaDisableScalarClockGating: glk */
psl_clkgate_wa = DISPLAY_VER(dev_priv) == 10 &&
- new_crtc_state->pch_pfit.enabled;
+ crtc_state->pch_pfit.enabled;
if (psl_clkgate_wa)
glk_pipe_scaler_clock_gating_wa(dev_priv, pipe, true);
if (DISPLAY_VER(dev_priv) >= 9)
- skl_pfit_enable(new_crtc_state);
+ skl_pfit_enable(crtc_state);
