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

[Philipp Zabel]

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 at mediatek.com>
> > 
> > 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 at mediatek.com>
> > Signed-off-by: YT Shen <yt.shen at mediatek.com>
> > Signed-off-by: Philipp Zabel <p.zabel at pengutronix.de>
> 
> 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