[Mesa-dev] [PATCH] i965: Stop lying about cpp and height of a stencil buffer.

[Paul Berry]

On 9 April 2012 10:59, Kenneth Graunke <kenneth at whitecape.org> wrote:

> On 04/09/2012 09:43 AM, Paul Berry wrote:
>
>> When using a separate stencil buffer, i965 requires that the pitch of
>> the buffer (in the 3DSTATE_STENCIL_BUFFER command) be specified as 2x
>> the actual pitch.
>>
>> Previously this was accomplished by doubling the "cpp" and "pitch"
>> values stored in the intel_region data structure, and halving the
>> height.  However, this was confusing, and it led to a subtle (but
>> benign) bug: since a stencil buffer is W-tiled, its true height must
>> be aligned to a multiple of 64; we were accidentally aligning its faux
>> height to a multiple of 64, causing memory to be wasted.
>>
>> Note that for window system stencil buffers, the X server also doubles
>> the cpp and pitch values.  To facilitate fixing this X server bug in
>> the future, we fix the cpp and pitch values we receive from the X
>> server only if cpp has the "incorrect" value of 2.
>>
>
> Paul,
>
> This seems like a nice clean-up.  I'm glad to see this.
>
> I reviewed the code and it seems to make sense, but I'd really want to see
> Chad or Eric's review on it before it gets pushed.
>
> Acked-by: Kenneth Graunke <kenneth at whitecape.org>
>
> One small comment below:
>
>
>  diff --git a/src/mesa/drivers/dri/intel/**intel_context.c
>> b/src/mesa/drivers/dri/intel/**intel_context.c
>> index 16a9887..fc3258b 100644
>> --- a/src/mesa/drivers/dri/intel/**intel_context.c
>> +++ b/src/mesa/drivers/dri/intel/**intel_context.c
>> @@ -1251,17 +1251,34 @@ intel_process_dri2_buffer_**with_separate_stencil(struct
>> intel_context *intel,
>>
>>     int buffer_width;
>>     int buffer_height;
>> +   int buffer_cpp = buffer->cpp;
>> +   int buffer_pitch = buffer->pitch;
>>     if (buffer->attachment == __DRI_BUFFER_STENCIL) {
>> -      /* The stencil buffer has quirky pitch requirements.  From Section
>> -       * 2.11.5.6.2.1 3DSTATE_STENCIL_BUFFER, field "Surface Pitch":
>> -       *    The pitch must be set to 2x the value computed based on
>> width, as
>> -       *    the stencil buffer is stored with two rows interleaved.
>> +      /* Stencil buffers use W tiling, a tiling format that the DRM
>> functions
>> +       * don't properly account for.  Therefore, when we allocate a
>> stencil
>> +       * buffer that is private to Mesa (see intel_miptree_create), we
>> round
>> +       * the height and width up to the next multiple of the tile size
>> (64x64)
>> +       * and then ask DRM to allocate an untiled buffer.  Consequently,
>> the
>> +       * height and the width stored in the stencil buffer's region
>> structure
>> +       * are always multiples of 64, even if the stencil buffer itself is
>> +       * smaller.
>>         *
>> -       * To satisfy the pitch requirement, the X driver allocated the
>> region
>> -       * with the following dimensions.
>> +       * To avoid inconsistencies between how we represent private
>> buffers and
>> +       * buffers shared with the window system, round up the height and
>> width
>> +       * for window system buffers too.
>>         */
>>         buffer_width = ALIGN(drawable->w, 64);
>> -       buffer_height = ALIGN(ALIGN(drawable->h, 2) / 2, 64);
>> +       buffer_height = ALIGN(drawable->h, 64);
>> +
>> +       /* As of 4/6/2012, the X server lies and sends cpp and pitch
>> values
>> +        * that are two times too large for stencil buffers.  Hopefully
>> this
>> +        * will be fixed someday, but for now detect the bug by checking
>> if cpp
>> +        * is 2, and fixing cpp and pitch if it is.
>> +        */
>> +       if (buffer_cpp == 2) {
>> +          buffer_cpp = 1;
>> +          buffer_pitch /= 2;
>> +       }
>>     } else {
>>         buffer_width = drawable->w;
>>         buffer_height = drawable->h;
>>
>
> I was going to suggest saying "xserver 1.12 lies and sends..." rather than
> using a date...but...I don't think it's the X server that's really at
> fault.  Isn't it xf86-video-intel (sometimes called the DDX)?
>
> So, maybe "As of 4/6/2012, the DDX lies and sends cpp and pitch values"?
>

(After some discussion with Chad) how about: "xf86-video-intel versions
2.17.0 and earlier lie and send....  Hopefully this will be fixed in a
future version of xf86-video-intel."


>
> Sadly, I'm not sure how to fix that without breaking new DDX + old Mesa.
>

Chad and I have had some discussion about this.  We are hoping to add
GetParam and SetParam messages to the DDX protocol, to allow Mesa and the
DDX to exchange arbitrary integer parameters describing protocol/behavior
options.  GetParam() will return 0 when queried about a parameter that it
does not recognize.  The idea is that at startup, Mesa will query the DDX
with GetParam to ask "are you capable of telling the truth about cpp and
pitch of stencil buffers?"  If it gets back a 1 (meaning yes), it will then
send a SetParam message to say "henceforth, please tell the truth about cpp
and pitch of stencil buffers."  Not the prettiest thing in the world, I
know, but it would allow old and new versions of the DDX and Mesa to be
arbitrarily intermingled.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20120409/5510965a/attachment.htm>