[PATCH 1/3] drm/vc4: Set PPF scaling when the source image is only vertically scaled

[PATCH 1/3] drm/vc4: Set PPF scaling when the source image is only vertically scaled

[Boris Brezillon]

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;
-- 
2.17.1

[PATCH 2/3] drm/vc4: Force ->x_scaling[1] should never be set to VC4_SCALING_NONE

[Boris Brezillon]

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;
-- 
2.17.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm/vc4: Force ->x_scaling[1] should never be set to VC4_SCALING_NONE

[Boris Brezillon]

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;

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm/vc4: Force ->x_scaling[1] should never be set to VC4_SCALING_NONE

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 594 bytes --]

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?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm/vc4: Force ->x_scaling[1] should never be set to VC4_SCALING_NONE

[Boris Brezillon]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm/vc4: Force ->x_scaling[1] should never be set to VC4_SCALING_NONE

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 1456 bytes --]

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.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm/vc4: Force ->x_scaling[1] should never be set to VC4_SCALING_NONE

[Dave Stevenson]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm/vc4: Force ->x_scaling[1] should never be set to VC4_SCALING_NONE

[Boris Brezillon]

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.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/3] drm/vc4: Prefer PPF over TPZ when dst >= 2/3 src

[Boris Brezillon]

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)
-- 
2.17.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/3] drm/vc4: Prefer PPF over TPZ when dst >= 2/3 src

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 1085 bytes --]

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;
>  	else
> -		return VC4_SCALING_NONE;
> +		return VC4_SCALING_TPZ;
>  }

Reviewed-by: Eric Anholt <eric@anholt.net>

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/3] drm/vc4: Set PPF scaling when the source image is only vertically scaled

[Boris Brezillon]

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?

Re: [PATCH 1/3] drm/vc4: Set PPF scaling when the source image is only vertically scaled

[Eric Anholt]

[-- Attachment #1: Type: text/plain, Size: 1829 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH 1/3] drm/vc4: Set PPF scaling when the source image is only vertically scaled

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 1829 bytes --]

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.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/3] drm/vc4: Set PPF scaling when the source image is only vertically scaled

[Boris Brezillon]

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.

Re: [PATCH 1/3] drm/vc4: Set PPF scaling when the source image is only vertically scaled

[Boris Brezillon]

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 mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel