* [PATCH] drm/i915/skl: Assume no scaling is available when things are not as expected @ 2015-06-01 12:04 Tvrtko Ursulin 2015-06-01 21:45 ` shuang.he 2015-06-15 10:46 ` Daniel Vetter 0 siblings, 2 replies; 7+ messages in thread From: Tvrtko Ursulin @ 2015-06-01 12:04 UTC (permalink / raw) To: Intel-gfx From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Cdclk < crtc_clock is not allowed and suggests a different problem elsewhere. It is more robust and safe to assume no scaling is possible in this case. Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> --- drivers/gpu/drm/i915/intel_display.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 93a5e51..4c99373 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -13234,7 +13234,7 @@ skl_max_scale(struct intel_crtc *intel_crtc, struct intel_crtc_state *crtc_state crtc_clock = crtc_state->base.adjusted_mode.crtc_clock; cdclk = dev_priv->display.get_display_clock_speed(dev); - if (!crtc_clock || !cdclk) + if (!crtc_clock || !cdclk || (cdclk < crtc_clock)) return DRM_PLANE_HELPER_NO_SCALING; /* -- 2.4.0 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/i915/skl: Assume no scaling is available when things are not as expected 2015-06-01 12:04 [PATCH] drm/i915/skl: Assume no scaling is available when things are not as expected Tvrtko Ursulin @ 2015-06-01 21:45 ` shuang.he 2015-06-15 10:46 ` Daniel Vetter 1 sibling, 0 replies; 7+ messages in thread From: shuang.he @ 2015-06-01 21:45 UTC (permalink / raw) To: shuang.he, lei.a.liu, intel-gfx, tvrtko.ursulin Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com) Task id: 6513 -------------------------------------Summary------------------------------------- Platform Delta drm-intel-nightly Series Applied PNV 276/276 276/276 ILK 303/303 303/303 SNB -1 315/315 314/315 IVB 343/343 343/343 BYT 287/287 287/287 BDW 321/321 321/321 -------------------------------------Detailed------------------------------------- Platform Test drm-intel-nightly Series Applied *SNB igt@pm_rpm@dpms-mode-unset-non-lpsp PASS(1) DMESG_WARN(1) (dmesg patch applied)WARNING:at_drivers/gpu/drm/i915/intel_uncore.c:#assert_device_not_suspended[i915]()@WARNING:.* at .* assert_device_not_suspended+0x Note: You need to pay more attention to line start with '*' _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/i915/skl: Assume no scaling is available when things are not as expected 2015-06-01 12:04 [PATCH] drm/i915/skl: Assume no scaling is available when things are not as expected Tvrtko Ursulin 2015-06-01 21:45 ` shuang.he @ 2015-06-15 10:46 ` Daniel Vetter 2015-06-15 21:03 ` Konduru, Chandra 1 sibling, 1 reply; 7+ messages in thread From: Daniel Vetter @ 2015-06-15 10:46 UTC (permalink / raw) To: Tvrtko Ursulin; +Cc: Intel-gfx On Mon, Jun 01, 2015 at 01:04:37PM +0100, Tvrtko Ursulin wrote: > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > Cdclk < crtc_clock is not allowed and suggests a different problem elsewhere. > > It is more robust and safe to assume no scaling is possible in this case. > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 93a5e51..4c99373 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -13234,7 +13234,7 @@ skl_max_scale(struct intel_crtc *intel_crtc, struct intel_crtc_state *crtc_state > crtc_clock = crtc_state->base.adjusted_mode.crtc_clock; > cdclk = dev_priv->display.get_display_clock_speed(dev); Probably fallout from the in-flight dynamic cdclk stuff - this code checks the wrong bits I guess. Chandra? Thanks, Daniel > > - if (!crtc_clock || !cdclk) > + if (!crtc_clock || !cdclk || (cdclk < crtc_clock)) > return DRM_PLANE_HELPER_NO_SCALING; > > /* > -- > 2.4.0 > > _______________________________________________ > 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/i915/skl: Assume no scaling is available when things are not as expected 2015-06-15 10:46 ` Daniel Vetter @ 2015-06-15 21:03 ` Konduru, Chandra 2015-06-16 13:40 ` Daniel Vetter 0 siblings, 1 reply; 7+ messages in thread From: Konduru, Chandra @ 2015-06-15 21:03 UTC (permalink / raw) To: Daniel Vetter, Tvrtko Ursulin; +Cc: Nikula, Jani, Intel-gfx, Lankhorst, Maarten > > > > Cdclk < crtc_clock is not allowed and suggests a different problem elsewhere. > > > > It is more robust and safe to assume no scaling is possible in this case. > > > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/i915/intel_display.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > > index 93a5e51..4c99373 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -13234,7 +13234,7 @@ skl_max_scale(struct intel_crtc *intel_crtc, struct > intel_crtc_state *crtc_state > > crtc_clock = crtc_state->base.adjusted_mode.crtc_clock; > > cdclk = dev_priv->display.get_display_clock_speed(dev); > > Probably fallout from the in-flight dynamic cdclk stuff - this code checks > the wrong bits I guess. Chandra? Looks like something elsewhere has fallen out and issue manifested here. Damien reported another issue where get_display_clock_speed causing an assert because it is called when dev_priv->pm.suspend is true during runtime resume. But later was resolved after one of atomic patch is reverted. While Maarten is addressing recently reported atomic issues, for time being some atomic crtc patches were reverted. I am not 100% sure whether issue here is due to same root cause or due to something different. > > Thanks, Daniel > > > > > - if (!crtc_clock || !cdclk) > > + if (!crtc_clock || !cdclk || (cdclk < crtc_clock)) > > return DRM_PLANE_HELPER_NO_SCALING; > > > > /* > > -- > > 2.4.0 > > > > _______________________________________________ > > 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/i915/skl: Assume no scaling is available when things are not as expected 2015-06-15 21:03 ` Konduru, Chandra @ 2015-06-16 13:40 ` Daniel Vetter 2015-06-16 13:46 ` Ville Syrjälä 0 siblings, 1 reply; 7+ messages in thread From: Daniel Vetter @ 2015-06-16 13:40 UTC (permalink / raw) To: Konduru, Chandra; +Cc: Nikula, Jani, Intel-gfx, Lankhorst, Maarten On Mon, Jun 15, 2015 at 09:03:09PM +0000, Konduru, Chandra wrote: > > > > > > Cdclk < crtc_clock is not allowed and suggests a different problem elsewhere. > > > > > > It is more robust and safe to assume no scaling is possible in this case. > > > > > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_display.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > b/drivers/gpu/drm/i915/intel_display.c > > > index 93a5e51..4c99373 100644 > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > @@ -13234,7 +13234,7 @@ skl_max_scale(struct intel_crtc *intel_crtc, struct > > intel_crtc_state *crtc_state > > > crtc_clock = crtc_state->base.adjusted_mode.crtc_clock; > > > cdclk = dev_priv->display.get_display_clock_speed(dev); > > > > Probably fallout from the in-flight dynamic cdclk stuff - this code checks > > the wrong bits I guess. Chandra? > > Looks like something elsewhere has fallen out and issue manifested here. > > Damien reported another issue where get_display_clock_speed causing > an assert because it is called when dev_priv->pm.suspend is true during > runtime resume. But later was resolved after one of atomic patch is > reverted. > > While Maarten is addressing recently reported atomic issues, for > time being some atomic crtc patches were reverted. > > I am not 100% sure whether issue here is due to same root cause or > due to something different. You need to check the cached (and soon the one in the global atomic modeset state structure) cdclk value, not the current one in the hw. And yeah that can result in asserts since the hw might not be one yet when this code is run. I.e. this isn't about atomic modeset but just about interaction with the recent cdclk work. And with the existing rpm feature. In the future we should even upclock the cdclck stuff (once dynamic cdclk is implemented on skl) to make sure it fits the desired scaler configuration. But that's follow-up work. -Daniel > > > > > Thanks, Daniel > > > > > > > > - if (!crtc_clock || !cdclk) > > > + if (!crtc_clock || !cdclk || (cdclk < crtc_clock)) > > > return DRM_PLANE_HELPER_NO_SCALING; > > > > > > /* > > > -- > > > 2.4.0 > > > > > > _______________________________________________ > > > 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 -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/i915/skl: Assume no scaling is available when things are not as expected 2015-06-16 13:40 ` Daniel Vetter @ 2015-06-16 13:46 ` Ville Syrjälä 2015-06-17 11:41 ` Daniel Vetter 0 siblings, 1 reply; 7+ messages in thread From: Ville Syrjälä @ 2015-06-16 13:46 UTC (permalink / raw) To: Daniel Vetter; +Cc: Nikula, Jani, Intel-gfx, Lankhorst, Maarten On Tue, Jun 16, 2015 at 03:40:16PM +0200, Daniel Vetter wrote: > On Mon, Jun 15, 2015 at 09:03:09PM +0000, Konduru, Chandra wrote: > > > > > > > > Cdclk < crtc_clock is not allowed and suggests a different problem elsewhere. > > > > > > > > It is more robust and safe to assume no scaling is possible in this case. > > > > > > > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > --- > > > > drivers/gpu/drm/i915/intel_display.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > > b/drivers/gpu/drm/i915/intel_display.c > > > > index 93a5e51..4c99373 100644 > > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > > @@ -13234,7 +13234,7 @@ skl_max_scale(struct intel_crtc *intel_crtc, struct > > > intel_crtc_state *crtc_state > > > > crtc_clock = crtc_state->base.adjusted_mode.crtc_clock; > > > > cdclk = dev_priv->display.get_display_clock_speed(dev); > > > > > > Probably fallout from the in-flight dynamic cdclk stuff - this code checks > > > the wrong bits I guess. Chandra? > > > > Looks like something elsewhere has fallen out and issue manifested here. > > > > Damien reported another issue where get_display_clock_speed causing > > an assert because it is called when dev_priv->pm.suspend is true during > > runtime resume. But later was resolved after one of atomic patch is > > reverted. > > > > While Maarten is addressing recently reported atomic issues, for > > time being some atomic crtc patches were reverted. > > > > I am not 100% sure whether issue here is due to same root cause or > > due to something different. > > You need to check the cached (and soon the one in the global atomic > modeset state structure) cdclk value, not the current one in the hw. And > yeah that can result in asserts since the hw might not be one yet when > this code is run. I.e. this isn't about atomic modeset but just about > interaction with the recent cdclk work. And with the existing rpm feature. > > In the future we should even upclock the cdclck stuff (once dynamic cdclk > is implemented on skl) to make sure it fits the desired scaler > configuration. But that's follow-up work. For something like that we probably need some kind of new property to request extra cdclk headroom when doing a modeset. Otherwise we're going to end up blinking the displays all the time. -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/i915/skl: Assume no scaling is available when things are not as expected 2015-06-16 13:46 ` Ville Syrjälä @ 2015-06-17 11:41 ` Daniel Vetter 0 siblings, 0 replies; 7+ messages in thread From: Daniel Vetter @ 2015-06-17 11:41 UTC (permalink / raw) To: Ville Syrjälä; +Cc: Nikula, Jani, Intel-gfx, Lankhorst, Maarten On Tue, Jun 16, 2015 at 04:46:40PM +0300, Ville Syrjälä wrote: > On Tue, Jun 16, 2015 at 03:40:16PM +0200, Daniel Vetter wrote: > > On Mon, Jun 15, 2015 at 09:03:09PM +0000, Konduru, Chandra wrote: > > > > > > > > > > Cdclk < crtc_clock is not allowed and suggests a different problem elsewhere. > > > > > > > > > > It is more robust and safe to assume no scaling is possible in this case. > > > > > > > > > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > --- > > > > > drivers/gpu/drm/i915/intel_display.c | 2 +- > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > > > b/drivers/gpu/drm/i915/intel_display.c > > > > > index 93a5e51..4c99373 100644 > > > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > > > @@ -13234,7 +13234,7 @@ skl_max_scale(struct intel_crtc *intel_crtc, struct > > > > intel_crtc_state *crtc_state > > > > > crtc_clock = crtc_state->base.adjusted_mode.crtc_clock; > > > > > cdclk = dev_priv->display.get_display_clock_speed(dev); > > > > > > > > Probably fallout from the in-flight dynamic cdclk stuff - this code checks > > > > the wrong bits I guess. Chandra? > > > > > > Looks like something elsewhere has fallen out and issue manifested here. > > > > > > Damien reported another issue where get_display_clock_speed causing > > > an assert because it is called when dev_priv->pm.suspend is true during > > > runtime resume. But later was resolved after one of atomic patch is > > > reverted. > > > > > > While Maarten is addressing recently reported atomic issues, for > > > time being some atomic crtc patches were reverted. > > > > > > I am not 100% sure whether issue here is due to same root cause or > > > due to something different. > > > > You need to check the cached (and soon the one in the global atomic > > modeset state structure) cdclk value, not the current one in the hw. And > > yeah that can result in asserts since the hw might not be one yet when > > this code is run. I.e. this isn't about atomic modeset but just about > > interaction with the recent cdclk work. And with the existing rpm feature. > > > > In the future we should even upclock the cdclck stuff (once dynamic cdclk > > is implemented on skl) to make sure it fits the desired scaler > > configuration. But that's follow-up work. > > For something like that we probably need some kind of new property to > request extra cdclk headroom when doing a modeset. Otherwise we're going > to end up blinking the displays all the time. Userspace can avoid the blinking by not setting the ALLOW_MODESET flag. Then we'll reject the atomic update if it would require a cdclk change. Same on the downclocking, we'd need to not force a modeset if userspace doesn't one one. We might still need some explicit headroom perhaps on top of this. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2015-06-17 11:38 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-06-01 12:04 [PATCH] drm/i915/skl: Assume no scaling is available when things are not as expected Tvrtko Ursulin 2015-06-01 21:45 ` shuang.he 2015-06-15 10:46 ` Daniel Vetter 2015-06-15 21:03 ` Konduru, Chandra 2015-06-16 13:40 ` Daniel Vetter 2015-06-16 13:46 ` Ville Syrjälä 2015-06-17 11:41 ` Daniel Vetter
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.