Re: [Intel-gfx] [PATCH v2 14/14] drm/i915/skl: Do not allow scaling when crtc is disabled.
On Tue, Nov 17, 2015 at 02:59:35PM +0100, Maarten Lankhorst wrote: > Op 03-11-15 om 14:11 schreef Ville Syrjälä: > > On Tue, Nov 03, 2015 at 01:00:32PM +0100, Maarten Lankhorst wrote: > >> Op 03-11-15 om 11:40 schreef Ville Syrjälä: > >>> On Tue, Nov 03, 2015 at 11:06:53AM +0100, Maarten Lankhorst wrote: > Op 03-11-15 om 10:09 schreef Ville Syrjälä: > > On Tue, Nov 03, 2015 at 08:31:53AM +0100, Maarten Lankhorst wrote: > >> This fixes a warning when the crtc is turned off. In that case fb > >> will be NULL, and crtc_clock will be 0. Because the crtc is no longer > >> active this is not a bug, and shouldn't trigger the WARN_ON. > > Mm. We want to do scaling checks and whatnot during DPMS. So this should > > check .enabled, no? > Not sure what the right decision would be here.. > > * skl max scale is lower of: > *close to 3 but not 3, -1 is for that purpose > *or > *cdclk/crtc_clock > > So when multiple pipes are enabled potentially 3x scaling is allowed, > but if you dpms them all off > cdclk might get set to 0. This means a previous valid amount of scaling > might suddenly become invalid. > > Maybe the fix is keeping 2 cdclk's, one for scaling and one for setting. > >>> I think we should keep around the min required cdclk in the crtc state. > >>> And we recalculate that one every time anything changes. Then we can > >>> just compare it against the current cdclk after plane/crtc states have > >>> otherwise been computed to see if we need to change the current cdclk. > >>> > >> What would the use be here? In that case the above formula reduces to > >> 1<<16. > >> If you want to treat dpms off the same as dpms on then you need the > >> separate cdclk's.. > > I don't understand what you're saying. We need to calculate the required > > cdclk based on dpms on. And that is what we must check against the > > hardware limit. If we then want to optimize the dpms off case we can > > just do 'cdclk = state->active ? state->cdclk : 0'. We probably want to > > do that when all pipes are in dpms off. But if we have some pipes in > > dpms off and some in dpms on, we may want to skip the 'active' check > > for all pipes (ie. behave as all pipes are dpms on), just to avoid flicker > > when some of the remaining pipes transition from dpms off to dpms on. > Yeah but this would break things, or we'd need to keep 2 cdclk freqs globally. > One with the real frequency, the other one used for calculations in atomic. > And only when all crtc's are disabled they would differ. > > In any case, when a plane is DPMS off this code won't be useful, visible = > false > because the rectangle will get clipped to (0,0)x(0,0). And that must be changed. > > Saying no scaling is possible would be fine here I think, without side > effects. -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 14/14] drm/i915/skl: Do not allow scaling when crtc is disabled.
Op 03-11-15 om 14:11 schreef Ville Syrjälä: > On Tue, Nov 03, 2015 at 01:00:32PM +0100, Maarten Lankhorst wrote: >> Op 03-11-15 om 11:40 schreef Ville Syrjälä: >>> On Tue, Nov 03, 2015 at 11:06:53AM +0100, Maarten Lankhorst wrote: Op 03-11-15 om 10:09 schreef Ville Syrjälä: > On Tue, Nov 03, 2015 at 08:31:53AM +0100, Maarten Lankhorst wrote: >> This fixes a warning when the crtc is turned off. In that case fb >> will be NULL, and crtc_clock will be 0. Because the crtc is no longer >> active this is not a bug, and shouldn't trigger the WARN_ON. > Mm. We want to do scaling checks and whatnot during DPMS. So this should > check .enabled, no? Not sure what the right decision would be here.. * skl max scale is lower of: *close to 3 but not 3, -1 is for that purpose *or *cdclk/crtc_clock So when multiple pipes are enabled potentially 3x scaling is allowed, but if you dpms them all off cdclk might get set to 0. This means a previous valid amount of scaling might suddenly become invalid. Maybe the fix is keeping 2 cdclk's, one for scaling and one for setting. >>> I think we should keep around the min required cdclk in the crtc state. >>> And we recalculate that one every time anything changes. Then we can >>> just compare it against the current cdclk after plane/crtc states have >>> otherwise been computed to see if we need to change the current cdclk. >>> >> What would the use be here? In that case the above formula reduces to 1<<16. >> If you want to treat dpms off the same as dpms on then you need the separate >> cdclk's.. > I don't understand what you're saying. We need to calculate the required > cdclk based on dpms on. And that is what we must check against the > hardware limit. If we then want to optimize the dpms off case we can > just do 'cdclk = state->active ? state->cdclk : 0'. We probably want to > do that when all pipes are in dpms off. But if we have some pipes in > dpms off and some in dpms on, we may want to skip the 'active' check > for all pipes (ie. behave as all pipes are dpms on), just to avoid flicker > when some of the remaining pipes transition from dpms off to dpms on. Yeah but this would break things, or we'd need to keep 2 cdclk freqs globally. One with the real frequency, the other one used for calculations in atomic. And only when all crtc's are disabled they would differ. In any case, when a plane is DPMS off this code won't be useful, visible = false because the rectangle will get clipped to (0,0)x(0,0). Saying no scaling is possible would be fine here I think, without side effects. ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 14/14] drm/i915/skl: Do not allow scaling when crtc is disabled.
On Tue, Nov 03, 2015 at 01:00:32PM +0100, Maarten Lankhorst wrote: > Op 03-11-15 om 11:40 schreef Ville Syrjälä: > > On Tue, Nov 03, 2015 at 11:06:53AM +0100, Maarten Lankhorst wrote: > >> Op 03-11-15 om 10:09 schreef Ville Syrjälä: > >>> On Tue, Nov 03, 2015 at 08:31:53AM +0100, Maarten Lankhorst wrote: > This fixes a warning when the crtc is turned off. In that case fb > will be NULL, and crtc_clock will be 0. Because the crtc is no longer > active this is not a bug, and shouldn't trigger the WARN_ON. > >>> Mm. We want to do scaling checks and whatnot during DPMS. So this should > >>> check .enabled, no? > >> Not sure what the right decision would be here.. > >> > >> * skl max scale is lower of: > >> *close to 3 but not 3, -1 is for that purpose > >> *or > >> *cdclk/crtc_clock > >> > >> So when multiple pipes are enabled potentially 3x scaling is allowed, but > >> if you dpms them all off > >> cdclk might get set to 0. This means a previous valid amount of scaling > >> might suddenly become invalid. > >> > >> Maybe the fix is keeping 2 cdclk's, one for scaling and one for setting. > > I think we should keep around the min required cdclk in the crtc state. > > And we recalculate that one every time anything changes. Then we can > > just compare it against the current cdclk after plane/crtc states have > > otherwise been computed to see if we need to change the current cdclk. > > > What would the use be here? In that case the above formula reduces to 1<<16. > If you want to treat dpms off the same as dpms on then you need the separate > cdclk's.. I don't understand what you're saying. We need to calculate the required cdclk based on dpms on. And that is what we must check against the hardware limit. If we then want to optimize the dpms off case we can just do 'cdclk = state->active ? state->cdclk : 0'. We probably want to do that when all pipes are in dpms off. But if we have some pipes in dpms off and some in dpms on, we may want to skip the 'active' check for all pipes (ie. behave as all pipes are dpms on), just to avoid flicker when some of the remaining pipes transition from dpms off to dpms on. -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 14/14] drm/i915/skl: Do not allow scaling when crtc is disabled.
Op 03-11-15 om 11:40 schreef Ville Syrjälä: > On Tue, Nov 03, 2015 at 11:06:53AM +0100, Maarten Lankhorst wrote: >> Op 03-11-15 om 10:09 schreef Ville Syrjälä: >>> On Tue, Nov 03, 2015 at 08:31:53AM +0100, Maarten Lankhorst wrote: This fixes a warning when the crtc is turned off. In that case fb will be NULL, and crtc_clock will be 0. Because the crtc is no longer active this is not a bug, and shouldn't trigger the WARN_ON. >>> Mm. We want to do scaling checks and whatnot during DPMS. So this should >>> check .enabled, no? >> Not sure what the right decision would be here.. >> >> * skl max scale is lower of: >> *close to 3 but not 3, -1 is for that purpose >> *or >> *cdclk/crtc_clock >> >> So when multiple pipes are enabled potentially 3x scaling is allowed, but if >> you dpms them all off >> cdclk might get set to 0. This means a previous valid amount of scaling >> might suddenly become invalid. >> >> Maybe the fix is keeping 2 cdclk's, one for scaling and one for setting. > I think we should keep around the min required cdclk in the crtc state. > And we recalculate that one every time anything changes. Then we can > just compare it against the current cdclk after plane/crtc states have > otherwise been computed to see if we need to change the current cdclk. > What would the use be here? In that case the above formula reduces to 1<<16. If you want to treat dpms off the same as dpms on then you need the separate cdclk's.. ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 14/14] drm/i915/skl: Do not allow scaling when crtc is disabled.
On Tue, Nov 03, 2015 at 11:06:53AM +0100, Maarten Lankhorst wrote: > Op 03-11-15 om 10:09 schreef Ville Syrjälä: > > On Tue, Nov 03, 2015 at 08:31:53AM +0100, Maarten Lankhorst wrote: > >> This fixes a warning when the crtc is turned off. In that case fb > >> will be NULL, and crtc_clock will be 0. Because the crtc is no longer > >> active this is not a bug, and shouldn't trigger the WARN_ON. > > Mm. We want to do scaling checks and whatnot during DPMS. So this should > > check .enabled, no? > Not sure what the right decision would be here.. > > * skl max scale is lower of: > *close to 3 but not 3, -1 is for that purpose > *or > *cdclk/crtc_clock > > So when multiple pipes are enabled potentially 3x scaling is allowed, but if > you dpms them all off > cdclk might get set to 0. This means a previous valid amount of scaling might > suddenly become invalid. > > Maybe the fix is keeping 2 cdclk's, one for scaling and one for setting. I think we should keep around the min required cdclk in the crtc state. And we recalculate that one every time anything changes. Then we can just compare it against the current cdclk after plane/crtc states have otherwise been computed to see if we need to change the current cdclk. > > This probably needs to be addressed in patch 3/14 then, so that one needs > more love. > > I will resend patch 3, 4, 5 and 14 as a separate series. The other patches > from this series will still work with some easily fixed rejects. > >> Also remove handling a null crtc_state, with all transitional helpers > >> gone this can no longer happen. > > What about the !intel_crtc check, how is that supposed to happen? > When fb == NULL. :) > >> Signed-off-by: Maarten Lankhorst > >> --- > >> 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 304e1028c9a4..7e2caeef9a11 100644 > >> --- a/drivers/gpu/drm/i915/intel_display.c > >> +++ b/drivers/gpu/drm/i915/intel_display.c > >> @@ -13671,7 +13671,7 @@ skl_max_scale(struct intel_crtc *intel_crtc, > >> struct intel_crtc_state *crtc_state > >>struct drm_i915_private *dev_priv; > >>int crtc_clock, cdclk; > >> > >> - if (!intel_crtc || !crtc_state) > >> + if (!intel_crtc || !crtc_state->base.active) > >>return DRM_PLANE_HELPER_NO_SCALING; > >> > >>dev = intel_crtc->base.dev; > >> -- > >> 2.1.0 > >> > >> ___ > >> Intel-gfx mailing list > >> Intel-gfx@lists.freedesktop.org > >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 14/14] drm/i915/skl: Do not allow scaling when crtc is disabled.
Op 03-11-15 om 10:09 schreef Ville Syrjälä: > On Tue, Nov 03, 2015 at 08:31:53AM +0100, Maarten Lankhorst wrote: >> This fixes a warning when the crtc is turned off. In that case fb >> will be NULL, and crtc_clock will be 0. Because the crtc is no longer >> active this is not a bug, and shouldn't trigger the WARN_ON. > Mm. We want to do scaling checks and whatnot during DPMS. So this should > check .enabled, no? Not sure what the right decision would be here.. * skl max scale is lower of: *close to 3 but not 3, -1 is for that purpose *or *cdclk/crtc_clock So when multiple pipes are enabled potentially 3x scaling is allowed, but if you dpms them all off cdclk might get set to 0. This means a previous valid amount of scaling might suddenly become invalid. Maybe the fix is keeping 2 cdclk's, one for scaling and one for setting. This probably needs to be addressed in patch 3/14 then, so that one needs more love. I will resend patch 3, 4, 5 and 14 as a separate series. The other patches from this series will still work with some easily fixed rejects. >> Also remove handling a null crtc_state, with all transitional helpers >> gone this can no longer happen. > What about the !intel_crtc check, how is that supposed to happen? When fb == NULL. :) >> Signed-off-by: Maarten Lankhorst >> --- >> 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 304e1028c9a4..7e2caeef9a11 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -13671,7 +13671,7 @@ skl_max_scale(struct intel_crtc *intel_crtc, struct >> intel_crtc_state *crtc_state >> struct drm_i915_private *dev_priv; >> int crtc_clock, cdclk; >> >> -if (!intel_crtc || !crtc_state) >> +if (!intel_crtc || !crtc_state->base.active) >> return DRM_PLANE_HELPER_NO_SCALING; >> >> dev = intel_crtc->base.dev; >> -- >> 2.1.0 >> >> ___ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 14/14] drm/i915/skl: Do not allow scaling when crtc is disabled.
On Tue, Nov 03, 2015 at 08:31:53AM +0100, Maarten Lankhorst wrote: > This fixes a warning when the crtc is turned off. In that case fb > will be NULL, and crtc_clock will be 0. Because the crtc is no longer > active this is not a bug, and shouldn't trigger the WARN_ON. Mm. We want to do scaling checks and whatnot during DPMS. So this should check .enabled, no? > Also remove handling a null crtc_state, with all transitional helpers > gone this can no longer happen. What about the !intel_crtc check, how is that supposed to happen? > > Signed-off-by: Maarten Lankhorst > --- > 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 304e1028c9a4..7e2caeef9a11 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -13671,7 +13671,7 @@ skl_max_scale(struct intel_crtc *intel_crtc, struct > intel_crtc_state *crtc_state > struct drm_i915_private *dev_priv; > int crtc_clock, cdclk; > > - if (!intel_crtc || !crtc_state) > + if (!intel_crtc || !crtc_state->base.active) > return DRM_PLANE_HELPER_NO_SCALING; > > dev = intel_crtc->base.dev; > -- > 2.1.0 > > ___ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v2 14/14] drm/i915/skl: Do not allow scaling when crtc is disabled.
This fixes a warning when the crtc is turned off. In that case fb will be NULL, and crtc_clock will be 0. Because the crtc is no longer active this is not a bug, and shouldn't trigger the WARN_ON. Also remove handling a null crtc_state, with all transitional helpers gone this can no longer happen. Signed-off-by: Maarten Lankhorst --- 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 304e1028c9a4..7e2caeef9a11 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -13671,7 +13671,7 @@ skl_max_scale(struct intel_crtc *intel_crtc, struct intel_crtc_state *crtc_state struct drm_i915_private *dev_priv; int crtc_clock, cdclk; - if (!intel_crtc || !crtc_state) + if (!intel_crtc || !crtc_state->base.active) return DRM_PLANE_HELPER_NO_SCALING; dev = intel_crtc->base.dev; -- 2.1.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
