Re: [PATCH v3 08/13] davinci: vpif: add support for clipping on output data

Re: [PATCH v3 08/13] davinci: vpif: add support for clipping on output data

[Laurent Pinchart]

Hi Manjunath,

Thank you for the patch.

On Monday 25 June 2012 16:37:30 Manjunath Hadli wrote:
> add hardware clipping support for VPIF output data. This
> is needed as it is possible that the external encoder
> might get confused between the FF or 00 which are a part
> of the data and that of the SAV or EAV codes.
> 
> Signed-off-by: Manjunath Hadli <manjunath.hadli@ti.com>
> Signed-off-by: Lad, Prabhakar <prabhakar.lad@ti.com>
> ---
>  drivers/media/video/davinci/vpif.h         |   30 +++++++++++++++++++++++++
>  drivers/media/video/davinci/vpif_display.c |   10 +++++++++
>  include/media/davinci/vpif_types.h         |    2 +
>  3 files changed, 42 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/media/video/davinci/vpif.h
> b/drivers/media/video/davinci/vpif.h index a4d2141..c2ce4d9 100644
> --- a/drivers/media/video/davinci/vpif.h
> +++ b/drivers/media/video/davinci/vpif.h
> @@ -211,6 +211,12 @@ static inline void vpif_clr_bit(u32 reg, u32 bit)
>  #define VPIF_CH3_INT_CTRL_SHIFT	(6)
>  #define VPIF_CH_INT_CTRL_SHIFT	(6)
> 
> +#define VPIF_CH2_CLIP_ANC_EN	14
> +#define VPIF_CH2_CLIP_ACTIVE_EN	13
> +
> +#define VPIF_CH3_CLIP_ANC_EN	14
> +#define VPIF_CH3_CLIP_ACTIVE_EN	13
> +
>  /* enabled interrupt on both the fields on vpid_ch0_ctrl register */
>  #define channel0_intr_assert()	(regw((regr(VPIF_CH0_CTRL)|\
>  	(VPIF_INT_BOTH << VPIF_CH0_INT_CTRL_SHIFT)), VPIF_CH0_CTRL))
> @@ -515,6 +521,30 @@ static inline void channel3_raw_enable(int enable, u8
> index) vpif_clr_bit(VPIF_CH3_CTRL, mask);
>  }
> 
> +/* function to enable clipping (for both active and blanking regions) on ch
> 2 */ +static inline void channel2_clipping_enable(int enable)
> +{
> +	if (enable) {
> +		vpif_set_bit(VPIF_CH2_CTRL, VPIF_CH2_CLIP_ANC_EN);
> +		vpif_set_bit(VPIF_CH2_CTRL, VPIF_CH2_CLIP_ACTIVE_EN);
> +	} else {
> +		vpif_clr_bit(VPIF_CH2_CTRL, VPIF_CH2_CLIP_ANC_EN);
> +		vpif_clr_bit(VPIF_CH2_CTRL, VPIF_CH2_CLIP_ACTIVE_EN);
> +	}
> +}
> +
> +/* function to enable clipping (for both active and blanking regions) on ch
> 2 */ +static inline void channel3_clipping_enable(int enable)
> +{
> +	if (enable) {
> +		vpif_set_bit(VPIF_CH3_CTRL, VPIF_CH3_CLIP_ANC_EN);
> +		vpif_set_bit(VPIF_CH3_CTRL, VPIF_CH3_CLIP_ACTIVE_EN);
> +	} else {
> +		vpif_clr_bit(VPIF_CH3_CTRL, VPIF_CH3_CLIP_ANC_EN);
> +		vpif_clr_bit(VPIF_CH3_CTRL, VPIF_CH3_CLIP_ACTIVE_EN);
> +	}
> +}
> +
>  /* inline function to set buffer addresses in case of Y/C non mux mode */
>  static inline void ch2_set_videobuf_addr_yc_nmux(unsigned long
> top_strt_luma, unsigned long btm_strt_luma,
> diff --git a/drivers/media/video/davinci/vpif_display.c
> b/drivers/media/video/davinci/vpif_display.c index 61ea8bc..4436ef6 100644
> --- a/drivers/media/video/davinci/vpif_display.c
> +++ b/drivers/media/video/davinci/vpif_display.c
> @@ -1046,6 +1046,8 @@ static int vpif_streamon(struct file *file, void
> *priv, channel2_intr_assert();
>  			channel2_intr_enable(1);
>  			enable_channel2(1);
> +			if (vpif_config_data->ch2_clip_en)
> +				channel2_clipping_enable(1);
>  		}
> 
>  		if ((VPIF_CHANNEL3_VIDEO == ch->channel_id)
> @@ -1053,6 +1055,8 @@ static int vpif_streamon(struct file *file, void
> *priv, channel3_intr_assert();
>  			channel3_intr_enable(1);
>  			enable_channel3(1);
> +			if (vpif_config_data->ch3_clip_en)
> +				channel3_clipping_enable(1);
>  		}
>  		channel_first_int[VPIF_VIDEO_INDEX][ch->channel_id] = 1;
>  	}
> @@ -1065,6 +1069,8 @@ static int vpif_streamoff(struct file *file, void
> *priv, struct vpif_fh *fh = priv;
>  	struct channel_obj *ch = fh->channel;
>  	struct common_obj *common = &ch->common[VPIF_VIDEO_INDEX];
> +	struct vpif_display_config *vpif_config_data =
> +					vpif_dev->platform_data;
> 
>  	if (buftype != V4L2_BUF_TYPE_VIDEO_OUTPUT) {
>  		vpif_err("buffer type not supported\n");
> @@ -1084,11 +1090,15 @@ static int vpif_streamoff(struct file *file, void
> *priv, if (buftype == V4L2_BUF_TYPE_VIDEO_OUTPUT) {
>  		/* disable channel */
>  		if (VPIF_CHANNEL2_VIDEO == ch->channel_id) {
> +			if (vpif_config_data->ch2_clip_en)
> +				channel2_clipping_enable(0);
>  			enable_channel2(0);
>  			channel2_intr_enable(0);
>  		}
>  		if ((VPIF_CHANNEL3_VIDEO == ch->channel_id) ||
>  					(2 == common->started)) {
> +			if (vpif_config_data->ch3_clip_en)
> +				channel3_clipping_enable(0);
>  			enable_channel3(0);
>  			channel3_intr_enable(0);
>  		}
> diff --git a/include/media/davinci/vpif_types.h
> b/include/media/davinci/vpif_types.h index bd8217c..d8f6ab1 100644
> --- a/include/media/davinci/vpif_types.h
> +++ b/include/media/davinci/vpif_types.h
> @@ -50,6 +50,8 @@ struct vpif_display_config {
>  	const char **output;
>  	int output_count;
>  	const char *card_name;
> +	bool ch2_clip_en;
> +	bool ch3_clip_en;

Instead of hardcoding this in platform data, I think it would be better to 
make this runtime-configurable. One option is to use the value of the 
v4l2_pix_format::colorspace field configured by userspace. We already have 
V4L2_COLORSPACE_JPEG which maps to the full 0-255 range, but we're missing a 
colorspace for the clipped 1-254 range used by the VPIF and I'm not sure 
whether it would really make sense to add one. Another option is to use a V4L2 
control.

>  };
> 
>  struct vpif_input {

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 09/13] davinci: vpif display: Add power management support

[Laurent Pinchart]

Hi Manjunath,

Thank you for the patch.

On Monday 25 June 2012 16:37:31 Manjunath Hadli wrote:
> Implement power management operations - suspend and resume as part of
> dev_pm_ops for VPIF display driver.
> 
> Signed-off-by: Manjunath Hadli <manjunath.hadli@ti.com>
> Signed-off-by: Lad, Prabhakar <prabhakar.lad@ti.com>
> ---
>  drivers/media/video/davinci/vpif_display.c |   75 +++++++++++++++++++++++++
>  1 files changed, 75 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/media/video/davinci/vpif_display.c
> b/drivers/media/video/davinci/vpif_display.c index 4436ef6..7408733 100644
> --- a/drivers/media/video/davinci/vpif_display.c
> +++ b/drivers/media/video/davinci/vpif_display.c
> @@ -1807,10 +1807,85 @@ static int vpif_remove(struct platform_device
> *device) return 0;
>  }
> 
> +#ifdef CONFIG_PM
> +static int vpif_suspend(struct device *dev)
> +{
> +	struct common_obj *common;
> +	struct channel_obj *ch;
> +	int i;
> +
> +	for (i = 0; i < VPIF_DISPLAY_MAX_DEVICES; i++) {
> +		/* Get the pointer to the channel object */
> +		ch = vpif_obj.dev[i];
> +		common = &ch->common[VPIF_VIDEO_INDEX];
> +		if (mutex_lock_interruptible(&common->lock))
> +			return -ERESTARTSYS;

I might be wrong, but I don't think the suspend/resume handlers react 
correctly to -ERESTARTSYS. If that's correct you should use mutex_lock() 
instead of mutex_lock_interruptible().

> +
> +		if (atomic_read(&ch->usrs) && common->io_usrs) {
> +			/* Disable channel */
> +			if (ch->channel_id == VPIF_CHANNEL2_VIDEO) {
> +				enable_channel2(0);
> +				channel2_intr_enable(0);
> +			}
> +			if (ch->channel_id == VPIF_CHANNEL3_VIDEO ||
> +					common->started == 2) {
> +				enable_channel3(0);
> +				channel3_intr_enable(0);
> +			}
> +		}
> +		mutex_unlock(&common->lock);
> +	}
> +
> +	return 0;
> +}
> +
> +static int vpif_resume(struct device *dev)
> +{
> +
> +	struct common_obj *common;
> +	struct channel_obj *ch;
> +	int i;
> +
> +	for (i = 0; i < VPIF_DISPLAY_MAX_DEVICES; i++) {
> +		/* Get the pointer to the channel object */
> +		ch = vpif_obj.dev[i];
> +		common = &ch->common[VPIF_VIDEO_INDEX];
> +		if (mutex_lock_interruptible(&common->lock))
> +			return -ERESTARTSYS;
> +
> +		if (atomic_read(&ch->usrs) && common->io_usrs) {
> +			/* Enable channel */
> +			if (ch->channel_id == VPIF_CHANNEL2_VIDEO) {
> +				enable_channel2(1);
> +				channel2_intr_enable(1);
> +			}
> +			if (ch->channel_id == VPIF_CHANNEL3_VIDEO ||
> +					common->started == 2) {
> +				enable_channel3(1);
> +				channel3_intr_enable(1);
> +			}
> +		}
> +		mutex_unlock(&common->lock);
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops vpif_pm = {
> +	.suspend        = vpif_suspend,
> +	.resume         = vpif_resume,
> +};
> +
> +#define vpif_pm_ops (&vpif_pm)
> +#else
> +#define vpif_pm_ops NULL
> +#endif
> +
>  static __refdata struct platform_driver vpif_driver = {
>  	.driver	= {
>  			.name	= "vpif_display",
>  			.owner	= THIS_MODULE,
> +			.pm	= vpif_pm_ops,
>  	},
>  	.probe	= vpif_probe,
>  	.remove	= vpif_remove,
-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 10/13] davinci: vpif capture:Add power management support

[Laurent Pinchart]

Hi Manjunath,

Thank you for the patch.

On Monday 25 June 2012 16:37:32 Manjunath Hadli wrote:
> Implement power management operations - suspend and resume as part of
> dev_pm_ops for VPIF capture driver.
> 
> Signed-off-by: Manjunath Hadli <manjunath.hadli@ti.com>
> Signed-off-by: Lad, Prabhakar <prabhakar.lad@ti.com>
> ---
>  drivers/media/video/davinci/vpif_capture.c |   77 +++++++++++++++++++++----
>  1 files changed, 65 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/media/video/davinci/vpif_capture.c
> b/drivers/media/video/davinci/vpif_capture.c index 097e136..f1ee137 100644
> --- a/drivers/media/video/davinci/vpif_capture.c
> +++ b/drivers/media/video/davinci/vpif_capture.c
> @@ -2300,26 +2300,74 @@ static int vpif_remove(struct platform_device
> *device) return 0;
>  }
> 
> +#ifdef CONFIG_PM
>  /**
>   * vpif_suspend: vpif device suspend
> - *
> - * TODO: Add suspend code here
>   */
> -static int
> -vpif_suspend(struct device *dev)
> +static int vpif_suspend(struct device *dev)
>  {
> -	return -1;
> +
> +	struct common_obj *common;
> +	struct channel_obj *ch;
> +	int i;
> +
> +	for (i = 0; i < VPIF_CAPTURE_MAX_DEVICES; i++) {
> +		/* Get the pointer to the channel object */
> +		ch = vpif_obj.dev[i];
> +		common = &ch->common[VPIF_VIDEO_INDEX];
> +		if (mutex_lock_interruptible(&common->lock))
> +			return -ERESTARTSYS;

As for the display driver, this should probably be replaced by mutex_lock().

> +		if (ch->usrs && common->io_usrs) {
> +			/* Disable channel */
> +			if (ch->channel_id == VPIF_CHANNEL0_VIDEO) {
> +				enable_channel0(0);
> +				channel0_intr_enable(0);
> +			}
> +			if (ch->channel_id == VPIF_CHANNEL1_VIDEO ||
> +			    common->started == 2) {
> +				enable_channel1(0);
> +				channel1_intr_enable(0);
> +			}
> +		}
> +		mutex_unlock(&common->lock);
> +	}
> +
> +	return 0;
>  }
> 
> -/**
> +/*
>   * vpif_resume: vpif device suspend
> - *
> - * TODO: Add resume code here
>   */
> -static int
> -vpif_resume(struct device *dev)
> +static int vpif_resume(struct device *dev)
>  {
> -	return -1;
> +	struct common_obj *common;
> +	struct channel_obj *ch;
> +	int i;
> +
> +	for (i = 0; i < VPIF_CAPTURE_MAX_DEVICES; i++) {
> +		/* Get the pointer to the channel object */
> +		ch = vpif_obj.dev[i];
> +		common = &ch->common[VPIF_VIDEO_INDEX];
> +		if (mutex_lock_interruptible(&common->lock))
> +			return -ERESTARTSYS;
> +
> +		if (ch->usrs && common->io_usrs) {
> +			/* Disable channel */
> +			if (ch->channel_id == VPIF_CHANNEL0_VIDEO) {
> +				enable_channel0(1);
> +				channel0_intr_enable(1);
> +			}
> +			if (ch->channel_id == VPIF_CHANNEL1_VIDEO ||
> +			    common->started == 2) {
> +				enable_channel1(1);
> +				channel1_intr_enable(1);
> +			}
> +		}
> +		mutex_unlock(&common->lock);
> +	}
> +
> +	return 0;
>  }
> 
>  static const struct dev_pm_ops vpif_dev_pm_ops = {
> @@ -2327,11 +2375,16 @@ static const struct dev_pm_ops vpif_dev_pm_ops = {
>  	.resume = vpif_resume,
>  };
> 
> +#define vpif_pm_ops (&vpif_dev_pm_ops)
> +#else
> +#define vpif_pm_ops NULL
> +#endif
> +
>  static __refdata struct platform_driver vpif_driver = {
>  	.driver	= {
>  		.name	= "vpif_capture",
>  		.owner	= THIS_MODULE,
> -		.pm = &vpif_dev_pm_ops,
> +		.pm	= vpif_pm_ops,
>  	},
>  	.probe = vpif_probe,
>  	.remove = vpif_remove,
-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 08/13] davinci: vpif: add support for clipping on output data

[Hans Verkuil]

On Mon 25 June 2012 14:54:39 Laurent Pinchart wrote:
> Hi Manjunath,
> 
> Thank you for the patch.
> 
> On Monday 25 June 2012 16:37:30 Manjunath Hadli wrote:
> > add hardware clipping support for VPIF output data. This
> > is needed as it is possible that the external encoder
> > might get confused between the FF or 00 which are a part
> > of the data and that of the SAV or EAV codes.
> > 
> > Signed-off-by: Manjunath Hadli <manjunath.hadli@ti.com>
> > Signed-off-by: Lad, Prabhakar <prabhakar.lad@ti.com>
> > ---
> >  drivers/media/video/davinci/vpif.h         |   30 +++++++++++++++++++++++++
> >  drivers/media/video/davinci/vpif_display.c |   10 +++++++++
> >  include/media/davinci/vpif_types.h         |    2 +
> >  3 files changed, 42 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/media/video/davinci/vpif.h
> > b/drivers/media/video/davinci/vpif.h index a4d2141..c2ce4d9 100644
> > --- a/drivers/media/video/davinci/vpif.h
> > +++ b/drivers/media/video/davinci/vpif.h
> > @@ -211,6 +211,12 @@ static inline void vpif_clr_bit(u32 reg, u32 bit)
> >  #define VPIF_CH3_INT_CTRL_SHIFT	(6)
> >  #define VPIF_CH_INT_CTRL_SHIFT	(6)
> > 
> > +#define VPIF_CH2_CLIP_ANC_EN	14
> > +#define VPIF_CH2_CLIP_ACTIVE_EN	13
> > +
> > +#define VPIF_CH3_CLIP_ANC_EN	14
> > +#define VPIF_CH3_CLIP_ACTIVE_EN	13
> > +
> >  /* enabled interrupt on both the fields on vpid_ch0_ctrl register */
> >  #define channel0_intr_assert()	(regw((regr(VPIF_CH0_CTRL)|\
> >  	(VPIF_INT_BOTH << VPIF_CH0_INT_CTRL_SHIFT)), VPIF_CH0_CTRL))
> > @@ -515,6 +521,30 @@ static inline void channel3_raw_enable(int enable, u8
> > index) vpif_clr_bit(VPIF_CH3_CTRL, mask);
> >  }
> > 
> > +/* function to enable clipping (for both active and blanking regions) on ch
> > 2 */ +static inline void channel2_clipping_enable(int enable)
> > +{
> > +	if (enable) {
> > +		vpif_set_bit(VPIF_CH2_CTRL, VPIF_CH2_CLIP_ANC_EN);
> > +		vpif_set_bit(VPIF_CH2_CTRL, VPIF_CH2_CLIP_ACTIVE_EN);
> > +	} else {
> > +		vpif_clr_bit(VPIF_CH2_CTRL, VPIF_CH2_CLIP_ANC_EN);
> > +		vpif_clr_bit(VPIF_CH2_CTRL, VPIF_CH2_CLIP_ACTIVE_EN);
> > +	}
> > +}
> > +
> > +/* function to enable clipping (for both active and blanking regions) on ch
> > 2 */ +static inline void channel3_clipping_enable(int enable)
> > +{
> > +	if (enable) {
> > +		vpif_set_bit(VPIF_CH3_CTRL, VPIF_CH3_CLIP_ANC_EN);
> > +		vpif_set_bit(VPIF_CH3_CTRL, VPIF_CH3_CLIP_ACTIVE_EN);
> > +	} else {
> > +		vpif_clr_bit(VPIF_CH3_CTRL, VPIF_CH3_CLIP_ANC_EN);
> > +		vpif_clr_bit(VPIF_CH3_CTRL, VPIF_CH3_CLIP_ACTIVE_EN);
> > +	}
> > +}
> > +
> >  /* inline function to set buffer addresses in case of Y/C non mux mode */
> >  static inline void ch2_set_videobuf_addr_yc_nmux(unsigned long
> > top_strt_luma, unsigned long btm_strt_luma,
> > diff --git a/drivers/media/video/davinci/vpif_display.c
> > b/drivers/media/video/davinci/vpif_display.c index 61ea8bc..4436ef6 100644
> > --- a/drivers/media/video/davinci/vpif_display.c
> > +++ b/drivers/media/video/davinci/vpif_display.c
> > @@ -1046,6 +1046,8 @@ static int vpif_streamon(struct file *file, void
> > *priv, channel2_intr_assert();
> >  			channel2_intr_enable(1);
> >  			enable_channel2(1);
> > +			if (vpif_config_data->ch2_clip_en)
> > +				channel2_clipping_enable(1);
> >  		}
> > 
> >  		if ((VPIF_CHANNEL3_VIDEO == ch->channel_id)
> > @@ -1053,6 +1055,8 @@ static int vpif_streamon(struct file *file, void
> > *priv, channel3_intr_assert();
> >  			channel3_intr_enable(1);
> >  			enable_channel3(1);
> > +			if (vpif_config_data->ch3_clip_en)
> > +				channel3_clipping_enable(1);
> >  		}
> >  		channel_first_int[VPIF_VIDEO_INDEX][ch->channel_id] = 1;
> >  	}
> > @@ -1065,6 +1069,8 @@ static int vpif_streamoff(struct file *file, void
> > *priv, struct vpif_fh *fh = priv;
> >  	struct channel_obj *ch = fh->channel;
> >  	struct common_obj *common = &ch->common[VPIF_VIDEO_INDEX];
> > +	struct vpif_display_config *vpif_config_data =
> > +					vpif_dev->platform_data;
> > 
> >  	if (buftype != V4L2_BUF_TYPE_VIDEO_OUTPUT) {
> >  		vpif_err("buffer type not supported\n");
> > @@ -1084,11 +1090,15 @@ static int vpif_streamoff(struct file *file, void
> > *priv, if (buftype == V4L2_BUF_TYPE_VIDEO_OUTPUT) {
> >  		/* disable channel */
> >  		if (VPIF_CHANNEL2_VIDEO == ch->channel_id) {
> > +			if (vpif_config_data->ch2_clip_en)
> > +				channel2_clipping_enable(0);
> >  			enable_channel2(0);
> >  			channel2_intr_enable(0);
> >  		}
> >  		if ((VPIF_CHANNEL3_VIDEO == ch->channel_id) ||
> >  					(2 == common->started)) {
> > +			if (vpif_config_data->ch3_clip_en)
> > +				channel3_clipping_enable(0);
> >  			enable_channel3(0);
> >  			channel3_intr_enable(0);
> >  		}
> > diff --git a/include/media/davinci/vpif_types.h
> > b/include/media/davinci/vpif_types.h index bd8217c..d8f6ab1 100644
> > --- a/include/media/davinci/vpif_types.h
> > +++ b/include/media/davinci/vpif_types.h
> > @@ -50,6 +50,8 @@ struct vpif_display_config {
> >  	const char **output;
> >  	int output_count;
> >  	const char *card_name;
> > +	bool ch2_clip_en;
> > +	bool ch3_clip_en;
> 
> Instead of hardcoding this in platform data, I think it would be better to 
> make this runtime-configurable. One option is to use the value of the 
> v4l2_pix_format::colorspace field configured by userspace. We already have 
> V4L2_COLORSPACE_JPEG which maps to the full 0-255 range, but we're missing a 
> colorspace for the clipped 1-254 range used by the VPIF and I'm not sure 
> whether it would really make sense to add one. Another option is to use a V4L2 
> control.

This is something for a V4L2 control I think since clipping is colorspace
independent (you have the same situation for e.g. YCbCr).

Regards,

	Hans

Re: [PATCH v3 08/13] davinci: vpif: add support for clipping on output data

[Laurent Pinchart]

Hi Hans,

On Monday 25 June 2012 15:08:10 Hans Verkuil wrote:
> On Mon 25 June 2012 14:54:39 Laurent Pinchart wrote:
> > On Monday 25 June 2012 16:37:30 Manjunath Hadli wrote:
> > > add hardware clipping support for VPIF output data. This
> > > is needed as it is possible that the external encoder
> > > might get confused between the FF or 00 which are a part
> > > of the data and that of the SAV or EAV codes.
> > > 
> > > Signed-off-by: Manjunath Hadli <manjunath.hadli@ti.com>
> > > Signed-off-by: Lad, Prabhakar <prabhakar.lad@ti.com>
> > > ---
> > > 
> > >  drivers/media/video/davinci/vpif.h         |   30 +++++++++++++++++++++
> > >  drivers/media/video/davinci/vpif_display.c |   10 +++++++++
> > >  include/media/davinci/vpif_types.h         |    2 +
> > >  3 files changed, 42 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/drivers/media/video/davinci/vpif.h
> > > b/drivers/media/video/davinci/vpif.h index a4d2141..c2ce4d9 100644
> > > --- a/drivers/media/video/davinci/vpif.h
> > > +++ b/drivers/media/video/davinci/vpif.h
> > > @@ -211,6 +211,12 @@ static inline void vpif_clr_bit(u32 reg, u32 bit)
> > > 
> > >  #define VPIF_CH3_INT_CTRL_SHIFT	(6)
> > >  #define VPIF_CH_INT_CTRL_SHIFT	(6)
> > > 
> > > +#define VPIF_CH2_CLIP_ANC_EN	14
> > > +#define VPIF_CH2_CLIP_ACTIVE_EN	13
> > > +
> > > +#define VPIF_CH3_CLIP_ANC_EN	14
> > > +#define VPIF_CH3_CLIP_ACTIVE_EN	13
> > > +
> > > 
> > >  /* enabled interrupt on both the fields on vpid_ch0_ctrl register */
> > >  #define channel0_intr_assert()	(regw((regr(VPIF_CH0_CTRL)|\
> > >  
> > >  	(VPIF_INT_BOTH << VPIF_CH0_INT_CTRL_SHIFT)), VPIF_CH0_CTRL))
> > > 
> > > @@ -515,6 +521,30 @@ static inline void channel3_raw_enable(int enable,
> > > u8
> > > index) vpif_clr_bit(VPIF_CH3_CTRL, mask);
> > > 
> > >  }
> > > 
> > > +/* function to enable clipping (for both active and blanking regions)
> > > on ch 2 */ +static inline void channel2_clipping_enable(int enable)
> > > +{
> > > +	if (enable) {
> > > +		vpif_set_bit(VPIF_CH2_CTRL, VPIF_CH2_CLIP_ANC_EN);
> > > +		vpif_set_bit(VPIF_CH2_CTRL, VPIF_CH2_CLIP_ACTIVE_EN);
> > > +	} else {
> > > +		vpif_clr_bit(VPIF_CH2_CTRL, VPIF_CH2_CLIP_ANC_EN);
> > > +		vpif_clr_bit(VPIF_CH2_CTRL, VPIF_CH2_CLIP_ACTIVE_EN);
> > > +	}
> > > +}
> > > +
> > > +/* function to enable clipping (for both active and blanking regions)
> > > on ch 2 */ +static inline void channel3_clipping_enable(int enable)
> > > +{
> > > +	if (enable) {
> > > +		vpif_set_bit(VPIF_CH3_CTRL, VPIF_CH3_CLIP_ANC_EN);
> > > +		vpif_set_bit(VPIF_CH3_CTRL, VPIF_CH3_CLIP_ACTIVE_EN);
> > > +	} else {
> > > +		vpif_clr_bit(VPIF_CH3_CTRL, VPIF_CH3_CLIP_ANC_EN);
> > > +		vpif_clr_bit(VPIF_CH3_CTRL, VPIF_CH3_CLIP_ACTIVE_EN);
> > > +	}
> > > +}
> > > +
> > > 
> > >  /* inline function to set buffer addresses in case of Y/C non mux mode
> > >  */
> > >  static inline void ch2_set_videobuf_addr_yc_nmux(unsigned long
> > > 
> > > top_strt_luma, unsigned long btm_strt_luma,
> > > diff --git a/drivers/media/video/davinci/vpif_display.c
> > > b/drivers/media/video/davinci/vpif_display.c index 61ea8bc..4436ef6
> > > 100644
> > > --- a/drivers/media/video/davinci/vpif_display.c
> > > +++ b/drivers/media/video/davinci/vpif_display.c
> > > @@ -1046,6 +1046,8 @@ static int vpif_streamon(struct file *file, void
> > > *priv, channel2_intr_assert();
> > > 
> > >  			channel2_intr_enable(1);
> > >  			enable_channel2(1);
> > > 
> > > +			if (vpif_config_data->ch2_clip_en)
> > > +				channel2_clipping_enable(1);
> > > 
> > >  		}
> > >  		
> > >  		if ((VPIF_CHANNEL3_VIDEO == ch->channel_id)
> > > 
> > > @@ -1053,6 +1055,8 @@ static int vpif_streamon(struct file *file, void
> > > *priv, channel3_intr_assert();
> > > 
> > >  			channel3_intr_enable(1);
> > >  			enable_channel3(1);
> > > 
> > > +			if (vpif_config_data->ch3_clip_en)
> > > +				channel3_clipping_enable(1);
> > > 
> > >  		}
> > >  		channel_first_int[VPIF_VIDEO_INDEX][ch->channel_id] = 1;
> > >  	
> > >  	}
> > > 
> > > @@ -1065,6 +1069,8 @@ static int vpif_streamoff(struct file *file, void
> > > *priv, struct vpif_fh *fh = priv;
> > > 
> > >  	struct channel_obj *ch = fh->channel;
> > >  	struct common_obj *common = &ch->common[VPIF_VIDEO_INDEX];
> > > 
> > > +	struct vpif_display_config *vpif_config_data =
> > > +					vpif_dev->platform_data;
> > > 
> > >  	if (buftype != V4L2_BUF_TYPE_VIDEO_OUTPUT) {
> > >  	
> > >  		vpif_err("buffer type not supported\n");
> > > 
> > > @@ -1084,11 +1090,15 @@ static int vpif_streamoff(struct file *file,
> > > void
> > > *priv, if (buftype == V4L2_BUF_TYPE_VIDEO_OUTPUT) {
> > > 
> > >  		/* disable channel */
> > >  		if (VPIF_CHANNEL2_VIDEO == ch->channel_id) {
> > > 
> > > +			if (vpif_config_data->ch2_clip_en)
> > > +				channel2_clipping_enable(0);
> > > 
> > >  			enable_channel2(0);
> > >  			channel2_intr_enable(0);
> > >  		
> > >  		}
> > >  		if ((VPIF_CHANNEL3_VIDEO == ch->channel_id) ||
> > >  		
> > >  					(2 == common->started)) {
> > > 
> > > +			if (vpif_config_data->ch3_clip_en)
> > > +				channel3_clipping_enable(0);
> > > 
> > >  			enable_channel3(0);
> > >  			channel3_intr_enable(0);
> > >  		
> > >  		}
> > > 
> > > diff --git a/include/media/davinci/vpif_types.h
> > > b/include/media/davinci/vpif_types.h index bd8217c..d8f6ab1 100644
> > > --- a/include/media/davinci/vpif_types.h
> > > +++ b/include/media/davinci/vpif_types.h
> > > @@ -50,6 +50,8 @@ struct vpif_display_config {
> > > 
> > >  	const char **output;
> > >  	int output_count;
> > >  	const char *card_name;
> > > 
> > > +	bool ch2_clip_en;
> > > +	bool ch3_clip_en;
> > 
> > Instead of hardcoding this in platform data, I think it would be better to
> > make this runtime-configurable. One option is to use the value of the
> > v4l2_pix_format::colorspace field configured by userspace. We already have
> > V4L2_COLORSPACE_JPEG which maps to the full 0-255 range, but we're missing
> > a colorspace for the clipped 1-254 range used by the VPIF and I'm not
> > sure whether it would really make sense to add one. Another option is to
> > use a V4L2 control.
> 
> This is something for a V4L2 control I think since clipping is colorspace
> independent (you have the same situation for e.g. YCbCr).

But some colorspaces define ranges smaller than 0-255. In this particular case 
I agree with you though, as the hardware clips to 1-255 without taking 
colorspaces into account.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 08/13] davinci: vpif: add support for clipping on output data

[Hans Verkuil]

On Mon 25 June 2012 15:18:41 Laurent Pinchart wrote:
> Hi Hans,
> 
> On Monday 25 June 2012 15:08:10 Hans Verkuil wrote:
> > On Mon 25 June 2012 14:54:39 Laurent Pinchart wrote:
> > > On Monday 25 June 2012 16:37:30 Manjunath Hadli wrote:
> > > > add hardware clipping support for VPIF output data. This
> > > > is needed as it is possible that the external encoder
> > > > might get confused between the FF or 00 which are a part
> > > > of the data and that of the SAV or EAV codes.
> > > > 
> > > > Signed-off-by: Manjunath Hadli <manjunath.hadli@ti.com>
> > > > Signed-off-by: Lad, Prabhakar <prabhakar.lad@ti.com>
> > > > ---
> > > > 
> > > >  drivers/media/video/davinci/vpif.h         |   30 +++++++++++++++++++++
> > > >  drivers/media/video/davinci/vpif_display.c |   10 +++++++++
> > > >  include/media/davinci/vpif_types.h         |    2 +
> > > >  3 files changed, 42 insertions(+), 0 deletions(-)
> > > > 
> > > > diff --git a/drivers/media/video/davinci/vpif.h
> > > > b/drivers/media/video/davinci/vpif.h index a4d2141..c2ce4d9 100644
> > > > --- a/drivers/media/video/davinci/vpif.h
> > > > +++ b/drivers/media/video/davinci/vpif.h
> > > > @@ -211,6 +211,12 @@ static inline void vpif_clr_bit(u32 reg, u32 bit)
> > > > 
> > > >  #define VPIF_CH3_INT_CTRL_SHIFT	(6)
> > > >  #define VPIF_CH_INT_CTRL_SHIFT	(6)
> > > > 
> > > > +#define VPIF_CH2_CLIP_ANC_EN	14
> > > > +#define VPIF_CH2_CLIP_ACTIVE_EN	13
> > > > +
> > > > +#define VPIF_CH3_CLIP_ANC_EN	14
> > > > +#define VPIF_CH3_CLIP_ACTIVE_EN	13
> > > > +
> > > > 
> > > >  /* enabled interrupt on both the fields on vpid_ch0_ctrl register */
> > > >  #define channel0_intr_assert()	(regw((regr(VPIF_CH0_CTRL)|\
> > > >  
> > > >  	(VPIF_INT_BOTH << VPIF_CH0_INT_CTRL_SHIFT)), VPIF_CH0_CTRL))
> > > > 
> > > > @@ -515,6 +521,30 @@ static inline void channel3_raw_enable(int enable,
> > > > u8
> > > > index) vpif_clr_bit(VPIF_CH3_CTRL, mask);
> > > > 
> > > >  }
> > > > 
> > > > +/* function to enable clipping (for both active and blanking regions)
> > > > on ch 2 */ +static inline void channel2_clipping_enable(int enable)
> > > > +{
> > > > +	if (enable) {
> > > > +		vpif_set_bit(VPIF_CH2_CTRL, VPIF_CH2_CLIP_ANC_EN);
> > > > +		vpif_set_bit(VPIF_CH2_CTRL, VPIF_CH2_CLIP_ACTIVE_EN);
> > > > +	} else {
> > > > +		vpif_clr_bit(VPIF_CH2_CTRL, VPIF_CH2_CLIP_ANC_EN);
> > > > +		vpif_clr_bit(VPIF_CH2_CTRL, VPIF_CH2_CLIP_ACTIVE_EN);
> > > > +	}
> > > > +}
> > > > +
> > > > +/* function to enable clipping (for both active and blanking regions)
> > > > on ch 2 */ +static inline void channel3_clipping_enable(int enable)
> > > > +{
> > > > +	if (enable) {
> > > > +		vpif_set_bit(VPIF_CH3_CTRL, VPIF_CH3_CLIP_ANC_EN);
> > > > +		vpif_set_bit(VPIF_CH3_CTRL, VPIF_CH3_CLIP_ACTIVE_EN);
> > > > +	} else {
> > > > +		vpif_clr_bit(VPIF_CH3_CTRL, VPIF_CH3_CLIP_ANC_EN);
> > > > +		vpif_clr_bit(VPIF_CH3_CTRL, VPIF_CH3_CLIP_ACTIVE_EN);
> > > > +	}
> > > > +}
> > > > +
> > > > 
> > > >  /* inline function to set buffer addresses in case of Y/C non mux mode
> > > >  */
> > > >  static inline void ch2_set_videobuf_addr_yc_nmux(unsigned long
> > > > 
> > > > top_strt_luma, unsigned long btm_strt_luma,
> > > > diff --git a/drivers/media/video/davinci/vpif_display.c
> > > > b/drivers/media/video/davinci/vpif_display.c index 61ea8bc..4436ef6
> > > > 100644
> > > > --- a/drivers/media/video/davinci/vpif_display.c
> > > > +++ b/drivers/media/video/davinci/vpif_display.c
> > > > @@ -1046,6 +1046,8 @@ static int vpif_streamon(struct file *file, void
> > > > *priv, channel2_intr_assert();
> > > > 
> > > >  			channel2_intr_enable(1);
> > > >  			enable_channel2(1);
> > > > 
> > > > +			if (vpif_config_data->ch2_clip_en)
> > > > +				channel2_clipping_enable(1);
> > > > 
> > > >  		}
> > > >  		
> > > >  		if ((VPIF_CHANNEL3_VIDEO == ch->channel_id)
> > > > 
> > > > @@ -1053,6 +1055,8 @@ static int vpif_streamon(struct file *file, void
> > > > *priv, channel3_intr_assert();
> > > > 
> > > >  			channel3_intr_enable(1);
> > > >  			enable_channel3(1);
> > > > 
> > > > +			if (vpif_config_data->ch3_clip_en)
> > > > +				channel3_clipping_enable(1);
> > > > 
> > > >  		}
> > > >  		channel_first_int[VPIF_VIDEO_INDEX][ch->channel_id] = 1;
> > > >  	
> > > >  	}
> > > > 
> > > > @@ -1065,6 +1069,8 @@ static int vpif_streamoff(struct file *file, void
> > > > *priv, struct vpif_fh *fh = priv;
> > > > 
> > > >  	struct channel_obj *ch = fh->channel;
> > > >  	struct common_obj *common = &ch->common[VPIF_VIDEO_INDEX];
> > > > 
> > > > +	struct vpif_display_config *vpif_config_data =
> > > > +					vpif_dev->platform_data;
> > > > 
> > > >  	if (buftype != V4L2_BUF_TYPE_VIDEO_OUTPUT) {
> > > >  	
> > > >  		vpif_err("buffer type not supported\n");
> > > > 
> > > > @@ -1084,11 +1090,15 @@ static int vpif_streamoff(struct file *file,
> > > > void
> > > > *priv, if (buftype == V4L2_BUF_TYPE_VIDEO_OUTPUT) {
> > > > 
> > > >  		/* disable channel */
> > > >  		if (VPIF_CHANNEL2_VIDEO == ch->channel_id) {
> > > > 
> > > > +			if (vpif_config_data->ch2_clip_en)
> > > > +				channel2_clipping_enable(0);
> > > > 
> > > >  			enable_channel2(0);
> > > >  			channel2_intr_enable(0);
> > > >  		
> > > >  		}
> > > >  		if ((VPIF_CHANNEL3_VIDEO == ch->channel_id) ||
> > > >  		
> > > >  					(2 == common->started)) {
> > > > 
> > > > +			if (vpif_config_data->ch3_clip_en)
> > > > +				channel3_clipping_enable(0);
> > > > 
> > > >  			enable_channel3(0);
> > > >  			channel3_intr_enable(0);
> > > >  		
> > > >  		}
> > > > 
> > > > diff --git a/include/media/davinci/vpif_types.h
> > > > b/include/media/davinci/vpif_types.h index bd8217c..d8f6ab1 100644
> > > > --- a/include/media/davinci/vpif_types.h
> > > > +++ b/include/media/davinci/vpif_types.h
> > > > @@ -50,6 +50,8 @@ struct vpif_display_config {
> > > > 
> > > >  	const char **output;
> > > >  	int output_count;
> > > >  	const char *card_name;
> > > > 
> > > > +	bool ch2_clip_en;
> > > > +	bool ch3_clip_en;
> > > 
> > > Instead of hardcoding this in platform data, I think it would be better to
> > > make this runtime-configurable. One option is to use the value of the
> > > v4l2_pix_format::colorspace field configured by userspace. We already have
> > > V4L2_COLORSPACE_JPEG which maps to the full 0-255 range, but we're missing
> > > a colorspace for the clipped 1-254 range used by the VPIF and I'm not
> > > sure whether it would really make sense to add one. Another option is to
> > > use a V4L2 control.
> > 
> > This is something for a V4L2 control I think since clipping is colorspace
> > independent (you have the same situation for e.g. YCbCr).
> 
> But some colorspaces define ranges smaller than 0-255.

Just because they define a smaller range doesn't mean you actually get that.
>From personal experience I can assure you that those are two different things :-)

Regards,

	Hans

> In this particular case 
> I agree with you though, as the hardware clips to 1-255 without taking 
> colorspaces into account.
> 
> 

Re: [PATCH v3 04/13] davinci: vpif: fix setting of data width in config_vpif_params() function

[Sergei Shtylyov]

Hello.

On 25-06-2012 15:07, Manjunath Hadli wrote:

> fix setting of data width in config_vpif_params() function,
> which was wrongly set.

> Signed-off-by: Manjunath Hadli<manjunath.hadli@ti.com>
> Signed-off-by: Lad, Prabhakar<prabhakar.lad@ti.com>
> ---
>   drivers/media/video/davinci/vpif.c |    2 +-
>   1 files changed, 1 insertions(+), 1 deletions(-)

> diff --git a/drivers/media/video/davinci/vpif.c b/drivers/media/video/davinci/vpif.c
> index 774bcd3..08fb81f 100644
> --- a/drivers/media/video/davinci/vpif.c
> +++ b/drivers/media/video/davinci/vpif.c
> @@ -346,7 +346,7 @@ static void config_vpif_params(struct vpif_params *vpifparams,
>
>   			value = regr(reg);
>   			/* Set data width */
> -			value&= ((~(unsigned int)(0x3))<<
> +			value&= ~(((unsigned int)(0x3))<<

    Why not just 0x3u instead of (unsigned int)(0x3)?

WBR, Sergei

RE: [PATCH v3 08/13] davinci: vpif: add support for clipping on output data

[Hadli, Manjunath]

Hi,

On Mon, Jun 25, 2012 at 18:53:20, Hans Verkuil wrote:
> On Mon 25 June 2012 15:18:41 Laurent Pinchart wrote:
> > Hi Hans,
> > 
> > On Monday 25 June 2012 15:08:10 Hans Verkuil wrote:
> > > On Mon 25 June 2012 14:54:39 Laurent Pinchart wrote:
> > > > On Monday 25 June 2012 16:37:30 Manjunath Hadli wrote:
> > > > > add hardware clipping support for VPIF output data. This
> > > > > is needed as it is possible that the external encoder
> > > > > might get confused between the FF or 00 which are a part
> > > > > of the data and that of the SAV or EAV codes.
> > > > > 
> > > > > Signed-off-by: Manjunath Hadli <manjunath.hadli@ti.com>
> > > > > Signed-off-by: Lad, Prabhakar <prabhakar.lad@ti.com>
> > > > > ---
> > > > > 
> > > > >  drivers/media/video/davinci/vpif.h         |   30 +++++++++++++++++++++
> > > > >  drivers/media/video/davinci/vpif_display.c |   10 +++++++++
> > > > >  include/media/davinci/vpif_types.h         |    2 +
> > > > >  3 files changed, 42 insertions(+), 0 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/media/video/davinci/vpif.h
> > > > > b/drivers/media/video/davinci/vpif.h index a4d2141..c2ce4d9 100644
> > > > > --- a/drivers/media/video/davinci/vpif.h
> > > > > +++ b/drivers/media/video/davinci/vpif.h
> > > > > @@ -211,6 +211,12 @@ static inline void vpif_clr_bit(u32 reg, u32 bit)
> > > > > 
> > > > >  #define VPIF_CH3_INT_CTRL_SHIFT	(6)
> > > > >  #define VPIF_CH_INT_CTRL_SHIFT	(6)
> > > > > 
> > > > > +#define VPIF_CH2_CLIP_ANC_EN	14
> > > > > +#define VPIF_CH2_CLIP_ACTIVE_EN	13
> > > > > +
> > > > > +#define VPIF_CH3_CLIP_ANC_EN	14
> > > > > +#define VPIF_CH3_CLIP_ACTIVE_EN	13
> > > > > +
> > > > > 
> > > > >  /* enabled interrupt on both the fields on vpid_ch0_ctrl register */
> > > > >  #define channel0_intr_assert()	(regw((regr(VPIF_CH0_CTRL)|\
> > > > >  
> > > > >  	(VPIF_INT_BOTH << VPIF_CH0_INT_CTRL_SHIFT)), VPIF_CH0_CTRL))
> > > > > 
> > > > > @@ -515,6 +521,30 @@ static inline void channel3_raw_enable(int enable,
> > > > > u8
> > > > > index) vpif_clr_bit(VPIF_CH3_CTRL, mask);
> > > > > 
> > > > >  }
> > > > > 
> > > > > +/* function to enable clipping (for both active and blanking regions)
> > > > > on ch 2 */ +static inline void channel2_clipping_enable(int enable)
> > > > > +{
> > > > > +	if (enable) {
> > > > > +		vpif_set_bit(VPIF_CH2_CTRL, VPIF_CH2_CLIP_ANC_EN);
> > > > > +		vpif_set_bit(VPIF_CH2_CTRL, VPIF_CH2_CLIP_ACTIVE_EN);
> > > > > +	} else {
> > > > > +		vpif_clr_bit(VPIF_CH2_CTRL, VPIF_CH2_CLIP_ANC_EN);
> > > > > +		vpif_clr_bit(VPIF_CH2_CTRL, VPIF_CH2_CLIP_ACTIVE_EN);
> > > > > +	}
> > > > > +}
> > > > > +
> > > > > +/* function to enable clipping (for both active and blanking regions)
> > > > > on ch 2 */ +static inline void channel3_clipping_enable(int enable)
> > > > > +{
> > > > > +	if (enable) {
> > > > > +		vpif_set_bit(VPIF_CH3_CTRL, VPIF_CH3_CLIP_ANC_EN);
> > > > > +		vpif_set_bit(VPIF_CH3_CTRL, VPIF_CH3_CLIP_ACTIVE_EN);
> > > > > +	} else {
> > > > > +		vpif_clr_bit(VPIF_CH3_CTRL, VPIF_CH3_CLIP_ANC_EN);
> > > > > +		vpif_clr_bit(VPIF_CH3_CTRL, VPIF_CH3_CLIP_ACTIVE_EN);
> > > > > +	}
> > > > > +}
> > > > > +
> > > > > 
> > > > >  /* inline function to set buffer addresses in case of Y/C non mux mode
> > > > >  */
> > > > >  static inline void ch2_set_videobuf_addr_yc_nmux(unsigned long
> > > > > 
> > > > > top_strt_luma, unsigned long btm_strt_luma,
> > > > > diff --git a/drivers/media/video/davinci/vpif_display.c
> > > > > b/drivers/media/video/davinci/vpif_display.c index 61ea8bc..4436ef6
> > > > > 100644
> > > > > --- a/drivers/media/video/davinci/vpif_display.c
> > > > > +++ b/drivers/media/video/davinci/vpif_display.c
> > > > > @@ -1046,6 +1046,8 @@ static int vpif_streamon(struct file *file, void
> > > > > *priv, channel2_intr_assert();
> > > > > 
> > > > >  			channel2_intr_enable(1);
> > > > >  			enable_channel2(1);
> > > > > 
> > > > > +			if (vpif_config_data->ch2_clip_en)
> > > > > +				channel2_clipping_enable(1);
> > > > > 
> > > > >  		}
> > > > >  		
> > > > >  		if ((VPIF_CHANNEL3_VIDEO == ch->channel_id)
> > > > > 
> > > > > @@ -1053,6 +1055,8 @@ static int vpif_streamon(struct file *file, void
> > > > > *priv, channel3_intr_assert();
> > > > > 
> > > > >  			channel3_intr_enable(1);
> > > > >  			enable_channel3(1);
> > > > > 
> > > > > +			if (vpif_config_data->ch3_clip_en)
> > > > > +				channel3_clipping_enable(1);
> > > > > 
> > > > >  		}
> > > > >  		channel_first_int[VPIF_VIDEO_INDEX][ch->channel_id] = 1;
> > > > >  	
> > > > >  	}
> > > > > 
> > > > > @@ -1065,6 +1069,8 @@ static int vpif_streamoff(struct file *file, void
> > > > > *priv, struct vpif_fh *fh = priv;
> > > > > 
> > > > >  	struct channel_obj *ch = fh->channel;
> > > > >  	struct common_obj *common = &ch->common[VPIF_VIDEO_INDEX];
> > > > > 
> > > > > +	struct vpif_display_config *vpif_config_data =
> > > > > +					vpif_dev->platform_data;
> > > > > 
> > > > >  	if (buftype != V4L2_BUF_TYPE_VIDEO_OUTPUT) {
> > > > >  	
> > > > >  		vpif_err("buffer type not supported\n");
> > > > > 
> > > > > @@ -1084,11 +1090,15 @@ static int vpif_streamoff(struct file *file,
> > > > > void
> > > > > *priv, if (buftype == V4L2_BUF_TYPE_VIDEO_OUTPUT) {
> > > > > 
> > > > >  		/* disable channel */
> > > > >  		if (VPIF_CHANNEL2_VIDEO == ch->channel_id) {
> > > > > 
> > > > > +			if (vpif_config_data->ch2_clip_en)
> > > > > +				channel2_clipping_enable(0);
> > > > > 
> > > > >  			enable_channel2(0);
> > > > >  			channel2_intr_enable(0);
> > > > >  		
> > > > >  		}
> > > > >  		if ((VPIF_CHANNEL3_VIDEO == ch->channel_id) ||
> > > > >  		
> > > > >  					(2 == common->started)) {
> > > > > 
> > > > > +			if (vpif_config_data->ch3_clip_en)
> > > > > +				channel3_clipping_enable(0);
> > > > > 
> > > > >  			enable_channel3(0);
> > > > >  			channel3_intr_enable(0);
> > > > >  		
> > > > >  		}
> > > > > 
> > > > > diff --git a/include/media/davinci/vpif_types.h
> > > > > b/include/media/davinci/vpif_types.h index bd8217c..d8f6ab1 100644
> > > > > --- a/include/media/davinci/vpif_types.h
> > > > > +++ b/include/media/davinci/vpif_types.h
> > > > > @@ -50,6 +50,8 @@ struct vpif_display_config {
> > > > > 
> > > > >  	const char **output;
> > > > >  	int output_count;
> > > > >  	const char *card_name;
> > > > > 
> > > > > +	bool ch2_clip_en;
> > > > > +	bool ch3_clip_en;
> > > > 
> > > > Instead of hardcoding this in platform data, I think it would be better to
> > > > make this runtime-configurable. One option is to use the value of the
> > > > v4l2_pix_format::colorspace field configured by userspace. We already have
> > > > V4L2_COLORSPACE_JPEG which maps to the full 0-255 range, but we're missing
> > > > a colorspace for the clipped 1-254 range used by the VPIF and I'm not
> > > > sure whether it would really make sense to add one. Another option is to
> > > > use a V4L2 control.
> > > 
> > > This is something for a V4L2 control I think since clipping is colorspace
> > > independent (you have the same situation for e.g. YCbCr).
> > 
> > But some colorspaces define ranges smaller than 0-255.
> 
> Just because they define a smaller range doesn't mean you actually get that.
> From personal experience I can assure you that those are two different things :-)
> 
> Regards,
> 
> 	Hans
> 
> > In this particular case 
> > I agree with you though, as the hardware clips to 1-255 without taking 
> > colorspaces into account.
> > 
> > 
> 
This is not really related to color spaces strictly. Since one of the versions of this IPs
did not clip, we had issues on the encoder front with the SAV/EAV and the data getting confused 
by the encoder. The platform hard coding is required, and at this point, this is the correct way to handle it. There is no need to make this runtime configurable in this case as at no point
 we would want to disable the clipping - unless it is not supported.

Thx and Regards,
-Manju

RE: [PATCH v3 04/13] davinci: vpif: fix setting of data width in config_vpif_params() function

[Hadli, Manjunath]

Hi Sergei,

On Tue, Jun 26, 2012 at 16:06:01, Sergei Shtylyov wrote:
> Hello.
> 
> On 25-06-2012 15:07, Manjunath Hadli wrote:
> 
> > fix setting of data width in config_vpif_params() function,
> > which was wrongly set.
> 
> > Signed-off-by: Manjunath Hadli<manjunath.hadli@ti.com>
> > Signed-off-by: Lad, Prabhakar<prabhakar.lad@ti.com>
> > ---
> >   drivers/media/video/davinci/vpif.c |    2 +-
> >   1 files changed, 1 insertions(+), 1 deletions(-)
> 
> > diff --git a/drivers/media/video/davinci/vpif.c b/drivers/media/video/davinci/vpif.c
> > index 774bcd3..08fb81f 100644
> > --- a/drivers/media/video/davinci/vpif.c
> > +++ b/drivers/media/video/davinci/vpif.c
> > @@ -346,7 +346,7 @@ static void config_vpif_params(struct vpif_params *vpifparams,
> >
> >   			value = regr(reg);
> >   			/* Set data width */
> > -			value&= ((~(unsigned int)(0x3))<<
> > +			value&= ~(((unsigned int)(0x3))<<
> 
>     Why not just 0x3u instead of (unsigned int)(0x3)?
> 
      Ok.

Thx,
--Manju

> WBR, Sergei
> 

RE: [PATCH v3 10/13] davinci: vpif capture:Add power management support

[Hadli, Manjunath]

Hi Laurent,

On Mon, Jun 25, 2012 at 18:27:23, Laurent Pinchart wrote:
> Hi Manjunath,
> 
> Thank you for the patch.
> 
> On Monday 25 June 2012 16:37:32 Manjunath Hadli wrote:
> > Implement power management operations - suspend and resume as part of 
> > dev_pm_ops for VPIF capture driver.
> > 
> > Signed-off-by: Manjunath Hadli <manjunath.hadli@ti.com>
> > Signed-off-by: Lad, Prabhakar <prabhakar.lad@ti.com>
> > ---
> >  drivers/media/video/davinci/vpif_capture.c |   77 +++++++++++++++++++++----
> >  1 files changed, 65 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/media/video/davinci/vpif_capture.c
> > b/drivers/media/video/davinci/vpif_capture.c index 097e136..f1ee137 
> > 100644
> > --- a/drivers/media/video/davinci/vpif_capture.c
> > +++ b/drivers/media/video/davinci/vpif_capture.c
> > @@ -2300,26 +2300,74 @@ static int vpif_remove(struct platform_device
> > *device) return 0;
> >  }
> > 
> > +#ifdef CONFIG_PM
> >  /**
> >   * vpif_suspend: vpif device suspend
> > - *
> > - * TODO: Add suspend code here
> >   */
> > -static int
> > -vpif_suspend(struct device *dev)
> > +static int vpif_suspend(struct device *dev)
> >  {
> > -	return -1;
> > +
> > +	struct common_obj *common;
> > +	struct channel_obj *ch;
> > +	int i;
> > +
> > +	for (i = 0; i < VPIF_CAPTURE_MAX_DEVICES; i++) {
> > +		/* Get the pointer to the channel object */
> > +		ch = vpif_obj.dev[i];
> > +		common = &ch->common[VPIF_VIDEO_INDEX];
> > +		if (mutex_lock_interruptible(&common->lock))
> > +			return -ERESTARTSYS;
> 
> As for the display driver, this should probably be replaced by mutex_lock().
> 
  Ok, I'll replace it with mutex_lock().

Thx,
--Manju

> > +		if (ch->usrs && common->io_usrs) {
> > +			/* Disable channel */
> > +			if (ch->channel_id == VPIF_CHANNEL0_VIDEO) {
> > +				enable_channel0(0);
> > +				channel0_intr_enable(0);
> > +			}
> > +			if (ch->channel_id == VPIF_CHANNEL1_VIDEO ||
> > +			    common->started == 2) {
> > +				enable_channel1(0);
> > +				channel1_intr_enable(0);
> > +			}
> > +		}
> > +		mutex_unlock(&common->lock);
> > +	}
> > +
> > +	return 0;
> >  }
> > 
> > -/**
> > +/*
> >   * vpif_resume: vpif device suspend
> > - *
> > - * TODO: Add resume code here
> >   */
> > -static int
> > -vpif_resume(struct device *dev)
> > +static int vpif_resume(struct device *dev)
> >  {
> > -	return -1;
> > +	struct common_obj *common;
> > +	struct channel_obj *ch;
> > +	int i;
> > +
> > +	for (i = 0; i < VPIF_CAPTURE_MAX_DEVICES; i++) {
> > +		/* Get the pointer to the channel object */
> > +		ch = vpif_obj.dev[i];
> > +		common = &ch->common[VPIF_VIDEO_INDEX];
> > +		if (mutex_lock_interruptible(&common->lock))
> > +			return -ERESTARTSYS;
> > +
> > +		if (ch->usrs && common->io_usrs) {
> > +			/* Disable channel */
> > +			if (ch->channel_id == VPIF_CHANNEL0_VIDEO) {
> > +				enable_channel0(1);
> > +				channel0_intr_enable(1);
> > +			}
> > +			if (ch->channel_id == VPIF_CHANNEL1_VIDEO ||
> > +			    common->started == 2) {
> > +				enable_channel1(1);
> > +				channel1_intr_enable(1);
> > +			}
> > +		}
> > +		mutex_unlock(&common->lock);
> > +	}
> > +
> > +	return 0;
> >  }
> > 
> >  static const struct dev_pm_ops vpif_dev_pm_ops = { @@ -2327,11 
> > +2375,16 @@ static const struct dev_pm_ops vpif_dev_pm_ops = {
> >  	.resume = vpif_resume,
> >  };
> > 
> > +#define vpif_pm_ops (&vpif_dev_pm_ops) #else #define vpif_pm_ops NULL 
> > +#endif
> > +
> >  static __refdata struct platform_driver vpif_driver = {
> >  	.driver	= {
> >  		.name	= "vpif_capture",
> >  		.owner	= THIS_MODULE,
> > -		.pm = &vpif_dev_pm_ops,
> > +		.pm	= vpif_pm_ops,
> >  	},
> >  	.probe = vpif_probe,
> >  	.remove = vpif_remove,
> --
> Regards,
> 
> Laurent Pinchart
> 
> 

RE: [PATCH v3 09/13] davinci: vpif display: Add power management support

[Hadli, Manjunath]

Hi Laurent,

On Mon, Jun 25, 2012 at 18:26:22, Laurent Pinchart wrote:
> Hi Manjunath,
> 
> Thank you for the patch.
> 
> On Monday 25 June 2012 16:37:31 Manjunath Hadli wrote:
> > Implement power management operations - suspend and resume as part of
> > dev_pm_ops for VPIF display driver.
> > 
> > Signed-off-by: Manjunath Hadli <manjunath.hadli@ti.com>
> > Signed-off-by: Lad, Prabhakar <prabhakar.lad@ti.com>
> > ---
> >  drivers/media/video/davinci/vpif_display.c |   75 +++++++++++++++++++++++++
> >  1 files changed, 75 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/media/video/davinci/vpif_display.c
> > b/drivers/media/video/davinci/vpif_display.c index 4436ef6..7408733 100644
> > --- a/drivers/media/video/davinci/vpif_display.c
> > +++ b/drivers/media/video/davinci/vpif_display.c
> > @@ -1807,10 +1807,85 @@ static int vpif_remove(struct platform_device
> > *device) return 0;
> >  }
> > 
> > +#ifdef CONFIG_PM
> > +static int vpif_suspend(struct device *dev)
> > +{
> > +	struct common_obj *common;
> > +	struct channel_obj *ch;
> > +	int i;
> > +
> > +	for (i = 0; i < VPIF_DISPLAY_MAX_DEVICES; i++) {
> > +		/* Get the pointer to the channel object */
> > +		ch = vpif_obj.dev[i];
> > +		common = &ch->common[VPIF_VIDEO_INDEX];
> > +		if (mutex_lock_interruptible(&common->lock))
> > +			return -ERESTARTSYS;
> 
> I might be wrong, but I don't think the suspend/resume handlers react 
> correctly to -ERESTARTSYS. If that's correct you should use mutex_lock() 
> instead of mutex_lock_interruptible().
> 
  I'll replace it with mutex_lock().

Thx,
--Manju

> > +
> > +		if (atomic_read(&ch->usrs) && common->io_usrs) {
> > +			/* Disable channel */
> > +			if (ch->channel_id == VPIF_CHANNEL2_VIDEO) {
> > +				enable_channel2(0);
> > +				channel2_intr_enable(0);
> > +			}
> > +			if (ch->channel_id == VPIF_CHANNEL3_VIDEO ||
> > +					common->started == 2) {
> > +				enable_channel3(0);
> > +				channel3_intr_enable(0);
> > +			}
> > +		}
> > +		mutex_unlock(&common->lock);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int vpif_resume(struct device *dev)
> > +{
> > +
> > +	struct common_obj *common;
> > +	struct channel_obj *ch;
> > +	int i;
> > +
> > +	for (i = 0; i < VPIF_DISPLAY_MAX_DEVICES; i++) {
> > +		/* Get the pointer to the channel object */
> > +		ch = vpif_obj.dev[i];
> > +		common = &ch->common[VPIF_VIDEO_INDEX];
> > +		if (mutex_lock_interruptible(&common->lock))
> > +			return -ERESTARTSYS;
> > +
> > +		if (atomic_read(&ch->usrs) && common->io_usrs) {
> > +			/* Enable channel */
> > +			if (ch->channel_id == VPIF_CHANNEL2_VIDEO) {
> > +				enable_channel2(1);
> > +				channel2_intr_enable(1);
> > +			}
> > +			if (ch->channel_id == VPIF_CHANNEL3_VIDEO ||
> > +					common->started == 2) {
> > +				enable_channel3(1);
> > +				channel3_intr_enable(1);
> > +			}
> > +		}
> > +		mutex_unlock(&common->lock);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct dev_pm_ops vpif_pm = {
> > +	.suspend        = vpif_suspend,
> > +	.resume         = vpif_resume,
> > +};
> > +
> > +#define vpif_pm_ops (&vpif_pm)
> > +#else
> > +#define vpif_pm_ops NULL
> > +#endif
> > +
> >  static __refdata struct platform_driver vpif_driver = {
> >  	.driver	= {
> >  			.name	= "vpif_display",
> >  			.owner	= THIS_MODULE,
> > +			.pm	= vpif_pm_ops,
> >  	},
> >  	.probe	= vpif_probe,
> >  	.remove	= vpif_remove,
> -- 
> Regards,
> 
> Laurent Pinchart
> 
>