From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: James Ausmus <james.ausmus@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH v5 03/10] drm/i915: s/enum plane/enum i9xx_plane_id/
Date: Fri, 17 Nov 2017 17:41:01 +0200 [thread overview]
Message-ID: <20171117154101.GL10981@intel.com> (raw)
In-Reply-To: <20171116232131.GB6676@jausmus-gentoo-dev6.jf.intel.com>
On Thu, Nov 16, 2017 at 03:21:32PM -0800, James Ausmus wrote:
> On Mon, Oct 23, 2017 at 05:50:32PM +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Rename enum plane to enum i9xx_plane_id to make it clear that it only
> > applies to pre-SKL platforms.
> >
> > enum i9xx_plane_id is a global identifier, whereas enum plane_id is
> > per-pipe. We need the old global identifier to index the primary plane
> > (and the pre-g4x sprite C if we ever expose it) registers on pre-SKL
> > platforms.
> >
> > v2: Reorder patches
> > v3: s/old_plane_id/i9xx_plane_id/ (Daniel)
> > Pimp the commit message a bit
> > Note that i9xx_plane_id doesn't apply to SKL+
> > v4: Rebase due to power domain handling in plane readout
> > v5: Rebase due to crtc->dspaddr_offset removal
> >
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> > drivers/gpu/drm/i915/i915_drv.h | 6 +--
> > drivers/gpu/drm/i915/intel_display.c | 87 ++++++++++++++++++------------------
> > drivers/gpu/drm/i915/intel_drv.h | 6 +--
> > 3 files changed, 49 insertions(+), 50 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 54b5d4c582b6..a6b912c646f9 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -305,9 +305,9 @@ static inline bool transcoder_is_dsi(enum transcoder transcoder)
> >
> > /*
> > * Global legacy plane identifier. Valid only for primary/sprite
> > - * planes on pre-g4x, and only for primary planes on g4x+.
> > + * planes on pre-g4x, and only for primary planes on g4x-bdw.
> > */
> > -enum plane {
> > +enum i9xx_plane_id {
> > PLANE_A,
> > PLANE_B,
> > PLANE_C,
> > @@ -1137,7 +1137,7 @@ struct intel_fbc {
> >
> > struct {
> > enum pipe pipe;
> > - enum plane plane;
> > + enum i9xx_plane_id plane;
> > unsigned int fence_y_offset;
> > } crtc;
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 4ea0f1ef249e..e726b65588aa 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -3223,16 +3223,16 @@ int i9xx_check_plane_surface(struct intel_plane_state *plane_state)
> > return 0;
> > }
> >
> > -static void i9xx_update_primary_plane(struct intel_plane *primary,
> > - const struct intel_crtc_state *crtc_state,
> > - const struct intel_plane_state *plane_state)
> > +static void i9xx_update_plane(struct intel_plane *plane,
> > + const struct intel_crtc_state *crtc_state,
> > + const struct intel_plane_state *plane_state)
> > {
> > - struct drm_i915_private *dev_priv = to_i915(primary->base.dev);
> > + struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > const struct drm_framebuffer *fb = plane_state->base.fb;
> > - enum plane plane = primary->plane;
> > + enum i9xx_plane_id plane_id = plane->plane;
>
> It feels a bit ugly and counter-intuitive to have the two "plane"s in
> "plane->plane"
It's always been like that. Well, ever since we had planes.
Nothing new there. At least I got rid of the magic
'plane->plane + 1' from the sprite code.
> be different types - since i9xx_plane_id is a global id,
> would it make sense to change the member naming to plane_gid or some
"gid" would confuse me more. It makes me think of uid/gid. We could name
it to plane->i9xx_plane[_id] I suppose to match the type.
> such (both in struct intel_plane and in struct intel_fbc->crtc)?
The fbc mess is going away thankfully. At which point the uses of
i9xx_plane_id will be tucked away neatly in platform specific code
instead of leaking too badly to common code.
> It
> feels like struct intel_plane should continue to be "plane", but we need
> something else for enum i9xx_plane_id just for clarity's sake.
This is I think the third attempt at coming up with something.
I might just have to rename plane->plane to plane->bikeshed to more
accurately reflect its role in i915 development ;)
>
> > u32 linear_offset;
> > u32 dspcntr = plane_state->ctl;
> > - i915_reg_t reg = DSPCNTR(plane);
> > + i915_reg_t reg = DSPCNTR(plane_id);
> > int x = plane_state->main.x;
> > int y = plane_state->main.y;
> > unsigned long irqflags;
> > @@ -3251,34 +3251,34 @@ static void i9xx_update_primary_plane(struct intel_plane *primary,
> > /* pipesrc and dspsize control the size that is scaled from,
> > * which should always be the user's requested size.
> > */
> > - I915_WRITE_FW(DSPSIZE(plane),
> > + I915_WRITE_FW(DSPSIZE(plane_id),
> > ((crtc_state->pipe_src_h - 1) << 16) |
> > (crtc_state->pipe_src_w - 1));
> > - I915_WRITE_FW(DSPPOS(plane), 0);
> > - } else if (IS_CHERRYVIEW(dev_priv) && plane == PLANE_B) {
> > - I915_WRITE_FW(PRIMSIZE(plane),
> > + I915_WRITE_FW(DSPPOS(plane_id), 0);
> > + } else if (IS_CHERRYVIEW(dev_priv) && plane_id == PLANE_B) {
> > + I915_WRITE_FW(PRIMSIZE(plane_id),
> > ((crtc_state->pipe_src_h - 1) << 16) |
> > (crtc_state->pipe_src_w - 1));
> > - I915_WRITE_FW(PRIMPOS(plane), 0);
> > - I915_WRITE_FW(PRIMCNSTALPHA(plane), 0);
> > + I915_WRITE_FW(PRIMPOS(plane_id), 0);
> > + I915_WRITE_FW(PRIMCNSTALPHA(plane_id), 0);
> > }
> >
> > I915_WRITE_FW(reg, dspcntr);
> >
> > - I915_WRITE_FW(DSPSTRIDE(plane), fb->pitches[0]);
> > + I915_WRITE_FW(DSPSTRIDE(plane_id), fb->pitches[0]);
> > if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
> > - I915_WRITE_FW(DSPSURF(plane),
> > + I915_WRITE_FW(DSPSURF(plane_id),
> > intel_plane_ggtt_offset(plane_state) +
> > dspaddr_offset);
> > - I915_WRITE_FW(DSPOFFSET(plane), (y << 16) | x);
> > + I915_WRITE_FW(DSPOFFSET(plane_id), (y << 16) | x);
> > } else if (INTEL_GEN(dev_priv) >= 4) {
> > - I915_WRITE_FW(DSPSURF(plane),
> > + I915_WRITE_FW(DSPSURF(plane_id),
> > intel_plane_ggtt_offset(plane_state) +
> > dspaddr_offset);
> > - I915_WRITE_FW(DSPTILEOFF(plane), (y << 16) | x);
> > - I915_WRITE_FW(DSPLINOFF(plane), linear_offset);
> > + I915_WRITE_FW(DSPTILEOFF(plane_id), (y << 16) | x);
> > + I915_WRITE_FW(DSPLINOFF(plane_id), linear_offset);
> > } else {
> > - I915_WRITE_FW(DSPADDR(plane),
> > + I915_WRITE_FW(DSPADDR(plane_id),
> > intel_plane_ggtt_offset(plane_state) +
> > dspaddr_offset);
> > }
> > @@ -3287,32 +3287,31 @@ static void i9xx_update_primary_plane(struct intel_plane *primary,
> > spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> > }
> >
> > -static void i9xx_disable_primary_plane(struct intel_plane *primary,
> > - struct intel_crtc *crtc)
> > +static void i9xx_disable_plane(struct intel_plane *plane,
> > + struct intel_crtc *crtc)
> > {
> > - struct drm_i915_private *dev_priv = to_i915(primary->base.dev);
> > - enum plane plane = primary->plane;
> > + struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > + enum i9xx_plane_id plane_id = plane->plane;
> > unsigned long irqflags;
> >
> > spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> >
> > - I915_WRITE_FW(DSPCNTR(plane), 0);
> > - if (INTEL_INFO(dev_priv)->gen >= 4)
> > - I915_WRITE_FW(DSPSURF(plane), 0);
> > + I915_WRITE_FW(DSPCNTR(plane_id), 0);
> > + if (INTEL_GEN(dev_priv) >= 4)
>
> Nit: unrelated change/cleanup that's not mentioned in the commit message
>
> > + I915_WRITE_FW(DSPSURF(plane_id), 0);
> > else
> > - I915_WRITE_FW(DSPADDR(plane), 0);
> > - POSTING_READ_FW(DSPCNTR(plane));
> > + I915_WRITE_FW(DSPADDR(plane_id), 0);
> > + POSTING_READ_FW(DSPCNTR(plane_id));
> >
> > spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> > }
> >
> > -static bool i9xx_plane_get_hw_state(struct intel_plane *primary)
> > +static bool i9xx_plane_get_hw_state(struct intel_plane *plane)
> > {
> > -
> > - struct drm_i915_private *dev_priv = to_i915(primary->base.dev);
> > + struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > enum intel_display_power_domain power_domain;
> > - enum plane plane = primary->plane;
> > - enum pipe pipe = primary->pipe;
> > + enum i9xx_plane_id plane_id = plane->plane;
> > + enum pipe pipe = plane->pipe;
> > bool ret;
> >
> > /*
> > @@ -3324,7 +3323,7 @@ static bool i9xx_plane_get_hw_state(struct intel_plane *primary)
> > if (!intel_display_power_get_if_enabled(dev_priv, power_domain))
> > return false;
> >
> > - ret = I915_READ(DSPCNTR(plane)) & DISPLAY_PLANE_ENABLE;
> > + ret = I915_READ(DSPCNTR(plane_id)) & DISPLAY_PLANE_ENABLE;
> >
> > intel_display_power_put(dev_priv, power_domain);
> >
> > @@ -13176,9 +13175,9 @@ intel_primary_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe)
> > * port is hooked to pipe B. Hence we want plane A feeding pipe B.
> > */
> > if (HAS_FBC(dev_priv) && INTEL_GEN(dev_priv) < 4)
> > - primary->plane = (enum plane) !pipe;
> > + primary->plane = (enum i9xx_plane_id) !pipe;
> > else
> > - primary->plane = (enum plane) pipe;
> > + primary->plane = (enum i9xx_plane_id) pipe;
> > primary->id = PLANE_PRIMARY;
> > primary->frontbuffer_bit = INTEL_FRONTBUFFER_PRIMARY(pipe);
> > primary->check_plane = intel_check_primary_plane;
> > @@ -13207,16 +13206,16 @@ intel_primary_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe)
> > num_formats = ARRAY_SIZE(i965_primary_formats);
> > modifiers = i9xx_format_modifiers;
> >
> > - primary->update_plane = i9xx_update_primary_plane;
> > - primary->disable_plane = i9xx_disable_primary_plane;
> > + primary->update_plane = i9xx_update_plane;
> > + primary->disable_plane = i9xx_disable_plane;
> > primary->get_hw_state = i9xx_plane_get_hw_state;
> > } else {
> > intel_primary_formats = i8xx_primary_formats;
> > num_formats = ARRAY_SIZE(i8xx_primary_formats);
> > modifiers = i9xx_format_modifiers;
> >
> > - primary->update_plane = i9xx_update_primary_plane;
> > - primary->disable_plane = i9xx_disable_primary_plane;
> > + primary->update_plane = i9xx_update_plane;
> > + primary->disable_plane = i9xx_disable_plane;
> > primary->get_hw_state = i9xx_plane_get_hw_state;
> > }
> >
> > @@ -13300,7 +13299,7 @@ intel_cursor_plane_create(struct drm_i915_private *dev_priv,
> > cursor->can_scale = false;
> > cursor->max_downscale = 1;
> > cursor->pipe = pipe;
> > - cursor->plane = pipe;
> > + cursor->plane = (enum i9xx_plane_id) pipe;
> > cursor->id = PLANE_CURSOR;
> > cursor->frontbuffer_bit = INTEL_FRONTBUFFER_CURSOR(pipe);
> >
> > @@ -14674,11 +14673,11 @@ void i830_disable_pipe(struct drm_i915_private *dev_priv, enum pipe pipe)
> > }
> >
> > static bool intel_plane_mapping_ok(struct intel_crtc *crtc,
> > - struct intel_plane *primary)
> > + struct intel_plane *plane)
> > {
> > struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > - enum plane plane = primary->plane;
> > - u32 val = I915_READ(DSPCNTR(plane));
> > + enum i9xx_plane_id plane_id = plane->plane;
> > + u32 val = I915_READ(DSPCNTR(plane_id));
> >
> > return (val & DISPLAY_PLANE_ENABLE) == 0 ||
> > (val & DISPPLANE_SEL_PIPE_MASK) == DISPPLANE_SEL_PIPE(crtc->pipe);
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 07ba376c6fff..8e20924ab9df 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -796,7 +796,7 @@ struct intel_crtc_state {
> > struct intel_crtc {
> > struct drm_crtc base;
> > enum pipe pipe;
> > - enum plane plane;
> > + enum i9xx_plane_id plane;
> > /*
> > * Whether the crtc and the connected output pipeline is active. Implies
> > * that crtc->enabled is set, i.e. the current mode configuration has
> > @@ -841,7 +841,7 @@ struct intel_crtc {
> >
> > struct intel_plane {
> > struct drm_plane base;
> > - u8 plane;
> > + enum i9xx_plane_id plane;
> > enum plane_id id;
> > enum pipe pipe;
> > bool can_scale;
> > @@ -1128,7 +1128,7 @@ intel_get_crtc_for_pipe(struct drm_i915_private *dev_priv, enum pipe pipe)
> > }
> >
> > static inline struct intel_crtc *
> > -intel_get_crtc_for_plane(struct drm_i915_private *dev_priv, enum plane plane)
> > +intel_get_crtc_for_plane(struct drm_i915_private *dev_priv, enum i9xx_plane_id plane)
> > {
> > return dev_priv->plane_to_crtc_mapping[plane];
> > }
> > --
> > 2.13.6
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2017-11-17 15:41 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-10-13 13:58 [PATCH v2 00/10] drm/i915: Plane assert/readout cleanups etc Ville Syrjala
2017-10-13 13:58 ` [PATCH v2 01/10] drm/i915: Add .get_hw_state() method for planes Ville Syrjala
2017-10-13 17:36 ` [PATCH v3 " Ville Syrjala
2017-10-23 14:50 ` [PATCH v4 " Ville Syrjala
2017-10-13 13:58 ` [PATCH v3 02/10] drm/i915: Redo plane sanitation during readout Ville Syrjala
2017-10-13 13:58 ` [PATCH v3 03/10] drm/i915: s/enum plane/enum i9xx_plane_id/ Ville Syrjala
2017-10-13 17:37 ` [PATCH v4 " Ville Syrjala
2017-10-23 14:50 ` [PATCH v5 " Ville Syrjala
2017-11-16 23:21 ` James Ausmus
2017-11-17 15:41 ` Ville Syrjälä [this message]
2017-10-13 13:58 ` [PATCH v2 04/10] drm/i915: Use enum i9xx_plane_id for the .get_fifo_size() hooks Ville Syrjala
2017-10-13 13:58 ` [PATCH v3 05/10] drm/i915: Cleanup enum pipe/enum plane_id/enum i9xx_plane_id in initial fb readout Ville Syrjala
2017-10-13 13:58 ` [PATCH 06/10] drm/i915: Nuke ironlake_get_initial_plane_config() Ville Syrjala
2017-10-13 13:58 ` [PATCH 07/10] drm/i915: Switch fbc over to for_each_new_intel_plane_in_state() Ville Syrjala
2017-10-13 13:58 ` [PATCH v2 08/10] drm/i915: Nuke crtc->plane Ville Syrjala
2017-10-23 14:51 ` [PATCH v3 " Ville Syrjala
2017-10-13 13:58 ` [PATCH 09/10] drm/i915: Use plane->get_hw_state() for initial plane fb readout Ville Syrjala
2017-11-16 23:49 ` James Ausmus
2017-10-13 13:58 ` [PATCH 10/10] drm/i915: Add rudimentary plane state verification Ville Syrjala
2017-11-02 16:38 ` [PATCH v2 " Ville Syrjala
2017-11-17 0:07 ` James Ausmus
2017-10-13 16:24 ` ✗ Fi.CI.BAT: warning for drm/i915: Plane assert/readout cleanups etc. (rev2) Patchwork
2017-10-13 16:53 ` Ville Syrjälä
2017-10-13 18:05 ` ✓ Fi.CI.BAT: success for drm/i915: Plane assert/readout cleanups etc. (rev4) Patchwork
2017-10-13 23:20 ` ✗ Fi.CI.IGT: warning for drm/i915: Plane assert/readout cleanups etc. (rev2) Patchwork
2017-10-14 2:35 ` ✗ Fi.CI.IGT: failure for drm/i915: Plane assert/readout cleanups etc. (rev4) Patchwork
2017-10-23 15:10 ` ✓ Fi.CI.BAT: success for drm/i915: Plane assert/readout cleanups etc. (rev7) Patchwork
2017-10-23 16:09 ` ✓ Fi.CI.IGT: " Patchwork
2017-11-02 17:06 ` ✓ Fi.CI.BAT: success for drm/i915: Plane assert/readout cleanups etc. (rev8) Patchwork
2017-11-02 18:13 ` ✓ Fi.CI.IGT: " Patchwork
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=20171117154101.GL10981@intel.com \
--to=ville.syrjala@linux.intel.com \
--cc=intel-gfx@lists.freedesktop.org \
--cc=james.ausmus@intel.com \
/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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.