drm/atomic: refuse changing CRTC for planes directly

Commit Message

Daniel Vetter Aug. 26, 2015, 7:49 p.m. UTC

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.

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 <[e-mail]>
Cc: Maarten Lankhorst <[e-mail]>
Cc: Daniel Stone <[e-mail]>
Acked-by: Daniel Stone <[e-mail]>
Signed-off-by: Daniel Vetter <[e-mail]>

---

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(+)

Comments

Rob Clark Aug. 26, 2015, 9:51 p.m. UTC | #1

On Wed, Aug 26, 2015 at 3:49 PM, Daniel Vetter <[e-mail]> 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

(w/ s/swihc/switch/ fix smashed in above)

Reviewed-by: Rob Clark <[e-mail]>


> 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 <[e-mail]>
> Cc: Maarten Lankhorst <[e-mail]>
> Cc: Daniel Stone <[e-mail]>
> Acked-by: Daniel Stone <[e-mail]>
> Signed-off-by: Daniel Vetter <[e-mail]>
>
> ---
>
> 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
> [e-mail]
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Maarten Lankhorst Aug. 27, 2015, 6:08 a.m. UTC | #2

Op 26-08-15 om 21:49 schreef Daniel Vetter:
> 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.
>
> 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 <[e-mail]>
> Cc: Maarten Lankhorst <[e-mail]>
Acked-by: Maarten Lankhorst <[e-mail]>
> Cc: Daniel Stone <[e-mail]>
> Acked-by: Daniel Stone <[e-mail]>
> Signed-off-by: Daniel Vetter <[e-mail]>
>
> ---
>
> 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;
>  }
>

Daniel Vetter Aug. 27, 2015, 7:45 a.m. UTC | #3

On Wed, Aug 26, 2015 at 05:51:46PM -0400, Rob Clark wrote:
> On Wed, Aug 26, 2015 at 3:49 PM, Daniel Vetter <[e-mail]> 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 <[e-mail]>

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 <[e-mail]>
> > Cc: Maarten Lankhorst <[e-mail]>
> > Cc: Daniel Stone <[e-mail]>
> > Acked-by: Daniel Stone <[e-mail]>
> > Signed-off-by: Daniel Vetter <[e-mail]>
> >
> > ---
> >
> > 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
> > [e-mail]
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Patch

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;
 }