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

[Roland Scheidegger]

Am 09.09.2016 um 02:19 schrieb Rob Clark:
> On Thu, Sep 8, 2016 at 7:54 PM, Rob Clark <robdclark at gmail.com> wrote:
>> On Thu, Sep 8, 2016 at 6:41 PM, Roland Scheidegger <sroland at vmware.com> wrote:
>>> 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.
>>>
>>
>> It would require a *lot* of changes to change st_texture_object::pt
>> into an array in mesa/st, I think.. plus a bunch of re-working the
>> egl-img code in mesa/st.. that sounds like a much worse option to me.
>>
>> Having a 'void *' opaque pointer in the resource (rather than a
>> 'struct pipe_resource *next') for the st to do whatever it wants with
>> seems semi-sane to me.  And plausibly useful to other st's as well.
>>
>> *however* making it an opaque ptr (or even handling it purely in
>> mesa/st) seems like a slight disadvantage compared to current patch..
>> unref'ing rsc->next in pipe_resource_reference() is a nice benefit of
>> the current approach..
>>
>> At the end of the day, I'm less a fan of making this all much harder
>> for the st only for the benefit of some hypothetical API "purity" ;-)
> 
> (or perhaps a better way to word that is "I'm a fan of pragmatism" ;-))
> 

Oh I'm all for pragmatism.
Yet I still care about api purity somewhat. But gallium is an interface
for hw drivers. Yet there's now this new field in pipe_resource which
the drivers should ignore - i.e. it's only really there for the state
tracker, it serves no purpose for the driver seemingly. Therefore it
shouldn't be there. Essentially you've hidden all the nasty
multi-sampler and color conversion stuff from the drivers, yet somehow
the resources themselves are still different.
Maybe I'm naive but why would it be more work to put that into the
appropriate state tracker structure instead?

Roland