Re: [Intel-gfx] [PATCH v5 03/10] drm/i915: s/enum plane/enum i9xx_plane_id/
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ä
> >
> > 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
> > Signed-off-by: Ville Syrjälä
> > ---
> > 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));
> > -
Re: [Intel-gfx] [PATCH v5 03/10] drm/i915: s/enum plane/enum i9xx_plane_id/
On Mon, Oct 23, 2017 at 05:50:32PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä
>
> 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
> Signed-off-by: Ville Syrjälä
> ---
> 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" 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
such (both in struct intel_plane and in struct intel_fbc->crtc)? 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.
> 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
[Intel-gfx] [PATCH v5 03/10] drm/i915: s/enum plane/enum i9xx_plane_id/
From: Ville Syrjälä
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
Signed-off-by: Ville Syrjälä
---
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;
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
