[Mesa-dev] [PATCH 2/2] llvmpipe: handle NULL color buffer pointers
Roland Scheidegger
sroland at vmware.com
Wed Jan 15 19:11:22 PST 2014
Looks good overall, just some minor quibbles inline.
Roland
Am 16.01.2014 03:15, schrieb Brian Paul:
> Fixes regression from 9baa45f78b8ca7d66280e36009b6a685055d7cd6
> ---
> src/gallium/drivers/llvmpipe/lp_rast.c | 44 +++++---
> src/gallium/drivers/llvmpipe/lp_rast_priv.h | 11 +-
> src/gallium/drivers/llvmpipe/lp_scene.c | 6 +-
> src/gallium/drivers/llvmpipe/lp_setup.c | 2 +-
> src/gallium/drivers/llvmpipe/lp_state_fs.c | 152 +++++++++++++++------------
> 5 files changed, 126 insertions(+), 89 deletions(-)
>
> diff --git a/src/gallium/drivers/llvmpipe/lp_rast.c b/src/gallium/drivers/llvmpipe/lp_rast.c
> index 6feec94..3e25ff0 100644
> --- a/src/gallium/drivers/llvmpipe/lp_rast.c
> +++ b/src/gallium/drivers/llvmpipe/lp_rast.c
> @@ -124,7 +124,8 @@ lp_rast_clear_color(struct lp_rasterizer_task *task,
> unsigned i;
> union util_color uc;
>
> - if (util_format_is_pure_integer(scene->fb.cbufs[0]->format)) {
> + if (scene->fb.cbufs[0] &&
> + util_format_is_pure_integer(scene->fb.cbufs[0]->format)) {
This isn't quite correct, since if the first fb is NULL but the second
is a pure int you'd end up in the non pure int code path below. Maybe
would be easier if just iterating over all fbs in the outermost loop and
doing that decision per fb (this only worked because mixed int/fixed fbs
weren't allowed, but it should be quite ok deciding that per fb anyway).
> /*
> * We expect int/uint clear values here, though some APIs
> * might disagree (but in any case util_pack_color()
> @@ -174,20 +175,22 @@ lp_rast_clear_color(struct lp_rasterizer_task *task,
> clear_color[3]);
>
> for (i = 0; i < scene->fb.nr_cbufs; i++) {
> - util_pack_color(arg.clear_color.f,
> - scene->fb.cbufs[i]->format, &uc);
> -
> - util_fill_box(scene->cbufs[i].map,
> - scene->fb.cbufs[i]->format,
> - scene->cbufs[i].stride,
> - scene->cbufs[i].layer_stride,
> - task->x,
> - task->y,
> - 0,
> - task->width,
> - task->height,
> - scene->fb_max_layer + 1,
> - &uc);
> + if (scene->fb.cbufs[i]) {
> + util_pack_color(arg.clear_color.f,
> + scene->fb.cbufs[i]->format, &uc);
> +
> + util_fill_box(scene->cbufs[i].map,
> + scene->fb.cbufs[i]->format,
> + scene->cbufs[i].stride,
> + scene->cbufs[i].layer_stride,
> + task->x,
> + task->y,
> + 0,
> + task->width,
> + task->height,
> + scene->fb_max_layer + 1,
> + &uc);
> + }
> }
> }
> }
> @@ -444,8 +447,15 @@ lp_rast_shade_quads_mask(struct lp_rasterizer_task *task,
>
> /* color buffer */
> for (i = 0; i < scene->fb.nr_cbufs; i++) {
> - stride[i] = scene->cbufs[i].stride;
> - color[i] = lp_rast_get_unswizzled_color_block_pointer(task, i, x, y, inputs->layer);
> + if (scene->fb.cbufs[i]) {
> + stride[i] = scene->cbufs[i].stride;
> + color[i] = lp_rast_get_unswizzled_color_block_pointer(task, i, x, y,
> + inputs->layer);
> + }
> + else {
> + stride[i] = 0;
> + color[i] = NULL;
> + }
> }
>
> /* depth buffer */
> diff --git a/src/gallium/drivers/llvmpipe/lp_rast_priv.h b/src/gallium/drivers/llvmpipe/lp_rast_priv.h
> index bc361b6..063a70e 100644
> --- a/src/gallium/drivers/llvmpipe/lp_rast_priv.h
> +++ b/src/gallium/drivers/llvmpipe/lp_rast_priv.h
> @@ -293,8 +293,15 @@ lp_rast_shade_quads_all( struct lp_rasterizer_task *task,
>
> /* color buffer */
> for (i = 0; i < scene->fb.nr_cbufs; i++) {
> - stride[i] = scene->cbufs[i].stride;
> - color[i] = lp_rast_get_unswizzled_color_block_pointer(task, i, x, y, inputs->layer);
> + if (scene->fb.cbufs[i]) {
> + stride[i] = scene->cbufs[i].stride;
> + color[i] = lp_rast_get_unswizzled_color_block_pointer(task, i, x, y,
> + inputs->layer);
> + }
> + else {
> + stride[i] = 0;
> + color[i] = NULL;
> + }
> }
>
> if (scene->zsbuf.map) {
> diff --git a/src/gallium/drivers/llvmpipe/lp_scene.c b/src/gallium/drivers/llvmpipe/lp_scene.c
> index 0296b79..134ab86 100644
> --- a/src/gallium/drivers/llvmpipe/lp_scene.c
> +++ b/src/gallium/drivers/llvmpipe/lp_scene.c
> @@ -156,6 +156,10 @@ lp_scene_begin_rasterization(struct lp_scene *scene)
>
> for (i = 0; i < scene->fb.nr_cbufs; i++) {
> struct pipe_surface *cbuf = scene->fb.cbufs[i];
> +
> + if (!cbuf)
> + continue;
I wonder if it would be a good idea to set the stride/map fields to 0
here just for some extra safety as hopefully they aren't accessed.
Though at second look it might be guaranteed they really are zero already.
> +
> if (llvmpipe_resource_is_texture(cbuf->texture)) {
> scene->cbufs[i].stride = llvmpipe_resource_stride(cbuf->texture,
> cbuf->u.tex.level);
> @@ -521,7 +525,7 @@ void lp_scene_begin_binning( struct lp_scene *scene,
> */
> for (i = 0; i < scene->fb.nr_cbufs; i++) {
> struct pipe_surface *cbuf = scene->fb.cbufs[i];
> - if (llvmpipe_resource_is_texture(cbuf->texture)) {
> + if (cbuf && llvmpipe_resource_is_texture(cbuf->texture)) {
> max_layer = MIN2(max_layer, cbuf->u.tex.last_layer - cbuf->u.tex.first_layer);
Hmm I think this can't work correctly, it there's any NULL cbuf the else
clause would be taken hence max_layer would incorrectly be set to zero.
> }
> else {
> diff --git a/src/gallium/drivers/llvmpipe/lp_setup.c b/src/gallium/drivers/llvmpipe/lp_setup.c
> index 7f22231..2437318 100644
> --- a/src/gallium/drivers/llvmpipe/lp_setup.c
> +++ b/src/gallium/drivers/llvmpipe/lp_setup.c
> @@ -882,7 +882,7 @@ lp_setup_is_resource_referenced( const struct lp_setup_context *setup,
>
> /* check the render targets */
> for (i = 0; i < setup->fb.nr_cbufs; i++) {
> - if (setup->fb.cbufs[i]->texture == texture)
> + if (setup->fb.cbufs[i] && setup->fb.cbufs[i]->texture == texture)
> return LP_REFERENCED_FOR_READ | LP_REFERENCED_FOR_WRITE;
> }
> if (setup->fb.zsbuf && setup->fb.zsbuf->texture == texture) {
> diff --git a/src/gallium/drivers/llvmpipe/lp_state_fs.c b/src/gallium/drivers/llvmpipe/lp_state_fs.c
> index eedafa3..abdb6b3 100644
> --- a/src/gallium/drivers/llvmpipe/lp_state_fs.c
> +++ b/src/gallium/drivers/llvmpipe/lp_state_fs.c
> @@ -2361,28 +2361,33 @@ generate_fragment(struct llvmpipe_context *lp,
> /* Loop over color outputs / color buffers to do blending.
> */
> for(cbuf = 0; cbuf < key->nr_cbufs; cbuf++) {
> - LLVMValueRef color_ptr;
> - LLVMValueRef stride;
> - LLVMValueRef index = lp_build_const_int32(gallivm, cbuf);
> + if (key->cbuf_format[cbuf] != PIPE_FORMAT_NONE) {
> + LLVMValueRef color_ptr;
> + LLVMValueRef stride;
> + LLVMValueRef index = lp_build_const_int32(gallivm, cbuf);
>
> - boolean do_branch = ((key->depth.enabled
> - || key->stencil[0].enabled
> - || key->alpha.enabled)
> - && !shader->info.base.uses_kill);
> + boolean do_branch = ((key->depth.enabled
> + || key->stencil[0].enabled
> + || key->alpha.enabled)
> + && !shader->info.base.uses_kill);
>
> - color_ptr = LLVMBuildLoad(builder,
> - LLVMBuildGEP(builder, color_ptr_ptr, &index, 1, ""),
> - "");
> + color_ptr = LLVMBuildLoad(builder,
> + LLVMBuildGEP(builder, color_ptr_ptr,
> + &index, 1, ""),
> + "");
>
> - lp_build_name(color_ptr, "color_ptr%d", cbuf);
> + lp_build_name(color_ptr, "color_ptr%d", cbuf);
>
> - stride = LLVMBuildLoad(builder,
> - LLVMBuildGEP(builder, stride_ptr, &index, 1, ""),
> - "");
> + stride = LLVMBuildLoad(builder,
> + LLVMBuildGEP(builder, stride_ptr, &index, 1, ""),
> + "");
>
> - generate_unswizzled_blend(gallivm, cbuf, variant, key->cbuf_format[cbuf],
> - num_fs, fs_type, fs_mask, fs_out_color,
> - context_ptr, color_ptr, stride, partial_mask, do_branch);
> + generate_unswizzled_blend(gallivm, cbuf, variant,
> + key->cbuf_format[cbuf],
> + num_fs, fs_type, fs_mask, fs_out_color,
> + context_ptr, color_ptr, stride,
> + partial_mask, do_branch);
> + }
> }
>
> LLVMBuildRetVoid(builder);
> @@ -2900,6 +2905,7 @@ make_variant_key(struct llvmpipe_context *lp,
>
> /* alpha test only applies if render buffer 0 is non-integer (or does not exist) */
> if (!lp->framebuffer.nr_cbufs ||
> + !lp->framebuffer.cbufs[0] ||
> !util_format_is_pure_integer(lp->framebuffer.cbufs[0]->format)) {
> key->alpha.enabled = lp->depth_stencil->alpha.enabled;
> }
> @@ -2927,64 +2933,74 @@ make_variant_key(struct llvmpipe_context *lp,
> }
>
> for (i = 0; i < lp->framebuffer.nr_cbufs; i++) {
> - enum pipe_format format = lp->framebuffer.cbufs[i]->format;
> struct pipe_rt_blend_state *blend_rt = &key->blend.rt[i];
> - const struct util_format_description *format_desc;
>
> - key->cbuf_format[i] = format;
> + if (lp->framebuffer.cbufs[i]) {
> + enum pipe_format format = lp->framebuffer.cbufs[i]->format;
> + const struct util_format_description *format_desc;
>
> - /*
> - * Figure out if this is a 1d resource. Note that OpenGL allows crazy
> - * mixing of 2d textures with height 1 and 1d textures, so make sure
> - * we pick 1d if any cbuf or zsbuf is 1d.
> - */
> - if (llvmpipe_resource_is_1d(lp->framebuffer.cbufs[0]->texture)) {
> - key->resource_1d = TRUE;
> - }
> + key->cbuf_format[i] = format;
>
> - format_desc = util_format_description(format);
> - assert(format_desc->colorspace == UTIL_FORMAT_COLORSPACE_RGB ||
> - format_desc->colorspace == UTIL_FORMAT_COLORSPACE_SRGB);
> + /*
> + * Figure out if this is a 1d resource. Note that OpenGL allows crazy
> + * mixing of 2d textures with height 1 and 1d textures, so make sure
> + * we pick 1d if any cbuf or zsbuf is 1d.
> + */
> + if (llvmpipe_resource_is_1d(lp->framebuffer.cbufs[i]->texture)) {
> + key->resource_1d = TRUE;
> + }
>
> - /*
> - * Mask out color channels not present in the color buffer.
> - */
> - blend_rt->colormask &= util_format_colormask(format_desc);
> + format_desc = util_format_description(format);
> + assert(format_desc->colorspace == UTIL_FORMAT_COLORSPACE_RGB ||
> + format_desc->colorspace == UTIL_FORMAT_COLORSPACE_SRGB);
>
> - /*
> - * Disable blend for integer formats.
> - */
> - if (util_format_is_pure_integer(format)) {
> - blend_rt->blend_enable = 0;
> - }
> + /*
> + * Mask out color channels not present in the color buffer.
> + */
> + blend_rt->colormask &= util_format_colormask(format_desc);
>
> - /*
> - * Our swizzled render tiles always have an alpha channel, but the linear
> - * render target format often does not, so force here the dst alpha to be
> - * one.
> - *
> - * This is not a mere optimization. Wrong results will be produced if the
> - * dst alpha is used, the dst format does not have alpha, and the previous
> - * rendering was not flushed from the swizzled to linear buffer. For
> - * example, NonPowTwo DCT.
> - *
> - * TODO: This should be generalized to all channels for better
> - * performance, but only alpha causes correctness issues.
> - *
> - * Also, force rgb/alpha func/factors match, to make AoS blending easier.
> - */
> - if (format_desc->swizzle[3] > UTIL_FORMAT_SWIZZLE_W ||
> - format_desc->swizzle[3] == format_desc->swizzle[0]) {
> - /* Doesn't cover mixed snorm/unorm but can't render to them anyway */
> - boolean clamped_zero = !util_format_is_float(format) &&
> - !util_format_is_snorm(format);
> - blend_rt->rgb_src_factor = force_dst_alpha_one(blend_rt->rgb_src_factor,
> - clamped_zero);
> - blend_rt->rgb_dst_factor = force_dst_alpha_one(blend_rt->rgb_dst_factor,
> - clamped_zero);
> - blend_rt->alpha_func = blend_rt->rgb_func;
> - blend_rt->alpha_src_factor = blend_rt->rgb_src_factor;
> - blend_rt->alpha_dst_factor = blend_rt->rgb_dst_factor;
> + /*
> + * Disable blend for integer formats.
> + */
> + if (util_format_is_pure_integer(format)) {
> + blend_rt->blend_enable = 0;
> + }
> +
> + /*
> + * Our swizzled render tiles always have an alpha channel, but the
> + * linear render target format often does not, so force here the dst
> + * alpha to be one.
> + *
> + * This is not a mere optimization. Wrong results will be produced if
> + * the dst alpha is used, the dst format does not have alpha, and the
> + * previous rendering was not flushed from the swizzled to linear
> + * buffer. For example, NonPowTwo DCT.
> + *
> + * TODO: This should be generalized to all channels for better
> + * performance, but only alpha causes correctness issues.
> + *
> + * Also, force rgb/alpha func/factors match, to make AoS blending
> + * easier.
> + */
> + if (format_desc->swizzle[3] > UTIL_FORMAT_SWIZZLE_W ||
> + format_desc->swizzle[3] == format_desc->swizzle[0]) {
> + /* Doesn't cover mixed snorm/unorm but can't render to them anyway */
> + boolean clamped_zero = !util_format_is_float(format) &&
> + !util_format_is_snorm(format);
> + blend_rt->rgb_src_factor =
> + force_dst_alpha_one(blend_rt->rgb_src_factor, clamped_zero);
> + blend_rt->rgb_dst_factor =
> + force_dst_alpha_one(blend_rt->rgb_dst_factor, clamped_zero);
> + blend_rt->alpha_func = blend_rt->rgb_func;
> + blend_rt->alpha_src_factor = blend_rt->rgb_src_factor;
> + blend_rt->alpha_dst_factor = blend_rt->rgb_dst_factor;
> + }
> + }
> + else {
> + /* no color buffer for this fragment output */
> + key->cbuf_format[i] = PIPE_FORMAT_NONE;
> + blend_rt->colormask = 0x0;
> + blend_rt->blend_enable = 0;
> }
> }
>
>
More information about the mesa-dev
mailing list