[RESEND PATCH v2] drm/mediatek: Add AFBC support to Mediatek DRM driver
AngeloGioacchino Del Regno
angelogioacchino.delregno at collabora.com
Wed Oct 12 08:25:45 UTC 2022
Il 11/10/22 20:28, Justin Green ha scritto:
> Hi Angelo,
> Thanks for the suggestions! I'll upload another patch with those changes.
>
> Re the pitch register math, would it be acceptable to define separate
> macros for the LSB and MSB to abstract away the magic numbers? For
> example:
> #define OVL_PITCH_MSB(n) ((n >> 16) & GENMASK(15, 0))
> #define OVL_PITCH_LSB(n) (n & GENMASK(15, 0))
>
These would be different from the macros that are available in bitfield.h, but
not *fundamentally* different, so these would look a little redundant...
I think that you refer to that `pitch` variable that's coming from the DRM(/fb)
API... and bitfield macros are for register access... so I guess that one clean
way of avoiding the magic shifting (that is purely used to split the 32-bits
number in two 16-bits 'chunks') would be to perhaps use a union, so that you
will have something like u.pitch_lsb, u.pitch_msb (with lsb/msb being two u16).
That'd better represent what is going on here, I believe?
Regards,
Angelo
> Regards,
> Justin
>
> On Tue, Oct 11, 2022 at 5:09 AM AngeloGioacchino Del Regno
> <angelogioacchino.delregno at collabora.com> wrote:
>>
>> Il 10/10/22 17:01, Justin Green ha scritto:
>>> From: Justin Green <greenjustin at chromium.org>
>>>
>>> Add AFBC support to Mediatek DRM driver and enable on MT8195.
>>>
>>> Tested on MT8195 and confirmed both correct video output and improved DRAM
>>> bandwidth performance.
>>>
>>> v2:
>>> Marked mtk_ovl_set_afbc as static, reflowed some lines to fit column
>>> limit.
>>>
>>> Signed-off-by: Justin Green <greenjustin at chromium.org>
>>
>> Hello Justin,
>> thanks for the patch!
>>
>> However, there's something to improve...
>>
>>> ---
>>> drivers/gpu/drm/mediatek/mtk_disp_ovl.c | 108 ++++++++++++++++++++---
>>> drivers/gpu/drm/mediatek/mtk_drm_plane.c | 37 +++++++-
>>> drivers/gpu/drm/mediatek/mtk_drm_plane.h | 8 ++
>>> 3 files changed, 140 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
>>> index 002b0f6cae1a..1724ea85a840 100644
>>> --- a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
>>> +++ b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
>>
>> ..snip..
>>
>>> @@ -208,6 +231,8 @@ int mtk_ovl_layer_check(struct device *dev, unsigned int idx,
>>> {
>>> struct drm_plane_state *state = &mtk_state->base;
>>> unsigned int rotation = 0;
>>> + unsigned long long modifier;
>>> + unsigned int fourcc;
>>>
>>> rotation = drm_rotation_simplify(state->rotation,
>>> DRM_MODE_ROTATE_0 |
>>> @@ -226,6 +251,30 @@ 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) {
>>> + struct mtk_disp_ovl *ovl = dev_get_drvdata(dev);
>>
>> Since you're introducing modifier and fourcc for this branch only, you
>> may as well just declare them here instead, but either way is fine.
>>
>>> +
>>> + 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_BGRA8888 &&
>>> + fourcc != DRM_FORMAT_ABGR8888 &&
>>> + fourcc != DRM_FORMAT_ARGB8888 &&
>>> + fourcc != DRM_FORMAT_XRGB8888 &&
>>> + fourcc != DRM_FORMAT_XBGR8888 &&
>>> + fourcc != DRM_FORMAT_RGB888 &&
>>> + fourcc != DRM_FORMAT_BGR888)
>>> + return -EINVAL;
>>> + }
>>> +
>>> state->rotation = rotation;
>>>
>>> return 0;
>>> @@ -310,11 +359,14 @@ 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 = &state->pending;
>>> unsigned int addr = pending->addr;
>>> - unsigned int pitch = pending->pitch & 0xffff;
>>> + unsigned int hdr_addr = pending->hdr_addr;
>>> + unsigned int pitch = pending->pitch;
>>> + unsigned int hdr_pitch = pending->hdr_pitch;
>>> unsigned int fmt = pending->format;
>>> unsigned int offset = (pending->y << 16) | pending->x;
>>> unsigned int src_size = (pending->height << 16) | pending->width;
>>> unsigned int con;
>>> + bool is_afbc = pending->modifier;
>>>
>>> if (!pending->enable) {
>>> mtk_ovl_layer_off(dev, idx, cmdq_pkt);
>>> @@ -335,16 +387,39 @@ void mtk_ovl_layer_config(struct device *dev, unsigned int idx,
>>> addr += pending->pitch - 1;
>>> }
>>>
>>> - mtk_ddp_write_relaxed(cmdq_pkt, con, &ovl->cmdq_reg, ovl->regs,
>>> - DISP_REG_OVL_CON(idx));
>>> - mtk_ddp_write_relaxed(cmdq_pkt, pitch, &ovl->cmdq_reg, ovl->regs,
>>> - DISP_REG_OVL_PITCH(idx));
>>> - mtk_ddp_write_relaxed(cmdq_pkt, src_size, &ovl->cmdq_reg, ovl->regs,
>>> - DISP_REG_OVL_SRC_SIZE(idx));
>>> - mtk_ddp_write_relaxed(cmdq_pkt, offset, &ovl->cmdq_reg, ovl->regs,
>>> - DISP_REG_OVL_OFFSET(idx));
>>> - mtk_ddp_write_relaxed(cmdq_pkt, addr, &ovl->cmdq_reg, ovl->regs,
>>> - DISP_REG_OVL_ADDR(ovl, idx));
>>> + mtk_ovl_set_afbc(dev, cmdq_pkt, idx, is_afbc);
>>> + if (!is_afbc) {
>>> + mtk_ddp_write_relaxed(cmdq_pkt, con, &ovl->cmdq_reg, ovl->regs,
>>> + DISP_REG_OVL_CON(idx));
>>> + mtk_ddp_write_relaxed(cmdq_pkt, pitch & 0xFFFF, &ovl->cmdq_reg, ovl->regs,
>>> + DISP_REG_OVL_PITCH(idx));
>>> + mtk_ddp_write_relaxed(cmdq_pkt, src_size, &ovl->cmdq_reg, ovl->regs,
>>> + DISP_REG_OVL_SRC_SIZE(idx));
>>> + mtk_ddp_write_relaxed(cmdq_pkt, offset, &ovl->cmdq_reg, ovl->regs,
>>> + DISP_REG_OVL_OFFSET(idx));
>>> + mtk_ddp_write_relaxed(cmdq_pkt, addr, &ovl->cmdq_reg, ovl->regs,
>>> + DISP_REG_OVL_ADDR(ovl, idx));
>>> + } else {
>>> + mtk_ddp_write_relaxed(cmdq_pkt, addr, &ovl->cmdq_reg, ovl->regs,
>>> + DISP_REG_OVL_ADDR(ovl, idx));
>>> + mtk_ddp_write_relaxed(cmdq_pkt, hdr_addr, &ovl->cmdq_reg, ovl->regs,
>>> + DISP_REG_OVL_HDR_ADDR(ovl, idx));
>>> + mtk_ddp_write_relaxed(cmdq_pkt, src_size, &ovl->cmdq_reg, ovl->regs,
>>> + DISP_REG_OVL_SRC_SIZE(idx));
>>> + mtk_ddp_write_relaxed(cmdq_pkt,
>>> + OVL_PITCH_MSB_2ND_SUBBUF | ((pitch >> 16) & 0xFFFF),
>>
>> I say that this would be more readable if you use bitfield macros instead but,
>> anyway, that magic "16" number must get a definition.
>> I have the same comment about the GENMASK(15, 0) (== 0xffff), but I know that
>> doesn't come from you... would still be nice to also add a definition, which
>> is anyway "practically mandatory" if you use FIELD_PREP(mask, val).
>>
>>> + &ovl->cmdq_reg, ovl->regs, DISP_REG_OVL_PITCH_MSB(idx));
>>> + mtk_ddp_write_relaxed(cmdq_pkt, pitch & 0xFFFF, &ovl->cmdq_reg, ovl->regs,
>>> + DISP_REG_OVL_PITCH(idx));
>>> + mtk_ddp_write_relaxed(cmdq_pkt, hdr_pitch, &ovl->cmdq_reg, ovl->regs,
>>> + DISP_REG_OVL_HDR_PITCH(ovl, idx));
>>> + mtk_ddp_write_relaxed(cmdq_pkt, con, &ovl->cmdq_reg, ovl->regs,
>>> + DISP_REG_OVL_CON(idx));
>>> + mtk_ddp_write_relaxed(cmdq_pkt, offset, &ovl->cmdq_reg, ovl->regs,
>>> + DISP_REG_OVL_OFFSET(idx));
>>> + mtk_ddp_write_relaxed(cmdq_pkt, 0, &ovl->cmdq_reg, ovl->regs,
>>> + DISP_REG_OVL_CLIP(idx));
>>> + }
>>
>> In any case - are you use that we *must* program the registers in this exact
>> sequence?
>>
>> Here, the OVL layer is supposed to be OFF (OVL_SRC_CON == 0, OVL_RDMA_CTRL == 0)
>> so the contents of the registers that you're modifying should not matter at all
>> until the layer is turned on.
>>
>> Note that, in case it doesn't matter, this gets greatly simplified, exactly as:
>>
>> at the top -- after DISP_REG_OVL_PITCH_MSB(n):
>> #define OVL_SRC_PITCH_MSB GENMASK(3, 0)
>>
>> after DISP_REG_OVL_PITCH(n):
>> #define OVL_SRC_PITCH_LSB GENMASK(15, 0)
>>
>> mtk_ddp_write_relaxed(cmdq_pkt, con, &ovl->cmdq_reg, ovl->regs,
>> DISP_REG_OVL_CON(idx));
>> mtk_ddp_write_relaxed(cmdq_pkt, FIELD_PREP(OVL_SRC_PITCH_LSB, pitch),
>> &ovl->cmdq_reg, ovl->regs, DISP_REG_OVL_PITCH(idx));
>> mtk_ddp_write_relaxed(cmdq_pkt, src_size, &ovl->cmdq_reg, ovl->regs,
>> DISP_REG_OVL_SRC_SIZE(idx));
>> mtk_ddp_write_relaxed(cmdq_pkt, offset, &ovl->cmdq_reg, ovl->regs,
>> DISP_REG_OVL_OFFSET(idx));
>> mtk_ddp_write_relaxed(cmdq_pkt, addr, &ovl->cmdq_reg, ovl->regs,
>> DISP_REG_OVL_ADDR(ovl, idx));
>>
>> if (is_afbc) {
>> pitch_msb = FIELD_PREP(OVL_SRC_PITCH_MSB, (pitch >> 16));
>> pitch_msb |= FIELD_PREP(OVL_PITCH_MSB_2ND_SUBBUF, 1);
>> mtk_ddp_write_relaxed(cmdq_pkt, hdr_addr, &ovl->cmdq_reg, ovl->regs,
>> DISP_REG_OVL_HDR_ADDR(ovl, idx));
>> mtk_ddp_write_relaxed(cmdq_pkt, val, &ovl->cmdq_reg, ovl->regs,
>> DISP_REG_OVL_PITCH_MSB(idx));
>> mtk_ddp_write_relaxed(cmdq_pkt, hdr_pitch, &ovl->cmdq_reg, ovl->regs,
>> DISP_REG_OVL_HDR_PITCH(ovl, idx));
>> mtk_ddp_write_relaxed(cmdq_pkt, 0, &ovl->cmdq_reg, ovl->regs,
>> DISP_REG_OVL_CLIP(idx));
>> }
>>
>>>
>>> mtk_ovl_layer_on(dev, idx, cmdq_pkt);
>>> }
>>> @@ -492,6 +567,15 @@ static const struct mtk_disp_ovl_data mt8192_ovl_2l_driver_data = {
>>
>> ..snip..
>>
>>> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.c b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
>>> index 5c0d9ce69931..734d2554b2b8 100644
>>> --- a/drivers/gpu/drm/mediatek/mtk_drm_plane.c
>>> +++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
>>> @@ -12,6 +12,7 @@
>>> #include <drm/drm_framebuffer.h>
>>> #include <drm/drm_gem_atomic_helper.h>
>>> #include <drm/drm_plane_helper.h>
>>> +#include <linux/align.h>
>>>
>>> #include "mtk_drm_crtc.h"
>>> #include "mtk_drm_ddp_comp.h"
>>> @@ -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;
>>> }
>>>
>>> static struct drm_plane_state *mtk_plane_duplicate_state(struct drm_plane *plane)
>>> @@ -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;
>>> dma_addr_t addr;
>>> + dma_addr_t hdr_addr = 0;
>>> + unsigned int hdr_pitch = 0;
>>>
>>> gem = fb->obj[0];
>>> mtk_gem = to_mtk_gem_obj(gem);
>>> addr = mtk_gem->dma_addr;
>>> pitch = fb->pitches[0];
>>> format = fb->format->format;
>>> + modifier = fb->modifier;
>>>
>>> - addr += (new_state->src.x1 >> 16) * fb->format->cpp[0];
>>> - addr += (new_state->src.y1 >> 16) * pitch;
>>> + if (!modifier) {
>>> + addr += (new_state->src.x1 >> 16) * fb->format->cpp[0];
>>> + addr += (new_state->src.y1 >> 16) * pitch;
>>> + } else {
>>> + int width_in_blocks = ALIGN(fb->width, AFBC_DATA_BLOCK_WIDTH)
>>> + / AFBC_DATA_BLOCK_WIDTH;
>>> + int height_in_blocks = ALIGN(fb->height, AFBC_DATA_BLOCK_HEIGHT)
>>> + / AFBC_DATA_BLOCK_HEIGHT;
>>> + int x_offset_in_blocks = (new_state->src.x1 >> 16) / AFBC_DATA_BLOCK_WIDTH;
>>> + int y_offset_in_blocks = (new_state->src.y1 >> 16) / AFBC_DATA_BLOCK_HEIGHT;
>>> + int hdr_size;
>>> +
>>> + hdr_pitch = width_in_blocks * AFBC_HEADER_BLOCK_SIZE;
>>> + pitch = width_in_blocks * AFBC_DATA_BLOCK_WIDTH *
>>> + AFBC_DATA_BLOCK_HEIGHT * fb->format->cpp[0];
>>> +
>>> + hdr_size = ALIGN(hdr_pitch * height_in_blocks, AFBC_HEADER_ALIGNMENT);
>>> +
>>> + hdr_addr = addr + hdr_pitch * y_offset_in_blocks +
>>> + AFBC_HEADER_BLOCK_SIZE * x_offset_in_blocks;
>>> + // The data plane is offset by 1 additional block.
>>
>> C-style comments please.
>>
>>> + addr = addr + hdr_size +
>>> + pitch * y_offset_in_blocks +
>>> + AFBC_DATA_BLOCK_WIDTH * AFBC_DATA_BLOCK_HEIGHT *
>>> + fb->format->cpp[0] * (x_offset_in_blocks + 1);
>>> + }
>>>
>>
>> Regards,
>> Angelo
>>
More information about the dri-devel
mailing list