Re: [PATCH] drm/atomic: refuse changing CRTC for planes directly

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Aug 26, 2015 at 05:51:46PM -0400, Rob Clark wrote:
> On Wed, Aug 26, 2015 at 3:49 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > Very strictly speaking this is possible if you have special hw and
> > genlocked CRTCs. In general switching a plane between two active CRTC
> > just won't work so well and is probably not tested at all. Just forbid
> > it.
> >
> > I've put this into the core since right now no helper or driver copes
> > with it, no userspace has code for it and no one asks for it. Yes
> > there's piles of corner-cases where this would be possible to do this
> > like:
> > - switch from inactive crtc to active crtc
> > - swithc from active crtc to inactive crtc
> > - genlocked display
> > - invisible plane (to do whatever)
> > - idle plane hw due to dsi cmd mode/psr
> > - whatever
> > but looking at details it's not that easy to implement this correctly.
> > Hence just put it into the core and add a comment, since the only
> > userspace we have right now for atomic (weston) doesn't want to use
> > direct plane switching either.
> 
> I suspect that eventually we'll want to make this a helper exposed to
> drivers and push this check down into the drivers.. perhaps that is
> not until weston (and/or atomic based hwc) grows driver specific
> userspace plugin type API to make smarter hw specific decisions..
> until then, this is probably the most reasonable thing to do.. so

Yeah this really is just to plug a "undefined behaviour" gap in the abi
until we know what exactly we want and have some userspace needing this.

> (w/ s/swihc/switch/ fix smashed in above)
> 
> Reviewed-by: Rob Clark <robdclark@gmail.com>

Fixed&merged to drm-misc, thanks for the review.
-Daniel

> 
> 
> > v2: don't bother with complexity and just outright disallow plane
> > switching without the intermediate OFF state. Simplifies drivers, we
> > don't have any hw that could do it anyway and current atomic userspace
> > (weston) works like this already anyway.
> >
> > v3: Bikeshed function name (Ville) and add comment (Rob).
> >
> > v4: Also bikeshed commit message (Rob).
> >
> > Cc: Thierry Reding <thierry.reding@gmail.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Daniel Stone <daniels@collabora.com>
> > Acked-by: Daniel Stone <daniels@collabora.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> >
> > ---
> >
> > Imo this is bikeshedded enough, so either someone takes this or
> > someone else can mangle this patch more.
> > -Daniel
> > ---
> >  drivers/gpu/drm/drm_atomic.c | 27 +++++++++++++++++++++++++++
> >  1 file changed, 27 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index 1066e4b658cf..40ddd6aa100f 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -663,6 +663,27 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
> >         return 0;
> >  }
> >
> > +static bool
> > +plane_switching_crtc(struct drm_atomic_state *state,
> > +                    struct drm_plane *plane,
> > +                    struct drm_plane_state *plane_state)
> > +{
> > +       struct drm_crtc_state *crtc_state, *curr_crtc_state;
> > +
> > +       if (!plane->state->crtc || !plane_state->crtc)
> > +               return false;
> > +
> > +       if (plane->state->crtc == plane_state->crtc)
> > +               return false;
> > +
> > +       /* This could be refined, but currently there's no helper or driver code
> > +        * to implement direct switching of active planes nor userspace to take
> > +        * advantage of more direct plane switching without the intermediate
> > +        * full OFF state.
> > +        */
> > +       return true;
> > +}
> > +
> >  /**
> >   * drm_atomic_plane_check - check plane state
> >   * @plane: plane to check
> > @@ -734,6 +755,12 @@ static int drm_atomic_plane_check(struct drm_plane *plane,
> >                 return -ENOSPC;
> >         }
> >
> > +       if (plane_switching_crtc(state->state, plane, state)) {
> > +               DRM_DEBUG_ATOMIC("[PLANE:%d] switching CRTC directly\n",
> > +                                plane->base.id);
> > +               return -EINVAL;
> > +       }
> > +
> >         return 0;
> >  }
> >
> > --
> > 2.5.0
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx