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

[Jason Ekstrand]

On Tue, Apr 28, 2015 at 3:37 PM, Rogovin, Kevin <kevin.rogovin at 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