Re: [PATCH v5 05/40] drm/i915: wrapping all hdcp var into intel_hdcp

On Wed, Jun 27, 2018 at 02:09:54PM +0530, Ramalingam C wrote:

Considering significant number of HDCP specific variables, it will be clean to have separate struct for HDCP.

New structure called intel_hdcp is added within intel_connector.

v2: struct hdcp statically allocated. [Sean Paul] enable and disable function parameters are retained.[Sean Paul] v3: No Changes. v4: Commit msg is rephrased [Uma] v5: Comment for mutex definition.

Signed-off-by: Ramalingam C ramalingam.c@intel.com

drivers/gpu/drm/i915/intel_display.c | 7 +-- drivers/gpu/drm/i915/intel_drv.h | 15 ++++-- drivers/gpu/drm/i915/intel_hdcp.c | 94 ++++++++++++++++++++---------------- 3 files changed, 67 insertions(+), 49 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 3d849ec17f5c..ef09bb89d2ca 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -15856,9 +15856,10 @@ static void intel_hpd_poll_fini(struct drm_device *dev) for_each_intel_connector_iter(connector, &conn_iter) { if (connector->modeset_retry_work.func) cancel_work_sync(&connector->modeset_retry_work);

• if (connector->hdcp_shim) {
  

• 	cancel_delayed_work_sync(&connector->hdcp_check_work);
  

• 	cancel_work_sync(&connector->hdcp_prop_work);
  

• if (connector->hdcp.hdcp_shim) {
  

• 	cancel_delayed_work_sync(
  

• 			&connector->hdcp.hdcp_check_work);
  

• 	cancel_work_sync(&connector->hdcp.hdcp_prop_work);
  

  } } drm_connector_list_iter_end(&conn_iter);

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 578346b8d7e2..eb480574a92e 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -377,6 +377,15 @@ struct intel_hdcp_shim { bool *hdcp_capable); };

+struct intel_hdcp {

• const struct intel_hdcp_shim *hdcp_shim;
• /* Mutex for hdcp state of the connector */
• struct mutex hdcp_mutex;
• uint64_t hdcp_value;
• struct delayed_work hdcp_check_work;
• struct work_struct hdcp_prop_work;

Now that they're in their own struct, you can probably drop the hdcp_* prefix from the struct members.

+};

struct intel_connector { struct drm_connector base; /* @@ -409,11 +418,7 @@ struct intel_connector { /* Work struct to schedule a uevent on link train failure */ struct work_struct modeset_retry_work;

• const struct intel_hdcp_shim *hdcp_shim;
• struct mutex hdcp_mutex;
• uint64_t hdcp_value; /* protected by hdcp_mutex */
• struct delayed_work hdcp_check_work;
• struct work_struct hdcp_prop_work;

• struct intel_hdcp hdcp;

};

struct intel_digital_connector_state { diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c index 0cc6a861bcf8..65bbe5874eee 100644 --- a/drivers/gpu/drm/i915/intel_hdcp.c +++ b/drivers/gpu/drm/i915/intel_hdcp.c @@ -626,6 +626,7 @@ struct intel_digital_port *conn_to_dig_port(struct intel_connector *connector)

static int _intel_hdcp_disable(struct intel_connector *connector) {

• struct intel_hdcp *hdcp = &connector->hdcp; struct drm_i915_private *dev_priv = connector->base.dev->dev_private; struct intel_digital_port *intel_dig_port = conn_to_dig_port(connector); enum port port = intel_dig_port->base.port;

@@ -641,7 +642,7 @@ static int _intel_hdcp_disable(struct intel_connector *connector) return -ETIMEDOUT; }

• ret = connector->hdcp_shim->toggle_signalling(intel_dig_port, false);

• ret = hdcp->hdcp_shim->toggle_signalling(intel_dig_port, false); if (ret) { DRM_ERROR("Failed to disable HDCP signalling\n"); return ret;

@@ -653,6 +654,7 @@ static int _intel_hdcp_disable(struct intel_connector *connector)

static int _intel_hdcp_enable(struct intel_connector *connector) {

• struct intel_hdcp *hdcp = &connector->hdcp; struct drm_i915_private *dev_priv = connector->base.dev->dev_private; int i, ret, tries = 3;

@@ -678,7 +680,7 @@ static int _intel_hdcp_enable(struct intel_connector *connector) /* Incase of authentication failures, HDCP spec expects reauth. */ for (i = 0; i < tries; i++) { ret = intel_hdcp_auth(conn_to_dig_port(connector),

• 		      connector->hdcp_shim);
  

• 		      hdcp->hdcp_shim);
  

  if (!ret) return 0;

@@ -694,36 +696,42 @@ static int _intel_hdcp_enable(struct intel_connector *connector)

static void intel_hdcp_check_work(struct work_struct *work) {

• struct intel_connector *connector = container_of(to_delayed_work(work),

• struct intel_hdcp *hdcp = container_of(to_delayed_work(work),

• 			       struct intel_hdcp,
  

• 			       hdcp_check_work);
  

• struct intel_connector *connector = container_of(hdcp, struct intel_connector,

• 					 hdcp_check_work);
  

• 					 hdcp);
  

Since you use this below, break this out into a static inline helper intel_hdcp_to_connector().

With those nits addressed,

Reviewed-by: Sean Paul seanpaul@chromium.org

• if (!intel_hdcp_check_link(connector))

• schedule_delayed_work(&connector->hdcp_check_work,
  

• schedule_delayed_work(&hdcp->hdcp_check_work,
  	      DRM_HDCP_CHECK_PERIOD_MS);
  

}

static void intel_hdcp_prop_work(struct work_struct *work) {

• struct intel_connector *connector = container_of(work,

• struct intel_hdcp *hdcp = container_of(work, struct intel_hdcp,

• 			       hdcp_prop_work);
  

• struct intel_connector *connector = container_of(hdcp, struct intel_connector,

• 					 hdcp_prop_work);
  

• 					 hdcp);
  

  struct drm_device *dev = connector->base.dev; struct drm_connector_state *state;

  drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);

• mutex_lock(&connector->hdcp_mutex);

• mutex_lock(&hdcp->hdcp_mutex);

  /*

  • This worker is only used to flip between ENABLED/DESIRED. Either of
  • those to UNDESIRED is handled by core. If hdcp_value == UNDESIRED,
  • we're running just after hdcp has been disabled, so just exit

  */

• if (connector->hdcp_value != DRM_MODE_CONTENT_PROTECTION_UNDESIRED) {

• if (hdcp->hdcp_value != DRM_MODE_CONTENT_PROTECTION_UNDESIRED) { state = connector->base.state;

• state->content_protection = connector->hdcp_value;
  

• state->content_protection = hdcp->hdcp_value;
  

  }

• mutex_unlock(&connector->hdcp_mutex);

• mutex_unlock(&hdcp->hdcp_mutex); drm_modeset_unlock(&dev->mode_config.connection_mutex);

}

@@ -737,6 +745,7 @@ bool is_hdcp_supported(struct drm_i915_private *dev_priv, enum port port) int intel_hdcp_init(struct intel_connector *connector, const struct intel_hdcp_shim *hdcp_shim) {

• struct intel_hdcp *hdcp = &connector->hdcp; int ret;

  ret = drm_connector_attach_content_protection_property(

@@ -744,51 +753,53 @@ int intel_hdcp_init(struct intel_connector *connector, if (ret) return ret;

• connector->hdcp_shim = hdcp_shim;
• mutex_init(&connector->hdcp_mutex);
• INIT_DELAYED_WORK(&connector->hdcp_check_work, intel_hdcp_check_work);
• INIT_WORK(&connector->hdcp_prop_work, intel_hdcp_prop_work);

• hdcp->hdcp_shim = hdcp_shim;
• mutex_init(&hdcp->hdcp_mutex);
• INIT_DELAYED_WORK(&hdcp->hdcp_check_work, intel_hdcp_check_work);
• INIT_WORK(&hdcp->hdcp_prop_work, intel_hdcp_prop_work); return 0;

}

int intel_hdcp_enable(struct intel_connector *connector) {

• struct intel_hdcp *hdcp = &connector->hdcp; int ret;

• if (!connector->hdcp_shim)

• if (!hdcp->hdcp_shim) return -ENOENT;

• mutex_lock(&connector->hdcp_mutex);

• mutex_lock(&hdcp->hdcp_mutex);

  ret = _intel_hdcp_enable(connector); if (ret) goto out;

• connector->hdcp_value = DRM_MODE_CONTENT_PROTECTION_ENABLED;
• schedule_work(&connector->hdcp_prop_work);
• schedule_delayed_work(&connector->hdcp_check_work,

• hdcp->hdcp_value = DRM_MODE_CONTENT_PROTECTION_ENABLED;
• schedule_work(&hdcp->hdcp_prop_work);
• schedule_delayed_work(&hdcp->hdcp_check_work, DRM_HDCP_CHECK_PERIOD_MS);

out:

• mutex_unlock(&connector->hdcp_mutex);

• mutex_unlock(&hdcp->hdcp_mutex); return ret;

}

int intel_hdcp_disable(struct intel_connector *connector) {

• struct intel_hdcp *hdcp = &connector->hdcp; int ret = 0;

• if (!connector->hdcp_shim)

• if (!hdcp->hdcp_shim) return -ENOENT;

• mutex_lock(&connector->hdcp_mutex);

• mutex_lock(&hdcp->hdcp_mutex);

• if (connector->hdcp_value != DRM_MODE_CONTENT_PROTECTION_UNDESIRED) {

• connector->hdcp_value = DRM_MODE_CONTENT_PROTECTION_UNDESIRED;
  

• if (hdcp->hdcp_value != DRM_MODE_CONTENT_PROTECTION_UNDESIRED) {

• hdcp->hdcp_value = DRM_MODE_CONTENT_PROTECTION_UNDESIRED;
  

  ret = _intel_hdcp_disable(connector); }

• mutex_unlock(&connector->hdcp_mutex);
• cancel_delayed_work_sync(&connector->hdcp_check_work);

• mutex_unlock(&hdcp->hdcp_mutex);
• cancel_delayed_work_sync(&hdcp->hdcp_check_work); return ret;

}

@@ -828,17 +839,18 @@ void intel_hdcp_atomic_check(struct drm_connector *connector, /* Implements Part 3 of the HDCP authorization procedure */ int intel_hdcp_check_link(struct intel_connector *connector) {

• struct intel_hdcp *hdcp = &connector->hdcp; struct drm_i915_private *dev_priv = connector->base.dev->dev_private; struct intel_digital_port *intel_dig_port = conn_to_dig_port(connector); enum port port = intel_dig_port->base.port; int ret = 0;

• if (!connector->hdcp_shim)

• if (!hdcp->hdcp_shim) return -ENOENT;

• mutex_lock(&connector->hdcp_mutex);

• mutex_lock(&hdcp->hdcp_mutex);

• if (connector->hdcp_value == DRM_MODE_CONTENT_PROTECTION_UNDESIRED)

• if (hdcp->hdcp_value == DRM_MODE_CONTENT_PROTECTION_UNDESIRED) goto out;

  if (!(I915_READ(PORT_HDCP_STATUS(port)) & HDCP_STATUS_ENC)) {

@@ -846,17 +858,17 @@ int intel_hdcp_check_link(struct intel_connector *connector) connector->base.name, connector->base.base.id, I915_READ(PORT_HDCP_STATUS(port))); ret = -ENXIO;

• connector->hdcp_value = DRM_MODE_CONTENT_PROTECTION_DESIRED;
  

• schedule_work(&connector->hdcp_prop_work);
  

• hdcp->hdcp_value = DRM_MODE_CONTENT_PROTECTION_DESIRED;
  

• schedule_work(&hdcp->hdcp_prop_work);
  

  goto out; }

• if (connector->hdcp_shim->check_link(intel_dig_port)) {

• if (connector->hdcp_value !=
  

• if (hdcp->hdcp_shim->check_link(intel_dig_port)) {

• if (hdcp->hdcp_value !=
  

  DRM_MODE_CONTENT_PROTECTION_UNDESIRED) {

• 	connector->hdcp_value =
  

• 	hdcp->hdcp_value =
  	DRM_MODE_CONTENT_PROTECTION_ENABLED;
  

• 	schedule_work(&connector->hdcp_prop_work);
  

• 	schedule_work(&hdcp->hdcp_prop_work);
  

  } goto out; }

@@ -867,20 +879,20 @@ int intel_hdcp_check_link(struct intel_connector *connector) ret = _intel_hdcp_disable(connector); if (ret) { DRM_ERROR("Failed to disable hdcp (%d)\n", ret);

• connector->hdcp_value = DRM_MODE_CONTENT_PROTECTION_DESIRED;
  

• schedule_work(&connector->hdcp_prop_work);
  

• hdcp->hdcp_value = DRM_MODE_CONTENT_PROTECTION_DESIRED;
  

• schedule_work(&hdcp->hdcp_prop_work);
  

  goto out; }

  ret = _intel_hdcp_enable(connector); if (ret) { DRM_ERROR("Failed to enable hdcp (%d)\n", ret);

• connector->hdcp_value = DRM_MODE_CONTENT_PROTECTION_DESIRED;
  

• schedule_work(&connector->hdcp_prop_work);
  

• hdcp->hdcp_value = DRM_MODE_CONTENT_PROTECTION_DESIRED;
  

• schedule_work(&hdcp->hdcp_prop_work);
  

  goto out; }

out:

• mutex_unlock(&connector->hdcp_mutex);

• mutex_unlock(&hdcp->hdcp_mutex); return ret;

}

2.7.4

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel