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

[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-dev at 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