On Tue, Nov 08, 2011 at 02:31:00PM -0800, Jesse Barnes wrote:
On Tue, 8 Nov 2011 22:57:03 +0100 Daniel Vetter daniel@ffwll.ch wrote:
@@ -270,8 +270,14 @@ void intel_fb_restore_mode(struct drm_device *dev) { int ret; drm_i915_private_t *dev_priv = dev->dev_private;
struct drm_mode_config *config = &dev->mode_config;
struct drm_plane *plane;
ret = drm_fb_helper_restore_fbdev_mode(&dev_priv->fbdev->helper); if (ret) DRM_DEBUG("failed to restore crtc mode\n");
/* Be sure to shut off any planes that may be active */
list_for_each_entry(plane, &config->plane_list, head)
plane->funcs->disable_plane(plane);This should be part of the fb_helper above.
That would be a bit invasive... and may not be what every driver wants. Does it belong in set_config? Or do we just forcibly disable the planes everywhere?
Core drm code currently does not call fb_helper_restore, that's the drivers job atm (yeah, somewhere on my todo). And i915 is currently the only one with sprite support. And I don't think we want the last video frame to occlude the OOPS when X died. So yes, this imo belongs into the fb_helper.
- Note on refcounting:
- When the user creates an fb for the GEM object to be used for the plane,
- a ref is taken on the object. However, if the application exits before
- disabling the plane, the DRM close handling will free all the fbs and
- unless we take a ref on the object, it will be destroyed before the
- plane disable hook is called, causing obvious trouble with our efforts
- to look up and unpin the object. So we take a ref after we move the
- object to the display plane so it won't be destroyed until our disable
- hook is called and we drop our private reference.
- */
Actually, this is wrong. Before the fb gets destroyed we call drm_framebuffer_cleanup which takes care of this problem. The fact that
- currently drivers call this instead of the drm core
- it's a ducttape solution instead of refcounting
doesn't really make it nice, but it works.
Not sure I understand what you mean about drm_framebuffer_cleanup taking care of the problem. The refcounting and fb handling may not be ideal, but taking refs on the underlying objects is what we need to do today.
Well, I think we don't need to take refs. set_base gets away with it, too: - the pin/unpin ensures that the buffer doesn't move. - the drm core holds onto both the old and the new fb for the duration of the update_plane, so we also have a reference on that. And that reference won't disappear without a disable_plane thanks to the addition to drm_framebuffer_cleanup I've prodded you into writing.
I don't want to change that as part of this series, but did want to explain why we take a private ref for the plane object here.
Yeah, cleanup up our fb handling will be a bit messy.
- sprctl = I915_READ(reg);
reg isn't really a great var name. Imo just drop it, only used twice and reduces readability.
Sure, easy enough.
- I915_WRITE(SPRCTL(pipe), I915_READ(SPRCTL(pipe)) & ~SPRITE_ENABLE);
- I915_WRITE(SPRSCALE(pipe), 0);
- I915_WRITE(SPRSURF(pipe), 0);
Is that required or just paranoia? I haven't found anything in bspec suggesting it's required.
The scaling disable was just to avoid surprises (in fact now that I look I need to mask it off in the update function).
The surf write is definitely needed, since it triggers the double buffered reg update.
Ok, missed that in bspec review. Maybe add a comment for dummies somewhere?
[snip]
- ret = i915_gem_object_finish_gpu(intel_plane->obj);
- if (ret)
goto out_unlock;What's the reason for that finish_gpu here?
Slavish emulation of some other teardown code paths. If we ever do flipping on the sprites, I think we'll want it.
I think when we do flipping on sprites, we should try MI_LOAD_REG to latch the double-buffered regs in the command stream. That way it'd work like the pageflip paths. -Daniel