| Message ID | 20170327185546.2977-16-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Mar 27, 2017 at 09:55:46PM +0300, ville.syrjala@linux.intel.com wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > It looks like simply writing all the cursor register every single > time might be slightly faster than checking to see of each of > them need to be written. So if any other register apart from > CURPOS needs to be written let's just write all the registers. > > CURPOS is left as a special case mainly for 845/865 where we have to > disable the cursor to change many of the cursor parameters. This > introduces a slight chance of the cursor flickering when things get > updated (since we're not currently doing the vblank evade for cursor > updates). If we write CURPOS alone then that obviously can't happen. > And let's follow the same pattern in the i9xx code just for symmetry. > I wasn't able to see a singificant performance difference between > this and just writing all the registers unconditionally. > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Reviewed-by: Imre Deak <imre.deak@intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 69 ++++++++++++++++++------------------ > 1 file changed, 34 insertions(+), 35 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index d78ab0d35274..40ac0f938a4e 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -9323,36 +9323,28 @@ static void i845_update_cursor(struct intel_plane *plane, > > spin_lock_irqsave(&dev_priv->uncore.lock, irqflags); > > - if (plane->cursor.cntl != 0 && > - (plane->cursor.base != base || > - plane->cursor.size != size || > - plane->cursor.cntl != cntl)) { > - /* On these chipsets we can only modify the base/size/stride > - * whilst the cursor is disabled. > - */ > + /* On these chipsets we can only modify the base/size/stride > + * whilst the cursor is disabled. > + */ > + if (plane->cursor.base != base || > + plane->cursor.size != size || > + plane->cursor.cntl != cntl) { > I915_WRITE_FW(CURCNTR(PIPE_A), 0); > - plane->cursor.cntl = 0; > - } > - > - if (plane->cursor.base != base) > I915_WRITE_FW(CURBASE(PIPE_A), base); > - > - if (plane->cursor.size != size) > I915_WRITE_FW(CURSIZE, size); > - > - if (cntl) > I915_WRITE_FW(CURPOS(PIPE_A), pos); > - > - if (plane->cursor.cntl != cntl) > I915_WRITE_FW(CURCNTR(PIPE_A), cntl); > > + plane->cursor.base = base; > + plane->cursor.size = size; > + plane->cursor.cntl = cntl; > + } else { > + I915_WRITE_FW(CURPOS(PIPE_A), pos); > + } > + > POSTING_READ_FW(CURCNTR(PIPE_A)); > > spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags); > - > - plane->cursor.cntl = cntl; > - plane->cursor.base = base; > - plane->cursor.size = size; > } > > static void i845_disable_cursor(struct intel_plane *plane, > @@ -9508,27 +9500,34 @@ static void i9xx_update_cursor(struct intel_plane *plane, > > spin_lock_irqsave(&dev_priv->uncore.lock, irqflags); > > - if (plane->cursor.cntl != cntl) > + /* > + * 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. > + * > + * CURCNTR and CUR_FBC_CTL are always > + * armed by the CURBASE write only. > + */ > + if (plane->cursor.base != base || > + plane->cursor.size != fbc_ctl || > + plane->cursor.cntl != cntl) { > I915_WRITE_FW(CURCNTR(pipe), cntl); > - > - if (plane->cursor.size != fbc_ctl) > - I915_WRITE_FW(CUR_FBC_CTL(pipe), fbc_ctl); > - > - if (cntl) > + if (HAS_CUR_FBC(dev_priv)) > + I915_WRITE_FW(CUR_FBC_CTL(pipe), fbc_ctl); > I915_WRITE_FW(CURPOS(pipe), pos); > - > - if (plane->cursor.cntl != cntl || > - plane->cursor.size != fbc_ctl || > - plane->cursor.base != base) > I915_WRITE_FW(CURBASE(pipe), base); > > + plane->cursor.base = base; > + plane->cursor.size = fbc_ctl; > + plane->cursor.cntl = cntl; > + } else { > + I915_WRITE_FW(CURPOS(pipe), pos); > + } > + > POSTING_READ_FW(CURBASE(pipe)); > > spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags); > - > - plane->cursor.cntl = cntl; > - plane->cursor.base = base; > - plane->cursor.size = fbc_ctl; > } > > static void i9xx_disable_cursor(struct intel_plane *plane, > -- > 2.10.2 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Sat, May 06, 2017 at 12:31:50AM +0300, Imre Deak wrote: > On Mon, Mar 27, 2017 at 09:55:46PM +0300, ville.syrjala@linux.intel.com wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > It looks like simply writing all the cursor register every single > > time might be slightly faster than checking to see of each of > > them need to be written. So if any other register apart from > > CURPOS needs to be written let's just write all the registers. > > > > CURPOS is left as a special case mainly for 845/865 where we have to > > disable the cursor to change many of the cursor parameters. This > > introduces a slight chance of the cursor flickering when things get > > updated (since we're not currently doing the vblank evade for cursor > > updates). If we write CURPOS alone then that obviously can't happen. > > And let's follow the same pattern in the i9xx code just for symmetry. > > I wasn't able to see a singificant performance difference between > > this and just writing all the registers unconditionally. > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Reviewed-by: Imre Deak <imre.deak@intel.com> Thanks for the reviews. Entire series pushed to dinq. > > > --- > > drivers/gpu/drm/i915/intel_display.c | 69 ++++++++++++++++++------------------ > > 1 file changed, 34 insertions(+), 35 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index d78ab0d35274..40ac0f938a4e 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -9323,36 +9323,28 @@ static void i845_update_cursor(struct intel_plane *plane, > > > > spin_lock_irqsave(&dev_priv->uncore.lock, irqflags); > > > > - if (plane->cursor.cntl != 0 && > > - (plane->cursor.base != base || > > - plane->cursor.size != size || > > - plane->cursor.cntl != cntl)) { > > - /* On these chipsets we can only modify the base/size/stride > > - * whilst the cursor is disabled. > > - */ > > + /* On these chipsets we can only modify the base/size/stride > > + * whilst the cursor is disabled. > > + */ > > + if (plane->cursor.base != base || > > + plane->cursor.size != size || > > + plane->cursor.cntl != cntl) { > > I915_WRITE_FW(CURCNTR(PIPE_A), 0); > > - plane->cursor.cntl = 0; > > - } > > - > > - if (plane->cursor.base != base) > > I915_WRITE_FW(CURBASE(PIPE_A), base); > > - > > - if (plane->cursor.size != size) > > I915_WRITE_FW(CURSIZE, size); > > - > > - if (cntl) > > I915_WRITE_FW(CURPOS(PIPE_A), pos); > > - > > - if (plane->cursor.cntl != cntl) > > I915_WRITE_FW(CURCNTR(PIPE_A), cntl); > > > > + plane->cursor.base = base; > > + plane->cursor.size = size; > > + plane->cursor.cntl = cntl; > > + } else { > > + I915_WRITE_FW(CURPOS(PIPE_A), pos); > > + } > > + > > POSTING_READ_FW(CURCNTR(PIPE_A)); > > > > spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags); > > - > > - plane->cursor.cntl = cntl; > > - plane->cursor.base = base; > > - plane->cursor.size = size; > > } > > > > static void i845_disable_cursor(struct intel_plane *plane, > > @@ -9508,27 +9500,34 @@ static void i9xx_update_cursor(struct intel_plane *plane, > > > > spin_lock_irqsave(&dev_priv->uncore.lock, irqflags); > > > > - if (plane->cursor.cntl != cntl) > > + /* > > + * 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. > > + * > > + * CURCNTR and CUR_FBC_CTL are always > > + * armed by the CURBASE write only. > > + */ > > + if (plane->cursor.base != base || > > + plane->cursor.size != fbc_ctl || > > + plane->cursor.cntl != cntl) { > > I915_WRITE_FW(CURCNTR(pipe), cntl); > > - > > - if (plane->cursor.size != fbc_ctl) > > - I915_WRITE_FW(CUR_FBC_CTL(pipe), fbc_ctl); > > - > > - if (cntl) > > + if (HAS_CUR_FBC(dev_priv)) > > + I915_WRITE_FW(CUR_FBC_CTL(pipe), fbc_ctl); > > I915_WRITE_FW(CURPOS(pipe), pos); > > - > > - if (plane->cursor.cntl != cntl || > > - plane->cursor.size != fbc_ctl || > > - plane->cursor.base != base) > > I915_WRITE_FW(CURBASE(pipe), base); > > > > + plane->cursor.base = base; > > + plane->cursor.size = fbc_ctl; > > + plane->cursor.cntl = cntl; > > + } else { > > + I915_WRITE_FW(CURPOS(pipe), pos); > > + } > > + > > POSTING_READ_FW(CURBASE(pipe)); > > > > spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags); > > - > > - plane->cursor.cntl = cntl; > > - plane->cursor.base = base; > > - plane->cursor.size = fbc_ctl; > > } > > > > static void i9xx_disable_cursor(struct intel_plane *plane, > > -- > > 2.10.2 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index d78ab0d35274..40ac0f938a4e 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -9323,36 +9323,28 @@ static void i845_update_cursor(struct intel_plane *plane, spin_lock_irqsave(&dev_priv->uncore.lock, irqflags); - if (plane->cursor.cntl != 0 && - (plane->cursor.base != base || - plane->cursor.size != size || - plane->cursor.cntl != cntl)) { - /* On these chipsets we can only modify the base/size/stride - * whilst the cursor is disabled. - */ + /* On these chipsets we can only modify the base/size/stride + * whilst the cursor is disabled. + */ + if (plane->cursor.base != base || + plane->cursor.size != size || + plane->cursor.cntl != cntl) { I915_WRITE_FW(CURCNTR(PIPE_A), 0); - plane->cursor.cntl = 0; - } - - if (plane->cursor.base != base) I915_WRITE_FW(CURBASE(PIPE_A), base); - - if (plane->cursor.size != size) I915_WRITE_FW(CURSIZE, size); - - if (cntl) I915_WRITE_FW(CURPOS(PIPE_A), pos); - - if (plane->cursor.cntl != cntl) I915_WRITE_FW(CURCNTR(PIPE_A), cntl); + plane->cursor.base = base; + plane->cursor.size = size; + plane->cursor.cntl = cntl; + } else { + I915_WRITE_FW(CURPOS(PIPE_A), pos); + } + POSTING_READ_FW(CURCNTR(PIPE_A)); spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags); - - plane->cursor.cntl = cntl; - plane->cursor.base = base; - plane->cursor.size = size; } static void i845_disable_cursor(struct intel_plane *plane, @@ -9508,27 +9500,34 @@ static void i9xx_update_cursor(struct intel_plane *plane, spin_lock_irqsave(&dev_priv->uncore.lock, irqflags); - if (plane->cursor.cntl != cntl) + /* + * 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. + * + * CURCNTR and CUR_FBC_CTL are always + * armed by the CURBASE write only. + */ + if (plane->cursor.base != base || + plane->cursor.size != fbc_ctl || + plane->cursor.cntl != cntl) { I915_WRITE_FW(CURCNTR(pipe), cntl); - - if (plane->cursor.size != fbc_ctl) - I915_WRITE_FW(CUR_FBC_CTL(pipe), fbc_ctl); - - if (cntl) + if (HAS_CUR_FBC(dev_priv)) + I915_WRITE_FW(CUR_FBC_CTL(pipe), fbc_ctl); I915_WRITE_FW(CURPOS(pipe), pos); - - if (plane->cursor.cntl != cntl || - plane->cursor.size != fbc_ctl || - plane->cursor.base != base) I915_WRITE_FW(CURBASE(pipe), base); + plane->cursor.base = base; + plane->cursor.size = fbc_ctl; + plane->cursor.cntl = cntl; + } else { + I915_WRITE_FW(CURPOS(pipe), pos); + } + POSTING_READ_FW(CURBASE(pipe)); spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags); - - plane->cursor.cntl = cntl; - plane->cursor.base = base; - plane->cursor.size = fbc_ctl; } static void i9xx_disable_cursor(struct intel_plane *plane,