Re: [PATCH v3] drm/mediatek: Add AFBC support to Mediatek DRM driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



 * , ,
 Hi, Justin:

Justin Green <greenjustin@xxxxxxxxxxxx> 於 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 <greenjustin@xxxxxxxxxxxx>
> ---
>  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_EN                            BIT(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_TRANS                        BIT(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, &ovl->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,
                               &ovl->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_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,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 = &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;
> +       union overlay_pitch {
> +               struct split_pitch {
> +                       u16 lsb;
> +                       u16 msb;
> +               } split_pitch;
> +               u32 pitch;
> +       } overlay_pitch;
> +
> +       overlay_pitch.pitch = pitch;
>
>         if (!pending->enable) {
>                 mtk_ovl_layer_off(dev, idx, cmdq_pkt);
> @@ -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);
>         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,
> +       mtk_ddp_write_relaxed(cmdq_pkt, overlay_pitch.split_pitch.lsb, &ovl->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.

>         mtk_ddp_write_relaxed(cmdq_pkt, src_size, &ovl->cmdq_reg, ovl->regs,
>                               DISP_REG_OVL_SRC_SIZE(idx));
> @@ -345,6 +407,19 @@ void mtk_ovl_layer_config(struct device *dev, unsigned int idx,
>                               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) {
> +               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,
> +                                     OVL_PITCH_MSB_2ND_SUBBUF | overlay_pitch.split_pitch.msb,
> +                                     &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));
> +       } else {
> +               mtk_ddp_write_relaxed(cmdq_pkt,
> +                                     overlay_pitch.split_pitch.msb,
> +                                     &ovl->cmdq_reg, ovl->regs, DISP_REG_OVL_PITCH_MSB(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 = {
>         .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.

> +       .addr = DISP_REG_OVL_ADDR_MT8173,
> +       .gmc_bits = 10,
> +       .layer_nr = 4,
> +       .fmt_rgb565_is_0 = true,
> +       .smi_id_en = true,
> +       .supports_afbc = true,
> +};
> +
>  static const struct of_device_id mtk_disp_ovl_driver_dt_match[] = {
>         { .compatible = "mediatek,mt2701-disp-ovl",
>           .data = &mt2701_ovl_driver_data},
> @@ -505,6 +589,8 @@ static const struct of_device_id mtk_disp_ovl_driver_dt_match[] = {
>           .data = &mt8192_ovl_driver_data},
>         { .compatible = "mediatek,mt8192-disp-ovl-2l",
>           .data = &mt8192_ovl_2l_driver_data},
> +       { .compatible = "mediatek,mt8195-disp-ovl",
> +         .data = &mt8195_ovl_driver_data},
>         {},
>  };
>  MODULE_DEVICE_TABLE(of, mtk_disp_ovl_driver_dt_match);
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.c b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> index 5c0d9ce69931..a285b9ff5081 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);

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;

Could you explain the meaning of hdr_pitch?

Regards,
Chun-Kuang.

> +
> +               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. */
> +               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);
> +       }
>
>         mtk_plane_state->pending.enable = true;
>         mtk_plane_state->pending.pitch = pitch;
> +       mtk_plane_state->pending.hdr_pitch = hdr_pitch;
>         mtk_plane_state->pending.format = format;
> +       mtk_plane_state->pending.modifier = modifier;
>         mtk_plane_state->pending.addr = addr;
> +       mtk_plane_state->pending.hdr_addr = hdr_addr;
>         mtk_plane_state->pending.x = new_state->dst.x1;
>         mtk_plane_state->pending.y = new_state->dst.y1;
>         mtk_plane_state->pending.width = drm_rect_width(&new_state->dst);
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.h b/drivers/gpu/drm/mediatek/mtk_drm_plane.h
> index 2d5ec66e3df1..8f39011cdbfc 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_plane.h
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.h
> @@ -10,12 +10,20 @@
>  #include <drm/drm_crtc.h>
>  #include <linux/types.h>
>
> +#define AFBC_DATA_BLOCK_WIDTH 32
> +#define AFBC_DATA_BLOCK_HEIGHT 8
> +#define AFBC_HEADER_BLOCK_SIZE 16
> +#define AFBC_HEADER_ALIGNMENT 1024
> +
>  struct mtk_plane_pending_state {
>         bool                            config;
>         bool                            enable;
>         dma_addr_t                      addr;
> +       dma_addr_t                      hdr_addr;
>         unsigned int                    pitch;
> +       unsigned int                    hdr_pitch;
>         unsigned int                    format;
> +       unsigned long long              modifier;
>         unsigned int                    x;
>         unsigned int                    y;
>         unsigned int                    width;
> --
> 2.38.0.rc1.362.ged0d419d3c-goog
>




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux