Re: [PATCH v5 04/49] media: davinci: vpif_display: release buffers in case start_streaming() call back fails

[Prabhakar Lad]

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-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/