| Message ID | 20220627112402.2332046-2-wenst@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: mediatek: vcodec: Fix 4K decoding support | expand |
Hi Chen-Yu, Le lundi 27 juin 2022 à 19:23 +0800, Chen-Yu Tsai a écrit : > This partially reverts commit b018be06f3c7 ("media: mediatek: vcodec: > Read max resolution from dec_capability"). In this commit, the maximum > resolution ended up being a function of both the firmware capability and > the current set format. > > However, frame size enumeration for output (coded) formats should not > depend on the format set, but should return supported resolutions for > the format requested by userspace. Good point. Though, I don't see any special casing for the CAPTURE case. As this HW does not include a scaler, it must only return 1 resolution when being enumerated for CAPTURE side (or not implement that enumeration, but its complicated to half implement something in m2m). The return unique size should match what G_FMT(CAPTURE) would return. > > Fix this so that the driver returns the supported resolutions correctly, > even if the instance only has default settings, or if the output format > is currently set to VP8F, which does not support 4K. > > Fixes: b018be06f3c7 ("media: mediatek: vcodec: Read max resolution from dec_capability") > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org> > --- > drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c | 2 -- > .../platform/mediatek/vcodec/mtk_vcodec_dec_stateless.c | 7 +++++++ > 2 files changed, 7 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c > index 5d6fdf18c3a6..fcb4b8131c49 100644 > --- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c > +++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c > @@ -595,8 +595,6 @@ static int vidioc_enum_framesizes(struct file *file, void *priv, > fsize->type = V4L2_FRMSIZE_TYPE_STEPWISE; > fsize->stepwise = dec_pdata->vdec_framesizes[i].stepwise; > > - fsize->stepwise.max_width = ctx->max_width; > - fsize->stepwise.max_height = ctx->max_height; > mtk_v4l2_debug(1, "%x, %d %d %d %d %d %d", > ctx->dev->dec_capability, > fsize->stepwise.min_width, > diff --git a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_stateless.c b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_stateless.c > index 16d55785d84b..9a4d3e3658aa 100644 > --- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_stateless.c > +++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_stateless.c > @@ -360,6 +360,13 @@ static void mtk_vcodec_add_formats(unsigned int fourcc, > > mtk_vdec_framesizes[count_framesizes].fourcc = fourcc; > mtk_vdec_framesizes[count_framesizes].stepwise = stepwise_fhd; > + if (!(ctx->dev->dec_capability & VCODEC_CAPABILITY_4K_DISABLED) && > + fourcc != V4L2_PIX_FMT_VP8_FRAME) { > + mtk_vdec_framesizes[count_framesizes].stepwise.max_width = > + VCODEC_DEC_4K_CODED_WIDTH; > + mtk_vdec_framesizes[count_framesizes].stepwise.max_height = > + VCODEC_DEC_4K_CODED_HEIGHT; > + } I don't particularly like to see this special cased check being added into multiple places. Its also in your patch 2, and I think it exist in a third place. Could it be possible to have an internal helper to ensure we don't duplicate this logic ? Somehow, it seems there is something in common between set_default, try_fmt and this code. > num_framesizes++; > break; > case V4L2_PIX_FMT_MM21:
Le lundi 27 juin 2022 à 19:23 +0800, Chen-Yu Tsai a écrit : > This partially reverts commit b018be06f3c7 ("media: mediatek: vcodec: > Read max resolution from dec_capability"). In this commit, the maximum > resolution ended up being a function of both the firmware capability and > the current set format. > > However, frame size enumeration for output (coded) formats should not > depend on the format set, but should return supported resolutions for > the format requested by userspace. > > Fix this so that the driver returns the supported resolutions correctly, > even if the instance only has default settings, or if the output format > is currently set to VP8F, which does not support 4K. > > Fixes: b018be06f3c7 ("media: mediatek: vcodec: Read max resolution from dec_capability") > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org> > --- > drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c | 2 -- > .../platform/mediatek/vcodec/mtk_vcodec_dec_stateless.c | 7 +++++++ > 2 files changed, 7 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c > index 5d6fdf18c3a6..fcb4b8131c49 100644 > --- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c > +++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c > @@ -595,8 +595,6 @@ static int vidioc_enum_framesizes(struct file *file, void *priv, > fsize->type = V4L2_FRMSIZE_TYPE_STEPWISE; > fsize->stepwise = dec_pdata->vdec_framesizes[i].stepwise; > > - fsize->stepwise.max_width = ctx->max_width; > - fsize->stepwise.max_height = ctx->max_height; > mtk_v4l2_debug(1, "%x, %d %d %d %d %d %d", > ctx->dev->dec_capability, > fsize->stepwise.min_width, > diff --git a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_stateless.c b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_stateless.c > index 16d55785d84b..9a4d3e3658aa 100644 > --- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_stateless.c > +++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_stateless.c > @@ -360,6 +360,13 @@ static void mtk_vcodec_add_formats(unsigned int fourcc, > > mtk_vdec_framesizes[count_framesizes].fourcc = fourcc; > mtk_vdec_framesizes[count_framesizes].stepwise = stepwise_fhd; While at it, can you constify stepwise_fhd, it is scary to think that someone could modify it and cause big headache. > + if (!(ctx->dev->dec_capability & VCODEC_CAPABILITY_4K_DISABLED) && > + fourcc != V4L2_PIX_FMT_VP8_FRAME) { > + mtk_vdec_framesizes[count_framesizes].stepwise.max_width = > + VCODEC_DEC_4K_CODED_WIDTH; > + mtk_vdec_framesizes[count_framesizes].stepwise.max_height = > + VCODEC_DEC_4K_CODED_HEIGHT; > + } > num_framesizes++; > break; > case V4L2_PIX_FMT_MM21:
On Mon, Jun 27, 2022 at 11:32 PM Nicolas Dufresne <nicolas.dufresne@collabora.com> wrote: > > Hi Chen-Yu, > > Le lundi 27 juin 2022 à 19:23 +0800, Chen-Yu Tsai a écrit : > > This partially reverts commit b018be06f3c7 ("media: mediatek: vcodec: > > Read max resolution from dec_capability"). In this commit, the maximum > > resolution ended up being a function of both the firmware capability and > > the current set format. > > > > However, frame size enumeration for output (coded) formats should not > > depend on the format set, but should return supported resolutions for > > the format requested by userspace. > > Good point. Though, I don't see any special casing for the CAPTURE case. As this > HW does not include a scaler, it must only return 1 resolution when being > enumerated for CAPTURE side (or not implement that enumeration, but its > complicated to half implement something in m2m). The return unique size should > match what G_FMT(CAPTURE) would return. There are no frame sizes added for the capture formats, so this function effectively returns -EINVAL for any of them. This is also what rkvdec does: it only looks through the list of coded formats. Also, struct v4l2_frmsizeenum does not have a field saying whether it's capture or output side; it simply specifies a pixel format. > > > > > Fix this so that the driver returns the supported resolutions correctly, > > even if the instance only has default settings, or if the output format > > is currently set to VP8F, which does not support 4K. > > > > Fixes: b018be06f3c7 ("media: mediatek: vcodec: Read max resolution from dec_capability") > > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org> > > --- > > drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c | 2 -- > > .../platform/mediatek/vcodec/mtk_vcodec_dec_stateless.c | 7 +++++++ > > 2 files changed, 7 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c > > index 5d6fdf18c3a6..fcb4b8131c49 100644 > > --- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c > > +++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c > > @@ -595,8 +595,6 @@ static int vidioc_enum_framesizes(struct file *file, void *priv, > > fsize->type = V4L2_FRMSIZE_TYPE_STEPWISE; > > fsize->stepwise = dec_pdata->vdec_framesizes[i].stepwise; > > > > - fsize->stepwise.max_width = ctx->max_width; > > - fsize->stepwise.max_height = ctx->max_height; > > mtk_v4l2_debug(1, "%x, %d %d %d %d %d %d", > > ctx->dev->dec_capability, > > fsize->stepwise.min_width, > > diff --git a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_stateless.c b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_stateless.c > > index 16d55785d84b..9a4d3e3658aa 100644 > > --- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_stateless.c > > +++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_stateless.c > > @@ -360,6 +360,13 @@ static void mtk_vcodec_add_formats(unsigned int fourcc, > > > > mtk_vdec_framesizes[count_framesizes].fourcc = fourcc; > > mtk_vdec_framesizes[count_framesizes].stepwise = stepwise_fhd; > > + if (!(ctx->dev->dec_capability & VCODEC_CAPABILITY_4K_DISABLED) && > > + fourcc != V4L2_PIX_FMT_VP8_FRAME) { > > + mtk_vdec_framesizes[count_framesizes].stepwise.max_width = > > + VCODEC_DEC_4K_CODED_WIDTH; > > + mtk_vdec_framesizes[count_framesizes].stepwise.max_height = > > + VCODEC_DEC_4K_CODED_HEIGHT; > > + } > > I don't particularly like to see this special cased check being added into > multiple places. Its also in your patch 2, and I think it exist in a third > place. Could it be possible to have an internal helper to ensure we don't It's also in s_fmt(), so touched on in patch 4. I could also rewrite it so only this spot has the special case, and all the other places look though mtk_vdec_framesizes to get the maximum, like what I did for try_fmt in patch 3. What do you think? Ultimately I think it would be better to move framesizes into the (driver-specific) pixel format data structure. That is a bigger refactoring than a simple fix though. > duplicate this logic ? Somehow, it seems there is something in common between > set_default, try_fmt and this code. Yes. That is what I mentioned in chat about refactoring the ioctls and format handling code. set_default should really not set anything format specific, but instead call set_fmt with a default format. Regards ChenYu > > > num_framesizes++; > > break; > > case V4L2_PIX_FMT_MM21: >
On Mon, Jun 27, 2022 at 11:33 PM Nicolas Dufresne <nicolas.dufresne@collabora.com> wrote: > > Le lundi 27 juin 2022 à 19:23 +0800, Chen-Yu Tsai a écrit : > > This partially reverts commit b018be06f3c7 ("media: mediatek: vcodec: > > Read max resolution from dec_capability"). In this commit, the maximum > > resolution ended up being a function of both the firmware capability and > > the current set format. > > > > However, frame size enumeration for output (coded) formats should not > > depend on the format set, but should return supported resolutions for > > the format requested by userspace. > > > > Fix this so that the driver returns the supported resolutions correctly, > > even if the instance only has default settings, or if the output format > > is currently set to VP8F, which does not support 4K. > > > > Fixes: b018be06f3c7 ("media: mediatek: vcodec: Read max resolution from dec_capability") > > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org> > > --- > > drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c | 2 -- > > .../platform/mediatek/vcodec/mtk_vcodec_dec_stateless.c | 7 +++++++ > > 2 files changed, 7 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c > > index 5d6fdf18c3a6..fcb4b8131c49 100644 > > --- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c > > +++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c > > @@ -595,8 +595,6 @@ static int vidioc_enum_framesizes(struct file *file, void *priv, > > fsize->type = V4L2_FRMSIZE_TYPE_STEPWISE; > > fsize->stepwise = dec_pdata->vdec_framesizes[i].stepwise; > > > > - fsize->stepwise.max_width = ctx->max_width; > > - fsize->stepwise.max_height = ctx->max_height; > > mtk_v4l2_debug(1, "%x, %d %d %d %d %d %d", > > ctx->dev->dec_capability, > > fsize->stepwise.min_width, > > diff --git a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_stateless.c b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_stateless.c > > index 16d55785d84b..9a4d3e3658aa 100644 > > --- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_stateless.c > > +++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_stateless.c > > @@ -360,6 +360,13 @@ static void mtk_vcodec_add_formats(unsigned int fourcc, > > > > mtk_vdec_framesizes[count_framesizes].fourcc = fourcc; > > mtk_vdec_framesizes[count_framesizes].stepwise = stepwise_fhd; > > While at it, can you constify stepwise_fhd, it is scary to think that someone > could modify it and cause big headache. I noticed that as well, but since a planned refactoring would get rid of it, I didn't fix it in this series. I can add a patch for that. ChenYu > > + if (!(ctx->dev->dec_capability & VCODEC_CAPABILITY_4K_DISABLED) && > > + fourcc != V4L2_PIX_FMT_VP8_FRAME) { > > + mtk_vdec_framesizes[count_framesizes].stepwise.max_width = > > + VCODEC_DEC_4K_CODED_WIDTH; > > + mtk_vdec_framesizes[count_framesizes].stepwise.max_height = > > + VCODEC_DEC_4K_CODED_HEIGHT; > > + } > > num_framesizes++; > > break; > > case V4L2_PIX_FMT_MM21: >
Le mardi 28 juin 2022 à 00:08 +0800, Chen-Yu Tsai a écrit : > On Mon, Jun 27, 2022 at 11:32 PM Nicolas Dufresne > <nicolas.dufresne@collabora.com> wrote: > > > > Hi Chen-Yu, > > > > Le lundi 27 juin 2022 à 19:23 +0800, Chen-Yu Tsai a écrit : > > > This partially reverts commit b018be06f3c7 ("media: mediatek: vcodec: > > > Read max resolution from dec_capability"). In this commit, the maximum > > > resolution ended up being a function of both the firmware capability and > > > the current set format. > > > > > > However, frame size enumeration for output (coded) formats should not > > > depend on the format set, but should return supported resolutions for > > > the format requested by userspace. > > > > Good point. Though, I don't see any special casing for the CAPTURE case. As this > > HW does not include a scaler, it must only return 1 resolution when being > > enumerated for CAPTURE side (or not implement that enumeration, but its > > complicated to half implement something in m2m). The return unique size should > > match what G_FMT(CAPTURE) would return. > > There are no frame sizes added for the capture formats, so this function > effectively returns -EINVAL for any of them. This is also what rkvdec > does: it only looks through the list of coded formats. This is effectively against the spec, ENOTTY would be the only alternative to not implementing both sides. Though, I'll agree with you, this bugs predates anything here. Perhaps you could at add MM21 to the switch and returns ENOTTY there ? > > Also, struct v4l2_frmsizeenum does not have a field saying whether it's > capture or output side; it simply specifies a pixel format. Acked. > > > > > > > > > Fix this so that the driver returns the supported resolutions correctly, > > > even if the instance only has default settings, or if the output format > > > is currently set to VP8F, which does not support 4K. > > > > > > Fixes: b018be06f3c7 ("media: mediatek: vcodec: Read max resolution from dec_capability") > > > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org> > > > --- > > > drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c | 2 -- > > > .../platform/mediatek/vcodec/mtk_vcodec_dec_stateless.c | 7 +++++++ > > > 2 files changed, 7 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c > > > index 5d6fdf18c3a6..fcb4b8131c49 100644 > > > --- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c > > > +++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c > > > @@ -595,8 +595,6 @@ static int vidioc_enum_framesizes(struct file *file, void *priv, > > > fsize->type = V4L2_FRMSIZE_TYPE_STEPWISE; > > > fsize->stepwise = dec_pdata->vdec_framesizes[i].stepwise; > > > > > > - fsize->stepwise.max_width = ctx->max_width; > > > - fsize->stepwise.max_height = ctx->max_height; > > > mtk_v4l2_debug(1, "%x, %d %d %d %d %d %d", > > > ctx->dev->dec_capability, > > > fsize->stepwise.min_width, > > > diff --git a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_stateless.c b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_stateless.c > > > index 16d55785d84b..9a4d3e3658aa 100644 > > > --- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_stateless.c > > > +++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_stateless.c > > > @@ -360,6 +360,13 @@ static void mtk_vcodec_add_formats(unsigned int fourcc, > > > > > > mtk_vdec_framesizes[count_framesizes].fourcc = fourcc; > > > mtk_vdec_framesizes[count_framesizes].stepwise = stepwise_fhd; > > > + if (!(ctx->dev->dec_capability & VCODEC_CAPABILITY_4K_DISABLED) && > > > + fourcc != V4L2_PIX_FMT_VP8_FRAME) { > > > + mtk_vdec_framesizes[count_framesizes].stepwise.max_width = > > > + VCODEC_DEC_4K_CODED_WIDTH; > > > + mtk_vdec_framesizes[count_framesizes].stepwise.max_height = > > > + VCODEC_DEC_4K_CODED_HEIGHT; > > > + } > > > > I don't particularly like to see this special cased check being added into > > multiple places. Its also in your patch 2, and I think it exist in a third > > place. Could it be possible to have an internal helper to ensure we don't > > It's also in s_fmt(), so touched on in patch 4. I could also rewrite it so > only this spot has the special case, and all the other places look though > mtk_vdec_framesizes to get the maximum, like what I did for try_fmt in > patch 3. What do you think? I don't have a strong opinion, could be a totally internal (and unrelated to any ioctl naming) helper that does the right thing. > > Ultimately I think it would be better to move framesizes into the > (driver-specific) pixel format data structure. That is a bigger refactoring > than a simple fix though. Agreed. > > > duplicate this logic ? Somehow, it seems there is something in common between > > set_default, try_fmt and this code. > > Yes. That is what I mentioned in chat about refactoring the ioctls and format > handling code. set_default should really not set anything format specific, > but instead call set_fmt with a default format. So if this could have a simple helper that returns the max width/height for the specified format and HW capability, I'm then fine with the series. If you can change the EINVAL (which means nothing is supported) into ENOTTY for the MM21 case, I'd also be more confortable (even though still a bit odd, but no longer a lie). regards, Nicolas > > > Regards > ChenYu > > > > > > num_framesizes++; > > > break; > > > case V4L2_PIX_FMT_MM21: > >
On Tue, Jun 28, 2022 at 3:28 AM Nicolas Dufresne <nicolas.dufresne@collabora.com> wrote: > > Le mardi 28 juin 2022 à 00:08 +0800, Chen-Yu Tsai a écrit : > > On Mon, Jun 27, 2022 at 11:32 PM Nicolas Dufresne > > <nicolas.dufresne@collabora.com> wrote: > > > > > > Hi Chen-Yu, > > > > > > Le lundi 27 juin 2022 à 19:23 +0800, Chen-Yu Tsai a écrit : > > > > This partially reverts commit b018be06f3c7 ("media: mediatek: vcodec: > > > > Read max resolution from dec_capability"). In this commit, the maximum > > > > resolution ended up being a function of both the firmware capability and > > > > the current set format. > > > > > > > > However, frame size enumeration for output (coded) formats should not > > > > depend on the format set, but should return supported resolutions for > > > > the format requested by userspace. > > > > > > Good point. Though, I don't see any special casing for the CAPTURE case. As this > > > HW does not include a scaler, it must only return 1 resolution when being > > > enumerated for CAPTURE side (or not implement that enumeration, but its > > > complicated to half implement something in m2m). The return unique size should > > > match what G_FMT(CAPTURE) would return. > > > > There are no frame sizes added for the capture formats, so this function > > effectively returns -EINVAL for any of them. This is also what rkvdec > > does: it only looks through the list of coded formats. > > This is effectively against the spec, ENOTTY would be the only alternative to > not implementing both sides. Though, I'll agree with you, this bugs predates > anything here. Perhaps you could at add MM21 to the switch and returns ENOTTY > there ? I think you are slightly misreading the code? The switch/case is in the function that adds supported formats, not the ioctl callback. But yeah, I can have the enum_framesizes callback match against capture formats and return -ENOTTY for them. For unmatched formats, either capture or output, is it still correct to return -EINVAL? > > > > Also, struct v4l2_frmsizeenum does not have a field saying whether it's > > capture or output side; it simply specifies a pixel format. > > Acked. > > > > > > > > > > > > > > Fix this so that the driver returns the supported resolutions correctly, > > > > even if the instance only has default settings, or if the output format > > > > is currently set to VP8F, which does not support 4K. > > > > > > > > Fixes: b018be06f3c7 ("media: mediatek: vcodec: Read max resolution from dec_capability") > > > > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org> > > > > --- > > > > drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c | 2 -- > > > > .../platform/mediatek/vcodec/mtk_vcodec_dec_stateless.c | 7 +++++++ > > > > 2 files changed, 7 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c > > > > index 5d6fdf18c3a6..fcb4b8131c49 100644 > > > > --- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c > > > > +++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c > > > > @@ -595,8 +595,6 @@ static int vidioc_enum_framesizes(struct file *file, void *priv, > > > > fsize->type = V4L2_FRMSIZE_TYPE_STEPWISE; > > > > fsize->stepwise = dec_pdata->vdec_framesizes[i].stepwise; > > > > > > > > - fsize->stepwise.max_width = ctx->max_width; > > > > - fsize->stepwise.max_height = ctx->max_height; > > > > mtk_v4l2_debug(1, "%x, %d %d %d %d %d %d", > > > > ctx->dev->dec_capability, > > > > fsize->stepwise.min_width, > > > > diff --git a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_stateless.c b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_stateless.c > > > > index 16d55785d84b..9a4d3e3658aa 100644 > > > > --- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_stateless.c > > > > +++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_stateless.c > > > > @@ -360,6 +360,13 @@ static void mtk_vcodec_add_formats(unsigned int fourcc, > > > > > > > > mtk_vdec_framesizes[count_framesizes].fourcc = fourcc; > > > > mtk_vdec_framesizes[count_framesizes].stepwise = stepwise_fhd; > > > > + if (!(ctx->dev->dec_capability & VCODEC_CAPABILITY_4K_DISABLED) && > > > > + fourcc != V4L2_PIX_FMT_VP8_FRAME) { > > > > + mtk_vdec_framesizes[count_framesizes].stepwise.max_width = > > > > + VCODEC_DEC_4K_CODED_WIDTH; > > > > + mtk_vdec_framesizes[count_framesizes].stepwise.max_height = > > > > + VCODEC_DEC_4K_CODED_HEIGHT; > > > > + } > > > > > > I don't particularly like to see this special cased check being added into > > > multiple places. Its also in your patch 2, and I think it exist in a third > > > place. Could it be possible to have an internal helper to ensure we don't > > > > It's also in s_fmt(), so touched on in patch 4. I could also rewrite it so > > only this spot has the special case, and all the other places look though > > mtk_vdec_framesizes to get the maximum, like what I did for try_fmt in > > patch 3. What do you think? > > I don't have a strong opinion, could be a totally internal (and unrelated to any > ioctl naming) helper that does the right thing. According to offline discussions in chat, and looking at the code again, it seems we can get rid of ctx->max_{width,height} altogether. The check will only exist in mtk_vcodec_add_formats(), and the resolution clamping will only happen in the try_fmt callback. ChenYu > > > > Ultimately I think it would be better to move framesizes into the > > (driver-specific) pixel format data structure. That is a bigger refactoring > > than a simple fix though. > > Agreed. > > > > > > duplicate this logic ? Somehow, it seems there is something in common between > > > set_default, try_fmt and this code. > > > > Yes. That is what I mentioned in chat about refactoring the ioctls and format > > handling code. set_default should really not set anything format specific, > > but instead call set_fmt with a default format. > > So if this could have a simple helper that returns the max width/height for the > specified format and HW capability, I'm then fine with the series. If you can > change the EINVAL (which means nothing is supported) into ENOTTY for the MM21 > case, I'd also be more confortable (even though still a bit odd, but no longer a > lie). > > regards, > Nicolas > > > > > > > Regards > > ChenYu > > > > > > > > > num_framesizes++; > > > > break; > > > > case V4L2_PIX_FMT_MM21: > > > >
diff --git a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c index 5d6fdf18c3a6..fcb4b8131c49 100644 --- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c +++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c @@ -595,8 +595,6 @@ static int vidioc_enum_framesizes(struct file *file, void *priv, fsize->type = V4L2_FRMSIZE_TYPE_STEPWISE; fsize->stepwise = dec_pdata->vdec_framesizes[i].stepwise; - fsize->stepwise.max_width = ctx->max_width; - fsize->stepwise.max_height = ctx->max_height; mtk_v4l2_debug(1, "%x, %d %d %d %d %d %d", ctx->dev->dec_capability, fsize->stepwise.min_width, diff --git a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_stateless.c b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_stateless.c index 16d55785d84b..9a4d3e3658aa 100644 --- a/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_stateless.c +++ b/drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec_stateless.c @@ -360,6 +360,13 @@ static void mtk_vcodec_add_formats(unsigned int fourcc, mtk_vdec_framesizes[count_framesizes].fourcc = fourcc; mtk_vdec_framesizes[count_framesizes].stepwise = stepwise_fhd; + if (!(ctx->dev->dec_capability & VCODEC_CAPABILITY_4K_DISABLED) && + fourcc != V4L2_PIX_FMT_VP8_FRAME) { + mtk_vdec_framesizes[count_framesizes].stepwise.max_width = + VCODEC_DEC_4K_CODED_WIDTH; + mtk_vdec_framesizes[count_framesizes].stepwise.max_height = + VCODEC_DEC_4K_CODED_HEIGHT; + } num_framesizes++; break; case V4L2_PIX_FMT_MM21:
This partially reverts commit b018be06f3c7 ("media: mediatek: vcodec: Read max resolution from dec_capability"). In this commit, the maximum resolution ended up being a function of both the firmware capability and the current set format. However, frame size enumeration for output (coded) formats should not depend on the format set, but should return supported resolutions for the format requested by userspace. Fix this so that the driver returns the supported resolutions correctly, even if the instance only has default settings, or if the output format is currently set to VP8F, which does not support 4K. Fixes: b018be06f3c7 ("media: mediatek: vcodec: Read max resolution from dec_capability") Signed-off-by: Chen-Yu Tsai <wenst@chromium.org> --- drivers/media/platform/mediatek/vcodec/mtk_vcodec_dec.c | 2 -- .../platform/mediatek/vcodec/mtk_vcodec_dec_stateless.c | 7 +++++++ 2 files changed, 7 insertions(+), 2 deletions(-)