diff mbox

[v2,2/2] drm/i915/guc: Change values for i915_guc_log_control

Message ID 20180111152441.21676-2-michal.wajdeczko@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Michal Wajdeczko Jan. 11, 2018, 3:24 p.m. UTC
Today we have format mismatch between read/write operations
of i915_guc_log_control entry. For read we return (0, 1..4)
that represents disable/verbosity levels, but for write we
force user to follow internal structure format (0,1,9,11,13).
Let's hide internals from the user and accept same values
as we support for read and related guc_log_level modparam.

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_guc_log.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

Comments

sagar.a.kamble@intel.com Jan. 12, 2018, 5:53 a.m. UTC | #1
On 1/11/2018 8:54 PM, Michal Wajdeczko wrote:
> Today we have format mismatch between read/write operations
> of i915_guc_log_control entry. For read we return (0, 1..4)
> that represents disable/verbosity levels, but for write we
> force user to follow internal structure format (0,1,9,11,13).
0x0, 0x1, 0x11, 0x21, 0x31
> Let's hide internals from the user and accept same values
> as we support for read and related guc_log_level modparam.
>
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_guc_log.c | 27 +++++++++++++++------------
>   1 file changed, 15 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
> index e6d59bf..1719f1e 100644
> --- a/drivers/gpu/drm/i915/intel_guc_log.c
> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
> @@ -58,11 +58,15 @@ static int guc_log_flush(struct intel_guc *guc)
>   	return intel_guc_send(guc, action, ARRAY_SIZE(action));
>   }
>   
> -static int guc_log_control(struct intel_guc *guc, u32 control_val)
> +static int guc_log_control(struct intel_guc *guc, bool enable, u32 verbosity)
>   {
> +	union guc_log_control control_val = { .logging_enabled = enable,
> +					      .reserved1 = 0,
> +					      .verbosity = verbosity,
> +					      .reserved2 = 0 };
>   	u32 action[] = {
>   		INTEL_GUC_ACTION_UK_LOG_ENABLE_LOGGING,
> -		control_val
> +		control_val.value
>   	};
>   
>   	return intel_guc_send(guc, action, ARRAY_SIZE(action));
> @@ -567,28 +571,27 @@ void intel_guc_log_destroy(struct intel_guc *guc)
>   int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val)
>   {
>   	struct intel_guc *guc = &dev_priv->guc;
> -
> -	union guc_log_control log_param;
> +	bool enable_logging = control_val > 0;
> +	u32 verbosity;
>   	int ret;
>   
> -	log_param.value = control_val;
> -
> -	if (log_param.verbosity < GUC_LOG_VERBOSITY_MIN ||
> -	    log_param.verbosity > GUC_LOG_VERBOSITY_MAX)
> +	BUILD_BUG_ON(GUC_LOG_VERBOSITY_MIN);
> +	if (control_val > 1 + GUC_LOG_VERBOSITY_MAX)
>   		return -EINVAL;
>   
>   	/* This combination doesn't make sense & won't have any effect */
> -	if (!log_param.logging_enabled && !i915_modparams.guc_log_level)
> +	if (!enable_logging && !i915_modparams.guc_log_level)
>   		return 0;
>   
> -	ret = guc_log_control(guc, log_param.value);
> +	verbosity = enable_logging ? control_val - 1 : 0;
> +	ret = guc_log_control(guc, enable_logging, verbosity);
>   	if (ret < 0) {
>   		DRM_DEBUG_DRIVER("guc_logging_control action failed %d\n", ret);
>   		return ret;
>   	}
>   
> -	if (log_param.logging_enabled) {
> -		i915_modparams.guc_log_level = 1 + log_param.verbosity;
> +	if (enable_logging) {
> +		i915_modparams.guc_log_level = 1 + verbosity;
>   
>   		/*
>   		 * If log was disabled at boot time, then the relay channel file
Chris Wilson Jan. 18, 2018, 5:21 p.m. UTC | #2
Quoting Sagar Arun Kamble (2018-01-12 05:53:28)
> 
> 
> On 1/11/2018 8:54 PM, Michal Wajdeczko wrote:
> > Today we have format mismatch between read/write operations
> > of i915_guc_log_control entry. For read we return (0, 1..4)
> > that represents disable/verbosity levels, but for write we
> > force user to follow internal structure format (0,1,9,11,13).
> 0x0, 0x1, 0x11, 0x21, 0x31
> > Let's hide internals from the user and accept same values
> > as we support for read and related guc_log_level modparam.
> >
> > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Reviewed-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>

Thanks for the patches and review, pushed.
-Chris
Chris Wilson Jan. 18, 2018, 5:56 p.m. UTC | #3
Quoting Chris Wilson (2018-01-18 17:21:57)
> Quoting Sagar Arun Kamble (2018-01-12 05:53:28)
> > 
> > 
> > On 1/11/2018 8:54 PM, Michal Wajdeczko wrote:
> > > Today we have format mismatch between read/write operations
> > > of i915_guc_log_control entry. For read we return (0, 1..4)
> > > that represents disable/verbosity levels, but for write we
> > > force user to follow internal structure format (0,1,9,11,13).
> > 0x0, 0x1, 0x11, 0x21, 0x31
> > > Let's hide internals from the user and accept same values
> > > as we support for read and related guc_log_level modparam.
> > >
> > > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > > Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> > > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> > > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Reviewed-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> 
> Thanks for the patches and review, pushed.

Oh darn it,

   30.062893] ======================================================
[   30.062894] WARNING: possible circular locking dependency detected
[   30.062895] 4.15.0-rc8-CI-CI_DRM_3648+ #1 Tainted: G     U          
[   30.062896] ------------------------------------------------------
[   30.062897] debugfs_test/1268 is trying to acquire lock:
[   30.062898]  (&dev->struct_mutex){+.+.}, at: [<00000000e4213449>] i915_mutex_lock_interruptible+0x47/0x130 [i915]
[   30.062921] 
               but task is already holding lock:
[   30.062921]  (&mm->mmap_sem){++++}, at: [<00000000dd7adc93>] __do_page_fault+0x106/0x560
[   30.062924] 
               which lock already depends on the new lock.

[   30.062925] 
               the existing dependency chain (in reverse order) is:
[   30.062926] 
               -> #3 (&mm->mmap_sem){++++}:
[   30.062930]        _copy_to_user+0x1e/0x70
[   30.062932]        filldir+0x8c/0xf0
[   30.062934]        dcache_readdir+0xeb/0x160
[   30.062935]        iterate_dir+0xdc/0x140
[   30.062936]        SyS_getdents+0xa0/0x130
[   30.062938]        entry_SYSCALL_64_fastpath+0x22/0x8f
[   30.062939] 
               -> #2 (&sb->s_type->i_mutex_key#3){++++}:
[   30.062942]        start_creating+0x59/0x110
[   30.062944]        __debugfs_create_file+0x2e/0xe0
[   30.062946]        relay_create_buf_file+0x62/0x80
[   30.062947]        relay_late_setup_files+0x84/0x250
[   30.062967]        guc_log_late_setup+0x52/0x110 [i915]
[   30.062985]        i915_guc_log_register+0x3a/0x60 [i915]
[   30.062997]        i915_driver_load+0x7b6/0x1720 [i915]
[   30.063014]        i915_pci_probe+0x2e/0x90 [i915]
[   30.063017]        pci_device_probe+0x9c/0x120
[   30.063018]        driver_probe_device+0x2a3/0x480
[   30.063020]        __driver_attach+0xd9/0xe0
[   30.063021]        bus_for_each_dev+0x57/0x90
[   30.063022]        bus_add_driver+0x168/0x260
[   30.063023]        driver_register+0x52/0xc0
[   30.063024]        do_one_initcall+0x39/0x150
[   30.063025]        do_init_module+0x56/0x1ef
[   30.063026]        load_module+0x231c/0x2d70
[   30.063027]        SyS_finit_module+0xa5/0xe0
[   30.063028]        do_syscall_64+0x59/0x1a0
[   30.063030]        return_from_SYSCALL_64+0x0/0x75
[   30.063030] 
               -> #1 (relay_channels_mutex){+.+.}:
[   30.063034]        relay_open+0x12c/0x2b0
[   30.063051]        guc_log_runtime_create+0xa0/0x220 [i915]
[   30.063067]        intel_guc_log_create+0xec/0x1c0 [i915]
[   30.063083]        intel_guc_init+0x5d/0x100 [i915]
[   30.063100]        intel_uc_init+0x29/0xa0 [i915]
[   30.063116]        i915_gem_init+0x18a/0x540 [i915]
[   30.063128]        i915_driver_load+0xaa9/0x1720 [i915]
[   30.063140]        i915_pci_probe+0x2e/0x90 [i915]
[   30.063141]        pci_device_probe+0x9c/0x120
[   30.063143]        driver_probe_device+0x2a3/0x480
[   30.063144]        __driver_attach+0xd9/0xe0
[   30.063145]        bus_for_each_dev+0x57/0x90
[   30.063146]        bus_add_driver+0x168/0x260
[   30.063147]        driver_register+0x52/0xc0
[   30.063148]        do_one_initcall+0x39/0x150
[   30.063149]        do_init_module+0x56/0x1ef
[   30.063150]        load_module+0x231c/0x2d70
[   30.063151]        SyS_finit_module+0xa5/0xe0
[   30.063152]        do_syscall_64+0x59/0x1a0
[   30.063153]        return_from_SYSCALL_64+0x0/0x75
[   30.063154] 
               -> #0 (&dev->struct_mutex){+.+.}:
[   30.063156]        __mutex_lock+0x81/0x9b0
[   30.063172]        i915_mutex_lock_interruptible+0x47/0x130 [i915]
[   30.063187]        i915_gem_fault+0x201/0x790 [i915]
[   30.063190]        __do_fault+0x15/0x70
[   30.063191]        __handle_mm_fault+0x677/0xdc0
[   30.063193]        handle_mm_fault+0x14f/0x2f0
[   30.063194]        __do_page_fault+0x2d1/0x560
[   30.063195]        page_fault+0x4c/0x60
[   30.063196] 
               other info that might help us debug this:

[   30.063197] Chain exists of:
                 &dev->struct_mutex --> &sb->s_type->i_mutex_key#3 --> &mm->mmap_sem

[   30.063200]  Possible unsafe locking scenario:

[   30.063201]        CPU0                    CPU1
[   30.063201]        ----                    ----
[   30.063202]   lock(&mm->mmap_sem);
[   30.063203]                                lock(&sb->s_type->i_mutex_key#3);
[   30.063205]                                lock(&mm->mmap_sem);
[   30.063206]   lock(&dev->struct_mutex);
[   30.063207] 
                *** DEADLOCK ***

[   30.063208] 1 lock held by debugfs_test/1268:
[   30.063209]  #0:  (&mm->mmap_sem){++++}, at: [<00000000dd7adc93>] __do_page_fault+0x106/0x560
[   30.063211] 
               stack backtrace:
[   30.063213] CPU: 4 PID: 1268 Comm: debugfs_test Tainted: G     U           4.15.0-rc8-CI-CI_DRM_3648+ #1
[   30.063214] Hardware name: System manufacturer System Product Name/Z170 PRO GAMING, BIOS 3402 04/26/2017
[   30.063214] Call Trace:
[   30.063216]  dump_stack+0x5f/0x86
[   30.063219]  print_circular_bug.isra.18+0x1d0/0x2c0
[   30.063221]  __lock_acquire+0x14ae/0x1b60
[   30.063224]  ? lock_acquire+0xaf/0x200
[   30.063226]  lock_acquire+0xaf/0x200
[   30.063240]  ? i915_mutex_lock_interruptible+0x47/0x130 [i915]
[   30.063242]  __mutex_lock+0x81/0x9b0
[   30.063256]  ? i915_mutex_lock_interruptible+0x47/0x130 [i915]
[   30.063270]  ? i915_mutex_lock_interruptible+0x47/0x130 [i915]
[   30.063285]  ? i915_mutex_lock_interruptible+0x47/0x130 [i915]
[   30.063298]  i915_mutex_lock_interruptible+0x47/0x130 [i915]
[   30.063300]  ? __pm_runtime_resume+0x4f/0x80
[   30.063315]  i915_gem_fault+0x201/0x790 [i915]
[   30.063317]  __do_fault+0x15/0x70
[   30.063318]  ? _raw_spin_unlock+0x29/0x40
[   30.063320]  __handle_mm_fault+0x677/0xdc0
[   30.063323]  handle_mm_fault+0x14f/0x2f0
[   30.063324]  __do_page_fault+0x2d1/0x560
[   30.063326]  ? page_fault+0x36/0x60
[   30.063327]  page_fault+0x4c/0x60
[   30.063328] RIP: 0033:0x7f6675e9ae4f
[   30.063329] RSP: 002b:00007ffc3ed48868 EFLAGS: 00010283

I thought I had seen a patch for that?
-Chris
sagar.a.kamble@intel.com Jan. 21, 2018, 8:34 a.m. UTC | #4
On 1/18/2018 11:26 PM, Chris Wilson wrote:
> Quoting Chris Wilson (2018-01-18 17:21:57)
>> Quoting Sagar Arun Kamble (2018-01-12 05:53:28)
>>>
>>> On 1/11/2018 8:54 PM, Michal Wajdeczko wrote:
>>>> Today we have format mismatch between read/write operations
>>>> of i915_guc_log_control entry. For read we return (0, 1..4)
>>>> that represents disable/verbosity levels, but for write we
>>>> force user to follow internal structure format (0,1,9,11,13).
>>> 0x0, 0x1, 0x11, 0x21, 0x31
>>>> Let's hide internals from the user and accept same values
>>>> as we support for read and related guc_log_level modparam.
>>>>
>>>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>>> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>>>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>> Reviewed-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>> Thanks for the patches and review, pushed.
> Oh darn it,
>
>     30.062893] ======================================================
> [   30.062894] WARNING: possible circular locking dependency detected
> [   30.062895] 4.15.0-rc8-CI-CI_DRM_3648+ #1 Tainted: G     U
> [   30.062896] ------------------------------------------------------
> [   30.062897] debugfs_test/1268 is trying to acquire lock:
> [   30.062898]  (&dev->struct_mutex){+.+.}, at: [<00000000e4213449>] i915_mutex_lock_interruptible+0x47/0x130 [i915]
> [   30.062921]
>                 but task is already holding lock:
> [   30.062921]  (&mm->mmap_sem){++++}, at: [<00000000dd7adc93>] __do_page_fault+0x106/0x560
> [   30.062924]
>                 which lock already depends on the new lock.
>
> [   30.062925]
>                 the existing dependency chain (in reverse order) is:
> [   30.062926]
>                 -> #3 (&mm->mmap_sem){++++}:
> [   30.062930]        _copy_to_user+0x1e/0x70
> [   30.062932]        filldir+0x8c/0xf0
> [   30.062934]        dcache_readdir+0xeb/0x160
> [   30.062935]        iterate_dir+0xdc/0x140
> [   30.062936]        SyS_getdents+0xa0/0x130
> [   30.062938]        entry_SYSCALL_64_fastpath+0x22/0x8f
> [   30.062939]
>                 -> #2 (&sb->s_type->i_mutex_key#3){++++}:
> [   30.062942]        start_creating+0x59/0x110
> [   30.062944]        __debugfs_create_file+0x2e/0xe0
> [   30.062946]        relay_create_buf_file+0x62/0x80
> [   30.062947]        relay_late_setup_files+0x84/0x250
> [   30.062967]        guc_log_late_setup+0x52/0x110 [i915]
> [   30.062985]        i915_guc_log_register+0x3a/0x60 [i915]
> [   30.062997]        i915_driver_load+0x7b6/0x1720 [i915]
> [   30.063014]        i915_pci_probe+0x2e/0x90 [i915]
> [   30.063017]        pci_device_probe+0x9c/0x120
> [   30.063018]        driver_probe_device+0x2a3/0x480
> [   30.063020]        __driver_attach+0xd9/0xe0
> [   30.063021]        bus_for_each_dev+0x57/0x90
> [   30.063022]        bus_add_driver+0x168/0x260
> [   30.063023]        driver_register+0x52/0xc0
> [   30.063024]        do_one_initcall+0x39/0x150
> [   30.063025]        do_init_module+0x56/0x1ef
> [   30.063026]        load_module+0x231c/0x2d70
> [   30.063027]        SyS_finit_module+0xa5/0xe0
> [   30.063028]        do_syscall_64+0x59/0x1a0
> [   30.063030]        return_from_SYSCALL_64+0x0/0x75
> [   30.063030]
>                 -> #1 (relay_channels_mutex){+.+.}:
> [   30.063034]        relay_open+0x12c/0x2b0
> [   30.063051]        guc_log_runtime_create+0xa0/0x220 [i915]
> [   30.063067]        intel_guc_log_create+0xec/0x1c0 [i915]
> [   30.063083]        intel_guc_init+0x5d/0x100 [i915]
> [   30.063100]        intel_uc_init+0x29/0xa0 [i915]
> [   30.063116]        i915_gem_init+0x18a/0x540 [i915]
> [   30.063128]        i915_driver_load+0xaa9/0x1720 [i915]
> [   30.063140]        i915_pci_probe+0x2e/0x90 [i915]
> [   30.063141]        pci_device_probe+0x9c/0x120
> [   30.063143]        driver_probe_device+0x2a3/0x480
> [   30.063144]        __driver_attach+0xd9/0xe0
> [   30.063145]        bus_for_each_dev+0x57/0x90
> [   30.063146]        bus_add_driver+0x168/0x260
> [   30.063147]        driver_register+0x52/0xc0
> [   30.063148]        do_one_initcall+0x39/0x150
> [   30.063149]        do_init_module+0x56/0x1ef
> [   30.063150]        load_module+0x231c/0x2d70
> [   30.063151]        SyS_finit_module+0xa5/0xe0
> [   30.063152]        do_syscall_64+0x59/0x1a0
> [   30.063153]        return_from_SYSCALL_64+0x0/0x75
> [   30.063154]
>                 -> #0 (&dev->struct_mutex){+.+.}:
> [   30.063156]        __mutex_lock+0x81/0x9b0
> [   30.063172]        i915_mutex_lock_interruptible+0x47/0x130 [i915]
> [   30.063187]        i915_gem_fault+0x201/0x790 [i915]
> [   30.063190]        __do_fault+0x15/0x70
> [   30.063191]        __handle_mm_fault+0x677/0xdc0
> [   30.063193]        handle_mm_fault+0x14f/0x2f0
> [   30.063194]        __do_page_fault+0x2d1/0x560
> [   30.063195]        page_fault+0x4c/0x60
> [   30.063196]
>                 other info that might help us debug this:
>
> [   30.063197] Chain exists of:
>                   &dev->struct_mutex --> &sb->s_type->i_mutex_key#3 --> &mm->mmap_sem
>
> [   30.063200]  Possible unsafe locking scenario:
>
> [   30.063201]        CPU0                    CPU1
> [   30.063201]        ----                    ----
> [   30.063202]   lock(&mm->mmap_sem);
> [   30.063203]                                lock(&sb->s_type->i_mutex_key#3);
> [   30.063205]                                lock(&mm->mmap_sem);
> [   30.063206]   lock(&dev->struct_mutex);
> [   30.063207]
>                  *** DEADLOCK ***
>
> [   30.063208] 1 lock held by debugfs_test/1268:
> [   30.063209]  #0:  (&mm->mmap_sem){++++}, at: [<00000000dd7adc93>] __do_page_fault+0x106/0x560
> [   30.063211]
>                 stack backtrace:
> [   30.063213] CPU: 4 PID: 1268 Comm: debugfs_test Tainted: G     U           4.15.0-rc8-CI-CI_DRM_3648+ #1
> [   30.063214] Hardware name: System manufacturer System Product Name/Z170 PRO GAMING, BIOS 3402 04/26/2017
> [   30.063214] Call Trace:
> [   30.063216]  dump_stack+0x5f/0x86
> [   30.063219]  print_circular_bug.isra.18+0x1d0/0x2c0
> [   30.063221]  __lock_acquire+0x14ae/0x1b60
> [   30.063224]  ? lock_acquire+0xaf/0x200
> [   30.063226]  lock_acquire+0xaf/0x200
> [   30.063240]  ? i915_mutex_lock_interruptible+0x47/0x130 [i915]
> [   30.063242]  __mutex_lock+0x81/0x9b0
> [   30.063256]  ? i915_mutex_lock_interruptible+0x47/0x130 [i915]
> [   30.063270]  ? i915_mutex_lock_interruptible+0x47/0x130 [i915]
> [   30.063285]  ? i915_mutex_lock_interruptible+0x47/0x130 [i915]
> [   30.063298]  i915_mutex_lock_interruptible+0x47/0x130 [i915]
> [   30.063300]  ? __pm_runtime_resume+0x4f/0x80
> [   30.063315]  i915_gem_fault+0x201/0x790 [i915]
> [   30.063317]  __do_fault+0x15/0x70
> [   30.063318]  ? _raw_spin_unlock+0x29/0x40
> [   30.063320]  __handle_mm_fault+0x677/0xdc0
> [   30.063323]  handle_mm_fault+0x14f/0x2f0
> [   30.063324]  __do_page_fault+0x2d1/0x560
> [   30.063326]  ? page_fault+0x36/0x60
> [   30.063327]  page_fault+0x4c/0x60
> [   30.063328] RIP: 0033:0x7f6675e9ae4f
> [   30.063329] RSP: 002b:00007ffc3ed48868 EFLAGS: 00010283
>
> I thought I had seen a patch for that?
Yes. It was being worked at while this series was also in progress.
Should have pushed that first. Currently checking sanity on trybot and 
will post once it passes.

Thanks,
Sagar
> -Chris
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
index e6d59bf..1719f1e 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -58,11 +58,15 @@  static int guc_log_flush(struct intel_guc *guc)
 	return intel_guc_send(guc, action, ARRAY_SIZE(action));
 }
 
-static int guc_log_control(struct intel_guc *guc, u32 control_val)
+static int guc_log_control(struct intel_guc *guc, bool enable, u32 verbosity)
 {
+	union guc_log_control control_val = { .logging_enabled = enable,
+					      .reserved1 = 0,
+					      .verbosity = verbosity,
+					      .reserved2 = 0 };
 	u32 action[] = {
 		INTEL_GUC_ACTION_UK_LOG_ENABLE_LOGGING,
-		control_val
+		control_val.value
 	};
 
 	return intel_guc_send(guc, action, ARRAY_SIZE(action));
@@ -567,28 +571,27 @@  void intel_guc_log_destroy(struct intel_guc *guc)
 int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val)
 {
 	struct intel_guc *guc = &dev_priv->guc;
-
-	union guc_log_control log_param;
+	bool enable_logging = control_val > 0;
+	u32 verbosity;
 	int ret;
 
-	log_param.value = control_val;
-
-	if (log_param.verbosity < GUC_LOG_VERBOSITY_MIN ||
-	    log_param.verbosity > GUC_LOG_VERBOSITY_MAX)
+	BUILD_BUG_ON(GUC_LOG_VERBOSITY_MIN);
+	if (control_val > 1 + GUC_LOG_VERBOSITY_MAX)
 		return -EINVAL;
 
 	/* This combination doesn't make sense & won't have any effect */
-	if (!log_param.logging_enabled && !i915_modparams.guc_log_level)
+	if (!enable_logging && !i915_modparams.guc_log_level)
 		return 0;
 
-	ret = guc_log_control(guc, log_param.value);
+	verbosity = enable_logging ? control_val - 1 : 0;
+	ret = guc_log_control(guc, enable_logging, verbosity);
 	if (ret < 0) {
 		DRM_DEBUG_DRIVER("guc_logging_control action failed %d\n", ret);
 		return ret;
 	}
 
-	if (log_param.logging_enabled) {
-		i915_modparams.guc_log_level = 1 + log_param.verbosity;
+	if (enable_logging) {
+		i915_modparams.guc_log_level = 1 + verbosity;
 
 		/*
 		 * If log was disabled at boot time, then the relay channel file