Re: [Mesa-dev] [PATCH 36/64] isl: Add support for filling out surface states all the way back to gen4

[Chad Versace]

On Mon 20 Jun 2016, Jason Ekstrand wrote:
> On Mon, Jun 20, 2016 at 3:53 PM, Chad Versace 
> wrote:
> 
> > On Sat 11 Jun 2016, Jason Ekstrand wrote:
> > > ---
> > >  src/intel/isl/Makefile.am | 12 
> > >  src/intel/isl/Makefile.sources| 13 +++--
> > >  src/intel/isl/isl.c   | 28 +++
> > >  src/intel/isl/isl_priv.h  | 24 
> > >  src/intel/isl/isl_surface_state.c | 58


> > > +   case 4:
> > > +  if (ISL_DEV_IS_G4X(dev)) {
> > > + isl_gen4_surf_fill_state_s(dev, state, info);
> > > +  } else {
> > > + /* G45 surface state is the same as gen5 */
> > > + isl_gen5_surf_fill_state_s(dev, state, info);
> > > +  }
> > > +  break;
> >
> > The above can't be correct, because...
> >
> >#define GEN_IS_G4X ((GEN_VERSIONx10) == 45)
> >
> > I think you meant this:
> >
> >if (ISL_DEV_IS_G4X(dev)) {
> >   /* G45 surface state is the same as gen5 */
> >   isl_gen5_surf_fill_state_s(dev, state, info);
> >} else {
> >   isl_gen4_surf_fill_state_s(dev, state, info);
> >}
> >
> 
> You are correct.  The reason it didn't trigger a bug is that the only
> difference bertween gen4 and 4.5 is that x/y offsets are introduced in
> 4.5.  Since we never use ISL for filling out surfaces with x/y offsets on
> gen4.5 with this series, I never caught it.  Thanks!

[snip]

> > > switch (ISL_DEV_GEN(dev)) {
> > > +   case 4:
> > > +  if (ISL_DEV_IS_G4X(dev)) {
> > > + isl_gen4_buffer_fill_state_s(state, info);
> > > +  } else {
> > > + /* G45 surface state is the same as gen5 */
> > > + isl_gen5_buffer_fill_state_s(state, info);
> > > +  }
> > > +  break;
> >
> > Same as above.
> >
> 
> My local fix just changes buffers to use the gen5 version for 4, 4.5, and 5

Sounds good, as buffers never use x/y offsets.

> > > -static struct isl_extent3d
> > > +static inline struct isl_extent3d
> > >  get_image_alignment(const struct isl_surf *surf)
> > >  {
> > > if (GEN_GEN >= 9) {
> > > @@ -199,6 +201,23 @@ isl_genX(surf_fill_state_s)(const struct isl_device
> > *dev, void *state,
> > > s.Width = info->surf->logical_level0_px.width - 1;
> > > s.Height = info->surf->logical_level0_px.height - 1;
> > >
> > > +   /* In the gen6 PRM Volume 1 Part 1: Graphics Core, Section 7.18.3.7.1
> > > +* (Surface Arrays For all surfaces other than separate stencil
> > buffer):
> > > +*
> > > +* "[DevSNB] Errata: Sampler MSAA Qpitch will be 4 greater than the
> > value
> > > +*  calculated in the equation above , for every other odd Surface
> > Height
> > > +*  starting from 1 i.e. 1,5,9,13"
> > > +*
> > > +* Since this Qpitch errata only impacts the sampler, we have to
> > adjust the
> > > +* input for the rendering surface to achieve the same qpitch. For
> > the
> > > +* affected heights, we increment the height by 1 for the rendering
> > > +* surface.
> > > +*/
> > > +   if (GEN_GEN == 6 && (info->view->usage &
> > ISL_SURF_USAGE_RENDER_TARGET_BIT) &&
> > > +   info->surf->samples > 1 &&
> > > +   (info->surf->logical_level0_px.height % 4) == 1)
> > > +  s.Height++;
> > > +
> >
> > Ouch, that's a surprising workaround.
> >
> 
> Yes, yes it is.  We'll have to add a similar hack to isl's gen6 layout code.

I believe isl's gen6 layout code already handles this errata, though
I could be remembering incorrectly. Specifically, grep shows that the
prm quote is already present in isl.

> > Waiting to hear your reply on the G4X comment.

With your local G4X changes,
Reviewed-by: Chad Versace 
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 36/64] isl: Add support for filling out surface states all the way back to gen4

[Jason Ekstrand]

On Mon, Jun 20, 2016 at 3:53 PM, Chad Versace 
wrote:

> On Sat 11 Jun 2016, Jason Ekstrand wrote:
> > ---
> >  src/intel/isl/Makefile.am | 12 
> >  src/intel/isl/Makefile.sources| 13 +++--
> >  src/intel/isl/isl.c   | 28 +++
> >  src/intel/isl/isl_priv.h  | 24 
> >  src/intel/isl/isl_surface_state.c | 58
> ---
> >  5 files changed, 129 insertions(+), 6 deletions(-)
> >
> > diff --git a/src/intel/isl/Makefile.am b/src/intel/isl/Makefile.am
> > index 74f863a..ae367a9 100644
> > --- a/src/intel/isl/Makefile.am
> > +++ b/src/intel/isl/Makefile.am
> > @@ -22,6 +22,9 @@
> >  include Makefile.sources
> >
> >  ISL_GEN_LIBS =   \
> > + libisl-gen4.la   \
> > + libisl-gen5.la   \
> > + libisl-gen6.la   \
> >   libisl-gen7.la   \
> >   libisl-gen75.la  \
> >   libisl-gen8.la   \
> > @@ -52,6 +55,15 @@ libisl_la_LIBADD = $(ISL_GEN_LIBS)
> >
> >  libisl_la_SOURCES = $(ISL_FILES) $(ISL_GENERATED_FILES)
> >
> > +libisl_gen4_la_SOURCES = $(ISL_GEN4_FILES)
> > +libisl_gen4_la_CFLAGS = $(libisl_la_CFLAGS) -DGEN_VERSIONx10=40
> > +
> > +libisl_gen5_la_SOURCES = $(ISL_GEN5_FILES)
> > +libisl_gen5_la_CFLAGS = $(libisl_la_CFLAGS) -DGEN_VERSIONx10=50
> > +
> > +libisl_gen6_la_SOURCES = $(ISL_GEN6_FILES)
> > +libisl_gen6_la_CFLAGS = $(libisl_la_CFLAGS) -DGEN_VERSIONx10=60
> > +
> >  libisl_gen7_la_SOURCES = $(ISL_GEN7_FILES)
> >  libisl_gen7_la_CFLAGS = $(libisl_la_CFLAGS) -DGEN_VERSIONx10=70
> >
> > diff --git a/src/intel/isl/Makefile.sources
> b/src/intel/isl/Makefile.sources
> > index 89b1418..aa20ed4 100644
> > --- a/src/intel/isl/Makefile.sources
> > +++ b/src/intel/isl/Makefile.sources
> > @@ -2,12 +2,21 @@ ISL_FILES = \
> >   isl.c \
> >   isl.h \
> >   isl_format.c \
> > + isl_priv.h \
> > + isl_storage_image.c
> > +
> > +ISL_GEN4_FILES = \
> >   isl_gen4.c \
> >   isl_gen4.h \
> > + isl_surface_state.c
> > +
> > +ISL_GEN5_FILES = \
> > + isl_surface_state.c
> > +
> > +ISL_GEN6_FILES = \
> >   isl_gen6.c \
> >   isl_gen6.h \
> > - isl_priv.h \
> > - isl_storage_image.c
> > + isl_surface_state.c
> >
> >  ISL_GEN7_FILES = \
> >   isl_gen7.c \
> > diff --git a/src/intel/isl/isl.c b/src/intel/isl/isl.c
> > index 77b570d..7343a55 100644
> > --- a/src/intel/isl/isl.c
> > +++ b/src/intel/isl/isl.c
> > @@ -1191,6 +1191,20 @@ isl_surf_fill_state_s(const struct isl_device
> *dev, void *state,
> > }
> >
> > switch (ISL_DEV_GEN(dev)) {
> > +   case 4:
> > +  if (ISL_DEV_IS_G4X(dev)) {
> > + isl_gen4_surf_fill_state_s(dev, state, info);
> > +  } else {
> > + /* G45 surface state is the same as gen5 */
> > + isl_gen5_surf_fill_state_s(dev, state, info);
> > +  }
> > +  break;
>
> The above can't be correct, because...
>
>#define GEN_IS_G4X ((GEN_VERSIONx10) == 45)
>
> I think you meant this:
>
>if (ISL_DEV_IS_G4X(dev)) {
>   /* G45 surface state is the same as gen5 */
>   isl_gen5_surf_fill_state_s(dev, state, info);
>} else {
>   isl_gen4_surf_fill_state_s(dev, state, info);
>}
>

You are correct.  The reason it didn't trigger a bug is that the only
difference bertween gen4 and 4.5 is that x/y offsets are introduced in
4.5.  Since we never use ISL for filling out surfaces with x/y offsets on
gen4.5 with this series, I never caught it.  Thanks!


>
>
>
> > +   case 5:
> > +  isl_gen5_surf_fill_state_s(dev, state, info);
> > +  break;
> > +   case 6:
> > +  isl_gen6_surf_fill_state_s(dev, state, info);
> > +  break;
> > case 7:
> >if (ISL_DEV_IS_HASWELL(dev)) {
> >   isl_gen75_surf_fill_state_s(dev, state, info);
> > @@ -1214,6 +1228,20 @@ isl_buffer_fill_state_s(const struct isl_device
> *dev, void *state,
> >  const struct isl_buffer_fill_state_info
> *restrict info)
> >  {
> > switch (ISL_DEV_GEN(dev)) {
> > +   case 4:
> > +  if (ISL_DEV_IS_G4X(dev)) {
> > + isl_gen4_buffer_fill_state_s(state, info);
> > +  } else {
> > + /* G45 surface state is the same as gen5 */
> > + isl_gen5_buffer_fill_state_s(state, info);
> > +  }
> > +  break;
>
> Same as above.
>

My local fix just changes buffers to use the gen5 version for 4, 4.5, and 5


>
> > +   case 5:
> > +  isl_gen5_buffer_fill_state_s(state, info);
> > +  break;
> > +   case 6:
> > +  isl_gen6_buffer_fill_state_s(state, info);
> > +  break;
> > case 7:
> >if (ISL_DEV_IS_HASWELL(dev)) {
> >   isl_gen75_buffer_fill_state_s(state, info);
> > diff --git a/src/intel/isl/isl_priv.h b/src/intel/isl/isl_priv.h
> > index 

Re: [Mesa-dev] [PATCH 36/64] isl: Add support for filling out surface states all the way back to gen4

[Chad Versace]

On Sat 11 Jun 2016, Jason Ekstrand wrote:
> ---
>  src/intel/isl/Makefile.am | 12 
>  src/intel/isl/Makefile.sources| 13 +++--
>  src/intel/isl/isl.c   | 28 +++
>  src/intel/isl/isl_priv.h  | 24 
>  src/intel/isl/isl_surface_state.c | 58 
> ---
>  5 files changed, 129 insertions(+), 6 deletions(-)
> 
> diff --git a/src/intel/isl/Makefile.am b/src/intel/isl/Makefile.am
> index 74f863a..ae367a9 100644
> --- a/src/intel/isl/Makefile.am
> +++ b/src/intel/isl/Makefile.am
> @@ -22,6 +22,9 @@
>  include Makefile.sources
>  
>  ISL_GEN_LIBS =   \
> + libisl-gen4.la   \
> + libisl-gen5.la   \
> + libisl-gen6.la   \
>   libisl-gen7.la   \
>   libisl-gen75.la  \
>   libisl-gen8.la   \
> @@ -52,6 +55,15 @@ libisl_la_LIBADD = $(ISL_GEN_LIBS)
>  
>  libisl_la_SOURCES = $(ISL_FILES) $(ISL_GENERATED_FILES)
>  
> +libisl_gen4_la_SOURCES = $(ISL_GEN4_FILES)
> +libisl_gen4_la_CFLAGS = $(libisl_la_CFLAGS) -DGEN_VERSIONx10=40
> +
> +libisl_gen5_la_SOURCES = $(ISL_GEN5_FILES)
> +libisl_gen5_la_CFLAGS = $(libisl_la_CFLAGS) -DGEN_VERSIONx10=50
> +
> +libisl_gen6_la_SOURCES = $(ISL_GEN6_FILES)
> +libisl_gen6_la_CFLAGS = $(libisl_la_CFLAGS) -DGEN_VERSIONx10=60
> +
>  libisl_gen7_la_SOURCES = $(ISL_GEN7_FILES)
>  libisl_gen7_la_CFLAGS = $(libisl_la_CFLAGS) -DGEN_VERSIONx10=70
>  
> diff --git a/src/intel/isl/Makefile.sources b/src/intel/isl/Makefile.sources
> index 89b1418..aa20ed4 100644
> --- a/src/intel/isl/Makefile.sources
> +++ b/src/intel/isl/Makefile.sources
> @@ -2,12 +2,21 @@ ISL_FILES = \
>   isl.c \
>   isl.h \
>   isl_format.c \
> + isl_priv.h \
> + isl_storage_image.c
> +
> +ISL_GEN4_FILES = \
>   isl_gen4.c \
>   isl_gen4.h \
> + isl_surface_state.c
> +
> +ISL_GEN5_FILES = \
> + isl_surface_state.c
> +
> +ISL_GEN6_FILES = \
>   isl_gen6.c \
>   isl_gen6.h \
> - isl_priv.h \
> - isl_storage_image.c
> + isl_surface_state.c
>  
>  ISL_GEN7_FILES = \
>   isl_gen7.c \
> diff --git a/src/intel/isl/isl.c b/src/intel/isl/isl.c
> index 77b570d..7343a55 100644
> --- a/src/intel/isl/isl.c
> +++ b/src/intel/isl/isl.c
> @@ -1191,6 +1191,20 @@ isl_surf_fill_state_s(const struct isl_device *dev, 
> void *state,
> }
>  
> switch (ISL_DEV_GEN(dev)) {
> +   case 4:
> +  if (ISL_DEV_IS_G4X(dev)) {
> + isl_gen4_surf_fill_state_s(dev, state, info);
> +  } else {
> + /* G45 surface state is the same as gen5 */
> + isl_gen5_surf_fill_state_s(dev, state, info);
> +  }
> +  break;

The above can't be correct, because...

   #define GEN_IS_G4X ((GEN_VERSIONx10) == 45)

I think you meant this:

   if (ISL_DEV_IS_G4X(dev)) {
  /* G45 surface state is the same as gen5 */
  isl_gen5_surf_fill_state_s(dev, state, info);
   } else {
  isl_gen4_surf_fill_state_s(dev, state, info);
   }



> +   case 5:
> +  isl_gen5_surf_fill_state_s(dev, state, info);
> +  break;
> +   case 6:
> +  isl_gen6_surf_fill_state_s(dev, state, info);
> +  break;
> case 7:
>if (ISL_DEV_IS_HASWELL(dev)) {
>   isl_gen75_surf_fill_state_s(dev, state, info);
> @@ -1214,6 +1228,20 @@ isl_buffer_fill_state_s(const struct isl_device *dev, 
> void *state,
>  const struct isl_buffer_fill_state_info *restrict 
> info)
>  {
> switch (ISL_DEV_GEN(dev)) {
> +   case 4:
> +  if (ISL_DEV_IS_G4X(dev)) {
> + isl_gen4_buffer_fill_state_s(state, info);
> +  } else {
> + /* G45 surface state is the same as gen5 */
> + isl_gen5_buffer_fill_state_s(state, info);
> +  }
> +  break;

Same as above.

> +   case 5:
> +  isl_gen5_buffer_fill_state_s(state, info);
> +  break;
> +   case 6:
> +  isl_gen6_buffer_fill_state_s(state, info);
> +  break;
> case 7:
>if (ISL_DEV_IS_HASWELL(dev)) {
>   isl_gen75_buffer_fill_state_s(state, info);
> diff --git a/src/intel/isl/isl_priv.h b/src/intel/isl/isl_priv.h
> index d98e707..3a7af1a 100644
> --- a/src/intel/isl/isl_priv.h
> +++ b/src/intel/isl/isl_priv.h
> @@ -136,6 +136,18 @@ isl_extent3d_el_to_sa(enum isl_format fmt, struct 
> isl_extent3d extent_el)
>  }
>  
>  void
> +isl_gen4_surf_fill_state_s(const struct isl_device *dev, void *state,
> +   const struct isl_surf_fill_state_info *restrict 
> info);
> +
> +void
> +isl_gen5_surf_fill_state_s(const struct isl_device *dev, void *state,
> +   const struct isl_surf_fill_state_info *restrict 
> info);
> +
> +void
> +isl_gen6_surf_fill_state_s(const struct isl_device *dev, void *state,
> +   const struct 

[Mesa-dev] [PATCH 36/64] isl: Add support for filling out surface states all the way back to gen4

[Jason Ekstrand]

---
 src/intel/isl/Makefile.am | 12 
 src/intel/isl/Makefile.sources| 13 +++--
 src/intel/isl/isl.c   | 28 +++
 src/intel/isl/isl_priv.h  | 24 
 src/intel/isl/isl_surface_state.c | 58 ---
 5 files changed, 129 insertions(+), 6 deletions(-)

diff --git a/src/intel/isl/Makefile.am b/src/intel/isl/Makefile.am
index 74f863a..ae367a9 100644
--- a/src/intel/isl/Makefile.am
+++ b/src/intel/isl/Makefile.am
@@ -22,6 +22,9 @@
 include Makefile.sources
 
 ISL_GEN_LIBS =   \
+   libisl-gen4.la   \
+   libisl-gen5.la   \
+   libisl-gen6.la   \
libisl-gen7.la   \
libisl-gen75.la  \
libisl-gen8.la   \
@@ -52,6 +55,15 @@ libisl_la_LIBADD = $(ISL_GEN_LIBS)
 
 libisl_la_SOURCES = $(ISL_FILES) $(ISL_GENERATED_FILES)
 
+libisl_gen4_la_SOURCES = $(ISL_GEN4_FILES)
+libisl_gen4_la_CFLAGS = $(libisl_la_CFLAGS) -DGEN_VERSIONx10=40
+
+libisl_gen5_la_SOURCES = $(ISL_GEN5_FILES)
+libisl_gen5_la_CFLAGS = $(libisl_la_CFLAGS) -DGEN_VERSIONx10=50
+
+libisl_gen6_la_SOURCES = $(ISL_GEN6_FILES)
+libisl_gen6_la_CFLAGS = $(libisl_la_CFLAGS) -DGEN_VERSIONx10=60
+
 libisl_gen7_la_SOURCES = $(ISL_GEN7_FILES)
 libisl_gen7_la_CFLAGS = $(libisl_la_CFLAGS) -DGEN_VERSIONx10=70
 
diff --git a/src/intel/isl/Makefile.sources b/src/intel/isl/Makefile.sources
index 89b1418..aa20ed4 100644
--- a/src/intel/isl/Makefile.sources
+++ b/src/intel/isl/Makefile.sources
@@ -2,12 +2,21 @@ ISL_FILES = \
isl.c \
isl.h \
isl_format.c \
+   isl_priv.h \
+   isl_storage_image.c
+
+ISL_GEN4_FILES = \
isl_gen4.c \
isl_gen4.h \
+   isl_surface_state.c
+
+ISL_GEN5_FILES = \
+   isl_surface_state.c
+
+ISL_GEN6_FILES = \
isl_gen6.c \
isl_gen6.h \
-   isl_priv.h \
-   isl_storage_image.c
+   isl_surface_state.c
 
 ISL_GEN7_FILES = \
isl_gen7.c \
diff --git a/src/intel/isl/isl.c b/src/intel/isl/isl.c
index 77b570d..7343a55 100644
--- a/src/intel/isl/isl.c
+++ b/src/intel/isl/isl.c
@@ -1191,6 +1191,20 @@ isl_surf_fill_state_s(const struct isl_device *dev, void 
*state,
}
 
switch (ISL_DEV_GEN(dev)) {
+   case 4:
+  if (ISL_DEV_IS_G4X(dev)) {
+ isl_gen4_surf_fill_state_s(dev, state, info);
+  } else {
+ /* G45 surface state is the same as gen5 */
+ isl_gen5_surf_fill_state_s(dev, state, info);
+  }
+  break;
+   case 5:
+  isl_gen5_surf_fill_state_s(dev, state, info);
+  break;
+   case 6:
+  isl_gen6_surf_fill_state_s(dev, state, info);
+  break;
case 7:
   if (ISL_DEV_IS_HASWELL(dev)) {
  isl_gen75_surf_fill_state_s(dev, state, info);
@@ -1214,6 +1228,20 @@ isl_buffer_fill_state_s(const struct isl_device *dev, 
void *state,
 const struct isl_buffer_fill_state_info *restrict info)
 {
switch (ISL_DEV_GEN(dev)) {
+   case 4:
+  if (ISL_DEV_IS_G4X(dev)) {
+ isl_gen4_buffer_fill_state_s(state, info);
+  } else {
+ /* G45 surface state is the same as gen5 */
+ isl_gen5_buffer_fill_state_s(state, info);
+  }
+  break;
+   case 5:
+  isl_gen5_buffer_fill_state_s(state, info);
+  break;
+   case 6:
+  isl_gen6_buffer_fill_state_s(state, info);
+  break;
case 7:
   if (ISL_DEV_IS_HASWELL(dev)) {
  isl_gen75_buffer_fill_state_s(state, info);
diff --git a/src/intel/isl/isl_priv.h b/src/intel/isl/isl_priv.h
index d98e707..3a7af1a 100644
--- a/src/intel/isl/isl_priv.h
+++ b/src/intel/isl/isl_priv.h
@@ -136,6 +136,18 @@ isl_extent3d_el_to_sa(enum isl_format fmt, struct 
isl_extent3d extent_el)
 }
 
 void
+isl_gen4_surf_fill_state_s(const struct isl_device *dev, void *state,
+   const struct isl_surf_fill_state_info *restrict 
info);
+
+void
+isl_gen5_surf_fill_state_s(const struct isl_device *dev, void *state,
+   const struct isl_surf_fill_state_info *restrict 
info);
+
+void
+isl_gen6_surf_fill_state_s(const struct isl_device *dev, void *state,
+   const struct isl_surf_fill_state_info *restrict 
info);
+
+void
 isl_gen7_surf_fill_state_s(const struct isl_device *dev, void *state,
const struct isl_surf_fill_state_info *restrict 
info);
 
@@ -150,6 +162,18 @@ isl_gen9_surf_fill_state_s(const struct isl_device *dev, 
void *state,
const struct isl_surf_fill_state_info *restrict 
info);
 
 void
+isl_gen4_buffer_fill_state_s(void *state,
+ const struct isl_buffer_fill_state_info *restrict 
info);
+
+void
+isl_gen5_buffer_fill_state_s(void *state,
+ const struct