On Thu, Jan 26, 2023 at 07:47:44PM +0100, Lucas Stach wrote:
> imx-drm doesn't mandate a modeset when the framebuffer modifier changes,
> but currently the tile prefetch and resolve (TPR) configuration of the
> PRE is only set up on the initial modeset.
>
> As the TPR configuration is double buffered, same as all the other PRE
> states, we can support dynamic reconfiguration of the buffer layout from
> one frame to another. As switching between (super-)tiled and linear
> prefetch needs to touch the CTRL register make sure to do the
> reconfiguration inside the safe window.
>
> Signed-off-by: Lucas Stach <l.stach@xxxxxxxxxxxxxx>
> ---
> drivers/gpu/ipu-v3/ipu-pre.c | 59 +++++++++++++++++++++++++++++-------
> drivers/gpu/ipu-v3/ipu-prg.c | 2 +-
> drivers/gpu/ipu-v3/ipu-prv.h | 2 +-
> 3 files changed, 50 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/ipu-v3/ipu-pre.c b/drivers/gpu/ipu-v3/ipu-pre.c
> index befffc85a146..e8d9792827dd 100644
> --- a/drivers/gpu/ipu-v3/ipu-pre.c
> +++ b/drivers/gpu/ipu-v3/ipu-pre.c
> @@ -99,8 +99,12 @@ struct ipu_pre {
>
> struct {
> bool in_use;
> + uint64_t modifier;
> + unsigned int height;
> unsigned int safe_window_end;
> unsigned int bufaddr;
> + u32 ctrl;
> + u8 cpp;
> } cur;
> };
>
> @@ -173,6 +177,11 @@ void ipu_pre_configure(struct ipu_pre *pre, unsigned int width,
> u32 active_bpp = info->cpp[0] >> 1;
> u32 val;
>
> + pre->cur.bufaddr = bufaddr;
> + pre->cur.height = height;
> + pre->cur.modifier = modifier;
> + pre->cur.cpp = info->cpp[0];
> +
> /* calculate safe window for ctrl register updates */
> if (modifier == DRM_FORMAT_MOD_LINEAR)
> pre->cur.safe_window_end = height - 2;
> @@ -181,7 +190,6 @@ void ipu_pre_configure(struct ipu_pre *pre, unsigned int width,
>
> writel(bufaddr, pre->regs + IPU_PRE_CUR_BUF);
> writel(bufaddr, pre->regs + IPU_PRE_NEXT_BUF);
> - pre->cur.bufaddr = bufaddr;
>
> val = IPU_PRE_PREF_ENG_CTRL_INPUT_PIXEL_FORMAT(0) |
> IPU_PRE_PREF_ENG_CTRL_INPUT_ACTIVE_BPP(active_bpp) |
> @@ -223,28 +231,56 @@ void ipu_pre_configure(struct ipu_pre *pre, unsigned int width,
> }
> writel(val, pre->regs + IPU_PRE_TPR_CTRL);
>
> - val = readl(pre->regs + IPU_PRE_CTRL);
> - val |= IPU_PRE_CTRL_EN_REPEAT | IPU_PRE_CTRL_ENABLE |
> - IPU_PRE_CTRL_SDW_UPDATE;
> + pre->cur.ctrl = readl(pre->regs + IPU_PRE_CTRL);
> + pre->cur.ctrl |= IPU_PRE_CTRL_EN_REPEAT | IPU_PRE_CTRL_ENABLE;
> if (modifier == DRM_FORMAT_MOD_LINEAR)
> - val &= ~IPU_PRE_CTRL_BLOCK_EN;
> + pre->cur.ctrl &= ~IPU_PRE_CTRL_BLOCK_EN;
> else
> - val |= IPU_PRE_CTRL_BLOCK_EN;
> - writel(val, pre->regs + IPU_PRE_CTRL);
> + pre->cur.ctrl |= IPU_PRE_CTRL_BLOCK_EN;
> + writel(pre->cur.ctrl | IPU_PRE_CTRL_SDW_UPDATE,
> + pre->regs + IPU_PRE_CTRL);
> }
>
> -void ipu_pre_update(struct ipu_pre *pre, unsigned int bufaddr)
> +void ipu_pre_update(struct ipu_pre *pre, uint64_t modifier, unsigned int bufaddr)
> {
> unsigned long timeout = jiffies + msecs_to_jiffies(5);
> unsigned short current_yblock;
> + unsigned int safe_window_end = pre->cur.safe_window_end;
> u32 val;
>
> - if (bufaddr == pre->cur.bufaddr)
> + if (bufaddr == pre->cur.bufaddr &&
> + modifier == pre->cur.modifier)
> return;
>
> writel(bufaddr, pre->regs + IPU_PRE_NEXT_BUF);
> pre->cur.bufaddr = bufaddr;
>
> + if (modifier != pre->cur.modifier) {
> + val = readl(pre->regs + IPU_PRE_TPR_CTRL);
> + val &= ~IPU_PRE_TPR_CTRL_TILE_FORMAT_MASK;
> + if (modifier != DRM_FORMAT_MOD_LINEAR) {
> + /* only support single buffer formats for now */
> + val |= IPU_PRE_TPR_CTRL_TILE_FORMAT_SINGLE_BUF;
> + if (modifier == DRM_FORMAT_MOD_VIVANTE_SUPER_TILED)
> + val |= IPU_PRE_TPR_CTRL_TILE_FORMAT_SUPER_TILED;
> + if (pre->cur.cpp == 2)
> + val |= IPU_PRE_TPR_CTRL_TILE_FORMAT_16_BIT;
> + }
> + writel(val, pre->regs + IPU_PRE_TPR_CTRL);
> +
> + if (modifier == DRM_FORMAT_MOD_LINEAR)
> + pre->cur.ctrl &= ~IPU_PRE_CTRL_BLOCK_EN;
> + else
> + pre->cur.ctrl |= IPU_PRE_CTRL_BLOCK_EN;
> +
> + if (modifier == DRM_FORMAT_MOD_LINEAR)
> + pre->cur.safe_window_end = pre->cur.height - 2;
> + else
> + pre->cur.safe_window_end = DIV_ROUND_UP(pre->cur.height, 4) - 1;
Could you extract the same code from ipu_pre_configure() into a separate
function, say ipu_pre_configure_modifier(), instead, and call that from
both places?
regards
Philipp
