[PATCH v2 0/3] Clean up it6505 driver and improve synchronization

[PATCH v2 0/3] Clean up it6505 driver and improve synchronization

[Pin-yen Lin]

The main purpose of this series is to improve the synchronizations of
it6505 driver. The first two patches are required for the third one, but
they alone can be clean ups to the driver.

Changes in v2:
- Remove redundant spaces in it6505_detect
- Read sink count in it6505_irq_hpd
- Add the empty line back

Pin-yen Lin (3):
  drm/bridge: it6505: Initialize AUX channel in it6505_i2c_probe
  drm/bridge: it6505: Setup links in it6505_irq_hpd
  drm/bridge: it6505: Improve synchronization between extcon subsystem

 drivers/gpu/drm/bridge/ite-it6505.c | 106 +++++++++++++---------------
 1 file changed, 51 insertions(+), 55 deletions(-)

-- 
2.38.0.rc1.362.ged0d419d3c-goog

[PATCH v2 1/3] drm/bridge: it6505: Initialize AUX channel in it6505_i2c_probe

[Pin-yen Lin]

During device boot, the HPD interrupt could be triggered before the DRM
subsystem registers it6505 as a DRM bridge. In such cases, the driver
tries to access AUX channel and causes NULL pointer dereference.
Initializing the AUX channel earlier to prevent such error.

Fixes: b5c84a9edcd4 ("drm/bridge: add it6505 driver")
Signed-off-by: Pin-yen Lin <treapking@chromium.org>
---

(no changes since v1)

 drivers/gpu/drm/bridge/ite-it6505.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
index a4302492cf8d..f7f6c3e20fae 100644
--- a/drivers/gpu/drm/bridge/ite-it6505.c
+++ b/drivers/gpu/drm/bridge/ite-it6505.c
@@ -2871,10 +2871,7 @@ static int it6505_bridge_attach(struct drm_bridge *bridge,
 	}
 
 	/* Register aux channel */
-	it6505->aux.name = "DP-AUX";
-	it6505->aux.dev = dev;
 	it6505->aux.drm_dev = bridge->dev;
-	it6505->aux.transfer = it6505_aux_transfer;
 
 	ret = drm_dp_aux_register(&it6505->aux);
 
@@ -3354,6 +3351,11 @@ static int it6505_i2c_probe(struct i2c_client *client,
 	debugfs_init(it6505);
 	pm_runtime_enable(dev);
 
+	it6505->aux.name = "DP-AUX";
+	it6505->aux.dev = dev;
+	it6505->aux.transfer = it6505_aux_transfer;
+	drm_dp_aux_init(&it6505->aux);
+
 	it6505->bridge.funcs = &it6505_bridge_funcs;
 	it6505->bridge.type = DRM_MODE_CONNECTOR_DisplayPort;
 	it6505->bridge.ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID |
-- 
2.38.0.rc1.362.ged0d419d3c-goog

[PATCH v2 2/3] drm/bridge: it6505: Setup links in it6505_irq_hpd

[Pin-yen Lin]

Move the DPCD read and link setup steps to HPD IRQ handler to remove
an unnecessary dependency between .detect callback and the HPD IRQ
handler before registering it6505 as a DRM bridge. This is safe because
there is always a .detect call after each HPD IRQ handler triggered by
the drm_helper_hpd_irq_event call.

Signed-off-by: Pin-yen Lin <treapking@chromium.org>
---

Changes in v2:
- Remove redundant spaces in it6505_detect
- Read sink count in it6505_irq_hpd

 drivers/gpu/drm/bridge/ite-it6505.c | 80 +++++++++++++----------------
 1 file changed, 35 insertions(+), 45 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
index f7f6c3e20fae..4b6061272599 100644
--- a/drivers/gpu/drm/bridge/ite-it6505.c
+++ b/drivers/gpu/drm/bridge/ite-it6505.c
@@ -725,28 +725,6 @@ static void it6505_calc_video_info(struct it6505 *it6505)
 			     DRM_MODE_ARG(&it6505->video_info));
 }
 
-static int it6505_drm_dp_link_probe(struct drm_dp_aux *aux,
-				    struct it6505_drm_dp_link *link)
-{
-	u8 values[3];
-	int err;
-
-	memset(link, 0, sizeof(*link));
-
-	err = drm_dp_dpcd_read(aux, DP_DPCD_REV, values, sizeof(values));
-	if (err < 0)
-		return err;
-
-	link->revision = values[0];
-	link->rate = drm_dp_bw_code_to_link_rate(values[1]);
-	link->num_lanes = values[2] & DP_MAX_LANE_COUNT_MASK;
-
-	if (values[2] & DP_ENHANCED_FRAME_CAP)
-		link->capabilities = DP_ENHANCED_FRAME_CAP;
-
-	return 0;
-}
-
 static int it6505_drm_dp_link_set_power(struct drm_dp_aux *aux,
 					struct it6505_drm_dp_link *link,
 					u8 mode)
@@ -1456,11 +1434,19 @@ static void it6505_parse_link_capabilities(struct it6505 *it6505)
 	int bcaps;
 
 	if (it6505->dpcd[0] == 0) {
-		it6505_aux_on(it6505);
-		it6505_get_dpcd(it6505, DP_DPCD_REV, it6505->dpcd,
-				ARRAY_SIZE(it6505->dpcd));
+		dev_err(dev, "DPCD is not initialized");
+		return;
 	}
 
+	memset(link, 0, sizeof(*link));
+
+	link->revision = it6505->dpcd[0];
+	link->rate = drm_dp_bw_code_to_link_rate(it6505->dpcd[1]);
+	link->num_lanes = it6505->dpcd[2] & DP_MAX_LANE_COUNT_MASK;
+
+	if (it6505->dpcd[2] & DP_ENHANCED_FRAME_CAP)
+		link->capabilities = DP_ENHANCED_FRAME_CAP;
+
 	DRM_DEV_DEBUG_DRIVER(dev, "DPCD Rev.: %d.%d",
 			     link->revision >> 4, link->revision & 0x0F);
 
@@ -2323,19 +2309,32 @@ static int it6505_process_hpd_irq(struct it6505 *it6505)
 static void it6505_irq_hpd(struct it6505 *it6505)
 {
 	struct device *dev = &it6505->client->dev;
+	int dp_sink_count;
 
 	it6505->hpd_state = it6505_get_sink_hpd_status(it6505);
 	DRM_DEV_DEBUG_DRIVER(dev, "hpd change interrupt, change to %s",
 			     it6505->hpd_state ? "high" : "low");
 
-	if (it6505->bridge.dev)
-		drm_helper_hpd_irq_event(it6505->bridge.dev);
-	DRM_DEV_DEBUG_DRIVER(dev, "it6505->sink_count: %d",
-			     it6505->sink_count);
-
 	if (it6505->hpd_state) {
 		wait_for_completion_timeout(&it6505->wait_edid_complete,
 					    msecs_to_jiffies(6000));
+		it6505_aux_on(it6505);
+		if (it6505->dpcd[0] == 0) {
+			it6505_get_dpcd(it6505, DP_DPCD_REV, it6505->dpcd,
+					ARRAY_SIZE(it6505->dpcd));
+			it6505_variable_config(it6505);
+			it6505_parse_link_capabilities(it6505);
+		}
+		it6505->auto_train_retry = AUTO_TRAIN_RETRY;
+
+		it6505_drm_dp_link_set_power(&it6505->aux, &it6505->link,
+					     DP_SET_POWER_D0);
+		dp_sink_count = it6505_dpcd_read(it6505, DP_SINK_COUNT);
+		it6505->sink_count = DP_GET_SINK_COUNT(dp_sink_count);
+
+		DRM_DEV_DEBUG_DRIVER(dev, "it6505->sink_count: %d",
+				     it6505->sink_count);
+
 		it6505_lane_termination_on(it6505);
 		it6505_lane_power_on(it6505);
 
@@ -2363,6 +2362,9 @@ static void it6505_irq_hpd(struct it6505 *it6505)
 		it6505_lane_off(it6505);
 		it6505_link_reset_step_train(it6505);
 	}
+
+	if (it6505->bridge.dev)
+		drm_helper_hpd_irq_event(it6505->bridge.dev);
 }
 
 static void it6505_irq_hpd_irq(struct it6505 *it6505)
@@ -2625,26 +2627,14 @@ static enum drm_connector_status it6505_detect(struct it6505 *it6505)
 		goto unlock;
 
 	if (it6505->enable_drv_hold) {
-		status = it6505_get_sink_hpd_status(it6505) ?
-					connector_status_connected :
-					connector_status_disconnected;
+		status = it6505->hpd_state ? connector_status_connected :
+					     connector_status_disconnected;
 		goto unlock;
 	}
 
-	if (it6505_get_sink_hpd_status(it6505)) {
-		it6505_aux_on(it6505);
-		it6505_drm_dp_link_probe(&it6505->aux, &it6505->link);
+	if (it6505->hpd_state) {
 		it6505_drm_dp_link_set_power(&it6505->aux, &it6505->link,
 					     DP_SET_POWER_D0);
-		it6505->auto_train_retry = AUTO_TRAIN_RETRY;
-
-		if (it6505->dpcd[0] == 0) {
-			it6505_get_dpcd(it6505, DP_DPCD_REV, it6505->dpcd,
-					ARRAY_SIZE(it6505->dpcd));
-			it6505_variable_config(it6505);
-			it6505_parse_link_capabilities(it6505);
-		}
-
 		dp_sink_count = it6505_dpcd_read(it6505, DP_SINK_COUNT);
 		it6505->sink_count = DP_GET_SINK_COUNT(dp_sink_count);
 		DRM_DEV_DEBUG_DRIVER(dev, "it6505->sink_count:%d branch:%d",
-- 
2.38.0.rc1.362.ged0d419d3c-goog

[PATCH v2 3/3] drm/bridge: it6505: Improve synchronization between extcon subsystem

[Pin-yen Lin]

Originally, the it6505 relies on a short sleep in the IRQ handler and a
long sleep to make sure it6505->lane_swap and it6505->lane_count is
configured in it6505_extcon_work and it6505_detect, respectively.

Use completion and additional DPCD read to remove the unnecessary waits,
and use a different lock for it6505_extcon_work and the threaded IRQ
handler because they no longer need to run exclusively.

The wait time of the completion is usually less than 10ms in local
experiments, but leave it larger here just in case.

Signed-off-by: Pin-yen Lin <treapking@chromium.org>
---

Changes in v2:
- Add the empty line back

 drivers/gpu/drm/bridge/ite-it6505.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
index 4b6061272599..0de44c651c60 100644
--- a/drivers/gpu/drm/bridge/ite-it6505.c
+++ b/drivers/gpu/drm/bridge/ite-it6505.c
@@ -412,6 +412,7 @@ struct it6505 {
 	 * Mutex protects extcon and interrupt functions from interfering
 	 * each other.
 	 */
+	struct mutex irq_lock;
 	struct mutex extcon_lock;
 	struct mutex mode_lock; /* used to bridge_detect */
 	struct mutex aux_lock; /* used to aux data transfers */
@@ -440,7 +441,7 @@ struct it6505 {
 	enum hdcp_state hdcp_status;
 	struct delayed_work hdcp_work;
 	struct work_struct hdcp_wait_ksv_list;
-	struct completion wait_edid_complete;
+	struct completion extcon_completion;
 	u8 auto_train_retry;
 	bool hdcp_desired;
 	bool is_repeater;
@@ -2316,8 +2317,8 @@ static void it6505_irq_hpd(struct it6505 *it6505)
 			     it6505->hpd_state ? "high" : "low");
 
 	if (it6505->hpd_state) {
-		wait_for_completion_timeout(&it6505->wait_edid_complete,
-					    msecs_to_jiffies(6000));
+		wait_for_completion_timeout(&it6505->extcon_completion,
+					    msecs_to_jiffies(1000));
 		it6505_aux_on(it6505);
 		if (it6505->dpcd[0] == 0) {
 			it6505_get_dpcd(it6505, DP_DPCD_REV, it6505->dpcd,
@@ -2493,8 +2494,7 @@ static irqreturn_t it6505_int_threaded_handler(int unused, void *data)
 	};
 	int int_status[3], i;
 
-	msleep(100);
-	mutex_lock(&it6505->extcon_lock);
+	mutex_lock(&it6505->irq_lock);
 
 	if (it6505->enable_drv_hold || !it6505->powered)
 		goto unlock;
@@ -2524,7 +2524,7 @@ static irqreturn_t it6505_int_threaded_handler(int unused, void *data)
 	}
 
 unlock:
-	mutex_unlock(&it6505->extcon_lock);
+	mutex_unlock(&it6505->irq_lock);
 
 	return IRQ_HANDLED;
 }
@@ -2701,9 +2701,12 @@ static void it6505_extcon_work(struct work_struct *work)
 		 */
 		if (ret)
 			it6505_poweron(it6505);
+
+		complete_all(&it6505->extcon_completion);
 	} else {
 		DRM_DEV_DEBUG_DRIVER(dev, "start to power off");
 		pm_runtime_put_sync(dev);
+		reinit_completion(&it6505->extcon_completion);
 
 		drm_helper_hpd_irq_event(it6505->bridge.dev);
 		memset(it6505->dpcd, 0, sizeof(it6505->dpcd));
@@ -3274,6 +3277,7 @@ static int it6505_i2c_probe(struct i2c_client *client,
 	if (!it6505)
 		return -ENOMEM;
 
+	mutex_init(&it6505->irq_lock);
 	mutex_init(&it6505->extcon_lock);
 	mutex_init(&it6505->mode_lock);
 	mutex_init(&it6505->aux_lock);
@@ -3329,7 +3333,7 @@ static int it6505_i2c_probe(struct i2c_client *client,
 	INIT_WORK(&it6505->link_works, it6505_link_training_work);
 	INIT_WORK(&it6505->hdcp_wait_ksv_list, it6505_hdcp_wait_ksv_list);
 	INIT_DELAYED_WORK(&it6505->hdcp_work, it6505_hdcp_work);
-	init_completion(&it6505->wait_edid_complete);
+	init_completion(&it6505->extcon_completion);
 	memset(it6505->dpcd, 0, sizeof(it6505->dpcd));
 	it6505->powered = false;
 	it6505->enable_drv_hold = DEFAULT_DRV_HOLD;
-- 
2.38.0.rc1.362.ged0d419d3c-goog

Re: [PATCH v2 1/3] drm/bridge: it6505: Initialize AUX channel in it6505_i2c_probe

[Robert Foss]

On Thu, 13 Oct 2022 at 13:04, Pin-yen Lin <treapking@chromium.org> wrote:
>
> During device boot, the HPD interrupt could be triggered before the DRM
> subsystem registers it6505 as a DRM bridge. In such cases, the driver
> tries to access AUX channel and causes NULL pointer dereference.
> Initializing the AUX channel earlier to prevent such error.
>
> Fixes: b5c84a9edcd4 ("drm/bridge: add it6505 driver")
> Signed-off-by: Pin-yen Lin <treapking@chromium.org>
> ---
>
> (no changes since v1)
>
>  drivers/gpu/drm/bridge/ite-it6505.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
> index a4302492cf8d..f7f6c3e20fae 100644
> --- a/drivers/gpu/drm/bridge/ite-it6505.c
> +++ b/drivers/gpu/drm/bridge/ite-it6505.c
> @@ -2871,10 +2871,7 @@ static int it6505_bridge_attach(struct drm_bridge *bridge,
>         }
>
>         /* Register aux channel */
> -       it6505->aux.name = "DP-AUX";
> -       it6505->aux.dev = dev;
>         it6505->aux.drm_dev = bridge->dev;
> -       it6505->aux.transfer = it6505_aux_transfer;
>
>         ret = drm_dp_aux_register(&it6505->aux);
>
> @@ -3354,6 +3351,11 @@ static int it6505_i2c_probe(struct i2c_client *client,
>         debugfs_init(it6505);
>         pm_runtime_enable(dev);
>
> +       it6505->aux.name = "DP-AUX";
> +       it6505->aux.dev = dev;
> +       it6505->aux.transfer = it6505_aux_transfer;
> +       drm_dp_aux_init(&it6505->aux);
> +
>         it6505->bridge.funcs = &it6505_bridge_funcs;
>         it6505->bridge.type = DRM_MODE_CONNECTOR_DisplayPort;
>         it6505->bridge.ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID |
> --
> 2.38.0.rc1.362.ged0d419d3c-goog
>

Reviewed-by: Robert Foss <robert.foss@linaro.org>

Re: [PATCH v2 2/3] drm/bridge: it6505: Setup links in it6505_irq_hpd

[Robert Foss]

On Thu, 13 Oct 2022 at 13:04, Pin-yen Lin <treapking@chromium.org> wrote:
>
> Move the DPCD read and link setup steps to HPD IRQ handler to remove
> an unnecessary dependency between .detect callback and the HPD IRQ
> handler before registering it6505 as a DRM bridge. This is safe because
> there is always a .detect call after each HPD IRQ handler triggered by
> the drm_helper_hpd_irq_event call.
>
> Signed-off-by: Pin-yen Lin <treapking@chromium.org>
> ---
>
> Changes in v2:
> - Remove redundant spaces in it6505_detect
> - Read sink count in it6505_irq_hpd
>
>  drivers/gpu/drm/bridge/ite-it6505.c | 80 +++++++++++++----------------
>  1 file changed, 35 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
> index f7f6c3e20fae..4b6061272599 100644
> --- a/drivers/gpu/drm/bridge/ite-it6505.c
> +++ b/drivers/gpu/drm/bridge/ite-it6505.c
> @@ -725,28 +725,6 @@ static void it6505_calc_video_info(struct it6505 *it6505)
>                              DRM_MODE_ARG(&it6505->video_info));
>  }
>
> -static int it6505_drm_dp_link_probe(struct drm_dp_aux *aux,
> -                                   struct it6505_drm_dp_link *link)
> -{
> -       u8 values[3];
> -       int err;
> -
> -       memset(link, 0, sizeof(*link));
> -
> -       err = drm_dp_dpcd_read(aux, DP_DPCD_REV, values, sizeof(values));
> -       if (err < 0)
> -               return err;
> -
> -       link->revision = values[0];
> -       link->rate = drm_dp_bw_code_to_link_rate(values[1]);
> -       link->num_lanes = values[2] & DP_MAX_LANE_COUNT_MASK;
> -
> -       if (values[2] & DP_ENHANCED_FRAME_CAP)
> -               link->capabilities = DP_ENHANCED_FRAME_CAP;
> -
> -       return 0;
> -}
> -
>  static int it6505_drm_dp_link_set_power(struct drm_dp_aux *aux,
>                                         struct it6505_drm_dp_link *link,
>                                         u8 mode)
> @@ -1456,11 +1434,19 @@ static void it6505_parse_link_capabilities(struct it6505 *it6505)
>         int bcaps;
>
>         if (it6505->dpcd[0] == 0) {
> -               it6505_aux_on(it6505);
> -               it6505_get_dpcd(it6505, DP_DPCD_REV, it6505->dpcd,
> -                               ARRAY_SIZE(it6505->dpcd));
> +               dev_err(dev, "DPCD is not initialized");
> +               return;
>         }
>
> +       memset(link, 0, sizeof(*link));
> +
> +       link->revision = it6505->dpcd[0];
> +       link->rate = drm_dp_bw_code_to_link_rate(it6505->dpcd[1]);
> +       link->num_lanes = it6505->dpcd[2] & DP_MAX_LANE_COUNT_MASK;
> +
> +       if (it6505->dpcd[2] & DP_ENHANCED_FRAME_CAP)
> +               link->capabilities = DP_ENHANCED_FRAME_CAP;
> +
>         DRM_DEV_DEBUG_DRIVER(dev, "DPCD Rev.: %d.%d",
>                              link->revision >> 4, link->revision & 0x0F);
>
> @@ -2323,19 +2309,32 @@ static int it6505_process_hpd_irq(struct it6505 *it6505)
>  static void it6505_irq_hpd(struct it6505 *it6505)
>  {
>         struct device *dev = &it6505->client->dev;
> +       int dp_sink_count;
>
>         it6505->hpd_state = it6505_get_sink_hpd_status(it6505);
>         DRM_DEV_DEBUG_DRIVER(dev, "hpd change interrupt, change to %s",
>                              it6505->hpd_state ? "high" : "low");
>
> -       if (it6505->bridge.dev)
> -               drm_helper_hpd_irq_event(it6505->bridge.dev);
> -       DRM_DEV_DEBUG_DRIVER(dev, "it6505->sink_count: %d",
> -                            it6505->sink_count);
> -
>         if (it6505->hpd_state) {
>                 wait_for_completion_timeout(&it6505->wait_edid_complete,
>                                             msecs_to_jiffies(6000));
> +               it6505_aux_on(it6505);
> +               if (it6505->dpcd[0] == 0) {
> +                       it6505_get_dpcd(it6505, DP_DPCD_REV, it6505->dpcd,
> +                                       ARRAY_SIZE(it6505->dpcd));
> +                       it6505_variable_config(it6505);
> +                       it6505_parse_link_capabilities(it6505);
> +               }
> +               it6505->auto_train_retry = AUTO_TRAIN_RETRY;
> +
> +               it6505_drm_dp_link_set_power(&it6505->aux, &it6505->link,
> +                                            DP_SET_POWER_D0);
> +               dp_sink_count = it6505_dpcd_read(it6505, DP_SINK_COUNT);
> +               it6505->sink_count = DP_GET_SINK_COUNT(dp_sink_count);
> +
> +               DRM_DEV_DEBUG_DRIVER(dev, "it6505->sink_count: %d",
> +                                    it6505->sink_count);
> +
>                 it6505_lane_termination_on(it6505);
>                 it6505_lane_power_on(it6505);
>
> @@ -2363,6 +2362,9 @@ static void it6505_irq_hpd(struct it6505 *it6505)
>                 it6505_lane_off(it6505);
>                 it6505_link_reset_step_train(it6505);
>         }
> +
> +       if (it6505->bridge.dev)
> +               drm_helper_hpd_irq_event(it6505->bridge.dev);
>  }
>
>  static void it6505_irq_hpd_irq(struct it6505 *it6505)
> @@ -2625,26 +2627,14 @@ static enum drm_connector_status it6505_detect(struct it6505 *it6505)
>                 goto unlock;
>
>         if (it6505->enable_drv_hold) {
> -               status = it6505_get_sink_hpd_status(it6505) ?
> -                                       connector_status_connected :
> -                                       connector_status_disconnected;
> +               status = it6505->hpd_state ? connector_status_connected :
> +                                            connector_status_disconnected;
>                 goto unlock;
>         }
>
> -       if (it6505_get_sink_hpd_status(it6505)) {
> -               it6505_aux_on(it6505);
> -               it6505_drm_dp_link_probe(&it6505->aux, &it6505->link);
> +       if (it6505->hpd_state) {
>                 it6505_drm_dp_link_set_power(&it6505->aux, &it6505->link,
>                                              DP_SET_POWER_D0);
> -               it6505->auto_train_retry = AUTO_TRAIN_RETRY;
> -
> -               if (it6505->dpcd[0] == 0) {
> -                       it6505_get_dpcd(it6505, DP_DPCD_REV, it6505->dpcd,
> -                                       ARRAY_SIZE(it6505->dpcd));
> -                       it6505_variable_config(it6505);
> -                       it6505_parse_link_capabilities(it6505);
> -               }
> -
>                 dp_sink_count = it6505_dpcd_read(it6505, DP_SINK_COUNT);
>                 it6505->sink_count = DP_GET_SINK_COUNT(dp_sink_count);
>                 DRM_DEV_DEBUG_DRIVER(dev, "it6505->sink_count:%d branch:%d",
> --
> 2.38.0.rc1.362.ged0d419d3c-goog
>

Reviewed-by: Robert Foss <robert.foss@linaro.org>

Re: [PATCH v2 3/3] drm/bridge: it6505: Improve synchronization between extcon subsystem

[Robert Foss]

On Thu, 13 Oct 2022 at 13:04, Pin-yen Lin <treapking@chromium.org> wrote:
>
> Originally, the it6505 relies on a short sleep in the IRQ handler and a
> long sleep to make sure it6505->lane_swap and it6505->lane_count is
> configured in it6505_extcon_work and it6505_detect, respectively.
>
> Use completion and additional DPCD read to remove the unnecessary waits,
> and use a different lock for it6505_extcon_work and the threaded IRQ
> handler because they no longer need to run exclusively.
>
> The wait time of the completion is usually less than 10ms in local
> experiments, but leave it larger here just in case.
>
> Signed-off-by: Pin-yen Lin <treapking@chromium.org>
> ---
>
> Changes in v2:
> - Add the empty line back
>
>  drivers/gpu/drm/bridge/ite-it6505.c | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
> index 4b6061272599..0de44c651c60 100644
> --- a/drivers/gpu/drm/bridge/ite-it6505.c
> +++ b/drivers/gpu/drm/bridge/ite-it6505.c
> @@ -412,6 +412,7 @@ struct it6505 {
>          * Mutex protects extcon and interrupt functions from interfering
>          * each other.
>          */
> +       struct mutex irq_lock;
>         struct mutex extcon_lock;
>         struct mutex mode_lock; /* used to bridge_detect */
>         struct mutex aux_lock; /* used to aux data transfers */
> @@ -440,7 +441,7 @@ struct it6505 {
>         enum hdcp_state hdcp_status;
>         struct delayed_work hdcp_work;
>         struct work_struct hdcp_wait_ksv_list;
> -       struct completion wait_edid_complete;
> +       struct completion extcon_completion;
>         u8 auto_train_retry;
>         bool hdcp_desired;
>         bool is_repeater;
> @@ -2316,8 +2317,8 @@ static void it6505_irq_hpd(struct it6505 *it6505)
>                              it6505->hpd_state ? "high" : "low");
>
>         if (it6505->hpd_state) {
> -               wait_for_completion_timeout(&it6505->wait_edid_complete,
> -                                           msecs_to_jiffies(6000));
> +               wait_for_completion_timeout(&it6505->extcon_completion,
> +                                           msecs_to_jiffies(1000));
>                 it6505_aux_on(it6505);
>                 if (it6505->dpcd[0] == 0) {
>                         it6505_get_dpcd(it6505, DP_DPCD_REV, it6505->dpcd,
> @@ -2493,8 +2494,7 @@ static irqreturn_t it6505_int_threaded_handler(int unused, void *data)
>         };
>         int int_status[3], i;
>
> -       msleep(100);
> -       mutex_lock(&it6505->extcon_lock);
> +       mutex_lock(&it6505->irq_lock);
>
>         if (it6505->enable_drv_hold || !it6505->powered)
>                 goto unlock;
> @@ -2524,7 +2524,7 @@ static irqreturn_t it6505_int_threaded_handler(int unused, void *data)
>         }
>
>  unlock:
> -       mutex_unlock(&it6505->extcon_lock);
> +       mutex_unlock(&it6505->irq_lock);
>
>         return IRQ_HANDLED;
>  }
> @@ -2701,9 +2701,12 @@ static void it6505_extcon_work(struct work_struct *work)
>                  */
>                 if (ret)
>                         it6505_poweron(it6505);
> +
> +               complete_all(&it6505->extcon_completion);
>         } else {
>                 DRM_DEV_DEBUG_DRIVER(dev, "start to power off");
>                 pm_runtime_put_sync(dev);
> +               reinit_completion(&it6505->extcon_completion);
>
>                 drm_helper_hpd_irq_event(it6505->bridge.dev);
>                 memset(it6505->dpcd, 0, sizeof(it6505->dpcd));
> @@ -3274,6 +3277,7 @@ static int it6505_i2c_probe(struct i2c_client *client,
>         if (!it6505)
>                 return -ENOMEM;
>
> +       mutex_init(&it6505->irq_lock);
>         mutex_init(&it6505->extcon_lock);
>         mutex_init(&it6505->mode_lock);
>         mutex_init(&it6505->aux_lock);
> @@ -3329,7 +3333,7 @@ static int it6505_i2c_probe(struct i2c_client *client,
>         INIT_WORK(&it6505->link_works, it6505_link_training_work);
>         INIT_WORK(&it6505->hdcp_wait_ksv_list, it6505_hdcp_wait_ksv_list);
>         INIT_DELAYED_WORK(&it6505->hdcp_work, it6505_hdcp_work);
> -       init_completion(&it6505->wait_edid_complete);
> +       init_completion(&it6505->extcon_completion);
>         memset(it6505->dpcd, 0, sizeof(it6505->dpcd));
>         it6505->powered = false;
>         it6505->enable_drv_hold = DEFAULT_DRV_HOLD;
> --
> 2.38.0.rc1.362.ged0d419d3c-goog
>

Reviewed-by: Robert Foss <robert.foss@linaro.org>

Re: [PATCH v2 0/3] Clean up it6505 driver and improve synchronization

[Robert Foss]

On Thu, 13 Oct 2022 at 13:04, Pin-yen Lin <treapking@chromium.org> wrote:
>
> The main purpose of this series is to improve the synchronizations of
> it6505 driver. The first two patches are required for the third one, but
> they alone can be clean ups to the driver.
>
> Changes in v2:
> - Remove redundant spaces in it6505_detect
> - Read sink count in it6505_irq_hpd
> - Add the empty line back
>
> Pin-yen Lin (3):
>   drm/bridge: it6505: Initialize AUX channel in it6505_i2c_probe
>   drm/bridge: it6505: Setup links in it6505_irq_hpd
>   drm/bridge: it6505: Improve synchronization between extcon subsystem
>
>  drivers/gpu/drm/bridge/ite-it6505.c | 106 +++++++++++++---------------
>  1 file changed, 51 insertions(+), 55 deletions(-)
>
> --
> 2.38.0.rc1.362.ged0d419d3c-goog
>

Applied to drm-misc-next.