On Tue, Nov 08, 2011 at 02:31:00PM -0800, Jesse Barnes wrote:
> On Tue, 8 Nov 2011 22:57:03 +0100 Daniel Vetter <dan...@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
--
Daniel Vetter
Mail: dan...@ffwll.ch
Mobile: +41 (0)79 365 57 48
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
