Re: [PATCH 01/19] drm/i915: WARN if !HAS_PC8 when enabling/disabling PC8

Paulo Zanoni
2013-11-29T12:55:12Z

2013/11/29 Rodrigo Vivi <rodrigo.vivi@gmail.com>:
> I think it should be a return instead a WARN.
> Myabe I'm just sleeping yet, but I think the delayed work will execute this even for non HAS_PC8 and warn will always ring on non HSW.
> But even if I'm sleeping, since it really touch hsw registers wouldn't be safer a return instead of a warn anyway?

The refcount should never reach zero on platforms that don't have PC8,
so this should never happen. Also, functions that lead to this
function also have early returns, so we really shouldn't hit this
thing at all.

The early return will hide the problem, while the WARN will tell me my
design is wrong and I need to fix bugs :)

>
>
> On Thu, Nov 21, 2013 at 01:47:15PM -0200, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>> We already have some checks and shouldn't be reaching these places on
>> !HAS_PC8 platforms, but add a WARN, just in case.
>>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> ---
>> drivers/gpu/drm/i915/intel_display.c | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index e85d838..5566de5 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -6623,6 +6623,8 @@ void hsw_enable_pc8_work(struct work_struct *__work)
>> struct drm_device *dev = dev_priv->dev;
>> uint32_t val;
>>
>> + WARN_ON(!HAS_PC8(dev));
>> +
>> if (dev_priv->pc8.enabled)
>> return;
>>
>> @@ -6668,6 +6670,8 @@ static void __hsw_disable_package_c8(struct drm_i915_private *dev_priv)
>> if (dev_priv->pc8.disable_count != 1)
>> return;
>>
>> + WARN_ON(!HAS_PC8(dev));
>> +
>> cancel_delayed_work_sync(&dev_priv->pc8.enable_work);
>> if (!dev_priv->pc8.enabled)
>> return;
>> --
>> 1.8.3.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

--
Paulo Zanoni