Re: [PATCH v3 1/4] dt-bindings: adv748x: make data-lanes property mandatory for CSI-2 endpoints
Hi Niklas, On Fri, Nov 02, 2018 at 05:00:06PM +0100, Niklas Söderlund wrote: > The CSI-2 transmitters can use a different number of lanes to transmit > data. Make the data-lanes mandatory for the endpoints that describe the > transmitters as no good default can be set to fallback on. > > Signed-off-by: Niklas Söderlund > > --- > * Changes since v2 > - Update paragraph according to Laurents comments. > --- > Documentation/devicetree/bindings/media/i2c/adv748x.txt | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/media/i2c/adv748x.txt > b/Documentation/devicetree/bindings/media/i2c/adv748x.txt > index 5dddc95f9cc46084..bffbabc879efd86c 100644 > --- a/Documentation/devicetree/bindings/media/i2c/adv748x.txt > +++ b/Documentation/devicetree/bindings/media/i2c/adv748x.txt > @@ -48,7 +48,9 @@ are numbered as follows. > TXA source 10 > TXB source 11 > > -The digital output port nodes must contain at least one endpoint. > +The digital output port nodes, when present, shall contain at least one > +endpoint. Each of those endpoints shall contain the data-lanes property as > +described in video-interfaces.txt. > > Ports are optional if they are not connected to anything at the hardware > level. > Re-vamping my ignored comment on v2, I still think you should list here the accepted values for each TX as they're actually a property of the hw device itself. Required endpoint properties: - data-lanes: See "video-interfaces.txt" for description. The property is mandatory for CSI-2 output endpoints and the accepted value depends on which endpoint the property is applied to: - TXA: accepted values are <1>, <2>, <4> - TXB: accepted value is <1> > -- > 2.19.1 > signature.asc Description: PGP signature
Re: [PATCH v3 4/4] i2c: adv748x: configure number of lanes used for TXA CSI-2 transmitter
Hi Niklas,
On Fri, Nov 02, 2018 at 05:00:09PM +0100, Niklas Söderlund wrote:
> The driver fixed the TXA CSI-2 transmitter in 4-lane mode while it could
> operate using 1-, 2- and 4-lanes. Update the driver to support all modes
> the hardware does.
>
> The driver make use of large tables of static register/value writes when
> powering up/down the TXA and TXB transmitters which include the write to
> the NUM_LANES register. By converting the tables into functions and
> using parameters the power up/down functions for TXA and TXB power
> up/down can be merged and used for both transmitters.
>
> Signed-off-by: Niklas Söderlund
>
> ---
> * Changes since v2
> - Fix typos in comments.
> - Remove unneeded boiler plait code in adv748x_tx_power() as suggested
> by Jacopo and Laurent.
> - Take into account the two different register used when powering up TXA
> and TXB due to an earlier patch in this series aligns the power
> sequence with the manual.
>
> * Changes since v1
> - Convert tables of register/value writes into functions instead of
> intercepting and modifying the writes to the NUM_LANES register.
> ---
> drivers/media/i2c/adv748x/adv748x-core.c | 157 ---
> 1 file changed, 79 insertions(+), 78 deletions(-)
>
> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c
> b/drivers/media/i2c/adv748x/adv748x-core.c
> index 9d80d7f3062b16bc..d94c63cb6a2efdba 100644
> --- a/drivers/media/i2c/adv748x/adv748x-core.c
> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> @@ -125,6 +125,16 @@ int adv748x_write(struct adv748x_state *state, u8 page,
> u8 reg, u8 value)
> return regmap_write(state->regmap[page], reg, value);
> }
>
> +static int adv748x_write_check(struct adv748x_state *state, u8 page, u8 reg,
> +u8 value, int *error)
> +{
> + if (*error)
> + return *error;
> +
> + *error = adv748x_write(state, page, reg, value);
> + return *error;
> +}
> +
> /* adv748x_write_block(): Write raw data with a maximum of
> I2C_SMBUS_BLOCK_MAX
> * size to one or more registers.
> *
> @@ -231,79 +241,77 @@ static int adv748x_write_regs(struct adv748x_state
> *state,
> * TXA and TXB
> */
>
> -static const struct adv748x_reg_value adv748x_power_up_txa_4lane[] = {
> -
> - {ADV748X_PAGE_TXA, 0x00, 0x84}, /* Enable 4-lane MIPI */
> - {ADV748X_PAGE_TXA, 0x00, 0xa4}, /* Set Auto DPHY Timing */
> - {ADV748X_PAGE_TXA, 0xdb, 0x10}, /* ADI Required Write */
> - {ADV748X_PAGE_TXA, 0xd6, 0x07}, /* ADI Required Write */
> - {ADV748X_PAGE_TXA, 0xc4, 0x0a}, /* ADI Required Write */
> - {ADV748X_PAGE_TXA, 0x71, 0x33}, /* ADI Required Write */
> - {ADV748X_PAGE_TXA, 0x72, 0x11}, /* ADI Required Write */
> - {ADV748X_PAGE_TXA, 0xf0, 0x00}, /* i2c_dphy_pwdn - 1'b0 */
> -
> - {ADV748X_PAGE_TXA, 0x31, 0x82}, /* ADI Required Write */
> - {ADV748X_PAGE_TXA, 0x1e, 0x40}, /* ADI Required Write */
> - {ADV748X_PAGE_TXA, 0xda, 0x01}, /* i2c_mipi_pll_en - 1'b1 */
> - {ADV748X_PAGE_WAIT, 0x00, 0x02},/* delay 2 */
> - {ADV748X_PAGE_TXA, 0x00, 0x24 },/* Power-up CSI-TX */
> - {ADV748X_PAGE_WAIT, 0x00, 0x01},/* delay 1 */
> - {ADV748X_PAGE_TXA, 0xc1, 0x2b}, /* ADI Required Write */
> - {ADV748X_PAGE_WAIT, 0x00, 0x01},/* delay 1 */
> - {ADV748X_PAGE_TXA, 0x31, 0x80}, /* ADI Required Write */
> -
> - {ADV748X_PAGE_EOR, 0xff, 0xff} /* End of register table */
> -};
> -
> -static const struct adv748x_reg_value adv748x_power_down_txa_4lane[] = {
> -
> - {ADV748X_PAGE_TXA, 0x31, 0x82}, /* ADI Required Write */
> - {ADV748X_PAGE_TXA, 0x1e, 0x00}, /* ADI Required Write */
> - {ADV748X_PAGE_TXA, 0x00, 0x84}, /* Enable 4-lane MIPI */
> - {ADV748X_PAGE_TXA, 0xda, 0x01}, /* i2c_mipi_pll_en - 1'b1 */
> - {ADV748X_PAGE_TXA, 0xc1, 0x3b}, /* ADI Required Write */
> -
> - {ADV748X_PAGE_EOR, 0xff, 0xff} /* End of register table */
> -};
> -
> -static const struct adv748x_reg_value adv748x_power_up_txb_1lane[] = {
> -
> - {ADV748X_PAGE_TXB, 0x00, 0x81}, /* Enable 1-lane MIPI */
> - {ADV748X_PAGE_TXB, 0x00, 0xa1}, /* Set Auto DPHY Timing */
> - {ADV748X_PAGE_TXB, 0xd2, 0x40}, /* ADI Required Write */
> - {ADV748X_PAGE_TXB, 0xc4, 0x0a}, /* ADI Required Write */
> - {ADV748X_PAGE_TXB, 0x71, 0x33}, /* ADI Required Write */
> - {ADV748X_PAGE_TXB, 0x72, 0x11}, /* ADI Required Write */
> - {ADV748X_PAGE_TXB, 0xf0, 0x00}, /* i2c_dphy_pwdn - 1'b0 */
> -
> - {ADV748X_PAGE_TXB, 0x31, 0x82}, /* ADI Required Write */
> - {ADV748X_PAGE_TXB, 0x1e, 0x40}, /* ADI Required Write */
> - {ADV748X_PAGE_TXB, 0xda, 0x01}, /* i2c_mipi_pll_en - 1'b1 */
> - {ADV748X_PAGE_WAIT, 0x00, 0x02},/* delay 2 */
> - {ADV748X_PAGE_TXB, 0x00, 0x21 },/* Power-up CSI-TX */
> - {ADV748X_PAGE_WAIT, 0x00, 0x01},/* delay 1 */
> - {ADV748X_PAGE_TXB, 0xc1, 0x2b}, /* ADI Required Write */
> - {ADV748X_PAGE_WAIT, 0x00, 0x01},/* delay 1 */
> - {ADV748X_PAGE_TXB, 0x31, 0x80
Re: [PATCH v3 3/4] i2c: adv748x: store number of CSI-2 lanes described in device tree
Hi Niklas,
thanks for the patches
On Fri, Nov 02, 2018 at 05:00:08PM +0100, Niklas Söderlund wrote:
> The adv748x CSI-2 transmitters TXA and TXB can use different number of
> lanes to transmit data. In order to be able to configure the device
> correctly this information need to be parsed from device tree and stored
> in each TX private data structure.
>
> TXA supports 1, 2 and 4 lanes while TXB supports 1 lane.
>
> Signed-off-by: Niklas Söderlund
Reviewed-by: Jacopo Mondi
Thanks
j
>
> ---
> * Changes since v2
> - Rebase to latest media-tree requires the bus_type filed in struct
> v4l2_fwnode_endpoint to be initialized, set it to V4L2_MBUS_CSI2_DPHY.
>
> * Changes since v1
> - Use %u instead of %d to print unsigned int.
> - Fix spelling in commit message, thanks Laurent.
> ---
> drivers/media/i2c/adv748x/adv748x-core.c | 50
> drivers/media/i2c/adv748x/adv748x.h | 1 +
> 2 files changed, 51 insertions(+)
>
> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c
> b/drivers/media/i2c/adv748x/adv748x-core.c
> index 2384f50dacb0ccff..9d80d7f3062b16bc 100644
> --- a/drivers/media/i2c/adv748x/adv748x-core.c
> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> @@ -23,6 +23,7 @@
> #include
> #include
> #include
> +#include
> #include
>
> #include "adv748x.h"
> @@ -521,12 +522,56 @@ void adv748x_subdev_init(struct v4l2_subdev *sd, struct
> adv748x_state *state,
> sd->entity.ops = &adv748x_media_ops;
> }
>
> +static int adv748x_parse_csi2_lanes(struct adv748x_state *state,
> + unsigned int port,
> + struct device_node *ep)
> +{
> + struct v4l2_fwnode_endpoint vep;
> + unsigned int num_lanes;
> + int ret;
> +
> + if (port != ADV748X_PORT_TXA && port != ADV748X_PORT_TXB)
> + return 0;
> +
> + vep.bus_type = V4L2_MBUS_CSI2_DPHY;
> + ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep), &vep);
> + if (ret)
> + return ret;
> +
> + num_lanes = vep.bus.mipi_csi2.num_data_lanes;
> +
> + if (vep.base.port == ADV748X_PORT_TXA) {
> + if (num_lanes != 1 && num_lanes != 2 && num_lanes != 4) {
> + adv_err(state, "TXA: Invalid number (%u) of lanes\n",
> + num_lanes);
> + return -EINVAL;
> + }
> +
> + state->txa.num_lanes = num_lanes;
> + adv_dbg(state, "TXA: using %u lanes\n", state->txa.num_lanes);
> + }
> +
> + if (vep.base.port == ADV748X_PORT_TXB) {
> + if (num_lanes != 1) {
> + adv_err(state, "TXB: Invalid number (%u) of lanes\n",
> + num_lanes);
> + return -EINVAL;
> + }
> +
> + state->txb.num_lanes = num_lanes;
> + adv_dbg(state, "TXB: using %u lanes\n", state->txb.num_lanes);
> + }
> +
> + return 0;
> +}
> +
> static int adv748x_parse_dt(struct adv748x_state *state)
> {
> struct device_node *ep_np = NULL;
> struct of_endpoint ep;
> bool out_found = false;
> bool in_found = false;
> + int ret;
>
> for_each_endpoint_of_node(state->dev->of_node, ep_np) {
> of_graph_parse_endpoint(ep_np, &ep);
> @@ -557,6 +602,11 @@ static int adv748x_parse_dt(struct adv748x_state *state)
> in_found = true;
> else
> out_found = true;
> +
> + /* Store number of CSI-2 lanes used for TXA and TXB. */
> + ret = adv748x_parse_csi2_lanes(state, ep.port, ep_np);
> + if (ret)
> + return ret;
> }
>
> return in_found && out_found ? 0 : -ENODEV;
> diff --git a/drivers/media/i2c/adv748x/adv748x.h
> b/drivers/media/i2c/adv748x/adv748x.h
> index 39c2fdc3b41667d8..b482c7fe6957eb85 100644
> --- a/drivers/media/i2c/adv748x/adv748x.h
> +++ b/drivers/media/i2c/adv748x/adv748x.h
> @@ -79,6 +79,7 @@ struct adv748x_csi2 {
> struct v4l2_mbus_framefmt format;
> unsigned int page;
> unsigned int port;
> + unsigned int num_lanes;
>
> struct media_pad pads[ADV748X_CSI2_NR_PADS];
> struct v4l2_ctrl_handler ctrl_hdl;
> --
> 2.19.1
>
signature.asc
Description: PGP signature
Re: [PATCH 05/11] [media] marvell-ccic: don't generate EOF on parallel bus
On Mon 2018-11-05 08:30:48, Lubomir Rintel wrote:
> The commit 05fed81625bf ("[media] marvell-ccic: add MIPI support for
> marvell-ccic driver") that claimed to add CSI2 turned on C0_EOF_VSYNC for
> parallel bus without a very good explanation.
>
> That broke camera on OLPC XO-1.75 which precisely uses a sensor on a
> parallel bus. Revert that chunk.
>
> Tested on an OLPC XO-1.75.
>
> Fixes: 05fed81625bf755cc67c5864cdfd18b69ea828d1
> Signed-off-by: Lubomir Rintel
You should really Cc original author and people that signed off on
that patch.
Pavel
> drivers/media/platform/marvell-ccic/mcam-core.c | 6 --
> 1 file changed, 6 deletions(-)
>
> diff --git a/drivers/media/platform/marvell-ccic/mcam-core.c
> b/drivers/media/platform/marvell-ccic/mcam-core.c
> index d97f39bde9bd..d24e5b7a3bc5 100644
> --- a/drivers/media/platform/marvell-ccic/mcam-core.c
> +++ b/drivers/media/platform/marvell-ccic/mcam-core.c
> @@ -792,12 +792,6 @@ static void mcam_ctlr_image(struct mcam_camera *cam)
>* Make sure it knows we want to use hsync/vsync.
>*/
> mcam_reg_write_mask(cam, REG_CTRL0, C0_SIF_HVSYNC, C0_SIFM_MASK);
> - /*
> - * This field controls the generation of EOF(DVP only)
> - */
> - if (cam->bus_type != V4L2_MBUS_CSI2_DPHY)
> - mcam_reg_set_bit(cam, REG_CTRL0,
> - C0_EOF_VSYNC | C0_VEDGE_CTRL);
> }
>
>
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures)
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
signature.asc
Description: Digital signature
Re: [PATCH 06/11] [media] marvell-ccic: drop ctlr_reset()
On Mon 2018-11-05 08:30:49, Lubomir Rintel wrote:
> This accesses the clock registers directly and thus is not too
> devicetree friendly. If it's actually needed it needs to be done
> differently.
Device-tree unfriendly is bad, but you still don't want to cause
regression to people needing this.
(Perhaps noone uses this, but...)
> Signed-off-by: Lubomir Rintel
Git blame says: 7c269f454 . Perhaps its authors should be Cced here,
too?
Pavel
> ---
> .../media/platform/marvell-ccic/mcam-core.c | 6 -
> .../media/platform/marvell-ccic/mcam-core.h | 1 -
> .../media/platform/marvell-ccic/mmp-driver.c | 23 ---
> 3 files changed, 30 deletions(-)
>
> diff --git a/drivers/media/platform/marvell-ccic/mcam-core.c
> b/drivers/media/platform/marvell-ccic/mcam-core.c
> index d24e5b7a3bc5..1b879035948c 100644
> --- a/drivers/media/platform/marvell-ccic/mcam-core.c
> +++ b/drivers/media/platform/marvell-ccic/mcam-core.c
> @@ -1154,12 +1154,6 @@ static void mcam_vb_stop_streaming(struct vb2_queue
> *vq)
> if (cam->state != S_STREAMING)
> return;
> mcam_ctlr_stop_dma(cam);
> - /*
> - * Reset the CCIC PHY after stopping streaming,
> - * otherwise, the CCIC may be unstable.
> - */
> - if (cam->ctlr_reset)
> - cam->ctlr_reset(cam);
> /*
>* VB2 reclaims the buffers, so we need to forget
>* about them.
> diff --git a/drivers/media/platform/marvell-ccic/mcam-core.h
> b/drivers/media/platform/marvell-ccic/mcam-core.h
> index ad8955f9f0a1..f93f23faf364 100644
> --- a/drivers/media/platform/marvell-ccic/mcam-core.h
> +++ b/drivers/media/platform/marvell-ccic/mcam-core.h
> @@ -137,7 +137,6 @@ struct mcam_camera {
> int (*plat_power_up) (struct mcam_camera *cam);
> void (*plat_power_down) (struct mcam_camera *cam);
> void (*calc_dphy) (struct mcam_camera *cam);
> - void (*ctlr_reset) (struct mcam_camera *cam);
>
> /*
>* Everything below here is private to the mcam core and
> diff --git a/drivers/media/platform/marvell-ccic/mmp-driver.c
> b/drivers/media/platform/marvell-ccic/mmp-driver.c
> index 70a2833db0d1..92a79ad8a12c 100644
> --- a/drivers/media/platform/marvell-ccic/mmp-driver.c
> +++ b/drivers/media/platform/marvell-ccic/mmp-driver.c
> @@ -183,28 +183,6 @@ static void mmpcam_power_down(struct mcam_camera *mcam)
> mcam_clk_disable(mcam);
> }
>
> -static void mcam_ctlr_reset(struct mcam_camera *mcam)
> -{
> - unsigned long val;
> - struct mmp_camera *cam = mcam_to_cam(mcam);
> -
> - if (mcam->ccic_id) {
> - /*
> - * Using CCIC2
> - */
> - val = ioread32(cam->power_regs + REG_CCIC2_CRCR);
> - iowrite32(val & ~0x2, cam->power_regs + REG_CCIC2_CRCR);
> - iowrite32(val | 0x2, cam->power_regs + REG_CCIC2_CRCR);
> - } else {
> - /*
> - * Using CCIC1
> - */
> - val = ioread32(cam->power_regs + REG_CCIC_CRCR);
> - iowrite32(val & ~0x2, cam->power_regs + REG_CCIC_CRCR);
> - iowrite32(val | 0x2, cam->power_regs + REG_CCIC_CRCR);
> - }
> -}
> -
> /*
> * calc the dphy register values
> * There are three dphy registers being used.
> @@ -352,7 +330,6 @@ static int mmpcam_probe(struct platform_device *pdev)
> mcam = &cam->mcam;
> mcam->plat_power_up = mmpcam_power_up;
> mcam->plat_power_down = mmpcam_power_down;
> - mcam->ctlr_reset = mcam_ctlr_reset;
> mcam->calc_dphy = mmpcam_calc_dphy;
> mcam->dev = &pdev->dev;
> mcam->use_smbus = 0;
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures)
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
signature.asc
Description: Digital signature
Re: [PATCH 08/11] [media] marvell-ccic/mmp: enable clock before accessing registers
On Mon 2018-11-05 08:30:51, Lubomir Rintel wrote: > The access to REG_CLKCTRL or REG_CTRL1 without the clock enabled hangs > the machine. Enable the clock first. > > Signed-off-by: Lubomir Rintel Acked-by: Pavel Machek > --- > drivers/media/platform/marvell-ccic/mmp-driver.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/media/platform/marvell-ccic/mmp-driver.c > b/drivers/media/platform/marvell-ccic/mmp-driver.c > index 9e988e527b0d..9c0238f72c40 100644 > --- a/drivers/media/platform/marvell-ccic/mmp-driver.c > +++ b/drivers/media/platform/marvell-ccic/mmp-driver.c > @@ -145,6 +145,7 @@ static int mmpcam_power_up(struct mcam_camera *mcam) > * Turn on power and clocks to the controller. > */ > mmpcam_power_up_ctlr(cam); > + mcam_clk_enable(mcam); > /* > * Provide power to the sensor. > */ > @@ -158,8 +159,6 @@ static int mmpcam_power_up(struct mcam_camera *mcam) > gpio_set_value(pdata->sensor_reset_gpio, 1); /* reset is active low */ > mdelay(5); > > - mcam_clk_enable(mcam); > - > return 0; > } > -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature
Re: [PATCH 07/11] [media] marvell-ccic: drop unused stuff
On Mon 2018-11-05 08:30:50, Lubomir Rintel wrote: > Remove structure members and headers that are not actually used. Saves > us from some noise in subsequent cleanup commits. > > Signed-off-by: Lubomir Rintel Acked-by: Pavel Machek -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature
Re: [PATCH 09/11] [media] marvell-ccic/mmp: add devicetree support
On Mon 2018-11-05 08:30:52, Lubomir Rintel wrote:
> The platform data is actually not used anywhere (along with the CSI
> support) and should be safe to remove.
>
> Signed-off-by: Lubomir Rintel
Acked-by: Pavel Machek
> + } else {
> + /*
> + * This are values that used to be hardcoded in mcam-core and
"These are values"? "and it"?
> + * work well on a OLPC XO 1.75 with a parallel bus sensor.
> + * If it turns out other setups make sense, the values should
> + * be obtained from the device tree.
> + */
> + mcam->mclk_src = 3;
> + mcam->mclk_div = 2;
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures)
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
signature.asc
Description: Digital signature
Re: [PATCH v3 0/4] i2c: adv748x: add support for CSI-2 TXA to work
Hi Niklas, On Fri, Nov 02, 2018 at 05:00:05PM +0100, Niklas Söderlund wrote: > Hi, > > This series allows the TXA CSI-2 transmitter of the adv748x to function > in 1-, 2- and 4- lane mode. Currently the driver fixes the hardware in > 4-lane mode. The driver looks at the standard DT property 'data-lanes' > to determine which mode it should operate in. > > Patch 1/4 lists the 'data-lanes' DT property as mandatory for endpoints > describing the CSI-2 transmitters. Patch 2/4 refactors the > initialization sequence of the adv748x to be able to reuse more code. > Patch 3/4 adds the DT parsing and storing of the number of lanes. Patch > 4/4 merges the TXA and TXB power up/down procedure while also taking the > configurable number of lanes into account. > > The series is based on the latest media-tree master and is tested on > Renesas M3-N in 1-, 2- and 4- lane mode. I have now tested v3 on Ebisu E3 which has only 2 data lanes connected. Tested-by: Jacopo Mondi Thanks j > > Niklas Söderlund (4): > dt-bindings: adv748x: make data-lanes property mandatory for CSI-2 > endpoints > i2c: adv748x: reuse power up sequence when initializing CSI-2 > i2c: adv748x: store number of CSI-2 lanes described in device tree > i2c: adv748x: configure number of lanes used for TXA CSI-2 transmitter > > .../devicetree/bindings/media/i2c/adv748x.txt | 4 +- > drivers/media/i2c/adv748x/adv748x-core.c | 235 ++ > drivers/media/i2c/adv748x/adv748x.h | 1 + > 3 files changed, 135 insertions(+), 105 deletions(-) > > -- > 2.19.1 > signature.asc Description: PGP signature
Re: [PATCH v3 1/4] dt-bindings: adv748x: make data-lanes property mandatory for CSI-2 endpoints
Hi Niklas, Thank you for the patch. On Friday, 2 November 2018 18:00:06 EET Niklas Söderlund wrote: > The CSI-2 transmitters can use a different number of lanes to transmit > data. Make the data-lanes mandatory for the endpoints that describe the > transmitters as no good default can be set to fallback on. > > Signed-off-by: Niklas Söderlund Reviewed-by: Laurent Pinchart > > --- > * Changes since v2 > - Update paragraph according to Laurents comments. > --- > Documentation/devicetree/bindings/media/i2c/adv748x.txt | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/media/i2c/adv748x.txt > b/Documentation/devicetree/bindings/media/i2c/adv748x.txt index > 5dddc95f9cc46084..bffbabc879efd86c 100644 > --- a/Documentation/devicetree/bindings/media/i2c/adv748x.txt > +++ b/Documentation/devicetree/bindings/media/i2c/adv748x.txt > @@ -48,7 +48,9 @@ are numbered as follows. > TXA source 10 > TXB source 11 > > -The digital output port nodes must contain at least one endpoint. > +The digital output port nodes, when present, shall contain at least one > +endpoint. Each of those endpoints shall contain the data-lanes property as > +described in video-interfaces.txt. > > Ports are optional if they are not connected to anything at the hardware > level. -- Regards, Laurent Pinchart
Re: [PATCH v3 2/4] i2c: adv748x: reuse power up sequence when initializing CSI-2
Hi Niklas,
Thank you for the patch.
On Friday, 2 November 2018 18:00:07 EET Niklas Söderlund wrote:
> Extend the MIPI CSI-2 power up sequence to match the power up sequence
> in the hardware manual chapter "9.5.1 Power Up Sequence". This change
> allows the power up functions to be reused when initializing the
> hardware reducing code duplicating as well aligning with the
> documentation.
>
> Signed-off-by: Niklas Söderlund
Reviewed-by: Laurent Pinchart
>
> ---
> * Changes since v2
> - Bring in the undocumented registers in the power on sequence from the
> initialization sequence after confirming in the hardware manual that
> this is the correct behavior.
> ---
> drivers/media/i2c/adv748x/adv748x-core.c | 50 ++--
> 1 file changed, 13 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c
> b/drivers/media/i2c/adv748x/adv748x-core.c index
> 6854d898fdd1f192..2384f50dacb0ccff 100644
> --- a/drivers/media/i2c/adv748x/adv748x-core.c
> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> @@ -234,6 +234,12 @@ static const struct adv748x_reg_value
> adv748x_power_up_txa_4lane[] = {
>
> {ADV748X_PAGE_TXA, 0x00, 0x84}, /* Enable 4-lane MIPI */
> {ADV748X_PAGE_TXA, 0x00, 0xa4}, /* Set Auto DPHY Timing */
> + {ADV748X_PAGE_TXA, 0xdb, 0x10}, /* ADI Required Write */
> + {ADV748X_PAGE_TXA, 0xd6, 0x07}, /* ADI Required Write */
> + {ADV748X_PAGE_TXA, 0xc4, 0x0a}, /* ADI Required Write */
> + {ADV748X_PAGE_TXA, 0x71, 0x33}, /* ADI Required Write */
> + {ADV748X_PAGE_TXA, 0x72, 0x11}, /* ADI Required Write */
> + {ADV748X_PAGE_TXA, 0xf0, 0x00}, /* i2c_dphy_pwdn - 1'b0 */
>
> {ADV748X_PAGE_TXA, 0x31, 0x82}, /* ADI Required Write */
> {ADV748X_PAGE_TXA, 0x1e, 0x40}, /* ADI Required Write */
> @@ -263,6 +269,11 @@ static const struct adv748x_reg_value
> adv748x_power_up_txb_1lane[] = {
>
> {ADV748X_PAGE_TXB, 0x00, 0x81}, /* Enable 1-lane MIPI */
> {ADV748X_PAGE_TXB, 0x00, 0xa1}, /* Set Auto DPHY Timing */
> + {ADV748X_PAGE_TXB, 0xd2, 0x40}, /* ADI Required Write */
> + {ADV748X_PAGE_TXB, 0xc4, 0x0a}, /* ADI Required Write */
> + {ADV748X_PAGE_TXB, 0x71, 0x33}, /* ADI Required Write */
> + {ADV748X_PAGE_TXB, 0x72, 0x11}, /* ADI Required Write */
> + {ADV748X_PAGE_TXB, 0xf0, 0x00}, /* i2c_dphy_pwdn - 1'b0 */
>
> {ADV748X_PAGE_TXB, 0x31, 0x82}, /* ADI Required Write */
> {ADV748X_PAGE_TXB, 0x1e, 0x40}, /* ADI Required Write */
> @@ -383,25 +394,6 @@ static const struct adv748x_reg_value
> adv748x_init_txa_4lane[] = { {ADV748X_PAGE_IO, 0x0c, 0xe0}, /* Enable
> LLC_DLL & Double LLC Timing */ {ADV748X_PAGE_IO, 0x0e, 0xdd}, /*
> LLC/PIX/SPI PINS TRISTATED AUD */
>
> - {ADV748X_PAGE_TXA, 0x00, 0x84}, /* Enable 4-lane MIPI */
> - {ADV748X_PAGE_TXA, 0x00, 0xa4}, /* Set Auto DPHY Timing */
> - {ADV748X_PAGE_TXA, 0xdb, 0x10}, /* ADI Required Write */
> - {ADV748X_PAGE_TXA, 0xd6, 0x07}, /* ADI Required Write */
> - {ADV748X_PAGE_TXA, 0xc4, 0x0a}, /* ADI Required Write */
> - {ADV748X_PAGE_TXA, 0x71, 0x33}, /* ADI Required Write */
> - {ADV748X_PAGE_TXA, 0x72, 0x11}, /* ADI Required Write */
> - {ADV748X_PAGE_TXA, 0xf0, 0x00}, /* i2c_dphy_pwdn - 1'b0 */
> -
> - {ADV748X_PAGE_TXA, 0x31, 0x82}, /* ADI Required Write */
> - {ADV748X_PAGE_TXA, 0x1e, 0x40}, /* ADI Required Write */
> - {ADV748X_PAGE_TXA, 0xda, 0x01}, /* i2c_mipi_pll_en - 1'b1 */
> - {ADV748X_PAGE_WAIT, 0x00, 0x02},/* delay 2 */
> - {ADV748X_PAGE_TXA, 0x00, 0x24 },/* Power-up CSI-TX */
> - {ADV748X_PAGE_WAIT, 0x00, 0x01},/* delay 1 */
> - {ADV748X_PAGE_TXA, 0xc1, 0x2b}, /* ADI Required Write */
> - {ADV748X_PAGE_WAIT, 0x00, 0x01},/* delay 1 */
> - {ADV748X_PAGE_TXA, 0x31, 0x80}, /* ADI Required Write */
> -
> {ADV748X_PAGE_EOR, 0xff, 0xff} /* End of register table */
> };
>
> @@ -435,24 +427,6 @@ static const struct adv748x_reg_value
> adv748x_init_txb_1lane[] = { {ADV748X_PAGE_SDP, 0x31, 0x12}, /* ADI
> Required Write */
> {ADV748X_PAGE_SDP, 0xe6, 0x4f}, /* V bit end pos manually in NTSC */
>
> - {ADV748X_PAGE_TXB, 0x00, 0x81}, /* Enable 1-lane MIPI */
> - {ADV748X_PAGE_TXB, 0x00, 0xa1}, /* Set Auto DPHY Timing */
> - {ADV748X_PAGE_TXB, 0xd2, 0x40}, /* ADI Required Write */
> - {ADV748X_PAGE_TXB, 0xc4, 0x0a}, /* ADI Required Write */
> - {ADV748X_PAGE_TXB, 0x71, 0x33}, /* ADI Required Write */
> - {ADV748X_PAGE_TXB, 0x72, 0x11}, /* ADI Required Write */
> - {ADV748X_PAGE_TXB, 0xf0, 0x00}, /* i2c_dphy_pwdn - 1'b0 */
> - {ADV748X_PAGE_TXB, 0x31, 0x82}, /* ADI Required Write */
> - {ADV748X_PAGE_TXB, 0x1e, 0x40}, /* ADI Required Write */
> - {ADV748X_PAGE_TXB, 0xda, 0x01}, /* i2c_mipi_pll_en - 1'b1 */
> -
> - {ADV748X_PAGE_WAIT, 0x00, 0x02},/* delay 2 */
> - {ADV748X_PAGE_TXB, 0x00, 0x21 },/* Power-up CSI-TX */
> - {ADV748X_PAGE_WAIT, 0x00, 0x01},/* delay 1 */
> - {ADV748X_PAGE_T
Re: [PATCH v3 3/4] i2c: adv748x: store number of CSI-2 lanes described in device tree
Hi Niklas,
Thank you for the patch.
On Friday, 2 November 2018 18:00:08 EET Niklas Söderlund wrote:
> The adv748x CSI-2 transmitters TXA and TXB can use different number of
> lanes to transmit data. In order to be able to configure the device
> correctly this information need to be parsed from device tree and stored
> in each TX private data structure.
>
> TXA supports 1, 2 and 4 lanes while TXB supports 1 lane.
>
> Signed-off-by: Niklas Söderlund
>
> ---
> * Changes since v2
> - Rebase to latest media-tree requires the bus_type filed in struct
> v4l2_fwnode_endpoint to be initialized, set it to V4L2_MBUS_CSI2_DPHY.
>
> * Changes since v1
> - Use %u instead of %d to print unsigned int.
> - Fix spelling in commit message, thanks Laurent.
> ---
> drivers/media/i2c/adv748x/adv748x-core.c | 50
> drivers/media/i2c/adv748x/adv748x.h | 1 +
> 2 files changed, 51 insertions(+)
>
> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c
> b/drivers/media/i2c/adv748x/adv748x-core.c index
> 2384f50dacb0ccff..9d80d7f3062b16bc 100644
> --- a/drivers/media/i2c/adv748x/adv748x-core.c
> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> @@ -23,6 +23,7 @@
> #include
> #include
> #include
> +#include
> #include
>
> #include "adv748x.h"
> @@ -521,12 +522,56 @@ void adv748x_subdev_init(struct v4l2_subdev *sd,
> struct adv748x_state *state, sd->entity.ops = &adv748x_media_ops;
> }
>
> +static int adv748x_parse_csi2_lanes(struct adv748x_state *state,
> + unsigned int port,
> + struct device_node *ep)
> +{
> + struct v4l2_fwnode_endpoint vep;
> + unsigned int num_lanes;
> + int ret;
> +
> + if (port != ADV748X_PORT_TXA && port != ADV748X_PORT_TXB)
> + return 0;
> +
> + vep.bus_type = V4L2_MBUS_CSI2_DPHY;
> + ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep), &vep);
> + if (ret)
> + return ret;
> +
> + num_lanes = vep.bus.mipi_csi2.num_data_lanes;
> +
> + if (vep.base.port == ADV748X_PORT_TXA) {
> + if (num_lanes != 1 && num_lanes != 2 && num_lanes != 4) {
> + adv_err(state, "TXA: Invalid number (%u) of lanes\n",
> + num_lanes);
> + return -EINVAL;
> + }
> +
> + state->txa.num_lanes = num_lanes;
> + adv_dbg(state, "TXA: using %u lanes\n", state->txa.num_lanes);
> + }
> +
> + if (vep.base.port == ADV748X_PORT_TXB) {
> + if (num_lanes != 1) {
> + adv_err(state, "TXB: Invalid number (%u) of lanes\n",
> + num_lanes);
> + return -EINVAL;
> + }
> +
> + state->txb.num_lanes = num_lanes;
> + adv_dbg(state, "TXB: using %u lanes\n", state->txb.num_lanes);
> + }
Should we set the number of lanes to 4 and 1 respectively by default to
support old DTs ? Apart from that,
Reviewed-by: Laurent Pinchart
> + return 0;
> +}
> +
> static int adv748x_parse_dt(struct adv748x_state *state)
> {
> struct device_node *ep_np = NULL;
> struct of_endpoint ep;
> bool out_found = false;
> bool in_found = false;
> + int ret;
>
> for_each_endpoint_of_node(state->dev->of_node, ep_np) {
> of_graph_parse_endpoint(ep_np, &ep);
> @@ -557,6 +602,11 @@ static int adv748x_parse_dt(struct adv748x_state
> *state) in_found = true;
> else
> out_found = true;
> +
> + /* Store number of CSI-2 lanes used for TXA and TXB. */
> + ret = adv748x_parse_csi2_lanes(state, ep.port, ep_np);
> + if (ret)
> + return ret;
> }
>
> return in_found && out_found ? 0 : -ENODEV;
> diff --git a/drivers/media/i2c/adv748x/adv748x.h
> b/drivers/media/i2c/adv748x/adv748x.h index
> 39c2fdc3b41667d8..b482c7fe6957eb85 100644
> --- a/drivers/media/i2c/adv748x/adv748x.h
> +++ b/drivers/media/i2c/adv748x/adv748x.h
> @@ -79,6 +79,7 @@ struct adv748x_csi2 {
> struct v4l2_mbus_framefmt format;
> unsigned int page;
> unsigned int port;
> + unsigned int num_lanes;
>
> struct media_pad pads[ADV748X_CSI2_NR_PADS];
> struct v4l2_ctrl_handler ctrl_hdl;
--
Regards,
Laurent Pinchart
Re: [PATCH] [media] ov7670: make "xclk" clock optional
Hi Lubomir,
+Sakari in Cc
I just noticed this, and the patch is now in v4.20, but let me comment
anyway on this.
On Thu, Oct 04, 2018 at 11:29:03PM +0200, Lubomir Rintel wrote:
> When the "xclk" clock was added, it was made mandatory. This broke the
> driver on an OLPC plaform which doesn't know such clock. Make it
> optional.
>
I don't think this is correct. The sensor needs a clock to work.
With this patch clock_speed which is used to calculate
the framerate is defaulted to 30MHz, crippling all the calculations if
that default value doesn't match what is actually installed on the
board.
If this patch breaks the OLPC, then might it be the DTS for said
device needs to be fixed instead of working around the issue here?
Also, the DT bindings should be updated too if we decide this property
can be omitted. At this point, with a follow-up patch.
Thanks
j
> Tested on a OLPC XO-1 laptop.
>
> Cc: sta...@vger.kernel.org # 4.11+
> Fixes: 0a024d634cee ("[media] ov7670: get xclk")
> Signed-off-by: Lubomir Rintel
> ---
> drivers/media/i2c/ov7670.c | 27 +--
> 1 file changed, 17 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/media/i2c/ov7670.c b/drivers/media/i2c/ov7670.c
> index 31bf577b0bd3..64d1402882c8 100644
> --- a/drivers/media/i2c/ov7670.c
> +++ b/drivers/media/i2c/ov7670.c
> @@ -1808,17 +1808,24 @@ static int ov7670_probe(struct i2c_client *client,
> info->pclk_hb_disable = true;
> }
>
> - info->clk = devm_clk_get(&client->dev, "xclk");
> - if (IS_ERR(info->clk))
> - return PTR_ERR(info->clk);
> - ret = clk_prepare_enable(info->clk);
> - if (ret)
> - return ret;
> + info->clk = devm_clk_get(&client->dev, "xclk"); /* optional */
> + if (IS_ERR(info->clk)) {
> + ret = PTR_ERR(info->clk);
> + if (ret == -ENOENT)
> + info->clk = NULL;
> + else
> + return ret;
> + }
> + if (info->clk) {
> + ret = clk_prepare_enable(info->clk);
> + if (ret)
> + return ret;
>
> - info->clock_speed = clk_get_rate(info->clk) / 100;
> - if (info->clock_speed < 10 || info->clock_speed > 48) {
> - ret = -EINVAL;
> - goto clk_disable;
> + info->clock_speed = clk_get_rate(info->clk) / 100;
> + if (info->clock_speed < 10 || info->clock_speed > 48) {
> + ret = -EINVAL;
> + goto clk_disable;
> + }
> }
>
> ret = ov7670_init_gpio(client, info);
> --
> 2.19.0
>
signature.asc
Description: PGP signature
Re: [PATCH v3 4/4] i2c: adv748x: configure number of lanes used for TXA CSI-2 transmitter
Hi Niklas,
Thank you for the patch.
On Friday, 2 November 2018 18:00:09 EET Niklas Söderlund wrote:
> The driver fixed the TXA CSI-2 transmitter in 4-lane mode while it could
> operate using 1-, 2- and 4-lanes. Update the driver to support all modes
> the hardware does.
"all modes the hardware does" sounds weird. How about simple "all available
modes" ?
> The driver make use of large tables of static register/value writes when
s/make/makes/
> powering up/down the TXA and TXB transmitters which include the write to
> the NUM_LANES register. By converting the tables into functions and
> using parameters the power up/down functions for TXA and TXB power
> up/down can be merged and used for both transmitters.
>
> Signed-off-by: Niklas Söderlund
Reviewed-by: Laurent Pinchart
The code looks so much better now :-)
> ---
> * Changes since v2
> - Fix typos in comments.
> - Remove unneeded boiler plait code in adv748x_tx_power() as suggested
> by Jacopo and Laurent.
> - Take into account the two different register used when powering up TXA
> and TXB due to an earlier patch in this series aligns the power
> sequence with the manual.
>
> * Changes since v1
> - Convert tables of register/value writes into functions instead of
> intercepting and modifying the writes to the NUM_LANES register.
> ---
> drivers/media/i2c/adv748x/adv748x-core.c | 157 ---
> 1 file changed, 79 insertions(+), 78 deletions(-)
>
> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c
> b/drivers/media/i2c/adv748x/adv748x-core.c index
> 9d80d7f3062b16bc..d94c63cb6a2efdba 100644
> --- a/drivers/media/i2c/adv748x/adv748x-core.c
> +++ b/drivers/media/i2c/adv748x/adv748x-core.c
> @@ -125,6 +125,16 @@ int adv748x_write(struct adv748x_state *state, u8 page,
> u8 reg, u8 value) return regmap_write(state->regmap[page], reg, value);
> }
>
> +static int adv748x_write_check(struct adv748x_state *state, u8 page, u8
> reg,
> +u8 value, int *error)
> +{
> + if (*error)
> + return *error;
> +
> + *error = adv748x_write(state, page, reg, value);
> + return *error;
> +}
> +
> /* adv748x_write_block(): Write raw data with a maximum of
> I2C_SMBUS_BLOCK_MAX * size to one or more registers.
> *
> @@ -231,79 +241,77 @@ static int adv748x_write_regs(struct adv748x_state
> *state, * TXA and TXB
> */
>
> -static const struct adv748x_reg_value adv748x_power_up_txa_4lane[] = {
> -
> - {ADV748X_PAGE_TXA, 0x00, 0x84}, /* Enable 4-lane MIPI */
> - {ADV748X_PAGE_TXA, 0x00, 0xa4}, /* Set Auto DPHY Timing */
> - {ADV748X_PAGE_TXA, 0xdb, 0x10}, /* ADI Required Write */
> - {ADV748X_PAGE_TXA, 0xd6, 0x07}, /* ADI Required Write */
> - {ADV748X_PAGE_TXA, 0xc4, 0x0a}, /* ADI Required Write */
> - {ADV748X_PAGE_TXA, 0x71, 0x33}, /* ADI Required Write */
> - {ADV748X_PAGE_TXA, 0x72, 0x11}, /* ADI Required Write */
> - {ADV748X_PAGE_TXA, 0xf0, 0x00}, /* i2c_dphy_pwdn - 1'b0 */
> -
> - {ADV748X_PAGE_TXA, 0x31, 0x82}, /* ADI Required Write */
> - {ADV748X_PAGE_TXA, 0x1e, 0x40}, /* ADI Required Write */
> - {ADV748X_PAGE_TXA, 0xda, 0x01}, /* i2c_mipi_pll_en - 1'b1 */
> - {ADV748X_PAGE_WAIT, 0x00, 0x02},/* delay 2 */
> - {ADV748X_PAGE_TXA, 0x00, 0x24 },/* Power-up CSI-TX */
> - {ADV748X_PAGE_WAIT, 0x00, 0x01},/* delay 1 */
> - {ADV748X_PAGE_TXA, 0xc1, 0x2b}, /* ADI Required Write */
> - {ADV748X_PAGE_WAIT, 0x00, 0x01},/* delay 1 */
> - {ADV748X_PAGE_TXA, 0x31, 0x80}, /* ADI Required Write */
> -
> - {ADV748X_PAGE_EOR, 0xff, 0xff} /* End of register table */
> -};
> -
> -static const struct adv748x_reg_value adv748x_power_down_txa_4lane[] = {
> -
> - {ADV748X_PAGE_TXA, 0x31, 0x82}, /* ADI Required Write */
> - {ADV748X_PAGE_TXA, 0x1e, 0x00}, /* ADI Required Write */
> - {ADV748X_PAGE_TXA, 0x00, 0x84}, /* Enable 4-lane MIPI */
> - {ADV748X_PAGE_TXA, 0xda, 0x01}, /* i2c_mipi_pll_en - 1'b1 */
> - {ADV748X_PAGE_TXA, 0xc1, 0x3b}, /* ADI Required Write */
> -
> - {ADV748X_PAGE_EOR, 0xff, 0xff} /* End of register table */
> -};
> -
> -static const struct adv748x_reg_value adv748x_power_up_txb_1lane[] = {
> -
> - {ADV748X_PAGE_TXB, 0x00, 0x81}, /* Enable 1-lane MIPI */
> - {ADV748X_PAGE_TXB, 0x00, 0xa1}, /* Set Auto DPHY Timing */
> - {ADV748X_PAGE_TXB, 0xd2, 0x40}, /* ADI Required Write */
> - {ADV748X_PAGE_TXB, 0xc4, 0x0a}, /* ADI Required Write */
> - {ADV748X_PAGE_TXB, 0x71, 0x33}, /* ADI Required Write */
> - {ADV748X_PAGE_TXB, 0x72, 0x11}, /* ADI Required Write */
> - {ADV748X_PAGE_TXB, 0xf0, 0x00}, /* i2c_dphy_pwdn - 1'b0 */
> -
> - {ADV748X_PAGE_TXB, 0x31, 0x82}, /* ADI Required Write */
> - {ADV748X_PAGE_TXB, 0x1e, 0x40}, /* ADI Required Write */
> - {ADV748X_PAGE_TXB, 0xda, 0x01}, /* i2c_mipi_pll_en - 1'b1 */
> - {ADV748X_PAGE_WAIT, 0x00, 0x02},/* delay 2 */
> - {ADV748X_PAGE_TXB, 0x00, 0x21 },/* Power-up CSI-TX */
> - {ADV748X_PAGE_WAIT,
[PATCH 3/6] media: dt-bindings: rcar-csi2: Add R8A77990
Add compatible string for R-Car E3 R8A77990 to the list of supported SoCs. Signed-off-by: Jacopo Mondi Reviewed-by: Rob Herring Reviewed-by: Simon Horman Reviewed-by: Laurent Pinchart Acked-by: Niklas Söderlund --- Documentation/devicetree/bindings/media/renesas,rcar-csi2.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/Documentation/devicetree/bindings/media/renesas,rcar-csi2.txt b/Documentation/devicetree/bindings/media/renesas,rcar-csi2.txt index 2d385b6..2824489 100644 --- a/Documentation/devicetree/bindings/media/renesas,rcar-csi2.txt +++ b/Documentation/devicetree/bindings/media/renesas,rcar-csi2.txt @@ -12,6 +12,7 @@ Mandatory properties - "renesas,r8a7796-csi2" for the R8A7796 device. - "renesas,r8a77965-csi2" for the R8A77965 device. - "renesas,r8a77970-csi2" for the R8A77970 device. + - "renesas,r8a77990-csi2" for the R8A77990 device. - reg: the register base and size for the device registers - interrupts: the interrupt for the device -- 2.7.4
[PATCH 5/6] media: rcar: rcar-csi2: Update V3M/E3 PHTW tables
Update PHTW tables for V3M and E3 SoCs to the latest datasheet release.
Signed-off-by: Jacopo Mondi
---
drivers/media/platform/rcar-vin/rcar-csi2.c | 71 -
1 file changed, 39 insertions(+), 32 deletions(-)
diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c
b/drivers/media/platform/rcar-vin/rcar-csi2.c
index 695686b..5689a60 100644
--- a/drivers/media/platform/rcar-vin/rcar-csi2.c
+++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
@@ -152,38 +152,45 @@ static const struct rcsi2_mbps_reg phtw_mbps_h3_v3h_m3n[]
= {
};
static const struct rcsi2_mbps_reg phtw_mbps_v3m_e3[] = {
- { .mbps = 80, .reg = 0x00 },
- { .mbps = 90, .reg = 0x20 },
- { .mbps = 100, .reg = 0x40 },
- { .mbps = 110, .reg = 0x02 },
- { .mbps = 130, .reg = 0x22 },
- { .mbps = 140, .reg = 0x42 },
- { .mbps = 150, .reg = 0x04 },
- { .mbps = 170, .reg = 0x24 },
- { .mbps = 180, .reg = 0x44 },
- { .mbps = 200, .reg = 0x06 },
- { .mbps = 220, .reg = 0x26 },
- { .mbps = 240, .reg = 0x46 },
- { .mbps = 250, .reg = 0x08 },
- { .mbps = 270, .reg = 0x28 },
- { .mbps = 300, .reg = 0x0a },
- { .mbps = 330, .reg = 0x2a },
- { .mbps = 360, .reg = 0x4a },
- { .mbps = 400, .reg = 0x0c },
- { .mbps = 450, .reg = 0x2c },
- { .mbps = 500, .reg = 0x0e },
- { .mbps = 550, .reg = 0x2e },
- { .mbps = 600, .reg = 0x10 },
- { .mbps = 650, .reg = 0x30 },
- { .mbps = 700, .reg = 0x12 },
- { .mbps = 750, .reg = 0x32 },
- { .mbps = 800, .reg = 0x52 },
- { .mbps = 850, .reg = 0x72 },
- { .mbps = 900, .reg = 0x14 },
- { .mbps = 950, .reg = 0x34 },
- { .mbps = 1000, .reg = 0x54 },
- { .mbps = 1050, .reg = 0x74 },
- { .mbps = 1125, .reg = 0x16 },
+ { .mbps = 89, .reg = 0x00 },
+ { .mbps = 99, .reg = 0x20 },
+ { .mbps = 109, .reg = 0x40 },
+ { .mbps = 129, .reg = 0x02 },
+ { .mbps = 139, .reg = 0x22 },
+ { .mbps = 149, .reg = 0x42 },
+ { .mbps = 169, .reg = 0x04 },
+ { .mbps = 179, .reg = 0x24 },
+ { .mbps = 199, .reg = 0x44 },
+ { .mbps = 219, .reg = 0x06 },
+ { .mbps = 239, .reg = 0x26 },
+ { .mbps = 249, .reg = 0x46 },
+ { .mbps = 269, .reg = 0x08 },
+ { .mbps = 299, .reg = 0x28 },
+ { .mbps = 329, .reg = 0x0a },
+ { .mbps = 359, .reg = 0x2a },
+ { .mbps = 399, .reg = 0x4a },
+ { .mbps = 449, .reg = 0x0c },
+ { .mbps = 499, .reg = 0x2c },
+ { .mbps = 549, .reg = 0x0e },
+ { .mbps = 599, .reg = 0x2e },
+ { .mbps = 649, .reg = 0x10 },
+ { .mbps = 699, .reg = 0x30 },
+ { .mbps = 749, .reg = 0x12 },
+ { .mbps = 799, .reg = 0x32 },
+ { .mbps = 849, .reg = 0x52 },
+ { .mbps = 899, .reg = 0x72 },
+ { .mbps = 949, .reg = 0x14 },
+ { .mbps = 999, .reg = 0x34 },
+ { .mbps = 1049, .reg = 0x54 },
+ { .mbps = 1099, .reg = 0x74 },
+ { .mbps = 1149, .reg = 0x16 },
+ { .mbps = 1199, .reg = 0x36 },
+ { .mbps = 1249, .reg = 0x56 },
+ { .mbps = 1299, .reg = 0x76 },
+ { .mbps = 1349, .reg = 0x18 },
+ { .mbps = 1399, .reg = 0x38 },
+ { .mbps = 1449, .reg = 0x58 },
+ { .mbps = 1500, .reg = 0x78 },
{ /* sentinel */ },
};
--
2.7.4
[PATCH 2/6] media: rcar-vin: Add support for R-Car R8A77990
Add R-Car E3 R8A77990 SoC to the rcar-vin supported ones.
Based on the experimental patch from Magnus Damm.
Signed-off-by: Jacopo Mondi
Reviewed-by: Laurent Pinchart
Acked-by: Niklas Söderlund
---
drivers/media/platform/rcar-vin/rcar-core.c | 20
1 file changed, 20 insertions(+)
diff --git a/drivers/media/platform/rcar-vin/rcar-core.c
b/drivers/media/platform/rcar-vin/rcar-core.c
index f476b2f..cae2166 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -1088,6 +1088,22 @@ static const struct rvin_info rcar_info_r8a77970 = {
.routes = rcar_info_r8a77970_routes,
};
+static const struct rvin_group_route rcar_info_r8a77990_routes[] = {
+ { .csi = RVIN_CSI40, .channel = 0, .vin = 4, .mask = BIT(0) | BIT(3) },
+ { .csi = RVIN_CSI40, .channel = 0, .vin = 5, .mask = BIT(2) },
+ { .csi = RVIN_CSI40, .channel = 1, .vin = 4, .mask = BIT(2) },
+ { .csi = RVIN_CSI40, .channel = 1, .vin = 5, .mask = BIT(1) | BIT(3) },
+ { /* Sentinel */ }
+};
+
+static const struct rvin_info rcar_info_r8a77990 = {
+ .model = RCAR_GEN3,
+ .use_mc = true,
+ .max_width = 4096,
+ .max_height = 4096,
+ .routes = rcar_info_r8a77990_routes,
+};
+
static const struct rvin_group_route rcar_info_r8a77995_routes[] = {
{ /* Sentinel */ }
};
@@ -1146,6 +1162,10 @@ static const struct of_device_id rvin_of_id_table[] = {
.data = &rcar_info_r8a77970,
},
{
+ .compatible = "renesas,vin-r8a77990",
+ .data = &rcar_info_r8a77990,
+ },
+ {
.compatible = "renesas,vin-r8a77995",
.data = &rcar_info_r8a77995,
},
--
2.7.4
[PATCH 4/6] media: rcar-csi2: Add R8A77990 support
Add support for R-Car E3 R8A77965 to R-Car CSI-2 driver.
Based on the experimental patch from Magnus Damm.
Signed-off-by: Jacopo Mondi
Reviewed-by: Laurent Pinchart
Acked-by: Niklas Söderlund
---
drivers/media/platform/rcar-vin/rcar-csi2.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c
b/drivers/media/platform/rcar-vin/rcar-csi2.c
index b0044a0..695686b 100644
--- a/drivers/media/platform/rcar-vin/rcar-csi2.c
+++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
@@ -963,6 +963,11 @@ static const struct rcar_csi2_info rcar_csi2_info_r8a77970
= {
.confirm_start = rcsi2_confirm_start_v3m_e3,
};
+static const struct rcar_csi2_info rcar_csi2_info_r8a77990 = {
+ .init_phtw = rcsi2_init_phtw_v3m_e3,
+ .confirm_start = rcsi2_confirm_start_v3m_e3,
+};
+
static const struct of_device_id rcar_csi2_of_table[] = {
{
.compatible = "renesas,r8a7795-csi2",
@@ -980,6 +985,10 @@ static const struct of_device_id rcar_csi2_of_table[] = {
.compatible = "renesas,r8a77970-csi2",
.data = &rcar_csi2_info_r8a77970,
},
+ {
+ .compatible = "renesas,r8a77990-csi2",
+ .data = &rcar_csi2_info_r8a77990,
+ },
{ /* sentinel */ },
};
MODULE_DEVICE_TABLE(of, rcar_csi2_of_table);
--
2.7.4
[PATCH 1/6] media: dt-bindings: rcar-vin: Add R8A77990 support
Add compatible string for R-Car E3 R8A77990 to the list of SoCs supported by rcar-vin driver. Signed-off-by: Jacopo Mondi Reviewed-by: Rob Herring Reviewed-by: Simon Horman Reviewed-by: Laurent Pinchart Acked-by: Niklas Söderlund --- Documentation/devicetree/bindings/media/rcar_vin.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/Documentation/devicetree/bindings/media/rcar_vin.txt b/Documentation/devicetree/bindings/media/rcar_vin.txt index d329a4e..7c878ca 100644 --- a/Documentation/devicetree/bindings/media/rcar_vin.txt +++ b/Documentation/devicetree/bindings/media/rcar_vin.txt @@ -24,6 +24,7 @@ on Gen3 platforms to a CSI-2 receiver. - "renesas,vin-r8a7796" for the R8A7796 device - "renesas,vin-r8a77965" for the R8A77965 device - "renesas,vin-r8a77970" for the R8A77970 device + - "renesas,vin-r8a77990" for the R8A77990 device - "renesas,vin-r8a77995" for the R8A77995 device - "renesas,rcar-gen2-vin" for a generic R-Car Gen2 or RZ/G1 compatible device. -- 2.7.4
[PATCH 6/6] media: rcar-csi2: Handle per-SoC number of channels
The R-Car CSI-2 interface has a number of selectable 'channels' that
provides pixel data to the VINs during image acquisition.
Each channel can be used to match a CSI-2 data type and a CSI-2 virtual
channel to be routed to output path.
Different SoCs have different number of channels, with R-Car E3 being the
notable exception supporting only 2 of them.
Signed-off-by: Jacopo Mondi
---
drivers/media/platform/rcar-vin/rcar-csi2.c | 15 ---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c
b/drivers/media/platform/rcar-vin/rcar-csi2.c
index 5689a60..95a3dd4 100644
--- a/drivers/media/platform/rcar-vin/rcar-csi2.c
+++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
@@ -349,6 +349,7 @@ struct rcar_csi2_info {
int (*confirm_start)(struct rcar_csi2 *priv);
const struct rcsi2_mbps_reg *hsfreqrange;
unsigned int csi0clkfreqrange;
+ unsigned int num_channels;
bool clear_ulps;
};
@@ -483,13 +484,14 @@ static int rcsi2_start(struct rcar_csi2 *priv)
format = rcsi2_code_to_fmt(priv->mf.code);
/*
-* Enable all Virtual Channels.
+* Enable all supported CSI-2 channels with virtual channel and
+* data type matching.
*
* NOTE: It's not possible to get individual datatype for each
* source virtual channel. Once this is possible in V4L2
* it should be used here.
*/
- for (i = 0; i < 4; i++) {
+ for (i = 0; i < priv->info->num_channels; i++) {
u32 vcdt_part;
vcdt_part = VCDT_SEL_VC(i) | VCDT_VCDTN_EN | VCDT_SEL_DTN_ON |
@@ -518,7 +520,8 @@ static int rcsi2_start(struct rcar_csi2 *priv)
rcsi2_write(priv, FLD_REG, FLD_FLD_NUM(2) | FLD_FLD_EN4 |
FLD_FLD_EN3 | FLD_FLD_EN2 | FLD_FLD_EN);
rcsi2_write(priv, VCDT_REG, vcdt);
- rcsi2_write(priv, VCDT2_REG, vcdt2);
+ if (vcdt2)
+ rcsi2_write(priv, VCDT2_REG, vcdt2);
/* Lanes are zero indexed. */
rcsi2_write(priv, LSWAP_REG,
LSWAP_L0SEL(priv->lane_swap[0] - 1) |
@@ -947,32 +950,38 @@ static const struct rcar_csi2_info rcar_csi2_info_r8a7795
= {
.init_phtw = rcsi2_init_phtw_h3_v3h_m3n,
.hsfreqrange = hsfreqrange_h3_v3h_m3n,
.csi0clkfreqrange = 0x20,
+ .num_channels = 4,
.clear_ulps = true,
};
static const struct rcar_csi2_info rcar_csi2_info_r8a7795es1 = {
.hsfreqrange = hsfreqrange_m3w_h3es1,
+ .num_channels = 4,
};
static const struct rcar_csi2_info rcar_csi2_info_r8a7796 = {
.hsfreqrange = hsfreqrange_m3w_h3es1,
+ .num_channels = 4,
};
static const struct rcar_csi2_info rcar_csi2_info_r8a77965 = {
.init_phtw = rcsi2_init_phtw_h3_v3h_m3n,
.hsfreqrange = hsfreqrange_h3_v3h_m3n,
.csi0clkfreqrange = 0x20,
+ .num_channels = 4,
.clear_ulps = true,
};
static const struct rcar_csi2_info rcar_csi2_info_r8a77970 = {
.init_phtw = rcsi2_init_phtw_v3m_e3,
.confirm_start = rcsi2_confirm_start_v3m_e3,
+ .num_channels = 4,
};
static const struct rcar_csi2_info rcar_csi2_info_r8a77990 = {
.init_phtw = rcsi2_init_phtw_v3m_e3,
.confirm_start = rcsi2_confirm_start_v3m_e3,
+ .num_channels = 2,
};
static const struct of_device_id rcar_csi2_of_table[] = {
--
2.7.4
[PATCH 0/6] media: rcar-vin: Add support for R-Car E3
Hello, this series add support for the R-Car E3 R8A77990 SoC to the rcar-vin and rcar-csi2 driver. Nothing new compared to v2, except for the last two patches, which updates PHTW tables to the latest SoC manual revision, and add a SoC-specific number of CSI-2 channels to handle the E3 case properly, as this is the only SoC in mainline with a number of CSI-2 channels < 4. Most patches are already reviewed-by and acked-by, so I expect this to be easy to have accepted. Tested on E3 Ebisu board capturing frames from the HDMI input in different resolutions (up to 1280x800). Testing requires Niklas' series to allow using a numer of data lanes < 4 for ADV748x, PFC and DTS updates. A branch for testing is available at: git://jmondi.org/linux ebisu/v4.20-rc1/test Thanks j Jacopo Mondi (6): media: dt-bindings: rcar-vin: Add R8A77990 support media: rcar-vin: Add support for R-Car R8A77990 media: dt-bindings: rcar-csi2: Add R8A77990 media: rcar-csi2: Add R8A77990 support media: rcar: rcar-csi2: Update V3M/E3 PHTW tables media: rcar-csi2: Handle per-SoC number of channels .../devicetree/bindings/media/rcar_vin.txt | 1 + .../bindings/media/renesas,rcar-csi2.txt | 1 + drivers/media/platform/rcar-vin/rcar-core.c| 20 + drivers/media/platform/rcar-vin/rcar-csi2.c| 95 ++ 4 files changed, 82 insertions(+), 35 deletions(-) -- 2.7.4
Re: [PATCH 6/6] media: rcar-csi2: Handle per-SoC number of channels
Hi Jacopo,
Thank you for the patch.
On Monday, 5 November 2018 13:19:11 EET Jacopo Mondi wrote:
> The R-Car CSI-2 interface has a number of selectable 'channels' that
> provides pixel data to the VINs during image acquisition.
>
> Each channel can be used to match a CSI-2 data type and a CSI-2 virtual
> channel to be routed to output path.
>
> Different SoCs have different number of channels, with R-Car E3 being the
> notable exception supporting only 2 of them.
>
> Signed-off-by: Jacopo Mondi
> ---
> drivers/media/platform/rcar-vin/rcar-csi2.c | 15 ---
> 1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c
> b/drivers/media/platform/rcar-vin/rcar-csi2.c index 5689a60..95a3dd4 100644
> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> @@ -349,6 +349,7 @@ struct rcar_csi2_info {
> int (*confirm_start)(struct rcar_csi2 *priv);
> const struct rcsi2_mbps_reg *hsfreqrange;
> unsigned int csi0clkfreqrange;
> + unsigned int num_channels;
> bool clear_ulps;
> };
>
> @@ -483,13 +484,14 @@ static int rcsi2_start(struct rcar_csi2 *priv)
> format = rcsi2_code_to_fmt(priv->mf.code);
>
> /*
> - * Enable all Virtual Channels.
> + * Enable all supported CSI-2 channels with virtual channel and
> + * data type matching.
>*
>* NOTE: It's not possible to get individual datatype for each
>* source virtual channel. Once this is possible in V4L2
>* it should be used here.
>*/
> - for (i = 0; i < 4; i++) {
> + for (i = 0; i < priv->info->num_channels; i++) {
> u32 vcdt_part;
>
> vcdt_part = VCDT_SEL_VC(i) | VCDT_VCDTN_EN | VCDT_SEL_DTN_ON |
> @@ -518,7 +520,8 @@ static int rcsi2_start(struct rcar_csi2 *priv)
> rcsi2_write(priv, FLD_REG, FLD_FLD_NUM(2) | FLD_FLD_EN4 |
> FLD_FLD_EN3 | FLD_FLD_EN2 | FLD_FLD_EN);
> rcsi2_write(priv, VCDT_REG, vcdt);
> - rcsi2_write(priv, VCDT2_REG, vcdt2);
> + if (vcdt2)
I would write this as
if (priv->info->num_channels > 2)
in order to later support configuration of virtual channels and data types
that could result in the output channels 2 and 3 being disabled.
Apart from this,
Reviewed-by: Laurent Pinchart
> + rcsi2_write(priv, VCDT2_REG, vcdt2);
> /* Lanes are zero indexed. */
> rcsi2_write(priv, LSWAP_REG,
> LSWAP_L0SEL(priv->lane_swap[0] - 1) |
> @@ -947,32 +950,38 @@ static const struct rcar_csi2_info
> rcar_csi2_info_r8a7795 = { .init_phtw = rcsi2_init_phtw_h3_v3h_m3n,
> .hsfreqrange = hsfreqrange_h3_v3h_m3n,
> .csi0clkfreqrange = 0x20,
> + .num_channels = 4,
> .clear_ulps = true,
> };
>
> static const struct rcar_csi2_info rcar_csi2_info_r8a7795es1 = {
> .hsfreqrange = hsfreqrange_m3w_h3es1,
> + .num_channels = 4,
> };
>
> static const struct rcar_csi2_info rcar_csi2_info_r8a7796 = {
> .hsfreqrange = hsfreqrange_m3w_h3es1,
> + .num_channels = 4,
> };
>
> static const struct rcar_csi2_info rcar_csi2_info_r8a77965 = {
> .init_phtw = rcsi2_init_phtw_h3_v3h_m3n,
> .hsfreqrange = hsfreqrange_h3_v3h_m3n,
> .csi0clkfreqrange = 0x20,
> + .num_channels = 4,
> .clear_ulps = true,
> };
>
> static const struct rcar_csi2_info rcar_csi2_info_r8a77970 = {
> .init_phtw = rcsi2_init_phtw_v3m_e3,
> .confirm_start = rcsi2_confirm_start_v3m_e3,
> + .num_channels = 4,
> };
>
> static const struct rcar_csi2_info rcar_csi2_info_r8a77990 = {
> .init_phtw = rcsi2_init_phtw_v3m_e3,
> .confirm_start = rcsi2_confirm_start_v3m_e3,
> + .num_channels = 2,
> };
>
> static const struct of_device_id rcar_csi2_of_table[] = {
--
Regards,
Laurent Pinchart
Re: [PATCH 5/6] media: rcar: rcar-csi2: Update V3M/E3 PHTW tables
Hi Jacopo,
Thank you for the patch.
On Monday, 5 November 2018 13:19:10 EET Jacopo Mondi wrote:
> Update PHTW tables for V3M and E3 SoCs to the latest datasheet release.
>
> Signed-off-by: Jacopo Mondi
> ---
> drivers/media/platform/rcar-vin/rcar-csi2.c | 71 +++---
> 1 file changed, 39 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c
> b/drivers/media/platform/rcar-vin/rcar-csi2.c index 695686b..5689a60 100644
> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> @@ -152,38 +152,45 @@ static const struct rcsi2_mbps_reg
> phtw_mbps_h3_v3h_m3n[] = { };
>
> static const struct rcsi2_mbps_reg phtw_mbps_v3m_e3[] = {
> - { .mbps = 80, .reg = 0x00 },
> - { .mbps = 90, .reg = 0x20 },
> - { .mbps = 100, .reg = 0x40 },
> - { .mbps = 110, .reg = 0x02 },
> - { .mbps = 130, .reg = 0x22 },
> - { .mbps = 140, .reg = 0x42 },
> - { .mbps = 150, .reg = 0x04 },
> - { .mbps = 170, .reg = 0x24 },
> - { .mbps = 180, .reg = 0x44 },
> - { .mbps = 200, .reg = 0x06 },
> - { .mbps = 220, .reg = 0x26 },
> - { .mbps = 240, .reg = 0x46 },
> - { .mbps = 250, .reg = 0x08 },
> - { .mbps = 270, .reg = 0x28 },
> - { .mbps = 300, .reg = 0x0a },
> - { .mbps = 330, .reg = 0x2a },
> - { .mbps = 360, .reg = 0x4a },
> - { .mbps = 400, .reg = 0x0c },
> - { .mbps = 450, .reg = 0x2c },
> - { .mbps = 500, .reg = 0x0e },
> - { .mbps = 550, .reg = 0x2e },
> - { .mbps = 600, .reg = 0x10 },
> - { .mbps = 650, .reg = 0x30 },
> - { .mbps = 700, .reg = 0x12 },
> - { .mbps = 750, .reg = 0x32 },
> - { .mbps = 800, .reg = 0x52 },
> - { .mbps = 850, .reg = 0x72 },
> - { .mbps = 900, .reg = 0x14 },
> - { .mbps = 950, .reg = 0x34 },
> - { .mbps = 1000, .reg = 0x54 },
> - { .mbps = 1050, .reg = 0x74 },
> - { .mbps = 1125, .reg = 0x16 },
> + { .mbps = 89, .reg = 0x00 },
> + { .mbps = 99, .reg = 0x20 },
> + { .mbps = 109, .reg = 0x40 },
> + { .mbps = 129, .reg = 0x02 },
> + { .mbps = 139, .reg = 0x22 },
> + { .mbps = 149, .reg = 0x42 },
> + { .mbps = 169, .reg = 0x04 },
> + { .mbps = 179, .reg = 0x24 },
> + { .mbps = 199, .reg = 0x44 },
> + { .mbps = 219, .reg = 0x06 },
> + { .mbps = 239, .reg = 0x26 },
> + { .mbps = 249, .reg = 0x46 },
> + { .mbps = 269, .reg = 0x08 },
> + { .mbps = 299, .reg = 0x28 },
> + { .mbps = 329, .reg = 0x0a },
> + { .mbps = 359, .reg = 0x2a },
> + { .mbps = 399, .reg = 0x4a },
> + { .mbps = 449, .reg = 0x0c },
> + { .mbps = 499, .reg = 0x2c },
> + { .mbps = 549, .reg = 0x0e },
> + { .mbps = 599, .reg = 0x2e },
> + { .mbps = 649, .reg = 0x10 },
> + { .mbps = 699, .reg = 0x30 },
> + { .mbps = 749, .reg = 0x12 },
> + { .mbps = 799, .reg = 0x32 },
> + { .mbps = 849, .reg = 0x52 },
> + { .mbps = 899, .reg = 0x72 },
> + { .mbps = 949, .reg = 0x14 },
> + { .mbps = 999, .reg = 0x34 },
> + { .mbps = 1049, .reg = 0x54 },
> + { .mbps = 1099, .reg = 0x74 },
> + { .mbps = 1149, .reg = 0x16 },
> + { .mbps = 1199, .reg = 0x36 },
> + { .mbps = 1249, .reg = 0x56 },
> + { .mbps = 1299, .reg = 0x76 },
> + { .mbps = 1349, .reg = 0x18 },
> + { .mbps = 1399, .reg = 0x38 },
> + { .mbps = 1449, .reg = 0x58 },
> + { .mbps = 1500, .reg = 0x78 },
> { /* sentinel */ },
> };
In the latest datasheet version I can find, the frequencies go up to 1125 MHz
only. I've verified values up to that point, but not beyond it.
--
Regards,
Laurent Pinchart
Re: [PATCH v2] media: vivid: Improve timestamping
Hi Gabriel,
This doesn't work. See my comment below.
I recommend testing with:
v4l2-ctl -P --stream-mmap --verbose
This shows the expected frame rate and --verbose shows the reported frame
periods.
On 10/22/2018 01:00 AM, Gabriel Francisco Mandaji wrote:
> Simulate a more precise timestamp by calculating it based on the
> current framerate.
>
> Signed-off-by: Gabriel Francisco Mandaji
> ---
> Changes in v2:
> - fix spelling
> - end of exposure is offset by 90% of the frame period
> - fix timestamp calculation for FIELD_ALTERNATE (untested)
> - timestamp is now calculated and set from vivid_thread_cap_tick()
> - capture vbi uses the same timestamp as non-vbi, but offset by 5%
> - timestamp stays consistent even if the FPS changes
> - tested with dropped frames
>
> If 'Start of Exposure'/'End of Frame' changes mid-capture, it will be
> completely ignored. If that's an issue, I'll change how the frame
> period and cap_stream_start are calculated.
>
> Also, should I modify the output's timestamp on this patch or on
> adrivers/media/platform/vivid/vivid-kthread-cap.c
> separated one?
Separate one.
> ---
> drivers/media/platform/vivid/vivid-core.h| 2 +
> drivers/media/platform/vivid/vivid-kthread-cap.c | 47
> +---
> drivers/media/platform/vivid/vivid-vbi-cap.c | 4 --
> 3 files changed, 35 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/media/platform/vivid/vivid-core.h
> b/drivers/media/platform/vivid/vivid-core.h
> index cd4c823..ba6fb3a 100644
> --- a/drivers/media/platform/vivid/vivid-core.h
> +++ b/drivers/media/platform/vivid/vivid-core.h
> @@ -384,6 +384,8 @@ struct vivid_dev {
> /* thread for generating video capture stream */
> struct task_struct *kthread_vid_cap;
> unsigned long jiffies_vid_cap;
> + u64 cap_stream_start;
> + u64 cap_frame_period;
> u32 cap_seq_offset;
> u32 cap_seq_count;
> boolcap_seq_resync;
> diff --git a/drivers/media/platform/vivid/vivid-kthread-cap.c
> b/drivers/media/platform/vivid/vivid-kthread-cap.c
> index f06003b..828a58c 100644
> --- a/drivers/media/platform/vivid/vivid-kthread-cap.c
> +++ b/drivers/media/platform/vivid/vivid-kthread-cap.c
> @@ -425,12 +425,6 @@ static void vivid_fillbuff(struct vivid_dev *dev, struct
> vivid_buffer *buf)
> is_loop = true;
>
> buf->vb.sequence = dev->vid_cap_seq_count;
> - /*
> - * Take the timestamp now if the timestamp source is set to
> - * "Start of Exposure".
> - */
> - if (dev->tstamp_src_is_soe)
> - buf->vb.vb2_buf.timestamp = ktime_get_ns();
> if (dev->field_cap == V4L2_FIELD_ALTERNATE) {
> /*
>* 60 Hz standards start with the bottom field, 50 Hz standards
> @@ -554,14 +548,6 @@ static void vivid_fillbuff(struct vivid_dev *dev, struct
> vivid_buffer *buf)
> }
> }
> }
> -
> - /*
> - * If "End of Frame" is specified at the timestamp source, then take
> - * the timestamp now.
> - */
> - if (!dev->tstamp_src_is_soe)
> - buf->vb.vb2_buf.timestamp = ktime_get_ns();
> - buf->vb.vb2_buf.timestamp += dev->time_wrap_offset;
> }
>
> /*
> @@ -667,10 +653,27 @@ static void vivid_overlay(struct vivid_dev *dev, struct
> vivid_buffer *buf)
> }
> }
>
> +static void vivid_cap_update_frame_period(struct vivid_dev *dev)
> +{
> + u64 f_period = dev->timeperframe_vid_cap.numerator * 10 /
> +dev->timeperframe_vid_cap.denominator;
> + if (dev->field_cap == V4L2_FIELD_ALTERNATE)
> + f_period /= 2;
> + /*
> + * If "End of Frame", then calculate the exposure time as 0.9
> + * of the frame period.
> + */
> + if (!dev->tstamp_src_is_soe)
> + f_period += f_period / 10 * 9;
This is very wrong: if the timestamp is for the "End of Frame" (the default),
then
that does not change the frame period value, it just adds an offset.
Easiest is probably to add a dev->cap_frame_offset field:
if (dev->tstamp_src_is_soe)
dev->cap_frame_offset = 0;
else
dev->cap_frame_offset = f_period * 9 / 10;
This bug gave really weird frame periods.
Regards,
Hans
> +
> + dev->cap_frame_period = f_period;
> +}
> +
> static void vivid_thread_vid_cap_tick(struct vivid_dev *dev, int
> dropped_bufs)
> {
> struct vivid_buffer *vid_cap_buf = NULL;
> struct vivid_buffer *vbi_cap_buf = NULL;
> + u64 f_time = 0;
>
> dprintk(dev, 1, "Video Capture Thread Tick\n");
>
> @@ -702,6 +705,9 @@ static void vivid_thread_vid_cap_tick(struct vivid_dev
> *dev, int dropped_bufs)
> if (!vid_cap_buf && !vbi_cap_buf)
> goto update_mv
Re: [PATCH] [media] ov7670: make "xclk" clock optional
Hello,
On Mon, 2018-11-05 at 11:58 +0100, jacopo mondi wrote:
> Hi Lubomir,
>+Sakari in Cc
>
> I just noticed this, and the patch is now in v4.20, but let me comment
> anyway on this.
>
> On Thu, Oct 04, 2018 at 11:29:03PM +0200, Lubomir Rintel wrote:
> > When the "xclk" clock was added, it was made mandatory. This broke the
> > driver on an OLPC plaform which doesn't know such clock. Make it
> > optional.
> >
>
> I don't think this is correct. The sensor needs a clock to work.
>
> With this patch clock_speed which is used to calculate
> the framerate is defaulted to 30MHz, crippling all the calculations if
> that default value doesn't match what is actually installed on the
> board.
How come? I kept this:
+ info->clock_speed = clk_get_rate(info->clk) / 100;
>
> If this patch breaks the OLPC, then might it be the DTS for said
> device needs to be fixed instead of working around the issue here?
No. Device tree is an ABI, and you can't just add mandatory properties.
There's no DTS for OLPC XO-1 either; it's an OpenFirmware machine.
You'd need to update all machines in the wild which is not realistic.
Alternatively, something else than DT could provide the clock. If this
gets in, then the OLPC would work even without the xclk patch:
https://lore.kernel.org/lkml/20181105073054.24407-12-lkund...@v3.sk/
(I just got a kbuild failure message, so I'll surely be following up
with a v2.)
> Also, the DT bindings should be updated too if we decide this property
> can be omitted. At this point, with a follow-up patch.
Yes.
>
> Thanks
Cheers
Lubo
>j
>
> > Tested on a OLPC XO-1 laptop.
> >
> > Cc: sta...@vger.kernel.org # 4.11+
> > Fixes: 0a024d634cee ("[media] ov7670: get xclk")
> > Signed-off-by: Lubomir Rintel
> > ---
> > drivers/media/i2c/ov7670.c | 27 +--
> > 1 file changed, 17 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov7670.c b/drivers/media/i2c/ov7670.c
> > index 31bf577b0bd3..64d1402882c8 100644
> > --- a/drivers/media/i2c/ov7670.c
> > +++ b/drivers/media/i2c/ov7670.c
> > @@ -1808,17 +1808,24 @@ static int ov7670_probe(struct i2c_client *client,
> > info->pclk_hb_disable = true;
> > }
> >
> > - info->clk = devm_clk_get(&client->dev, "xclk");
> > - if (IS_ERR(info->clk))
> > - return PTR_ERR(info->clk);
> > - ret = clk_prepare_enable(info->clk);
> > - if (ret)
> > - return ret;
> > + info->clk = devm_clk_get(&client->dev, "xclk"); /* optional */
> > + if (IS_ERR(info->clk)) {
> > + ret = PTR_ERR(info->clk);
> > + if (ret == -ENOENT)
> > + info->clk = NULL;
> > + else
> > + return ret;
> > + }
> > + if (info->clk) {
> > + ret = clk_prepare_enable(info->clk);
> > + if (ret)
> > + return ret;
> >
> > - info->clock_speed = clk_get_rate(info->clk) / 100;
> > - if (info->clock_speed < 10 || info->clock_speed > 48) {
> > - ret = -EINVAL;
> > - goto clk_disable;
> > + info->clock_speed = clk_get_rate(info->clk) / 100;
> > + if (info->clock_speed < 10 || info->clock_speed > 48) {
> > + ret = -EINVAL;
> > + goto clk_disable;
> > + }
> > }
> >
> > ret = ov7670_init_gpio(client, info);
> > --
> > 2.19.0
> >
Re: [PATCH 11/11] [media] marvell-ccic: provide a clock for the sensor
Hi Lubomir,
I love your patch! Yet something to improve:
[auto build test ERROR on linuxtv-media/master]
[also build test ERROR on v4.20-rc1 next-20181105]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system]
url:
https://github.com/0day-ci/linux/commits/Lubomir-Rintel/media-ov7670-hook-s_power-onto-v4l2-core/20181105-163336
base: git://linuxtv.org/media_tree.git master
config: ia64-allmodconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 8.1.0
reproduce:
wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=8.1.0 make.cross ARCH=ia64
All error/warnings (new ones prefixed by >>):
In file included from drivers/media/platform/marvell-ccic/cafe-driver.c:38:
>> drivers/media/platform/marvell-ccic/mcam-core.h:129:16: error: field
>> 'mclk_hw' has incomplete type
struct clk_hw mclk_hw;
^~~
--
In file included from drivers/media/platform/marvell-ccic/mmp-driver.c:30:
>> drivers/media/platform/marvell-ccic/mcam-core.h:129:16: error: field
>> 'mclk_hw' has incomplete type
struct clk_hw mclk_hw;
^~~
drivers/media/platform/marvell-ccic/mmp-driver.c: In function 'mmpcam_probe':
>> drivers/media/platform/marvell-ccic/mmp-driver.c:302:8: error: implicit
>> declaration of function 'of_clk_add_provider'; did you mean
>> 'of_clk_get_from_provider'? [-Werror=implicit-function-declaration]
ret = of_clk_add_provider(pdev->dev.of_node, of_clk_src_simple_get,
^~~
of_clk_get_from_provider
>> drivers/media/platform/marvell-ccic/mmp-driver.c:302:47: error:
>> 'of_clk_src_simple_get' undeclared (first use in this function); did you
>> mean 'ida_simple_get'?
ret = of_clk_add_provider(pdev->dev.of_node, of_clk_src_simple_get,
^
ida_simple_get
drivers/media/platform/marvell-ccic/mmp-driver.c:302:47: note: each
undeclared identifier is reported only once for each function it appears in
cc1: some warnings being treated as errors
--
In file included from drivers/media/platform/marvell-ccic/mcam-core.c:35:
>> drivers/media/platform/marvell-ccic/mcam-core.h:129:16: error: field
>> 'mclk_hw' has incomplete type
struct clk_hw mclk_hw;
^~~
In file included from include/linux/kernel.h:10,
from drivers/media/platform/marvell-ccic/mcam-core.c:9:
drivers/media/platform/marvell-ccic/mcam-core.c: In function 'mclk_prepare':
include/linux/kernel.h:997:32: error: dereferencing pointer to incomplete
type 'struct clk_hw'
BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) && \
^~
include/linux/compiler.h:353:9: note: in definition of macro
'__compiletime_assert'
if (!(condition)) \
^
include/linux/compiler.h:373:2: note: in expansion of macro
'_compiletime_assert'
_compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
^~~
include/linux/build_bug.h:45:37: note: in expansion of macro
'compiletime_assert'
#define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
^~
include/linux/kernel.h:997:2: note: in expansion of macro 'BUILD_BUG_ON_MSG'
BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) && \
^~~~
include/linux/kernel.h:997:20: note: in expansion of macro '__same_type'
BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) && \
^~~
drivers/media/platform/marvell-ccic/mcam-core.c:952:28: note: in expansion
of macro 'container_of'
struct mcam_camera *cam = container_of(hw, struct mcam_camera, mclk_hw);
^~~~
drivers/media/platform/marvell-ccic/mcam-core.c: At top level:
>> drivers/media/platform/marvell-ccic/mcam-core.c:1004:21: error: variable
>> 'mclk_ops' has initializer but incomplete type
static const struct clk_ops mclk_ops = {
^~~
>> drivers/media/platform/marvell-ccic/mcam-core.c:1005:3: error: 'const struct
>> clk_ops' has no member named 'prepare'
.prepare = mclk_prepare,
^~~
>> drivers/media/platform/marvell-ccic/mcam-core.c:1005:13: warning: excess
>> elements in struct initializer
.prepare = mclk
Re: [PATCH] [media] ov7670: make "xclk" clock optional
Hi Lubo,
On Mon, Nov 05, 2018 at 02:12:18PM +0100, Lubomir Rintel wrote:
> Hello,
>
> On Mon, 2018-11-05 at 11:58 +0100, jacopo mondi wrote:
> > Hi Lubomir,
> >+Sakari in Cc
> >
> > I just noticed this, and the patch is now in v4.20, but let me comment
> > anyway on this.
> >
> > On Thu, Oct 04, 2018 at 11:29:03PM +0200, Lubomir Rintel wrote:
> > > When the "xclk" clock was added, it was made mandatory. This broke the
> > > driver on an OLPC plaform which doesn't know such clock. Make it
> > > optional.
> > >
> >
> > I don't think this is correct. The sensor needs a clock to work.
> >
> > With this patch clock_speed which is used to calculate
> > the framerate is defaulted to 30MHz, crippling all the calculations if
> > that default value doesn't match what is actually installed on the
> > board.
>
> How come? I kept this:
>
> + info->clock_speed = clk_get_rate(info->clk) / 100;
Yes, but only if
if (info->clk) { }
if (!info->clk) the 'clock_speed' variable is defaulted to 30 at the
beginning of the probe routine. Am I missing something obvious here?
>
> >
> > If this patch breaks the OLPC, then might it be the DTS for said
> > device needs to be fixed instead of working around the issue here?
>
> No. Device tree is an ABI, and you can't just add mandatory properties.
>
Well, as I read the ov7670 bindings documentation:
Required Properties:
- compatible: should be "ovti,ov7670"
- clocks: reference to the xclk input clock.
- clock-names: should be "xclk".
It was mandatory already since the bindings have been first created:
bba582894a ("[media] ov7670: document device tree bindings")
And yes, bindings establishes an ABI we have not to break or make
incompatible with DTs created for an older version of the same binding,
but the DTs itself is free to change and we need to do so to update
it when required (to fix bugs, add new components, enable/disable them
etc).
> There's no DTS for OLPC XO-1 either; it's an OpenFirmware machine.
>
I thought OLPC was an ARM machine, that's why I mentioned DTS. Sorry
about this.
A quick read of the wikipedia page for "OpenFirmware" gives me back
that it a standardized firmware interface:
"Open Firmware allows the system to load platform-independent drivers
directly from the PCI card, improving compatibility".
I know nothing on this, and that's not the point, so I'll better stop
here and refrain to express how much the "loading platform-independent
(BINARY) drivers from the PCI card" scares me :p
> You'd need to update all machines in the wild which is not realistic.
Machines which have received a kernel update which includes the patch
that makes the clock for the sensor driver mandatory [1], will have their
board files updated by the same kernel update, with the proper clock
provider instantiated for that sensor.
That's what I would expect from a kernel update for those devices (or
any device in general..)
If this didn't happen, blame OLPC kernel maintainers :p
[1] 0a024d634cee ("[media] ov7670: get xclk"); which went in v4.12
> Alternatively, something else than DT could provide the clock. If this
> gets in, then the OLPC would work even without the xclk patch:
> https://lore.kernel.org/lkml/20181105073054.24407-12-lkund...@v3.sk/
That's what I meant, more or less.
If you don't have a DTS you have a board file, isn't it?
( arch/x86/platform/olpc/ maybe? )
The patch you linked here makes the video interface (the marvel-ccic
one) provide the clock source for the sensor:
+ clkdev_create(mcam->mclk, "xclk", "%d-%04x",
+ i2c_adapter_id(cam->i2c_adapter), ov7670_info.addr);
+
While I would expect the board file to do that, as that's where all
pieces gets put together, and it knows which clock source has to be
fed to the sensor depending on your hardware design. As I don't know
much of x86 or openfirmare, feel free to explain me why it is not
possible ;)
Anyway, my whole point is that the sensor needs a clock to work. With
your patch if it is not provided it gets defaulted (if I'm not
mis-reading the code) to a value that would break frame interval
calculations. This is what concerns me and I would prefer the driver
to fail probing quite nosily to make sure all its users (dts, board
files etc) gets updated.
>
> (I just got a kbuild failure message, so I'll surely be following up
> with a v2.)
>
> > Also, the DT bindings should be updated too if we decide this property
> > can be omitted. At this point, with a follow-up patch.
>
> Yes.
>
This would actually be an ABI change (one that would not break
retro-compatibility probably, but still...)
Thanks
j
> >
> > Thanks
>
> Cheers
> Lubo
>
> >j
> >
> > > Tested on a OLPC XO-1 laptop.
> > >
> > > Cc: sta...@vger.kernel.org # 4.11+
> > > Fixes: 0a024d634cee ("[media] ov7670: get xclk")
> > > Signed-off-by: Lubomir Rintel
> > > ---
> > > drivers/media/i2c/ov7670.c | 27 +--
> > > 1 file changed, 17 insertions(+), 10 deletions(-)
> > >
> > > diff
Re: [PATCH] [media] ov7670: make "xclk" clock optional
On Mon, 2018-11-05 at 15:22 +0100, jacopo mondi wrote:
> Hi Lubo,
>
> On Mon, Nov 05, 2018 at 02:12:18PM +0100, Lubomir Rintel wrote:
> > Hello,
> >
> > On Mon, 2018-11-05 at 11:58 +0100, jacopo mondi wrote:
> > > Hi Lubomir,
> > >+Sakari in Cc
> > >
> > > I just noticed this, and the patch is now in v4.20, but let me comment
> > > anyway on this.
> > >
> > > On Thu, Oct 04, 2018 at 11:29:03PM +0200, Lubomir Rintel wrote:
> > > > When the "xclk" clock was added, it was made mandatory. This broke the
> > > > driver on an OLPC plaform which doesn't know such clock. Make it
> > > > optional.
> > > >
> > >
> > > I don't think this is correct. The sensor needs a clock to work.
> > >
> > > With this patch clock_speed which is used to calculate
> > > the framerate is defaulted to 30MHz, crippling all the calculations if
> > > that default value doesn't match what is actually installed on the
> > > board.
> >
> > How come? I kept this:
> >
> > + info->clock_speed = clk_get_rate(info->clk) / 100;
>
> Yes, but only if
> if (info->clk) { }
>
> if (!info->clk) the 'clock_speed' variable is defaulted to 30 at the
> beginning of the probe routine. Am I missing something obvious here?
Maybe. Or I am.
I thought you care about the situation where you *have* the clk, and
thus you shouldn't be caring about the defaults?
> > > If this patch breaks the OLPC, then might it be the DTS for said
> > > device needs to be fixed instead of working around the issue here?
> >
> > No. Device tree is an ABI, and you can't just add mandatory properties.
> >
>
> Well, as I read the ov7670 bindings documentation:
>
> Required Properties:
> - compatible: should be "ovti,ov7670"
> - clocks: reference to the xclk input clock.
> - clock-names: should be "xclk".
>
> It was mandatory already since the bindings have been first created:
> bba582894a ("[media] ov7670: document device tree bindings")
>
> And yes, bindings establishes an ABI we have not to break or make
> incompatible with DTs created for an older version of the same binding,
> but the DTs itself is free to change and we need to do so to update
> it when required (to fix bugs, add new components, enable/disable them
> etc).
Ah, right, you're correct. No DTS ABI breakage there. I guess it would
be fine to revert my patch if we provide the xclk on the OLPC instead.
> > There's no DTS for OLPC XO-1 either; it's an OpenFirmware machine.
> >
>
> I thought OLPC was an ARM machine, that's why I mentioned DTS. Sorry
> about this.
Well, you're sort of right here. The XO-1.75 generation is ARM, the XO-
1 is x86. They both use devicetree provided by the firmware. However,
they predate FDT (or the definitions of bindings that are used here for
tht matter) and the trees are unfortunately quite incomplete. The
sensor is not there.
> A quick read of the wikipedia page for "OpenFirmware" gives me back
> that it a standardized firmware interface:
> "Open Firmware allows the system to load platform-independent drivers
> directly from the PCI card, improving compatibility".
>
> I know nothing on this, and that's not the point, so I'll better stop
> here and refrain to express how much the "loading platform-independent
> (BINARY) drivers from the PCI card" scares me :p
>
> > You'd need to update all machines in the wild which is not realistic.
>
> Machines which have received a kernel update which includes the patch
> that makes the clock for the sensor driver mandatory [1], will have their
> board files updated by the same kernel update, with the proper clock
> provider instantiated for that sensor.
>
> That's what I would expect from a kernel update for those devices (or
> any device in general..)
>
> If this didn't happen, blame OLPC kernel maintainers :p
>
> [1] 0a024d634cee ("[media] ov7670: get xclk"); which went in v4.12
>
> > Alternatively, something else than DT could provide the clock. If this
> > gets in, then the OLPC would work even without the xclk patch:
> > https://lore.kernel.org/lkml/20181105073054.24407-12-lkund...@v3.sk/
>
> That's what I meant, more or less.
>
> If you don't have a DTS you have a board file, isn't it?
> ( arch/x86/platform/olpc/ maybe? )
The device tree on XO-1 is not constructed from a FDT, it's gotten from
the OpenFirmware. See arch/x86/platform/olpc/olpc_dt.c
But as I said -- it's hopelessly incomplete and is not going to be of
much help here. If you're curious, I've recently uploaded /proc/device-
tree dumps here, since someone else asked:
https://people.freedesktop.org/~lkundrak/olpc/
> The patch you linked here makes the video interface (the marvel-ccic
> one) provide the clock source for the sensor:
>
> + clkdev_create(mcam->mclk, "xclk", "%d-%04x",
> + i2c_adapter_id(cam->i2c_adapter), ov7670_info.addr);
> +
>
> While I would expect the board file to do that, as that's where all
> pieces gets put together, and it knows which clock source has to be
> fed to the sensor depending on you
Re: [PATCH][staging-next] drivers: staging: cedrus: find ctx before dereferencing it ctx
On Fri, Nov 02, 2018 at 07:01:26PM +, Colin King wrote:
> From: Colin Ian King
>
> Currently if count is an invalid value the v4l2_info message will
> dereference a null ctx pointer to get the dev information. Fix
> this by finding ctx first and then checking for an invalid count,
> this way ctxt will be non-null hence avoiding the null pointer
> dereference.
>
> Detected by CoverityScan, CID#1475337 ("Explicit null dereferenced")
>
> Fixes: 50e761516f2b ("media: platform: Add Cedrus VPU decoder driver")
> Signed-off-by: Colin Ian King
Acked-by: Maxime Ripard
Thanks!
Maxime
--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
signature.asc
Description: PGP signature
Re: [PATCH] [media] ov7670: make "xclk" clock optional
Hi Lubo,
On Mon, Nov 05, 2018 at 04:22:15PM +0100, Lubomir Rintel wrote:
> On Mon, 2018-11-05 at 15:22 +0100, jacopo mondi wrote:
> > Hi Lubo,
> >
> > On Mon, Nov 05, 2018 at 02:12:18PM +0100, Lubomir Rintel wrote:
> > > Hello,
> > >
> > > On Mon, 2018-11-05 at 11:58 +0100, jacopo mondi wrote:
> > > > Hi Lubomir,
> > > >+Sakari in Cc
> > > >
> > > > I just noticed this, and the patch is now in v4.20, but let me comment
> > > > anyway on this.
> > > >
> > > > On Thu, Oct 04, 2018 at 11:29:03PM +0200, Lubomir Rintel wrote:
> > > > > When the "xclk" clock was added, it was made mandatory. This broke the
> > > > > driver on an OLPC plaform which doesn't know such clock. Make it
> > > > > optional.
> > > > >
> > > >
> > > > I don't think this is correct. The sensor needs a clock to work.
> > > >
> > > > With this patch clock_speed which is used to calculate
> > > > the framerate is defaulted to 30MHz, crippling all the calculations if
> > > > that default value doesn't match what is actually installed on the
> > > > board.
> > >
> > > How come? I kept this:
> > >
> > > + info->clock_speed = clk_get_rate(info->clk) / 100;
> >
> > Yes, but only if
> > if (info->clk) { }
> >
> > if (!info->clk) the 'clock_speed' variable is defaulted to 30 at the
> > beginning of the probe routine. Am I missing something obvious here?
>
> Maybe. Or I am.
>
> I thought you care about the situation where you *have* the clk, and
> thus you shouldn't be caring about the defaults?
>
I care about the fact that with this version, the clock speed might be
default to a totally random value making the driver malfunctioning. My
specific use case doesn't matter.
> > > > If this patch breaks the OLPC, then might it be the DTS for said
> > > > device needs to be fixed instead of working around the issue here?
> > >
> > > No. Device tree is an ABI, and you can't just add mandatory properties.
> > >
> >
> > Well, as I read the ov7670 bindings documentation:
> >
> > Required Properties:
> > - compatible: should be "ovti,ov7670"
> > - clocks: reference to the xclk input clock.
> > - clock-names: should be "xclk".
> >
> > It was mandatory already since the bindings have been first created:
> > bba582894a ("[media] ov7670: document device tree bindings")
> >
> > And yes, bindings establishes an ABI we have not to break or make
> > incompatible with DTs created for an older version of the same binding,
> > but the DTs itself is free to change and we need to do so to update
> > it when required (to fix bugs, add new components, enable/disable them
> > etc).
>
> Ah, right, you're correct. No DTS ABI breakage there. I guess it would
> be fine to revert my patch if we provide the xclk on the OLPC instead.
>
Thanks I feel that would be the right thing to do.
> > > There's no DTS for OLPC XO-1 either; it's an OpenFirmware machine.
> > >
> >
> > I thought OLPC was an ARM machine, that's why I mentioned DTS. Sorry
> > about this.
>
> Well, you're sort of right here. The XO-1.75 generation is ARM, the XO-
> 1 is x86. They both use devicetree provided by the firmware. However,
> they predate FDT (or the definitions of bindings that are used here for
> tht matter) and the trees are unfortunately quite incomplete. The
> sensor is not there.
Ouch. I see...
>
> > A quick read of the wikipedia page for "OpenFirmware" gives me back
> > that it a standardized firmware interface:
> > "Open Firmware allows the system to load platform-independent drivers
> > directly from the PCI card, improving compatibility".
> >
> > I know nothing on this, and that's not the point, so I'll better stop
> > here and refrain to express how much the "loading platform-independent
> > (BINARY) drivers from the PCI card" scares me :p
> >
> > > You'd need to update all machines in the wild which is not realistic.
> >
> > Machines which have received a kernel update which includes the patch
> > that makes the clock for the sensor driver mandatory [1], will have their
> > board files updated by the same kernel update, with the proper clock
> > provider instantiated for that sensor.
> >
> > That's what I would expect from a kernel update for those devices (or
> > any device in general..)
> >
> > If this didn't happen, blame OLPC kernel maintainers :p
> >
> > [1] 0a024d634cee ("[media] ov7670: get xclk"); which went in v4.12
> >
> > > Alternatively, something else than DT could provide the clock. If this
> > > gets in, then the OLPC would work even without the xclk patch:
> > > https://lore.kernel.org/lkml/20181105073054.24407-12-lkund...@v3.sk/
> >
> > That's what I meant, more or less.
> >
> > If you don't have a DTS you have a board file, isn't it?
> > ( arch/x86/platform/olpc/ maybe? )
>
> The device tree on XO-1 is not constructed from a FDT, it's gotten from
> the OpenFirmware. See arch/x86/platform/olpc/olpc_dt.c
>
Ouch^2
> But as I said -- it's hopelessly incomplete and is not going to be of
> much help here. If you're curious, I've recently uploaded /proc/dev
Re: [PATCH] [media] ov7670: make "xclk" clock optional
On Mon, 2018-11-05 at 18:15 +0100, jacopo mondi wrote:
> Hi Lubo,
>
> On Mon, Nov 05, 2018 at 04:22:15PM +0100, Lubomir Rintel wrote:
> > On Mon, 2018-11-05 at 15:22 +0100, jacopo mondi wrote:
> > > Hi Lubo,
> > >
> > > On Mon, Nov 05, 2018 at 02:12:18PM +0100, Lubomir Rintel wrote:
> > > > Hello,
> > > >
> > > > On Mon, 2018-11-05 at 11:58 +0100, jacopo mondi wrote:
> > > > > Hi Lubomir,
> > > > >+Sakari in Cc
> > > > >
> > > > > I just noticed this, and the patch is now in v4.20, but let me comment
> > > > > anyway on this.
> > > > >
> > > > > On Thu, Oct 04, 2018 at 11:29:03PM +0200, Lubomir Rintel wrote:
> > > > > > When the "xclk" clock was added, it was made mandatory. This broke
> > > > > > the
> > > > > > driver on an OLPC plaform which doesn't know such clock. Make it
> > > > > > optional.
> > > > > >
> > > > >
> > > > > I don't think this is correct. The sensor needs a clock to work.
> > > > >
> > > > > With this patch clock_speed which is used to calculate
> > > > > the framerate is defaulted to 30MHz, crippling all the calculations if
> > > > > that default value doesn't match what is actually installed on the
> > > > > board.
> > > >
> > > > How come? I kept this:
> > > >
> > > > + info->clock_speed = clk_get_rate(info->clk) / 100;
> > >
> > > Yes, but only if
> > > if (info->clk) { }
> > >
> > > if (!info->clk) the 'clock_speed' variable is defaulted to 30 at the
> > > beginning of the probe routine. Am I missing something obvious here?
> >
> > Maybe. Or I am.
> >
> > I thought you care about the situation where you *have* the clk, and
> > thus you shouldn't be caring about the defaults?
> >
>
> I care about the fact that with this version, the clock speed might be
> default to a totally random value making the driver malfunctioning. My
> specific use case doesn't matter.
>
> > > > > If this patch breaks the OLPC, then might it be the DTS for said
> > > > > device needs to be fixed instead of working around the issue here?
> > > >
> > > > No. Device tree is an ABI, and you can't just add mandatory properties.
> > > >
> > >
> > > Well, as I read the ov7670 bindings documentation:
> > >
> > > Required Properties:
> > > - compatible: should be "ovti,ov7670"
> > > - clocks: reference to the xclk input clock.
> > > - clock-names: should be "xclk".
> > >
> > > It was mandatory already since the bindings have been first created:
> > > bba582894a ("[media] ov7670: document device tree bindings")
> > >
> > > And yes, bindings establishes an ABI we have not to break or make
> > > incompatible with DTs created for an older version of the same binding,
> > > but the DTs itself is free to change and we need to do so to update
> > > it when required (to fix bugs, add new components, enable/disable them
> > > etc).
> >
> > Ah, right, you're correct. No DTS ABI breakage there. I guess it would
> > be fine to revert my patch if we provide the xclk on the OLPC instead.
> >
>
> Thanks I feel that would be the right thing to do.
>
> > > > There's no DTS for OLPC XO-1 either; it's an OpenFirmware machine.
> > > >
> > >
> > > I thought OLPC was an ARM machine, that's why I mentioned DTS. Sorry
> > > about this.
> >
> > Well, you're sort of right here. The XO-1.75 generation is ARM, the XO-
> > 1 is x86. They both use devicetree provided by the firmware. However,
> > they predate FDT (or the definitions of bindings that are used here for
> > tht matter) and the trees are unfortunately quite incomplete. The
> > sensor is not there.
>
> Ouch. I see...
>
> > > A quick read of the wikipedia page for "OpenFirmware" gives me back
> > > that it a standardized firmware interface:
> > > "Open Firmware allows the system to load platform-independent drivers
> > > directly from the PCI card, improving compatibility".
> > >
> > > I know nothing on this, and that's not the point, so I'll better stop
> > > here and refrain to express how much the "loading platform-independent
> > > (BINARY) drivers from the PCI card" scares me :p
> > >
> > > > You'd need to update all machines in the wild which is not realistic.
> > >
> > > Machines which have received a kernel update which includes the patch
> > > that makes the clock for the sensor driver mandatory [1], will have their
> > > board files updated by the same kernel update, with the proper clock
> > > provider instantiated for that sensor.
> > >
> > > That's what I would expect from a kernel update for those devices (or
> > > any device in general..)
> > >
> > > If this didn't happen, blame OLPC kernel maintainers :p
> > >
> > > [1] 0a024d634cee ("[media] ov7670: get xclk"); which went in v4.12
> > >
> > > > Alternatively, something else than DT could provide the clock. If this
> > > > gets in, then the OLPC would work even without the xclk patch:
> > > > https://lore.kernel.org/lkml/20181105073054.24407-12-lkund...@v3.sk/
> > >
> > > That's what I meant, more or less.
> > >
> > > If you don't have a DTS you have a boar
Re: [PATCH v4 1/4] dt-bindings: media: i2c: Add bindings for Maxim Integrated MAX9286
On Fri, Nov 02, 2018 at 03:47:20PM +, Kieran Bingham wrote:
> From: Laurent Pinchart
>
> The MAX9286 deserializes video data received on up to 4 Gigabit
> Multimedia Serial Links (GMSL) and outputs them on a CSI-2 port using up
> to 4 data lanes.
>
> Signed-off-by: Laurent Pinchart
> Signed-off-by: Jacopo Mondi
> Signed-off-by: Kieran Bingham
>
> ---
> v3:
> - Update binding descriptions
>
> v4:
> - Define the use of a CSI2 D-PHY
> - Rename pwdn-gpios to enable-gpios (with inverted polarity)
> - Remove clock-lanes mapping support
> - rewrap text blocks
> - Fix typos
> ---
> .../bindings/media/i2c/maxim,max9286.txt | 182 ++
> 1 file changed, 182 insertions(+)
> create mode 100644
> Documentation/devicetree/bindings/media/i2c/maxim,max9286.txt
>
> diff --git a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.txt
> b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.txt
> new file mode 100644
> index ..672f6a4d417d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.txt
> @@ -0,0 +1,182 @@
> +Maxim Integrated Quad GMSL Deserializer
> +---
> +
> +The MAX9286 deserializer receives video data on up to 4 Gigabit Multimedia
> +Serial Links (GMSL) and outputs them on a CSI-2 D-PHY port using up to 4 data
> +lanes.
> +
> +In addition to video data, the GMSL links carry a bidirectional control
> channel
> +that encapsulates I2C messages. The MAX9286 forwards all I2C traffic not
> +addressed to itself to the other side of the links, where a GMSL serializer
> +will output it on a local I2C bus. In the other direction all I2C traffic
> +received over GMSL by the MAX9286 is output on the local I2C bus.
> +
> +Required Properties:
> +
> +- compatible: Shall be "maxim,max9286"
> +- reg: I2C device address
> +
> +Optional Properties:
> +
> +- poc-supply: Regulator providing Power over Coax to the cameras
> +- enable-gpios: GPIO connected to the #PWDN pin with inverted polarity
> +
> +Required endpoint nodes:
> +---
> +
> +The connections to the MAX9286 GMSL and its endpoint nodes are modeled using
> +the OF graph bindings in accordance with the video interface bindings defined
> +in Documentation/devicetree/bindings/media/video-interfaces.txt.
> +
> +The following table lists the port number corresponding to each device port.
> +
> +PortDescription
> +
> +Port 0 GMSL Input 0
> +Port 1 GMSL Input 1
> +Port 2 GMSL Input 2
> +Port 3 GMSL Input 3
> +Port 4 CSI-2 Output
> +
> +Optional Endpoint Properties for GMSL Input Ports (Port [0-3]):
> +
> +- remote-endpoint: phandle to the remote GMSL source endpoint subnode in the
> + remote node port.
> +
> +Required Endpoint Properties for CSI-2 Output Port (Port 4):
> +
> +- remote-endpoint: phandle to the remote CSI-2 sink endpoint node.
> +- data-lanes: array of physical CSI-2 data lane indexes.
> +
> +Required i2c-mux nodes:
> +--
> +
> +Each GMSL link is modeled as a child bus of an i2c bus multiplexer/switch, in
> +accordance with bindings described in
> +Documentation/devicetree/bindings/i2c/i2c-mux.txt. The serializer device on
> the
> +remote end of the GMSL link shall be modelled as a child node of the
> +corresponding I2C bus.
> +
> +Required i2c child bus properties:
> +- all properties described as required i2c child bus nodes properties in
> + Documentation/devicetree/bindings/i2c/i2c-mux.txt.
> +
> +Example:
> +---
> +
> + gmsl-deserializer@2c {
> + compatible = "maxim,max9286";
> + reg = <0x2c>;
> + poc-supply = <&camera_poc_12v>;
> + enable-gpios = <&gpio 13 GPIO_ACTIVE_LOW>;
> +
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + i2c@0 {
> + #address-cells = <1>;
> + #size-cells = <0>;
It's better to not have a mixture of nodes at a level with and without
unit-addresses. So I'd move all the i2c nodes under an 'i2c-mux' node.
Rob
Re: [PATCH v4 2/4] dt-bindings: media: i2c: Add bindings for IMI RDACM20
On Fri, 2 Nov 2018 15:47:21 +, Kieran Bingham wrote: > From: Jacopo Mondi > > The IMI RDACM20 is a Gigabit Multimedia Serial Link (GMSL) camera > capable of transmitting video and I2C control messages on a coax cable > physical link for automotive applications. > > Document its device tree binding interface. > > Signed-off-by: Jacopo Mondi > Signed-off-by: Kieran Bingham > Reviewed-by: Laurent Pinchart > > --- > v2: > - Provide imi vendor prefix > - Fix minor spelling > > v3: > - update binding descriptions > --- > .../bindings/media/i2c/imi,rdacm20.txt| 65 +++ > .../devicetree/bindings/vendor-prefixes.txt | 1 + > 2 files changed, 66 insertions(+) > create mode 100644 > Documentation/devicetree/bindings/media/i2c/imi,rdacm20.txt > Reviewed-by: Rob Herring
Re: [PATCH v3 1/4] dt-bindings: adv748x: make data-lanes property mandatory for CSI-2 endpoints
On Mon, Nov 05, 2018 at 09:41:06AM +0100, jacopo mondi wrote: > Hi Niklas, > > On Fri, Nov 02, 2018 at 05:00:06PM +0100, Niklas Söderlund wrote: > > The CSI-2 transmitters can use a different number of lanes to transmit > > data. Make the data-lanes mandatory for the endpoints that describe the > > transmitters as no good default can be set to fallback on. > > > > Signed-off-by: Niklas Söderlund > > > > --- > > * Changes since v2 > > - Update paragraph according to Laurents comments. > > --- > > Documentation/devicetree/bindings/media/i2c/adv748x.txt | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/adv748x.txt > > b/Documentation/devicetree/bindings/media/i2c/adv748x.txt > > index 5dddc95f9cc46084..bffbabc879efd86c 100644 > > --- a/Documentation/devicetree/bindings/media/i2c/adv748x.txt > > +++ b/Documentation/devicetree/bindings/media/i2c/adv748x.txt > > @@ -48,7 +48,9 @@ are numbered as follows. > > TXA source 10 > > TXB source 11 > > > > -The digital output port nodes must contain at least one endpoint. > > +The digital output port nodes, when present, shall contain at least one > > +endpoint. Each of those endpoints shall contain the data-lanes property as > > +described in video-interfaces.txt. > > > > Ports are optional if they are not connected to anything at the hardware > > level. > > > > Re-vamping my ignored comment on v2, I still think you should list here the > accepted values for each TX as they're actually a property of the hw > device itself. > > Required endpoint properties: > - data-lanes: See "video-interfaces.txt" for description. The property > is mandatory for CSI-2 output endpoints and the accepted value > depends on which endpoint the property is applied to: > - TXA: accepted values are <1>, <2>, <4> > - TXB: accepted value is <1> +1 Rob
Re: i.MX6 MIPI-CSI2 OV5640 Camera testing on Mainline Linux
On Mon, Oct 29, 2018 at 8:49 AM Fabio Estevam wrote: > > Hi Adam, > > On Sun, Oct 28, 2018 at 3:58 PM Adam Ford wrote: > > > Does anyone know when the media branch get's merged into the mainline > > kernel? I assume we're in the merge window with 4.19 just having been > > released. Once these have been merged into the mainline, I'll go > > through and start requesting they get pulled into 4.19 and/or 4.14 > > if/when appropriate. > > This should happen in 4.20-rc1, which will probably be out next Sunday. I sent an e-mail to stable with a list of a variety of patches for the ov5640 to be applied to 4.19.y So far all looks pretty good, but I think I found on minor bug: If I attempt to change just the resolution, it doesn't take. Initial read media-ctl --get-v4l2 "'ov5640 2-0010':0" [fmt:UYVY8_2X8/640x480@1/30 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:full-range] Change resolution # media-ctl --set-v4l2 "'ov5640 2-0010':0[fmt:UYVY2X8/720x480 field:none]" Read it back # media-ctl --get-v4l2 "'ov5640 2-0010':0" [fmt:UYVY8_2X8/640x480@1/30 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:full-range] However, if I change the resolution AND the format to something other than UYVY2x8, the resolution changes. I can then change the format back to UYVY and capture and stream video at 1080p and 720x480. I can work around this, but I thought I'd mention it. I was trying to figure out a patch to apply to the mailing list myself, but I wasn't able to fix it quickly. adam adam
[PATCH v4 1/3] dt-bindings: media: add Amlogic Video Decoder Bindings
Add documentation for the meson vdec dts node.
Signed-off-by: Maxime Jourdan
Reviewed-by: Rob Herring
---
.../bindings/media/amlogic,vdec.txt | 71 +++
1 file changed, 71 insertions(+)
create mode 100644 Documentation/devicetree/bindings/media/amlogic,vdec.txt
diff --git a/Documentation/devicetree/bindings/media/amlogic,vdec.txt
b/Documentation/devicetree/bindings/media/amlogic,vdec.txt
new file mode 100644
index ..aabdd01bcf32
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/amlogic,vdec.txt
@@ -0,0 +1,71 @@
+Amlogic Video Decoder
+
+
+The video decoding IP lies within the DOS memory region,
+except for the hardware bitstream parser that makes use of an undocumented
+region.
+
+It makes use of the following blocks:
+
+- ESPARSER is a bitstream parser that outputs to a VIFIFO. Further VDEC blocks
+then feed from this VIFIFO.
+- VDEC_1 can decode MPEG-1, MPEG-2, MPEG-4 part 2, MJPEG, H.263, H.264, VC-1.
+- VDEC_HEVC can decode HEVC and VP9.
+
+Both VDEC_1 and VDEC_HEVC share the "vdec" IRQ and as such cannot run
+concurrently.
+
+Device Tree Bindings:
+-
+
+VDEC: Video Decoder
+--
+
+Required properties:
+- compatible: value should be different for each SoC family as :
+ - GXBB (S905) : "amlogic,gxbb-vdec"
+ - GXL (S905X, S905D) : "amlogic,gxl-vdec"
+ - GXM (S912) : "amlogic,gxm-vdec"
+- reg: base address and size of he following memory-mapped regions :
+ - dos
+ - esparser
+- reg-names: should contain the names of the previous memory regions
+- interrupts: should contain the following IRQs:
+ - vdec
+ - esparser
+- interrupt-names: should contain the names of the previous interrupts
+- amlogic,ao-sysctrl: should point to the AOBUS sysctrl node
+- amlogic,canvas: should point to a canvas provider node
+- clocks: should contain the following clocks :
+ - dos_parser
+ - dos
+ - vdec_1
+ - vdec_hevc
+- clock-names: should contain the names of the previous clocks
+- resets: should contain the parser reset
+- reset-names: should be "esparser"
+
+Example:
+
+vdec: video-decoder@c882 {
+ compatible = "amlogic,gxbb-vdec";
+ reg = <0x0 0xc882 0x0 0x1>,
+ <0x0 0xc110a580 0x0 0xe4>;
+ reg-names = "dos", "esparser";
+
+ interrupts = ,
+;
+ interrupt-names = "vdec", "esparser";
+
+ amlogic,ao-sysctrl = <&sysctrl_AO>;
+ amlogic,canvas = <&canvas>;
+
+ clocks = <&clkc CLKID_DOS_PARSER>,
+<&clkc CLKID_DOS>,
+<&clkc CLKID_VDEC_1>,
+<&clkc CLKID_VDEC_HEVC>;
+ clock-names = "dos_parser", "dos", "vdec_1", "vdec_hevc";
+
+ resets = <&reset RESET_PARSER>;
+ reset-names = "esparser";
+};
--
2.19.1
[PATCH v4 3/3] MAINTAINERS: Add meson video decoder
Add an entry for the meson video decoder for amlogic SoCs. Signed-off-by: Maxime Jourdan --- MAINTAINERS | 8 1 file changed, 8 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index f4855974f325..df013e7758b6 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -9621,6 +9621,14 @@ F: drivers/media/platform/meson/ao-cec.c F: Documentation/devicetree/bindings/media/meson-ao-cec.txt T: git git://linuxtv.org/media_tree.git +MESON VIDEO DECODER DRIVER FOR AMLOGIC SOCS +M: Maxime Jourdan +L: linux-me...@lists.freedesktop.org +L: linux-amlo...@lists.infradead.org +S: Supported +F: drivers/media/platform/meson/vdec/ +T: git git://linuxtv.org/media_tree.git + MICROBLAZE ARCHITECTURE M: Michal Simek W: http://www.monstr.eu/fdt/ -- 2.19.1
[PATCH v4 2/3] media: meson: add v4l2 m2m video decoder driver
Amlogic SoCs feature a powerful video decoder unit able to decode many formats, with a performance of usually up to 4k60. This is a driver for this IP that is based around the v4l2 m2m framework. It features decoding for: - MPEG 1 - MPEG 2 Supported SoCs are: GXBB (S905), GXL (S905X/W/D), GXM (S912) There is also a hardware bitstream parser (ESPARSER) that is handled here. Signed-off-by: Maxime Jourdan --- drivers/media/platform/Kconfig| 10 + drivers/media/platform/meson/Makefile |1 + drivers/media/platform/meson/vdec/Makefile|8 + .../media/platform/meson/vdec/codec_mpeg12.c | 209 .../media/platform/meson/vdec/codec_mpeg12.h | 14 + drivers/media/platform/meson/vdec/dos_regs.h | 98 ++ drivers/media/platform/meson/vdec/esparser.c | 322 + drivers/media/platform/meson/vdec/esparser.h | 32 + drivers/media/platform/meson/vdec/vdec.c | 1034 + drivers/media/platform/meson/vdec/vdec.h | 251 drivers/media/platform/meson/vdec/vdec_1.c| 231 drivers/media/platform/meson/vdec/vdec_1.h| 14 + .../media/platform/meson/vdec/vdec_helpers.c | 412 +++ .../media/platform/meson/vdec/vdec_helpers.h | 48 + .../media/platform/meson/vdec/vdec_platform.c | 101 ++ .../media/platform/meson/vdec/vdec_platform.h | 30 + 16 files changed, 2815 insertions(+) create mode 100644 drivers/media/platform/meson/vdec/Makefile create mode 100644 drivers/media/platform/meson/vdec/codec_mpeg12.c create mode 100644 drivers/media/platform/meson/vdec/codec_mpeg12.h create mode 100644 drivers/media/platform/meson/vdec/dos_regs.h create mode 100644 drivers/media/platform/meson/vdec/esparser.c create mode 100644 drivers/media/platform/meson/vdec/esparser.h create mode 100644 drivers/media/platform/meson/vdec/vdec.c create mode 100644 drivers/media/platform/meson/vdec/vdec.h create mode 100644 drivers/media/platform/meson/vdec/vdec_1.c create mode 100644 drivers/media/platform/meson/vdec/vdec_1.h create mode 100644 drivers/media/platform/meson/vdec/vdec_helpers.c create mode 100644 drivers/media/platform/meson/vdec/vdec_helpers.h create mode 100644 drivers/media/platform/meson/vdec/vdec_platform.c create mode 100644 drivers/media/platform/meson/vdec/vdec_platform.h diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig index 70c4f6c54881..619a9b57afc6 100644 --- a/drivers/media/platform/Kconfig +++ b/drivers/media/platform/Kconfig @@ -492,6 +492,16 @@ config VIDEO_QCOM_VENUS on various Qualcomm SoCs. To compile this driver as a module choose m here. +config VIDEO_MESON_VDEC + tristate "Amlogic video decoder driver" + depends on VIDEO_DEV && VIDEO_V4L2 && HAS_DMA + depends on ARCH_MESON || COMPILE_TEST + select VIDEOBUF2_DMA_CONTIG + select V4L2_MEM2MEM_DEV + select MESON_CANVAS + help + Support for the video decoder found in gxbb/gxl/gxm chips. + endif # V4L_MEM2MEM_DRIVERS # TI VIDEO PORT Helper Modules diff --git a/drivers/media/platform/meson/Makefile b/drivers/media/platform/meson/Makefile index 597beb8f34d1..f7c6e1031f25 100644 --- a/drivers/media/platform/meson/Makefile +++ b/drivers/media/platform/meson/Makefile @@ -1 +1,2 @@ obj-$(CONFIG_VIDEO_MESON_AO_CEC) += ao-cec.o +obj-$(CONFIG_VIDEO_MESON_VDEC) += vdec/ diff --git a/drivers/media/platform/meson/vdec/Makefile b/drivers/media/platform/meson/vdec/Makefile new file mode 100644 index ..6bea129084b7 --- /dev/null +++ b/drivers/media/platform/meson/vdec/Makefile @@ -0,0 +1,8 @@ +# SPDX-License-Identifier: GPL-2.0 +# Makefile for Amlogic meson video decoder driver + +meson-vdec-objs = esparser.o vdec.o vdec_helpers.o vdec_platform.o +meson-vdec-objs += vdec_1.o +meson-vdec-objs += codec_mpeg12.o + +obj-$(CONFIG_VIDEO_MESON_VDEC) += meson-vdec.o diff --git a/drivers/media/platform/meson/vdec/codec_mpeg12.c b/drivers/media/platform/meson/vdec/codec_mpeg12.c new file mode 100644 index ..1bd6fb7d531d --- /dev/null +++ b/drivers/media/platform/meson/vdec/codec_mpeg12.c @@ -0,0 +1,209 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Copyright (C) 2018 BayLibre, SAS + * Author: Maxime Jourdan + */ + +#include +#include + +#include "vdec_helpers.h" +#include "dos_regs.h" + +#define SIZE_WORKSPACE SZ_128K +/* Offset substracted by the firmware from the workspace paddr */ +#define WORKSPACE_OFFSET (5 * SZ_1K) + +/* map firmware registers to known MPEG1/2 functions */ +#define MREG_SEQ_INFO AV_SCRATCH_4 + #define MPEG2_SEQ_DAR_MASK GENMASK(3, 0) + #define MPEG2_DAR_4_3 2 + #define MPEG2_DAR_16_9 3 + #define MPEG2_DAR_221_100 4 +#define MREG_PIC_INFO AV_SCRATCH_5 +#define MREG_PIC_WIDTH AV_SCRATCH_6 +#define MREG_PIC_HEIGHTAV_SCRATCH_7 +#define MREG_BUFFERIN AV_SCRATCH_8 +#define MREG_BUFFEROUT AV_SCRAT
Re: [PATCH v7 05/16] intel-ipu3: abi: Add structs
Hi Yong,
On Mon, Oct 29, 2018 at 03:22:59PM -0700, Yong Zhi wrote:
> This add all the structs of IPU3 firmware ABI.
>
> Signed-off-by: Yong Zhi
> Signed-off-by: Rajmohan Mani
...
> +struct imgu_abi_shd_intra_frame_operations_data {
> + struct imgu_abi_acc_operation
> + operation_list[IMGU_ABI_SHD_MAX_OPERATIONS]
> __attribute__((aligned(32)));
> + struct imgu_abi_acc_process_lines_cmd_data
> + process_lines_data[IMGU_ABI_SHD_MAX_PROCESS_LINES]
> __attribute__((aligned(32)));
> + struct imgu_abi_shd_transfer_luts_set_data
> + transfer_data[IMGU_ABI_SHD_MAX_TRANSFERS]
> __attribute__((aligned(32)));
Could you replace this wth __aligned(32), please? The same for the rest of
the header.
--
Regards,
Sakari Ailus
sakari.ai...@linux.intel.com
Re: [PATCH v7 06/16] intel-ipu3: mmu: Implement driver
Hi Yong,
On Mon, Oct 29, 2018 at 03:23:00PM -0700, Yong Zhi wrote:
> From: Tomasz Figa
>
> This driver translates IO virtual address to physical
> address based on two levels page tables.
>
> Signed-off-by: Tomasz Figa
> Signed-off-by: Yong Zhi
> ---
...
> +static void call_if_ipu3_is_powered(struct ipu3_mmu *mmu,
> + void (*func)(struct ipu3_mmu *mmu))
> +{
> + pm_runtime_get_noresume(mmu->dev);
> + if (pm_runtime_active(mmu->dev))
> + func(mmu);
> + pm_runtime_put(mmu->dev);
How about:
if (!pm_runtime_get_if_in_use(mmu->dev))
return;
func(mmu);
pm_runtime_put(mmu->dev);
> +}
--
Sakari Ailus
sakari.ai...@linux.intel.com
VIDIOC_SUBSCRIBE_EVENT for V4L2_EVENT_CTRL regression?
Hi All I'm testing with 4.19 and finding that testEvents in v4l2-compliance is failing with ""failed to find event for control '%s' type %u", ie it hasn't got the event for the inital values. This is with the various BCM2835 drivers that I'm involved with. Having looked at the v4l2-core history I tried reverting "ad608fb media: v4l: event: Prevent freeing event subscriptions while accessed". The test passes again. Enabling all logging, and adding a couple of logging messages at the beginning and end of v4l2_ctrl_add_event and __v4l2_event_queue_fh error path, I get the following logs: [ 90.62] v4l2_ctrl_add_event: ctrl a2b86fa8 "User Controls" type 6, flags 0001 [ 90.630002] video0: VIDIOC_SUBSCRIBE_EVENT: type=0x3, id=0x980001, flags=0x1 [ 91.630166] videodev: v4l2_poll: video0: poll: [ 91.630311] videodev: v4l2_poll: video0: poll: [ 91.630325] video0: VIDIOC_UNSUBSCRIBE_EVENT: type=0x3, id=0x980001, flags=0x1 [ 91.630396] v4l2_ctrl_add_event: ctrl 8f6fcc61 "Brightness" type 1, flags 0001 [ 91.630403] __v4l2_event_queue_fh: Not subscribed to event type 3 id 0x980001 [ 91.630407] v4l2_ctrl_add_event: ctrl 8f6fcc61 "Brightness" type 1 - initial values queued [ 91.630409] video0: VIDIOC_SUBSCRIBE_EVENT: type=0x3, id=0x980900, flags=0x1 [ 92.630513] videodev: v4l2_poll: video0: poll: [ 92.630660] videodev: v4l2_poll: video0: poll: [ 92.630729] videodev: v4l2_release: video0: release So __v4l2_event_queue_fh is dropping the event as we aren't subscribed at the point that the initial values are queued. Sorry, I don't have any other devices that support subscribing for events to hand (uvcvideo passes the test as it reports unsupported). I don't have a media tree build immediately available either, but I can't see anything related to this in the recent history. I can go down that route if needed. v4l-utils repo was synced today - head at "f9881444e8 cec-compliance: wake-up on Active Source is warn for <2.0" Could someone test on other hardware to confirm that it's not the drivers I'm using? I'm fairly certain it isn't as that patch does call sev->ops->add(sev, elems); before list_add(&sev->list, &fh->subscribed), and that is guaranteed to fail if sev->ops->add immediately queues an event. Thanks Dave
Re: [RFC PATCH 00/11] Convert last remaining g/s_crop/cropcap drivers
Hi Sylwester, On 11/02/2018 05:16 PM, Sylwester Nawrocki wrote: > Hi Hans, > > On Fri, 5 Oct 2018 at 09:49, Hans Verkuil wrote: >> >> From: Hans Verkuil >> >> This patch series converts the last remaining drivers that use g/s_crop and >> cropcap to g/s_selection. > > Thank you for this clean up! I remember attempting conversion of those > remaining > drivers to selection API long time ago but I didn't have a good idea > then how to address > that crop and compose target inversion mess so I abandoned that efforts then. Thank you for the review. One question: have you also tested this with at least one of the affected drivers? I'd like to have at least one Tested-by line. Regards, Hans
Re: VIDIOC_SUBSCRIBE_EVENT for V4L2_EVENT_CTRL regression?
On 11/05/2018 01:21 PM, Dave Stevenson wrote: > Hi All > > I'm testing with 4.19 and finding that testEvents in v4l2-compliance > is failing with ""failed to find event for control '%s' type %u", ie > it hasn't got the event for the inital values. This is with the > various BCM2835 drivers that I'm involved with. > > Having looked at the v4l2-core history I tried reverting "ad608fb > media: v4l: event: Prevent freeing event subscriptions while > accessed". The test passes again. > > Enabling all logging, and adding a couple of logging messages at the > beginning and end of v4l2_ctrl_add_event and __v4l2_event_queue_fh > error path, I get the following logs: > > [ 90.62] v4l2_ctrl_add_event: ctrl a2b86fa8 "User Controls" type > 6, flags 0001 > [ 90.630002] video0: VIDIOC_SUBSCRIBE_EVENT: type=0x3, id=0x980001, > flags=0x1 > [ 91.630166] videodev: v4l2_poll: video0: poll: > [ 91.630311] videodev: v4l2_poll: video0: poll: > [ 91.630325] video0: VIDIOC_UNSUBSCRIBE_EVENT: type=0x3, > id=0x980001, flags=0x1 > [ 91.630396] v4l2_ctrl_add_event: ctrl 8f6fcc61 "Brightness" type 1, > flags 0001 > [ 91.630403] __v4l2_event_queue_fh: Not subscribed to event type 3 id > 0x980001 > [ 91.630407] v4l2_ctrl_add_event: ctrl 8f6fcc61 "Brightness" type 1 > - initial values queued > [ 91.630409] video0: VIDIOC_SUBSCRIBE_EVENT: type=0x3, id=0x980900, > flags=0x1 > [ 92.630513] videodev: v4l2_poll: video0: poll: > [ 92.630660] videodev: v4l2_poll: video0: poll: > [ 92.630729] videodev: v4l2_release: video0: release > > So __v4l2_event_queue_fh is dropping the event as we aren't subscribed > at the point that the initial values are queued. > > Sorry, I don't have any other devices that support subscribing for > events to hand (uvcvideo passes the test as it reports unsupported). I > don't have a media tree build immediately available either, but I > can't see anything related to this in the recent history. I can go > down that route if needed. > v4l-utils repo was synced today - head at "f9881444e8 cec-compliance: > wake-up on Active Source is warn for <2.0" > > Could someone test on other hardware to confirm that it's not the > drivers I'm using? I'm fairly certain it isn't as that patch does call > sev->ops->add(sev, elems); before list_add(&sev->list, > &fh->subscribed), and that is guaranteed to fail if sev->ops->add > immediately queues an event. Just to pitch in, I got similar issues when I tried to apply that commit to our Cisco code base. I've been away for a week and had no time to look into the cause, but it really appears that that commit was bad. Sakari, can you take a look at this? Regards, Hans
Re: VIDIOC_SUBSCRIBE_EVENT for V4L2_EVENT_CTRL regression?
On 11/05/2018 02:18 PM, Hans Verkuil wrote: > On 11/05/2018 01:21 PM, Dave Stevenson wrote: >> Hi All >> >> I'm testing with 4.19 and finding that testEvents in v4l2-compliance >> is failing with ""failed to find event for control '%s' type %u", ie >> it hasn't got the event for the inital values. This is with the >> various BCM2835 drivers that I'm involved with. >> >> Having looked at the v4l2-core history I tried reverting "ad608fb >> media: v4l: event: Prevent freeing event subscriptions while >> accessed". The test passes again. >> >> Enabling all logging, and adding a couple of logging messages at the >> beginning and end of v4l2_ctrl_add_event and __v4l2_event_queue_fh >> error path, I get the following logs: >> >> [ 90.62] v4l2_ctrl_add_event: ctrl a2b86fa8 "User Controls" type >> 6, flags 0001 >> [ 90.630002] video0: VIDIOC_SUBSCRIBE_EVENT: type=0x3, id=0x980001, >> flags=0x1 >> [ 91.630166] videodev: v4l2_poll: video0: poll: >> [ 91.630311] videodev: v4l2_poll: video0: poll: >> [ 91.630325] video0: VIDIOC_UNSUBSCRIBE_EVENT: type=0x3, >> id=0x980001, flags=0x1 >> [ 91.630396] v4l2_ctrl_add_event: ctrl 8f6fcc61 "Brightness" type 1, >> flags 0001 >> [ 91.630403] __v4l2_event_queue_fh: Not subscribed to event type 3 id >> 0x980001 >> [ 91.630407] v4l2_ctrl_add_event: ctrl 8f6fcc61 "Brightness" type 1 >> - initial values queued >> [ 91.630409] video0: VIDIOC_SUBSCRIBE_EVENT: type=0x3, id=0x980900, >> flags=0x1 >> [ 92.630513] videodev: v4l2_poll: video0: poll: >> [ 92.630660] videodev: v4l2_poll: video0: poll: >> [ 92.630729] videodev: v4l2_release: video0: release >> >> So __v4l2_event_queue_fh is dropping the event as we aren't subscribed >> at the point that the initial values are queued. >> >> Sorry, I don't have any other devices that support subscribing for >> events to hand (uvcvideo passes the test as it reports unsupported). I >> don't have a media tree build immediately available either, but I >> can't see anything related to this in the recent history. I can go >> down that route if needed. >> v4l-utils repo was synced today - head at "f9881444e8 cec-compliance: >> wake-up on Active Source is warn for <2.0" >> >> Could someone test on other hardware to confirm that it's not the >> drivers I'm using? I'm fairly certain it isn't as that patch does call >> sev->ops->add(sev, elems); before list_add(&sev->list, >> &fh->subscribed), and that is guaranteed to fail if sev->ops->add >> immediately queues an event. > > Just to pitch in, I got similar issues when I tried to apply that commit > to our Cisco code base. I've been away for a week and had no time to look > into the cause, but it really appears that that commit was bad. > > Sakari, can you take a look at this? Note: running v4l2-compliance with vivid (modprobe vivid no_error_inj=1) will fail with this patch: Control ioctls (Input 0): test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK test VIDIOC_QUERYCTRL: OK test VIDIOC_G/S_CTRL: OK test VIDIOC_G/S/TRY_EXT_CTRLS: OK fail: v4l2-test-controls.cpp(823): failed to find event for control 'Brightness' test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: FAIL test VIDIOC_G/S_JPEGCOMP: OK (Not Supported) Standard Controls: 15 Private Controls: 39 We really need regular vivid tests to find these regressions :-( Regards, Hans
Re: VIDIOC_SUBSCRIBE_EVENT for V4L2_EVENT_CTRL regression?
Hi Hans
On Mon, 5 Nov 2018 at 13:18, Hans Verkuil wrote:
>
> On 11/05/2018 01:21 PM, Dave Stevenson wrote:
> > Hi All
> >
> > I'm testing with 4.19 and finding that testEvents in v4l2-compliance
> > is failing with ""failed to find event for control '%s' type %u", ie
> > it hasn't got the event for the inital values. This is with the
> > various BCM2835 drivers that I'm involved with.
> >
> > Having looked at the v4l2-core history I tried reverting "ad608fb
> > media: v4l: event: Prevent freeing event subscriptions while
> > accessed". The test passes again.
> >
> > Enabling all logging, and adding a couple of logging messages at the
> > beginning and end of v4l2_ctrl_add_event and __v4l2_event_queue_fh
> > error path, I get the following logs:
> >
> > [ 90.62] v4l2_ctrl_add_event: ctrl a2b86fa8 "User Controls" type
> > 6, flags 0001
> > [ 90.630002] video0: VIDIOC_SUBSCRIBE_EVENT: type=0x3, id=0x980001,
> > flags=0x1
> > [ 91.630166] videodev: v4l2_poll: video0: poll:
> > [ 91.630311] videodev: v4l2_poll: video0: poll:
> > [ 91.630325] video0: VIDIOC_UNSUBSCRIBE_EVENT: type=0x3,
> > id=0x980001, flags=0x1
> > [ 91.630396] v4l2_ctrl_add_event: ctrl 8f6fcc61 "Brightness" type 1,
> > flags 0001
> > [ 91.630403] __v4l2_event_queue_fh: Not subscribed to event type 3 id
> > 0x980001
> > [ 91.630407] v4l2_ctrl_add_event: ctrl 8f6fcc61 "Brightness" type 1
> > - initial values queued
> > [ 91.630409] video0: VIDIOC_SUBSCRIBE_EVENT: type=0x3, id=0x980900,
> > flags=0x1
> > [ 92.630513] videodev: v4l2_poll: video0: poll:
> > [ 92.630660] videodev: v4l2_poll: video0: poll:
> > [ 92.630729] videodev: v4l2_release: video0: release
> >
> > So __v4l2_event_queue_fh is dropping the event as we aren't subscribed
> > at the point that the initial values are queued.
> >
> > Sorry, I don't have any other devices that support subscribing for
> > events to hand (uvcvideo passes the test as it reports unsupported). I
> > don't have a media tree build immediately available either, but I
> > can't see anything related to this in the recent history. I can go
> > down that route if needed.
> > v4l-utils repo was synced today - head at "f9881444e8 cec-compliance:
> > wake-up on Active Source is warn for <2.0"
> >
> > Could someone test on other hardware to confirm that it's not the
> > drivers I'm using? I'm fairly certain it isn't as that patch does call
> > sev->ops->add(sev, elems); before list_add(&sev->list,
> > &fh->subscribed), and that is guaranteed to fail if sev->ops->add
> > immediately queues an event.
>
> Just to pitch in, I got similar issues when I tried to apply that commit
> to our Cisco code base. I've been away for a week and had no time to look
> into the cause, but it really appears that that commit was bad.
>
> Sakari, can you take a look at this?
Thanks for confirming that it's not just me!
Swapping around the order of the list_add and ops->add fixes the
issue, but I'm not clear enough on whether there is a chance for
events to have been raised and need clearing to propose a full patch.
I'm currently running with:
diff --git a/drivers/media/v4l2-core/v4l2-event.c
b/drivers/media/v4l2-core/v4l2-event.c
index a3ef1f5..a997d2e 100644
--- a/drivers/media/v4l2-core/v4l2-event.c
+++ b/drivers/media/v4l2-core/v4l2-event.c
@@ -232,18 +234,20 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
goto out_unlock;
}
+ spin_lock_irqsave(&fh->vdev->fh_lock, flags);
+ list_add(&sev->list, &fh->subscribed);
+ spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
+
if (sev->ops && sev->ops->add) {
ret = sev->ops->add(sev, elems);
if (ret) {
+ spin_lock_irqsave(&fh->vdev->fh_lock, flags);
+ list_del(&sev->list);
+ spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
kvfree(sev);
- goto out_unlock;
}
}
- spin_lock_irqsave(&fh->vdev->fh_lock, flags);
- list_add(&sev->list, &fh->subscribed);
- spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
-
out_unlock:
mutex_unlock(&fh->subscribe_lock);
Is there a need to iterate the sev->events list and free any
potentially pending events there in the ops->add error path?
We still have the subscribe_lock held, so I don't think that it
reintroduces the issue that the patch was fixing of unsubscribing
before subscribed.
This patch has also been merged to stable, so 4.14 is affected as well.
Dave
Re: VIDIOC_SUBSCRIBE_EVENT for V4L2_EVENT_CTRL regression?
Hi Dave, Hans,
Thanks for reporting the issue!
On Mon, Nov 05, 2018 at 01:56:40PM +, Dave Stevenson wrote:
> Hi Hans
>
> On Mon, 5 Nov 2018 at 13:18, Hans Verkuil wrote:
> >
> > On 11/05/2018 01:21 PM, Dave Stevenson wrote:
> > > Hi All
> > >
> > > I'm testing with 4.19 and finding that testEvents in v4l2-compliance
> > > is failing with ""failed to find event for control '%s' type %u", ie
> > > it hasn't got the event for the inital values. This is with the
> > > various BCM2835 drivers that I'm involved with.
> > >
> > > Having looked at the v4l2-core history I tried reverting "ad608fb
> > > media: v4l: event: Prevent freeing event subscriptions while
> > > accessed". The test passes again.
> > >
> > > Enabling all logging, and adding a couple of logging messages at the
> > > beginning and end of v4l2_ctrl_add_event and __v4l2_event_queue_fh
> > > error path, I get the following logs:
> > >
> > > [ 90.62] v4l2_ctrl_add_event: ctrl a2b86fa8 "User Controls" type
> > > 6, flags 0001
> > > [ 90.630002] video0: VIDIOC_SUBSCRIBE_EVENT: type=0x3, id=0x980001,
> > > flags=0x1
> > > [ 91.630166] videodev: v4l2_poll: video0: poll:
> > > [ 91.630311] videodev: v4l2_poll: video0: poll:
> > > [ 91.630325] video0: VIDIOC_UNSUBSCRIBE_EVENT: type=0x3,
> > > id=0x980001, flags=0x1
> > > [ 91.630396] v4l2_ctrl_add_event: ctrl 8f6fcc61 "Brightness" type 1,
> > > flags 0001
> > > [ 91.630403] __v4l2_event_queue_fh: Not subscribed to event type 3 id
> > > 0x980001
> > > [ 91.630407] v4l2_ctrl_add_event: ctrl 8f6fcc61 "Brightness" type 1
> > > - initial values queued
> > > [ 91.630409] video0: VIDIOC_SUBSCRIBE_EVENT: type=0x3, id=0x980900,
> > > flags=0x1
> > > [ 92.630513] videodev: v4l2_poll: video0: poll:
> > > [ 92.630660] videodev: v4l2_poll: video0: poll:
> > > [ 92.630729] videodev: v4l2_release: video0: release
> > >
> > > So __v4l2_event_queue_fh is dropping the event as we aren't subscribed
> > > at the point that the initial values are queued.
> > >
> > > Sorry, I don't have any other devices that support subscribing for
> > > events to hand (uvcvideo passes the test as it reports unsupported). I
> > > don't have a media tree build immediately available either, but I
> > > can't see anything related to this in the recent history. I can go
> > > down that route if needed.
> > > v4l-utils repo was synced today - head at "f9881444e8 cec-compliance:
> > > wake-up on Active Source is warn for <2.0"
> > >
> > > Could someone test on other hardware to confirm that it's not the
> > > drivers I'm using? I'm fairly certain it isn't as that patch does call
> > > sev->ops->add(sev, elems); before list_add(&sev->list,
> > > &fh->subscribed), and that is guaranteed to fail if sev->ops->add
> > > immediately queues an event.
> >
> > Just to pitch in, I got similar issues when I tried to apply that commit
> > to our Cisco code base. I've been away for a week and had no time to look
> > into the cause, but it really appears that that commit was bad.
> >
> > Sakari, can you take a look at this?
Yes.
>
> Thanks for confirming that it's not just me!
>
> Swapping around the order of the list_add and ops->add fixes the
> issue, but I'm not clear enough on whether there is a chance for
> events to have been raised and need clearing to propose a full patch.
> I'm currently running with:
> diff --git a/drivers/media/v4l2-core/v4l2-event.c
> b/drivers/media/v4l2-core/v4l2-event.c
> index a3ef1f5..a997d2e 100644
> --- a/drivers/media/v4l2-core/v4l2-event.c
> +++ b/drivers/media/v4l2-core/v4l2-event.c
> @@ -232,18 +234,20 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
> goto out_unlock;
> }
>
> + spin_lock_irqsave(&fh->vdev->fh_lock, flags);
> + list_add(&sev->list, &fh->subscribed);
> + spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
> +
> if (sev->ops && sev->ops->add) {
> ret = sev->ops->add(sev, elems);
> if (ret) {
> + spin_lock_irqsave(&fh->vdev->fh_lock, flags);
> + list_del(&sev->list);
> + spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
> kvfree(sev);
> - goto out_unlock;
> }
> }
>
> - spin_lock_irqsave(&fh->vdev->fh_lock, flags);
> - list_add(&sev->list, &fh->subscribed);
> - spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
> -
> out_unlock:
> mutex_unlock(&fh->subscribe_lock);
>
> Is there a need to iterate the sev->events list and free any
> potentially pending events there in the ops->add error path?
> We still have the subscribe_lock held, so I don't think that it
> reintroduces the issue that the patch was fixing of unsubscribing
> before subscribed.
The events queued during that time need to be removed. Indeed the issue
appears to be that the add op is called before the event is added to the
list
Re: [PATCH] media: imx: csi: fix enum_mbus_code for unknown mbus format codes
Hi Steve, On Fri, 2018-02-09 at 17:43 -0800, Steve Longerbeam wrote: [...] > I *think* by implementing init_cfg in the CSI, it will prevent the > NULL deref in csi_enum_mbus_code(). However I think this patch > is a good idea in any case. Ack on both. Can we still get this patch applied? regards Philipp
[RFC] media: imx: queue subdevice events on the video device in the same pipeline
While subdevice and video device are in the same pipeline, pass
subdevice events on to userspace via the video device node.
Signed-off-by: Philipp Zabel
---
This would allow to see source change events from the source subdevice
on the video device node, for example.
---
drivers/staging/media/imx/imx-media-dev.c | 18 ++
1 file changed, 18 insertions(+)
diff --git a/drivers/staging/media/imx/imx-media-dev.c
b/drivers/staging/media/imx/imx-media-dev.c
index 4b344a4a3706..2fe6fdf2faf1 100644
--- a/drivers/staging/media/imx/imx-media-dev.c
+++ b/drivers/staging/media/imx/imx-media-dev.c
@@ -442,6 +442,23 @@ static const struct media_device_ops imx_media_md_ops = {
.link_notify = imx_media_link_notify,
};
+static void imx_media_notify(struct v4l2_subdev *sd, unsigned int notification,
+void *arg)
+{
+ struct imx_media_dev *imxmd;
+ struct imx_media_video_dev *vdev;
+
+ imxmd = container_of(sd->v4l2_dev, struct imx_media_dev, v4l2_dev);
+ list_for_each_entry(vdev, &imxmd->vdev_list, list) {
+ if (sd->entity.pipe &&
+ sd->entity.pipe == vdev->vfd->entity.pipe &&
+ notification == V4L2_DEVICE_NOTIFY_EVENT) {
+ v4l2_event_queue(vdev->vfd, arg);
+ break;
+ }
+ }
+}
+
static int imx_media_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
@@ -464,6 +481,7 @@ static int imx_media_probe(struct platform_device *pdev)
imxmd->v4l2_dev.mdev = &imxmd->md;
strscpy(imxmd->v4l2_dev.name, "imx-media",
sizeof(imxmd->v4l2_dev.name));
+ imxmd->v4l2_dev.notify = imx_media_notify;
media_device_init(&imxmd->md);
--
2.19.1
[PATCH 2/3] media: imx: set compose rectangle to mbus format
Prepare for mbus format being smaller than the written rectangle
due to burst size.
Signed-off-by: Philipp Zabel
---
drivers/staging/media/imx/imx-media-capture.c | 55 +--
1 file changed, 38 insertions(+), 17 deletions(-)
diff --git a/drivers/staging/media/imx/imx-media-capture.c
b/drivers/staging/media/imx/imx-media-capture.c
index cace8a51aca8..2d49d9573056 100644
--- a/drivers/staging/media/imx/imx-media-capture.c
+++ b/drivers/staging/media/imx/imx-media-capture.c
@@ -203,21 +203,14 @@ static int capture_g_fmt_vid_cap(struct file *file, void
*fh,
return 0;
}
-static int capture_try_fmt_vid_cap(struct file *file, void *fh,
- struct v4l2_format *f)
+static int __capture_try_fmt_vid_cap(struct capture_priv *priv,
+struct v4l2_subev_format *fmt_src,
+struct v4l2_format *f)
{
struct capture_priv *priv = video_drvdata(file);
- struct v4l2_subdev_format fmt_src;
const struct imx_media_pixfmt *cc, *cc_src;
- int ret;
-
- fmt_src.pad = priv->src_sd_pad;
- fmt_src.which = V4L2_SUBDEV_FORMAT_ACTIVE;
- ret = v4l2_subdev_call(priv->src_sd, pad, get_fmt, NULL, &fmt_src);
- if (ret)
- return ret;
- cc_src = imx_media_find_ipu_format(fmt_src.format.code, CS_SEL_ANY);
+ cc_src = imx_media_find_ipu_format(fmt_src->format.code, CS_SEL_ANY);
if (cc_src) {
u32 fourcc, cs_sel;
@@ -231,7 +224,7 @@ static int capture_try_fmt_vid_cap(struct file *file, void
*fh,
cc = imx_media_find_format(fourcc, cs_sel, false);
}
} else {
- cc_src = imx_media_find_mbus_format(fmt_src.format.code,
+ cc_src = imx_media_find_mbus_format(fmt_src->format.code,
CS_SEL_ANY, true);
if (WARN_ON(!cc_src))
return -EINVAL;
@@ -239,15 +232,32 @@ static int capture_try_fmt_vid_cap(struct file *file,
void *fh,
cc = cc_src;
}
- imx_media_mbus_fmt_to_pix_fmt(&f->fmt.pix, &fmt_src.format, cc);
+ imx_media_mbus_fmt_to_pix_fmt(&f->fmt.pix, &fmt_src->format, cc);
return 0;
}
+static int capture_try_fmt_vid_cap(struct file *file, void *fh,
+ struct v4l2_format *f)
+{
+ struct capture_priv *priv = video_drvdata(file);
+ struct v4l2_subdev_format fmt_src;
+ int ret;
+
+ fmt_src.pad = priv->src_sd_pad;
+ fmt_src.which = V4L2_SUBDEV_FORMAT_ACTIVE;
+ ret = v4l2_subdev_call(priv->src_sd, pad, get_fmt, NULL, &fmt_src);
+ if (ret)
+ return ret;
+
+ return __capture_try_fmt(priv, &fmt_src, f);
+}
+
static int capture_s_fmt_vid_cap(struct file *file, void *fh,
struct v4l2_format *f)
{
struct capture_priv *priv = video_drvdata(file);
+ struct v4l2_subdev_format fmt_src;
int ret;
if (vb2_is_busy(&priv->q)) {
@@ -255,7 +265,13 @@ static int capture_s_fmt_vid_cap(struct file *file, void
*fh,
return -EBUSY;
}
- ret = capture_try_fmt_vid_cap(file, priv, f);
+ fmt_src.pad = priv->src_sd_pad;
+ fmt_src.which = V4L2_SUBDEV_FORMAT_ACTIVE;
+ ret = v4l2_subdev_call(priv->src_sd, pad, get_fmt, NULL, &fmt_src);
+ if (ret)
+ return ret;
+
+ ret = __capture_try_fmt_vid_cap(priv, &fmt_src, f);
if (ret)
return ret;
@@ -264,8 +280,8 @@ static int capture_s_fmt_vid_cap(struct file *file, void
*fh,
CS_SEL_ANY, true);
priv->vdev.compose.left = 0;
priv->vdev.compose.top = 0;
- priv->vdev.compose.width = f->fmt.fmt.pix.width;
- priv->vdev.compose.height = f->fmt.fmt.pix.height;
+ priv->vdev.compose.width = fmt_src.width;
+ priv->vdev.compose.height = fmt_src.height;
return 0;
}
@@ -307,9 +323,14 @@ static int capture_g_selection(struct file *file, void *fh,
case V4L2_SEL_TGT_COMPOSE:
case V4L2_SEL_TGT_COMPOSE_DEFAULT:
case V4L2_SEL_TGT_COMPOSE_BOUNDS:
- case V4L2_SEL_TGT_COMPOSE_PADDED:
s->r = priv->vdev.compose;
break;
+ case V4L2_SEL_TGT_COMPOSE_PADDED:
+ s->r.left = 0;
+ s->r.top = 0;
+ s->r.width = priv->vdev.fmt.fmt.pix.width;
+ s->r.height = priv->vdev.fmt.fmt.pix.height;
+ break;
default:
return -EINVAL;
}
--
2.19.1
[PATCH 1/3] media: imx: add capture compose rectangle
Allowing to compose captured images into larger memory buffers
will let us lift alignment restrictions on CSI crop width.
Signed-off-by: Philipp Zabel
---
drivers/staging/media/imx/imx-ic-prpencvf.c | 3 +-
drivers/staging/media/imx/imx-media-capture.c | 38 +++
drivers/staging/media/imx/imx-media-csi.c | 3 +-
drivers/staging/media/imx/imx-media-vdic.c| 4 +-
drivers/staging/media/imx/imx-media.h | 2 +
5 files changed, 44 insertions(+), 6 deletions(-)
diff --git a/drivers/staging/media/imx/imx-ic-prpencvf.c
b/drivers/staging/media/imx/imx-ic-prpencvf.c
index 28f41caba05d..fe5a77baa592 100644
--- a/drivers/staging/media/imx/imx-ic-prpencvf.c
+++ b/drivers/staging/media/imx/imx-ic-prpencvf.c
@@ -366,8 +366,7 @@ static int prp_setup_channel(struct prp_priv *priv,
memset(&image, 0, sizeof(image));
image.pix = vdev->fmt.fmt.pix;
- image.rect.width = image.pix.width;
- image.rect.height = image.pix.height;
+ image.rect = vdev->compose;
if (rot_swap_width_height) {
swap(image.pix.width, image.pix.height);
diff --git a/drivers/staging/media/imx/imx-media-capture.c
b/drivers/staging/media/imx/imx-media-capture.c
index b37e1186eb2f..cace8a51aca8 100644
--- a/drivers/staging/media/imx/imx-media-capture.c
+++ b/drivers/staging/media/imx/imx-media-capture.c
@@ -262,6 +262,10 @@ static int capture_s_fmt_vid_cap(struct file *file, void
*fh,
priv->vdev.fmt.fmt.pix = f->fmt.pix;
priv->vdev.cc = imx_media_find_format(f->fmt.pix.pixelformat,
CS_SEL_ANY, true);
+ priv->vdev.compose.left = 0;
+ priv->vdev.compose.top = 0;
+ priv->vdev.compose.width = f->fmt.fmt.pix.width;
+ priv->vdev.compose.height = f->fmt.fmt.pix.height;
return 0;
}
@@ -290,6 +294,35 @@ static int capture_s_std(struct file *file, void *fh,
v4l2_std_id std)
return v4l2_subdev_call(priv->src_sd, video, s_std, std);
}
+static int capture_g_selection(struct file *file, void *fh,
+ struct v4l2_selection *s)
+{
+ struct capture_priv *priv = video_drvdata(file);
+
+ switch (s->target) {
+ case V4L2_SEL_TGT_CROP:
+ case V4L2_SEL_TGT_CROP_DEFAULT:
+ case V4L2_SEL_TGT_CROP_BOUNDS:
+ case V4L2_SEL_TGT_NATIVE_SIZE:
+ case V4L2_SEL_TGT_COMPOSE:
+ case V4L2_SEL_TGT_COMPOSE_DEFAULT:
+ case V4L2_SEL_TGT_COMPOSE_BOUNDS:
+ case V4L2_SEL_TGT_COMPOSE_PADDED:
+ s->r = priv->vdev.compose;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static int capture_s_selection(struct file *file, void *fh,
+ struct v4l2_selection *s)
+{
+ return capture_g_selection(file, fh, s);
+}
+
static int capture_g_parm(struct file *file, void *fh,
struct v4l2_streamparm *a)
{
@@ -350,6 +383,9 @@ static const struct v4l2_ioctl_ops capture_ioctl_ops = {
.vidioc_g_std = capture_g_std,
.vidioc_s_std = capture_s_std,
+ .vidioc_g_selection = capture_g_selection,
+ .vidioc_s_selection = capture_s_selection,
+
.vidioc_g_parm = capture_g_parm,
.vidioc_s_parm = capture_s_parm,
@@ -687,6 +723,8 @@ int imx_media_capture_device_register(struct
imx_media_video_dev *vdev)
vdev->fmt.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
imx_media_mbus_fmt_to_pix_fmt(&vdev->fmt.fmt.pix,
&fmt_src.format, NULL);
+ vdev->compose.width = fmt_src.format.width;
+ vdev->compose.height = fmt_src.format.height;
vdev->cc = imx_media_find_format(vdev->fmt.fmt.pix.pixelformat,
CS_SEL_ANY, false);
diff --git a/drivers/staging/media/imx/imx-media-csi.c
b/drivers/staging/media/imx/imx-media-csi.c
index 4223f8d418ae..c4523afe7b48 100644
--- a/drivers/staging/media/imx/imx-media-csi.c
+++ b/drivers/staging/media/imx/imx-media-csi.c
@@ -413,8 +413,7 @@ static int csi_idmac_setup_channel(struct csi_priv *priv)
memset(&image, 0, sizeof(image));
image.pix = vdev->fmt.fmt.pix;
- image.rect.width = image.pix.width;
- image.rect.height = image.pix.height;
+ image.rect = vdev->compose;
csi_idmac_setup_vb2_buf(priv, phys);
diff --git a/drivers/staging/media/imx/imx-media-vdic.c
b/drivers/staging/media/imx/imx-media-vdic.c
index 482250d47e7c..e08d296cf4eb 100644
--- a/drivers/staging/media/imx/imx-media-vdic.c
+++ b/drivers/staging/media/imx/imx-media-vdic.c
@@ -263,10 +263,10 @@ static int setup_vdi_channel(struct vdic_priv *priv,
memset(&image, 0, sizeof(image));
image.pix = vdev->fmt.fmt.pix;
+ image.rect = vdev->compose;
/* one field to VDIC channels */
image.pix.height /= 2;
- image.rect.width = image.pix.width;
-
[PATCH 3/3] media: imx: lift CSI width alignment restriction
The CSI subdevice shouldn't have to care about IDMAC line start
address alignment. With compose rectangle support in the capture
driver, it doesn't have to anymore.
Signed-off-by: Philipp Zabel
---
drivers/staging/media/imx/imx-media-capture.c | 9 -
drivers/staging/media/imx/imx-media-csi.c | 2 +-
drivers/staging/media/imx/imx-media-utils.c | 15 ---
3 files changed, 17 insertions(+), 9 deletions(-)
diff --git a/drivers/staging/media/imx/imx-media-capture.c
b/drivers/staging/media/imx/imx-media-capture.c
index 2d49d9573056..f87d6e8019e5 100644
--- a/drivers/staging/media/imx/imx-media-capture.c
+++ b/drivers/staging/media/imx/imx-media-capture.c
@@ -204,10 +204,9 @@ static int capture_g_fmt_vid_cap(struct file *file, void
*fh,
}
static int __capture_try_fmt_vid_cap(struct capture_priv *priv,
-struct v4l2_subev_format *fmt_src,
+struct v4l2_subdev_format *fmt_src,
struct v4l2_format *f)
{
- struct capture_priv *priv = video_drvdata(file);
const struct imx_media_pixfmt *cc, *cc_src;
cc_src = imx_media_find_ipu_format(fmt_src->format.code, CS_SEL_ANY);
@@ -250,7 +249,7 @@ static int capture_try_fmt_vid_cap(struct file *file, void
*fh,
if (ret)
return ret;
- return __capture_try_fmt(priv, &fmt_src, f);
+ return __capture_try_fmt_vid_cap(priv, &fmt_src, f);
}
static int capture_s_fmt_vid_cap(struct file *file, void *fh,
@@ -280,8 +279,8 @@ static int capture_s_fmt_vid_cap(struct file *file, void
*fh,
CS_SEL_ANY, true);
priv->vdev.compose.left = 0;
priv->vdev.compose.top = 0;
- priv->vdev.compose.width = fmt_src.width;
- priv->vdev.compose.height = fmt_src.height;
+ priv->vdev.compose.width = fmt_src.format.width;
+ priv->vdev.compose.height = fmt_src.format.height;
return 0;
}
diff --git a/drivers/staging/media/imx/imx-media-csi.c
b/drivers/staging/media/imx/imx-media-csi.c
index c4523afe7b48..d39682192a67 100644
--- a/drivers/staging/media/imx/imx-media-csi.c
+++ b/drivers/staging/media/imx/imx-media-csi.c
@@ -41,7 +41,7 @@
#define MIN_H 144
#define MAX_W 4096
#define MAX_H 4096
-#define W_ALIGN4 /* multiple of 16 pixels */
+#define W_ALIGN1 /* multiple of 2 pixels */
#define H_ALIGN1 /* multiple of 2 lines */
#define S_ALIGN1 /* multiple of 2 */
diff --git a/drivers/staging/media/imx/imx-media-utils.c
b/drivers/staging/media/imx/imx-media-utils.c
index 0eaa353d5cb3..5f110d90a4ef 100644
--- a/drivers/staging/media/imx/imx-media-utils.c
+++ b/drivers/staging/media/imx/imx-media-utils.c
@@ -580,6 +580,7 @@ int imx_media_mbus_fmt_to_pix_fmt(struct v4l2_pix_format
*pix,
struct v4l2_mbus_framefmt *mbus,
const struct imx_media_pixfmt *cc)
{
+ u32 width;
u32 stride;
if (!cc) {
@@ -602,9 +603,16 @@ int imx_media_mbus_fmt_to_pix_fmt(struct v4l2_pix_format
*pix,
cc = imx_media_find_mbus_format(code, CS_SEL_YUV, false);
}
- stride = cc->planar ? mbus->width : (mbus->width * cc->bpp) >> 3;
+ /* Round up width for minimum burst size */
+ width = round_up(mbus->width, 8);
- pix->width = mbus->width;
+ /* Round up stride for IDMAC line start address alignment */
+ if (cc->planar)
+ stride = round_up(width, 16);
+ else
+ stride = round_up((width * cc->bpp) >> 3, 8);
+
+ pix->width = width;
pix->height = mbus->height;
pix->pixelformat = cc->fourcc;
pix->colorspace = mbus->colorspace;
@@ -613,7 +621,8 @@ int imx_media_mbus_fmt_to_pix_fmt(struct v4l2_pix_format
*pix,
pix->quantization = mbus->quantization;
pix->field = mbus->field;
pix->bytesperline = stride;
- pix->sizeimage = (pix->width * pix->height * cc->bpp) >> 3;
+ pix->sizeimage = cc->planar ? ((stride * pix->height * cc->bpp) >> 3) :
+stride * pix->height;
return 0;
}
--
2.19.1
[PATCH 01/15] media: coda: fix memory corruption in case more than 32 instances are opened
The ffz() return value is undefined if the instance mask does not
contain any zeros. If it returned 32, the following set_bit would
corrupt the debugfs_root pointer.
Switch to IDA for context index allocation. This also removes the
artificial 32 instance limit for all except CodaDx6.
Signed-off-by: Philipp Zabel
---
drivers/media/platform/coda/coda-common.c | 25 ---
drivers/media/platform/coda/coda.h| 2 +-
2 files changed, 10 insertions(+), 17 deletions(-)
diff --git a/drivers/media/platform/coda/coda-common.c
b/drivers/media/platform/coda/coda-common.c
index 2848ea5f464d..cbb59c2f3a82 100644
--- a/drivers/media/platform/coda/coda-common.c
+++ b/drivers/media/platform/coda/coda-common.c
@@ -2099,17 +2099,6 @@ int coda_decoder_queue_init(void *priv, struct vb2_queue
*src_vq,
return coda_queue_init(priv, dst_vq);
}
-static int coda_next_free_instance(struct coda_dev *dev)
-{
- int idx = ffz(dev->instance_mask);
-
- if ((idx < 0) ||
- (dev->devtype->product == CODA_DX6 && idx > CODADX6_MAX_INSTANCES))
- return -EBUSY;
-
- return idx;
-}
-
/*
* File operations
*/
@@ -2118,7 +2107,8 @@ static int coda_open(struct file *file)
{
struct video_device *vdev = video_devdata(file);
struct coda_dev *dev = video_get_drvdata(vdev);
- struct coda_ctx *ctx = NULL;
+ struct coda_ctx *ctx;
+ unsigned int max = ~0;
char *name;
int ret;
int idx;
@@ -2127,12 +2117,13 @@ static int coda_open(struct file *file)
if (!ctx)
return -ENOMEM;
- idx = coda_next_free_instance(dev);
+ if (dev->devtype->product == CODA_DX6)
+ max = CODADX6_MAX_INSTANCES - 1;
+ idx = ida_alloc_max(&dev->ida, max, GFP_KERNEL);
if (idx < 0) {
ret = idx;
goto err_coda_max;
}
- set_bit(idx, &dev->instance_mask);
name = kasprintf(GFP_KERNEL, "context%d", idx);
if (!name) {
@@ -2241,8 +2232,8 @@ static int coda_open(struct file *file)
err_pm_get:
v4l2_fh_del(&ctx->fh);
v4l2_fh_exit(&ctx->fh);
- clear_bit(ctx->idx, &dev->instance_mask);
err_coda_name_init:
+ ida_free(&dev->ida, ctx->idx);
err_coda_max:
kfree(ctx);
return ret;
@@ -2284,7 +2275,7 @@ static int coda_release(struct file *file)
pm_runtime_put_sync(&dev->plat_dev->dev);
v4l2_fh_del(&ctx->fh);
v4l2_fh_exit(&ctx->fh);
- clear_bit(ctx->idx, &dev->instance_mask);
+ ida_free(&dev->ida, ctx->idx);
if (ctx->ops->release)
ctx->ops->release(ctx);
debugfs_remove_recursive(ctx->debugfs_entry);
@@ -2745,6 +2736,7 @@ static int coda_probe(struct platform_device *pdev)
mutex_init(&dev->dev_mutex);
mutex_init(&dev->coda_mutex);
+ ida_init(&dev->ida);
dev->debugfs_root = debugfs_create_dir("coda", NULL);
if (!dev->debugfs_root)
@@ -2832,6 +2824,7 @@ static int coda_remove(struct platform_device *pdev)
coda_free_aux_buf(dev, &dev->tempbuf);
coda_free_aux_buf(dev, &dev->workbuf);
debugfs_remove_recursive(dev->debugfs_root);
+ ida_destroy(&dev->ida);
return 0;
}
diff --git a/drivers/media/platform/coda/coda.h
b/drivers/media/platform/coda/coda.h
index 19ac0b9dc6eb..b6cd14ee91ea 100644
--- a/drivers/media/platform/coda/coda.h
+++ b/drivers/media/platform/coda/coda.h
@@ -95,7 +95,7 @@ struct coda_dev {
struct workqueue_struct *workqueue;
struct v4l2_m2m_dev *m2m_dev;
struct list_headinstances;
- unsigned long instance_mask;
+ struct ida ida;
struct dentry *debugfs_root;
};
--
2.19.1
[PATCH 09/15] media: coda: implement ENUM_FRAMEINTERVALS
v4l2-compliance complains about S_PARM being supported, but not
ENUM_FRAMEINTERVALS.
Report a continuous frame interval even though the hardware only
supports 16-bit numerator and denominator, with min/max values
that can be programmed into the mailbox registers.
Signed-off-by: Philipp Zabel
---
drivers/media/platform/coda/coda-common.c | 34 +++
1 file changed, 34 insertions(+)
diff --git a/drivers/media/platform/coda/coda-common.c
b/drivers/media/platform/coda/coda-common.c
index 1ba3301b35de..32998da39cac 100644
--- a/drivers/media/platform/coda/coda-common.c
+++ b/drivers/media/platform/coda/coda-common.c
@@ -1044,6 +1044,38 @@ static int coda_decoder_cmd(struct file *file, void *fh,
return 0;
}
+static int coda_enum_frameintervals(struct file *file, void *fh,
+ struct v4l2_frmivalenum *f)
+{
+ struct coda_ctx *ctx = fh_to_ctx(fh);
+ int i;
+
+ if (f->index)
+ return -EINVAL;
+
+ /* Disallow YUYV if the vdoa is not available */
+ if (!ctx->vdoa && f->pixel_format == V4L2_PIX_FMT_YUYV)
+ return -EINVAL;
+
+ for (i = 0; i < CODA_MAX_FORMATS; i++) {
+ if (f->pixel_format == ctx->cvd->src_formats[i] ||
+ f->pixel_format == ctx->cvd->dst_formats[i])
+ break;
+ }
+ if (i == CODA_MAX_FORMATS)
+ return -EINVAL;
+
+ f->type = V4L2_FRMIVAL_TYPE_CONTINUOUS;
+ f->stepwise.min.numerator = 1;
+ f->stepwise.min.denominator = 65535;
+ f->stepwise.max.numerator = 65536;
+ f->stepwise.max.denominator = 1;
+ f->stepwise.step.numerator = 1;
+ f->stepwise.step.denominator = 1;
+
+ return 0;
+}
+
static int coda_g_parm(struct file *file, void *fh, struct v4l2_streamparm *a)
{
struct coda_ctx *ctx = fh_to_ctx(fh);
@@ -1190,6 +1222,8 @@ static const struct v4l2_ioctl_ops coda_ioctl_ops = {
.vidioc_g_parm = coda_g_parm,
.vidioc_s_parm = coda_s_parm,
+ .vidioc_enum_frameintervals = coda_enum_frameintervals,
+
.vidioc_subscribe_event = coda_subscribe_event,
.vidioc_unsubscribe_event = v4l2_event_unsubscribe,
};
--
2.19.1
[PATCH 12/15] media: coda: print SEQ_INIT error code as hex value
From: Michael Tretter
The error code looks much more like a bit field than an error value.
Print it as hex rather than decimal.
Signed-off-by: Michael Tretter
Signed-off-by: Philipp Zabel
---
drivers/media/platform/coda/coda-bit.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/platform/coda/coda-bit.c
b/drivers/media/platform/coda/coda-bit.c
index 348b17140715..53f1a83e72a9 100644
--- a/drivers/media/platform/coda/coda-bit.c
+++ b/drivers/media/platform/coda/coda-bit.c
@@ -1748,7 +1748,7 @@ static int __coda_start_decoding(struct coda_ctx *ctx)
if (coda_read(dev, CODA_RET_DEC_SEQ_SUCCESS) == 0) {
v4l2_err(&dev->v4l2_dev,
- "CODA_COMMAND_SEQ_INIT failed, error code = %d\n",
+ "CODA_COMMAND_SEQ_INIT failed, error code = 0x%x\n",
coda_read(dev, CODA_RET_DEC_SEQ_ERR_REASON));
return -EAGAIN;
}
--
2.19.1
[PATCH 14/15] media: coda: normalise debug output
Consistently add the context index to debug output, which otherwise is
impossible to make sense of when two contexts are running concurrently.
For this purpose, add a convenience macro coda_dbg(). Use the function
name with the coda_ prefix stripped as keyword where applicable, and
consistently use vid-out and vid-cap names for the queues. Add sequence
counters to the decoder job finished message and correctly indicate B
frames. Add a start streaming message to complement the stop streaming
message and a start encoding message to complement the existing start
decoding message.
Signed-off-by: Philipp Zabel
---
drivers/media/platform/coda/coda-bit.c| 55 +++-
drivers/media/platform/coda/coda-common.c | 79 ++-
drivers/media/platform/coda/coda.h| 7 ++
3 files changed, 65 insertions(+), 76 deletions(-)
diff --git a/drivers/media/platform/coda/coda-bit.c
b/drivers/media/platform/coda/coda-bit.c
index 53f1a83e72a9..f2c0aa261c9b 100644
--- a/drivers/media/platform/coda/coda-bit.c
+++ b/drivers/media/platform/coda/coda-bit.c
@@ -725,8 +725,7 @@ static void coda_setup_iram(struct coda_ctx *ctx)
out:
if (!(iram_info->axi_sram_use & CODA7_USE_HOST_IP_ENABLE))
- v4l2_dbg(1, coda_debug, &ctx->dev->v4l2_dev,
-"IRAM smaller than needed\n");
+ coda_dbg(1, ctx, "IRAM smaller than needed\n");
if (dev->devtype->product == CODA_HX4 ||
dev->devtype->product == CODA_7541) {
@@ -1213,6 +1212,12 @@ static int coda_start_encoding(struct coda_ctx *ctx)
goto out;
}
+ coda_dbg(1, ctx, "start encoding %dx%d %4.4s->%4.4s @ %d/%d Hz\n",
+q_data_src->rect.width, q_data_src->rect.height,
+(char *)&ctx->codec->src_fourcc, (char *)&dst_fourcc,
+ctx->params.framerate & 0x,
+(ctx->params.framerate >> 16) + 1);
+
/* Save stream headers */
buf = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx);
switch (dst_fourcc) {
@@ -1474,8 +1479,7 @@ static void coda_finish_encode(struct coda_ctx *ctx)
vb2_set_plane_payload(&dst_buf->vb2_buf, 0, wr_ptr - start_ptr);
}
- v4l2_dbg(1, coda_debug, &ctx->dev->v4l2_dev, "frame size = %u\n",
-wr_ptr - start_ptr);
+ coda_dbg(1, ctx, "frame size = %u\n", wr_ptr - start_ptr);
coda_read(dev, CODA_RET_ENC_PIC_SLICE_NUM);
coda_read(dev, CODA_RET_ENC_PIC_FLAG);
@@ -1504,11 +1508,9 @@ static void coda_finish_encode(struct coda_ctx *ctx)
if (ctx->gopcounter < 0)
ctx->gopcounter = ctx->params.gop_size - 1;
- v4l2_dbg(1, coda_debug, &dev->v4l2_dev,
- "job finished: encoding frame (%d) (%s)\n",
- dst_buf->sequence,
- (dst_buf->flags & V4L2_BUF_FLAG_KEYFRAME) ?
- "KEYFRAME" : "PFRAME");
+ coda_dbg(1, ctx, "job finished: encoded %c frame (%d)\n",
+(dst_buf->flags & V4L2_BUF_FLAG_KEYFRAME) ? 'I' : 'P',
+dst_buf->sequence);
}
static void coda_seq_end_work(struct work_struct *work)
@@ -1522,9 +1524,7 @@ static void coda_seq_end_work(struct work_struct *work)
if (ctx->initialized == 0)
goto out;
- v4l2_dbg(1, coda_debug, &dev->v4l2_dev,
-"%d: %s: sent command 'SEQ_END' to coda\n", ctx->idx,
-__func__);
+ coda_dbg(1, ctx, "%s: sent command 'SEQ_END' to coda\n", __func__);
if (coda_command_sync(ctx, CODA_COMMAND_SEQ_END)) {
v4l2_err(&dev->v4l2_dev,
"CODA_COMMAND_SEQ_END failed\n");
@@ -1667,8 +1667,7 @@ static int __coda_start_decoding(struct coda_ctx *ctx)
u32 val;
int ret;
- v4l2_dbg(1, coda_debug, &dev->v4l2_dev,
-"Video Data Order Adapter: %s\n",
+ coda_dbg(1, ctx, "Video Data Order Adapter: %s\n",
ctx->use_vdoa ? "Enabled" : "Disabled");
/* Start decoding */
@@ -1772,8 +1771,7 @@ static int __coda_start_decoding(struct coda_ctx *ctx)
width = round_up(width, 16);
height = round_up(height, 16);
- v4l2_dbg(1, coda_debug, &dev->v4l2_dev, "%s instance %d now: %dx%d\n",
-__func__, ctx->idx, width, height);
+ coda_dbg(1, ctx, "start decoding: %dx%d\n", width, height);
ctx->num_internal_frames = coda_read(dev, CODA_RET_DEC_SEQ_FRAME_NEED);
/*
@@ -1904,8 +1902,7 @@ static int coda_prepare_decode(struct coda_ctx *ctx)
if (coda_get_bitstream_payload(ctx) < 512 &&
(!(ctx->bit_stream_param & CODA_BIT_STREAM_END_FLAG))) {
- v4l2_dbg(1, coda_debug, &dev->v4l2_dev,
-"bitstream payload: %d, skipping\n",
+ coda_dbg(1, ctx, "bitstream payload: %d, skipping\n",
coda_get_bitstream_payload(ctx));
v4l2_m2m_job_finish(ctx->dev->m2m_dev, ctx->fh.
[PATCH 08/15] media: coda: set V4L2_CAP_TIMEPERFRAME flag in coda_s_parm
The flag is already set in coda_g_parm, but v4l2-compliance complains about it not being set during S_PARM. Signed-off-by: Philipp Zabel --- drivers/media/platform/coda/coda-common.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/media/platform/coda/coda-common.c b/drivers/media/platform/coda/coda-common.c index 60b866160094..1ba3301b35de 100644 --- a/drivers/media/platform/coda/coda-common.c +++ b/drivers/media/platform/coda/coda-common.c @@ -1135,6 +1135,7 @@ static int coda_s_parm(struct file *file, void *fh, struct v4l2_streamparm *a) if (a->type != V4L2_BUF_TYPE_VIDEO_OUTPUT) return -EINVAL; + a->parm.output.capability = V4L2_CAP_TIMEPERFRAME; tpf = &a->parm.output.timeperframe; coda_approximate_timeperframe(tpf); ctx->params.framerate = coda_timeperframe_to_frate(tpf); -- 2.19.1
[PATCH 02/15] media: coda: store unmasked fifo position in meta
Storing the unmasked kfifo->in position as meta->start and ->end allows
to more easily compare a point past meta->end with the current
kfifo->in.
Signed-off-by: Philipp Zabel
---
drivers/media/platform/coda/coda-bit.c| 9 +++--
drivers/media/platform/coda/coda-common.c | 1 -
drivers/media/platform/coda/coda.h| 4 ++--
drivers/media/platform/coda/trace.h | 10 ++
4 files changed, 11 insertions(+), 13 deletions(-)
diff --git a/drivers/media/platform/coda/coda-bit.c
b/drivers/media/platform/coda/coda-bit.c
index d26c2d85a009..e5ce0bec8ec3 100644
--- a/drivers/media/platform/coda/coda-bit.c
+++ b/drivers/media/platform/coda/coda-bit.c
@@ -299,8 +299,7 @@ void coda_fill_bitstream(struct coda_ctx *ctx, struct
list_head *buffer_list)
}
/* Buffer start position */
- start = ctx->bitstream_fifo.kfifo.in &
- ctx->bitstream_fifo.kfifo.mask;
+ start = ctx->bitstream_fifo.kfifo.in;
if (coda_bitstream_try_queue(ctx, src_buf)) {
/*
@@ -315,8 +314,7 @@ void coda_fill_bitstream(struct coda_ctx *ctx, struct
list_head *buffer_list)
meta->timecode = src_buf->timecode;
meta->timestamp = src_buf->vb2_buf.timestamp;
meta->start = start;
- meta->end = ctx->bitstream_fifo.kfifo.in &
- ctx->bitstream_fifo.kfifo.mask;
+ meta->end = ctx->bitstream_fifo.kfifo.in;
spin_lock_irqsave(&ctx->buffer_meta_lock,
flags);
list_add_tail(&meta->list,
@@ -1980,8 +1978,7 @@ static int coda_prepare_decode(struct coda_ctx *ctx)
if (meta && ctx->codec->src_fourcc == V4L2_PIX_FMT_JPEG) {
/* If this is the last buffer in the bitstream, add padding */
- if (meta->end == (ctx->bitstream_fifo.kfifo.in &
- ctx->bitstream_fifo.kfifo.mask)) {
+ if (meta->end == ctx->bitstream_fifo.kfifo.in) {
static unsigned char buf[512];
unsigned int pad;
diff --git a/drivers/media/platform/coda/coda-common.c
b/drivers/media/platform/coda/coda-common.c
index cbb59c2f3a82..c53ecc884e15 100644
--- a/drivers/media/platform/coda/coda-common.c
+++ b/drivers/media/platform/coda/coda-common.c
@@ -1297,7 +1297,6 @@ static int coda_job_ready(void *m2m_priv)
return 0;
}
-
if (!src_bufs && !stream_end &&
(coda_get_bitstream_payload(ctx) < 512)) {
v4l2_dbg(1, coda_debug, &ctx->dev->v4l2_dev,
diff --git a/drivers/media/platform/coda/coda.h
b/drivers/media/platform/coda/coda.h
index b6cd14ee91ea..00d0fa50bcd1 100644
--- a/drivers/media/platform/coda/coda.h
+++ b/drivers/media/platform/coda/coda.h
@@ -144,8 +144,8 @@ struct coda_buffer_meta {
u32 sequence;
struct v4l2_timecodetimecode;
u64 timestamp;
- u32 start;
- u32 end;
+ unsigned intstart;
+ unsigned intend;
};
/* Per-queue, driver-specific private data */
diff --git a/drivers/media/platform/coda/trace.h
b/drivers/media/platform/coda/trace.h
index ca671e315ad0..a672bfc4c6ba 100644
--- a/drivers/media/platform/coda/trace.h
+++ b/drivers/media/platform/coda/trace.h
@@ -97,8 +97,8 @@ DECLARE_EVENT_CLASS(coda_buf_meta_class,
TP_fast_assign(
__entry->minor = ctx->fh.vdev->minor;
__entry->index = buf->vb2_buf.index;
- __entry->start = meta->start;
- __entry->end = meta->end;
+ __entry->start = meta->start & ctx->bitstream_fifo.kfifo.mask;
+ __entry->end = meta->end & ctx->bitstream_fifo.kfifo.mask;
__entry->ctx = ctx->idx;
),
@@ -127,8 +127,10 @@ DECLARE_EVENT_CLASS(coda_meta_class,
TP_fast_assign(
__entry->minor = ctx->fh.vdev->minor;
- __entry->start = meta ? meta->start : 0;
- __entry->end = meta ? meta->end : 0;
+ __entry->start = meta ? (meta->start &
+ctx->bitstream_fifo.kfifo.mask) : 0;
+ __entry->end = meta ? (meta->end &
+ ctx->bitstream_fifo.kfifo.mask) : 0;
__entry->ctx = ctx->idx;
),
--
2.19.1
[PATCH 11/15] media: coda: fail S_SELECTION for read-only targets
v4l2-compose complains if S_SELECTION returns 0 for read-only targets.
Signed-off-by: Philipp Zabel
---
drivers/media/platform/coda/coda-common.c | 51 +--
1 file changed, 29 insertions(+), 22 deletions(-)
diff --git a/drivers/media/platform/coda/coda-common.c
b/drivers/media/platform/coda/coda-common.c
index c4d48069606c..fd9bc19cd79b 100644
--- a/drivers/media/platform/coda/coda-common.c
+++ b/drivers/media/platform/coda/coda-common.c
@@ -938,32 +938,39 @@ static int coda_s_selection(struct file *file, void *fh,
struct coda_ctx *ctx = fh_to_ctx(fh);
struct coda_q_data *q_data;
- if (ctx->inst_type == CODA_INST_ENCODER &&
- s->type == V4L2_BUF_TYPE_VIDEO_OUTPUT &&
- s->target == V4L2_SEL_TGT_CROP) {
- q_data = get_q_data(ctx, s->type);
- if (!q_data)
- return -EINVAL;
-
- s->r.left = 0;
- s->r.top = 0;
- s->r.width = clamp(s->r.width, 2U, q_data->width);
- s->r.height = clamp(s->r.height, 2U, q_data->height);
+ switch (s->target) {
+ case V4L2_SEL_TGT_CROP:
+ if (ctx->inst_type == CODA_INST_ENCODER &&
+ s->type == V4L2_BUF_TYPE_VIDEO_OUTPUT) {
+ q_data = get_q_data(ctx, s->type);
+ if (!q_data)
+ return -EINVAL;
- if (s->flags & V4L2_SEL_FLAG_LE) {
- s->r.width = round_up(s->r.width, 2);
- s->r.height = round_up(s->r.height, 2);
- } else {
- s->r.width = round_down(s->r.width, 2);
- s->r.height = round_down(s->r.height, 2);
- }
+ s->r.left = 0;
+ s->r.top = 0;
+ s->r.width = clamp(s->r.width, 2U, q_data->width);
+ s->r.height = clamp(s->r.height, 2U, q_data->height);
+
+ if (s->flags & V4L2_SEL_FLAG_LE) {
+ s->r.width = round_up(s->r.width, 2);
+ s->r.height = round_up(s->r.height, 2);
+ } else {
+ s->r.width = round_down(s->r.width, 2);
+ s->r.height = round_down(s->r.height, 2);
+ }
- q_data->rect = s->r;
+ q_data->rect = s->r;
- return 0;
+ return 0;
+ }
+ /* else fall through */
+ case V4L2_SEL_TGT_NATIVE_SIZE:
+ case V4L2_SEL_TGT_COMPOSE:
+ return coda_g_selection(file, fh, s);
+ default:
+ /* v4l2-compliance expects this to fail for read-only targets */
+ return -EINVAL;
}
-
- return coda_g_selection(file, fh, s);
}
static int coda_try_encoder_cmd(struct file *file, void *fh,
--
2.19.1
[PATCH 03/15] media: coda: always hold back decoder jobs until we have enough bitstream payload
The bitstream prefetch unit reads data in 256 byte blocks with some kind
of queueing. For the decoder to see data up to a desired position in the
next run, the bitstream has to be filled for 2 256 byte blocks past that
position aligned up to the next 256 byte boundary.
This should make sure we never run into a buffer underrun condition if
userspace does not supply new input buffers fast enough.
Signed-off-by: Philipp Zabel
---
drivers/media/platform/coda/coda-common.c | 13 -
drivers/media/platform/coda/coda.h| 12
2 files changed, 20 insertions(+), 5 deletions(-)
diff --git a/drivers/media/platform/coda/coda-common.c
b/drivers/media/platform/coda/coda-common.c
index c53ecc884e15..c7a274c60ff9 100644
--- a/drivers/media/platform/coda/coda-common.c
+++ b/drivers/media/platform/coda/coda-common.c
@@ -1272,6 +1272,7 @@ static int coda_job_ready(void *m2m_priv)
bool stream_end = ctx->bit_stream_param &
CODA_BIT_STREAM_END_FLAG;
int num_metas = ctx->num_metas;
+ struct coda_buffer_meta *meta;
unsigned int count;
count = hweight32(ctx->frm_dis_flg);
@@ -1292,16 +1293,18 @@ static int coda_job_ready(void *m2m_priv)
if (!stream_end && (num_metas + src_bufs) < 2) {
v4l2_dbg(1, coda_debug, &ctx->dev->v4l2_dev,
-"%d: not ready: need 2 buffers available (%d,
%d)\n",
+"%d: not ready: need 2 buffers available
(queue:%d + bitstream:%d)\n",
ctx->idx, num_metas, src_bufs);
return 0;
}
- if (!src_bufs && !stream_end &&
- (coda_get_bitstream_payload(ctx) < 512)) {
+ meta = list_first_entry(&ctx->buffer_meta_list,
+ struct coda_buffer_meta, list);
+ if (!coda_bitstream_can_fetch_past(ctx, meta->end) &&
+ !stream_end) {
v4l2_dbg(1, coda_debug, &ctx->dev->v4l2_dev,
-"%d: not ready: not enough bitstream data
(%d).\n",
-ctx->idx, coda_get_bitstream_payload(ctx));
+"not ready: not enough bitstream data to read
past %u (%u)\n",
+meta->end, ctx->bitstream_fifo.kfifo.in);
return 0;
}
}
diff --git a/drivers/media/platform/coda/coda.h
b/drivers/media/platform/coda/coda.h
index 00d0fa50bcd1..6cb19f47cbed 100644
--- a/drivers/media/platform/coda/coda.h
+++ b/drivers/media/platform/coda/coda.h
@@ -295,6 +295,18 @@ static inline unsigned int
coda_get_bitstream_payload(struct coda_ctx *ctx)
return kfifo_len(&ctx->bitstream_fifo);
}
+/*
+ * The bitstream prefetcher needs to read at least 2 256 byte periods past
+ * the desired bitstream position for all data to reach the decoder.
+ */
+static inline bool coda_bitstream_can_fetch_past(struct coda_ctx *ctx,
+unsigned int pos)
+{
+ return (int)(ctx->bitstream_fifo.kfifo.in - ALIGN(pos, 256)) > 512;
+}
+
+bool coda_bitstream_can_fetch_past(struct coda_ctx *ctx, unsigned int pos);
+
void coda_bit_stream_end_flag(struct coda_ctx *ctx);
void coda_m2m_buf_done(struct coda_ctx *ctx, struct vb2_v4l2_buffer *buf,
--
2.19.1
[PATCH 13/15] media: coda: improve queue busy error message
Use v4l2_type_names to indicate which of the two queues is busy.
Signed-off-by: Philipp Zabel
---
drivers/media/platform/coda/coda-common.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/media/platform/coda/coda-common.c
b/drivers/media/platform/coda/coda-common.c
index fd9bc19cd79b..b3d73965614a 100644
--- a/drivers/media/platform/coda/coda-common.c
+++ b/drivers/media/platform/coda/coda-common.c
@@ -703,7 +703,8 @@ static int coda_s_fmt(struct coda_ctx *ctx, struct
v4l2_format *f,
return -EINVAL;
if (vb2_is_busy(vq)) {
- v4l2_err(&ctx->dev->v4l2_dev, "%s queue busy\n", __func__);
+ v4l2_err(&ctx->dev->v4l2_dev, "%s: %s queue busy: %d\n",
+__func__, v4l2_type_names[f->type], vq->num_buffers);
return -EBUSY;
}
--
2.19.1
[PATCH 06/15] media: coda: remove unused instances list
The per-device instance list is unused, remove it.
Signed-off-by: Philipp Zabel
---
drivers/media/platform/coda/coda-common.c | 9 -
drivers/media/platform/coda/coda.h| 2 --
2 files changed, 11 deletions(-)
diff --git a/drivers/media/platform/coda/coda-common.c
b/drivers/media/platform/coda/coda-common.c
index 01deb454e60b..54b7344231c0 100644
--- a/drivers/media/platform/coda/coda-common.c
+++ b/drivers/media/platform/coda/coda-common.c
@@ -2214,10 +2214,6 @@ static int coda_open(struct file *file)
INIT_LIST_HEAD(&ctx->buffer_meta_list);
spin_lock_init(&ctx->buffer_meta_lock);
- mutex_lock(&dev->dev_mutex);
- list_add(&ctx->list, &dev->instances);
- mutex_unlock(&dev->dev_mutex);
-
v4l2_dbg(1, coda_debug, &dev->v4l2_dev, "Created instance %d (%p)\n",
ctx->idx, ctx);
@@ -2264,10 +2260,6 @@ static int coda_release(struct file *file)
flush_work(&ctx->seq_end_work);
}
- mutex_lock(&dev->dev_mutex);
- list_del(&ctx->list);
- mutex_unlock(&dev->dev_mutex);
-
if (ctx->dev->devtype->product == CODA_DX6)
coda_free_aux_buf(dev, &ctx->workbuf);
@@ -2672,7 +2664,6 @@ static int coda_probe(struct platform_device *pdev)
return -EINVAL;
spin_lock_init(&dev->irqlock);
- INIT_LIST_HEAD(&dev->instances);
dev->plat_dev = pdev;
dev->clk_per = devm_clk_get(&pdev->dev, "per");
diff --git a/drivers/media/platform/coda/coda.h
b/drivers/media/platform/coda/coda.h
index 6cb19f47cbed..aaa90c3d9a16 100644
--- a/drivers/media/platform/coda/coda.h
+++ b/drivers/media/platform/coda/coda.h
@@ -94,7 +94,6 @@ struct coda_dev {
struct mutexcoda_mutex;
struct workqueue_struct *workqueue;
struct v4l2_m2m_dev *m2m_dev;
- struct list_headinstances;
struct ida ida;
struct dentry *debugfs_root;
};
@@ -192,7 +191,6 @@ struct coda_context_ops {
struct coda_ctx {
struct coda_dev *dev;
struct mutexbuffer_mutex;
- struct list_headlist;
struct work_struct pic_run_work;
struct work_struct seq_end_work;
struct completion completion;
--
2.19.1
[PATCH 10/15] media: coda: never set infinite timeperframe
v4l2-compliance complains if G_PARM returns 0 in the denominator.
Signed-off-by: Philipp Zabel
---
drivers/media/platform/coda/coda-common.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/media/platform/coda/coda-common.c
b/drivers/media/platform/coda/coda-common.c
index 32998da39cac..c4d48069606c 100644
--- a/drivers/media/platform/coda/coda-common.c
+++ b/drivers/media/platform/coda/coda-common.c
@@ -1112,10 +1112,10 @@ static void coda_approximate_timeperframe(struct
v4l2_fract *timeperframe)
return;
}
- /* Upper bound is 65536/1, map everything above to infinity */
+ /* Upper bound is 65536/1 */
if (s.denominator == 0 || s.numerator / s.denominator > 65536) {
- timeperframe->numerator = 1;
- timeperframe->denominator = 0;
+ timeperframe->numerator = 65536;
+ timeperframe->denominator = 1;
return;
}
--
2.19.1
[PATCH 05/15] media: coda: reduce minimum frame size to 48x16 pixels.
Three macroblocks seem to be the minimum resolution that can be encoded and decoded by the CODA960 h.264 codec. Picture run commands fail for smaller resolutions. Signed-off-by: Philipp Zabel --- drivers/media/platform/coda/coda-common.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/media/platform/coda/coda-common.c b/drivers/media/platform/coda/coda-common.c index c7a274c60ff9..01deb454e60b 100644 --- a/drivers/media/platform/coda/coda-common.c +++ b/drivers/media/platform/coda/coda-common.c @@ -50,8 +50,8 @@ #define CODA_ISRAM_SIZE(2048 * 2) -#define MIN_W 176 -#define MIN_H 144 +#define MIN_W 48 +#define MIN_H 16 #define S_ALIGN1 /* multiple of 2 */ #define W_ALIGN1 /* multiple of 2 */ -- 2.19.1
[PATCH 04/15] media: coda: limit queueing into internal bitstream buffer
From: Lucas Stach
The ringbuffer used to hold the bitstream is very conservatively sized,
as keyframes can get very large and still need to fit into this buffer.
This means that the buffer is way oversized for the average stream to
the extend that it will hold a few hundred frames when the video data
is compressing well.
The current strategy of queueing as much bitstream data as possible
leads to large delays when draining the decoder. In order to keep the
drain latency to a reasonable bound, try to only queue a full reorder
window of buffers. We can't always hit this low target for very well
compressible video data, as we might end up with less than the minimum
amount of data that needs to be available to the bitstream prefetcher,
so we must take this into account and allow more buffers to be queued
in this case.
Signed-off-by: Lucas Stach
---
drivers/media/platform/coda/coda-bit.c | 28 ++
1 file changed, 28 insertions(+)
diff --git a/drivers/media/platform/coda/coda-bit.c
b/drivers/media/platform/coda/coda-bit.c
index e5ce0bec8ec3..ee9d2a402ccd 100644
--- a/drivers/media/platform/coda/coda-bit.c
+++ b/drivers/media/platform/coda/coda-bit.c
@@ -269,6 +269,23 @@ void coda_fill_bitstream(struct coda_ctx *ctx, struct
list_head *buffer_list)
ctx->num_metas > 1)
break;
+ if (ctx->num_internal_frames &&
+ ctx->num_metas >= ctx->num_internal_frames) {
+ meta = list_first_entry(&ctx->buffer_meta_list,
+ struct coda_buffer_meta, list);
+
+ /*
+* If we managed to fill in at least a full reorder
+* window of buffers (num_internal_frames is a
+* conservative estimate for this) and the bitstream
+* prefetcher has at least 2 256 bytes periods beyond
+* the first buffer to fetch, we can safely stop queuing
+* in order to limit the decoder drain latency.
+*/
+ if (coda_bitstream_can_fetch_past(ctx, meta->end))
+ break;
+ }
+
src_buf = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx);
/* Drop frames that do not start/end with a SOI/EOI markers */
@@ -2252,6 +2269,17 @@ static void coda_finish_decode(struct coda_ctx *ctx)
/* The rotator will copy the current display frame next time */
ctx->display_idx = display_idx;
+
+ /*
+* The current decode run might have brought the bitstream fill level
+* below the size where we can start the next decode run. As userspace
+* might have filled the output queue completely and might thus be
+* blocked, we can't rely on the next qbuf to trigger the bitstream
+* refill. Check if we have data to refill the bitstream now.
+*/
+ mutex_lock(&ctx->bitstream_mutex);
+ coda_fill_bitstream(ctx, NULL);
+ mutex_unlock(&ctx->bitstream_mutex);
}
static void coda_decode_timeout(struct coda_ctx *ctx)
--
2.19.1
[PATCH 15/15] media: coda: debug output when setting visible size via crop selection
In addition to the S_FMT debug output, S_SELECTION (SEL_TGT_CROP) is relevant to determine encoded size. Add debug output for it. Signed-off-by: Philipp Zabel --- drivers/media/platform/coda/coda-common.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/media/platform/coda/coda-common.c b/drivers/media/platform/coda/coda-common.c index 2a0e0d04c67a..bf4b21b7cdb3 100644 --- a/drivers/media/platform/coda/coda-common.c +++ b/drivers/media/platform/coda/coda-common.c @@ -961,6 +961,9 @@ static int coda_s_selection(struct file *file, void *fh, q_data->rect = s->r; + coda_dbg(1, ctx, "Setting crop rectangle: %dx%d\n", +s->r.width, s->r.height); + return 0; } /* else fall through */ -- 2.19.1
[PATCH 07/15] media: coda: don't disable IRQs across buffer meta handling
From: Lucas Stach
The CODA driver uses threaded IRQs only, so there is nothing happening
in hardirq context that could interfere with the buffer meta handling.
Signed-off-by: Lucas Stach
---
drivers/media/platform/coda/coda-bit.c| 19 +++
drivers/media/platform/coda/coda-common.c | 5 ++---
2 files changed, 9 insertions(+), 15 deletions(-)
diff --git a/drivers/media/platform/coda/coda-bit.c
b/drivers/media/platform/coda/coda-bit.c
index ee9d2a402ccd..348b17140715 100644
--- a/drivers/media/platform/coda/coda-bit.c
+++ b/drivers/media/platform/coda/coda-bit.c
@@ -253,7 +253,6 @@ void coda_fill_bitstream(struct coda_ctx *ctx, struct
list_head *buffer_list)
{
struct vb2_v4l2_buffer *src_buf;
struct coda_buffer_meta *meta;
- unsigned long flags;
u32 start;
if (ctx->bit_stream_param & CODA_BIT_STREAM_END_FLAG)
@@ -332,13 +331,11 @@ void coda_fill_bitstream(struct coda_ctx *ctx, struct
list_head *buffer_list)
meta->timestamp = src_buf->vb2_buf.timestamp;
meta->start = start;
meta->end = ctx->bitstream_fifo.kfifo.in;
- spin_lock_irqsave(&ctx->buffer_meta_lock,
- flags);
+ spin_lock(&ctx->buffer_meta_lock);
list_add_tail(&meta->list,
&ctx->buffer_meta_list);
ctx->num_metas++;
- spin_unlock_irqrestore(&ctx->buffer_meta_lock,
- flags);
+ spin_unlock(&ctx->buffer_meta_lock);
trace_coda_bit_queue(ctx, src_buf, meta);
}
@@ -1894,7 +1891,6 @@ static int coda_prepare_decode(struct coda_ctx *ctx)
struct coda_dev *dev = ctx->dev;
struct coda_q_data *q_data_dst;
struct coda_buffer_meta *meta;
- unsigned long flags;
u32 rot_mode = 0;
u32 reg_addr, reg_stride;
@@ -1988,7 +1984,7 @@ static int coda_prepare_decode(struct coda_ctx *ctx)
coda_write(dev, ctx->iram_info.axi_sram_use,
CODA7_REG_BIT_AXI_SRAM_USE);
- spin_lock_irqsave(&ctx->buffer_meta_lock, flags);
+ spin_lock(&ctx->buffer_meta_lock);
meta = list_first_entry_or_null(&ctx->buffer_meta_list,
struct coda_buffer_meta, list);
@@ -2007,7 +2003,7 @@ static int coda_prepare_decode(struct coda_ctx *ctx)
kfifo_in(&ctx->bitstream_fifo, buf, pad);
}
}
- spin_unlock_irqrestore(&ctx->buffer_meta_lock, flags);
+ spin_unlock(&ctx->buffer_meta_lock);
coda_kfifo_sync_to_device_full(ctx);
@@ -2029,7 +2025,6 @@ static void coda_finish_decode(struct coda_ctx *ctx)
struct vb2_v4l2_buffer *dst_buf;
struct coda_buffer_meta *meta;
unsigned long payload;
- unsigned long flags;
int width, height;
int decoded_idx;
int display_idx;
@@ -2161,13 +2156,13 @@ static void coda_finish_decode(struct coda_ctx *ctx)
} else {
val = coda_read(dev, CODA_RET_DEC_PIC_FRAME_NUM) - 1;
val -= ctx->sequence_offset;
- spin_lock_irqsave(&ctx->buffer_meta_lock, flags);
+ spin_lock(&ctx->buffer_meta_lock);
if (!list_empty(&ctx->buffer_meta_list)) {
meta = list_first_entry(&ctx->buffer_meta_list,
struct coda_buffer_meta, list);
list_del(&meta->list);
ctx->num_metas--;
- spin_unlock_irqrestore(&ctx->buffer_meta_lock, flags);
+ spin_unlock(&ctx->buffer_meta_lock);
/*
* Clamp counters to 16 bits for comparison, as the HW
* counter rolls over at this point for h.264. This
@@ -2184,7 +2179,7 @@ static void coda_finish_decode(struct coda_ctx *ctx)
ctx->frame_metas[decoded_idx] = *meta;
kfree(meta);
} else {
- spin_unlock_irqrestore(&ctx->buffer_meta_lock, flags);
+ spin_unlock(&ctx->buffer_meta_lock);
v4l2_err(&dev->v4l2_dev, "empty timestamp list!\n");
memset(&ctx->frame_metas[decoded_idx], 0,
sizeof(struct coda_buffer_meta));
diff --git a/drivers/media/platform/coda/coda-common.c
b/drivers/media/platform/coda/coda-common.c
index 54b7344231c0..60b866160094 100644
--- a/drivers/media/platform/coda/coda-common.c
+++ b/drivers/media/platform/coda/coda-common.c
@@ -1689,7 +1689,6 @@ static void coda_sto
[PATCH 1/1] v4l: event: Add subscription to list before calling "add" operation
Patch ad608fbcf166 changed how events were subscribed to address an issue
elsewhere. As a side effect of that change, the "add" callback was called
before the event subscription was added to the list of subscribed events,
causing the first event (and possibly other events arriving soon
afterwards) to be lost.
Fix this by adding the subscription to the list before calling the "add"
callback, and clean up afterwards if that fails.
Fixes: ad608fbcf166 ("media: v4l: event: Prevent freeing event subscriptions
while accessed")
Reported-by: Dave Stevenson
Signed-off-by: Sakari Ailus
---
Hi Dave, Hans,
This should address the issue.
The functions can (and probably should) be re-arranged later but let's get
the fix right first.
I haven't tested this using vivid yet as it crashes where I was testing
it. I'll see later if it works elsewhere but I'm sending the patch for
review in the meantime.
drivers/media/v4l2-core/v4l2-event.c | 39 +++-
1 file changed, 25 insertions(+), 14 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-event.c
b/drivers/media/v4l2-core/v4l2-event.c
index a3ef1f50a4b3..2b6651aeb89b 100644
--- a/drivers/media/v4l2-core/v4l2-event.c
+++ b/drivers/media/v4l2-core/v4l2-event.c
@@ -193,6 +193,22 @@ int v4l2_event_pending(struct v4l2_fh *fh)
}
EXPORT_SYMBOL_GPL(v4l2_event_pending);
+static void __v4l2_event_unsubscribe(struct v4l2_subscribed_event *sev)
+{
+ struct v4l2_fh *fh = sev->fh;
+ unsigned int i;
+
+ lockdep_assert_held(&fh->subscribe_lock);
+ assert_spin_locked(&fh->vdev->fh_lock);
+
+ /* Remove any pending events for this subscription */
+ for (i = 0; i < sev->in_use; i++) {
+ list_del(&sev->events[sev_pos(sev, i)].list);
+ fh->navailable--;
+ }
+ list_del(&sev->list);
+}
+
int v4l2_event_subscribe(struct v4l2_fh *fh,
const struct v4l2_event_subscription *sub, unsigned
elems,
const struct v4l2_subscribed_event_ops *ops)
@@ -232,18 +248,20 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
goto out_unlock;
}
+ spin_lock_irqsave(&fh->vdev->fh_lock, flags);
+ list_add(&sev->list, &fh->subscribed);
+ spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
+
if (sev->ops && sev->ops->add) {
ret = sev->ops->add(sev, elems);
if (ret) {
+ spin_lock_irqsave(&fh->vdev->fh_lock, flags);
+ __v4l2_event_unsubscribe(sev);
+ spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
kvfree(sev);
- goto out_unlock;
}
}
- spin_lock_irqsave(&fh->vdev->fh_lock, flags);
- list_add(&sev->list, &fh->subscribed);
- spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
-
out_unlock:
mutex_unlock(&fh->subscribe_lock);
@@ -279,7 +297,6 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
{
struct v4l2_subscribed_event *sev;
unsigned long flags;
- int i;
if (sub->type == V4L2_EVENT_ALL) {
v4l2_event_unsubscribe_all(fh);
@@ -291,14 +308,8 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
spin_lock_irqsave(&fh->vdev->fh_lock, flags);
sev = v4l2_event_subscribed(fh, sub->type, sub->id);
- if (sev != NULL) {
- /* Remove any pending events for this subscription */
- for (i = 0; i < sev->in_use; i++) {
- list_del(&sev->events[sev_pos(sev, i)].list);
- fh->navailable--;
- }
- list_del(&sev->list);
- }
+ if (sev != NULL)
+ __v4l2_event_unsubscribe(sev);
spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
--
2.11.0
Re: [PATCH 1/1] v4l: event: Add subscription to list before calling "add" operation
On 11/05/2018 04:46 PM, Sakari Ailus wrote:
> Patch ad608fbcf166 changed how events were subscribed to address an issue
> elsewhere. As a side effect of that change, the "add" callback was called
> before the event subscription was added to the list of subscribed events,
> causing the first event (and possibly other events arriving soon
> afterwards) to be lost.
>
> Fix this by adding the subscription to the list before calling the "add"
> callback, and clean up afterwards if that fails.
>
> Fixes: ad608fbcf166 ("media: v4l: event: Prevent freeing event subscriptions
> while accessed")
>
> Reported-by: Dave Stevenson
> Signed-off-by: Sakari Ailus
> ---
> Hi Dave, Hans,
>
> This should address the issue.
>
> The functions can (and probably should) be re-arranged later but let's get
> the fix right first.
>
> I haven't tested this using vivid yet as it crashes where I was testing
> it. I'll see later if it works elsewhere but I'm sending the patch for
> review in the meantime.
Did a quick vivid test and it passes the tests now.
I'll review the code tomorrow.
Regards,
Hans
>
> drivers/media/v4l2-core/v4l2-event.c | 39
> +++-
> 1 file changed, 25 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-event.c
> b/drivers/media/v4l2-core/v4l2-event.c
> index a3ef1f50a4b3..2b6651aeb89b 100644
> --- a/drivers/media/v4l2-core/v4l2-event.c
> +++ b/drivers/media/v4l2-core/v4l2-event.c
> @@ -193,6 +193,22 @@ int v4l2_event_pending(struct v4l2_fh *fh)
> }
> EXPORT_SYMBOL_GPL(v4l2_event_pending);
>
> +static void __v4l2_event_unsubscribe(struct v4l2_subscribed_event *sev)
> +{
> + struct v4l2_fh *fh = sev->fh;
> + unsigned int i;
> +
> + lockdep_assert_held(&fh->subscribe_lock);
> + assert_spin_locked(&fh->vdev->fh_lock);
> +
> + /* Remove any pending events for this subscription */
> + for (i = 0; i < sev->in_use; i++) {
> + list_del(&sev->events[sev_pos(sev, i)].list);
> + fh->navailable--;
> + }
> + list_del(&sev->list);
> +}
> +
> int v4l2_event_subscribe(struct v4l2_fh *fh,
>const struct v4l2_event_subscription *sub, unsigned
> elems,
>const struct v4l2_subscribed_event_ops *ops)
> @@ -232,18 +248,20 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
> goto out_unlock;
> }
>
> + spin_lock_irqsave(&fh->vdev->fh_lock, flags);
> + list_add(&sev->list, &fh->subscribed);
> + spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
> +
> if (sev->ops && sev->ops->add) {
> ret = sev->ops->add(sev, elems);
> if (ret) {
> + spin_lock_irqsave(&fh->vdev->fh_lock, flags);
> + __v4l2_event_unsubscribe(sev);
> + spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
> kvfree(sev);
> - goto out_unlock;
> }
> }
>
> - spin_lock_irqsave(&fh->vdev->fh_lock, flags);
> - list_add(&sev->list, &fh->subscribed);
> - spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
> -
> out_unlock:
> mutex_unlock(&fh->subscribe_lock);
>
> @@ -279,7 +297,6 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
> {
> struct v4l2_subscribed_event *sev;
> unsigned long flags;
> - int i;
>
> if (sub->type == V4L2_EVENT_ALL) {
> v4l2_event_unsubscribe_all(fh);
> @@ -291,14 +308,8 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
> spin_lock_irqsave(&fh->vdev->fh_lock, flags);
>
> sev = v4l2_event_subscribed(fh, sub->type, sub->id);
> - if (sev != NULL) {
> - /* Remove any pending events for this subscription */
> - for (i = 0; i < sev->in_use; i++) {
> - list_del(&sev->events[sev_pos(sev, i)].list);
> - fh->navailable--;
> - }
> - list_del(&sev->list);
> - }
> + if (sev != NULL)
> + __v4l2_event_unsubscribe(sev);
>
> spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
>
>
[PATCH] media: rc: ensure close() is called on rc_unregister_device
If userspace has an open file descriptor on the rc input device or lirc device when rc_unregister_device() is called, then the rc close() is never called. This ensures that the receiver is turned off on the nuvoton-cir driver during shutdown. Signed-off-by: Sean Young --- drivers/media/rc/rc-main.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c index 552bbe82a160..8863da4204a3 100644 --- a/drivers/media/rc/rc-main.c +++ b/drivers/media/rc/rc-main.c @@ -1950,6 +1950,8 @@ void rc_unregister_device(struct rc_dev *dev) rc_free_rx_device(dev); mutex_lock(&dev->lock); + if (dev->users && dev->close) + dev->close(dev); dev->registered = false; mutex_unlock(&dev->lock); -- 2.17.2
Re: [RFC PATCH 00/11] Convert last remaining g/s_crop/cropcap drivers
Hi Hans, On 11/05/2018 02:12 PM, Hans Verkuil wrote: > Thank you for the review. One question: have you also tested this with at > least > one of the affected drivers? > > I'd like to have at least one Tested-by line. I just tested it now - video playback on Exynos4210 Trats2 so it covers the s5p-mfc and exynos4-is (fimc-m2m) drivers. Well done, I couldn't see any breakage. You can add "Tested-by: Sylwester Nawrocki " to patches: 1, 2, 3, 7, 8, 10. -- Regards, Sylwester
Re: [RFC PATCH 00/11] Convert last remaining g/s_crop/cropcap drivers
On 11/05/2018 05:08 PM, Sylwester Nawrocki wrote: > Hi Hans, > > On 11/05/2018 02:12 PM, Hans Verkuil wrote: >> Thank you for the review. One question: have you also tested this with at >> least >> one of the affected drivers? >> >> I'd like to have at least one Tested-by line. > > I just tested it now - video playback on Exynos4210 Trats2 so it covers > the s5p-mfc and exynos4-is (fimc-m2m) drivers. Well done, I couldn't see > any breakage. > > You can add "Tested-by: Sylwester Nawrocki " > to patches: 1, 2, 3, 7, 8, 10. > Fantastic, I'll see if I can make a pull request for this series this week. Regards, Hans
Re: [PATCH 1/1] v4l: event: Add subscription to list before calling "add" operation
Hi Sakari
On Mon, 5 Nov 2018 at 15:46, Sakari Ailus wrote:
>
> Patch ad608fbcf166 changed how events were subscribed to address an issue
> elsewhere. As a side effect of that change, the "add" callback was called
> before the event subscription was added to the list of subscribed events,
> causing the first event (and possibly other events arriving soon
> afterwards) to be lost.
>
> Fix this by adding the subscription to the list before calling the "add"
> callback, and clean up afterwards if that fails.
>
> Fixes: ad608fbcf166 ("media: v4l: event: Prevent freeing event subscriptions
> while accessed")
>
> Reported-by: Dave Stevenson
> Signed-off-by: Sakari Ailus
> ---
> Hi Dave, Hans,
>
> This should address the issue.
>
> The functions can (and probably should) be re-arranged later but let's get
> the fix right first.
>
> I haven't tested this using vivid yet as it crashes where I was testing
> it. I'll see later if it works elsewhere but I'm sending the patch for
> review in the meantime.
Thanks. That seems to fix the failure for my 3 drivers.
For what it's worth:
Tested-by: Dave Stevenson
I haven't tested the failure path, but it looks sane.
> drivers/media/v4l2-core/v4l2-event.c | 39
> +++-
> 1 file changed, 25 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-event.c
> b/drivers/media/v4l2-core/v4l2-event.c
> index a3ef1f50a4b3..2b6651aeb89b 100644
> --- a/drivers/media/v4l2-core/v4l2-event.c
> +++ b/drivers/media/v4l2-core/v4l2-event.c
> @@ -193,6 +193,22 @@ int v4l2_event_pending(struct v4l2_fh *fh)
> }
> EXPORT_SYMBOL_GPL(v4l2_event_pending);
>
> +static void __v4l2_event_unsubscribe(struct v4l2_subscribed_event *sev)
> +{
> + struct v4l2_fh *fh = sev->fh;
> + unsigned int i;
> +
> + lockdep_assert_held(&fh->subscribe_lock);
> + assert_spin_locked(&fh->vdev->fh_lock);
> +
> + /* Remove any pending events for this subscription */
> + for (i = 0; i < sev->in_use; i++) {
> + list_del(&sev->events[sev_pos(sev, i)].list);
> + fh->navailable--;
> + }
> + list_del(&sev->list);
> +}
> +
> int v4l2_event_subscribe(struct v4l2_fh *fh,
> const struct v4l2_event_subscription *sub, unsigned
> elems,
> const struct v4l2_subscribed_event_ops *ops)
> @@ -232,18 +248,20 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
> goto out_unlock;
> }
>
> + spin_lock_irqsave(&fh->vdev->fh_lock, flags);
> + list_add(&sev->list, &fh->subscribed);
> + spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
> +
> if (sev->ops && sev->ops->add) {
> ret = sev->ops->add(sev, elems);
> if (ret) {
> + spin_lock_irqsave(&fh->vdev->fh_lock, flags);
> + __v4l2_event_unsubscribe(sev);
> + spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
> kvfree(sev);
> - goto out_unlock;
> }
> }
>
> - spin_lock_irqsave(&fh->vdev->fh_lock, flags);
> - list_add(&sev->list, &fh->subscribed);
> - spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
> -
> out_unlock:
> mutex_unlock(&fh->subscribe_lock);
>
> @@ -279,7 +297,6 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
> {
> struct v4l2_subscribed_event *sev;
> unsigned long flags;
> - int i;
>
> if (sub->type == V4L2_EVENT_ALL) {
> v4l2_event_unsubscribe_all(fh);
> @@ -291,14 +308,8 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
> spin_lock_irqsave(&fh->vdev->fh_lock, flags);
>
> sev = v4l2_event_subscribed(fh, sub->type, sub->id);
> - if (sev != NULL) {
> - /* Remove any pending events for this subscription */
> - for (i = 0; i < sev->in_use; i++) {
> - list_del(&sev->events[sev_pos(sev, i)].list);
> - fh->navailable--;
> - }
> - list_del(&sev->list);
> - }
> + if (sev != NULL)
> + __v4l2_event_unsubscribe(sev);
>
> spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
>
> --
> 2.11.0
>
Re: [PATCH 01/15] media: coda: fix memory corruption in case more than 32 instances are opened
Hi Philipp,
On 05/11/2018 15:24, Philipp Zabel wrote:
The ffz() return value is undefined if the instance mask does not
contain any zeros. If it returned 32, the following set_bit would
corrupt the debugfs_root pointer.
Switch to IDA for context index allocation. This also removes the
artificial 32 instance limit for all except CodaDx6.
Signed-off-by: Philipp Zabel
---
drivers/media/platform/coda/coda-common.c | 25 ---
drivers/media/platform/coda/coda.h| 2 +-
2 files changed, 10 insertions(+), 17 deletions(-)
diff --git a/drivers/media/platform/coda/coda-common.c
b/drivers/media/platform/coda/coda-common.c
index 2848ea5f464d..cbb59c2f3a82 100644
--- a/drivers/media/platform/coda/coda-common.c
+++ b/drivers/media/platform/coda/coda-common.c
@@ -2099,17 +2099,6 @@ int coda_decoder_queue_init(void *priv, struct vb2_queue
*src_vq,
return coda_queue_init(priv, dst_vq);
}
-static int coda_next_free_instance(struct coda_dev *dev)
-{
- int idx = ffz(dev->instance_mask);
-
- if ((idx < 0) ||
- (dev->devtype->product == CODA_DX6 && idx > CODADX6_MAX_INSTANCES))
- return -EBUSY;
-
- return idx;
-}
-
/*
* File operations
*/
@@ -2118,7 +2107,8 @@ static int coda_open(struct file *file)
{
struct video_device *vdev = video_devdata(file);
struct coda_dev *dev = video_get_drvdata(vdev);
- struct coda_ctx *ctx = NULL;
+ struct coda_ctx *ctx;
+ unsigned int max = ~0;
char *name;
int ret;
int idx;
@@ -2127,12 +2117,13 @@ static int coda_open(struct file *file)
if (!ctx)
return -ENOMEM;
- idx = coda_next_free_instance(dev);
+ if (dev->devtype->product == CODA_DX6)
+ max = CODADX6_MAX_INSTANCES - 1;
+ idx = ida_alloc_max(&dev->ida, max, GFP_KERNEL);
if (idx < 0) {
ret = idx;
goto err_coda_max;
}
- set_bit(idx, &dev->instance_mask);
name = kasprintf(GFP_KERNEL, "context%d", idx);
if (!name) {
@@ -2241,8 +2232,8 @@ static int coda_open(struct file *file)
err_pm_get:
v4l2_fh_del(&ctx->fh);
v4l2_fh_exit(&ctx->fh);
- clear_bit(ctx->idx, &dev->instance_mask);
err_coda_name_init:
+ ida_free(&dev->ida, ctx->idx);
err_coda_max:
kfree(ctx);
return ret;
@@ -2284,7 +2275,7 @@ static int coda_release(struct file *file)
pm_runtime_put_sync(&dev->plat_dev->dev);
v4l2_fh_del(&ctx->fh);
v4l2_fh_exit(&ctx->fh);
- clear_bit(ctx->idx, &dev->instance_mask);
+ ida_free(&dev->ida, ctx->idx);
if (ctx->ops->release)
ctx->ops->release(ctx);
debugfs_remove_recursive(ctx->debugfs_entry);
@@ -2745,6 +2736,7 @@ static int coda_probe(struct platform_device *pdev)
mutex_init(&dev->dev_mutex);
mutex_init(&dev->coda_mutex);
+ ida_init(&dev->ida);
dev->debugfs_root = debugfs_create_dir("coda", NULL);
if (!dev->debugfs_root)
@@ -2832,6 +2824,7 @@ static int coda_remove(struct platform_device *pdev)
coda_free_aux_buf(dev, &dev->tempbuf);
coda_free_aux_buf(dev, &dev->workbuf);
debugfs_remove_recursive(dev->debugfs_root);
+ ida_destroy(&dev->ida);
return 0;
}
diff --git a/drivers/media/platform/coda/coda.h b/drivers/media/platform/coda/coda.h
index 19ac0b9dc6eb..b6cd14ee91ea 100644
--- a/drivers/media/platform/coda/coda.h
+++ b/drivers/media/platform/coda/coda.h
Should you add:
#include
to this header?
Regards,
Ian
@@ -95,7 +95,7 @@ struct coda_dev {
struct workqueue_struct *workqueue;
struct v4l2_m2m_dev *m2m_dev;
struct list_headinstances;
- unsigned long instance_mask;
+ struct ida ida;
struct dentry *debugfs_root;
};
RE: [PATCH v7 05/16] intel-ipu3: abi: Add structs
Hi Sakari,
> -Original Message-
> From: Sakari Ailus [mailto:sakari.ai...@linux.intel.com]
> Sent: Monday, November 05, 2018 12:28 AM
> To: Zhi, Yong
> Cc: linux-media@vger.kernel.org; tf...@chromium.org; mche...@kernel.org;
> hans.verk...@cisco.com; laurent.pinch...@ideasonboard.com; Mani,
> Rajmohan ; Zheng, Jian Xu
> ; Hu, Jerry W ; Toivonen,
> Tuukka ; Qiu, Tian Shu
> ; Cao, Bingbu
> Subject: Re: [PATCH v7 05/16] intel-ipu3: abi: Add structs
>
> Hi Yong,
>
> On Mon, Oct 29, 2018 at 03:22:59PM -0700, Yong Zhi wrote:
> > This add all the structs of IPU3 firmware ABI.
> >
> > Signed-off-by: Yong Zhi
> > Signed-off-by: Rajmohan Mani
>
> ...
>
> > +struct imgu_abi_shd_intra_frame_operations_data {
> > + struct imgu_abi_acc_operation
> > + operation_list[IMGU_ABI_SHD_MAX_OPERATIONS]
> __attribute__((aligned(32)));
> > + struct imgu_abi_acc_process_lines_cmd_data
> > + process_lines_data[IMGU_ABI_SHD_MAX_PROCESS_LINES]
> __attribute__((aligned(32)));
> > + struct imgu_abi_shd_transfer_luts_set_data
> > + transfer_data[IMGU_ABI_SHD_MAX_TRANSFERS]
> > +__attribute__((aligned(32)));
>
> Could you replace this wth __aligned(32), please? The same for the rest of the
> header.
>
Using __aligned(32) in the uAPI header resulted in compilation errors in
user space / camera HAL code.
e.g
../../../../../../../../usr/include/linux/intel-ipu3.h:464:57: error: expected
';'
at end of declaration list
__u8 bayer_table[IPU3_UAPI_AWB_FR_BAYER_TABLE_MAX_SIZE] __aligned(32);
So we ended up using __attribute__((aligned(32))) format in uAPI header and
to be consistent, we followed the same format in ABI header as well.
Let us know if it's okay to deviate between uAPI and ABI header for this
alignment qualifier.
> --
> Regards,
>
> Sakari Ailus
> sakari.ai...@linux.intel.com
Astrometa DVB-T2 2018 update
Enable Sony CXD2837ER slave demon on the Astrometa DVB-T2, known as the 2018
update.
Originally based on the patch by kapitanf at
https://github.com/torvalds/linux/pull/567, it was not quite right. This is
more correct, but probably still wrong. I'm not a kernel dev, but someone may
be better positioned to handle the niceties.
diff --git a/drivers/media/usb/dvb-usb-v2/Kconfig
b/drivers/media/usb/dvb-usb-v2/Kconfig
index df4412245a8a..d44ddd5ee29e 100644
--- a/drivers/media/usb/dvb-usb-v2/Kconfig
+++ b/drivers/media/usb/dvb-usb-v2/Kconfig
@@ -137,6 +137,7 @@ config DVB_USB_RTL28XXU
select DVB_RTL2832
select DVB_RTL2832_SDR if (MEDIA_SUBDRV_AUTOSELECT && MEDIA_SDR_SUPPORT)
select DVB_SI2168 if MEDIA_SUBDRV_AUTOSELECT
+ select DVB_CXD2841ER if MEDIA_SUBDRV_AUTOSELECT
select MEDIA_TUNER_E4000 if MEDIA_SUBDRV_AUTOSELECT
select MEDIA_TUNER_FC0012 if MEDIA_SUBDRV_AUTOSELECT
select MEDIA_TUNER_FC0013 if MEDIA_SUBDRV_AUTOSELECT
diff --git a/drivers/media/usb/dvb-usb-v2/rtl28xxu.c
b/drivers/media/usb/dvb-usb-v2/rtl28xxu.c
index a970224a94bd..db4f4da43781 100644
--- a/drivers/media/usb/dvb-usb-v2/rtl28xxu.c
+++ b/drivers/media/usb/dvb-usb-v2/rtl28xxu.c
@@ -386,6 +386,7 @@ static int rtl2832u_read_config(struct dvb_usb_device *d)
struct rtl28xxu_req req_mn88473 = {0xff38, CMD_I2C_RD, 1, buf};
struct rtl28xxu_req req_si2157 = {0x00c0, CMD_I2C_RD, 1, buf};
struct rtl28xxu_req req_si2168 = {0x00c8, CMD_I2C_RD, 1, buf};
+ struct rtl28xxu_req req_cxd2837er = {0x68d8, CMD_I2C_RD, 1, buf};
dev_dbg(&d->intf->dev, "\n");
@@ -567,6 +568,13 @@ static int rtl2832u_read_config(struct dvb_usb_device *d)
dev->slave_demod = SLAVE_DEMOD_MN88473;
goto demod_found;
}
+
+ ret = rtl28xxu_ctrl_msg(d, &req_cxd2837er);
+ if (ret == 0 && buf[0] == 0x03) {
+ dev_dbg(&d->intf->dev, "CXD2837ER found");
+ dev->slave_demod = SLAVE_DEMOD_CXD2841ER;
+ goto demod_found;
+ }
}
if (dev->tuner == TUNER_RTL2832_SI2157) {
/* check Si2168 ID register; reg=c8 val=80 */
@@ -988,6 +996,27 @@ static int rtl2832u_frontend_attach(struct dvb_usb_adapter
*adap)
goto err_slave_demod_failed;
}
+ dev->i2c_client_slave_demod = client;
+ } else if (dev->slave_demod == SLAVE_DEMOD_CXD2841ER) {
+ struct cxd2841er_config cxd2837er_config = {};
+ cxd2837er_config.i2c_addr = 0xd8;
+ cxd2837er_config.xtal = SONY_XTAL_20500;
+ cxd2837er_config.flags = CXD2841ER_AUTO_IFHZ|
CXD2841ER_EARLY_TUNE |
+ CXD2841ER_NO_WAIT_LOCK |
CXD2841ER_NO_AGCNEG |
+ CXD2841ER_TSBITS |
CXD2841ER_TS_SERIAL;
+
+ adap->fe[1] = dvb_attach( cxd2841er_attach_t_c,
&cxd2837er_config, &d->i2c_adap );
+ if (!adap->fe[1]) {
+ dev_err(&d->intf->dev, "CXD2837ER attach
failed!\n");
+ return -ENODEV;
+ }
+
+ if (!try_module_get(client->dev.driver->owner)) {
+ i2c_unregister_device(client);
+ dev->slave_demod = SLAVE_DEMOD_NONE;
+ goto err_slave_demod_failed;
+ }
+
dev->i2c_client_slave_demod = client;
} else {
struct si2168_config si2168_config = {};
@@ -1046,10 +1075,14 @@ static int rtl28xxu_frontend_detach(struct
dvb_usb_adapter *adap)
dev_dbg(&d->intf->dev, "\n");
/* remove I2C slave demod */
- client = dev->i2c_client_slave_demod;
- if (client) {
- module_put(client->dev.driver->owner);
- i2c_unregister_device(client);
+ if (dev->slave_demod == SLAVE_DEMOD_CXD2841ER) {
+ dev_info(&d->intf->dev,"Sony CXD2837ER detached
automatically.");
+ } else {
+ client = dev->i2c_client_slave_demod;
+ if (client) {
+ module_put(client->dev.driver->owner);
+ i2c_unregister_device(client);
+ }
}
/* remove I2C demod */
diff --git a/drivers/media/usb/dvb-usb-v2/rtl28xxu.h
b/drivers/media/usb/dvb-usb-v2/rtl28xxu.h
index 138062960a73..5a615d73fc34 100644
--- a/drivers/media/usb/dvb-usb-v2/rtl28xxu.h
+++ b/drivers/media/usb/dvb-usb-v2/rtl28xxu.h
@@ -43,6 +43,7 @@
#include "r820t.h"
#include "si2168.h"
#include "si2157.h"
+#include "cxd2841er.h"
/*
* USB commands
@@ -87,7 +88,8 @@ struct rtl28xxu_dev {
#define SLAVE_DEMOD_MN884721
Astrometa DVB-T2 2018 update
Enable Sony CXD2837ER slave demon on the Astrometa DVB-T2, known as the 2018
update.
Originally based on the patch by kapitanf at
https://github.com/torvalds/linux/pull/567, it was not quite right. This is
more correct, but probably still wrong. I'm not a kernel dev, but someone may
be better positioned to handle the niceties.
diff --git a/drivers/media/usb/dvb-usb-v2/Kconfig
b/drivers/media/usb/dvb-usb-v2/Kconfig
index df4412245a8a..d44ddd5ee29e 100644
--- a/drivers/media/usb/dvb-usb-v2/Kconfig
+++ b/drivers/media/usb/dvb-usb-v2/Kconfig
@@ -137,6 +137,7 @@ config DVB_USB_RTL28XXU
select DVB_RTL2832
select DVB_RTL2832_SDR if (MEDIA_SUBDRV_AUTOSELECT && MEDIA_SDR_SUPPORT)
select DVB_SI2168 if MEDIA_SUBDRV_AUTOSELECT
+ select DVB_CXD2841ER if MEDIA_SUBDRV_AUTOSELECT
select MEDIA_TUNER_E4000 if MEDIA_SUBDRV_AUTOSELECT
select MEDIA_TUNER_FC0012 if MEDIA_SUBDRV_AUTOSELECT
select MEDIA_TUNER_FC0013 if MEDIA_SUBDRV_AUTOSELECT
diff --git a/drivers/media/usb/dvb-usb-v2/rtl28xxu.c
b/drivers/media/usb/dvb-usb-v2/rtl28xxu.c
index a970224a94bd..db4f4da43781 100644
--- a/drivers/media/usb/dvb-usb-v2/rtl28xxu.c
+++ b/drivers/media/usb/dvb-usb-v2/rtl28xxu.c
@@ -386,6 +386,7 @@ static int rtl2832u_read_config(struct dvb_usb_device *d)
struct rtl28xxu_req req_mn88473 = {0xff38, CMD_I2C_RD, 1, buf};
struct rtl28xxu_req req_si2157 = {0x00c0, CMD_I2C_RD, 1, buf};
struct rtl28xxu_req req_si2168 = {0x00c8, CMD_I2C_RD, 1, buf};
+ struct rtl28xxu_req req_cxd2837er = {0x68d8, CMD_I2C_RD, 1, buf};
dev_dbg(&d->intf->dev, "\n");
@@ -567,6 +568,13 @@ static int rtl2832u_read_config(struct dvb_usb_device *d)
dev->slave_demod = SLAVE_DEMOD_MN88473;
goto demod_found;
}
+
+ ret = rtl28xxu_ctrl_msg(d, &req_cxd2837er);
+ if (ret == 0 && buf[0] == 0x03) {
+ dev_dbg(&d->intf->dev, "CXD2837ER found");
+ dev->slave_demod = SLAVE_DEMOD_CXD2841ER;
+ goto demod_found;
+ }
}
if (dev->tuner == TUNER_RTL2832_SI2157) {
/* check Si2168 ID register; reg=c8 val=80 */
@@ -988,6 +996,27 @@ static int rtl2832u_frontend_attach(struct dvb_usb_adapter
*adap)
goto err_slave_demod_failed;
}
+ dev->i2c_client_slave_demod = client;
+ } else if (dev->slave_demod == SLAVE_DEMOD_CXD2841ER) {
+ struct cxd2841er_config cxd2837er_config = {};
+ cxd2837er_config.i2c_addr = 0xd8;
+ cxd2837er_config.xtal = SONY_XTAL_20500;
+ cxd2837er_config.flags = CXD2841ER_AUTO_IFHZ|
CXD2841ER_EARLY_TUNE |
+ CXD2841ER_NO_WAIT_LOCK |
CXD2841ER_NO_AGCNEG |
+ CXD2841ER_TSBITS |
CXD2841ER_TS_SERIAL;
+
+ adap->fe[1] = dvb_attach( cxd2841er_attach_t_c,
&cxd2837er_config, &d->i2c_adap );
+ if (!adap->fe[1]) {
+ dev_err(&d->intf->dev, "CXD2837ER attach
failed!\n");
+ return -ENODEV;
+ }
+
+ if (!try_module_get(client->dev.driver->owner)) {
+ i2c_unregister_device(client);
+ dev->slave_demod = SLAVE_DEMOD_NONE;
+ goto err_slave_demod_failed;
+ }
+
dev->i2c_client_slave_demod = client;
} else {
struct si2168_config si2168_config = {};
@@ -1046,10 +1075,14 @@ static int rtl28xxu_frontend_detach(struct
dvb_usb_adapter *adap)
dev_dbg(&d->intf->dev, "\n");
/* remove I2C slave demod */
- client = dev->i2c_client_slave_demod;
- if (client) {
- module_put(client->dev.driver->owner);
- i2c_unregister_device(client);
+ if (dev->slave_demod == SLAVE_DEMOD_CXD2841ER) {
+ dev_info(&d->intf->dev,"Sony CXD2837ER detached
automatically.");
+ } else {
+ client = dev->i2c_client_slave_demod;
+ if (client) {
+ module_put(client->dev.driver->owner);
+ i2c_unregister_device(client);
+ }
}
/* remove I2C demod */
diff --git a/drivers/media/usb/dvb-usb-v2/rtl28xxu.h
b/drivers/media/usb/dvb-usb-v2/rtl28xxu.h
index 138062960a73..5a615d73fc34 100644
--- a/drivers/media/usb/dvb-usb-v2/rtl28xxu.h
+++ b/drivers/media/usb/dvb-usb-v2/rtl28xxu.h
@@ -43,6 +43,7 @@
#include "r820t.h"
#include "si2168.h"
#include "si2157.h"
+#include "cxd2841er.h"
/*
* USB commands
@@ -87,7 +88,8 @@ struct rtl28xxu_dev {
#define SLAVE_DEMOD_MN884721
[PATCH v4l-utils] Add missing linux/bpf_common.h
Copy from [1], needed by bpf.h. [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/plain/include/uapi/linux/bpf_common.h?h=v4.19 Signed-off-by: Peter Seiderer --- include/linux/bpf_common.h | 57 ++ 1 file changed, 57 insertions(+) create mode 100644 include/linux/bpf_common.h diff --git a/include/linux/bpf_common.h b/include/linux/bpf_common.h new file mode 100644 index ..ee97668b --- /dev/null +++ b/include/linux/bpf_common.h @@ -0,0 +1,57 @@ +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */ +#ifndef _UAPI__LINUX_BPF_COMMON_H__ +#define _UAPI__LINUX_BPF_COMMON_H__ + +/* Instruction classes */ +#define BPF_CLASS(code) ((code) & 0x07) +#defineBPF_LD 0x00 +#defineBPF_LDX 0x01 +#defineBPF_ST 0x02 +#defineBPF_STX 0x03 +#defineBPF_ALU 0x04 +#defineBPF_JMP 0x05 +#defineBPF_RET 0x06 +#defineBPF_MISC0x07 + +/* ld/ldx fields */ +#define BPF_SIZE(code) ((code) & 0x18) +#defineBPF_W 0x00 /* 32-bit */ +#defineBPF_H 0x08 /* 16-bit */ +#defineBPF_B 0x10 /* 8-bit */ +/* eBPFBPF_DW 0x1864-bit */ +#define BPF_MODE(code) ((code) & 0xe0) +#defineBPF_IMM 0x00 +#defineBPF_ABS 0x20 +#defineBPF_IND 0x40 +#defineBPF_MEM 0x60 +#defineBPF_LEN 0x80 +#defineBPF_MSH 0xa0 + +/* alu/jmp fields */ +#define BPF_OP(code)((code) & 0xf0) +#defineBPF_ADD 0x00 +#defineBPF_SUB 0x10 +#defineBPF_MUL 0x20 +#defineBPF_DIV 0x30 +#defineBPF_OR 0x40 +#defineBPF_AND 0x50 +#defineBPF_LSH 0x60 +#defineBPF_RSH 0x70 +#defineBPF_NEG 0x80 +#defineBPF_MOD 0x90 +#defineBPF_XOR 0xa0 + +#defineBPF_JA 0x00 +#defineBPF_JEQ 0x10 +#defineBPF_JGT 0x20 +#defineBPF_JGE 0x30 +#defineBPF_JSET0x40 +#define BPF_SRC(code) ((code) & 0x08) +#defineBPF_K 0x00 +#defineBPF_X 0x08 + +#ifndef BPF_MAXINSNS +#define BPF_MAXINSNS 4096 +#endif + +#endif /* _UAPI__LINUX_BPF_COMMON_H__ */ -- 2.19.1
[PATCH 2/2] media: imx-pxp: Check for pxp_soft_reset() error
pxp_soft_reset() may fail with a timeout, so it is better to propagate
the error in this case.
Signed-off-by: Fabio Estevam
---
drivers/media/platform/imx-pxp.c | 12 +---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/drivers/media/platform/imx-pxp.c b/drivers/media/platform/imx-pxp.c
index 27780f1..b3700b8 100644
--- a/drivers/media/platform/imx-pxp.c
+++ b/drivers/media/platform/imx-pxp.c
@@ -1607,7 +1607,7 @@ static const struct v4l2_m2m_ops m2m_ops = {
.job_abort = pxp_job_abort,
};
-static void pxp_soft_reset(struct pxp_dev *dev)
+static int pxp_soft_reset(struct pxp_dev *dev)
{
int ret;
u32 val;
@@ -1619,11 +1619,15 @@ static void pxp_soft_reset(struct pxp_dev *dev)
ret = readl_poll_timeout(dev->mmio + HW_PXP_CTRL, val,
val & BM_PXP_CTRL_CLKGATE, 0, 100);
- if (ret < 0)
+ if (ret < 0) {
pr_err("PXP reset timeout\n");
+ return ret;
+ }
writel(BM_PXP_CTRL_SFTRST, dev->mmio + HW_PXP_CTRL_CLR);
writel(BM_PXP_CTRL_CLKGATE, dev->mmio + HW_PXP_CTRL_CLR);
+
+ return 0;
}
static int pxp_probe(struct platform_device *pdev)
@@ -1670,7 +1674,9 @@ static int pxp_probe(struct platform_device *pdev)
if (ret < 0)
return ret;
- pxp_soft_reset(dev);
+ ret = pxp_soft_reset(dev);
+ if (ret < 0)
+ return ret;
spin_lock_init(&dev->irqlock);
--
2.7.4
[PATCH] media: imx-pxp: Check the return value from clk_prepare_enable()
clk_prepare_enable() may fail, so we should better check its return value and propagate it in the case of error. Signed-off-by: Fabio Estevam --- drivers/media/platform/imx-pxp.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/drivers/media/platform/imx-pxp.c b/drivers/media/platform/imx-pxp.c index b76cd0e..27780f1 100644 --- a/drivers/media/platform/imx-pxp.c +++ b/drivers/media/platform/imx-pxp.c @@ -1666,7 +1666,10 @@ static int pxp_probe(struct platform_device *pdev) return ret; } - clk_prepare_enable(dev->clk); + ret = clk_prepare_enable(dev->clk); + if (ret < 0) + return ret; + pxp_soft_reset(dev); spin_lock_init(&dev->irqlock); -- 2.7.4
[PATCH v2 2/3] media: imx-pxp: Check for pxp_soft_reset() error
pxp_soft_reset() may fail with a timeout, so it is better to propagate
the error in this case.
Signed-off-by: Fabio Estevam
---
Changes since v1:
- None
drivers/media/platform/imx-pxp.c | 12 +---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/drivers/media/platform/imx-pxp.c b/drivers/media/platform/imx-pxp.c
index 27780f1..b3700b8 100644
--- a/drivers/media/platform/imx-pxp.c
+++ b/drivers/media/platform/imx-pxp.c
@@ -1607,7 +1607,7 @@ static const struct v4l2_m2m_ops m2m_ops = {
.job_abort = pxp_job_abort,
};
-static void pxp_soft_reset(struct pxp_dev *dev)
+static int pxp_soft_reset(struct pxp_dev *dev)
{
int ret;
u32 val;
@@ -1619,11 +1619,15 @@ static void pxp_soft_reset(struct pxp_dev *dev)
ret = readl_poll_timeout(dev->mmio + HW_PXP_CTRL, val,
val & BM_PXP_CTRL_CLKGATE, 0, 100);
- if (ret < 0)
+ if (ret < 0) {
pr_err("PXP reset timeout\n");
+ return ret;
+ }
writel(BM_PXP_CTRL_SFTRST, dev->mmio + HW_PXP_CTRL_CLR);
writel(BM_PXP_CTRL_CLKGATE, dev->mmio + HW_PXP_CTRL_CLR);
+
+ return 0;
}
static int pxp_probe(struct platform_device *pdev)
@@ -1670,7 +1674,9 @@ static int pxp_probe(struct platform_device *pdev)
if (ret < 0)
return ret;
- pxp_soft_reset(dev);
+ ret = pxp_soft_reset(dev);
+ if (ret < 0)
+ return ret;
spin_lock_init(&dev->irqlock);
--
2.7.4
[PATCH v2 3/3] media: imx-pxp: Improve pxp_soft_reset() error message
Improve the pxp_soft_reset() error message by moving it to the
caller function, associating it with a proper device and also
by displaying the error code.
Signed-off-by: Fabio Estevam
---
Changes since v1:
- Newly introduced in this version
drivers/media/platform/imx-pxp.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/media/platform/imx-pxp.c b/drivers/media/platform/imx-pxp.c
index b3700b8..1b765c9 100644
--- a/drivers/media/platform/imx-pxp.c
+++ b/drivers/media/platform/imx-pxp.c
@@ -1619,10 +1619,8 @@ static int pxp_soft_reset(struct pxp_dev *dev)
ret = readl_poll_timeout(dev->mmio + HW_PXP_CTRL, val,
val & BM_PXP_CTRL_CLKGATE, 0, 100);
- if (ret < 0) {
- pr_err("PXP reset timeout\n");
+ if (ret < 0)
return ret;
- }
writel(BM_PXP_CTRL_SFTRST, dev->mmio + HW_PXP_CTRL_CLR);
writel(BM_PXP_CTRL_CLKGATE, dev->mmio + HW_PXP_CTRL_CLR);
@@ -1675,8 +1673,10 @@ static int pxp_probe(struct platform_device *pdev)
return ret;
ret = pxp_soft_reset(dev);
- if (ret < 0)
+ if (ret < 0) {
+ dev_err(&pdev->dev, "PXP reset timeout: %d\n", ret);
return ret;
+ }
spin_lock_init(&dev->irqlock);
--
2.7.4
[PATCH v2 1/3] media: imx-pxp: Check the return value from clk_prepare_enable()
clk_prepare_enable() may fail, so we should better check its return value and propagate it in the case of error. Signed-off-by: Fabio Estevam --- Changes since v1: - Properly enumerate the series drivers/media/platform/imx-pxp.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/drivers/media/platform/imx-pxp.c b/drivers/media/platform/imx-pxp.c index b76cd0e..27780f1 100644 --- a/drivers/media/platform/imx-pxp.c +++ b/drivers/media/platform/imx-pxp.c @@ -1666,7 +1666,10 @@ static int pxp_probe(struct platform_device *pdev) return ret; } - clk_prepare_enable(dev->clk); + ret = clk_prepare_enable(dev->clk); + if (ret < 0) + return ret; + pxp_soft_reset(dev); spin_lock_init(&dev->irqlock); -- 2.7.4
Re: [PATCH v2] Input: Add missing event codes for common IR remote buttons
On Sat, Nov 03, 2018 at 07:55:32AM -0700, Derek Kelly wrote: > The following patch adds event codes for common buttons found on various > provider and universal remote controls. They represent functions not > covered by existing event codes. Once added, rc_keymaps can be updated > accordingly where applicable. > > v2 changes: > Renamed KEY_SYSTEM to KEY_SYSTEM_MENU to avoid conflict with powerpc > KEY_SYSTEM define. > > Signed-off-by: Derek Kelly Reviewed-by: Sean Young There are many remotes with these buttons, this is a very useful addition. Thanks, Sean > --- > include/uapi/linux/input-event-codes.h | 13 + > 1 file changed, 13 insertions(+) > > diff --git a/include/uapi/linux/input-event-codes.h > b/include/uapi/linux/input-event-codes.h > index 53fbae27b280..a15fd3c944d2 100644 > --- a/include/uapi/linux/input-event-codes.h > +++ b/include/uapi/linux/input-event-codes.h > @@ -689,6 +689,19 @@ > #define BTN_TRIGGER_HAPPY39 0x2e6 > #define BTN_TRIGGER_HAPPY40 0x2e7 > > +/* Remote control buttons found across provider & universal remotes */ > +#define KEY_LIVE_TV 0x2e8 /* Jump to live tv viewing */ > +#define KEY_OPTIONS 0x2e9 /* Jump to options */ > +#define KEY_INTERACTIVE 0x2ea /* Jump to interactive > system/menu/item */ > +#define KEY_MIC_INPUT0x2eb /* Trigger MIC > input/listen mode */ > +#define KEY_SCREEN_INPUT 0x2ec /* Open on-screen input system > */ > +#define KEY_SYSTEM_MENU 0x2ed /* Open systems > menu/display */ > +#define KEY_SERVICES 0x2ee /* Access services */ > +#define KEY_DISPLAY_FORMAT 0x2ef /* Cycle display formats */ > +#define KEY_PIP 0x2f0 /* Toggle > Picture-in-Picture on/off */ > +#define KEY_PIP_SWAP 0x2f1 /* Swap contents between main > view and PIP window */ > +#define KEY_PIP_POSITION 0x2f2 /* Cycle PIP window position */ > + > /* We avoid low common keys in module aliases so they don't get huge. */ > #define KEY_MIN_INTERESTING KEY_MUTE > #define KEY_MAX 0x2ff > -- > 2.19.1
Re: [RFC] media: imx: queue subdevice events on the video device in the same pipeline
Hi Philipp,
Thanks, I've been meaning this too. Comments below.
On 11/5/18 7:03 AM, Philipp Zabel wrote:
While subdevice and video device are in the same pipeline, pass
subdevice events on to userspace via the video device node.
Signed-off-by: Philipp Zabel
---
This would allow to see source change events from the source subdevice
on the video device node, for example.
---
drivers/staging/media/imx/imx-media-dev.c | 18 ++
1 file changed, 18 insertions(+)
diff --git a/drivers/staging/media/imx/imx-media-dev.c
b/drivers/staging/media/imx/imx-media-dev.c
index 4b344a4a3706..2fe6fdf2faf1 100644
--- a/drivers/staging/media/imx/imx-media-dev.c
+++ b/drivers/staging/media/imx/imx-media-dev.c
@@ -442,6 +442,23 @@ static const struct media_device_ops imx_media_md_ops = {
.link_notify = imx_media_link_notify,
};
+static void imx_media_notify(struct v4l2_subdev *sd, unsigned int notification,
+void *arg)
+{
+ struct imx_media_dev *imxmd;
+ struct imx_media_video_dev *vdev;
+
+ imxmd = container_of(sd->v4l2_dev, struct imx_media_dev, v4l2_dev);
+ list_for_each_entry(vdev, &imxmd->vdev_list, list) {
+ if (sd->entity.pipe &&
+ sd->entity.pipe == vdev->vfd->entity.pipe &&
The problem with this check is that a sensor can be streaming to
multiple video capture devices simultaneously (to prpenc and prpvf). The
media framework doesn't support an entity being a member of multiple
pipelines (there's only one pipe object in 'struct media_entity') so the
pipe object ends up just pointing to whichever stream was started last
that included that entity. The result being the event will only get
queued to whichever video device who's stream was enabled last.
So I think there should be no if statement in the loop. Also it's better
to loop through the sub-devices vdev_list, because that list contains
only the video devices reachable from that sub-device. So the function
should read:
static void imx_media_notify(struct v4l2_subdev *sd,
unsigned int notification,
void *arg)
{
struct media_entity *entity = &sd->entity;
int i;
if (notification != V4L2_DEVICE_NOTIFY_EVENT)
return;
for (i = 0; i < entity->num_pads; i++) {
struct media_pad *pad = &entity->pads[i];
struct imx_media_pad_vdev *pad_vdev;
struct list_head *pad_vdev_list;
pad_vdev_list = to_pad_vdev_list(sd, pad->index);
if (!pad_vdev_list)
continue;
list_for_each_entry(pad_vdev, pad_vdev_list, list)
v4l2_event_queue(pad_vdev->vdev->vfd, arg);
}
}
I posted this to my media-tree fork, see
da05ccab97 ("media: imx: queue subdevice events to the reachable video devices")
and this Note in the commit header:
Note this will queue the event to a video device even if there is
no actual _enabled_ path from the sub-device to the video device.
So a future fix is to skip the video device if there is no enabled
path to it from the sub-device. The entity->pipe pointer can't be
used for this check because in imx-media a sub-device can be a
member to more than one streaming pipeline at a time.
Steve
+ notification == V4L2_DEVICE_NOTIFY_EVENT) {
+ v4l2_event_queue(vdev->vfd, arg);
+ break;
+ }
+ }
+}
+
static int imx_media_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
@@ -464,6 +481,7 @@ static int imx_media_probe(struct platform_device *pdev)
imxmd->v4l2_dev.mdev = &imxmd->md;
strscpy(imxmd->v4l2_dev.name, "imx-media",
sizeof(imxmd->v4l2_dev.name));
+ imxmd->v4l2_dev.notify = imx_media_notify;
media_device_init(&imxmd->md);
cron job: media_tree daily build: ERRORS
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: Tue Nov 6 05:00:10 CET 2018 media-tree git hash:dafb7f9aef2fd44991ff1691721ff765a23be27b media_build git hash: 0c8bb27f3aaa682b9548b656f77505c3d1f11e71 v4l-utils git hash: 0aa28f4293ee3306b34ea2866ef5f26fa75d2ed0 edid-decode git hash: 5eeb151a748788666534d6ea3da07f90400d24c2 gcc version:i686-linux-gcc (GCC) 8.2.0 sparse version: 0.5.2 smatch version: 0.5.1 host hardware: x86_64 host os:4.18.0-2-amd64 linux-git-arm-at91: WARNINGS linux-git-arm-davinci: WARNINGS linux-git-arm-multi: WARNINGS linux-git-arm-pxa: WARNINGS linux-git-arm-stm32: WARNINGS linux-git-arm64: WARNINGS linux-git-i686: WARNINGS linux-git-mips: OK linux-git-powerpc64: WARNINGS linux-git-sh: OK linux-git-x86_64: WARNINGS Check COMPILE_TEST: OK linux-3.10.108-i686: ERRORS linux-3.10.108-x86_64: ERRORS linux-3.11.10-i686: ERRORS linux-3.11.10-x86_64: ERRORS linux-3.12.74-i686: ERRORS linux-3.12.74-x86_64: ERRORS linux-3.13.11-i686: ERRORS linux-3.13.11-x86_64: ERRORS linux-3.14.79-i686: ERRORS linux-3.14.79-x86_64: ERRORS linux-3.15.10-i686: ERRORS linux-3.15.10-x86_64: ERRORS linux-3.16.57-i686: ERRORS linux-3.16.57-x86_64: ERRORS linux-3.17.8-i686: ERRORS linux-3.17.8-x86_64: ERRORS linux-3.18.123-i686: ERRORS linux-3.18.123-x86_64: ERRORS linux-3.19.8-i686: ERRORS linux-3.19.8-x86_64: ERRORS linux-4.0.9-i686: ERRORS linux-4.0.9-x86_64: ERRORS linux-4.1.52-i686: ERRORS linux-4.1.52-x86_64: ERRORS linux-4.2.8-i686: ERRORS linux-4.2.8-x86_64: ERRORS linux-4.3.6-i686: ERRORS linux-4.3.6-x86_64: ERRORS linux-4.4.159-i686: ERRORS linux-4.4.159-x86_64: ERRORS linux-4.5.7-i686: ERRORS linux-4.5.7-x86_64: ERRORS linux-4.6.7-i686: ERRORS linux-4.6.7-x86_64: ERRORS linux-4.7.10-i686: ERRORS linux-4.7.10-x86_64: ERRORS linux-4.8.17-i686: ERRORS linux-4.8.17-x86_64: ERRORS linux-4.9.131-i686: ERRORS linux-4.9.131-x86_64: ERRORS linux-4.10.17-i686: ERRORS linux-4.10.17-x86_64: ERRORS linux-4.11.12-i686: ERRORS linux-4.11.12-x86_64: ERRORS linux-4.12.14-i686: ERRORS linux-4.12.14-x86_64: ERRORS linux-4.13.16-i686: OK linux-4.13.16-x86_64: OK linux-4.14.74-i686: OK linux-4.14.74-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.12-i686: OK linux-4.18.12-x86_64: OK linux-4.19-i686: OK linux-4.19-x86_64: OK apps: OK spec-git: OK sparse: WARNINGS Logs weren't copied as they are too large (1692 kB) The Media Infrastructure API from this daily build is here: http://www.xs4all.nl/~hverkuil/spec/index.html
RE: [PATCH v7 06/16] intel-ipu3: mmu: Implement driver
Hi, Sakari,
Thanks for the feedback.
> -Original Message-
> From: Sakari Ailus [mailto:sakari.ai...@linux.intel.com]
> Sent: Monday, November 5, 2018 3:55 AM
> To: Zhi, Yong
> Cc: linux-media@vger.kernel.org; tf...@chromium.org;
> mche...@kernel.org; hans.verk...@cisco.com;
> laurent.pinch...@ideasonboard.com; Mani, Rajmohan
> ; Zheng, Jian Xu ; Hu,
> Jerry W ; Toivonen, Tuukka
> ; Qiu, Tian Shu ; Cao,
> Bingbu
> Subject: Re: [PATCH v7 06/16] intel-ipu3: mmu: Implement driver
>
> Hi Yong,
>
> On Mon, Oct 29, 2018 at 03:23:00PM -0700, Yong Zhi wrote:
> > From: Tomasz Figa
> >
> > This driver translates IO virtual address to physical address based on
> > two levels page tables.
> >
> > Signed-off-by: Tomasz Figa
> > Signed-off-by: Yong Zhi
> > ---
>
> ...
>
> > +static void call_if_ipu3_is_powered(struct ipu3_mmu *mmu,
> > + void (*func)(struct ipu3_mmu *mmu)) {
> > + pm_runtime_get_noresume(mmu->dev);
> > + if (pm_runtime_active(mmu->dev))
> > + func(mmu);
> > + pm_runtime_put(mmu->dev);
>
> How about:
>
> if (!pm_runtime_get_if_in_use(mmu->dev))
> return;
>
> func(mmu);
> pm_runtime_put(mmu->dev);
>
Ack, unless Tomasz has different opinion.
>
> > +}
>
> --
> Sakari Ailus
> sakari.ai...@linux.intel.com
Re: [PATCH v7 06/16] intel-ipu3: mmu: Implement driver
On Tue, Nov 6, 2018 at 2:50 PM Zhi, Yong wrote:
>
> Hi, Sakari,
>
> Thanks for the feedback.
>
> > -Original Message-
> > From: Sakari Ailus [mailto:sakari.ai...@linux.intel.com]
> > Sent: Monday, November 5, 2018 3:55 AM
> > To: Zhi, Yong
> > Cc: linux-media@vger.kernel.org; tf...@chromium.org;
> > mche...@kernel.org; hans.verk...@cisco.com;
> > laurent.pinch...@ideasonboard.com; Mani, Rajmohan
> > ; Zheng, Jian Xu ; Hu,
> > Jerry W ; Toivonen, Tuukka
> > ; Qiu, Tian Shu ; Cao,
> > Bingbu
> > Subject: Re: [PATCH v7 06/16] intel-ipu3: mmu: Implement driver
> >
> > Hi Yong,
> >
> > On Mon, Oct 29, 2018 at 03:23:00PM -0700, Yong Zhi wrote:
> > > From: Tomasz Figa
> > >
> > > This driver translates IO virtual address to physical address based on
> > > two levels page tables.
> > >
> > > Signed-off-by: Tomasz Figa
> > > Signed-off-by: Yong Zhi
> > > ---
> >
> > ...
> >
> > > +static void call_if_ipu3_is_powered(struct ipu3_mmu *mmu,
> > > + void (*func)(struct ipu3_mmu *mmu)) {
> > > + pm_runtime_get_noresume(mmu->dev);
> > > + if (pm_runtime_active(mmu->dev))
> > > + func(mmu);
> > > + pm_runtime_put(mmu->dev);
> >
> > How about:
> >
> > if (!pm_runtime_get_if_in_use(mmu->dev))
> > return;
> >
> > func(mmu);
> > pm_runtime_put(mmu->dev);
> >
>
> Ack, unless Tomasz has different opinion.
It's actually the proper way of doing it. Thanks for the suggestion.
Best regards,
Tomasz
[PATCH v2 1/1] v4l: event: Add subscription to list before calling "add" operation
Patch ad608fbcf166 changed how events were subscribed to address an issue
elsewhere. As a side effect of that change, the "add" callback was called
before the event subscription was added to the list of subscribed events,
causing the first event (and possibly other events arriving soon
afterwards) to be lost.
Fix this by adding the subscription to the list before calling the "add"
callback, and clean up afterwards if that fails.
Fixes: ad608fbcf166 ("media: v4l: event: Prevent freeing event subscriptions
while accessed")
Reported-by: Dave Stevenson
Signed-off-by: Sakari Ailus
---
Hi Dave, Hans,
I figured there was room for some refactoring... so I did that.
Functionality-wise it should be equivalent.
drivers/media/v4l2-core/v4l2-event.c | 43
1 file changed, 24 insertions(+), 19 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-event.c
b/drivers/media/v4l2-core/v4l2-event.c
index a3ef1f50a4b3..481e3c65cf97 100644
--- a/drivers/media/v4l2-core/v4l2-event.c
+++ b/drivers/media/v4l2-core/v4l2-event.c
@@ -193,6 +193,22 @@ int v4l2_event_pending(struct v4l2_fh *fh)
}
EXPORT_SYMBOL_GPL(v4l2_event_pending);
+static void __v4l2_event_unsubscribe(struct v4l2_subscribed_event *sev)
+{
+ struct v4l2_fh *fh = sev->fh;
+ unsigned int i;
+
+ lockdep_assert_held(&fh->subscribe_lock);
+ assert_spin_locked(&fh->vdev->fh_lock);
+
+ /* Remove any pending events for this subscription */
+ for (i = 0; i < sev->in_use; i++) {
+ list_del(&sev->events[sev_pos(sev, i)].list);
+ fh->navailable--;
+ }
+ list_del(&sev->list);
+}
+
int v4l2_event_subscribe(struct v4l2_fh *fh,
const struct v4l2_event_subscription *sub, unsigned
elems,
const struct v4l2_subscribed_event_ops *ops)
@@ -224,27 +240,23 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
spin_lock_irqsave(&fh->vdev->fh_lock, flags);
found_ev = v4l2_event_subscribed(fh, sub->type, sub->id);
+ if (!found_ev)
+ list_add(&sev->list, &fh->subscribed);
spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
if (found_ev) {
/* Already listening */
kvfree(sev);
- goto out_unlock;
- }
-
- if (sev->ops && sev->ops->add) {
+ } else if (sev->ops && sev->ops->add) {
ret = sev->ops->add(sev, elems);
if (ret) {
+ spin_lock_irqsave(&fh->vdev->fh_lock, flags);
+ __v4l2_event_unsubscribe(sev);
+ spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
kvfree(sev);
- goto out_unlock;
}
}
- spin_lock_irqsave(&fh->vdev->fh_lock, flags);
- list_add(&sev->list, &fh->subscribed);
- spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
-
-out_unlock:
mutex_unlock(&fh->subscribe_lock);
return ret;
@@ -279,7 +291,6 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
{
struct v4l2_subscribed_event *sev;
unsigned long flags;
- int i;
if (sub->type == V4L2_EVENT_ALL) {
v4l2_event_unsubscribe_all(fh);
@@ -291,14 +302,8 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
spin_lock_irqsave(&fh->vdev->fh_lock, flags);
sev = v4l2_event_subscribed(fh, sub->type, sub->id);
- if (sev != NULL) {
- /* Remove any pending events for this subscription */
- for (i = 0; i < sev->in_use; i++) {
- list_del(&sev->events[sev_pos(sev, i)].list);
- fh->navailable--;
- }
- list_del(&sev->list);
- }
+ if (sev != NULL)
+ __v4l2_event_unsubscribe(sev);
spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
--
2.11.0
