Re: [RFC v2 2/4] drm/mediatek: Add DRM Driver for Mediatek SoC MT8173.

[Philipp Zabel <p.zabel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>]

Hi Daniel,

thank you for the comments.

Am Dienstag, den 22.09.2015, 11:38 +0200 schrieb Daniel Vetter:
[...]
> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> > new file mode 100644
> > index 0000000..fc071fe
> > --- /dev/null
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> > @@ -0,0 +1,471 @@
[...]
> > +static int mtk_atomic_commit(struct drm_device *dev,
> > +			     struct drm_atomic_state *state,
> > +			     bool async)
> > +{
> > +	return drm_atomic_helper_commit(dev, state, false);
> 
> This isn't a proper async commit operation, it will still block userspace
> unecessarily. See e.g. the vc4 patches for a proper one.

I'll drop this function and assign .atomic_commit to
drm_atomic_helper_commit directly.

[...]
> > +static int mtk_plane_atomic_check(struct drm_plane *plane,
> > +		struct drm_plane_state *state)
> > +{
[...]
> > +	ret = drm_plane_helper_check_update(plane, state->crtc, fb,
> > +					    &src, &dest, &clip,
> > +					    DRM_PLANE_HELPER_NO_SCALING,
> > +					    DRM_PLANE_HELPER_NO_SCALING,
> > +					    true, true, &visible);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (!visible)
> > +		return 0;
> > +
> > +	mtk_plane->disp_size = (dest.y2 - dest.y1) << 16 | (dest.x2 - dest.x1);
> 
> I think it might work out ok but it's very fragile to update object state
> from your atomic_check hooks - atomic allows a TEST_ONLY mode and if
> that's used (generic userspace will do that a few times for each frame at
> least) then you clobber shared state. Instead it's better to store that in
> your own mtk_plane_state which subclasses drm_plane_state.

Ok, will do.

[...]
> > +static void mtk_plane_attach_zpos_property(struct drm_plane *plane,
> > +		unsigned int zpos, unsigned int max_plane)
> > +{
> > +	struct drm_device *dev = plane->dev;
> > +	struct mtk_drm_private *dev_priv = dev->dev_private;
> > +	struct drm_property *prop;
> > +
> > +	prop = dev_priv->plane_zpos_property;
> > +	if (!prop) {
> > +		prop = drm_property_create_range(dev, DRM_MODE_PROP_IMMUTABLE,
> > +						 "zpos", 0, max_plane - 1);
> 
> I know that there's lots of other drivers exposing zpos already, but I
> really think we should standardize this properly and document what it
> means. So
> - add a bit more text to the kerneldoc/doobook, especially what should
>   happen when there's a conflict in zpos.
> - move zpos registration/decoding into drm core, which means adding it to
>   drm_plane_state
> - have an open-source implementation using this somewhere (ddx, wayland,
>   hwc, ...).
> 
> I think for now it's better to drop the zpos property from initial
> mediatek enabling.

Alright, I'll separate this from the initial patchset.

best regards
Philipp

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html