[PATCH v2 0/3] Remove #ifdef ISP2401 and unifying sh_css_sp_group structure

[PATCH v2 0/3] Remove #ifdef ISP2401 and unifying sh_css_sp_group structure

[Kate Hsuan]

The changes in v2 include:
1. Squashed patches #1 and #3 to resolve the build issue.
2. Introduced a new patch to remove the unused debug codes and variables.
3. Added a NULL pointer check for memory allocation failure.
4. Fixed the memory offset when copying the configuration to the buffer.

Kate Hsuan (3):
  media: atomisp: sh_css_internal: Unifying sh_css_sp_group to remove
    #ifdef ISP2401
  media: atomisp: ia_css_debug: remove unused
    HAS_WATCHDOG_SP_THREAD_DEBUG codes
  atomisp: sh_css_params: write the sp_group config according to the ISP
    model

 .../pci/hive_isp_css_common/debug_global.h    |   7 -
 .../media/atomisp/pci/ia_css_acc_types.h      |   6 +-
 .../runtime/debug/interface/ia_css_debug.h    |  12 --
 .../pci/runtime/debug/src/ia_css_debug.c      | 165 ------------------
 .../media/atomisp/pci/sh_css_internal.h       |  41 +----
 .../staging/media/atomisp/pci/sh_css_params.c |  41 ++++-
 6 files changed, 42 insertions(+), 230 deletions(-)

-- 
2.40.1

[PATCH v2 1/3] media: atomisp: sh_css_internal: Unifying sh_css_sp_group to remove #ifdef ISP2401

[Kate Hsuan]

Since some parts of the data scructure elements are determined in compile
time, the configuration data structure should be compiled for both two
ISP models. In order to set the configuration for both ISP model in
runtime, Thesh_css_sp_group is unified to one data structure for the
configuration to ensure the data structure can be used for both ISP2400
and 2401. Some of the unsed codes from ia_css_debug.c are also removed.

Signed-off-by: Kate Hsuan <hpa@redhat.com>
---
 .../runtime/debug/interface/ia_css_debug.h    |  6 --
 .../pci/runtime/debug/src/ia_css_debug.c      | 76 +------------------
 .../media/atomisp/pci/sh_css_internal.h       | 35 +++------
 3 files changed, 13 insertions(+), 104 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/runtime/debug/interface/ia_css_debug.h b/drivers/staging/media/atomisp/pci/runtime/debug/interface/ia_css_debug.h
index fff89e9b4b01..3a3d72c6eaaa 100644
--- a/drivers/staging/media/atomisp/pci/runtime/debug/interface/ia_css_debug.h
+++ b/drivers/staging/media/atomisp/pci/runtime/debug/interface/ia_css_debug.h
@@ -141,12 +141,6 @@ static inline void __printf(2, 0) ia_css_debug_vdtrace(unsigned int level,
 __printf(2, 3) void ia_css_debug_dtrace(unsigned int level,
 					const char *fmt, ...);
 
-/*! @brief Dump sp thread's stack contents
- * SP thread's stack contents are set to 0xcafecafe. This function dumps the
- * stack to inspect if the stack's boundaries are compromised.
- * @return	None
- */
-void ia_css_debug_dump_sp_stack_info(void);
 
 /*! @brief Function to set the global dtrace verbosity level.
  * @param[in]	trace_level	Maximum level of the messages to be traced.
diff --git a/drivers/staging/media/atomisp/pci/runtime/debug/src/ia_css_debug.c b/drivers/staging/media/atomisp/pci/runtime/debug/src/ia_css_debug.c
index bb6204cb42c5..bb30146c5fe7 100644
--- a/drivers/staging/media/atomisp/pci/runtime/debug/src/ia_css_debug.c
+++ b/drivers/staging/media/atomisp/pci/runtime/debug/src/ia_css_debug.c
@@ -105,7 +105,8 @@
  * TODO:SH_CSS_MAX_SP_THREADS is not the max number of sp threads
  * future rework should fix this and remove the define MAX_THREAD_NUM
  */
-#define MAX_THREAD_NUM (SH_CSS_MAX_SP_THREADS + SH_CSS_MAX_SP_INTERNAL_THREADS)
+#define MAX_THREAD_NUM_2400 (SH_CSS_MAX_SP_THREADS + SH_CSS_MAX_SP_INTERNAL_THREADS_2400)
+#define MAX_THREAD_NUM_2401 (SH_CSS_MAX_SP_THREADS + SH_CSS_MAX_SP_INTERNAL_THREADS_2401)
 
 static struct pipe_graph_class {
 	bool do_init;
@@ -147,79 +148,6 @@ void ia_css_debug_dtrace(unsigned int level, const char *fmt, ...)
 	va_end(ap);
 }
 
-static void debug_dump_long_array_formatted(
-    const sp_ID_t sp_id,
-    hrt_address stack_sp_addr,
-    unsigned int stack_size)
-{
-	unsigned int i;
-	u32 val;
-	u32 addr = (uint32_t)stack_sp_addr;
-	u32 stack_size_words = CEIL_DIV(stack_size, sizeof(uint32_t));
-
-	/* When size is not multiple of four, last word is only relevant for
-	 * remaining bytes */
-	for (i = 0; i < stack_size_words; i++) {
-		val = sp_dmem_load_uint32(sp_id, (hrt_address)addr);
-		if ((i % 8) == 0)
-			ia_css_debug_dtrace(IA_CSS_DEBUG_VERBOSE, "\n");
-
-		ia_css_debug_dtrace(IA_CSS_DEBUG_VERBOSE, "0x%08x ", val);
-		addr += sizeof(uint32_t);
-	}
-
-	ia_css_debug_dtrace(IA_CSS_DEBUG_VERBOSE, "\n");
-}
-
-static void debug_dump_sp_stack_info(
-    const sp_ID_t sp_id)
-{
-	const struct ia_css_fw_info *fw;
-	unsigned int HIVE_ADDR_sp_threads_stack;
-	unsigned int HIVE_ADDR_sp_threads_stack_size;
-	u32 stack_sizes[MAX_THREAD_NUM];
-	u32 stack_sp_addr[MAX_THREAD_NUM];
-	unsigned int i;
-
-	fw = &sh_css_sp_fw;
-
-	ia_css_debug_dtrace(IA_CSS_DEBUG_VERBOSE, "sp_id(%u) stack info\n", sp_id);
-	ia_css_debug_dtrace(IA_CSS_DEBUG_VERBOSE,
-			    "from objects stack_addr_offset:0x%x stack_size_offset:0x%x\n",
-			    fw->info.sp.threads_stack,
-			    fw->info.sp.threads_stack_size);
-
-	HIVE_ADDR_sp_threads_stack = fw->info.sp.threads_stack;
-	HIVE_ADDR_sp_threads_stack_size = fw->info.sp.threads_stack_size;
-
-	if (fw->info.sp.threads_stack == 0 ||
-	    fw->info.sp.threads_stack_size == 0)
-		return;
-
-	(void)HIVE_ADDR_sp_threads_stack;
-	(void)HIVE_ADDR_sp_threads_stack_size;
-
-	sp_dmem_load(sp_id,
-		     (unsigned int)sp_address_of(sp_threads_stack),
-		     &stack_sp_addr, sizeof(stack_sp_addr));
-	sp_dmem_load(sp_id,
-		     (unsigned int)sp_address_of(sp_threads_stack_size),
-		     &stack_sizes, sizeof(stack_sizes));
-
-	for (i = 0 ; i < MAX_THREAD_NUM; i++) {
-		ia_css_debug_dtrace(IA_CSS_DEBUG_VERBOSE,
-				    "thread: %u stack_addr: 0x%08x stack_size: %u\n",
-				    i, stack_sp_addr[i], stack_sizes[i]);
-		debug_dump_long_array_formatted(sp_id, (hrt_address)stack_sp_addr[i],
-						stack_sizes[i]);
-	}
-}
-
-void ia_css_debug_dump_sp_stack_info(void)
-{
-	debug_dump_sp_stack_info(SP0_ID);
-}
-
 void ia_css_debug_set_dtrace_level(const unsigned int trace_level)
 {
 	dbg_level = trace_level;
diff --git a/drivers/staging/media/atomisp/pci/sh_css_internal.h b/drivers/staging/media/atomisp/pci/sh_css_internal.h
index d98f1323441e..2fa0b3e45fe0 100644
--- a/drivers/staging/media/atomisp/pci/sh_css_internal.h
+++ b/drivers/staging/media/atomisp/pci/sh_css_internal.h
@@ -22,9 +22,7 @@
 #include <platform_support.h>
 #include <linux/stdarg.h>
 
-#if !defined(ISP2401)
 #include "input_formatter.h"
-#endif
 #include "input_system.h"
 
 #include "ia_css_types.h"
@@ -95,19 +93,20 @@
  *	 these threads can't be used as image pipe
  */
 
-#if !defined(ISP2401)
-#define SH_CSS_SP_INTERNAL_METADATA_THREAD	1
-#else
-#define SH_CSS_SP_INTERNAL_METADATA_THREAD	0
-#endif
+#define SH_CSS_SP_INTERNAL_METADATA_THREAD_2400	1
+#define SH_CSS_SP_INTERNAL_METADATA_THREAD_2401	0
 
 #define SH_CSS_SP_INTERNAL_SERVICE_THREAD		1
 
 #define SH_CSS_MAX_SP_THREADS		5
 
-#define SH_CSS_MAX_SP_INTERNAL_THREADS	(\
-	 SH_CSS_SP_INTERNAL_SERVICE_THREAD +\
-	 SH_CSS_SP_INTERNAL_METADATA_THREAD)
+#define SH_CSS_MAX_SP_INTERNAL_THREADS_2400	(\
+	SH_CSS_SP_INTERNAL_SERVICE_THREAD +\
+	 SH_CSS_SP_INTERNAL_METADATA_THREAD_2400)
+
+#define SH_CSS_MAX_SP_INTERNAL_THREADS_2401	(\
+	SH_CSS_SP_INTERNAL_SERVICE_THREAD +\
+	 SH_CSS_SP_INTERNAL_METADATA_THREAD_2401)
 
 #define SH_CSS_MAX_PIPELINES	SH_CSS_MAX_SP_THREADS
 
@@ -357,14 +356,12 @@ struct sh_css_sp_debug_command {
 	u32 dma_sw_reg;
 };
 
-#if !defined(ISP2401)
 /* SP input formatter configuration.*/
 struct sh_css_sp_input_formatter_set {
 	u32				stream_format;
 	input_formatter_cfg_t	config_a;
 	input_formatter_cfg_t	config_b;
 };
-#endif
 
 #define IA_CSS_MIPI_SIZE_CHECK_MAX_NOF_ENTRIES_PER_PORT (3)
 
@@ -377,7 +374,7 @@ struct sh_css_sp_config {
 	     frames are locked when their EOF event is successfully sent to the
 	     host (true) or when they are passed to the preview/video pipe
 	     (false). */
-#if !defined(ISP2401)
+
 	struct {
 		u8					a_changed;
 		u8					b_changed;
@@ -385,15 +382,13 @@ struct sh_css_sp_config {
 		struct sh_css_sp_input_formatter_set
 			set[SH_CSS_MAX_IF_CONFIGS]; /* CSI-2 port is used as index. */
 	} input_formatter;
-#endif
-#if !defined(ISP2401)
+
 	sync_generator_cfg_t	sync_gen;
 	tpg_cfg_t		tpg;
 	prbs_cfg_t		prbs;
 	input_system_cfg_t	input_circuit;
 	u8			input_circuit_cfg_changed;
 	u32		mipi_sizes_for_check[N_CSI_PORTS][IA_CSS_MIPI_SIZE_CHECK_MAX_NOF_ENTRIES_PER_PORT];
-#endif
 	u8                 enable_isys_event_queue;
 	u8			disable_cont_vf;
 };
@@ -409,7 +404,6 @@ enum sh_css_stage_type {
 #define SH_CSS_PIPE_CONFIG_SAMPLE_PARAMS_MASK \
 	((SH_CSS_PIPE_CONFIG_SAMPLE_PARAMS << SH_CSS_MAX_SP_THREADS) - 1)
 
-#if defined(ISP2401)
 struct sh_css_sp_pipeline_terminal {
 	union {
 		/* Input System 2401 */
@@ -442,7 +436,6 @@ struct sh_css_sp_pipeline_io_status {
 	u32	running[N_INPUT_SYSTEM_CSI_PORT];	/** configured streams */
 };
 
-#endif
 enum sh_css_port_dir {
 	SH_CSS_PORT_INPUT  = 0,
 	SH_CSS_PORT_OUTPUT  = 1
@@ -641,10 +634,8 @@ struct sh_css_sp_stage {
 struct sh_css_sp_group {
 	struct sh_css_sp_config		config;
 	struct sh_css_sp_pipeline	pipe[SH_CSS_MAX_SP_THREADS];
-#if defined(ISP2401)
 	struct sh_css_sp_pipeline_io	pipe_io[SH_CSS_MAX_SP_THREADS];
 	struct sh_css_sp_pipeline_io_status	pipe_io_status;
-#endif
 	struct sh_css_sp_debug_command	debug;
 };
 
@@ -922,13 +913,11 @@ sh_css_frame_info_set_width(struct ia_css_frame_info *info,
 			    unsigned int width,
 			    unsigned int aligned);
 
-#if !defined(ISP2401)
 
 unsigned int
 sh_css_get_mipi_sizes_for_check(const unsigned int port,
 				const unsigned int idx);
 
-#endif
 
 ia_css_ptr
 sh_css_store_sp_group_to_ddr(void);
@@ -971,11 +960,9 @@ sh_css_continuous_is_enabled(uint8_t pipe_num);
 struct ia_css_pipe *
 find_pipe_by_num(uint32_t pipe_num);
 
-#ifdef ISP2401
 void
 ia_css_get_crop_offsets(
     struct ia_css_pipe *pipe,
     struct ia_css_frame_info *in_frame);
-#endif
 
 #endif /* _SH_CSS_INTERNAL_H_ */
-- 
2.40.1

Re: [PATCH v2 1/3] media: atomisp: sh_css_internal: Unifying sh_css_sp_group to remove #ifdef ISP2401

[Hans de Goede]

Hi Kate,

Thank you for the new version.

It seems that this is a mismatch of multiple previous v1 patches
squashed into 1, so I'm afraid that this is going to need
a version 3 to fix this.

I see bits of both:

[PATCH 1/3] media: atomisp: sh_css_internal: Unifying sh_css_sp_group to remove #ifdef ISP2401
[PATCH 3/3] media: atomisp: ia_css_debug: remove unused codes

from v1 of the series in here. Where I think this should be only patch 3/3,
removing the unused ia_css_debug code as a prep patch for further changes.

See below for details.

On 6/12/23 07:57, Kate Hsuan wrote:
> Since some parts of the data scructure elements are determined in compile
> time, the configuration data structure should be compiled for both two
> ISP models. In order to set the configuration for both ISP model in
> runtime, Thesh_css_sp_group is unified to one data structure for the
> configuration to ensure the data structure can be used for both ISP2400
> and 2401. Some of the unsed codes from ia_css_debug.c are also removed.

It seems that when squashing patches you have ended up re-using the wrong
commit message for this one.

Instead the commit msg of the v1:
"[PATCH 3/3] media: atomisp: ia_css_debug: remove unused codes"

patch should be used here.


> Signed-off-by: Kate Hsuan <hpa@redhat.com>
> ---
>  .../runtime/debug/interface/ia_css_debug.h    |  6 --
>  .../pci/runtime/debug/src/ia_css_debug.c      | 76 +------------------
>  .../media/atomisp/pci/sh_css_internal.h       | 35 +++------
>  3 files changed, 13 insertions(+), 104 deletions(-)
> 
> diff --git a/drivers/staging/media/atomisp/pci/runtime/debug/interface/ia_css_debug.h b/drivers/staging/media/atomisp/pci/runtime/debug/interface/ia_css_debug.h
> index fff89e9b4b01..3a3d72c6eaaa 100644
> --- a/drivers/staging/media/atomisp/pci/runtime/debug/interface/ia_css_debug.h
> +++ b/drivers/staging/media/atomisp/pci/runtime/debug/interface/ia_css_debug.h
> @@ -141,12 +141,6 @@ static inline void __printf(2, 0) ia_css_debug_vdtrace(unsigned int level,
>  __printf(2, 3) void ia_css_debug_dtrace(unsigned int level,
>  					const char *fmt, ...);
>  
> -/*! @brief Dump sp thread's stack contents
> - * SP thread's stack contents are set to 0xcafecafe. This function dumps the
> - * stack to inspect if the stack's boundaries are compromised.
> - * @return	None
> - */
> -void ia_css_debug_dump_sp_stack_info(void);
>  
>  /*! @brief Function to set the global dtrace verbosity level.
>   * @param[in]	trace_level	Maximum level of the messages to be traced.
> diff --git a/drivers/staging/media/atomisp/pci/runtime/debug/src/ia_css_debug.c b/drivers/staging/media/atomisp/pci/runtime/debug/src/ia_css_debug.c
> index bb6204cb42c5..bb30146c5fe7 100644
> --- a/drivers/staging/media/atomisp/pci/runtime/debug/src/ia_css_debug.c
> +++ b/drivers/staging/media/atomisp/pci/runtime/debug/src/ia_css_debug.c

The patch-hunk starting here:

> @@ -105,7 +105,8 @@
>   * TODO:SH_CSS_MAX_SP_THREADS is not the max number of sp threads
>   * future rework should fix this and remove the define MAX_THREAD_NUM
>   */
> -#define MAX_THREAD_NUM (SH_CSS_MAX_SP_THREADS + SH_CSS_MAX_SP_INTERNAL_THREADS)
> +#define MAX_THREAD_NUM_2400 (SH_CSS_MAX_SP_THREADS + SH_CSS_MAX_SP_INTERNAL_THREADS_2400)
> +#define MAX_THREAD_NUM_2401 (SH_CSS_MAX_SP_THREADS + SH_CSS_MAX_SP_INTERNAL_THREADS_2401)
>  
>  static struct pipe_graph_class {
>  	bool do_init;

And ending here, does not belong in this patch. Instead it should be squashed into patch 2/3
which will result in patch 2/3 simply removing the original lines.

> @@ -147,79 +148,6 @@ void ia_css_debug_dtrace(unsigned int level, const char *fmt, ...)
>  	va_end(ap);
>  }
>  
> -static void debug_dump_long_array_formatted(
> -    const sp_ID_t sp_id,
> -    hrt_address stack_sp_addr,
> -    unsigned int stack_size)
> -{
> -	unsigned int i;
> -	u32 val;
> -	u32 addr = (uint32_t)stack_sp_addr;
> -	u32 stack_size_words = CEIL_DIV(stack_size, sizeof(uint32_t));
> -
> -	/* When size is not multiple of four, last word is only relevant for
> -	 * remaining bytes */
> -	for (i = 0; i < stack_size_words; i++) {
> -		val = sp_dmem_load_uint32(sp_id, (hrt_address)addr);
> -		if ((i % 8) == 0)
> -			ia_css_debug_dtrace(IA_CSS_DEBUG_VERBOSE, "\n");
> -
> -		ia_css_debug_dtrace(IA_CSS_DEBUG_VERBOSE, "0x%08x ", val);
> -		addr += sizeof(uint32_t);
> -	}
> -
> -	ia_css_debug_dtrace(IA_CSS_DEBUG_VERBOSE, "\n");
> -}
> -
> -static void debug_dump_sp_stack_info(
> -    const sp_ID_t sp_id)
> -{
> -	const struct ia_css_fw_info *fw;
> -	unsigned int HIVE_ADDR_sp_threads_stack;
> -	unsigned int HIVE_ADDR_sp_threads_stack_size;
> -	u32 stack_sizes[MAX_THREAD_NUM];
> -	u32 stack_sp_addr[MAX_THREAD_NUM];
> -	unsigned int i;
> -
> -	fw = &sh_css_sp_fw;
> -
> -	ia_css_debug_dtrace(IA_CSS_DEBUG_VERBOSE, "sp_id(%u) stack info\n", sp_id);
> -	ia_css_debug_dtrace(IA_CSS_DEBUG_VERBOSE,
> -			    "from objects stack_addr_offset:0x%x stack_size_offset:0x%x\n",
> -			    fw->info.sp.threads_stack,
> -			    fw->info.sp.threads_stack_size);
> -
> -	HIVE_ADDR_sp_threads_stack = fw->info.sp.threads_stack;
> -	HIVE_ADDR_sp_threads_stack_size = fw->info.sp.threads_stack_size;
> -
> -	if (fw->info.sp.threads_stack == 0 ||
> -	    fw->info.sp.threads_stack_size == 0)
> -		return;
> -
> -	(void)HIVE_ADDR_sp_threads_stack;
> -	(void)HIVE_ADDR_sp_threads_stack_size;
> -
> -	sp_dmem_load(sp_id,
> -		     (unsigned int)sp_address_of(sp_threads_stack),
> -		     &stack_sp_addr, sizeof(stack_sp_addr));
> -	sp_dmem_load(sp_id,
> -		     (unsigned int)sp_address_of(sp_threads_stack_size),
> -		     &stack_sizes, sizeof(stack_sizes));
> -
> -	for (i = 0 ; i < MAX_THREAD_NUM; i++) {
> -		ia_css_debug_dtrace(IA_CSS_DEBUG_VERBOSE,
> -				    "thread: %u stack_addr: 0x%08x stack_size: %u\n",
> -				    i, stack_sp_addr[i], stack_sizes[i]);
> -		debug_dump_long_array_formatted(sp_id, (hrt_address)stack_sp_addr[i],
> -						stack_sizes[i]);
> -	}
> -}
> -
> -void ia_css_debug_dump_sp_stack_info(void)
> -{
> -	debug_dump_sp_stack_info(SP0_ID);
> -}
> -
>  void ia_css_debug_set_dtrace_level(const unsigned int trace_level)
>  {
>  	dbg_level = trace_level;

All the sh_css_internal.h changes below should be
squashed into / moved to:

[PATCH v2 3/3] atomisp: sh_css_params: write the sp_group config according to the ISP model

> diff --git a/drivers/staging/media/atomisp/pci/sh_css_internal.h b/drivers/staging/media/atomisp/pci/sh_css_internal.h
> index d98f1323441e..2fa0b3e45fe0 100644
> --- a/drivers/staging/media/atomisp/pci/sh_css_internal.h
> +++ b/drivers/staging/media/atomisp/pci/sh_css_internal.h
> @@ -22,9 +22,7 @@
>  #include <platform_support.h>
>  #include <linux/stdarg.h>
>  
> -#if !defined(ISP2401)
>  #include "input_formatter.h"
> -#endif
>  #include "input_system.h"
>  
>  #include "ia_css_types.h"
> @@ -95,19 +93,20 @@
>   *	 these threads can't be used as image pipe
>   */
>  
> -#if !defined(ISP2401)
> -#define SH_CSS_SP_INTERNAL_METADATA_THREAD	1
> -#else
> -#define SH_CSS_SP_INTERNAL_METADATA_THREAD	0
> -#endif
> +#define SH_CSS_SP_INTERNAL_METADATA_THREAD_2400	1
> +#define SH_CSS_SP_INTERNAL_METADATA_THREAD_2401	0
>  
>  #define SH_CSS_SP_INTERNAL_SERVICE_THREAD		1
>  
>  #define SH_CSS_MAX_SP_THREADS		5
>  
> -#define SH_CSS_MAX_SP_INTERNAL_THREADS	(\
> -	 SH_CSS_SP_INTERNAL_SERVICE_THREAD +\
> -	 SH_CSS_SP_INTERNAL_METADATA_THREAD)
> +#define SH_CSS_MAX_SP_INTERNAL_THREADS_2400	(\
> +	SH_CSS_SP_INTERNAL_SERVICE_THREAD +\
> +	 SH_CSS_SP_INTERNAL_METADATA_THREAD_2400)
> +
> +#define SH_CSS_MAX_SP_INTERNAL_THREADS_2401	(\
> +	SH_CSS_SP_INTERNAL_SERVICE_THREAD +\
> +	 SH_CSS_SP_INTERNAL_METADATA_THREAD_2401)
>  
>  #define SH_CSS_MAX_PIPELINES	SH_CSS_MAX_SP_THREADS
>  
> @@ -357,14 +356,12 @@ struct sh_css_sp_debug_command {
>  	u32 dma_sw_reg;
>  };
>  
> -#if !defined(ISP2401)
>  /* SP input formatter configuration.*/
>  struct sh_css_sp_input_formatter_set {
>  	u32				stream_format;
>  	input_formatter_cfg_t	config_a;
>  	input_formatter_cfg_t	config_b;
>  };
> -#endif
>  
>  #define IA_CSS_MIPI_SIZE_CHECK_MAX_NOF_ENTRIES_PER_PORT (3)
>  
> @@ -377,7 +374,7 @@ struct sh_css_sp_config {
>  	     frames are locked when their EOF event is successfully sent to the
>  	     host (true) or when they are passed to the preview/video pipe
>  	     (false). */
> -#if !defined(ISP2401)
> +
>  	struct {
>  		u8					a_changed;
>  		u8					b_changed;
> @@ -385,15 +382,13 @@ struct sh_css_sp_config {
>  		struct sh_css_sp_input_formatter_set
>  			set[SH_CSS_MAX_IF_CONFIGS]; /* CSI-2 port is used as index. */
>  	} input_formatter;
> -#endif
> -#if !defined(ISP2401)
> +
>  	sync_generator_cfg_t	sync_gen;
>  	tpg_cfg_t		tpg;
>  	prbs_cfg_t		prbs;
>  	input_system_cfg_t	input_circuit;
>  	u8			input_circuit_cfg_changed;
>  	u32		mipi_sizes_for_check[N_CSI_PORTS][IA_CSS_MIPI_SIZE_CHECK_MAX_NOF_ENTRIES_PER_PORT];
> -#endif
>  	u8                 enable_isys_event_queue;
>  	u8			disable_cont_vf;
>  };
> @@ -409,7 +404,6 @@ enum sh_css_stage_type {
>  #define SH_CSS_PIPE_CONFIG_SAMPLE_PARAMS_MASK \
>  	((SH_CSS_PIPE_CONFIG_SAMPLE_PARAMS << SH_CSS_MAX_SP_THREADS) - 1)
>  
> -#if defined(ISP2401)
>  struct sh_css_sp_pipeline_terminal {
>  	union {
>  		/* Input System 2401 */
> @@ -442,7 +436,6 @@ struct sh_css_sp_pipeline_io_status {
>  	u32	running[N_INPUT_SYSTEM_CSI_PORT];	/** configured streams */
>  };
>  
> -#endif
>  enum sh_css_port_dir {
>  	SH_CSS_PORT_INPUT  = 0,
>  	SH_CSS_PORT_OUTPUT  = 1
> @@ -641,10 +634,8 @@ struct sh_css_sp_stage {
>  struct sh_css_sp_group {
>  	struct sh_css_sp_config		config;
>  	struct sh_css_sp_pipeline	pipe[SH_CSS_MAX_SP_THREADS];
> -#if defined(ISP2401)
>  	struct sh_css_sp_pipeline_io	pipe_io[SH_CSS_MAX_SP_THREADS];
>  	struct sh_css_sp_pipeline_io_status	pipe_io_status;
> -#endif
>  	struct sh_css_sp_debug_command	debug;
>  };
>  
> @@ -922,13 +913,11 @@ sh_css_frame_info_set_width(struct ia_css_frame_info *info,
>  			    unsigned int width,
>  			    unsigned int aligned);
>  
> -#if !defined(ISP2401)
>  
>  unsigned int
>  sh_css_get_mipi_sizes_for_check(const unsigned int port,
>  				const unsigned int idx);
>  
> -#endif
>  
>  ia_css_ptr
>  sh_css_store_sp_group_to_ddr(void);
> @@ -971,11 +960,9 @@ sh_css_continuous_is_enabled(uint8_t pipe_num);
>  struct ia_css_pipe *
>  find_pipe_by_num(uint32_t pipe_num);
>  
> -#ifdef ISP2401
>  void
>  ia_css_get_crop_offsets(
>      struct ia_css_pipe *pipe,
>      struct ia_css_frame_info *in_frame);
> -#endif
>  
>  #endif /* _SH_CSS_INTERNAL_H_ */



Regards,

Hans

Re: [PATCH v2 1/3] media: atomisp: sh_css_internal: Unifying sh_css_sp_group to remove #ifdef ISP2401

[Kate Hsuan]

Hi Hans

Thank you for reviewing.


On Tue, Jun 13, 2023 at 5:34 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi Kate,
>
> Thank you for the new version.
>
> It seems that this is a mismatch of multiple previous v1 patches
> squashed into 1, so I'm afraid that this is going to need
> a version 3 to fix this.
>
> I see bits of both:
>
> [PATCH 1/3] media: atomisp: sh_css_internal: Unifying sh_css_sp_group to remove #ifdef ISP2401
> [PATCH 3/3] media: atomisp: ia_css_debug: remove unused codes
>
> from v1 of the series in here. Where I think this should be only patch 3/3,
> removing the unused ia_css_debug code as a prep patch for further changes.
>
> See below for details.
>
> On 6/12/23 07:57, Kate Hsuan wrote:
> > Since some parts of the data scructure elements are determined in compile
> > time, the configuration data structure should be compiled for both two
> > ISP models. In order to set the configuration for both ISP model in
> > runtime, Thesh_css_sp_group is unified to one data structure for the
> > configuration to ensure the data structure can be used for both ISP2400
> > and 2401. Some of the unsed codes from ia_css_debug.c are also removed.
>
> It seems that when squashing patches you have ended up re-using the wrong
> commit message for this one.
>
> Instead the commit msg of the v1:
> "[PATCH 3/3] media: atomisp: ia_css_debug: remove unused codes"
>
> patch should be used here.
>
>
> > Signed-off-by: Kate Hsuan <hpa@redhat.com>
> > ---
> >  .../runtime/debug/interface/ia_css_debug.h    |  6 --
> >  .../pci/runtime/debug/src/ia_css_debug.c      | 76 +------------------
> >  .../media/atomisp/pci/sh_css_internal.h       | 35 +++------
> >  3 files changed, 13 insertions(+), 104 deletions(-)
> >
> > diff --git a/drivers/staging/media/atomisp/pci/runtime/debug/interface/ia_css_debug.h b/drivers/staging/media/atomisp/pci/runtime/debug/interface/ia_css_debug.h
> > index fff89e9b4b01..3a3d72c6eaaa 100644
> > --- a/drivers/staging/media/atomisp/pci/runtime/debug/interface/ia_css_debug.h
> > +++ b/drivers/staging/media/atomisp/pci/runtime/debug/interface/ia_css_debug.h
> > @@ -141,12 +141,6 @@ static inline void __printf(2, 0) ia_css_debug_vdtrace(unsigned int level,
> >  __printf(2, 3) void ia_css_debug_dtrace(unsigned int level,
> >                                       const char *fmt, ...);
> >
> > -/*! @brief Dump sp thread's stack contents
> > - * SP thread's stack contents are set to 0xcafecafe. This function dumps the
> > - * stack to inspect if the stack's boundaries are compromised.
> > - * @return   None
> > - */
> > -void ia_css_debug_dump_sp_stack_info(void);
> >
> >  /*! @brief Function to set the global dtrace verbosity level.
> >   * @param[in]        trace_level     Maximum level of the messages to be traced.
> > diff --git a/drivers/staging/media/atomisp/pci/runtime/debug/src/ia_css_debug.c b/drivers/staging/media/atomisp/pci/runtime/debug/src/ia_css_debug.c
> > index bb6204cb42c5..bb30146c5fe7 100644
> > --- a/drivers/staging/media/atomisp/pci/runtime/debug/src/ia_css_debug.c
> > +++ b/drivers/staging/media/atomisp/pci/runtime/debug/src/ia_css_debug.c
>
> The patch-hunk starting here:
>
> > @@ -105,7 +105,8 @@
> >   * TODO:SH_CSS_MAX_SP_THREADS is not the max number of sp threads
> >   * future rework should fix this and remove the define MAX_THREAD_NUM
> >   */
> > -#define MAX_THREAD_NUM (SH_CSS_MAX_SP_THREADS + SH_CSS_MAX_SP_INTERNAL_THREADS)
> > +#define MAX_THREAD_NUM_2400 (SH_CSS_MAX_SP_THREADS + SH_CSS_MAX_SP_INTERNAL_THREADS_2400)
> > +#define MAX_THREAD_NUM_2401 (SH_CSS_MAX_SP_THREADS + SH_CSS_MAX_SP_INTERNAL_THREADS_2401)
> >
> >  static struct pipe_graph_class {
> >       bool do_init;
>
> And ending here, does not belong in this patch. Instead it should be squashed into patch 2/3
> which will result in patch 2/3 simply removing the original lines.
>
> > @@ -147,79 +148,6 @@ void ia_css_debug_dtrace(unsigned int level, const char *fmt, ...)
> >       va_end(ap);
> >  }
> >
> > -static void debug_dump_long_array_formatted(
> > -    const sp_ID_t sp_id,
> > -    hrt_address stack_sp_addr,
> > -    unsigned int stack_size)
> > -{
> > -     unsigned int i;
> > -     u32 val;
> > -     u32 addr = (uint32_t)stack_sp_addr;
> > -     u32 stack_size_words = CEIL_DIV(stack_size, sizeof(uint32_t));
> > -
> > -     /* When size is not multiple of four, last word is only relevant for
> > -      * remaining bytes */
> > -     for (i = 0; i < stack_size_words; i++) {
> > -             val = sp_dmem_load_uint32(sp_id, (hrt_address)addr);
> > -             if ((i % 8) == 0)
> > -                     ia_css_debug_dtrace(IA_CSS_DEBUG_VERBOSE, "\n");
> > -
> > -             ia_css_debug_dtrace(IA_CSS_DEBUG_VERBOSE, "0x%08x ", val);
> > -             addr += sizeof(uint32_t);
> > -     }
> > -
> > -     ia_css_debug_dtrace(IA_CSS_DEBUG_VERBOSE, "\n");
> > -}
> > -
> > -static void debug_dump_sp_stack_info(
> > -    const sp_ID_t sp_id)
> > -{
> > -     const struct ia_css_fw_info *fw;
> > -     unsigned int HIVE_ADDR_sp_threads_stack;
> > -     unsigned int HIVE_ADDR_sp_threads_stack_size;
> > -     u32 stack_sizes[MAX_THREAD_NUM];
> > -     u32 stack_sp_addr[MAX_THREAD_NUM];
> > -     unsigned int i;
> > -
> > -     fw = &sh_css_sp_fw;
> > -
> > -     ia_css_debug_dtrace(IA_CSS_DEBUG_VERBOSE, "sp_id(%u) stack info\n", sp_id);
> > -     ia_css_debug_dtrace(IA_CSS_DEBUG_VERBOSE,
> > -                         "from objects stack_addr_offset:0x%x stack_size_offset:0x%x\n",
> > -                         fw->info.sp.threads_stack,
> > -                         fw->info.sp.threads_stack_size);
> > -
> > -     HIVE_ADDR_sp_threads_stack = fw->info.sp.threads_stack;
> > -     HIVE_ADDR_sp_threads_stack_size = fw->info.sp.threads_stack_size;
> > -
> > -     if (fw->info.sp.threads_stack == 0 ||
> > -         fw->info.sp.threads_stack_size == 0)
> > -             return;
> > -
> > -     (void)HIVE_ADDR_sp_threads_stack;
> > -     (void)HIVE_ADDR_sp_threads_stack_size;
> > -
> > -     sp_dmem_load(sp_id,
> > -                  (unsigned int)sp_address_of(sp_threads_stack),
> > -                  &stack_sp_addr, sizeof(stack_sp_addr));
> > -     sp_dmem_load(sp_id,
> > -                  (unsigned int)sp_address_of(sp_threads_stack_size),
> > -                  &stack_sizes, sizeof(stack_sizes));
> > -
> > -     for (i = 0 ; i < MAX_THREAD_NUM; i++) {
> > -             ia_css_debug_dtrace(IA_CSS_DEBUG_VERBOSE,
> > -                                 "thread: %u stack_addr: 0x%08x stack_size: %u\n",
> > -                                 i, stack_sp_addr[i], stack_sizes[i]);
> > -             debug_dump_long_array_formatted(sp_id, (hrt_address)stack_sp_addr[i],
> > -                                             stack_sizes[i]);
> > -     }
> > -}
> > -
> > -void ia_css_debug_dump_sp_stack_info(void)
> > -{
> > -     debug_dump_sp_stack_info(SP0_ID);
> > -}
> > -
> >  void ia_css_debug_set_dtrace_level(const unsigned int trace_level)
> >  {
> >       dbg_level = trace_level;
>
> All the sh_css_internal.h changes below should be
> squashed into / moved to:
>
> [PATCH v2 3/3] atomisp: sh_css_params: write the sp_group config according to the ISP model

Sorry for the mistakes.
I'll start over again and all the fixes will be based on v1 patch.

Here is my plan for the v3 patch.
patch 1/3 will squash the changes of  sh_css_params.c and
sh_css_internal.h into one patch.
patch 2/3 will remove the unused code and remove the unused variable
in sh_css_internal.h.
patch 3/3 will remove the debug code for HAS_WATCHDOG_SP_THREAD_DEBUG.

>
> > diff --git a/drivers/staging/media/atomisp/pci/sh_css_internal.h b/drivers/staging/media/atomisp/pci/sh_css_internal.h
> > index d98f1323441e..2fa0b3e45fe0 100644
> > --- a/drivers/staging/media/atomisp/pci/sh_css_internal.h
> > +++ b/drivers/staging/media/atomisp/pci/sh_css_internal.h
> > @@ -22,9 +22,7 @@
> >  #include <platform_support.h>
> >  #include <linux/stdarg.h>
> >
> > -#if !defined(ISP2401)
> >  #include "input_formatter.h"
> > -#endif
> >  #include "input_system.h"
> >
> >  #include "ia_css_types.h"
> > @@ -95,19 +93,20 @@
> >   *    these threads can't be used as image pipe
> >   */
> >
> > -#if !defined(ISP2401)
> > -#define SH_CSS_SP_INTERNAL_METADATA_THREAD   1
> > -#else
> > -#define SH_CSS_SP_INTERNAL_METADATA_THREAD   0
> > -#endif
> > +#define SH_CSS_SP_INTERNAL_METADATA_THREAD_2400      1
> > +#define SH_CSS_SP_INTERNAL_METADATA_THREAD_2401      0
> >
> >  #define SH_CSS_SP_INTERNAL_SERVICE_THREAD            1
> >
> >  #define SH_CSS_MAX_SP_THREADS                5
> >
> > -#define SH_CSS_MAX_SP_INTERNAL_THREADS       (\
> > -      SH_CSS_SP_INTERNAL_SERVICE_THREAD +\
> > -      SH_CSS_SP_INTERNAL_METADATA_THREAD)
> > +#define SH_CSS_MAX_SP_INTERNAL_THREADS_2400  (\
> > +     SH_CSS_SP_INTERNAL_SERVICE_THREAD +\
> > +      SH_CSS_SP_INTERNAL_METADATA_THREAD_2400)
> > +
> > +#define SH_CSS_MAX_SP_INTERNAL_THREADS_2401  (\
> > +     SH_CSS_SP_INTERNAL_SERVICE_THREAD +\
> > +      SH_CSS_SP_INTERNAL_METADATA_THREAD_2401)
> >
> >  #define SH_CSS_MAX_PIPELINES SH_CSS_MAX_SP_THREADS
> >
> > @@ -357,14 +356,12 @@ struct sh_css_sp_debug_command {
> >       u32 dma_sw_reg;
> >  };
> >
> > -#if !defined(ISP2401)
> >  /* SP input formatter configuration.*/
> >  struct sh_css_sp_input_formatter_set {
> >       u32                             stream_format;
> >       input_formatter_cfg_t   config_a;
> >       input_formatter_cfg_t   config_b;
> >  };
> > -#endif
> >
> >  #define IA_CSS_MIPI_SIZE_CHECK_MAX_NOF_ENTRIES_PER_PORT (3)
> >
> > @@ -377,7 +374,7 @@ struct sh_css_sp_config {
> >            frames are locked when their EOF event is successfully sent to the
> >            host (true) or when they are passed to the preview/video pipe
> >            (false). */
> > -#if !defined(ISP2401)
> > +
> >       struct {
> >               u8                                      a_changed;
> >               u8                                      b_changed;
> > @@ -385,15 +382,13 @@ struct sh_css_sp_config {
> >               struct sh_css_sp_input_formatter_set
> >                       set[SH_CSS_MAX_IF_CONFIGS]; /* CSI-2 port is used as index. */
> >       } input_formatter;
> > -#endif
> > -#if !defined(ISP2401)
> > +
> >       sync_generator_cfg_t    sync_gen;
> >       tpg_cfg_t               tpg;
> >       prbs_cfg_t              prbs;
> >       input_system_cfg_t      input_circuit;
> >       u8                      input_circuit_cfg_changed;
> >       u32             mipi_sizes_for_check[N_CSI_PORTS][IA_CSS_MIPI_SIZE_CHECK_MAX_NOF_ENTRIES_PER_PORT];
> > -#endif
> >       u8                 enable_isys_event_queue;
> >       u8                      disable_cont_vf;
> >  };
> > @@ -409,7 +404,6 @@ enum sh_css_stage_type {
> >  #define SH_CSS_PIPE_CONFIG_SAMPLE_PARAMS_MASK \
> >       ((SH_CSS_PIPE_CONFIG_SAMPLE_PARAMS << SH_CSS_MAX_SP_THREADS) - 1)
> >
> > -#if defined(ISP2401)
> >  struct sh_css_sp_pipeline_terminal {
> >       union {
> >               /* Input System 2401 */
> > @@ -442,7 +436,6 @@ struct sh_css_sp_pipeline_io_status {
> >       u32     running[N_INPUT_SYSTEM_CSI_PORT];       /** configured streams */
> >  };
> >
> > -#endif
> >  enum sh_css_port_dir {
> >       SH_CSS_PORT_INPUT  = 0,
> >       SH_CSS_PORT_OUTPUT  = 1
> > @@ -641,10 +634,8 @@ struct sh_css_sp_stage {
> >  struct sh_css_sp_group {
> >       struct sh_css_sp_config         config;
> >       struct sh_css_sp_pipeline       pipe[SH_CSS_MAX_SP_THREADS];
> > -#if defined(ISP2401)
> >       struct sh_css_sp_pipeline_io    pipe_io[SH_CSS_MAX_SP_THREADS];
> >       struct sh_css_sp_pipeline_io_status     pipe_io_status;
> > -#endif
> >       struct sh_css_sp_debug_command  debug;
> >  };
> >
> > @@ -922,13 +913,11 @@ sh_css_frame_info_set_width(struct ia_css_frame_info *info,
> >                           unsigned int width,
> >                           unsigned int aligned);
> >
> > -#if !defined(ISP2401)
> >
> >  unsigned int
> >  sh_css_get_mipi_sizes_for_check(const unsigned int port,
> >                               const unsigned int idx);
> >
> > -#endif
> >
> >  ia_css_ptr
> >  sh_css_store_sp_group_to_ddr(void);
> > @@ -971,11 +960,9 @@ sh_css_continuous_is_enabled(uint8_t pipe_num);
> >  struct ia_css_pipe *
> >  find_pipe_by_num(uint32_t pipe_num);
> >
> > -#ifdef ISP2401
> >  void
> >  ia_css_get_crop_offsets(
> >      struct ia_css_pipe *pipe,
> >      struct ia_css_frame_info *in_frame);
> > -#endif
> >
> >  #endif /* _SH_CSS_INTERNAL_H_ */
>
>
>
> Regards,
>
> Hans
>
>


--
BR,
Kate

Re: [PATCH v2 1/3] media: atomisp: sh_css_internal: Unifying sh_css_sp_group to remove #ifdef ISP2401

[Kate Hsuan]

On Fri, Jun 16, 2023 at 2:08 PM Kate Hsuan <hpa@redhat.com> wrote:
>
> Hi Hans
>
> Thank you for reviewing.
>
>
> On Tue, Jun 13, 2023 at 5:34 PM Hans de Goede <hdegoede@redhat.com> wrote:
> >
> > Hi Kate,
> >
> > Thank you for the new version.
> >
> > It seems that this is a mismatch of multiple previous v1 patches
> > squashed into 1, so I'm afraid that this is going to need
> > a version 3 to fix this.
> >
> > I see bits of both:
> >
> > [PATCH 1/3] media: atomisp: sh_css_internal: Unifying sh_css_sp_group to remove #ifdef ISP2401
> > [PATCH 3/3] media: atomisp: ia_css_debug: remove unused codes
> >
> > from v1 of the series in here. Where I think this should be only patch 3/3,
> > removing the unused ia_css_debug code as a prep patch for further changes.
> >
> > See below for details.
> >
> > On 6/12/23 07:57, Kate Hsuan wrote:
> > > Since some parts of the data scructure elements are determined in compile
> > > time, the configuration data structure should be compiled for both two
> > > ISP models. In order to set the configuration for both ISP model in
> > > runtime, Thesh_css_sp_group is unified to one data structure for the
> > > configuration to ensure the data structure can be used for both ISP2400
> > > and 2401. Some of the unsed codes from ia_css_debug.c are also removed.
> >
> > It seems that when squashing patches you have ended up re-using the wrong
> > commit message for this one.
> >
> > Instead the commit msg of the v1:
> > "[PATCH 3/3] media: atomisp: ia_css_debug: remove unused codes"
> >
> > patch should be used here.
> >
> >
> > > Signed-off-by: Kate Hsuan <hpa@redhat.com>
> > > ---
> > >  .../runtime/debug/interface/ia_css_debug.h    |  6 --
> > >  .../pci/runtime/debug/src/ia_css_debug.c      | 76 +------------------
> > >  .../media/atomisp/pci/sh_css_internal.h       | 35 +++------
> > >  3 files changed, 13 insertions(+), 104 deletions(-)
> > >
> > > diff --git a/drivers/staging/media/atomisp/pci/runtime/debug/interface/ia_css_debug.h b/drivers/staging/media/atomisp/pci/runtime/debug/interface/ia_css_debug.h
> > > index fff89e9b4b01..3a3d72c6eaaa 100644
> > > --- a/drivers/staging/media/atomisp/pci/runtime/debug/interface/ia_css_debug.h
> > > +++ b/drivers/staging/media/atomisp/pci/runtime/debug/interface/ia_css_debug.h
> > > @@ -141,12 +141,6 @@ static inline void __printf(2, 0) ia_css_debug_vdtrace(unsigned int level,
> > >  __printf(2, 3) void ia_css_debug_dtrace(unsigned int level,
> > >                                       const char *fmt, ...);
> > >
> > > -/*! @brief Dump sp thread's stack contents
> > > - * SP thread's stack contents are set to 0xcafecafe. This function dumps the
> > > - * stack to inspect if the stack's boundaries are compromised.
> > > - * @return   None
> > > - */
> > > -void ia_css_debug_dump_sp_stack_info(void);
> > >
> > >  /*! @brief Function to set the global dtrace verbosity level.
> > >   * @param[in]        trace_level     Maximum level of the messages to be traced.
> > > diff --git a/drivers/staging/media/atomisp/pci/runtime/debug/src/ia_css_debug.c b/drivers/staging/media/atomisp/pci/runtime/debug/src/ia_css_debug.c
> > > index bb6204cb42c5..bb30146c5fe7 100644
> > > --- a/drivers/staging/media/atomisp/pci/runtime/debug/src/ia_css_debug.c
> > > +++ b/drivers/staging/media/atomisp/pci/runtime/debug/src/ia_css_debug.c
> >
> > The patch-hunk starting here:
> >
> > > @@ -105,7 +105,8 @@
> > >   * TODO:SH_CSS_MAX_SP_THREADS is not the max number of sp threads
> > >   * future rework should fix this and remove the define MAX_THREAD_NUM
> > >   */
> > > -#define MAX_THREAD_NUM (SH_CSS_MAX_SP_THREADS + SH_CSS_MAX_SP_INTERNAL_THREADS)
> > > +#define MAX_THREAD_NUM_2400 (SH_CSS_MAX_SP_THREADS + SH_CSS_MAX_SP_INTERNAL_THREADS_2400)
> > > +#define MAX_THREAD_NUM_2401 (SH_CSS_MAX_SP_THREADS + SH_CSS_MAX_SP_INTERNAL_THREADS_2401)
> > >
> > >  static struct pipe_graph_class {
> > >       bool do_init;
> >
> > And ending here, does not belong in this patch. Instead it should be squashed into patch 2/3
> > which will result in patch 2/3 simply removing the original lines.
> >
> > > @@ -147,79 +148,6 @@ void ia_css_debug_dtrace(unsigned int level, const char *fmt, ...)
> > >       va_end(ap);
> > >  }
> > >
> > > -static void debug_dump_long_array_formatted(
> > > -    const sp_ID_t sp_id,
> > > -    hrt_address stack_sp_addr,
> > > -    unsigned int stack_size)
> > > -{
> > > -     unsigned int i;
> > > -     u32 val;
> > > -     u32 addr = (uint32_t)stack_sp_addr;
> > > -     u32 stack_size_words = CEIL_DIV(stack_size, sizeof(uint32_t));
> > > -
> > > -     /* When size is not multiple of four, last word is only relevant for
> > > -      * remaining bytes */
> > > -     for (i = 0; i < stack_size_words; i++) {
> > > -             val = sp_dmem_load_uint32(sp_id, (hrt_address)addr);
> > > -             if ((i % 8) == 0)
> > > -                     ia_css_debug_dtrace(IA_CSS_DEBUG_VERBOSE, "\n");
> > > -
> > > -             ia_css_debug_dtrace(IA_CSS_DEBUG_VERBOSE, "0x%08x ", val);
> > > -             addr += sizeof(uint32_t);
> > > -     }
> > > -
> > > -     ia_css_debug_dtrace(IA_CSS_DEBUG_VERBOSE, "\n");
> > > -}
> > > -
> > > -static void debug_dump_sp_stack_info(
> > > -    const sp_ID_t sp_id)
> > > -{
> > > -     const struct ia_css_fw_info *fw;
> > > -     unsigned int HIVE_ADDR_sp_threads_stack;
> > > -     unsigned int HIVE_ADDR_sp_threads_stack_size;
> > > -     u32 stack_sizes[MAX_THREAD_NUM];
> > > -     u32 stack_sp_addr[MAX_THREAD_NUM];
> > > -     unsigned int i;
> > > -
> > > -     fw = &sh_css_sp_fw;
> > > -
> > > -     ia_css_debug_dtrace(IA_CSS_DEBUG_VERBOSE, "sp_id(%u) stack info\n", sp_id);
> > > -     ia_css_debug_dtrace(IA_CSS_DEBUG_VERBOSE,
> > > -                         "from objects stack_addr_offset:0x%x stack_size_offset:0x%x\n",
> > > -                         fw->info.sp.threads_stack,
> > > -                         fw->info.sp.threads_stack_size);
> > > -
> > > -     HIVE_ADDR_sp_threads_stack = fw->info.sp.threads_stack;
> > > -     HIVE_ADDR_sp_threads_stack_size = fw->info.sp.threads_stack_size;
> > > -
> > > -     if (fw->info.sp.threads_stack == 0 ||
> > > -         fw->info.sp.threads_stack_size == 0)
> > > -             return;
> > > -
> > > -     (void)HIVE_ADDR_sp_threads_stack;
> > > -     (void)HIVE_ADDR_sp_threads_stack_size;
> > > -
> > > -     sp_dmem_load(sp_id,
> > > -                  (unsigned int)sp_address_of(sp_threads_stack),
> > > -                  &stack_sp_addr, sizeof(stack_sp_addr));
> > > -     sp_dmem_load(sp_id,
> > > -                  (unsigned int)sp_address_of(sp_threads_stack_size),
> > > -                  &stack_sizes, sizeof(stack_sizes));
> > > -
> > > -     for (i = 0 ; i < MAX_THREAD_NUM; i++) {
> > > -             ia_css_debug_dtrace(IA_CSS_DEBUG_VERBOSE,
> > > -                                 "thread: %u stack_addr: 0x%08x stack_size: %u\n",
> > > -                                 i, stack_sp_addr[i], stack_sizes[i]);
> > > -             debug_dump_long_array_formatted(sp_id, (hrt_address)stack_sp_addr[i],
> > > -                                             stack_sizes[i]);
> > > -     }
> > > -}
> > > -
> > > -void ia_css_debug_dump_sp_stack_info(void)
> > > -{
> > > -     debug_dump_sp_stack_info(SP0_ID);
> > > -}
> > > -
> > >  void ia_css_debug_set_dtrace_level(const unsigned int trace_level)
> > >  {
> > >       dbg_level = trace_level;
> >
> > All the sh_css_internal.h changes below should be
> > squashed into / moved to:
> >
> > [PATCH v2 3/3] atomisp: sh_css_params: write the sp_group config according to the ISP model
>
> Sorry for the mistakes.
> I'll start over again and all the fixes will be based on v1 patch.
>
> Here is my plan for the v3 patch.
> patch 1/3 will squash the changes of  sh_css_params.c and
> sh_css_internal.h into one patch.
> patch 2/3 will remove the unused code and remove the unused variable
> in sh_css_internal.h.
> patch 3/3 will remove the debug code for HAS_WATCHDOG_SP_THREAD_DEBUG.
>

It should be
patch 1/3 will remove the unused code and remove the unused variable
in sh_css_internal.h.
patch 2/3 will remove the debug code for HAS_WATCHDOG_SP_THREAD_DEBUG.
patch 3/3 will squash the changes of  sh_css_params.c and
sh_css_internal.h into one patch.

> >
> > > diff --git a/drivers/staging/media/atomisp/pci/sh_css_internal.h b/drivers/staging/media/atomisp/pci/sh_css_internal.h
> > > index d98f1323441e..2fa0b3e45fe0 100644
> > > --- a/drivers/staging/media/atomisp/pci/sh_css_internal.h
> > > +++ b/drivers/staging/media/atomisp/pci/sh_css_internal.h
> > > @@ -22,9 +22,7 @@
> > >  #include <platform_support.h>
> > >  #include <linux/stdarg.h>
> > >
> > > -#if !defined(ISP2401)
> > >  #include "input_formatter.h"
> > > -#endif
> > >  #include "input_system.h"
> > >
> > >  #include "ia_css_types.h"
> > > @@ -95,19 +93,20 @@
> > >   *    these threads can't be used as image pipe
> > >   */
> > >
> > > -#if !defined(ISP2401)
> > > -#define SH_CSS_SP_INTERNAL_METADATA_THREAD   1
> > > -#else
> > > -#define SH_CSS_SP_INTERNAL_METADATA_THREAD   0
> > > -#endif
> > > +#define SH_CSS_SP_INTERNAL_METADATA_THREAD_2400      1
> > > +#define SH_CSS_SP_INTERNAL_METADATA_THREAD_2401      0
> > >
> > >  #define SH_CSS_SP_INTERNAL_SERVICE_THREAD            1
> > >
> > >  #define SH_CSS_MAX_SP_THREADS                5
> > >
> > > -#define SH_CSS_MAX_SP_INTERNAL_THREADS       (\
> > > -      SH_CSS_SP_INTERNAL_SERVICE_THREAD +\
> > > -      SH_CSS_SP_INTERNAL_METADATA_THREAD)
> > > +#define SH_CSS_MAX_SP_INTERNAL_THREADS_2400  (\
> > > +     SH_CSS_SP_INTERNAL_SERVICE_THREAD +\
> > > +      SH_CSS_SP_INTERNAL_METADATA_THREAD_2400)
> > > +
> > > +#define SH_CSS_MAX_SP_INTERNAL_THREADS_2401  (\
> > > +     SH_CSS_SP_INTERNAL_SERVICE_THREAD +\
> > > +      SH_CSS_SP_INTERNAL_METADATA_THREAD_2401)
> > >
> > >  #define SH_CSS_MAX_PIPELINES SH_CSS_MAX_SP_THREADS
> > >
> > > @@ -357,14 +356,12 @@ struct sh_css_sp_debug_command {
> > >       u32 dma_sw_reg;
> > >  };
> > >
> > > -#if !defined(ISP2401)
> > >  /* SP input formatter configuration.*/
> > >  struct sh_css_sp_input_formatter_set {
> > >       u32                             stream_format;
> > >       input_formatter_cfg_t   config_a;
> > >       input_formatter_cfg_t   config_b;
> > >  };
> > > -#endif
> > >
> > >  #define IA_CSS_MIPI_SIZE_CHECK_MAX_NOF_ENTRIES_PER_PORT (3)
> > >
> > > @@ -377,7 +374,7 @@ struct sh_css_sp_config {
> > >            frames are locked when their EOF event is successfully sent to the
> > >            host (true) or when they are passed to the preview/video pipe
> > >            (false). */
> > > -#if !defined(ISP2401)
> > > +
> > >       struct {
> > >               u8                                      a_changed;
> > >               u8                                      b_changed;
> > > @@ -385,15 +382,13 @@ struct sh_css_sp_config {
> > >               struct sh_css_sp_input_formatter_set
> > >                       set[SH_CSS_MAX_IF_CONFIGS]; /* CSI-2 port is used as index. */
> > >       } input_formatter;
> > > -#endif
> > > -#if !defined(ISP2401)
> > > +
> > >       sync_generator_cfg_t    sync_gen;
> > >       tpg_cfg_t               tpg;
> > >       prbs_cfg_t              prbs;
> > >       input_system_cfg_t      input_circuit;
> > >       u8                      input_circuit_cfg_changed;
> > >       u32             mipi_sizes_for_check[N_CSI_PORTS][IA_CSS_MIPI_SIZE_CHECK_MAX_NOF_ENTRIES_PER_PORT];
> > > -#endif
> > >       u8                 enable_isys_event_queue;
> > >       u8                      disable_cont_vf;
> > >  };
> > > @@ -409,7 +404,6 @@ enum sh_css_stage_type {
> > >  #define SH_CSS_PIPE_CONFIG_SAMPLE_PARAMS_MASK \
> > >       ((SH_CSS_PIPE_CONFIG_SAMPLE_PARAMS << SH_CSS_MAX_SP_THREADS) - 1)
> > >
> > > -#if defined(ISP2401)
> > >  struct sh_css_sp_pipeline_terminal {
> > >       union {
> > >               /* Input System 2401 */
> > > @@ -442,7 +436,6 @@ struct sh_css_sp_pipeline_io_status {
> > >       u32     running[N_INPUT_SYSTEM_CSI_PORT];       /** configured streams */
> > >  };
> > >
> > > -#endif
> > >  enum sh_css_port_dir {
> > >       SH_CSS_PORT_INPUT  = 0,
> > >       SH_CSS_PORT_OUTPUT  = 1
> > > @@ -641,10 +634,8 @@ struct sh_css_sp_stage {
> > >  struct sh_css_sp_group {
> > >       struct sh_css_sp_config         config;
> > >       struct sh_css_sp_pipeline       pipe[SH_CSS_MAX_SP_THREADS];
> > > -#if defined(ISP2401)
> > >       struct sh_css_sp_pipeline_io    pipe_io[SH_CSS_MAX_SP_THREADS];
> > >       struct sh_css_sp_pipeline_io_status     pipe_io_status;
> > > -#endif
> > >       struct sh_css_sp_debug_command  debug;
> > >  };
> > >
> > > @@ -922,13 +913,11 @@ sh_css_frame_info_set_width(struct ia_css_frame_info *info,
> > >                           unsigned int width,
> > >                           unsigned int aligned);
> > >
> > > -#if !defined(ISP2401)
> > >
> > >  unsigned int
> > >  sh_css_get_mipi_sizes_for_check(const unsigned int port,
> > >                               const unsigned int idx);
> > >
> > > -#endif
> > >
> > >  ia_css_ptr
> > >  sh_css_store_sp_group_to_ddr(void);
> > > @@ -971,11 +960,9 @@ sh_css_continuous_is_enabled(uint8_t pipe_num);
> > >  struct ia_css_pipe *
> > >  find_pipe_by_num(uint32_t pipe_num);
> > >
> > > -#ifdef ISP2401
> > >  void
> > >  ia_css_get_crop_offsets(
> > >      struct ia_css_pipe *pipe,
> > >      struct ia_css_frame_info *in_frame);
> > > -#endif
> > >
> > >  #endif /* _SH_CSS_INTERNAL_H_ */
> >
> >
> >
> > Regards,
> >
> > Hans
> >
> >
>
>
> --
> BR,
> Kate



-- 
BR,
Kate

Re: [PATCH v2 1/3] media: atomisp: sh_css_internal: Unifying sh_css_sp_group to remove #ifdef ISP2401

[Hans de Goede]

Hi Kate,

On 6/16/23 08:18, Kate Hsuan wrote:
> On Fri, Jun 16, 2023 at 2:08 PM Kate Hsuan <hpa@redhat.com> wrote:
>>
>> Hi Hans
>>
>> Thank you for reviewing.
>>
>>
>> On Tue, Jun 13, 2023 at 5:34 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>>
>>> Hi Kate,
>>>
>>> Thank you for the new version.
>>>
>>> It seems that this is a mismatch of multiple previous v1 patches
>>> squashed into 1, so I'm afraid that this is going to need
>>> a version 3 to fix this.
>>>
>>> I see bits of both:
>>>
>>> [PATCH 1/3] media: atomisp: sh_css_internal: Unifying sh_css_sp_group to remove #ifdef ISP2401
>>> [PATCH 3/3] media: atomisp: ia_css_debug: remove unused codes
>>>
>>> from v1 of the series in here. Where I think this should be only patch 3/3,
>>> removing the unused ia_css_debug code as a prep patch for further changes.
>>>
>>> See below for details.
>>>
>>> On 6/12/23 07:57, Kate Hsuan wrote:
>>>> Since some parts of the data scructure elements are determined in compile
>>>> time, the configuration data structure should be compiled for both two
>>>> ISP models. In order to set the configuration for both ISP model in
>>>> runtime, Thesh_css_sp_group is unified to one data structure for the
>>>> configuration to ensure the data structure can be used for both ISP2400
>>>> and 2401. Some of the unsed codes from ia_css_debug.c are also removed.
>>>
>>> It seems that when squashing patches you have ended up re-using the wrong
>>> commit message for this one.
>>>
>>> Instead the commit msg of the v1:
>>> "[PATCH 3/3] media: atomisp: ia_css_debug: remove unused codes"
>>>
>>> patch should be used here.
>>>
>>>
>>>> Signed-off-by: Kate Hsuan <hpa@redhat.com>
>>>> ---
>>>>  .../runtime/debug/interface/ia_css_debug.h    |  6 --
>>>>  .../pci/runtime/debug/src/ia_css_debug.c      | 76 +------------------
>>>>  .../media/atomisp/pci/sh_css_internal.h       | 35 +++------
>>>>  3 files changed, 13 insertions(+), 104 deletions(-)
>>>>
>>>> diff --git a/drivers/staging/media/atomisp/pci/runtime/debug/interface/ia_css_debug.h b/drivers/staging/media/atomisp/pci/runtime/debug/interface/ia_css_debug.h
>>>> index fff89e9b4b01..3a3d72c6eaaa 100644
>>>> --- a/drivers/staging/media/atomisp/pci/runtime/debug/interface/ia_css_debug.h
>>>> +++ b/drivers/staging/media/atomisp/pci/runtime/debug/interface/ia_css_debug.h
>>>> @@ -141,12 +141,6 @@ static inline void __printf(2, 0) ia_css_debug_vdtrace(unsigned int level,
>>>>  __printf(2, 3) void ia_css_debug_dtrace(unsigned int level,
>>>>                                       const char *fmt, ...);
>>>>
>>>> -/*! @brief Dump sp thread's stack contents
>>>> - * SP thread's stack contents are set to 0xcafecafe. This function dumps the
>>>> - * stack to inspect if the stack's boundaries are compromised.
>>>> - * @return   None
>>>> - */
>>>> -void ia_css_debug_dump_sp_stack_info(void);
>>>>
>>>>  /*! @brief Function to set the global dtrace verbosity level.
>>>>   * @param[in]        trace_level     Maximum level of the messages to be traced.
>>>> diff --git a/drivers/staging/media/atomisp/pci/runtime/debug/src/ia_css_debug.c b/drivers/staging/media/atomisp/pci/runtime/debug/src/ia_css_debug.c
>>>> index bb6204cb42c5..bb30146c5fe7 100644
>>>> --- a/drivers/staging/media/atomisp/pci/runtime/debug/src/ia_css_debug.c
>>>> +++ b/drivers/staging/media/atomisp/pci/runtime/debug/src/ia_css_debug.c
>>>
>>> The patch-hunk starting here:
>>>
>>>> @@ -105,7 +105,8 @@
>>>>   * TODO:SH_CSS_MAX_SP_THREADS is not the max number of sp threads
>>>>   * future rework should fix this and remove the define MAX_THREAD_NUM
>>>>   */
>>>> -#define MAX_THREAD_NUM (SH_CSS_MAX_SP_THREADS + SH_CSS_MAX_SP_INTERNAL_THREADS)
>>>> +#define MAX_THREAD_NUM_2400 (SH_CSS_MAX_SP_THREADS + SH_CSS_MAX_SP_INTERNAL_THREADS_2400)
>>>> +#define MAX_THREAD_NUM_2401 (SH_CSS_MAX_SP_THREADS + SH_CSS_MAX_SP_INTERNAL_THREADS_2401)
>>>>
>>>>  static struct pipe_graph_class {
>>>>       bool do_init;
>>>
>>> And ending here, does not belong in this patch. Instead it should be squashed into patch 2/3
>>> which will result in patch 2/3 simply removing the original lines.
>>>
>>>> @@ -147,79 +148,6 @@ void ia_css_debug_dtrace(unsigned int level, const char *fmt, ...)
>>>>       va_end(ap);
>>>>  }
>>>>
>>>> -static void debug_dump_long_array_formatted(
>>>> -    const sp_ID_t sp_id,
>>>> -    hrt_address stack_sp_addr,
>>>> -    unsigned int stack_size)
>>>> -{
>>>> -     unsigned int i;
>>>> -     u32 val;
>>>> -     u32 addr = (uint32_t)stack_sp_addr;
>>>> -     u32 stack_size_words = CEIL_DIV(stack_size, sizeof(uint32_t));
>>>> -
>>>> -     /* When size is not multiple of four, last word is only relevant for
>>>> -      * remaining bytes */
>>>> -     for (i = 0; i < stack_size_words; i++) {
>>>> -             val = sp_dmem_load_uint32(sp_id, (hrt_address)addr);
>>>> -             if ((i % 8) == 0)
>>>> -                     ia_css_debug_dtrace(IA_CSS_DEBUG_VERBOSE, "\n");
>>>> -
>>>> -             ia_css_debug_dtrace(IA_CSS_DEBUG_VERBOSE, "0x%08x ", val);
>>>> -             addr += sizeof(uint32_t);
>>>> -     }
>>>> -
>>>> -     ia_css_debug_dtrace(IA_CSS_DEBUG_VERBOSE, "\n");
>>>> -}
>>>> -
>>>> -static void debug_dump_sp_stack_info(
>>>> -    const sp_ID_t sp_id)
>>>> -{
>>>> -     const struct ia_css_fw_info *fw;
>>>> -     unsigned int HIVE_ADDR_sp_threads_stack;
>>>> -     unsigned int HIVE_ADDR_sp_threads_stack_size;
>>>> -     u32 stack_sizes[MAX_THREAD_NUM];
>>>> -     u32 stack_sp_addr[MAX_THREAD_NUM];
>>>> -     unsigned int i;
>>>> -
>>>> -     fw = &sh_css_sp_fw;
>>>> -
>>>> -     ia_css_debug_dtrace(IA_CSS_DEBUG_VERBOSE, "sp_id(%u) stack info\n", sp_id);
>>>> -     ia_css_debug_dtrace(IA_CSS_DEBUG_VERBOSE,
>>>> -                         "from objects stack_addr_offset:0x%x stack_size_offset:0x%x\n",
>>>> -                         fw->info.sp.threads_stack,
>>>> -                         fw->info.sp.threads_stack_size);
>>>> -
>>>> -     HIVE_ADDR_sp_threads_stack = fw->info.sp.threads_stack;
>>>> -     HIVE_ADDR_sp_threads_stack_size = fw->info.sp.threads_stack_size;
>>>> -
>>>> -     if (fw->info.sp.threads_stack == 0 ||
>>>> -         fw->info.sp.threads_stack_size == 0)
>>>> -             return;
>>>> -
>>>> -     (void)HIVE_ADDR_sp_threads_stack;
>>>> -     (void)HIVE_ADDR_sp_threads_stack_size;
>>>> -
>>>> -     sp_dmem_load(sp_id,
>>>> -                  (unsigned int)sp_address_of(sp_threads_stack),
>>>> -                  &stack_sp_addr, sizeof(stack_sp_addr));
>>>> -     sp_dmem_load(sp_id,
>>>> -                  (unsigned int)sp_address_of(sp_threads_stack_size),
>>>> -                  &stack_sizes, sizeof(stack_sizes));
>>>> -
>>>> -     for (i = 0 ; i < MAX_THREAD_NUM; i++) {
>>>> -             ia_css_debug_dtrace(IA_CSS_DEBUG_VERBOSE,
>>>> -                                 "thread: %u stack_addr: 0x%08x stack_size: %u\n",
>>>> -                                 i, stack_sp_addr[i], stack_sizes[i]);
>>>> -             debug_dump_long_array_formatted(sp_id, (hrt_address)stack_sp_addr[i],
>>>> -                                             stack_sizes[i]);
>>>> -     }
>>>> -}
>>>> -
>>>> -void ia_css_debug_dump_sp_stack_info(void)
>>>> -{
>>>> -     debug_dump_sp_stack_info(SP0_ID);
>>>> -}
>>>> -
>>>>  void ia_css_debug_set_dtrace_level(const unsigned int trace_level)
>>>>  {
>>>>       dbg_level = trace_level;
>>>
>>> All the sh_css_internal.h changes below should be
>>> squashed into / moved to:
>>>
>>> [PATCH v2 3/3] atomisp: sh_css_params: write the sp_group config according to the ISP model
>>
>> Sorry for the mistakes.
>> I'll start over again and all the fixes will be based on v1 patch.
>>
>> Here is my plan for the v3 patch.
>> patch 1/3 will squash the changes of  sh_css_params.c and
>> sh_css_internal.h into one patch.
>> patch 2/3 will remove the unused code and remove the unused variable
>> in sh_css_internal.h.
>> patch 3/3 will remove the debug code for HAS_WATCHDOG_SP_THREAD_DEBUG.
>>
> 
> It should be
> patch 1/3 will remove the unused code and remove the unused variable
> in sh_css_internal.h.
> patch 2/3 will remove the debug code for HAS_WATCHDOG_SP_THREAD_DEBUG.
> patch 3/3 will squash the changes of  sh_css_params.c and
> sh_css_internal.h into one patch.

That sounds good to me.

Note instead of starting over you could also split the existing
patches into multiple patches and the apply those in the right
order.

E.g. turn v2 set into .patch files:

git format-patch HEAD~3

and then reset your tree to before the patches:

git reset --hard HEAD~3

Then start a new temp.patch file with:

--- a/drivers/staging/media/atomisp/pci/runtime/debug/src/ia_css_debug.c
+++ b/drivers/staging/media/atomisp/pci/runtime/debug/src/ia_css_debug.c

In there and then *move* the troublesome block modifying ia_css_debug.
from 0001-....patch to temp.patch, resulting in a temp.patch with just:

--- a/drivers/staging/media/atomisp/pci/runtime/debug/src/ia_css_debug.c
+++ b/drivers/staging/media/atomisp/pci/runtime/debug/src/ia_css_debug.c
@@ -105,7 +105,8 @@
  * TODO:SH_CSS_MAX_SP_THREADS is not the max number of sp threads
  * future rework should fix this and remove the define MAX_THREAD_NUM
  */
-#define MAX_THREAD_NUM (SH_CSS_MAX_SP_THREADS + SH_CSS_MAX_SP_INTERNAL_THREADS)
+#define MAX_THREAD_NUM_2400 (SH_CSS_MAX_SP_THREADS + SH_CSS_MAX_SP_INTERNAL_THREADS_2400)
+#define MAX_THREAD_NUM_2401 (SH_CSS_MAX_SP_THREADS + SH_CSS_MAX_SP_INTERNAL_THREADS_2401)
 
 static struct pipe_graph_class {
 	bool do_init;

In there. Likewise take all of the changes in 0001-...patch starting with:

--- a/drivers/staging/media/atomisp/pci/sh_css_internal.h
+++ b/drivers/staging/media/atomisp/pci/sh_css_internal.h

All the way to the end of 0001-...patch and move that entire block
to a new temp2.patch file.

Then do:

patch -p1 < 0001....patch

To apply the remaining changes in 0001....patch, which should now
just be the changes to "remove the unused code and remove the
unused variable > in sh_css_internal.h."

do "git diff" to check the results + test build and the do:

git commit -as

To get your first v3 commit without needing to actually re-write
anything.

Then do:

patch -p1 < temp.patch

To apply the changes moved there, which are needed as a base
for patch 0002 to apply cleanly and then do:

patch -p1 < 0002-....patch

do "git diff" to check the results + test build and the do:

git commit -as

To get your second v3 commit removing
the HAS_WATCHDOG_SP_THREAD_DEBUG stuff.

Last do:

patch -p1 < temp2.patch
patch -p1 < 0003....patch

to get the actual Unifying sh_css_sp_group to remove #ifdef ISP2401
changes applied and then again do:

do "git diff" to check the results + test build and the do:

git commit -as


This moving around hunks of patch files can be a good skill to
have when reviewers ask you to split a patch, or re-order
some things in a patch series.

I think there also is a way to do stuff (like committing
staged changed) in git on a hunk by hunk basis. But I'm
old-school and just edit .patch files when I need to do
this kinda stuff.

If you want to just redo all the changes that is fine too,
but learning how to move hunks of patches around is a good
skill to have.

Regards,

Hans















> 
>>>
>>>> diff --git a/drivers/staging/media/atomisp/pci/sh_css_internal.h b/drivers/staging/media/atomisp/pci/sh_css_internal.h
>>>> index d98f1323441e..2fa0b3e45fe0 100644
>>>> --- a/drivers/staging/media/atomisp/pci/sh_css_internal.h
>>>> +++ b/drivers/staging/media/atomisp/pci/sh_css_internal.h
>>>> @@ -22,9 +22,7 @@
>>>>  #include <platform_support.h>
>>>>  #include <linux/stdarg.h>
>>>>
>>>> -#if !defined(ISP2401)
>>>>  #include "input_formatter.h"
>>>> -#endif
>>>>  #include "input_system.h"
>>>>
>>>>  #include "ia_css_types.h"
>>>> @@ -95,19 +93,20 @@
>>>>   *    these threads can't be used as image pipe
>>>>   */
>>>>
>>>> -#if !defined(ISP2401)
>>>> -#define SH_CSS_SP_INTERNAL_METADATA_THREAD   1
>>>> -#else
>>>> -#define SH_CSS_SP_INTERNAL_METADATA_THREAD   0
>>>> -#endif
>>>> +#define SH_CSS_SP_INTERNAL_METADATA_THREAD_2400      1
>>>> +#define SH_CSS_SP_INTERNAL_METADATA_THREAD_2401      0
>>>>
>>>>  #define SH_CSS_SP_INTERNAL_SERVICE_THREAD            1
>>>>
>>>>  #define SH_CSS_MAX_SP_THREADS                5
>>>>
>>>> -#define SH_CSS_MAX_SP_INTERNAL_THREADS       (\
>>>> -      SH_CSS_SP_INTERNAL_SERVICE_THREAD +\
>>>> -      SH_CSS_SP_INTERNAL_METADATA_THREAD)
>>>> +#define SH_CSS_MAX_SP_INTERNAL_THREADS_2400  (\
>>>> +     SH_CSS_SP_INTERNAL_SERVICE_THREAD +\
>>>> +      SH_CSS_SP_INTERNAL_METADATA_THREAD_2400)
>>>> +
>>>> +#define SH_CSS_MAX_SP_INTERNAL_THREADS_2401  (\
>>>> +     SH_CSS_SP_INTERNAL_SERVICE_THREAD +\
>>>> +      SH_CSS_SP_INTERNAL_METADATA_THREAD_2401)
>>>>
>>>>  #define SH_CSS_MAX_PIPELINES SH_CSS_MAX_SP_THREADS
>>>>
>>>> @@ -357,14 +356,12 @@ struct sh_css_sp_debug_command {
>>>>       u32 dma_sw_reg;
>>>>  };
>>>>
>>>> -#if !defined(ISP2401)
>>>>  /* SP input formatter configuration.*/
>>>>  struct sh_css_sp_input_formatter_set {
>>>>       u32                             stream_format;
>>>>       input_formatter_cfg_t   config_a;
>>>>       input_formatter_cfg_t   config_b;
>>>>  };
>>>> -#endif
>>>>
>>>>  #define IA_CSS_MIPI_SIZE_CHECK_MAX_NOF_ENTRIES_PER_PORT (3)
>>>>
>>>> @@ -377,7 +374,7 @@ struct sh_css_sp_config {
>>>>            frames are locked when their EOF event is successfully sent to the
>>>>            host (true) or when they are passed to the preview/video pipe
>>>>            (false). */
>>>> -#if !defined(ISP2401)
>>>> +
>>>>       struct {
>>>>               u8                                      a_changed;
>>>>               u8                                      b_changed;
>>>> @@ -385,15 +382,13 @@ struct sh_css_sp_config {
>>>>               struct sh_css_sp_input_formatter_set
>>>>                       set[SH_CSS_MAX_IF_CONFIGS]; /* CSI-2 port is used as index. */
>>>>       } input_formatter;
>>>> -#endif
>>>> -#if !defined(ISP2401)
>>>> +
>>>>       sync_generator_cfg_t    sync_gen;
>>>>       tpg_cfg_t               tpg;
>>>>       prbs_cfg_t              prbs;
>>>>       input_system_cfg_t      input_circuit;
>>>>       u8                      input_circuit_cfg_changed;
>>>>       u32             mipi_sizes_for_check[N_CSI_PORTS][IA_CSS_MIPI_SIZE_CHECK_MAX_NOF_ENTRIES_PER_PORT];
>>>> -#endif
>>>>       u8                 enable_isys_event_queue;
>>>>       u8                      disable_cont_vf;
>>>>  };
>>>> @@ -409,7 +404,6 @@ enum sh_css_stage_type {
>>>>  #define SH_CSS_PIPE_CONFIG_SAMPLE_PARAMS_MASK \
>>>>       ((SH_CSS_PIPE_CONFIG_SAMPLE_PARAMS << SH_CSS_MAX_SP_THREADS) - 1)
>>>>
>>>> -#if defined(ISP2401)
>>>>  struct sh_css_sp_pipeline_terminal {
>>>>       union {
>>>>               /* Input System 2401 */
>>>> @@ -442,7 +436,6 @@ struct sh_css_sp_pipeline_io_status {
>>>>       u32     running[N_INPUT_SYSTEM_CSI_PORT];       /** configured streams */
>>>>  };
>>>>
>>>> -#endif
>>>>  enum sh_css_port_dir {
>>>>       SH_CSS_PORT_INPUT  = 0,
>>>>       SH_CSS_PORT_OUTPUT  = 1
>>>> @@ -641,10 +634,8 @@ struct sh_css_sp_stage {
>>>>  struct sh_css_sp_group {
>>>>       struct sh_css_sp_config         config;
>>>>       struct sh_css_sp_pipeline       pipe[SH_CSS_MAX_SP_THREADS];
>>>> -#if defined(ISP2401)
>>>>       struct sh_css_sp_pipeline_io    pipe_io[SH_CSS_MAX_SP_THREADS];
>>>>       struct sh_css_sp_pipeline_io_status     pipe_io_status;
>>>> -#endif
>>>>       struct sh_css_sp_debug_command  debug;
>>>>  };
>>>>
>>>> @@ -922,13 +913,11 @@ sh_css_frame_info_set_width(struct ia_css_frame_info *info,
>>>>                           unsigned int width,
>>>>                           unsigned int aligned);
>>>>
>>>> -#if !defined(ISP2401)
>>>>
>>>>  unsigned int
>>>>  sh_css_get_mipi_sizes_for_check(const unsigned int port,
>>>>                               const unsigned int idx);
>>>>
>>>> -#endif
>>>>
>>>>  ia_css_ptr
>>>>  sh_css_store_sp_group_to_ddr(void);
>>>> @@ -971,11 +960,9 @@ sh_css_continuous_is_enabled(uint8_t pipe_num);
>>>>  struct ia_css_pipe *
>>>>  find_pipe_by_num(uint32_t pipe_num);
>>>>
>>>> -#ifdef ISP2401
>>>>  void
>>>>  ia_css_get_crop_offsets(
>>>>      struct ia_css_pipe *pipe,
>>>>      struct ia_css_frame_info *in_frame);
>>>> -#endif
>>>>
>>>>  #endif /* _SH_CSS_INTERNAL_H_ */
>>>
>>>
>>>
>>> Regards,
>>>
>>> Hans
>>>
>>>
>>
>>
>> --
>> BR,
>> Kate
> 
> 
> 

[PATCH v2 2/3] media: atomisp: ia_css_debug: remove unused HAS_WATCHDOG_SP_THREAD_DEBUG codes

[Kate Hsuan]

Since the debug codes around #ifdef HAS_WATCHDOG_SP_THREAD_DEBUG are
no longer used, the debug implementation of this can be removed. Also,
the unsed variables are removed.

Signed-off-by: Kate Hsuan <hpa@redhat.com>
---
 .../pci/hive_isp_css_common/debug_global.h    |  7 --
 .../media/atomisp/pci/ia_css_acc_types.h      |  6 +-
 .../runtime/debug/interface/ia_css_debug.h    |  6 --
 .../pci/runtime/debug/src/ia_css_debug.c      | 93 -------------------
 .../media/atomisp/pci/sh_css_internal.h       | 24 -----
 5 files changed, 1 insertion(+), 135 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/hive_isp_css_common/debug_global.h b/drivers/staging/media/atomisp/pci/hive_isp_css_common/debug_global.h
index b6538beca18a..f2e17945fd45 100644
--- a/drivers/staging/media/atomisp/pci/hive_isp_css_common/debug_global.h
+++ b/drivers/staging/media/atomisp/pci/hive_isp_css_common/debug_global.h
@@ -35,13 +35,6 @@
 
 #define DEBUG_BUFFER_ISP_DMEM_ADDR       0x0
 
-/*
- * Enable HAS_WATCHDOG_SP_THREAD_DEBUG for additional SP thread and
- * pipe information on watchdog output
- * #undef HAS_WATCHDOG_SP_THREAD_DEBUG
- * #define HAS_WATCHDOG_SP_THREAD_DEBUG
- */
-
 /*
  * The linear buffer mode will accept data until the first
  * overflow and then stop accepting new data
diff --git a/drivers/staging/media/atomisp/pci/ia_css_acc_types.h b/drivers/staging/media/atomisp/pci/ia_css_acc_types.h
index a20879aedef6..d6e52b4971d6 100644
--- a/drivers/staging/media/atomisp/pci/ia_css_acc_types.h
+++ b/drivers/staging/media/atomisp/pci/ia_css_acc_types.h
@@ -331,11 +331,7 @@ struct ia_css_sp_info {
 	of DDR debug queue */
 	u32 perf_counter_input_system_error; /** input system perf
 	counter array */
-#ifdef HAS_WATCHDOG_SP_THREAD_DEBUG
-	u32 debug_wait; /** thread/pipe post mortem debug */
-	u32 debug_stage; /** thread/pipe post mortem debug */
-	u32 debug_stripe; /** thread/pipe post mortem debug */
-#endif
+
 	u32 threads_stack; /** sp thread's stack pointers */
 	u32 threads_stack_size; /** sp thread's stack sizes */
 	u32 curr_binary_id;        /** current binary id */
diff --git a/drivers/staging/media/atomisp/pci/runtime/debug/interface/ia_css_debug.h b/drivers/staging/media/atomisp/pci/runtime/debug/interface/ia_css_debug.h
index 3a3d72c6eaaa..efa136294836 100644
--- a/drivers/staging/media/atomisp/pci/runtime/debug/interface/ia_css_debug.h
+++ b/drivers/staging/media/atomisp/pci/runtime/debug/interface/ia_css_debug.h
@@ -306,12 +306,6 @@ void ia_css_debug_dump_isp_params(struct ia_css_stream *stream,
  */
 void ia_css_debug_dump_perf_counters(void);
 
-#ifdef HAS_WATCHDOG_SP_THREAD_DEBUG
-void sh_css_dump_thread_wait_info(void);
-void sh_css_dump_pipe_stage_info(void);
-void sh_css_dump_pipe_stripe_info(void);
-#endif
-
 void ia_css_debug_dump_isp_binary(void);
 
 void sh_css_dump_sp_raw_copy_linecount(bool reduced);
diff --git a/drivers/staging/media/atomisp/pci/runtime/debug/src/ia_css_debug.c b/drivers/staging/media/atomisp/pci/runtime/debug/src/ia_css_debug.c
index bb30146c5fe7..cf66a40bd6a4 100644
--- a/drivers/staging/media/atomisp/pci/runtime/debug/src/ia_css_debug.c
+++ b/drivers/staging/media/atomisp/pci/runtime/debug/src/ia_css_debug.c
@@ -101,13 +101,6 @@
 
 #define ENABLE_LINE_MAX_LENGTH (25)
 
-/*
- * TODO:SH_CSS_MAX_SP_THREADS is not the max number of sp threads
- * future rework should fix this and remove the define MAX_THREAD_NUM
- */
-#define MAX_THREAD_NUM_2400 (SH_CSS_MAX_SP_THREADS + SH_CSS_MAX_SP_INTERNAL_THREADS_2400)
-#define MAX_THREAD_NUM_2401 (SH_CSS_MAX_SP_THREADS + SH_CSS_MAX_SP_INTERNAL_THREADS_2401)
-
 static struct pipe_graph_class {
 	bool do_init;
 	int height;
@@ -2075,12 +2068,6 @@ void ia_css_debug_dump_debug_info(const char *context)
 	ia_css_debug_dump_isp_gdc_fifo_state();
 	ia_css_debug_dump_sp_state();
 	ia_css_debug_dump_perf_counters();
-
-#ifdef HAS_WATCHDOG_SP_THREAD_DEBUG
-	sh_css_dump_thread_wait_info();
-	sh_css_dump_pipe_stage_info();
-	sh_css_dump_pipe_stripe_info();
-#endif
 	ia_css_debug_dump_dma_isp_fifo_state();
 	ia_css_debug_dump_dma_sp_fifo_state();
 	ia_css_debug_dump_dma_state();
@@ -2392,86 +2379,6 @@ static void __printf(1, 2) dtrace_dot(const char *fmt, ...)
 	va_end(ap);
 }
 
-#ifdef HAS_WATCHDOG_SP_THREAD_DEBUG
-void sh_css_dump_thread_wait_info(void)
-{
-	const struct ia_css_fw_info *fw;
-	int i;
-	unsigned int HIVE_ADDR_sp_thread_wait;
-	s32 sp_thread_wait[MAX_THREAD_NUM];
-
-	ia_css_debug_dtrace(IA_CSS_DEBUG_VERBOSE, "SEM WAITS:\n");
-
-	fw = &sh_css_sp_fw;
-	HIVE_ADDR_sp_thread_wait =
-	    fw->info.sp.debug_wait;
-
-	(void)HIVE_ADDR_sp_thread_wait;
-
-	sp_dmem_load(SP0_ID,
-		     (unsigned int)sp_address_of(sp_thread_wait),
-		     &sp_thread_wait,
-		     sizeof(sp_thread_wait));
-	for (i = 0; i < MAX_THREAD_NUM; i++) {
-		ia_css_debug_dtrace(IA_CSS_DEBUG_VERBOSE,
-				    "\twait[%d] = 0x%X\n",
-				    i, sp_thread_wait[i]);
-	}
-}
-
-void sh_css_dump_pipe_stage_info(void)
-{
-	const struct ia_css_fw_info *fw;
-	int i;
-	unsigned int HIVE_ADDR_sp_pipe_stage;
-	s32 sp_pipe_stage[MAX_THREAD_NUM];
-
-	ia_css_debug_dtrace(IA_CSS_DEBUG_VERBOSE, "PIPE STAGE:\n");
-
-	fw = &sh_css_sp_fw;
-	HIVE_ADDR_sp_pipe_stage =
-	    fw->info.sp.debug_stage;
-
-	(void)HIVE_ADDR_sp_pipe_stage;
-
-	sp_dmem_load(SP0_ID,
-		     (unsigned int)sp_address_of(sp_pipe_stage),
-		     &sp_pipe_stage,
-		     sizeof(sp_pipe_stage));
-	for (i = 0; i < MAX_THREAD_NUM; i++) {
-		ia_css_debug_dtrace(IA_CSS_DEBUG_VERBOSE,
-				    "\tstage[%d] = %d\n",
-				    i, sp_pipe_stage[i]);
-	}
-}
-
-void sh_css_dump_pipe_stripe_info(void)
-{
-	const struct ia_css_fw_info *fw;
-	int i;
-	unsigned int HIVE_ADDR_sp_pipe_stripe;
-	s32 sp_pipe_stripe[MAX_THREAD_NUM];
-
-	ia_css_debug_dtrace(IA_CSS_DEBUG_VERBOSE, "PIPE STRIPE:\n");
-
-	fw = &sh_css_sp_fw;
-	HIVE_ADDR_sp_pipe_stripe =
-	    fw->info.sp.debug_stripe;
-
-	(void)HIVE_ADDR_sp_pipe_stripe;
-
-	sp_dmem_load(SP0_ID,
-		     (unsigned int)sp_address_of(sp_pipe_stripe),
-		     &sp_pipe_stripe,
-		     sizeof(sp_pipe_stripe));
-	for (i = 0; i < MAX_THREAD_NUM; i++) {
-		ia_css_debug_dtrace(IA_CSS_DEBUG_VERBOSE,
-				    "\tstripe[%d] = %d\n",
-				    i, sp_pipe_stripe[i]);
-	}
-}
-#endif
-
 static void
 ia_css_debug_pipe_graph_dump_frame(
     const struct ia_css_frame *frame,
diff --git a/drivers/staging/media/atomisp/pci/sh_css_internal.h b/drivers/staging/media/atomisp/pci/sh_css_internal.h
index 2fa0b3e45fe0..2349eb4d3767 100644
--- a/drivers/staging/media/atomisp/pci/sh_css_internal.h
+++ b/drivers/staging/media/atomisp/pci/sh_css_internal.h
@@ -84,32 +84,8 @@
 #define SH_CSS_MAX_IF_CONFIGS	3 /* Must match with IA_CSS_NR_OF_CONFIGS (not defined yet).*/
 #define SH_CSS_IF_CONFIG_NOT_NEEDED	0xFF
 
-/*
- * SH_CSS_MAX_SP_THREADS:
- *	 sp threads visible to host with connected communication queues
- *	 these threads are capable of running an image pipe
- * SH_CSS_MAX_SP_INTERNAL_THREADS:
- *	 internal sp service threads, no communication queues to host
- *	 these threads can't be used as image pipe
- */
-
-#define SH_CSS_SP_INTERNAL_METADATA_THREAD_2400	1
-#define SH_CSS_SP_INTERNAL_METADATA_THREAD_2401	0
-
-#define SH_CSS_SP_INTERNAL_SERVICE_THREAD		1
-
 #define SH_CSS_MAX_SP_THREADS		5
 
-#define SH_CSS_MAX_SP_INTERNAL_THREADS_2400	(\
-	SH_CSS_SP_INTERNAL_SERVICE_THREAD +\
-	 SH_CSS_SP_INTERNAL_METADATA_THREAD_2400)
-
-#define SH_CSS_MAX_SP_INTERNAL_THREADS_2401	(\
-	SH_CSS_SP_INTERNAL_SERVICE_THREAD +\
-	 SH_CSS_SP_INTERNAL_METADATA_THREAD_2401)
-
-#define SH_CSS_MAX_PIPELINES	SH_CSS_MAX_SP_THREADS
-
 /**
  * The C99 standard does not specify the exact object representation of structs;
  * the representation is compiler dependent.
-- 
2.40.1

Re: [PATCH v2 2/3] media: atomisp: ia_css_debug: remove unused HAS_WATCHDOG_SP_THREAD_DEBUG codes

[Hans de Goede]

Hi Kate,

On 6/12/23 07:57, Kate Hsuan wrote:
> Since the debug codes around #ifdef HAS_WATCHDOG_SP_THREAD_DEBUG are
> no longer used, the debug implementation of this can be removed. Also,
> the unsed variables are removed.
> 
> Signed-off-by: Kate Hsuan <hpa@redhat.com>
> ---
>  .../pci/hive_isp_css_common/debug_global.h    |  7 --
>  .../media/atomisp/pci/ia_css_acc_types.h      |  6 +-
>  .../runtime/debug/interface/ia_css_debug.h    |  6 --
>  .../pci/runtime/debug/src/ia_css_debug.c      | 93 -------------------
>  .../media/atomisp/pci/sh_css_internal.h       | 24 -----
>  5 files changed, 1 insertion(+), 135 deletions(-)
> 
> diff --git a/drivers/staging/media/atomisp/pci/hive_isp_css_common/debug_global.h b/drivers/staging/media/atomisp/pci/hive_isp_css_common/debug_global.h
> index b6538beca18a..f2e17945fd45 100644
> --- a/drivers/staging/media/atomisp/pci/hive_isp_css_common/debug_global.h
> +++ b/drivers/staging/media/atomisp/pci/hive_isp_css_common/debug_global.h
> @@ -35,13 +35,6 @@
>  
>  #define DEBUG_BUFFER_ISP_DMEM_ADDR       0x0
>  
> -/*
> - * Enable HAS_WATCHDOG_SP_THREAD_DEBUG for additional SP thread and
> - * pipe information on watchdog output
> - * #undef HAS_WATCHDOG_SP_THREAD_DEBUG
> - * #define HAS_WATCHDOG_SP_THREAD_DEBUG
> - */
> -
>  /*
>   * The linear buffer mode will accept data until the first
>   * overflow and then stop accepting new data
> diff --git a/drivers/staging/media/atomisp/pci/ia_css_acc_types.h b/drivers/staging/media/atomisp/pci/ia_css_acc_types.h
> index a20879aedef6..d6e52b4971d6 100644
> --- a/drivers/staging/media/atomisp/pci/ia_css_acc_types.h
> +++ b/drivers/staging/media/atomisp/pci/ia_css_acc_types.h
> @@ -331,11 +331,7 @@ struct ia_css_sp_info {
>  	of DDR debug queue */
>  	u32 perf_counter_input_system_error; /** input system perf
>  	counter array */
> -#ifdef HAS_WATCHDOG_SP_THREAD_DEBUG
> -	u32 debug_wait; /** thread/pipe post mortem debug */
> -	u32 debug_stage; /** thread/pipe post mortem debug */
> -	u32 debug_stripe; /** thread/pipe post mortem debug */
> -#endif
> +
>  	u32 threads_stack; /** sp thread's stack pointers */
>  	u32 threads_stack_size; /** sp thread's stack sizes */
>  	u32 curr_binary_id;        /** current binary id */
> diff --git a/drivers/staging/media/atomisp/pci/runtime/debug/interface/ia_css_debug.h b/drivers/staging/media/atomisp/pci/runtime/debug/interface/ia_css_debug.h
> index 3a3d72c6eaaa..efa136294836 100644
> --- a/drivers/staging/media/atomisp/pci/runtime/debug/interface/ia_css_debug.h
> +++ b/drivers/staging/media/atomisp/pci/runtime/debug/interface/ia_css_debug.h
> @@ -306,12 +306,6 @@ void ia_css_debug_dump_isp_params(struct ia_css_stream *stream,
>   */
>  void ia_css_debug_dump_perf_counters(void);
>  
> -#ifdef HAS_WATCHDOG_SP_THREAD_DEBUG
> -void sh_css_dump_thread_wait_info(void);
> -void sh_css_dump_pipe_stage_info(void);
> -void sh_css_dump_pipe_stripe_info(void);
> -#endif
> -
>  void ia_css_debug_dump_isp_binary(void);
>  
>  void sh_css_dump_sp_raw_copy_linecount(bool reduced);
> diff --git a/drivers/staging/media/atomisp/pci/runtime/debug/src/ia_css_debug.c b/drivers/staging/media/atomisp/pci/runtime/debug/src/ia_css_debug.c
> index bb30146c5fe7..cf66a40bd6a4 100644
> --- a/drivers/staging/media/atomisp/pci/runtime/debug/src/ia_css_debug.c
> +++ b/drivers/staging/media/atomisp/pci/runtime/debug/src/ia_css_debug.c
> @@ -101,13 +101,6 @@
>  
>  #define ENABLE_LINE_MAX_LENGTH (25)
>  
> -/*
> - * TODO:SH_CSS_MAX_SP_THREADS is not the max number of sp threads
> - * future rework should fix this and remove the define MAX_THREAD_NUM
> - */
> -#define MAX_THREAD_NUM_2400 (SH_CSS_MAX_SP_THREADS + SH_CSS_MAX_SP_INTERNAL_THREADS_2400)
> -#define MAX_THREAD_NUM_2401 (SH_CSS_MAX_SP_THREADS + SH_CSS_MAX_SP_INTERNAL_THREADS_2401)
> -
>  static struct pipe_graph_class {
>  	bool do_init;
>  	int height;

This patch looks good, except that as mentioned this bit should simply
remove the original lines from before this series, rather then first
having patch 1/3 modify these lines and then removing the modified
lines here.

Regards,

Hans



> @@ -2075,12 +2068,6 @@ void ia_css_debug_dump_debug_info(const char *context)
>  	ia_css_debug_dump_isp_gdc_fifo_state();
>  	ia_css_debug_dump_sp_state();
>  	ia_css_debug_dump_perf_counters();
> -
> -#ifdef HAS_WATCHDOG_SP_THREAD_DEBUG
> -	sh_css_dump_thread_wait_info();
> -	sh_css_dump_pipe_stage_info();
> -	sh_css_dump_pipe_stripe_info();
> -#endif
>  	ia_css_debug_dump_dma_isp_fifo_state();
>  	ia_css_debug_dump_dma_sp_fifo_state();
>  	ia_css_debug_dump_dma_state();
> @@ -2392,86 +2379,6 @@ static void __printf(1, 2) dtrace_dot(const char *fmt, ...)
>  	va_end(ap);
>  }
>  
> -#ifdef HAS_WATCHDOG_SP_THREAD_DEBUG
> -void sh_css_dump_thread_wait_info(void)
> -{
> -	const struct ia_css_fw_info *fw;
> -	int i;
> -	unsigned int HIVE_ADDR_sp_thread_wait;
> -	s32 sp_thread_wait[MAX_THREAD_NUM];
> -
> -	ia_css_debug_dtrace(IA_CSS_DEBUG_VERBOSE, "SEM WAITS:\n");
> -
> -	fw = &sh_css_sp_fw;
> -	HIVE_ADDR_sp_thread_wait =
> -	    fw->info.sp.debug_wait;
> -
> -	(void)HIVE_ADDR_sp_thread_wait;
> -
> -	sp_dmem_load(SP0_ID,
> -		     (unsigned int)sp_address_of(sp_thread_wait),
> -		     &sp_thread_wait,
> -		     sizeof(sp_thread_wait));
> -	for (i = 0; i < MAX_THREAD_NUM; i++) {
> -		ia_css_debug_dtrace(IA_CSS_DEBUG_VERBOSE,
> -				    "\twait[%d] = 0x%X\n",
> -				    i, sp_thread_wait[i]);
> -	}
> -}
> -
> -void sh_css_dump_pipe_stage_info(void)
> -{
> -	const struct ia_css_fw_info *fw;
> -	int i;
> -	unsigned int HIVE_ADDR_sp_pipe_stage;
> -	s32 sp_pipe_stage[MAX_THREAD_NUM];
> -
> -	ia_css_debug_dtrace(IA_CSS_DEBUG_VERBOSE, "PIPE STAGE:\n");
> -
> -	fw = &sh_css_sp_fw;
> -	HIVE_ADDR_sp_pipe_stage =
> -	    fw->info.sp.debug_stage;
> -
> -	(void)HIVE_ADDR_sp_pipe_stage;
> -
> -	sp_dmem_load(SP0_ID,
> -		     (unsigned int)sp_address_of(sp_pipe_stage),
> -		     &sp_pipe_stage,
> -		     sizeof(sp_pipe_stage));
> -	for (i = 0; i < MAX_THREAD_NUM; i++) {
> -		ia_css_debug_dtrace(IA_CSS_DEBUG_VERBOSE,
> -				    "\tstage[%d] = %d\n",
> -				    i, sp_pipe_stage[i]);
> -	}
> -}
> -
> -void sh_css_dump_pipe_stripe_info(void)
> -{
> -	const struct ia_css_fw_info *fw;
> -	int i;
> -	unsigned int HIVE_ADDR_sp_pipe_stripe;
> -	s32 sp_pipe_stripe[MAX_THREAD_NUM];
> -
> -	ia_css_debug_dtrace(IA_CSS_DEBUG_VERBOSE, "PIPE STRIPE:\n");
> -
> -	fw = &sh_css_sp_fw;
> -	HIVE_ADDR_sp_pipe_stripe =
> -	    fw->info.sp.debug_stripe;
> -
> -	(void)HIVE_ADDR_sp_pipe_stripe;
> -
> -	sp_dmem_load(SP0_ID,
> -		     (unsigned int)sp_address_of(sp_pipe_stripe),
> -		     &sp_pipe_stripe,
> -		     sizeof(sp_pipe_stripe));
> -	for (i = 0; i < MAX_THREAD_NUM; i++) {
> -		ia_css_debug_dtrace(IA_CSS_DEBUG_VERBOSE,
> -				    "\tstripe[%d] = %d\n",
> -				    i, sp_pipe_stripe[i]);
> -	}
> -}
> -#endif
> -
>  static void
>  ia_css_debug_pipe_graph_dump_frame(
>      const struct ia_css_frame *frame,
> diff --git a/drivers/staging/media/atomisp/pci/sh_css_internal.h b/drivers/staging/media/atomisp/pci/sh_css_internal.h
> index 2fa0b3e45fe0..2349eb4d3767 100644
> --- a/drivers/staging/media/atomisp/pci/sh_css_internal.h
> +++ b/drivers/staging/media/atomisp/pci/sh_css_internal.h
> @@ -84,32 +84,8 @@
>  #define SH_CSS_MAX_IF_CONFIGS	3 /* Must match with IA_CSS_NR_OF_CONFIGS (not defined yet).*/
>  #define SH_CSS_IF_CONFIG_NOT_NEEDED	0xFF
>  
> -/*
> - * SH_CSS_MAX_SP_THREADS:
> - *	 sp threads visible to host with connected communication queues
> - *	 these threads are capable of running an image pipe
> - * SH_CSS_MAX_SP_INTERNAL_THREADS:
> - *	 internal sp service threads, no communication queues to host
> - *	 these threads can't be used as image pipe
> - */
> -
> -#define SH_CSS_SP_INTERNAL_METADATA_THREAD_2400	1
> -#define SH_CSS_SP_INTERNAL_METADATA_THREAD_2401	0
> -
> -#define SH_CSS_SP_INTERNAL_SERVICE_THREAD		1
> -
>  #define SH_CSS_MAX_SP_THREADS		5
>  
> -#define SH_CSS_MAX_SP_INTERNAL_THREADS_2400	(\
> -	SH_CSS_SP_INTERNAL_SERVICE_THREAD +\
> -	 SH_CSS_SP_INTERNAL_METADATA_THREAD_2400)
> -
> -#define SH_CSS_MAX_SP_INTERNAL_THREADS_2401	(\
> -	SH_CSS_SP_INTERNAL_SERVICE_THREAD +\
> -	 SH_CSS_SP_INTERNAL_METADATA_THREAD_2401)
> -
> -#define SH_CSS_MAX_PIPELINES	SH_CSS_MAX_SP_THREADS
> -
>  /**
>   * The C99 standard does not specify the exact object representation of structs;
>   * the representation is compiler dependent.

[PATCH v2 3/3] atomisp: sh_css_params: write the sp_group config according to the ISP model

[Kate Hsuan]

Pick up the necessary part of sp_group configuration for both model and
then copy those parts into a tempetory buffer. This buffer is finally
written to the ISP with correct length.

Signed-off-by: Kate Hsuan <hpa@redhat.com>
---
 .../staging/media/atomisp/pci/sh_css_params.c | 41 ++++++++++++++++++-
 1 file changed, 39 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/sh_css_params.c b/drivers/staging/media/atomisp/pci/sh_css_params.c
index 588f2adab058..22a9fed006f6 100644
--- a/drivers/staging/media/atomisp/pci/sh_css_params.c
+++ b/drivers/staging/media/atomisp/pci/sh_css_params.c
@@ -3720,10 +3720,47 @@ struct ia_css_shading_table *ia_css_get_shading_table(struct ia_css_stream
 
 ia_css_ptr sh_css_store_sp_group_to_ddr(void)
 {
+	u8 *write_buf;
+	u8 *buf_ptr;
+
 	IA_CSS_ENTER_LEAVE_PRIVATE("void");
+
+	write_buf = kzalloc(sizeof(struct sh_css_sp_group), GFP_KERNEL);
+	if (!write_buf)
+		return 0;
+
+	buf_ptr = write_buf;
+	if (IS_ISP2401) {
+		memcpy(buf_ptr, &sh_css_sp_group.config, 3);
+		buf_ptr += 3;
+		memcpy(buf_ptr, &sh_css_sp_group.config.enable_isys_event_queue, 2);
+		buf_ptr += 2;
+		memset(buf_ptr, 0, 3);
+		buf_ptr += 3; /* Padding 3 bytes for struct sh_css_sp_config*/
+	} else {
+		memcpy(buf_ptr, &sh_css_sp_group.config, sizeof(sh_css_sp_group.config));
+		buf_ptr += sizeof(sh_css_sp_group.config);
+	}
+
+	memcpy(buf_ptr, &sh_css_sp_group.pipe, sizeof(sh_css_sp_group.pipe));
+	buf_ptr += sizeof(sh_css_sp_group.pipe);
+
+	if (IS_ISP2401) {
+		memcpy(buf_ptr, &sh_css_sp_group.pipe_io, sizeof(sh_css_sp_group.pipe_io));
+		buf_ptr += sizeof(sh_css_sp_group.pipe_io);
+		memcpy(buf_ptr, &sh_css_sp_group.pipe_io_status,
+		       sizeof(sh_css_sp_group.pipe_io_status));
+		buf_ptr += sizeof(sh_css_sp_group.pipe_io_status);
+	}
+
+	memcpy(buf_ptr, &sh_css_sp_group.debug, sizeof(sh_css_sp_group.debug));
+	buf_ptr += sizeof(sh_css_sp_group.debug);
+
 	hmm_store(xmem_sp_group_ptrs,
-		   &sh_css_sp_group,
-		   sizeof(struct sh_css_sp_group));
+		  write_buf,
+		  buf_ptr - write_buf);
+
+	kfree(write_buf);
 	return xmem_sp_group_ptrs;
 }
 
-- 
2.40.1

Re: [PATCH v2 3/3] atomisp: sh_css_params: write the sp_group config according to the ISP model

[Hans de Goede]

Hi Kate,

On 6/12/23 07:57, Kate Hsuan wrote:
> Pick up the necessary part of sp_group configuration for both model and
> then copy those parts into a tempetory buffer. This buffer is finally
> written to the ISP with correct length.
> 
> Signed-off-by: Kate Hsuan <hpa@redhat.com>

As mentioned in the review of patch 1/3 the sh_css_internal.h changes
from patch 1/3 should all be moved here.

Other then that this looks good to me.

Regards,

Hans


> ---
>  .../staging/media/atomisp/pci/sh_css_params.c | 41 ++++++++++++++++++-
>  1 file changed, 39 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/media/atomisp/pci/sh_css_params.c b/drivers/staging/media/atomisp/pci/sh_css_params.c
> index 588f2adab058..22a9fed006f6 100644
> --- a/drivers/staging/media/atomisp/pci/sh_css_params.c
> +++ b/drivers/staging/media/atomisp/pci/sh_css_params.c
> @@ -3720,10 +3720,47 @@ struct ia_css_shading_table *ia_css_get_shading_table(struct ia_css_stream
>  
>  ia_css_ptr sh_css_store_sp_group_to_ddr(void)
>  {
> +	u8 *write_buf;
> +	u8 *buf_ptr;
> +
>  	IA_CSS_ENTER_LEAVE_PRIVATE("void");
> +
> +	write_buf = kzalloc(sizeof(struct sh_css_sp_group), GFP_KERNEL);
> +	if (!write_buf)
> +		return 0;
> +
> +	buf_ptr = write_buf;
> +	if (IS_ISP2401) {
> +		memcpy(buf_ptr, &sh_css_sp_group.config, 3);
> +		buf_ptr += 3;
> +		memcpy(buf_ptr, &sh_css_sp_group.config.enable_isys_event_queue, 2);
> +		buf_ptr += 2;
> +		memset(buf_ptr, 0, 3);
> +		buf_ptr += 3; /* Padding 3 bytes for struct sh_css_sp_config*/
> +	} else {
> +		memcpy(buf_ptr, &sh_css_sp_group.config, sizeof(sh_css_sp_group.config));
> +		buf_ptr += sizeof(sh_css_sp_group.config);
> +	}
> +
> +	memcpy(buf_ptr, &sh_css_sp_group.pipe, sizeof(sh_css_sp_group.pipe));
> +	buf_ptr += sizeof(sh_css_sp_group.pipe);
> +
> +	if (IS_ISP2401) {
> +		memcpy(buf_ptr, &sh_css_sp_group.pipe_io, sizeof(sh_css_sp_group.pipe_io));
> +		buf_ptr += sizeof(sh_css_sp_group.pipe_io);
> +		memcpy(buf_ptr, &sh_css_sp_group.pipe_io_status,
> +		       sizeof(sh_css_sp_group.pipe_io_status));
> +		buf_ptr += sizeof(sh_css_sp_group.pipe_io_status);
> +	}
> +
> +	memcpy(buf_ptr, &sh_css_sp_group.debug, sizeof(sh_css_sp_group.debug));
> +	buf_ptr += sizeof(sh_css_sp_group.debug);
> +
>  	hmm_store(xmem_sp_group_ptrs,
> -		   &sh_css_sp_group,
> -		   sizeof(struct sh_css_sp_group));
> +		  write_buf,
> +		  buf_ptr - write_buf);
> +
> +	kfree(write_buf);
>  	return xmem_sp_group_ptrs;
>  }
>