[Mesa-dev] [PATCH] radeon/vce: cleanup cpb handling

[Christian König]

Am 03.04.2014 16:03, schrieb Leo Liu:
> From: Leo Liu <leo.liu at amd.com>
>
> Signed-off-by: Leo Liu <leo.liu at amd.com>
> ---
>   src/gallium/drivers/radeon/radeon_vce.c        | 22 +++++++++++++++++++---
>   src/gallium/drivers/radeon/radeon_vce.h        |  3 +++
>   src/gallium/drivers/radeon/radeon_vce_40_2_2.c |  6 ++++--
>   3 files changed, 26 insertions(+), 5 deletions(-)
>
> diff --git a/src/gallium/drivers/radeon/radeon_vce.c b/src/gallium/drivers/radeon/radeon_vce.c
> index 0d3fe08..eb49863 100644
> --- a/src/gallium/drivers/radeon/radeon_vce.c
> +++ b/src/gallium/drivers/radeon/radeon_vce.c
> @@ -45,8 +45,6 @@
>   #include "radeon_video.h"
>   #include "radeon_vce.h"
>   
> -#define CPB_SIZE (40 * 1024 * 1024)
> -
>   /**
>    * flush commands to the hardware
>    */
> @@ -213,6 +211,9 @@ struct pipe_video_codec *rvce_create_encoder(struct pipe_context *context,
>   {
>   	struct r600_common_screen *rscreen = (struct r600_common_screen *)context->screen;
>   	struct rvce_encoder *enc;
> +	struct pipe_video_buffer *tmp_buf, templat = {};
> +	struct radeon_surface *tmp_surf;
> +	unsigned pitch, vpitch;
>   
>   	if (!rscreen->info.vce_fw_version) {
>   		RVID_ERR("Kernel doesn't supports VCE!\n");
> @@ -246,8 +247,23 @@ struct pipe_video_codec *rvce_create_encoder(struct pipe_context *context,
>   	}
>   
>   	enc->ws->cs_set_flush_callback(enc->cs, rvce_cs_flush, enc);
> +	templat.buffer_format = PIPE_FORMAT_NV12;
> +	templat.chroma_format = PIPE_VIDEO_CHROMA_FORMAT_420;
> +	templat.width = enc->base.width;
> +	templat.height = enc->base.height;
> +	templat.interlaced = false;
> +	if (!(tmp_buf = context->create_video_buffer(context, &templat))) {
> +		RVID_ERR("Can't create video buffer.\n");
> +		goto error;
> +	}
>   
> -	if (!rvid_create_buffer(enc->ws, &enc->cpb, CPB_SIZE, RADEON_DOMAIN_VRAM)) {
> +	get_buffer(((struct vl_video_buffer *)tmp_buf)->resources[0], NULL, &tmp_surf);
> +	pitch = align(tmp_surf->level[0].pitch_bytes, 128);
> +	vpitch = align(tmp_surf->npix_y, 16);
> +	tmp_buf->destroy(tmp_buf);
> +	if (!rvid_create_buffer(enc->ws, &enc->cpb,
> +			pitch * vpitch * 1.5 * (RVCE_NUM_CPB_FRAMES + RVCE_NUM_CPB_EXTRA_FRAMES),
> +			RADEON_DOMAIN_VRAM)) {
>   		RVID_ERR("Can't create CPB buffer.\n");
>   		goto error;
>   	}
> diff --git a/src/gallium/drivers/radeon/radeon_vce.h b/src/gallium/drivers/radeon/radeon_vce.h
> index 80495e5..1dc1a61 100644
> --- a/src/gallium/drivers/radeon/radeon_vce.h
> +++ b/src/gallium/drivers/radeon/radeon_vce.h
> @@ -43,6 +43,9 @@
>   #define RVCE_READWRITE(buf, domain) RVCE_CS(RVCE_RELOC(buf, RADEON_USAGE_READWRITE, domain) * 4)
>   #define RVCE_END() *begin = (&enc->cs->buf[enc->cs->cdw] - begin) * 4; }
>   
> +#define RVCE_NUM_CPB_FRAMES 3

3 not 2?

> +#define RVCE_NUM_CPB_EXTRA_FRAMES 2
> +
>   struct r600_common_screen;
>   
>   /* driver dependent callback */
> diff --git a/src/gallium/drivers/radeon/radeon_vce_40_2_2.c b/src/gallium/drivers/radeon/radeon_vce_40_2_2.c
> index b0961a9..1ac959c 100644
> --- a/src/gallium/drivers/radeon/radeon_vce_40_2_2.c
> +++ b/src/gallium/drivers/radeon/radeon_vce_40_2_2.c
> @@ -282,7 +282,8 @@ static void encode(struct rvce_encoder *enc)
>   
>   	RVCE_CS(0x00000000); // pictureStructure
>   
> -	luma_offset = (2 * ((enc->pic.frame_num - 1) % 2) * fsize + 2 * fsize);
> +	luma_offset = (RVCE_NUM_CPB_FRAMES - 1) * ((enc->pic.frame_num - 1) % 2) * fsize +
> +		      RVCE_NUM_CPB_EXTRA_FRAMES * fsize;
>   	if (enc->pic.picture_type == PIPE_H264_ENC_PICTURE_TYPE_IDR) {
>   		RVCE_CS(0x00000000); // encPicType
>   		RVCE_CS(0x00000000); // frameNumber
> @@ -306,7 +307,8 @@ static void encode(struct rvce_encoder *enc)
>   		RVCE_CS(0xffffffff); // chromaOffset
>   	}
>   	
> -	luma_offset = (2 * (enc->pic.frame_num % 2) * fsize + 2 * fsize);
> +	luma_offset = (RVCE_NUM_CPB_FRAMES - 1) * (enc->pic.frame_num % 2) * fsize +
> +		      RVCE_NUM_CPB_EXTRA_FRAMES * fsize;

Those calculations actually look rather odd to me.

First of all we should probably put "RVCE_NUM_CPB_EXTRA_FRAMES * fsize" 
into an extra variable, something like "base_offset" or so.

And then my guess for the calculation would be something like 
"(RVCE_NUM_CPB_FRAMES) * (enc->pic.frame_num % RVCE_NUM_CPB_FRAMES) * 
fsize" instead of this.

And while at it please put the frame offset calculation into a separate 
function.

Thanks,
Christian.

>   	RVCE_CS(luma_offset); // encReconstructedLumaOffset
>   	RVCE_CS(chroma_offset + luma_offset); // encReconstructedChromaOffset
>   	RVCE_CS(0x00000000); // encColocBufferOffset