| Message ID | 50A6C086.50208@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Sylwester, On Sat, Nov 17, 2012 at 2:39 AM, Sylwester Nawrocki <sylvester.nawrocki@gmail.com> 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 :)
diff --git a/drivers/media/platform/s3c-camif/camif-capture.c b/drivers/media/platform/s3c-camif/camif-capture.c index c2ecdcc..6401fdb 100644 --- a/drivers/media/platform/s3c-camif/camif-capture.c +++ b/drivers/media/platform/s3c-camif/camif-capture.c @@ -43,7 +43,7 @@ static int debug; module_param(debug, int, 0644); -/* Locking: called with vp->camif->slock held */ +/* Locking: called with vp->camif->slock spinlock held */ static void camif_cfg_video_path(struct camif_vp *vp) { WARN_ON(s3c_camif_get_scaler_config(vp, &vp->scaler)); @@ -64,16 +64,14 @@ static void camif_prepare_dma_offset(struct camif_vp *vp) f->dma_offset.initial, f->dma_offset.line); } +/* Locking: called with camif->slock spinlock held */ static int s3c_camif_hw_init(struct camif_dev *camif, struct camif_vp *vp) { const struct s3c_camif_variant *variant = camif->variant; - unsigned long flags; if (camif->sensor.sd == NULL || vp->out_fmt == NULL) return -EINVAL; - spin_lock_irqsave(&camif->slock, flags); - if (variant->ip_revision == S3C244X_CAMIF_IP_REV) camif_hw_clear_fifo_overflow(vp);