| Message ID | 20191112143643.21821-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] drm/i915: Explicitly cleanup initial_plane_config | expand |
On Tue, Nov 12, 2019 at 02:36:42PM +0000, Chris Wilson wrote: > I am about to stuff more objects into the plane_config and would like to > have it clean up after itself. Move the current framebuffer release into > a common function so it can be extended with the new object with > relative ease. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/display/intel_display.c | 20 +++++++++++++++----- > 1 file changed, 15 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > index 5f3340554149..f571c6575c62 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -3241,8 +3241,6 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc, > goto valid_fb; > } > > - kfree(plane_config->fb); > - > /* > * Failed to alloc the obj, check to see if we should share > * an fb with another CRTC instead > @@ -3262,7 +3260,6 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc, > > if (intel_plane_ggtt_offset(state) == plane_config->base) { > fb = state->hw.fb; > - drm_framebuffer_get(fb); > goto valid_fb; > } > } > @@ -3295,7 +3292,6 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc, > intel_crtc->pipe, PTR_ERR(intel_state->vma)); > > intel_state->vma = NULL; > - drm_framebuffer_put(fb); > return; > } > > @@ -3317,7 +3313,6 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc, > if (plane_config->tiling) > dev_priv->preserve_bios_swizzle = true; > > - plane_state->fb = fb; This stuff looks wrong. We still want the plane state to point at the fb. Ideally I'd like to nuke the plane_config thing entirely and just read everything directly into the plane_states(s), but that's probably a bunch of actual work so not going to happen soon. > plane_state->crtc = &intel_crtc->base; > intel_plane_copy_uapi_to_hw_state(intel_state, intel_state); > > @@ -16952,6 +16947,19 @@ static void intel_mode_config_init(struct drm_i915_private *i915) > } > } > > +static void plane_config_fini(struct intel_initial_plane_config *plane_config) > +{ > + if (plane_config->fb) { > + struct drm_framebuffer *fb = &plane_config->fb->base; > + > + /* We may only have the stub and not a full framebuffer */ > + if (drm_framebuffer_read_refcount(fb)) > + drm_framebuffer_put(fb); > + else > + kfree(fb); > + } > +} > + > int intel_modeset_init(struct drm_i915_private *i915) > { > struct drm_device *dev = &i915->drm; > @@ -17036,6 +17044,8 @@ int intel_modeset_init(struct drm_i915_private *i915) > * just get the first one. > */ > intel_find_initial_plane_obj(crtc, &plane_config); > + > + plane_config_fini(&plane_config); > } > > /* > -- > 2.24.0
Quoting Ville Syrjälä (2019-11-12 15:20:52) > On Tue, Nov 12, 2019 at 02:36:42PM +0000, Chris Wilson wrote: > > @@ -3317,7 +3313,6 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc, > > if (plane_config->tiling) > > dev_priv->preserve_bios_swizzle = true; > > > > - plane_state->fb = fb; > > This stuff looks wrong. We still want the plane state to point at the > fb. Oh, it used to be @@ -3266,8 +3262,9 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc, if (plane_config->tiling) dev_priv->preserve_bios_swizzle = true; - plane_state->fb = fb; plane_state->crtc = &intel_crtc->base; + plane_state->fb = fb; + drm_framebuffer_get(fb); atomic_or(to_intel_plane(primary)->frontbuffer_bit, &to_intel_frontbuffer(fb)->bits); I'll blame it on not paying enough attention during the rebase. -Chris
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index 5f3340554149..f571c6575c62 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -3241,8 +3241,6 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc, goto valid_fb; } - kfree(plane_config->fb); - /* * Failed to alloc the obj, check to see if we should share * an fb with another CRTC instead @@ -3262,7 +3260,6 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc, if (intel_plane_ggtt_offset(state) == plane_config->base) { fb = state->hw.fb; - drm_framebuffer_get(fb); goto valid_fb; } } @@ -3295,7 +3292,6 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc, intel_crtc->pipe, PTR_ERR(intel_state->vma)); intel_state->vma = NULL; - drm_framebuffer_put(fb); return; } @@ -3317,7 +3313,6 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc, if (plane_config->tiling) dev_priv->preserve_bios_swizzle = true; - plane_state->fb = fb; plane_state->crtc = &intel_crtc->base; intel_plane_copy_uapi_to_hw_state(intel_state, intel_state); @@ -16952,6 +16947,19 @@ static void intel_mode_config_init(struct drm_i915_private *i915) } } +static void plane_config_fini(struct intel_initial_plane_config *plane_config) +{ + if (plane_config->fb) { + struct drm_framebuffer *fb = &plane_config->fb->base; + + /* We may only have the stub and not a full framebuffer */ + if (drm_framebuffer_read_refcount(fb)) + drm_framebuffer_put(fb); + else + kfree(fb); + } +} + int intel_modeset_init(struct drm_i915_private *i915) { struct drm_device *dev = &i915->drm; @@ -17036,6 +17044,8 @@ int intel_modeset_init(struct drm_i915_private *i915) * just get the first one. */ intel_find_initial_plane_obj(crtc, &plane_config); + + plane_config_fini(&plane_config); } /*
I am about to stuff more objects into the plane_config and would like to have it clean up after itself. Move the current framebuffer release into a common function so it can be extended with the new object with relative ease. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> --- drivers/gpu/drm/i915/display/intel_display.c | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-)