[PATCH] drm/rockchip: Require the YTR modifier for AFBC

[PATCH] drm/rockchip: Require the YTR modifier for AFBC

[Alyssa Rosenzweig]

The AFBC decoder used in the Rockchip VOP assumes the use of the
YUV-like colourspace transform (YTR). YTR is lossless for RGB(A)
buffers, which covers the RGBA8 and RGB565 formats supported in
vop_convert_afbc_format. Use of YTR is signaled with the
AFBC_FORMAT_MOD_YTR modifier, which prior to this commit was missing. As
such, a producer would have to generate buffers that do not use YTR,
which the VOP would erroneously decode as YTR, leading to severe visual
corruption.

The upstream AFBC support was developed against a captured frame, which
failed to exercise modifier support. Prior to bring-up of AFBC in Mesa
(in the Panfrost driver), no open userspace respected modifier
reporting. As such, this change is not expected to affect broken
userspaces.

Tested on RK3399 with Panfrost and Weston.

Signed-off-by: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
---
 drivers/gpu/drm/rockchip/rockchip_drm_vop.h | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
index 4a2099cb5..857d97cdc 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
@@ -17,9 +17,20 @@
 
 #define NUM_YUV2YUV_COEFFICIENTS 12
 
+/* AFBC supports a number of configurable modes. Relevant to us is block size
+ * (16x16 or 32x8), storage modifiers (SPARSE, SPLIT), and the YUV-like
+ * colourspace transform (YTR). 16x16 SPARSE mode is always used. SPLIT mode
+ * could be enabled via the hreg_block_split register, but is not currently
+ * handled. The colourspace transform is implicitly always assumed by the
+ * decoder, so consumers must use this transform as well.
+ *
+ * Failure to match modifiers will cause errors displaying AFBC buffers
+ * produced by conformant AFBC producers, including Mesa.
+ */
 #define ROCKCHIP_AFBC_MOD \
 	DRM_FORMAT_MOD_ARM_AFBC( \
 		AFBC_FORMAT_MOD_BLOCK_SIZE_16x16 | AFBC_FORMAT_MOD_SPARSE \
+			| AFBC_FORMAT_MOD_YTR \
 	)
 
 enum vop_data_format {
-- 
2.28.0

[PATCH] drm/rockchip: Require the YTR modifier for AFBC

[Alyssa Rosenzweig]

The AFBC decoder used in the Rockchip VOP assumes the use of the
YUV-like colourspace transform (YTR). YTR is lossless for RGB(A)
buffers, which covers the RGBA8 and RGB565 formats supported in
vop_convert_afbc_format. Use of YTR is signaled with the
AFBC_FORMAT_MOD_YTR modifier, which prior to this commit was missing. As
such, a producer would have to generate buffers that do not use YTR,
which the VOP would erroneously decode as YTR, leading to severe visual
corruption.

The upstream AFBC support was developed against a captured frame, which
failed to exercise modifier support. Prior to bring-up of AFBC in Mesa
(in the Panfrost driver), no open userspace respected modifier
reporting. As such, this change is not expected to affect broken
userspaces.

Tested on RK3399 with Panfrost and Weston.

Signed-off-by: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
---
 drivers/gpu/drm/rockchip/rockchip_drm_vop.h | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
index 4a2099cb5..857d97cdc 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
@@ -17,9 +17,20 @@
 
 #define NUM_YUV2YUV_COEFFICIENTS 12
 
+/* AFBC supports a number of configurable modes. Relevant to us is block size
+ * (16x16 or 32x8), storage modifiers (SPARSE, SPLIT), and the YUV-like
+ * colourspace transform (YTR). 16x16 SPARSE mode is always used. SPLIT mode
+ * could be enabled via the hreg_block_split register, but is not currently
+ * handled. The colourspace transform is implicitly always assumed by the
+ * decoder, so consumers must use this transform as well.
+ *
+ * Failure to match modifiers will cause errors displaying AFBC buffers
+ * produced by conformant AFBC producers, including Mesa.
+ */
 #define ROCKCHIP_AFBC_MOD \
 	DRM_FORMAT_MOD_ARM_AFBC( \
 		AFBC_FORMAT_MOD_BLOCK_SIZE_16x16 | AFBC_FORMAT_MOD_SPARSE \
+			| AFBC_FORMAT_MOD_YTR \
 	)
 
 enum vop_data_format {
-- 
2.28.0


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH] drm/rockchip: Require the YTR modifier for AFBC

[Greg KH]

On Tue, Aug 11, 2020 at 04:26:31PM -0400, Alyssa Rosenzweig wrote:
> The AFBC decoder used in the Rockchip VOP assumes the use of the
> YUV-like colourspace transform (YTR). YTR is lossless for RGB(A)
> buffers, which covers the RGBA8 and RGB565 formats supported in
> vop_convert_afbc_format. Use of YTR is signaled with the
> AFBC_FORMAT_MOD_YTR modifier, which prior to this commit was missing. As
> such, a producer would have to generate buffers that do not use YTR,
> which the VOP would erroneously decode as YTR, leading to severe visual
> corruption.
> 
> The upstream AFBC support was developed against a captured frame, which
> failed to exercise modifier support. Prior to bring-up of AFBC in Mesa
> (in the Panfrost driver), no open userspace respected modifier
> reporting. As such, this change is not expected to affect broken
> userspaces.
> 
> Tested on RK3399 with Panfrost and Weston.
> 
> Signed-off-by: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
> ---
>  drivers/gpu/drm/rockchip/rockchip_drm_vop.h | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
> index 4a2099cb5..857d97cdc 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
> @@ -17,9 +17,20 @@
>  
>  #define NUM_YUV2YUV_COEFFICIENTS 12
>  
> +/* AFBC supports a number of configurable modes. Relevant to us is block size
> + * (16x16 or 32x8), storage modifiers (SPARSE, SPLIT), and the YUV-like
> + * colourspace transform (YTR). 16x16 SPARSE mode is always used. SPLIT mode
> + * could be enabled via the hreg_block_split register, but is not currently
> + * handled. The colourspace transform is implicitly always assumed by the
> + * decoder, so consumers must use this transform as well.
> + *
> + * Failure to match modifiers will cause errors displaying AFBC buffers
> + * produced by conformant AFBC producers, including Mesa.
> + */
>  #define ROCKCHIP_AFBC_MOD \
>  	DRM_FORMAT_MOD_ARM_AFBC( \
>  		AFBC_FORMAT_MOD_BLOCK_SIZE_16x16 | AFBC_FORMAT_MOD_SPARSE \
> +			| AFBC_FORMAT_MOD_YTR \
>  	)
>  
>  enum vop_data_format {
> -- 
> 2.28.0
> 

<formletter>

This is not the correct way to submit patches for inclusion in the
stable kernel tree.  Please read:
    https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly.

</formletter>

Re: [PATCH] drm/rockchip: Require the YTR modifier for AFBC

[Greg KH]

On Tue, Aug 11, 2020 at 04:26:31PM -0400, Alyssa Rosenzweig wrote:
> The AFBC decoder used in the Rockchip VOP assumes the use of the
> YUV-like colourspace transform (YTR). YTR is lossless for RGB(A)
> buffers, which covers the RGBA8 and RGB565 formats supported in
> vop_convert_afbc_format. Use of YTR is signaled with the
> AFBC_FORMAT_MOD_YTR modifier, which prior to this commit was missing. As
> such, a producer would have to generate buffers that do not use YTR,
> which the VOP would erroneously decode as YTR, leading to severe visual
> corruption.
> 
> The upstream AFBC support was developed against a captured frame, which
> failed to exercise modifier support. Prior to bring-up of AFBC in Mesa
> (in the Panfrost driver), no open userspace respected modifier
> reporting. As such, this change is not expected to affect broken
> userspaces.
> 
> Tested on RK3399 with Panfrost and Weston.
> 
> Signed-off-by: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
> ---
>  drivers/gpu/drm/rockchip/rockchip_drm_vop.h | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
> index 4a2099cb5..857d97cdc 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
> @@ -17,9 +17,20 @@
>  
>  #define NUM_YUV2YUV_COEFFICIENTS 12
>  
> +/* AFBC supports a number of configurable modes. Relevant to us is block size
> + * (16x16 or 32x8), storage modifiers (SPARSE, SPLIT), and the YUV-like
> + * colourspace transform (YTR). 16x16 SPARSE mode is always used. SPLIT mode
> + * could be enabled via the hreg_block_split register, but is not currently
> + * handled. The colourspace transform is implicitly always assumed by the
> + * decoder, so consumers must use this transform as well.
> + *
> + * Failure to match modifiers will cause errors displaying AFBC buffers
> + * produced by conformant AFBC producers, including Mesa.
> + */
>  #define ROCKCHIP_AFBC_MOD \
>  	DRM_FORMAT_MOD_ARM_AFBC( \
>  		AFBC_FORMAT_MOD_BLOCK_SIZE_16x16 | AFBC_FORMAT_MOD_SPARSE \
> +			| AFBC_FORMAT_MOD_YTR \
>  	)
>  
>  enum vop_data_format {
> -- 
> 2.28.0
> 

<formletter>

This is not the correct way to submit patches for inclusion in the
stable kernel tree.  Please read:
    https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly.

</formletter>

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH] drm/rockchip: Require the YTR modifier for AFBC

[Daniel Vetter]

On Wed, Aug 12, 2020 at 08:31:54AM +0200, Greg KH wrote:
> On Tue, Aug 11, 2020 at 04:26:31PM -0400, Alyssa Rosenzweig wrote:
> > The AFBC decoder used in the Rockchip VOP assumes the use of the
> > YUV-like colourspace transform (YTR). YTR is lossless for RGB(A)
> > buffers, which covers the RGBA8 and RGB565 formats supported in
> > vop_convert_afbc_format. Use of YTR is signaled with the
> > AFBC_FORMAT_MOD_YTR modifier, which prior to this commit was missing. As
> > such, a producer would have to generate buffers that do not use YTR,
> > which the VOP would erroneously decode as YTR, leading to severe visual
> > corruption.
> > 
> > The upstream AFBC support was developed against a captured frame, which
> > failed to exercise modifier support. Prior to bring-up of AFBC in Mesa
> > (in the Panfrost driver), no open userspace respected modifier
> > reporting. As such, this change is not expected to affect broken
> > userspaces.
> > 
> > Tested on RK3399 with Panfrost and Weston.
> > 
> > Signed-off-by: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
> > ---
> >  drivers/gpu/drm/rockchip/rockchip_drm_vop.h | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
> > index 4a2099cb5..857d97cdc 100644
> > --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
> > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
> > @@ -17,9 +17,20 @@
> >  
> >  #define NUM_YUV2YUV_COEFFICIENTS 12
> >  
> > +/* AFBC supports a number of configurable modes. Relevant to us is block size
> > + * (16x16 or 32x8), storage modifiers (SPARSE, SPLIT), and the YUV-like
> > + * colourspace transform (YTR). 16x16 SPARSE mode is always used. SPLIT mode
> > + * could be enabled via the hreg_block_split register, but is not currently
> > + * handled. The colourspace transform is implicitly always assumed by the
> > + * decoder, so consumers must use this transform as well.
> > + *
> > + * Failure to match modifiers will cause errors displaying AFBC buffers
> > + * produced by conformant AFBC producers, including Mesa.
> > + */
> >  #define ROCKCHIP_AFBC_MOD \
> >  	DRM_FORMAT_MOD_ARM_AFBC( \
> >  		AFBC_FORMAT_MOD_BLOCK_SIZE_16x16 | AFBC_FORMAT_MOD_SPARSE \
> > +			| AFBC_FORMAT_MOD_YTR \
> >  	)
> >  
> >  enum vop_data_format {
> > -- 
> > 2.28.0
> > 
> 
> <formletter>
> 
> This is not the correct way to submit patches for inclusion in the
> stable kernel tree.  Please read:
>     https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
> for how to do this properly.

Greg's bot wants a cc: stable on the commit (i.e. in the commit message),
otherwise it's lost since it doesn't track what's all submitted to it
before it's merged.
-Daniel

> 
> </formletter>

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH] drm/rockchip: Require the YTR modifier for AFBC

[Daniel Stone]

Hi,

On Wed, 12 Aug 2020 at 08:05, Alyssa Rosenzweig
<alyssa.rosenzweig@collabora.com> wrote:
> The AFBC decoder used in the Rockchip VOP assumes the use of the
> YUV-like colourspace transform (YTR). YTR is lossless for RGB(A)
> buffers, which covers the RGBA8 and RGB565 formats supported in
> vop_convert_afbc_format. Use of YTR is signaled with the
> AFBC_FORMAT_MOD_YTR modifier, which prior to this commit was missing. As
> such, a producer would have to generate buffers that do not use YTR,
> which the VOP would erroneously decode as YTR, leading to severe visual
> corruption.
>
> The upstream AFBC support was developed against a captured frame, which
> failed to exercise modifier support. Prior to bring-up of AFBC in Mesa
> (in the Panfrost driver), no open userspace respected modifier
> reporting. As such, this change is not expected to affect broken
> userspaces.
>
> Tested on RK3399 with Panfrost and Weston.

Bumping this one: it seems like the Rockchip VOP either always applies
the YTR transform, or has a YTR control bit which is not documented in
the driver's register definitions. This means that it is incorrect to
advertise the currently-used modifier, which specifies that YTR is
_not_ used, and doing so breaks Panfrost which correctly uses the
modifier as documented. Based on our knowledge of Mali, we believe
that Panfrost is correct, and the error lies with Rockchip erroneously
using the YTR transform in the VOP's AFBC decoder despite declaring
through the modifier that YTR is not in use.

Looking at the downstream vendor tree, VOP2 as used in newer SoCs has
explicit control bits for YTR and other AFBC knobs, but this has been
substantially reworked from the original VOP and is not applicable to
this IP block.

Mark, or others from Rockchip, can you please:
- explain if there is a way to enable/disable the YTR transform in the
VOP's AFBC decoder, similar to the split-block control bit?
- ack this patch which correctly declares that the YTR transform is in
use in order to make Panfrost work, so it can be merged through
drm-misc, or provide another solution which fixes this API mistake?
- if VOP does have a hidden YTR-disable bit, add support to disable
YTR so rockchip-drm can continue advertising the non-YTR modifier, and
Cc this patch for backporting to every kernel tree which declares AFBC
modifier support?

Thanks in advance.

Cheers,
Daniel

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH] drm/rockchip: Require the YTR modifier for AFBC

[Daniel Stone]

Hi,

On Tue, 23 Feb 2021 at 14:27, Daniel Stone <daniel@fooishbar.org> wrote:
> Bumping this one: it seems like the Rockchip VOP either always applies
> the YTR transform, or has a YTR control bit which is not documented in
> the driver's register definitions. This means that it is incorrect to
> advertise the currently-used modifier, which specifies that YTR is
> _not_ used, and doing so breaks Panfrost which correctly uses the
> modifier as documented. Based on our knowledge of Mali, we believe
> that Panfrost is correct, and the error lies with Rockchip erroneously
> using the YTR transform in the VOP's AFBC decoder despite declaring
> through the modifier that YTR is not in use.
>
> Looking at the downstream vendor tree, VOP2 as used in newer SoCs has
> explicit control bits for YTR and other AFBC knobs, but this has been
> substantially reworked from the original VOP and is not applicable to
> this IP block.
>
> Mark, or others from Rockchip, can you please:

Sorry, I meant Sandy rather than Mark! Old habits die hard.

Anyway, this patch itself is:
Acked-by: Daniel Stone <daniels@collabora.com>

Cheers,
Daniel

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH] drm/rockchip: Require the YTR modifier for AFBC

[Brian Starkey]

Hi,

On Tue, Feb 23, 2021 at 02:27:11PM +0000, Daniel Stone wrote:
> Mark, or others from Rockchip, can you please:
> - explain if there is a way to enable/disable the YTR transform in the
> VOP's AFBC decoder, similar to the split-block control bit?
> - ack this patch which correctly declares that the YTR transform is in
> use in order to make Panfrost work, so it can be merged through
> drm-misc, or provide another solution which fixes this API mistake?
> - if VOP does have a hidden YTR-disable bit, add support to disable
> YTR so rockchip-drm can continue advertising the non-YTR modifier, and
> Cc this patch for backporting to every kernel tree which declares AFBC
> modifier support?
> 

Drive-by $0.02:

As described in https://www.kernel.org/doc/Documentation/gpu/afbc.rst,
YTR is only valid for "BGR" component order, so this shouldn't be set
or used for "RGB" order (or YUV) formats. For BGR-order formats, it's
best to always enable YTR to get the best compression ratio.

In an ideal world, there wouldn't be hardware/drivers which
support/allow non-standard encodings, but we don't live in that world
:-(

-Brian

> Thanks in advance.
> 
> Cheers,
> Daniel
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH] drm/rockchip: Require the YTR modifier for AFBC

[Daniel Stone]

On Tue, 23 Feb 2021 at 14:58, Brian Starkey <brian.starkey@arm.com> wrote:
> On Tue, Feb 23, 2021 at 02:27:11PM +0000, Daniel Stone wrote:
> > Mark, or others from Rockchip, can you please:
> > - explain if there is a way to enable/disable the YTR transform in the
> > VOP's AFBC decoder, similar to the split-block control bit?
> > - ack this patch which correctly declares that the YTR transform is in
> > use in order to make Panfrost work, so it can be merged through
> > drm-misc, or provide another solution which fixes this API mistake?
> > - if VOP does have a hidden YTR-disable bit, add support to disable
> > YTR so rockchip-drm can continue advertising the non-YTR modifier, and
> > Cc this patch for backporting to every kernel tree which declares AFBC
> > modifier support?
>
> Drive-by $0.02:
>
> As described in https://www.kernel.org/doc/Documentation/gpu/afbc.rst,
> YTR is only valid for "BGR" component order, so this shouldn't be set
> or used for "RGB" order (or YUV) formats. For BGR-order formats, it's
> best to always enable YTR to get the best compression ratio.
>
> In an ideal world, there wouldn't be hardware/drivers which
> support/allow non-standard encodings, but we don't live in that world
> :-(

This implies that RGB component ordering cannot be used together with
AFBC on RK3399, no?

Cheers,
Daniel

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH] drm/rockchip: Require the YTR modifier for AFBC

[Brian Starkey]

On Tue, Feb 23, 2021 at 03:29:13PM +0000, Daniel Stone wrote:
> On Tue, 23 Feb 2021 at 14:58, Brian Starkey <brian.starkey@arm.com> wrote:
> > On Tue, Feb 23, 2021 at 02:27:11PM +0000, Daniel Stone wrote:
> > > Mark, or others from Rockchip, can you please:
> > > - explain if there is a way to enable/disable the YTR transform in the
> > > VOP's AFBC decoder, similar to the split-block control bit?
> > > - ack this patch which correctly declares that the YTR transform is in
> > > use in order to make Panfrost work, so it can be merged through
> > > drm-misc, or provide another solution which fixes this API mistake?
> > > - if VOP does have a hidden YTR-disable bit, add support to disable
> > > YTR so rockchip-drm can continue advertising the non-YTR modifier, and
> > > Cc this patch for backporting to every kernel tree which declares AFBC
> > > modifier support?
> >
> > Drive-by $0.02:
> >
> > As described in https://www.kernel.org/doc/Documentation/gpu/afbc.rst,
> > YTR is only valid for "BGR" component order, so this shouldn't be set
> > or used for "RGB" order (or YUV) formats. For BGR-order formats, it's
> > best to always enable YTR to get the best compression ratio.
> >
> > In an ideal world, there wouldn't be hardware/drivers which
> > support/allow non-standard encodings, but we don't live in that world
> > :-(
> 
> This implies that RGB component ordering cannot be used together with
> AFBC on RK3399, no?

If YTR can't be turned off, then according to the AFBC spec - correct.

If the hardware allows it to be configured to use YTR with other
component orders, I don't know exactly what the impact would be -
maybe nothing.

I raised some of these concerns when the patches were first sent:
https://lore.kernel.org/dri-devel/20190925093913.z4vduybwcokn3awi@DESKTOP-E1NTVVP.localdomain/

We wrote the .rst doc to try and avoid incompatibilities between
different drivers and parts of the stack. BGR is Arm's preferred order
for AFBC. Unfortunately amongst shifting organisations and priorities,
AFBC in upstream hasn't got much attention.

Cheers,
-Brian

> 
> Cheers,
> Daniel

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH] drm/rockchip: Require the YTR modifier for AFBC

[Alyssa Rosenzweig]

> If YTR can't be turned off, then according to the AFBC spec - correct.

There is no public AFBC spec, so I'm not sure how to respond to this.

> If the hardware allows it to be configured to use YTR with other
> component orders, I don't know exactly what the impact would be -
> maybe nothing.

It's legal to use YTR with a BGR framebuffer regardless of the content
of the framebuffer, yes? Could I postprocess with the following shader?

	void main() {
		vec4 colour = ....;
		gl_FragColor = colour.bgra;
	}

That's just 3D rendering. But now if I feed that rendered "BGR"
framebuffer in I get the illusion of RGB out.

Doing the colourspace conversion in hardware (with the
GPU's component reordering) is mathematically indistinguishable from
rendering BGR with that shader.

I sympathize with reducing AFBC's combinatoric explosion, and I realize
that the Rockchip VOP is probably wrong. I also realize that the
transform is defined for BGR inputs, not RGB ones, so it might be
slightly less effective for real content. But it seems to me allowing
both BGR+YTR and RGB+YTR upstream is the better route than simply
preventing hardware from using AFBC at all, and there are natural
encodings for both with fourcc modifiers.

Maybe there's a deeper reason to require BGR that I'm missing? Please
let me know if I've misunderstood, I only know AFBC from the GPU side.

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH] drm/rockchip: Require the YTR modifier for AFBC

[Brian Starkey]

On Tue, Feb 23, 2021 at 05:10:16PM +0000, Alyssa Rosenzweig wrote:
> > If YTR can't be turned off, then according to the AFBC spec - correct.
> 
> There is no public AFBC spec, so I'm not sure how to respond to this.
> 
> > If the hardware allows it to be configured to use YTR with other
> > component orders, I don't know exactly what the impact would be -
> > maybe nothing.
> 
> It's legal to use YTR with a BGR framebuffer regardless of the content
> of the framebuffer, yes? Could I postprocess with the following shader?
> 
> 	void main() {
> 		vec4 colour = ....;
> 		gl_FragColor = colour.bgra;
> 	}
> 
> That's just 3D rendering. But now if I feed that rendered "BGR"
> framebuffer in I get the illusion of RGB out.
> 
> Doing the colourspace conversion in hardware (with the
> GPU's component reordering) is mathematically indistinguishable from
> rendering BGR with that shader.

I agree that *if* all 2^24 possible values of BGR 8:8:8 can be
perfectly and "invert-ably" converted, then it shouldn't matter what
color channel is in which slot, mathematically speaking.

> 
> I sympathize with reducing AFBC's combinatoric explosion, and I realize
> that the Rockchip VOP is probably wrong. I also realize that the
> transform is defined for BGR inputs, not RGB ones, so it might be
> slightly less effective for real content.

Yes, I think the practical impact is likely to be compression
efficiency.

> But it seems to me allowing
> both BGR+YTR and RGB+YTR upstream is the better route than simply
> preventing hardware from using AFBC at all, and there are natural
> encodings for both with fourcc modifiers.
> 

Are those the only options? I see XBGR8888, ABGR8888, BGR888 and
BGR565 are all handled in vop_convert_afbc_format(), which are all
"valid" for use with YTR, and all except XBGR are on the "preferred"
AFBC format list in afbc.rst.

> Maybe there's a deeper reason to require BGR that I'm missing? Please
> let me know if I've misunderstood, I only know AFBC from the GPU side.

If deeper reason means "a reason that the chip will explode if you use
RGB" then I'm not aware of one.

As you've said above, combinatoric explosion is a real issue with
AFBC, so there's a certain subset which Arm has declared "valid", and
an even smaller subset which forms part of conformance testing
(...also not public). The "valid" usage, and a set of preferred
formats are documented in afbc.rst, which is the closest thing I have
to a public spec.

I was _hoping_ we could limit upstream implementations to at least the
subset which Arm declares "valid", with a preference for the subset
which forms part of the conformance set - because those are the ones
likely to be supported across the widest set of hardware. *But*, I'm
also very aware that waving around "invisible" specifications doesn't
carry much weight upstream versus actual code and hardware _doing
stuff_.

So, all I can really do here is say "spec says this isn't allowed",
and make the point that I expect (though I don't 100% know for sure)
all hardware that supports AFBC to also support BGR order AFBC. With
that in mind, if I were trying to implement AFBC in a way that's
likely to work across multiple platforms, I'd go with BGR order.

Thanks,
-Brian

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH] drm/rockchip: Require the YTR modifier for AFBC

[Daniel Stone]

Hi Brian,

On Tue, 23 Feb 2021 at 18:34, Brian Starkey <brian.starkey@arm.com> wrote:
> On Tue, Feb 23, 2021 at 05:10:16PM +0000, Alyssa Rosenzweig wrote:
> > But it seems to me allowing
> > both BGR+YTR and RGB+YTR upstream is the better route than simply
> > preventing hardware from using AFBC at all, and there are natural
> > encodings for both with fourcc modifiers.
>
> Are those the only options? I see XBGR8888, ABGR8888, BGR888 and
> BGR565 are all handled in vop_convert_afbc_format(), which are all
> "valid" for use with YTR, and all except XBGR are on the "preferred"
> AFBC format list in afbc.rst.

The issue is a userspace one though, not a kernel one. Userspace (e.g.
GNOME Shell, Weston, Xorg) decides ahead of time that it's going to
use XRGB8888, then use the modifiers available to it for that format.
There's no logic in those projects to look at the list of 8bpc
non-alpha formats, examine XRGB vs. XBGR, decide that XBGR is 'better'
since it has more modifiers available, then go use XBGR.

So whilst removing XRGB+AFBC wouldn't technically remove the
possibility to use AFBC, the practical effect is that it wouldn't be
used.

Cheers,
Daniel

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH] drm/rockchip: Require the YTR modifier for AFBC

[Heiko Stuebner]

On Tue, 11 Aug 2020 16:26:31 -0400, Alyssa Rosenzweig wrote:
> The AFBC decoder used in the Rockchip VOP assumes the use of the
> YUV-like colourspace transform (YTR). YTR is lossless for RGB(A)
> buffers, which covers the RGBA8 and RGB565 formats supported in
> vop_convert_afbc_format. Use of YTR is signaled with the
> AFBC_FORMAT_MOD_YTR modifier, which prior to this commit was missing. As
> such, a producer would have to generate buffers that do not use YTR,
> which the VOP would erroneously decode as YTR, leading to severe visual
> corruption.
> 
> [...]

Applied, thanks!

[1/1] drm/rockchip: Require the YTR modifier for AFBC
      commit: 0de764474e6e0a74bd75715fed227d82dcda054c

Best regards,
-- 
Heiko Stuebner <heiko@sntech.de>

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH] drm/rockchip: Require the YTR modifier for AFBC

[Daniel Stone]

On Tue, 23 Feb 2021 at 21:49, Heiko Stuebner <heiko@sntech.de> wrote:
> On Tue, 11 Aug 2020 16:26:31 -0400, Alyssa Rosenzweig wrote:
> > The AFBC decoder used in the Rockchip VOP assumes the use of the
> > YUV-like colourspace transform (YTR). YTR is lossless for RGB(A)
> > buffers, which covers the RGBA8 and RGB565 formats supported in
> > vop_convert_afbc_format. Use of YTR is signaled with the
> > AFBC_FORMAT_MOD_YTR modifier, which prior to this commit was missing. As
> > such, a producer would have to generate buffers that do not use YTR,
> > which the VOP would erroneously decode as YTR, leading to severe visual
> > corruption.
>
> Applied, thanks!
>
> [1/1] drm/rockchip: Require the YTR modifier for AFBC
>       commit: 0de764474e6e0a74bd75715fed227d82dcda054c

Thanks Heiko!

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH] drm/rockchip: Require the YTR modifier for AFBC

[Daniel Vetter]

On Tue, Feb 23, 2021 at 7:50 PM Daniel Stone <daniel@fooishbar.org> wrote:
>
> Hi Brian,
>
> On Tue, 23 Feb 2021 at 18:34, Brian Starkey <brian.starkey@arm.com> wrote:
> > On Tue, Feb 23, 2021 at 05:10:16PM +0000, Alyssa Rosenzweig wrote:
> > > But it seems to me allowing
> > > both BGR+YTR and RGB+YTR upstream is the better route than simply
> > > preventing hardware from using AFBC at all, and there are natural
> > > encodings for both with fourcc modifiers.
> >
> > Are those the only options? I see XBGR8888, ABGR8888, BGR888 and
> > BGR565 are all handled in vop_convert_afbc_format(), which are all
> > "valid" for use with YTR, and all except XBGR are on the "preferred"
> > AFBC format list in afbc.rst.
>
> The issue is a userspace one though, not a kernel one. Userspace (e.g.
> GNOME Shell, Weston, Xorg) decides ahead of time that it's going to
> use XRGB8888, then use the modifiers available to it for that format.
> There's no logic in those projects to look at the list of 8bpc
> non-alpha formats, examine XRGB vs. XBGR, decide that XBGR is 'better'
> since it has more modifiers available, then go use XBGR.

That sounds a bit like userspace being too simple. Since if they're ok
with dealing with modifiers accessing the raw buffer is out the window
anyway, so bgr vs rgb shouldn't matter.

> So whilst removing XRGB+AFBC wouldn't technically remove the
> possibility to use AFBC, the practical effect is that it wouldn't be
> used.

But also this ofc.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH] drm/rockchip: Require the YTR modifier for AFBC

[Daniel Stone]

Hey,

On Wed, 24 Feb 2021 at 14:58, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Tue, Feb 23, 2021 at 7:50 PM Daniel Stone <daniel@fooishbar.org> wrote:
> > The issue is a userspace one though, not a kernel one. Userspace (e.g.
> > GNOME Shell, Weston, Xorg) decides ahead of time that it's going to
> > use XRGB8888, then use the modifiers available to it for that format.
> > There's no logic in those projects to look at the list of 8bpc
> > non-alpha formats, examine XRGB vs. XBGR, decide that XBGR is 'better'
> > since it has more modifiers available, then go use XBGR.
>
> That sounds a bit like userspace being too simple. Since if they're ok
> with dealing with modifiers accessing the raw buffer is out the window
> anyway, so bgr vs rgb shouldn't matter.

Sure, you can add colour-channel permutations, if you have concrete
'rough equivalence' mappings, and you check that between KMS + EGL +
anything external you want to use like PipeWire.

We pretty much punted on that long ago and just use XRGB for
everything. Android punted on that from the beginning and just uses
XBGR for everything ... so it's not really a matter of dumb vs. smart
userspace, just equally dumb userspaces which disagree with each
other. ;)

Cheers,
Daniel

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH] drm/rockchip: Require the YTR modifier for AFBC

[Brian Starkey]

On Wed, Feb 24, 2021 at 04:06:05PM +0000, Daniel Stone wrote:
> Android punted on that from the beginning and just uses
> XBGR for everything ... so it's not really a matter of dumb vs. smart
> userspace, just equally dumb userspaces which disagree with each
> other. ;)

...apart from HAL_PIXEL_FORMAT_RGB_565, which has the opposite
component order despite having the same "name" order. So Android
disagrees with itself, too.

Sigh, pixel formats are awful.

-Brian

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH] drm/rockchip: Require the YTR modifier for AFBC

[Huang Jiachai]

Hi Daniel,

     RK3399 and px30 can support YTR afbc format[RGB only], there is an 
hidden control bit to control this.

Hi Alyssa,

     Can you add the following patch to test on your platform? thanks.

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c 
b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index 99bdb5a2a185..0780ad46230a 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -105,7 +105,7 @@
  #define AFBC_FMT_U8U8U8U8      0x5
  #define AFBC_FMT_U8U8U8                0x4

-#define AFBC_TILE_16x16                BIT(4)
+#define AFBC_FMT_YTR           BIT(4)

  /*
   * The coefficients of the following matrix are all fixed points.
@@ -952,7 +952,9 @@ static void vop_plane_atomic_update(struct drm_plane 
*plane,
         if (rockchip_afbc(fb->modifier)) {
                 int afbc_format = 
vop_convert_afbc_format(fb->format->format);

-               VOP_AFBC_SET(vop, format, afbc_format | AFBC_TILE_16x16);
+               if (fb->modifier & AFBC_FORMAT_MOD_YTR)
+                       afbc_format |= AFBC_FMT_YTR;
+               VOP_AFBC_SET(vop, format, afbc_format);
                 VOP_AFBC_SET(vop, hreg_block_split, 0);
                 VOP_AFBC_SET(vop, win_sel, VOP_WIN_TO_INDEX(vop_win));
                 VOP_AFBC_SET(vop, hdr_ptr, dma_addr);
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h 
b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
index 4a2099cb582e..48e131b88c23 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
@@ -20,6 +20,7 @@
  #define ROCKCHIP_AFBC_MOD \
         DRM_FORMAT_MOD_ARM_AFBC( \
                 AFBC_FORMAT_MOD_BLOCK_SIZE_16x16 | AFBC_FORMAT_MOD_SPARSE \
+                       | AFBC_FORMAT_MOD_YTR \
         )

在 2021/2/23 22:27, Daniel Stone 写道:
> Hi,
>
> On Wed, 12 Aug 2020 at 08:05, Alyssa Rosenzweig
> <alyssa.rosenzweig@collabora.com> wrote:
>> The AFBC decoder used in the Rockchip VOP assumes the use of the
>> YUV-like colourspace transform (YTR). YTR is lossless for RGB(A)
>> buffers, which covers the RGBA8 and RGB565 formats supported in
>> vop_convert_afbc_format. Use of YTR is signaled with the
>> AFBC_FORMAT_MOD_YTR modifier, which prior to this commit was missing. As
>> such, a producer would have to generate buffers that do not use YTR,
>> which the VOP would erroneously decode as YTR, leading to severe visual
>> corruption.
>>
>> The upstream AFBC support was developed against a captured frame, which
>> failed to exercise modifier support. Prior to bring-up of AFBC in Mesa
>> (in the Panfrost driver), no open userspace respected modifier
>> reporting. As such, this change is not expected to affect broken
>> userspaces.
>>
>> Tested on RK3399 with Panfrost and Weston.
> Bumping this one: it seems like the Rockchip VOP either always applies
> the YTR transform, or has a YTR control bit which is not documented in
> the driver's register definitions. This means that it is incorrect to
> advertise the currently-used modifier, which specifies that YTR is
> _not_ used, and doing so breaks Panfrost which correctly uses the
> modifier as documented. Based on our knowledge of Mali, we believe
> that Panfrost is correct, and the error lies with Rockchip erroneously
> using the YTR transform in the VOP's AFBC decoder despite declaring
> through the modifier that YTR is not in use.
>
> Looking at the downstream vendor tree, VOP2 as used in newer SoCs has
> explicit control bits for YTR and other AFBC knobs, but this has been
> substantially reworked from the original VOP and is not applicable to
> this IP block.
>
> Mark, or others from Rockchip, can you please:
> - explain if there is a way to enable/disable the YTR transform in the
> VOP's AFBC decoder, similar to the split-block control bit?
> - ack this patch which correctly declares that the YTR transform is in
> use in order to make Panfrost work, so it can be merged through
> drm-misc, or provide another solution which fixes this API mistake?
> - if VOP does have a hidden YTR-disable bit, add support to disable
> YTR so rockchip-drm can continue advertising the non-YTR modifier, and
> Cc this patch for backporting to every kernel tree which declares AFBC
> modifier support?
>
> Thanks in advance.
>
> Cheers,
> Daniel
>
>
>
-- 
Best Regard

黄家钗
Sandy Huang
Addr: 福州市鼓楼区铜盘路软件大道89号福州软件园A区21号楼(350003)
       No. 21 Building, A District, No.89,software Boulevard 
Fuzhou,Fujian,PRC
Tel：+86 0591-87884919  8690
E-mail：hjc@rock-chips.com




_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH] drm/rockchip: Require the YTR modifier for AFBC

[Daniel Stone]

Hi Sandy,

On Thu, 25 Feb 2021 at 02:17, Huang Jiachai <hjc@rock-chips.com> wrote:
>      RK3399 and px30 can support YTR afbc format[RGB only], there is an
> hidden control bit to control this.

Great, thanks for providing this information!

> Hi Alyssa,
>
>      Can you add the following patch to test on your platform? thanks.
>
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> index 99bdb5a2a185..0780ad46230a 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> @@ -105,7 +105,7 @@
>   #define AFBC_FMT_U8U8U8U8      0x5
>   #define AFBC_FMT_U8U8U8                0x4
>
> -#define AFBC_TILE_16x16                BIT(4)
> +#define AFBC_FMT_YTR           BIT(4)
>
>   /*
>    * The coefficients of the following matrix are all fixed points.
> @@ -952,7 +952,9 @@ static void vop_plane_atomic_update(struct drm_plane
> *plane,
>          if (rockchip_afbc(fb->modifier)) {
>                  int afbc_format =
> vop_convert_afbc_format(fb->format->format);
>
> -               VOP_AFBC_SET(vop, format, afbc_format | AFBC_TILE_16x16);
> +               if (fb->modifier & AFBC_FORMAT_MOD_YTR)
> +                       afbc_format |= AFBC_FMT_YTR;
> +               VOP_AFBC_SET(vop, format, afbc_format);
>                  VOP_AFBC_SET(vop, hreg_block_split, 0);
>                  VOP_AFBC_SET(vop, win_sel, VOP_WIN_TO_INDEX(vop_win));
>                  VOP_AFBC_SET(vop, hdr_ptr, dma_addr);
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
> b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
> index 4a2099cb582e..48e131b88c23 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
> @@ -20,6 +20,7 @@
>   #define ROCKCHIP_AFBC_MOD \
>          DRM_FORMAT_MOD_ARM_AFBC( \
>                  AFBC_FORMAT_MOD_BLOCK_SIZE_16x16 | AFBC_FORMAT_MOD_SPARSE \
> +                       | AFBC_FORMAT_MOD_YTR \
>          )

Looks good - this will help us confirm. I think the complete patch
though would advertise both YTR and non-YTR modifiers: per Arm's
recommendation, it sounds like [AX]RGB8888 formats should only
advertise the non-YTR variant, and [AX]BGR8888 should advertise both
variants. Does that make sense?

Cheers,
Daniel

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH] drm/rockchip: Require the YTR modifier for AFBC【请注意，邮件由linux-rockchip-bounces+sandy.huang=rock-chips.com@lists.infradead.org代发】

[Huang Jiachai]

Hi Daniel,

在 2021/2/25 20:46, Daniel Stone 写道:
> Hi Sandy,
>
> On Thu, 25 Feb 2021 at 02:17, Huang Jiachai <hjc@rock-chips.com> wrote:
>>       RK3399 and px30 can support YTR afbc format[RGB only], there is an
>> hidden control bit to control this.
> Great, thanks for providing this information!
>
>> Hi Alyssa,
>>
>>       Can you add the following patch to test on your platform? thanks.
>>
>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> index 99bdb5a2a185..0780ad46230a 100644
>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> @@ -105,7 +105,7 @@
>>    #define AFBC_FMT_U8U8U8U8      0x5
>>    #define AFBC_FMT_U8U8U8                0x4
>>
>> -#define AFBC_TILE_16x16                BIT(4)
>> +#define AFBC_FMT_YTR           BIT(4)
>>
>>    /*
>>     * The coefficients of the following matrix are all fixed points.
>> @@ -952,7 +952,9 @@ static void vop_plane_atomic_update(struct drm_plane
>> *plane,
>>           if (rockchip_afbc(fb->modifier)) {
>>                   int afbc_format =
>> vop_convert_afbc_format(fb->format->format);
>>
>> -               VOP_AFBC_SET(vop, format, afbc_format | AFBC_TILE_16x16);
>> +               if (fb->modifier & AFBC_FORMAT_MOD_YTR)
>> +                       afbc_format |= AFBC_FMT_YTR;
>> +               VOP_AFBC_SET(vop, format, afbc_format);
>>                   VOP_AFBC_SET(vop, hreg_block_split, 0);
>>                   VOP_AFBC_SET(vop, win_sel, VOP_WIN_TO_INDEX(vop_win));
>>                   VOP_AFBC_SET(vop, hdr_ptr, dma_addr);
>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
>> b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
>> index 4a2099cb582e..48e131b88c23 100644
>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
>> @@ -20,6 +20,7 @@
>>    #define ROCKCHIP_AFBC_MOD \
>>           DRM_FORMAT_MOD_ARM_AFBC( \
>>                   AFBC_FORMAT_MOD_BLOCK_SIZE_16x16 | AFBC_FORMAT_MOD_SPARSE \
>> +                       | AFBC_FORMAT_MOD_YTR \
>>           )
> Looks good - this will help us confirm. I think the complete patch
> though would advertise both YTR and non-YTR modifiers: per Arm's
> recommendation, it sounds like [AX]RGB8888 formats should only
> advertise the non-YTR variant, and [AX]BGR8888 should advertise both
> variants. Does that make sense?

yes, RGB format have YTR and non-YTR variant, YUV format only have 
non-YTR variant.

>
> Cheers,
> Daniel
>
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip
>
>
>



_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH] drm/rockchip: Require the YTR modifier for AFBC

[Huang Jiachai]

Hi Daniel,

在 2021/2/25 20:46, Daniel Stone 写道:
> Hi Sandy,
>
> On Thu, 25 Feb 2021 at 02:17, Huang Jiachai <hjc@rock-chips.com> wrote:
>>       RK3399 and px30 can support YTR afbc format[RGB only], there is an
>> hidden control bit to control this.
> Great, thanks for providing this information!
>
>> Hi Alyssa,
>>
>>       Can you add the following patch to test on your platform? thanks.
>>
>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> index 99bdb5a2a185..0780ad46230a 100644
>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> @@ -105,7 +105,7 @@
>>    #define AFBC_FMT_U8U8U8U8      0x5
>>    #define AFBC_FMT_U8U8U8                0x4
>>
>> -#define AFBC_TILE_16x16                BIT(4)
>> +#define AFBC_FMT_YTR           BIT(4)
>>
>>    /*
>>     * The coefficients of the following matrix are all fixed points.
>> @@ -952,7 +952,9 @@ static void vop_plane_atomic_update(struct drm_plane
>> *plane,
>>           if (rockchip_afbc(fb->modifier)) {
>>                   int afbc_format =
>> vop_convert_afbc_format(fb->format->format);
>>
>> -               VOP_AFBC_SET(vop, format, afbc_format | AFBC_TILE_16x16);
>> +               if (fb->modifier & AFBC_FORMAT_MOD_YTR)
>> +                       afbc_format |= AFBC_FMT_YTR;
>> +               VOP_AFBC_SET(vop, format, afbc_format);
>>                   VOP_AFBC_SET(vop, hreg_block_split, 0);
>>                   VOP_AFBC_SET(vop, win_sel, VOP_WIN_TO_INDEX(vop_win));
>>                   VOP_AFBC_SET(vop, hdr_ptr, dma_addr);
>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
>> b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
>> index 4a2099cb582e..48e131b88c23 100644
>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h
>> @@ -20,6 +20,7 @@
>>    #define ROCKCHIP_AFBC_MOD \
>>           DRM_FORMAT_MOD_ARM_AFBC( \
>>                   AFBC_FORMAT_MOD_BLOCK_SIZE_16x16 | AFBC_FORMAT_MOD_SPARSE \
>> +                       | AFBC_FORMAT_MOD_YTR \
>>           )
> Looks good - this will help us confirm. I think the complete patch
> though would advertise both YTR and non-YTR modifiers: per Arm's
> recommendation, it sounds like [AX]RGB8888 formats should only
> advertise the non-YTR variant, and [AX]BGR8888 should advertise both
> variants. Does that make sense?
yes, RGB format have YTR and non-YTR variant, YUV format only have 
non-YTR variant.
> Cheers,
> Daniel
>
>
>



_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip