2015-07-03 12:56 GMT-03:00 Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>:
> On Thu, Jul 02, 2015 at 07:25:14PM -0300, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
>>
>> 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@xxxxxxxxxxxxxxxxxx>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
>> ---
>> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
