Message ID | 20180220134208.24988-4-chris@chris-wilson.co.uk (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On Tue, Feb 20, 2018 at 01:42:08PM +0000, Chris Wilson wrote: > Rather than trusting the cached value of plane_state->vma->fence to > imply whether the plane_state itself holds a reference on the > framebuffer's fence, use the information provided in the > plane_state->flags (PLANE_HAS_FENCE). Note that we still assume that FBC > is entirely bounded by the plane_state active life span; it's not clear > if that is a safe assumption. > > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 2 ++ > drivers/gpu/drm/i915/intel_fbc.c | 13 +++++++++---- > 2 files changed, 11 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 20575b3ee406..848dc8f68b47 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -667,6 +667,7 @@ struct intel_fbc { > */ > struct intel_fbc_state_cache { > struct i915_vma *vma; > + unsigned long flags; > > struct { > unsigned int mode_flags; > @@ -705,6 +706,7 @@ struct intel_fbc { > */ > struct intel_fbc_reg_params { > struct i915_vma *vma; > + unsigned long flags; > > struct { > enum pipe pipe; > diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c > index d7d1ac79c38a..f66f6fb5743d 100644 > --- a/drivers/gpu/drm/i915/intel_fbc.c > +++ b/drivers/gpu/drm/i915/intel_fbc.c > @@ -183,7 +183,7 @@ static void g4x_fbc_activate(struct drm_i915_private *dev_priv) > else > dpfc_ctl |= DPFC_CTL_LIMIT_1X; > > - if (params->vma->fence) { > + if (params->flags & PLANE_HAS_FENCE) { These checks seem redundant now that we shouldn't even try to enable fbc without the fence. But they're not harming anyone (well, apart from making the code a bit inconsistent by not having the check in the fbc1 codepath). But the patch is Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> regardless > dpfc_ctl |= DPFC_CTL_FENCE_EN | params->vma->fence->id; > I915_WRITE(DPFC_FENCE_YOFF, params->crtc.fence_y_offset); > } else { > @@ -241,7 +241,7 @@ static void ilk_fbc_activate(struct drm_i915_private *dev_priv) > break; > } > > - if (params->vma->fence) { > + if (params->flags & PLANE_HAS_FENCE) { > dpfc_ctl |= DPFC_CTL_FENCE_EN; > if (IS_GEN5(dev_priv)) > dpfc_ctl |= params->vma->fence->id; > @@ -324,7 +324,7 @@ static void gen7_fbc_activate(struct drm_i915_private *dev_priv) > break; > } > > - if (params->vma->fence) { > + if (params->flags & PLANE_HAS_FENCE) { > dpfc_ctl |= IVB_DPFC_CTL_FENCE_EN; > I915_WRITE(SNB_DPFC_CTL_SA, > SNB_CPU_FENCE_ENABLE | > @@ -753,6 +753,7 @@ static void intel_fbc_update_state_cache(struct intel_crtc *crtc, > struct drm_framebuffer *fb = plane_state->base.fb; > > cache->vma = NULL; > + cache->flags = 0; > > cache->crtc.mode_flags = crtc_state->base.adjusted_mode.flags; > if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) > @@ -778,6 +779,9 @@ static void intel_fbc_update_state_cache(struct intel_crtc *crtc, > cache->fb.stride = fb->pitches[0]; > > cache->vma = plane_state->vma; > + cache->flags = plane_state->flags; > + if (WARN_ON(cache->flags & PLANE_HAS_FENCE && !cache->vma->fence)) > + cache->flags &= ~PLANE_HAS_FENCE; > } > > static bool intel_fbc_can_activate(struct intel_crtc *crtc) > @@ -816,7 +820,7 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc) > * so have no fence associated with it) due to aperture constaints > * at the time of pinning. > */ > - if (!cache->vma->fence) { > + if (!(cache->flags & PLANE_HAS_FENCE)) { > fbc->no_fbc_reason = "framebuffer not tiled or fenced"; > return false; > } > @@ -897,6 +901,7 @@ static void intel_fbc_get_reg_params(struct intel_crtc *crtc, > memset(params, 0, sizeof(*params)); > > params->vma = cache->vma; > + params->flags = cache->flags; > > params->crtc.pipe = crtc->pipe; > params->crtc.i9xx_plane = to_intel_plane(crtc->base.primary)->i9xx_plane; > -- > 2.16.1
Quoting Ville Syrjälä (2018-02-20 13:59:53) > On Tue, Feb 20, 2018 at 01:42:08PM +0000, Chris Wilson wrote: > > Rather than trusting the cached value of plane_state->vma->fence to > > imply whether the plane_state itself holds a reference on the > > framebuffer's fence, use the information provided in the > > plane_state->flags (PLANE_HAS_FENCE). Note that we still assume that FBC > > is entirely bounded by the plane_state active life span; it's not clear > > if that is a safe assumption. > > > > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/i915/i915_drv.h | 2 ++ > > drivers/gpu/drm/i915/intel_fbc.c | 13 +++++++++---- > > 2 files changed, 11 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > > index 20575b3ee406..848dc8f68b47 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -667,6 +667,7 @@ struct intel_fbc { > > */ > > struct intel_fbc_state_cache { > > struct i915_vma *vma; > > + unsigned long flags; > > > > struct { > > unsigned int mode_flags; > > @@ -705,6 +706,7 @@ struct intel_fbc { > > */ > > struct intel_fbc_reg_params { > > struct i915_vma *vma; > > + unsigned long flags; > > > > struct { > > enum pipe pipe; > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c > > index d7d1ac79c38a..f66f6fb5743d 100644 > > --- a/drivers/gpu/drm/i915/intel_fbc.c > > +++ b/drivers/gpu/drm/i915/intel_fbc.c > > @@ -183,7 +183,7 @@ static void g4x_fbc_activate(struct drm_i915_private *dev_priv) > > else > > dpfc_ctl |= DPFC_CTL_LIMIT_1X; > > > > - if (params->vma->fence) { > > + if (params->flags & PLANE_HAS_FENCE) { > > These checks seem redundant now that we shouldn't even try to enable fbc > without the fence. But they're not harming anyone (well, apart from making > the code a bit inconsistent by not having the check in the fbc1 codepath). I'm always optimistic fenceless FBC will make a comeback. -Chris
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 20575b3ee406..848dc8f68b47 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -667,6 +667,7 @@ struct intel_fbc { */ struct intel_fbc_state_cache { struct i915_vma *vma; + unsigned long flags; struct { unsigned int mode_flags; @@ -705,6 +706,7 @@ struct intel_fbc { */ struct intel_fbc_reg_params { struct i915_vma *vma; + unsigned long flags; struct { enum pipe pipe; diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c index d7d1ac79c38a..f66f6fb5743d 100644 --- a/drivers/gpu/drm/i915/intel_fbc.c +++ b/drivers/gpu/drm/i915/intel_fbc.c @@ -183,7 +183,7 @@ static void g4x_fbc_activate(struct drm_i915_private *dev_priv) else dpfc_ctl |= DPFC_CTL_LIMIT_1X; - if (params->vma->fence) { + if (params->flags & PLANE_HAS_FENCE) { dpfc_ctl |= DPFC_CTL_FENCE_EN | params->vma->fence->id; I915_WRITE(DPFC_FENCE_YOFF, params->crtc.fence_y_offset); } else { @@ -241,7 +241,7 @@ static void ilk_fbc_activate(struct drm_i915_private *dev_priv) break; } - if (params->vma->fence) { + if (params->flags & PLANE_HAS_FENCE) { dpfc_ctl |= DPFC_CTL_FENCE_EN; if (IS_GEN5(dev_priv)) dpfc_ctl |= params->vma->fence->id; @@ -324,7 +324,7 @@ static void gen7_fbc_activate(struct drm_i915_private *dev_priv) break; } - if (params->vma->fence) { + if (params->flags & PLANE_HAS_FENCE) { dpfc_ctl |= IVB_DPFC_CTL_FENCE_EN; I915_WRITE(SNB_DPFC_CTL_SA, SNB_CPU_FENCE_ENABLE | @@ -753,6 +753,7 @@ static void intel_fbc_update_state_cache(struct intel_crtc *crtc, struct drm_framebuffer *fb = plane_state->base.fb; cache->vma = NULL; + cache->flags = 0; cache->crtc.mode_flags = crtc_state->base.adjusted_mode.flags; if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) @@ -778,6 +779,9 @@ static void intel_fbc_update_state_cache(struct intel_crtc *crtc, cache->fb.stride = fb->pitches[0]; cache->vma = plane_state->vma; + cache->flags = plane_state->flags; + if (WARN_ON(cache->flags & PLANE_HAS_FENCE && !cache->vma->fence)) + cache->flags &= ~PLANE_HAS_FENCE; } static bool intel_fbc_can_activate(struct intel_crtc *crtc) @@ -816,7 +820,7 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc) * so have no fence associated with it) due to aperture constaints * at the time of pinning. */ - if (!cache->vma->fence) { + if (!(cache->flags & PLANE_HAS_FENCE)) { fbc->no_fbc_reason = "framebuffer not tiled or fenced"; return false; } @@ -897,6 +901,7 @@ static void intel_fbc_get_reg_params(struct intel_crtc *crtc, memset(params, 0, sizeof(*params)); params->vma = cache->vma; + params->flags = cache->flags; params->crtc.pipe = crtc->pipe; params->crtc.i9xx_plane = to_intel_plane(crtc->base.primary)->i9xx_plane;
Rather than trusting the cached value of plane_state->vma->fence to imply whether the plane_state itself holds a reference on the framebuffer's fence, use the information provided in the plane_state->flags (PLANE_HAS_FENCE). Note that we still assume that FBC is entirely bounded by the plane_state active life span; it's not clear if that is a safe assumption. Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> --- drivers/gpu/drm/i915/i915_drv.h | 2 ++ drivers/gpu/drm/i915/intel_fbc.c | 13 +++++++++---- 2 files changed, 11 insertions(+), 4 deletions(-)