[Intel-gfx] [PATCH] intel: Don't keep intel->pClipRects, and instead just calculate it when needed.

[Keith Packard]

On Sun, 2008-10-19 at 20:09 -0700, Eric Anholt wrote:
> This avoids issues with dereferencing stale cliprects around intel_draw_buffer
> time.  Additionally, take advantage of cliprects staying constant for FBOs and
> DRI2, and emit cliprects in the batchbuffer instead of having to flush batch
> each time they change.

I'm going to start off with some style comments so we can get the patch
looking good at least.

> +   clip_mode = intel->constant_cliprect ? NO_LOOP_CLIPRECTS: LOOP_CLIPRECTS;

It seems like we have three modes here for managing clipping:

     1. Non-rendering commands (classic NO_LOOP_CLIPRECTS)
     2. Legacy shared buffer rendering (classic LOOP_CLIPRECTS)
     3. Constant single rectangle clipping (new 'constant_cliprect'
        mode)

Inside the intel structure, we'd have the 'rendering mode', using either
mode 2 or 3.

> +   } else {
> +      state->Buffer[I830_DESTREG_DRAWRECT0] = MI_NOOP;
> +      state->Buffer[I830_DESTREG_DRAWRECT1] = MI_NOOP;
> +      state->Buffer[I830_DESTREG_DRAWRECT2] = MI_NOOP;
> +      state->Buffer[I830_DESTREG_DRAWRECT3] = MI_NOOP;
> +      state->Buffer[I830_DESTREG_DRAWRECT4] = MI_NOOP;
> +      state->Buffer[I830_DESTREG_DRAWRECT5] = MI_NOOP;
> +   }

A nice comment here explaining that these values will not actually be
used might be good. An assert that I830_DESTREG_DRAWRECT0 != MI_NOOP
where they are used would also be a good check.

> +void
> +intel_get_cliprects(struct intel_context *intel,
> +		    struct drm_clip_rect **cliprects,
> +		    unsigned int *num_cliprects,
> +		    int *x_off, int *y_off)

The question I have here is whether this will be called 'too late'
sometimes -- will we be flushing rendering after the old clip list has
been replaced with the new clip list?

> +	 if (fb->_ColorDrawBufferIndexes[0] == BUFFER_FRONT_LEFT) {
> +	    if (!intel->constant_cliprect && !intel->front_cliprects)
> +	       intel_batchbuffer_flush(intel->batch);

Right here, I'm concerned that flushing the buffer will fetch the wrong
clip list. It seems like this will be called after the new clip list has
been installed, or am I just misreading the code (again)?

I'll have to look at the pixel fetching code in more detail before I
understand what's going on.

-- 
keith.packard at intel.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
URL: <http://lists.freedesktop.org/archives/intel-gfx/attachments/20081021/baa9d8e4/attachment.sig>