On 01/15/2014 07:11 PM, Roland Scheidegger wrote:
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).
OK, I'll put in a loop.
/* * 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.
I could add the zero-ing code. I did so elsewhere too just to be safe. So let's be consistent.
We should also set layer_stride=0 for non-texture resources below that, just to be safe.
+ 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.
Yeah, that's wrong. But another questions is: if all the cbufs[] happen to be NULL, then max_layer will be ~0. Is that what we want, or don't we care?
I'll post v2 in a minute... -Brian _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
