Re: [PATCH RFC 2/4] media: qcom: camss: vfe: Describe the IFE pixel pipe per version

[bod]

On 2026-10-13 09:44 +0100, William Bright wrote:
> The SM8550 IFE 780 runs the same pixel pipe register map as the
> SM8650 IFE 880 but holds its WB gain in a different format.
> 
> Pass the pipe a small description of what differs between the IFEs
> that run it, starting with the WB gain value for full scale, and keep
> the IFE 880 on its current 4.0. No functional change.
> 
> Assisted-by: Claude:claude-opus-5-5
> Signed-off-by: William Bright <william.bright@xxxxxxxxxxx>
> ---
>  drivers/media/platform/qcom/camss/camss-vfe-780.c    |  8 +++++++-
>  drivers/media/platform/qcom/camss/camss-vfe-880-pp.c | 20 +++++++++++++-------
>  drivers/media/platform/qcom/camss/camss-vfe-880-pp.h | 10 +++++++++-
>  3 files changed, 29 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/camss/camss-vfe-780.c b/drivers/media/platform/qcom/camss/camss-vfe-780.c
> index 6d483f917d15..c2ffe39f929a 100644
> --- a/drivers/media/platform/qcom/camss/camss-vfe-780.c
> +++ b/drivers/media/platform/qcom/camss/camss-vfe-780.c
> @@ -236,12 +236,18 @@ static inline void vfe_reg_update_clear(struct vfe_device *vfe,
>   * Program the pixel pipe outside output_lock, before the write masters and
>   * the IPP reg update that latches both.
>   */
> +static const struct vfe_pp_desc *vfe_pp_desc(struct vfe_device *vfe)
> +{
> +	return &vfe_pp_desc_880;
> +}
> +
>  static void vfe_line_prepare(struct vfe_line *line)
>  {
>  	struct v4l2_pix_format_mplane *pix = &line->output[0].video_out.active_fmt.fmt.pix_mp;
> +	struct vfe_device *vfe = to_vfe(line);
>  
>  	if (line->is_pix && pix->pixelformat == V4L2_PIX_FMT_NV12)
> -		vfe_880_setup_pixel_pipe(to_vfe(line), pix->width, pix->height);
> +		vfe_880_setup_pixel_pipe(vfe, vfe_pp_desc(vfe), pix->width, pix->height);
>  }
>  
>  static const struct camss_video_ops vfe_video_ops_780 = {
> diff --git a/drivers/media/platform/qcom/camss/camss-vfe-880-pp.c b/drivers/media/platform/qcom/camss/camss-vfe-880-pp.c
> index 9b4bdad6d396..f962016ef3c6 100644
> --- a/drivers/media/platform/qcom/camss/camss-vfe-880-pp.c
> +++ b/drivers/media/platform/qcom/camss/camss-vfe-880-pp.c
> @@ -313,8 +313,13 @@ static void vfe_setup_chroma_downscale_nv12(struct vfe_device *vfe, u32 w, u32 h
>  	writel_relaxed(0x00000000, vfe->base + IFE_DOWNSCALE_MN_C_VID_OUT_DOWNSCALE_MN_C_V_PHASE_CFG);
>  }
>  
> -/* Demosaic + WB: sub offsets 0, gains 4.0, add offsets 0 */
> -static void vfe_setup_demosaic(struct vfe_device *vfe)
> +/* IFE 880: Q10 gain of 4.0 */
> +const struct vfe_pp_desc vfe_pp_desc_880 = {
> +	.wb_gain = 0x00001000,
> +};
> +
> +/* Demosaic + WB: sub offsets 0, full scale gains, add offsets 0 */
> +static void vfe_setup_demosaic(struct vfe_device *vfe, const struct vfe_pp_desc *desc)
>  {
>  	writel_relaxed(0x00000001, vfe->base + IFE_DEMOSAIC_MODULE_CFG);
>  	writel_relaxed(0x00000080, vfe->base + IFE_DEMOSAIC_INTERP_COEFF_CFG_0);
> @@ -322,9 +327,9 @@ static void vfe_setup_demosaic(struct vfe_device *vfe)
>  	writel_relaxed(0x00000040, vfe->base + IFE_DEMOSAIC_INTERP_CLASSIFIER_CFG_0);
>  	writel_relaxed(0x00000066, vfe->base + IFE_DEMOSAIC_INTERP_CLASSIFIER_CFG_1);
>  	writel_relaxed(0x00000001, vfe->base + IFE_WB_GAIN_MODULE_CFG);
> -	writel_relaxed(0x00001000, vfe->base + IFE_WB_GAIN_WB_CFG_0);
> -	writel_relaxed(0x00001000, vfe->base + IFE_WB_GAIN_WB_CFG_1);
> -	writel_relaxed(0x00001000, vfe->base + IFE_WB_GAIN_WB_CFG_2);
> +	writel_relaxed(desc->wb_gain, vfe->base + IFE_WB_GAIN_WB_CFG_0);
> +	writel_relaxed(desc->wb_gain, vfe->base + IFE_WB_GAIN_WB_CFG_1);
> +	writel_relaxed(desc->wb_gain, vfe->base + IFE_WB_GAIN_WB_CFG_2);

wait ... doesn't this write the same value as was already there...

ah ok your description says no functional change.

>  	writel_relaxed(0x00000000, vfe->base + IFE_WB_GAIN_WB_OFFSET_CFG_0);
>  	writel_relaxed(0x00000000, vfe->base + IFE_WB_GAIN_WB_OFFSET_CFG_1);
>  	writel_relaxed(0x00000000, vfe->base + IFE_WB_GAIN_WB_OFFSET_CFG_2);
> @@ -611,11 +616,12 @@ static void vfe_setup_ds16(struct vfe_device *vfe, u32 w, u32 h)
>  }
>  
>  /* Called from vfe_line_prepare() for NV12, ahead of the write masters and the IPP reg update */
> -void vfe_880_setup_pixel_pipe(struct vfe_device *vfe, u32 w, u32 h)
> +void vfe_880_setup_pixel_pipe(struct vfe_device *vfe, const struct vfe_pp_desc *desc,
> +			      u32 w, u32 h)
>  {
>  	vfe_setup_core_config(vfe);
>  	vfe_setup_chroma_downscale_nv12(vfe, w, h);
> -	vfe_setup_demosaic(vfe);
> +	vfe_setup_demosaic(vfe, desc);

yeah it looks OK in principle probably implies some SoC specific params
will be required for all/some of these setups.
---
bod