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@xxxxxxxxx>
> ---
> 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@xxxxxxxxxxxx>
> +
> 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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
