diff mbox

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

Message ID 1464686243-25418-5-git-send-email-tomi.valkeinen@ti.com (mailing list archive)
State New, archived
Headers show

Commit Message

Tomi Valkeinen May 31, 2016, 9:17 a.m. UTC
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(-)

Comments

Daniel Vetter May 31, 2016, 10:56 a.m. UTC | #1
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 May 31, 2016, 10:59 a.m. UTC | #2
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
diff mbox

Patch

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