[PATCH 0/3] drm/vc4: 3 minor fixes from mismatched drm_dev_[enter|exit] calls

[PATCH 0/3] drm/vc4: 3 minor fixes from mismatched drm_dev_[enter|exit] calls

[Dave Stevenson]

These are 3 fixes noted from Marek's report that he was seeing lock
held issues.
The place he had noticed was actually triggered due to an incorrect
change to a conditional, but also a further place was noted where
it was possible to leave the function without calling drm_dev_exit.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
Dave Stevenson (3):
      drm/vc4: Match drm_dev_enter and exit calls in vc4_hvs_lut_load
      drm/vc4: Match drm_dev_enter and exit calls in vc4_hvs_atomic_flush
      drm/vc4: Correct generation check in vc4_hvs_lut_load

 drivers/gpu/drm/vc4/vc4_hvs.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)
---
base-commit: 2c34a5464007c45142af009d13c668b0630ef9c0
change-id: 20241008-drm-vc4-fixes-4aceb259455c

Best regards,
-- 
Dave Stevenson <dave.stevenson@raspberrypi.com>

[PATCH 1/3] drm/vc4: Match drm_dev_enter and exit calls in vc4_hvs_lut_load

[Dave Stevenson]

Commit 52efe364d196 ("drm/vc4: hvs: Don't write gamma luts on 2711")
added a return path to vc4_hvs_lut_load that had called
drm_dev_enter, but not drm_dev_exit.

Ensure we call drm_dev_exit.

Fixes: 52efe364d196 ("drm/vc4: hvs: Don't write gamma luts on 2711")
Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
Closes: https://lore.kernel.org/dri-devel/37051126-3921-4afe-a936-5f828bff5752@samsung.com/
Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
 drivers/gpu/drm/vc4/vc4_hvs.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/vc4/vc4_hvs.c b/drivers/gpu/drm/vc4/vc4_hvs.c
index 2a366a607fcc..546ee11016b2 100644
--- a/drivers/gpu/drm/vc4/vc4_hvs.c
+++ b/drivers/gpu/drm/vc4/vc4_hvs.c
@@ -225,7 +225,7 @@ static void vc4_hvs_lut_load(struct vc4_hvs *hvs,
 		return;
 
 	if (hvs->vc4->gen == VC4_GEN_4)
-		return;
+		goto exit;
 
 	/* The LUT memory is laid out with each HVS channel in order,
 	 * each of which takes 256 writes for R, 256 for G, then 256
@@ -242,6 +242,7 @@ static void vc4_hvs_lut_load(struct vc4_hvs *hvs,
 	for (i = 0; i < crtc->gamma_size; i++)
 		HVS_WRITE(SCALER_GAMDATA, vc4_crtc->lut_b[i]);
 
+exit:
 	drm_dev_exit(idx);
 }
 

-- 
2.34.1

[PATCH 2/3] drm/vc4: Match drm_dev_enter and exit calls in vc4_hvs_atomic_flush

[Dave Stevenson]

Commit 92c17d16476c ("drm/vc4: hvs: Ignore atomic_flush if we're disabled")
added a path which returned early without having called drm_dev_exit.

Ensure all paths call drm_dev_exit.

Fixes: 92c17d16476c ("drm/vc4: hvs: Ignore atomic_flush if we're disabled")
Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
 drivers/gpu/drm/vc4/vc4_hvs.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/vc4/vc4_hvs.c b/drivers/gpu/drm/vc4/vc4_hvs.c
index 546ee11016b2..c5ebc317188a 100644
--- a/drivers/gpu/drm/vc4/vc4_hvs.c
+++ b/drivers/gpu/drm/vc4/vc4_hvs.c
@@ -603,7 +603,7 @@ void vc4_hvs_atomic_flush(struct drm_crtc *crtc,
 	}
 
 	if (vc4_state->assigned_channel == VC4_HVS_CHANNEL_DISABLED)
-		return;
+		goto exit;
 
 	if (debug_dump_regs) {
 		DRM_INFO("CRTC %d HVS before:\n", drm_crtc_index(crtc));
@@ -686,6 +686,7 @@ void vc4_hvs_atomic_flush(struct drm_crtc *crtc,
 		vc4_hvs_dump_state(hvs);
 	}
 
+exit:
 	drm_dev_exit(idx);
 }
 

-- 
2.34.1

[PATCH 3/3] drm/vc4: Correct generation check in vc4_hvs_lut_load

[Dave Stevenson]

Commit 24c5ed3ddf27 ("drm/vc4: Introduce generation number enum")
incorrectly swapped a check of hvs->vc4->is_vc5 to
hvs->vc4->gen == VC4_GEN_4 in vc4_hvs_lut_load, hence breaking
loading the gamma look up table on Pi0-3.

Correct that conditional.

Fixes: 24c5ed3ddf27 ("drm/vc4: Introduce generation number enum")
Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
 drivers/gpu/drm/vc4/vc4_hvs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/vc4/vc4_hvs.c b/drivers/gpu/drm/vc4/vc4_hvs.c
index c5ebc317188a..1edf6e3fa7e6 100644
--- a/drivers/gpu/drm/vc4/vc4_hvs.c
+++ b/drivers/gpu/drm/vc4/vc4_hvs.c
@@ -224,7 +224,7 @@ static void vc4_hvs_lut_load(struct vc4_hvs *hvs,
 	if (!drm_dev_enter(drm, &idx))
 		return;
 
-	if (hvs->vc4->gen == VC4_GEN_4)
+	if (hvs->vc4->gen != VC4_GEN_4)
 		goto exit;
 
 	/* The LUT memory is laid out with each HVS channel in order,

-- 
2.34.1

Re: [PATCH 1/3] drm/vc4: Match drm_dev_enter and exit calls in vc4_hvs_lut_load

[Maíra Canal]

Hi Dave,

On 10/8/24 13:44, Dave Stevenson wrote:
> Commit 52efe364d196 ("drm/vc4: hvs: Don't write gamma luts on 2711")
> added a return path to vc4_hvs_lut_load that had called
> drm_dev_enter, but not drm_dev_exit.
> 
> Ensure we call drm_dev_exit.
> 
> Fixes: 52efe364d196 ("drm/vc4: hvs: Don't write gamma luts on 2711")
> Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Closes: https://lore.kernel.org/dri-devel/37051126-3921-4afe-a936-5f828bff5752@samsung.com/
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> ---
>   drivers/gpu/drm/vc4/vc4_hvs.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/vc4/vc4_hvs.c b/drivers/gpu/drm/vc4/vc4_hvs.c
> index 2a366a607fcc..546ee11016b2 100644
> --- a/drivers/gpu/drm/vc4/vc4_hvs.c
> +++ b/drivers/gpu/drm/vc4/vc4_hvs.c
> @@ -225,7 +225,7 @@ static void vc4_hvs_lut_load(struct vc4_hvs *hvs,
>   		return;
>   
>   	if (hvs->vc4->gen == VC4_GEN_4)
> -		return;
> +		goto exit;

As exit only has one function, I'd probably switch `return;` to
`return drm_dev_exit(idx);` as `drm_dev_exit()` returns void.

But this isn't critical, so:

Reviewed-by: Maíra Canal <mcanal@igalia.com>

Best Regards,
- Maíra

>   
>   	/* The LUT memory is laid out with each HVS channel in order,
>   	 * each of which takes 256 writes for R, 256 for G, then 256
> @@ -242,6 +242,7 @@ static void vc4_hvs_lut_load(struct vc4_hvs *hvs,
>   	for (i = 0; i < crtc->gamma_size; i++)
>   		HVS_WRITE(SCALER_GAMDATA, vc4_crtc->lut_b[i]);
>   
> +exit:
>   	drm_dev_exit(idx);
>   }
>   
> 

Re: [PATCH 2/3] drm/vc4: Match drm_dev_enter and exit calls in vc4_hvs_atomic_flush

[Maíra Canal]

Hi Dave,

On 10/8/24 13:44, Dave Stevenson wrote:
> Commit 92c17d16476c ("drm/vc4: hvs: Ignore atomic_flush if we're disabled")
> added a path which returned early without having called drm_dev_exit.
> 
> Ensure all paths call drm_dev_exit.
> 
> Fixes: 92c17d16476c ("drm/vc4: hvs: Ignore atomic_flush if we're disabled")
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

With the same comment as PATCH 1/3,

Reviewed-by: Maíra Canal <mcanal@igalia.com>

Best Regards,
- Maíra

> ---
>   drivers/gpu/drm/vc4/vc4_hvs.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/vc4/vc4_hvs.c b/drivers/gpu/drm/vc4/vc4_hvs.c
> index 546ee11016b2..c5ebc317188a 100644
> --- a/drivers/gpu/drm/vc4/vc4_hvs.c
> +++ b/drivers/gpu/drm/vc4/vc4_hvs.c
> @@ -603,7 +603,7 @@ void vc4_hvs_atomic_flush(struct drm_crtc *crtc,
>   	}
>   
>   	if (vc4_state->assigned_channel == VC4_HVS_CHANNEL_DISABLED)
> -		return;
> +		goto exit;
>   
>   	if (debug_dump_regs) {
>   		DRM_INFO("CRTC %d HVS before:\n", drm_crtc_index(crtc));
> @@ -686,6 +686,7 @@ void vc4_hvs_atomic_flush(struct drm_crtc *crtc,
>   		vc4_hvs_dump_state(hvs);
>   	}
>   
> +exit:
>   	drm_dev_exit(idx);
>   }
>   
> 

Re: [PATCH 3/3] drm/vc4: Correct generation check in vc4_hvs_lut_load

[Maíra Canal]

Hi Dave,

On 10/8/24 13:44, Dave Stevenson wrote:
> Commit 24c5ed3ddf27 ("drm/vc4: Introduce generation number enum")
> incorrectly swapped a check of hvs->vc4->is_vc5 to
> hvs->vc4->gen == VC4_GEN_4 in vc4_hvs_lut_load, hence breaking
> loading the gamma look up table on Pi0-3.
> 
> Correct that conditional.
> 
> Fixes: 24c5ed3ddf27 ("drm/vc4: Introduce generation number enum")
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> ---
>   drivers/gpu/drm/vc4/vc4_hvs.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/vc4/vc4_hvs.c b/drivers/gpu/drm/vc4/vc4_hvs.c
> index c5ebc317188a..1edf6e3fa7e6 100644
> --- a/drivers/gpu/drm/vc4/vc4_hvs.c
> +++ b/drivers/gpu/drm/vc4/vc4_hvs.c
> @@ -224,7 +224,7 @@ static void vc4_hvs_lut_load(struct vc4_hvs *hvs,
>   	if (!drm_dev_enter(drm, &idx))
>   		return;
>   
> -	if (hvs->vc4->gen == VC4_GEN_4)
> +	if (hvs->vc4->gen != VC4_GEN_4)

Again, another nit: I believe `hvs->vc4->gen > VC4_GEN_4` is more
semantic and it's the standard I usually see around the driver.

But this isn't critical, so:

Reviewed-by: Maíra Canal <mcanal@igalia.com>

Best Regards,
- Maíra

>   		goto exit;
>   
>   	/* The LUT memory is laid out with each HVS channel in order,
> 

Re: [PATCH 1/3] drm/vc4: Match drm_dev_enter and exit calls in vc4_hvs_lut_load

[Marek Szyprowski]

On 08.10.2024 18:44, Dave Stevenson wrote:
> Commit 52efe364d196 ("drm/vc4: hvs: Don't write gamma luts on 2711")
> added a return path to vc4_hvs_lut_load that had called
> drm_dev_enter, but not drm_dev_exit.
>
> Ensure we call drm_dev_exit.
>
> Fixes: 52efe364d196 ("drm/vc4: hvs: Don't write gamma luts on 2711")
> Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Closes: https://lore.kernel.org/dri-devel/37051126-3921-4afe-a936-5f828bff5752@samsung.com/
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>   drivers/gpu/drm/vc4/vc4_hvs.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_hvs.c b/drivers/gpu/drm/vc4/vc4_hvs.c
> index 2a366a607fcc..546ee11016b2 100644
> --- a/drivers/gpu/drm/vc4/vc4_hvs.c
> +++ b/drivers/gpu/drm/vc4/vc4_hvs.c
> @@ -225,7 +225,7 @@ static void vc4_hvs_lut_load(struct vc4_hvs *hvs,
>   		return;
>   
>   	if (hvs->vc4->gen == VC4_GEN_4)
> -		return;
> +		goto exit;
>   
>   	/* The LUT memory is laid out with each HVS channel in order,
>   	 * each of which takes 256 writes for R, 256 for G, then 256
> @@ -242,6 +242,7 @@ static void vc4_hvs_lut_load(struct vc4_hvs *hvs,
>   	for (i = 0; i < crtc->gamma_size; i++)
>   		HVS_WRITE(SCALER_GAMDATA, vc4_crtc->lut_b[i]);
>   
> +exit:
>   	drm_dev_exit(idx);
>   }
>   
>
Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH 2/3] drm/vc4: Match drm_dev_enter and exit calls in vc4_hvs_atomic_flush

[Marek Szyprowski]

On 08.10.2024 18:44, Dave Stevenson wrote:
> Commit 92c17d16476c ("drm/vc4: hvs: Ignore atomic_flush if we're disabled")
> added a path which returned early without having called drm_dev_exit.
>
> Ensure all paths call drm_dev_exit.
>
> Fixes: 92c17d16476c ("drm/vc4: hvs: Ignore atomic_flush if we're disabled")
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>   drivers/gpu/drm/vc4/vc4_hvs.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_hvs.c b/drivers/gpu/drm/vc4/vc4_hvs.c
> index 546ee11016b2..c5ebc317188a 100644
> --- a/drivers/gpu/drm/vc4/vc4_hvs.c
> +++ b/drivers/gpu/drm/vc4/vc4_hvs.c
> @@ -603,7 +603,7 @@ void vc4_hvs_atomic_flush(struct drm_crtc *crtc,
>   	}
>   
>   	if (vc4_state->assigned_channel == VC4_HVS_CHANNEL_DISABLED)
> -		return;
> +		goto exit;
>   
>   	if (debug_dump_regs) {
>   		DRM_INFO("CRTC %d HVS before:\n", drm_crtc_index(crtc));
> @@ -686,6 +686,7 @@ void vc4_hvs_atomic_flush(struct drm_crtc *crtc,
>   		vc4_hvs_dump_state(hvs);
>   	}
>   
> +exit:
>   	drm_dev_exit(idx);
>   }
>   
>
Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH 3/3] drm/vc4: Correct generation check in vc4_hvs_lut_load

[Marek Szyprowski]

On 08.10.2024 18:44, Dave Stevenson wrote:
> Commit 24c5ed3ddf27 ("drm/vc4: Introduce generation number enum")
> incorrectly swapped a check of hvs->vc4->is_vc5 to
> hvs->vc4->gen == VC4_GEN_4 in vc4_hvs_lut_load, hence breaking
> loading the gamma look up table on Pi0-3.
>
> Correct that conditional.
>
> Fixes: 24c5ed3ddf27 ("drm/vc4: Introduce generation number enum")
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>   drivers/gpu/drm/vc4/vc4_hvs.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_hvs.c b/drivers/gpu/drm/vc4/vc4_hvs.c
> index c5ebc317188a..1edf6e3fa7e6 100644
> --- a/drivers/gpu/drm/vc4/vc4_hvs.c
> +++ b/drivers/gpu/drm/vc4/vc4_hvs.c
> @@ -224,7 +224,7 @@ static void vc4_hvs_lut_load(struct vc4_hvs *hvs,
>   	if (!drm_dev_enter(drm, &idx))
>   		return;
>   
> -	if (hvs->vc4->gen == VC4_GEN_4)
> +	if (hvs->vc4->gen != VC4_GEN_4)
>   		goto exit;
>   
>   	/* The LUT memory is laid out with each HVS channel in order,
>
Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH 3/3] drm/vc4: Correct generation check in vc4_hvs_lut_load

[Dave Stevenson]

Hi Maira

Thanks for the review

On Wed, 9 Oct 2024 at 02:30, Maíra Canal <mcanal@igalia.com> wrote:
>
> Hi Dave,
>
> On 10/8/24 13:44, Dave Stevenson wrote:
> > Commit 24c5ed3ddf27 ("drm/vc4: Introduce generation number enum")
> > incorrectly swapped a check of hvs->vc4->is_vc5 to
> > hvs->vc4->gen == VC4_GEN_4 in vc4_hvs_lut_load, hence breaking
> > loading the gamma look up table on Pi0-3.
> >
> > Correct that conditional.
> >
> > Fixes: 24c5ed3ddf27 ("drm/vc4: Introduce generation number enum")
> > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > ---
> >   drivers/gpu/drm/vc4/vc4_hvs.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/vc4/vc4_hvs.c b/drivers/gpu/drm/vc4/vc4_hvs.c
> > index c5ebc317188a..1edf6e3fa7e6 100644
> > --- a/drivers/gpu/drm/vc4/vc4_hvs.c
> > +++ b/drivers/gpu/drm/vc4/vc4_hvs.c
> > @@ -224,7 +224,7 @@ static void vc4_hvs_lut_load(struct vc4_hvs *hvs,
> >       if (!drm_dev_enter(drm, &idx))
> >               return;
> >
> > -     if (hvs->vc4->gen == VC4_GEN_4)
> > +     if (hvs->vc4->gen != VC4_GEN_4)
>
> Again, another nit: I believe `hvs->vc4->gen > VC4_GEN_4` is more
> semantic and it's the standard I usually see around the driver.

With 24c5ed3ddf27 which this is fixing, all instances in vc4_hvs.c
were changed to == VC4_GEN_4 or == VC4_GEN_5. So I guess == VC4_GEN_5
would have been the better option here.

Yes downstream we now have GEN_6 (which I'm nearly at a point to send
to the list), and I'll need to update this conditional when that's
added.
I'll leave it as is for now, and ensure I've done an audit of all of
them before pushing GEN_6.

Thanks
  Dave

> But this isn't critical, so:
>
> Reviewed-by: Maíra Canal <mcanal@igalia.com>
>
> Best Regards,
> - Maíra
>
> >               goto exit;
> >
> >       /* The LUT memory is laid out with each HVS channel in order,
> >

Re: [PATCH 0/3] drm/vc4: 3 minor fixes from mismatched drm_dev_[enter|exit] calls

[Dave Stevenson]

On Tue, 8 Oct 2024 at 17:44, Dave Stevenson
<dave.stevenson@raspberrypi.com> wrote:
>
> These are 3 fixes noted from Marek's report that he was seeing lock
> held issues.
> The place he had noticed was actually triggered due to an incorrect
> change to a conditional, but also a further place was noted where
> it was possible to leave the function without calling drm_dev_exit.
>
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

Applied to drm-misc-next.

> ---
> Dave Stevenson (3):
>       drm/vc4: Match drm_dev_enter and exit calls in vc4_hvs_lut_load
>       drm/vc4: Match drm_dev_enter and exit calls in vc4_hvs_atomic_flush
>       drm/vc4: Correct generation check in vc4_hvs_lut_load
>
>  drivers/gpu/drm/vc4/vc4_hvs.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> ---
> base-commit: 2c34a5464007c45142af009d13c668b0630ef9c0
> change-id: 20241008-drm-vc4-fixes-4aceb259455c
>
> Best regards,
> --
> Dave Stevenson <dave.stevenson@raspberrypi.com>
>