Re: [PATCH 1/2] drm/i915: Reuse normal state readout for LVDS/DVO fixed mode

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

On Wed, Oct 11, 2017 at 05:21:56PM +0100, Chris Wilson wrote:
> Quoting Chris Wilson (2017-10-10 15:33:33)
> > Quoting Ville Syrjala (2017-10-09 17:19:50)
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > Reuse the normal state readout code to get the fixed mode for LVDS/DVO
> > > encoders. This removes some partially duplicated state readout code
> > > from LVDS/DVO encoders. The duplicated code wasn't actually even
> > > populating the negative h/vsync flags, leading to possible state checker
> > > complaints. The normal readout code populates that stuff fully.
> > > 
> > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c | 50 +++++++++++++++++-------------------
> > >  drivers/gpu/drm/i915/intel_drv.h     |  5 ++--
> > >  drivers/gpu/drm/i915/intel_dvo.c     | 33 ++++++------------------
> > >  drivers/gpu/drm/i915/intel_lvds.c    | 18 ++++---------
> > >  4 files changed, 39 insertions(+), 67 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index 15844bf92434..f8693374955c 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -10247,48 +10247,44 @@ static void ironlake_pch_clock_get(struct intel_crtc *crtc,
> > >                                          &pipe_config->fdi_m_n);
> > >  }
> > >  
> > > -/** Returns the currently programmed mode of the given pipe. */
> > > -struct drm_display_mode *intel_crtc_mode_get(struct drm_device *dev,
> > > -                                            struct drm_crtc *crtc)
> > > +/* Returns the currently programmed mode of the given encoder. */
> > > +struct drm_display_mode *
> > > +intel_encoder_current_mode(struct intel_encoder *encoder)
> > >  {
> > > -       struct drm_i915_private *dev_priv = to_i915(dev);
> > > -       struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > > +       struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > > +       struct intel_crtc_state *crtc_state;
> > >         struct drm_display_mode *mode;
> > > -       struct intel_crtc_state *pipe_config;
> > > -       enum pipe pipe = intel_crtc->pipe;
> > > +       struct intel_crtc *crtc;
> > > +       enum pipe pipe;
> > > +
> > > +       if (!encoder->get_hw_state(encoder, &pipe))
> > > +               return NULL;
> > 
> > There's no chance that get_hw_state can return a pipe beyond our
> > knowledge? I'm presuming we are part of the early hw setup here, so may
> > not have sanitized everything?
> > 
> > > +
> > > +       crtc = intel_get_crtc_for_pipe(dev_priv, pipe);
> > >  
> > >         mode = kzalloc(sizeof(*mode), GFP_KERNEL);
> > >         if (!mode)
> > >                 return NULL;
> > >  
> > > -       pipe_config = kzalloc(sizeof(*pipe_config), GFP_KERNEL);
> > > -       if (!pipe_config) {
> > > +       crtc_state = kzalloc(sizeof(*crtc_state), GFP_KERNEL);
> > > +       if (!crtc_state) {
> > >                 kfree(mode);
> > >                 return NULL;
> > >         }
> > >  
> > > -       /*
> > > -        * Construct a pipe_config sufficient for getting the clock info
> > > -        * back out of crtc_clock_get.
> > > -        *
> > > -        * Note, if LVDS ever uses a non-1 pixel multiplier, we'll need
> > > -        * to use a real value here instead.
> > > -        */
> > > -       pipe_config->cpu_transcoder = (enum transcoder) pipe;
> > > -       pipe_config->pixel_multiplier = 1;
> > > -       pipe_config->dpll_hw_state.dpll = I915_READ(DPLL(pipe));
> > > -       pipe_config->dpll_hw_state.fp0 = I915_READ(FP0(pipe));
> > > -       pipe_config->dpll_hw_state.fp1 = I915_READ(FP1(pipe));
> > > -       i9xx_crtc_clock_get(intel_crtc, pipe_config);
> > > +       crtc_state->base.crtc = &crtc->base;
> > >  
> > > -       pipe_config->base.adjusted_mode.crtc_clock =
> > > -               pipe_config->port_clock / pipe_config->pixel_multiplier;
> > > +       if (!dev_priv->display.get_pipe_config(crtc, crtc_state)) {
> > > +               kfree(crtc_state);
> > > +               kfree(mode);
> > > +               return NULL;
> > > +       }
> > >  
> > > -       intel_get_pipe_timings(intel_crtc, pipe_config);
> > > +       encoder->get_config(encoder, crtc_state);
> > >  
> > > -       intel_mode_from_pipe_config(mode, pipe_config);
> > > +       intel_mode_from_pipe_config(mode, crtc_state);
> > >  
> > > -       kfree(pipe_config);
> > > +       kfree(crtc_state);
> > 
> > This all looks consistent to my eyes.
> > >  
> > >         return mode;
> > >  }
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > index 0cab667fff57..b57a691409c4 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -1363,8 +1363,9 @@ struct intel_connector *intel_connector_alloc(void);
> > >  bool intel_connector_get_hw_state(struct intel_connector *connector);
> > >  void intel_connector_attach_encoder(struct intel_connector *connector,
> > >                                     struct intel_encoder *encoder);
> > > -struct drm_display_mode *intel_crtc_mode_get(struct drm_device *dev,
> > > -                                            struct drm_crtc *crtc);
> > > +struct drm_display_mode *
> > > +intel_encoder_current_mode(struct intel_encoder *encoder);
> > > +
> > >  enum pipe intel_get_pipe_from_connector(struct intel_connector *connector);
> > >  int intel_get_pipe_from_crtc_id(struct drm_device *dev, void *data,
> > >                                 struct drm_file *file_priv);
> > > diff --git a/drivers/gpu/drm/i915/intel_dvo.c b/drivers/gpu/drm/i915/intel_dvo.c
> > > index 5c562e1f56ae..53c9b763f4ce 100644
> > > --- a/drivers/gpu/drm/i915/intel_dvo.c
> > > +++ b/drivers/gpu/drm/i915/intel_dvo.c
> > > @@ -379,32 +379,15 @@ static const struct drm_encoder_funcs intel_dvo_enc_funcs = {
> > >   * chip being on DVOB/C and having multiple pipes.
> > >   */
> > >  static struct drm_display_mode *
> > > -intel_dvo_get_current_mode(struct drm_connector *connector)
> > > +intel_dvo_get_current_mode(struct intel_encoder *encoder)
> > >  {
> > > -       struct drm_device *dev = connector->dev;
> > > -       struct drm_i915_private *dev_priv = to_i915(dev);
> > > -       struct intel_dvo *intel_dvo = intel_attached_dvo(connector);
> > > -       uint32_t dvo_val = I915_READ(intel_dvo->dev.dvo_reg);
> > > -       struct drm_display_mode *mode = NULL;
> > > +       struct drm_display_mode *mode;
> > >  
> > > -       /* If the DVO port is active, that'll be the LVDS, so we can pull out
> > > -        * its timings to get how the BIOS set up the panel.
> > > -        */
> > > -       if (dvo_val & DVO_ENABLE) {
> > > -               struct intel_crtc *crtc;
> > > -               int pipe = (dvo_val & DVO_PIPE_B_SELECT) ? 1 : 0;
> > > -
> > > -               crtc = intel_get_crtc_for_pipe(dev_priv, pipe);
> > > -               if (crtc) {
> > > -                       mode = intel_crtc_mode_get(dev, &crtc->base);
> > > -                       if (mode) {
> > > -                               mode->type |= DRM_MODE_TYPE_PREFERRED;
> > > -                               if (dvo_val & DVO_HSYNC_ACTIVE_HIGH)
> > > -                                       mode->flags |= DRM_MODE_FLAG_PHSYNC;
> > > -                               if (dvo_val & DVO_VSYNC_ACTIVE_HIGH)
> > > -                                       mode->flags |= DRM_MODE_FLAG_PVSYNC;
> > > -                       }
> > > -               }
> > > +       mode = intel_encoder_current_mode(encoder);
> > 
> > Ok.
> > 
> > > +       if (mode) {
> > > +               DRM_DEBUG_KMS("using current (BIOS) mode: ");
> > > +               drm_mode_debug_printmodeline(mode);
> > > +               mode->type |= DRM_MODE_TYPE_PREFERRED;
> > >         }
> > >  
> > >         return mode;
> > > @@ -551,7 +534,7 @@ void intel_dvo_init(struct drm_i915_private *dev_priv)
> > >                          * mode being output through DVO.
> > >                          */
> > >                         intel_panel_init(&intel_connector->panel,
> > > -                                        intel_dvo_get_current_mode(connector),
> > > +                                        intel_dvo_get_current_mode(intel_encoder),
> > >                                          NULL, NULL);
> > >                         intel_dvo->panel_wants_dither = true;
> > >                 }
> > > diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> > > index a55954a89148..c5072b951b9c 100644
> > > --- a/drivers/gpu/drm/i915/intel_lvds.c
> > > +++ b/drivers/gpu/drm/i915/intel_lvds.c
> > > @@ -939,10 +939,8 @@ void intel_lvds_init(struct drm_i915_private *dev_priv)
> > >         struct drm_display_mode *fixed_mode = NULL;
> > >         struct drm_display_mode *downclock_mode = NULL;
> > >         struct edid *edid;
> > > -       struct intel_crtc *crtc;
> > >         i915_reg_t lvds_reg;
> > >         u32 lvds;
> > > -       int pipe;
> > >         u8 pin;
> > >         u32 allowed_scalers;
> > >  
> > > @@ -1118,17 +1116,11 @@ void intel_lvds_init(struct drm_i915_private *dev_priv)
> > >         if (HAS_PCH_SPLIT(dev_priv))
> > >                 goto failed;
> > >  
> > > -       pipe = (lvds & LVDS_PIPEB_SELECT) ? 1 : 0;
> > > -       crtc = intel_get_crtc_for_pipe(dev_priv, pipe);
> > > -
> > > -       if (crtc && (lvds & LVDS_PORT_EN)) {
> > > -               fixed_mode = intel_crtc_mode_get(dev, &crtc->base);
> > > -               if (fixed_mode) {
> > > -                       DRM_DEBUG_KMS("using current (BIOS) mode: ");
> > > -                       drm_mode_debug_printmodeline(fixed_mode);
> > > -                       fixed_mode->type |= DRM_MODE_TYPE_PREFERRED;
> > > -                       goto out;
> > > -               }
> > > +       fixed_mode = intel_encoder_current_mode(intel_encoder);
> > > +       if (fixed_mode) {
> > > +               DRM_DEBUG_KMS("using current (BIOS) mode: ");
> > > +               drm_mode_debug_printmodeline(fixed_mode);
> > > +               fixed_mode->type |= DRM_MODE_TYPE_PREFERRED;
> > >         }
> > >  
> > >         /* If we still don't have a mode after all that, give up. */
> > 
> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> > 
> > I will give it a spin shortly (give or take compilation times on a slow
> > machine and forgetfulness).
> 
> Compiled, booted, and lightly toasted,
> Tested-by: Chris Wilson <chris@chris-wilson.co.uk>

Cool. Thanks for checking again. Patch pushed to dinq.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx