From: Archit Taneja <archit@ti.com>
To: "Hiremath, Vaibhav" <hvaibhav@ti.com>
Cc: "Valkeinen, Tomi" <tomi.valkeinen@ti.com>,
"linux-omap@vger.kernel.org" <linux-omap@vger.kernel.org>,
"Semwal, Sumit" <sumit.semwal@ti.com>,
"linux-media@vger.kernel.org" <linux-media@vger.kernel.org>
Subject: Re: [PATCH 2/5] [media]: OMAP_VOUT: CLEANUP: Remove redundant code from omap_vout_isr
Date: Wed, 21 Sep 2011 16:15:39 +0530 [thread overview]
Message-ID: <4E79C053.903@ti.com> (raw)
In-Reply-To: <19F8576C6E063C45BE387C64729E739404EC941E8B@dbde02.ent.ti.com>
Hi,
On Wednesday 21 September 2011 03:35 PM, Hiremath, Vaibhav wrote:
>
>> -----Original Message-----
>> From: Taneja, Archit
>> Sent: Friday, September 16, 2011 3:31 PM
>> To: Hiremath, Vaibhav
>> Cc: Valkeinen, Tomi; linux-omap@vger.kernel.org; Semwal, Sumit; linux-
>> media@vger.kernel.org; Taneja, Archit
>> Subject: [PATCH 2/5] [media]: OMAP_VOUT: CLEANUP: Remove redundant code
>> from omap_vout_isr
>>
>> Currently, there is a lot of redundant code is between DPI and VENC panels,
>> this
>> can be made common by moving out field/interlace specific code to a
>> separate
>> function called omapvid_handle_interlace_display(). There is no functional
>> change made.
>>
>> Signed-off-by: Archit Taneja<archit@ti.com>
>> ---
>> drivers/media/video/omap/omap_vout.c | 172 ++++++++++++++++-------------
>> -----
>> 1 files changed, 82 insertions(+), 90 deletions(-)
>>
>> diff --git a/drivers/media/video/omap/omap_vout.c
>> b/drivers/media/video/omap/omap_vout.c
>> index e14c82b..c5f2ea0 100644
>> --- a/drivers/media/video/omap/omap_vout.c
>> +++ b/drivers/media/video/omap/omap_vout.c
>> @@ -524,10 +524,50 @@ static int omapvid_apply_changes(struct
>> omap_vout_device *vout)
>> return 0;
>> }
>>
>> +static int omapvid_handle_interlace_display(struct omap_vout_device *vout,
>> + unsigned int irqstatus, struct timeval timevalue)
>> +{
>> + u32 fid;
>> +
>> + if (vout->first_int) {
>> + vout->first_int = 0;
>> + goto err;
>> + }
>> +
>> + if (irqstatus& DISPC_IRQ_EVSYNC_ODD)
>> + fid = 1;
>> + else if (irqstatus& DISPC_IRQ_EVSYNC_EVEN)
>> + fid = 0;
>> + else
>> + goto err;
>> +
>> + vout->field_id ^= 1;
>> + if (fid != vout->field_id) {
>> + /* reset field ID */
>> + vout->field_id = 0;
> [Hiremath, Vaibhav] You should check whether fid == 0 before resetting it.
>
>> + } else if (0 == fid) {
> [Hiremath, Vaibhav] This is not matching with the original code, probably I have to be more careful here. I need to spend more time here...
If you do a dry run of it you'll see that it does the same thing
functionally. If fid was 1, vout->field_id would have been 0 anyway.
So the check for fid == 0 looked a bit redundant to me. However, if you
think that doing this makes the code less clear, we can surely keep this
check.
>
>
>> + if (vout->cur_frm == vout->next_frm)
>> + goto err;
>> +
>> + vout->cur_frm->ts = timevalue;
>> + vout->cur_frm->state = VIDEOBUF_DONE;
>> + wake_up_interruptible(&vout->cur_frm->done);
>> + vout->cur_frm = vout->next_frm;
>> + } else {
>> + if (list_empty(&vout->dma_queue) ||
>> + (vout->cur_frm != vout->next_frm))
>> + goto err;
>> + }
>> +
>> + return vout->field_id;
>> +err:
>> + return 0;
>> +}
>> +
>> static void omap_vout_isr(void *arg, unsigned int irqstatus)
>> {
>> - int ret;
>> - u32 addr, fid;
>> + int ret, fid;
>> + u32 addr;
>> struct omap_overlay *ovl;
>> struct timeval timevalue;
>> struct omapvideo_info *ovid;
>> @@ -548,107 +588,59 @@ static void omap_vout_isr(void *arg, unsigned int
>> irqstatus)
>> spin_lock(&vout->vbq_lock);
>> do_gettimeofday(&timevalue);
>>
>> - if (cur_display->type != OMAP_DISPLAY_TYPE_VENC) {
>> - switch (cur_display->type) {
>> - case OMAP_DISPLAY_TYPE_DPI:
>> - if (!(irqstatus& (DISPC_IRQ_VSYNC | DISPC_IRQ_VSYNC2)))
>> - goto vout_isr_err;
>> - break;
>> - case OMAP_DISPLAY_TYPE_HDMI:
>> - if (!(irqstatus& DISPC_IRQ_EVSYNC_EVEN))
>> - goto vout_isr_err;
>> - break;
>> - default:
>> + switch (cur_display->type) {
>> + case OMAP_DISPLAY_TYPE_DPI:
>> + if (!(irqstatus& (DISPC_IRQ_VSYNC | DISPC_IRQ_VSYNC2)))
>> goto vout_isr_err;
>> - }
>> - if (!vout->first_int&& (vout->cur_frm != vout->next_frm)) {
>> - vout->cur_frm->ts = timevalue;
>> - vout->cur_frm->state = VIDEOBUF_DONE;
>> - wake_up_interruptible(&vout->cur_frm->done);
>> - vout->cur_frm = vout->next_frm;
>> - }
>> - vout->first_int = 0;
>> - if (list_empty(&vout->dma_queue))
>> + break;
>> + case OMAP_DISPLAY_TYPE_VENC:
>> + fid = omapvid_handle_interlace_display(vout, irqstatus,
>> + timevalue);
>> + if (!fid)
>> goto vout_isr_err;
> [Hiremath, Vaibhav]
> Have you tested TV out functionality?
I haven't checked it yet to be totally honest. Its hard to find a VENC
TV! I wanted to anyway get some kind of Ack from you before starting to
test this. Since you also feel that this clean up is needed, I'll start
testing this out :)
>
>> + break;
>> + case OMAP_DISPLAY_TYPE_HDMI:
>> + if (!(irqstatus& DISPC_IRQ_EVSYNC_EVEN))
>> + goto vout_isr_err;
>> + break;
>> + default:
>> + goto vout_isr_err;
>> + }
>>
>> - vout->next_frm = list_entry(vout->dma_queue.next,
>> - struct videobuf_buffer, queue);
>> - list_del(&vout->next_frm->queue);
>> -
>> - vout->next_frm->state = VIDEOBUF_ACTIVE;
>> -
>> - addr = (unsigned long) vout->queued_buf_addr[vout->next_frm-
>>> i]
>> - + vout->cropped_offset;
>> + if (!vout->first_int&& (vout->cur_frm != vout->next_frm)) {
>> + vout->cur_frm->ts = timevalue;
>> + vout->cur_frm->state = VIDEOBUF_DONE;
>> + wake_up_interruptible(&vout->cur_frm->done);
>> + vout->cur_frm = vout->next_frm;
>> + }
>>
>> - /* First save the configuration in ovelray structure */
>> - ret = omapvid_init(vout, addr);
>> - if (ret)
>> - printk(KERN_ERR VOUT_NAME
>> - "failed to set overlay info\n");
>> - /* Enable the pipeline and set the Go bit */
>> - ret = omapvid_apply_changes(vout);
>> - if (ret)
>> - printk(KERN_ERR VOUT_NAME "failed to change mode\n");
>> - } else {
>> + vout->first_int = 0;
>> + if (list_empty(&vout->dma_queue))
>> + goto vout_isr_err;
>>
>> - if (vout->first_int) {
>> - vout->first_int = 0;
>> - goto vout_isr_err;
>> - }
>> - if (irqstatus& DISPC_IRQ_EVSYNC_ODD)
>> - fid = 1;
>> - else if (irqstatus& DISPC_IRQ_EVSYNC_EVEN)
>> - fid = 0;
>> - else
>> - goto vout_isr_err;
>> + vout->next_frm = list_entry(vout->dma_queue.next,
>> + struct videobuf_buffer, queue);
>> + list_del(&vout->next_frm->queue);
>>
>> - vout->field_id ^= 1;
>> - if (fid != vout->field_id) {
>> - if (0 == fid)
>> - vout->field_id = fid;
>> + vout->next_frm->state = VIDEOBUF_ACTIVE;
>>
>> - goto vout_isr_err;
>> - }
>> - if (0 == fid) {
>> - if (vout->cur_frm == vout->next_frm)
>> - goto vout_isr_err;
>> -
>> - vout->cur_frm->ts = timevalue;
>> - vout->cur_frm->state = VIDEOBUF_DONE;
>> - wake_up_interruptible(&vout->cur_frm->done);
>> - vout->cur_frm = vout->next_frm;
>> - } else if (1 == fid) {
>> - if (list_empty(&vout->dma_queue) ||
>> - (vout->cur_frm != vout->next_frm))
>> - goto vout_isr_err;
>> -
>> - vout->next_frm = list_entry(vout->dma_queue.next,
>> - struct videobuf_buffer, queue);
>> - list_del(&vout->next_frm->queue);
>> -
>> - vout->next_frm->state = VIDEOBUF_ACTIVE;
>> - addr = (unsigned long)
>> - vout->queued_buf_addr[vout->next_frm->i] +
>> - vout->cropped_offset;
>> - /* First save the configuration in ovelray structure */
>> - ret = omapvid_init(vout, addr);
>> - if (ret)
>> - printk(KERN_ERR VOUT_NAME
>> - "failed to set overlay info\n");
>> - /* Enable the pipeline and set the Go bit */
>> - ret = omapvid_apply_changes(vout);
>> - if (ret)
>> - printk(KERN_ERR VOUT_NAME
>> - "failed to change mode\n");
>> - }
>> + addr = (unsigned long) vout->queued_buf_addr[vout->next_frm->i]
>> + + vout->cropped_offset;
>>
>> - }
>> + /* First save the configuration in ovelray structure */
>> + ret = omapvid_init(vout, addr);
>> + if (ret)
>> + printk(KERN_ERR VOUT_NAME
>> + "failed to set overlay info\n");
>> + /* Enable the pipeline and set the Go bit */
>> + ret = omapvid_apply_changes(vout);
>> + if (ret)
>> + printk(KERN_ERR VOUT_NAME "failed to change mode\n");
>>
>> vout_isr_err:
>> spin_unlock(&vout->vbq_lock);
>> }
> [Hiremath, Vaibhav] Overall this clean-up was required, thanks for working on this patch.
Thanks for the review!
Archit
>
> Thanks,
> Vaibhav
>>
>> -
>> /* Video buffer call backs */
>>
>> /*
>> --
>> 1.7.1
>
>
next prev parent reply other threads:[~2011-09-21 10:43 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-09-16 10:00 [PATCH 0/5] [media]: OMAP_VOUT: Misc fixes and cleanup patches for 3.2 Archit Taneja
2011-09-16 10:00 ` Archit Taneja
2011-09-16 10:00 ` [PATCH 1/5] [media]: OMAP_VOUT: Fix check in reqbuf & mmap for buf_size allocation Archit Taneja
2011-09-16 10:00 ` Archit Taneja
2011-09-21 8:40 ` Hiremath, Vaibhav
2011-09-21 10:49 ` Archit Taneja
2011-09-16 10:00 ` [PATCH 2/5] [media]: OMAP_VOUT: CLEANUP: Remove redundant code from omap_vout_isr Archit Taneja
2011-09-16 10:00 ` Archit Taneja
2011-09-21 10:05 ` Hiremath, Vaibhav
2011-09-21 10:45 ` Archit Taneja [this message]
2011-09-26 5:34 ` Archit Taneja
2011-09-16 10:00 ` [PATCH 3/5] [media]: OMAP_VOUT: Fix VSYNC IRQ handling in omap_vout_isr Archit Taneja
2011-09-16 10:00 ` Archit Taneja
2011-09-21 13:34 ` Hiremath, Vaibhav
2011-09-22 6:15 ` Archit Taneja
2011-09-26 10:19 ` Hiremath, Vaibhav
[not found] ` <CAB2ybb8ab9jSFB1J_CQfObB11QcdtQ=6Kf9zdbg0v5Jckf09sw@mail.gmail.com>
[not found] ` <CAB2ybb-rZgDvS9Bo6AJF=KVd0irXHa0S0LrPJ=SWr0daJ6gX1w@mail.gmail.com>
2011-09-27 5:41 ` Semwal, Sumit
2011-09-27 5:41 ` Semwal, Sumit
2011-09-27 6:39 ` Hiremath, Vaibhav
2011-09-27 6:49 ` Tomi Valkeinen
2011-09-27 6:54 ` Hiremath, Vaibhav
2011-09-27 7:01 ` Archit Taneja
2011-09-27 7:09 ` Hiremath, Vaibhav
2011-09-27 7:05 ` Tomi Valkeinen
2011-09-27 6:51 ` Semwal, Sumit
2011-09-27 6:51 ` Semwal, Sumit
2011-09-16 10:00 ` [PATCH 4/5] [media] OMAP_VOUT: Add support for DSI panels Archit Taneja
2011-09-16 10:00 ` Archit Taneja
2011-09-16 10:00 ` [PATCH 5/5] [media]: OMAP_VOUT: Don't trigger updates in omap_vout_probe Archit Taneja
2011-09-16 10:00 ` Archit Taneja
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4E79C053.903@ti.com \
--to=archit@ti.com \
--cc=hvaibhav@ti.com \
--cc=linux-media@vger.kernel.org \
--cc=linux-omap@vger.kernel.org \
--cc=sumit.semwal@ti.com \
--cc=tomi.valkeinen@ti.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.