[Intel-gfx] [PATCH 8/8] drm/i915: protect FBC functions with HAS_FBC checks

[Chris Wilson]

On Fri, Jul 03, 2015 at 02:59:28PM -0300, Paulo Zanoni wrote:
> 2015-07-03 12:56 GMT-03:00 Chris Wilson <chris at chris-wilson.co.uk>:
> > On Thu, Jul 02, 2015 at 07:25:14PM -0300, Paulo Zanoni wrote:
> >> From: Paulo Zanoni <paulo.r.zanoni at 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 at chris-wilson.co.uk>
> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni at 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

-- 
Chris Wilson, Intel Open Source Technology Centre