[PATCH 1/2] drm/vc4: hdmi: Take our lock to reset the link

[PATCH 1/2] drm/vc4: hdmi: Take our lock to reset the link

[maxime]

We access some fields protected by our internal mutex in
vc4_hdmi_reset_link() (saved_adjusted_mode, output_bpc, output_format)
and are calling functions that need to have that lock taken
(vc4_hdmi_supports_scrambling()).

However, the current code doesn't lock that mutex. Let's make sure it
does.

Fixes: 6bed2ea3cb38 ("drm/vc4: hdmi: Reset link on hotplug")
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_hdmi.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index 596e311d6e58..9e549fa07467 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -349,27 +349,40 @@ static int vc4_hdmi_reset_link(struct drm_connector *connector,
 	if (!crtc_state->active)
 		return 0;
 
-	if (!vc4_hdmi_supports_scrambling(encoder))
+	mutex_lock(&vc4_hdmi->mutex);
+
+	if (!vc4_hdmi_supports_scrambling(encoder)) {
+		mutex_unlock(&vc4_hdmi->mutex);
 		return 0;
+	}
 
 	scrambling_needed = vc4_hdmi_mode_needs_scrambling(&vc4_hdmi->saved_adjusted_mode,
 							   vc4_hdmi->output_bpc,
 							   vc4_hdmi->output_format);
-	if (!scrambling_needed)
+	if (!scrambling_needed) {
+		mutex_unlock(&vc4_hdmi->mutex);
 		return 0;
+	}
 
 	if (conn_state->commit &&
-	    !try_wait_for_completion(&conn_state->commit->hw_done))
+	    !try_wait_for_completion(&conn_state->commit->hw_done)) {
+		mutex_unlock(&vc4_hdmi->mutex);
 		return 0;
+	}
 
 	ret = drm_scdc_readb(connector->ddc, SCDC_TMDS_CONFIG, &config);
 	if (ret < 0) {
 		drm_err(drm, "Failed to read TMDS config: %d\n", ret);
+		mutex_unlock(&vc4_hdmi->mutex);
 		return 0;
 	}
 
-	if (!!(config & SCDC_SCRAMBLING_ENABLE) == scrambling_needed)
+	if (!!(config & SCDC_SCRAMBLING_ENABLE) == scrambling_needed) {
+		mutex_unlock(&vc4_hdmi->mutex);
 		return 0;
+	}
+
+	mutex_unlock(&vc4_hdmi->mutex);
 
 	/*
 	 * HDMI 2.0 says that one should not send scrambled data
-- 
2.37.3

[PATCH 2/2] drm/vc4: hdmi: Fix outdated function name in comment

[maxime]

A comment introduced by commit 6bed2ea3cb38 ("drm/vc4: hdmi: Reset link
on hotplug") mentions a drm_atomic_helper_connector_hdmi_reset_link()
function that was part of the earlier versions but got moved internally
and is now named vc4_hdmi_reset_link(). Let's fix the function name.

Fixes: 6bed2ea3cb38 ("drm/vc4: hdmi: Reset link on hotplug")
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_hdmi.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index 9e549fa07467..79eda8f5fea0 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -410,9 +410,8 @@ static void vc4_hdmi_handle_hotplug(struct vc4_hdmi *vc4_hdmi,
 	 * .adap_enable, which leads to that funtion being called with
 	 * our mutex held.
 	 *
-	 * A similar situation occurs with
-	 * drm_atomic_helper_connector_hdmi_reset_link() that will call
-	 * into our KMS hooks if the scrambling was enabled.
+	 * A similar situation occurs with vc4_hdmi_reset_link() that
+	 * will call into our KMS hooks if the scrambling was enabled.
 	 *
 	 * Concurrency isn't an issue at the moment since we don't share
 	 * any state with any of the other frameworks so we can ignore
-- 
2.37.3

Re: [PATCH 1/2] drm/vc4: hdmi: Take our lock to reset the link

[Javier Martinez Canillas]

Hello Maxime,

On 10/24/22 11:36, maxime@cerno.tech wrote:
> We access some fields protected by our internal mutex in
> vc4_hdmi_reset_link() (saved_adjusted_mode, output_bpc, output_format)
> and are calling functions that need to have that lock taken
> (vc4_hdmi_supports_scrambling()).
> 
> However, the current code doesn't lock that mutex. Let's make sure it
> does.
> 
> Fixes: 6bed2ea3cb38 ("drm/vc4: hdmi: Reset link on hotplug")
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

Just a trivial nit below:

>  drivers/gpu/drm/vc4/vc4_hdmi.c | 21 +++++++++++++++++----
>  1 file changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> index 596e311d6e58..9e549fa07467 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> @@ -349,27 +349,40 @@ static int vc4_hdmi_reset_link(struct drm_connector *connector,
>  	if (!crtc_state->active)
>  		return 0;
>  
> -	if (!vc4_hdmi_supports_scrambling(encoder))
> +	mutex_lock(&vc4_hdmi->mutex);
> +
> +	if (!vc4_hdmi_supports_scrambling(encoder)) {
> +		mutex_unlock(&vc4_hdmi->mutex);
>  		return 0;
> +	}
>  
>  	scrambling_needed = vc4_hdmi_mode_needs_scrambling(&vc4_hdmi->saved_adjusted_mode,
>  							   vc4_hdmi->output_bpc,
>  							   vc4_hdmi->output_format);
> -	if (!scrambling_needed)
> +	if (!scrambling_needed) {
> +		mutex_unlock(&vc4_hdmi->mutex);
>  		return 0;
> +	}
>  
>  	if (conn_state->commit &&
> -	    !try_wait_for_completion(&conn_state->commit->hw_done))
> +	    !try_wait_for_completion(&conn_state->commit->hw_done)) {
> +		mutex_unlock(&vc4_hdmi->mutex);
>  		return 0;
> +	}

I think the convention is to have a ret_unlock label followed by the
mutex_unlock() and ret 0, to avoid duplicating this. You could have
that label after the return reset_pipe(crtc, ctx).

But it's OK if you prefer to land this version too.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 2/2] drm/vc4: hdmi: Fix outdated function name in comment

[Javier Martinez Canillas]

On 10/24/22 11:36, maxime@cerno.tech wrote:
> A comment introduced by commit 6bed2ea3cb38 ("drm/vc4: hdmi: Reset link
> on hotplug") mentions a drm_atomic_helper_connector_hdmi_reset_link()
> function that was part of the earlier versions but got moved internally
> and is now named vc4_hdmi_reset_link(). Let's fix the function name.
> 
> Fixes: 6bed2ea3cb38 ("drm/vc4: hdmi: Reset link on hotplug")
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 1/2] drm/vc4: hdmi: Take our lock to reset the link

[Maxime Ripard]

On Mon, 24 Oct 2022 11:36:33 +0200, maxime@cerno.tech wrote:
> We access some fields protected by our internal mutex in
> vc4_hdmi_reset_link() (saved_adjusted_mode, output_bpc, output_format)
> and are calling functions that need to have that lock taken
> (vc4_hdmi_supports_scrambling()).
> 
> However, the current code doesn't lock that mutex. Let's make sure it
> does.
> 
> [...]

Applied to drm/drm-misc (drm-misc-fixes).

Thanks!
Maxime