| Message ID | 20171220093545.613-4-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Dec 20, 2017 at 10:35:45AM +0100, Maarten Lankhorst wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > We don't need any active planes during load detection, so just disable > them all. This saves us from having to come up with a suitable > framebuffer. And we also avoid leaving sprite/cursor planes on and > potentially presenting them at a peculiar location during the load > detection. > > Changes since v1 (Maarten): > - Add missing call to add_all_affected_planes. > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102707 Less code, I like. And I think we have enough load-detect machines (+ plus the nasty igt to do it anywhere we still have native vga) to have reasonable ensurance it actually works. But maybe let's soak this in -next first for a while, then cherry-pick over after a few weeks once it's solid. With the missing Fixes: line added. Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/i915/intel_display.c | 147 +++++------------------------------ > 1 file changed, 18 insertions(+), 129 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 7e833268c9df..ef61f9a75c93 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -9689,111 +9689,27 @@ intel_framebuffer_create(struct drm_i915_gem_object *obj, > return ERR_PTR(ret); > } > > -static u32 > -intel_framebuffer_pitch_for_width(int width, int bpp) > -{ > - u32 pitch = DIV_ROUND_UP(width * bpp, 8); > - return ALIGN(pitch, 64); > -} > - > -static u32 > -intel_framebuffer_size_for_mode(const struct drm_display_mode *mode, int bpp) > -{ > - u32 pitch = intel_framebuffer_pitch_for_width(mode->hdisplay, bpp); > - return PAGE_ALIGN(pitch * mode->vdisplay); > -} > - > -static struct drm_framebuffer * > -intel_framebuffer_create_for_mode(struct drm_device *dev, > - const struct drm_display_mode *mode, > - int depth, int bpp) > -{ > - struct drm_framebuffer *fb; > - struct drm_i915_gem_object *obj; > - struct drm_mode_fb_cmd2 mode_cmd = { 0 }; > - > - obj = i915_gem_object_create(to_i915(dev), > - intel_framebuffer_size_for_mode(mode, bpp)); > - if (IS_ERR(obj)) > - return ERR_CAST(obj); > - > - mode_cmd.width = mode->hdisplay; > - mode_cmd.height = mode->vdisplay; > - mode_cmd.pitches[0] = intel_framebuffer_pitch_for_width(mode_cmd.width, > - bpp); > - mode_cmd.pixel_format = drm_mode_legacy_fb_format(bpp, depth); > - > - fb = intel_framebuffer_create(obj, &mode_cmd); > - if (IS_ERR(fb)) > - i915_gem_object_put(obj); > - > - return fb; > -} > - > -static struct drm_framebuffer * > -mode_fits_in_fbdev(struct drm_device *dev, > - const struct drm_display_mode *mode) > -{ > -#ifdef CONFIG_DRM_FBDEV_EMULATION > - struct drm_i915_private *dev_priv = to_i915(dev); > - struct drm_i915_gem_object *obj; > - struct drm_framebuffer *fb; > - > - if (!dev_priv->fbdev) > - return NULL; > - > - if (!dev_priv->fbdev->fb) > - return NULL; > - > - obj = dev_priv->fbdev->fb->obj; > - BUG_ON(!obj); > - > - fb = &dev_priv->fbdev->fb->base; > - if (fb->pitches[0] < intel_framebuffer_pitch_for_width(mode->hdisplay, > - fb->format->cpp[0] * 8)) > - return NULL; > - > - if (obj->base.size < mode->vdisplay * fb->pitches[0]) > - return NULL; > - > - drm_framebuffer_get(fb); > - return fb; > -#else > - return NULL; > -#endif > -} > - > -static int intel_modeset_setup_plane_state(struct drm_atomic_state *state, > - struct drm_crtc *crtc, > - const struct drm_display_mode *mode, > - struct drm_framebuffer *fb, > - int x, int y) > +static int intel_modeset_disable_planes(struct drm_atomic_state *state, > + struct drm_crtc *crtc) > { > + struct drm_plane *plane; > struct drm_plane_state *plane_state; > - int hdisplay, vdisplay; > - int ret; > - > - plane_state = drm_atomic_get_plane_state(state, crtc->primary); > - if (IS_ERR(plane_state)) > - return PTR_ERR(plane_state); > - > - if (mode) > - drm_mode_get_hv_timing(mode, &hdisplay, &vdisplay); > - else > - hdisplay = vdisplay = 0; > + int ret, i; > > - ret = drm_atomic_set_crtc_for_plane(plane_state, fb ? crtc : NULL); > + ret = drm_atomic_add_affected_planes(state, crtc); > if (ret) > return ret; > - drm_atomic_set_fb_for_plane(plane_state, fb); > - plane_state->crtc_x = 0; > - plane_state->crtc_y = 0; > - plane_state->crtc_w = hdisplay; > - plane_state->crtc_h = vdisplay; > - plane_state->src_x = x << 16; > - plane_state->src_y = y << 16; > - plane_state->src_w = hdisplay << 16; > - plane_state->src_h = vdisplay << 16; > + > + for_each_new_plane_in_state(state, plane, plane_state, i) { > + if (plane_state->crtc != crtc) > + continue; > + > + ret = drm_atomic_set_crtc_for_plane(plane_state, NULL); > + if (ret) > + return ret; > + > + drm_atomic_set_fb_for_plane(plane_state, NULL); > + } > > return 0; > } > @@ -9811,7 +9727,6 @@ int intel_get_load_detect_pipe(struct drm_connector *connector, > struct drm_crtc *crtc = NULL; > struct drm_device *dev = encoder->dev; > struct drm_i915_private *dev_priv = to_i915(dev); > - struct drm_framebuffer *fb; > struct drm_mode_config *config = &dev->mode_config; > struct drm_atomic_state *state = NULL, *restore_state = NULL; > struct drm_connector_state *connector_state; > @@ -9879,10 +9794,6 @@ int intel_get_load_detect_pipe(struct drm_connector *connector, > found: > intel_crtc = to_intel_crtc(crtc); > > - ret = drm_modeset_lock(&crtc->primary->mutex, ctx); > - if (ret) > - goto fail; > - > state = drm_atomic_state_alloc(dev); > restore_state = drm_atomic_state_alloc(dev); > if (!state || !restore_state) { > @@ -9914,39 +9825,17 @@ int intel_get_load_detect_pipe(struct drm_connector *connector, > if (!mode) > mode = &load_detect_mode; > > - /* We need a framebuffer large enough to accommodate all accesses > - * that the plane may generate whilst we perform load detection. > - * We can not rely on the fbcon either being present (we get called > - * during its initialisation to detect all boot displays, or it may > - * not even exist) or that it is large enough to satisfy the > - * requested mode. > - */ > - fb = mode_fits_in_fbdev(dev, mode); > - if (fb == NULL) { > - DRM_DEBUG_KMS("creating tmp fb for load-detection\n"); > - fb = intel_framebuffer_create_for_mode(dev, mode, 24, 32); > - } else > - DRM_DEBUG_KMS("reusing fbdev for load-detection framebuffer\n"); > - if (IS_ERR(fb)) { > - DRM_DEBUG_KMS("failed to allocate framebuffer for load-detection\n"); > - ret = PTR_ERR(fb); > - goto fail; > - } > - > - ret = intel_modeset_setup_plane_state(state, crtc, mode, fb, 0, 0); > - drm_framebuffer_put(fb); > + ret = drm_atomic_set_mode_for_crtc(&crtc_state->base, mode); > if (ret) > goto fail; > > - ret = drm_atomic_set_mode_for_crtc(&crtc_state->base, mode); > + ret = intel_modeset_disable_planes(state, crtc); > if (ret) > goto fail; > > ret = PTR_ERR_OR_ZERO(drm_atomic_get_connector_state(restore_state, connector)); > if (!ret) > ret = PTR_ERR_OR_ZERO(drm_atomic_get_crtc_state(restore_state, crtc)); > - if (!ret) > - ret = PTR_ERR_OR_ZERO(drm_atomic_get_plane_state(restore_state, crtc->primary)); > if (ret) { > DRM_DEBUG_KMS("Failed to create a copy of old state to restore: %i\n", ret); > goto fail; > -- > 2.15.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Op 20-12-17 om 11:28 schreef Daniel Vetter: > On Wed, Dec 20, 2017 at 10:35:45AM +0100, Maarten Lankhorst wrote: >> From: Ville Syrjälä <ville.syrjala@linux.intel.com> >> >> We don't need any active planes during load detection, so just disable >> them all. This saves us from having to come up with a suitable >> framebuffer. And we also avoid leaving sprite/cursor planes on and >> potentially presenting them at a peculiar location during the load >> detection. >> >> Changes since v1 (Maarten): >> - Add missing call to add_all_affected_planes. >> >> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102707 > Less code, I like. And I think we have enough load-detect machines (+ plus > the nasty igt to do it anywhere we still have native vga) to have > reasonable ensurance it actually works. > > But maybe let's soak this in -next first for a while, then cherry-pick > over after a few weeks once it's solid. > > With the missing Fixes: line added. > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> Pushed, didn't add fixes since it's an ancient bug, for a piece of code that changed a lot. Since in most cases the fbcon fb will be leaked, the impact is just a warning and not even a real leak. :) Thanks for review, ~Maarten
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 7e833268c9df..ef61f9a75c93 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -9689,111 +9689,27 @@ intel_framebuffer_create(struct drm_i915_gem_object *obj, return ERR_PTR(ret); } -static u32 -intel_framebuffer_pitch_for_width(int width, int bpp) -{ - u32 pitch = DIV_ROUND_UP(width * bpp, 8); - return ALIGN(pitch, 64); -} - -static u32 -intel_framebuffer_size_for_mode(const struct drm_display_mode *mode, int bpp) -{ - u32 pitch = intel_framebuffer_pitch_for_width(mode->hdisplay, bpp); - return PAGE_ALIGN(pitch * mode->vdisplay); -} - -static struct drm_framebuffer * -intel_framebuffer_create_for_mode(struct drm_device *dev, - const struct drm_display_mode *mode, - int depth, int bpp) -{ - struct drm_framebuffer *fb; - struct drm_i915_gem_object *obj; - struct drm_mode_fb_cmd2 mode_cmd = { 0 }; - - obj = i915_gem_object_create(to_i915(dev), - intel_framebuffer_size_for_mode(mode, bpp)); - if (IS_ERR(obj)) - return ERR_CAST(obj); - - mode_cmd.width = mode->hdisplay; - mode_cmd.height = mode->vdisplay; - mode_cmd.pitches[0] = intel_framebuffer_pitch_for_width(mode_cmd.width, - bpp); - mode_cmd.pixel_format = drm_mode_legacy_fb_format(bpp, depth); - - fb = intel_framebuffer_create(obj, &mode_cmd); - if (IS_ERR(fb)) - i915_gem_object_put(obj); - - return fb; -} - -static struct drm_framebuffer * -mode_fits_in_fbdev(struct drm_device *dev, - const struct drm_display_mode *mode) -{ -#ifdef CONFIG_DRM_FBDEV_EMULATION - struct drm_i915_private *dev_priv = to_i915(dev); - struct drm_i915_gem_object *obj; - struct drm_framebuffer *fb; - - if (!dev_priv->fbdev) - return NULL; - - if (!dev_priv->fbdev->fb) - return NULL; - - obj = dev_priv->fbdev->fb->obj; - BUG_ON(!obj); - - fb = &dev_priv->fbdev->fb->base; - if (fb->pitches[0] < intel_framebuffer_pitch_for_width(mode->hdisplay, - fb->format->cpp[0] * 8)) - return NULL; - - if (obj->base.size < mode->vdisplay * fb->pitches[0]) - return NULL; - - drm_framebuffer_get(fb); - return fb; -#else - return NULL; -#endif -} - -static int intel_modeset_setup_plane_state(struct drm_atomic_state *state, - struct drm_crtc *crtc, - const struct drm_display_mode *mode, - struct drm_framebuffer *fb, - int x, int y) +static int intel_modeset_disable_planes(struct drm_atomic_state *state, + struct drm_crtc *crtc) { + struct drm_plane *plane; struct drm_plane_state *plane_state; - int hdisplay, vdisplay; - int ret; - - plane_state = drm_atomic_get_plane_state(state, crtc->primary); - if (IS_ERR(plane_state)) - return PTR_ERR(plane_state); - - if (mode) - drm_mode_get_hv_timing(mode, &hdisplay, &vdisplay); - else - hdisplay = vdisplay = 0; + int ret, i; - ret = drm_atomic_set_crtc_for_plane(plane_state, fb ? crtc : NULL); + ret = drm_atomic_add_affected_planes(state, crtc); if (ret) return ret; - drm_atomic_set_fb_for_plane(plane_state, fb); - plane_state->crtc_x = 0; - plane_state->crtc_y = 0; - plane_state->crtc_w = hdisplay; - plane_state->crtc_h = vdisplay; - plane_state->src_x = x << 16; - plane_state->src_y = y << 16; - plane_state->src_w = hdisplay << 16; - plane_state->src_h = vdisplay << 16; + + for_each_new_plane_in_state(state, plane, plane_state, i) { + if (plane_state->crtc != crtc) + continue; + + ret = drm_atomic_set_crtc_for_plane(plane_state, NULL); + if (ret) + return ret; + + drm_atomic_set_fb_for_plane(plane_state, NULL); + } return 0; } @@ -9811,7 +9727,6 @@ int intel_get_load_detect_pipe(struct drm_connector *connector, struct drm_crtc *crtc = NULL; struct drm_device *dev = encoder->dev; struct drm_i915_private *dev_priv = to_i915(dev); - struct drm_framebuffer *fb; struct drm_mode_config *config = &dev->mode_config; struct drm_atomic_state *state = NULL, *restore_state = NULL; struct drm_connector_state *connector_state; @@ -9879,10 +9794,6 @@ int intel_get_load_detect_pipe(struct drm_connector *connector, found: intel_crtc = to_intel_crtc(crtc); - ret = drm_modeset_lock(&crtc->primary->mutex, ctx); - if (ret) - goto fail; - state = drm_atomic_state_alloc(dev); restore_state = drm_atomic_state_alloc(dev); if (!state || !restore_state) { @@ -9914,39 +9825,17 @@ int intel_get_load_detect_pipe(struct drm_connector *connector, if (!mode) mode = &load_detect_mode; - /* We need a framebuffer large enough to accommodate all accesses - * that the plane may generate whilst we perform load detection. - * We can not rely on the fbcon either being present (we get called - * during its initialisation to detect all boot displays, or it may - * not even exist) or that it is large enough to satisfy the - * requested mode. - */ - fb = mode_fits_in_fbdev(dev, mode); - if (fb == NULL) { - DRM_DEBUG_KMS("creating tmp fb for load-detection\n"); - fb = intel_framebuffer_create_for_mode(dev, mode, 24, 32); - } else - DRM_DEBUG_KMS("reusing fbdev for load-detection framebuffer\n"); - if (IS_ERR(fb)) { - DRM_DEBUG_KMS("failed to allocate framebuffer for load-detection\n"); - ret = PTR_ERR(fb); - goto fail; - } - - ret = intel_modeset_setup_plane_state(state, crtc, mode, fb, 0, 0); - drm_framebuffer_put(fb); + ret = drm_atomic_set_mode_for_crtc(&crtc_state->base, mode); if (ret) goto fail; - ret = drm_atomic_set_mode_for_crtc(&crtc_state->base, mode); + ret = intel_modeset_disable_planes(state, crtc); if (ret) goto fail; ret = PTR_ERR_OR_ZERO(drm_atomic_get_connector_state(restore_state, connector)); if (!ret) ret = PTR_ERR_OR_ZERO(drm_atomic_get_crtc_state(restore_state, crtc)); - if (!ret) - ret = PTR_ERR_OR_ZERO(drm_atomic_get_plane_state(restore_state, crtc->primary)); if (ret) { DRM_DEBUG_KMS("Failed to create a copy of old state to restore: %i\n", ret); goto fail;