[Mesa-dev] [PATCH 1/4] r300g: Simplify destroy_context() code

[Marek Olšák]

Hi nobled,

On Fri, Dec 3, 2010 at 12:08 AM, nobled <nobled at dreamwidth.org> wrote:

> Minimize code duplication and avoid null-free in case any of
> the allocations failed.
> ---
>  src/gallium/drivers/r300/r300_context.c |   30
> +++++++++---------------------
>  1 files changed, 9 insertions(+), 21 deletions(-)
>
> diff --git a/src/gallium/drivers/r300/r300_context.c
> b/src/gallium/drivers/r300/r300_context.c
> index 2b06a24..757da60 100644
> --- a/src/gallium/drivers/r300/r300_context.c
> +++ b/src/gallium/drivers/r300/r300_context.c
> @@ -139,27 +139,11 @@
>
>     r300_update_num_contexts(r300->screen, -1);
>
> -    /* Free the structs allocated in r300_setup_atoms() */
> -    if (r300->aa_state.state) {
> -        FREE(r300->aa_state.state);
> -        FREE(r300->blend_color_state.state);
> -        FREE(r300->clip_state.state);
> -        FREE(r300->fb_state.state);
> -        FREE(r300->gpu_flush.state);
> -        FREE(r300->hyperz_state.state);
> -        FREE(r300->invariant_state.state);
> -        FREE(r300->rs_block_state.state);
> -        FREE(r300->scissor_state.state);
> -        FREE(r300->textures_state.state);
> -        FREE(r300->vap_invariant_state.state);
> -        FREE(r300->viewport_state.state);
> -        FREE(r300->ztop_state.state);
> -        FREE(r300->fs_constants.state);
> -        FREE(r300->vs_constants.state);
> -        if (!r300->screen->caps.has_tcl) {
> -            FREE(r300->vertex_stream_state.state);
> -        }
> -    }
> +    /* Free the structs allocated in r300_setup_atoms(). */
> +    foreach(atom, &r300->atom_list)
> +        if (atom->state != NULL)
> +            FREE(atom->state);
> +
>

I don't think this is correct. We should only release those atoms we
allocate manually (as is listed in the code). There are also the atoms in
"atom_list" which are set via the pipe_context::bind_* functions and should
not be released here. Instead, they should be released by a state tracker
using the appropriate delete_* functions. Moreover, free(NULL) is legal and
has no effect.

I will commit the other patches. Thanks.

Best regards
Marek



>     FREE(r300);
>  }
>
> @@ -420,11 +404,15 @@
>     r300->context.destroy = r300_destroy_context;
>
>     make_empty_list(&r300->query_list);
> +    make_empty_list(&r300->atom_list);
>
>     util_slab_create(&r300->pool_transfers,
>                      sizeof(struct pipe_transfer), 64,
>                      UTIL_SLAB_SINGLETHREADED);
>
> +    /* The above calls must not fail, since r300_destroy_context()
> +       depends on them. */
> +
>     r300->cs = rws->cs_create(rws);
>     if (r300->cs == NULL)
>         goto fail;
> --
> 1.7.0.4
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20101203/3f025f1e/attachment-0001.htm>