[Mesa-dev] [PATCH 1/4] r600_state_common: check NULL return from u_upload_alloc

[Nicolai Hähnle]

On 24.03.2017 13:11, Julien Isorce wrote:
> 1: Ack, thx.
> 2: u_upload_alloc can fail, i.e. set output param *ptr to NULL, for 2
> reasons: alloc fails or map fails. For both there are already a
> fprintf/stderr, i.e., in radeon_create_bo and radeon_bo_do_map .
> It looks to me that in src/gallium/drivers/ it is a common usage to just
> avoid to crash by doing a silent check. But it defers the fprintf to
> where the error comes from, i.e. libdrm calls.

Okay, that makes sense.


> Also I think that if drmCommandWriteRead returns -ENOMEM it should be
> translated to _mesa_error_no_memory(__func__); at some point. But it
> seems it is not possible to use _mesa_error_no_memory in
> mesa/src/gallium/winsys (though it is possible from
> gallium/state_trackers/glx). And it would require to be in the gl
> context's thread to call it.

Unfortunately, that doesn't work because the driver could be called from 
another state tracker (video APIs, nine, opencl).

Although, we could add a way for the state tracker to register an 
out-of-memory callback function, similar to the debug callback.

Also, I remember thinking at one point that in si_state_draw.c, it might 
be helpful to just flag out-of-memory conditions in a context-wide bool 
which is checked once before the actual draw commands are written. This 
would reduce the need to bubble up error conditions.

Cheers,
Nicolai

>
> Regards
> Julien
>
>
> On 24 March 2017 at 11:19, Nicolai Hähnle <nhaehnle at gmail.com
> <mailto:nhaehnle at gmail.com>> wrote:
>
>     I generally like the patches in the series, thanks for that. Two
>     points though:
>
>     1. The order of patches in series is usually general code before
>     driver code, i.e. the st/cb_bitmap should come first.
>
>     2. I don't like having silent errors, as that could be confusing. In
>     places where the error isn't propagated to the application (as it
>     really should be...), I think we should have an fprintf to stderr.
>
>     Cheers,
>     Nicolai
>
>     On 24.03.2017 12:08, Julien Isorce wrote:
>
>         Like done in si_state_draw.c::si_draw_vbo
>
>         Signed-off-by: Julien Isorce <jisorce at oblong.com
>         <mailto:jisorce at oblong.com>>
>         ---
>          src/gallium/drivers/r600/r600_state_common.c | 4 ++++
>          1 file changed, 4 insertions(+)
>
>         diff --git a/src/gallium/drivers/r600/r600_state_common.c
>         b/src/gallium/drivers/r600/r600_state_common.c
>         index 6f8279f..cedeb74 100644
>         --- a/src/gallium/drivers/r600/r600_state_common.c
>         +++ b/src/gallium/drivers/r600/r600_state_common.c
>         @@ -1746,6 +1746,10 @@ static void r600_draw_vbo(struct
>         pipe_context *ctx, const struct pipe_draw_info
>
>                                 u_upload_alloc(ctx->stream_uploader,
>         start, count * 2,
>                                                 256, &out_offset,
>         &out_buffer, &ptr);
>         +                       if (unlikely(!ptr)) {
>         +
>          pipe_resource_reference(&ib.buffer, NULL);
>         +                               return;
>         +                       }
>
>                                 util_shorten_ubyte_elts_to_userptr(
>                                                         &rctx->b.b, &ib,
>         0, 0, ib.offset + start, count, ptr);
>
>
>
>     --
>     Lerne, wie die Welt wirklich ist,
>     Aber vergiss niemals, wie sie sein sollte.
>
>


-- 
Lerne, wie die Welt wirklich ist,
Aber vergiss niemals, wie sie sein sollte.