Hi
Emil Velikov <emil.l.veli...@gmail.com> 于2019年12月11日周三 上午1:14写道：
> Hi Kevin,
>
> On Tue, 10 Dec 2019 at 08:41, Kevin Tang <kevin3.t...@gmail.com> wrote:
> >
> > From: Kevin Tang <kevin.t...@unisoc.com>
> >
> > 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 | 290 +++++++
> > drivers/gpu/drm/sprd/disp_lib.h | 40 +
> > drivers/gpu/drm/sprd/dpu/Makefile | 8 +
> > drivers/gpu/drm/sprd/dpu/dpu_r2p0.c | 1464
> +++++++++++++++++++++++++++++++++++
> > drivers/gpu/drm/sprd/sprd_dpu.c | 1152 +++++++++++++++++++++++++++
> > drivers/gpu/drm/sprd/sprd_dpu.h | 217 ++++++
> > 7 files changed, 3176 insertions(+), 1 deletion(-)
>
> As we can see from the diff stat this patch is huge. So it would be fairly
> hard
> to provide meaningful review as-is.
>
> One can combine my earlier suggestion (to keep modeset/atomic out of 2/8),
> with
> the following split:
> - 4/8 add basic atomic modeset support - one format, one rotation 0, no
> extra
> attributes
> - 5/8 add extra formats
> - 6/8 add extra rotation support
> - ... add custom attributes
>
Ok, i will split this patch, upstream modeset and atomic at first. clock,
gloabl, enhance, extra
attributes and so on will be upload later.
>
>
> <snip>
>
> > --- /dev/null
> > +++ b/drivers/gpu/drm/sprd/disp_lib.c
>
> > --- /dev/null
> > +++ b/drivers/gpu/drm/sprd/disp_lib.h
>
> Let's keep this code out, for now. If we really need it we could
> revive/add it
> at a later stage.
>
Ok
>
> <snip>
>
> > --- /dev/null
> > +++ b/drivers/gpu/drm/sprd/dpu/dpu_r2p0.c
> > @@ -0,0 +1,1464 @@
>
>
> > +struct gamma_entry {
> > + u16 r;
> > + u16 g;
> > + u16 b;
> > +};
> > +
> Seem to be unused. Please drop this and double-check for other unused
> structs
>
>
> > +static struct scale_cfg scale_copy;
> > +static struct cm_cfg cm_copy;
> > +static struct slp_cfg slp_copy;
> > +static struct gamma_lut gamma_copy;
> > +static struct hsv_lut hsv_copy;
> > +static struct epf_cfg epf_copy;
> > +static u32 enhance_en;
> > +
> > +static DECLARE_WAIT_QUEUE_HEAD(wait_queue);
> > +static bool panel_ready = true;
> > +static bool need_scale;
> > +static bool mode_changed;
> > +static bool evt_update;
> > +static bool evt_stop;
> > +static u32 prev_y2r_coef;
> > +
> We should really avoid static variables. Some of the above look like
> enhancer
> state. One could create a struct keeping it alongside the rest of the
> display
> pipeline, right?
>
>
> <snip>
>
> > +static void dpu_enhance_backup(u32 id, void *param)
> > +{
> As the enhance code is fairly large, lets keep the portions to a separate
> patch.
>
enhance code is a big feature, so i will removed it at first commit.
>
>
> <snip>
>
> > +static struct dpu_core_ops dpu_r2p0_ops = {
> Nit: as a general rule ops should be const.
>
>
> > +static int __init dpu_core_register(void)
> > +{
> > + return dpu_core_ops_register(&entry);
> > +}
> > +
> > +subsys_initcall(dpu_core_register);
> I think that subsys_initcall, __init and MODULE area applicable only
> when we have multiple
> modules. Not 100% sure though ;-)
>
Yes, our display controller have multiple ip version, every version maybe
different
>
>
> > diff --git a/drivers/gpu/drm/sprd/sprd_dpu.c
> b/drivers/gpu/drm/sprd/sprd_dpu.c
> > new file mode 100644
> > index 0000000..43142b3
> > --- /dev/null
> > +++ b/drivers/gpu/drm/sprd/sprd_dpu.c
>
> > +struct sprd_plane {
> > + struct drm_plane plane;
> > + struct drm_property *alpha_property;
> > + struct drm_property *blend_mode_property;
> Core DRM already has alpha and blend_mode properties. Please reuse the code
>
>
> > + struct drm_property *fbc_hsize_r_property;
> > + struct drm_property *fbc_hsize_y_property;
> > + struct drm_property *fbc_hsize_uv_property;
> > + struct drm_property *y2r_coef_property;
> > + struct drm_property *pallete_en_property;
> > + struct drm_property *pallete_color_property;
> Let's have these properties introduced with separate follow-up patches.
> Please mention, in the commit message, why they are specific to the driver.
>
Ok, i will split it and commit later...
>
>
> <snip>
>
> > +static int sprd_dpu_init(struct sprd_dpu *dpu)
> > +{
> > + struct dpu_context *ctx = &dpu->ctx;
> > +
> > + down(&ctx->refresh_lock);
> > +
> > + if (dpu->ctx.is_inited) {
> > + up(&ctx->refresh_lock);
> > + return 0;
> > + }
> > +
> > + if (dpu->glb && dpu->glb->power)
> > + dpu->glb->power(ctx, true);
> > + if (dpu->glb && dpu->glb->enable)
> > + dpu->glb->enable(ctx);
> > +
> > + if (ctx->is_stopped && dpu->glb && dpu->glb->reset)
> > + dpu->glb->reset(ctx);
> > +
> > + if (dpu->clk && dpu->clk->init)
> > + dpu->clk->init(ctx);
> > + if (dpu->clk && dpu->clk->enable)
> > + dpu->clk->enable(ctx);
> > +
> > + if (dpu->core && dpu->core->init)
> > + dpu->core->init(ctx);
> > + if (dpu->core && dpu->core->ifconfig)
> > + dpu->core->ifconfig(ctx);
> > +
> Hmm I can see the core, clk and glb (ops) being added to the respective
> lists.
> Yet the code which adds those to the dpu isn't so obvious. Where is it?
>
clk and glb ops have not been submitted, so i will remove clk and glb ops
at first.
>
>
> > + ctx->is_inited = true;
> > +
> Nit: is_inited -> initialized
>
>
>
> <snip>
>
> > +struct dpu_core_ops {
> > + int (*parse_dt)(struct dpu_context *ctx,
> > + struct device_node *np);
> > + u32 (*version)(struct dpu_context *ctx);
> > + int (*init)(struct dpu_context *ctx);
> > + void (*uninit)(struct dpu_context *ctx);
> > + void (*run)(struct dpu_context *ctx);
> > + void (*stop)(struct dpu_context *ctx);
> > + void (*disable_vsync)(struct dpu_context *ctx);
> > + void (*enable_vsync)(struct dpu_context *ctx);
> > + u32 (*isr)(struct dpu_context *ctx);
> > + void (*ifconfig)(struct dpu_context *ctx);
> > + void (*write_back)(struct dpu_context *ctx, u8 count, bool
> debug);
> > + void (*flip)(struct dpu_context *ctx,
> > + struct sprd_dpu_layer layers[], u8 count);
> > + int (*capability)(struct dpu_context *ctx,
> > + struct dpu_capability *cap);
> > + void (*bg_color)(struct dpu_context *ctx, u32 color);
> > + void (*enhance_set)(struct dpu_context *ctx, u32 id, void
> *param);
> > + void (*enhance_get)(struct dpu_context *ctx, u32 id, void
> *param);
> > + int (*modeset)(struct dpu_context *ctx,
> > + struct drm_mode_modeinfo *mode);
> > + bool (*check_raw_int)(struct dpu_context *ctx, u32 mask);
> > +};
> > +
> > +struct dpu_clk_ops {
> > + int (*parse_dt)(struct dpu_context *ctx,
> > + struct device_node *np);
> > + int (*init)(struct dpu_context *ctx);
> > + int (*uinit)(struct dpu_context *ctx);
> > + int (*enable)(struct dpu_context *ctx);
> > + int (*disable)(struct dpu_context *ctx);
> > + int (*update)(struct dpu_context *ctx, int clk_id, int val);
> > +};
> > +
> > +struct dpu_glb_ops {
> > + int (*parse_dt)(struct dpu_context *ctx,
> > + struct device_node *np);
> > + void (*enable)(struct dpu_context *ctx);
> > + void (*disable)(struct dpu_context *ctx);
> > + void (*reset)(struct dpu_context *ctx);
> > + void (*power)(struct dpu_context *ctx, int enable);
> > +};
> > +
> Some of the above seem unused - parse_dt for example. Please drop the dead
> code.
>
>
> HTH
> Emil
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
