diff mbox

[7/7] drm/i915/guc: Add support for define guc_log_size in megabytes.

Message ID 20180530135334.25113-7-piotr.piorkowski@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Piotr Piórkowski May 30, 2018, 1:53 p.m. UTC 
At this moment we can define GuC logs sizes only using pages.
But GuC also allows use for this values expressed in megabytes.
Lets add support for define guc_log_size in megabytes when we
debug of GuC.

Signed-off-by: Piotr Piórkowski <piotr.piorkowski@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Michał Winiarski <michal.winiarski@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_guc.c     | 12 ++++++++++--
 drivers/gpu/drm/i915/intel_guc_log.h |  6 ++++++
 2 files changed, 16 insertions(+), 2 deletions(-)

Comments

Michal Wajdeczko May 30, 2018, 4:46 p.m. UTC | #1 
On Wed, 30 May 2018 15:53:34 +0200, Piotr Piorkowski  
<piotr.piorkowski@intel.com> wrote:

> At this moment we can define GuC logs sizes only using pages.
> But GuC also allows use for this values expressed in megabytes.
> Lets add support for define guc_log_size in megabytes when we
> debug of GuC.
>
> Signed-off-by: Piotr Piórkowski <piotr.piorkowski@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_guc.c     | 12 ++++++++++--
>  drivers/gpu/drm/i915/intel_guc_log.h |  6 ++++++
>  2 files changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_guc.c  
> b/drivers/gpu/drm/i915/intel_guc.c
> index e15047fedb45..5a42db47521b 100644
> --- a/drivers/gpu/drm/i915/intel_guc.c
> +++ b/drivers/gpu/drm/i915/intel_guc.c
> @@ -263,7 +263,13 @@ static u32 guc_ctl_log_params_flags(struct  
> intel_guc *guc)
>  	u32 offset = intel_guc_ggtt_offset(guc, guc->log.vma) >> PAGE_SHIFT;
>  	u32 flags;
> +	#if (((CRASH_BUFFER_SIZE) % (1 << 20)) == 0)
> +	#define UNIT (1 << 20)
> +	#define FLAG GUC_LOG_ALLOC_IN_MEGABYTE
> +	#else
>  	#define UNIT (4 << 10)
> +	#define FLAG 0
> +	#endif
> 	BUILD_BUG_ON(!CRASH_BUFFER_SIZE);
>  	BUILD_BUG_ON(!IS_ALIGNED(CRASH_BUFFER_SIZE, UNIT));
> @@ -280,13 +286,15 @@ static u32 guc_ctl_log_params_flags(struct  
> intel_guc *guc)
>  			(GUC_LOG_ISR_MASK >> GUC_LOG_ISR_SHIFT));
> 	flags = GUC_LOG_VALID |
> -		GUC_LOG_NOTIFY_ON_HALF_FULL |
> -		((CRASH_BUFFER_SIZE/UNIT - 1) << GUC_LOG_CRASH_SHIFT) |
> +		GUC_LOG_NOTIFY_ON_HALF_FULL;
> +	flags |= FLAG;

I think you can inject FLAG into existing statement without
introducing two additional |=

> +	flags |= ((CRASH_BUFFER_SIZE/UNIT - 1) << GUC_LOG_CRASH_SHIFT) |
>  		((DPC_BUFFER_SIZE/UNIT - 1) << GUC_LOG_DPC_SHIFT) |
>  		((ISR_BUFFER_SIZE/UNIT - 1) << GUC_LOG_ISR_SHIFT) |
>  		(offset << GUC_LOG_BUF_ADDR_SHIFT);
> 	#undef UNIT
> +	#undef FLAG
> 	return flags;
>  }
> diff --git a/drivers/gpu/drm/i915/intel_guc_log.h  
> b/drivers/gpu/drm/i915/intel_guc_log.h
> index 1b3afdae6d0d..de39b965ae7a 100644
> --- a/drivers/gpu/drm/i915/intel_guc_log.h
> +++ b/drivers/gpu/drm/i915/intel_guc_log.h
> @@ -34,9 +34,15 @@
> struct intel_guc;
> +#ifdef DRM_I915_DEBUG_GUC
> +#define CRASH_BUFFER_SIZE	2097152
> +#define DPC_BUFFER_SIZE		8388608
> +#define ISR_BUFFER_SIZE		8388608

can we make it more friendly: (8 * 1024 * 1024)

> +#else
>  #define CRASH_BUFFER_SIZE	8192
>  #define DPC_BUFFER_SIZE		32768
>  #define ISR_BUFFER_SIZE		32768
> +#endif

btw, are these values just max possible or selected
as most valuable ? question for both debug/ndebug

Michal

> /*
>   * While we're using plain log level in i915, GuC controls are much  
> more...
Piotr Piórkowski June 4, 2018, 12:53 p.m. UTC | #2 
On Wed, 2018-05-30 at 18:46 +0200, Michal Wajdeczko wrote:
> On Wed, 30 May 2018 15:53:34 +0200, Piotr Piorkowski  
> <piotr.piorkowski@intel.com> wrote:
> 
> > At this moment we can define GuC logs sizes only using pages.
> > But GuC also allows use for this values expressed in megabytes.
> > Lets add support for define guc_log_size in megabytes when we
> > debug of GuC.
> > 
> > Signed-off-by: Piotr Piórkowski <piotr.piorkowski@intel.com>
> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > Cc: Michał Winiarski <michal.winiarski@intel.com>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/intel_guc.c     | 12 ++++++++++--
> >  drivers/gpu/drm/i915/intel_guc_log.h |  6 ++++++
> >  2 files changed, 16 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_guc.c  
> > b/drivers/gpu/drm/i915/intel_guc.c
> > index e15047fedb45..5a42db47521b 100644
> > --- a/drivers/gpu/drm/i915/intel_guc.c
> > +++ b/drivers/gpu/drm/i915/intel_guc.c
> > @@ -263,7 +263,13 @@ static u32 guc_ctl_log_params_flags(struct  
> > intel_guc *guc)
> >  	u32 offset = intel_guc_ggtt_offset(guc, guc->log.vma) >>
> > PAGE_SHIFT;
> >  	u32 flags;
> > +	#if (((CRASH_BUFFER_SIZE) % (1 << 20)) == 0)
> > +	#define UNIT (1 << 20)
> > +	#define FLAG GUC_LOG_ALLOC_IN_MEGABYTE
> > +	#else
> >  	#define UNIT (4 << 10)
> > +	#define FLAG 0
> > +	#endif
> > 	BUILD_BUG_ON(!CRASH_BUFFER_SIZE);
> >  	BUILD_BUG_ON(!IS_ALIGNED(CRASH_BUFFER_SIZE, UNIT));
> > @@ -280,13 +286,15 @@ static u32 guc_ctl_log_params_flags(struct  
> > intel_guc *guc)
> >  			(GUC_LOG_ISR_MASK >> GUC_LOG_ISR_SHIFT));
> > 	flags = GUC_LOG_VALID |
> > -		GUC_LOG_NOTIFY_ON_HALF_FULL |
> > -		((CRASH_BUFFER_SIZE/UNIT - 1) <<
> > GUC_LOG_CRASH_SHIFT) |
> > +		GUC_LOG_NOTIFY_ON_HALF_FULL;
> > +	flags |= FLAG;
> 
> I think you can inject FLAG into existing statement without
> introducing two additional |=
> 
> > +	flags |= ((CRASH_BUFFER_SIZE/UNIT - 1) <<
> > GUC_LOG_CRASH_SHIFT) |
> >  		((DPC_BUFFER_SIZE/UNIT - 1) << GUC_LOG_DPC_SHIFT)
> > |
> >  		((ISR_BUFFER_SIZE/UNIT - 1) << GUC_LOG_ISR_SHIFT)
> > |
> >  		(offset << GUC_LOG_BUF_ADDR_SHIFT);
> > 	#undef UNIT
> > +	#undef FLAG
> > 	return flags;
> >  }
> > diff --git a/drivers/gpu/drm/i915/intel_guc_log.h  
> > b/drivers/gpu/drm/i915/intel_guc_log.h
> > index 1b3afdae6d0d..de39b965ae7a 100644
> > --- a/drivers/gpu/drm/i915/intel_guc_log.h
> > +++ b/drivers/gpu/drm/i915/intel_guc_log.h
> > @@ -34,9 +34,15 @@
> > struct intel_guc;
> > +#ifdef DRM_I915_DEBUG_GUC
> > +#define CRASH_BUFFER_SIZE	2097152
> > +#define DPC_BUFFER_SIZE		8388608
> > +#define ISR_BUFFER_SIZE		8388608
> 
> can we make it more friendly: (8 * 1024 * 1024)
> 
> > +#else
> >  #define CRASH_BUFFER_SIZE	8192
> >  #define DPC_BUFFER_SIZE		32768
> >  #define ISR_BUFFER_SIZE		32768
> > +#endif
> 
> btw, are these values just max possible or selected
> as most valuable ? question for both debug/ndebug

For debug I use the possibility of defining values in MB and I set the
maximum possible values, and for ndebug I set old values of these
buffers
> 
> Michal
> 
> > /*
> >   * While we're using plain log level in i915, GuC controls are
> > much  
> > more...
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
index e15047fedb45..5a42db47521b 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -263,7 +263,13 @@  static u32 guc_ctl_log_params_flags(struct intel_guc *guc)
 	u32 offset = intel_guc_ggtt_offset(guc, guc->log.vma) >> PAGE_SHIFT;
 	u32 flags;
 
+	#if (((CRASH_BUFFER_SIZE) % (1 << 20)) == 0)
+	#define UNIT (1 << 20)
+	#define FLAG GUC_LOG_ALLOC_IN_MEGABYTE
+	#else
 	#define UNIT (4 << 10)
+	#define FLAG 0
+	#endif
 
 	BUILD_BUG_ON(!CRASH_BUFFER_SIZE);
 	BUILD_BUG_ON(!IS_ALIGNED(CRASH_BUFFER_SIZE, UNIT));
@@ -280,13 +286,15 @@  static u32 guc_ctl_log_params_flags(struct intel_guc *guc)
 			(GUC_LOG_ISR_MASK >> GUC_LOG_ISR_SHIFT));
 
 	flags = GUC_LOG_VALID |
-		GUC_LOG_NOTIFY_ON_HALF_FULL |
-		((CRASH_BUFFER_SIZE/UNIT - 1) << GUC_LOG_CRASH_SHIFT) |
+		GUC_LOG_NOTIFY_ON_HALF_FULL;
+	flags |= FLAG;
+	flags |= ((CRASH_BUFFER_SIZE/UNIT - 1) << GUC_LOG_CRASH_SHIFT) |
 		((DPC_BUFFER_SIZE/UNIT - 1) << GUC_LOG_DPC_SHIFT) |
 		((ISR_BUFFER_SIZE/UNIT - 1) << GUC_LOG_ISR_SHIFT) |
 		(offset << GUC_LOG_BUF_ADDR_SHIFT);
 
 	#undef UNIT
+	#undef FLAG
 
 	return flags;
 }
diff --git a/drivers/gpu/drm/i915/intel_guc_log.h b/drivers/gpu/drm/i915/intel_guc_log.h
index 1b3afdae6d0d..de39b965ae7a 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.h
+++ b/drivers/gpu/drm/i915/intel_guc_log.h
@@ -34,9 +34,15 @@ 
 
 struct intel_guc;
 
+#ifdef DRM_I915_DEBUG_GUC
+#define CRASH_BUFFER_SIZE	2097152
+#define DPC_BUFFER_SIZE		8388608
+#define ISR_BUFFER_SIZE		8388608
+#else
 #define CRASH_BUFFER_SIZE	8192
 #define DPC_BUFFER_SIZE		32768
 #define ISR_BUFFER_SIZE		32768
+#endif
 
 /*
  * While we're using plain log level in i915, GuC controls are much more...