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

[Jesse Barnes]

On Tue, 8 Nov 2011 22:57:03 +0100
Daniel Vetter <daniel at ffwll.ch> wrote:
> I've not checked the watermarks, that kind of magic eludes me ;-) A few
> other comments below.

I need to get docs on that; in particular WM handling with downscaling
is an unknown.

> > +#define   DVS_SCALE_ENABLE	(1<<31)
> > +#define   DVS_FILTER_MASK	(3<<29)
> > +#define   DVS_FILTER_MEDIUM	(0<<29)
> > +#define   DVS_FILTER_ENHANCING	(1<<29)
> > +#define   DVS_FILTER_SOFTENING	(2<<29)
> 
> BSpec has some bits for deinterlace support here, maybe add them just for
> documentation.

Sure, I can toss those in.

> > +#define _SPRA_CTL		0x70280
> > +#define   SPRITE_ENABLE			(1<<31)
> > +#define   SPRITE_GAMMA_ENABLE		(1<<30)
> > +#define   SPRITE_PIXFORMAT_MASK		(7<<25)
> > +#define   SPRITE_FORMAT_YUV422		(0<<25)
> > +#define   SPRITE_FORMAT_RGBX101010	(1<<25)
> > +#define   SPRITE_FORMAT_RGBX888		(2<<25)
> > +#define   SPRITE_FORMAT_RGBX161616	(3<<25)
> > +#define   SPRITE_FORMAT_YUV444		(4<<25)
> > +#define   SPRITE_FORMAT_XBGR101010	(5<<25)
> 
> That XBRG is a bit misleading, because the X here means extended range as
> opposed to the usual gl meaning of unused. It sounds like a format with
> shared exponent, gl seems to use E for that, i.e E2BGR101010.

Ah I see, yeah there are 2 30 bit formats, one with XR bias...  I
should probably drop that since we don't know the actual bit layout.

> > +#define _SPRA_GAMC		0x70400
> 
> Gamma regs are a bit funky, especially on SNB. I assume you'll add all the
> necessary defines when set_gamma support shows up.

Yeah we may need some new ioctls to control the different colorspace
correction and gamma stuff the latest hw supports.  I think we can
correct at various places in the pipeline as well...

> > +struct intel_plane {
> > +	struct drm_plane base;
> > +	enum pipe pipe;
> > +	struct drm_i915_gem_object *obj;
> > +	int max_downscale;
> > +	u32 lut_r[1024], lut_g[1024], lut_b[1024];
> > +	void (*update_plane)(struct drm_plane *plane,
> > +			     struct drm_framebuffer *fb, unsigned long start,
> 
> start is a strange name for gtt_offset/fb_base/... I'd just pass the
> i915_gem_object.

Yeah, good point.

> > @@ -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?

> > + * 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.

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.

> > +	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.

> > +
> > +	/* Pipe must be running... */
> > +	if (!(I915_READ(PIPECONF(pipe)) & PIPECONF_ENABLE))
> > +		return -EINVAL;
> 
> Ok, how does this tie in with dpms handling? This here feels like a major
> cludge ... I suspect we want a dpms function on the plane that gets called
> by the common dpms helper before switching of the crtc and after switching
> it on. Or at least some driver-hack like I've done for the overlay.

It doesn't atm, but it should.

> > +	/*
> > +	 * Clamp the width & height into the visible area.  Note we don't
> > +	 * try to scale the source if part of the visible region is offscreen.
> > +	 * The caller must handle that by adjusting source offset and size.
> > +	 */
> 
> Allowing the crtc dest rect to extend beyond that of the crtc and then refusing to
> properly adjust tiling is a bit inconsistent. I call design-by-committee
> on this one ;-)
> 
> If the goal is that this plane stuff is somewhat generically useable, I
> think this needs to be fixed. If the goal is simply to keep the bubble
> intact, I don't mind this as-is, because I don't care ;-)

The primary goal here is to keep the plane from breaking if a crtc rect
that includes an offscreen portion is passed in.

Moving the source rect would involve potential scaling.  I can do that,
but was trying to avoid it.  We can also change it when we see what the
embedded folks had in mind (I think Rob was the one who wanted signed
crtc position to make some things easy).

> > +	if (obj->tiling_mode != I915_TILING_X) {
> > +		DRM_ERROR("plane surfaces must be X tiled\n");
> > +		ret = -EINVAL;
> > +		goto out_unlock;
> > +	}
> 
> Afaics the hw supports untiled buffers (you need to frob the linear offset
> register instead of the x/y register for tiled surfaces). Does that not
> work?

Yes it does, and I see the regs on IVB now.  I'll implement support for
them to make it easier to display surfaces from misc. sources without
copying.

> > +out_unlock:
> > +	mutex_unlock(&dev->struct_mutex);
> 
> Minor locking nitpick: You only need to hold around pin and unpin, not
> over the vblank. Stopping gem for 20 msec just to show a sprite is not so
> great. I know, our locking isn't really great in general and there are
> many other places where we stall in similarly bad ways, still ...

Yes, good point.  Will fix.

> 
> > +
> > +	return ret;
> > +}
> > +
> > +static int
> > +intel_disable_plane(struct drm_plane *plane)
> > +{
> > +	struct drm_device *dev = plane->dev;
> > +	struct intel_plane *intel_plane = to_intel_plane(plane);
> > +	int ret = 0;
> > +
> > +	mutex_lock(&dev->struct_mutex);
> > +
> > +	intel_plane->disable_plane(plane);
> 
> Again, move the vblank_wait which is inside the disable_plane outside of
> the struct_mutex lock. We really need some generic infrastructure to run
> something after the next vblank in a workqueu. Would clean up pageflip,
> the legacy overlay and kill the vblank here ...

Yeah, that would be nice.  Blocking userspace on the vblank is also not
nice.

> > +	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.

Thanks,
-- 
Jesse Barnes, Intel Open Source Technology Center
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/intel-gfx/attachments/20111108/106eae76/attachment.sig>