[Mesa-dev] [PATCH 3/7] mesa/st: support lowering multi-planar YUV
Rob Clark
robdclark at gmail.com
Thu Sep 8 23:54:42 UTC 2016
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" ;-)
>
>>
>> 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.)
well, this is getting a bit onto a different topic.. but once we start
implementing the same API workarounds in multiple driver backends, imo
it is maybe worth thinking about alternatives. It seems reasonable to
allow the driver to support separate z32 and x24s8 formats but not
z32_z24s8, and in that case have the logic to copy to a temp staging
buffer for the benefit of transfer_map implemented once rather than
per-driver.. either in the st or via some aux/util helpers perhaps.
I don't think it is a problem to abstract away since it really only
effects two places.. pipe_framebuffer_state, and transfer_map.
Anyways, perhaps not worth worrying about until the n+1 driver comes
along and needs to implement the same logic yet again in transfer_map
;-)
BR,
-R
> Roland
>
More information about the mesa-dev
mailing list