Re: [Intel-gfx] [PATCH 7/8] drm/i915: fixup the initial fb base on DG1

[Matthew Auld <matthew.auld@intel.com>]

On 04/03/2022 19:33, Ville Syrjälä wrote:
> On Fri, Mar 04, 2022 at 05:23:32PM +0000, Matthew Auld wrote:
>> The offset we get looks to be the exact start of DSM, but the
>> inital_plane_vma expects the address to be relative.
>>
>> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
>> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
>> ---
>>   .../drm/i915/display/intel_plane_initial.c    | 22 +++++++++++++++----
>>   1 file changed, 18 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_plane_initial.c b/drivers/gpu/drm/i915/display/intel_plane_initial.c
>> index f797fcef18fc..b39d3a8dfe45 100644
>> --- a/drivers/gpu/drm/i915/display/intel_plane_initial.c
>> +++ b/drivers/gpu/drm/i915/display/intel_plane_initial.c
>> @@ -56,10 +56,24 @@ initial_plane_vma(struct drm_i915_private *i915,
>>   	if (!mem || plane_config->size == 0)
>>   		return NULL;
>>   
>> -	base = round_down(plane_config->base,
>> -			  I915_GTT_MIN_ALIGNMENT);
>> -	size = round_up(plane_config->base + plane_config->size,
>> -			mem->min_page_size);
>> +	base = plane_config->base;
>> +	if (IS_DGFX(i915)) {
>> +		/*
>> +		 * On discrete the base address should be somewhere in LMEM, but
>> +		 * depending on the size of LMEM the base address might
>> +		 * intersect with the start of DSM, like on DG1, in which case
>> +		 * we need the relative address. In such cases we might also
>> +		 * need to choose between inital fb vs fbc, if space is limited.
>> +		 *
>> +		 * On future discrete HW, like DG2, we should be able to just
>> +		 * allocate directly from LMEM, due to larger LMEM size.
>> +		 */
>> +		if (base >= i915->dsm.start)
>> +			base -= i915->dsm.start;
> 
> Subsequent code expects the object to actually be inside stolen.
> If that is not the case we should just give up.

Thanks for taking a look at this. Is that subsequent code outside 
initial_plane_vma()? In the next patch this is now using LMEM directly 
for dg2. Would that blow up somewhere else?

> 
> The fact that we fail to confirm any of that on integrated
> parts has always bugged me, but not enough to actually do
> anything about it. Such a check would be somewhat more involved
> since we'd have to look at the PTEs. But on discrete sounds like
> we can get away with a trivial check.

Which PTEs? Is this for the below GGTT bind? I would have assumed that 
the create_at/for_preallocated would simply refuse to allocate the pages 
if the requested range is outside the regions usable range? Or maybe 
there is more going on behind the scenes here?

> 
>> +	}
>> +
>> +	size = roundup(base + plane_config->size, mem->min_page_size);
>> +	base = round_down(base, I915_GTT_MIN_ALIGNMENT);
>>   	size -= base;
>>   
>>   	/*
>> -- 
>> 2.34.1
>