RE: [PATCH v6 06/12] intel-ipu3: css: Add support for firmware management
Hi, Sakari,
> -Original Message-
> From: Sakari Ailus [mailto:sakari.ai...@linux.intel.com]
> Sent: Friday, September 21, 2018 6:52 AM
> To: Zhi, Yong
> Cc: Tomasz Figa ; Linux Media Mailing List me...@vger.kernel.org>; Mani, Rajmohan ;
> Toivonen, Tuukka ; Hu, Jerry W
> ; Zheng, Jian Xu
> Subject: Re: [PATCH v6 06/12] intel-ipu3: css: Add support for firmware
> management
>
> Hi Yong,
>
> On Wed, Sep 19, 2018 at 10:57:55PM +, Zhi, Yong wrote:
> ...
> > > > +struct imgu_abi_osys_frame_params {
> > > > + /* Output pins */
> > > > + __u32 enable;
> > > > + __u32 format; /* enum imgu_abi_osys_format */
> > > > + __u32 flip;
> > > > + __u32 mirror;
> > > > + __u32 tiling; /* enum imgu_abi_osys_tiling */
> > > > + __u32 width;
> > > > + __u32 height;
> > > > + __u32 stride;
> > > > + __u32 scaled;
> > > > +} __packed;
> > > [snip]
> > > > +/* Defect pixel correction */
> > > > +
> > > > +struct imgu_abi_dpc_config {
> > > > + __u8 __reserved[240832];
> > > > +} __packed;
> > >
> > > Do we need this structure? One could just add a reserved field in
> > > the parent structure. Also, just to confirm, is 240832 really the right
> value here?
> > > Where does it come from? Please create a macro for it, possibly
> > > further breaking it down into the values used to compute this number.
> > >
> >
> > We can add a reserved field in the parent structure, the size is based
> > on original definition of dpc config which was removed since it's not
> > enabled/used.
>
> What's your plan with the DPC? If you don't plan to add it now, you could
> as well drop the configuration for that block. If there's a need to add it
> later
> on, you can still do it by defining a new struct for the buffer. Or simply
> adding it at the end of the existing struct while allowing the use of the old
> size without the DPC configuration.
>
> There would be a little extra work to do there by that time when DPC
> support would be added, but OTOH it seems silly to have quarter of a
> megabyte of extra stuff to pass around in a struct that's never used.
>
> --
> Regards,
>
> Sakari Ailus
> sakari.ai...@linux.intel.com
It's a very good point, but as I was informed, there is no plan to update the
abi between the driver and firmware, so the size of imgu_abi_acc_param has not
changed since v1 to maintain the compatibility.
Re: [PATCH v6 06/12] intel-ipu3: css: Add support for firmware management
Hi Yong,
On Wed, Sep 19, 2018 at 10:57:55PM +, Zhi, Yong wrote:
...
> > > +struct imgu_abi_osys_frame_params {
> > > + /* Output pins */
> > > + __u32 enable;
> > > + __u32 format; /* enum imgu_abi_osys_format */
> > > + __u32 flip;
> > > + __u32 mirror;
> > > + __u32 tiling; /* enum imgu_abi_osys_tiling */
> > > + __u32 width;
> > > + __u32 height;
> > > + __u32 stride;
> > > + __u32 scaled;
> > > +} __packed;
> > [snip]
> > > +/* Defect pixel correction */
> > > +
> > > +struct imgu_abi_dpc_config {
> > > + __u8 __reserved[240832];
> > > +} __packed;
> >
> > Do we need this structure? One could just add a reserved field in the parent
> > structure. Also, just to confirm, is 240832 really the right value here?
> > Where does it come from? Please create a macro for it, possibly further
> > breaking it down into the values used to compute this number.
> >
>
> We can add a reserved field in the parent structure, the size is based on
> original definition of dpc config which was removed since it's not
> enabled/used.
What's your plan with the DPC? If you don't plan to add it now, you could
as well drop the configuration for that block. If there's a need to add it
later on, you can still do it by defining a new struct for the buffer. Or
simply adding it at the end of the existing struct while allowing the use
of the old size without the DPC configuration.
There would be a little extra work to do there by that time when DPC
support would be added, but OTOH it seems silly to have quarter of a
megabyte of extra stuff to pass around in a struct that's never used.
--
Regards,
Sakari Ailus
sakari.ai...@linux.intel.com
RE: [PATCH v6 06/12] intel-ipu3: css: Add support for firmware management
Hi, Tomasz,
> -Original Message-
> From: linux-media-ow...@vger.kernel.org [mailto:linux-media-
> ow...@vger.kernel.org] On Behalf Of Tomasz Figa
> Sent: Monday, July 2, 2018 2:05 AM
> To: Zhi, Yong
> Cc: Linux Media Mailing List ; Sakari Ailus
> ; Mani, Rajmohan
> ; Toivonen, Tuukka
> ; Hu, Jerry W ; Zheng,
> Jian Xu
> Subject: Re: [PATCH v6 06/12] intel-ipu3: css: Add support for firmware
> management
>
> Hi Yong,
>
> Continuing my review. Sorry for the delay.
>
> On Fri, Mar 30, 2018 at 11:15 AM Yong Zhi wrote:
> >
> > Introduce functions to load and install ImgU FW blobs.
> >
> > Signed-off-by: Yong Zhi
> > ---
> > drivers/media/pci/intel/ipu3/ipu3-abi.h| 1888
>
> > drivers/media/pci/intel/ipu3/ipu3-css-fw.c | 261
> > drivers/media/pci/intel/ipu3/ipu3-css-fw.h | 198 +++
> > 3 files changed, 2347 insertions(+)
> > create mode 100644 drivers/media/pci/intel/ipu3/ipu3-abi.h
> > create mode 100644 drivers/media/pci/intel/ipu3/ipu3-css-fw.c
> > create mode 100644 drivers/media/pci/intel/ipu3/ipu3-css-fw.h
> >
> > diff --git a/drivers/media/pci/intel/ipu3/ipu3-abi.h
> > b/drivers/media/pci/intel/ipu3/ipu3-abi.h
> > new file mode 100644
> > index ..24102647a89e
> > --- /dev/null
> > +++ b/drivers/media/pci/intel/ipu3/ipu3-abi.h
> [snip]
> > +/* SYSTEM_REQ_0_5_0_IMGHMMADR */
> > +#define IMGU_REG_SYSTEM_REQ0x18
> > +#define IMGU_SYSTEM_REQ_FREQ_MASK 0x3f
> > +#define IMGU_SYSTEM_REQ_FREQ_DIVIDER 25
> > +#define IMGU_REG_INT_STATUS0x30
> > +#define IMGU_REG_INT_ENABLE0x34
> > +#define IMGU_REG_INT_CSS_IRQ (1 << 31)
>
> BIT(31)
>
Ack.
> [snip]
> > + IMGU_ABI_FRAME_FORMAT_CSI_MIPI_LEGACY_YUV420_8, /*
> Legacy YUV420.
> > +* UY odd line;
> > +* VY even line
> > +*/
> > + IMGU_ABI_FRAME_FORMAT_CSI_MIPI_YUV420_10,/* 10 bit per
> Y/U/V. Y odd
> > + * line; UYVY interleaved
> > + * even line
> > + */
> > + IMGU_ABI_FRAME_FORMAT_YCgCo444_16, /* Internal format for
> > + ISP2.7,
>
> Macros and enums should be uppercase.
>
Ack.
> [snip]
> > +struct imgu_abi_shd_intra_frame_operations_data {
> > + struct imgu_abi_acc_operation
> > + operation_list[IMGU_ABI_SHD_MAX_OPERATIONS]
> IPU3_ALIGN;
> > + struct imgu_abi_acc_process_lines_cmd_data
> > + process_lines_data[IMGU_ABI_SHD_MAX_PROCESS_LINES]
> IPU3_ALIGN;
> > + struct imgu_abi_shd_transfer_luts_set_data
> > + transfer_data[IMGU_ABI_SHD_MAX_TRANSFERS] IPU3_ALIGN;
> > +} __packed;
> > +
> > +struct imgu_abi_shd_config {
> > + struct ipu3_uapi_shd_config_static shd IMGU_ABI_PAD;
> > + struct imgu_abi_shd_intra_frame_operations_data shd_ops
> IMGU_ABI_PAD;
> > + struct ipu3_uapi_shd_lut shd_lut IMGU_ABI_PAD;
>
> Definitions of both IPU3_ALIGN and IMGU_ABI_PAD seem to be equivalent.
> Could we remove one and use the other everywhere?
>
Agree, will remove IMGU_ABI_PAD.
> [snip]
> > +struct imgu_abi_osys_scaler_params {
> > + __u32 inp_buf_y_st_addr;
> > + __u32 inp_buf_y_line_stride;
> > + __u32 inp_buf_y_buffer_stride;
> > + __u32 inp_buf_u_st_addr;
> > + __u32 inp_buf_v_st_addr;
> > + __u32 inp_buf_uv_line_stride;
> > + __u32 inp_buf_uv_buffer_stride;
> > + __u32 inp_buf_chunk_width;
> > + __u32 inp_buf_nr_buffers;
> > + /* Output buffers */
> > + __u32 out_buf_y_st_addr;
> > + __u32 out_buf_y_line_stride;
> > + __u32 out_buf_y_buffer_stride;
> > + __u32 out_buf_u_st_addr;
> > + __u32 out_buf_v_st_addr;
> > + __u32 out_buf_uv_line_stride;
> > + __u32 out_buf_uv_buffer_stride;
> > + __u32 out_buf_nr_buffers;
> > + /* Intermediate buffers */
> > + __u32 int_buf_y_st_addr;
> > + __u32 int_buf_y_line_stride;
> > + __u32 int_buf_u_st_addr;
> > + __u32 int_buf_v_st_addr;
> > + __u32 int_buf_uv_line_stride;
> > + __u32 int_buf_height;
> > + __u32 int_buf_chunk_width;
> &
Re: [PATCH v6 06/12] intel-ipu3: css: Add support for firmware management
Hi Yong,
Continuing my review. Sorry for the delay.
On Fri, Mar 30, 2018 at 11:15 AM Yong Zhi wrote:
>
> Introduce functions to load and install ImgU FW blobs.
>
> Signed-off-by: Yong Zhi
> ---
> drivers/media/pci/intel/ipu3/ipu3-abi.h| 1888
>
> drivers/media/pci/intel/ipu3/ipu3-css-fw.c | 261
> drivers/media/pci/intel/ipu3/ipu3-css-fw.h | 198 +++
> 3 files changed, 2347 insertions(+)
> create mode 100644 drivers/media/pci/intel/ipu3/ipu3-abi.h
> create mode 100644 drivers/media/pci/intel/ipu3/ipu3-css-fw.c
> create mode 100644 drivers/media/pci/intel/ipu3/ipu3-css-fw.h
>
> diff --git a/drivers/media/pci/intel/ipu3/ipu3-abi.h
> b/drivers/media/pci/intel/ipu3/ipu3-abi.h
> new file mode 100644
> index ..24102647a89e
> --- /dev/null
> +++ b/drivers/media/pci/intel/ipu3/ipu3-abi.h
[snip]
> +/* SYSTEM_REQ_0_5_0_IMGHMMADR */
> +#define IMGU_REG_SYSTEM_REQ0x18
> +#define IMGU_SYSTEM_REQ_FREQ_MASK 0x3f
> +#define IMGU_SYSTEM_REQ_FREQ_DIVIDER 25
> +#define IMGU_REG_INT_STATUS0x30
> +#define IMGU_REG_INT_ENABLE0x34
> +#define IMGU_REG_INT_CSS_IRQ (1 << 31)
BIT(31)
[snip]
> + IMGU_ABI_FRAME_FORMAT_CSI_MIPI_LEGACY_YUV420_8, /* Legacy YUV420.
> +* UY odd line;
> +* VY even line
> +*/
> + IMGU_ABI_FRAME_FORMAT_CSI_MIPI_YUV420_10,/* 10 bit per Y/U/V. Y odd
> + * line; UYVY interleaved
> + * even line
> + */
> + IMGU_ABI_FRAME_FORMAT_YCgCo444_16, /* Internal format for ISP2.7,
Macros and enums should be uppercase.
[snip]
> +struct imgu_abi_shd_intra_frame_operations_data {
> + struct imgu_abi_acc_operation
> + operation_list[IMGU_ABI_SHD_MAX_OPERATIONS] IPU3_ALIGN;
> + struct imgu_abi_acc_process_lines_cmd_data
> + process_lines_data[IMGU_ABI_SHD_MAX_PROCESS_LINES] IPU3_ALIGN;
> + struct imgu_abi_shd_transfer_luts_set_data
> + transfer_data[IMGU_ABI_SHD_MAX_TRANSFERS] IPU3_ALIGN;
> +} __packed;
> +
> +struct imgu_abi_shd_config {
> + struct ipu3_uapi_shd_config_static shd IMGU_ABI_PAD;
> + struct imgu_abi_shd_intra_frame_operations_data shd_ops IMGU_ABI_PAD;
> + struct ipu3_uapi_shd_lut shd_lut IMGU_ABI_PAD;
Definitions of both IPU3_ALIGN and IMGU_ABI_PAD seem to be equivalent.
Could we remove one and use the other everywhere?
[snip]
> +struct imgu_abi_osys_scaler_params {
> + __u32 inp_buf_y_st_addr;
> + __u32 inp_buf_y_line_stride;
> + __u32 inp_buf_y_buffer_stride;
> + __u32 inp_buf_u_st_addr;
> + __u32 inp_buf_v_st_addr;
> + __u32 inp_buf_uv_line_stride;
> + __u32 inp_buf_uv_buffer_stride;
> + __u32 inp_buf_chunk_width;
> + __u32 inp_buf_nr_buffers;
> + /* Output buffers */
> + __u32 out_buf_y_st_addr;
> + __u32 out_buf_y_line_stride;
> + __u32 out_buf_y_buffer_stride;
> + __u32 out_buf_u_st_addr;
> + __u32 out_buf_v_st_addr;
> + __u32 out_buf_uv_line_stride;
> + __u32 out_buf_uv_buffer_stride;
> + __u32 out_buf_nr_buffers;
> + /* Intermediate buffers */
> + __u32 int_buf_y_st_addr;
> + __u32 int_buf_y_line_stride;
> + __u32 int_buf_u_st_addr;
> + __u32 int_buf_v_st_addr;
> + __u32 int_buf_uv_line_stride;
> + __u32 int_buf_height;
> + __u32 int_buf_chunk_width;
> + __u32 int_buf_chunk_height;
> + /* Context buffers */
> + __u32 ctx_buf_hor_y_st_addr;
> + __u32 ctx_buf_hor_u_st_addr;
> + __u32 ctx_buf_hor_v_st_addr;
> + __u32 ctx_buf_ver_y_st_addr;
> + __u32 ctx_buf_ver_u_st_addr;
> + __u32 ctx_buf_ver_v_st_addr;
> + /* Addresses for release-input and process-output tokens */
> + __u32 release_inp_buf_addr;
> + __u32 release_inp_buf_en;
> + __u32 release_out_buf_en;
> + __u32 process_out_buf_addr;
> + /* Settings dimensions, padding, cropping */
> + __u32 input_image_y_width;
> + __u32 input_image_y_height;
> + __u32 input_image_y_start_column;
> + __u32 input_image_uv_start_column;
> + __u32 input_image_y_left_pad;
> + __u32 input_image_uv_left_pad;
> + __u32 input_image_y_right_pad;
> + __u32 input_image_uv_right_pad;
> + __u32 input_image_y_top_pad;
> + __u32 input_image_uv_top_pad;
> + __u32 input_image_y_bottom_pad;
> + __u32 input_image_uv_bottom_pad;
> + __u32 processing_mode;
> +#define IMGU_ABI_OSYS_PROCMODE_BYPASS 0
> +#define IMGU_ABI_OSYS_PROCMODE_UPSCALE 1
> +#define IMGU_ABI_OSYS_PROCMODE_DOWNSCALE 2
