diff mbox series

[RFC] drm/i915: don't treat small BAR as an error with CSS

Message ID 20220511102509.19927-1-nirmoy.das@intel.com (mailing list archive)
State New, archived
Headers show
Series [RFC] drm/i915: don't treat small BAR as an error with CSS | expand

Commit Message

Nirmoy Das May 11, 2022, 10:25 a.m. UTC
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.

 drivers/gpu/drm/i915/gt/intel_region_lmem.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

Comments

Nirmoy Das May 11, 2022, 10:28 a.m. UTC | #1
+Ram (git send-email didn't add Ram to the cc list for some reason)

On 5/11/2022 12:25 PM, Nirmoy Das 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.
>
>   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);
> +	} 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;
> +	}
> +
>   	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;
>
Matthew Auld May 11, 2022, 11:31 a.m. UTC | #2
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. 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? Also it looks like we are returning ENODEV in some places here,
which looks iffy.

> +       }
> +
>         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
>
Nirmoy Das May 11, 2022, 12:34 p.m. UTC | #3
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?


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
>>
Matthew Auld May 11, 2022, 2:14 p.m. UTC | #4
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.

>
>
> 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
> >>
Das, Nirmoy May 11, 2022, 2:41 p.m. UTC | #5
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
>>>>
diff mbox series

Patch

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);
+	} 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;
+	}
+
 	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;