Re: [PATCH v9 02/14] drm/mediatek: Add DRM Driver for Mediatek SoC MT8173.

[Philipp Zabel <p.zabel@pengutronix.de>]

Hi Daniel,

Am Mittwoch, den 20.01.2016, 14:23 -0800 schrieb Daniel Kurtz:
> Hi Philipp,
> 
> This driver is looking very good now to me.
> 
> Sorry for the delay reviewing.   Some comments inline below.
[...]
> > diff --git a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> > new file mode 100644
> > index 0000000..455e62e
> > --- /dev/null
> > +++ b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
[...]
> > +/**
> > + * struct mtk_disp_ovl - DISP_OVL driver structure
> > + * @ddp_comp - structure containing type enum and hardware resources
> > + * @drm_device - backlink to allow the irq handler to find the associated crtc
> > + */
> > +struct mtk_disp_ovl {
> > +       struct mtk_ddp_comp             ddp_comp;
> > +       struct drm_device               *drm_dev;
> 
> Storing the crtc here would be more consistent with the comment above.

Ok.

> > +};
> > +
> > +static irqreturn_t mtk_disp_ovl_irq_handler(int irq, void *dev_id)
> > +{
> > +       struct mtk_disp_ovl *priv = dev_id;
> > +       struct mtk_ddp_comp *ovl = &priv->ddp_comp;
> > +
> > +       /* Clear frame completion interrupt */
> > +       writel(0x0, ovl->regs + DISP_REG_OVL_INTSTA);
> > +
> > +       mtk_crtc_ddp_irq(priv->drm_dev, ovl);
> 
> Likewise, passing the crtc here would be a bit more consistent with the name
> of this function mtk_crtc_ddp_irq().  Also, see below; if the CRTC is not found,
> we can return IRQ_NONE here to indicate that this was a spurious
> (unhandled) interrupt.

Ok.

> > +
> > +       return IRQ_HANDLED;
> > +}
> 
> [snip...]
> 
> > +static void mtk_ovl_layer_config(struct mtk_ddp_comp *comp, unsigned int idx,
> > +               struct mtk_plane_state *state)
> > +{
> > +       struct mtk_plane_pending_state *pending = &state->pending;
> > +       unsigned int addr = pending->addr;
> > +       unsigned int pitch = pending->pitch & 0xffff;
> > +       unsigned int fmt = pending->format;
> > +       unsigned int offset = (pending->y << 16) | pending->x;
> > +       unsigned int src_size = (pending->height << 16) | pending->width;
> > +       unsigned int con;
> > +
> > +       con = has_rb_swapped(fmt) << 24 | ovl_fmt_convert(fmt) << 12;
> 
> Since has_rb_swapped() and ovl_fmt_convert() can theoretically fail, but we
> can't fail here during .layer_config, can we instead do this in an atomic state
> check phase, and just store the resulting .has_rb_swapped and .ovl_infmt values
> to mtk_plane_pending_state for later application during .layer_config?

I'd rather keep the OVL specific bitfields contained in mtk_disp_ovl.c
in case we want to use RDMA planes later.

But I can change the conversion functions not to throw an error and move
the check up into the atomic plane check code.

> > +       if (idx != 0)
> > +               con |= OVL_AEN | OVL_ALPHA;
> > +
> > +       writel(con, comp->regs + DISP_REG_OVL_CON(idx));
> > +       writel(pitch, comp->regs + DISP_REG_OVL_PITCH(idx));
> > +       writel(src_size, comp->regs + DISP_REG_OVL_SRC_SIZE(idx));
> > +       writel(offset, comp->regs + DISP_REG_OVL_OFFSET(idx));
> > +       writel(addr, comp->regs + DISP_REG_OVL_ADDR(idx));
> 
> Can the above all be writel_relaxed() ?

Yes.

[...]
> > +static int mtk_crtc_ddp_hw_init(struct mtk_drm_crtc *mtk_crtc)
> > +{
[...]
> > +       mtk_disp_mutex_add_comp(mtk_crtc->mutex, mtk_crtc->ddp_comp[i]->id);
> > +       mtk_disp_mutex_enable(mtk_crtc->mutex);
> > +
> > +       DRM_DEBUG_DRIVER("ddp_disp_path_power_on %dx%d\n", width, height);
> 
> I think this debug message is now stale?

I'll remove it.

[...]
> > +void mtk_drm_crtc_check_flush(struct drm_crtc *crtc)
> > +{
> > +       struct mtk_drm_crtc *mtk_crtc = to_mtk_crtc(crtc);
> > +       int i;
> > +
> > +       if (mtk_crtc->do_flush) {
> > +               if (mtk_crtc->event)
> > +                       mtk_crtc->pending_needs_vblank = true;
> > +               for (i = 0; i < OVL_LAYER_NR; i++) {
> > +                       struct drm_plane *plane = &mtk_crtc->planes[i].base;
> > +                       struct mtk_plane_state *plane_state;
> > +
> > +                       plane_state = to_mtk_plane_state(plane->state);
> > +                       if (plane_state->pending.dirty) {
> > +                               plane_state->pending.config = true;
> > +                               plane_state->pending.dirty = false;
> > +                       }
> > +               }
> > +               mtk_crtc->pending_planes = true;
> 
> Only set pending_planes is true iff at least 1 of the ovl was
> pending.dirty, right?

Right.

[...]
> > +static void mtk_drm_crtc_atomic_flush(struct drm_crtc *crtc,
> > +                                     struct drm_crtc_state *old_crtc_state)
> > +{
> > +       struct mtk_drm_crtc *mtk_crtc = to_mtk_crtc(crtc);
> > +
> > +       mtk_crtc->do_flush = true;
> > +       mtk_drm_crtc_check_flush(crtc);
> 
> I can't figure out how this is supposed to work.
> mtk_drm_crtc_check_flush() only does something if do_flush is set.
> And, do_flush is only set here in mtk_drm_crtc_atomic_flush(), just
> above mtk_drm_crtc_check_flush(), and it is cleared by that function.
> 
> So, why do we also call mtk_drm_crtc_check_flush() from
> mtk_plane_atomic_update()?
> In what condition will the call to mtk_drm_crtc_check_flush() from
> mtk_plane_atomic_update() have do_flush == true?

Hm, me neither. With the CMDQ update this will all vanish anyway. I'll
check if I can just merge this into mtk_drm_crtc_atomic_flush() and get
rid of the call from mtk_plane_atomic_update().

[...]
> > +void mtk_crtc_ddp_irq(struct drm_device *drm_dev, struct mtk_ddp_comp *ovl)
> > +{
> > +       struct mtk_drm_private *priv = drm_dev->dev_private;
> > +       struct mtk_drm_crtc *mtk_crtc;
> > +       struct mtk_crtc_state *state;
> > +       unsigned int i;
> > +
> > +       mtk_crtc = mtk_crtc_by_src_comp(priv, ovl);
> 
> Hmm.  Can we do this lookup once and store it somewhere to avoid this
> lookup during every IRQ?

Yes. I'll store the crtc in mtk_disp_ovl as you suggested above.

> > +       if (WARN_ON(!mtk_crtc))
> > +               return;
> 
> I think the typical thing to do if we can't handle an interrupt is to
> return IRQ_NONE, so that it can be tracked by the kernel's spurious
> interrupt infrastructure.  We can probably remove the WARN_ON, too.

Ack.

[...]
> > +       if (mtk_crtc->pending_planes) {
> > +               for (i = 0; i < OVL_LAYER_NR; i++) {
> > +                       struct drm_plane *plane = &mtk_crtc->planes[i].base;
> > +                       struct mtk_plane_state *plane_state;
> > +
> > +                       plane_state = to_mtk_plane_state(plane->state);
> > +
> > +                       if (plane_state->pending.config) {
> > +                               if (!plane_state->pending.enable)
> > +                                       mtk_ddp_comp_layer_off(ovl, i);
> > +
> > +                               mtk_ddp_comp_layer_config(ovl, i, plane_state);
> > +
> > +                               if (plane_state->pending.enable)
> > +                                       mtk_ddp_comp_layer_on(ovl, i);
> 
> .layer_off() and .layer_on() are redundant, and can just be handled inside
> .layer_config(), since we pass plane_state to .layer_config() anyway.
> 
> That will also be slightly more efficient, since the mtk_ddp_comp_layer*() will
> then all be static functions in the same file and on/off can be inlined.

Ok.

[...]
> > +static struct mtk_ddp_comp_match mtk_ddp_matches[DDP_COMPONENT_ID_MAX] = {
> 
> static const struct ...

Ok.

[...]
> > +int mtk_ddp_comp_init(struct device *dev, struct device_node *node,
> > +                     struct mtk_ddp_comp *comp, enum mtk_ddp_comp_id comp_id,
> > +                     const struct mtk_ddp_comp_funcs *funcs)
> > +{
[...]
> > +       larb_node = of_parse_phandle(node, "mediatek,larb", 0);
> 
> Need to call of_node_put() for larb_node somewhere.

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..a89c7af
> > --- /dev/null
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_fb.h
> > @@ -0,0 +1,29 @@
[...]
> > +#define MAX_FB_OBJ     3
> 
> Remove from this patch

Ok.

> > +
> > +struct drm_gem_object *mtk_fb_get_gem_obj(struct drm_framebuffer *fb);
> > +int mtk_fb_wait(struct drm_framebuffer *fb);
> > +struct drm_framebuffer *mtk_drm_mode_fb_create(struct drm_device *dev,
> > +                                              struct drm_file *file,
> > +                                              struct drm_mode_fb_cmd2 *cmd);
> > +
> > +void mtk_drm_mode_output_poll_changed(struct drm_device *dev);
> > +int mtk_fbdev_create(struct drm_device *dev);
> > +void mtk_fbdev_destroy(struct drm_device *dev);
> 
> Remove these last three function prototypes from this patch.

Ok.

[...]
> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_gem.c b/drivers/gpu/drm/mediatek/mtk_drm_gem.c
> > new file mode 100644
> > index 0000000..96cc980
> > --- /dev/null
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_gem.c
> > @@ -0,0 +1,227 @@
[...]
> > +static struct mtk_drm_gem_obj *mtk_drm_gem_init(struct drm_device *dev,
> > +                                               unsigned long size)
> 
> nit: I think these "size in bytes" parameters should use size_t.

Ok.

[...]
> > +struct mtk_drm_gem_obj *mtk_drm_gem_create(struct drm_device *dev,
> > +                                          unsigned long size, bool alloc_kmap)
> > +{
[...]
> > +       mtk_gem->cookie = dma_alloc_attrs(dev->dev, obj->size,
> > +                               (dma_addr_t *)&mtk_gem->dma_addr, GFP_KERNEL,
> 
> Cast here is not necessary, since dma_addr is dma_addr_t.

Ok.

> > +                               &mtk_gem->dma_attrs);
> > +       if (!mtk_gem->cookie) {
> > +               DRM_ERROR("failed to allocate %zx byte dma buffer", obj->size);
> > +               ret = -ENOMEM;
> > +               goto err_gem_free;
> > +       }
> > +
> > +       if (alloc_kmap)
> > +               mtk_gem->kvaddr = mtk_gem->cookie;
> > +
> > +       DRM_DEBUG_DRIVER("cookie = %p dma_addr = %pad\n",
> > +                        mtk_gem->cookie, &mtk_gem->dma_addr);
> 
> Print size too.

Ok.

[...]
> > +int mtk_drm_gem_dumb_create(struct drm_file *file_priv, struct drm_device *dev,
> > +                           struct drm_mode_create_dumb *args)
> > +{
> > +       struct mtk_drm_gem_obj *mtk_gem;
> > +       int ret;
> > +
> > +       args->pitch = args->width * DIV_ROUND_UP(args->bpp, 8);
> 
> Perhaps this more correctly aligns the pitch (this is what cma does):
>   args->pitch = DIV_ROUND_UP(args->width * args->bpp, 8);

Ok.

[...]
> > +int mtk_drm_gem_dumb_map_offset(struct drm_file *file_priv,
> > +                               struct drm_device *dev, uint32_t handle,
> > +                               uint64_t *offset)
> > +{
> > +       struct drm_gem_object *obj;
> > +       int ret;
> > +
> 
> The cma helper locks struct_mutex here.

See commit 141518b64f61 ("drm/cma-helper: Don't grab dev->struct_mutex
for in mmap offset ioctl")

[...]
> > +/*
> > + * Allocate a sg_table for this GEM object.
> > + * Note: Both the table's contents, and the sg_table itself must be freed by
> > + *       the caller.
> > + * Returns a pointer to the newly allocated sg_table, or an ERR_PTR() error.
> > + */
> > +struct sg_table *mtk_gem_prime_get_sg_table(struct drm_gem_object *obj)
> > +{
> > +       struct mtk_drm_gem_obj *mtk_gem = to_mtk_gem_obj(obj);
> > +       struct drm_device *drm = obj->dev;
> > +       struct sg_table *sgt;
> > +       int ret;
> > +
> > +       sgt = kzalloc(sizeof(*sgt), GFP_KERNEL);
> > +       if (!sgt)
> > +               return ERR_PTR(-ENOMEM);
> > +
> > +       ret = dma_get_sgtable_attrs(drm->dev, sgt, mtk_gem->cookie,
> > +                                   mtk_gem->dma_addr, obj->size,
> > +                                   &mtk_gem->dma_attrs);
> > +       if (ret) {
> > +               DRM_ERROR("failed to allocate sgt, %d\n", ret);
> > +               kfree(sgt);
> > +               return ERR_PTR(ret);
> > +       }
> > +
> > +       return sgt;
> > +}
> 
> Do we also want a prime_import_sg_table() here for importing foreign allocated
> dma bufs for display?

I'll include the patch by Mao Huang next round.

[...]
> > 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..e9b6bf6
> > --- /dev/null
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> > @@ -0,0 +1,242 @@
[...]
> > +static void mtk_plane_enable(struct mtk_drm_plane *mtk_plane, bool enable,
> > +                            dma_addr_t addr, struct drm_rect *dest)
> > +{
[...]
> > +       state->pending.enable = enable;
> > +       state->pending.pitch = pitch;
> > +       state->pending.format = format;
> > +       state->pending.addr = addr;
> > +       state->pending.x = x;
> > +       state->pending.y = y;
> > +       state->pending.width = dest->x2 - dest->x1;
> > +       state->pending.height = dest->y2 - dest->y1;
> 
> wmb(); ?
> 
> BTW, it is a good idea to document all wmb() calls.
> If you forget, I believe checkpatch.pl will remind you.

Ok.

> That's it!

thanks
Philipp

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel