| Message ID | 20250124191250.56833-1-gustavo.sousa@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm/i915/dmc_wl: Do not check for DMC payload | expand |
Quoting Patchwork (2025-01-24 18:22:41-03:00) >== Series Details == > >Series: drm/i915/dmc_wl: Do not check for DMC payload >URL : https://patchwork.freedesktop.org/series/143951/ >State : failure > >== Summary == > >CI Bug Log - changes from CI_DRM_16018 -> Patchwork_143951v1 >==================================================== > >Summary >------- > > **FAILURE** > > Serious unknown changes coming with Patchwork_143951v1 absolutely need to be > verified manually. > > If you think the reported changes have nothing to do with the changes > introduced in Patchwork_143951v1, please notify your bug team (I915-ci-infra@lists.freedesktop.org) to allow them > to document this new failure mode, which will reduce false positives in CI. > > External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_143951v1/index.html > >Participating hosts (38 -> 37) >------------------------------ > > Missing (1): fi-snb-2520m > >Possible new issues >------------------- > > Here are the unknown changes that may have been introduced in Patchwork_143951v1: > >### IGT changes ### > >#### Possible regressions #### > > * igt@kms_pm_rpm@basic-rte: > - bat-apl-1: [PASS][1] -> [DMESG-WARN][2] > [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_16018/bat-apl-1/igt@kms_pm_rpm@basic-rte.html > [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_143951v1/bat-apl-1/igt@kms_pm_rpm@basic-rte.html Hi CI Team, DMC wakelock is not exercised for this platform. This should not be related to this patch. Could you please re-report? -- Gustavo Sousa
Hi, https://patchwork.freedesktop.org/series/143951/ - Re-reported. i915.CI.BAT - Re-reported. Thanks, Ravali. -----Original Message----- From: I915-ci-infra <i915-ci-infra-bounces@lists.freedesktop.org> On Behalf Of Gustavo Sousa Sent: 27 January 2025 19:01 To: I915-ci-infra@lists.freedesktop.org; intel-gfx@lists.freedesktop.org Cc: intel-gfx@lists.freedesktop.org Subject: Re: ✗ i915.CI.BAT: failure for drm/i915/dmc_wl: Do not check for DMC payload Quoting Patchwork (2025-01-24 18:22:41-03:00) >== Series Details == > >Series: drm/i915/dmc_wl: Do not check for DMC payload >URL : https://patchwork.freedesktop.org/series/143951/ >State : failure > >== Summary == > >CI Bug Log - changes from CI_DRM_16018 -> Patchwork_143951v1 >==================================================== > >Summary >------- > > **FAILURE** > > Serious unknown changes coming with Patchwork_143951v1 absolutely > need to be verified manually. > > If you think the reported changes have nothing to do with the changes > introduced in Patchwork_143951v1, please notify your bug team > (I915-ci-infra@lists.freedesktop.org) to allow them to document this new failure mode, which will reduce false positives in CI. > > External URL: > https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_143951v1/index.html > >Participating hosts (38 -> 37) >------------------------------ > > Missing (1): fi-snb-2520m > >Possible new issues >------------------- > > Here are the unknown changes that may have been introduced in Patchwork_143951v1: > >### IGT changes ### > >#### Possible regressions #### > > * igt@kms_pm_rpm@basic-rte: > - bat-apl-1: [PASS][1] -> [DMESG-WARN][2] > [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_16018/bat-apl-1/igt@kms_pm_rpm@basic-rte.html > [2]: > https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_143951v1/bat-apl-1/ > igt@kms_pm_rpm@basic-rte.html Hi CI Team, DMC wakelock is not exercised for this platform. This should not be related to this patch. Could you please re-report? -- Gustavo Sousa
On Fri, 2025-01-24 at 16:12 -0300, Gustavo Sousa wrote: > Enabling and disabling of DMC wakelock is already coupled with enabling > and disabling of dynamic DC states, which already depend on the DMC > being properly loaded. As such, we do not need to check if we already > have a DMC payload parsed in __intel_dmc_wl_supported(). > > Furthermore, the presence of such a check causes inconsistencies in the > refcount if the following sequence of events happen: > > 1. A call to one of the register accessors from intel_de.h is done > before the DMC payload is parsed. That causes intel_dmc_wl_get() to > be called. Suppose the register offset qualifies as needing the > wakelock. > > In normal circumstances, the refcount would be incremented, but, > because __intel_dmc_wl_supported() returns false, the refcount is > untouched. > > 2. In a separate worker thread, the DMC firmware is parsed. Parsing of > the DMC payload is finished before the corresponding > intel_dmc_wl_put() from (1) is called. > > 3. When in the context of (1), intel_dmc_wl_put() gets called, now we > have __intel_dmc_wl_supported() returning true and we hit the > warning, because the code doesn't expect a zero refcount. > > Let's remove that check, since it is unnecessary and causes the > inconsistency illustrated above. > > Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com> > --- Reviewed-by: Luca Coelho <luciano.coelho@intel.com> -- Cheers, Luca.
Hi Gustavo, [...] > Let's remove that check, since it is unnecessary and causes the > inconsistency illustrated above. > > Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com> > --- Would you add: Fixes: c01e78a96e12 ("drm/i915/dmc_wl: Sanitize enable_dmc_wl according to hardware support")? after that LGTM: Reviewed-by: Krzysztof Karas <krzysztof.karas@intel.com> Krzysztof
Quoting Krzysztof Karas (2025-01-30 11:18:28-03:00) >Hi Gustavo, > >[...] >> Let's remove that check, since it is unnecessary and causes the >> inconsistency illustrated above. >> >> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com> >> --- > >Would you add: >Fixes: c01e78a96e12 ("drm/i915/dmc_wl: Sanitize enable_dmc_wl according to hardware support")? I believe the above commit should not be a "fixes" candidate, the check for intel_dmc_has_payload() was already there when that commit was applied. Also, do we need a "Fixes:" tag here? There only platform where the DMC Wakelock mechanism is enabled by default is PTL, which is still under force_probe protection. -- Gustavo Sousa > >after that LGTM: >Reviewed-by: Krzysztof Karas <krzysztof.karas@intel.com> > >Krzysztof
On Fri, Jan 31, 2025 at 05:16:29PM -0300, Gustavo Sousa wrote: >Quoting Krzysztof Karas (2025-01-30 11:18:28-03:00) >>Hi Gustavo, >> >>[...] >>> Let's remove that check, since it is unnecessary and causes the >>> inconsistency illustrated above. >>> >>> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com> >>> --- >> >>Would you add: >>Fixes: c01e78a96e12 ("drm/i915/dmc_wl: Sanitize enable_dmc_wl according to hardware support")? > >I believe the above commit should not be a "fixes" candidate, the check >for intel_dmc_has_payload() was already there when that commit was >applied. > >Also, do we need a "Fixes:" tag here? There only platform where the DMC >Wakelock mechanism is enabled by default is PTL, which is still under >force_probe protection. it's not user visible, so no. Sometimes we may want to backport fixes for platforms that may be used with previous kernel versions, but that is mainly when we are close to remove the force_probe. Lucas De Marchi > >-- >Gustavo Sousa > >> >>after that LGTM: >>Reviewed-by: Krzysztof Karas <krzysztof.karas@intel.com> >> >>Krzysztof
Quoting Lucas De Marchi (2025-01-31 20:49:35-03:00) >On Fri, Jan 31, 2025 at 05:16:29PM -0300, Gustavo Sousa wrote: >>Quoting Krzysztof Karas (2025-01-30 11:18:28-03:00) >>>Hi Gustavo, >>> >>>[...] >>>> Let's remove that check, since it is unnecessary and causes the >>>> inconsistency illustrated above. >>>> >>>> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com> >>>> --- >>> >>>Would you add: >>>Fixes: c01e78a96e12 ("drm/i915/dmc_wl: Sanitize enable_dmc_wl according to hardware support")? >> >>I believe the above commit should not be a "fixes" candidate, the check >>for intel_dmc_has_payload() was already there when that commit was >>applied. >> >>Also, do we need a "Fixes:" tag here? There only platform where the DMC >>Wakelock mechanism is enabled by default is PTL, which is still under >>force_probe protection. > >it's not user visible, so no. Sometimes we may want to backport fixes >for platforms that may be used with previous kernel versions, but that >is mainly when we are close to remove the force_probe. Okay. Thanks! Krzysztof, does the r-b stand without the "Fixes" tag? -- Gustavo Sousa > >Lucas De Marchi > >> >>-- >>Gustavo Sousa >> >>> >>>after that LGTM: >>>Reviewed-by: Krzysztof Karas <krzysztof.karas@intel.com> >>> >>>Krzysztof
> Okay. Thanks! > > Krzysztof, does the r-b stand without the "Fixes" tag? > Yup, sorry for the late response: Reviewed-by: Krzysztof Karas <krzysztof.karas@intel.com> Krzysztof
Quoting Patchwork (2025-01-30 11:50:41-03:00) >== Series Details == > >Series: drm/i915/dmc_wl: Do not check for DMC payload (rev2) >URL : https://patchwork.freedesktop.org/series/143951/ >State : failure > >== Summary == > >CI Bug Log - changes from CI_DRM_16041_full -> Patchwork_143951v2_full >==================================================== > >Summary >------- > > **FAILURE** > > Serious unknown changes coming with Patchwork_143951v2_full absolutely need to be > verified manually. > > If you think the reported changes have nothing to do with the changes > introduced in Patchwork_143951v2_full, please notify your bug team (I915-ci-infra@lists.freedesktop.org) to allow them > to document this new failure mode, which will reduce false positives in CI. > > External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_143951v2/index.html > >Participating hosts (11 -> 11) >------------------------------ > > No changes in participating hosts > >Possible new issues >------------------- > > Here are the unknown changes that may have been introduced in Patchwork_143951v2_full: > >### IGT changes ### > >#### Possible regressions #### None of the failures below are related to this patch, since only PTL effectively uses the DMC wakelock. > * igt@gem_exec_gttfill@engines: > - shard-dg2: [PASS][1] -> [FAIL][2] +1 other test fail > [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_16041/shard-dg2-5/igt@gem_exec_gttfill@engines.html > [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_143951v2/shard-dg2-3/igt@gem_exec_gttfill@engines.html This maps to https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/13546 . > > * igt@perf_pmu@module-unload: > - shard-tglu: [PASS][3] -> [INCOMPLETE][4] > [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_16041/shard-tglu-5/igt@perf_pmu@module-unload.html > [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_143951v2/shard-tglu-9/igt@perf_pmu@module-unload.html This possibly maps to https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/13604 . > > * igt@sysfs_heartbeat_interval@nopreempt@vcs1: > - shard-mtlp: [PASS][5] -> [DMESG-WARN][6] > [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_16041/shard-mtlp-1/igt@sysfs_heartbeat_interval@nopreempt@vcs1.html > [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_143951v2/shard-mtlp-7/igt@sysfs_heartbeat_interval@nopreempt@vcs1.html This maps to https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/13193 . -- Gustavo Sousa
Quoting Gustavo Sousa (2025-01-24 16:12:34-03:00) >Enabling and disabling of DMC wakelock is already coupled with enabling >and disabling of dynamic DC states, which already depend on the DMC >being properly loaded. As such, we do not need to check if we already >have a DMC payload parsed in __intel_dmc_wl_supported(). > >Furthermore, the presence of such a check causes inconsistencies in the >refcount if the following sequence of events happen: > > 1. A call to one of the register accessors from intel_de.h is done > before the DMC payload is parsed. That causes intel_dmc_wl_get() to > be called. Suppose the register offset qualifies as needing the > wakelock. > > In normal circumstances, the refcount would be incremented, but, > because __intel_dmc_wl_supported() returns false, the refcount is > untouched. > > 2. In a separate worker thread, the DMC firmware is parsed. Parsing of > the DMC payload is finished before the corresponding > intel_dmc_wl_put() from (1) is called. > > 3. When in the context of (1), intel_dmc_wl_put() gets called, now we > have __intel_dmc_wl_supported() returning true and we hit the > warning, because the code doesn't expect a zero refcount. > >Let's remove that check, since it is unnecessary and causes the >inconsistency illustrated above. > >Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com> Pushed to drm-intel-next. Thank you all for the reviews/feedback! -- Gustavo Sousa >--- > drivers/gpu/drm/i915/display/intel_dmc_wl.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > >diff --git a/drivers/gpu/drm/i915/display/intel_dmc_wl.c b/drivers/gpu/drm/i915/display/intel_dmc_wl.c >index 43884740f8ea..9be6ad11ff5d 100644 >--- a/drivers/gpu/drm/i915/display/intel_dmc_wl.c >+++ b/drivers/gpu/drm/i915/display/intel_dmc_wl.c >@@ -10,7 +10,6 @@ > #include "i915_drv.h" > #include "i915_reg.h" > #include "intel_de.h" >-#include "intel_dmc.h" > #include "intel_dmc_regs.h" > #include "intel_dmc_wl.h" > >@@ -282,7 +281,7 @@ static bool intel_dmc_wl_check_range(struct intel_display *display, > > static bool __intel_dmc_wl_supported(struct intel_display *display) > { >- return display->params.enable_dmc_wl && intel_dmc_has_payload(display); >+ return display->params.enable_dmc_wl; > } > > static void intel_dmc_wl_sanitize_param(struct intel_display *display) >-- >2.48.1 >
diff --git a/drivers/gpu/drm/i915/display/intel_dmc_wl.c b/drivers/gpu/drm/i915/display/intel_dmc_wl.c index 43884740f8ea..9be6ad11ff5d 100644 --- a/drivers/gpu/drm/i915/display/intel_dmc_wl.c +++ b/drivers/gpu/drm/i915/display/intel_dmc_wl.c @@ -10,7 +10,6 @@ #include "i915_drv.h" #include "i915_reg.h" #include "intel_de.h" -#include "intel_dmc.h" #include "intel_dmc_regs.h" #include "intel_dmc_wl.h" @@ -282,7 +281,7 @@ static bool intel_dmc_wl_check_range(struct intel_display *display, static bool __intel_dmc_wl_supported(struct intel_display *display) { - return display->params.enable_dmc_wl && intel_dmc_has_payload(display); + return display->params.enable_dmc_wl; } static void intel_dmc_wl_sanitize_param(struct intel_display *display)
Enabling and disabling of DMC wakelock is already coupled with enabling and disabling of dynamic DC states, which already depend on the DMC being properly loaded. As such, we do not need to check if we already have a DMC payload parsed in __intel_dmc_wl_supported(). Furthermore, the presence of such a check causes inconsistencies in the refcount if the following sequence of events happen: 1. A call to one of the register accessors from intel_de.h is done before the DMC payload is parsed. That causes intel_dmc_wl_get() to be called. Suppose the register offset qualifies as needing the wakelock. In normal circumstances, the refcount would be incremented, but, because __intel_dmc_wl_supported() returns false, the refcount is untouched. 2. In a separate worker thread, the DMC firmware is parsed. Parsing of the DMC payload is finished before the corresponding intel_dmc_wl_put() from (1) is called. 3. When in the context of (1), intel_dmc_wl_put() gets called, now we have __intel_dmc_wl_supported() returning true and we hit the warning, because the code doesn't expect a zero refcount. Let's remove that check, since it is unnecessary and causes the inconsistency illustrated above. Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com> --- drivers/gpu/drm/i915/display/intel_dmc_wl.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)