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

[Eric Anholt]

On Tue, 2008-10-21 at 08:52 -0700, Keith Packard wrote:
> 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)

NO_LOOP_CLIPRECTS is "Under no circumstances run my commands multiple
times for your cliprects handling" (whcih REFERENCES_CILPRECTS is a
special case of).  I think you meant IGNORE_CLIPRECTS which is "this is
a state update that could be run multiple times or not, I don't care"

>      2. Legacy shared buffer rendering (classic LOOP_CLIPRECTS)
>      3. Constant single rectangle clipping (new 'constant_cliprect'
>         mode)

I've moved the constant_cliprect upgrading of LOOP_CLIPRECTS to
NO_LOOP_CLIPRECTS to one place (intel_batchbuffer_require_space), and
also extended it to the REFERENCES_CLIPRECTS case.

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

Added the assert.

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

When we flush rendering we get the current cliprects which are chosen by
intel->constant_cliprect and intel->front_cliprects.  The flushing for
changing intel->constant_cliprect or intel->front_cliprects occurs
before setting those fields, but if we do flush then, and we're
!intel->constant_cliprect, we still pick up the latest cliprects from
DRI.

> > +	 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.
> 
-- 
Eric Anholt
eric at anholt.net                         eric.anholt at intel.com


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 197 bytes
Desc: This is a digitally signed message part
URL: <http://lists.freedesktop.org/archives/intel-gfx/attachments/20081028/cb50be5b/attachment.sig>