[Mesa-dev] [PATCH 16/27] i965/miptree: Allocate mcs_buf for an image's CCS_E
Ben Widawsky
ben at bwidawsk.net
Fri Dec 30 01:41:49 UTC 2016
On 16-12-10 15:26:02, Pohjolainen, Topi wrote:
>On Thu, Dec 01, 2016 at 02:09:57PM -0800, Ben Widawsky wrote:
>> From: Ben Widawsky <ben at bwidawsk.net>
>>
>> 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.
>>
>> Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
>> ---
>> src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 89 +++++++++++++++++++++++----
>> 1 file changed, 77 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 cfa2dc0..d002546 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->disable_aux_buffers)
>> 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,36 @@ 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;
>> + uint32_t offset = mt->offset + image->aux_offset;
>
>I need to ask about this as I'm not sure myself. We usually use offset to
>move to a slice other than the first. Here the offset to the start of the
>auxiliary is taken as relative to that. This seems awkward to me. Moreover,
>if we move to another slice, don't we need to move equally within the auxiliary
>also.
>
There are restrictions in the hardware that the x,y offset of the main buffer
need to match that of the aux buffer - that is what this is trying to achieve.
The scanout shouldn't ever have more than 1 slice (AFAIK), and even if it does,
we'd do the allocations for all slices here. What do you mean by, "if we move to
another slice"?
>Further down you assert for mipmap levels "assert(mt->last_level < 2)" just
>before calling this. So shoudln't we actually assert here that:
>
> assert(mt->offset == 0);
>
I think today that is fine, but I don't believe there is any reason that the
offset couldn't be non-zero. I am not the expert though.
>> +
>> + 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 = 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->no_ccs = false;
>> + 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 +859,43 @@ 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;
>> +
>> + layout_flags &= ~MIPTREE_LAYOUT_FOR_SCANOUT;
>
>This isn't used in the rest of the function anymore.
>
Thanks. This was used earlier before rebasing on top of your patches.
>> +
>> + assert(image->aux_offset);
>> + assert(mt->num_samples >= 0);
>
>This is always true isn't it? Did you mean:
>
> assert(mt->num_samples <= 1);
>
>
Yes, that is what I meant :)
>> + assert(mt->last_level < 2);
>
>Should we also:
>
> assert(mt->logical_depth0 == 1);
>
Sure
>> +
>> + create_ccs_buf_for_image(intel, image, mt);
>> +
>> + return mt;
>> }
>>
>> /**
>> @@ -991,7 +1056,7 @@ intel_miptree_release(struct intel_mipmap_tree **mt)
>> drm_intel_bo_unreference((*mt)->hiz_buf->aux_base.bo);
>> free((*mt)->hiz_buf);
>> }
>> - if ((*mt)->mcs_buf) {
>> + if ((*mt)->mcs_buf && !(*mt)->is_scanout) {
>> drm_intel_bo_unreference((*mt)->mcs_buf->bo);
>> free((*mt)->mcs_buf);
>> }
>> --
>> 2.10.2
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
More information about the mesa-dev
mailing list