* [PATCH 1/7] drm/meson: add AFBC decoder registers for GXM and G12A
2019-10-10 9:25 [PATCH 0/7] drm/meson: add AFBC support Neil Armstrong
@ 2019-10-10 9:25 ` Neil Armstrong
2019-10-10 9:25 ` [PATCH 2/7] drm/meson: store the framebuffer width for plane commit Neil Armstrong
` (5 subsequent siblings)
6 siblings, 0 replies; 22+ messages in thread
From: Neil Armstrong @ 2019-10-10 9:25 UTC (permalink / raw)
To: dri-devel
Cc: khilman, linux-amlogic, ayan.halder, linux-arm-kernel, Neil Armstrong
Add the registers used to program the ARM Framebuffer Compression decoders
used in the Amlogic GXM and G12A SoCs families.
This also adds the routing and pipeline configuration bits and registers
needed to enable AFBC support.
Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
drivers/gpu/drm/meson/meson_registers.h | 61 +++++++++++++++++++++++++
drivers/gpu/drm/meson/meson_viu.h | 15 ++++++
2 files changed, 76 insertions(+)
diff --git a/drivers/gpu/drm/meson/meson_registers.h b/drivers/gpu/drm/meson/meson_registers.h
index 05fce48ceee0..69e566acb8b9 100644
--- a/drivers/gpu/drm/meson/meson_registers.h
+++ b/drivers/gpu/drm/meson/meson_registers.h
@@ -138,14 +138,19 @@
#define VIU_ADDR_START 0x1a00
#define VIU_ADDR_END 0x1aff
#define VIU_SW_RESET 0x1a01
+#define VIU_SW_RESET_OSD1_AFBCD BIT(31)
+#define VIU_SW_RESET_G12A_OSD1_AFBCD BIT(21)
+#define VIU_SW_RESET_G12A_AFBC_ARB BIT(19)
#define VIU_SW_RESET_OSD1 BIT(0)
#define VIU_MISC_CTRL0 0x1a06
#define VIU_CTRL0_VD1_AFBC_MASK 0x170000
#define VIU_MISC_CTRL1 0x1a07
+#define MALI_AFBC_MISC GENMASK(15, 8)
#define D2D3_INTF_LENGTH 0x1a08
#define D2D3_INTF_CTRL0 0x1a09
#define VIU_OSD1_CTRL_STAT 0x1a10
#define VIU_OSD1_OSD_BLK_ENABLE BIT(0)
+#define VIU_OSD1_OSD_MEM_MODE_LINEAR BIT(2)
#define VIU_OSD1_POSTBLD_SRC_VD1 (1 << 8)
#define VIU_OSD1_POSTBLD_SRC_VD2 (2 << 8)
#define VIU_OSD1_POSTBLD_SRC_OSD1 (3 << 8)
@@ -181,6 +186,16 @@
#define VIU_OSD1_FIFO_CTRL_STAT 0x1a2b
#define VIU_OSD1_TEST_RDDATA 0x1a2c
#define VIU_OSD1_PROT_CTRL 0x1a2e
+#define VIU_OSD1_MALI_UNPACK_CTRL 0x1a2f
+#define VIU_OSD1_MALI_UNPACK_EN BIT(31)
+#define VIU_OSD1_MALI_AFBCD_R_REORDER GENMASK(15, 12)
+#define VIU_OSD1_MALI_AFBCD_G_REORDER GENMASK(11, 8)
+#define VIU_OSD1_MALI_AFBCD_B_REORDER GENMASK(7, 4)
+#define VIU_OSD1_MALI_AFBCD_A_REORDER GENMASK(3, 0)
+#define VIU_OSD1_MALI_REORDER_R 1
+#define VIU_OSD1_MALI_REORDER_G 2
+#define VIU_OSD1_MALI_REORDER_B 3
+#define VIU_OSD1_MALI_REORDER_A 4
#define VIU_OSD2_CTRL_STAT 0x1a30
#define VIU_OSD2_CTRL_STAT2 0x1a4d
#define VIU_OSD2_COLOR_ADDR 0x1a31
@@ -1595,15 +1610,33 @@
/* osd afbcd on gxtvbb */
#define OSD1_AFBCD_ENABLE 0x31a0
+#define OSD1_AFBCD_ID_FIFO_THRD GENMASK(15, 9)
+#define OSD1_AFBCD_DEC_ENABLE BIT(8)
+#define OSD1_AFBCD_FRM_START BIT(0)
#define OSD1_AFBCD_MODE 0x31a1
+#define OSD1_AFBCD_SOFT_RESET BIT(31)
+#define OSD1_AFBCD_AXI_REORDER_MODE BIT(28)
+#define OSD1_AFBCD_MIF_URGENT GENMASK(25, 24)
+#define OSD1_AFBCD_HOLD_LINE_NUM GENMASK(22, 16)
+#define OSD1_AFBCD_RGBA_EXCHAN_CTRL GENMASK(15, 8)
+#define OSD1_AFBCD_HREG_BLOCK_SPLIT BIT(6)
+#define OSD1_AFBCD_HREG_HALF_BLOCK BIT(5)
+#define OSD1_AFBCD_HREG_PIXEL_PACKING_FMT GENMASK(4, 0)
#define OSD1_AFBCD_SIZE_IN 0x31a2
+#define OSD1_AFBCD_HREG_VSIZE_IN GENMASK(31, 16)
+#define OSD1_AFBCD_HREG_HSIZE_IN GENMASK(15, 0)
#define OSD1_AFBCD_HDR_PTR 0x31a3
#define OSD1_AFBCD_FRAME_PTR 0x31a4
#define OSD1_AFBCD_CHROMA_PTR 0x31a5
#define OSD1_AFBCD_CONV_CTRL 0x31a6
+#define OSD1_AFBCD_CONV_LBUF_LEN GENMASK(15, 0)
#define OSD1_AFBCD_STATUS 0x31a8
#define OSD1_AFBCD_PIXEL_HSCOPE 0x31a9
+#define OSD1_AFBCD_DEC_PIXEL_BGN_H GENMASK(31, 16)
+#define OSD1_AFBCD_DEC_PIXEL_END_H GENMASK(15, 0)
#define OSD1_AFBCD_PIXEL_VSCOPE 0x31aa
+#define OSD1_AFBCD_DEC_PIXEL_BGN_V GENMASK(31, 16)
+#define OSD1_AFBCD_DEC_PIXEL_END_V GENMASK(15, 0)
/* add for gxm and 962e dv core2 */
#define DOLBY_CORE2A_SWAP_CTRL1 0x3434
@@ -1615,12 +1648,34 @@
#define VPU_MAFBC_IRQ_CLEAR 0x3a02
#define VPU_MAFBC_IRQ_MASK 0x3a03
#define VPU_MAFBC_IRQ_STATUS 0x3a04
+#define VPU_MAFBC_IRQ_SECURE_ID_ERROR BIT(5)
+#define VPU_MAFBC_IRQ_AXI_ERROR BIT(4)
+#define VPU_MAFBC_IRQ_DETILING_ERROR BIT(3)
+#define VPU_MAFBC_IRQ_DECODE_ERROR BIT(2)
+#define VPU_MAFBC_IRQ_CONFIGURATION_SWAPPED BIT(1)
+#define VPU_MAFBC_IRQ_SURFACES_COMPLETED BIT(0)
#define VPU_MAFBC_COMMAND 0x3a05
+#define VPU_MAFBC_PENDING_SWAP BIT(1)
+#define VPU_MAFBC_DIRECT_SWAP BIT(0)
#define VPU_MAFBC_STATUS 0x3a06
+#define VPU_MAFBC_ERROR BIT(2)
+#define VPU_MAFBC_SWAPPING BIT(1)
+#define VPU_MAFBC_ACTIVE BIT(0)
#define VPU_MAFBC_SURFACE_CFG 0x3a07
+#define VPU_MAFBC_CONTINUOUS_DECODING_ENABLE BIT(16)
+#define VPU_MAFBC_S3_ENABLE BIT(3)
+#define VPU_MAFBC_S2_ENABLE BIT(2)
+#define VPU_MAFBC_S1_ENABLE BIT(1)
+#define VPU_MAFBC_S0_ENABLE BIT(0)
#define VPU_MAFBC_HEADER_BUF_ADDR_LOW_S0 0x3a10
#define VPU_MAFBC_HEADER_BUF_ADDR_HIGH_S0 0x3a11
#define VPU_MAFBC_FORMAT_SPECIFIER_S0 0x3a12
+#define VPU_MAFBC_PAYLOAD_LIMIT_EN BIT(19)
+#define VPU_MAFBC_TILED_HEADER_EN BIT(18)
+#define VPU_MAFBC_SUPER_BLOCK_ASPECT GENMASK(17, 16)
+#define VPU_MAFBC_BLOCK_SPLIT BIT(9)
+#define VPU_MAFBC_YUV_TRANSFORM BIT(8)
+#define VPU_MAFBC_PIXEL_FORMAT GENMASK(3, 0)
#define VPU_MAFBC_BUFFER_WIDTH_S0 0x3a13
#define VPU_MAFBC_BUFFER_HEIGHT_S0 0x3a14
#define VPU_MAFBC_BOUNDING_BOX_X_START_S0 0x3a15
@@ -1631,6 +1686,8 @@
#define VPU_MAFBC_OUTPUT_BUF_ADDR_HIGH_S0 0x3a1a
#define VPU_MAFBC_OUTPUT_BUF_STRIDE_S0 0x3a1b
#define VPU_MAFBC_PREFETCH_CFG_S0 0x3a1c
+#define VPU_MAFBC_PREFETCH_READ_DIRECTION_Y BIT(1)
+#define VPU_MAFBC_PREFETCH_READ_DIRECTION_X BIT(0)
#define VPU_MAFBC_HEADER_BUF_ADDR_LOW_S1 0x3a30
#define VPU_MAFBC_HEADER_BUF_ADDR_HIGH_S1 0x3a31
@@ -1677,7 +1734,11 @@
#define DOLBY_PATH_CTRL 0x1a0c
#define DOLBY_BYPASS_EN(val) (val & 0xf)
#define OSD_PATH_MISC_CTRL 0x1a0e
+#define OSD_PATH_OSD_AXI_SEL_OSD1_AFBCD BIT(4)
+#define OSD_PATH_OSD_AXI_SEL_OSD2_AFBCD BIT(5)
+#define OSD_PATH_OSD_AXI_SEL_OSD3_AFBCD BIT(6)
#define MALI_AFBCD_TOP_CTRL 0x1a0f
+#define MALI_AFBCD_MANUAL_RESET BIT(23)
#define VIU_OSD_BLEND_CTRL 0x39b0
#define VIU_OSD_BLEND_REORDER(dest, src) ((src) << (dest * 4))
diff --git a/drivers/gpu/drm/meson/meson_viu.h b/drivers/gpu/drm/meson/meson_viu.h
index a112e8d18850..e297772d967f 100644
--- a/drivers/gpu/drm/meson/meson_viu.h
+++ b/drivers/gpu/drm/meson/meson_viu.h
@@ -10,6 +10,8 @@
#define __MESON_VIU_H
/* OSDx_BLKx_CFG */
+#define OSD_MALI_SRC_EN BIT(30)
+
#define OSD_CANVAS_SEL 16
#define OSD_ENDIANNESS_LE BIT(15)
@@ -33,19 +35,32 @@
#define OSD_COLOR_MATRIX_16_RGB655 (0x00 << 2)
#define OSD_COLOR_MATRIX_16_RGB565 (0x04 << 2)
+#define OSD_MALI_COLOR_MODE_R8 (0 << 8)
+#define OSD_MALI_COLOR_MODE_YUV422 (1 << 8)
+#define OSD_MALI_COLOR_MODE_RGB565 (2 << 8)
+#define OSD_MALI_COLOR_MODE_RGBA5551 (3 << 8)
+#define OSD_MALI_COLOR_MODE_RGBA4444 (4 << 8)
+#define OSD_MALI_COLOR_MODE_RGBA8888 (5 << 8)
+#define OSD_MALI_COLOR_MODE_RGB888 (7 << 8)
+#define OSD_MALI_COLOR_MODE_YUV422_10B (8 << 8)
+#define OSD_MALI_COLOR_MODE_RGBA1010102 (9 << 8)
+
#define OSD_INTERLACE_ENABLED BIT(1)
#define OSD_INTERLACE_ODD BIT(0)
#define OSD_INTERLACE_EVEN (0)
/* OSDx_CTRL_STAT */
#define OSD_ENABLE BIT(21)
+#define OSD_MEM_LINEAR_ADDR BIT(2)
#define OSD_BLK0_ENABLE BIT(0)
#define OSD_GLOBAL_ALPHA_SHIFT 12
/* OSDx_CTRL_STAT2 */
+#define OSD_DPATH_MALI_AFBCD BIT(15)
#define OSD_REPLACE_EN BIT(14)
#define OSD_REPLACE_SHIFT 6
+#define OSD_PENDING_STAT_CLEAN BIT(1)
void meson_viu_osd1_reset(struct meson_drm *priv);
void meson_viu_init(struct meson_drm *priv);
--
2.22.0
_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH 2/7] drm/meson: store the framebuffer width for plane commit
2019-10-10 9:25 [PATCH 0/7] drm/meson: add AFBC support Neil Armstrong
2019-10-10 9:25 ` [PATCH 1/7] drm/meson: add AFBC decoder registers for GXM and G12A Neil Armstrong
@ 2019-10-10 9:25 ` Neil Armstrong
2019-10-10 9:25 ` [PATCH 3/7] drm/meson: Add AFBCD module driver Neil Armstrong
` (4 subsequent siblings)
6 siblings, 0 replies; 22+ messages in thread
From: Neil Armstrong @ 2019-10-10 9:25 UTC (permalink / raw)
To: dri-devel
Cc: khilman, linux-amlogic, ayan.halder, linux-arm-kernel, Neil Armstrong
Also store the framebuffer width in the private common struct
to be used by the AFBC decoder module driver when committing the AFBC
plane.
Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
drivers/gpu/drm/meson/meson_drv.h | 1 +
drivers/gpu/drm/meson/meson_plane.c | 1 +
2 files changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/meson/meson_drv.h b/drivers/gpu/drm/meson/meson_drv.h
index 820d07bdd42a..3287282450a9 100644
--- a/drivers/gpu/drm/meson/meson_drv.h
+++ b/drivers/gpu/drm/meson/meson_drv.h
@@ -52,6 +52,7 @@ struct meson_drm {
uint32_t osd1_addr;
uint32_t osd1_stride;
uint32_t osd1_height;
+ uint32_t osd1_width;
uint32_t osd_sc_ctrl0;
uint32_t osd_sc_i_wh_m1;
uint32_t osd_sc_o_h_start_end;
diff --git a/drivers/gpu/drm/meson/meson_plane.c b/drivers/gpu/drm/meson/meson_plane.c
index ed543227b00d..5e798c276037 100644
--- a/drivers/gpu/drm/meson/meson_plane.c
+++ b/drivers/gpu/drm/meson/meson_plane.c
@@ -305,6 +305,7 @@ static void meson_plane_atomic_update(struct drm_plane *plane,
priv->viu.osd1_addr = gem->paddr;
priv->viu.osd1_stride = fb->pitches[0];
priv->viu.osd1_height = fb->height;
+ priv->viu.osd1_width = fb->width;
if (!meson_plane->enabled) {
/* Reset OSD1 before enabling it on GXL+ SoCs */
--
2.22.0
_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH 3/7] drm/meson: Add AFBCD module driver
2019-10-10 9:25 [PATCH 0/7] drm/meson: add AFBC support Neil Armstrong
2019-10-10 9:25 ` [PATCH 1/7] drm/meson: add AFBC decoder registers for GXM and G12A Neil Armstrong
2019-10-10 9:25 ` [PATCH 2/7] drm/meson: store the framebuffer width for plane commit Neil Armstrong
@ 2019-10-10 9:25 ` Neil Armstrong
2019-10-10 9:25 ` [PATCH 4/7] drm/meson: plane: add support for AFBC mode for OSD1 plane Neil Armstrong
` (3 subsequent siblings)
6 siblings, 0 replies; 22+ messages in thread
From: Neil Armstrong @ 2019-10-10 9:25 UTC (permalink / raw)
To: dri-devel
Cc: khilman, linux-amlogic, ayan.halder, linux-arm-kernel, Neil Armstrong
This adds the driver for the ARM Framebuffer Compression decoders found
in the Amlogic GXM and G12A SoCs.
The Amlogic GXM and G12A AFBC decoder are totally different, the GXM only
handling only the AFBC v1.0 modes and the G12A decoder handling the
AFBC v1.2 modes.
The G12A AFBC decoder is an external IP integrated in the video pipeline,
and the GXM AFBC decoder seems to the an Amlogic custom decoder more
tighly integrated in the video pipeline.
The GXM AFBC decoder can handle only one AFBC plane for 2 available
OSD planes available in HW, and the G12A AFBC decoder can handle up
to 4 AFBC planes for up to 3 OSD planes available in HW.
The Amlogic GXM supports 16x16 SPARSE and 16x16 SPLIT AFBC buffers up
to 4k.
On the other side, for G12A SPLIT is mandatory in 16x16 block mode, but
for 4k modes 32x8+SPLIT AFBC buffers is manadatory for performances reasons.
Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
drivers/gpu/drm/meson/Makefile | 1 +
drivers/gpu/drm/meson/meson_drv.c | 38 ++-
drivers/gpu/drm/meson/meson_drv.h | 12 +
drivers/gpu/drm/meson/meson_osd_afbcd.c | 370 ++++++++++++++++++++++++
drivers/gpu/drm/meson/meson_osd_afbcd.h | 28 ++
5 files changed, 443 insertions(+), 6 deletions(-)
create mode 100644 drivers/gpu/drm/meson/meson_osd_afbcd.c
create mode 100644 drivers/gpu/drm/meson/meson_osd_afbcd.h
diff --git a/drivers/gpu/drm/meson/Makefile b/drivers/gpu/drm/meson/Makefile
index c389e2399133..b1fa055aaed3 100644
--- a/drivers/gpu/drm/meson/Makefile
+++ b/drivers/gpu/drm/meson/Makefile
@@ -1,6 +1,7 @@
# SPDX-License-Identifier: GPL-2.0-only
meson-drm-y := meson_drv.o meson_plane.o meson_crtc.o meson_venc_cvbs.o
meson-drm-y += meson_viu.o meson_vpp.o meson_venc.o meson_vclk.o meson_overlay.o
+meson-drm-y += meson_osd_afbcd.o
obj-$(CONFIG_DRM_MESON) += meson-drm.o
obj-$(CONFIG_DRM_MESON_DW_HDMI) += meson_dw_hdmi.o
diff --git a/drivers/gpu/drm/meson/meson_drv.c b/drivers/gpu/drm/meson/meson_drv.c
index a24f8dec5adc..0f31e70bb94f 100644
--- a/drivers/gpu/drm/meson/meson_drv.c
+++ b/drivers/gpu/drm/meson/meson_drv.c
@@ -28,6 +28,7 @@
#include "meson_drv.h"
#include "meson_overlay.h"
#include "meson_plane.h"
+#include "meson_osd_afbcd.h"
#include "meson_registers.h"
#include "meson_venc_cvbs.h"
#include "meson_viu.h"
@@ -184,6 +185,7 @@ static void meson_remove_framebuffers(void)
static int meson_drv_bind_master(struct device *dev, bool has_components)
{
struct platform_device *pdev = to_platform_device(dev);
+ const struct meson_drm_match_data *match;
struct meson_drm *priv;
struct drm_device *drm;
struct resource *res;
@@ -196,6 +198,10 @@ static int meson_drv_bind_master(struct device *dev, bool has_components)
return -ENODEV;
}
+ match = of_device_get_match_data(dev);
+ if (!match)
+ return -ENODEV;
+
drm = drm_dev_alloc(&meson_driver, dev);
if (IS_ERR(drm))
return PTR_ERR(drm);
@@ -208,8 +214,8 @@ static int meson_drv_bind_master(struct device *dev, bool has_components)
drm->dev_private = priv;
priv->drm = drm;
priv->dev = dev;
-
- priv->compat = (enum vpu_compatible)of_device_get_match_data(priv->dev);
+ priv->compat = match->compat;
+ priv->afbcd.ops = match->afbcd_ops;
res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "vpu");
regs = devm_ioremap_resource(dev, res);
@@ -289,6 +295,8 @@ static int meson_drv_bind_master(struct device *dev, bool has_components)
meson_venc_init(priv);
meson_vpp_init(priv);
meson_viu_init(priv);
+ if (priv->afbcd.ops)
+ priv->afbcd.ops->init(priv);
/* Encoder Initialization */
@@ -454,15 +462,33 @@ static int meson_drv_probe(struct platform_device *pdev)
return 0;
};
+static struct meson_drm_match_data meson_drm_gxbb_data = {
+ .compat = VPU_COMPATIBLE_GXBB,
+};
+
+static struct meson_drm_match_data meson_drm_gxl_data = {
+ .compat = VPU_COMPATIBLE_GXL,
+};
+
+static struct meson_drm_match_data meson_drm_gxm_data = {
+ .compat = VPU_COMPATIBLE_GXM,
+ .afbcd_ops = &meson_afbcd_gxm_ops,
+};
+
+static struct meson_drm_match_data meson_drm_g12a_data = {
+ .compat = VPU_COMPATIBLE_G12A,
+ .afbcd_ops = &meson_afbcd_g12a_ops,
+};
+
static const struct of_device_id dt_match[] = {
{ .compatible = "amlogic,meson-gxbb-vpu",
- .data = (void *)VPU_COMPATIBLE_GXBB },
+ .data = (void *)&meson_drm_gxbb_data },
{ .compatible = "amlogic,meson-gxl-vpu",
- .data = (void *)VPU_COMPATIBLE_GXL },
+ .data = (void *)&meson_drm_gxl_data },
{ .compatible = "amlogic,meson-gxm-vpu",
- .data = (void *)VPU_COMPATIBLE_GXM },
+ .data = (void *)&meson_drm_gxm_data },
{ .compatible = "amlogic,meson-g12a-vpu",
- .data = (void *)VPU_COMPATIBLE_G12A },
+ .data = (void *)&meson_drm_g12a_data },
{}
};
MODULE_DEVICE_TABLE(of, dt_match);
diff --git a/drivers/gpu/drm/meson/meson_drv.h b/drivers/gpu/drm/meson/meson_drv.h
index 3287282450a9..60f13c6f34e5 100644
--- a/drivers/gpu/drm/meson/meson_drv.h
+++ b/drivers/gpu/drm/meson/meson_drv.h
@@ -16,6 +16,7 @@ struct drm_crtc;
struct drm_device;
struct drm_plane;
struct meson_drm;
+struct meson_afbcd_ops;
enum vpu_compatible {
VPU_COMPATIBLE_GXBB = 0,
@@ -24,6 +25,11 @@ enum vpu_compatible {
VPU_COMPATIBLE_G12A = 3,
};
+struct meson_drm_match_data {
+ enum vpu_compatible compat;
+ struct meson_afbcd_ops *afbcd_ops;
+};
+
struct meson_drm {
struct device *dev;
enum vpu_compatible compat;
@@ -123,6 +129,12 @@ struct meson_drm {
bool venc_repeat;
bool hdmi_use_enci;
} venc;
+
+ struct {
+ struct meson_afbcd_ops *ops;
+ u64 modifier;
+ u32 format;
+ } afbcd;
};
static inline int meson_vpu_is_compatible(struct meson_drm *priv,
diff --git a/drivers/gpu/drm/meson/meson_osd_afbcd.c b/drivers/gpu/drm/meson/meson_osd_afbcd.c
new file mode 100644
index 000000000000..36caf5716e8c
--- /dev/null
+++ b/drivers/gpu/drm/meson/meson_osd_afbcd.c
@@ -0,0 +1,370 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2019 BayLibre, SAS
+ * Author: Neil Armstrong <narmstrong@baylibre.com>
+ */
+
+#include <linux/bitfield.h>
+
+#include <drm/drm_print.h>
+#include <drm/drm_fourcc.h>
+
+#include "meson_drv.h"
+#include "meson_registers.h"
+#include "meson_viu.h"
+#include "meson_osd_afbcd.h"
+
+/*
+ * DOC: Driver for the ARM FrameBuffer Compression Decoders
+ *
+ * The Amlogic GXM and G12A SoC families embeds an AFBC Decoder,
+ * to decode compressed buffers generated by the ARM Mali GPU.
+ *
+ * For the GXM Family, Amlogic designed their own Decoder, named in
+ * the vendor source as "MESON_AFBC", and a single decoder is available
+ * for the 2 OSD planes.
+ * This decoder is compatible with the AFBC 1.0 specifications and the
+ * Mali T820 GPU capabilities.
+ * It supports :
+ * - basic AFBC buffer for RGB32 only, thus YTR feature is mandatory
+ * - SPARSE layout and SPLIT layout
+ * - only 16x16 superblock
+ *
+ * The decoder reads the data from the SDRAM, decodes and sends the
+ * decoded pixel stream to the OSD1 Plane pixel composer.
+ *
+ * For the G12A Family, Amlogic integrated an ARM AFBC Decoder, named
+ * in the vendor source as "MALI_AFBC", and the decoder can decode up
+ * to 4 surfaces, one for each of the 4 available OSDs.
+ * This decoder is compatible with the AFBC 1.2 specifications for the
+ * Mali G31 and G52 GPUs.
+ * Is supports :
+ * - basic AFBC buffer for multiple RGB and YUV pixel formats
+ * - SPARSE layout and SPLIT layout
+ * - 16x16 and 32x8 "wideblk" superblocks
+ * - Tiled header
+ *
+ * The ARM AFBC Decoder independent from the VPU Pixel Pipeline, so
+ * the ARM AFBC Decoder reads the data from the SDRAM then decodes
+ * into a private internal physical address where the OSD1 Plane pixel
+ * composer unpacks the decoded data.
+ */
+
+/* Amlogic AFBC Decoder for GXM Family */
+
+#define OSD1_AFBCD_RGB32 0x15
+
+static int meson_gxm_afbcd_pixel_fmt(u64 modifier, uint32_t format)
+{
+ switch (format) {
+ case DRM_FORMAT_XRGB8888:
+ case DRM_FORMAT_ARGB8888:
+ case DRM_FORMAT_XBGR8888:
+ case DRM_FORMAT_ABGR8888:
+ return OSD1_AFBCD_RGB32;
+ /* TOFIX support mode formats */
+ default:
+ DRM_DEBUG("unsupported afbc format[%08x]\n", format);
+ return -EINVAL;
+ }
+}
+
+static bool meson_gxm_afbcd_supported_fmt(u64 modifier, uint32_t format)
+{
+ if (modifier & AFBC_FORMAT_MOD_BLOCK_SIZE_32x8)
+ return false;
+
+ if (!(modifier & AFBC_FORMAT_MOD_YTR))
+ return false;
+
+ return meson_gxm_afbcd_pixel_fmt(modifier, format) >= 0;
+}
+
+static int meson_gxm_afbcd_init(struct meson_drm *priv)
+{
+ return 0;
+}
+
+static int meson_gxm_afbcd_reset(struct meson_drm *priv)
+{
+ writel_relaxed(VIU_SW_RESET_OSD1_AFBCD,
+ priv->io_base + _REG(VIU_SW_RESET));
+ writel_relaxed(0, priv->io_base + _REG(VIU_SW_RESET));
+
+ return 0;
+}
+
+static int meson_gxm_afbcd_enable(struct meson_drm *priv)
+{
+ writel_relaxed(FIELD_PREP(OSD1_AFBCD_ID_FIFO_THRD, 0x40) |
+ OSD1_AFBCD_DEC_ENABLE,
+ priv->io_base + _REG(OSD1_AFBCD_ENABLE));
+
+ return 0;
+}
+
+static int meson_gxm_afbcd_disable(struct meson_drm *priv)
+{
+ writel_bits_relaxed(OSD1_AFBCD_DEC_ENABLE, 0,
+ priv->io_base + _REG(OSD1_AFBCD_ENABLE));
+
+ return 0;
+}
+
+static int meson_gxm_afbcd_setup(struct meson_drm *priv)
+{
+ u32 conv_lbuf_len;
+ u32 mode = FIELD_PREP(OSD1_AFBCD_MIF_URGENT, 3) |
+ FIELD_PREP(OSD1_AFBCD_HOLD_LINE_NUM, 4) |
+ FIELD_PREP(OSD1_AFBCD_RGBA_EXCHAN_CTRL, 0x34) |
+ meson_gxm_afbcd_pixel_fmt(priv->afbcd.modifier,
+ priv->afbcd.format);
+
+ if (priv->afbcd.modifier & AFBC_FORMAT_MOD_SPARSE)
+ mode |= OSD1_AFBCD_HREG_HALF_BLOCK;
+
+ if (priv->afbcd.modifier & AFBC_FORMAT_MOD_SPLIT)
+ mode |= OSD1_AFBCD_HREG_BLOCK_SPLIT;
+
+ writel_relaxed(mode, priv->io_base + _REG(OSD1_AFBCD_MODE));
+
+ writel_relaxed(FIELD_PREP(OSD1_AFBCD_HREG_VSIZE_IN,
+ priv->viu.osd1_width) |
+ FIELD_PREP(OSD1_AFBCD_HREG_HSIZE_IN,
+ priv->viu.osd1_height),
+ priv->io_base + _REG(OSD1_AFBCD_SIZE_IN));
+
+ writel_relaxed(priv->viu.osd1_addr >> 4,
+ priv->io_base + _REG(OSD1_AFBCD_HDR_PTR));
+ writel_relaxed(priv->viu.osd1_addr >> 4,
+ priv->io_base + _REG(OSD1_AFBCD_FRAME_PTR));
+ /* TOFIX: bits 31:24 are not documented, nor the meaning of 0xe4 */
+ writel_relaxed((0xe4 << 24) | (priv->viu.osd1_addr & 0xffffff),
+ priv->io_base + _REG(OSD1_AFBCD_CHROMA_PTR));
+
+ if (priv->viu.osd1_width <= 128)
+ conv_lbuf_len = 32;
+ else if (priv->viu.osd1_width <= 256)
+ conv_lbuf_len = 64;
+ else if (priv->viu.osd1_width <= 512)
+ conv_lbuf_len = 128;
+ else if (priv->viu.osd1_width <= 1024)
+ conv_lbuf_len = 256;
+ else if (priv->viu.osd1_width <= 2048)
+ conv_lbuf_len = 512;
+ else
+ conv_lbuf_len = 1024;
+
+ writel_relaxed(conv_lbuf_len,
+ priv->io_base + _REG(OSD1_AFBCD_CONV_CTRL));
+
+ writel_relaxed(FIELD_PREP(OSD1_AFBCD_DEC_PIXEL_BGN_H, 0) |
+ FIELD_PREP(OSD1_AFBCD_DEC_PIXEL_END_H,
+ priv->viu.osd1_width - 1),
+ priv->io_base + _REG(OSD1_AFBCD_PIXEL_HSCOPE));
+
+ writel_relaxed(FIELD_PREP(OSD1_AFBCD_DEC_PIXEL_BGN_V, 0) |
+ FIELD_PREP(OSD1_AFBCD_DEC_PIXEL_END_V,
+ priv->viu.osd1_height - 1),
+ priv->io_base + _REG(OSD1_AFBCD_PIXEL_VSCOPE));
+
+ return 0;
+}
+
+struct meson_afbcd_ops meson_afbcd_gxm_ops = {
+ .init = meson_gxm_afbcd_init,
+ .reset = meson_gxm_afbcd_reset,
+ .enable = meson_gxm_afbcd_enable,
+ .disable = meson_gxm_afbcd_disable,
+ .setup = meson_gxm_afbcd_setup,
+ .supported_fmt = meson_gxm_afbcd_supported_fmt,
+};
+
+/* ARM AFBC Decoder for G12A Family */
+
+/* Amlogic G12A Mali AFBC Decoder supported formats */
+enum {
+ MAFBC_FMT_RGB565 = 0,
+ MAFBC_FMT_RGBA5551,
+ MAFBC_FMT_RGBA1010102,
+ MAFBC_FMT_YUV420_10B,
+ MAFBC_FMT_RGB888,
+ MAFBC_FMT_RGBA8888,
+ MAFBC_FMT_RGBA4444,
+ MAFBC_FMT_R8,
+ MAFBC_FMT_RG88,
+ MAFBC_FMT_YUV420_8B,
+ MAFBC_FMT_YUV422_8B = 11,
+ MAFBC_FMT_YUV422_10B = 14,
+};
+
+static int meson_g12a_afbcd_pixel_fmt(u64 modifier, uint32_t format)
+{
+ switch (format) {
+ case DRM_FORMAT_XRGB8888:
+ case DRM_FORMAT_ARGB8888:
+ case DRM_FORMAT_XBGR8888:
+ case DRM_FORMAT_ABGR8888:
+ return MAFBC_FMT_RGBA8888;
+ case DRM_FORMAT_RGB888:
+ return MAFBC_FMT_RGB888;
+ case DRM_FORMAT_RGB565:
+ return MAFBC_FMT_RGB565;
+ /* TOFIX support mode formats */
+ default:
+ DRM_DEBUG("unsupported afbc format[%08x]\n", format);
+ return -EINVAL;
+ }
+}
+
+static int meson_g12a_afbcd_bpp(uint32_t format)
+{
+ switch (format) {
+ case DRM_FORMAT_XRGB8888:
+ case DRM_FORMAT_ARGB8888:
+ case DRM_FORMAT_XBGR8888:
+ case DRM_FORMAT_ABGR8888:
+ return 32;
+ case DRM_FORMAT_RGB888:
+ return 24;
+ case DRM_FORMAT_RGB565:
+ return 16;
+ /* TOFIX support mode formats */
+ default:
+ DRM_ERROR("unsupported afbc format[%08x]\n", format);
+ return 0;
+ }
+}
+
+static int meson_g12a_afbcd_fmt_to_blk_mode(u64 modifier, uint32_t format)
+{
+ switch (format) {
+ case DRM_FORMAT_XRGB8888:
+ case DRM_FORMAT_ARGB8888:
+ case DRM_FORMAT_XBGR8888:
+ case DRM_FORMAT_ABGR8888:
+ return OSD_MALI_COLOR_MODE_RGBA8888;
+ case DRM_FORMAT_RGB888:
+ return OSD_MALI_COLOR_MODE_RGB888;
+ case DRM_FORMAT_RGB565:
+ return OSD_MALI_COLOR_MODE_RGB565;
+ /* TOFIX support mode formats */
+ default:
+ DRM_DEBUG("unsupported afbc format[%08x]\n", format);
+ return -EINVAL;
+ }
+}
+
+static bool meson_g12a_afbcd_supported_fmt(u64 modifier, uint32_t format)
+{
+ if (!(modifier & AFBC_FORMAT_MOD_YTR))
+ return false;
+
+ return meson_g12a_afbcd_pixel_fmt(modifier, format) >= 0;
+}
+
+static int meson_g12a_afbcd_init(struct meson_drm *priv)
+{
+ /* Handle AFBC Decoder reset manually */
+ writel_bits_relaxed(MALI_AFBCD_MANUAL_RESET, MALI_AFBCD_MANUAL_RESET,
+ priv->io_base + _REG(MALI_AFBCD_TOP_CTRL));
+
+ return 0;
+}
+
+static int meson_g12a_afbcd_reset(struct meson_drm *priv)
+{
+ writel_relaxed(VIU_SW_RESET_G12A_AFBC_ARB |
+ VIU_SW_RESET_G12A_OSD1_AFBCD,
+ priv->io_base + _REG(VIU_SW_RESET));
+ writel_relaxed(0, priv->io_base + _REG(VIU_SW_RESET));
+
+ return 0;
+}
+
+static int meson_g12a_afbcd_enable(struct meson_drm *priv)
+{
+ writel_relaxed(VPU_MAFBC_IRQ_SURFACES_COMPLETED |
+ VPU_MAFBC_IRQ_CONFIGURATION_SWAPPED |
+ VPU_MAFBC_IRQ_DECODE_ERROR |
+ VPU_MAFBC_IRQ_DETILING_ERROR,
+ priv->io_base + _REG(VPU_MAFBC_IRQ_MASK));
+
+ writel_bits_relaxed(VPU_MAFBC_S0_ENABLE, VPU_MAFBC_S0_ENABLE,
+ priv->io_base + _REG(VPU_MAFBC_SURFACE_CFG));
+
+ writel_relaxed(VPU_MAFBC_DIRECT_SWAP,
+ priv->io_base + _REG(VPU_MAFBC_COMMAND));
+
+ return 0;
+}
+
+static int meson_g12a_afbcd_disable(struct meson_drm *priv)
+{
+ writel_bits_relaxed(VPU_MAFBC_S0_ENABLE, 0,
+ priv->io_base + _REG(VPU_MAFBC_SURFACE_CFG));
+
+ return 0;
+}
+
+static int meson_g12a_afbcd_setup(struct meson_drm *priv)
+{
+ u32 format = meson_g12a_afbcd_pixel_fmt(priv->afbcd.modifier,
+ priv->afbcd.format);
+
+ if (priv->afbcd.modifier & AFBC_FORMAT_MOD_YTR)
+ format |= VPU_MAFBC_YUV_TRANSFORM;
+
+ if (priv->afbcd.modifier & AFBC_FORMAT_MOD_SPLIT)
+ format |= VPU_MAFBC_BLOCK_SPLIT;
+
+ if (priv->afbcd.modifier & AFBC_FORMAT_MOD_TILED)
+ format |= VPU_MAFBC_TILED_HEADER_EN;
+
+ if ((priv->afbcd.modifier & AFBC_FORMAT_MOD_BLOCK_SIZE_MASK) ==
+ AFBC_FORMAT_MOD_BLOCK_SIZE_32x8)
+ format |= FIELD_PREP(VPU_MAFBC_SUPER_BLOCK_ASPECT, 1);
+
+ writel_relaxed(format,
+ priv->io_base + _REG(VPU_MAFBC_FORMAT_SPECIFIER_S0));
+
+ writel_relaxed(priv->viu.osd1_addr,
+ priv->io_base + _REG(VPU_MAFBC_HEADER_BUF_ADDR_LOW_S0));
+ writel_relaxed(0,
+ priv->io_base + _REG(VPU_MAFBC_HEADER_BUF_ADDR_HIGH_S0));
+
+ writel_relaxed(priv->viu.osd1_width,
+ priv->io_base + _REG(VPU_MAFBC_BUFFER_WIDTH_S0));
+ writel_relaxed(ALIGN(priv->viu.osd1_height, 32),
+ priv->io_base + _REG(VPU_MAFBC_BUFFER_HEIGHT_S0));
+
+ writel_relaxed(0,
+ priv->io_base + _REG(VPU_MAFBC_BOUNDING_BOX_X_START_S0));
+ writel_relaxed(priv->viu.osd1_width - 1,
+ priv->io_base + _REG(VPU_MAFBC_BOUNDING_BOX_X_END_S0));
+ writel_relaxed(0,
+ priv->io_base + _REG(VPU_MAFBC_BOUNDING_BOX_Y_START_S0));
+ writel_relaxed(priv->viu.osd1_height - 1,
+ priv->io_base + _REG(VPU_MAFBC_BOUNDING_BOX_Y_END_S0));
+
+ writel_relaxed(MESON_G12A_AFBCD_OUT_ADDR,
+ priv->io_base + _REG(VPU_MAFBC_OUTPUT_BUF_ADDR_LOW_S0));
+ writel_relaxed(0,
+ priv->io_base + _REG(VPU_MAFBC_OUTPUT_BUF_ADDR_HIGH_S0));
+
+ writel_relaxed(priv->viu.osd1_width *
+ (meson_g12a_afbcd_bpp(priv->afbcd.format) / 8),
+ priv->io_base + _REG(VPU_MAFBC_OUTPUT_BUF_STRIDE_S0));
+
+ return 0;
+}
+
+struct meson_afbcd_ops meson_afbcd_g12a_ops = {
+ .init = meson_g12a_afbcd_init,
+ .reset = meson_g12a_afbcd_reset,
+ .enable = meson_g12a_afbcd_enable,
+ .disable = meson_g12a_afbcd_disable,
+ .setup = meson_g12a_afbcd_setup,
+ .fmt_to_blk_mode = meson_g12a_afbcd_fmt_to_blk_mode,
+ .supported_fmt = meson_g12a_afbcd_supported_fmt,
+};
diff --git a/drivers/gpu/drm/meson/meson_osd_afbcd.h b/drivers/gpu/drm/meson/meson_osd_afbcd.h
new file mode 100644
index 000000000000..5e5523304f42
--- /dev/null
+++ b/drivers/gpu/drm/meson/meson_osd_afbcd.h
@@ -0,0 +1,28 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Copyright (C) 2019 BayLibre, SAS
+ * Author: Neil Armstrong <narmstrong@baylibre.com>
+ */
+
+#ifndef __MESON_OSD_AFBCD_H
+#define __MESON_OSD_AFBCD_H
+
+#include "meson_drv.h"
+
+/* This is an internal address used to transfer pixel from AFBC to the VIU */
+#define MESON_G12A_AFBCD_OUT_ADDR 0x1000000
+
+struct meson_afbcd_ops {
+ int (*init)(struct meson_drm *priv);
+ int (*reset)(struct meson_drm *priv);
+ int (*enable)(struct meson_drm *priv);
+ int (*disable)(struct meson_drm *priv);
+ int (*setup)(struct meson_drm *priv);
+ int (*fmt_to_blk_mode)(u64 modifier, uint32_t format);
+ bool (*supported_fmt)(u64 modifier, uint32_t format);
+};
+
+extern struct meson_afbcd_ops meson_afbcd_gxm_ops;
+extern struct meson_afbcd_ops meson_afbcd_g12a_ops;
+
+#endif /* __MESON_OSD_AFBCD_H */
--
2.22.0
_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH 4/7] drm/meson: plane: add support for AFBC mode for OSD1 plane
2019-10-10 9:25 [PATCH 0/7] drm/meson: add AFBC support Neil Armstrong
` (2 preceding siblings ...)
2019-10-10 9:25 ` [PATCH 3/7] drm/meson: Add AFBCD module driver Neil Armstrong
@ 2019-10-10 9:25 ` Neil Armstrong
2019-10-10 13:26 ` Ayan Halder
2019-10-10 9:25 ` [PATCH 5/7] drm/meson: viu: add AFBC modules routing functions Neil Armstrong
` (2 subsequent siblings)
6 siblings, 1 reply; 22+ messages in thread
From: Neil Armstrong @ 2019-10-10 9:25 UTC (permalink / raw)
To: dri-devel
Cc: khilman, linux-amlogic, ayan.halder, linux-arm-kernel, Neil Armstrong
This adds all the OSD configuration plumbing to support the AFBC decoders
path to display of the OSD1 plane.
The Amlogic GXM and G12A AFBC decoders are integrated very differently.
The Amlogic GXM has a direct output path to the OSD1 VIU pixel input,
because the GXM AFBC decoder seem to be a custom IP developed by Amlogic.
On the other side, the Amlogic G12A AFBC decoder seems to be an external
IP that emit pixels on an AXI master hooked to a "Mali Unpack" block
feeding the OSD1 VIU pixel input.
This uses a weird "0x1000000" internal HW physical address on both
sides to transfer the pixels.
For Amlogic GXM, the supported pixel formats are the same as the normal
linear OSD1 mode.
On the other side, Amlogic added support for all AFBC v1.2 formats for
the G12A AFBC integration.
For simplicity, we stick to the already supported formats for now.
Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
drivers/gpu/drm/meson/meson_crtc.c | 2 +
drivers/gpu/drm/meson/meson_drv.h | 4 +
drivers/gpu/drm/meson/meson_plane.c | 215 ++++++++++++++++++++++++----
3 files changed, 190 insertions(+), 31 deletions(-)
diff --git a/drivers/gpu/drm/meson/meson_crtc.c b/drivers/gpu/drm/meson/meson_crtc.c
index 57ae1c13d1e6..d478fa232951 100644
--- a/drivers/gpu/drm/meson/meson_crtc.c
+++ b/drivers/gpu/drm/meson/meson_crtc.c
@@ -281,6 +281,8 @@ void meson_crtc_irq(struct meson_drm *priv)
if (priv->viu.osd1_enabled && priv->viu.osd1_commit) {
writel_relaxed(priv->viu.osd1_ctrl_stat,
priv->io_base + _REG(VIU_OSD1_CTRL_STAT));
+ writel_relaxed(priv->viu.osd1_ctrl_stat2,
+ priv->io_base + _REG(VIU_OSD1_CTRL_STAT2));
writel_relaxed(priv->viu.osd1_blk0_cfg[0],
priv->io_base + _REG(VIU_OSD1_BLK0_CFG_W0));
writel_relaxed(priv->viu.osd1_blk0_cfg[1],
diff --git a/drivers/gpu/drm/meson/meson_drv.h b/drivers/gpu/drm/meson/meson_drv.h
index 60f13c6f34e5..de25349be8aa 100644
--- a/drivers/gpu/drm/meson/meson_drv.h
+++ b/drivers/gpu/drm/meson/meson_drv.h
@@ -53,8 +53,12 @@ struct meson_drm {
bool osd1_enabled;
bool osd1_interlace;
bool osd1_commit;
+ bool osd1_afbcd;
uint32_t osd1_ctrl_stat;
+ uint32_t osd1_ctrl_stat2;
uint32_t osd1_blk0_cfg[5];
+ uint32_t osd1_blk1_cfg4;
+ uint32_t osd1_blk2_cfg4;
uint32_t osd1_addr;
uint32_t osd1_stride;
uint32_t osd1_height;
diff --git a/drivers/gpu/drm/meson/meson_plane.c b/drivers/gpu/drm/meson/meson_plane.c
index 5e798c276037..412941aa8402 100644
--- a/drivers/gpu/drm/meson/meson_plane.c
+++ b/drivers/gpu/drm/meson/meson_plane.c
@@ -23,6 +23,7 @@
#include "meson_plane.h"
#include "meson_registers.h"
#include "meson_viu.h"
+#include "meson_osd_afbcd.h"
/* OSD_SCI_WH_M1 */
#define SCI_WH_M1_W(w) FIELD_PREP(GENMASK(28, 16), w)
@@ -92,12 +93,38 @@ static int meson_plane_atomic_check(struct drm_plane *plane,
false, true);
}
+#define MESON_MOD_AFBC_VALID_BITS (AFBC_FORMAT_MOD_BLOCK_SIZE_16x16 | \
+ AFBC_FORMAT_MOD_BLOCK_SIZE_32x8 | \
+ AFBC_FORMAT_MOD_YTR | \
+ AFBC_FORMAT_MOD_SPARSE | \
+ AFBC_FORMAT_MOD_SPLIT)
+
/* Takes a fixed 16.16 number and converts it to integer. */
static inline int64_t fixed16_to_int(int64_t value)
{
return value >> 16;
}
+static u32 meson_g12a_afbcd_line_stride(struct meson_drm *priv)
+{
+ u32 line_stride = 0;
+
+ switch (priv->afbcd.format) {
+ case DRM_FORMAT_RGB565:
+ line_stride = ((priv->viu.osd1_width << 4) + 127) >> 7;
+ break;
+ case DRM_FORMAT_RGB888:
+ case DRM_FORMAT_XRGB8888:
+ case DRM_FORMAT_ARGB8888:
+ case DRM_FORMAT_XBGR8888:
+ case DRM_FORMAT_ABGR8888:
+ line_stride = ((priv->viu.osd1_width << 5) + 127) >> 7;
+ break;
+ }
+
+ return ((line_stride + 1) >> 1) << 1;
+}
+
static void meson_plane_atomic_update(struct drm_plane *plane,
struct drm_plane_state *old_state)
{
@@ -126,57 +153,88 @@ static void meson_plane_atomic_update(struct drm_plane *plane,
*/
spin_lock_irqsave(&priv->drm->event_lock, flags);
+ /* Check if AFBC decoder is required for this buffer */
+ if ((meson_vpu_is_compatible(priv, VPU_COMPATIBLE_GXM) ||
+ meson_vpu_is_compatible(priv, VPU_COMPATIBLE_G12A)) &&
+ fb->modifier & DRM_FORMAT_MOD_ARM_AFBC(MESON_MOD_AFBC_VALID_BITS))
+ priv->viu.osd1_afbcd = true;
+ else
+ priv->viu.osd1_afbcd = false;
+
/* Enable OSD and BLK0, set max global alpha */
priv->viu.osd1_ctrl_stat = OSD_ENABLE |
(0xFF << OSD_GLOBAL_ALPHA_SHIFT) |
OSD_BLK0_ENABLE;
+ priv->viu.osd1_ctrl_stat2 = readl(priv->io_base +
+ _REG(VIU_OSD1_CTRL_STAT2));
+
canvas_id_osd1 = priv->canvas_id_osd1;
/* Set up BLK0 to point to the right canvas */
- priv->viu.osd1_blk0_cfg[0] = ((canvas_id_osd1 << OSD_CANVAS_SEL) |
- OSD_ENDIANNESS_LE);
+ priv->viu.osd1_blk0_cfg[0] = canvas_id_osd1 << OSD_CANVAS_SEL;
+
+ if (priv->viu.osd1_afbcd) {
+ if (meson_vpu_is_compatible(priv, VPU_COMPATIBLE_G12A)) {
+ /* This is the internal decoding memory address */
+ priv->viu.osd1_blk1_cfg4 = MESON_G12A_AFBCD_OUT_ADDR;
+ priv->viu.osd1_blk0_cfg[0] |= OSD_ENDIANNESS_BE;
+ priv->viu.osd1_ctrl_stat2 |= OSD_PENDING_STAT_CLEAN;
+ }
+
+ if (meson_vpu_is_compatible(priv, VPU_COMPATIBLE_GXM)) {
+ priv->viu.osd1_blk0_cfg[0] |= OSD_ENDIANNESS_LE;
+ priv->viu.osd1_ctrl_stat2 |= OSD_DPATH_MALI_AFBCD;
+ }
+ } else {
+ priv->viu.osd1_blk0_cfg[0] |= OSD_ENDIANNESS_LE;
+
+ if (meson_vpu_is_compatible(priv, VPU_COMPATIBLE_GXM))
+ priv->viu.osd1_ctrl_stat2 &= ~OSD_DPATH_MALI_AFBCD;
+ }
/* On GXBB, Use the old non-HDR RGB2YUV converter */
if (meson_vpu_is_compatible(priv, VPU_COMPATIBLE_GXBB))
priv->viu.osd1_blk0_cfg[0] |= OSD_OUTPUT_COLOR_RGB;
+ if (priv->viu.osd1_afbcd &&
+ meson_vpu_is_compatible(priv, VPU_COMPATIBLE_G12A)) {
+ priv->viu.osd1_blk0_cfg[0] |= OSD_MALI_SRC_EN |
+ priv->afbcd.ops->fmt_to_blk_mode(fb->modifier,
+ fb->format->format);
+ } else {
+ switch (fb->format->format) {
+ case DRM_FORMAT_XRGB8888:
+ case DRM_FORMAT_ARGB8888:
+ priv->viu.osd1_blk0_cfg[0] |= OSD_BLK_MODE_32 |
+ OSD_COLOR_MATRIX_32_ARGB;
+ break;
+ case DRM_FORMAT_XBGR8888:
+ case DRM_FORMAT_ABGR8888:
+ priv->viu.osd1_blk0_cfg[0] |= OSD_BLK_MODE_32 |
+ OSD_COLOR_MATRIX_32_ABGR;
+ break;
+ case DRM_FORMAT_RGB888:
+ priv->viu.osd1_blk0_cfg[0] |= OSD_BLK_MODE_24 |
+ OSD_COLOR_MATRIX_24_RGB;
+ break;
+ case DRM_FORMAT_RGB565:
+ priv->viu.osd1_blk0_cfg[0] |= OSD_BLK_MODE_16 |
+ OSD_COLOR_MATRIX_16_RGB565;
+ break;
+ };
+ }
+
switch (fb->format->format) {
case DRM_FORMAT_XRGB8888:
- /* For XRGB, replace the pixel's alpha by 0xFF */
- writel_bits_relaxed(OSD_REPLACE_EN, OSD_REPLACE_EN,
- priv->io_base + _REG(VIU_OSD1_CTRL_STAT2));
- priv->viu.osd1_blk0_cfg[0] |= OSD_BLK_MODE_32 |
- OSD_COLOR_MATRIX_32_ARGB;
- break;
case DRM_FORMAT_XBGR8888:
/* For XRGB, replace the pixel's alpha by 0xFF */
- writel_bits_relaxed(OSD_REPLACE_EN, OSD_REPLACE_EN,
- priv->io_base + _REG(VIU_OSD1_CTRL_STAT2));
- priv->viu.osd1_blk0_cfg[0] |= OSD_BLK_MODE_32 |
- OSD_COLOR_MATRIX_32_ABGR;
+ priv->viu.osd1_ctrl_stat2 |= OSD_REPLACE_EN;
break;
case DRM_FORMAT_ARGB8888:
- /* For ARGB, use the pixel's alpha */
- writel_bits_relaxed(OSD_REPLACE_EN, 0,
- priv->io_base + _REG(VIU_OSD1_CTRL_STAT2));
- priv->viu.osd1_blk0_cfg[0] |= OSD_BLK_MODE_32 |
- OSD_COLOR_MATRIX_32_ARGB;
- break;
case DRM_FORMAT_ABGR8888:
/* For ARGB, use the pixel's alpha */
- writel_bits_relaxed(OSD_REPLACE_EN, 0,
- priv->io_base + _REG(VIU_OSD1_CTRL_STAT2));
- priv->viu.osd1_blk0_cfg[0] |= OSD_BLK_MODE_32 |
- OSD_COLOR_MATRIX_32_ABGR;
- break;
- case DRM_FORMAT_RGB888:
- priv->viu.osd1_blk0_cfg[0] |= OSD_BLK_MODE_24 |
- OSD_COLOR_MATRIX_24_RGB;
- break;
- case DRM_FORMAT_RGB565:
- priv->viu.osd1_blk0_cfg[0] |= OSD_BLK_MODE_16 |
- OSD_COLOR_MATRIX_16_RGB565;
+ priv->viu.osd1_ctrl_stat2 &= ~OSD_REPLACE_EN;
break;
};
@@ -307,6 +365,16 @@ static void meson_plane_atomic_update(struct drm_plane *plane,
priv->viu.osd1_height = fb->height;
priv->viu.osd1_width = fb->width;
+ if (priv->viu.osd1_afbcd) {
+ priv->afbcd.modifier = fb->modifier;
+ priv->afbcd.format = fb->format->format;
+
+ /* Calculate decoder write stride */
+ if (meson_vpu_is_compatible(priv, VPU_COMPATIBLE_G12A))
+ priv->viu.osd1_blk2_cfg4 =
+ meson_g12a_afbcd_line_stride(priv);
+ }
+
if (!meson_plane->enabled) {
/* Reset OSD1 before enabling it on GXL+ SoCs */
if (meson_vpu_is_compatible(priv, VPU_COMPATIBLE_GXM) ||
@@ -346,6 +414,42 @@ static const struct drm_plane_helper_funcs meson_plane_helper_funcs = {
.prepare_fb = drm_gem_fb_prepare_fb,
};
+static bool meson_plane_format_mod_supported(struct drm_plane *plane,
+ u32 format, u64 modifier)
+{
+ struct meson_plane *meson_plane = to_meson_plane(plane);
+ struct meson_drm *priv = meson_plane->priv;
+ int i;
+
+ if (modifier == DRM_FORMAT_MOD_INVALID)
+ return false;
+
+ if (modifier == DRM_FORMAT_MOD_LINEAR)
+ return true;
+
+ if (!meson_vpu_is_compatible(priv, VPU_COMPATIBLE_GXM) &&
+ !meson_vpu_is_compatible(priv, VPU_COMPATIBLE_G12A))
+ return false;
+
+ if (modifier & ~DRM_FORMAT_MOD_ARM_AFBC(MESON_MOD_AFBC_VALID_BITS))
+ return false;
+
+ for (i = 0 ; i < plane->modifier_count ; ++i)
+ if (plane->modifiers[i] == modifier)
+ break;
+
+ if (i == plane->modifier_count) {
+ DRM_DEBUG_KMS("Unsupported modifier\n");
+ return false;
+ }
+
+ if (priv->afbcd.ops && priv->afbcd.ops->supported_fmt)
+ return priv->afbcd.ops->supported_fmt(modifier, format);
+
+ DRM_DEBUG_KMS("AFBC Unsupported\n");
+ return false;
+}
+
static const struct drm_plane_funcs meson_plane_funcs = {
.update_plane = drm_atomic_helper_update_plane,
.disable_plane = drm_atomic_helper_disable_plane,
@@ -353,6 +457,7 @@ static const struct drm_plane_funcs meson_plane_funcs = {
.reset = drm_atomic_helper_plane_reset,
.atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
.atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
+ .format_mod_supported = meson_plane_format_mod_supported,
};
static const uint32_t supported_drm_formats[] = {
@@ -364,10 +469,53 @@ static const uint32_t supported_drm_formats[] = {
DRM_FORMAT_RGB565,
};
+static const uint64_t format_modifiers_afbc_gxm[] = {
+ DRM_FORMAT_MOD_ARM_AFBC(AFBC_FORMAT_MOD_BLOCK_SIZE_16x16 |
+ AFBC_FORMAT_MOD_SPARSE |
+ AFBC_FORMAT_MOD_YTR),
+ /* SPLIT mandates SPARSE, RGB modes mandates YTR */
+ DRM_FORMAT_MOD_ARM_AFBC(AFBC_FORMAT_MOD_BLOCK_SIZE_16x16 |
+ AFBC_FORMAT_MOD_YTR |
+ AFBC_FORMAT_MOD_SPARSE |
+ AFBC_FORMAT_MOD_SPLIT),
+ DRM_FORMAT_MOD_LINEAR,
+ DRM_FORMAT_MOD_INVALID,
+};
+
+static const uint64_t format_modifiers_afbc_g12a[] = {
+ /*
+ * - TOFIX Support AFBC modifiers for YUV formats (16x16 + TILED)
+ * - AFBC_FORMAT_MOD_YTR is mandatory since we only support RGB
+ * - SPLIT is mandatory for performances reasons when in 16x16
+ * block size
+ * - 32x8 block size + SPLIT is mandatory with 4K frame size
+ * for performances reasons
+ */
+ DRM_FORMAT_MOD_ARM_AFBC(AFBC_FORMAT_MOD_BLOCK_SIZE_16x16 |
+ AFBC_FORMAT_MOD_YTR |
+ AFBC_FORMAT_MOD_SPARSE |
+ AFBC_FORMAT_MOD_SPLIT),
+ DRM_FORMAT_MOD_ARM_AFBC(AFBC_FORMAT_MOD_BLOCK_SIZE_32x8 |
+ AFBC_FORMAT_MOD_YTR |
+ AFBC_FORMAT_MOD_SPARSE),
+ DRM_FORMAT_MOD_ARM_AFBC(AFBC_FORMAT_MOD_BLOCK_SIZE_32x8 |
+ AFBC_FORMAT_MOD_YTR |
+ AFBC_FORMAT_MOD_SPARSE |
+ AFBC_FORMAT_MOD_SPLIT),
+ DRM_FORMAT_MOD_LINEAR,
+ DRM_FORMAT_MOD_INVALID,
+};
+
+static const uint64_t format_modifiers_default[] = {
+ DRM_FORMAT_MOD_LINEAR,
+ DRM_FORMAT_MOD_INVALID,
+};
+
int meson_plane_create(struct meson_drm *priv)
{
struct meson_plane *meson_plane;
struct drm_plane *plane;
+ const uint64_t *format_modifiers = format_modifiers_default;
meson_plane = devm_kzalloc(priv->drm->dev, sizeof(*meson_plane),
GFP_KERNEL);
@@ -377,11 +525,16 @@ int meson_plane_create(struct meson_drm *priv)
meson_plane->priv = priv;
plane = &meson_plane->base;
+ if (meson_vpu_is_compatible(priv, VPU_COMPATIBLE_GXM))
+ format_modifiers = format_modifiers_afbc_gxm;
+ else if (meson_vpu_is_compatible(priv, VPU_COMPATIBLE_G12A))
+ format_modifiers = format_modifiers_afbc_g12a;
+
drm_universal_plane_init(priv->drm, plane, 0xFF,
&meson_plane_funcs,
supported_drm_formats,
ARRAY_SIZE(supported_drm_formats),
- NULL,
+ format_modifiers,
DRM_PLANE_TYPE_PRIMARY, "meson_primary_plane");
drm_plane_helper_add(plane, &meson_plane_helper_funcs);
--
2.22.0
_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH 4/7] drm/meson: plane: add support for AFBC mode for OSD1 plane
2019-10-10 9:25 ` [PATCH 4/7] drm/meson: plane: add support for AFBC mode for OSD1 plane Neil Armstrong
@ 2019-10-10 13:26 ` Ayan Halder
2019-10-10 13:41 ` Neil Armstrong
0 siblings, 1 reply; 22+ messages in thread
From: Ayan Halder @ 2019-10-10 13:26 UTC (permalink / raw)
To: Neil Armstrong; +Cc: khilman, nd, linux-arm-kernel, dri-devel, linux-amlogic
On Thu, Oct 10, 2019 at 11:25:23AM +0200, Neil Armstrong wrote:
> This adds all the OSD configuration plumbing to support the AFBC decoders
> path to display of the OSD1 plane.
>
> The Amlogic GXM and G12A AFBC decoders are integrated very differently.
>
> The Amlogic GXM has a direct output path to the OSD1 VIU pixel input,
> because the GXM AFBC decoder seem to be a custom IP developed by Amlogic.
>
> On the other side, the Amlogic G12A AFBC decoder seems to be an external
> IP that emit pixels on an AXI master hooked to a "Mali Unpack" block
> feeding the OSD1 VIU pixel input.
> This uses a weird "0x1000000" internal HW physical address on both
> sides to transfer the pixels.
>
> For Amlogic GXM, the supported pixel formats are the same as the normal
> linear OSD1 mode.
>
> On the other side, Amlogic added support for all AFBC v1.2 formats for
> the G12A AFBC integration.
>
> For simplicity, we stick to the already supported formats for now.
>
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
> drivers/gpu/drm/meson/meson_crtc.c | 2 +
> drivers/gpu/drm/meson/meson_drv.h | 4 +
> drivers/gpu/drm/meson/meson_plane.c | 215 ++++++++++++++++++++++++----
> 3 files changed, 190 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/gpu/drm/meson/meson_crtc.c b/drivers/gpu/drm/meson/meson_crtc.c
> index 57ae1c13d1e6..d478fa232951 100644
> --- a/drivers/gpu/drm/meson/meson_crtc.c
> +++ b/drivers/gpu/drm/meson/meson_crtc.c
> @@ -281,6 +281,8 @@ void meson_crtc_irq(struct meson_drm *priv)
> if (priv->viu.osd1_enabled && priv->viu.osd1_commit) {
> writel_relaxed(priv->viu.osd1_ctrl_stat,
> priv->io_base + _REG(VIU_OSD1_CTRL_STAT));
> + writel_relaxed(priv->viu.osd1_ctrl_stat2,
> + priv->io_base + _REG(VIU_OSD1_CTRL_STAT2));
> writel_relaxed(priv->viu.osd1_blk0_cfg[0],
> priv->io_base + _REG(VIU_OSD1_BLK0_CFG_W0));
> writel_relaxed(priv->viu.osd1_blk0_cfg[1],
> diff --git a/drivers/gpu/drm/meson/meson_drv.h b/drivers/gpu/drm/meson/meson_drv.h
> index 60f13c6f34e5..de25349be8aa 100644
> --- a/drivers/gpu/drm/meson/meson_drv.h
> +++ b/drivers/gpu/drm/meson/meson_drv.h
> @@ -53,8 +53,12 @@ struct meson_drm {
> bool osd1_enabled;
> bool osd1_interlace;
> bool osd1_commit;
> + bool osd1_afbcd;
> uint32_t osd1_ctrl_stat;
> + uint32_t osd1_ctrl_stat2;
> uint32_t osd1_blk0_cfg[5];
> + uint32_t osd1_blk1_cfg4;
> + uint32_t osd1_blk2_cfg4;
> uint32_t osd1_addr;
> uint32_t osd1_stride;
> uint32_t osd1_height;
> diff --git a/drivers/gpu/drm/meson/meson_plane.c b/drivers/gpu/drm/meson/meson_plane.c
> index 5e798c276037..412941aa8402 100644
> --- a/drivers/gpu/drm/meson/meson_plane.c
> +++ b/drivers/gpu/drm/meson/meson_plane.c
> @@ -23,6 +23,7 @@
> #include "meson_plane.h"
> #include "meson_registers.h"
> #include "meson_viu.h"
> +#include "meson_osd_afbcd.h"
>
> /* OSD_SCI_WH_M1 */
> #define SCI_WH_M1_W(w) FIELD_PREP(GENMASK(28, 16), w)
> @@ -92,12 +93,38 @@ static int meson_plane_atomic_check(struct drm_plane *plane,
> false, true);
> }
>
> +#define MESON_MOD_AFBC_VALID_BITS (AFBC_FORMAT_MOD_BLOCK_SIZE_16x16 | \
> + AFBC_FORMAT_MOD_BLOCK_SIZE_32x8 | \
> + AFBC_FORMAT_MOD_YTR | \
> + AFBC_FORMAT_MOD_SPARSE | \
> + AFBC_FORMAT_MOD_SPLIT)
> +
> /* Takes a fixed 16.16 number and converts it to integer. */
> static inline int64_t fixed16_to_int(int64_t value)
> {
> return value >> 16;
> }
>
> +static u32 meson_g12a_afbcd_line_stride(struct meson_drm *priv)
> +{
> + u32 line_stride = 0;
> +
> + switch (priv->afbcd.format) {
> + case DRM_FORMAT_RGB565:
> + line_stride = ((priv->viu.osd1_width << 4) + 127) >> 7;
> + break;
> + case DRM_FORMAT_RGB888:
> + case DRM_FORMAT_XRGB8888:
> + case DRM_FORMAT_ARGB8888:
> + case DRM_FORMAT_XBGR8888:
> + case DRM_FORMAT_ABGR8888:
Please have a look at
https://www.kernel.org/doc/html/latest/gpu/afbc.html for our
recommendation. We suggest that *X* formats are avoided.
Also, for interoperability and maximum compression efficiency (with
AFBC_FORMAT_MOD_YTR), we suggest the following order :-
Component 0: R
Component 1: G
Component 2: B
Component 3: A (if available)
Thus, DRM_FORMAT_ABGR, DRM_FORMAT_BGR should only be allowed.
> + line_stride = ((priv->viu.osd1_width << 5) + 127) >> 7;
> + break;
> + }
> +
> + return ((line_stride + 1) >> 1) << 1;
> +}
> +
> static void meson_plane_atomic_update(struct drm_plane *plane,
> struct drm_plane_state *old_state)
> {
> @@ -126,57 +153,88 @@ static void meson_plane_atomic_update(struct drm_plane *plane,
> */
> spin_lock_irqsave(&priv->drm->event_lock, flags);
>
> + /* Check if AFBC decoder is required for this buffer */
> + if ((meson_vpu_is_compatible(priv, VPU_COMPATIBLE_GXM) ||
> + meson_vpu_is_compatible(priv, VPU_COMPATIBLE_G12A)) &&
> + fb->modifier & DRM_FORMAT_MOD_ARM_AFBC(MESON_MOD_AFBC_VALID_BITS))
> + priv->viu.osd1_afbcd = true;
> + else
> + priv->viu.osd1_afbcd = false;
> +
> /* Enable OSD and BLK0, set max global alpha */
> priv->viu.osd1_ctrl_stat = OSD_ENABLE |
> (0xFF << OSD_GLOBAL_ALPHA_SHIFT) |
> OSD_BLK0_ENABLE;
>
> + priv->viu.osd1_ctrl_stat2 = readl(priv->io_base +
> + _REG(VIU_OSD1_CTRL_STAT2));
> +
> canvas_id_osd1 = priv->canvas_id_osd1;
>
> /* Set up BLK0 to point to the right canvas */
> - priv->viu.osd1_blk0_cfg[0] = ((canvas_id_osd1 << OSD_CANVAS_SEL) |
> - OSD_ENDIANNESS_LE);
> + priv->viu.osd1_blk0_cfg[0] = canvas_id_osd1 << OSD_CANVAS_SEL;
> +
> + if (priv->viu.osd1_afbcd) {
> + if (meson_vpu_is_compatible(priv, VPU_COMPATIBLE_G12A)) {
> + /* This is the internal decoding memory address */
> + priv->viu.osd1_blk1_cfg4 = MESON_G12A_AFBCD_OUT_ADDR;
> + priv->viu.osd1_blk0_cfg[0] |= OSD_ENDIANNESS_BE;
> + priv->viu.osd1_ctrl_stat2 |= OSD_PENDING_STAT_CLEAN;
> + }
> +
> + if (meson_vpu_is_compatible(priv, VPU_COMPATIBLE_GXM)) {
> + priv->viu.osd1_blk0_cfg[0] |= OSD_ENDIANNESS_LE;
> + priv->viu.osd1_ctrl_stat2 |= OSD_DPATH_MALI_AFBCD;
> + }
> + } else {
> + priv->viu.osd1_blk0_cfg[0] |= OSD_ENDIANNESS_LE;
> +
> + if (meson_vpu_is_compatible(priv, VPU_COMPATIBLE_GXM))
> + priv->viu.osd1_ctrl_stat2 &= ~OSD_DPATH_MALI_AFBCD;
> + }
>
> /* On GXBB, Use the old non-HDR RGB2YUV converter */
> if (meson_vpu_is_compatible(priv, VPU_COMPATIBLE_GXBB))
> priv->viu.osd1_blk0_cfg[0] |= OSD_OUTPUT_COLOR_RGB;
>
> + if (priv->viu.osd1_afbcd &&
> + meson_vpu_is_compatible(priv, VPU_COMPATIBLE_G12A)) {
> + priv->viu.osd1_blk0_cfg[0] |= OSD_MALI_SRC_EN |
> + priv->afbcd.ops->fmt_to_blk_mode(fb->modifier,
> + fb->format->format);
> + } else {
> + switch (fb->format->format) {
> + case DRM_FORMAT_XRGB8888:
> + case DRM_FORMAT_ARGB8888:
> + priv->viu.osd1_blk0_cfg[0] |= OSD_BLK_MODE_32 |
> + OSD_COLOR_MATRIX_32_ARGB;
> + break;
> + case DRM_FORMAT_XBGR8888:
> + case DRM_FORMAT_ABGR8888:
> + priv->viu.osd1_blk0_cfg[0] |= OSD_BLK_MODE_32 |
> + OSD_COLOR_MATRIX_32_ABGR;
> + break;
> + case DRM_FORMAT_RGB888:
> + priv->viu.osd1_blk0_cfg[0] |= OSD_BLK_MODE_24 |
> + OSD_COLOR_MATRIX_24_RGB;
> + break;
> + case DRM_FORMAT_RGB565:
> + priv->viu.osd1_blk0_cfg[0] |= OSD_BLK_MODE_16 |
> + OSD_COLOR_MATRIX_16_RGB565;
> + break;
> + };
> + }
> +
> switch (fb->format->format) {
> case DRM_FORMAT_XRGB8888:
> - /* For XRGB, replace the pixel's alpha by 0xFF */
> - writel_bits_relaxed(OSD_REPLACE_EN, OSD_REPLACE_EN,
> - priv->io_base + _REG(VIU_OSD1_CTRL_STAT2));
> - priv->viu.osd1_blk0_cfg[0] |= OSD_BLK_MODE_32 |
> - OSD_COLOR_MATRIX_32_ARGB;
> - break;
> case DRM_FORMAT_XBGR8888:
> /* For XRGB, replace the pixel's alpha by 0xFF */
> - writel_bits_relaxed(OSD_REPLACE_EN, OSD_REPLACE_EN,
> - priv->io_base + _REG(VIU_OSD1_CTRL_STAT2));
> - priv->viu.osd1_blk0_cfg[0] |= OSD_BLK_MODE_32 |
> - OSD_COLOR_MATRIX_32_ABGR;
> + priv->viu.osd1_ctrl_stat2 |= OSD_REPLACE_EN;
> break;
> case DRM_FORMAT_ARGB8888:
> - /* For ARGB, use the pixel's alpha */
> - writel_bits_relaxed(OSD_REPLACE_EN, 0,
> - priv->io_base + _REG(VIU_OSD1_CTRL_STAT2));
> - priv->viu.osd1_blk0_cfg[0] |= OSD_BLK_MODE_32 |
> - OSD_COLOR_MATRIX_32_ARGB;
> - break;
> case DRM_FORMAT_ABGR8888:
> /* For ARGB, use the pixel's alpha */
> - writel_bits_relaxed(OSD_REPLACE_EN, 0,
> - priv->io_base + _REG(VIU_OSD1_CTRL_STAT2));
> - priv->viu.osd1_blk0_cfg[0] |= OSD_BLK_MODE_32 |
> - OSD_COLOR_MATRIX_32_ABGR;
> - break;
> - case DRM_FORMAT_RGB888:
> - priv->viu.osd1_blk0_cfg[0] |= OSD_BLK_MODE_24 |
> - OSD_COLOR_MATRIX_24_RGB;
> - break;
> - case DRM_FORMAT_RGB565:
> - priv->viu.osd1_blk0_cfg[0] |= OSD_BLK_MODE_16 |
> - OSD_COLOR_MATRIX_16_RGB565;
> + priv->viu.osd1_ctrl_stat2 &= ~OSD_REPLACE_EN;
> break;
> };
>
> @@ -307,6 +365,16 @@ static void meson_plane_atomic_update(struct drm_plane *plane,
> priv->viu.osd1_height = fb->height;
> priv->viu.osd1_width = fb->width;
>
> + if (priv->viu.osd1_afbcd) {
> + priv->afbcd.modifier = fb->modifier;
> + priv->afbcd.format = fb->format->format;
> +
> + /* Calculate decoder write stride */
> + if (meson_vpu_is_compatible(priv, VPU_COMPATIBLE_G12A))
> + priv->viu.osd1_blk2_cfg4 =
> + meson_g12a_afbcd_line_stride(priv);
> + }
> +
> if (!meson_plane->enabled) {
> /* Reset OSD1 before enabling it on GXL+ SoCs */
> if (meson_vpu_is_compatible(priv, VPU_COMPATIBLE_GXM) ||
> @@ -346,6 +414,42 @@ static const struct drm_plane_helper_funcs meson_plane_helper_funcs = {
> .prepare_fb = drm_gem_fb_prepare_fb,
> };
>
> +static bool meson_plane_format_mod_supported(struct drm_plane *plane,
> + u32 format, u64 modifier)
> +{
> + struct meson_plane *meson_plane = to_meson_plane(plane);
> + struct meson_drm *priv = meson_plane->priv;
> + int i;
> +
> + if (modifier == DRM_FORMAT_MOD_INVALID)
> + return false;
> +
> + if (modifier == DRM_FORMAT_MOD_LINEAR)
> + return true;
> +
> + if (!meson_vpu_is_compatible(priv, VPU_COMPATIBLE_GXM) &&
> + !meson_vpu_is_compatible(priv, VPU_COMPATIBLE_G12A))
> + return false;
> +
> + if (modifier & ~DRM_FORMAT_MOD_ARM_AFBC(MESON_MOD_AFBC_VALID_BITS))
> + return false;
> +
> + for (i = 0 ; i < plane->modifier_count ; ++i)
> + if (plane->modifiers[i] == modifier)
> + break;
> +
> + if (i == plane->modifier_count) {
> + DRM_DEBUG_KMS("Unsupported modifier\n");
> + return false;
> + }
> +
> + if (priv->afbcd.ops && priv->afbcd.ops->supported_fmt)
> + return priv->afbcd.ops->supported_fmt(modifier, format);
> +
> + DRM_DEBUG_KMS("AFBC Unsupported\n");
> + return false;
> +}
> +
> static const struct drm_plane_funcs meson_plane_funcs = {
> .update_plane = drm_atomic_helper_update_plane,
> .disable_plane = drm_atomic_helper_disable_plane,
> @@ -353,6 +457,7 @@ static const struct drm_plane_funcs meson_plane_funcs = {
> .reset = drm_atomic_helper_plane_reset,
> .atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
> .atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
> + .format_mod_supported = meson_plane_format_mod_supported,
> };
>
> static const uint32_t supported_drm_formats[] = {
> @@ -364,10 +469,53 @@ static const uint32_t supported_drm_formats[] = {
> DRM_FORMAT_RGB565,
> };
>
> +static const uint64_t format_modifiers_afbc_gxm[] = {
> + DRM_FORMAT_MOD_ARM_AFBC(AFBC_FORMAT_MOD_BLOCK_SIZE_16x16 |
> + AFBC_FORMAT_MOD_SPARSE |
> + AFBC_FORMAT_MOD_YTR),
> + /* SPLIT mandates SPARSE, RGB modes mandates YTR */
> + DRM_FORMAT_MOD_ARM_AFBC(AFBC_FORMAT_MOD_BLOCK_SIZE_16x16 |
> + AFBC_FORMAT_MOD_YTR |
> + AFBC_FORMAT_MOD_SPARSE |
> + AFBC_FORMAT_MOD_SPLIT),
> + DRM_FORMAT_MOD_LINEAR,
> + DRM_FORMAT_MOD_INVALID,
> +};
> +
> +static const uint64_t format_modifiers_afbc_g12a[] = {
> + /*
> + * - TOFIX Support AFBC modifiers for YUV formats (16x16 + TILED)
> + * - AFBC_FORMAT_MOD_YTR is mandatory since we only support RGB
> + * - SPLIT is mandatory for performances reasons when in 16x16
> + * block size
> + * - 32x8 block size + SPLIT is mandatory with 4K frame size
> + * for performances reasons
> + */
> + DRM_FORMAT_MOD_ARM_AFBC(AFBC_FORMAT_MOD_BLOCK_SIZE_16x16 |
> + AFBC_FORMAT_MOD_YTR |
> + AFBC_FORMAT_MOD_SPARSE |
> + AFBC_FORMAT_MOD_SPLIT),
> + DRM_FORMAT_MOD_ARM_AFBC(AFBC_FORMAT_MOD_BLOCK_SIZE_32x8 |
> + AFBC_FORMAT_MOD_YTR |
> + AFBC_FORMAT_MOD_SPARSE),
> + DRM_FORMAT_MOD_ARM_AFBC(AFBC_FORMAT_MOD_BLOCK_SIZE_32x8 |
> + AFBC_FORMAT_MOD_YTR |
> + AFBC_FORMAT_MOD_SPARSE |
> + AFBC_FORMAT_MOD_SPLIT),
> + DRM_FORMAT_MOD_LINEAR,
> + DRM_FORMAT_MOD_INVALID,
> +};
> +
> +static const uint64_t format_modifiers_default[] = {
> + DRM_FORMAT_MOD_LINEAR,
> + DRM_FORMAT_MOD_INVALID,
> +};
> +
> int meson_plane_create(struct meson_drm *priv)
> {
> struct meson_plane *meson_plane;
> struct drm_plane *plane;
> + const uint64_t *format_modifiers = format_modifiers_default;
>
> meson_plane = devm_kzalloc(priv->drm->dev, sizeof(*meson_plane),
> GFP_KERNEL);
> @@ -377,11 +525,16 @@ int meson_plane_create(struct meson_drm *priv)
> meson_plane->priv = priv;
> plane = &meson_plane->base;
>
> + if (meson_vpu_is_compatible(priv, VPU_COMPATIBLE_GXM))
> + format_modifiers = format_modifiers_afbc_gxm;
> + else if (meson_vpu_is_compatible(priv, VPU_COMPATIBLE_G12A))
> + format_modifiers = format_modifiers_afbc_g12a;
> +
> drm_universal_plane_init(priv->drm, plane, 0xFF,
> &meson_plane_funcs,
> supported_drm_formats,
> ARRAY_SIZE(supported_drm_formats),
> - NULL,
> + format_modifiers,
> DRM_PLANE_TYPE_PRIMARY, "meson_primary_plane");
>
> drm_plane_helper_add(plane, &meson_plane_helper_funcs);
> --
> 2.22.0
_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 4/7] drm/meson: plane: add support for AFBC mode for OSD1 plane
2019-10-10 13:26 ` Ayan Halder
@ 2019-10-10 13:41 ` Neil Armstrong
2019-10-10 17:31 ` Ayan Halder
2019-10-11 8:41 ` Brian Starkey
0 siblings, 2 replies; 22+ messages in thread
From: Neil Armstrong @ 2019-10-10 13:41 UTC (permalink / raw)
To: Ayan Halder; +Cc: khilman, nd, linux-arm-kernel, dri-devel, linux-amlogic
Hi Ayan,
On 10/10/2019 15:26, Ayan Halder wrote:
> On Thu, Oct 10, 2019 at 11:25:23AM +0200, Neil Armstrong wrote:
>> This adds all the OSD configuration plumbing to support the AFBC decoders
>> path to display of the OSD1 plane.
>>
>> The Amlogic GXM and G12A AFBC decoders are integrated very differently.
>>
>> The Amlogic GXM has a direct output path to the OSD1 VIU pixel input,
>> because the GXM AFBC decoder seem to be a custom IP developed by Amlogic.
>>
>> On the other side, the Amlogic G12A AFBC decoder seems to be an external
>> IP that emit pixels on an AXI master hooked to a "Mali Unpack" block
>> feeding the OSD1 VIU pixel input.
>> This uses a weird "0x1000000" internal HW physical address on both
>> sides to transfer the pixels.
>>
>> For Amlogic GXM, the supported pixel formats are the same as the normal
>> linear OSD1 mode.
>>
>> On the other side, Amlogic added support for all AFBC v1.2 formats for
>> the G12A AFBC integration.
>>
>> For simplicity, we stick to the already supported formats for now.
>>
>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>> ---
>> drivers/gpu/drm/meson/meson_crtc.c | 2 +
>> drivers/gpu/drm/meson/meson_drv.h | 4 +
>> drivers/gpu/drm/meson/meson_plane.c | 215 ++++++++++++++++++++++++----
>> 3 files changed, 190 insertions(+), 31 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/meson/meson_crtc.c b/drivers/gpu/drm/meson/meson_crtc.c
>> index 57ae1c13d1e6..d478fa232951 100644
>> --- a/drivers/gpu/drm/meson/meson_crtc.c
>> +++ b/drivers/gpu/drm/meson/meson_crtc.c
>> @@ -281,6 +281,8 @@ void meson_crtc_irq(struct meson_drm *priv)
>> if (priv->viu.osd1_enabled && priv->viu.osd1_commit) {
>> writel_relaxed(priv->viu.osd1_ctrl_stat,
>> priv->io_base + _REG(VIU_OSD1_CTRL_STAT));
>> + writel_relaxed(priv->viu.osd1_ctrl_stat2,
>> + priv->io_base + _REG(VIU_OSD1_CTRL_STAT2));
>> writel_relaxed(priv->viu.osd1_blk0_cfg[0],
>> priv->io_base + _REG(VIU_OSD1_BLK0_CFG_W0));
>> writel_relaxed(priv->viu.osd1_blk0_cfg[1],
>> diff --git a/drivers/gpu/drm/meson/meson_drv.h b/drivers/gpu/drm/meson/meson_drv.h
>> index 60f13c6f34e5..de25349be8aa 100644
>> --- a/drivers/gpu/drm/meson/meson_drv.h
>> +++ b/drivers/gpu/drm/meson/meson_drv.h
>> @@ -53,8 +53,12 @@ struct meson_drm {
>> bool osd1_enabled;
>> bool osd1_interlace;
>> bool osd1_commit;
>> + bool osd1_afbcd;
>> uint32_t osd1_ctrl_stat;
>> + uint32_t osd1_ctrl_stat2;
>> uint32_t osd1_blk0_cfg[5];
>> + uint32_t osd1_blk1_cfg4;
>> + uint32_t osd1_blk2_cfg4;
>> uint32_t osd1_addr;
>> uint32_t osd1_stride;
>> uint32_t osd1_height;
>> diff --git a/drivers/gpu/drm/meson/meson_plane.c b/drivers/gpu/drm/meson/meson_plane.c
>> index 5e798c276037..412941aa8402 100644
>> --- a/drivers/gpu/drm/meson/meson_plane.c
>> +++ b/drivers/gpu/drm/meson/meson_plane.c
>> @@ -23,6 +23,7 @@
>> #include "meson_plane.h"
>> #include "meson_registers.h"
>> #include "meson_viu.h"
>> +#include "meson_osd_afbcd.h"
>>
>> /* OSD_SCI_WH_M1 */
>> #define SCI_WH_M1_W(w) FIELD_PREP(GENMASK(28, 16), w)
>> @@ -92,12 +93,38 @@ static int meson_plane_atomic_check(struct drm_plane *plane,
>> false, true);
>> }
>>
>> +#define MESON_MOD_AFBC_VALID_BITS (AFBC_FORMAT_MOD_BLOCK_SIZE_16x16 | \
>> + AFBC_FORMAT_MOD_BLOCK_SIZE_32x8 | \
>> + AFBC_FORMAT_MOD_YTR | \
>> + AFBC_FORMAT_MOD_SPARSE | \
>> + AFBC_FORMAT_MOD_SPLIT)
>> +
>> /* Takes a fixed 16.16 number and converts it to integer. */
>> static inline int64_t fixed16_to_int(int64_t value)
>> {
>> return value >> 16;
>> }
>>
>> +static u32 meson_g12a_afbcd_line_stride(struct meson_drm *priv)
>> +{
>> + u32 line_stride = 0;
>> +
>> + switch (priv->afbcd.format) {
>> + case DRM_FORMAT_RGB565:
>> + line_stride = ((priv->viu.osd1_width << 4) + 127) >> 7;
>> + break;
>> + case DRM_FORMAT_RGB888:
>> + case DRM_FORMAT_XRGB8888:
>> + case DRM_FORMAT_ARGB8888:
>> + case DRM_FORMAT_XBGR8888:
>> + case DRM_FORMAT_ABGR8888:
> Please have a look at
> https://www.kernel.org/doc/html/latest/gpu/afbc.html for our
> recommendation. We suggest that *X* formats are avoided.
>
> Also, for interoperability and maximum compression efficiency (with
> AFBC_FORMAT_MOD_YTR), we suggest the following order :-
>
> Component 0: R
> Component 1: G
> Component 2: B
> Component 3: A (if available)
Sorry I don't understand, you ask me to limit AFBC to ABGR8888 ?
But why if the HW (GPU and DPU) is capable of ?
Isn't it an userspace choice ? I understand XRGB8888 is a waste
of memory space and compression efficiency, but this is not the
kernel driver's to decide this, right ?
For interoperability I'll understand recommending a minimal set
of modifiers and formats. But here, each platform is also limited
by it's GPU capabilites aswell.
Limiting to ABGR8888 would discard like every non-Android renderers,
using AFBC, I'm not sure it's the kernels driver's responsibility.
>
> Thus, DRM_FORMAT_ABGR, DRM_FORMAT_BGR should only be allowed.
>> + line_stride = ((priv->viu.osd1_width << 5) + 127) >> 7;
>> + break;
>> + }
>> +
>> + return ((line_stride + 1) >> 1) << 1;
>> +}
>> +
>> static void meson_plane_atomic_update(struct drm_plane *plane,
>> struct drm_plane_state *old_state)
>> {
[...]
>>
>> +static bool meson_plane_format_mod_supported(struct drm_plane *plane,
>> + u32 format, u64 modifier)
>> +{
>> + struct meson_plane *meson_plane = to_meson_plane(plane);
>> + struct meson_drm *priv = meson_plane->priv;
>> + int i;
>> +
>> + if (modifier == DRM_FORMAT_MOD_INVALID)
>> + return false;
>> +
>> + if (modifier == DRM_FORMAT_MOD_LINEAR)
>> + return true;
>> +
>> + if (!meson_vpu_is_compatible(priv, VPU_COMPATIBLE_GXM) &&
>> + !meson_vpu_is_compatible(priv, VPU_COMPATIBLE_G12A))
>> + return false;
>> +
>> + if (modifier & ~DRM_FORMAT_MOD_ARM_AFBC(MESON_MOD_AFBC_VALID_BITS))
>> + return false;
>> +
>> + for (i = 0 ; i < plane->modifier_count ; ++i)
>> + if (plane->modifiers[i] == modifier)
>> + break;
>> +
>> + if (i == plane->modifier_count) {
>> + DRM_DEBUG_KMS("Unsupported modifier\n");
>> + return false;
>> + }
I can add a warn_once here, would it be enough ?
>> +
>> + if (priv->afbcd.ops && priv->afbcd.ops->supported_fmt)
>> + return priv->afbcd.ops->supported_fmt(modifier, format);
>> +
>> + DRM_DEBUG_KMS("AFBC Unsupported\n");
>> + return false;
>> +}
>> +
>> static const struct drm_plane_funcs meson_plane_funcs = {
>> .update_plane = drm_atomic_helper_update_plane,
>> .disable_plane = drm_atomic_helper_disable_plane,
>> @@ -353,6 +457,7 @@ static const struct drm_plane_funcs meson_plane_funcs = {
>> .reset = drm_atomic_helper_plane_reset,
>> .atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
>> .atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
>> + .format_mod_supported = meson_plane_format_mod_supported,
>> };
>>
>> static const uint32_t supported_drm_formats[] = {
>> @@ -364,10 +469,53 @@ static const uint32_t supported_drm_formats[] = {
>> DRM_FORMAT_RGB565,
>> };
>>
>> +static const uint64_t format_modifiers_afbc_gxm[] = {
>> + DRM_FORMAT_MOD_ARM_AFBC(AFBC_FORMAT_MOD_BLOCK_SIZE_16x16 |
>> + AFBC_FORMAT_MOD_SPARSE |
>> + AFBC_FORMAT_MOD_YTR),
>> + /* SPLIT mandates SPARSE, RGB modes mandates YTR */
>> + DRM_FORMAT_MOD_ARM_AFBC(AFBC_FORMAT_MOD_BLOCK_SIZE_16x16 |
>> + AFBC_FORMAT_MOD_YTR |
>> + AFBC_FORMAT_MOD_SPARSE |
>> + AFBC_FORMAT_MOD_SPLIT),
>> + DRM_FORMAT_MOD_LINEAR,
>> + DRM_FORMAT_MOD_INVALID,
>> +};
>> +
>> +static const uint64_t format_modifiers_afbc_g12a[] = {
>> + /*
>> + * - TOFIX Support AFBC modifiers for YUV formats (16x16 + TILED)
>> + * - AFBC_FORMAT_MOD_YTR is mandatory since we only support RGB
>> + * - SPLIT is mandatory for performances reasons when in 16x16
>> + * block size
>> + * - 32x8 block size + SPLIT is mandatory with 4K frame size
>> + * for performances reasons
>> + */
>> + DRM_FORMAT_MOD_ARM_AFBC(AFBC_FORMAT_MOD_BLOCK_SIZE_16x16 |
>> + AFBC_FORMAT_MOD_YTR |
>> + AFBC_FORMAT_MOD_SPARSE |
>> + AFBC_FORMAT_MOD_SPLIT),
>> + DRM_FORMAT_MOD_ARM_AFBC(AFBC_FORMAT_MOD_BLOCK_SIZE_32x8 |
>> + AFBC_FORMAT_MOD_YTR |
>> + AFBC_FORMAT_MOD_SPARSE),
>> + DRM_FORMAT_MOD_ARM_AFBC(AFBC_FORMAT_MOD_BLOCK_SIZE_32x8 |
>> + AFBC_FORMAT_MOD_YTR |
>> + AFBC_FORMAT_MOD_SPARSE |
>> + AFBC_FORMAT_MOD_SPLIT),
>> + DRM_FORMAT_MOD_LINEAR,
>> + DRM_FORMAT_MOD_INVALID,
>> +};
>> +
>> +static const uint64_t format_modifiers_default[] = {
>> + DRM_FORMAT_MOD_LINEAR,
>> + DRM_FORMAT_MOD_INVALID,
>> +};
>> +
>> int meson_plane_create(struct meson_drm *priv)
>> {
>> struct meson_plane *meson_plane;
>> struct drm_plane *plane;
>> + const uint64_t *format_modifiers = format_modifiers_default;
>>
>> meson_plane = devm_kzalloc(priv->drm->dev, sizeof(*meson_plane),
>> GFP_KERNEL);
>> @@ -377,11 +525,16 @@ int meson_plane_create(struct meson_drm *priv)
>> meson_plane->priv = priv;
>> plane = &meson_plane->base;
>>
>> + if (meson_vpu_is_compatible(priv, VPU_COMPATIBLE_GXM))
>> + format_modifiers = format_modifiers_afbc_gxm;
>> + else if (meson_vpu_is_compatible(priv, VPU_COMPATIBLE_G12A))
>> + format_modifiers = format_modifiers_afbc_g12a;
>> +
>> drm_universal_plane_init(priv->drm, plane, 0xFF,
>> &meson_plane_funcs,
>> supported_drm_formats,
>> ARRAY_SIZE(supported_drm_formats),
>> - NULL,
>> + format_modifiers,
>> DRM_PLANE_TYPE_PRIMARY, "meson_primary_plane");
>>
>> drm_plane_helper_add(plane, &meson_plane_helper_funcs);
>> --
>> 2.22.0
_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 4/7] drm/meson: plane: add support for AFBC mode for OSD1 plane
2019-10-10 13:41 ` Neil Armstrong
@ 2019-10-10 17:31 ` Ayan Halder
2019-10-11 7:46 ` Daniel Vetter
2019-10-11 8:41 ` Brian Starkey
1 sibling, 1 reply; 22+ messages in thread
From: Ayan Halder @ 2019-10-10 17:31 UTC (permalink / raw)
To: Neil Armstrong; +Cc: khilman, nd, linux-arm-kernel, dri-devel, linux-amlogic
On Thu, Oct 10, 2019 at 03:41:15PM +0200, Neil Armstrong wrote:
> Hi Ayan,
>
> On 10/10/2019 15:26, Ayan Halder wrote:
> > On Thu, Oct 10, 2019 at 11:25:23AM +0200, Neil Armstrong wrote:
> >> This adds all the OSD configuration plumbing to support the AFBC decoders
> >> path to display of the OSD1 plane.
> >>
> >> The Amlogic GXM and G12A AFBC decoders are integrated very differently.
> >>
> >> The Amlogic GXM has a direct output path to the OSD1 VIU pixel input,
> >> because the GXM AFBC decoder seem to be a custom IP developed by Amlogic.
> >>
> >> On the other side, the Amlogic G12A AFBC decoder seems to be an external
> >> IP that emit pixels on an AXI master hooked to a "Mali Unpack" block
> >> feeding the OSD1 VIU pixel input.
> >> This uses a weird "0x1000000" internal HW physical address on both
> >> sides to transfer the pixels.
> >>
> >> For Amlogic GXM, the supported pixel formats are the same as the normal
> >> linear OSD1 mode.
> >>
> >> On the other side, Amlogic added support for all AFBC v1.2 formats for
> >> the G12A AFBC integration.
> >>
> >> For simplicity, we stick to the already supported formats for now.
> >>
> >> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> >> ---
> >> drivers/gpu/drm/meson/meson_crtc.c | 2 +
> >> drivers/gpu/drm/meson/meson_drv.h | 4 +
> >> drivers/gpu/drm/meson/meson_plane.c | 215 ++++++++++++++++++++++++----
> >> 3 files changed, 190 insertions(+), 31 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/meson/meson_crtc.c b/drivers/gpu/drm/meson/meson_crtc.c
> >> index 57ae1c13d1e6..d478fa232951 100644
> >> --- a/drivers/gpu/drm/meson/meson_crtc.c
> >> +++ b/drivers/gpu/drm/meson/meson_crtc.c
> >> @@ -281,6 +281,8 @@ void meson_crtc_irq(struct meson_drm *priv)
> >> if (priv->viu.osd1_enabled && priv->viu.osd1_commit) {
> >> writel_relaxed(priv->viu.osd1_ctrl_stat,
> >> priv->io_base + _REG(VIU_OSD1_CTRL_STAT));
> >> + writel_relaxed(priv->viu.osd1_ctrl_stat2,
> >> + priv->io_base + _REG(VIU_OSD1_CTRL_STAT2));
> >> writel_relaxed(priv->viu.osd1_blk0_cfg[0],
> >> priv->io_base + _REG(VIU_OSD1_BLK0_CFG_W0));
> >> writel_relaxed(priv->viu.osd1_blk0_cfg[1],
> >> diff --git a/drivers/gpu/drm/meson/meson_drv.h b/drivers/gpu/drm/meson/meson_drv.h
> >> index 60f13c6f34e5..de25349be8aa 100644
> >> --- a/drivers/gpu/drm/meson/meson_drv.h
> >> +++ b/drivers/gpu/drm/meson/meson_drv.h
> >> @@ -53,8 +53,12 @@ struct meson_drm {
> >> bool osd1_enabled;
> >> bool osd1_interlace;
> >> bool osd1_commit;
> >> + bool osd1_afbcd;
> >> uint32_t osd1_ctrl_stat;
> >> + uint32_t osd1_ctrl_stat2;
> >> uint32_t osd1_blk0_cfg[5];
> >> + uint32_t osd1_blk1_cfg4;
> >> + uint32_t osd1_blk2_cfg4;
> >> uint32_t osd1_addr;
> >> uint32_t osd1_stride;
> >> uint32_t osd1_height;
> >> diff --git a/drivers/gpu/drm/meson/meson_plane.c b/drivers/gpu/drm/meson/meson_plane.c
> >> index 5e798c276037..412941aa8402 100644
> >> --- a/drivers/gpu/drm/meson/meson_plane.c
> >> +++ b/drivers/gpu/drm/meson/meson_plane.c
> >> @@ -23,6 +23,7 @@
> >> #include "meson_plane.h"
> >> #include "meson_registers.h"
> >> #include "meson_viu.h"
> >> +#include "meson_osd_afbcd.h"
> >>
> >> /* OSD_SCI_WH_M1 */
> >> #define SCI_WH_M1_W(w) FIELD_PREP(GENMASK(28, 16), w)
> >> @@ -92,12 +93,38 @@ static int meson_plane_atomic_check(struct drm_plane *plane,
> >> false, true);
> >> }
> >>
> >> +#define MESON_MOD_AFBC_VALID_BITS (AFBC_FORMAT_MOD_BLOCK_SIZE_16x16 | \
> >> + AFBC_FORMAT_MOD_BLOCK_SIZE_32x8 | \
> >> + AFBC_FORMAT_MOD_YTR | \
> >> + AFBC_FORMAT_MOD_SPARSE | \
> >> + AFBC_FORMAT_MOD_SPLIT)
> >> +
> >> /* Takes a fixed 16.16 number and converts it to integer. */
> >> static inline int64_t fixed16_to_int(int64_t value)
> >> {
> >> return value >> 16;
> >> }
> >>
> >> +static u32 meson_g12a_afbcd_line_stride(struct meson_drm *priv)
> >> +{
> >> + u32 line_stride = 0;
> >> +
> >> + switch (priv->afbcd.format) {
> >> + case DRM_FORMAT_RGB565:
> >> + line_stride = ((priv->viu.osd1_width << 4) + 127) >> 7;
> >> + break;
> >> + case DRM_FORMAT_RGB888:
> >> + case DRM_FORMAT_XRGB8888:
> >> + case DRM_FORMAT_ARGB8888:
> >> + case DRM_FORMAT_XBGR8888:
> >> + case DRM_FORMAT_ABGR8888:
> > Please have a look at
> > https://www.kernel.org/doc/html/latest/gpu/afbc.html for our
> > recommendation. We suggest that *X* formats are avoided.
> >
> > Also, for interoperability and maximum compression efficiency (with
> > AFBC_FORMAT_MOD_YTR), we suggest the following order :-
> >
> > Component 0: R
> > Component 1: G
> > Component 2: B
> > Component 3: A (if available)
>
>
> Sorry I don't understand, you ask me to limit AFBC to ABGR8888 ?
Apologies for the confusion, as per the link, the formats which are
suggested with AFBC_FORMAT_MOD_YTR are the BGR/ABGR formats (as
listed in the 'AFBC formats' table)
Thus, any other permutation of the components might make it incompatible
with some other AFBC producers/consumers.
>
> But why if the HW (GPU and DPU) is capable of ?
>
> Isn't it an userspace choice ? I understand XRGB8888 is a waste
> of memory space and compression efficiency, but this is not the
> kernel driver's to decide this, right ?
It is a reccomendation by the AFBC spec. As far as I understand, it
depends upon the implementor of the AFBC spec(ie dpu, gpu, vpu, etc)
to allow/disallow *X* formats for AFBC encoding/decoding.
>
> For interoperability I'll understand recommending a minimal set
> of modifiers and formats. But here, each platform is also limited
> by it's GPU capabilites aswell.
Agreed
>
> Limiting to ABGR8888 would discard like every non-Android renderers,
> using AFBC, I'm not sure it's the kernels driver's responsibility.
I am not familiar with non-Android renderers.
>
> >
> > Thus, DRM_FORMAT_ABGR, DRM_FORMAT_BGR should only be allowed.
> >> + line_stride = ((priv->viu.osd1_width << 5) + 127) >> 7;
> >> + break;
> >> + }
> >> +
> >> + return ((line_stride + 1) >> 1) << 1;
> >> +}
> >> +
> >> static void meson_plane_atomic_update(struct drm_plane *plane,
> >> struct drm_plane_state *old_state)
> >> {
>
> [...]
>
> >>
> >> +static bool meson_plane_format_mod_supported(struct drm_plane *plane,
> >> + u32 format, u64 modifier)
> >> +{
> >> + struct meson_plane *meson_plane = to_meson_plane(plane);
> >> + struct meson_drm *priv = meson_plane->priv;
> >> + int i;
> >> +
> >> + if (modifier == DRM_FORMAT_MOD_INVALID)
> >> + return false;
> >> +
> >> + if (modifier == DRM_FORMAT_MOD_LINEAR)
> >> + return true;
> >> +
> >> + if (!meson_vpu_is_compatible(priv, VPU_COMPATIBLE_GXM) &&
> >> + !meson_vpu_is_compatible(priv, VPU_COMPATIBLE_G12A))
> >> + return false;
> >> +
> >> + if (modifier & ~DRM_FORMAT_MOD_ARM_AFBC(MESON_MOD_AFBC_VALID_BITS))
> >> + return false;
> >> +
> >> + for (i = 0 ; i < plane->modifier_count ; ++i)
> >> + if (plane->modifiers[i] == modifier)
> >> + break;
> >> +
> >> + if (i == plane->modifier_count) {
> >> + DRM_DEBUG_KMS("Unsupported modifier\n");
> >> + return false;
> >> + }
>
> I can add a warn_once here, would it be enough ?
>
> >> +
> >> + if (priv->afbcd.ops && priv->afbcd.ops->supported_fmt)
> >> + return priv->afbcd.ops->supported_fmt(modifier, format);
> >> +
> >> + DRM_DEBUG_KMS("AFBC Unsupported\n");
> >> + return false;
> >> +}
> >> +
> >> static const struct drm_plane_funcs meson_plane_funcs = {
> >> .update_plane = drm_atomic_helper_update_plane,
> >> .disable_plane = drm_atomic_helper_disable_plane,
> >> @@ -353,6 +457,7 @@ static const struct drm_plane_funcs meson_plane_funcs = {
> >> .reset = drm_atomic_helper_plane_reset,
> >> .atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
> >> .atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
> >> + .format_mod_supported = meson_plane_format_mod_supported,
> >> };
> >>
> >> static const uint32_t supported_drm_formats[] = {
> >> @@ -364,10 +469,53 @@ static const uint32_t supported_drm_formats[] = {
> >> DRM_FORMAT_RGB565,
> >> };
> >>
> >> +static const uint64_t format_modifiers_afbc_gxm[] = {
> >> + DRM_FORMAT_MOD_ARM_AFBC(AFBC_FORMAT_MOD_BLOCK_SIZE_16x16 |
> >> + AFBC_FORMAT_MOD_SPARSE |
> >> + AFBC_FORMAT_MOD_YTR),
> >> + /* SPLIT mandates SPARSE, RGB modes mandates YTR */
> >> + DRM_FORMAT_MOD_ARM_AFBC(AFBC_FORMAT_MOD_BLOCK_SIZE_16x16 |
> >> + AFBC_FORMAT_MOD_YTR |
> >> + AFBC_FORMAT_MOD_SPARSE |
> >> + AFBC_FORMAT_MOD_SPLIT),
> >> + DRM_FORMAT_MOD_LINEAR,
> >> + DRM_FORMAT_MOD_INVALID,
> >> +};
> >> +
> >> +static const uint64_t format_modifiers_afbc_g12a[] = {
> >> + /*
> >> + * - TOFIX Support AFBC modifiers for YUV formats (16x16 + TILED)
> >> + * - AFBC_FORMAT_MOD_YTR is mandatory since we only support RGB
> >> + * - SPLIT is mandatory for performances reasons when in 16x16
> >> + * block size
> >> + * - 32x8 block size + SPLIT is mandatory with 4K frame size
> >> + * for performances reasons
> >> + */
> >> + DRM_FORMAT_MOD_ARM_AFBC(AFBC_FORMAT_MOD_BLOCK_SIZE_16x16 |
> >> + AFBC_FORMAT_MOD_YTR |
> >> + AFBC_FORMAT_MOD_SPARSE |
> >> + AFBC_FORMAT_MOD_SPLIT),
> >> + DRM_FORMAT_MOD_ARM_AFBC(AFBC_FORMAT_MOD_BLOCK_SIZE_32x8 |
> >> + AFBC_FORMAT_MOD_YTR |
> >> + AFBC_FORMAT_MOD_SPARSE),
> >> + DRM_FORMAT_MOD_ARM_AFBC(AFBC_FORMAT_MOD_BLOCK_SIZE_32x8 |
> >> + AFBC_FORMAT_MOD_YTR |
> >> + AFBC_FORMAT_MOD_SPARSE |
> >> + AFBC_FORMAT_MOD_SPLIT),
> >> + DRM_FORMAT_MOD_LINEAR,
> >> + DRM_FORMAT_MOD_INVALID,
> >> +};
> >> +
> >> +static const uint64_t format_modifiers_default[] = {
> >> + DRM_FORMAT_MOD_LINEAR,
> >> + DRM_FORMAT_MOD_INVALID,
> >> +};
> >> +
> >> int meson_plane_create(struct meson_drm *priv)
> >> {
> >> struct meson_plane *meson_plane;
> >> struct drm_plane *plane;
> >> + const uint64_t *format_modifiers = format_modifiers_default;
> >>
> >> meson_plane = devm_kzalloc(priv->drm->dev, sizeof(*meson_plane),
> >> GFP_KERNEL);
> >> @@ -377,11 +525,16 @@ int meson_plane_create(struct meson_drm *priv)
> >> meson_plane->priv = priv;
> >> plane = &meson_plane->base;
> >>
> >> + if (meson_vpu_is_compatible(priv, VPU_COMPATIBLE_GXM))
> >> + format_modifiers = format_modifiers_afbc_gxm;
> >> + else if (meson_vpu_is_compatible(priv, VPU_COMPATIBLE_G12A))
> >> + format_modifiers = format_modifiers_afbc_g12a;
> >> +
> >> drm_universal_plane_init(priv->drm, plane, 0xFF,
> >> &meson_plane_funcs,
> >> supported_drm_formats,
> >> ARRAY_SIZE(supported_drm_formats),
> >> - NULL,
> >> + format_modifiers,
> >> DRM_PLANE_TYPE_PRIMARY, "meson_primary_plane");
> >>
> >> drm_plane_helper_add(plane, &meson_plane_helper_funcs);
> >> --
> >> 2.22.0
_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 4/7] drm/meson: plane: add support for AFBC mode for OSD1 plane
2019-10-10 17:31 ` Ayan Halder
@ 2019-10-11 7:46 ` Daniel Vetter
2019-10-11 7:56 ` Daniel Stone
0 siblings, 1 reply; 22+ messages in thread
From: Daniel Vetter @ 2019-10-11 7:46 UTC (permalink / raw)
To: Ayan Halder
Cc: Neil Armstrong, khilman, dri-devel, linux-amlogic, nd, linux-arm-kernel
On Thu, Oct 10, 2019 at 7:32 PM Ayan Halder <Ayan.Halder@arm.com> wrote:
>
> On Thu, Oct 10, 2019 at 03:41:15PM +0200, Neil Armstrong wrote:
> > Hi Ayan,
> >
> > On 10/10/2019 15:26, Ayan Halder wrote:
> > > On Thu, Oct 10, 2019 at 11:25:23AM +0200, Neil Armstrong wrote:
> > >> This adds all the OSD configuration plumbing to support the AFBC decoders
> > >> path to display of the OSD1 plane.
> > >>
> > >> The Amlogic GXM and G12A AFBC decoders are integrated very differently.
> > >>
> > >> The Amlogic GXM has a direct output path to the OSD1 VIU pixel input,
> > >> because the GXM AFBC decoder seem to be a custom IP developed by Amlogic.
> > >>
> > >> On the other side, the Amlogic G12A AFBC decoder seems to be an external
> > >> IP that emit pixels on an AXI master hooked to a "Mali Unpack" block
> > >> feeding the OSD1 VIU pixel input.
> > >> This uses a weird "0x1000000" internal HW physical address on both
> > >> sides to transfer the pixels.
> > >>
> > >> For Amlogic GXM, the supported pixel formats are the same as the normal
> > >> linear OSD1 mode.
> > >>
> > >> On the other side, Amlogic added support for all AFBC v1.2 formats for
> > >> the G12A AFBC integration.
> > >>
> > >> For simplicity, we stick to the already supported formats for now.
> > >>
> > >> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> > >> ---
> > >> drivers/gpu/drm/meson/meson_crtc.c | 2 +
> > >> drivers/gpu/drm/meson/meson_drv.h | 4 +
> > >> drivers/gpu/drm/meson/meson_plane.c | 215 ++++++++++++++++++++++++----
> > >> 3 files changed, 190 insertions(+), 31 deletions(-)
> > >>
> > >> diff --git a/drivers/gpu/drm/meson/meson_crtc.c b/drivers/gpu/drm/meson/meson_crtc.c
> > >> index 57ae1c13d1e6..d478fa232951 100644
> > >> --- a/drivers/gpu/drm/meson/meson_crtc.c
> > >> +++ b/drivers/gpu/drm/meson/meson_crtc.c
> > >> @@ -281,6 +281,8 @@ void meson_crtc_irq(struct meson_drm *priv)
> > >> if (priv->viu.osd1_enabled && priv->viu.osd1_commit) {
> > >> writel_relaxed(priv->viu.osd1_ctrl_stat,
> > >> priv->io_base + _REG(VIU_OSD1_CTRL_STAT));
> > >> + writel_relaxed(priv->viu.osd1_ctrl_stat2,
> > >> + priv->io_base + _REG(VIU_OSD1_CTRL_STAT2));
> > >> writel_relaxed(priv->viu.osd1_blk0_cfg[0],
> > >> priv->io_base + _REG(VIU_OSD1_BLK0_CFG_W0));
> > >> writel_relaxed(priv->viu.osd1_blk0_cfg[1],
> > >> diff --git a/drivers/gpu/drm/meson/meson_drv.h b/drivers/gpu/drm/meson/meson_drv.h
> > >> index 60f13c6f34e5..de25349be8aa 100644
> > >> --- a/drivers/gpu/drm/meson/meson_drv.h
> > >> +++ b/drivers/gpu/drm/meson/meson_drv.h
> > >> @@ -53,8 +53,12 @@ struct meson_drm {
> > >> bool osd1_enabled;
> > >> bool osd1_interlace;
> > >> bool osd1_commit;
> > >> + bool osd1_afbcd;
> > >> uint32_t osd1_ctrl_stat;
> > >> + uint32_t osd1_ctrl_stat2;
> > >> uint32_t osd1_blk0_cfg[5];
> > >> + uint32_t osd1_blk1_cfg4;
> > >> + uint32_t osd1_blk2_cfg4;
> > >> uint32_t osd1_addr;
> > >> uint32_t osd1_stride;
> > >> uint32_t osd1_height;
> > >> diff --git a/drivers/gpu/drm/meson/meson_plane.c b/drivers/gpu/drm/meson/meson_plane.c
> > >> index 5e798c276037..412941aa8402 100644
> > >> --- a/drivers/gpu/drm/meson/meson_plane.c
> > >> +++ b/drivers/gpu/drm/meson/meson_plane.c
> > >> @@ -23,6 +23,7 @@
> > >> #include "meson_plane.h"
> > >> #include "meson_registers.h"
> > >> #include "meson_viu.h"
> > >> +#include "meson_osd_afbcd.h"
> > >>
> > >> /* OSD_SCI_WH_M1 */
> > >> #define SCI_WH_M1_W(w) FIELD_PREP(GENMASK(28, 16), w)
> > >> @@ -92,12 +93,38 @@ static int meson_plane_atomic_check(struct drm_plane *plane,
> > >> false, true);
> > >> }
> > >>
> > >> +#define MESON_MOD_AFBC_VALID_BITS (AFBC_FORMAT_MOD_BLOCK_SIZE_16x16 | \
> > >> + AFBC_FORMAT_MOD_BLOCK_SIZE_32x8 | \
> > >> + AFBC_FORMAT_MOD_YTR | \
> > >> + AFBC_FORMAT_MOD_SPARSE | \
> > >> + AFBC_FORMAT_MOD_SPLIT)
> > >> +
> > >> /* Takes a fixed 16.16 number and converts it to integer. */
> > >> static inline int64_t fixed16_to_int(int64_t value)
> > >> {
> > >> return value >> 16;
> > >> }
> > >>
> > >> +static u32 meson_g12a_afbcd_line_stride(struct meson_drm *priv)
> > >> +{
> > >> + u32 line_stride = 0;
> > >> +
> > >> + switch (priv->afbcd.format) {
> > >> + case DRM_FORMAT_RGB565:
> > >> + line_stride = ((priv->viu.osd1_width << 4) + 127) >> 7;
> > >> + break;
> > >> + case DRM_FORMAT_RGB888:
> > >> + case DRM_FORMAT_XRGB8888:
> > >> + case DRM_FORMAT_ARGB8888:
> > >> + case DRM_FORMAT_XBGR8888:
> > >> + case DRM_FORMAT_ABGR8888:
> > > Please have a look at
> > > https://www.kernel.org/doc/html/latest/gpu/afbc.html for our
> > > recommendation. We suggest that *X* formats are avoided.
> > >
> > > Also, for interoperability and maximum compression efficiency (with
> > > AFBC_FORMAT_MOD_YTR), we suggest the following order :-
> > >
> > > Component 0: R
> > > Component 1: G
> > > Component 2: B
> > > Component 3: A (if available)
> >
> >
> > Sorry I don't understand, you ask me to limit AFBC to ABGR8888 ?
>
> Apologies for the confusion, as per the link, the formats which are
> suggested with AFBC_FORMAT_MOD_YTR are the BGR/ABGR formats (as
> listed in the 'AFBC formats' table)
>
> Thus, any other permutation of the components might make it incompatible
> with some other AFBC producers/consumers.
Uh, that's not how this is supposed to be used. Drivers are meant to
expose _everything_ they support (bonus if you roughly sort it in
preference order). Userspace then computes the intersection of
modifiers/formats supported by all devices it needs to share a buffer
with. Allowing that was the entire point of modifiers, if we
artificially limit to the common denominator we're back "only linear
works everywhere".
-Daniel
>
> >
> > But why if the HW (GPU and DPU) is capable of ?
> >
> > Isn't it an userspace choice ? I understand XRGB8888 is a waste
> > of memory space and compression efficiency, but this is not the
> > kernel driver's to decide this, right ?
> It is a reccomendation by the AFBC spec. As far as I understand, it
> depends upon the implementor of the AFBC spec(ie dpu, gpu, vpu, etc)
> to allow/disallow *X* formats for AFBC encoding/decoding.
>
> >
> > For interoperability I'll understand recommending a minimal set
> > of modifiers and formats. But here, each platform is also limited
> > by it's GPU capabilites aswell.
> Agreed
>
> >
> > Limiting to ABGR8888 would discard like every non-Android renderers,
> > using AFBC, I'm not sure it's the kernels driver's responsibility.
> I am not familiar with non-Android renderers.
> >
> > >
> > > Thus, DRM_FORMAT_ABGR, DRM_FORMAT_BGR should only be allowed.
> > >> + line_stride = ((priv->viu.osd1_width << 5) + 127) >> 7;
> > >> + break;
> > >> + }
> > >> +
> > >> + return ((line_stride + 1) >> 1) << 1;
> > >> +}
> > >> +
> > >> static void meson_plane_atomic_update(struct drm_plane *plane,
> > >> struct drm_plane_state *old_state)
> > >> {
> >
> > [...]
> >
> > >>
> > >> +static bool meson_plane_format_mod_supported(struct drm_plane *plane,
> > >> + u32 format, u64 modifier)
> > >> +{
> > >> + struct meson_plane *meson_plane = to_meson_plane(plane);
> > >> + struct meson_drm *priv = meson_plane->priv;
> > >> + int i;
> > >> +
> > >> + if (modifier == DRM_FORMAT_MOD_INVALID)
> > >> + return false;
> > >> +
> > >> + if (modifier == DRM_FORMAT_MOD_LINEAR)
> > >> + return true;
> > >> +
> > >> + if (!meson_vpu_is_compatible(priv, VPU_COMPATIBLE_GXM) &&
> > >> + !meson_vpu_is_compatible(priv, VPU_COMPATIBLE_G12A))
> > >> + return false;
> > >> +
> > >> + if (modifier & ~DRM_FORMAT_MOD_ARM_AFBC(MESON_MOD_AFBC_VALID_BITS))
> > >> + return false;
> > >> +
> > >> + for (i = 0 ; i < plane->modifier_count ; ++i)
> > >> + if (plane->modifiers[i] == modifier)
> > >> + break;
> > >> +
> > >> + if (i == plane->modifier_count) {
> > >> + DRM_DEBUG_KMS("Unsupported modifier\n");
> > >> + return false;
> > >> + }
> >
> > I can add a warn_once here, would it be enough ?
> >
> > >> +
> > >> + if (priv->afbcd.ops && priv->afbcd.ops->supported_fmt)
> > >> + return priv->afbcd.ops->supported_fmt(modifier, format);
> > >> +
> > >> + DRM_DEBUG_KMS("AFBC Unsupported\n");
> > >> + return false;
> > >> +}
> > >> +
> > >> static const struct drm_plane_funcs meson_plane_funcs = {
> > >> .update_plane = drm_atomic_helper_update_plane,
> > >> .disable_plane = drm_atomic_helper_disable_plane,
> > >> @@ -353,6 +457,7 @@ static const struct drm_plane_funcs meson_plane_funcs = {
> > >> .reset = drm_atomic_helper_plane_reset,
> > >> .atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
> > >> .atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
> > >> + .format_mod_supported = meson_plane_format_mod_supported,
> > >> };
> > >>
> > >> static const uint32_t supported_drm_formats[] = {
> > >> @@ -364,10 +469,53 @@ static const uint32_t supported_drm_formats[] = {
> > >> DRM_FORMAT_RGB565,
> > >> };
> > >>
> > >> +static const uint64_t format_modifiers_afbc_gxm[] = {
> > >> + DRM_FORMAT_MOD_ARM_AFBC(AFBC_FORMAT_MOD_BLOCK_SIZE_16x16 |
> > >> + AFBC_FORMAT_MOD_SPARSE |
> > >> + AFBC_FORMAT_MOD_YTR),
> > >> + /* SPLIT mandates SPARSE, RGB modes mandates YTR */
> > >> + DRM_FORMAT_MOD_ARM_AFBC(AFBC_FORMAT_MOD_BLOCK_SIZE_16x16 |
> > >> + AFBC_FORMAT_MOD_YTR |
> > >> + AFBC_FORMAT_MOD_SPARSE |
> > >> + AFBC_FORMAT_MOD_SPLIT),
> > >> + DRM_FORMAT_MOD_LINEAR,
> > >> + DRM_FORMAT_MOD_INVALID,
> > >> +};
> > >> +
> > >> +static const uint64_t format_modifiers_afbc_g12a[] = {
> > >> + /*
> > >> + * - TOFIX Support AFBC modifiers for YUV formats (16x16 + TILED)
> > >> + * - AFBC_FORMAT_MOD_YTR is mandatory since we only support RGB
> > >> + * - SPLIT is mandatory for performances reasons when in 16x16
> > >> + * block size
> > >> + * - 32x8 block size + SPLIT is mandatory with 4K frame size
> > >> + * for performances reasons
> > >> + */
> > >> + DRM_FORMAT_MOD_ARM_AFBC(AFBC_FORMAT_MOD_BLOCK_SIZE_16x16 |
> > >> + AFBC_FORMAT_MOD_YTR |
> > >> + AFBC_FORMAT_MOD_SPARSE |
> > >> + AFBC_FORMAT_MOD_SPLIT),
> > >> + DRM_FORMAT_MOD_ARM_AFBC(AFBC_FORMAT_MOD_BLOCK_SIZE_32x8 |
> > >> + AFBC_FORMAT_MOD_YTR |
> > >> + AFBC_FORMAT_MOD_SPARSE),
> > >> + DRM_FORMAT_MOD_ARM_AFBC(AFBC_FORMAT_MOD_BLOCK_SIZE_32x8 |
> > >> + AFBC_FORMAT_MOD_YTR |
> > >> + AFBC_FORMAT_MOD_SPARSE |
> > >> + AFBC_FORMAT_MOD_SPLIT),
> > >> + DRM_FORMAT_MOD_LINEAR,
> > >> + DRM_FORMAT_MOD_INVALID,
> > >> +};
> > >> +
> > >> +static const uint64_t format_modifiers_default[] = {
> > >> + DRM_FORMAT_MOD_LINEAR,
> > >> + DRM_FORMAT_MOD_INVALID,
> > >> +};
> > >> +
> > >> int meson_plane_create(struct meson_drm *priv)
> > >> {
> > >> struct meson_plane *meson_plane;
> > >> struct drm_plane *plane;
> > >> + const uint64_t *format_modifiers = format_modifiers_default;
> > >>
> > >> meson_plane = devm_kzalloc(priv->drm->dev, sizeof(*meson_plane),
> > >> GFP_KERNEL);
> > >> @@ -377,11 +525,16 @@ int meson_plane_create(struct meson_drm *priv)
> > >> meson_plane->priv = priv;
> > >> plane = &meson_plane->base;
> > >>
> > >> + if (meson_vpu_is_compatible(priv, VPU_COMPATIBLE_GXM))
> > >> + format_modifiers = format_modifiers_afbc_gxm;
> > >> + else if (meson_vpu_is_compatible(priv, VPU_COMPATIBLE_G12A))
> > >> + format_modifiers = format_modifiers_afbc_g12a;
> > >> +
> > >> drm_universal_plane_init(priv->drm, plane, 0xFF,
> > >> &meson_plane_funcs,
> > >> supported_drm_formats,
> > >> ARRAY_SIZE(supported_drm_formats),
> > >> - NULL,
> > >> + format_modifiers,
> > >> DRM_PLANE_TYPE_PRIMARY, "meson_primary_plane");
> > >>
> > >> drm_plane_helper_add(plane, &meson_plane_helper_funcs);
> > >> --
> > >> 2.22.0
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 4/7] drm/meson: plane: add support for AFBC mode for OSD1 plane
2019-10-11 7:46 ` Daniel Vetter
@ 2019-10-11 7:56 ` Daniel Stone
0 siblings, 0 replies; 22+ messages in thread
From: Daniel Stone @ 2019-10-11 7:56 UTC (permalink / raw)
To: Daniel Vetter
Cc: nd, Neil Armstrong, khilman, dri-devel, linux-amlogic,
Ayan Halder, linux-arm-kernel
Hi,
On Fri, 11 Oct 2019 at 08:46, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Thu, Oct 10, 2019 at 7:32 PM Ayan Halder <Ayan.Halder@arm.com> wrote:
> > On Thu, Oct 10, 2019 at 03:41:15PM +0200, Neil Armstrong wrote:
> > > Sorry I don't understand, you ask me to limit AFBC to ABGR8888 ?
> >
> > Apologies for the confusion, as per the link, the formats which are
> > suggested with AFBC_FORMAT_MOD_YTR are the BGR/ABGR formats (as
> > listed in the 'AFBC formats' table)
> >
> > Thus, any other permutation of the components might make it incompatible
> > with some other AFBC producers/consumers.
>
> Uh, that's not how this is supposed to be used. Drivers are meant to
> expose _everything_ they support (bonus if you roughly sort it in
> preference order). Userspace then computes the intersection of
> modifiers/formats supported by all devices it needs to share a buffer
> with. Allowing that was the entire point of modifiers, if we
> artificially limit to the common denominator we're back "only linear
> works everywhere".
Correct.
A lot of userspace will select for format first, then find a modifier
which can be used with that format. If userspace has specific
knowledge of AFBC and decides that it prefers to use AFBC so will seek
out an alpha format - and make sure everyone fills the channel solid -
then that's fine. But that's putting the cart before the horse.
Not exposing XRGB8888 on the primary plane will break a lot of
userspace, so in this case AFBC would just never really be used.
Cheers,
Daniel
_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 4/7] drm/meson: plane: add support for AFBC mode for OSD1 plane
2019-10-10 13:41 ` Neil Armstrong
2019-10-10 17:31 ` Ayan Halder
@ 2019-10-11 8:41 ` Brian Starkey
2019-10-11 9:14 ` Neil Armstrong
1 sibling, 1 reply; 22+ messages in thread
From: Brian Starkey @ 2019-10-11 8:41 UTC (permalink / raw)
To: Neil Armstrong
Cc: Ayan Halder, khilman, dri-devel, linux-amlogic, nd, linux-arm-kernel
Hi Neil,
On Thu, Oct 10, 2019 at 03:41:15PM +0200, Neil Armstrong wrote:
> Hi Ayan,
>
> On 10/10/2019 15:26, Ayan Halder wrote:
> > On Thu, Oct 10, 2019 at 11:25:23AM +0200, Neil Armstrong wrote:
> >> This adds all the OSD configuration plumbing to support the AFBC decoders
> >> path to display of the OSD1 plane.
> >>
> >> The Amlogic GXM and G12A AFBC decoders are integrated very differently.
> >>
> >> The Amlogic GXM has a direct output path to the OSD1 VIU pixel input,
> >> because the GXM AFBC decoder seem to be a custom IP developed by Amlogic.
> >>
> >> On the other side, the Amlogic G12A AFBC decoder seems to be an external
> >> IP that emit pixels on an AXI master hooked to a "Mali Unpack" block
> >> feeding the OSD1 VIU pixel input.
> >> This uses a weird "0x1000000" internal HW physical address on both
> >> sides to transfer the pixels.
> >>
> >> For Amlogic GXM, the supported pixel formats are the same as the normal
> >> linear OSD1 mode.
> >>
> >> On the other side, Amlogic added support for all AFBC v1.2 formats for
> >> the G12A AFBC integration.
> >>
> >> For simplicity, we stick to the already supported formats for now.
> >>
> >> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> >> ---
> >> drivers/gpu/drm/meson/meson_crtc.c | 2 +
> >> drivers/gpu/drm/meson/meson_drv.h | 4 +
> >> drivers/gpu/drm/meson/meson_plane.c | 215 ++++++++++++++++++++++++----
> >> 3 files changed, 190 insertions(+), 31 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/meson/meson_crtc.c b/drivers/gpu/drm/meson/meson_crtc.c
> >> index 57ae1c13d1e6..d478fa232951 100644
> >> --- a/drivers/gpu/drm/meson/meson_crtc.c
> >> +++ b/drivers/gpu/drm/meson/meson_crtc.c
> >> @@ -281,6 +281,8 @@ void meson_crtc_irq(struct meson_drm *priv)
> >> if (priv->viu.osd1_enabled && priv->viu.osd1_commit) {
> >> writel_relaxed(priv->viu.osd1_ctrl_stat,
> >> priv->io_base + _REG(VIU_OSD1_CTRL_STAT));
> >> + writel_relaxed(priv->viu.osd1_ctrl_stat2,
> >> + priv->io_base + _REG(VIU_OSD1_CTRL_STAT2));
> >> writel_relaxed(priv->viu.osd1_blk0_cfg[0],
> >> priv->io_base + _REG(VIU_OSD1_BLK0_CFG_W0));
> >> writel_relaxed(priv->viu.osd1_blk0_cfg[1],
> >> diff --git a/drivers/gpu/drm/meson/meson_drv.h b/drivers/gpu/drm/meson/meson_drv.h
> >> index 60f13c6f34e5..de25349be8aa 100644
> >> --- a/drivers/gpu/drm/meson/meson_drv.h
> >> +++ b/drivers/gpu/drm/meson/meson_drv.h
> >> @@ -53,8 +53,12 @@ struct meson_drm {
> >> bool osd1_enabled;
> >> bool osd1_interlace;
> >> bool osd1_commit;
> >> + bool osd1_afbcd;
> >> uint32_t osd1_ctrl_stat;
> >> + uint32_t osd1_ctrl_stat2;
> >> uint32_t osd1_blk0_cfg[5];
> >> + uint32_t osd1_blk1_cfg4;
> >> + uint32_t osd1_blk2_cfg4;
> >> uint32_t osd1_addr;
> >> uint32_t osd1_stride;
> >> uint32_t osd1_height;
> >> diff --git a/drivers/gpu/drm/meson/meson_plane.c b/drivers/gpu/drm/meson/meson_plane.c
> >> index 5e798c276037..412941aa8402 100644
> >> --- a/drivers/gpu/drm/meson/meson_plane.c
> >> +++ b/drivers/gpu/drm/meson/meson_plane.c
> >> @@ -23,6 +23,7 @@
> >> #include "meson_plane.h"
> >> #include "meson_registers.h"
> >> #include "meson_viu.h"
> >> +#include "meson_osd_afbcd.h"
> >>
> >> /* OSD_SCI_WH_M1 */
> >> #define SCI_WH_M1_W(w) FIELD_PREP(GENMASK(28, 16), w)
> >> @@ -92,12 +93,38 @@ static int meson_plane_atomic_check(struct drm_plane *plane,
> >> false, true);
> >> }
> >>
> >> +#define MESON_MOD_AFBC_VALID_BITS (AFBC_FORMAT_MOD_BLOCK_SIZE_16x16 | \
> >> + AFBC_FORMAT_MOD_BLOCK_SIZE_32x8 | \
> >> + AFBC_FORMAT_MOD_YTR | \
> >> + AFBC_FORMAT_MOD_SPARSE | \
> >> + AFBC_FORMAT_MOD_SPLIT)
> >> +
> >> /* Takes a fixed 16.16 number and converts it to integer. */
> >> static inline int64_t fixed16_to_int(int64_t value)
> >> {
> >> return value >> 16;
> >> }
> >>
> >> +static u32 meson_g12a_afbcd_line_stride(struct meson_drm *priv)
> >> +{
> >> + u32 line_stride = 0;
> >> +
> >> + switch (priv->afbcd.format) {
> >> + case DRM_FORMAT_RGB565:
> >> + line_stride = ((priv->viu.osd1_width << 4) + 127) >> 7;
> >> + break;
> >> + case DRM_FORMAT_RGB888:
> >> + case DRM_FORMAT_XRGB8888:
> >> + case DRM_FORMAT_ARGB8888:
> >> + case DRM_FORMAT_XBGR8888:
> >> + case DRM_FORMAT_ABGR8888:
> > Please have a look at
> > https://www.kernel.org/doc/html/latest/gpu/afbc.html for our
> > recommendation. We suggest that *X* formats are avoided.
> >
> > Also, for interoperability and maximum compression efficiency (with
> > AFBC_FORMAT_MOD_YTR), we suggest the following order :-
> >
> > Component 0: R
> > Component 1: G
> > Component 2: B
> > Component 3: A (if available)
>
>
> Sorry I don't understand, you ask me to limit AFBC to ABGR8888 ?
>
> But why if the HW (GPU and DPU) is capable of ?
AFBC doesn't have an in-memory component order in the traditional
sense (i.e. a bit-position to component mapping), so Arm
have decided to define the convention that DRM_FORMAT_ABGR8888
represents the AFBC layout with R in component 0.
Are you sure the GPU supports other orders? I think any Arm driver
will only be producing DRM_FORMATs with "BGR" order e.g. ABGR8888.
I'm not convinced the GPU HW actually supports any other order, but
it's all rather confusing with texture swizzling. What I can tell you
for sure is that it _does_ support BGR order (in DRM naming
convention).
If you do choose to expose orders other than BGR/ABGR, then you should
certainly not allow YTR to be used with any orders other than
BGR/ABGR. The AFBC spec defines YTR as using R in component 0, which
Arm has defined as DRM_FORMAT_*BGR* (component 0 in LE LSBs).
>
> Isn't it an userspace choice ? I understand XRGB8888 is a waste
> of memory space and compression efficiency, but this is not the
> kernel driver's to decide this, right ?
>
As long as it's agreed and understood what XRGB8888 means. It must be
an AFBC bitstream with 4-components, with B in component 0, G in
component 1, R in component 2 and 8 wasted bits in component 3.
I know of HW which treats "XBGR" with AFBC as a 3-component format,
which isn't correct but can easily lead to confusion and
incompatibility.
> For interoperability I'll understand recommending a minimal set
> of modifiers and formats. But here, each platform is also limited
> by it's GPU capabilites aswell.
>
The (Arm) GPUs support ABGR ordering, so if everyone sticks to that we
can make sure everything's nice and compatible (until someone turns up
with HW which _doesn't_ support that ordering).
> Limiting to ABGR8888 would discard like every non-Android renderers,
> using AFBC, I'm not sure it's the kernels driver's responsibility.
>
It prevents renderers with hard-coded pixel formats, perhaps. But
those are already fragile by nature, surely?
Cheers,
-Brian
> >
> > Thus, DRM_FORMAT_ABGR, DRM_FORMAT_BGR should only be allowed.
> >> + line_stride = ((priv->viu.osd1_width << 5) + 127) >> 7;
> >> + break;
> >> + }
> >> +
> >> + return ((line_stride + 1) >> 1) << 1;
> >> +}
> >> +
> >> static void meson_plane_atomic_update(struct drm_plane *plane,
> >> struct drm_plane_state *old_state)
> >> {
>
> [...]
>
> >>
> >> +static bool meson_plane_format_mod_supported(struct drm_plane *plane,
> >> + u32 format, u64 modifier)
> >> +{
> >> + struct meson_plane *meson_plane = to_meson_plane(plane);
> >> + struct meson_drm *priv = meson_plane->priv;
> >> + int i;
> >> +
> >> + if (modifier == DRM_FORMAT_MOD_INVALID)
> >> + return false;
> >> +
> >> + if (modifier == DRM_FORMAT_MOD_LINEAR)
> >> + return true;
> >> +
> >> + if (!meson_vpu_is_compatible(priv, VPU_COMPATIBLE_GXM) &&
> >> + !meson_vpu_is_compatible(priv, VPU_COMPATIBLE_G12A))
> >> + return false;
> >> +
> >> + if (modifier & ~DRM_FORMAT_MOD_ARM_AFBC(MESON_MOD_AFBC_VALID_BITS))
> >> + return false;
> >> +
> >> + for (i = 0 ; i < plane->modifier_count ; ++i)
> >> + if (plane->modifiers[i] == modifier)
> >> + break;
> >> +
> >> + if (i == plane->modifier_count) {
> >> + DRM_DEBUG_KMS("Unsupported modifier\n");
> >> + return false;
> >> + }
>
> I can add a warn_once here, would it be enough ?
>
> >> +
> >> + if (priv->afbcd.ops && priv->afbcd.ops->supported_fmt)
> >> + return priv->afbcd.ops->supported_fmt(modifier, format);
> >> +
> >> + DRM_DEBUG_KMS("AFBC Unsupported\n");
> >> + return false;
> >> +}
> >> +
> >> static const struct drm_plane_funcs meson_plane_funcs = {
> >> .update_plane = drm_atomic_helper_update_plane,
> >> .disable_plane = drm_atomic_helper_disable_plane,
> >> @@ -353,6 +457,7 @@ static const struct drm_plane_funcs meson_plane_funcs = {
> >> .reset = drm_atomic_helper_plane_reset,
> >> .atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
> >> .atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
> >> + .format_mod_supported = meson_plane_format_mod_supported,
> >> };
> >>
> >> static const uint32_t supported_drm_formats[] = {
> >> @@ -364,10 +469,53 @@ static const uint32_t supported_drm_formats[] = {
> >> DRM_FORMAT_RGB565,
> >> };
> >>
> >> +static const uint64_t format_modifiers_afbc_gxm[] = {
> >> + DRM_FORMAT_MOD_ARM_AFBC(AFBC_FORMAT_MOD_BLOCK_SIZE_16x16 |
> >> + AFBC_FORMAT_MOD_SPARSE |
> >> + AFBC_FORMAT_MOD_YTR),
> >> + /* SPLIT mandates SPARSE, RGB modes mandates YTR */
> >> + DRM_FORMAT_MOD_ARM_AFBC(AFBC_FORMAT_MOD_BLOCK_SIZE_16x16 |
> >> + AFBC_FORMAT_MOD_YTR |
> >> + AFBC_FORMAT_MOD_SPARSE |
> >> + AFBC_FORMAT_MOD_SPLIT),
> >> + DRM_FORMAT_MOD_LINEAR,
> >> + DRM_FORMAT_MOD_INVALID,
> >> +};
> >> +
> >> +static const uint64_t format_modifiers_afbc_g12a[] = {
> >> + /*
> >> + * - TOFIX Support AFBC modifiers for YUV formats (16x16 + TILED)
> >> + * - AFBC_FORMAT_MOD_YTR is mandatory since we only support RGB
> >> + * - SPLIT is mandatory for performances reasons when in 16x16
> >> + * block size
> >> + * - 32x8 block size + SPLIT is mandatory with 4K frame size
> >> + * for performances reasons
> >> + */
> >> + DRM_FORMAT_MOD_ARM_AFBC(AFBC_FORMAT_MOD_BLOCK_SIZE_16x16 |
> >> + AFBC_FORMAT_MOD_YTR |
> >> + AFBC_FORMAT_MOD_SPARSE |
> >> + AFBC_FORMAT_MOD_SPLIT),
> >> + DRM_FORMAT_MOD_ARM_AFBC(AFBC_FORMAT_MOD_BLOCK_SIZE_32x8 |
> >> + AFBC_FORMAT_MOD_YTR |
> >> + AFBC_FORMAT_MOD_SPARSE),
> >> + DRM_FORMAT_MOD_ARM_AFBC(AFBC_FORMAT_MOD_BLOCK_SIZE_32x8 |
> >> + AFBC_FORMAT_MOD_YTR |
> >> + AFBC_FORMAT_MOD_SPARSE |
> >> + AFBC_FORMAT_MOD_SPLIT),
> >> + DRM_FORMAT_MOD_LINEAR,
> >> + DRM_FORMAT_MOD_INVALID,
> >> +};
> >> +
> >> +static const uint64_t format_modifiers_default[] = {
> >> + DRM_FORMAT_MOD_LINEAR,
> >> + DRM_FORMAT_MOD_INVALID,
> >> +};
> >> +
> >> int meson_plane_create(struct meson_drm *priv)
> >> {
> >> struct meson_plane *meson_plane;
> >> struct drm_plane *plane;
> >> + const uint64_t *format_modifiers = format_modifiers_default;
> >>
> >> meson_plane = devm_kzalloc(priv->drm->dev, sizeof(*meson_plane),
> >> GFP_KERNEL);
> >> @@ -377,11 +525,16 @@ int meson_plane_create(struct meson_drm *priv)
> >> meson_plane->priv = priv;
> >> plane = &meson_plane->base;
> >>
> >> + if (meson_vpu_is_compatible(priv, VPU_COMPATIBLE_GXM))
> >> + format_modifiers = format_modifiers_afbc_gxm;
> >> + else if (meson_vpu_is_compatible(priv, VPU_COMPATIBLE_G12A))
> >> + format_modifiers = format_modifiers_afbc_g12a;
> >> +
> >> drm_universal_plane_init(priv->drm, plane, 0xFF,
> >> &meson_plane_funcs,
> >> supported_drm_formats,
> >> ARRAY_SIZE(supported_drm_formats),
> >> - NULL,
> >> + format_modifiers,
> >> DRM_PLANE_TYPE_PRIMARY, "meson_primary_plane");
> >>
> >> drm_plane_helper_add(plane, &meson_plane_helper_funcs);
> >> --
> >> 2.22.0
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 4/7] drm/meson: plane: add support for AFBC mode for OSD1 plane
2019-10-11 8:41 ` Brian Starkey
@ 2019-10-11 9:14 ` Neil Armstrong
2019-10-11 10:56 ` Brian Starkey
0 siblings, 1 reply; 22+ messages in thread
From: Neil Armstrong @ 2019-10-11 9:14 UTC (permalink / raw)
To: Brian Starkey
Cc: Ayan Halder, khilman, dri-devel, linux-amlogic, nd, linux-arm-kernel
Hi Brian,
On 11/10/2019 10:41, Brian Starkey wrote:
> Hi Neil,
>
> On Thu, Oct 10, 2019 at 03:41:15PM +0200, Neil Armstrong wrote:
>> Hi Ayan,
>>
>> On 10/10/2019 15:26, Ayan Halder wrote:
>>> On Thu, Oct 10, 2019 at 11:25:23AM +0200, Neil Armstrong wrote:
>>>> This adds all the OSD configuration plumbing to support the AFBC decoders
>>>> path to display of the OSD1 plane.
>>>>
>>>> The Amlogic GXM and G12A AFBC decoders are integrated very differently.
>>>>
>>>> The Amlogic GXM has a direct output path to the OSD1 VIU pixel input,
>>>> because the GXM AFBC decoder seem to be a custom IP developed by Amlogic.
>>>>
>>>> On the other side, the Amlogic G12A AFBC decoder seems to be an external
>>>> IP that emit pixels on an AXI master hooked to a "Mali Unpack" block
>>>> feeding the OSD1 VIU pixel input.
>>>> This uses a weird "0x1000000" internal HW physical address on both
>>>> sides to transfer the pixels.
>>>>
>>>> For Amlogic GXM, the supported pixel formats are the same as the normal
>>>> linear OSD1 mode.
>>>>
>>>> On the other side, Amlogic added support for all AFBC v1.2 formats for
>>>> the G12A AFBC integration.
>>>>
>>>> For simplicity, we stick to the already supported formats for now.
>>>>
>>>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>>>> ---
>>>> drivers/gpu/drm/meson/meson_crtc.c | 2 +
>>>> drivers/gpu/drm/meson/meson_drv.h | 4 +
>>>> drivers/gpu/drm/meson/meson_plane.c | 215 ++++++++++++++++++++++++----
>>>> 3 files changed, 190 insertions(+), 31 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/meson/meson_crtc.c b/drivers/gpu/drm/meson/meson_crtc.c
>>>> index 57ae1c13d1e6..d478fa232951 100644
>>>> --- a/drivers/gpu/drm/meson/meson_crtc.c
>>>> +++ b/drivers/gpu/drm/meson/meson_crtc.c
>>>> @@ -281,6 +281,8 @@ void meson_crtc_irq(struct meson_drm *priv)
>>>> if (priv->viu.osd1_enabled && priv->viu.osd1_commit) {
>>>> writel_relaxed(priv->viu.osd1_ctrl_stat,
>>>> priv->io_base + _REG(VIU_OSD1_CTRL_STAT));
>>>> + writel_relaxed(priv->viu.osd1_ctrl_stat2,
>>>> + priv->io_base + _REG(VIU_OSD1_CTRL_STAT2));
>>>> writel_relaxed(priv->viu.osd1_blk0_cfg[0],
>>>> priv->io_base + _REG(VIU_OSD1_BLK0_CFG_W0));
>>>> writel_relaxed(priv->viu.osd1_blk0_cfg[1],
>>>> diff --git a/drivers/gpu/drm/meson/meson_drv.h b/drivers/gpu/drm/meson/meson_drv.h
>>>> index 60f13c6f34e5..de25349be8aa 100644
>>>> --- a/drivers/gpu/drm/meson/meson_drv.h
>>>> +++ b/drivers/gpu/drm/meson/meson_drv.h
>>>> @@ -53,8 +53,12 @@ struct meson_drm {
>>>> bool osd1_enabled;
>>>> bool osd1_interlace;
>>>> bool osd1_commit;
>>>> + bool osd1_afbcd;
>>>> uint32_t osd1_ctrl_stat;
>>>> + uint32_t osd1_ctrl_stat2;
>>>> uint32_t osd1_blk0_cfg[5];
>>>> + uint32_t osd1_blk1_cfg4;
>>>> + uint32_t osd1_blk2_cfg4;
>>>> uint32_t osd1_addr;
>>>> uint32_t osd1_stride;
>>>> uint32_t osd1_height;
>>>> diff --git a/drivers/gpu/drm/meson/meson_plane.c b/drivers/gpu/drm/meson/meson_plane.c
>>>> index 5e798c276037..412941aa8402 100644
>>>> --- a/drivers/gpu/drm/meson/meson_plane.c
>>>> +++ b/drivers/gpu/drm/meson/meson_plane.c
>>>> @@ -23,6 +23,7 @@
>>>> #include "meson_plane.h"
>>>> #include "meson_registers.h"
>>>> #include "meson_viu.h"
>>>> +#include "meson_osd_afbcd.h"
>>>>
>>>> /* OSD_SCI_WH_M1 */
>>>> #define SCI_WH_M1_W(w) FIELD_PREP(GENMASK(28, 16), w)
>>>> @@ -92,12 +93,38 @@ static int meson_plane_atomic_check(struct drm_plane *plane,
>>>> false, true);
>>>> }
>>>>
>>>> +#define MESON_MOD_AFBC_VALID_BITS (AFBC_FORMAT_MOD_BLOCK_SIZE_16x16 | \
>>>> + AFBC_FORMAT_MOD_BLOCK_SIZE_32x8 | \
>>>> + AFBC_FORMAT_MOD_YTR | \
>>>> + AFBC_FORMAT_MOD_SPARSE | \
>>>> + AFBC_FORMAT_MOD_SPLIT)
>>>> +
>>>> /* Takes a fixed 16.16 number and converts it to integer. */
>>>> static inline int64_t fixed16_to_int(int64_t value)
>>>> {
>>>> return value >> 16;
>>>> }
>>>>
>>>> +static u32 meson_g12a_afbcd_line_stride(struct meson_drm *priv)
>>>> +{
>>>> + u32 line_stride = 0;
>>>> +
>>>> + switch (priv->afbcd.format) {
>>>> + case DRM_FORMAT_RGB565:
>>>> + line_stride = ((priv->viu.osd1_width << 4) + 127) >> 7;
>>>> + break;
>>>> + case DRM_FORMAT_RGB888:
>>>> + case DRM_FORMAT_XRGB8888:
>>>> + case DRM_FORMAT_ARGB8888:
>>>> + case DRM_FORMAT_XBGR8888:
>>>> + case DRM_FORMAT_ABGR8888:
>>> Please have a look at
>>> https://www.kernel.org/doc/html/latest/gpu/afbc.html for our
>>> recommendation. We suggest that *X* formats are avoided.
>>>
>>> Also, for interoperability and maximum compression efficiency (with
>>> AFBC_FORMAT_MOD_YTR), we suggest the following order :-
>>>
>>> Component 0: R
>>> Component 1: G
>>> Component 2: B
>>> Component 3: A (if available)
>>
>>
>> Sorry I don't understand, you ask me to limit AFBC to ABGR8888 ?
>>
>> But why if the HW (GPU and DPU) is capable of ?
>
> AFBC doesn't have an in-memory component order in the traditional
> sense (i.e. a bit-position to component mapping), so Arm
> have decided to define the convention that DRM_FORMAT_ABGR8888
> represents the AFBC layout with R in component 0.
In this implementation, we handle the ARGB/ABGR as the same mode
since the AFBC can only represent the layout as "ABGR" anyway.
>
> Are you sure the GPU supports other orders? I think any Arm driver
> will only be producing DRM_FORMATs with "BGR" order e.g. ABGR888>
> I'm not convinced the GPU HW actually supports any other order, but
> it's all rather confusing with texture swizzling. What I can tell you
> for sure is that it _does_ support BGR order (in DRM naming
> convention).
Well, since the Bifrost Mali blobs are closed-source and delivered
by licensees, it's hard to define what is supported from a closed
GPU HW, closed SW implementation to a closed pixel format implementation.
You'll have to tell us if the closed libMali handling AFBC would accept
ARGB8888 as format to render with AFBC enabled, if not you're right
I'll discard XRGB8888/ARGB8888 for AFBC buffers completely.
But it the libMali chooses tt generate an ARGB8888 buffer whatever
ARGB8888/XRGB8888/ABGR888/XBGR888 is asked, then no I'll keep it that way.
BTW I kept the vendor implementation here, which may be wrong but since
they have the AFBC IP license and Mali Bifrost GPU license...
>
> If you do choose to expose orders other than BGR/ABGR, then you should
> certainly not allow YTR to be used with any orders other than
> BGR/ABGR. The AFBC spec defines YTR as using R in component 0, which
> Arm has defined as DRM_FORMAT_*BGR* (component 0 in LE LSBs).
>
The MAFBC_FMT_RGBA8888 pixel format is defined in the AFBC decoder,
which seems to be an ARM IP, the registers documentation is in the
SoC datasheet at [1] and the formats bits are defined in the patch 3 at [2].
So it seems the decoder handles only a single type for 32bit RGB buffer
format, as Amlogic names it MAFBC_FMT_RGBA8888
For XRGB8888/XBGR8888 we simply "replace" the A component with a fixed
value in the pixel generator.
[1] https://dl.khadas.com/Hardware/VIM3/Datasheet/S905D3_datasheet_0.2_Wesion.pdf page 772
[2] https://patchwork.freedesktop.org/patch/335199/?series=67832&rev=1
>>
>> Isn't it an userspace choice ? I understand XRGB8888 is a waste
>> of memory space and compression efficiency, but this is not the
>> kernel driver's to decide this, right ?
>>
>
> As long as it's agreed and understood what XRGB8888 means. It must be
> an AFBC bitstream with 4-components, with B in component 0, G in
> component 1, R in component 2 and 8 wasted bits in component 3.
Yes, but this is something userspace must assume, and it's already
wasted in the linear XRGB8888 format anyway.
>
> I know of HW which treats "XBGR" with AFBC as a 3-component format,
> which isn't correct but can easily lead to confusion and
> incompatibility.
Seems it's not the case here, at least for the G12A SoC family.
>
>> For interoperability I'll understand recommending a minimal set
>> of modifiers and formats. But here, each platform is also limited
>> by it's GPU capabilites aswell.
>>
>
> The (Arm) GPUs support ABGR ordering, so if everyone sticks to that we
> can make sure everything's nice and compatible (until someone turns up
> with HW which _doesn't_ support that ordering).
This is not clean enough in the https://www.kernel.org/doc/html/latest/gpu/afbc.html
document. Since ARM is in control of the renderers, saying AFBC does _not_
support another components format as ABGR ordering in all the
OpenGL ES/Vulkan implementations, it would be clear we couldn't render
anything using AFBC with ARGB.
But we hit the closed-source/closed-specifications here again.
>
>> Limiting to ABGR8888 would discard like every non-Android renderers,
>> using AFBC, I'm not sure it's the kernels driver's responsibility.
>>
>
> It prevents renderers with hard-coded pixel formats, perhaps. But
> those are already fragile by nature, surely?
Well, except Android, all the other renderers uses ARGB8888/XRGB8888,
as fixed pixel format, which is quite a large amount of code.
Anyway, thanks for these technical clarifications, it makes things
much more clearer.
Neil
>
> Cheers,
> -Brian
>
>>>
>>> Thus, DRM_FORMAT_ABGR, DRM_FORMAT_BGR should only be allowed.
>>>> + line_stride = ((priv->viu.osd1_width << 5) + 127) >> 7;
>>>> + break;
>>>> + }
>>>> +
>>>> + return ((line_stride + 1) >> 1) << 1;
>>>> +}
>>>> +
>>>> static void meson_plane_atomic_update(struct drm_plane *plane,
>>>> struct drm_plane_state *old_state)
>>>> {
>>
>> [...]
>>
>>>>
>>>> +static bool meson_plane_format_mod_supported(struct drm_plane *plane,
>>>> + u32 format, u64 modifier)
>>>> +{
>>>> + struct meson_plane *meson_plane = to_meson_plane(plane);
>>>> + struct meson_drm *priv = meson_plane->priv;
>>>> + int i;
>>>> +
>>>> + if (modifier == DRM_FORMAT_MOD_INVALID)
>>>> + return false;
>>>> +
>>>> + if (modifier == DRM_FORMAT_MOD_LINEAR)
>>>> + return true;
>>>> +
>>>> + if (!meson_vpu_is_compatible(priv, VPU_COMPATIBLE_GXM) &&
>>>> + !meson_vpu_is_compatible(priv, VPU_COMPATIBLE_G12A))
>>>> + return false;
>>>> +
>>>> + if (modifier & ~DRM_FORMAT_MOD_ARM_AFBC(MESON_MOD_AFBC_VALID_BITS))
>>>> + return false;
>>>> +
>>>> + for (i = 0 ; i < plane->modifier_count ; ++i)
>>>> + if (plane->modifiers[i] == modifier)
>>>> + break;
>>>> +
>>>> + if (i == plane->modifier_count) {
>>>> + DRM_DEBUG_KMS("Unsupported modifier\n");
>>>> + return false;
>>>> + }
>>
>> I can add a warn_once here, would it be enough ?
>>
>>>> +
>>>> + if (priv->afbcd.ops && priv->afbcd.ops->supported_fmt)
>>>> + return priv->afbcd.ops->supported_fmt(modifier, format);
>>>> +
>>>> + DRM_DEBUG_KMS("AFBC Unsupported\n");
>>>> + return false;
>>>> +}
>>>> +
>>>> static const struct drm_plane_funcs meson_plane_funcs = {
>>>> .update_plane = drm_atomic_helper_update_plane,
>>>> .disable_plane = drm_atomic_helper_disable_plane,
>>>> @@ -353,6 +457,7 @@ static const struct drm_plane_funcs meson_plane_funcs = {
>>>> .reset = drm_atomic_helper_plane_reset,
>>>> .atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
>>>> .atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
>>>> + .format_mod_supported = meson_plane_format_mod_supported,
>>>> };
>>>>
>>>> static const uint32_t supported_drm_formats[] = {
>>>> @@ -364,10 +469,53 @@ static const uint32_t supported_drm_formats[] = {
>>>> DRM_FORMAT_RGB565,
>>>> };
>>>>
>>>> +static const uint64_t format_modifiers_afbc_gxm[] = {
>>>> + DRM_FORMAT_MOD_ARM_AFBC(AFBC_FORMAT_MOD_BLOCK_SIZE_16x16 |
>>>> + AFBC_FORMAT_MOD_SPARSE |
>>>> + AFBC_FORMAT_MOD_YTR),
>>>> + /* SPLIT mandates SPARSE, RGB modes mandates YTR */
>>>> + DRM_FORMAT_MOD_ARM_AFBC(AFBC_FORMAT_MOD_BLOCK_SIZE_16x16 |
>>>> + AFBC_FORMAT_MOD_YTR |
>>>> + AFBC_FORMAT_MOD_SPARSE |
>>>> + AFBC_FORMAT_MOD_SPLIT),
>>>> + DRM_FORMAT_MOD_LINEAR,
>>>> + DRM_FORMAT_MOD_INVALID,
>>>> +};
>>>> +
>>>> +static const uint64_t format_modifiers_afbc_g12a[] = {
>>>> + /*
>>>> + * - TOFIX Support AFBC modifiers for YUV formats (16x16 + TILED)
>>>> + * - AFBC_FORMAT_MOD_YTR is mandatory since we only support RGB
>>>> + * - SPLIT is mandatory for performances reasons when in 16x16
>>>> + * block size
>>>> + * - 32x8 block size + SPLIT is mandatory with 4K frame size
>>>> + * for performances reasons
>>>> + */
>>>> + DRM_FORMAT_MOD_ARM_AFBC(AFBC_FORMAT_MOD_BLOCK_SIZE_16x16 |
>>>> + AFBC_FORMAT_MOD_YTR |
>>>> + AFBC_FORMAT_MOD_SPARSE |
>>>> + AFBC_FORMAT_MOD_SPLIT),
>>>> + DRM_FORMAT_MOD_ARM_AFBC(AFBC_FORMAT_MOD_BLOCK_SIZE_32x8 |
>>>> + AFBC_FORMAT_MOD_YTR |
>>>> + AFBC_FORMAT_MOD_SPARSE),
>>>> + DRM_FORMAT_MOD_ARM_AFBC(AFBC_FORMAT_MOD_BLOCK_SIZE_32x8 |
>>>> + AFBC_FORMAT_MOD_YTR |
>>>> + AFBC_FORMAT_MOD_SPARSE |
>>>> + AFBC_FORMAT_MOD_SPLIT),
>>>> + DRM_FORMAT_MOD_LINEAR,
>>>> + DRM_FORMAT_MOD_INVALID,
>>>> +};
>>>> +
>>>> +static const uint64_t format_modifiers_default[] = {
>>>> + DRM_FORMAT_MOD_LINEAR,
>>>> + DRM_FORMAT_MOD_INVALID,
>>>> +};
>>>> +
>>>> int meson_plane_create(struct meson_drm *priv)
>>>> {
>>>> struct meson_plane *meson_plane;
>>>> struct drm_plane *plane;
>>>> + const uint64_t *format_modifiers = format_modifiers_default;
>>>>
>>>> meson_plane = devm_kzalloc(priv->drm->dev, sizeof(*meson_plane),
>>>> GFP_KERNEL);
>>>> @@ -377,11 +525,16 @@ int meson_plane_create(struct meson_drm *priv)
>>>> meson_plane->priv = priv;
>>>> plane = &meson_plane->base;
>>>>
>>>> + if (meson_vpu_is_compatible(priv, VPU_COMPATIBLE_GXM))
>>>> + format_modifiers = format_modifiers_afbc_gxm;
>>>> + else if (meson_vpu_is_compatible(priv, VPU_COMPATIBLE_G12A))
>>>> + format_modifiers = format_modifiers_afbc_g12a;
>>>> +
>>>> drm_universal_plane_init(priv->drm, plane, 0xFF,
>>>> &meson_plane_funcs,
>>>> supported_drm_formats,
>>>> ARRAY_SIZE(supported_drm_formats),
>>>> - NULL,
>>>> + format_modifiers,
>>>> DRM_PLANE_TYPE_PRIMARY, "meson_primary_plane");
>>>>
>>>> drm_plane_helper_add(plane, &meson_plane_helper_funcs);
>>>> --
>>>> 2.22.0
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 4/7] drm/meson: plane: add support for AFBC mode for OSD1 plane
2019-10-11 9:14 ` Neil Armstrong
@ 2019-10-11 10:56 ` Brian Starkey
2019-10-11 12:07 ` Neil Armstrong
2019-10-11 17:25 ` Daniel Vetter
0 siblings, 2 replies; 22+ messages in thread
From: Brian Starkey @ 2019-10-11 10:56 UTC (permalink / raw)
To: Neil Armstrong
Cc: Ayan Halder, khilman, dri-devel, linux-amlogic, nd, linux-arm-kernel
Hi,
On Fri, Oct 11, 2019 at 11:14:43AM +0200, Neil Armstrong wrote:
> Hi Brian,
>
> On 11/10/2019 10:41, Brian Starkey wrote:
> > Hi Neil,
> >
> > On Thu, Oct 10, 2019 at 03:41:15PM +0200, Neil Armstrong wrote:
> >> Hi Ayan,
> >>
> >> On 10/10/2019 15:26, Ayan Halder wrote:
> >>> On Thu, Oct 10, 2019 at 11:25:23AM +0200, Neil Armstrong wrote:
> >>>> This adds all the OSD configuration plumbing to support the AFBC decoders
> >>>> path to display of the OSD1 plane.
> >>>>
> >>>> The Amlogic GXM and G12A AFBC decoders are integrated very differently.
> >>>>
> >>>> The Amlogic GXM has a direct output path to the OSD1 VIU pixel input,
> >>>> because the GXM AFBC decoder seem to be a custom IP developed by Amlogic.
> >>>>
> >>>> On the other side, the Amlogic G12A AFBC decoder seems to be an external
> >>>> IP that emit pixels on an AXI master hooked to a "Mali Unpack" block
> >>>> feeding the OSD1 VIU pixel input.
> >>>> This uses a weird "0x1000000" internal HW physical address on both
> >>>> sides to transfer the pixels.
> >>>>
> >>>> For Amlogic GXM, the supported pixel formats are the same as the normal
> >>>> linear OSD1 mode.
> >>>>
> >>>> On the other side, Amlogic added support for all AFBC v1.2 formats for
> >>>> the G12A AFBC integration.
> >>>>
> >>>> For simplicity, we stick to the already supported formats for now.
> >>>>
> >>>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> >>>> ---
> >>>> drivers/gpu/drm/meson/meson_crtc.c | 2 +
> >>>> drivers/gpu/drm/meson/meson_drv.h | 4 +
> >>>> drivers/gpu/drm/meson/meson_plane.c | 215 ++++++++++++++++++++++++----
> >>>> 3 files changed, 190 insertions(+), 31 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/meson/meson_crtc.c b/drivers/gpu/drm/meson/meson_crtc.c
> >>>> index 57ae1c13d1e6..d478fa232951 100644
> >>>> --- a/drivers/gpu/drm/meson/meson_crtc.c
> >>>> +++ b/drivers/gpu/drm/meson/meson_crtc.c
> >>>> @@ -281,6 +281,8 @@ void meson_crtc_irq(struct meson_drm *priv)
> >>>> if (priv->viu.osd1_enabled && priv->viu.osd1_commit) {
> >>>> writel_relaxed(priv->viu.osd1_ctrl_stat,
> >>>> priv->io_base + _REG(VIU_OSD1_CTRL_STAT));
> >>>> + writel_relaxed(priv->viu.osd1_ctrl_stat2,
> >>>> + priv->io_base + _REG(VIU_OSD1_CTRL_STAT2));
> >>>> writel_relaxed(priv->viu.osd1_blk0_cfg[0],
> >>>> priv->io_base + _REG(VIU_OSD1_BLK0_CFG_W0));
> >>>> writel_relaxed(priv->viu.osd1_blk0_cfg[1],
> >>>> diff --git a/drivers/gpu/drm/meson/meson_drv.h b/drivers/gpu/drm/meson/meson_drv.h
> >>>> index 60f13c6f34e5..de25349be8aa 100644
> >>>> --- a/drivers/gpu/drm/meson/meson_drv.h
> >>>> +++ b/drivers/gpu/drm/meson/meson_drv.h
> >>>> @@ -53,8 +53,12 @@ struct meson_drm {
> >>>> bool osd1_enabled;
> >>>> bool osd1_interlace;
> >>>> bool osd1_commit;
> >>>> + bool osd1_afbcd;
> >>>> uint32_t osd1_ctrl_stat;
> >>>> + uint32_t osd1_ctrl_stat2;
> >>>> uint32_t osd1_blk0_cfg[5];
> >>>> + uint32_t osd1_blk1_cfg4;
> >>>> + uint32_t osd1_blk2_cfg4;
> >>>> uint32_t osd1_addr;
> >>>> uint32_t osd1_stride;
> >>>> uint32_t osd1_height;
> >>>> diff --git a/drivers/gpu/drm/meson/meson_plane.c b/drivers/gpu/drm/meson/meson_plane.c
> >>>> index 5e798c276037..412941aa8402 100644
> >>>> --- a/drivers/gpu/drm/meson/meson_plane.c
> >>>> +++ b/drivers/gpu/drm/meson/meson_plane.c
> >>>> @@ -23,6 +23,7 @@
> >>>> #include "meson_plane.h"
> >>>> #include "meson_registers.h"
> >>>> #include "meson_viu.h"
> >>>> +#include "meson_osd_afbcd.h"
> >>>>
> >>>> /* OSD_SCI_WH_M1 */
> >>>> #define SCI_WH_M1_W(w) FIELD_PREP(GENMASK(28, 16), w)
> >>>> @@ -92,12 +93,38 @@ static int meson_plane_atomic_check(struct drm_plane *plane,
> >>>> false, true);
> >>>> }
> >>>>
> >>>> +#define MESON_MOD_AFBC_VALID_BITS (AFBC_FORMAT_MOD_BLOCK_SIZE_16x16 | \
> >>>> + AFBC_FORMAT_MOD_BLOCK_SIZE_32x8 | \
> >>>> + AFBC_FORMAT_MOD_YTR | \
> >>>> + AFBC_FORMAT_MOD_SPARSE | \
> >>>> + AFBC_FORMAT_MOD_SPLIT)
> >>>> +
> >>>> /* Takes a fixed 16.16 number and converts it to integer. */
> >>>> static inline int64_t fixed16_to_int(int64_t value)
> >>>> {
> >>>> return value >> 16;
> >>>> }
> >>>>
> >>>> +static u32 meson_g12a_afbcd_line_stride(struct meson_drm *priv)
> >>>> +{
> >>>> + u32 line_stride = 0;
> >>>> +
> >>>> + switch (priv->afbcd.format) {
> >>>> + case DRM_FORMAT_RGB565:
> >>>> + line_stride = ((priv->viu.osd1_width << 4) + 127) >> 7;
> >>>> + break;
> >>>> + case DRM_FORMAT_RGB888:
> >>>> + case DRM_FORMAT_XRGB8888:
> >>>> + case DRM_FORMAT_ARGB8888:
> >>>> + case DRM_FORMAT_XBGR8888:
> >>>> + case DRM_FORMAT_ABGR8888:
> >>> Please have a look at
> >>> https://www.kernel.org/doc/html/latest/gpu/afbc.html for our
> >>> recommendation. We suggest that *X* formats are avoided.
> >>>
> >>> Also, for interoperability and maximum compression efficiency (with
> >>> AFBC_FORMAT_MOD_YTR), we suggest the following order :-
> >>>
> >>> Component 0: R
> >>> Component 1: G
> >>> Component 2: B
> >>> Component 3: A (if available)
> >>
> >>
> >> Sorry I don't understand, you ask me to limit AFBC to ABGR8888 ?
> >>
> >> But why if the HW (GPU and DPU) is capable of ?
> >
> > AFBC doesn't have an in-memory component order in the traditional
> > sense (i.e. a bit-position to component mapping), so Arm
> > have decided to define the convention that DRM_FORMAT_ABGR8888
> > represents the AFBC layout with R in component 0.
>
> In this implementation, we handle the ARGB/ABGR as the same mode
> since the AFBC can only represent the layout as "ABGR" anyway.
>
In this case, with the external AFBC IP, there's a whole extra layer
of potential confusion :-(
The decoder only needs to know the number of components - so
irrespective of what color channel is mapped to what component, it can
always be configured with the same mode for 4-component 32-bit
formats.
For everything to work correctly with YTR, the thing consuming the
output from the decoder must treat component 0 as 'R', but otherwise
it doesn't matter.
Is your HW able to treat the decoder output in different ways? e.g.
mapping component 0 to 'B'? If that's the case, then exposing the
different orders is valid - but only ABGR should allow YTR.
> >
> > Are you sure the GPU supports other orders? I think any Arm driver
> > will only be producing DRM_FORMATs with "BGR" order e.g. ABGR888>
> > I'm not convinced the GPU HW actually supports any other order, but
> > it's all rather confusing with texture swizzling. What I can tell you
> > for sure is that it _does_ support BGR order (in DRM naming
> > convention).
>
> Well, since the Bifrost Mali blobs are closed-source and delivered
> by licensees, it's hard to define what is supported from a closed
> GPU HW, closed SW implementation to a closed pixel format implementation.
>
I hear you. IMO the only way to make any of this clear is to publish
reference data and tests which make sure implementations match each
other. It's something I'm trying to make happen.
> You'll have to tell us if the closed libMali handling AFBC would accept
> ARGB8888 as format to render with AFBC enabled, if not you're right
> I'll discard XRGB8888/ARGB8888 for AFBC buffers completely.
>
> But it the libMali chooses tt generate an ARGB8888 buffer whatever
> ARGB8888/XRGB8888/ABGR888/XBGR888 is asked, then no I'll keep it that way.
>
Yeah, I'll try and get clarity on this. It's not at all clear to me
either. When you say "accept ARGB8888 as format to render with AFBC
enabled", which API are you referring to, just so I can be clear? Do
you have an example of some code you're using to render AFBC with the
GPU blob?
In many APIs, there's no real expectation on in-memory component
order, so perhaps there treating them as all the same is acceptable.
However, fourcc + AFBC modifier is explicit in terms of component
order, and so IMO it's very harmful to "ignore" component order in
interfaces using fourcc + AFBC modifier.
There are implementations which support other orders, so ignoring
order will break those implementations. In some cases (Android, maybe
GL), this can be hidden behind "driver magic", but if the API is
fourcc + AFBC modifier, IMO it had better be completely explicit with
no tricks - irrespective of whatever other less-prescriptive APIs do.
> BTW I kept the vendor implementation here, which may be wrong but since
> they have the AFBC IP license and Mali Bifrost GPU license...
>
> >
> > If you do choose to expose orders other than BGR/ABGR, then you should
> > certainly not allow YTR to be used with any orders other than
> > BGR/ABGR. The AFBC spec defines YTR as using R in component 0, which
> > Arm has defined as DRM_FORMAT_*BGR* (component 0 in LE LSBs).
> >
>
> The MAFBC_FMT_RGBA8888 pixel format is defined in the AFBC decoder,
> which seems to be an ARM IP, the registers documentation is in the
> SoC datasheet at [1] and the formats bits are defined in the patch 3 at [2].
>
> So it seems the decoder handles only a single type for 32bit RGB buffer
> format, as Amlogic names it MAFBC_FMT_RGBA8888
>
Hopefully my comments at the beginning of this mail helps clear this
part up a bit.
> For XRGB8888/XBGR8888 we simply "replace" the A component with a fixed
> value in the pixel generator.
That seems correct, so long as the decoder is configured in the
4-component mode.
>
> [1] https://dl.khadas.com/Hardware/VIM3/Datasheet/S905D3_datasheet_0.2_Wesion.pdf page 772
> [2] https://patchwork.freedesktop.org/patch/335199/?series=67832&rev=1
>
> >>
> >> Isn't it an userspace choice ? I understand XRGB8888 is a waste
> >> of memory space and compression efficiency, but this is not the
> >> kernel driver's to decide this, right ?
> >>
> >
> > As long as it's agreed and understood what XRGB8888 means. It must be
> > an AFBC bitstream with 4-components, with B in component 0, G in
> > component 1, R in component 2 and 8 wasted bits in component 3.
>
> Yes, but this is something userspace must assume, and it's already
> wasted in the linear XRGB8888 format anyway.
>
> >
> > I know of HW which treats "XBGR" with AFBC as a 3-component format,
> > which isn't correct but can easily lead to confusion and
> > incompatibility.
>
> Seems it's not the case here, at least for the G12A SoC family.
That's good :-)
>
> >
> >> For interoperability I'll understand recommending a minimal set
> >> of modifiers and formats. But here, each platform is also limited
> >> by it's GPU capabilites aswell.
> >>
> >
> > The (Arm) GPUs support ABGR ordering, so if everyone sticks to that we
> > can make sure everything's nice and compatible (until someone turns up
> > with HW which _doesn't_ support that ordering).
>
> This is not clean enough in the https://www.kernel.org/doc/html/latest/gpu/afbc.html
> document. Since ARM is in control of the renderers, saying AFBC does _not_
> support another components format as ABGR ordering in all the
> OpenGL ES/Vulkan implementations, it would be clear we couldn't render
> anything using AFBC with ARGB.
> But we hit the closed-source/closed-specifications here again.
>
I didn't really understand the middle sentence.
I know and understand that the "closed-ness" is a problem. The page
you linked was an initial attempt at making a clear, public
specification.
What I need to be clear about, though, is that it describes _only_
cases where DRM fourcc + AFBC modifier are used. I don't think there's
any sane way to apply it to other APIs, because the formats are
described differently, and the "leeway" allowed for doing things
"under-the-hood" is very different.
> >
> >> Limiting to ABGR8888 would discard like every non-Android renderers,
> >> using AFBC, I'm not sure it's the kernels driver's responsibility.
> >>
> >
> > It prevents renderers with hard-coded pixel formats, perhaps. But
> > those are already fragile by nature, surely?
>
> Well, except Android, all the other renderers uses ARGB8888/XRGB8888,
> as fixed pixel format, which is quite a large amount of code.
>
I think whether that matters or not really depends on which graphics
APIs you're referring to. IMO it's inevitable that modifiers don't
simply "drop in" everywhere. The kernel API allows you to query what's
supported and pick that.
Thanks,
-Brian
>
> Anyway, thanks for these technical clarifications, it makes things
> much more clearer.
>
> Neil
>
> >
> > Cheers,
> > -Brian
> >
> >>>
> >>> Thus, DRM_FORMAT_ABGR, DRM_FORMAT_BGR should only be allowed.
> >>>> + line_stride = ((priv->viu.osd1_width << 5) + 127) >> 7;
> >>>> + break;
> >>>> + }
> >>>> +
> >>>> + return ((line_stride + 1) >> 1) << 1;
> >>>> +}
> >>>> +
> >>>> static void meson_plane_atomic_update(struct drm_plane *plane,
> >>>> struct drm_plane_state *old_state)
> >>>> {
> >>
> >> [...]
> >>
> >>>>
> >>>> +static bool meson_plane_format_mod_supported(struct drm_plane *plane,
> >>>> + u32 format, u64 modifier)
> >>>> +{
> >>>> + struct meson_plane *meson_plane = to_meson_plane(plane);
> >>>> + struct meson_drm *priv = meson_plane->priv;
> >>>> + int i;
> >>>> +
> >>>> + if (modifier == DRM_FORMAT_MOD_INVALID)
> >>>> + return false;
> >>>> +
> >>>> + if (modifier == DRM_FORMAT_MOD_LINEAR)
> >>>> + return true;
> >>>> +
> >>>> + if (!meson_vpu_is_compatible(priv, VPU_COMPATIBLE_GXM) &&
> >>>> + !meson_vpu_is_compatible(priv, VPU_COMPATIBLE_G12A))
> >>>> + return false;
> >>>> +
> >>>> + if (modifier & ~DRM_FORMAT_MOD_ARM_AFBC(MESON_MOD_AFBC_VALID_BITS))
> >>>> + return false;
> >>>> +
> >>>> + for (i = 0 ; i < plane->modifier_count ; ++i)
> >>>> + if (plane->modifiers[i] == modifier)
> >>>> + break;
> >>>> +
> >>>> + if (i == plane->modifier_count) {
> >>>> + DRM_DEBUG_KMS("Unsupported modifier\n");
> >>>> + return false;
> >>>> + }
> >>
> >> I can add a warn_once here, would it be enough ?
> >>
> >>>> +
> >>>> + if (priv->afbcd.ops && priv->afbcd.ops->supported_fmt)
> >>>> + return priv->afbcd.ops->supported_fmt(modifier, format);
> >>>> +
> >>>> + DRM_DEBUG_KMS("AFBC Unsupported\n");
> >>>> + return false;
> >>>> +}
> >>>> +
> >>>> static const struct drm_plane_funcs meson_plane_funcs = {
> >>>> .update_plane = drm_atomic_helper_update_plane,
> >>>> .disable_plane = drm_atomic_helper_disable_plane,
> >>>> @@ -353,6 +457,7 @@ static const struct drm_plane_funcs meson_plane_funcs = {
> >>>> .reset = drm_atomic_helper_plane_reset,
> >>>> .atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
> >>>> .atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
> >>>> + .format_mod_supported = meson_plane_format_mod_supported,
> >>>> };
> >>>>
> >>>> static const uint32_t supported_drm_formats[] = {
> >>>> @@ -364,10 +469,53 @@ static const uint32_t supported_drm_formats[] = {
> >>>> DRM_FORMAT_RGB565,
> >>>> };
> >>>>
> >>>> +static const uint64_t format_modifiers_afbc_gxm[] = {
> >>>> + DRM_FORMAT_MOD_ARM_AFBC(AFBC_FORMAT_MOD_BLOCK_SIZE_16x16 |
> >>>> + AFBC_FORMAT_MOD_SPARSE |
> >>>> + AFBC_FORMAT_MOD_YTR),
> >>>> + /* SPLIT mandates SPARSE, RGB modes mandates YTR */
> >>>> + DRM_FORMAT_MOD_ARM_AFBC(AFBC_FORMAT_MOD_BLOCK_SIZE_16x16 |
> >>>> + AFBC_FORMAT_MOD_YTR |
> >>>> + AFBC_FORMAT_MOD_SPARSE |
> >>>> + AFBC_FORMAT_MOD_SPLIT),
> >>>> + DRM_FORMAT_MOD_LINEAR,
> >>>> + DRM_FORMAT_MOD_INVALID,
> >>>> +};
> >>>> +
> >>>> +static const uint64_t format_modifiers_afbc_g12a[] = {
> >>>> + /*
> >>>> + * - TOFIX Support AFBC modifiers for YUV formats (16x16 + TILED)
> >>>> + * - AFBC_FORMAT_MOD_YTR is mandatory since we only support RGB
> >>>> + * - SPLIT is mandatory for performances reasons when in 16x16
> >>>> + * block size
> >>>> + * - 32x8 block size + SPLIT is mandatory with 4K frame size
> >>>> + * for performances reasons
> >>>> + */
> >>>> + DRM_FORMAT_MOD_ARM_AFBC(AFBC_FORMAT_MOD_BLOCK_SIZE_16x16 |
> >>>> + AFBC_FORMAT_MOD_YTR |
> >>>> + AFBC_FORMAT_MOD_SPARSE |
> >>>> + AFBC_FORMAT_MOD_SPLIT),
> >>>> + DRM_FORMAT_MOD_ARM_AFBC(AFBC_FORMAT_MOD_BLOCK_SIZE_32x8 |
> >>>> + AFBC_FORMAT_MOD_YTR |
> >>>> + AFBC_FORMAT_MOD_SPARSE),
> >>>> + DRM_FORMAT_MOD_ARM_AFBC(AFBC_FORMAT_MOD_BLOCK_SIZE_32x8 |
> >>>> + AFBC_FORMAT_MOD_YTR |
> >>>> + AFBC_FORMAT_MOD_SPARSE |
> >>>> + AFBC_FORMAT_MOD_SPLIT),
> >>>> + DRM_FORMAT_MOD_LINEAR,
> >>>> + DRM_FORMAT_MOD_INVALID,
> >>>> +};
> >>>> +
> >>>> +static const uint64_t format_modifiers_default[] = {
> >>>> + DRM_FORMAT_MOD_LINEAR,
> >>>> + DRM_FORMAT_MOD_INVALID,
> >>>> +};
> >>>> +
> >>>> int meson_plane_create(struct meson_drm *priv)
> >>>> {
> >>>> struct meson_plane *meson_plane;
> >>>> struct drm_plane *plane;
> >>>> + const uint64_t *format_modifiers = format_modifiers_default;
> >>>>
> >>>> meson_plane = devm_kzalloc(priv->drm->dev, sizeof(*meson_plane),
> >>>> GFP_KERNEL);
> >>>> @@ -377,11 +525,16 @@ int meson_plane_create(struct meson_drm *priv)
> >>>> meson_plane->priv = priv;
> >>>> plane = &meson_plane->base;
> >>>>
> >>>> + if (meson_vpu_is_compatible(priv, VPU_COMPATIBLE_GXM))
> >>>> + format_modifiers = format_modifiers_afbc_gxm;
> >>>> + else if (meson_vpu_is_compatible(priv, VPU_COMPATIBLE_G12A))
> >>>> + format_modifiers = format_modifiers_afbc_g12a;
> >>>> +
> >>>> drm_universal_plane_init(priv->drm, plane, 0xFF,
> >>>> &meson_plane_funcs,
> >>>> supported_drm_formats,
> >>>> ARRAY_SIZE(supported_drm_formats),
> >>>> - NULL,
> >>>> + format_modifiers,
> >>>> DRM_PLANE_TYPE_PRIMARY, "meson_primary_plane");
> >>>>
> >>>> drm_plane_helper_add(plane, &meson_plane_helper_funcs);
> >>>> --
> >>>> 2.22.0
> >>
> >> _______________________________________________
> >> dri-devel mailing list
> >> dri-devel@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 4/7] drm/meson: plane: add support for AFBC mode for OSD1 plane
2019-10-11 10:56 ` Brian Starkey
@ 2019-10-11 12:07 ` Neil Armstrong
2019-10-15 11:18 ` Brian Starkey
2019-10-11 17:25 ` Daniel Vetter
1 sibling, 1 reply; 22+ messages in thread
From: Neil Armstrong @ 2019-10-11 12:07 UTC (permalink / raw)
To: Brian Starkey
Cc: Ayan Halder, khilman, dri-devel, linux-amlogic, nd, linux-arm-kernel
On 11/10/2019 12:56, Brian Starkey wrote:
> Hi,
>
> On Fri, Oct 11, 2019 at 11:14:43AM +0200, Neil Armstrong wrote:
>> Hi Brian,
>>
>> On 11/10/2019 10:41, Brian Starkey wrote:
>>> Hi Neil,
>>>
>>> On Thu, Oct 10, 2019 at 03:41:15PM +0200, Neil Armstrong wrote:
>>>> Hi Ayan,
>>>>
>>>> On 10/10/2019 15:26, Ayan Halder wrote:
>>>>> On Thu, Oct 10, 2019 at 11:25:23AM +0200, Neil Armstrong wrote:
>>>>>> This adds all the OSD configuration plumbing to support the AFBC decoders
>>>>>> path to display of the OSD1 plane.
>>>>>>
>>>>>> The Amlogic GXM and G12A AFBC decoders are integrated very differently.
>>>>>>
>>>>>> The Amlogic GXM has a direct output path to the OSD1 VIU pixel input,
>>>>>> because the GXM AFBC decoder seem to be a custom IP developed by Amlogic.
>>>>>>
>>>>>> On the other side, the Amlogic G12A AFBC decoder seems to be an external
>>>>>> IP that emit pixels on an AXI master hooked to a "Mali Unpack" block
>>>>>> feeding the OSD1 VIU pixel input.
>>>>>> This uses a weird "0x1000000" internal HW physical address on both
>>>>>> sides to transfer the pixels.
>>>>>>
>>>>>> For Amlogic GXM, the supported pixel formats are the same as the normal
>>>>>> linear OSD1 mode.
>>>>>>
>>>>>> On the other side, Amlogic added support for all AFBC v1.2 formats for
>>>>>> the G12A AFBC integration.
>>>>>>
>>>>>> For simplicity, we stick to the already supported formats for now.
>>>>>>
>>>>>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>>>>>> ---
>>>>>> drivers/gpu/drm/meson/meson_crtc.c | 2 +
>>>>>> drivers/gpu/drm/meson/meson_drv.h | 4 +
>>>>>> drivers/gpu/drm/meson/meson_plane.c | 215 ++++++++++++++++++++++++----
>>>>>> 3 files changed, 190 insertions(+), 31 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/meson/meson_crtc.c b/drivers/gpu/drm/meson/meson_crtc.c
>>>>>> index 57ae1c13d1e6..d478fa232951 100644
>>>>>> --- a/drivers/gpu/drm/meson/meson_crtc.c
>>>>>> +++ b/drivers/gpu/drm/meson/meson_crtc.c
>>>>>> @@ -281,6 +281,8 @@ void meson_crtc_irq(struct meson_drm *priv)
>>>>>> if (priv->viu.osd1_enabled && priv->viu.osd1_commit) {
>>>>>> writel_relaxed(priv->viu.osd1_ctrl_stat,
>>>>>> priv->io_base + _REG(VIU_OSD1_CTRL_STAT));
>>>>>> + writel_relaxed(priv->viu.osd1_ctrl_stat2,
>>>>>> + priv->io_base + _REG(VIU_OSD1_CTRL_STAT2));
>>>>>> writel_relaxed(priv->viu.osd1_blk0_cfg[0],
>>>>>> priv->io_base + _REG(VIU_OSD1_BLK0_CFG_W0));
>>>>>> writel_relaxed(priv->viu.osd1_blk0_cfg[1],
>>>>>> diff --git a/drivers/gpu/drm/meson/meson_drv.h b/drivers/gpu/drm/meson/meson_drv.h
>>>>>> index 60f13c6f34e5..de25349be8aa 100644
>>>>>> --- a/drivers/gpu/drm/meson/meson_drv.h
>>>>>> +++ b/drivers/gpu/drm/meson/meson_drv.h
>>>>>> @@ -53,8 +53,12 @@ struct meson_drm {
>>>>>> bool osd1_enabled;
>>>>>> bool osd1_interlace;
>>>>>> bool osd1_commit;
>>>>>> + bool osd1_afbcd;
>>>>>> uint32_t osd1_ctrl_stat;
>>>>>> + uint32_t osd1_ctrl_stat2;
>>>>>> uint32_t osd1_blk0_cfg[5];
>>>>>> + uint32_t osd1_blk1_cfg4;
>>>>>> + uint32_t osd1_blk2_cfg4;
>>>>>> uint32_t osd1_addr;
>>>>>> uint32_t osd1_stride;
>>>>>> uint32_t osd1_height;
>>>>>> diff --git a/drivers/gpu/drm/meson/meson_plane.c b/drivers/gpu/drm/meson/meson_plane.c
>>>>>> index 5e798c276037..412941aa8402 100644
>>>>>> --- a/drivers/gpu/drm/meson/meson_plane.c
>>>>>> +++ b/drivers/gpu/drm/meson/meson_plane.c
>>>>>> @@ -23,6 +23,7 @@
>>>>>> #include "meson_plane.h"
>>>>>> #include "meson_registers.h"
>>>>>> #include "meson_viu.h"
>>>>>> +#include "meson_osd_afbcd.h"
>>>>>>
>>>>>> /* OSD_SCI_WH_M1 */
>>>>>> #define SCI_WH_M1_W(w) FIELD_PREP(GENMASK(28, 16), w)
>>>>>> @@ -92,12 +93,38 @@ static int meson_plane_atomic_check(struct drm_plane *plane,
>>>>>> false, true);
>>>>>> }
>>>>>>
>>>>>> +#define MESON_MOD_AFBC_VALID_BITS (AFBC_FORMAT_MOD_BLOCK_SIZE_16x16 | \
>>>>>> + AFBC_FORMAT_MOD_BLOCK_SIZE_32x8 | \
>>>>>> + AFBC_FORMAT_MOD_YTR | \
>>>>>> + AFBC_FORMAT_MOD_SPARSE | \
>>>>>> + AFBC_FORMAT_MOD_SPLIT)
>>>>>> +
>>>>>> /* Takes a fixed 16.16 number and converts it to integer. */
>>>>>> static inline int64_t fixed16_to_int(int64_t value)
>>>>>> {
>>>>>> return value >> 16;
>>>>>> }
>>>>>>
>>>>>> +static u32 meson_g12a_afbcd_line_stride(struct meson_drm *priv)
>>>>>> +{
>>>>>> + u32 line_stride = 0;
>>>>>> +
>>>>>> + switch (priv->afbcd.format) {
>>>>>> + case DRM_FORMAT_RGB565:
>>>>>> + line_stride = ((priv->viu.osd1_width << 4) + 127) >> 7;
>>>>>> + break;
>>>>>> + case DRM_FORMAT_RGB888:
>>>>>> + case DRM_FORMAT_XRGB8888:
>>>>>> + case DRM_FORMAT_ARGB8888:
>>>>>> + case DRM_FORMAT_XBGR8888:
>>>>>> + case DRM_FORMAT_ABGR8888:
>>>>> Please have a look at
>>>>> https://www.kernel.org/doc/html/latest/gpu/afbc.html for our
>>>>> recommendation. We suggest that *X* formats are avoided.
>>>>>
>>>>> Also, for interoperability and maximum compression efficiency (with
>>>>> AFBC_FORMAT_MOD_YTR), we suggest the following order :-
>>>>>
>>>>> Component 0: R
>>>>> Component 1: G
>>>>> Component 2: B
>>>>> Component 3: A (if available)
>>>>
>>>>
>>>> Sorry I don't understand, you ask me to limit AFBC to ABGR8888 ?
>>>>
>>>> But why if the HW (GPU and DPU) is capable of ?
>>>
>>> AFBC doesn't have an in-memory component order in the traditional
>>> sense (i.e. a bit-position to component mapping), so Arm
>>> have decided to define the convention that DRM_FORMAT_ABGR8888
>>> represents the AFBC layout with R in component 0.
>>
>> In this implementation, we handle the ARGB/ABGR as the same mode
>> since the AFBC can only represent the layout as "ABGR" anyway.
>>
>
> In this case, with the external AFBC IP, there's a whole extra layer
> of potential confusion :-(
>
> The decoder only needs to know the number of components - so
> irrespective of what color channel is mapped to what component, it can
> always be configured with the same mode for 4-component 32-bit
> formats.
>
> For everything to work correctly with YTR, the thing consuming the
> output from the decoder must treat component 0 as 'R', but otherwise
> it doesn't matter.
>
> Is your HW able to treat the decoder output in different ways? e.g.
> mapping component 0 to 'B'? If that's the case, then exposing the
> different orders is valid - but only ABGR should allow YTR.
Yes, we can remap each components from AFBC in any order.
Ok thanks for clarifying, so:
- I'll allow only ABGR/XBGR with YTR
- I'll allow ABGR/XBGR/ARGB/XRGB only if !YTR and use the AFBC components remapping
for ARGB/XRGB
I'll also need to clean up the RGB888/BGR888 as we support only RGB888 for now.
And I'll reject RGB565 since we don't support it without AFBC.
>
>>>
>>> Are you sure the GPU supports other orders? I think any Arm driver
>>> will only be producing DRM_FORMATs with "BGR" order e.g. ABGR888>
>>> I'm not convinced the GPU HW actually supports any other order, but
>>> it's all rather confusing with texture swizzling. What I can tell you
>>> for sure is that it _does_ support BGR order (in DRM naming
>>> convention).
>>
>> Well, since the Bifrost Mali blobs are closed-source and delivered
>> by licensees, it's hard to define what is supported from a closed
>> GPU HW, closed SW implementation to a closed pixel format implementation.
>>
>
> I hear you. IMO the only way to make any of this clear is to publish
> reference data and tests which make sure implementations match each
> other. It's something I'm trying to make happen.
I'll be happy to run them when available and fix the implementation accordingly !
>
>> You'll have to tell us if the closed libMali handling AFBC would accept
>> ARGB8888 as format to render with AFBC enabled, if not you're right
>> I'll discard XRGB8888/ARGB8888 for AFBC buffers completely.
>>
>> But it the libMali chooses tt generate an ARGB8888 buffer whatever
>> ARGB8888/XRGB8888/ABGR888/XBGR888 is asked, then no I'll keep it that way.
>>
>
> Yeah, I'll try and get clarity on this. It's not at all clear to me
> either. When you say "accept ARGB8888 as format to render with AFBC
> enabled", which API are you referring to, just so I can be clear? Do
> you have an example of some code you're using to render AFBC with the
> GPU blob?
Let's take kmscube using EGL and GBM.
The buffer is allocated using gbm_surface_create_with_modifiers(),
but the gbm implementation is vendor specified.
Then the surface is passed to eglCreateWindowSurface().
Then the format is matched using eglGetConfigAttrib() with the
returned configs.
Here support on the gbm and EGL implementation.
>
> In many APIs, there's no real expectation on in-memory component
> order, so perhaps there treating them as all the same is acceptable.
>
> However, fourcc + AFBC modifier is explicit in terms of component
> order, and so IMO it's very harmful to "ignore" component order in
> interfaces using fourcc + AFBC modifier.
>
> There are implementations which support other orders, so ignoring
> order will break those implementations. In some cases (Android, maybe
> GL), this can be hidden behind "driver magic", but if the API is
> fourcc + AFBC modifier, IMO it had better be completely explicit with
> no tricks - irrespective of whatever other less-prescriptive APIs do.
Sure
>
>> BTW I kept the vendor implementation here, which may be wrong but since
>> they have the AFBC IP license and Mali Bifrost GPU license...
>>
>>>
>>> If you do choose to expose orders other than BGR/ABGR, then you should
>>> certainly not allow YTR to be used with any orders other than
>>> BGR/ABGR. The AFBC spec defines YTR as using R in component 0, which
>>> Arm has defined as DRM_FORMAT_*BGR* (component 0 in LE LSBs).
>>>
>>
>> The MAFBC_FMT_RGBA8888 pixel format is defined in the AFBC decoder,
>> which seems to be an ARM IP, the registers documentation is in the
>> SoC datasheet at [1] and the formats bits are defined in the patch 3 at [2].
>>
>> So it seems the decoder handles only a single type for 32bit RGB buffer
>> format, as Amlogic names it MAFBC_FMT_RGBA8888
>>
>
> Hopefully my comments at the beginning of this mail helps clear this
> part up a bit.
>
>> For XRGB8888/XBGR8888 we simply "replace" the A component with a fixed
>> value in the pixel generator.
>
> That seems correct, so long as the decoder is configured in the
> 4-component mode.
>
>>
>> [1] https://dl.khadas.com/Hardware/VIM3/Datasheet/S905D3_datasheet_0.2_Wesion.pdf page 772
>> [2] https://patchwork.freedesktop.org/patch/335199/?series=67832&rev=1
>>
>>>>
>>>> Isn't it an userspace choice ? I understand XRGB8888 is a waste
>>>> of memory space and compression efficiency, but this is not the
>>>> kernel driver's to decide this, right ?
>>>>
>>>
>>> As long as it's agreed and understood what XRGB8888 means. It must be
>>> an AFBC bitstream with 4-components, with B in component 0, G in
>>> component 1, R in component 2 and 8 wasted bits in component 3.
>>
>> Yes, but this is something userspace must assume, and it's already
>> wasted in the linear XRGB8888 format anyway.
>>
>>>
>>> I know of HW which treats "XBGR" with AFBC as a 3-component format,
>>> which isn't correct but can easily lead to confusion and
>>> incompatibility.
>>
>> Seems it's not the case here, at least for the G12A SoC family.
>
> That's good :-)
>
>>
>>>
>>>> For interoperability I'll understand recommending a minimal set
>>>> of modifiers and formats. But here, each platform is also limited
>>>> by it's GPU capabilites aswell.
>>>>
>>>
>>> The (Arm) GPUs support ABGR ordering, so if everyone sticks to that we
>>> can make sure everything's nice and compatible (until someone turns up
>>> with HW which _doesn't_ support that ordering).
>>
>> This is not clean enough in the https://www.kernel.org/doc/html/latest/gpu/afbc.html
>> document. Since ARM is in control of the renderers, saying AFBC does _not_
>> support another components format as ABGR ordering in all the
>> OpenGL ES/Vulkan implementations, it would be clear we couldn't render
>> anything using AFBC with ARGB.
>> But we hit the closed-source/closed-specifications here again.
>>
>
> I didn't really understand the middle sentence.
>
> I know and understand that the "closed-ness" is a problem. The page
> you linked was an initial attempt at making a clear, public
> specification.
>
> What I need to be clear about, though, is that it describes _only_
> cases where DRM fourcc + AFBC modifier are used. I don't think there's
> any sane way to apply it to other APIs, because the formats are
> described differently, and the "leeway" allowed for doing things
> "under-the-hood" is very different.
Indeed
>
>>>
>>>> Limiting to ABGR8888 would discard like every non-Android renderers,
>>>> using AFBC, I'm not sure it's the kernels driver's responsibility.
>>>>
>>>
>>> It prevents renderers with hard-coded pixel formats, perhaps. But
>>> those are already fragile by nature, surely?
>>
>> Well, except Android, all the other renderers uses ARGB8888/XRGB8888,
>> as fixed pixel format, which is quite a large amount of code.
>>
>
> I think whether that matters or not really depends on which graphics
> APIs you're referring to. IMO it's inevitable that modifiers don't
> simply "drop in" everywhere. The kernel API allows you to query what's
> supported and pick that.
Sure, we'll need to add an extra layer to discover the GL API capabilities
vs the DRM Display driver capabilities in term of fourcc+modifiers at some point.
This may be an goal for the liboutput library !
Thanks,
Neil
>
> Thanks,
> -Brian
>
>>
>> Anyway, thanks for these technical clarifications, it makes things
>> much more clearer.
>>
>> Neil
>>
>>>
>>> Cheers,
>>> -Brian
>>>
>>>>>
>>>>> Thus, DRM_FORMAT_ABGR, DRM_FORMAT_BGR should only be allowed.
>>>>>> + line_stride = ((priv->viu.osd1_width << 5) + 127) >> 7;
>>>>>> + break;
>>>>>> + }
>>>>>> +
>>>>>> + return ((line_stride + 1) >> 1) << 1;
>>>>>> +}
>>>>>> +
>>>>>> static void meson_plane_atomic_update(struct drm_plane *plane,
>>>>>> struct drm_plane_state *old_state)
>>>>>> {
>>>>
>>>> [...]
>>>>
>>>>>>
>>>>>> +static bool meson_plane_format_mod_supported(struct drm_plane *plane,
>>>>>> + u32 format, u64 modifier)
>>>>>> +{
>>>>>> + struct meson_plane *meson_plane = to_meson_plane(plane);
>>>>>> + struct meson_drm *priv = meson_plane->priv;
>>>>>> + int i;
>>>>>> +
>>>>>> + if (modifier == DRM_FORMAT_MOD_INVALID)
>>>>>> + return false;
>>>>>> +
>>>>>> + if (modifier == DRM_FORMAT_MOD_LINEAR)
>>>>>> + return true;
>>>>>> +
>>>>>> + if (!meson_vpu_is_compatible(priv, VPU_COMPATIBLE_GXM) &&
>>>>>> + !meson_vpu_is_compatible(priv, VPU_COMPATIBLE_G12A))
>>>>>> + return false;
>>>>>> +
>>>>>> + if (modifier & ~DRM_FORMAT_MOD_ARM_AFBC(MESON_MOD_AFBC_VALID_BITS))
>>>>>> + return false;
>>>>>> +
>>>>>> + for (i = 0 ; i < plane->modifier_count ; ++i)
>>>>>> + if (plane->modifiers[i] == modifier)
>>>>>> + break;
>>>>>> +
>>>>>> + if (i == plane->modifier_count) {
>>>>>> + DRM_DEBUG_KMS("Unsupported modifier\n");
>>>>>> + return false;
>>>>>> + }
>>>>
>>>> I can add a warn_once here, would it be enough ?
>>>>
>>>>>> +
>>>>>> + if (priv->afbcd.ops && priv->afbcd.ops->supported_fmt)
>>>>>> + return priv->afbcd.ops->supported_fmt(modifier, format);
>>>>>> +
>>>>>> + DRM_DEBUG_KMS("AFBC Unsupported\n");
>>>>>> + return false;
>>>>>> +}
>>>>>> +
>>>>>> static const struct drm_plane_funcs meson_plane_funcs = {
>>>>>> .update_plane = drm_atomic_helper_update_plane,
>>>>>> .disable_plane = drm_atomic_helper_disable_plane,
>>>>>> @@ -353,6 +457,7 @@ static const struct drm_plane_funcs meson_plane_funcs = {
>>>>>> .reset = drm_atomic_helper_plane_reset,
>>>>>> .atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
>>>>>> .atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
>>>>>> + .format_mod_supported = meson_plane_format_mod_supported,
>>>>>> };
>>>>>>
>>>>>> static const uint32_t supported_drm_formats[] = {
>>>>>> @@ -364,10 +469,53 @@ static const uint32_t supported_drm_formats[] = {
>>>>>> DRM_FORMAT_RGB565,
>>>>>> };
>>>>>>
>>>>>> +static const uint64_t format_modifiers_afbc_gxm[] = {
>>>>>> + DRM_FORMAT_MOD_ARM_AFBC(AFBC_FORMAT_MOD_BLOCK_SIZE_16x16 |
>>>>>> + AFBC_FORMAT_MOD_SPARSE |
>>>>>> + AFBC_FORMAT_MOD_YTR),
>>>>>> + /* SPLIT mandates SPARSE, RGB modes mandates YTR */
>>>>>> + DRM_FORMAT_MOD_ARM_AFBC(AFBC_FORMAT_MOD_BLOCK_SIZE_16x16 |
>>>>>> + AFBC_FORMAT_MOD_YTR |
>>>>>> + AFBC_FORMAT_MOD_SPARSE |
>>>>>> + AFBC_FORMAT_MOD_SPLIT),
>>>>>> + DRM_FORMAT_MOD_LINEAR,
>>>>>> + DRM_FORMAT_MOD_INVALID,
>>>>>> +};
>>>>>> +
>>>>>> +static const uint64_t format_modifiers_afbc_g12a[] = {
>>>>>> + /*
>>>>>> + * - TOFIX Support AFBC modifiers for YUV formats (16x16 + TILED)
>>>>>> + * - AFBC_FORMAT_MOD_YTR is mandatory since we only support RGB
>>>>>> + * - SPLIT is mandatory for performances reasons when in 16x16
>>>>>> + * block size
>>>>>> + * - 32x8 block size + SPLIT is mandatory with 4K frame size
>>>>>> + * for performances reasons
>>>>>> + */
>>>>>> + DRM_FORMAT_MOD_ARM_AFBC(AFBC_FORMAT_MOD_BLOCK_SIZE_16x16 |
>>>>>> + AFBC_FORMAT_MOD_YTR |
>>>>>> + AFBC_FORMAT_MOD_SPARSE |
>>>>>> + AFBC_FORMAT_MOD_SPLIT),
>>>>>> + DRM_FORMAT_MOD_ARM_AFBC(AFBC_FORMAT_MOD_BLOCK_SIZE_32x8 |
>>>>>> + AFBC_FORMAT_MOD_YTR |
>>>>>> + AFBC_FORMAT_MOD_SPARSE),
>>>>>> + DRM_FORMAT_MOD_ARM_AFBC(AFBC_FORMAT_MOD_BLOCK_SIZE_32x8 |
>>>>>> + AFBC_FORMAT_MOD_YTR |
>>>>>> + AFBC_FORMAT_MOD_SPARSE |
>>>>>> + AFBC_FORMAT_MOD_SPLIT),
>>>>>> + DRM_FORMAT_MOD_LINEAR,
>>>>>> + DRM_FORMAT_MOD_INVALID,
>>>>>> +};
>>>>>> +
>>>>>> +static const uint64_t format_modifiers_default[] = {
>>>>>> + DRM_FORMAT_MOD_LINEAR,
>>>>>> + DRM_FORMAT_MOD_INVALID,
>>>>>> +};
>>>>>> +
>>>>>> int meson_plane_create(struct meson_drm *priv)
>>>>>> {
>>>>>> struct meson_plane *meson_plane;
>>>>>> struct drm_plane *plane;
>>>>>> + const uint64_t *format_modifiers = format_modifiers_default;
>>>>>>
>>>>>> meson_plane = devm_kzalloc(priv->drm->dev, sizeof(*meson_plane),
>>>>>> GFP_KERNEL);
>>>>>> @@ -377,11 +525,16 @@ int meson_plane_create(struct meson_drm *priv)
>>>>>> meson_plane->priv = priv;
>>>>>> plane = &meson_plane->base;
>>>>>>
>>>>>> + if (meson_vpu_is_compatible(priv, VPU_COMPATIBLE_GXM))
>>>>>> + format_modifiers = format_modifiers_afbc_gxm;
>>>>>> + else if (meson_vpu_is_compatible(priv, VPU_COMPATIBLE_G12A))
>>>>>> + format_modifiers = format_modifiers_afbc_g12a;
>>>>>> +
>>>>>> drm_universal_plane_init(priv->drm, plane, 0xFF,
>>>>>> &meson_plane_funcs,
>>>>>> supported_drm_formats,
>>>>>> ARRAY_SIZE(supported_drm_formats),
>>>>>> - NULL,
>>>>>> + format_modifiers,
>>>>>> DRM_PLANE_TYPE_PRIMARY, "meson_primary_plane");
>>>>>>
>>>>>> drm_plane_helper_add(plane, &meson_plane_helper_funcs);
>>>>>> --
>>>>>> 2.22.0
>>>>
>>>> _______________________________________________
>>>> dri-devel mailing list
>>>> dri-devel@lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 4/7] drm/meson: plane: add support for AFBC mode for OSD1 plane
2019-10-11 12:07 ` Neil Armstrong
@ 2019-10-15 11:18 ` Brian Starkey
2019-10-15 11:46 ` Neil Armstrong
0 siblings, 1 reply; 22+ messages in thread
From: Brian Starkey @ 2019-10-15 11:18 UTC (permalink / raw)
To: Neil Armstrong
Cc: Ayan Halder, khilman, dri-devel, linux-amlogic, nd, linux-arm-kernel
Hi Neil,
On Fri, Oct 11, 2019 at 02:07:01PM +0200, Neil Armstrong wrote:
> On 11/10/2019 12:56, Brian Starkey wrote:
> > Hi,
> >
> > On Fri, Oct 11, 2019 at 11:14:43AM +0200, Neil Armstrong wrote:
> >> Hi Brian,
> >>
> >> On 11/10/2019 10:41, Brian Starkey wrote:
> >>> Hi Neil,
> >>>
> >>> On Thu, Oct 10, 2019 at 03:41:15PM +0200, Neil Armstrong wrote:
[snip]
> >> You'll have to tell us if the closed libMali handling AFBC would accept
> >> ARGB8888 as format to render with AFBC enabled, if not you're right
> >> I'll discard XRGB8888/ARGB8888 for AFBC buffers completely.
> >>
> >> But it the libMali chooses tt generate an ARGB8888 buffer whatever
> >> ARGB8888/XRGB8888/ABGR888/XBGR888 is asked, then no I'll keep it that way.
> >>
> >
> > Yeah, I'll try and get clarity on this. It's not at all clear to me
> > either. When you say "accept ARGB8888 as format to render with AFBC
> > enabled", which API are you referring to, just so I can be clear? Do
> > you have an example of some code you're using to render AFBC with the
> > GPU blob?
>
> Let's take kmscube using EGL and GBM.
>
> The buffer is allocated using gbm_surface_create_with_modifiers(),
> but the gbm implementation is vendor specified.
>
> Then the surface is passed to eglCreateWindowSurface().
> Then the format is matched using eglGetConfigAttrib() with the
> returned configs.
>
> Here support on the gbm and EGL implementation.
>
Is this a use-case that works on your platform today?
I went and asked around. An Arm gbm implementation supporting AFBC
will reject AFBC allocations for orders other than (DRM-convention)
BGR.
Thanks,
-Brian
_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 4/7] drm/meson: plane: add support for AFBC mode for OSD1 plane
2019-10-15 11:18 ` Brian Starkey
@ 2019-10-15 11:46 ` Neil Armstrong
0 siblings, 0 replies; 22+ messages in thread
From: Neil Armstrong @ 2019-10-15 11:46 UTC (permalink / raw)
To: Brian Starkey
Cc: Ayan Halder, khilman, dri-devel, linux-amlogic, nd, linux-arm-kernel
On 15/10/2019 13:18, Brian Starkey wrote:
> Hi Neil,
>
> On Fri, Oct 11, 2019 at 02:07:01PM +0200, Neil Armstrong wrote:
>> On 11/10/2019 12:56, Brian Starkey wrote:
>>> Hi,
>>>
>>> On Fri, Oct 11, 2019 at 11:14:43AM +0200, Neil Armstrong wrote:
>>>> Hi Brian,
>>>>
>>>> On 11/10/2019 10:41, Brian Starkey wrote:
>>>>> Hi Neil,
>>>>>
>>>>> On Thu, Oct 10, 2019 at 03:41:15PM +0200, Neil Armstrong wrote:
>
> [snip]
>
>>>> You'll have to tell us if the closed libMali handling AFBC would accept
>>>> ARGB8888 as format to render with AFBC enabled, if not you're right
>>>> I'll discard XRGB8888/ARGB8888 for AFBC buffers completely.
>>>>
>>>> But it the libMali chooses tt generate an ARGB8888 buffer whatever
>>>> ARGB8888/XRGB8888/ABGR888/XBGR888 is asked, then no I'll keep it that way.
>>>>
>>>
>>> Yeah, I'll try and get clarity on this. It's not at all clear to me
>>> either. When you say "accept ARGB8888 as format to render with AFBC
>>> enabled", which API are you referring to, just so I can be clear? Do
>>> you have an example of some code you're using to render AFBC with the
>>> GPU blob?
>>
>> Let's take kmscube using EGL and GBM.
>>
>> The buffer is allocated using gbm_surface_create_with_modifiers(),
>> but the gbm implementation is vendor specified.
>>
>> Then the surface is passed to eglCreateWindowSurface().
>> Then the format is matched using eglGetConfigAttrib() with the
>> returned configs.
>>
>> Here support on the gbm and EGL implementation.
>>
>
> Is this a use-case that works on your platform today?
Amlogic gave ma a libMali for miniGBM with AFBC enabled, but I haven't
been able to enable AFBC yet.
>
> I went and asked around. An Arm gbm implementation supporting AFBC
> will reject AFBC allocations for orders other than (DRM-convention)
> BGR.
I trust you on this point, thanks for asking around.
Neil
>
> Thanks,
> -Brian
>
_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 4/7] drm/meson: plane: add support for AFBC mode for OSD1 plane
2019-10-11 10:56 ` Brian Starkey
2019-10-11 12:07 ` Neil Armstrong
@ 2019-10-11 17:25 ` Daniel Vetter
2019-10-14 9:11 ` Brian Starkey
1 sibling, 1 reply; 22+ messages in thread
From: Daniel Vetter @ 2019-10-11 17:25 UTC (permalink / raw)
To: Brian Starkey
Cc: nd, Neil Armstrong, khilman, dri-devel, linux-amlogic,
Ayan Halder, linux-arm-kernel
On Fri, Oct 11, 2019 at 12:56 PM Brian Starkey <Brian.Starkey@arm.com> wrote:
>
> Hi,
>
> On Fri, Oct 11, 2019 at 11:14:43AM +0200, Neil Armstrong wrote:
> > Hi Brian,
> >
> > On 11/10/2019 10:41, Brian Starkey wrote:
> > > Are you sure the GPU supports other orders? I think any Arm driver
> > > will only be producing DRM_FORMATs with "BGR" order e.g. ABGR888>
> > > I'm not convinced the GPU HW actually supports any other order, but
> > > it's all rather confusing with texture swizzling. What I can tell you
> > > for sure is that it _does_ support BGR order (in DRM naming
> > > convention).
> >
> > Well, since the Bifrost Mali blobs are closed-source and delivered
> > by licensees, it's hard to define what is supported from a closed
> > GPU HW, closed SW implementation to a closed pixel format implementation.
> >
>
> I hear you. IMO the only way to make any of this clear is to publish
> reference data and tests which make sure implementations match each
> other. It's something I'm trying to make happen.
*cough* igt test with crc/writeback validation *cough*
And you don't even need anything that actually compresses. All you
need is the minimal enough AFBC knowledge to set up the metadata that
everything is uncompressed. We're doing that for our intel compression
formats already, and it works great in making sure that we have
end-to-end agreement on all the bits and ordering and everything. Ofc
it doesn't validate the decoder, but that's not really igts job.
Should be possible to convince ARM to release that (as open source, in
igt), since it would be fairly valuable for the entire ecosystem here
...
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 4/7] drm/meson: plane: add support for AFBC mode for OSD1 plane
2019-10-11 17:25 ` Daniel Vetter
@ 2019-10-14 9:11 ` Brian Starkey
2019-10-14 9:20 ` Daniel Vetter
0 siblings, 1 reply; 22+ messages in thread
From: Brian Starkey @ 2019-10-14 9:11 UTC (permalink / raw)
To: Daniel Vetter
Cc: nd, Neil Armstrong, khilman, dri-devel, linux-amlogic,
Ayan Halder, linux-arm-kernel
On Fri, Oct 11, 2019 at 07:25:02PM +0200, Daniel Vetter wrote:
> On Fri, Oct 11, 2019 at 12:56 PM Brian Starkey <Brian.Starkey@arm.com> wrote:
> >
> > Hi,
> >
> > On Fri, Oct 11, 2019 at 11:14:43AM +0200, Neil Armstrong wrote:
> > > Hi Brian,
> > >
> > > On 11/10/2019 10:41, Brian Starkey wrote:
>
> > > > Are you sure the GPU supports other orders? I think any Arm driver
> > > > will only be producing DRM_FORMATs with "BGR" order e.g. ABGR888>
> > > > I'm not convinced the GPU HW actually supports any other order, but
> > > > it's all rather confusing with texture swizzling. What I can tell you
> > > > for sure is that it _does_ support BGR order (in DRM naming
> > > > convention).
> > >
> > > Well, since the Bifrost Mali blobs are closed-source and delivered
> > > by licensees, it's hard to define what is supported from a closed
> > > GPU HW, closed SW implementation to a closed pixel format implementation.
> > >
> >
> > I hear you. IMO the only way to make any of this clear is to publish
> > reference data and tests which make sure implementations match each
> > other. It's something I'm trying to make happen.
>
> *cough* igt test with crc/writeback validation *cough*
>
> And you don't even need anything that actually compresses. All you
> need is the minimal enough AFBC knowledge to set up the metadata that
> everything is uncompressed. We're doing that for our intel compression
> formats already, and it works great in making sure that we have
> end-to-end agreement on all the bits and ordering and everything.
Yeah this was my original thinking too. Sadly, a decent number of the
AFBC parameters can't be tested with uncompressed data, as the codec
kicks into a different mode there.
> Ofc
> it doesn't validate the decoder, but that's not really igts job.
> Should be possible to convince ARM to release that (as open source, in
> igt), since it would be fairly valuable for the entire ecosystem here
> ...
I fully agree about the value proposition.
-Brian
> -Daniel
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 4/7] drm/meson: plane: add support for AFBC mode for OSD1 plane
2019-10-14 9:11 ` Brian Starkey
@ 2019-10-14 9:20 ` Daniel Vetter
0 siblings, 0 replies; 22+ messages in thread
From: Daniel Vetter @ 2019-10-14 9:20 UTC (permalink / raw)
To: Brian Starkey
Cc: nd, Neil Armstrong, khilman, dri-devel, Daniel Vetter,
linux-amlogic, Ayan Halder, linux-arm-kernel
On Mon, Oct 14, 2019 at 09:11:17AM +0000, Brian Starkey wrote:
> On Fri, Oct 11, 2019 at 07:25:02PM +0200, Daniel Vetter wrote:
> > On Fri, Oct 11, 2019 at 12:56 PM Brian Starkey <Brian.Starkey@arm.com> wrote:
> > >
> > > Hi,
> > >
> > > On Fri, Oct 11, 2019 at 11:14:43AM +0200, Neil Armstrong wrote:
> > > > Hi Brian,
> > > >
> > > > On 11/10/2019 10:41, Brian Starkey wrote:
> >
> > > > > Are you sure the GPU supports other orders? I think any Arm driver
> > > > > will only be producing DRM_FORMATs with "BGR" order e.g. ABGR888>
> > > > > I'm not convinced the GPU HW actually supports any other order, but
> > > > > it's all rather confusing with texture swizzling. What I can tell you
> > > > > for sure is that it _does_ support BGR order (in DRM naming
> > > > > convention).
> > > >
> > > > Well, since the Bifrost Mali blobs are closed-source and delivered
> > > > by licensees, it's hard to define what is supported from a closed
> > > > GPU HW, closed SW implementation to a closed pixel format implementation.
> > > >
> > >
> > > I hear you. IMO the only way to make any of this clear is to publish
> > > reference data and tests which make sure implementations match each
> > > other. It's something I'm trying to make happen.
> >
> > *cough* igt test with crc/writeback validation *cough*
> >
> > And you don't even need anything that actually compresses. All you
> > need is the minimal enough AFBC knowledge to set up the metadata that
> > everything is uncompressed. We're doing that for our intel compression
> > formats already, and it works great in making sure that we have
> > end-to-end agreement on all the bits and ordering and everything.
>
> Yeah this was my original thinking too. Sadly, a decent number of the
> AFBC parameters can't be tested with uncompressed data, as the codec
> kicks into a different mode there.
Hm right I guess some of the flags/parameters only matter if you deal with
actual compressed data. Still, better than nothing I guess - this should
at least catch stuff like color channels wired up the wrong way compared
to linear, and fun stuff like that.
> > Ofc
> > it doesn't validate the decoder, but that's not really igts job.
> > Should be possible to convince ARM to release that (as open source, in
> > igt), since it would be fairly valuable for the entire ecosystem here
> > ...
>
> I fully agree about the value proposition.
I'll be waiting for patch from arm then :-)
Cheers, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 5/7] drm/meson: viu: add AFBC modules routing functions
2019-10-10 9:25 [PATCH 0/7] drm/meson: add AFBC support Neil Armstrong
` (3 preceding siblings ...)
2019-10-10 9:25 ` [PATCH 4/7] drm/meson: plane: add support for AFBC mode for OSD1 plane Neil Armstrong
@ 2019-10-10 9:25 ` Neil Armstrong
2019-10-10 9:25 ` [PATCH 6/7] drm/meson: hold 32 lines after vsync to give time for AFBC start Neil Armstrong
2019-10-10 9:25 ` [PATCH 7/7] drm/meson: crtc: add OSD1 plane AFBC commit Neil Armstrong
6 siblings, 0 replies; 22+ messages in thread
From: Neil Armstrong @ 2019-10-10 9:25 UTC (permalink / raw)
To: dri-devel
Cc: khilman, linux-amlogic, ayan.halder, linux-arm-kernel, Neil Armstrong
The Amlogic G12A AFBC Decoder pixel input need to be routed diferently
than the Amlogic GXM AFBC decoder, this adds support for routing the
VIU OSD1 pixel source to the AFBC "Mali Unpack" module.
This "Mali Unpack" module is also configured with a static RGBA mapping
for now until we support more pixel formats.
Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
drivers/gpu/drm/meson/meson_viu.c | 52 +++++++++++++++++++++++++++++++
drivers/gpu/drm/meson/meson_viu.h | 4 +++
2 files changed, 56 insertions(+)
diff --git a/drivers/gpu/drm/meson/meson_viu.c b/drivers/gpu/drm/meson/meson_viu.c
index 68cf2c2eca5f..cdd4962484db 100644
--- a/drivers/gpu/drm/meson/meson_viu.c
+++ b/drivers/gpu/drm/meson/meson_viu.c
@@ -7,6 +7,7 @@
*/
#include <linux/export.h>
+#include <linux/bitfield.h>
#include "meson_drv.h"
#include "meson_viu.h"
@@ -335,6 +336,57 @@ void meson_viu_osd1_reset(struct meson_drm *priv)
meson_viu_load_matrix(priv);
}
+void meson_viu_g12a_enable_osd1_afbc(struct meson_drm *priv)
+{
+ /* Enable Mali AFBC Unpack */
+ writel_bits_relaxed(VIU_OSD1_MALI_UNPACK_EN,
+ VIU_OSD1_MALI_UNPACK_EN,
+ priv->io_base + _REG(VIU_OSD1_MALI_UNPACK_CTRL));
+
+ /* Setup RGBA Reordering */
+ writel_bits_relaxed(VIU_OSD1_MALI_AFBCD_A_REORDER |
+ VIU_OSD1_MALI_AFBCD_B_REORDER |
+ VIU_OSD1_MALI_AFBCD_G_REORDER |
+ VIU_OSD1_MALI_AFBCD_R_REORDER,
+ FIELD_PREP(VIU_OSD1_MALI_AFBCD_A_REORDER,
+ VIU_OSD1_MALI_REORDER_A) |
+ FIELD_PREP(VIU_OSD1_MALI_AFBCD_B_REORDER,
+ VIU_OSD1_MALI_REORDER_B) |
+ FIELD_PREP(VIU_OSD1_MALI_AFBCD_G_REORDER,
+ VIU_OSD1_MALI_REORDER_G) |
+ FIELD_PREP(VIU_OSD1_MALI_AFBCD_R_REORDER,
+ VIU_OSD1_MALI_REORDER_R),
+ priv->io_base + _REG(VIU_OSD1_MALI_UNPACK_CTRL));
+
+ /* Select AFBCD path for OSD1 */
+ writel_bits_relaxed(OSD_PATH_OSD_AXI_SEL_OSD1_AFBCD,
+ OSD_PATH_OSD_AXI_SEL_OSD1_AFBCD,
+ priv->io_base + _REG(OSD_PATH_MISC_CTRL));
+}
+
+void meson_viu_g12a_disable_osd1_afbc(struct meson_drm *priv)
+{
+ /* Disable AFBCD path for OSD1 */
+ writel_bits_relaxed(OSD_PATH_OSD_AXI_SEL_OSD1_AFBCD, 0,
+ priv->io_base + _REG(OSD_PATH_MISC_CTRL));
+
+ /* Disable AFBCD unpack */
+ writel_bits_relaxed(VIU_OSD1_MALI_UNPACK_EN, 0,
+ priv->io_base + _REG(VIU_OSD1_MALI_UNPACK_CTRL));
+}
+
+void meson_viu_gxm_enable_osd1_afbc(struct meson_drm *priv)
+{
+ writel_bits_relaxed(MALI_AFBC_MISC, FIELD_PREP(MALI_AFBC_MISC, 0x90),
+ priv->io_base + _REG(VIU_MISC_CTRL1));
+}
+
+void meson_viu_gxm_disable_osd1_afbc(struct meson_drm *priv)
+{
+ writel_bits_relaxed(MALI_AFBC_MISC, FIELD_PREP(MALI_AFBC_MISC, 0x00),
+ priv->io_base + _REG(VIU_MISC_CTRL1));
+}
+
static inline uint32_t meson_viu_osd_burst_length_reg(uint32_t length)
{
uint32_t val = (((length & 0x80) % 24) / 12);
diff --git a/drivers/gpu/drm/meson/meson_viu.h b/drivers/gpu/drm/meson/meson_viu.h
index e297772d967f..e4a2f24d7c38 100644
--- a/drivers/gpu/drm/meson/meson_viu.h
+++ b/drivers/gpu/drm/meson/meson_viu.h
@@ -63,6 +63,10 @@
#define OSD_PENDING_STAT_CLEAN BIT(1)
void meson_viu_osd1_reset(struct meson_drm *priv);
+void meson_viu_g12a_enable_osd1_afbc(struct meson_drm *priv);
+void meson_viu_g12a_disable_osd1_afbc(struct meson_drm *priv);
+void meson_viu_gxm_enable_osd1_afbc(struct meson_drm *priv);
+void meson_viu_gxm_disable_osd1_afbc(struct meson_drm *priv);
void meson_viu_init(struct meson_drm *priv);
#endif /* __MESON_VIU_H */
--
2.22.0
_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH 6/7] drm/meson: hold 32 lines after vsync to give time for AFBC start
2019-10-10 9:25 [PATCH 0/7] drm/meson: add AFBC support Neil Armstrong
` (4 preceding siblings ...)
2019-10-10 9:25 ` [PATCH 5/7] drm/meson: viu: add AFBC modules routing functions Neil Armstrong
@ 2019-10-10 9:25 ` Neil Armstrong
2019-10-10 9:25 ` [PATCH 7/7] drm/meson: crtc: add OSD1 plane AFBC commit Neil Armstrong
6 siblings, 0 replies; 22+ messages in thread
From: Neil Armstrong @ 2019-10-10 9:25 UTC (permalink / raw)
To: dri-devel
Cc: khilman, linux-amlogic, ayan.halder, linux-arm-kernel, Neil Armstrong
When using an AFBC encoded frame, the AFBC Decoder must be resetted,
configured and enabled at each vsync IRQ.
To leave time for that, use the maximum lines hold time to give time
for AFBC setup and avoid visual glitches.
Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
drivers/gpu/drm/meson/meson_viu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/meson/meson_viu.c b/drivers/gpu/drm/meson/meson_viu.c
index cdd4962484db..be535e23fc99 100644
--- a/drivers/gpu/drm/meson/meson_viu.c
+++ b/drivers/gpu/drm/meson/meson_viu.c
@@ -414,7 +414,7 @@ void meson_viu_init(struct meson_drm *priv)
/* Initialize OSD1 fifo control register */
reg = VIU_OSD_DDR_PRIORITY_URGENT |
- VIU_OSD_HOLD_FIFO_LINES(4) |
+ VIU_OSD_HOLD_FIFO_LINES(31) |
VIU_OSD_FIFO_DEPTH_VAL(32) | /* fifo_depth_val: 32*8=256 */
VIU_OSD_WORDS_PER_BURST(4) | /* 4 words in 1 burst */
VIU_OSD_FIFO_LIMITS(2); /* fifo_lim: 2*16=32 */
--
2.22.0
_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH 7/7] drm/meson: crtc: add OSD1 plane AFBC commit
2019-10-10 9:25 [PATCH 0/7] drm/meson: add AFBC support Neil Armstrong
` (5 preceding siblings ...)
2019-10-10 9:25 ` [PATCH 6/7] drm/meson: hold 32 lines after vsync to give time for AFBC start Neil Armstrong
@ 2019-10-10 9:25 ` Neil Armstrong
6 siblings, 0 replies; 22+ messages in thread
From: Neil Armstrong @ 2019-10-10 9:25 UTC (permalink / raw)
To: dri-devel
Cc: khilman, linux-amlogic, ayan.halder, linux-arm-kernel, Neil Armstrong
Finally, setup the VIU registers and start the AFBC decoder at each
vsync IRQ to support displaying AFBC encoded buffers on Amlogic GXM
and G12A SoCs.
Since the DRM core will stop the vsync IRQ after a few interrupts,
we need to keep the IRQ enabled while using the AFBC decoder.
Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
drivers/gpu/drm/meson/meson_crtc.c | 79 ++++++++++++++++++++++++++++--
1 file changed, 74 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/meson/meson_crtc.c b/drivers/gpu/drm/meson/meson_crtc.c
index d478fa232951..d28efd0dbf11 100644
--- a/drivers/gpu/drm/meson/meson_crtc.c
+++ b/drivers/gpu/drm/meson/meson_crtc.c
@@ -24,6 +24,7 @@
#include "meson_venc.h"
#include "meson_viu.h"
#include "meson_vpp.h"
+#include "meson_osd_afbcd.h"
#define MESON_G12A_VIU_OFFSET 0x5ec0
@@ -35,7 +36,11 @@ struct meson_crtc {
struct meson_drm *priv;
void (*enable_osd1)(struct meson_drm *priv);
void (*enable_vd1)(struct meson_drm *priv);
+ void (*enable_osd1_afbc)(struct meson_drm *priv);
+ void (*disable_osd1_afbc)(struct meson_drm *priv);
unsigned int viu_offset;
+ bool vsync_forced;
+ bool vsync_disabled;
};
#define to_meson_crtc(x) container_of(x, struct meson_crtc, base)
@@ -46,6 +51,7 @@ static int meson_crtc_enable_vblank(struct drm_crtc *crtc)
struct meson_crtc *meson_crtc = to_meson_crtc(crtc);
struct meson_drm *priv = meson_crtc->priv;
+ meson_crtc->vsync_disabled = false;
meson_venc_enable_vsync(priv);
return 0;
@@ -56,7 +62,10 @@ static void meson_crtc_disable_vblank(struct drm_crtc *crtc)
struct meson_crtc *meson_crtc = to_meson_crtc(crtc);
struct meson_drm *priv = meson_crtc->priv;
- meson_venc_disable_vsync(priv);
+ if (!meson_crtc->vsync_forced) {
+ meson_crtc->vsync_disabled = true;
+ meson_venc_disable_vsync(priv);
+ }
}
static const struct drm_crtc_funcs meson_crtc_funcs = {
@@ -236,6 +245,26 @@ static void meson_crtc_enable_osd1(struct meson_drm *priv)
priv->io_base + _REG(VPP_MISC));
}
+static void meson_crtc_g12a_enable_osd1_afbc(struct meson_drm *priv)
+{
+ writel_relaxed(priv->viu.osd1_blk2_cfg4,
+ priv->io_base + _REG(VIU_OSD1_BLK2_CFG_W4));
+
+ writel_bits_relaxed(OSD_MEM_LINEAR_ADDR, OSD_MEM_LINEAR_ADDR,
+ priv->io_base + _REG(VIU_OSD1_CTRL_STAT));
+
+ writel_relaxed(priv->viu.osd1_blk1_cfg4,
+ priv->io_base + _REG(VIU_OSD1_BLK1_CFG_W4));
+
+ meson_viu_g12a_enable_osd1_afbc(priv);
+
+ writel_bits_relaxed(OSD_MEM_LINEAR_ADDR, OSD_MEM_LINEAR_ADDR,
+ priv->io_base + _REG(VIU_OSD1_CTRL_STAT));
+
+ writel_bits_relaxed(OSD_MALI_SRC_EN, OSD_MALI_SRC_EN,
+ priv->io_base + _REG(VIU_OSD1_BLK0_CFG_W0));
+}
+
static void meson_g12a_crtc_enable_osd1(struct meson_drm *priv)
{
writel_relaxed(priv->viu.osd_blend_din0_scope_h,
@@ -293,6 +322,19 @@ void meson_crtc_irq(struct meson_drm *priv)
priv->io_base + _REG(VIU_OSD1_BLK0_CFG_W3));
writel_relaxed(priv->viu.osd1_blk0_cfg[4],
priv->io_base + _REG(VIU_OSD1_BLK0_CFG_W4));
+
+ if (priv->viu.osd1_afbcd) {
+ if (meson_crtc->enable_osd1_afbc)
+ meson_crtc->enable_osd1_afbc(priv);
+ } else {
+ if (meson_crtc->disable_osd1_afbc)
+ meson_crtc->disable_osd1_afbc(priv);
+ if (priv->afbcd.ops) {
+ priv->afbcd.ops->reset(priv);
+ priv->afbcd.ops->disable(priv);
+ }
+ }
+
writel_relaxed(priv->viu.osd_sc_ctrl0,
priv->io_base + _REG(VPP_OSD_SC_CTRL0));
writel_relaxed(priv->viu.osd_sc_i_wh_m1,
@@ -314,15 +356,23 @@ void meson_crtc_irq(struct meson_drm *priv)
writel_relaxed(priv->viu.osd_sc_v_ctrl0,
priv->io_base + _REG(VPP_OSD_VSC_CTRL0));
- meson_canvas_config(priv->canvas, priv->canvas_id_osd1,
- priv->viu.osd1_addr, priv->viu.osd1_stride,
- priv->viu.osd1_height, MESON_CANVAS_WRAP_NONE,
- MESON_CANVAS_BLKMODE_LINEAR, 0);
+ if (!priv->viu.osd1_afbcd)
+ meson_canvas_config(priv->canvas, priv->canvas_id_osd1,
+ priv->viu.osd1_addr,
+ priv->viu.osd1_stride,
+ priv->viu.osd1_height,
+ MESON_CANVAS_WRAP_NONE,
+ MESON_CANVAS_BLKMODE_LINEAR, 0);
/* Enable OSD1 */
if (meson_crtc->enable_osd1)
meson_crtc->enable_osd1(priv);
+ if (priv->viu.osd1_afbcd)
+ meson_crtc->vsync_forced = true;
+ else
+ meson_crtc->vsync_forced = false;
+
priv->viu.osd1_commit = false;
}
@@ -545,6 +595,15 @@ void meson_crtc_irq(struct meson_drm *priv)
priv->viu.vd1_commit = false;
}
+ if (meson_crtc->vsync_forced && priv->viu.osd1_afbcd) {
+ priv->afbcd.ops->reset(priv);
+ priv->afbcd.ops->setup(priv);
+ priv->afbcd.ops->enable(priv);
+ }
+
+ if (meson_crtc->vsync_disabled)
+ return;
+
drm_crtc_handle_vblank(priv->crtc);
spin_lock_irqsave(&priv->drm->event_lock, flags);
@@ -581,10 +640,20 @@ int meson_crtc_create(struct meson_drm *priv)
meson_crtc->enable_osd1 = meson_g12a_crtc_enable_osd1;
meson_crtc->enable_vd1 = meson_g12a_crtc_enable_vd1;
meson_crtc->viu_offset = MESON_G12A_VIU_OFFSET;
+ meson_crtc->enable_osd1_afbc =
+ meson_crtc_g12a_enable_osd1_afbc;
+ meson_crtc->disable_osd1_afbc =
+ meson_viu_g12a_disable_osd1_afbc;
drm_crtc_helper_add(crtc, &meson_g12a_crtc_helper_funcs);
} else {
meson_crtc->enable_osd1 = meson_crtc_enable_osd1;
meson_crtc->enable_vd1 = meson_crtc_enable_vd1;
+ if (meson_vpu_is_compatible(priv, VPU_COMPATIBLE_GXM)) {
+ meson_crtc->enable_osd1_afbc =
+ meson_viu_gxm_enable_osd1_afbc;
+ meson_crtc->disable_osd1_afbc =
+ meson_viu_gxm_disable_osd1_afbc;
+ }
drm_crtc_helper_add(crtc, &meson_crtc_helper_funcs);
}
--
2.22.0
_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic
^ permalink raw reply related [flat|nested] 22+ messages in thread