Hi Daniel,
Am Montag, den 19.10.2015, 10:56 +0200 schrieb Daniel Vetter:
> On Fri, Oct 16, 2015 at 10:12:04PM +0200, Philipp Zabel wrote:
> > From: CK Hu <ck.hu@xxxxxxxxxxxx>
> >
> > This patch adds an initial DRM driver for the Mediatek MT8173 DISP
> > subsystem. It currently supports two fixed output streams from the
> > OVL0/OVL1 sources to the DSI0/DPI0 sinks, respectively.
> >
> > Signed-off-by: CK Hu <ck.hu@xxxxxxxxxxxx>
> > Signed-off-by: YT Shen <yt.shen@xxxxxxxxxxxx>
> > Signed-off-by: Philipp Zabel <p.zabel@xxxxxxxxxxxxxx>
>
> Bunch of drive-by comments below to point out deprecated functions and
> more common approaches used by other drivers. Don't consider this a full
> review ;-)
Much appreciated all the same.
> Cheers, Daniel
>
> > ---
> > Changes since v3:
> > - Removed crtc enabling/disabling on bind/unbind, the enable/disable
> > callbacks should suffice.
> > - Find sibling components in the device tree via compatible value
>
> btw for DT components stuff there's piles of RFCs floating around to
> extract this into helper libraries. Would be great we could push one of
> them forward.
The non-mediatek-specific part currently is the
for_each_child_of_node(bus_node, node) {
of_id = of_match_node(dt_ids, node);
if (!of_id)
continue;
if (!of_device_is_available(node))
continue;
/* ... */
}
loop. This is somewhat similar to a combination of
for_each_matching_node_and_match and for_each_available_child_of_node.
for_each_available_matching_child_of_node_and_match would be quite a
mouthful though.
[...]
> > +struct mtk_drm_crtc {
> > + struct drm_crtc base;
> > + unsigned int pipe;
> > + bool enabled;
> > + struct mtk_crtc_ddp_context *ctx;
> > +
> > + struct drm_pending_vblank_event *event;
> > + bool pending_needs_vblank;
> > +};
> > +
> > +struct mtk_crtc_ddp_context {
> > + struct device *dev;
> > + struct drm_device *drm_dev;
> > + struct mtk_drm_crtc *crtc;
> > + struct mtk_drm_plane planes[OVL_LAYER_NR];
> > + int pipe;
> > +
> > + void __iomem *config_regs;
> > + struct device *mutex_dev;
> > + u32 ddp_comp_nr;
> > + struct mtk_ddp_comp *ddp_comp;
>
> All the above probably should just be moved into mtk_drm_crtc. At least I
> don't understand why you need this indirection.
Agreed. This was needed for a debugfs patch that I have left out for
now.
> > +
> > + bool pending_config;
> > + unsigned int pending_width;
> > + unsigned int pending_height;
> > +
> > + 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];
>
> This works since you only touch these in the atomic_commit phase, but the
> recommend way to do this with atomic is to subclass drm_crtc_state:
>
> struct mtk_crtc_state {
> struct drm_crtc_state base;
>
> /* all the pending_ stuff above */
> };
>
> Then you just pass the mtk to your irq handler to do the update.
I'll move these into mtk_crtc_state, but I'm not sure what you mean with
the last sentence. Currently I pass the mtk_crtc_ddp_context to the irq
handler. I can get to the mtk_crtc_state from that.
> > +static int mtk_drm_bind(struct device *dev)
> > +{
> > + return drm_platform_init(&mtk_drm_driver, to_platform_device(dev));
>
> This is deprecated, please use drm_dev_alloc/drm_dev_register instead and
> remove your ->load driver callback.
Will replace drm_platform_init with drm_dev_alloc/drm_dev_register and
integrate mtk_drm_load into mtk_drm_bind.
> > +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;
> > +
> > + mutex_lock(&dev->struct_mutex);
>
> struct_mutex isn't needed here (gem object lookup and the vma stuff are
> all protected by looks already). Please drop it.
[...]
> > +out:
> > + drm_gem_object_unreference(obj);
>
> But then you need unreference_unlocked here.
Ok, dropped the lock.
[...]
> > +int mtk_drm_gem_mmap_buf(struct drm_gem_object *obj, struct vm_area_struct *vma)
>
> This function seems unused.
Currently unused, yes. I'll move it out of this series.
> > +{
> > + struct drm_device *drm = obj->dev;
> > + int ret;
> > +
> > + mutex_lock(&drm->struct_mutex);
> > + ret = drm_gem_mmap_obj(obj, obj->size, vma);
> > + mutex_unlock(&drm->struct_mutex);
>
> This locking isn't required with latest drm-misc anymore, please rebase
> onto latest linux-next and drop struct_mutex.
Will do.
[...]
> > +int mtk_drm_gem_mmap(struct file *filp, struct vm_area_struct *vma)
> > +{
> > + struct drm_gem_object *obj;
> > + int ret;
> > +
> > + ret = drm_gem_mmap(filp, vma);
> > + if (ret)
> > + return ret;
> > +
> > + obj = vma->vm_private_data;
> > +
> > + return mtk_drm_gem_object_mmap(obj, vma);
>
> Aside: Why can't you just use cma helpers for gem objects? CMA just
> essentially means backed by dma_alloc memory. Would avoid a lot of
> duplicated code I think.
I suppose we won't need dma_alloc memory eventually, due to the iommu.
[...]
> > +static void mtk_plane_atomic_update(struct drm_plane *plane,
> > + struct drm_plane_state *old_state)
> > +{
[...]
> > + if (plane->fb)
> > + drm_framebuffer_unreference(plane->fb);
> > + if (state->fb)
> > + drm_framebuffer_reference(state->fb);
> > + plane->fb = state->fb;
>
> There shouldn't be any need to refcount framebuffers yourself. If that's
> the case then there's a bug in the drm core/helpers.
I was still using plane->fb in mtk_drm_crtc_plane_config. Switching to
plane->state->fb there should allow me to drop this.
thanks
Philipp
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
