Re: [PATCH v3] drm/mediatek: Add AFBC support to Mediatek DRM driver
Hi Chun-Kuang,
> > + mtk_ovl_set_afbc(dev, cmdq_pkt, idx, is_afbc);
> > mtk_ddp_write_relaxed(cmdq_pkt, con, >cmdq_reg, ovl->regs,
> > DISP_REG_OVL_CON(idx));
> > - mtk_ddp_write_relaxed(cmdq_pkt, pitch, >cmdq_reg, ovl->regs,
> > + mtk_ddp_write_relaxed(cmdq_pkt, overlay_pitch.split_pitch.lsb,
> > >cmdq_reg, ovl->regs,
> > DISP_REG_OVL_PITCH(idx));
>
> Is this general for all MediaTek SoC? If so, separate this to an
> independent patch. Otherwise, use a device variable to separate this
> setting.
Yes and no. Technically all MediaTek SoCs have two separate registers
for the pitch, each are 16 bits, so this code is technically always
needed. However, because the lsb register is 16 bit, this issue has
never come up, because nobody has tried to display a plane that was
16384 ARGB pixels across. In fact, I think most MediaTek SoCs have a
resolution limit of 4K. The reason this issue comes up now is because
"pitch" is calculated differently for AFBC frames, and actually refers
to the size in bytes of one row of AFBC blocks. Should I still
separate this into an independent patch?
> > }
> > @@ -492,6 +567,15 @@ static const struct mtk_disp_ovl_data
> > mt8192_ovl_2l_driver_data = {
> > .smi_id_en = true,
> > };
> >
> > +static const struct mtk_disp_ovl_data mt8195_ovl_driver_data = {
>
> In this binding document, mt8195 ovl is compatible to mt8133 ovl.
> Please confirm that mt8195 is not identical with mt8133.
Do you mean MT8183? If so, we do not have any documentation indicating
that the MT8183 supports AFBC. Do you have some reason to believe
otherwise?
> Usually the pitch needs alignment. So I guess the formula is
>
> hdr_pitch = ALIGN(width_in_blocks * AFBC_HEADER_BLOCK_SIZE,
> AFBC_HEADER_ALIGNMENT);
> hdr_size = hdr_pitch * height_in_blocks;
The documentation does not indicate that the pitch needs alignment
beyond the AFBC header block size.
> Could you explain the meaning of hdr_pitch?
hdr_pitch refers to the size in bytes of one row of AFBC header
blocks. AFBC is a proprietary compressed frame buffer format, but from
what public information we have, it appears to be block compressed
data stored in 2 contiguous planes. The first is called the "header"
plane, and the second is called the "body" plane. The header plane
contains metadata for each block of pixel data, and the body plane
contains sparse compressed block data.
I'll send another patch with the other changes you mentioned.
Thanks!
Justin
Re: [PATCH v3] drm/mediatek: Add AFBC support to Mediatek DRM driver
* , ,
Hi, Justin:
Justin Green 於 2022年10月13日 週四 凌晨3:12寫道：
>
> Tested on MT8195 and confirmed both correct video output and improved DRAM
> bandwidth performance.
>
> v3:
> * Replaced pitch bitshift math with union based approach.
> * Refactored overlay register writes to shared code between non-AFBC and
> AFBC.
> * Minor code cleanups.
>
> v2:
> * Marked mtk_ovl_set_afbc as static.
> * Reflowed some lines to fit column limit.
>
> Signed-off-by: Justin Green
> ---
> drivers/gpu/drm/mediatek/mtk_disp_ovl.c | 90 +++-
> drivers/gpu/drm/mediatek/mtk_drm_plane.c | 37 +-
> drivers/gpu/drm/mediatek/mtk_drm_plane.h | 8 +++
> 3 files changed, 131 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> index 002b0f6cae1a..3f89b5f5064f 100644
> --- a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> +++ b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> @@ -29,17 +29,24 @@
> #define DISP_REG_OVL_DATAPATH_CON 0x0024
> #define OVL_LAYER_SMI_ID_ENBIT(0)
> #define OVL_BGCLR_SEL_IN BIT(2)
> +#define OVL_LAYER_AFBC_EN(n) BIT(4+n)
> #define DISP_REG_OVL_ROI_BGCLR 0x0028
> #define DISP_REG_OVL_SRC_CON 0x002c
> #define DISP_REG_OVL_CON(n)(0x0030 + 0x20 * (n))
> #define DISP_REG_OVL_SRC_SIZE(n) (0x0038 + 0x20 * (n))
> #define DISP_REG_OVL_OFFSET(n) (0x003c + 0x20 * (n))
> +#define DISP_REG_OVL_PITCH_MSB(n) (0x0040 + 0x20 * (n))
> +#define OVL_PITCH_MSB_2ND_SUBBUF BIT(16)
> +#define OVL_PITCH_MSB_YUV_TRANSBIT(20)
Useless, so drop it.
> #define DISP_REG_OVL_PITCH(n) (0x0044 + 0x20 * (n))
> +#define DISP_REG_OVL_CLIP(n) (0x004c + 0x20 * (n))
Useless, so drop it.
> #define DISP_REG_OVL_RDMA_CTRL(n) (0x00c0 + 0x20 * (n))
> #define DISP_REG_OVL_RDMA_GMC(n) (0x00c8 + 0x20 * (n))
> #define DISP_REG_OVL_ADDR_MT2701 0x0040
> #define DISP_REG_OVL_ADDR_MT8173 0x0f40
> #define DISP_REG_OVL_ADDR(ovl, n) ((ovl)->data->addr + 0x20 *
> (n))
> +#define DISP_REG_OVL_HDR_ADDR(ovl, n) ((ovl)->data->addr + 0x20 *
> (n) + 0x04)
> +#define DISP_REG_OVL_HDR_PITCH(ovl, n) ((ovl)->data->addr + 0x20 *
> (n) + 0x08)
>
> #define GMC_THRESHOLD_BITS 16
> #define GMC_THRESHOLD_HIGH ((1 << GMC_THRESHOLD_BITS) / 4)
> @@ -67,6 +74,7 @@ struct mtk_disp_ovl_data {
> unsigned int layer_nr;
> bool fmt_rgb565_is_0;
> bool smi_id_en;
> + bool supports_afbc;
> };
>
> /*
> @@ -172,7 +180,22 @@ void mtk_ovl_stop(struct device *dev)
> reg = reg & ~OVL_LAYER_SMI_ID_EN;
> writel_relaxed(reg, ovl->regs + DISP_REG_OVL_DATAPATH_CON);
> }
> +}
> +
> +static void mtk_ovl_set_afbc(struct device *dev, struct cmdq_pkt *cmdq_pkt,
> +int idx, bool enabled)
> +{
> + struct mtk_disp_ovl *ovl = dev_get_drvdata(dev);
This is a ovl internal function, so you could pass ovl directly into
this function.
> + unsigned int reg;
>
> + reg = readl(ovl->regs + DISP_REG_OVL_DATAPATH_CON);
> + if (enabled)
> + reg = reg | OVL_LAYER_AFBC_EN(idx);
> + else
> + reg = reg & ~OVL_LAYER_AFBC_EN(idx);
> +
> + mtk_ddp_write_relaxed(cmdq_pkt, reg, >cmdq_reg,
> + ovl->regs, DISP_REG_OVL_DATAPATH_CON);
In normal case, read/write register by cmdq, so
mtk_ddp_write_mask(cmdq_pkt, enable ? OVL_LAYER_AFBC_EN(idx) : 0,
>cmdq_reg, ovl->regs,
DISP_REG_OVL_DATAPATH_CON,
OVL_LAYER_AFBC_EN(idx));
> }
>
> void mtk_ovl_config(struct device *dev, unsigned int w,
> @@ -226,6 +249,32 @@ int mtk_ovl_layer_check(struct device *dev, unsigned int
> idx,
> if (state->fb->format->is_yuv && rotation != 0)
> return -EINVAL;
>
> + if (state->fb->modifier) {
> + unsigned long long modifier;
> + unsigned int fourcc;
> + struct mtk_disp_ovl *ovl = dev_get_drvdata(dev);
> +
> + if (!ovl->data->supports_afbc)
> + return -EINVAL;
> +
> + modifier = state->fb->modifier;
> +
> + if (modifier !=
> DRM_FORMAT_MOD_ARM_AFBC(AFBC_FORMAT_MOD_BLOCK_SIZE_32x8 |
> + AFBC_FORMAT_MOD_SPLIT
> |
> +
> AFBC_FORMAT_MOD_SPARSE))
> + return -EINVAL;
> +
> + fourcc = state->fb->format->format;
> + if (fourcc != DRM_FORMAT_BGRA &&
> + fourcc != DRM_FORMAT_ABGR &&
> + fourcc !=
Re: [PATCH v3] drm/mediatek: Add AFBC support to Mediatek DRM driver
Thanks for the comments everyone! I'll upload a new CL sometime today. I did want to ask though, I realize I should be using u32/u64 for kernel code in general, but the rest of this file seems to be written using unsigned int/unsigned long long. In this circumstance, does keeping with the style of the original source take precedence over general style guidelines, or vice versa?
Re: [PATCH v3] drm/mediatek: Add AFBC support to Mediatek DRM driver
Hi Justin,
On Wed, 12 Oct 2022 at 20:12, Justin Green wrote:
> @@ -226,6 +249,32 @@ int mtk_ovl_layer_check(struct device *dev, unsigned
> int idx,
> if (state->fb->format->is_yuv && rotation != 0)
> return -EINVAL;
>
> + if (state->fb->modifier) {
>
Please spell this out explicitly as DRM_FORMAT_MOD_LINEAR. For some reason,
we specify that 0 is explicitly block-linear, whilst INVALID ('unknown
layout, figure it out by magic') is a non-zero value. So != 0 isn't a check
for 'was a modifier explicitly specified', even if != 0 is almost always
'is this buffer non-linear'.
+ unsigned long long modifier;
> + unsigned int fourcc;
>
u64, u32, but ...
> + if (!ovl->data->supports_afbc)
> + return -EINVAL;
> +
> + modifier = state->fb->modifier;
> +
> + if (modifier !=
> DRM_FORMAT_MOD_ARM_AFBC(AFBC_FORMAT_MOD_BLOCK_SIZE_32x8 |
> +
> AFBC_FORMAT_MOD_SPLIT |
> +
> AFBC_FORMAT_MOD_SPARSE))
> + return -EINVAL;
> +
> + fourcc = state->fb->format->format;
> + if (fourcc != DRM_FORMAT_BGRA &&
> + fourcc != DRM_FORMAT_ABGR &&
> + fourcc != DRM_FORMAT_ARGB &&
> + fourcc != DRM_FORMAT_XRGB &&
> + fourcc != DRM_FORMAT_XBGR &&
> + fourcc != DRM_FORMAT_RGB888 &&
> + fourcc != DRM_FORMAT_BGR888)
> + return -EINVAL;
The core shouldn't allow a framebuffer to be created with a format/modifier
pair which wasn't advertised, so these checks can be dropped. Except that
it's never advertised.
mtk_plane_init() passes a set of formats and modifiers to
drm_universal_plane_init(); the AFBC modifier needs to be added here, as
well as LINEAR and INVALID. Then you'll need to set the
format_mod_supported() hook on the plane to filter out format/modifier
pairs. After that, you should see (e.g. with drm_info) that you get an
IN_FORMATS blob which advertises DRM_FORMAT_MOD_ARM_AFBC(...) as well as
linear for DRM_FORMAT_XRGB, but only linear for DRM_FORMAT_RGB565.
After doing that, you should see framebuffer creation fail for unsupported
pairings, e.g. RGB565 + AFBC.
> + bool is_afbc = pending->modifier;
... != DRM_FORMAT_MOD_LINEAR
> @@ -52,6 +53,7 @@ static void mtk_plane_reset(struct drm_plane *plane)
>
> state->base.plane = plane;
> state->pending.format = DRM_FORMAT_RGB565;
> + state->pending.modifier = 0;
>
= DRM_FORMAT_MOD_LINEAR
> @@ -120,21 +122,52 @@ static void mtk_plane_update_new_state(struct
> drm_plane_state *new_state,
> struct drm_gem_object *gem;
> struct mtk_drm_gem_obj *mtk_gem;
> unsigned int pitch, format;
> + unsigned long long modifier;
>
u64
> + if (!modifier) {
>
modifier == DRM_FORMAT_MOD_LINEAR
Cheers,
Daniel
Re: [PATCH v3] drm/mediatek: Add AFBC support to Mediatek DRM driver
Il 12/10/22 21:12, Justin Green ha scritto: Tested on MT8195 and confirmed both correct video output and improved DRAM bandwidth performance. v3: * Replaced pitch bitshift math with union based approach. * Refactored overlay register writes to shared code between non-AFBC and AFBC. * Minor code cleanups. v2: * Marked mtk_ovl_set_afbc as static. * Reflowed some lines to fit column limit. Signed-off-by: Justin Green --- drivers/gpu/drm/mediatek/mtk_disp_ovl.c | 90 +++- drivers/gpu/drm/mediatek/mtk_drm_plane.c | 37 +- drivers/gpu/drm/mediatek/mtk_drm_plane.h | 8 +++ 3 files changed, 131 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c index 002b0f6cae1a..3f89b5f5064f 100644 --- a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c +++ b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c ..snip.. @@ -335,9 +396,10 @@ void mtk_ovl_layer_config(struct device *dev, unsigned int idx, addr += pending->pitch - 1; } + mtk_ovl_set_afbc(dev, cmdq_pkt, idx, is_afbc); I'm sorry for not noticing that in the previous review - there's only one more issue here: I'm not sure that *all* of the MediaTek chips have the AFBC bits in OVL_DATAPATH_CON... this may be clashing with something else in the layout of (very?) old chips. The solution is simple here. Please, guard this call with: if (ovl->data->supports_afbc) mtk_ovl_set_afbc(dev, cmdq_pkt, idx, is_afbc); ...after which Reviewed-by: AngeloGioacchino Del Regno Cheers! Angelo
[PATCH v3] drm/mediatek: Add AFBC support to Mediatek DRM driver
Tested on MT8195 and confirmed both correct video output and improved DRAM
bandwidth performance.
v3:
* Replaced pitch bitshift math with union based approach.
* Refactored overlay register writes to shared code between non-AFBC and
AFBC.
* Minor code cleanups.
v2:
* Marked mtk_ovl_set_afbc as static.
* Reflowed some lines to fit column limit.
Signed-off-by: Justin Green
---
drivers/gpu/drm/mediatek/mtk_disp_ovl.c | 90 +++-
drivers/gpu/drm/mediatek/mtk_drm_plane.c | 37 +-
drivers/gpu/drm/mediatek/mtk_drm_plane.h | 8 +++
3 files changed, 131 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
index 002b0f6cae1a..3f89b5f5064f 100644
--- a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
+++ b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
@@ -29,17 +29,24 @@
#define DISP_REG_OVL_DATAPATH_CON 0x0024
#define OVL_LAYER_SMI_ID_ENBIT(0)
#define OVL_BGCLR_SEL_IN BIT(2)
+#define OVL_LAYER_AFBC_EN(n) BIT(4+n)
#define DISP_REG_OVL_ROI_BGCLR 0x0028
#define DISP_REG_OVL_SRC_CON 0x002c
#define DISP_REG_OVL_CON(n)(0x0030 + 0x20 * (n))
#define DISP_REG_OVL_SRC_SIZE(n) (0x0038 + 0x20 * (n))
#define DISP_REG_OVL_OFFSET(n) (0x003c + 0x20 * (n))
+#define DISP_REG_OVL_PITCH_MSB(n) (0x0040 + 0x20 * (n))
+#define OVL_PITCH_MSB_2ND_SUBBUF BIT(16)
+#define OVL_PITCH_MSB_YUV_TRANSBIT(20)
#define DISP_REG_OVL_PITCH(n) (0x0044 + 0x20 * (n))
+#define DISP_REG_OVL_CLIP(n) (0x004c + 0x20 * (n))
#define DISP_REG_OVL_RDMA_CTRL(n) (0x00c0 + 0x20 * (n))
#define DISP_REG_OVL_RDMA_GMC(n) (0x00c8 + 0x20 * (n))
#define DISP_REG_OVL_ADDR_MT2701 0x0040
#define DISP_REG_OVL_ADDR_MT8173 0x0f40
#define DISP_REG_OVL_ADDR(ovl, n) ((ovl)->data->addr + 0x20 * (n))
+#define DISP_REG_OVL_HDR_ADDR(ovl, n) ((ovl)->data->addr + 0x20 * (n)
+ 0x04)
+#define DISP_REG_OVL_HDR_PITCH(ovl, n) ((ovl)->data->addr + 0x20 * (n)
+ 0x08)
#define GMC_THRESHOLD_BITS 16
#define GMC_THRESHOLD_HIGH ((1 << GMC_THRESHOLD_BITS) / 4)
@@ -67,6 +74,7 @@ struct mtk_disp_ovl_data {
unsigned int layer_nr;
bool fmt_rgb565_is_0;
bool smi_id_en;
+ bool supports_afbc;
};
/*
@@ -172,7 +180,22 @@ void mtk_ovl_stop(struct device *dev)
reg = reg & ~OVL_LAYER_SMI_ID_EN;
writel_relaxed(reg, ovl->regs + DISP_REG_OVL_DATAPATH_CON);
}
+}
+
+static void mtk_ovl_set_afbc(struct device *dev, struct cmdq_pkt *cmdq_pkt,
+int idx, bool enabled)
+{
+ struct mtk_disp_ovl *ovl = dev_get_drvdata(dev);
+ unsigned int reg;
+ reg = readl(ovl->regs + DISP_REG_OVL_DATAPATH_CON);
+ if (enabled)
+ reg = reg | OVL_LAYER_AFBC_EN(idx);
+ else
+ reg = reg & ~OVL_LAYER_AFBC_EN(idx);
+
+ mtk_ddp_write_relaxed(cmdq_pkt, reg, >cmdq_reg,
+ ovl->regs, DISP_REG_OVL_DATAPATH_CON);
}
void mtk_ovl_config(struct device *dev, unsigned int w,
@@ -226,6 +249,32 @@ int mtk_ovl_layer_check(struct device *dev, unsigned int
idx,
if (state->fb->format->is_yuv && rotation != 0)
return -EINVAL;
+ if (state->fb->modifier) {
+ unsigned long long modifier;
+ unsigned int fourcc;
+ struct mtk_disp_ovl *ovl = dev_get_drvdata(dev);
+
+ if (!ovl->data->supports_afbc)
+ return -EINVAL;
+
+ modifier = state->fb->modifier;
+
+ if (modifier !=
DRM_FORMAT_MOD_ARM_AFBC(AFBC_FORMAT_MOD_BLOCK_SIZE_32x8 |
+ AFBC_FORMAT_MOD_SPLIT |
+ AFBC_FORMAT_MOD_SPARSE))
+ return -EINVAL;
+
+ fourcc = state->fb->format->format;
+ if (fourcc != DRM_FORMAT_BGRA &&
+ fourcc != DRM_FORMAT_ABGR &&
+ fourcc != DRM_FORMAT_ARGB &&
+ fourcc != DRM_FORMAT_XRGB &&
+ fourcc != DRM_FORMAT_XBGR &&
+ fourcc != DRM_FORMAT_RGB888 &&
+ fourcc != DRM_FORMAT_BGR888)
+ return -EINVAL;
+ }
+
state->rotation = rotation;
return 0;
@@ -310,11 +359,23 @@ void mtk_ovl_layer_config(struct device *dev, unsigned
int idx,
struct mtk_disp_ovl *ovl = dev_get_drvdata(dev);
struct mtk_plane_pending_state *pending = >pending;
unsigned int addr = pending->addr;
- unsigned int pitch = pending->pitch & 0x;
+ unsigned int
