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

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

Posted by Dave Stevenson 1 month, 2 weeks ago

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 3/3] drm/vc4: Correct generation check in vc4_hvs_lut_load

Posted by Marek Szyprowski 1 month, 2 weeks ago

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

Posted by Maíra Canal 1 month, 2 weeks ago

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 3/3] drm/vc4: Correct generation check in vc4_hvs_lut_load

Posted by Dave Stevenson 1 month, 2 weeks ago

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,
> >