[Intel-gfx] [PATCH] support tiled rendering in 2D driver

[Jesse Barnes]

On Friday, January 23, 2009 4:24 pm Eric Anholt wrote:
> > +/**
> > + * On some chips, pitch width has to be a power of two tile width, so
> > + * calculate that here.
> > + */
> > +unsigned long
> > +I830GetFencePitch(I830Ptr pI830, unsigned long pitch, int format)
>
> DeathToStudlyCaps()

Yeah sorry, was just trying to blend in.  We should just have a flag day on 
the driver and change all the formatting.

> > +    if (IS_I965G(pI830)) {
> > +	/* 965 is flexible */
> > +	if (format == I915_TILING_Y)
> > +	    tile_width = 128;
> > +
> > +	return ROUND_TO(pitch, tile_width);
> > +    }
> > +
> > +    if (IS_I945GM(pI830) && format == I915_TILING_Y)
> > +	tile_width = 128;
>
> I'd skip the chipset-based conditionals here.  If they asked for
> TILING_Y, give them the TILING_Y-aligned width.  You missed
> IS_I945G(pI830) here.

Yeah, good point.  I think we want G33 too; may as well create a SUPPORT_YTILE 
or such macro.

> > +    /* Pre-965 needs power of two tile width */
> > +    for (i = tile_width; i < pitch; i <<= 1)
> > +	;
> > +
> > +    return ROUND_TO(pitch, i);
>
> this is just return i, right?

Yep.

> > +}
> > +
> >  typedef struct {
> >      PixmapPtr pPixmap;
> >  } I830DRI2BufferPrivateRec, *I830DRI2BufferPrivatePtr;
> > @@ -1570,7 +1598,7 @@ I830DRI2CreateBuffers(DrawablePtr pDraw, unsigned
> > int *attachments, int count) pDraw->depth, 0);
> >  	    switch (attachments[i]) {
> >  	    case DRI2BufferDepth:
> > -		if (IS_I965G(pI830))
> > +		if (IS_I965G(pI830) || IS_I945GM(pI830))
> >  		    tiling = I915_TILING_Y;
> >  		else
> >  		    tiling = I915_TILING_X;
>
> Interesting.  This is the first time we've done TILING_Y on pre-965.  I
> think Mesa is prepared for it.  Oh, and 945G probably wants it too in
> that case.

Yeah, even the 915 docs say it's supported for non-frontbuffer surfaces, but I 
haven't managed to get it working yet; need to play around some more I guess.

> > @@ -1583,19 +1611,15 @@ I830DRI2CreateBuffers(DrawablePtr pDraw, unsigned
> > int *attachments, int count) break;
> >  	    }
> >
> > -	    /* Disable tiling on 915-class 3D for now.  Because the 2D blitter
> > -	     * requires fence regs to operate, and they're not being managed
> > -	     * by the kernel yet, we don't want to expose tiled buffers to the
> > -	     * 3D client as it'll just render incorrectly if it pays attention
> > -	     * to our tiling bits at all.
> > -	     */
> > -	    if (!IS_I965G(pI830))
> > +	    if (!pI830->tiling)
> >  		tiling = I915_TILING_NONE;
>
> Since we've got released kernels without the support, we should probably
> only enable this if we got success from the ioctl for setting the number
> of fences or from getting available fences.

Yeah, definitely.

> >  	    if (tiling != I915_TILING_NONE) {
> > +		int pitch = (pDraw->width * pDraw->bitsPerPixel + 7) / 8;
> > +		int stride = I830GetFencePitch(pI830, pitch, tiling);
> > +
> >  		bo = i830_get_pixmap_bo(pPixmap);
> > -		drm_intel_bo_set_tiling(bo, &tiling,
> > -					pDraw->width * pDraw->bitsPerPixel / 8);
> > +		drm_intel_bo_set_tiling(bo, &tiling, stride);
>
> If stride is anything but i830_pixmap_pitch(pPixmap), we'd be in for
> some serious trouble.  Let's just use that.

Hm yeah...

> >      if (w > 32767 || h > 32767)
> > @@ -884,9 +884,12 @@ i830_uxa_create_pixmap (ScreenPtr screen, int w, int
> > h, int depth, unsigned usag {
> >  	stride = ROUND_TO((w * pixmap->drawable.bitsPerPixel + 7) / 8,
> >  			  i830->accel_pixmap_pitch_alignment);
> > +
> > +	stride = I830GetFencePitch(i830, stride, I915_TILING_X);
> > +
> > +	align = stride * h > 1024*1024 ? stride * h : 1024*1024;
>
> If we're going to have to calculate alignment in userland for the
> purpose of tiling, it should be a separate function like it is in the
> kernel, and match its behavior (I still think the kernel should just do
> it for us).

Alignments are easy I think, we can handle those in the kernel (though it 
looked like we were getting them wrong in some cases for display buffers in 
i830_memory.c).  But sizes & strides need to be done in userland, and will 
depend on the surface type, chipset and whether the buffer will be used for 
scanout (the 915 docs even say you can use Y tiling for color buffers if 
they're not going to be displayed directly).  Maybe we should add some 
surface allocation routines to libdrm_intel to keep the logic in one place?  
Of course we'd need to pass that info to create_pixmap somehow so it can make 
the right call...

> >  	if (pI830->memory_manager != NULL) {
> > +	    sp.param = I915_SETPARAM_NUM_USED_FENCES;
> > +	    if (pI830->use_drm_mode)
> > +		sp.value = 0; /* kernel gets them all */
> > +	    else if (pI830->directRenderingType == DRI_XF86DRI)
> > +		sp.value = 3; /* front/back/depth */
> > +	    else
> > +		sp.value = 2; /* just front for DRI2 (both old & new though) */
> > +	    /* Don't care about failure, we'll just get slow rendering */
> > +	    drmCommandWrite(pI830->drmSubFD, DRM_I915_SETPARAM, &sp,
> > +			    sizeof(sp));
> > +
>
> Well, we'll get wrong rendering as well if we enable tiling despite not
> having a new enough kernel.  Any SW fallbacks will use the tiling state
> rather than the unset fence registers, and you'll get swizzled.

Yeah we need the flag you mentioned, looks like the patch you posted has that 
covered.

-- 
Jesse Barnes, Intel Open Source Technology Center