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? > > > >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
