On Fri, Oct 13, 2017 at 01:35:05PM +0300, Ville Syrjälä wrote:
> On Thu, Oct 12, 2017 at 09:06:24PM +0200, Daniel Vetter wrote:
> > On Wed, Oct 11, 2017 at 07:04:49PM +0300, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > >
> > > Rename enum plane to enum old_plane_id to make it clear that it only
> > > applies to pre-SKL platforms.
> > >
> > > v2: Reorder patches
> > >
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> >
> > old_ sounds to me like the previous in a temporal aka at runtime sense.
> > Which is confusing, why exactly do we want to have different enums for the
> > current vs. previous plane id?
>
> The "old" ID is a global identifier, the "new" one is per-pipe. We need
> the old one to index the primary plane registers on pre-skl. I've not yet
> entirely figured out how we should handle planes that can move between
> pipes (on pre-g4x) when it comes to the per-pipe identifier. But for now
> I'm happy to defer that until I really need to ;)
>
> >
> > This needs a better prefix, please pick one of:
> >
> > i8xx_
> > i9xx_
> > i915_
> > legacy_
>
> legacy_ is what I proposed previously, but I don't particularly like it
> on account of its length. i9xx_ might be a decent choice actually, since
> all the pre-skl (primary) plane functions are now called i9xx_ as well.
> I'll respin with that, unless someone has a better idea?
+1 from me on i9xx_, seems like the best choice. I just tried to list all
the ones that crossed my mind.
-Daniel
>
> >
> > Since you stare at this code way more than I do, pls pick the one you
> > think is most consistent.
> >
> > > ---
> > > drivers/gpu/drm/i915/i915_drv.h | 4 +-
> > > drivers/gpu/drm/i915/intel_display.c | 84 ++++++++++++++++++------------------
> > > drivers/gpu/drm/i915/intel_drv.h | 6 +--
> > > 3 files changed, 47 insertions(+), 47 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > index 6bbc4b83aa0a..7280f9eb2e95 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -307,7 +307,7 @@ 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+.
> > > */
> > > -enum plane {
> > > +enum old_plane_id {
> > > PLANE_A,
> > > PLANE_B,
> > > PLANE_C,
> > > @@ -1128,7 +1128,7 @@ struct intel_fbc {
> > >
> > > struct {
> > > enum pipe pipe;
> > > - enum plane plane;
> > > + enum old_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 a9fd3b8fa922..9d37c758f7b5 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -3223,17 +3223,17 @@ 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);
> > > struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> > > const struct drm_framebuffer *fb = plane_state->base.fb;
> > > - enum plane plane = primary->plane;
> > > + enum old_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;
> > > @@ -3254,34 +3254,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) +
> > > crtc->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) +
> > > crtc->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) +
> > > crtc->dspaddr_offset);
> > > }
> > > @@ -3290,31 +3290,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 old_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)
> > > + 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);
> > > - enum plane plane = primary->plane;
> > > + struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > > + enum old_plane_id plane_id = plane->plane;
> > >
> > > - return I915_READ(DSPCNTR(plane)) & DISPLAY_PLANE_ENABLE;
> > > + return I915_READ(DSPCNTR(plane_id)) & DISPLAY_PLANE_ENABLE;
> > > }
> > >
> > > static u32
> > > @@ -13195,9 +13195,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 old_plane_id) !pipe;
> > > else
> > > - primary->plane = (enum plane) pipe;
> > > + primary->plane = (enum old_plane_id) pipe;
> > > primary->id = PLANE_PRIMARY;
> > > primary->frontbuffer_bit = INTEL_FRONTBUFFER_PRIMARY(pipe);
> > > primary->check_plane = intel_check_primary_plane;
> > > @@ -13226,16 +13226,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;
> >
> > Misplace hunk I presume, presumable in the patch reordering. Please split
> > out.
> >
> > With those two bikesheds addressed:
> >
> > Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
> >
> > (for both of the resulting patches, assuming you still appease CI and gcc
> > and the 2nd patch has a reasonable commit message ofc).
> >
> > Cheers, Daniel
> >
> > > }
> > >
> > > @@ -13319,7 +13319,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 old_plane_id) pipe;
> > > cursor->id = PLANE_CURSOR;
> > > cursor->frontbuffer_bit = INTEL_FRONTBUFFER_CURSOR(pipe);
> > >
> > > @@ -14693,11 +14693,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 old_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 24bbf0518473..08318260453b 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -793,7 +793,7 @@ struct intel_crtc_state {
> > > struct intel_crtc {
> > > struct drm_crtc base;
> > > enum pipe pipe;
> > > - enum plane plane;
> > > + enum old_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
> > > @@ -846,7 +846,7 @@ struct intel_crtc {
> > >
> > > struct intel_plane {
> > > struct drm_plane base;
> > > - u8 plane;
> > > + enum old_plane_id plane;
> > > enum plane_id id;
> > > enum pipe pipe;
> > > bool can_scale;
> > > @@ -1133,7 +1133,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 old_plane_id plane)
> > > {
> > > return dev_priv->plane_to_crtc_mapping[plane];
> > > }
> > > --
> > > 2.13.6
> > >
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
>
> --
> Ville Syrjälä
> Intel OTC
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
