>-----Original Message----- >From: Ville Syrjälä [mailto:ville.syrjala@xxxxxxxxxxxxxxx] >Sent: Friday, April 17, 2015 3:30 PM >To: R, Durgadoss >Cc: Jani Nikula; intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Syrjala, Ville; Zanoni, Paulo R >Subject: Re: [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@xxxxxxxxxxxxxxx] >> >Sent: Thursday, April 16, 2015 8:03 PM >> >To: Jani Nikula >> >Cc: R, Durgadoss; intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Syrjala, Ville; Zanoni, Paulo R >> >Subject: Re: [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@xxxxxxxxx> 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@xxxxxxxxx> >> >> > --- >> >> > 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@xxxxxxxxxxxxxxxxxxxxx >> >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx >> > >> >-- >> >Ville Syrjälä >> >Intel OTC > >-- >Ville Syrjälä >Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
