On Thursday 22 February 2018 08:17 PM, Sean Paul wrote:
On Wed, Feb 14, 2018 at 07:43:38PM +0530, Ramalingam C wrote:Considering the upcoming significant no HDCP2.2 variables, it will be clean to have separate struct fo HDCP. New structure called intel_hdcp is introduced. Signed-off-by: Ramalingam C <ramalingam.c@xxxxxxxxx> --- drivers/gpu/drm/i915/intel_display.c | 9 ++- drivers/gpu/drm/i915/intel_dp.c | 4 +- drivers/gpu/drm/i915/intel_drv.h | 20 +++-- drivers/gpu/drm/i915/intel_hdcp.c | 141 ++++++++++++++++++++--------------- 4 files changed, 100 insertions(+), 74 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 048d60b5143b..22097349529b 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -15314,10 +15314,11 @@ 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); - cancel_work_sync(&connector->hdcp_enable_work); + if (connector->hdcp && connector->hdcp->hdcp_shim) { + cancel_delayed_work_sync( + &connector->hdcp->hdcp_check_work); + cancel_work_sync(&connector->hdcp->hdcp_prop_work); + cancel_work_sync(&connector->hdcp->hdcp_enable_work); } } drm_connector_list_iter_end(&conn_iter); diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index f20b25f98e5a..80476689754f 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -5445,7 +5445,9 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd) WARN(iret, "Acquiring modeset locks failed with %i\n", iret);/* Short pulse can signify loss of hdcp authentication */- intel_hdcp_check_link(intel_dp->attached_connector); + if (intel_dp->attached_connector->hdcp) + intel_hdcp_check_link( + intel_dp->attached_connector->hdcp);if (!handled) {intel_dp->detect_done = false; diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 7b9e5f70826f..5b170ff7ec14 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -375,6 +375,17 @@ struct intel_hdcp_shim { bool *hdcp_capable); };+struct intel_hdcp {+ struct intel_connector *connector;You don't need a backpointer, use container_of if necessary.
ok sure.
didn't want the mem spared for struct hdcp, incase of non hdcp capable connectors. May be its ok to have small struct for all connectors. i will move it from dynamic alloc.+ + 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 work_struct hdcp_enable_work; +}; + struct intel_connector { struct drm_connector base; /* @@ -407,12 +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 work_struct hdcp_enable_work; + struct intel_hdcp *hdcp; };struct intel_digital_connector_state {@@ -1853,7 +1859,7 @@ int intel_hdcp_init(struct intel_connector *connector, const struct intel_hdcp_shim *hdcp_shim); int intel_hdcp_enable(struct intel_connector *connector); int intel_hdcp_disable(struct intel_connector *connector); -int intel_hdcp_check_link(struct intel_connector *connector); +int intel_hdcp_check_link(struct intel_hdcp *hdcp); bool is_hdcp_supported(struct drm_i915_private *dev_priv, enum port port);/* intel_psr.c */diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c index e03bd376d92c..9147fb17a9fc 100644 --- a/drivers/gpu/drm/i915/intel_hdcp.c +++ b/drivers/gpu/drm/i915/intel_hdcp.c @@ -545,8 +545,9 @@ struct intel_digital_port *conn_to_dig_port(struct intel_connector *connector) return enc_to_dig_port(&intel_attached_encoder(&connector->base)->base); }-static int _intel_hdcp_disable(struct intel_connector *connector)+static int _intel_hdcp_disable(struct intel_hdcp *hdcp) { + struct intel_connector *connector = hdcp->connector;I'm not sure why we need to change all of the function arguments to intel_hdcp if the first thing we do is fetch the connector.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; @@ -562,7 +563,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; @@ -572,8 +573,9 @@ static int _intel_hdcp_disable(struct intel_connector *connector) return 0; }-static int _intel_hdcp_enable(struct intel_connector *connector)+static int _intel_hdcp_enable(struct intel_hdcp *hdcp) { + struct intel_connector *connector = hdcp->connector; struct drm_i915_private *dev_priv = connector->base.dev->dev_private; int i, ret, tries = 3;@@ -599,12 +601,12 @@ 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) { - connector->hdcp_value = + hdcp->hdcp_value = DRM_MODE_CONTENT_PROTECTION_ENABLED; - schedule_work(&connector->hdcp_prop_work); - schedule_delayed_work(&connector->hdcp_check_work, + schedule_work(&hdcp->hdcp_prop_work); + schedule_delayed_work(&hdcp->hdcp_check_work, DRM_HDCP_CHECK_PERIOD_MS); return 0; } @@ -612,7 +614,7 @@ static int _intel_hdcp_enable(struct intel_connector *connector) DRM_DEBUG_KMS("HDCP Auth failure (%d)\n", ret);/* Ensuring HDCP encryption and signalling are stopped. */- _intel_hdcp_disable(connector); + _intel_hdcp_disable(hdcp); }DRM_ERROR("HDCP authentication failed (%d tries/%d)\n", tries, ret);@@ -621,47 +623,45 @@ static int _intel_hdcp_enable(struct intel_connector *connector)static void intel_hdcp_enable_work(struct work_struct *work){ - struct intel_connector *connector = container_of(work, - struct intel_connector, - hdcp_enable_work); + struct intel_hdcp *hdcp = container_of(work, struct intel_hdcp, + hdcp_enable_work);- mutex_lock(&connector->hdcp_mutex);- _intel_hdcp_enable(connector); - mutex_unlock(&connector->hdcp_mutex); + mutex_lock(&hdcp->hdcp_mutex); + _intel_hdcp_enable(hdcp); + mutex_unlock(&hdcp->hdcp_mutex); }static void intel_hdcp_check_work(struct work_struct *work){ - struct intel_connector *connector = container_of(to_delayed_work(work), - struct intel_connector, - hdcp_check_work); - if (!intel_hdcp_check_link(connector)) - schedule_delayed_work(&connector->hdcp_check_work, + struct intel_hdcp *hdcp = container_of(to_delayed_work(work), + struct intel_hdcp, + hdcp_check_work); + if (!intel_hdcp_check_link(hdcp)) + 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_connector, - hdcp_prop_work); - struct drm_device *dev = connector->base.dev; + struct intel_hdcp *hdcp = container_of(work, struct intel_hdcp, + hdcp_prop_work); + struct drm_device *dev = hdcp->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) { - state = connector->base.state; - state->content_protection = connector->hdcp_value; + if (hdcp->hdcp_value != DRM_MODE_CONTENT_PROTECTION_UNDESIRED) { + state = hdcp->connector->base.state; + state->content_protection = hdcp->hdcp_value; }- mutex_unlock(&connector->hdcp_mutex);+ mutex_unlock(&hdcp->hdcp_mutex); drm_modeset_unlock(&dev->mode_config.connection_mutex); }@@ -676,48 +676,64 @@ int intel_hdcp_init(struct intel_connector *connector,const struct intel_hdcp_shim *hdcp_shim) { int ret; + struct intel_hdcp *hdcp; + + hdcp = kzalloc(sizeof(struct intel_hdcp), GFP_KERNEL);You're leaking this. You probably don't need it to be dynamically allocated.
+ if (!hdcp) { + ret = -ENOMEM; + goto err; + }ret = drm_connector_attach_content_protection_property(&connector->base); if (ret) - return ret; + goto err; + + 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); + INIT_WORK(&hdcp->hdcp_enable_work, intel_hdcp_enable_work);- 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); - INIT_WORK(&connector->hdcp_enable_work, intel_hdcp_enable_work); + connector->hdcp = hdcp; return 0; + +err: + kfree(hdcp); + return ret; }int intel_hdcp_enable(struct intel_connector *connector){ - if (!connector->hdcp_shim) + struct intel_hdcp *hdcp = connector->hdcp; + + if (!hdcp || !hdcp->hdcp_shim)Is it possible for hdcp to be != NULL, but hdcp_shim == NULL?
Along with hdcp2.2, this could be a possibility. Anyway by moving to static allocation for hdcp, no more check for hdcp ptr :) --Ram
return -ENOENT;- mutex_lock(&connector->hdcp_mutex);- schedule_work(&connector->hdcp_enable_work); - mutex_unlock(&connector->hdcp_mutex); + mutex_lock(&hdcp->hdcp_mutex); + schedule_work(&hdcp->hdcp_enable_work); + mutex_unlock(&hdcp->hdcp_mutex);return 0;}int intel_hdcp_disable(struct intel_connector *connector){ + struct intel_hdcp *hdcp = connector->hdcp; int ret = 0;- if (!connector->hdcp_shim)+ if (!hdcp || !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; - ret = _intel_hdcp_disable(connector); + if (hdcp->hdcp_value != DRM_MODE_CONTENT_PROTECTION_UNDESIRED) { + hdcp->hdcp_value = DRM_MODE_CONTENT_PROTECTION_UNDESIRED; + ret = _intel_hdcp_disable(hdcp); }- 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; }@@ -755,19 +771,20 @@ 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) +int intel_hdcp_check_link(struct intel_hdcp *hdcp) { + struct intel_connector *connector = hdcp->connector; 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)) {@@ -775,17 +792,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; } @@ -793,23 +810,23 @@ int intel_hdcp_check_link(struct intel_connector *connector) DRM_DEBUG_KMS("[%s:%d] HDCP link failed, retrying authentication\n", connector->base.name, connector->base.base.id);- ret = _intel_hdcp_disable(connector);+ ret = _intel_hdcp_disable(hdcp); 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);+ ret = _intel_hdcp_enable(hdcp); 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
_______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
