| Message ID | 20160708104327.6329-1-niklas.soderlund+renesas@ragnatech.se (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 07/08/2016 12:43 PM, Niklas Söderlund wrote: > A recent bugfix to adv7180 brought to light that the rcar-vin driver are > looking for the wrong media bus format. It was looking for a YUVU format > but then expecting UYVY data. The bugfix for adv7180 will break the > usage of rcar-vin together with a adv7180 as found on Renesas R-Car2 > Koelsch boards for example. > > This patch fix the rcar-vin driver to look for the correct UYVU formats > and adds a legacy mode. The legacy mode is needed since I don't know if > other devices provide a incorrect media bus format and I don't want to > break any working configurations. Hopefully the legacy mode can be > removed sometime in the future. I'd rather have a version without the legacy code. You have to assume that subdevs return correct values otherwise what's the point of the mediabus formats? So this is simply an adv7180 bug fix + this r-car fix to stay consistent with the adv7180. Regards, Hans > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > --- > drivers/media/platform/rcar-vin/rcar-core.c | 39 +++++++++++++++++++++++++++-- > drivers/media/platform/rcar-vin/rcar-dma.c | 4 +-- > 2 files changed, 39 insertions(+), 4 deletions(-) > > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c > index 4b2007b..481d82a 100644 > --- a/drivers/media/platform/rcar-vin/rcar-core.c > +++ b/drivers/media/platform/rcar-vin/rcar-core.c > @@ -37,6 +37,7 @@ static int rvin_mbus_supported(struct rvin_dev *vin) > struct v4l2_subdev_mbus_code_enum code = { > .which = V4L2_SUBDEV_FORMAT_ACTIVE, > }; > + bool found; > > sd = vin_to_source(vin); > > @@ -45,8 +46,8 @@ static int rvin_mbus_supported(struct rvin_dev *vin) > code.index++; > switch (code.code) { > case MEDIA_BUS_FMT_YUYV8_1X16: > - case MEDIA_BUS_FMT_YUYV8_2X8: > - case MEDIA_BUS_FMT_YUYV10_2X10: > + case MEDIA_BUS_FMT_UYVY8_2X8: > + case MEDIA_BUS_FMT_UYVY10_2X10: > case MEDIA_BUS_FMT_RGB888_1X24: > vin->source.code = code.code; > vin_dbg(vin, "Found supported media bus format: %d\n", > @@ -57,6 +58,40 @@ static int rvin_mbus_supported(struct rvin_dev *vin) > } > } > > + /* > + * Older versions where looking for the wrong media bus format. > + * It where looking for a YUVY format but then treated it as a > + * UYVY format. This was not noticed since atlest one subdevice > + * used for testing (adv7180) reported a YUVY media bus format > + * but provided UYVY data. There might be other unknown subdevices > + * which also do this, to not break compatibility try to use them > + * in legacy mode. > + */ > + found = false; > + code.index = 0; > + while (!v4l2_subdev_call(sd, pad, enum_mbus_code, NULL, &code)) { > + code.index++; > + switch (code.code) { > + case MEDIA_BUS_FMT_YUYV8_2X8: > + vin->source.code = MEDIA_BUS_FMT_UYVY8_2X8; > + found = true; > + break; > + case MEDIA_BUS_FMT_YUYV10_2X10: > + vin->source.code = MEDIA_BUS_FMT_UYVY10_2X10; > + found = true; > + break; > + default: > + break; > + } > + > + if (found) { > + vin_err(vin, > + "media bus %d not supported, trying legacy mode %d\n", > + code.code, vin->source.code); > + return true; > + } > + } > + > return false; > } > > diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c > index dad3b03..0836b15 100644 > --- a/drivers/media/platform/rcar-vin/rcar-dma.c > +++ b/drivers/media/platform/rcar-vin/rcar-dma.c > @@ -169,7 +169,7 @@ static int rvin_setup(struct rvin_dev *vin) > vnmc |= VNMC_INF_YUV16; > input_is_yuv = true; > break; > - case MEDIA_BUS_FMT_YUYV8_2X8: > + case MEDIA_BUS_FMT_UYVY8_2X8: > /* BT.656 8bit YCbCr422 or BT.601 8bit YCbCr422 */ > vnmc |= vin->mbus_cfg.type == V4L2_MBUS_BT656 ? > VNMC_INF_YUV8_BT656 : VNMC_INF_YUV8_BT601; > @@ -178,7 +178,7 @@ static int rvin_setup(struct rvin_dev *vin) > case MEDIA_BUS_FMT_RGB888_1X24: > vnmc |= VNMC_INF_RGB888; > break; > - case MEDIA_BUS_FMT_YUYV10_2X10: > + case MEDIA_BUS_FMT_UYVY10_2X10: > /* BT.656 10bit YCbCr422 or BT.601 10bit YCbCr422 */ > vnmc |= vin->mbus_cfg.type == V4L2_MBUS_BT656 ? > VNMC_INF_YUV10_BT656 : VNMC_INF_YUV10_BT601; > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Hans, Thanks for your feedback. On 2016-07-20 11:48:40 +0200, Hans Verkuil wrote: > On 07/08/2016 12:43 PM, Niklas Söderlund wrote: > > A recent bugfix to adv7180 brought to light that the rcar-vin driver are > > looking for the wrong media bus format. It was looking for a YUVU format > > but then expecting UYVY data. The bugfix for adv7180 will break the > > usage of rcar-vin together with a adv7180 as found on Renesas R-Car2 > > Koelsch boards for example. > > > > This patch fix the rcar-vin driver to look for the correct UYVU formats > > and adds a legacy mode. The legacy mode is needed since I don't know if > > other devices provide a incorrect media bus format and I don't want to > > break any working configurations. Hopefully the legacy mode can be > > removed sometime in the future. > > I'd rather have a version without the legacy code. You have to assume that > subdevs return correct values otherwise what's the point of the mediabus > formats? > > So this is simply an adv7180 bug fix + this r-car fix to stay consistent > with the adv7180. On principal I agree with you. My goal with this patch is just to make sure there is no case where the rcar-vin driver won't work with the adv7180. The plan was to drop the legacy mode in a separate patch after both the adv7182 and rcar-vin patches where picked up. I'm happy to drop the 'legacy support' for the wrong formats from this patch as long as I can be sure that there is no breaking. Should I rewrite this patch to drop the wrong formats and submit it as a series together with the adv7180 patch so they can be picked up together? Or do you know of a better way? > > Regards, > > Hans > > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > --- > > drivers/media/platform/rcar-vin/rcar-core.c | 39 +++++++++++++++++++++++++++-- > > drivers/media/platform/rcar-vin/rcar-dma.c | 4 +-- > > 2 files changed, 39 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c > > index 4b2007b..481d82a 100644 > > --- a/drivers/media/platform/rcar-vin/rcar-core.c > > +++ b/drivers/media/platform/rcar-vin/rcar-core.c > > @@ -37,6 +37,7 @@ static int rvin_mbus_supported(struct rvin_dev *vin) > > struct v4l2_subdev_mbus_code_enum code = { > > .which = V4L2_SUBDEV_FORMAT_ACTIVE, > > }; > > + bool found; > > > > sd = vin_to_source(vin); > > > > @@ -45,8 +46,8 @@ static int rvin_mbus_supported(struct rvin_dev *vin) > > code.index++; > > switch (code.code) { > > case MEDIA_BUS_FMT_YUYV8_1X16: > > - case MEDIA_BUS_FMT_YUYV8_2X8: > > - case MEDIA_BUS_FMT_YUYV10_2X10: > > + case MEDIA_BUS_FMT_UYVY8_2X8: > > + case MEDIA_BUS_FMT_UYVY10_2X10: > > case MEDIA_BUS_FMT_RGB888_1X24: > > vin->source.code = code.code; > > vin_dbg(vin, "Found supported media bus format: %d\n", > > @@ -57,6 +58,40 @@ static int rvin_mbus_supported(struct rvin_dev *vin) > > } > > } > > > > + /* > > + * Older versions where looking for the wrong media bus format. > > + * It where looking for a YUVY format but then treated it as a > > + * UYVY format. This was not noticed since atlest one subdevice > > + * used for testing (adv7180) reported a YUVY media bus format > > + * but provided UYVY data. There might be other unknown subdevices > > + * which also do this, to not break compatibility try to use them > > + * in legacy mode. > > + */ > > + found = false; > > + code.index = 0; > > + while (!v4l2_subdev_call(sd, pad, enum_mbus_code, NULL, &code)) { > > + code.index++; > > + switch (code.code) { > > + case MEDIA_BUS_FMT_YUYV8_2X8: > > + vin->source.code = MEDIA_BUS_FMT_UYVY8_2X8; > > + found = true; > > + break; > > + case MEDIA_BUS_FMT_YUYV10_2X10: > > + vin->source.code = MEDIA_BUS_FMT_UYVY10_2X10; > > + found = true; > > + break; > > + default: > > + break; > > + } > > + > > + if (found) { > > + vin_err(vin, > > + "media bus %d not supported, trying legacy mode %d\n", > > + code.code, vin->source.code); > > + return true; > > + } > > + } > > + > > return false; > > } > > > > diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c > > index dad3b03..0836b15 100644 > > --- a/drivers/media/platform/rcar-vin/rcar-dma.c > > +++ b/drivers/media/platform/rcar-vin/rcar-dma.c > > @@ -169,7 +169,7 @@ static int rvin_setup(struct rvin_dev *vin) > > vnmc |= VNMC_INF_YUV16; > > input_is_yuv = true; > > break; > > - case MEDIA_BUS_FMT_YUYV8_2X8: > > + case MEDIA_BUS_FMT_UYVY8_2X8: > > /* BT.656 8bit YCbCr422 or BT.601 8bit YCbCr422 */ > > vnmc |= vin->mbus_cfg.type == V4L2_MBUS_BT656 ? > > VNMC_INF_YUV8_BT656 : VNMC_INF_YUV8_BT601; > > @@ -178,7 +178,7 @@ static int rvin_setup(struct rvin_dev *vin) > > case MEDIA_BUS_FMT_RGB888_1X24: > > vnmc |= VNMC_INF_RGB888; > > break; > > - case MEDIA_BUS_FMT_YUYV10_2X10: > > + case MEDIA_BUS_FMT_UYVY10_2X10: > > /* BT.656 10bit YCbCr422 or BT.601 10bit YCbCr422 */ > > vnmc |= vin->mbus_cfg.type == V4L2_MBUS_BT656 ? > > VNMC_INF_YUV10_BT656 : VNMC_INF_YUV10_BT601; > >
On 07/20/2016 02:29 PM, Niklas Söderlund wrote: > Hi Hans, > > Thanks for your feedback. > > On 2016-07-20 11:48:40 +0200, Hans Verkuil wrote: >> On 07/08/2016 12:43 PM, Niklas Söderlund wrote: >>> A recent bugfix to adv7180 brought to light that the rcar-vin driver are >>> looking for the wrong media bus format. It was looking for a YUVU format >>> but then expecting UYVY data. The bugfix for adv7180 will break the >>> usage of rcar-vin together with a adv7180 as found on Renesas R-Car2 >>> Koelsch boards for example. >>> >>> This patch fix the rcar-vin driver to look for the correct UYVU formats >>> and adds a legacy mode. The legacy mode is needed since I don't know if >>> other devices provide a incorrect media bus format and I don't want to >>> break any working configurations. Hopefully the legacy mode can be >>> removed sometime in the future. >> >> I'd rather have a version without the legacy code. You have to assume that >> subdevs return correct values otherwise what's the point of the mediabus >> formats? >> >> So this is simply an adv7180 bug fix + this r-car fix to stay consistent >> with the adv7180. > > On principal I agree with you. My goal with this patch is just to make > sure there is no case where the rcar-vin driver won't work with the > adv7180. The plan was to drop the legacy mode in a separate patch after > both the adv7182 and rcar-vin patches where picked up. > > I'm happy to drop the 'legacy support' for the wrong formats from this > patch as long as I can be sure that there is no breaking. Should I > rewrite this patch to drop the wrong formats and submit it as a series > together with the adv7180 patch so they can be picked up together? Or do > you know of a better way? Why not combine this patch and the adv7180 patch in a single patch? Just keep Steve's Signed-off-by line together with yours. That way everything stays in sync. The only other user of the adv7180 doesn't look at the mediabus formats at all, so it isn't affected. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/20/2016 02:36 PM, Hans Verkuil wrote: > On 07/20/2016 02:29 PM, Niklas Söderlund wrote: >> Hi Hans, >> >> Thanks for your feedback. >> >> On 2016-07-20 11:48:40 +0200, Hans Verkuil wrote: >>> On 07/08/2016 12:43 PM, Niklas Söderlund wrote: >>>> A recent bugfix to adv7180 brought to light that the rcar-vin driver are >>>> looking for the wrong media bus format. It was looking for a YUVU format >>>> but then expecting UYVY data. The bugfix for adv7180 will break the >>>> usage of rcar-vin together with a adv7180 as found on Renesas R-Car2 >>>> Koelsch boards for example. >>>> >>>> This patch fix the rcar-vin driver to look for the correct UYVU formats >>>> and adds a legacy mode. The legacy mode is needed since I don't know if >>>> other devices provide a incorrect media bus format and I don't want to >>>> break any working configurations. Hopefully the legacy mode can be >>>> removed sometime in the future. >>> >>> I'd rather have a version without the legacy code. You have to assume that >>> subdevs return correct values otherwise what's the point of the mediabus >>> formats? >>> >>> So this is simply an adv7180 bug fix + this r-car fix to stay consistent >>> with the adv7180. >> >> On principal I agree with you. My goal with this patch is just to make >> sure there is no case where the rcar-vin driver won't work with the >> adv7180. The plan was to drop the legacy mode in a separate patch after >> both the adv7182 and rcar-vin patches where picked up. >> >> I'm happy to drop the 'legacy support' for the wrong formats from this >> patch as long as I can be sure that there is no breaking. Should I >> rewrite this patch to drop the wrong formats and submit it as a series >> together with the adv7180 patch so they can be picked up together? Or do >> you know of a better way? > > Why not combine this patch and the adv7180 patch in a single patch? Just keep > Steve's Signed-off-by line together with yours. That way everything stays > in sync. The only other user of the adv7180 doesn't look at the mediabus > formats at all, so it isn't affected. Niklas, Were you planning to make a combined adv7180/rcar-vin patch for this? I would prefer this solution rather than keeping legacy code around. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2016-08-22 11:12:59 +0200, Hans Verkuil wrote: > On 07/20/2016 02:36 PM, Hans Verkuil wrote: > > On 07/20/2016 02:29 PM, Niklas Söderlund wrote: > >> Hi Hans, > >> > >> Thanks for your feedback. > >> > >> On 2016-07-20 11:48:40 +0200, Hans Verkuil wrote: > >>> On 07/08/2016 12:43 PM, Niklas Söderlund wrote: > >>>> A recent bugfix to adv7180 brought to light that the rcar-vin driver are > >>>> looking for the wrong media bus format. It was looking for a YUVU format > >>>> but then expecting UYVY data. The bugfix for adv7180 will break the > >>>> usage of rcar-vin together with a adv7180 as found on Renesas R-Car2 > >>>> Koelsch boards for example. > >>>> > >>>> This patch fix the rcar-vin driver to look for the correct UYVU formats > >>>> and adds a legacy mode. The legacy mode is needed since I don't know if > >>>> other devices provide a incorrect media bus format and I don't want to > >>>> break any working configurations. Hopefully the legacy mode can be > >>>> removed sometime in the future. > >>> > >>> I'd rather have a version without the legacy code. You have to assume that > >>> subdevs return correct values otherwise what's the point of the mediabus > >>> formats? > >>> > >>> So this is simply an adv7180 bug fix + this r-car fix to stay consistent > >>> with the adv7180. > >> > >> On principal I agree with you. My goal with this patch is just to make > >> sure there is no case where the rcar-vin driver won't work with the > >> adv7180. The plan was to drop the legacy mode in a separate patch after > >> both the adv7182 and rcar-vin patches where picked up. > >> > >> I'm happy to drop the 'legacy support' for the wrong formats from this > >> patch as long as I can be sure that there is no breaking. Should I > >> rewrite this patch to drop the wrong formats and submit it as a series > >> together with the adv7180 patch so they can be picked up together? Or do > >> you know of a better way? > > > > Why not combine this patch and the adv7180 patch in a single patch? Just keep > > Steve's Signed-off-by line together with yours. That way everything stays > > in sync. The only other user of the adv7180 doesn't look at the mediabus > > formats at all, so it isn't affected. > > Niklas, > > Were you planning to make a combined adv7180/rcar-vin patch for this? > > I would prefer this solution rather than keeping legacy code around. Sorry for the delay Hans. I just posted a combined patch for adv7180 and rcar-vin. Thanks for picking up the cleanup patches!
diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c index 4b2007b..481d82a 100644 --- a/drivers/media/platform/rcar-vin/rcar-core.c +++ b/drivers/media/platform/rcar-vin/rcar-core.c @@ -37,6 +37,7 @@ static int rvin_mbus_supported(struct rvin_dev *vin) struct v4l2_subdev_mbus_code_enum code = { .which = V4L2_SUBDEV_FORMAT_ACTIVE, }; + bool found; sd = vin_to_source(vin); @@ -45,8 +46,8 @@ static int rvin_mbus_supported(struct rvin_dev *vin) code.index++; switch (code.code) { case MEDIA_BUS_FMT_YUYV8_1X16: - case MEDIA_BUS_FMT_YUYV8_2X8: - case MEDIA_BUS_FMT_YUYV10_2X10: + case MEDIA_BUS_FMT_UYVY8_2X8: + case MEDIA_BUS_FMT_UYVY10_2X10: case MEDIA_BUS_FMT_RGB888_1X24: vin->source.code = code.code; vin_dbg(vin, "Found supported media bus format: %d\n", @@ -57,6 +58,40 @@ static int rvin_mbus_supported(struct rvin_dev *vin) } } + /* + * Older versions where looking for the wrong media bus format. + * It where looking for a YUVY format but then treated it as a + * UYVY format. This was not noticed since atlest one subdevice + * used for testing (adv7180) reported a YUVY media bus format + * but provided UYVY data. There might be other unknown subdevices + * which also do this, to not break compatibility try to use them + * in legacy mode. + */ + found = false; + code.index = 0; + while (!v4l2_subdev_call(sd, pad, enum_mbus_code, NULL, &code)) { + code.index++; + switch (code.code) { + case MEDIA_BUS_FMT_YUYV8_2X8: + vin->source.code = MEDIA_BUS_FMT_UYVY8_2X8; + found = true; + break; + case MEDIA_BUS_FMT_YUYV10_2X10: + vin->source.code = MEDIA_BUS_FMT_UYVY10_2X10; + found = true; + break; + default: + break; + } + + if (found) { + vin_err(vin, + "media bus %d not supported, trying legacy mode %d\n", + code.code, vin->source.code); + return true; + } + } + return false; } diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c index dad3b03..0836b15 100644 --- a/drivers/media/platform/rcar-vin/rcar-dma.c +++ b/drivers/media/platform/rcar-vin/rcar-dma.c @@ -169,7 +169,7 @@ static int rvin_setup(struct rvin_dev *vin) vnmc |= VNMC_INF_YUV16; input_is_yuv = true; break; - case MEDIA_BUS_FMT_YUYV8_2X8: + case MEDIA_BUS_FMT_UYVY8_2X8: /* BT.656 8bit YCbCr422 or BT.601 8bit YCbCr422 */ vnmc |= vin->mbus_cfg.type == V4L2_MBUS_BT656 ? VNMC_INF_YUV8_BT656 : VNMC_INF_YUV8_BT601; @@ -178,7 +178,7 @@ static int rvin_setup(struct rvin_dev *vin) case MEDIA_BUS_FMT_RGB888_1X24: vnmc |= VNMC_INF_RGB888; break; - case MEDIA_BUS_FMT_YUYV10_2X10: + case MEDIA_BUS_FMT_UYVY10_2X10: /* BT.656 10bit YCbCr422 or BT.601 10bit YCbCr422 */ vnmc |= vin->mbus_cfg.type == V4L2_MBUS_BT656 ? VNMC_INF_YUV10_BT656 : VNMC_INF_YUV10_BT601;
A recent bugfix to adv7180 brought to light that the rcar-vin driver are looking for the wrong media bus format. It was looking for a YUVU format but then expecting UYVY data. The bugfix for adv7180 will break the usage of rcar-vin together with a adv7180 as found on Renesas R-Car2 Koelsch boards for example. This patch fix the rcar-vin driver to look for the correct UYVU formats and adds a legacy mode. The legacy mode is needed since I don't know if other devices provide a incorrect media bus format and I don't want to break any working configurations. Hopefully the legacy mode can be removed sometime in the future. Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> --- drivers/media/platform/rcar-vin/rcar-core.c | 39 +++++++++++++++++++++++++++-- drivers/media/platform/rcar-vin/rcar-dma.c | 4 +-- 2 files changed, 39 insertions(+), 4 deletions(-)