[Intel-gfx] [PATCH 23/43] drm/i915: wrapping all hdcp var into intel_hdcp
Ramalingam C
ramalingam.c at intel.com
Mon Feb 26 06:05:41 UTC 2018
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 at intel.com>
>> ---
>> 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.
>
>> +
>> + 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.
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.
>
>> + 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
>>
More information about the Intel-gfx
mailing list