[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 at 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 at rock-chips.com