Re: [PATCH 1/2] rendercopy/bdw: Enable hw-generated binding tables

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, May 08, 2014 at 01:18:40PM +0300, Ville Syrjälä wrote:
> On Thu, May 08, 2014 at 12:54:47PM +0300, Ville Syrjälä wrote:
> > On Wed, May 07, 2014 at 11:59:23PM +0300, Abdiel Janulgue wrote:
> > > On Wednesday, May 07, 2014 02:49:31 PM Ville Syrjälä wrote:
> > > > I quickly cobbled together a hsw version of this and gave it a whirl on
> > > > one machine. Seems to work just fine here, and no lockups when switching
> > > > between hw and sw binding tables. Did you get the lockups on hsw even
> > > > with rendercopy?
> > > > 
> > > > Here's my hsw version:
> > > > 
> > > > 
> > > > +static void
> > > > +gen7_hw_binding_table(struct intel_batchbuffer *batch, bool enable)
> > > > +{
> > > > +	if (!enable) {
> > > > +		OUT_BATCH(MI_RS_CONTROL | 0x0);
> > > > +
> > > > +		OUT_BATCH(HSW_3DSTATE_BINDING_TABLE_POOL_ALLOC | (3 - 2));
> > > > +		/* binding table pool base address */
> > > 
> > > This bit I missed and the source of my troubles for the past few months.
> > > 
> > > > +		OUT_BATCH(3 << 5);
> > > 
> > > Yep, I confirm toggling on HSW does work quite well now. I'll now update the 
> > > patches to include HSW path on the kernel. I also take back my previous 
> > > statement that RS is broken on HSW! :)
> > 
> > Excellent.
> > 
> > I was wondering a bit if we need to make the kernel turn off the hw
> > binding tables between batches, but since we now have per fd default
> > contexts and 3DSTATE_BINDING_TABLE_POOL_ALLOC should be saved in the
> > context, maybe we don't actually need to do that. Although it seems
> > like that would cause problems when we switch to the global default
> > context since we use the restore_inhibit flag there. So maybe we need
> > to special case the default context here and force the hw binding
> > tables off when switching to it.
> 
> Ah actually we still inhibit restore even with the per-file default
> contexts. So I guess it's going to be a problemn whenever we switch
> to any default context and the hw binding tables were left enabled.
> 
> So I guess there are two options: either drop the restore_inhibit flag
> for default contexts (I kind of like this idea but maybe other people
> hate it), or have the kernel turn off the hw binding tables when
> switching to any default context. The latter would also imply that if
> user space wants to use the resource streamer with the default context,
> it has to turn the binding tables on at the start of every batch since
> it can't know whether the kernel turned them off.

I think as soon as we have the golden context stuff from Mika we could
drop our usage of restore_inhibit. We only need that to avoid the hw
getting angry if the context state is illegal afaik.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch