| Message ID | 20200814133634.95665-17-ezequiel@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Clean H264 stateless uAPI | expand |
Hi Ezequiel, On 2020-08-14 15:36, Ezequiel Garcia wrote: > Currently, the drivers makes no distinction between per_request > and mandatory, as both are used in the same request validate check. > > The driver only cares to know if a given control is > required to be part of a request, so only one flag is needed. This patch cause decoding issues with ffmpeg. The removal of per_request makes DECODE_MODE and START_CODE ctrls mandatory to be included in the request. Best regards, Jonas > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > --- > drivers/staging/media/rkvdec/rkvdec.c | 6 +----- > drivers/staging/media/rkvdec/rkvdec.h | 1 - > 2 files changed, 1 insertion(+), 6 deletions(-) > > diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c > index 7c5129593921..cd720d726d7f 100644 > --- a/drivers/staging/media/rkvdec/rkvdec.c > +++ b/drivers/staging/media/rkvdec/rkvdec.c > @@ -55,23 +55,19 @@ static const struct v4l2_ctrl_ops rkvdec_ctrl_ops = { > > static const struct rkvdec_ctrl_desc rkvdec_h264_ctrl_descs[] = { > { > - .per_request = true, > .mandatory = true, > .cfg.id = V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS, > }, > { > - .per_request = true, > .mandatory = true, > .cfg.id = V4L2_CID_MPEG_VIDEO_H264_SPS, > .cfg.ops = &rkvdec_ctrl_ops, > }, > { > - .per_request = true, > .mandatory = true, > .cfg.id = V4L2_CID_MPEG_VIDEO_H264_PPS, > }, > { > - .per_request = true, > .mandatory = true, > .cfg.id = V4L2_CID_MPEG_VIDEO_H264_SCALING_MATRIX, > }, > @@ -615,7 +611,7 @@ static int rkvdec_request_validate(struct media_request *req) > u32 id = ctrls->ctrls[i].cfg.id; > struct v4l2_ctrl *ctrl; > > - if (!ctrls->ctrls[i].per_request || !ctrls->ctrls[i].mandatory) > + if (!ctrls->ctrls[i].mandatory) > continue; > > ctrl = v4l2_ctrl_request_hdl_ctrl_find(hdl, id); > diff --git a/drivers/staging/media/rkvdec/rkvdec.h b/drivers/staging/media/rkvdec/rkvdec.h > index 2fc9f46b6910..77a137cca88e 100644 > --- a/drivers/staging/media/rkvdec/rkvdec.h > +++ b/drivers/staging/media/rkvdec/rkvdec.h > @@ -25,7 +25,6 @@ > struct rkvdec_ctx; > > struct rkvdec_ctrl_desc { > - u32 per_request : 1; > u32 mandatory : 1; > struct v4l2_ctrl_config cfg; > }; >
On Tue, 2020-08-18 at 20:17 +0000, Jonas Karlman wrote: > Hi Ezequiel, > > On 2020-08-14 15:36, Ezequiel Garcia wrote: > > Currently, the drivers makes no distinction between per_request > > and mandatory, as both are used in the same request validate check. > > > > The driver only cares to know if a given control is > > required to be part of a request, so only one flag is needed. > > This patch cause decoding issues with ffmpeg. > > The removal of per_request makes DECODE_MODE and START_CODE ctrls > mandatory to be included in the request. > Ugh, I just failed boolean logic 101. Yeah, we those controls shouldn't be mandatory. I'll send a fix for that. Other than this, can I add your tested-by to the series? Thanks, Ezequiel > Best regards, > Jonas > > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > > --- > > drivers/staging/media/rkvdec/rkvdec.c | 6 +----- > > drivers/staging/media/rkvdec/rkvdec.h | 1 - > > 2 files changed, 1 insertion(+), 6 deletions(-) > > > > diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c > > index 7c5129593921..cd720d726d7f 100644 > > --- a/drivers/staging/media/rkvdec/rkvdec.c > > +++ b/drivers/staging/media/rkvdec/rkvdec.c > > @@ -55,23 +55,19 @@ static const struct v4l2_ctrl_ops rkvdec_ctrl_ops = { > > > > static const struct rkvdec_ctrl_desc rkvdec_h264_ctrl_descs[] = { > > { > > - .per_request = true, > > .mandatory = true, > > .cfg.id = V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS, > > }, > > { > > - .per_request = true, > > .mandatory = true, > > .cfg.id = V4L2_CID_MPEG_VIDEO_H264_SPS, > > .cfg.ops = &rkvdec_ctrl_ops, > > }, > > { > > - .per_request = true, > > .mandatory = true, > > .cfg.id = V4L2_CID_MPEG_VIDEO_H264_PPS, > > }, > > { > > - .per_request = true, > > .mandatory = true, > > .cfg.id = V4L2_CID_MPEG_VIDEO_H264_SCALING_MATRIX, > > }, > > @@ -615,7 +611,7 @@ static int rkvdec_request_validate(struct media_request *req) > > u32 id = ctrls->ctrls[i].cfg.id; > > struct v4l2_ctrl *ctrl; > > > > - if (!ctrls->ctrls[i].per_request || !ctrls->ctrls[i].mandatory) > > + if (!ctrls->ctrls[i].mandatory) > > continue; > > > > ctrl = v4l2_ctrl_request_hdl_ctrl_find(hdl, id); > > diff --git a/drivers/staging/media/rkvdec/rkvdec.h b/drivers/staging/media/rkvdec/rkvdec.h > > index 2fc9f46b6910..77a137cca88e 100644 > > --- a/drivers/staging/media/rkvdec/rkvdec.h > > +++ b/drivers/staging/media/rkvdec/rkvdec.h > > @@ -25,7 +25,6 @@ > > struct rkvdec_ctx; > > > > struct rkvdec_ctrl_desc { > > - u32 per_request : 1; > > u32 mandatory : 1; > > struct v4l2_ctrl_config cfg; > > }; > >
On 2020-08-18 23:38, Ezequiel Garcia wrote: > On Tue, 2020-08-18 at 20:17 +0000, Jonas Karlman wrote: >> Hi Ezequiel, >> >> On 2020-08-14 15:36, Ezequiel Garcia wrote: >>> Currently, the drivers makes no distinction between per_request >>> and mandatory, as both are used in the same request validate check. >>> >>> The driver only cares to know if a given control is >>> required to be part of a request, so only one flag is needed. >> >> This patch cause decoding issues with ffmpeg. >> >> The removal of per_request makes DECODE_MODE and START_CODE ctrls >> mandatory to be included in the request. >> > > Ugh, I just failed boolean logic 101. > > Yeah, we those controls shouldn't be mandatory. Yep, removing mandatory flag makes rkvdec decoding work again :-) > > I'll send a fix for that. Other than this, can I add your tested-by to the series? Yes, with above fix this series is Tested-by: Jonas Karlman <jonas@kwiboo.se> using ffmpeg [1] on rk3288 (hantro) and rk3399 (rkvdec). I have also done limited testing of field decoding on H.264 conformance video samples and rkvdec manage to generate matching checksums. On hantro the output is slightly different for fld and picaff samples and match for frm and mbaff samples. Because field decoding works correctly with rkvdec I am confident that uapi contains everything needed to support field decoding. [1] https://github.com/Kwiboo/FFmpeg/commits/v4l2-request-hwaccel-4.3.1 Best regards, Jonas > > Thanks, > Ezequiel > >> Best regards, >> Jonas >> >>> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> >>> --- >>> drivers/staging/media/rkvdec/rkvdec.c | 6 +----- >>> drivers/staging/media/rkvdec/rkvdec.h | 1 - >>> 2 files changed, 1 insertion(+), 6 deletions(-) >>> >>> diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c >>> index 7c5129593921..cd720d726d7f 100644 >>> --- a/drivers/staging/media/rkvdec/rkvdec.c >>> +++ b/drivers/staging/media/rkvdec/rkvdec.c >>> @@ -55,23 +55,19 @@ static const struct v4l2_ctrl_ops rkvdec_ctrl_ops = { >>> >>> static const struct rkvdec_ctrl_desc rkvdec_h264_ctrl_descs[] = { >>> { >>> - .per_request = true, >>> .mandatory = true, >>> .cfg.id = V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS, >>> }, >>> { >>> - .per_request = true, >>> .mandatory = true, >>> .cfg.id = V4L2_CID_MPEG_VIDEO_H264_SPS, >>> .cfg.ops = &rkvdec_ctrl_ops, >>> }, >>> { >>> - .per_request = true, >>> .mandatory = true, >>> .cfg.id = V4L2_CID_MPEG_VIDEO_H264_PPS, >>> }, >>> { >>> - .per_request = true, >>> .mandatory = true, >>> .cfg.id = V4L2_CID_MPEG_VIDEO_H264_SCALING_MATRIX, >>> }, >>> @@ -615,7 +611,7 @@ static int rkvdec_request_validate(struct media_request *req) >>> u32 id = ctrls->ctrls[i].cfg.id; >>> struct v4l2_ctrl *ctrl; >>> >>> - if (!ctrls->ctrls[i].per_request || !ctrls->ctrls[i].mandatory) >>> + if (!ctrls->ctrls[i].mandatory) >>> continue; >>> >>> ctrl = v4l2_ctrl_request_hdl_ctrl_find(hdl, id); >>> diff --git a/drivers/staging/media/rkvdec/rkvdec.h b/drivers/staging/media/rkvdec/rkvdec.h >>> index 2fc9f46b6910..77a137cca88e 100644 >>> --- a/drivers/staging/media/rkvdec/rkvdec.h >>> +++ b/drivers/staging/media/rkvdec/rkvdec.h >>> @@ -25,7 +25,6 @@ >>> struct rkvdec_ctx; >>> >>> struct rkvdec_ctrl_desc { >>> - u32 per_request : 1; >>> u32 mandatory : 1; >>> struct v4l2_ctrl_config cfg; >>> }; >>> > >
diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c index 7c5129593921..cd720d726d7f 100644 --- a/drivers/staging/media/rkvdec/rkvdec.c +++ b/drivers/staging/media/rkvdec/rkvdec.c @@ -55,23 +55,19 @@ static const struct v4l2_ctrl_ops rkvdec_ctrl_ops = { static const struct rkvdec_ctrl_desc rkvdec_h264_ctrl_descs[] = { { - .per_request = true, .mandatory = true, .cfg.id = V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS, }, { - .per_request = true, .mandatory = true, .cfg.id = V4L2_CID_MPEG_VIDEO_H264_SPS, .cfg.ops = &rkvdec_ctrl_ops, }, { - .per_request = true, .mandatory = true, .cfg.id = V4L2_CID_MPEG_VIDEO_H264_PPS, }, { - .per_request = true, .mandatory = true, .cfg.id = V4L2_CID_MPEG_VIDEO_H264_SCALING_MATRIX, }, @@ -615,7 +611,7 @@ static int rkvdec_request_validate(struct media_request *req) u32 id = ctrls->ctrls[i].cfg.id; struct v4l2_ctrl *ctrl; - if (!ctrls->ctrls[i].per_request || !ctrls->ctrls[i].mandatory) + if (!ctrls->ctrls[i].mandatory) continue; ctrl = v4l2_ctrl_request_hdl_ctrl_find(hdl, id); diff --git a/drivers/staging/media/rkvdec/rkvdec.h b/drivers/staging/media/rkvdec/rkvdec.h index 2fc9f46b6910..77a137cca88e 100644 --- a/drivers/staging/media/rkvdec/rkvdec.h +++ b/drivers/staging/media/rkvdec/rkvdec.h @@ -25,7 +25,6 @@ struct rkvdec_ctx; struct rkvdec_ctrl_desc { - u32 per_request : 1; u32 mandatory : 1; struct v4l2_ctrl_config cfg; };
Currently, the drivers makes no distinction between per_request and mandatory, as both are used in the same request validate check. The driver only cares to know if a given control is required to be part of a request, so only one flag is needed. Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> --- drivers/staging/media/rkvdec/rkvdec.c | 6 +----- drivers/staging/media/rkvdec/rkvdec.h | 1 - 2 files changed, 1 insertion(+), 6 deletions(-)