Hi Prashant,
Sorry for the very late review, I finally have time today to go through
my pending patches.
I have one question, see below:
On 04/23/2015 10:59 AM, Prashant Laddha wrote:
> Extended detect_cvt/gtf API to indicate the format type (interlaced
> or progressive). In case of interlaced, the vertical front and back
> porch and vsync values for both (odd,even) fields are considered to
> derive image height. Populated vsync, verical front, back porch
> values in bt timing structure for even and odd fields and updated
> the flags appropriately.
>
> Also modified the functions calling the detect_cvt/gtf(). As of now
> these functions are calling detect_cvt/gtf() with interlaced flag
> set to false.
>
> Cc: Hans Verkuil <hans.verkuil@xxxxxxxxx>
> Cc: Martin Bugge <marbugge@xxxxxxxxx>
> Cc: Mats Randgaard <matrandg@xxxxxxxxx>
> Cc: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> Signed-off-by: Prashant Laddha <prladdha@xxxxxxxxx>
> ---
> drivers/media/i2c/adv7604.c | 4 +--
> drivers/media/i2c/adv7842.c | 4 +--
> drivers/media/platform/vivid/vivid-vid-cap.c | 5 ++--
> drivers/media/v4l2-core/v4l2-dv-timings.c | 39 +++++++++++++++++++++++-----
> include/media/v4l2-dv-timings.h | 6 +++--
> 5 files changed, 44 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/media/i2c/adv7604.c b/drivers/media/i2c/adv7604.c
> index 60ffcf0..74abfd4 100644
> --- a/drivers/media/i2c/adv7604.c
> +++ b/drivers/media/i2c/adv7604.c
> @@ -1304,12 +1304,12 @@ static int stdi2dv_timings(struct v4l2_subdev *sd,
> if (v4l2_detect_cvt(stdi->lcf + 1, hfreq, stdi->lcvs,
> (stdi->hs_pol == '+' ? V4L2_DV_HSYNC_POS_POL : 0) |
> (stdi->vs_pol == '+' ? V4L2_DV_VSYNC_POS_POL : 0),
> - timings))
> + false, timings))
> return 0;
> if (v4l2_detect_gtf(stdi->lcf + 1, hfreq, stdi->lcvs,
> (stdi->hs_pol == '+' ? V4L2_DV_HSYNC_POS_POL : 0) |
> (stdi->vs_pol == '+' ? V4L2_DV_VSYNC_POS_POL : 0),
> - state->aspect_ratio, timings))
> + false, state->aspect_ratio, timings))
> return 0;
>
> v4l2_dbg(2, debug, sd,
> diff --git a/drivers/media/i2c/adv7842.c b/drivers/media/i2c/adv7842.c
> index b5a37fe..90cbead 100644
> --- a/drivers/media/i2c/adv7842.c
> +++ b/drivers/media/i2c/adv7842.c
> @@ -1333,12 +1333,12 @@ static int stdi2dv_timings(struct v4l2_subdev *sd,
> if (v4l2_detect_cvt(stdi->lcf + 1, hfreq, stdi->lcvs,
> (stdi->hs_pol == '+' ? V4L2_DV_HSYNC_POS_POL : 0) |
> (stdi->vs_pol == '+' ? V4L2_DV_VSYNC_POS_POL : 0),
> - timings))
> + false, timings))
> return 0;
> if (v4l2_detect_gtf(stdi->lcf + 1, hfreq, stdi->lcvs,
> (stdi->hs_pol == '+' ? V4L2_DV_HSYNC_POS_POL : 0) |
> (stdi->vs_pol == '+' ? V4L2_DV_VSYNC_POS_POL : 0),
> - state->aspect_ratio, timings))
> + false, state->aspect_ratio, timings))
> return 0;
>
> v4l2_dbg(2, debug, sd,
> diff --git a/drivers/media/platform/vivid/vivid-vid-cap.c b/drivers/media/platform/vivid/vivid-vid-cap.c
> index be10b72..a3b19dc 100644
> --- a/drivers/media/platform/vivid/vivid-vid-cap.c
> +++ b/drivers/media/platform/vivid/vivid-vid-cap.c
> @@ -1623,7 +1623,7 @@ static bool valid_cvt_gtf_timings(struct v4l2_dv_timings *timings)
>
> if (bt->standards == 0 || (bt->standards & V4L2_DV_BT_STD_CVT)) {
> if (v4l2_detect_cvt(total_v_lines, h_freq, bt->vsync,
> - bt->polarities, timings))
> + bt->polarities, false, timings))
> return true;
> }
>
> @@ -1634,7 +1634,8 @@ static bool valid_cvt_gtf_timings(struct v4l2_dv_timings *timings)
> &aspect_ratio.numerator,
> &aspect_ratio.denominator);
> if (v4l2_detect_gtf(total_v_lines, h_freq, bt->vsync,
> - bt->polarities, aspect_ratio, timings))
> + bt->polarities, false,
> + aspect_ratio, timings))
> return true;
> }
> return false;
> diff --git a/drivers/media/v4l2-core/v4l2-dv-timings.c b/drivers/media/v4l2-core/v4l2-dv-timings.c
> index 37f0d6f..86e11d1 100644
> --- a/drivers/media/v4l2-core/v4l2-dv-timings.c
> +++ b/drivers/media/v4l2-core/v4l2-dv-timings.c
> @@ -338,6 +338,7 @@ EXPORT_SYMBOL_GPL(v4l2_print_dv_timings);
> * @vsync - the height of the vertical sync in lines.
> * @polarities - the horizontal and vertical polarities (same as struct
> * v4l2_bt_timings polarities).
> + * @interlaced - if this flag is true, it indicates interlaced format
> * @fmt - the resulting timings.
> *
> * This function will attempt to detect if the given values correspond to a
> @@ -349,7 +350,7 @@ EXPORT_SYMBOL_GPL(v4l2_print_dv_timings);
> * detection function.
> */
> bool v4l2_detect_cvt(unsigned frame_height, unsigned hfreq, unsigned vsync,
> - u32 polarities, struct v4l2_dv_timings *fmt)
> + u32 polarities, bool interlaced, struct v4l2_dv_timings *fmt)
> {
> int v_fp, v_bp, h_fp, h_bp, hsync;
> int frame_width, image_height, image_width;
> @@ -384,7 +385,11 @@ bool v4l2_detect_cvt(unsigned frame_height, unsigned hfreq, unsigned vsync,
> if (v_bp < CVT_MIN_V_BPORCH)
> v_bp = CVT_MIN_V_BPORCH;
> }
> - image_height = (frame_height - v_fp - vsync - v_bp + 1) & ~0x1;
> +
> + if (interlaced)
> + image_height = (frame_height - 2 * v_fp - 2 * vsync - 2 * v_bp) & ~0x1;
> + else
> + image_height = (frame_height - v_fp - vsync - v_bp + 1) & ~0x1;
>
> if (image_height < 0)
> return false;
> @@ -457,11 +462,20 @@ bool v4l2_detect_cvt(unsigned frame_height, unsigned hfreq, unsigned vsync,
> fmt->bt.hsync = hsync;
> fmt->bt.vsync = vsync;
> fmt->bt.hbackporch = frame_width - image_width - h_fp - hsync;
> - fmt->bt.vbackporch = frame_height - image_height - v_fp - vsync;
> + fmt->bt.vbackporch = v_bp;
> fmt->bt.pixelclock = pix_clk;
> fmt->bt.standards = V4L2_DV_BT_STD_CVT;
> if (reduced_blanking)
> fmt->bt.flags |= V4L2_DV_FL_REDUCED_BLANKING;
> + if (interlaced) {
> + fmt->bt.interlaced = V4L2_DV_INTERLACED;
> + fmt->bt.il_vfrontporch = v_fp;
> + fmt->bt.il_vsync = vsync;
> + fmt->bt.il_vbackporch = v_bp + 1;
> + fmt->bt.flags |= V4L2_DV_FL_HALF_LINE;
> + } else {
> + fmt->bt.interlaced = V4L2_DV_PROGRESSIVE;
> + }
> return true;
> }
> EXPORT_SYMBOL_GPL(v4l2_detect_cvt);
> @@ -500,6 +514,7 @@ EXPORT_SYMBOL_GPL(v4l2_detect_cvt);
> * @vsync - the height of the vertical sync in lines.
> * @polarities - the horizontal and vertical polarities (same as struct
> * v4l2_bt_timings polarities).
> + * @interlaced - if this flag is true, it indicates interlaced format
> * @aspect - preferred aspect ratio. GTF has no method of determining the
> * aspect ratio in order to derive the image width from the
> * image height, so it has to be passed explicitly. Usually
> @@ -515,6 +530,7 @@ bool v4l2_detect_gtf(unsigned frame_height,
> unsigned hfreq,
> unsigned vsync,
> u32 polarities,
> + bool interlaced,
> struct v4l2_fract aspect,
> struct v4l2_dv_timings *fmt)
> {
> @@ -539,9 +555,11 @@ bool v4l2_detect_gtf(unsigned frame_height,
>
> /* Vertical */
> v_fp = GTF_V_FP;
> -
> v_bp = (GTF_MIN_VSYNC_BP * hfreq + 500000) / 1000000 - vsync;
> - image_height = (frame_height - v_fp - vsync - v_bp + 1) & ~0x1;
> + if (interlaced)
> + image_height = (frame_height - 2 * v_fp - 2 * vsync - 2 * v_bp) & ~0x1;
> + else
> + image_height = (frame_height - v_fp - vsync - v_bp + 1) & ~0x1;
>
> if (image_height < 0)
> return false;
> @@ -586,11 +604,20 @@ bool v4l2_detect_gtf(unsigned frame_height,
> fmt->bt.hsync = hsync;
> fmt->bt.vsync = vsync;
> fmt->bt.hbackporch = frame_width - image_width - h_fp - hsync;
> - fmt->bt.vbackporch = frame_height - image_height - v_fp - vsync;
> + fmt->bt.vbackporch = v_bp;
Is this change correct? My main concern comes from the earlier image_height calculation
in the chunk above. The image_height value is rounded to an even value, but if the value
is actually changed due to rounding, then one of the v_fp, vsync or v_bp values must
change by one as well. After all, the frame_height is fixed and image_height must be
even. And frame_height can be even or odd, both are possible. So that one line of rounding
difference must go somewhere in the blanking timings.
Regards,
Hans
> fmt->bt.pixelclock = pix_clk;
> fmt->bt.standards = V4L2_DV_BT_STD_GTF;
> if (!default_gtf)
> fmt->bt.flags |= V4L2_DV_FL_REDUCED_BLANKING;
> + if (interlaced) {
> + fmt->bt.interlaced = V4L2_DV_INTERLACED;
> + fmt->bt.il_vfrontporch = v_fp;
> + fmt->bt.il_vsync = vsync;
> + fmt->bt.il_vbackporch = v_bp + 1;
> + fmt->bt.flags |= V4L2_DV_FL_HALF_LINE;
> + } else {
> + fmt->bt.interlaced = V4L2_DV_PROGRESSIVE;
> + }
> return true;
> }
> EXPORT_SYMBOL_GPL(v4l2_detect_gtf);
> diff --git a/include/media/v4l2-dv-timings.h b/include/media/v4l2-dv-timings.h
> index 4becc67..eecd310 100644
> --- a/include/media/v4l2-dv-timings.h
> +++ b/include/media/v4l2-dv-timings.h
> @@ -117,6 +117,7 @@ void v4l2_print_dv_timings(const char *dev_prefix, const char *prefix,
> * @vsync - the height of the vertical sync in lines.
> * @polarities - the horizontal and vertical polarities (same as struct
> * v4l2_bt_timings polarities).
> + * @interlaced - if this flag is true, it indicates interlaced format
> * @fmt - the resulting timings.
> *
> * This function will attempt to detect if the given values correspond to a
> @@ -124,7 +125,7 @@ void v4l2_print_dv_timings(const char *dev_prefix, const char *prefix,
> * in with the found CVT timings.
> */
> bool v4l2_detect_cvt(unsigned frame_height, unsigned hfreq, unsigned vsync,
> - u32 polarities, struct v4l2_dv_timings *fmt);
> + u32 polarities, bool interlaced, struct v4l2_dv_timings *fmt);
>
> /** v4l2_detect_gtf - detect if the given timings follow the GTF standard
> * @frame_height - the total height of the frame (including blanking) in lines.
> @@ -132,6 +133,7 @@ bool v4l2_detect_cvt(unsigned frame_height, unsigned hfreq, unsigned vsync,
> * @vsync - the height of the vertical sync in lines.
> * @polarities - the horizontal and vertical polarities (same as struct
> * v4l2_bt_timings polarities).
> + * @interlaced - if this flag is true, it indicates interlaced format
> * @aspect - preferred aspect ratio. GTF has no method of determining the
> * aspect ratio in order to derive the image width from the
> * image height, so it has to be passed explicitly. Usually
> @@ -144,7 +146,7 @@ bool v4l2_detect_cvt(unsigned frame_height, unsigned hfreq, unsigned vsync,
> * in with the found GTF timings.
> */
> bool v4l2_detect_gtf(unsigned frame_height, unsigned hfreq, unsigned vsync,
> - u32 polarities, struct v4l2_fract aspect,
> + u32 polarities, bool interlaced, struct v4l2_fract aspect,
> struct v4l2_dv_timings *fmt);
>
> /** v4l2_calc_aspect_ratio - calculate the aspect ratio based on bytes
>
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
