* [PATCH v2] media: davinci: vpif: display: separate out subdev from output
@ 2012-09-24 10:44 Prabhakar
2012-09-24 10:59 ` Hans Verkuil
0 siblings, 1 reply; 9+ messages in thread
From: Prabhakar @ 2012-09-24 10:44 UTC (permalink / raw)
To: LMML
Cc: DLOS, LAK, Sekhar Nori, Mauro Carvalho Chehab, VGER, Lad,
Prabhakar, Manjunath Hadli, Hans Verkuil
From: Lad, Prabhakar <prabhakar.lad@ti.com>
vpif_display relied on a 1-1 mapping of output and subdev. This is not
necessarily the case. Separate the two. So there is a list of subdevs
and a list of outputs. Each output refers to a subdev and has routing
information. An output does not have to have a subdev.
The initial output for each channel is set to the fist output.
Currently missing is support for associating multiple subdevs with
an output.
Signed-off-by: Lad, Prabhakar <prabhakar.lad@ti.com>
Signed-off-by: Manjunath Hadli <manjunath.hadli@ti.com>
Cc: Hans Verkuil <hans.verkuil@cisco.com>
Cc: Sekhar Nori <nsekhar@ti.com>
---
This patch is dependent on the patch series from Hans
(http://www.mail-archive.com/linux-media@vger.kernel.org/msg52270.html)
Changes for V2:
1: Changed v4l2_device_call_until_err() call to v4l2_subdev_call() for
s_routing, since this call is for specific subdev, pointed out by Hans.
arch/arm/mach-davinci/board-da850-evm.c | 29 +++++-
arch/arm/mach-davinci/board-dm646x-evm.c | 39 ++++++-
drivers/media/platform/davinci/vpif_display.c | 136 ++++++++++++++++++++-----
include/media/davinci/vpif_types.h | 20 +++-
4 files changed, 183 insertions(+), 41 deletions(-)
diff --git a/arch/arm/mach-davinci/board-da850-evm.c b/arch/arm/mach-davinci/board-da850-evm.c
index 3081ea4..23a7012 100644
--- a/arch/arm/mach-davinci/board-da850-evm.c
+++ b/arch/arm/mach-davinci/board-da850-evm.c
@@ -46,6 +46,7 @@
#include <mach/spi.h>
#include <media/tvp514x.h>
+#include <media/adv7343.h>
#define DA850_EVM_PHY_ID "davinci_mdio-0:00"
#define DA850_LCD_PWR_PIN GPIO_TO_PIN(2, 8)
@@ -1257,16 +1258,34 @@ static struct vpif_subdev_info da850_vpif_subdev[] = {
},
};
-static const char const *vpif_output[] = {
- "Composite",
- "S-Video",
+static const struct vpif_output da850_ch0_outputs[] = {
+ {
+ .output = {
+ .index = 0,
+ .name = "Composite",
+ .type = V4L2_OUTPUT_TYPE_ANALOG,
+ },
+ .subdev_name = "adv7343",
+ .output_route = ADV7343_COMPOSITE_ID,
+ },
+ {
+ .output = {
+ .index = 1,
+ .name = "S-Video",
+ .type = V4L2_OUTPUT_TYPE_ANALOG,
+ },
+ .subdev_name = "adv7343",
+ .output_route = ADV7343_SVIDEO_ID,
+ },
};
static struct vpif_display_config da850_vpif_display_config = {
.subdevinfo = da850_vpif_subdev,
.subdev_count = ARRAY_SIZE(da850_vpif_subdev),
- .output = vpif_output,
- .output_count = ARRAY_SIZE(vpif_output),
+ .chan_config[0] = {
+ .outputs = da850_ch0_outputs,
+ .output_count = ARRAY_SIZE(da850_ch0_outputs),
+ },
.card_name = "DA850/OMAP-L138 Video Display",
};
diff --git a/arch/arm/mach-davinci/board-dm646x-evm.c b/arch/arm/mach-davinci/board-dm646x-evm.c
index ad249c7..c206768 100644
--- a/arch/arm/mach-davinci/board-dm646x-evm.c
+++ b/arch/arm/mach-davinci/board-dm646x-evm.c
@@ -26,6 +26,7 @@
#include <linux/i2c/pcf857x.h>
#include <media/tvp514x.h>
+#include <media/adv7343.h>
#include <linux/mtd/mtd.h>
#include <linux/mtd/nand.h>
@@ -496,18 +497,44 @@ static struct vpif_subdev_info dm646x_vpif_subdev[] = {
},
};
-static const char *output[] = {
- "Composite",
- "Component",
- "S-Video",
+static const struct vpif_output dm6467_ch0_outputs[] = {
+ {
+ .output = {
+ .index = 0,
+ .name = "Composite",
+ .type = V4L2_OUTPUT_TYPE_ANALOG,
+ },
+ .subdev_name = "adv7343",
+ .output_route = ADV7343_COMPOSITE_ID,
+ },
+ {
+ .output = {
+ .index = 1,
+ .name = "Component",
+ .type = V4L2_OUTPUT_TYPE_ANALOG,
+ },
+ .subdev_name = "adv7343",
+ .output_route = ADV7343_COMPONENT_ID,
+ },
+ {
+ .output = {
+ .index = 2,
+ .name = "S-Video",
+ .type = V4L2_OUTPUT_TYPE_ANALOG,
+ },
+ .subdev_name = "adv7343",
+ .output_route = ADV7343_SVIDEO_ID,
+ },
};
static struct vpif_display_config dm646x_vpif_display_config = {
.set_clock = set_vpif_clock,
.subdevinfo = dm646x_vpif_subdev,
.subdev_count = ARRAY_SIZE(dm646x_vpif_subdev),
- .output = output,
- .output_count = ARRAY_SIZE(output),
+ .chan_config[0] = {
+ .outputs = dm6467_ch0_outputs,
+ .output_count = ARRAY_SIZE(dm6467_ch0_outputs),
+ },
.card_name = "DM646x EVM",
};
diff --git a/drivers/media/platform/davinci/vpif_display.c b/drivers/media/platform/davinci/vpif_display.c
index 8d1ce09..f68780f 100644
--- a/drivers/media/platform/davinci/vpif_display.c
+++ b/drivers/media/platform/davinci/vpif_display.c
@@ -308,7 +308,7 @@ static int vpif_start_streaming(struct vb2_queue *vq, unsigned int count)
channel2_intr_assert();
channel2_intr_enable(1);
enable_channel2(1);
- if (vpif_config_data->ch2_clip_en)
+ if (vpif_config_data->chan_config[VPIF_CHANNEL2_VIDEO].clip_en)
channel2_clipping_enable(1);
}
@@ -317,7 +317,7 @@ static int vpif_start_streaming(struct vb2_queue *vq, unsigned int count)
channel3_intr_assert();
channel3_intr_enable(1);
enable_channel3(1);
- if (vpif_config_data->ch3_clip_en)
+ if (vpif_config_data->chan_config[VPIF_CHANNEL3_VIDEO].clip_en)
channel3_clipping_enable(1);
}
@@ -1174,14 +1174,16 @@ 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)
+ if (vpif_config_data->
+ chan_config[VPIF_CHANNEL2_VIDEO].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)
+ if (vpif_config_data->
+ chan_config[VPIF_CHANNEL3_VIDEO].clip_en)
channel3_clipping_enable(0);
enable_channel3(0);
channel3_intr_enable(0);
@@ -1214,41 +1216,118 @@ static int vpif_enum_output(struct file *file, void *fh,
{
struct vpif_display_config *config = vpif_dev->platform_data;
+ struct vpif_display_chan_config *chan_cfg;
+ struct vpif_fh *vpif_handler = fh;
+ struct channel_obj *ch = vpif_handler->channel;
- if (output->index >= config->output_count) {
+ chan_cfg = &config->chan_config[ch->channel_id];
+ if (output->index >= chan_cfg->output_count) {
vpif_dbg(1, debug, "Invalid output index\n");
return -EINVAL;
}
- strcpy(output->name, config->output[output->index]);
- output->type = V4L2_OUTPUT_TYPE_ANALOG;
+ memcpy(output, &chan_cfg->outputs[output->index].output,
+ sizeof(*output));
output->std = VPIF_V4L2_STD;
return 0;
}
+/**
+ * vpif_output_to_subdev() - Maps output to sub device
+ * @vpif_cfg - global config ptr
+ * @chan_cfg - channel config ptr
+ * @index - Given output index from application
+ *
+ * lookup the sub device information for a given output index.
+ * we report all the output to application. output table also
+ * has sub device name for the each output
+ */
+static int
+vpif_output_to_subdev(struct vpif_display_config *vpif_cfg,
+ struct vpif_display_chan_config *chan_cfg, int index)
+{
+ struct vpif_subdev_info *subdev_info;
+ const char *subdev_name;
+ int i;
+
+ vpif_dbg(2, debug, "vpif_output_to_subdev\n");
+
+ if (chan_cfg->outputs == NULL)
+ return -1;
+
+ subdev_name = chan_cfg->outputs[index].subdev_name;
+ if (subdev_name == NULL)
+ return -1;
+
+ /* loop through the sub device list to get the sub device info */
+ for (i = 0; i < vpif_cfg->subdev_count; i++) {
+ subdev_info = &vpif_cfg->subdevinfo[i];
+ if (!strcmp(subdev_info->name, subdev_name))
+ return i;
+ }
+ return -1;
+}
+
+/**
+ * vpif_set_output() - Select an output
+ * @vpif_cfg - global config ptr
+ * @ch - channel
+ * @index - Given output index from application
+ *
+ * Select the given output.
+ */
+static int vpif_set_output(struct vpif_display_config *vpif_cfg,
+ struct channel_obj *ch, int index)
+{
+ struct vpif_display_chan_config *chan_cfg =
+ &vpif_cfg->chan_config[ch->channel_id];
+ struct vpif_subdev_info *subdev_info = NULL;
+ struct v4l2_subdev *sd = NULL;
+ u32 input = 0, output = 0;
+ int sd_index;
+ int ret;
+
+ sd_index = vpif_output_to_subdev(vpif_cfg, chan_cfg, index);
+ if (sd_index >= 0) {
+ sd = vpif_obj.sd[sd_index];
+ subdev_info = &vpif_cfg->subdevinfo[sd_index];
+ }
+
+ if (sd) {
+ input = chan_cfg->outputs[index].input_route;
+ output = chan_cfg->outputs[index].output_route;
+ ret = v4l2_subdev_call(sd, video, s_routing, input, output, 0);
+ if (ret < 0) {
+ vpif_err("Failed to set output\n");
+ return ret;
+ }
+
+ }
+ ch->output_idx = index;
+ ch->sd = sd;
+ return 0;
+}
+
static int vpif_s_output(struct file *file, void *priv, unsigned int i)
{
+ struct vpif_display_config *config = vpif_dev->platform_data;
+ struct vpif_display_chan_config *chan_cfg;
struct vpif_fh *fh = priv;
struct channel_obj *ch = fh->channel;
struct common_obj *common = &ch->common[VPIF_VIDEO_INDEX];
- int ret = 0;
+
+ chan_cfg = &config->chan_config[ch->channel_id];
+
+ if (i >= chan_cfg->output_count)
+ return -EINVAL;
if (common->started) {
vpif_err("Streaming in progress\n");
return -EBUSY;
}
- ret = v4l2_device_call_until_err(&vpif_obj.v4l2_dev, 1, video,
- s_routing, 0, i, 0);
-
- if (ret < 0)
- vpif_err("Failed to set output standard\n");
-
- ch->output_idx = i;
- if (vpif_obj.sd[i])
- ch->sd = vpif_obj.sd[i];
- return ret;
+ return vpif_set_output(config, ch, i);
}
static int vpif_g_output(struct file *file, void *priv, unsigned int *i)
@@ -1291,9 +1370,12 @@ vpif_enum_dv_timings(struct file *file, void *priv,
{
struct vpif_fh *fh = priv;
struct channel_obj *ch = fh->channel;
+ int ret;
- return v4l2_subdev_call(vpif_obj.sd[ch->output_idx],
- video, enum_dv_timings, timings);
+ ret = v4l2_subdev_call(ch->sd, video, enum_dv_timings, timings);
+ if (ret == -ENOIOCTLCMD && ret == -ENODEV)
+ return -EINVAL;
+ return ret;
}
/**
@@ -1320,12 +1402,9 @@ static int vpif_s_dv_timings(struct file *file, void *priv,
/* Configure subdevice timings, if any */
ret = v4l2_subdev_call(ch->sd, video, s_dv_timings, timings);
- if (ret == -ENOIOCTLCMD) {
- vpif_dbg(2, debug, "Custom DV timings not supported by "
- "subdevice\n");
- return -ENODATA;
- }
- if (ret < 0 && ret != -ENODEV) {
+ if (ret == -ENOIOCTLCMD || ret == -ENODEV)
+ ret = 0;
+ if (ret < 0) {
vpif_dbg(2, debug, "Error setting custom DV timings\n");
return ret;
}
@@ -1754,6 +1833,11 @@ static __init int vpif_probe(struct platform_device *pdev)
ch->video_dev->lock = &common->lock;
video_set_drvdata(ch->video_dev, ch);
+ /* select output 0 */
+ err = vpif_set_output(config, ch, 0);
+ if (err)
+ goto probe_out;
+
/* register video device */
vpif_dbg(1, debug, "channel=%x,channel->video_dev=%x\n",
(int)ch, (int)&ch->video_dev);
diff --git a/include/media/davinci/vpif_types.h b/include/media/davinci/vpif_types.h
index 65e8fe1..4b297eb 100644
--- a/include/media/davinci/vpif_types.h
+++ b/include/media/davinci/vpif_types.h
@@ -20,6 +20,7 @@
#include <linux/i2c.h>
#define VPIF_CAPTURE_MAX_CHANNELS 2
+#define VPIF_DISPLAY_MAX_CHANNELS 2
enum vpif_if_type {
VPIF_IF_BT656,
@@ -39,15 +40,26 @@ struct vpif_subdev_info {
struct i2c_board_info board_info;
};
+struct vpif_output {
+ struct v4l2_output output;
+ const char *subdev_name;
+ u32 input_route;
+ u32 output_route;
+};
+
+struct vpif_display_chan_config {
+ struct vpif_interface vpif_if;
+ const struct vpif_output *outputs;
+ int output_count;
+ bool clip_en;
+};
+
struct vpif_display_config {
int (*set_clock)(int, int);
struct vpif_subdev_info *subdevinfo;
int subdev_count;
- const char **output;
- int output_count;
+ struct vpif_display_chan_config chan_config[VPIF_DISPLAY_MAX_CHANNELS];
const char *card_name;
- bool ch2_clip_en;
- bool ch3_clip_en;
};
struct vpif_input {
--
1.7.4.1
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2] media: davinci: vpif: display: separate out subdev from output
2012-09-24 10:44 [PATCH v2] media: davinci: vpif: display: separate out subdev from output Prabhakar
@ 2012-09-24 10:59 ` Hans Verkuil
2012-09-24 11:50 ` Hans Verkuil
0 siblings, 1 reply; 9+ messages in thread
From: Hans Verkuil @ 2012-09-24 10:59 UTC (permalink / raw)
To: Prabhakar
Cc: LMML, DLOS, LAK, Sekhar Nori, Mauro Carvalho Chehab, VGER, Lad,
Prabhakar, Manjunath Hadli, Hans Verkuil
On Mon September 24 2012 12:44:11 Prabhakar wrote:
> From: Lad, Prabhakar <prabhakar.lad@ti.com>
>
> vpif_display relied on a 1-1 mapping of output and subdev. This is not
> necessarily the case. Separate the two. So there is a list of subdevs
> and a list of outputs. Each output refers to a subdev and has routing
> information. An output does not have to have a subdev.
>
> The initial output for each channel is set to the fist output.
>
> Currently missing is support for associating multiple subdevs with
> an output.
>
> Signed-off-by: Lad, Prabhakar <prabhakar.lad@ti.com>
> Signed-off-by: Manjunath Hadli <manjunath.hadli@ti.com>
> Cc: Hans Verkuil <hans.verkuil@cisco.com>
> Cc: Sekhar Nori <nsekhar@ti.com>
Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
Regards,
Hans
> ---
> This patch is dependent on the patch series from Hans
> (http://www.mail-archive.com/linux-media@vger.kernel.org/msg52270.html)
>
> Changes for V2:
> 1: Changed v4l2_device_call_until_err() call to v4l2_subdev_call() for
> s_routing, since this call is for specific subdev, pointed out by Hans.
>
> arch/arm/mach-davinci/board-da850-evm.c | 29 +++++-
> arch/arm/mach-davinci/board-dm646x-evm.c | 39 ++++++-
> drivers/media/platform/davinci/vpif_display.c | 136 ++++++++++++++++++++-----
> include/media/davinci/vpif_types.h | 20 +++-
> 4 files changed, 183 insertions(+), 41 deletions(-)
>
> diff --git a/arch/arm/mach-davinci/board-da850-evm.c b/arch/arm/mach-davinci/board-da850-evm.c
> index 3081ea4..23a7012 100644
> --- a/arch/arm/mach-davinci/board-da850-evm.c
> +++ b/arch/arm/mach-davinci/board-da850-evm.c
> @@ -46,6 +46,7 @@
> #include <mach/spi.h>
>
> #include <media/tvp514x.h>
> +#include <media/adv7343.h>
>
> #define DA850_EVM_PHY_ID "davinci_mdio-0:00"
> #define DA850_LCD_PWR_PIN GPIO_TO_PIN(2, 8)
> @@ -1257,16 +1258,34 @@ static struct vpif_subdev_info da850_vpif_subdev[] = {
> },
> };
>
> -static const char const *vpif_output[] = {
> - "Composite",
> - "S-Video",
> +static const struct vpif_output da850_ch0_outputs[] = {
> + {
> + .output = {
> + .index = 0,
> + .name = "Composite",
> + .type = V4L2_OUTPUT_TYPE_ANALOG,
> + },
> + .subdev_name = "adv7343",
> + .output_route = ADV7343_COMPOSITE_ID,
> + },
> + {
> + .output = {
> + .index = 1,
> + .name = "S-Video",
> + .type = V4L2_OUTPUT_TYPE_ANALOG,
> + },
> + .subdev_name = "adv7343",
> + .output_route = ADV7343_SVIDEO_ID,
> + },
> };
>
> static struct vpif_display_config da850_vpif_display_config = {
> .subdevinfo = da850_vpif_subdev,
> .subdev_count = ARRAY_SIZE(da850_vpif_subdev),
> - .output = vpif_output,
> - .output_count = ARRAY_SIZE(vpif_output),
> + .chan_config[0] = {
> + .outputs = da850_ch0_outputs,
> + .output_count = ARRAY_SIZE(da850_ch0_outputs),
> + },
> .card_name = "DA850/OMAP-L138 Video Display",
> };
>
> diff --git a/arch/arm/mach-davinci/board-dm646x-evm.c b/arch/arm/mach-davinci/board-dm646x-evm.c
> index ad249c7..c206768 100644
> --- a/arch/arm/mach-davinci/board-dm646x-evm.c
> +++ b/arch/arm/mach-davinci/board-dm646x-evm.c
> @@ -26,6 +26,7 @@
> #include <linux/i2c/pcf857x.h>
>
> #include <media/tvp514x.h>
> +#include <media/adv7343.h>
>
> #include <linux/mtd/mtd.h>
> #include <linux/mtd/nand.h>
> @@ -496,18 +497,44 @@ static struct vpif_subdev_info dm646x_vpif_subdev[] = {
> },
> };
>
> -static const char *output[] = {
> - "Composite",
> - "Component",
> - "S-Video",
> +static const struct vpif_output dm6467_ch0_outputs[] = {
> + {
> + .output = {
> + .index = 0,
> + .name = "Composite",
> + .type = V4L2_OUTPUT_TYPE_ANALOG,
> + },
> + .subdev_name = "adv7343",
> + .output_route = ADV7343_COMPOSITE_ID,
> + },
> + {
> + .output = {
> + .index = 1,
> + .name = "Component",
> + .type = V4L2_OUTPUT_TYPE_ANALOG,
> + },
> + .subdev_name = "adv7343",
> + .output_route = ADV7343_COMPONENT_ID,
> + },
> + {
> + .output = {
> + .index = 2,
> + .name = "S-Video",
> + .type = V4L2_OUTPUT_TYPE_ANALOG,
> + },
> + .subdev_name = "adv7343",
> + .output_route = ADV7343_SVIDEO_ID,
> + },
> };
>
> static struct vpif_display_config dm646x_vpif_display_config = {
> .set_clock = set_vpif_clock,
> .subdevinfo = dm646x_vpif_subdev,
> .subdev_count = ARRAY_SIZE(dm646x_vpif_subdev),
> - .output = output,
> - .output_count = ARRAY_SIZE(output),
> + .chan_config[0] = {
> + .outputs = dm6467_ch0_outputs,
> + .output_count = ARRAY_SIZE(dm6467_ch0_outputs),
> + },
> .card_name = "DM646x EVM",
> };
>
> diff --git a/drivers/media/platform/davinci/vpif_display.c b/drivers/media/platform/davinci/vpif_display.c
> index 8d1ce09..f68780f 100644
> --- a/drivers/media/platform/davinci/vpif_display.c
> +++ b/drivers/media/platform/davinci/vpif_display.c
> @@ -308,7 +308,7 @@ static int vpif_start_streaming(struct vb2_queue *vq, unsigned int count)
> channel2_intr_assert();
> channel2_intr_enable(1);
> enable_channel2(1);
> - if (vpif_config_data->ch2_clip_en)
> + if (vpif_config_data->chan_config[VPIF_CHANNEL2_VIDEO].clip_en)
> channel2_clipping_enable(1);
> }
>
> @@ -317,7 +317,7 @@ static int vpif_start_streaming(struct vb2_queue *vq, unsigned int count)
> channel3_intr_assert();
> channel3_intr_enable(1);
> enable_channel3(1);
> - if (vpif_config_data->ch3_clip_en)
> + if (vpif_config_data->chan_config[VPIF_CHANNEL3_VIDEO].clip_en)
> channel3_clipping_enable(1);
> }
>
> @@ -1174,14 +1174,16 @@ 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)
> + if (vpif_config_data->
> + chan_config[VPIF_CHANNEL2_VIDEO].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)
> + if (vpif_config_data->
> + chan_config[VPIF_CHANNEL3_VIDEO].clip_en)
> channel3_clipping_enable(0);
> enable_channel3(0);
> channel3_intr_enable(0);
> @@ -1214,41 +1216,118 @@ static int vpif_enum_output(struct file *file, void *fh,
> {
>
> struct vpif_display_config *config = vpif_dev->platform_data;
> + struct vpif_display_chan_config *chan_cfg;
> + struct vpif_fh *vpif_handler = fh;
> + struct channel_obj *ch = vpif_handler->channel;
>
> - if (output->index >= config->output_count) {
> + chan_cfg = &config->chan_config[ch->channel_id];
> + if (output->index >= chan_cfg->output_count) {
> vpif_dbg(1, debug, "Invalid output index\n");
> return -EINVAL;
> }
>
> - strcpy(output->name, config->output[output->index]);
> - output->type = V4L2_OUTPUT_TYPE_ANALOG;
> + memcpy(output, &chan_cfg->outputs[output->index].output,
> + sizeof(*output));
> output->std = VPIF_V4L2_STD;
>
> return 0;
> }
>
> +/**
> + * vpif_output_to_subdev() - Maps output to sub device
> + * @vpif_cfg - global config ptr
> + * @chan_cfg - channel config ptr
> + * @index - Given output index from application
> + *
> + * lookup the sub device information for a given output index.
> + * we report all the output to application. output table also
> + * has sub device name for the each output
> + */
> +static int
> +vpif_output_to_subdev(struct vpif_display_config *vpif_cfg,
> + struct vpif_display_chan_config *chan_cfg, int index)
> +{
> + struct vpif_subdev_info *subdev_info;
> + const char *subdev_name;
> + int i;
> +
> + vpif_dbg(2, debug, "vpif_output_to_subdev\n");
> +
> + if (chan_cfg->outputs == NULL)
> + return -1;
> +
> + subdev_name = chan_cfg->outputs[index].subdev_name;
> + if (subdev_name == NULL)
> + return -1;
> +
> + /* loop through the sub device list to get the sub device info */
> + for (i = 0; i < vpif_cfg->subdev_count; i++) {
> + subdev_info = &vpif_cfg->subdevinfo[i];
> + if (!strcmp(subdev_info->name, subdev_name))
> + return i;
> + }
> + return -1;
> +}
> +
> +/**
> + * vpif_set_output() - Select an output
> + * @vpif_cfg - global config ptr
> + * @ch - channel
> + * @index - Given output index from application
> + *
> + * Select the given output.
> + */
> +static int vpif_set_output(struct vpif_display_config *vpif_cfg,
> + struct channel_obj *ch, int index)
> +{
> + struct vpif_display_chan_config *chan_cfg =
> + &vpif_cfg->chan_config[ch->channel_id];
> + struct vpif_subdev_info *subdev_info = NULL;
> + struct v4l2_subdev *sd = NULL;
> + u32 input = 0, output = 0;
> + int sd_index;
> + int ret;
> +
> + sd_index = vpif_output_to_subdev(vpif_cfg, chan_cfg, index);
> + if (sd_index >= 0) {
> + sd = vpif_obj.sd[sd_index];
> + subdev_info = &vpif_cfg->subdevinfo[sd_index];
> + }
> +
> + if (sd) {
> + input = chan_cfg->outputs[index].input_route;
> + output = chan_cfg->outputs[index].output_route;
> + ret = v4l2_subdev_call(sd, video, s_routing, input, output, 0);
> + if (ret < 0) {
> + vpif_err("Failed to set output\n");
> + return ret;
> + }
> +
> + }
> + ch->output_idx = index;
> + ch->sd = sd;
> + return 0;
> +}
> +
> static int vpif_s_output(struct file *file, void *priv, unsigned int i)
> {
> + struct vpif_display_config *config = vpif_dev->platform_data;
> + struct vpif_display_chan_config *chan_cfg;
> struct vpif_fh *fh = priv;
> struct channel_obj *ch = fh->channel;
> struct common_obj *common = &ch->common[VPIF_VIDEO_INDEX];
> - int ret = 0;
> +
> + chan_cfg = &config->chan_config[ch->channel_id];
> +
> + if (i >= chan_cfg->output_count)
> + return -EINVAL;
>
> if (common->started) {
> vpif_err("Streaming in progress\n");
> return -EBUSY;
> }
>
> - ret = v4l2_device_call_until_err(&vpif_obj.v4l2_dev, 1, video,
> - s_routing, 0, i, 0);
> -
> - if (ret < 0)
> - vpif_err("Failed to set output standard\n");
> -
> - ch->output_idx = i;
> - if (vpif_obj.sd[i])
> - ch->sd = vpif_obj.sd[i];
> - return ret;
> + return vpif_set_output(config, ch, i);
> }
>
> static int vpif_g_output(struct file *file, void *priv, unsigned int *i)
> @@ -1291,9 +1370,12 @@ vpif_enum_dv_timings(struct file *file, void *priv,
> {
> struct vpif_fh *fh = priv;
> struct channel_obj *ch = fh->channel;
> + int ret;
>
> - return v4l2_subdev_call(vpif_obj.sd[ch->output_idx],
> - video, enum_dv_timings, timings);
> + ret = v4l2_subdev_call(ch->sd, video, enum_dv_timings, timings);
> + if (ret == -ENOIOCTLCMD && ret == -ENODEV)
> + return -EINVAL;
> + return ret;
> }
>
> /**
> @@ -1320,12 +1402,9 @@ static int vpif_s_dv_timings(struct file *file, void *priv,
>
> /* Configure subdevice timings, if any */
> ret = v4l2_subdev_call(ch->sd, video, s_dv_timings, timings);
> - if (ret == -ENOIOCTLCMD) {
> - vpif_dbg(2, debug, "Custom DV timings not supported by "
> - "subdevice\n");
> - return -ENODATA;
> - }
> - if (ret < 0 && ret != -ENODEV) {
> + if (ret == -ENOIOCTLCMD || ret == -ENODEV)
> + ret = 0;
> + if (ret < 0) {
> vpif_dbg(2, debug, "Error setting custom DV timings\n");
> return ret;
> }
> @@ -1754,6 +1833,11 @@ static __init int vpif_probe(struct platform_device *pdev)
> ch->video_dev->lock = &common->lock;
> video_set_drvdata(ch->video_dev, ch);
>
> + /* select output 0 */
> + err = vpif_set_output(config, ch, 0);
> + if (err)
> + goto probe_out;
> +
> /* register video device */
> vpif_dbg(1, debug, "channel=%x,channel->video_dev=%x\n",
> (int)ch, (int)&ch->video_dev);
> diff --git a/include/media/davinci/vpif_types.h b/include/media/davinci/vpif_types.h
> index 65e8fe1..4b297eb 100644
> --- a/include/media/davinci/vpif_types.h
> +++ b/include/media/davinci/vpif_types.h
> @@ -20,6 +20,7 @@
> #include <linux/i2c.h>
>
> #define VPIF_CAPTURE_MAX_CHANNELS 2
> +#define VPIF_DISPLAY_MAX_CHANNELS 2
>
> enum vpif_if_type {
> VPIF_IF_BT656,
> @@ -39,15 +40,26 @@ struct vpif_subdev_info {
> struct i2c_board_info board_info;
> };
>
> +struct vpif_output {
> + struct v4l2_output output;
> + const char *subdev_name;
> + u32 input_route;
> + u32 output_route;
> +};
> +
> +struct vpif_display_chan_config {
> + struct vpif_interface vpif_if;
> + const struct vpif_output *outputs;
> + int output_count;
> + bool clip_en;
> +};
> +
> struct vpif_display_config {
> int (*set_clock)(int, int);
> struct vpif_subdev_info *subdevinfo;
> int subdev_count;
> - const char **output;
> - int output_count;
> + struct vpif_display_chan_config chan_config[VPIF_DISPLAY_MAX_CHANNELS];
> const char *card_name;
> - bool ch2_clip_en;
> - bool ch3_clip_en;
> };
>
> struct vpif_input {
>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2] media: davinci: vpif: display: separate out subdev from output
2012-09-24 10:59 ` Hans Verkuil
@ 2012-09-24 11:50 ` Hans Verkuil
2012-09-24 11:59 ` Hans Verkuil
2012-09-24 13:21 ` Prabhakar Lad
0 siblings, 2 replies; 9+ messages in thread
From: Hans Verkuil @ 2012-09-24 11:50 UTC (permalink / raw)
To: Prabhakar
Cc: LMML, DLOS, LAK, Sekhar Nori, Mauro Carvalho Chehab, VGER, Lad,
Prabhakar, Manjunath Hadli, Hans Verkuil
On Mon September 24 2012 12:59:11 Hans Verkuil wrote:
> On Mon September 24 2012 12:44:11 Prabhakar wrote:
> > From: Lad, Prabhakar <prabhakar.lad@ti.com>
> >
> > vpif_display relied on a 1-1 mapping of output and subdev. This is not
> > necessarily the case. Separate the two. So there is a list of subdevs
> > and a list of outputs. Each output refers to a subdev and has routing
> > information. An output does not have to have a subdev.
> >
> > The initial output for each channel is set to the fist output.
> >
> > Currently missing is support for associating multiple subdevs with
> > an output.
> >
> > Signed-off-by: Lad, Prabhakar <prabhakar.lad@ti.com>
> > Signed-off-by: Manjunath Hadli <manjunath.hadli@ti.com>
> > Cc: Hans Verkuil <hans.verkuil@cisco.com>
> > Cc: Sekhar Nori <nsekhar@ti.com>
>
> Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
I'm retracting this Ack. I did see something that wasn't right but I thought
it was harmless, but after thinking some more I believe it should be fixed.
Luckily, it's easy to fix. See below. Since we need a new version anyway I
also added a comment to a few minor issues that can be fixed at the same time.
>
> Regards,
>
> Hans
>
> > ---
> > This patch is dependent on the patch series from Hans
> > (http://www.mail-archive.com/linux-media@vger.kernel.org/msg52270.html)
> >
> > Changes for V2:
> > 1: Changed v4l2_device_call_until_err() call to v4l2_subdev_call() for
> > s_routing, since this call is for specific subdev, pointed out by Hans.
> >
> > arch/arm/mach-davinci/board-da850-evm.c | 29 +++++-
> > arch/arm/mach-davinci/board-dm646x-evm.c | 39 ++++++-
> > drivers/media/platform/davinci/vpif_display.c | 136 ++++++++++++++++++++-----
> > include/media/davinci/vpif_types.h | 20 +++-
> > 4 files changed, 183 insertions(+), 41 deletions(-)
> >
> > diff --git a/arch/arm/mach-davinci/board-da850-evm.c b/arch/arm/mach-davinci/board-da850-evm.c
> > index 3081ea4..23a7012 100644
> > --- a/arch/arm/mach-davinci/board-da850-evm.c
> > +++ b/arch/arm/mach-davinci/board-da850-evm.c
> > @@ -46,6 +46,7 @@
> > #include <mach/spi.h>
> >
> > #include <media/tvp514x.h>
> > +#include <media/adv7343.h>
> >
> > #define DA850_EVM_PHY_ID "davinci_mdio-0:00"
> > #define DA850_LCD_PWR_PIN GPIO_TO_PIN(2, 8)
> > @@ -1257,16 +1258,34 @@ static struct vpif_subdev_info da850_vpif_subdev[] = {
> > },
> > };
> >
> > -static const char const *vpif_output[] = {
> > - "Composite",
> > - "S-Video",
> > +static const struct vpif_output da850_ch0_outputs[] = {
> > + {
> > + .output = {
> > + .index = 0,
> > + .name = "Composite",
> > + .type = V4L2_OUTPUT_TYPE_ANALOG,
> > + },
> > + .subdev_name = "adv7343",
> > + .output_route = ADV7343_COMPOSITE_ID,
> > + },
> > + {
> > + .output = {
> > + .index = 1,
> > + .name = "S-Video",
> > + .type = V4L2_OUTPUT_TYPE_ANALOG,
> > + },
> > + .subdev_name = "adv7343",
> > + .output_route = ADV7343_SVIDEO_ID,
> > + },
> > };
> >
> > static struct vpif_display_config da850_vpif_display_config = {
> > .subdevinfo = da850_vpif_subdev,
> > .subdev_count = ARRAY_SIZE(da850_vpif_subdev),
> > - .output = vpif_output,
> > - .output_count = ARRAY_SIZE(vpif_output),
> > + .chan_config[0] = {
> > + .outputs = da850_ch0_outputs,
> > + .output_count = ARRAY_SIZE(da850_ch0_outputs),
> > + },
> > .card_name = "DA850/OMAP-L138 Video Display",
> > };
> >
> > diff --git a/arch/arm/mach-davinci/board-dm646x-evm.c b/arch/arm/mach-davinci/board-dm646x-evm.c
> > index ad249c7..c206768 100644
> > --- a/arch/arm/mach-davinci/board-dm646x-evm.c
> > +++ b/arch/arm/mach-davinci/board-dm646x-evm.c
> > @@ -26,6 +26,7 @@
> > #include <linux/i2c/pcf857x.h>
> >
> > #include <media/tvp514x.h>
> > +#include <media/adv7343.h>
> >
> > #include <linux/mtd/mtd.h>
> > #include <linux/mtd/nand.h>
> > @@ -496,18 +497,44 @@ static struct vpif_subdev_info dm646x_vpif_subdev[] = {
> > },
> > };
> >
> > -static const char *output[] = {
> > - "Composite",
> > - "Component",
> > - "S-Video",
> > +static const struct vpif_output dm6467_ch0_outputs[] = {
> > + {
> > + .output = {
> > + .index = 0,
> > + .name = "Composite",
> > + .type = V4L2_OUTPUT_TYPE_ANALOG,
> > + },
> > + .subdev_name = "adv7343",
> > + .output_route = ADV7343_COMPOSITE_ID,
> > + },
> > + {
> > + .output = {
> > + .index = 1,
> > + .name = "Component",
> > + .type = V4L2_OUTPUT_TYPE_ANALOG,
> > + },
> > + .subdev_name = "adv7343",
> > + .output_route = ADV7343_COMPONENT_ID,
> > + },
> > + {
> > + .output = {
> > + .index = 2,
> > + .name = "S-Video",
> > + .type = V4L2_OUTPUT_TYPE_ANALOG,
> > + },
> > + .subdev_name = "adv7343",
> > + .output_route = ADV7343_SVIDEO_ID,
> > + },
> > };
> >
> > static struct vpif_display_config dm646x_vpif_display_config = {
> > .set_clock = set_vpif_clock,
> > .subdevinfo = dm646x_vpif_subdev,
> > .subdev_count = ARRAY_SIZE(dm646x_vpif_subdev),
> > - .output = output,
> > - .output_count = ARRAY_SIZE(output),
> > + .chan_config[0] = {
> > + .outputs = dm6467_ch0_outputs,
> > + .output_count = ARRAY_SIZE(dm6467_ch0_outputs),
> > + },
> > .card_name = "DM646x EVM",
> > };
> >
> > diff --git a/drivers/media/platform/davinci/vpif_display.c b/drivers/media/platform/davinci/vpif_display.c
> > index 8d1ce09..f68780f 100644
> > --- a/drivers/media/platform/davinci/vpif_display.c
> > +++ b/drivers/media/platform/davinci/vpif_display.c
> > @@ -308,7 +308,7 @@ static int vpif_start_streaming(struct vb2_queue *vq, unsigned int count)
> > channel2_intr_assert();
> > channel2_intr_enable(1);
> > enable_channel2(1);
> > - if (vpif_config_data->ch2_clip_en)
> > + if (vpif_config_data->chan_config[VPIF_CHANNEL2_VIDEO].clip_en)
> > channel2_clipping_enable(1);
> > }
> >
> > @@ -317,7 +317,7 @@ static int vpif_start_streaming(struct vb2_queue *vq, unsigned int count)
> > channel3_intr_assert();
> > channel3_intr_enable(1);
> > enable_channel3(1);
> > - if (vpif_config_data->ch3_clip_en)
> > + if (vpif_config_data->chan_config[VPIF_CHANNEL3_VIDEO].clip_en)
> > channel3_clipping_enable(1);
> > }
> >
> > @@ -1174,14 +1174,16 @@ 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)
> > + if (vpif_config_data->
> > + chan_config[VPIF_CHANNEL2_VIDEO].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)
> > + if (vpif_config_data->
> > + chan_config[VPIF_CHANNEL3_VIDEO].clip_en)
> > channel3_clipping_enable(0);
> > enable_channel3(0);
> > channel3_intr_enable(0);
> > @@ -1214,41 +1216,118 @@ static int vpif_enum_output(struct file *file, void *fh,
> > {
> >
> > struct vpif_display_config *config = vpif_dev->platform_data;
> > + struct vpif_display_chan_config *chan_cfg;
> > + struct vpif_fh *vpif_handler = fh;
> > + struct channel_obj *ch = vpif_handler->channel;
> >
> > - if (output->index >= config->output_count) {
> > + chan_cfg = &config->chan_config[ch->channel_id];
> > + if (output->index >= chan_cfg->output_count) {
> > vpif_dbg(1, debug, "Invalid output index\n");
> > return -EINVAL;
> > }
> >
> > - strcpy(output->name, config->output[output->index]);
> > - output->type = V4L2_OUTPUT_TYPE_ANALOG;
> > + memcpy(output, &chan_cfg->outputs[output->index].output,
> > + sizeof(*output));
*output = chan_cfg->outputs[output->index].output;
> > output->std = VPIF_V4L2_STD;
This should be part of chan_cfg->outputs[output->index].output and not be
overridden. If the output is a HDTV output, then std should be 0, otherwise
it should be whatever the std is that is supported by the output.
Really no different to what we do in vpif_capture.
> >
> > return 0;
> > }
> >
> > +/**
> > + * vpif_output_to_subdev() - Maps output to sub device
> > + * @vpif_cfg - global config ptr
> > + * @chan_cfg - channel config ptr
> > + * @index - Given output index from application
> > + *
> > + * lookup the sub device information for a given output index.
> > + * we report all the output to application. output table also
> > + * has sub device name for the each output
> > + */
> > +static int
> > +vpif_output_to_subdev(struct vpif_display_config *vpif_cfg,
> > + struct vpif_display_chan_config *chan_cfg, int index)
> > +{
> > + struct vpif_subdev_info *subdev_info;
> > + const char *subdev_name;
> > + int i;
> > +
> > + vpif_dbg(2, debug, "vpif_output_to_subdev\n");
> > +
> > + if (chan_cfg->outputs == NULL)
> > + return -1;
> > +
> > + subdev_name = chan_cfg->outputs[index].subdev_name;
> > + if (subdev_name == NULL)
> > + return -1;
> > +
> > + /* loop through the sub device list to get the sub device info */
> > + for (i = 0; i < vpif_cfg->subdev_count; i++) {
> > + subdev_info = &vpif_cfg->subdevinfo[i];
> > + if (!strcmp(subdev_info->name, subdev_name))
> > + return i;
> > + }
> > + return -1;
> > +}
> > +
> > +/**
> > + * vpif_set_output() - Select an output
> > + * @vpif_cfg - global config ptr
> > + * @ch - channel
> > + * @index - Given output index from application
> > + *
> > + * Select the given output.
> > + */
> > +static int vpif_set_output(struct vpif_display_config *vpif_cfg,
> > + struct channel_obj *ch, int index)
> > +{
> > + struct vpif_display_chan_config *chan_cfg =
> > + &vpif_cfg->chan_config[ch->channel_id];
> > + struct vpif_subdev_info *subdev_info = NULL;
> > + struct v4l2_subdev *sd = NULL;
> > + u32 input = 0, output = 0;
> > + int sd_index;
> > + int ret;
> > +
> > + sd_index = vpif_output_to_subdev(vpif_cfg, chan_cfg, index);
> > + if (sd_index >= 0) {
> > + sd = vpif_obj.sd[sd_index];
> > + subdev_info = &vpif_cfg->subdevinfo[sd_index];
> > + }
> > +
> > + if (sd) {
> > + input = chan_cfg->outputs[index].input_route;
> > + output = chan_cfg->outputs[index].output_route;
> > + ret = v4l2_subdev_call(sd, video, s_routing, input, output, 0);
> > + if (ret < 0) {
Replace with:
if (ret < 0 && ret != -ENOIOCTLCMD) {
It's OK if the subdev doesn't support this s_routing operation.
> > + vpif_err("Failed to set output\n");
> > + return ret;
> > + }
> > +
> > + }
> > + ch->output_idx = index;
> > + ch->sd = sd;
Just like in the capture case you should update tvnorms:
ch->video_dev->tvnorms = chan_cfg->inputs[index].output.std;
Make sure tvnorms is no longer set in vpif_video_template.
Ditto for current_norm. Since vpif_display.c supports g_std the current_norm
field shouldn't be used anymore.
The capture case also updates vpifparams.iface:
ch->vpifparams.iface = chan_cfg->vpif_if;
Isn't that needed for output as well?
> > + return 0;
> > +}
> > +
> > static int vpif_s_output(struct file *file, void *priv, unsigned int i)
> > {
> > + struct vpif_display_config *config = vpif_dev->platform_data;
> > + struct vpif_display_chan_config *chan_cfg;
> > struct vpif_fh *fh = priv;
> > struct channel_obj *ch = fh->channel;
> > struct common_obj *common = &ch->common[VPIF_VIDEO_INDEX];
> > - int ret = 0;
> > +
> > + chan_cfg = &config->chan_config[ch->channel_id];
> > +
> > + if (i >= chan_cfg->output_count)
> > + return -EINVAL;
> >
> > if (common->started) {
> > vpif_err("Streaming in progress\n");
> > return -EBUSY;
> > }
> >
> > - ret = v4l2_device_call_until_err(&vpif_obj.v4l2_dev, 1, video,
> > - s_routing, 0, i, 0);
> > -
> > - if (ret < 0)
> > - vpif_err("Failed to set output standard\n");
> > -
> > - ch->output_idx = i;
> > - if (vpif_obj.sd[i])
> > - ch->sd = vpif_obj.sd[i];
> > - return ret;
> > + return vpif_set_output(config, ch, i);
> > }
> >
> > static int vpif_g_output(struct file *file, void *priv, unsigned int *i)
> > @@ -1291,9 +1370,12 @@ vpif_enum_dv_timings(struct file *file, void *priv,
> > {
> > struct vpif_fh *fh = priv;
> > struct channel_obj *ch = fh->channel;
> > + int ret;
> >
> > - return v4l2_subdev_call(vpif_obj.sd[ch->output_idx],
> > - video, enum_dv_timings, timings);
> > + ret = v4l2_subdev_call(ch->sd, video, enum_dv_timings, timings);
> > + if (ret == -ENOIOCTLCMD && ret == -ENODEV)
> > + return -EINVAL;
> > + return ret;
> > }
> >
> > /**
> > @@ -1320,12 +1402,9 @@ static int vpif_s_dv_timings(struct file *file, void *priv,
> >
> > /* Configure subdevice timings, if any */
> > ret = v4l2_subdev_call(ch->sd, video, s_dv_timings, timings);
> > - if (ret == -ENOIOCTLCMD) {
> > - vpif_dbg(2, debug, "Custom DV timings not supported by "
> > - "subdevice\n");
> > - return -ENODATA;
> > - }
> > - if (ret < 0 && ret != -ENODEV) {
> > + if (ret == -ENOIOCTLCMD || ret == -ENODEV)
> > + ret = 0;
> > + if (ret < 0) {
> > vpif_dbg(2, debug, "Error setting custom DV timings\n");
> > return ret;
> > }
> > @@ -1754,6 +1833,11 @@ static __init int vpif_probe(struct platform_device *pdev)
> > ch->video_dev->lock = &common->lock;
> > video_set_drvdata(ch->video_dev, ch);
> >
> > + /* select output 0 */
> > + err = vpif_set_output(config, ch, 0);
> > + if (err)
> > + goto probe_out;
> > +
> > /* register video device */
> > vpif_dbg(1, debug, "channel=%x,channel->video_dev=%x\n",
> > (int)ch, (int)&ch->video_dev);
Regards,
Hans
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2] media: davinci: vpif: display: separate out subdev from output
2012-09-24 11:50 ` Hans Verkuil
@ 2012-09-24 11:59 ` Hans Verkuil
2012-09-24 13:21 ` Prabhakar Lad
1 sibling, 0 replies; 9+ messages in thread
From: Hans Verkuil @ 2012-09-24 11:59 UTC (permalink / raw)
To: Prabhakar
Cc: LMML, DLOS, LAK, Sekhar Nori, Mauro Carvalho Chehab, VGER, Lad,
Prabhakar, Manjunath Hadli, Hans Verkuil
On Mon September 24 2012 13:50:00 Hans Verkuil wrote:
> On Mon September 24 2012 12:59:11 Hans Verkuil wrote:
> > On Mon September 24 2012 12:44:11 Prabhakar wrote:
> > > From: Lad, Prabhakar <prabhakar.lad@ti.com>
> > >
> > > vpif_display relied on a 1-1 mapping of output and subdev. This is not
> > > necessarily the case. Separate the two. So there is a list of subdevs
> > > and a list of outputs. Each output refers to a subdev and has routing
> > > information. An output does not have to have a subdev.
> > >
> > > The initial output for each channel is set to the fist output.
> > >
> > > Currently missing is support for associating multiple subdevs with
> > > an output.
> > >
> > > Signed-off-by: Lad, Prabhakar <prabhakar.lad@ti.com>
> > > Signed-off-by: Manjunath Hadli <manjunath.hadli@ti.com>
> > > Cc: Hans Verkuil <hans.verkuil@cisco.com>
> > > Cc: Sekhar Nori <nsekhar@ti.com>
> >
> > Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
>
> I'm retracting this Ack. I did see something that wasn't right but I thought
> it was harmless, but after thinking some more I believe it should be fixed.
> Luckily, it's easy to fix. See below. Since we need a new version anyway I
> also added a comment to a few minor issues that can be fixed at the same time.
>
> >
> > Regards,
> >
> > Hans
> >
> > > ---
> > > This patch is dependent on the patch series from Hans
> > > (http://www.mail-archive.com/linux-media@vger.kernel.org/msg52270.html)
> > >
> > > Changes for V2:
> > > 1: Changed v4l2_device_call_until_err() call to v4l2_subdev_call() for
> > > s_routing, since this call is for specific subdev, pointed out by Hans.
> > >
> > > arch/arm/mach-davinci/board-da850-evm.c | 29 +++++-
> > > arch/arm/mach-davinci/board-dm646x-evm.c | 39 ++++++-
> > > drivers/media/platform/davinci/vpif_display.c | 136 ++++++++++++++++++++-----
> > > include/media/davinci/vpif_types.h | 20 +++-
> > > 4 files changed, 183 insertions(+), 41 deletions(-)
> > >
> > > diff --git a/arch/arm/mach-davinci/board-da850-evm.c b/arch/arm/mach-davinci/board-da850-evm.c
> > > index 3081ea4..23a7012 100644
> > > --- a/arch/arm/mach-davinci/board-da850-evm.c
> > > +++ b/arch/arm/mach-davinci/board-da850-evm.c
> > > @@ -46,6 +46,7 @@
> > > #include <mach/spi.h>
> > >
> > > #include <media/tvp514x.h>
> > > +#include <media/adv7343.h>
> > >
> > > #define DA850_EVM_PHY_ID "davinci_mdio-0:00"
> > > #define DA850_LCD_PWR_PIN GPIO_TO_PIN(2, 8)
> > > @@ -1257,16 +1258,34 @@ static struct vpif_subdev_info da850_vpif_subdev[] = {
> > > },
> > > };
> > >
> > > -static const char const *vpif_output[] = {
> > > - "Composite",
> > > - "S-Video",
> > > +static const struct vpif_output da850_ch0_outputs[] = {
> > > + {
> > > + .output = {
> > > + .index = 0,
> > > + .name = "Composite",
> > > + .type = V4L2_OUTPUT_TYPE_ANALOG,
> > > + },
> > > + .subdev_name = "adv7343",
> > > + .output_route = ADV7343_COMPOSITE_ID,
> > > + },
> > > + {
> > > + .output = {
> > > + .index = 1,
> > > + .name = "S-Video",
> > > + .type = V4L2_OUTPUT_TYPE_ANALOG,
> > > + },
> > > + .subdev_name = "adv7343",
> > > + .output_route = ADV7343_SVIDEO_ID,
> > > + },
> > > };
> > >
> > > static struct vpif_display_config da850_vpif_display_config = {
> > > .subdevinfo = da850_vpif_subdev,
> > > .subdev_count = ARRAY_SIZE(da850_vpif_subdev),
> > > - .output = vpif_output,
> > > - .output_count = ARRAY_SIZE(vpif_output),
> > > + .chan_config[0] = {
> > > + .outputs = da850_ch0_outputs,
> > > + .output_count = ARRAY_SIZE(da850_ch0_outputs),
> > > + },
> > > .card_name = "DA850/OMAP-L138 Video Display",
> > > };
> > >
> > > diff --git a/arch/arm/mach-davinci/board-dm646x-evm.c b/arch/arm/mach-davinci/board-dm646x-evm.c
> > > index ad249c7..c206768 100644
> > > --- a/arch/arm/mach-davinci/board-dm646x-evm.c
> > > +++ b/arch/arm/mach-davinci/board-dm646x-evm.c
> > > @@ -26,6 +26,7 @@
> > > #include <linux/i2c/pcf857x.h>
> > >
> > > #include <media/tvp514x.h>
> > > +#include <media/adv7343.h>
> > >
> > > #include <linux/mtd/mtd.h>
> > > #include <linux/mtd/nand.h>
> > > @@ -496,18 +497,44 @@ static struct vpif_subdev_info dm646x_vpif_subdev[] = {
> > > },
> > > };
> > >
> > > -static const char *output[] = {
> > > - "Composite",
> > > - "Component",
> > > - "S-Video",
> > > +static const struct vpif_output dm6467_ch0_outputs[] = {
> > > + {
> > > + .output = {
> > > + .index = 0,
> > > + .name = "Composite",
> > > + .type = V4L2_OUTPUT_TYPE_ANALOG,
> > > + },
> > > + .subdev_name = "adv7343",
> > > + .output_route = ADV7343_COMPOSITE_ID,
> > > + },
> > > + {
> > > + .output = {
> > > + .index = 1,
> > > + .name = "Component",
> > > + .type = V4L2_OUTPUT_TYPE_ANALOG,
> > > + },
> > > + .subdev_name = "adv7343",
> > > + .output_route = ADV7343_COMPONENT_ID,
> > > + },
> > > + {
> > > + .output = {
> > > + .index = 2,
> > > + .name = "S-Video",
> > > + .type = V4L2_OUTPUT_TYPE_ANALOG,
> > > + },
> > > + .subdev_name = "adv7343",
> > > + .output_route = ADV7343_SVIDEO_ID,
> > > + },
> > > };
Oops, and another thing: for both struct v4l2_input and struct v4l2_output
you should also fill in the capabilities field in the board files.
This should be either V4L2_IN_CAP_STD (SDTV) or V4L2_IN_CAP_CUSTOM_TIMINGS
(HDTV) for input and the V4L2_OUT_ variants for v4l2_output.
Regards,
Hans
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2] media: davinci: vpif: display: separate out subdev from output
2012-09-24 11:50 ` Hans Verkuil
2012-09-24 11:59 ` Hans Verkuil
@ 2012-09-24 13:21 ` Prabhakar Lad
2012-09-24 13:32 ` Hans Verkuil
1 sibling, 1 reply; 9+ messages in thread
From: Prabhakar Lad @ 2012-09-24 13:21 UTC (permalink / raw)
To: Hans Verkuil
Cc: LMML, DLOS, LAK, Sekhar Nori, Mauro Carvalho Chehab, VGER, Lad,
Prabhakar, Manjunath Hadli, Hans Verkuil
Hi Hans,
On Mon, Sep 24, 2012 at 5:20 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> On Mon September 24 2012 12:59:11 Hans Verkuil wrote:
>> On Mon September 24 2012 12:44:11 Prabhakar wrote:
>> > From: Lad, Prabhakar <prabhakar.lad@ti.com>
>> >
>> > vpif_display relied on a 1-1 mapping of output and subdev. This is not
>> > necessarily the case. Separate the two. So there is a list of subdevs
>> > and a list of outputs. Each output refers to a subdev and has routing
>> > information. An output does not have to have a subdev.
>> >
>> > The initial output for each channel is set to the fist output.
>> >
>> > Currently missing is support for associating multiple subdevs with
>> > an output.
>> >
>> > Signed-off-by: Lad, Prabhakar <prabhakar.lad@ti.com>
>> > Signed-off-by: Manjunath Hadli <manjunath.hadli@ti.com>
>> > Cc: Hans Verkuil <hans.verkuil@cisco.com>
>> > Cc: Sekhar Nori <nsekhar@ti.com>
>>
>> Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
>
> I'm retracting this Ack. I did see something that wasn't right but I thought
> it was harmless, but after thinking some more I believe it should be fixed.
> Luckily, it's easy to fix. See below. Since we need a new version anyway I
> also added a comment to a few minor issues that can be fixed at the same time.
>
>>
>> Regards,
>>
>> Hans
>>
>> > ---
>> > This patch is dependent on the patch series from Hans
>> > (http://www.mail-archive.com/linux-media@vger.kernel.org/msg52270.html)
>> >
>> > Changes for V2:
>> > 1: Changed v4l2_device_call_until_err() call to v4l2_subdev_call() for
>> > s_routing, since this call is for specific subdev, pointed out by Hans.
>> >
>> > arch/arm/mach-davinci/board-da850-evm.c | 29 +++++-
>> > arch/arm/mach-davinci/board-dm646x-evm.c | 39 ++++++-
>> > drivers/media/platform/davinci/vpif_display.c | 136 ++++++++++++++++++++-----
>> > include/media/davinci/vpif_types.h | 20 +++-
>> > 4 files changed, 183 insertions(+), 41 deletions(-)
>> >
>> > diff --git a/arch/arm/mach-davinci/board-da850-evm.c b/arch/arm/mach-davinci/board-da850-evm.c
>> > index 3081ea4..23a7012 100644
>> > --- a/arch/arm/mach-davinci/board-da850-evm.c
>> > +++ b/arch/arm/mach-davinci/board-da850-evm.c
>> > @@ -46,6 +46,7 @@
>> > #include <mach/spi.h>
>> >
>> > #include <media/tvp514x.h>
>> > +#include <media/adv7343.h>
>> >
>> > #define DA850_EVM_PHY_ID "davinci_mdio-0:00"
>> > #define DA850_LCD_PWR_PIN GPIO_TO_PIN(2, 8)
>> > @@ -1257,16 +1258,34 @@ static struct vpif_subdev_info da850_vpif_subdev[] = {
>> > },
>> > };
>> >
>> > -static const char const *vpif_output[] = {
>> > - "Composite",
>> > - "S-Video",
>> > +static const struct vpif_output da850_ch0_outputs[] = {
>> > + {
>> > + .output = {
>> > + .index = 0,
>> > + .name = "Composite",
>> > + .type = V4L2_OUTPUT_TYPE_ANALOG,
>> > + },
>> > + .subdev_name = "adv7343",
>> > + .output_route = ADV7343_COMPOSITE_ID,
>> > + },
>> > + {
>> > + .output = {
>> > + .index = 1,
>> > + .name = "S-Video",
>> > + .type = V4L2_OUTPUT_TYPE_ANALOG,
>> > + },
>> > + .subdev_name = "adv7343",
>> > + .output_route = ADV7343_SVIDEO_ID,
>> > + },
>> > };
>> >
>> > static struct vpif_display_config da850_vpif_display_config = {
>> > .subdevinfo = da850_vpif_subdev,
>> > .subdev_count = ARRAY_SIZE(da850_vpif_subdev),
>> > - .output = vpif_output,
>> > - .output_count = ARRAY_SIZE(vpif_output),
>> > + .chan_config[0] = {
>> > + .outputs = da850_ch0_outputs,
>> > + .output_count = ARRAY_SIZE(da850_ch0_outputs),
>> > + },
>> > .card_name = "DA850/OMAP-L138 Video Display",
>> > };
>> >
>> > diff --git a/arch/arm/mach-davinci/board-dm646x-evm.c b/arch/arm/mach-davinci/board-dm646x-evm.c
>> > index ad249c7..c206768 100644
>> > --- a/arch/arm/mach-davinci/board-dm646x-evm.c
>> > +++ b/arch/arm/mach-davinci/board-dm646x-evm.c
>> > @@ -26,6 +26,7 @@
>> > #include <linux/i2c/pcf857x.h>
>> >
>> > #include <media/tvp514x.h>
>> > +#include <media/adv7343.h>
>> >
>> > #include <linux/mtd/mtd.h>
>> > #include <linux/mtd/nand.h>
>> > @@ -496,18 +497,44 @@ static struct vpif_subdev_info dm646x_vpif_subdev[] = {
>> > },
>> > };
>> >
>> > -static const char *output[] = {
>> > - "Composite",
>> > - "Component",
>> > - "S-Video",
>> > +static const struct vpif_output dm6467_ch0_outputs[] = {
>> > + {
>> > + .output = {
>> > + .index = 0,
>> > + .name = "Composite",
>> > + .type = V4L2_OUTPUT_TYPE_ANALOG,
>> > + },
>> > + .subdev_name = "adv7343",
>> > + .output_route = ADV7343_COMPOSITE_ID,
>> > + },
>> > + {
>> > + .output = {
>> > + .index = 1,
>> > + .name = "Component",
>> > + .type = V4L2_OUTPUT_TYPE_ANALOG,
>> > + },
>> > + .subdev_name = "adv7343",
>> > + .output_route = ADV7343_COMPONENT_ID,
>> > + },
>> > + {
>> > + .output = {
>> > + .index = 2,
>> > + .name = "S-Video",
>> > + .type = V4L2_OUTPUT_TYPE_ANALOG,
>> > + },
>> > + .subdev_name = "adv7343",
>> > + .output_route = ADV7343_SVIDEO_ID,
>> > + },
>> > };
>> >
>> > static struct vpif_display_config dm646x_vpif_display_config = {
>> > .set_clock = set_vpif_clock,
>> > .subdevinfo = dm646x_vpif_subdev,
>> > .subdev_count = ARRAY_SIZE(dm646x_vpif_subdev),
>> > - .output = output,
>> > - .output_count = ARRAY_SIZE(output),
>> > + .chan_config[0] = {
>> > + .outputs = dm6467_ch0_outputs,
>> > + .output_count = ARRAY_SIZE(dm6467_ch0_outputs),
>> > + },
>> > .card_name = "DM646x EVM",
>> > };
>> >
>> > diff --git a/drivers/media/platform/davinci/vpif_display.c b/drivers/media/platform/davinci/vpif_display.c
>> > index 8d1ce09..f68780f 100644
>> > --- a/drivers/media/platform/davinci/vpif_display.c
>> > +++ b/drivers/media/platform/davinci/vpif_display.c
>> > @@ -308,7 +308,7 @@ static int vpif_start_streaming(struct vb2_queue *vq, unsigned int count)
>> > channel2_intr_assert();
>> > channel2_intr_enable(1);
>> > enable_channel2(1);
>> > - if (vpif_config_data->ch2_clip_en)
>> > + if (vpif_config_data->chan_config[VPIF_CHANNEL2_VIDEO].clip_en)
>> > channel2_clipping_enable(1);
>> > }
>> >
>> > @@ -317,7 +317,7 @@ static int vpif_start_streaming(struct vb2_queue *vq, unsigned int count)
>> > channel3_intr_assert();
>> > channel3_intr_enable(1);
>> > enable_channel3(1);
>> > - if (vpif_config_data->ch3_clip_en)
>> > + if (vpif_config_data->chan_config[VPIF_CHANNEL3_VIDEO].clip_en)
>> > channel3_clipping_enable(1);
>> > }
>> >
>> > @@ -1174,14 +1174,16 @@ 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)
>> > + if (vpif_config_data->
>> > + chan_config[VPIF_CHANNEL2_VIDEO].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)
>> > + if (vpif_config_data->
>> > + chan_config[VPIF_CHANNEL3_VIDEO].clip_en)
>> > channel3_clipping_enable(0);
>> > enable_channel3(0);
>> > channel3_intr_enable(0);
>> > @@ -1214,41 +1216,118 @@ static int vpif_enum_output(struct file *file, void *fh,
>> > {
>> >
>> > struct vpif_display_config *config = vpif_dev->platform_data;
>> > + struct vpif_display_chan_config *chan_cfg;
>> > + struct vpif_fh *vpif_handler = fh;
>> > + struct channel_obj *ch = vpif_handler->channel;
>> >
>> > - if (output->index >= config->output_count) {
>> > + chan_cfg = &config->chan_config[ch->channel_id];
>> > + if (output->index >= chan_cfg->output_count) {
>> > vpif_dbg(1, debug, "Invalid output index\n");
>> > return -EINVAL;
>> > }
>> >
>> > - strcpy(output->name, config->output[output->index]);
>> > - output->type = V4L2_OUTPUT_TYPE_ANALOG;
>> > + memcpy(output, &chan_cfg->outputs[output->index].output,
>> > + sizeof(*output));
>
> *output = chan_cfg->outputs[output->index].output;
>
>> > output->std = VPIF_V4L2_STD;
>
Should I move 'VPIF_V4L2_STD' this macro to vpif_types.h ?
> This should be part of chan_cfg->outputs[output->index].output and not be
> overridden. If the output is a HDTV output, then std should be 0, otherwise
> it should be whatever the std is that is supported by the output.
>
> Really no different to what we do in vpif_capture.
>
Ok
>> >
>> > return 0;
>> > }
>> >
>> > +/**
>> > + * vpif_output_to_subdev() - Maps output to sub device
>> > + * @vpif_cfg - global config ptr
>> > + * @chan_cfg - channel config ptr
>> > + * @index - Given output index from application
>> > + *
>> > + * lookup the sub device information for a given output index.
>> > + * we report all the output to application. output table also
>> > + * has sub device name for the each output
>> > + */
>> > +static int
>> > +vpif_output_to_subdev(struct vpif_display_config *vpif_cfg,
>> > + struct vpif_display_chan_config *chan_cfg, int index)
>> > +{
>> > + struct vpif_subdev_info *subdev_info;
>> > + const char *subdev_name;
>> > + int i;
>> > +
>> > + vpif_dbg(2, debug, "vpif_output_to_subdev\n");
>> > +
>> > + if (chan_cfg->outputs == NULL)
>> > + return -1;
>> > +
>> > + subdev_name = chan_cfg->outputs[index].subdev_name;
>> > + if (subdev_name == NULL)
>> > + return -1;
>> > +
>> > + /* loop through the sub device list to get the sub device info */
>> > + for (i = 0; i < vpif_cfg->subdev_count; i++) {
>> > + subdev_info = &vpif_cfg->subdevinfo[i];
>> > + if (!strcmp(subdev_info->name, subdev_name))
>> > + return i;
>> > + }
>> > + return -1;
>> > +}
>> > +
>> > +/**
>> > + * vpif_set_output() - Select an output
>> > + * @vpif_cfg - global config ptr
>> > + * @ch - channel
>> > + * @index - Given output index from application
>> > + *
>> > + * Select the given output.
>> > + */
>> > +static int vpif_set_output(struct vpif_display_config *vpif_cfg,
>> > + struct channel_obj *ch, int index)
>> > +{
>> > + struct vpif_display_chan_config *chan_cfg =
>> > + &vpif_cfg->chan_config[ch->channel_id];
>> > + struct vpif_subdev_info *subdev_info = NULL;
>> > + struct v4l2_subdev *sd = NULL;
>> > + u32 input = 0, output = 0;
>> > + int sd_index;
>> > + int ret;
>> > +
>> > + sd_index = vpif_output_to_subdev(vpif_cfg, chan_cfg, index);
>> > + if (sd_index >= 0) {
>> > + sd = vpif_obj.sd[sd_index];
>> > + subdev_info = &vpif_cfg->subdevinfo[sd_index];
>> > + }
>> > +
>> > + if (sd) {
>> > + input = chan_cfg->outputs[index].input_route;
>> > + output = chan_cfg->outputs[index].output_route;
>> > + ret = v4l2_subdev_call(sd, video, s_routing, input, output, 0);
>> > + if (ret < 0) {
>
> Replace with:
>
> if (ret < 0 && ret != -ENOIOCTLCMD) {
>
> It's OK if the subdev doesn't support this s_routing operation.
>
Ok
>> > + vpif_err("Failed to set output\n");
>> > + return ret;
>> > + }
>> > +
>> > + }
>> > + ch->output_idx = index;
>> > + ch->sd = sd;
>
> Just like in the capture case you should update tvnorms:
>
> ch->video_dev->tvnorms = chan_cfg->inputs[index].output.std;
>
> Make sure tvnorms is no longer set in vpif_video_template.
> Ditto for current_norm. Since vpif_display.c supports g_std the current_norm
> field shouldn't be used anymore.
>
> The capture case also updates vpifparams.iface:
>
> ch->vpifparams.iface = chan_cfg->vpif_if;
>
> Isn't that needed for output as well?
>
The iface is never used in display, so there isnt any necessity to assign it.
Regards,
--Prabhakar Lad
>> > + return 0;
>> > +}
>> > +
>> > static int vpif_s_output(struct file *file, void *priv, unsigned int i)
>> > {
>> > + struct vpif_display_config *config = vpif_dev->platform_data;
>> > + struct vpif_display_chan_config *chan_cfg;
>> > struct vpif_fh *fh = priv;
>> > struct channel_obj *ch = fh->channel;
>> > struct common_obj *common = &ch->common[VPIF_VIDEO_INDEX];
>> > - int ret = 0;
>> > +
>> > + chan_cfg = &config->chan_config[ch->channel_id];
>> > +
>> > + if (i >= chan_cfg->output_count)
>> > + return -EINVAL;
>> >
>> > if (common->started) {
>> > vpif_err("Streaming in progress\n");
>> > return -EBUSY;
>> > }
>> >
>> > - ret = v4l2_device_call_until_err(&vpif_obj.v4l2_dev, 1, video,
>> > - s_routing, 0, i, 0);
>> > -
>> > - if (ret < 0)
>> > - vpif_err("Failed to set output standard\n");
>> > -
>> > - ch->output_idx = i;
>> > - if (vpif_obj.sd[i])
>> > - ch->sd = vpif_obj.sd[i];
>> > - return ret;
>> > + return vpif_set_output(config, ch, i);
>> > }
>> >
>> > static int vpif_g_output(struct file *file, void *priv, unsigned int *i)
>> > @@ -1291,9 +1370,12 @@ vpif_enum_dv_timings(struct file *file, void *priv,
>> > {
>> > struct vpif_fh *fh = priv;
>> > struct channel_obj *ch = fh->channel;
>> > + int ret;
>> >
>> > - return v4l2_subdev_call(vpif_obj.sd[ch->output_idx],
>> > - video, enum_dv_timings, timings);
>> > + ret = v4l2_subdev_call(ch->sd, video, enum_dv_timings, timings);
>> > + if (ret == -ENOIOCTLCMD && ret == -ENODEV)
>> > + return -EINVAL;
>> > + return ret;
>> > }
>> >
>> > /**
>> > @@ -1320,12 +1402,9 @@ static int vpif_s_dv_timings(struct file *file, void *priv,
>> >
>> > /* Configure subdevice timings, if any */
>> > ret = v4l2_subdev_call(ch->sd, video, s_dv_timings, timings);
>> > - if (ret == -ENOIOCTLCMD) {
>> > - vpif_dbg(2, debug, "Custom DV timings not supported by "
>> > - "subdevice\n");
>> > - return -ENODATA;
>> > - }
>> > - if (ret < 0 && ret != -ENODEV) {
>> > + if (ret == -ENOIOCTLCMD || ret == -ENODEV)
>> > + ret = 0;
>> > + if (ret < 0) {
>> > vpif_dbg(2, debug, "Error setting custom DV timings\n");
>> > return ret;
>> > }
>> > @@ -1754,6 +1833,11 @@ static __init int vpif_probe(struct platform_device *pdev)
>> > ch->video_dev->lock = &common->lock;
>> > video_set_drvdata(ch->video_dev, ch);
>> >
>> > + /* select output 0 */
>> > + err = vpif_set_output(config, ch, 0);
>> > + if (err)
>> > + goto probe_out;
>> > +
>> > /* register video device */
>> > vpif_dbg(1, debug, "channel=%x,channel->video_dev=%x\n",
>> > (int)ch, (int)&ch->video_dev);
>
> Regards,
>
> Hans
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2] media: davinci: vpif: display: separate out subdev from output
2012-09-24 13:21 ` Prabhakar Lad
@ 2012-09-24 13:32 ` Hans Verkuil
2012-09-25 5:38 ` Prabhakar Lad
0 siblings, 1 reply; 9+ messages in thread
From: Hans Verkuil @ 2012-09-24 13:32 UTC (permalink / raw)
To: Prabhakar Lad
Cc: LMML, DLOS, LAK, Sekhar Nori, Mauro Carvalho Chehab, VGER, Lad,
Prabhakar, Manjunath Hadli, Hans Verkuil
On Mon September 24 2012 15:21:44 Prabhakar Lad wrote:
> Hi Hans,
>
> On Mon, Sep 24, 2012 at 5:20 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> > On Mon September 24 2012 12:59:11 Hans Verkuil wrote:
> >> On Mon September 24 2012 12:44:11 Prabhakar wrote:
> >> > From: Lad, Prabhakar <prabhakar.lad@ti.com>
> >> >
> >> > vpif_display relied on a 1-1 mapping of output and subdev. This is not
> >> > necessarily the case. Separate the two. So there is a list of subdevs
> >> > and a list of outputs. Each output refers to a subdev and has routing
> >> > information. An output does not have to have a subdev.
> >> >
> >> > The initial output for each channel is set to the fist output.
> >> >
> >> > Currently missing is support for associating multiple subdevs with
> >> > an output.
> >> >
> >> > Signed-off-by: Lad, Prabhakar <prabhakar.lad@ti.com>
> >> > Signed-off-by: Manjunath Hadli <manjunath.hadli@ti.com>
> >> > Cc: Hans Verkuil <hans.verkuil@cisco.com>
> >> > Cc: Sekhar Nori <nsekhar@ti.com>
> >>
> >> Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
> >
> > I'm retracting this Ack. I did see something that wasn't right but I thought
> > it was harmless, but after thinking some more I believe it should be fixed.
> > Luckily, it's easy to fix. See below. Since we need a new version anyway I
> > also added a comment to a few minor issues that can be fixed at the same time.
> >
> >>
> >> Regards,
> >>
> >> Hans
> >>
> >> > ---
> >> > This patch is dependent on the patch series from Hans
> >> > (http://www.mail-archive.com/linux-media@vger.kernel.org/msg52270.html)
> >> >
> >> > Changes for V2:
> >> > 1: Changed v4l2_device_call_until_err() call to v4l2_subdev_call() for
> >> > s_routing, since this call is for specific subdev, pointed out by Hans.
> >> >
> >> > arch/arm/mach-davinci/board-da850-evm.c | 29 +++++-
> >> > arch/arm/mach-davinci/board-dm646x-evm.c | 39 ++++++-
> >> > drivers/media/platform/davinci/vpif_display.c | 136 ++++++++++++++++++++-----
> >> > include/media/davinci/vpif_types.h | 20 +++-
> >> > 4 files changed, 183 insertions(+), 41 deletions(-)
> >> >
> >> > diff --git a/arch/arm/mach-davinci/board-da850-evm.c b/arch/arm/mach-davinci/board-da850-evm.c
> >> > index 3081ea4..23a7012 100644
> >> > --- a/arch/arm/mach-davinci/board-da850-evm.c
> >> > +++ b/arch/arm/mach-davinci/board-da850-evm.c
> >> > @@ -46,6 +46,7 @@
> >> > #include <mach/spi.h>
> >> >
> >> > #include <media/tvp514x.h>
> >> > +#include <media/adv7343.h>
> >> >
> >> > #define DA850_EVM_PHY_ID "davinci_mdio-0:00"
> >> > #define DA850_LCD_PWR_PIN GPIO_TO_PIN(2, 8)
> >> > @@ -1257,16 +1258,34 @@ static struct vpif_subdev_info da850_vpif_subdev[] = {
> >> > },
> >> > };
> >> >
> >> > -static const char const *vpif_output[] = {
> >> > - "Composite",
> >> > - "S-Video",
> >> > +static const struct vpif_output da850_ch0_outputs[] = {
> >> > + {
> >> > + .output = {
> >> > + .index = 0,
> >> > + .name = "Composite",
> >> > + .type = V4L2_OUTPUT_TYPE_ANALOG,
> >> > + },
> >> > + .subdev_name = "adv7343",
> >> > + .output_route = ADV7343_COMPOSITE_ID,
> >> > + },
> >> > + {
> >> > + .output = {
> >> > + .index = 1,
> >> > + .name = "S-Video",
> >> > + .type = V4L2_OUTPUT_TYPE_ANALOG,
> >> > + },
> >> > + .subdev_name = "adv7343",
> >> > + .output_route = ADV7343_SVIDEO_ID,
> >> > + },
> >> > };
> >> >
> >> > static struct vpif_display_config da850_vpif_display_config = {
> >> > .subdevinfo = da850_vpif_subdev,
> >> > .subdev_count = ARRAY_SIZE(da850_vpif_subdev),
> >> > - .output = vpif_output,
> >> > - .output_count = ARRAY_SIZE(vpif_output),
> >> > + .chan_config[0] = {
> >> > + .outputs = da850_ch0_outputs,
> >> > + .output_count = ARRAY_SIZE(da850_ch0_outputs),
> >> > + },
> >> > .card_name = "DA850/OMAP-L138 Video Display",
> >> > };
> >> >
> >> > diff --git a/arch/arm/mach-davinci/board-dm646x-evm.c b/arch/arm/mach-davinci/board-dm646x-evm.c
> >> > index ad249c7..c206768 100644
> >> > --- a/arch/arm/mach-davinci/board-dm646x-evm.c
> >> > +++ b/arch/arm/mach-davinci/board-dm646x-evm.c
> >> > @@ -26,6 +26,7 @@
> >> > #include <linux/i2c/pcf857x.h>
> >> >
> >> > #include <media/tvp514x.h>
> >> > +#include <media/adv7343.h>
> >> >
> >> > #include <linux/mtd/mtd.h>
> >> > #include <linux/mtd/nand.h>
> >> > @@ -496,18 +497,44 @@ static struct vpif_subdev_info dm646x_vpif_subdev[] = {
> >> > },
> >> > };
> >> >
> >> > -static const char *output[] = {
> >> > - "Composite",
> >> > - "Component",
> >> > - "S-Video",
> >> > +static const struct vpif_output dm6467_ch0_outputs[] = {
> >> > + {
> >> > + .output = {
> >> > + .index = 0,
> >> > + .name = "Composite",
> >> > + .type = V4L2_OUTPUT_TYPE_ANALOG,
> >> > + },
> >> > + .subdev_name = "adv7343",
> >> > + .output_route = ADV7343_COMPOSITE_ID,
> >> > + },
> >> > + {
> >> > + .output = {
> >> > + .index = 1,
> >> > + .name = "Component",
> >> > + .type = V4L2_OUTPUT_TYPE_ANALOG,
> >> > + },
> >> > + .subdev_name = "adv7343",
> >> > + .output_route = ADV7343_COMPONENT_ID,
> >> > + },
> >> > + {
> >> > + .output = {
> >> > + .index = 2,
> >> > + .name = "S-Video",
> >> > + .type = V4L2_OUTPUT_TYPE_ANALOG,
> >> > + },
> >> > + .subdev_name = "adv7343",
> >> > + .output_route = ADV7343_SVIDEO_ID,
> >> > + },
> >> > };
> >> >
> >> > static struct vpif_display_config dm646x_vpif_display_config = {
> >> > .set_clock = set_vpif_clock,
> >> > .subdevinfo = dm646x_vpif_subdev,
> >> > .subdev_count = ARRAY_SIZE(dm646x_vpif_subdev),
> >> > - .output = output,
> >> > - .output_count = ARRAY_SIZE(output),
> >> > + .chan_config[0] = {
> >> > + .outputs = dm6467_ch0_outputs,
> >> > + .output_count = ARRAY_SIZE(dm6467_ch0_outputs),
> >> > + },
> >> > .card_name = "DM646x EVM",
> >> > };
> >> >
> >> > diff --git a/drivers/media/platform/davinci/vpif_display.c b/drivers/media/platform/davinci/vpif_display.c
> >> > index 8d1ce09..f68780f 100644
> >> > --- a/drivers/media/platform/davinci/vpif_display.c
> >> > +++ b/drivers/media/platform/davinci/vpif_display.c
> >> > @@ -308,7 +308,7 @@ static int vpif_start_streaming(struct vb2_queue *vq, unsigned int count)
> >> > channel2_intr_assert();
> >> > channel2_intr_enable(1);
> >> > enable_channel2(1);
> >> > - if (vpif_config_data->ch2_clip_en)
> >> > + if (vpif_config_data->chan_config[VPIF_CHANNEL2_VIDEO].clip_en)
> >> > channel2_clipping_enable(1);
> >> > }
> >> >
> >> > @@ -317,7 +317,7 @@ static int vpif_start_streaming(struct vb2_queue *vq, unsigned int count)
> >> > channel3_intr_assert();
> >> > channel3_intr_enable(1);
> >> > enable_channel3(1);
> >> > - if (vpif_config_data->ch3_clip_en)
> >> > + if (vpif_config_data->chan_config[VPIF_CHANNEL3_VIDEO].clip_en)
> >> > channel3_clipping_enable(1);
> >> > }
> >> >
> >> > @@ -1174,14 +1174,16 @@ 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)
> >> > + if (vpif_config_data->
> >> > + chan_config[VPIF_CHANNEL2_VIDEO].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)
> >> > + if (vpif_config_data->
> >> > + chan_config[VPIF_CHANNEL3_VIDEO].clip_en)
> >> > channel3_clipping_enable(0);
> >> > enable_channel3(0);
> >> > channel3_intr_enable(0);
> >> > @@ -1214,41 +1216,118 @@ static int vpif_enum_output(struct file *file, void *fh,
> >> > {
> >> >
> >> > struct vpif_display_config *config = vpif_dev->platform_data;
> >> > + struct vpif_display_chan_config *chan_cfg;
> >> > + struct vpif_fh *vpif_handler = fh;
> >> > + struct channel_obj *ch = vpif_handler->channel;
> >> >
> >> > - if (output->index >= config->output_count) {
> >> > + chan_cfg = &config->chan_config[ch->channel_id];
> >> > + if (output->index >= chan_cfg->output_count) {
> >> > vpif_dbg(1, debug, "Invalid output index\n");
> >> > return -EINVAL;
> >> > }
> >> >
> >> > - strcpy(output->name, config->output[output->index]);
> >> > - output->type = V4L2_OUTPUT_TYPE_ANALOG;
> >> > + memcpy(output, &chan_cfg->outputs[output->index].output,
> >> > + sizeof(*output));
> >
> > *output = chan_cfg->outputs[output->index].output;
> >
> >> > output->std = VPIF_V4L2_STD;
> >
> Should I move 'VPIF_V4L2_STD' this macro to vpif_types.h ?
No. VPIF_V4L2_STD == V4L2_STD_ALL, so just use V4L2_STD_ALL directly.
> > This should be part of chan_cfg->outputs[output->index].output and not be
> > overridden. If the output is a HDTV output, then std should be 0, otherwise
> > it should be whatever the std is that is supported by the output.
> >
> > Really no different to what we do in vpif_capture.
> >
> Ok
>
> >> >
> >> > return 0;
> >> > }
> >> >
> >> > +/**
> >> > + * vpif_output_to_subdev() - Maps output to sub device
> >> > + * @vpif_cfg - global config ptr
> >> > + * @chan_cfg - channel config ptr
> >> > + * @index - Given output index from application
> >> > + *
> >> > + * lookup the sub device information for a given output index.
> >> > + * we report all the output to application. output table also
> >> > + * has sub device name for the each output
> >> > + */
> >> > +static int
> >> > +vpif_output_to_subdev(struct vpif_display_config *vpif_cfg,
> >> > + struct vpif_display_chan_config *chan_cfg, int index)
> >> > +{
> >> > + struct vpif_subdev_info *subdev_info;
> >> > + const char *subdev_name;
> >> > + int i;
> >> > +
> >> > + vpif_dbg(2, debug, "vpif_output_to_subdev\n");
> >> > +
> >> > + if (chan_cfg->outputs == NULL)
> >> > + return -1;
> >> > +
> >> > + subdev_name = chan_cfg->outputs[index].subdev_name;
> >> > + if (subdev_name == NULL)
> >> > + return -1;
> >> > +
> >> > + /* loop through the sub device list to get the sub device info */
> >> > + for (i = 0; i < vpif_cfg->subdev_count; i++) {
> >> > + subdev_info = &vpif_cfg->subdevinfo[i];
> >> > + if (!strcmp(subdev_info->name, subdev_name))
> >> > + return i;
> >> > + }
> >> > + return -1;
> >> > +}
> >> > +
> >> > +/**
> >> > + * vpif_set_output() - Select an output
> >> > + * @vpif_cfg - global config ptr
> >> > + * @ch - channel
> >> > + * @index - Given output index from application
> >> > + *
> >> > + * Select the given output.
> >> > + */
> >> > +static int vpif_set_output(struct vpif_display_config *vpif_cfg,
> >> > + struct channel_obj *ch, int index)
> >> > +{
> >> > + struct vpif_display_chan_config *chan_cfg =
> >> > + &vpif_cfg->chan_config[ch->channel_id];
> >> > + struct vpif_subdev_info *subdev_info = NULL;
> >> > + struct v4l2_subdev *sd = NULL;
> >> > + u32 input = 0, output = 0;
> >> > + int sd_index;
> >> > + int ret;
> >> > +
> >> > + sd_index = vpif_output_to_subdev(vpif_cfg, chan_cfg, index);
> >> > + if (sd_index >= 0) {
> >> > + sd = vpif_obj.sd[sd_index];
> >> > + subdev_info = &vpif_cfg->subdevinfo[sd_index];
> >> > + }
> >> > +
> >> > + if (sd) {
> >> > + input = chan_cfg->outputs[index].input_route;
> >> > + output = chan_cfg->outputs[index].output_route;
> >> > + ret = v4l2_subdev_call(sd, video, s_routing, input, output, 0);
> >> > + if (ret < 0) {
> >
> > Replace with:
> >
> > if (ret < 0 && ret != -ENOIOCTLCMD) {
> >
> > It's OK if the subdev doesn't support this s_routing operation.
> >
> Ok
>
> >> > + vpif_err("Failed to set output\n");
> >> > + return ret;
> >> > + }
> >> > +
> >> > + }
> >> > + ch->output_idx = index;
> >> > + ch->sd = sd;
> >
> > Just like in the capture case you should update tvnorms:
> >
> > ch->video_dev->tvnorms = chan_cfg->inputs[index].output.std;
> >
> > Make sure tvnorms is no longer set in vpif_video_template.
> > Ditto for current_norm. Since vpif_display.c supports g_std the current_norm
> > field shouldn't be used anymore.
> >
> > The capture case also updates vpifparams.iface:
> >
> > ch->vpifparams.iface = chan_cfg->vpif_if;
> >
> > Isn't that needed for output as well?
> >
> The iface is never used in display, so there isnt any necessity to assign it.
But it is used in vpif.c through vpif_set_video_params()!
Regards,
Hans
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2] media: davinci: vpif: display: separate out subdev from output
2012-09-24 13:32 ` Hans Verkuil
@ 2012-09-25 5:38 ` Prabhakar Lad
2012-09-25 6:29 ` Hans Verkuil
0 siblings, 1 reply; 9+ messages in thread
From: Prabhakar Lad @ 2012-09-25 5:38 UTC (permalink / raw)
To: Hans Verkuil
Cc: LMML, DLOS, LAK, Sekhar Nori, Mauro Carvalho Chehab, VGER, Lad,
Prabhakar, Manjunath Hadli, Hans Verkuil
Hi Hans,
On Mon, Sep 24, 2012 at 7:02 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> On Mon September 24 2012 15:21:44 Prabhakar Lad wrote:
>> Hi Hans,
>>
>> On Mon, Sep 24, 2012 at 5:20 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>> > On Mon September 24 2012 12:59:11 Hans Verkuil wrote:
>> >> On Mon September 24 2012 12:44:11 Prabhakar wrote:
>> >> > From: Lad, Prabhakar <prabhakar.lad@ti.com>
>> >> >
>> >> > vpif_display relied on a 1-1 mapping of output and subdev. This is not
>> >> > necessarily the case. Separate the two. So there is a list of subdevs
>> >> > and a list of outputs. Each output refers to a subdev and has routing
>> >> > information. An output does not have to have a subdev.
>> >> >
>> >> > The initial output for each channel is set to the fist output.
>> >> >
>> >> > Currently missing is support for associating multiple subdevs with
>> >> > an output.
>> >> >
>> >> > Signed-off-by: Lad, Prabhakar <prabhakar.lad@ti.com>
>> >> > Signed-off-by: Manjunath Hadli <manjunath.hadli@ti.com>
>> >> > Cc: Hans Verkuil <hans.verkuil@cisco.com>
>> >> > Cc: Sekhar Nori <nsekhar@ti.com>
>> >>
>> >> Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
>> >
>> > I'm retracting this Ack. I did see something that wasn't right but I thought
>> > it was harmless, but after thinking some more I believe it should be fixed.
>> > Luckily, it's easy to fix. See below. Since we need a new version anyway I
>> > also added a comment to a few minor issues that can be fixed at the same time.
>> >
>> >>
>> >> Regards,
>> >>
>> >> Hans
>> >>
>> >> > ---
>> >> > This patch is dependent on the patch series from Hans
>> >> > (http://www.mail-archive.com/linux-media@vger.kernel.org/msg52270.html)
>> >> >
>> >> > Changes for V2:
>> >> > 1: Changed v4l2_device_call_until_err() call to v4l2_subdev_call() for
>> >> > s_routing, since this call is for specific subdev, pointed out by Hans.
>> >> >
>> >> > arch/arm/mach-davinci/board-da850-evm.c | 29 +++++-
>> >> > arch/arm/mach-davinci/board-dm646x-evm.c | 39 ++++++-
>> >> > drivers/media/platform/davinci/vpif_display.c | 136 ++++++++++++++++++++-----
>> >> > include/media/davinci/vpif_types.h | 20 +++-
>> >> > 4 files changed, 183 insertions(+), 41 deletions(-)
>> >> >
>> >> > diff --git a/arch/arm/mach-davinci/board-da850-evm.c b/arch/arm/mach-davinci/board-da850-evm.c
>> >> > index 3081ea4..23a7012 100644
>> >> > --- a/arch/arm/mach-davinci/board-da850-evm.c
>> >> > +++ b/arch/arm/mach-davinci/board-da850-evm.c
>> >> > @@ -46,6 +46,7 @@
>> >> > #include <mach/spi.h>
>> >> >
>> >> > #include <media/tvp514x.h>
>> >> > +#include <media/adv7343.h>
>> >> >
>> >> > #define DA850_EVM_PHY_ID "davinci_mdio-0:00"
>> >> > #define DA850_LCD_PWR_PIN GPIO_TO_PIN(2, 8)
>> >> > @@ -1257,16 +1258,34 @@ static struct vpif_subdev_info da850_vpif_subdev[] = {
>> >> > },
>> >> > };
>> >> >
>> >> > -static const char const *vpif_output[] = {
>> >> > - "Composite",
>> >> > - "S-Video",
>> >> > +static const struct vpif_output da850_ch0_outputs[] = {
>> >> > + {
>> >> > + .output = {
>> >> > + .index = 0,
>> >> > + .name = "Composite",
>> >> > + .type = V4L2_OUTPUT_TYPE_ANALOG,
>> >> > + },
>> >> > + .subdev_name = "adv7343",
>> >> > + .output_route = ADV7343_COMPOSITE_ID,
>> >> > + },
>> >> > + {
>> >> > + .output = {
>> >> > + .index = 1,
>> >> > + .name = "S-Video",
>> >> > + .type = V4L2_OUTPUT_TYPE_ANALOG,
>> >> > + },
>> >> > + .subdev_name = "adv7343",
>> >> > + .output_route = ADV7343_SVIDEO_ID,
>> >> > + },
>> >> > };
>> >> >
>> >> > static struct vpif_display_config da850_vpif_display_config = {
>> >> > .subdevinfo = da850_vpif_subdev,
>> >> > .subdev_count = ARRAY_SIZE(da850_vpif_subdev),
>> >> > - .output = vpif_output,
>> >> > - .output_count = ARRAY_SIZE(vpif_output),
>> >> > + .chan_config[0] = {
>> >> > + .outputs = da850_ch0_outputs,
>> >> > + .output_count = ARRAY_SIZE(da850_ch0_outputs),
>> >> > + },
>> >> > .card_name = "DA850/OMAP-L138 Video Display",
>> >> > };
>> >> >
>> >> > diff --git a/arch/arm/mach-davinci/board-dm646x-evm.c b/arch/arm/mach-davinci/board-dm646x-evm.c
>> >> > index ad249c7..c206768 100644
>> >> > --- a/arch/arm/mach-davinci/board-dm646x-evm.c
>> >> > +++ b/arch/arm/mach-davinci/board-dm646x-evm.c
>> >> > @@ -26,6 +26,7 @@
>> >> > #include <linux/i2c/pcf857x.h>
>> >> >
>> >> > #include <media/tvp514x.h>
>> >> > +#include <media/adv7343.h>
>> >> >
>> >> > #include <linux/mtd/mtd.h>
>> >> > #include <linux/mtd/nand.h>
>> >> > @@ -496,18 +497,44 @@ static struct vpif_subdev_info dm646x_vpif_subdev[] = {
>> >> > },
>> >> > };
>> >> >
>> >> > -static const char *output[] = {
>> >> > - "Composite",
>> >> > - "Component",
>> >> > - "S-Video",
>> >> > +static const struct vpif_output dm6467_ch0_outputs[] = {
>> >> > + {
>> >> > + .output = {
>> >> > + .index = 0,
>> >> > + .name = "Composite",
>> >> > + .type = V4L2_OUTPUT_TYPE_ANALOG,
>> >> > + },
>> >> > + .subdev_name = "adv7343",
>> >> > + .output_route = ADV7343_COMPOSITE_ID,
>> >> > + },
>> >> > + {
>> >> > + .output = {
>> >> > + .index = 1,
>> >> > + .name = "Component",
>> >> > + .type = V4L2_OUTPUT_TYPE_ANALOG,
>> >> > + },
>> >> > + .subdev_name = "adv7343",
>> >> > + .output_route = ADV7343_COMPONENT_ID,
>> >> > + },
>> >> > + {
>> >> > + .output = {
>> >> > + .index = 2,
>> >> > + .name = "S-Video",
>> >> > + .type = V4L2_OUTPUT_TYPE_ANALOG,
>> >> > + },
>> >> > + .subdev_name = "adv7343",
>> >> > + .output_route = ADV7343_SVIDEO_ID,
>> >> > + },
>> >> > };
>> >> >
>> >> > static struct vpif_display_config dm646x_vpif_display_config = {
>> >> > .set_clock = set_vpif_clock,
>> >> > .subdevinfo = dm646x_vpif_subdev,
>> >> > .subdev_count = ARRAY_SIZE(dm646x_vpif_subdev),
>> >> > - .output = output,
>> >> > - .output_count = ARRAY_SIZE(output),
>> >> > + .chan_config[0] = {
>> >> > + .outputs = dm6467_ch0_outputs,
>> >> > + .output_count = ARRAY_SIZE(dm6467_ch0_outputs),
>> >> > + },
>> >> > .card_name = "DM646x EVM",
>> >> > };
>> >> >
>> >> > diff --git a/drivers/media/platform/davinci/vpif_display.c b/drivers/media/platform/davinci/vpif_display.c
>> >> > index 8d1ce09..f68780f 100644
>> >> > --- a/drivers/media/platform/davinci/vpif_display.c
>> >> > +++ b/drivers/media/platform/davinci/vpif_display.c
>> >> > @@ -308,7 +308,7 @@ static int vpif_start_streaming(struct vb2_queue *vq, unsigned int count)
>> >> > channel2_intr_assert();
>> >> > channel2_intr_enable(1);
>> >> > enable_channel2(1);
>> >> > - if (vpif_config_data->ch2_clip_en)
>> >> > + if (vpif_config_data->chan_config[VPIF_CHANNEL2_VIDEO].clip_en)
>> >> > channel2_clipping_enable(1);
>> >> > }
>> >> >
>> >> > @@ -317,7 +317,7 @@ static int vpif_start_streaming(struct vb2_queue *vq, unsigned int count)
>> >> > channel3_intr_assert();
>> >> > channel3_intr_enable(1);
>> >> > enable_channel3(1);
>> >> > - if (vpif_config_data->ch3_clip_en)
>> >> > + if (vpif_config_data->chan_config[VPIF_CHANNEL3_VIDEO].clip_en)
>> >> > channel3_clipping_enable(1);
>> >> > }
>> >> >
>> >> > @@ -1174,14 +1174,16 @@ 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)
>> >> > + if (vpif_config_data->
>> >> > + chan_config[VPIF_CHANNEL2_VIDEO].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)
>> >> > + if (vpif_config_data->
>> >> > + chan_config[VPIF_CHANNEL3_VIDEO].clip_en)
>> >> > channel3_clipping_enable(0);
>> >> > enable_channel3(0);
>> >> > channel3_intr_enable(0);
>> >> > @@ -1214,41 +1216,118 @@ static int vpif_enum_output(struct file *file, void *fh,
>> >> > {
>> >> >
>> >> > struct vpif_display_config *config = vpif_dev->platform_data;
>> >> > + struct vpif_display_chan_config *chan_cfg;
>> >> > + struct vpif_fh *vpif_handler = fh;
>> >> > + struct channel_obj *ch = vpif_handler->channel;
>> >> >
>> >> > - if (output->index >= config->output_count) {
>> >> > + chan_cfg = &config->chan_config[ch->channel_id];
>> >> > + if (output->index >= chan_cfg->output_count) {
>> >> > vpif_dbg(1, debug, "Invalid output index\n");
>> >> > return -EINVAL;
>> >> > }
>> >> >
>> >> > - strcpy(output->name, config->output[output->index]);
>> >> > - output->type = V4L2_OUTPUT_TYPE_ANALOG;
>> >> > + memcpy(output, &chan_cfg->outputs[output->index].output,
>> >> > + sizeof(*output));
>> >
>> > *output = chan_cfg->outputs[output->index].output;
>> >
>> >> > output->std = VPIF_V4L2_STD;
>> >
>> Should I move 'VPIF_V4L2_STD' this macro to vpif_types.h ?
>
> No. VPIF_V4L2_STD == V4L2_STD_ALL, so just use V4L2_STD_ALL directly.
>
Ok.
>> > This should be part of chan_cfg->outputs[output->index].output and not be
>> > overridden. If the output is a HDTV output, then std should be 0, otherwise
>> > it should be whatever the std is that is supported by the output.
>> >
>> > Really no different to what we do in vpif_capture.
>> >
>> Ok
>>
>> >> >
>> >> > return 0;
>> >> > }
>> >> >
>> >> > +/**
>> >> > + * vpif_output_to_subdev() - Maps output to sub device
>> >> > + * @vpif_cfg - global config ptr
>> >> > + * @chan_cfg - channel config ptr
>> >> > + * @index - Given output index from application
>> >> > + *
>> >> > + * lookup the sub device information for a given output index.
>> >> > + * we report all the output to application. output table also
>> >> > + * has sub device name for the each output
>> >> > + */
>> >> > +static int
>> >> > +vpif_output_to_subdev(struct vpif_display_config *vpif_cfg,
>> >> > + struct vpif_display_chan_config *chan_cfg, int index)
>> >> > +{
>> >> > + struct vpif_subdev_info *subdev_info;
>> >> > + const char *subdev_name;
>> >> > + int i;
>> >> > +
>> >> > + vpif_dbg(2, debug, "vpif_output_to_subdev\n");
>> >> > +
>> >> > + if (chan_cfg->outputs == NULL)
>> >> > + return -1;
>> >> > +
>> >> > + subdev_name = chan_cfg->outputs[index].subdev_name;
>> >> > + if (subdev_name == NULL)
>> >> > + return -1;
>> >> > +
>> >> > + /* loop through the sub device list to get the sub device info */
>> >> > + for (i = 0; i < vpif_cfg->subdev_count; i++) {
>> >> > + subdev_info = &vpif_cfg->subdevinfo[i];
>> >> > + if (!strcmp(subdev_info->name, subdev_name))
>> >> > + return i;
>> >> > + }
>> >> > + return -1;
>> >> > +}
>> >> > +
>> >> > +/**
>> >> > + * vpif_set_output() - Select an output
>> >> > + * @vpif_cfg - global config ptr
>> >> > + * @ch - channel
>> >> > + * @index - Given output index from application
>> >> > + *
>> >> > + * Select the given output.
>> >> > + */
>> >> > +static int vpif_set_output(struct vpif_display_config *vpif_cfg,
>> >> > + struct channel_obj *ch, int index)
>> >> > +{
>> >> > + struct vpif_display_chan_config *chan_cfg =
>> >> > + &vpif_cfg->chan_config[ch->channel_id];
>> >> > + struct vpif_subdev_info *subdev_info = NULL;
>> >> > + struct v4l2_subdev *sd = NULL;
>> >> > + u32 input = 0, output = 0;
>> >> > + int sd_index;
>> >> > + int ret;
>> >> > +
>> >> > + sd_index = vpif_output_to_subdev(vpif_cfg, chan_cfg, index);
>> >> > + if (sd_index >= 0) {
>> >> > + sd = vpif_obj.sd[sd_index];
>> >> > + subdev_info = &vpif_cfg->subdevinfo[sd_index];
>> >> > + }
>> >> > +
>> >> > + if (sd) {
>> >> > + input = chan_cfg->outputs[index].input_route;
>> >> > + output = chan_cfg->outputs[index].output_route;
>> >> > + ret = v4l2_subdev_call(sd, video, s_routing, input, output, 0);
>> >> > + if (ret < 0) {
>> >
>> > Replace with:
>> >
>> > if (ret < 0 && ret != -ENOIOCTLCMD) {
>> >
>> > It's OK if the subdev doesn't support this s_routing operation.
>> >
>> Ok
>>
>> >> > + vpif_err("Failed to set output\n");
>> >> > + return ret;
>> >> > + }
>> >> > +
>> >> > + }
>> >> > + ch->output_idx = index;
>> >> > + ch->sd = sd;
>> >
>> > Just like in the capture case you should update tvnorms:
>> >
>> > ch->video_dev->tvnorms = chan_cfg->inputs[index].output.std;
>> >
>> > Make sure tvnorms is no longer set in vpif_video_template.
>> > Ditto for current_norm. Since vpif_display.c supports g_std the current_norm
>> > field shouldn't be used anymore.
>> >
>> > The capture case also updates vpifparams.iface:
>> >
>> > ch->vpifparams.iface = chan_cfg->vpif_if;
>> >
>> > Isn't that needed for output as well?
>> >
>> The iface is never used in display, so there isnt any necessity to assign it.
>
> But it is used in vpif.c through vpif_set_video_params()!
>
Agreed iface is used in vpif_set_video_params(), but it still for capture.
there is check 'if (channel_id > 1)' in the function while accessing the iface
variable, so for display this will be always true, so no point in assigning it.
Thanks and Regards,
--Prabhakar Lad
> Regards,
>
> Hans
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2] media: davinci: vpif: display: separate out subdev from output
2012-09-25 5:38 ` Prabhakar Lad
@ 2012-09-25 6:29 ` Hans Verkuil
2012-09-25 6:34 ` Prabhakar Lad
0 siblings, 1 reply; 9+ messages in thread
From: Hans Verkuil @ 2012-09-25 6:29 UTC (permalink / raw)
To: Prabhakar Lad
Cc: LMML, DLOS, LAK, Sekhar Nori, Mauro Carvalho Chehab, VGER, Lad,
Prabhakar, Manjunath Hadli, Hans Verkuil
On Tue September 25 2012 07:38:12 Prabhakar Lad wrote:
> Hi Hans,
>
> On Mon, Sep 24, 2012 at 7:02 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> > On Mon September 24 2012 15:21:44 Prabhakar Lad wrote:
> >> Hi Hans,
> >>
> >> On Mon, Sep 24, 2012 at 5:20 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> >> > On Mon September 24 2012 12:59:11 Hans Verkuil wrote:
> >> >> On Mon September 24 2012 12:44:11 Prabhakar wrote:
> >> >> > From: Lad, Prabhakar <prabhakar.lad@ti.com>
> >> >> >
> >> >> > vpif_display relied on a 1-1 mapping of output and subdev. This is not
> >> >> > necessarily the case. Separate the two. So there is a list of subdevs
> >> >> > and a list of outputs. Each output refers to a subdev and has routing
> >> >> > information. An output does not have to have a subdev.
> >> >> >
> >> >> > The initial output for each channel is set to the fist output.
> >> >> >
> >> >> > Currently missing is support for associating multiple subdevs with
> >> >> > an output.
> >> >> >
> >> >> > Signed-off-by: Lad, Prabhakar <prabhakar.lad@ti.com>
> >> >> > Signed-off-by: Manjunath Hadli <manjunath.hadli@ti.com>
> >> >> > Cc: Hans Verkuil <hans.verkuil@cisco.com>
> >> >> > Cc: Sekhar Nori <nsekhar@ti.com>
> >> >>
> >> >> Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
> >> >
> >> > I'm retracting this Ack. I did see something that wasn't right but I thought
> >> > it was harmless, but after thinking some more I believe it should be fixed.
> >> > Luckily, it's easy to fix. See below. Since we need a new version anyway I
> >> > also added a comment to a few minor issues that can be fixed at the same time.
> >> >
> >> >>
> >> >> Regards,
> >> >>
> >> >> Hans
> >> >>
> >> >> > ---
> >> >> > This patch is dependent on the patch series from Hans
> >> >> > (http://www.mail-archive.com/linux-media@vger.kernel.org/msg52270.html)
> >> >> >
> >> >> > Changes for V2:
> >> >> > 1: Changed v4l2_device_call_until_err() call to v4l2_subdev_call() for
> >> >> > s_routing, since this call is for specific subdev, pointed out by Hans.
> >> >> >
> >> >> > arch/arm/mach-davinci/board-da850-evm.c | 29 +++++-
> >> >> > arch/arm/mach-davinci/board-dm646x-evm.c | 39 ++++++-
> >> >> > drivers/media/platform/davinci/vpif_display.c | 136 ++++++++++++++++++++-----
> >> >> > include/media/davinci/vpif_types.h | 20 +++-
> >> >> > 4 files changed, 183 insertions(+), 41 deletions(-)
> >> >> >
> >> >> > diff --git a/arch/arm/mach-davinci/board-da850-evm.c b/arch/arm/mach-davinci/board-da850-evm.c
> >> >> > index 3081ea4..23a7012 100644
> >> >> > --- a/arch/arm/mach-davinci/board-da850-evm.c
> >> >> > +++ b/arch/arm/mach-davinci/board-da850-evm.c
> >> >> > @@ -46,6 +46,7 @@
> >> >> > #include <mach/spi.h>
> >> >> >
> >> >> > #include <media/tvp514x.h>
> >> >> > +#include <media/adv7343.h>
> >> >> >
> >> >> > #define DA850_EVM_PHY_ID "davinci_mdio-0:00"
> >> >> > #define DA850_LCD_PWR_PIN GPIO_TO_PIN(2, 8)
> >> >> > @@ -1257,16 +1258,34 @@ static struct vpif_subdev_info da850_vpif_subdev[] = {
> >> >> > },
> >> >> > };
> >> >> >
> >> >> > -static const char const *vpif_output[] = {
> >> >> > - "Composite",
> >> >> > - "S-Video",
> >> >> > +static const struct vpif_output da850_ch0_outputs[] = {
> >> >> > + {
> >> >> > + .output = {
> >> >> > + .index = 0,
> >> >> > + .name = "Composite",
> >> >> > + .type = V4L2_OUTPUT_TYPE_ANALOG,
> >> >> > + },
> >> >> > + .subdev_name = "adv7343",
> >> >> > + .output_route = ADV7343_COMPOSITE_ID,
> >> >> > + },
> >> >> > + {
> >> >> > + .output = {
> >> >> > + .index = 1,
> >> >> > + .name = "S-Video",
> >> >> > + .type = V4L2_OUTPUT_TYPE_ANALOG,
> >> >> > + },
> >> >> > + .subdev_name = "adv7343",
> >> >> > + .output_route = ADV7343_SVIDEO_ID,
> >> >> > + },
> >> >> > };
> >> >> >
> >> >> > static struct vpif_display_config da850_vpif_display_config = {
> >> >> > .subdevinfo = da850_vpif_subdev,
> >> >> > .subdev_count = ARRAY_SIZE(da850_vpif_subdev),
> >> >> > - .output = vpif_output,
> >> >> > - .output_count = ARRAY_SIZE(vpif_output),
> >> >> > + .chan_config[0] = {
> >> >> > + .outputs = da850_ch0_outputs,
> >> >> > + .output_count = ARRAY_SIZE(da850_ch0_outputs),
> >> >> > + },
> >> >> > .card_name = "DA850/OMAP-L138 Video Display",
> >> >> > };
> >> >> >
> >> >> > diff --git a/arch/arm/mach-davinci/board-dm646x-evm.c b/arch/arm/mach-davinci/board-dm646x-evm.c
> >> >> > index ad249c7..c206768 100644
> >> >> > --- a/arch/arm/mach-davinci/board-dm646x-evm.c
> >> >> > +++ b/arch/arm/mach-davinci/board-dm646x-evm.c
> >> >> > @@ -26,6 +26,7 @@
> >> >> > #include <linux/i2c/pcf857x.h>
> >> >> >
> >> >> > #include <media/tvp514x.h>
> >> >> > +#include <media/adv7343.h>
> >> >> >
> >> >> > #include <linux/mtd/mtd.h>
> >> >> > #include <linux/mtd/nand.h>
> >> >> > @@ -496,18 +497,44 @@ static struct vpif_subdev_info dm646x_vpif_subdev[] = {
> >> >> > },
> >> >> > };
> >> >> >
> >> >> > -static const char *output[] = {
> >> >> > - "Composite",
> >> >> > - "Component",
> >> >> > - "S-Video",
> >> >> > +static const struct vpif_output dm6467_ch0_outputs[] = {
> >> >> > + {
> >> >> > + .output = {
> >> >> > + .index = 0,
> >> >> > + .name = "Composite",
> >> >> > + .type = V4L2_OUTPUT_TYPE_ANALOG,
> >> >> > + },
> >> >> > + .subdev_name = "adv7343",
> >> >> > + .output_route = ADV7343_COMPOSITE_ID,
> >> >> > + },
> >> >> > + {
> >> >> > + .output = {
> >> >> > + .index = 1,
> >> >> > + .name = "Component",
> >> >> > + .type = V4L2_OUTPUT_TYPE_ANALOG,
> >> >> > + },
> >> >> > + .subdev_name = "adv7343",
> >> >> > + .output_route = ADV7343_COMPONENT_ID,
> >> >> > + },
> >> >> > + {
> >> >> > + .output = {
> >> >> > + .index = 2,
> >> >> > + .name = "S-Video",
> >> >> > + .type = V4L2_OUTPUT_TYPE_ANALOG,
> >> >> > + },
> >> >> > + .subdev_name = "adv7343",
> >> >> > + .output_route = ADV7343_SVIDEO_ID,
> >> >> > + },
> >> >> > };
> >> >> >
> >> >> > static struct vpif_display_config dm646x_vpif_display_config = {
> >> >> > .set_clock = set_vpif_clock,
> >> >> > .subdevinfo = dm646x_vpif_subdev,
> >> >> > .subdev_count = ARRAY_SIZE(dm646x_vpif_subdev),
> >> >> > - .output = output,
> >> >> > - .output_count = ARRAY_SIZE(output),
> >> >> > + .chan_config[0] = {
> >> >> > + .outputs = dm6467_ch0_outputs,
> >> >> > + .output_count = ARRAY_SIZE(dm6467_ch0_outputs),
> >> >> > + },
> >> >> > .card_name = "DM646x EVM",
> >> >> > };
> >> >> >
> >> >> > diff --git a/drivers/media/platform/davinci/vpif_display.c b/drivers/media/platform/davinci/vpif_display.c
> >> >> > index 8d1ce09..f68780f 100644
> >> >> > --- a/drivers/media/platform/davinci/vpif_display.c
> >> >> > +++ b/drivers/media/platform/davinci/vpif_display.c
> >> >> > @@ -308,7 +308,7 @@ static int vpif_start_streaming(struct vb2_queue *vq, unsigned int count)
> >> >> > channel2_intr_assert();
> >> >> > channel2_intr_enable(1);
> >> >> > enable_channel2(1);
> >> >> > - if (vpif_config_data->ch2_clip_en)
> >> >> > + if (vpif_config_data->chan_config[VPIF_CHANNEL2_VIDEO].clip_en)
> >> >> > channel2_clipping_enable(1);
> >> >> > }
> >> >> >
> >> >> > @@ -317,7 +317,7 @@ static int vpif_start_streaming(struct vb2_queue *vq, unsigned int count)
> >> >> > channel3_intr_assert();
> >> >> > channel3_intr_enable(1);
> >> >> > enable_channel3(1);
> >> >> > - if (vpif_config_data->ch3_clip_en)
> >> >> > + if (vpif_config_data->chan_config[VPIF_CHANNEL3_VIDEO].clip_en)
> >> >> > channel3_clipping_enable(1);
> >> >> > }
> >> >> >
> >> >> > @@ -1174,14 +1174,16 @@ 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)
> >> >> > + if (vpif_config_data->
> >> >> > + chan_config[VPIF_CHANNEL2_VIDEO].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)
> >> >> > + if (vpif_config_data->
> >> >> > + chan_config[VPIF_CHANNEL3_VIDEO].clip_en)
> >> >> > channel3_clipping_enable(0);
> >> >> > enable_channel3(0);
> >> >> > channel3_intr_enable(0);
> >> >> > @@ -1214,41 +1216,118 @@ static int vpif_enum_output(struct file *file, void *fh,
> >> >> > {
> >> >> >
> >> >> > struct vpif_display_config *config = vpif_dev->platform_data;
> >> >> > + struct vpif_display_chan_config *chan_cfg;
> >> >> > + struct vpif_fh *vpif_handler = fh;
> >> >> > + struct channel_obj *ch = vpif_handler->channel;
> >> >> >
> >> >> > - if (output->index >= config->output_count) {
> >> >> > + chan_cfg = &config->chan_config[ch->channel_id];
> >> >> > + if (output->index >= chan_cfg->output_count) {
> >> >> > vpif_dbg(1, debug, "Invalid output index\n");
> >> >> > return -EINVAL;
> >> >> > }
> >> >> >
> >> >> > - strcpy(output->name, config->output[output->index]);
> >> >> > - output->type = V4L2_OUTPUT_TYPE_ANALOG;
> >> >> > + memcpy(output, &chan_cfg->outputs[output->index].output,
> >> >> > + sizeof(*output));
> >> >
> >> > *output = chan_cfg->outputs[output->index].output;
> >> >
> >> >> > output->std = VPIF_V4L2_STD;
> >> >
> >> Should I move 'VPIF_V4L2_STD' this macro to vpif_types.h ?
> >
> > No. VPIF_V4L2_STD == V4L2_STD_ALL, so just use V4L2_STD_ALL directly.
> >
> Ok.
>
> >> > This should be part of chan_cfg->outputs[output->index].output and not be
> >> > overridden. If the output is a HDTV output, then std should be 0, otherwise
> >> > it should be whatever the std is that is supported by the output.
> >> >
> >> > Really no different to what we do in vpif_capture.
> >> >
> >> Ok
> >>
> >> >> >
> >> >> > return 0;
> >> >> > }
> >> >> >
> >> >> > +/**
> >> >> > + * vpif_output_to_subdev() - Maps output to sub device
> >> >> > + * @vpif_cfg - global config ptr
> >> >> > + * @chan_cfg - channel config ptr
> >> >> > + * @index - Given output index from application
> >> >> > + *
> >> >> > + * lookup the sub device information for a given output index.
> >> >> > + * we report all the output to application. output table also
> >> >> > + * has sub device name for the each output
> >> >> > + */
> >> >> > +static int
> >> >> > +vpif_output_to_subdev(struct vpif_display_config *vpif_cfg,
> >> >> > + struct vpif_display_chan_config *chan_cfg, int index)
> >> >> > +{
> >> >> > + struct vpif_subdev_info *subdev_info;
> >> >> > + const char *subdev_name;
> >> >> > + int i;
> >> >> > +
> >> >> > + vpif_dbg(2, debug, "vpif_output_to_subdev\n");
> >> >> > +
> >> >> > + if (chan_cfg->outputs == NULL)
> >> >> > + return -1;
> >> >> > +
> >> >> > + subdev_name = chan_cfg->outputs[index].subdev_name;
> >> >> > + if (subdev_name == NULL)
> >> >> > + return -1;
> >> >> > +
> >> >> > + /* loop through the sub device list to get the sub device info */
> >> >> > + for (i = 0; i < vpif_cfg->subdev_count; i++) {
> >> >> > + subdev_info = &vpif_cfg->subdevinfo[i];
> >> >> > + if (!strcmp(subdev_info->name, subdev_name))
> >> >> > + return i;
> >> >> > + }
> >> >> > + return -1;
> >> >> > +}
> >> >> > +
> >> >> > +/**
> >> >> > + * vpif_set_output() - Select an output
> >> >> > + * @vpif_cfg - global config ptr
> >> >> > + * @ch - channel
> >> >> > + * @index - Given output index from application
> >> >> > + *
> >> >> > + * Select the given output.
> >> >> > + */
> >> >> > +static int vpif_set_output(struct vpif_display_config *vpif_cfg,
> >> >> > + struct channel_obj *ch, int index)
> >> >> > +{
> >> >> > + struct vpif_display_chan_config *chan_cfg =
> >> >> > + &vpif_cfg->chan_config[ch->channel_id];
> >> >> > + struct vpif_subdev_info *subdev_info = NULL;
> >> >> > + struct v4l2_subdev *sd = NULL;
> >> >> > + u32 input = 0, output = 0;
> >> >> > + int sd_index;
> >> >> > + int ret;
> >> >> > +
> >> >> > + sd_index = vpif_output_to_subdev(vpif_cfg, chan_cfg, index);
> >> >> > + if (sd_index >= 0) {
> >> >> > + sd = vpif_obj.sd[sd_index];
> >> >> > + subdev_info = &vpif_cfg->subdevinfo[sd_index];
> >> >> > + }
> >> >> > +
> >> >> > + if (sd) {
> >> >> > + input = chan_cfg->outputs[index].input_route;
> >> >> > + output = chan_cfg->outputs[index].output_route;
> >> >> > + ret = v4l2_subdev_call(sd, video, s_routing, input, output, 0);
> >> >> > + if (ret < 0) {
> >> >
> >> > Replace with:
> >> >
> >> > if (ret < 0 && ret != -ENOIOCTLCMD) {
> >> >
> >> > It's OK if the subdev doesn't support this s_routing operation.
> >> >
> >> Ok
> >>
> >> >> > + vpif_err("Failed to set output\n");
> >> >> > + return ret;
> >> >> > + }
> >> >> > +
> >> >> > + }
> >> >> > + ch->output_idx = index;
> >> >> > + ch->sd = sd;
> >> >
> >> > Just like in the capture case you should update tvnorms:
> >> >
> >> > ch->video_dev->tvnorms = chan_cfg->inputs[index].output.std;
> >> >
> >> > Make sure tvnorms is no longer set in vpif_video_template.
> >> > Ditto for current_norm. Since vpif_display.c supports g_std the current_norm
> >> > field shouldn't be used anymore.
> >> >
> >> > The capture case also updates vpifparams.iface:
> >> >
> >> > ch->vpifparams.iface = chan_cfg->vpif_if;
> >> >
> >> > Isn't that needed for output as well?
> >> >
> >> The iface is never used in display, so there isnt any necessity to assign it.
> >
> > But it is used in vpif.c through vpif_set_video_params()!
> >
> Agreed iface is used in vpif_set_video_params(), but it still for capture.
> there is check 'if (channel_id > 1)' in the function while accessing the iface
> variable, so for display this will be always true, so no point in assigning it.
True, I missed that. In that case, shouldn't the vpif_if field be removed from
struct vpif_display_chan_config?
Regards,
Hans
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2] media: davinci: vpif: display: separate out subdev from output
2012-09-25 6:29 ` Hans Verkuil
@ 2012-09-25 6:34 ` Prabhakar Lad
0 siblings, 0 replies; 9+ messages in thread
From: Prabhakar Lad @ 2012-09-25 6:34 UTC (permalink / raw)
To: Hans Verkuil
Cc: LMML, DLOS, LAK, Sekhar Nori, Mauro Carvalho Chehab, VGER, Lad,
Prabhakar, Manjunath Hadli, Hans Verkuil
Hi Hans,
On Tue, Sep 25, 2012 at 11:59 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> On Tue September 25 2012 07:38:12 Prabhakar Lad wrote:
>> Hi Hans,
>>
>> On Mon, Sep 24, 2012 at 7:02 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>> > On Mon September 24 2012 15:21:44 Prabhakar Lad wrote:
>> >> Hi Hans,
>> >>
>> >> On Mon, Sep 24, 2012 at 5:20 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>> >> > On Mon September 24 2012 12:59:11 Hans Verkuil wrote:
>> >> >> On Mon September 24 2012 12:44:11 Prabhakar wrote:
>> >> >> > From: Lad, Prabhakar <prabhakar.lad@ti.com>
>> >> >> >
>> >> >> > vpif_display relied on a 1-1 mapping of output and subdev. This is not
>> >> >> > necessarily the case. Separate the two. So there is a list of subdevs
>> >> >> > and a list of outputs. Each output refers to a subdev and has routing
>> >> >> > information. An output does not have to have a subdev.
>> >> >> >
>> >> >> > The initial output for each channel is set to the fist output.
>> >> >> >
>> >> >> > Currently missing is support for associating multiple subdevs with
>> >> >> > an output.
>> >> >> >
>> >> >> > Signed-off-by: Lad, Prabhakar <prabhakar.lad@ti.com>
>> >> >> > Signed-off-by: Manjunath Hadli <manjunath.hadli@ti.com>
>> >> >> > Cc: Hans Verkuil <hans.verkuil@cisco.com>
>> >> >> > Cc: Sekhar Nori <nsekhar@ti.com>
>> >> >>
>> >> >> Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
>> >> >
>> >> > I'm retracting this Ack. I did see something that wasn't right but I thought
>> >> > it was harmless, but after thinking some more I believe it should be fixed.
>> >> > Luckily, it's easy to fix. See below. Since we need a new version anyway I
>> >> > also added a comment to a few minor issues that can be fixed at the same time.
>> >> >
>> >> >>
>> >> >> Regards,
>> >> >>
>> >> >> Hans
>> >> >>
>> >> >> > ---
>> >> >> > This patch is dependent on the patch series from Hans
>> >> >> > (http://www.mail-archive.com/linux-media@vger.kernel.org/msg52270.html)
>> >> >> >
>> >> >> > Changes for V2:
>> >> >> > 1: Changed v4l2_device_call_until_err() call to v4l2_subdev_call() for
>> >> >> > s_routing, since this call is for specific subdev, pointed out by Hans.
>> >> >> >
>> >> >> > arch/arm/mach-davinci/board-da850-evm.c | 29 +++++-
>> >> >> > arch/arm/mach-davinci/board-dm646x-evm.c | 39 ++++++-
>> >> >> > drivers/media/platform/davinci/vpif_display.c | 136 ++++++++++++++++++++-----
>> >> >> > include/media/davinci/vpif_types.h | 20 +++-
>> >> >> > 4 files changed, 183 insertions(+), 41 deletions(-)
>> >> >> >
>> >> >> > diff --git a/arch/arm/mach-davinci/board-da850-evm.c b/arch/arm/mach-davinci/board-da850-evm.c
>> >> >> > index 3081ea4..23a7012 100644
>> >> >> > --- a/arch/arm/mach-davinci/board-da850-evm.c
>> >> >> > +++ b/arch/arm/mach-davinci/board-da850-evm.c
>> >> >> > @@ -46,6 +46,7 @@
>> >> >> > #include <mach/spi.h>
>> >> >> >
>> >> >> > #include <media/tvp514x.h>
>> >> >> > +#include <media/adv7343.h>
>> >> >> >
>> >> >> > #define DA850_EVM_PHY_ID "davinci_mdio-0:00"
>> >> >> > #define DA850_LCD_PWR_PIN GPIO_TO_PIN(2, 8)
>> >> >> > @@ -1257,16 +1258,34 @@ static struct vpif_subdev_info da850_vpif_subdev[] = {
>> >> >> > },
>> >> >> > };
>> >> >> >
>> >> >> > -static const char const *vpif_output[] = {
>> >> >> > - "Composite",
>> >> >> > - "S-Video",
>> >> >> > +static const struct vpif_output da850_ch0_outputs[] = {
>> >> >> > + {
>> >> >> > + .output = {
>> >> >> > + .index = 0,
>> >> >> > + .name = "Composite",
>> >> >> > + .type = V4L2_OUTPUT_TYPE_ANALOG,
>> >> >> > + },
>> >> >> > + .subdev_name = "adv7343",
>> >> >> > + .output_route = ADV7343_COMPOSITE_ID,
>> >> >> > + },
>> >> >> > + {
>> >> >> > + .output = {
>> >> >> > + .index = 1,
>> >> >> > + .name = "S-Video",
>> >> >> > + .type = V4L2_OUTPUT_TYPE_ANALOG,
>> >> >> > + },
>> >> >> > + .subdev_name = "adv7343",
>> >> >> > + .output_route = ADV7343_SVIDEO_ID,
>> >> >> > + },
>> >> >> > };
>> >> >> >
>> >> >> > static struct vpif_display_config da850_vpif_display_config = {
>> >> >> > .subdevinfo = da850_vpif_subdev,
>> >> >> > .subdev_count = ARRAY_SIZE(da850_vpif_subdev),
>> >> >> > - .output = vpif_output,
>> >> >> > - .output_count = ARRAY_SIZE(vpif_output),
>> >> >> > + .chan_config[0] = {
>> >> >> > + .outputs = da850_ch0_outputs,
>> >> >> > + .output_count = ARRAY_SIZE(da850_ch0_outputs),
>> >> >> > + },
>> >> >> > .card_name = "DA850/OMAP-L138 Video Display",
>> >> >> > };
>> >> >> >
>> >> >> > diff --git a/arch/arm/mach-davinci/board-dm646x-evm.c b/arch/arm/mach-davinci/board-dm646x-evm.c
>> >> >> > index ad249c7..c206768 100644
>> >> >> > --- a/arch/arm/mach-davinci/board-dm646x-evm.c
>> >> >> > +++ b/arch/arm/mach-davinci/board-dm646x-evm.c
>> >> >> > @@ -26,6 +26,7 @@
>> >> >> > #include <linux/i2c/pcf857x.h>
>> >> >> >
>> >> >> > #include <media/tvp514x.h>
>> >> >> > +#include <media/adv7343.h>
>> >> >> >
>> >> >> > #include <linux/mtd/mtd.h>
>> >> >> > #include <linux/mtd/nand.h>
>> >> >> > @@ -496,18 +497,44 @@ static struct vpif_subdev_info dm646x_vpif_subdev[] = {
>> >> >> > },
>> >> >> > };
>> >> >> >
>> >> >> > -static const char *output[] = {
>> >> >> > - "Composite",
>> >> >> > - "Component",
>> >> >> > - "S-Video",
>> >> >> > +static const struct vpif_output dm6467_ch0_outputs[] = {
>> >> >> > + {
>> >> >> > + .output = {
>> >> >> > + .index = 0,
>> >> >> > + .name = "Composite",
>> >> >> > + .type = V4L2_OUTPUT_TYPE_ANALOG,
>> >> >> > + },
>> >> >> > + .subdev_name = "adv7343",
>> >> >> > + .output_route = ADV7343_COMPOSITE_ID,
>> >> >> > + },
>> >> >> > + {
>> >> >> > + .output = {
>> >> >> > + .index = 1,
>> >> >> > + .name = "Component",
>> >> >> > + .type = V4L2_OUTPUT_TYPE_ANALOG,
>> >> >> > + },
>> >> >> > + .subdev_name = "adv7343",
>> >> >> > + .output_route = ADV7343_COMPONENT_ID,
>> >> >> > + },
>> >> >> > + {
>> >> >> > + .output = {
>> >> >> > + .index = 2,
>> >> >> > + .name = "S-Video",
>> >> >> > + .type = V4L2_OUTPUT_TYPE_ANALOG,
>> >> >> > + },
>> >> >> > + .subdev_name = "adv7343",
>> >> >> > + .output_route = ADV7343_SVIDEO_ID,
>> >> >> > + },
>> >> >> > };
>> >> >> >
>> >> >> > static struct vpif_display_config dm646x_vpif_display_config = {
>> >> >> > .set_clock = set_vpif_clock,
>> >> >> > .subdevinfo = dm646x_vpif_subdev,
>> >> >> > .subdev_count = ARRAY_SIZE(dm646x_vpif_subdev),
>> >> >> > - .output = output,
>> >> >> > - .output_count = ARRAY_SIZE(output),
>> >> >> > + .chan_config[0] = {
>> >> >> > + .outputs = dm6467_ch0_outputs,
>> >> >> > + .output_count = ARRAY_SIZE(dm6467_ch0_outputs),
>> >> >> > + },
>> >> >> > .card_name = "DM646x EVM",
>> >> >> > };
>> >> >> >
>> >> >> > diff --git a/drivers/media/platform/davinci/vpif_display.c b/drivers/media/platform/davinci/vpif_display.c
>> >> >> > index 8d1ce09..f68780f 100644
>> >> >> > --- a/drivers/media/platform/davinci/vpif_display.c
>> >> >> > +++ b/drivers/media/platform/davinci/vpif_display.c
>> >> >> > @@ -308,7 +308,7 @@ static int vpif_start_streaming(struct vb2_queue *vq, unsigned int count)
>> >> >> > channel2_intr_assert();
>> >> >> > channel2_intr_enable(1);
>> >> >> > enable_channel2(1);
>> >> >> > - if (vpif_config_data->ch2_clip_en)
>> >> >> > + if (vpif_config_data->chan_config[VPIF_CHANNEL2_VIDEO].clip_en)
>> >> >> > channel2_clipping_enable(1);
>> >> >> > }
>> >> >> >
>> >> >> > @@ -317,7 +317,7 @@ static int vpif_start_streaming(struct vb2_queue *vq, unsigned int count)
>> >> >> > channel3_intr_assert();
>> >> >> > channel3_intr_enable(1);
>> >> >> > enable_channel3(1);
>> >> >> > - if (vpif_config_data->ch3_clip_en)
>> >> >> > + if (vpif_config_data->chan_config[VPIF_CHANNEL3_VIDEO].clip_en)
>> >> >> > channel3_clipping_enable(1);
>> >> >> > }
>> >> >> >
>> >> >> > @@ -1174,14 +1174,16 @@ 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)
>> >> >> > + if (vpif_config_data->
>> >> >> > + chan_config[VPIF_CHANNEL2_VIDEO].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)
>> >> >> > + if (vpif_config_data->
>> >> >> > + chan_config[VPIF_CHANNEL3_VIDEO].clip_en)
>> >> >> > channel3_clipping_enable(0);
>> >> >> > enable_channel3(0);
>> >> >> > channel3_intr_enable(0);
>> >> >> > @@ -1214,41 +1216,118 @@ static int vpif_enum_output(struct file *file, void *fh,
>> >> >> > {
>> >> >> >
>> >> >> > struct vpif_display_config *config = vpif_dev->platform_data;
>> >> >> > + struct vpif_display_chan_config *chan_cfg;
>> >> >> > + struct vpif_fh *vpif_handler = fh;
>> >> >> > + struct channel_obj *ch = vpif_handler->channel;
>> >> >> >
>> >> >> > - if (output->index >= config->output_count) {
>> >> >> > + chan_cfg = &config->chan_config[ch->channel_id];
>> >> >> > + if (output->index >= chan_cfg->output_count) {
>> >> >> > vpif_dbg(1, debug, "Invalid output index\n");
>> >> >> > return -EINVAL;
>> >> >> > }
>> >> >> >
>> >> >> > - strcpy(output->name, config->output[output->index]);
>> >> >> > - output->type = V4L2_OUTPUT_TYPE_ANALOG;
>> >> >> > + memcpy(output, &chan_cfg->outputs[output->index].output,
>> >> >> > + sizeof(*output));
>> >> >
>> >> > *output = chan_cfg->outputs[output->index].output;
>> >> >
>> >> >> > output->std = VPIF_V4L2_STD;
>> >> >
>> >> Should I move 'VPIF_V4L2_STD' this macro to vpif_types.h ?
>> >
>> > No. VPIF_V4L2_STD == V4L2_STD_ALL, so just use V4L2_STD_ALL directly.
>> >
>> Ok.
>>
>> >> > This should be part of chan_cfg->outputs[output->index].output and not be
>> >> > overridden. If the output is a HDTV output, then std should be 0, otherwise
>> >> > it should be whatever the std is that is supported by the output.
>> >> >
>> >> > Really no different to what we do in vpif_capture.
>> >> >
>> >> Ok
>> >>
>> >> >> >
>> >> >> > return 0;
>> >> >> > }
>> >> >> >
>> >> >> > +/**
>> >> >> > + * vpif_output_to_subdev() - Maps output to sub device
>> >> >> > + * @vpif_cfg - global config ptr
>> >> >> > + * @chan_cfg - channel config ptr
>> >> >> > + * @index - Given output index from application
>> >> >> > + *
>> >> >> > + * lookup the sub device information for a given output index.
>> >> >> > + * we report all the output to application. output table also
>> >> >> > + * has sub device name for the each output
>> >> >> > + */
>> >> >> > +static int
>> >> >> > +vpif_output_to_subdev(struct vpif_display_config *vpif_cfg,
>> >> >> > + struct vpif_display_chan_config *chan_cfg, int index)
>> >> >> > +{
>> >> >> > + struct vpif_subdev_info *subdev_info;
>> >> >> > + const char *subdev_name;
>> >> >> > + int i;
>> >> >> > +
>> >> >> > + vpif_dbg(2, debug, "vpif_output_to_subdev\n");
>> >> >> > +
>> >> >> > + if (chan_cfg->outputs == NULL)
>> >> >> > + return -1;
>> >> >> > +
>> >> >> > + subdev_name = chan_cfg->outputs[index].subdev_name;
>> >> >> > + if (subdev_name == NULL)
>> >> >> > + return -1;
>> >> >> > +
>> >> >> > + /* loop through the sub device list to get the sub device info */
>> >> >> > + for (i = 0; i < vpif_cfg->subdev_count; i++) {
>> >> >> > + subdev_info = &vpif_cfg->subdevinfo[i];
>> >> >> > + if (!strcmp(subdev_info->name, subdev_name))
>> >> >> > + return i;
>> >> >> > + }
>> >> >> > + return -1;
>> >> >> > +}
>> >> >> > +
>> >> >> > +/**
>> >> >> > + * vpif_set_output() - Select an output
>> >> >> > + * @vpif_cfg - global config ptr
>> >> >> > + * @ch - channel
>> >> >> > + * @index - Given output index from application
>> >> >> > + *
>> >> >> > + * Select the given output.
>> >> >> > + */
>> >> >> > +static int vpif_set_output(struct vpif_display_config *vpif_cfg,
>> >> >> > + struct channel_obj *ch, int index)
>> >> >> > +{
>> >> >> > + struct vpif_display_chan_config *chan_cfg =
>> >> >> > + &vpif_cfg->chan_config[ch->channel_id];
>> >> >> > + struct vpif_subdev_info *subdev_info = NULL;
>> >> >> > + struct v4l2_subdev *sd = NULL;
>> >> >> > + u32 input = 0, output = 0;
>> >> >> > + int sd_index;
>> >> >> > + int ret;
>> >> >> > +
>> >> >> > + sd_index = vpif_output_to_subdev(vpif_cfg, chan_cfg, index);
>> >> >> > + if (sd_index >= 0) {
>> >> >> > + sd = vpif_obj.sd[sd_index];
>> >> >> > + subdev_info = &vpif_cfg->subdevinfo[sd_index];
>> >> >> > + }
>> >> >> > +
>> >> >> > + if (sd) {
>> >> >> > + input = chan_cfg->outputs[index].input_route;
>> >> >> > + output = chan_cfg->outputs[index].output_route;
>> >> >> > + ret = v4l2_subdev_call(sd, video, s_routing, input, output, 0);
>> >> >> > + if (ret < 0) {
>> >> >
>> >> > Replace with:
>> >> >
>> >> > if (ret < 0 && ret != -ENOIOCTLCMD) {
>> >> >
>> >> > It's OK if the subdev doesn't support this s_routing operation.
>> >> >
>> >> Ok
>> >>
>> >> >> > + vpif_err("Failed to set output\n");
>> >> >> > + return ret;
>> >> >> > + }
>> >> >> > +
>> >> >> > + }
>> >> >> > + ch->output_idx = index;
>> >> >> > + ch->sd = sd;
>> >> >
>> >> > Just like in the capture case you should update tvnorms:
>> >> >
>> >> > ch->video_dev->tvnorms = chan_cfg->inputs[index].output.std;
>> >> >
>> >> > Make sure tvnorms is no longer set in vpif_video_template.
>> >> > Ditto for current_norm. Since vpif_display.c supports g_std the current_norm
>> >> > field shouldn't be used anymore.
>> >> >
>> >> > The capture case also updates vpifparams.iface:
>> >> >
>> >> > ch->vpifparams.iface = chan_cfg->vpif_if;
>> >> >
>> >> > Isn't that needed for output as well?
>> >> >
>> >> The iface is never used in display, so there isnt any necessity to assign it.
>> >
>> > But it is used in vpif.c through vpif_set_video_params()!
>> >
>> Agreed iface is used in vpif_set_video_params(), but it still for capture.
>> there is check 'if (channel_id > 1)' in the function while accessing the iface
>> variable, so for display this will be always true, so no point in assigning it.
>
> True, I missed that. In that case, shouldn't the vpif_if field be removed from
> struct vpif_display_chan_config?
>
Yes it should be removed.
Thanks And Regards,
--Prabhakar Lad
> Regards,
>
> Hans
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2012-09-25 6:34 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-09-24 10:44 [PATCH v2] media: davinci: vpif: display: separate out subdev from output Prabhakar
2012-09-24 10:59 ` Hans Verkuil
2012-09-24 11:50 ` Hans Verkuil
2012-09-24 11:59 ` Hans Verkuil
2012-09-24 13:21 ` Prabhakar Lad
2012-09-24 13:32 ` Hans Verkuil
2012-09-25 5:38 ` Prabhakar Lad
2012-09-25 6:29 ` Hans Verkuil
2012-09-25 6:34 ` Prabhakar Lad
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®