The source image might be only vertically scaled, and in this case ->is_unity will be false, but we'd still have to force ->x_scaling[0] to VC4_SCALING_PPF for YUV conversion to work properly.
Let's replace the ->is_unity test by->x_scaling[0] == VC4_SCALING_NONE to cope with that.
Fixes: 658d8cbd07da ("drm/vc4: Fix the "no scaling" case on multi-planar YUV formats") Cc: stable@vger.kernel.org Signed-off-by: Boris Brezillon boris.brezillon@bootlin.com --- drivers/gpu/drm/vc4/vc4_plane.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c index 60d5ad19cedd..32b7b9f47c5d 100644 --- a/drivers/gpu/drm/vc4/vc4_plane.c +++ b/drivers/gpu/drm/vc4/vc4_plane.c @@ -318,7 +318,7 @@ static int vc4_plane_setup_clipping_and_scaling(struct drm_plane_state *state) * even on a plane that's otherwise 1:1. Looks like only PPF * works in that case, so let's pick that one. */ - if (vc4_state->is_unity) + if (vc4_state->x_scaling[0] == VC4_SCALING_NONE) vc4_state->x_scaling[0] = VC4_SCALING_PPF; } else { vc4_state->is_yuv = false;
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;
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;
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.
The HVS spec recommends using PPF when the downscaling ratio is between 2/3 and 1. Let's modify vc4_get_scaling_mode() to follow this recommendation.
Signed-off-by: Boris Brezillon boris.brezillon@bootlin.com --- drivers/gpu/drm/vc4/vc4_plane.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c index 5950e6b6b7f0..1d0d91e50aaf 100644 --- a/drivers/gpu/drm/vc4/vc4_plane.c +++ b/drivers/gpu/drm/vc4/vc4_plane.c @@ -129,12 +129,12 @@ static const struct hvs_format *vc4_get_hvs_format(u32 drm_format)
static enum vc4_scaling_mode vc4_get_scaling_mode(u32 src, u32 dst) { - if (dst > src) + if (dst == src) + return VC4_SCALING_NONE; + if (3 * dst >= 2 * src) return VC4_SCALING_PPF; - else if (dst < src) - return VC4_SCALING_TPZ; else - return VC4_SCALING_NONE; + return VC4_SCALING_TPZ; }
static bool plane_enabled(struct drm_plane_state *state)
Boris Brezillon boris.brezillon@bootlin.com writes:
The HVS spec recommends using PPF when the downscaling ratio is between 2/3 and 1. Let's modify vc4_get_scaling_mode() to follow this recommendation.
Signed-off-by: Boris Brezillon boris.brezillon@bootlin.com
drivers/gpu/drm/vc4/vc4_plane.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c index 5950e6b6b7f0..1d0d91e50aaf 100644 --- a/drivers/gpu/drm/vc4/vc4_plane.c +++ b/drivers/gpu/drm/vc4/vc4_plane.c @@ -129,12 +129,12 @@ static const struct hvs_format *vc4_get_hvs_format(u32 drm_format)
static enum vc4_scaling_mode vc4_get_scaling_mode(u32 src, u32 dst) {
- if (dst > src)
- if (dst == src)
return VC4_SCALING_NONE;- if (3 * dst >= 2 * src) return VC4_SCALING_PPF;
- else if (dst < src)
return VC4_SCALING_TPZ;return VC4_SCALING_NONE;
return VC4_SCALING_TPZ;}
Reviewed-by: Eric Anholt eric@anholt.net
On Wed, 24 Oct 2018 12:05:03 +0200 Boris Brezillon boris.brezillon@bootlin.com wrote:
The source image might be only vertically scaled, and in this case ->is_unity will be false, but we'd still have to force ->x_scaling[0] to VC4_SCALING_PPF for YUV conversion to work properly.
Let's replace the ->is_unity test by->x_scaling[0] == VC4_SCALING_NONE to cope with that.
Fixes: 658d8cbd07da ("drm/vc4: Fix the "no scaling" case on multi-planar YUV formats") Cc: stable@vger.kernel.org Signed-off-by: Boris Brezillon boris.brezillon@bootlin.com
drivers/gpu/drm/vc4/vc4_plane.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c index 60d5ad19cedd..32b7b9f47c5d 100644 --- a/drivers/gpu/drm/vc4/vc4_plane.c +++ b/drivers/gpu/drm/vc4/vc4_plane.c @@ -318,7 +318,7 @@ static int vc4_plane_setup_clipping_and_scaling(struct drm_plane_state *state) * even on a plane that's otherwise 1:1. Looks like only PPF * works in that case, so let's pick that one. */
if (vc4_state->is_unity)
if (vc4_state->x_scaling[0] == VC4_SCALING_NONE) vc4_state->x_scaling[0] = VC4_SCALING_PPF;
Actually, I'm not sure about this patch is needed. According to the spec, we should not enable the scaler on the Y channel when ->is_unity is true. I tested it, and it seems to work if we just leave x_scaling[0] to VC4_SCALING_NONE.
Eric, do you remember why you did that?
Boris Brezillon boris.brezillon@bootlin.com writes:
On Wed, 24 Oct 2018 12:05:03 +0200 Boris Brezillon boris.brezillon@bootlin.com wrote:
The source image might be only vertically scaled, and in this case ->is_unity will be false, but we'd still have to force ->x_scaling[0] to VC4_SCALING_PPF for YUV conversion to work properly.
Let's replace the ->is_unity test by->x_scaling[0] == VC4_SCALING_NONE to cope with that.
Fixes: 658d8cbd07da ("drm/vc4: Fix the "no scaling" case on multi-planar YUV formats") Cc: stable@vger.kernel.org Signed-off-by: Boris Brezillon boris.brezillon@bootlin.com
drivers/gpu/drm/vc4/vc4_plane.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c index 60d5ad19cedd..32b7b9f47c5d 100644 --- a/drivers/gpu/drm/vc4/vc4_plane.c +++ b/drivers/gpu/drm/vc4/vc4_plane.c @@ -318,7 +318,7 @@ static int vc4_plane_setup_clipping_and_scaling(struct drm_plane_state *state) * even on a plane that's otherwise 1:1. Looks like only PPF * works in that case, so let's pick that one. */
if (vc4_state->is_unity)
if (vc4_state->x_scaling[0] == VC4_SCALING_NONE) vc4_state->x_scaling[0] = VC4_SCALING_PPF;Actually, I'm not sure about this patch is needed. According to the spec, we should not enable the scaler on the Y channel when ->is_unity is true. I tested it, and it seems to work if we just leave x_scaling[0] to VC4_SCALING_NONE.
Sorry, I've been delaying on this series until I could spend some time looking at the spec.
I think you're right, I see no reason to kick us out of is_unity for YUV -- you can have is_unity, Y unscaled, and UV scaled.
On Wed, 07 Nov 2018 09:08:02 -0800 Eric Anholt eric@anholt.net wrote:
Boris Brezillon boris.brezillon@bootlin.com writes:
On Wed, 24 Oct 2018 12:05:03 +0200 Boris Brezillon boris.brezillon@bootlin.com wrote:
The source image might be only vertically scaled, and in this case ->is_unity will be false, but we'd still have to force ->x_scaling[0] to VC4_SCALING_PPF for YUV conversion to work properly.
Let's replace the ->is_unity test by->x_scaling[0] == VC4_SCALING_NONE to cope with that.
Fixes: 658d8cbd07da ("drm/vc4: Fix the "no scaling" case on multi-planar YUV formats") Cc: stable@vger.kernel.org Signed-off-by: Boris Brezillon boris.brezillon@bootlin.com
drivers/gpu/drm/vc4/vc4_plane.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c index 60d5ad19cedd..32b7b9f47c5d 100644 --- a/drivers/gpu/drm/vc4/vc4_plane.c +++ b/drivers/gpu/drm/vc4/vc4_plane.c @@ -318,7 +318,7 @@ static int vc4_plane_setup_clipping_and_scaling(struct drm_plane_state *state) * even on a plane that's otherwise 1:1. Looks like only PPF * works in that case, so let's pick that one. */
if (vc4_state->is_unity)
if (vc4_state->x_scaling[0] == VC4_SCALING_NONE) vc4_state->x_scaling[0] = VC4_SCALING_PPF;Actually, I'm not sure about this patch is needed. According to the spec, we should not enable the scaler on the Y channel when ->is_unity is true. I tested it, and it seems to work if we just leave x_scaling[0] to VC4_SCALING_NONE.
Sorry, I've been delaying on this series until I could spend some time looking at the spec.
I think you're right, I see no reason to kick us out of is_unity for YUV -- you can have is_unity, Y unscaled, and UV scaled.
Okay, so I'll just remove
if (vc4_state->x_scaling[0] == VC4_SCALING_NONE) vc4_state->x_scaling[0] = VC4_SCALING_PPF;
What about the 2 other patches in this series? Patch 2 is fixing a real bug, patch 3 is not fixing a bug, but according to the spec PPF is better when the downscaling ratio is small.
dri-devel@lists.freedesktop.org
-
Boris Brezillon -
Dave Stevenson -
Eric Anholt