Hi Emil,
Thanks for your feedback
On Tue, Feb 25, 2020 at 1:35 AM Emil Velikov <emil.l.veli...@gmail.com>
wrote:
> On Fri, 21 Feb 2020 at 11:15, Kevin Tang <kevin3.t...@gmail.com> 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 <orsonz...@gmail.com>
> > Cc: Baolin Wang <baolin.w...@linaro.org>
> > Cc: Chunyan Zhang <zhang.l...@gmail.com>
> > Signed-off-by: Kevin Tang <kevin.t...@unisoc.com>
> > ---
> > 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.
>
These function will be used in the encoder driver, i commit it with encoder
driver.
>
>
> > --- /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?
>
Our HW controller registers(32bit) map to struct "dpu_reg",
so we can directly access the registers through the struct "dpu_reg",
rather than through I/O(readl&writel).
But we found on ARM64, if two adjacent member variables have the same value，
the gcc compiler maybe optimize them into a 64-bit value when compiling, access
io registers with 64bit?
So we add "mstrict-align", do not permit unaligned accesses
We need to strictly follow the 32bit width to access the struct "dpu_reg",
not 64bit.
>
>
> > +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.
>
>
> > +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.
>
FBC will be remove later.
All other options are required for layer format and rotation, these are the
basic functions.
Why need to split it?
>
> > +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.
>
Currently we only submitted one crtc(dsi)， the crtc of DP(DisplayPort) will
be added as follow-up.
So I hope to keep the current design
>
>
>
>
> > +};
> > 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.
>
In fact， plane state check, we put in our HW abstraction layer, include
layer format, layer addr, rotation... so maybe there is no need to
impliment atomic_check
funtion. I will remove later...
>
>
> > +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.
>
Our layers commit to HW on atomic_flush, not in atomic_update.
We will wait for all layers to be ready, then flip.
So we need to disable all the planes before flip.
>
>
> > +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.
>
Because sysfs debug driver, not uploaded yet.
We will run or stop our display controller on sysfs debug driver, so we
need to add semaphore lock to
protect atomic codepath. I will be remove it later...
Maybe the statemachine "is_inited" is also not needed now.
>
>
>
> > + 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.
>
Sorry, this will be trigger on our sysfs debug driver, i will remove it.
>
> > + 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.
>
The encoder driver will call it, so commit those with encoder driver?
>
>
> Thanks
> Emil
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
