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 {
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>
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>
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
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
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
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
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-E1...
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
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.
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
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
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
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
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
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
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 anhidden 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
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 anhidden 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
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,
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!
dri-devel@lists.freedesktop.org
-
Alyssa Rosenzweig -
Alyssa Rosenzweig -
Brian Starkey -
Daniel Stone -
Daniel Vetter -
Greg KH -
Heiko Stuebner -
Huang Jiachai