| Message ID | 1477328158-10817-1-git-send-email-praveen.paneri@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 24/10/2016 17:55, Praveen Paneri wrote: > This adds Y-tiling check in igt_create_fb_with_bo_size as > now we should also be able to create Y-tiled FBs. > > Signed-off-by: Praveen Paneri <praveen.paneri@intel.com> > --- > lib/igt_fb.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/lib/igt_fb.c b/lib/igt_fb.c > index 47472f4..bf1d372 100644 > --- a/lib/igt_fb.c > +++ b/lib/igt_fb.c > @@ -608,7 +608,8 @@ igt_create_fb_with_bo_size(int fd, int width, int height, > __func__, fb->gem_handle, fb->stride); > > if (tiling != LOCAL_DRM_FORMAT_MOD_NONE && > - tiling != LOCAL_I915_FORMAT_MOD_X_TILED) { > + tiling != LOCAL_I915_FORMAT_MOD_X_TILED && > + tiling != LOCAL_I915_FORMAT_MOD_Y_TILED) { > do_or_die(__kms_addfb(fd, fb->gem_handle, width, height, > fb->stride, format, tiling, > LOCAL_DRM_MODE_FB_MODIFIERS, &fb_id)); > This works now? Ie. doesn't hit "No Y Tiling for legacy addfb" error in the driver? Regards, Tvrtko
On Tue, Oct 25, 2016 at 09:06:26AM +0100, Tvrtko Ursulin wrote: > > On 24/10/2016 17:55, Praveen Paneri wrote: > >This adds Y-tiling check in igt_create_fb_with_bo_size as > >now we should also be able to create Y-tiled FBs. > > > >Signed-off-by: Praveen Paneri <praveen.paneri@intel.com> > >--- > > lib/igt_fb.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > >diff --git a/lib/igt_fb.c b/lib/igt_fb.c > >index 47472f4..bf1d372 100644 > >--- a/lib/igt_fb.c > >+++ b/lib/igt_fb.c > >@@ -608,7 +608,8 @@ igt_create_fb_with_bo_size(int fd, int width, int height, > > __func__, fb->gem_handle, fb->stride); > > > > if (tiling != LOCAL_DRM_FORMAT_MOD_NONE && > >- tiling != LOCAL_I915_FORMAT_MOD_X_TILED) { > >+ tiling != LOCAL_I915_FORMAT_MOD_X_TILED && > >+ tiling != LOCAL_I915_FORMAT_MOD_Y_TILED) { > > do_or_die(__kms_addfb(fd, fb->gem_handle, width, height, > > fb->stride, format, tiling, > > LOCAL_DRM_MODE_FB_MODIFIERS, &fb_id)); > > > > This works now? Ie. doesn't hit "No Y Tiling for legacy addfb" error > in the driver? That legacy check still exists :(, but __kms_addfb() lies and calls addfb2. -Chris
On 25/10/2016 09:18, Chris Wilson wrote: > On Tue, Oct 25, 2016 at 09:06:26AM +0100, Tvrtko Ursulin wrote: >> >> On 24/10/2016 17:55, Praveen Paneri wrote: >>> This adds Y-tiling check in igt_create_fb_with_bo_size as >>> now we should also be able to create Y-tiled FBs. >>> >>> Signed-off-by: Praveen Paneri <praveen.paneri@intel.com> >>> --- >>> lib/igt_fb.c | 3 ++- >>> 1 file changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/lib/igt_fb.c b/lib/igt_fb.c >>> index 47472f4..bf1d372 100644 >>> --- a/lib/igt_fb.c >>> +++ b/lib/igt_fb.c >>> @@ -608,7 +608,8 @@ igt_create_fb_with_bo_size(int fd, int width, int height, >>> __func__, fb->gem_handle, fb->stride); >>> >>> if (tiling != LOCAL_DRM_FORMAT_MOD_NONE && >>> - tiling != LOCAL_I915_FORMAT_MOD_X_TILED) { >>> + tiling != LOCAL_I915_FORMAT_MOD_X_TILED && >>> + tiling != LOCAL_I915_FORMAT_MOD_Y_TILED) { >>> do_or_die(__kms_addfb(fd, fb->gem_handle, width, height, >>> fb->stride, format, tiling, >>> LOCAL_DRM_MODE_FB_MODIFIERS, &fb_id)); >>> >> >> This works now? Ie. doesn't hit "No Y Tiling for legacy addfb" error >> in the driver? > > That legacy check still exists :(, but __kms_addfb() lies and calls addfb2. And this change makes the code not call __kms_addfb but the else branch which does not set the modifiers at all AFAICS. REgards, Tvrtko
On Tue, Oct 25, 2016 at 09:20:31AM +0100, Tvrtko Ursulin wrote: > > > On 25/10/2016 09:18, Chris Wilson wrote: > >On Tue, Oct 25, 2016 at 09:06:26AM +0100, Tvrtko Ursulin wrote: > >> > >>On 24/10/2016 17:55, Praveen Paneri wrote: > >>>This adds Y-tiling check in igt_create_fb_with_bo_size as > >>>now we should also be able to create Y-tiled FBs. > >>> > >>>Signed-off-by: Praveen Paneri <praveen.paneri@intel.com> > >>>--- > >>>lib/igt_fb.c | 3 ++- > >>>1 file changed, 2 insertions(+), 1 deletion(-) > >>> > >>>diff --git a/lib/igt_fb.c b/lib/igt_fb.c > >>>index 47472f4..bf1d372 100644 > >>>--- a/lib/igt_fb.c > >>>+++ b/lib/igt_fb.c > >>>@@ -608,7 +608,8 @@ igt_create_fb_with_bo_size(int fd, int width, int height, > >>> __func__, fb->gem_handle, fb->stride); > >>> > >>> if (tiling != LOCAL_DRM_FORMAT_MOD_NONE && > >>>- tiling != LOCAL_I915_FORMAT_MOD_X_TILED) { > >>>+ tiling != LOCAL_I915_FORMAT_MOD_X_TILED && > >>>+ tiling != LOCAL_I915_FORMAT_MOD_Y_TILED) { > >>> do_or_die(__kms_addfb(fd, fb->gem_handle, width, height, > >>> fb->stride, format, tiling, > >>> LOCAL_DRM_MODE_FB_MODIFIERS, &fb_id)); > >>> > >> > >>This works now? Ie. doesn't hit "No Y Tiling for legacy addfb" error > >>in the driver? > > > >That legacy check still exists :(, but __kms_addfb() lies and calls addfb2. > > And this change makes the code not call __kms_addfb but the else > branch which does not set the modifiers at all AFAICS. Ah. Was looking at the wrong thing, say addfb and thought it was the vanilla ADDFB (which should of course support y-tiling!!!). :( -Chris
On 25/10/2016 13:06, Paneri, Praveen wrote: > Hi Tvrtko, > > Along with this change I made following change in the kernel side. I was not sure if this is a hack of a legitimate change. Could you please give me a pointer about how to move fwd from here? Without this all Y-tiling tests would fail. > > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -15406,8 +15406,7 @@ static int intel_framebuffer_init(struct drm_device *dev, > if (obj->tiling_mode == I915_TILING_X) > mode_cmd->modifier[0] = I915_FORMAT_MOD_X_TILED; > else if (obj->tiling_mode == I915_TILING_Y) { > - DRM_DEBUG("No Y tiling for legacy addfb\n"); > - return -EINVAL; > + mode_cmd->modifier[0] = I915_FORMAT_MOD_Y_TILED; > } > } It would be a controversial change, "beyond my pay grade". :) If you drop this particular IGT patch, you can still create Y tiled framebuffers and display them (as the existing tests already do that). So when you say that all Y tiling tests fail without this kernel hack, which tests you are referring to? Regards, Tvrtko > Regards, > Praveen > > > -----Original Message----- > From: Tvrtko Ursulin [mailto:tvrtko.ursulin@linux.intel.com] > Sent: Tuesday, October 25, 2016 1:36 PM > To: Paneri, Praveen <praveen.paneri@intel.com>; intel-gfx@lists.freedesktop.org > Subject: Re: [Intel-gfx] [PATCH 1/4] igt_fb: Add Y-tiling support > > > On 24/10/2016 17:55, Praveen Paneri wrote: >> This adds Y-tiling check in igt_create_fb_with_bo_size as now we >> should also be able to create Y-tiled FBs. >> >> Signed-off-by: Praveen Paneri <praveen.paneri@intel.com> >> --- >> lib/igt_fb.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/lib/igt_fb.c b/lib/igt_fb.c index 47472f4..bf1d372 100644 >> --- a/lib/igt_fb.c >> +++ b/lib/igt_fb.c >> @@ -608,7 +608,8 @@ igt_create_fb_with_bo_size(int fd, int width, int height, >> __func__, fb->gem_handle, fb->stride); >> >> if (tiling != LOCAL_DRM_FORMAT_MOD_NONE && >> - tiling != LOCAL_I915_FORMAT_MOD_X_TILED) { >> + tiling != LOCAL_I915_FORMAT_MOD_X_TILED && >> + tiling != LOCAL_I915_FORMAT_MOD_Y_TILED) { >> do_or_die(__kms_addfb(fd, fb->gem_handle, width, height, >> fb->stride, format, tiling, >> LOCAL_DRM_MODE_FB_MODIFIERS, &fb_id)); >> > > This works now? Ie. doesn't hit "No Y Tiling for legacy addfb" error in the driver? > > Regards, > > Tvrtko >
> So when you say that all Y tiling tests fail without this kernel hack, which tests you are referring to? If I revert this IGT patch and do not make below kernel change, kms_draw_crc (ytiled cases, last patch in this series) fail with following error. Test assertion failure function igt_assert_crc_equal, file hardware/intel/intel-gpu-tools/lib/igt_debugfs.c:266: Failed assertion: a->crc[i] == b->crc[i] error: 0x68c96b8b != 0x948e53b I think, I shall debug it further and try to fix it without making this change. Regards, Praveen -----Original Message----- From: Tvrtko Ursulin [mailto:tvrtko.ursulin@linux.intel.com] Sent: Tuesday, October 25, 2016 6:07 PM To: Paneri, Praveen <praveen.paneri@intel.com>; intel-gfx@lists.freedesktop.org Subject: Re: [Intel-gfx] [PATCH 1/4] igt_fb: Add Y-tiling support On 25/10/2016 13:06, Paneri, Praveen wrote: > Hi Tvrtko, > > Along with this change I made following change in the kernel side. I was not sure if this is a hack of a legitimate change. Could you please give me a pointer about how to move fwd from here? Without this all Y-tiling tests would fail. > > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -15406,8 +15406,7 @@ static int intel_framebuffer_init(struct drm_device *dev, > if (obj->tiling_mode == I915_TILING_X) > mode_cmd->modifier[0] = I915_FORMAT_MOD_X_TILED; > else if (obj->tiling_mode == I915_TILING_Y) { > - DRM_DEBUG("No Y tiling for legacy addfb\n"); > - return -EINVAL; > + mode_cmd->modifier[0] = > + I915_FORMAT_MOD_Y_TILED; > } > } It would be a controversial change, "beyond my pay grade". :) If you drop this particular IGT patch, you can still create Y tiled framebuffers and display them (as the existing tests already do that). So when you say that all Y tiling tests fail without this kernel hack, which tests you are referring to? Regards, Tvrtko > Regards, > Praveen > > > -----Original Message----- > From: Tvrtko Ursulin [mailto:tvrtko.ursulin@linux.intel.com] > Sent: Tuesday, October 25, 2016 1:36 PM > To: Paneri, Praveen <praveen.paneri@intel.com>; > intel-gfx@lists.freedesktop.org > Subject: Re: [Intel-gfx] [PATCH 1/4] igt_fb: Add Y-tiling support > > > On 24/10/2016 17:55, Praveen Paneri wrote: >> This adds Y-tiling check in igt_create_fb_with_bo_size as now we >> should also be able to create Y-tiled FBs. >> >> Signed-off-by: Praveen Paneri <praveen.paneri@intel.com> >> --- >> lib/igt_fb.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/lib/igt_fb.c b/lib/igt_fb.c index 47472f4..bf1d372 >> 100644 >> --- a/lib/igt_fb.c >> +++ b/lib/igt_fb.c >> @@ -608,7 +608,8 @@ igt_create_fb_with_bo_size(int fd, int width, int height, >> __func__, fb->gem_handle, fb->stride); >> >> if (tiling != LOCAL_DRM_FORMAT_MOD_NONE && >> - tiling != LOCAL_I915_FORMAT_MOD_X_TILED) { >> + tiling != LOCAL_I915_FORMAT_MOD_X_TILED && >> + tiling != LOCAL_I915_FORMAT_MOD_Y_TILED) { >> do_or_die(__kms_addfb(fd, fb->gem_handle, width, height, >> fb->stride, format, tiling, >> LOCAL_DRM_MODE_FB_MODIFIERS, &fb_id)); >> > > This works now? Ie. doesn't hit "No Y Tiling for legacy addfb" error in the driver? > > Regards, > > Tvrtko >
On Tue, Oct 25, 2016 at 03:02:33PM +0000, Paneri, Praveen wrote: > > So when you say that all Y tiling tests fail without this kernel hack, which tests you are referring to? > If I revert this IGT patch and do not make below kernel change, kms_draw_crc (ytiled cases, last patch in this series) fail with following error. > > Test assertion failure function igt_assert_crc_equal, file hardware/intel/intel-gpu-tools/lib/igt_debugfs.c:266: > Failed assertion: a->crc[i] == b->crc[i] > error: 0x68c96b8b != 0x948e53b > > I think, I shall debug it further and try to fix it without making this change. Can't we just set the fb modifiers when creating the drm_fb in the igt helpers? -Daniel > > Regards, > Praveen > > > -----Original Message----- > From: Tvrtko Ursulin [mailto:tvrtko.ursulin@linux.intel.com] > Sent: Tuesday, October 25, 2016 6:07 PM > To: Paneri, Praveen <praveen.paneri@intel.com>; intel-gfx@lists.freedesktop.org > Subject: Re: [Intel-gfx] [PATCH 1/4] igt_fb: Add Y-tiling support > > > On 25/10/2016 13:06, Paneri, Praveen wrote: > > Hi Tvrtko, > > > > Along with this change I made following change in the kernel side. I was not sure if this is a hack of a legitimate change. Could you please give me a pointer about how to move fwd from here? Without this all Y-tiling tests would fail. > > > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -15406,8 +15406,7 @@ static int intel_framebuffer_init(struct drm_device *dev, > > if (obj->tiling_mode == I915_TILING_X) > > mode_cmd->modifier[0] = I915_FORMAT_MOD_X_TILED; > > else if (obj->tiling_mode == I915_TILING_Y) { > > - DRM_DEBUG("No Y tiling for legacy addfb\n"); > > - return -EINVAL; > > + mode_cmd->modifier[0] = > > + I915_FORMAT_MOD_Y_TILED; > > } > > } > > It would be a controversial change, "beyond my pay grade". :) > > If you drop this particular IGT patch, you can still create Y tiled framebuffers and display them (as the existing tests already do that). > So when you say that all Y tiling tests fail without this kernel hack, which tests you are referring to? > > Regards, > > Tvrtko > > > Regards, > > Praveen > > > > > > -----Original Message----- > > From: Tvrtko Ursulin [mailto:tvrtko.ursulin@linux.intel.com] > > Sent: Tuesday, October 25, 2016 1:36 PM > > To: Paneri, Praveen <praveen.paneri@intel.com>; > > intel-gfx@lists.freedesktop.org > > Subject: Re: [Intel-gfx] [PATCH 1/4] igt_fb: Add Y-tiling support > > > > > > On 24/10/2016 17:55, Praveen Paneri wrote: > >> This adds Y-tiling check in igt_create_fb_with_bo_size as now we > >> should also be able to create Y-tiled FBs. > >> > >> Signed-off-by: Praveen Paneri <praveen.paneri@intel.com> > >> --- > >> lib/igt_fb.c | 3 ++- > >> 1 file changed, 2 insertions(+), 1 deletion(-) > >> > >> diff --git a/lib/igt_fb.c b/lib/igt_fb.c index 47472f4..bf1d372 > >> 100644 > >> --- a/lib/igt_fb.c > >> +++ b/lib/igt_fb.c > >> @@ -608,7 +608,8 @@ igt_create_fb_with_bo_size(int fd, int width, int height, > >> __func__, fb->gem_handle, fb->stride); > >> > >> if (tiling != LOCAL_DRM_FORMAT_MOD_NONE && > >> - tiling != LOCAL_I915_FORMAT_MOD_X_TILED) { > >> + tiling != LOCAL_I915_FORMAT_MOD_X_TILED && > >> + tiling != LOCAL_I915_FORMAT_MOD_Y_TILED) { > >> do_or_die(__kms_addfb(fd, fb->gem_handle, width, height, > >> fb->stride, format, tiling, > >> LOCAL_DRM_MODE_FB_MODIFIERS, &fb_id)); > >> > > > > This works now? Ie. doesn't hit "No Y Tiling for legacy addfb" error in the driver? > > > > Regards, > > > > Tvrtko > > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 26/10/2016 07:32, Daniel Vetter wrote: > On Tue, Oct 25, 2016 at 03:02:33PM +0000, Paneri, Praveen wrote: >>> So when you say that all Y tiling tests fail without this kernel hack, which tests you are referring to? >> If I revert this IGT patch and do not make below kernel change, kms_draw_crc (ytiled cases, last patch in this series) fail with following error. >> >> Test assertion failure function igt_assert_crc_equal, file hardware/intel/intel-gpu-tools/lib/igt_debugfs.c:266: >> Failed assertion: a->crc[i] == b->crc[i] >> error: 0x68c96b8b != 0x948e53b >> >> I think, I shall debug it further and try to fix it without making this change. > > Can't we just set the fb modifiers when creating the drm_fb in the igt > helpers? Thats already there, if I understand what you mean. But it only does the modifer and does not set the object tiling. So I suspect this test might be depending on object tiling being set as well? In which case we may be missing the capability to create the object separately from the fb creation, because otherwise it is too late to try to change the object tiling. Regards, Tvrtko
On Wed, Oct 26, 2016 at 09:09:26AM +0100, Tvrtko Ursulin wrote: > > On 26/10/2016 07:32, Daniel Vetter wrote: > > On Tue, Oct 25, 2016 at 03:02:33PM +0000, Paneri, Praveen wrote: > >>> So when you say that all Y tiling tests fail without this kernel hack, which tests you are referring to? > >> If I revert this IGT patch and do not make below kernel change, kms_draw_crc (ytiled cases, last patch in this series) fail with following error. > >> > >> Test assertion failure function igt_assert_crc_equal, file hardware/intel/intel-gpu-tools/lib/igt_debugfs.c:266: > >> Failed assertion: a->crc[i] == b->crc[i] > >> error: 0x68c96b8b != 0x948e53b > >> > >> I think, I shall debug it further and try to fix it without making this change. > > > > Can't we just set the fb modifiers when creating the drm_fb in the igt > > helpers? > > Thats already there, if I understand what you mean. But it only does the > modifer and does not set the object tiling. So I suspect this test might > be depending on object tiling being set as well? In which case we may > be missing the capability to create the object separately from the fb > creation, because otherwise it is too late to try to change the object > tiling. I'm pretty sure I had patches to split the two apart. Assuming I never posted them, they must be on a branch... somewhere.
On Wed, Oct 26, 2016 at 11:20:51AM +0300, Ville Syrjälä wrote: > On Wed, Oct 26, 2016 at 09:09:26AM +0100, Tvrtko Ursulin wrote: > > > > On 26/10/2016 07:32, Daniel Vetter wrote: > > > On Tue, Oct 25, 2016 at 03:02:33PM +0000, Paneri, Praveen wrote: > > >>> So when you say that all Y tiling tests fail without this kernel hack, which tests you are referring to? > > >> If I revert this IGT patch and do not make below kernel change, kms_draw_crc (ytiled cases, last patch in this series) fail with following error. > > >> > > >> Test assertion failure function igt_assert_crc_equal, file hardware/intel/intel-gpu-tools/lib/igt_debugfs.c:266: > > >> Failed assertion: a->crc[i] == b->crc[i] > > >> error: 0x68c96b8b != 0x948e53b > > >> > > >> I think, I shall debug it further and try to fix it without making this change. > > > > > > Can't we just set the fb modifiers when creating the drm_fb in the igt > > > helpers? > > > > Thats already there, if I understand what you mean. But it only does the > > modifer and does not set the object tiling. So I suspect this test might > > be depending on object tiling being set as well? In which case we may > > be missing the capability to create the object separately from the fb > > creation, because otherwise it is too late to try to change the object > > tiling. > > I'm pretty sure I had patches to split the two apart. Assuming I never > posted them, they must be on a branch... somewhere. Failed to find them. So someone will have to reimplement if needed.
On Wed, Oct 26, 2016 at 09:09:26AM +0100, Tvrtko Ursulin wrote: > > On 26/10/2016 07:32, Daniel Vetter wrote: > > On Tue, Oct 25, 2016 at 03:02:33PM +0000, Paneri, Praveen wrote: > > > > So when you say that all Y tiling tests fail without this kernel hack, which tests you are referring to? > > > If I revert this IGT patch and do not make below kernel change, kms_draw_crc (ytiled cases, last patch in this series) fail with following error. > > > > > > Test assertion failure function igt_assert_crc_equal, file hardware/intel/intel-gpu-tools/lib/igt_debugfs.c:266: > > > Failed assertion: a->crc[i] == b->crc[i] > > > error: 0x68c96b8b != 0x948e53b > > > > > > I think, I shall debug it further and try to fix it without making this change. > > > > Can't we just set the fb modifiers when creating the drm_fb in the igt > > helpers? > > Thats already there, if I understand what you mean. But it only does the > modifer and does not set the object tiling. So I suspect this test might be > depending on object tiling being set as well? In which case we may be > missing the capability to create the object separately from the fb creation, > because otherwise it is too late to try to change the object tiling. Yeah I guess then my question is: Why can't we use that? This patch sounds like duct-tape instead of proper bugfix ... -Daniel
diff --git a/lib/igt_fb.c b/lib/igt_fb.c index 47472f4..bf1d372 100644 --- a/lib/igt_fb.c +++ b/lib/igt_fb.c @@ -608,7 +608,8 @@ igt_create_fb_with_bo_size(int fd, int width, int height, __func__, fb->gem_handle, fb->stride); if (tiling != LOCAL_DRM_FORMAT_MOD_NONE && - tiling != LOCAL_I915_FORMAT_MOD_X_TILED) { + tiling != LOCAL_I915_FORMAT_MOD_X_TILED && + tiling != LOCAL_I915_FORMAT_MOD_Y_TILED) { do_or_die(__kms_addfb(fd, fb->gem_handle, width, height, fb->stride, format, tiling, LOCAL_DRM_MODE_FB_MODIFIERS, &fb_id));
This adds Y-tiling check in igt_create_fb_with_bo_size as now we should also be able to create Y-tiled FBs. Signed-off-by: Praveen Paneri <praveen.paneri@intel.com> --- lib/igt_fb.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)