Hi Kevin,
On Tue, 10 Dec 2019 at 08:41, Kevin Tang <kevin3.tang@xxxxxxxxx> wrote:
>
> From: Kevin Tang <kevin.tang@xxxxxxxxxx>
>
> 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 <orsonzhai@xxxxxxxxx>
> Cc: Baolin Wang <baolin.wang@xxxxxxxxxx>
> Cc: Chunyan Zhang <zhang.lyra@xxxxxxxxx>
> Signed-off-by: Kevin Tang <kevin.tang@xxxxxxxxxx>
> ---
> 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
<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.
<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.
<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 ;-)
> 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.
<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?
> + 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
