Re: [Mesa-dev] [PATCH 00/14] Enable sRGB-encoded fast-clears on CannonLake

2018-04-04 Thread Nanley Chery 
On Tue, Apr 03, 2018 at 03:50:38PM -0700, Jason Ekstrand wrote:
> I think I've reviewed all the ones that make significant functional
> changes.  The exception is the patch that makes us not do a fast depth
> clear.  I think what you did is probably better but I haven't thought about
> it enough to be sure.
> 

I can drop this patch. Although I think it's an improvement, I should
probably find some benchmarks to substantiate the claim.

> I'm not sure what I think about the last several that mostly move code
> around.  On the one hand, it kind-of unifies the "should we fast-clear"
> question into intel_mipmap_tree.c.  On the other hand, it pulls it away
> from the function that actually does the clear spreads the code out more.
> Multiple times in the past, I've wanted to pull the guts of the color
> clearing code out of brw_blorp.c and into brw_clear.c and just have
> brw_clear_miptree and brw_ccs/mcs_clear_miptree helpers.  Then all of those
> decisions would be together in brw_clear.c.  In any case, I'll think on it
> more.
> 

I understand your concern. There are multiple ways to go about hiding
::fast_clear_color and the one I went for (with super-powered setters)
does have the issues you bring up. I'll send out a v2 that accomplishes
hiding it in a less invasive manner. Please, let me know which version
you prefer (if either).

-Nanley

> --Jason
> 
> 
> On Fri, Mar 30, 2018 at 11:12 AM, Nanley Chery 
> wrote:
> 
> > Starting with CannonLake, the sampler no longer decodes the surface
> > state clear color when using an sRGB-formatted texture. This change
> > requires that our driver perform this decode in software instead. We
> > accounted for this change initially by disabling fast-clears when sRGB
> > encode was enabled. This series implements the software decode and
> > re-enables sRGB-encoded fast-clears.
> >
> > The software decode is performed through a new getter for the miptree
> > clear color. To keep the miptree API balanced and to discourage its
> > users from accessing the clear color field directly, we add a getter for
> > the depth clear value and change the existing setters so that the user
> > no longer needs to know the current clear color to perform an efficient
> > clear operation.
> >
> > Two piglit tests have been modified to test that the linearization of
> > the clear color occurs (when appropriate) for shader texture() calls and
> > on framebuffer blit sources. The modification patches can be found here:
> > https://lists.freedesktop.org/archives/piglit/2018-March/023996.html
> >
> > Jason Ekstrand (1):
> >   util/srgb: Add a float sRGB -> linear helper
> >
> > Nanley Chery (13):
> >   i965: Use the brw_context for the clear color and value setters
> >   i965/miptree: Move the clear color and value setter implementations
> >   i965: Make the miptree clear color setter take a gl_color_union
> >   i965/miptree: Add and use a getter for the clear color
> >   i965/miptree: Extend the sRGB-blending WA to future platforms
> >   i965/meta_util: Re-enable sRGB-encoded fast-clears on CNL
> >   i965: Add and use a getter for depth miptree clear values
> >   i965: Allow failure when setting the depth clear value
> >   i965/brw_clear: Don't resolve to change the depth clear value
> >   i965/brw_clear: Delete redundant code
> >   i965/blorp: Also skip the fast clear if the clear color differs
> >   i965/miptree: Allow failure when setting the clear color
> >   i965/blorp: Delete redundant code
> >
> >  src/mesa/drivers/dri/i965/brw_blorp.c|  60 +++--
> >  src/mesa/drivers/dri/i965/brw_clear.c|  45 +--
> >  src/mesa/drivers/dri/i965/brw_meta_util.c|  11 --
> >  src/mesa/drivers/dri/i965/brw_misc_state.c   |  15 ---
> >  src/mesa/drivers/dri/i965/brw_wm_surface_state.c |   4 +-
> >  src/mesa/drivers/dri/i965/gen6_depth_state.c |   4 +-
> >  src/mesa/drivers/dri/i965/gen7_misc_state.c  |   3 +-
> >  src/mesa/drivers/dri/i965/gen8_depth_state.c |   3 +-
> >  src/mesa/drivers/dri/i965/intel_mipmap_tree.c| 165
> > ++-
> >  src/mesa/drivers/dri/i965/intel_mipmap_tree.h|  53 
> >  src/util/format_srgb.h   |  14 ++
> >  11 files changed, 233 insertions(+), 144 deletions(-)
> >
> > --
> > 2.16.2
> >
> > ___
> > mesa-dev mailing list
> > mesa-dev@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> >
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 00/14] Enable sRGB-encoded fast-clears on CannonLake

2018-04-03 Thread Jason Ekstrand 
I think I've reviewed all the ones that make significant functional
changes.  The exception is the patch that makes us not do a fast depth
clear.  I think what you did is probably better but I haven't thought about
it enough to be sure.

I'm not sure what I think about the last several that mostly move code
around.  On the one hand, it kind-of unifies the "should we fast-clear"
question into intel_mipmap_tree.c.  On the other hand, it pulls it away
from the function that actually does the clear spreads the code out more.
Multiple times in the past, I've wanted to pull the guts of the color
clearing code out of brw_blorp.c and into brw_clear.c and just have
brw_clear_miptree and brw_ccs/mcs_clear_miptree helpers.  Then all of those
decisions would be together in brw_clear.c.  In any case, I'll think on it
more.

--Jason


On Fri, Mar 30, 2018 at 11:12 AM, Nanley Chery 
wrote:

> Starting with CannonLake, the sampler no longer decodes the surface
> state clear color when using an sRGB-formatted texture. This change
> requires that our driver perform this decode in software instead. We
> accounted for this change initially by disabling fast-clears when sRGB
> encode was enabled. This series implements the software decode and
> re-enables sRGB-encoded fast-clears.
>
> The software decode is performed through a new getter for the miptree
> clear color. To keep the miptree API balanced and to discourage its
> users from accessing the clear color field directly, we add a getter for
> the depth clear value and change the existing setters so that the user
> no longer needs to know the current clear color to perform an efficient
> clear operation.
>
> Two piglit tests have been modified to test that the linearization of
> the clear color occurs (when appropriate) for shader texture() calls and
> on framebuffer blit sources. The modification patches can be found here:
> https://lists.freedesktop.org/archives/piglit/2018-March/023996.html
>
> Jason Ekstrand (1):
>   util/srgb: Add a float sRGB -> linear helper
>
> Nanley Chery (13):
>   i965: Use the brw_context for the clear color and value setters
>   i965/miptree: Move the clear color and value setter implementations
>   i965: Make the miptree clear color setter take a gl_color_union
>   i965/miptree: Add and use a getter for the clear color
>   i965/miptree: Extend the sRGB-blending WA to future platforms
>   i965/meta_util: Re-enable sRGB-encoded fast-clears on CNL
>   i965: Add and use a getter for depth miptree clear values
>   i965: Allow failure when setting the depth clear value
>   i965/brw_clear: Don't resolve to change the depth clear value
>   i965/brw_clear: Delete redundant code
>   i965/blorp: Also skip the fast clear if the clear color differs
>   i965/miptree: Allow failure when setting the clear color
>   i965/blorp: Delete redundant code
>
>  src/mesa/drivers/dri/i965/brw_blorp.c|  60 +++--
>  src/mesa/drivers/dri/i965/brw_clear.c|  45 +--
>  src/mesa/drivers/dri/i965/brw_meta_util.c|  11 --
>  src/mesa/drivers/dri/i965/brw_misc_state.c   |  15 ---
>  src/mesa/drivers/dri/i965/brw_wm_surface_state.c |   4 +-
>  src/mesa/drivers/dri/i965/gen6_depth_state.c |   4 +-
>  src/mesa/drivers/dri/i965/gen7_misc_state.c  |   3 +-
>  src/mesa/drivers/dri/i965/gen8_depth_state.c |   3 +-
>  src/mesa/drivers/dri/i965/intel_mipmap_tree.c| 165
> ++-
>  src/mesa/drivers/dri/i965/intel_mipmap_tree.h|  53 
>  src/util/format_srgb.h   |  14 ++
>  11 files changed, 233 insertions(+), 144 deletions(-)
>
> --
> 2.16.2
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 00/14] Enable sRGB-encoded fast-clears on CannonLake

2018-03-30 Thread Nanley Chery 
Starting with CannonLake, the sampler no longer decodes the surface
state clear color when using an sRGB-formatted texture. This change
requires that our driver perform this decode in software instead. We
accounted for this change initially by disabling fast-clears when sRGB
encode was enabled. This series implements the software decode and
re-enables sRGB-encoded fast-clears.

The software decode is performed through a new getter for the miptree
clear color. To keep the miptree API balanced and to discourage its
users from accessing the clear color field directly, we add a getter for
the depth clear value and change the existing setters so that the user
no longer needs to know the current clear color to perform an efficient
clear operation.

Two piglit tests have been modified to test that the linearization of
the clear color occurs (when appropriate) for shader texture() calls and
on framebuffer blit sources. The modification patches can be found here:
https://lists.freedesktop.org/archives/piglit/2018-March/023996.html

Jason Ekstrand (1):
  util/srgb: Add a float sRGB -> linear helper

Nanley Chery (13):
  i965: Use the brw_context for the clear color and value setters
  i965/miptree: Move the clear color and value setter implementations
  i965: Make the miptree clear color setter take a gl_color_union
  i965/miptree: Add and use a getter for the clear color
  i965/miptree: Extend the sRGB-blending WA to future platforms
  i965/meta_util: Re-enable sRGB-encoded fast-clears on CNL
  i965: Add and use a getter for depth miptree clear values
  i965: Allow failure when setting the depth clear value
  i965/brw_clear: Don't resolve to change the depth clear value
  i965/brw_clear: Delete redundant code
  i965/blorp: Also skip the fast clear if the clear color differs
  i965/miptree: Allow failure when setting the clear color
  i965/blorp: Delete redundant code

 src/mesa/drivers/dri/i965/brw_blorp.c|  60 +++--
 src/mesa/drivers/dri/i965/brw_clear.c|  45 +--
 src/mesa/drivers/dri/i965/brw_meta_util.c|  11 --
 src/mesa/drivers/dri/i965/brw_misc_state.c   |  15 ---
 src/mesa/drivers/dri/i965/brw_wm_surface_state.c |   4 +-
 src/mesa/drivers/dri/i965/gen6_depth_state.c |   4 +-
 src/mesa/drivers/dri/i965/gen7_misc_state.c  |   3 +-
 src/mesa/drivers/dri/i965/gen8_depth_state.c |   3 +-
 src/mesa/drivers/dri/i965/intel_mipmap_tree.c| 165 ++-
 src/mesa/drivers/dri/i965/intel_mipmap_tree.h|  53 
 src/util/format_srgb.h   |  14 ++
 11 files changed, 233 insertions(+), 144 deletions(-)

-- 
2.16.2

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