Re: [Intel-gfx] [RFC 0/2] drm/i915/ttm: Evict and store of compressed object

["Christian König" <christian.koenig@amd.com>]

Am 07.02.22 um 14:53 schrieb Ramalingam C:
> On 2022-02-07 at 12:41:59 +0100, Christian König wrote:
>> Am 07.02.22 um 10:37 schrieb Ramalingam C:
>>> On flat-ccs capable platform we need to evict and resore the ccs data
>>> along with the corresponding main memory.
>>>
>>> This ccs data can only be access through BLT engine through a special
>>> cmd ( )
>>>
>>> To support above requirement of flat-ccs enabled i915 platforms this
>>> series adds new param called ccs_pages_needed to the ttm_tt_init(),
>>> to increase the ttm_tt->num_pages of system memory when the obj has the
>>> lmem placement possibility.
>> Well question is why isn't the buffer object allocated with the extra space
>> in the first place?
> Hi Christian,
>
> On Xe-HP and later devices, we use dedicated compression control state (CCS)
> stored in local memory for each surface, to support the 3D and media
> compression formats.
>
> The memory required for the CCS of the entire local memory is 1/256 of the
> local memory size. So before the kernel boot, the required memory is reserved
> for the CCS data and a secure register will be programmed with the CCS base
> address
>
> So when we allocate a object in local memory we dont need to explicitly
> allocate the space for ccs data. But when we evict the obj into the smem
>   to hold the compression related data along with the obj we need smem
>   space of obj_size + (obj_size/256).
>
>   Hence when we create smem for an obj with lmem placement possibility we
>   create with the extra space.

Exactly that's what I've been missing in the cover letter and/or commit 
messages, comments etc..

Over all sounds like a valid explanation to me, just one comment on the 
code/naming:

>   int ttm_tt_init(struct ttm_tt *ttm, struct ttm_buffer_object *bo,
> -		uint32_t page_flags, enum ttm_caching caching)
> +		uint32_t page_flags, enum ttm_caching caching,
> +		unsigned long ccs_pages)

Please don't try to leak any i915 specific stuff into common TTM code.

For example use the wording extra_pages instead of ccs_pages here.

Apart from that looks good to me,
Christian.

>
>   Ram.
>> Regards,
>> Christian.
>>
>>> This will be on top of the flat-ccs enabling series
>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.freedesktop.org%2Fseries%2F95686%2F&data=04%7C01%7Cchristian.koenig%40amd.com%7Ce54bb7576a334a76cab008d9ea4138e5%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637798388115252727%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=V9wQZvb0JwtplIBSYYXGzrg%2BEMvn4hfkscziPFDvZDY%3D&reserved=0
>>>
>>> For more about flat-ccs feature please have a look at
>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.freedesktop.org%2Fpatch%2F471777%2F%3Fseries%3D95686%26rev%3D5&data=04%7C01%7Cchristian.koenig%40amd.com%7Ce54bb7576a334a76cab008d9ea4138e5%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637798388115252727%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=aYjoTKMbZvi%2Fnr7hkSH4SYGxZIv8Dj210dNrBnUNpQw%3D&reserved=0
>>>
>>> Testing of the series is WIP and looking forward for the early review on
>>> the amendment to ttm_tt_init and the approach.
>>>
>>> Ramalingam C (2):
>>>     drm/i915/ttm: Add extra pages for handling ccs data
>>>     drm/i915/migrate: Evict and restore the ccs data
>>>
>>>    drivers/gpu/drm/drm_gem_vram_helper.c      |   2 +-
>>>    drivers/gpu/drm/i915/gem/i915_gem_ttm.c    |  23 +-
>>>    drivers/gpu/drm/i915/gt/intel_migrate.c    | 283 +++++++++++----------
>>>    drivers/gpu/drm/qxl/qxl_ttm.c              |   2 +-
>>>    drivers/gpu/drm/ttm/ttm_agp_backend.c      |   2 +-
>>>    drivers/gpu/drm/ttm/ttm_tt.c               |  12 +-
>>>    drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c |   2 +-
>>>    include/drm/ttm/ttm_tt.h                   |   4 +-
>>>    8 files changed, 191 insertions(+), 139 deletions(-)
>>>