[igt-dev] [PATCH i-g-t v4 4/4] lib/intel_batchbuffer: Move batch functions from media/render/gpgpu libs

[Daniele Ceraolo Spurio]

On 20/04/18 04:15, Lukasz Kalamarz wrote:
> Batch functions were copy/pasted across several libs.
> With moving it into intel_batchbuffer lib test can now be
> easly maintained without worrying that we forgot to modify
> older version of lib.
> 
> v2: Added documentation into lib and rebased patch
> v3: Fixed typos and rebased patch
> v4: Fixed documentation issues
> 
> Signed-off-by: Lukasz Kalamarz <lukasz.kalamarz at intel.com>
> Cc: Katarzyna Dec <katarzyna.dec at intel.com>
> Cc: Radoslaw Szwichtenberg <radoslaw.szwichtenberg at intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> ---

<snip>

> +
> +/**
> + * intel_batchbuffer_subdata_alloc:
> + * @batch: batchbuffer object
> + * @size: amount of bytes need to allocate
> + * @align: value in bytes to which we want to align
> + *
> + * Verify if sufficient @size within @batch is available to deny overflow.
> + * Then allocate @size bytes within @batch.
> + *
> + * Returns: Offset within @batch between allocated subdata and base of @batch.
> + */
> +void *
> +intel_batchbuffer_subdata_alloc(struct intel_batchbuffer *batch, uint32_t size,
> +				uint32_t align)
> +{
> +	uint32_t offset = intel_batchbuffer_align(batch, align);
> +
> +	igt_assert(batch->ptr + size < &batch->buffer[4095]);

Here you're not considering the reserved space for the BBEND. This is ok 
for the current use-case because the libraries actually put the BBEND in 
the middle of the batch, but I'd still either mention it in the 
description or change the check to:

	igt_assert(size <= intel_batchbuffer_space(batch));

to make sure we don't hit weird behaviours in the future.

> +
> +	batch->ptr += size;
> +	return memset(batch->buffer + offset, 0, size);
> +}
> +
> +/**
> + * intel_batchbuffer_subdata_offset:
> + * @batch: batchbuffer object
> + * @ptr: pointer to given data
> + *
> + * Returns: Offset within @batch between @ptr and base of @batch.
> + */
> +uint32_t
> +intel_batchbuffer_subdata_offset(struct intel_batchbuffer *batch, void *ptr)
> +{
> +	return (uint8_t *)ptr - batch->buffer;
> +}
> +
> +/**
>    * intel_batchbuffer_reset:
>    * @batch: batchbuffer object
>    *
> @@ -288,22 +343,28 @@ intel_batchbuffer_emit_reloc(struct intel_batchbuffer *batch,
>   }
>   
>   /**
> - * intel_batchbuffer_data:
> + * intel_batchbuffer_copy_data:
>    * @batch: batchbuffer object
>    * @data: pointer to the data to write into the batchbuffer
>    * @bytes: number of bytes to write into the batchbuffer
> + * @align: value in bytes to which we want to align
>    *
>    * This transfers the given @data into the batchbuffer. Note that the length
>    * must be DWORD aligned, i.e. multiples of 32bits.

Could add something like: "the caller must confirm that there is enough 
space in the batch for the data to be copied".

> + *
> + * Returns: Offset of copied data.
>    */
> -void
> -intel_batchbuffer_data(struct intel_batchbuffer *batch,
> -                       const void *data, unsigned int bytes)
> +uint32_t
> +intel_batchbuffer_copy_data(struct intel_batchbuffer *batch,
> +			    const void *data, unsigned int bytes,
> +			    uint32_t align)
>   {
> -	igt_assert((bytes & 3) == 0);

The length should be dword-aligned (as also mentioned in the doc above) 
so this check should stay as well.

> -	intel_batchbuffer_require_space(batch, bytes);
> -	memcpy(batch->ptr, data, bytes);
> -	batch->ptr += bytes;
> +	uint32_t *subdata;
> +
> +	subdata = intel_batchbuffer_subdata_alloc(batch, bytes, align);
> +	memcpy(subdata, data, bytes);
> +
> +	return intel_batchbuffer_subdata_offset(batch, subdata);
>   }
>   
>   /**
> diff --git a/lib/intel_batchbuffer.h b/lib/intel_batchbuffer.h
> index 6744bcb..3183fee 100644
> --- a/lib/intel_batchbuffer.h
> +++ b/lib/intel_batchbuffer.h
> @@ -40,8 +40,9 @@ void intel_batchbuffer_flush_with_context(struct intel_batchbuffer *batch,
>   
>   void intel_batchbuffer_reset(struct intel_batchbuffer *batch);
>   
> -void intel_batchbuffer_data(struct intel_batchbuffer *batch,
> -                            const void *data, unsigned int bytes);
> +uint32_t intel_batchbuffer_copy_data(struct intel_batchbuffer *batch,
> +				const void *data, unsigned int bytes,
> +				uint32_t align);
>   
>   void intel_batchbuffer_emit_reloc(struct intel_batchbuffer *batch,
>   				  drm_intel_bo *buffer,
> @@ -50,6 +51,19 @@ void intel_batchbuffer_emit_reloc(struct intel_batchbuffer *batch,
>   				  uint32_t write_domain,
>   				  int fenced);
>   
> +uint32_t
> +intel_batchbuffer_align(struct intel_batchbuffer *batch, uint32_t align);
> +
> +uint32_t
> +intel_batchbuffer_round_upto(struct intel_batchbuffer *batch, uint32_t divisor);

This is not part of intel_batchbuffer.c anymore so it doesn't belong 
here. No need to declare it anywhere since it is only used in the same 
file it is implemented. A small extra comment on this function below.

> +
> +void *
> +intel_batchbuffer_subdata_alloc(struct intel_batchbuffer *batch,
> +				uint32_t size, uint32_t align);
> +
> +uint32_t
> +intel_batchbuffer_subdata_offset(struct intel_batchbuffer *batch, void *ptr);
> +
>   /* Inline functions - might actually be better off with these
>    * non-inlined.  Certainly better off switching all command packets to
>    * be passed as structs rather than dwords, but that's a little bit of


<snip>

> --- a/lib/rendercopy_gen6.c
> +++ b/lib/rendercopy_gen6.c
> @@ -48,50 +48,16 @@ static const uint32_t ps_kernel_nomask_affine[][4] = {
>   	{ 0x0000007e, 0x00000000, 0x00000000, 0x00000000 },
>   };
>   
> -static uint32_t
> -batch_used(struct intel_batchbuffer *batch)
> +uint32_t
> +intel_batchbuffer_round_upto(struct intel_batchbuffer *batch, uint32_t divisor)

this is now local to this file so you can skip the renaming. Even if you 
keep the renaming you should keep it static.

Daniele