[Mesa-dev] [PATCH 1/4] radeon/vce: allocate at least 4KB of memory for the feedback buffer

[Christian König]

NAK to the whole approach.

VCE feedback buffers are completely separated to UVD or other MM 
feedback buffers, so they shouldn't be allocated in radeon_video.c

Additional to that older UVD message, feedback and bitstream buffers 
have special memory placement requirements. So you clearly shouldn't 
allocate those from the sub allocator.

Sorry that I didn't objected earlier, but I wasn't aware of this change 
till now.

Regards,
Christian.

Am 29.09.2016 um 18:35 schrieb Nicolai Hähnle:
> From: Nicolai Hähnle <nicolai.haehnle at amd.com>
>
> The kernel's CS checker requires it. This fixes a regression introduced by
> the buffer sub-allocation.
>
> Cc: Christian König <christian.koenig at amd.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97976
> ---
>   src/gallium/drivers/radeon/radeon_vce.c   |  6 +++---
>   src/gallium/drivers/radeon/radeon_video.c | 12 ++++++++++++
>   src/gallium/drivers/radeon/radeon_video.h |  3 +++
>   3 files changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/src/gallium/drivers/radeon/radeon_vce.c b/src/gallium/drivers/radeon/radeon_vce.c
> index 10c5a78..dd4c367 100644
> --- a/src/gallium/drivers/radeon/radeon_vce.c
> +++ b/src/gallium/drivers/radeon/radeon_vce.c
> @@ -232,21 +232,21 @@ void rvce_frame_offset(struct rvce_encoder *enc, struct rvce_cpb_slot *slot,
>   }
>   
>   /**
>    * destroy this video encoder
>    */
>   static void rvce_destroy(struct pipe_video_codec *encoder)
>   {
>   	struct rvce_encoder *enc = (struct rvce_encoder*)encoder;
>   	if (enc->stream_handle) {
>   		struct rvid_buffer fb;
> -		rvid_create_buffer(enc->screen, &fb, 512, PIPE_USAGE_STAGING);
> +		rvid_create_feedback_buffer(enc->screen, &fb);
>   		enc->fb = &fb;
>   		enc->session(enc);
>   		enc->feedback(enc);
>   		enc->destroy(enc);
>   		flush(enc);
>   		rvid_destroy_buffer(&fb);
>   	}
>   	rvid_destroy_buffer(&enc->cpb);
>   	enc->ws->cs_destroy(enc->cs);
>   	FREE(enc->cpb_array);
> @@ -275,21 +275,21 @@ static void rvce_begin_frame(struct pipe_video_codec *encoder,
>   
>   	if (pic->picture_type == PIPE_H264_ENC_PICTURE_TYPE_IDR)
>   		reset_cpb(enc);
>   	else if (pic->picture_type == PIPE_H264_ENC_PICTURE_TYPE_P ||
>   	         pic->picture_type == PIPE_H264_ENC_PICTURE_TYPE_B)
>   		sort_cpb(enc);
>   	
>   	if (!enc->stream_handle) {
>   		struct rvid_buffer fb;
>   		enc->stream_handle = rvid_alloc_stream_handle();
> -		rvid_create_buffer(enc->screen, &fb, 512, PIPE_USAGE_STAGING);
> +		rvid_create_feedback_buffer(enc->screen, &fb);
>   		enc->fb = &fb;
>   		enc->session(enc);
>   		enc->create(enc);
>   		enc->config(enc);
>   		enc->feedback(enc);
>   		flush(enc);
>   		//dump_feedback(enc, &fb);
>   		rvid_destroy_buffer(&fb);
>   		need_rate_control = false;
>   	}
> @@ -304,21 +304,21 @@ static void rvce_begin_frame(struct pipe_video_codec *encoder,
>   static void rvce_encode_bitstream(struct pipe_video_codec *encoder,
>   				  struct pipe_video_buffer *source,
>   				  struct pipe_resource *destination,
>   				  void **fb)
>   {
>   	struct rvce_encoder *enc = (struct rvce_encoder*)encoder;
>   	enc->get_buffer(destination, &enc->bs_handle, NULL);
>   	enc->bs_size = destination->width0;
>   
>   	*fb = enc->fb = CALLOC_STRUCT(rvid_buffer);
> -	if (!rvid_create_buffer(enc->screen, enc->fb, 512, PIPE_USAGE_STAGING)) {
> +	if (!rvid_create_feedback_buffer(enc->screen, enc->fb)) {
>   		RVID_ERR("Can't create feedback buffer.\n");
>   		return;
>   	}
>   	if (!radeon_emitted(enc->cs, 0))
>   		enc->session(enc);
>   	enc->encode(enc);
>   	enc->feedback(enc);
>   }
>   
>   static void rvce_end_frame(struct pipe_video_codec *encoder,
> diff --git a/src/gallium/drivers/radeon/radeon_video.c b/src/gallium/drivers/radeon/radeon_video.c
> index d7c5a16..f60ae05 100644
> --- a/src/gallium/drivers/radeon/radeon_video.c
> +++ b/src/gallium/drivers/radeon/radeon_video.c
> @@ -65,20 +65,32 @@ bool rvid_create_buffer(struct pipe_screen *screen, struct rvid_buffer *buffer,
>   			unsigned size, unsigned usage)
>   {
>   	memset(buffer, 0, sizeof(*buffer));
>   	buffer->usage = usage;
>   	buffer->res = (struct r600_resource *)
>   		pipe_buffer_create(screen, PIPE_BIND_CUSTOM, usage, size);
>   
>   	return buffer->res != NULL;
>   }
>   
> +bool rvid_create_feedback_buffer(struct pipe_screen *screen,
> +				 struct rvid_buffer *buffer)
> +{
> +	/* The kernel's CS checker asks for at least 4KB space.
> +	 *
> +	 * TODO If we update the kernel checker to be satisfied with less,
> +	 * we could save some memory here (since the sub-allocator could be
> +	 * used).
> +	 */
> +	return rvid_create_buffer(screen, buffer, 4096, PIPE_USAGE_STAGING);
> +}
> +
>   /* destroy a buffer */
>   void rvid_destroy_buffer(struct rvid_buffer *buffer)
>   {
>   	r600_resource_reference(&buffer->res, NULL);
>   }
>   
>   /* reallocate a buffer, preserving its content */
>   bool rvid_resize_buffer(struct pipe_screen *screen, struct radeon_winsys_cs *cs,
>   			struct rvid_buffer *new_buf, unsigned new_size)
>   {
> diff --git a/src/gallium/drivers/radeon/radeon_video.h b/src/gallium/drivers/radeon/radeon_video.h
> index 39305b4..76c5f30 100644
> --- a/src/gallium/drivers/radeon/radeon_video.h
> +++ b/src/gallium/drivers/radeon/radeon_video.h
> @@ -47,20 +47,23 @@ struct rvid_buffer
>   	struct r600_resource	*res;
>   };
>   
>   /* generate an stream handle */
>   unsigned rvid_alloc_stream_handle(void);
>   
>   /* create a buffer in the winsys */
>   bool rvid_create_buffer(struct pipe_screen *screen, struct rvid_buffer *buffer,
>   			unsigned size, unsigned usage);
>   
> +bool rvid_create_feedback_buffer(struct pipe_screen *screen,
> +				 struct rvid_buffer *buffer);
> +
>   /* destroy a buffer */
>   void rvid_destroy_buffer(struct rvid_buffer *buffer);
>   
>   /* reallocate a buffer, preserving its content */
>   bool rvid_resize_buffer(struct pipe_screen *screen, struct radeon_winsys_cs *cs,
>   			struct rvid_buffer *new_buf, unsigned new_size);
>   
>   /* clear the buffer with zeros */
>   void rvid_clear_buffer(struct pipe_context *context, struct rvid_buffer* buffer);
>