> -----Original Message-----
> From: Jani Nikula <jani.nikula@xxxxxxxxx>
> Sent: Thursday, January 14, 2021 2:27 PM
> To: Gupta, Anshuman <anshuman.gupta@xxxxxxxxx>
> Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; ville.syrjala@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v2 15/17] drm/i915/pps: move pps code over from
> intel_display.c and refactor
>
> On Wed, 13 Jan 2021, Anshuman Gupta <anshuman.gupta@xxxxxxxxx>
> wrote:
> > On 2021-01-08 at 19:44:23 +0200, Jani Nikula wrote:
> >> intel_display.c has some pps functions that belong to intel_pps.c.
> >> Move them over.
> >>
> >> While at it, refactor the duplicate intel_pps_init() in
> >> intel_display.c into an orthogonal intel_pps_setup() in intel_pps.c,
> >> and call it earlier in intel_modeset_init_nogem().
> >>
> >> Signed-off-by: Jani Nikula <jani.nikula@xxxxxxxxx>
> >> ---
> >> drivers/gpu/drm/i915/display/intel_display.c | 41
> >> ++------------------ drivers/gpu/drm/i915/display/intel_display.h | 1 -
> >> drivers/gpu/drm/i915/display/intel_pps.c | 34 ++++++++++++++++
> >> drivers/gpu/drm/i915/display/intel_pps.h | 3 ++
> >> drivers/gpu/drm/i915/i915_drv.c | 1 +
> >> 5 files changed, 42 insertions(+), 38 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> >> b/drivers/gpu/drm/i915/display/intel_display.c
> >> index 0189d379a55e..f8806c4ecb21 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> >> @@ -81,6 +81,7 @@
> >> #include "intel_overlay.h"
> >> #include "intel_pipe_crc.h"
> >> #include "intel_pm.h"
> >> +#include "intel_pps.h"
> >> #include "intel_psr.h"
> >> #include "intel_quirks.h"
> >> #include "intel_sideband.h"
> >> @@ -16100,48 +16101,12 @@ static bool intel_ddi_crt_present(struct
> drm_i915_private *dev_priv)
> >> return true;
> >> }
> >>
> >> -void intel_pps_unlock_regs_wa(struct drm_i915_private *dev_priv) -{
> >> - int pps_num;
> >> - int pps_idx;
> >> -
> >> - if (HAS_DDI(dev_priv))
> >> - return;
> >> - /*
> >> - * This w/a is needed at least on CPT/PPT, but to be sure apply it
> >> - * everywhere where registers can be write protected.
> >> - */
> >> - if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> >> - pps_num = 2;
> >> - else
> >> - pps_num = 1;
> >> -
> >> - for (pps_idx = 0; pps_idx < pps_num; pps_idx++) {
> >> - u32 val = intel_de_read(dev_priv, PP_CONTROL(pps_idx));
> >> -
> >> - val = (val & ~PANEL_UNLOCK_MASK) |
> PANEL_UNLOCK_REGS;
> >> - intel_de_write(dev_priv, PP_CONTROL(pps_idx), val);
> >> - }
> >> -}
> >> -
> >> -static void intel_pps_init(struct drm_i915_private *dev_priv) -{
> >> - if (HAS_PCH_SPLIT(dev_priv) || IS_GEN9_LP(dev_priv))
> >> - dev_priv->pps_mmio_base = PCH_PPS_BASE;
> >> - else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> >> - dev_priv->pps_mmio_base = VLV_PPS_BASE;
> >> - else
> >> - dev_priv->pps_mmio_base = PPS_BASE;
> >> -
> >> - intel_pps_unlock_regs_wa(dev_priv);
> >> -}
> >> -
> >> static void intel_setup_outputs(struct drm_i915_private *dev_priv)
> >> {
> >> struct intel_encoder *encoder;
> >> bool dpd_is_edp = false;
> >>
> >> - intel_pps_init(dev_priv);
> >> + intel_pps_unlock_regs_wa(dev_priv);
> >>
> >> if (!HAS_DISPLAY(dev_priv))
> >> return;
> >> @@ -17199,6 +17164,8 @@ int intel_modeset_init_nogem(struct
> >> drm_i915_private *i915)
> >>
> >> intel_panel_sanitize_ssc(i915);
> >>
> >> + intel_pps_setup(i915);
> >> +
> > This is mmio init, could you please guide me why
> > i915_driver_mmio_probe() has not chosen to call intel_pps_setup() ?
>
> Long term the goal is to divide gt and display probe more and more; there's
> no clear place in i915_driver_mmio_probe() to stick this. I certainly don't
> want to call intel_pps_setup() directly from there.
>
> This is at least close to where gpio_mmio_base gets setup in
> intel_gmbus_setup() right below.
Looks good to me.
Reviewed-by: Anshuman Gupta <anshuman.gupta@xxxxxxxxx>
>
> BR,
> Jani.
>
> >> intel_gmbus_setup(i915);
> >>
> >> drm_dbg_kms(&i915->drm, "%d display pipe%s available.\n", diff
> >> --git a/drivers/gpu/drm/i915/display/intel_display.h
> >> b/drivers/gpu/drm/i915/display/intel_display.h
> >> index 7ddbc00a0f41..bbd5dbc61ce9 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_display.h
> >> +++ b/drivers/gpu/drm/i915/display/intel_display.h
> >> @@ -546,7 +546,6 @@ unsigned int intel_rotation_info_size(const
> >> struct intel_rotation_info *rot_info unsigned int
> >> intel_remapped_info_size(const struct intel_remapped_info
> *rem_info);
> >> bool intel_has_pending_fb_unpin(struct drm_i915_private *dev_priv);
> >> int intel_display_suspend(struct drm_device *dev); -void
> >> intel_pps_unlock_regs_wa(struct drm_i915_private *dev_priv); void
> >> intel_encoder_destroy(struct drm_encoder *encoder); struct
> >> drm_display_mode * intel_encoder_current_mode(struct
> intel_encoder
> >> *encoder); diff --git a/drivers/gpu/drm/i915/display/intel_pps.c
> >> b/drivers/gpu/drm/i915/display/intel_pps.c
> >> index b4d026ca3313..c3a0fc933500 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_pps.c
> >> +++ b/drivers/gpu/drm/i915/display/intel_pps.c
> >> @@ -1370,3 +1370,37 @@ void intel_pps_init(struct intel_dp
> *intel_dp)
> >>
> >> intel_pps_encoder_reset(intel_dp);
> >> }
> >> +
> >> +void intel_pps_unlock_regs_wa(struct drm_i915_private *dev_priv) {
> >> + int pps_num;
> >> + int pps_idx;
> >> +
> >> + if (HAS_DDI(dev_priv))
> >> + return;
> >> + /*
> >> + * This w/a is needed at least on CPT/PPT, but to be sure apply it
> >> + * everywhere where registers can be write protected.
> >> + */
> >> + if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> >> + pps_num = 2;
> >> + else
> >> + pps_num = 1;
> >> +
> >> + for (pps_idx = 0; pps_idx < pps_num; pps_idx++) {
> >> + u32 val = intel_de_read(dev_priv, PP_CONTROL(pps_idx));
> >> +
> >> + val = (val & ~PANEL_UNLOCK_MASK) |
> PANEL_UNLOCK_REGS;
> >> + intel_de_write(dev_priv, PP_CONTROL(pps_idx), val);
> >> + }
> >> +}
> >> +
> >> +void intel_pps_setup(struct drm_i915_private *i915) {
> >> + if (HAS_PCH_SPLIT(i915) || IS_GEN9_LP(i915))
> >> + i915->pps_mmio_base = PCH_PPS_BASE;
> >> + else if (IS_VALLEYVIEW(i915) || IS_CHERRYVIEW(i915))
> >> + i915->pps_mmio_base = VLV_PPS_BASE;
> >> + else
> >> + i915->pps_mmio_base = PPS_BASE;
> >> +}
> >> diff --git a/drivers/gpu/drm/i915/display/intel_pps.h
> >> b/drivers/gpu/drm/i915/display/intel_pps.h
> >> index 22045c5cdc86..fbbcca782e7b 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_pps.h
> >> +++ b/drivers/gpu/drm/i915/display/intel_pps.h
> >> @@ -46,4 +46,7 @@ void intel_pps_reset_all(struct drm_i915_private
> >> *i915); void vlv_pps_init(struct intel_encoder *encoder,
> >> const struct intel_crtc_state *crtc_state);
> >>
> >> +void intel_pps_unlock_regs_wa(struct drm_i915_private *i915); void
> >> +intel_pps_setup(struct drm_i915_private *i915);
> >> +
> >> #endif /* __INTEL_PPS_H__ */
> >> diff --git a/drivers/gpu/drm/i915/i915_drv.c
> >> b/drivers/gpu/drm/i915/i915_drv.c index
> 3e504247f2da..7282c8f08318
> >> 100644
> >> --- a/drivers/gpu/drm/i915/i915_drv.c
> >> +++ b/drivers/gpu/drm/i915/i915_drv.c
> >> @@ -58,6 +58,7 @@
> >> #include "display/intel_hotplug.h"
> >> #include "display/intel_overlay.h"
> >> #include "display/intel_pipe_crc.h"
> >> +#include "display/intel_pps.h"
> >> #include "display/intel_sprite.h"
> >> #include "display/intel_vga.h"
> >>
> >> --
> >> 2.20.1
> >>
>
> --
> Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
