Re: [PATCH 3/3] drm/i915: Pin error_capture to high end of mappable

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Jan 10, 2024 at 05:09:38PM +0100, Andrzej Hajda wrote:
> On 15.12.2023 12:09, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > If we fail to pin error_capture to the start of ggtt (which
> > is likely given the BIOS fb is usually there), we currently
> > fall back to pinning it at the next available low address.
> > This seems somewhat sub-optimal to me in case we later discard
> > the BIOS fb (fairly likely if there are multiple different sized
> > displays connected at boot). We are then then left with a
> > permanenly pinned object somewhere in the middle of the mappable
> > range of ggtt. It seems more sensible to pin the error capture
> > object to the end of mappable as a fallback, so even if we discard
> > the BIOS fb we are left with the mappable region mostly in one
> > piece (potentially allowing for more/larger objects to be pinned
> > there later).
> > 
> > Though I suppose we are chopping the ggtt address space as a
> > whole into two chunks in a slightly different way. Essentially
> > reducing the size of the second (larger) chunk a bit. So perhaps
> > pinning truly massive objects (which don't strictly need to
> > be mappable) could become a bit more difficult.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >   drivers/gpu/drm/i915/gt/intel_ggtt.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt.c b/drivers/gpu/drm/i915/gt/intel_ggtt.c
> > index 21a7e3191c18..f62008962eb5 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_ggtt.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_ggtt.c
> > @@ -876,7 +876,7 @@ static int init_ggtt(struct i915_ggtt *ggtt)
> >   						    ggtt->error_capture.size, 0,
> >   						    ggtt->error_capture.color,
> >   						    0, ggtt->mappable_end,
> > -						    DRM_MM_INSERT_LOW);
> > +						    DRM_MM_INSERT_HIGH);
> 
> In this case Chris raised concern that since error_capture must be 
> mappable we will end up with worse fragmentation in case of 
> DRM_MM_INSERT_HIGH.
> 
> Also see the comment above the change (my concern):
>   * We strongly prefer taking address 0x0 in order to protect
>   * other critical buffers against accidental overwrites,
>   * as writing to address 0 is a very common mistake.
> 
> Maybe the ultimate solution would be to relocate BIOS fb vma also if it 
> is at 0, I don't know.

That would fragment things even worse as we then end up with the BIOS fb
somewhere in the middle of mappable. Maybe if we move that explciitly to
the end of mappable it might work. Although I'm not sure that's always
possible either, so we still probably have to deal with the case where
the BIOS fb is at the start of mappable.

But yeah, I'm not entirely sold on this change yet. I think we might be
pinning other stuff low in the mappable as well (rings/status pages/contexts/etc.).
We should probably do a real analysis what we have pinned and where and 
then try to come up with a good strategy how to handle it all.

-- 
Ville Syrjälä
Intel