| Message ID | 1454093192-23857-1-git-send-email-paulo.r.zanoni@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jan 29, 2016 at 04:46:30PM -0200, Paulo Zanoni wrote: > The interesting thing is that if we don't do this, we still get a > Y tiled framebuffer, but there won't be a fence around it, which makes > the GTT mmaps less interesting. Is this a Kernel bug? I think some tests currently depend on not having a fence for Y tiled fbs. So this could break stuff. > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > --- > lib/igt_fb.c | 39 ++++++++++++++++++++------------------- > 1 file changed, 20 insertions(+), 19 deletions(-) > > diff --git a/lib/igt_fb.c b/lib/igt_fb.c > index 5f23136..efdd793 100644 > --- a/lib/igt_fb.c > +++ b/lib/igt_fb.c > @@ -73,6 +73,22 @@ static struct format_desc_struct { > #define for_each_format(f) \ > for (f = format_desc; f - format_desc < ARRAY_SIZE(format_desc); f++) > > +static unsigned int fb_mod_to_obj_tiling(uint64_t fb_mod) > +{ > + switch (fb_mod) { > + case LOCAL_DRM_FORMAT_MOD_NONE: > + return I915_TILING_NONE; > + case LOCAL_I915_FORMAT_MOD_X_TILED: > + return I915_TILING_X; > + case LOCAL_I915_FORMAT_MOD_Y_TILED: > + return I915_TILING_Y; > + case LOCAL_I915_FORMAT_MOD_Yf_TILED: > + return I915_TILING_Yf; > + default: > + igt_assert(0); > + } > +} > + > static void igt_get_fb_tile_size(int fd, uint64_t tiling, int fb_bpp, > unsigned *width_ret, unsigned *height_ret) > { > @@ -191,9 +207,10 @@ static int create_bo_for_fb(int fd, int width, int height, int bpp, > > gem_handle = gem_create(fd, bo_size); > > - if (tiling == LOCAL_I915_FORMAT_MOD_X_TILED) > - ret = __gem_set_tiling(fd, gem_handle, I915_TILING_X, > - bo_stride); > + if (tiling == LOCAL_I915_FORMAT_MOD_X_TILED || > + tiling == LOCAL_I915_FORMAT_MOD_Y_TILED) > + ret = __gem_set_tiling(fd, gem_handle, > + fb_mod_to_obj_tiling(tiling), bo_stride); > > *stride_ret = bo_stride; > *size_ret = bo_size; > @@ -862,22 +879,6 @@ struct fb_blit_upload { > } linear; > }; > > -static unsigned int fb_mod_to_obj_tiling(uint64_t fb_mod) > -{ > - switch (fb_mod) { > - case LOCAL_DRM_FORMAT_MOD_NONE: > - return I915_TILING_NONE; > - case LOCAL_I915_FORMAT_MOD_X_TILED: > - return I915_TILING_X; > - case LOCAL_I915_FORMAT_MOD_Y_TILED: > - return I915_TILING_Y; > - case LOCAL_I915_FORMAT_MOD_Yf_TILED: > - return I915_TILING_Yf; > - default: > - igt_assert(0); > - } > -} > - > static void destroy_cairo_surface__blit(void *arg) > { > struct fb_blit_upload *blit = arg; > -- > 2.7.0.rc3 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Em Sex, 2016-01-29 às 21:06 +0200, Ville Syrjälä escreveu: > On Fri, Jan 29, 2016 at 04:46:30PM -0200, Paulo Zanoni wrote: > > The interesting thing is that if we don't do this, we still get a > > Y tiled framebuffer, but there won't be a fence around it, which > > makes > > the GTT mmaps less interesting. Is this a Kernel bug? > > I think some tests currently depend on not having a fence for Y tiled > fbs. So this could break stuff. Do you have any additional information that could help me discover which ones? A quick look on the IGT tests mentioning tiling didn't point anything obvious. Besides, I think it's probably not a good idea to have such a high level helper function behaving differently depending on the tiling type, I'd vote to either call set_tiling on both or on none. > > > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > --- > > lib/igt_fb.c | 39 ++++++++++++++++++++------------------- > > 1 file changed, 20 insertions(+), 19 deletions(-) > > > > diff --git a/lib/igt_fb.c b/lib/igt_fb.c > > index 5f23136..efdd793 100644 > > --- a/lib/igt_fb.c > > +++ b/lib/igt_fb.c > > @@ -73,6 +73,22 @@ static struct format_desc_struct { > > #define for_each_format(f) \ > > for (f = format_desc; f - format_desc < > > ARRAY_SIZE(format_desc); f++) > > > > +static unsigned int fb_mod_to_obj_tiling(uint64_t fb_mod) > > +{ > > + switch (fb_mod) { > > + case LOCAL_DRM_FORMAT_MOD_NONE: > > + return I915_TILING_NONE; > > + case LOCAL_I915_FORMAT_MOD_X_TILED: > > + return I915_TILING_X; > > + case LOCAL_I915_FORMAT_MOD_Y_TILED: > > + return I915_TILING_Y; > > + case LOCAL_I915_FORMAT_MOD_Yf_TILED: > > + return I915_TILING_Yf; > > + default: > > + igt_assert(0); > > + } > > +} > > + > > static void igt_get_fb_tile_size(int fd, uint64_t tiling, int > > fb_bpp, > > unsigned *width_ret, unsigned > > *height_ret) > > { > > @@ -191,9 +207,10 @@ static int create_bo_for_fb(int fd, int width, > > int height, int bpp, > > > > gem_handle = gem_create(fd, bo_size); > > > > - if (tiling == LOCAL_I915_FORMAT_MOD_X_TILED) > > - ret = __gem_set_tiling(fd, gem_handle, > > I915_TILING_X, > > - bo_stride); > > + if (tiling == LOCAL_I915_FORMAT_MOD_X_TILED || > > + tiling == LOCAL_I915_FORMAT_MOD_Y_TILED) > > + ret = __gem_set_tiling(fd, gem_handle, > > + fb_mod_to_obj_tiling(tiling) > > , bo_stride); > > > > *stride_ret = bo_stride; > > *size_ret = bo_size; > > @@ -862,22 +879,6 @@ struct fb_blit_upload { > > } linear; > > }; > > > > -static unsigned int fb_mod_to_obj_tiling(uint64_t fb_mod) > > -{ > > - switch (fb_mod) { > > - case LOCAL_DRM_FORMAT_MOD_NONE: > > - return I915_TILING_NONE; > > - case LOCAL_I915_FORMAT_MOD_X_TILED: > > - return I915_TILING_X; > > - case LOCAL_I915_FORMAT_MOD_Y_TILED: > > - return I915_TILING_Y; > > - case LOCAL_I915_FORMAT_MOD_Yf_TILED: > > - return I915_TILING_Yf; > > - default: > > - igt_assert(0); > > - } > > -} > > - > > static void destroy_cairo_surface__blit(void *arg) > > { > > struct fb_blit_upload *blit = arg; > > -- > > 2.7.0.rc3 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx >
On Mon, Feb 01, 2016 at 05:16:25PM +0000, Zanoni, Paulo R wrote: > Em Sex, 2016-01-29 às 21:06 +0200, Ville Syrjälä escreveu: > > On Fri, Jan 29, 2016 at 04:46:30PM -0200, Paulo Zanoni wrote: > > > The interesting thing is that if we don't do this, we still get a > > > Y tiled framebuffer, but there won't be a fence around it, which > > > makes > > > the GTT mmaps less interesting. Is this a Kernel bug? > > > > I think some tests currently depend on not having a fence for Y tiled > > fbs. So this could break stuff. > > Do you have any additional information that could help me discover > which ones? A quick look on the IGT tests mentioning tiling didn't > point anything obvious. Some Y tiling/rotation ones, can't recall the specific details. There was some discussion on this recently when I suggested that we could just use a fence on Y tiled buffers as well. Also if you look further, the cairo glue will actually use a linear temp buffer for rendering and then blit between the linear temp buffer and the Y tiled fb. Though it's only implemented for SKL+ fast blit so far. I have a patch locally adding support for the regular blit as well since I wanted to play around with that stuff on gen7/X tiled too. > > Besides, I think it's probably not a good idea to have such a high > level helper function behaving differently depending on the tiling > type, I'd vote to either call set_tiling on both or on none. > > > > > > > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > --- > > > lib/igt_fb.c | 39 ++++++++++++++++++++------------------- > > > 1 file changed, 20 insertions(+), 19 deletions(-) > > > > > > diff --git a/lib/igt_fb.c b/lib/igt_fb.c > > > index 5f23136..efdd793 100644 > > > --- a/lib/igt_fb.c > > > +++ b/lib/igt_fb.c > > > @@ -73,6 +73,22 @@ static struct format_desc_struct { > > > #define for_each_format(f) \ > > > for (f = format_desc; f - format_desc < > > > ARRAY_SIZE(format_desc); f++) > > > > > > +static unsigned int fb_mod_to_obj_tiling(uint64_t fb_mod) > > > +{ > > > + switch (fb_mod) { > > > + case LOCAL_DRM_FORMAT_MOD_NONE: > > > + return I915_TILING_NONE; > > > + case LOCAL_I915_FORMAT_MOD_X_TILED: > > > + return I915_TILING_X; > > > + case LOCAL_I915_FORMAT_MOD_Y_TILED: > > > + return I915_TILING_Y; > > > + case LOCAL_I915_FORMAT_MOD_Yf_TILED: > > > + return I915_TILING_Yf; > > > + default: > > > + igt_assert(0); > > > + } > > > +} > > > + > > > static void igt_get_fb_tile_size(int fd, uint64_t tiling, int > > > fb_bpp, > > > unsigned *width_ret, unsigned > > > *height_ret) > > > { > > > @@ -191,9 +207,10 @@ static int create_bo_for_fb(int fd, int width, > > > int height, int bpp, > > > > > > gem_handle = gem_create(fd, bo_size); > > > > > > - if (tiling == LOCAL_I915_FORMAT_MOD_X_TILED) > > > - ret = __gem_set_tiling(fd, gem_handle, > > > I915_TILING_X, > > > - bo_stride); > > > + if (tiling == LOCAL_I915_FORMAT_MOD_X_TILED || > > > + tiling == LOCAL_I915_FORMAT_MOD_Y_TILED) > > > + ret = __gem_set_tiling(fd, gem_handle, > > > + fb_mod_to_obj_tiling(tiling) > > > , bo_stride); > > > > > > *stride_ret = bo_stride; > > > *size_ret = bo_size; > > > @@ -862,22 +879,6 @@ struct fb_blit_upload { > > > } linear; > > > }; > > > > > > -static unsigned int fb_mod_to_obj_tiling(uint64_t fb_mod) > > > -{ > > > - switch (fb_mod) { > > > - case LOCAL_DRM_FORMAT_MOD_NONE: > > > - return I915_TILING_NONE; > > > - case LOCAL_I915_FORMAT_MOD_X_TILED: > > > - return I915_TILING_X; > > > - case LOCAL_I915_FORMAT_MOD_Y_TILED: > > > - return I915_TILING_Y; > > > - case LOCAL_I915_FORMAT_MOD_Yf_TILED: > > > - return I915_TILING_Yf; > > > - default: > > > - igt_assert(0); > > > - } > > > -} > > > - > > > static void destroy_cairo_surface__blit(void *arg) > > > { > > > struct fb_blit_upload *blit = arg; > > > -- > > > 2.7.0.rc3 > > > > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > >
On 01/02/16 17:16, Zanoni, Paulo R wrote: > Em Sex, 2016-01-29 às 21:06 +0200, Ville Syrjälä escreveu: >> On Fri, Jan 29, 2016 at 04:46:30PM -0200, Paulo Zanoni wrote: >>> The interesting thing is that if we don't do this, we still get a >>> Y tiled framebuffer, but there won't be a fence around it, which >>> makes >>> the GTT mmaps less interesting. Is this a Kernel bug? >> >> I think some tests currently depend on not having a fence for Y tiled >> fbs. So this could break stuff. > > Do you have any additional information that could help me discover > which ones? A quick look on the IGT tests mentioning tiling didn't > point anything obvious. > > Besides, I think it's probably not a good idea to have such a high > level helper function behaving differently depending on the tiling > type, I'd vote to either call set_tiling on both or on none. Noticed the thread by accident. :) I can't help with the question of which tests might be affected by this. Some low level ones like kms_addfb don't use the fb helpers so they shouldn't be. Can't remember if any other would be. But just a little bit of background: Basically with the introduction of Y tiled (and Yf) scanout in Gen9 we have forked the path and destroyed the coupling between obj->tiling and framebuffer tiling. The X special casing in create_bo_for_fb is for compatibility with old userspace, but going forward it was decided fb modifiers should be used to tell the driver about tiling and get/set_tiling ioctl is about fencing and only that. Paths implemented in IGT back then were rendering to Y and Yf tiling fbs via a temporary linear surface which is then blitted (blit?) to the real fb obj. (With the blitter doing the appropriate transformation.) So in that respect adding Y tiling to create_bo_for_fb would be wrong because it is not aligned with the above, and also you cannot support Yf this way at all. But I do agree this creates a problem for some use cases within the IGT since the fb and backing obj are created atomically and once that is done you cannot fiddle with obj->tiling (aka fencing). Regards, Tvrtko
On Mon, Feb 01, 2016 at 05:44:42PM +0000, Tvrtko Ursulin wrote: > > On 01/02/16 17:16, Zanoni, Paulo R wrote: > > Em Sex, 2016-01-29 às 21:06 +0200, Ville Syrjälä escreveu: > >> On Fri, Jan 29, 2016 at 04:46:30PM -0200, Paulo Zanoni wrote: > >>> The interesting thing is that if we don't do this, we still get a > >>> Y tiled framebuffer, but there won't be a fence around it, which > >>> makes > >>> the GTT mmaps less interesting. Is this a Kernel bug? > >> > >> I think some tests currently depend on not having a fence for Y tiled > >> fbs. So this could break stuff. > > > > Do you have any additional information that could help me discover > > which ones? A quick look on the IGT tests mentioning tiling didn't > > point anything obvious. > > > > Besides, I think it's probably not a good idea to have such a high > > level helper function behaving differently depending on the tiling > > type, I'd vote to either call set_tiling on both or on none. > > Noticed the thread by accident. :) > > I can't help with the question of which tests might be affected by this. > Some low level ones like kms_addfb don't use the fb helpers so they > shouldn't be. Can't remember if any other would be. > > But just a little bit of background: > > Basically with the introduction of Y tiled (and Yf) scanout in Gen9 we > have forked the path and destroyed the coupling between obj->tiling and > framebuffer tiling. > > The X special casing in create_bo_for_fb is for compatibility with old > userspace, but going forward it was decided fb modifiers should be used > to tell the driver about tiling and get/set_tiling ioctl is about > fencing and only that. > > Paths implemented in IGT back then were rendering to Y and Yf tiling fbs > via a temporary linear surface which is then blitted (blit?) to the real > fb obj. (With the blitter doing the appropriate transformation.) > > So in that respect adding Y tiling to create_bo_for_fb would be wrong > because it is not aligned with the above, and also you cannot support Yf > this way at all. > > But I do agree this creates a problem for some use cases within the IGT > since the fb and backing obj are created atomically and once that is > done you cannot fiddle with obj->tiling (aka fencing). I suppose we could either make it easier to create the obj and fb separately, or we could add a parameter to the fb funcs to indicate whether we want a fence or not.
On 01/02/16 17:57, Ville Syrjälä wrote: > On Mon, Feb 01, 2016 at 05:44:42PM +0000, Tvrtko Ursulin wrote: >> >> On 01/02/16 17:16, Zanoni, Paulo R wrote: >>> Em Sex, 2016-01-29 às 21:06 +0200, Ville Syrjälä escreveu: >>>> On Fri, Jan 29, 2016 at 04:46:30PM -0200, Paulo Zanoni wrote: >>>>> The interesting thing is that if we don't do this, we still get a >>>>> Y tiled framebuffer, but there won't be a fence around it, which >>>>> makes >>>>> the GTT mmaps less interesting. Is this a Kernel bug? >>>> >>>> I think some tests currently depend on not having a fence for Y tiled >>>> fbs. So this could break stuff. >>> >>> Do you have any additional information that could help me discover >>> which ones? A quick look on the IGT tests mentioning tiling didn't >>> point anything obvious. >>> >>> Besides, I think it's probably not a good idea to have such a high >>> level helper function behaving differently depending on the tiling >>> type, I'd vote to either call set_tiling on both or on none. >> >> Noticed the thread by accident. :) >> >> I can't help with the question of which tests might be affected by this. >> Some low level ones like kms_addfb don't use the fb helpers so they >> shouldn't be. Can't remember if any other would be. >> >> But just a little bit of background: >> >> Basically with the introduction of Y tiled (and Yf) scanout in Gen9 we >> have forked the path and destroyed the coupling between obj->tiling and >> framebuffer tiling. >> >> The X special casing in create_bo_for_fb is for compatibility with old >> userspace, but going forward it was decided fb modifiers should be used >> to tell the driver about tiling and get/set_tiling ioctl is about >> fencing and only that. >> >> Paths implemented in IGT back then were rendering to Y and Yf tiling fbs >> via a temporary linear surface which is then blitted (blit?) to the real >> fb obj. (With the blitter doing the appropriate transformation.) >> >> So in that respect adding Y tiling to create_bo_for_fb would be wrong >> because it is not aligned with the above, and also you cannot support Yf >> this way at all. >> >> But I do agree this creates a problem for some use cases within the IGT >> since the fb and backing obj are created atomically and once that is >> done you cannot fiddle with obj->tiling (aka fencing). > > I suppose we could either make it easier to create the obj and fb > separately, or we could add a parameter to the fb funcs to indicate > whether we want a fence or not. Either way sounds good to me. Will depend on whatever fits better with what Paulo is working on at the moment. Regards, Tvrtko
On Tue, Feb 02, 2016 at 09:34:11AM +0000, Tvrtko Ursulin wrote: > > On 01/02/16 17:57, Ville Syrjälä wrote: > >On Mon, Feb 01, 2016 at 05:44:42PM +0000, Tvrtko Ursulin wrote: > >> > >>On 01/02/16 17:16, Zanoni, Paulo R wrote: > >>>Em Sex, 2016-01-29 às 21:06 +0200, Ville Syrjälä escreveu: > >>>>On Fri, Jan 29, 2016 at 04:46:30PM -0200, Paulo Zanoni wrote: > >>>>>The interesting thing is that if we don't do this, we still get a > >>>>>Y tiled framebuffer, but there won't be a fence around it, which > >>>>>makes > >>>>>the GTT mmaps less interesting. Is this a Kernel bug? > >>>> > >>>>I think some tests currently depend on not having a fence for Y tiled > >>>>fbs. So this could break stuff. > >>> > >>>Do you have any additional information that could help me discover > >>>which ones? A quick look on the IGT tests mentioning tiling didn't > >>>point anything obvious. > >>> > >>>Besides, I think it's probably not a good idea to have such a high > >>>level helper function behaving differently depending on the tiling > >>>type, I'd vote to either call set_tiling on both or on none. > >> > >>Noticed the thread by accident. :) > >> > >>I can't help with the question of which tests might be affected by this. > >>Some low level ones like kms_addfb don't use the fb helpers so they > >>shouldn't be. Can't remember if any other would be. > >> > >>But just a little bit of background: > >> > >>Basically with the introduction of Y tiled (and Yf) scanout in Gen9 we > >>have forked the path and destroyed the coupling between obj->tiling and > >>framebuffer tiling. > >> > >>The X special casing in create_bo_for_fb is for compatibility with old > >>userspace, but going forward it was decided fb modifiers should be used > >>to tell the driver about tiling and get/set_tiling ioctl is about > >>fencing and only that. > >> > >>Paths implemented in IGT back then were rendering to Y and Yf tiling fbs > >>via a temporary linear surface which is then blitted (blit?) to the real > >>fb obj. (With the blitter doing the appropriate transformation.) > >> > >>So in that respect adding Y tiling to create_bo_for_fb would be wrong > >>because it is not aligned with the above, and also you cannot support Yf > >>this way at all. > >> > >>But I do agree this creates a problem for some use cases within the IGT > >>since the fb and backing obj are created atomically and once that is > >>done you cannot fiddle with obj->tiling (aka fencing). > > > >I suppose we could either make it easier to create the obj and fb > >separately, or we could add a parameter to the fb funcs to indicate > >whether we want a fence or not. > > Either way sounds good to me. Will depend on whatever fits better with what > Paulo is working on at the moment. I'm ok with not tiling by default for anything but X-tiled in the igt_fb code. Users can just call set_tiling on the underlying bo if they want it, but in the shiny new world of Yf/Ys we should by default not use gtt mmaps really for anything. See also some of the changes planned and then cancelled/delayed for future products, where the gtt might go poof entirely. -Daniel
diff --git a/lib/igt_fb.c b/lib/igt_fb.c index 5f23136..efdd793 100644 --- a/lib/igt_fb.c +++ b/lib/igt_fb.c @@ -73,6 +73,22 @@ static struct format_desc_struct { #define for_each_format(f) \ for (f = format_desc; f - format_desc < ARRAY_SIZE(format_desc); f++) +static unsigned int fb_mod_to_obj_tiling(uint64_t fb_mod) +{ + switch (fb_mod) { + case LOCAL_DRM_FORMAT_MOD_NONE: + return I915_TILING_NONE; + case LOCAL_I915_FORMAT_MOD_X_TILED: + return I915_TILING_X; + case LOCAL_I915_FORMAT_MOD_Y_TILED: + return I915_TILING_Y; + case LOCAL_I915_FORMAT_MOD_Yf_TILED: + return I915_TILING_Yf; + default: + igt_assert(0); + } +} + static void igt_get_fb_tile_size(int fd, uint64_t tiling, int fb_bpp, unsigned *width_ret, unsigned *height_ret) { @@ -191,9 +207,10 @@ static int create_bo_for_fb(int fd, int width, int height, int bpp, gem_handle = gem_create(fd, bo_size); - if (tiling == LOCAL_I915_FORMAT_MOD_X_TILED) - ret = __gem_set_tiling(fd, gem_handle, I915_TILING_X, - bo_stride); + if (tiling == LOCAL_I915_FORMAT_MOD_X_TILED || + tiling == LOCAL_I915_FORMAT_MOD_Y_TILED) + ret = __gem_set_tiling(fd, gem_handle, + fb_mod_to_obj_tiling(tiling), bo_stride); *stride_ret = bo_stride; *size_ret = bo_size; @@ -862,22 +879,6 @@ struct fb_blit_upload { } linear; }; -static unsigned int fb_mod_to_obj_tiling(uint64_t fb_mod) -{ - switch (fb_mod) { - case LOCAL_DRM_FORMAT_MOD_NONE: - return I915_TILING_NONE; - case LOCAL_I915_FORMAT_MOD_X_TILED: - return I915_TILING_X; - case LOCAL_I915_FORMAT_MOD_Y_TILED: - return I915_TILING_Y; - case LOCAL_I915_FORMAT_MOD_Yf_TILED: - return I915_TILING_Yf; - default: - igt_assert(0); - } -} - static void destroy_cairo_surface__blit(void *arg) { struct fb_blit_upload *blit = arg;
The interesting thing is that if we don't do this, we still get a Y tiled framebuffer, but there won't be a fence around it, which makes the GTT mmaps less interesting. Is this a Kernel bug? Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> --- lib/igt_fb.c | 39 ++++++++++++++++++++------------------- 1 file changed, 20 insertions(+), 19 deletions(-)