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

[Andrzej Hajda <andrzej.hajda@intel.com>]

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.

Regards
Andrzej

>   	}
>   	if (drm_mm_node_allocated(&ggtt->error_capture)) {
>   		u64 start = ggtt->error_capture.start;