> Hans,
> Thanks for the comments. Only one comment from me. Rest everything I have
> taken care.
>
> Thanks and Regards,
> -Manju
>
> On Tue, Dec 07, 2010 at 02:13:45, Hans Verkuil wrote:
>> Comments below...
>>
>> On Thursday, December 02, 2010 13:38:36 Manjunath Hadli wrote:
>> > This patch implements the coe functionality of the dislay driver,
>> > mainly controlling the VENC and other encoders, and acting as the one
>> > point interface for the man V4L2 driver.This implements the cre of
>> > each of the V4L2 IOCTLs.
>> >
>> > Signed-off-by: Manjunath Hadli <manjunath.hadli@xxxxxx>
>> > Signed-off-by: Muralidharan Karicheri <m-karicheri2@xxxxxx>
>> > ---
>> > drivers/media/video/davinci/vpbe.c | 847
>> ++++++++++++++++++++++++++++++++++++
>> > include/media/davinci/vpbe.h | 186 ++++++++
>> > 2 files changed, 1033 insertions(+), 0 deletions(-) create mode
>> > 100644 drivers/media/video/davinci/vpbe.c
>> > create mode 100644 include/media/davinci/vpbe.h
>> >
>> > diff --git a/drivers/media/video/davinci/vpbe.c
>> > b/drivers/media/video/davinci/vpbe.c
>> > new file mode 100644
>> > index 0000000..96c0eea
>> > --- /dev/null
>> > +++ b/drivers/media/video/davinci/vpbe.c
>> > @@ -0,0 +1,847 @@
>> > +/*
>> > + * Copyright (C) 2010 Texas Instruments Inc
>> > + *
>> > + * This program is free software; you can redistribute it and/or
>> > +modify
>> > + * it under the terms of the GNU General Public License as published
>> > +by
>> > + * the Free Software Foundation; either version 2 of the License.
>> > + *
>> > + * This program is distributed in the hope that it will be useful,
>> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> > + * GNU General Public License for more details.
>> > + *
>> > + * You should have received a copy of the GNU General Public License
>> > + * along with this program; if not, write to the Free Software
>> > + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA
>> > +02111-1307 USA */ #include <linux/kernel.h> #include <linux/init.h>
>> > +#include <linux/module.h> #include <linux/errno.h> #include
>> > +<linux/fs.h> #include <linux/string.h> #include <linux/wait.h>
>> > +#include <linux/time.h> #include <linux/platform_device.h> #include
>> > +<linux/io.h> #include <linux/slab.h> #include <linux/clk.h> #include
>> > +<linux/err.h>
>> > +
>> > +#include <media/v4l2-device.h>
>> > +#include <media/davinci/vpbe_types.h> #include <media/davinci/vpbe.h>
>> > +#include <media/davinci/vpss.h>
>> > +
>> > +
>> > +#define VPBE_DEFAULT_OUTPUT "Composite"
>> > +#define VPBE_DEFAULT_MODE "ntsc"
>> > +
>> > +static char *def_output = VPBE_DEFAULT_OUTPUT; static char *def_mode
>> > += VPBE_DEFAULT_MODE; static struct osd_state *osd_device; static
>> > +struct venc_platform_data *venc_device; static int debug;
>> > +
>> > +module_param(def_output, charp, S_IRUGO); module_param(def_mode,
>> > +charp, S_IRUGO); module_param(debug, int, 0644);
>> > +
>> > +MODULE_PARM_DESC(def_output, "vpbe output name (default:Composite)");
>> > +MODULE_PARM_DESC(ef_mode, "vpbe output mode name (default:ntsc");
>> > +MODULE_PARM_DESC(debug, "Debug level 0-1");
>> > +
>> > +MODULE_DESCRIPTION("TI DMXXX VPBE Display controller");
>> > +MODULE_LICENSE("GPL"); MODULE_AUTHOR("Texas Instruments");
>> > +
>> > +/**
>> > + * vpbe_current_encoder_info - Get config info for current encoder
>> > + * @vpbe_dev - vpbe device ptr
>> > + *
>> > + * Return ptr to current encoder config info */ static struct
>> > +encoder_config_info* vpbe_current_encoder_info(struct vpbe_device
>> > +*vpbe_dev) {
>> > + struct vpbe_display_config *vpbe_config = vpbe_dev->cfg;
>> > + int index = vpbe_dev->current_sd_index;
>> > + return ((index == 0) ? &vpbe_config->venc :
>> > + &vpbe_config->ext_encoders[index-1]);
>> > +}
>> > +
>> > +/**
>> > + * vpbe_find_encoder_sd_index - Given a name find encoder sd index
>> > + *
>> > + * @vpbe_config - ptr to vpbe cfg
>> > + * @output_index - index used by application
>> > + *
>> > + * Return sd index of the encoder
>> > + */
>> > +static int vpbe_find_encoder_sd_index(struct vpbe_display_config
>> *vpbe_config,
>> > + int index)
>> > +{
>> > + char *encoder_name = vpbe_config->outputs[index].subdev_name;
>> > + int i;
>> > +
>> > + /* Venc is always first */
>> > + if (!strcmp(encoder_name, vpbe_config->venc.module_name))
>> > + return 0;
>> > +
>> > + for (i = 0; i < vpbe_config->num_ext_encoders; i++) {
>> > + if (!strcmp(encoder_name,
>> > + vpbe_config->ext_encoders[i].module_name))
>> > + return i+1;
>> > + }
>> > + return -EINVAL;
>> > +}
>> > +
>> > +/**
>> > + * vpbe_g_cropcap - Get crop capabilities of the display
>> > + * @vpbe_dev - vpbe device ptr
>> > + * @cropcap - cropcap is a ptr to struct v4l2_cropcap
>> > + *
>> > + * Update the crop capabilities in crop cap for current
>> > + * mode
>> > + */
>> > +static int vpbe_g_cropcap(struct vpbe_device *vpbe_dev,
>> > + struct v4l2_cropcap *cropcap)
>> > +{
>> > + if (NULL == cropcap)
>> > + return -EINVAL;
>> > + cropcap->bounds.left = 0;
>> > + cropcap->bounds.top = 0;
>> > + cropcap->bounds.width = vpbe_dev->current_timings.xres;
>> > + cropcap->bounds.height = vpbe_dev->current_timings.yres;
>> > + cropcap->defrect = cropcap->bounds;
>> > + return 0;
>> > +}
>> > +
>> > +/**
>> > + * vpbe_enum_outputs - enumerate outputs
>> > + * @vpbe_dev - vpbe device ptr
>> > + * @output - ptr to v4l2_output structure
>> > + *
>> > + * Enumerates the outputs available at the vpbe display
>> > + * returns the status, -EINVAL if end of output list */ static int
>> > +vpbe_enum_outputs(struct vpbe_device *vpbe_dev,
>> > + struct v4l2_output *output)
>> > +{
>> > + struct vpbe_display_config *vpbe_config = vpbe_dev->cfg;
>> > + int temp_index = output->index;
>> > +
>> > + if (temp_index >= vpbe_config->num_outputs)
>> > + return -EINVAL;
>> > +
>> > + *output = vpbe_config->outputs[temp_index].output;
>> > + output->index = temp_index;
>> > + return 0;
>> > +}
>> > +
>> > +static int vpbe_get_mode_info(struct vpbe_device *vpbe_dev, char
>> > +*mode) {
>> > + struct vpbe_display_config *cfg = vpbe_dev->cfg;
>> > + struct vpbe_enc_mode_info var;
>> > + int curr_output = vpbe_dev->current_out_index, i;
>> > +
>> > + if (NULL == mode)
>> > + return -EINVAL;
>> > +
>> > + for (i = 0; i < cfg->outputs[curr_output].num_modes; i++) {
>> > + var = cfg->outputs[curr_output].modes[i];
>> > + if (!strcmp(mode, var.name)) {
>> > + vpbe_dev->current_timings = var;
>> > + return 0;
>> > + }
>> > + }
>> > + return -EINVAL;
>> > +}
>> > +
>> > +static int vpbe_get_current_mode_info(struct vpbe_device *vpbe_dev,
>> > + struct vpbe_enc_mode_info
>> *mode_info) {
>> > + if (NULL == mode_info)
>> > + return -EINVAL;
>> > +
>> > + *mode_info = vpbe_dev->current_timings;
>> > + return 0;
>> > +}
>> > +
>> > +static int vpbe_get_dv_preset_info(struct vpbe_device *vpbe_dev,
>> > + unsigned int dv_preset)
>> > +{
>> > +
>> > + struct vpbe_display_config *cfg = vpbe_dev->cfg;
>> > + struct vpbe_enc_mode_info var;
>> > + int curr_output = vpbe_dev->current_out_index, i;
>> > +
>> > + for (i = 0; i < vpbe_dev->cfg->outputs[curr_output].num_modes;
>> i++) {
>> > + var = cfg->outputs[curr_output].modes[i];
>> > + if ((var.timings_type & VPBE_ENC_DV_PRESET) &&
>> > + (var.timings.dv_preset == dv_preset)) {
>> > + vpbe_dev->current_timings = var;
>> > + return 0;
>> > + }
>> > + }
>> > + return -EINVAL;
>> > +}
>> > +
>> > +/* Get std by std id */
>> > +static int vpbe_get_std_info(struct vpbe_device *vpbe_dev,
>> > + v4l2_std_id std_id)
>> > +{
>> > + struct vpbe_display_config *cfg = vpbe_dev->cfg;
>> > + struct vpbe_enc_mode_info var;
>> > + int curr_output = vpbe_dev->current_out_index, i;
>> > +
>> > + for (i = 0; i < vpbe_dev->cfg->outputs[curr_output].num_modes;
>> i++) {
>> > + var = cfg->outputs[curr_output].modes[i];
>> > + if ((var.timings_type & VPBE_ENC_STD) &&
>> > + (var.timings.std_id & std_id)) {
>> > + vpbe_dev->current_timings = var;
>> > + return 0;
>> > + }
>> > + }
>> > + return -EINVAL;
>> > +}
>> > +
>> > +static int vpbe_get_std_info_by_name(struct vpbe_device *vpbe_dev,
>> > + char *std_name)
>> > +{
>> > + struct vpbe_display_config *cfg = vpbe_dev->cfg;
>> > + struct vpbe_enc_mode_info var;
>> > + int curr_output = vpbe_dev->current_out_index, i;
>> > +
>> > + for (i = 0; i < vpbe_dev->cfg->outputs[curr_output].num_modes;
>> i++) {
>> > + var = cfg->outputs[curr_output].modes[i];
>> > + if (!strcmp(var.name, std_name)) {
>> > + vpbe_dev->current_timings = var;
>> > + return 0;
>> > + }
>> > + }
>> > + return -EINVAL;
>> > +}
>> > +
>> > +/**
>> > + * vpbe_set_output - Set output
>> > + * @vpbe_dev - vpbe device ptr
>> > + * @index - index of output
>> > + *
>> > + * Set vpbe output to the output specified by the index */ static
>> > +int vpbe_set_output(struct vpbe_device *vpbe_dev, int index) {
>> > +
>> > + struct vpbe_display_config *vpbe_config = vpbe_dev->cfg;
>> > + struct encoder_config_info *curr_enc_info =
>> > + vpbe_current_encoder_info(vpbe_dev);
>> > + int ret = 0, enc_out_index = 0, sd_index;
>> > +
>> > + if (index >= vpbe_config->num_outputs)
>> > + return -EINVAL;
>> > +
>> > + ret = mutex_lock_interruptible(&vpbe_dev->lock);
>> > + if (ret)
>> > + return ret;
>>
>> I am not sure about this mutex. Is it still needed now that the main
>> driver is serialized via opslock? And if it is, does it make sense to
>> use the _interruptible variant? That only makes sense if some of the
>> critical sections can sleep or otherwise take a long time.
>>
>> And if mutex_lock_interruptible() is indeed valid, then you should
>> return -ERESTARTSYS on error. This will cause the scheduler to handle
>> the signal and call the system call again. That's probably what you
>> want.
>
> This is needed as the function might be called from multiple v4l2 video
> Nodes. The ioctl is protected but it is for each node. I will add the
> - ERESTARTSYS as you suggest.
OK, clear.
But isn't it better to just use mutex_lock here? As I mentioned, the
interruptible variant is usually only relevant if you can expect
substantial delays/sleeps in critical sections.
All the checks against the return code of mutex_lock_interruptible add a
lot of code, and if it is not necessary then it's better to use the normal
mutex_lock.
Regards,
Hans
--
Hans Verkuil - video4linux developer - sponsored by Cisco
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
