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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
