| Message ID | 20220304172333.991778-8-matthew.auld@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Some more bits for small BAR enabling | expand |
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. 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. > + } > + > + size = roundup(base + plane_config->size, mem->min_page_size); > + base = round_down(base, I915_GTT_MIN_ALIGNMENT); > size -= base; > > /* > -- > 2.34.1
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 >
On Mon, Mar 07, 2022 at 10:32:36AM +0000, Matthew Auld wrote: > 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? It uses i915_gem_object_create_stolen_for_preallocated() which assumes the stuff is inside stolen. > > 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? The PTEs the plane is actually using. We have no idea where they actually point to and just assume they represent a 1:1 mapping of stolen. I suppose with lmem we'll just start assuming a 1:1 mapping of the whole lmem rather than just stolen.
On 07/03/2022 17:06, Ville Syrjälä wrote: > On Mon, Mar 07, 2022 at 10:32:36AM +0000, Matthew Auld wrote: >> 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? > > It uses i915_gem_object_create_stolen_for_preallocated() which assumes > the stuff is inside stolen. At the start of the series that gets ripped out and replaced with i915_gem_object_create_region_at(), where we can now just pass in the intel_memory_region, and the backend hopefully takes care of the rest. > >>> 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? > > The PTEs the plane is actually using. We have no idea where they > actually point to and just assume they represent a 1:1 mapping of > stolen. > > I suppose with lmem we'll just start assuming a 1:1 mapping of > the whole lmem rather than just stolen. So IIUC the base that we read is actually some GGTT address(I guess it comes pre-programmed or something?), and that hopefully 1:1 maps to stolen. Ok, so as you say, I guess we only want to subtract the dsm.start for the physical allocation, and not the GGTT address, when dealing with stolen lmem. >
On Mon, Mar 07, 2022 at 06:26:32PM +0000, Matthew Auld wrote: > On 07/03/2022 17:06, Ville Syrjälä wrote: > > On Mon, Mar 07, 2022 at 10:32:36AM +0000, Matthew Auld wrote: > >> 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? > > > > It uses i915_gem_object_create_stolen_for_preallocated() which assumes > > the stuff is inside stolen. > > At the start of the series that gets ripped out and replaced with > i915_gem_object_create_region_at(), where we can now just pass in the > intel_memory_region, and the backend hopefully takes care of the rest. Why? Is the BIOS no longer allocating its fbs from stolen? > > > > >>> 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? > > > > The PTEs the plane is actually using. We have no idea where they > > actually point to and just assume they represent a 1:1 mapping of > > stolen. > > > > I suppose with lmem we'll just start assuming a 1:1 mapping of > > the whole lmem rather than just stolen. > > So IIUC the base that we read is actually some GGTT address(I guess it > comes pre-programmed or something?), and that hopefully 1:1 maps to > stolen. Ok, so as you say, I guess we only want to subtract the > dsm.start for the physical allocation, and not the GGTT address, when > dealing with stolen lmem. > > >
On Mon, 7 Mar 2022 at 18:41, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > > On Mon, Mar 07, 2022 at 06:26:32PM +0000, Matthew Auld wrote: > > On 07/03/2022 17:06, Ville Syrjälä wrote: > > > On Mon, Mar 07, 2022 at 10:32:36AM +0000, Matthew Auld wrote: > > >> 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? > > > > > > It uses i915_gem_object_create_stolen_for_preallocated() which assumes > > > the stuff is inside stolen. > > > > At the start of the series that gets ripped out and replaced with > > i915_gem_object_create_region_at(), where we can now just pass in the > > intel_memory_region, and the backend hopefully takes care of the rest. > > Why? Is the BIOS no longer allocating its fbs from stolen? On discrete, so far DSM is always just snipped off the end of lmem. On DG1, which only has 4G lmem, the base seems to always exactly match the DSM start(not sure if this is a fluke). However on DG2, which has much larger lmem size, the base is still the same IIRC, but it isn't even close to where DSM is located on such a device. Best guess is that we were meant to just treat the bios fb(or that part of stolen lmem) as a part of normal lmem, and might explain why the base is not relative to the dsm.start like on integrated? > > > > > > > > >>> 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? > > > > > > The PTEs the plane is actually using. We have no idea where they > > > actually point to and just assume they represent a 1:1 mapping of > > > stolen. > > > > > > I suppose with lmem we'll just start assuming a 1:1 mapping of > > > the whole lmem rather than just stolen. > > > > So IIUC the base that we read is actually some GGTT address(I guess it > > comes pre-programmed or something?), and that hopefully 1:1 maps to > > stolen. Ok, so as you say, I guess we only want to subtract the > > dsm.start for the physical allocation, and not the GGTT address, when > > dealing with stolen lmem. > > > > > > > -- > Ville Syrjälä > Intel
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; + } + + size = roundup(base + plane_config->size, mem->min_page_size); + base = round_down(base, I915_GTT_MIN_ALIGNMENT); size -= base; /*
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(-)