| Message ID | 20200316221606.2648820-1-niklas.soderlund+renesas@ragnatech.se (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vimc: Report a colorspace | expand |
Hi Niklas, Thank you for the patch. I would just change the title of the commit to start with the tags: media: vimc: cap: On 3/16/20 7:16 PM, Niklas Söderlund wrote: > The colorspace reported by a video nodes should not be > V4L2_COLORSPACE_DEFAULT. Instead a default colorspace should be picked > by the driver if V4L2_COLORSPACE_DEFAULT is given by userspace to > {G,S,TRY}_FMT. > > The colorspace V4L2_COLORSPACE_SRGB is arbitrary chosen as the vimc > default format to report as it's used for most webcams. > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> Acked-by: Helen Koike <helen.koike@collabora.com> Do the subdevices also need this change? They also use V4L2_COLORSPACE_DEFAULT in their default format. Regards, Helen > --- > drivers/media/platform/vimc/vimc-capture.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > --- > Hi, > > This was found while adding V4L2_CAP_IO_MC support to vimc and adding > tests to v4l2-compliance. The issue will hence only show up in > v4l2-compliance if V4L2_CAP_IO_MC series is enabled for vimc. > > Best regards, > Niklas Söderlund > > diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c > index 23e740c1c5c00802..747ea9cc1bd7cb12 100644 > --- a/drivers/media/platform/vimc/vimc-capture.c > +++ b/drivers/media/platform/vimc/vimc-capture.c > @@ -37,7 +37,7 @@ static const struct v4l2_pix_format fmt_default = { > .height = 480, > .pixelformat = V4L2_PIX_FMT_RGB24, > .field = V4L2_FIELD_NONE, > - .colorspace = V4L2_COLORSPACE_DEFAULT, > + .colorspace = V4L2_COLORSPACE_SRGB, > }; > > struct vimc_cap_buffer { > @@ -107,6 +107,9 @@ static int vimc_cap_try_fmt_vid_cap(struct file *file, void *priv, > > vimc_colorimetry_clamp(format); > > + if (format->colorspace == V4L2_COLORSPACE_DEFAULT) > + format->colorspace = fmt_default.colorspace; > + > return 0; > } > >
On 17/03/2020 12:27, Helen Koike wrote: > Hi Niklas, > > Thank you for the patch. > > I would just change the title of the commit to start with the tags: > > media: vimc: cap: > > On 3/16/20 7:16 PM, Niklas Söderlund wrote: >> The colorspace reported by a video nodes should not be >> V4L2_COLORSPACE_DEFAULT. Instead a default colorspace should be picked >> by the driver if V4L2_COLORSPACE_DEFAULT is given by userspace to >> {G,S,TRY}_FMT. >> >> The colorspace V4L2_COLORSPACE_SRGB is arbitrary chosen as the vimc >> default format to report as it's used for most webcams. >> >> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > Acked-by: Helen Koike <helen.koike@collabora.com> > > Do the subdevices also need this change? > They also use V4L2_COLORSPACE_DEFAULT in their default format. The sensor specifically should report a non-default colorspace. Ideally the colorimetry information should propagate from the source (sensor) to the capture device. To be honest, I'm not sure how existing MC drivers handle this. Regards, Hans > > Regards, > Helen > >> --- >> drivers/media/platform/vimc/vimc-capture.c | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> --- >> Hi, >> >> This was found while adding V4L2_CAP_IO_MC support to vimc and adding >> tests to v4l2-compliance. The issue will hence only show up in >> v4l2-compliance if V4L2_CAP_IO_MC series is enabled for vimc. >> >> Best regards, >> Niklas Söderlund >> >> diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c >> index 23e740c1c5c00802..747ea9cc1bd7cb12 100644 >> --- a/drivers/media/platform/vimc/vimc-capture.c >> +++ b/drivers/media/platform/vimc/vimc-capture.c >> @@ -37,7 +37,7 @@ static const struct v4l2_pix_format fmt_default = { >> .height = 480, >> .pixelformat = V4L2_PIX_FMT_RGB24, >> .field = V4L2_FIELD_NONE, >> - .colorspace = V4L2_COLORSPACE_DEFAULT, >> + .colorspace = V4L2_COLORSPACE_SRGB, >> }; >> >> struct vimc_cap_buffer { >> @@ -107,6 +107,9 @@ static int vimc_cap_try_fmt_vid_cap(struct file *file, void *priv, >> >> vimc_colorimetry_clamp(format); >> >> + if (format->colorspace == V4L2_COLORSPACE_DEFAULT) >> + format->colorspace = fmt_default.colorspace; >> + >> return 0; >> } >> >>
diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c index 23e740c1c5c00802..747ea9cc1bd7cb12 100644 --- a/drivers/media/platform/vimc/vimc-capture.c +++ b/drivers/media/platform/vimc/vimc-capture.c @@ -37,7 +37,7 @@ static const struct v4l2_pix_format fmt_default = { .height = 480, .pixelformat = V4L2_PIX_FMT_RGB24, .field = V4L2_FIELD_NONE, - .colorspace = V4L2_COLORSPACE_DEFAULT, + .colorspace = V4L2_COLORSPACE_SRGB, }; struct vimc_cap_buffer { @@ -107,6 +107,9 @@ static int vimc_cap_try_fmt_vid_cap(struct file *file, void *priv, vimc_colorimetry_clamp(format); + if (format->colorspace == V4L2_COLORSPACE_DEFAULT) + format->colorspace = fmt_default.colorspace; + return 0; }
The colorspace reported by a video nodes should not be V4L2_COLORSPACE_DEFAULT. Instead a default colorspace should be picked by the driver if V4L2_COLORSPACE_DEFAULT is given by userspace to {G,S,TRY}_FMT. The colorspace V4L2_COLORSPACE_SRGB is arbitrary chosen as the vimc default format to report as it's used for most webcams. Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> --- drivers/media/platform/vimc/vimc-capture.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) --- Hi, This was found while adding V4L2_CAP_IO_MC support to vimc and adding tests to v4l2-compliance. The issue will hence only show up in v4l2-compliance if V4L2_CAP_IO_MC series is enabled for vimc. Best regards, Niklas Söderlund