Re: [Intel-gfx] [PATCH 8/8] drm/i915/skl+: debugfs entry to control IPC

[Maarten Lankhorst]

Op 10-08-17 om 10:56 schreef Mahesh Kumar:
> Hi,
>
>
> On Wednesday 09 August 2017 03:16 PM, Maarten Lankhorst wrote:
>> Op 18-07-17 om 14:49 schreef Mahesh Kumar:
>>> From: "Kumar, Mahesh" 
>>>
>>> This patch creates an entry in debugfs to check the status of IPC.
>>> This can also be used to enable/disable IPC in supported platforms.
>>>
>>> Signed-off-by: Mahesh Kumar 
>>> ---
>>>   drivers/gpu/drm/i915/i915_debugfs.c | 73 
>>> -
>>>   1 file changed, 72 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
>>> b/drivers/gpu/drm/i915/i915_debugfs.c
>>> index 2ef75c1a6119..368f64de0fdc 100644
>>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>>> @@ -3592,6 +3592,76 @@ static int i915_wa_registers(struct seq_file *m, 
>>> void *unused)
>>>   return 0;
>>>   }
>>>   +static int i915_ipc_status_show(struct seq_file *m, void *data)
>>> +{
>>> +struct drm_i915_private *dev_priv = m->private;
>>> +
>>> +seq_printf(m, "Isochronous Priority Control: %s\n",
>>> +enableddisabled(dev_priv->ipc_enabled));
>>> +return 0;
>>> +}
>>> +
>>> +static int i915_ipc_status_open(struct inode *inode, struct file *file)
>>> +{
>>> +struct drm_i915_private *dev_priv = inode->i_private;
>>> +
>>> +if (HAS_IPC(dev_priv))
>>> +return -ENODEV;
>>> +
>>> +return single_open(file, i915_ipc_status_show, dev_priv);
>>> +}
>>> +
>>> +static ssize_t i915_ipc_status_write(struct file *file, const char __user 
>>> *ubuf,
>>> + size_t len, loff_t *offp)
>>> +{
>>> +struct seq_file *m = file->private_data;
>>> +struct drm_i915_private *dev_priv = m->private;
>>> +char *newline;
>>> +char tmp[16];
>>> +bool enable;
>>> +
>>> +if (HAS_IPC(dev_priv))
>>> +return -ENODEV;
>>> +
>>> +if (len >= sizeof(tmp))
>>> +return -EINVAL;
>>> +
>>> +if (copy_from_user(tmp, ubuf, len))
>>> +return -EFAULT;
>>> +
>>> +tmp[len] = '\0';
>>> +
>>> +/* Strip newline, if any */
>>> +newline = strchr(tmp, '\n');
>>> +if (newline)
>>> +*newline = '\0';
>>> +
>>> +if (strcmp(tmp, "0") == 0 || strcmp(tmp, "disable") == 0 ||
>>> +strcmp(tmp, "off") == 0 || strcmp(tmp, "dis") == 0)
>>> +enable = false;
>>> +else if (strcmp(tmp, "1") == 0 || strcmp(tmp, "enable") == 0 ||
>>> +strcmp(tmp, "on") == 0 || strcmp(tmp, "en") == 0)
>>> +enable = true;
>>> +else
>>> +return -EINVAL;
>>> +
>>> +intel_runtime_pm_get(dev_priv);
>>> +dev_priv->ipc_enabled = enable;
>>> +intel_enable_ipc(dev_priv);
>>> +intel_runtime_pm_put(dev_priv);
>> Hm, intel_enable_ipc should take a bool on whether to enable ipc.
> intel_enable_ipc takes decision to enable/disable ipc based on 
> dev_priv->ipc_enabled value. which is anyway input to function.
>>
>> Forcefully enabling IPC here might give weird effects until the next plane 
>> update. The transition watermarks are not updated yet until the next commit. 
>> This could probably be handled here, but that would be overkill..
> yes, agree, but I don't wanted to call a commit internally. Will add a 
> drm_info when changing IPC disable => enable as suggested. 

True, perhaps we could set dev_priv->wm.distrust_bios_wm to force recalculation 
next time?

~Maarten

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

Re: [Intel-gfx] [PATCH 8/8] drm/i915/skl+: debugfs entry to control IPC

[Mahesh Kumar]

Hi,


On Wednesday 09 August 2017 03:16 PM, Maarten Lankhorst wrote:

Op 18-07-17 om 14:49 schreef Mahesh Kumar:

From: "Kumar, Mahesh" 

This patch creates an entry in debugfs to check the status of IPC.
This can also be used to enable/disable IPC in supported platforms.

Signed-off-by: Mahesh Kumar 
---
  drivers/gpu/drm/i915/i915_debugfs.c | 73 -
  1 file changed, 72 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
b/drivers/gpu/drm/i915/i915_debugfs.c
index 2ef75c1a6119..368f64de0fdc 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -3592,6 +3592,76 @@ static int i915_wa_registers(struct seq_file *m, void 
*unused)
return 0;
  }
  
+static int i915_ipc_status_show(struct seq_file *m, void *data)

+{
+   struct drm_i915_private *dev_priv = m->private;
+
+   seq_printf(m, "Isochronous Priority Control: %s\n",
+   enableddisabled(dev_priv->ipc_enabled));
+   return 0;
+}
+
+static int i915_ipc_status_open(struct inode *inode, struct file *file)
+{
+   struct drm_i915_private *dev_priv = inode->i_private;
+
+   if (HAS_IPC(dev_priv))
+   return -ENODEV;
+
+   return single_open(file, i915_ipc_status_show, dev_priv);
+}
+
+static ssize_t i915_ipc_status_write(struct file *file, const char __user 
*ubuf,
+size_t len, loff_t *offp)
+{
+   struct seq_file *m = file->private_data;
+   struct drm_i915_private *dev_priv = m->private;
+   char *newline;
+   char tmp[16];
+   bool enable;
+
+   if (HAS_IPC(dev_priv))
+   return -ENODEV;
+
+   if (len >= sizeof(tmp))
+   return -EINVAL;
+
+   if (copy_from_user(tmp, ubuf, len))
+   return -EFAULT;
+
+   tmp[len] = '\0';
+
+   /* Strip newline, if any */
+   newline = strchr(tmp, '\n');
+   if (newline)
+   *newline = '\0';
+
+   if (strcmp(tmp, "0") == 0 || strcmp(tmp, "disable") == 0 ||
+   strcmp(tmp, "off") == 0 || strcmp(tmp, "dis") == 0)
+   enable = false;
+   else if (strcmp(tmp, "1") == 0 || strcmp(tmp, "enable") == 0 ||
+   strcmp(tmp, "on") == 0 || strcmp(tmp, "en") == 0)
+   enable = true;
+   else
+   return -EINVAL;
+
+   intel_runtime_pm_get(dev_priv);
+   dev_priv->ipc_enabled = enable;
+   intel_enable_ipc(dev_priv);
+   intel_runtime_pm_put(dev_priv);

Hm, intel_enable_ipc should take a bool on whether to enable ipc.
intel_enable_ipc takes decision to enable/disable ipc based on 
dev_priv->ipc_enabled value. which is anyway input to function.


Forcefully enabling IPC here might give weird effects until the next plane 
update. The transition watermarks are not updated yet until the next commit. 
This could probably be handled here, but that would be overkill..
yes, agree, but I don't wanted to call a commit internally. Will add a 
drm_info when changing IPC disable => enable as suggested.

-Mahesh


Perhaps add a drm_info or something about it when setting enable = true, when 
it was previously false?

+
+   return len;
+}
+
+static const struct file_operations i915_ipc_status_fops = {
+   .owner = THIS_MODULE,
+   .open = i915_ipc_status_open,
+   .read = seq_read,
+   .llseek = seq_lseek,
+   .release = single_release,
+   .write = i915_ipc_status_write
+};
+
  static int i915_ddb_info(struct seq_file *m, void *unused)
  {
struct drm_i915_private *dev_priv = node_to_i915(m->private);
@@ -4929,7 +4999,8 @@ static const struct i915_debugfs_files {
{"i915_dp_test_type", _displayport_test_type_fops},
{"i915_dp_test_active", _displayport_test_active_fops},
{"i915_guc_log_control", _guc_log_control_fops},
-   {"i915_hpd_storm_ctl", _hpd_storm_ctl_fops}
+   {"i915_hpd_storm_ctl", _hpd_storm_ctl_fops},
+   {"i915_ipc_status", _ipc_status_fops}
  };
  
  int i915_debugfs_register(struct drm_i915_private *dev_priv)




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

Re: [Intel-gfx] [PATCH 8/8] drm/i915/skl+: debugfs entry to control IPC

[Mahesh Kumar]

Hi,

Thanks for review.


On Wednesday 09 August 2017 03:03 PM, Maarten Lankhorst wrote:

Op 18-07-17 om 14:49 schreef Mahesh Kumar:

From: "Kumar, Mahesh" 

This patch creates an entry in debugfs to check the status of IPC.
This can also be used to enable/disable IPC in supported platforms.

Signed-off-by: Mahesh Kumar 
---
  drivers/gpu/drm/i915/i915_debugfs.c | 73 -
  1 file changed, 72 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
b/drivers/gpu/drm/i915/i915_debugfs.c
index 2ef75c1a6119..368f64de0fdc 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -3592,6 +3592,76 @@ static int i915_wa_registers(struct seq_file *m, void 
*unused)
return 0;
  }
  
+static int i915_ipc_status_show(struct seq_file *m, void *data)

+{
+   struct drm_i915_private *dev_priv = m->private;
+
+   seq_printf(m, "Isochronous Priority Control: %s\n",
+   enableddisabled(dev_priv->ipc_enabled));
+   return 0;
+}
+
+static int i915_ipc_status_open(struct inode *inode, struct file *file)
+{
+   struct drm_i915_private *dev_priv = inode->i_private;
+
+   if (HAS_IPC(dev_priv))
+   return -ENODEV;

I take it you didn't test this version of the patch? :p
hmm, I switched HAS_IPC macro in this version of patch only. will fix 
this. thanks for catching it.

+
+   return single_open(file, i915_ipc_status_show, dev_priv);
+}
+
+static ssize_t i915_ipc_status_write(struct file *file, const char __user 
*ubuf,
+size_t len, loff_t *offp)
+{
+   struct seq_file *m = file->private_data;
+   struct drm_i915_private *dev_priv = m->private;
+   char *newline;
+   char tmp[16];
+   bool enable;
+
+   if (HAS_IPC(dev_priv))
+   return -ENODEV;

Again, though I would remove this check since you already test in open().

ok, will remove this check.

+   if (len >= sizeof(tmp))
+   return -EINVAL;
+
+   if (copy_from_user(tmp, ubuf, len))
+   return -EFAULT;
+
+   tmp[len] = '\0';
+
+   /* Strip newline, if any */
+   newline = strchr(tmp, '\n');
+   if (newline)
+   *newline = '\0';
+
+   if (strcmp(tmp, "0") == 0 || strcmp(tmp, "disable") == 0 ||
+   strcmp(tmp, "off") == 0 || strcmp(tmp, "dis") == 0)
+   enable = false;
+   else if (strcmp(tmp, "1") == 0 || strcmp(tmp, "enable") == 0 ||
+   strcmp(tmp, "on") == 0 || strcmp(tmp, "en") == 0)
+   enable = true;
+   else
+   return -EINVAL;

Maybe replace with kstrtobool_from_user, and use yesno for ipc_enabled in 
show()? That way you won't have to do all these special cases here. :)

kstrtobool_from_user sounds good, will use this macro :)

-Mahesh

+
+   intel_runtime_pm_get(dev_priv);
+   dev_priv->ipc_enabled = enable;
+   intel_enable_ipc(dev_priv);
+   intel_runtime_pm_put(dev_priv);
+
+   return len;
+}
+
+static const struct file_operations i915_ipc_status_fops = {
+   .owner = THIS_MODULE,
+   .open = i915_ipc_status_open,
+   .read = seq_read,
+   .llseek = seq_lseek,
+   .release = single_release,
+   .write = i915_ipc_status_write
+};
+
  static int i915_ddb_info(struct seq_file *m, void *unused)
  {
struct drm_i915_private *dev_priv = node_to_i915(m->private);
@@ -4929,7 +4999,8 @@ static const struct i915_debugfs_files {
{"i915_dp_test_type", _displayport_test_type_fops},
{"i915_dp_test_active", _displayport_test_active_fops},
{"i915_guc_log_control", _guc_log_control_fops},
-   {"i915_hpd_storm_ctl", _hpd_storm_ctl_fops}
+   {"i915_hpd_storm_ctl", _hpd_storm_ctl_fops},
+   {"i915_ipc_status", _ipc_status_fops}
  };
  
  int i915_debugfs_register(struct drm_i915_private *dev_priv)




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

Re: [Intel-gfx] [PATCH 8/8] drm/i915/skl+: debugfs entry to control IPC

[Maarten Lankhorst]

Op 18-07-17 om 14:49 schreef Mahesh Kumar:
> From: "Kumar, Mahesh" 
>
> This patch creates an entry in debugfs to check the status of IPC.
> This can also be used to enable/disable IPC in supported platforms.
>
> Signed-off-by: Mahesh Kumar 
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 73 
> -
>  1 file changed, 72 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index 2ef75c1a6119..368f64de0fdc 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -3592,6 +3592,76 @@ static int i915_wa_registers(struct seq_file *m, void 
> *unused)
>   return 0;
>  }
>  
> +static int i915_ipc_status_show(struct seq_file *m, void *data)
> +{
> + struct drm_i915_private *dev_priv = m->private;
> +
> + seq_printf(m, "Isochronous Priority Control: %s\n",
> + enableddisabled(dev_priv->ipc_enabled));
> + return 0;
> +}
> +
> +static int i915_ipc_status_open(struct inode *inode, struct file *file)
> +{
> + struct drm_i915_private *dev_priv = inode->i_private;
> +
> + if (HAS_IPC(dev_priv))
> + return -ENODEV;
> +
> + return single_open(file, i915_ipc_status_show, dev_priv);
> +}
> +
> +static ssize_t i915_ipc_status_write(struct file *file, const char __user 
> *ubuf,
> +  size_t len, loff_t *offp)
> +{
> + struct seq_file *m = file->private_data;
> + struct drm_i915_private *dev_priv = m->private;
> + char *newline;
> + char tmp[16];
> + bool enable;
> +
> + if (HAS_IPC(dev_priv))
> + return -ENODEV;
> +
> + if (len >= sizeof(tmp))
> + return -EINVAL;
> +
> + if (copy_from_user(tmp, ubuf, len))
> + return -EFAULT;
> +
> + tmp[len] = '\0';
> +
> + /* Strip newline, if any */
> + newline = strchr(tmp, '\n');
> + if (newline)
> + *newline = '\0';
> +
> + if (strcmp(tmp, "0") == 0 || strcmp(tmp, "disable") == 0 ||
> + strcmp(tmp, "off") == 0 || strcmp(tmp, "dis") == 0)
> + enable = false;
> + else if (strcmp(tmp, "1") == 0 || strcmp(tmp, "enable") == 0 ||
> + strcmp(tmp, "on") == 0 || strcmp(tmp, "en") == 0)
> + enable = true;
> + else
> + return -EINVAL;
> +
> + intel_runtime_pm_get(dev_priv);
> + dev_priv->ipc_enabled = enable;
> + intel_enable_ipc(dev_priv);
> + intel_runtime_pm_put(dev_priv);
Hm, intel_enable_ipc should take a bool on whether to enable ipc.

Forcefully enabling IPC here might give weird effects until the next plane 
update. The transition watermarks are not updated yet until the next commit. 
This could probably be handled here, but that would be overkill..

Perhaps add a drm_info or something about it when setting enable = true, when 
it was previously false?
> +
> + return len;
> +}
> +
> +static const struct file_operations i915_ipc_status_fops = {
> + .owner = THIS_MODULE,
> + .open = i915_ipc_status_open,
> + .read = seq_read,
> + .llseek = seq_lseek,
> + .release = single_release,
> + .write = i915_ipc_status_write
> +};
> +
>  static int i915_ddb_info(struct seq_file *m, void *unused)
>  {
>   struct drm_i915_private *dev_priv = node_to_i915(m->private);
> @@ -4929,7 +4999,8 @@ static const struct i915_debugfs_files {
>   {"i915_dp_test_type", _displayport_test_type_fops},
>   {"i915_dp_test_active", _displayport_test_active_fops},
>   {"i915_guc_log_control", _guc_log_control_fops},
> - {"i915_hpd_storm_ctl", _hpd_storm_ctl_fops}
> + {"i915_hpd_storm_ctl", _hpd_storm_ctl_fops},
> + {"i915_ipc_status", _ipc_status_fops}
>  };
>  
>  int i915_debugfs_register(struct drm_i915_private *dev_priv)


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

Re: [Intel-gfx] [PATCH 8/8] drm/i915/skl+: debugfs entry to control IPC

[Maarten Lankhorst]

Op 18-07-17 om 14:49 schreef Mahesh Kumar:
> From: "Kumar, Mahesh" 
>
> This patch creates an entry in debugfs to check the status of IPC.
> This can also be used to enable/disable IPC in supported platforms.
>
> Signed-off-by: Mahesh Kumar 
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 73 
> -
>  1 file changed, 72 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index 2ef75c1a6119..368f64de0fdc 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -3592,6 +3592,76 @@ static int i915_wa_registers(struct seq_file *m, void 
> *unused)
>   return 0;
>  }
>  
> +static int i915_ipc_status_show(struct seq_file *m, void *data)
> +{
> + struct drm_i915_private *dev_priv = m->private;
> +
> + seq_printf(m, "Isochronous Priority Control: %s\n",
> + enableddisabled(dev_priv->ipc_enabled));
> + return 0;
> +}
> +
> +static int i915_ipc_status_open(struct inode *inode, struct file *file)
> +{
> + struct drm_i915_private *dev_priv = inode->i_private;
> +
> + if (HAS_IPC(dev_priv))
> + return -ENODEV;
I take it you didn't test this version of the patch? :p
> +
> + return single_open(file, i915_ipc_status_show, dev_priv);
> +}
> +
> +static ssize_t i915_ipc_status_write(struct file *file, const char __user 
> *ubuf,
> +  size_t len, loff_t *offp)
> +{
> + struct seq_file *m = file->private_data;
> + struct drm_i915_private *dev_priv = m->private;
> + char *newline;
> + char tmp[16];
> + bool enable;
> +
> + if (HAS_IPC(dev_priv))
> + return -ENODEV;
Again, though I would remove this check since you already test in open().
> + if (len >= sizeof(tmp))
> + return -EINVAL;
> +
> + if (copy_from_user(tmp, ubuf, len))
> + return -EFAULT;
> +
> + tmp[len] = '\0';
> +
> + /* Strip newline, if any */
> + newline = strchr(tmp, '\n');
> + if (newline)
> + *newline = '\0';
> +
> + if (strcmp(tmp, "0") == 0 || strcmp(tmp, "disable") == 0 ||
> + strcmp(tmp, "off") == 0 || strcmp(tmp, "dis") == 0)
> + enable = false;
> + else if (strcmp(tmp, "1") == 0 || strcmp(tmp, "enable") == 0 ||
> + strcmp(tmp, "on") == 0 || strcmp(tmp, "en") == 0)
> + enable = true;
> + else
> + return -EINVAL;
Maybe replace with kstrtobool_from_user, and use yesno for ipc_enabled in 
show()? That way you won't have to do all these special cases here. :)
> +
> + intel_runtime_pm_get(dev_priv);
> + dev_priv->ipc_enabled = enable;
> + intel_enable_ipc(dev_priv);
> + intel_runtime_pm_put(dev_priv);
> +
> + return len;
> +}
> +
> +static const struct file_operations i915_ipc_status_fops = {
> + .owner = THIS_MODULE,
> + .open = i915_ipc_status_open,
> + .read = seq_read,
> + .llseek = seq_lseek,
> + .release = single_release,
> + .write = i915_ipc_status_write
> +};
> +
>  static int i915_ddb_info(struct seq_file *m, void *unused)
>  {
>   struct drm_i915_private *dev_priv = node_to_i915(m->private);
> @@ -4929,7 +4999,8 @@ static const struct i915_debugfs_files {
>   {"i915_dp_test_type", _displayport_test_type_fops},
>   {"i915_dp_test_active", _displayport_test_active_fops},
>   {"i915_guc_log_control", _guc_log_control_fops},
> - {"i915_hpd_storm_ctl", _hpd_storm_ctl_fops}
> + {"i915_hpd_storm_ctl", _hpd_storm_ctl_fops},
> + {"i915_ipc_status", _ipc_status_fops}
>  };
>  
>  int i915_debugfs_register(struct drm_i915_private *dev_priv)


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