diff mbox series

[v7,3/3] drm/bridge: it6505: handle HDCP request

Message ID 20221123100529.3943662-3-hsinyi@chromium.org
State New
Headers
Series [v7,1/3] drm_bridge: register content protect property |

Commit Message

Hsin-Yi Wang Nov. 23, 2022, 10:05 a.m. UTC 
  it6505 supports HDCP 1.3, but current implementation lacks the update of
HDCP status through drm_hdcp_update_content_protection().

it6505 default enables the HDCP. Remove this and only turn on when user
requests it.

Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
Reviewed-by: allen chen <allen.chen@ite.com.tw>
---
v6->v7: remove enable hdcp by default.
---
 drivers/gpu/drm/bridge/ite-it6505.c | 60 +++++++++++++++++++++++++++--
 1 file changed, 57 insertions(+), 3 deletions(-)

Comments

Sean Paul Nov. 23, 2022, 8:16 p.m. UTC | #1 
On Wed, Nov 23, 2022 at 10:05:29AM +0000, Hsin-Yi Wang wrote:
> it6505 supports HDCP 1.3, but current implementation lacks the update of
> HDCP status through drm_hdcp_update_content_protection().
> 
> it6505 default enables the HDCP. Remove this and only turn on when user
> requests it.
> 
> Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
> Reviewed-by: allen chen <allen.chen@ite.com.tw>
> ---
> v6->v7: remove enable hdcp by default.
> ---
>  drivers/gpu/drm/bridge/ite-it6505.c | 60 +++++++++++++++++++++++++++--
>  1 file changed, 57 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
> index 21a9b8422bda..93626698c31e 100644
> --- a/drivers/gpu/drm/bridge/ite-it6505.c
> +++ b/drivers/gpu/drm/bridge/ite-it6505.c
> @@ -423,6 +423,7 @@ struct it6505 {
>  	struct extcon_dev *extcon;
>  	struct work_struct extcon_wq;
>  	int extcon_state;
> +	struct drm_connector *connector;
>  	enum drm_connector_status connector_status;
>  	enum link_train_status link_state;
>  	struct work_struct link_works;
> @@ -2159,9 +2160,6 @@ static void it6505_link_train_ok(struct it6505 *it6505)
>  		DRM_DEV_DEBUG_DRIVER(dev, "Enable audio!");
>  		it6505_enable_audio(it6505);
>  	}
> -
> -	if (it6505->hdcp_desired)
> -		it6505_start_hdcp(it6505);
>  }
>  
>  static void it6505_link_step_train_process(struct it6505 *it6505)
> @@ -2399,6 +2397,14 @@ static void it6505_irq_hdcp_done(struct it6505 *it6505)
>  
>  	DRM_DEV_DEBUG_DRIVER(dev, "hdcp done interrupt");
>  	it6505->hdcp_status = HDCP_AUTH_DONE;
> +	if (it6505->connector) {
> +		struct drm_device *drm_dev = it6505->connector->dev;
> +
> +		drm_modeset_lock(&drm_dev->mode_config.connection_mutex, NULL);
> +		drm_hdcp_update_content_protection(it6505->connector,
> +						   DRM_MODE_CONTENT_PROTECTION_ENABLED);
> +		drm_modeset_unlock(&drm_dev->mode_config.connection_mutex);
> +	}
>  	it6505_show_hdcp_info(it6505);
>  }
>  
> @@ -2931,6 +2937,7 @@ static void it6505_bridge_atomic_enable(struct drm_bridge *bridge,
>  	if (WARN_ON(!connector))
>  		return;
>  
> +	it6505->connector = connector;
>  	conn_state = drm_atomic_get_new_connector_state(state, connector);
>  
>  	if (WARN_ON(!conn_state))
> @@ -2974,6 +2981,7 @@ static void it6505_bridge_atomic_disable(struct drm_bridge *bridge,
>  
>  	DRM_DEV_DEBUG_DRIVER(dev, "start");
>  
> +	it6505->connector = NULL;
>  	if (it6505->powered) {
>  		it6505_drm_dp_link_set_power(&it6505->aux, &it6505->link,
>  					     DP_SET_POWER_D3);
> @@ -3028,6 +3036,50 @@ static struct edid *it6505_bridge_get_edid(struct drm_bridge *bridge,
>  	return edid;
>  }
>  
> +static int it6505_connector_atomic_check(struct it6505 *it6505,
> +					 struct drm_connector_state *state)
> +{
> +	struct device *dev = &it6505->client->dev;
> +	int cp = state->content_protection;
> +
> +	DRM_DEV_DEBUG_DRIVER(dev, "hdcp connector state:%d, curr hdcp state:%d",
> +			     cp, it6505->hdcp_status);
> +
> +	if (!it6505->hdcp_desired) {
> +		DRM_DEV_DEBUG_DRIVER(dev, "sink not support hdcp");
> +		return 0;
> +	}
> +
> +	if (it6505->hdcp_status == HDCP_AUTH_GOING)
> +		return -EINVAL;
> +
> +	if (cp == DRM_MODE_CONTENT_PROTECTION_UNDESIRED) {
> +		if (it6505->hdcp_status == HDCP_AUTH_DONE)
> +			it6505_stop_hdcp(it6505);

You shouldn't touch the hardware in atomic_check, this should be done in the
commit.

> +	} else if (cp == DRM_MODE_CONTENT_PROTECTION_DESIRED) {
> +		if (it6505->hdcp_status == HDCP_AUTH_IDLE &&
> +		    it6505->link_state == LINK_OK)
> +			it6505_start_hdcp(it6505);

Same here

> +	} else {
> +		if (it6505->hdcp_status == HDCP_AUTH_IDLE) {
> +			DRM_DEV_DEBUG_DRIVER(dev, "invalid to set hdcp enabled");
> +			return -EINVAL;
> +		}
> +	}

In general, I think there are a number of locking and state issues with this
approach. I had pulled all of this logic into a set of helpers [1], but the
patchset has gotten stale on the list. You might consider looking at patches 1-4
to see how the state and locking should be handled to avoid races.

Sean

[1] - https://lore.kernel.org/all/20220411204741.1074308-1-sean@poorly.run/

> +
> +	return 0;
> +}
> +
> +static int it6505_bridge_atomic_check(struct drm_bridge *bridge,
> +				      struct drm_bridge_state *bridge_state,
> +				      struct drm_crtc_state *crtc_state,
> +				      struct drm_connector_state *conn_state)
> +{
> +	struct it6505 *it6505 = bridge_to_it6505(bridge);
> +
> +	return it6505_connector_atomic_check(it6505, conn_state);
> +}
> +
>  static const struct drm_bridge_funcs it6505_bridge_funcs = {
>  	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
>  	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
> @@ -3035,6 +3087,7 @@ static const struct drm_bridge_funcs it6505_bridge_funcs = {
>  	.attach = it6505_bridge_attach,
>  	.detach = it6505_bridge_detach,
>  	.mode_valid = it6505_bridge_mode_valid,
> +	.atomic_check = it6505_bridge_atomic_check,
>  	.atomic_enable = it6505_bridge_atomic_enable,
>  	.atomic_disable = it6505_bridge_atomic_disable,
>  	.atomic_pre_enable = it6505_bridge_atomic_pre_enable,
> @@ -3354,6 +3407,7 @@ static int it6505_i2c_probe(struct i2c_client *client,
>  	it6505->bridge.type = DRM_MODE_CONNECTOR_DisplayPort;
>  	it6505->bridge.ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID |
>  			     DRM_BRIDGE_OP_HPD;
> +	it6505->bridge.support_hdcp = true;
>  	drm_bridge_add(&it6505->bridge);
>  
>  	return 0;
> -- 
> 2.38.1.584.g0f3c55d4c2-goog
>
Hsin-Yi Wang Nov. 24, 2022, 4:53 a.m. UTC | #2 
On Thu, Nov 24, 2022 at 4:16 AM Sean Paul <sean@poorly.run> wrote:
>
> On Wed, Nov 23, 2022 at 10:05:29AM +0000, Hsin-Yi Wang wrote:
> > it6505 supports HDCP 1.3, but current implementation lacks the update of
> > HDCP status through drm_hdcp_update_content_protection().
> >
> > it6505 default enables the HDCP. Remove this and only turn on when user
> > requests it.
> >
> > Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
> > Reviewed-by: allen chen <allen.chen@ite.com.tw>
> > ---
> > v6->v7: remove enable hdcp by default.
> > ---
> >  drivers/gpu/drm/bridge/ite-it6505.c | 60 +++++++++++++++++++++++++++--
> >  1 file changed, 57 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
> > index 21a9b8422bda..93626698c31e 100644
> > --- a/drivers/gpu/drm/bridge/ite-it6505.c
> > +++ b/drivers/gpu/drm/bridge/ite-it6505.c
> > @@ -423,6 +423,7 @@ struct it6505 {
> >       struct extcon_dev *extcon;
> >       struct work_struct extcon_wq;
> >       int extcon_state;
> > +     struct drm_connector *connector;
> >       enum drm_connector_status connector_status;
> >       enum link_train_status link_state;
> >       struct work_struct link_works;
> > @@ -2159,9 +2160,6 @@ static void it6505_link_train_ok(struct it6505 *it6505)
> >               DRM_DEV_DEBUG_DRIVER(dev, "Enable audio!");
> >               it6505_enable_audio(it6505);
> >       }
> > -
> > -     if (it6505->hdcp_desired)
> > -             it6505_start_hdcp(it6505);
> >  }
> >
> >  static void it6505_link_step_train_process(struct it6505 *it6505)
> > @@ -2399,6 +2397,14 @@ static void it6505_irq_hdcp_done(struct it6505 *it6505)
> >
> >       DRM_DEV_DEBUG_DRIVER(dev, "hdcp done interrupt");
> >       it6505->hdcp_status = HDCP_AUTH_DONE;
> > +     if (it6505->connector) {
> > +             struct drm_device *drm_dev = it6505->connector->dev;
> > +
> > +             drm_modeset_lock(&drm_dev->mode_config.connection_mutex, NULL);
> > +             drm_hdcp_update_content_protection(it6505->connector,
> > +                                                DRM_MODE_CONTENT_PROTECTION_ENABLED);
> > +             drm_modeset_unlock(&drm_dev->mode_config.connection_mutex);
> > +     }
> >       it6505_show_hdcp_info(it6505);
> >  }
> >
> > @@ -2931,6 +2937,7 @@ static void it6505_bridge_atomic_enable(struct drm_bridge *bridge,
> >       if (WARN_ON(!connector))
> >               return;
> >
> > +     it6505->connector = connector;
> >       conn_state = drm_atomic_get_new_connector_state(state, connector);
> >
> >       if (WARN_ON(!conn_state))
> > @@ -2974,6 +2981,7 @@ static void it6505_bridge_atomic_disable(struct drm_bridge *bridge,
> >
> >       DRM_DEV_DEBUG_DRIVER(dev, "start");
> >
> > +     it6505->connector = NULL;
> >       if (it6505->powered) {
> >               it6505_drm_dp_link_set_power(&it6505->aux, &it6505->link,
> >                                            DP_SET_POWER_D3);
> > @@ -3028,6 +3036,50 @@ static struct edid *it6505_bridge_get_edid(struct drm_bridge *bridge,
> >       return edid;
> >  }
> >
> > +static int it6505_connector_atomic_check(struct it6505 *it6505,
> > +                                      struct drm_connector_state *state)
> > +{
> > +     struct device *dev = &it6505->client->dev;
> > +     int cp = state->content_protection;
> > +
> > +     DRM_DEV_DEBUG_DRIVER(dev, "hdcp connector state:%d, curr hdcp state:%d",
> > +                          cp, it6505->hdcp_status);
> > +
> > +     if (!it6505->hdcp_desired) {
> > +             DRM_DEV_DEBUG_DRIVER(dev, "sink not support hdcp");
> > +             return 0;
> > +     }
> > +
> > +     if (it6505->hdcp_status == HDCP_AUTH_GOING)
> > +             return -EINVAL;
> > +
> > +     if (cp == DRM_MODE_CONTENT_PROTECTION_UNDESIRED) {
> > +             if (it6505->hdcp_status == HDCP_AUTH_DONE)
> > +                     it6505_stop_hdcp(it6505);
>
> You shouldn't touch the hardware in atomic_check, this should be done in the
> commit.
>
Since it6505 is a bridge, do you have suggested callbacks in
drm_bridge_funcs similar to commit?

> > +     } else if (cp == DRM_MODE_CONTENT_PROTECTION_DESIRED) {
> > +             if (it6505->hdcp_status == HDCP_AUTH_IDLE &&
> > +                 it6505->link_state == LINK_OK)
> > +                     it6505_start_hdcp(it6505);
>
> Same here
>
> > +     } else {
> > +             if (it6505->hdcp_status == HDCP_AUTH_IDLE) {
> > +                     DRM_DEV_DEBUG_DRIVER(dev, "invalid to set hdcp enabled");
> > +                     return -EINVAL;
> > +             }
> > +     }
>
> In general, I think there are a number of locking and state issues with this
> approach. I had pulled all of this logic into a set of helpers [1], but the
> patchset has gotten stale on the list. You might consider looking at patches 1-4
> to see how the state and locking should be handled to avoid races.
>

After checking the series, all the hdcp controls are done in dp
controller instead of bridges. Is it proper for the bridge to update
the HDCP state?
- If it's not, then this series might be going in the wrong direction.
- If it is, since the current it6505 enables HDCP in the hardware
(it6505_start_hdcp() is called ) all the time, can we just update the
state without turning it off?


> Sean
>
> [1] - https://lore.kernel.org/all/20220411204741.1074308-1-sean@poorly.run/
>
> > +
> > +     return 0;
> > +}
> > +
> > +static int it6505_bridge_atomic_check(struct drm_bridge *bridge,
> > +                                   struct drm_bridge_state *bridge_state,
> > +                                   struct drm_crtc_state *crtc_state,
> > +                                   struct drm_connector_state *conn_state)
> > +{
> > +     struct it6505 *it6505 = bridge_to_it6505(bridge);
> > +
> > +     return it6505_connector_atomic_check(it6505, conn_state);
> > +}
> > +
> >  static const struct drm_bridge_funcs it6505_bridge_funcs = {
> >       .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
> >       .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
> > @@ -3035,6 +3087,7 @@ static const struct drm_bridge_funcs it6505_bridge_funcs = {
> >       .attach = it6505_bridge_attach,
> >       .detach = it6505_bridge_detach,
> >       .mode_valid = it6505_bridge_mode_valid,
> > +     .atomic_check = it6505_bridge_atomic_check,
> >       .atomic_enable = it6505_bridge_atomic_enable,
> >       .atomic_disable = it6505_bridge_atomic_disable,
> >       .atomic_pre_enable = it6505_bridge_atomic_pre_enable,
> > @@ -3354,6 +3407,7 @@ static int it6505_i2c_probe(struct i2c_client *client,
> >       it6505->bridge.type = DRM_MODE_CONNECTOR_DisplayPort;
> >       it6505->bridge.ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID |
> >                            DRM_BRIDGE_OP_HPD;
> > +     it6505->bridge.support_hdcp = true;
> >       drm_bridge_add(&it6505->bridge);
> >
> >       return 0;
> > --
> > 2.38.1.584.g0f3c55d4c2-goog
> >
>
> --
> Sean Paul, Software Engineer, Google / Chromium OS
diff mbox series

Patch

diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
index 21a9b8422bda..93626698c31e 100644
--- a/drivers/gpu/drm/bridge/ite-it6505.c
+++ b/drivers/gpu/drm/bridge/ite-it6505.c
@@ -423,6 +423,7 @@  struct it6505 {
 	struct extcon_dev *extcon;
 	struct work_struct extcon_wq;
 	int extcon_state;
+	struct drm_connector *connector;
 	enum drm_connector_status connector_status;
 	enum link_train_status link_state;
 	struct work_struct link_works;
@@ -2159,9 +2160,6 @@  static void it6505_link_train_ok(struct it6505 *it6505)
 		DRM_DEV_DEBUG_DRIVER(dev, "Enable audio!");
 		it6505_enable_audio(it6505);
 	}
-
-	if (it6505->hdcp_desired)
-		it6505_start_hdcp(it6505);
 }
 
 static void it6505_link_step_train_process(struct it6505 *it6505)
@@ -2399,6 +2397,14 @@  static void it6505_irq_hdcp_done(struct it6505 *it6505)
 
 	DRM_DEV_DEBUG_DRIVER(dev, "hdcp done interrupt");
 	it6505->hdcp_status = HDCP_AUTH_DONE;
+	if (it6505->connector) {
+		struct drm_device *drm_dev = it6505->connector->dev;
+
+		drm_modeset_lock(&drm_dev->mode_config.connection_mutex, NULL);
+		drm_hdcp_update_content_protection(it6505->connector,
+						   DRM_MODE_CONTENT_PROTECTION_ENABLED);
+		drm_modeset_unlock(&drm_dev->mode_config.connection_mutex);
+	}
 	it6505_show_hdcp_info(it6505);
 }
 
@@ -2931,6 +2937,7 @@  static void it6505_bridge_atomic_enable(struct drm_bridge *bridge,
 	if (WARN_ON(!connector))
 		return;
 
+	it6505->connector = connector;
 	conn_state = drm_atomic_get_new_connector_state(state, connector);
 
 	if (WARN_ON(!conn_state))
@@ -2974,6 +2981,7 @@  static void it6505_bridge_atomic_disable(struct drm_bridge *bridge,
 
 	DRM_DEV_DEBUG_DRIVER(dev, "start");
 
+	it6505->connector = NULL;
 	if (it6505->powered) {
 		it6505_drm_dp_link_set_power(&it6505->aux, &it6505->link,
 					     DP_SET_POWER_D3);
@@ -3028,6 +3036,50 @@  static struct edid *it6505_bridge_get_edid(struct drm_bridge *bridge,
 	return edid;
 }
 
+static int it6505_connector_atomic_check(struct it6505 *it6505,
+					 struct drm_connector_state *state)
+{
+	struct device *dev = &it6505->client->dev;
+	int cp = state->content_protection;
+
+	DRM_DEV_DEBUG_DRIVER(dev, "hdcp connector state:%d, curr hdcp state:%d",
+			     cp, it6505->hdcp_status);
+
+	if (!it6505->hdcp_desired) {
+		DRM_DEV_DEBUG_DRIVER(dev, "sink not support hdcp");
+		return 0;
+	}
+
+	if (it6505->hdcp_status == HDCP_AUTH_GOING)
+		return -EINVAL;
+
+	if (cp == DRM_MODE_CONTENT_PROTECTION_UNDESIRED) {
+		if (it6505->hdcp_status == HDCP_AUTH_DONE)
+			it6505_stop_hdcp(it6505);
+	} else if (cp == DRM_MODE_CONTENT_PROTECTION_DESIRED) {
+		if (it6505->hdcp_status == HDCP_AUTH_IDLE &&
+		    it6505->link_state == LINK_OK)
+			it6505_start_hdcp(it6505);
+	} else {
+		if (it6505->hdcp_status == HDCP_AUTH_IDLE) {
+			DRM_DEV_DEBUG_DRIVER(dev, "invalid to set hdcp enabled");
+			return -EINVAL;
+		}
+	}
+
+	return 0;
+}
+
+static int it6505_bridge_atomic_check(struct drm_bridge *bridge,
+				      struct drm_bridge_state *bridge_state,
+				      struct drm_crtc_state *crtc_state,
+				      struct drm_connector_state *conn_state)
+{
+	struct it6505 *it6505 = bridge_to_it6505(bridge);
+
+	return it6505_connector_atomic_check(it6505, conn_state);
+}
+
 static const struct drm_bridge_funcs it6505_bridge_funcs = {
 	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
 	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
@@ -3035,6 +3087,7 @@  static const struct drm_bridge_funcs it6505_bridge_funcs = {
 	.attach = it6505_bridge_attach,
 	.detach = it6505_bridge_detach,
 	.mode_valid = it6505_bridge_mode_valid,
+	.atomic_check = it6505_bridge_atomic_check,
 	.atomic_enable = it6505_bridge_atomic_enable,
 	.atomic_disable = it6505_bridge_atomic_disable,
 	.atomic_pre_enable = it6505_bridge_atomic_pre_enable,
@@ -3354,6 +3407,7 @@  static int it6505_i2c_probe(struct i2c_client *client,
 	it6505->bridge.type = DRM_MODE_CONNECTOR_DisplayPort;
 	it6505->bridge.ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID |
 			     DRM_BRIDGE_OP_HPD;
+	it6505->bridge.support_hdcp = true;
 	drm_bridge_add(&it6505->bridge);
 
 	return 0;