Re: [Intel-gfx] [PATCH 3/5] drm/i915: add SNB and IVB video sprite support

[Daniel Vetter <daniel@ffwll.ch>]

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
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48