[Mesa-dev] [PATCH 3/7] mesa/st: support lowering multi-planar YUV
Roland Scheidegger
sroland at vmware.com
Tue Sep 13 01:39:57 UTC 2016
Am 12.09.2016 um 21:36 schrieb Rob Clark:
> On Sat, Sep 10, 2016 at 10:04 AM, Roland Scheidegger <sroland at vmware.com> wrote:
>> Am 09.09.2016 um 02:58 schrieb Rob Clark:
>>> On Thu, Sep 8, 2016 at 8:28 PM, Roland Scheidegger <sroland at vmware.com> wrote:
>>>> 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?
>>>
>>> I guess I would say that if we were starting from scratch, and that
>>> mesa/st (and various interfaces between it and egl/dri2 bits) didn't
>>> already exist, then I would agree with you.. probably.. although I
>>> think the case could still be made that there is some convenience in
>>> being able to treat multi-planar resources as a single thing (whether
>>> it be yuv or weird z/s formats)
>> Even if there were (and I don't really agree there) that's not _quite_
>> what this does. Multiple linked resources aren't exactly the same thing
>> as a single resource with multiple planes (obviously some parameters
>> have to match).
>>
>>>
>>> But mesa/st is already rather big, and the change isn't completely
>>> contained in mesa/st. So changing st_texture_object to encapsulate
>>> multiple resources would be much more invasive and significantly
>>> larger patchset.
>> I suppose it would need some changes so you actually get the multiple
>> resources into st_texture_object, but from an interface perspective that
>> would imho be better. You've decided the state tracker does the lowering
>> of the multi-planar yuv stuff, so it should do all of it and not do most
>> of it and leave the weirdo resources intact - I consider that a gross
>> hack and abuse of the gallium interface. But I'm not going to stop you,
>> whatever works...
>
> so, fwiw, reworking __DRIImage + st_texture_object + st_texture_image
> to have multiple resources, tacks on about another +223/-192..
> untested (so might have missed some things.. the
> st_texture_image/st_texture_object split is a bit fuzzy to me), and
> haven't dropped the pipe_resource::next bit yet. This was just to
> give a rough idea about why I went w/ pipe_resource::next approach ;-)
Oh yes that's indeed quite more churn than I thought - I didn't think
there were both st_texture_object and st_texture_image...
Maybe if there's only very few locations which need to know about the
multiple planes, you could keep stObj->pt as is and use an additional
stObj->pt23[2] or something. Albeit what you did here is cleaner.
> I guess Marek's idea gets a *bit* less intrusive on drivers if we
> limit all 2 or 3 planes to have same backing dmabuf/bo, but that is
> otherwise a restriction I'd like to not have.
>
> I still prefer the pipe_resource::next approach, but if others feel
> strongly I can re-order the patches and make the YUV patch work on top
> of the three that add multiple rsc support to
> __DRIImage/st_texture_object/st_texture_image..
I think Marek's idea to basically make those yuv planar formats
supported gallium formats has merit if it is useful outside this egl
misfeature. But just for this seems way complicated to me since as you
said all drivers would need to handle it (and transfers and such look
like needing additional work?). But the planar yuv stuff is always a
pain to handle, keeping it out of the interface imho makes more sense.
But maybe from a practical point of view it's not desirable...
Roland
More information about the mesa-dev
mailing list