Re: [PATCH 02/13] media: v4l2: taint pads with the signal types for consumer devices
Em Wed, 26 Sep 2018 17:09:19 +0300
Laurent Pinchart escreveu:
> Hi Mauro,
>
> Thank you for the patch.
>
> Could you please CC me on patches touching the media controller core ? I can
> send a MAINTAINERS patch to make sure that gets handled automatically.
>
> On Wednesday, 1 August 2018 18:55:04 EEST Mauro Carvalho Chehab wrote:
> > Consumer devices are provided with a wide diferent range of types
> > supported by the same driver, allowing different configutations.
> >
> > In order to make easier to setup media controller links, "taint"
> > pads with the signal type it carries.
> >
> > While here, get rid of DEMOD_PAD_VBI_OUT, as the signal it carries
> > is actually the same as the normal video output.
> >
> > The difference happens at the video/VBI interface:
> > - for VBI, only the hidden lines are streamed;
> > - for video, the stream is usually cropped to hide the
> > vbi lines.
> >
> > Signed-off-by: Mauro Carvalho Chehab
> > ---
> > drivers/media/dvb-frontends/au8522_decoder.c | 3 ++
> > drivers/media/i2c/msp3400-driver.c | 2 ++
> > drivers/media/i2c/saa7115.c | 2 ++
> > drivers/media/i2c/tvp5150.c | 2 ++
> > drivers/media/pci/saa7134/saa7134-core.c | 2 ++
> > drivers/media/tuners/si2157.c| 3 ++
> > drivers/media/usb/dvb-usb-v2/mxl111sf.c | 2 ++
> > drivers/media/v4l2-core/tuner-core.c | 5 +++
> > include/media/media-entity.h | 35
> > 9 files changed, 56 insertions(+)
>
> [snip]
>
> > diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> > index 3aa3d58d1d58..8bfbe6b59fa9 100644
> > --- a/include/media/media-entity.h
> > +++ b/include/media/media-entity.h
> > @@ -155,6 +155,40 @@ struct media_link {
> > bool is_backlink;
> > };
> >
> > +/**
> > + * struct media_pad_signal_type - type of the signal inside a media pad
>
> I'd say "carried by a media pad" instead of "inside a media pad".
Ok.
>
> > + *
> > + * @PAD_SIGNAL_DEFAULT
>
> Shouldn't we use a MEDIA_PAD_ prefix ?
Ok.
>
> > + * Default signal. Use this when all inputs or all outputs are
> > + * uniquely identified by the pad number.
>
> How about "Use this when the pad can carry a single signal type" ? I
> understand your formulation as meaning that all pads of the entity have to be
> of the default type, or none can be.
Describing the MEDIA_PAD_SIGNAL_DEFAULT is not trivial.
Basically, almost all pads at the subsystem are MEDIA_PAD_SIGNAL_DEFAULT.
Only certain types of entities need to "taint" the pad (usually,
the entities related to the tuning and audio/video decoding stages).
In other words, the decision if it should either use
MEDIA_PAD_SIGNAL_DEFAULT or not is per-entity (and not per-pad).
Typically, it is needed when a certain entity function may have
pads at the same direction with different meanings.
A real example would be a digital tuner entity that has two pad sources:
- pad source #0 - carries a RF signal;
- pad source #1 - carries a digital audio signal.
On this case, both pad sources should use a signal different than
MEDIA_PAD_SIGNAL_DEFAULT.
Writing it on a concise way is not a trivial task ;-)
Maybe something like would work:
"Default signal. Use this when there's no need to specify the
signal carried inside a pad."
>
> > + * @PAD_SIGNAL_ANALOG
>
> This isn't a very good name given that PAD_SIGNAL_TV_CARRIERS is also analog.
>
> > + * The pad contains an analog signa. It can be Radio Frequency,
>
> s/signa/signal/
Ok (this is actually fixed already at the version applied).
> > + * Intermediate Frequency or baseband signal.
> > + * Tuner inputs, composite and s-video signals should use it.
> > + * On tuner sources, this is used for digital TV demodulators and for
> > + * IF-PLL demodulator like tda9887.
> > + * @PAD_SIGNAL_TV_CARRIERS
> > + * The pad contains analog signals carrying either a digital or an analog
> > + * modulated (or baseband) signal.
>
> As above, maybe "The pad carries either ...".
Replaced "contains" with "carries".
> > This is provided by tuner source
> > + * pads and used by analog TV standard decoders and by digital TV demods.
> > + * @PAD_SIGNAL_DV
> > + * Contains a digital video signal, with can be a bitstream of samples
> > + * taken from an analog TV video source. On such case, it usually
> > + * contains the VBI data on it.
> > + * @PAD_SIGNAL_AUDIO
> > + * Contains an Intermediate Frequency analog signal from an audio
> > + * sub-carrier or an audio bitstream. IF signals are provided by tuners
> > + * and consumed by audio AM/FM decoders. Bitstream audio is provided by
>
> s/ / /
>
> > + * an audio decoder.
>
> Generally speaking the types you propose here seem quite ad-hoc, without much
> coherency. For instance you split analog and digital video, but group all
> audio under a single type. It's also not very clear from the description how
> to
Re: [PATCH 03/13] v4l2-mc: switch it to use the new approach to setup pipelines
Em Wed, 26 Sep 2018 17:44:53 +0300
Laurent Pinchart escreveu:
> Hi Mauro,
>
> Thank you for the patch.
>
> On Wednesday, 1 August 2018 18:55:05 EEST Mauro Carvalho Chehab wrote:
> > Instead of relying on a static map for pids, use the new sig_type
> > "taint" type to setup the pipelines with the same tipe between
>
> s/tipe/type/
>
> > different entities.
> >
> > Signed-off-by: Mauro Carvalho Chehab
> > ---
> > drivers/media/media-entity.c | 26 +++
> > drivers/media/v4l2-core/v4l2-mc.c | 73 ---
> > include/media/media-entity.h | 19
> > 3 files changed, 101 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> > index 3498551e618e..0b1cb3559140 100644
> > --- a/drivers/media/media-entity.c
> > +++ b/drivers/media/media-entity.c
> > @@ -662,6 +662,32 @@ static void __media_entity_remove_link(struct
> > media_entity *entity, kfree(link);
> > }
> >
> > +int media_get_pad_index(struct media_entity *entity, bool is_sink,
> > + enum media_pad_signal_type sig_type)
> > +{
> > + int i;
>
> is is never negative, please use an unsigned int.
>
> > + bool pad_is_sink;
> > +
> > + if (!entity)
> > + return -EINVAL;
> > +
> > + for (i = 0; i < entity->num_pads; i++) {
> > + if (entity->pads[i].flags == MEDIA_PAD_FL_SINK)
> > + pad_is_sink = true;
> > + else if (entity->pads[i].flags == MEDIA_PAD_FL_SOURCE)
> > + pad_is_sink = false;
> > + else
> > + continue; /* This is an error! */
> > +
> > + if (pad_is_sink != is_sink)
> > + continue;
> > + if (entity->pads[i].sig_type == sig_type)
> > + return i;
> > + }
> > + return -EINVAL;
> > +}
> > +EXPORT_SYMBOL_GPL(media_get_pad_index);
> > +
> > int
> > media_create_pad_link(struct media_entity *source, u16 source_pad,
> > struct media_entity *sink, u16 sink_pad, u32 flags)
> > diff --git a/drivers/media/v4l2-core/v4l2-mc.c
> > b/drivers/media/v4l2-core/v4l2-mc.c index 982bab3530f6..1925e1a3b861 100644
> > --- a/drivers/media/v4l2-core/v4l2-mc.c
> > +++ b/drivers/media/v4l2-core/v4l2-mc.c
> > @@ -28,7 +28,7 @@ int v4l2_mc_create_media_graph(struct media_device *mdev)
> > struct media_entity *io_v4l = NULL, *io_vbi = NULL, *io_swradio = NULL;
> > bool is_webcam = false;
> > u32 flags;
> > - int ret;
> > + int ret, pad_sink, pad_source;
> >
> > if (!mdev)
> > return 0;
> > @@ -97,29 +97,52 @@ int v4l2_mc_create_media_graph(struct media_device
> > *mdev) /* Link the tuner and IF video output pads */
> > if (tuner) {
> > if (if_vid) {
> > - ret = media_create_pad_link(tuner, TUNER_PAD_OUTPUT,
> > - if_vid,
> > - IF_VID_DEC_PAD_IF_INPUT,
> > + pad_source = media_get_pad_index(tuner, false,
> > +PAD_SIGNAL_ANALOG);
> > + pad_sink = media_get_pad_index(if_vid, true,
> > + PAD_SIGNAL_ANALOG);
> > + if (pad_source < 0 || pad_sink < 0)
> > + return -EINVAL;
> > + ret = media_create_pad_link(tuner, pad_source,
> > + if_vid, pad_sink,
> > MEDIA_LNK_FL_ENABLED);
> > if (ret)
> > return ret;
> > - ret = media_create_pad_link(if_vid, IF_VID_DEC_PAD_OUT,
> > - decoder, DEMOD_PAD_IF_INPUT,
> > +
> > + pad_source = media_get_pad_index(if_vid, false,
> > +PAD_SIGNAL_DV);
> > + pad_sink = media_get_pad_index(decoder, true,
> > + PAD_SIGNAL_DV);
> > + if (pad_source < 0 || pad_sink < 0)
> > + return -EINVAL;
> > + ret = media_create_pad_link(if_vid, pad_source,
> > + decoder, pad_sink,
> > MEDIA_LNK_FL_ENABLED);
> > if (ret)
> > return ret;
> > } else {
> > - ret = media_create_pad_link(tuner, TUNER_PAD_OUTPUT,
> > - decoder, DEMOD_PAD_IF_INPUT,
> > + pad_source = media_get_pad_index(tuner, false,
> > +PAD_SIGNAL_ANALOG);
> > + pad_sink = media_get_pad_index(decoder, true,
> > +
[PATCH 3/4] media: dt-bindings: media: Document pclk-max-frequency property
This optional property aims to inform parallel video devices of the maximum pixel clock frequency admissible by host video interface. If bandwidth of data to be transferred requires a pixel clock which is higher than this value, parallel video device could then typically adapt framerate to reach this constraint. Signed-off-by: Hugues Fruchet --- Documentation/devicetree/bindings/media/video-interfaces.txt | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt index baf9d97..fa4c112 100644 --- a/Documentation/devicetree/bindings/media/video-interfaces.txt +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt @@ -147,6 +147,8 @@ Optional endpoint properties as 0 (normal). This property is valid for serial busses only. - strobe: Whether the clock signal is used as clock (0) or strobe (1). Used with CCP2, for instance. +- pclk-max-frequency: maximum pixel clock frequency admissible by video + host interface. Example --- -- 2.7.4
[PATCH 4/4] media: ov5640: reduce rate according to maximum pixel clock frequency
Reduce parallel port rate according to maximum pixel clock frequency
admissible by camera interface.
This allows to support any resolutions/framerate requests by decreasing
the framerate according to maximum camera interface capabilities.
Signed-off-by: Hugues Fruchet
---
drivers/media/i2c/ov5640.c | 12
1 file changed, 12 insertions(+)
diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index da4d754..9f3c32e 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -918,6 +918,8 @@ static int ov5640_set_dvp_pclk(struct ov5640_dev *sensor,
{
u8 prediv, mult, sysdiv, pll_rdiv, bit_div, pclk_div;
int ret;
+ struct i2c_client *client = sensor->i2c_client;
+ unsigned int pclk_freq, max_pclk_freq;
/*
* FIXME, value of PCLK divider deduced from
* mode registers hardcoded sequence and tests
@@ -941,6 +943,16 @@ static int ov5640_set_dvp_pclk(struct ov5640_dev *sensor,
if (ret)
return ret;
+ pclk_freq = rate / dvp_pclk_divider;
+ max_pclk_freq = sensor->ep.bus.parallel.pclk_max_frequency;
+
+ /* clip rate according to optional maximum pixel clock limit */
+ if (max_pclk_freq && pclk_freq > max_pclk_freq) {
+ rate = max_pclk_freq * dvp_pclk_divider;
+ dev_dbg(&client->dev, "DVP pixel clock too high (%d > %d Hz),
reducing rate...\n",
+ pclk_freq, max_pclk_freq);
+ }
+
ov5640_calc_pclk(sensor, rate, &prediv, &mult, &sysdiv, &pll_rdiv,
&bit_div, &pclk_div);
--
2.7.4
[PATCH 2/4] media: v4l2-core: add pixel clock max frequency parallel port property
Add pclk-max-frequency property in parallel port endpoint in order
to inform sensor of the maximum pixel clock frequency admissible
by camera interface that is connected on.
Signed-off-by: Hugues Fruchet
---
drivers/media/v4l2-core/v4l2-fwnode.c | 3 +++
include/media/v4l2-fwnode.h | 2 ++
2 files changed, 5 insertions(+)
diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c
b/drivers/media/v4l2-core/v4l2-fwnode.c
index 169bdbb..505338e 100644
--- a/drivers/media/v4l2-core/v4l2-fwnode.c
+++ b/drivers/media/v4l2-core/v4l2-fwnode.c
@@ -158,6 +158,9 @@ static void v4l2_fwnode_endpoint_parse_parallel_bus(
flags |= v ? V4L2_MBUS_DATA_ENABLE_HIGH :
V4L2_MBUS_DATA_ENABLE_LOW;
+ if (!fwnode_property_read_u32(fwnode, "pclk-max-frequency", &v))
+ bus->pclk_max_frequency = v;
+
bus->flags = flags;
}
diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
index 9cccab6..946b48d 100644
--- a/include/media/v4l2-fwnode.h
+++ b/include/media/v4l2-fwnode.h
@@ -52,11 +52,13 @@ struct v4l2_fwnode_bus_mipi_csi2 {
* @flags: media bus (V4L2_MBUS_*) flags
* @bus_width: bus width in bits
* @data_shift: data shift in bits
+ * @max_pclk_frequency: maximum pixel clock in hertz
*/
struct v4l2_fwnode_bus_parallel {
unsigned int flags;
unsigned char bus_width;
unsigned char data_shift;
+ unsigned int pclk_max_frequency;
};
/**
--
2.7.4
[PATCH 1/4] media: ov5640: move parallel port pixel clock divider out of registers set
Set DVP_PCLK_DIVIDER (0x3824) and VFIFO_CTRL0C (0x460c) registers
in ov5640_set_dvp_pclk() according to mode selected.
Signed-off-by: Hugues Fruchet
---
drivers/media/i2c/ov5640.c | 66 --
1 file changed, 46 insertions(+), 20 deletions(-)
diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 1a64bb6..da4d754 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -66,6 +66,7 @@
#define OV5640_REG_TIMING_VTS 0x380e
#define OV5640_REG_TIMING_TC_REG20 0x3820
#define OV5640_REG_TIMING_TC_REG21 0x3821
+#define OV5640_REG_DVP_PCLK_DIVIDER0x3824
#define OV5640_REG_AEC_CTRL00 0x3a00
#define OV5640_REG_AEC_B50_STEP0x3a08
#define OV5640_REG_AEC_B60_STEP0x3a0a
@@ -82,6 +83,7 @@
#define OV5640_REG_SIGMADELTA_CTRL0C 0x3c0c
#define OV5640_REG_FRAME_CTRL010x4202
#define OV5640_REG_FORMAT_CONTROL000x4300
+#define OV5640_REG_VFIFO_CTRL0C0x460C
#define OV5640_REG_POLARITY_CTRL00 0x4740
#define OV5640_REG_MIPI_CTRL00 0x4800
#define OV5640_REG_DEBUG_MODE 0x4814
@@ -355,8 +357,8 @@ static const struct reg_value ov5640_setting_VGA_640_480[]
= {
{0x3a0a, 0x00, 0, 0}, {0x3a0b, 0xf6, 0, 0}, {0x3a0e, 0x03, 0, 0},
{0x3a0d, 0x04, 0, 0}, {0x3a14, 0x03, 0, 0}, {0x3a15, 0xd8, 0, 0},
{0x4001, 0x02, 0, 0}, {0x4004, 0x02, 0, 0}, {0x4713, 0x03, 0, 0},
- {0x4407, 0x04, 0, 0}, {0x460b, 0x35, 0, 0}, {0x460c, 0x22, 0, 0},
- {0x3824, 0x02, 0, 0}, {0x5001, 0xa3, 0, 0},
+ {0x4407, 0x04, 0, 0}, {0x460b, 0x35, 0, 0},
+ {0x5001, 0xa3, 0, 0},
};
static const struct reg_value ov5640_setting_XGA_1024_768[] = {
@@ -374,8 +376,8 @@ static const struct reg_value ov5640_setting_XGA_1024_768[]
= {
{0x3a0a, 0x00, 0, 0}, {0x3a0b, 0xf6, 0, 0}, {0x3a0e, 0x03, 0, 0},
{0x3a0d, 0x04, 0, 0}, {0x3a14, 0x03, 0, 0}, {0x3a15, 0xd8, 0, 0},
{0x4001, 0x02, 0, 0}, {0x4004, 0x02, 0, 0}, {0x4713, 0x03, 0, 0},
- {0x4407, 0x04, 0, 0}, {0x460b, 0x35, 0, 0}, {0x460c, 0x22, 0, 0},
- {0x3824, 0x02, 0, 0}, {0x5001, 0xa3, 0, 0},
+ {0x4407, 0x04, 0, 0}, {0x460b, 0x35, 0, 0},
+ {0x5001, 0xa3, 0, 0},
};
static const struct reg_value ov5640_setting_QVGA_320_240[] = {
@@ -393,8 +395,8 @@ static const struct reg_value ov5640_setting_QVGA_320_240[]
= {
{0x3a0a, 0x00, 0, 0}, {0x3a0b, 0xf6, 0, 0}, {0x3a0e, 0x03, 0, 0},
{0x3a0d, 0x04, 0, 0}, {0x3a14, 0x03, 0, 0}, {0x3a15, 0xd8, 0, 0},
{0x4001, 0x02, 0, 0}, {0x4004, 0x02, 0, 0}, {0x4713, 0x03, 0, 0},
- {0x4407, 0x04, 0, 0}, {0x460b, 0x35, 0, 0}, {0x460c, 0x22, 0, 0},
- {0x3824, 0x02, 0, 0}, {0x5001, 0xa3, 0, 0},
+ {0x4407, 0x04, 0, 0}, {0x460b, 0x35, 0, 0},
+ {0x5001, 0xa3, 0, 0},
};
static const struct reg_value ov5640_setting_QCIF_176_144[] = {
@@ -412,8 +414,8 @@ static const struct reg_value ov5640_setting_QCIF_176_144[]
= {
{0x3a0a, 0x00, 0, 0}, {0x3a0b, 0xf6, 0, 0}, {0x3a0e, 0x03, 0, 0},
{0x3a0d, 0x04, 0, 0}, {0x3a14, 0x03, 0, 0}, {0x3a15, 0xd8, 0, 0},
{0x4001, 0x02, 0, 0}, {0x4004, 0x02, 0, 0}, {0x4713, 0x03, 0, 0},
- {0x4407, 0x04, 0, 0}, {0x460b, 0x35, 0, 0}, {0x460c, 0x22, 0, 0},
- {0x3824, 0x02, 0, 0}, {0x5001, 0xa3, 0, 0},
+ {0x4407, 0x04, 0, 0}, {0x460b, 0x35, 0, 0},
+ {0x5001, 0xa3, 0, 0},
};
static const struct reg_value ov5640_setting_NTSC_720_480[] = {
@@ -431,8 +433,8 @@ static const struct reg_value ov5640_setting_NTSC_720_480[]
= {
{0x3a0a, 0x00, 0, 0}, {0x3a0b, 0xf6, 0, 0}, {0x3a0e, 0x03, 0, 0},
{0x3a0d, 0x04, 0, 0}, {0x3a14, 0x03, 0, 0}, {0x3a15, 0xd8, 0, 0},
{0x4001, 0x02, 0, 0}, {0x4004, 0x02, 0, 0}, {0x4713, 0x03, 0, 0},
- {0x4407, 0x04, 0, 0}, {0x460b, 0x35, 0, 0}, {0x460c, 0x22, 0, 0},
- {0x3824, 0x02, 0, 0}, {0x5001, 0xa3, 0, 0},
+ {0x4407, 0x04, 0, 0}, {0x460b, 0x35, 0, 0},
+ {0x5001, 0xa3, 0, 0},
};
static const struct reg_value ov5640_setting_PAL_720_576[] = {
@@ -450,8 +452,8 @@ static const struct reg_value ov5640_setting_PAL_720_576[]
= {
{0x3a0a, 0x00, 0, 0}, {0x3a0b, 0xf6, 0, 0}, {0x3a0e, 0x03, 0, 0},
{0x3a0d, 0x04, 0, 0}, {0x3a14, 0x03, 0, 0}, {0x3a15, 0xd8, 0, 0},
{0x4001, 0x02, 0, 0}, {0x4004, 0x02, 0, 0}, {0x4713, 0x03, 0, 0},
- {0x4407, 0x04, 0, 0}, {0x460b, 0x35, 0, 0}, {0x460c, 0x22, 0, 0},
- {0x3824, 0x02, 0, 0}, {0x5001, 0xa3, 0, 0},
+ {0x4407, 0x04, 0, 0}, {0x460b, 0x35, 0, 0},
+ {0x5001, 0xa3, 0, 0},
};
static const struct reg_value ov5640_setting_720P_1280_720[] = {
@@ -469,8 +471,8 @@ static const struct reg_value
ov5640_setting_720P_1280_720[] = {
{0x3a0a, 0x01, 0, 0}, {0x3a0b, 0x72, 0, 0}, {0x3a0e, 0x01, 0, 0},
{0x3a0d, 0x02, 0, 0}, {0x3a14, 0x02, 0, 0}, {0x3a15, 0xe4, 0, 0},
{0x4001, 0x02, 0, 0}, {0x4004, 0x02, 0, 0}, {0x4713, 0x02, 0, 0},
[PATCH 0/4] OV5640: reduce rate according to maximum pixel clock
This patch serie aims to reduce parallel port rate according to maximum pixel clock frequency admissible by camera interface in front of the sensor. This allows to support any resolutions/framerate requests by decreasing the framerate according to maximum camera interface capabilities. This allows typically to enable 5Mp YUV/RGB frame capture even if 15fps framerate could not be reached by platform. This work is based on OV5640 Maxime Ripard's runtime clock computing serie [1] which allows to adapt the clock tree registers according to maximum pixel clock. Then the first patch adds handling of pclk divider registers DVP_PCLK_DIVIDER (0x3824) and VFIFO_CTRL0C (0x460c) in order to correlate the rate to the effective pixel clock output on parallel interface. A new devicetree property "pclk-max-frequency" is introduced in order to inform sensor of the camera interface maximum admissible pixel clock. This new devicetree property handling is added to V4L2 core. Then OV5640 ov5640_set_dvp_pclk() is modified to clip rate according to optional maximum pixel clock property. References: [1] [PATCH v3 00/12] media: ov5640: Misc cleanup and improvements https://www.mail-archive.com/linux-media@vger.kernel.org/msg131655.html Hugues Fruchet (4): media: ov5640: move parallel port pixel clock divider out of registers set media: v4l2-core: add pixel clock max frequency parallel port property media: dt-bindings: media: Document pclk-max-frequency property media: ov5640: reduce rate according to maximum pixel clock frequency .../devicetree/bindings/media/video-interfaces.txt | 2 + drivers/media/i2c/ov5640.c | 78 -- drivers/media/v4l2-core/v4l2-fwnode.c | 3 + include/media/v4l2-fwnode.h| 2 + 4 files changed, 65 insertions(+), 20 deletions(-) -- 2.7.4
For editing of your photos 18
Do you have needs for your photos cutting out and retouching? We do editing for e-commerce photos, portrait photos and wedding photos. You may choose to send us one or tow photos, we will provide testing to check quality. Thanks, Jessica
Re: [PATCH v3 00/12] media: ov5640: Misc cleanup and improvements
Hi Maxime & all OV5640 stakeholders, I've just pushed a new patchset also related to rate/pixel clock handling [1], based on your V3 great work: >media: ov5640: Adjust the clock based on the expected rate >media: ov5640: Remove the clocks registers initialization >media: ov5640: Remove redundant defines >media: ov5640: Remove redundant register setup >media: ov5640: Compute the clock rate at runtime >media: ov5640: Remove pixel clock rates >media: ov5640: Enhance FPS handling This is working perfectly fine on my parallel setup and allows me to well support VGA@30fps (instead 27) and also support XGA(1024x768)@15fps that I never seen working before. So at least for the parallel setup, this serie is working fine for all the discrete resolutions and framerate exposed by the driver for the moment: * QCIF 176x144 15/30fps * QVGA 320x240 15/30fps * VGA 640x480 15/30fps * 480p 720x480 15/30fps * XGA 1024x768 15/30fps * 720p 1280x720 15/30fps * 1080p 1920x1080 15/30fps * 5Mp 2592x1944 15fps Moreover I'm not clear on relationship between rate and pixel clock frequency. I've understood that to DVP_PCLK_DIVIDER (0x3824) register and VFIFO_CTRL0C (0x460c) affects the effective pixel clock frequency. All the resolutions up to 720x576 are forcing a manual value of 2 for divider (0x460c=0x22), but including 720p and more, the divider value is controlled by "auto-mode" (0x460c=0x20)... from what I measured and understood, for those resolutions, the divider must be set to 1 in order that your rate computation match the effective pixel clock on output, see [2]. So I wonder if this PCLK divider register should be included or not into your rate computation, what do you think ? [1] OV5640: reduce rate according to maximum pixel clock https://www.spinics.net/lists/linux-media/msg140958.html: [2] media: ov5640: move parallel port pixel clock divider out of registers set https://www.spinics.net/lists/linux-media/msg140960.html BR, Hugues. On 05/17/2018 10:53 AM, Maxime Ripard wrote: > Hi, > > Here is a "small" series that mostly cleans up the ov5640 driver code, > slowly getting rid of the big data array for more understandable code > (hopefully). > > The biggest addition would be the clock rate computation at runtime, > instead of relying on those arrays to setup the clock tree > properly. As a side effect, it fixes the framerate that was off by > around 10% on the smaller resolutions, and we now support 60fps. > > This also introduces a bunch of new features. > > Let me know what you think, > Maxime > > Changes from v2: >- Rebased on latest Sakari PR >- Fixed the issues reported by Hugues: improper FPS returned for > formats, improper rounding of the FPS, some with his suggestions, > some by simplifying the logic. >- Expanded the clock tree comments based on the feedback from Samuel > Bobrowicz and Loic Poulain >- Merged some of the changes made by Samuel Bobrowicz to fix the > MIPI rate computation, fix the call sites of the > ov5640_set_timings function, the auto-exposure calculation call, > etc. >- Split the patches into smaller ones in order to make it more > readable (hopefully) > > Changes from v1: >- Integrated Hugues' suggestions to fix v4l2-compliance >- Fixed the bus width with JPEG >- Dropped the clock rate calculation loops for something simpler as > suggested by Sakari >- Cache the exposure value instead of using the control value >- Rebased on top of 4.17 > > Maxime Ripard (11): >media: ov5640: Adjust the clock based on the expected rate >media: ov5640: Remove the clocks registers initialization >media: ov5640: Remove redundant defines >media: ov5640: Remove redundant register setup >media: ov5640: Compute the clock rate at runtime >media: ov5640: Remove pixel clock rates >media: ov5640: Enhance FPS handling >media: ov5640: Make the return rate type more explicit >media: ov5640: Make the FPS clamping / rounding more extendable >media: ov5640: Add 60 fps support >media: ov5640: Remove duplicate auto-exposure setup > > Samuel Bobrowicz (1): >media: ov5640: Fix timings setup code > > drivers/media/i2c/ov5640.c | 701 + > 1 file changed, 392 insertions(+), 309 deletions(-) >
Re: [PATCH 3/4] media: dt-bindings: media: Document pclk-max-frequency property
Hi! On Thu, Sep 27, 2018 at 04:46:06PM +0200, Hugues Fruchet wrote: > This optional property aims to inform parallel video devices > of the maximum pixel clock frequency admissible by host video > interface. If bandwidth of data to be transferred requires a > pixel clock which is higher than this value, parallel video > device could then typically adapt framerate to reach > this constraint. > > Signed-off-by: Hugues Fruchet > --- > Documentation/devicetree/bindings/media/video-interfaces.txt | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt > b/Documentation/devicetree/bindings/media/video-interfaces.txt > index baf9d97..fa4c112 100644 > --- a/Documentation/devicetree/bindings/media/video-interfaces.txt > +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt > @@ -147,6 +147,8 @@ Optional endpoint properties >as 0 (normal). This property is valid for serial busses only. > - strobe: Whether the clock signal is used as clock (0) or strobe (1). Used >with CCP2, for instance. > +- pclk-max-frequency: maximum pixel clock frequency admissible by video > + host interface. That seems to be a property of the capture device, not the camera itself. Can't that be negotiated through the media API? Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
Re: [PATCH v4 3/7] [media] ad5820: DT new optional field enable-gpios
On Thu, Sep 20, 2018 at 10:47:47PM +0200, Ricardo Ribalda Delgado wrote: > Document new enable-gpio field. It can be used to disable the part enable-gpios > without turning down its regulator. > > Cc: devicet...@vger.kernel.org > Signed-off-by: Ricardo Ribalda Delgado > Acked-by: Pavel Machek > --- > Documentation/devicetree/bindings/media/i2c/ad5820.txt | 7 +++ > 1 file changed, 7 insertions(+) > > diff --git a/Documentation/devicetree/bindings/media/i2c/ad5820.txt > b/Documentation/devicetree/bindings/media/i2c/ad5820.txt > index 5940ca11c021..9ccd96d3d5f0 100644 > --- a/Documentation/devicetree/bindings/media/i2c/ad5820.txt > +++ b/Documentation/devicetree/bindings/media/i2c/ad5820.txt > @@ -8,6 +8,12 @@ Required Properties: > >- VANA-supply: supply of voltage for VANA pin > > +Optional properties: > + > + - enable-gpios : GPIO spec for the XSHUTDOWN pin. Note that the polarity > of > +the enable GPIO is the opposite of the XSHUTDOWN pin (asserting the enable > +GPIO deasserts the XSHUTDOWN signal and vice versa). shutdown-gpios is also standard and seems like it would make more sense here. Yes, it is a bit redundant to have both, but things just evolved that way and we don't want to totally abandon the hardware names (just all the variants). Rob
Re: [PATCH v4 7/7] [media] ad5820: DT new compatible devices
On Thu, 20 Sep 2018 22:47:51 +0200, Ricardo Ribalda Delgado wrote: > Document new compatible devices. > > Cc: devicet...@vger.kernel.org > Signed-off-by: Ricardo Ribalda Delgado > --- > Documentation/devicetree/bindings/media/i2c/ad5820.txt | 5 - > 1 file changed, 4 insertions(+), 1 deletion(-) > Reviewed-by: Rob Herring
Re: [PATCH v4 4/7] [media] ad5820: Add support for of-autoload
Hi Ricardo,
On Thu, Sep 20, 2018 at 10:47:48PM +0200, Ricardo Ribalda Delgado wrote:
> Since kernel 4.16, i2c devices with DT compatible tag are modprobed
> using their DT modalias.
> Without this patch, if this driver is build as module it would never
> be autoprobed.
>
> Signed-off-by: Ricardo Ribalda Delgado
> Acked-by: Pavel Machek
> ---
> drivers/media/i2c/ad5820.c | 9 +
> 1 file changed, 9 insertions(+)
>
> diff --git a/drivers/media/i2c/ad5820.c b/drivers/media/i2c/ad5820.c
> index 625867472929..e461d36201a4 100644
> --- a/drivers/media/i2c/ad5820.c
> +++ b/drivers/media/i2c/ad5820.c
> @@ -372,12 +372,21 @@ static const struct i2c_device_id ad5820_id_table[] = {
> };
> MODULE_DEVICE_TABLE(i2c, ad5820_id_table);
>
> +#ifdef CONFIG_OF
> +static const struct of_device_id ad5820_of_table[] = {
> + { .compatible = "adi,ad5820" },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, ad5820_of_table);
> +#endif
> +
> static SIMPLE_DEV_PM_OPS(ad5820_pm, ad5820_suspend, ad5820_resume);
>
> static struct i2c_driver ad5820_i2c_driver = {
> .driver = {
> .name = AD5820_NAME,
> .pm = &ad5820_pm,
> + .of_match_table = of_match_ptr(ad5820_of_table),
No need to use of_match_ptr() or #ifdef above --- not doing so makes this
work on ACPI, too.
> },
> .probe = ad5820_probe,
> .remove = ad5820_remove,
--
Regards,
Sakari Ailus
e-mail: sakari.ai...@iki.fi
Re: [PATCH v4 5/7] [media] ad5820: Add support for acpi autoload
Hi Ricardo,
On Thu, Sep 20, 2018 at 10:47:49PM +0200, Ricardo Ribalda Delgado wrote:
> Allow module autoloading of ad5820 ACPI devices.
>
> Signed-off-by: Ricardo Ribalda Delgado
> ---
> drivers/media/i2c/ad5820.c | 11 +++
> 1 file changed, 11 insertions(+)
>
> diff --git a/drivers/media/i2c/ad5820.c b/drivers/media/i2c/ad5820.c
> index e461d36201a4..5d1185e7f78d 100644
> --- a/drivers/media/i2c/ad5820.c
> +++ b/drivers/media/i2c/ad5820.c
> @@ -22,6 +22,7 @@
> * General Public License for more details.
> */
>
> +#include
> #include
> #include
> #include
> @@ -380,6 +381,15 @@ static const struct of_device_id ad5820_of_table[] = {
> MODULE_DEVICE_TABLE(of, ad5820_of_table);
> #endif
>
> +#ifdef CONFIG_ACPI
> +static const struct acpi_device_id ad5820_acpi_ids[] = {
> + { "AD5820" },
This is not a valid ACPI _HID. Is there a need to add ACPI support for the
chip this way?
> + { }
> +};
> +
> +MODULE_DEVICE_TABLE(acpi, ad5820_acpi_ids);
> +#endif
> +
> static SIMPLE_DEV_PM_OPS(ad5820_pm, ad5820_suspend, ad5820_resume);
>
> static struct i2c_driver ad5820_i2c_driver = {
> @@ -387,6 +397,7 @@ static struct i2c_driver ad5820_i2c_driver = {
> .name = AD5820_NAME,
> .pm = &ad5820_pm,
> .of_match_table = of_match_ptr(ad5820_of_table),
> + .acpi_match_table = ACPI_PTR(ad5820_acpi_ids),
> },
> .probe = ad5820_probe,
> .remove = ad5820_remove,
--
Regards,
Sakari Ailus
e-mail: sakari.ai...@iki.fi
Re: [PATCH v4 6/7] [media] ad5820: Add support for ad5821 and ad5823
Hi Ricardo,
On Thu, Sep 20, 2018 at 10:47:50PM +0200, Ricardo Ribalda Delgado wrote:
> According to the datasheet, both AD5821 and AD5820 share a compatible
> register-set:
> http://www.analog.com/media/en/technical-documentation/data-sheets/AD5821.pdf
>
> Some camera modules also refer that AD5823 is a replacement of AD5820:
> https://download.kamami.com/p564094-OV8865_DS.pdf
A silly question --- the maximum current of these devices differs from each
other. Is the control value range still the same?
>
> Suggested-by: Pavel Machek
> Signed-off-by: Ricardo Ribalda Delgado
> ---
> drivers/media/i2c/ad5820.c | 12
> 1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/i2c/ad5820.c b/drivers/media/i2c/ad5820.c
> index 5d1185e7f78d..c52af302d516 100644
> --- a/drivers/media/i2c/ad5820.c
> +++ b/drivers/media/i2c/ad5820.c
> @@ -34,8 +34,6 @@
> #include
> #include
>
> -#define AD5820_NAME "ad5820"
> -
> /* Register definitions */
> #define AD5820_POWER_DOWN(1 << 15)
> #define AD5820_DAC_SHIFT 4
> @@ -368,7 +366,9 @@ static int ad5820_remove(struct i2c_client *client)
> }
>
> static const struct i2c_device_id ad5820_id_table[] = {
> - { AD5820_NAME, 0 },
> + { "ad5820", 0 },
> + { "ad5821", 0 },
> + { "ad5823", 0 },
> { }
> };
> MODULE_DEVICE_TABLE(i2c, ad5820_id_table);
> @@ -376,6 +376,8 @@ MODULE_DEVICE_TABLE(i2c, ad5820_id_table);
> #ifdef CONFIG_OF
> static const struct of_device_id ad5820_of_table[] = {
> { .compatible = "adi,ad5820" },
> + { .compatible = "adi,ad5821" },
> + { .compatible = "adi,ad5823" },
You could set the subdev name accordingly as well.
> { }
> };
> MODULE_DEVICE_TABLE(of, ad5820_of_table);
> @@ -384,6 +386,8 @@ MODULE_DEVICE_TABLE(of, ad5820_of_table);
> #ifdef CONFIG_ACPI
> static const struct acpi_device_id ad5820_acpi_ids[] = {
> { "AD5820" },
> + { "AD5821" },
> + { "AD5823" },
> { }
> };
>
> @@ -394,7 +398,7 @@ static SIMPLE_DEV_PM_OPS(ad5820_pm, ad5820_suspend,
> ad5820_resume);
>
> static struct i2c_driver ad5820_i2c_driver = {
> .driver = {
> - .name = AD5820_NAME,
> + .name = "ad5820",
> .pm = &ad5820_pm,
> .of_match_table = of_match_ptr(ad5820_of_table),
> .acpi_match_table = ACPI_PTR(ad5820_acpi_ids),
--
Regards,
Sakari Ailus
e-mail: sakari.ai...@iki.fi
Re: [PATCH v3 1/2] dt-bindings: media: Add Aspeed Video Engine binding documentation
On Tue, 25 Sep 2018 14:27:08 -0500, Eddie James wrote: > Document the bindings. > > Signed-off-by: Eddie James > --- > .../devicetree/bindings/media/aspeed-video.txt | 26 > ++ > 1 file changed, 26 insertions(+) > create mode 100644 Documentation/devicetree/bindings/media/aspeed-video.txt > Reviewed-by: Rob Herring
Dear Friend i need your help
-- Dear Assalamu Alaikum, I came across your contact during my private search Mrs Aisha Al-Qaddafi is my name, the only daughter of late Libyan president, I have funds the sum of $27.5 million USD for investment, I am interested in you for investment project assistance in your country, i shall compensate you 30% of the total sum after the funds are transfer into your account, Greetings from Mrs Aisha Al-Qaddafi Mrs Aisha Al-Qaddafi --
cron job: media_tree daily build: OK
This message is generated daily by a cron job that builds media_tree for the kernels and architectures in the list below. Results of the daily build of media_tree: date: Fri Sep 28 05:00:11 CEST 2018 media-tree git hash:4158757395b300b6eb308fc20b96d1d231484413 media_build git hash: 44385b9c61ecc27059a651885895c8ea09cd4179 v4l-utils git hash: 3874aa8eb1ff0c2e103d024ba5af915b1b26f098 edid-decode git hash: 5eeb151a748788666534d6ea3da07f90400d24c2 gcc version:i686-linux-gcc (GCC) 8.1.0 sparse version: 0.5.2 smatch version: 0.5.1 host hardware: x86_64 host os:4.18.10-marune linux-git-arm-at91: OK linux-git-arm-davinci: OK linux-git-arm-multi: OK linux-git-arm-pxa: OK linux-git-arm-stm32: OK linux-git-arm64: OK linux-git-i686: OK linux-git-mips: OK linux-git-powerpc64: OK linux-git-sh: OK linux-git-x86_64: OK Check COMPILE_TEST: OK linux-3.10.108-i686: OK linux-3.10.108-x86_64: OK linux-3.11.10-i686: OK linux-3.11.10-x86_64: OK linux-3.12.74-i686: OK linux-3.12.74-x86_64: OK linux-3.13.11-i686: OK linux-3.13.11-x86_64: OK linux-3.14.79-i686: OK linux-3.14.79-x86_64: OK linux-3.15.10-i686: OK linux-3.15.10-x86_64: OK linux-3.16.57-i686: OK linux-3.16.57-x86_64: OK linux-3.17.8-i686: OK linux-3.17.8-x86_64: OK linux-3.18.123-i686: OK linux-3.18.123-x86_64: OK linux-3.19.8-i686: OK linux-3.19.8-x86_64: OK linux-4.0.9-i686: OK linux-4.0.9-x86_64: OK linux-4.1.52-i686: OK linux-4.1.52-x86_64: OK linux-4.2.8-i686: OK linux-4.2.8-x86_64: OK linux-4.3.6-i686: OK linux-4.3.6-x86_64: OK linux-4.4.158-i686: OK linux-4.4.158-x86_64: OK linux-4.5.7-i686: OK linux-4.5.7-x86_64: OK linux-4.6.7-i686: OK linux-4.6.7-x86_64: OK linux-4.7.10-i686: OK linux-4.7.10-x86_64: OK linux-4.8.17-i686: OK linux-4.8.17-x86_64: OK linux-4.9.129-i686: OK linux-4.9.129-x86_64: OK linux-4.10.17-i686: OK linux-4.10.17-x86_64: OK linux-4.11.12-i686: OK linux-4.11.12-x86_64: OK linux-4.12.14-i686: OK linux-4.12.14-x86_64: OK linux-4.13.16-i686: OK linux-4.13.16-x86_64: OK linux-4.14.72-i686: OK linux-4.14.72-x86_64: OK linux-4.15.18-i686: OK linux-4.15.18-x86_64: OK linux-4.16.18-i686: OK linux-4.16.18-x86_64: OK linux-4.17.19-i686: OK linux-4.17.19-x86_64: OK linux-4.18.10-i686: OK linux-4.18.10-x86_64: OK linux-4.19-rc5-i686: OK linux-4.19-rc5-x86_64: OK apps: OK spec-git: OK sparse: WARNINGS Detailed results are available here: http://www.xs4all.nl/~hverkuil/logs/Friday.log Full logs are available here: http://www.xs4all.nl/~hverkuil/logs/Friday.tar.bz2 The Media Infrastructure API from this daily build is here: http://www.xs4all.nl/~hverkuil/spec/index.html
