| Message ID | 221af8c6-3c2b-ca59-16a3-ff14e17bdde6@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Aug 09, 2016 at 12:15:51PM +0200, Maarten Lankhorst wrote: > With gem requests reworked to only keep some memory around after > it's completed it's now mostly harmless to keep a reference to the > request until the state is destroyed. This makes it easy to hang > on to the request until the plane state is destroyed since it is > just a bunch of memory now. > > On my 64-bits system a i915_gem_request is 352 bytes. With all > 3 crtc's, each 4 planes enabled the total is 4224 bytes, > low enough not to optimize this case too much. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Keith Packard <keithp@keithp.com> > Cc: Daniel Vetter <daniel.vetter@intel.com> > Cc: David Airlie <airlied@linux.ie> > Cc: intel-gfx@lists.freedesktop.org > Cc: dri-devel@lists.freedesktop.org > Fixes: 849782575325 ("drm/i915: cleanup_plane_fb: also drop reference to current state wait_req") Yeah, I think hanging onto requests like that is perfectly fine, and it simplifies the gymnastics for tracking things if we always allocate drm_*_state resources in atomic_duplicate, update the refs in atomic_check (with the various set functions the core has, or our own) and then release references in atomic_state destroy. Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/i915/intel_atomic_plane.c | 5 ++++- > drivers/gpu/drm/i915/intel_display.c | 6 ------ > 2 files changed, 4 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c > index 7de7721f65bc..f98071a1dcc0 100644 > --- a/drivers/gpu/drm/i915/intel_atomic_plane.c > +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c > @@ -101,7 +101,10 @@ void > intel_plane_destroy_state(struct drm_plane *plane, > struct drm_plane_state *state) > { > - WARN_ON(state && to_intel_plane_state(state)->wait_req); > + struct intel_plane_state *intel_state = to_intel_plane_state(state); > + > + i915_gem_request_assign(&intel_state->wait_req, NULL); > + > drm_atomic_helper_plane_destroy_state(plane, state); > } > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index af45ee206239..774895288bcc 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -14063,17 +14063,11 @@ intel_cleanup_plane_fb(struct drm_plane *plane, > const struct drm_plane_state *old_state) > { > struct drm_device *dev = plane->dev; > - struct intel_plane_state *intel_state = to_intel_plane_state(plane->state); > struct drm_i915_gem_object *old_obj = intel_fb_obj(old_state->fb); > - struct intel_plane_state *old_intel_state = > - to_intel_plane_state(old_state); > > if (old_obj && (plane->type != DRM_PLANE_TYPE_CURSOR || > !INTEL_INFO(dev)->cursor_needs_physical)) > intel_unpin_fb_obj(old_state->fb, old_state->rotation); > - > - i915_gem_request_assign(&intel_state->wait_req, NULL); > - i915_gem_request_assign(&old_intel_state->wait_req, NULL); > } > > int > -- > 2.7.4 > >
diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c index 7de7721f65bc..f98071a1dcc0 100644 --- a/drivers/gpu/drm/i915/intel_atomic_plane.c +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c @@ -101,7 +101,10 @@ void intel_plane_destroy_state(struct drm_plane *plane, struct drm_plane_state *state) { - WARN_ON(state && to_intel_plane_state(state)->wait_req); + struct intel_plane_state *intel_state = to_intel_plane_state(state); + + i915_gem_request_assign(&intel_state->wait_req, NULL); + drm_atomic_helper_plane_destroy_state(plane, state); } diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index af45ee206239..774895288bcc 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -14063,17 +14063,11 @@ intel_cleanup_plane_fb(struct drm_plane *plane, const struct drm_plane_state *old_state) { struct drm_device *dev = plane->dev; - struct intel_plane_state *intel_state = to_intel_plane_state(plane->state); struct drm_i915_gem_object *old_obj = intel_fb_obj(old_state->fb); - struct intel_plane_state *old_intel_state = - to_intel_plane_state(old_state); if (old_obj && (plane->type != DRM_PLANE_TYPE_CURSOR || !INTEL_INFO(dev)->cursor_needs_physical)) intel_unpin_fb_obj(old_state->fb, old_state->rotation); - - i915_gem_request_assign(&intel_state->wait_req, NULL); - i915_gem_request_assign(&old_intel_state->wait_req, NULL); } int
With gem requests reworked to only keep some memory around after it's completed it's now mostly harmless to keep a reference to the request until the state is destroyed. This makes it easy to hang on to the request until the plane state is destroyed since it is just a bunch of memory now. On my 64-bits system a i915_gem_request is 352 bytes. With all 3 crtc's, each 4 planes enabled the total is 4224 bytes, low enough not to optimize this case too much. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Cc: Keith Packard <keithp@keithp.com> Cc: Daniel Vetter <daniel.vetter@intel.com> Cc: David Airlie <airlied@linux.ie> Cc: intel-gfx@lists.freedesktop.org Cc: dri-devel@lists.freedesktop.org Fixes: 849782575325 ("drm/i915: cleanup_plane_fb: also drop reference to current state wait_req") --- drivers/gpu/drm/i915/intel_atomic_plane.c | 5 ++++- drivers/gpu/drm/i915/intel_display.c | 6 ------ 2 files changed, 4 insertions(+), 7 deletions(-)