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

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