subject:"\[Mesa\-dev\] \[PATCH 17\/64\] isl\/state\: Refactor the setup of clear colors"

Re: [Mesa-dev] [PATCH 17/64] isl/state: Refactor the setup of clear colors

2016-06-16 Thread Chad Versace

On Thu 16 Jun 2016, Jason Ekstrand wrote:
> On Thu, Jun 16, 2016 at 11:05 AM, Chad Versace 
> wrote:
> 
> > On Sat 11 Jun 2016, Jason Ekstrand wrote:
> > > This commit switches clear colors to use #if's instead of a C if.  This
> > > lets us properly handle SNB where the clear color field doesn't exist.
> > > ---
> > >  src/intel/isl/isl_surface_state.c | 44
> > +++
> > >  1 file changed, 22 insertions(+), 22 deletions(-)
> > >
> > > diff --git a/src/intel/isl/isl_surface_state.c
> > b/src/intel/isl/isl_surface_state.c
> > > index db90936..aa720d8 100644
> > > --- a/src/intel/isl/isl_surface_state.c
> > > +++ b/src/intel/isl/isl_surface_state.c
> > > @@ -372,31 +372,31 @@ isl_genX(surf_fill_state_s)(const struct
> > isl_device *dev, void *state,
> > > }
> > >  #endif
> > >
> > > -   if (GEN_GEN <= 8) {
> > > -  /* Prior to Sky Lake, we only have one bit for the clear color
> > which
> > > -   * gives us 0 or 1 in whatever the surface's format happens to be.
> > > -   */
> > > -  if (isl_format_has_int_channel(info->view->format)) {
> > > - for (unsigned i = 0; i < 4; i++) {
> > > -assert(info->clear_color.u32[i] == 0 ||
> > > -   info->clear_color.u32[i] == 1);
> > > - }
> > > -  } else {
> > > - for (unsigned i = 0; i < 4; i++) {
> > > -assert(info->clear_color.f32[i] == 0.0f ||
> > > -   info->clear_color.f32[i] == 1.0f);
> > > - }
> > > +#if GEN_GEN >= 9
> > > +   s.RedClearColor = info->clear_color.u32[0];
> > > +   s.GreenClearColor = info->clear_color.u32[1];
> > > +   s.BlueClearColor = info->clear_color.u32[2];
> > > +   s.AlphaClearColor = info->clear_color.u32[3];
> >
> > The gen9 block looks good.
> >
> > > +#elif GEN_GEN >= 7
> > > +   /* Prior to Sky Lake, we only have one bit for the clear color which
> > > +* gives us 0 or 1 in whatever the surface's format happens to be.
> > > +*/
> > > +   if (isl_format_has_int_channel(info->view->format)) {
> > > +  for (unsigned i = 0; i < 4; i++) {
> > > + assert(info->clear_color.u32[i] == 0 ||
> > > +info->clear_color.u32[i] == 1);
> > >}
> > > -  s.RedClearColor = info->clear_color.u32[0] != 0;
> > > -  s.GreenClearColor = info->clear_color.u32[1] != 0;
> > > -  s.BlueClearColor = info->clear_color.u32[2] != 0;
> > > -  s.AlphaClearColor = info->clear_color.u32[3] != 0;
> > > } else {
> > > -  s.RedClearColor = info->clear_color.u32[0];
> > > -  s.GreenClearColor = info->clear_color.u32[1];
> > > -  s.BlueClearColor = info->clear_color.u32[2];
> > > -  s.AlphaClearColor = info->clear_color.u32[3];
> > > +  for (unsigned i = 0; i < 4; i++) {
> > > + assert(info->clear_color.f32[i] == 0.0f ||
> > > +info->clear_color.f32[i] == 1.0f);
> > > +  }
> > > }
> > > +   s.RedClearColor = info->clear_color.u32[0] != 0;
> > > +   s.GreenClearColor = info->clear_color.u32[1] != 0;
> > > +   s.BlueClearColor = info->clear_color.u32[2] != 0;
> > > +   s.AlphaClearColor = info->clear_color.u32[3] != 0;
> >
> > This block looks broken, both pre- and post-patch. Is this code actually
> > used anywhere in the Vulkan driver? I expect not, as the driver doesn't
> > use the CCS yet.
> >
> > In the for loop, the code asserts clear_color.f32[i] is 0.0f or 1.0f.
> > I believe the assignment expressions should also use f32, not u32. That
> > is,
> >
> > s.RedClearColor = info->clear_color.f32[0] != 0.0f;
> > s.GreenClearColor = info->clear_color.f32[1] != 0.0f;
> > s.BlueClearColor = info->clear_color.f32[2] != 0.0f;
> > s.AlphaClearColor = info->clear_color.f32[3] != 0.0f;
> >
> 
> Right.  That's probably more correct but given that 0.0f is 0u as bits, I
> don't know if it actually matters.  Maybe for -0?

Yes, we should do it for -0. The user inputs to glClearColor may be
calculated in convoluted ways that produce -0.

Also, I think
s.RedClearColor = info->clear_color.f32[0] == 1.0f
is clearer, but it's not important.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 17/64] isl/state: Refactor the setup of clear colors

2016-06-16 Thread Jason Ekstrand

On Thu, Jun 16, 2016 at 11:05 AM, Chad Versace 
wrote:

> On Sat 11 Jun 2016, Jason Ekstrand wrote:
> > This commit switches clear colors to use #if's instead of a C if.  This
> > lets us properly handle SNB where the clear color field doesn't exist.
> > ---
> >  src/intel/isl/isl_surface_state.c | 44
> +++
> >  1 file changed, 22 insertions(+), 22 deletions(-)
> >
> > diff --git a/src/intel/isl/isl_surface_state.c
> b/src/intel/isl/isl_surface_state.c
> > index db90936..aa720d8 100644
> > --- a/src/intel/isl/isl_surface_state.c
> > +++ b/src/intel/isl/isl_surface_state.c
> > @@ -372,31 +372,31 @@ isl_genX(surf_fill_state_s)(const struct
> isl_device *dev, void *state,
> > }
> >  #endif
> >
> > -   if (GEN_GEN <= 8) {
> > -  /* Prior to Sky Lake, we only have one bit for the clear color
> which
> > -   * gives us 0 or 1 in whatever the surface's format happens to be.
> > -   */
> > -  if (isl_format_has_int_channel(info->view->format)) {
> > - for (unsigned i = 0; i < 4; i++) {
> > -assert(info->clear_color.u32[i] == 0 ||
> > -   info->clear_color.u32[i] == 1);
> > - }
> > -  } else {
> > - for (unsigned i = 0; i < 4; i++) {
> > -assert(info->clear_color.f32[i] == 0.0f ||
> > -   info->clear_color.f32[i] == 1.0f);
> > - }
> > +#if GEN_GEN >= 9
> > +   s.RedClearColor = info->clear_color.u32[0];
> > +   s.GreenClearColor = info->clear_color.u32[1];
> > +   s.BlueClearColor = info->clear_color.u32[2];
> > +   s.AlphaClearColor = info->clear_color.u32[3];
>
> The gen9 block looks good.
>
> > +#elif GEN_GEN >= 7
> > +   /* Prior to Sky Lake, we only have one bit for the clear color which
> > +* gives us 0 or 1 in whatever the surface's format happens to be.
> > +*/
> > +   if (isl_format_has_int_channel(info->view->format)) {
> > +  for (unsigned i = 0; i < 4; i++) {
> > + assert(info->clear_color.u32[i] == 0 ||
> > +info->clear_color.u32[i] == 1);
> >}
> > -  s.RedClearColor = info->clear_color.u32[0] != 0;
> > -  s.GreenClearColor = info->clear_color.u32[1] != 0;
> > -  s.BlueClearColor = info->clear_color.u32[2] != 0;
> > -  s.AlphaClearColor = info->clear_color.u32[3] != 0;
> > } else {
> > -  s.RedClearColor = info->clear_color.u32[0];
> > -  s.GreenClearColor = info->clear_color.u32[1];
> > -  s.BlueClearColor = info->clear_color.u32[2];
> > -  s.AlphaClearColor = info->clear_color.u32[3];
> > +  for (unsigned i = 0; i < 4; i++) {
> > + assert(info->clear_color.f32[i] == 0.0f ||
> > +info->clear_color.f32[i] == 1.0f);
> > +  }
> > }
> > +   s.RedClearColor = info->clear_color.u32[0] != 0;
> > +   s.GreenClearColor = info->clear_color.u32[1] != 0;
> > +   s.BlueClearColor = info->clear_color.u32[2] != 0;
> > +   s.AlphaClearColor = info->clear_color.u32[3] != 0;
>
> This block looks broken, both pre- and post-patch. Is this code actually
> used anywhere in the Vulkan driver? I expect not, as the driver doesn't
> use the CCS yet.
>
> In the for loop, the code asserts clear_color.f32[i] is 0.0f or 1.0f.
> I believe the assignment expressions should also use f32, not u32. That
> is,
>
> s.RedClearColor = info->clear_color.f32[0] != 0.0f;
> s.GreenClearColor = info->clear_color.f32[1] != 0.0f;
> s.BlueClearColor = info->clear_color.f32[2] != 0.0f;
> s.AlphaClearColor = info->clear_color.f32[3] != 0.0f;
>

Right.  That's probably more correct but given that 0.0f is 0u as bits, I
don't know if it actually matters.  Maybe for -0?
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 17/64] isl/state: Refactor the setup of clear colors

2016-06-16 Thread Chad Versace

On Sat 11 Jun 2016, Jason Ekstrand wrote:
> This commit switches clear colors to use #if's instead of a C if.  This
> lets us properly handle SNB where the clear color field doesn't exist.
> ---
>  src/intel/isl/isl_surface_state.c | 44 
> +++
>  1 file changed, 22 insertions(+), 22 deletions(-)
> 
> diff --git a/src/intel/isl/isl_surface_state.c 
> b/src/intel/isl/isl_surface_state.c
> index db90936..aa720d8 100644
> --- a/src/intel/isl/isl_surface_state.c
> +++ b/src/intel/isl/isl_surface_state.c
> @@ -372,31 +372,31 @@ isl_genX(surf_fill_state_s)(const struct isl_device 
> *dev, void *state,
> }
>  #endif
>  
> -   if (GEN_GEN <= 8) {
> -  /* Prior to Sky Lake, we only have one bit for the clear color which
> -   * gives us 0 or 1 in whatever the surface's format happens to be.
> -   */
> -  if (isl_format_has_int_channel(info->view->format)) {
> - for (unsigned i = 0; i < 4; i++) {
> -assert(info->clear_color.u32[i] == 0 ||
> -   info->clear_color.u32[i] == 1);
> - }
> -  } else {
> - for (unsigned i = 0; i < 4; i++) {
> -assert(info->clear_color.f32[i] == 0.0f ||
> -   info->clear_color.f32[i] == 1.0f);
> - }
> +#if GEN_GEN >= 9
> +   s.RedClearColor = info->clear_color.u32[0];
> +   s.GreenClearColor = info->clear_color.u32[1];
> +   s.BlueClearColor = info->clear_color.u32[2];
> +   s.AlphaClearColor = info->clear_color.u32[3];

The gen9 block looks good.

> +#elif GEN_GEN >= 7
> +   /* Prior to Sky Lake, we only have one bit for the clear color which
> +* gives us 0 or 1 in whatever the surface's format happens to be.
> +*/
> +   if (isl_format_has_int_channel(info->view->format)) {
> +  for (unsigned i = 0; i < 4; i++) {
> + assert(info->clear_color.u32[i] == 0 ||
> +info->clear_color.u32[i] == 1);
>}
> -  s.RedClearColor = info->clear_color.u32[0] != 0;
> -  s.GreenClearColor = info->clear_color.u32[1] != 0;
> -  s.BlueClearColor = info->clear_color.u32[2] != 0;
> -  s.AlphaClearColor = info->clear_color.u32[3] != 0;
> } else {
> -  s.RedClearColor = info->clear_color.u32[0];
> -  s.GreenClearColor = info->clear_color.u32[1];
> -  s.BlueClearColor = info->clear_color.u32[2];
> -  s.AlphaClearColor = info->clear_color.u32[3];
> +  for (unsigned i = 0; i < 4; i++) {
> + assert(info->clear_color.f32[i] == 0.0f ||
> +info->clear_color.f32[i] == 1.0f);
> +  }
> }
> +   s.RedClearColor = info->clear_color.u32[0] != 0;
> +   s.GreenClearColor = info->clear_color.u32[1] != 0;
> +   s.BlueClearColor = info->clear_color.u32[2] != 0;
> +   s.AlphaClearColor = info->clear_color.u32[3] != 0;

This block looks broken, both pre- and post-patch. Is this code actually
used anywhere in the Vulkan driver? I expect not, as the driver doesn't
use the CCS yet.

In the for loop, the code asserts clear_color.f32[i] is 0.0f or 1.0f.
I believe the assignment expressions should also use f32, not u32. That
is,

s.RedClearColor = info->clear_color.f32[0] != 0.0f;
s.GreenClearColor = info->clear_color.f32[1] != 0.0f;
s.BlueClearColor = info->clear_color.f32[2] != 0.0f;
s.AlphaClearColor = info->clear_color.f32[3] != 0.0f;
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 17/64] isl/state: Refactor the setup of clear colors

2016-06-11 Thread Jason Ekstrand

This commit switches clear colors to use #if's instead of a C if.  This
lets us properly handle SNB where the clear color field doesn't exist.
---
 src/intel/isl/isl_surface_state.c | 44 +++
 1 file changed, 22 insertions(+), 22 deletions(-)

diff --git a/src/intel/isl/isl_surface_state.c 
b/src/intel/isl/isl_surface_state.c
index db90936..aa720d8 100644
--- a/src/intel/isl/isl_surface_state.c
+++ b/src/intel/isl/isl_surface_state.c
@@ -372,31 +372,31 @@ isl_genX(surf_fill_state_s)(const struct isl_device *dev, 
void *state,
}
 #endif
 
-   if (GEN_GEN <= 8) {
-  /* Prior to Sky Lake, we only have one bit for the clear color which
-   * gives us 0 or 1 in whatever the surface's format happens to be.
-   */
-  if (isl_format_has_int_channel(info->view->format)) {
- for (unsigned i = 0; i < 4; i++) {
-assert(info->clear_color.u32[i] == 0 ||
-   info->clear_color.u32[i] == 1);
- }
-  } else {
- for (unsigned i = 0; i < 4; i++) {
-assert(info->clear_color.f32[i] == 0.0f ||
-   info->clear_color.f32[i] == 1.0f);
- }
+#if GEN_GEN >= 9
+   s.RedClearColor = info->clear_color.u32[0];
+   s.GreenClearColor = info->clear_color.u32[1];
+   s.BlueClearColor = info->clear_color.u32[2];
+   s.AlphaClearColor = info->clear_color.u32[3];
+#elif GEN_GEN >= 7
+   /* Prior to Sky Lake, we only have one bit for the clear color which
+* gives us 0 or 1 in whatever the surface's format happens to be.
+*/
+   if (isl_format_has_int_channel(info->view->format)) {
+  for (unsigned i = 0; i < 4; i++) {
+ assert(info->clear_color.u32[i] == 0 ||
+info->clear_color.u32[i] == 1);
   }
-  s.RedClearColor = info->clear_color.u32[0] != 0;
-  s.GreenClearColor = info->clear_color.u32[1] != 0;
-  s.BlueClearColor = info->clear_color.u32[2] != 0;
-  s.AlphaClearColor = info->clear_color.u32[3] != 0;
} else {
-  s.RedClearColor = info->clear_color.u32[0];
-  s.GreenClearColor = info->clear_color.u32[1];
-  s.BlueClearColor = info->clear_color.u32[2];
-  s.AlphaClearColor = info->clear_color.u32[3];
+  for (unsigned i = 0; i < 4; i++) {
+ assert(info->clear_color.f32[i] == 0.0f ||
+info->clear_color.f32[i] == 1.0f);
+  }
}
+   s.RedClearColor = info->clear_color.u32[0] != 0;
+   s.GreenClearColor = info->clear_color.u32[1] != 0;
+   s.BlueClearColor = info->clear_color.u32[2] != 0;
+   s.AlphaClearColor = info->clear_color.u32[3] != 0;
+#endif
 
GENX(RENDER_SURFACE_STATE_pack)(NULL, state, );
 }
-- 
2.5.0.400.gff86faf

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

Re: [Mesa-dev] [PATCH 17/64] isl/state: Refactor the setup of clear colors

Re: [Mesa-dev] [PATCH 17/64] isl/state: Refactor the setup of clear colors

Re: [Mesa-dev] [PATCH 17/64] isl/state: Refactor the setup of clear colors

[Mesa-dev] [PATCH 17/64] isl/state: Refactor the setup of clear colors

4 matches

Site Navigation

Mail list logo

Footer information