Re: [PATCH 07/12] intel-ipu3: css: firmware management

[Tomasz Figa]

Hi Yong,

Please see my comments inline.

On Tue, Jun 6, 2017 at 5:39 AM, Yong Zhi  wrote:
> Functions to load and install imgu FW blobs
>
> Signed-off-by: Yong Zhi 
> ---
>  drivers/media/pci/intel/ipu3/ipu3-abi.h| 1572 
> 
>  drivers/media/pci/intel/ipu3/ipu3-css-fw.c |  272 +
>  drivers/media/pci/intel/ipu3/ipu3-css-fw.h |  215 
>  drivers/media/pci/intel/ipu3/ipu3-css.h|   54 +
>  4 files changed, 2113 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
>  create mode 100644 drivers/media/pci/intel/ipu3/ipu3-css.h
[snip]
> diff --git a/drivers/media/pci/intel/ipu3/ipu3-css-fw.c 
> b/drivers/media/pci/intel/ipu3/ipu3-css-fw.c
> new file mode 100644
> index 000..55edbb8
> --- /dev/null
> +++ b/drivers/media/pci/intel/ipu3/ipu3-css-fw.c
> @@ -0,0 +1,272 @@
> +/*
> + * Copyright (c) 2017 Intel Corporation.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License version
> + * 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include 

Shouldn't need this.

> +#include 
> +#include 
> +#include 
> +
> +#include "ipu3-css.h"
> +#include "ipu3-css-fw.h"
> +
> +static void ipu3_css_fw_show_binary(struct device *dev,
> +   struct imgu_fw_info *bi, const char *name)
> +{
> +   int i;
> +
> +   dev_dbg(dev, "found firmware binary type %i size %i name %s\n",
> +   bi->type, bi->blob.size, name);
> +   if (bi->type != IMGU_FW_ISP_FIRMWARE)
> +   return;
> +
> +   dev_dbg(dev, "id %i mode %i bds 0x%x veceven %i/%i out_pins %i\n",
> +   bi->info.isp.sp.id, bi->info.isp.sp.pipeline.mode,
> +   bi->info.isp.sp.bds.supported_bds_factors,
> +   bi->info.isp.sp.enable.vf_veceven,
> +   bi->info.isp.sp.vf_dec.is_variable,
> +   bi->info.isp.num_output_pins);
> +
> +   dev_dbg(dev, "input (%i,%i)-(%i,%i) formats %s%s%s\n",
> +   bi->info.isp.sp.input.min_width,
> +   bi->info.isp.sp.input.min_height,
> +   bi->info.isp.sp.input.max_width,
> +   bi->info.isp.sp.input.max_height,
> +   bi->info.isp.sp.enable.input_yuv ? "yuv420 " : "",
> +   bi->info.isp.sp.enable.input_feeder ||
> +   bi->info.isp.sp.enable.input_raw ? "raw8 raw10 " : "",
> +   bi->info.isp.sp.enable.input_raw ? "raw12" : "");
> +
> +   dev_dbg(dev, "internal (%i,%i)\n",
> +   bi->info.isp.sp.internal.max_width,
> +   bi->info.isp.sp.internal.max_height);
> +
> +   dev_dbg(dev, "output (%i,%i)-(%i,%i) formats",
> +   bi->info.isp.sp.output.min_width,
> +   bi->info.isp.sp.output.min_height,
> +   bi->info.isp.sp.output.max_width,
> +   bi->info.isp.sp.output.max_height);
> +   for (i = 0; i < bi->info.isp.num_output_formats; i++)
> +   dev_dbg(dev, " %i", bi->info.isp.output_formats[i]);
> +   dev_dbg(dev, " vf");
> +   for (i = 0; i < bi->info.isp.num_vf_formats; i++)
> +   dev_dbg(dev, " %i", bi->info.isp.vf_formats[i]);

When the function is called, neither num_output_formats nor
num_vf_formats is validated. It will cause an out of bound read here
if it isn't correct.

> +   dev_dbg(dev, "\n");
> +}
> +
> +const int ipu3_css_fw_obgrid_size(const struct imgu_fw_info *bi)
> +{
> +   unsigned int stripes = bi->info.isp.sp.iterator.num_stripes;
> +   unsigned int width, height, obgrid_size;
> +
> +   width = ALIGN(DIV_ROUND_UP(bi->info.isp.sp.internal.max_width,
> +   IMGU_OBGRID_TILE_SIZE * 2) + 1, IPU3_UAPI_ISP_VEC_ELEMS / 4);
> +   height = DIV_ROUND_UP(bi->info.isp.sp.internal.max_height,
> +   IMGU_OBGRID_TILE_SIZE * 2) + 1;
> +   obgrid_size = PAGE_ALIGN(width * height *
> +   sizeof(struct ipu3_uapi_obgrid_param)) * stripes;
> +
> +   return obgrid_size;
> +}
> +
> +void *ipu3_css_fw_pipeline_params(struct ipu3_css *css,
> +   enum imgu_abi_param_class c, enum imgu_abi_memories m,
> +   struct imgu_fw_isp_parameter *par, size_t par_size,
> +   void *binary_params)
> +{
> +   struct imgu_fw_info *bi = 
> >fwp->binary_header[css->current_binary];
> +
> +   if (par->offset + par->size >
> +   bi->info.isp.sp.mem_initializers.params[c][m].size)
> +   return NULL;
> +
> +   if (par->size != 

RE: [PATCH 07/12] intel-ipu3: css: firmware management

[Zhi, Yong]

> -Original Message-
> From: linux-media-ow...@vger.kernel.org [mailto:linux-media-
> ow...@vger.kernel.org] On Behalf Of Hans Verkuil
> Sent: Tuesday, June 6, 2017 1:39 AM
> To: Zhi, Yong <yong@intel.com>; linux-media@vger.kernel.org;
> sakari.ai...@linux.intel.com
> Cc: Zheng, Jian Xu <jian.xu.zh...@intel.com>; tf...@chromium.org; Mani,
> Rajmohan <rajmohan.m...@intel.com>; Toivonen, Tuukka
> <tuukka.toivo...@intel.com>
> Subject: Re: [PATCH 07/12] intel-ipu3: css: firmware management
> 
> On 05/06/17 22:39, Yong Zhi wrote:
> > Functions to load and install imgu FW blobs
> >
> > Signed-off-by: Yong Zhi <yong@intel.com>
> > ---
> >  drivers/media/pci/intel/ipu3/ipu3-abi.h| 1572
> 
> >  drivers/media/pci/intel/ipu3/ipu3-css-fw.c |  272 +
> > drivers/media/pci/intel/ipu3/ipu3-css-fw.h |  215 
> >  drivers/media/pci/intel/ipu3/ipu3-css.h|   54 +
> >  4 files changed, 2113 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
> >  create mode 100644 drivers/media/pci/intel/ipu3/ipu3-css.h
> 
> Has this been tested for both i686 and x86_64 modes?
> 
> Regards,
> 
>   Hans

Sorry for the late response, the above code has been tested for x86_64 mode 
only.

Re: [PATCH 07/12] intel-ipu3: css: firmware management

[Hans Verkuil]

On 05/06/17 22:39, Yong Zhi wrote:
> Functions to load and install imgu FW blobs
> 
> Signed-off-by: Yong Zhi 
> ---
>  drivers/media/pci/intel/ipu3/ipu3-abi.h| 1572 
> 
>  drivers/media/pci/intel/ipu3/ipu3-css-fw.c |  272 +
>  drivers/media/pci/intel/ipu3/ipu3-css-fw.h |  215 
>  drivers/media/pci/intel/ipu3/ipu3-css.h|   54 +
>  4 files changed, 2113 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
>  create mode 100644 drivers/media/pci/intel/ipu3/ipu3-css.h

Has this been tested for both i686 and x86_64 modes?

Regards,

Hans