[Mesa-dev] [PATCH 1/3] gallium/st: add pipe_context::generate_mipmap()

[Michel Dänzer]

On 13.01.2016 04:20, Charmaine Lee wrote:
> 
>> From: Michel Dänzer <michel at daenzer.net>
>> Sent: Monday, January 11, 2016 11:37 PM
>> To: Charmaine Lee
>> Cc: mesa-dev at lists.freedesktop.org
>> Subject: Re: [Mesa-dev] [PATCH 1/3] gallium/st: add pipe_context::generate_mipmap()
> 
>> On 12.01.2016 15:18, Charmaine Lee wrote:
>>> This patch adds a new interface to support hardware mipmap generation.
>>> PIPE_CAP_GENERATE_MIPMAP is added to allow a driver to specify
>>> if this new interface is supported; if not supported, the state tracker will
>>> fallback to mipmap generation by rendering/texturing.
>>>
>>> v2: add PIPE_CAP_GENERATE_MIPMAP to the disabled section for all drivers
> 
>> [...]
> 
>>> diff --git a/src/gallium/drivers/trace/tr_context.c b/src/gallium/drivers/trace/tr_context.c
>>> index d4c88c9..17da64e 100644
>>> --- a/src/gallium/drivers/trace/tr_context.c
>>> +++ b/src/gallium/drivers/trace/tr_context.c
>>> @@ -1292,6 +1292,35 @@ trace_context_flush(struct pipe_context *_pipe,
>>>
>>>
>>>  static inline void
>>> +trace_context_generate_mipmap(struct pipe_context *_pipe,
>>> +                              struct pipe_resource *res,
>>> +                              unsigned base_level,
>>> +                              unsigned last_level,
>>> +                              unsigned first_layer,
>>> +                              unsigned last_layer)
>>> +{
>>> +   struct trace_context *tr_ctx = trace_context(_pipe);
>>> +   struct pipe_context *pipe = tr_ctx->pipe;
>>> +
>>> +   res = trace_resource_unwrap(tr_ctx, res);
>>> +
>>> +   trace_dump_call_begin("pipe_context", "generate_mipmap");
>>> +
>>> +   trace_dump_arg(ptr, pipe);
>>> +   trace_dump_arg(ptr, res);
>>> +
>>> +   trace_dump_arg(uint, base_level);
>>> +   trace_dump_arg(uint, last_level);
>>> +   trace_dump_arg(uint, first_layer);
>>> +   trace_dump_arg(uint, last_layer);
>>> +
>>> +   pipe->generate_mipmap(pipe, res, base_level, last_level, first_layer, last_layer);
>>> +
>>> +   trace_dump_call_end();
>>> +}
>>> +
>>> +
>>> +static inline void
>>>  trace_context_destroy(struct pipe_context *_pipe)
>>>  {
>>>     struct trace_context *tr_ctx = trace_context(_pipe);
> 
>> [...]
> 
>>> diff --git a/src/gallium/include/pipe/p_context.h b/src/gallium/include/pipe/p_context.h
>>> index be7447d..a0774c7 100644
>>> --- a/src/gallium/include/pipe/p_context.h
>>> +++ b/src/gallium/include/pipe/p_context.h
>>> @@ -673,6 +673,17 @@ struct pipe_context {
>>>      */
>>>     void (*dump_debug_state)(struct pipe_context *ctx, FILE *stream,
>>>                              unsigned flags);
>>> +
>>> +   /**
>>> +    * Generate mipmap.
>>> +    * \return TRUE if mipmap generation succeeds, FALSE otherwise
>>> +    */
>>> +   boolean (*generate_mipmap)(struct pipe_context *ctx,
>>> +                              struct pipe_resource *resource,
>>> +                              unsigned base_level,
>>> +                              unsigned last_level,
>>> +                              unsigned first_layer,
>>> +                              unsigned last_layer);
>>>  };
> 
>> Maybe I'm missing something, but the prototypes of the pipe_context
>> field and trace driver implementation don't match.
> 
> Hmm. I don't know what you mean. They look correct to me.

The return types differ, void vs. boolean.


>>> diff --git a/src/mesa/state_tracker/st_gen_mipmap.c b/src/mesa/state_tracker/st_gen_mipmap.c
>>> index b370040..94c1171 100644
>>> --- a/src/mesa/state_tracker/st_gen_mipmap.c
>>> +++ b/src/mesa/state_tracker/st_gen_mipmap.c
>>> @@ -149,12 +149,19 @@ st_generate_mipmap(struct gl_context *ctx, GLenum target,
>>>        last_layer = util_max_layer(pt, baseLevel);
>>>     }
>>>
>>> -   /* Try to generate the mipmap by rendering/texturing.  If that fails,
>>> -    * use the software fallback.
>>> +   /* First see if the driver supports hardware mipmap generation,
>>> +    * if not then generate the mipmap by rendering/texturing.
>>> +    * If that fails, use the software fallback.
>>>      */
>>> -   if (!util_gen_mipmap(st->pipe, pt, pt->format, baseLevel, lastLevel,
>>> -                        first_layer, last_layer, PIPE_TEX_FILTER_LINEAR)) {
>>> -      _mesa_generate_mipmap(ctx, target, texObj);
>>> +   if (!st->pipe->screen->get_param(st->pipe->screen,
>>> +                                    PIPE_CAP_GENERATE_MIPMAP) ||
>>> +       !st->pipe->generate_mipmap(st->pipe, pt, baseLevel, lastLevel,
>>> +                                  first_layer, last_layer)) {
>>> +
>>> +      if (!util_gen_mipmap(st->pipe, pt, pt->format, baseLevel, lastLevel,
>>> +                           first_layer, last_layer, PIPE_TEX_FILTER_LINEAR)) {
>>> +         _mesa_generate_mipmap(ctx, target, texObj);
>>> +      }
>>>     }
> 
>> Do we really need a cap for this? Drivers not implementing this hook can
>> just leave it at NULL, then state trackers can fall back and wrapper
>> drivers can return false in that case.
> 
> My initial attempt was to check for NULL too, but Jose pointed out to me that we need a cap
> for this capability. This makes trace/svga implementation a little simpler.

I'd be interested in why that is, if either of you has the time to
enlighten me. :)


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer