On 17-01-04 10:34:17, Topi Pohjolainen Topi Pohjolainen wrote:
On Wed, Jan 04, 2017 at 09:40:51AM +0200, Pohjolainen, Topi wrote:
On Mon, Jan 02, 2017 at 06:37:10PM -0800, Ben Widawsky wrote:
> This code will disable actually creating these buffers for the scanout,
> but it puts the allocation in place.
>
> Primarily this patch is split out for review, it can be squashed in
> later if preferred.
>
> assert(mt->offset == 0) in ccs creation (as requested by Topi)
>
> Cc: Topi Pohjolainen <topi.pohjolai...@intel.com>
> Signed-off-by: Ben Widawsky <b...@bwidawsk.net>
> Acked-by: Daniel Stone <dani...@collabora.com>
> ---
>  src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 97 
+++++++++++++++++++++++----
>  1 file changed, 85 insertions(+), 12 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c 
b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> index d79cc61ac4..dce8ce3350 100644
> --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> @@ -58,6 +58,11 @@ intel_miptree_alloc_mcs(struct brw_context *brw,
>                          struct intel_mipmap_tree *mt,
>                          GLuint num_samples);
>
> +static void
> +intel_miptree_init_mcs(struct brw_context *brw,
> +                       struct intel_mipmap_tree *mt,
> +                       int init_value);
> +
>  /**
>   * Determine which MSAA layout should be used by the MSAA surface being
>   * created, based on the chip generation and the surface type.
> @@ -152,6 +157,9 @@ intel_miptree_supports_non_msrt_fast_clear(struct 
brw_context *brw,
>     if (mt->aux_disable & INTEL_AUX_DISABLE_MCS)
>        return false;
>
> +   if (mt->is_scanout)
> +      return false;
> +
>     /* This function applies only to non-multisampled render targets. */
>     if (mt->num_samples > 1)
>        return false;
> @@ -744,6 +752,7 @@ intel_miptree_create(struct brw_context *brw,
>         * resolves.
>         */
>        const bool lossless_compression_disabled = INTEL_DEBUG & DEBUG_NO_RBC;
> +      assert(!mt->is_scanout);
>        const bool is_lossless_compressed =
>           unlikely(!lossless_compression_disabled) &&
>           brw->gen >= 9 && !mt->is_scanout &&
> @@ -810,6 +819,45 @@ intel_miptree_create_for_bo(struct brw_context *brw,
>     return mt;
>  }
>
> +static bool
> +create_ccs_buf_for_image(struct brw_context *intel,
> +                         __DRIimage *image,
> +                         struct intel_mipmap_tree *mt)
> +{
> +
> +   struct isl_surf temp_main_surf;
> +   struct isl_surf temp_ccs_surf;
> +
> +   /* There isn't anything specifically wrong with there being an offset, in
> +    * which case, the CCS miptree's offset should be mt->offset +
> +    * image->aux_offset. However, the code today only will have an offset 
when
> +    * this miptree is pointing to a slice from another miptree, and in that 
case
> +    * we'd need to offset within the AUX CCS buffer properly. It's 
questionable
> +    * whether our code handles that case properly, and since it can never 
happen
> +    * for scanout, just use the assertion to prevent it.
> +    */
> +   assert(mt->offset == 0);

Thanks a lot for adding this, especially the comment!

> +
> +   intel_miptree_get_isl_surf(intel, mt, &temp_main_surf);
> +   if (!isl_surf_get_ccs_surf(&intel->isl_dev, &temp_main_surf, 
&temp_ccs_surf))
> +      return false;
> +
> +   mt->mcs_buf = calloc(1, sizeof(*mt->mcs_buf));
> +   mt->mcs_buf->bo = image->bo;
> +   drm_intel_bo_reference(image->bo);
> +
> +   mt->mcs_buf->offset = image->aux_offset;
> +   mt->mcs_buf->size = temp_ccs_surf.size;
> +   mt->mcs_buf->pitch = temp_ccs_surf.row_pitch;
> +   mt->mcs_buf->qpitch = isl_surf_get_array_pitch_sa_rows(&temp_ccs_surf);
> +
> +   intel_miptree_init_mcs(intel, mt, 0);
> +   mt->aux_disable &= ~INTEL_AUX_DISABLE_CCS;
> +   mt->msaa_layout = INTEL_MSAA_LAYOUT_CMS;
> +
> +   return true;
> +}
> +
>  struct intel_mipmap_tree *
>  intel_miptree_create_for_image(struct brw_context *intel,
>                                 __DRIimage *image,
> @@ -820,17 +868,42 @@ intel_miptree_create_for_image(struct brw_context 
*intel,
>                                 uint32_t pitch,
>                                 uint32_t layout_flags)
>  {
> -   layout_flags = (layout_flags & MIPTREE_LAYOUT_FOR_SCANOUT) ?
> -      MIPTREE_LAYOUT_FOR_SCANOUT : MIPTREE_LAYOUT_DISABLE_AUX;
> -   return intel_miptree_create_for_bo(intel,
> -                                      image->bo,
> -                                      format,
> -                                      offset,
> -                                      width,
> -                                      height,
> -                                      1,
> -                                      pitch,
> -                                      layout_flags);
> +   struct intel_mipmap_tree *mt;
> +
> +   /* Other flags will be ignored, so make sure the caller didn't pass any. 
*/
> +   assert((layout_flags & ~MIPTREE_LAYOUT_FOR_SCANOUT) == 0);
> +
> +   if (!image->aux_offset)
> +      layout_flags |= MIPTREE_LAYOUT_DISABLE_AUX;
> +   else
> +      layout_flags |= MIPTREE_LAYOUT_FORCE_HALIGN16;
> +
> +   mt = intel_miptree_create_for_bo(intel,
> +                                    image->bo,
> +                                    format,
> +                                    offset,
> +                                    width,
> +                                    height,
> +                                    1,
> +                                    pitch,
> +                                    layout_flags);
> +
> +   if (!intel_tiling_supports_non_msrt_mcs(intel, mt->tiling)) {
> +      assert(image->aux_offset == 0);
> +      return mt;
> +   }
> +
> +   if (layout_flags & MIPTREE_LAYOUT_DISABLE_AUX)
> +      return mt;
> +
> +   assert(image->aux_offset);
> +   assert(mt->num_samples <= 1);
> +   assert(mt->last_level < 2);
> +   assert(mt->logical_depth0 == 1);
> +
> +   create_ccs_buf_for_image(intel, image, mt);
> +
> +   return mt;
>  }
>
>  /**
> @@ -998,7 +1071,7 @@ intel_miptree_release(struct intel_mipmap_tree **mt)
>        intel_miptree_release(&(*mt)->stencil_mt);
>        intel_miptree_release(&(*mt)->r8stencil_mt);
>        intel_miptree_hiz_buffer_free((*mt)->hiz_buf);
> -      if ((*mt)->mcs_buf) {
> +      if ((*mt)->mcs_buf && !(*mt)->is_scanout) {

Reference count gets incremented in create_ccs_buf_for_image(), so shouldn't
we decrease it here? Original image should hold reference as long as it needs
regardless the logic here. Or am I missing something?

You drop this change again in patch 31. Between here and patch 31,
(*mt)->mcs_buf would always be NULL if (*mt)->is_scanout is true. Is this a
leftover from some early work?


Yeah, I think this was a bug with my original work which got pulled in by
accident on rebase. This was like an interim fix. Originally I didn't hold two
references (the ccs data in the "mcs_buf" and the image's pixel's data are both
in the same BO), and so the objective of this hunk was to only release the
reference once - which seems broken anyway.

Anyway, like you noticed, this hunk is bogus and gone later in the series. I'll
get it out of this patch.


Otherwise the patch looks good:

Reviewed-by: Topi Pohjolainen <topi.pohjolai...@intel.com>


Thanks.

>           drm_intel_bo_unreference((*mt)->mcs_buf->bo);
>           free((*mt)->mcs_buf);
>        }
> --
> 2.11.0
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Reply via email to