Re: [PATCH 2/3] drm/xe: Don't rely on indirect includes from xe_mmio.h
On Tue, May 21, 2024 at 04:10:25PM +0200, Michal Wajdeczko wrote:
>
>
> On 21.05.2024 16:01, Francois Dugast wrote:
> > Hi Michal,
> >
> > On Mon, May 20, 2024 at 08:18:13PM +0200, Michal Wajdeczko wrote:
> >> These compilation units use udelay() or some GT oriented printk
> >> functions without explicitly including proper header files, and
> >> relying on #includes from the xe_mmio.h instead. Fix that.
> >>
> >> Signed-off-by: Michal Wajdeczko
> >> ---
> >> drivers/gpu/drm/xe/xe_device.c | 2 ++
> >> drivers/gpu/drm/xe/xe_gsc.c| 2 ++
> >> drivers/gpu/drm/xe/xe_gt_ccs_mode.c| 1 +
> >> drivers/gpu/drm/xe/xe_guc_ads.c| 1 +
> >> drivers/gpu/drm/xe/xe_huc.c| 2 ++
> >> drivers/gpu/drm/xe/xe_mocs.c | 1 +
> >> drivers/gpu/drm/xe/xe_ttm_stolen_mgr.c | 1 +
> >> drivers/gpu/drm/xe/xe_uc_fw.c | 1 +
> >> 8 files changed, 11 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/xe/xe_device.c
> >> b/drivers/gpu/drm/xe/xe_device.c
> >> index 8da90934c900..28a4e0c3b1fe 100644
> >> --- a/drivers/gpu/drm/xe/xe_device.c
> >> +++ b/drivers/gpu/drm/xe/xe_device.c
> >> @@ -5,6 +5,7 @@
> >>
> >> #include "xe_device.h"
> >>
> >> +#include
> >> #include
> >>
> >> #include
> >> @@ -33,6 +34,7 @@
> >> #include "xe_gsc_proxy.h"
> >> #include "xe_gt.h"
> >> #include "xe_gt_mcr.h"
> >> +#include "xe_gt_printk.h"
> >
> > It is obvious in the occurrences of this include in other compilation
> > units below, but in xe_device.c I am not seeing the need for
> > xe_gt_printk.h, am I missing something?
>
> void xe_device_td_flush(struct xe_device *xe)
> ...
> xe_gt_err_once(gt, "TD flush timeout\n");
Indeed, I was looking at drm-xe-next and was missing
commit c01c6066e6fa6 ("drm/xe/device: implement transient flush").
Reviewed-by: Francois Dugast
>
> >
> > Francois
> >
> >> #include "xe_hwmon.h"
> >> #include "xe_irq.h"
> >> #include "xe_memirq.h"
> >> diff --git a/drivers/gpu/drm/xe/xe_gsc.c b/drivers/gpu/drm/xe/xe_gsc.c
> >> index 8cc6420a9e7f..80a61934decc 100644
> >> --- a/drivers/gpu/drm/xe/xe_gsc.c
> >> +++ b/drivers/gpu/drm/xe/xe_gsc.c
> >> @@ -5,6 +5,8 @@
> >>
> >> #include "xe_gsc.h"
> >>
> >> +#include
> >> +
> >> #include
> >>
> >> #include
> >> diff --git a/drivers/gpu/drm/xe/xe_gt_ccs_mode.c
> >> b/drivers/gpu/drm/xe/xe_gt_ccs_mode.c
> >> index a34c9a24dafc..f90cf679c5d7 100644
> >> --- a/drivers/gpu/drm/xe/xe_gt_ccs_mode.c
> >> +++ b/drivers/gpu/drm/xe/xe_gt_ccs_mode.c
> >> @@ -9,6 +9,7 @@
> >> #include "xe_assert.h"
> >> #include "xe_gt.h"
> >> #include "xe_gt_ccs_mode.h"
> >> +#include "xe_gt_printk.h"
> >> #include "xe_gt_sysfs.h"
> >> #include "xe_mmio.h"
> >>
> >> diff --git a/drivers/gpu/drm/xe/xe_guc_ads.c
> >> b/drivers/gpu/drm/xe/xe_guc_ads.c
> >> index 9c33cca4e370..1c60b685dbc6 100644
> >> --- a/drivers/gpu/drm/xe/xe_guc_ads.c
> >> +++ b/drivers/gpu/drm/xe/xe_guc_ads.c
> >> @@ -16,6 +16,7 @@
> >> #include "xe_bo.h"
> >> #include "xe_gt.h"
> >> #include "xe_gt_ccs_mode.h"
> >> +#include "xe_gt_printk.h"
> >> #include "xe_guc.h"
> >> #include "xe_guc_ct.h"
> >> #include "xe_hw_engine.h"
> >> diff --git a/drivers/gpu/drm/xe/xe_huc.c b/drivers/gpu/drm/xe/xe_huc.c
> >> index 39a484a57585..b039ff49341b 100644
> >> --- a/drivers/gpu/drm/xe/xe_huc.c
> >> +++ b/drivers/gpu/drm/xe/xe_huc.c
> >> @@ -5,6 +5,8 @@
> >>
> >> #include "xe_huc.h"
> >>
> >> +#include
> >> +
> >> #include
> >>
> >> #include "abi/gsc_pxp_commands_abi.h"
> >> diff --git a/drivers/gpu/drm/xe/xe_mocs.c b/drivers/gpu/drm/xe/xe_mocs.c
> >> index f04754ad911b..de3f2d3f1b04 100644
> >> --- a/drivers/gpu/drm/xe/xe_mocs.c
> >> +++ b/drivers/gpu/drm/xe/xe_mocs.c
> >> @@ -12,6 +12,7 @@
> >> #include "xe_force_wake.h"
> >> #include "xe_gt.h"
> >> #include "xe_gt_mcr.h"
> >> +#include "xe_gt_printk.h"
> >> #include "xe_mmio.h"
> >> #include "xe_platform_types.h"
> >> #include "xe_pm.h"
> >> diff --git a/drivers/gpu/drm/xe/xe_ttm_stolen_mgr.c
> >> b/drivers/gpu/drm/xe/xe_ttm_stolen_mgr.c
> >> index f77367329760..64592a8e527b 100644
> >> --- a/drivers/gpu/drm/xe/xe_ttm_stolen_mgr.c
> >> +++ b/drivers/gpu/drm/xe/xe_ttm_stolen_mgr.c
> >> @@ -18,6 +18,7 @@
> >> #include "xe_bo.h"
> >> #include "xe_device.h"
> >> #include "xe_gt.h"
> >> +#include "xe_gt_printk.h"
> >> #include "xe_mmio.h"
> >> #include "xe_res_cursor.h"
> >> #include "xe_sriov.h"
> >> diff --git a/drivers/gpu/drm/xe/xe_uc_fw.c b/drivers/gpu/drm/xe/xe_uc_fw.c
> >> index ed819f1df888..12346645a8e5 100644
> >> --- a/drivers/gpu/drm/xe/xe_uc_fw.c
> >> +++ b/drivers/gpu/drm/xe/xe_uc_fw.c
> >> @@ -14,6 +14,7 @@
> >> #include "xe_force_wake.h"
> >> #include "xe_gsc.h"
> >> #include "xe_gt.h"
> >> +#include "xe_gt_printk.h"
> >> #include "xe_map.h"
> >> #include "xe_mmio.h"
> >> #include "xe_module.h"
> >> --
> >> 2.43.0
> >>
Re: [PATCH 2/3] drm/xe: Don't rely on indirect includes from xe_mmio.h
Hi Michal, On Mon, May 20, 2024 at 08:18:13PM +0200, Michal Wajdeczko wrote: > These compilation units use udelay() or some GT oriented printk > functions without explicitly including proper header files, and > relying on #includes from the xe_mmio.h instead. Fix that. > > Signed-off-by: Michal Wajdeczko > --- > drivers/gpu/drm/xe/xe_device.c | 2 ++ > drivers/gpu/drm/xe/xe_gsc.c| 2 ++ > drivers/gpu/drm/xe/xe_gt_ccs_mode.c| 1 + > drivers/gpu/drm/xe/xe_guc_ads.c| 1 + > drivers/gpu/drm/xe/xe_huc.c| 2 ++ > drivers/gpu/drm/xe/xe_mocs.c | 1 + > drivers/gpu/drm/xe/xe_ttm_stolen_mgr.c | 1 + > drivers/gpu/drm/xe/xe_uc_fw.c | 1 + > 8 files changed, 11 insertions(+) > > diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c > index 8da90934c900..28a4e0c3b1fe 100644 > --- a/drivers/gpu/drm/xe/xe_device.c > +++ b/drivers/gpu/drm/xe/xe_device.c > @@ -5,6 +5,7 @@ > > #include "xe_device.h" > > +#include > #include > > #include > @@ -33,6 +34,7 @@ > #include "xe_gsc_proxy.h" > #include "xe_gt.h" > #include "xe_gt_mcr.h" > +#include "xe_gt_printk.h" It is obvious in the occurrences of this include in other compilation units below, but in xe_device.c I am not seeing the need for xe_gt_printk.h, am I missing something? Francois > #include "xe_hwmon.h" > #include "xe_irq.h" > #include "xe_memirq.h" > diff --git a/drivers/gpu/drm/xe/xe_gsc.c b/drivers/gpu/drm/xe/xe_gsc.c > index 8cc6420a9e7f..80a61934decc 100644 > --- a/drivers/gpu/drm/xe/xe_gsc.c > +++ b/drivers/gpu/drm/xe/xe_gsc.c > @@ -5,6 +5,8 @@ > > #include "xe_gsc.h" > > +#include > + > #include > > #include > diff --git a/drivers/gpu/drm/xe/xe_gt_ccs_mode.c > b/drivers/gpu/drm/xe/xe_gt_ccs_mode.c > index a34c9a24dafc..f90cf679c5d7 100644 > --- a/drivers/gpu/drm/xe/xe_gt_ccs_mode.c > +++ b/drivers/gpu/drm/xe/xe_gt_ccs_mode.c > @@ -9,6 +9,7 @@ > #include "xe_assert.h" > #include "xe_gt.h" > #include "xe_gt_ccs_mode.h" > +#include "xe_gt_printk.h" > #include "xe_gt_sysfs.h" > #include "xe_mmio.h" > > diff --git a/drivers/gpu/drm/xe/xe_guc_ads.c b/drivers/gpu/drm/xe/xe_guc_ads.c > index 9c33cca4e370..1c60b685dbc6 100644 > --- a/drivers/gpu/drm/xe/xe_guc_ads.c > +++ b/drivers/gpu/drm/xe/xe_guc_ads.c > @@ -16,6 +16,7 @@ > #include "xe_bo.h" > #include "xe_gt.h" > #include "xe_gt_ccs_mode.h" > +#include "xe_gt_printk.h" > #include "xe_guc.h" > #include "xe_guc_ct.h" > #include "xe_hw_engine.h" > diff --git a/drivers/gpu/drm/xe/xe_huc.c b/drivers/gpu/drm/xe/xe_huc.c > index 39a484a57585..b039ff49341b 100644 > --- a/drivers/gpu/drm/xe/xe_huc.c > +++ b/drivers/gpu/drm/xe/xe_huc.c > @@ -5,6 +5,8 @@ > > #include "xe_huc.h" > > +#include > + > #include > > #include "abi/gsc_pxp_commands_abi.h" > diff --git a/drivers/gpu/drm/xe/xe_mocs.c b/drivers/gpu/drm/xe/xe_mocs.c > index f04754ad911b..de3f2d3f1b04 100644 > --- a/drivers/gpu/drm/xe/xe_mocs.c > +++ b/drivers/gpu/drm/xe/xe_mocs.c > @@ -12,6 +12,7 @@ > #include "xe_force_wake.h" > #include "xe_gt.h" > #include "xe_gt_mcr.h" > +#include "xe_gt_printk.h" > #include "xe_mmio.h" > #include "xe_platform_types.h" > #include "xe_pm.h" > diff --git a/drivers/gpu/drm/xe/xe_ttm_stolen_mgr.c > b/drivers/gpu/drm/xe/xe_ttm_stolen_mgr.c > index f77367329760..64592a8e527b 100644 > --- a/drivers/gpu/drm/xe/xe_ttm_stolen_mgr.c > +++ b/drivers/gpu/drm/xe/xe_ttm_stolen_mgr.c > @@ -18,6 +18,7 @@ > #include "xe_bo.h" > #include "xe_device.h" > #include "xe_gt.h" > +#include "xe_gt_printk.h" > #include "xe_mmio.h" > #include "xe_res_cursor.h" > #include "xe_sriov.h" > diff --git a/drivers/gpu/drm/xe/xe_uc_fw.c b/drivers/gpu/drm/xe/xe_uc_fw.c > index ed819f1df888..12346645a8e5 100644 > --- a/drivers/gpu/drm/xe/xe_uc_fw.c > +++ b/drivers/gpu/drm/xe/xe_uc_fw.c > @@ -14,6 +14,7 @@ > #include "xe_force_wake.h" > #include "xe_gsc.h" > #include "xe_gt.h" > +#include "xe_gt_printk.h" > #include "xe_map.h" > #include "xe_mmio.h" > #include "xe_module.h" > -- > 2.43.0 >
Re: [PATCH 03/11] drm/i915/display: convert inner wakeref get towards get_if_in_use
On Thu, Mar 14, 2024 at 10:10:13AM -0400, Rodrigo Vivi wrote:
> This patch brings no functional change. Since at this point of
> the code we are already asserting a wakeref was held, it means
> that we are with runtime_pm 'in_use' and in practical terms we
> are only bumping the pm_runtime usage counter and moving on.
>
> However, xe driver has a lockdep annotation that warned us that
> if a sync resume was actually called at this point, we could have
> a deadlock because we are inside the power_domains->lock locked
> area and the resume would call the irq_reset, which would also
> try to get the power_domains->lock.
>
> For this reason, let's convert this call to a safer option and
> calm lockdep on.
>
> v2: use _noresume variant instead of get_in_use (Ville, Imre)
>
> Cc: Ville Syrjälä
> Acked-by: Imre Deak
> Cc: Matthew Auld
> Signed-off-by: Rodrigo Vivi
Reviewed-by: Francois Dugast
> ---
> drivers/gpu/drm/i915/display/intel_display_power.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c
> b/drivers/gpu/drm/i915/display/intel_display_power.c
> index 6fd4fa52253a..048943d0a881 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_power.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_power.c
> @@ -646,7 +646,7 @@ release_async_put_domains(struct i915_power_domains
> *power_domains,
>* power well disabling.
>*/
> assert_rpm_raw_wakeref_held(rpm);
> - wakeref = intel_runtime_pm_get(rpm);
> + wakeref = intel_runtime_pm_get_noresume(rpm);
>
> for_each_power_domain(domain, mask) {
> /* Clear before put, so put's sanity check is happy. */
> --
> 2.44.0
>
Re: [PATCH 02/11] drm/xe: Introduce intel_runtime_pm_get_noresume at compat-i915-headers for display
Hi,
On Thu, Mar 14, 2024 at 10:10:12AM -0400, Rodrigo Vivi wrote:
> The i915-display will start using the intel_runtime_pm_noresume.
> So we need to add the compat header before it.
Or "So we need to add it to the compat header before"?
>
> Signed-off-by: Rodrigo Vivi
> ---
> drivers/gpu/drm/xe/compat-i915-headers/i915_drv.h | 8
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/gpu/drm/xe/compat-i915-headers/i915_drv.h
> b/drivers/gpu/drm/xe/compat-i915-headers/i915_drv.h
> index fef969112b1d..ecaaef3df4bf 100644
> --- a/drivers/gpu/drm/xe/compat-i915-headers/i915_drv.h
> +++ b/drivers/gpu/drm/xe/compat-i915-headers/i915_drv.h
> @@ -176,6 +176,14 @@ static inline intel_wakeref_t
> intel_runtime_pm_get_if_in_use(struct xe_runtime_p
> return xe_pm_runtime_get_if_in_use(xe);
> }
>
> +static inline intel_wakeref_t intel_runtime_pm_get_noresume(struct
> xe_runtime_pm *pm)
> +{
> + struct xe_device *xe = container_of(pm, struct xe_device, runtime_pm);
> +
> + xe_pm_runtime_get_noresume(xe);
> + return true;
> +}
> +
LGTM but wondering if this and the next patch in the series should be
combined in order to have at least one use of this new definition.
Either way:
Reviewed-by: Francois Dugast
Francois
> static inline void intel_runtime_pm_put_unchecked(struct xe_runtime_pm *pm)
> {
> struct xe_device *xe = container_of(pm, struct xe_device, runtime_pm);
> --
> 2.44.0
>
Re: [PATCH v3 1/2] drm/i915: Disable DSB in Xe KMD
On Thu, Jan 04, 2024 at 08:05:56AM -0800, José Roberto de Souza wrote: > Often getting DBS overflows when starting Xorg or Wayland compositors > when running Xe KMD. s/DBS overflows/DSB overflows/ > Issue was reported but nothing was done, so disabling DSB as whole > until properly fixed in Xe KMD. > > v2: > - move check to HAS_DSB(Jani) > > v3: > - use IS_ENABLED(I915) check in intel_dsb_prepare() > > Link: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/989 > Link: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/1031 > Link: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/1072 > Cc: Animesh Manna > Cc: Rodrigo Vivi > Cc: Jani Nikula > Signed-off-by: José Roberto de Souza > --- > drivers/gpu/drm/i915/display/intel_dsb.c | 4 > 1 file changed, 4 insertions(+) > > diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c > b/drivers/gpu/drm/i915/display/intel_dsb.c > index 482c28b5c2de5..a6c7122fd671d 100644 > --- a/drivers/gpu/drm/i915/display/intel_dsb.c > +++ b/drivers/gpu/drm/i915/display/intel_dsb.c > @@ -453,6 +453,10 @@ struct intel_dsb *intel_dsb_prepare(const struct > intel_crtc_state *crtc_state, > if (!HAS_DSB(i915)) > return NULL; > > + /* TODO: DSB is broken in Xe KMD, so disabling it until fixed */ > + if (!IS_ENABLED(I915)) > + return NULL; > + > dsb = kzalloc(sizeof(*dsb), GFP_KERNEL); > if (!dsb) > goto out; > -- > 2.43.0 >
