| Message ID | 1410544415-3059-3-git-send-email-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Sep 12, 2014 at 08:53:34PM +0300, ville.syrjala@linux.intel.com wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Only write CURBASE when something about the cursor changed. Also > eliminate the unnecessary posting read after writing CURCNTR. > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 82c0ad1..60c1aa4 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -8322,16 +8322,18 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base) > cntl |= CURSOR_PIPE_CSC_ENABLE; > } > > - if (intel_crtc->cursor_cntl != cntl) { > + if (intel_crtc->cursor_cntl != cntl) > I915_WRITE(CURCNTR(pipe), cntl); > - POSTING_READ(CURCNTR(pipe)); > - intel_crtc->cursor_cntl = cntl; > - } > + > + if (intel_crtc->cursor_cntl == cntl && > + intel_crtc->cursor_base == base) > + return; I'd vote for doing this first and then I915_WRITE(CURCNTR(pipe), cntl); unconditionally along with the CURBASE flush. > /* and commit changes on next vblank */ > I915_WRITE(CURBASE(pipe), base); > POSTING_READ(CURBASE(pipe)); > > + intel_crtc->cursor_cntl = cntl; > intel_crtc->cursor_base = base; > }
On Sat, Sep 13, 2014 at 05:20:10PM +0100, Chris Wilson wrote: > On Fri, Sep 12, 2014 at 08:53:34PM +0300, ville.syrjala@linux.intel.com wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Only write CURBASE when something about the cursor changed. Also > > eliminate the unnecessary posting read after writing CURCNTR. > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/i915/intel_display.c | 10 ++++++---- > > 1 file changed, 6 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index 82c0ad1..60c1aa4 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -8322,16 +8322,18 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base) > > cntl |= CURSOR_PIPE_CSC_ENABLE; > > } > > > > - if (intel_crtc->cursor_cntl != cntl) { > > + if (intel_crtc->cursor_cntl != cntl) > > I915_WRITE(CURCNTR(pipe), cntl); > > - POSTING_READ(CURCNTR(pipe)); > > - intel_crtc->cursor_cntl = cntl; > > - } > > + > > + if (intel_crtc->cursor_cntl == cntl && > > + intel_crtc->cursor_base == base) > > + return; > > I'd vote for doing this first and then > > I915_WRITE(CURCNTR(pipe), cntl); > > unconditionally along with the CURBASE flush. So you want to write all the cursor registers unconditionally when anything changes? Would work for CURCNTR, but it would also make CUR_FBC_CTL stand out like a sore thumb since it can't be written uncoditionally. > > > /* and commit changes on next vblank */ > > I915_WRITE(CURBASE(pipe), base); > > POSTING_READ(CURBASE(pipe)); > > > > + intel_crtc->cursor_cntl = cntl; > > intel_crtc->cursor_base = base; > > } > > -- > Chris Wilson, Intel Open Source Technology Centre
On Mon, Sep 15, 2014 at 04:36:38PM +0300, Ville Syrjälä wrote: > On Sat, Sep 13, 2014 at 05:20:10PM +0100, Chris Wilson wrote: > > On Fri, Sep 12, 2014 at 08:53:34PM +0300, ville.syrjala@linux.intel.com wrote: > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > Only write CURBASE when something about the cursor changed. Also > > > eliminate the unnecessary posting read after writing CURCNTR. > > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_display.c | 10 ++++++---- > > > 1 file changed, 6 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > > index 82c0ad1..60c1aa4 100644 > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > @@ -8322,16 +8322,18 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base) > > > cntl |= CURSOR_PIPE_CSC_ENABLE; > > > } > > > > > > - if (intel_crtc->cursor_cntl != cntl) { > > > + if (intel_crtc->cursor_cntl != cntl) > > > I915_WRITE(CURCNTR(pipe), cntl); > > > - POSTING_READ(CURCNTR(pipe)); > > > - intel_crtc->cursor_cntl = cntl; > > > - } > > > + > > > + if (intel_crtc->cursor_cntl == cntl && > > > + intel_crtc->cursor_base == base) > > > + return; > > > > I'd vote for doing this first and then > > > > I915_WRITE(CURCNTR(pipe), cntl); > > > > unconditionally along with the CURBASE flush. > > So you want to write all the cursor registers unconditionally when > anything changes? Would work for CURCNTR, but it would also make > CUR_FBC_CTL stand out like a sore thumb since it can't be written > uncoditionally. Alternatively, something like bool dirty = intel_crtc->cursor_base != base; if (intel_crtc->cursor_cntl != cntl) { I915_WRITE(CURCNTR(pipe), cntl); dirty = true; } /* ... */ if (!dirty) return; I915_WRITE(CURBASE(pipe), base); POSTING_READ(CURBASE(pipe)); intel_crtc->cursor_cntl = cntl; intel_crtc->cursor_base = base; I think would have clearer control flow. -Chris
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 82c0ad1..60c1aa4 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -8322,16 +8322,18 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base) cntl |= CURSOR_PIPE_CSC_ENABLE; } - if (intel_crtc->cursor_cntl != cntl) { + if (intel_crtc->cursor_cntl != cntl) I915_WRITE(CURCNTR(pipe), cntl); - POSTING_READ(CURCNTR(pipe)); - intel_crtc->cursor_cntl = cntl; - } + + if (intel_crtc->cursor_cntl == cntl && + intel_crtc->cursor_base == base) + return; /* and commit changes on next vblank */ I915_WRITE(CURBASE(pipe), base); POSTING_READ(CURBASE(pipe)); + intel_crtc->cursor_cntl = cntl; intel_crtc->cursor_base = base; }