On Thu, Nov 08, 2018 at 11:30:07AM -0800, Matt Roper wrote:
> On Wed, Nov 07, 2018 at 08:42:55PM +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> >
> > Some observations about the plane registers:
> > - the control register will self-arm if the plane is not already
> > enabled, thus we want to write it as close to (or ideally after)
> > the surface register
> > - tileoff/linoff/offset/aux_offset are self-arming as well so we want
> > them close to the surface register as well
> > - color keying registers we maybe self arming before SKL. Not 100%
> > sure but we can try to keep them near to the surface register
> > as well
> > - chv pipe b csc register are double buffered but self arming so
> > moving them down a bit
> > - the rest should be mostly armed by the surface register so we can
> > safely write them first, and to just for some consistency let's try
> > to follow keep them in order based on the register offset
>
> In general these functions all run under vblank evasion currently, so
> this won't have a visible effect on atomicity unless we fail the
> evasion, right?
Yes.
> Is the longer-term goal here to eventually pull a bunch
> of the register writing out and only evade around the actual arming
> registers?
That has been on my mind off and on. But I haven't actually
measured how much we'd gain. So not sure if it's worth the hassle
currently. We may need to think much harder on this when our mmio
becomes more expensive...
>
> >
> > v2: Rebase due to input CSC
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/intel_display.c | 40 +++++----
> > drivers/gpu/drm/i915/intel_sprite.c | 120 +++++++++++++++------------
> > 2 files changed, 89 insertions(+), 71 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index ae6d58dbf1ed..ac46497cfc52 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -3313,7 +3313,6 @@ static void i9xx_update_plane(struct intel_plane *plane,
> > enum i9xx_plane_id i9xx_plane = plane->i9xx_plane;
> > u32 linear_offset;
> > u32 dspcntr = plane_state->ctl;
> > - i915_reg_t reg = DSPCNTR(i9xx_plane);
> > int x = plane_state->color_plane[0].x;
> > int y = plane_state->color_plane[0].y;
> > unsigned long irqflags;
> > @@ -3328,41 +3327,45 @@ static void i9xx_update_plane(struct intel_plane *plane,
> >
> > spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> >
> > + I915_WRITE_FW(DSPSTRIDE(i9xx_plane), plane_state->color_plane[0].stride);
> > +
> > if (INTEL_GEN(dev_priv) < 4) {
> > /* pipesrc and dspsize control the size that is scaled from,
> > * which should always be the user's requested size.
> > */
> > + I915_WRITE_FW(DSPPOS(i9xx_plane), 0);
> > I915_WRITE_FW(DSPSIZE(i9xx_plane),
> > ((crtc_state->pipe_src_h - 1) << 16) |
> > (crtc_state->pipe_src_w - 1));
> > - I915_WRITE_FW(DSPPOS(i9xx_plane), 0);
> > } else if (IS_CHERRYVIEW(dev_priv) && i9xx_plane == PLANE_B) {
> > + I915_WRITE_FW(PRIMPOS(i9xx_plane), 0);
> > I915_WRITE_FW(PRIMSIZE(i9xx_plane),
> > ((crtc_state->pipe_src_h - 1) << 16) |
> > (crtc_state->pipe_src_w - 1));
> > - I915_WRITE_FW(PRIMPOS(i9xx_plane), 0);
> > I915_WRITE_FW(PRIMCNSTALPHA(i9xx_plane), 0);
> > }
> >
> > - I915_WRITE_FW(reg, dspcntr);
> > -
> > - I915_WRITE_FW(DSPSTRIDE(i9xx_plane), plane_state->color_plane[0].stride);
> > if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
> > - I915_WRITE_FW(DSPSURF(i9xx_plane),
> > - intel_plane_ggtt_offset(plane_state) +
> > - dspaddr_offset);
> > I915_WRITE_FW(DSPOFFSET(i9xx_plane), (y << 16) | x);
> > } else if (INTEL_GEN(dev_priv) >= 4) {
> > + I915_WRITE_FW(DSPTILEOFF(i9xx_plane), (y << 16) | x);
> > + I915_WRITE_FW(DSPLINOFF(i9xx_plane), linear_offset);
> > + }
> > +
> > + /*
> > + * The control register self-arms if the plane was previously
> > + * disabled. Try to make the plane enable atomic by writing
> > + * the control register just before the surface register.
> > + */
> > + I915_WRITE_FW(DSPCNTR(i9xx_plane), dspcntr);
> > + if (INTEL_GEN(dev_priv) >= 4)
> > I915_WRITE_FW(DSPSURF(i9xx_plane),
> > intel_plane_ggtt_offset(plane_state) +
> > dspaddr_offset);
> > - I915_WRITE_FW(DSPTILEOFF(i9xx_plane), (y << 16) | x);
> > - I915_WRITE_FW(DSPLINOFF(i9xx_plane), linear_offset);
> > - } else {
> > + else
> > I915_WRITE_FW(DSPADDR(i9xx_plane),
> > intel_plane_ggtt_offset(plane_state) +
> > dspaddr_offset);
> > - }
> >
> > spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> > }
> > @@ -10013,8 +10016,8 @@ static void i9xx_update_cursor(struct intel_plane *plane,
> > * On some platforms writing CURCNTR first will also
> > * cause CURPOS to be armed by the CURBASE write.
> > * Without the CURCNTR write the CURPOS write would
> > - * arm itself. Thus we always start the full update
> > - * with a CURCNTR write.
> > + * arm itself. Thus we always update CURCNTR before
> > + * CURPOS.
> > *
> > * On other platforms CURPOS always requires the
> > * CURBASE write to arm the update. Additonally
> > @@ -10024,15 +10027,16 @@ static void i9xx_update_cursor(struct intel_plane *plane,
> > * cursor that doesn't appear to move, or even change
> > * shape. Thus we always write CURBASE.
> > *
> > - * CURCNTR and CUR_FBC_CTL are always
> > - * armed by the CURBASE write only.
> > + * The other registers are armed by by the CURBASE write
> > + * except when the plane is getting enabled at which time
> > + * the CURCNTR write arms the update.
> > */
> > if (plane->cursor.base != base ||
> > plane->cursor.size != fbc_ctl ||
> > plane->cursor.cntl != cntl) {
> > - I915_WRITE_FW(CURCNTR(pipe), cntl);
> > if (HAS_CUR_FBC(dev_priv))
> > I915_WRITE_FW(CUR_FBC_CTL(pipe), fbc_ctl);
> > + I915_WRITE_FW(CURCNTR(pipe), cntl);
> > I915_WRITE_FW(CURPOS(pipe), pos);
> > I915_WRITE_FW(CURBASE(pipe), base);
> >
> > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > index facf7ca8f14f..be6e3c3de1ff 100644
> > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > @@ -500,27 +500,12 @@ skl_program_plane(struct intel_plane *plane,
> >
> > spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> >
> > - if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
> > - I915_WRITE_FW(PLANE_COLOR_CTL(pipe, plane_id),
> > - plane_state->color_ctl);
> > -
> > - if (fb->format->is_yuv && icl_is_hdr_plane(plane))
> > - icl_program_input_csc_coeff(crtc_state, plane_state);
> > -
> > - I915_WRITE_FW(PLANE_KEYVAL(pipe, plane_id), key->min_value);
> > - I915_WRITE_FW(PLANE_KEYMAX(pipe, plane_id), keymax);
> > - I915_WRITE_FW(PLANE_KEYMSK(pipe, plane_id), keymsk);
> > -
> > - I915_WRITE_FW(PLANE_OFFSET(pipe, plane_id), (y << 16) | x);
> > I915_WRITE_FW(PLANE_STRIDE(pipe, plane_id), stride);
> > + I915_WRITE_FW(PLANE_POS(pipe, plane_id), (crtc_y << 16) | crtc_x);
> > I915_WRITE_FW(PLANE_SIZE(pipe, plane_id), (src_h << 16) | src_w);
> > I915_WRITE_FW(PLANE_AUX_DIST(pipe, plane_id),
> > - (plane_state->color_plane[1].offset - surf_addr) | aux_stride);
> > -
> > - if (INTEL_GEN(dev_priv) < 11)
> > - I915_WRITE_FW(PLANE_AUX_OFFSET(pipe, plane_id),
> > - (plane_state->color_plane[1].y << 16) |
> > - plane_state->color_plane[1].x);
> > + (plane_state->color_plane[1].offset - surf_addr) |
> > + aux_stride);
> >
> > if (icl_is_hdr_plane(plane)) {
> > u32 cus_ctl = 0;
> > @@ -543,15 +528,36 @@ skl_program_plane(struct intel_plane *plane,
> > I915_WRITE_FW(PLANE_CUS_CTL(pipe, plane_id), cus_ctl);
> > }
> >
> > - if (!slave && plane_state->scaler_id >= 0)
> > - skl_program_scaler(plane, crtc_state, plane_state);
> > + if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
> > + I915_WRITE_FW(PLANE_COLOR_CTL(pipe, plane_id),
> > + plane_state->color_ctl);
> >
> > - I915_WRITE_FW(PLANE_POS(pipe, plane_id), (crtc_y << 16) | crtc_x);
> > + if (fb->format->is_yuv && icl_is_hdr_plane(plane))
> > + icl_program_input_csc_coeff(crtc_state, plane_state);
> > +
> > + I915_WRITE_FW(PLANE_KEYVAL(pipe, plane_id), key->min_value);
> > + I915_WRITE_FW(PLANE_KEYMAX(pipe, plane_id), keymax);
> > + I915_WRITE_FW(PLANE_KEYMSK(pipe, plane_id), keymsk);
>
> You mentioned above that you wanted to follow register offset ordering
> where possible, so I believe MSK should come before MAX here. Same
> comment for the vlv and g4x functions farther down.
>
> > +
> > + I915_WRITE_FW(PLANE_OFFSET(pipe, plane_id), (y << 16) | x);
> > +
> > + if (INTEL_GEN(dev_priv) < 11)
> > + I915_WRITE_FW(PLANE_AUX_OFFSET(pipe, plane_id),
> > + (plane_state->color_plane[1].y << 16) |
> > + plane_state->color_plane[1].x);
> >
> > + /*
> > + * The control register self-arms if the plane was previously
> > + * disabled. Try to make the plane enable atomic by writing
> > + * the control register just before the surface register.
> > + */
> > I915_WRITE_FW(PLANE_CTL(pipe, plane_id), plane_ctl);
> > I915_WRITE_FW(PLANE_SURF(pipe, plane_id),
> > intel_plane_ggtt_offset(plane_state) + surf_addr);
> >
> > + if (!slave && plane_state->scaler_id >= 0)
> > + skl_program_scaler(plane, crtc_state, plane_state);
> > +
> > spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> > }
> >
> > @@ -789,7 +795,6 @@ vlv_update_plane(struct intel_plane *plane,
> > const struct intel_plane_state *plane_state)
> > {
> > struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > - const struct drm_framebuffer *fb = plane_state->base.fb;
> > enum pipe pipe = plane->pipe;
> > enum plane_id plane_id = plane->id;
> > u32 sprctl = plane_state->ctl;
> > @@ -814,6 +819,12 @@ vlv_update_plane(struct intel_plane *plane,
> >
> > vlv_update_clrc(plane_state);
> >
> > + I915_WRITE_FW(SPSTRIDE(pipe, plane_id),
> > + plane_state->color_plane[0].stride);
> > + I915_WRITE_FW(SPPOS(pipe, plane_id), (crtc_y << 16) | crtc_x);
> > + I915_WRITE_FW(SPSIZE(pipe, plane_id), (crtc_h << 16) | crtc_w);
> > + I915_WRITE_FW(SPCONSTALPHA(pipe, plane_id), 0);
> > +
> > if (IS_CHERRYVIEW(dev_priv) && pipe == PIPE_B)
> > chv_update_csc(plane_state);
> >
> > @@ -822,18 +833,15 @@ vlv_update_plane(struct intel_plane *plane,
> > I915_WRITE_FW(SPKEYMAXVAL(pipe, plane_id), key->max_value);
> > I915_WRITE_FW(SPKEYMSK(pipe, plane_id), key->channel_mask);
> > }
> > - I915_WRITE_FW(SPSTRIDE(pipe, plane_id),
> > - plane_state->color_plane[0].stride);
> > - I915_WRITE_FW(SPPOS(pipe, plane_id), (crtc_y << 16) | crtc_x);
> > -
> > - if (fb->modifier == I915_FORMAT_MOD_X_TILED)
> > - I915_WRITE_FW(SPTILEOFF(pipe, plane_id), (y << 16) | x);
> > - else
> > - I915_WRITE_FW(SPLINOFF(pipe, plane_id), linear_offset);
> >
> > - I915_WRITE_FW(SPCONSTALPHA(pipe, plane_id), 0);
> > + I915_WRITE_FW(SPTILEOFF(pipe, plane_id), (y << 16) | x);
> > + I915_WRITE_FW(SPLINOFF(pipe, plane_id), linear_offset);
>
> I think you already indicated you were going to pull the "ignore tiling
> and just write both offset registers" out into a separate patch, but
> these are another case where you might want to flip the order to match
> register offset (linear < tiled).
Sure. It looks like my ocd wasn't quite strong enough to follow
my own rule 100% :)
>
>
> Matt
>
> >
> > - I915_WRITE_FW(SPSIZE(pipe, plane_id), (crtc_h << 16) | crtc_w);
> > + /*
> > + * The control register self-arms if the plane was previously
> > + * disabled. Try to make the plane enable atomic by writing
> > + * the control register just before the surface register.
> > + */
> > I915_WRITE_FW(SPCNTR(pipe, plane_id), sprctl);
> > I915_WRITE_FW(SPSURF(pipe, plane_id),
> > intel_plane_ggtt_offset(plane_state) + sprsurf_offset);
> > @@ -947,7 +955,6 @@ ivb_update_plane(struct intel_plane *plane,
> > const struct intel_plane_state *plane_state)
> > {
> > struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > - const struct drm_framebuffer *fb = plane_state->base.fb;
> > enum pipe pipe = plane->pipe;
> > u32 sprctl = plane_state->ctl, sprscale = 0;
> > u32 sprsurf_offset = plane_state->color_plane[0].offset;
> > @@ -976,27 +983,32 @@ ivb_update_plane(struct intel_plane *plane,
> >
> > spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> >
> > + I915_WRITE_FW(SPRSTRIDE(pipe), plane_state->color_plane[0].stride);
> > + I915_WRITE_FW(SPRPOS(pipe), (crtc_y << 16) | crtc_x);
> > + I915_WRITE_FW(SPRSIZE(pipe), (crtc_h << 16) | crtc_w);
> > + if (IS_IVYBRIDGE(dev_priv))
> > + I915_WRITE_FW(SPRSCALE(pipe), sprscale);
> > +
> > if (key->flags) {
> > I915_WRITE_FW(SPRKEYVAL(pipe), key->min_value);
> > I915_WRITE_FW(SPRKEYMAX(pipe), key->max_value);
> > I915_WRITE_FW(SPRKEYMSK(pipe), key->channel_mask);
> > }
> >
> > - I915_WRITE_FW(SPRSTRIDE(pipe), plane_state->color_plane[0].stride);
> > - I915_WRITE_FW(SPRPOS(pipe), (crtc_y << 16) | crtc_x);
> > -
> > /* HSW consolidates SPRTILEOFF and SPRLINOFF into a single SPROFFSET
> > * register */
> > - if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
> > + if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
> > I915_WRITE_FW(SPROFFSET(pipe), (y << 16) | x);
> > - else if (fb->modifier == I915_FORMAT_MOD_X_TILED)
> > + } else {
> > I915_WRITE_FW(SPRTILEOFF(pipe), (y << 16) | x);
> > - else
> > I915_WRITE_FW(SPRLINOFF(pipe), linear_offset);
> > + }
> >
> > - I915_WRITE_FW(SPRSIZE(pipe), (crtc_h << 16) | crtc_w);
> > - if (IS_IVYBRIDGE(dev_priv))
> > - I915_WRITE_FW(SPRSCALE(pipe), sprscale);
> > + /*
> > + * The control register self-arms if the plane was previously
> > + * disabled. Try to make the plane enable atomic by writing
> > + * the control register just before the surface register.
> > + */
> > I915_WRITE_FW(SPRCTL(pipe), sprctl);
> > I915_WRITE_FW(SPRSURF(pipe),
> > intel_plane_ggtt_offset(plane_state) + sprsurf_offset);
> > @@ -1014,7 +1026,7 @@ ivb_disable_plane(struct intel_plane *plane, struct intel_crtc *crtc)
> > spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> >
> > I915_WRITE_FW(SPRCTL(pipe), 0);
> > - /* Can't leave the scaler enabled... */
> > + /* Disable the scaler */
> > if (IS_IVYBRIDGE(dev_priv))
> > I915_WRITE_FW(SPRSCALE(pipe), 0);
> > I915_WRITE_FW(SPRSURF(pipe), 0);
> > @@ -1116,7 +1128,6 @@ g4x_update_plane(struct intel_plane *plane,
> > const struct intel_plane_state *plane_state)
> > {
> > struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > - const struct drm_framebuffer *fb = plane_state->base.fb;
> > enum pipe pipe = plane->pipe;
> > u32 dvscntr = plane_state->ctl, dvsscale = 0;
> > u32 dvssurf_offset = plane_state->color_plane[0].offset;
> > @@ -1145,22 +1156,25 @@ g4x_update_plane(struct intel_plane *plane,
> >
> > spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> >
> > + I915_WRITE_FW(DVSSTRIDE(pipe), plane_state->color_plane[0].stride);
> > + I915_WRITE_FW(DVSPOS(pipe), (crtc_y << 16) | crtc_x);
> > + I915_WRITE_FW(DVSSIZE(pipe), (crtc_h << 16) | crtc_w);
> > + I915_WRITE_FW(DVSSCALE(pipe), dvsscale);
> > +
> > if (key->flags) {
> > I915_WRITE_FW(DVSKEYVAL(pipe), key->min_value);
> > I915_WRITE_FW(DVSKEYMAX(pipe), key->max_value);
> > I915_WRITE_FW(DVSKEYMSK(pipe), key->channel_mask);
> > }
> >
> > - I915_WRITE_FW(DVSSTRIDE(pipe), plane_state->color_plane[0].stride);
> > - I915_WRITE_FW(DVSPOS(pipe), (crtc_y << 16) | crtc_x);
> > + I915_WRITE_FW(DVSTILEOFF(pipe), (y << 16) | x);
> > + I915_WRITE_FW(DVSLINOFF(pipe), linear_offset);
> >
> > - if (fb->modifier == I915_FORMAT_MOD_X_TILED)
> > - I915_WRITE_FW(DVSTILEOFF(pipe), (y << 16) | x);
> > - else
> > - I915_WRITE_FW(DVSLINOFF(pipe), linear_offset);
> > -
> > - I915_WRITE_FW(DVSSIZE(pipe), (crtc_h << 16) | crtc_w);
> > - I915_WRITE_FW(DVSSCALE(pipe), dvsscale);
> > + /*
> > + * The control register self-arms if the plane was previously
> > + * disabled. Try to make the plane enable atomic by writing
> > + * the control register just before the surface register.
> > + */
> > I915_WRITE_FW(DVSCNTR(pipe), dvscntr);
> > I915_WRITE_FW(DVSSURF(pipe),
> > intel_plane_ggtt_offset(plane_state) + dvssurf_offset);
> > --
> > 2.18.1
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
