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: 22+ 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 ` [PATCH 1/5] [media]: OMAP_VOUT: Fix check in reqbuf & mmap for buf_size allocation 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-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-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 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-16 10:00 ` [PATCH 4/5] [media] OMAP_VOUT: Add support for DSI panels Archit Taneja
2011-09-16 10:00 ` [PATCH 5/5] [media]: OMAP_VOUT: Don't trigger updates in omap_vout_probe 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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox