Re: [Mesa-dev] [PATCH 5/7] i965: use _mesa_geometry_width/height/layers/samples for programming geometry of framebuffer to GEN

2015-04-28 Thread Rogovin, Kevin 
Hello,


 No, because the non-shared code is (by your own admission) untested and/or 
 dead code.  Untested code is broken code.  I would personally be ok with a 
 lot  of the changes that just replace fb-Width with
 _mesa_geometric_width(fb) since it's effectively just replacing a direct 
 access with a getter.  However, almost half of the patch is updating the 
 upload_sf_vp  function which is only used for gen = 5.  A comment or assert 
 there would be sufficient rather than reworking it.

Fair enough. Would the following be good:
 - keep all those that replace fb-whatever with _mesa_geomety_whatever,
 - instead of the ick I have done to upload_sf_vp, place a big comment warning

I would be happy with the above  as it addresses my main concern and the 
dead-is-broken code concern as well. If I had physical access to a Gen4 and 5 
box I would test it and if it worked, enable the extension on those platforms 
as well.

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

Re: [Mesa-dev] [PATCH 5/7] i965: use _mesa_geometry_width/height/layers/samples for programming geometry of framebuffer to GEN

2015-04-28 Thread Jason Ekstrand 
On Tue, Apr 28, 2015 at 6:17 PM, Rogovin, Kevin kevin.rogo...@intel.com wrote:
 Hello,


 No, because the non-shared code is (by your own admission) untested and/or 
 dead code.  Untested code is broken code.  I would personally be ok with a 
 lot  of the changes that just replace fb-Width with
 _mesa_geometric_width(fb) since it's effectively just replacing a direct 
 access with a getter.  However, almost half of the patch is updating the 
 upload_sf_vp  function which is only used for gen = 5.  A comment or 
 assert there would be sufficient rather than reworking it.

 Fair enough. Would the following be good:
  - keep all those that replace fb-whatever with _mesa_geomety_whatever,
  - instead of the ick I have done to upload_sf_vp, place a big comment warning

Yes, I think that would be sufficient.

 I would be happy with the above  as it addresses my main concern and the 
 dead-is-broken code concern as well. If I had physical access to a Gen4 and 5 
 box I would test it and if it worked, enable the extension on those platforms 
 as well.

I don't think there's any good reason to turn it on for Gen5 or older.
However, we should still test it because it does touch code that hits
those platforms.  Testing across all the hardware can be done fairly
easily by pushing to our Jenkins system.  I know that Martin and Topi
(and probably Curro) have accounts and could run it easily enough.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 5/7] i965: use _mesa_geometry_width/height/layers/samples for programming geometry of framebuffer to GEN

2015-04-28 Thread Jason Ekstrand 
On Tue, Apr 28, 2015 at 3:37 PM, Rogovin, Kevin kevin.rogo...@intel.com wrote:

 I read the patch again and I'm still in the opinion that the changes to the
 pure pre-gen7 logic (i.e., logic that is not re-used for later gens) are not 
 needed.

 As I have tried and apparently failed to communicate, it is -better- and more 
 consistent. Need
 is a far stronger word. Without a doubt, if the extension is never enabled 
 for those older
 Gens, then it does not matter in terms of produced output. However, I stated 
 that it leaves
 a trap and an inconsistency which I find quite bothering.

You have very clearly communicated that you *think* it's better to
change it everywhere.  Others have chosen to disagree for a variety of
reasons.  Defending your choice to the death isn't aiding in the
discussion.

 The shared logic between pre-gen7 and later, namely setup for renderbuffers, 
 drawing rectangle and
 fragment shader compilation key are safe to do as they only introduce new 
 logic that is conditional to
 no-attachments being used.

 And that is exactly for the case for that code that is not shared. Indeed, if 
 the shared code is safe
 for pre-Gen7, then so is the non-shared code.

No, because the non-shared code is (by your own admission) untested
and/or dead code.  Untested code is broken code.  I would personally
be ok with a lot of the changes that just replace fb-Width with
_mesa_geometric_width(fb) since it's effectively just replacing a
direct access with a getter.  However, almost half of the patch is
updating the upload_sf_vp function which is only used for gen = 5.  A
comment or assert there would be sufficient rather than reworking it.

 Your concern about the readers getting confused could be also addressed with 
 assert(brw-gen = 7)
  and a comment saying that the no-attachment specific path is not applicable 
 for older gens.

 There is only one occurrence of no-attachment specific code paths in these 
 i965 patches
 and that is associated to scissoring.  The rest is existing code is changed 
 from accessing Width,
 Height of gl_framebuffer to getting those values from a function. There is 
 no proper place
 to insert an assert(brw-gen =7 ), since, with the exception of the 
 scissoring (and it is just
 one if block) there is no such no attachment code path. I had thought the 
 diffs of the series
 made that quite clear.

 And when it comes to the pure pre-gen7 logic, I, in fact, have just the 
 opposite opinion on making it to go through the no-attachment-aware path.
 As the extension is not possible for older gens, I find it clearer that 
 logic explicitly by-passes such paths that even consider it.

 Um, I am pretty sure than pre Gen7 hardware can do the extension. The crux is 
 that the extension
 is pointless for such hardware because pre Gen7 hardware does not (AFAIK) 
 have a feature that
 allows for a fragment shader to have a side effect. Even that statement is 
 not totally true. Indeed,
 one can argue performance queries and occlusion queries with 
 framebuffer_no_attachments
 make some form of sense (it would give an application a count of sorts).

That's a contingency I think we can ignore for the moment.  If someone
really wants to do occlusion queries with no framebuffer on ILK, we
can add it then.  Until that unlikely event happens, let's concentrate
on HW that at least exposes atomics.
--Jason
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 5/7] i965: use _mesa_geometry_width/height/layers/samples for programming geometry of framebuffer to GEN

2015-04-28 Thread Rogovin, Kevin 

 I read the patch again and I'm still in the opinion that the changes to the 
 pure pre-gen7 logic (i.e., logic that is not re-used for later gens) are not 
 needed.

As I have tried and apparently failed to communicate, it is -better- and more 
consistent. Need 
is a far stronger word. Without a doubt, if the extension is never enabled for 
those older
Gens, then it does not matter in terms of produced output. However, I stated 
that it leaves 
a trap and an inconsistency which I find quite bothering.

 The shared logic between pre-gen7 and later, namely setup for renderbuffers, 
 drawing rectangle and 
 fragment shader compilation key are safe to do as they only introduce new 
 logic that is conditional to 
 no-attachments being used.

And that is exactly for the case for that code that is not shared. Indeed, if 
the shared code is safe 
for pre-Gen7, then so is the non-shared code. 

 Your concern about the readers getting confused could be also addressed with 
 assert(brw-gen = 7) 
  and a comment saying that the no-attachment specific path is not applicable 
 for older gens.

There is only one occurrence of no-attachment specific code paths in these 
i965 patches
and that is associated to scissoring.  The rest is existing code is changed 
from accessing Width, 
Height of gl_framebuffer to getting those values from a function. There is no 
proper place
to insert an assert(brw-gen =7 ), since, with the exception of the scissoring 
(and it is just
one if block) there is no such no attachment code path. I had thought the 
diffs of the series
made that quite clear.

 And when it comes to the pure pre-gen7 logic, I, in fact, have just the 
 opposite opinion on making it to go through the no-attachment-aware path.
 As the extension is not possible for older gens, I find it clearer that logic 
 explicitly by-passes such paths that even consider it.

Um, I am pretty sure than pre Gen7 hardware can do the extension. The crux is 
that the extension
is pointless for such hardware because pre Gen7 hardware does not (AFAIK) have 
a feature that
allows for a fragment shader to have a side effect. Even that statement is not 
totally true. Indeed,
one can argue performance queries and occlusion queries with 
framebuffer_no_attachments 
make some form of sense (it would give an application a count of sorts). 

-Kevin




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

Re: [Mesa-dev] [PATCH 5/7] i965: use _mesa_geometry_width/height/layers/samples for programming geometry of framebuffer to GEN

2015-04-28 Thread Pohjolainen, Topi 
On Tue, Apr 28, 2015 at 10:28:17AM +0300, Rogovin, Kevin wrote:
 Hi, 
 
 A couple of us chatted about this, and we all agreed that it's probably not 
 useful to 
 enable ARB_framebuffer_no_attachments on pre-Gen7.  We don't expose atomics, 
 SSBOs, 
  or image load/store on those platforms (and probably won't), so the 
  only way fragment shaders can output any data is via framebuffer writes.
 
  So I'd be inclined to just not touch the pre-Gen7 code at all.
 
 There are a number of issues lurking. 
 
 Firstly, there are atoms from Gen4 (for example brw_drawing_rect) 
 which are used all the way to Gen8.  Therefore, Gen4 code must be 
 changed. At this point then one can advocate to only change those 
 atoms that are active in Gen7 and Gen8. If that is done, then there 
 is an style inconsistency where some atoms , for Gen4,5 and 6 use 
 the new functions and some do not. That is just icky in my opinion,
 as it  leads to the awkward question why? is there something 
 delicate here? Compounding the style issue is that the code
 should -convey- what it being done to the reader what is going on.
 Using the functions makes the convey more trivial for the reader
 to know.
 
 Secondly, not using those functions for Gen4,5  and 6 leaves the code 
 with a trap for  later. Namely that trap if someone says you know 
 although the extension is silly for Gens 4,5 and 6, it still is trivial to 
 enable. I hate leaving traps behind for others.
 
 Thirdly, there is the real meat of reviewing patch 6: a good review 
 of that patch will make sure that any remaining references to 
 gl_framebuffer::Width, Height, and so on are correct (i.e. the code 
 requires the dimension of the backing store and not the geometry).
 That checking is made easier, more mechanical if -all- of i965 is made 
 consistent in this fashion, for otherwise the checker has more to check 
 (i.e. instead of is this for setting rasterizer it is setting rasterizer and 
 active for Gen7 and 8). 
 
 When I was making these changes to i965 I was also tempted
 to add a functions of the sort _mesa_buffer_width, that
 returned gl_fraembufffer::Width regardless of the value
 of gl_framebuffer::_HasAttachments. The reason why I was
 tempted was to help (via lovely grep) to make patch 6 and 
 the reviewing of it more mechanical and easier. but I chose 
 not to since the meaning of the fields from gl_framebuffer
 became the exact meaning of those fields.  That third 
 reason is the one reason that patch 6 should make people
 itch (in my opinion): where all references hit? Checking
 that require more than just checking the diff, that requires
 knowing all the references to gl_framebuffer::Width and
 friends, knowing the use there and then checking if the
 patch does or DOES NOT change that code block 
 appropriately. 
 

I read the patch again and I'm still in the opinion that the changes
to the pure pre-gen7 logic (i.e., logic that is not re-used for later gens)
are not needed.

The shared logic between pre-gen7 and later, namely setup for renderbuffers,
drawing rectangle and fragment shader compilation key are safe to do as
they only introduce new logic that is conditional to no-attachments being
used.
Your concern about the readers getting confused could be also addressed
with assert(brw-gen = 7) and a comment saying that the no-attachment
specific path is not applicable for older gens.

And when it comes to the pure pre-gen7 logic, I, in fact, have just the
opposite opinion on making it to go through the no-attachment-aware path.
As the extension is not possible for older gens, I find it clearer that logic
explicitly by-passes such paths that even consider it.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 5/7] i965: use _mesa_geometry_width/height/layers/samples for programming geometry of framebuffer to GEN

2015-04-28 Thread Rogovin, Kevin 
Hi, 

A couple of us chatted about this, and we all agreed that it's probably not 
useful to 
enable ARB_framebuffer_no_attachments on pre-Gen7.  We don't expose atomics, 
SSBOs, 
 or image load/store on those platforms (and probably won't), so the 
 only way fragment shaders can output any data is via framebuffer writes.

 So I'd be inclined to just not touch the pre-Gen7 code at all.

There are a number of issues lurking. 

Firstly, there are atoms from Gen4 (for example brw_drawing_rect) 
which are used all the way to Gen8.  Therefore, Gen4 code must be 
changed. At this point then one can advocate to only change those 
atoms that are active in Gen7 and Gen8. If that is done, then there 
is an style inconsistency where some atoms , for Gen4,5 and 6 use 
the new functions and some do not. That is just icky in my opinion,
as it  leads to the awkward question why? is there something 
delicate here? Compounding the style issue is that the code
should -convey- what it being done to the reader what is going on.
Using the functions makes the convey more trivial for the reader
to know.

Secondly, not using those functions for Gen4,5  and 6 leaves the code 
with a trap for  later. Namely that trap if someone says you know 
although the extension is silly for Gens 4,5 and 6, it still is trivial to 
enable. I hate leaving traps behind for others.

Thirdly, there is the real meat of reviewing patch 6: a good review 
of that patch will make sure that any remaining references to 
gl_framebuffer::Width, Height, and so on are correct (i.e. the code 
requires the dimension of the backing store and not the geometry).
That checking is made easier, more mechanical if -all- of i965 is made 
consistent in this fashion, for otherwise the checker has more to check 
(i.e. instead of is this for setting rasterizer it is setting rasterizer and 
active for Gen7 and 8). 

When I was making these changes to i965 I was also tempted
to add a functions of the sort _mesa_buffer_width, that
returned gl_fraembufffer::Width regardless of the value
of gl_framebuffer::_HasAttachments. The reason why I was
tempted was to help (via lovely grep) to make patch 6 and 
the reviewing of it more mechanical and easier. but I chose 
not to since the meaning of the fields from gl_framebuffer
became the exact meaning of those fields.  That third 
reason is the one reason that patch 6 should make people
itch (in my opinion): where all references hit? Checking
that require more than just checking the diff, that requires
knowing all the references to gl_framebuffer::Width and
friends, knowing the use there and then checking if the
patch does or DOES NOT change that code block 
appropriately. 

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

Re: [Mesa-dev] [PATCH 5/7] i965: use _mesa_geometry_width/height/layers/samples for programming geometry of framebuffer to GEN

2015-04-27 Thread Rogovin, Kevin 
Why make it two separate patches even? Seems silly (not to mention more work 
really). I guess the issue is the commit message portion saying for 
framebuffer_no_attachements.. perhaps I just change the commit to use new 
geometry function to indicate want geometry not buffer thingies.
 -Kevin

-Original Message-
From: Pohjolainen, Topi 
Sent: Monday, April 27, 2015 10:43 AM
To: Rogovin, Kevin
Cc: mesa-...@freedesktop.org
Subject: Re: [Mesa-dev] [PATCH 5/7] i965: use 
_mesa_geometry_width/height/layers/samples for programming geometry of 
framebuffer to GEN

On Fri, Apr 24, 2015 at 11:36:27PM +0300, Rogovin, Kevin wrote:
 I want to add one more comment on why to replace with the _mesa_geometry_ 
 functions, which I had thought was so obvious I neglected to mention it:
 
 With this series the meaning of gl_framebuffer::Width, Height, and so on have 
 a different meaning. They give the intersection of the backing stores of the 
 render targets. In contrast, the _mesa_geometry_* functions give the geometry 
 to feed a rasterizer/windower. By using _mesa_geometry_* functions the code 
 communicates clearly it wants the geometry to feed windower/rasterizer and 
 not the geometry of the intersection of the (potentially empty) set of 
 backing stores. Moreover, it is better to be consistent as well, as later 
 someone will likely wonder: why in Gen7 and higher are those _mesa_geometry 
 functions used but not before? That question has no good answer because it 
 does not make sense to not use those functions when programming the 
 rasterizer/windower thingies.
 

Could we split the patch in two? One part dealing with the necessary needed for 
gen7 and higher to support no_attachments and then another just making older 
gens consistent.

 -Kevin
 
 -Original Message-
 From: Rogovin, Kevin
 Sent: Friday, April 24, 2015 7:43 PM
 To: Pohjolainen, Topi
 Cc: mesa-...@freedesktop.org
 Subject: RE: [Mesa-dev] [PATCH 5/7] i965: use 
 _mesa_geometry_width/height/layers/samples for programming geometry of 
 framebuffer to GEN
 
 
  My point specifically was that you are also updating atoms that _are not_ 
  re-used. 
  And as those changes are not really needed, I wouldn't take the risk 
  of changing something in vain. I would introduce them only when you have 
  patches to really enable older generations.
 
 My take is the following:
 
  1. Tracking (and guaranteeing) that those function left unchanged as is are 
 exactly just those for before Gen7 is a pain. Much easier, and more reliable 
 to hit them all instead. A significant number of functions in i965 are not 
 emit functions of any atom but emit functions of atoms map to them. Again, 
 more reliable and -safer- to change them all, then just the bare minimum. 
 
 2. The change is benign. If _HasAttachments is true, then the function 
 substitution gives the same value. For Gens not supporting the extension 
 there is no effect.
 
 3. Lastly, as stated: for later it leaves the option to enable it for Gen6 
 and below, it is just trivial change, but it needs testing on hardware.
 
 When I writing this work, I originally had it for all Gens, but changed to 
 support only Gen7and higher because that is all on which I can test it. 
 
 -Kevin
  
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 5/7] i965: use _mesa_geometry_width/height/layers/samples for programming geometry of framebuffer to GEN

2015-04-27 Thread Pohjolainen, Topi 
On Fri, Apr 24, 2015 at 11:36:27PM +0300, Rogovin, Kevin wrote:
 I want to add one more comment on why to replace with the _mesa_geometry_ 
 functions, which I had thought was so obvious I neglected to mention it:
 
 With this series the meaning of gl_framebuffer::Width, Height, and so on have 
 a different meaning. They give the intersection of the backing stores of the 
 render targets. In contrast, the _mesa_geometry_* functions give the geometry 
 to feed a rasterizer/windower. By using _mesa_geometry_* functions the code 
 communicates clearly it wants the geometry to feed windower/rasterizer and 
 not the geometry of the intersection of the (potentially empty) set of 
 backing stores. Moreover, it is better to be consistent as well, as later 
 someone will likely wonder: why in Gen7 and higher are those _mesa_geometry 
 functions used but not before? That question has no good answer because it 
 does not make sense to not use those functions when programming the 
 rasterizer/windower thingies.
 

Could we split the patch in two? One part dealing with the necessary needed
for gen7 and higher to support no_attachments and then another just making
older gens consistent.

 -Kevin
 
 -Original Message-
 From: Rogovin, Kevin 
 Sent: Friday, April 24, 2015 7:43 PM
 To: Pohjolainen, Topi
 Cc: mesa-...@freedesktop.org
 Subject: RE: [Mesa-dev] [PATCH 5/7] i965: use 
 _mesa_geometry_width/height/layers/samples for programming geometry of 
 framebuffer to GEN
 
 
  My point specifically was that you are also updating atoms that _are not_ 
  re-used. 
  And as those changes are not really needed, I wouldn't take the risk 
  of changing something in vain. I would introduce them only when you have 
  patches to really enable older generations.
 
 My take is the following:
 
  1. Tracking (and guaranteeing) that those function left unchanged as is are 
 exactly just those for before Gen7 is a pain. Much easier, and more reliable 
 to hit them all instead. A significant number of functions in i965 are not 
 emit functions of any atom but emit functions of atoms map to them. Again, 
 more reliable and -safer- to change them all, then just the bare minimum. 
 
 2. The change is benign. If _HasAttachments is true, then the function 
 substitution gives the same value. For Gens not supporting the extension 
 there is no effect.
 
 3. Lastly, as stated: for later it leaves the option to enable it for Gen6 
 and below, it is just trivial change, but it needs testing on hardware.
 
 When I writing this work, I originally had it for all Gens, but changed to 
 support only Gen7and higher because that is all on which I can test it. 
 
 -Kevin
  
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 5/7] i965: use _mesa_geometry_width/height/layers/samples for programming geometry of framebuffer to GEN

2015-04-27 Thread Kenneth Graunke 
On Friday, April 24, 2015 04:02:18 PM Rogovin, Kevin wrote:
 
  Actually I realized that you add quite a bit of support to gen4-6 logic 
  that 
  _isn't_ used for gen7 and higher. In the last patch of the series you claim 
  to enable this only for gen7 and higher - I'm confused.
 
 There are two reasons:
 1. Because atoms get reused all the time across generations, it is just 
 easier to use
 the _mesa_geomety_* functions in any batch buffer builder that is concerned 
 about the geometry of the render target. It keeps  the code consistent and 
 much
 easier than checking what functions and atoms are directly or indirectly used 
 by 
 different Gens. However,  blorp, blitting and a few others are left untouched 
 since 
 they want to talk about the buffer, not really 3D pipeline rasterization 
 things. 
 
 2. At first I was going to support pre Gen7 hardware with the series. However,
 I do not have hardware on which to test it. In truth I want this to also run 
 on 
 pre-Gen7, but without testing on device, I cannot vouch for the patches. 
 I believe it should just work for pre Gen7 (by just tweaking the last patch 
 to 
 enable it on pre Gen7), but I would rather be careful than in this case. I 
 also 
 confess, it is a silly extension for pre Gen7 anyways...
 
  -Kevin

A couple of us chatted about this, and we all agreed that it's probably
not useful to enable ARB_framebuffer_no_attachments on pre-Gen7.  We
don't expose atomics, SSBOs, or image load/store on those platforms (and
probably won't), so the only way fragment shaders can output any data is
via framebuffer writes.

So I'd be inclined to just not touch the pre-Gen7 code at all.


signature.asc
Description: This is a digitally signed message part.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 5/7] i965: use _mesa_geometry_width/height/layers/samples for programming geometry of framebuffer to GEN

2015-04-24 Thread kevin . rogovin 
From: Kevin Rogovin kevin.rogo...@intel.com

To prepare for i965 to support ARB_framebuffer_no_attachment, use
the convenience functions mesa_geometry_width/height/layers/samples
to specify the geometry of the render target surfaces to the GPU.


---
 src/mesa/drivers/dri/i965/brw_clip_state.c |  9 -
 src/mesa/drivers/dri/i965/brw_misc_state.c | 12 --
 src/mesa/drivers/dri/i965/brw_sf_state.c   | 46 --
 src/mesa/drivers/dri/i965/brw_state_upload.c   |  7 +++-
 src/mesa/drivers/dri/i965/brw_wm.c |  7 ++--
 src/mesa/drivers/dri/i965/brw_wm_surface_state.c   | 13 --
 src/mesa/drivers/dri/i965/gen6_clip_state.c| 11 --
 src/mesa/drivers/dri/i965/gen6_multisample_state.c |  3 +-
 src/mesa/drivers/dri/i965/gen6_scissor_state.c | 14 +--
 src/mesa/drivers/dri/i965/gen6_sf_state.c  |  3 +-
 src/mesa/drivers/dri/i965/gen6_viewport_state.c|  3 +-
 src/mesa/drivers/dri/i965/gen6_wm_state.c  |  3 +-
 src/mesa/drivers/dri/i965/gen7_sf_state.c  |  3 +-
 src/mesa/drivers/dri/i965/gen7_viewport_state.c|  3 +-
 src/mesa/drivers/dri/i965/gen7_wm_state.c  |  3 +-
 src/mesa/drivers/dri/i965/gen8_viewport_state.c|  9 +++--
 16 files changed, 108 insertions(+), 41 deletions(-)

diff --git a/src/mesa/drivers/dri/i965/brw_clip_state.c 
b/src/mesa/drivers/dri/i965/brw_clip_state.c
index 3223834..3aa679f 100644
--- a/src/mesa/drivers/dri/i965/brw_clip_state.c
+++ b/src/mesa/drivers/dri/i965/brw_clip_state.c
@@ -32,6 +32,7 @@
 #include brw_context.h
 #include brw_state.h
 #include brw_defines.h
+#include main/framebuffer.h
 
 static void
 upload_clip_vp(struct brw_context *brw)
@@ -60,6 +61,10 @@ brw_upload_clip_unit(struct brw_context *brw)
 
/* _NEW_BUFFERS */
struct gl_framebuffer *fb = ctx-DrawBuffer;
+   GLint fb_width, fb_height;
+
+   fb_width = _mesa_geometric_width(fb);
+   fb_height = _mesa_geometric_height(fb);
 
upload_clip_vp(brw);
 
@@ -127,8 +132,8 @@ brw_upload_clip_unit(struct brw_context *brw)
/* enable guardband clipping if we can */
if (ctx-ViewportArray[0].X == 0 
ctx-ViewportArray[0].Y == 0 
-   ctx-ViewportArray[0].Width == (float) fb-Width 
-   ctx-ViewportArray[0].Height == (float) fb-Height)
+   ctx-ViewportArray[0].Width == (float) fb_width 
+   ctx-ViewportArray[0].Height == (float) fb_height)
{
   clip-clip5.guard_band_enable = 1;
   clip-clip6.clipper_viewport_state_ptr =
diff --git a/src/mesa/drivers/dri/i965/brw_misc_state.c 
b/src/mesa/drivers/dri/i965/brw_misc_state.c
index 78a46cb..ef94a6e 100644
--- a/src/mesa/drivers/dri/i965/brw_misc_state.c
+++ b/src/mesa/drivers/dri/i965/brw_misc_state.c
@@ -39,6 +39,7 @@
 #include brw_state.h
 #include brw_defines.h
 
+#include main/framebuffer.h
 #include main/fbobject.h
 #include main/glformats.h
 
@@ -46,12 +47,17 @@
 static void upload_drawing_rect(struct brw_context *brw)
 {
struct gl_context *ctx = brw-ctx;
+   GLint fb_width, fb_height;
+   struct gl_framebuffer *fb = ctx-DrawBuffer;
+
+   fb_width = _mesa_geometric_width(fb);
+   fb_height = _mesa_geometric_height(fb);
 
BEGIN_BATCH(4);
OUT_BATCH(_3DSTATE_DRAWING_RECTANGLE  16 | (4 - 2));
OUT_BATCH(0); /* xmin, ymin */
-   OUT_BATCH(((ctx-DrawBuffer-Width - 1)  0x) |
-   ((ctx-DrawBuffer-Height - 1)  16));
+   OUT_BATCH(((fb_width - 1)  0x) |
+   ((fb_height - 1)  16));
OUT_BATCH(0);
ADVANCE_BATCH();
 }
@@ -767,7 +773,7 @@ static void upload_polygon_stipple_offset(struct 
brw_context *brw)
 * works just fine, and there's no window system to worry about.
 */
if (_mesa_is_winsys_fbo(ctx-DrawBuffer))
-  OUT_BATCH((32 - (ctx-DrawBuffer-Height  31))  31);
+  OUT_BATCH((32 - (_mesa_geometric_height(ctx-DrawBuffer)  31))  31);
else
   OUT_BATCH(0);
ADVANCE_BATCH();
diff --git a/src/mesa/drivers/dri/i965/brw_sf_state.c 
b/src/mesa/drivers/dri/i965/brw_sf_state.c
index 014b434..1fa3d44 100644
--- a/src/mesa/drivers/dri/i965/brw_sf_state.c
+++ b/src/mesa/drivers/dri/i965/brw_sf_state.c
@@ -35,6 +35,7 @@
 #include main/macros.h
 #include main/fbobject.h
 #include main/viewport.h
+#include main/framebuffer.h
 #include brw_context.h
 #include brw_state.h
 #include brw_defines.h
@@ -47,18 +48,42 @@ static void upload_sf_vp(struct brw_context *brw)
GLfloat y_scale, y_bias;
double scale[3], translate[3];
const bool render_to_fbo = _mesa_is_user_fbo(ctx-DrawBuffer);
+   GLint fb_width, fb_height, xmin, xmax, ymin, ymax;
 
sfv = brw_state_batch(brw, AUB_TRACE_SF_VP_STATE,
 sizeof(*sfv), 32, brw-sf.vp_offset);
memset(sfv, 0, sizeof(*sfv));
 
+   if (ctx-DrawBuffer-_HasAttachments) {
+  fb_width = ctx-DrawBuffer-Width;
+  fb_height = ctx-DrawBuffer-Height;
+  xmin = ctx-DrawBuffer-_Xmin;
+  xmax = ctx-DrawBuffer-_Xmax;
+  ymin = ctx-DrawBuffer-_Ymin;
+  ymax = ctx-DrawBuffer-_Ymax;
+

Re: [Mesa-dev] [PATCH 5/7] i965: use _mesa_geometry_width/height/layers/samples for programming geometry of framebuffer to GEN

2015-04-24 Thread Pohjolainen, Topi 
On Fri, Apr 24, 2015 at 09:59:08AM +0300, kevin.rogo...@intel.com wrote:
 From: Kevin Rogovin kevin.rogo...@intel.com
 
 To prepare for i965 to support ARB_framebuffer_no_attachment, use
 the convenience functions mesa_geometry_width/height/layers/samples
 to specify the geometry of the render target surfaces to the GPU.
 
 
 ---
  src/mesa/drivers/dri/i965/brw_clip_state.c |  9 -
  src/mesa/drivers/dri/i965/brw_misc_state.c | 12 --
  src/mesa/drivers/dri/i965/brw_sf_state.c   | 46 
 --
  src/mesa/drivers/dri/i965/brw_state_upload.c   |  7 +++-
  src/mesa/drivers/dri/i965/brw_wm.c |  7 ++--
  src/mesa/drivers/dri/i965/brw_wm_surface_state.c   | 13 --
  src/mesa/drivers/dri/i965/gen6_clip_state.c| 11 --
  src/mesa/drivers/dri/i965/gen6_multisample_state.c |  3 +-
  src/mesa/drivers/dri/i965/gen6_scissor_state.c | 14 +--
  src/mesa/drivers/dri/i965/gen6_sf_state.c  |  3 +-
  src/mesa/drivers/dri/i965/gen6_viewport_state.c|  3 +-
  src/mesa/drivers/dri/i965/gen6_wm_state.c  |  3 +-
  src/mesa/drivers/dri/i965/gen7_sf_state.c  |  3 +-
  src/mesa/drivers/dri/i965/gen7_viewport_state.c|  3 +-
  src/mesa/drivers/dri/i965/gen7_wm_state.c  |  3 +-
  src/mesa/drivers/dri/i965/gen8_viewport_state.c|  9 +++--
  16 files changed, 108 insertions(+), 41 deletions(-)
 
 diff --git a/src/mesa/drivers/dri/i965/brw_clip_state.c 
 b/src/mesa/drivers/dri/i965/brw_clip_state.c
 index 3223834..3aa679f 100644
 --- a/src/mesa/drivers/dri/i965/brw_clip_state.c
 +++ b/src/mesa/drivers/dri/i965/brw_clip_state.c
 @@ -32,6 +32,7 @@
  #include brw_context.h
  #include brw_state.h
  #include brw_defines.h
 +#include main/framebuffer.h
  
  static void
  upload_clip_vp(struct brw_context *brw)
 @@ -60,6 +61,10 @@ brw_upload_clip_unit(struct brw_context *brw)
  
 /* _NEW_BUFFERS */
 struct gl_framebuffer *fb = ctx-DrawBuffer;
 +   GLint fb_width, fb_height;
 +
 +   fb_width = _mesa_geometric_width(fb);
 +   fb_height = _mesa_geometric_height(fb);

You defined _mesa_geometric_width() and _mesa_geometric_height() to return
unsigned, in principle we should use unsigned here also. But you actually
need them converted to floats so why not convert already the returned
value. Internally in the driver we also try to avoid using gl-types. There
is also no need to separate the declaration and definition of the
variables. I would write this as follows dropping the cast when using them.

  const float fb_width = (float)_mesa_geometric_width(fb);
  const float fb_height = (float)_mesa_geometric_height(fb);

Same applies to the rest of the patch.

  
 upload_clip_vp(brw);
  
 @@ -127,8 +132,8 @@ brw_upload_clip_unit(struct brw_context *brw)
 /* enable guardband clipping if we can */
 if (ctx-ViewportArray[0].X == 0 
 ctx-ViewportArray[0].Y == 0 
 -   ctx-ViewportArray[0].Width == (float) fb-Width 
 -   ctx-ViewportArray[0].Height == (float) fb-Height)
 +   ctx-ViewportArray[0].Width == (float) fb_width 
 +   ctx-ViewportArray[0].Height == (float) fb_height)
 {
clip-clip5.guard_band_enable = 1;
clip-clip6.clipper_viewport_state_ptr =
 diff --git a/src/mesa/drivers/dri/i965/brw_misc_state.c 
 b/src/mesa/drivers/dri/i965/brw_misc_state.c
 index 78a46cb..ef94a6e 100644
 --- a/src/mesa/drivers/dri/i965/brw_misc_state.c
 +++ b/src/mesa/drivers/dri/i965/brw_misc_state.c
 @@ -39,6 +39,7 @@
  #include brw_state.h
  #include brw_defines.h
  
 +#include main/framebuffer.h
  #include main/fbobject.h
  #include main/glformats.h
  
 @@ -46,12 +47,17 @@
  static void upload_drawing_rect(struct brw_context *brw)
  {
 struct gl_context *ctx = brw-ctx;
 +   GLint fb_width, fb_height;
 +   struct gl_framebuffer *fb = ctx-DrawBuffer;

Use 'const', you are only reading.

 +
 +   fb_width = _mesa_geometric_width(fb);
 +   fb_height = _mesa_geometric_height(fb);
  
 BEGIN_BATCH(4);
 OUT_BATCH(_3DSTATE_DRAWING_RECTANGLE  16 | (4 - 2));
 OUT_BATCH(0); /* xmin, ymin */
 -   OUT_BATCH(((ctx-DrawBuffer-Width - 1)  0x) |
 - ((ctx-DrawBuffer-Height - 1)  16));
 +   OUT_BATCH(((fb_width - 1)  0x) |
 + ((fb_height - 1)  16));
 OUT_BATCH(0);
 ADVANCE_BATCH();
  }
 @@ -767,7 +773,7 @@ static void upload_polygon_stipple_offset(struct 
 brw_context *brw)
  * works just fine, and there's no window system to worry about.
  */
 if (_mesa_is_winsys_fbo(ctx-DrawBuffer))
 -  OUT_BATCH((32 - (ctx-DrawBuffer-Height  31))  31);
 +  OUT_BATCH((32 - (_mesa_geometric_height(ctx-DrawBuffer)  31))  31);
 else
OUT_BATCH(0);
 ADVANCE_BATCH();
 diff --git a/src/mesa/drivers/dri/i965/brw_sf_state.c 
 b/src/mesa/drivers/dri/i965/brw_sf_state.c
 index 014b434..1fa3d44 100644
 --- a/src/mesa/drivers/dri/i965/brw_sf_state.c
 +++ b/src/mesa/drivers/dri/i965/brw_sf_state.c
 @@ -35,6 +35,7 @@
  #include main/macros.h

Re: [Mesa-dev] [PATCH 5/7] i965: use _mesa_geometry_width/height/layers/samples for programming geometry of framebuffer to GEN

2015-04-24 Thread Pohjolainen, Topi 
On Fri, Apr 24, 2015 at 07:02:18PM +0300, Rogovin, Kevin wrote:
 
  Actually I realized that you add quite a bit of support to gen4-6 logic 
  that 
  _isn't_ used for gen7 and higher. In the last patch of the series you claim 
  to enable this only for gen7 and higher - I'm confused.
 
 There are two reasons:
 1. Because atoms get reused all the time across generations, it is just 
 easier to use
 the _mesa_geomety_* functions in any batch buffer builder that is concerned 
 about the geometry of the render target. It keeps  the code consistent and 
 much
 easier than checking what functions and atoms are directly or indirectly used 
 by 
 different Gens. However,  blorp, blitting and a few others are left untouched 
 since 
 they want to talk about the buffer, not really 3D pipeline rasterization 
 things. 
 

My point specifically was that you are also updating atoms that _are not_
re-used. And as those changes are not really needed, I wouldn't take the
risk of changing something in vain. I would introduce them only when
you have patches to really enable older generations.

 2. At first I was going to support pre Gen7 hardware with the series. However,
 I do not have hardware on which to test it. In truth I want this to also run 
 on 
 pre-Gen7, but without testing on device, I cannot vouch for the patches. 
 I believe it should just work for pre Gen7 (by just tweaking the last patch 
 to 
 enable it on pre Gen7), but I would rather be careful than in this case. I 
 also 
 confess, it is a silly extension for pre Gen7 anyways...
 
  -Kevin
 
 
 
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 5/7] i965: use _mesa_geometry_width/height/layers/samples for programming geometry of framebuffer to GEN

2015-04-24 Thread Rogovin, Kevin 

 Actually I realized that you add quite a bit of support to gen4-6 logic that 
 _isn't_ used for gen7 and higher. In the last patch of the series you claim 
 to enable this only for gen7 and higher - I'm confused.

There are two reasons:
1. Because atoms get reused all the time across generations, it is just easier 
to use
the _mesa_geomety_* functions in any batch buffer builder that is concerned 
about the geometry of the render target. It keeps  the code consistent and much
easier than checking what functions and atoms are directly or indirectly used 
by 
different Gens. However,  blorp, blitting and a few others are left untouched 
since 
they want to talk about the buffer, not really 3D pipeline rasterization 
things. 

2. At first I was going to support pre Gen7 hardware with the series. However,
I do not have hardware on which to test it. In truth I want this to also run on 
pre-Gen7, but without testing on device, I cannot vouch for the patches. 
I believe it should just work for pre Gen7 (by just tweaking the last patch to 
enable it on pre Gen7), but I would rather be careful than in this case. I also 
confess, it is a silly extension for pre Gen7 anyways...

 -Kevin



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

Re: [Mesa-dev] [PATCH 5/7] i965: use _mesa_geometry_width/height/layers/samples for programming geometry of framebuffer to GEN

2015-04-24 Thread Rogovin, Kevin 

 My point specifically was that you are also updating atoms that _are not_ 
 re-used. 
 And as those changes are not really needed, I wouldn't take the risk of 
 changing 
 something in vain. I would introduce them only when you have patches to 
 really enable older generations.

My take is the following:

 1. Tracking (and guaranteeing) that those function left unchanged as is are 
exactly just those for before Gen7 is a pain. Much easier, and more reliable 
to hit them all instead. A significant number of functions in i965 are not emit 
functions of any atom but emit functions of atoms map to them. Again, more 
reliable and -safer- to change them all, then just the bare minimum. 

2. The change is benign. If _HasAttachments is true, then the function 
substitution 
gives the same value. For Gens not supporting the extension there is no effect.

3. Lastly, as stated: for later it leaves the option to enable it for Gen6 and 
below,
it is just trivial change, but it needs testing on hardware.

When I writing this work, I originally had it for all Gens, but changed to 
support
only Gen7and higher because that is all on which I can test it. 

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

Re: [Mesa-dev] [PATCH 5/7] i965: use _mesa_geometry_width/height/layers/samples for programming geometry of framebuffer to GEN

2015-04-24 Thread Pohjolainen, Topi 
On Fri, Apr 24, 2015 at 04:19:17PM +0300, Pohjolainen, Topi wrote:
 On Fri, Apr 24, 2015 at 09:59:08AM +0300, kevin.rogo...@intel.com wrote:
  From: Kevin Rogovin kevin.rogo...@intel.com
  
  To prepare for i965 to support ARB_framebuffer_no_attachment, use
  the convenience functions mesa_geometry_width/height/layers/samples
  to specify the geometry of the render target surfaces to the GPU.
  
  
  ---
   src/mesa/drivers/dri/i965/brw_clip_state.c |  9 -
   src/mesa/drivers/dri/i965/brw_misc_state.c | 12 --
   src/mesa/drivers/dri/i965/brw_sf_state.c   | 46 
  --
   src/mesa/drivers/dri/i965/brw_state_upload.c   |  7 +++-
   src/mesa/drivers/dri/i965/brw_wm.c |  7 ++--
   src/mesa/drivers/dri/i965/brw_wm_surface_state.c   | 13 --
   src/mesa/drivers/dri/i965/gen6_clip_state.c| 11 --
   src/mesa/drivers/dri/i965/gen6_multisample_state.c |  3 +-
   src/mesa/drivers/dri/i965/gen6_scissor_state.c | 14 +--
   src/mesa/drivers/dri/i965/gen6_sf_state.c  |  3 +-
   src/mesa/drivers/dri/i965/gen6_viewport_state.c|  3 +-
   src/mesa/drivers/dri/i965/gen6_wm_state.c  |  3 +-
   src/mesa/drivers/dri/i965/gen7_sf_state.c  |  3 +-
   src/mesa/drivers/dri/i965/gen7_viewport_state.c|  3 +-
   src/mesa/drivers/dri/i965/gen7_wm_state.c  |  3 +-
   src/mesa/drivers/dri/i965/gen8_viewport_state.c|  9 +++--
   16 files changed, 108 insertions(+), 41 deletions(-)
  
  diff --git a/src/mesa/drivers/dri/i965/brw_clip_state.c 
  b/src/mesa/drivers/dri/i965/brw_clip_state.c
  index 3223834..3aa679f 100644
  --- a/src/mesa/drivers/dri/i965/brw_clip_state.c
  +++ b/src/mesa/drivers/dri/i965/brw_clip_state.c
  @@ -32,6 +32,7 @@
   #include brw_context.h
   #include brw_state.h
   #include brw_defines.h
  +#include main/framebuffer.h
   
   static void
   upload_clip_vp(struct brw_context *brw)
  @@ -60,6 +61,10 @@ brw_upload_clip_unit(struct brw_context *brw)
   
  /* _NEW_BUFFERS */
  struct gl_framebuffer *fb = ctx-DrawBuffer;
  +   GLint fb_width, fb_height;
  +
  +   fb_width = _mesa_geometric_width(fb);
  +   fb_height = _mesa_geometric_height(fb);
 
 You defined _mesa_geometric_width() and _mesa_geometric_height() to return
 unsigned, in principle we should use unsigned here also. But you actually
 need them converted to floats so why not convert already the returned
 value. Internally in the driver we also try to avoid using gl-types. There
 is also no need to separate the declaration and definition of the
 variables. I would write this as follows dropping the cast when using them.
 
   const float fb_width = (float)_mesa_geometric_width(fb);
   const float fb_height = (float)_mesa_geometric_height(fb);
 
 Same applies to the rest of the patch.

Actually I realized that you add quite a bit of support to gen4-6 logic
that _isn't_ used for gen7 and higher. In the last patch of the series
you claim to enable this only for gen7 and higher - I'm confused.

 
   
  upload_clip_vp(brw);
   
  @@ -127,8 +132,8 @@ brw_upload_clip_unit(struct brw_context *brw)
  /* enable guardband clipping if we can */
  if (ctx-ViewportArray[0].X == 0 
  ctx-ViewportArray[0].Y == 0 
  -   ctx-ViewportArray[0].Width == (float) fb-Width 
  -   ctx-ViewportArray[0].Height == (float) fb-Height)
  +   ctx-ViewportArray[0].Width == (float) fb_width 
  +   ctx-ViewportArray[0].Height == (float) fb_height)
  {
 clip-clip5.guard_band_enable = 1;
 clip-clip6.clipper_viewport_state_ptr =
  diff --git a/src/mesa/drivers/dri/i965/brw_misc_state.c 
  b/src/mesa/drivers/dri/i965/brw_misc_state.c
  index 78a46cb..ef94a6e 100644
  --- a/src/mesa/drivers/dri/i965/brw_misc_state.c
  +++ b/src/mesa/drivers/dri/i965/brw_misc_state.c
  @@ -39,6 +39,7 @@
   #include brw_state.h
   #include brw_defines.h
   
  +#include main/framebuffer.h
   #include main/fbobject.h
   #include main/glformats.h
   
  @@ -46,12 +47,17 @@
   static void upload_drawing_rect(struct brw_context *brw)
   {
  struct gl_context *ctx = brw-ctx;
  +   GLint fb_width, fb_height;
  +   struct gl_framebuffer *fb = ctx-DrawBuffer;
 
 Use 'const', you are only reading.
 
  +
  +   fb_width = _mesa_geometric_width(fb);
  +   fb_height = _mesa_geometric_height(fb);
   
  BEGIN_BATCH(4);
  OUT_BATCH(_3DSTATE_DRAWING_RECTANGLE  16 | (4 - 2));
  OUT_BATCH(0); /* xmin, ymin */
  -   OUT_BATCH(((ctx-DrawBuffer-Width - 1)  0x) |
  -   ((ctx-DrawBuffer-Height - 1)  16));
  +   OUT_BATCH(((fb_width - 1)  0x) |
  +   ((fb_height - 1)  16));
  OUT_BATCH(0);
  ADVANCE_BATCH();
   }
  @@ -767,7 +773,7 @@ static void upload_polygon_stipple_offset(struct 
  brw_context *brw)
   * works just fine, and there's no window system to worry about.
   */
  if (_mesa_is_winsys_fbo(ctx-DrawBuffer))
  -  OUT_BATCH((32 - (ctx-DrawBuffer-Height  31))  31);
  +

Re: [Mesa-dev] [PATCH 5/7] i965: use _mesa_geometry_width/height/layers/samples for programming geometry of framebuffer to GEN

2015-04-24 Thread Rogovin, Kevin 
I want to add one more comment on why to replace with the _mesa_geometry_ 
functions, which I had thought was so obvious I neglected to mention it:

With this series the meaning of gl_framebuffer::Width, Height, and so on have a 
different meaning. They give the intersection of the backing stores of the 
render targets. In contrast, the _mesa_geometry_* functions give the geometry 
to feed a rasterizer/windower. By using _mesa_geometry_* functions the code 
communicates clearly it wants the geometry to feed windower/rasterizer and not 
the geometry of the intersection of the (potentially empty) set of backing 
stores. Moreover, it is better to be consistent as well, as later someone will 
likely wonder: why in Gen7 and higher are those _mesa_geometry functions used 
but not before? That question has no good answer because it does not make 
sense to not use those functions when programming the rasterizer/windower 
thingies.

-Kevin

-Original Message-
From: Rogovin, Kevin 
Sent: Friday, April 24, 2015 7:43 PM
To: Pohjolainen, Topi
Cc: mesa-...@freedesktop.org
Subject: RE: [Mesa-dev] [PATCH 5/7] i965: use 
_mesa_geometry_width/height/layers/samples for programming geometry of 
framebuffer to GEN


 My point specifically was that you are also updating atoms that _are not_ 
 re-used. 
 And as those changes are not really needed, I wouldn't take the risk 
 of changing something in vain. I would introduce them only when you have 
 patches to really enable older generations.

My take is the following:

 1. Tracking (and guaranteeing) that those function left unchanged as is are 
exactly just those for before Gen7 is a pain. Much easier, and more reliable to 
hit them all instead. A significant number of functions in i965 are not emit 
functions of any atom but emit functions of atoms map to them. Again, more 
reliable and -safer- to change them all, then just the bare minimum. 

2. The change is benign. If _HasAttachments is true, then the function 
substitution gives the same value. For Gens not supporting the extension there 
is no effect.

3. Lastly, as stated: for later it leaves the option to enable it for Gen6 and 
below, it is just trivial change, but it needs testing on hardware.

When I writing this work, I originally had it for all Gens, but changed to 
support only Gen7and higher because that is all on which I can test it. 

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