Re: [PATCH] media: hws: report measured colorimetry

[Ben Hoff]

Hi,

Just following up on this series, originally posted on August 7. I
haven't seen any review comments yet and wanted to check whether
there's anything I should address or whether it's in a good state for
consideration.

Thanks,
Ben

On Fri, Aug 7, 2026 at 3:47 AM Ben Hoff <hoff.benjamin.k@xxxxxxxxx> wrote:
>
> At 1920x1080, the driver advertises V4L2_COLORSPACE_REC709 with
> V4L2_YCBCR_ENC_DEFAULT. The default maps to a Rec.709 matrix, but
> controlled A/B captures using the in-tree and baseline drivers show that
> both produce YUYV samples using a BT.601 matrix and full-range
> quantization. The captures differ by at most one code value, whereas a
> 601-to-709 conversion would move primary luma values by roughly 11 to 33
> codes.
>
> Set the Y'CbCr encoding explicitly to V4L2_YCBCR_ENC_601 at every
> resolution. Keep the resolution-based colorspace so HD retains presumed
> Rec.709 primaries and transfer characteristics while SD continues to use
> SMPTE 170M. Refresh the colorspace on detected geometry changes so an
> SD/HD transition does not leave stale metadata.
>
> The decoder mode register is undocumented. Stop claiming that value 0x13
> selects BT.709; both drivers use that value while producing the measured
> output.
>
> Fixes: ba07fd2f5742 ("media: pci: add AVMatrix HWS capture driver")
> Assisted-by: Codex:gpt-5.6
> Signed-off-by: Ben Hoff <hoff.benjamin.k@xxxxxxxxx>
> ---
>  drivers/media/pci/hws/hws.h            | 4 ++--
>  drivers/media/pci/hws/hws_reg.h        | 3 ++-
>  drivers/media/pci/hws/hws_v4l2_ioctl.c | 5 +++--
>  drivers/media/pci/hws/hws_video.c      | 4 +++-
>  4 files changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/media/pci/hws/hws.h b/drivers/media/pci/hws/hws.h
> index 8fbe1fe27844..d87d52674b69 100644
> --- a/drivers/media/pci/hws/hws.h
> +++ b/drivers/media/pci/hws/hws.h
> @@ -33,8 +33,8 @@ struct hws_pix_state {
>         u32 sizeimage;          /* full frame */
>         enum v4l2_field field;  /* V4L2_FIELD_NONE or INTERLACED */
>         enum v4l2_colorspace colorspace;        /* e.g., REC709 */
> -       enum v4l2_ycbcr_encoding ycbcr_enc;     /* V4L2_YCBCR_ENC_DEFAULT */
> -       enum v4l2_quantization quantization;    /* V4L2_QUANTIZATION_LIM_RANGE */
> +       enum v4l2_ycbcr_encoding ycbcr_enc;     /* V4L2_YCBCR_ENC_601 */
> +       enum v4l2_quantization quantization;    /* V4L2_QUANTIZATION_FULL_RANGE */
>         enum v4l2_xfer_func xfer_func;  /* V4L2_XFER_FUNC_DEFAULT */
>         bool interlaced;        /* cached hardware state */
>         u32 half_size;          /* hardware half-frame size */
> diff --git a/drivers/media/pci/hws/hws_reg.h b/drivers/media/pci/hws/hws_reg.h
> index e4fb4af44434..f6027a16a193 100644
> --- a/drivers/media/pci/hws/hws_reg.h
> +++ b/drivers/media/pci/hws/hws_reg.h
> @@ -70,7 +70,8 @@
>  #define HWS_CTL_IRQ_ENABLE_BIT BIT(0)   /* Global interrupt enable bit */
>  /*  Write 0x00 to fully reset decoder,
>   *  set bit 31=1 to "start run",
> - *  low byte=0x13 selects YUYV/BT.709/etc,
> + *  low byte=0x13 is the vendor baseline capture-mode value; its individual
> + *  bit meanings are not documented,
>   *  in ReadChipId() we also write 0x00 and 0x10 here for chip-ID sequencing.
>   */
>
> diff --git a/drivers/media/pci/hws/hws_v4l2_ioctl.c b/drivers/media/pci/hws/hws_v4l2_ioctl.c
> index 0303e311ee4a..ce396b7225d2 100644
> --- a/drivers/media/pci/hws/hws_v4l2_ioctl.c
> +++ b/drivers/media/pci/hws/hws_v4l2_ioctl.c
> @@ -511,8 +511,9 @@ static inline void hws_set_colorimetry_state(struct hws_pix_state *p)
>  {
>         bool sd = p->height <= 576;
>
> +       /* Captured samples use full-range BT.601 Y'CbCr at every resolution. */
>         p->colorspace   = sd ? V4L2_COLORSPACE_SMPTE170M : V4L2_COLORSPACE_REC709;
> -       p->ycbcr_enc    = V4L2_YCBCR_ENC_DEFAULT;
> +       p->ycbcr_enc    = V4L2_YCBCR_ENC_601;
>         p->quantization = V4L2_QUANTIZATION_FULL_RANGE;
>         p->xfer_func    = V4L2_XFER_FUNC_DEFAULT;
>  }
> @@ -736,7 +737,7 @@ static inline void hws_set_colorimetry_fmt(struct v4l2_pix_format *p)
>         bool sd = p->height <= 576;
>
>         p->colorspace   = sd ? V4L2_COLORSPACE_SMPTE170M : V4L2_COLORSPACE_REC709;
> -       p->ycbcr_enc    = V4L2_YCBCR_ENC_DEFAULT;
> +       p->ycbcr_enc    = V4L2_YCBCR_ENC_601;
>         p->quantization = V4L2_QUANTIZATION_FULL_RANGE;
>         p->xfer_func    = V4L2_XFER_FUNC_DEFAULT;
>  }
> diff --git a/drivers/media/pci/hws/hws_video.c b/drivers/media/pci/hws/hws_video.c
> index 18e4bc6901d3..09cfb5a909f9 100644
> --- a/drivers/media/pci/hws/hws_video.c
> +++ b/drivers/media/pci/hws/hws_video.c
> @@ -327,7 +327,7 @@ int hws_video_init_channel(struct hws_pcie_dev *pdev, int ch)
>         vid->pix.sizeimage = vid->pix.bytesperline * vid->pix.height;
>         vid->pix.field = V4L2_FIELD_NONE;
>         vid->pix.colorspace = V4L2_COLORSPACE_REC709;
> -       vid->pix.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
> +       vid->pix.ycbcr_enc = V4L2_YCBCR_ENC_601;
>         vid->pix.quantization = V4L2_QUANTIZATION_FULL_RANGE;
>         vid->pix.xfer_func = V4L2_XFER_FUNC_DEFAULT;
>         vid->pix.interlaced = false;
> @@ -895,6 +895,8 @@ static void hws_video_apply_mode_change(struct hws_pcie_dev *pdx,
>         v->pix.width = w;
>         v->pix.height = h;
>         v->pix.interlaced = interlaced;
> +       v->pix.colorspace = h <= 576 ? V4L2_COLORSPACE_SMPTE170M :
> +                                        V4L2_COLORSPACE_REC709;
>         hws_set_current_dv_timings(v, w, h, interlaced);
>         v->current_fps = fps;
>
> --
> 2.55.0
>