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

[Rob Clark]

On Wed, Aug 26, 2015 at 12:30 PM, Ville Syrjälä
<ville.syrjala at linux.intel.com> wrote:
> On Wed, Aug 26, 2015 at 12:07:30PM -0400, Rob Clark wrote:
>> On Wed, Aug 26, 2015 at 12:03 PM, Rob Clark <robdclark at gmail.com> wrote:
>> > On Wed, Aug 26, 2015 at 11:41 AM, Daniel Vetter <daniel.vetter at 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.
>> >
>> > So, I expect msm should actually be able to do this w/ dual-dsi (where
>> > we are using two CRTC's, w/ synchronized flushes)..
>> >
>> > Probably someone who has a dual-dsi panel should actually test that to
>> > confirm.  But it seems like it should work.  Maybe we need something
>> > in 'struct drm_crtc' so core can realize that two CRTC's are locked
>> > together..
>>
>> oh, and for most drivers, switching plane between CRTCs without an
>> off-cycle would probably also work for DSI cmd mode..
>
> You'd need to wait for any ongoing transfer on the old crtc to finish
> before moving the plane. So that's not really any different than the
> driver doing the dance with a vblank wait on a video mode display.

except that update would need to block from previous xfer anyways, so
there isn't really a race w/ frame n+1 like there would be with video
mode..

I do think that *somehow* we need to expose whether or not the display
is cmd-mode to userspace, since that factors into decisions about
whether it can immediately re-use a plane on another CRTC, and I think
it factors into discussion about some differences between ADF fencing
and upstream fencing as we add explicit fencing support to atomic..
but that is probably a different topic..

BR,
-R

>>
>> BR,
>> -R
>>
>> >
>> >> I've put this into the core since I really couldn't come up with a
>> >> case where we don't want to enforce that. But if that ever happens it
>> >> would be easy to move this check into helpers.
>> >>
>> >> 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.
>> >>
>> >> Cc: Thierry Reding <thierry.reding at gmail.com>
>> >> Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
>> >> Cc: Daniel Stone <daniels at collabora.com>
>> >> Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
>> >> ---
>> >>  drivers/gpu/drm/drm_atomic.c | 22 ++++++++++++++++++++++
>> >>  1 file changed, 22 insertions(+)
>> >>
>> >> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>> >> index 434915448ea0..f27aae3fa765 100644
>> >> --- a/drivers/gpu/drm/drm_atomic.c
>> >> +++ b/drivers/gpu/drm/drm_atomic.c
>> >> @@ -663,6 +663,22 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
>> >>         return 0;
>> >>  }
>> >>
>> >> +static bool
>> >> +plane_switching(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;
>> >> +
>> >> +       return true;
>> >> +}
>> >> +
>> >>  /**
>> >>   * drm_atomic_plane_check - check plane state
>> >>   * @plane: plane to check
>> >> @@ -734,6 +750,12 @@ static int drm_atomic_plane_check(struct drm_plane *plane,
>> >>                 return -ENOSPC;
>> >>         }
>> >>
>> >> +       if (plane_switching(state->state, plane, state)) {
>> >> +               DRM_DEBUG_ATOMIC("[PLANE:%d] switching CRTC directly\n",
>> >> +                                plane->base.id);
>> >> +               return -EINVAL;
>> >> +       }
>> >> +
>> >>         return 0;
>> >>  }
>> >>
>> >> --
>> >> 2.5.0
>> >>
>> >> _______________________________________________
>> >> dri-devel mailing list
>> >> dri-devel at lists.freedesktop.org
>> >> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Ville Syrjälä
> Intel OTC