From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.8 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 10AFAC2D0DB for ; Mon, 20 Jan 2020 10:20:19 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id E33E820684 for ; Mon, 20 Jan 2020 10:20:18 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org E33E820684 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linux.intel.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=intel-gfx-bounces@lists.freedesktop.org Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 67A6D6E1F8; Mon, 20 Jan 2020 10:20:18 +0000 (UTC) Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by gabe.freedesktop.org (Postfix) with ESMTPS id 996006E1F8 for ; Mon, 20 Jan 2020 10:20:17 +0000 (UTC) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga006.jf.intel.com ([10.7.209.51]) by fmsmga106.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 20 Jan 2020 02:20:12 -0800 X-IronPort-AV: E=Sophos;i="5.70,341,1574150400"; d="scan'208";a="228338715" Received: from jnikula-mobl3.fi.intel.com (HELO localhost) ([10.237.66.161]) by orsmga006-auth.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 20 Jan 2020 02:20:10 -0800 From: Jani Nikula To: Anshuman Gupta , intel-gfx@lists.freedesktop.org In-Reply-To: <20200120054954.5786-1-anshuman.gupta@intel.com> Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo References: <20200120054954.5786-1-anshuman.gupta@intel.com> Date: Mon, 20 Jan 2020 12:20:07 +0200 Message-ID: <87zheifmzs.fsf@intel.com> MIME-Version: 1.0 Subject: Re: [Intel-gfx] [PATCH v3] drm/i915/hdcp: Update CP as per the kernel internal state X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" On Mon, 20 Jan 2020, Anshuman Gupta wrote: > Content Protection property should be updated as per the kernel > internal state. Let's say if Content protection is disabled > by userspace, CP property should be set to UNDESIRED so that > reauthentication will not happen until userspace request it again, > but when kernel disables the HDCP due to any DDI disabling sequences > like modeset/DPMS operation, kernel should set the property to > DESIRED, so that when opportunity arises, kernel will start the > HDCP authentication on its own. > > Somewhere in the line, state machine to set content protection to > DESIRED from kernel was broken and IGT coverage was missing for it. > This patch fixes it. > IGT patch to catch further regression on this features is being > worked upon. > > v2: > - Incorporated the necessary locking. (Ram) > - Set content protection property to CP_DESIRED only when > user has not asked explicitly to set CP_UNDESIRED. > > v3: > - Reset the is_hdcp_undesired flag to false. (Ram) > - Rephrasing commit log and small comment for is_hdcp_desired > flag. (Ram) > > CC: Ramalingam C > Reviewed-by: Ramalingam C > Signed-off-by: Anshuman Gupta > --- > drivers/gpu/drm/i915/display/intel_display_types.h | 6 ++++++ > drivers/gpu/drm/i915/display/intel_hdcp.c | 13 ++++++++++++- > 2 files changed, 18 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h > index 630a94892b7b..401a9a7689fb 100644 > --- a/drivers/gpu/drm/i915/display/intel_display_types.h > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > @@ -345,6 +345,12 @@ struct intel_hdcp { > struct delayed_work check_work; > struct work_struct prop_work; > > + /* > + * Flag to differentiate that HDCP is being disabled originated from > + * userspace or triggered from kernel DDI disable sequence. > + */ > + bool is_hdcp_undesired; > + First, the comment and the name of the member don't align. The member name does not explain what it is, and you absolutely need to look at the comment for it to make sense in code. > /* HDCP1.4 Encryption status */ > bool hdcp_encrypted; > > diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.c b/drivers/gpu/drm/i915/display/intel_hdcp.c > index 0fdbd39f6641..7f631ebd8395 100644 > --- a/drivers/gpu/drm/i915/display/intel_hdcp.c > +++ b/drivers/gpu/drm/i915/display/intel_hdcp.c > @@ -2002,11 +2002,18 @@ int intel_hdcp_disable(struct intel_connector *connector) > mutex_lock(&hdcp->mutex); > > if (hdcp->value != DRM_MODE_CONTENT_PROTECTION_UNDESIRED) { > - hdcp->value = DRM_MODE_CONTENT_PROTECTION_UNDESIRED; > if (hdcp->hdcp2_encrypted) > ret = _intel_hdcp2_disable(connector); > else if (hdcp->hdcp_encrypted) > ret = _intel_hdcp_disable(connector); > + > + if (hdcp->is_hdcp_undesired) { > + hdcp->value = DRM_MODE_CONTENT_PROTECTION_UNDESIRED; > + hdcp->is_hdcp_undesired = false; > + } else { > + hdcp->value = DRM_MODE_CONTENT_PROTECTION_DESIRED; > + schedule_work(&hdcp->prop_work); > + } Second, all the places you call intel_hdcp_disable() from have access to the connector state, so IIUC you could deduce the state from there and pass it to intel_hdcp_disable() without having to add another flag/sub-state such as ->is_hdcp_undesired. Third, reading the code, I am not sure I fully appreciate why there is a duplicated hdcp->value, instead of using the content_protection member from connector state to begin with. Between those two and the one added in this patch, looks like there's a lot of state that could go out of sync. > } > > mutex_unlock(&hdcp->mutex); > @@ -2044,6 +2051,7 @@ void intel_hdcp_atomic_check(struct drm_connector *connector, > { > u64 old_cp = old_state->content_protection; > u64 new_cp = new_state->content_protection; > + struct intel_connector *intel_conn = to_intel_connector(connector); > struct drm_crtc_state *crtc_state; > > if (!new_state->crtc) { > @@ -2069,6 +2077,9 @@ void intel_hdcp_atomic_check(struct drm_connector *connector, > return; > } > > + if (new_cp == DRM_MODE_CONTENT_PROTECTION_UNDESIRED) > + intel_conn->hdcp.is_hdcp_undesired = true; > + Finally, this underlines the above points. If atomic check fails later for some unrelated reason, you've still ended up changing the connector. You'll tell userspace things failed, but mysteriously things will change anyway. It is almost never a good idea to add more state variables if you can figure the same thing from the single point of truth. BR, Jani. > crtc_state = drm_atomic_get_new_crtc_state(new_state->state, > new_state->crtc); > crtc_state->mode_changed = true; -- Jani Nikula, Intel Open Source Graphics Center _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx