[Mesa-dev] [PATCH 08/15] intel: Allocate miptree for multisample DRI2 buffers

[Chad Versace]

On 07/23/2012 03:16 PM, Paul Berry wrote:
> On 21 July 2012 17:36, Chad Versace <chad.versace at linux.intel.com
> <mailto:chad.versace at linux.intel.com>> wrote:
> 
>     Immediately after obtaining, with DRI2GetBuffersWithFormat, the DRM buffer
>     handle for a DRI2 buffer, we wrap that DRM buffer handle with a region and
>     a miptre. This patch additionally allocates an accompanying multisample
> 
> 
> "miptree"

Fixed.

>  
> 
>     miptree if the DRI2 buffer is multisampled.
> 
>     Since we do not yet advertise multisample GL configs, the code for
>     allocating the multisample miptree is currently inactive.
> 
>     CC: Eric Anholt <eric at anholt.net <mailto:eric at anholt.net>>
>     CC: Paul Berry <stereotype441 at gmail.com <mailto:stereotype441 at gmail.com>>
>     Signed-off-by: Chad Versace <chad.versace at linux.intel.com
>     <mailto:chad.versace at linux.intel.com>>
>     ---
>      src/mesa/drivers/dri/intel/intel_context.c     | 26 +++++++++----
>      src/mesa/drivers/dri/intel/intel_mipmap_tree.c | 52 ++++++++++++++++++++++++++
>      src/mesa/drivers/dri/intel/intel_mipmap_tree.h |  6 +++
>      3 files changed, 76 insertions(+), 8 deletions(-)
> 
>     diff --git a/src/mesa/drivers/dri/intel/intel_context.c
>     b/src/mesa/drivers/dri/intel/intel_context.c
>     index 378859c..9a85d83 100644
>     --- a/src/mesa/drivers/dri/intel/intel_context.c
>     +++ b/src/mesa/drivers/dri/intel/intel_context.c
>     @@ -893,14 +893,24 @@ intel_process_dri2_buffer(struct intel_context *intel,
>         if (!rb)
>            return;
> 
>     +   unsigned num_samples = rb->Base.Base.NumSamples;
>     +
>         /* We try to avoid closing and reopening the same BO name, because the first
>          * use of a mapping of the buffer involves a bunch of page faulting which is
>          * moderately expensive.
>          */
>     -   if (rb->mt &&
>     -       rb->mt->region &&
>     -       rb->mt->region->name == buffer->name)
>     -      return;
>     +   if (num_samples == 0) {
>     +       if (rb->mt &&
>     +           rb->mt->region &&
>     +           rb->mt->region->name == buffer->name)
>     +          return;
>     +   } else {
>     +       if (rb->mt &&
>     +           rb->mt->singlesample_mt &&
>     +           rb->mt->singlesample_mt->region &&
>     +           rb->mt->singlesample_mt->region->name == buffer->name)
>     +          return;
>     +   }
> 
>         if (unlikely(INTEL_DEBUG & DEBUG_DRI)) {
>            fprintf(stderr,
>     @@ -920,9 +930,9 @@ intel_process_dri2_buffer(struct intel_context *intel,
>         if (!region)
>            return;
> 
>     -   rb->mt = intel_miptree_create_for_region(intel,
>     -                                            GL_TEXTURE_2D,
>     -                                            intel_rb_format(rb),
>     -                                            region);
>     +   rb->mt = intel_miptree_create_for_dri2_buffer(intel,
>     +                                                 intel_rb_format(rb),
>     +                                                 num_samples,
>     +                                                 region);
>         intel_region_release(&region);
>      }
>     diff --git a/src/mesa/drivers/dri/intel/intel_mipmap_tree.c
>     b/src/mesa/drivers/dri/intel/intel_mipmap_tree.c
>     index b402099..c4496ea 100644
>     --- a/src/mesa/drivers/dri/intel/intel_mipmap_tree.c
>     +++ b/src/mesa/drivers/dri/intel/intel_mipmap_tree.c
>     @@ -324,6 +324,58 @@ compute_msaa_layout(struct intel_context *intel,
>     gl_format format)
>         }
>      }
> 
>     +/**
>     + * If the DRI2 buffer is multisampled, then its content is undefined
>     + * after calling this. This behavior violates the GLX spec for the
>     + * benefit of avoiding a performance penalty.
>     + */
>     +struct intel_mipmap_tree*
>     +intel_miptree_create_for_dri2_buffer(struct intel_context *intel,
>     +                                     gl_format format,
>     +                                     uint32_t num_samples,
>     +                                     struct intel_region *region)
>     +{
>     +   struct intel_mipmap_tree *singlesample_mt = NULL;
>     +   struct intel_mipmap_tree *multisample_mt = NULL;
>     +   GLenum base_format = _mesa_get_format_base_format(format);
>     +
>     +   /* Only the front and back buffers, which are color buffers, are shared
>     +    * through DRI2.
>     +    */
>     +   assert(base_format == GL_RGB || base_format == GL_RGBA);
>     +
>     +   singlesample_mt = intel_miptree_create_for_region(intel, GL_TEXTURE_2D,
>     +                                                     format, region);
>     +   if (!singlesample_mt)
>     +      return NULL;
>     +
>     +   if (num_samples == 0) {
>     +      return singlesample_mt;
>     +   } else {
>     +      multisample_mt = intel_miptree_create_for_renderbuffer(intel,
>     +                                                             format,
>     +                                                             region->width,
>     +                                                             region->height,
>     +                                                             num_samples);
>     +      if (!multisample_mt) {
>     +         intel_miptree_release(&singlesample_mt);
>     +         return NULL;
>     +      }
>     +
>     +      multisample_mt->singlesample_mt = singlesample_mt;
>     +      multisample_mt->need_downsample = false;
>     +
>     +      /* If we wanted to preserve the contents of the DRI2 buffer, here we
>     +       * would need to do an upsample from singlesample_mt to multisample_mt.
>     +       * However, it is unlikely that any app desires that behavior. So we
>     +       * invalidate its content for the benefit of avoiding the upsample
>     +       * performance penalty.
>     +       */
> 
> 
> Is this function called after every buffer swap, or just on the first draw and
> after a window resize?  If it's called after every buffer swap, I agree with you
> that the performance penalty of upsampling isn't justified.  But then I'm
> worried about the performance penalty of intel_miptree_create_for_renderbuffer
> (and especially intel_miptree_alloc_mcs, which it calls--that function spends
> CPU time clearing the MCS buffer, which seems wasteful to do on every frame).
> 
> If this function is only called for the first draw and after a resize, then it
> might be better to go ahead and upsample, just so that the behaviour is more
> consistent between multisampled and single-sampled visuals.

It's called only on first draw and resize.

After our discussion at lunch, I plan to upsample in
intel_miptree_create_for_dri2_buffer if intel->is_front_buffer_rendering.
I'll resubmit the patch series with that change.