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@xxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> > ---
> >   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@xxxxxxxxxx>
>
> Best Regards,
> - Maíra
>
> >               goto exit;
> >
> >       /* The LUT memory is laid out with each HVS channel in order,
> >