| Message ID | 1417717662-12822-1-git-send-email-matthew.d.roper@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Reviewed-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> On 12/04/2014 08:27 PM, Matt Roper wrote: > If we extend the commit_plane handlers for each plane type to be able to > handle fb=0, then we can easily implement plane disable via the > update_plane handler. The cursor plane already works this way, and this > is the direction we need to go to integrate with the atomic plane > handler. We can now kill off the type-specific disable functions, as > well as the redundant intel_plane_disable() (not to be confused with > intel_disable_plane()). > > Note that prepare_plane_fb() only gets called as part of update_plane > when fb!=NULL (by design, to match the semantics of the atomic plane > helpers); this means that our commit_plane handlers need to handle the > frontbuffer tracking for the disable case, even though they don't handle > it for normal updates. > > v2: > - Change BUG_ON to WARN_ON (Ander/Daniel) > > v3: > - Drop unnecessary plane->crtc check since a previous patch to plane > update ensures that plane->crtc will always be non-NULL, even for > disable calls that might pass NULL from userspace. (Ander) > - Drop a s/crtc/plane->crtc/ hunk that was unnecessary. (Ander) > > v4: > - Fix missing whitespace (Ander) > > v5: > - Use state's crtc rather than plane's crtc in > intel_check_primary_plane(). plane->crtc could be NULL, but we've > already fixed up state->crtc to ensure it's non-NULL (even if > userspace passed it as NULL during a disable call). (Ander) > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > Reviewed-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 89 ++++++++++++++---------------------- > drivers/gpu/drm/i915/intel_drv.h | 2 +- > drivers/gpu/drm/i915/intel_sprite.c | 71 ++++++++-------------------- > 3 files changed, 54 insertions(+), 108 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index d13015c..7e53994 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -4060,7 +4060,7 @@ static void intel_disable_planes(struct drm_crtc *crtc) > drm_for_each_legacy_plane(plane, &dev->mode_config.plane_list) { > intel_plane = to_intel_plane(plane); > if (intel_plane->pipe == pipe) > - intel_plane_disable(&intel_plane->base); > + plane->funcs->disable_plane(plane); > } > } > > @@ -11605,43 +11605,6 @@ static void intel_shared_dpll_init(struct drm_device *dev) > BUG_ON(dev_priv->num_shared_dpll > I915_NUM_PLLS); > } > > -static int > -intel_primary_plane_disable(struct drm_plane *plane) > -{ > - struct drm_device *dev = plane->dev; > - struct intel_crtc *intel_crtc; > - > - if (!plane->fb) > - return 0; > - > - BUG_ON(!plane->crtc); > - > - intel_crtc = to_intel_crtc(plane->crtc); > - > - /* > - * Even though we checked plane->fb above, it's still possible that > - * the primary plane has been implicitly disabled because the crtc > - * coordinates given weren't visible, or because we detected > - * that it was 100% covered by a sprite plane. Or, the CRTC may be > - * off and we've set a fb, but haven't actually turned on the CRTC yet. > - * In either case, we need to unpin the FB and let the fb pointer get > - * updated, but otherwise we don't need to touch the hardware. > - */ > - if (intel_crtc->primary_enabled) { > - intel_crtc_wait_for_pending_flips(plane->crtc); > - intel_disable_primary_hw_plane(plane, plane->crtc); > - } > - > - mutex_lock(&dev->struct_mutex); > - i915_gem_track_fb(intel_fb_obj(plane->fb), NULL, > - INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe)); > - intel_unpin_fb_obj(intel_fb_obj(plane->fb)); > - mutex_unlock(&dev->struct_mutex); > - plane->fb = NULL; > - > - return 0; > -} > - > /** > * intel_prepare_plane_fb - Prepare fb for usage on plane > * @plane: drm plane to prepare for > @@ -11763,12 +11726,23 @@ intel_commit_primary_plane(struct drm_plane *plane, > struct drm_device *dev = plane->dev; > struct drm_i915_private *dev_priv = dev->dev_private; > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > - enum pipe pipe = intel_crtc->pipe; > struct drm_i915_gem_object *obj = intel_fb_obj(fb); > struct intel_plane *intel_plane = to_intel_plane(plane); > struct drm_rect *src = &state->src; > + enum pipe pipe = intel_plane->pipe; > > - crtc->primary->fb = fb; > + if (!fb) { > + /* > + * 'prepare' is never called when plane is being disabled, so > + * we need to handle frontbuffer tracking here > + */ > + mutex_lock(&dev->struct_mutex); > + i915_gem_track_fb(intel_fb_obj(plane->fb), NULL, > + INTEL_FRONTBUFFER_PRIMARY(pipe)); > + mutex_unlock(&dev->struct_mutex); > + } > + > + plane->fb = fb; > crtc->x = src->x1 >> 16; > crtc->y = src->y1 >> 16; > > @@ -11897,6 +11871,25 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, > return 0; > } > > +/** > + * intel_disable_plane - disable a plane > + * @plane: plane to disable > + * > + * General disable handler for all plane types. > + */ > +int > +intel_disable_plane(struct drm_plane *plane) > +{ > + if (!plane->fb) > + return 0; > + > + if (WARN_ON(!plane->crtc)) > + return -EINVAL; > + > + return plane->funcs->update_plane(plane, plane->crtc, NULL, > + 0, 0, 0, 0, 0, 0, 0, 0); > +} > + > /* Common destruction function for both primary and cursor planes */ > static void intel_plane_destroy(struct drm_plane *plane) > { > @@ -11907,7 +11900,7 @@ static void intel_plane_destroy(struct drm_plane *plane) > > static const struct drm_plane_funcs intel_primary_plane_funcs = { > .update_plane = intel_update_plane, > - .disable_plane = intel_primary_plane_disable, > + .disable_plane = intel_disable_plane, > .destroy = intel_plane_destroy, > .set_property = intel_plane_set_property > }; > @@ -11962,18 +11955,6 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev, > } > > static int > -intel_cursor_plane_disable(struct drm_plane *plane) > -{ > - if (!plane->fb) > - return 0; > - > - BUG_ON(!plane->crtc); > - > - return plane->funcs->update_plane(plane, plane->crtc, NULL, > - 0, 0, 0, 0, 0, 0, 0, 0); > -} > - > -static int > intel_check_cursor_plane(struct drm_plane *plane, > struct intel_plane_state *state) > { > @@ -12097,7 +12078,7 @@ update: > > static const struct drm_plane_funcs intel_cursor_plane_funcs = { > .update_plane = intel_update_plane, > - .disable_plane = intel_cursor_plane_disable, > + .disable_plane = intel_disable_plane, > .destroy = intel_plane_destroy, > .set_property = intel_plane_set_property, > }; > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index f7b6619..53b696e 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -1020,6 +1020,7 @@ int intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, > unsigned int crtc_w, unsigned int crtc_h, > uint32_t src_x, uint32_t src_y, > uint32_t src_w, uint32_t src_h); > +int intel_disable_plane(struct drm_plane *plane); > > /* intel_dp_mst.c */ > int intel_dp_mst_encoder_init(struct intel_digital_port *intel_dig_port, int conn_id); > @@ -1201,7 +1202,6 @@ int intel_plane_set_property(struct drm_plane *plane, > struct drm_property *prop, > uint64_t val); > int intel_plane_restore(struct drm_plane *plane); > -void intel_plane_disable(struct drm_plane *plane); > int intel_sprite_set_colorkey(struct drm_device *dev, void *data, > struct drm_file *file_priv); > int intel_sprite_get_colorkey(struct drm_device *dev, void *data, > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c > index bfd5270..bc5834b 100644 > --- a/drivers/gpu/drm/i915/intel_sprite.c > +++ b/drivers/gpu/drm/i915/intel_sprite.c > @@ -1109,7 +1109,12 @@ intel_check_sprite_plane(struct drm_plane *plane, > const struct drm_rect *clip = &state->clip; > int hscale, vscale; > int max_scale, min_scale; > - int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0); > + int pixel_size; > + > + if (!fb) { > + state->visible = false; > + return 0; > + } > > /* Don't modify another pipe's plane */ > if (intel_plane->pipe != intel_crtc->pipe) { > @@ -1232,6 +1237,7 @@ intel_check_sprite_plane(struct drm_plane *plane, > if (src_w < 3 || src_h < 3) > state->visible = false; > > + pixel_size = drm_format_plane_cpp(fb->pixel_format, 0); > width_bytes = ((src_x * pixel_size) & 63) + > src_w * pixel_size; > > @@ -1276,6 +1282,17 @@ intel_commit_sprite_plane(struct drm_plane *plane, > bool primary_enabled; > > /* > + * 'prepare' is never called when plane is being disabled, so we need > + * to handle frontbuffer tracking here > + */ > + if (!fb) { > + mutex_lock(&dev->struct_mutex); > + i915_gem_track_fb(intel_fb_obj(plane->fb), NULL, > + INTEL_FRONTBUFFER_SPRITE(pipe)); > + mutex_unlock(&dev->struct_mutex); > + } > + > + /* > * If the sprite is completely covering the primary plane, > * we can disable the primary and save power. > */ > @@ -1327,50 +1344,6 @@ intel_commit_sprite_plane(struct drm_plane *plane, > } > } > > -static int > -intel_disable_plane(struct drm_plane *plane) > -{ > - struct drm_device *dev = plane->dev; > - struct intel_plane *intel_plane = to_intel_plane(plane); > - struct intel_crtc *intel_crtc; > - enum pipe pipe; > - > - if (!plane->fb) > - return 0; > - > - if (WARN_ON(!plane->crtc)) > - return -EINVAL; > - > - intel_crtc = to_intel_crtc(plane->crtc); > - pipe = intel_crtc->pipe; > - > - if (intel_crtc->active) { > - bool primary_was_enabled = intel_crtc->primary_enabled; > - > - intel_crtc->primary_enabled = true; > - > - intel_plane->disable_plane(plane, plane->crtc); > - > - if (!primary_was_enabled && intel_crtc->primary_enabled) > - intel_post_enable_primary(plane->crtc); > - } > - > - if (intel_plane->obj) { > - if (intel_crtc->active) > - intel_wait_for_vblank(dev, intel_plane->pipe); > - > - mutex_lock(&dev->struct_mutex); > - intel_unpin_fb_obj(intel_plane->obj); > - i915_gem_track_fb(intel_plane->obj, NULL, > - INTEL_FRONTBUFFER_SPRITE(pipe)); > - mutex_unlock(&dev->struct_mutex); > - > - intel_plane->obj = NULL; > - } > - > - return 0; > -} > - > static void intel_destroy_plane(struct drm_plane *plane) > { > struct intel_plane *intel_plane = to_intel_plane(plane); > @@ -1478,14 +1451,6 @@ int intel_plane_restore(struct drm_plane *plane) > intel_plane->src_w, intel_plane->src_h); > } > > -void intel_plane_disable(struct drm_plane *plane) > -{ > - if (!plane->crtc || !plane->fb) > - return; > - > - intel_disable_plane(plane); > -} > - > static const struct drm_plane_funcs intel_plane_funcs = { > .update_plane = intel_update_plane, > .disable_plane = intel_disable_plane, >
On Fri, Dec 05, 2014 at 10:15:07AM +0200, Ander Conselvan de Oliveira wrote: > Reviewed-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> > > On 12/04/2014 08:27 PM, Matt Roper wrote: > > If we extend the commit_plane handlers for each plane type to be able to > > handle fb=0, then we can easily implement plane disable via the > > update_plane handler. The cursor plane already works this way, and this > > is the direction we need to go to integrate with the atomic plane > > handler. We can now kill off the type-specific disable functions, as > > well as the redundant intel_plane_disable() (not to be confused with > > intel_disable_plane()). > > > > Note that prepare_plane_fb() only gets called as part of update_plane > > when fb!=NULL (by design, to match the semantics of the atomic plane > > helpers); this means that our commit_plane handlers need to handle the > > frontbuffer tracking for the disable case, even though they don't handle > > it for normal updates. > > > > v2: > > - Change BUG_ON to WARN_ON (Ander/Daniel) > > > > v3: > > - Drop unnecessary plane->crtc check since a previous patch to plane > > update ensures that plane->crtc will always be non-NULL, even for > > disable calls that might pass NULL from userspace. (Ander) > > - Drop a s/crtc/plane->crtc/ hunk that was unnecessary. (Ander) > > > > v4: > > - Fix missing whitespace (Ander) > > > > v5: > > - Use state's crtc rather than plane's crtc in > > intel_check_primary_plane(). plane->crtc could be NULL, but we've > > already fixed up state->crtc to ensure it's non-NULL (even if > > userspace passed it as NULL during a disable call). (Ander) > > > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > > Reviewed-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> Ok, merged all 10 patches to dinq, thanks. There was a minor conflict in patch 8 due to the slight changes in patch 7, but nothing big really. -Daniel
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index d13015c..7e53994 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -4060,7 +4060,7 @@ static void intel_disable_planes(struct drm_crtc *crtc) drm_for_each_legacy_plane(plane, &dev->mode_config.plane_list) { intel_plane = to_intel_plane(plane); if (intel_plane->pipe == pipe) - intel_plane_disable(&intel_plane->base); + plane->funcs->disable_plane(plane); } } @@ -11605,43 +11605,6 @@ static void intel_shared_dpll_init(struct drm_device *dev) BUG_ON(dev_priv->num_shared_dpll > I915_NUM_PLLS); } -static int -intel_primary_plane_disable(struct drm_plane *plane) -{ - struct drm_device *dev = plane->dev; - struct intel_crtc *intel_crtc; - - if (!plane->fb) - return 0; - - BUG_ON(!plane->crtc); - - intel_crtc = to_intel_crtc(plane->crtc); - - /* - * Even though we checked plane->fb above, it's still possible that - * the primary plane has been implicitly disabled because the crtc - * coordinates given weren't visible, or because we detected - * that it was 100% covered by a sprite plane. Or, the CRTC may be - * off and we've set a fb, but haven't actually turned on the CRTC yet. - * In either case, we need to unpin the FB and let the fb pointer get - * updated, but otherwise we don't need to touch the hardware. - */ - if (intel_crtc->primary_enabled) { - intel_crtc_wait_for_pending_flips(plane->crtc); - intel_disable_primary_hw_plane(plane, plane->crtc); - } - - mutex_lock(&dev->struct_mutex); - i915_gem_track_fb(intel_fb_obj(plane->fb), NULL, - INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe)); - intel_unpin_fb_obj(intel_fb_obj(plane->fb)); - mutex_unlock(&dev->struct_mutex); - plane->fb = NULL; - - return 0; -} - /** * intel_prepare_plane_fb - Prepare fb for usage on plane * @plane: drm plane to prepare for @@ -11763,12 +11726,23 @@ intel_commit_primary_plane(struct drm_plane *plane, struct drm_device *dev = plane->dev; struct drm_i915_private *dev_priv = dev->dev_private; struct intel_crtc *intel_crtc = to_intel_crtc(crtc); - enum pipe pipe = intel_crtc->pipe; struct drm_i915_gem_object *obj = intel_fb_obj(fb); struct intel_plane *intel_plane = to_intel_plane(plane); struct drm_rect *src = &state->src; + enum pipe pipe = intel_plane->pipe; - crtc->primary->fb = fb; + if (!fb) { + /* + * 'prepare' is never called when plane is being disabled, so + * we need to handle frontbuffer tracking here + */ + mutex_lock(&dev->struct_mutex); + i915_gem_track_fb(intel_fb_obj(plane->fb), NULL, + INTEL_FRONTBUFFER_PRIMARY(pipe)); + mutex_unlock(&dev->struct_mutex); + } + + plane->fb = fb; crtc->x = src->x1 >> 16; crtc->y = src->y1 >> 16; @@ -11897,6 +11871,25 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, return 0; } +/** + * intel_disable_plane - disable a plane + * @plane: plane to disable + * + * General disable handler for all plane types. + */ +int +intel_disable_plane(struct drm_plane *plane) +{ + if (!plane->fb) + return 0; + + if (WARN_ON(!plane->crtc)) + return -EINVAL; + + return plane->funcs->update_plane(plane, plane->crtc, NULL, + 0, 0, 0, 0, 0, 0, 0, 0); +} + /* Common destruction function for both primary and cursor planes */ static void intel_plane_destroy(struct drm_plane *plane) { @@ -11907,7 +11900,7 @@ static void intel_plane_destroy(struct drm_plane *plane) static const struct drm_plane_funcs intel_primary_plane_funcs = { .update_plane = intel_update_plane, - .disable_plane = intel_primary_plane_disable, + .disable_plane = intel_disable_plane, .destroy = intel_plane_destroy, .set_property = intel_plane_set_property }; @@ -11962,18 +11955,6 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev, } static int -intel_cursor_plane_disable(struct drm_plane *plane) -{ - if (!plane->fb) - return 0; - - BUG_ON(!plane->crtc); - - return plane->funcs->update_plane(plane, plane->crtc, NULL, - 0, 0, 0, 0, 0, 0, 0, 0); -} - -static int intel_check_cursor_plane(struct drm_plane *plane, struct intel_plane_state *state) { @@ -12097,7 +12078,7 @@ update: static const struct drm_plane_funcs intel_cursor_plane_funcs = { .update_plane = intel_update_plane, - .disable_plane = intel_cursor_plane_disable, + .disable_plane = intel_disable_plane, .destroy = intel_plane_destroy, .set_property = intel_plane_set_property, }; diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index f7b6619..53b696e 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1020,6 +1020,7 @@ int intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, unsigned int crtc_w, unsigned int crtc_h, uint32_t src_x, uint32_t src_y, uint32_t src_w, uint32_t src_h); +int intel_disable_plane(struct drm_plane *plane); /* intel_dp_mst.c */ int intel_dp_mst_encoder_init(struct intel_digital_port *intel_dig_port, int conn_id); @@ -1201,7 +1202,6 @@ int intel_plane_set_property(struct drm_plane *plane, struct drm_property *prop, uint64_t val); int intel_plane_restore(struct drm_plane *plane); -void intel_plane_disable(struct drm_plane *plane); int intel_sprite_set_colorkey(struct drm_device *dev, void *data, struct drm_file *file_priv); int intel_sprite_get_colorkey(struct drm_device *dev, void *data, diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c index bfd5270..bc5834b 100644 --- a/drivers/gpu/drm/i915/intel_sprite.c +++ b/drivers/gpu/drm/i915/intel_sprite.c @@ -1109,7 +1109,12 @@ intel_check_sprite_plane(struct drm_plane *plane, const struct drm_rect *clip = &state->clip; int hscale, vscale; int max_scale, min_scale; - int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0); + int pixel_size; + + if (!fb) { + state->visible = false; + return 0; + } /* Don't modify another pipe's plane */ if (intel_plane->pipe != intel_crtc->pipe) { @@ -1232,6 +1237,7 @@ intel_check_sprite_plane(struct drm_plane *plane, if (src_w < 3 || src_h < 3) state->visible = false; + pixel_size = drm_format_plane_cpp(fb->pixel_format, 0); width_bytes = ((src_x * pixel_size) & 63) + src_w * pixel_size; @@ -1276,6 +1282,17 @@ intel_commit_sprite_plane(struct drm_plane *plane, bool primary_enabled; /* + * 'prepare' is never called when plane is being disabled, so we need + * to handle frontbuffer tracking here + */ + if (!fb) { + mutex_lock(&dev->struct_mutex); + i915_gem_track_fb(intel_fb_obj(plane->fb), NULL, + INTEL_FRONTBUFFER_SPRITE(pipe)); + mutex_unlock(&dev->struct_mutex); + } + + /* * If the sprite is completely covering the primary plane, * we can disable the primary and save power. */ @@ -1327,50 +1344,6 @@ intel_commit_sprite_plane(struct drm_plane *plane, } } -static int -intel_disable_plane(struct drm_plane *plane) -{ - struct drm_device *dev = plane->dev; - struct intel_plane *intel_plane = to_intel_plane(plane); - struct intel_crtc *intel_crtc; - enum pipe pipe; - - if (!plane->fb) - return 0; - - if (WARN_ON(!plane->crtc)) - return -EINVAL; - - intel_crtc = to_intel_crtc(plane->crtc); - pipe = intel_crtc->pipe; - - if (intel_crtc->active) { - bool primary_was_enabled = intel_crtc->primary_enabled; - - intel_crtc->primary_enabled = true; - - intel_plane->disable_plane(plane, plane->crtc); - - if (!primary_was_enabled && intel_crtc->primary_enabled) - intel_post_enable_primary(plane->crtc); - } - - if (intel_plane->obj) { - if (intel_crtc->active) - intel_wait_for_vblank(dev, intel_plane->pipe); - - mutex_lock(&dev->struct_mutex); - intel_unpin_fb_obj(intel_plane->obj); - i915_gem_track_fb(intel_plane->obj, NULL, - INTEL_FRONTBUFFER_SPRITE(pipe)); - mutex_unlock(&dev->struct_mutex); - - intel_plane->obj = NULL; - } - - return 0; -} - static void intel_destroy_plane(struct drm_plane *plane) { struct intel_plane *intel_plane = to_intel_plane(plane); @@ -1478,14 +1451,6 @@ int intel_plane_restore(struct drm_plane *plane) intel_plane->src_w, intel_plane->src_h); } -void intel_plane_disable(struct drm_plane *plane) -{ - if (!plane->crtc || !plane->fb) - return; - - intel_disable_plane(plane); -} - static const struct drm_plane_funcs intel_plane_funcs = { .update_plane = intel_update_plane, .disable_plane = intel_disable_plane,