Hi Hans,
On Tuesday 18 March 2014 10:32:32 Hans Verkuil wrote:
> Hi Laurent,
>
> I've tested it and I thought I was going crazy. Everything was fine after
> applying this patch, but as soon as I applied the next patch (37/48) the
> colors were wrong. But that patch had nothing whatsoever to do with the
> bus ordering. You managed to make a small but crucial bug and it was pure
> bad luck that it ever worked.
>
> See details below:
>
> On 03/11/14 16:10, Laurent Pinchart wrote:
> > Replace the dummy video format operations by pad format operations that
> > configure the output format.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinch...@ideasonboard.com>
> > ---
> >
> > drivers/media/i2c/adv7604.c | 280 +++++++++++++++++++++++++++++++++++----
> > include/media/adv7604.h | 56 ++++-----
> > 2 files changed, 275 insertions(+), 61 deletions(-)
> >
> > diff --git a/drivers/media/i2c/adv7604.c b/drivers/media/i2c/adv7604.c
> > index 851b350..5aa7c29 100644
> > --- a/drivers/media/i2c/adv7604.c
> > +++ b/drivers/media/i2c/adv7604.c
> > @@ -53,6 +53,28 @@ MODULE_LICENSE("GPL");
> >
> > /* ADV7604 system clock frequency */
> > #define ADV7604_fsc (28636360)
> >
> > +#define ADV7604_RGB_OUT (1 << 1)
> > +
> > +#define ADV7604_OP_FORMAT_SEL_8BIT (0 << 0)
> > +#define ADV7604_OP_FORMAT_SEL_10BIT (1 << 0)
> > +#define ADV7604_OP_FORMAT_SEL_12BIT (2 << 0)
> > +
> > +#define ADV7604_OP_MODE_SEL_SDR_422 (0 << 5)
> > +#define ADV7604_OP_MODE_SEL_DDR_422 (1 << 5)
> > +#define ADV7604_OP_MODE_SEL_SDR_444 (2 << 5)
> > +#define ADV7604_OP_MODE_SEL_DDR_444 (3 << 5)
> > +#define ADV7604_OP_MODE_SEL_SDR_422_2X (4 << 5)
> > +#define ADV7604_OP_MODE_SEL_ADI_CM (5 << 5)
> > +
> > +#define ADV7604_OP_CH_SEL_GBR (0 << 5)
> > +#define ADV7604_OP_CH_SEL_GRB (1 << 5)
> > +#define ADV7604_OP_CH_SEL_BGR (2 << 5)
> > +#define ADV7604_OP_CH_SEL_RGB (3 << 5)
> > +#define ADV7604_OP_CH_SEL_BRG (4 << 5)
> > +#define ADV7604_OP_CH_SEL_RBG (5 << 5)
>
> Note that these values are shifted 5 bits to the left...
[snip]
> > +struct adv7604_format_info {
> > + enum v4l2_mbus_pixelcode code;
> > + u8 op_ch_sel;
> > + bool rgb_out;
> > + bool swap_cb_cr;
> > + u8 op_format_sel;
> > +};
[snip]
> > +static const struct adv7604_format_info adv7604_formats[] = {
> > + { V4L2_MBUS_FMT_RGB888_1X24, ADV7604_OP_CH_SEL_RGB, true, false,
> > + ADV7604_OP_MODE_SEL_SDR_444 | ADV7604_OP_FORMAT_SEL_8BIT },
> > + { V4L2_MBUS_FMT_YUYV8_2X8, ADV7604_OP_CH_SEL_RGB, false, false,
> > + ADV7604_OP_MODE_SEL_SDR_422 | ADV7604_OP_FORMAT_SEL_8BIT },
> > + { V4L2_MBUS_FMT_YVYU8_2X8, ADV7604_OP_CH_SEL_RGB, false, true,
> > + ADV7604_OP_MODE_SEL_SDR_422 | ADV7604_OP_FORMAT_SEL_8BIT },
> > + { V4L2_MBUS_FMT_YUYV10_2X10, ADV7604_OP_CH_SEL_RGB, false, false,
> > + ADV7604_OP_MODE_SEL_SDR_422 | ADV7604_OP_FORMAT_SEL_10BIT },
> > + { V4L2_MBUS_FMT_YVYU10_2X10, ADV7604_OP_CH_SEL_RGB, false, true,
> > + ADV7604_OP_MODE_SEL_SDR_422 | ADV7604_OP_FORMAT_SEL_10BIT },
> > + { V4L2_MBUS_FMT_YUYV12_2X12, ADV7604_OP_CH_SEL_RGB, false, false,
> > + ADV7604_OP_MODE_SEL_SDR_422 | ADV7604_OP_FORMAT_SEL_12BIT },
> > + { V4L2_MBUS_FMT_YVYU12_2X12, ADV7604_OP_CH_SEL_RGB, false, true,
> > + ADV7604_OP_MODE_SEL_SDR_422 | ADV7604_OP_FORMAT_SEL_12BIT },
> > + { V4L2_MBUS_FMT_UYVY8_1X16, ADV7604_OP_CH_SEL_RBG, false, false,
> > + ADV7604_OP_MODE_SEL_SDR_422_2X | ADV7604_OP_FORMAT_SEL_8BIT },
> > + { V4L2_MBUS_FMT_VYUY8_1X16, ADV7604_OP_CH_SEL_RBG, false, true,
> > + ADV7604_OP_MODE_SEL_SDR_422_2X | ADV7604_OP_FORMAT_SEL_8BIT },
> > + { V4L2_MBUS_FMT_YUYV8_1X16, ADV7604_OP_CH_SEL_RGB, false, false,
> > + ADV7604_OP_MODE_SEL_SDR_422_2X | ADV7604_OP_FORMAT_SEL_8BIT },
> > + { V4L2_MBUS_FMT_YVYU8_1X16, ADV7604_OP_CH_SEL_RGB, false, true,
> > + ADV7604_OP_MODE_SEL_SDR_422_2X | ADV7604_OP_FORMAT_SEL_8BIT },
> > + { V4L2_MBUS_FMT_UYVY10_1X20, ADV7604_OP_CH_SEL_RBG, false, false,
> > + ADV7604_OP_MODE_SEL_SDR_422_2X | ADV7604_OP_FORMAT_SEL_10BIT },
> > + { V4L2_MBUS_FMT_VYUY10_1X20, ADV7604_OP_CH_SEL_RBG, false, true,
> > + ADV7604_OP_MODE_SEL_SDR_422_2X | ADV7604_OP_FORMAT_SEL_10BIT },
> > + { V4L2_MBUS_FMT_YUYV10_1X20, ADV7604_OP_CH_SEL_RGB, false, false,
> > + ADV7604_OP_MODE_SEL_SDR_422_2X | ADV7604_OP_FORMAT_SEL_10BIT },
> > + { V4L2_MBUS_FMT_YVYU10_1X20, ADV7604_OP_CH_SEL_RGB, false, true,
> > + ADV7604_OP_MODE_SEL_SDR_422_2X | ADV7604_OP_FORMAT_SEL_10BIT },
> > + { V4L2_MBUS_FMT_UYVY12_1X24, ADV7604_OP_CH_SEL_RBG, false, false,
> > + ADV7604_OP_MODE_SEL_SDR_422_2X | ADV7604_OP_FORMAT_SEL_12BIT },
> > + { V4L2_MBUS_FMT_VYUY12_1X24, ADV7604_OP_CH_SEL_RBG, false, true,
> > + ADV7604_OP_MODE_SEL_SDR_422_2X | ADV7604_OP_FORMAT_SEL_12BIT },
> > + { V4L2_MBUS_FMT_YUYV12_1X24, ADV7604_OP_CH_SEL_RGB, false, false,
> > + ADV7604_OP_MODE_SEL_SDR_422_2X | ADV7604_OP_FORMAT_SEL_12BIT },
> > + { V4L2_MBUS_FMT_YVYU12_1X24, ADV7604_OP_CH_SEL_RGB, false, true,
> > + ADV7604_OP_MODE_SEL_SDR_422_2X | ADV7604_OP_FORMAT_SEL_12BIT },
> > +};
> > +
> > +static const struct adv7604_format_info adv7611_formats[] = {
> > + { V4L2_MBUS_FMT_RGB888_1X24, ADV7604_OP_CH_SEL_RGB, true, false,
> > + ADV7604_OP_MODE_SEL_SDR_444 | ADV7604_OP_FORMAT_SEL_8BIT },
> > + { V4L2_MBUS_FMT_YUYV8_2X8, ADV7604_OP_CH_SEL_RGB, false, false,
> > + ADV7604_OP_MODE_SEL_SDR_422 | ADV7604_OP_FORMAT_SEL_8BIT },
> > + { V4L2_MBUS_FMT_YVYU8_2X8, ADV7604_OP_CH_SEL_RGB, false, true,
> > + ADV7604_OP_MODE_SEL_SDR_422 | ADV7604_OP_FORMAT_SEL_8BIT },
> > + { V4L2_MBUS_FMT_YUYV12_2X12, ADV7604_OP_CH_SEL_RGB, false, false,
> > + ADV7604_OP_MODE_SEL_SDR_422 | ADV7604_OP_FORMAT_SEL_12BIT },
> > + { V4L2_MBUS_FMT_YVYU12_2X12, ADV7604_OP_CH_SEL_RGB, false, true,
> > + ADV7604_OP_MODE_SEL_SDR_422 | ADV7604_OP_FORMAT_SEL_12BIT },
> > + { V4L2_MBUS_FMT_UYVY8_1X16, ADV7604_OP_CH_SEL_RBG, false, false,
> > + ADV7604_OP_MODE_SEL_SDR_422_2X | ADV7604_OP_FORMAT_SEL_8BIT },
> > + { V4L2_MBUS_FMT_VYUY8_1X16, ADV7604_OP_CH_SEL_RBG, false, true,
> > + ADV7604_OP_MODE_SEL_SDR_422_2X | ADV7604_OP_FORMAT_SEL_8BIT },
> > + { V4L2_MBUS_FMT_YUYV8_1X16, ADV7604_OP_CH_SEL_RGB, false, false,
> > + ADV7604_OP_MODE_SEL_SDR_422_2X | ADV7604_OP_FORMAT_SEL_8BIT },
> > + { V4L2_MBUS_FMT_YVYU8_1X16, ADV7604_OP_CH_SEL_RGB, false, true,
> > + ADV7604_OP_MODE_SEL_SDR_422_2X | ADV7604_OP_FORMAT_SEL_8BIT },
> > + { V4L2_MBUS_FMT_UYVY12_1X24, ADV7604_OP_CH_SEL_RBG, false, false,
> > + ADV7604_OP_MODE_SEL_SDR_422_2X | ADV7604_OP_FORMAT_SEL_12BIT },
> > + { V4L2_MBUS_FMT_VYUY12_1X24, ADV7604_OP_CH_SEL_RBG, false, true,
> > + ADV7604_OP_MODE_SEL_SDR_422_2X | ADV7604_OP_FORMAT_SEL_12BIT },
> > + { V4L2_MBUS_FMT_YUYV12_1X24, ADV7604_OP_CH_SEL_RGB, false, false,
> > + ADV7604_OP_MODE_SEL_SDR_422_2X | ADV7604_OP_FORMAT_SEL_12BIT },
> > + { V4L2_MBUS_FMT_YVYU12_1X24, ADV7604_OP_CH_SEL_RGB, false, true,
> > + ADV7604_OP_MODE_SEL_SDR_422_2X | ADV7604_OP_FORMAT_SEL_12BIT },
> > +};
[snip]
> > +/*
> > + * Compute the op_ch_sel value required to obtain on the bus the
> > component order
> > + * corresponding to the selected format taking into account bus
> > reordering
> > + * applied by the board at the output of the device.
> > + *
> > + * The following table gives the op_ch_value from the format component
> > order
> > + * (expressed as op_ch_sel value in column) and the bus reordering
> > (expressed as
> > + * adv7604_bus_order value in row).
> > + *
> > + * | GBR(0) GRB(1) BGR(2) RGB(3) BRG(4) RBG(5)
> > + * ----------+-------------------------------------------------
> > + * RGB (NOP) | GBR GRB BGR RGB BRG RBG
> > + * GRB (1-2) | BGR RGB GBR GRB RBG BRG
> > + * RBG (2-3) | GRB GBR BRG RBG BGR RGB
> > + * BGR (1-3) | RBG BRG RGB BGR GRB GBR
> > + * BRG (ROR) | BRG RBG GRB GBR RGB BGR
> > + * GBR (ROL) | RGB BGR RBG BRG GBR GRB
> > + */
> > +static unsigned int adv7604_op_ch_sel(struct adv7604_state *state)
> > +{
> > +#define _SEL(a,b,c,d,e,f) { \
> > + ADV7604_OP_CH_SEL_##a, ADV7604_OP_CH_SEL_##b, ADV7604_OP_CH_SEL_##c,
\
> > + ADV7604_OP_CH_SEL_##d, ADV7604_OP_CH_SEL_##e, ADV7604_OP_CH_SEL_##f }
> > +#define _BUS(x) [ADV7604_BUS_ORDER_##x]
> > +
> > + static const unsigned int op_ch_sel[6][6] = {
> > + _BUS(RGB) /* NOP */ = _SEL(GBR, GRB, BGR, RGB, BRG, RBG),
> > + _BUS(GRB) /* 1-2 */ = _SEL(BGR, RGB, GBR, GRB, RBG, BRG),
> > + _BUS(RBG) /* 2-3 */ = _SEL(GRB, GBR, BRG, RBG, BGR, RGB),
> > + _BUS(BGR) /* 1-3 */ = _SEL(RBG, BRG, RGB, BGR, GRB, GBR),
> > + _BUS(BRG) /* ROR */ = _SEL(BRG, RBG, GRB, GBR, RGB, BGR),
> > + _BUS(GBR) /* ROL */ = _SEL(RGB, BGR, RBG, BRG, GBR, GRB),
> > + };
> > +
> > + return op_ch_sel[state->pdata.bus_order][state->format->op_ch_sel];
>
> But you don't shift state->format->op_ch_sel back 5 bits to the right, so
> you end up with a random memory value. It should be:
>
> return op_ch_sel[state->pdata.bus_order][state->format->op_ch_sel >> 5];
>
> After correcting this everything worked fine for me.
Good catch ! Thank you. I've fixed that and submitted v4.
In addition to this patch, I'm only missing your Acked-by or Reviewed-by tag
for patch 47/48 ("adv7604: Add LLC polarity configuration"). Could you please
provide that ? I'll then send a pull request to Mauro for the whole series.
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
