| Message ID | 20181220132120.15318-3-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/3] drm/i915: Add an update_pipe callback to intel_encoder and call this on fastsets (v2) | expand |
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. > > 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@linux.intel.com> > Signed-off-by: Hans de Goede <hdegoede@redhat.com> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> Cc: José Roberto de Souza <jose.souza@intel.com> Acked-by: Rodrigo Vivi <rodrigo.vivi@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)) > + 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 >
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? > > > > 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@linux.intel.com> > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > Cc: José Roberto de Souza <jose.souza@intel.com> > > Acked-by: Rodrigo Vivi <rodrigo.vivi@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 > >
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. > > > > > > 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@linux.intel.com > > > > > > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > Cc: José Roberto de Souza <jose.souza@intel.com> > > > > Acked-by: Rodrigo Vivi <rodrigo.vivi@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 > > >
On Fri, Dec 21, 2018 at 8:42 PM Dhinakaran Pandiyan <dhinakaran.pandiyan@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@linux.intel.com > > > > > > > > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > > > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > Cc: José Roberto de Souza <jose.souza@intel.com> > > > > > > Acked-by: Rodrigo Vivi <rodrigo.vivi@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@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Hi, On 20-12-18 18:10, 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. >> >> 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@linux.intel.com> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > Cc: José Roberto de Souza <jose.souza@intel.com> > > Acked-by: Rodrigo Vivi <rodrigo.vivi@intel.com> After restoring my dinq mistake from yesterday I now have pushed this properly. Maarten, Rodrigo, thank you for the Review/Ack. And with this wrapped up I'm to celebrate christmas with my family. Merry christmas everyone. Regards, Hans > >> --- >> 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)) >> + 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 >>
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)) + 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;