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

[Paulo Zanoni]

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 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



-- 
Paulo Zanoni