[Mesa-dev] [PATCH v3 01/15] st/dri: refactor multi-planar YUV import path
Varad Gautam
varadgautam at gmail.com
Wed May 24 09:43:27 UTC 2017
Hi Lucas,
On Tue, May 23, 2017 at 9:10 PM, Lucas Stach <l.stach at pengutronix.de> wrote:
> Hi Varad,
>
> Am Dienstag, den 23.05.2017, 14:40 +0530 schrieb Varad Gautam:
>> Hi Lucas,
>>
>> On Mon, May 22, 2017 at 11:16 PM, Lucas Stach <l.stach at pengutronix.de> wrote:
>> > Am Mittwoch, den 10.05.2017, 23:15 +0530 schrieb Varad Gautam:
>> >> From: Varad Gautam <varad.gautam at collabora.com>
>> >>
>> >> we currently ignore the plane count when converting from
>> >> __DRI_IMAGE_FORMAT* tokens to __DRI_IMAGE_FOURCC* for multiplanar
>> >> images, and only return the first plane's simplified fourcc.
>> >>
>> >> this adds a fourcc to __DRI_IMAGE_FORMAT_* mapping to dri, allowing
>> >> us to return the correct fourcc format from DRIimage queries, and
>> >> simplifies the multiplane import logic.
>> >>
>> >> Signed-off-by: Varad Gautam <varad.gautam at collabora.com>
>> >> ---
>> >> src/gallium/state_trackers/dri/dri2.c | 288 +++++++++++++++-------------
>> >> src/gallium/state_trackers/dri/dri_screen.h | 13 ++
>> >> 2 files changed, 168 insertions(+), 133 deletions(-)
>> >>
>> >> diff --git a/src/gallium/state_trackers/dri/dri2.c b/src/gallium/state_trackers/dri/dri2.c
>> >> index ed6004f..0c5783c 100644
>> >> --- a/src/gallium/state_trackers/dri/dri2.c
>> >> +++ b/src/gallium/state_trackers/dri/dri2.c
>> >> @@ -52,93 +52,133 @@
>> >> #include "dri_query_renderer.h"
>> >> #include "dri2_buffer.h"
>> >>
>> >> -static int convert_fourcc(int format, int *dri_components_p)
>> >> +/* format list taken from intel_screen.c */
>> >> +static struct image_format image_formats[] = {
>> >> + { __DRI_IMAGE_FOURCC_ARGB8888, __DRI_IMAGE_COMPONENTS_RGBA, 1,
>> >> + { { 0, 0, 0, __DRI_IMAGE_FORMAT_ARGB8888, 4 } } },
>> >> +
>> >> + { __DRI_IMAGE_FOURCC_ABGR8888, __DRI_IMAGE_COMPONENTS_RGBA, 1,
>> >> + { { 0, 0, 0, __DRI_IMAGE_FORMAT_ABGR8888, 4 } } },
>> >> +
>> >> + { __DRI_IMAGE_FOURCC_SARGB8888, __DRI_IMAGE_COMPONENTS_RGBA, 1,
>> >> + { { 0, 0, 0, __DRI_IMAGE_FORMAT_SARGB8, 4 } } },
>> >> +
>> >> + { __DRI_IMAGE_FOURCC_XRGB8888, __DRI_IMAGE_COMPONENTS_RGB, 1,
>> >> + { { 0, 0, 0, __DRI_IMAGE_FORMAT_XRGB8888, 4 }, } },
>> >> +
>> >> + { __DRI_IMAGE_FOURCC_XBGR8888, __DRI_IMAGE_COMPONENTS_RGB, 1,
>> >> + { { 0, 0, 0, __DRI_IMAGE_FORMAT_XBGR8888, 4 }, } },
>> >> +
>> >> + { __DRI_IMAGE_FOURCC_ARGB1555, __DRI_IMAGE_COMPONENTS_RGBA, 1,
>> >> + { { 0, 0, 0, __DRI_IMAGE_FORMAT_ARGB1555, 2 } } },
>> >> +
>> >> + { __DRI_IMAGE_FOURCC_RGB565, __DRI_IMAGE_COMPONENTS_RGB, 1,
>> >> + { { 0, 0, 0, __DRI_IMAGE_FORMAT_RGB565, 2 } } },
>> >> +
>> >> + { __DRI_IMAGE_FOURCC_R8, __DRI_IMAGE_COMPONENTS_R, 1,
>> >> + { { 0, 0, 0, __DRI_IMAGE_FORMAT_R8, 1 }, } },
>> >> +
>> >> + { __DRI_IMAGE_FOURCC_R16, __DRI_IMAGE_COMPONENTS_R, 1,
>> >> + { { 0, 0, 0, __DRI_IMAGE_FORMAT_R16, 1 }, } },
>> >> +
>> >> + { __DRI_IMAGE_FOURCC_GR88, __DRI_IMAGE_COMPONENTS_RG, 1,
>> >> + { { 0, 0, 0, __DRI_IMAGE_FORMAT_GR88, 2 }, } },
>> >> +
>> >> + { __DRI_IMAGE_FOURCC_GR1616, __DRI_IMAGE_COMPONENTS_RG, 1,
>> >> + { { 0, 0, 0, __DRI_IMAGE_FORMAT_GR1616, 2 }, } },
>> >> +
>> >> + { __DRI_IMAGE_FOURCC_YUV410, __DRI_IMAGE_COMPONENTS_Y_U_V, 3,
>> >> + { { 0, 0, 0, __DRI_IMAGE_FORMAT_R8, 1 },
>> >> + { 1, 2, 2, __DRI_IMAGE_FORMAT_R8, 1 },
>> >> + { 2, 2, 2, __DRI_IMAGE_FORMAT_R8, 1 } } },
>> >> +
>> >> + { __DRI_IMAGE_FOURCC_YUV411, __DRI_IMAGE_COMPONENTS_Y_U_V, 3,
>> >> + { { 0, 0, 0, __DRI_IMAGE_FORMAT_R8, 1 },
>> >> + { 1, 2, 0, __DRI_IMAGE_FORMAT_R8, 1 },
>> >> + { 2, 2, 0, __DRI_IMAGE_FORMAT_R8, 1 } } },
>> >> +
>> >> + { __DRI_IMAGE_FOURCC_YUV420, __DRI_IMAGE_COMPONENTS_Y_U_V, 3,
>> >> + { { 0, 0, 0, __DRI_IMAGE_FORMAT_R8, 1 },
>> >> + { 1, 1, 1, __DRI_IMAGE_FORMAT_R8, 1 },
>> >> + { 2, 1, 1, __DRI_IMAGE_FORMAT_R8, 1 } } },
>> >> +
>> >> + { __DRI_IMAGE_FOURCC_YUV422, __DRI_IMAGE_COMPONENTS_Y_U_V, 3,
>> >> + { { 0, 0, 0, __DRI_IMAGE_FORMAT_R8, 1 },
>> >> + { 1, 1, 0, __DRI_IMAGE_FORMAT_R8, 1 },
>> >> + { 2, 1, 0, __DRI_IMAGE_FORMAT_R8, 1 } } },
>> >> +
>> >> + { __DRI_IMAGE_FOURCC_YUV444, __DRI_IMAGE_COMPONENTS_Y_U_V, 3,
>> >> + { { 0, 0, 0, __DRI_IMAGE_FORMAT_R8, 1 },
>> >> + { 1, 0, 0, __DRI_IMAGE_FORMAT_R8, 1 },
>> >> + { 2, 0, 0, __DRI_IMAGE_FORMAT_R8, 1 } } },
>> >> +
>> >> + { __DRI_IMAGE_FOURCC_YVU410, __DRI_IMAGE_COMPONENTS_Y_U_V, 3,
>> >> + { { 0, 0, 0, __DRI_IMAGE_FORMAT_R8, 1 },
>> >> + { 2, 2, 2, __DRI_IMAGE_FORMAT_R8, 1 },
>> >> + { 1, 2, 2, __DRI_IMAGE_FORMAT_R8, 1 } } },
>> >> +
>> >> + { __DRI_IMAGE_FOURCC_YVU411, __DRI_IMAGE_COMPONENTS_Y_U_V, 3,
>> >> + { { 0, 0, 0, __DRI_IMAGE_FORMAT_R8, 1 },
>> >> + { 2, 2, 0, __DRI_IMAGE_FORMAT_R8, 1 },
>> >> + { 1, 2, 0, __DRI_IMAGE_FORMAT_R8, 1 } } },
>> >> +
>> >> + { __DRI_IMAGE_FOURCC_YVU420, __DRI_IMAGE_COMPONENTS_Y_U_V, 3,
>> >> + { { 0, 0, 0, __DRI_IMAGE_FORMAT_R8, 1 },
>> >> + { 2, 1, 1, __DRI_IMAGE_FORMAT_R8, 1 },
>> >> + { 1, 1, 1, __DRI_IMAGE_FORMAT_R8, 1 } } },
>> >> +
>> >> + { __DRI_IMAGE_FOURCC_YVU422, __DRI_IMAGE_COMPONENTS_Y_U_V, 3,
>> >> + { { 0, 0, 0, __DRI_IMAGE_FORMAT_R8, 1 },
>> >> + { 2, 1, 0, __DRI_IMAGE_FORMAT_R8, 1 },
>> >> + { 1, 1, 0, __DRI_IMAGE_FORMAT_R8, 1 } } },
>> >> +
>> >> + { __DRI_IMAGE_FOURCC_YVU444, __DRI_IMAGE_COMPONENTS_Y_U_V, 3,
>> >> + { { 0, 0, 0, __DRI_IMAGE_FORMAT_R8, 1 },
>> >> + { 2, 0, 0, __DRI_IMAGE_FORMAT_R8, 1 },
>> >> + { 1, 0, 0, __DRI_IMAGE_FORMAT_R8, 1 } } },
>> >> +
>> >> + { __DRI_IMAGE_FOURCC_NV12, __DRI_IMAGE_COMPONENTS_Y_UV, 2,
>> >> + { { 0, 0, 0, __DRI_IMAGE_FORMAT_R8, 1 },
>> >> + { 1, 1, 1, __DRI_IMAGE_FORMAT_GR88, 2 } } },
>> >> +
>> >> + { __DRI_IMAGE_FOURCC_NV16, __DRI_IMAGE_COMPONENTS_Y_UV, 2,
>> >> + { { 0, 0, 0, __DRI_IMAGE_FORMAT_R8, 1 },
>> >> + { 1, 1, 0, __DRI_IMAGE_FORMAT_GR88, 2 } } },
>> >> +
>> >> + { __DRI_IMAGE_FOURCC_YUYV, __DRI_IMAGE_COMPONENTS_Y_XUXV, 2,
>> >> + { { 0, 0, 0, __DRI_IMAGE_FORMAT_GR88, 2 },
>> >> + { 0, 1, 0, __DRI_IMAGE_FORMAT_ARGB8888, 4 } } }
>> >> +};
>> >
>> > I'm not sure how this is supposed to work. This is transparently
>> > converting YUV images to RGB pipe formats, with no "behind the scenes"
>> > shader conversion, relying on the application to provide the correct
>> > conversion shader. This seems really inconsistent.
>> >
>> > Also there is no way to pass trough YUV images to the driver (Vivante
>> > GPUs support sampling from YUYV, so we would really like to see a single
>> > plane with PIPE_FORMAT_YUYV trickling down into the driver.
>> >
>>
>> The current way of dealing with YUV images is to feed a YUV image to
>> the driver with each plane as a distinct pipe_resource, with the
>> pipe_format used to determine the size of a plane's components and
>> setup samplers (eg. an NV12 image translates to R8 and RG88 for the Y
>> and UV planes respectively). This does not affect the colorspace of
>> the image data here - the YUV->RGB conversion pass is emitted when the
>> sampling happens via an external sampler [1].
>
> I don't see where we are keeping the information that the image is
> actually YUV, to trigger emission of the shader conversion code.
>
The YUV is 'inferred' from the underlying DRIimage's components during
tex creation (happens in dri_get_egl_image), and placed in the
associated st_texture_object which is used to fill the fragshader's
st_external_sampler_key, which is used to decide if colorspace
conversion should be emitted. Only NV12 and IYUV so far, so it works,
but I agree this seriously breaks as more formats come in :D.
> Related to that we are loosing the correct image FOURCC on a round trip.
> If we import a YUV image te "dri_format" will contain the format of one
> of the planes. Then when querying the image FOURCC we use dri_format to
> reconstruct the FOURCC.
>
An idea would be to maintain the real YUV format for multiplane images
inside the per-plane pipe_resources and use that instead of
reconstructing the real format everywhere. Or, better yet, adding a
struct image_format entry to st/DRIimage, that holds fourcc and
per-plane dri_format (similar to i965/DRIimage); and then rework the
color-conversion shader lowering path to be more generic.
> This isn't more broken than it was before, but I don't think this is
> what we should implement.
>
Do you think we can keep this patch while we sort out the right way to
do YUV, since this doesn't break anything and some later patches in
the series end up using the image_format array for egl format/modifier
queries? Otherwise I can move the table to the later patches.
Thanks,
Varad
> Regards,
> Lucas
>
>
More information about the mesa-dev
mailing list