[RFC v5 02/12] drm/mediatek: Add DRM Driver for Mediatek SoC MT8173.

Philipp Zabel p.zabel at pengutronix.de
Thu Nov 5 06:55:36 PST 2015


Hi Daniel,

Am Donnerstag, den 05.11.2015, 02:49 +0800 schrieb Daniel Kurtz:
> Hi Philipp,
> 
> A bunch of review comments inline.

A bunch indeed. Thank you for the feedback.

> On Wed, Nov 4, 2015 at 7:44 PM, Philipp Zabel <p.zabel at pengutronix.de> wrote:
[...]
> > +struct mtk_drm_crtc {
> > +       struct drm_crtc                         base;
> > +       unsigned int                            pipe;
> 
> There is one pipe too many :-)
> I think this one is not used."
> 
> > +       bool                                    enabled;
> > +       struct mtk_crtc_ddp_context             *ctx;
> 
> I think you should be able to just embed the "mtk_crtc_ddp_context"
> right into mtk_drm_crtc.
> Or maybe just get rid of mtk_crtc_ddp_context completely and just use
> mtk_drm_crtc eveywhere.

I'll combine them and get rid of the superfluous pipe.

[...]
> > +static void mtk_crtc_ddp_hw_init(struct mtk_drm_crtc *crtc)
> > +{
[...]
> > +       /* disp_mtcmos */
> > +       ret = pm_runtime_get_sync(drm->dev);
> > +       if (ret < 0)
> > +               DRM_ERROR("Failed to enable power domain: %d\n", ret);
> > +
> > +       mtk_ddp_clock_on(ctx->mutex_dev);
> > +       mtk_crtc_ddp_power_on(ctx);
> 
> get_sync(), clock_on() and ddp_power_on() really can fail; we are just
> ignoring errors here.
> Since they can fail, shouldn't they be moved out of the atomic
> "->enable()" path into the ->check() path that is allowed to fail?

You suggest I move them into atomic_check?

To me it sounds like this should not be called from check, but from the
drm_mode_config_funcs atomic_commit callback, right after calling 
drm_atomic_helper_prepare_planes. But there is no prepare equivalent to
drm_atomic_helper_commit_modeset_enables.

> > +
> > +       DRM_INFO("mediatek_ddp_ddp_path_setup\n");
> > +       for (i = 0; i < (ctx->ddp_comp_nr - 1); i++) {
> 
> nit: the inner () are not necessary.

Will remove those.

> > +               mtk_ddp_add_comp_to_path(ctx->config_regs, ctx->pipe,
> > +                                        ctx->ddp_comp[i].funcs->comp_id,
> > +                                        ctx->ddp_comp[i+1].funcs->comp_id);
> > +               mtk_ddp_add_comp_to_mutex(ctx->mutex_dev, ctx->pipe,
> > +                                         ctx->ddp_comp[i].funcs->comp_id,
> > +                                         ctx->ddp_comp[i+1].funcs->comp_id);
> > +       }
> 
> Do you really have to do this here in the enable path?  This looks
> like something that should be done in bind()?
> 
> Perhaps all we really need here is to walk the path and write to
> DISP_REG_MUTEX_EN at the end of mtk_ddp_add_comp_to_mutex().
> By the way, where are those bits cleared in the disable path?

Disabling or changing the path is not implemented, the currently static
setup could well be moved into bind().

[...]
> > +static void mtk_crtc_ddp_hw_fini(struct mtk_drm_crtc *crtc)
> > +{
[...]
> > +       pm_runtime_put_sync(drm->dev);
> 
> Why sync?

I can think of no reason, will use the async version here.

[...]
> > +static void mtk_drm_crtc_disable(struct drm_crtc *crtc)
> > +{
> > +       struct mtk_drm_crtc *mtk_crtc = to_mtk_crtc(crtc);
> > +       struct mtk_crtc_state *state = to_mtk_crtc_state(crtc->state);
> > +
> > +       DRM_INFO("mtk_drm_crtc_disable %d\n", crtc->base.id);
> > +       if (WARN_ON(!mtk_crtc->enabled))
> > +               return;
> > +
> > +       mtk_crtc_ddp_hw_fini(mtk_crtc);
> > +       mtk_crtc->do_flush = false;
> > +       if (state->event)
> > +               mtk_drm_crtc_finish_page_flip(mtk_crtc);
> 
> It is a bit awkward to send the page flip event before the actual page
> flip has completed (in this case, for a page flip that you are
> canceling by disabling the crtc).
> Would it be better to wait for vblank here in crtc_disable instead?

Yes, I will wait for vblank here instead.

[...]
> > +static void mtk_drm_crtc_atomic_begin(struct drm_crtc *crtc,
> > +                                     struct drm_crtc_state *old_crtc_state)
> > +{
> > +       struct mtk_crtc_state *state = to_mtk_crtc_state(crtc->state);
> > +
> > +       if (state->base.event) {
> > +               state->base.event->pipe = drm_crtc_index(crtc);
> > +               WARN_ON(drm_crtc_vblank_get(crtc) != 0);
> > +               state->event = state->base.event;
> > +               state->base.event = NULL;
> 
> Hmm. Why are we "stealing" event from drm_crtc_state and handing it
> over to the wrapper mtk_crtc_state?
> Won't we still be able to retrieve state->base.event later when we
> need it in the mtk_drm_crtc_finish_page_flip?

Hmm, good point. I'll try that.

[...]
> > +static void mtk_crtc_ddp_irq(struct mtk_crtc_ddp_context *ctx)
> 
> ==> So, this is the part of the driver that makes me the most nervous.
> Why are we writing the actual hardware registers in the *vblank
> interrupt handler*?!
> Every other display controller that I've ever seen has shadow
> registers that are used to stage a page flip at the next vblank.
> Are you sure this hardware doesn't support this?
>
> In any case, how do we get that first interrupt in which to apply the register?

I'll try to rework this using the DISP_MUTEX to trigger register updates
on SOF.

> > +{
> > +       struct mtk_drm_crtc *mtk_crtc = ctx->crtc;
> > +       struct mtk_ddp_comp *ovl = &ctx->ddp_comp[0];
> 
> ddp_comp[0] here looks a bit like black magic.  "The 0th component is
> always the ovl".
> Perhaps make an explicitly named "ovl" field for mtk_crtc_ddp_context.

Ok.

> > +       struct mtk_crtc_state *state = to_mtk_crtc_state(mtk_crtc->base.state);
> > +       unsigned int i;
> > +
> > +       if (state->pending_config) {
> > +               state->pending_config = false;
> > +
> > +               for (i = 0; i < OVL_LAYER_NR; i++)
> > +                       ovl->funcs->comp_layer_off(ovl->regs, i);
> 
> Why do you need to turn off all layers before setting mode?

I am not sure if this is necessary. It seems to work without.

> > +               ovl->funcs->comp_config(ovl->regs, state->pending_width,
> > +                                                  state->pending_height,
> > +                                                  state->pending_vrefresh);
> > +       }
> > +
> > +       for (i = 0; i < OVL_LAYER_NR; i++) {
> > +               if (state->pending_ovl_config[i]) {
> > +                       if (!state->pending_ovl_enable[i])
> > +                               ovl->funcs->comp_layer_off(ovl->regs, i);
> > +
> > +                       ovl->funcs->comp_layer_config(ovl->regs, i,
> > +                                       state->pending_ovl_addr[i],
> > +                                       state->pending_ovl_pitch[i],
> > +                                       state->pending_ovl_format[i],
> > +                                       state->pending_ovl_x[i],
> > +                                       state->pending_ovl_y[i],
> > +                                       state->pending_ovl_size[i]);
> > +
> > +                       if (state->pending_ovl_enable[i])
> > +                               ovl->funcs->comp_layer_on(ovl->regs, i);
> > +               }
> 
> I'd move this all all with an mtk_plane function, and let each
> mtk_plane store its own pending state.

Ok.

[...]
> > +static int mtk_crtc_ddp_bind(struct device *dev, struct device *master,
> > +               void *data)
> > +{
[...]
> > +       for (zpos = 0; zpos < OVL_LAYER_NR; zpos++) {
> > +               type = (zpos == 0) ? DRM_PLANE_TYPE_PRIMARY :
> > +                               (zpos == 1) ? DRM_PLANE_TYPE_CURSOR :
> > +                                               DRM_PLANE_TYPE_OVERLAY;
> > +               ret = mtk_plane_init(drm_dev, &ctx->planes[zpos],
> > +                               1 << ctx->pipe, type, zpos, OVL_LAYER_NR);
> > +               if (ret)
> > +                       goto unprepare;
> > +       }
> 
> I think you should just let mtk_drm_crtc_create() create & init its own planes.
> Currently, its overlay planes are unassigned.
> Furthermore, the crtc context doesn't really need an array of planes at all.
> It only really uses the array to know which planes to update during
> the vblank irq.
> But, that information is actually already present in the atomic state
> to be applied itself.



[...]
> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.h b/drivers/gpu/drm/mediatek/mtk_drm_crtc.h
> > new file mode 100644
> > index 0000000..b696066
> > --- /dev/null
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.h
> > @@ -0,0 +1,56 @@
[...]
> > +struct mtk_crtc_state {
> > +       struct                          drm_crtc_state base;
> > +       struct drm_pending_vblank_event *event;
> > +
> > +       unsigned int                    pending_update;
> > +       bool                            pending_needs_vblank;
> > +
> > +       bool                            pending_config;
> > +       unsigned int                    pending_width;
> > +       unsigned int                    pending_height;
> > +       unsigned int                    pending_vrefresh;
> > +
> > +       bool                            pending_ovl_config[OVL_LAYER_NR];
> > +       bool                            pending_ovl_enable[OVL_LAYER_NR];
> > +       unsigned int                    pending_ovl_addr[OVL_LAYER_NR];
> > +       unsigned int                    pending_ovl_pitch[OVL_LAYER_NR];
> > +       unsigned int                    pending_ovl_format[OVL_LAYER_NR];
> > +       int                             pending_ovl_x[OVL_LAYER_NR];
> > +       int                             pending_ovl_y[OVL_LAYER_NR];
> > +       unsigned int                    pending_ovl_size[OVL_LAYER_NR];
> > +       bool                            pending_ovl_dirty[OVL_LAYER_NR];
> 
> All of these OVL_LAYER_NR length arrays look like mtk_plane_state to me.
> I think we want to do something similar and wrap drm_plane_state.



[...]
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp.c
> > @@ -0,0 +1,218 @@
[...]
> > +#define DISP_REG_MUTEX_EN(n)   (0x20 + 0x20 * n)
> > +#define DISP_REG_MUTEX_RST(n)  (0x28 + 0x20 * n)
> > +#define DISP_REG_MUTEX_MOD(n)  (0x2c + 0x20 * n)
> > +#define DISP_REG_MUTEX_SOF(n)  (0x30 + 0x20 * n)
> 
> The n should be in parens here.

Ok.

[...]
> > +void mtk_ddp_add_comp_to_path(void __iomem *config_regs, unsigned int pipe,
> > +                             enum mtk_ddp_comp_id cur,
> > +                             enum mtk_ddp_comp_id next)
>
> Why pass pipe if we don't use it?

Leftover from before the split of mtk_ddp_add_to_path/mutex.

[...]
> > +       if (value) {
> > +               unsigned int reg = readl(config_regs + addr) | value;
> > +
> > +               writel(reg, config_regs + addr);
> 
> Almost all of the writel() in this patch can really be writel_relaxed().
> The only ones that need writel() are when you really need a barrier.

Ok.

> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> > new file mode 100644
> > index 0000000..2f3b32b
> > --- /dev/null
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> > @@ -0,0 +1,424 @@
[...]
> > +static void mtk_ovl_config(void __iomem *ovl_base,
> > +               unsigned int w, unsigned int h, unsigned int vrefresh)
> > +{
> > +       if (w != 0 && h != 0)
> > +               writel(h << 16 | w, ovl_base + DISP_REG_OVL_ROI_SIZE);
> 
> Should you just write 0 to DISP_REG_OVL_ROI_SIZE if w or h are 0?

According to the docs, width and height must be at least 1.
But of course I should check this earlier.

[...]
> > +static unsigned int ovl_fmt_convert(unsigned int fmt)
> 
> return int so the error code stays negative?

I'll just return RGB888 on error.

[...]
> > +static void mtk_ovl_layer_config(void __iomem *ovl_base, unsigned int idx,
> > +               unsigned int addr, unsigned int pitch, unsigned int fmt,
> > +               int x, int y, unsigned int size)
> > +{
> > +       unsigned int reg;
> > +
> > +       reg = has_rb_swapped(fmt) << 24 | ovl_fmt_convert(fmt) << 12;
> 
> ovl_fmt_convert() can return < 0 for unsupported fmt.
> But, perhaps we can remove that check, since it should actually
> already be guaranteed by the drm core that a plane is only ever asked
> to display an FB with a format that the plane advertises.

Exactly. If something unsupported gets through here, it's a bug.

[...]
> > +static const struct mtk_ddp_comp_funcs ddp_color0 = {
> > +       .comp_id = DDP_COMPONENT_COLOR0,
> > +       .comp_power_on = mtk_color_start,
> 
> For consistency, can you name all of the "*_start()" functions
> "*_power_on", or change the .comp_power_on to .comp_start.
> Actually, just drop the "comp_" from all of the fields of
> mtk_ddp_comp_funcs, since it is redundant.

I'd change them all to .enable/.disable.

> > +static const struct mtk_ddp_comp_funcs ddp_ovl1 = {
> > +       .comp_id = DDP_COMPONENT_OVL1,
> > +       .comp_config = mtk_ovl_config,
> > +       .comp_power_on = mtk_ovl_start,
> > +       .comp_enable_vblank = mtk_ovl_enable_vblank,
> > +       .comp_disable_vblank = mtk_ovl_disable_vblank,
> > +       .comp_clear_vblank = mtk_ovl_clear_vblank,
> > +       .comp_layer_on = mtk_ovl_layer_on,
> > +       .comp_layer_off = mtk_ovl_layer_off,
> > +       .comp_layer_config = mtk_ovl_layer_config,
> > +};
> 
> The callback duplication here suggests the component model can be refined a bit.
> I think you probably want a single one of these:
>
> static const struct mtk_ddp_comp_funcs ddp_ovl = {
>        .config = mtk_ovl_config,
>        .start = mtk_ovl_start,
>        .enable_vblank = mtk_ovl_enable_vblank,
>        .disable_vblank = mtk_ovl_disable_vblank,
>        .clear_vblank = mtk_ovl_clear_vblank,
>        .layer_on = mtk_ovl_layer_on,
>        .layer_off = mtk_ovl_layer_off,
>        .layer_config = mtk_ovl_layer_config,
> };

Yes, the next version will drop .comp_id from the funcs.

> and then to use it for both ovl's.  Maybe something like this:
> 
> static const struct mtk_ddp_comp_desc ddp_ovl0 = {
>   .id = DDP_COMPONENT_OVL0,
>   .ops = ddp_ovl
> };
> 
> static const struct mtk_ddp_comp_desc ddp_ovl1 = {
>   .id = DDP_COMPONENT_OVL1,
>   .ops = ddp_ovl
> };

These will not be necessary, I'll store the id in mtk_ddp_comp.

> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
> > new file mode 100644
> > index 0000000..ae3a6e8
> > --- /dev/null
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
> > @@ -0,0 +1,86 @@
[...]
> > +static void mtk_atomic_complete(struct mtk_drm_private *private,
> > +                               struct drm_atomic_state *state)
> > +{
> > +       struct drm_device *drm = private->drm;
> > +
> > +       drm_atomic_helper_commit_modeset_disables(drm, state);
> > +       drm_atomic_helper_commit_planes(drm, state);
> > +       drm_atomic_helper_commit_modeset_enables(drm, state);
> > +       drm_atomic_helper_wait_for_vblanks(drm, state);
> 
> Why wait for vblank here?
> Are you sure we waited long enough here?
> This will return after the very next vblank.
> However, what guarantees that the new state was really transferred to
> the hardware in time?

I'll check out how to use the DISP_MUTEX for this. It has an interrupt
to signal when the register settings were applied.

[...]
> > +static int mtk_atomic_commit(struct drm_device *drm,
> > +                            struct drm_atomic_state *state,
> > +                            bool async)
> > +{
> > +       struct mtk_drm_private *private = drm->dev_private;
> > +       int ret;
> > +
> > +       ret = drm_atomic_helper_prepare_planes(drm, state);
> > +       if (ret)
> > +               return ret;
> > +
> > +       mutex_lock(&private->commit.lock);
> > +       flush_work(&private->commit.work);
> > +
> > +       drm_atomic_helper_swap_state(drm, state);
> > +
> > +       if (async)
> > +               mtk_atomic_schedule(private, state);
> > +       else
> > +               mtk_atomic_complete(private, state);
> > +
> > +       mutex_unlock(&private->commit.lock);
> 
> What does this lock do?
> AFAICT, it only ensures that the second half of mtk_atomic_commit()
> cannot execute twice at the same time.
> However, I expect simultaneous calls to atomic_commit() should already
> be prohibited by the drm core?

I don't see it. DRM_IOCTL_MODE_ATOMIC is unlocked, and
drm_mode_atomic_ioctl only calls drm_modeset_acquire_init but doesn't
get any lock before calling drm_atomic_(async_)commit.

[...]
> > +static int mtk_drm_kms_init(struct drm_device *drm)
> > +{
> > +       struct mtk_drm_private *private = drm->dev_private;
> > +       struct platform_device *pdev;
> > +       int ret;
> > +       int i;
> > +
> > +       pdev = of_find_device_by_node(private->mutex_node);
> > +       if (!pdev) {
> > +               dev_err(drm->dev, "Waiting for disp-mutex device %s\n",
> > +                       private->mutex_node->full_name);
> > +               of_node_put(private->mutex_node);
> > +               return -EPROBE_DEFER;
> > +       }
> > +       private->mutex_dev = &pdev->dev;
> > +
> > +       for (i = 0; i < MAX_CRTC; i++) {
> > +               if (!private->larb_node[i])
> > +                       break;
> > +
> > +               pdev = of_find_device_by_node(private->larb_node[i]);
> > +               if (!pdev) {
> > +                       dev_err(drm->dev, "Waiting for larb device %s\n",
> > +                               private->larb_node[i]->full_name);
> 
> Nit: dev_warn() perhaps?

Ok.

[...]
> > +       for (i = 0; i < MAX_CRTC; i++) {
> > +               if (!private->larb_dev[i])
> > +                       break;
> > +
> > +               ret = mtk_smi_larb_get(private->larb_dev[i]);
> 
> Hmm. this looks like it should be done by a crtc function, and, only
> for CRTCs that are actually going to be used.

Yes, I suppose these should only be enabled while the respective DMA
components (OVL, RDMA, WDMA) are active.
I think the correct place for this would be in the component's .enable
(or a new .prepare if necessary).

> > +               if (ret) {
> > +                       DRM_ERROR("Failed to get larb: %d\n", ret);
> > +                       goto err_larb_get;
> > +               }
> > +       }
> > +
> > +       drm_kms_helper_poll_init(drm);
> > +       drm_mode_config_reset(drm);
> > +
> > +       return 0;
> > +
> > +err_larb_get:
> > +       for (i = i - 1; i >= 0; i--)
> > +               mtk_smi_larb_put(private->larb_dev[i]);
> > +       drm_kms_helper_poll_fini(drm);
> 
> Not drm_kms_helper_poll_fini().

Ok.

[...]
> > +static void mtk_drm_kms_deinit(struct drm_device *drm)
> > +{
> 
> put the larbs?

Ok, unless I move them elsewhere.

> > +       drm_kms_helper_poll_fini(drm);
> > +
> > +       drm_vblank_cleanup(drm);
> 
> unbind_all ?

Ok.

[...]
> > +static struct drm_driver mtk_drm_driver = {
> > +       .driver_features = DRIVER_MODESET | DRIVER_GEM,
> 
> | DRIVER_ATOMIC ?

Yes.

[...]
> > +static int mtk_drm_probe(struct platform_device *pdev)
> > +{
[...]
> > +       /* Iterate over sibling DISP function blocks */
> > +       for_each_child_of_node(dev->of_node->parent, node) {
> > +               const struct of_device_id *of_id;
> > +               enum mtk_ddp_comp_type comp_type;
> > +               int comp_id;
> > +
> > +               of_id = of_match_node(mtk_ddp_comp_dt_ids, node);
> > +               if (!of_id)
> > +                       continue;
> > +
> > +               comp_type = (enum mtk_ddp_comp_type)of_id->data;
> > +
> > +               if (comp_type == MTK_DISP_MUTEX) {
> > +                       private->mutex_node = of_node_get(node);
> > +                       continue;
> > +               }
> 
> I'd move the MUTEX check after "is_available()", to catch the odd case
> where the MUTEX node is disabled (it will still be NULL, and fail
> below).

Ok.

> > +
> > +               if (!of_device_is_available(node)) {
> > +                       dev_dbg(dev, "Skipping disabled component %s\n",
> > +                               node->full_name);
> > +                       continue;
> > +               }
> > +
> > +               comp_id = mtk_ddp_comp_get_id(node, comp_type);
> > +               if (comp_id < 0) {
> > +                       dev_info(dev, "Skipping unknown component %s\n",
> > +                                node->full_name);
> 
> dev_warn()

Ok.

[...]
> > +               if (comp_type == MTK_DISP_OVL) {
> > +                       struct device_node *larb_node;
> > +
> > +                       larb_node = of_parse_phandle(node, "mediatek,larb", 0);
> > +                       if (larb_node && num_larbs < MAX_CRTC)
> > +                               private->larb_node[num_larbs++] = larb_node;
> 
> It feels like the larb_nodes should be handled directly by the ovl driver.

Yes. And the rdma/wdma, probably. For that I'd like to split the ovl
driver from the crtc implementation.

> > +               }
> > +       }
> > +
> > +       if (!private->mutex_node) {
> > +               dev_err(dev, "Failed to find disp-mutex node\n");
> 
> Cleanup:
>   of_node_put() any nodes already in comp_node[]
>   Anything to clean after component_match_add()?
>   of_node_put() larb_nodes?


[...]
> > +static int mtk_drm_remove(struct platform_device *pdev)
> > +{
> > +       component_master_del(&pdev->dev, &mtk_drm_ops);
> > +       pm_runtime_disable(&pdev->dev);
> 
> Cleanup:
>   of_node_put() any nodes already in comp_node[]

Ok.

>   Anything to clean after component_match_add()?

No, that just calls devm_kmalloc internally.

>   of_node_put() larb_nodes?

Ok.

[...]
> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.h b/drivers/gpu/drm/mediatek/mtk_drm_drv.h
> > new file mode 100644
> > index 0000000..5e5128e
> > --- /dev/null
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.h
> > @@ -0,0 +1,61 @@
[...]
> > +struct mtk_drm_private {
> > +       struct drm_fb_helper *fb_helper;
> 
> Not used?

Will drop, this belongs into a separate fb_helper patch (not part of
this series).

> > +       struct drm_device *drm;
> > +
> > +       /*
> > +        * created crtc object would be contained at this array and
> > +        * this array is used to be aware of which crtc did it request vblank.
> 
> I don't understand this comment.

I'll just drop it. This array of pointers is needed to translate from
pipe number to struct mtk_drm_crtc in crtc_enable/disable_vblank.

> > +        */
> > +       struct drm_crtc *crtc[MAX_CRTC];
> > +       struct drm_property *plane_zpos_property;
> 
> Not used.

Will drop, this belongs into a separate zpos property patch (not part of
this series).

> > +       unsigned int pipe;
> 
> what pipe is this?

That should be renamed to num_pipes.

> > +       struct device_node *larb_node[MAX_CRTC];
> > +       struct device *larb_dev[MAX_CRTC];
> > +       struct device_node *mutex_node;
> > +       struct device *mutex_dev;
> > +       void __iomem *config_regs;
> > +       unsigned int path_len[MAX_CRTC];
> > +       const enum mtk_ddp_comp_id *path[MAX_CRTC];
> 
> Having 5 arrays of length MAX_CRTC suggests you really want one to
> combine these fields into a struct, and have an MAX_CRTC array of the
> struct.
> In fact, this struct sounds like it should be "mtk_crtc", which wraps
> a drm_crtc.

The larb_node/dev should be moved into DMA component drivers, but a
static path indeed currently maps directly to a crtc.

[...]
> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_fb.c b/drivers/gpu/drm/mediatek/mtk_drm_fb.c
> > new file mode 100644
> > index 0000000..dfa931b
> > --- /dev/null
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_fb.c
> > @@ -0,0 +1,151 @@
[...]
> > +struct mtk_drm_fb {
> > +       struct drm_framebuffer  base;
> > +       struct drm_gem_object   *gem_obj[MAX_FB_OBJ];
> 
> Nit: The display controller driver does not handle multi-buffer formats.
> So, maybe just add the array later when it does.
> Arguably, for now it is just adding complexity with no benefit.

Ok.

[...]
> > +struct drm_framebuffer *mtk_drm_mode_fb_create(struct drm_device *dev,
> > +                                              struct drm_file *file,
> > +                                              struct drm_mode_fb_cmd2 *cmd)
> > +{
> > +       unsigned int hsub, vsub, i;
> > +       struct mtk_drm_fb *mtk_fb;
> > +       struct drm_gem_object *gem[MAX_FB_OBJ];
> > +       int err;
> > +
> > +       hsub = drm_format_horz_chroma_subsampling(cmd->pixel_format);
> > +       vsub = drm_format_vert_chroma_subsampling(cmd->pixel_format);
> 
> nit: blank line here would help readability.
>
> > +       for (i = 0; i < drm_format_num_planes(cmd->pixel_format); i++) {

Ok.

[...]
> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_fb.h b/drivers/gpu/drm/mediatek/mtk_drm_fb.h
> > new file mode 100644
> > index 0000000..9ce7307
> > --- /dev/null
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_fb.h
> > @@ -0,0 +1,29 @@
> > +struct mtk_drm_gem_obj *mtk_drm_gem_init(struct drm_device *dev,
> > +                                        unsigned long size)
> 
> static?
> But actually, just folding this directly into mtk_drm_gem_create()
> would make the code flow clearer, since this function has no
> corresponding _fini().

Yes.

[...]
> > +struct mtk_drm_gem_obj *mtk_drm_gem_create(struct drm_device *dev,
> > +                                          unsigned long size, bool alloc_kmap)
> 
> static?

It is referenced from mtk_drm_drv.c

[...]
> > +void mtk_drm_gem_free_object(struct drm_gem_object *obj)
> > +{
> > +       struct mtk_drm_gem_obj *mtk_gem = to_mtk_gem_obj(obj);
> > +
> > +       drm_gem_free_mmap_offset(obj);
> > +
> > +       /* release file pointer to gem object. */
> > +       drm_gem_object_release(obj);
> 
> drm_gem_object_release calls drm_gem_free_mmap_offset().

Ok.

> > +
> > +       dma_free_attrs(obj->dev->dev, obj->size, mtk_gem->cookie,
> > +                      mtk_gem->dma_addr, &mtk_gem->dma_attrs);
> 
> I think you want to do this before calling drm_gem_object_release.

Why?

[...]
> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_gem.h b/drivers/gpu/drm/mediatek/mtk_drm_gem.h
> > new file mode 100644
> > index 0000000..fb7953e
> > --- /dev/null
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_gem.h
> > @@ -0,0 +1,56 @@
[...]
> > +struct mtk_drm_gem_obj {
> > +       struct drm_gem_object   base;
> > +       void __iomem            *cookie;
> > +       void __iomem            *kvaddr;
> 
> Is kvaddr really __iomem?

No, and neither is cookie.

[...]
> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.c b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> > new file mode 100644
> > index 0000000..3a8843c
> > --- /dev/null
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> > @@ -0,0 +1,167 @@
[...]
> > +static void mtk_plane_atomic_update(struct drm_plane *plane,
> > +                                   struct drm_plane_state *old_state)
> > +{
> > +       struct drm_plane_state *state = plane->state;
> > +       struct drm_gem_object *gem_obj;
> > +       struct drm_crtc *crtc = state->crtc;
> > +       struct mtk_drm_plane *mtk_plane = to_mtk_plane(plane);
> > +       struct drm_rect dest = {
> > +               .x1 = state->crtc_x,
> > +               .y1 = state->crtc_y,
> > +               .x2 = state->crtc_x + state->crtc_w,
> > +               .y2 = state->crtc_y + state->crtc_h,
> > +       };
> > +       struct drm_rect clip = { 0, };
> > +
> > +       if (!crtc)
> > +               return;
> > +
> > +       clip.x2 = state->crtc->state->mode.hdisplay;
> > +       clip.y2 = state->crtc->state->mode.vdisplay;
> > +       drm_rect_intersect(&dest, &clip);
> > +       mtk_plane->disp_size = (dest.y2 - dest.y1) << 16 | (dest.x2 - dest.x1);
> > +
> > +       plane->fb = state->fb;
> 
> This doesn't look right.  The drm core should be doing this for us.
> Why do you need this here?
> 
> I really think we want to be using a "struct mtk_plane_state" which
> wraps a drm_plane_state and accumulates our ovl specific changes.
> During init, when creating crtcs & planes, we should tell each plane
> its id, and which ovl it should use.
> The mtk_plane code can then provide a function for the vblank irq to
> apply mtk_plane's pending state.

I agree.

> > +
> > +       gem_obj = mtk_fb_get_gem_obj(state->fb, 0);
> > +       mtk_plane->flip_obj = to_mtk_gem_obj(gem_obj);
> 
> We do not need to store "flip_obj".
> We only use it twice, +3 and +5 lines down from here.
> Also, this doesn't look safe.  What if state->fb is NULL?

Since we implement atomic_disable, atomic_update should never be called
with state->fb == NULL.

> It works because to_mtk_gem_obj(NULL) == NULL, but it relies on an
> internal implementation detail - gem_obj.base is at offset 0.
> 
> > +       mtk_plane->crtc = crtc;
> 
> This isn't used, either.

Will drop.

[...]
> > +static void mtk_plane_atomic_disable(struct drm_plane *plane,
> > +                                    struct drm_plane_state *old_state)
> > +{
> > +       struct mtk_drm_plane *mtk_plane = to_mtk_plane(plane);
> > +       struct drm_crtc *crtc = old_state->crtc;
> > +
> > +       if (!crtc)
> > +               return;
> > +
> > +       mtk_drm_crtc_plane_config(crtc, mtk_plane->idx, false, 0);
> > +
> > +       mtk_drm_crtc_commit(crtc);
> 
> Why this here?  Won't this happen during mtk_drm_crtc_atomic_flush?

Because that just marks a configuration change in pending_ovl_config,
which you already suggested rightfully belongs in mtk_plane_state. I'll
fix that.

[...]
> > +int mtk_plane_init(struct drm_device *dev, struct mtk_drm_plane *mtk_plane,
> > +                  unsigned long possible_crtcs, enum drm_plane_type type,
> > +                  unsigned int zpos, unsigned int max_plane)
> 
> max_plane is not used.
> 
> Ok... enough for today :-).

This is also part of the zpos property patch, I'll drop it.

regards
Philipp




More information about the Linux-mediatek mailing list