From: Emil Velikov <emil.l.velikov@gmail.com>
To: Kevin Tang <kevin3.tang@gmail.com>
Cc: David Airlie <airlied@linux.ie>, Daniel Vetter <daniel@ffwll.ch>,
orsonzhai@gmail.com,
ML dri-devel <dri-devel@lists.freedesktop.org>,
"Linux-Kernel@Vger. Kernel. Org" <linux-kernel@vger.kernel.org>,
zhang.lyra@gmail.com, baolin.wang@linaro.org
Subject: Re: [PATCH RFC 4/8] drm/sprd: add Unisoc's drm display controller driver
Date: Tue, 10 Dec 2019 17:13:37 +0000 [thread overview]
Message-ID: <CACvgo50Hgbb8ywX2RgFqkitxwBG64EhP9g1TSxgLkQf-6L6soA@mail.gmail.com> (raw)
In-Reply-To: <1575966995-13757-5-git-send-email-kevin3.tang@gmail.com>
Hi Kevin,
On Tue, 10 Dec 2019 at 08:41, Kevin Tang <kevin3.tang@gmail.com> wrote:
>
> From: Kevin Tang <kevin.tang@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 <orsonzhai@gmail.com>
> Cc: Baolin Wang <baolin.wang@linaro.org>
> Cc: Chunyan Zhang <zhang.lyra@gmail.com>
> Signed-off-by: Kevin Tang <kevin.tang@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
<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
next prev parent reply other threads:[~2019-12-10 17:14 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-12-10 8:36 [PATCH RFC 0/8] Add Unisoc's drm kms module Kevin Tang
2019-12-10 8:36 ` [PATCH RFC 1/8] dt-bindings: display: add Unisoc's drm master bindings Kevin Tang
2019-12-13 9:42 ` Maxime Ripard
2019-12-10 8:36 ` [PATCH RFC 2/8] drm/sprd: add Unisoc's drm kms master Kevin Tang
2019-12-10 10:32 ` Thomas Zimmermann
[not found] ` <CAFPSGXb-pbmXFXrN7adK1TO+09T8qtJjwk+dK-TidZSsfPqHFg@mail.gmail.com>
2019-12-10 12:47 ` Thomas Zimmermann
2019-12-10 10:41 ` Daniel Vetter
2019-12-10 16:06 ` Emil Velikov
2019-12-10 22:01 ` Daniel Vetter
2019-12-10 8:36 ` [PATCH RFC 3/8] dt-bindings: display: add Unisoc's dpu bindings Kevin Tang
2019-12-13 9:44 ` Maxime Ripard
2019-12-10 8:36 ` [PATCH RFC 4/8] drm/sprd: add Unisoc's drm display controller driver Kevin Tang
2019-12-10 10:44 ` Thomas Zimmermann
2019-12-10 17:13 ` Emil Velikov [this message]
[not found] ` <CAFPSGXZMmfeBxkNhuNR59bX26_69_y5C13P7qY-UawVDa7Q3Jw@mail.gmail.com>
2019-12-11 11:46 ` Emil Velikov
2019-12-10 8:36 ` [PATCH RFC 5/8] dt-bindings: display: add Unisoc's mipi dsi&dphy bindings Kevin Tang
2019-12-13 9:46 ` Maxime Ripard
2019-12-10 8:36 ` [PATCH RFC 6/8] drm/sprd: add Unisoc's drm mipi dsi&dphy driver Kevin Tang
2019-12-10 8:36 ` [PATCH RFC 7/8] dt-bindings: display: add Unisoc's generic mipi panel bindings Kevin Tang
2019-12-13 9:48 ` Maxime Ripard
2019-12-10 8:36 ` [PATCH RFC 8/8] drm/sprd: add Unisoc's drm generic mipi panel driver Kevin Tang
2019-12-10 18:54 ` [PATCH RFC 0/8] Add Unisoc's drm kms module Sam Ravnborg
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CACvgo50Hgbb8ywX2RgFqkitxwBG64EhP9g1TSxgLkQf-6L6soA@mail.gmail.com \
--to=emil.l.velikov@gmail.com \
--cc=airlied@linux.ie \
--cc=baolin.wang@linaro.org \
--cc=daniel@ffwll.ch \
--cc=dri-devel@lists.freedesktop.org \
--cc=kevin3.tang@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=orsonzhai@gmail.com \
--cc=zhang.lyra@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).