Re: [PATCH RFC v6 4/6] drm/sprd: add Unisoc's drm display controller driver
Hi Sam, Here is feedback of RFC V7 Sam Ravnborg 于2020年7月29日周三 上午5:14写道： > > Hi Kevin. > > Some feedback in the following. > I lost track of thing for the atomic modesettting stuff and I hope other > will review that. > > Sam > > On Tue, Jul 28, 2020 at 06:07:57PM +0800, Kevin Tang wrote: > > From: Kevin Tang > > > > Adds DPU(Display Processor Unit) support for the Unisoc's display subsystem. > > It's support multi planes, scaler, rotation, PQ(Picture Quality) and more. > > > > RFC v6: > > - Access registers via readl/writel > > - Checking for unsupported KMS properties (format, rotation, blend_mode, > > etc) on plane_check ops > > - Remove always true checks for dpu core ops > > > > Cc: Orson Zhai > > Cc: Chunyan Zhang > > Signed-off-by: Kevin Tang > > --- > > drivers/gpu/drm/sprd/Makefile | 5 +- > > drivers/gpu/drm/sprd/dpu/Makefile | 3 + > > drivers/gpu/drm/sprd/dpu/dpu_r2p0.c | 503 > > drivers/gpu/drm/sprd/sprd_dpu.c | 646 > > > > drivers/gpu/drm/sprd/sprd_dpu.h | 187 +++ > > drivers/gpu/drm/sprd/sprd_drm.c | 1 + > > drivers/gpu/drm/sprd/sprd_drm.h | 2 + > > 7 files changed, 1346 insertions(+), 1 deletion(-) > > 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 86d95d9..88ab32a 100644 > > --- a/drivers/gpu/drm/sprd/Makefile > > +++ b/drivers/gpu/drm/sprd/Makefile > > @@ -2,4 +2,7 @@ > > > > subdir-ccflags-y += -I$(srctree)/$(src) > Not needed - drop. It has been fixed on RFC V7. > > > > > -obj-y := sprd_drm.o > > +obj-y := sprd_drm.o \ > > + sprd_dpu.o > > + > > +obj-y += dpu/ > > Until there are several DPU's there is no need for a separate directory. > Make it simpler by keeping it all in drm/sprd/* It has been fixed on RFC V7. > > > diff --git a/drivers/gpu/drm/sprd/dpu/Makefile > > b/drivers/gpu/drm/sprd/dpu/Makefile > > new file mode 100644 > > index 000..40278b6 > > --- /dev/null > > +++ b/drivers/gpu/drm/sprd/dpu/Makefile > > @@ -0,0 +1,3 @@ > > +# SPDX-License-Identifier: GPL-2.0 > > + > > +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 000..4b9521d > > --- /dev/null > > +++ b/drivers/gpu/drm/sprd/dpu/dpu_r2p0.c > > @@ -0,0 +1,503 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Copyright (C) 2020 Unisoc Inc. > > + */ > > + > > +#include > > +#include > > +#include > > +#include > > + > > +#include "sprd_dpu.h" > > + > > +/* DPU registers size, 4 Bytes(32 Bits) */ > > +#define DPU_REG_SIZE 0x04 > > + > > +/* Layer registers offset */ > > +#define DPU_LAY_REG_OFFSET 0x0C > > + > > +#define DPU_LAY_REG(reg, index) \ > > + (reg + index * DPU_LAY_REG_OFFSET * DPU_REG_SIZE) > Use a static inline to get better typecheck. It has been fixed on RFC V7. > > > +#define DPU_REG_RD(reg) readl_relaxed(reg) > > + > > +#define DPU_REG_WR(reg, mask) writel_relaxed(mask, reg) > > + > > +#define DPU_REG_SET(reg, mask) \ > > + writel_relaxed(readl_relaxed(reg) | mask, reg) > > + > > +#define DPU_REG_CLR(reg, mask) \ > > + writel_relaxed(readl_relaxed(reg) & ~mask, reg) > I am no fan of macros used like this. > > Maybe use static inlines and add struct dpu_context * as first argument. > Then adding base can be hidden away. It has been fixed on RFC V7. > > > I had a hard time convincing myself that _relaxed was the right > variants. If I get it correct we may see writes re-ordered with the > _relaxed variants wich would be no good when dealign with interrupts. > But I may miss somethign here. I need to think about it > > > + > > +/* Global control registers */ > > +#define REG_DPU_CTRL 0x04 > > +#define REG_DPU_CFG0 0x08 > > +#define REG_DPU_CFG1 0x0C > > +#define REG_DPU_CFG2 0x10 > > +#define REG_PANEL_SIZE 0x20 > > +#define REG_BLEND_SIZE 0x24 > > +#define REG_BG_COLOR 0x2C > > + > > +/* Layer0 control registers */ > > +#define REG_LAY_BASE_ADDR0 0x30 > > +#define REG_LAY_BASE_ADDR1 0x34 > > +#define REG_LAY_BASE_ADDR2 0x38 > > +#define REG_LAY_CTRL 0x40 > > +#define REG_LAY_SIZE 0x44 > > +#define REG_LAY_PITCH0x48 > > +#define REG_LAY_POS 0x4C > > +#define REG_LAY_ALPHA0x50 > > +#define REG_LAY_PALLETE 0x58 > > +#define REG_LAY_CROP_START 0x5C > > + > > +/* Interrupt control registers */ > > +#define REG_DPU_INT_EN 0x1E0 > > +#define REG_DPU_INT_CLR 0x1E4 > > +#define REG_DPU_INT_STS 0x1E8 > > + > > +/* DPI control registers */ > > +#define REG_DPI_CTRL 0x1F0 > > +#define REG_DPI_H_TIMING 0x1F4 > > +#define REG_DPI_V_TIMING
Re: [PATCH RFC v6 4/6] drm/sprd: add Unisoc's drm display controller driver
Daniel Vetter 于2020年7月29日周三 上午5:51写道： > > On Tue, Jul 28, 2020 at 12:08 PM Kevin Tang wrote: > > > > From: Kevin Tang > > > > Adds DPU(Display Processor Unit) support for the Unisoc's display subsystem. > > It's support multi planes, scaler, rotation, PQ(Picture Quality) and more. > > > > RFC v6: > > - Access registers via readl/writel > > - Checking for unsupported KMS properties (format, rotation, blend_mode, > > etc) on plane_check ops > > - Remove always true checks for dpu core ops > > > > Cc: Orson Zhai > > Cc: Chunyan Zhang > > Signed-off-by: Kevin Tang > > Quickly scrolled through this, and the entire thing very much leaves a > midlayer heavy aftertaste. Do we really need stuff like struct dpu_layer > and struct dpu_core_ops? They only seem to complicate the code base, and > seem to have no real reason. The indirection with first computing register > values into a sprd_plane/crtc structure, and then writing it into hardware > is also a bit much - I recommend to only do that if you have to compute > values in _check to validate them, so that the computation doesn't have to > be repeated in the commit phase functions. Hi daniel, Maybe some DPU's IP verison, there will be some differences in the DPU on each Soc. Not only control registers, there will be some differences in control logic and functions If access H/W registers all put into crtc or plane driver, how did others deal with it? Add Soc's macro? When there are too many H/W IPs, split into different files(dpu_core_ops) according to different IP, I think the code project will be more convenient to keeping. For multiple H/W IP, the popular design method is? > > Also, the layer and pending_flips stuff in sprd_dpu don't work with > atomic, that races. You have to put all that stuff into state objects, or > if it's some data shared with interrupt handlers (doesn't seem to be the > case here), it needs its own locking, and any data you need in the > interrupt handler must be copied over. the layer and pending_flips, we put into atomic_flush, We do have some features related to interrupt handlers, but it has not been submitted yet. So pending_flips lock have not been added. > > Also no devm_kzalloc for anything containined a drm_* structure, that's > the wrong lifetime. > > So yeah high level review is that I think this driver would benefit a lot > from a pile of demidlayer. > > Cheers, Daniel > > > --- > > drivers/gpu/drm/sprd/Makefile | 5 +- > > drivers/gpu/drm/sprd/dpu/Makefile | 3 + > > drivers/gpu/drm/sprd/dpu/dpu_r2p0.c | 503 > > drivers/gpu/drm/sprd/sprd_dpu.c | 646 > > > > drivers/gpu/drm/sprd/sprd_dpu.h | 187 +++ > > drivers/gpu/drm/sprd/sprd_drm.c | 1 + > > drivers/gpu/drm/sprd/sprd_drm.h | 2 + > > 7 files changed, 1346 insertions(+), 1 deletion(-) > > 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 86d95d9..88ab32a 100644 > > --- a/drivers/gpu/drm/sprd/Makefile > > +++ b/drivers/gpu/drm/sprd/Makefile > > @@ -2,4 +2,7 @@ > > > > subdir-ccflags-y += -I$(srctree)/$(src) > > > > -obj-y := sprd_drm.o > > +obj-y := sprd_drm.o \ > > + sprd_dpu.o > > + > > +obj-y += dpu/ > > diff --git a/drivers/gpu/drm/sprd/dpu/Makefile > > b/drivers/gpu/drm/sprd/dpu/Makefile > > new file mode 100644 > > index 000..40278b6 > > --- /dev/null > > +++ b/drivers/gpu/drm/sprd/dpu/Makefile > > @@ -0,0 +1,3 @@ > > +# SPDX-License-Identifier: GPL-2.0 > > + > > +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 000..4b9521d > > --- /dev/null > > +++ b/drivers/gpu/drm/sprd/dpu/dpu_r2p0.c > > @@ -0,0 +1,503 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Copyright (C) 2020 Unisoc Inc. > > + */ > > + > > +#include > > +#include > > +#include > > +#include > > + > > +#include "sprd_dpu.h" > > + > > +/* DPU registers size, 4 Bytes(32 Bits) */ > > +#define DPU_REG_SIZE 0x04 > > + > > +/* Layer registers offset */ > > +#define DPU_LAY_REG_OFFSET 0x0C > > + > > +#define DPU_LAY_REG(reg, index) \ > > + (reg + index * DPU_LAY_REG_OFFSET * DPU_REG_SIZE) > > + > > +#define DPU_REG_RD(reg) readl_relaxed(reg) > > + > > +#define DPU_REG_WR(reg, mask) writel_relaxed(mask, reg) > > + > > +#define DPU_REG_SET(reg, mask) \ > > + writel_relaxed(readl_relaxed(reg) | mask, reg) > > + > > +#define DPU_REG_CLR(reg, mask) \ > > + writel_relaxed(readl_relaxed(reg) & ~mask, reg) > > + > > +/* Global control registers */ > > +#define REG_DPU_CTRL 0x04 > > +#define REG_DPU_CFG0 0x08 > > +#define REG_DPU_CFG1 0x0C > > +#define REG_DPU_CFG2 0x10 >
Re: [PATCH RFC v6 4/6] drm/sprd: add Unisoc's drm display controller driver
Sam Ravnborg 于2020年7月29日周三 上午5:14写道：
>
> Hi Kevin.
>
> Some feedback in the following.
> I lost track of thing for the atomic modesettting stuff and I hope other
> will review that.
>
> Sam
>
> On Tue, Jul 28, 2020 at 06:07:57PM +0800, Kevin Tang wrote:
> > From: Kevin Tang
> >
> > Adds DPU(Display Processor Unit) support for the Unisoc's display subsystem.
> > It's support multi planes, scaler, rotation, PQ(Picture Quality) and more.
> >
> > RFC v6:
> > - Access registers via readl/writel
> > - Checking for unsupported KMS properties (format, rotation, blend_mode,
> > etc) on plane_check ops
> > - Remove always true checks for dpu core ops
> >
> > Cc: Orson Zhai
> > Cc: Chunyan Zhang
> > Signed-off-by: Kevin Tang
> > ---
> > drivers/gpu/drm/sprd/Makefile | 5 +-
> > drivers/gpu/drm/sprd/dpu/Makefile | 3 +
> > drivers/gpu/drm/sprd/dpu/dpu_r2p0.c | 503
> > drivers/gpu/drm/sprd/sprd_dpu.c | 646
> >
> > drivers/gpu/drm/sprd/sprd_dpu.h | 187 +++
> > drivers/gpu/drm/sprd/sprd_drm.c | 1 +
> > drivers/gpu/drm/sprd/sprd_drm.h | 2 +
> > 7 files changed, 1346 insertions(+), 1 deletion(-)
> > 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 86d95d9..88ab32a 100644
> > --- a/drivers/gpu/drm/sprd/Makefile
> > +++ b/drivers/gpu/drm/sprd/Makefile
> > @@ -2,4 +2,7 @@
> >
> > subdir-ccflags-y += -I$(srctree)/$(src)
> Not needed - drop.
>
> >
> > -obj-y := sprd_drm.o
> > +obj-y := sprd_drm.o \
> > + sprd_dpu.o
> > +
> > +obj-y += dpu/
>
> Until there are several DPU's there is no need for a separate directory.
> Make it simpler by keeping it all in drm/sprd/*
Ok I will move DPU IP code to drm/sprd
>
> > diff --git a/drivers/gpu/drm/sprd/dpu/Makefile
> > b/drivers/gpu/drm/sprd/dpu/Makefile
> > new file mode 100644
> > index 000..40278b6
> > --- /dev/null
> > +++ b/drivers/gpu/drm/sprd/dpu/Makefile
> > @@ -0,0 +1,3 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +
> > +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 000..4b9521d
> > --- /dev/null
> > +++ b/drivers/gpu/drm/sprd/dpu/dpu_r2p0.c
> > @@ -0,0 +1,503 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2020 Unisoc Inc.
> > + */
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +#include "sprd_dpu.h"
> > +
> > +/* DPU registers size, 4 Bytes(32 Bits) */
> > +#define DPU_REG_SIZE 0x04
> > +
> > +/* Layer registers offset */
> > +#define DPU_LAY_REG_OFFSET 0x0C
> > +
> > +#define DPU_LAY_REG(reg, index) \
> > + (reg + index * DPU_LAY_REG_OFFSET * DPU_REG_SIZE)
> Use a static inline to get better typecheck.
>
> > +#define DPU_REG_RD(reg) readl_relaxed(reg)
> > +
> > +#define DPU_REG_WR(reg, mask) writel_relaxed(mask, reg)
> > +
> > +#define DPU_REG_SET(reg, mask) \
> > + writel_relaxed(readl_relaxed(reg) | mask, reg)
> > +
> > +#define DPU_REG_CLR(reg, mask) \
> > + writel_relaxed(readl_relaxed(reg) & ~mask, reg)
> I am no fan of macros used like this.
>
> Maybe use static inlines and add struct dpu_context * as first argument.
> Then adding base can be hidden away.
Maybe a good idea, i will try it, like this:
static inline void DPU_REG_CLR(dpu_context *ctx, u32 reg, u32 mask) {
writel_relaxed(readl_relaxed(ctx->base + reg) & ~mask, ctx->base + reg);
}
>
>
> I had a hard time convincing myself that _relaxed was the right
> variants. If I get it correct we may see writes re-ordered with the
> _relaxed variants wich would be no good when dealign with interrupts.
> But I may miss somethign here.
>
> > +
> > +/* Global control registers */
> > +#define REG_DPU_CTRL 0x04
> > +#define REG_DPU_CFG0 0x08
> > +#define REG_DPU_CFG1 0x0C
> > +#define REG_DPU_CFG2 0x10
> > +#define REG_PANEL_SIZE 0x20
> > +#define REG_BLEND_SIZE 0x24
> > +#define REG_BG_COLOR 0x2C
> > +
> > +/* Layer0 control registers */
> > +#define REG_LAY_BASE_ADDR0 0x30
> > +#define REG_LAY_BASE_ADDR1 0x34
> > +#define REG_LAY_BASE_ADDR2 0x38
> > +#define REG_LAY_CTRL 0x40
> > +#define REG_LAY_SIZE 0x44
> > +#define REG_LAY_PITCH0x48
> > +#define REG_LAY_POS 0x4C
> > +#define REG_LAY_ALPHA0x50
> > +#define REG_LAY_PALLETE 0x58
> > +#define REG_LAY_CROP_START 0x5C
> > +
> > +/* Interrupt control registers */
> > +#define REG_DPU_INT_EN 0x1E0
> > +#define REG_DPU_INT_CLR 0x1E4
> > +#define REG_DPU_INT_STS 0x1E8
> > +
> > +/* DPI control registers */
> > +#define REG_DPI_CTRL 0x1F0
> > +#define
Re: [PATCH RFC v6 4/6] drm/sprd: add Unisoc's drm display controller driver
On Tue, Jul 28, 2020 at 12:08 PM Kevin Tang wrote: > > From: Kevin Tang > > Adds DPU(Display Processor Unit) support for the Unisoc's display subsystem. > It's support multi planes, scaler, rotation, PQ(Picture Quality) and more. > > RFC v6: > - Access registers via readl/writel > - Checking for unsupported KMS properties (format, rotation, blend_mode, > etc) on plane_check ops > - Remove always true checks for dpu core ops > > Cc: Orson Zhai > Cc: Chunyan Zhang > Signed-off-by: Kevin Tang Quickly scrolled through this, and the entire thing very much leaves a midlayer heavy aftertaste. Do we really need stuff like struct dpu_layer and struct dpu_core_ops? They only seem to complicate the code base, and seem to have no real reason. The indirection with first computing register values into a sprd_plane/crtc structure, and then writing it into hardware is also a bit much - I recommend to only do that if you have to compute values in _check to validate them, so that the computation doesn't have to be repeated in the commit phase functions. Also, the layer and pending_flips stuff in sprd_dpu don't work with atomic, that races. You have to put all that stuff into state objects, or if it's some data shared with interrupt handlers (doesn't seem to be the case here), it needs its own locking, and any data you need in the interrupt handler must be copied over. Also no devm_kzalloc for anything containined a drm_* structure, that's the wrong lifetime. So yeah high level review is that I think this driver would benefit a lot from a pile of demidlayer. Cheers, Daniel > --- > drivers/gpu/drm/sprd/Makefile | 5 +- > drivers/gpu/drm/sprd/dpu/Makefile | 3 + > drivers/gpu/drm/sprd/dpu/dpu_r2p0.c | 503 > drivers/gpu/drm/sprd/sprd_dpu.c | 646 > > drivers/gpu/drm/sprd/sprd_dpu.h | 187 +++ > drivers/gpu/drm/sprd/sprd_drm.c | 1 + > drivers/gpu/drm/sprd/sprd_drm.h | 2 + > 7 files changed, 1346 insertions(+), 1 deletion(-) > 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 86d95d9..88ab32a 100644 > --- a/drivers/gpu/drm/sprd/Makefile > +++ b/drivers/gpu/drm/sprd/Makefile > @@ -2,4 +2,7 @@ > > subdir-ccflags-y += -I$(srctree)/$(src) > > -obj-y := sprd_drm.o > +obj-y := sprd_drm.o \ > + sprd_dpu.o > + > +obj-y += dpu/ > diff --git a/drivers/gpu/drm/sprd/dpu/Makefile > b/drivers/gpu/drm/sprd/dpu/Makefile > new file mode 100644 > index 000..40278b6 > --- /dev/null > +++ b/drivers/gpu/drm/sprd/dpu/Makefile > @@ -0,0 +1,3 @@ > +# SPDX-License-Identifier: GPL-2.0 > + > +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 000..4b9521d > --- /dev/null > +++ b/drivers/gpu/drm/sprd/dpu/dpu_r2p0.c > @@ -0,0 +1,503 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (C) 2020 Unisoc Inc. > + */ > + > +#include > +#include > +#include > +#include > + > +#include "sprd_dpu.h" > + > +/* DPU registers size, 4 Bytes(32 Bits) */ > +#define DPU_REG_SIZE 0x04 > + > +/* Layer registers offset */ > +#define DPU_LAY_REG_OFFSET 0x0C > + > +#define DPU_LAY_REG(reg, index) \ > + (reg + index * DPU_LAY_REG_OFFSET * DPU_REG_SIZE) > + > +#define DPU_REG_RD(reg) readl_relaxed(reg) > + > +#define DPU_REG_WR(reg, mask) writel_relaxed(mask, reg) > + > +#define DPU_REG_SET(reg, mask) \ > + writel_relaxed(readl_relaxed(reg) | mask, reg) > + > +#define DPU_REG_CLR(reg, mask) \ > + writel_relaxed(readl_relaxed(reg) & ~mask, reg) > + > +/* Global control registers */ > +#define REG_DPU_CTRL 0x04 > +#define REG_DPU_CFG0 0x08 > +#define REG_DPU_CFG1 0x0C > +#define REG_DPU_CFG2 0x10 > +#define REG_PANEL_SIZE 0x20 > +#define REG_BLEND_SIZE 0x24 > +#define REG_BG_COLOR 0x2C > + > +/* Layer0 control registers */ > +#define REG_LAY_BASE_ADDR0 0x30 > +#define REG_LAY_BASE_ADDR1 0x34 > +#define REG_LAY_BASE_ADDR2 0x38 > +#define REG_LAY_CTRL 0x40 > +#define REG_LAY_SIZE 0x44 > +#define REG_LAY_PITCH 0x48 > +#define REG_LAY_POS0x4C > +#define REG_LAY_ALPHA 0x50 > +#define REG_LAY_PALLETE0x58 > +#define REG_LAY_CROP_START 0x5C > + > +/* Interrupt control registers */ > +#define REG_DPU_INT_EN 0x1E0 > +#define REG_DPU_INT_CLR0x1E4 > +#define REG_DPU_INT_STS0x1E8 > + > +/* DPI control registers */ > +#define REG_DPI_CTRL 0x1F0 > +#define REG_DPI_H_TIMING 0x1F4 > +#define REG_DPI_V_TIMING 0x1F8 > + > +/* MMU control registers */ > +#define REG_MMU_EN 0x800 > +#define REG_MMU_VPN_RANGE
Re: [PATCH RFC v6 4/6] drm/sprd: add Unisoc's drm display controller driver
Hi Kevin. Some feedback in the following. I lost track of thing for the atomic modesettting stuff and I hope other will review that. Sam On Tue, Jul 28, 2020 at 06:07:57PM +0800, Kevin Tang wrote: > From: Kevin Tang > > Adds DPU(Display Processor Unit) support for the Unisoc's display subsystem. > It's support multi planes, scaler, rotation, PQ(Picture Quality) and more. > > RFC v6: > - Access registers via readl/writel > - Checking for unsupported KMS properties (format, rotation, blend_mode, > etc) on plane_check ops > - Remove always true checks for dpu core ops > > Cc: Orson Zhai > Cc: Chunyan Zhang > Signed-off-by: Kevin Tang > --- > drivers/gpu/drm/sprd/Makefile | 5 +- > drivers/gpu/drm/sprd/dpu/Makefile | 3 + > drivers/gpu/drm/sprd/dpu/dpu_r2p0.c | 503 > drivers/gpu/drm/sprd/sprd_dpu.c | 646 > > drivers/gpu/drm/sprd/sprd_dpu.h | 187 +++ > drivers/gpu/drm/sprd/sprd_drm.c | 1 + > drivers/gpu/drm/sprd/sprd_drm.h | 2 + > 7 files changed, 1346 insertions(+), 1 deletion(-) > 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 86d95d9..88ab32a 100644 > --- a/drivers/gpu/drm/sprd/Makefile > +++ b/drivers/gpu/drm/sprd/Makefile > @@ -2,4 +2,7 @@ > > subdir-ccflags-y += -I$(srctree)/$(src) Not needed - drop. > > -obj-y := sprd_drm.o > +obj-y := sprd_drm.o \ > + sprd_dpu.o > + > +obj-y += dpu/ Until there are several DPU's there is no need for a separate directory. Make it simpler by keeping it all in drm/sprd/* > diff --git a/drivers/gpu/drm/sprd/dpu/Makefile > b/drivers/gpu/drm/sprd/dpu/Makefile > new file mode 100644 > index 000..40278b6 > --- /dev/null > +++ b/drivers/gpu/drm/sprd/dpu/Makefile > @@ -0,0 +1,3 @@ > +# SPDX-License-Identifier: GPL-2.0 > + > +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 000..4b9521d > --- /dev/null > +++ b/drivers/gpu/drm/sprd/dpu/dpu_r2p0.c > @@ -0,0 +1,503 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (C) 2020 Unisoc Inc. > + */ > + > +#include > +#include > +#include > +#include > + > +#include "sprd_dpu.h" > + > +/* DPU registers size, 4 Bytes(32 Bits) */ > +#define DPU_REG_SIZE 0x04 > + > +/* Layer registers offset */ > +#define DPU_LAY_REG_OFFSET 0x0C > + > +#define DPU_LAY_REG(reg, index) \ > + (reg + index * DPU_LAY_REG_OFFSET * DPU_REG_SIZE) Use a static inline to get better typecheck. > +#define DPU_REG_RD(reg) readl_relaxed(reg) > + > +#define DPU_REG_WR(reg, mask) writel_relaxed(mask, reg) > + > +#define DPU_REG_SET(reg, mask) \ > + writel_relaxed(readl_relaxed(reg) | mask, reg) > + > +#define DPU_REG_CLR(reg, mask) \ > + writel_relaxed(readl_relaxed(reg) & ~mask, reg) I am no fan of macros used like this. Maybe use static inlines and add struct dpu_context * as first argument. Then adding base can be hidden away. I had a hard time convincing myself that _relaxed was the right variants. If I get it correct we may see writes re-ordered with the _relaxed variants wich would be no good when dealign with interrupts. But I may miss somethign here. > + > +/* Global control registers */ > +#define REG_DPU_CTRL 0x04 > +#define REG_DPU_CFG0 0x08 > +#define REG_DPU_CFG1 0x0C > +#define REG_DPU_CFG2 0x10 > +#define REG_PANEL_SIZE 0x20 > +#define REG_BLEND_SIZE 0x24 > +#define REG_BG_COLOR 0x2C > + > +/* Layer0 control registers */ > +#define REG_LAY_BASE_ADDR0 0x30 > +#define REG_LAY_BASE_ADDR1 0x34 > +#define REG_LAY_BASE_ADDR2 0x38 > +#define REG_LAY_CTRL 0x40 > +#define REG_LAY_SIZE 0x44 > +#define REG_LAY_PITCH0x48 > +#define REG_LAY_POS 0x4C > +#define REG_LAY_ALPHA0x50 > +#define REG_LAY_PALLETE 0x58 > +#define REG_LAY_CROP_START 0x5C > + > +/* Interrupt control registers */ > +#define REG_DPU_INT_EN 0x1E0 > +#define REG_DPU_INT_CLR 0x1E4 > +#define REG_DPU_INT_STS 0x1E8 > + > +/* DPI control registers */ > +#define REG_DPI_CTRL 0x1F0 > +#define REG_DPI_H_TIMING 0x1F4 > +#define REG_DPI_V_TIMING 0x1F8 > + > +/* MMU control registers */ > +#define REG_MMU_EN 0x800 > +#define REG_MMU_VPN_RANGE0x80C > +#define REG_MMU_VAOR_ADDR_RD 0x818 > +#define REG_MMU_VAOR_ADDR_WR 0x81C > +#define REG_MMU_INV_ADDR_RD 0x820 > +#define REG_MMU_INV_ADDR_WR 0x824 > +#define REG_MMU_PPN1 0x83C > +#define REG_MMU_RANGE1 0x840 > +#define REG_MMU_PPN2 0x844 > +#define REG_MMU_RANGE2
[PATCH RFC v6 4/6] drm/sprd: add Unisoc's drm display controller driver
From: Kevin Tang Adds DPU(Display Processor Unit) support for the Unisoc's display subsystem. It's support multi planes, scaler, rotation, PQ(Picture Quality) and more. RFC v6: - Access registers via readl/writel - Checking for unsupported KMS properties (format, rotation, blend_mode, etc) on plane_check ops - Remove always true checks for dpu core ops Cc: Orson Zhai Cc: Chunyan Zhang Signed-off-by: Kevin Tang --- drivers/gpu/drm/sprd/Makefile | 5 +- drivers/gpu/drm/sprd/dpu/Makefile | 3 + drivers/gpu/drm/sprd/dpu/dpu_r2p0.c | 503 drivers/gpu/drm/sprd/sprd_dpu.c | 646 drivers/gpu/drm/sprd/sprd_dpu.h | 187 +++ drivers/gpu/drm/sprd/sprd_drm.c | 1 + drivers/gpu/drm/sprd/sprd_drm.h | 2 + 7 files changed, 1346 insertions(+), 1 deletion(-) 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 86d95d9..88ab32a 100644 --- a/drivers/gpu/drm/sprd/Makefile +++ b/drivers/gpu/drm/sprd/Makefile @@ -2,4 +2,7 @@ subdir-ccflags-y += -I$(srctree)/$(src) -obj-y := sprd_drm.o +obj-y := sprd_drm.o \ + sprd_dpu.o + +obj-y += dpu/ diff --git a/drivers/gpu/drm/sprd/dpu/Makefile b/drivers/gpu/drm/sprd/dpu/Makefile new file mode 100644 index 000..40278b6 --- /dev/null +++ b/drivers/gpu/drm/sprd/dpu/Makefile @@ -0,0 +1,3 @@ +# SPDX-License-Identifier: GPL-2.0 + +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 000..4b9521d --- /dev/null +++ b/drivers/gpu/drm/sprd/dpu/dpu_r2p0.c @@ -0,0 +1,503 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (C) 2020 Unisoc Inc. + */ + +#include +#include +#include +#include + +#include "sprd_dpu.h" + +/* DPU registers size, 4 Bytes(32 Bits) */ +#define DPU_REG_SIZE 0x04 + +/* Layer registers offset */ +#define DPU_LAY_REG_OFFSET 0x0C + +#define DPU_LAY_REG(reg, index) \ + (reg + index * DPU_LAY_REG_OFFSET * DPU_REG_SIZE) + +#define DPU_REG_RD(reg) readl_relaxed(reg) + +#define DPU_REG_WR(reg, mask) writel_relaxed(mask, reg) + +#define DPU_REG_SET(reg, mask) \ + writel_relaxed(readl_relaxed(reg) | mask, reg) + +#define DPU_REG_CLR(reg, mask) \ + writel_relaxed(readl_relaxed(reg) & ~mask, reg) + +/* Global control registers */ +#define REG_DPU_CTRL 0x04 +#define REG_DPU_CFG0 0x08 +#define REG_DPU_CFG1 0x0C +#define REG_DPU_CFG2 0x10 +#define REG_PANEL_SIZE 0x20 +#define REG_BLEND_SIZE 0x24 +#define REG_BG_COLOR 0x2C + +/* Layer0 control registers */ +#define REG_LAY_BASE_ADDR0 0x30 +#define REG_LAY_BASE_ADDR1 0x34 +#define REG_LAY_BASE_ADDR2 0x38 +#define REG_LAY_CTRL 0x40 +#define REG_LAY_SIZE 0x44 +#define REG_LAY_PITCH 0x48 +#define REG_LAY_POS0x4C +#define REG_LAY_ALPHA 0x50 +#define REG_LAY_PALLETE0x58 +#define REG_LAY_CROP_START 0x5C + +/* Interrupt control registers */ +#define REG_DPU_INT_EN 0x1E0 +#define REG_DPU_INT_CLR0x1E4 +#define REG_DPU_INT_STS0x1E8 + +/* DPI control registers */ +#define REG_DPI_CTRL 0x1F0 +#define REG_DPI_H_TIMING 0x1F4 +#define REG_DPI_V_TIMING 0x1F8 + +/* MMU control registers */ +#define REG_MMU_EN 0x800 +#define REG_MMU_VPN_RANGE 0x80C +#define REG_MMU_VAOR_ADDR_RD 0x818 +#define REG_MMU_VAOR_ADDR_WR 0x81C +#define REG_MMU_INV_ADDR_RD0x820 +#define REG_MMU_INV_ADDR_WR0x824 +#define REG_MMU_PPN1 0x83C +#define REG_MMU_RANGE1 0x840 +#define REG_MMU_PPN2 0x844 +#define REG_MMU_RANGE2 0x848 + +/* Global control bits */ +#define BIT_DPU_RUNBIT(0) +#define BIT_DPU_STOP BIT(1) +#define BIT_DPU_REG_UPDATE BIT(2) +#define BIT_DPU_IF_EDPIBIT(0) +#define BIT_DPU_COEF_NARROW_RANGE BIT(4) +#define BIT_DPU_Y2R_COEF_ITU709_STANDARD BIT(5) + +/* Layer control bits */ +#define BIT_DPU_LAY_EN BIT(0) + +/* Interrupt control & status bits */ +#define BIT_DPU_INT_DONE BIT(0) +#define BIT_DPU_INT_TE BIT(1) +#define BIT_DPU_INT_ERRBIT(2) +#define BIT_DPU_INT_UPDATE_DONEBIT(4) +#define BIT_DPU_INT_VSYNC BIT(5) +#define BIT_DPU_INT_FBC_PLD_ERRBIT(8) +#define BIT_DPU_INT_FBC_HDR_ERRBIT(9) +#define BIT_DPU_INT_MMU_VAOR_RDBIT(16) +#define BIT_DPU_INT_MMU_VAOR_WRBIT(17) +#define BIT_DPU_INT_MMU_INV_RD BIT(18) +#define
