Re: [Mesa-dev] [PATCH 13/30] i965/miptree: Add an explicit format parameter to create_for_dri_image

[Daniel Stone]

Hi,

On 28 June 2017 at 19:09, Jason Ekstrand  wrote:
> On Wed, Jun 28, 2017 at 10:59 AM, Daniel Stone  wrote:
>> i965 tries pretty hard to allocate sRGB images in the pre-DRIImage,
>> DRI2 (as in the X11 protocol named 'DRI2') codepath, but this isn't
>> used by Wayland, GBM, or DRI3.
>
> Except that whether you get an sRGB renderbuffer or not is governed by GLX
> and EGL and not Wayland/DRI2/DRI3.  In one of them (I think it's ES), the
> default is to get an sRGB renderbuffer but either is possible with both
> independent of how the image comes in.  We *do* see it on DRI3 and Wayland
> which is why this patch exists in the first place.

Well, that's fairly depressing. So I guess SARGB8 is only used for
GLX_ARB_framebuffer_sRGB, and the rest is just magically transforming
(ostensibly) _UNORM Mesa formats into _SRGB?

intel_gles3_srgb_workaround() is ... quite a thing.

>> So no, not for pretty much any externally-visible images AFAICT. Even
>> if it were true for scanout, the client would need to tell KMS, so KMS
>> could send a HDMI infoframe telling the display.
>
> But scanout always does sRGB.  If you want real UNORM, then you'll have to
> add kernel API.

I'm kinda confused on this point; the colour transform matrix set up
by default is an identity mapping, rather than a degamma-to-linear
(ignoring the 16-235 vs. limited dance ...). In theory, if we're
sending sRGB, we should inform the sink via an AVI infoframe, but I
can't see anywhere we actually do that.

Anyway, I don't see this patch making the historical mistake any
better or worse, so let's just mentally file it away to bottom out one
day and move on.

Cheers,
Daniel
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 13/30] i965/miptree: Add an explicit format parameter to create_for_dri_image

[Jason Ekstrand]

On Wed, Jun 28, 2017 at 11:09 AM, Jason Ekstrand 
wrote:

> On Wed, Jun 28, 2017 at 10:59 AM, Daniel Stone 
> wrote:
>
>> Hi,
>>
>> On 28 June 2017 at 16:35, Jason Ekstrand  wrote:
>> > On Wed, Jun 28, 2017 at 4:06 AM, Daniel Stone 
>> wrote:
>> >> On 28 June 2017 at 02:05, Jason Ekstrand  wrote:
>> >> > The long answer is that the DRI formats do not specify a colorspace.
>> >>
>> >> Also, strictly speaking, the DRI_IMAGE_FORMAT_* tokens don't specify a
>> >> colourspace, nor do the DRM FourCC tokens. DRI_IMAGE_FOURCC_* is
>> >> equivalent to the latter, bar the addition of a special and unique
>> >> SARGB8 token, i.e. ARGB with the sRGB transfer function (and
>> >> presumably primaries?). The rest are presumed UNORM.
>> >
>> > Wha?  What's the difference between SARGB8 and ARGB then?  My
>> > understanding was that scanout basically treats everything as sRGB
>> anyway.
>> > Clearly, my sRGB knowledge is imperfect.
>>
>> GBM_FORMAT_ARGB (aka DRI_IMAGE_FOURCC_ARGB), gets mapped to
>> DRI_IMAGE_FORMAT_ARGB, which gets mapped to
>> MESA_FORMAT_B8G8R8X8_UNORM (dri_util.c). Only
>> DRI_IMAGE_{FORMAT,FOURCC}_SARGB8 (no defined GBM token, but you can
>> pass it through the GBM API and it'll work sometimes) gets mapped to a
>> MESA_FORMAT_*_SRGB. So AFAICT, to get an sRGB scanout buffer from
>> Mesa/GBM, you'd need to allocate UNORM and do inverse-gamma in your
>> frag shader.
>>
>> Wayland similarly never maps anything to sRGB.
>>
>> X11 always imports EGLImages as UNORM, so blending would be broken in
>> a composited environment if we were actually allocating sRGB.
>>
>
> Blending *is* broken.  I had a long chat with Owen Taylor about this some
> time ago.  Everything comes into X11 sRGB encoded and scanout treats it's
> buffer as sRGB.  X11 then stomps everything to UNORM and blends in the
> wrong colorspace.
>
>
>> i965 tries pretty hard to allocate sRGB images in the pre-DRIImage,
>> DRI2 (as in the X11 protocol named 'DRI2') codepath, but this isn't
>> used by Wayland, GBM, or DRI3.
>>
>
> Except that whether you get an sRGB renderbuffer or not is governed by GLX
> and EGL and not Wayland/DRI2/DRI3.  In one of them (I think it's ES), the
> default is to get an sRGB renderbuffer but either is possible with both
> independent of how the image comes in.  We *do* see it on DRI3 and Wayland
> which is why this patch exists in the first place.
>

Inserting some asserts and running through CI confirms this.  There are
piles of times when we take a nominally UNORM DRI format and interpret it
as sRGB.


> So no, not for pretty much any externally-visible images AFAICT. Even
>> if it were true for scanout, the client would need to tell KMS, so KMS
>> could send a HDMI infoframe telling the display.
>>
>
> But scanout always does sRGB.  If you want real UNORM, then you'll have to
> add kernel API.
>
>
>> Colourspaces \_o_/
>>
>> > As for enums, sure, that can probably happen.  GL and ISL both have
>> enums
>> > for colorspace that we could re-use.
>>
>> Yes, having too few format tokens is not a problem we have. We seem to
>> have about as many of those as we have things called 'DRI2'.
>>
>
> Heh
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 13/30] i965/miptree: Add an explicit format parameter to create_for_dri_image

[Jason Ekstrand]

On Wed, Jun 28, 2017 at 10:59 AM, Daniel Stone  wrote:

> Hi,
>
> On 28 June 2017 at 16:35, Jason Ekstrand  wrote:
> > On Wed, Jun 28, 2017 at 4:06 AM, Daniel Stone 
> wrote:
> >> On 28 June 2017 at 02:05, Jason Ekstrand  wrote:
> >> > The long answer is that the DRI formats do not specify a colorspace.
> >>
> >> Also, strictly speaking, the DRI_IMAGE_FORMAT_* tokens don't specify a
> >> colourspace, nor do the DRM FourCC tokens. DRI_IMAGE_FOURCC_* is
> >> equivalent to the latter, bar the addition of a special and unique
> >> SARGB8 token, i.e. ARGB with the sRGB transfer function (and
> >> presumably primaries?). The rest are presumed UNORM.
> >
> > Wha?  What's the difference between SARGB8 and ARGB then?  My
> > understanding was that scanout basically treats everything as sRGB
> anyway.
> > Clearly, my sRGB knowledge is imperfect.
>
> GBM_FORMAT_ARGB (aka DRI_IMAGE_FOURCC_ARGB), gets mapped to
> DRI_IMAGE_FORMAT_ARGB, which gets mapped to
> MESA_FORMAT_B8G8R8X8_UNORM (dri_util.c). Only
> DRI_IMAGE_{FORMAT,FOURCC}_SARGB8 (no defined GBM token, but you can
> pass it through the GBM API and it'll work sometimes) gets mapped to a
> MESA_FORMAT_*_SRGB. So AFAICT, to get an sRGB scanout buffer from
> Mesa/GBM, you'd need to allocate UNORM and do inverse-gamma in your
> frag shader.
>
> Wayland similarly never maps anything to sRGB.
>
> X11 always imports EGLImages as UNORM, so blending would be broken in
> a composited environment if we were actually allocating sRGB.
>

Blending *is* broken.  I had a long chat with Owen Taylor about this some
time ago.  Everything comes into X11 sRGB encoded and scanout treats it's
buffer as sRGB.  X11 then stomps everything to UNORM and blends in the
wrong colorspace.


> i965 tries pretty hard to allocate sRGB images in the pre-DRIImage,
> DRI2 (as in the X11 protocol named 'DRI2') codepath, but this isn't
> used by Wayland, GBM, or DRI3.
>

Except that whether you get an sRGB renderbuffer or not is governed by GLX
and EGL and not Wayland/DRI2/DRI3.  In one of them (I think it's ES), the
default is to get an sRGB renderbuffer but either is possible with both
independent of how the image comes in.  We *do* see it on DRI3 and Wayland
which is why this patch exists in the first place.


> So no, not for pretty much any externally-visible images AFAICT. Even
> if it were true for scanout, the client would need to tell KMS, so KMS
> could send a HDMI infoframe telling the display.
>

But scanout always does sRGB.  If you want real UNORM, then you'll have to
add kernel API.


> Colourspaces \_o_/
>
> > As for enums, sure, that can probably happen.  GL and ISL both have enums
> > for colorspace that we could re-use.
>
> Yes, having too few format tokens is not a problem we have. We seem to
> have about as many of those as we have things called 'DRI2'.
>

Heh
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 13/30] i965/miptree: Add an explicit format parameter to create_for_dri_image

[Daniel Stone]

Hi,

On 28 June 2017 at 16:35, Jason Ekstrand  wrote:
> On Wed, Jun 28, 2017 at 4:06 AM, Daniel Stone  wrote:
>> On 28 June 2017 at 02:05, Jason Ekstrand  wrote:
>> > The long answer is that the DRI formats do not specify a colorspace.
>>
>> Also, strictly speaking, the DRI_IMAGE_FORMAT_* tokens don't specify a
>> colourspace, nor do the DRM FourCC tokens. DRI_IMAGE_FOURCC_* is
>> equivalent to the latter, bar the addition of a special and unique
>> SARGB8 token, i.e. ARGB with the sRGB transfer function (and
>> presumably primaries?). The rest are presumed UNORM.
>
> Wha?  What's the difference between SARGB8 and ARGB then?  My
> understanding was that scanout basically treats everything as sRGB anyway.
> Clearly, my sRGB knowledge is imperfect.

GBM_FORMAT_ARGB (aka DRI_IMAGE_FOURCC_ARGB), gets mapped to
DRI_IMAGE_FORMAT_ARGB, which gets mapped to
MESA_FORMAT_B8G8R8X8_UNORM (dri_util.c). Only
DRI_IMAGE_{FORMAT,FOURCC}_SARGB8 (no defined GBM token, but you can
pass it through the GBM API and it'll work sometimes) gets mapped to a
MESA_FORMAT_*_SRGB. So AFAICT, to get an sRGB scanout buffer from
Mesa/GBM, you'd need to allocate UNORM and do inverse-gamma in your
frag shader.

Wayland similarly never maps anything to sRGB.

X11 always imports EGLImages as UNORM, so blending would be broken in
a composited environment if we were actually allocating sRGB.

i965 tries pretty hard to allocate sRGB images in the pre-DRIImage,
DRI2 (as in the X11 protocol named 'DRI2') codepath, but this isn't
used by Wayland, GBM, or DRI3.

So no, not for pretty much any externally-visible images AFAICT. Even
if it were true for scanout, the client would need to tell KMS, so KMS
could send a HDMI infoframe telling the display.

Colourspaces \_o_/

> As for enums, sure, that can probably happen.  GL and ISL both have enums
> for colorspace that we could re-use.

Yes, having too few format tokens is not a problem we have. We seem to
have about as many of those as we have things called 'DRI2'.

Cheers,
Daniel
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 13/30] i965/miptree: Add an explicit format parameter to create_for_dri_image

[Chad Versace]

On Wed 28 Jun 2017, Daniel Stone wrote:
> Hi,
> 
> On 28 June 2017 at 02:05, Jason Ekstrand  wrote:

> > Would you feel more comfortable with a boolean sRGB parameter?  That would
> > make the answers to the above questions much more obvious at the cost of
> > some code.
> 
> s/boolean/enum/ and you're on. As said before, the number of booleans
> in this series already makes me sad, let alone adding more.

Yes, please. Pass an enum, and this code will become understandable.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 13/30] i965/miptree: Add an explicit format parameter to create_for_dri_image

[Jason Ekstrand]

On Wed, Jun 28, 2017 at 4:06 AM, Daniel Stone  wrote:

> Hi,
>
> On 28 June 2017 at 02:05, Jason Ekstrand  wrote:

> The long answer is that the DRI formats do not specify a colorspace.
>
> Also, strictly speaking, the DRI_IMAGE_FORMAT_* tokens don't specify a
> colourspace, nor do the DRM FourCC tokens. DRI_IMAGE_FOURCC_* is
> equivalent to the latter, bar the addition of a special and unique
> SARGB8 token, i.e. ARGB with the sRGB transfer function (and
> presumably primaries?). The rest are presumed UNORM.
>

Wha?  What's the difference between SARGB8 and ARGB then?  My
understanding was that scanout basically treats everything as sRGB anyway.
Clearly, my sRGB knowledge is imperfect.

As for enums, sure, that can probably happen.  GL and ISL both have enums
for colorspace that we could re-use.

--Jason
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 13/30] i965/miptree: Add an explicit format parameter to create_for_dri_image

[Daniel Stone]

Hi,

On 28 June 2017 at 02:05, Jason Ekstrand  wrote:
> The long answer is that the DRI formats do not specify a colorspace.

Also, strictly speaking, the DRI_IMAGE_FORMAT_* tokens don't specify a
colourspace, nor do the DRM FourCC tokens. DRI_IMAGE_FOURCC_* is
equivalent to the latter, bar the addition of a special and unique
SARGB8 token, i.e. ARGB with the sRGB transfer function (and
presumably primaries?). The rest are presumed UNORM.

Cheers,
Daniel
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 13/30] i965/miptree: Add an explicit format parameter to create_for_dri_image

[Daniel Stone]

Hi,

On 28 June 2017 at 02:05, Jason Ekstrand  wrote:
> On Tue, Jun 27, 2017 at 12:49 PM, Chad Versace 
> wrote:
>> In patch "i965: Use create_for_dri_image in intel_update_image_buffer",
>> I see that you pass intel_rb_format(rb) down as the 'format' parameter.
>> Is that the only place the override is needed? In that function, why do
>> the image's format and the renderbuffer's format differ? When do they
>> differ? When they do differ, is it illegal then to update the
>> image's format to match? If we don't update the image's format in
>> intel_update_image_buffer(), then does the invalidity of
>> __DRIimage::format cause potential issues elsewhere?
>
> I understand your concern.
>
> Short answer to all of the above: sRGB.
>
> The long answer is that the DRI formats do not specify a colorspace.  (To be
> fair, they don't need to because all window system buffers are sRGB).
> Depending on the selected visual, the renderbuffer format may be sRGB or
> not.  In order for other i965 internals to work sanely, we need the miptree
> format to match the renderbuffer format.  We need to somehow copy the
> sRGBness.
>
> Would you feel more comfortable with a boolean sRGB parameter?  That would
> make the answers to the above questions much more obvious at the cost of
> some code.

s/boolean/enum/ and you're on. As said before, the number of booleans
in this series already makes me sad, let alone adding more.

Cheers,
Daniel
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 13/30] i965/miptree: Add an explicit format parameter to create_for_dri_image

[Jason Ekstrand]

On Tue, Jun 27, 2017 at 12:49 PM, Chad Versace 
wrote:

> On Fri 16 Jun 2017, Jason Ekstrand wrote:
> > ---
> >  src/mesa/drivers/dri/i965/intel_fbo.c | 3 ++-
> >  src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 7 ---
> >  src/mesa/drivers/dri/i965/intel_mipmap_tree.h | 3 ++-
> >  src/mesa/drivers/dri/i965/intel_tex_image.c   | 3 ++-
> >  4 files changed, 10 insertions(+), 6 deletions(-)
>
> I dislike this patch. A lot.
>
> The __DRIimage already has a 'format' member. Why is it necessary to
> override that format? More importantly, *when* is it necessary?
>
> In patch "i965: Use create_for_dri_image in intel_update_image_buffer",
> I see that you pass intel_rb_format(rb) down as the 'format' parameter.
> Is that the only place the override is needed? In that function, why do
> the image's format and the renderbuffer's format differ? When do they
> differ? When they do differ, is it illegal then to update the
> image's format to match? If we don't update the image's format in
> intel_update_image_buffer(), then does the invalidity of
> __DRIimage::format cause potential issues elsewhere?
>

I understand your concern.

Short answer to all of the above: sRGB.

The long answer is that the DRI formats do not specify a colorspace.  (To
be fair, they don't need to because all window system buffers are sRGB).
Depending on the selected visual, the renderbuffer format may be sRGB or
not.  In order for other i965 internals to work sanely, we need the miptree
format to match the renderbuffer format.  We need to somehow copy the
sRGBness.

Would you feel more comfortable with a boolean sRGB parameter?  That would
make the answers to the above questions much more obvious at the cost of
some code.


> [...]
>
> > diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > index 08c13fc..7b4d431 100644
> > --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > @@ -900,12 +900,13 @@ miptree_create_for_planar_image(struct
> brw_context *brw,
> >
> >  struct intel_mipmap_tree *
> >  intel_miptree_create_for_dri_image(struct brw_context *brw,
> > -   __DRIimage *image, GLenum target)
> > +   __DRIimage *image, GLenum target,
> > +   mesa_format format)
> >  {
> > if (image->planar_format && image->planar_format->nplanes > 0)
> >return miptree_create_for_planar_image(brw, image, target);
> >
> > -   if (!brw->ctx.TextureFormatSupported[image->format])
> > +   if (!brw->ctx.TextureFormatSupported[format])
> >return NULL;
>
> The 'format' parameter is ignored if the image has a planar format. That
> makes me suspicious. At a minimum, this needs
>
>   assert(!format == !image->planar_format)
>
> or an explanation of why the assertion is invalid.
>

I think if we do what I suggested above, this will become obvious.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 13/30] i965/miptree: Add an explicit format parameter to create_for_dri_image

[Chad Versace]

On Fri 16 Jun 2017, Jason Ekstrand wrote:
> ---
>  src/mesa/drivers/dri/i965/intel_fbo.c | 3 ++-
>  src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 7 ---
>  src/mesa/drivers/dri/i965/intel_mipmap_tree.h | 3 ++-
>  src/mesa/drivers/dri/i965/intel_tex_image.c   | 3 ++-
>  4 files changed, 10 insertions(+), 6 deletions(-)

I dislike this patch. A lot.

The __DRIimage already has a 'format' member. Why is it necessary to
override that format? More importantly, *when* is it necessary?

In patch "i965: Use create_for_dri_image in intel_update_image_buffer",
I see that you pass intel_rb_format(rb) down as the 'format' parameter.
Is that the only place the override is needed? In that function, why do
the image's format and the renderbuffer's format differ? When do they
differ? When they do differ, is it illegal then to update the
image's format to match? If we don't update the image's format in
intel_update_image_buffer(), then does the invalidity of
__DRIimage::format cause potential issues elsewhere?

[...]

> diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c 
> b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> index 08c13fc..7b4d431 100644
> --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> @@ -900,12 +900,13 @@ miptree_create_for_planar_image(struct brw_context *brw,
>  
>  struct intel_mipmap_tree *
>  intel_miptree_create_for_dri_image(struct brw_context *brw,
> -   __DRIimage *image, GLenum target)
> +   __DRIimage *image, GLenum target,
> +   mesa_format format)
>  {
> if (image->planar_format && image->planar_format->nplanes > 0)
>return miptree_create_for_planar_image(brw, image, target);
>  
> -   if (!brw->ctx.TextureFormatSupported[image->format])
> +   if (!brw->ctx.TextureFormatSupported[format])
>return NULL;

The 'format' parameter is ignored if the image has a planar format. That
makes me suspicious. At a minimum, this needs

  assert(!format == !image->planar_format)

or an explanation of why the assertion is invalid.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev