* Re: [PATCH v2 07/13] davinci: vpif: add support to use videobuf_iolock() [not found] ` <1334652791-15833-8-git-send-email-manjunath.hadli@ti.com> @ 2012-04-17 9:46 ` Laurent Pinchart 2012-05-11 5:26 ` Hadli, Manjunath 0 siblings, 1 reply; 10+ messages in thread From: Laurent Pinchart @ 2012-04-17 9:46 UTC (permalink / raw) To: davinci-linux-open-source; +Cc: Manjunath Hadli, LMML Hi Manjunath, Thanks for the patch. On Tuesday 17 April 2012 14:23:05 Manjunath Hadli wrote: > add support to use videobuf_iolock() instead of VPIF > defined vpif_uservirt_to_phys API. Use videobuf_to_dma_contig > API for both memory-mapped and userptr buffer allocations. > Correspondingly removed vpif_uservirt_to_phys() VPIF defined API. What about using videobuf2 instead ? :-) -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH v2 07/13] davinci: vpif: add support to use videobuf_iolock() 2012-04-17 9:46 ` [PATCH v2 07/13] davinci: vpif: add support to use videobuf_iolock() Laurent Pinchart @ 2012-05-11 5:26 ` Hadli, Manjunath 0 siblings, 0 replies; 10+ messages in thread From: Hadli, Manjunath @ 2012-05-11 5:26 UTC (permalink / raw) To: Laurent Pinchart, davinci-linux-open-source@linux.davincidsp.com; +Cc: LMML Hi Laurent, On Tue, Apr 17, 2012 at 15:16:50, Laurent Pinchart wrote: > Hi Manjunath, > > Thanks for the patch. > > On Tuesday 17 April 2012 14:23:05 Manjunath Hadli wrote: > > add support to use videobuf_iolock() instead of VPIF defined > > vpif_uservirt_to_phys API. Use videobuf_to_dma_contig API for both > > memory-mapped and userptr buffer allocations. > > Correspondingly removed vpif_uservirt_to_phys() VPIF defined API. > > What about using videobuf2 instead ? :-) We will definitely migrate to videobuf2 in the near future, but I wish to go ahead with the implementation as of now. Thx, --Manju > -- > Regards, > > Laurent Pinchart > > ^ permalink raw reply [flat|nested] 10+ messages in thread
[parent not found: <1334652791-15833-6-git-send-email-manjunath.hadli@ti.com>]
* Re: [PATCH v2 05/13] davinci: vpif display: declare contiguous region of memory handled by dma_alloc_coherent [not found] ` <1334652791-15833-6-git-send-email-manjunath.hadli@ti.com> @ 2012-04-17 10:02 ` Laurent Pinchart 2012-05-11 5:30 ` Hadli, Manjunath 0 siblings, 1 reply; 10+ messages in thread From: Laurent Pinchart @ 2012-04-17 10:02 UTC (permalink / raw) To: davinci-linux-open-source; +Cc: Manjunath Hadli, LMML Hi Manjunath, Thanks for the patch. On Tuesday 17 April 2012 14:23:03 Manjunath Hadli wrote: > add support to declare contiguous region of memory to be handled > when requested by dma_alloc_coherent call. The user can specify > the size of the buffers with an offset from the kernel image > using cont_bufsize and cont_bufoffset module parameters respectively. > > Signed-off-by: Manjunath Hadli <manjunath.hadli@ti.com> > --- > drivers/media/video/davinci/vpif_display.c | 65 ++++++++++++++++++++++++- > drivers/media/video/davinci/vpif_display.h | 1 + > 2 files changed, 64 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/video/davinci/vpif_display.c > b/drivers/media/video/davinci/vpif_display.c index 355ad5c..27bc03d 100644 > --- a/drivers/media/video/davinci/vpif_display.c > +++ b/drivers/media/video/davinci/vpif_display.c > @@ -57,18 +57,24 @@ static u32 ch2_numbuffers = 3; > static u32 ch3_numbuffers = 3; > static u32 ch2_bufsize = 1920 * 1080 * 2; > static u32 ch3_bufsize = 720 * 576 * 2; > +static u32 cont_bufoffset; > +static u32 cont_bufsize; > > module_param(debug, int, 0644); > module_param(ch2_numbuffers, uint, S_IRUGO); > module_param(ch3_numbuffers, uint, S_IRUGO); > module_param(ch2_bufsize, uint, S_IRUGO); > module_param(ch3_bufsize, uint, S_IRUGO); > +module_param(cont_bufoffset, uint, S_IRUGO); > +module_param(cont_bufsize, uint, S_IRUGO); > > MODULE_PARM_DESC(debug, "Debug level 0-1"); > MODULE_PARM_DESC(ch2_numbuffers, "Channel2 buffer count (default:3)"); > MODULE_PARM_DESC(ch3_numbuffers, "Channel3 buffer count (default:3)"); > MODULE_PARM_DESC(ch2_bufsize, "Channel2 buffer size (default:1920 x 1080 x > 2)"); MODULE_PARM_DESC(ch3_bufsize, "Channel3 buffer size (default:720 x > 576 x 2)"); +MODULE_PARM_DESC(cont_bufoffset, "Display offset(default 0)"); > +MODULE_PARM_DESC(cont_bufsize, "Display buffer size(default 0)"); > > static struct vpif_config_params config_params = { > .min_numbuffers = 3, > @@ -187,6 +193,24 @@ static int vpif_buffer_setup(struct videobuf_queue *q, > unsigned int *count, return 0; > > *size = config_params.channel_bufsize[ch->channel_id]; > + > + /* > + * Checking if the buffer size exceeds the available buffer > + * ycmux_mode = 0 means 1 channel mode HD and > + * ycmux_mode = 1 means 2 channels mode SD > + */ > + if (ch->vpifparams.std_info.ycmux_mode == 0) { > + if (config_params.video_limit[ch->channel_id]) > + while (*size * *count > (config_params.video_limit[0] > + + config_params.video_limit[1])) > + (*count)--; > + } else { > + if (config_params.video_limit[ch->channel_id]) > + while (*size * *count > > + config_params.video_limit[ch->channel_id]) > + (*count)--; > + } > + > if (*count < config_params.min_numbuffers) > *count = config_params.min_numbuffers; > > @@ -830,7 +854,7 @@ static int vpif_reqbufs(struct file *file, void *priv, > > common = &ch->common[index]; > > - if (common->fmt.type != reqbuf->type) > + if (common->fmt.type != reqbuf->type || !vpif_dev) > return -EINVAL; > > if (0 != common->io_usrs) > @@ -847,7 +871,7 @@ static int vpif_reqbufs(struct file *file, void *priv, > > /* Initialize videobuf queue as per the buffer type */ > videobuf_queue_dma_contig_init(&common->buffer_queue, > - &video_qops, NULL, > + &video_qops, vpif_dev, > &common->irqlock, > reqbuf->type, field, > sizeof(struct videobuf_buffer), fh, > @@ -1686,12 +1710,14 @@ static __init int vpif_probe(struct platform_device > *pdev) struct vpif_subdev_info *subdevdata; > struct vpif_display_config *config; > int i, j = 0, k, q, m, err = 0; > + unsigned long phys_end_kernel; > struct i2c_adapter *i2c_adap; > struct common_obj *common; > struct channel_obj *ch; > struct video_device *vfd; > struct resource *res; > int subdev_count; > + size_t size; > > vpif_dev = &pdev->dev; > > @@ -1749,6 +1775,41 @@ static __init int vpif_probe(struct platform_device > *pdev) ch->video_dev = vfd; > } > > + /* Initialising the memory from the input arguments file for > + * contiguous memory buffers and avoid defragmentation > + */ > + if (cont_bufsize) { > + /* attempt to determine the end of Linux kernel memory */ > + phys_end_kernel = virt_to_phys((void *)PAGE_OFFSET) + > + (num_physpages << PAGE_SHIFT); > + phys_end_kernel += cont_bufoffset; > + size = cont_bufsize; > + > + err = dma_declare_coherent_memory(&pdev->dev, phys_end_kernel, > + phys_end_kernel, size, > + DMA_MEMORY_MAP | DMA_MEMORY_EXCLUSIVE); This is a pretty dangerous hack. You should compute the memory address and size in board code, and pass it to the driver through a device resource (don't forget to call request_mem_region on the resource). I think the dma_declare_coherent_memory() call should be moved to board code as well. > + if (!err) { > + dev_err(&pdev->dev, "Unable to declare MMAP memory.\n"); > + err = -ENOMEM; > + goto probe_out; > + } > + > + /* The resources are divided into two equal memory and when > + * we have HD output we can add them together > + */ > + for (j = 0; j < VPIF_DISPLAY_MAX_DEVICES; j++) { > + ch = vpif_obj.dev[j]; > + ch->channel_id = j; > + > + /* only enabled if second resource exists */ > + config_params.video_limit[ch->channel_id] = 0; > + if (cont_bufsize) > + config_params.video_limit[ch->channel_id] = > + size/2; > + } > + } > + > for (j = 0; j < VPIF_DISPLAY_MAX_DEVICES; j++) { > ch = vpif_obj.dev[j]; > /* Initialize field of the channel objects */ > diff --git a/drivers/media/video/davinci/vpif_display.h > b/drivers/media/video/davinci/vpif_display.h index dd4887c..8a311f1 100644 > --- a/drivers/media/video/davinci/vpif_display.h > +++ b/drivers/media/video/davinci/vpif_display.h > @@ -158,6 +158,7 @@ struct vpif_config_params { > u32 min_bufsize[VPIF_DISPLAY_NUM_CHANNELS]; > u32 channel_bufsize[VPIF_DISPLAY_NUM_CHANNELS]; > u8 numbuffers[VPIF_DISPLAY_NUM_CHANNELS]; > + u32 video_limit[VPIF_DISPLAY_NUM_CHANNELS]; > u8 min_numbuffers; > }; -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH v2 05/13] davinci: vpif display: declare contiguous region of memory handled by dma_alloc_coherent 2012-04-17 10:02 ` [PATCH v2 05/13] davinci: vpif display: declare contiguous region of memory handled by dma_alloc_coherent Laurent Pinchart @ 2012-05-11 5:30 ` Hadli, Manjunath 2012-05-14 7:46 ` Laurent Pinchart 0 siblings, 1 reply; 10+ messages in thread From: Hadli, Manjunath @ 2012-05-11 5:30 UTC (permalink / raw) To: Laurent Pinchart, davinci-linux-open-source@linux.davincidsp.com; +Cc: LMML Hi Laurent, On Tue, Apr 17, 2012 at 15:32:55, Laurent Pinchart wrote: > Hi Manjunath, > > Thanks for the patch. > > On Tuesday 17 April 2012 14:23:03 Manjunath Hadli wrote: > > add support to declare contiguous region of memory to be handled when > > requested by dma_alloc_coherent call. The user can specify the size of > > the buffers with an offset from the kernel image using cont_bufsize > > and cont_bufoffset module parameters respectively. > > > > Signed-off-by: Manjunath Hadli <manjunath.hadli@ti.com> > > --- > > drivers/media/video/davinci/vpif_display.c | 65 ++++++++++++++++++++++++- > > drivers/media/video/davinci/vpif_display.h | 1 + > > 2 files changed, 64 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/media/video/davinci/vpif_display.c > > b/drivers/media/video/davinci/vpif_display.c index 355ad5c..27bc03d > > 100644 > > --- a/drivers/media/video/davinci/vpif_display.c > > +++ b/drivers/media/video/davinci/vpif_display.c > > @@ -57,18 +57,24 @@ static u32 ch2_numbuffers = 3; static u32 > > ch3_numbuffers = 3; static u32 ch2_bufsize = 1920 * 1080 * 2; static > > u32 ch3_bufsize = 720 * 576 * 2; > > +static u32 cont_bufoffset; > > +static u32 cont_bufsize; > > > > module_param(debug, int, 0644); > > module_param(ch2_numbuffers, uint, S_IRUGO); > > module_param(ch3_numbuffers, uint, S_IRUGO); > > module_param(ch2_bufsize, uint, S_IRUGO); module_param(ch3_bufsize, > > uint, S_IRUGO); > > +module_param(cont_bufoffset, uint, S_IRUGO); > > +module_param(cont_bufsize, uint, S_IRUGO); > > > > MODULE_PARM_DESC(debug, "Debug level 0-1"); > > MODULE_PARM_DESC(ch2_numbuffers, "Channel2 buffer count (default:3)"); > > MODULE_PARM_DESC(ch3_numbuffers, "Channel3 buffer count (default:3)"); > > MODULE_PARM_DESC(ch2_bufsize, "Channel2 buffer size (default:1920 x > > 1080 x 2)"); MODULE_PARM_DESC(ch3_bufsize, "Channel3 buffer size > > (default:720 x > > 576 x 2)"); +MODULE_PARM_DESC(cont_bufoffset, "Display offset(default > > 0)"); > > +MODULE_PARM_DESC(cont_bufsize, "Display buffer size(default 0)"); > > > > static struct vpif_config_params config_params = { > > .min_numbuffers = 3, > > @@ -187,6 +193,24 @@ static int vpif_buffer_setup(struct > > videobuf_queue *q, unsigned int *count, return 0; > > > > *size = config_params.channel_bufsize[ch->channel_id]; > > + > > + /* > > + * Checking if the buffer size exceeds the available buffer > > + * ycmux_mode = 0 means 1 channel mode HD and > > + * ycmux_mode = 1 means 2 channels mode SD > > + */ > > + if (ch->vpifparams.std_info.ycmux_mode == 0) { > > + if (config_params.video_limit[ch->channel_id]) > > + while (*size * *count > (config_params.video_limit[0] > > + + config_params.video_limit[1])) > > + (*count)--; > > + } else { > > + if (config_params.video_limit[ch->channel_id]) > > + while (*size * *count > > > + config_params.video_limit[ch->channel_id]) > > + (*count)--; > > + } > > + > > if (*count < config_params.min_numbuffers) > > *count = config_params.min_numbuffers; > > > > @@ -830,7 +854,7 @@ static int vpif_reqbufs(struct file *file, void > > *priv, > > > > common = &ch->common[index]; > > > > - if (common->fmt.type != reqbuf->type) > > + if (common->fmt.type != reqbuf->type || !vpif_dev) > > return -EINVAL; > > > > if (0 != common->io_usrs) > > @@ -847,7 +871,7 @@ static int vpif_reqbufs(struct file *file, void > > *priv, > > > > /* Initialize videobuf queue as per the buffer type */ > > videobuf_queue_dma_contig_init(&common->buffer_queue, > > - &video_qops, NULL, > > + &video_qops, vpif_dev, > > &common->irqlock, > > reqbuf->type, field, > > sizeof(struct videobuf_buffer), fh, @@ -1686,12 +1710,14 @@ > > static __init int vpif_probe(struct platform_device > > *pdev) struct vpif_subdev_info *subdevdata; > > struct vpif_display_config *config; > > int i, j = 0, k, q, m, err = 0; > > + unsigned long phys_end_kernel; > > struct i2c_adapter *i2c_adap; > > struct common_obj *common; > > struct channel_obj *ch; > > struct video_device *vfd; > > struct resource *res; > > int subdev_count; > > + size_t size; > > > > vpif_dev = &pdev->dev; > > > > @@ -1749,6 +1775,41 @@ static __init int vpif_probe(struct > > platform_device > > *pdev) ch->video_dev = vfd; > > } > > > > + /* Initialising the memory from the input arguments file for > > + * contiguous memory buffers and avoid defragmentation > > + */ > > + if (cont_bufsize) { > > + /* attempt to determine the end of Linux kernel memory */ > > + phys_end_kernel = virt_to_phys((void *)PAGE_OFFSET) + > > + (num_physpages << PAGE_SHIFT); > > + phys_end_kernel += cont_bufoffset; > > + size = cont_bufsize; > > + > > + err = dma_declare_coherent_memory(&pdev->dev, phys_end_kernel, > > + phys_end_kernel, size, > > + DMA_MEMORY_MAP | DMA_MEMORY_EXCLUSIVE); > > This is a pretty dangerous hack. You should compute the memory address and size in board code, and pass it to the driver through a device resource (don't forget to call request_mem_region on the resource). I think the > dma_declare_coherent_memory() call should be moved to board code as well. > I don't think it is a hack. Since the device does not support scatter gather MMU, we need to make sure that the requirement of memory scucceeds for the Driver buffers. Here the size is taken as part of module parameters, which If I am right, cannot be moved to board file. Thx, --Manju > > + if (!err) { > > + dev_err(&pdev->dev, "Unable to declare MMAP memory.\n"); > > + err = -ENOMEM; > > + goto probe_out; > > + } > > + > > + /* The resources are divided into two equal memory and when > > + * we have HD output we can add them together > > + */ > > + for (j = 0; j < VPIF_DISPLAY_MAX_DEVICES; j++) { > > + ch = vpif_obj.dev[j]; > > + ch->channel_id = j; > > + > > + /* only enabled if second resource exists */ > > + config_params.video_limit[ch->channel_id] = 0; > > + if (cont_bufsize) > > + config_params.video_limit[ch->channel_id] = > > + size/2; > > + } > > + } > > + > > for (j = 0; j < VPIF_DISPLAY_MAX_DEVICES; j++) { > > ch = vpif_obj.dev[j]; > > /* Initialize field of the channel objects */ diff --git > > a/drivers/media/video/davinci/vpif_display.h > > b/drivers/media/video/davinci/vpif_display.h index dd4887c..8a311f1 > > 100644 > > --- a/drivers/media/video/davinci/vpif_display.h > > +++ b/drivers/media/video/davinci/vpif_display.h > > @@ -158,6 +158,7 @@ struct vpif_config_params { > > u32 min_bufsize[VPIF_DISPLAY_NUM_CHANNELS]; > > u32 channel_bufsize[VPIF_DISPLAY_NUM_CHANNELS]; > > u8 numbuffers[VPIF_DISPLAY_NUM_CHANNELS]; > > + u32 video_limit[VPIF_DISPLAY_NUM_CHANNELS]; > > u8 min_numbuffers; > > }; > > -- > Regards, > > Laurent Pinchart > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 05/13] davinci: vpif display: declare contiguous region of memory handled by dma_alloc_coherent 2012-05-11 5:30 ` Hadli, Manjunath @ 2012-05-14 7:46 ` Laurent Pinchart 2012-06-07 11:15 ` Hadli, Manjunath 0 siblings, 1 reply; 10+ messages in thread From: Laurent Pinchart @ 2012-05-14 7:46 UTC (permalink / raw) To: Hadli, Manjunath; +Cc: davinci-linux-open-source@linux.davincidsp.com, LMML Hi Manjunath, On Friday 11 May 2012 05:30:43 Hadli, Manjunath wrote: > On Tue, Apr 17, 2012 at 15:32:55, Laurent Pinchart wrote: > > On Tuesday 17 April 2012 14:23:03 Manjunath Hadli wrote: > > > add support to declare contiguous region of memory to be handled when > > > requested by dma_alloc_coherent call. The user can specify the size of > > > the buffers with an offset from the kernel image using cont_bufsize > > > and cont_bufoffset module parameters respectively. [snip] > > > @@ -1686,12 +1710,14 @@ static __init int vpif_probe(struct > > > platform_device *pdev) struct vpif_subdev_info *subdevdata; > > > > > > struct vpif_display_config *config; > > > int i, j = 0, k, q, m, err = 0; > > > > > > + unsigned long phys_end_kernel; > > > > > > struct i2c_adapter *i2c_adap; > > > struct common_obj *common; > > > struct channel_obj *ch; > > > struct video_device *vfd; > > > struct resource *res; > > > int subdev_count; > > > > > > + size_t size; > > > > > > vpif_dev = &pdev->dev; > > > > > > @@ -1749,6 +1775,41 @@ static __init int vpif_probe(struct > > > platform_device > > > *pdev) ch->video_dev = vfd; > > > > > > } > > > > > > + /* Initialising the memory from the input arguments file for > > > + * contiguous memory buffers and avoid defragmentation > > > + */ > > > + if (cont_bufsize) { > > > + /* attempt to determine the end of Linux kernel memory */ > > > + phys_end_kernel = virt_to_phys((void *)PAGE_OFFSET) + > > > + (num_physpages << PAGE_SHIFT); > > > + phys_end_kernel += cont_bufoffset; > > > + size = cont_bufsize; > > > + > > > + err = dma_declare_coherent_memory(&pdev->dev, phys_end_kernel, > > > + phys_end_kernel, size, > > > + DMA_MEMORY_MAP | DMA_MEMORY_EXCLUSIVE); > > > > This is a pretty dangerous hack. You should compute the memory address and > > size in board code, and pass it to the driver through a device resource > > (don't forget to call request_mem_region on the resource). I think the > > dma_declare_coherent_memory() call should be moved to board code as well. > > I don't think it is a hack. Since the device does not support scatter gather > MMU, we need to make sure that the requirement of memory scucceeds for the > Driver buffers. If two drivers attempt to do the same you're screwed. That's why this should be moved to board code, where memory reservation for all devices that need it can be coordinated. You should call dma_declare_coherent_memory() there, not in the VPIF driver. > Here the size is taken as part of module parameters, which If I am right, > cannot be moved to board file. Depending on how early you need the information in board code you can use early_param(), __setup() or normal module parameter macros in the board code. > > > + if (!err) { > > > + dev_err(&pdev->dev, "Unable to declare MMAP memory.\n"); > > > + err = -ENOMEM; > > > + goto probe_out; > > > + } > > > + > > > + /* The resources are divided into two equal memory and when > > > + * we have HD output we can add them together > > > + */ > > > + for (j = 0; j < VPIF_DISPLAY_MAX_DEVICES; j++) { > > > + ch = vpif_obj.dev[j]; > > > + ch->channel_id = j; > > > + > > > + /* only enabled if second resource exists */ > > > + config_params.video_limit[ch->channel_id] = 0; > > > + if (cont_bufsize) > > > + config_params.video_limit[ch->channel_id] = > > > + size/2; > > > + } > > > + } > > > + > > > > > > for (j = 0; j < VPIF_DISPLAY_MAX_DEVICES; j++) { > > > > > > ch = vpif_obj.dev[j]; > > > /* Initialize field of the channel objects */ diff --git -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH v2 05/13] davinci: vpif display: declare contiguous region of memory handled by dma_alloc_coherent 2012-05-14 7:46 ` Laurent Pinchart @ 2012-06-07 11:15 ` Hadli, Manjunath 0 siblings, 0 replies; 10+ messages in thread From: Hadli, Manjunath @ 2012-06-07 11:15 UTC (permalink / raw) To: 'Laurent Pinchart' Cc: davinci-linux-open-source@linux.davincidsp.com, LMML Hi Laurent, On Mon, May 14, 2012 at 13:16:27, Laurent Pinchart wrote: > Hi Manjunath, > > On Friday 11 May 2012 05:30:43 Hadli, Manjunath wrote: > > On Tue, Apr 17, 2012 at 15:32:55, Laurent Pinchart wrote: > > > On Tuesday 17 April 2012 14:23:03 Manjunath Hadli wrote: > > > > add support to declare contiguous region of memory to be handled when > > > > requested by dma_alloc_coherent call. The user can specify the size of > > > > the buffers with an offset from the kernel image using cont_bufsize > > > > and cont_bufoffset module parameters respectively. > > [snip] > > > > > @@ -1686,12 +1710,14 @@ static __init int vpif_probe(struct > > > > platform_device *pdev) struct vpif_subdev_info *subdevdata; > > > > > > > > struct vpif_display_config *config; > > > > int i, j = 0, k, q, m, err = 0; > > > > > > > > + unsigned long phys_end_kernel; > > > > > > > > struct i2c_adapter *i2c_adap; > > > > struct common_obj *common; > > > > struct channel_obj *ch; > > > > struct video_device *vfd; > > > > struct resource *res; > > > > int subdev_count; > > > > > > > > + size_t size; > > > > > > > > vpif_dev = &pdev->dev; > > > > > > > > @@ -1749,6 +1775,41 @@ static __init int vpif_probe(struct > > > > platform_device > > > > *pdev) ch->video_dev = vfd; > > > > > > > > } > > > > > > > > + /* Initialising the memory from the input arguments file for > > > > + * contiguous memory buffers and avoid defragmentation > > > > + */ > > > > + if (cont_bufsize) { > > > > + /* attempt to determine the end of Linux kernel memory */ > > > > + phys_end_kernel = virt_to_phys((void *)PAGE_OFFSET) + > > > > + (num_physpages << PAGE_SHIFT); > > > > + phys_end_kernel += cont_bufoffset; > > > > + size = cont_bufsize; > > > > + > > > > + err = dma_declare_coherent_memory(&pdev->dev, phys_end_kernel, > > > > + phys_end_kernel, size, > > > > + DMA_MEMORY_MAP | DMA_MEMORY_EXCLUSIVE); > > > > > > This is a pretty dangerous hack. You should compute the memory address and > > > size in board code, and pass it to the driver through a device resource > > > (don't forget to call request_mem_region on the resource). I think the > > > dma_declare_coherent_memory() call should be moved to board code as well. > > > > I don't think it is a hack. Since the device does not support scatter gather > > MMU, we need to make sure that the requirement of memory scucceeds for the > > Driver buffers. > > If two drivers attempt to do the same you're screwed. That's why this should > be moved to board code, where memory reservation for all devices that need it > can be coordinated. You should call dma_declare_coherent_memory() there, not > in the VPIF driver. > Ok I'll move dma_declare_coherent_memory() to board file. > > Here the size is taken as part of module parameters, which If I am right, > > cannot be moved to board file. > > Depending on how early you need the information in board code you can use > early_param(), __setup() or normal module parameter macros in the board code. > Ok I'll fix this. Thx, --Manju > > > > + if (!err) { > > > > + dev_err(&pdev->dev, "Unable to declare MMAP memory.\n"); > > > > + err = -ENOMEM; > > > > + goto probe_out; > > > > + } > > > > + > > > > + /* The resources are divided into two equal memory and when > > > > + * we have HD output we can add them together > > > > + */ > > > > + for (j = 0; j < VPIF_DISPLAY_MAX_DEVICES; j++) { > > > > + ch = vpif_obj.dev[j]; > > > > + ch->channel_id = j; > > > > + > > > > + /* only enabled if second resource exists */ > > > > + config_params.video_limit[ch->channel_id] = 0; > > > > + if (cont_bufsize) > > > > + config_params.video_limit[ch->channel_id] = > > > > + size/2; > > > > + } > > > > + } > > > > + > > > > > > > > for (j = 0; j < VPIF_DISPLAY_MAX_DEVICES; j++) { > > > > > > > > ch = vpif_obj.dev[j]; > > > > /* Initialize field of the channel objects */ diff --git > > -- > Regards, > > Laurent Pinchart > > ^ permalink raw reply [flat|nested] 10+ messages in thread
[parent not found: <1334652791-15833-2-git-send-email-manjunath.hadli@ti.com>]
* Re: [PATCH v2 01/13] davinci: vpif: add check for genuine interrupts in the isr [not found] ` <1334652791-15833-2-git-send-email-manjunath.hadli@ti.com> @ 2012-04-17 10:06 ` Laurent Pinchart 2012-05-11 5:32 ` Hadli, Manjunath 0 siblings, 1 reply; 10+ messages in thread From: Laurent Pinchart @ 2012-04-17 10:06 UTC (permalink / raw) To: davinci-linux-open-source; +Cc: Manjunath Hadli, LMML Hi Manjunath, Thanks for the patch. On Tuesday 17 April 2012 14:22:59 Manjunath Hadli wrote: > As the same interrupt is shared between capture and display devices, > sometimes we get isr calls where the interrupt might not genuinely belong > to capture or display. Hence, add a condition in the isr to check for > interrupt ownership and channel number to make sure we do not > service wrong interrupts. > > Signed-off-by: Manjunath Hadli <manjunath.hadli@ti.com> > --- > drivers/media/video/davinci/vpif_capture.c | 5 +++++ > drivers/media/video/davinci/vpif_display.c | 5 +++++ > include/media/davinci/vpif_types.h | 2 ++ > 3 files changed, 12 insertions(+), 0 deletions(-) > > diff --git a/drivers/media/video/davinci/vpif_capture.c > b/drivers/media/video/davinci/vpif_capture.c index 6504e40..33d865d 100644 > --- a/drivers/media/video/davinci/vpif_capture.c > +++ b/drivers/media/video/davinci/vpif_capture.c > @@ -333,6 +333,7 @@ static void vpif_schedule_next_buffer(struct common_obj > *common) */ > static irqreturn_t vpif_channel_isr(int irq, void *dev_id) > { > + struct vpif_capture_config *config = vpif_dev->platform_data; > struct vpif_device *dev = &vpif_obj; > struct common_obj *common; > struct channel_obj *ch; > @@ -341,6 +342,10 @@ static irqreturn_t vpif_channel_isr(int irq, void > *dev_id) int fid = -1, i; > > channel_id = *(int *)(dev_id); > + if (!config->intr_status || > + !config->intr_status(vpif_base, channel_id)) > + return IRQ_NONE; > + I don't think this is the right way to handle the situation. You should instead read the interrupt source register for the VPIF capture device, and return IRQ_NONE if you find that no interrupt source has been flagged (and similarly for the display device below). > ch = dev->dev[channel_id]; > > field = ch->common[VPIF_VIDEO_INDEX].fmt.fmt.pix.field; > diff --git a/drivers/media/video/davinci/vpif_display.c > b/drivers/media/video/davinci/vpif_display.c index 7fa34b4..9b59e0b 100644 > --- a/drivers/media/video/davinci/vpif_display.c > +++ b/drivers/media/video/davinci/vpif_display.c > @@ -299,6 +299,7 @@ static void process_interlaced_mode(int fid, struct > common_obj *common) */ > static irqreturn_t vpif_channel_isr(int irq, void *dev_id) > { > + struct vpif_display_config *config = vpif_dev->platform_data; > struct vpif_device *dev = &vpif_obj; > struct channel_obj *ch; > struct common_obj *common; > @@ -307,6 +308,10 @@ static irqreturn_t vpif_channel_isr(int irq, void > *dev_id) int channel_id = 0; > > channel_id = *(int *)(dev_id); > + if (!config->intr_status || > + !config->intr_status(vpif_base, channel_id + 2)) > + return IRQ_NONE; > + > ch = dev->dev[channel_id]; > field = ch->common[VPIF_VIDEO_INDEX].fmt.fmt.pix.field; > for (i = 0; i < VPIF_NUMOBJECTS; i++) { > diff --git a/include/media/davinci/vpif_types.h > b/include/media/davinci/vpif_types.h index bd8217c..2845bda 100644 > --- a/include/media/davinci/vpif_types.h > +++ b/include/media/davinci/vpif_types.h > @@ -45,6 +45,7 @@ struct vpif_subdev_info { > > struct vpif_display_config { > int (*set_clock)(int, int); > + int (*intr_status)(void __iomem *vpif_base, int); > struct vpif_subdev_info *subdevinfo; > int subdev_count; > const char **output; > @@ -65,6 +66,7 @@ struct vpif_capture_chan_config { > struct vpif_capture_config { > int (*setup_input_channel_mode)(int); > int (*setup_input_path)(int, const char *); > + int (*intr_status)(void __iomem *vpif_base, int); > struct vpif_capture_chan_config chan_config[VPIF_CAPTURE_MAX_CHANNELS]; > struct vpif_subdev_info *subdev_info; > int subdev_count; -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH v2 01/13] davinci: vpif: add check for genuine interrupts in the isr 2012-04-17 10:06 ` [PATCH v2 01/13] davinci: vpif: add check for genuine interrupts in the isr Laurent Pinchart @ 2012-05-11 5:32 ` Hadli, Manjunath 2012-05-14 7:49 ` Laurent Pinchart 0 siblings, 1 reply; 10+ messages in thread From: Hadli, Manjunath @ 2012-05-11 5:32 UTC (permalink / raw) To: Laurent Pinchart, davinci-linux-open-source@linux.davincidsp.com; +Cc: LMML Hi Laurent, On Tue, Apr 17, 2012 at 15:36:16, Laurent Pinchart wrote: > Hi Manjunath, > > Thanks for the patch. > > On Tuesday 17 April 2012 14:22:59 Manjunath Hadli wrote: > > As the same interrupt is shared between capture and display devices, > > sometimes we get isr calls where the interrupt might not genuinely > > belong to capture or display. Hence, add a condition in the isr to > > check for interrupt ownership and channel number to make sure we do > > not service wrong interrupts. > > > > Signed-off-by: Manjunath Hadli <manjunath.hadli@ti.com> > > --- > > drivers/media/video/davinci/vpif_capture.c | 5 +++++ > > drivers/media/video/davinci/vpif_display.c | 5 +++++ > > include/media/davinci/vpif_types.h | 2 ++ > > 3 files changed, 12 insertions(+), 0 deletions(-) > > > > diff --git a/drivers/media/video/davinci/vpif_capture.c > > b/drivers/media/video/davinci/vpif_capture.c index 6504e40..33d865d > > 100644 > > --- a/drivers/media/video/davinci/vpif_capture.c > > +++ b/drivers/media/video/davinci/vpif_capture.c > > @@ -333,6 +333,7 @@ static void vpif_schedule_next_buffer(struct > > common_obj > > *common) */ > > static irqreturn_t vpif_channel_isr(int irq, void *dev_id) { > > + struct vpif_capture_config *config = vpif_dev->platform_data; > > struct vpif_device *dev = &vpif_obj; > > struct common_obj *common; > > struct channel_obj *ch; > > @@ -341,6 +342,10 @@ static irqreturn_t vpif_channel_isr(int irq, void > > *dev_id) int fid = -1, i; > > > > channel_id = *(int *)(dev_id); > > + if (!config->intr_status || > > + !config->intr_status(vpif_base, channel_id)) > > + return IRQ_NONE; > > + > > I don't think this is the right way to handle the situation. You should instead read the interrupt source register for the VPIF capture device, and return IRQ_NONE if you find that no interrupt source has been flagged (and similarly for the display device below). Agreed, and this is what is being done in intr_status() function, which is implemented in the board file. This function checks the interrupt source register for VPIF capture and display devices the way you mentioned. Thx, --Manju > > > ch = dev->dev[channel_id]; > > > > field = ch->common[VPIF_VIDEO_INDEX].fmt.fmt.pix.field; > > diff --git a/drivers/media/video/davinci/vpif_display.c > > b/drivers/media/video/davinci/vpif_display.c index 7fa34b4..9b59e0b > > 100644 > > --- a/drivers/media/video/davinci/vpif_display.c > > +++ b/drivers/media/video/davinci/vpif_display.c > > @@ -299,6 +299,7 @@ static void process_interlaced_mode(int fid, > > struct common_obj *common) */ static irqreturn_t vpif_channel_isr(int > > irq, void *dev_id) { > > + struct vpif_display_config *config = vpif_dev->platform_data; > > struct vpif_device *dev = &vpif_obj; > > struct channel_obj *ch; > > struct common_obj *common; > > @@ -307,6 +308,10 @@ static irqreturn_t vpif_channel_isr(int irq, void > > *dev_id) int channel_id = 0; > > > > channel_id = *(int *)(dev_id); > > + if (!config->intr_status || > > + !config->intr_status(vpif_base, channel_id + 2)) > > + return IRQ_NONE; > > + > > ch = dev->dev[channel_id]; > > field = ch->common[VPIF_VIDEO_INDEX].fmt.fmt.pix.field; > > for (i = 0; i < VPIF_NUMOBJECTS; i++) { diff --git > > a/include/media/davinci/vpif_types.h > > b/include/media/davinci/vpif_types.h index bd8217c..2845bda 100644 > > --- a/include/media/davinci/vpif_types.h > > +++ b/include/media/davinci/vpif_types.h > > @@ -45,6 +45,7 @@ struct vpif_subdev_info { > > > > struct vpif_display_config { > > int (*set_clock)(int, int); > > + int (*intr_status)(void __iomem *vpif_base, int); > > struct vpif_subdev_info *subdevinfo; > > int subdev_count; > > const char **output; > > @@ -65,6 +66,7 @@ struct vpif_capture_chan_config { struct > > vpif_capture_config { > > int (*setup_input_channel_mode)(int); > > int (*setup_input_path)(int, const char *); > > + int (*intr_status)(void __iomem *vpif_base, int); > > struct vpif_capture_chan_config chan_config[VPIF_CAPTURE_MAX_CHANNELS]; > > struct vpif_subdev_info *subdev_info; > > int subdev_count; > > -- > Regards, > > Laurent Pinchart > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 01/13] davinci: vpif: add check for genuine interrupts in the isr 2012-05-11 5:32 ` Hadli, Manjunath @ 2012-05-14 7:49 ` Laurent Pinchart 2012-06-07 11:25 ` Hadli, Manjunath 0 siblings, 1 reply; 10+ messages in thread From: Laurent Pinchart @ 2012-05-14 7:49 UTC (permalink / raw) To: Hadli, Manjunath; +Cc: davinci-linux-open-source@linux.davincidsp.com, LMML Hi Manjunath, On Friday 11 May 2012 05:32:13 Hadli, Manjunath wrote: > On Tue, Apr 17, 2012 at 15:36:16, Laurent Pinchart wrote: > > On Tuesday 17 April 2012 14:22:59 Manjunath Hadli wrote: > > > As the same interrupt is shared between capture and display devices, > > > sometimes we get isr calls where the interrupt might not genuinely > > > belong to capture or display. Hence, add a condition in the isr to > > > check for interrupt ownership and channel number to make sure we do > > > not service wrong interrupts. > > > > > > Signed-off-by: Manjunath Hadli <manjunath.hadli@ti.com> > > > --- > > > > > > drivers/media/video/davinci/vpif_capture.c | 5 +++++ > > > drivers/media/video/davinci/vpif_display.c | 5 +++++ > > > include/media/davinci/vpif_types.h | 2 ++ > > > 3 files changed, 12 insertions(+), 0 deletions(-) > > > > > > diff --git a/drivers/media/video/davinci/vpif_capture.c > > > b/drivers/media/video/davinci/vpif_capture.c index 6504e40..33d865d > > > 100644 > > > --- a/drivers/media/video/davinci/vpif_capture.c > > > +++ b/drivers/media/video/davinci/vpif_capture.c > > > @@ -333,6 +333,7 @@ static void vpif_schedule_next_buffer(struct > > > common_obj > > > *common) */ > > > > > > static irqreturn_t vpif_channel_isr(int irq, void *dev_id) { > > > > > > + struct vpif_capture_config *config = vpif_dev->platform_data; > > > > > > struct vpif_device *dev = &vpif_obj; > > > struct common_obj *common; > > > struct channel_obj *ch; > > > > > > @@ -341,6 +342,10 @@ static irqreturn_t vpif_channel_isr(int irq, void > > > *dev_id) int fid = -1, i; > > > > > > channel_id = *(int *)(dev_id); > > > > > > + if (!config->intr_status || > > > + !config->intr_status(vpif_base, channel_id)) > > > + return IRQ_NONE; > > > + > > > > I don't think this is the right way to handle the situation. You should > > instead read the interrupt source register for the VPIF capture device, > > and return IRQ_NONE if you find that no interrupt source has been flagged > > (and similarly for the display device below). > > Agreed, and this is what is being done in intr_status() function, which > is implemented in the board file. This function checks the interrupt source > register for VPIF capture and display devices the way you mentioned. Why do you need to do that in board code ? You can just check whether the VPIF capture hardware has generated an interrupt here exactly the same way as you do in your board code, and return IRQ_NONE if it hasn't. There's no need for the VPIF capture driver to be aware of the VPIF display driver (and vice versa). > > > ch = dev->dev[channel_id]; > > > > > > field = ch->common[VPIF_VIDEO_INDEX].fmt.fmt.pix.field; -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH v2 01/13] davinci: vpif: add check for genuine interrupts in the isr 2012-05-14 7:49 ` Laurent Pinchart @ 2012-06-07 11:25 ` Hadli, Manjunath 0 siblings, 0 replies; 10+ messages in thread From: Hadli, Manjunath @ 2012-06-07 11:25 UTC (permalink / raw) To: 'Laurent Pinchart' Cc: davinci-linux-open-source@linux.davincidsp.com, LMML Hi Laurent, On Mon, May 14, 2012 at 13:19:40, Laurent Pinchart wrote: > Hi Manjunath, > > On Friday 11 May 2012 05:32:13 Hadli, Manjunath wrote: > > On Tue, Apr 17, 2012 at 15:36:16, Laurent Pinchart wrote: > > > On Tuesday 17 April 2012 14:22:59 Manjunath Hadli wrote: > > > > As the same interrupt is shared between capture and display devices, > > > > sometimes we get isr calls where the interrupt might not genuinely > > > > belong to capture or display. Hence, add a condition in the isr to > > > > check for interrupt ownership and channel number to make sure we do > > > > not service wrong interrupts. > > > > > > > > Signed-off-by: Manjunath Hadli <manjunath.hadli@ti.com> > > > > --- > > > > > > > > drivers/media/video/davinci/vpif_capture.c | 5 +++++ > > > > drivers/media/video/davinci/vpif_display.c | 5 +++++ > > > > include/media/davinci/vpif_types.h | 2 ++ > > > > 3 files changed, 12 insertions(+), 0 deletions(-) > > > > > > > > diff --git a/drivers/media/video/davinci/vpif_capture.c > > > > b/drivers/media/video/davinci/vpif_capture.c index 6504e40..33d865d > > > > 100644 > > > > --- a/drivers/media/video/davinci/vpif_capture.c > > > > +++ b/drivers/media/video/davinci/vpif_capture.c > > > > @@ -333,6 +333,7 @@ static void vpif_schedule_next_buffer(struct > > > > common_obj > > > > *common) */ > > > > > > > > static irqreturn_t vpif_channel_isr(int irq, void *dev_id) { > > > > > > > > + struct vpif_capture_config *config = vpif_dev->platform_data; > > > > > > > > struct vpif_device *dev = &vpif_obj; > > > > struct common_obj *common; > > > > struct channel_obj *ch; > > > > > > > > @@ -341,6 +342,10 @@ static irqreturn_t vpif_channel_isr(int irq, void > > > > *dev_id) int fid = -1, i; > > > > > > > > channel_id = *(int *)(dev_id); > > > > > > > > + if (!config->intr_status || > > > > + !config->intr_status(vpif_base, channel_id)) > > > > + return IRQ_NONE; > > > > + > > > > > > I don't think this is the right way to handle the situation. You should > > > instead read the interrupt source register for the VPIF capture device, > > > and return IRQ_NONE if you find that no interrupt source has been flagged > > > (and similarly for the display device below). > > > > Agreed, and this is what is being done in intr_status() function, which > > is implemented in the board file. This function checks the interrupt source > > register for VPIF capture and display devices the way you mentioned. > > Why do you need to do that in board code ? You can just check whether the VPIF > capture hardware has generated an interrupt here exactly the same way as you > do in your board code, and return IRQ_NONE if it hasn't. There's no need for > the VPIF capture driver to be aware of the VPIF display driver (and vice > versa). > Ok I'll add this in the driver itself. Thx, --Manju > > > > ch = dev->dev[channel_id]; > > > > > > > > field = ch->common[VPIF_VIDEO_INDEX].fmt.fmt.pix.field; > > -- > Regards, > > Laurent Pinchart > > ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2012-06-07 11:25 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1334652791-15833-1-git-send-email-manjunath.hadli@ti.com>
[not found] ` <1334652791-15833-8-git-send-email-manjunath.hadli@ti.com>
2012-04-17 9:46 ` [PATCH v2 07/13] davinci: vpif: add support to use videobuf_iolock() Laurent Pinchart
2012-05-11 5:26 ` Hadli, Manjunath
[not found] ` <1334652791-15833-6-git-send-email-manjunath.hadli@ti.com>
2012-04-17 10:02 ` [PATCH v2 05/13] davinci: vpif display: declare contiguous region of memory handled by dma_alloc_coherent Laurent Pinchart
2012-05-11 5:30 ` Hadli, Manjunath
2012-05-14 7:46 ` Laurent Pinchart
2012-06-07 11:15 ` Hadli, Manjunath
[not found] ` <1334652791-15833-2-git-send-email-manjunath.hadli@ti.com>
2012-04-17 10:06 ` [PATCH v2 01/13] davinci: vpif: add check for genuine interrupts in the isr Laurent Pinchart
2012-05-11 5:32 ` Hadli, Manjunath
2012-05-14 7:49 ` Laurent Pinchart
2012-06-07 11:25 ` Hadli, Manjunath
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).