> -----Original Message-----
> From: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>
> Sent: Friday, February 7, 2025 4:24 PM
> To: Kandpal, Suraj <suraj.kandpal@xxxxxxxxx>; intel-
> xe@xxxxxxxxxxxxxxxxxxxxx; intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> Cc: Nautiyal, Ankit K <ankit.k.nautiyal@xxxxxxxxx>; Reddy Guddati, Santhosh
> <santhosh.reddy.guddati@xxxxxxxxx>; Kandpal, Suraj
> <suraj.kandpal@xxxxxxxxx>
> Subject: Re: [PATCH] drm/i915/hdcp: Create force_hdcp14 debug fs entry
>
> On Fri, 07 Feb 2025, Suraj Kandpal <suraj.kandpal@xxxxxxxxx> wrote:
> > Testing HDCP 1.4 becomes tough since the only way our code comes to
> > HDCP 1.4 pathway is if the monitor only supports HDCP 1.4 which
> > becomes tough to find sometimes.
> > Setting this debug_fs entry will force use to use the HDCP 1.4 path so
> > that more robust HDCP 1.4 testing can take place.
>
> For several years now the direction has been to move and place the debugfs
> handling next to the implementation, instead of piling on in
> intel_display_debugfs.c. It's been happening gradually.
>
> It's time to draw a line in the sand. I'm not accepting any new code in
> intel_display_debugfs.c, including this one. It needs to move.
>
> I've rebased and reposted an old patch to move hdcp debugfs to
> intel_hdcp.c. After that, this needs to go to intel_hdcp.c.
>
> Some other comments inline.
>
> > Signed-off-by: Suraj Kandpal <suraj.kandpal@xxxxxxxxx>
> > ---
> > .../drm/i915/display/intel_display_debugfs.c | 76 +++++++++++++++++++
> > .../drm/i915/display/intel_display_types.h | 2 +
> > drivers/gpu/drm/i915/display/intel_hdcp.c | 2 +-
> > 3 files changed, 79 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> > b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> > index 926f09c35084..1b34aed98849 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> > @@ -1383,6 +1383,80 @@ static const struct file_operations
> i915_joiner_fops = {
> > .write = i915_joiner_write
> > };
> >
> > +static ssize_t i915_force_hdcp14_write(struct file *file,
> > + const char __user *ubuf,
> > + size_t len, loff_t *offp)
> > +{
> > + struct seq_file *m = file->private_data;
> > + struct intel_connector *connector = m->private;
> > + struct intel_display *display = to_intel_display(connector);
> > + struct intel_hdcp *hdcp = &connector->hdcp;
> > + bool force_hdcp14 = false;
> > + int ret;
> > +
> > + if (len == 0)
> > + return 0;
> > +
> > + drm_dbg(display->drm,
> > + "Copied %zu bytes from user to force DSC\n", len);
>
> Useless debug logging.
Will remove.
>
> > +
> > + ret = kstrtobool_from_user(ubuf, len, &force_hdcp14);
> > + if (ret < 0)
> > + return ret;
> > +
> > + drm_dbg(display->drm, "Got %s for force HDCP1.4\n",
> > + (force_hdcp14) ? "true" : "false");
>
> Also useless debug logging.
Sure will remove it.
>
> > + hdcp->force_hdcp14 = force_hdcp14;
> > +
> > + *offp += len;
> > + return len;
> > +}
> > +
> > +static int i915_force_hdcp14_show(struct seq_file *m, void *data) {
> > + struct intel_connector *connector = m->private;
> > + struct intel_display *display = to_intel_display(connector);
> > + struct intel_encoder *encoder = intel_attached_encoder(connector);
> > + struct intel_hdcp *hdcp = &connector->hdcp;
> > + struct drm_crtc *crtc;
> > + int ret;
> > +
> > + if (!encoder)
> > + return -ENODEV;
> > +
> > + ret = drm_modeset_lock_single_interruptible(&display->drm-
> >mode_config.connection_mutex);
> > + if (ret)
> > + return ret;
> > +
> > + crtc = connector->base.state->crtc;
> > + if (connector->base.status != connector_status_connected || !crtc) {
> > + ret = -ENODEV;
> > + goto out;
> > + }
> > +
> > + seq_printf(m, "Force_HDCP14: %s\n",
> > + str_yes_no(hdcp->force_hdcp14));
>
> So I'm not sure why we're printing "Force_HDCP14: " in there. Why isn't just
> yes/no enough? The name of the debugfs attribute is literally
> i915_force_hdcp14.
>
Okay will just print yes or no here I just went with what we have previously been doing with debug fs entry.
> > +out:
> > + drm_modeset_unlock(&display->drm-
> >mode_config.connection_mutex);
>
> Blank line before return please.
>
Sure will add it.
> > + return ret;
> > +}
> > +
> > +static int i915_force_hdcp14_open(struct inode *inode,
> > + struct file *file)
> > +{
> > + return single_open(file, i915_force_hdcp14_show,
> > + inode->i_private);
> > +}
> > +
> > +static const struct file_operations i915_force_hdcp14_fops = {
> > + .owner = THIS_MODULE,
> > + .open = i915_force_hdcp14_open,
> > + .read = seq_read,
> > + .llseek = seq_lseek,
> > + .release = single_release,
> > + .write = i915_force_hdcp14_write
> > +};
> > +
> > /**
> > * intel_connector_debugfs_add - add i915 specific connector debugfs
> files
> > * @connector: pointer to a registered intel_connector @@ -1411,6
> > +1485,8 @@ void intel_connector_debugfs_add(struct intel_connector
> *connector)
> > connector_type == DRM_MODE_CONNECTOR_HDMIB) {
> > debugfs_create_file("i915_hdcp_sink_capability", 0444, root,
> > connector,
> &i915_hdcp_sink_capability_fops);
> > + debugfs_create_file("i915_force_hdcp14", 0644, root,
> > + connector, &i915_force_hdcp14_fops);
> > }
> >
> > if (DISPLAY_VER(i915) >= 11 &&
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> > b/drivers/gpu/drm/i915/display/intel_display_types.h
> > index 6a82c6ade549..c78dd77ef74c 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > @@ -496,6 +496,8 @@ struct intel_hdcp {
> > enum transcoder cpu_transcoder;
> > /* Only used for DP MST stream encryption */
> > enum transcoder stream_transcoder;
> > + /* Used to force HDCP 1.4 bypassing HDCP 2.x */
> > + bool force_hdcp14;
> > };
> >
> > struct intel_connector {
> > diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.c
> > b/drivers/gpu/drm/i915/display/intel_hdcp.c
> > index 7cc0399b2a5d..c008e4f1ce05 100644
> > --- a/drivers/gpu/drm/i915/display/intel_hdcp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_hdcp.c
> > @@ -2473,7 +2473,7 @@ static int _intel_hdcp_enable(struct
> intel_atomic_state *state,
> > * Considering that HDCP2.2 is more secure than HDCP1.4, If the
> setup
> > * is capable of HDCP2.2, it is preferred to use HDCP2.2.
> > */
> > - if (intel_hdcp2_get_capability(connector)) {
> > + if (intel_hdcp2_get_capability(connector) && !hdcp->force_hdcp14)
> {
>
> Why isn't this check inside intel_hdcp2_get_capability()? That's used in
> more places, and now you've selected just one place that forces 1.4. Isn't
> that inconsistent?
Well we really don't what to change what capability we report here to the user,
what we want is that we have to ability to forcefully enable HDCP 1.4 even if HDCP 2.2 is available
(majorly used for ease of testing).
This is the only place where we need to check to see have that happen.
>
> Also, that might be a place to debug log about forcing hdcp2 support off, if
> you really need debugfs.
Sure will add the debug log here.
Regards,
Suraj Kandpal
>
> > ret = _intel_hdcp2_enable(state, connector);
> > if (!ret)
> > check_link_interval =
>
> --
> Jani Nikula, Intel
