| Message ID | 1427749208-8961-5-git-send-email-anuj.phogat@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Mar 30, 2015 at 02:00:07PM -0700, Anuj Phogat wrote: > Signed-off-by: Anuj Phogat <anuj.phogat@gmail.com> > --- > intel/intel_bufmgr_gem.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c > index 7c50e26..775a9f9 100644 > --- a/intel/intel_bufmgr_gem.c > +++ b/intel/intel_bufmgr_gem.c > @@ -289,8 +289,13 @@ drm_intel_gem_bo_tile_size(drm_intel_bufmgr_gem *bufmgr_gem, unsigned long size, > if (*tiling_mode == I915_TILING_NONE) > return size; > > + /* Tiled surface base addresses must be tile aligned (64KB aligned > + * for TileYS, 4KB aligned for all other tile modes). > + */ > + if (*tiling_mode == I915_TILING_YS) > + return ROUND_UP_TO(size, 64 * 1024); > /* 965+ just need multiples of page size for tiling */ > - if (bufmgr_gem->gen >= 4) > + else if (bufmgr_gem->gen >= 4) > return ROUND_UP_TO(size, 4096); I'm confused. You're saying you want to align the address of those buffers to 64k, but here we're talking about the object size. At the moment, the kernel places buffers in the address space and it was chosen that the kernel didn't need to know about those tiling formats. So we need something else if that constraint is indeed true (could you tell us the source for this assertion? privately if needed). Thanks,
On Tue, Mar 31, 2015 at 7:26 AM, Damien Lespiau <damien.lespiau@intel.com> wrote: > On Mon, Mar 30, 2015 at 02:00:07PM -0700, Anuj Phogat wrote: >> Signed-off-by: Anuj Phogat <anuj.phogat@gmail.com> >> --- >> intel/intel_bufmgr_gem.c | 7 ++++++- >> 1 file changed, 6 insertions(+), 1 deletion(-) >> >> diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c >> index 7c50e26..775a9f9 100644 >> --- a/intel/intel_bufmgr_gem.c >> +++ b/intel/intel_bufmgr_gem.c >> @@ -289,8 +289,13 @@ drm_intel_gem_bo_tile_size(drm_intel_bufmgr_gem *bufmgr_gem, unsigned long size, >> if (*tiling_mode == I915_TILING_NONE) >> return size; >> >> + /* Tiled surface base addresses must be tile aligned (64KB aligned >> + * for TileYS, 4KB aligned for all other tile modes). >> + */ >> + if (*tiling_mode == I915_TILING_YS) >> + return ROUND_UP_TO(size, 64 * 1024); >> /* 965+ just need multiples of page size for tiling */ >> - if (bufmgr_gem->gen >= 4) >> + else if (bufmgr_gem->gen >= 4) >> return ROUND_UP_TO(size, 4096); > > I'm confused. You're saying you want to align the address of those > buffers to 64k, but here we're talking about the object size. At the > moment, the kernel places buffers in the address space and it was chosen > that the kernel didn't need to know about those tiling formats. So we > need something else if that constraint is indeed true (could you tell > us the source for this assertion? privately if needed). > This comment is invalid here. It was meant for surface state in Mesa. I'll remove it. > Thanks, > > -- > Damien
On Tue, Mar 31, 2015 at 10:49:22AM -0700, Anuj Phogat wrote: > On Tue, Mar 31, 2015 at 7:26 AM, Damien Lespiau > <damien.lespiau@intel.com> wrote: > > On Mon, Mar 30, 2015 at 02:00:07PM -0700, Anuj Phogat wrote: > >> Signed-off-by: Anuj Phogat <anuj.phogat@gmail.com> > >> --- > >> intel/intel_bufmgr_gem.c | 7 ++++++- > >> 1 file changed, 6 insertions(+), 1 deletion(-) > >> > >> diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c > >> index 7c50e26..775a9f9 100644 > >> --- a/intel/intel_bufmgr_gem.c > >> +++ b/intel/intel_bufmgr_gem.c > >> @@ -289,8 +289,13 @@ drm_intel_gem_bo_tile_size(drm_intel_bufmgr_gem *bufmgr_gem, unsigned long size, > >> if (*tiling_mode == I915_TILING_NONE) > >> return size; > >> > >> + /* Tiled surface base addresses must be tile aligned (64KB aligned > >> + * for TileYS, 4KB aligned for all other tile modes). > >> + */ > >> + if (*tiling_mode == I915_TILING_YS) > >> + return ROUND_UP_TO(size, 64 * 1024); > >> /* 965+ just need multiples of page size for tiling */ > >> - if (bufmgr_gem->gen >= 4) > >> + else if (bufmgr_gem->gen >= 4) > >> return ROUND_UP_TO(size, 4096); > > > > I'm confused. You're saying you want to align the address of those > > buffers to 64k, but here we're talking about the object size. At the > > moment, the kernel places buffers in the address space and it was chosen > > that the kernel didn't need to know about those tiling formats. So we > > need something else if that constraint is indeed true (could you tell > > us the source for this assertion? privately if needed). > > > This comment is invalid here. It was meant for surface state in Mesa. > I'll remove it. Don't you have the exact same problem? how does the kernel know about this alignment constraint when resolving the relocation?
On Tue, Mar 31, 2015 at 06:57:07PM +0100, Damien Lespiau wrote: > On Tue, Mar 31, 2015 at 10:49:22AM -0700, Anuj Phogat wrote: > > On Tue, Mar 31, 2015 at 7:26 AM, Damien Lespiau > > <damien.lespiau@intel.com> wrote: > > > On Mon, Mar 30, 2015 at 02:00:07PM -0700, Anuj Phogat wrote: > > >> Signed-off-by: Anuj Phogat <anuj.phogat@gmail.com> > > >> --- > > >> intel/intel_bufmgr_gem.c | 7 ++++++- > > >> 1 file changed, 6 insertions(+), 1 deletion(-) > > >> > > >> diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c > > >> index 7c50e26..775a9f9 100644 > > >> --- a/intel/intel_bufmgr_gem.c > > >> +++ b/intel/intel_bufmgr_gem.c > > >> @@ -289,8 +289,13 @@ drm_intel_gem_bo_tile_size(drm_intel_bufmgr_gem *bufmgr_gem, unsigned long size, > > >> if (*tiling_mode == I915_TILING_NONE) > > >> return size; > > >> > > >> + /* Tiled surface base addresses must be tile aligned (64KB aligned > > >> + * for TileYS, 4KB aligned for all other tile modes). > > >> + */ > > >> + if (*tiling_mode == I915_TILING_YS) > > >> + return ROUND_UP_TO(size, 64 * 1024); > > >> /* 965+ just need multiples of page size for tiling */ > > >> - if (bufmgr_gem->gen >= 4) > > >> + else if (bufmgr_gem->gen >= 4) > > >> return ROUND_UP_TO(size, 4096); > > > > > > I'm confused. You're saying you want to align the address of those > > > buffers to 64k, but here we're talking about the object size. At the > > > moment, the kernel places buffers in the address space and it was chosen > > > that the kernel didn't need to know about those tiling formats. So we > > > need something else if that constraint is indeed true (could you tell > > > us the source for this assertion? privately if needed). > > > > > This comment is invalid here. It was meant for surface state in Mesa. > > I'll remove it. > > Don't you have the exact same problem? how does the kernel know about > this alignment constraint when resolving the relocation? struct drm_i915_gem_exec_object2.alignment goes back to some gen2/3 design ideas that (afaik at least) have never been used really but survived until now. Fancy how we can reuse that 7 generations later ;-) And at least on a quick readthrough the code is all still functional too. -Daniel
On Tue, Mar 31, 2015 at 11:11 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Tue, Mar 31, 2015 at 06:57:07PM +0100, Damien Lespiau wrote: >> On Tue, Mar 31, 2015 at 10:49:22AM -0700, Anuj Phogat wrote: >> > On Tue, Mar 31, 2015 at 7:26 AM, Damien Lespiau >> > <damien.lespiau@intel.com> wrote: >> > > On Mon, Mar 30, 2015 at 02:00:07PM -0700, Anuj Phogat wrote: >> > >> Signed-off-by: Anuj Phogat <anuj.phogat@gmail.com> >> > >> --- >> > >> intel/intel_bufmgr_gem.c | 7 ++++++- >> > >> 1 file changed, 6 insertions(+), 1 deletion(-) >> > >> >> > >> diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c >> > >> index 7c50e26..775a9f9 100644 >> > >> --- a/intel/intel_bufmgr_gem.c >> > >> +++ b/intel/intel_bufmgr_gem.c >> > >> @@ -289,8 +289,13 @@ drm_intel_gem_bo_tile_size(drm_intel_bufmgr_gem *bufmgr_gem, unsigned long size, >> > >> if (*tiling_mode == I915_TILING_NONE) >> > >> return size; >> > >> >> > >> + /* Tiled surface base addresses must be tile aligned (64KB aligned >> > >> + * for TileYS, 4KB aligned for all other tile modes). >> > >> + */ >> > >> + if (*tiling_mode == I915_TILING_YS) >> > >> + return ROUND_UP_TO(size, 64 * 1024); >> > >> /* 965+ just need multiples of page size for tiling */ >> > >> - if (bufmgr_gem->gen >= 4) >> > >> + else if (bufmgr_gem->gen >= 4) >> > >> return ROUND_UP_TO(size, 4096); >> > > >> > > I'm confused. You're saying you want to align the address of those >> > > buffers to 64k, but here we're talking about the object size. At the >> > > moment, the kernel places buffers in the address space and it was chosen >> > > that the kernel didn't need to know about those tiling formats. So we >> > > need something else if that constraint is indeed true (could you tell >> > > us the source for this assertion? privately if needed). >> > > >> > This comment is invalid here. It was meant for surface state in Mesa. >> > I'll remove it. >> >> Don't you have the exact same problem? how does the kernel know about >> this alignment constraint when resolving the relocation? > > struct drm_i915_gem_exec_object2.alignment goes back to some gen2/3 design > ideas that (afaik at least) have never been used really but survived until > now. Fancy how we can reuse that 7 generations later ;-) > > And at least on a quick readthrough the code is all still functional too. Yes, I'm planning to make use of it and pass alignment value for Yf/Ys from Mesa. Thanks. > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c index 7c50e26..775a9f9 100644 --- a/intel/intel_bufmgr_gem.c +++ b/intel/intel_bufmgr_gem.c @@ -289,8 +289,13 @@ drm_intel_gem_bo_tile_size(drm_intel_bufmgr_gem *bufmgr_gem, unsigned long size, if (*tiling_mode == I915_TILING_NONE) return size; + /* Tiled surface base addresses must be tile aligned (64KB aligned + * for TileYS, 4KB aligned for all other tile modes). + */ + if (*tiling_mode == I915_TILING_YS) + return ROUND_UP_TO(size, 64 * 1024); /* 965+ just need multiples of page size for tiling */ - if (bufmgr_gem->gen >= 4) + else if (bufmgr_gem->gen >= 4) return ROUND_UP_TO(size, 4096); /* Older chips need powers of two, of at least 512k or 1M */
Signed-off-by: Anuj Phogat <anuj.phogat@gmail.com> --- intel/intel_bufmgr_gem.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)