[PATCH] drm/vc4: plane: Expand the lower bits using the LSB

[PATCH] drm/vc4: plane: Expand the lower bits using the LSB

[Maxime Ripard]

The vc4 HVS uses an internal RGB888 representation of the frames, and will
by default expand formats using a lower depth using zeros.

This causes an issue when we try to use other compositing software such as
pixman that seems to be filling the missing bits using the format least
significant bit value. As such, this prevents us from checking the display
output in a reliable way.

To prevent this, force the same behaviour so that we can do such things.

Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
 drivers/gpu/drm/vc4/vc4_plane.c | 1 +
 drivers/gpu/drm/vc4/vc4_regs.h  | 8 ++++++++
 2 files changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c
index ce39390be389..8dd33c6e9fd8 100644
--- a/drivers/gpu/drm/vc4/vc4_plane.c
+++ b/drivers/gpu/drm/vc4/vc4_plane.c
@@ -542,6 +542,7 @@ static int vc4_plane_mode_set(struct drm_plane *plane,
 	/* Control word */
 	vc4_dlist_write(vc4_state,
 			SCALER_CTL0_VALID |
+			VC4_SET_FIELD(SCALER_CTL0_EXPAND_LSB, SCALER_CTL0_EXPAND) |
 			(format->pixel_order << SCALER_CTL0_ORDER_SHIFT) |
 			(format->hvs << SCALER_CTL0_PIXEL_FORMAT_SHIFT) |
 			VC4_SET_FIELD(tiling, SCALER_CTL0_TILING) |
diff --git a/drivers/gpu/drm/vc4/vc4_regs.h b/drivers/gpu/drm/vc4/vc4_regs.h
index a141496104a6..7c28e6207ec2 100644
--- a/drivers/gpu/drm/vc4/vc4_regs.h
+++ b/drivers/gpu/drm/vc4/vc4_regs.h
@@ -806,12 +806,20 @@ enum hvs_pixel_format {
 #define SCALER_CTL0_ORDER_MASK			VC4_MASK(14, 13)
 #define SCALER_CTL0_ORDER_SHIFT			13
 
+#define SCALER_CTL0_EXPAND_MASK			VC4_MASK(12, 11)
+#define SCALER_CTL0_EXPAND_SHIFT		11
+
 #define SCALER_CTL0_SCL1_MASK			VC4_MASK(10, 8)
 #define SCALER_CTL0_SCL1_SHIFT			8
 
 #define SCALER_CTL0_SCL0_MASK			VC4_MASK(7, 5)
 #define SCALER_CTL0_SCL0_SHIFT			5
 
+#define SCALER_CTL0_EXPAND_ZERO			0
+#define SCALER_CTL0_EXPAND_LSB			1
+#define SCALER_CTL0_EXPAND_MSB			2
+#define SCALER_CTL0_EXPAND_REPEAT		3
+
 #define SCALER_CTL0_SCL_H_PPF_V_PPF		0
 #define SCALER_CTL0_SCL_H_TPZ_V_PPF		1
 #define SCALER_CTL0_SCL_H_PPF_V_TPZ		2
-- 
2.17.0

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

Re: [PATCH] drm/vc4: plane: Expand the lower bits using the LSB

[Eric Anholt]

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

Maxime Ripard <maxime.ripard@bootlin.com> writes:

> The vc4 HVS uses an internal RGB888 representation of the frames, and will
> by default expand formats using a lower depth using zeros.
>
> This causes an issue when we try to use other compositing software such as
> pixman that seems to be filling the missing bits using the format least
> significant bit value. As such, this prevents us from checking the display
> output in a reliable way.

I don't think this is the right expansion function, though?  My
understanding of proper unorm expansion, and what pixman's
unorm_to_unorm() does, is that you replicate the value in the missing
bits until you've filled them all (so SCALER_CTL0_EXPAND_REPEAT)

[-- 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] drm/vc4: plane: Expand the lower bits using the LSB

[Maxime Ripard]

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

Hi Eric,

On Tue, Apr 24, 2018 at 09:53:28AM -0700, Eric Anholt wrote:
> Maxime Ripard <maxime.ripard@bootlin.com> writes:
> 
> > The vc4 HVS uses an internal RGB888 representation of the frames, and will
> > by default expand formats using a lower depth using zeros.
> >
> > This causes an issue when we try to use other compositing software such as
> > pixman that seems to be filling the missing bits using the format least
> > significant bit value. As such, this prevents us from checking the display
> > output in a reliable way.
> 
> I don't think this is the right expansion function, though?  My
> understanding of proper unorm expansion, and what pixman's
> unorm_to_unorm() does, is that you replicate the value in the missing
> bits until you've filled them all (so SCALER_CTL0_EXPAND_REPEAT)

This was based on experiment, so I might have missed that case and you
are probably right. I'll test with SCALER_CTL0_EXPAND_REPEAT and let
you know :)

Thanks!
Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 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] drm/vc4: plane: Expand the lower bits using the LSB

[Stefan Schake]

On Tue, Apr 24, 2018 at 9:48 AM, Maxime Ripard
<maxime.ripard@bootlin.com> wrote:
> The vc4 HVS uses an internal RGB888 representation of the frames, and will
> by default expand formats using a lower depth using zeros.
>
> This causes an issue when we try to use other compositing software such as
> pixman that seems to be filling the missing bits using the format least
> significant bit value. As such, this prevents us from checking the display
> output in a reliable way.
>
> To prevent this, force the same behaviour so that we can do such things.
>
> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> ---
>  drivers/gpu/drm/vc4/vc4_plane.c | 1 +
>  drivers/gpu/drm/vc4/vc4_regs.h  | 8 ++++++++
>  2 files changed, 9 insertions(+)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c
> index ce39390be389..8dd33c6e9fd8 100644
> --- a/drivers/gpu/drm/vc4/vc4_plane.c
> +++ b/drivers/gpu/drm/vc4/vc4_plane.c
> @@ -542,6 +542,7 @@ static int vc4_plane_mode_set(struct drm_plane *plane,
>         /* Control word */
>         vc4_dlist_write(vc4_state,
>                         SCALER_CTL0_VALID |
> +                       VC4_SET_FIELD(SCALER_CTL0_EXPAND_LSB, SCALER_CTL0_EXPAND) |
>                         (format->pixel_order << SCALER_CTL0_ORDER_SHIFT) |
>                         (format->hvs << SCALER_CTL0_PIXEL_FORMAT_SHIFT) |
>                         VC4_SET_FIELD(tiling, SCALER_CTL0_TILING) |
> diff --git a/drivers/gpu/drm/vc4/vc4_regs.h b/drivers/gpu/drm/vc4/vc4_regs.h
> index a141496104a6..7c28e6207ec2 100644
> --- a/drivers/gpu/drm/vc4/vc4_regs.h
> +++ b/drivers/gpu/drm/vc4/vc4_regs.h
> @@ -806,12 +806,20 @@ enum hvs_pixel_format {
>  #define SCALER_CTL0_ORDER_MASK                 VC4_MASK(14, 13)
>  #define SCALER_CTL0_ORDER_SHIFT                        13
>
> +#define SCALER_CTL0_EXPAND_MASK                        VC4_MASK(12, 11)
> +#define SCALER_CTL0_EXPAND_SHIFT               11
> +
>  #define SCALER_CTL0_SCL1_MASK                  VC4_MASK(10, 8)
>  #define SCALER_CTL0_SCL1_SHIFT                 8
>
>  #define SCALER_CTL0_SCL0_MASK                  VC4_MASK(7, 5)
>  #define SCALER_CTL0_SCL0_SHIFT                 5
>
> +#define SCALER_CTL0_EXPAND_ZERO                        0
> +#define SCALER_CTL0_EXPAND_LSB                 1
> +#define SCALER_CTL0_EXPAND_MSB                 2
> +#define SCALER_CTL0_EXPAND_REPEAT              3

Just a heads up, those defines have recently landed in drm-misc, with
slightly different naming: SCALER_CTL0_RGBA_EXPAND_*

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