On Thu, Nov 24, 2022 at 4:16 AM Sean Paul <sean@xxxxxxxxxx> 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@xxxxxxxxxxxx>
> > Reviewed-by: allen chen <allen.chen@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxx/
>
> > +
> > + 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
