Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.38 as permitted sender) client-ip=23.128.96.38;
Date:   Fri, 17 Nov 2023 23:01:50 +0000
From:   Paz Zcharya <pazz@chromium.org>
To:     Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc:     matthew.auld@intel.com, Tvrtko Ursulin <tvrtko.ursulin@intel.com>,
        Daniel Vetter <daniel@ffwll.ch>,
        Subrata Banik <subratabanik@google.com>,
        dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org,
        Sean Paul <seanpaul@chromium.org>,
        Andrzej Hajda <andrzej.hajda@intel.com>,
        Marcin Wojtas <mwojtas@chromium.org>,
        Drew Davenport <ddavenport@chromium.org>,
        David Airlie <airlied@gmail.com>,
        intel-gfx@lists.freedesktop.org, Nirmoy Das <nirmoy.das@intel.com>
Subject: Re: [Intel-gfx] [PATCH] drm/i915/display: Fix phys_base to be
 relative not absolute
Message-ID: <ZVfw3ghfBLdHB7uk@google.com>
References: <20231105172718.18673-1-pazz@chromium.org>
 <ZVQ3d8FFqxsy0OX7@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZVQ3d8FFqxsy0OX7@intel.com>
Precedence: bulk

On Tue, Nov 14, 2023 at 10:13:59PM -0500, Rodrigo Vivi wrote:
> On Sun, Nov 05, 2023 at 05:27:03PM +0000, Paz Zcharya wrote:
> > Fix the value of variable `phys_base` to be the relative offset in
> > stolen memory, and not the absolute offset of the GSM.
> 
> to me it looks like the other way around. phys_base is the physical
> base address for the frame_buffer. Setting it to zero doesn't seem
> to make that relative. And also doesn't look right.
>
> > 
> > Currently, the value of `phys_base` is set to "Surface Base Address,"
> > which in the case of Meter Lake is 0xfc00_0000.
> 
> I don't believe this is a fixed value. IIRC this comes from the register
> set by video bios, where the idea is to reuse the fb that was used so
> far.
> 
> With this in mind I don't understand how that could overflow. Maybe
> the size of the stolen is not right? maybe the size? maybe different
> memory region?
>

Hi Rodrigo, thanks for the great comments.

Apologies for using a wrong/confusing terminology. I think 'phys_base'
is supposed to be the offset in the GEM BO, where base (or
"Surface Base Address") is supposed to be the GTT offset.

Other than what I wrote before, I noticed that the function 'i915_vma_pin'
which calls to 'i915_gem_gtt_reserve' is the one that binds the right
address space in the GTT for that stolen region.

I see that in the function 'i915_vma_insert' (full call stack below),
where if (flags & PIN_OFFSET_FIXED), then when calling 'i915_gem_gtt_reserve'
we add an offset.

Specifically in MeteorLake, and specifically when using GOP driver, this
offset is equal to 0xfc00_0000. But as you mentioned, this is not strict.

The if statement always renders true because in the function
'initial_plane_vma' we always set
pinctl = PIN_GLOBAL | PIN_OFFSET_FIXED | base;
where pinctl == flags (see file 'intel_plane_initial.c' line 145).

Call stack:
drm_mm_reserve_node
i915_gem_gtt_reserve
	i915_vma_insert
i915_vma_pin_ww
i915_vma_pin
initial_plane_vma
intel_alloc_initial_plane_obj
intel_find_initial_plane_obj

Therefore, I believe the variable 'phys_base' in the
function 'initial_plane_vma,' should be the the offset in the GEM BO
and not the GTT offset, and because the base is added later on
in the function 'i915_gem_gtt_reserve', this value should not be
equal to base and be 0.

Hope it makes more sense.

> > This causes the
> > function `i915_gem_object_create_region_at` to fail in line 128, when
> > it attempts to verify that the range does not overflow:
> > 
> > if (range_overflows(offset, size, resource_size(&mem->region)))
> >       return ERR_PTR(-EINVAL);
> > 
> > where:
> >   offset = 0xfc000000
> >   size = 0x8ca000
> >   mem->region.end + 1 = 0x4400000
> >   mem->region.start = 0x800000
> >   resource_size(&mem->region) = 0x3c00000
> > 
> > call stack:
> >   i915_gem_object_create_region_at
> >   initial_plane_vma
> >   intel_alloc_initial_plane_obj
> >   intel_find_initial_plane_obj
> >   intel_crtc_initial_plane_config
> > 
> > Looking at the flow coming next, we see that `phys_base` is only used
> > once, in function `_i915_gem_object_stolen_init`, in the context of
> > the offset *in* the stolen memory. Combining that with an
> > examinination of the history of the file seems to indicate the
> > current value set is invalid.
> > 
> > call stack (functions using `phys_base`)
> >   _i915_gem_object_stolen_init
> >   __i915_gem_object_create_region
> >   i915_gem_object_create_region_at
> >   initial_plane_vma
> >   intel_alloc_initial_plane_obj
> >   intel_find_initial_plane_obj
> >   intel_crtc_initial_plane_config
> > 
> > [drm:_i915_gem_object_stolen_init] creating preallocated stolen
> > object: stolen_offset=0x0000000000000000, size=0x00000000008ca000
> > 
> > Signed-off-by: Paz Zcharya <pazz@chromium.org>
> > ---
> > 
> >  drivers/gpu/drm/i915/display/intel_plane_initial.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_plane_initial.c b/drivers/gpu/drm/i915/display/intel_plane_initial.c
> > index a55c09cbd0e4..e696cb13756a 100644
> > --- a/drivers/gpu/drm/i915/display/intel_plane_initial.c
> > +++ b/drivers/gpu/drm/i915/display/intel_plane_initial.c
> > @@ -90,7 +90,7 @@ initial_plane_vma(struct drm_i915_private *i915,
> >  			"Using phys_base=%pa, based on initial plane programming\n",
> >  			&phys_base);
> >  	} else {
> > -		phys_base = base;
> > +		phys_base = 0;
> >  		mem = i915->mm.stolen_region;
> >  	}
> >  
> > -- 
> > 2.42.0.869.gea05f2083d-goog
> >