drm: Don't force all planes to be added to the state due to zpos

Submitted by Ville Syrjälä on Oct. 10, 2016, 12:19 p.m.

Details

Message ID 1476101987-31986-1-git-send-email-ville.syrjala@linux.intel.com
State New
Headers show
Series "drm: Don't force all planes to be added to the state due to zpos" ( rev: 1 ) in DRI devel

Not browsing as part of any series.

Commit Message

Ville Syrjälä Oct. 10, 2016, 12:19 p.m.
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

We don't want all planes to be added to the state whenever a
plane with fixed zpos gets enabled/disabled. This is true
especially for eg. cursor planes on i915, as we want cursor
updates to go through w/o throttling. Same holds for drivers
that don't support zpos at all (i915 actually falls into this
category right now since we've not yet added zpos support).

Allow drivers more freedom by letting them deal with zpos
themselves instead of doing it in drm_atomic_helper_check_planes()
unconditionally. Easiest solution seems to be to move the call
up to drm_atomic_helper_check(). But as some drivers might want
to use that function without the zpos handling, let's provide
two variants: the normal one, and one that deals with zpos.

Cc: Marek Szyprowski <m.szyprowski@samsung.com>
Cc: Benjamin Gaignard <benjamin.gaignard@linaro.org>
Cc: Vincent Abriou <vincent.abriou@st.com>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Inki Dae <inki.dae@samsung.com>
Cc: Joonyoung Shim <jy0922.shim@samsung.com>
Cc: Seung-Woo Kim <sw0312.kim@samsung.com>
Cc: Kyungmin Park <kyungmin.park@samsung.com>
Cc: Lyude <cpaul@redhat.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: stable@vger.kernel.org
Fixes: 44d1240d006c ("drm: add generic zpos property")
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_atomic_helper.c    | 46 +++++++++++++++++++++++++++++++---
 drivers/gpu/drm/exynos/exynos_drm_fb.c |  2 +-
 drivers/gpu/drm/rcar-du/rcar_du_kms.c  |  2 +-
 drivers/gpu/drm/sti/sti_drv.c          |  2 +-
 include/drm/drm_atomic_helper.h        |  2 ++
 5 files changed, 47 insertions(+), 7 deletions(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index c3f83476f996..cd19281cdefb 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -594,10 +594,6 @@  drm_atomic_helper_check_planes(struct drm_device *dev,
 	struct drm_plane_state *plane_state;
 	int i, ret = 0;
 
-	ret = drm_atomic_normalize_zpos(dev, state);
-	if (ret)
-		return ret;
-
 	for_each_plane_in_state(state, plane, plane_state, i) {
 		const struct drm_plane_helper_funcs *funcs;
 
@@ -673,6 +669,48 @@  int drm_atomic_helper_check(struct drm_device *dev,
 }
 EXPORT_SYMBOL(drm_atomic_helper_check);
 
+/**
+ * drm_atomic_helper_check_with_zpos - validate state object, dealing with zpos
+ * @dev: DRM device
+ * @state: the driver state object
+ *
+ * Check the state object to see if the requested state is physically possible.
+ * Only crtcs and planes have check callbacks, so for any additional (global)
+ * checking that a driver needs it can simply wrap that around this function.
+ * Drivers without such needs can directly use this as their ->atomic_check()
+ * callback.
+ *
+ * This just wraps the two parts of the state checking for planes and modeset
+ * state in the default order: First it calls drm_atomic_helper_check_modeset(),
+ * followed by drm_atomic_normalize_zpos(), and finally
+ * drm_atomic_helper_check_planes(). The assumption is that the
+ * ->atomic_check functions depend upon an updated adjusted_mode.clock to
+ * e.g. properly compute watermarks.
+ *
+ * RETURNS:
+ * Zero for success or -errno
+ */
+int drm_atomic_helper_check_with_zpos(struct drm_device *dev,
+				      struct drm_atomic_state *state)
+{
+	int ret;
+
+	ret = drm_atomic_helper_check_modeset(dev, state);
+	if (ret)
+		return ret;
+
+	ret = drm_atomic_normalize_zpos(dev, state);
+	if (ret)
+		return ret;
+
+	ret = drm_atomic_helper_check_planes(dev, state);
+	if (ret)
+		return ret;
+
+	return ret;
+}
+EXPORT_SYMBOL(drm_atomic_helper_check_with_zpos);
+
 static void
 disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
 {
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.c b/drivers/gpu/drm/exynos/exynos_drm_fb.c
index 40ce841eb952..5c0930af01fa 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fb.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fb.c
@@ -190,7 +190,7 @@  dma_addr_t exynos_drm_fb_dma_addr(struct drm_framebuffer *fb, int index)
 static const struct drm_mode_config_funcs exynos_drm_mode_config_funcs = {
 	.fb_create = exynos_user_fb_create,
 	.output_poll_changed = exynos_drm_output_poll_changed,
-	.atomic_check = drm_atomic_helper_check,
+	.atomic_check = drm_atomic_helper_check_with_zpos,
 	.atomic_commit = exynos_atomic_commit,
 };
 
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
index bd9c3bb9252c..2cfd35f3f2f6 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
@@ -231,7 +231,7 @@  static int rcar_du_atomic_check(struct drm_device *dev,
 	struct rcar_du_device *rcdu = dev->dev_private;
 	int ret;
 
-	ret = drm_atomic_helper_check(dev, state);
+	ret = drm_atomic_helper_check_with_zpos(dev, state);
 	if (ret < 0)
 		return ret;
 
diff --git a/drivers/gpu/drm/sti/sti_drv.c b/drivers/gpu/drm/sti/sti_drv.c
index 2784919a7366..df5f150021d0 100644
--- a/drivers/gpu/drm/sti/sti_drv.c
+++ b/drivers/gpu/drm/sti/sti_drv.c
@@ -248,7 +248,7 @@  static void sti_output_poll_changed(struct drm_device *ddev)
 static const struct drm_mode_config_funcs sti_mode_config_funcs = {
 	.fb_create = drm_fb_cma_create,
 	.output_poll_changed = sti_output_poll_changed,
-	.atomic_check = drm_atomic_helper_check,
+	.atomic_check = drm_atomic_helper_check_with_zpos,
 	.atomic_commit = sti_atomic_commit,
 };
 
diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
index 7ff92b09fd9c..b1e7193c9d1c 100644
--- a/include/drm/drm_atomic_helper.h
+++ b/include/drm/drm_atomic_helper.h
@@ -40,6 +40,8 @@  int drm_atomic_helper_check_planes(struct drm_device *dev,
 			       struct drm_atomic_state *state);
 int drm_atomic_helper_check(struct drm_device *dev,
 			    struct drm_atomic_state *state);
+int drm_atomic_helper_check_with_zpos(struct drm_device *dev,
+				      struct drm_atomic_state *state);
 void drm_atomic_helper_commit_tail(struct drm_atomic_state *state);
 int drm_atomic_helper_commit(struct drm_device *dev,
 			     struct drm_atomic_state *state,

Comments

On Mon, Oct 10, 2016 at 2:19 PM,  <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> We don't want all planes to be added to the state whenever a
> plane with fixed zpos gets enabled/disabled. This is true
> especially for eg. cursor planes on i915, as we want cursor
> updates to go through w/o throttling. Same holds for drivers
> that don't support zpos at all (i915 actually falls into this
> category right now since we've not yet added zpos support).
>
> Allow drivers more freedom by letting them deal with zpos
> themselves instead of doing it in drm_atomic_helper_check_planes()
> unconditionally. Easiest solution seems to be to move the call
> up to drm_atomic_helper_check(). But as some drivers might want
> to use that function without the zpos handling, let's provide
> two variants: the normal one, and one that deals with zpos.
>
> Cc: Marek Szyprowski <m.szyprowski@samsung.com>
> Cc: Benjamin Gaignard <benjamin.gaignard@linaro.org>
> Cc: Vincent Abriou <vincent.abriou@st.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Inki Dae <inki.dae@samsung.com>
> Cc: Joonyoung Shim <jy0922.shim@samsung.com>
> Cc: Seung-Woo Kim <sw0312.kim@samsung.com>
> Cc: Kyungmin Park <kyungmin.park@samsung.com>
> Cc: Lyude <cpaul@redhat.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: stable@vger.kernel.org
> Fixes: 44d1240d006c ("drm: add generic zpos property")
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Seems a bit fragile, and then drivers still need to not overshot when
they do zpos (which we want eventually in i915 too). I think the
proper way is to keep track of a per-plane zpos changed (or compute
that ad-hoc, we have both states). And only grab more planes if a zpos
value changed.

That would fix the issue at the source, also work for us in the
future, and it should be contained to just the helper function itself.
Win all around ;-)
-Daniel
On Mon, Oct 10, 2016 at 02:46:35PM +0200, Daniel Vetter wrote:
> On Mon, Oct 10, 2016 at 2:19 PM,  <ville.syrjala@linux.intel.com> wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > We don't want all planes to be added to the state whenever a
> > plane with fixed zpos gets enabled/disabled. This is true
> > especially for eg. cursor planes on i915, as we want cursor
> > updates to go through w/o throttling. Same holds for drivers
> > that don't support zpos at all (i915 actually falls into this
> > category right now since we've not yet added zpos support).
> >
> > Allow drivers more freedom by letting them deal with zpos
> > themselves instead of doing it in drm_atomic_helper_check_planes()
> > unconditionally. Easiest solution seems to be to move the call
> > up to drm_atomic_helper_check(). But as some drivers might want
> > to use that function without the zpos handling, let's provide
> > two variants: the normal one, and one that deals with zpos.
> >
> > Cc: Marek Szyprowski <m.szyprowski@samsung.com>
> > Cc: Benjamin Gaignard <benjamin.gaignard@linaro.org>
> > Cc: Vincent Abriou <vincent.abriou@st.com>
> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Cc: Inki Dae <inki.dae@samsung.com>
> > Cc: Joonyoung Shim <jy0922.shim@samsung.com>
> > Cc: Seung-Woo Kim <sw0312.kim@samsung.com>
> > Cc: Kyungmin Park <kyungmin.park@samsung.com>
> > Cc: Lyude <cpaul@redhat.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: stable@vger.kernel.org
> > Fixes: 44d1240d006c ("drm: add generic zpos property")
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Seems a bit fragile, and then drivers still need to not overshot when
> they do zpos (which we want eventually in i915 too).

The only platform where we can do it is pre-g4x, and vlv/chv. And I'm
thinking I can do a better job of it in the driver.

> I think the
> proper way is to keep track of a per-plane zpos changed (or compute
> that ad-hoc, we have both states). And only grab more planes if a zpos
> value changed.

Doesn't work with normalized zpos. The plane's actual zpos may be
unchanged even if the normalized zpos changes.

> 
> That would fix the issue at the source, also work for us in the
> future, and it should be contained to just the helper function itself.
> Win all around ;-)
> -Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Mon, Oct 10, 2016 at 2:56 PM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
>> I think the
>> proper way is to keep track of a per-plane zpos changed (or compute
>> that ad-hoc, we have both states). And only grab more planes if a zpos
>> value changed.
>
> Doesn't work with normalized zpos. The plane's actual zpos may be
> unchanged even if the normalized zpos changes.

Well I meant to do a first loop to check for any zpos changes, and
only then add all the planes. If no one touched zpos, then also no
normalized zpos can change. I think at least ... Or what am I missing?
-Daniel
On Mon, Oct 10, 2016 at 03:14:22PM +0200, Daniel Vetter wrote:
> On Mon, Oct 10, 2016 at 2:56 PM, Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> >> I think the
> >> proper way is to keep track of a per-plane zpos changed (or compute
> >> that ad-hoc, we have both states). And only grab more planes if a zpos
> >> value changed.
> >
> > Doesn't work with normalized zpos. The plane's actual zpos may be
> > unchanged even if the normalized zpos changes.
> 
> Well I meant to do a first loop to check for any zpos changes, and
> only then add all the planes. If no one touched zpos, then also no
> normalized zpos can change. I think at least ... Or what am I missing?

Enabling planes can change zpos. Which is what's currently causing all
the planes to be added to the state every time the cursor is
enabled/disabled on i915. Which isn't nice.

And anyway adding all the planes to the state when some zpos change
happens is entirely pointless on i915. There's no need to run through
any of .check_plane() hooks in this case. The only thing we need to
do is rewrite the sprite control registers (+ base addresses of course
to arm the update). So we could just add the sprite planes to state
after the .check_plane() stuff has been done so that we'll just get the
.commit_plane(). So knowing somehting about the hardware allows us to
do a much better job of this.
On Mon, Oct 10, 2016 at 3:32 PM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> On Mon, Oct 10, 2016 at 03:14:22PM +0200, Daniel Vetter wrote:
>> On Mon, Oct 10, 2016 at 2:56 PM, Ville Syrjälä
>> <ville.syrjala@linux.intel.com> wrote:
>> >> I think the
>> >> proper way is to keep track of a per-plane zpos changed (or compute
>> >> that ad-hoc, we have both states). And only grab more planes if a zpos
>> >> value changed.
>> >
>> > Doesn't work with normalized zpos. The plane's actual zpos may be
>> > unchanged even if the normalized zpos changes.
>>
>> Well I meant to do a first loop to check for any zpos changes, and
>> only then add all the planes. If no one touched zpos, then also no
>> normalized zpos can change. I think at least ... Or what am I missing?
>
> Enabling planes can change zpos. Which is what's currently causing all
> the planes to be added to the state every time the cursor is
> enabled/disabled on i915. Which isn't nice.
>
> And anyway adding all the planes to the state when some zpos change
> happens is entirely pointless on i915. There's no need to run through
> any of .check_plane() hooks in this case. The only thing we need to
> do is rewrite the sprite control registers (+ base addresses of course
> to arm the update). So we could just add the sprite planes to state
> after the .check_plane() stuff has been done so that we'll just get the
> .commit_plane(). So knowing somehting about the hardware allows us to
> do a much better job of this.

Ok, I buy it ;-)

So we only need this if we have a driver that's using
plane_state->normalized_zpos. Having all combinatorials of
atomic_check_with_featureA_featureB will get ugly. I think it'd be
better to just split it out and dump it only into drivers's
atomic_check that need it. Plus add a very big warning to
drm_plane_state->normalized_zpos that without calling that helper it
will be invalid. Bit more copypaste, but I think that's actually good
(since we don't want to support all combinatorial variations with
pre-made helpers imo).
-Daniel
On Mon, Oct 10, 2016 at 03:19:47PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> We don't want all planes to be added to the state whenever a
> plane with fixed zpos gets enabled/disabled. This is true
> especially for eg. cursor planes on i915, as we want cursor
> updates to go through w/o throttling. Same holds for drivers
> that don't support zpos at all (i915 actually falls into this
> category right now since we've not yet added zpos support).
> 
> Allow drivers more freedom by letting them deal with zpos
> themselves instead of doing it in drm_atomic_helper_check_planes()
> unconditionally. Easiest solution seems to be to move the call
> up to drm_atomic_helper_check(). But as some drivers might want
> to use that function without the zpos handling, let's provide
> two variants: the normal one, and one that deals with zpos.
> 
> Cc: Marek Szyprowski <m.szyprowski@samsung.com>
> Cc: Benjamin Gaignard <benjamin.gaignard@linaro.org>
> Cc: Vincent Abriou <vincent.abriou@st.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Inki Dae <inki.dae@samsung.com>
> Cc: Joonyoung Shim <jy0922.shim@samsung.com>
> Cc: Seung-Woo Kim <sw0312.kim@samsung.com>
> Cc: Kyungmin Park <kyungmin.park@samsung.com>
> Cc: Lyude <cpaul@redhat.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: stable@vger.kernel.org
> Fixes: 44d1240d006c ("drm: add generic zpos property")
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Ping. Can I get some buy-in from the relevant folks?

> ---
>  drivers/gpu/drm/drm_atomic_helper.c    | 46 +++++++++++++++++++++++++++++++---
>  drivers/gpu/drm/exynos/exynos_drm_fb.c |  2 +-
>  drivers/gpu/drm/rcar-du/rcar_du_kms.c  |  2 +-
>  drivers/gpu/drm/sti/sti_drv.c          |  2 +-
>  include/drm/drm_atomic_helper.h        |  2 ++
>  5 files changed, 47 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index c3f83476f996..cd19281cdefb 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -594,10 +594,6 @@ drm_atomic_helper_check_planes(struct drm_device *dev,
>  	struct drm_plane_state *plane_state;
>  	int i, ret = 0;
>  
> -	ret = drm_atomic_normalize_zpos(dev, state);
> -	if (ret)
> -		return ret;
> -
>  	for_each_plane_in_state(state, plane, plane_state, i) {
>  		const struct drm_plane_helper_funcs *funcs;
>  
> @@ -673,6 +669,48 @@ int drm_atomic_helper_check(struct drm_device *dev,
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_check);
>  
> +/**
> + * drm_atomic_helper_check_with_zpos - validate state object, dealing with zpos
> + * @dev: DRM device
> + * @state: the driver state object
> + *
> + * Check the state object to see if the requested state is physically possible.
> + * Only crtcs and planes have check callbacks, so for any additional (global)
> + * checking that a driver needs it can simply wrap that around this function.
> + * Drivers without such needs can directly use this as their ->atomic_check()
> + * callback.
> + *
> + * This just wraps the two parts of the state checking for planes and modeset
> + * state in the default order: First it calls drm_atomic_helper_check_modeset(),
> + * followed by drm_atomic_normalize_zpos(), and finally
> + * drm_atomic_helper_check_planes(). The assumption is that the
> + * ->atomic_check functions depend upon an updated adjusted_mode.clock to
> + * e.g. properly compute watermarks.
> + *
> + * RETURNS:
> + * Zero for success or -errno
> + */
> +int drm_atomic_helper_check_with_zpos(struct drm_device *dev,
> +				      struct drm_atomic_state *state)
> +{
> +	int ret;
> +
> +	ret = drm_atomic_helper_check_modeset(dev, state);
> +	if (ret)
> +		return ret;
> +
> +	ret = drm_atomic_normalize_zpos(dev, state);
> +	if (ret)
> +		return ret;
> +
> +	ret = drm_atomic_helper_check_planes(dev, state);
> +	if (ret)
> +		return ret;
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_check_with_zpos);
> +
>  static void
>  disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
>  {
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.c b/drivers/gpu/drm/exynos/exynos_drm_fb.c
> index 40ce841eb952..5c0930af01fa 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_fb.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_fb.c
> @@ -190,7 +190,7 @@ dma_addr_t exynos_drm_fb_dma_addr(struct drm_framebuffer *fb, int index)
>  static const struct drm_mode_config_funcs exynos_drm_mode_config_funcs = {
>  	.fb_create = exynos_user_fb_create,
>  	.output_poll_changed = exynos_drm_output_poll_changed,
> -	.atomic_check = drm_atomic_helper_check,
> +	.atomic_check = drm_atomic_helper_check_with_zpos,
>  	.atomic_commit = exynos_atomic_commit,
>  };
>  
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> index bd9c3bb9252c..2cfd35f3f2f6 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> @@ -231,7 +231,7 @@ static int rcar_du_atomic_check(struct drm_device *dev,
>  	struct rcar_du_device *rcdu = dev->dev_private;
>  	int ret;
>  
> -	ret = drm_atomic_helper_check(dev, state);
> +	ret = drm_atomic_helper_check_with_zpos(dev, state);
>  	if (ret < 0)
>  		return ret;
>  
> diff --git a/drivers/gpu/drm/sti/sti_drv.c b/drivers/gpu/drm/sti/sti_drv.c
> index 2784919a7366..df5f150021d0 100644
> --- a/drivers/gpu/drm/sti/sti_drv.c
> +++ b/drivers/gpu/drm/sti/sti_drv.c
> @@ -248,7 +248,7 @@ static void sti_output_poll_changed(struct drm_device *ddev)
>  static const struct drm_mode_config_funcs sti_mode_config_funcs = {
>  	.fb_create = drm_fb_cma_create,
>  	.output_poll_changed = sti_output_poll_changed,
> -	.atomic_check = drm_atomic_helper_check,
> +	.atomic_check = drm_atomic_helper_check_with_zpos,
>  	.atomic_commit = sti_atomic_commit,
>  };
>  
> diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
> index 7ff92b09fd9c..b1e7193c9d1c 100644
> --- a/include/drm/drm_atomic_helper.h
> +++ b/include/drm/drm_atomic_helper.h
> @@ -40,6 +40,8 @@ int drm_atomic_helper_check_planes(struct drm_device *dev,
>  			       struct drm_atomic_state *state);
>  int drm_atomic_helper_check(struct drm_device *dev,
>  			    struct drm_atomic_state *state);
> +int drm_atomic_helper_check_with_zpos(struct drm_device *dev,
> +				      struct drm_atomic_state *state);
>  void drm_atomic_helper_commit_tail(struct drm_atomic_state *state);
>  int drm_atomic_helper_commit(struct drm_device *dev,
>  			     struct drm_atomic_state *state,
> -- 
> 2.7.4
On Tue, Oct 25, 2016 at 10:43 AM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> On Mon, Oct 10, 2016 at 03:19:47PM +0300, ville.syrjala@linux.intel.com wrote:
>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>
>> We don't want all planes to be added to the state whenever a
>> plane with fixed zpos gets enabled/disabled. This is true
>> especially for eg. cursor planes on i915, as we want cursor
>> updates to go through w/o throttling. Same holds for drivers
>> that don't support zpos at all (i915 actually falls into this
>> category right now since we've not yet added zpos support).
>>
>> Allow drivers more freedom by letting them deal with zpos
>> themselves instead of doing it in drm_atomic_helper_check_planes()
>> unconditionally. Easiest solution seems to be to move the call
>> up to drm_atomic_helper_check(). But as some drivers might want
>> to use that function without the zpos handling, let's provide
>> two variants: the normal one, and one that deals with zpos.
>>
>> Cc: Marek Szyprowski <m.szyprowski@samsung.com>
>> Cc: Benjamin Gaignard <benjamin.gaignard@linaro.org>
>> Cc: Vincent Abriou <vincent.abriou@st.com>
>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> Cc: Inki Dae <inki.dae@samsung.com>
>> Cc: Joonyoung Shim <jy0922.shim@samsung.com>
>> Cc: Seung-Woo Kim <sw0312.kim@samsung.com>
>> Cc: Kyungmin Park <kyungmin.park@samsung.com>
>> Cc: Lyude <cpaul@redhat.com>
>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Cc: stable@vger.kernel.org
>> Fixes: 44d1240d006c ("drm: add generic zpos property")
>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Ping. Can I get some buy-in from the relevant folks?
>

Reviewed-by: Sean Paul <seanpaul@chromium.org>

FWIW :)

>> ---
>>  drivers/gpu/drm/drm_atomic_helper.c    | 46 +++++++++++++++++++++++++++++++---
>>  drivers/gpu/drm/exynos/exynos_drm_fb.c |  2 +-
>>  drivers/gpu/drm/rcar-du/rcar_du_kms.c  |  2 +-
>>  drivers/gpu/drm/sti/sti_drv.c          |  2 +-
>>  include/drm/drm_atomic_helper.h        |  2 ++
>>  5 files changed, 47 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>> index c3f83476f996..cd19281cdefb 100644
>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>> @@ -594,10 +594,6 @@ drm_atomic_helper_check_planes(struct drm_device *dev,
>>       struct drm_plane_state *plane_state;
>>       int i, ret = 0;
>>
>> -     ret = drm_atomic_normalize_zpos(dev, state);
>> -     if (ret)
>> -             return ret;
>> -
>>       for_each_plane_in_state(state, plane, plane_state, i) {
>>               const struct drm_plane_helper_funcs *funcs;
>>
>> @@ -673,6 +669,48 @@ int drm_atomic_helper_check(struct drm_device *dev,
>>  }
>>  EXPORT_SYMBOL(drm_atomic_helper_check);
>>
>> +/**
>> + * drm_atomic_helper_check_with_zpos - validate state object, dealing with zpos
>> + * @dev: DRM device
>> + * @state: the driver state object
>> + *
>> + * Check the state object to see if the requested state is physically possible.
>> + * Only crtcs and planes have check callbacks, so for any additional (global)
>> + * checking that a driver needs it can simply wrap that around this function.
>> + * Drivers without such needs can directly use this as their ->atomic_check()
>> + * callback.
>> + *
>> + * This just wraps the two parts of the state checking for planes and modeset
>> + * state in the default order: First it calls drm_atomic_helper_check_modeset(),
>> + * followed by drm_atomic_normalize_zpos(), and finally
>> + * drm_atomic_helper_check_planes(). The assumption is that the
>> + * ->atomic_check functions depend upon an updated adjusted_mode.clock to
>> + * e.g. properly compute watermarks.
>> + *
>> + * RETURNS:
>> + * Zero for success or -errno
>> + */
>> +int drm_atomic_helper_check_with_zpos(struct drm_device *dev,
>> +                                   struct drm_atomic_state *state)
>> +{
>> +     int ret;
>> +
>> +     ret = drm_atomic_helper_check_modeset(dev, state);
>> +     if (ret)
>> +             return ret;
>> +
>> +     ret = drm_atomic_normalize_zpos(dev, state);
>> +     if (ret)
>> +             return ret;
>> +
>> +     ret = drm_atomic_helper_check_planes(dev, state);
>> +     if (ret)
>> +             return ret;
>> +
>> +     return ret;
>> +}
>> +EXPORT_SYMBOL(drm_atomic_helper_check_with_zpos);
>> +
>>  static void
>>  disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
>>  {
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.c b/drivers/gpu/drm/exynos/exynos_drm_fb.c
>> index 40ce841eb952..5c0930af01fa 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_fb.c
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_fb.c
>> @@ -190,7 +190,7 @@ dma_addr_t exynos_drm_fb_dma_addr(struct drm_framebuffer *fb, int index)
>>  static const struct drm_mode_config_funcs exynos_drm_mode_config_funcs = {
>>       .fb_create = exynos_user_fb_create,
>>       .output_poll_changed = exynos_drm_output_poll_changed,
>> -     .atomic_check = drm_atomic_helper_check,
>> +     .atomic_check = drm_atomic_helper_check_with_zpos,
>>       .atomic_commit = exynos_atomic_commit,
>>  };
>>
>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
>> index bd9c3bb9252c..2cfd35f3f2f6 100644
>> --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
>> @@ -231,7 +231,7 @@ static int rcar_du_atomic_check(struct drm_device *dev,
>>       struct rcar_du_device *rcdu = dev->dev_private;
>>       int ret;
>>
>> -     ret = drm_atomic_helper_check(dev, state);
>> +     ret = drm_atomic_helper_check_with_zpos(dev, state);
>>       if (ret < 0)
>>               return ret;
>>
>> diff --git a/drivers/gpu/drm/sti/sti_drv.c b/drivers/gpu/drm/sti/sti_drv.c
>> index 2784919a7366..df5f150021d0 100644
>> --- a/drivers/gpu/drm/sti/sti_drv.c
>> +++ b/drivers/gpu/drm/sti/sti_drv.c
>> @@ -248,7 +248,7 @@ static void sti_output_poll_changed(struct drm_device *ddev)
>>  static const struct drm_mode_config_funcs sti_mode_config_funcs = {
>>       .fb_create = drm_fb_cma_create,
>>       .output_poll_changed = sti_output_poll_changed,
>> -     .atomic_check = drm_atomic_helper_check,
>> +     .atomic_check = drm_atomic_helper_check_with_zpos,
>>       .atomic_commit = sti_atomic_commit,
>>  };
>>
>> diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
>> index 7ff92b09fd9c..b1e7193c9d1c 100644
>> --- a/include/drm/drm_atomic_helper.h
>> +++ b/include/drm/drm_atomic_helper.h
>> @@ -40,6 +40,8 @@ int drm_atomic_helper_check_planes(struct drm_device *dev,
>>                              struct drm_atomic_state *state);
>>  int drm_atomic_helper_check(struct drm_device *dev,
>>                           struct drm_atomic_state *state);
>> +int drm_atomic_helper_check_with_zpos(struct drm_device *dev,
>> +                                   struct drm_atomic_state *state);
>>  void drm_atomic_helper_commit_tail(struct drm_atomic_state *state);
>>  int drm_atomic_helper_commit(struct drm_device *dev,
>>                            struct drm_atomic_state *state,
>> --
>> 2.7.4
>
> --
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
I have just tested it on my board, no regression :-)

Acked-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>

2016-10-25 22:53 GMT+02:00 Sean Paul <seanpaul@chromium.org>:
> On Tue, Oct 25, 2016 at 10:43 AM, Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
>> On Mon, Oct 10, 2016 at 03:19:47PM +0300, ville.syrjala@linux.intel.com wrote:
>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>
>>> We don't want all planes to be added to the state whenever a
>>> plane with fixed zpos gets enabled/disabled. This is true
>>> especially for eg. cursor planes on i915, as we want cursor
>>> updates to go through w/o throttling. Same holds for drivers
>>> that don't support zpos at all (i915 actually falls into this
>>> category right now since we've not yet added zpos support).
>>>
>>> Allow drivers more freedom by letting them deal with zpos
>>> themselves instead of doing it in drm_atomic_helper_check_planes()
>>> unconditionally. Easiest solution seems to be to move the call
>>> up to drm_atomic_helper_check(). But as some drivers might want
>>> to use that function without the zpos handling, let's provide
>>> two variants: the normal one, and one that deals with zpos.
>>>
>>> Cc: Marek Szyprowski <m.szyprowski@samsung.com>
>>> Cc: Benjamin Gaignard <benjamin.gaignard@linaro.org>
>>> Cc: Vincent Abriou <vincent.abriou@st.com>
>>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>> Cc: Inki Dae <inki.dae@samsung.com>
>>> Cc: Joonyoung Shim <jy0922.shim@samsung.com>
>>> Cc: Seung-Woo Kim <sw0312.kim@samsung.com>
>>> Cc: Kyungmin Park <kyungmin.park@samsung.com>
>>> Cc: Lyude <cpaul@redhat.com>
>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>> Cc: stable@vger.kernel.org
>>> Fixes: 44d1240d006c ("drm: add generic zpos property")
>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>
>> Ping. Can I get some buy-in from the relevant folks?
>>
>
> Reviewed-by: Sean Paul <seanpaul@chromium.org>
>
> FWIW :)
>
>>> ---
>>>  drivers/gpu/drm/drm_atomic_helper.c    | 46 +++++++++++++++++++++++++++++++---
>>>  drivers/gpu/drm/exynos/exynos_drm_fb.c |  2 +-
>>>  drivers/gpu/drm/rcar-du/rcar_du_kms.c  |  2 +-
>>>  drivers/gpu/drm/sti/sti_drv.c          |  2 +-
>>>  include/drm/drm_atomic_helper.h        |  2 ++
>>>  5 files changed, 47 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>>> index c3f83476f996..cd19281cdefb 100644
>>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>>> @@ -594,10 +594,6 @@ drm_atomic_helper_check_planes(struct drm_device *dev,
>>>       struct drm_plane_state *plane_state;
>>>       int i, ret = 0;
>>>
>>> -     ret = drm_atomic_normalize_zpos(dev, state);
>>> -     if (ret)
>>> -             return ret;
>>> -
>>>       for_each_plane_in_state(state, plane, plane_state, i) {
>>>               const struct drm_plane_helper_funcs *funcs;
>>>
>>> @@ -673,6 +669,48 @@ int drm_atomic_helper_check(struct drm_device *dev,
>>>  }
>>>  EXPORT_SYMBOL(drm_atomic_helper_check);
>>>
>>> +/**
>>> + * drm_atomic_helper_check_with_zpos - validate state object, dealing with zpos
>>> + * @dev: DRM device
>>> + * @state: the driver state object
>>> + *
>>> + * Check the state object to see if the requested state is physically possible.
>>> + * Only crtcs and planes have check callbacks, so for any additional (global)
>>> + * checking that a driver needs it can simply wrap that around this function.
>>> + * Drivers without such needs can directly use this as their ->atomic_check()
>>> + * callback.
>>> + *
>>> + * This just wraps the two parts of the state checking for planes and modeset
>>> + * state in the default order: First it calls drm_atomic_helper_check_modeset(),
>>> + * followed by drm_atomic_normalize_zpos(), and finally
>>> + * drm_atomic_helper_check_planes(). The assumption is that the
>>> + * ->atomic_check functions depend upon an updated adjusted_mode.clock to
>>> + * e.g. properly compute watermarks.
>>> + *
>>> + * RETURNS:
>>> + * Zero for success or -errno
>>> + */
>>> +int drm_atomic_helper_check_with_zpos(struct drm_device *dev,
>>> +                                   struct drm_atomic_state *state)
>>> +{
>>> +     int ret;
>>> +
>>> +     ret = drm_atomic_helper_check_modeset(dev, state);
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +     ret = drm_atomic_normalize_zpos(dev, state);
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +     ret = drm_atomic_helper_check_planes(dev, state);
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +     return ret;
>>> +}
>>> +EXPORT_SYMBOL(drm_atomic_helper_check_with_zpos);
>>> +
>>>  static void
>>>  disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
>>>  {
>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.c b/drivers/gpu/drm/exynos/exynos_drm_fb.c
>>> index 40ce841eb952..5c0930af01fa 100644
>>> --- a/drivers/gpu/drm/exynos/exynos_drm_fb.c
>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_fb.c
>>> @@ -190,7 +190,7 @@ dma_addr_t exynos_drm_fb_dma_addr(struct drm_framebuffer *fb, int index)
>>>  static const struct drm_mode_config_funcs exynos_drm_mode_config_funcs = {
>>>       .fb_create = exynos_user_fb_create,
>>>       .output_poll_changed = exynos_drm_output_poll_changed,
>>> -     .atomic_check = drm_atomic_helper_check,
>>> +     .atomic_check = drm_atomic_helper_check_with_zpos,
>>>       .atomic_commit = exynos_atomic_commit,
>>>  };
>>>
>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
>>> index bd9c3bb9252c..2cfd35f3f2f6 100644
>>> --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
>>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
>>> @@ -231,7 +231,7 @@ static int rcar_du_atomic_check(struct drm_device *dev,
>>>       struct rcar_du_device *rcdu = dev->dev_private;
>>>       int ret;
>>>
>>> -     ret = drm_atomic_helper_check(dev, state);
>>> +     ret = drm_atomic_helper_check_with_zpos(dev, state);
>>>       if (ret < 0)
>>>               return ret;
>>>
>>> diff --git a/drivers/gpu/drm/sti/sti_drv.c b/drivers/gpu/drm/sti/sti_drv.c
>>> index 2784919a7366..df5f150021d0 100644
>>> --- a/drivers/gpu/drm/sti/sti_drv.c
>>> +++ b/drivers/gpu/drm/sti/sti_drv.c
>>> @@ -248,7 +248,7 @@ static void sti_output_poll_changed(struct drm_device *ddev)
>>>  static const struct drm_mode_config_funcs sti_mode_config_funcs = {
>>>       .fb_create = drm_fb_cma_create,
>>>       .output_poll_changed = sti_output_poll_changed,
>>> -     .atomic_check = drm_atomic_helper_check,
>>> +     .atomic_check = drm_atomic_helper_check_with_zpos,
>>>       .atomic_commit = sti_atomic_commit,
>>>  };
>>>
>>> diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
>>> index 7ff92b09fd9c..b1e7193c9d1c 100644
>>> --- a/include/drm/drm_atomic_helper.h
>>> +++ b/include/drm/drm_atomic_helper.h
>>> @@ -40,6 +40,8 @@ int drm_atomic_helper_check_planes(struct drm_device *dev,
>>>                              struct drm_atomic_state *state);
>>>  int drm_atomic_helper_check(struct drm_device *dev,
>>>                           struct drm_atomic_state *state);
>>> +int drm_atomic_helper_check_with_zpos(struct drm_device *dev,
>>> +                                   struct drm_atomic_state *state);
>>>  void drm_atomic_helper_commit_tail(struct drm_atomic_state *state);
>>>  int drm_atomic_helper_commit(struct drm_device *dev,
>>>                            struct drm_atomic_state *state,
>>> --
>>> 2.7.4
>>
>> --
>> Ville Syrjälä
>> Intel OTC
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Wed, Oct 26, 2016 at 04:09:00PM +0200, Benjamin Gaignard wrote:
> I have just tested it on my board, no regression :-)
> 
> Acked-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
> 
> 2016-10-25 22:53 GMT+02:00 Sean Paul <seanpaul@chromium.org>:
> > On Tue, Oct 25, 2016 at 10:43 AM, Ville Syrjälä
> > <ville.syrjala@linux.intel.com> wrote:
> >> On Mon, Oct 10, 2016 at 03:19:47PM +0300, ville.syrjala@linux.intel.com wrote:
> >>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>>
> >>> We don't want all planes to be added to the state whenever a
> >>> plane with fixed zpos gets enabled/disabled. This is true
> >>> especially for eg. cursor planes on i915, as we want cursor
> >>> updates to go through w/o throttling. Same holds for drivers
> >>> that don't support zpos at all (i915 actually falls into this
> >>> category right now since we've not yet added zpos support).
> >>>
> >>> Allow drivers more freedom by letting them deal with zpos
> >>> themselves instead of doing it in drm_atomic_helper_check_planes()
> >>> unconditionally. Easiest solution seems to be to move the call
> >>> up to drm_atomic_helper_check(). But as some drivers might want
> >>> to use that function without the zpos handling, let's provide
> >>> two variants: the normal one, and one that deals with zpos.
> >>>
> >>> Cc: Marek Szyprowski <m.szyprowski@samsung.com>
> >>> Cc: Benjamin Gaignard <benjamin.gaignard@linaro.org>
> >>> Cc: Vincent Abriou <vincent.abriou@st.com>
> >>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>> Cc: Inki Dae <inki.dae@samsung.com>
> >>> Cc: Joonyoung Shim <jy0922.shim@samsung.com>
> >>> Cc: Seung-Woo Kim <sw0312.kim@samsung.com>
> >>> Cc: Kyungmin Park <kyungmin.park@samsung.com>
> >>> Cc: Lyude <cpaul@redhat.com>
> >>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >>> Cc: stable@vger.kernel.org
> >>> Fixes: 44d1240d006c ("drm: add generic zpos property")
> >>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>
> >> Ping. Can I get some buy-in from the relevant folks?
> >>
> >
> > Reviewed-by: Sean Paul <seanpaul@chromium.org>
> >
> > FWIW :)

Applied to drm-misc-fixes, thanks.
-Daniel