<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Jun 27, 2017 at 12:49 PM, Chad Versace <span dir="ltr"><<a href="mailto:chadversary@chromium.org" target="_blank">chadversary@chromium.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On Fri 16 Jun 2017, Jason Ekstrand wrote:<br>
> ---<br>
>  src/mesa/drivers/dri/i965/<wbr>intel_fbo.c         | 3 ++-<br>
>  src/mesa/drivers/dri/i965/<wbr>intel_mipmap_tree.c | 7 ++++---<br>
>  src/mesa/drivers/dri/i965/<wbr>intel_mipmap_tree.h | 3 ++-<br>
>  src/mesa/drivers/dri/i965/<wbr>intel_tex_image.c   | 3 ++-<br>
>  4 files changed, 10 insertions(+), 6 deletions(-)<br>
<br>
</span>I dislike this patch. A lot.<br>
<br>
The __DRIimage already has a 'format' member. Why is it necessary to<br>
override that format? More importantly, *when* is it necessary?<br>
<br>
In patch "i965: Use create_for_dri_image in intel_update_image_buffer",<br>
I see that you pass intel_rb_format(rb) down as the 'format' parameter.<br>
Is that the only place the override is needed? In that function, why do<br>
the image's format and the renderbuffer's format differ? When do they<br>
differ? When they do differ, is it illegal then to update the<br>
image's format to match? If we don't update the image's format in<br>
intel_update_image_buffer(), then does the invalidity of<br>
__DRIimage::format cause potential issues elsewhere?<br></blockquote><div><br></div><div>I understand your concern.<br><br>Short answer to all of the above: sRGB.<br><br></div><div>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.<br><br>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.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
[...]<br>
<span class=""><br>
> diff --git a/src/mesa/drivers/dri/i965/<wbr>intel_mipmap_tree.c b/src/mesa/drivers/dri/i965/<wbr>intel_mipmap_tree.c<br>
> index 08c13fc..7b4d431 100644<br>
> --- a/src/mesa/drivers/dri/i965/<wbr>intel_mipmap_tree.c<br>
> +++ b/src/mesa/drivers/dri/i965/<wbr>intel_mipmap_tree.c<br>
> @@ -900,12 +900,13 @@ miptree_create_for_planar_<wbr>image(struct brw_context *brw,<br>
><br>
>  struct intel_mipmap_tree *<br>
>  intel_miptree_create_for_dri_<wbr>image(struct brw_context *brw,<br>
> -                                   __DRIimage *image, GLenum target)<br>
> +                                   __DRIimage *image, GLenum target,<br>
> +                                   mesa_format format)<br>
>  {<br>
>     if (image->planar_format && image->planar_format->nplanes > 0)<br>
>        return miptree_create_for_planar_<wbr>image(brw, image, target);<br>
><br>
> -   if (!brw->ctx.<wbr>TextureFormatSupported[image-><wbr>format])<br>
> +   if (!brw->ctx.<wbr>TextureFormatSupported[format]<wbr>)<br>
>        return NULL;<br>
<br>
</span>The 'format' parameter is ignored if the image has a planar format. That<br>
makes me suspicious. At a minimum, this needs<br>
<br>
  assert(!format == !image->planar_format)<br>
<br>
or an explanation of why the assertion is invalid.<br>
</blockquote></div><br></div><div class="gmail_extra">I think if we do what I suggested above, this will become obvious.<br></div></div>