Re: [Intel-gfx] [PATCH 03/19] drm/i915: Create stolen memory region from local memory

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 16/04/2021 16:04, Matthew Auld wrote:
> On 14/04/2021 16:01, Tvrtko Ursulin wrote:
>>
>> On 12/04/2021 10:05, Matthew Auld wrote:
>>> From: CQ Tang <cq.tang@intel.com>
>>>
>>> Add "REGION_STOLEN" device info to dg1, create stolen memory
>>> region from upper portion of local device memory, starting
>>> from DSMBASE.
>>>
>>> v2:
>>>      - s/drm_info/drm_dbg; userspace likely doesn't care about stolen.
>>>      - mem->type is only setup after the region probe, so setting the 
>>> name
>>>        as stolen-local or stolen-system based on this value won't 
>>> work. Split
>>>        system vs local stolen setup to fix this.
>>>      - kill all the region->devmem/is_devmem stuff. We already 
>>> differentiate
>>>        the different types of stolen so such things shouldn't be needed
>>>        anymore.
>>>
>>> Signed-off-by: CQ Tang <cq.tang@intel.com>
>>> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/gem/i915_gem_stolen.c | 99 +++++++++++++++++++---
>>>   drivers/gpu/drm/i915/gem/i915_gem_stolen.h |  3 +
>>>   drivers/gpu/drm/i915/i915_pci.c            |  2 +-
>>>   drivers/gpu/drm/i915/i915_reg.h            |  1 +
>>>   drivers/gpu/drm/i915/intel_memory_region.c |  6 ++
>>>   drivers/gpu/drm/i915/intel_memory_region.h |  5 +-
>>>   6 files changed, 102 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c 
>>> b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
>>> index b0597de206de..56dd58bef5ee 100644
>>> --- a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
>>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
>>> @@ -10,6 +10,7 @@
>>>   #include <drm/drm_mm.h>
>>>   #include <drm/i915_drm.h>
>>> +#include "gem/i915_gem_lmem.h"
>>>   #include "gem/i915_gem_region.h"
>>>   #include "i915_drv.h"
>>>   #include "i915_gem_stolen.h"
>>> @@ -121,6 +122,14 @@ static int i915_adjust_stolen(struct 
>>> drm_i915_private *i915,
>>>           }
>>>       }
>>> +    /*
>>> +     * With device local memory, we don't need to check the address 
>>> range,
>>> +     * this is device memory physical address, could overlap with 
>>> system
>>> +     * memory.
>>> +     */
>>> +    if (HAS_LMEM(i915))
>>> +        return 0;
>>> +
>>>       /*
>>>        * Verify that nothing else uses this physical address. Stolen
>>>        * memory should be reserved by the BIOS and hidden from the
>>> @@ -374,8 +383,9 @@ static void icl_get_stolen_reserved(struct 
>>> drm_i915_private *i915,
>>>       }
>>>   }
>>> -static int i915_gem_init_stolen(struct drm_i915_private *i915)
>>> +static int i915_gem_init_stolen(struct intel_memory_region *mem)
>>>   {
>>> +    struct drm_i915_private *i915 = mem->i915;
>>>       struct intel_uncore *uncore = &i915->uncore;
>>>       resource_size_t reserved_base, stolen_top;
>>>       resource_size_t reserved_total, reserved_size;
>>> @@ -396,10 +406,10 @@ static int i915_gem_init_stolen(struct 
>>> drm_i915_private *i915)
>>>           return 0;
>>>       }
>>> -    if (resource_size(&intel_graphics_stolen_res) == 0)
>>> +    if (resource_size(&mem->region) == 0)
>>>           return 0;
>>> -    i915->dsm = intel_graphics_stolen_res;
>>> +    i915->dsm = mem->region;
>>>       if (i915_adjust_stolen(i915, &i915->dsm))
>>>           return 0;
>>> @@ -684,23 +694,36 @@ static int _i915_gem_object_stolen_init(struct 
>>> intel_memory_region *mem,
>>>       return ret;
>>>   }
>>> +struct intel_memory_region *i915_stolen_region(struct 
>>> drm_i915_private *i915)
>>> +{
>>> +    if (HAS_LMEM(i915))
>>> +        return i915->mm.regions[INTEL_REGION_STOLEN_LMEM];
>>> +
>>> +    return i915->mm.regions[INTEL_REGION_STOLEN_SMEM];
>>> +}
>>
>> Could be a bikeshedding comment only - especially since I think this 
>> path gets very little used at runtime so it is most likely pointless 
>> to fiddle with it, but it just strikes me a bit not fully elegant to do:
>>
>> i915_gem_object_create_stolen
>>   -> i915_gem_object_create_region
>>      -> i915_stolen_region
>>
>> And end up in here, when alternative could be at driver init:
>>
>> i915->stolen_region_id = HAS_LMEM() ? ... : ...;
>>
>> i915_gem_object_create_stolen
>>   -> 
>> i915_gem_object_create_region(i915->mm.regions[i915->stolen_region_id]);
>>
>> Or pointer to region. Would avoid having to export i915_stolen_region 
>> as well.
>>
>> Or is i915->dsm already the right thing? Because..
> 
> I guess we could just have an i915->stolen_region short-cut or something?

i915->dsm is not it? Where does i915_gem_init_stolen exists for 
local-stolen then? At the "resource_size(&mem->region) == 0" check?

> 
>>
>>> +
>>>   struct drm_i915_gem_object *
>>>   i915_gem_object_create_stolen(struct drm_i915_private *i915,
>>>                     resource_size_t size)
>>>   {
>>> -    return 
>>> i915_gem_object_create_region(i915->mm.regions[INTEL_REGION_STOLEN_SMEM], 
>>>
>>> +    return i915_gem_object_create_region(i915_stolen_region(i915),
>>>                            size, I915_BO_ALLOC_CONTIGUOUS);
>>>   }
>>>   static int init_stolen(struct intel_memory_region *mem)
>>>   {
>>> -    intel_memory_region_set_name(mem, "stolen");
>>> +    if (HAS_LMEM(mem->i915)) {
>>> +        if (!io_mapping_init_wc(&mem->iomap,
>>> +                    mem->io_start,
>>> +                    resource_size(&mem->region)))
>>> +            return -EIO;
>>> +    }
>>>       /*
>>>        * Initialise stolen early so that we may reserve preallocated
>>>        * objects for the BIOS to KMS transition.
>>>        */
>>> -    return i915_gem_init_stolen(mem->i915);
>>> +    return i915_gem_init_stolen(mem);
>>
>> ... I find the mem region init paths a bit convoluted, stolen 
>> especially, and struggle to figure it out every time.
>>
>> For instance we have i915_region_stolen_ops shared between system and 
>> local stolen. But then shared vfuncs branch depending on system vs 
>> stolen?
> 
> We could split the intel_memory_region ops? Maybe that will make it 
> slightly less muddled?

I think so. Each vfunc table with it's own ->init() should make it 
easier to follow.

> The probing is slightly different, but that's kind of expected since 
> it's quite different from the HW pov.
> 
> But once we get an intel_memory_region, it should be the same whether 
> it's stolen device memory or whatever.
> 
>>
>> i915_gem_init_stolen is shared - but which parts of it are relevant 
>> for local stolen?
> 
> Asking all the difficult questions :)
> 
> It's just to populate dsm I think. I can rip that out and then we don't 
> call i915_gem_init_stolen() for the stolen device memory path? Maybe 
> that will look slightly better?

Yes, with the above approach of two struct intel_memory_region_ops? Even 
if some vfuncs are shared it should be better.

I am also confused by ->release ie. i915_gem_cleanup_stolen. How does 
that work for two stolen regions, I mean one i915->mm.stolen?

Regards,

Tvrtko
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel