Hello Uwe,
On 07/02/2011 10:09 PM, Uwe Kleine-König wrote:
> Hello Sylwester,
>
> thanks for your feedback. A few comments below. For the statements I
> don't reply to, you can consider a "OK, will be fixed in v2".
>
> On Fri, Jul 01, 2011 at 11:20:32PM +0200, Sylwester Nawrocki wrote:
>> On 06/29/2011 09:58 PM, Uwe Kleine-König wrote:
>>> + if (handled& heoimr)
>>> + return IRQ_HANDLED;
>>> + else
>>
>> else could be omitted
> I like the else, but don't care much.
sure, it was purely a personal preference.
>>> + if (rect->left< 0)
>>> + hwxpos = 0;
>>> + else
>>> + hwxpos = rect->left;
>>
>> Could be rewritten as:
>>
>> hwxpos = rect->left< 0 ? 0 : rect->left;
> could even be rewritten as
>
> hwxpos = max(rect->left, 0);
ok, I give up, couldn't make it any simpler;)
>
>>> +static void at91sam9x5_video_vb_wait_prepare(struct vb2_queue *q)
>>> +{
>>> + struct at91sam9x5_video_priv *priv =
>>> + container_of(q, struct at91sam9x5_video_priv, queue);
>>> + unsigned long flags;
>>> +
>>> + debug("cfgupdate=%d hwstate=%d cfgstate=%d\n",
>>> + priv->cfgupdate, priv->hwstate, priv->cfgstate);
>>> + debug("bufs=%p,%p\n", priv->cur.vb, priv->next.vb);
>>> + spin_lock_irqsave(&priv->lock, flags);
>>> +
>>> + at91sam9x5_video_handle_irqstat(priv);
>>> +
>>> + at91sam9x5_video_write32(priv, REG_HEOIER,
>>> + REG_HEOIxR_ADD | REG_HEOIxR_DMA |
>>> + REG_HEOIxR_UADD | REG_HEOIxR_UDMA |
>>> + REG_HEOIxR_VADD | REG_HEOIxR_VDMA);
>>
>> What the above two calls are intended to be doing ?
>
> handle_irqstat handles the eventual pending irqs. The second call
> enables irqs for "frame done" (..._DMA) and "new descriptor loaded"
> (..._ADD).
OK, so it looks to me like irqs are unmasked in wait_prepare and masked
back in wait_finish. I would try to move this logic to start_streaming and
the interrupt handler.
It seems this way too much dependant on when wait_prepare/wait_finish are
called by videobuf2. AFAIK those callbacks are not called in non-blocking
mode.
>
>>> +const struct vb2_ops at91sam9x5_video_vb_ops = {
>>> + .queue_setup = at91sam9x5_video_vb_queue_setup,
>>> +
>>> + .wait_prepare = at91sam9x5_video_vb_wait_prepare,
>>> + .wait_finish = at91sam9x5_video_vb_wait_finish,
>>
>> These 2 functions are intended to unlock and lock respectively the mutex
>> that is used to serialize ioctl handlers, in particular DQBUF.
>> I'm not sure if you're doing the right thing in
>> at91sam9x5_video_vb_wait_prepare/at91sam9x5_video_vb_wait_finish.
> I'm not taking a mutex for sure.
All right, so this needs to be changed. If you decide to add a file
operations mutex and protect each file operation individually in the driver,
rather than assigning a pointer to such mutex to struct video_device::lock
and let the core serialize file ops, then wait_prepare/wait_finish
could be omitted.
>
>>> +
>>> + .buf_prepare = at91sam9x5_video_vb_buf_prepare,
>>> + .buf_queue = at91sam9x5_video_vb_buf_queue,
>>> +};
Also if your driver is supposed to support write() method,
vidioc_streamon/vidioc_streamoff should be just a pass-through for
vb2_streamon/vb2_streamoff and the hardware control should happen in
start_streaming, stop_streaming callbacks.
I can't see a stop_streaming callback in your vb2 operations set.
It's been made mandatory recently, thus it would be good to add it.
>>> +
>>> +static int at91sam9x5_video_vidioc_querycap(struct file *filp,
>>> + void *fh, struct v4l2_capability *cap)
>>> +{
>>> + strcpy(cap->driver, DRIVER_NAME);
I would go for a strlcpy here, better to be safe than sorry. ;-)
>>> + cap->capabilities = V4L2_CAP_VIDEO_OUTPUT | V4L2_CAP_STREAMING |
>>> + V4L2_CAP_VIDEO_OVERLAY;
>>> +
>>> + /* XXX */
>>> + cap->version = 0;
>>
>> There is no need to set this field any more. It will be overwritten
>> with kernel versions in __video_do_ioctl(). See this for more details:
>> http://git.linuxtv.org/media_tree.git?a=commit;h=33436a81b0d4d1036ffcf0edb7e3bfa65d18ad08
> I saw the discussion on the ML, but missed that it was already
> committed.
>
>>> + cap->card[0] = '\0';
>>> + cap->bus_info[0] = '\0';
> I assume I need to fill these with more sensible values?
I think bus_info is not very useful for this driver and can be left as is.
As for cap->card, I'm not sure. Some drivers just just fill it in with
a video node name (/dev/video*), some are more creative.
Here is what the V4L2 specifications says:
http://linuxtv.org/downloads/v4l-dvb-apis/vidioc-querycap.html
--
Regards,
Sylwester
--
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
