[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 <chadversary at chromium.org>
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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170627/3be2a82e/attachment-0001.html>