<div dir="ltr"><div>Hi Maxime,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">Maxime Ripard <maxime@cerno.tech> 于2021年3月24日周三 下午7:10写道:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi,<br>
<br>
On Mon, Feb 22, 2021 at 09:28:20PM +0800, Kevin Tang wrote:<br>
> Adds DPU(Display Processor Unit) support for the Unisoc's display subsystem.<br>
> It's support multi planes, scaler, rotation, PQ(Picture Quality) and more.<br>
> <br>
> Cc: Orson Zhai <<a href="mailto:orsonzhai@gmail.com" target="_blank">orsonzhai@gmail.com</a>><br>
> Cc: Chunyan Zhang <<a href="mailto:zhang.lyra@gmail.com" target="_blank">zhang.lyra@gmail.com</a>><br>
> Signed-off-by: Kevin Tang <<a href="mailto:kevin.tang@unisoc.com" target="_blank">kevin.tang@unisoc.com</a>><br>
> <br>
> v2:<br>
>   - Use drm_xxx to replace all DRM_XXX.<br>
>   - Use kzalloc to replace devm_kzalloc for sprd_dpu structure init.<br>
> <br>
> v3:<br>
>   - Remove dpu_layer stuff layer and commit layers by aotmic_update<br>
> <br>
> v4:<br>
>   - Use drmm_helpers to allocate crtc and planes.<br>
>   - Move rotation enum definitions to crtc layer reg bitfields.<br>
>   - Move allocate crtc and planes to bind funtion.<br>
<br>
There's a number of checkpatch warnings, make sure to fix them<br></blockquote><div>

Thks, it's will be fixed on patch v5. 

 </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> ---<br>
>  drivers/gpu/drm/sprd/Kconfig    |   1 +<br>
>  drivers/gpu/drm/sprd/Makefile   |   4 +-<br>
>  drivers/gpu/drm/sprd/sprd_dpu.c | 964 ++++++++++++++++++++++++++++++++<br>
>  drivers/gpu/drm/sprd/sprd_dpu.h | 109 ++++<br>
>  drivers/gpu/drm/sprd/sprd_drm.c |   1 +<br>
>  drivers/gpu/drm/sprd/sprd_drm.h |   2 +<br>
>  6 files changed, 1079 insertions(+), 2 deletions(-)<br>
>  create mode 100644 drivers/gpu/drm/sprd/sprd_dpu.c<br>
>  create mode 100644 drivers/gpu/drm/sprd/sprd_dpu.h<br>
> <br>
> diff --git a/drivers/gpu/drm/sprd/Kconfig b/drivers/gpu/drm/sprd/Kconfig<br>
> index 6e80cc9f3..9b4ef9aea 100644<br>
> --- a/drivers/gpu/drm/sprd/Kconfig<br>
> +++ b/drivers/gpu/drm/sprd/Kconfig<br>
> @@ -3,6 +3,7 @@ config DRM_SPRD<br>
>       depends on ARCH_SPRD || COMPILE_TEST<br>
>       depends on DRM && OF<br>
>       select DRM_KMS_HELPER<br>
> +     select VIDEOMODE_HELPERS<br>
>       select DRM_GEM_CMA_HELPER<br>
>       select DRM_KMS_CMA_HELPER<br>
>       select DRM_MIPI_DSI<br>
> diff --git a/drivers/gpu/drm/sprd/Makefile b/drivers/gpu/drm/sprd/Makefile<br>
> index 86d95d93a..6c25bfa99 100644<br>
> --- a/drivers/gpu/drm/sprd/Makefile<br>
> +++ b/drivers/gpu/drm/sprd/Makefile<br>
> @@ -1,5 +1,5 @@<br>
>  # SPDX-License-Identifier: GPL-2.0<br>
>  <br>
> -subdir-ccflags-y += -I$(srctree)/$(src)<br>
> +obj-y := sprd_drm.o \<br>
> +     sprd_dpu.o<br>
>  <br>
> -obj-y := sprd_drm.o<br>
> diff --git a/drivers/gpu/drm/sprd/sprd_dpu.c b/drivers/gpu/drm/sprd/sprd_dpu.c<br>
> new file mode 100644<br>
> index 000000000..75b7e40d9<br>
> --- /dev/null<br>
> +++ b/drivers/gpu/drm/sprd/sprd_dpu.c<br>
> @@ -0,0 +1,964 @@<br>
> +// SPDX-License-Identifier: GPL-2.0<br>
> +/*<br>
> + * Copyright (C) 2020 Unisoc Inc.<br>
> + */<br>
> +<br>
> +#include <linux/component.h><br>
> +#include <linux/delay.h><br>
> +#include <linux/dma-buf.h><br>
> +#include <linux/io.h><br>
> +#include <linux/module.h><br>
> +#include <linux/of.h><br>
> +#include <linux/of_address.h><br>
> +#include <linux/of_device.h><br>
> +#include <linux/of_irq.h><br>
> +#include <linux/wait.h><br>
> +#include <linux/workqueue.h><br>
> +<br>
> +#include <drm/drm_atomic_helper.h><br>
> +#include <drm/drm_crtc_helper.h><br>
> +#include <drm/drm_fb_cma_helper.h><br>
> +#include <drm/drm_gem_cma_helper.h><br>
> +#include <drm/drm_gem_framebuffer_helper.h><br>
> +#include <drm/drm_plane_helper.h><br>
> +<br>
> +#include "sprd_drm.h"<br>
> +#include "sprd_dpu.h"<br>
> +<br>
> +/* Global control registers */<br>
> +#define REG_DPU_CTRL 0x04<br>
> +#define REG_DPU_CFG0 0x08<br>
> +#define REG_PANEL_SIZE       0x20<br>
> +#define REG_BLEND_SIZE       0x24<br>
> +#define REG_BG_COLOR 0x2C<br>
> +<br>
> +/* Layer0 control registers */<br>
> +#define REG_LAY_BASE_ADDR0   0x30<br>
> +#define REG_LAY_BASE_ADDR1   0x34<br>
> +#define REG_LAY_BASE_ADDR2   0x38<br>
> +#define REG_LAY_CTRL         0x40<br>
> +#define REG_LAY_SIZE         0x44<br>
> +#define REG_LAY_PITCH                0x48<br>
> +#define REG_LAY_POS          0x4C<br>
> +#define REG_LAY_ALPHA                0x50<br>
> +#define REG_LAY_CROP_START   0x5C<br>
> +<br>
> +/* Interrupt control registers */<br>
> +#define REG_DPU_INT_EN               0x1E0<br>
> +#define REG_DPU_INT_CLR              0x1E4<br>
> +#define REG_DPU_INT_STS              0x1E8<br>
> +<br>
> +/* DPI control registers */<br>
> +#define REG_DPI_CTRL         0x1F0<br>
> +#define REG_DPI_H_TIMING     0x1F4<br>
> +#define REG_DPI_V_TIMING     0x1F8<br>
> +<br>
> +/* MMU control registers */<br>
> +#define REG_MMU_EN                   0x800<br>
> +#define REG_MMU_VPN_RANGE            0x80C<br>
> +#define REG_MMU_VAOR_ADDR_RD         0x818<br>
> +#define REG_MMU_VAOR_ADDR_WR         0x81C<br>
> +#define REG_MMU_INV_ADDR_RD          0x820<br>
> +#define REG_MMU_INV_ADDR_WR          0x824<br>
> +#define REG_MMU_PPN1                 0x83C<br>
> +#define REG_MMU_RANGE1                       0x840<br>
> +#define REG_MMU_PPN2                 0x844<br>
> +#define REG_MMU_RANGE2                       0x848<br>
> +<br>
> +/* Global control bits */<br>
> +#define BIT_DPU_RUN                  BIT(0)<br>
> +#define BIT_DPU_STOP                 BIT(1)<br>
> +#define BIT_DPU_REG_UPDATE           BIT(2)<br>
> +#define BIT_DPU_IF_EDPI                      BIT(0)<br>
> +<br>
> +/* Layer control bits */<br>
> +#define BIT_DPU_LAY_EN                               BIT(0)<br>
> +#define BIT_DPU_LAY_LAYER_ALPHA                      (0x01 << 2)<br>
> +#define BIT_DPU_LAY_COMBO_ALPHA                      (0x02 << 2)<br>
> +#define BIT_DPU_LAY_FORMAT_YUV422_2PLANE             (0x00 << 4)<br>
> +#define BIT_DPU_LAY_FORMAT_YUV420_2PLANE             (0x01 << 4)<br>
> +#define BIT_DPU_LAY_FORMAT_YUV420_3PLANE             (0x02 << 4)<br>
> +#define BIT_DPU_LAY_FORMAT_ARGB8888                  (0x03 << 4)<br>
> +#define BIT_DPU_LAY_FORMAT_RGB565                    (0x04 << 4)<br>
> +#define BIT_DPU_LAY_DATA_ENDIAN_B0B1B2B3             (0x00 << 8)<br>
> +#define BIT_DPU_LAY_DATA_ENDIAN_B3B2B1B0             (0x01 << 8)<br>
> +#define BIT_DPU_LAY_NO_SWITCH                        (0x00 << 10)<br>
> +#define BIT_DPU_LAY_RB_OR_UV_SWITCH          (0x01 << 10)<br>
> +#define BIT_DPU_LAY_MODE_BLEND_NORMAL                (0x00 << 16)<br>
> +#define BIT_DPU_LAY_MODE_BLEND_PREMULT               (0x01 << 16)<br>
> +#define BIT_DPU_LAY_ROTATION_0               (0x00 << 20)<br>
> +#define BIT_DPU_LAY_ROTATION_90              (0x01 << 20)<br>
> +#define BIT_DPU_LAY_ROTATION_180     (0x02 << 20)<br>
> +#define BIT_DPU_LAY_ROTATION_270     (0x03 << 20)<br>
> +#define BIT_DPU_LAY_ROTATION_0_M     (0x04 << 20)<br>
> +#define BIT_DPU_LAY_ROTATION_90_M    (0x05 << 20)<br>
> +#define BIT_DPU_LAY_ROTATION_180_M   (0x06 << 20)<br>
> +#define BIT_DPU_LAY_ROTATION_270_M   (0x07 << 20)<br>
> +<br>
> +/* Interrupt control & status bits */<br>
> +#define BIT_DPU_INT_DONE             BIT(0)<br>
> +#define BIT_DPU_INT_TE                       BIT(1)<br>
> +#define BIT_DPU_INT_ERR                      BIT(2)<br>
> +#define BIT_DPU_INT_UPDATE_DONE              BIT(4)<br>
> +#define BIT_DPU_INT_VSYNC            BIT(5)<br>
> +#define BIT_DPU_INT_MMU_VAOR_RD              BIT(16)<br>
> +#define BIT_DPU_INT_MMU_VAOR_WR              BIT(17)<br>
> +#define BIT_DPU_INT_MMU_INV_RD               BIT(18)<br>
> +#define BIT_DPU_INT_MMU_INV_WR               BIT(19)<br>
> +<br>
> +/* DPI control bits */<br>
> +#define BIT_DPU_EDPI_TE_EN           BIT(8)<br>
> +#define BIT_DPU_EDPI_FROM_EXTERNAL_PAD       BIT(10)<br>
> +#define BIT_DPU_DPI_HALT_EN          BIT(16)<br>
> +<br>
> +static const u32 primary_fmts[] = {<br>
> +     DRM_FORMAT_XRGB8888,<br>
> +     DRM_FORMAT_XBGR8888,<br>
> +     DRM_FORMAT_ARGB8888,<br>
> +     DRM_FORMAT_ABGR8888,<br>
> +     DRM_FORMAT_RGBA8888,<br>
> +     DRM_FORMAT_BGRA8888,<br>
> +     DRM_FORMAT_RGBX8888,<br>
> +     DRM_FORMAT_RGB565,<br>
> +     DRM_FORMAT_BGR565,<br>
> +     DRM_FORMAT_NV12,<br>
> +     DRM_FORMAT_NV21,<br>
> +     DRM_FORMAT_NV16,<br>
> +     DRM_FORMAT_NV61,<br>
> +     DRM_FORMAT_YUV420,<br>
> +     DRM_FORMAT_YVU420,<br>
> +};<br>
> +<br>
> +struct sprd_plane {<br>
> +     struct drm_plane base;<br>
> +     u32 index;<br>
> +};<br>
> +<br>
> +static inline struct sprd_plane *to_sprd_plane(struct drm_plane *plane)<br>
> +{<br>
> +     return container_of(plane, struct sprd_plane, base);<br>
> +}<br>
> +<br>
> +static u32 check_mmu_isr(struct sprd_dpu *dpu, u32 reg_val)<br>
> +{<br>
> +     struct dpu_context *ctx = &dpu->ctx;<br>
> +     u32 mmu_mask = BIT_DPU_INT_MMU_VAOR_RD |<br>
> +                     BIT_DPU_INT_MMU_VAOR_WR |<br>
> +                     BIT_DPU_INT_MMU_INV_RD |<br>
> +                     BIT_DPU_INT_MMU_INV_WR;<br>
> +     u32 val = reg_val & mmu_mask;<br>
> +     int i;<br>
> +<br>
> +     if (val) {<br>
> +             drm_err(dpu->drm, "--- iommu interrupt err: 0x%04x ---\n", val);<br>
> +<br>
> +             if (val & BIT_DPU_INT_MMU_INV_RD)<br>
> +                     drm_err(dpu->drm, "iommu invalid read error, addr: 0x%08x\n",<br>
> +                             readl(ctx->base + REG_MMU_INV_ADDR_RD));<br>
> +             if (val & BIT_DPU_INT_MMU_INV_WR)<br>
> +                     drm_err(dpu->drm, "iommu invalid write error, addr: 0x%08x\n",<br>
> +                             readl(ctx->base + REG_MMU_INV_ADDR_WR));<br>
> +             if (val & BIT_DPU_INT_MMU_VAOR_RD)<br>
> +                     drm_err(dpu->drm, "iommu va out of range read error, addr: 0x%08x\n",<br>
> +                             readl(ctx->base + REG_MMU_VAOR_ADDR_RD));<br>
> +             if (val & BIT_DPU_INT_MMU_VAOR_WR)<br>
> +                     drm_err(dpu->drm, "iommu va out of range write error, addr: 0x%08x\n",<br>
> +                             readl(ctx->base + REG_MMU_VAOR_ADDR_WR));<br>
<br>
Is that the IOMMU page fault interrupt? I would expect it to be in the iommu driver.</blockquote><div><span lang="en">Our <span>iommu</span> driver is indeed an separate driver, <span lang="en">and also in upstreaming,</span></span></div><div><span lang="en"><span lang="en">but <span>iommu</span> fault interrupts reporting by display controller, not <span>iommu</span> itself,</span></span></div><div><span lang="en"><span lang="en">if use iommu_set_fault_handler() to hook in our reporting function, <span lang="en">there</span></span></span></div><div><span lang="en"><span lang="en"><span lang="en">must be cross-module access to h/w registers.</span></span></span></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> +             for (i = 0; i < 8; i++) { </blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> +                     reg_val = layer_reg_rd(ctx, REG_LAY_CTRL, i);<br>
> +                     if (reg_val & 0x1)<br>
> +                             drm_info(dpu->drm, "layer%d: 0x%08x 0x%08x 0x%08x ctrl: 0x%08x\n", i,<br>
> +                                     layer_reg_rd(ctx, REG_LAY_BASE_ADDR0, i),<br>
> +                                     layer_reg_rd(ctx, REG_LAY_BASE_ADDR1, i),<br>
> +                                     layer_reg_rd(ctx, REG_LAY_BASE_ADDR2, i),<br>
> +                                     layer_reg_rd(ctx, REG_LAY_CTRL, i));<br>
> +             }<br>
> +     }<br>
> +<br>
> +     return val;<br>
> +}<br>
> +<br>
> +static int dpu_wait_stop_done(struct sprd_dpu *dpu)<br>
> +{<br>
> +     struct dpu_context *ctx = &dpu->ctx;<br>
> +     int rc;<br>
> +<br>
> +     if (ctx->stopped)<br>
> +             return 0;<br>
> +<br>
> +     rc = wait_event_interruptible_timeout(ctx->wait_queue, ctx->evt_stop,<br>
> +                                            msecs_to_jiffies(500));<br>
> +     ctx->evt_stop = false;<br>
> +<br>
> +     ctx->stopped = true;<br>
> +<br>
> +     if (!rc) {<br>
> +             drm_err(dpu->drm, "dpu wait for stop done time out!\n");<br>
> +             return -ETIMEDOUT;<br>
> +     }<br>
> +<br>
> +     return 0;<br>
> +}<br>
> +<br>
> +static int dpu_wait_update_done(struct sprd_dpu *dpu)<br>
> +{<br>
> +     struct dpu_context *ctx = &dpu->ctx;<br>
> +     int rc;<br>
> +<br>
> +     ctx->evt_update = false;<br>
> +<br>
> +     rc = wait_event_interruptible_timeout(ctx->wait_queue, ctx->evt_update,<br>
> +                                            msecs_to_jiffies(500));<br>
> +<br>
> +     if (!rc) {<br>
> +             drm_err(dpu->drm, "dpu wait for reg update done time out!\n");<br>
> +             return -ETIMEDOUT;<br>
> +     }<br>
> +<br>
> +     return 0;<br>
> +}<br>
> +<br>
> +static u32 drm_format_to_dpu(struct drm_framebuffer *fb)<br>
> +{<br>
> +     u32 format = 0;<br>
> +<br>
> +     switch (fb->format->format) {<br>
> +     case DRM_FORMAT_BGRA8888:<br>
> +             /* BGRA8888 -> ARGB8888 */<br>
> +             format |= BIT_DPU_LAY_DATA_ENDIAN_B3B2B1B0;<br>
> +             format |= BIT_DPU_LAY_FORMAT_ARGB8888;<br>
> +             break;<br>
> +     case DRM_FORMAT_RGBX8888:<br>
> +     case DRM_FORMAT_RGBA8888:<br>
> +             /* RGBA8888 -> ABGR8888 */<br>
> +             format |= BIT_DPU_LAY_DATA_ENDIAN_B3B2B1B0;<br>
> +             /* fallthrough */<br>
> +     case DRM_FORMAT_ABGR8888:<br>
> +             /* RB switch */<br>
> +             format |= BIT_DPU_LAY_RB_OR_UV_SWITCH;<br>
> +             /* fallthrough */<br>
> +     case DRM_FORMAT_ARGB8888:<br>
> +             format |= BIT_DPU_LAY_FORMAT_ARGB8888;<br>
> +             break;<br>
> +     case DRM_FORMAT_XBGR8888:<br>
> +             /* RB switch */<br>
> +             format |= BIT_DPU_LAY_RB_OR_UV_SWITCH;<br>
> +             /* fallthrough */<br>
> +     case DRM_FORMAT_XRGB8888:<br>
> +             format |= BIT_DPU_LAY_FORMAT_ARGB8888;<br>
> +             break;<br>
> +     case DRM_FORMAT_BGR565:<br>
> +             /* RB switch */<br>
> +             format |= BIT_DPU_LAY_RB_OR_UV_SWITCH;<br>
> +             /* fallthrough */<br>
> +     case DRM_FORMAT_RGB565:<br>
> +             format |= BIT_DPU_LAY_FORMAT_RGB565;<br>
> +             break;<br>
> +     case DRM_FORMAT_NV12:<br>
> +             /* 2-Lane: Yuv420 */<br>
> +             format |= BIT_DPU_LAY_FORMAT_YUV420_2PLANE;<br>
> +             /* Y endian */<br>
> +             format |= BIT_DPU_LAY_DATA_ENDIAN_B0B1B2B3;<br>
> +             /* UV endian */<br>
> +             format |= BIT_DPU_LAY_NO_SWITCH;<br>
> +             break;<br>
> +     case DRM_FORMAT_NV21:<br>
> +             /* 2-Lane: Yuv420 */<br>
> +             format |= BIT_DPU_LAY_FORMAT_YUV420_2PLANE;<br>
> +             /* Y endian */<br>
> +             format |= BIT_DPU_LAY_DATA_ENDIAN_B0B1B2B3;<br>
> +             /* UV endian */<br>
> +             format |= BIT_DPU_LAY_RB_OR_UV_SWITCH;<br>
> +             break;<br>
> +     case DRM_FORMAT_NV16:<br>
> +             /* 2-Lane: Yuv422 */<br>
> +             format |= BIT_DPU_LAY_FORMAT_YUV422_2PLANE;<br>
> +             /* Y endian */<br>
> +             format |= BIT_DPU_LAY_DATA_ENDIAN_B3B2B1B0;<br>
> +             /* UV endian */<br>
> +             format |= BIT_DPU_LAY_RB_OR_UV_SWITCH;<br>
> +             break;<br>
> +     case DRM_FORMAT_NV61:<br>
> +             /* 2-Lane: Yuv422 */<br>
> +             format |= BIT_DPU_LAY_FORMAT_YUV422_2PLANE;<br>
> +             /* Y endian */<br>
> +             format |= BIT_DPU_LAY_DATA_ENDIAN_B0B1B2B3;<br>
> +             /* UV endian */<br>
> +             format |= BIT_DPU_LAY_NO_SWITCH;<br>
> +             break;<br>
> +     case DRM_FORMAT_YUV420:<br>
> +             format |= BIT_DPU_LAY_FORMAT_YUV420_3PLANE;<br>
> +             /* Y endian */<br>
> +             format |= BIT_DPU_LAY_DATA_ENDIAN_B0B1B2B3;<br>
> +             /* UV endian */<br>
> +             format |= BIT_DPU_LAY_NO_SWITCH;<br>
> +             break;<br>
> +     case DRM_FORMAT_YVU420:<br>
> +             format |= BIT_DPU_LAY_FORMAT_YUV420_3PLANE;<br>
> +             /* Y endian */<br>
> +             format |= BIT_DPU_LAY_DATA_ENDIAN_B0B1B2B3;<br>
> +             /* UV endian */<br>
> +             format |= BIT_DPU_LAY_RB_OR_UV_SWITCH;<br>
> +             break;<br>
> +     default:<br>
> +             break;<br>
> +     }<br>
> +<br>
> +     return format;<br>
> +}<br>
> +<br>
> +static u32 drm_rotation_to_dpu(struct drm_plane_state *state)<br>
> +{<br>
> +     u32 rotation = 0;<br>
> +<br>
> +     switch (state->rotation) {<br>
> +     default:<br>
> +     case DRM_MODE_ROTATE_0:<br>
> +             rotation = BIT_DPU_LAY_ROTATION_0;<br>
> +             break;<br>
> +     case DRM_MODE_ROTATE_90:<br>
> +             rotation = BIT_DPU_LAY_ROTATION_90;<br>
> +             break;<br>
> +     case DRM_MODE_ROTATE_180:<br>
> +             rotation = BIT_DPU_LAY_ROTATION_180;<br>
> +             break;<br>
> +     case DRM_MODE_ROTATE_270:<br>
> +             rotation = BIT_DPU_LAY_ROTATION_270;<br>
> +             break;<br>
> +     case DRM_MODE_REFLECT_Y:<br>
> +             rotation = BIT_DPU_LAY_ROTATION_180_M;<br>
> +             break;<br>
> +     case (DRM_MODE_REFLECT_Y | DRM_MODE_ROTATE_90):<br>
> +             rotation = BIT_DPU_LAY_ROTATION_90_M;<br>
> +             break;<br>
> +     case DRM_MODE_REFLECT_X:<br>
> +             rotation = BIT_DPU_LAY_ROTATION_0_M;<br>
> +             break;<br>
> +     case (DRM_MODE_REFLECT_X | DRM_MODE_ROTATE_90):<br>
> +             rotation = BIT_DPU_LAY_ROTATION_270_M;<br>
> +             break;<br>
> +     }<br>
> +<br>
> +     return rotation;<br>
> +}<br>
> +<br>
> +static u32 drm_blend_to_dpu(struct drm_plane_state *state)<br>
> +{<br>
> +     u32 blend = 0;<br>
> +<br>
> +     switch (state->pixel_blend_mode) {<br>
> +     case DRM_MODE_BLEND_COVERAGE:<br>
> +             /* alpha mode select - combo alpha */<br>
> +             blend |= BIT_DPU_LAY_COMBO_ALPHA;<br>
> +             /* Normal mode */<br>
> +             blend |= BIT_DPU_LAY_MODE_BLEND_NORMAL;<br>
> +             break;<br>
> +     case DRM_MODE_BLEND_PREMULTI:<br>
> +             /* alpha mode select - combo alpha */<br>
> +             blend |= BIT_DPU_LAY_COMBO_ALPHA;<br>
> +             /* Pre-mult mode */<br>
> +             blend |= BIT_DPU_LAY_MODE_BLEND_PREMULT;<br>
> +             break;<br>
> +     case DRM_MODE_BLEND_PIXEL_NONE:<br>
> +     default:<br>
> +             /* don't do blending, maybe RGBX */<br>
> +             /* alpha mode select - layer alpha */<br>
> +             blend |= BIT_DPU_LAY_LAYER_ALPHA;<br>
> +             break;<br>
> +     }<br>
> +<br>
> +     return blend;<br>
> +}<br>
> +<br>
> +static void sprd_dpu_layer(struct sprd_dpu *dpu, struct sprd_plane *plane,<br>
> +                     struct drm_plane_state *state)<br>
> +{<br>
> +     struct dpu_context *ctx = &dpu->ctx;<br>
> +     struct drm_gem_cma_object *cma_obj;<br>
> +     const struct drm_format_info *info;<br>
> +     struct drm_framebuffer *fb = state->fb;<br>
> +     u32 addr, size, offset, pitch, blend, format, rotation;<br>
> +     u32 src_x = state->src_x >> 16;<br>
> +     u32 src_y = state->src_y >> 16;<br>
> +     u32 src_w = state->src_w >> 16;<br>
> +     u32 src_h = state->src_h >> 16;<br>
> +     u32 dst_x = state->crtc_x;<br>
> +     u32 dst_y = state->crtc_y;<br>
> +     u32 alpha = state->alpha;<br>
> +     int i;<br>
> +<br>
> +     offset = (dst_x & 0xffff) | (dst_y << 16);<br>
> +     size = (src_w & 0xffff) | (src_h << 16);<br>
> +<br>
> +     for (i = 0; i < fb->format->num_planes; i++) {<br>
> +             cma_obj = drm_fb_cma_get_gem_obj(fb, i);<br>
> +             addr = cma_obj->paddr + fb->offsets[i];<br>
> +<br>
> +             if (i == 0)<br>
> +                     layer_reg_wr(ctx, REG_LAY_BASE_ADDR0, addr, plane->index);<br>
> +             else if (i == 1)<br>
> +                     layer_reg_wr(ctx, REG_LAY_BASE_ADDR1, addr, plane->index);<br>
> +             else<br>
> +                     layer_reg_wr(ctx, REG_LAY_BASE_ADDR2, addr, plane->index);<br>
> +     }<br>
> +<br>
> +     info = drm_format_info(fb->format->format);<br>
> +     if (fb->format->num_planes == 3) {<br>
> +             /* UV pitch is 1/2 of Y pitch */<br>
> +             pitch = (fb->pitches[0] / info->cpp[0]) |<br>
> +                             (fb->pitches[0] / info->cpp[0] << 15);<br>
> +     } else {<br>
> +             pitch = fb->pitches[0] / info->cpp[0];<br>
> +     }<br>
> +<br>
> +     layer_reg_wr(ctx, REG_LAY_POS, offset, plane->index);<br>
> +     layer_reg_wr(ctx, REG_LAY_SIZE, size, plane->index);<br>
> +     layer_reg_wr(ctx, REG_LAY_CROP_START,<br>
> +                     src_y << 16 | src_x, plane->index);<br>
> +     layer_reg_wr(ctx, REG_LAY_ALPHA, alpha, plane->index);<br>
> +     layer_reg_wr(ctx, REG_LAY_PITCH, pitch, plane->index);<br>
> +<br>
> +     format = drm_format_to_dpu(fb);<br>
> +     blend = drm_blend_to_dpu(state);<br>
> +     rotation = drm_rotation_to_dpu(state);<br>
> +<br>
> +     layer_reg_wr(ctx, REG_LAY_CTRL, BIT_DPU_LAY_EN |<br>
> +                             format |<br>
> +                             blend |<br>
> +                             rotation,<br>
> +                             plane->index);<br>
> +}<br>
> +<br>
> +static void sprd_dpu_flip(struct sprd_dpu *dpu)<br>
> +{<br>
> +     struct dpu_context *ctx = &dpu->ctx;<br>
> +     u32 reg_val;<br>
> +<br>
> +     /*<br>
> +      * Make sure the dpu is in stop status. DPU has no shadow<br>
> +      * registers in EDPI mode. So the config registers can only be<br>
> +      * updated in the rising edge of DPU_RUN bit.<br>
> +      */<br>
> +     if (ctx->if_type == SPRD_DPU_IF_EDPI)<br>
> +             dpu_wait_stop_done(dpu);<br>
> +<br>
> +     /* update trigger and wait */<br>
> +     if (ctx->if_type == SPRD_DPU_IF_DPI) {<br>
> +             if (!ctx->stopped) {<br>
> +                     dpu_reg_set(ctx, REG_DPU_CTRL, BIT_DPU_REG_UPDATE);<br>
> +                     dpu_wait_update_done(dpu);<br>
> +             }<br>
> +<br>
> +             dpu_reg_set(ctx, REG_DPU_INT_EN, BIT_DPU_INT_ERR);<br>
> +     } else if (ctx->if_type == SPRD_DPU_IF_EDPI) {<br>
> +             dpu_reg_set(ctx, REG_DPU_CTRL, BIT_DPU_RUN);<br>
> +<br>
> +             ctx->stopped = false;<br>
> +     }<br>
> +<br>
> +     /*<br>
> +      * If the following interrupt was disabled in isr,<br>
> +      * re-enable it.<br>
> +      */<br>
> +     reg_val = BIT_DPU_INT_MMU_VAOR_RD |<br>
> +               BIT_DPU_INT_MMU_VAOR_WR |<br>
> +               BIT_DPU_INT_MMU_INV_RD |<br>
> +               BIT_DPU_INT_MMU_INV_WR;<br>
> +     dpu_reg_set(ctx, REG_DPU_INT_EN, reg_val);<br>
> +}<br>
> +<br>
> +static void sprd_dpu_init(struct sprd_dpu *dpu)<br>
> +{<br>
> +     struct dpu_context *ctx = &dpu->ctx;<br>
> +     u32 size;<br>
> +<br>
> +     writel(0x00, ctx->base + REG_BG_COLOR);<br>
> +<br>
> +     size = (ctx->vm.vactive << 16) | ctx->vm.hactive;<br>
> +     writel(size, ctx->base + REG_PANEL_SIZE);<br>
> +     writel(size, ctx->base + REG_BLEND_SIZE);<br>
> +<br>
> +     writel(0x00, ctx->base + REG_MMU_EN);<br>
> +     writel(0x00, ctx->base + REG_MMU_PPN1);<br>
> +     writel(0xffff, ctx->base + REG_MMU_RANGE1);<br>
> +     writel(0x00, ctx->base + REG_MMU_PPN2);<br>
> +     writel(0xffff, ctx->base + REG_MMU_RANGE2);<br>
> +     writel(0x1ffff, ctx->base + REG_MMU_VPN_RANGE);<br>
> +}<br>
> +<br>
> +static void sprd_dpu_fini(struct sprd_dpu *dpu)<br>
> +{<br>
> +     struct dpu_context *ctx = &dpu->ctx;<br>
> +<br>
> +     writel(0x00, ctx->base + REG_DPU_INT_EN);<br>
> +     writel(0xff, ctx->base + REG_DPU_INT_CLR);<br>
> +}<br>
> +<br>
> +static void sprd_dpi_init(struct sprd_dpu *dpu)<br>
> +{<br>
> +     struct dpu_context *ctx = &dpu->ctx;<br>
> +     u32 int_mask = 0;<br>
> +     u32 reg_val;<br>
> +<br>
> +     if (ctx->if_type == SPRD_DPU_IF_DPI) {<br>
> +             /* use dpi as interface */<br>
> +             dpu_reg_clr(ctx, REG_DPU_CFG0, BIT_DPU_IF_EDPI);<br>
> +             /* disable Halt function for SPRD DSI */<br>
> +             dpu_reg_clr(ctx, REG_DPI_CTRL, BIT_DPU_DPI_HALT_EN);<br>
> +             /* select te from external pad */<br>
> +             dpu_reg_set(ctx, REG_DPI_CTRL, BIT_DPU_EDPI_FROM_EXTERNAL_PAD);<br>
> +<br>
> +             /* set dpi timing */<br>
> +             reg_val = ctx->vm.hsync_len << 0 |<br>
> +                       ctx->vm.hback_porch << 8 |<br>
> +                       ctx->vm.hfront_porch << 20;<br>
> +             writel(reg_val, ctx->base + REG_DPI_H_TIMING);<br>
> +<br>
> +             reg_val = ctx->vm.vsync_len << 0 |<br>
> +                       ctx->vm.vback_porch << 8 |<br>
> +                       ctx->vm.vfront_porch << 20;<br>
> +             writel(reg_val, ctx->base + REG_DPI_V_TIMING);<br>
> +<br>
> +             if (ctx->vm.vsync_len + ctx->vm.vback_porch < 32)<br>
> +                     drm_warn(dpu->drm, "Warning: (vsync + vbp) < 32, "<br>
> +                             "underflow risk!\n");<br>
<br>
I don't think a warning is appropriate here. Maybe we should just<br>
outright reject any mode that uses it?<br></blockquote><div> This issue has been fixed on the new soc, maybe I should remove it.</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> +             /* enable dpu update done INT */<br>
> +             int_mask |= BIT_DPU_INT_UPDATE_DONE;<br>
> +             /* enable dpu done INT */<br>
> +             int_mask |= BIT_DPU_INT_DONE;<br>
> +             /* enable dpu dpi vsync */<br>
> +             int_mask |= BIT_DPU_INT_VSYNC;<br>
> +             /* enable dpu TE INT */<br>
> +             int_mask |= BIT_DPU_INT_TE;<br>
> +             /* enable underflow err INT */<br>
> +             int_mask |= BIT_DPU_INT_ERR;<br>
> +     } else if (ctx->if_type == SPRD_DPU_IF_EDPI) {<br>
> +             /* use edpi as interface */<br>
> +             dpu_reg_set(ctx, REG_DPU_CFG0, BIT_DPU_IF_EDPI);<br>
> +             /* use external te */<br>
> +             dpu_reg_set(ctx, REG_DPI_CTRL, BIT_DPU_EDPI_FROM_EXTERNAL_PAD);<br>
> +             /* enable te */<br>
> +             dpu_reg_set(ctx, REG_DPI_CTRL, BIT_DPU_EDPI_TE_EN);<br>
> +<br>
> +             /* enable stop done INT */<br>
> +             int_mask |= BIT_DPU_INT_DONE;<br>
> +             /* enable TE INT */<br>
> +             int_mask |= BIT_DPU_INT_TE;<br>
> +     }<br>
> +<br>
> +     /* enable iommu va out of range read error INT */<br>
> +     int_mask |= BIT_DPU_INT_MMU_VAOR_RD;<br>
> +     /* enable iommu va out of range write error INT */<br>
> +     int_mask |= BIT_DPU_INT_MMU_VAOR_WR;<br>
> +     /* enable iommu invalid read error INT */<br>
> +     int_mask |= BIT_DPU_INT_MMU_INV_RD;<br>
> +     /* enable iommu invalid write error INT */<br>
> +     int_mask |= BIT_DPU_INT_MMU_INV_WR;<br>
> +<br>
> +     writel(int_mask, ctx->base + REG_DPU_INT_EN);<br>
> +}<br>
> +<br>
> +void sprd_dpu_run(struct sprd_dpu *dpu)<br>
> +{<br>
> +     struct dpu_context *ctx = &dpu->ctx;<br>
> +<br>
> +     dpu_reg_set(ctx, REG_DPU_CTRL, BIT_DPU_RUN);<br>
> +<br>
> +     ctx->stopped = false;<br>
> +}<br>
> +<br>
> +void sprd_dpu_stop(struct sprd_dpu *dpu)<br>
> +{<br>
> +     struct dpu_context *ctx = &dpu->ctx;<br>
> +<br>
> +     if (ctx->if_type == SPRD_DPU_IF_DPI)<br>
> +             dpu_reg_set(ctx, REG_DPU_CTRL, BIT_DPU_STOP);<br>
> +<br>
> +     dpu_wait_stop_done(dpu);<br>
> +}<br>
> +<br>
> +static int sprd_plane_atomic_check(struct drm_plane *plane,<br>
> +                               struct drm_plane_state *state)<br>
> +{<br>
> +     struct drm_framebuffer *fb = state->fb;<br>
> +     struct drm_crtc_state *crtc_state;<br>
> +     u32 fmt;<br>
> +<br>
> +     if (!fb || !state->crtc)<br>
> +             return 0;<br>
> +<br>
> +     fmt = drm_format_to_dpu(fb);<br>
> +     if (!fmt)<br>
> +             return -EINVAL;<br>
> +<br>
> +     crtc_state = drm_atomic_get_crtc_state(state->state, state->crtc);<br>
> +     if (IS_ERR(crtc_state))<br>
> +             return PTR_ERR(crtc_state);<br>
> +<br>
> +     return drm_atomic_helper_check_plane_state(state, crtc_state,<br>
> +                                               DRM_PLANE_HELPER_NO_SCALING,<br>
> +                                               DRM_PLANE_HELPER_NO_SCALING,<br>
> +                                               true, true);<br>
> +}<br>
> +<br>
> +static void sprd_plane_atomic_update(struct drm_plane *drm_plane,<br>
> +                                 struct drm_plane_state *old_state)<br>
> +{<br>
> +     struct drm_plane_state *state = drm_plane->state;<br>
> +     struct sprd_plane *plane = to_sprd_plane(drm_plane);<br>
> +     struct sprd_dpu *dpu = to_sprd_crtc(state->crtc);<br>
> +<br>
> +     /* start configure dpu layers */<br>
> +     sprd_dpu_layer(dpu, plane, state);<br>
> +}<br>
> +<br>
> +static void sprd_plane_atomic_disable(struct drm_plane *drm_plane,<br>
> +                                  struct drm_plane_state *old_state)<br>
> +{<br>
> +     struct sprd_plane *plane = to_sprd_plane(drm_plane);<br>
> +     struct sprd_dpu *dpu = to_sprd_crtc(old_state->crtc);<br>
> +<br>
> +     layer_reg_wr(&dpu->ctx, REG_LAY_CTRL, 0x00, plane->index);<br>
> +}<br>
> +<br>
> +static void sprd_plane_create_properties(struct sprd_plane *plane, int index)<br>
> +{<br>
> +     unsigned int supported_modes = BIT(DRM_MODE_BLEND_PIXEL_NONE) |<br>
> +                                    BIT(DRM_MODE_BLEND_PREMULTI) |<br>
> +                                    BIT(DRM_MODE_BLEND_COVERAGE);<br>
> +<br>
> +     /* create rotation property */<br>
> +     drm_plane_create_rotation_property(&plane->base,<br>
> +                                        DRM_MODE_ROTATE_0,<br>
> +                                        DRM_MODE_ROTATE_MASK |<br>
> +                                        DRM_MODE_REFLECT_MASK);<br>
> +<br>
> +     /* create alpha property */<br>
> +     drm_plane_create_alpha_property(&plane->base);<br>
> +<br>
> +     /* create blend mode property */<br>
> +     drm_plane_create_blend_mode_property(&plane->base, supported_modes);<br>
> +<br>
> +     /* create zpos property */<br>
> +     drm_plane_create_zpos_immutable_property(&plane->base, index);<br>
> +}<br>
> +<br>
> +static const struct drm_plane_helper_funcs sprd_plane_helper_funcs = {<br>
> +     .atomic_check = sprd_plane_atomic_check,<br>
> +     .atomic_update = sprd_plane_atomic_update,<br>
> +     .atomic_disable = sprd_plane_atomic_disable,<br>
> +};<br>
> +<br>
> +static const struct drm_plane_funcs sprd_plane_funcs = {<br>
> +     .update_plane = drm_atomic_helper_update_plane,<br>
> +     .disable_plane  = drm_atomic_helper_disable_plane,<br>
> +     .destroy = drm_plane_cleanup,<br>
> +     .reset = drm_atomic_helper_plane_reset,<br>
> +     .atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,<br>
> +     .atomic_destroy_state = drm_atomic_helper_plane_destroy_state,<br>
> +};<br>
> +<br>
> +static struct sprd_plane *sprd_plane_init(struct drm_device *drm)<br>
> +{<br>
> +     struct sprd_plane *plane, *primary;<br>
> +     int i;<br>
> +<br>
> +     for (i = 0; i < 6; i++) {<br>
> +             plane = drmm_universal_plane_alloc(drm, struct sprd_plane, base,<br>
> +                                            1, &sprd_plane_funcs,<br>
> +                                            primary_fmts, ARRAY_SIZE(primary_fmts),<br>
> +                                            NULL, DRM_PLANE_TYPE_PRIMARY, NULL);<br>
> +             if (IS_ERR(plane)) {<br>
> +                     drm_err(drm, "fail to init primary plane\n");<br>
> +                     return plane;<br>
> +             }<br>
> +<br>
> +             drm_plane_helper_add(&plane->base, &sprd_plane_helper_funcs);<br>
> +<br>
> +             sprd_plane_create_properties(plane, i);<br>
> +<br>
> +             plane->index = i;<br>
> +             if (i == 0)<br>
> +                     primary = plane;<br>
> +     }<br>
> +<br>
> +     return primary;<br>
> +}<br>
> +<br>
> +static enum drm_mode_status sprd_crtc_mode_valid(struct drm_crtc *crtc,<br>
> +                                     const struct drm_display_mode *mode)<br>
> +{<br>
> +     struct sprd_dpu *dpu = to_sprd_crtc(crtc);<br>
> +<br>
> +     drm_dbg(dpu->drm, "%s() mode: "DRM_MODE_FMT"\n", __func__, DRM_MODE_ARG(mode));<br>
> +<br>
> +     if (mode->type & DRM_MODE_TYPE_PREFERRED) {<br>
> +             drm_display_mode_to_videomode(mode, &dpu->ctx.vm);<br>
<br>
You don't seem to use that anywhere else? And that's a bit fragile,<br>
nothing really guarantees that it's the mode you're going to use, and<br>
even then it can be adjusted.<br></blockquote><div> drm_mode convert to video_mode is been use in "sprd_dpu_init" and "sprd_dpi_init "</div><div> Preferred mode should be fixed mode, we generally don’t adjust it.</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> +<br>
> +             if ((mode->hdisplay == mode->htotal) ||<br>
> +                 (mode->vdisplay == mode->vtotal))<br>
> +                     dpu->ctx.if_type = SPRD_DPU_IF_EDPI;<br>
> +             else<br>
> +                     dpu->ctx.if_type = SPRD_DPU_IF_DPI;<br>
<br>
>From an API PoV, you would want that to be in atomic_check. However, I'm<br>
not even sure what it's doing in the first place?<br></blockquote><div>dpi interface mode: DPI(dsi video mode panel) and <span>EDPI</span>(dsi cmd mode panel)</div><div>dpi interface mode has been used on crtc atomic_enable foo, so we need check dpi interface</div><div>mode before atomic_enable. </div><div><br></div><div>Must be put it in atomic_check? Here is the dpi interface mode selection, maybe here is better?<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> +     }<br>
> +<br>
> +     return MODE_OK;<br>
> +}<br>
> +<br>
> +static void sprd_crtc_atomic_enable(struct drm_crtc *crtc,<br>
> +                                struct drm_atomic_state *state)<br>
> +{<br>
> +     struct sprd_dpu *dpu = to_sprd_crtc(crtc);<br>
> +<br>
> +     sprd_dpu_init(dpu);<br>
> +<br>
> +     sprd_dpi_init(dpu);<br>
> +<br>
> +     enable_irq(dpu->ctx.irq);<br>
<br>
Shouldn't this be in enable_vblank? And I would assume that you would<br>
have the interrupts enabled all the time, but disabled in your device?<br></blockquote><div>It seems better to put in enable_vblank, i will try and test it... Thks </div><div><br></div><div>  And I would assume that you would<br>
have the interrupts enabled all the time, but disabled in your device?</div><div>[kevin]I don’t quite understand this, can you help me explain it in detail?  </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> +     drm_crtc_vblank_on(&dpu->base);<br>
> +}<br>
> +<br>
> +static void sprd_crtc_atomic_disable(struct drm_crtc *crtc,<br>
> +                                 struct drm_atomic_state *state)<br>
> +{<br>
> +     struct sprd_dpu *dpu = to_sprd_crtc(crtc);<br>
> +     struct drm_device *drm = dpu-><a href="http://base.dev" rel="noreferrer" target="_blank">base.dev</a>;<br>
> +<br>
> +     drm_crtc_vblank_off(&dpu->base);<br>
> +<br>
> +     disable_irq(dpu->ctx.irq);<br>
> +<br>
> +     sprd_dpu_fini(dpu);<br>
> +<br>
> +     spin_lock_irq(&drm->event_lock);<br>
> +     if (crtc->state->event) {<br>
> +             drm_crtc_send_vblank_event(crtc, crtc->state->event);<br>
> +             crtc->state->event = NULL;<br>
> +     }<br>
> +     spin_unlock_irq(&drm->event_lock);<br>
> +}<br>
> +<br>
> +static int sprd_crtc_atomic_check(struct drm_crtc *crtc,<br>
> +                              struct drm_atomic_state *state)<br>
> +{<br>
> +     drm_dbg(crtc->dev, "%s()\n", __func__);<br>
> +<br>
> +     return 0;<br>
> +}<br>
> +<br>
> +static void sprd_crtc_atomic_flush(struct drm_crtc *crtc,<br>
> +                               struct drm_atomic_state *state)<br>
> +<br>
> +{<br>
> +     struct sprd_dpu *dpu = to_sprd_crtc(crtc);<br>
> +     struct drm_device *drm = dpu-><a href="http://base.dev" rel="noreferrer" target="_blank">base.dev</a>;<br>
> +<br>
> +     sprd_dpu_flip(dpu);<br>
> +<br>
> +     spin_lock_irq(&drm->event_lock);<br>
> +     if (crtc->state->event) {<br>
> +             drm_crtc_send_vblank_event(crtc, crtc->state->event);<br>
> +             crtc->state->event = NULL;<br>
> +     }<br>
> +     spin_unlock_irq(&drm->event_lock);<br>
> +}<br>
> +<br>
> +static int sprd_crtc_enable_vblank(struct drm_crtc *crtc)<br>
> +{<br>
> +     struct sprd_dpu *dpu = to_sprd_crtc(crtc);<br>
> +<br>
> +     dpu_reg_set(&dpu->ctx, REG_DPU_INT_EN, BIT_DPU_INT_VSYNC);<br>
> +<br>
> +     return 0;<br>
> +}<br>
> +<br>
> +static void sprd_crtc_disable_vblank(struct drm_crtc *crtc)<br>
> +{<br>
> +     struct sprd_dpu *dpu = to_sprd_crtc(crtc);<br>
> +<br>
> +     dpu_reg_clr(&dpu->ctx, REG_DPU_INT_EN, BIT_DPU_INT_VSYNC);<br>
> +}<br>
> +<br>
> +static const struct drm_crtc_helper_funcs sprd_crtc_helper_funcs = {<br>
> +     .mode_valid     = sprd_crtc_mode_valid,<br>
> +     .atomic_check   = sprd_crtc_atomic_check,<br>
> +     .atomic_flush   = sprd_crtc_atomic_flush,<br>
> +     .atomic_enable  = sprd_crtc_atomic_enable,<br>
> +     .atomic_disable = sprd_crtc_atomic_disable,<br>
> +};<br>
> +<br>
> +static const struct drm_crtc_funcs sprd_crtc_funcs = {<br>
> +     .destroy        = drm_crtc_cleanup,<br>
> +     .set_config     = drm_atomic_helper_set_config,<br>
> +     .page_flip      = drm_atomic_helper_page_flip,<br>
> +     .reset          = drm_atomic_helper_crtc_reset,<br>
> +     .atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,<br>
> +     .atomic_destroy_state   = drm_atomic_helper_crtc_destroy_state,<br>
> +     .enable_vblank  = sprd_crtc_enable_vblank,<br>
> +     .disable_vblank = sprd_crtc_disable_vblank,<br>
> +};<br>
> +<br>
> +static struct sprd_dpu *sprd_crtc_init(struct drm_device *drm,<br>
> +                      struct drm_plane *primary)<br>
> +{<br>
> +     struct device_node *port;<br>
> +     struct sprd_dpu *dpu;<br>
> +<br>
> +     /*<br>
> +      * set crtc port so that drm_of_find_possible_crtcs call works<br>
> +      */<br>
> +     port = of_parse_phandle(drm->dev->of_node, "ports", 0);<br>
> +     if (!port) {<br>
> +             drm_err(drm, "find 'ports' phandle of %s failed\n",<br>
> +                       drm->dev->of_node->full_name);<br>
> +             return ERR_PTR(-EINVAL);<br>
> +     }<br>
> +     of_node_put(port);<br>
<br>
The YAML binding should already make sure that your binding is sane, and<br>
if you still get a DT that doesn't follow it, you have a whole lot of<br>
other issues than whether ports is there :)<br>
<br>
> +     dpu = drmm_crtc_alloc_with_planes(drm, struct sprd_dpu, base,<br>
> +                                     primary, NULL,<br>
> +                                     &sprd_crtc_funcs, NULL);<br>
> +     if (IS_ERR(dpu)) {<br>
> +             drm_err(drm, "failed to init crtc.\n");<br>
> +             return dpu;<br>
> +     }<br>
> +<br>
> +     dpu->base.port = port;<br>
<br>
But you're still referencing it here, while you called of_node_put on it<br>
already? You should only call it once you're done with it.<br>
<br>
I'm not really sure why you would need drm_of_find_possible_crtcs to<br>
work then if you don't follow the OF-Graph bindings.</blockquote><div>My reference here is:</div><div><a href="https://elixir.bootlin.com/linux/v4.14.228/source/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c#L593" target="_blank">https://elixir.bootlin.com/linux/v4.14.228/source/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c#L593</a></div><div><br></div><div>It’s been a long time, I don’t remember the specific reason, i will recheck it.<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Maxime<br>
</blockquote></div></div>