| Message ID | 20181024100505.22436-2-boris.brezillon@bootlin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] drm/vc4: Set PPF scaling when the source image is only vertically scaled | expand |
In the subject: s/Force// On Wed, 24 Oct 2018 12:05:04 +0200 Boris Brezillon <boris.brezillon@bootlin.com> wrote: > For the YUV conversion to work properly, ->x_scaling[0,1] should never > be set to VC4_SCALING_NONE, but vc4_get_scaling_mode() might return > VC4_SCALING_NONE if the horizontal scaling ratio exactly matches the > horizontal subsampling factor. Add a test to turn VC4_SCALING_NONE > into VC4_SCALING_PPF when that happens. > > Fixes: fc04023fafec ("drm/vc4: Add support for YUV planes.") > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> > --- > > The Cc-stable tag has been omitted on purpose: a few things have > changed in this portion of code and backporting this fix is not > trivial. Since noone complained so far, let's not bother backporting > it. > --- > drivers/gpu/drm/vc4/vc4_plane.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c > index 32b7b9f47c5d..5950e6b6b7f0 100644 > --- a/drivers/gpu/drm/vc4/vc4_plane.c > +++ b/drivers/gpu/drm/vc4/vc4_plane.c > @@ -320,6 +320,9 @@ static int vc4_plane_setup_clipping_and_scaling(struct drm_plane_state *state) > */ > if (vc4_state->x_scaling[0] == VC4_SCALING_NONE) > vc4_state->x_scaling[0] = VC4_SCALING_PPF; > + > + if (vc4_state->x_scaling[1] == VC4_SCALING_NONE) > + vc4_state->x_scaling[1] = VC4_SCALING_PPF; > } else { > vc4_state->is_yuv = false; > vc4_state->x_scaling[1] = VC4_SCALING_NONE;
Boris Brezillon <boris.brezillon@bootlin.com> writes: > For the YUV conversion to work properly, ->x_scaling[0,1] should never > be set to VC4_SCALING_NONE, but vc4_get_scaling_mode() might return > VC4_SCALING_NONE if the horizontal scaling ratio exactly matches the > horizontal subsampling factor. Add a test to turn VC4_SCALING_NONE > into VC4_SCALING_PPF when that happens. > > Fixes: fc04023fafec ("drm/vc4: Add support for YUV planes.") > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> I couldn't find a spec justification for this -- did you have a testcase that fails?
On Thu, 08 Nov 2018 06:52:44 -0800 Eric Anholt <eric@anholt.net> wrote: > Boris Brezillon <boris.brezillon@bootlin.com> writes: > > > For the YUV conversion to work properly, ->x_scaling[0,1] should never > > be set to VC4_SCALING_NONE, but vc4_get_scaling_mode() might return > > VC4_SCALING_NONE if the horizontal scaling ratio exactly matches the > > horizontal subsampling factor. Add a test to turn VC4_SCALING_NONE > > into VC4_SCALING_PPF when that happens. > > > > Fixes: fc04023fafec ("drm/vc4: Add support for YUV planes.") > > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> > > I couldn't find a spec justification for this -- did you have a testcase > that fails? Yep. Just set the downscaling ratio to 0.5 with an NV12 format and you'll hit the issue (I used modetest to do that): # modetest -M vc4 -s 29:1920x1080-60 -P 96@95:1920x1080*0.5@NV12
Boris Brezillon <boris.brezillon@bootlin.com> writes: > On Thu, 08 Nov 2018 06:52:44 -0800 > Eric Anholt <eric@anholt.net> wrote: > >> Boris Brezillon <boris.brezillon@bootlin.com> writes: >> >> > For the YUV conversion to work properly, ->x_scaling[0,1] should never >> > be set to VC4_SCALING_NONE, but vc4_get_scaling_mode() might return >> > VC4_SCALING_NONE if the horizontal scaling ratio exactly matches the >> > horizontal subsampling factor. Add a test to turn VC4_SCALING_NONE >> > into VC4_SCALING_PPF when that happens. >> > >> > Fixes: fc04023fafec ("drm/vc4: Add support for YUV planes.") >> > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> >> >> I couldn't find a spec justification for this -- did you have a testcase >> that fails? > > Yep. Just set the downscaling ratio to 0.5 with an NV12 format and > you'll hit the issue (I used modetest to do that): > > # modetest -M vc4 -s 29:1920x1080-60 -P 96@95:1920x1080*0.5@NV12 I found that the firmware has a similar behavior to your patch ("if Y is !unity (x or scaling) and UV is unity, set UV to HPPF/VPPF scaling"). They also select the unity flag after the YUV scaling fixup. Regardless, if this works, it's got my reviewed-by. Hopefully we can do some IGT with writeback or chamelium testing all of the X/Y scaling options with a focus on hitting these 1:1 ratios. The state space is big and the docs are just ambiguous enough.
Hi Boris & Eric. On Thu, 8 Nov 2018 at 15:12, Eric Anholt <eric@anholt.net> wrote: > > Boris Brezillon <boris.brezillon@bootlin.com> writes: > > > On Thu, 08 Nov 2018 06:52:44 -0800 > > Eric Anholt <eric@anholt.net> wrote: > > > >> Boris Brezillon <boris.brezillon@bootlin.com> writes: > >> > >> > For the YUV conversion to work properly, ->x_scaling[0,1] should never > >> > be set to VC4_SCALING_NONE, but vc4_get_scaling_mode() might return > >> > VC4_SCALING_NONE if the horizontal scaling ratio exactly matches the > >> > horizontal subsampling factor. Add a test to turn VC4_SCALING_NONE > >> > into VC4_SCALING_PPF when that happens. > >> > > >> > Fixes: fc04023fafec ("drm/vc4: Add support for YUV planes.") > >> > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> > >> > >> I couldn't find a spec justification for this -- did you have a testcase > >> that fails? > > > > Yep. Just set the downscaling ratio to 0.5 with an NV12 format and > > you'll hit the issue (I used modetest to do that): > > > > # modetest -M vc4 -s 29:1920x1080-60 -P 96@95:1920x1080*0.5@NV12 > > I found that the firmware has a similar behavior to your patch ("if Y is > !unity (x or scaling) and UV is unity, set UV to HPPF/VPPF scaling"). > They also select the unity flag after the YUV scaling fixup. > > Regardless, if this works, it's got my reviewed-by. > > Hopefully we can do some IGT with writeback or chamelium testing all of > the X/Y scaling options with a focus on hitting these 1:1 ratios. The > state space is big and the docs are just ambiguous enough. Great timing as I've hit exactly this when playing back a 1080P video on a 1080P screen. The colours were very muted in this situation, whilst playing any other resolution or any RGB format was fine. Took me a while to realise it wasn't the conversion matrices being set incorrectly :-/ Applying this patch sorts the problem. This was on the downstream 4.19 kernel, and the v2 of this set backported fairly easily. Can I request that for stable? Otherwise we can cherry-pick it for downstream. Thanks Dave
On Mon, 12 Nov 2018 10:20:35 +0000 Dave Stevenson <dave.stevenson@raspberrypi.org> wrote: > Hi Boris & Eric. > > On Thu, 8 Nov 2018 at 15:12, Eric Anholt <eric@anholt.net> wrote: > > > > Boris Brezillon <boris.brezillon@bootlin.com> writes: > > > > > On Thu, 08 Nov 2018 06:52:44 -0800 > > > Eric Anholt <eric@anholt.net> wrote: > > > > > >> Boris Brezillon <boris.brezillon@bootlin.com> writes: > > >> > > >> > For the YUV conversion to work properly, ->x_scaling[0,1] should never > > >> > be set to VC4_SCALING_NONE, but vc4_get_scaling_mode() might return > > >> > VC4_SCALING_NONE if the horizontal scaling ratio exactly matches the > > >> > horizontal subsampling factor. Add a test to turn VC4_SCALING_NONE > > >> > into VC4_SCALING_PPF when that happens. > > >> > > > >> > Fixes: fc04023fafec ("drm/vc4: Add support for YUV planes.") > > >> > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> > > >> > > >> I couldn't find a spec justification for this -- did you have a testcase > > >> that fails? > > > > > > Yep. Just set the downscaling ratio to 0.5 with an NV12 format and > > > you'll hit the issue (I used modetest to do that): > > > > > > # modetest -M vc4 -s 29:1920x1080-60 -P 96@95:1920x1080*0.5@NV12 > > > > I found that the firmware has a similar behavior to your patch ("if Y is > > !unity (x or scaling) and UV is unity, set UV to HPPF/VPPF scaling"). > > They also select the unity flag after the YUV scaling fixup. > > > > Regardless, if this works, it's got my reviewed-by. > > > > Hopefully we can do some IGT with writeback or chamelium testing all of > > the X/Y scaling options with a focus on hitting these 1:1 ratios. The > > state space is big and the docs are just ambiguous enough. > > Great timing as I've hit exactly this when playing back a 1080P video > on a 1080P screen. The colours were very muted in this situation, > whilst playing any other resolution or any RGB format was fine. Took > me a while to realise it wasn't the conversion matrices being set > incorrectly :-/ Applying this patch sorts the problem. > This was on the downstream 4.19 kernel, and the v2 of this set > backported fairly easily. Can I request that for stable? Otherwise we > can cherry-pick it for downstream. Sure.
diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c index 32b7b9f47c5d..5950e6b6b7f0 100644 --- a/drivers/gpu/drm/vc4/vc4_plane.c +++ b/drivers/gpu/drm/vc4/vc4_plane.c @@ -320,6 +320,9 @@ static int vc4_plane_setup_clipping_and_scaling(struct drm_plane_state *state) */ if (vc4_state->x_scaling[0] == VC4_SCALING_NONE) vc4_state->x_scaling[0] = VC4_SCALING_PPF; + + if (vc4_state->x_scaling[1] == VC4_SCALING_NONE) + vc4_state->x_scaling[1] = VC4_SCALING_PPF; } else { vc4_state->is_yuv = false; vc4_state->x_scaling[1] = VC4_SCALING_NONE;
For the YUV conversion to work properly, ->x_scaling[0,1] should never be set to VC4_SCALING_NONE, but vc4_get_scaling_mode() might return VC4_SCALING_NONE if the horizontal scaling ratio exactly matches the horizontal subsampling factor. Add a test to turn VC4_SCALING_NONE into VC4_SCALING_PPF when that happens. Fixes: fc04023fafec ("drm/vc4: Add support for YUV planes.") Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com> --- The Cc-stable tag has been omitted on purpose: a few things have changed in this portion of code and backporting this fix is not trivial. Since noone complained so far, let's not bother backporting it. --- drivers/gpu/drm/vc4/vc4_plane.c | 3 +++ 1 file changed, 3 insertions(+)