Re: [PATCH v2 2/4] drm/i915/display: add intel_display_snapshot abstraction

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Sep 09, 2024 at 11:53:02PM +0300, Jani Nikula wrote:
> On Mon, 09 Sep 2024, Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx> wrote:
> > On Mon, Sep 09, 2024 at 04:32:57PM +0300, Jani Nikula wrote:
> >> The error state capture still handles display info at a too detailed
> >> level. Start abstracting the whole display snapshot capture and printing
> >> at a higher level. Move overlay to display snapshot first.
> >> 
> >> Use the same nomenclature and style as in xe devcoredump, in preparation
> >> for perhaps some day bolting the snapshots there as well.
> >> 
> >> v3: Fix build harder for CONFIG_DRM_I915_CAPTURE_ERROR=n
> >> 
> >> v2: Fix build for CONFIG_DRM_I915_CAPTURE_ERROR=n (kernel test robot)
> >> 
> >> Signed-off-by: Jani Nikula <jani.nikula@xxxxxxxxx>
> >> ---
> >>  drivers/gpu/drm/i915/Makefile                 |  1 +
> >>  .../drm/i915/display/intel_display_snapshot.c | 42 +++++++++++++++++++
> >>  .../drm/i915/display/intel_display_snapshot.h | 16 +++++++
> >>  drivers/gpu/drm/i915/display/intel_overlay.c  | 16 ++++---
> >>  drivers/gpu/drm/i915/display/intel_overlay.h  | 25 +++++++----
> >>  drivers/gpu/drm/i915/i915_gpu_error.c         | 12 +++---
> >>  drivers/gpu/drm/i915/i915_gpu_error.h         |  6 +--
> >>  7 files changed, 94 insertions(+), 24 deletions(-)
> >>  create mode 100644 drivers/gpu/drm/i915/display/intel_display_snapshot.c
> >>  create mode 100644 drivers/gpu/drm/i915/display/intel_display_snapshot.h
> >> 
> >> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> >> index c63fa2133ccb..9fcd9e09bc0b 100644
> >> --- a/drivers/gpu/drm/i915/Makefile
> >> +++ b/drivers/gpu/drm/i915/Makefile
> >> @@ -242,6 +242,7 @@ i915-y += \
> >>  	display/intel_display_power_well.o \
> >>  	display/intel_display_reset.o \
> >>  	display/intel_display_rps.o \
> >> +	display/intel_display_snapshot.o \
> >>  	display/intel_display_wa.o \
> >>  	display/intel_dmc.o \
> >>  	display/intel_dmc_wl.o \
> >> diff --git a/drivers/gpu/drm/i915/display/intel_display_snapshot.c b/drivers/gpu/drm/i915/display/intel_display_snapshot.c
> >> new file mode 100644
> >> index 000000000000..78b019dcd41d
> >> --- /dev/null
> >> +++ b/drivers/gpu/drm/i915/display/intel_display_snapshot.c
> >> @@ -0,0 +1,42 @@
> >> +// SPDX-License-Identifier: MIT
> >> +/* Copyright © 2024 Intel Corporation */
> >> +
> >> +#include <linux/slab.h>
> >> +
> >> +#include "intel_display_snapshot.h"
> >> +#include "intel_overlay.h"
> >> +
> >> +struct intel_display_snapshot {
> >> +	struct intel_overlay_snapshot *overlay;
> >> +};
> >> +
> >> +struct intel_display_snapshot *intel_display_snapshot_capture(struct intel_display *display)
> >> +{
> >> +	struct intel_display_snapshot *snapshot;
> >> +
> >> +	snapshot = kzalloc(sizeof(*snapshot), GFP_ATOMIC);
> >> +	if (!snapshot)
> >> +		return NULL;
> >> +
> >> +	snapshot->overlay = intel_overlay_snapshot_capture(display);
> >> +
> >> +	return snapshot;
> >> +}
> >> +
> >> +void intel_display_snapshot_print(const struct intel_display_snapshot *snapshot,
> >> +				  struct drm_printer *p)
> >> +{
> >> +	if (!snapshot)
> >> +		return;
> >> +
> >> +	intel_overlay_snapshot_print(snapshot->overlay, p);
> >> +}
> >> +
> >> +void intel_display_snapshot_free(struct intel_display_snapshot *snapshot)
> >> +{
> >> +	if (!snapshot)
> >> +		return;
> >> +
> >> +	kfree(snapshot->overlay);
> >> +	kfree(snapshot);
> >> +}
> >> diff --git a/drivers/gpu/drm/i915/display/intel_display_snapshot.h b/drivers/gpu/drm/i915/display/intel_display_snapshot.h
> >> new file mode 100644
> >> index 000000000000..7ed27cdea644
> >> --- /dev/null
> >> +++ b/drivers/gpu/drm/i915/display/intel_display_snapshot.h
> >> @@ -0,0 +1,16 @@
> >> +/* SPDX-License-Identifier: MIT */
> >> +/* Copyright © 2024 Intel Corporation */
> >> +
> >> +#ifndef __INTEL_DISPLAY_SNAPSHOT_H__
> >> +#define __INTEL_DISPLAY_SNAPSHOT_H__
> >> +
> >> +struct drm_printer;
> >> +struct intel_display;
> >> +struct intel_display_snapshot;
> >> +
> >> +struct intel_display_snapshot *intel_display_snapshot_capture(struct intel_display *display);
> >> +void intel_display_snapshot_print(const struct intel_display_snapshot *snapshot,
> >> +				  struct drm_printer *p);
> >> +void intel_display_snapshot_free(struct intel_display_snapshot *snapshot);
> >> +
> >> +#endif /* __INTEL_DISPLAY_SNAPSHOT_H__ */
> >> diff --git a/drivers/gpu/drm/i915/display/intel_overlay.c b/drivers/gpu/drm/i915/display/intel_overlay.c
> >> index 06b1122ec13e..b89541458765 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_overlay.c
> >> +++ b/drivers/gpu/drm/i915/display/intel_overlay.c
> >> @@ -1457,18 +1457,19 @@ void intel_overlay_cleanup(struct drm_i915_private *dev_priv)
> >>  
> >>  #if IS_ENABLED(CONFIG_DRM_I915_CAPTURE_ERROR)
> >>  
> >> -struct intel_overlay_error_state {
> >> +struct intel_overlay_snapshot {
> >>  	struct overlay_registers regs;
> >>  	unsigned long base;
> >>  	u32 dovsta;
> >>  	u32 isr;
> >>  };
> >>  
> >> -struct intel_overlay_error_state *
> >> -intel_overlay_capture_error_state(struct drm_i915_private *dev_priv)
> >> +struct intel_overlay_snapshot *
> >> +intel_overlay_snapshot_capture(struct intel_display *display)
> >>  {
> >> +	struct drm_i915_private *dev_priv = to_i915(display->drm);
> >>  	struct intel_overlay *overlay = dev_priv->display.overlay;
> >> -	struct intel_overlay_error_state *error;
> >> +	struct intel_overlay_snapshot *error;
> >>  
> >>  	if (!overlay || !overlay->active)
> >>  		return NULL;
> >> @@ -1487,9 +1488,12 @@ intel_overlay_capture_error_state(struct drm_i915_private *dev_priv)
> >>  }
> >>  
> >>  void
> >> -intel_overlay_print_error_state(struct drm_printer *p,
> >> -				struct intel_overlay_error_state *error)
> >> +intel_overlay_snapshot_print(const struct intel_overlay_snapshot *error,
> >> +			     struct drm_printer *p)
> >>  {
> >> +	if (!error)
> >> +		return;
> >> +
> >>  	drm_printf(p, "Overlay, status: 0x%08x, interrupt: 0x%08x\n",
> >>  		   error->dovsta, error->isr);
> >>  	drm_printf(p, "  Register file at 0x%08lx:\n", error->base);
> >> diff --git a/drivers/gpu/drm/i915/display/intel_overlay.h b/drivers/gpu/drm/i915/display/intel_overlay.h
> >> index f28a09c062d0..eafac24d1de8 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_overlay.h
> >> +++ b/drivers/gpu/drm/i915/display/intel_overlay.h
> >> @@ -6,12 +6,15 @@
> >>  #ifndef __INTEL_OVERLAY_H__
> >>  #define __INTEL_OVERLAY_H__
> >>  
> >> +#include <linux/types.h>
> >
> > so, that was it?
> > I cannot spot any other difference between the v3 and v2.
> > But I also cannot correlate this to the reported errors.
> 
> I'm not sure if the test robot actually tested v2, it just sent the same
> results for gcc and clang. But I found this myself when trying locally
> with CONFIG_DRM_I915_CAPTURE_ERROR=n. It's needed for returning NULL in
> the stub...

fair enough. the code looks right to me and if build-bots are okay
now:

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>

> 
> BR,
> Jani.
> 
> >
> >> +
> >>  struct drm_device;
> >>  struct drm_file;
> >>  struct drm_i915_private;
> >>  struct drm_printer;
> >> +struct intel_display;
> >>  struct intel_overlay;
> >> -struct intel_overlay_error_state;
> >> +struct intel_overlay_snapshot;
> >>  
> >>  #ifdef I915
> >>  void intel_overlay_setup(struct drm_i915_private *dev_priv);
> >> @@ -22,10 +25,6 @@ int intel_overlay_put_image_ioctl(struct drm_device *dev, void *data,
> >>  int intel_overlay_attrs_ioctl(struct drm_device *dev, void *data,
> >>  			      struct drm_file *file_priv);
> >>  void intel_overlay_reset(struct drm_i915_private *dev_priv);
> >> -struct intel_overlay_error_state *
> >> -intel_overlay_capture_error_state(struct drm_i915_private *dev_priv);
> >> -void intel_overlay_print_error_state(struct drm_printer *p,
> >> -				     struct intel_overlay_error_state *error);
> >>  #else
> >>  static inline void intel_overlay_setup(struct drm_i915_private *dev_priv)
> >>  {
> >> @@ -50,13 +49,21 @@ static inline int intel_overlay_attrs_ioctl(struct drm_device *dev, void *data,
> >>  static inline void intel_overlay_reset(struct drm_i915_private *dev_priv)
> >>  {
> >>  }
> >> -static inline struct intel_overlay_error_state *
> >> -intel_overlay_capture_error_state(struct drm_i915_private *dev_priv)
> >> +#endif
> >> +
> >> +#if IS_ENABLED(CONFIG_DRM_I915_CAPTURE_ERROR) && defined(I915)
> >> +struct intel_overlay_snapshot *
> >> +intel_overlay_snapshot_capture(struct intel_display *display);
> >> +void intel_overlay_snapshot_print(const struct intel_overlay_snapshot *error,
> >> +				  struct drm_printer *p);
> >> +#else
> >> +static inline struct intel_overlay_snapshot *
> >> +intel_overlay_snapshot_capture(struct intel_display *display)
> >>  {
> >>  	return NULL;
> >>  }
> >> -static inline void intel_overlay_print_error_state(struct drm_printer *p,
> >> -						   struct intel_overlay_error_state *error)
> >> +static inline void intel_overlay_snapshot_print(const struct intel_overlay_snapshot *error,
> >> +						struct drm_printer *p)
> >>  {
> >>  }
> >>  #endif
> >> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> >> index f23769ccf050..b047b24a90d5 100644
> >> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> >> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> >> @@ -40,8 +40,8 @@
> >>  #include <drm/drm_cache.h>
> >>  #include <drm/drm_print.h>
> >>  
> >> +#include "display/intel_display_snapshot.h"
> >>  #include "display/intel_dmc.h"
> >> -#include "display/intel_overlay.h"
> >>  
> >>  #include "gem/i915_gem_context.h"
> >>  #include "gem/i915_gem_lmem.h"
> >> @@ -905,11 +905,10 @@ static void __err_print_to_sgl(struct drm_i915_error_state_buf *m,
> >>  		err_print_gt_info(m, error->gt);
> >>  	}
> >>  
> >> -	if (error->overlay)
> >> -		intel_overlay_print_error_state(&p, error->overlay);
> >> -
> >>  	err_print_capabilities(m, error);
> >>  	err_print_params(m, &error->params);
> >> +
> >> +	intel_display_snapshot_print(error->display_snapshot, &p);
> >>  }
> >>  
> >>  static int err_print_to_sgl(struct i915_gpu_coredump *error)
> >> @@ -1077,7 +1076,7 @@ void __i915_gpu_coredump_free(struct kref *error_ref)
> >>  		cleanup_gt(gt);
> >>  	}
> >>  
> >> -	kfree(error->overlay);
> >> +	intel_display_snapshot_free(error->display_snapshot);
> >>  
> >>  	cleanup_params(error);
> >>  
> >> @@ -2097,6 +2096,7 @@ static struct i915_gpu_coredump *
> >>  __i915_gpu_coredump(struct intel_gt *gt, intel_engine_mask_t engine_mask, u32 dump_flags)
> >>  {
> >>  	struct drm_i915_private *i915 = gt->i915;
> >> +	struct intel_display *display = &i915->display;
> >>  	struct i915_gpu_coredump *error;
> >>  
> >>  	/* Check if GPU capture has been disabled */
> >> @@ -2138,7 +2138,7 @@ __i915_gpu_coredump(struct intel_gt *gt, intel_engine_mask_t engine_mask, u32 du
> >>  		error->simulated |= error->gt->simulated;
> >>  	}
> >>  
> >> -	error->overlay = intel_overlay_capture_error_state(i915);
> >> +	error->display_snapshot = intel_display_snapshot_capture(display);
> >>  
> >>  	return error;
> >>  }
> >> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.h b/drivers/gpu/drm/i915/i915_gpu_error.h
> >> index 7c255bb1c319..1a11942d7800 100644
> >> --- a/drivers/gpu/drm/i915/i915_gpu_error.h
> >> +++ b/drivers/gpu/drm/i915/i915_gpu_error.h
> >> @@ -31,7 +31,7 @@
> >>  struct drm_i915_private;
> >>  struct i915_vma_compress;
> >>  struct intel_engine_capture_vma;
> >> -struct intel_overlay_error_state;
> >> +struct intel_display_snapshot;
> >>  
> >>  struct i915_vma_coredump {
> >>  	struct i915_vma_coredump *next;
> >> @@ -218,9 +218,9 @@ struct i915_gpu_coredump {
> >>  	struct i915_params params;
> >>  	struct intel_display_params display_params;
> >>  
> >> -	struct intel_overlay_error_state *overlay;
> >> -
> >>  	struct scatterlist *sgl, *fit;
> >> +
> >> +	struct intel_display_snapshot *display_snapshot;
> >>  };
> >>  
> >>  struct i915_gpu_error {
> >> -- 
> >> 2.39.2
> >> 
> 
> -- 
> Jani Nikula, Intel



[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux