[PATCH 11/17] drm: convert plane to properties/state
Daniel Vetter
daniel at ffwll.ch
Mon May 26 07:52:29 PDT 2014
On Mon, May 26, 2014 at 07:32:13AM -0400, Rob Clark wrote:
> On Mon, May 26, 2014 at 5:12 AM, Daniel Vetter <daniel at ffwll.ch> wrote:
> > On Sat, May 24, 2014 at 02:30:20PM -0400, Rob Clark wrote:
> >> Break the mutable state of a plane out into a separate structure
> >> and use atomic properties mechanism to set plane attributes. This
> >> makes it easier to have some helpers for plane->set_property()
> >> and for checking for invalid params. The idea is that individual
> >> drivers can wrap the state struct in their own struct which adds
> >> driver specific parameters, for easy build-up of state across
> >> multiple set_property() calls and for easy atomic commit or roll-
> >> back.
> >>
> >> The same should be done for CRTC, encoder, and connector, but this
> >> patch only includes the first part (plane).
> >>
> >> Signed-off-by: Rob Clark <robdclark at gmail.com>
> >
> > Imo s/plane->id/plane->index/ since plane_id can be too easily confused
> > with the mode object id used for the idr lookup. We already have
> > drm_crtc_index, so this is established convention already.
>
> fair enough.. I suppose I could do the same in crtc.
I've forgotten to mention: We then should replace drm_crtc_index with
crtc->index since that helper is redundant.
> > A few comments below.
> >
> >> ---
> >> drivers/gpu/drm/armada/armada_overlay.c | 11 +-
> >> drivers/gpu/drm/drm_atomic.c | 225 ++++++++++++++-
> >> drivers/gpu/drm/drm_crtc.c | 433 ++++++++++++++++++++---------
> >> drivers/gpu/drm/drm_fb_helper.c | 17 +-
> >> drivers/gpu/drm/drm_plane_helper.c | 2 +
> >> drivers/gpu/drm/exynos/exynos_drm_plane.c | 7 +-
> >> drivers/gpu/drm/i915/intel_sprite.c | 1 +
> >> drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c | 6 +-
> >> drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c | 6 +-
> >> drivers/gpu/drm/nouveau/dispnv04/overlay.c | 8 +-
> >> drivers/gpu/drm/omapdrm/omap_drv.c | 2 +-
> >> drivers/gpu/drm/omapdrm/omap_plane.c | 7 +
> >> drivers/gpu/drm/rcar-du/rcar_du_plane.c | 8 +-
> >> drivers/gpu/drm/shmobile/shmob_drm_plane.c | 2 +
> >> include/drm/drm_atomic.h | 29 +-
> >> include/drm/drm_crtc.h | 114 +++++++-
> >> 16 files changed, 725 insertions(+), 153 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/armada/armada_overlay.c b/drivers/gpu/drm/armada/armada_overlay.c
> >> index 601ba9a..041ea89 100644
> >> --- a/drivers/gpu/drm/armada/armada_overlay.c
> >> +++ b/drivers/gpu/drm/armada/armada_overlay.c
> >> @@ -7,6 +7,7 @@
> >> * published by the Free Software Foundation.
> >> */
> >> #include <drm/drmP.h>
> >> +#include <drm/drm_atomic.h>
> >> #include "armada_crtc.h"
> >> #include "armada_drm.h"
> >> #include "armada_fb.h"
> >> @@ -288,7 +289,12 @@ static int armada_plane_set_property(struct drm_plane *plane,
> >> {
> >> struct armada_private *priv = plane->dev->dev_private;
> >> struct armada_plane *dplane = drm_to_armada_plane(plane);
> >> + struct drm_plane_state *pstate = drm_atomic_get_plane_state(plane, state);
> >> bool update_attr = false;
> >> + int ret = 0;
> >> +
> >> + if (IS_ERR(pstate))
> >> + return PTR_ERR(pstate);
> >>
> >> if (property == priv->colorkey_prop) {
> >> #define CCC(v) ((v) << 24 | (v) << 16 | (v) << 8)
> >> @@ -342,13 +348,16 @@ static int armada_plane_set_property(struct drm_plane *plane,
> >> } else if (property == priv->saturation_prop) {
> >> dplane->prop.saturation = val;
> >> update_attr = true;
> >> + } else {
> >> + ret = drm_plane_set_property(plane, pstate, property,
> >> + val, blob_data);
> >> }
> >>
> >> if (update_attr && dplane->base.crtc)
> >> armada_ovl_update_attr(&dplane->prop,
> >> drm_to_armada_crtc(dplane->base.crtc));
> >>
> >> - return 0;
> >> + return ret;
> >> }
> >>
> >> static const struct drm_plane_funcs armada_plane_funcs = {
> >> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> >> index 45df5e5..403ffc5 100644
> >> --- a/drivers/gpu/drm/drm_atomic.c
> >> +++ b/drivers/gpu/drm/drm_atomic.c
> >> @@ -24,6 +24,7 @@
> >>
> >> #include <drm/drmP.h>
> >> #include <drm/drm_atomic.h>
> >> +#include <drm/drm_plane_helper.h>
> >>
> >> /**
> >> * drm_atomic_begin - start a sequence of atomic updates
> >> @@ -46,10 +47,12 @@ struct drm_atomic_state *drm_atomic_begin(struct drm_device *dev,
> >> uint32_t flags)
> >> {
> >> struct drm_atomic_state *state;
> >> + int nplanes = dev->mode_config.num_total_plane;
> >> int sz;
> >> void *ptr;
> >>
> >> sz = sizeof(*state);
> >> + sz += (sizeof(state->planes) + sizeof(state->pstates)) * nplanes;
> >>
> >> ptr = kzalloc(sz, GFP_KERNEL);
> >>
> >> @@ -65,6 +68,12 @@ struct drm_atomic_state *drm_atomic_begin(struct drm_device *dev,
> >> state->dev = dev;
> >> state->flags = flags;
> >>
> >> + state->planes = ptr;
> >> + ptr = &state->planes[nplanes];
> >> +
> >> + state->pstates = ptr;
> >> + ptr = &state->pstates[nplanes];
> >> +
> >> return state;
> >> }
> >> EXPORT_SYMBOL(drm_atomic_begin);
> >> @@ -101,8 +110,20 @@ EXPORT_SYMBOL(drm_atomic_set_event);
> >> int drm_atomic_check(struct drm_device *dev, struct drm_atomic_state *state)
> >> {
> >> struct drm_atomic_state *a = state;
> >> + int nplanes = dev->mode_config.num_total_plane;
> >> + int i, ret = 0;
> >> +
> >> + for (i = 0; i < nplanes; i++) {
> >> + if (a->planes[i]) {
> >> + ret = drm_atomic_check_plane_state(a->planes[i], a->pstates[i]);
> >> + if (ret)
> >> + break;
> >> + }
> >> + }
> >> +
> >> a->acquire_ctx.frozen = true;
> >> - return 0; /* for now */
> >> +
> >> + return ret;
> >> }
> >> EXPORT_SYMBOL(drm_atomic_check);
> >>
> >> @@ -180,6 +201,18 @@ fail:
> >> static void commit_locks(struct drm_atomic_state *a,
> >> struct ww_acquire_ctx *ww_ctx)
> >> {
> >> + struct drm_device *dev = a->dev;
> >> + int nplanes = dev->mode_config.num_total_plane;
> >> + int i;
> >> +
> >> + for (i = 0; i < nplanes; i++) {
> >> + struct drm_plane *plane = a->planes[i];
> >> + if (plane) {
> >> + plane->state->state = NULL;
> >> + drm_plane_destroy_state(plane, a->pstates[i]);
> >> + }
> >> + }
> >> +
> >> /* and properly release them (clear in_atomic, remove from list): */
> >> drm_modeset_drop_locks(&a->acquire_ctx);
> >> ww_acquire_fini(ww_ctx);
> >> @@ -189,7 +222,17 @@ static void commit_locks(struct drm_atomic_state *a,
> >> static int atomic_commit(struct drm_atomic_state *a,
> >> struct ww_acquire_ctx *ww_ctx)
> >> {
> >> - int ret = 0;
> >> + int nplanes = a->dev->mode_config.num_total_plane;
> >> + int i, ret = 0;
> >> +
> >> + for (i = 0; i < nplanes; i++) {
> >> + struct drm_plane *plane = a->planes[i];
> >> + if (plane) {
> >> + ret = drm_atomic_commit_plane_state(plane, a->pstates[i]);
> >> + if (ret)
> >> + break;
> >> + }
> >> + }
> >>
> >> commit_locks(a, ww_ctx);
> >>
> >> @@ -264,7 +307,185 @@ void _drm_atomic_state_free(struct kref *kref)
> >> }
> >> EXPORT_SYMBOL(_drm_atomic_state_free);
> >>
> >> +int drm_atomic_plane_set_property(struct drm_plane *plane,
> >> + struct drm_atomic_state *state, struct drm_property *property,
> >> + uint64_t val, void *blob_data)
> >> +{
> >> + struct drm_plane_state *pstate = drm_atomic_get_plane_state(plane, state);
> >> + if (IS_ERR(pstate))
> >> + return PTR_ERR(pstate);
> >> + return drm_plane_set_property(plane, pstate, property, val, blob_data);
> >> +}
> >> +EXPORT_SYMBOL(drm_atomic_plane_set_property);
> >> +
> >> +static void init_plane_state(struct drm_plane *plane,
> >> + struct drm_plane_state *pstate, struct drm_atomic_state *state)
> >> +{
> >> + /* snapshot current state: */
> >> + *pstate = *plane->state;
> >> + pstate->state = state;
> >> + if (pstate->fb)
> >> + drm_framebuffer_reference(pstate->fb);
> >> +}
> >> +
> >> +struct drm_plane_state *
> >> +drm_atomic_get_plane_state(struct drm_plane *plane,
> >> + struct drm_atomic_state *state)
> >> +{
> >> + struct drm_atomic_state *a = state;
> >> + struct drm_plane_state *pstate;
> >> + int ret;
> >> +
> >> + pstate = a->pstates[plane->id];
> >> +
> >> + if (!pstate) {
> >> + struct drm_modeset_acquire_ctx *ctx = &a->acquire_ctx;
> >> +
> >> + /* grab lock of current crtc.. if crtc is NULL then grab all: */
> >> + if (plane->state->crtc)
> >
> > Looking here looks fishy - who's preventing someone else from touching
> > plane->state while we don't yet hold any locks? Or do I miss something big
> > here?
>
> Yeah, probably should hoist that out into a local variable to avoid
> problems on transition to null crtc. I don't think it can happen at
> the moment, but when we start to relax locking it could
So what if some manages to race a 2nd atomic modeset and flips out the
plane->state from underneath you're now chasing a freed structure?
Trying to be clever and attempting to do this without locks is imo just
not worth the (review) pain to keep it race-free.
>
> >> + ret = drm_modeset_lock(&plane->state->crtc->mutex, ctx);
> >> + else
> >> + ret = drm_modeset_lock_all_crtcs(plane->dev, ctx);
> >> + if (ret)
> >> + return ERR_PTR(ret);
> >> +
> >> + pstate = drm_plane_create_state(plane);
> >> + if (!pstate)
> >> + return ERR_PTR(-ENOMEM);
> >> + init_plane_state(plane, pstate, state);
> >> + a->planes[plane->id] = plane;
> >> + a->pstates[plane->id] = pstate;
> >> + }
> >> +
> >> + return pstate;
> >> +}
> >> +EXPORT_SYMBOL(drm_atomic_get_plane_state);
> >> +
> >> +static void
> >> +swap_plane_state(struct drm_plane *plane, struct drm_atomic_state *a)
> >> +{
> >> + struct drm_plane_state *pstate = a->pstates[plane->id];
> >> +
> >> + /* clear transient state (only valid during atomic update): */
> >> + pstate->update_plane = false;
> >> + pstate->new_fb = false;
> >> +
> >> + swap(plane->state, a->pstates[plane->id]);
> >> + plane->base.propvals = &plane->state->propvals;
> >> +}
> >> +
> >> +/* For primary plane, if the driver implements ->page_flip(), then
> >> + * we can use that. But drivers can now choose not to bother with
> >> + * implementing page_flip().
> >> + */
> >> +static bool can_flip(struct drm_plane *plane, struct drm_plane_state *pstate)
> >> +{
> >> + struct drm_crtc *crtc = pstate->crtc;
> >> + return (plane == crtc->primary) && crtc->funcs->page_flip &&
> >> + !pstate->update_plane;
> >> +}
> >> +
> >> +/* clear crtc/fb, ie. after disable_plane(). But takes care to keep
> >> + * the property state in sync. Once we get rid of plane->crtc/fb ptrs
> >> + * and just use state, we can get rid of this fxn:
> >> + */
> >> +static void
> >> +reset_plane(struct drm_plane *plane, struct drm_plane_state *pstate)
> >> +{
> >> + struct drm_mode_config *config = &plane->dev->mode_config;
> >> + drm_plane_set_property(plane, pstate, config->prop_fb_id, 0, NULL);
> >> + drm_plane_set_property(plane, pstate, config->prop_crtc_id, 0, NULL);
> >> + plane->crtc = NULL;
> >> + plane->fb = NULL;
> >> +}
> >> +
> >> +static int
> >> +commit_plane_state(struct drm_plane *plane, struct drm_plane_state *pstate)
> >> +{
> >> + struct drm_atomic_state *a = pstate->state;
> >> + struct drm_framebuffer *old_fb = plane->fb;
> >> + struct drm_framebuffer *fb = pstate->fb;
> >> + bool enabled = pstate->crtc && fb;
> >> + int ret = 0;
> >> +
> >> + if (fb)
> >> + drm_framebuffer_reference(fb);
> >> +
> >> + if (!enabled) {
> >> + if ((plane->type == DRM_PLANE_TYPE_PRIMARY) &&
> >> + (plane->funcs == &drm_primary_helper_funcs)) {
> >> + /* primary plane helpers don't like ->disable_plane()..
> >> + * so this hack for now until someone comes up with
> >> + * something better:
> >> + */
> >
> > Imo that's something we could check for in ->check and reject early.
>
> well, we aren't actually treating it as a failure. If we did want to
> treat it as an error, then yes, we should fail it in ->check(). I'm
> going to let someone who actually uses primary planes helpers decide
> how they want it and change it if necessary.
Atm all drivers use the primary plane helpers. And I guess for a long time
most will stick to them, so I think we should have an answer here.
Rejecting the modeset sounds like the right one, since after all arbitrary
driver restrictions is what we have the ->check hook for.
> >> + ret = 0;
> >> + } else {
> >> + ret = plane->funcs->disable_plane(plane);
> >> + reset_plane(plane, pstate);
> >> + }
> >> + } else {
> >> + struct drm_crtc *crtc = pstate->crtc;
> >> + if (pstate->update_plane ||
> >> + (pstate->new_fb && !can_flip(plane, pstate))) {
> >> + ret = plane->funcs->update_plane(plane, crtc, pstate->fb,
> >> + pstate->crtc_x, pstate->crtc_y,
> >> + pstate->crtc_w, pstate->crtc_h,
> >> + pstate->src_x, pstate->src_y,
> >> + pstate->src_w, pstate->src_h);
> >> + if (ret == 0) {
> >> + /*
> >> + * For page_flip(), the driver does this, but for
> >> + * update_plane() it doesn't.. hurray \o/
> >> + */
> >
> > Imo a patch to unify this first wouldn't hurt ...
>
> I was kinda leaning more towards introducing a new API with unified
> behaviour, and deprecating the old eventually. Doing it all at once
> and not having someone who is more expert in each different drivers,
> seems like too big a chance to introduce problems.
>
> I kinda want to more to an api that is more like atomic_commit() on a
> per object basis (ie. {crtc,plane,etc}->atomic_commit()). Then driver
> can hook in to the commit process at device level
> (dev->atomic_commit()) and/or per-object level as needed.
>
> So yes, I want to unify.. but by building on top of atomic and
> deprecating the old.
I only meant to unify the refcounting and updating rules for obj->fb and
similar. Like I've done for update_plane and set_crtc as part of Matt's
primary plane work. Unifying all the flip interfaces is seriously out of
scope ;-)
Also I really don't think you can make atomic pageflips work in a generic
fashion with per-obj callbacks. After all the entire point is to have an
atomic, global update, and the means (or lack thereof) the hw provides to
make this happen will be different everywhere.
Atomic modesets (i.e. "do we have sufficient plls for this output config")
are a bit different and for those it might work. But that should clearly
be part of some helper library.
Yet another reason why imo it's really important to make a clear
disdinction between atomic modesets (which we can mostly do for everyone
using crtc helpers) and atomic/nuclear pageflips (for which we completely
lack the unified driver interface and for which a per-obj ->commit hook is
imo useless).
> >> + plane->crtc = crtc;
> >> + plane->fb = fb;
> >> + fb = NULL; /* don't unref */
> >> + }
> >> +
> >> + } else if (pstate->new_fb) {
> >> + ret = crtc->funcs->page_flip(crtc, fb, NULL, a->flags);
> >> + if (ret == 0) {
> >> + /*
> >> + * Warn if the driver hasn't properly updated the plane->fb
> >> + * field to reflect that the new framebuffer is now used.
> >> + * Failing to do so will screw with the reference counting
> >> + * on framebuffers.
> >> + */
> >> + WARN_ON(plane->fb != fb);
> >> + fb = NULL; /* don't unref */
> >> + }
> >> + } else {
> >> + old_fb = NULL;
> >> + ret = 0;
> >> + }
> >> + }
> >
> > This entire logic here kinda raises the question about transitioning
> > drivers to the atomic interfaces. For modeset operations it might work
> > fairly well since everything but i915 uses the crtc helpers and so can be
> > converted fairly easily.
> >
> > But doing nuclear pageflips, even more so if we want completion events is
> > an entire new deal. No idea how that should work really.
>
> Currently we only have completion events on CRTCs, but we are going to
> need no APIs internally for completion events on planes. Which might
> end up being the motivation for plane->atomic_commit() API.
>
> At an rate, that should not hold up this patchset, or even adding
> atomic ioctl itself (as long as we don't expose new events yet, which
> is something we could leave out of the first version).
Hm, what's the use-case for per-plane completion events? Userspace can
always multiplex it when they want, and one single nuclear pageflip
certainly shouldn't result in different vblank events for different planes
on the same crtc. Otherwise it's just not atomic enough ;-)
Imo the pain we gain in making this generic (I've commented about this in
other places) doesn't justify the possible gain. Especially since I don't
see a real use case.
I think we should have a NUCLEAR_FLIP option in the atomic ioctl which
requests that:
- all updates happen on one vblank
- all updates happen asynchronously
and promises that the update only concerns one crtc. All drivers without
support for it (i.e. atm everything, hopefully soonish i915 will have
support for this) would then reject this.
Which means that for the start atomic updates would only work for
modesets, but I also think that's about as good as it gets without
driver-specific work anyway.
> >> +
> >> + if (ret) {
> >> + /* Keep the old fb, don't unref it. */
> >> + old_fb = NULL;
> >> + } else {
> >> + /* on success, update state and fb refcnting: */
> >> + /* NOTE: if we ensure no driver sets plane->state->fb = NULL
> >> + * on disable, we can move this up a level and not duplicate
> >> + * nearly the same thing for both update_plane and disable_plane
> >> + * cases.. I leave it like this for now to be paranoid due to
> >> + * the slightly different ordering in the two cases in the
> >> + * original code.
> >> + */
> >> + swap_plane_state(plane, pstate->state);
> >> + }
> >> +
> >> +
> >> + if (fb)
> >> + drm_framebuffer_unreference(fb);
> >> + if (old_fb)
> >> + drm_framebuffer_unreference(old_fb);
> >> +
> >> + return ret;
> >> +}
> >>
> >> const struct drm_atomic_funcs drm_atomic_funcs = {
> >> + .check_plane_state = drm_plane_check_state,
> >> + .commit_plane_state = commit_plane_state,
> >> };
> >> EXPORT_SYMBOL(drm_atomic_funcs);
> >> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> >> index 48555724..b556a31 100644
> >> --- a/drivers/gpu/drm/drm_crtc.c
> >> +++ b/drivers/gpu/drm/drm_crtc.c
> >> @@ -712,6 +712,18 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb)
> >> * in this manner.
> >> */
> >> if (atomic_read(&fb->refcount.refcount) > 1) {
> >> + void *state;
> >> +
> >> + state = dev->driver->atomic_begin(dev, 0);
> >> + if (IS_ERR(state)) {
> >> + DRM_ERROR("failed to disable crtc and/or plane when fb was deleted\n");
> >> + return;
> >> + }
> >> +
> >> + /* TODO once CRTC is converted to state/properties, we can push the
> >> + * locking down into drm_atomic_commit(), since that is where
> >> + * the actual changes take place..
> >> + */
> >> drm_modeset_lock_all(dev);
> >> /* remove from any CRTC */
> >> list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
> >> @@ -728,8 +740,17 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb)
> >>
> >> list_for_each_entry(plane, &dev->mode_config.plane_list, head) {
> >> if (plane->fb == fb)
> >> - drm_plane_force_disable(plane);
> >> + drm_plane_force_disable(plane, state);
> >> }
> >> +
> >> + /* just disabling stuff shouldn't fail, hopefully: */
> >> + if(dev->driver->atomic_check(dev, state))
> >> + DRM_ERROR("failed to disable crtc and/or plane when fb was deleted\n");
> >> + else
> >> + dev->driver->atomic_commit(dev, state);
> >> +
> >> + dev->driver->atomic_end(dev, state);
> >> +
> >> drm_modeset_unlock_all(dev);
> >> }
> >>
> >> @@ -1090,18 +1111,23 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
> >> const uint32_t *formats, uint32_t format_count,
> >> enum drm_plane_type type)
> >> {
> >> + struct drm_mode_config *config = &dev->mode_config;
> >> int ret;
> >>
> >> + /* this is now required: */
> >> + WARN_ON(!funcs->set_property);
> >> +
> >> drm_modeset_lock_all(dev);
> >>
> >> ret = drm_mode_object_get(dev, &plane->base, DRM_MODE_OBJECT_PLANE);
> >> if (ret)
> >> goto out;
> >>
> >> + plane->funcs = funcs;
> >> + plane->state = drm_plane_create_state(plane);
> >> plane->base.properties = &plane->properties;
> >> - plane->base.propvals = &plane->propvals;
> >> + plane->base.propvals = &plane->state->propvals;
> >> plane->dev = dev;
> >> - plane->funcs = funcs;
> >> plane->format_types = kmalloc(sizeof(uint32_t) * format_count,
> >> GFP_KERNEL);
> >> if (!plane->format_types) {
> >> @@ -1116,15 +1142,27 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
> >> plane->possible_crtcs = possible_crtcs;
> >> plane->type = type;
> >>
> >> - list_add_tail(&plane->head, &dev->mode_config.plane_list);
> >> - dev->mode_config.num_total_plane++;
> >> + list_add_tail(&plane->head, &config->plane_list);
> >> + plane->id = config->num_total_plane;
> >> + config->num_total_plane++;
> >> if (plane->type == DRM_PLANE_TYPE_OVERLAY)
> >> - dev->mode_config.num_overlay_plane++;
> >> + config->num_overlay_plane++;
> >>
> >> drm_object_attach_property(&plane->base,
> >> - dev->mode_config.plane_type_property,
> >> + config->plane_type_property,
> >> plane->type);
> >>
> >> + drm_object_attach_property(&plane->base, config->prop_fb_id, 0);
> >> + drm_object_attach_property(&plane->base, config->prop_crtc_id, 0);
> >> + drm_object_attach_property(&plane->base, config->prop_crtc_x, 0);
> >> + drm_object_attach_property(&plane->base, config->prop_crtc_y, 0);
> >> + drm_object_attach_property(&plane->base, config->prop_crtc_w, 0);
> >> + drm_object_attach_property(&plane->base, config->prop_crtc_h, 0);
> >> + drm_object_attach_property(&plane->base, config->prop_src_x, 0);
> >> + drm_object_attach_property(&plane->base, config->prop_src_y, 0);
> >> + drm_object_attach_property(&plane->base, config->prop_src_w, 0);
> >> + drm_object_attach_property(&plane->base, config->prop_src_h, 0);
> >> +
> >> out:
> >> drm_modeset_unlock_all(dev);
> >>
> >> @@ -1185,10 +1223,151 @@ void drm_plane_cleanup(struct drm_plane *plane)
> >> dev->mode_config.num_total_plane--;
> >> if (plane->type == DRM_PLANE_TYPE_OVERLAY)
> >> dev->mode_config.num_overlay_plane--;
> >> + drm_plane_destroy_state(plane, plane->state);
> >> drm_modeset_unlock_all(dev);
> >> }
> >> EXPORT_SYMBOL(drm_plane_cleanup);
> >>
> >> +int drm_plane_check_state(struct drm_plane *plane,
> >> + struct drm_plane_state *state)
> >> +{
> >> + unsigned int fb_width, fb_height;
> >> + struct drm_framebuffer *fb = state->fb;
> >> + int i;
> >> +
> >> + /* disabling the plane is allowed: */
> >> + if (!fb)
> >> + return 0;
> >> +
> >> + fb_width = fb->width << 16;
> >> + fb_height = fb->height << 16;
> >> +
> >> + /* Check whether this plane supports the fb pixel format. */
> >> + for (i = 0; i < plane->format_count; i++)
> >> + if (fb->pixel_format == plane->format_types[i])
> >> + break;
> >> + if (i == plane->format_count) {
> >> + DRM_DEBUG_KMS("Invalid pixel format 0x%08x\n", fb->pixel_format);
> >> + return -EINVAL;
> >> + }
> >> +
> >> + /* Make sure source coordinates are inside the fb. */
> >> + if (state->src_w > fb_width ||
> >> + state->src_x > fb_width - state->src_w ||
> >> + state->src_h > fb_height ||
> >> + state->src_y > fb_height - state->src_h) {
> >> + DRM_DEBUG_KMS("Invalid source coordinates "
> >> + "%u.%06ux%u.%06u+%u.%06u+%u.%06u\n",
> >> + state->src_w >> 16,
> >> + ((state->src_w & 0xffff) * 15625) >> 10,
> >> + state->src_h >> 16,
> >> + ((state->src_h & 0xffff) * 15625) >> 10,
> >> + state->src_x >> 16,
> >> + ((state->src_x & 0xffff) * 15625) >> 10,
> >> + state->src_y >> 16,
> >> + ((state->src_y & 0xffff) * 15625) >> 10);
> >> + return -ENOSPC;
> >> + }
> >> +
> >> + /* Give drivers some help against integer overflows */
> >> + if (state->crtc_w > INT_MAX ||
> >> + state->crtc_x > INT_MAX - (int32_t) state->crtc_w ||
> >> + state->crtc_h > INT_MAX ||
> >> + state->crtc_y > INT_MAX - (int32_t) state->crtc_h) {
> >> + DRM_DEBUG_KMS("Invalid CRTC coordinates %ux%u+%d+%d\n",
> >> + state->crtc_w, state->crtc_h,
> >> + state->crtc_x, state->crtc_y);
> >> + return -ERANGE;
> >> + }
> >> +
> >> + return 0;
> >> +}
> >> +EXPORT_SYMBOL(drm_plane_check_state);
> >> +
> >> +void drm_plane_commit_state(struct drm_plane *plane,
> >> + struct drm_plane_state *state)
> >> +{
> >> + plane->state = state;
> >> + plane->base.propvals = &state->propvals;
> >> +}
> >> +EXPORT_SYMBOL(drm_plane_commit_state);
> >> +
> >> +int drm_plane_set_property(struct drm_plane *plane,
> >> + struct drm_plane_state *state,
> >> + struct drm_property *property,
> >> + uint64_t value, void *blob_data)
> >> +{
> >> + struct drm_device *dev = plane->dev;
> >> + struct drm_mode_config *config = &dev->mode_config;
> >> +
> >> + drm_object_property_set_value(&plane->base,
> >> + &state->propvals, property, value, blob_data);
> >> +
> >> + if (property == config->prop_fb_id) {
> >> + struct drm_framebuffer *old_fb = state->fb;
> >> + /*
> >> + * NOTE: the ref to the fb could have been lost between
> >> + * drm_property_change_is_valid() and now. The upshot
> >> + * is that drm_framebuffer_lookup() could return NULL
> >> + * and we'd disable the plane.
> >> + *
> >> + * We *could* return an error in that case. But if (for
> >> + * example) _setcrtc() raced with _rmfb() and _rmfb()
> >> + * came after, it would disable what was enabled in the
> >> + * _setcrtc(). Which is the same end result that we get
> >> + * here, just skipping briefly setting the mode.
> >> + */
> >> + state->fb = drm_framebuffer_lookup(dev, value);
> >> + if (old_fb)
> >> + drm_framebuffer_unreference(old_fb);
> >> + state->new_fb = true;
> >> + } else if (property == config->prop_crtc_id) {
> >> + struct drm_mode_object *obj = drm_property_get_obj(property, value);
> >> + struct drm_crtc *crtc = obj ? obj_to_crtc(obj) : NULL;
> >> + /* take the lock of the incoming crtc as well, moving
> >> + * plane between crtcs is synchronized on both incoming
> >> + * and outgoing crtc.
> >> + */
> >> + if (crtc) {
> >> + struct drm_atomic_state *a = state->state;
> >> + int ret = drm_modeset_lock(&crtc->mutex, &a->acquire_ctx);
> >> + if (ret)
> >> + return ret;
> >> + }
> >> + state->crtc = crtc;
> >> + state->update_plane = true;
> >> + } else if (property == config->prop_crtc_x) {
> >> + state->crtc_x = U642I64(value);
> >> + state->update_plane = true;
> >> + } else if (property == config->prop_crtc_y) {
> >> + state->crtc_y = U642I64(value);
> >> + state->update_plane = true;
> >> + } else if (property == config->prop_crtc_w) {
> >> + state->crtc_w = value;
> >> + state->update_plane = true;
> >> + } else if (property == config->prop_crtc_h) {
> >> + state->crtc_h = value;
> >> + state->update_plane = true;
> >> + } else if (property == config->prop_src_x) {
> >> + state->src_x = value;
> >> + state->update_plane = true;
> >> + } else if (property == config->prop_src_y) {
> >> + state->src_y = value;
> >> + state->update_plane = true;
> >> + } else if (property == config->prop_src_w) {
> >> + state->src_w = value;
> >> + state->update_plane = true;
> >> + } else if (property == config->prop_src_h) {
> >> + state->src_h = value;
> >> + state->update_plane = true;
> >> + } else {
> >> + return -EINVAL;
> >> + }
> >> +
> >> + return 0;
> >> +}
> >> +EXPORT_SYMBOL(drm_plane_set_property);
> >> +
> >> /**
> >> * drm_plane_force_disable - Forcibly disable a plane
> >> * @plane: plane to disable
> >> @@ -1198,43 +1377,93 @@ EXPORT_SYMBOL(drm_plane_cleanup);
> >> * Used when the plane's current framebuffer is destroyed,
> >> * and when restoring fbdev mode.
> >> */
> >> -void drm_plane_force_disable(struct drm_plane *plane)
> >> +void drm_plane_force_disable(struct drm_plane *plane,
> >> + struct drm_atomic_state *state)
> >> {
> >> - struct drm_framebuffer *old_fb = plane->fb;
> >> - int ret;
> >> + struct drm_mode_config *config = &plane->dev->mode_config;
> >>
> >> - if (!old_fb)
> >> - return;
> >> -
> >> - ret = plane->funcs->disable_plane(plane);
> >> - if (ret) {
> >> - DRM_ERROR("failed to disable plane with busy fb\n");
> >> - return;
> >> - }
> >> - /* disconnect the plane from the fb and crtc: */
> >> - __drm_framebuffer_unreference(old_fb);
> >> - plane->fb = NULL;
> >> - plane->crtc = NULL;
> >> + /* should turn off the crtc */
> >> + drm_mode_plane_set_obj_prop(plane, state,
> >> + config->prop_crtc_id, 0, NULL);
> >> + drm_mode_plane_set_obj_prop(plane, state,
> >> + config->prop_fb_id, 0, NULL);
> >> }
> >> EXPORT_SYMBOL(drm_plane_force_disable);
> >>
> >> static int drm_mode_create_standard_connector_properties(struct drm_device *dev)
> >> {
> >> - struct drm_property *edid;
> >> - struct drm_property *dpms;
> >> + struct drm_property *prop;
> >>
> >> /*
> >> * Standard properties (apply to all connectors)
> >> */
> >> - edid = drm_property_create(dev, DRM_MODE_PROP_BLOB |
> >> + prop = drm_property_create(dev, DRM_MODE_PROP_BLOB |
> >> DRM_MODE_PROP_IMMUTABLE,
> >> "EDID", 0);
> >> - dev->mode_config.edid_property = edid;
> >> + if (!prop)
> >> + return -ENOMEM;
> >> + dev->mode_config.edid_property = prop;
> >>
> >> - dpms = drm_property_create_enum(dev, 0,
> >> + prop = drm_property_create_enum(dev, 0,
> >> "DPMS", drm_dpms_enum_list,
> >> ARRAY_SIZE(drm_dpms_enum_list));
> >> - dev->mode_config.dpms_property = dpms;
> >> + if (!prop)
> >> + return -ENOMEM;
> >> + dev->mode_config.dpms_property = prop;
> >> +
> >> +
> >> + prop = drm_property_create_range(dev, 0, "SRC_X", 0, UINT_MAX);
> >> + if (!prop)
> >> + return -ENOMEM;
> >> + dev->mode_config.prop_src_x = prop;
> >> +
> >> + prop = drm_property_create_range(dev, 0, "SRC_Y", 0, UINT_MAX);
> >> + if (!prop)
> >> + return -ENOMEM;
> >> + dev->mode_config.prop_src_y = prop;
> >> +
> >> + prop = drm_property_create_range(dev, 0, "SRC_W", 0, UINT_MAX);
> >> + if (!prop)
> >> + return -ENOMEM;
> >> + dev->mode_config.prop_src_w = prop;
> >> +
> >> + prop = drm_property_create_range(dev, 0, "SRC_H", 0, UINT_MAX);
> >> + if (!prop)
> >> + return -ENOMEM;
> >> + dev->mode_config.prop_src_h = prop;
> >> +
> >> + prop = drm_property_create_signed_range(dev, 0, "CRTC_X",
> >> + INT_MIN, INT_MAX);
> >> + if (!prop)
> >> + return -ENOMEM;
> >> + dev->mode_config.prop_crtc_x = prop;
> >> +
> >> + prop = drm_property_create_signed_range(dev, 0, "CRTC_Y",
> >> + INT_MIN, INT_MAX);
> >> + if (!prop)
> >> + return -ENOMEM;
> >> + dev->mode_config.prop_crtc_y = prop;
> >> +
> >> + prop = drm_property_create_range(dev, 0, "CRTC_W", 0, INT_MAX);
> >> + if (!prop)
> >> + return -ENOMEM;
> >> + dev->mode_config.prop_crtc_w = prop;
> >> +
> >> + prop = drm_property_create_range(dev, 0, "CRTC_H", 0, INT_MAX);
> >> + if (!prop)
> >> + return -ENOMEM;
> >> + dev->mode_config.prop_crtc_h = prop;
> >> +
> >> + prop = drm_property_create_object(dev, 0, "FB_ID", DRM_MODE_OBJECT_FB);
> >> + if (!prop)
> >> + return -ENOMEM;
> >> + dev->mode_config.prop_fb_id = prop;
> >> +
> >> + prop = drm_property_create_object(dev, 0,
> >> + "CRTC_ID", DRM_MODE_OBJECT_CRTC);
> >> + if (!prop)
> >> + return -ENOMEM;
> >> + dev->mode_config.prop_crtc_id = prop;
> >>
> >> return 0;
> >> }
> >> @@ -2140,20 +2369,19 @@ int drm_mode_setplane(struct drm_device *dev, void *data,
> >> struct drm_file *file_priv)
> >> {
> >> struct drm_mode_set_plane *plane_req = data;
> >> + struct drm_mode_config *config = &dev->mode_config;
> >> struct drm_plane *plane;
> >> - struct drm_crtc *crtc;
> >> - struct drm_framebuffer *fb = NULL, *old_fb = NULL;
> >> + struct drm_atomic_state *state;
> >> int ret = 0;
> >> - unsigned int fb_width, fb_height;
> >> - int i;
> >>
> >> if (!drm_core_check_feature(dev, DRIVER_MODESET))
> >> return -EINVAL;
> >>
> >> - /*
> >> - * First, find the plane, crtc, and fb objects. If not available,
> >> - * we don't bother to call the driver.
> >> - */
> >> +retry:
> >> + state = dev->driver->atomic_begin(dev, 0);
> >> + if (IS_ERR(state))
> >> + return PTR_ERR(state);
> >> +
> >> plane = drm_plane_find(dev, plane_req->plane_id);
> >> if (!plane) {
> >> DRM_DEBUG_KMS("Unknown plane ID %d\n",
> >> @@ -2161,104 +2389,37 @@ int drm_mode_setplane(struct drm_device *dev, void *data,
> >> return -ENOENT;
> >> }
> >>
> >> - /* No fb means shut it down */
> >> - if (!plane_req->fb_id) {
> >> - drm_modeset_lock_all(dev);
> >> - old_fb = plane->fb;
> >> - ret = plane->funcs->disable_plane(plane);
> >> - if (!ret) {
> >> - plane->crtc = NULL;
> >> - plane->fb = NULL;
> >> - } else {
> >> - old_fb = NULL;
> >> - }
> >> - drm_modeset_unlock_all(dev);
> >> - goto out;
> >> - }
> >> -
> >> - crtc = drm_crtc_find(dev, plane_req->crtc_id);
> >> - if (!crtc) {
> >> - DRM_DEBUG_KMS("Unknown crtc ID %d\n",
> >> - plane_req->crtc_id);
> >> - ret = -ENOENT;
> >> - goto out;
> >> - }
> >> -
> >> - fb = drm_framebuffer_lookup(dev, plane_req->fb_id);
> >> - if (!fb) {
> >> - DRM_DEBUG_KMS("Unknown framebuffer ID %d\n",
> >> - plane_req->fb_id);
> >> - ret = -ENOENT;
> >> - goto out;
> >> - }
> >> -
> >> - /* Check whether this plane supports the fb pixel format. */
> >> - for (i = 0; i < plane->format_count; i++)
> >> - if (fb->pixel_format == plane->format_types[i])
> >> - break;
> >> - if (i == plane->format_count) {
> >> - DRM_DEBUG_KMS("Invalid pixel format %s\n",
> >> - drm_get_format_name(fb->pixel_format));
> >> - ret = -EINVAL;
> >> - goto out;
> >> - }
> >> -
> >> - fb_width = fb->width << 16;
> >> - fb_height = fb->height << 16;
> >> -
> >> - /* Make sure source coordinates are inside the fb. */
> >> - if (plane_req->src_w > fb_width ||
> >> - plane_req->src_x > fb_width - plane_req->src_w ||
> >> - plane_req->src_h > fb_height ||
> >> - plane_req->src_y > fb_height - plane_req->src_h) {
> >> - DRM_DEBUG_KMS("Invalid source coordinates "
> >> - "%u.%06ux%u.%06u+%u.%06u+%u.%06u\n",
> >> - plane_req->src_w >> 16,
> >> - ((plane_req->src_w & 0xffff) * 15625) >> 10,
> >> - plane_req->src_h >> 16,
> >> - ((plane_req->src_h & 0xffff) * 15625) >> 10,
> >> - plane_req->src_x >> 16,
> >> - ((plane_req->src_x & 0xffff) * 15625) >> 10,
> >> - plane_req->src_y >> 16,
> >> - ((plane_req->src_y & 0xffff) * 15625) >> 10);
> >> - ret = -ENOSPC;
> >> - goto out;
> >> - }
> >> -
> >> - /* Give drivers some help against integer overflows */
> >> - if (plane_req->crtc_w > INT_MAX ||
> >> - plane_req->crtc_x > INT_MAX - (int32_t) plane_req->crtc_w ||
> >> - plane_req->crtc_h > INT_MAX ||
> >> - plane_req->crtc_y > INT_MAX - (int32_t) plane_req->crtc_h) {
> >> - DRM_DEBUG_KMS("Invalid CRTC coordinates %ux%u+%d+%d\n",
> >> - plane_req->crtc_w, plane_req->crtc_h,
> >> - plane_req->crtc_x, plane_req->crtc_y);
> >> - ret = -ERANGE;
> >> + ret =
> >> + drm_mode_plane_set_obj_prop(plane, state,
> >> + config->prop_crtc_id, plane_req->crtc_id, NULL) ||
> >> + drm_mode_plane_set_obj_prop(plane, state,
> >> + config->prop_fb_id, plane_req->fb_id, NULL) ||
> >> + drm_mode_plane_set_obj_prop(plane, state,
> >> + config->prop_crtc_x, I642U64(plane_req->crtc_x), NULL) ||
> >> + drm_mode_plane_set_obj_prop(plane, state,
> >> + config->prop_crtc_y, I642U64(plane_req->crtc_y), NULL) ||
> >> + drm_mode_plane_set_obj_prop(plane, state,
> >> + config->prop_crtc_w, plane_req->crtc_w, NULL) ||
> >> + drm_mode_plane_set_obj_prop(plane, state,
> >> + config->prop_crtc_h, plane_req->crtc_h, NULL) ||
> >> + drm_mode_plane_set_obj_prop(plane, state,
> >> + config->prop_src_w, plane_req->src_w, NULL) ||
> >> + drm_mode_plane_set_obj_prop(plane, state,
> >> + config->prop_src_h, plane_req->src_h, NULL) ||
> >> + drm_mode_plane_set_obj_prop(plane, state,
> >> + config->prop_src_x, plane_req->src_x, NULL) ||
> >> + drm_mode_plane_set_obj_prop(plane, state,
> >> + config->prop_src_y, plane_req->src_y, NULL) ||
> >> + dev->driver->atomic_check(dev, state);
> >> + if (ret)
> >> goto out;
> >> - }
> >>
> >> - drm_modeset_lock_all(dev);
> >> - old_fb = plane->fb;
> >> - ret = plane->funcs->update_plane(plane, crtc, fb,
> >> - plane_req->crtc_x, plane_req->crtc_y,
> >> - plane_req->crtc_w, plane_req->crtc_h,
> >> - plane_req->src_x, plane_req->src_y,
> >> - plane_req->src_w, plane_req->src_h);
> >> - if (!ret) {
> >> - plane->crtc = crtc;
> >> - plane->fb = fb;
> >> - fb = NULL;
> >> - } else {
> >> - old_fb = NULL;
> >> - }
> >> - drm_modeset_unlock_all(dev);
> >> + ret = dev->driver->atomic_commit(dev, state);
> >>
> >> out:
> >> - if (fb)
> >> - drm_framebuffer_unreference(fb);
> >> - if (old_fb)
> >> - drm_framebuffer_unreference(old_fb);
> >> -
> >> + dev->driver->atomic_end(dev, state);
> >> + if (ret == -EDEADLK)
> >> + goto retry;
> >> return ret;
> >> }
> >>
> >> @@ -3833,7 +3994,7 @@ int drm_mode_connector_property_set_ioctl(struct drm_device *dev,
> >> return drm_mode_obj_set_property_ioctl(dev, &obj_set_prop, file_priv);
> >> }
> >>
> >> -static int drm_mode_connector_set_obj_prop(struct drm_connector *connector,
> >> +int drm_mode_connector_set_obj_prop(struct drm_connector *connector,
> >> struct drm_atomic_state *state, struct drm_property *property,
> >> uint64_t value, void *blob_data)
> >> {
> >> @@ -3856,8 +4017,9 @@ static int drm_mode_connector_set_obj_prop(struct drm_connector *connector,
> >>
> >> return ret;
> >> }
> >> +EXPORT_SYMBOL(drm_mode_connector_set_obj_prop);
> >>
> >> -static int drm_mode_crtc_set_obj_prop(struct drm_crtc *crtc,
> >> +int drm_mode_crtc_set_obj_prop(struct drm_crtc *crtc,
> >> struct drm_atomic_state *state, struct drm_property *property,
> >> uint64_t value, void *blob_data)
> >> {
> >> @@ -3872,8 +4034,9 @@ static int drm_mode_crtc_set_obj_prop(struct drm_crtc *crtc,
> >>
> >> return ret;
> >> }
> >> +EXPORT_SYMBOL(drm_mode_crtc_set_obj_prop);
> >>
> >> -static int drm_mode_plane_set_obj_prop(struct drm_plane *plane,
> >> +int drm_mode_plane_set_obj_prop(struct drm_plane *plane,
> >> struct drm_atomic_state *state, struct drm_property *property,
> >> uint64_t value, void *blob_data)
> >> {
> >> @@ -3882,12 +4045,10 @@ static int drm_mode_plane_set_obj_prop(struct drm_plane *plane,
> >> if (plane->funcs->set_property)
> >> ret = plane->funcs->set_property(plane, state, property,
> >> value, blob_data);
> >> - if (!ret)
> >> - drm_object_property_set_value(&plane->base, &plane->propvals,
> >> - property, value, NULL);
> >>
> >> return ret;
> >> }
> >> +EXPORT_SYMBOL(drm_mode_plane_set_obj_prop);
> >>
> >> static int drm_mode_set_obj_prop(struct drm_mode_object *obj,
> >> struct drm_atomic_state *state, struct drm_property *property,
> >> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> >> index 97b0d84..b73d3b0 100644
> >> --- a/drivers/gpu/drm/drm_fb_helper.c
> >> +++ b/drivers/gpu/drm/drm_fb_helper.c
> >> @@ -286,13 +286,28 @@ bool drm_fb_helper_restore_fbdev_mode(struct drm_fb_helper *fb_helper)
> >> struct drm_device *dev = fb_helper->dev;
> >> struct drm_plane *plane;
> >> bool error = false;
> >> + void *state;
> >> int i;
> >>
> >> drm_warn_on_modeset_not_all_locked(dev);
> >>
> >> + state = dev->driver->atomic_begin(dev, 0);
> >> + if (IS_ERR(state)) {
> >> + DRM_ERROR("failed to restore fbdev mode\n");
> >> + return true;
> >> + }
> >> +
> >> list_for_each_entry(plane, &dev->mode_config.plane_list, head)
> >> if (plane->type != DRM_PLANE_TYPE_PRIMARY)
> >> - drm_plane_force_disable(plane);
> >> + drm_plane_force_disable(plane, state);
> >> +
> >> + /* just disabling stuff shouldn't fail, hopefully: */
> >> + if(dev->driver->atomic_check(dev, state))
> >> + DRM_ERROR("failed to restore fbdev mode\n");
> >> + else
> >> + dev->driver->atomic_commit(dev, state);
> >> +
> >> + dev->driver->atomic_end(dev, state);
> >>
> >> for (i = 0; i < fb_helper->crtc_count; i++) {
> >> struct drm_mode_set *mode_set = &fb_helper->crtc_info[i].mode_set;
> >> diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
> >> index d966afa..7a32383 100644
> >> --- a/drivers/gpu/drm/drm_plane_helper.c
> >> +++ b/drivers/gpu/drm/drm_plane_helper.c
> >> @@ -26,6 +26,7 @@
> >> #include <linux/list.h>
> >> #include <drm/drmP.h>
> >> #include <drm/drm_rect.h>
> >> +#include <drm/drm_atomic.h>
> >>
> >> #define SUBPIXEL_MASK 0xffff
> >>
> >> @@ -234,6 +235,7 @@ EXPORT_SYMBOL(drm_primary_helper_destroy);
> >> const struct drm_plane_funcs drm_primary_helper_funcs = {
> >> .update_plane = drm_primary_helper_update,
> >> .disable_plane = drm_primary_helper_disable,
> >> + .set_property = drm_atomic_plane_set_property,
> >> .destroy = drm_primary_helper_destroy,
> >> };
> >> EXPORT_SYMBOL(drm_primary_helper_funcs);
> >> diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.c b/drivers/gpu/drm/exynos/exynos_drm_plane.c
> >> index 9da0935..8cf7442 100644
> >> --- a/drivers/gpu/drm/exynos/exynos_drm_plane.c
> >> +++ b/drivers/gpu/drm/exynos/exynos_drm_plane.c
> >> @@ -10,6 +10,7 @@
> >> */
> >>
> >> #include <drm/drmP.h>
> >> +#include <drm/drm_atomic.h>
> >>
> >> #include <drm/exynos_drm.h>
> >> #include "exynos_drm_drv.h"
> >> @@ -220,13 +221,17 @@ static int exynos_plane_set_property(struct drm_plane *plane,
> >> struct drm_device *dev = plane->dev;
> >> struct exynos_plane *exynos_plane = to_exynos_plane(plane);
> >> struct exynos_drm_private *dev_priv = dev->dev_private;
> >> + struct drm_plane_state *pstate = drm_atomic_get_plane_state(plane, state);
> >> +
> >> + if (IS_ERR(pstate))
> >> + return PTR_ERR(pstate);
> >>
> >> if (property == dev_priv->plane_zpos_property) {
> >> exynos_plane->overlay.zpos = val;
> >> return 0;
> >> }
> >>
> >> - return -EINVAL;
> >> + return drm_plane_set_property(plane, pstate, property, val, blob_data);
> >> }
> >
> > Imo the interfaces here are a bit wonky - most drivers have the exact same
> > structure of allocating a plane state object if it's not there and setting
> > the property. Imo we should push this down a bit and have type-specific
> > set_prop interfaces which take the state-specific state object. Core
> > properties would be fully handled in the core. Which means that driver
> > don't need to implement set_prop callbacks if they don't have any special
> > properties on top of the core stuff. Much less boilerplate that way.
>
> Drivers which do not have any custom properties already just directly
> use drm_{plane,crtc}_set_property().
>
> Basically, a vanilla driver with no planes, no custom properties, and
> no inter-crtc dependencies could plug in helpers everywhere a be
> "fully atomic"(TM).
My critique is two-fold:
- It looks like the core property parsing is helper code which can be
overriden. We very much don't want that, since we want to expose a
common set of kms operations that work everywhere.
- Since the callbacks are not object specific there's a bit too much
casting going on for my taste.
- For drivers that do have driver props we could move the allocation error
handling into the core. Which is imo a Very Good Thing(tm).
Cheers, Daniel
>
> BR,
> -R
>
> > This would also give us a strong incentive to have common properties for
> > e.g. blending since we could simply pimp the core with them, and leave
> > drivers to just register the properties if they support them. If they have
> > additional restrictions they only need to implement the ->check hook,
> > which has the awesome advantage that they can deal with a real structure
> > instead of abstract prop arrays.
> >
> > E.g. we could add a plane->supports_blending bool which would
> > enabled/disable the default blending/Z-order properties for updating.
> >
> >>
> >> static struct drm_plane_funcs exynos_plane_funcs = {
> >> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> >> index c235546..3f742f5 100644
> >> --- a/drivers/gpu/drm/i915/intel_sprite.c
> >> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> >> @@ -1190,6 +1190,7 @@ static const struct drm_plane_funcs intel_plane_funcs = {
> >> .update_plane = intel_update_plane,
> >> .disable_plane = intel_disable_plane,
> >> .destroy = intel_destroy_plane,
> >> + .set_property = drm_atomic_plane_set_property,
> >> };
> >>
> >> static uint32_t ilk_plane_formats[] = {
> >> diff --git a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c
> >> index 8c064dc..4c92985 100644
> >> --- a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c
> >> +++ b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_plane.c
> >> @@ -88,8 +88,10 @@ int mdp4_plane_set_property(struct drm_plane *plane,
> >> struct drm_atomic_state *state, struct drm_property *property,
> >> uint64_t val, void *blob_data)
> >> {
> >> - // XXX
> >> - return -EINVAL;
> >> + struct drm_plane_state *pstate = drm_atomic_get_plane_state(plane, state);
> >> + if (IS_ERR(pstate))
> >> + return PTR_ERR(pstate);
> >> + return drm_plane_set_property(plane, pstate, property, val, blob_data);
> >> }
> >>
> >> static const struct drm_plane_funcs mdp4_plane_funcs = {
> >> diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
> >> index 5cbf226..53cc8c6 100644
> >> --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
> >> +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
> >> @@ -103,8 +103,10 @@ int mdp5_plane_set_property(struct drm_plane *plane,
> >> struct drm_atomic_state *state, struct drm_property *property,
> >> uint64_t val, void *blob_data)
> >> {
> >> - // XXX
> >> - return -EINVAL;
> >> + struct drm_plane_state *pstate = drm_atomic_get_plane_state(plane, state);
> >> + if (IS_ERR(pstate))
> >> + return PTR_ERR(pstate);
> >> + return drm_plane_set_property(plane, pstate, property, val, blob_data);
> >> }
> >>
> >> static const struct drm_plane_funcs mdp5_plane_funcs = {
> >> diff --git a/drivers/gpu/drm/nouveau/dispnv04/overlay.c b/drivers/gpu/drm/nouveau/dispnv04/overlay.c
> >> index 577e6aa..97b48b5 100644
> >> --- a/drivers/gpu/drm/nouveau/dispnv04/overlay.c
> >> +++ b/drivers/gpu/drm/nouveau/dispnv04/overlay.c
> >> @@ -24,6 +24,7 @@
> >> */
> >>
> >> #include <drm/drmP.h>
> >> +#include <drm/drm_atomic.h>
> >> #include <drm/drm_crtc.h>
> >> #include <drm/drm_fourcc.h>
> >>
> >> @@ -226,6 +227,10 @@ nv_set_property(struct drm_plane *plane,
> >> uint64_t value, void *blob_data)
> >> {
> >> struct nouveau_plane *nv_plane = (struct nouveau_plane *)plane;
> >> + struct drm_plane_state *pstate = drm_atomic_get_plane_state(plane, state);
> >> +
> >> + if (IS_ERR(pstate))
> >> + return PTR_ERR(pstate);
> >>
> >> if (property == nv_plane->props.colorkey)
> >> nv_plane->colorkey = value;
> >> @@ -240,7 +245,8 @@ nv_set_property(struct drm_plane *plane,
> >> else if (property == nv_plane->props.iturbt_709)
> >> nv_plane->iturbt_709 = value;
> >> else
> >> - return -EINVAL;
> >> + return drm_plane_set_property(plane, pstate,
> >> + property, value, blob_data);
> >>
> >> if (nv_plane->set_params)
> >> nv_plane->set_params(nv_plane);
> >> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
> >> index 3dca538..da80bdc 100644
> >> --- a/drivers/gpu/drm/omapdrm/omap_drv.c
> >> +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
> >> @@ -585,7 +585,7 @@ static void dev_lastclose(struct drm_device *dev)
> >>
> >> for (i = 0; i < priv->num_planes; i++) {
> >> drm_object_property_set_value(&priv->planes[i]->base,
> >> - &priv->planes[i]->propvals,
> >> + &priv->planes[i]->state->propvals,
> >> priv->rotation_prop, 0, NULL);
> >> }
> >> }
> >> diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/omapdrm/omap_plane.c
> >> index 2d3c975..a9acc58 100644
> >> --- a/drivers/gpu/drm/omapdrm/omap_plane.c
> >> +++ b/drivers/gpu/drm/omapdrm/omap_plane.c
> >> @@ -341,8 +341,12 @@ int omap_plane_set_property(struct drm_plane *plane,
> >> {
> >> struct omap_plane *omap_plane = to_omap_plane(plane);
> >> struct omap_drm_private *priv = plane->dev->dev_private;
> >> + struct drm_plane_state *pstate = drm_atomic_get_plane_state(plane, state);
> >> int ret = -EINVAL;
> >>
> >> + if (IS_ERR(pstate))
> >> + return PTR_ERR(pstate);
> >> +
> >> if (property == priv->rotation_prop) {
> >> DBG("%s: rotation: %02x", omap_plane->name, (uint32_t)val);
> >> omap_plane->win.rotation = val;
> >> @@ -351,6 +355,9 @@ int omap_plane_set_property(struct drm_plane *plane,
> >> DBG("%s: zorder: %02x", omap_plane->name, (uint32_t)val);
> >> omap_plane->info.zorder = val;
> >> ret = apply(plane);
> >> + } else {
> >> + ret = drm_plane_set_property(plane, pstate, property,
> >> + val, blob_data);
> >> }
> >>
> >> return ret;
> >> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_plane.c b/drivers/gpu/drm/rcar-du/rcar_du_plane.c
> >> index 3a5d843..015c76a 100644
> >> --- a/drivers/gpu/drm/rcar-du/rcar_du_plane.c
> >> +++ b/drivers/gpu/drm/rcar-du/rcar_du_plane.c
> >> @@ -14,6 +14,7 @@
> >> #include <drm/drmP.h>
> >> #include <drm/drm_crtc.h>
> >> #include <drm/drm_crtc_helper.h>
> >> +#include <drm/drm_atomic.h>
> >> #include <drm/drm_fb_cma_helper.h>
> >> #include <drm/drm_gem_cma_helper.h>
> >>
> >> @@ -404,6 +405,10 @@ static int rcar_du_plane_set_property(struct drm_plane *plane,
> >> {
> >> struct rcar_du_plane *rplane = to_rcar_plane(plane);
> >> struct rcar_du_group *rgrp = rplane->group;
> >> + struct drm_plane_state *pstate = drm_atomic_get_plane_state(plane, state);
> >> +
> >> + if (IS_ERR(pstate))
> >> + return PTR_ERR(pstate);
> >>
> >> if (property == rgrp->planes.alpha)
> >> rcar_du_plane_set_alpha(rplane, value);
> >> @@ -412,7 +417,8 @@ static int rcar_du_plane_set_property(struct drm_plane *plane,
> >> else if (property == rgrp->planes.zpos)
> >> rcar_du_plane_set_zpos(rplane, value);
> >> else
> >> - return -EINVAL;
> >> + return drm_plane_set_property(plane, pstate,
> >> + property, value, blob_data);
> >>
> >> return 0;
> >> }
> >> diff --git a/drivers/gpu/drm/shmobile/shmob_drm_plane.c b/drivers/gpu/drm/shmobile/shmob_drm_plane.c
> >> index 060ae03..ccf03ea 100644
> >> --- a/drivers/gpu/drm/shmobile/shmob_drm_plane.c
> >> +++ b/drivers/gpu/drm/shmobile/shmob_drm_plane.c
> >> @@ -14,6 +14,7 @@
> >> #include <drm/drmP.h>
> >> #include <drm/drm_crtc.h>
> >> #include <drm/drm_crtc_helper.h>
> >> +#include <drm/drm_atomic.h>
> >> #include <drm/drm_fb_cma_helper.h>
> >> #include <drm/drm_gem_cma_helper.h>
> >>
> >> @@ -228,6 +229,7 @@ static void shmob_drm_plane_destroy(struct drm_plane *plane)
> >> static const struct drm_plane_funcs shmob_drm_plane_funcs = {
> >> .update_plane = shmob_drm_plane_update,
> >> .disable_plane = shmob_drm_plane_disable,
> >> + .set_property = drm_atomic_plane_set_property,
> >> .destroy = shmob_drm_plane_destroy,
> >> };
> >>
> >> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> >> index ff72b81..78e93ec 100644
> >> --- a/include/drm/drm_atomic.h
> >> +++ b/include/drm/drm_atomic.h
> >> @@ -68,7 +68,8 @@
> >> * struct drm_atomic_funcs - helper funcs used by the atomic helpers
> >> */
> >> struct drm_atomic_funcs {
> >> - int dummy; /* for now */
> >> + int (*check_plane_state)(struct drm_plane *plane, struct drm_plane_state *pstate);
> >> + int (*commit_plane_state)(struct drm_plane *plane, struct drm_plane_state *pstate);
> >> };
> >>
> >> const extern struct drm_atomic_funcs drm_atomic_funcs;
> >> @@ -84,6 +85,30 @@ int drm_atomic_commit_unlocked(struct drm_device *dev,
> >> struct drm_atomic_state *state);
> >> void drm_atomic_end(struct drm_device *dev, struct drm_atomic_state *state);
> >>
> >> +int drm_atomic_plane_set_property(struct drm_plane *plane,
> >> + struct drm_atomic_state *state, struct drm_property *property,
> >> + uint64_t val, void *blob_data);
> >> +struct drm_plane_state *drm_atomic_get_plane_state(struct drm_plane *plane,
> >> + struct drm_atomic_state *state);
> >> +
> >> +static inline int
> >> +drm_atomic_check_plane_state(struct drm_plane *plane,
> >> + struct drm_plane_state *pstate)
> >> +{
> >> + const struct drm_atomic_funcs *funcs =
> >> + plane->dev->driver->atomic_funcs;
> >> + return funcs->check_plane_state(plane, pstate);
> >> +}
> >> +
> >> +static inline int
> >> +drm_atomic_commit_plane_state(struct drm_plane *plane,
> >> + struct drm_plane_state *pstate)
> >> +{
> >> + const struct drm_atomic_funcs *funcs =
> >> + plane->dev->driver->atomic_funcs;
> >> + return funcs->commit_plane_state(plane, pstate);
> >> +}
> >> +
> >> /**
> >> * struct drm_atomic_state - the state object used by atomic helpers
> >> */
> >> @@ -91,6 +116,8 @@ struct drm_atomic_state {
> >> struct kref refcount;
> >> struct drm_device *dev;
> >> uint32_t flags;
> >> + struct drm_plane **planes;
> >> + struct drm_plane_state **pstates;
> >>
> >> bool committed;
> >> bool checked; /* just for debugging */
> >> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> >> index 547b75a..58309cc 100644
> >> --- a/include/drm/drm_crtc.h
> >> +++ b/include/drm/drm_crtc.h
> >> @@ -569,7 +569,10 @@ struct drm_plane_funcs {
> >> int (*disable_plane)(struct drm_plane *plane);
> >> void (*destroy)(struct drm_plane *plane);
> >>
> >> - int (*set_property)(struct drm_plane *plane,
> >> + struct drm_plane_state *(*create_state)(struct drm_plane *plane);
> >> + void (*destroy_state)(struct drm_plane *plane,
> >> + struct drm_plane_state *pstate);
> >> + int (*set_property)(struct drm_plane *plane,
> >> struct drm_atomic_state *state,
> >> struct drm_property *property, uint64_t val,
> >> void *blob_data);
> >> @@ -582,6 +585,48 @@ enum drm_plane_type {
> >> };
> >>
> >> /**
> >> + * drm_plane_state - mutable plane state
> >> + * @update_plane: if full update_plane() is needed (vs pageflip)
> >> + * @new_fb: has the fb been changed
> >> + * @crtc: currently bound CRTC
> >> + * @fb: currently bound fb
> >> + * @crtc_x: left position of visible portion of plane on crtc
> >> + * @crtc_y: upper position of visible portion of plane on crtc
> >> + * @crtc_w: width of visible portion of plane on crtc
> >> + * @crtc_h: height of visible portion of plane on crtc
> >> + * @src_x: left position of visible portion of plane within
> >> + * plane (in 16.16)
> >> + * @src_y: upper position of visible portion of plane within
> >> + * plane (in 16.16)
> >> + * @src_w: width of visible portion of plane (in 16.16)
> >> + * @src_h: height of visible portion of plane (in 16.16)
> >> + * @propvals: property values
> >> + * @state: current global/toplevel state object (for atomic) while an
> >> + * update is in progress, NULL otherwise.
> >> + */
> >> +struct drm_plane_state {
> >> + bool update_plane : 1;
> >> + bool new_fb : 1;
> >> +
> >> + struct drm_crtc *crtc;
> >> + struct drm_framebuffer *fb;
> >> +
> >> + /* Signed dest location allows it to be partially off screen */
> >> + int32_t crtc_x, crtc_y;
> >> + uint32_t crtc_w, crtc_h;
> >> +
> >> + /* Source values are 16.16 fixed point */
> >> + uint32_t src_x, src_y;
> >> + uint32_t src_h, src_w;
> >> +
> >> + bool enabled;
> >> +
> >> + struct drm_object_property_values propvals;
> >> +
> >> + struct drm_atomic_state *state;
> >> +};
> >> +
> >> +/**
> >> * drm_plane - central DRM plane control structure
> >> * @dev: DRM device this plane belongs to
> >> * @head: for list management
> >> @@ -591,6 +636,8 @@ enum drm_plane_type {
> >> * @format_count: number of formats supported
> >> * @crtc: currently bound CRTC
> >> * @fb: currently bound fb
> >> + * @id: plane number, 0..n
> >> + * @state: the mutable state
> >> * @funcs: helper functions
> >> * @properties: property tracking for this plane
> >> * @type: type of plane (overlay, primary, cursor)
> >> @@ -608,10 +655,17 @@ struct drm_plane {
> >> struct drm_crtc *crtc;
> >> struct drm_framebuffer *fb;
> >>
> >> + int id;
> >> +
> >> + /*
> >> + * State that can be updated from userspace, and atomically
> >> + * commited or rolled back:
> >> + */
> >> + struct drm_plane_state *state;
> >> +
> >> const struct drm_plane_funcs *funcs;
> >>
> >> struct drm_object_properties properties;
> >> - struct drm_object_property_values propvals;
> >>
> >> enum drm_plane_type type;
> >> };
> >> @@ -807,8 +861,20 @@ struct drm_mode_config {
> >> bool poll_running;
> >> struct delayed_work output_poll_work;
> >>
> >> - /* pointers to standard properties */
> >> + /* just so blob properties can always be in a list: */
> >> struct list_head property_blob_list;
> >> +
> >> + /* pointers to standard properties */
> >> + struct drm_property *prop_src_x;
> >> + struct drm_property *prop_src_y;
> >> + struct drm_property *prop_src_w;
> >> + struct drm_property *prop_src_h;
> >> + struct drm_property *prop_crtc_x;
> >> + struct drm_property *prop_crtc_y;
> >> + struct drm_property *prop_crtc_w;
> >> + struct drm_property *prop_crtc_h;
> >> + struct drm_property *prop_fb_id;
> >> + struct drm_property *prop_crtc_id;
> >> struct drm_property *edid_property;
> >> struct drm_property *dpms_property;
> >> struct drm_property *plane_type_property;
> >> @@ -930,11 +996,20 @@ extern int drm_plane_init(struct drm_device *dev,
> >> const uint32_t *formats, uint32_t format_count,
> >> bool is_primary);
> >> extern void drm_plane_cleanup(struct drm_plane *plane);
> >> -extern void drm_plane_force_disable(struct drm_plane *plane);
> >> +extern void drm_plane_force_disable(struct drm_plane *plane,
> >> + struct drm_atomic_state *state);
> >> extern int drm_crtc_check_viewport(const struct drm_crtc *crtc,
> >> int x, int y,
> >> const struct drm_display_mode *mode,
> >> const struct drm_framebuffer *fb);
> >> +extern int drm_plane_check_state(struct drm_plane *plane,
> >> + struct drm_plane_state *state);
> >> +extern void drm_plane_commit_state(struct drm_plane *plane,
> >> + struct drm_plane_state *state);
> >> +extern int drm_plane_set_property(struct drm_plane *plane,
> >> + struct drm_plane_state *state,
> >> + struct drm_property *property,
> >> + uint64_t value, void *blob_data);
> >>
> >> extern void drm_encoder_cleanup(struct drm_encoder *encoder);
> >>
> >> @@ -984,6 +1059,17 @@ extern int drm_object_property_set_value(struct drm_mode_object *obj,
> >> extern int drm_object_property_get_value(struct drm_mode_object *obj,
> >> struct drm_property *property,
> >> uint64_t *value);
> >> +
> >> +int drm_mode_connector_set_obj_prop(struct drm_connector *connector,
> >> + struct drm_atomic_state *state, struct drm_property *property,
> >> + uint64_t value, void *blob_data);
> >> +int drm_mode_crtc_set_obj_prop(struct drm_crtc *crtc,
> >> + struct drm_atomic_state *state, struct drm_property *property,
> >> + uint64_t value, void *blob_data);
> >> +int drm_mode_plane_set_obj_prop(struct drm_plane *plane,
> >> + struct drm_atomic_state *state, struct drm_property *property,
> >> + uint64_t value, void *blob_data);
> >> +
> >> extern int drm_framebuffer_init(struct drm_device *dev,
> >> struct drm_framebuffer *fb,
> >> const struct drm_framebuffer_funcs *funcs);
> >> @@ -1165,6 +1251,26 @@ drm_property_blob_find(struct drm_device *dev, uint32_t id)
> >> return mo ? obj_to_blob(mo) : NULL;
> >> }
> >>
> >> +static inline struct drm_plane_state *
> >> +drm_plane_create_state(struct drm_plane *plane)
> >> +{
> >> + if (plane->funcs->create_state)
> >> + return plane->funcs->create_state(plane);
> >> + return kzalloc(sizeof(struct drm_plane_state), GFP_KERNEL);
> >> +}
> >> +
> >> +static inline void
> >> +drm_plane_destroy_state(struct drm_plane *plane,
> >> + struct drm_plane_state *pstate)
> >> +{
> >> + if (pstate->fb)
> >> + drm_framebuffer_unreference(pstate->fb);
> >> + if (plane->funcs->destroy_state)
> >> + plane->funcs->destroy_state(plane, pstate);
> >> + else
> >> + kfree(pstate);
> >> +}
> >> +
> >> /* Plane list iterator for legacy (overlay only) planes. */
> >> #define drm_for_each_legacy_plane(plane, planelist) \
> >> list_for_each_entry(plane, planelist, head) \
> >> --
> >> 1.9.0
> >>
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
More information about the dri-devel
mailing list