On 2020-12-04 at 17:51:34 +0200, Ville Syrjälä wrote:
> On Fri, Dec 04, 2020 at 01:40:03PM +0530, Anshuman Gupta wrote:
> > On 2020-11-30 at 17:28:32 +0200, Imre Deak wrote:
> > > On Mon, Nov 30, 2020 at 02:46:46PM +0530, Anshuman Gupta wrote:
> > > > At usual case DC3CO exit happen automatically by DMC f/w whenever
> > > > PSR2 clears idle. This happens smoothly by DMC f/w to work with flips.
> > > > But there are certain scenario where DC3CO Disallowed by driver
> > > > asynchronous with flips. In such scenario display engine could
> > > > be already in DC3CO state and driver has disallowed it,
> > > > It initiates DC3CO exit sequence in DMC f/w which requires a
> > > > dc3co exit delay of 200us in driver.
> > > > It requires to protect intel_pipe_update_{update_end} with
> > > > dc3co exit delay.
> > > >
> > > > Cc: Imre Deak <imre.deak@xxxxxxxxx>
> > > > Cc: <stable@xxxxxxxxxxxxxxx>
> > > > Signed-off-by: Anshuman Gupta <anshuman.gupta@xxxxxxxxx>
> > >
> > > To make sure that it doesn't hide the root cause (or affects unrelated
> > > platforms), I'd only add locking around DC3co changes with a new lock,
> > > using lock/unlock helpers in intel_display_power.c called from
> > > intel_pipe_update_start/end.
> > >
> > > Also please submit this patch separately, w/o the optimization in patch
> > > 1/2, so we know that this change fixes the problem.
> > This patch doesn't seems to fix the issue.
> > Looks like there is some other set of display register updates before
> > completing the dc3co exit delay beyond intel_pipe_update_start/end causing this issue.
>
> Not really sure I understand the DC3CO issue here, nor how grabbing a
> mutex across the update could help.
Thanks Ville for providing your input here, the display glitches is fixed by
https://patchwork.freedesktop.org/patch/405585/?series=84394&rev=2 patch in case
of brightness being updated simultaneously with flips, so it was our wild guess
that if intel_pipe_update_start triggers before completing DC3CO exit delay in
tgl_disable_dc3co could cause the display glitches but that was not true.
>
> But anyways, maybe we should just:
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 2e2dd746921f..96276f0feddc 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -16268,8 +16268,7 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state)
>
> drm_atomic_helper_wait_for_dependencies(&state->base);
>
> - if (state->modeset)
> - wakeref = intel_display_power_get(dev_priv, POWER_DOMAIN_MODESET);
> + wakeref = intel_display_power_get(dev_priv, POWER_DOMAIN_MODESET);
Certainly this should fix the issue. I will try this out but i feel this
could cause heavy lock contention around power_domains->lock in case
brightness being updated rapidly as the scenario of this issue.
We would also need https://patchwork.freedesktop.org/patch/405585/?series=84394&rev=2 patch as well ?
Thanks,
Anshuman Gupta.
>
> for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> new_crtc_state, i) {
> @@ -16415,8 +16414,8 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state)
> * the culprit.
> */
> intel_uncore_arm_unclaimed_mmio_detection(&dev_priv->uncore);
> - intel_display_power_put(dev_priv, POWER_DOMAIN_MODESET, wakeref);
> }
> + intel_display_power_put(dev_priv, POWER_DOMAIN_MODESET, wakeref);
> intel_runtime_pm_put(&dev_priv->runtime_pm, state->wakeref);
>
> /*
>
> To get the DMC out of equation entirely for all plane updates?
>
> --
> Ville Syrjälä
> Intel
