Re: [Mesa-dev] [PATCH 5/7] i965: use _mesa_geometry_width/height/layers/samples for programming geometry of framebuffer to GEN
On Tue, Apr 28, 2015 at 6:17 PM, Rogovin, Kevin 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
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
On Tue, Apr 28, 2015 at 3:37 PM, Rogovin, Kevin 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
> 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
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
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
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
Re: [Mesa-dev] [PATCH 5/7] i965: use _mesa_geometry_width/height/layers/samples for programming geometry of framebuffer to GEN
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
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
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
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
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
> 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
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 > > > > 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
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 09:59:08AM +0300, kevin.rogo...@intel.com wrote: > From: Kevin Rogovin > > 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
[Mesa-dev] [PATCH 5/7] i965: use _mesa_geometry_width/height/layers/samples for programming geometry of framebuffer to GEN
From: Kevin Rogovin 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