From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 3/4] drm/i915: Add .get_hw_state() method for planes
Date: Thu, 10 Sep 2015 19:36:39 +0300 [thread overview]
Message-ID: <20150910163639.GR29811@intel.com> (raw)
In-Reply-To: <20150910163020.GB31002@phenom.ffwll.local>
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
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2015-09-10 16:36 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-09-10 15:59 [PATCH 1/4] drm/i915: Assign hwmode after encoder state readout ville.syrjala
2015-09-10 15:59 ` [PATCH 2/4] drm/i915: Move sprite/cursor plane disable to intel_sanitize_crtc() ville.syrjala
2015-09-14 11:48 ` Patrik Jakobsson
2015-09-10 15:59 ` [PATCH 3/4] drm/i915: Add .get_hw_state() method for planes ville.syrjala
2015-09-10 16:07 ` Daniel Vetter
2015-09-10 16:13 ` Ville Syrjälä
2015-09-10 16:20 ` Ville Syrjälä
2015-09-10 16:30 ` Daniel Vetter
2015-09-10 16:36 ` Ville Syrjälä [this message]
2015-09-14 9:18 ` Daniel Vetter
2015-09-14 12:10 ` Maarten Lankhorst
2015-09-14 13:15 ` Ville Syrjälä
2015-09-10 15:59 ` [PATCH 4/4] drm/i915: Move scanline_offset and timestamping constant setup to intel_modeset_readout_hw_state() ville.syrjala
2015-09-14 11:57 ` Maarten Lankhorst
2015-09-14 14:23 ` Ville Syrjälä
2015-09-21 12:25 ` Maarten Lankhorst
2015-09-14 12:04 ` Patrik Jakobsson
2015-09-23 8:33 ` Daniel Vetter
2015-09-14 11:36 ` [PATCH 1/4] drm/i915: Assign hwmode after encoder state readout Patrik Jakobsson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20150910163639.GR29811@intel.com \
--to=ville.syrjala@linux.intel.com \
--cc=daniel@ffwll.ch \
--cc=intel-gfx@lists.freedesktop.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox