[PATCH 0/4] drm: fix two atomic modesetting issues on v4.7-rc1

[PATCH 0/4] drm: fix two atomic modesetting issues on v4.7-rc1

[Tomi Valkeinen]

These fix two atomic modesetting issues on v4.7-rc1. The last one is a bit of
an RFC.

 Tomi

Tomi Valkeinen (4):
  drm: add missing drm_mode_set_crtcinfo call
  drm/sti: remove extra mode fixup
  drm: make drm_atomic_set_mode_prop_for_crtc() more reliable
  drm: fix fb refcount issue with atomic modesetting

 drivers/gpu/drm/drm_atomic.c   | 16 ++++++++++++----
 drivers/gpu/drm/drm_crtc.c     |  5 ++---
 drivers/gpu/drm/drm_modes.c    |  2 ++
 drivers/gpu/drm/sti/sti_crtc.c | 10 ----------
 4 files changed, 16 insertions(+), 17 deletions(-)

-- 
2.5.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/4] drm: add missing drm_mode_set_crtcinfo call

[Tomi Valkeinen]

When setting mode via MODE_ID property,
drm_atomic_set_mode_prop_for_crtc() does not call
drm_mode_set_crtcinfo() which possibly causes:

"[drm:drm_calc_timestamping_constants [drm]] *ERROR* crtc 32: Can't
calculate constants, dotclock = 0!"

Whether the error is seen depends on the previous data in state->mode,
as state->mode is not cleared when setting new mode.

This patch adds drm_mode_set_crtcinfo() call to
drm_mode_convert_umode(), which is called in both legacy and atomic
paths. This should be fine as there's no reason to call
drm_mode_convert_umode() without also setting the crtc related fields.

drm_mode_set_crtcinfo() is removed from the legacy drm_mode_setcrtc() as
that is no longer needed.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/drm_crtc.c  | 2 --
 drivers/gpu/drm/drm_modes.c | 2 ++
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index d2a6d958ca76..06b6e2173697 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -2821,8 +2821,6 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
 			goto out;
 		}
 
-		drm_mode_set_crtcinfo(mode, CRTC_INTERLACE_HALVE_V);
-
 		/*
 		 * Check whether the primary plane supports the fb pixel format.
 		 * Drivers not implementing the universal planes API use a
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index 7def3d58da18..e5e6f504d8cc 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -1518,6 +1518,8 @@ int drm_mode_convert_umode(struct drm_display_mode *out,
 	if (out->status != MODE_OK)
 		goto out;
 
+	drm_mode_set_crtcinfo(out, CRTC_INTERLACE_HALVE_V);
+
 	ret = 0;
 
 out:
-- 
2.5.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/4] drm/sti: remove extra mode fixup

[Tomi Valkeinen]

Commit 652353e6e561c2aeeac62df183f721f6f9b5b45f ("drm/sti: set CRTC
modesetting parameters") added a hack to avoid warnings related to
setting mode with atomic API. With the previous patch, the hack should
no longer be necessary.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: Benjamin Gaignard <benjamin.gaignard@linaro.org>
Cc: Vincent Abriou <vincent.abriou@st.com>
---
 drivers/gpu/drm/sti/sti_crtc.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/drivers/gpu/drm/sti/sti_crtc.c b/drivers/gpu/drm/sti/sti_crtc.c
index 505620c7c2c8..e04deedabd4a 100644
--- a/drivers/gpu/drm/sti/sti_crtc.c
+++ b/drivers/gpu/drm/sti/sti_crtc.c
@@ -51,15 +51,6 @@ static void sti_crtc_disabling(struct drm_crtc *crtc)
 	mixer->status = STI_MIXER_DISABLING;
 }
 
-static bool sti_crtc_mode_fixup(struct drm_crtc *crtc,
-				const struct drm_display_mode *mode,
-				struct drm_display_mode *adjusted_mode)
-{
-	/* accept the provided drm_display_mode, do not fix it up */
-	drm_mode_set_crtcinfo(adjusted_mode, CRTC_INTERLACE_HALVE_V);
-	return true;
-}
-
 static int
 sti_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mode)
 {
@@ -230,7 +221,6 @@ static void sti_crtc_atomic_flush(struct drm_crtc *crtc,
 static const struct drm_crtc_helper_funcs sti_crtc_helper_funcs = {
 	.enable = sti_crtc_enable,
 	.disable = sti_crtc_disabling,
-	.mode_fixup = sti_crtc_mode_fixup,
 	.mode_set = drm_helper_crtc_mode_set,
 	.mode_set_nofb = sti_crtc_mode_set_nofb,
 	.mode_set_base = drm_helper_crtc_mode_set_base,
-- 
2.5.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/4] drm: make drm_atomic_set_mode_prop_for_crtc() more reliable

[Tomi Valkeinen]

drm_atomic_set_mode_prop_for_crtc() has a few issues:

- it doesn't clear the state->mode, so old data may be left there when a
  new mode is set.
- if an error happens, state->mode is left in a partially updated state.

This patch improves the situation by:

- bail out early if blob is of wrong length.
- construct drm_display_mode first in an initialized local variable, and
  copy it to state->mode only when we know the call has succeeded.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/drm_atomic.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 3ff1ed7b33db..5bfecb2bbedf 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -348,16 +348,24 @@ int drm_atomic_set_mode_prop_for_crtc(struct drm_crtc_state *state,
 	if (blob == state->mode_blob)
 		return 0;
 
+	if (blob && blob->length != sizeof(struct drm_mode_modeinfo))
+		return -EINVAL;
+
 	drm_property_unreference_blob(state->mode_blob);
 	state->mode_blob = NULL;
 
 	if (blob) {
-		if (blob->length != sizeof(struct drm_mode_modeinfo) ||
-		    drm_mode_convert_umode(&state->mode,
-		                           (const struct drm_mode_modeinfo *)
-		                            blob->data))
+		const struct drm_mode_modeinfo *umode =
+			(const struct drm_mode_modeinfo *)blob->data;
+		struct drm_display_mode mode;
+
+		memset(&mode, 0, sizeof(mode));
+
+		if (drm_mode_convert_umode(&mode, umode))
 			return -EINVAL;
 
+		state->mode = mode;
+
 		state->mode_blob = drm_property_reference_blob(blob);
 		state->enable = true;
 		DRM_DEBUG_ATOMIC("Set [MODE:%s] for CRTC state %p\n",
-- 
2.5.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 4/4] drm: fix fb refcount issue with atomic modesetting

[Tomi Valkeinen]

After commit 027b3f8ba9277410c3191d72d1ed2c6146d8a668 ("drm/modes: stop
handling framebuffer special") extra fb refs are left around when doing
atomic modesetting.

The problem is that the new drm_property_change_valid_get() does not
return anything in the '**ref' parameter, which causes
'drm_property_change_valid_put' never to be called.

For some reason this doesn't cause problems with legacy API.

Also, previously the code only set the 'ref' variable for fbs, with this
patch the 'ref' is set for all objects.

So this is a bit of an RFC, as I don't understand all what's going on
here.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/drm_crtc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 06b6e2173697..0e3cc66aa8b7 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -4839,7 +4839,8 @@ bool drm_property_change_valid_get(struct drm_property *property,
 		if (value == 0)
 			return true;
 
-		return _object_find(property->dev, value, property->values[0]) != NULL;
+		*ref = _object_find(property->dev, value, property->values[0]);
+		return *ref != NULL;
 	}
 
 	for (i = 0; i < property->num_values; i++)
-- 
2.5.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/4] drm: make drm_atomic_set_mode_prop_for_crtc() more reliable

[Daniel Vetter]

On Tue, May 31, 2016 at 12:17:22PM +0300, Tomi Valkeinen wrote:
> drm_atomic_set_mode_prop_for_crtc() has a few issues:
> 
> - it doesn't clear the state->mode, so old data may be left there when a
>   new mode is set.
> - if an error happens, state->mode is left in a partially updated state.
> 
> This patch improves the situation by:
> 
> - bail out early if blob is of wrong length.
> - construct drm_display_mode first in an initialized local variable, and
>   copy it to state->mode only when we know the call has succeeded.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/drm_atomic.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 3ff1ed7b33db..5bfecb2bbedf 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -348,16 +348,24 @@ int drm_atomic_set_mode_prop_for_crtc(struct drm_crtc_state *state,
>  	if (blob == state->mode_blob)
>  		return 0;
>  
> +	if (blob && blob->length != sizeof(struct drm_mode_modeinfo))
> +		return -EINVAL;
> +
>  	drm_property_unreference_blob(state->mode_blob);
>  	state->mode_blob = NULL;
>  
>  	if (blob) {
> -		if (blob->length != sizeof(struct drm_mode_modeinfo) ||
> -		    drm_mode_convert_umode(&state->mode,
> -		                           (const struct drm_mode_modeinfo *)
> -		                            blob->data))
> +		const struct drm_mode_modeinfo *umode =
> +			(const struct drm_mode_modeinfo *)blob->data;
> +		struct drm_display_mode mode;
> +
> +		memset(&mode, 0, sizeof(mode));
> +
> +		if (drm_mode_convert_umode(&mode, umode))
>  			return -EINVAL;

The code flow changes aren't required - when an atomic commit fails we
throw away all the state structures, they're duplicates. Hence leaving
garabge in there is totally fine.

I'd just move the memset in front of the if (blob), that should take care
of everything.
-Daniel

>  
> +		state->mode = mode;
> +
>  		state->mode_blob = drm_property_reference_blob(blob);
>  		state->enable = true;
>  		DRM_DEBUG_ATOMIC("Set [MODE:%s] for CRTC state %p\n",
> -- 
> 2.5.0
> 

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

Re: [PATCH 4/4] drm: fix fb refcount issue with atomic modesetting

[Daniel Vetter]

On Tue, May 31, 2016 at 12:17:23PM +0300, Tomi Valkeinen wrote:
> After commit 027b3f8ba9277410c3191d72d1ed2c6146d8a668 ("drm/modes: stop
> handling framebuffer special") extra fb refs are left around when doing
> atomic modesetting.
> 
> The problem is that the new drm_property_change_valid_get() does not
> return anything in the '**ref' parameter, which causes
> 'drm_property_change_valid_put' never to be called.
> 
> For some reason this doesn't cause problems with legacy API.
> 
> Also, previously the code only set the 'ref' variable for fbs, with this
> patch the 'ref' is set for all objects.
> 
> So this is a bit of an RFC, as I don't understand all what's going on
> here.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

On patches 1,2&4:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Imo everything but the sti patche should also grow a cc: stable, plus an
appropriate Fixes: line for this one here.
-Daniel

> ---
>  drivers/gpu/drm/drm_crtc.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 06b6e2173697..0e3cc66aa8b7 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -4839,7 +4839,8 @@ bool drm_property_change_valid_get(struct drm_property *property,
>  		if (value == 0)
>  			return true;
>  
> -		return _object_find(property->dev, value, property->values[0]) != NULL;
> +		*ref = _object_find(property->dev, value, property->values[0]);
> +		return *ref != NULL;
>  	}
>  
>  	for (i = 0; i < property->num_values; i++)
> -- 
> 2.5.0
> 

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

Re: [PATCH 4/4] drm: fix fb refcount issue with atomic modesetting

[Daniel Vetter]

On Tue, May 31, 2016 at 12:56:23PM +0200, Daniel Vetter wrote:
> On Tue, May 31, 2016 at 12:17:23PM +0300, Tomi Valkeinen wrote:
> > After commit 027b3f8ba9277410c3191d72d1ed2c6146d8a668 ("drm/modes: stop
> > handling framebuffer special") extra fb refs are left around when doing
> > atomic modesetting.
> > 
> > The problem is that the new drm_property_change_valid_get() does not
> > return anything in the '**ref' parameter, which causes
> > 'drm_property_change_valid_put' never to be called.
> > 
> > For some reason this doesn't cause problems with legacy API.
> > 
> > Also, previously the code only set the 'ref' variable for fbs, with this
> > patch the 'ref' is set for all objects.
> > 
> > So this is a bit of an RFC, as I don't understand all what's going on
> > here.
> > 
> > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> 
> On patches 1,2&4:
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> Imo everything but the sti patche should also grow a cc: stable, plus an
> appropriate Fixes: line for this one here.

Correction: Patch 4 seems in 4.7 only, so doens't need the cc: stable. But
patches 1&3 need it I think.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel