On Tue, 18 Jun 2019, Ville Syrjala <ville.syrjala@xxxxxxxxxxxxxxx> wrote: > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > > Switch from the driver-wide vblank vfuncs to the per-crtc ones so that > we don't have so many platform specific vfuncs in the driver struct. > > We still need to do something about the rest fo the irq vfuncs... We'll also need to do something about having all of this in intel_display.c. Would intel_crtc.[ch] be too close to intel_crt.[ch]? Also, intel_encoder.[ch]? Or, move everything *else* out of intel_display.c? Not part of this patch, obviously. Some comments inline. > > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > --- > drivers/gpu/drm/i915/display/intel_display.c | 104 ++++++++++++++++--- > drivers/gpu/drm/i915/i915_irq.c | 93 +++++++---------- > drivers/gpu/drm/i915/i915_irq.h | 14 +++ > 3 files changed, 143 insertions(+), 68 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > index 8d7e4c8b60bc..dc602a8a52fe 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -13556,7 +13556,7 @@ u32 intel_crtc_get_vblank_counter(struct intel_crtc *crtc) > if (!vblank->max_vblank_count) > return (u32)drm_crtc_accurate_vblank_count(&crtc->base); > > - return dev->driver->get_vblank_counter(dev, crtc->pipe); > + return crtc->base.funcs->get_vblank_counter(&crtc->base); > } > > static void intel_update_crtc(struct drm_crtc *crtc, > @@ -14094,18 +14094,6 @@ static int intel_atomic_commit(struct drm_device *dev, > return 0; > } > > -static const struct drm_crtc_funcs intel_crtc_funcs = { > - .gamma_set = drm_atomic_helper_legacy_gamma_set, > - .set_config = drm_atomic_helper_set_config, > - .destroy = intel_crtc_destroy, > - .page_flip = drm_atomic_helper_page_flip, > - .atomic_duplicate_state = intel_crtc_duplicate_state, > - .atomic_destroy_state = intel_crtc_destroy_state, > - .set_crc_source = intel_crtc_set_crc_source, > - .verify_crc_source = intel_crtc_verify_crc_source, > - .get_crc_sources = intel_crtc_get_crc_sources, > -}; > - > struct wait_rps_boost { > struct wait_queue_entry wait; > > @@ -14899,8 +14887,76 @@ static void intel_crtc_init_scalers(struct intel_crtc *crtc, > scaler_state->scaler_id = -1; > } > > +#define INTEL_CRTC_FUNCS \ > + .gamma_set = drm_atomic_helper_legacy_gamma_set, \ > + .set_config = drm_atomic_helper_set_config, \ > + .destroy = intel_crtc_destroy, \ > + .page_flip = drm_atomic_helper_page_flip, \ > + .atomic_duplicate_state = intel_crtc_duplicate_state, \ > + .atomic_destroy_state = intel_crtc_destroy_state, \ > + .set_crc_source = intel_crtc_set_crc_source, \ > + .verify_crc_source = intel_crtc_verify_crc_source, \ > + .get_crc_sources = intel_crtc_get_crc_sources > + > +static const struct drm_crtc_funcs bdw_crtc_funcs = { > + INTEL_CRTC_FUNCS, > + > + .get_vblank_counter = g4x_get_vblank_counter, > + .enable_vblank = bdw_enable_vblank, > + .disable_vblank = bdw_disable_vblank, > +}; > + > +static const struct drm_crtc_funcs ilk_crtc_funcs = { > + INTEL_CRTC_FUNCS, > + > + .get_vblank_counter = g4x_get_vblank_counter, > + .enable_vblank = ilk_enable_vblank, > + .disable_vblank = ilk_disable_vblank, > +}; > + > +static const struct drm_crtc_funcs g4x_crtc_funcs = { > + INTEL_CRTC_FUNCS, > + > + .get_vblank_counter = g4x_get_vblank_counter, > + .enable_vblank = i965_enable_vblank, > + .disable_vblank = i965_disable_vblank, > +}; > + > +static const struct drm_crtc_funcs i965_crtc_funcs = { > + INTEL_CRTC_FUNCS, > + > + .get_vblank_counter = i915_get_vblank_counter, > + .enable_vblank = i965_enable_vblank, > + .disable_vblank = i965_disable_vblank, > +}; > + > +static const struct drm_crtc_funcs i945gm_crtc_funcs = { > + INTEL_CRTC_FUNCS, > + > + .get_vblank_counter = i915_get_vblank_counter, > + .enable_vblank = i945gm_enable_vblank, > + .disable_vblank = i945gm_disable_vblank, > +}; > + > +static const struct drm_crtc_funcs i915_crtc_funcs = { > + INTEL_CRTC_FUNCS, > + > + .get_vblank_counter = i915_get_vblank_counter, > + .enable_vblank = i8xx_enable_vblank, > + .disable_vblank = i8xx_disable_vblank, > +}; > + > +static const struct drm_crtc_funcs i8xx_crtc_funcs = { > + INTEL_CRTC_FUNCS, > + > + /* no hw vblank counter */ > + .enable_vblank = i8xx_enable_vblank, > + .disable_vblank = i8xx_disable_vblank, > +}; > + > static int intel_crtc_init(struct drm_i915_private *dev_priv, enum pipe pipe) > { > + const struct drm_crtc_funcs *funcs; > struct intel_crtc *intel_crtc; > struct intel_crtc_state *crtc_state = NULL; > struct intel_plane *primary = NULL; > @@ -14944,10 +15000,28 @@ static int intel_crtc_init(struct drm_i915_private *dev_priv, enum pipe pipe) > } > intel_crtc->plane_ids_mask |= BIT(cursor->id); > > + if (HAS_GMCH(dev_priv)) { > + if (IS_CHERRYVIEW(dev_priv) || > + IS_VALLEYVIEW(dev_priv) || IS_G4X(dev_priv)) > + funcs = &g4x_crtc_funcs; > + else if (IS_GEN(dev_priv, 4)) > + funcs = &i965_crtc_funcs; > + else if (IS_I945GM(dev_priv)) > + funcs = &i945gm_crtc_funcs; > + else if (INTEL_GEN(dev_priv) >= 3) > + funcs = &i915_crtc_funcs; > + else > + funcs = &i8xx_crtc_funcs; > + } else { > + if (INTEL_GEN(dev_priv) >= 8) > + funcs = &bdw_crtc_funcs; > + else > + funcs = &ilk_crtc_funcs; > + } > + That's a kind of odd ordering wrt everything else we have. I guess it's CHV that messes up the regular order worst? > ret = drm_crtc_init_with_planes(&dev_priv->drm, &intel_crtc->base, > &primary->base, &cursor->base, > - &intel_crtc_funcs, > - "pipe %c", pipe_name(pipe)); > + funcs, "pipe %c", pipe_name(pipe)); > if (ret) > goto fail; > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > index 2aeb0431c432..1cc27eeae3de 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -918,11 +918,12 @@ static void i915_enable_asle_pipestat(struct drm_i915_private *dev_priv) > /* Called from drm generic code, passed a 'crtc', which > * we use as a pipe index > */ > -static u32 i915_get_vblank_counter(struct drm_device *dev, unsigned int pipe) > +u32 i915_get_vblank_counter(struct drm_crtc *crtc) > { > - struct drm_i915_private *dev_priv = to_i915(dev); > - struct drm_vblank_crtc *vblank = &dev->vblank[pipe]; > + struct drm_i915_private *dev_priv = to_i915(crtc->dev); > + struct drm_vblank_crtc *vblank = &dev_priv->drm.vblank[drm_crtc_index(crtc)]; > const struct drm_display_mode *mode = &vblank->hwmode; > + enum pipe pipe = to_intel_crtc(crtc)->pipe; > i915_reg_t high_frame, low_frame; > u32 high1, high2, low, pixel, vbl_start, hsync_start, htotal; > unsigned long irqflags; > @@ -983,9 +984,10 @@ static u32 i915_get_vblank_counter(struct drm_device *dev, unsigned int pipe) > return (((high1 << 8) | low) + (pixel >= vbl_start)) & 0xffffff; > } > > -static u32 g4x_get_vblank_counter(struct drm_device *dev, unsigned int pipe) > +u32 g4x_get_vblank_counter(struct drm_crtc *crtc) > { > - struct drm_i915_private *dev_priv = to_i915(dev); > + struct drm_i915_private *dev_priv = to_i915(crtc->dev); > + enum pipe pipe = to_intel_crtc(crtc)->pipe; > > return I915_READ(PIPE_FRMCOUNT_G4X(pipe)); > } > @@ -3234,9 +3236,10 @@ static irqreturn_t gen11_irq_handler(int irq, void *arg) > /* Called from drm generic code, passed 'crtc' which > * we use as a pipe index > */ > -static int i8xx_enable_vblank(struct drm_device *dev, unsigned int pipe) > +int i8xx_enable_vblank(struct drm_crtc *crtc) > { > - struct drm_i915_private *dev_priv = to_i915(dev); > + struct drm_i915_private *dev_priv = to_i915(crtc->dev); > + enum pipe pipe = to_intel_crtc(crtc)->pipe; > unsigned long irqflags; > > spin_lock_irqsave(&dev_priv->irq_lock, irqflags); > @@ -3246,19 +3249,20 @@ static int i8xx_enable_vblank(struct drm_device *dev, unsigned int pipe) > return 0; > } > > -static int i945gm_enable_vblank(struct drm_device *dev, unsigned int pipe) > +int i945gm_enable_vblank(struct drm_crtc *crtc) > { > - struct drm_i915_private *dev_priv = to_i915(dev); > + struct drm_i915_private *dev_priv = to_i915(crtc->dev); > > if (dev_priv->i945gm_vblank.enabled++ == 0) > schedule_work(&dev_priv->i945gm_vblank.work); > > - return i8xx_enable_vblank(dev, pipe); > + return i8xx_enable_vblank(crtc); > } > > -static int i965_enable_vblank(struct drm_device *dev, unsigned int pipe) > +int i965_enable_vblank(struct drm_crtc *crtc) > { > - struct drm_i915_private *dev_priv = to_i915(dev); > + struct drm_i915_private *dev_priv = to_i915(crtc->dev); > + enum pipe pipe = to_intel_crtc(crtc)->pipe; > unsigned long irqflags; > > spin_lock_irqsave(&dev_priv->irq_lock, irqflags); > @@ -3269,9 +3273,10 @@ static int i965_enable_vblank(struct drm_device *dev, unsigned int pipe) > return 0; > } > > -static int ironlake_enable_vblank(struct drm_device *dev, unsigned int pipe) > +int ilk_enable_vblank(struct drm_crtc *crtc) > { > - struct drm_i915_private *dev_priv = to_i915(dev); > + struct drm_i915_private *dev_priv = to_i915(crtc->dev); > + enum pipe pipe = to_intel_crtc(crtc)->pipe; > unsigned long irqflags; > u32 bit = INTEL_GEN(dev_priv) >= 7 ? > DE_PIPE_VBLANK_IVB(pipe) : DE_PIPE_VBLANK(pipe); > @@ -3284,14 +3289,15 @@ static int ironlake_enable_vblank(struct drm_device *dev, unsigned int pipe) > * PSR is active as no frames are generated. > */ > if (HAS_PSR(dev_priv)) > - drm_vblank_restore(dev, pipe); > + drm_crtc_vblank_restore(crtc); > > return 0; > } > > -static int gen8_enable_vblank(struct drm_device *dev, unsigned int pipe) > +int bdw_enable_vblank(struct drm_crtc *crtc) > { > - struct drm_i915_private *dev_priv = to_i915(dev); > + struct drm_i915_private *dev_priv = to_i915(crtc->dev); > + enum pipe pipe = to_intel_crtc(crtc)->pipe; > unsigned long irqflags; > > spin_lock_irqsave(&dev_priv->irq_lock, irqflags); > @@ -3302,7 +3308,7 @@ static int gen8_enable_vblank(struct drm_device *dev, unsigned int pipe) > * PSR is active as no frames are generated, so check only for PSR. > */ > if (HAS_PSR(dev_priv)) > - drm_vblank_restore(dev, pipe); > + drm_crtc_vblank_restore(crtc); > > return 0; > } > @@ -3310,9 +3316,10 @@ static int gen8_enable_vblank(struct drm_device *dev, unsigned int pipe) > /* Called from drm generic code, passed 'crtc' which > * we use as a pipe index > */ > -static void i8xx_disable_vblank(struct drm_device *dev, unsigned int pipe) > +void i8xx_disable_vblank(struct drm_crtc *crtc) > { > - struct drm_i915_private *dev_priv = to_i915(dev); > + struct drm_i915_private *dev_priv = to_i915(crtc->dev); > + enum pipe pipe = to_intel_crtc(crtc)->pipe; > unsigned long irqflags; > > spin_lock_irqsave(&dev_priv->irq_lock, irqflags); > @@ -3320,19 +3327,20 @@ static void i8xx_disable_vblank(struct drm_device *dev, unsigned int pipe) > spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags); > } > > -static void i945gm_disable_vblank(struct drm_device *dev, unsigned int pipe) > +void i945gm_disable_vblank(struct drm_crtc *crtc) > { > - struct drm_i915_private *dev_priv = to_i915(dev); > + struct drm_i915_private *dev_priv = to_i915(crtc->dev); > > - i8xx_disable_vblank(dev, pipe); > + i8xx_disable_vblank(crtc); > > if (--dev_priv->i945gm_vblank.enabled == 0) > schedule_work(&dev_priv->i945gm_vblank.work); > } > > -static void i965_disable_vblank(struct drm_device *dev, unsigned int pipe) > +void i965_disable_vblank(struct drm_crtc *crtc) > { > - struct drm_i915_private *dev_priv = to_i915(dev); > + struct drm_i915_private *dev_priv = to_i915(crtc->dev); > + enum pipe pipe = to_intel_crtc(crtc)->pipe; > unsigned long irqflags; > > spin_lock_irqsave(&dev_priv->irq_lock, irqflags); > @@ -3341,9 +3349,10 @@ static void i965_disable_vblank(struct drm_device *dev, unsigned int pipe) > spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags); > } > > -static void ironlake_disable_vblank(struct drm_device *dev, unsigned int pipe) > +void ilk_disable_vblank(struct drm_crtc *crtc) > { > - struct drm_i915_private *dev_priv = to_i915(dev); > + struct drm_i915_private *dev_priv = to_i915(crtc->dev); > + enum pipe pipe = to_intel_crtc(crtc)->pipe; > unsigned long irqflags; > u32 bit = INTEL_GEN(dev_priv) >= 7 ? > DE_PIPE_VBLANK_IVB(pipe) : DE_PIPE_VBLANK(pipe); > @@ -3353,9 +3362,10 @@ static void ironlake_disable_vblank(struct drm_device *dev, unsigned int pipe) > spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags); > } > > -static void gen8_disable_vblank(struct drm_device *dev, unsigned int pipe) > +void bdw_disable_vblank(struct drm_crtc *crtc) > { > - struct drm_i915_private *dev_priv = to_i915(dev); > + struct drm_i915_private *dev_priv = to_i915(crtc->dev); > + enum pipe pipe = to_intel_crtc(crtc)->pipe; > unsigned long irqflags; > > spin_lock_irqsave(&dev_priv->irq_lock, irqflags); > @@ -3363,7 +3373,7 @@ static void gen8_disable_vblank(struct drm_device *dev, unsigned int pipe) > spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags); > } > > -static void i945gm_vblank_work_func(struct work_struct *work) > +void i945gm_vblank_work_func(struct work_struct *work) > { > struct drm_i915_private *dev_priv = > container_of(work, struct drm_i915_private, i945gm_vblank.work); > @@ -4796,11 +4806,6 @@ void intel_irq_init(struct drm_i915_private *dev_priv) > if (INTEL_GEN(dev_priv) >= 8) > rps->pm_intrmsk_mbz |= GEN8_PMINTR_DISABLE_REDIRECT_TO_GUC; > > - if (INTEL_GEN(dev_priv) >= 5 || IS_G4X(dev_priv)) > - dev->driver->get_vblank_counter = g4x_get_vblank_counter; > - else if (INTEL_GEN(dev_priv) >= 3) > - dev->driver->get_vblank_counter = i915_get_vblank_counter; > - > dev->vblank_disable_immediate = true; > > /* Most platforms treat the display irq block as an always-on > @@ -4830,32 +4835,24 @@ void intel_irq_init(struct drm_i915_private *dev_priv) > dev->driver->irq_preinstall = cherryview_irq_reset; > dev->driver->irq_postinstall = cherryview_irq_postinstall; > dev->driver->irq_uninstall = cherryview_irq_reset; > - dev->driver->enable_vblank = i965_enable_vblank; > - dev->driver->disable_vblank = i965_disable_vblank; > dev_priv->display.hpd_irq_setup = i915_hpd_irq_setup; > } else if (IS_VALLEYVIEW(dev_priv)) { > dev->driver->irq_handler = valleyview_irq_handler; > dev->driver->irq_preinstall = valleyview_irq_reset; > dev->driver->irq_postinstall = valleyview_irq_postinstall; > dev->driver->irq_uninstall = valleyview_irq_reset; > - dev->driver->enable_vblank = i965_enable_vblank; > - dev->driver->disable_vblank = i965_disable_vblank; > dev_priv->display.hpd_irq_setup = i915_hpd_irq_setup; > } else if (INTEL_GEN(dev_priv) >= 11) { > dev->driver->irq_handler = gen11_irq_handler; > dev->driver->irq_preinstall = gen11_irq_reset; > dev->driver->irq_postinstall = gen11_irq_postinstall; > dev->driver->irq_uninstall = gen11_irq_reset; > - dev->driver->enable_vblank = gen8_enable_vblank; > - dev->driver->disable_vblank = gen8_disable_vblank; > dev_priv->display.hpd_irq_setup = gen11_hpd_irq_setup; > } else if (INTEL_GEN(dev_priv) >= 8) { > dev->driver->irq_handler = gen8_irq_handler; > dev->driver->irq_preinstall = gen8_irq_reset; > dev->driver->irq_postinstall = gen8_irq_postinstall; > dev->driver->irq_uninstall = gen8_irq_reset; > - dev->driver->enable_vblank = gen8_enable_vblank; > - dev->driver->disable_vblank = gen8_disable_vblank; > if (IS_GEN9_LP(dev_priv)) > dev_priv->display.hpd_irq_setup = bxt_hpd_irq_setup; > else if (INTEL_PCH_TYPE(dev_priv) >= PCH_SPT) > @@ -4867,8 +4864,6 @@ void intel_irq_init(struct drm_i915_private *dev_priv) > dev->driver->irq_preinstall = ironlake_irq_reset; > dev->driver->irq_postinstall = ironlake_irq_postinstall; > dev->driver->irq_uninstall = ironlake_irq_reset; > - dev->driver->enable_vblank = ironlake_enable_vblank; > - dev->driver->disable_vblank = ironlake_disable_vblank; > dev_priv->display.hpd_irq_setup = ilk_hpd_irq_setup; > } else { > if (IS_GEN(dev_priv, 2)) { > @@ -4876,29 +4871,21 @@ void intel_irq_init(struct drm_i915_private *dev_priv) > dev->driver->irq_postinstall = i8xx_irq_postinstall; > dev->driver->irq_handler = i8xx_irq_handler; > dev->driver->irq_uninstall = i8xx_irq_reset; > - dev->driver->enable_vblank = i8xx_enable_vblank; > - dev->driver->disable_vblank = i8xx_disable_vblank; > } else if (IS_I945GM(dev_priv)) { > dev->driver->irq_preinstall = i915_irq_reset; > dev->driver->irq_postinstall = i915_irq_postinstall; > dev->driver->irq_uninstall = i915_irq_reset; > dev->driver->irq_handler = i915_irq_handler; > - dev->driver->enable_vblank = i945gm_enable_vblank; > - dev->driver->disable_vblank = i945gm_disable_vblank; > } else if (IS_GEN(dev_priv, 3)) { > dev->driver->irq_preinstall = i915_irq_reset; > dev->driver->irq_postinstall = i915_irq_postinstall; > dev->driver->irq_uninstall = i915_irq_reset; > dev->driver->irq_handler = i915_irq_handler; > - dev->driver->enable_vblank = i8xx_enable_vblank; > - dev->driver->disable_vblank = i8xx_disable_vblank; > } else { > dev->driver->irq_preinstall = i965_irq_reset; > dev->driver->irq_postinstall = i965_irq_postinstall; > dev->driver->irq_uninstall = i965_irq_reset; > dev->driver->irq_handler = i965_irq_handler; > - dev->driver->enable_vblank = i965_enable_vblank; > - dev->driver->disable_vblank = i965_disable_vblank; > } > if (I915_HAS_HOTPLUG(dev_priv)) > dev_priv->display.hpd_irq_setup = i915_hpd_irq_setup; > diff --git a/drivers/gpu/drm/i915/i915_irq.h b/drivers/gpu/drm/i915/i915_irq.h > index cb25dd213308..ef782e5ab240 100644 > --- a/drivers/gpu/drm/i915/i915_irq.h > +++ b/drivers/gpu/drm/i915/i915_irq.h > @@ -114,4 +114,18 @@ void gen11_reset_guc_interrupts(struct drm_i915_private *i915); > void gen11_enable_guc_interrupts(struct drm_i915_private *i915); > void gen11_disable_guc_interrupts(struct drm_i915_private *i915); > > +u32 i915_get_vblank_counter(struct drm_crtc *crtc); > +u32 g4x_get_vblank_counter(struct drm_crtc *crtc); > + > +int i8xx_enable_vblank(struct drm_crtc *crtc); > +int i945gm_enable_vblank(struct drm_crtc *crtc); > +int i965_enable_vblank(struct drm_crtc *crtc); > +int ilk_enable_vblank(struct drm_crtc *crtc); > +int bdw_enable_vblank(struct drm_crtc *crtc); > +void i8xx_disable_vblank(struct drm_crtc *crtc); > +void i945gm_disable_vblank(struct drm_crtc *crtc); > +void i965_disable_vblank(struct drm_crtc *crtc); > +void ilk_disable_vblank(struct drm_crtc *crtc); > +void bdw_disable_vblank(struct drm_crtc *crtc); *cringe* at exposing so many platform specific functions from i915_irq.c. BR, Jani. > + > #endif /* __I915_IRQ_H__ */ -- Jani Nikula, Intel Open Source Graphics Center _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx