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@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.
>
> 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@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
>
> --
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
