Re: [PATCH v2 2/4] drm/fb-helper: Keep references for the current set of used connectors

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Oct 26, 2016 at 02:54:45PM +0100, Chris Wilson wrote:
> On Wed, Oct 26, 2016 at 04:31:20PM +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > The fbdev helper code keeps around two lists of connectors. One is the
> > list of all connectors it could use, and that list already holds
> > references for all the connectors. However the other list, or rather
> > lists, is the one actively being used. That list is tracked per-crtc
> > and currently doesn't hold any extra references. Let's grab those
> > extra references to avoid oopsing when the connector vanishes. The
> > list of all possible connectors should get updated when the hpd happens,
> > but the list of actively used connectors would not get updated until
> > the next time the fb-helper picks through the set of possible connectors.
> > And so we need to hang on to the connectors until that time.
> > 
> > Since we need to clean up in drm_fb_helper_crtc_free() as well,
> > let's pull the code to a common place. And while at it let's
> > pull in up the modeset->mode cleanup in there as well. The case
> > of modeset->fb is a bit less clear. I'm thinking we should probably
> > hold a reference to it, but for now I just slapped on a FIXME.
> > 
> > v2: Cleanup things drm_fb_helper_crtc_free() too (Chris)
> > 
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: stable@vger.kernel.org
> > Cc: Carlos Santa <carlos.santa@intel.com>
> > Cc: Kirill A. Shutemov <kirill@shutemov.name>
> > Tested-by: Carlos Santa <carlos.santa@intel.com> (v1)
> > Tested-by: Kirill A. Shutemov <kirill@shutemov.name> (v1)
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97666
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/drm_fb_helper.c | 58 +++++++++++++++++++++++------------------
> >  1 file changed, 32 insertions(+), 26 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> > index db469d12d195..83961f1a97d2 100644
> > --- a/drivers/gpu/drm/drm_fb_helper.c
> > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > @@ -605,6 +605,24 @@ int drm_fb_helper_blank(int blank, struct fb_info *info)
> >  }
> >  EXPORT_SYMBOL(drm_fb_helper_blank);
> >  
> > +static void drm_fb_helper_modeset_free(struct drm_fb_helper *helper,
> > +				       struct drm_mode_set *modeset)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < modeset->num_connectors; i++) {
> > +		drm_connector_unreference(modeset->connectors[i]);
> > +		modeset->connectors[i] = NULL;
> > +	}
> > +	modeset->num_connectors = 0;
> > +
> > +	drm_mode_destroy(helper->dev, modeset->mode);
> > +	modeset->mode = NULL;
> > +
> > +	/* FIXME should hold a ref? */
> > +	modeset->fb = NULL;
> > +}
> > +
> >  static void drm_fb_helper_crtc_free(struct drm_fb_helper *helper)
> >  {
> >  	int i;
> > @@ -614,11 +632,10 @@ static void drm_fb_helper_crtc_free(struct drm_fb_helper *helper)
> >  		kfree(helper->connector_info[i]);
> >  	}
> >  	kfree(helper->connector_info);
> > -	for (i = 0; i < helper->crtc_count; i++) {
> > -		kfree(helper->crtc_info[i].mode_set.connectors);
> > -		if (helper->crtc_info[i].mode_set.mode)
> > -			drm_mode_destroy(helper->dev, helper->crtc_info[i].mode_set.mode);
> > -	}
> > +
> > +	for (i = 0; i < helper->crtc_count; i++)
> > +		drm_fb_helper_modeset_free(helper,
> > +					   &helper->crtc_info[i].mode_set);
> 
> We lose the kfree(mode_set.connectors) here.

Doh.

> 
> So 
> 	for (i = 0; i < helper->crtc_count; i++)
> 		struct drm_mode_set *modeset = &helper->crtc_info[i].mode_set);
> 
> 		drm_fb_helper_modeset_release(helper, modeset);

Hmm. Yeah _release() does seem better than _free() if we don't actually
free all of it.

> 		kfree(modeset->connectors);
> 	}
> ?
> 
> Couldn't spot any other missing calls to release the new ref, so with
> the tiny leak fixed,
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Thanks. I'll respin it once more.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel