[Mesa-dev] [PATCH 2/2] llvmpipe: handle NULL color buffer pointers

[Brian Paul]

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