[PATCH] drm/i915/guc: Fix a warning message problem during driver unload

[PATCH] drm/i915/guc: Fix a warning message problem during driver unload

[yu.dai]

From: Alex Dai <yu.dai@intel.com>

The device struct_mutex needs to be held before releasing any GEM
objects allocated by GuC.

Signed-off-by: Alex Dai <yu.dai@intel.com>
---
 drivers/gpu/drm/i915/intel_guc_loader.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index 625272f4..4748651 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -631,10 +631,10 @@ void intel_guc_ucode_fini(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
 
+	mutex_lock(&dev->struct_mutex);
 	direct_interrupts_to_host(dev_priv);
 	i915_guc_submission_fini(dev);
 
-	mutex_lock(&dev->struct_mutex);
 	if (guc_fw->guc_fw_obj)
 		drm_gem_object_unreference(&guc_fw->guc_fw_obj->base);
 	guc_fw->guc_fw_obj = NULL;
-- 
2.5.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/guc: Fix a warning message problem during driver unload

[Jani Nikula]

On Thu, 17 Dec 2015, yu.dai@intel.com wrote:
> From: Alex Dai <yu.dai@intel.com>
>
> The device struct_mutex needs to be held before releasing any GEM
> objects allocated by GuC.

This is indeed so, but your patch subject needs to say it fixes an
actual bug rather than a "warning message problem" which makes one think
it's benign.

Also, if you see a warning splat in dmesg, please include that in the
commit message when you fix it. It's *much* easier to match bug reports
with fixes when you have that.

BR,
Jani.


>
> Signed-off-by: Alex Dai <yu.dai@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_guc_loader.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> index 625272f4..4748651 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -631,10 +631,10 @@ void intel_guc_ucode_fini(struct drm_device *dev)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
>  
> +	mutex_lock(&dev->struct_mutex);
>  	direct_interrupts_to_host(dev_priv);
>  	i915_guc_submission_fini(dev);
>  
> -	mutex_lock(&dev->struct_mutex);
>  	if (guc_fw->guc_fw_obj)
>  		drm_gem_object_unreference(&guc_fw->guc_fw_obj->base);
>  	guc_fw->guc_fw_obj = NULL;

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/guc: Fix a warning message problem during driver unload

[Yu Dai]

On 12/18/2015 01:55 AM, Jani Nikula wrote:
> On Thu, 17 Dec 2015, yu.dai@intel.com wrote:
> > From: Alex Dai <yu.dai@intel.com>
> >
> > The device struct_mutex needs to be held before releasing any GEM
> > objects allocated by GuC.
>
> This is indeed so, but your patch subject needs to say it fixes an
> actual bug rather than a "warning message problem" which makes one think
> it's benign.
>
> Also, if you see a warning splat in dmesg, please include that in the
> commit message when you fix it. It's *much* easier to match bug reports
> with fixes when you have that.
>
I will change subject to something like "Fix a potential issue ..." and 
with a warning message log in comment.

Thanks,
Alex
> >
> > Signed-off-by: Alex Dai <yu.dai@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_guc_loader.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> > index 625272f4..4748651 100644
> > --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> > +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> > @@ -631,10 +631,10 @@ void intel_guc_ucode_fini(struct drm_device *dev)
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
> >
> > +	mutex_lock(&dev->struct_mutex);
> >  	direct_interrupts_to_host(dev_priv);
> >  	i915_guc_submission_fini(dev);
> >
> > -	mutex_lock(&dev->struct_mutex);
> >  	if (guc_fw->guc_fw_obj)
> >  		drm_gem_object_unreference(&guc_fw->guc_fw_obj->base);
> >  	guc_fw->guc_fw_obj = NULL;
>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v1] drm/i915/guc: Fix a potential issue during driver unload

[yu.dai]

From: Alex Dai <yu.dai@intel.com>

The device struct_mutex needs to be held before releasing any GEM
objects allocated by GuC.

    WARNING: CPU: 0 PID: 1575 at include/drm/drm_gem.h:217 gem_release_guc_obj+0x5f/0x70 [i915]()
    Call Trace:
     [<ffffffff813c8b64>] dump_stack+0x44/0x60
     [<ffffffff8107d642>] warn_slowpath_common+0x82/0xc0
     [<ffffffff8107d78a>] warn_slowpath_null+0x1a/0x20
     [<ffffffffc02369ef>] gem_release_guc_obj+0x5f/0x70 [i915]
     [<ffffffffc02380ba>] i915_guc_submission_fini+0x1a/0x70 [i915]
     [<ffffffffc0235d09>] intel_guc_ucode_fini+0x29/0xa0 [i915]
     [<ffffffffc029595d>] i915_driver_unload+0x14d/0x290 [i915]
     [<ffffffffc00912a9>] drm_dev_unregister+0x29/0xb0 [drm]
     [<ffffffffc00919d3>] drm_put_dev+0x23/0x60 [drm]
     [<ffffffffc01d51b5>] i915_pci_remove+0x15/0x20 [i915]
     [<ffffffff81419b49>] pci_device_remove+0x39/0xc0
     [<ffffffff81527f51>] __device_release_driver+0xa1/0x150
     [<ffffffff81528a85>] driver_detach+0xb5/0xc0
     [<ffffffff815279b5>] bus_remove_driver+0x55/0xd0
     [<ffffffff8152921c>] driver_unregister+0x2c/0x50
     [<ffffffff814181e9>] pci_unregister_driver+0x29/0x90
     [<ffffffffc0093174>] drm_pci_exit+0x94/0xb0 [drm]
     [<ffffffffc02960c4>] i915_exit+0x20/0xf5c [i915]
     [<ffffffff81102bc5>] SyS_delete_module+0x1b5/0x210
     [<ffffffff817fc2b6>] entry_SYSCALL_64_fastpath+0x16/0x75
    ---[ end trace f711c4eb63588bb7 ]---

v1: Add backtrace log.

Signed-off-by: Alex Dai <yu.dai@intel.com>

diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index 625272f4..4748651 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -631,10 +631,10 @@ void intel_guc_ucode_fini(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
 
+	mutex_lock(&dev->struct_mutex);
 	direct_interrupts_to_host(dev_priv);
 	i915_guc_submission_fini(dev);
 
-	mutex_lock(&dev->struct_mutex);
 	if (guc_fw->guc_fw_obj)
 		drm_gem_object_unreference(&guc_fw->guc_fw_obj->base);
 	guc_fw->guc_fw_obj = NULL;
-- 
2.5.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ warning: Fi.CI.BAT

[Patchwork]

== Summary ==

Built on 7cdc548e77f503593b83a1c5d58f4dcc862c17e2 drm-intel-nightly: 2015y-12m-18d-19h-26m-21s UTC integration manifest

Test kms_flip:
        Subgroup basic-flip-vs-modeset:
                dmesg-warn -> PASS       (skl-i5k-2)
                dmesg-warn -> PASS       (hsw-brixbox)
                dmesg-warn -> PASS       (byt-nuc)
        Subgroup basic-flip-vs-wf_vblank:
                pass       -> DMESG-WARN (byt-nuc)
Test kms_pipe_crc_basic:
        Subgroup read-crc-pipe-a:
                dmesg-warn -> PASS       (snb-x220t)
        Subgroup read-crc-pipe-a-frame-sequence:
                dmesg-warn -> PASS       (byt-nuc)
        Subgroup read-crc-pipe-b:
                pass       -> DMESG-WARN (snb-dellxps)
        Subgroup read-crc-pipe-c:
                dmesg-warn -> PASS       (skl-i7k-2)
        Subgroup suspend-read-crc-pipe-b:
                pass       -> DMESG-WARN (snb-x220t)
Test kms_setmode:
        Subgroup basic-clone-single-crtc:
                dmesg-warn -> PASS       (snb-dellxps)

bdw-nuci7        total:135  pass:125  dwarn:1   dfail:0   fail:0   skip:9  
bdw-ultra        total:132  pass:124  dwarn:2   dfail:0   fail:0   skip:6  
byt-nuc          total:135  pass:120  dwarn:2   dfail:0   fail:0   skip:13 
hsw-brixbox      total:135  pass:127  dwarn:1   dfail:0   fail:0   skip:7  
hsw-gt2          total:135  pass:130  dwarn:1   dfail:0   fail:0   skip:4  
ilk-hp8440p      total:135  pass:100  dwarn:0   dfail:0   fail:0   skip:35 
ivb-t430s        total:135  pass:127  dwarn:2   dfail:0   fail:0   skip:6  
skl-i5k-2        total:135  pass:123  dwarn:4   dfail:0   fail:0   skip:8  
skl-i7k-2        total:135  pass:123  dwarn:4   dfail:0   fail:0   skip:8  
snb-dellxps      total:135  pass:121  dwarn:2   dfail:0   fail:0   skip:12 
snb-x220t        total:135  pass:121  dwarn:2   dfail:0   fail:1   skip:11 

Results at /archive/results/CI_IGT_test/Patchwork_727/

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v1] drm/i915/guc: Fix a potential issue during driver unload

[Daniel Vetter]

On Fri, Dec 18, 2015 at 12:38:59PM -0800, yu.dai@intel.com wrote:
> From: Alex Dai <yu.dai@intel.com>
> 
> The device struct_mutex needs to be held before releasing any GEM
> objects allocated by GuC.
> 
>     WARNING: CPU: 0 PID: 1575 at include/drm/drm_gem.h:217 gem_release_guc_obj+0x5f/0x70 [i915]()
>     Call Trace:
>      [<ffffffff813c8b64>] dump_stack+0x44/0x60
>      [<ffffffff8107d642>] warn_slowpath_common+0x82/0xc0
>      [<ffffffff8107d78a>] warn_slowpath_null+0x1a/0x20
>      [<ffffffffc02369ef>] gem_release_guc_obj+0x5f/0x70 [i915]
>      [<ffffffffc02380ba>] i915_guc_submission_fini+0x1a/0x70 [i915]
>      [<ffffffffc0235d09>] intel_guc_ucode_fini+0x29/0xa0 [i915]
>      [<ffffffffc029595d>] i915_driver_unload+0x14d/0x290 [i915]
>      [<ffffffffc00912a9>] drm_dev_unregister+0x29/0xb0 [drm]
>      [<ffffffffc00919d3>] drm_put_dev+0x23/0x60 [drm]
>      [<ffffffffc01d51b5>] i915_pci_remove+0x15/0x20 [i915]
>      [<ffffffff81419b49>] pci_device_remove+0x39/0xc0
>      [<ffffffff81527f51>] __device_release_driver+0xa1/0x150
>      [<ffffffff81528a85>] driver_detach+0xb5/0xc0
>      [<ffffffff815279b5>] bus_remove_driver+0x55/0xd0
>      [<ffffffff8152921c>] driver_unregister+0x2c/0x50
>      [<ffffffff814181e9>] pci_unregister_driver+0x29/0x90
>      [<ffffffffc0093174>] drm_pci_exit+0x94/0xb0 [drm]
>      [<ffffffffc02960c4>] i915_exit+0x20/0xf5c [i915]
>      [<ffffffff81102bc5>] SyS_delete_module+0x1b5/0x210
>      [<ffffffff817fc2b6>] entry_SYSCALL_64_fastpath+0x16/0x75
>     ---[ end trace f711c4eb63588bb7 ]---
> 
> v1: Add backtrace log.
> 

When resending a patch, please add everyone who commented on the previous
patch to the Cc: list here.

Thanks, Daniel

> Signed-off-by: Alex Dai <yu.dai@intel.com>
> 
> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> index 625272f4..4748651 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -631,10 +631,10 @@ void intel_guc_ucode_fini(struct drm_device *dev)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
>  
> +	mutex_lock(&dev->struct_mutex);
>  	direct_interrupts_to_host(dev_priv);
>  	i915_guc_submission_fini(dev);
>  
> -	mutex_lock(&dev->struct_mutex);
>  	if (guc_fw->guc_fw_obj)
>  		drm_gem_object_unreference(&guc_fw->guc_fw_obj->base);
>  	guc_fw->guc_fw_obj = NULL;
> -- 
> 2.5.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx