From patchwork Fri Nov 16 22:39:02 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Sylwester Nawrocki X-Patchwork-Id: 1757501 Return-Path: X-Original-To: patchwork-linux-media@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork2.kernel.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by patchwork2.kernel.org (Postfix) with ESMTP id EE444DF288 for ; Fri, 16 Nov 2012 22:39:10 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753501Ab2KPWjJ (ORCPT ); Fri, 16 Nov 2012 17:39:09 -0500 Received: from mail-ea0-f174.google.com ([209.85.215.174]:43066 "EHLO mail-ea0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753454Ab2KPWjI (ORCPT ); Fri, 16 Nov 2012 17:39:08 -0500 Received: by mail-ea0-f174.google.com with SMTP id e13so1259969eaa.19 for ; Fri, 16 Nov 2012 14:39:05 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=message-id:date:from:user-agent:mime-version:to:cc:subject :references:in-reply-to:content-type:content-transfer-encoding; bh=vuaK+5pjIXVTCQUMhwSSLU6D5dZnwqqsQNB4ButJfeQ=; b=u4tBkWTq75+cStEErd0hNrTHr4sJuupL2ftN4pGj5wQ0DvrRx9/iiwfnCnIS316JL3 4Upt2MwC9moStjeNc4bcNgOMTNPJy9ZFsRmzoMDTeb482F24x378ZO0fcRL+zKC13ygO 2HJqMZYSGUChH7pHZ9sonNYriorr3j+wumFztFw+gWQ5BSil6lhJ5wvSFBHcP825pbJx aESXvtkTw+DNyiOpOeKK44s4t4lVMFCPNyjdsFXUqRfF+ltJZuvycGKnR2gJXsfnGT9V CUZKyiBQWfPNGKa+xuFE77dWsmwxnBPMuuZmDVn8Qi/8QUGaiY/nduKvAr9Zj9v5yT38 WNQg== Received: by 10.14.176.68 with SMTP id a44mr1245496eem.1.1353105545091; Fri, 16 Nov 2012 14:39:05 -0800 (PST) Received: from [192.168.1.110] (178235118103.warszawa.vectranet.pl. [178.235.118.103]) by mx.google.com with ESMTPS id e1sm6216086eem.3.2012.11.16.14.39.03 (version=SSLv3 cipher=OTHER); Fri, 16 Nov 2012 14:39:04 -0800 (PST) Message-ID: <50A6C086.50208@gmail.com> Date: Fri, 16 Nov 2012 23:39:02 +0100 From: Sylwester Nawrocki User-Agent: Mozilla/5.0 (X11; Linux i686; rv:11.0) Gecko/20120412 Thunderbird/11.0.1 MIME-Version: 1.0 To: Alexey Klimov CC: linux-media@vger.kernel.org, dron0gus@gmail.com, tomasz.figa@gmail.com, oselas@community.pengutronix.de Subject: Re: [PATCH RFC v3 1/3] V4L: Add driver for S3C244X/S3C64XX SoC series camera interface References: <1353017115-11492-1-git-send-email-sylvester.nawrocki@gmail.com> <1353017115-11492-2-git-send-email-sylvester.nawrocki@gmail.com> In-Reply-To: Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-media@vger.kernel.org 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 ;) Nevertheless, it looks locking can be removed from functions s3c_camif_hw_init() and s3c_camif_vp_init(), those are called only from one place, where in addition the spinlock is already held. I'm going to squash following patch into that one: ----8<------ camif_hw_set_camera_bus(camif); @@ -88,7 +86,6 @@ static int s3c_camif_hw_init(struct camif_dev *camif, struct camif_vp *vp) camif_cfg_video_path(vp); vp->state &= ~ST_VP_CONFIG; - spin_unlock_irqrestore(&camif->slock, flags); return 0; } @@ -96,23 +93,20 @@ static int s3c_camif_hw_init(struct camif_dev *camif, struct camif_vp *vp) * 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. + * Locking: called with camif->slock spinlock held. */ 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); vp->state &= ~ST_VP_CONFIG; - - spin_unlock_irqrestore(&camif->slock, flags); return 0; } @@ -401,12 +395,11 @@ static int start_streaming(struct vb2_queue *vq, unsigned int count) 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); } + spin_unlock_irqrestore(&camif->slock, flags); if (ret < 0) { camif_reinitialize(vp); @@ -437,8 +430,8 @@ static int start_streaming(struct vb2_queue *vq, unsigned int count) return ret; } } - spin_unlock_irqrestore(&camif->slock, flags); + spin_unlock_irqrestore(&camif->slock, flags); return 0; } ---->8------ Thank you. --- Regards, Sylwester -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html 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);