Re: [PATCH v6 02/10] drm/hdcp: Avoid changing crtc state in hdcp atomic check

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 18/01/2023 21:30, Mark Yacoub wrote:
From: Sean Paul <seanpaul@xxxxxxxxxxxx>

Instead of forcing a modeset in the hdcp atomic check, simply return
true if the content protection value is changing and let the driver
decide whether a modeset is required or not.

I don't think this is a good idea. All foo_atomic_check functions return an error code (or 0 if atomic check passes). Making drm_hpcp_atomic_check return bool is against that custom and is a clear way to make one forget to notice that. Please rename function to remove possible confusion.


Acked-by: Jani Nikula <jani.nikula@xxxxxxxxx>
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
Signed-off-by: Sean Paul <seanpaul@xxxxxxxxxxxx>
Signed-off-by: Mark Yacoub <markyacoub@xxxxxxxxxxxx>
Link: https://patchwork.freedesktop.org/patch/msgid/20210913175747.47456-3-sean@xxxxxxxxxx #v1
Link: https://patchwork.freedesktop.org/patch/msgid/20210915203834.1439-3-sean@xxxxxxxxxx #v2
Link: https://patchwork.freedesktop.org/patch/msgid/20211001151145.55916-3-sean@xxxxxxxxxx #v3
Link: https://patchwork.freedesktop.org/patch/msgid/20211105030434.2828845-3-sean@xxxxxxxxxx #v4
Link: https://patchwork.freedesktop.org/patch/msgid/20220411204741.1074308-3-sean@xxxxxxxxxx #v5

Changes in v2:
-None
Changes in v3:
-None
Changes in v4:
-None
Changes in v5:
-None
Changes in V6:
-Rebase: modifications in drm_hdcp_helper.c instead of drm_hdcp.c

---
  drivers/gpu/drm/display/drm_hdcp_helper.c   | 33 +++++++++++++++------
  drivers/gpu/drm/i915/display/intel_atomic.c |  6 ++--
  include/drm/display/drm_hdcp_helper.h       |  2 +-
  3 files changed, 27 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/display/drm_hdcp_helper.c b/drivers/gpu/drm/display/drm_hdcp_helper.c
index 7d910523b05f..a3896b0904b5 100644
--- a/drivers/gpu/drm/display/drm_hdcp_helper.c
+++ b/drivers/gpu/drm/display/drm_hdcp_helper.c
@@ -428,11 +428,14 @@ EXPORT_SYMBOL(drm_hdcp_update_content_protection);
   * @connector: drm_connector on which content protection state needs an update
   *
   * This function can be used by display drivers to perform an atomic check on the
- * hdcp state elements. If hdcp state has changed, this function will set
- * mode_changed on the crtc driving the connector so it can update its hardware
- * to match the hdcp state.
+ * hdcp state elements. If hdcp state has changed in a manner which requires the
+ * driver to enable or disable content protection, this function will return
+ * true.
+ *
+ * Returns:
+ * true if the driver must enable/disable hdcp, false otherwise
   */
-void drm_hdcp_atomic_check(struct drm_connector *connector,
+bool drm_hdcp_atomic_check(struct drm_connector *connector,
  			   struct drm_atomic_state *state)
  {
  	struct drm_connector_state *new_conn_state, *old_conn_state;
@@ -450,10 +453,12 @@ void drm_hdcp_atomic_check(struct drm_connector *connector,
  		 * If the connector is being disabled with CP enabled, mark it
  		 * desired so it's re-enabled when the connector is brought back
  		 */
-		if (old_hdcp == DRM_MODE_CONTENT_PROTECTION_ENABLED)
+		if (old_hdcp == DRM_MODE_CONTENT_PROTECTION_ENABLED) {
  			new_conn_state->content_protection =
  				DRM_MODE_CONTENT_PROTECTION_DESIRED;
-		return;
+			return true;
+		}
+		return false;
  	}
new_crtc_state =
@@ -465,9 +470,19 @@ void drm_hdcp_atomic_check(struct drm_connector *connector,
  	*/
  	if (drm_atomic_crtc_needs_modeset(new_crtc_state) &&
  	    (old_hdcp == DRM_MODE_CONTENT_PROTECTION_ENABLED &&
-	     new_hdcp != DRM_MODE_CONTENT_PROTECTION_UNDESIRED))
+	     new_hdcp != DRM_MODE_CONTENT_PROTECTION_UNDESIRED)) {
  		new_conn_state->content_protection =
  			DRM_MODE_CONTENT_PROTECTION_DESIRED;
+		return true;
+	}
+
+	/*
+	 * Coming back from disable or changing CRTC with DESIRED state requires
+	 * that the driver try CP enable.
+	 */
+	if (new_hdcp == DRM_MODE_CONTENT_PROTECTION_DESIRED &&
+	    new_conn_state->crtc != old_conn_state->crtc)
+		return true;
/*
  	 * Nothing to do if content type is unchanged and one of:
@@ -482,9 +497,9 @@ void drm_hdcp_atomic_check(struct drm_connector *connector,
  	     new_hdcp == DRM_MODE_CONTENT_PROTECTION_DESIRED)) {
  		if (old_conn_state->hdcp_content_type ==
  		    new_conn_state->hdcp_content_type)
-			return;
+			return false;
  	}
- new_crtc_state->mode_changed = true;
+	return true;
  }
  EXPORT_SYMBOL(drm_hdcp_atomic_check);
diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c b/drivers/gpu/drm/i915/display/intel_atomic.c
index 8a473199c4bf..a2067cbae2d5 100644
--- a/drivers/gpu/drm/i915/display/intel_atomic.c
+++ b/drivers/gpu/drm/i915/display/intel_atomic.c
@@ -123,8 +123,6 @@ int intel_digital_connector_atomic_check(struct drm_connector *conn,
  		to_intel_digital_connector_state(old_state);
  	struct drm_crtc_state *crtc_state;
- drm_hdcp_atomic_check(conn, state);
-
  	if (!new_state->crtc)
  		return 0;
@@ -140,8 +138,8 @@ int intel_digital_connector_atomic_check(struct drm_connector *conn,
  	    new_conn_state->base.picture_aspect_ratio != old_conn_state->base.picture_aspect_ratio ||
  	    new_conn_state->base.content_type != old_conn_state->base.content_type ||
  	    new_conn_state->base.scaling_mode != old_conn_state->base.scaling_mode ||
-	    new_conn_state->base.privacy_screen_sw_state != old_conn_state->base.privacy_screen_sw_state ||
-	    !drm_connector_atomic_hdr_metadata_equal(old_state, new_state))
+	    !drm_connector_atomic_hdr_metadata_equal(old_state, new_state) ||
+	    drm_hdcp_atomic_check(conn, state))
  		crtc_state->mode_changed = true;
return 0;
diff --git a/include/drm/display/drm_hdcp_helper.h b/include/drm/display/drm_hdcp_helper.h
index dd02b2e72a50..cb2cc5002f65 100644
--- a/include/drm/display/drm_hdcp_helper.h
+++ b/include/drm/display/drm_hdcp_helper.h
@@ -19,7 +19,7 @@ int drm_hdcp_check_ksvs_revoked(struct drm_device *dev, u8 *ksvs, u32 ksv_count)
  int drm_connector_attach_content_protection_property(struct drm_connector *connector,
  						     bool hdcp_content_type);
  void drm_hdcp_update_content_protection(struct drm_connector *connector, u64 val);
-void drm_hdcp_atomic_check(struct drm_connector *connector,
+bool drm_hdcp_atomic_check(struct drm_connector *connector,
  			   struct drm_atomic_state *state);
#endif

--
With best wishes
Dmitry




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux