Hi, I was confused the source and destination. I agree with you mostly. In my opinion, the way how to notify the resolution change is return value of the DQBUF. But if we use DQBUF, there are some problem as below. DQBUF means that the buffer has been operated already and application will have the buffer's right. Although application new QBUF destination buffers after changing destination(CAPTURE), driver cannot re-decode the I-frame which has the resolution change information because the I-frame has been dequeued already. If application re-QBUF the buffer, the buffer sequence will be out of order as below. Original: I -> B -> B ..... Out of order: missed -> B -> B -> I ..... How do you think about the following sequence. 1. getting the resolution change from the MFC H/W 2. copy the buffer to driver's internal memory. 3. send the result with DQBUF 4. changing destination buffers by application 5. QBUF for new destination buffers 6. in the first vb2_try_schedule re-decode the driver's internal buffer instead of the B frame. 7. in the next vb2_try_schedule decode the B frame in order. I also welcome your comments. Best regards, > -----Original Message----- > From: Kamil Debski [mailto:k.debski@xxxxxxxxxxx] > Sent: Friday, January 14, 2011 4:24 PM > To: 'Jonghun Han'; linux-media@xxxxxxxxxxxxxxx; linux-samsung- > soc@xxxxxxxxxxxxxxx > Cc: Marek Szyprowski; pawel@xxxxxxxxxx; kyungmin.park@xxxxxxxxxxx; > jaeryul.oh@xxxxxxxxxxx; kgene.kim@xxxxxxxxxxx > Subject: RE: [RFC/PATCH v6 3/4] MFC: Add MFC 5.1 V4L2 driver > > Hi, > > > -----Original Message----- > > From: Jonghun Han [mailto:jonghun.han@xxxxxxxxxxx] > > > > Hi, > > > > Kamil Debski wrote: > > > > <snip> > > > > > +/* Reqeust buffers */ > > > +static int vidioc_reqbufs(struct file *file, void *priv, > > > + struct v4l2_requestbuffers > > *reqbufs) > > > +{ > > > + struct s5p_mfc_dev *dev = video_drvdata(file); > > > + struct s5p_mfc_ctx *ctx = priv; > > > + int ret = 0; > > > + unsigned long flags; > > > + > > > + mfc_debug_enter(); > > > + mfc_debug("Memory type: %d\n", reqbufs->memory); > > > + if (reqbufs->memory != V4L2_MEMORY_MMAP) { > > > + mfc_err("Only V4L2_MEMORY_MAP is supported.\n"); > > > + return -EINVAL; > > > + } > > > + if (reqbufs->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) { > > > + /* Can only request buffers after an instance has been > > opened.*/ > > > + if (ctx->state == MFCINST_DEC_GOT_INST) { > > > + /* Decoding */ > > > + if (ctx->output_state != QUEUE_FREE) { > > > + mfc_err("Bufs have already been > > requested.\n"); > > > + return -EINVAL; > > > + } > > > + ret = vb2_reqbufs(&ctx->vq_src, reqbufs); > > > + if (ret) { > > > + mfc_err("vb2_reqbufs on output failed.\n"); > > > + return ret; > > > + } > > > + mfc_debug("vb2_reqbufs: %d\n", ret); > > > + ctx->output_state = QUEUE_BUFS_REQUESTED; > > > + } > > > + } else if (reqbufs->type == > > > V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) { > > > + if (ctx->capture_state != QUEUE_FREE) { > > > + mfc_err("Bufs have already been requested.\n"); > > > + return -EINVAL; > > > + } > > > + ctx->capture_state = QUEUE_BUFS_REQUESTED; > > > + ret = vb2_reqbufs(&ctx->vq_dst, reqbufs); > > > + if (ret) { > > > + mfc_err("vb2_reqbufs on capture failed.\n"); > > > + return ret; > > > + } > > > + if (reqbufs->count < ctx->dpb_count) { > > > + mfc_err("Not enough buffers allocated.\n"); > > > + reqbufs->count = 0; > > > + ret = vb2_reqbufs(&ctx->vq_dst, reqbufs); > > > + return -ENOMEM; > > > + } > > > + ctx->total_dpb_count = reqbufs->count; > > > + ret = s5p_mfc_alloc_dec_buffers(ctx); > > > + if (ret) { > > > + mfc_err("Failed to allocate decoding buffers.\n"); > > > + reqbufs->count = 0; > > > + ret = vb2_reqbufs(&ctx->vq_dst, reqbufs); > > > + return -ENOMEM; > > > + } > > > + if (ctx->dst_bufs_cnt == ctx->total_dpb_count) { > > > + ctx->capture_state = QUEUE_BUFS_MMAPED; > > > + } else { > > > + mfc_err("Not all buffers passed to buf_init.\n"); > > > + reqbufs->count = 0; > > > + ret = vb2_reqbufs(&ctx->vq_dst, reqbufs); > > > + s5p_mfc_release_dec_buffers(ctx); > > > + return -ENOMEM; > > > + } > > > + if (s5p_mfc_ctx_ready(ctx)) { > > > + spin_lock_irqsave(&dev->condlock, flags); > > > + set_bit(ctx->num, &dev->ctx_work_bits); > > > + spin_unlock_irqrestore(&dev->condlock, flags); > > > + } > > > + s5p_mfc_try_run(dev); > > > + s5p_mfc_wait_for_done_ctx(ctx, > > > + > > > S5P_FIMV_R2H_CMD_INIT_BUFFERS_RET, 1); > > > + } > > > + mfc_debug_leave(); > > > + return ret; > > > +} > > > > I don't know how to handle the followings. > > > > So I suggest that in reqbufs->type == V4L2_BUF_TYPE_VIDEO_OUTPUT case, > > how about return -EINVAL if reqbufs->count is bigger than 1. > > > > Because if reqbufs->count is bigger than 1, it is hard to handle the > > encoded > > input stream. > > > > For example: Dynamic resolution change > > Dynamic resolution change means that resolution can be changed at any > > I-frame with header on the fly during streaming. > > > > MFC H/W can detect it after getting decoding command from the driver. > > If the dynamic resolution change is detected by MFC H/W, > > driver should let application know the fact to do the following > > Sequence 1 > > by application. > > > > Sequence 1: > > streamoff -> munmap -> reqbufs(0) -> S_FMT(changed resolution) -> > > querybuf > > -> mmap -> re-QBUF with the I-frame -> STREAMON -> ... > > > > Why it is hard to handle the encoded input stream in multiple input > > stream > > case is the following Sequence 2. > > > > Sequence 2: > > QBUF(0) -> QBUF(1: resolution changed I-frame) -> QBUF(2: already > > changed) > > -> QBUF(3: already changed) -> DQBUF(0) -> DQBUF(1): return fail -> ... > > > > Application cannot know the resolution change in the QBUF ioctl. > > Driver will return 0 at the QBUF because all parameters are fine. > > But after sending the decode command to MFC H/W, driver can know that > > the > > I-frame needs to change resolution. > > In that case driver can return error at the DQBUF of the buffer. > > > > In the sequence 2, application can know the resolution change in the > > DQBUF(1). > > So the application should re-QBUF the buffer 2, 3 after Sequence 1. > > It is hard to re-control the buffers which are already queued in the > > point > > of application. > > Because in the reqbufs(0) buffers will be freed. > > So application has to copy them to the temporary buffer to re-QBUF > > after > > Sequence 1. > > > > How can we solve this case ? > > There are two buffer queues - the OUTPUT is the queue for the compressed > source. I don't see the need to do anything with this queue when resolution > is changed. > > There could be 3 src buffers queued for example. Let's say the first is an > I-frame > with changed resolution. This does not affect the following source buffers. > I > agree with you that it will have impact on the destination (CAPTURE) > buffers. > The problem is how to notify the application that the resolution has been > changed. > > After the application is notified by the driver that resolution has been > changed it has to do the following: > 1. DQBUF all the remaining destination CAPTURE buffers (with old resolution) > 2. Do stream off on CAPUTRE > 3. unmap all the CAPTURE buffers > 4. REQBUFS(0) on CAPTURE > 5. G_FMT on CAPTURE to get the new resolution > 6. REQBUFS(n) on CAPTURE > 7. mmap the CAPTURE buffers > 8. QBUF all the new CAPTURE buffers > 9. Do stream on on CAPTURE > > As you can see, the OUTPUT queue has not been modified. All the 3 source > buffers > are still queued until after step 9 when the processing restarts. > > From the driver perspective it looks like this: > a) After it has received DISP_STATUS [5:4] != 0 it sends the > FRAME_START_REALLOC > command. Then it behave the same as if the stream was finished - running > FRAME_START > and returning the remaining buffers. This is the step 1 for application > described above. > b) When no more buffers are left (DISP_STATUS[2:0]= 3) it has to notify the > application > that the resolution have changed. I will discuss how to do it below. > c) The application was notified and completed steps 2-4, at this time the > driver has to > reinitialize the stream. Here it will use the source buffer that had > resolution change > again with command INIT_CODEC. > d) The instance is reinitialized, and new resolution is read from MFC. The > application now > completes steps 5-9. > e) As destination (CAPTURE) buffers are now queued the driver continues > decoding. > FRAME_START command is issued for the source buffer that had resolution > change and it is > decoded. This is the place when this source buffer is marked as done and it > can be dequeued > by the application. > > As you can see - there was no need to reinitialize the OUTPUT queue. Only > CAPTURE, so there > is no need to restrict number of source (OUTPUT) buffers to 1. > > The question is how to notify the application. I think I could be done same > as end of stream > notification - by returning a buffer with size equal to 0. > If the application knows that source stream has ended, and queued a source > buffer of size 0 > to notify the driver - then a destination buffer of size 0 means that > decoding is finished. > In case of resolution change the application still has source buffers queued > and it receives > a destination buffer with size=0. Knowing this the application can do the > resolution change > procedure. > > I welcome your comments. > > Best regards, > -- > Kamil Debski > Linux Platform Group > Samsung Poland R&D Center -- 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
