[Mesa-dev] [PATCH] radeonsi: fix PIPE_FORMAT_R11G11B10_FLOAT handling

[Jakob Sinclair]

On 2016-04-29 23:21, Nicolai Hähnle wrote:
> From: Nicolai Hähnle <nicolai.haehnle at amd.com>
> 
> That format has first_non_void < 0. This fixes a regression in piglit
> arb_shader_image_load_store-semantics that was introduced by commit 
> 76b8c5cc602,
> while hopefully still shutting Coverity up (and failing in a more 
> obvious way
> if a similar error should re-appear).
> ---
>  src/gallium/drivers/radeonsi/si_state.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/src/gallium/drivers/radeonsi/si_state.c
> b/src/gallium/drivers/radeonsi/si_state.c
> index 1dd468b..16c941e 100644
> --- a/src/gallium/drivers/radeonsi/si_state.c
> +++ b/src/gallium/drivers/radeonsi/si_state.c
> @@ -1760,18 +1760,18 @@ static uint32_t
> si_translate_buffer_dataformat(struct pipe_screen *screen,
>  					       const struct util_format_description *desc,
>  					       int first_non_void)
>  {
> -	if (first_non_void < 0)
> -		return V_008F0C_BUF_DATA_FORMAT_INVALID;
> -
> -	unsigned type = desc->channel[first_non_void].type;
> +	unsigned type;
>  	int i;
> 
> -	if (type == UTIL_FORMAT_TYPE_FIXED)
> -		return V_008F0C_BUF_DATA_FORMAT_INVALID;
> -
>  	if (desc->format == PIPE_FORMAT_R11G11B10_FLOAT)
>  		return V_008F0C_BUF_DATA_FORMAT_10_11_11;
> 
> +	assert(first_non_void >= 0);
> +	type = desc->channel[first_non_void].type;
> +
> +	if (type == UTIL_FORMAT_TYPE_FIXED)
> +		return V_008F0C_BUF_DATA_FORMAT_INVALID;
> +
>  	if (desc->nr_channels == 4 &&
>  	    desc->channel[0].size == 10 &&
>  	    desc->channel[1].size == 10 &&
> @@ -1837,9 +1837,11 @@ static uint32_t
> si_translate_buffer_numformat(struct pipe_screen *screen,
>  					      const struct util_format_description *desc,
>  					      int first_non_void)
>  {
> -	if (desc->format == PIPE_FORMAT_R11G11B10_FLOAT || first_non_void < 
> 0)
> +	if (desc->format == PIPE_FORMAT_R11G11B10_FLOAT)
>  		return V_008F0C_BUF_NUM_FORMAT_FLOAT;
> 
> +	assert(first_non_void >= 0);
> +
>  	switch (desc->channel[first_non_void].type) {
>  	case UTIL_FORMAT_TYPE_SIGNED:
>  		if (desc->channel[first_non_void].normalized)

Ahh, I should have been more careful with testing the patch. Anyways 
this looks all good to me. By the fact that you are using assert I'm 
guessing that first_non_void shouldn't be negative very often and that 
when it is negative something has gone horribly wrong. If that's the 
case then:

Reviewed-by: Jakob Sinclair <sinclair.jakob at openmailbox.org>
-- 
Mvh Jakob Sinclair