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

[Kenneth Graunke]

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).

> + * 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. :)

>   struct intel_screen
>   {
>      int deviceID;