[Mesa-dev] [PATCH 3/7] mesa/st: support lowering multi-planar YUV
Rob Clark
robdclark at gmail.com
Mon Sep 12 19:36:40 UTC 2016
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 ;-)
https://github.com/freedreno/mesa/commits/wip-yuv2
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..
BR,
-R
More information about the mesa-dev
mailing list