[Intel-gfx] [PATCH v2 3/3] drm/i915: DDI: call intel_psr_ and _edp_drrs_enable() on pipe updates (v2)

[Daniel Vetter]

On Fri, Dec 21, 2018 at 8:42 PM Dhinakaran Pandiyan
<dhinakaran.pandiyan at intel.com> wrote:
>
> On Thu, 2018-12-20 at 15:13 -0800, Dhinakaran Pandiyan wrote:
> > On Thu, 2018-12-20 at 09:10 -0800, Rodrigo Vivi wrote:
> > > On Thu, Dec 20, 2018 at 02:21:20PM +0100, Hans de Goede wrote:
> > > > Call intel_psr_enable() and intel_edp_drrs_enable() on pipe
> > > > updates
> > > > to make
> > > > sure that we enable PSR / DRRS (when applicable) on fastsets.
> >
> > I am probably missing something, doesn't intel_pipe_config_compare()
> > need to check for pipe_config->has_psr? And also read the hardware
> > PSR
> > state at boot?
> Answering my own question here, pipe_config_compare() returns true
> lacking a comparison for ->has_psr. And we assume the bios does not
> enable PSR, so no need to read the hardware state.

pipe_config_compare is also a validation tool, ensuring that all the
various fastboot (and other modeset) transitions work. Not reading out
& not comparing state reduces validation coverage. Some exceptions
apply, but generally they should be justified with other test
coverage, not with "it works without that". Everything is supposed to
work without the validation tools/tests :-)
-Daniel

> > > >
> > > > Note calling these functions when PSR / DRRS has already been
> > > > enabled is a
> > > > no-op, so it is safe to do this on every encoder->update_pipe
> > > > callback.
> > > >
> > > > Changes in v2:
> > > > -Merge the patches adding the intel_psr_enable() and
> > > > intel_edp_drrs_enable()
> > > >  calls into a single patch
> > > >
> > > > Reviewed-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com
> > > > >
> > > > Signed-off-by: Hans de Goede <hdegoede at redhat.com>
> > >
> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com>
> > > Cc: José Roberto de Souza <jose.souza at intel.com>
> > >
> > > Acked-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > >
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_ddi.c | 19 +++++++++++++++++++
> > > >  1 file changed, 19 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> > > > b/drivers/gpu/drm/i915/intel_ddi.c
> > > > index e3cc19e19199..fdf57f451b72 100644
> > > > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > > > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > > > @@ -3537,6 +3537,24 @@ static void intel_disable_ddi(struct
> > > > intel_encoder *encoder,
> > > >           intel_disable_ddi_dp(encoder, old_crtc_state,
> > > > old_conn_state);
> > > >  }
> > > >
> > > > +static void intel_ddi_update_pipe_dp(struct intel_encoder
> > > > *encoder,
> > > > +                              const struct intel_crtc_state
> > > > *crtc_state,
> > > > +                              const struct drm_connector_state
> > > > *conn_state)
> > > > +{
> > > > + struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
> > > > +
> > > > + intel_psr_enable(intel_dp, crtc_state);
> > > > + intel_edp_drrs_enable(intel_dp, crtc_state);
> > > > +}
> > > > +
> > > > +static void intel_ddi_update_pipe(struct intel_encoder *encoder,
> > > > +                           const struct intel_crtc_state
> > > > *crtc_state,
> > > > +                           const struct drm_connector_state
> > > > *conn_state)
> > > > +{
> > > > + if (!intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI))
> >
> > We could restrict this to eDP outputs as both PSR and DRRS are eDP
> > features.
> >
> > > > +         intel_ddi_update_pipe_dp(encoder, crtc_state,
> > > > conn_state);
> > > > +}
> > > > +
> > > >  static void intel_ddi_set_fia_lane_count(struct intel_encoder
> > > > *encoder,
> > > >                                    const struct intel_crtc_state
> > > > *pipe_config,
> > > >                                    enum port port)
> > > > @@ -4169,6 +4187,7 @@ void intel_ddi_init(struct drm_i915_private
> > > > *dev_priv, enum port port)
> > > >   intel_encoder->pre_enable = intel_ddi_pre_enable;
> > > >   intel_encoder->disable = intel_disable_ddi;
> > > >   intel_encoder->post_disable = intel_ddi_post_disable;
> > > > + intel_encoder->update_pipe = intel_ddi_update_pipe;
> > > >   intel_encoder->get_hw_state = intel_ddi_get_hw_state;
> > > >   intel_encoder->get_config = intel_ddi_get_config;
> > > >   intel_encoder->suspend = intel_ddi_encoder_suspend;
> > > > --
> > > > 2.20.1
> > > >
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch