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.
> +
> + 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?
> 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
>
--
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
