[Mesa-dev] [PATCH 1/2] st/va: ensure linear memory for dmabuf

[Emil Velikov]

On 30 November 2015 at 09:23, Julien Isorce <julien.isorce at gmail.com> wrote:
> On 29 November 2015 at 17:26, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>> On 27 November 2015 at 08:57, Julien Isorce <j.isorce at samsung.com> wrote:
>> > In order to do zero-copy between two different devices
>> > the memory should not be tiled.
>> >
>> > This is currently no way to set pipe_resource template's flag
>> > from pipe_video_buffer template. So disabled_tiling is added.
>> >
>> > Choosed "disable" prefix so that CALLOC keeps tiling enabled
>> > by default.
>> >
>> > Tested with GStreamer on a laptop that has 2 GPUs:
>> > 1- gstvaapidecode:
>> >    HW decoding and dmabuf export with nouveau driver on Nvidia GPU.
>> > 2- glimagesink:
>> >    EGLImage imports dmabuf on Intel GPU.
>> >
>> > Note that tiling is working if 1 and 2 are done on the same GPU.
>> > So it is up to the application to set or not the flag:
>> > VA_SURFACE_EXTBUF_DESC_ENABLE_TILING
>> >
>> > Signed-off-by: Julien Isorce <j.isorce at samsung.com>
>> > ---
>> >  src/gallium/auxiliary/vl/vl_video_buffer.c | 3 +++
>> >  src/gallium/include/pipe/p_video_codec.h   | 1 +
>> >  src/gallium/state_trackers/va/surface.c    | 5 +++++
>> >  3 files changed, 9 insertions(+)
>> >
>> > diff --git a/src/gallium/auxiliary/vl/vl_video_buffer.c
>> > b/src/gallium/auxiliary/vl/vl_video_buffer.c
>> > index 6cd2557..62f4aa9 100644
>> > --- a/src/gallium/auxiliary/vl/vl_video_buffer.c
>> > +++ b/src/gallium/auxiliary/vl/vl_video_buffer.c
>> > @@ -253,6 +253,9 @@ vl_video_buffer_template(struct pipe_resource
>> > *templ,
>> >     templ->bind = PIPE_BIND_SAMPLER_VIEW | PIPE_BIND_RENDER_TARGET;
>> >     templ->usage = usage;
>> >
>> > +   if (tmpl->disable_tiling)
>> > +      templ->bind |= PIPE_BIND_LINEAR;
>> > +
>> >     if (plane > 0) {
>> >        if (tmpl->chroma_format == PIPE_VIDEO_CHROMA_FORMAT_420) {
>> >           templ->width0 /= 2;
>> > diff --git a/src/gallium/include/pipe/p_video_codec.h
>> > b/src/gallium/include/pipe/p_video_codec.h
>> > index 196d00b..dbfffd9 100644
>> > --- a/src/gallium/include/pipe/p_video_codec.h
>> > +++ b/src/gallium/include/pipe/p_video_codec.h
>> > @@ -125,6 +125,7 @@ struct pipe_video_buffer
>> >     enum pipe_video_chroma_format chroma_format;
>> >     unsigned width;
>> >     unsigned height;
>> > +   bool disable_tiling;
>> >     bool interlaced;
>> >
>> >     /**
>> > diff --git a/src/gallium/state_trackers/va/surface.c
>> > b/src/gallium/state_trackers/va/surface.c
>> > index c052c8f..f7043ad 100644
>> > --- a/src/gallium/state_trackers/va/surface.c
>> > +++ b/src/gallium/state_trackers/va/surface.c
>> > @@ -616,6 +616,11 @@ vlVaCreateSurfaces2(VADriverContextP ctx, unsigned
>> > int format,
>> >
>> >        switch (memory_type) {
>> >        case VA_SURFACE_ATTRIB_MEM_TYPE_VA:
>> > +         /* The application will clear the TILING flag when the surface
>> > is
>> > +          * intended to be exported as dmabuf. */
>> > +         templat.disable_tiling = memory_attibute &&
>> > +            !(memory_attibute->flags &
>> > VA_SURFACE_EXTBUF_DESC_ENABLE_TILING);
>> The condition seems to be flipped, no ? Currently it's doing
>> "disable_tiling = ENABLE_TILING_BIT_SET"
>
>
> Hi Emil, thx for the review. I do not think it is flipped but maybe I
> missing something.
>
Condition is fine. Had a bit of a brain fart.

>>
>>
>> Other than that, the idea is ok imho, although I'd appreciate
>> Christian and others' feedback.
>>
>> A few things worth mentioning while looking around for this:
>>  - suface_from_external_memory should (must) also know about
>> disable_tiling.
>
> Ack
>>
>>  - missing R in function name ^^ suRface_ ...
>
> Ack
>>
>>  - sometimes radeon (see r600_video_buffer_create) completely
>> overwrites the existing flags, as opposed to just set the linear bit.
>> Bug, intentional, worth adding a comment ?
>>  - in many cases nouveau won't create a linear surface as it's not
>> using the above vl helper (hint, earlier suggesting about
>> reworking/cleaning things up a bit, hint)
>
>
> It is actually only useful to set it linear when it hits vl helper. For
> nouveau, for NV12, it uses a special layout
> (http://cgit.freedesktop.org/mesa/mesa/tree/src/gallium/drivers/nouveau/nvc0/nvc0_miptree.c#n290)
> and also vaAcquireBufferHandle (dmabuf export) is not practicable since it
> requires the memory to be contiguous. So hence not useful to optionally set
> it linear (linear flag will be ignored with existing code as you noticed)
>
I was wondering about a better way to handle this case - i.e. driver
cannot work with the specific requirements, while at the same time the
ST allows the functionality. The can think of the following 1) force
use of VL or 2) use getparam (or alike) to query if the driver and the
specific format, etc is capable and bail out accordingly.

I'm leaning towards the latter, as the driver can fall-back to VL
where appropriate.

Cheers,
Emil