Re: [Intel-gfx] [RFC PATCH] drm/i915: don't treat small BAR as an error with CSS

["Das, Nirmoy" <nirmoy.das@linux.intel.com>]

On 5/11/2022 4:14 PM, Matthew Auld wrote:
> On Wed, 11 May 2022 at 13:34, Das, Nirmoy <nirmoy.das@intel.com> wrote:
>>
>> On 5/11/2022 1:31 PM, Matthew Auld wrote:
>>> On Wed, 11 May 2022 at 11:25, Nirmoy Das <nirmoy.das@intel.com> wrote:
>>>> Determine lmem_size using ADDR_RANGE register so that module
>>>> load on platfrom with small bar with css  works.
>>>>
>>>> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
>>>> ---
>>>> I once reserved a dg2 machine with small bar and module load failed on
>>>> it. I can't find that machine anymore hence sending this as RFC.
>>> AFAIK we currently don't want to load the driver on such dg2
>>> configurations, until we first have all the uapi bits landed.
>>
>> Ok, sounds good.
>>
>>>    The last
>>> patch in that series then turns this on, or at least that's what I
>>> have locally.
>>>
>>>>    drivers/gpu/drm/i915/gt/intel_region_lmem.c | 11 ++++++++++-
>>>>    1 file changed, 10 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/gt/intel_region_lmem.c b/drivers/gpu/drm/i915/gt/intel_region_lmem.c
>>>> index f5111c0a0060..a55eecac104e 100644
>>>> --- a/drivers/gpu/drm/i915/gt/intel_region_lmem.c
>>>> +++ b/drivers/gpu/drm/i915/gt/intel_region_lmem.c
>>>> @@ -100,10 +100,19 @@ static struct intel_memory_region *setup_lmem(struct intel_gt *gt)
>>>>           if (!IS_DGFX(i915))
>>>>                   return ERR_PTR(-ENODEV);
>>>>
>>>> +       if (IS_DG1(uncore->i915)) {
>>>> +               lmem_size = pci_resource_len(pdev, 2);
>>> We can drop this, since this is set below also.
>>>
>>>> +       } else {
>>>> +               resource_size_t lmem_range;
>>>> +
>>>> +               lmem_range = intel_gt_read_register(&i915->gt0, XEHPSDV_TILE0_ADDR_RANGE) & 0xFFFF;
>>>> +               lmem_size = lmem_range >> XEHPSDV_TILE_LMEM_RANGE_SHIFT;
>>>> +               lmem_size *= SZ_1G;
>>> We can move this under HAS_FLAT_CCS.
>>>
>>> I think we need another patch that then just gracefully returns
>>> -EINVAL if this is a small-bar configuration, along with maybe some
>>> helpful drm_err() or so, which can be removed once we properly support
>>> it?
>> I will resend with this suggestion.
>>
>>
>>>    Also it looks like we are returning ENODEV in some places here,
>>> which looks iffy.
>>
>> We do
>>
>>           io_start = pci_resource_start(pdev, 2);
>>           io_size = min(pci_resource_len(pdev, 2), lmem_size);
>>           if (!io_size)
>>                   return ERR_PTR(-ENODEV);
>>
>> Is this return looks iffy?
> Yeah, since it will only skips the lmem init, without erroring out
> during module load, which I guess leads to nasty errors laters. Also
> the lmem_size < flat_ccs_base check.


Yes, makes sense. Going to send patch to clean that up.


Thanks,

Nirmoy

>
>>
>> Thanks,
>>
>> Nirmoy
>>
>>>> +       }
>>>> +
>>>>           if (HAS_FLAT_CCS(i915)) {
>>>>                   u64 tile_stolen, flat_ccs_base;
>>>>
>>>> -               lmem_size = pci_resource_len(pdev, 2);
>>>>                   flat_ccs_base = intel_gt_read_register(gt, XEHPSDV_FLAT_CCS_BASE_ADDR);
>>>>                   flat_ccs_base = (flat_ccs_base >> XEHPSDV_CCS_BASE_SHIFT) * SZ_64K;
>>>>
>>>> --
>>>> 2.35.1
>>>>