<div dir="ltr"><div dir="ltr"><div>Hi Emil, </div><div>Thanks for your feedback</div></div> <div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Feb 25, 2020 at 1:35 AM Emil Velikov <<a href="mailto:emil.l.velikov@gmail.com">emil.l.velikov@gmail.com</a>> wrote: </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Fri, 21 Feb 2020 at 11:15, Kevin Tang <<a href="mailto:kevin3.tang@gmail.com" target="_blank">kevin3.tang@gmail.com</a>> wrote: > > Adds DPU(Display Processor Unit) support for the Unisoc's display subsystem. > It's support multi planes, scaler, rotation, PQ(Picture Quality) and more. > > Cc: Orson Zhai <<a href="mailto:orsonzhai@gmail.com" target="_blank">orsonzhai@gmail.com</a>> > Cc: Baolin Wang <<a href="mailto:baolin.wang@linaro.org" target="_blank">baolin.wang@linaro.org</a>> > Cc: Chunyan Zhang <<a href="mailto:zhang.lyra@gmail.com" target="_blank">zhang.lyra@gmail.com</a>> > Signed-off-by: Kevin Tang <<a href="mailto:kevin.tang@unisoc.com" target="_blank">kevin.tang@unisoc.com</a>> > --- > drivers/gpu/drm/sprd/Makefile | 6 +- > drivers/gpu/drm/sprd/disp_lib.c | 59 +++ > drivers/gpu/drm/sprd/disp_lib.h | 21 + > drivers/gpu/drm/sprd/dpu/Makefile | 7 + > drivers/gpu/drm/sprd/dpu/dpu_r2p0.c | 787 ++++++++++++++++++++++++++++++++++++ > drivers/gpu/drm/sprd/sprd_dpu.c | 678 +++++++++++++++++++++++++++++++ > drivers/gpu/drm/sprd/sprd_dpu.h | 130 ++++++ > drivers/gpu/drm/sprd/sprd_drm.c | 1 + > drivers/gpu/drm/sprd/sprd_drm.h | 2 + > 9 files changed, 1690 insertions(+), 1 deletion(-) > create mode 100644 drivers/gpu/drm/sprd/disp_lib.c > create mode 100644 drivers/gpu/drm/sprd/disp_lib.h > create mode 100644 drivers/gpu/drm/sprd/dpu/Makefile > create mode 100644 drivers/gpu/drm/sprd/dpu/dpu_r2p0.c > create mode 100644 drivers/gpu/drm/sprd/sprd_dpu.c > create mode 100644 drivers/gpu/drm/sprd/sprd_dpu.h > > diff --git a/drivers/gpu/drm/sprd/Makefile b/drivers/gpu/drm/sprd/Makefile > index 63b8751..c94c8ac 100644 > --- a/drivers/gpu/drm/sprd/Makefile > +++ b/drivers/gpu/drm/sprd/Makefile > @@ -4,4 +4,8 @@ ccflags-y += -Iinclude/drm > > subdir-ccflags-y += -I$(src) > > -obj-y := sprd_drm.o > +obj-y := sprd_drm.o \ > + sprd_dpu.o > + > +obj-y += disp_lib.o > +obj-y += dpu/ > diff --git a/drivers/gpu/drm/sprd/disp_lib.c b/drivers/gpu/drm/sprd/disp_lib.c > new file mode 100644 > index 0000000..c887822 > +++ b/drivers/gpu/drm/sprd/disp_lib.c > +++ b/drivers/gpu/drm/sprd/disp_lib.h These are completely unused. Drop them for now as well as their Makefile hunk. </blockquote><div>These function will be used in the encoder driver, i commit it with encoder driver. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> > --- /dev/null > +++ b/drivers/gpu/drm/sprd/dpu/Makefile > @@ -0,0 +1,7 @@ > +# SPDX-License-Identifier: GPL-2.0 > + > +ifdef CONFIG_ARM64 > +KBUILD_CFLAGS += -mstrict-align > +endif > + There is only a single case of this in the whole kernel. This is a strong indication that there is something wrong, perhaps. Why do we need this? </blockquote><div>Our HW controller registers(32bit) map to struct "dpu_reg",</div><div>so we can directly access the registers through the struct "dpu_reg", rather than through I/O(readl&writel). </div><div> </div><div>But we found on ARM64, if two adjacent member variables have the same value，</div><div>the gcc compiler maybe optimize them into a 64-bit value when compiling, access io registers with 64bit? </div><div> </div><div>So we add "mstrict-align", do not permit unaligned accesses</div><div>We need to strictly follow the 32bit width to access the struct "dpu_reg", not 64bit. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> > +obj-y += dpu_r2p0.o > diff --git a/drivers/gpu/drm/sprd/dpu/dpu_r2p0.c b/drivers/gpu/drm/sprd/dpu/dpu_r2p0.c > new file mode 100644 > index 0000000..b96e2e2 > --- /dev/null > +++ b/drivers/gpu/drm/sprd/dpu/dpu_r2p0.c > @@ -0,0 +1,787 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (C) 2019 Unisoc Inc. > + */ > + > +#include <linux/delay.h> > +#include <linux/wait.h> > +#include <linux/workqueue.h> > +#include "sprd_dpu.h" > + > +#define DISPC_INT_FBC_PLD_ERR_MASK BIT(8) > +#define DISPC_INT_FBC_HDR_ERR_MASK BIT(9) > + > +#define DISPC_INT_MMU_INV_WR_MASK BIT(19) > +#define DISPC_INT_MMU_INV_RD_MASK BIT(18) > +#define DISPC_INT_MMU_VAOR_WR_MASK BIT(17) > +#define DISPC_INT_MMU_VAOR_RD_MASK BIT(16) > + > +struct layer_reg { > + u32 addr[4]; > + u32 ctrl; > + u32 size; > + u32 pitch; > + u32 pos; > + u32 alpha; > + u32 ck; > + u32 pallete; > + u32 crop_start; > +}; > + > +struct wb_region_reg { > + u32 pos; > + u32 size; > +}; > + > +struct dpu_reg { ... There are actual HW registers and we get the base via ioremap_nocache(). Please add a small comment. </blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> > +static DECLARE_WAIT_QUEUE_HEAD(wait_queue); > +static bool panel_ready = true; > +static bool evt_update; > +static bool evt_stop; > + Using static variables tend to be an example of badly designed code. ... > +static void dpu_uninit(struct dpu_context *ctx) Normally people use init - for initialization and fini for "uninitialization" > +enum { > + DPU_LAYER_FORMAT_YUV422_2PLANE, > + DPU_LAYER_FORMAT_YUV420_2PLANE, > + DPU_LAYER_FORMAT_YUV420_3PLANE, > + DPU_LAYER_FORMAT_ARGB8888, > + DPU_LAYER_FORMAT_RGB565, > + DPU_LAYER_FORMAT_XFBC_ARGB8888 = 8, > + DPU_LAYER_FORMAT_XFBC_RGB565, > + DPU_LAYER_FORMAT_MAX_TYPES, > +}; > + > +enum { > + DPU_LAYER_ROTATION_0, > + DPU_LAYER_ROTATION_90, > + DPU_LAYER_ROTATION_180, > + DPU_LAYER_ROTATION_270, > + DPU_LAYER_ROTATION_0_M, > + DPU_LAYER_ROTATION_90_M, > + DPU_LAYER_ROTATION_180_M, > + DPU_LAYER_ROTATION_270_M, > +}; > + Starting with one format and rotation is good. Others can be added as follow-up. Especially since FBC is something that is just becoming fashionable. </blockquote><div>FBC will be remove later.</div><div>All other options are required for layer format and rotation, these are the basic functions.</div><div>Why need to split it? </div><div></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> > +static void dpu_dpi_init(struct dpu_context *ctx) > +{ ... > + } else if (ctx->if_type == SPRD_DISPC_IF_EDPI) { > + /* use edpi as interface */ > + reg->dpu_cfg0 |= BIT(0); > + > + /* use external te */ > + reg->dpi_ctrl |= BIT(10); > + > + /* enable te */ > + reg->dpi_ctrl |= BIT(8); Try to avoid using BIT() randomly. A well chosen name, removes the need for a comment. Perhaps it's just me but the "enable foo" comments do not help much. ... > +struct dpu_core_ops sharkl3_dpu_core_ops = { In general ops should be const. > + .version = dpu_get_version, > + .init = dpu_init, > + .uninit = dpu_uninit, > + .run = dpu_run, > + .stop = dpu_stop, > + .isr = dpu_isr, > + .ifconfig = dpu_dpi_init, > + .capability = dpu_capability, > + .flip = dpu_flip, > + .bg_color = dpu_bgcolor, > + .enable_vsync = enable_vsync, > + .disable_vsync = disable_vsync, ... then again, we have only a single set of dpu_core_ops. So the whole abstraction layer seems unnessesary. </blockquote><div>Currently we only submitted one crtc(dsi)， the crtc of DP(DisplayPort) will be added as follow-up.</div><div>So I hope to keep the current design</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> > +}; > diff --git a/drivers/gpu/drm/sprd/sprd_dpu.c b/drivers/gpu/drm/sprd/sprd_dpu.c > new file mode 100644 > index 0000000..331f236 > --- /dev/null > +++ b/drivers/gpu/drm/sprd/sprd_dpu.c > +static int sprd_plane_atomic_check(struct drm_plane *plane, > + struct drm_plane_state *state) > +{ > + DRM_DEBUG("%s()\n", __func__); > + This does not feel right. Here the driver must ensure that the state given will always work. Is the hardware so versatile that any permutation (given by userspace) will work? Same goes for the other atomic_check functions that are no-op. </blockquote><div>In fact， plane state check, we put in our HW abstraction layer, include</div><div>layer format, layer addr, rotation... so maybe there is no need to impliment atomic_check</div><div>funtion. I will remove later... </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> > +static void sprd_plane_atomic_disable(struct drm_plane *plane, > + struct drm_plane_state *old_state) > +{ > + struct sprd_plane *p = to_sprd_plane(plane); > + > + /* > + * NOTE: > + * The dpu->core->flip() will disable all the planes each time. > + * So there is no need to impliment the atomic_disable() function. > + * But this function can not be removed, because it will change > + * to call atomic_update() callback instead. Which will cause > + * kernel panic in sprd_plane_atomic_update(). > + * Why do we disable all the planes on flip? This is the first time I see such driver decision. </blockquote>Our layers commit to HW on atomic_flush, not in atomic_update.</div><div class="gmail_quote">We will wait for all layers to be ready, then flip.</div><div class="gmail_quote">So we need to disable all the planes before flip. </div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> > +static void sprd_crtc_atomic_enable(struct drm_crtc *crtc, > + struct drm_crtc_state *old_state) > +{ > + struct sprd_dpu *dpu = crtc_to_dpu(crtc); > + static bool is_enabled = true; > + More static variables - please remove. > +static int sprd_dpu_init(struct sprd_dpu *dpu) > +{ > + struct dpu_context *ctx = &dpu->ctx; > + > + down(&ctx->refresh_lock); > + Why do we need the semaphore? From a quick look all the other drivers are semaphore/lock free in their atomic codepath. One notable exception is the event_lock. </blockquote><div>Because sysfs debug driver, not uploaded yet. </div><div>We will run or stop our display controller on sysfs debug driver, so we need to add semaphore lock to</div><div>protect atomic codepath. I will be remove it later...</div><div> </div><div>Maybe the statemachine "is_inited" is also not needed now. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> > + if (dpu->ctx.is_inited) { > + up(&ctx->refresh_lock); > + return 0; > + } > + How can did this trigger? IMHO we either have a serious bug or the check is dead code. </blockquote><div>Sorry, this will be trigger on our sysfs debug driver, i will 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"> > + if (dpu->core && dpu->core->init) Using if dpu->core && dpu->core->FOO is a recurring pattern, yet they are always set. Unless needed we can kill the abstraction layer all together can call FOO directly. > --- /dev/null > +++ b/drivers/gpu/drm/sprd/sprd_dpu.h > +struct dpu_context { > + unsigned long base; > + const char *version; > + int irq; > + u8 if_type; > + bool is_inited; > + bool is_stopped; Nit: more natural names for these are "initialized" and "stopped" > + struct videomode vm; > + struct semaphore refresh_lock; I've mentioned it earlier - not 100% sure that the semaphore is needed. If it is please add a comment just above it. > +int sprd_dpu_run(struct sprd_dpu *dpu); > +int sprd_dpu_stop(struct sprd_dpu *dpu); > + These two functions seem to be unused in this patch. Let's drop them for now. </blockquote><div>The encoder driver will call it, so commit those with encoder driver? </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> Thanks Emil </blockquote></div></div>