[Mesa-dev] [PATCH 04/10] intel: Define enum intel_dri2_has_hiz

[Chad Versace]

On Mon, 06 Jun 2011 17:50:06 -0700, Kenneth Graunke <kenneth at whitecape.org> wrote:
> On 06/04/2011 05:45 PM, Chad Versace wrote:
> > ... which indicates if the X driver supports DRI2BufferHiz and
> > DRI2BufferStencil.
> >
> > I'm placing this in its own commit due to the large comment block.
> >
> > CC: Eric Anholt<eric at anholt.net>
> > CC: Ian Romanick<idr at freedesktop.org>
> > CC: Kenneth Graunke<kenneth at whitecape.org>
> > CC: Kristian Høgsberg<krh at bitplanet.net>
> > Signed-off-by: Chad Versace<chad at chad-versace.us>
> > ---
> >   src/mesa/drivers/dri/intel/intel_screen.h |   54 +++++++++++++++++++++++++++++
> >   1 files changed, 54 insertions(+), 0 deletions(-)
> >
> > diff --git a/src/mesa/drivers/dri/intel/intel_screen.h b/src/mesa/drivers/dri/intel/intel_screen.h
> > index 4613c98..d4add6a 100644
> > --- a/src/mesa/drivers/dri/intel/intel_screen.h
> > +++ b/src/mesa/drivers/dri/intel/intel_screen.h
> > @@ -34,6 +34,60 @@
> >   #include "i915_drm.h"
> >   #include "xmlconfig.h"
> >
> > +/**
> > + * \brief Does the X driver support DRI2BufferHiz and DRI2BufferStencil?
> > + *
> > + * The DRI2 protocol does not allow us to query the X driver's version nor
> > + * query for a list of buffer formats that the driver supports. So, to
> > + * determine if the X driver supports DRI2BufferHiz and DRI2BufferStencil we
> > + * must resort to a handshake.
> > + *
> > + * If the hardware lacks support for separate stencil (and consequently, lacks
> > + * support for hiz also), then the X driver's separate stencil and hiz support
> > + * is irrelevant and the handshake never occurs.
> > + *
> > + * Complications
> > + * -------------
> > + * The handshake is complicated by a bug in the current driver version. Even
> 
> I might say "xf86-video-intel 2.15" rather than "current driver 
> version".  It's future-proof (saves us from reassociating this commit's 
> date with DDX versions) and more specific (the issue is the DDX driver, 
> not the Mesa driver).

You're right: "current driver version" is too ambiguous. It's now
replaced with "xf86-video-intel 2.15".

To eliminate any ambiguity, I also placed this near the top of the comment:
> (Here, "X driver" referes to the DDX driver, xf86-video-intel).                                 
 
> > + * though the driver currently does not support requests for DRI2BufferHiz or
> > + * DRI2BufferStencil, if requested one it still allocates and returns one. The
> > + * returned buffer, however, is incorrectly X tiled.
> > + *
> > + * How the handshake works
> > + * -----------------------
> > + * (TODO: To be implemented on a future commit).
> > + *
> > + * Initially, intel_screen.dri2_has_hiz is set to unknown. The first time the
> > + * user requests a depth and stencil buffer, intelCreateBuffers() creates a
> > + * framebuffer with separate depth and stencil attachments (with formats
> > + * x8_z24 and s8).
> > + *
> 
> As I mentioned in person today, this isn't necessary for Ivybridge---the 
> released DDX driver doesn't include Ivybridge PCI IDs.
> 
> So we could possibly do:
> 
> if (intel->gen >= 7)
>     intel_screen.dri2_has_hiz = INTEL_DRI2_HAS_HIZ_TRUE;
> 
> But your handshake should already work that out anyway, so I suppose 
> it's pointless to add that.
> 
> > + * Eventually, intel_update_renderbuffers() makes a DRI2 request for
> > + * DRI2BufferStencil and DRI2BufferHiz. If the returned buffers are Y tiled,
> > + * then we joyfully set intel_screen.dri2_has_hiz to true and continue as if
> > + * nothing happend.
> > + *
> > + * If the buffers are X tiled, however, the handshake has failed and we must
> > + * clean up.
> > + *    1. Angrily set intel_screen.dri2_has_hiz to false.
> > + *    2. Discard the framebuffer's depth and stencil attachments.
> > + *    3. Attach a packed depth/stencil buffer to the framebuffer (with format
> > + *       s8_z24).
> > + *    4. Make a DRI2 request for the new buffer, using attachment type
> > + *       DRI2BufferDepthStencil).
> > + *
> > + * Future Considerations
> > + * ---------------------
> > + * On a sunny day in the far future, when we are certain that no one has an
> > + * X driver installed without hiz and separate stencil support, then this
> > + * enumerant and the handshake should die.
> > + */
> > +enum intel_dri2_has_hiz {
> > +   INTEL_DRI2_HAS_HIZ_UNKNOWN,
> > +   INTEL_DRI2_HAS_HIZ_TRUE,
> > +   INTEL_DRI2_HAS_HIZ_FALSE,
> > +};
> > +
> 
> I was going to suggest doing the following:
> 
> +enum intel_dri2_has_hiz {
> +   INTEL_DRI2_HAS_HIZ_UNKNOWN = -1,
> +   INTEL_DRI2_HAS_HIZ_TRUE = 1
> +   INTEL_DRI2_HAS_HIZ_FALSE = 0,
> +};
> 
> That way, TRUE == true and FALSE == false, which is nice if you forget 
> to use the enums.  But of course that would make UNKNOWN == true too, 
> which is bullshit.  So not using the enums is bad.  What you have should 
> be fine. :)

It's shame that there is no #include <ternary_logic.h>.

I was worried about the same issues you mention here, and came to the same
conclusions. UKNOWN == true is bullshit, and we just have to use the
enums everywhere. Since that's the case, it doesn't matter what their values are.

----
Chad Versace
chad at chad-versace.us