| Message ID | 1436389139-16282-6-git-send-email-przanoni@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jul 08, 2015 at 05:58:58PM -0300, Paulo Zanoni wrote: > From: Paulo Zanoni <paulo.r.zanoni@intel.com> > > I could only find the restrictions for HSW+, but I think it's safe to > assume that the older platforms also can't support the configurations > HSW can't support. The older platforms probably have additional > restrictions, so we need to figure out those and implement them later. > Let's not block HSW+ FBC based on missing information for the older > platforms. > > This may fix kms_frontbuffer_tracking failures depending on your > monitor configuration. > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 1 + > drivers/gpu/drm/i915/intel_fbc.c | 10 ++++++++++ > 2 files changed, 11 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 52d07fb..4a4b2a9 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -936,6 +936,7 @@ struct i915_fbc { > FBC_CHIP_DEFAULT, /* disabled by default on this chip */ > FBC_ROTATION, /* rotation is not supported */ > FBC_IN_DBG_MASTER, /* kernel debugger is active */ > + FBC_BAD_STRIDE, /* stride is not supported */ > } no_fbc_reason; > > bool (*fbc_enabled)(struct drm_i915_private *dev_priv); > diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c > index 0a24e96..7b65b00 100644 > --- a/drivers/gpu/drm/i915/intel_fbc.c > +++ b/drivers/gpu/drm/i915/intel_fbc.c > @@ -501,6 +501,8 @@ const char *intel_no_fbc_reason_str(enum no_fbc_reason reason) > return "rotation unsupported"; > case FBC_IN_DBG_MASTER: > return "Kernel debugger is active"; > + case FBC_BAD_STRIDE: > + return "framebuffer stride not supported"; > default: > MISSING_CASE(reason); > return "unknown reason"; > @@ -804,6 +806,14 @@ static void __intel_fbc_update(struct drm_i915_private *dev_priv) > goto out_disable; > } > > + /* TODO: we need to figure out what are the stride restrictions for the > + * older platforms. */ > + if (fb->pitches[0] < 512 || fb->pitches[0] > 16384 || < 512 is already take care of by the X-tiling restriction (requires a multiple of 512 bytes stride). > 16kb might be the general stride limit, we should probably reject that in intel_framebuffer_init. 64b alignment is strange since X-tiled is stricter, and we that already in intel_fb_stride_alignment. So summary: Sounds like we need to add per-platform stride limit checks to fb create code. Plus igt testcases to make sure we check for this (since right now it seems like we don't). Additional checks here in the fbc code don't seem required. But if you want to I guess you could convert them to WARN_ON (without bailing out). -Daniel > + (fb->pitches[0] & (64 - 1)) != 0) { > + set_no_fbc_reason(dev_priv, FBC_BAD_STRIDE); > + goto out_disable; > + } > + > /* If the kernel debugger is active, always disable compression */ > if (in_dbg_master()) { > set_no_fbc_reason(dev_priv, FBC_IN_DBG_MASTER); > -- > 2.1.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
2015-07-09 14:15 GMT-03:00 Daniel Vetter <daniel@ffwll.ch>: > On Wed, Jul 08, 2015 at 05:58:58PM -0300, Paulo Zanoni wrote: >> From: Paulo Zanoni <paulo.r.zanoni@intel.com> >> >> I could only find the restrictions for HSW+, but I think it's safe to >> assume that the older platforms also can't support the configurations >> HSW can't support. The older platforms probably have additional >> restrictions, so we need to figure out those and implement them later. >> Let's not block HSW+ FBC based on missing information for the older >> platforms. >> >> This may fix kms_frontbuffer_tracking failures depending on your >> monitor configuration. >> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> >> --- >> drivers/gpu/drm/i915/i915_drv.h | 1 + >> drivers/gpu/drm/i915/intel_fbc.c | 10 ++++++++++ >> 2 files changed, 11 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >> index 52d07fb..4a4b2a9 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -936,6 +936,7 @@ struct i915_fbc { >> FBC_CHIP_DEFAULT, /* disabled by default on this chip */ >> FBC_ROTATION, /* rotation is not supported */ >> FBC_IN_DBG_MASTER, /* kernel debugger is active */ >> + FBC_BAD_STRIDE, /* stride is not supported */ >> } no_fbc_reason; >> >> bool (*fbc_enabled)(struct drm_i915_private *dev_priv); >> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c >> index 0a24e96..7b65b00 100644 >> --- a/drivers/gpu/drm/i915/intel_fbc.c >> +++ b/drivers/gpu/drm/i915/intel_fbc.c >> @@ -501,6 +501,8 @@ const char *intel_no_fbc_reason_str(enum no_fbc_reason reason) >> return "rotation unsupported"; >> case FBC_IN_DBG_MASTER: >> return "Kernel debugger is active"; >> + case FBC_BAD_STRIDE: >> + return "framebuffer stride not supported"; >> default: >> MISSING_CASE(reason); >> return "unknown reason"; >> @@ -804,6 +806,14 @@ static void __intel_fbc_update(struct drm_i915_private *dev_priv) >> goto out_disable; >> } >> >> + /* TODO: we need to figure out what are the stride restrictions for the >> + * older platforms. */ >> + if (fb->pitches[0] < 512 || fb->pitches[0] > 16384 || > > < 512 is already take care of by the X-tiling restriction (requires a > multiple of 512 bytes stride) . > 16kb might be the general stride limit, > we should probably reject that in intel_framebuffer_init. 64b alignment is > strange since X-tiled is stricter, and we that already in > intel_fb_stride_alignment. 16384 bytes is just 4096 32bit pixels wide. If you have 2 modern monitors configured in wide mode you already beat that (or maybe 3 not-so-fancy monitors). So if you want FBC you'll have to "xrandr --output DP1 --above eDP1 --auto", insead of "--left-of" (also, pray that the guys developing your desktop environment actually tested this case). So this is certainly a check that we want in the FBC code but not in other places. I can remove the other checks if you want. I also did find that gen3 requires specifically 4k or 8k strides: nothing except for that. But I'll only write this patch much later (gen3 is not a priority for now, as you know). > > So summary: Sounds like we need to add per-platform stride limit checks to > fb create code. As mentioned above: no. FBC can be more strict. > Plus igt testcases to make sure we check for this (since > right now it seems like we don't). It's on the TODO list but it's not a priority since the Kernel checks are very straightforward. One of the problems is that different platforms have different requirements, so I'll have to hardcode those requirements on IGT too. And I'll have to stop using igt_create_fb for that case since it only use powers of 2 for stride. So yeah, one day we may have the test, but not a priority right now since it's very easy to look at the Kernel code and make sure it's correct. > Additional checks here in the fbc code > don't seem required. But if you want to I guess you could convert them to > WARN_ON (without bailing out). > -Daniel > >> + (fb->pitches[0] & (64 - 1)) != 0) { >> + set_no_fbc_reason(dev_priv, FBC_BAD_STRIDE); >> + goto out_disable; >> + } >> + >> /* If the kernel debugger is active, always disable compression */ >> if (in_dbg_master()) { >> set_no_fbc_reason(dev_priv, FBC_IN_DBG_MASTER); >> -- >> 2.1.4 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
2015-07-09 14:28 GMT-03:00 Paulo Zanoni <przanoni@gmail.com>: > 2015-07-09 14:15 GMT-03:00 Daniel Vetter <daniel@ffwll.ch>: >> Plus igt testcases to make sure we check for this (since >> right now it seems like we don't). > > It's on the TODO list but it's not a priority since the Kernel checks > are very straightforward. One of the problems is that different > platforms have different requirements, so I'll have to hardcode those > requirements on IGT too. And I'll have to stop using igt_create_fb for > that case since it only use powers of 2 for stride. > > So yeah, one day we may have the test, but not a priority right now > since it's very easy to look at the Kernel code and make sure it's > correct. Ok, so I implemented fbc-badstride with the restrictions mentioned in this patch: http://cgit.freedesktop.org/xorg/app/intel-gpu-tools/commit/?id=cb3861a9e3f1bc12765160345bb0dd1d543f5086 If we add more restrictions we'll have to update the test. > >> Additional checks here in the fbc code >> don't seem required. But if you want to I guess you could convert them to >> WARN_ON (without bailing out). >> -Daniel >> >>> + (fb->pitches[0] & (64 - 1)) != 0) { >>> + set_no_fbc_reason(dev_priv, FBC_BAD_STRIDE); >>> + goto out_disable; >>> + } >>> + >>> /* If the kernel debugger is active, always disable compression */ >>> if (in_dbg_master()) { >>> set_no_fbc_reason(dev_priv, FBC_IN_DBG_MASTER); >>> -- >>> 2.1.4 >>> >>> _______________________________________________ >>> Intel-gfx mailing list >>> Intel-gfx@lists.freedesktop.org >>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx >> >> -- >> Daniel Vetter >> Software Engineer, Intel Corporation >> http://blog.ffwll.ch > > > > -- > Paulo Zanoni
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 52d07fb..4a4b2a9 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -936,6 +936,7 @@ struct i915_fbc { FBC_CHIP_DEFAULT, /* disabled by default on this chip */ FBC_ROTATION, /* rotation is not supported */ FBC_IN_DBG_MASTER, /* kernel debugger is active */ + FBC_BAD_STRIDE, /* stride is not supported */ } no_fbc_reason; bool (*fbc_enabled)(struct drm_i915_private *dev_priv); diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c index 0a24e96..7b65b00 100644 --- a/drivers/gpu/drm/i915/intel_fbc.c +++ b/drivers/gpu/drm/i915/intel_fbc.c @@ -501,6 +501,8 @@ const char *intel_no_fbc_reason_str(enum no_fbc_reason reason) return "rotation unsupported"; case FBC_IN_DBG_MASTER: return "Kernel debugger is active"; + case FBC_BAD_STRIDE: + return "framebuffer stride not supported"; default: MISSING_CASE(reason); return "unknown reason"; @@ -804,6 +806,14 @@ static void __intel_fbc_update(struct drm_i915_private *dev_priv) goto out_disable; } + /* TODO: we need to figure out what are the stride restrictions for the + * older platforms. */ + if (fb->pitches[0] < 512 || fb->pitches[0] > 16384 || + (fb->pitches[0] & (64 - 1)) != 0) { + set_no_fbc_reason(dev_priv, FBC_BAD_STRIDE); + goto out_disable; + } + /* If the kernel debugger is active, always disable compression */ if (in_dbg_master()) { set_no_fbc_reason(dev_priv, FBC_IN_DBG_MASTER);