[Mesa-dev] [PATCH 03/13] gallium/util: add threaded_context as a pipe_context wrapper

[Marek Olšák]

[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;
>> +   }

[snip]
>> +         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.

It's necessary because this codepath is also used by textures where
tres->latest == NULL because it's not initialized explicitly.

[snip]
>> +
>> +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...

True, but the Gallium interface is currently defined such that drivers do
it.

Marek
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170512/53ffa995/attachment-0001.html>