* [PATCH 1/3] drm/i915: Remove 10% cdclk guardband from BXT @ 2015-10-27 13:23 ville.syrjala 2015-10-27 13:23 ` [PATCH 2/3] drm/i915: Untangle .fdi_link_train() and cdclk function pointer assignments ville.syrjala 2015-10-27 13:23 ` [PATCH 3/3] drm/i915: Share cdclk code for BDW and BXT ville.syrjala 0 siblings, 2 replies; 7+ messages in thread From: ville.syrjala @ 2015-10-27 13:23 UTC (permalink / raw) To: intel-gfx From: Ville Syrjälä <ville.syrjala@linux.intel.com> BXT doesn't need any cdclk guardband, so don't use any. Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> --- drivers/gpu/drm/i915/intel_display.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index bc1907e..b8095be 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -5937,16 +5937,15 @@ static int broxton_calc_cdclk(struct drm_i915_private *dev_priv, { /* * FIXME: - * - remove the guardband, it's not needed on BXT * - set 19.2MHz bypass frequency if there are no active pipes */ - if (max_pixclk > 576000*9/10) + if (max_pixclk > 576000) return 624000; - else if (max_pixclk > 384000*9/10) + else if (max_pixclk > 384000) return 576000; - else if (max_pixclk > 288000*9/10) + else if (max_pixclk > 288000) return 384000; - else if (max_pixclk > 144000*9/10) + else if (max_pixclk > 144000) return 288000; else return 144000; -- 2.4.10 _______________________________________________ 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
* [PATCH 2/3] drm/i915: Untangle .fdi_link_train() and cdclk function pointer assignments 2015-10-27 13:23 [PATCH 1/3] drm/i915: Remove 10% cdclk guardband from BXT ville.syrjala @ 2015-10-27 13:23 ` ville.syrjala 2015-10-27 13:23 ` [PATCH 3/3] drm/i915: Share cdclk code for BDW and BXT ville.syrjala 1 sibling, 0 replies; 7+ messages in thread From: ville.syrjala @ 2015-10-27 13:23 UTC (permalink / raw) To: intel-gfx From: Ville Syrjälä <ville.syrjala@linux.intel.com> Clarify the code by doing the .fdi_link_train() and cdclk function pointer assignments using two separate if-ladders. Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> --- drivers/gpu/drm/i915/intel_display.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index b8095be..978f543 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -14617,12 +14617,13 @@ static void intel_init_display(struct drm_device *dev) dev_priv->display.fdi_link_train = ivb_manual_fdi_link_train; } else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) { dev_priv->display.fdi_link_train = hsw_fdi_link_train; - if (IS_BROADWELL(dev)) { - dev_priv->display.modeset_commit_cdclk = - broadwell_modeset_commit_cdclk; - dev_priv->display.modeset_calc_cdclk = - broadwell_modeset_calc_cdclk; - } + } + + if (IS_BROADWELL(dev)) { + dev_priv->display.modeset_commit_cdclk = + broadwell_modeset_commit_cdclk; + dev_priv->display.modeset_calc_cdclk = + broadwell_modeset_calc_cdclk; } else if (IS_VALLEYVIEW(dev)) { dev_priv->display.modeset_commit_cdclk = valleyview_modeset_commit_cdclk; -- 2.4.10 _______________________________________________ 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
* [PATCH 3/3] drm/i915: Share cdclk code for BDW and BXT 2015-10-27 13:23 [PATCH 1/3] drm/i915: Remove 10% cdclk guardband from BXT ville.syrjala 2015-10-27 13:23 ` [PATCH 2/3] drm/i915: Untangle .fdi_link_train() and cdclk function pointer assignments ville.syrjala @ 2015-10-27 13:23 ` ville.syrjala 2015-10-27 13:43 ` Jani Nikula 1 sibling, 1 reply; 7+ messages in thread From: ville.syrjala @ 2015-10-27 13:23 UTC (permalink / raw) To: intel-gfx From: Ville Syrjälä <ville.syrjala@linux.intel.com> The difference betwen the BXT and BDW cdclk code boils down to two things: claming the cdclk to the maximum supported, and panel fitter downscaling ratio Unifying the the max cdclk clamping is easy, just do it. And as far as the panel fitter is concerned, SKL+ already (ab)use the pch pfit state for its pipe scaler information, so it will compute the adjusted pixel rate correctly. So we can just use the BDW code for BXT, as long as we lift the BXT pixel rate -> cdclk selection into the correct place. Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> --- drivers/gpu/drm/i915/intel_display.c | 87 +++++++++++++----------------------- 1 file changed, 30 insertions(+), 57 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 978f543..0c782c7 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -5932,25 +5932,6 @@ static int valleyview_calc_cdclk(struct drm_i915_private *dev_priv, return 200000; } -static int broxton_calc_cdclk(struct drm_i915_private *dev_priv, - int max_pixclk) -{ - /* - * FIXME: - * - set 19.2MHz bypass frequency if there are no active pipes - */ - if (max_pixclk > 576000) - return 624000; - else if (max_pixclk > 384000) - return 576000; - else if (max_pixclk > 288000) - return 384000; - else if (max_pixclk > 144000) - return 288000; - else - return 144000; -} - /* Compute the max pixel clock for new configuration. Uses atomic state if * that's non-NULL, look at current state otherwise. */ static int intel_mode_max_pixclk(struct drm_device *dev, @@ -5990,21 +5971,6 @@ static int valleyview_modeset_calc_cdclk(struct drm_atomic_state *state) return 0; } -static int broxton_modeset_calc_cdclk(struct drm_atomic_state *state) -{ - struct drm_device *dev = state->dev; - struct drm_i915_private *dev_priv = dev->dev_private; - int max_pixclk = intel_mode_max_pixclk(dev, state); - - if (max_pixclk < 0) - return max_pixclk; - - to_intel_atomic_state(state)->cdclk = - broxton_calc_cdclk(dev_priv, max_pixclk); - - return 0; -} - static void vlv_program_pfi_credits(struct drm_i915_private *dev_priv) { unsigned int credits, default_credits; @@ -9497,14 +9463,6 @@ void hsw_disable_pc8(struct drm_i915_private *dev_priv) intel_prepare_ddi(dev); } -static void broxton_modeset_commit_cdclk(struct drm_atomic_state *old_state) -{ - struct drm_device *dev = old_state->dev; - unsigned int req_cdclk = to_intel_atomic_state(old_state)->cdclk; - - broxton_set_cdclk(dev, req_cdclk); -} - /* compute the max rate for new configuration */ static int ilk_max_pixel_rate(struct drm_atomic_state *state) { @@ -9621,14 +9579,31 @@ static int broadwell_modeset_calc_cdclk(struct drm_atomic_state *state) * FIXME should also account for plane ratio * once 64bpp pixel formats are supported. */ - if (max_pixclk > 540000) - cdclk = 675000; - else if (max_pixclk > 450000) - cdclk = 540000; - else if (max_pixclk > 337500) - cdclk = 450000; - else - cdclk = 337500; + if (IS_BROXTON(dev_priv)) { + /* + * FIXME: + * - set 19.2MHz bypass frequency if there are no active pipes + */ + if (max_pixclk > 576000) + cdclk = 624000; + else if (max_pixclk > 384000) + cdclk = 576000; + else if (max_pixclk > 288000) + cdclk = 384000; + else if (max_pixclk > 144000) + cdclk = 288000; + else + cdclk = 144000; + } else { + if (max_pixclk > 540000) + cdclk = 675000; + else if (max_pixclk > 450000) + cdclk = 540000; + else if (max_pixclk > 337500) + cdclk = 450000; + else + cdclk = 337500; + } /* * FIXME move the cdclk caclulation to @@ -9650,7 +9625,10 @@ static void broadwell_modeset_commit_cdclk(struct drm_atomic_state *old_state) struct drm_device *dev = old_state->dev; unsigned int req_cdclk = to_intel_atomic_state(old_state)->cdclk; - broadwell_set_cdclk(dev, req_cdclk); + if (IS_BROXTON(dev)) + broxton_set_cdclk(dev, req_cdclk); + else + broadwell_set_cdclk(dev, req_cdclk); } static int haswell_crtc_compute_clock(struct intel_crtc *crtc, @@ -14619,7 +14597,7 @@ static void intel_init_display(struct drm_device *dev) dev_priv->display.fdi_link_train = hsw_fdi_link_train; } - if (IS_BROADWELL(dev)) { + if (IS_BROADWELL(dev) || IS_BROXTON(dev)) { dev_priv->display.modeset_commit_cdclk = broadwell_modeset_commit_cdclk; dev_priv->display.modeset_calc_cdclk = @@ -14629,11 +14607,6 @@ static void intel_init_display(struct drm_device *dev) valleyview_modeset_commit_cdclk; dev_priv->display.modeset_calc_cdclk = valleyview_modeset_calc_cdclk; - } else if (IS_BROXTON(dev)) { - dev_priv->display.modeset_commit_cdclk = - broxton_modeset_commit_cdclk; - dev_priv->display.modeset_calc_cdclk = - broxton_modeset_calc_cdclk; } switch (INTEL_INFO(dev)->gen) { -- 2.4.10 _______________________________________________ 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 3/3] drm/i915: Share cdclk code for BDW and BXT 2015-10-27 13:23 ` [PATCH 3/3] drm/i915: Share cdclk code for BDW and BXT ville.syrjala @ 2015-10-27 13:43 ` Jani Nikula 2015-10-27 14:31 ` Ville Syrjälä 0 siblings, 1 reply; 7+ messages in thread From: Jani Nikula @ 2015-10-27 13:43 UTC (permalink / raw) To: ville.syrjala, intel-gfx On Tue, 27 Oct 2015, ville.syrjala@linux.intel.com wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > The difference betwen the BXT and BDW cdclk code boils down to two > things: claming the cdclk to the maximum supported, and panel fitter > downscaling ratio > > Unifying the the max cdclk clamping is easy, just do it. > > And as far as the panel fitter is concerned, SKL+ already (ab)use the > pch pfit state for its pipe scaler information, so it will compute the > adjusted pixel rate correctly. > > So we can just use the BDW code for BXT, as long as we lift the BXT > pixel rate -> cdclk selection into the correct place. I don't like the direction taken by this patch. We have the platform specific functions to keep stuff platform specific. Now you claim bdw and bxt are almost the same, yet the functions are filled with conditionals on the platform. I value having straightforward and readable platform specific hooks much higher than the slight reduction in line count. BR, Jani. > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 87 +++++++++++++----------------------- > 1 file changed, 30 insertions(+), 57 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 978f543..0c782c7 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -5932,25 +5932,6 @@ static int valleyview_calc_cdclk(struct drm_i915_private *dev_priv, > return 200000; > } > > -static int broxton_calc_cdclk(struct drm_i915_private *dev_priv, > - int max_pixclk) > -{ > - /* > - * FIXME: > - * - set 19.2MHz bypass frequency if there are no active pipes > - */ > - if (max_pixclk > 576000) > - return 624000; > - else if (max_pixclk > 384000) > - return 576000; > - else if (max_pixclk > 288000) > - return 384000; > - else if (max_pixclk > 144000) > - return 288000; > - else > - return 144000; > -} > - > /* Compute the max pixel clock for new configuration. Uses atomic state if > * that's non-NULL, look at current state otherwise. */ > static int intel_mode_max_pixclk(struct drm_device *dev, > @@ -5990,21 +5971,6 @@ static int valleyview_modeset_calc_cdclk(struct drm_atomic_state *state) > return 0; > } > > -static int broxton_modeset_calc_cdclk(struct drm_atomic_state *state) > -{ > - struct drm_device *dev = state->dev; > - struct drm_i915_private *dev_priv = dev->dev_private; > - int max_pixclk = intel_mode_max_pixclk(dev, state); > - > - if (max_pixclk < 0) > - return max_pixclk; > - > - to_intel_atomic_state(state)->cdclk = > - broxton_calc_cdclk(dev_priv, max_pixclk); > - > - return 0; > -} > - > static void vlv_program_pfi_credits(struct drm_i915_private *dev_priv) > { > unsigned int credits, default_credits; > @@ -9497,14 +9463,6 @@ void hsw_disable_pc8(struct drm_i915_private *dev_priv) > intel_prepare_ddi(dev); > } > > -static void broxton_modeset_commit_cdclk(struct drm_atomic_state *old_state) > -{ > - struct drm_device *dev = old_state->dev; > - unsigned int req_cdclk = to_intel_atomic_state(old_state)->cdclk; > - > - broxton_set_cdclk(dev, req_cdclk); > -} > - > /* compute the max rate for new configuration */ > static int ilk_max_pixel_rate(struct drm_atomic_state *state) > { > @@ -9621,14 +9579,31 @@ static int broadwell_modeset_calc_cdclk(struct drm_atomic_state *state) > * FIXME should also account for plane ratio > * once 64bpp pixel formats are supported. > */ > - if (max_pixclk > 540000) > - cdclk = 675000; > - else if (max_pixclk > 450000) > - cdclk = 540000; > - else if (max_pixclk > 337500) > - cdclk = 450000; > - else > - cdclk = 337500; > + if (IS_BROXTON(dev_priv)) { > + /* > + * FIXME: > + * - set 19.2MHz bypass frequency if there are no active pipes > + */ > + if (max_pixclk > 576000) > + cdclk = 624000; > + else if (max_pixclk > 384000) > + cdclk = 576000; > + else if (max_pixclk > 288000) > + cdclk = 384000; > + else if (max_pixclk > 144000) > + cdclk = 288000; > + else > + cdclk = 144000; > + } else { > + if (max_pixclk > 540000) > + cdclk = 675000; > + else if (max_pixclk > 450000) > + cdclk = 540000; > + else if (max_pixclk > 337500) > + cdclk = 450000; > + else > + cdclk = 337500; > + } > > /* > * FIXME move the cdclk caclulation to > @@ -9650,7 +9625,10 @@ static void broadwell_modeset_commit_cdclk(struct drm_atomic_state *old_state) > struct drm_device *dev = old_state->dev; > unsigned int req_cdclk = to_intel_atomic_state(old_state)->cdclk; > > - broadwell_set_cdclk(dev, req_cdclk); > + if (IS_BROXTON(dev)) > + broxton_set_cdclk(dev, req_cdclk); > + else > + broadwell_set_cdclk(dev, req_cdclk); > } > > static int haswell_crtc_compute_clock(struct intel_crtc *crtc, > @@ -14619,7 +14597,7 @@ static void intel_init_display(struct drm_device *dev) > dev_priv->display.fdi_link_train = hsw_fdi_link_train; > } > > - if (IS_BROADWELL(dev)) { > + if (IS_BROADWELL(dev) || IS_BROXTON(dev)) { > dev_priv->display.modeset_commit_cdclk = > broadwell_modeset_commit_cdclk; > dev_priv->display.modeset_calc_cdclk = > @@ -14629,11 +14607,6 @@ static void intel_init_display(struct drm_device *dev) > valleyview_modeset_commit_cdclk; > dev_priv->display.modeset_calc_cdclk = > valleyview_modeset_calc_cdclk; > - } else if (IS_BROXTON(dev)) { > - dev_priv->display.modeset_commit_cdclk = > - broxton_modeset_commit_cdclk; > - dev_priv->display.modeset_calc_cdclk = > - broxton_modeset_calc_cdclk; > } > > switch (INTEL_INFO(dev)->gen) { -- Jani Nikula, Intel Open Source Technology Center _______________________________________________ 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 3/3] drm/i915: Share cdclk code for BDW and BXT 2015-10-27 13:43 ` Jani Nikula @ 2015-10-27 14:31 ` Ville Syrjälä 2015-10-30 16:06 ` Daniel Vetter 0 siblings, 1 reply; 7+ messages in thread From: Ville Syrjälä @ 2015-10-27 14:31 UTC (permalink / raw) To: Jani Nikula; +Cc: intel-gfx On Tue, Oct 27, 2015 at 03:43:03PM +0200, Jani Nikula wrote: > On Tue, 27 Oct 2015, ville.syrjala@linux.intel.com wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > The difference betwen the BXT and BDW cdclk code boils down to two > > things: claming the cdclk to the maximum supported, and panel fitter > > downscaling ratio > > > > Unifying the the max cdclk clamping is easy, just do it. > > > > And as far as the panel fitter is concerned, SKL+ already (ab)use the > > pch pfit state for its pipe scaler information, so it will compute the > > adjusted pixel rate correctly. > > > > So we can just use the BDW code for BXT, as long as we lift the BXT > > pixel rate -> cdclk selection into the correct place. > > I don't like the direction taken by this patch. > > We have the platform specific functions to keep stuff platform > specific. Now you claim bdw and bxt are almost the same, yet the > functions are filled with conditionals on the platform. I value having > straightforward and readable platform specific hooks much higher than > the slight reduction in line count. I agree that it's not all that nice. The real problem is that the current cdclk function pointers are just too high level. They really should be at the calc_cdclk and set_cdclk level. But fixing that would require actually dealing with the gmch pfit, and stuffing the power domain hack (assuming we still need it) and the pfi programming for vlv/chv into the lower level functions. And looks like no one has bothered to do any of that. > > BR, > Jani. > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/i915/intel_display.c | 87 +++++++++++++----------------------- > > 1 file changed, 30 insertions(+), 57 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index 978f543..0c782c7 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -5932,25 +5932,6 @@ static int valleyview_calc_cdclk(struct drm_i915_private *dev_priv, > > return 200000; > > } > > > > -static int broxton_calc_cdclk(struct drm_i915_private *dev_priv, > > - int max_pixclk) > > -{ > > - /* > > - * FIXME: > > - * - set 19.2MHz bypass frequency if there are no active pipes > > - */ > > - if (max_pixclk > 576000) > > - return 624000; > > - else if (max_pixclk > 384000) > > - return 576000; > > - else if (max_pixclk > 288000) > > - return 384000; > > - else if (max_pixclk > 144000) > > - return 288000; > > - else > > - return 144000; > > -} > > - > > /* Compute the max pixel clock for new configuration. Uses atomic state if > > * that's non-NULL, look at current state otherwise. */ > > static int intel_mode_max_pixclk(struct drm_device *dev, > > @@ -5990,21 +5971,6 @@ static int valleyview_modeset_calc_cdclk(struct drm_atomic_state *state) > > return 0; > > } > > > > -static int broxton_modeset_calc_cdclk(struct drm_atomic_state *state) > > -{ > > - struct drm_device *dev = state->dev; > > - struct drm_i915_private *dev_priv = dev->dev_private; > > - int max_pixclk = intel_mode_max_pixclk(dev, state); > > - > > - if (max_pixclk < 0) > > - return max_pixclk; > > - > > - to_intel_atomic_state(state)->cdclk = > > - broxton_calc_cdclk(dev_priv, max_pixclk); > > - > > - return 0; > > -} > > - > > static void vlv_program_pfi_credits(struct drm_i915_private *dev_priv) > > { > > unsigned int credits, default_credits; > > @@ -9497,14 +9463,6 @@ void hsw_disable_pc8(struct drm_i915_private *dev_priv) > > intel_prepare_ddi(dev); > > } > > > > -static void broxton_modeset_commit_cdclk(struct drm_atomic_state *old_state) > > -{ > > - struct drm_device *dev = old_state->dev; > > - unsigned int req_cdclk = to_intel_atomic_state(old_state)->cdclk; > > - > > - broxton_set_cdclk(dev, req_cdclk); > > -} > > - > > /* compute the max rate for new configuration */ > > static int ilk_max_pixel_rate(struct drm_atomic_state *state) > > { > > @@ -9621,14 +9579,31 @@ static int broadwell_modeset_calc_cdclk(struct drm_atomic_state *state) > > * FIXME should also account for plane ratio > > * once 64bpp pixel formats are supported. > > */ > > - if (max_pixclk > 540000) > > - cdclk = 675000; > > - else if (max_pixclk > 450000) > > - cdclk = 540000; > > - else if (max_pixclk > 337500) > > - cdclk = 450000; > > - else > > - cdclk = 337500; > > + if (IS_BROXTON(dev_priv)) { > > + /* > > + * FIXME: > > + * - set 19.2MHz bypass frequency if there are no active pipes > > + */ > > + if (max_pixclk > 576000) > > + cdclk = 624000; > > + else if (max_pixclk > 384000) > > + cdclk = 576000; > > + else if (max_pixclk > 288000) > > + cdclk = 384000; > > + else if (max_pixclk > 144000) > > + cdclk = 288000; > > + else > > + cdclk = 144000; > > + } else { > > + if (max_pixclk > 540000) > > + cdclk = 675000; > > + else if (max_pixclk > 450000) > > + cdclk = 540000; > > + else if (max_pixclk > 337500) > > + cdclk = 450000; > > + else > > + cdclk = 337500; > > + } > > > > /* > > * FIXME move the cdclk caclulation to > > @@ -9650,7 +9625,10 @@ static void broadwell_modeset_commit_cdclk(struct drm_atomic_state *old_state) > > struct drm_device *dev = old_state->dev; > > unsigned int req_cdclk = to_intel_atomic_state(old_state)->cdclk; > > > > - broadwell_set_cdclk(dev, req_cdclk); > > + if (IS_BROXTON(dev)) > > + broxton_set_cdclk(dev, req_cdclk); > > + else > > + broadwell_set_cdclk(dev, req_cdclk); > > } > > > > static int haswell_crtc_compute_clock(struct intel_crtc *crtc, > > @@ -14619,7 +14597,7 @@ static void intel_init_display(struct drm_device *dev) > > dev_priv->display.fdi_link_train = hsw_fdi_link_train; > > } > > > > - if (IS_BROADWELL(dev)) { > > + if (IS_BROADWELL(dev) || IS_BROXTON(dev)) { > > dev_priv->display.modeset_commit_cdclk = > > broadwell_modeset_commit_cdclk; > > dev_priv->display.modeset_calc_cdclk = > > @@ -14629,11 +14607,6 @@ static void intel_init_display(struct drm_device *dev) > > valleyview_modeset_commit_cdclk; > > dev_priv->display.modeset_calc_cdclk = > > valleyview_modeset_calc_cdclk; > > - } else if (IS_BROXTON(dev)) { > > - dev_priv->display.modeset_commit_cdclk = > > - broxton_modeset_commit_cdclk; > > - dev_priv->display.modeset_calc_cdclk = > > - broxton_modeset_calc_cdclk; > > } > > > > switch (INTEL_INFO(dev)->gen) { > > -- > Jani Nikula, Intel Open Source Technology Center -- 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 3/3] drm/i915: Share cdclk code for BDW and BXT 2015-10-27 14:31 ` Ville Syrjälä @ 2015-10-30 16:06 ` Daniel Vetter 2015-10-30 17:32 ` Ville Syrjälä 0 siblings, 1 reply; 7+ messages in thread From: Daniel Vetter @ 2015-10-30 16:06 UTC (permalink / raw) To: Ville Syrjälä; +Cc: intel-gfx On Tue, Oct 27, 2015 at 04:31:56PM +0200, Ville Syrjälä wrote: > On Tue, Oct 27, 2015 at 03:43:03PM +0200, Jani Nikula wrote: > > On Tue, 27 Oct 2015, ville.syrjala@linux.intel.com wrote: > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > The difference betwen the BXT and BDW cdclk code boils down to two > > > things: claming the cdclk to the maximum supported, and panel fitter > > > downscaling ratio > > > > > > Unifying the the max cdclk clamping is easy, just do it. > > > > > > And as far as the panel fitter is concerned, SKL+ already (ab)use the > > > pch pfit state for its pipe scaler information, so it will compute the > > > adjusted pixel rate correctly. > > > > > > So we can just use the BDW code for BXT, as long as we lift the BXT > > > pixel rate -> cdclk selection into the correct place. > > > > I don't like the direction taken by this patch. > > > > We have the platform specific functions to keep stuff platform > > specific. Now you claim bdw and bxt are almost the same, yet the > > functions are filled with conditionals on the platform. I value having > > straightforward and readable platform specific hooks much higher than > > the slight reduction in line count. > > I agree that it's not all that nice. The real problem is that the current > cdclk function pointers are just too high level. They really should > be at the calc_cdclk and set_cdclk level. But fixing that would require > actually dealing with the gmch pfit, and stuffing the power domain > hack (assuming we still need it) and the pfi programming for vlv/chv > into the lower level functions. And looks like no one has bothered to do > any of that. vfunc hooks imo should be high-level, with common code shared by extracting helpers. Trying to make vfuncs super flexible is imo an approach which often just leads to big trouble. We probably should rip out a pile of the current cdlock vfunc hooks ... Unfortunately atomic is still in-flight a lot, and it's also lacking some really important top-level vfuncs for handling modesetting. -Daniel > > > > > BR, > > Jani. > > > > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_display.c | 87 +++++++++++++----------------------- > > > 1 file changed, 30 insertions(+), 57 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > > index 978f543..0c782c7 100644 > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > @@ -5932,25 +5932,6 @@ static int valleyview_calc_cdclk(struct drm_i915_private *dev_priv, > > > return 200000; > > > } > > > > > > -static int broxton_calc_cdclk(struct drm_i915_private *dev_priv, > > > - int max_pixclk) > > > -{ > > > - /* > > > - * FIXME: > > > - * - set 19.2MHz bypass frequency if there are no active pipes > > > - */ > > > - if (max_pixclk > 576000) > > > - return 624000; > > > - else if (max_pixclk > 384000) > > > - return 576000; > > > - else if (max_pixclk > 288000) > > > - return 384000; > > > - else if (max_pixclk > 144000) > > > - return 288000; > > > - else > > > - return 144000; > > > -} > > > - > > > /* Compute the max pixel clock for new configuration. Uses atomic state if > > > * that's non-NULL, look at current state otherwise. */ > > > static int intel_mode_max_pixclk(struct drm_device *dev, > > > @@ -5990,21 +5971,6 @@ static int valleyview_modeset_calc_cdclk(struct drm_atomic_state *state) > > > return 0; > > > } > > > > > > -static int broxton_modeset_calc_cdclk(struct drm_atomic_state *state) > > > -{ > > > - struct drm_device *dev = state->dev; > > > - struct drm_i915_private *dev_priv = dev->dev_private; > > > - int max_pixclk = intel_mode_max_pixclk(dev, state); > > > - > > > - if (max_pixclk < 0) > > > - return max_pixclk; > > > - > > > - to_intel_atomic_state(state)->cdclk = > > > - broxton_calc_cdclk(dev_priv, max_pixclk); > > > - > > > - return 0; > > > -} > > > - > > > static void vlv_program_pfi_credits(struct drm_i915_private *dev_priv) > > > { > > > unsigned int credits, default_credits; > > > @@ -9497,14 +9463,6 @@ void hsw_disable_pc8(struct drm_i915_private *dev_priv) > > > intel_prepare_ddi(dev); > > > } > > > > > > -static void broxton_modeset_commit_cdclk(struct drm_atomic_state *old_state) > > > -{ > > > - struct drm_device *dev = old_state->dev; > > > - unsigned int req_cdclk = to_intel_atomic_state(old_state)->cdclk; > > > - > > > - broxton_set_cdclk(dev, req_cdclk); > > > -} > > > - > > > /* compute the max rate for new configuration */ > > > static int ilk_max_pixel_rate(struct drm_atomic_state *state) > > > { > > > @@ -9621,14 +9579,31 @@ static int broadwell_modeset_calc_cdclk(struct drm_atomic_state *state) > > > * FIXME should also account for plane ratio > > > * once 64bpp pixel formats are supported. > > > */ > > > - if (max_pixclk > 540000) > > > - cdclk = 675000; > > > - else if (max_pixclk > 450000) > > > - cdclk = 540000; > > > - else if (max_pixclk > 337500) > > > - cdclk = 450000; > > > - else > > > - cdclk = 337500; > > > + if (IS_BROXTON(dev_priv)) { > > > + /* > > > + * FIXME: > > > + * - set 19.2MHz bypass frequency if there are no active pipes > > > + */ > > > + if (max_pixclk > 576000) > > > + cdclk = 624000; > > > + else if (max_pixclk > 384000) > > > + cdclk = 576000; > > > + else if (max_pixclk > 288000) > > > + cdclk = 384000; > > > + else if (max_pixclk > 144000) > > > + cdclk = 288000; > > > + else > > > + cdclk = 144000; > > > + } else { > > > + if (max_pixclk > 540000) > > > + cdclk = 675000; > > > + else if (max_pixclk > 450000) > > > + cdclk = 540000; > > > + else if (max_pixclk > 337500) > > > + cdclk = 450000; > > > + else > > > + cdclk = 337500; > > > + } > > > > > > /* > > > * FIXME move the cdclk caclulation to > > > @@ -9650,7 +9625,10 @@ static void broadwell_modeset_commit_cdclk(struct drm_atomic_state *old_state) > > > struct drm_device *dev = old_state->dev; > > > unsigned int req_cdclk = to_intel_atomic_state(old_state)->cdclk; > > > > > > - broadwell_set_cdclk(dev, req_cdclk); > > > + if (IS_BROXTON(dev)) > > > + broxton_set_cdclk(dev, req_cdclk); > > > + else > > > + broadwell_set_cdclk(dev, req_cdclk); > > > } > > > > > > static int haswell_crtc_compute_clock(struct intel_crtc *crtc, > > > @@ -14619,7 +14597,7 @@ static void intel_init_display(struct drm_device *dev) > > > dev_priv->display.fdi_link_train = hsw_fdi_link_train; > > > } > > > > > > - if (IS_BROADWELL(dev)) { > > > + if (IS_BROADWELL(dev) || IS_BROXTON(dev)) { > > > dev_priv->display.modeset_commit_cdclk = > > > broadwell_modeset_commit_cdclk; > > > dev_priv->display.modeset_calc_cdclk = > > > @@ -14629,11 +14607,6 @@ static void intel_init_display(struct drm_device *dev) > > > valleyview_modeset_commit_cdclk; > > > dev_priv->display.modeset_calc_cdclk = > > > valleyview_modeset_calc_cdclk; > > > - } else if (IS_BROXTON(dev)) { > > > - dev_priv->display.modeset_commit_cdclk = > > > - broxton_modeset_commit_cdclk; > > > - dev_priv->display.modeset_calc_cdclk = > > > - broxton_modeset_calc_cdclk; > > > } > > > > > > switch (INTEL_INFO(dev)->gen) { > > > > -- > > Jani Nikula, Intel Open Source Technology Center > > -- > Ville Syrjälä > Intel OTC > _______________________________________________ > 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 3/3] drm/i915: Share cdclk code for BDW and BXT 2015-10-30 16:06 ` Daniel Vetter @ 2015-10-30 17:32 ` Ville Syrjälä 0 siblings, 0 replies; 7+ messages in thread From: Ville Syrjälä @ 2015-10-30 17:32 UTC (permalink / raw) To: Daniel Vetter; +Cc: intel-gfx On Fri, Oct 30, 2015 at 05:06:27PM +0100, Daniel Vetter wrote: > On Tue, Oct 27, 2015 at 04:31:56PM +0200, Ville Syrjälä wrote: > > On Tue, Oct 27, 2015 at 03:43:03PM +0200, Jani Nikula wrote: > > > On Tue, 27 Oct 2015, ville.syrjala@linux.intel.com wrote: > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > > > The difference betwen the BXT and BDW cdclk code boils down to two > > > > things: claming the cdclk to the maximum supported, and panel fitter > > > > downscaling ratio > > > > > > > > Unifying the the max cdclk clamping is easy, just do it. > > > > > > > > And as far as the panel fitter is concerned, SKL+ already (ab)use the > > > > pch pfit state for its pipe scaler information, so it will compute the > > > > adjusted pixel rate correctly. > > > > > > > > So we can just use the BDW code for BXT, as long as we lift the BXT > > > > pixel rate -> cdclk selection into the correct place. > > > > > > I don't like the direction taken by this patch. > > > > > > We have the platform specific functions to keep stuff platform > > > specific. Now you claim bdw and bxt are almost the same, yet the > > > functions are filled with conditionals on the platform. I value having > > > straightforward and readable platform specific hooks much higher than > > > the slight reduction in line count. > > > > I agree that it's not all that nice. The real problem is that the current > > cdclk function pointers are just too high level. They really should > > be at the calc_cdclk and set_cdclk level. But fixing that would require > > actually dealing with the gmch pfit, and stuffing the power domain > > hack (assuming we still need it) and the pfi programming for vlv/chv > > into the lower level functions. And looks like no one has bothered to do > > any of that. > > vfunc hooks imo should be high-level, with common code shared by > extracting helpers. Trying to make vfuncs super flexible is imo an > approach which often just leads to big trouble. We probably should rip out > a pile of the current cdlock vfunc hooks ... Unfortunately atomic is still > in-flight a lot, and it's also lacking some really important top-level > vfuncs for handling modesetting. The top level cdclk logic is (or rather should be) the same for all platforms. Shoveling it into vfuncs doesn't make sense to me. Only the low level details differ. > -Daniel > > > > > > > > > BR, > > > Jani. > > > > > > > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > --- > > > > drivers/gpu/drm/i915/intel_display.c | 87 +++++++++++++----------------------- > > > > 1 file changed, 30 insertions(+), 57 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > > > index 978f543..0c782c7 100644 > > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > > @@ -5932,25 +5932,6 @@ static int valleyview_calc_cdclk(struct drm_i915_private *dev_priv, > > > > return 200000; > > > > } > > > > > > > > -static int broxton_calc_cdclk(struct drm_i915_private *dev_priv, > > > > - int max_pixclk) > > > > -{ > > > > - /* > > > > - * FIXME: > > > > - * - set 19.2MHz bypass frequency if there are no active pipes > > > > - */ > > > > - if (max_pixclk > 576000) > > > > - return 624000; > > > > - else if (max_pixclk > 384000) > > > > - return 576000; > > > > - else if (max_pixclk > 288000) > > > > - return 384000; > > > > - else if (max_pixclk > 144000) > > > > - return 288000; > > > > - else > > > > - return 144000; > > > > -} > > > > - > > > > /* Compute the max pixel clock for new configuration. Uses atomic state if > > > > * that's non-NULL, look at current state otherwise. */ > > > > static int intel_mode_max_pixclk(struct drm_device *dev, > > > > @@ -5990,21 +5971,6 @@ static int valleyview_modeset_calc_cdclk(struct drm_atomic_state *state) > > > > return 0; > > > > } > > > > > > > > -static int broxton_modeset_calc_cdclk(struct drm_atomic_state *state) > > > > -{ > > > > - struct drm_device *dev = state->dev; > > > > - struct drm_i915_private *dev_priv = dev->dev_private; > > > > - int max_pixclk = intel_mode_max_pixclk(dev, state); > > > > - > > > > - if (max_pixclk < 0) > > > > - return max_pixclk; > > > > - > > > > - to_intel_atomic_state(state)->cdclk = > > > > - broxton_calc_cdclk(dev_priv, max_pixclk); > > > > - > > > > - return 0; > > > > -} > > > > - > > > > static void vlv_program_pfi_credits(struct drm_i915_private *dev_priv) > > > > { > > > > unsigned int credits, default_credits; > > > > @@ -9497,14 +9463,6 @@ void hsw_disable_pc8(struct drm_i915_private *dev_priv) > > > > intel_prepare_ddi(dev); > > > > } > > > > > > > > -static void broxton_modeset_commit_cdclk(struct drm_atomic_state *old_state) > > > > -{ > > > > - struct drm_device *dev = old_state->dev; > > > > - unsigned int req_cdclk = to_intel_atomic_state(old_state)->cdclk; > > > > - > > > > - broxton_set_cdclk(dev, req_cdclk); > > > > -} > > > > - > > > > /* compute the max rate for new configuration */ > > > > static int ilk_max_pixel_rate(struct drm_atomic_state *state) > > > > { > > > > @@ -9621,14 +9579,31 @@ static int broadwell_modeset_calc_cdclk(struct drm_atomic_state *state) > > > > * FIXME should also account for plane ratio > > > > * once 64bpp pixel formats are supported. > > > > */ > > > > - if (max_pixclk > 540000) > > > > - cdclk = 675000; > > > > - else if (max_pixclk > 450000) > > > > - cdclk = 540000; > > > > - else if (max_pixclk > 337500) > > > > - cdclk = 450000; > > > > - else > > > > - cdclk = 337500; > > > > + if (IS_BROXTON(dev_priv)) { > > > > + /* > > > > + * FIXME: > > > > + * - set 19.2MHz bypass frequency if there are no active pipes > > > > + */ > > > > + if (max_pixclk > 576000) > > > > + cdclk = 624000; > > > > + else if (max_pixclk > 384000) > > > > + cdclk = 576000; > > > > + else if (max_pixclk > 288000) > > > > + cdclk = 384000; > > > > + else if (max_pixclk > 144000) > > > > + cdclk = 288000; > > > > + else > > > > + cdclk = 144000; > > > > + } else { > > > > + if (max_pixclk > 540000) > > > > + cdclk = 675000; > > > > + else if (max_pixclk > 450000) > > > > + cdclk = 540000; > > > > + else if (max_pixclk > 337500) > > > > + cdclk = 450000; > > > > + else > > > > + cdclk = 337500; > > > > + } > > > > > > > > /* > > > > * FIXME move the cdclk caclulation to > > > > @@ -9650,7 +9625,10 @@ static void broadwell_modeset_commit_cdclk(struct drm_atomic_state *old_state) > > > > struct drm_device *dev = old_state->dev; > > > > unsigned int req_cdclk = to_intel_atomic_state(old_state)->cdclk; > > > > > > > > - broadwell_set_cdclk(dev, req_cdclk); > > > > + if (IS_BROXTON(dev)) > > > > + broxton_set_cdclk(dev, req_cdclk); > > > > + else > > > > + broadwell_set_cdclk(dev, req_cdclk); > > > > } > > > > > > > > static int haswell_crtc_compute_clock(struct intel_crtc *crtc, > > > > @@ -14619,7 +14597,7 @@ static void intel_init_display(struct drm_device *dev) > > > > dev_priv->display.fdi_link_train = hsw_fdi_link_train; > > > > } > > > > > > > > - if (IS_BROADWELL(dev)) { > > > > + if (IS_BROADWELL(dev) || IS_BROXTON(dev)) { > > > > dev_priv->display.modeset_commit_cdclk = > > > > broadwell_modeset_commit_cdclk; > > > > dev_priv->display.modeset_calc_cdclk = > > > > @@ -14629,11 +14607,6 @@ static void intel_init_display(struct drm_device *dev) > > > > valleyview_modeset_commit_cdclk; > > > > dev_priv->display.modeset_calc_cdclk = > > > > valleyview_modeset_calc_cdclk; > > > > - } else if (IS_BROXTON(dev)) { > > > > - dev_priv->display.modeset_commit_cdclk = > > > > - broxton_modeset_commit_cdclk; > > > > - dev_priv->display.modeset_calc_cdclk = > > > > - broxton_modeset_calc_cdclk; > > > > } > > > > > > > > switch (INTEL_INFO(dev)->gen) { > > > > > > -- > > > Jani Nikula, Intel Open Source Technology Center > > > > -- > > Ville Syrjälä > > Intel OTC > > _______________________________________________ > > 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 -- 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
end of thread, other threads:[~2015-10-30 17:32 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-10-27 13:23 [PATCH 1/3] drm/i915: Remove 10% cdclk guardband from BXT ville.syrjala 2015-10-27 13:23 ` [PATCH 2/3] drm/i915: Untangle .fdi_link_train() and cdclk function pointer assignments ville.syrjala 2015-10-27 13:23 ` [PATCH 3/3] drm/i915: Share cdclk code for BDW and BXT ville.syrjala 2015-10-27 13:43 ` Jani Nikula 2015-10-27 14:31 ` Ville Syrjälä 2015-10-30 16:06 ` Daniel Vetter 2015-10-30 17:32 ` Ville Syrjälä
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox