Re: [PATCH 1/10 - v2] vpfe capture bridge driver for DM355 and DM6446
On Wednesday 17 June 2009 17:02:01 Karicheri, Muralidharan wrote: > >> > > > >Can you post your latest proposal for the s_bus op? > > > >I propose a few changes: the name of the struct should be something like > >v4l2_bus_settings, the master/slave bit should be renamed to something > >like 'host_is_master', and we should have two widths: subdev_width and > >host_width. > > > >That way the same structure can be used for both host and subdev, unless > >some of the polarities are inverted. In that case you need to make two > >structs, one for host and one for the subdev. > > > >It is possible to add info on inverters to the struct, but unless > > inverters are used a lot more frequently than I expect I am inclined > > not to do that at this time. > > [MK]Today I am planning to send my v3 version of the vpfe capture patch > and also tvp514x patch since Vaibhav is pre-occupied with some other > activities. I have discussed the changes with Vaibhav for this driver. > > For s_bus, I will try if I can send a patch today. BTW, do you expect me > to add one bool for active high, one for active low etc as done in SoC > camera ? Since I remain opposed to autonegotiation, there is IMO no need for this. Regards, Hans -- Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 1/10 - v2] vpfe capture bridge driver for DM355 and DM6446
>> >> > >Can you post your latest proposal for the s_bus op? > >I propose a few changes: the name of the struct should be something like >v4l2_bus_settings, the master/slave bit should be renamed to something >like 'host_is_master', and we should have two widths: subdev_width and >host_width. > >That way the same structure can be used for both host and subdev, unless >some of the polarities are inverted. In that case you need to make two >structs, one for host and one for the subdev. > >It is possible to add info on inverters to the struct, but unless inverters >are used a lot more frequently than I expect I am inclined not to do that >at this time. > [MK]Today I am planning to send my v3 version of the vpfe capture patch and also tvp514x patch since Vaibhav is pre-occupied with some other activities. I have discussed the changes with Vaibhav for this driver. For s_bus, I will try if I can send a patch today. BTW, do you expect me to add one bool for active high, one for active low etc as done in SoC camera ? Murali -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/10 - v2] vpfe capture bridge driver for DM355 and DM6446
On Tuesday 16 June 2009 16:26:05 Karicheri, Muralidharan wrote: > Hans, > > Thanks for the response. > > > > >Polarities have to be set for each side, that's correct. The other > >parameters are common for both. Although I can think of scenarios where > > the bus width can differ between host and subdev (e.g. subdev sends > > data on 8 pins and the host captures on 10 with the least significant > > two pins pulled low). But that's probably really paranoid of me :-) > > [MK] You are right on width. The MT9T001/31 sensor has 10 bits and > MT9P031 has 12 bits, but on DM355 we the vpfe will take in only 10 bits > and on DM365 it takes 12 bits. But this is applicable only on the host > (vpfe) side though (at least in this case) , not on sub device side. Can you post your latest proposal for the s_bus op? I propose a few changes: the name of the struct should be something like v4l2_bus_settings, the master/slave bit should be renamed to something like 'host_is_master', and we should have two widths: subdev_width and host_width. That way the same structure can be used for both host and subdev, unless some of the polarities are inverted. In that case you need to make two structs, one for host and one for the subdev. It is possible to add info on inverters to the struct, but unless inverters are used a lot more frequently than I expect I am inclined not to do that at this time. > > > >First of all, this isn't a blocking issue at all. This is more a > >nice-to-have. > > > >The reason I mentioned it is because of how we use this (or the dm646x > > to be > >precise) at my work: the dm646x is connected to our FPGA so we had to > > make dummy encoder/decoder drivers to allow it to work with that > > driver. What made that somewhat annoying is that those dummy drivers > > really didn't do anything since the FPGA isn't programmed from the > > linux kernel at all. So we have basically dead code in our kernel just > > to satisfy a dm646x driver requirement. > > > >And you are right: a subdev is bus independent, so it is perfectly > > possible to make a dummy subdev instead. The key phrase was really > > 'doesn't require any setup'. It would be nice to be able to use this > > driver 'standalone' without requiring a subdev. Something to think > > about. > > > >And apologies for my poor review comment, that was phrased rather badly. > > [MK] This is the first version of the driver and I assure you that there > are opportunities to improve the driver as we add more features. Since > many of the other activities like adding camera interface, supporting > resizer/previewer are dependent on this, it is important for us to get > this to the kernel tree as quickly as possible. So I would prefer to keep > it as is now and change it part of later patches. If this can go in > 2.6.31, that will be really great. It's no problem to keep this as is. Regards, Hans -- Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 1/10 - v2] vpfe capture bridge driver for DM355 and DM6446
Hans, Thanks for the response. >Polarities have to be set for each side, that's correct. The other >parameters are common for both. Although I can think of scenarios where the >bus width can differ between host and subdev (e.g. subdev sends data on 8 >pins and the host captures on 10 with the least significant two pins pulled >low). But that's probably really paranoid of me :-) [MK] You are right on width. The MT9T001/31 sensor has 10 bits and MT9P031 has 12 bits, but on DM355 we the vpfe will take in only 10 bits and on DM365 it takes 12 bits. But this is applicable only on the host (vpfe) side though (at least in this case) , not on sub device side. >First of all, this isn't a blocking issue at all. This is more a >nice-to-have. > >The reason I mentioned it is because of how we use this (or the dm646x to >be >precise) at my work: the dm646x is connected to our FPGA so we had to make >dummy encoder/decoder drivers to allow it to work with that driver. What >made that somewhat annoying is that those dummy drivers really didn't do >anything since the FPGA isn't programmed from the linux kernel at all. So >we have basically dead code in our kernel just to satisfy a dm646x driver >requirement. > >And you are right: a subdev is bus independent, so it is perfectly possible >to make a dummy subdev instead. The key phrase was really 'doesn't require >any setup'. It would be nice to be able to use this driver 'standalone' >without requiring a subdev. Something to think about. > >And apologies for my poor review comment, that was phrased rather badly. > [MK] This is the first version of the driver and I assure you that there are opportunities to improve the driver as we add more features. Since many of the other activities like adding camera interface, supporting resizer/previewer are dependent on this, it is important for us to get this to the kernel tree as quickly as possible. So I would prefer to keep it as is now and change it part of later patches. If this can go in 2.6.31, that will be really great. >> >> >Don't use these PRIVATE_BASE controls. See also this post regarding >> >the current situation regarding private controls: >> > >> >http://www.mail-archive.com/linux-omap%40vger.kernel.org/msg07999.html >> >> [MK] Looks like it is better to add it to TBD and address it when I add >> camera interface support. > >OK, but please make sure it is revisited. Improving the control handling >code in the v4l2 framework is very high on my TODO list since it is getting >really annoying to implement in drivers. And the inconsistent driver >support isn't helping applications either. > [MK] Sure. I will add it to the TODO list and address it later. >I really hope I'll have time for it in the next few weeks. > >Regards, > > Hans > >-- >Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/10 - v2] vpfe capture bridge driver for DM355 and DM6446
On Monday 15 June 2009 23:47:07 Karicheri, Muralidharan wrote:
> Hans,
>
> I am not clear about some of your comments. Please see below with a [MK]
> prefix.
>
> >> + /* set if client specific interface param is available */
> >> + if (subdev->pdata) {
> >> + /* each client will have different interface requirements */
> >> + if (!strcmp(subdev->name, "tvp5146")) {
> >> + struct tvp514x_platform_data *pdata = subdev->pdata;
> >> +
> >> + if (pdata->hs_polarity)
> >> + vpfe_dev->vpfe_if_params.hdpol =
> >> + VPFE_PINPOL_POSITIVE;
> >> + else
> >> + vpfe_dev->vpfe_if_params.hdpol =
> >> + VPFE_PINPOL_NEGATIVE;
> >> +
> >> + if (pdata->vs_polarity)
> >> + vpfe_dev->vpfe_if_params.vdpol =
> >> + VPFE_PINPOL_POSITIVE;
> >> + else
> >> + vpfe_dev->vpfe_if_params.hdpol =
> >> + VPFE_PINPOL_NEGATIVE;
> >
> >This won't work. Instead this should be data associated with the
> >platform_data.
> >I.e. the platform_data for the dm355/dm6446 contains not only the subdev
> >information, but for each subdev also the information on how to setup
> > the vpfe
> >polarities. You cannot derive that information from what subdevs are
> > used since
> >the board designer might have added e.g. inverters or something like
> > that. Such
> >information can only come from the platform_data.
>
> [MK] I know this code is not correct. But I was waiting for the
> discussion on my bus parameter patch to make this change. Currently
> TVP514x driver that you have reviewed configure output bus based on
> route->output parameter set by s_route(). This doesn't make sense. The
> input param make sense since application can choose between Composite and
> S-Video inputs. There is only one bus going out of the sub device to
> vpfe. So the output selection @ sub device is redundant. I think the
> output is part of as the bus parameter structure I added in the bus
> parameter patch which is under review. It can be read by TVP514x from the
> platform data (using the structure added by my patch) and can be
> overridden by s_bus(). Do you expect the bridge driver and sub devices
> having platform data for bus type (For example, BT.656)? It appears to be
> required only for sub device and bridge driver can configure the ccdc
> based on sub device bus type. But for polarities I need to define them
> for both sides. comments?
Part of the current functionality of s_route will be taken over by s_bus.
But right now this is all we have. Usually the output argument of s_route
is used by audio devices that have multiple output pins. Most video devices
have only one output so it does not normally apply to those. But without an
s_bus function it is (ab)used to set things like bus parameters. One can
argue that those parameters define the output of the chip but it will
definitely be nicer to set this with a proper s_bus call.
Polarities have to be set for each side, that's correct. The other
parameters are common for both. Although I can think of scenarios where the
bus width can differ between host and subdev (e.g. subdev sends data on 8
pins and the host captures on 10 with the least significant two pins pulled
low). But that's probably really paranoid of me :-)
>
> >> + } else {
> >> + v4l2_err(&vpfe_dev->v4l2_dev, "No interface params"
> >> + " defined for subdevice, %d\n", route->output);
> >> + return -EFAULT;
> >> + }
> >> + }
> >> + return ccdc_dev->hw_ops.set_hw_if_params(&vpfe_dev->vpfe_if_params);
> >> +}
> >> +
> >> +/*
> >> +
> >> + struct vpfe_fh *fh;
> >> +
> >> + v4l2_dbg(1, debug, &vpfe_dev->v4l2_dev, "vpfe_open\n");
> >> +
> >> + if (!vpfe_dev->cfg->num_subdevs) {
> >> + v4l2_err(&vpfe_dev->v4l2_dev, "No decoder registered\n");
> >> + return -ENODEV;
> >> + }
> >
> >Why would this be an error? I might have an FPGA connected instead or
> > some other non-i2c device that doesn't require any setup from this
> > driver.
>
> [MK] What you mean by this? Are you saying an FPGA logic will implement
> the decoder hardware? That is quite possible, so also it could be
> non-i2c. But my understanding was that sub device can be anything that
> basically implement the sub device API and need not always be an i2c
> device. So for FPGA or some other bus based device, the bridge device
> doesn't care how the command to change input, detect standard etc are
> communicated by the sub device driver to its hardware. It could be
> writing into some FPGA register or sending a proprietary protocol
> command. Is my understanding correct? In that case each of the above
> (FPGA or non-i2c) is a sub device and at least one sub d
RE: [PATCH 1/10 - v2] vpfe capture bridge driver for DM355 and DM6446
Hans,
I am not clear about some of your comments. Please see below with a [MK] prefix.
>> +static int debug;
>> +static u32 numbuffers = 3;
>> +static u32 bufsize = (720 * 576 * 2);
>> +
>> +module_param(numbuffers, uint, S_IRUGO);
>> +module_param(bufsize, uint, S_IRUGO);
>> +module_param(debug, int, 0644);
>> +
>> +
>> +/* Set interface params based on client interface */
>> +static int vpfe_set_hw_if_params(struct vpfe_device *vpfe_dev)
>> +{
>> +struct vpfe_subdev_info *subdev = vpfe_dev->current_subdev;
>> +struct v4l2_routing *route =
>> +&(subdev->routes[vpfe_dev->current_input]);
>> +
>> +switch (route->output) {
>> +case OUTPUT_10BIT_422_EMBEDDED_SYNC:
>> +vpfe_dev->vpfe_if_params.if_type = VPFE_BT656;
>> +break;
>> +case OUTPUT_20BIT_422_SEPERATE_SYNC:
>> +vpfe_dev->vpfe_if_params.if_type = VPFE_YCBCR_SYNC_16;
>> +break;
>> +case OUTPUT_10BIT_422_SEPERATE_SYNC:
>> +vpfe_dev->vpfe_if_params.if_type = VPFE_YCBCR_SYNC_8;
>> +break;
>> +default:
>> +v4l2_err(&vpfe_dev->v4l2_dev, "decoder output"
>> +" not supported, %d\n", route->output);
>> +return -EINVAL;
>> +}
>> +
>> +/* set if client specific interface param is available */
>> +if (subdev->pdata) {
>> +/* each client will have different interface requirements */
>> +if (!strcmp(subdev->name, "tvp5146")) {
>> +struct tvp514x_platform_data *pdata = subdev->pdata;
>> +
>> +if (pdata->hs_polarity)
>> +vpfe_dev->vpfe_if_params.hdpol =
>> +VPFE_PINPOL_POSITIVE;
>> +else
>> +vpfe_dev->vpfe_if_params.hdpol =
>> +VPFE_PINPOL_NEGATIVE;
>> +
>> +if (pdata->vs_polarity)
>> +vpfe_dev->vpfe_if_params.vdpol =
>> +VPFE_PINPOL_POSITIVE;
>> +else
>> +vpfe_dev->vpfe_if_params.hdpol =
>> +VPFE_PINPOL_NEGATIVE;
>
>This won't work. Instead this should be data associated with the
>platform_data.
>I.e. the platform_data for the dm355/dm6446 contains not only the subdev
>information, but for each subdev also the information on how to setup the
>vpfe
>polarities. You cannot derive that information from what subdevs are used
>since
>the board designer might have added e.g. inverters or something like that.
>Such
>information can only come from the platform_data.
>
[MK] I know this code is not correct. But I was waiting for the discussion on
my bus parameter patch to make this change. Currently TVP514x driver that you
have reviewed configure output bus based on route->output parameter set by
s_route(). This doesn't make sense. The input param make sense since
application can choose between Composite and S-Video inputs. There is only one
bus going out of the sub device to vpfe. So the output selection @ sub device
is redundant. I think the output is part of as the bus parameter structure I
added in the bus parameter patch which is under review. It can be read by
TVP514x from the platform data (using the structure added by my patch) and can
be overridden by s_bus(). Do you expect the bridge driver and sub devices
having platform data for bus type (For example, BT.656)? It appears to be
required only for sub device and bridge driver can configure the ccdc based on
sub device bus type. But for polarities I need to define them for both sides.
comments?
>> +} else {
>> +v4l2_err(&vpfe_dev->v4l2_dev, "No interface params"
>> +" defined for subdevice, %d\n", route->output);
>> +return -EFAULT;
>> +}
>> +}
>> +return ccdc_dev->hw_ops.set_hw_if_params(&vpfe_dev->vpfe_if_params);
>> +}
>> +
>> +/*
>> +
>> +struct vpfe_fh *fh;
>> +
>> +v4l2_dbg(1, debug, &vpfe_dev->v4l2_dev, "vpfe_open\n");
>> +
>> +if (!vpfe_dev->cfg->num_subdevs) {
>> +v4l2_err(&vpfe_dev->v4l2_dev, "No decoder registered\n");
>> +return -ENODEV;
>> +}
>
>Why would this be an error? I might have an FPGA connected instead or some
>other non-i2c device that doesn't require any setup from this driver.
>
[MK] What you mean by this? Are you saying an FPGA logic will implement the
decoder hardware? That is quite possible, so also it could be non-i2c. But my
understanding was that sub device can be anything that basically implement the
sub device API and need not always be an i2c device. So for FPGA or some other
bus based device, the bridge device doesn't care how the command to change
input, detect standard etc are communicated by the sub device driver to its
hardware. It could be writing into some FPGA register or sending
RE: [PATCH 1/10 - v2] vpfe capture bridge driver for DM355 and DM6446
>> + /* set the default image parameters in the device */ >> + ret = vpfe_config_image_format(vpfe_dev, >> + &vpfe_standards[vpfe_dev- >>std_index].std_id); >> + if (ret) >> + goto unlock_out; > >Why you check ret value and go to label below? >Probably you can remove if-check and goto here. > Ok. >> + >> +unlock_out: >> + mutex_unlock(&vpfe_dev->lock); ; > >return -EIO? > Ok. >-- >Best regards, Klimov Alexey -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/10 - v2] vpfe capture bridge driver for DM355 and DM6446
Hello,
Very small suggestion, please see below.
On Thu, Jun 11, 2009 at 9:00 PM, wrote:
> From: Muralidharan Karicheri
>
> Re-sending since previous one missed a file (vpfe_types.h)
>
> VPFE Capture bridge driver
>
> This is version, v2 of vpfe capture bridge driver for doing video
> capture on DM355 and DM6446 evms. The ccdc hw modules register with the
> driver and are used for configuring the CCD Controller for a specific
> decoder interface. The driver also registers the sub devices required
> for a specific evm. More than one sub devices can be registered.
> This allows driver to switch dynamically to capture video from
> any sub device that is registered. Currently only one sub device
> (tvp5146) is supported. But in future this driver is expected
> to do capture from sensor devices such as Micron's MT9T001,MT9T031
> and MT9P031 etc. The driver currently supports MMAP based IO.
>
> Following are the updates based on review comments:-
> 1) minor number is allocated dynamically
> 2) updates to QUERYCAP handling
> 3) eliminated intermediate vpfe pixel format
> 4) refactored few functions
> 5) reworked isr routines for reducing indentation
> 6) reworked vpfe_check_format and added a documentation
> for algorithm
> 7) fixed memory leak in probe()
>
> TODO list :
> 1) load sub device from bridge driver. Hans has enhanced
> the v4l2-subdevice framework to do this. Will be updated
> soon to pick this.
>
>
> Reviewed By "Hans Verkuil".
> Reviewed By "Laurent Pinchart".
>
> Signed-off-by: Muralidharan Karicheri
> ---
> Applies to v4l-dvb repository
>
> drivers/media/video/davinci/vpfe_capture.c | 2252
>
> include/media/davinci/vpfe_capture.h | 183 +++
> include/media/davinci/vpfe_types.h | 51 +
> 3 files changed, 2486 insertions(+), 0 deletions(-)
> create mode 100644 drivers/media/video/davinci/vpfe_capture.c
> create mode 100644 include/media/davinci/vpfe_capture.h
> create mode 100644 include/media/davinci/vpfe_types.h
> +/* vpfe capture driver file operations */
> +static struct v4l2_file_operations vpfe_fops = {
> + .owner = THIS_MODULE,
> + .open = vpfe_open,
> + .release = vpfe_release,
> + .ioctl = video_ioctl2,
> + .mmap = vpfe_mmap,
> + .poll = vpfe_poll
> +};
> +
> +/*
> + * vpfe_check_format()
> + * This function adjust the input pixel format as per hardware
> + * capabilities and update the same in pixfmt.
> + * Following algorithm used :-
> + *
> + * If given pixformat is not in the vpfe list of pix formats or not
> + * supported by the hardware, current value of pixformat in the device
> + * is used
> + * If given field is not supported, then current field is used. If field
> + * is different from current, then it is matched with that from sub
> device.
> + * Minimum height is 2 lines for interlaced or tb field and 1 line for
> + * progressive. Maximum height is clamped to active active lines of scan
> + * Minimum width is 32 bytes in memory and width is clamped to active
> + * pixels of scan.
> + * bytesperline is a multiple of 32.
> + */
> +static const struct vpfe_pixel_format *
> + vpfe_check_format(struct vpfe_device *vpfe_dev,
> + struct v4l2_pix_format *pixfmt)
> +{
> + u32 min_height = 1, min_width = 32, max_width, max_height;
> + const struct vpfe_pixel_format *vpfe_pix_fmt;
> + u32 pix;
> + int temp, found;
> +
> + vpfe_pix_fmt = vpfe_lookup_pix_format(pixfmt->pixelformat);
> + if (NULL == vpfe_pix_fmt) {
> + /*
> + * use current pixel format in the vpfe device. We
> + * will find this pix format in the table
> + */
> + pixfmt->pixelformat = vpfe_dev->fmt.fmt.pix.pixelformat;
> + vpfe_pix_fmt = vpfe_lookup_pix_format(pixfmt->pixelformat);
> + }
> +
> + /* check if hw supports it */
> + temp = 0;
> + found = 0;
> + while (ccdc_dev->hw_ops.enum_pix(&pix, temp) >= 0) {
> + if (vpfe_pix_fmt->fmtdesc.pixelformat == pix) {
> + found = 1;
> + break;
> + }
> + temp++;
> + }
> +
> + if (!found) {
> + /* use current pixel format */
> + pixfmt->pixelformat = vpfe_dev->fmt.fmt.pix.pixelformat;
> + /*
> + * Since this is currently used in the vpfe device, we
> + * will find this pix format in the table
> + */
> + vpfe_pix_fmt = vpfe_lookup_pix_format(pixfmt->pixelformat);
> + }
> +
> + /* check what field format is supported */
> + if (pixfmt->field == V4L2_FIELD_ANY) {
> + /* if field is any, use current value as default */
> + pixfmt->field = vpfe_dev->fmt.fmt.
