| Message ID | 1435875914-11516-9-git-send-email-przanoni@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jul 02, 2015 at 07:25:14PM -0300, Paulo Zanoni wrote: > From: Paulo Zanoni <paulo.r.zanoni@intel.com> > > Now all the functions called by other files have the HAS_FBC > protection. This allows us to drop the checks for the low level > function pointers. > > Suggested-by: Chris Wilson <chris@chris-wilson.co.uk> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > --- > drivers/gpu/drm/i915/intel_fbc.c | 27 ++++++++++++++++++--------- > 1 file changed, 18 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c > index cc9b7ef..bc3cdb3 100644 > --- a/drivers/gpu/drm/i915/intel_fbc.c > +++ b/drivers/gpu/drm/i915/intel_fbc.c > @@ -388,9 +388,6 @@ static void intel_fbc_enable(struct drm_crtc *crtc) > > WARN_ON(!mutex_is_locked(&dev_priv->fbc.lock)); > > - if (!dev_priv->display.enable_fbc) > - return; > - > intel_fbc_cancel_work(dev_priv); > > work = kzalloc(sizeof(*work), GFP_KERNEL); > @@ -661,6 +661,9 @@ void intel_fbc_cleanup_cfb(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > > + if (!HAS_FBC(dev_priv)) > + return; > + > mutex_lock(&dev_priv->fbc.lock); > __intel_fbc_cleanup_cfb(dev); > mutex_unlock(&dev_priv->fbc.lock); dev_priv->display.enable_fbc is one less pointer dereference than HAS_FBC() and is a better indication of whether we have initialised the display device for FBC (i.e. it also carries information about whether the setup succeeded, maybe some platforms have HAS_FBC but we fail to negotiate etc). -Chris
On Fri, Jul 03, 2015 at 04:56:31PM +0100, Chris Wilson wrote: > On Thu, Jul 02, 2015 at 07:25:14PM -0300, Paulo Zanoni wrote: > > From: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > > Now all the functions called by other files have the HAS_FBC > > protection. This allows us to drop the checks for the low level > > function pointers. > > > > Suggested-by: Chris Wilson <chris@chris-wilson.co.uk> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > --- > > drivers/gpu/drm/i915/intel_fbc.c | 27 ++++++++++++++++++--------- > > 1 file changed, 18 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c > > index cc9b7ef..bc3cdb3 100644 > > --- a/drivers/gpu/drm/i915/intel_fbc.c > > +++ b/drivers/gpu/drm/i915/intel_fbc.c > > @@ -388,9 +388,6 @@ static void intel_fbc_enable(struct drm_crtc *crtc) > > > > WARN_ON(!mutex_is_locked(&dev_priv->fbc.lock)); > > > > - if (!dev_priv->display.enable_fbc) > > - return; > > - > > intel_fbc_cancel_work(dev_priv); > > > > work = kzalloc(sizeof(*work), GFP_KERNEL); > > > @@ -661,6 +661,9 @@ void intel_fbc_cleanup_cfb(struct drm_device *dev) > > { > > struct drm_i915_private *dev_priv = dev->dev_private; > > > > + if (!HAS_FBC(dev_priv)) > > + return; > > + > > mutex_lock(&dev_priv->fbc.lock); > > __intel_fbc_cleanup_cfb(dev); > > mutex_unlock(&dev_priv->fbc.lock); > > dev_priv->display.enable_fbc is one less pointer dereference than > HAS_FBC() and is a better indication of whether we have initialised the > display device for FBC (i.e. it also carries information about whether > the setup succeeded, maybe some platforms have HAS_FBC but we fail to > negotiate etc). With or without that change (would prefer with!), Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> -Chris
2015-07-03 12:56 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>: > On Thu, Jul 02, 2015 at 07:25:14PM -0300, Paulo Zanoni wrote: >> From: Paulo Zanoni <paulo.r.zanoni@intel.com> >> >> Now all the functions called by other files have the HAS_FBC >> protection. This allows us to drop the checks for the low level >> function pointers. >> >> Suggested-by: Chris Wilson <chris@chris-wilson.co.uk> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> >> --- >> drivers/gpu/drm/i915/intel_fbc.c | 27 ++++++++++++++++++--------- >> 1 file changed, 18 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c >> index cc9b7ef..bc3cdb3 100644 >> --- a/drivers/gpu/drm/i915/intel_fbc.c >> +++ b/drivers/gpu/drm/i915/intel_fbc.c >> @@ -388,9 +388,6 @@ static void intel_fbc_enable(struct drm_crtc *crtc) >> >> WARN_ON(!mutex_is_locked(&dev_priv->fbc.lock)); >> >> - if (!dev_priv->display.enable_fbc) >> - return; >> - >> intel_fbc_cancel_work(dev_priv); >> >> work = kzalloc(sizeof(*work), GFP_KERNEL); > >> @@ -661,6 +661,9 @@ void intel_fbc_cleanup_cfb(struct drm_device *dev) >> { >> struct drm_i915_private *dev_priv = dev->dev_private; >> >> + if (!HAS_FBC(dev_priv)) >> + return; >> + >> mutex_lock(&dev_priv->fbc.lock); >> __intel_fbc_cleanup_cfb(dev); >> mutex_unlock(&dev_priv->fbc.lock); > > dev_priv->display.enable_fbc is one less pointer dereference than HAS_FBC() HAS_FBC(dev_priv) is supposed to be dev_priv->info.has_fbc, which has the same number of pointer dereferences than dev_priv->display.enable_fbc. The problem would be if I had used HAS_FBC(dev). I also took a look at the __I915__ macro and, although I didn't check the assembly, it seems the "if" statement is removed by the compiler since we never compile BUILD_BUG(). If we wanted to be even more sure we could replace the if statement with nested __builtin_choose_expr() expressions (I even tried doing that, but it didn't reduce the size of stripped i915.ko). > and is a better indication of whether we have initialised the > display device for FBC (i.e. it also carries information about whether > the setup succeeded, maybe some platforms have HAS_FBC but we fail to > negotiate etc). This argument argument is valid - although that case doesn't happen -, but OTOH a check for HAS_FBC makes it very clear to the code reader what is being excluded, while a check for a function pointer makes the reader wonder about those cases which you mentioned, and they don't really exist, so I'm not sure if this is a good thing. So I guess it's a matter of style preference here. > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre
On Fri, Jul 03, 2015 at 02:59:28PM -0300, Paulo Zanoni wrote: > 2015-07-03 12:56 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>: > > On Thu, Jul 02, 2015 at 07:25:14PM -0300, Paulo Zanoni wrote: > >> From: Paulo Zanoni <paulo.r.zanoni@intel.com> > >> > >> Now all the functions called by other files have the HAS_FBC > >> protection. This allows us to drop the checks for the low level > >> function pointers. > >> > >> Suggested-by: Chris Wilson <chris@chris-wilson.co.uk> > >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > >> --- > >> drivers/gpu/drm/i915/intel_fbc.c | 27 ++++++++++++++++++--------- > >> 1 file changed, 18 insertions(+), 9 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c > >> index cc9b7ef..bc3cdb3 100644 > >> --- a/drivers/gpu/drm/i915/intel_fbc.c > >> +++ b/drivers/gpu/drm/i915/intel_fbc.c > >> @@ -388,9 +388,6 @@ static void intel_fbc_enable(struct drm_crtc *crtc) > >> > >> WARN_ON(!mutex_is_locked(&dev_priv->fbc.lock)); > >> > >> - if (!dev_priv->display.enable_fbc) > >> - return; > >> - > >> intel_fbc_cancel_work(dev_priv); > >> > >> work = kzalloc(sizeof(*work), GFP_KERNEL); > > > >> @@ -661,6 +661,9 @@ void intel_fbc_cleanup_cfb(struct drm_device *dev) > >> { > >> struct drm_i915_private *dev_priv = dev->dev_private; > >> > >> + if (!HAS_FBC(dev_priv)) > >> + return; > >> + > >> mutex_lock(&dev_priv->fbc.lock); > >> __intel_fbc_cleanup_cfb(dev); > >> mutex_unlock(&dev_priv->fbc.lock); > > > > dev_priv->display.enable_fbc is one less pointer dereference than HAS_FBC() > > HAS_FBC(dev_priv) is supposed to be dev_priv->info.has_fbc, which has > the same number of pointer dereferences than > dev_priv->display.enable_fbc. The problem would be if I had used > HAS_FBC(dev). I had forgotten we did the copy of intel_device_info into drm_i915_private and still lived in the pre-PCH_NOP days. > I also took a look at the __I915__ macro and, although I didn't check > the assembly, it seems the "if" statement is removed by the compiler > since we never compile BUILD_BUG(). If we wanted to be even more sure > we could replace the if statement with nested __builtin_choose_expr() > expressions (I even tried doing that, but it didn't reduce the size of > stripped i915.ko). if (compile_time_constant) is dead-code eliminated into the single branch (without the jmp). Hmm, the big advantage (not useful here) is __builtin_choose_expr() can be used as an lvalue. Otherwise it doesn't look like it will generate more readable code than the current macro. > > and is a better indication of whether we have initialised the > > display device for FBC (i.e. it also carries information about whether > > the setup succeeded, maybe some platforms have HAS_FBC but we fail to > > negotiate etc). > > This argument argument is valid - although that case doesn't happen -, > but OTOH a check for HAS_FBC makes it very clear to the code reader > what is being excluded, while a check for a function pointer makes the > reader wonder about those cases which you mentioned, and they don't > really exist, so I'm not sure if this is a good thing. So I guess it's > a matter of style preference here. It's the style we have been using elsewhere, certainly in new code (hopefully!). Do the general test during init, then later we ask whether the init suceeded before using the feature. -Chris
diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c index cc9b7ef..bc3cdb3 100644 --- a/drivers/gpu/drm/i915/intel_fbc.c +++ b/drivers/gpu/drm/i915/intel_fbc.c @@ -388,9 +388,6 @@ static void intel_fbc_enable(struct drm_crtc *crtc) WARN_ON(!mutex_is_locked(&dev_priv->fbc.lock)); - if (!dev_priv->display.enable_fbc) - return; - intel_fbc_cancel_work(dev_priv); work = kzalloc(sizeof(*work), GFP_KERNEL); @@ -430,9 +427,6 @@ static void __intel_fbc_disable(struct drm_device *dev) intel_fbc_cancel_work(dev_priv); - if (!dev_priv->display.disable_fbc) - return; - dev_priv->display.disable_fbc(dev); dev_priv->fbc.crtc = NULL; } @@ -447,6 +441,9 @@ void intel_fbc_disable(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; + if (!HAS_FBC(dev_priv)) + return; + mutex_lock(&dev_priv->fbc.lock); __intel_fbc_disable(dev); mutex_unlock(&dev_priv->fbc.lock); @@ -463,6 +460,9 @@ void intel_fbc_disable_crtc(struct intel_crtc *crtc) struct drm_device *dev = crtc->base.dev; struct drm_i915_private *dev_priv = dev->dev_private; + if (!HAS_FBC(dev_priv)) + return; + mutex_lock(&dev_priv->fbc.lock); if (dev_priv->fbc.crtc == crtc) __intel_fbc_disable(dev); @@ -661,6 +661,9 @@ void intel_fbc_cleanup_cfb(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; + if (!HAS_FBC(dev_priv)) + return; + mutex_lock(&dev_priv->fbc.lock); __intel_fbc_cleanup_cfb(dev); mutex_unlock(&dev_priv->fbc.lock); @@ -708,9 +711,6 @@ static void __intel_fbc_update(struct drm_device *dev) const struct drm_display_mode *adjusted_mode; unsigned int max_width, max_height; - if (!HAS_FBC(dev)) - return; - WARN_ON(!mutex_is_locked(&dev_priv->fbc.lock)); /* disable framebuffer compression in vGPU */ @@ -857,6 +857,9 @@ void intel_fbc_update(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; + if (!HAS_FBC(dev_priv)) + return; + mutex_lock(&dev_priv->fbc.lock); __intel_fbc_update(dev); mutex_unlock(&dev_priv->fbc.lock); @@ -869,6 +872,9 @@ void intel_fbc_invalidate(struct drm_i915_private *dev_priv, struct drm_device *dev = dev_priv->dev; unsigned int fbc_bits; + if (!HAS_FBC(dev_priv)) + return; + if (origin == ORIGIN_GTT) return; @@ -895,6 +901,9 @@ void intel_fbc_flush(struct drm_i915_private *dev_priv, { struct drm_device *dev = dev_priv->dev; + if (!HAS_FBC(dev_priv)) + return; + mutex_lock(&dev_priv->fbc.lock); if (!dev_priv->fbc.busy_bits)