| Message ID | 1416858786-25376-10-git-send-email-matthew.d.roper@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
-------------------------------------Summary-------------------------------------
Platform Delta drm-intel-nightly Series Applied
PNV -1 367/367 366/367
ILK -5 375/375 370/375
SNB -1 450/450 449/450
IVB -4 503/503 499/503
BYT 289/289 289/289
HSW -3 567/567 564/567
BDW 417/417 417/417
-------------------------------------Detailed-------------------------------------
Platform Test drm-intel-nightly Series Applied
*PNV igt_gen3_mixed_blits PASS(6, M23) CRASH(1, M23)
ILK igt_gem_reset_stats_close-pending-fork-render TIMEOUT(11, M37M26)PASS(1, M26) TIMEOUT(1, M26)
ILK igt_kms_3d DMESG_WARN(1, M26)PASS(3, M37M26) DMESG_WARN(1, M26)
*ILK igt_kms_render_direct-render PASS(4, M37M26) DMESG_WARN(1, M26)
ILK igt_kms_flip_rcs-flip-vs-modeset DMESG_WARN(2, M26)PASS(1, M37) DMESG_WARN(1, M26)
ILK igt_kms_flip_vblank-vs-hang TIMEOUT(11, M37M26)PASS(1, M26) TIMEOUT(1, M26)
*SNB igt_kms_3d PASS(2, M22M35) DMESG_WARN(1, M35)
IVB igt_gem_bad_reloc_negative-reloc NSPT(14, M34M21M4)PASS(1, M21) NSPT(1, M21)
IVB igt_gem_bad_reloc_negative-reloc-lut NSPT(3, M21M34M4)PASS(15, M21M34M4) NSPT(1, M21)
*IVB igt_kms_3d PASS(2, M21) DMESG_WARN(1, M21)
*IVB igt_kms_cursor_crc_cursor-64x64-offscreen PASS(2, M21) DMESG_WARN(1, M21)
HSW igt_gem_bad_reloc_negative-reloc-lut NSPT(24, M40M20M19)PASS(1, M20) NSPT(1, M19)
*HSW igt_kms_rotation_crc_primary-rotation PASS(23, M20M40M19) DMESG_WARN(1, M19)
*HSW igt_pm_rc6_residency_rc6-accuracy PASS(25, M20M40M19) FAIL(1, M19)
Note: You need to pay more attention to line start with '*'
Hi Shuang, The threading is still broken in some MUAs. I believe we also need the References: header in addition to in-reply-to: to solve that. HTH,
No problem, I will add it Thanks --Shuang > -----Original Message----- > From: Lespiau, Damien > Sent: Thursday, November 27, 2014 11:58 PM > To: He, Shuang > Cc: intel-gfx@lists.freedesktop.org > Subject: Re: [Intel-gfx] [PATCH 9/9] drm/i915: Make all plane disables use > > Hi Shuang, > > The threading is still broken in some MUAs. I believe we also need the > References: header in addition to in-reply-to: to solve that. > > HTH, > > -- > Damien > > On Mon, Nov 24, 2014 at 11:39:48PM -0800, shuang.he@intel.com wrote: > > Tested-By: PRC QA PRTS (Patch Regression Test System Contact: > shuang.he@intel.com) > > -------------------------------------Summary------------------------------------- > > Platform Delta drm-intel-nightly Series > Applied > > PNV -1 367/367 > 366/367 > > ILK -5 375/375 370/375 > > SNB -1 450/450 > 449/450 > > IVB -4 503/503 > 499/503 > > BYT 289/289 > 289/289 > > HSW -3 567/567 > 564/567 > > BDW 417/417 > 417/417 > > -------------------------------------Detailed------------------------------------- > > Platform Test drm-intel-nightly > Series Applied > > *PNV igt_gen3_mixed_blits PASS(6, M23) CRASH(1, M23) > > ILK igt_gem_reset_stats_close-pending-fork-render TIMEOUT(11, > M37M26)PASS(1, M26) TIMEOUT(1, M26) > > ILK igt_kms_3d DMESG_WARN(1, M26)PASS(3, M37M26) > DMESG_WARN(1, M26) > > *ILK igt_kms_render_direct-render PASS(4, M37M26) > DMESG_WARN(1, M26) > > ILK igt_kms_flip_rcs-flip-vs-modeset DMESG_WARN(2, M26)PASS(1, > M37) DMESG_WARN(1, M26) > > ILK igt_kms_flip_vblank-vs-hang TIMEOUT(11, M37M26)PASS(1, > M26) TIMEOUT(1, M26) > > *SNB igt_kms_3d PASS(2, M22M35) DMESG_WARN(1, M35) > > IVB igt_gem_bad_reloc_negative-reloc NSPT(14, > M34M21M4)PASS(1, M21) NSPT(1, M21) > > IVB igt_gem_bad_reloc_negative-reloc-lut NSPT(3, > M21M34M4)PASS(15, M21M34M4) NSPT(1, M21) > > *IVB igt_kms_3d PASS(2, M21) DMESG_WARN(1, M21) > > *IVB igt_kms_cursor_crc_cursor-64x64-offscreen PASS(2, M21) > DMESG_WARN(1, M21) > > HSW igt_gem_bad_reloc_negative-reloc-lut NSPT(24, > M40M20M19)PASS(1, M20) NSPT(1, M19) > > *HSW igt_kms_rotation_crc_primary-rotation PASS(23, > M20M40M19) DMESG_WARN(1, M19) > > *HSW igt_pm_rc6_residency_rc6-accuracy PASS(25, M20M40M19) > FAIL(1, M19) > > Note: You need to pay more attention to line start with '*' > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Hi, So References: is now in the new replies, but that's still not enough to make threading work in gmail. Maybe truncating the subject is the cause then? Headers can be multi-lines and you're only taking the first line. For instance: Subject: [Intel-gfx] [PATCH] drm/i915: Disable FBC if primary plane is rotated on Haswell The Subject header carries over a second line, while your script only takes the first. Can we try to fix that and see if it then threads properly? Thanks,
On Fri, 28 Nov 2014, Damien Lespiau <damien.lespiau@intel.com> wrote: > Hi, > > So References: is now in the new replies, but that's still not enough to > make threading work in gmail. > > Maybe truncating the subject is the cause then? Headers can be > multi-lines and you're only taking the first line. For instance: > > Subject: [Intel-gfx] [PATCH] drm/i915: Disable FBC if primary plane is > rotated on Haswell > > The Subject header carries over a second line, while your script only > takes the first. Can we try to fix that and see if it then threads > properly? Arguably gmail is broken in this sense, but the subject changing is the likely culprit here. http://mid.gmane.org/87fvd83ooi.fsf@intel.com BR, Jani.
On 11/24/2014 09:53 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. > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 109 +++++++++++++++-------------------- > drivers/gpu/drm/i915/intel_drv.h | 2 +- > drivers/gpu/drm/i915/intel_sprite.c | 71 ++++++----------------- > 3 files changed, 67 insertions(+), 115 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index dd10dc6..fd1eaf5 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -3996,7 +3996,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); > } > } > > @@ -11535,45 +11535,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) > - goto disable_unpin; > - > - intel_crtc_wait_for_pending_flips(plane->crtc); > - intel_disable_primary_hw_plane(plane, plane->crtc); > - > -disable_unpin: > - 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 > @@ -11678,10 +11639,12 @@ intel_check_primary_plane(struct drm_plane *plane, > if (ret) > return ret; > > - intel_crtc_wait_for_pending_flips(crtc); > - if (intel_crtc_has_pending_flip(crtc)) { > - DRM_ERROR("pipe is still busy with an old pageflip\n"); > - return -EBUSY; > + if (plane->crtc) { > + intel_crtc_wait_for_pending_flips(plane->crtc); > + if (intel_crtc_has_pending_flip(plane->crtc)) { > + DRM_ERROR("pipe is still busy with an old pageflip\n"); > + return -EBUSY; > + } > } > > return 0; > @@ -11696,12 +11659,30 @@ 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); > + > + /* > + * crtc may be passed as NULL when disabling, so set it to > + * the proper value now > + */ > + crtc = plane->crtc; > + intel_crtc = to_intel_crtc(crtc); > + } > + > + plane->fb = fb; > crtc->x = src->x1 >> 16; > crtc->y = src->y1 >> 16; > > @@ -11759,7 +11740,7 @@ intel_commit_primary_plane(struct drm_plane *plane, > * explicitly disables the plane by passing fb=0 > * because plane->fb still gets set and pinned. > */ > - intel_disable_primary_hw_plane(plane, crtc); > + intel_disable_primary_hw_plane(plane, plane->crtc); I can't see why this hunk is necessary, since you set crtc = plane->crtc above for the !fb case. Or am I missing something? > } > > intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_PRIMARY(pipe)); > @@ -11831,6 +11812,24 @@ 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; > + > + BUG_ON(!plane->crtc); WARN_ON? Maybe not since intel_update_plane will oops. I'm not sure what's the guideline here. Anyway, the series look pretty good overall. Cheers, Ander > + > + 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) > { > @@ -11841,7 +11840,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 > }; > @@ -11896,18 +11895,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) > { > @@ -12030,7 +12017,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 5f7a071..da4da2c 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -1021,6 +1021,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 Mon, Dec 01, 2014 at 04:04:18PM +0200, Ander Conselvan de Oliveira wrote: > On 11/24/2014 09:53 PM, Matt Roper wrote: > >+/** > >+ * 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; > >+ > >+ BUG_ON(!plane->crtc); > > WARN_ON? Maybe not since intel_update_plane will oops. I'm not sure what's > the guideline here. I'd go with a if (WARN_ON) return -EINVAL; or so. We've had a bunch of fun cases when we've done the original hw state readout code where code fizzled out on an Oops because the fb wasn't reconstructed. And doing that at boot-up while holding console_lock is really bad for debugging. But yeah in general keeping the BUG_ON if you'll oops otherwise anyway is the correct thing to do. -Daniel
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index dd10dc6..fd1eaf5 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -3996,7 +3996,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); } } @@ -11535,45 +11535,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) - goto disable_unpin; - - intel_crtc_wait_for_pending_flips(plane->crtc); - intel_disable_primary_hw_plane(plane, plane->crtc); - -disable_unpin: - 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 @@ -11678,10 +11639,12 @@ intel_check_primary_plane(struct drm_plane *plane, if (ret) return ret; - intel_crtc_wait_for_pending_flips(crtc); - if (intel_crtc_has_pending_flip(crtc)) { - DRM_ERROR("pipe is still busy with an old pageflip\n"); - return -EBUSY; + if (plane->crtc) { + intel_crtc_wait_for_pending_flips(plane->crtc); + if (intel_crtc_has_pending_flip(plane->crtc)) { + DRM_ERROR("pipe is still busy with an old pageflip\n"); + return -EBUSY; + } } return 0; @@ -11696,12 +11659,30 @@ 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); + + /* + * crtc may be passed as NULL when disabling, so set it to + * the proper value now + */ + crtc = plane->crtc; + intel_crtc = to_intel_crtc(crtc); + } + + plane->fb = fb; crtc->x = src->x1 >> 16; crtc->y = src->y1 >> 16; @@ -11759,7 +11740,7 @@ intel_commit_primary_plane(struct drm_plane *plane, * explicitly disables the plane by passing fb=0 * because plane->fb still gets set and pinned. */ - intel_disable_primary_hw_plane(plane, crtc); + intel_disable_primary_hw_plane(plane, plane->crtc); } intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_PRIMARY(pipe)); @@ -11831,6 +11812,24 @@ 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; + + BUG_ON(!plane->crtc); + + 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) { @@ -11841,7 +11840,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 }; @@ -11896,18 +11895,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) { @@ -12030,7 +12017,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 5f7a071..da4da2c 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1021,6 +1021,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,
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. Signed-off-by: Matt Roper <matthew.d.roper@intel.com> --- drivers/gpu/drm/i915/intel_display.c | 109 +++++++++++++++-------------------- drivers/gpu/drm/i915/intel_drv.h | 2 +- drivers/gpu/drm/i915/intel_sprite.c | 71 ++++++----------------- 3 files changed, 67 insertions(+), 115 deletions(-)