[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

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

[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.