Hi Hans,
Thanks for the review.
On Fri, May 23, 2014 at 2:05 PM, Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
> On 05/16/2014 03:33 PM, Lad, Prabhakar wrote:
>> From: "Lad, Prabhakar" <prabhakar.csengg@xxxxxxxxx>
>>
>> this patch adds support to release the buffer by calling
>> vb2_buffer_done(), with state marked as VB2_BUF_STATE_QUEUED
>> if start_streaming() call back fails.
>>
>> Signed-off-by: Lad, Prabhakar <prabhakar.csengg@xxxxxxxxx>
>> ---
>> drivers/media/platform/davinci/vpif_display.c | 42 +++++++++++++++----------
>> 1 file changed, 26 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/media/platform/davinci/vpif_display.c b/drivers/media/platform/davinci/vpif_display.c
>> index 8bb9f02..1a17a45 100644
>> --- a/drivers/media/platform/davinci/vpif_display.c
>> +++ b/drivers/media/platform/davinci/vpif_display.c
>> @@ -196,26 +196,16 @@ static int vpif_start_streaming(struct vb2_queue *vq, unsigned int count)
>> struct channel_obj *ch = vb2_get_drv_priv(vq);
>> struct common_obj *common = &ch->common[VPIF_VIDEO_INDEX];
>> struct vpif_params *vpif = &ch->vpifparams;
>> - unsigned long addr = 0;
>> - unsigned long flags;
>> + struct vpif_disp_buffer *buf, *tmp;
>> + unsigned long addr, flags;
>> int ret;
>>
>> spin_lock_irqsave(&common->irqlock, flags);
>>
>> - /* Get the next frame from the buffer queue */
>> - common->next_frm = common->cur_frm =
>> - list_entry(common->dma_queue.next,
>> - struct vpif_disp_buffer, list);
>> -
>> - list_del(&common->cur_frm->list);
>> - spin_unlock_irqrestore(&common->irqlock, flags);
>> - /* Mark state of the current frame to active */
>> - common->cur_frm->vb.state = VB2_BUF_STATE_ACTIVE;
>> -
>> /* Initialize field_id and started member */
>> ch->field_id = 0;
>> common->started = 1;
>> - addr = vb2_dma_contig_plane_dma_addr(&common->cur_frm->vb, 0);
>> +
>> /* Calculate the offset for Y and C data in the buffer */
>> vpif_calculate_offsets(ch);
>>
>> @@ -225,7 +215,8 @@ static int vpif_start_streaming(struct vb2_queue *vq, unsigned int count)
>> || (!ch->vpifparams.std_info.frm_fmt
>> && (common->fmt.fmt.pix.field == V4L2_FIELD_NONE))) {
>> vpif_err("conflict in field format and std format\n");
>> - return -EINVAL;
>> + ret = -EINVAL;
>> + goto err;
>> }
>>
>> /* clock settings */
>> @@ -234,17 +225,28 @@ static int vpif_start_streaming(struct vb2_queue *vq, unsigned int count)
>> ycmux_mode, ch->vpifparams.std_info.hd_sd);
>> if (ret < 0) {
>> vpif_err("can't set clock\n");
>> - return ret;
>> + goto err;
>> }
>> }
>>
>> /* set the parameters and addresses */
>> ret = vpif_set_video_params(vpif, ch->channel_id + 2);
>> if (ret < 0)
>> - return ret;
>> + goto err;
>>
>> common->started = ret;
>> vpif_config_addr(ch, ret);
>> + /* Get the next frame from the buffer queue */
>> + common->next_frm = common->cur_frm =
>> + list_entry(common->dma_queue.next,
>> + struct vpif_disp_buffer, list);
>> +
>> + list_del(&common->cur_frm->list);
>> + spin_unlock_irqrestore(&common->irqlock, flags);
>> + /* Mark state of the current frame to active */
>> + common->cur_frm->vb.state = VB2_BUF_STATE_ACTIVE;
>
> There is no need to set this, all buffers queued to the driver are always in state
> ACTIVE. The vb2 core sets that for you. In general drivers never need to change the
> state manually.
>
> It happens twice in this driver and in both cases the assignment can be removed.
>
OK, will drop this.
Regards,
--Prabhakar Lad
--
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
