[Mesa-dev] [PATCH 3/7] mesa/st: support lowering multi-planar YUV

[Roland Scheidegger]

Am 08.09.2016 um 23:43 schrieb Rob Clark:
> On Thu, Sep 8, 2016 at 5:11 PM, Roland Scheidegger <sroland at vmware.com> wrote:
>> Am 08.09.2016 um 22:30 schrieb Rob Clark:
>>> Support multi-planar YUV for external EGLImage's (currently just in the
>>> dma-buf import path) by lowering to multiple texture fetch's for each
>>> plane and CSC in shader.
>>>
>>> Signed-off-by: Rob Clark <robdclark at gmail.com>
>>> ---
>>>  src/gallium/auxiliary/util/u_inlines.h      |   4 +-
>>>  src/gallium/include/pipe/p_state.h          |   9 +++
>>>  src/gallium/include/state_tracker/st_api.h  |   3 +
>>>  src/gallium/state_trackers/dri/dri2.c       | 119 +++++++++++++++++++++++-----
>>>  src/gallium/state_trackers/dri/dri_screen.c |  11 +++
>>>  src/mesa/main/mtypes.h                      |  16 ++++
>>>  src/mesa/program/ir_to_mesa.cpp             |   1 +
>>>  src/mesa/state_tracker/st_atom_sampler.c    |  41 +++++++++-
>>>  src/mesa/state_tracker/st_atom_shader.c     |   3 +
>>>  src/mesa/state_tracker/st_atom_texture.c    |  58 ++++++++++++++
>>>  src/mesa/state_tracker/st_cb_eglimage.c     |  18 +++++
>>>  src/mesa/state_tracker/st_context.c         |   7 +-
>>>  src/mesa/state_tracker/st_glsl_to_nir.cpp   |   1 +
>>>  src/mesa/state_tracker/st_glsl_to_tgsi.cpp  |   4 +
>>>  src/mesa/state_tracker/st_manager.c         |   1 +
>>>  src/mesa/state_tracker/st_program.c         |  35 ++++++++
>>>  src/mesa/state_tracker/st_program.h         |  37 +++++++++
>>>  src/mesa/state_tracker/st_texture.h         |  21 +++++
>>>  18 files changed, 362 insertions(+), 27 deletions(-)
>>>
>>> diff --git a/src/gallium/auxiliary/util/u_inlines.h b/src/gallium/auxiliary/util/u_inlines.h
>>> index c2a0b08..b7b8313 100644
>>> --- a/src/gallium/auxiliary/util/u_inlines.h
>>> +++ b/src/gallium/auxiliary/util/u_inlines.h
>>> @@ -136,8 +136,10 @@ pipe_resource_reference(struct pipe_resource **ptr, struct pipe_resource *tex)
>>>     struct pipe_resource *old_tex = *ptr;
>>>
>>>     if (pipe_reference_described(&(*ptr)->reference, &tex->reference,
>>> -                                (debug_reference_descriptor)debug_describe_resource))
>>> +                                (debug_reference_descriptor)debug_describe_resource)) {
>>> +      pipe_resource_reference(&old_tex->next, NULL);
>>>        old_tex->screen->resource_destroy(old_tex->screen, old_tex);
>>> +   }
>>>     *ptr = tex;
>>>  }
>>>
>>> diff --git a/src/gallium/include/pipe/p_state.h b/src/gallium/include/pipe/p_state.h
>>> index ebd0337..4a88da6 100644
>>> --- a/src/gallium/include/pipe/p_state.h
>>> +++ b/src/gallium/include/pipe/p_state.h
>>> @@ -498,6 +498,15 @@ struct pipe_resource
>>>
>>>     unsigned bind;            /**< bitmask of PIPE_BIND_x */
>>>     unsigned flags;           /**< bitmask of PIPE_RESOURCE_FLAG_x */
>>> +
>>> +   /**
>>> +    * For planar images, ie. YUV EGLImage external, etc, pointer to the
>>> +    * next plane.
>>> +    *
>>> +    * TODO might be useful for dealing w/ z32s8 too, since at least a
>>> +    * couple drivers split these out into separate buffers internally.
>>> +    */
>>> +   struct pipe_resource *next;
>> Would it be possible to stuff the multiple resources somewhere else
>> (__DRIImage ?)? Seems a bit of a hack to have resources referencing
>> other resources that way.
>> (Also, it's odd since things are mostly lowered really outside of
>> gallium so it's odd that some of the yuv state still sneaks in there.)
> 
> I did originally start down the path of making __DRIImage have
> multiple pipe_resource's.. I'm not really sure that would end up
> better, and it certainly would be more invasive.
> 
> Maybe we should just make that something like 'void *stpriv' to let st
> stick whatever it wants in there.  That seems more sane than making
> the st use a hashtable to map the rsc back to something else.
Can't you just put 3 resources in somewhere without pointers?
I just think it really should be outside gallium interfaces. The
lowering is all done by the state tracker, hence having those bits there
referencing other resources in gallium looks wrong to me.


> 
> One note I would make, is that I think at least both radeon and
> freedreno (and maybe others) already do similar things (in driver
> backend) for formats like z32_x24s8, since from hw PoV, they are
> actually two separate buffers, while from GL and gallium API they are
> conceptually a single buffer.  Having a chain of resources, like I did
> for planar YUV, seems like a reasonable approach if we ever wanted to
> refactor some of that duplicated logic into mesa/st.  (Not that it is
> high on my todo list.. just pointing out there are other cases where
> we want to treat multiple buffers as one logical buffer.)
Honestly, I don't think that's a good idea. Unless I'm mistaken even for
things like z24s8 some hw has separate buffers, and drivers should (and
do) handle that internally. This is just something which can't be
abstracted away at the interface level reasonably, and drivers just need
to deal with it (hopefully you don't have to export that but such
buffers typically aren't really in any standardized layout anyway).
(Couldn't actually in GL be separate buffers for depth and stencil? At
least there used to be two attachment points.)

Roland