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

[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" ;-))

BR,
-R