Hi Sylwester,
On Sat, Nov 17, 2012 at 2:39 AM, Sylwester Nawrocki
<sylvester.nawrocki@xxxxxxxxx> wrote:
> Hi Alexey,
>
>
> On 11/16/2012 03:10 PM, Alexey Klimov wrote:
>>>>
>>>> +static int s3c_camif_hw_init(struct camif_dev *camif, struct camif_vp
>>>> *vp)
>>>> +{
>>>> + unsigned int ip_rev = camif->variant->ip_revision;
>>>> + unsigned long flags;
>>>> +
>>>> + if (camif->sensor.sd == NULL || vp->out_fmt == NULL)
>>>> + return -EINVAL;
>>>> +
>>>> + spin_lock_irqsave(&camif->slock, flags);
>>>> +
>>>> + if (ip_rev == S3C244X_CAMIF_IP_REV)
>>>> + camif_hw_clear_fifo_overflow(vp);
>>>> + camif_hw_set_camera_bus(camif);
>>>> + camif_hw_set_source_format(camif);
>>>> + camif_hw_set_camera_crop(camif);
>>>> + camif_hw_set_test_pattern(camif, camif->test_pattern->val);
>>>> + if (ip_rev == S3C6410_CAMIF_IP_REV)
>>>> + camif_hw_set_input_path(vp);
>>>> + camif_cfg_video_path(vp);
>>>> + vp->state&= ~ST_VP_CONFIG;
>>>>
>>>> +
>>>> + spin_unlock_irqrestore(&camif->slock, flags);
>>>> + return 0;
>>>> +}
>>>> +
>>>> +/*
>>>> + * Initialize the video path, only up from the scaler stage. The camera
>>>> + * input interface set up is skipped. This is useful to enable one of
>>>> the
>>>> + * video paths when the other is already running.
>>>> + */
>>>> +static int s3c_camif_hw_vp_init(struct camif_dev *camif, struct
>>>> camif_vp
>>>> *vp)
>>>> +{
>>>> + unsigned int ip_rev = camif->variant->ip_revision;
>>>> + unsigned long flags;
>>>> +
>>>> + if (vp->out_fmt == NULL)
>>>> + return -EINVAL;
>>>> +
>>>> + spin_lock_irqsave(&camif->slock, flags);
>>>> + camif_prepare_dma_offset(vp);
>>>> + if (ip_rev == S3C244X_CAMIF_IP_REV)
>>>> + camif_hw_clear_fifo_overflow(vp);
>>>> + camif_cfg_video_path(vp);
>>>> + if (ip_rev == S3C6410_CAMIF_IP_REV)
>>>> + camif_hw_set_effect(vp, false);
>>>> + vp->state&= ~ST_VP_CONFIG;
>>>>
>>>> +
>>>> + spin_unlock_irqrestore(&camif->slock, flags);
>>>> + return 0;
>>>> +}
>
> ...
>>>>
>>>> +/*
>>>> + * Reinitialize the driver so it is ready to start streaming again.
>>>> + * Return any buffers to vb2, perform CAMIF software reset and
>>>> + * turn off streaming at the data pipeline (sensor) if required.
>>>> + */
>>>> +static int camif_reinitialize(struct camif_vp *vp)
>>>> +{
>>>> + struct camif_dev *camif = vp->camif;
>>>> + struct camif_buffer *buf;
>>>> + unsigned long flags;
>>>> + bool streaming;
>>>> +
>>>> + spin_lock_irqsave(&camif->slock, flags);
>>>> + streaming = vp->state& ST_VP_SENSOR_STREAMING;
>>>> +
>>>> + vp->state&= ~(ST_VP_PENDING | ST_VP_RUNNING | ST_VP_OFF |
>>>>
>>>> + ST_VP_ABORTING | ST_VP_STREAMING |
>>>> + ST_VP_SENSOR_STREAMING | ST_VP_LASTIRQ);
>>>> +
>>>> + /* Release unused buffers */
>>>> + while (!list_empty(&vp->pending_buf_q)) {
>>>> + buf = camif_pending_queue_pop(vp);
>>>> + vb2_buffer_done(&buf->vb, VB2_BUF_STATE_ERROR);
>>>> + }
>>>> +
>>>> + while (!list_empty(&vp->active_buf_q)) {
>>>> + buf = camif_active_queue_pop(vp);
>>>> + vb2_buffer_done(&buf->vb, VB2_BUF_STATE_ERROR);
>>>> + }
>>>> +
>>>> + spin_unlock_irqrestore(&camif->slock, flags);
>>>> +
>>>> + if (!streaming)
>>>> + return 0;
>>>> +
>>>> + return sensor_set_streaming(camif, 0);
>>>> +}
>
> ...
>
>>>> +static int start_streaming(struct vb2_queue *vq, unsigned int count)
>>>> +{
>>>> + struct camif_vp *vp = vb2_get_drv_priv(vq);
>>>> + struct camif_dev *camif = vp->camif;
>>>> + unsigned long flags;
>>>> + int ret;
>>>> +
>>>> + /*
>>>> + * We assume the codec capture path is always activated
>>>> + * first, before the preview path starts streaming.
>>>> + * This is required to avoid internal FIFO overflow and
>>>> + * a need for CAMIF software reset.
>>>> + */
>>>> + spin_lock_irqsave(&camif->slock, flags);
>>
>>
>> Here.
>>
>>>>
>>>> +
>>>> + if (camif->stream_count == 0) {
>>>> + camif_hw_reset(camif);
>>>> + spin_unlock_irqrestore(&camif->slock, flags);
>>>> + ret = s3c_camif_hw_init(camif, vp);
>>>> + } else {
>>>> + spin_unlock_irqrestore(&camif->slock, flags);
>>>> + ret = s3c_camif_hw_vp_init(camif, vp);
>>>> + }
>>>> +
>>>> + if (ret< 0) {
>>>> + camif_reinitialize(vp);
>>>> + return ret;
>>>> + }
>>>> +
>>>> + spin_lock_irqsave(&camif->slock, flags);
>>
>>
>> Could you please check this function? Is it ok that you have double
>> spin_lock_irqsave()? I don't know may be it's okay. Also when you call
>> camif_reinitialize() you didn't call spin_unlock_irqrestore() before and
>> inside camif_reinitialize() you will also call spin_lock_irqsave()..
>
>
> Certainly with nested spinlock locking this code would have been useless.
> I suppose this is what you mean by "double spin_lock_irqsave()". Since
> it is known to work there must be spin_unlock_irqrestore() somewhere,
> before the second spin_lock_irqsave() above. Just look around with more
> focus ;)
You are right. I'm very sorry, i need to be more focus :)
--
Best regards, Klimov Alexey
--
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
