All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC PATCH] [media] v4l2-subdev: check colorimetry in link validate
@ 2017-05-30 19:08 Helen Koike
  2017-05-31  6:31 ` Sakari Ailus
  0 siblings, 1 reply; 7+ messages in thread
From: Helen Koike @ 2017-05-30 19:08 UTC (permalink / raw)
  To: linux-media; +Cc: Hans Verkuil

colorspace, ycbcr_enc, quantization and xfer_func must match across the
link.
Check if they match in v4l2_subdev_link_validate_default unless they are
set as _DEFAULT.

Signed-off-by: Helen Koike <helen.koike@collabora.com>

---

Hi,

I think we should validate colorimetry as having different colorimetry
across a link doesn't make sense.
But I am confused about what to do when they are set to _DEFAULT, what
do you think?

Thanks
---
 drivers/media/v4l2-core/v4l2-subdev.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index da78497..784ae92 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -502,10 +502,27 @@ int v4l2_subdev_link_validate_default(struct v4l2_subdev *sd,
 				      struct v4l2_subdev_format *source_fmt,
 				      struct v4l2_subdev_format *sink_fmt)
 {
-	/* The width, height and code must match. */
+	/* The width, height, code and colorspace must match. */
 	if (source_fmt->format.width != sink_fmt->format.width
 	    || source_fmt->format.height != sink_fmt->format.height
-	    || source_fmt->format.code != sink_fmt->format.code)
+	    || source_fmt->format.code != sink_fmt->format.code
+	    || source_fmt->format.colorspace != sink_fmt->format.colorspace)
+		return -EPIPE;
+
+	/* Colorimetry must match if they are not set to DEFAULT */
+	if (source_fmt->format.ycbcr_enc != V4L2_YCBCR_ENC_DEFAULT
+	    && sink_fmt->format.ycbcr_enc != V4L2_YCBCR_ENC_DEFAULT
+	    && source_fmt->format.ycbcr_enc != sink_fmt->format.ycbcr_enc)
+		return -EPIPE;
+
+	if (source_fmt->format.quantization != V4L2_QUANTIZATION_DEFAULT
+	    && sink_fmt->format.quantization != V4L2_QUANTIZATION_DEFAULT
+	    && source_fmt->format.quantization != sink_fmt->format.quantization)
+		return -EPIPE;
+
+	if (source_fmt->format.xfer_func != V4L2_XFER_FUNC_DEFAULT
+	    && sink_fmt->format.xfer_func != V4L2_XFER_FUNC_DEFAULT
+	    && source_fmt->format.xfer_func != sink_fmt->format.xfer_func)
 		return -EPIPE;
 
 	/* The field order must match, or the sink field order must be NONE
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [RFC PATCH] [media] v4l2-subdev: check colorimetry in link validate
  2017-05-30 19:08 [RFC PATCH] [media] v4l2-subdev: check colorimetry in link validate Helen Koike
@ 2017-05-31  6:31 ` Sakari Ailus
  2017-06-06 22:15   ` Helen Koike
  0 siblings, 1 reply; 7+ messages in thread
From: Sakari Ailus @ 2017-05-31  6:31 UTC (permalink / raw)
  To: Helen Koike; +Cc: linux-media, Hans Verkuil

Hi Helen,

On Tue, May 30, 2017 at 04:08:08PM -0300, Helen Koike wrote:
> colorspace, ycbcr_enc, quantization and xfer_func must match across the
> link.
> Check if they match in v4l2_subdev_link_validate_default unless they are
> set as _DEFAULT.
> 
> Signed-off-by: Helen Koike <helen.koike@collabora.com>
> 
> ---
> 
> Hi,
> 
> I think we should validate colorimetry as having different colorimetry
> across a link doesn't make sense.
> But I am confused about what to do when they are set to _DEFAULT, what
> do you think?

These fields were added at various points over the course of the past three
years or so. User space code that was written before that will certainly not
set anything and I'm not sure many drivers care about these fields nor they
are relevant for many formats. In practice that means that they are very
likely zero in many cases.

Driver changes will probably be necessary for removing the explicit checks
for the default value.

The same applies to checking the colour space: drivers should enforce using
the correct colour space before the check can be merged. I might move that
to a separate patch.

> 
> Thanks
> ---
>  drivers/media/v4l2-core/v4l2-subdev.c | 21 +++++++++++++++++++--
>  1 file changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index da78497..784ae92 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -502,10 +502,27 @@ int v4l2_subdev_link_validate_default(struct v4l2_subdev *sd,
>  				      struct v4l2_subdev_format *source_fmt,
>  				      struct v4l2_subdev_format *sink_fmt)
>  {
> -	/* The width, height and code must match. */
> +	/* The width, height, code and colorspace must match. */
>  	if (source_fmt->format.width != sink_fmt->format.width
>  	    || source_fmt->format.height != sink_fmt->format.height
> -	    || source_fmt->format.code != sink_fmt->format.code)
> +	    || source_fmt->format.code != sink_fmt->format.code
> +	    || source_fmt->format.colorspace != sink_fmt->format.colorspace)
> +		return -EPIPE;
> +
> +	/* Colorimetry must match if they are not set to DEFAULT */
> +	if (source_fmt->format.ycbcr_enc != V4L2_YCBCR_ENC_DEFAULT
> +	    && sink_fmt->format.ycbcr_enc != V4L2_YCBCR_ENC_DEFAULT
> +	    && source_fmt->format.ycbcr_enc != sink_fmt->format.ycbcr_enc)
> +		return -EPIPE;
> +
> +	if (source_fmt->format.quantization != V4L2_QUANTIZATION_DEFAULT
> +	    && sink_fmt->format.quantization != V4L2_QUANTIZATION_DEFAULT
> +	    && source_fmt->format.quantization != sink_fmt->format.quantization)
> +		return -EPIPE;
> +
> +	if (source_fmt->format.xfer_func != V4L2_XFER_FUNC_DEFAULT
> +	    && sink_fmt->format.xfer_func != V4L2_XFER_FUNC_DEFAULT
> +	    && source_fmt->format.xfer_func != sink_fmt->format.xfer_func)
>  		return -EPIPE;
>  
>  	/* The field order must match, or the sink field order must be NONE

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC PATCH] [media] v4l2-subdev: check colorimetry in link validate
  2017-05-31  6:31 ` Sakari Ailus
@ 2017-06-06 22:15   ` Helen Koike
  2017-06-08 11:41     ` Mauro Carvalho Chehab
  0 siblings, 1 reply; 7+ messages in thread
From: Helen Koike @ 2017-06-06 22:15 UTC (permalink / raw)
  To: Sakari Ailus; +Cc: linux-media, Hans Verkuil

Hi Sakari,


Thanks for replying

On 2017-05-31 03:31 AM, Sakari Ailus wrote:
> Hi Helen,
>
> On Tue, May 30, 2017 at 04:08:08PM -0300, Helen Koike wrote:
>> colorspace, ycbcr_enc, quantization and xfer_func must match across the
>> link.
>> Check if they match in v4l2_subdev_link_validate_default unless they are
>> set as _DEFAULT.
>>
>> Signed-off-by: Helen Koike <helen.koike@collabora.com>
>>
>> ---
>>
>> Hi,
>>
>> I think we should validate colorimetry as having different colorimetry
>> across a link doesn't make sense.
>> But I am confused about what to do when they are set to _DEFAULT, what
>> do you think?
>
> These fields were added at various points over the course of the past three
> years or so. User space code that was written before that will certainly not
> set anything and I'm not sure many drivers care about these fields nor they
> are relevant for many formats. In practice that means that they are very
> likely zero in many cases.

If they are set to zero then they won't be affected by this patch.

>
> Driver changes will probably be necessary for removing the explicit checks
> for the default value.

At least in the drivers/media tree I couldn't find many drivers that 
implement its own link_validate, there is only 
platform/omap3isp/ispccdc.c and platform/omap3isp/ispresizer.c that 
implements a custom value, but from a quick look it doesn't seems that 
they will be affected.

>
> The same applies to checking the colour space: drivers should enforce using
> the correct colour space before the check can be merged. I might move that
> to a separate patch.

I am not sure if I got what you mean. If driver don't care about 
colourspace then probably it will be set to zero and won't be affected 
by this patch, if colourspace is different across the link then the user 
space must set both ends to the same colourspace.

>
>>
>> Thanks
>> ---
>>  drivers/media/v4l2-core/v4l2-subdev.c | 21 +++++++++++++++++++--
>>  1 file changed, 19 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
>> index da78497..784ae92 100644
>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
>> @@ -502,10 +502,27 @@ int v4l2_subdev_link_validate_default(struct v4l2_subdev *sd,
>>  				      struct v4l2_subdev_format *source_fmt,
>>  				      struct v4l2_subdev_format *sink_fmt)
>>  {
>> -	/* The width, height and code must match. */
>> +	/* The width, height, code and colorspace must match. */
>>  	if (source_fmt->format.width != sink_fmt->format.width
>>  	    || source_fmt->format.height != sink_fmt->format.height
>> -	    || source_fmt->format.code != sink_fmt->format.code)
>> +	    || source_fmt->format.code != sink_fmt->format.code
>> +	    || source_fmt->format.colorspace != sink_fmt->format.colorspace)
>> +		return -EPIPE;
>> +
>> +	/* Colorimetry must match if they are not set to DEFAULT */
>> +	if (source_fmt->format.ycbcr_enc != V4L2_YCBCR_ENC_DEFAULT
>> +	    && sink_fmt->format.ycbcr_enc != V4L2_YCBCR_ENC_DEFAULT
>> +	    && source_fmt->format.ycbcr_enc != sink_fmt->format.ycbcr_enc)
>> +		return -EPIPE;
>> +
>> +	if (source_fmt->format.quantization != V4L2_QUANTIZATION_DEFAULT
>> +	    && sink_fmt->format.quantization != V4L2_QUANTIZATION_DEFAULT
>> +	    && source_fmt->format.quantization != sink_fmt->format.quantization)
>> +		return -EPIPE;
>> +
>> +	if (source_fmt->format.xfer_func != V4L2_XFER_FUNC_DEFAULT
>> +	    && sink_fmt->format.xfer_func != V4L2_XFER_FUNC_DEFAULT
>> +	    && source_fmt->format.xfer_func != sink_fmt->format.xfer_func)
>>  		return -EPIPE;
>>
>>  	/* The field order must match, or the sink field order must be NONE
>

LN

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC PATCH] [media] v4l2-subdev: check colorimetry in link validate
  2017-06-06 22:15   ` Helen Koike
@ 2017-06-08 11:41     ` Mauro Carvalho Chehab
  2017-06-08 16:34       ` Helen Koike
  0 siblings, 1 reply; 7+ messages in thread
From: Mauro Carvalho Chehab @ 2017-06-08 11:41 UTC (permalink / raw)
  To: Helen Koike; +Cc: Sakari Ailus, linux-media, Hans Verkuil

Em Tue, 6 Jun 2017 19:15:34 -0300
Helen Koike <helen.koike@collabora.com> escreveu:

> Hi Sakari,
> 
> 
> Thanks for replying
> 
> On 2017-05-31 03:31 AM, Sakari Ailus wrote:
> > Hi Helen,
> >
> > On Tue, May 30, 2017 at 04:08:08PM -0300, Helen Koike wrote:  
> >> colorspace, ycbcr_enc, quantization and xfer_func must match across the
> >> link.
> >> Check if they match in v4l2_subdev_link_validate_default unless they are
> >> set as _DEFAULT.
> >>
> >> Signed-off-by: Helen Koike <helen.koike@collabora.com>
> >>
> >> ---
> >>
> >> Hi,
> >>
> >> I think we should validate colorimetry as having different colorimetry
> >> across a link doesn't make sense.
> >> But I am confused about what to do when they are set to _DEFAULT, what
> >> do you think?  
> >
> > These fields were added at various points over the course of the past three
> > years or so. User space code that was written before that will certainly not
> > set anything and I'm not sure many drivers care about these fields nor they
> > are relevant for many formats. In practice that means that they are very
> > likely zero in many cases.  
> 
> If they are set to zero then they won't be affected by this patch.
> 
> >
> > Driver changes will probably be necessary for removing the explicit checks
> > for the default value.  
> 
> At least in the drivers/media tree I couldn't find many drivers that 
> implement its own link_validate, there is only 
> platform/omap3isp/ispccdc.c and platform/omap3isp/ispresizer.c that 
> implements a custom value, but from a quick look it doesn't seems that 
> they will be affected.
> 
> >
> > The same applies to checking the colour space: drivers should enforce using
> > the correct colour space before the check can be merged. I might move that
> > to a separate patch.  
> 
> I am not sure if I got what you mean. If driver don't care about 
> colourspace then probably it will be set to zero and won't be affected 
> by this patch, if colourspace is different across the link then the user 
> space must set both ends to the same colourspace.

I guess what Sakari is concerned about is to avoid regressions.

Colorimetry properties were added after the addition of most of
the drivers. Adding a mandatory link validation may break drivers
that don't set it right. Even on new drivers, this may not be ok.

Just to give you an example, this week I just applied a patch fixing
colorimetry handling at the coda driver:
	https://git.linuxtv.org/media_tree.git/commit/?id=b14ac545688d8cc4b2b707d71d106799ad476964

If a change like that would have been applied before such fix, it 
could be breaking coda.

So, before adding such patch, we need to check how existing drivers are
setting colorimetry fields, to be sure that the ones that don't
touch it won't break.

Perhaps one alternative would be to write a patchset that would,
instead, print a warning at dmesg, and let it be upstream for a
while, to give people time to check if the colorimetry logic is
ok at the subdevs.

Thanks,
Mauro

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC PATCH] [media] v4l2-subdev: check colorimetry in link validate
  2017-06-08 11:41     ` Mauro Carvalho Chehab
@ 2017-06-08 16:34       ` Helen Koike
  2017-06-08 17:05         ` [PATCH v2] " Helen Koike
  0 siblings, 1 reply; 7+ messages in thread
From: Helen Koike @ 2017-06-08 16:34 UTC (permalink / raw)
  To: Mauro Carvalho Chehab; +Cc: Sakari Ailus, linux-media, Hans Verkuil

Hi

On 2017-06-08 08:41 AM, Mauro Carvalho Chehab wrote:
> Em Tue, 6 Jun 2017 19:15:34 -0300
> Helen Koike <helen.koike@collabora.com> escreveu:
>
>> Hi Sakari,
>>
>>
>> Thanks for replying
>>
>> On 2017-05-31 03:31 AM, Sakari Ailus wrote:
>>> Hi Helen,
>>>
>>> On Tue, May 30, 2017 at 04:08:08PM -0300, Helen Koike wrote:
>>>> colorspace, ycbcr_enc, quantization and xfer_func must match across the
>>>> link.
>>>> Check if they match in v4l2_subdev_link_validate_default unless they are
>>>> set as _DEFAULT.
>>>>
>>>> Signed-off-by: Helen Koike <helen.koike@collabora.com>
>>>>
>>>> ---
>>>>
>>>> Hi,
>>>>
>>>> I think we should validate colorimetry as having different colorimetry
>>>> across a link doesn't make sense.
>>>> But I am confused about what to do when they are set to _DEFAULT, what
>>>> do you think?
>>>
>>> These fields were added at various points over the course of the past three
>>> years or so. User space code that was written before that will certainly not
>>> set anything and I'm not sure many drivers care about these fields nor they
>>> are relevant for many formats. In practice that means that they are very
>>> likely zero in many cases.
>>
>> If they are set to zero then they won't be affected by this patch.
>>
>>>
>>> Driver changes will probably be necessary for removing the explicit checks
>>> for the default value.
>>
>> At least in the drivers/media tree I couldn't find many drivers that
>> implement its own link_validate, there is only
>> platform/omap3isp/ispccdc.c and platform/omap3isp/ispresizer.c that
>> implements a custom value, but from a quick look it doesn't seems that
>> they will be affected.
>>
>>>
>>> The same applies to checking the colour space: drivers should enforce using
>>> the correct colour space before the check can be merged. I might move that
>>> to a separate patch.
>>
>> I am not sure if I got what you mean. If driver don't care about
>> colourspace then probably it will be set to zero and won't be affected
>> by this patch, if colourspace is different across the link then the user
>> space must set both ends to the same colourspace.
>
> I guess what Sakari is concerned about is to avoid regressions.
>
> Colorimetry properties were added after the addition of most of
> the drivers. Adding a mandatory link validation may break drivers
> that don't set it right. Even on new drivers, this may not be ok.
>
> Just to give you an example, this week I just applied a patch fixing
> colorimetry handling at the coda driver:
> 	https://git.linuxtv.org/media_tree.git/commit/?id=b14ac545688d8cc4b2b707d71d106799ad476964
>
> If a change like that would have been applied before such fix, it
> could be breaking coda.

If I understand correct, coda doesn't have subdevs or links so it 
wouldn't be affected by this patch.

>
> So, before adding such patch, we need to check how existing drivers are
> setting colorimetry fields, to be sure that the ones that don't
> touch it won't break.

In my view, if they don't touch colorimetry they shouldn't be affected 
by this patch as values would be zero, and there are very few drivers 
that implement a custom link validation under driver/media tree, and 
from a quick look it doesn't seems to change colorimetry values.

>
> Perhaps one alternative would be to write a patchset that would,
> instead, print a warning at dmesg, and let it be upstream for a
> while, to give people time to check if the colorimetry logic is
> ok at the subdevs.

Agreed, lets start with that, I'll re-send this patch which this change.

Thanks
Helen

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v2] [media] v4l2-subdev: check colorimetry in link validate
  2017-06-08 16:34       ` Helen Koike
@ 2017-06-08 17:05         ` Helen Koike
  2017-06-14  7:45           ` Sakari Ailus
  0 siblings, 1 reply; 7+ messages in thread
From: Helen Koike @ 2017-06-08 17:05 UTC (permalink / raw)
  To: Mauro Carvalho Chehab; +Cc: Sakari Ailus, linux-media, Hans Verkuil

colorspace, ycbcr_enc, quantization and xfer_func must match
across the link.
Check if they match in v4l2_subdev_link_validate_default
unless they are set as _DEFAULT.

Signed-off-by: Helen Koike <helen.koike@collabora.com>

---

Hi,

As discussed previously, I added a warn message instead of returning
error to give drivers some time to adapt.
But the problem is that: as the format is set by userspace, it is
possible that userspace will set the wrong format at pads and see these
messages when there is no error in the driver's code at all (or maybe
this is not a problem, just noise in the log).

Helen
---
 drivers/media/v4l2-core/v4l2-subdev.c | 38 +++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index da78497..1a642c7 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -508,6 +508,44 @@ int v4l2_subdev_link_validate_default(struct v4l2_subdev *sd,
 	    || source_fmt->format.code != sink_fmt->format.code)
 		return -EPIPE;
 
+	/*
+	 * TODO: return -EPIPE instead of printing a warning in the following
+	 * checks. As colorimetry properties were added after most of the
+	 * drivers, only a warning was added to avoid potential regressions
+	 */
+
+	/* colorspace match. */
+	if (source_fmt->format.colorspace != sink_fmt->format.colorspace)
+		dev_warn(sd->v4l2_dev->dev,
+			 "colorspace doesn't match in link \"%s\":%d->\"%s\":%d\n",
+			link->source->entity->name, link->source->index,
+			link->sink->entity->name, link->sink->index);
+
+	/* Colorimetry must match if they are not set to DEFAULT */
+	if (source_fmt->format.ycbcr_enc != V4L2_YCBCR_ENC_DEFAULT
+	    && sink_fmt->format.ycbcr_enc != V4L2_YCBCR_ENC_DEFAULT
+	    && source_fmt->format.ycbcr_enc != sink_fmt->format.ycbcr_enc)
+		dev_warn(sd->v4l2_dev->dev,
+			 "YCbCr encoding doesn't match in link \"%s\":%d->\"%s\":%d\n",
+			link->source->entity->name, link->source->index,
+			link->sink->entity->name, link->sink->index);
+
+	if (source_fmt->format.quantization != V4L2_QUANTIZATION_DEFAULT
+	    && sink_fmt->format.quantization != V4L2_QUANTIZATION_DEFAULT
+	    && source_fmt->format.quantization != sink_fmt->format.quantization)
+		dev_warn(sd->v4l2_dev->dev,
+			 "quantization doesn't match in link \"%s\":%d->\"%s\":%d\n",
+			link->source->entity->name, link->source->index,
+			link->sink->entity->name, link->sink->index);
+
+	if (source_fmt->format.xfer_func != V4L2_XFER_FUNC_DEFAULT
+	    && sink_fmt->format.xfer_func != V4L2_XFER_FUNC_DEFAULT
+	    && source_fmt->format.xfer_func != sink_fmt->format.xfer_func)
+		dev_warn(sd->v4l2_dev->dev,
+			 "transfer function doesn't match in link \"%s\":%d->\"%s\":%d\n",
+			link->source->entity->name, link->source->index,
+			link->sink->entity->name, link->sink->index);
+
 	/* The field order must match, or the sink field order must be NONE
 	 * to support interlaced hardware connected to bridges that support
 	 * progressive formats only.
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH v2] [media] v4l2-subdev: check colorimetry in link validate
  2017-06-08 17:05         ` [PATCH v2] " Helen Koike
@ 2017-06-14  7:45           ` Sakari Ailus
  0 siblings, 0 replies; 7+ messages in thread
From: Sakari Ailus @ 2017-06-14  7:45 UTC (permalink / raw)
  To: Helen Koike; +Cc: Mauro Carvalho Chehab, linux-media, Hans Verkuil

Hi Helen and Mauro,

On Thu, Jun 08, 2017 at 02:05:08PM -0300, Helen Koike wrote:
> colorspace, ycbcr_enc, quantization and xfer_func must match
> across the link.
> Check if they match in v4l2_subdev_link_validate_default
> unless they are set as _DEFAULT.

I think you could ignore my earlier comments on this --- the check will take
place only iff both are not defaults, i.e. non-zero. And these values
definitely should be zero unless explicitly set otherwise -- by the driver.
I missed this on the previous review round.

So I think it'd be fine to return an error on these.

How about using dev_dbg() instead if dev_warn()? Using dev_warn() gives an
easy way to flood the logs to the user. A debug level message is still
important as it's next to impossible for the user to figure out what
actually went wrong. Getting a single numeric error code from starting the
pipeline isn't telling much...

> 
> Signed-off-by: Helen Koike <helen.koike@collabora.com>
> 
> ---
> 
> Hi,
> 
> As discussed previously, I added a warn message instead of returning
> error to give drivers some time to adapt.
> But the problem is that: as the format is set by userspace, it is
> possible that userspace will set the wrong format at pads and see these
> messages when there is no error in the driver's code at all (or maybe
> this is not a problem, just noise in the log).
> 
> Helen
> ---
>  drivers/media/v4l2-core/v4l2-subdev.c | 38 +++++++++++++++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index da78497..1a642c7 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -508,6 +508,44 @@ int v4l2_subdev_link_validate_default(struct v4l2_subdev *sd,
>  	    || source_fmt->format.code != sink_fmt->format.code)
>  		return -EPIPE;
>  
> +	/*
> +	 * TODO: return -EPIPE instead of printing a warning in the following
> +	 * checks. As colorimetry properties were added after most of the
> +	 * drivers, only a warning was added to avoid potential regressions
> +	 */
> +
> +	/* colorspace match. */
> +	if (source_fmt->format.colorspace != sink_fmt->format.colorspace)
> +		dev_warn(sd->v4l2_dev->dev,
> +			 "colorspace doesn't match in link \"%s\":%d->\"%s\":%d\n",
> +			link->source->entity->name, link->source->index,
> +			link->sink->entity->name, link->sink->index);
> +
> +	/* Colorimetry must match if they are not set to DEFAULT */
> +	if (source_fmt->format.ycbcr_enc != V4L2_YCBCR_ENC_DEFAULT
> +	    && sink_fmt->format.ycbcr_enc != V4L2_YCBCR_ENC_DEFAULT
> +	    && source_fmt->format.ycbcr_enc != sink_fmt->format.ycbcr_enc)
> +		dev_warn(sd->v4l2_dev->dev,
> +			 "YCbCr encoding doesn't match in link \"%s\":%d->\"%s\":%d\n",
> +			link->source->entity->name, link->source->index,
> +			link->sink->entity->name, link->sink->index);
> +
> +	if (source_fmt->format.quantization != V4L2_QUANTIZATION_DEFAULT
> +	    && sink_fmt->format.quantization != V4L2_QUANTIZATION_DEFAULT
> +	    && source_fmt->format.quantization != sink_fmt->format.quantization)
> +		dev_warn(sd->v4l2_dev->dev,
> +			 "quantization doesn't match in link \"%s\":%d->\"%s\":%d\n",
> +			link->source->entity->name, link->source->index,
> +			link->sink->entity->name, link->sink->index);
> +
> +	if (source_fmt->format.xfer_func != V4L2_XFER_FUNC_DEFAULT
> +	    && sink_fmt->format.xfer_func != V4L2_XFER_FUNC_DEFAULT
> +	    && source_fmt->format.xfer_func != sink_fmt->format.xfer_func)
> +		dev_warn(sd->v4l2_dev->dev,
> +			 "transfer function doesn't match in link \"%s\":%d->\"%s\":%d\n",
> +			link->source->entity->name, link->source->index,
> +			link->sink->entity->name, link->sink->index);
> +
>  	/* The field order must match, or the sink field order must be NONE
>  	 * to support interlaced hardware connected to bridges that support
>  	 * progressive formats only.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2017-06-14  7:45 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-30 19:08 [RFC PATCH] [media] v4l2-subdev: check colorimetry in link validate Helen Koike
2017-05-31  6:31 ` Sakari Ailus
2017-06-06 22:15   ` Helen Koike
2017-06-08 11:41     ` Mauro Carvalho Chehab
2017-06-08 16:34       ` Helen Koike
2017-06-08 17:05         ` [PATCH v2] " Helen Koike
2017-06-14  7:45           ` Sakari Ailus

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.