Re: [PATCH v2 06/11] v4l: Add support for omap4iss driver

[Sakari Ailus <sakari.ailus@iki.fi>]

Hi Sergio,

Aguirre, Sergio wrote:
>> On Wed, Nov 30, 2011 at 06:14:55PM -0600, Sergio Aguirre wrote:
...
>>> +/*
>>> + * iss_save_ctx - Saves ISS context.
>>> + * @iss: OMAP4 ISS device
>>> + *
>>> + * Routine for saving the context of each module in the ISS.
>>> + */
>>> +static void iss_save_ctx(struct iss_device *iss)
>>> +{
>>> +     iss_save_context(iss, iss_reg_list);
>>> +}
>>
>> Do you really, really need to save context related data? Couldn't you
>> initialise the device the usual way when e.g. returning from suspended
>> state?
>
> I'll actually have to revisit this more carefuly. I haven't really
> tested Runtime PM on this.
>
> I think I'll remove this for now, until i come up with something better.

Usually it's best to recreate the configuration the same way the driver did
it in the first place. Some registers have side effects so that restoring
them requires extra care.

>>> +/*
>>> + * iss_restore_ctx - Restores ISS context.
>>> + * @iss: OMAP4 ISS device
>>> + *
>>> + * Routine for restoring the context of each module in the ISS.
>>> + */
>>> +static void iss_restore_ctx(struct iss_device *iss)
>>> +{
>>> +     iss_restore_context(iss, iss_reg_list);
>>> +}
>>> +

...

>>> +/*
>>> + * csi2_irq_ctx_set - Enables CSI2 Context IRQs.
>>> + * @enable: Enable/disable CSI2 Context interrupts
>>> + */
>>> +static void csi2_irq_ctx_set(struct iss_csi2_device *csi2, int enable)
>>> +{
>>> +     u32 reg = CSI2_CTX_IRQ_FE;
>>> +     int i;
>>> +
>>> +     if (csi2->use_fs_irq)
>>> +             reg |= CSI2_CTX_IRQ_FS;
>>> +
>>> +     for (i = 0; i<  8; i++) {
>>
>> 8 == number of contexts?
>
> Yes. This loops from 0 to 7.
>
> Are you implying that I need to add a define to this?

I think something that tells it is the number of contexts would be nicer.

...

>>> +     writel(readl(csi2->regs1 + CSI2_SYSCONFIG) |
>>> +             CSI2_SYSCONFIG_SOFT_RESET,
>>> +             csi2->regs1 + CSI2_SYSCONFIG);
>>> +
>>> +     do {
>>> +             reg = readl(csi2->regs1 + CSI2_SYSSTATUS)&
>>> +                                 CSI2_SYSSTATUS_RESET_DONE;
>>> +             if (reg == CSI2_SYSSTATUS_RESET_DONE)
>>> +                     break;
>>> +             soft_reset_retries++;
>>> +             if (soft_reset_retries<  5)
>>> +                     udelay(100);
>>
>> How about usleep_range() here? Or omit such a long busyloop. Hardware
>> typically resets quite fast.
>
> I guess i'll try to fine-tune this then.

I think it's fine to replace it with usleep_range() for now. Fine
optimisations can be left for later if really needed.

...

>>> +static void csi2_print_status(struct iss_csi2_device *csi2)
>>> +{
>>> +     struct iss_device *iss = csi2->iss;
>>> +
>>> +     if (!csi2->available)
>>> +             return;
>>> +
>>> +     dev_dbg(iss->dev, "-------------CSI2 Register dump-------------\n");
>>> +
>>> +     CSI2_PRINT_REGISTER(iss, csi2->regs1, SYSCONFIG);
>>> +     CSI2_PRINT_REGISTER(iss, csi2->regs1, SYSSTATUS);
>>> +     CSI2_PRINT_REGISTER(iss, csi2->regs1, IRQENABLE);
>>> +     CSI2_PRINT_REGISTER(iss, csi2->regs1, IRQSTATUS);
>>> +     CSI2_PRINT_REGISTER(iss, csi2->regs1, CTRL);
>>> +     CSI2_PRINT_REGISTER(iss, csi2->regs1, DBG_H);
>>> +     CSI2_PRINT_REGISTER(iss, csi2->regs1, COMPLEXIO_CFG);
>>> +     CSI2_PRINT_REGISTER(iss, csi2->regs1, COMPLEXIO_IRQSTATUS);
>>> +     CSI2_PRINT_REGISTER(iss, csi2->regs1, SHORT_PACKET);
>>> +     CSI2_PRINT_REGISTER(iss, csi2->regs1, COMPLEXIO_IRQENABLE);
>>> +     CSI2_PRINT_REGISTER(iss, csi2->regs1, DBG_P);
>>> +     CSI2_PRINT_REGISTER(iss, csi2->regs1, TIMING);
>>> +     CSI2_PRINT_REGISTER(iss, csi2->regs1, CTX_CTRL1(0));
>>> +     CSI2_PRINT_REGISTER(iss, csi2->regs1, CTX_CTRL2(0));
>>> +     CSI2_PRINT_REGISTER(iss, csi2->regs1, CTX_DAT_OFST(0));
>>> +     CSI2_PRINT_REGISTER(iss, csi2->regs1, CTX_PING_ADDR(0));
>>> +     CSI2_PRINT_REGISTER(iss, csi2->regs1, CTX_PONG_ADDR(0));
>>> +     CSI2_PRINT_REGISTER(iss, csi2->regs1, CTX_IRQENABLE(0));
>>> +     CSI2_PRINT_REGISTER(iss, csi2->regs1, CTX_IRQSTATUS(0));
>>> +     CSI2_PRINT_REGISTER(iss, csi2->regs1, CTX_CTRL3(0));
>>> +
>>> +     dev_dbg(iss->dev, "--------------------------------------------\n");
>>
>> _If_ this is user-triggered, you might want to consider using debugfs for
>> the same. Just FYI.
>
> Ok. I'll see what can I do.

Just FYI. :-) Thinking about this agian, debugfs might not go well with this
since the prints may be triggered by the driver.

...

>>> +     /* Skip interrupts until we reach the frame skip count. The CSI2 will be
>>> +      * automatically disabled, as the frame skip count has been programmed
>>> +      * in the CSI2_CTx_CTRL1::COUNT field, so reenable it.
>>> +      *
>>> +      * It would have been nice to rely on the FRAME_NUMBER interrupt instead
>>> +      * but it turned out that the interrupt is only generated when the CSI2
>>> +      * writes to memory (the CSI2_CTx_CTRL1::COUNT field is decreased
>>> +      * correctly and reaches 0 when data is forwarded to the video port only
>>> +      * but no interrupt arrives). Maybe a CSI2 hardware bug.
>>> +      */
>>> +     if (csi2->frame_skip) {
>>> +             csi2->frame_skip--;
>>> +             if (csi2->frame_skip == 0) {
>>> +                     ctx->format_id = csi2_ctx_map_format(csi2);
>>> +                     csi2_ctx_config(csi2, ctx);
>>
>> Is configuration of the context needed elsewhere than in streamon? What
>> changes while streaming?
>
> Nothing I think...
>
> Same thing is done for OMAP3, so I tried to keep the driver as similar
> as possible.

Ok. Perhaps this could be fixed for OMAP 3, too. :-)

> I'll try removing this then.
>

...

>>> +/* This is not an exhaustive list */
>>> +enum iss_csi2_pix_formats {
>>> +     CSI2_PIX_FMT_OTHERS = 0,
>>> +     CSI2_PIX_FMT_YUV422_8BIT = 0x1e,
>>> +     CSI2_PIX_FMT_YUV422_8BIT_VP = 0x9e,
>>> +     CSI2_PIX_FMT_RAW10_EXP16 = 0xab,
>>> +     CSI2_PIX_FMT_RAW10_EXP16_VP = 0x12f,
>>> +     CSI2_PIX_FMT_RAW8 = 0x2a,
>>> +     CSI2_PIX_FMT_RAW8_DPCM10_EXP16 = 0x2aa,
>>> +     CSI2_PIX_FMT_RAW8_DPCM10_VP = 0x32a,
>>> +     CSI2_PIX_FMT_RAW8_VP = 0x12a,
>>> +     CSI2_USERDEF_8BIT_DATA1_DPCM10_VP = 0x340,
>>> +     CSI2_USERDEF_8BIT_DATA1_DPCM10 = 0x2c0,
>>> +     CSI2_USERDEF_8BIT_DATA1 = 0x40,
>>
>> What are the USERDEF formats?
>
> Again, inherited and adapted these from OMAP3 driver.
>
> Should I delete them?

It's fine to keep them for now I guess --- Laurent must know what they
actually are. :-) Looks like CSI-2 pixel format codes (plus VP control, I
guess).

...

>>> +struct iss_video {
>>> +     struct video_device video;
>>> +     enum v4l2_buf_type type;
>>> +     struct media_pad pad;
>>> +
>>> +     struct mutex mutex;             /* format and crop settings */
>>> +     atomic_t active;
>>> +
>>> +     struct iss_device *iss;
>>> +
>>> +     unsigned int capture_mem;
>>> +     unsigned int bpl_alignment;     /* alignment value */
>>> +     unsigned int bpl_zero_padding;  /* whether the alignment is optional */
>>> +     unsigned int bpl_max;           /* maximum bytes per line value */
>>> +     unsigned int bpl_value;         /* bytes per line value */
>>> +     unsigned int bpl_padding;       /* padding at end of line */
>>> +
>>> +     /* Entity video node streaming */
>>> +     unsigned int streaming:1;
>>> +
>>> +     /* Pipeline state */
>>> +     struct iss_pipeline pipe;
>>> +     struct mutex stream_lock;       /* pipeline and stream states */
>>> +
>>> +     /* Video buffers queue */
>>> +     struct vb2_queue *queue;
>>> +     spinlock_t qlock;       /* Spinlock for dmaqueue */
>>> +     struct list_head dmaqueue;
>>> +     enum iss_video_dmaqueue_flags dmaqueue_flags;
>>> +     struct vb2_alloc_ctx *alloc_ctx;
>>> +
>>> +     const struct iss_video_operations *ops;
>>> +};
>>> +
>>> +#define to_iss_video(vdev)   container_of(vdev, struct iss_video, video)
>>> +
>>> +struct iss_video_fh {
>>> +     struct v4l2_fh vfh;
>>> +     struct iss_video *video;
>>> +     struct vb2_queue queue;
>>> +     struct v4l2_format format;
>>
>> Format shouldn't be part of the file handle anymore. There was a reason for
>> it in the past before PREPARE_BUF --- which is also supported by videobuf2.
>
> Ok. So should I just remove it completely?
>
> Sorry if i'm not understanding this PREPARE_BUF thing... What should I
> have there?

PREPARE_BUF prepares the buffer for the use in the device's DMA engine. This
is exactly what QBUF does, except that PREPARE_BUF doesn't put the buffer to
the driver's queue (calling QBUF will then do that, and only that).

Together with CREATE_BUFS this allows having different kinds of buffers in
the same queue. This didn't use to be possible.

Think of sets of buffers for still capture and viewfinder. They can now
co-exist in the same queue.

Kind regards,

-- 
Sakari Ailus
sakari.ailus@iki.fi