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

[Daniel Vetter]

On Wed, Oct 28, 2015 at 06:13:49PM +0100, Philipp Zabel wrote:
> 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.

That works too.

> > > +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. I'll check if
> switching to plane->state->fb there will allow me to drop this.

My comment was about the refcounting, not about assigning plane->fb.
Assigning plane->fb = state->fb in your commit functions is allowed (the
drm core will do it if you don't do it), but it's only allowed for
backwards compat with existing drivers. New drivers really should only
look at state->fb and totally ignore plane->fb.

The refcounting otoh is simply buggy since it'll confuse the refcounting
the core does and I think should result in some leaks even.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch