<div dir="ltr"><div dir="ltr">Hi</div> <div class="gmail_quote"><div dir="ltr" class="gmail_attr">Emil Velikov <<a href="mailto:emil.l.velikov@gmail.com">emil.l.velikov@gmail.com</a>> 于2019年12月11日周三上午1:14写道： </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Kevin, On Tue, 10 Dec 2019 at 08:41, Kevin Tang <<a href="mailto:kevin3.tang@gmail.com" target="_blank">kevin3.tang@gmail.com</a>> wrote: > > From: Kevin Tang <<a href="mailto:kevin.tang@unisoc.com" target="_blank">kevin.tang@unisoc.com</a>> > > 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 | 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 </blockquote><div>Ok, i will split this patch, upstream modeset and atomic at first. clock, gloabl, enhance, extra</div>attributes and so on will be upload later.<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <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. </blockquote><div>Ok </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <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. </blockquote><div>enhance code is a big feature, so i will removed it at first commit.</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <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 ;-) </blockquote><div>Yes, our display controller have multiple ip version, every version maybe different</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..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. </blockquote><div>Ok, i will split it and commit later... </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <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? </blockquote><div>clk and glb ops have not been submitted, so i will remove clk and glb ops at first.</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> > + 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 </blockquote></div></div>