Re: [Intel-gfx] [RFC] drm: drm_vblank_cleanup: WARN when refcount is more than 0

[PrasannaKumar Muralidharan]

Hi Ville,

On 12 October 2017 at 22:52, Ville Syrjälä
 wrote:
> On Thu, Oct 12, 2017 at 10:30:06PM +0530, PrasannaKumar Muralidharan wrote:
>> Warn when refcount > 0 in drm_vblank_cleanup.
>>
>> In i915 driver unload drm_vblank_get is added to test whether this patch
>> is working. This will be removed once the patch gets tested.
>>
>> Signed-off-by: PrasannaKumar Muralidharan 
>> ---
>>  drivers/gpu/drm/drm_vblank.c| 2 ++
>>  drivers/gpu/drm/i915/i915_drv.c | 1 +
>>  2 files changed, 3 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
>> index 70f2b95..2a6630a 100644
>> --- a/drivers/gpu/drm/drm_vblank.c
>> +++ b/drivers/gpu/drm/drm_vblank.c
>> @@ -402,6 +402,8 @@ void drm_vblank_cleanup(struct drm_device *dev)
>>   if (dev->num_crtcs == 0)
>>   return;
>>
>> + WARN_ON(atomic_read(>refcount) > 0);
>> +
>
> That won't even compile. Always at least compile + sparse check +
> checkpatch your stuff, otherwise you're just going to be wasting
> other people's or the CI systems's time.

CI just told me this. I will keep this in mind even for simple patches.

>>   for (pipe = 0; pipe < dev->num_crtcs; pipe++) {
>>   struct drm_vblank_crtc *vblank = >vblank[pipe];
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c 
>> b/drivers/gpu/drm/i915/i915_drv.c
>> index 9f45cfe..c7a93a9 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -1373,6 +1373,7 @@ void i915_driver_unload(struct drm_device *dev)
>>   struct drm_i915_private *dev_priv = to_i915(dev);
>>   struct pci_dev *pdev = dev_priv->drm.pdev;
>>
>> + drm_vblank_get(dev_priv, 0);
>>   i915_driver_unregister(dev_priv);
>>
>>   if (i915_gem_suspend(dev_priv))
>> --
>> 2.10.0
>>
>> ___
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Ville Syrjälä
> Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [RFC] drm: drm_vblank_cleanup: WARN when refcount is more than 0

[Ville Syrjälä]

On Thu, Oct 12, 2017 at 10:30:06PM +0530, PrasannaKumar Muralidharan wrote:
> Warn when refcount > 0 in drm_vblank_cleanup.
> 
> In i915 driver unload drm_vblank_get is added to test whether this patch
> is working. This will be removed once the patch gets tested.
> 
> Signed-off-by: PrasannaKumar Muralidharan 
> ---
>  drivers/gpu/drm/drm_vblank.c| 2 ++
>  drivers/gpu/drm/i915/i915_drv.c | 1 +
>  2 files changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index 70f2b95..2a6630a 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -402,6 +402,8 @@ void drm_vblank_cleanup(struct drm_device *dev)
>   if (dev->num_crtcs == 0)
>   return;
>  
> + WARN_ON(atomic_read(>refcount) > 0);
> +

That won't even compile. Always at least compile + sparse check +
checkpatch your stuff, otherwise you're just going to be wasting
other people's or the CI systems's time.

>   for (pipe = 0; pipe < dev->num_crtcs; pipe++) {
>   struct drm_vblank_crtc *vblank = >vblank[pipe];
>  
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 9f45cfe..c7a93a9 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1373,6 +1373,7 @@ void i915_driver_unload(struct drm_device *dev)
>   struct drm_i915_private *dev_priv = to_i915(dev);
>   struct pci_dev *pdev = dev_priv->drm.pdev;
>  
> + drm_vblank_get(dev_priv, 0);
>   i915_driver_unregister(dev_priv);
>  
>   if (i915_gem_suspend(dev_priv))
> -- 
> 2.10.0
> 
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [RFC] drm: drm_vblank_cleanup: WARN when refcount is more than 0

[PrasannaKumar Muralidharan]

Hi Chris,

On 12 October 2017 at 22:37, Chris Wilson  wrote:
> Quoting PrasannaKumar Muralidharan (2017-10-12 18:00:06)
>> Warn when refcount > 0 in drm_vblank_cleanup.
>>
>> In i915 driver unload drm_vblank_get is added to test whether this patch
>> is working. This will be removed once the patch gets tested.
>
> You want to send it as two patches. The first to add the debug
> infrastructure, then the second to trigger it. We obviously don't want
> to commit a patch that itself warns about the bug it introduces! ;)
> -Chris

True. That's why I marked it as RFC. I hope this will trigger Intel
graphics driver CI so I can wait for the outcome and send a single
patch based on the result. Will that work?

Thanks,
PrasannaKumar
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [RFC] drm: drm_vblank_cleanup: WARN when refcount is more than 0

[Chris Wilson]

Quoting PrasannaKumar Muralidharan (2017-10-12 18:00:06)
> Warn when refcount > 0 in drm_vblank_cleanup.
> 
> In i915 driver unload drm_vblank_get is added to test whether this patch
> is working. This will be removed once the patch gets tested.

You want to send it as two patches. The first to add the debug
infrastructure, then the second to trigger it. We obviously don't want
to commit a patch that itself warns about the bug it introduces! ;)
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx