Commented as belows. > -----Original Message----- > From: Kamil Debski [mailto:k.debski@xxxxxxxxxxx] > Sent: Monday, January 17, 2011 1:59 PM > To: jaeryul.oh@xxxxxxxxxxx; 'Jonghun Han'; linux-media@xxxxxxxxxxxxxxx; > linux-samsung-soc@xxxxxxxxxxxxxxx > Cc: 'Marek Szyprowski'; pawel@xxxxxxxxxx; kyungmin.park@xxxxxxxxxxx; > kgene.kim@xxxxxxxxxxx > Subject: RE: [RFC/PATCH v6 3/4] MFC: Add MFC 5.1 V4L2 driver > > Hi, > > I don't see the need to do DQBUF on the source buffer that has the > I-frame with changed resolution. I think that one could notify the > application by setting size=0 on the CAPTURE buffer. So the OUTPUT > buffer would not be dequeued. It would be dequeued after it has > been decoded. Do you think anything is wrong with this approach? > > I really think that copying the source buffer contents is unnecessary. > I just thought of this based on current driver sequence. Without copy for I frame(resolution changed frame), it could be possible, but we need smart driver control for this situation(res. change). So, one time queued, three times should be used : recognition for res. changed / header parsing / I frame decoding > Best wishes, > -- > Kamil Debski > Linux Platform Group > Samsung Poland R&D Center > > > -----Original Message----- > > From: Jaeryul Oh [mailto:jaeryul.oh@xxxxxxxxxxx] > > Sent: 14 January 2011 19:21 > > To: 'Jonghun Han'; 'Kamil Debski'; linux-media@xxxxxxxxxxxxxxx; linux- > > samsung-soc@xxxxxxxxxxxxxxx > > Cc: 'Marek Szyprowski'; pawel@xxxxxxxxxx; kyungmin.park@xxxxxxxxxxx; > > kgene.kim@xxxxxxxxxxx > > Subject: RE: [RFC/PATCH v6 3/4] MFC: Add MFC 5.1 V4L2 driver > > > > I added my comments. > > I guess that Kamil's way could be possible to run seq. of dynamic > > resolution > > change. > > > > > -----Original Message----- > > > From: Jonghun Han [mailto:jonghun.han@xxxxxxxxxxx] > > > Sent: Friday, January 14, 2011 6:45 PM > > > To: 'Kamil Debski'; 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, > > > > > > 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. > > > > In the blocking sequence, blocking will be done in DQBUF. > > 1. QBUF done for I1 frame with hdr(first buf, res. chd) > > : Copy I1 frame in the driver > > 2. DQBUF done for I1 frame > > 3. QBUF P2,P3,P4 frame, but it should NOT be DEQUED which means > > Qbufed p2,p3,p4 is not executed > > : P4(third buf) -> P3(second buf) -> P2 frame(first buf) > > 4. While waiting for DEQUE about P2, copied I1 frame is executed for > > INIT_CODEC > > 5. Copied I1 frame is runned FRAME_START after dst buffer is > > reallocated > > (5~9 in your seq.) > > 6. Queued P2 frame is executed, then DQBUF will be OK > > > > > > > > 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
