On Tue, 27 Oct 2015, ville.syrjala@xxxxxxxxxxxxxxx wrote:
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
