| Message ID | 1441900750-25599-3-git-send-email-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Sep 10, 2015 at 06:59:09PM +0300, ville.syrjala@linux.intel.com wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Add a .get_hw_state() method for planes, returning true or false > depending on whether the plane is enabled. Use it to populate the > plane state 'visible' during state readout. > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Patrik Jakobsson <patrik.jakobsson@linux.intel.com> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Do we really need this? The plane readout is fairly limited and we don't really care about recovering anything but the initial primary plane on driver load. Anything else can just be nuked if it misfits ... -Daniel > --- > drivers/gpu/drm/i915/intel_display.c | 67 ++++++++++++++++++++++++++++-------- > drivers/gpu/drm/i915/intel_drv.h | 1 + > drivers/gpu/drm/i915/intel_sprite.c | 37 ++++++++++++++++++++ > 3 files changed, 91 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 3707212..5fed120 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -2841,6 +2841,13 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc, > POSTING_READ(reg); > } > > +static bool i9xx_primary_get_hw_state(struct intel_plane *plane) > +{ > + struct drm_i915_private *dev_priv = to_i915(plane->base.dev); > + > + return I915_READ(DSPSURF(plane->plane)) & DISPLAY_PLANE_ENABLE; > +} > + > u32 intel_fb_stride_alignment(struct drm_device *dev, uint64_t fb_modifier, > uint32_t pixel_format) > { > @@ -3104,6 +3111,13 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc, > POSTING_READ(PLANE_SURF(pipe, 0)); > } > > +static bool skylake_primary_get_hw_state(struct intel_plane *plane) > +{ > + struct drm_i915_private *dev_priv = to_i915(plane->base.dev); > + > + return I915_READ(PLANE_CTL(plane->pipe, 0)) & PLANE_CTL_ENABLE; > +} > + > /* Assume fb object is pinned & idle & fenced and just update base pointers */ > static int > intel_pipe_set_base_atomic(struct drm_crtc *crtc, struct drm_framebuffer *fb, > @@ -9862,6 +9876,13 @@ static void i845_update_cursor(struct drm_crtc *crtc, u32 base) > } > } > > +static bool i845_cursor_get_hw_state(struct intel_plane *plane) > +{ > + struct drm_i915_private *dev_priv = to_i915(plane->base.dev); > + > + return I915_READ(_CURACNTR) & CURSOR_ENABLE; > +} > + > static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base) > { > struct drm_device *dev = crtc->dev; > @@ -9909,6 +9930,13 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base) > intel_crtc->cursor_base = base; > } > > +static bool i9xx_cursor_get_hw_state(struct intel_plane *plane) > +{ > + struct drm_i915_private *dev_priv = to_i915(plane->base.dev); > + > + return I915_READ(CURCNTR(plane->pipe)) & CURSOR_MODE; > +} > + > /* If no-part of the cursor is visible on the framebuffer, then the GPU may hang... */ > static void intel_crtc_update_cursor(struct drm_crtc *crtc, > bool on) > @@ -13518,12 +13546,17 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev, > } > primary->pipe = pipe; > primary->plane = pipe; > + if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4) > + primary->plane = !pipe; > primary->frontbuffer_bit = INTEL_FRONTBUFFER_PRIMARY(pipe); > primary->check_plane = intel_check_primary_plane; > primary->commit_plane = intel_commit_primary_plane; > primary->disable_plane = intel_disable_primary_plane; > - if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4) > - primary->plane = !pipe; > + > + if (INTEL_INFO(dev)->gen >= 9) > + primary->get_hw_state = skylake_primary_get_hw_state; > + else > + primary->get_hw_state = i9xx_primary_get_hw_state; > > if (INTEL_INFO(dev)->gen >= 9) { > intel_primary_formats = skl_primary_formats; > @@ -13679,6 +13712,11 @@ static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev, > cursor->commit_plane = intel_commit_cursor_plane; > cursor->disable_plane = intel_disable_cursor_plane; > > + if (IS_845G(dev) || IS_I865G(dev)) > + cursor->get_hw_state = i845_cursor_get_hw_state; > + else > + cursor->get_hw_state = i9xx_cursor_get_hw_state; > + > drm_universal_plane_init(dev, &cursor->base, 0, > &intel_plane_funcs, > intel_cursor_formats, > @@ -14886,7 +14924,11 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc) > > /* Disable everything but the primary plane */ > for_each_intel_plane_on_crtc(dev, crtc, plane) { > - if (plane->base.type == DRM_PLANE_TYPE_PRIMARY) > + struct intel_plane_state *state = > + to_intel_plane_state(plane->base.state); > + > + if (plane->base.type == DRM_PLANE_TYPE_PRIMARY || > + !state->visible) > continue; > > plane->disable_plane(&plane->base, &crtc->base); > @@ -15053,21 +15095,18 @@ void i915_redisable_vga(struct drm_device *dev) > i915_redisable_vga_power_on(dev); > } > > -static bool primary_get_hw_state(struct intel_plane *plane) > -{ > - struct drm_i915_private *dev_priv = to_i915(plane->base.dev); > - > - return I915_READ(DSPCNTR(plane->plane)) & DISPLAY_PLANE_ENABLE; > -} > - > /* FIXME read out full plane state for all planes */ > static void readout_plane_state(struct intel_crtc *crtc) > { > - struct intel_plane_state *plane_state = > - to_intel_plane_state(crtc->base.primary->state); > + struct drm_device *dev = crtc->base.dev; > + struct intel_plane *plane; > > - plane_state->visible = > - primary_get_hw_state(to_intel_plane(crtc->base.primary)); > + for_each_intel_plane_on_crtc(dev, crtc, plane) { > + struct intel_plane_state *state = > + to_intel_plane_state(plane->base.state); > + > + state->visible = plane->get_hw_state(plane); > + } > } > > static void intel_modeset_readout_hw_state(struct drm_device *dev) > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 46484e4..64d6d90 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -628,6 +628,7 @@ struct intel_plane { > struct intel_plane_state *state); > void (*commit_plane)(struct drm_plane *plane, > struct intel_plane_state *state); > + bool (*get_hw_state)(struct intel_plane *plane); > }; > > struct intel_watermark_params { > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c > index ca7e264..8b2c744 100644 > --- a/drivers/gpu/drm/i915/intel_sprite.c > +++ b/drivers/gpu/drm/i915/intel_sprite.c > @@ -290,6 +290,15 @@ skl_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc) > intel_update_sprite_watermarks(dplane, crtc, 0, 0, 0, false, false); > } > > +static bool > +skl_plane_get_hw_state(struct intel_plane *plane) > +{ > + struct drm_i915_private *dev_priv = to_i915(plane->base.dev); > + > + return I915_READ(PLANE_CTL(plane->pipe, > + plane->plane + 1)) & PLANE_CTL_ENABLE; > +} > + > static void > chv_update_csc(struct intel_plane *intel_plane, uint32_t format) > { > @@ -469,6 +478,14 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc) > POSTING_READ(SPSURF(pipe, plane)); > } > > +static bool > +vlv_plane_get_hw_state(struct intel_plane *plane) > +{ > + struct drm_i915_private *dev_priv = to_i915(plane->base.dev); > + > + return I915_READ(SPCNTR(plane->pipe, plane->plane)) & SP_ENABLE; > +} > + > static void > ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, > struct drm_framebuffer *fb, > @@ -611,6 +628,14 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc) > POSTING_READ(SPRSURF(pipe)); > } > > +static bool > +ivb_plane_get_hw_state(struct intel_plane *plane) > +{ > + struct drm_i915_private *dev_priv = to_i915(plane->base.dev); > + > + return I915_READ(SPRCTL(plane->pipe)) & SPRITE_ENABLE; > +} > + > static void > ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, > struct drm_framebuffer *fb, > @@ -739,6 +764,14 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc) > POSTING_READ(DVSSURF(pipe)); > } > > +static bool > +ilk_plane_get_hw_state(struct intel_plane *plane) > +{ > + struct drm_i915_private *dev_priv = to_i915(plane->base.dev); > + > + return I915_READ(DVSCNTR(plane->pipe)) & DVS_ENABLE; > +} > + > static int > intel_check_sprite_plane(struct drm_plane *plane, > struct intel_crtc_state *crtc_state, > @@ -1075,6 +1108,7 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane) > intel_plane->max_downscale = 16; > intel_plane->update_plane = ilk_update_plane; > intel_plane->disable_plane = ilk_disable_plane; > + intel_plane->get_hw_state = ilk_plane_get_hw_state; > > if (IS_GEN6(dev)) { > plane_formats = snb_plane_formats; > @@ -1098,12 +1132,14 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane) > if (IS_VALLEYVIEW(dev)) { > intel_plane->update_plane = vlv_update_plane; > intel_plane->disable_plane = vlv_disable_plane; > + intel_plane->get_hw_state = vlv_plane_get_hw_state; > > plane_formats = vlv_plane_formats; > num_plane_formats = ARRAY_SIZE(vlv_plane_formats); > } else { > intel_plane->update_plane = ivb_update_plane; > intel_plane->disable_plane = ivb_disable_plane; > + intel_plane->get_hw_state = ivb_plane_get_hw_state; > > plane_formats = snb_plane_formats; > num_plane_formats = ARRAY_SIZE(snb_plane_formats); > @@ -1113,6 +1149,7 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane) > intel_plane->can_scale = true; > intel_plane->update_plane = skl_update_plane; > intel_plane->disable_plane = skl_disable_plane; > + intel_plane->get_hw_state = skl_plane_get_hw_state; > state->scaler_id = -1; > > plane_formats = skl_plane_formats; > -- > 2.4.6 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Sep 10, 2015 at 06:07:34PM +0200, Daniel Vetter wrote: > On Thu, Sep 10, 2015 at 06:59:09PM +0300, ville.syrjala@linux.intel.com wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Add a .get_hw_state() method for planes, returning true or false > > depending on whether the plane is enabled. Use it to populate the > > plane state 'visible' during state readout. > > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > Cc: Patrik Jakobsson <patrik.jakobsson@linux.intel.com> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Do we really need this? The plane readout is fairly limited and we don't > really care about recovering anything but the initial primary plane on > driver load. Anything else can just be nuked if it misfits ... At least avoids calling ->disable_plane for an already disabled plane. We could even put a WARN there to catch broken code. > -Daniel > > > --- > > drivers/gpu/drm/i915/intel_display.c | 67 ++++++++++++++++++++++++++++-------- > > drivers/gpu/drm/i915/intel_drv.h | 1 + > > drivers/gpu/drm/i915/intel_sprite.c | 37 ++++++++++++++++++++ > > 3 files changed, 91 insertions(+), 14 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index 3707212..5fed120 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -2841,6 +2841,13 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc, > > POSTING_READ(reg); > > } > > > > +static bool i9xx_primary_get_hw_state(struct intel_plane *plane) > > +{ > > + struct drm_i915_private *dev_priv = to_i915(plane->base.dev); > > + > > + return I915_READ(DSPSURF(plane->plane)) & DISPLAY_PLANE_ENABLE; > > +} > > + > > u32 intel_fb_stride_alignment(struct drm_device *dev, uint64_t fb_modifier, > > uint32_t pixel_format) > > { > > @@ -3104,6 +3111,13 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc, > > POSTING_READ(PLANE_SURF(pipe, 0)); > > } > > > > +static bool skylake_primary_get_hw_state(struct intel_plane *plane) > > +{ > > + struct drm_i915_private *dev_priv = to_i915(plane->base.dev); > > + > > + return I915_READ(PLANE_CTL(plane->pipe, 0)) & PLANE_CTL_ENABLE; > > +} > > + > > /* Assume fb object is pinned & idle & fenced and just update base pointers */ > > static int > > intel_pipe_set_base_atomic(struct drm_crtc *crtc, struct drm_framebuffer *fb, > > @@ -9862,6 +9876,13 @@ static void i845_update_cursor(struct drm_crtc *crtc, u32 base) > > } > > } > > > > +static bool i845_cursor_get_hw_state(struct intel_plane *plane) > > +{ > > + struct drm_i915_private *dev_priv = to_i915(plane->base.dev); > > + > > + return I915_READ(_CURACNTR) & CURSOR_ENABLE; > > +} > > + > > static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base) > > { > > struct drm_device *dev = crtc->dev; > > @@ -9909,6 +9930,13 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base) > > intel_crtc->cursor_base = base; > > } > > > > +static bool i9xx_cursor_get_hw_state(struct intel_plane *plane) > > +{ > > + struct drm_i915_private *dev_priv = to_i915(plane->base.dev); > > + > > + return I915_READ(CURCNTR(plane->pipe)) & CURSOR_MODE; > > +} > > + > > /* If no-part of the cursor is visible on the framebuffer, then the GPU may hang... */ > > static void intel_crtc_update_cursor(struct drm_crtc *crtc, > > bool on) > > @@ -13518,12 +13546,17 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev, > > } > > primary->pipe = pipe; > > primary->plane = pipe; > > + if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4) > > + primary->plane = !pipe; > > primary->frontbuffer_bit = INTEL_FRONTBUFFER_PRIMARY(pipe); > > primary->check_plane = intel_check_primary_plane; > > primary->commit_plane = intel_commit_primary_plane; > > primary->disable_plane = intel_disable_primary_plane; > > - if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4) > > - primary->plane = !pipe; > > + > > + if (INTEL_INFO(dev)->gen >= 9) > > + primary->get_hw_state = skylake_primary_get_hw_state; > > + else > > + primary->get_hw_state = i9xx_primary_get_hw_state; > > > > if (INTEL_INFO(dev)->gen >= 9) { > > intel_primary_formats = skl_primary_formats; > > @@ -13679,6 +13712,11 @@ static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev, > > cursor->commit_plane = intel_commit_cursor_plane; > > cursor->disable_plane = intel_disable_cursor_plane; > > > > + if (IS_845G(dev) || IS_I865G(dev)) > > + cursor->get_hw_state = i845_cursor_get_hw_state; > > + else > > + cursor->get_hw_state = i9xx_cursor_get_hw_state; > > + > > drm_universal_plane_init(dev, &cursor->base, 0, > > &intel_plane_funcs, > > intel_cursor_formats, > > @@ -14886,7 +14924,11 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc) > > > > /* Disable everything but the primary plane */ > > for_each_intel_plane_on_crtc(dev, crtc, plane) { > > - if (plane->base.type == DRM_PLANE_TYPE_PRIMARY) > > + struct intel_plane_state *state = > > + to_intel_plane_state(plane->base.state); > > + > > + if (plane->base.type == DRM_PLANE_TYPE_PRIMARY || > > + !state->visible) > > continue; > > > > plane->disable_plane(&plane->base, &crtc->base); > > @@ -15053,21 +15095,18 @@ void i915_redisable_vga(struct drm_device *dev) > > i915_redisable_vga_power_on(dev); > > } > > > > -static bool primary_get_hw_state(struct intel_plane *plane) > > -{ > > - struct drm_i915_private *dev_priv = to_i915(plane->base.dev); > > - > > - return I915_READ(DSPCNTR(plane->plane)) & DISPLAY_PLANE_ENABLE; > > -} > > - > > /* FIXME read out full plane state for all planes */ > > static void readout_plane_state(struct intel_crtc *crtc) > > { > > - struct intel_plane_state *plane_state = > > - to_intel_plane_state(crtc->base.primary->state); > > + struct drm_device *dev = crtc->base.dev; > > + struct intel_plane *plane; > > > > - plane_state->visible = > > - primary_get_hw_state(to_intel_plane(crtc->base.primary)); > > + for_each_intel_plane_on_crtc(dev, crtc, plane) { > > + struct intel_plane_state *state = > > + to_intel_plane_state(plane->base.state); > > + > > + state->visible = plane->get_hw_state(plane); > > + } > > } > > > > static void intel_modeset_readout_hw_state(struct drm_device *dev) > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > > index 46484e4..64d6d90 100644 > > --- a/drivers/gpu/drm/i915/intel_drv.h > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > @@ -628,6 +628,7 @@ struct intel_plane { > > struct intel_plane_state *state); > > void (*commit_plane)(struct drm_plane *plane, > > struct intel_plane_state *state); > > + bool (*get_hw_state)(struct intel_plane *plane); > > }; > > > > struct intel_watermark_params { > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c > > index ca7e264..8b2c744 100644 > > --- a/drivers/gpu/drm/i915/intel_sprite.c > > +++ b/drivers/gpu/drm/i915/intel_sprite.c > > @@ -290,6 +290,15 @@ skl_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc) > > intel_update_sprite_watermarks(dplane, crtc, 0, 0, 0, false, false); > > } > > > > +static bool > > +skl_plane_get_hw_state(struct intel_plane *plane) > > +{ > > + struct drm_i915_private *dev_priv = to_i915(plane->base.dev); > > + > > + return I915_READ(PLANE_CTL(plane->pipe, > > + plane->plane + 1)) & PLANE_CTL_ENABLE; > > +} > > + > > static void > > chv_update_csc(struct intel_plane *intel_plane, uint32_t format) > > { > > @@ -469,6 +478,14 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc) > > POSTING_READ(SPSURF(pipe, plane)); > > } > > > > +static bool > > +vlv_plane_get_hw_state(struct intel_plane *plane) > > +{ > > + struct drm_i915_private *dev_priv = to_i915(plane->base.dev); > > + > > + return I915_READ(SPCNTR(plane->pipe, plane->plane)) & SP_ENABLE; > > +} > > + > > static void > > ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, > > struct drm_framebuffer *fb, > > @@ -611,6 +628,14 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc) > > POSTING_READ(SPRSURF(pipe)); > > } > > > > +static bool > > +ivb_plane_get_hw_state(struct intel_plane *plane) > > +{ > > + struct drm_i915_private *dev_priv = to_i915(plane->base.dev); > > + > > + return I915_READ(SPRCTL(plane->pipe)) & SPRITE_ENABLE; > > +} > > + > > static void > > ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, > > struct drm_framebuffer *fb, > > @@ -739,6 +764,14 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc) > > POSTING_READ(DVSSURF(pipe)); > > } > > > > +static bool > > +ilk_plane_get_hw_state(struct intel_plane *plane) > > +{ > > + struct drm_i915_private *dev_priv = to_i915(plane->base.dev); > > + > > + return I915_READ(DVSCNTR(plane->pipe)) & DVS_ENABLE; > > +} > > + > > static int > > intel_check_sprite_plane(struct drm_plane *plane, > > struct intel_crtc_state *crtc_state, > > @@ -1075,6 +1108,7 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane) > > intel_plane->max_downscale = 16; > > intel_plane->update_plane = ilk_update_plane; > > intel_plane->disable_plane = ilk_disable_plane; > > + intel_plane->get_hw_state = ilk_plane_get_hw_state; > > > > if (IS_GEN6(dev)) { > > plane_formats = snb_plane_formats; > > @@ -1098,12 +1132,14 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane) > > if (IS_VALLEYVIEW(dev)) { > > intel_plane->update_plane = vlv_update_plane; > > intel_plane->disable_plane = vlv_disable_plane; > > + intel_plane->get_hw_state = vlv_plane_get_hw_state; > > > > plane_formats = vlv_plane_formats; > > num_plane_formats = ARRAY_SIZE(vlv_plane_formats); > > } else { > > intel_plane->update_plane = ivb_update_plane; > > intel_plane->disable_plane = ivb_disable_plane; > > + intel_plane->get_hw_state = ivb_plane_get_hw_state; > > > > plane_formats = snb_plane_formats; > > num_plane_formats = ARRAY_SIZE(snb_plane_formats); > > @@ -1113,6 +1149,7 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane) > > intel_plane->can_scale = true; > > intel_plane->update_plane = skl_update_plane; > > intel_plane->disable_plane = skl_disable_plane; > > + intel_plane->get_hw_state = skl_plane_get_hw_state; > > state->scaler_id = -1; > > > > plane_formats = skl_plane_formats; > > -- > > 2.4.6 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
On Thu, Sep 10, 2015 at 07:13:46PM +0300, Ville Syrjälä wrote: > On Thu, Sep 10, 2015 at 06:07:34PM +0200, Daniel Vetter wrote: > > On Thu, Sep 10, 2015 at 06:59:09PM +0300, ville.syrjala@linux.intel.com wrote: > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > Add a .get_hw_state() method for planes, returning true or false > > > depending on whether the plane is enabled. Use it to populate the > > > plane state 'visible' during state readout. > > > > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > > Cc: Patrik Jakobsson <patrik.jakobsson@linux.intel.com> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Do we really need this? The plane readout is fairly limited and we don't > > really care about recovering anything but the initial primary plane on > > driver load. Anything else can just be nuked if it misfits ... > > At least avoids calling ->disable_plane for an already disabled plane. > We could even put a WARN there to catch broken code. Oh and we could also do something like for_each_plane() assert(visible == get_hw_state); somewhere to perhaps catch even more crap. > > > -Daniel > > > > > --- > > > drivers/gpu/drm/i915/intel_display.c | 67 ++++++++++++++++++++++++++++-------- > > > drivers/gpu/drm/i915/intel_drv.h | 1 + > > > drivers/gpu/drm/i915/intel_sprite.c | 37 ++++++++++++++++++++ > > > 3 files changed, 91 insertions(+), 14 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > > index 3707212..5fed120 100644 > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > @@ -2841,6 +2841,13 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc, > > > POSTING_READ(reg); > > > } > > > > > > +static bool i9xx_primary_get_hw_state(struct intel_plane *plane) > > > +{ > > > + struct drm_i915_private *dev_priv = to_i915(plane->base.dev); > > > + > > > + return I915_READ(DSPSURF(plane->plane)) & DISPLAY_PLANE_ENABLE; > > > +} > > > + > > > u32 intel_fb_stride_alignment(struct drm_device *dev, uint64_t fb_modifier, > > > uint32_t pixel_format) > > > { > > > @@ -3104,6 +3111,13 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc, > > > POSTING_READ(PLANE_SURF(pipe, 0)); > > > } > > > > > > +static bool skylake_primary_get_hw_state(struct intel_plane *plane) > > > +{ > > > + struct drm_i915_private *dev_priv = to_i915(plane->base.dev); > > > + > > > + return I915_READ(PLANE_CTL(plane->pipe, 0)) & PLANE_CTL_ENABLE; > > > +} > > > + > > > /* Assume fb object is pinned & idle & fenced and just update base pointers */ > > > static int > > > intel_pipe_set_base_atomic(struct drm_crtc *crtc, struct drm_framebuffer *fb, > > > @@ -9862,6 +9876,13 @@ static void i845_update_cursor(struct drm_crtc *crtc, u32 base) > > > } > > > } > > > > > > +static bool i845_cursor_get_hw_state(struct intel_plane *plane) > > > +{ > > > + struct drm_i915_private *dev_priv = to_i915(plane->base.dev); > > > + > > > + return I915_READ(_CURACNTR) & CURSOR_ENABLE; > > > +} > > > + > > > static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base) > > > { > > > struct drm_device *dev = crtc->dev; > > > @@ -9909,6 +9930,13 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base) > > > intel_crtc->cursor_base = base; > > > } > > > > > > +static bool i9xx_cursor_get_hw_state(struct intel_plane *plane) > > > +{ > > > + struct drm_i915_private *dev_priv = to_i915(plane->base.dev); > > > + > > > + return I915_READ(CURCNTR(plane->pipe)) & CURSOR_MODE; > > > +} > > > + > > > /* If no-part of the cursor is visible on the framebuffer, then the GPU may hang... */ > > > static void intel_crtc_update_cursor(struct drm_crtc *crtc, > > > bool on) > > > @@ -13518,12 +13546,17 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev, > > > } > > > primary->pipe = pipe; > > > primary->plane = pipe; > > > + if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4) > > > + primary->plane = !pipe; > > > primary->frontbuffer_bit = INTEL_FRONTBUFFER_PRIMARY(pipe); > > > primary->check_plane = intel_check_primary_plane; > > > primary->commit_plane = intel_commit_primary_plane; > > > primary->disable_plane = intel_disable_primary_plane; > > > - if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4) > > > - primary->plane = !pipe; > > > + > > > + if (INTEL_INFO(dev)->gen >= 9) > > > + primary->get_hw_state = skylake_primary_get_hw_state; > > > + else > > > + primary->get_hw_state = i9xx_primary_get_hw_state; > > > > > > if (INTEL_INFO(dev)->gen >= 9) { > > > intel_primary_formats = skl_primary_formats; > > > @@ -13679,6 +13712,11 @@ static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev, > > > cursor->commit_plane = intel_commit_cursor_plane; > > > cursor->disable_plane = intel_disable_cursor_plane; > > > > > > + if (IS_845G(dev) || IS_I865G(dev)) > > > + cursor->get_hw_state = i845_cursor_get_hw_state; > > > + else > > > + cursor->get_hw_state = i9xx_cursor_get_hw_state; > > > + > > > drm_universal_plane_init(dev, &cursor->base, 0, > > > &intel_plane_funcs, > > > intel_cursor_formats, > > > @@ -14886,7 +14924,11 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc) > > > > > > /* Disable everything but the primary plane */ > > > for_each_intel_plane_on_crtc(dev, crtc, plane) { > > > - if (plane->base.type == DRM_PLANE_TYPE_PRIMARY) > > > + struct intel_plane_state *state = > > > + to_intel_plane_state(plane->base.state); > > > + > > > + if (plane->base.type == DRM_PLANE_TYPE_PRIMARY || > > > + !state->visible) > > > continue; > > > > > > plane->disable_plane(&plane->base, &crtc->base); > > > @@ -15053,21 +15095,18 @@ void i915_redisable_vga(struct drm_device *dev) > > > i915_redisable_vga_power_on(dev); > > > } > > > > > > -static bool primary_get_hw_state(struct intel_plane *plane) > > > -{ > > > - struct drm_i915_private *dev_priv = to_i915(plane->base.dev); > > > - > > > - return I915_READ(DSPCNTR(plane->plane)) & DISPLAY_PLANE_ENABLE; > > > -} > > > - > > > /* FIXME read out full plane state for all planes */ > > > static void readout_plane_state(struct intel_crtc *crtc) > > > { > > > - struct intel_plane_state *plane_state = > > > - to_intel_plane_state(crtc->base.primary->state); > > > + struct drm_device *dev = crtc->base.dev; > > > + struct intel_plane *plane; > > > > > > - plane_state->visible = > > > - primary_get_hw_state(to_intel_plane(crtc->base.primary)); > > > + for_each_intel_plane_on_crtc(dev, crtc, plane) { > > > + struct intel_plane_state *state = > > > + to_intel_plane_state(plane->base.state); > > > + > > > + state->visible = plane->get_hw_state(plane); > > > + } > > > } > > > > > > static void intel_modeset_readout_hw_state(struct drm_device *dev) > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > > > index 46484e4..64d6d90 100644 > > > --- a/drivers/gpu/drm/i915/intel_drv.h > > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > > @@ -628,6 +628,7 @@ struct intel_plane { > > > struct intel_plane_state *state); > > > void (*commit_plane)(struct drm_plane *plane, > > > struct intel_plane_state *state); > > > + bool (*get_hw_state)(struct intel_plane *plane); > > > }; > > > > > > struct intel_watermark_params { > > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c > > > index ca7e264..8b2c744 100644 > > > --- a/drivers/gpu/drm/i915/intel_sprite.c > > > +++ b/drivers/gpu/drm/i915/intel_sprite.c > > > @@ -290,6 +290,15 @@ skl_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc) > > > intel_update_sprite_watermarks(dplane, crtc, 0, 0, 0, false, false); > > > } > > > > > > +static bool > > > +skl_plane_get_hw_state(struct intel_plane *plane) > > > +{ > > > + struct drm_i915_private *dev_priv = to_i915(plane->base.dev); > > > + > > > + return I915_READ(PLANE_CTL(plane->pipe, > > > + plane->plane + 1)) & PLANE_CTL_ENABLE; > > > +} > > > + > > > static void > > > chv_update_csc(struct intel_plane *intel_plane, uint32_t format) > > > { > > > @@ -469,6 +478,14 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc) > > > POSTING_READ(SPSURF(pipe, plane)); > > > } > > > > > > +static bool > > > +vlv_plane_get_hw_state(struct intel_plane *plane) > > > +{ > > > + struct drm_i915_private *dev_priv = to_i915(plane->base.dev); > > > + > > > + return I915_READ(SPCNTR(plane->pipe, plane->plane)) & SP_ENABLE; > > > +} > > > + > > > static void > > > ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, > > > struct drm_framebuffer *fb, > > > @@ -611,6 +628,14 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc) > > > POSTING_READ(SPRSURF(pipe)); > > > } > > > > > > +static bool > > > +ivb_plane_get_hw_state(struct intel_plane *plane) > > > +{ > > > + struct drm_i915_private *dev_priv = to_i915(plane->base.dev); > > > + > > > + return I915_READ(SPRCTL(plane->pipe)) & SPRITE_ENABLE; > > > +} > > > + > > > static void > > > ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, > > > struct drm_framebuffer *fb, > > > @@ -739,6 +764,14 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc) > > > POSTING_READ(DVSSURF(pipe)); > > > } > > > > > > +static bool > > > +ilk_plane_get_hw_state(struct intel_plane *plane) > > > +{ > > > + struct drm_i915_private *dev_priv = to_i915(plane->base.dev); > > > + > > > + return I915_READ(DVSCNTR(plane->pipe)) & DVS_ENABLE; > > > +} > > > + > > > static int > > > intel_check_sprite_plane(struct drm_plane *plane, > > > struct intel_crtc_state *crtc_state, > > > @@ -1075,6 +1108,7 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane) > > > intel_plane->max_downscale = 16; > > > intel_plane->update_plane = ilk_update_plane; > > > intel_plane->disable_plane = ilk_disable_plane; > > > + intel_plane->get_hw_state = ilk_plane_get_hw_state; > > > > > > if (IS_GEN6(dev)) { > > > plane_formats = snb_plane_formats; > > > @@ -1098,12 +1132,14 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane) > > > if (IS_VALLEYVIEW(dev)) { > > > intel_plane->update_plane = vlv_update_plane; > > > intel_plane->disable_plane = vlv_disable_plane; > > > + intel_plane->get_hw_state = vlv_plane_get_hw_state; > > > > > > plane_formats = vlv_plane_formats; > > > num_plane_formats = ARRAY_SIZE(vlv_plane_formats); > > > } else { > > > intel_plane->update_plane = ivb_update_plane; > > > intel_plane->disable_plane = ivb_disable_plane; > > > + intel_plane->get_hw_state = ivb_plane_get_hw_state; > > > > > > plane_formats = snb_plane_formats; > > > num_plane_formats = ARRAY_SIZE(snb_plane_formats); > > > @@ -1113,6 +1149,7 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane) > > > intel_plane->can_scale = true; > > > intel_plane->update_plane = skl_update_plane; > > > intel_plane->disable_plane = skl_disable_plane; > > > + intel_plane->get_hw_state = skl_plane_get_hw_state; > > > state->scaler_id = -1; > > > > > > plane_formats = skl_plane_formats; > > > -- > > > 2.4.6 > > > > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > > Daniel Vetter > > Software Engineer, Intel Corporation > > http://blog.ffwll.ch > > -- > Ville Syrjälä > Intel OTC
On Thu, Sep 10, 2015 at 07:20:11PM +0300, Ville Syrjälä wrote: > On Thu, Sep 10, 2015 at 07:13:46PM +0300, Ville Syrjälä wrote: > > On Thu, Sep 10, 2015 at 06:07:34PM +0200, Daniel Vetter wrote: > > > On Thu, Sep 10, 2015 at 06:59:09PM +0300, ville.syrjala@linux.intel.com wrote: > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > > > Add a .get_hw_state() method for planes, returning true or false > > > > depending on whether the plane is enabled. Use it to populate the > > > > plane state 'visible' during state readout. > > > > > > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > > > Cc: Patrik Jakobsson <patrik.jakobsson@linux.intel.com> > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > Do we really need this? The plane readout is fairly limited and we don't > > > really care about recovering anything but the initial primary plane on > > > driver load. Anything else can just be nuked if it misfits ... > > > > At least avoids calling ->disable_plane for an already disabled plane. > > We could even put a WARN there to catch broken code. > > Oh and we could also do something like > for_each_plane() > assert(visible == get_hw_state); > > somewhere to perhaps catch even more crap. Yeah maybe, but I think that should be done as part of the patch series with this patch here. As-is I don't think it adds a lot really ... -Daniel > > > > > > > -Daniel > > > > > > > --- > > > > drivers/gpu/drm/i915/intel_display.c | 67 ++++++++++++++++++++++++++++-------- > > > > drivers/gpu/drm/i915/intel_drv.h | 1 + > > > > drivers/gpu/drm/i915/intel_sprite.c | 37 ++++++++++++++++++++ > > > > 3 files changed, 91 insertions(+), 14 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > > > index 3707212..5fed120 100644 > > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > > @@ -2841,6 +2841,13 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc, > > > > POSTING_READ(reg); > > > > } > > > > > > > > +static bool i9xx_primary_get_hw_state(struct intel_plane *plane) > > > > +{ > > > > + struct drm_i915_private *dev_priv = to_i915(plane->base.dev); > > > > + > > > > + return I915_READ(DSPSURF(plane->plane)) & DISPLAY_PLANE_ENABLE; > > > > +} > > > > + > > > > u32 intel_fb_stride_alignment(struct drm_device *dev, uint64_t fb_modifier, > > > > uint32_t pixel_format) > > > > { > > > > @@ -3104,6 +3111,13 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc, > > > > POSTING_READ(PLANE_SURF(pipe, 0)); > > > > } > > > > > > > > +static bool skylake_primary_get_hw_state(struct intel_plane *plane) > > > > +{ > > > > + struct drm_i915_private *dev_priv = to_i915(plane->base.dev); > > > > + > > > > + return I915_READ(PLANE_CTL(plane->pipe, 0)) & PLANE_CTL_ENABLE; > > > > +} > > > > + > > > > /* Assume fb object is pinned & idle & fenced and just update base pointers */ > > > > static int > > > > intel_pipe_set_base_atomic(struct drm_crtc *crtc, struct drm_framebuffer *fb, > > > > @@ -9862,6 +9876,13 @@ static void i845_update_cursor(struct drm_crtc *crtc, u32 base) > > > > } > > > > } > > > > > > > > +static bool i845_cursor_get_hw_state(struct intel_plane *plane) > > > > +{ > > > > + struct drm_i915_private *dev_priv = to_i915(plane->base.dev); > > > > + > > > > + return I915_READ(_CURACNTR) & CURSOR_ENABLE; > > > > +} > > > > + > > > > static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base) > > > > { > > > > struct drm_device *dev = crtc->dev; > > > > @@ -9909,6 +9930,13 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base) > > > > intel_crtc->cursor_base = base; > > > > } > > > > > > > > +static bool i9xx_cursor_get_hw_state(struct intel_plane *plane) > > > > +{ > > > > + struct drm_i915_private *dev_priv = to_i915(plane->base.dev); > > > > + > > > > + return I915_READ(CURCNTR(plane->pipe)) & CURSOR_MODE; > > > > +} > > > > + > > > > /* If no-part of the cursor is visible on the framebuffer, then the GPU may hang... */ > > > > static void intel_crtc_update_cursor(struct drm_crtc *crtc, > > > > bool on) > > > > @@ -13518,12 +13546,17 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev, > > > > } > > > > primary->pipe = pipe; > > > > primary->plane = pipe; > > > > + if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4) > > > > + primary->plane = !pipe; > > > > primary->frontbuffer_bit = INTEL_FRONTBUFFER_PRIMARY(pipe); > > > > primary->check_plane = intel_check_primary_plane; > > > > primary->commit_plane = intel_commit_primary_plane; > > > > primary->disable_plane = intel_disable_primary_plane; > > > > - if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4) > > > > - primary->plane = !pipe; > > > > + > > > > + if (INTEL_INFO(dev)->gen >= 9) > > > > + primary->get_hw_state = skylake_primary_get_hw_state; > > > > + else > > > > + primary->get_hw_state = i9xx_primary_get_hw_state; > > > > > > > > if (INTEL_INFO(dev)->gen >= 9) { > > > > intel_primary_formats = skl_primary_formats; > > > > @@ -13679,6 +13712,11 @@ static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev, > > > > cursor->commit_plane = intel_commit_cursor_plane; > > > > cursor->disable_plane = intel_disable_cursor_plane; > > > > > > > > + if (IS_845G(dev) || IS_I865G(dev)) > > > > + cursor->get_hw_state = i845_cursor_get_hw_state; > > > > + else > > > > + cursor->get_hw_state = i9xx_cursor_get_hw_state; > > > > + > > > > drm_universal_plane_init(dev, &cursor->base, 0, > > > > &intel_plane_funcs, > > > > intel_cursor_formats, > > > > @@ -14886,7 +14924,11 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc) > > > > > > > > /* Disable everything but the primary plane */ > > > > for_each_intel_plane_on_crtc(dev, crtc, plane) { > > > > - if (plane->base.type == DRM_PLANE_TYPE_PRIMARY) > > > > + struct intel_plane_state *state = > > > > + to_intel_plane_state(plane->base.state); > > > > + > > > > + if (plane->base.type == DRM_PLANE_TYPE_PRIMARY || > > > > + !state->visible) > > > > continue; > > > > > > > > plane->disable_plane(&plane->base, &crtc->base); > > > > @@ -15053,21 +15095,18 @@ void i915_redisable_vga(struct drm_device *dev) > > > > i915_redisable_vga_power_on(dev); > > > > } > > > > > > > > -static bool primary_get_hw_state(struct intel_plane *plane) > > > > -{ > > > > - struct drm_i915_private *dev_priv = to_i915(plane->base.dev); > > > > - > > > > - return I915_READ(DSPCNTR(plane->plane)) & DISPLAY_PLANE_ENABLE; > > > > -} > > > > - > > > > /* FIXME read out full plane state for all planes */ > > > > static void readout_plane_state(struct intel_crtc *crtc) > > > > { > > > > - struct intel_plane_state *plane_state = > > > > - to_intel_plane_state(crtc->base.primary->state); > > > > + struct drm_device *dev = crtc->base.dev; > > > > + struct intel_plane *plane; > > > > > > > > - plane_state->visible = > > > > - primary_get_hw_state(to_intel_plane(crtc->base.primary)); > > > > + for_each_intel_plane_on_crtc(dev, crtc, plane) { > > > > + struct intel_plane_state *state = > > > > + to_intel_plane_state(plane->base.state); > > > > + > > > > + state->visible = plane->get_hw_state(plane); > > > > + } > > > > } > > > > > > > > static void intel_modeset_readout_hw_state(struct drm_device *dev) > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > > > > index 46484e4..64d6d90 100644 > > > > --- a/drivers/gpu/drm/i915/intel_drv.h > > > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > > > @@ -628,6 +628,7 @@ struct intel_plane { > > > > struct intel_plane_state *state); > > > > void (*commit_plane)(struct drm_plane *plane, > > > > struct intel_plane_state *state); > > > > + bool (*get_hw_state)(struct intel_plane *plane); > > > > }; > > > > > > > > struct intel_watermark_params { > > > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c > > > > index ca7e264..8b2c744 100644 > > > > --- a/drivers/gpu/drm/i915/intel_sprite.c > > > > +++ b/drivers/gpu/drm/i915/intel_sprite.c > > > > @@ -290,6 +290,15 @@ skl_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc) > > > > intel_update_sprite_watermarks(dplane, crtc, 0, 0, 0, false, false); > > > > } > > > > > > > > +static bool > > > > +skl_plane_get_hw_state(struct intel_plane *plane) > > > > +{ > > > > + struct drm_i915_private *dev_priv = to_i915(plane->base.dev); > > > > + > > > > + return I915_READ(PLANE_CTL(plane->pipe, > > > > + plane->plane + 1)) & PLANE_CTL_ENABLE; > > > > +} > > > > + > > > > static void > > > > chv_update_csc(struct intel_plane *intel_plane, uint32_t format) > > > > { > > > > @@ -469,6 +478,14 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc) > > > > POSTING_READ(SPSURF(pipe, plane)); > > > > } > > > > > > > > +static bool > > > > +vlv_plane_get_hw_state(struct intel_plane *plane) > > > > +{ > > > > + struct drm_i915_private *dev_priv = to_i915(plane->base.dev); > > > > + > > > > + return I915_READ(SPCNTR(plane->pipe, plane->plane)) & SP_ENABLE; > > > > +} > > > > + > > > > static void > > > > ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, > > > > struct drm_framebuffer *fb, > > > > @@ -611,6 +628,14 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc) > > > > POSTING_READ(SPRSURF(pipe)); > > > > } > > > > > > > > +static bool > > > > +ivb_plane_get_hw_state(struct intel_plane *plane) > > > > +{ > > > > + struct drm_i915_private *dev_priv = to_i915(plane->base.dev); > > > > + > > > > + return I915_READ(SPRCTL(plane->pipe)) & SPRITE_ENABLE; > > > > +} > > > > + > > > > static void > > > > ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, > > > > struct drm_framebuffer *fb, > > > > @@ -739,6 +764,14 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc) > > > > POSTING_READ(DVSSURF(pipe)); > > > > } > > > > > > > > +static bool > > > > +ilk_plane_get_hw_state(struct intel_plane *plane) > > > > +{ > > > > + struct drm_i915_private *dev_priv = to_i915(plane->base.dev); > > > > + > > > > + return I915_READ(DVSCNTR(plane->pipe)) & DVS_ENABLE; > > > > +} > > > > + > > > > static int > > > > intel_check_sprite_plane(struct drm_plane *plane, > > > > struct intel_crtc_state *crtc_state, > > > > @@ -1075,6 +1108,7 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane) > > > > intel_plane->max_downscale = 16; > > > > intel_plane->update_plane = ilk_update_plane; > > > > intel_plane->disable_plane = ilk_disable_plane; > > > > + intel_plane->get_hw_state = ilk_plane_get_hw_state; > > > > > > > > if (IS_GEN6(dev)) { > > > > plane_formats = snb_plane_formats; > > > > @@ -1098,12 +1132,14 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane) > > > > if (IS_VALLEYVIEW(dev)) { > > > > intel_plane->update_plane = vlv_update_plane; > > > > intel_plane->disable_plane = vlv_disable_plane; > > > > + intel_plane->get_hw_state = vlv_plane_get_hw_state; > > > > > > > > plane_formats = vlv_plane_formats; > > > > num_plane_formats = ARRAY_SIZE(vlv_plane_formats); > > > > } else { > > > > intel_plane->update_plane = ivb_update_plane; > > > > intel_plane->disable_plane = ivb_disable_plane; > > > > + intel_plane->get_hw_state = ivb_plane_get_hw_state; > > > > > > > > plane_formats = snb_plane_formats; > > > > num_plane_formats = ARRAY_SIZE(snb_plane_formats); > > > > @@ -1113,6 +1149,7 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane) > > > > intel_plane->can_scale = true; > > > > intel_plane->update_plane = skl_update_plane; > > > > intel_plane->disable_plane = skl_disable_plane; > > > > + intel_plane->get_hw_state = skl_plane_get_hw_state; > > > > state->scaler_id = -1; > > > > > > > > plane_formats = skl_plane_formats; > > > > -- > > > > 2.4.6 > > > > > > > > _______________________________________________ > > > > Intel-gfx mailing list > > > > Intel-gfx@lists.freedesktop.org > > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > > > -- > > > Daniel Vetter > > > Software Engineer, Intel Corporation > > > http://blog.ffwll.ch > > > > -- > > Ville Syrjälä > > Intel OTC > > -- > Ville Syrjälä > Intel OTC
On Thu, Sep 10, 2015 at 06:30:20PM +0200, Daniel Vetter wrote: > On Thu, Sep 10, 2015 at 07:20:11PM +0300, Ville Syrjälä wrote: > > On Thu, Sep 10, 2015 at 07:13:46PM +0300, Ville Syrjälä wrote: > > > On Thu, Sep 10, 2015 at 06:07:34PM +0200, Daniel Vetter wrote: > > > > On Thu, Sep 10, 2015 at 06:59:09PM +0300, ville.syrjala@linux.intel.com wrote: > > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > > > > > Add a .get_hw_state() method for planes, returning true or false > > > > > depending on whether the plane is enabled. Use it to populate the > > > > > plane state 'visible' during state readout. > > > > > > > > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > > > > Cc: Patrik Jakobsson <patrik.jakobsson@linux.intel.com> > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > > > Do we really need this? The plane readout is fairly limited and we don't > > > > really care about recovering anything but the initial primary plane on > > > > driver load. Anything else can just be nuked if it misfits ... > > > > > > At least avoids calling ->disable_plane for an already disabled plane. > > > We could even put a WARN there to catch broken code. > > > > Oh and we could also do something like > > for_each_plane() > > assert(visible == get_hw_state); > > > > somewhere to perhaps catch even more crap. > > Yeah maybe, but I think that should be done as part of the patch series > with this patch here. As-is I don't think it adds a lot really ... It would at least fix SKL/BXT primary readout, which I forgot to mention in the commit message naturally. > -Daniel > > > > > > > > > > > > -Daniel > > > > > > > > > --- > > > > > drivers/gpu/drm/i915/intel_display.c | 67 ++++++++++++++++++++++++++++-------- > > > > > drivers/gpu/drm/i915/intel_drv.h | 1 + > > > > > drivers/gpu/drm/i915/intel_sprite.c | 37 ++++++++++++++++++++ > > > > > 3 files changed, 91 insertions(+), 14 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > > > > index 3707212..5fed120 100644 > > > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > > > @@ -2841,6 +2841,13 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc, > > > > > POSTING_READ(reg); > > > > > } > > > > > > > > > > +static bool i9xx_primary_get_hw_state(struct intel_plane *plane) > > > > > +{ > > > > > + struct drm_i915_private *dev_priv = to_i915(plane->base.dev); > > > > > + > > > > > + return I915_READ(DSPSURF(plane->plane)) & DISPLAY_PLANE_ENABLE; > > > > > +} > > > > > + > > > > > u32 intel_fb_stride_alignment(struct drm_device *dev, uint64_t fb_modifier, > > > > > uint32_t pixel_format) > > > > > { > > > > > @@ -3104,6 +3111,13 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc, > > > > > POSTING_READ(PLANE_SURF(pipe, 0)); > > > > > } > > > > > > > > > > +static bool skylake_primary_get_hw_state(struct intel_plane *plane) > > > > > +{ > > > > > + struct drm_i915_private *dev_priv = to_i915(plane->base.dev); > > > > > + > > > > > + return I915_READ(PLANE_CTL(plane->pipe, 0)) & PLANE_CTL_ENABLE; > > > > > +} > > > > > + > > > > > /* Assume fb object is pinned & idle & fenced and just update base pointers */ > > > > > static int > > > > > intel_pipe_set_base_atomic(struct drm_crtc *crtc, struct drm_framebuffer *fb, > > > > > @@ -9862,6 +9876,13 @@ static void i845_update_cursor(struct drm_crtc *crtc, u32 base) > > > > > } > > > > > } > > > > > > > > > > +static bool i845_cursor_get_hw_state(struct intel_plane *plane) > > > > > +{ > > > > > + struct drm_i915_private *dev_priv = to_i915(plane->base.dev); > > > > > + > > > > > + return I915_READ(_CURACNTR) & CURSOR_ENABLE; > > > > > +} > > > > > + > > > > > static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base) > > > > > { > > > > > struct drm_device *dev = crtc->dev; > > > > > @@ -9909,6 +9930,13 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base) > > > > > intel_crtc->cursor_base = base; > > > > > } > > > > > > > > > > +static bool i9xx_cursor_get_hw_state(struct intel_plane *plane) > > > > > +{ > > > > > + struct drm_i915_private *dev_priv = to_i915(plane->base.dev); > > > > > + > > > > > + return I915_READ(CURCNTR(plane->pipe)) & CURSOR_MODE; > > > > > +} > > > > > + > > > > > /* If no-part of the cursor is visible on the framebuffer, then the GPU may hang... */ > > > > > static void intel_crtc_update_cursor(struct drm_crtc *crtc, > > > > > bool on) > > > > > @@ -13518,12 +13546,17 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev, > > > > > } > > > > > primary->pipe = pipe; > > > > > primary->plane = pipe; > > > > > + if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4) > > > > > + primary->plane = !pipe; > > > > > primary->frontbuffer_bit = INTEL_FRONTBUFFER_PRIMARY(pipe); > > > > > primary->check_plane = intel_check_primary_plane; > > > > > primary->commit_plane = intel_commit_primary_plane; > > > > > primary->disable_plane = intel_disable_primary_plane; > > > > > - if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4) > > > > > - primary->plane = !pipe; > > > > > + > > > > > + if (INTEL_INFO(dev)->gen >= 9) > > > > > + primary->get_hw_state = skylake_primary_get_hw_state; > > > > > + else > > > > > + primary->get_hw_state = i9xx_primary_get_hw_state; > > > > > > > > > > if (INTEL_INFO(dev)->gen >= 9) { > > > > > intel_primary_formats = skl_primary_formats; > > > > > @@ -13679,6 +13712,11 @@ static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev, > > > > > cursor->commit_plane = intel_commit_cursor_plane; > > > > > cursor->disable_plane = intel_disable_cursor_plane; > > > > > > > > > > + if (IS_845G(dev) || IS_I865G(dev)) > > > > > + cursor->get_hw_state = i845_cursor_get_hw_state; > > > > > + else > > > > > + cursor->get_hw_state = i9xx_cursor_get_hw_state; > > > > > + > > > > > drm_universal_plane_init(dev, &cursor->base, 0, > > > > > &intel_plane_funcs, > > > > > intel_cursor_formats, > > > > > @@ -14886,7 +14924,11 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc) > > > > > > > > > > /* Disable everything but the primary plane */ > > > > > for_each_intel_plane_on_crtc(dev, crtc, plane) { > > > > > - if (plane->base.type == DRM_PLANE_TYPE_PRIMARY) > > > > > + struct intel_plane_state *state = > > > > > + to_intel_plane_state(plane->base.state); > > > > > + > > > > > + if (plane->base.type == DRM_PLANE_TYPE_PRIMARY || > > > > > + !state->visible) > > > > > continue; > > > > > > > > > > plane->disable_plane(&plane->base, &crtc->base); > > > > > @@ -15053,21 +15095,18 @@ void i915_redisable_vga(struct drm_device *dev) > > > > > i915_redisable_vga_power_on(dev); > > > > > } > > > > > > > > > > -static bool primary_get_hw_state(struct intel_plane *plane) > > > > > -{ > > > > > - struct drm_i915_private *dev_priv = to_i915(plane->base.dev); > > > > > - > > > > > - return I915_READ(DSPCNTR(plane->plane)) & DISPLAY_PLANE_ENABLE; > > > > > -} > > > > > - > > > > > /* FIXME read out full plane state for all planes */ > > > > > static void readout_plane_state(struct intel_crtc *crtc) > > > > > { > > > > > - struct intel_plane_state *plane_state = > > > > > - to_intel_plane_state(crtc->base.primary->state); > > > > > + struct drm_device *dev = crtc->base.dev; > > > > > + struct intel_plane *plane; > > > > > > > > > > - plane_state->visible = > > > > > - primary_get_hw_state(to_intel_plane(crtc->base.primary)); > > > > > + for_each_intel_plane_on_crtc(dev, crtc, plane) { > > > > > + struct intel_plane_state *state = > > > > > + to_intel_plane_state(plane->base.state); > > > > > + > > > > > + state->visible = plane->get_hw_state(plane); > > > > > + } > > > > > } > > > > > > > > > > static void intel_modeset_readout_hw_state(struct drm_device *dev) > > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > > > > > index 46484e4..64d6d90 100644 > > > > > --- a/drivers/gpu/drm/i915/intel_drv.h > > > > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > > > > @@ -628,6 +628,7 @@ struct intel_plane { > > > > > struct intel_plane_state *state); > > > > > void (*commit_plane)(struct drm_plane *plane, > > > > > struct intel_plane_state *state); > > > > > + bool (*get_hw_state)(struct intel_plane *plane); > > > > > }; > > > > > > > > > > struct intel_watermark_params { > > > > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c > > > > > index ca7e264..8b2c744 100644 > > > > > --- a/drivers/gpu/drm/i915/intel_sprite.c > > > > > +++ b/drivers/gpu/drm/i915/intel_sprite.c > > > > > @@ -290,6 +290,15 @@ skl_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc) > > > > > intel_update_sprite_watermarks(dplane, crtc, 0, 0, 0, false, false); > > > > > } > > > > > > > > > > +static bool > > > > > +skl_plane_get_hw_state(struct intel_plane *plane) > > > > > +{ > > > > > + struct drm_i915_private *dev_priv = to_i915(plane->base.dev); > > > > > + > > > > > + return I915_READ(PLANE_CTL(plane->pipe, > > > > > + plane->plane + 1)) & PLANE_CTL_ENABLE; > > > > > +} > > > > > + > > > > > static void > > > > > chv_update_csc(struct intel_plane *intel_plane, uint32_t format) > > > > > { > > > > > @@ -469,6 +478,14 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc) > > > > > POSTING_READ(SPSURF(pipe, plane)); > > > > > } > > > > > > > > > > +static bool > > > > > +vlv_plane_get_hw_state(struct intel_plane *plane) > > > > > +{ > > > > > + struct drm_i915_private *dev_priv = to_i915(plane->base.dev); > > > > > + > > > > > + return I915_READ(SPCNTR(plane->pipe, plane->plane)) & SP_ENABLE; > > > > > +} > > > > > + > > > > > static void > > > > > ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, > > > > > struct drm_framebuffer *fb, > > > > > @@ -611,6 +628,14 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc) > > > > > POSTING_READ(SPRSURF(pipe)); > > > > > } > > > > > > > > > > +static bool > > > > > +ivb_plane_get_hw_state(struct intel_plane *plane) > > > > > +{ > > > > > + struct drm_i915_private *dev_priv = to_i915(plane->base.dev); > > > > > + > > > > > + return I915_READ(SPRCTL(plane->pipe)) & SPRITE_ENABLE; > > > > > +} > > > > > + > > > > > static void > > > > > ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, > > > > > struct drm_framebuffer *fb, > > > > > @@ -739,6 +764,14 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc) > > > > > POSTING_READ(DVSSURF(pipe)); > > > > > } > > > > > > > > > > +static bool > > > > > +ilk_plane_get_hw_state(struct intel_plane *plane) > > > > > +{ > > > > > + struct drm_i915_private *dev_priv = to_i915(plane->base.dev); > > > > > + > > > > > + return I915_READ(DVSCNTR(plane->pipe)) & DVS_ENABLE; > > > > > +} > > > > > + > > > > > static int > > > > > intel_check_sprite_plane(struct drm_plane *plane, > > > > > struct intel_crtc_state *crtc_state, > > > > > @@ -1075,6 +1108,7 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane) > > > > > intel_plane->max_downscale = 16; > > > > > intel_plane->update_plane = ilk_update_plane; > > > > > intel_plane->disable_plane = ilk_disable_plane; > > > > > + intel_plane->get_hw_state = ilk_plane_get_hw_state; > > > > > > > > > > if (IS_GEN6(dev)) { > > > > > plane_formats = snb_plane_formats; > > > > > @@ -1098,12 +1132,14 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane) > > > > > if (IS_VALLEYVIEW(dev)) { > > > > > intel_plane->update_plane = vlv_update_plane; > > > > > intel_plane->disable_plane = vlv_disable_plane; > > > > > + intel_plane->get_hw_state = vlv_plane_get_hw_state; > > > > > > > > > > plane_formats = vlv_plane_formats; > > > > > num_plane_formats = ARRAY_SIZE(vlv_plane_formats); > > > > > } else { > > > > > intel_plane->update_plane = ivb_update_plane; > > > > > intel_plane->disable_plane = ivb_disable_plane; > > > > > + intel_plane->get_hw_state = ivb_plane_get_hw_state; > > > > > > > > > > plane_formats = snb_plane_formats; > > > > > num_plane_formats = ARRAY_SIZE(snb_plane_formats); > > > > > @@ -1113,6 +1149,7 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane) > > > > > intel_plane->can_scale = true; > > > > > intel_plane->update_plane = skl_update_plane; > > > > > intel_plane->disable_plane = skl_disable_plane; > > > > > + intel_plane->get_hw_state = skl_plane_get_hw_state; > > > > > state->scaler_id = -1; > > > > > > > > > > plane_formats = skl_plane_formats; > > > > > -- > > > > > 2.4.6 > > > > > > > > > > _______________________________________________ > > > > > Intel-gfx mailing list > > > > > Intel-gfx@lists.freedesktop.org > > > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > > > > > -- > > > > Daniel Vetter > > > > Software Engineer, Intel Corporation > > > > http://blog.ffwll.ch > > > > > > -- > > > Ville Syrjälä > > > Intel OTC > > > > -- > > Ville Syrjälä > > Intel OTC > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
On Thu, Sep 10, 2015 at 07:36:39PM +0300, Ville Syrjälä wrote: > On Thu, Sep 10, 2015 at 06:30:20PM +0200, Daniel Vetter wrote: > > On Thu, Sep 10, 2015 at 07:20:11PM +0300, Ville Syrjälä wrote: > > > On Thu, Sep 10, 2015 at 07:13:46PM +0300, Ville Syrjälä wrote: > > > > On Thu, Sep 10, 2015 at 06:07:34PM +0200, Daniel Vetter wrote: > > > > > On Thu, Sep 10, 2015 at 06:59:09PM +0300, ville.syrjala@linux.intel.com wrote: > > > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > > > > > > > Add a .get_hw_state() method for planes, returning true or false > > > > > > depending on whether the plane is enabled. Use it to populate the > > > > > > plane state 'visible' during state readout. > > > > > > > > > > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > > > > > Cc: Patrik Jakobsson <patrik.jakobsson@linux.intel.com> > > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > > > > > Do we really need this? The plane readout is fairly limited and we don't > > > > > really care about recovering anything but the initial primary plane on > > > > > driver load. Anything else can just be nuked if it misfits ... > > > > > > > > At least avoids calling ->disable_plane for an already disabled plane. > > > > We could even put a WARN there to catch broken code. > > > > > > Oh and we could also do something like > > > for_each_plane() > > > assert(visible == get_hw_state); > > > > > > somewhere to perhaps catch even more crap. > > > > Yeah maybe, but I think that should be done as part of the patch series > > with this patch here. As-is I don't think it adds a lot really ... > > It would at least fix SKL/BXT primary readout, which I forgot to mention > in the commit message naturally. Yeah, please hide the bugfix less ;-) Adding ->get_hw_state shouldn't be the subject either, it should be something like "Add plane hw state readouf for skl" and the commit message then explains why you added a new vfunc. And we should have some idea what to do about the get_initial_plane_config hook - having two very similar hooks is a bit confusing imo. -Daniel
Op 10-09-15 om 17:59 schreef ville.syrjala@linux.intel.com: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Add a .get_hw_state() method for planes, returning true or false > depending on whether the plane is enabled. Use it to populate the > plane state 'visible' during state readout. > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Patrik Jakobsson <patrik.jakobsson@linux.intel.com> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 67 ++++++++++++++++++++++++++++-------- > drivers/gpu/drm/i915/intel_drv.h | 1 + > drivers/gpu/drm/i915/intel_sprite.c | 37 ++++++++++++++++++++ > 3 files changed, 91 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 3707212..5fed120 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -2841,6 +2841,13 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc, > POSTING_READ(reg); > } > > +static bool i9xx_primary_get_hw_state(struct intel_plane *plane) > +{ > + struct drm_i915_private *dev_priv = to_i915(plane->base.dev); > + > + return I915_READ(DSPSURF(plane->plane)) & DISPLAY_PLANE_ENABLE; > +} > + > u32 intel_fb_stride_alignment(struct drm_device *dev, uint64_t fb_modifier, > uint32_t pixel_format) > { > @@ -3104,6 +3111,13 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc, > POSTING_READ(PLANE_SURF(pipe, 0)); > } > > +static bool skylake_primary_get_hw_state(struct intel_plane *plane) > +{ > + struct drm_i915_private *dev_priv = to_i915(plane->base.dev); > + > + return I915_READ(PLANE_CTL(plane->pipe, 0)) & PLANE_CTL_ENABLE; > +} > + > /* Assume fb object is pinned & idle & fenced and just update base pointers */ > static int > intel_pipe_set_base_atomic(struct drm_crtc *crtc, struct drm_framebuffer *fb, > @@ -9862,6 +9876,13 @@ static void i845_update_cursor(struct drm_crtc *crtc, u32 base) > } > } > > +static bool i845_cursor_get_hw_state(struct intel_plane *plane) > +{ > + struct drm_i915_private *dev_priv = to_i915(plane->base.dev); > + > + return I915_READ(_CURACNTR) & CURSOR_ENABLE; > +} > + > static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base) > { > struct drm_device *dev = crtc->dev; > @@ -9909,6 +9930,13 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base) > intel_crtc->cursor_base = base; > } > > +static bool i9xx_cursor_get_hw_state(struct intel_plane *plane) > +{ > + struct drm_i915_private *dev_priv = to_i915(plane->base.dev); > + > + return I915_READ(CURCNTR(plane->pipe)) & CURSOR_MODE; > +} > + > /* If no-part of the cursor is visible on the framebuffer, then the GPU may hang... */ > static void intel_crtc_update_cursor(struct drm_crtc *crtc, > bool on) > @@ -13518,12 +13546,17 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev, > } > primary->pipe = pipe; > primary->plane = pipe; > + if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4) > + primary->plane = !pipe; > primary->frontbuffer_bit = INTEL_FRONTBUFFER_PRIMARY(pipe); > primary->check_plane = intel_check_primary_plane; > primary->commit_plane = intel_commit_primary_plane; > primary->disable_plane = intel_disable_primary_plane; > - if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4) > - primary->plane = !pipe; > + > + if (INTEL_INFO(dev)->gen >= 9) > + primary->get_hw_state = skylake_primary_get_hw_state; > + else > + primary->get_hw_state = i9xx_primary_get_hw_state; > > if (INTEL_INFO(dev)->gen >= 9) { > intel_primary_formats = skl_primary_formats; > @@ -13679,6 +13712,11 @@ static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev, > cursor->commit_plane = intel_commit_cursor_plane; > cursor->disable_plane = intel_disable_cursor_plane; > > + if (IS_845G(dev) || IS_I865G(dev)) > + cursor->get_hw_state = i845_cursor_get_hw_state; > + else > + cursor->get_hw_state = i9xx_cursor_get_hw_state; > + > drm_universal_plane_init(dev, &cursor->base, 0, > &intel_plane_funcs, > intel_cursor_formats, > @@ -14886,7 +14924,11 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc) > > /* Disable everything but the primary plane */ > for_each_intel_plane_on_crtc(dev, crtc, plane) { > - if (plane->base.type == DRM_PLANE_TYPE_PRIMARY) > + struct intel_plane_state *state = > + to_intel_plane_state(plane->base.state); > + > + if (plane->base.type == DRM_PLANE_TYPE_PRIMARY || > + !state->visible) > continue; > > plane->disable_plane(&plane->base, &crtc->base); > @@ -15053,21 +15095,18 @@ void i915_redisable_vga(struct drm_device *dev) > i915_redisable_vga_power_on(dev); > } > > -static bool primary_get_hw_state(struct intel_plane *plane) > -{ > - struct drm_i915_private *dev_priv = to_i915(plane->base.dev); > - > - return I915_READ(DSPCNTR(plane->plane)) & DISPLAY_PLANE_ENABLE; > -} > - > /* FIXME read out full plane state for all planes */ > static void readout_plane_state(struct intel_crtc *crtc) > { > - struct intel_plane_state *plane_state = > - to_intel_plane_state(crtc->base.primary->state); > + struct drm_device *dev = crtc->base.dev; > + struct intel_plane *plane; > > - plane_state->visible = > - primary_get_hw_state(to_intel_plane(crtc->base.primary)); > + for_each_intel_plane_on_crtc(dev, crtc, plane) { > + struct intel_plane_state *state = > + to_intel_plane_state(plane->base.state); > + > + state->visible = plane->get_hw_state(plane); > + } > } > I would like a get_hw_state for the state checker, but it would need to take a intel_plane_state. I don't think you need it here, blindly calling disable_plane would be good enough if it's done after setting up mode. ~Maarten
On Mon, Sep 14, 2015 at 02:10:20PM +0200, Maarten Lankhorst wrote: > Op 10-09-15 om 17:59 schreef ville.syrjala@linux.intel.com: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Add a .get_hw_state() method for planes, returning true or false > > depending on whether the plane is enabled. Use it to populate the > > plane state 'visible' during state readout. > > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > Cc: Patrik Jakobsson <patrik.jakobsson@linux.intel.com> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/i915/intel_display.c | 67 ++++++++++++++++++++++++++++-------- > > drivers/gpu/drm/i915/intel_drv.h | 1 + > > drivers/gpu/drm/i915/intel_sprite.c | 37 ++++++++++++++++++++ > > 3 files changed, 91 insertions(+), 14 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index 3707212..5fed120 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -2841,6 +2841,13 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc, > > POSTING_READ(reg); > > } > > > > +static bool i9xx_primary_get_hw_state(struct intel_plane *plane) > > +{ > > + struct drm_i915_private *dev_priv = to_i915(plane->base.dev); > > + > > + return I915_READ(DSPSURF(plane->plane)) & DISPLAY_PLANE_ENABLE; > > +} > > + > > u32 intel_fb_stride_alignment(struct drm_device *dev, uint64_t fb_modifier, > > uint32_t pixel_format) > > { > > @@ -3104,6 +3111,13 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc, > > POSTING_READ(PLANE_SURF(pipe, 0)); > > } > > > > +static bool skylake_primary_get_hw_state(struct intel_plane *plane) > > +{ > > + struct drm_i915_private *dev_priv = to_i915(plane->base.dev); > > + > > + return I915_READ(PLANE_CTL(plane->pipe, 0)) & PLANE_CTL_ENABLE; > > +} > > + > > /* Assume fb object is pinned & idle & fenced and just update base pointers */ > > static int > > intel_pipe_set_base_atomic(struct drm_crtc *crtc, struct drm_framebuffer *fb, > > @@ -9862,6 +9876,13 @@ static void i845_update_cursor(struct drm_crtc *crtc, u32 base) > > } > > } > > > > +static bool i845_cursor_get_hw_state(struct intel_plane *plane) > > +{ > > + struct drm_i915_private *dev_priv = to_i915(plane->base.dev); > > + > > + return I915_READ(_CURACNTR) & CURSOR_ENABLE; > > +} > > + > > static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base) > > { > > struct drm_device *dev = crtc->dev; > > @@ -9909,6 +9930,13 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base) > > intel_crtc->cursor_base = base; > > } > > > > +static bool i9xx_cursor_get_hw_state(struct intel_plane *plane) > > +{ > > + struct drm_i915_private *dev_priv = to_i915(plane->base.dev); > > + > > + return I915_READ(CURCNTR(plane->pipe)) & CURSOR_MODE; > > +} > > + > > /* If no-part of the cursor is visible on the framebuffer, then the GPU may hang... */ > > static void intel_crtc_update_cursor(struct drm_crtc *crtc, > > bool on) > > @@ -13518,12 +13546,17 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev, > > } > > primary->pipe = pipe; > > primary->plane = pipe; > > + if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4) > > + primary->plane = !pipe; > > primary->frontbuffer_bit = INTEL_FRONTBUFFER_PRIMARY(pipe); > > primary->check_plane = intel_check_primary_plane; > > primary->commit_plane = intel_commit_primary_plane; > > primary->disable_plane = intel_disable_primary_plane; > > - if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4) > > - primary->plane = !pipe; > > + > > + if (INTEL_INFO(dev)->gen >= 9) > > + primary->get_hw_state = skylake_primary_get_hw_state; > > + else > > + primary->get_hw_state = i9xx_primary_get_hw_state; > > > > if (INTEL_INFO(dev)->gen >= 9) { > > intel_primary_formats = skl_primary_formats; > > @@ -13679,6 +13712,11 @@ static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev, > > cursor->commit_plane = intel_commit_cursor_plane; > > cursor->disable_plane = intel_disable_cursor_plane; > > > > + if (IS_845G(dev) || IS_I865G(dev)) > > + cursor->get_hw_state = i845_cursor_get_hw_state; > > + else > > + cursor->get_hw_state = i9xx_cursor_get_hw_state; > > + > > drm_universal_plane_init(dev, &cursor->base, 0, > > &intel_plane_funcs, > > intel_cursor_formats, > > @@ -14886,7 +14924,11 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc) > > > > /* Disable everything but the primary plane */ > > for_each_intel_plane_on_crtc(dev, crtc, plane) { > > - if (plane->base.type == DRM_PLANE_TYPE_PRIMARY) > > + struct intel_plane_state *state = > > + to_intel_plane_state(plane->base.state); > > + > > + if (plane->base.type == DRM_PLANE_TYPE_PRIMARY || > > + !state->visible) > > continue; > > > > plane->disable_plane(&plane->base, &crtc->base); > > @@ -15053,21 +15095,18 @@ void i915_redisable_vga(struct drm_device *dev) > > i915_redisable_vga_power_on(dev); > > } > > > > -static bool primary_get_hw_state(struct intel_plane *plane) > > -{ > > - struct drm_i915_private *dev_priv = to_i915(plane->base.dev); > > - > > - return I915_READ(DSPCNTR(plane->plane)) & DISPLAY_PLANE_ENABLE; > > -} > > - > > /* FIXME read out full plane state for all planes */ > > static void readout_plane_state(struct intel_crtc *crtc) > > { > > - struct intel_plane_state *plane_state = > > - to_intel_plane_state(crtc->base.primary->state); > > + struct drm_device *dev = crtc->base.dev; > > + struct intel_plane *plane; > > > > - plane_state->visible = > > - primary_get_hw_state(to_intel_plane(crtc->base.primary)); > > + for_each_intel_plane_on_crtc(dev, crtc, plane) { > > + struct intel_plane_state *state = > > + to_intel_plane_state(plane->base.state); > > + > > + state->visible = plane->get_hw_state(plane); > > + } > > } > > > I would like a get_hw_state for the state checker, but it would need to take a intel_plane_state. That would be inconsistent with existing practices. .get_hw_state() != .get_config()
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 3707212..5fed120 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -2841,6 +2841,13 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc, POSTING_READ(reg); } +static bool i9xx_primary_get_hw_state(struct intel_plane *plane) +{ + struct drm_i915_private *dev_priv = to_i915(plane->base.dev); + + return I915_READ(DSPSURF(plane->plane)) & DISPLAY_PLANE_ENABLE; +} + u32 intel_fb_stride_alignment(struct drm_device *dev, uint64_t fb_modifier, uint32_t pixel_format) { @@ -3104,6 +3111,13 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc, POSTING_READ(PLANE_SURF(pipe, 0)); } +static bool skylake_primary_get_hw_state(struct intel_plane *plane) +{ + struct drm_i915_private *dev_priv = to_i915(plane->base.dev); + + return I915_READ(PLANE_CTL(plane->pipe, 0)) & PLANE_CTL_ENABLE; +} + /* Assume fb object is pinned & idle & fenced and just update base pointers */ static int intel_pipe_set_base_atomic(struct drm_crtc *crtc, struct drm_framebuffer *fb, @@ -9862,6 +9876,13 @@ static void i845_update_cursor(struct drm_crtc *crtc, u32 base) } } +static bool i845_cursor_get_hw_state(struct intel_plane *plane) +{ + struct drm_i915_private *dev_priv = to_i915(plane->base.dev); + + return I915_READ(_CURACNTR) & CURSOR_ENABLE; +} + static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base) { struct drm_device *dev = crtc->dev; @@ -9909,6 +9930,13 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base) intel_crtc->cursor_base = base; } +static bool i9xx_cursor_get_hw_state(struct intel_plane *plane) +{ + struct drm_i915_private *dev_priv = to_i915(plane->base.dev); + + return I915_READ(CURCNTR(plane->pipe)) & CURSOR_MODE; +} + /* If no-part of the cursor is visible on the framebuffer, then the GPU may hang... */ static void intel_crtc_update_cursor(struct drm_crtc *crtc, bool on) @@ -13518,12 +13546,17 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev, } primary->pipe = pipe; primary->plane = pipe; + if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4) + primary->plane = !pipe; primary->frontbuffer_bit = INTEL_FRONTBUFFER_PRIMARY(pipe); primary->check_plane = intel_check_primary_plane; primary->commit_plane = intel_commit_primary_plane; primary->disable_plane = intel_disable_primary_plane; - if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4) - primary->plane = !pipe; + + if (INTEL_INFO(dev)->gen >= 9) + primary->get_hw_state = skylake_primary_get_hw_state; + else + primary->get_hw_state = i9xx_primary_get_hw_state; if (INTEL_INFO(dev)->gen >= 9) { intel_primary_formats = skl_primary_formats; @@ -13679,6 +13712,11 @@ static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev, cursor->commit_plane = intel_commit_cursor_plane; cursor->disable_plane = intel_disable_cursor_plane; + if (IS_845G(dev) || IS_I865G(dev)) + cursor->get_hw_state = i845_cursor_get_hw_state; + else + cursor->get_hw_state = i9xx_cursor_get_hw_state; + drm_universal_plane_init(dev, &cursor->base, 0, &intel_plane_funcs, intel_cursor_formats, @@ -14886,7 +14924,11 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc) /* Disable everything but the primary plane */ for_each_intel_plane_on_crtc(dev, crtc, plane) { - if (plane->base.type == DRM_PLANE_TYPE_PRIMARY) + struct intel_plane_state *state = + to_intel_plane_state(plane->base.state); + + if (plane->base.type == DRM_PLANE_TYPE_PRIMARY || + !state->visible) continue; plane->disable_plane(&plane->base, &crtc->base); @@ -15053,21 +15095,18 @@ void i915_redisable_vga(struct drm_device *dev) i915_redisable_vga_power_on(dev); } -static bool primary_get_hw_state(struct intel_plane *plane) -{ - struct drm_i915_private *dev_priv = to_i915(plane->base.dev); - - return I915_READ(DSPCNTR(plane->plane)) & DISPLAY_PLANE_ENABLE; -} - /* FIXME read out full plane state for all planes */ static void readout_plane_state(struct intel_crtc *crtc) { - struct intel_plane_state *plane_state = - to_intel_plane_state(crtc->base.primary->state); + struct drm_device *dev = crtc->base.dev; + struct intel_plane *plane; - plane_state->visible = - primary_get_hw_state(to_intel_plane(crtc->base.primary)); + for_each_intel_plane_on_crtc(dev, crtc, plane) { + struct intel_plane_state *state = + to_intel_plane_state(plane->base.state); + + state->visible = plane->get_hw_state(plane); + } } static void intel_modeset_readout_hw_state(struct drm_device *dev) diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 46484e4..64d6d90 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -628,6 +628,7 @@ struct intel_plane { struct intel_plane_state *state); void (*commit_plane)(struct drm_plane *plane, struct intel_plane_state *state); + bool (*get_hw_state)(struct intel_plane *plane); }; struct intel_watermark_params { diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c index ca7e264..8b2c744 100644 --- a/drivers/gpu/drm/i915/intel_sprite.c +++ b/drivers/gpu/drm/i915/intel_sprite.c @@ -290,6 +290,15 @@ skl_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc) intel_update_sprite_watermarks(dplane, crtc, 0, 0, 0, false, false); } +static bool +skl_plane_get_hw_state(struct intel_plane *plane) +{ + struct drm_i915_private *dev_priv = to_i915(plane->base.dev); + + return I915_READ(PLANE_CTL(plane->pipe, + plane->plane + 1)) & PLANE_CTL_ENABLE; +} + static void chv_update_csc(struct intel_plane *intel_plane, uint32_t format) { @@ -469,6 +478,14 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc) POSTING_READ(SPSURF(pipe, plane)); } +static bool +vlv_plane_get_hw_state(struct intel_plane *plane) +{ + struct drm_i915_private *dev_priv = to_i915(plane->base.dev); + + return I915_READ(SPCNTR(plane->pipe, plane->plane)) & SP_ENABLE; +} + static void ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, struct drm_framebuffer *fb, @@ -611,6 +628,14 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc) POSTING_READ(SPRSURF(pipe)); } +static bool +ivb_plane_get_hw_state(struct intel_plane *plane) +{ + struct drm_i915_private *dev_priv = to_i915(plane->base.dev); + + return I915_READ(SPRCTL(plane->pipe)) & SPRITE_ENABLE; +} + static void ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, struct drm_framebuffer *fb, @@ -739,6 +764,14 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc) POSTING_READ(DVSSURF(pipe)); } +static bool +ilk_plane_get_hw_state(struct intel_plane *plane) +{ + struct drm_i915_private *dev_priv = to_i915(plane->base.dev); + + return I915_READ(DVSCNTR(plane->pipe)) & DVS_ENABLE; +} + static int intel_check_sprite_plane(struct drm_plane *plane, struct intel_crtc_state *crtc_state, @@ -1075,6 +1108,7 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane) intel_plane->max_downscale = 16; intel_plane->update_plane = ilk_update_plane; intel_plane->disable_plane = ilk_disable_plane; + intel_plane->get_hw_state = ilk_plane_get_hw_state; if (IS_GEN6(dev)) { plane_formats = snb_plane_formats; @@ -1098,12 +1132,14 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane) if (IS_VALLEYVIEW(dev)) { intel_plane->update_plane = vlv_update_plane; intel_plane->disable_plane = vlv_disable_plane; + intel_plane->get_hw_state = vlv_plane_get_hw_state; plane_formats = vlv_plane_formats; num_plane_formats = ARRAY_SIZE(vlv_plane_formats); } else { intel_plane->update_plane = ivb_update_plane; intel_plane->disable_plane = ivb_disable_plane; + intel_plane->get_hw_state = ivb_plane_get_hw_state; plane_formats = snb_plane_formats; num_plane_formats = ARRAY_SIZE(snb_plane_formats); @@ -1113,6 +1149,7 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane) intel_plane->can_scale = true; intel_plane->update_plane = skl_update_plane; intel_plane->disable_plane = skl_disable_plane; + intel_plane->get_hw_state = skl_plane_get_hw_state; state->scaler_id = -1; plane_formats = skl_plane_formats;