-----Original Message----- From: Sean Paul [mailto:seanpaul@chromium.org] Sent: Tuesday, July 10, 2018 2:00 AM To: C, Ramalingam ramalingam.c@intel.com Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; daniel@ffwll.ch; Winkler, Tomas tomas.winkler@intel.com; Usyskin, Alexander alexander.usyskin@intel.com; Shankar, Uma uma.shankar@intel.com Subject: 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.
Sure I will do that.
+};
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);@@ -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().
Sure.
With those nits addressed,
Reviewed-by: Sean Paul seanpaul@chromium.org
Thanks for the R-b.
Ram.
- 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);
- 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 !=
connector->hdcp_value =
hdcp->hdcp_value =DRM_MODE_CONTENT_PROTECTION_ENABLED;
schedule_work(&connector->hdcp_prop_work);
schedule_work(&hdcp->hdcp_prop_work);@@ -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);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
-- Sean Paul, Software Engineer, Google / Chromium OS