* [PATCH v2] drm/i915: Add debugfs to read any DPCD register
@ 2015-04-08 11:41 Durgadoss R
2015-04-16 14:07 ` Jani Nikula
0 siblings, 1 reply; 7+ messages in thread
From: Durgadoss R @ 2015-04-08 11:41 UTC (permalink / raw)
To: intel-gfx
This patch creates a connector specific debugfs
interface to read any particular DPCD register.
The DPCD register address (hex format) is written
to 'i915_dpcd_addr' interface and the corresponding
value can be read from 'i915_dpcd_val' interface.
Example usage:
echo 0x70 > i915_dpcd_addr
cat i915_dpcd_val
0x1
v2: Address Jani's comments.
Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
---
drivers/gpu/drm/i915/i915_debugfs.c | 88 ++++++++++++++++++++++++++++++++++++-
drivers/gpu/drm/i915/intel_drv.h | 1 +
2 files changed, 88 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 10ca511..f1f365e 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -4865,6 +4865,87 @@ static const struct file_operations i915_dpcd_fops = {
.release = single_release,
};
+static ssize_t i915_dpcd_addr_write(struct file *file, const char __user *ubuf,
+ size_t len, loff_t *offp)
+{
+ struct seq_file *m = file->private_data;
+ struct drm_connector *connector = m->private;
+ struct intel_dp *intel_dp =
+ enc_to_intel_dp(&intel_attached_encoder(connector)->base);
+ unsigned int reg_addr;
+ char tmp[16];
+
+ if (len >= sizeof(tmp))
+ return -EINVAL;
+
+ if (copy_from_user(tmp, ubuf, len))
+ return -EFAULT;
+
+ tmp[len] = '\0';
+
+ if (kstrtouint(tmp, 16, ®_addr))
+ return -EINVAL;
+
+ intel_dp->debugfs_dpcd_addr = reg_addr;
+
+ return len;
+}
+
+static int i915_dpcd_addr_show(struct seq_file *m, void *data)
+{
+ struct drm_connector *connector = m->private;
+ struct intel_dp *intel_dp =
+ enc_to_intel_dp(&intel_attached_encoder(connector)->base);
+
+ seq_printf(m, "0x%x\n", intel_dp->debugfs_dpcd_addr);
+
+ return 0;
+}
+
+static int i915_dpcd_addr_open(struct inode *inode, struct file *file)
+{
+ return single_open(file, i915_dpcd_addr_show, inode->i_private);
+}
+
+static const struct file_operations i915_dpcd_addr_fops = {
+ .owner = THIS_MODULE,
+ .open = i915_dpcd_addr_open,
+ .read = seq_read,
+ .write = i915_dpcd_addr_write,
+ .llseek = seq_lseek,
+ .release = single_release,
+};
+
+static int i915_dpcd_val_show(struct seq_file *m, void *data)
+{
+ struct drm_connector *connector = m->private;
+ struct intel_dp *intel_dp =
+ enc_to_intel_dp(&intel_attached_encoder(connector)->base);
+ u8 val;
+ int ret;
+
+ ret = drm_dp_dpcd_readb(&intel_dp->aux,
+ intel_dp->debugfs_dpcd_addr, &val);
+ if (ret < 0)
+ return ret;
+
+ seq_printf(m, "0x%x\n", val);
+ return 0;
+}
+
+static int i915_dpcd_val_open(struct inode *inode, struct file *file)
+{
+ return single_open(file, i915_dpcd_val_show, inode->i_private);
+}
+
+static const struct file_operations i915_dpcd_val_fops = {
+ .owner = THIS_MODULE,
+ .open = i915_dpcd_val_open,
+ .read = seq_read,
+ .llseek = seq_lseek,
+ .release = single_release,
+};
+
/**
* i915_debugfs_connector_add - add i915 specific connector debugfs files
* @connector: pointer to a registered drm_connector
@@ -4883,9 +4964,14 @@ int i915_debugfs_connector_add(struct drm_connector *connector)
return -ENODEV;
if (connector->connector_type == DRM_MODE_CONNECTOR_DisplayPort ||
- connector->connector_type == DRM_MODE_CONNECTOR_eDP)
+ connector->connector_type == DRM_MODE_CONNECTOR_eDP) {
debugfs_create_file("i915_dpcd", S_IRUGO, root, connector,
&i915_dpcd_fops);
+ debugfs_create_file("i915_dpcd_addr", S_IRUGO | S_IWUSR,
+ root, connector, &i915_dpcd_addr_fops);
+ debugfs_create_file("i915_dpcd_val", S_IRUGO, root,
+ connector, &i915_dpcd_val_fops);
+ }
return 0;
}
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 4799b11..0594baa 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -642,6 +642,7 @@ struct intel_dp {
unsigned long last_power_cycle;
unsigned long last_power_on;
unsigned long last_backlight_off;
+ unsigned int debugfs_dpcd_addr;
struct notifier_block edp_notifier;
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] drm/i915: Add debugfs to read any DPCD register
2015-04-08 11:41 [PATCH v2] drm/i915: Add debugfs to read any DPCD register Durgadoss R
@ 2015-04-16 14:07 ` Jani Nikula
2015-04-16 14:32 ` Ville Syrjälä
0 siblings, 1 reply; 7+ messages in thread
From: Jani Nikula @ 2015-04-16 14:07 UTC (permalink / raw)
To: Durgadoss R, intel-gfx; +Cc: Syrjala, Ville, Zanoni, Paulo R
On Wed, 08 Apr 2015, Durgadoss R <durgadoss.r@intel.com> wrote:
> This patch creates a connector specific debugfs
> interface to read any particular DPCD register.
> The DPCD register address (hex format) is written
> to 'i915_dpcd_addr' interface and the corresponding
> value can be read from 'i915_dpcd_val' interface.
>
> Example usage:
> echo 0x70 > i915_dpcd_addr
> cat i915_dpcd_val
> 0x1
>
> v2: Address Jani's comments.
So, code-wise this looks good to me.
But like I said in my review of v1, I am undecided on whether we want to
have this interface or not. I could really be persuaded either way.
Perhaps Ville or Paulo have an opinion?
BR,
Jani.
>
> Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> ---
> drivers/gpu/drm/i915/i915_debugfs.c | 88 ++++++++++++++++++++++++++++++++++++-
> drivers/gpu/drm/i915/intel_drv.h | 1 +
> 2 files changed, 88 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 10ca511..f1f365e 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -4865,6 +4865,87 @@ static const struct file_operations i915_dpcd_fops = {
> .release = single_release,
> };
>
> +static ssize_t i915_dpcd_addr_write(struct file *file, const char __user *ubuf,
> + size_t len, loff_t *offp)
> +{
> + struct seq_file *m = file->private_data;
> + struct drm_connector *connector = m->private;
> + struct intel_dp *intel_dp =
> + enc_to_intel_dp(&intel_attached_encoder(connector)->base);
> + unsigned int reg_addr;
> + char tmp[16];
> +
> + if (len >= sizeof(tmp))
> + return -EINVAL;
> +
> + if (copy_from_user(tmp, ubuf, len))
> + return -EFAULT;
> +
> + tmp[len] = '\0';
> +
> + if (kstrtouint(tmp, 16, ®_addr))
> + return -EINVAL;
> +
> + intel_dp->debugfs_dpcd_addr = reg_addr;
> +
> + return len;
> +}
> +
> +static int i915_dpcd_addr_show(struct seq_file *m, void *data)
> +{
> + struct drm_connector *connector = m->private;
> + struct intel_dp *intel_dp =
> + enc_to_intel_dp(&intel_attached_encoder(connector)->base);
> +
> + seq_printf(m, "0x%x\n", intel_dp->debugfs_dpcd_addr);
> +
> + return 0;
> +}
> +
> +static int i915_dpcd_addr_open(struct inode *inode, struct file *file)
> +{
> + return single_open(file, i915_dpcd_addr_show, inode->i_private);
> +}
> +
> +static const struct file_operations i915_dpcd_addr_fops = {
> + .owner = THIS_MODULE,
> + .open = i915_dpcd_addr_open,
> + .read = seq_read,
> + .write = i915_dpcd_addr_write,
> + .llseek = seq_lseek,
> + .release = single_release,
> +};
> +
> +static int i915_dpcd_val_show(struct seq_file *m, void *data)
> +{
> + struct drm_connector *connector = m->private;
> + struct intel_dp *intel_dp =
> + enc_to_intel_dp(&intel_attached_encoder(connector)->base);
> + u8 val;
> + int ret;
> +
> + ret = drm_dp_dpcd_readb(&intel_dp->aux,
> + intel_dp->debugfs_dpcd_addr, &val);
> + if (ret < 0)
> + return ret;
> +
> + seq_printf(m, "0x%x\n", val);
> + return 0;
> +}
> +
> +static int i915_dpcd_val_open(struct inode *inode, struct file *file)
> +{
> + return single_open(file, i915_dpcd_val_show, inode->i_private);
> +}
> +
> +static const struct file_operations i915_dpcd_val_fops = {
> + .owner = THIS_MODULE,
> + .open = i915_dpcd_val_open,
> + .read = seq_read,
> + .llseek = seq_lseek,
> + .release = single_release,
> +};
> +
> /**
> * i915_debugfs_connector_add - add i915 specific connector debugfs files
> * @connector: pointer to a registered drm_connector
> @@ -4883,9 +4964,14 @@ int i915_debugfs_connector_add(struct drm_connector *connector)
> return -ENODEV;
>
> if (connector->connector_type == DRM_MODE_CONNECTOR_DisplayPort ||
> - connector->connector_type == DRM_MODE_CONNECTOR_eDP)
> + connector->connector_type == DRM_MODE_CONNECTOR_eDP) {
> debugfs_create_file("i915_dpcd", S_IRUGO, root, connector,
> &i915_dpcd_fops);
> + debugfs_create_file("i915_dpcd_addr", S_IRUGO | S_IWUSR,
> + root, connector, &i915_dpcd_addr_fops);
> + debugfs_create_file("i915_dpcd_val", S_IRUGO, root,
> + connector, &i915_dpcd_val_fops);
> + }
>
> return 0;
> }
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 4799b11..0594baa 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -642,6 +642,7 @@ struct intel_dp {
> unsigned long last_power_cycle;
> unsigned long last_power_on;
> unsigned long last_backlight_off;
> + unsigned int debugfs_dpcd_addr;
>
> struct notifier_block edp_notifier;
>
> --
> 1.9.1
>
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] drm/i915: Add debugfs to read any DPCD register
2015-04-16 14:07 ` Jani Nikula
@ 2015-04-16 14:32 ` Ville Syrjälä
2015-04-17 6:33 ` R, Durgadoss
0 siblings, 1 reply; 7+ messages in thread
From: Ville Syrjälä @ 2015-04-16 14:32 UTC (permalink / raw)
To: Jani Nikula; +Cc: Syrjala, Ville, intel-gfx, Zanoni, Paulo R
On Thu, Apr 16, 2015 at 05:07:00PM +0300, Jani Nikula wrote:
> On Wed, 08 Apr 2015, Durgadoss R <durgadoss.r@intel.com> wrote:
> > This patch creates a connector specific debugfs
> > interface to read any particular DPCD register.
> > The DPCD register address (hex format) is written
> > to 'i915_dpcd_addr' interface and the corresponding
> > value can be read from 'i915_dpcd_val' interface.
> >
> > Example usage:
> > echo 0x70 > i915_dpcd_addr
> > cat i915_dpcd_val
> > 0x1
> >
> > v2: Address Jani's comments.
>
> So, code-wise this looks good to me.
>
> But like I said in my review of v1, I am undecided on whether we want to
> have this interface or not. I could really be persuaded either way.
>
> Perhaps Ville or Paulo have an opinion?
I think having the ability to read (and also write actually) DPCD would
be nice.
So I like the idea, but it does seem a bit limited since it doesn't
allow writes. So perhaps we should just expose the entire DPCD as a
binary file and include a tool in i-g-t to read/write individual
pieces?
We could also have a tool to parse the entire thing like we have
for the vbt stuff.
>
> BR,
> Jani.
>
>
> >
> > Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> > ---
> > drivers/gpu/drm/i915/i915_debugfs.c | 88 ++++++++++++++++++++++++++++++++++++-
> > drivers/gpu/drm/i915/intel_drv.h | 1 +
> > 2 files changed, 88 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > index 10ca511..f1f365e 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -4865,6 +4865,87 @@ static const struct file_operations i915_dpcd_fops = {
> > .release = single_release,
> > };
> >
> > +static ssize_t i915_dpcd_addr_write(struct file *file, const char __user *ubuf,
> > + size_t len, loff_t *offp)
> > +{
> > + struct seq_file *m = file->private_data;
> > + struct drm_connector *connector = m->private;
> > + struct intel_dp *intel_dp =
> > + enc_to_intel_dp(&intel_attached_encoder(connector)->base);
> > + unsigned int reg_addr;
> > + char tmp[16];
> > +
> > + if (len >= sizeof(tmp))
> > + return -EINVAL;
> > +
> > + if (copy_from_user(tmp, ubuf, len))
> > + return -EFAULT;
> > +
> > + tmp[len] = '\0';
> > +
> > + if (kstrtouint(tmp, 16, ®_addr))
> > + return -EINVAL;
> > +
> > + intel_dp->debugfs_dpcd_addr = reg_addr;
> > +
> > + return len;
> > +}
> > +
> > +static int i915_dpcd_addr_show(struct seq_file *m, void *data)
> > +{
> > + struct drm_connector *connector = m->private;
> > + struct intel_dp *intel_dp =
> > + enc_to_intel_dp(&intel_attached_encoder(connector)->base);
> > +
> > + seq_printf(m, "0x%x\n", intel_dp->debugfs_dpcd_addr);
> > +
> > + return 0;
> > +}
> > +
> > +static int i915_dpcd_addr_open(struct inode *inode, struct file *file)
> > +{
> > + return single_open(file, i915_dpcd_addr_show, inode->i_private);
> > +}
> > +
> > +static const struct file_operations i915_dpcd_addr_fops = {
> > + .owner = THIS_MODULE,
> > + .open = i915_dpcd_addr_open,
> > + .read = seq_read,
> > + .write = i915_dpcd_addr_write,
> > + .llseek = seq_lseek,
> > + .release = single_release,
> > +};
> > +
> > +static int i915_dpcd_val_show(struct seq_file *m, void *data)
> > +{
> > + struct drm_connector *connector = m->private;
> > + struct intel_dp *intel_dp =
> > + enc_to_intel_dp(&intel_attached_encoder(connector)->base);
> > + u8 val;
> > + int ret;
> > +
> > + ret = drm_dp_dpcd_readb(&intel_dp->aux,
> > + intel_dp->debugfs_dpcd_addr, &val);
> > + if (ret < 0)
> > + return ret;
> > +
> > + seq_printf(m, "0x%x\n", val);
> > + return 0;
> > +}
> > +
> > +static int i915_dpcd_val_open(struct inode *inode, struct file *file)
> > +{
> > + return single_open(file, i915_dpcd_val_show, inode->i_private);
> > +}
> > +
> > +static const struct file_operations i915_dpcd_val_fops = {
> > + .owner = THIS_MODULE,
> > + .open = i915_dpcd_val_open,
> > + .read = seq_read,
> > + .llseek = seq_lseek,
> > + .release = single_release,
> > +};
> > +
> > /**
> > * i915_debugfs_connector_add - add i915 specific connector debugfs files
> > * @connector: pointer to a registered drm_connector
> > @@ -4883,9 +4964,14 @@ int i915_debugfs_connector_add(struct drm_connector *connector)
> > return -ENODEV;
> >
> > if (connector->connector_type == DRM_MODE_CONNECTOR_DisplayPort ||
> > - connector->connector_type == DRM_MODE_CONNECTOR_eDP)
> > + connector->connector_type == DRM_MODE_CONNECTOR_eDP) {
> > debugfs_create_file("i915_dpcd", S_IRUGO, root, connector,
> > &i915_dpcd_fops);
> > + debugfs_create_file("i915_dpcd_addr", S_IRUGO | S_IWUSR,
> > + root, connector, &i915_dpcd_addr_fops);
> > + debugfs_create_file("i915_dpcd_val", S_IRUGO, root,
> > + connector, &i915_dpcd_val_fops);
> > + }
> >
> > return 0;
> > }
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 4799b11..0594baa 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -642,6 +642,7 @@ struct intel_dp {
> > unsigned long last_power_cycle;
> > unsigned long last_power_on;
> > unsigned long last_backlight_off;
> > + unsigned int debugfs_dpcd_addr;
> >
> > struct notifier_block edp_notifier;
> >
> > --
> > 1.9.1
> >
>
> --
> Jani Nikula, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] drm/i915: Add debugfs to read any DPCD register
2015-04-16 14:32 ` Ville Syrjälä
@ 2015-04-17 6:33 ` R, Durgadoss
2015-04-17 10:00 ` Ville Syrjälä
0 siblings, 1 reply; 7+ messages in thread
From: R, Durgadoss @ 2015-04-17 6:33 UTC (permalink / raw)
To: Ville Syrjälä, Jani Nikula
Cc: intel-gfx@lists.freedesktop.org, Syrjala, Ville, Zanoni, Paulo R
>-----Original Message-----
>From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
>Sent: Thursday, April 16, 2015 8:03 PM
>To: Jani Nikula
>Cc: R, Durgadoss; intel-gfx@lists.freedesktop.org; Syrjala, Ville; Zanoni, Paulo R
>Subject: Re: [Intel-gfx] [PATCH v2] drm/i915: Add debugfs to read any DPCD register
>
>On Thu, Apr 16, 2015 at 05:07:00PM +0300, Jani Nikula wrote:
>> On Wed, 08 Apr 2015, Durgadoss R <durgadoss.r@intel.com> wrote:
>> > This patch creates a connector specific debugfs
>> > interface to read any particular DPCD register.
>> > The DPCD register address (hex format) is written
>> > to 'i915_dpcd_addr' interface and the corresponding
>> > value can be read from 'i915_dpcd_val' interface.
>> >
>> > Example usage:
>> > echo 0x70 > i915_dpcd_addr
>> > cat i915_dpcd_val
>> > 0x1
>> >
>> > v2: Address Jani's comments.
>>
>> So, code-wise this looks good to me.
>>
>> But like I said in my review of v1, I am undecided on whether we want to
>> have this interface or not. I could really be persuaded either way.
>>
>> Perhaps Ville or Paulo have an opinion?
>
>I think having the ability to read (and also write actually) DPCD would
>be nice.
>
>So I like the idea, but it does seem a bit limited since it doesn't
>allow writes. So perhaps we should just expose the entire DPCD as a
>binary file and include a tool in i-g-t to read/write individual
>pieces?
>
>We could also have a tool to parse the entire thing like we have
>for the vbt stuff.
Yes, This seems to be a good idea. I will start working on this.
Although I am not sure on the 'write' part.. I guess we can discuss
as we go along..
But I still think, a pointed read/write would also be useful
at times for debug. So, If you agree, I can extend this interface
to take values for write as well and submit a v3.
Thanks,
Durga
>
>>
>> BR,
>> Jani.
>>
>>
>> >
>> > Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
>> > ---
>> > drivers/gpu/drm/i915/i915_debugfs.c | 88 ++++++++++++++++++++++++++++++++++++-
>> > drivers/gpu/drm/i915/intel_drv.h | 1 +
>> > 2 files changed, 88 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
>> > index 10ca511..f1f365e 100644
>> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> > @@ -4865,6 +4865,87 @@ static const struct file_operations i915_dpcd_fops = {
>> > .release = single_release,
>> > };
>> >
>> > +static ssize_t i915_dpcd_addr_write(struct file *file, const char __user *ubuf,
>> > + size_t len, loff_t *offp)
>> > +{
>> > + struct seq_file *m = file->private_data;
>> > + struct drm_connector *connector = m->private;
>> > + struct intel_dp *intel_dp =
>> > + enc_to_intel_dp(&intel_attached_encoder(connector)->base);
>> > + unsigned int reg_addr;
>> > + char tmp[16];
>> > +
>> > + if (len >= sizeof(tmp))
>> > + return -EINVAL;
>> > +
>> > + if (copy_from_user(tmp, ubuf, len))
>> > + return -EFAULT;
>> > +
>> > + tmp[len] = '\0';
>> > +
>> > + if (kstrtouint(tmp, 16, ®_addr))
>> > + return -EINVAL;
>> > +
>> > + intel_dp->debugfs_dpcd_addr = reg_addr;
>> > +
>> > + return len;
>> > +}
>> > +
>> > +static int i915_dpcd_addr_show(struct seq_file *m, void *data)
>> > +{
>> > + struct drm_connector *connector = m->private;
>> > + struct intel_dp *intel_dp =
>> > + enc_to_intel_dp(&intel_attached_encoder(connector)->base);
>> > +
>> > + seq_printf(m, "0x%x\n", intel_dp->debugfs_dpcd_addr);
>> > +
>> > + return 0;
>> > +}
>> > +
>> > +static int i915_dpcd_addr_open(struct inode *inode, struct file *file)
>> > +{
>> > + return single_open(file, i915_dpcd_addr_show, inode->i_private);
>> > +}
>> > +
>> > +static const struct file_operations i915_dpcd_addr_fops = {
>> > + .owner = THIS_MODULE,
>> > + .open = i915_dpcd_addr_open,
>> > + .read = seq_read,
>> > + .write = i915_dpcd_addr_write,
>> > + .llseek = seq_lseek,
>> > + .release = single_release,
>> > +};
>> > +
>> > +static int i915_dpcd_val_show(struct seq_file *m, void *data)
>> > +{
>> > + struct drm_connector *connector = m->private;
>> > + struct intel_dp *intel_dp =
>> > + enc_to_intel_dp(&intel_attached_encoder(connector)->base);
>> > + u8 val;
>> > + int ret;
>> > +
>> > + ret = drm_dp_dpcd_readb(&intel_dp->aux,
>> > + intel_dp->debugfs_dpcd_addr, &val);
>> > + if (ret < 0)
>> > + return ret;
>> > +
>> > + seq_printf(m, "0x%x\n", val);
>> > + return 0;
>> > +}
>> > +
>> > +static int i915_dpcd_val_open(struct inode *inode, struct file *file)
>> > +{
>> > + return single_open(file, i915_dpcd_val_show, inode->i_private);
>> > +}
>> > +
>> > +static const struct file_operations i915_dpcd_val_fops = {
>> > + .owner = THIS_MODULE,
>> > + .open = i915_dpcd_val_open,
>> > + .read = seq_read,
>> > + .llseek = seq_lseek,
>> > + .release = single_release,
>> > +};
>> > +
>> > /**
>> > * i915_debugfs_connector_add - add i915 specific connector debugfs files
>> > * @connector: pointer to a registered drm_connector
>> > @@ -4883,9 +4964,14 @@ int i915_debugfs_connector_add(struct drm_connector *connector)
>> > return -ENODEV;
>> >
>> > if (connector->connector_type == DRM_MODE_CONNECTOR_DisplayPort ||
>> > - connector->connector_type == DRM_MODE_CONNECTOR_eDP)
>> > + connector->connector_type == DRM_MODE_CONNECTOR_eDP) {
>> > debugfs_create_file("i915_dpcd", S_IRUGO, root, connector,
>> > &i915_dpcd_fops);
>> > + debugfs_create_file("i915_dpcd_addr", S_IRUGO | S_IWUSR,
>> > + root, connector, &i915_dpcd_addr_fops);
>> > + debugfs_create_file("i915_dpcd_val", S_IRUGO, root,
>> > + connector, &i915_dpcd_val_fops);
>> > + }
>> >
>> > return 0;
>> > }
>> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> > index 4799b11..0594baa 100644
>> > --- a/drivers/gpu/drm/i915/intel_drv.h
>> > +++ b/drivers/gpu/drm/i915/intel_drv.h
>> > @@ -642,6 +642,7 @@ struct intel_dp {
>> > unsigned long last_power_cycle;
>> > unsigned long last_power_on;
>> > unsigned long last_backlight_off;
>> > + unsigned int debugfs_dpcd_addr;
>> >
>> > struct notifier_block edp_notifier;
>> >
>> > --
>> > 1.9.1
>> >
>>
>> --
>> Jani Nikula, Intel Open Source Technology Center
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>--
>Ville Syrjälä
>Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] drm/i915: Add debugfs to read any DPCD register
2015-04-17 6:33 ` R, Durgadoss
@ 2015-04-17 10:00 ` Ville Syrjälä
2015-04-20 11:29 ` R, Durgadoss
2015-04-20 15:44 ` Daniel Vetter
0 siblings, 2 replies; 7+ messages in thread
From: Ville Syrjälä @ 2015-04-17 10:00 UTC (permalink / raw)
To: R, Durgadoss
Cc: intel-gfx@lists.freedesktop.org, Syrjala, Ville, Zanoni, Paulo R
On Fri, Apr 17, 2015 at 06:33:57AM +0000, R, Durgadoss wrote:
> >-----Original Message-----
> >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> >Sent: Thursday, April 16, 2015 8:03 PM
> >To: Jani Nikula
> >Cc: R, Durgadoss; intel-gfx@lists.freedesktop.org; Syrjala, Ville; Zanoni, Paulo R
> >Subject: Re: [Intel-gfx] [PATCH v2] drm/i915: Add debugfs to read any DPCD register
> >
> >On Thu, Apr 16, 2015 at 05:07:00PM +0300, Jani Nikula wrote:
> >> On Wed, 08 Apr 2015, Durgadoss R <durgadoss.r@intel.com> wrote:
> >> > This patch creates a connector specific debugfs
> >> > interface to read any particular DPCD register.
> >> > The DPCD register address (hex format) is written
> >> > to 'i915_dpcd_addr' interface and the corresponding
> >> > value can be read from 'i915_dpcd_val' interface.
> >> >
> >> > Example usage:
> >> > echo 0x70 > i915_dpcd_addr
> >> > cat i915_dpcd_val
> >> > 0x1
> >> >
> >> > v2: Address Jani's comments.
> >>
> >> So, code-wise this looks good to me.
> >>
> >> But like I said in my review of v1, I am undecided on whether we want to
> >> have this interface or not. I could really be persuaded either way.
> >>
> >> Perhaps Ville or Paulo have an opinion?
> >
> >I think having the ability to read (and also write actually) DPCD would
> >be nice.
> >
> >So I like the idea, but it does seem a bit limited since it doesn't
> >allow writes. So perhaps we should just expose the entire DPCD as a
> >binary file and include a tool in i-g-t to read/write individual
> >pieces?
> >
> >We could also have a tool to parse the entire thing like we have
> >for the vbt stuff.
>
> Yes, This seems to be a good idea. I will start working on this.
> Although I am not sure on the 'write' part.. I guess we can discuss
> as we go along..
>
> But I still think, a pointed read/write would also be useful
> at times for debug. So, If you agree, I can extend this interface
> to take values for write as well and submit a v3.
For me at least i-g-t is always around so if a small tool is needed
to make the read/write a single register case easy, I'm fine with
that. We anyway need tools to read/write everything else, so I don't
see a big difference here. But if other people would prefer something
that doesn't need a specific tool I won't object to it either.
>
> Thanks,
> Durga
>
> >
> >>
> >> BR,
> >> Jani.
> >>
> >>
> >> >
> >> > Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
> >> > ---
> >> > drivers/gpu/drm/i915/i915_debugfs.c | 88 ++++++++++++++++++++++++++++++++++++-
> >> > drivers/gpu/drm/i915/intel_drv.h | 1 +
> >> > 2 files changed, 88 insertions(+), 1 deletion(-)
> >> >
> >> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> >> > index 10ca511..f1f365e 100644
> >> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> >> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> >> > @@ -4865,6 +4865,87 @@ static const struct file_operations i915_dpcd_fops = {
> >> > .release = single_release,
> >> > };
> >> >
> >> > +static ssize_t i915_dpcd_addr_write(struct file *file, const char __user *ubuf,
> >> > + size_t len, loff_t *offp)
> >> > +{
> >> > + struct seq_file *m = file->private_data;
> >> > + struct drm_connector *connector = m->private;
> >> > + struct intel_dp *intel_dp =
> >> > + enc_to_intel_dp(&intel_attached_encoder(connector)->base);
> >> > + unsigned int reg_addr;
> >> > + char tmp[16];
> >> > +
> >> > + if (len >= sizeof(tmp))
> >> > + return -EINVAL;
> >> > +
> >> > + if (copy_from_user(tmp, ubuf, len))
> >> > + return -EFAULT;
> >> > +
> >> > + tmp[len] = '\0';
> >> > +
> >> > + if (kstrtouint(tmp, 16, ®_addr))
> >> > + return -EINVAL;
> >> > +
> >> > + intel_dp->debugfs_dpcd_addr = reg_addr;
> >> > +
> >> > + return len;
> >> > +}
> >> > +
> >> > +static int i915_dpcd_addr_show(struct seq_file *m, void *data)
> >> > +{
> >> > + struct drm_connector *connector = m->private;
> >> > + struct intel_dp *intel_dp =
> >> > + enc_to_intel_dp(&intel_attached_encoder(connector)->base);
> >> > +
> >> > + seq_printf(m, "0x%x\n", intel_dp->debugfs_dpcd_addr);
> >> > +
> >> > + return 0;
> >> > +}
> >> > +
> >> > +static int i915_dpcd_addr_open(struct inode *inode, struct file *file)
> >> > +{
> >> > + return single_open(file, i915_dpcd_addr_show, inode->i_private);
> >> > +}
> >> > +
> >> > +static const struct file_operations i915_dpcd_addr_fops = {
> >> > + .owner = THIS_MODULE,
> >> > + .open = i915_dpcd_addr_open,
> >> > + .read = seq_read,
> >> > + .write = i915_dpcd_addr_write,
> >> > + .llseek = seq_lseek,
> >> > + .release = single_release,
> >> > +};
> >> > +
> >> > +static int i915_dpcd_val_show(struct seq_file *m, void *data)
> >> > +{
> >> > + struct drm_connector *connector = m->private;
> >> > + struct intel_dp *intel_dp =
> >> > + enc_to_intel_dp(&intel_attached_encoder(connector)->base);
> >> > + u8 val;
> >> > + int ret;
> >> > +
> >> > + ret = drm_dp_dpcd_readb(&intel_dp->aux,
> >> > + intel_dp->debugfs_dpcd_addr, &val);
> >> > + if (ret < 0)
> >> > + return ret;
> >> > +
> >> > + seq_printf(m, "0x%x\n", val);
> >> > + return 0;
> >> > +}
> >> > +
> >> > +static int i915_dpcd_val_open(struct inode *inode, struct file *file)
> >> > +{
> >> > + return single_open(file, i915_dpcd_val_show, inode->i_private);
> >> > +}
> >> > +
> >> > +static const struct file_operations i915_dpcd_val_fops = {
> >> > + .owner = THIS_MODULE,
> >> > + .open = i915_dpcd_val_open,
> >> > + .read = seq_read,
> >> > + .llseek = seq_lseek,
> >> > + .release = single_release,
> >> > +};
> >> > +
> >> > /**
> >> > * i915_debugfs_connector_add - add i915 specific connector debugfs files
> >> > * @connector: pointer to a registered drm_connector
> >> > @@ -4883,9 +4964,14 @@ int i915_debugfs_connector_add(struct drm_connector *connector)
> >> > return -ENODEV;
> >> >
> >> > if (connector->connector_type == DRM_MODE_CONNECTOR_DisplayPort ||
> >> > - connector->connector_type == DRM_MODE_CONNECTOR_eDP)
> >> > + connector->connector_type == DRM_MODE_CONNECTOR_eDP) {
> >> > debugfs_create_file("i915_dpcd", S_IRUGO, root, connector,
> >> > &i915_dpcd_fops);
> >> > + debugfs_create_file("i915_dpcd_addr", S_IRUGO | S_IWUSR,
> >> > + root, connector, &i915_dpcd_addr_fops);
> >> > + debugfs_create_file("i915_dpcd_val", S_IRUGO, root,
> >> > + connector, &i915_dpcd_val_fops);
> >> > + }
> >> >
> >> > return 0;
> >> > }
> >> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> >> > index 4799b11..0594baa 100644
> >> > --- a/drivers/gpu/drm/i915/intel_drv.h
> >> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> >> > @@ -642,6 +642,7 @@ struct intel_dp {
> >> > unsigned long last_power_cycle;
> >> > unsigned long last_power_on;
> >> > unsigned long last_backlight_off;
> >> > + unsigned int debugfs_dpcd_addr;
> >> >
> >> > struct notifier_block edp_notifier;
> >> >
> >> > --
> >> > 1.9.1
> >> >
> >>
> >> --
> >> Jani Nikula, Intel Open Source Technology Center
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> Intel-gfx@lists.freedesktop.org
> >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> >--
> >Ville Syrjälä
> >Intel OTC
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] drm/i915: Add debugfs to read any DPCD register
2015-04-17 10:00 ` Ville Syrjälä
@ 2015-04-20 11:29 ` R, Durgadoss
2015-04-20 15:44 ` Daniel Vetter
1 sibling, 0 replies; 7+ messages in thread
From: R, Durgadoss @ 2015-04-20 11:29 UTC (permalink / raw)
To: Ville Syrjälä
Cc: intel-gfx@lists.freedesktop.org, Syrjala, Ville, Zanoni, Paulo R
>-----Original Message-----
>From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
>Sent: Friday, April 17, 2015 3:30 PM
>To: R, Durgadoss
>Cc: Jani Nikula; intel-gfx@lists.freedesktop.org; Syrjala, Ville; Zanoni, Paulo R
>Subject: Re: [Intel-gfx] [PATCH v2] drm/i915: Add debugfs to read any DPCD register
>
>On Fri, Apr 17, 2015 at 06:33:57AM +0000, R, Durgadoss wrote:
>> >-----Original Message-----
>> >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
>> >Sent: Thursday, April 16, 2015 8:03 PM
>> >To: Jani Nikula
>> >Cc: R, Durgadoss; intel-gfx@lists.freedesktop.org; Syrjala, Ville; Zanoni, Paulo R
>> >Subject: Re: [Intel-gfx] [PATCH v2] drm/i915: Add debugfs to read any DPCD register
>> >
>> >On Thu, Apr 16, 2015 at 05:07:00PM +0300, Jani Nikula wrote:
>> >> On Wed, 08 Apr 2015, Durgadoss R <durgadoss.r@intel.com> wrote:
>> >> > This patch creates a connector specific debugfs
>> >> > interface to read any particular DPCD register.
>> >> > The DPCD register address (hex format) is written
>> >> > to 'i915_dpcd_addr' interface and the corresponding
>> >> > value can be read from 'i915_dpcd_val' interface.
>> >> >
>> >> > Example usage:
>> >> > echo 0x70 > i915_dpcd_addr
>> >> > cat i915_dpcd_val
>> >> > 0x1
>> >> >
>> >> > v2: Address Jani's comments.
>> >>
>> >> So, code-wise this looks good to me.
>> >>
>> >> But like I said in my review of v1, I am undecided on whether we want to
>> >> have this interface or not. I could really be persuaded either way.
>> >>
>> >> Perhaps Ville or Paulo have an opinion?
>> >
>> >I think having the ability to read (and also write actually) DPCD would
>> >be nice.
>> >
>> >So I like the idea, but it does seem a bit limited since it doesn't
>> >allow writes. So perhaps we should just expose the entire DPCD as a
>> >binary file and include a tool in i-g-t to read/write individual
>> >pieces?
Ok, I started with this, but realized that it becomes almost
equivalent of the 'i915_dpcd' interface introduced by Jani.
So, it looks like, extending that interface by adding more
relevant offsets seems to be a better option,
than creating one more interface for dumping.
Thanks,
Durga
>> >
>> >We could also have a tool to parse the entire thing like we have
>> >for the vbt stuff.
>>
>> Yes, This seems to be a good idea. I will start working on this.
>> Although I am not sure on the 'write' part.. I guess we can discuss
>> as we go along..
>>
>> But I still think, a pointed read/write would also be useful
>> at times for debug. So, If you agree, I can extend this interface
>> to take values for write as well and submit a v3.
>
>For me at least i-g-t is always around so if a small tool is needed
>to make the read/write a single register case easy, I'm fine with
>that. We anyway need tools to read/write everything else, so I don't
>see a big difference here. But if other people would prefer something
>that doesn't need a specific tool I won't object to it either.
>
>>
>> Thanks,
>> Durga
>>
>> >
>> >>
>> >> BR,
>> >> Jani.
>> >>
>> >>
>> >> >
>> >> > Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
>> >> > ---
>> >> > drivers/gpu/drm/i915/i915_debugfs.c | 88 ++++++++++++++++++++++++++++++++++++-
>> >> > drivers/gpu/drm/i915/intel_drv.h | 1 +
>> >> > 2 files changed, 88 insertions(+), 1 deletion(-)
>> >> >
>> >> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
>> >> > index 10ca511..f1f365e 100644
>> >> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> >> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> >> > @@ -4865,6 +4865,87 @@ static const struct file_operations i915_dpcd_fops = {
>> >> > .release = single_release,
>> >> > };
>> >> >
>> >> > +static ssize_t i915_dpcd_addr_write(struct file *file, const char __user *ubuf,
>> >> > + size_t len, loff_t *offp)
>> >> > +{
>> >> > + struct seq_file *m = file->private_data;
>> >> > + struct drm_connector *connector = m->private;
>> >> > + struct intel_dp *intel_dp =
>> >> > + enc_to_intel_dp(&intel_attached_encoder(connector)->base);
>> >> > + unsigned int reg_addr;
>> >> > + char tmp[16];
>> >> > +
>> >> > + if (len >= sizeof(tmp))
>> >> > + return -EINVAL;
>> >> > +
>> >> > + if (copy_from_user(tmp, ubuf, len))
>> >> > + return -EFAULT;
>> >> > +
>> >> > + tmp[len] = '\0';
>> >> > +
>> >> > + if (kstrtouint(tmp, 16, ®_addr))
>> >> > + return -EINVAL;
>> >> > +
>> >> > + intel_dp->debugfs_dpcd_addr = reg_addr;
>> >> > +
>> >> > + return len;
>> >> > +}
>> >> > +
>> >> > +static int i915_dpcd_addr_show(struct seq_file *m, void *data)
>> >> > +{
>> >> > + struct drm_connector *connector = m->private;
>> >> > + struct intel_dp *intel_dp =
>> >> > + enc_to_intel_dp(&intel_attached_encoder(connector)->base);
>> >> > +
>> >> > + seq_printf(m, "0x%x\n", intel_dp->debugfs_dpcd_addr);
>> >> > +
>> >> > + return 0;
>> >> > +}
>> >> > +
>> >> > +static int i915_dpcd_addr_open(struct inode *inode, struct file *file)
>> >> > +{
>> >> > + return single_open(file, i915_dpcd_addr_show, inode->i_private);
>> >> > +}
>> >> > +
>> >> > +static const struct file_operations i915_dpcd_addr_fops = {
>> >> > + .owner = THIS_MODULE,
>> >> > + .open = i915_dpcd_addr_open,
>> >> > + .read = seq_read,
>> >> > + .write = i915_dpcd_addr_write,
>> >> > + .llseek = seq_lseek,
>> >> > + .release = single_release,
>> >> > +};
>> >> > +
>> >> > +static int i915_dpcd_val_show(struct seq_file *m, void *data)
>> >> > +{
>> >> > + struct drm_connector *connector = m->private;
>> >> > + struct intel_dp *intel_dp =
>> >> > + enc_to_intel_dp(&intel_attached_encoder(connector)->base);
>> >> > + u8 val;
>> >> > + int ret;
>> >> > +
>> >> > + ret = drm_dp_dpcd_readb(&intel_dp->aux,
>> >> > + intel_dp->debugfs_dpcd_addr, &val);
>> >> > + if (ret < 0)
>> >> > + return ret;
>> >> > +
>> >> > + seq_printf(m, "0x%x\n", val);
>> >> > + return 0;
>> >> > +}
>> >> > +
>> >> > +static int i915_dpcd_val_open(struct inode *inode, struct file *file)
>> >> > +{
>> >> > + return single_open(file, i915_dpcd_val_show, inode->i_private);
>> >> > +}
>> >> > +
>> >> > +static const struct file_operations i915_dpcd_val_fops = {
>> >> > + .owner = THIS_MODULE,
>> >> > + .open = i915_dpcd_val_open,
>> >> > + .read = seq_read,
>> >> > + .llseek = seq_lseek,
>> >> > + .release = single_release,
>> >> > +};
>> >> > +
>> >> > /**
>> >> > * i915_debugfs_connector_add - add i915 specific connector debugfs files
>> >> > * @connector: pointer to a registered drm_connector
>> >> > @@ -4883,9 +4964,14 @@ int i915_debugfs_connector_add(struct drm_connector *connector)
>> >> > return -ENODEV;
>> >> >
>> >> > if (connector->connector_type == DRM_MODE_CONNECTOR_DisplayPort ||
>> >> > - connector->connector_type == DRM_MODE_CONNECTOR_eDP)
>> >> > + connector->connector_type == DRM_MODE_CONNECTOR_eDP) {
>> >> > debugfs_create_file("i915_dpcd", S_IRUGO, root, connector,
>> >> > &i915_dpcd_fops);
>> >> > + debugfs_create_file("i915_dpcd_addr", S_IRUGO | S_IWUSR,
>> >> > + root, connector, &i915_dpcd_addr_fops);
>> >> > + debugfs_create_file("i915_dpcd_val", S_IRUGO, root,
>> >> > + connector, &i915_dpcd_val_fops);
>> >> > + }
>> >> >
>> >> > return 0;
>> >> > }
>> >> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> >> > index 4799b11..0594baa 100644
>> >> > --- a/drivers/gpu/drm/i915/intel_drv.h
>> >> > +++ b/drivers/gpu/drm/i915/intel_drv.h
>> >> > @@ -642,6 +642,7 @@ struct intel_dp {
>> >> > unsigned long last_power_cycle;
>> >> > unsigned long last_power_on;
>> >> > unsigned long last_backlight_off;
>> >> > + unsigned int debugfs_dpcd_addr;
>> >> >
>> >> > struct notifier_block edp_notifier;
>> >> >
>> >> > --
>> >> > 1.9.1
>> >> >
>> >>
>> >> --
>> >> Jani Nikula, Intel Open Source Technology Center
>> >> _______________________________________________
>> >> Intel-gfx mailing list
>> >> Intel-gfx@lists.freedesktop.org
>> >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> >
>> >--
>> >Ville Syrjälä
>> >Intel OTC
>
>--
>Ville Syrjälä
>Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] drm/i915: Add debugfs to read any DPCD register
2015-04-17 10:00 ` Ville Syrjälä
2015-04-20 11:29 ` R, Durgadoss
@ 2015-04-20 15:44 ` Daniel Vetter
1 sibling, 0 replies; 7+ messages in thread
From: Daniel Vetter @ 2015-04-20 15:44 UTC (permalink / raw)
To: Ville Syrjälä
Cc: Syrjala, Ville, intel-gfx@lists.freedesktop.org, Zanoni, Paulo R
On Fri, Apr 17, 2015 at 01:00:28PM +0300, Ville Syrjälä wrote:
> On Fri, Apr 17, 2015 at 06:33:57AM +0000, R, Durgadoss wrote:
> > >-----Original Message-----
> > >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> > >Sent: Thursday, April 16, 2015 8:03 PM
> > >To: Jani Nikula
> > >Cc: R, Durgadoss; intel-gfx@lists.freedesktop.org; Syrjala, Ville; Zanoni, Paulo R
> > >Subject: Re: [Intel-gfx] [PATCH v2] drm/i915: Add debugfs to read any DPCD register
> > >
> > >On Thu, Apr 16, 2015 at 05:07:00PM +0300, Jani Nikula wrote:
> > >> On Wed, 08 Apr 2015, Durgadoss R <durgadoss.r@intel.com> wrote:
> > >> > This patch creates a connector specific debugfs
> > >> > interface to read any particular DPCD register.
> > >> > The DPCD register address (hex format) is written
> > >> > to 'i915_dpcd_addr' interface and the corresponding
> > >> > value can be read from 'i915_dpcd_val' interface.
> > >> >
> > >> > Example usage:
> > >> > echo 0x70 > i915_dpcd_addr
> > >> > cat i915_dpcd_val
> > >> > 0x1
> > >> >
> > >> > v2: Address Jani's comments.
> > >>
> > >> So, code-wise this looks good to me.
> > >>
> > >> But like I said in my review of v1, I am undecided on whether we want to
> > >> have this interface or not. I could really be persuaded either way.
> > >>
> > >> Perhaps Ville or Paulo have an opinion?
> > >
> > >I think having the ability to read (and also write actually) DPCD would
> > >be nice.
> > >
> > >So I like the idea, but it does seem a bit limited since it doesn't
> > >allow writes. So perhaps we should just expose the entire DPCD as a
> > >binary file and include a tool in i-g-t to read/write individual
> > >pieces?
> > >
> > >We could also have a tool to parse the entire thing like we have
> > >for the vbt stuff.
> >
> > Yes, This seems to be a good idea. I will start working on this.
> > Although I am not sure on the 'write' part.. I guess we can discuss
> > as we go along..
> >
> > But I still think, a pointed read/write would also be useful
> > at times for debug. So, If you agree, I can extend this interface
> > to take values for write as well and submit a v3.
>
> For me at least i-g-t is always around so if a small tool is needed
> to make the read/write a single register case easy, I'm fine with
> that. We anyway need tools to read/write everything else, so I don't
> see a big difference here. But if other people would prefer something
> that doesn't need a specific tool I won't object to it either.
Imo adding a /dev node or debugfs files with ioctl to do generic dp aux
transactions and then add userspace tooling on top (similar to i2c) would
be the right approach. Adding all the possible dp aux registers
(especially once we look at mst) seems to be an unending task.
-Daniel
--
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2015-04-20 15:45 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-04-08 11:41 [PATCH v2] drm/i915: Add debugfs to read any DPCD register Durgadoss R
2015-04-16 14:07 ` Jani Nikula
2015-04-16 14:32 ` Ville Syrjälä
2015-04-17 6:33 ` R, Durgadoss
2015-04-17 10:00 ` Ville Syrjälä
2015-04-20 11:29 ` R, Durgadoss
2015-04-20 15:44 ` Daniel Vetter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox