Re: [PATCH 6/8] drm/i915: Rework the FBC interval/stall stuff a bit

Imre Deak
2013-12-12T14:04:53Z

[-- Attachment #1.1: Type: text/plain, Size: 8289 bytes --]

On Thu, 2013-11-28 at 17:30 +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Don't touch DPFC_RECOMP_CTL on FBC2, use RMW to update
> the FBC_CONTROL on FBC1 to make it easier for people to
> experiment with different numbers. Also fix the interval
> mask for FBC1.

As I understood the reason for removing DPFC_RECOMP support is that we
need to understand better how it affects performance etc. before
enabling it. That's something for the future.

We could still zero that register in case BIOS leaves there something
else but that can be done as a follow-up too. So:
Reviewed-by: Imre Deak <imre.deak@intel.com>

>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 4 +---
> drivers/gpu/drm/i915/intel_pm.c | 46 +++++++++++++++--------------------------
> 2 files changed, 18 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index c5cbbc2..20a9811 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -373,8 +373,7 @@ struct dpll;
> struct drm_i915_display_funcs {
> bool (*fbc_enabled)(struct drm_device *dev);
> void (*enable_fbc)(struct drm_crtc *crtc,
> - struct drm_framebuffer *fb,
> - unsigned long interval);
> + struct drm_framebuffer *fb);
> void (*disable_fbc)(struct drm_device *dev);
> int (*get_display_clock_speed)(struct drm_device *dev);
> int (*get_fifo_size)(struct drm_device *dev, int plane);
> @@ -692,7 +691,6 @@ struct i915_fbc {
> struct delayed_work work;
> struct drm_crtc *crtc;
> struct drm_framebuffer *fb;
> - int interval;
> } *fbc_work;
>
> enum no_fbc_reason {
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index b296438..af2bc2b 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -87,8 +87,7 @@ static void i8xx_disable_fbc(struct drm_device *dev)
> }
>
> static void i8xx_enable_fbc(struct drm_crtc *crtc,
> - struct drm_framebuffer *fb,
> - unsigned long interval)
> + struct drm_framebuffer *fb)
> {
> struct drm_device *dev = crtc->dev;
> struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -125,11 +124,12 @@ static void i8xx_enable_fbc(struct drm_crtc *crtc,
> }
>
> /* enable it... */
> - fbc_ctl = FBC_CTL_EN | FBC_CTL_PERIODIC;
> + fbc_ctl = I915_READ(FBC_CONTROL);
> + fbc_ctl &= 0x3fff << FBC_CTL_INTERVAL_SHIFT;
> + fbc_ctl |= FBC_CTL_EN | FBC_CTL_PERIODIC;
> if (IS_I945GM(dev))
> fbc_ctl |= FBC_CTL_C3_IDLE; /* 945 needs special SR handling */
> fbc_ctl |= (cfb_pitch & 0xff) << FBC_CTL_STRIDE_SHIFT;
> - fbc_ctl |= (interval & 0x2fff) << FBC_CTL_INTERVAL_SHIFT;
> fbc_ctl |= obj->fence_reg;
> I915_WRITE(FBC_CONTROL, fbc_ctl);
>
> @@ -145,8 +145,7 @@ static bool i8xx_fbc_enabled(struct drm_device *dev)
> }
>
> static void g4x_enable_fbc(struct drm_crtc *crtc,
> - struct drm_framebuffer *fb,
> - unsigned long interval)
> + struct drm_framebuffer *fb)
> {
> struct drm_device *dev = crtc->dev;
> struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -154,15 +153,10 @@ static void g4x_enable_fbc(struct drm_crtc *crtc,
> struct drm_i915_gem_object *obj = intel_fb->obj;
> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> int plane = intel_crtc->plane == 0 ? DPFC_CTL_PLANEA : DPFC_CTL_PLANEB;
> - unsigned long stall_watermark = 200;
> u32 dpfc_ctl;
>
> dpfc_ctl = plane | DPFC_SR_EN | DPFC_CTL_LIMIT_1X;
> dpfc_ctl |= DPFC_CTL_FENCE_EN | obj->fence_reg;
> -
> - I915_WRITE(DPFC_RECOMP_CTL, DPFC_RECOMP_STALL_EN |
> - (stall_watermark << DPFC_RECOMP_STALL_WM_SHIFT) |
> - (interval << DPFC_RECOMP_TIMER_COUNT_SHIFT));
> I915_WRITE(DPFC_FENCE_YOFF, crtc->y);
>
> /* enable it... */
> @@ -194,8 +188,7 @@ static bool g4x_fbc_enabled(struct drm_device *dev)
> }
>
> static void ironlake_enable_fbc(struct drm_crtc *crtc,
> - struct drm_framebuffer *fb,
> - unsigned long interval)
> + struct drm_framebuffer *fb)
> {
> struct drm_device *dev = crtc->dev;
> struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -203,7 +196,6 @@ static void ironlake_enable_fbc(struct drm_crtc *crtc,
> struct drm_i915_gem_object *obj = intel_fb->obj;
> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> int plane = intel_crtc->plane == 0 ? DPFC_CTL_PLANEA : DPFC_CTL_PLANEB;
> - unsigned long stall_watermark = 200;
> u32 dpfc_ctl;
>
> dpfc_ctl = I915_READ(ILK_DPFC_CONTROL);
> @@ -212,10 +204,6 @@ static void ironlake_enable_fbc(struct drm_crtc *crtc,
> dpfc_ctl |= DPFC_CTL_FENCE_EN;
> if (IS_GEN5(dev))
> dpfc_ctl |= obj->fence_reg;
> -
> - I915_WRITE(ILK_DPFC_RECOMP_CTL, DPFC_RECOMP_STALL_EN |
> - (stall_watermark << DPFC_RECOMP_STALL_WM_SHIFT) |
> - (interval << DPFC_RECOMP_TIMER_COUNT_SHIFT));
> I915_WRITE(ILK_DPFC_FENCE_YOFF, crtc->y);
> /* enable it... */
> I915_WRITE(ILK_DPFC_CONTROL, dpfc_ctl | DPFC_CTL_EN);
> @@ -252,8 +240,7 @@ static bool ironlake_fbc_enabled(struct drm_device *dev)
> }
>
> static void gen7_enable_fbc(struct drm_crtc *crtc,
> - struct drm_framebuffer *fb,
> - unsigned long interval)
> + struct drm_framebuffer *fb)
> {
> struct drm_device *dev = crtc->dev;
> struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -292,12 +279,11 @@ bool intel_fbc_enabled(struct drm_device *dev)
> }
>
> static void intel_setup_fbc(struct drm_crtc *crtc,
> - struct drm_framebuffer *fb,
> - unsigned long interval)
> + struct drm_framebuffer *fb)
> {
> struct drm_i915_private *dev_priv = crtc->dev->dev_private;
>
> - dev_priv->display.enable_fbc(crtc, fb, interval);
> + dev_priv->display.enable_fbc(crtc, fb);
>
> dev_priv->fbc.plane = to_intel_crtc(crtc)->plane;
> drm_framebuffer_reference(fb);
> @@ -324,7 +310,7 @@ static void intel_fbc_work_fn(struct work_struct *__work)
> * the prior work.
> */
> if (work->crtc->fb == work->fb)
> - intel_setup_fbc(work->crtc, work->fb, work->interval);
> + intel_setup_fbc(work->crtc, work->fb);
> dev_priv->fbc.fbc_work = NULL;
> }
> mutex_unlock(&dev->struct_mutex);
> @@ -361,7 +347,7 @@ static void intel_cancel_fbc_work(struct drm_i915_private *dev_priv)
> dev_priv->fbc.fbc_work = NULL;
> }
>
> -static void intel_enable_fbc(struct drm_crtc *crtc, unsigned long interval)
> +static void intel_enable_fbc(struct drm_crtc *crtc)
> {
> struct intel_fbc_work *work;
> struct drm_device *dev = crtc->dev;
> @@ -375,13 +361,12 @@ static void intel_enable_fbc(struct drm_crtc *crtc, unsigned long interval)
> work = kzalloc(sizeof(*work), GFP_KERNEL);
> if (work == NULL) {
> DRM_ERROR("Failed to allocate FBC work structure\n");
> - intel_setup_fbc(crtc, crtc->fb, interval);
> + intel_setup_fbc(crtc, crtc->fb);
> return;
> }
>
> work->crtc = crtc;
> work->fb = crtc->fb;
> - work->interval = interval;
> INIT_DELAYED_WORK(&work->work, intel_fbc_work_fn);
>
> drm_framebuffer_reference(work->fb);
> @@ -448,7 +433,7 @@ void intel_fbc_nuke(struct drm_i915_gem_object *obj)
> * Must wait until the next vblank before re-enabling
> * otherwise the nuking won't actually happen.
> */
> - intel_enable_fbc(crtc, 500);
> + intel_enable_fbc(crtc);
> }
>
> static bool set_no_fbc_reason(struct drm_i915_private *dev_priv,
> @@ -632,7 +617,7 @@ void intel_update_fbc(struct drm_device *dev)
> intel_disable_fbc(dev);
> }
>
> - intel_enable_fbc(crtc, 500);
> + intel_enable_fbc(crtc);
> dev_priv->fbc.no_fbc_reason = FBC_OK;
> return;
>
> @@ -5993,6 +5978,9 @@ void intel_init_pm(struct drm_device *dev)
> dev_priv->display.fbc_enabled = i8xx_fbc_enabled;
> dev_priv->display.enable_fbc = i8xx_enable_fbc;
> dev_priv->display.disable_fbc = i8xx_disable_fbc;
> +
> + /* This value was pulled out of someone's hat */
> + I915_WRITE(FBC_CONTROL, 500 << FBC_CTL_INTERVAL_SHIFT);
> }
> }
>

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org