[Mesa-dev] [PATCH 2/2] llvmpipe: handle NULL color buffer pointers
Roland Scheidegger
sroland at vmware.com
Thu Jan 16 09:42:16 PST 2014
Am 16.01.2014 17:19, schrieb 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?
As this should be only happening if there's no cbuf and no zsbuf
at all I think this should be ok (as the value shouldn't matter at all).
Unless there's side-effects due to the value being non-null but I think
everything should work.
Roland
>
> I'll post v2 in a minute...
>
> -Brian
More information about the mesa-dev
mailing list