| Message ID | 1477488680-16641-1-git-send-email-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. 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); 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> -Chris
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.
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); kfree(helper->crtc_info); } @@ -2090,7 +2107,6 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper) struct drm_fb_helper_crtc **crtcs; struct drm_display_mode **modes; struct drm_fb_offset *offsets; - struct drm_mode_set *modeset; bool *enabled; int width, height; int i; @@ -2138,45 +2154,35 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper) /* need to set the modesets up here for use later */ /* fill out the connector<->crtc mappings into the modesets */ - for (i = 0; i < fb_helper->crtc_count; i++) { - modeset = &fb_helper->crtc_info[i].mode_set; - modeset->num_connectors = 0; - modeset->fb = NULL; - } + for (i = 0; i < fb_helper->crtc_count; i++) + drm_fb_helper_modeset_free(fb_helper, + &fb_helper->crtc_info[i].mode_set); for (i = 0; i < fb_helper->connector_count; i++) { struct drm_display_mode *mode = modes[i]; struct drm_fb_helper_crtc *fb_crtc = crtcs[i]; struct drm_fb_offset *offset = &offsets[i]; - modeset = &fb_crtc->mode_set; + struct drm_mode_set *modeset = &fb_crtc->mode_set; if (mode && fb_crtc) { + struct drm_connector *connector = + fb_helper->connector_info[i]->connector; + DRM_DEBUG_KMS("desired mode %s set on crtc %d (%d,%d)\n", mode->name, fb_crtc->mode_set.crtc->base.id, offset->x, offset->y); + fb_crtc->desired_mode = mode; fb_crtc->x = offset->x; fb_crtc->y = offset->y; - if (modeset->mode) - drm_mode_destroy(dev, modeset->mode); modeset->mode = drm_mode_duplicate(dev, fb_crtc->desired_mode); - modeset->connectors[modeset->num_connectors++] = fb_helper->connector_info[i]->connector; + drm_connector_reference(connector); + modeset->connectors[modeset->num_connectors++] = connector; modeset->fb = fb_helper->fb; modeset->x = offset->x; modeset->y = offset->y; } } - - /* Clear out any old modes if there are no more connected outputs. */ - for (i = 0; i < fb_helper->crtc_count; i++) { - modeset = &fb_helper->crtc_info[i].mode_set; - if (modeset->num_connectors == 0) { - BUG_ON(modeset->fb); - if (modeset->mode) - drm_mode_destroy(dev, modeset->mode); - modeset->mode = NULL; - } - } out: kfree(crtcs); kfree(modes);