| Message ID | 20241008-drm-vc4-fixes-v1-3-9d0396ca9f42@raspberrypi.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/vc4: 3 minor fixes from mismatched drm_dev_[enter|exit] calls | expand |
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, >
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
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, > >
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,
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(-)