Re: [Intel-gfx] [PATCH 4/9] drm/i915: Move LPT PCH readout code

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, Oct 18, 2021 at 10:19:31AM +1000, David Airlie wrote:
> On Fri, Oct 15, 2021 at 5:16 PM Ville Syrjala
> <ville.syrjala@linux.intel.com> wrote:
> >
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Nuke the hsw_get_ddi_port_state() eyesore by putting the
> > readout code into intel_pch_display.c, and calling it directly
> > from hsw_crt_get_config().
> >
> > Cc: Dave Airlie <airlied@redhat.com>
> > Cc: Jani Nikula <jani.nikula@intel.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_crt.c      |  2 +
> >  drivers/gpu/drm/i915/display/intel_display.c  | 46 ++-----------------
> >  drivers/gpu/drm/i915/display/intel_display.h  |  2 +
> >  .../gpu/drm/i915/display/intel_pch_display.c  | 18 ++++++++
> >  .../gpu/drm/i915/display/intel_pch_display.h  |  1 +
> >  5 files changed, 26 insertions(+), 43 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_crt.c b/drivers/gpu/drm/i915/display/intel_crt.c
> > index 4038ae342ea1..03cfae46f92f 100644
> > --- a/drivers/gpu/drm/i915/display/intel_crt.c
> > +++ b/drivers/gpu/drm/i915/display/intel_crt.c
> > @@ -147,6 +147,8 @@ static void hsw_crt_get_config(struct intel_encoder *encoder,
> >  {
> >         struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> >
> > +       lpt_pch_get_config(pipe_config);
> > +
> >         hsw_ddi_get_config(encoder, pipe_config);
> >
> >         pipe_config->hw.adjusted_mode.flags &= ~(DRM_MODE_FLAG_PHSYNC |
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index 2ee02c16bd1c..8f65b8b6a306 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -4090,8 +4090,8 @@ void intel_dp_get_m_n(struct intel_crtc *crtc,
> >                                              &pipe_config->dp_m2_n2);
> >  }
> >
> > -static void ilk_get_fdi_m_n_config(struct intel_crtc *crtc,
> > -                                  struct intel_crtc_state *pipe_config)
> > +void ilk_get_fdi_m_n_config(struct intel_crtc *crtc,
> > +                           struct intel_crtc_state *pipe_config)
> >  {
> >         intel_cpu_transcoder_get_m_n(crtc, pipe_config->cpu_transcoder,
> >                                      &pipe_config->fdi_m_n, NULL);
> > @@ -4486,45 +4486,6 @@ static bool bxt_get_dsi_transcoder_state(struct intel_crtc *crtc,
> >         return transcoder_is_dsi(pipe_config->cpu_transcoder);
> >  }
> >
> > -static void hsw_get_ddi_port_state(struct intel_crtc *crtc,
> > -                                  struct intel_crtc_state *pipe_config)
> > -{
> > -       struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > -       enum transcoder cpu_transcoder = pipe_config->cpu_transcoder;
> > -       enum port port;
> > -       u32 tmp;
> > -
> > -       if (transcoder_is_dsi(cpu_transcoder)) {
> > -               port = (cpu_transcoder == TRANSCODER_DSI_A) ?
> > -                                               PORT_A : PORT_B;
> > -       } else {
> > -               tmp = intel_de_read(dev_priv,
> > -                                   TRANS_DDI_FUNC_CTL(cpu_transcoder));
> > -               if (!(tmp & TRANS_DDI_FUNC_ENABLE))
> > -                       return;
> > -               if (DISPLAY_VER(dev_priv) >= 12)
> > -                       port = TGL_TRANS_DDI_FUNC_CTL_VAL_TO_PORT(tmp);
> > -               else
> > -                       port = TRANS_DDI_FUNC_CTL_VAL_TO_PORT(tmp);
> > -       }
> 
> Where does thie code go? is it necessary, maybe make a precursor patch
> showing why this isn't needed?
> or just more commentary on why it's not needed anymore, since PORT_E
> is hardcoded to the crt?

Yeah, since the thing is now called from encoder->get_config() we
already know we're dealing with the correct port. This code was
called from a place where it had no idea which port we were driving
so it had to check manually. I'll amend the commit message a bit.

> 
> This is also the only use of those two macros
> *DDI_FUNC_CTL_VAL_TO_PORT(tmp), should those  be nuked as well?

Probably. If someone needs them in the future they can just
reimplement using REG_FIELD_GET().

Thanks.

> 
> Dave.
> 
> > -
> > -       /*
> > -        * Haswell has only FDI/PCH transcoder A. It is which is connected to
> > -        * DDI E. So just check whether this pipe is wired to DDI E and whether
> > -        * the PCH transcoder is on.
> > -        */
> > -       if (DISPLAY_VER(dev_priv) < 9 &&
> > -           (port == PORT_E) && intel_de_read(dev_priv, LPT_TRANSCONF) & TRANS_ENABLE) {
> > -               pipe_config->has_pch_encoder = true;
> > -
> > -               tmp = intel_de_read(dev_priv, FDI_RX_CTL(PIPE_A));
> > -               pipe_config->fdi_lanes = ((FDI_DP_PORT_WIDTH_MASK & tmp) >>
> > -                                         FDI_DP_PORT_WIDTH_SHIFT) + 1;
> > -
> > -               ilk_get_fdi_m_n_config(crtc, pipe_config);
> > -       }
> > -}
> > -
> >  static bool hsw_get_pipe_config(struct intel_crtc *crtc,
> >                                 struct intel_crtc_state *pipe_config)
> >  {
> > @@ -4562,8 +4523,7 @@ static bool hsw_get_pipe_config(struct intel_crtc *crtc,
> >                 /* we cannot read out most state, so don't bother.. */
> >                 pipe_config->quirks |= PIPE_CONFIG_QUIRK_BIGJOINER_SLAVE;
> >         } else if (!transcoder_is_dsi(pipe_config->cpu_transcoder) ||
> > -           DISPLAY_VER(dev_priv) >= 11) {
> > -               hsw_get_ddi_port_state(crtc, pipe_config);
> > +                  DISPLAY_VER(dev_priv) >= 11) {
> >                 intel_get_transcoder_timings(crtc, pipe_config);
> >         }
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.h b/drivers/gpu/drm/i915/display/intel_display.h
> > index 93c84f2174b5..5bc8d8913178 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display.h
> > @@ -584,6 +584,8 @@ void intel_dp_get_m_n(struct intel_crtc *crtc,
> >                       struct intel_crtc_state *pipe_config);
> >  void intel_dp_set_m_n(const struct intel_crtc_state *crtc_state,
> >                       enum link_m_n_set m_n);
> > +void ilk_get_fdi_m_n_config(struct intel_crtc *crtc,
> > +                           struct intel_crtc_state *pipe_config);
> >  int intel_dotclock_calculate(int link_freq, const struct intel_link_m_n *m_n);
> >
> >  bool hsw_crtc_state_ips_capable(const struct intel_crtc_state *crtc_state);
> > diff --git a/drivers/gpu/drm/i915/display/intel_pch_display.c b/drivers/gpu/drm/i915/display/intel_pch_display.c
> > index 50995c4f2aaa..df7195ed1aaa 100644
> > --- a/drivers/gpu/drm/i915/display/intel_pch_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_pch_display.c
> > @@ -366,3 +366,21 @@ void lpt_pch_enable(struct intel_atomic_state *state,
> >
> >         lpt_enable_pch_transcoder(dev_priv, cpu_transcoder);
> >  }
> > +
> > +void lpt_pch_get_config(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);
> > +       u32 tmp;
> > +
> > +       if ((intel_de_read(dev_priv, LPT_TRANSCONF) & TRANS_ENABLE) == 0)
> > +               return;
> > +
> > +       crtc_state->has_pch_encoder = true;
> > +
> > +       tmp = intel_de_read(dev_priv, FDI_RX_CTL(PIPE_A));
> > +       crtc_state->fdi_lanes = ((FDI_DP_PORT_WIDTH_MASK & tmp) >>
> > +                                FDI_DP_PORT_WIDTH_SHIFT) + 1;
> > +
> > +       ilk_get_fdi_m_n_config(crtc, crtc_state);
> > +}
> > diff --git a/drivers/gpu/drm/i915/display/intel_pch_display.h b/drivers/gpu/drm/i915/display/intel_pch_display.h
> > index 7f9df2c13cf3..e0ff331c0bc6 100644
> > --- a/drivers/gpu/drm/i915/display/intel_pch_display.h
> > +++ b/drivers/gpu/drm/i915/display/intel_pch_display.h
> > @@ -18,5 +18,6 @@ void ilk_pch_enable(struct intel_atomic_state *state,
> >  void lpt_disable_pch_transcoder(struct drm_i915_private *dev_priv);
> >  void lpt_pch_enable(struct intel_atomic_state *state,
> >                     struct intel_crtc *crtc);
> > +void lpt_pch_get_config(struct intel_crtc_state *crtc_state);
> >
> >  #endif
> > --
> > 2.32.0
> >

-- 
Ville Syrjälä
Intel