Re: [PATCH RFC 4/8] drm/sprd: add Unisoc's drm display controller driver

[Emil Velikov]

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