Re: [Intel-gfx] [PATCH 1/2] drm/i915: Tidy workaround batch buffer emission

[Chris Wilson]

On Thu, Feb 16, 2017 at 07:59:30AM +, Tvrtko Ursulin wrote:
> 
> On 15/02/2017 21:18, Chris Wilson wrote:
> >Reviewed-by: Chris Wilson 
> 
> Thanks. So you think it is worth it?

Yes. As you say, it brings it into line with the rest of the command
emission sequences - and using a consistent pattern is worth the
investment.

> I am just annoyed with the
> BUG_ON. I don't like to add them, but GEM_BUG_ON seemed to weak.

That came across in your choice. At the point we hit the BUG_ON, the
system is probably/should be dead already.

> On
> balance it felt that it is OK. Overall I think it is easier to
> follow the flow from the code with this organisation and it is more
> compact in all respects.

gen8_emit_pipe_control() is all the justification I need. That makes
the benefits very clear.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/2] drm/i915: Tidy workaround batch buffer emission

[Tvrtko Ursulin]

On 15/02/2017 21:18, Chris Wilson wrote:

On Wed, Feb 15, 2017 at 04:06:33PM +, Tvrtko Ursulin wrote:

From: Tvrtko Ursulin 

Use the "*batch++ = " style as in the ring emission for better
readability and also simplify the logic a bit by consolidating
the offset and size calculations and overflow checking. The
latter is a programming error so it is not required to check
for it after each write to the object, but rather do it once the
whole state has been written and fail the driver if something
went wrong.

v2: Rebase.


I did not spot any mistakes in the mechanical translations.


+   /*
+* Emit the two workaround batch buffers, recording the offset from the
+* start of the workaround batch buffer object for each and their
+* respective sizes.
+*/
+   for (i = 0; i < ARRAY_SIZE(wa_bb_f); i++) {
+   wa_bb[i]->offset = ALIGN(batch_ptr - batch, CACHELINE_DWORDS);
+   batch_ptr = wa_bb_f[i](engine, batch_ptr);


I'm just a bit more familar with using _fn for function pointers.


Will change it, pattern must have escaped me.


+   wa_bb[i]->size = batch_ptr - [wa_bb[i]->offset];


Surprised me that we store the offset/size in dwords not bytes. And then
convert to bytes to store in the registers.


I'll check it out.


}

-out:
+   BUG_ON(batch_ptr - batch > CTX_WA_BB_OBJ_SIZE / sizeof(u32));


Would it make sense going forward just use void *batch, batch_ptr here
and compute bytes?


Together with this.


Reviewed-by: Chris Wilson 


Thanks. So you think it is worth it? I am just annoyed with the BUG_ON. 
I don't like to add them, but GEM_BUG_ON seemed to weak. On balance it 
felt that it is OK. Overall I think it is easier to follow the flow from 
the code with this organisation and it is more compact in all respects.


Regards,

Tvrtko
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/2] drm/i915: Tidy workaround batch buffer emission

[Chris Wilson]

On Wed, Feb 15, 2017 at 04:06:33PM +, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin 
> 
> Use the "*batch++ = " style as in the ring emission for better
> readability and also simplify the logic a bit by consolidating
> the offset and size calculations and overflow checking. The
> latter is a programming error so it is not required to check
> for it after each write to the object, but rather do it once the
> whole state has been written and fail the driver if something
> went wrong.
> 
> v2: Rebase.

I did not spot any mistakes in the mechanical translations.

> + /*
> +  * Emit the two workaround batch buffers, recording the offset from the
> +  * start of the workaround batch buffer object for each and their
> +  * respective sizes.
> +  */
> + for (i = 0; i < ARRAY_SIZE(wa_bb_f); i++) {
> + wa_bb[i]->offset = ALIGN(batch_ptr - batch, CACHELINE_DWORDS);
> + batch_ptr = wa_bb_f[i](engine, batch_ptr);

I'm just a bit more familar with using _fn for function pointers.

> + wa_bb[i]->size = batch_ptr - [wa_bb[i]->offset];

Surprised me that we store the offset/size in dwords not bytes. And then
convert to bytes to store in the registers.

>   }
>  
> -out:
> + BUG_ON(batch_ptr - batch > CTX_WA_BB_OBJ_SIZE / sizeof(u32));

Would it make sense going forward just use void *batch, batch_ptr here
and compute bytes?

Reviewed-by: Chris Wilson 
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx