<div dir="ltr"><div>[snip] >> +static unsigned >> +tc_improve_map_buffer_flags(struct threaded_context *tc, >> + struct threaded_resource *tres, unsigned >> usage, >> + unsigned offset, unsigned size) >> +{ >> + /* Handle CPU reads trivially. */ >> + if (usage & PIPE_TRANSFER_READ) { >> + /* Driver aren't allowed to do buffer invalidations. */ >> + return (usage & ~PIPE_TRANSFER_DISCARD_WHOLE_RESOURCE) | >> + TC_TRANSFER_MAP_NO_INVALIDATE | >> + TC_TRANSFER_MAP_IGNORE_VALID_RANGE; >> + } >> + >> + /* Sparse buffers can't be mapped directly. Use a staging buffer. */ >> + if (tres->b.flags & PIPE_RESOURCE_FLAG_SPARSE) { >> + return (usage & ~(PIPE_TRANSFER_DISCARD_WHOLE_RESOURCE | >> + PIPE_TRANSFER_UNSYNCHRONIZED)) | >> + PIPE_TRANSFER_DISCARD_RANGE | > > > Why are we allowed to discard here? Even when a range is mapped only for > writing, we can't just assume that the whole range will be written by the > application. > > Also, why do we clear the unsynchronized flag? As I understand the code, we > do need to synchronize the threads because we're going to use a staging > buffer. But theoretically, if the driver had a way to do the staging copy > without waiting for previously submitted draws, it could do so. So... I > don't think it has a visible effect right now, but I'd rather not remove the > unsynchronized flag here. > > This may need a bit of clarification in the big comment in the header file. That code was indeed completely wrong. I've changed it to this and I moved this block to the beginning of the function (before the READ handling): /* Sparse buffers can't be mapped directly and can't be reallocated * (fully invalidated). That may just be a radeonsi limitation, but * the threaded context must obey it with radeonsi. */ if (tres->b.flags & PIPE_RESOURCE_FLAG_SPARSE) { /* We can use DISCARD_RANGE instead of full discard. This is the only * fast path for sparse buffers that doesn't need thread synchronization. */ if (usage & PIPE_TRANSFER_DISCARD_WHOLE_RESOURCE) usage |= PIPE_TRANSFER_DISCARD_RANGE; /* Allow DISCARD_WHOLE_RESOURCE and infering UNSYNCHRONIZED in drivers. * The threaded context doesn't do unsychronized mappings and invalida- * tions of sparse buffers, therefore a correct driver behavior won't * result in an incorrect behavior with the threaded context. */ return usage; } > > > >> + TC_TRANSFER_MAP_NO_INVALIDATE | >> + TC_TRANSFER_MAP_IGNORE_VALID_RANGE; >> + } </div> [snip] <div>>> + ttrans->b.level = 0; >> + ttrans->b.usage = usage; >> + ttrans->b.box = *box; >> + ttrans->b.stride = 0; >> + ttrans->b.layer_stride = 0; >> + *transfer = &ttrans->b; >> + return map + (box->x % tc->map_buffer_alignment); >> + } >> + } >> + >> + /* Unsychronized buffer mappings don't have to synchronize the thread. >> */ >> + if (!(usage & TC_TRANSFER_MAP_THREADED_UNSYNC)) >> + tc_sync_msg(tc, resource->target != PIPE_BUFFER ? " texture" : >> + usage & PIPE_TRANSFER_DISCARD_RANGE ? " >> discard_range" : >> + usage & PIPE_TRANSFER_READ ? " read" : " ??"); >> + >> + return pipe->transfer_map(pipe, tres->latest ? tres->latest : >> resource, >> + level, usage, box, transfer); > > > The ternary operator here should be unnecessary -- tres->latest should > always be non-NULL. </div><div>It's necessary because this codepath is also used by textures where tres->latest == NULL because it's not initialized explicitly. </div><div> </div><div>[snip] </div><div>>> + >> +static boolean >> +tc_generate_mipmap(struct pipe_context *_pipe, >> + struct pipe_resource *res, >> + enum pipe_format format, >> + unsigned base_level, >> + unsigned last_level, >> + unsigned first_layer, >> + unsigned last_layer) >> +{ >> + struct threaded_context *tc = threaded_context(_pipe); >> + struct pipe_context *pipe = tc->pipe; >> + struct pipe_screen *screen = pipe->screen; >> + unsigned bind = PIPE_BIND_SAMPLER_VIEW; >> + >> + if (util_format_is_depth_or_stencil(format)) >> + bind = PIPE_BIND_DEPTH_STENCIL; >> + else >> + bind = PIPE_BIND_RENDER_TARGET; >> + >> + if (!screen->is_format_supported(screen, format, res->target, >> + res->nr_samples, bind)) >> + return false; > > > This feels like the kind of thing the state tracker should be checking > before it calls this function... </div><div>True, but the Gallium interface is currently defined such that drivers do it. </div><div>Marek</div> </div>