| Message ID | 20180705122654.17072-1-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jul 05, 2018 at 03:26:54PM +0300, Imre Deak wrote: > We can simplify the encoder's get_power_domains() hook by calling it > only if the encoder is active. That way the hook can return its power > domains unconditionally without checking the active state by calling > encoder::get_hw_state(). This get_hw_state() query is in fact > redundant since it's already done by intel_modeset_readout_hw_state() > setting the encoder's crtc or leaving it NULL accordingly. Let's use > this fact to decide if the encoder is active. > > While at it clarify the comment in intel_ddi_get_power_domains() about > primary vs. fake MST encoders and make sure we never do an incorrect > encoder->dig_port cast for fake MST encoders. > > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Signed-off-by: Imre Deak <imre.deak@intel.com> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_ddi.c | 19 +++++++------------ > drivers/gpu/drm/i915/intel_display.c | 11 +++++------ > 2 files changed, 12 insertions(+), 18 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > index 0319825b725b..6b74eee5a3ac 100644 > --- a/drivers/gpu/drm/i915/intel_ddi.c > +++ b/drivers/gpu/drm/i915/intel_ddi.c > @@ -2005,24 +2005,19 @@ intel_ddi_main_link_aux_domain(struct intel_dp *intel_dp) > static u64 intel_ddi_get_power_domains(struct intel_encoder *encoder, > struct intel_crtc_state *crtc_state) > { > - struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base); > - enum pipe pipe; > + struct intel_digital_port *dig_port; > u64 domains; > > - if (!intel_ddi_get_hw_state(encoder, &pipe)) > - return 0; > - > - domains = BIT_ULL(dig_port->ddi_io_power_domain); > - if (!crtc_state) > - return domains; > - > /* > * TODO: Add support for MST encoders. Atm, the following should never > - * happen since this function will be called only for the primary > - * encoder which doesn't have its own pipe/crtc_state. > + * happen since fake-MST encoders don't set their get_power_domains() > + * hook. > */ > if (WARN_ON(intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST))) > - return domains; > + return 0; > + > + dig_port = enc_to_dig_port(&encoder->base); > + domains = BIT_ULL(dig_port->ddi_io_power_domain); > > /* AUX power is only needed for (e)DP mode, not for HDMI. */ > if (intel_crtc_has_dp_encoder(crtc_state)) { > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 681e0710a467..a61dbdf2f612 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -15705,14 +15705,13 @@ get_encoder_power_domains(struct drm_i915_private *dev_priv) > continue; > > /* > - * For MST and inactive encoders we don't have a crtc state. > - * FIXME: no need to call get_power_domains in such cases, it > - * will always return 0. > + * MST-primary and inactive encoders don't have a crtc state > + * and neither of these require any power domain references. > */ > - crtc_state = encoder->base.crtc ? > - to_intel_crtc_state(encoder->base.crtc->state) : > - NULL; > + if (!encoder->base.crtc) > + continue; > > + crtc_state = to_intel_crtc_state(encoder->base.crtc->state); > get_domains = encoder->get_power_domains(encoder, crtc_state); > for_each_power_domain(domain, get_domains) > intel_display_power_get(dev_priv, domain); > -- > 2.13.2
On Thu, Jul 05, 2018 at 04:33:50PM +0000, Patchwork wrote: > == Series Details == > > Series: drm/i915/ddi: Simplify get_encoder_power_domains() > URL : https://patchwork.freedesktop.org/series/45980/ > State : success Pushed to -dinq, thanks for the review. > > == Summary == > > = CI Bug Log - changes from CI_DRM_4431_full -> Patchwork_9537_full = > > == Summary - WARNING == > > Minor unknown changes coming with Patchwork_9537_full need to be verified > manually. > > If you think the reported changes have nothing to do with the changes > introduced in Patchwork_9537_full, please notify your bug team to allow them > to document this new failure mode, which will reduce false positives in CI. > > > > == Possible new issues == > > Here are the unknown changes that may have been introduced in Patchwork_9537_full: > > === IGT changes === > > ==== Warnings ==== > > igt@gem_exec_schedule@deep-bsd2: > shard-kbl: PASS -> SKIP +2 > > igt@gem_mocs_settings@mocs-rc6-vebox: > shard-kbl: SKIP -> PASS +2 > > > == Known issues == > > Here are the changes found in Patchwork_9537_full that come from known issues: > > === IGT changes === > > ==== Issues hit ==== > > igt@drv_selftest@live_gtt: > shard-glk: PASS -> FAIL (fdo#107127, fdo#105347) > > igt@gem_softpin@noreloc-s3: > shard-glk: PASS -> FAIL (fdo#103375) > > igt@kms_cursor_legacy@cursora-vs-flipb-toggle: > shard-glk: PASS -> FAIL (fdo#106765) > > igt@kms_flip@2x-plain-flip-ts-check: > shard-glk: PASS -> FAIL (fdo#100368) > > igt@kms_flip_tiling@flip-to-x-tiled: > shard-glk: PASS -> FAIL (fdo#103822) +1 > > igt@kms_setmode@basic: > shard-kbl: PASS -> FAIL (fdo#99912) > > igt@perf_pmu@other-read-4: > shard-snb: PASS -> INCOMPLETE (fdo#105411) > > > ==== Possible fixes ==== > > igt@drv_suspend@shrink: > shard-kbl: INCOMPLETE (fdo#103665, fdo#106886) -> PASS > > igt@kms_chv_cursor_fail@pipe-a-64x64-left-edge: > shard-kbl: FAIL (fdo#104671) -> PASS > > igt@kms_cursor_legacy@2x-long-cursor-vs-flip-legacy: > shard-hsw: FAIL (fdo#105767) -> PASS > > igt@kms_flip_tiling@flip-to-y-tiled: > shard-glk: FAIL -> PASS > > igt@kms_flip_tiling@flip-x-tiled: > shard-glk: FAIL (fdo#103822) -> PASS > > igt@kms_setmode@basic: > shard-apl: FAIL (fdo#99912) -> PASS > > igt@perf@rc6-disable: > shard-kbl: FAIL (fdo#103179) -> PASS > > > ==== Warnings ==== > > igt@drv_selftest@live_gtt: > shard-apl: INCOMPLETE (fdo#103927) -> FAIL (fdo#107127, fdo#105347) > > > fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368 > fdo#103179 https://bugs.freedesktop.org/show_bug.cgi?id=103179 > fdo#103375 https://bugs.freedesktop.org/show_bug.cgi?id=103375 > fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665 > fdo#103822 https://bugs.freedesktop.org/show_bug.cgi?id=103822 > fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927 > fdo#104671 https://bugs.freedesktop.org/show_bug.cgi?id=104671 > fdo#105347 https://bugs.freedesktop.org/show_bug.cgi?id=105347 > fdo#105411 https://bugs.freedesktop.org/show_bug.cgi?id=105411 > fdo#105767 https://bugs.freedesktop.org/show_bug.cgi?id=105767 > fdo#106765 https://bugs.freedesktop.org/show_bug.cgi?id=106765 > fdo#106886 https://bugs.freedesktop.org/show_bug.cgi?id=106886 > fdo#107127 https://bugs.freedesktop.org/show_bug.cgi?id=107127 > fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912 > > > == Participating hosts (5 -> 5) == > > No changes in participating hosts > > > == Build changes == > > * Linux: CI_DRM_4431 -> Patchwork_9537 > > CI_DRM_4431: b9bf725e2d248638a834b4b4db5b684098216b86 @ git://anongit.freedesktop.org/gfx-ci/linux > IGT_4537: 5a160e9e1fe19c67e58e9c298303cb94c96aeb7d @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools > Patchwork_9537: a91699cb337092c140ca4433445fa66e48d93ded @ git://anongit.freedesktop.org/gfx-ci/linux > piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit > > == Logs == > > For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9537/shards.html
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c index 0319825b725b..6b74eee5a3ac 100644 --- a/drivers/gpu/drm/i915/intel_ddi.c +++ b/drivers/gpu/drm/i915/intel_ddi.c @@ -2005,24 +2005,19 @@ intel_ddi_main_link_aux_domain(struct intel_dp *intel_dp) static u64 intel_ddi_get_power_domains(struct intel_encoder *encoder, struct intel_crtc_state *crtc_state) { - struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base); - enum pipe pipe; + struct intel_digital_port *dig_port; u64 domains; - if (!intel_ddi_get_hw_state(encoder, &pipe)) - return 0; - - domains = BIT_ULL(dig_port->ddi_io_power_domain); - if (!crtc_state) - return domains; - /* * TODO: Add support for MST encoders. Atm, the following should never - * happen since this function will be called only for the primary - * encoder which doesn't have its own pipe/crtc_state. + * happen since fake-MST encoders don't set their get_power_domains() + * hook. */ if (WARN_ON(intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST))) - return domains; + return 0; + + dig_port = enc_to_dig_port(&encoder->base); + domains = BIT_ULL(dig_port->ddi_io_power_domain); /* AUX power is only needed for (e)DP mode, not for HDMI. */ if (intel_crtc_has_dp_encoder(crtc_state)) { diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 681e0710a467..a61dbdf2f612 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -15705,14 +15705,13 @@ get_encoder_power_domains(struct drm_i915_private *dev_priv) continue; /* - * For MST and inactive encoders we don't have a crtc state. - * FIXME: no need to call get_power_domains in such cases, it - * will always return 0. + * MST-primary and inactive encoders don't have a crtc state + * and neither of these require any power domain references. */ - crtc_state = encoder->base.crtc ? - to_intel_crtc_state(encoder->base.crtc->state) : - NULL; + if (!encoder->base.crtc) + continue; + crtc_state = to_intel_crtc_state(encoder->base.crtc->state); get_domains = encoder->get_power_domains(encoder, crtc_state); for_each_power_domain(domain, get_domains) intel_display_power_get(dev_priv, domain);
We can simplify the encoder's get_power_domains() hook by calling it only if the encoder is active. That way the hook can return its power domains unconditionally without checking the active state by calling encoder::get_hw_state(). This get_hw_state() query is in fact redundant since it's already done by intel_modeset_readout_hw_state() setting the encoder's crtc or leaving it NULL accordingly. Let's use this fact to decide if the encoder is active. While at it clarify the comment in intel_ddi_get_power_domains() about primary vs. fake MST encoders and make sure we never do an incorrect encoder->dig_port cast for fake MST encoders. Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Signed-off-by: Imre Deak <imre.deak@intel.com> --- drivers/gpu/drm/i915/intel_ddi.c | 19 +++++++------------ drivers/gpu/drm/i915/intel_display.c | 11 +++++------ 2 files changed, 12 insertions(+), 18 deletions(-)