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

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 15/02/2017 21:18, Chris Wilson wrote:
> On Wed, Feb 15, 2017 at 04:06:33PM +0000, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> 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 - &batch[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 <chris@chris-wilson.co.uk>

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