| Message ID | 20200608100103.19472-3-ankit.k.nautiyal@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add debugfs for requesting HDCP version | expand |
On 2020-06-08 at 15:31:03 +0530, Ankit Nautiyal wrote: > As per the current HDCP design, the driver selects the highest > version of HDCP that can be used to satisfy the content-protection > requirements of the user. Due to this, the content-protection > tests cannot test a lower version of HDCP, if the platform and the > display panel, both support higher HDCP version. > > To provide some support for testing and debugging, a per-connector > debugfs is required to set the HDCP version via debugfs that the > kernel can consider, while enabling HDCP. > > This patch adds a new debugfs entry for each connector that supports > HDCP. For enforcing a particular HDCP version for a connector, the user > can write into the debugfs for that connector. IMHO this doesn't feel like a debugfs per connector, even if it is a global singleton resource for all connectors, i don't see any problem in that, may be a global debugfs would make sense here ? > > v2: As suggested by Jani Nikula: > -used kstrtouint_from_user() to directly read as uint from user buffer. > -used 32 bit flag instead of 64 bit for hdcp_ver flag. > -removed unnecessary prints and fixed other minor formatting issues. > > Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com> > --- > .../drm/i915/display/intel_display_debugfs.c | 68 +++++++++++++++++++ > 1 file changed, 68 insertions(+) > > diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c b/drivers/gpu/drm/i915/display/intel_display_debugfs.c > index 70525623bcdf..c01653d412e7 100644 > --- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c > +++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c > @@ -2185,6 +2185,72 @@ static const struct file_operations i915_dsc_fec_support_fops = { > .write = i915_dsc_fec_support_write > }; > > +static int i915_hdcp_ver_request_show(struct seq_file *m, void *data) > +{ > + struct drm_connector *connector = m->private; > + struct intel_connector *intel_connector = to_intel_connector(connector); > + u32 hdcp_ver_flag; > + > + if (connector->status != connector_status_connected) > + return -ENODEV; > + > + /* HDCP is supported by connector */ > + if (!intel_connector->hdcp.shim) > + return -EINVAL; > + > + hdcp_ver_flag = intel_connector->hdcp.debugfs_ver_request; > + seq_printf(m, "HDCP_VER_FLAGS: %u\n", hdcp_ver_flag); > + > + return 0; > +} > + > +static int i915_hdcp_ver_request_open(struct inode *inode, > + struct file *file) > +{ > + return single_open(file, i915_hdcp_ver_request_show, > + inode->i_private); > +} > + > +static ssize_t i915_hdcp_ver_request_write(struct file *file, > + const char __user *ubuf, > + size_t len, loff_t *offp) > +{ > + unsigned int hdcp_ver = 0; > + int ret; > + struct drm_connector *connector = > + ((struct seq_file *)file->private_data)->private; > + struct intel_connector *intel_connector = to_intel_connector(connector); > + struct intel_hdcp *hdcp = &intel_connector->hdcp; > + > + if (!hdcp->shim) > + return -EINVAL; > + > + if (len == 0) > + return 0; > + > + ret = kstrtouint_from_user(ubuf, len, 0, &hdcp_ver); > + if (ret < 0) > + return ret; > + > + if (hdcp_ver > HDCP_VERSION_MASK) > + return -EINVAL; > + > + hdcp->debugfs_ver_request = hdcp_ver; A lockless assignment, this would probably not scale. Could u please add some comment here for current IGT need this is ok, but for any concurrent usgaes proper locking is required. Thanks, Anshuman > + > + *offp += len; > + > + return len; > +} > + > +static const struct file_operations i915_hdcp_ver_request_fops = { > + .owner = THIS_MODULE, > + .open = i915_hdcp_ver_request_open, > + .read = seq_read, > + .llseek = seq_lseek, > + .release = single_release, > + .write = i915_hdcp_ver_request_write > +}; > + > /** > * intel_connector_debugfs_add - add i915 specific connector debugfs files > * @connector: pointer to a registered drm_connector > @@ -2215,6 +2281,8 @@ int intel_connector_debugfs_add(struct drm_connector *connector) > connector->connector_type == DRM_MODE_CONNECTOR_HDMIB) { > debugfs_create_file("i915_hdcp_sink_capability", S_IRUGO, root, > connector, &i915_hdcp_sink_capability_fops); > + debugfs_create_file("i915_hdcp_version_request", 0444, root, > + connector, &i915_hdcp_ver_request_fops); > } > > if (INTEL_GEN(dev_priv) >= 10 && > -- > 2.17.1 >
Hi Anshuman, Thanks for the review comments and suggestions. Please find my response inline: On 6/15/2020 10:15 AM, Anshuman Gupta wrote: > On 2020-06-08 at 15:31:03 +0530, Ankit Nautiyal wrote: >> As per the current HDCP design, the driver selects the highest >> version of HDCP that can be used to satisfy the content-protection >> requirements of the user. Due to this, the content-protection >> tests cannot test a lower version of HDCP, if the platform and the >> display panel, both support higher HDCP version. >> >> To provide some support for testing and debugging, a per-connector >> debugfs is required to set the HDCP version via debugfs that the >> kernel can consider, while enabling HDCP. >> >> This patch adds a new debugfs entry for each connector that supports >> HDCP. For enforcing a particular HDCP version for a connector, the user >> can write into the debugfs for that connector. > IMHO this doesn't feel like a debugfs per connector, even if it is a > global singleton resource for all connectors, i don't see any problem in > that, may be a global debugfs would make sense here ? The current solution was inline with the comments in the IGT patch, where the approach for a connector level debugfs was discussed by the community members. But I agree we don't necessarily require per connector debugfs for our use-case here. We can use a global resource for storing the request for HDCP version and use it if it is set, while enabling HDCP. Perhaps will store it in dev_priv, instead of connector->hdcp. I will try that out and send next version. >> v2: As suggested by Jani Nikula: >> -used kstrtouint_from_user() to directly read as uint from user buffer. >> -used 32 bit flag instead of 64 bit for hdcp_ver flag. >> -removed unnecessary prints and fixed other minor formatting issues. >> >> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com> >> --- >> .../drm/i915/display/intel_display_debugfs.c | 68 +++++++++++++++++++ >> 1 file changed, 68 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c b/drivers/gpu/drm/i915/display/intel_display_debugfs.c >> index 70525623bcdf..c01653d412e7 100644 >> --- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c >> +++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c >> @@ -2185,6 +2185,72 @@ static const struct file_operations i915_dsc_fec_support_fops = { >> .write = i915_dsc_fec_support_write >> }; >> >> +static int i915_hdcp_ver_request_show(struct seq_file *m, void *data) >> +{ >> + struct drm_connector *connector = m->private; >> + struct intel_connector *intel_connector = to_intel_connector(connector); >> + u32 hdcp_ver_flag; >> + >> + if (connector->status != connector_status_connected) >> + return -ENODEV; >> + >> + /* HDCP is supported by connector */ >> + if (!intel_connector->hdcp.shim) >> + return -EINVAL; >> + >> + hdcp_ver_flag = intel_connector->hdcp.debugfs_ver_request; >> + seq_printf(m, "HDCP_VER_FLAGS: %u\n", hdcp_ver_flag); >> + >> + return 0; >> +} >> + >> +static int i915_hdcp_ver_request_open(struct inode *inode, >> + struct file *file) >> +{ >> + return single_open(file, i915_hdcp_ver_request_show, >> + inode->i_private); >> +} >> + >> +static ssize_t i915_hdcp_ver_request_write(struct file *file, >> + const char __user *ubuf, >> + size_t len, loff_t *offp) >> +{ >> + unsigned int hdcp_ver = 0; >> + int ret; >> + struct drm_connector *connector = >> + ((struct seq_file *)file->private_data)->private; >> + struct intel_connector *intel_connector = to_intel_connector(connector); >> + struct intel_hdcp *hdcp = &intel_connector->hdcp; >> + >> + if (!hdcp->shim) >> + return -EINVAL; >> + >> + if (len == 0) >> + return 0; >> + >> + ret = kstrtouint_from_user(ubuf, len, 0, &hdcp_ver); >> + if (ret < 0) >> + return ret; >> + >> + if (hdcp_ver > HDCP_VERSION_MASK) >> + return -EINVAL; >> + >> + hdcp->debugfs_ver_request = hdcp_ver; > A lockless assignment, this would probably not scale. > Could u please add some comment here for current IGT need this is ok, > but for any concurrent usgaes proper locking is required. > Thanks, > Anshuman Agreed. I will add this as a note in comments in next version. Thanks & Regards, Ankit >> + >> + *offp += len; >> + >> + return len; >> +} >> + >> +static const struct file_operations i915_hdcp_ver_request_fops = { >> + .owner = THIS_MODULE, >> + .open = i915_hdcp_ver_request_open, >> + .read = seq_read, >> + .llseek = seq_lseek, >> + .release = single_release, >> + .write = i915_hdcp_ver_request_write >> +}; >> + >> /** >> * intel_connector_debugfs_add - add i915 specific connector debugfs files >> * @connector: pointer to a registered drm_connector >> @@ -2215,6 +2281,8 @@ int intel_connector_debugfs_add(struct drm_connector *connector) >> connector->connector_type == DRM_MODE_CONNECTOR_HDMIB) { >> debugfs_create_file("i915_hdcp_sink_capability", S_IRUGO, root, >> connector, &i915_hdcp_sink_capability_fops); >> + debugfs_create_file("i915_hdcp_version_request", 0444, root, >> + connector, &i915_hdcp_ver_request_fops); >> } >> >> if (INTEL_GEN(dev_priv) >= 10 && >> -- >> 2.17.1 >>
diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c b/drivers/gpu/drm/i915/display/intel_display_debugfs.c index 70525623bcdf..c01653d412e7 100644 --- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c +++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c @@ -2185,6 +2185,72 @@ static const struct file_operations i915_dsc_fec_support_fops = { .write = i915_dsc_fec_support_write }; +static int i915_hdcp_ver_request_show(struct seq_file *m, void *data) +{ + struct drm_connector *connector = m->private; + struct intel_connector *intel_connector = to_intel_connector(connector); + u32 hdcp_ver_flag; + + if (connector->status != connector_status_connected) + return -ENODEV; + + /* HDCP is supported by connector */ + if (!intel_connector->hdcp.shim) + return -EINVAL; + + hdcp_ver_flag = intel_connector->hdcp.debugfs_ver_request; + seq_printf(m, "HDCP_VER_FLAGS: %u\n", hdcp_ver_flag); + + return 0; +} + +static int i915_hdcp_ver_request_open(struct inode *inode, + struct file *file) +{ + return single_open(file, i915_hdcp_ver_request_show, + inode->i_private); +} + +static ssize_t i915_hdcp_ver_request_write(struct file *file, + const char __user *ubuf, + size_t len, loff_t *offp) +{ + unsigned int hdcp_ver = 0; + int ret; + struct drm_connector *connector = + ((struct seq_file *)file->private_data)->private; + struct intel_connector *intel_connector = to_intel_connector(connector); + struct intel_hdcp *hdcp = &intel_connector->hdcp; + + if (!hdcp->shim) + return -EINVAL; + + if (len == 0) + return 0; + + ret = kstrtouint_from_user(ubuf, len, 0, &hdcp_ver); + if (ret < 0) + return ret; + + if (hdcp_ver > HDCP_VERSION_MASK) + return -EINVAL; + + hdcp->debugfs_ver_request = hdcp_ver; + + *offp += len; + + return len; +} + +static const struct file_operations i915_hdcp_ver_request_fops = { + .owner = THIS_MODULE, + .open = i915_hdcp_ver_request_open, + .read = seq_read, + .llseek = seq_lseek, + .release = single_release, + .write = i915_hdcp_ver_request_write +}; + /** * intel_connector_debugfs_add - add i915 specific connector debugfs files * @connector: pointer to a registered drm_connector @@ -2215,6 +2281,8 @@ int intel_connector_debugfs_add(struct drm_connector *connector) connector->connector_type == DRM_MODE_CONNECTOR_HDMIB) { debugfs_create_file("i915_hdcp_sink_capability", S_IRUGO, root, connector, &i915_hdcp_sink_capability_fops); + debugfs_create_file("i915_hdcp_version_request", 0444, root, + connector, &i915_hdcp_ver_request_fops); } if (INTEL_GEN(dev_priv) >= 10 &&
As per the current HDCP design, the driver selects the highest version of HDCP that can be used to satisfy the content-protection requirements of the user. Due to this, the content-protection tests cannot test a lower version of HDCP, if the platform and the display panel, both support higher HDCP version. To provide some support for testing and debugging, a per-connector debugfs is required to set the HDCP version via debugfs that the kernel can consider, while enabling HDCP. This patch adds a new debugfs entry for each connector that supports HDCP. For enforcing a particular HDCP version for a connector, the user can write into the debugfs for that connector. v2: As suggested by Jani Nikula: -used kstrtouint_from_user() to directly read as uint from user buffer. -used 32 bit flag instead of 64 bit for hdcp_ver flag. -removed unnecessary prints and fixed other minor formatting issues. Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com> --- .../drm/i915/display/intel_display_debugfs.c | 68 +++++++++++++++++++ 1 file changed, 68 insertions(+)