Re: [PATCH v4 03/11] gpu: ipu-v3: Add planar support to interlaced scan
On Thu, 2018-10-04 at 11:53 -0700, Steve Longerbeam wrote:
> To support interlaced scan with planar formats, cpmem SLUV must
> be programmed with the correct chroma line stride. For full and
> partial planar 4:2:2 (YUV422P, NV16), chroma line stride must
> be doubled. For full and partial planar 4:2:0 (YUV420, YVU420, NV12),
> chroma line stride must _not_ be doubled, since a single chroma line
> is shared by two luma lines.
>
> Signed-off-by: Steve Longerbeam
> ---
> drivers/gpu/ipu-v3/ipu-cpmem.c | 26 +++--
> drivers/staging/media/imx/imx-ic-prpencvf.c | 3 ++-
> drivers/staging/media/imx/imx-media-csi.c | 3 ++-
> include/video/imx-ipu-v3.h | 3 ++-
> 4 files changed, 30 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/ipu-v3/ipu-cpmem.c b/drivers/gpu/ipu-v3/ipu-cpmem.c
> index a9d2501500a1..d41df8034c5b 100644
> --- a/drivers/gpu/ipu-v3/ipu-cpmem.c
> +++ b/drivers/gpu/ipu-v3/ipu-cpmem.c
> @@ -273,9 +273,10 @@ void ipu_cpmem_set_uv_offset(struct ipuv3_channel *ch,
> u32 u_off, u32 v_off)
> }
> EXPORT_SYMBOL_GPL(ipu_cpmem_set_uv_offset);
>
> -void ipu_cpmem_interlaced_scan(struct ipuv3_channel *ch, int stride)
> +void ipu_cpmem_interlaced_scan(struct ipuv3_channel *ch, int stride,
> +u32 pixelformat)
> {
> - u32 ilo, sly;
> + u32 ilo, sly, sluv;
>
> if (stride < 0) {
> stride = -stride;
> @@ -286,9 +287,30 @@ void ipu_cpmem_interlaced_scan(struct ipuv3_channel *ch,
> int stride)
>
> sly = (stride * 2) - 1;
>
> + switch (pixelformat) {
> + case V4L2_PIX_FMT_YUV420:
> + case V4L2_PIX_FMT_YVU420:
> + sluv = stride / 2 - 1;
> + break;
> + case V4L2_PIX_FMT_NV12:
> + sluv = stride - 1;
> + break;
> + case V4L2_PIX_FMT_YUV422P:
> + sluv = stride - 1;
> + break;
> + case V4L2_PIX_FMT_NV16:
> + sluv = stride * 2 - 1;
> + break;
> + default:
> + sluv = 0;
> + break;
> + }
> +
> ipu_ch_param_write_field(ch, IPU_FIELD_SO, 1);
> ipu_ch_param_write_field(ch, IPU_FIELD_ILO, ilo);
> ipu_ch_param_write_field(ch, IPU_FIELD_SLY, sly);
> + if (sluv)
> + ipu_ch_param_write_field(ch, IPU_FIELD_SLUV, sluv);
> };
> EXPORT_SYMBOL_GPL(ipu_cpmem_interlaced_scan);
[...]
Reviewed-by: Philipp Zabel
and
Acked-by: Philipp Zabel
to be merged with the rest of the series via the media tree. I'll take
care not to introduce nontrivial conflicts in imx-drm.
regards
Philipp
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 02/11] gpu: ipu-csi: Swap fields according to input/output field types
Hi Steve,
On Thu, 2018-10-04 at 11:53 -0700, Steve Longerbeam wrote:
[...]
> int ipu_csi_init_interface(struct ipu_csi *csi,
> struct v4l2_mbus_config *mbus_cfg,
> -struct v4l2_mbus_framefmt *mbus_fmt)
> +struct v4l2_mbus_framefmt *infmt,
> +struct v4l2_mbus_framefmt *outfmt)
> {
[...]
> + std = V4L2_STD_UNKNOWN;
> + if (width == 720) {
> + switch (height) {
> + case 480:
> + std = V4L2_STD_NTSC;
> + break;
> + case 576:
> + std = V4L2_STD_PAL;
> + break;
> + default:
> + break;
> + }
> + }
> +
> + if (std == V4L2_STD_UNKNOWN) {
> dev_err(csi->ipu->dev,
[...]
> + "Unsupported interlaced video mode\n");
> + ret = -EINVAL;
> + goto out_unlock;
> }
[...]
> +
> + /* framelines for NTSC / PAL */
> + height = (std & V4L2_STD_525_60) ? 525 : 625;
I think this is a bit convoluted. Instead of initializing std, then
possibly changing it, and then comparing to the inital value, and then
checking it again to determine the new height, why not just:
if (width == 720 && height == 480) {
std = V4L2_STD_NTSC;
height = 525;
} else if (width == 720 && height == 576) {
std = V4L2_STD_PAL;
height = 625;
} else {
dev_err(csi->ipu->dev,
"Unsupported interlaced video mode\n");
ret = -EINVAL;
goto out_unlock;
}
?
> break;
> case IPU_CSI_CLK_MODE_CCIR1120_PROGRESSIVE_DDR:
> case IPU_CSI_CLK_MODE_CCIR1120_PROGRESSIVE_SDR:
> @@ -476,9 +529,10 @@ int ipu_csi_init_interface(struct ipu_csi *csi,
> dev_dbg(csi->ipu->dev, "CSI_ACT_FRM_SIZE = 0x%08X\n",
> ipu_csi_read(csi, CSI_ACT_FRM_SIZE));
>
> +out_unlock:
> spin_unlock_irqrestore(&csi->lock, flags);
>
> - return 0;
> + return ret;
> }
> EXPORT_SYMBOL_GPL(ipu_csi_init_interface);
>
> diff --git a/drivers/staging/media/imx/imx-media-csi.c
> b/drivers/staging/media/imx/imx-media-csi.c
> index 4acdd7ae612b..ad66f007d395 100644
> --- a/drivers/staging/media/imx/imx-media-csi.c
> +++ b/drivers/staging/media/imx/imx-media-csi.c
> @@ -666,7 +666,6 @@ static int csi_setup(struct csi_priv *priv)
> struct v4l2_mbus_framefmt *infmt, *outfmt;
> const struct imx_media_pixfmt *incc;
> struct v4l2_mbus_config mbus_cfg;
> - struct v4l2_mbus_framefmt if_fmt;
> struct v4l2_rect crop;
>
> infmt = &priv->format_mbus[CSI_SINK_PAD];
> @@ -679,22 +678,14 @@ static int csi_setup(struct csi_priv *priv)
> priv->upstream_ep.bus.parallel.flags :
> priv->upstream_ep.bus.mipi_csi2.flags;
>
> - /*
> - * we need to pass input frame to CSI interface, but
> - * with translated field type from output format
> - */
> - if_fmt = *infmt;
> - if_fmt.field = outfmt->field;
> crop = priv->crop;
>
> /*
>* if cycles is set, we need to handle this over multiple cycles as
>* generic/bayer data
>*/
> - if (is_parallel_bus(&priv->upstream_ep) && incc->cycles) {
> - if_fmt.width *= incc->cycles;
If the input format width passed to ipu_csi_init_interface is not
multiplied by the number of cycles per pixel anymore, width in the
CSI_SENS_FRM_SIZE register will be set to the unmultiplied value from
infmt.
This breaks 779680e2e793 ("media: imx: add support for RGB565_2X8 on
parallel bus").
> + if (is_parallel_bus(&priv->upstream_ep) && incc->cycles)
> crop.width *= incc->cycles;
> - }
>
> ipu_csi_set_window(priv->csi, &crop);
>
> @@ -702,7 +693,7 @@ static int csi_setup(struct csi_priv *priv)
>priv->crop.width == 2 * priv->compose.width,
>priv->crop.height == 2 * priv->compose.height);
>
> - ipu_csi_init_interface(priv->csi, &mbus_cfg, &if_fmt);
> + ipu_csi_init_interface(priv->csi, &mbus_cfg, infmt, outfmt);
regards
Philipp
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 04/11] media: imx: Fix field negotiation
On Thu, 2018-10-04 at 11:53 -0700, Steve Longerbeam wrote: > IDMAC interlaced scan, a.k.a. interweave, should be enabled in the > IDMAC output channels only if the IDMAC output pad field type is > 'seq-bt' or 'seq-tb', and field type at the capture interface is > 'interlaced*'. > > V4L2_FIELD_HAS_BOTH() macro should not be used on the input to determine > enabling interlaced/interweave scan. That macro includes the 'interlaced' > field types, and in those cases the data is already interweaved with > top/bottom field lines. > > The CSI will capture whole frames when the source specifies alternate > field mode. So the CSI also enables interweave for alternate input > field type and the field type at capture interface is interlaced. > > Fix the logic for setting field type in try_fmt in CSI entity. > The behavior should be: > > - No restrictions on field type at sink pad. > > - At the output pads, allow sequential fields in TB order, if the sink pad > field type is sequential or alternate. Otherwise passthrough the field > type from sink to source pad. > > Move this logic to new function csi_try_field(). > > These changes result in the following allowed field transformations > from CSI sink -> source pads (all other field types at sink are passed > through to source): > > seq-tb -> seq-tb > seq-bt -> seq-tb > alternate -> seq-tb > > In a future patch, the CSI sink -> source will allow: > > seq-tb -> seq-bt > seq-bt -> seq-bt > alternate -> seq-bt > > This will require supporting interweave with top/bottom line swapping. > Until then seq-bt is not allowed at the CSI source pad because there is > no way to swap top/bottom lines when interweaving to INTERLACED_BT -- > note that despite the name, INTERLACED_BT is top-bottom order in memory. > The BT in this case refers to field dominance: the bottom lines are > older in time than the top lines. > > The capture interface device allows selecting IDMAC interweave by > choosing INTERLACED_TB if the CSI/PRPENCVF source pad is seq-tb and > INTERLACED_BT if the source pad is seq-bt (for future support of seq-bt). > > Signed-off-by: Steve Longerbeam Reviewed-by: Philipp Zabel regards Philipp ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 05/11] media: imx-csi: Double crop height for alternate fields at sink
On Thu, 2018-10-04 at 11:53 -0700, Steve Longerbeam wrote: > If the incoming sink field type is alternate, the reset crop height > and crop height bounds must be set to twice the incoming height, > because in alternate field mode, upstream will report only the > lines for a single field, and the CSI captures the whole frame. > > Signed-off-by: Steve Longerbeam Reviewed-by: Philipp Zabel regards Philipp ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 07/11] media: imx-csi: Allow skipping odd chroma rows for YVU420
On Thu, 2018-10-04 at 11:53 -0700, Steve Longerbeam wrote: > Skip writing U/V components to odd rows for YVU420 in addition to > YUV420 and NV12. > > Signed-off-by: Steve Longerbeam > Reviewed-by: Philipp Zabel > --- > drivers/staging/media/imx/imx-media-csi.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/staging/media/imx/imx-media-csi.c > b/drivers/staging/media/imx/imx-media-csi.c > index 4b075bc949de..6dd53a8e35d2 100644 > --- a/drivers/staging/media/imx/imx-media-csi.c > +++ b/drivers/staging/media/imx/imx-media-csi.c > @@ -452,6 +452,7 @@ static int csi_idmac_setup_channel(struct csi_priv *priv) > passthrough_bits = 16; > break; > case V4L2_PIX_FMT_YUV420: > + case V4L2_PIX_FMT_YVU420: > case V4L2_PIX_FMT_NV12: > burst_size = (image.pix.width & 0x3f) ? > ((image.pix.width & 0x1f) ? Reviewed-by: Philipp Zabel regards Philipp ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 06/11] media: imx: interweave and odd-chroma-row skip are incompatible
On Thu, 2018-10-04 at 11:53 -0700, Steve Longerbeam wrote: > If IDMAC interweaving is enabled in a write channel, the channel must > write the odd chroma rows for 4:2:0 formats. Skipping writing the odd > chroma rows produces corrupted captured 4:2:0 images when interweave > is enabled. > > Reported-by: Krzysztof Hałasa > Signed-off-by: Steve Longerbeam Reviewed-by: Philipp Zabel regards Philipp ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 09/11] media: imx-csi: Move crop/compose reset after filling default mbus fields
On Thu, 2018-10-04 at 11:53 -0700, Steve Longerbeam wrote: > If caller passes un-initialized field type V4L2_FIELD_ANY to CSI > sink pad, the reset CSI crop window would not be correct, because > the crop window depends on a valid input field type. To fix move > the reset of crop and compose windows to after the call to > imx_media_fill_default_mbus_fields(). > > Signed-off-by: Steve Longerbeam Reviewed-by: Philipp Zabel regards Philipp ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 10/11] media: imx: Allow interweave with top/bottom lines swapped
Hi Steve,
On Thu, 2018-10-04 at 11:54 -0700, Steve Longerbeam wrote:
> Allow sequential->interlaced interweaving but with top/bottom
> lines swapped to the output buffer.
>
> This can be accomplished by adding one line length to IDMAC output
> channel address, with a negative line length for the interlace offset.
>
> This is to allow the seq-bt -> interlaced-bt transformation, where
> bottom lines are still dominant (older in time) but with top lines
> first in the interweaved output buffer.
>
> With this support, the CSI can now allow seq-bt at its source pads,
> e.g. the following transformations are allowed in CSI from sink to
> source:
>
> seq-tb -> seq-bt
> seq-bt -> seq-bt
> alternate -> seq-bt
>
> Suggested-by: Philipp Zabel
> Signed-off-by: Steve Longerbeam
> ---
> drivers/staging/media/imx/imx-ic-prpencvf.c | 17 +++-
> drivers/staging/media/imx/imx-media-csi.c | 46 +
> 2 files changed, 53 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/staging/media/imx/imx-ic-prpencvf.c
> b/drivers/staging/media/imx/imx-ic-prpencvf.c
> index cf76b0432371..1499b0c62d74 100644
> --- a/drivers/staging/media/imx/imx-ic-prpencvf.c
> +++ b/drivers/staging/media/imx/imx-ic-prpencvf.c
> @@ -106,6 +106,8 @@ struct prp_priv {
> u32 frame_sequence; /* frame sequence counter */
> bool last_eof; /* waiting for last EOF at stream off */
> bool nfb4eof;/* NFB4EOF encountered during streaming */
> + u32 interweave_offset; /* interweave line offset to swap
> + top/bottom lines */
We have to store this instead of using vdev->fmt.fmt.bytesperline
because this potentially is the pre-rotation stride instead?
> struct completion last_eof_comp;
> };
>
> @@ -235,6 +237,9 @@ static void prp_vb2_buf_done(struct prp_priv *priv,
> struct ipuv3_channel *ch)
> if (ipu_idmac_buffer_is_ready(ch, priv->ipu_buf_num))
> ipu_idmac_clear_buffer(ch, priv->ipu_buf_num);
>
> + if (ch == priv->out_ch)
> + phys += priv->interweave_offset;
> +
> ipu_cpmem_set_buffer(ch, priv->ipu_buf_num, phys);
> }
>
> @@ -388,6 +393,13 @@ static int prp_setup_channel(struct prp_priv *priv,
> (image.pix.width * outcc->bpp) >> 3;
> }
>
> + priv->interweave_offset = 0;
> +
> + if (interweave && image.pix.field == V4L2_FIELD_INTERLACED_BT) {
> + image.rect.top = 1;
> + priv->interweave_offset = image.pix.bytesperline;
> + }
> +
> image.phys0 = addr0;
> image.phys1 = addr1;
>
> @@ -425,7 +437,10 @@ static int prp_setup_channel(struct prp_priv *priv,
> ipu_cpmem_set_rotation(channel, rot_mode);
>
> if (interweave)
> - ipu_cpmem_interlaced_scan(channel, image.pix.bytesperline,
> + ipu_cpmem_interlaced_scan(channel,
> + priv->interweave_offset ?
> + -image.pix.bytesperline :
> + image.pix.bytesperline,
> image.pix.pixelformat);
>
> ret = ipu_ic_task_idma_init(priv->ic, channel,
> diff --git a/drivers/staging/media/imx/imx-media-csi.c
> b/drivers/staging/media/imx/imx-media-csi.c
> index 679295da5dde..592f7d6edec1 100644
> --- a/drivers/staging/media/imx/imx-media-csi.c
> +++ b/drivers/staging/media/imx/imx-media-csi.c
> @@ -114,6 +114,8 @@ struct csi_priv {
> u32 frame_sequence; /* frame sequence counter */
> bool last_eof; /* waiting for last EOF at stream off */
> bool nfb4eof;/* NFB4EOF encountered during streaming */
> + u32 interweave_offset; /* interweave line offset to swap
> + top/bottom lines */
This doesn't seem necessary. Since there is no rotation here, the offset
is just vdev->fmt.fmt.pix.bytesperline if interweave_swap is enabled.
Maybe turn this into a bool interweave_swap?
> struct completion last_eof_comp;
> };
>
> @@ -286,7 +288,8 @@ static void csi_vb2_buf_done(struct csi_priv *priv)
> if (ipu_idmac_buffer_is_ready(priv->idmac_ch, priv->ipu_buf_num))
> ipu_idmac_clear_buffer(priv->idmac_ch, priv->ipu_buf_num);
>
> - ipu_cpmem_set_buffer(priv->idmac_ch, priv->ipu_buf_num, phys);
> + ipu_cpmem_set_buffer(priv->idmac_ch, priv->ipu_buf_num,
> + phys + priv->interweave_offset);
> }
>
> static irqreturn_t csi_idmac_eof_interrupt(int irq, void *dev_id)
> @@ -396,10 +399,10 @@ static void csi_idmac_
Re: [PATCH v3 0/5] Reset driver for IMX8MQ VPU hardware block
Hi Benjamin, On Mon, 2021-03-01 at 16:17 +0100, Benjamin Gaignard wrote: > The two VPUs inside IMX8MQ share the same control block which can be see > as a reset hardware block. This isn't a reset controller though. The control block also contains clock gates of some sort and a filter register for the featureset fuses. Those shouldn't be manipulated via the reset API. > In order to be able to add the second VPU (for HECV decoding) it will be > more handy if the both VPU drivers instance don't have to share the > control block registers. This lead to implement it as an independ reset > driver and to change the VPU driver to use it. Why not switch to a syscon regmap for the control block? That should also allow to keep backwards compatibility with the old binding with minimal effort. > Please note that this series break the compatibility between the DTB and > kernel. This break is limited to IMX8MQ SoC and is done when the driver > is still in staging directory. I know in this case we are pretty sure there are no users of this binding except for a staging driver, but it would still be nice to keep support for the deprecated binding, to avoid the requirement of updating kernel and DT in lock-step. regards Philipp ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 3/5] reset: Add reset driver for IMX8MQ VPU block
On Mon, 2021-03-01 at 16:17 +0100, Benjamin Gaignard wrote:
> IMX8MQ SoC got a dedicated hardware block to reset the video processor
> units (G1 and G2).
>
> Signed-off-by: Benjamin Gaignard
> ---
> drivers/reset/Kconfig| 8 ++
> drivers/reset/Makefile | 1 +
> drivers/reset/reset-imx8mq-vpu.c | 169 +++
> 3 files changed, 178 insertions(+)
> create mode 100644 drivers/reset/reset-imx8mq-vpu.c
>
> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> index 71ab75a46491..fa95380b271a 100644
> --- a/drivers/reset/Kconfig
> +++ b/drivers/reset/Kconfig
> @@ -80,6 +80,14 @@ config RESET_IMX7
> help
> This enables the reset controller driver for i.MX7 SoCs.
>
> +config RESET_VPU_IMX8MQ
> + tristate "i.MX8MQ VPU Reset Driver"
> + depends on HAS_IOMEM
> + depends on (ARM64 && ARCH_MXC) || COMPILE_TEST
> + select MFD_SYSCON
> + help
> + This enables the VPU reset controller driver for i.MX8MQ SoCs.
> +
> config RESET_INTEL_GW
> bool "Intel Reset Controller Driver"
> depends on OF && HAS_IOMEM
> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
> index 1054123fd187..6007e0cdfc05 100644
> --- a/drivers/reset/Makefile
> +++ b/drivers/reset/Makefile
> @@ -12,6 +12,7 @@ obj-$(CONFIG_RESET_BRCMSTB) += reset-brcmstb.o
> obj-$(CONFIG_RESET_BRCMSTB_RESCAL) += reset-brcmstb-rescal.o
> obj-$(CONFIG_RESET_HSDK) += reset-hsdk.o
> obj-$(CONFIG_RESET_IMX7) += reset-imx7.o
> +obj-$(CONFIG_RESET_VPU_IMX8MQ) += reset-imx8mq-vpu.o
> obj-$(CONFIG_RESET_INTEL_GW) += reset-intel-gw.o
> obj-$(CONFIG_RESET_LANTIQ) += reset-lantiq.o
> obj-$(CONFIG_RESET_LPC18XX) += reset-lpc18xx.o
> diff --git a/drivers/reset/reset-imx8mq-vpu.c
> b/drivers/reset/reset-imx8mq-vpu.c
> new file mode 100644
> index ..14c589f19266
> --- /dev/null
> +++ b/drivers/reset/reset-imx8mq-vpu.c
> @@ -0,0 +1,169 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2021, Collabora
> + *
> + * i.MX8MQ VPU Reset Controller driver
> + *
> + * Author: Benjamin Gaignard
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define CTRL_SOFT_RESET 0x00
> +#define RESET_G1 ((u32)BIT(1))
> +#define RESET_G2 ((u32)BIT(0))
> +
> +#define CTRL_ENABLE 0x04
> +#define ENABLE_G1BIT(1)
> +#define ENABLE_G2BIT(0)
> +
> +#define CTRL_G1_DEC_FUSE 0x08
> +#define CTRL_G1_PP_FUSE 0x0c
> +#define CTRL_G2_DEC_FUSE 0x10
> +
> +struct imx8mq_vpu_reset {
> + struct reset_controller_dev rcdev;
> + struct regmap *regmap;
> + struct clk_bulk_data *clocks;
> + int num_clocks;
> + struct device *dev;
> +};
> +
> +static inline struct imx8mq_vpu_reset *to_imx8mq_vpu_reset(struct
> reset_controller_dev *rcdev)
> +{
> + return container_of(rcdev, struct imx8mq_vpu_reset, rcdev);
> +}
> +
> +static int imx8mq_vpu_reset_assert(struct reset_controller_dev *rcdev,
> +unsigned long id)
> +{
> + struct imx8mq_vpu_reset *reset = to_imx8mq_vpu_reset(rcdev);
> + int ret = -EINVAL;
> +
> + ret = clk_bulk_prepare_enable(reset->num_clocks, reset->clocks);
> + if (ret) {
> + dev_err(reset->dev, "Failed to prepare clocks\n");
> + return ret;
> + }
> +
> + switch (id) {
> + case IMX8MQ_RESET_VPU_RESET_G1:
> + ret = regmap_update_bits(reset->regmap, CTRL_SOFT_RESET,
> RESET_G1, ~RESET_G1);
> + ret |= regmap_update_bits(reset->regmap, CTRL_ENABLE,
> ENABLE_G1, ENABLE_G1);
> + break;
> + case IMX8MQ_RESET_VPU_RESET_G2:
> + ret = regmap_update_bits(reset->regmap, CTRL_SOFT_RESET,
> RESET_G2, ~RESET_G2);
> + ret |= regmap_update_bits(reset->regmap, CTRL_ENABLE,
> ENABLE_G2, ENABLE_G2);
This doesn't belong in reset_assert.
> + break;
> + }
> +
> + /* Set values of the fuse registers */
> + ret |= regmap_write(reset->regmap, CTRL_G1_DEC_FUSE, 0x);
> + ret |= regmap_write(reset->regmap, CTRL_G1_PP_FUSE, 0x);
> + ret |= regmap_write(reset->regmap, CTRL_G2_DEC_FUSE, 0x);
Same as above, this doesn't belong in reset_assert.
> + clk_bulk_disable_unprepare(reset->num_clocks, reset->clocks);
Also I assume that only the VPU_DEC_ROOT clock is required to control
these registers. Enabling the VPU_G1_ROOT and VPU_G2_ROOT clocks
(presumably to make sure the resets propagate into the respective VPU
core) would be the reset consumer's responsibility.
regards
Philipp
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 4/5] media: hantro: Use reset driver
On Mon, 2021-03-01 at 16:17 +0100, Benjamin Gaignard wrote:
> Rather use a reset like feature inside the driver use the reset
> controller API to get the same result.
>
> Signed-off-by: Benjamin Gaignard
> ---
> drivers/staging/media/hantro/Kconfig| 1 +
> drivers/staging/media/hantro/imx8m_vpu_hw.c | 61 -
> 2 files changed, 12 insertions(+), 50 deletions(-)
>
> diff --git a/drivers/staging/media/hantro/Kconfig
> b/drivers/staging/media/hantro/Kconfig
> index 5b6cf9f62b1a..dd1d4dde2658 100644
> --- a/drivers/staging/media/hantro/Kconfig
> +++ b/drivers/staging/media/hantro/Kconfig
> @@ -20,6 +20,7 @@ config VIDEO_HANTRO_IMX8M
> bool "Hantro VPU i.MX8M support"
> depends on VIDEO_HANTRO
> depends on ARCH_MXC || COMPILE_TEST
> + select RESET_VPU_IMX8MQ
> default y
> help
> Enable support for i.MX8M SoCs.
> diff --git a/drivers/staging/media/hantro/imx8m_vpu_hw.c
> b/drivers/staging/media/hantro/imx8m_vpu_hw.c
> index c222de075ef4..d5b4312b9391 100644
> --- a/drivers/staging/media/hantro/imx8m_vpu_hw.c
> +++ b/drivers/staging/media/hantro/imx8m_vpu_hw.c
> @@ -7,49 +7,12 @@
>
> #include
> #include
> +#include
>
> #include "hantro.h"
> #include "hantro_jpeg.h"
> #include "hantro_g1_regs.h"
>
> -#define CTRL_SOFT_RESET 0x00
> -#define RESET_G1 BIT(1)
> -#define RESET_G2 BIT(0)
> -
> -#define CTRL_CLOCK_ENABLE0x04
> -#define CLOCK_G1 BIT(1)
> -#define CLOCK_G2 BIT(0)
> -
> -#define CTRL_G1_DEC_FUSE 0x08
> -#define CTRL_G1_PP_FUSE 0x0c
> -#define CTRL_G2_DEC_FUSE 0x10
> -
> -static void imx8m_soft_reset(struct hantro_dev *vpu, u32 reset_bits)
> -{
> - u32 val;
> -
> - /* Assert */
> - val = readl(vpu->ctrl_base + CTRL_SOFT_RESET);
> - val &= ~reset_bits;
> - writel(val, vpu->ctrl_base + CTRL_SOFT_RESET);
> -
> - udelay(2);
> -
> - /* Release */
> - val = readl(vpu->ctrl_base + CTRL_SOFT_RESET);
> - val |= reset_bits;
> - writel(val, vpu->ctrl_base + CTRL_SOFT_RESET);
> -}
> -
> -static void imx8m_clk_enable(struct hantro_dev *vpu, u32 clock_bits)
> -{
> - u32 val;
> -
> - val = readl(vpu->ctrl_base + CTRL_CLOCK_ENABLE);
> - val |= clock_bits;
> - writel(val, vpu->ctrl_base + CTRL_CLOCK_ENABLE);
The way it is implemented in the reset driver, the clocks are now
ungated between assert and deassert instead of afterwards. Is this on
purpose?
regards
Philipp
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 0/5] Reset driver for IMX8MQ VPU hardware block
On Wed, 2021-03-03 at 16:20 +0100, Benjamin Gaignard wrote: > Le 03/03/2021 à 15:17, Philipp Zabel a écrit : > > Hi Benjamin, > > > > On Mon, 2021-03-01 at 16:17 +0100, Benjamin Gaignard wrote: > > > The two VPUs inside IMX8MQ share the same control block which can be see > > > as a reset hardware block. > > This isn't a reset controller though. The control block also contains > > clock gates of some sort and a filter register for the featureset fuses. > > Those shouldn't be manipulated via the reset API. > > They are all part of the control block and of the reset process for this > hardware that why I put them here. I guess it is border line :-) I'm pushing back to keep the reset control framework focused on controlling reset lines. Every side effect (such as the asymmetric clock ungating) in a random driver makes it harder to reason about behaviour at the API level, and to review patches for hardware I am not familiar with. > > > In order to be able to add the second VPU (for HECV decoding) it will be > > > more handy if the both VPU drivers instance don't have to share the > > > control block registers. This lead to implement it as an independ reset > > > driver and to change the VPU driver to use it. > > Why not switch to a syscon regmap for the control block? That should > > also allow to keep backwards compatibility with the old binding with > > minimal effort. > > I will give a try in this direction. Thank you. > > > Please note that this series break the compatibility between the DTB and > > > kernel. This break is limited to IMX8MQ SoC and is done when the driver > > > is still in staging directory. > > I know in this case we are pretty sure there are no users of this > > binding except for a staging driver, but it would still be nice to keep > > support for the deprecated binding, to avoid the requirement of updating > > kernel and DT in lock-step. > > If I want to use a syscon (or a reset) the driver must not ioremap the "ctrl" > registers. It means that "ctrl" has to be removed from the driver requested > reg-names (imx8mq_reg_names[]). Doing that break the kernel/DT compatibility. > Somehow syscon and "ctrl" are exclusive. The way the driver is set up currently, yes. You could add a bit of platform specific probe code, though, that would set up the regmap either by calling syscon_regmap_lookup_by_phandle(); for the new binding, or, if the phandle is not available, fall back to platform_get_resource_byname(..., "ctrl"); devm_ioremap_resource(); devm_regmap_init_mmio(); for the old binding. The actual codec .reset and variant .runtime_resume ops could be identical then. regards Philipp ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v6 02/13] dt-bindings: media: nxp,imx8mq-vpu: Update the bindings for G2 support
On Thu, Mar 18, 2021 at 09:20:35AM +0100, Benjamin Gaignard wrote:
> Introducing G2 hevc video decoder lead to modify the bindings to allow
> to get one node per VPUs.
> VPUs share one hardware control block which is provided as a phandle on
> an syscon.
> Each node got now one reg and one interrupt.
> Add a compatible for G2 hardware block: nxp,imx8mq-vpu-g2.
>
> To be compatible with older DT the driver is still capable to use 'ctrl'
> reg-name even if it is deprecated now.
>
> Signed-off-by: Benjamin Gaignard
> ---
> version 5:
> - This version doesn't break the backward compatibilty between kernel
> and DT.
>
> .../bindings/media/nxp,imx8mq-vpu.yaml| 53 ---
> 1 file changed, 34 insertions(+), 19 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml
> b/Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml
> index 762be3f96ce9..79502fc8bde5 100644
> --- a/Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml
> +++ b/Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml
> @@ -15,22 +15,18 @@ description:
>
> properties:
>compatible:
> -const: nxp,imx8mq-vpu
> +oneOf:
> + - const: nxp,imx8mq-vpu
> + - const: nxp,imx8mq-vpu-g2
>
>reg:
> -maxItems: 3
> -
> - reg-names:
> -items:
> - - const: g1
> - - const: g2
> - - const: ctrl
> +maxItems: 1
>
>interrupts:
> -maxItems: 2
> +maxItems: 1
>
>interrupt-names:
> -items:
> +oneOf:
>- const: g1
>- const: g2
>
> @@ -46,14 +42,18 @@ properties:
>power-domains:
> maxItems: 1
>
> + nxp,imx8mq-vpu-ctrl:
> +description: Specifies a phandle to syscon VPU hardware control block
> +$ref: "/schemas/types.yaml#/definitions/phandle"
> +
Should we drop the 'q' here, i.e. nxp,imx8m-vpu-ctrl so we can use the same
binding for i.MX8MM later?
> required:
>- compatible
>- reg
> - - reg-names
>- interrupts
>- interrupt-names
>- clocks
>- clock-names
> + - nxp,imx8mq-vpu-ctrl
>
> additionalProperties: false
>
> @@ -62,18 +62,33 @@ examples:
> #include
> #include
>
> -vpu: video-codec@3830 {
> +vpu_ctrl: syscon@3832 {
> + compatible = "nxp,imx8mq-vpu-ctrl", "syscon";
> + reg = <0x3832 0x1>;
> +};
> +
> +vpu_g1: video-codec@3830 {
> compatible = "nxp,imx8mq-vpu";
> -reg = <0x3830 0x1>,
> - <0x3831 0x1>,
> - <0x3832 0x1>;
> -reg-names = "g1", "g2", "ctrl";
> -interrupts = ,
> - ;
> -interrupt-names = "g1", "g2";
> +reg = <0x3830 0x1>;
> +interrupts = ;
> +interrupt-names = "g1";
> +clocks = <&clk IMX8MQ_CLK_VPU_G1_ROOT>,
> + <&clk IMX8MQ_CLK_VPU_G2_ROOT>,
Does the G1 VPU require the G2 clock and vice versa?
regards
Philipp
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v6 03/13] media: hantro: Use syscon instead of 'ctrl' register
On Thu, Mar 18, 2021 at 09:20:36AM +0100, Benjamin Gaignard wrote:
> In order to be able to share the control hardware block between
> VPUs use a syscon instead a ioremap it in the driver.
> To keep the compatibility with older DT if 'nxp,imx8mq-vpu-ctrl'
> phandle is not found look at 'ctrl' reg-name.
> With the method it becomes useless to provide a list of register
> names so remove it.
>
> Signed-off-by: Benjamin Gaignard
> ---
> version 5:
> - use syscon instead of VPU reset driver.
> - if DT doesn't provide syscon keep backward compatibilty by using
>'ctrl' reg-name.
>
> drivers/staging/media/hantro/hantro.h | 5 +-
> drivers/staging/media/hantro/imx8m_vpu_hw.c | 52 -
> 2 files changed, 34 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/staging/media/hantro/hantro.h
> b/drivers/staging/media/hantro/hantro.h
> index 65f9f7ea7dcf..a99a96b84b5e 100644
> --- a/drivers/staging/media/hantro/hantro.h
> +++ b/drivers/staging/media/hantro/hantro.h
> @@ -13,6 +13,7 @@
> #define HANTRO_H_
>
> #include
> +#include
> #include
> #include
> #include
> @@ -167,7 +168,7 @@ hantro_vdev_to_func(struct video_device *vdev)
> * @reg_bases: Mapped addresses of VPU registers.
> * @enc_base:Mapped address of VPU encoder register for
> convenience.
> * @dec_base:Mapped address of VPU decoder register for
> convenience.
> - * @ctrl_base: Mapped address of VPU control block.
> + * @ctrl_base: Regmap of VPU control block.
> * @vpu_mutex: Mutex to synchronize V4L2 calls.
> * @irqlock: Spinlock to synchronize access to data structures
> * shared with interrupt handlers.
> @@ -186,7 +187,7 @@ struct hantro_dev {
> void __iomem **reg_bases;
> void __iomem *enc_base;
> void __iomem *dec_base;
> - void __iomem *ctrl_base;
> + struct regmap *ctrl_base;
>
> struct mutex vpu_mutex; /* video_device lock */
> spinlock_t irqlock;
> diff --git a/drivers/staging/media/hantro/imx8m_vpu_hw.c
> b/drivers/staging/media/hantro/imx8m_vpu_hw.c
> index c222de075ef4..bd9d135dd440 100644
> --- a/drivers/staging/media/hantro/imx8m_vpu_hw.c
> +++ b/drivers/staging/media/hantro/imx8m_vpu_hw.c
> @@ -7,6 +7,7 @@
>
> #include
> #include
> +#include
>
> #include "hantro.h"
> #include "hantro_jpeg.h"
> @@ -24,30 +25,28 @@
> #define CTRL_G1_PP_FUSE 0x0c
> #define CTRL_G2_DEC_FUSE 0x10
>
> +static const struct regmap_config ctrl_regmap_ctrl = {
> + .reg_bits = 32,
> + .val_bits = 32,
> + .reg_stride = 0x14,
> +};
> +
> static void imx8m_soft_reset(struct hantro_dev *vpu, u32 reset_bits)
> {
> - u32 val;
> -
> /* Assert */
> - val = readl(vpu->ctrl_base + CTRL_SOFT_RESET);
> - val &= ~reset_bits;
> - writel(val, vpu->ctrl_base + CTRL_SOFT_RESET);
> + regmap_update_bits(vpu->ctrl_base, CTRL_SOFT_RESET, reset_bits, 0);
>
> udelay(2);
>
> /* Release */
> - val = readl(vpu->ctrl_base + CTRL_SOFT_RESET);
> - val |= reset_bits;
> - writel(val, vpu->ctrl_base + CTRL_SOFT_RESET);
> + regmap_update_bits(vpu->ctrl_base, CTRL_SOFT_RESET,
> +reset_bits, reset_bits);
> }
>
> static void imx8m_clk_enable(struct hantro_dev *vpu, u32 clock_bits)
> {
> - u32 val;
> -
> - val = readl(vpu->ctrl_base + CTRL_CLOCK_ENABLE);
> - val |= clock_bits;
> - writel(val, vpu->ctrl_base + CTRL_CLOCK_ENABLE);
> + regmap_update_bits(vpu->ctrl_base, CTRL_CLOCK_ENABLE,
> +clock_bits, clock_bits);
> }
>
> static int imx8mq_runtime_resume(struct hantro_dev *vpu)
> @@ -64,9 +63,9 @@ static int imx8mq_runtime_resume(struct hantro_dev *vpu)
> imx8m_clk_enable(vpu, CLOCK_G1 | CLOCK_G2);
>
> /* Set values of the fuse registers */
> - writel(0x, vpu->ctrl_base + CTRL_G1_DEC_FUSE);
> - writel(0x, vpu->ctrl_base + CTRL_G1_PP_FUSE);
> - writel(0x, vpu->ctrl_base + CTRL_G2_DEC_FUSE);
> + regmap_write(vpu->ctrl_base, CTRL_G1_DEC_FUSE, 0x);
> + regmap_write(vpu->ctrl_base, CTRL_G1_PP_FUSE, 0x);
> + regmap_write(vpu->ctrl_base, CTRL_G2_DEC_FUSE, 0x);
>
> clk_bulk_disable_unprepare(vpu->variant->num_clocks, vpu->clocks);
>
> @@ -150,8 +149,22 @@ static irqreturn_t imx8m_vpu_g1_irq(int irq, void
> *dev_id)
>
> static int imx8mq_vpu_hw_init
Re: [PATCH v6 12/13] media: hantro: IMX8M: add variant for G2/HEVC codec
On Thu, Mar 18, 2021 at 09:20:45AM +0100, Benjamin Gaignard wrote: > Add variant to IMX8M to enable G2/HEVC codec. > Define the capabilities for the hardware up to 3840x2160. > G2 doesn't have postprocessor, use the same clocks and got it > own interruption. > > Signed-off-by: Benjamin Gaignard Reviewed-by: Philipp Zabel regards Philipp ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v6 13/13] arm64: dts: imx8mq: Add node to G2 hardware
On Thu, Mar 18, 2021 at 09:20:46AM +0100, Benjamin Gaignard wrote:
> Split VPU node in two: one for G1 and one for G2 since they are
> different hardware blocks.
> Add syscon for hardware control block.
> Remove reg-names property that is useless.
> Each VPU node only need one interrupt.
>
> Signed-off-by: Benjamin Gaignard
> ---
> version 5:
> - use syscon instead of VPU reset
>
> arch/arm64/boot/dts/freescale/imx8mq.dtsi | 43 ++-
> 1 file changed, 34 insertions(+), 9 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> index 17c449e12c2e..b537d153ebbd 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> @@ -1329,15 +1329,16 @@ usb3_phy1: usb-phy@382f0040 {
> status = "disabled";
> };
>
> - vpu: video-codec@3830 {
> + vpu_ctrl: syscon@3832 {
> + compatible = "nxp,imx8mq-vpu-ctrl", "syscon";
> + reg = <0x3832 0x1>;
> + };
> +
> + vpu_g1: video-codec@3830 {
> compatible = "nxp,imx8mq-vpu";
> - reg = <0x3830 0x1>,
> - <0x3831 0x1>,
> - <0x3832 0x1>;
> - reg-names = "g1", "g2", "ctrl";
> - interrupts = ,
> - ;
> - interrupt-names = "g1", "g2";
> + reg = <0x3830 0x1>;
> + interrupts = ;
> + interrupt-names = "g1";
> clocks = <&clk IMX8MQ_CLK_VPU_G1_ROOT>,
><&clk IMX8MQ_CLK_VPU_G2_ROOT>,
><&clk IMX8MQ_CLK_VPU_DEC_ROOT>;
> @@ -1350,9 +1351,33 @@ vpu: video-codec@3830 {
><&clk IMX8MQ_VPU_PLL_OUT>,
><&clk IMX8MQ_SYS1_PLL_800M>,
><&clk IMX8MQ_VPU_PLL>;
> - assigned-clock-rates = <6>, <6>,
> + assigned-clock-rates = <6>, <3>,
I'd like to see this mentioned in the commit message.
> +<8>, <0>;
> + power-domains = <&pgc_vpu>;
> + nxp,imx8mq-vpu-ctrl = <&vpu_ctrl>;
> + };
> +
> + vpu_g2: video-codec@3831 {
> + compatible = "nxp,imx8mq-vpu-g2";
> + reg = <0x3831 0x1>;
> + interrupts = ;
> + interrupt-names = "g2";
> + clocks = <&clk IMX8MQ_CLK_VPU_G1_ROOT>,
> + <&clk IMX8MQ_CLK_VPU_G2_ROOT>,
> + <&clk IMX8MQ_CLK_VPU_DEC_ROOT>;
> + clock-names = "g1", "g2", "bus";
> + assigned-clocks = <&clk IMX8MQ_CLK_VPU_G1>,
Can the G1 clock configuration be dropped from the G2 device node and
the G2 clock configuration from the G1 device node? It looks weird that
these devices configure each other's clocks.
regards
Philipp
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v6 02/13] dt-bindings: media: nxp,imx8mq-vpu: Update the bindings for G2 support
On Fri, Mar 26, 2021 at 03:26:15PM +0100, Benjamin Gaignard wrote:
>
> Le 26/03/2021 à 15:11, Philipp Zabel a écrit :
> > On Thu, Mar 18, 2021 at 09:20:35AM +0100, Benjamin Gaignard wrote:
> > > Introducing G2 hevc video decoder lead to modify the bindings to allow
> > > to get one node per VPUs.
> > > VPUs share one hardware control block which is provided as a phandle on
> > > an syscon.
> > > Each node got now one reg and one interrupt.
> > > Add a compatible for G2 hardware block: nxp,imx8mq-vpu-g2.
> > >
> > > To be compatible with older DT the driver is still capable to use 'ctrl'
> > > reg-name even if it is deprecated now.
> > >
> > > Signed-off-by: Benjamin Gaignard
> > > ---
> > > version 5:
> > > - This version doesn't break the backward compatibilty between kernel
> > >and DT.
> > >
> > > .../bindings/media/nxp,imx8mq-vpu.yaml| 53 ---
> > > 1 file changed, 34 insertions(+), 19 deletions(-)
> > >
> > > diff --git a/Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml
> > > b/Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml
> > > index 762be3f96ce9..79502fc8bde5 100644
> > > --- a/Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml
> > > +++ b/Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml
> > > @@ -15,22 +15,18 @@ description:
> > > properties:
> > > compatible:
> > > -const: nxp,imx8mq-vpu
> > > +oneOf:
> > > + - const: nxp,imx8mq-vpu
> > > + - const: nxp,imx8mq-vpu-g2
> > > reg:
> > > -maxItems: 3
> > > -
> > > - reg-names:
> > > -items:
> > > - - const: g1
> > > - - const: g2
> > > - - const: ctrl
> > > +maxItems: 1
> > > interrupts:
> > > -maxItems: 2
> > > +maxItems: 1
> > > interrupt-names:
> > > -items:
> > > +oneOf:
> > > - const: g1
> > > - const: g2
> > > @@ -46,14 +42,18 @@ properties:
> > > power-domains:
> > > maxItems: 1
> > > + nxp,imx8mq-vpu-ctrl:
> > > +description: Specifies a phandle to syscon VPU hardware control block
> > > +$ref: "/schemas/types.yaml#/definitions/phandle"
> > > +
> > Should we drop the 'q' here, i.e. nxp,imx8m-vpu-ctrl so we can use the same
> > binding for i.MX8MM later?
>
> I don't know if the control block is the same or not on IMX8MM, so I have only
> put a compatible targeting IMX8MQ.
Oh, the compatible property of the control handle node can be different.
I'm just suggesting that this phandle property be called the same.
Otherwise we'd have to add another nxp,imx8mm-vpu-ctrl property and then
mark either of the two as required, depending on the compatible.
> >
> > > required:
> > > - compatible
> > > - reg
> > > - - reg-names
> > > - interrupts
> > > - interrupt-names
> > > - clocks
> > > - clock-names
> > > + - nxp,imx8mq-vpu-ctrl
> > > additionalProperties: false
> > > @@ -62,18 +62,33 @@ examples:
> > > #include
> > > #include
> > > -vpu: video-codec@3830 {
> > > +vpu_ctrl: syscon@3832 {
> > > + compatible = "nxp,imx8mq-vpu-ctrl", "syscon";
> > > + reg = <0x3832 0x1>;
> > > +};
> > > +
> > > +vpu_g1: video-codec@38300000 {
> > > compatible = "nxp,imx8mq-vpu";
> > > -reg = <0x3830 0x1>,
> > > - <0x3831 0x1>,
> > > - <0x3832 0x1>;
> > > -reg-names = "g1", "g2", "ctrl";
> > > -interrupts = ,
> > > - ;
> > > -interrupt-names = "g1", "g2";
> > > +reg = <0x3830 0x1>;
> > > +interrupts = ;
> > > +interrupt-names = "g1";
> > > +clocks = <&clk IMX8MQ_CLK_VPU_G1_ROOT>,
> > > + <&clk IMX8MQ_CLK_VPU_G2_ROOT>,
> > Does the G1 VPU require the G2 clock and vice versa?
>
> Yes either the control hardware block won't work.
Ok.
Reviewed-by: Philipp Zabel
regards
Philipp
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging:hantro: Fixed "replace comma with semicolon" Warning:
Hi Travis, On Fri, 2020-12-04 at 17:51 -0600, Travis Carter wrote: > Corrected the following Warning: > drivers/staging/media/hantro/hantro_v4l2.c:319: WARNING: Possible comma where > semicolon could be used > > Signed-off-by: Travis Carter > --- > drivers/staging/media/hantro/hantro_v4l2.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/staging/media/hantro/hantro_v4l2.c > b/drivers/staging/media/hantro/hantro_v4l2.c > index b668a82d40ad..e1081c16f56a 100644 > --- a/drivers/staging/media/hantro/hantro_v4l2.c > +++ b/drivers/staging/media/hantro/hantro_v4l2.c > @@ -316,7 +316,7 @@ hantro_reset_fmt(struct v4l2_pix_format_mplane *fmt, > > fmt->pixelformat = vpu_fmt->fourcc; > fmt->field = V4L2_FIELD_NONE; > - fmt->colorspace = V4L2_COLORSPACE_JPEG, > + fmt->colorspace = V4L2_COLORSPACE_JPEG; > fmt->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT; > fmt->quantization = V4L2_QUANTIZATION_DEFAULT; > fmt->xfer_func = V4L2_XFER_FUNC_DEFAULT; Thank you, Reviewed-by: Philipp Zabel regards Philipp ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH -next] media: hantro: use resource_size
Hi Zheng, On Wed, 2021-01-06 at 21:18 +0800, Zheng Yongjun wrote: > Use resource_size rather than a verbose computation on > the end and start fields. > > Signed-off-by: Zheng Yongjun > --- > drivers/staging/media/hantro/hantro_v4l2.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/staging/media/hantro/hantro_v4l2.c > b/drivers/staging/media/hantro/hantro_v4l2.c > index b668a82d40ad..e1081c16f56a 100644 > --- a/drivers/staging/media/hantro/hantro_v4l2.c > +++ b/drivers/staging/media/hantro/hantro_v4l2.c > @@ -316,7 +316,7 @@ hantro_reset_fmt(struct v4l2_pix_format_mplane *fmt, > > fmt->pixelformat = vpu_fmt->fourcc; > fmt->field = V4L2_FIELD_NONE; > - fmt->colorspace = V4L2_COLORSPACE_JPEG, > + fmt->colorspace = V4L2_COLORSPACE_JPEG; > fmt->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT; > fmt->quantization = V4L2_QUANTIZATION_DEFAULT; > fmt->xfer_func = V4L2_XFER_FUNC_DEFAULT; Subject and commit message do not describe the patch. regards Philipp ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 -next] media: hantro: convert comma to semicolon
On Fri, 2021-01-08 at 17:22 +0800, Zheng Yongjun wrote: > Replace a comma between expression statements by a semicolon. > > Signed-off-by: Zheng Yongjun > --- > drivers/staging/media/hantro/hantro_v4l2.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/staging/media/hantro/hantro_v4l2.c > b/drivers/staging/media/hantro/hantro_v4l2.c > index b668a82d40ad..e1081c16f56a 100644 > --- a/drivers/staging/media/hantro/hantro_v4l2.c > +++ b/drivers/staging/media/hantro/hantro_v4l2.c > @@ -316,7 +316,7 @@ hantro_reset_fmt(struct v4l2_pix_format_mplane *fmt, > > fmt->pixelformat = vpu_fmt->fourcc; > fmt->field = V4L2_FIELD_NONE; > - fmt->colorspace = V4L2_COLORSPACE_JPEG, > + fmt->colorspace = V4L2_COLORSPACE_JPEG; > fmt->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT; > fmt->quantization = V4L2_QUANTIZATION_DEFAULT; > fmt->xfer_func = V4L2_XFER_FUNC_DEFAULT; Thank you, Reviewed-by: Philipp Zabel regards Philipp ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v5 02/12] gpu: ipu-csi: Swap fields according to input/output field types
Hi Steve,
On Tue, 2018-10-16 at 17:00 -0700, Steve Longerbeam wrote:
> The function ipu_csi_init_interface() was inverting the F-bit for
> NTSC case, in the CCIR_CODE_1/2 registers. The result being that
> for NTSC bottom-top field order, the CSI would swap fields and
> capture in top-bottom order.
>
> Instead, base field swap on the field order of the input to the CSI,
> and the field order of the requested output. If the input/output
> fields are sequential but different, swap fields, otherwise do
> not swap. This requires passing both the input and output mbus
> frame formats to ipu_csi_init_interface().
>
> Move this code to a new private function ipu_csi_set_bt_interlaced_codes()
> that programs the CCIR_CODE_1/2 registers for interlaced BT.656 (and
> possibly interlaced BT.1120 in the future).
>
> When detecting input video standard from the input frame width/height,
> make sure to double height if input field type is alternate, since
> in that case input height only includes lines for one field.
>
> Signed-off-by: Steve Longerbeam
> ---
> Changes since v4:
> - Cleaned up some convoluted code in ipu_csi_init_interface(), suggested
> by Philipp Zabel.
> - Fixed a regression in csi_setup(), caught by Philipp.
> ---
> drivers/gpu/ipu-v3/ipu-csi.c | 119 +++---
> drivers/staging/media/imx/imx-media-csi.c | 17 +---
> include/video/imx-ipu-v3.h| 3 +-
> 3 files changed, 88 insertions(+), 51 deletions(-)
>
> diff --git a/drivers/gpu/ipu-v3/ipu-csi.c b/drivers/gpu/ipu-v3/ipu-csi.c
> index aa0e30a2ba18..4a15e513fa05 100644
> --- a/drivers/gpu/ipu-v3/ipu-csi.c
> +++ b/drivers/gpu/ipu-v3/ipu-csi.c
> @@ -325,6 +325,15 @@ static int mbus_code_to_bus_cfg(struct
> ipu_csi_bus_config *cfg, u32 mbus_code,
> return 0;
> }
>
> +/* translate alternate field mode based on given standard */
> +static inline enum v4l2_field
> +ipu_csi_translate_field(enum v4l2_field field, v4l2_std_id std)
> +{
> + return (field != V4L2_FIELD_ALTERNATE) ? field :
> + ((std & V4L2_STD_525_60) ?
> + V4L2_FIELD_SEQ_BT : V4L2_FIELD_SEQ_TB);
> +}
> +
> /*
> * Fill a CSI bus config struct from mbus_config and mbus_framefmt.
> */
> @@ -374,22 +383,75 @@ static int fill_csi_bus_cfg(struct ipu_csi_bus_config
> *csicfg,
> return 0;
> }
>
> +static int ipu_csi_set_bt_interlaced_codes(struct ipu_csi *csi,
> +struct v4l2_mbus_framefmt *infmt,
> +struct v4l2_mbus_framefmt *outfmt,
infmt and outfmt parameters could be const.
> +v4l2_std_id std)
> +{
> + enum v4l2_field infield, outfield;
> + bool swap_fields;
> +
> + /* get translated field type of input and output */
> + infield = ipu_csi_translate_field(infmt->field, std);
> + outfield = ipu_csi_translate_field(outfmt->field, std);
> +
> + /*
> + * Write the H-V-F codes the CSI will match against the
> + * incoming data for start/end of active and blanking
> + * field intervals. If input and output field types are
> + * sequential but not the same (one is SEQ_BT and the other
> + * is SEQ_TB), swap the F-bit so that the CSI will capture
> + * field 1 lines before field 0 lines.
> + */
> + swap_fields = (V4L2_FIELD_IS_SEQUENTIAL(infield) &&
> +V4L2_FIELD_IS_SEQUENTIAL(outfield) &&
> +infield != outfield);
> +
> + if (!swap_fields) {
> + /*
> + * Field0BlankEnd = 110, Field0BlankStart = 010
> + * Field0ActiveEnd = 100, Field0ActiveStart = 000
> + * Field1BlankEnd = 111, Field1BlankStart = 011
> + * Field1ActiveEnd = 101, Field1ActiveStart = 001
> + */
> + ipu_csi_write(csi, 0x40596 | CSI_CCIR_ERR_DET_EN,
> + CSI_CCIR_CODE_1);
> + ipu_csi_write(csi, 0xD07DF, CSI_CCIR_CODE_2);
> + } else {
> + dev_dbg(csi->ipu->dev, "capture field swap\n");
> +
> + /* same as above but with F-bit inverted */
> + ipu_csi_write(csi, 0xD07DF | CSI_CCIR_ERR_DET_EN,
> + CSI_CCIR_CODE_1);
> + ipu_csi_write(csi, 0x40596, CSI_CCIR_CODE_2);
> + }
> +
> + ipu_csi_write(csi, 0xFF, CSI_CCIR_CODE_3);
> +
> + return 0;
> +}
> +
> +
> int ipu_csi_init_interface(struct ipu_csi *csi,
> struct v4l2_mbus_config *mbus_cfg,
> -struct v4l2_mbus_framefmt *mbus_fmt)
>
Re: [PATCH v6 02/12] gpu: ipu-csi: Swap fields according to input/output field types
On Tue, 2019-01-08 at 16:15 -0800, Steve Longerbeam wrote: > The function ipu_csi_init_interface() was inverting the F-bit for > NTSC case, in the CCIR_CODE_1/2 registers. The result being that > for NTSC bottom-top field order, the CSI would swap fields and > capture in top-bottom order. > > Instead, base field swap on the field order of the input to the CSI, > and the field order of the requested output. If the input/output > fields are sequential but different, swap fields, otherwise do > not swap. This requires passing both the input and output mbus > frame formats to ipu_csi_init_interface(). > > Move this code to a new private function ipu_csi_set_bt_interlaced_codes() > that programs the CCIR_CODE_1/2 registers for interlaced BT.656 (and > possibly interlaced BT.1120 in the future). > > When detecting input video standard from the input frame width/height, > make sure to double height if input field type is alternate, since > in that case input height only includes lines for one field. > > Signed-off-by: Steve Longerbeam > Reviewed-by: Philipp Zabel Also Acked-by: Philipp Zabel to be merged via the media tree regards Philipp ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v6 05/12] media: imx-csi: Input connections to CSI should be optional
On Tue, 2019-01-08 at 16:15 -0800, Steve Longerbeam wrote:
> Some imx platforms do not have fwnode connections to all CSI input
> ports, and should not be treated as an error. This includes the
> imx6q SabreAuto, which has no connections to ipu1_csi1 and ipu2_csi0.
> Return -ENOTCONN in imx_csi_parse_endpoint() so that v4l2-fwnode
> endpoint parsing will not treat an unconnected endpoint as an error.
>
> Fixes: c893500a16baf ("media: imx: csi: Register a subdev notifier")
>
> Signed-off-by: Steve Longerbeam
> ---
> drivers/staging/media/imx/imx-media-csi.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/media/imx/imx-media-csi.c
> b/drivers/staging/media/imx/imx-media-csi.c
> index e3a4f39dbf73..b276672cae1d 100644
> --- a/drivers/staging/media/imx/imx-media-csi.c
> +++ b/drivers/staging/media/imx/imx-media-csi.c
> @@ -1815,7 +1815,7 @@ static int imx_csi_parse_endpoint(struct device *dev,
> struct v4l2_fwnode_endpoint *vep,
> struct v4l2_async_subdev *asd)
> {
> - return fwnode_device_is_available(asd->match.fwnode) ? 0 : -EINVAL;
> + return fwnode_device_is_available(asd->match.fwnode) ? 0 : -ENOTCONN;
> }
>
> static int imx_csi_async_register(struct csi_priv *priv)
Is this something that should be applied as a fix, separately from this
series?
regards
Philipp
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] media: imx-csi: Input connections to CSI should be optional
On Wed, 2019-01-09 at 10:34 -0800, Steve Longerbeam wrote:
> Some imx platforms do not have fwnode connections to all CSI input
> ports, and should not be treated as an error. This includes the
> imx6q SabreAuto, which has no connections to ipu1_csi1 and ipu2_csi0.
> Return -ENOTCONN in imx_csi_parse_endpoint() so that v4l2-fwnode
> endpoint parsing will not treat an unconnected CSI input port as
> an error.
>
> Fixes: c893500a16baf ("media: imx: csi: Register a subdev notifier")
>
> Signed-off-by: Steve Longerbeam
Reviewed-by: Philipp Zabel
regards
Philipp
> ---
> drivers/staging/media/imx/imx-media-csi.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/media/imx/imx-media-csi.c
> b/drivers/staging/media/imx/imx-media-csi.c
> index 4223f8d418ae..30b1717982ae 100644
> --- a/drivers/staging/media/imx/imx-media-csi.c
> +++ b/drivers/staging/media/imx/imx-media-csi.c
> @@ -1787,7 +1787,7 @@ static int imx_csi_parse_endpoint(struct device *dev,
> struct v4l2_fwnode_endpoint *vep,
> struct v4l2_async_subdev *asd)
> {
> - return fwnode_device_is_available(asd->match.fwnode) ? 0 : -EINVAL;
> + return fwnode_device_is_available(asd->match.fwnode) ? 0 : -ENOTCONN;
> }
>
> static int imx_csi_async_register(struct csi_priv *priv)
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 1/2] media: imx: csi: Disable CSI immediately after last EOF
On Thu, 2019-01-17 at 12:49 -0800, Steve Longerbeam wrote:
> Disable the CSI immediately after receiving the last EOF before stream
> off (and thus before disabling the IDMA channel).
>
> This fixes a complete system hard lockup on the SabreAuto when streaming
> from the ADV7180, by repeatedly sending a stream off immediately followed
> by stream on:
>
> while true; do v4l2-ctl -d4 --stream-mmap --stream-count=3; done
>
> Eventually this either causes the system lockup or EOF timeouts at all
> subsequent stream on, until a system reset.
>
> The lockup occurs when disabling the IDMA channel at stream off. Disabling
> the CSI before disabling the IDMA channel appears to be a reliable fix for
> the hard lockup.
>
> Fixes: 4a34ec8e470cb ("[media] media: imx: Add CSI subdev driver")
>
> Reported-by: Gaël PORTAY
> Signed-off-by: Steve Longerbeam
> Cc: sta...@vger.kernel.org
> ---
> Changes in v2:
> - restore an empty line
> - Add Fixes: and Cc: stable
> ---
> drivers/staging/media/imx/imx-media-csi.c | 6 --
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/media/imx/imx-media-csi.c
> b/drivers/staging/media/imx/imx-media-csi.c
> index e18f58f56dfb..e0f6f88e2e70 100644
> --- a/drivers/staging/media/imx/imx-media-csi.c
> +++ b/drivers/staging/media/imx/imx-media-csi.c
> @@ -681,6 +681,8 @@ static void csi_idmac_stop(struct csi_priv *priv)
> if (ret == 0)
> v4l2_warn(&priv->sd, "wait last EOF timeout\n");
>
> + ipu_csi_disable(priv->csi);
> +
Can you add a short comment why this call is here? Since now
csi_idmac_stop is kind of a misnomer and symmetry with csi(_idmac)_start
is broken, I think this is a bit un-obvious.
Also note that now the error path of csi_start() will now call
ipu_csi_disable() while the CSI is disabled. This happens to work
because that just calls ipu_module_disable(), which is not refcounted.
> devm_free_irq(priv->dev, priv->eof_irq, priv);
> devm_free_irq(priv->dev, priv->nfb4eof_irq, priv);
>
> @@ -793,9 +795,9 @@ static void csi_stop(struct csi_priv *priv)
> /* stop the frame interval monitor */
> if (priv->fim)
> imx_media_fim_set_stream(priv->fim, NULL, false);
> + } else {
> + ipu_csi_disable(priv->csi);
> }
> -
> - ipu_csi_disable(priv->csi);
Reviewed-by: Philipp Zabel
regards
Philipp
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 1/2] media: imx: csi: Disable SMFC before disabling IDMA channel
Hi,
On Fri, 2019-01-18 at 17:04 -0800, Steve Longerbeam wrote:
> Disable the SMFC before disabling the IDMA channel, instead of after,
> in csi_idmac_unsetup().
>
> This fixes a complete system hard lockup on the SabreAuto when streaming
> from the ADV7180, by repeatedly sending a stream off immediately followed
> by stream on:
>
> while true; do v4l2-ctl -d4 --stream-mmap --stream-count=3; done
>
> Eventually this either causes the system lockup or EOF timeouts at all
> subsequent stream on, until a system reset.
>
> The lockup occurs when disabling the IDMA channel at stream off. Stopping
> the video data stream entering the IDMA channel before disabling the
> channel itself appears to be a reliable fix for the hard lockup. That can
> be done either by disabling the SMFC or the CSI before disabling the
> channel. Disabling the SMFC before the channel is the easiest solution,
> so do that.
>
> Fixes: 4a34ec8e470cb ("[media] media: imx: Add CSI subdev driver")
>
> Suggested-by: Peter Seiderer
> Reported-by: Gaël PORTAY
> Signed-off-by: Steve Longerbeam
Gaël, could we get a Tested-by: for this as well?
> Cc: sta...@vger.kernel.org
> ---
> Changes in v3:
> - switch from disabling the CSI before the channel to disabling the
> SMFC before the channel.
> Changes in v2:
> - restore an empty line
> - Add Fixes: and Cc: stable
> ---
> drivers/staging/media/imx/imx-media-csi.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/media/imx/imx-media-csi.c
> b/drivers/staging/media/imx/imx-media-csi.c
> index e18f58f56dfb..8610027eac97 100644
> --- a/drivers/staging/media/imx/imx-media-csi.c
> +++ b/drivers/staging/media/imx/imx-media-csi.c
> @@ -560,8 +560,8 @@ static int csi_idmac_setup_channel(struct csi_priv *priv)
> static void csi_idmac_unsetup(struct csi_priv *priv,
> enum vb2_buffer_state state)
> {
> - ipu_idmac_disable_channel(priv->idmac_ch);
> ipu_smfc_disable(priv->smfc);
> + ipu_idmac_disable_channel(priv->idmac_ch);
Steve, do you have any theory why this helps? It's a bit weird to
disable the SMFC module while the DMA channel is still enabled. I think
this warrants a big comment, given that enable order is SMFC_EN before
IDMAC channel enable.
Also ipu_smfc_disable is refcounted, so if the other CSI is capturing
simultaneously, this change has no effect.
FWIW, I don't see any regressions though.
regards
Philipp
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/4] media: imx: csi: Allow unknown nearest upstream entities
On Sat, 2019-01-19 at 13:45 -0800, Steve Longerbeam wrote:
> On i.MX6, the nearest upstream entity to the CSI can only be the
> CSI video muxes or the Synopsys DW MIPI CSI-2 receiver.
>
> However the i.MX53 has no CSI video muxes or a MIPI CSI-2 receiver.
> So allow for the nearest upstream entity to the CSI to be something
> other than those.
>
> Fixes: bf3cfaa712e5c ("media: staging/imx: get CSI bus type from nearest
> upstream entity")
>
> Signed-off-by: Steve Longerbeam
> Cc: sta...@vger.kernel.org
> ---
> drivers/staging/media/imx/imx-media-csi.c | 18 ++
> 1 file changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/media/imx/imx-media-csi.c
> b/drivers/staging/media/imx/imx-media-csi.c
> index 555aa45e02e3..b9af7d3d4974 100644
> --- a/drivers/staging/media/imx/imx-media-csi.c
> +++ b/drivers/staging/media/imx/imx-media-csi.c
> @@ -154,9 +154,10 @@ static inline bool requires_passthrough(struct
> v4l2_fwnode_endpoint *ep,
> /*
> * Parses the fwnode endpoint from the source pad of the entity
> * connected to this CSI. This will either be the entity directly
> - * upstream from the CSI-2 receiver, or directly upstream from the
> - * video mux. The endpoint is needed to determine the bus type and
> - * bus config coming into the CSI.
> + * upstream from the CSI-2 receiver, directly upstream from the
> + * video mux, or directly upstream from the CSI itself. The endpoint
> + * is needed to determine the bus type and bus config coming into
> + * the CSI.
> */
> static int csi_get_upstream_endpoint(struct csi_priv *priv,
>struct v4l2_fwnode_endpoint *ep)
> @@ -172,7 +173,8 @@ static int csi_get_upstream_endpoint(struct csi_priv
> *priv,
> if (!priv->src_sd)
> return -EPIPE;
>
> - src = &priv->src_sd->entity;
> + sd = priv->src_sd;
> + src = &sd->entity;
>
> if (src->function == MEDIA_ENT_F_VID_MUX) {
> /*
> @@ -186,6 +188,14 @@ static int csi_get_upstream_endpoint(struct csi_priv
> *priv,
> src = &sd->entity;
> }
>
> + /*
> + * If the source is neither the video mux nor the CSI-2 receiver,
> + * get the source pad directly upstream from CSI itself.
> + */
> + if (src->function != MEDIA_ENT_F_VID_MUX &&
Will it work correctly if there's an external MUX connected to the CSI?
> + sd->grp_id != IMX_MEDIA_GRP_ID_CSI2)
> + src = &priv->sd.entity;
> +
> /* get source pad of entity directly upstream from src */
> pad = imx_media_find_upstream_pad(priv->md, src, 0);
> if (IS_ERR(pad))
regards
Philipp
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 3/4] media: imx: Rename functions that add IPU-internal subdevs/links
On Sat, 2019-01-19 at 13:45 -0800, Steve Longerbeam wrote: > For the functions that add and remove the internal IPU subdevice > descriptors and links between them, rename them to make clear they > are the subdevs and links internal to the IPU. Also rename the > platform data structure for the internal IPU subdevices. > No functional changes. > > Signed-off-by: Steve Longerbeam Acked-by: Philipp Zabel regards Philipp ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 4/4] media: imx: Don't register IPU subdevs/links if CSI port missing
On Sat, 2019-01-19 at 13:46 -0800, Steve Longerbeam wrote:
> The second IPU internal sub-devices were being registered and links
> to them created even when the second IPU is not present. This is wrong
> for i.MX6 S/DL and i.MX53 which have only a single IPU.
>
> Fixes: e130291212df5 ("[media] media: Add i.MX media core driver")
>
> Signed-off-by: Steve Longerbeam
Reviewed-by: Philipp Zabel
regards
Philipp
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 1/2] media: imx: csi: Disable SMFC before disabling IDMA channel
On Mon, 2019-01-21 at 10:46 -0800, Steve Longerbeam wrote: > > On 1/21/19 10:43 AM, Steve Longerbeam wrote: > > > > > > On 1/21/19 3:49 AM, Philipp Zabel wrote: > > > Also ipu_smfc_disable is refcounted, so if the other CSI is capturing > > > simultaneously, this change has no effect. > > > > Sigh, you're right. Let me go back to disabling the CSI before the > > channel, the CSI enable/disable is not refcounted (it doesn't need to > > be since it is single use) so it doesn't have this problem. > > > > Should we drop this patch or keep it (with a big comment)? By only > > changing the disable order to "CSI then channel", the hang is reliably > > fixed from my and Gael's testing, but my concern is that by not > > disabling the SMFC before the channel, the SMFC could still empty its > > FIFO to the channel's internal FIFO and still create a hang. > > Well, as you said it will have no effect if both CSI's are streaming > with the SMFC, in which case the danger would still exist. Perhaps it > would be best to just drop this patch. Hm, if we can't guarantee the intended effect with this patch, and stopping the CSI first helps reliably, it's indeed better to just do that instead. regards Philipp ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v10 12/13] media: video-mux: add bayer formats
On Wed, 2019-01-23 at 10:52 +, Rui Miguel Silva wrote: > Add non vendor bayer formats to the allowed format array. > > Signed-off-by: Rui Miguel Silva > --- > drivers/media/platform/video-mux.c | 20 > 1 file changed, 20 insertions(+) > > diff --git a/drivers/media/platform/video-mux.c > b/drivers/media/platform/video-mux.c > index c33900e3c23e..0ba30756e1e4 100644 > --- a/drivers/media/platform/video-mux.c > +++ b/drivers/media/platform/video-mux.c > @@ -263,6 +263,26 @@ static int video_mux_set_format(struct v4l2_subdev *sd, > case MEDIA_BUS_FMT_UYYVYY16_0_5X48: > case MEDIA_BUS_FMT_JPEG_1X8: > case MEDIA_BUS_FMT_AHSV_1X32: > + case MEDIA_BUS_FMT_SBGGR8_1X8: > + case MEDIA_BUS_FMT_SGBRG8_1X8: > + case MEDIA_BUS_FMT_SGRBG8_1X8: > + case MEDIA_BUS_FMT_SRGGB8_1X8: > + case MEDIA_BUS_FMT_SBGGR10_1X10: > + case MEDIA_BUS_FMT_SGBRG10_1X10: > + case MEDIA_BUS_FMT_SGRBG10_1X10: > + case MEDIA_BUS_FMT_SRGGB10_1X10: > + case MEDIA_BUS_FMT_SBGGR12_1X12: > + case MEDIA_BUS_FMT_SGBRG12_1X12: > + case MEDIA_BUS_FMT_SGRBG12_1X12: > + case MEDIA_BUS_FMT_SRGGB12_1X12: > + case MEDIA_BUS_FMT_SBGGR14_1X14: > + case MEDIA_BUS_FMT_SGBRG14_1X14: > + case MEDIA_BUS_FMT_SGRBG14_1X14: > + case MEDIA_BUS_FMT_SRGGB14_1X14: > + case MEDIA_BUS_FMT_SBGGR16_1X16: > + case MEDIA_BUS_FMT_SGBRG16_1X16: > + case MEDIA_BUS_FMT_SGRBG16_1X16: > + case MEDIA_BUS_FMT_SRGGB16_1X16: > break; > default: > sdformat->format.code = MEDIA_BUS_FMT_Y8_1X8; This could be merged independently from the other changes. Reviewed-by: Philipp Zabel regards Philipp ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 3/4] gpu: ipu-v3: ipu-ic: Add support for BT.709 encoding
On Fri, 2019-02-08 at 17:47 -0800, Steve Longerbeam wrote:
> Pass v4l2 encoding enum to the ipu_ic task init functions, and add
> support for the BT.709 encoding and inverse encoding matrices.
>
> Reported-by: Tim Harvey
> Signed-off-by: Steve Longerbeam
> ---
> Changes in v4:
> - fix compile error.
> Chnges in v3:
> - none.
> Changes in v2:
> - only return "Unsupported YCbCr encoding" error if inf != outf,
> since if inf == outf, the identity matrix can be used. Reported
> by Tim Harvey.
> ---
> drivers/gpu/ipu-v3/ipu-ic.c | 71 +++--
> drivers/gpu/ipu-v3/ipu-image-convert.c | 1 +
> drivers/staging/media/imx/imx-ic-prpencvf.c | 4 +-
> include/video/imx-ipu-v3.h | 5 +-
> 4 files changed, 71 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/ipu-v3/ipu-ic.c b/drivers/gpu/ipu-v3/ipu-ic.c
> index e459615a49a1..c5f83d7e357f 100644
> --- a/drivers/gpu/ipu-v3/ipu-ic.c
> +++ b/drivers/gpu/ipu-v3/ipu-ic.c
> @@ -212,6 +212,23 @@ static const struct ic_csc_params ic_csc_identity = {
> .scale = 2,
> };
>
> +/*
> + * BT.709 encoding from RGB full range to YUV limited range:
> + *
> + * Y = R * .2126 + G * .7152 + B * .0722;
> + * U = R * -.1146 + G * -.3854 + B * .5000 + 128.;
> + * V = R * .5000 + G * -.4542 + B * -.0458 + 128.;
This is a conversion to YUV full range. Limited range should be:
Y R * .1826 + G * .6142 + B * .0620 + 16;
U = R * -.1007 + G * -.3385 + B * .4392 + 128;
V R * .4392 + G * -.3990 + B * -.0402 + 128;
> + */
> +static const struct ic_csc_params ic_csc_rgb2ycbcr_bt709 = {
> + .coeff = {
> + { 54, 183, 18 },
> + { 483, 413, 128 },
> + { 128, 396, 500 },
> + },
> + .offset = { 0, 512, 512 },
> + .scale = 1,
> +};
> +
> /*
> * Inverse BT.601 encoding from YUV limited range to RGB full range:
> *
> @@ -229,12 +246,31 @@ static const struct ic_csc_params
> ic_csc_ycbcr2rgb_bt601 = {
> .scale = 2,
> };
>
> +/*
> + * Inverse BT.709 encoding from YUV limited range to RGB full range:
> + *
> + * R = (1. * (Y - 16)) + (1.5748 * (Cr - 128));
> + * G = (1. * (Y - 16)) - (0.1873 * (Cb - 128)) - (0.4681 * (Cr - 128));
> + * B = (1. * (Y - 16)) + (1.8556 * (Cb - 128);
The coefficients look like full range again, conversion from limited
range YUV should look like:
R = (1.1644 * (Y - 16)) + (1.7927 * (Cr - 128));
G = (1.1644 * (Y - 16)) - (0.2132 * (Cb - 128)) - (0.5329 * (Cr - 128));
B = (1.1644 * (Y - 16)) + (2.1124 * (Cb - 128);
> + */
> +static const struct ic_csc_params ic_csc_ycbcr2rgb_bt709 = {
> + .coeff = {
> + { 128, 0, 202 },
> + { 128, 488, 452 },
> + { 128, 238, 0 },
> + },
> + .offset = { -435, 136, -507 },
> + .scale = 2,
> +};
> +
> static int init_csc(struct ipu_ic *ic,
> enum ipu_color_space inf,
> enum ipu_color_space outf,
> + enum v4l2_ycbcr_encoding encoding,
Should we support YUV BT.601 <-> YUV REC.709 conversions? That would
require separate encodings for input and output. Also, this might be a
good time to think about adding quantization range parameters as well.
regards
Philipp
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 3/4] gpu: ipu-v3: ipu-ic: Add support for BT.709 encoding
Hi Steve, On Mon, 2019-02-11 at 17:20 -0800, Steve Longerbeam wrote: [...] > > Should we support YUV BT.601 <-> YUV REC.709 conversions? That would > > require separate encodings for input and output. > > How about if we pass the input and output encodings to the init ic task > functions, but for now require they be the same? We can support > transcoding in a later series. [...] > Again, I think for now, just include input/output quantization but > require full range for RGB and limited range for YUV. Yes, that is fine. I'd just like to avoid unnecessary interface changes between ipu-v3 and imx-media. So if we have to change it right now, why not plan ahead. > But that really balloons the arguments to ipu_ic_task_init_*(). Should > we create an ipu_ic_task_init structure? I wonder if we should just expose struct ic_csc_params and provide a helper to fill it given colorspace and V4L2 encoding/quantization parameters. Something like: struct ipu_ic_csc_params csc; imx_media_init_ic_csc_params(&csc, in_cs, in_encoding, in_quantization, out_cs, out_encoding, out_quantization); ipu_ic_task_init(ic, in_width, in_height, out_width, out_height, &csc); // or ipu_ic_task_init_rsc(ic, rsc, &csc); regards Philipp ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v6 3/7] gpu: ipu-v3: ipu-ic: Fully describe colorspace conversions
On Thu, 2019-03-07 at 15:33 -0800, Steve Longerbeam wrote:
> Only providing the input and output RGB/YUV space to the IC task init
> functions is not sufficient. To fully characterize a colorspace
> conversion, the colorspace (chromaticities), Y'CbCr encoding standard,
> and quantization also need to be specified.
>
> Define a 'struct ipu_ic_colorspace' that includes all the above, and pass
> the input and output ipu_ic_colorspace to the IC task init functions.
>
> This allows to actually enforce the fact that the IC:
>
> - can only encode to/from YUV full range (follow-up patch will remove
> this restriction).
> - can only encode to/from RGB full range.
> - can only encode using BT.601 standard (follow-up patch will add
> Rec.709 encoding support).
> - cannot convert colorspaces from input to output, the
> input and output colorspace chromaticities must be the same.
>
> The determination of the CSC coefficients based on the input/output
> colorspace parameters are moved to a new function calc_csc_coeffs(),
> called by init_csc().
>
> Signed-off-by: Steve Longerbeam
> ---
> drivers/gpu/ipu-v3/ipu-ic.c | 136 +---
> drivers/gpu/ipu-v3/ipu-image-convert.c | 27 ++--
> drivers/staging/media/imx/imx-ic-prpencvf.c | 22 +++-
> include/video/imx-ipu-v3.h | 37 +-
> 4 files changed, 154 insertions(+), 68 deletions(-)
>
> diff --git a/drivers/gpu/ipu-v3/ipu-ic.c b/drivers/gpu/ipu-v3/ipu-ic.c
> index b63a2826b629..c4048c921801 100644
> --- a/drivers/gpu/ipu-v3/ipu-ic.c
> +++ b/drivers/gpu/ipu-v3/ipu-ic.c
> @@ -146,8 +146,10 @@ struct ipu_ic {
> const struct ic_task_regoffs *reg;
> const struct ic_task_bitfields *bit;
>
> - enum ipu_color_space in_cs, g_in_cs;
> - enum ipu_color_space out_cs;
> + struct ipu_ic_colorspace in_cs;
> + struct ipu_ic_colorspace g_in_cs;
> + struct ipu_ic_colorspace out_cs;
> +
> bool graphics;
> bool rotation;
> bool in_use;
> @@ -235,42 +237,83 @@ static const struct ic_encode_coeff
> ic_encode_ycbcr2rgb_601 = {
> .scale = 2,
> };
>
> +static int calc_csc_coeffs(struct ipu_ic_priv *priv,
> +struct ic_encode_coeff *coeff_out,
> +const struct ipu_ic_colorspace *in,
> +const struct ipu_ic_colorspace *out)
> +{
> + bool inverse_encode;
> +
> + if (in->colorspace != out->colorspace) {
> + dev_err(priv->ipu->dev, "Cannot convert colorspaces\n");
> + return -ENOTSUPP;
> + }
I don't think this is useful enough to warrant having the colorspace
field in ipu_ic_colorspace. Let the caller make sure of this, same as
for xfer_func.
> + if (out->enc != V4L2_YCBCR_ENC_601) {
> + dev_err(priv->ipu->dev, "Only BT.601 encoding supported\n");
> + return -ENOTSUPP;
> + }
This is only important if out->cs is IPUV3_COLORSPACE_YUV, right? If the
output is RGB this field shouldn't matter.
> +
> + if ((in->cs == IPUV3_COLORSPACE_YUV &&
> + in->quant != V4L2_QUANTIZATION_FULL_RANGE) ||
> + (out->cs == IPUV3_COLORSPACE_YUV &&
> + out->quant != V4L2_QUANTIZATION_FULL_RANGE)) {
> + dev_err(priv->ipu->dev, "Limited range YUV not supported\n");
> + return -ENOTSUPP;
> + }
> +
> + if ((in->cs == IPUV3_COLORSPACE_RGB &&
> + in->quant != V4L2_QUANTIZATION_FULL_RANGE) ||
> + (out->cs == IPUV3_COLORSPACE_RGB &&
> + out->quant != V4L2_QUANTIZATION_FULL_RANGE)) {
> + dev_err(priv->ipu->dev, "Limited range RGB not supported\n");
> + return -ENOTSUPP;
> + }
> +
> + if (in->cs == out->cs) {
> + *coeff_out = ic_encode_identity;
> +
> + return 0;
> + }
> +
> + inverse_encode = (in->cs == IPUV3_COLORSPACE_YUV);
What does inverse_encode mean in this context?
> +
> + *coeff_out = inverse_encode ?
> + ic_encode_ycbcr2rgb_601 : ic_encode_rgb2ycbcr_601;
> +
> + return 0;
> +}
> +
> static int init_csc(struct ipu_ic *ic,
> - enum ipu_color_space inf,
> - enum ipu_color_space outf,
> + const struct ipu_ic_colorspace *in,
> + const struct ipu_ic_colorspace *out,
> int csc_index)
> {
> struct ipu_ic_priv *priv = ic->priv;
> - const struct ic_encode_coeff *coeff;
> + struct ic_encode_coeff coeff;
I understand this is a preparation for patch 5, but on its own this
introduces an unnecessary copy.
> u32 __iomem *base;
> const u16 (*c)[3];
> const u16 *a;
> u32 param;
> + int ret;
> +
> + ret = calc_csc_coeffs(priv, &coeff, in, out);
> + if (ret)
> + return ret;
>
> base = (u32 __iomem *)
> (priv->tpmem_base + ic->reg->tpmem_csc[csc_index]);
>
> - if (inf == IPUV3_COLORSPACE_YUV && outf == IPUV3_COLORSPACE_RGB)
> -
Re: [PATCH] media: imx: vdic: Fix wrong CSI group ID
On Fri, 2019-03-01 at 15:27 -0800, Steve Longerbeam wrote:
> The i.MX7 capture support forgot to change the group ID for the CSI
> to the IPU CSI in VDIC sub-device, it was left at the i.MX7 CSI
> group ID.
>
> Fixes: 67673ed55084 ("media: staging/imx: rearrange group id to take in
> account IPU")
>
> Signed-off-by: Steve Longerbeam
Reviewed-by: Philipp Zabel
regards
Philipp
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/2] imx-drm: imx-hdmi: split imx soc specific code from imx-hdmi
Hi Andy,
I think separating the core from the SoC specific part is a good step
into the right direction.
Am Mittwoch, den 05.11.2014, 20:55 +0800 schrieb Andy Yan:
> imx6 and rockchip rk3288 and JZ4780 (Ingenic Xburst/MIPS)
> use the interface compatible Designware HDMI IP, but they
> also have some lightly difference, such as phy pll configuration,
> register width(imx hdmi register is one byte, but rk3288 is 4
> bytes width), 4K support(imx6 doesn't support 4k, but rk3288 does),
> clk useage,and the crtc mux configuration is also platform specific.
>
> To reuse the imx hdmi driver, split the platform specific code out
> to dw_hdmi-imx.c.
>
> Signed-off-by: Andy Yan
[...]
> +static int imx_hdmi_parse_dt(struct imx_hdmi_priv *hdmi)
> +{
> + struct device_node *np = hdmi->dev->of_node;
> +
> + hdmi->regmap = syscon_regmap_lookup_by_phandle(np, "gpr");
> + if (IS_ERR(hdmi->regmap)) {
> + dev_err(hdmi->dev, "Unable to get gpr\n");
> + return PTR_ERR(hdmi->regmap);
> + }
> +
> + hdmi->isfr_clk = devm_clk_get(hdmi->dev, "isfr");
> + if (IS_ERR(hdmi->isfr_clk)) {
> + dev_err(hdmi->dev, "Unable to get HDMI isfr clk\n");
> + return PTR_ERR(hdmi->isfr_clk);
> + }
> +
> + hdmi->iahb_clk = devm_clk_get(hdmi->dev, "iahb");
> + if (IS_ERR(hdmi->iahb_clk)) {
> + dev_err(hdmi->dev, "Unable to get HDMI iahb clk\n");
> + return PTR_ERR(hdmi->iahb_clk);
> + }
Surely this IP core needs to be clocked regardless of the SoC? How are
clocks controlled on rk3288 and jz4780?
[...]
> +/*On rockchip platform, no-word access to the hdmi
> + * register will causes an imprecise external abort
> + */
> +static inline void hdmi_writel(struct imx_hdmi *hdmi, u32 val, int offset)
> +{
> + writel(val, hdmi->regs + (offset << 2));
> +}
>
> - bool phy_enabled;
> - struct drm_display_mode previous_mode;
> +static inline u32 hdmi_readl(struct imx_hdmi *hdmi, int offset)
> +{
> + return readl(hdmi->regs + (offset << 2));
> +}
>
> - struct regmap *regmap;
> - struct i2c_adapter *ddc;
> - void __iomem *regs;
> +static void hdmi_modl(struct imx_hdmi *hdmi, u32 data, u32 mask, unsigned
> reg)
> +{
> + u32 val = hdmi_readl(hdmi, reg) & ~mask;
>
> - unsigned int sample_rate;
> - int ratio;
> -};
> + val |= data & mask;
> + hdmi_writel(hdmi, val, reg);
> +}
>
> -static void imx_hdmi_set_ipu_di_mux(struct imx_hdmi *hdmi, int ipu_di)
> +static void hdmi_mask_writel(struct imx_hdmi *hdmi, u32 data, unsigned int
> reg,
> + u32 shift, u32 mask)
> {
> - regmap_update_bits(hdmi->regmap, IOMUXC_GPR3,
> -IMX6Q_GPR3_HDMI_MUX_CTL_MASK,
> -ipu_di << IMX6Q_GPR3_HDMI_MUX_CTL_SHIFT);
> + hdmi_modl(hdmi, data << shift, mask, reg);
> }
>
> -static inline void hdmi_writeb(struct imx_hdmi *hdmi, u8 val, int offset)
> +static inline void hdmi_writeb(struct imx_hdmi *hdmi, u32 val, int offset)
> {
> writeb(val, hdmi->regs + offset);
> }
>
> -static inline u8 hdmi_readb(struct imx_hdmi *hdmi, int offset)
> +static inline u32 hdmi_readb(struct imx_hdmi *hdmi, int offset)
> {
> return readb(hdmi->regs + offset);
> }
>
> -static void hdmi_modb(struct imx_hdmi *hdmi, u8 data, u8 mask, unsigned reg)
> +static void hdmi_modb(struct imx_hdmi *hdmi, u32 data, u32 mask, unsigned
> reg)
> {
> u8 val = hdmi_readb(hdmi, reg) & ~mask;
Do you really need to use readl instead of readb? In my opinion it would
be better then to convert the driver to use regmap for register access
(in a separate patch) and then let the SoC specific driver extension
provide the regmap to the core driver.
[...]
> diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
> new file mode 100644
> index 000..e7e8285
> --- /dev/null
> +++ b/include/drm/bridge/dw_hdmi.h
> @@ -0,0 +1,114 @@
> +/*
> + * Copyright (C) 2011 Freescale Semiconductor, Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#ifndef __DW_HDMI_H__
> +#define __DW_HDMI_H__
> +
> +#include
> +
> +#define HDMI_EDID_LEN 512
> +
> +enum {
> + RES_8,
> + RES_10,
> + RES_12,
> + RES_MAX,
> +};
> +
> +enum imx_hdmi_devtype {
> + IMX6Q_HDMI,
> + IMX6DL_HDMI,
> +};
> +
> +struct mpll_config {
> + unsigned long mpixelclock;
> + struct {
> + u16 cpce;
> + u16 gmp;
> + } res[RES_MAX];
> +};
> +
> +struct curr_ctrl {
> + unsigned long mpixelclock;
> + u16 curr[RES_MAX];
> +};
For a header file in include/drm/bridge maybe these struct names are a
bit too generic now.
regards
Philipp
___
devel maili
Re: [PATCH v3 09/10] gpu: ipu-v3: Make use of media_bus_format enum
Am Freitag, den 07.11.2014, 15:07 +0100 schrieb Boris Brezillon: > In order to have subsytem agnostic media bus format definitions we've > moved media bus definition to include/uapi/linux/media-bus-format.h and > prefixed enum values with MEDIA_BUS_FMT instead of V4L2_MBUS_FMT. > > Reference new definitions in the ipu-v3 driver. > > Signed-off-by: Boris Brezillon Acked-by: Philipp Zabel regards Philipp ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v5 1/6] of: Decrement refcount of previous endpoint in of_graph_get_next_endpoint
Hi Guennadi, On Fri, Nov 07, 2014 at 11:06:21PM +0100, Guennadi Liakhovetski wrote: > Hi Philipp, > > Thanks for the patch and sorry for a late reply. I did look at your > patches earlier too, but maybe not attentively enough, or maybe I'm > misunderstanding something now. In the scan_of_host() function in > soc_camera.c as of current -next I see: > > epn = of_graph_get_next_endpoint(np, epn); > > which already looks like a refcount leak to me. If epn != NULL, its > refcount is incremented, but then immediately the variable gets > overwritten, and there's no extra copy of that variable to fix this. If > I'm right, then that bug in itself should be fixed, ideally before your > patch is applied. But in fact, your patch fixes this, since it modifies > of_graph_get_next_endpoint() to return with prev's refcount not > incremented, right? Whereas the of_node_put(epn) later down in > scan_of_host() decrements refcount of the _next_ endpoint, not the > previous one, so, it should be left alone? I.e. AFAICT your modification > to of_graph_get_next_endpoint() fixes soc_camera.c with no further > modifications to it required? You are right. With the old implementation, you'd have to do the epn = of_graph_get_next_endpoint(np, prev); of_node_put(prev); prev = epn; dance to avoid leaking a reference to the first endpoint. This series accidentally fixes soc_camera by changing of_graph_get_next_endpoint to decrement the reference count itself. regards Philipp ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 1/2] drm: imx: Move imx-drm driver out of staging
The imx-drm driver was put into staging mostly for the following reasons,
all of which have been addressed or superseded:
- convert the irq driver to use linear irq domains
- work out the device tree bindings, this lead to the common of_graph
bindings being used
- factor out common helper functions, this mostly resulted in the
component framework and drm of_graph helpers.
Before adding new fixes, and certainly before adding new features,
move it into its proper place below drivers/gpu/drm.
Signed-off-by: Philipp Zabel
---
.../{staging/imx-drm => drm/imx}/fsl-imx-drm.txt| 0
.../bindings/{staging/imx-drm => drm/imx}/hdmi.txt | 0
.../bindings/{staging/imx-drm => drm/imx}/ldb.txt | 0
drivers/gpu/drm/Kconfig | 2 ++
drivers/gpu/drm/Makefile| 1 +
drivers/{staging/imx-drm => gpu/drm/imx}/Kconfig| 0
drivers/{staging/imx-drm => gpu/drm/imx}/Makefile | 0
drivers/{staging/imx-drm => gpu/drm/imx}/imx-drm-core.c | 0
drivers/{staging/imx-drm => gpu/drm/imx}/imx-drm.h | 0
drivers/{staging/imx-drm => gpu/drm/imx}/imx-hdmi.c | 0
drivers/{staging/imx-drm => gpu/drm/imx}/imx-hdmi.h | 0
drivers/{staging/imx-drm => gpu/drm/imx}/imx-ldb.c | 0
drivers/{staging/imx-drm => gpu/drm/imx}/imx-tve.c | 0
drivers/{staging/imx-drm => gpu/drm/imx}/ipuv3-crtc.c | 0
drivers/{staging/imx-drm => gpu/drm/imx}/ipuv3-plane.c | 0
drivers/{staging/imx-drm => gpu/drm/imx}/ipuv3-plane.h | 0
.../{staging/imx-drm => gpu/drm/imx}/parallel-display.c | 0
drivers/staging/Kconfig | 2 --
drivers/staging/Makefile| 1 -
drivers/staging/imx-drm/TODO| 17 -
20 files changed, 3 insertions(+), 20 deletions(-)
rename Documentation/devicetree/bindings/{staging/imx-drm =>
drm/imx}/fsl-imx-drm.txt (100%)
rename Documentation/devicetree/bindings/{staging/imx-drm => drm/imx}/hdmi.txt
(100%)
rename Documentation/devicetree/bindings/{staging/imx-drm => drm/imx}/ldb.txt
(100%)
rename drivers/{staging/imx-drm => gpu/drm/imx}/Kconfig (100%)
rename drivers/{staging/imx-drm => gpu/drm/imx}/Makefile (100%)
rename drivers/{staging/imx-drm => gpu/drm/imx}/imx-drm-core.c (100%)
rename drivers/{staging/imx-drm => gpu/drm/imx}/imx-drm.h (100%)
rename drivers/{staging/imx-drm => gpu/drm/imx}/imx-hdmi.c (100%)
rename drivers/{staging/imx-drm => gpu/drm/imx}/imx-hdmi.h (100%)
rename drivers/{staging/imx-drm => gpu/drm/imx}/imx-ldb.c (100%)
rename drivers/{staging/imx-drm => gpu/drm/imx}/imx-tve.c (100%)
rename drivers/{staging/imx-drm => gpu/drm/imx}/ipuv3-crtc.c (100%)
rename drivers/{staging/imx-drm => gpu/drm/imx}/ipuv3-plane.c (100%)
rename drivers/{staging/imx-drm => gpu/drm/imx}/ipuv3-plane.h (100%)
rename drivers/{staging/imx-drm => gpu/drm/imx}/parallel-display.c (100%)
delete mode 100644 drivers/staging/imx-drm/TODO
diff --git a/Documentation/devicetree/bindings/staging/imx-drm/fsl-imx-drm.txt
b/Documentation/devicetree/bindings/drm/imx/fsl-imx-drm.txt
similarity index 100%
rename from Documentation/devicetree/bindings/staging/imx-drm/fsl-imx-drm.txt
rename to Documentation/devicetree/bindings/drm/imx/fsl-imx-drm.txt
diff --git a/Documentation/devicetree/bindings/staging/imx-drm/hdmi.txt
b/Documentation/devicetree/bindings/drm/imx/hdmi.txt
similarity index 100%
rename from Documentation/devicetree/bindings/staging/imx-drm/hdmi.txt
rename to Documentation/devicetree/bindings/drm/imx/hdmi.txt
diff --git a/Documentation/devicetree/bindings/staging/imx-drm/ldb.txt
b/Documentation/devicetree/bindings/drm/imx/ldb.txt
similarity index 100%
rename from Documentation/devicetree/bindings/staging/imx-drm/ldb.txt
rename to Documentation/devicetree/bindings/drm/imx/ldb.txt
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 37c5a6e..24c2d7c 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -202,3 +202,5 @@ source "drivers/gpu/drm/panel/Kconfig"
source "drivers/gpu/drm/sti/Kconfig"
source "drivers/gpu/drm/amd/amdkfd/Kconfig"
+
+source "drivers/gpu/drm/imx/Kconfig"
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index dd9d35b..47d8986 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -62,6 +62,7 @@ obj-$(CONFIG_DRM_BOCHS) += bochs/
obj-$(CONFIG_DRM_MSM) += msm/
obj-$(CONFIG_DRM_TEGRA) += tegra/
obj-$(CONFIG_DRM_STI) += sti/
+obj-$(CONFIG_DRM_IMX) += imx/
obj-y += i2c/
obj-y += panel/
obj-y += bridge/
diff --git a/drivers/staging/imx-drm/Kconfig b/drivers/gpu/drm/imx/Kconfig
similarity index 100%
rename from drivers/staging/imx-drm/Kconfig
rename to drivers/gpu/drm/imx/Kconfig
diff --git a/drivers/staging/imx-drm/Makefile b/driv
[PATCH v2 2/2] MAINTAINERS: add maintainer for i.MX DRM driver
Add myself as the maintainer of the i.MX DRM driver. Signed-off-by: Philipp Zabel Acked-by: Shawn Guo --- MAINTAINERS | 7 +++ 1 file changed, 7 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index 3c69a3c7..b29325d 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -3202,6 +3202,13 @@ F: drivers/gpu/drm/exynos/ F: include/drm/exynos* F: include/uapi/drm/exynos* +DRM DRIVERS FOR FREESCALE IMX +M: Philipp Zabel +L: dri-de...@lists.freedesktop.org +S: Maintained +F: drivers/gpu/drm/imx/ +F: Documentation/devicetree/bindings/drm/imx/ + DRM DRIVERS FOR NVIDIA TEGRA M: Thierry Reding M: Terje Bergström -- 2.1.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v13 0/12] dw-hdmi: convert imx hdmi to bridge/dw_hdmi
Hi Andy, I have yet to look at this in more detail, but from a quick test starting with patch 3, the HDMI display stays black on Nitrogen6X, and starting with patch 8 I get the following error. imx-drm display-subsystem: [CONNECTOR:21:HDMI-A-1] drm_connector_register failed: -12 [ cut here ] WARNING: CPU: 0 PID: 1 at drivers/clk/clk.c:851 __clk_disable+0x6c/0x70() Modules linked in: CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.18.0-rc6+ #8377 Backtrace: [<80012414>] (dump_backtrace) from [<80012754>] (show_stack+0x20/0x24) r6:0353 r5: r4:8083ea08 r3: [<80012734>] (show_stack) from [<805ae670>] (dump_stack+0x8c/0x9c) [<805ae5e4>] (dump_stack) from [<80022744>] (warn_slowpath_common+0x80/0x9c) r5:0009 r4: [<800226c4>] (warn_slowpath_common) from [<8002281c>] (warn_slowpath_null+0x2c/0x34) r8:b721c610 r7:b72b0400 r6:b735504c r5:8113 r4:b735504c [<800227f0>] (warn_slowpath_null) from [<80458088>] (__clk_disable+0x6c/0x70) [<8045801c>] (__clk_disable) from [<804581a8>] (clk_disable+0x34/0x40) r4:b735504c r3:b700e000 [<80458174>] (clk_disable) from [<803241f0>] (dw_hdmi_imx_unbind+0x30/0x60) r5:b7355010 r4:b7219a10 [<803241c0>] (dw_hdmi_imx_unbind) from [<8032fc84>] (component_unbind.isra.3+0x40/0x78) r8:b72a1e40 r7:b725f158 r6:b72b0400 r5:b725f158 r4:b725f4c0 r3:803241c0 [<8032fc44>] (component_unbind.isra.3) from [<8032fd44>] (component_unbind_all+0x88/0xb8) r5:b725f4c0 r4:b725f140 [<8032fcbc>] (component_unbind_all) from [<80321a3c>] (imx_drm_driver_load+0x100/0x13c) r7:b72b05cc r6:fff4 r5:b7355010 r4:b72b0400 [<8032193c>] (imx_drm_driver_load) from [<8030c260>] (drm_dev_register+0xb8/0x114) r7:b686ad10 r6: r5: r4:b72b0400 [<8030c1a8>] (drm_dev_register) from [<8030dd58>] (drm_platform_init+0x54/0xe8) r6:80844bf4 r5:b721c600 r4:b72b0400 r3: [<8030dd04>] (drm_platform_init) from [<803218e8>] (imx_drm_bind+0x20/0x28) r6:b725f140 r5:000c r4:b686ad70 [<803218c8>] (imx_drm_bind) from [<8032f9b0>] (try_to_bring_up_master.part.2+0xd8/0x118) [<8032f8d8>] (try_to_bring_up_master.part.2) from [<8032fbe4>] (component_add+0xa0/0x100) r8:b72a1c40 r7:80602b6c r6:b72a1e40 r5:808451e8 r4:b725f140 r3: [<8032fb44>] (component_add) from [<8032330c>] (ipu_drm_probe+0x7c/0x150) r7:b682ba10 r6:b77b3cc0 r5:b682ba00 r4:b77b4868 [<80323290>] (ipu_drm_probe) from [<80336640>] (platform_drv_probe+0x54/0xb4) r9: r8: r7:80844e94 r6:80844e94 r5:fdfb r4:b682ba10 [<803365ec>] (platform_drv_probe) from [<803348a8>] (driver_probe_device+0x128/0x25c) r7:80844e94 r6: r5:808bc064 r4:b682ba10 [<80334780>] (driver_probe_device) from [<80334a2c>] (__device_attach+0x50/0x54) r8: r7:b721c410 r6:803349dc r5:b682ba10 r4:80844e94 r3:80336838 [<803349dc>] (__device_attach) from [<80332c34>] (bus_for_each_drv+0x68/0x9c) r5:b682ba10 r4: [<80332bcc>] (bus_for_each_drv) from [<8033473c>] (device_attach+0x84/0x98) r6:808454e0 r5:b682ba44 r4:b682ba10 [<803346b8>] (device_attach) from [<80333d14>] (bus_probe_device+0x94/0xb8) r6:808454e0 r5:b682ba10 r4:b682ba18 r3:b7046800 [<80333c80>] (bus_probe_device) from [<80331e58>] (device_add+0x450/0x530) r6:b682ba10 r5: r4:b682ba18 r3: [<80331a08>] (device_add) from [<803362c8>] (platform_device_add+0xc4/0x228) r9:0006 r8:b721c410 r7:b7079cc4 r6:b682ba10 r5:b682ba00 r4: [<80336204>] (platform_device_add) from [<80336b60>] (platform_device_register_full+0xcc/0xf0) r7:b7079cc4 r6:b7079ce0 r5:b682ba00 r4:b7079ce0 [<80336a94>] (platform_device_register_full) from [<80328354>] (ipu_add_client_devices.isra.10+0x164/0x19c) r5: r4:b7079ce0 [<803281f0>] (ipu_add_client_devices.isra.10) from [<80328940>] (ipu_probe+0x5b4/0x740) r10:808450d4 r9:0001 r8:b7028180 r7:b72b0010 r6:808450d4 r5: r4:b721c410 [<8032838c>] (ipu_probe) from [<80336640>] (platform_drv_probe+0x54/0xb4) r10: r9:b725f580 r8: r7:80845078 r6:80845078 r5:fdfb r4:b721c410 [<803365ec>] (platform_drv_probe) from [<803348a8>] (driver_probe_device+0x128/0x25c) r7:80845078 r6: r5:808bc064 r4:b721c410 [<80334780>] (driver_probe_device) from [<80334acc>] (__driver_attach+0x9c/0xa0) r8:807ab5e8 r7: r6:b721c444 r5:80845078 r4:b721c410 r3: [<80334a30>] (__driver_attach) from [<80332b6c>] (bus_for_each_dev+0x70/0xa4) r6:80334a30 r5:80845078 r4: r3:b704685c [<80332afc>] (bus_for_each_dev) from [<80334334>] (driver_attach+0x2c/0x30) r6:808454e0 r5:b728d000 r4:80845078 [<80334308>] (driver_attach) from [<80333fac>] (bus_add_driver+0x15c/0x204) [<80333e50>] (bus_add_driver) from [<803352d4>] (driver_register+0x88/0x108) r7:b7078000 r6:807d86c0 r5:8082be60 r4:80845078 [<8033524c>] (driver_register) from [<8033656c>] (__platform_driver_register+0x64/0x6c) r5:8082be60 r4:8082be60 [<80336508>] (__platform_driver_register) from [<807d86dc>] (imx_ipu_driver_init+0x1c/0x20) [<807d86c0>] (imx_ipu_dr
Re: [PATCH v13 08/12] drm: bridge/dw_hdmi: add mode_valid support
Am Mittwoch, den 26.11.2014, 21:33 +0800 schrieb Andy Yan:
> some platform may not support all the display mode,
> add mode_valid interface check it
>
> also add drm_connector_register which add a debugfs
> interface for dump display modes and edid information
>
> Signed-off-by: Andy Yan
> ---
>
> Changes in v13: None
> Changes in v12: None
> Changes in v11: None
> Changes in v10: None
> Changes in v9: None
> Changes in v8: None
> Changes in v7: None
> Changes in v6: None
> Changes in v5: None
> Changes in v4: None
> Changes in v3: None
>
> drivers/gpu/drm/bridge/dw_hdmi.c | 17 +
> include/drm/bridge/dw_hdmi.h | 2 ++
> 2 files changed, 19 insertions(+)
>
> diff --git a/drivers/gpu/drm/bridge/dw_hdmi.c
> b/drivers/gpu/drm/bridge/dw_hdmi.c
> index 5e88c8d..b13e782 100644
> --- a/drivers/gpu/drm/bridge/dw_hdmi.c
> +++ b/drivers/gpu/drm/bridge/dw_hdmi.c
> @@ -1406,6 +1406,20 @@ static int dw_hdmi_connector_get_modes(struct
> drm_connector *connector)
> return 0;
> }
>
> +static enum drm_mode_status
> +dw_hdmi_connector_mode_valid(struct drm_connector *connector,
> + struct drm_display_mode *mode)
> +{
> + struct dw_hdmi *hdmi = container_of(connector,
> +struct dw_hdmi, connector);
> + enum drm_mode_status mode_status = MODE_OK;
> +
> + if (hdmi->plat_data->mode_valid)
> + mode_status = hdmi->plat_data->mode_valid(connector, mode);
> +
> + return mode_status;
> +}
> +
> static struct drm_encoder *dw_hdmi_connector_best_encoder(struct
> drm_connector
> *connector)
> {
> @@ -1430,6 +1444,7 @@ static struct drm_connector_funcs
> dw_hdmi_connector_funcs = {
>
> static struct drm_connector_helper_funcs dw_hdmi_connector_helper_funcs = {
> .get_modes = dw_hdmi_connector_get_modes,
> + .mode_valid = dw_hdmi_connector_mode_valid,
> .best_encoder = dw_hdmi_connector_best_encoder,
> };
>
> @@ -1631,6 +1646,8 @@ int dw_hdmi_bind(struct device *dev, struct device
> *master,
>
> dev_set_drvdata(dev, hdmi);
>
> + drm_connector_register(&hdmi->connector);
> +
This is not right, the connector is registered by the imx-drm core in
the drm_driver .load callback.
regards
Philipp
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v13 07/12] drm: bridge/dw_hdmi: add support for multi-byte register width access
Am Mittwoch, den 26.11.2014, 21:32 +0800 schrieb Andy Yan:
> On rockchip rk3288, only word(32-bit) accesses are
> permitted for hdmi registers. Byte width accesses (writeb,
> readb) generate an imprecise external abort.
>
> Signed-off-by: Andy Yan
>
> ---
>
> Changes in v13: None
> Changes in v12: None
> Changes in v11: None
> Changes in v10: None
> Changes in v9: None
> Changes in v8: None
> Changes in v7: None
> Changes in v6:
> - refactor register access without reg_shift
>
> Changes in v5:
> - refactor reg-io-width
>
> Changes in v4: None
> Changes in v3:
> - split multi-register access to one indepent patch
>
> drivers/gpu/drm/bridge/dw_hdmi.c | 57
> +++-
> 1 file changed, 51 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/dw_hdmi.c
> b/drivers/gpu/drm/bridge/dw_hdmi.c
> index a53bf63..5e88c8d 100644
> --- a/drivers/gpu/drm/bridge/dw_hdmi.c
> +++ b/drivers/gpu/drm/bridge/dw_hdmi.c
> @@ -100,6 +100,11 @@ struct hdmi_data_info {
> struct hdmi_vmode video_mode;
> };
>
> +union dw_reg_ptr {
> + u32 __iomem *p32;
> + u8 __iomem *p8;
> +};
I see no need to introduce this. Just explicitly multiply the offset in
dw_hdmi_writel.
> struct dw_hdmi {
> struct drm_connector connector;
> struct drm_encoder *encoder;
> @@ -121,20 +126,43 @@ struct dw_hdmi {
>
> struct regmap *regmap;
> struct i2c_adapter *ddc;
> - void __iomem *regs;
> + union dw_reg_ptr regs;
Keep this as void __iomem *
> unsigned int sample_rate;
> int ratio;
> +
> + void (*write)(struct dw_hdmi *hdmi, u8 val, int offset);
> + u8 (*read)(struct dw_hdmi *hdmi, int offset);
> };
>
> +static void dw_hdmi_writel(struct dw_hdmi *hdmi, u8 val, int offset)
> +{
> + writel(val, hdmi->regs.p32 + offset);
hdmi->regs + 4 * offset
> +}
> +
> +static u8 dw_hdmi_readl(struct dw_hdmi *hdmi, int offset)
> +{
> + return readl(hdmi->regs.p32 + offset);
same here
> +}
> +
> +static void dw_hdmi_writeb(struct dw_hdmi *hdmi, u8 val, int offset)
> +{
> + writeb(val, hdmi->regs.p8 + offset);
> +}
> +
> +static u8 dw_hdmi_readb(struct dw_hdmi *hdmi, int offset)
> +{
> + return readb(hdmi->regs.p8 + offset);
> +}
> +
> static inline void hdmi_writeb(struct dw_hdmi *hdmi, u8 val, int offset)
> {
> - writeb(val, hdmi->regs + offset);
> + hdmi->write(hdmi, val, offset);
> }
>
> static inline u8 hdmi_readb(struct dw_hdmi *hdmi, int offset)
> {
> - return readb(hdmi->regs + offset);
> + return hdmi->read(hdmi, offset);
> }
>
> static void hdmi_modb(struct dw_hdmi *hdmi, u8 data, u8 mask, unsigned reg)
> @@ -1508,6 +1536,7 @@ int dw_hdmi_bind(struct device *dev, struct device
> *master,
> struct dw_hdmi *hdmi;
> struct resource *iores;
> int ret, irq;
> + u32 val = 1;
>
> hdmi = devm_kzalloc(&pdev->dev, sizeof(*hdmi), GFP_KERNEL);
> if (!hdmi)
> @@ -1520,6 +1549,22 @@ int dw_hdmi_bind(struct device *dev, struct device
> *master,
> hdmi->ratio = 100;
> hdmi->encoder = encoder;
>
> + of_property_read_u32(np, "reg-io-width", &val);
> +
> + switch (val) {
> + case 4:
> + hdmi->write = dw_hdmi_writel;
> + hdmi->read = dw_hdmi_readl;
> + break;
> + case 1:
> + hdmi->write = dw_hdmi_writeb;
> + hdmi->read = dw_hdmi_readb;
> + break;
> + default:
> + dev_err(dev, "reg-io-width must be 1 or 4\n");
> + return -EINVAL;
> + }
> +
> ddc_node = of_parse_phandle(np, "ddc-i2c-bus", 0);
> if (ddc_node) {
> hdmi->ddc = of_find_i2c_adapter_by_node(ddc_node);
> @@ -1544,9 +1589,9 @@ int dw_hdmi_bind(struct device *dev, struct device
> *master,
> return ret;
>
> iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> - hdmi->regs = devm_ioremap_resource(dev, iores);
> - if (IS_ERR(hdmi->regs))
> - return PTR_ERR(hdmi->regs);
> + hdmi->regs.p32 = devm_ioremap_resource(dev, iores);
> + if (IS_ERR(hdmi->regs.p32))
> + return PTR_ERR(hdmi->regs.p32);
>
> /* Product and revision IDs */
> dev_info(dev,
regards
Philipp
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 1/2] drm: imx: Move imx-drm driver out of staging
Am Mittwoch, den 26.11.2014, 09:43 +1000 schrieb Dave Airlie: > On 25 November 2014 at 01:33, Philipp Zabel wrote: > > The imx-drm driver was put into staging mostly for the following reasons, > > all of which have been addressed or superseded: > > - convert the irq driver to use linear irq domains > > - work out the device tree bindings, this lead to the common of_graph > >bindings being used > > - factor out common helper functions, this mostly resulted in the > >component framework and drm of_graph helpers. > > > > Before adding new fixes, and certainly before adding new features, > > move it into its proper place below drivers/gpu/drm. > > > > Signed-off-by: Philipp Zabel > > --- > > FYI I've merged this into drm-next, which I think is probably the best place. Thank you. This would have been my request in the cover letter that I prepared and then forgot to send out with the patches. regards Philipp ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v13 0/12] dw-hdmi: convert imx hdmi to bridge/dw_hdmi
Am Freitag, den 28.11.2014, 17:57 +0800 schrieb Andy Yan: > Hi Philipp: [...] >Very sorry about this trouble. Because I have no imx board, we do all > the test on > RK3288 board. > I had sent a mail with a debug patch to you directly yesterday, hope > it will helpful. > If you have received the mail, would you please give me a reply? Yes, that was helpful, thank you. I needed to apply Russell's "imx-drm: convert imx-drm to use the generic DRM OF helper" patch for drm_of_find_possible_crtcs to work on imx-drm. regards Philipp ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v14 05/12] drm: imx: imx-hdmi: move imx-hdmi to bridge/dw_hdmi
Hi Andy,
Am Montag, den 01.12.2014, 19:24 +0800 schrieb Andy Yan:
[...]
> diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
> new file mode 100644
> index 000..1bbf3ca
> --- /dev/null
> +++ b/include/drm/bridge/dw_hdmi.h
> @@ -0,0 +1,57 @@
> +/*
> + * Copyright (C) 2011 Freescale Semiconductor, Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#ifndef __DW_HDMI__
> +#define __DW_HDMI__
> +
> +#include
> +
> +enum {
> + RES_8,
> + RES_10,
> + RES_12,
> + RES_MAX,
> +};
> +
> +enum dw_hdmi_devtype {
> + IMX6Q_HDMI,
> + IMX6DL_HDMI,
> +};
> +
> +struct mpll_config {
> + unsigned long mpixelclock;
> + struct {
> + u16 cpce;
> + u16 gmp;
> + } res[RES_MAX];
> +};
> +
> +struct curr_ctrl {
> + unsigned long mpixelclock;
> + u16 curr[RES_MAX];
> +};
> +
> +struct sym_term {
> + unsigned long mpixelclock;
> + u16 sym_ctr;/*clock symbol and transmitter control*/
> + u16 term; /*transmission termination value*/
> +};
since this is going to be used by multiple drivers, the enums and
structs should all be properly namespaced. How about DW_HDMI_RES_x,
struct dw_hdmi_mpll_config, struct dw_hdmi_curr_ctrl, and struct
dw_hdmi_sym_term?
> +struct dw_hdmi_plat_data {
> + enum dw_hdmi_devtype dev_type;
> + const struct mpll_config *mpll_cfg;
> + const struct curr_ctrl *cur_ctr;
> + const struct sym_term *sym_term;
> +};
> +
> +void dw_hdmi_unbind(struct device *dev, struct device *master, void *data);
> +int dw_hdmi_bind(struct device *dev, struct device *master,
> + void *data, struct drm_encoder *encoder,
> + const struct dw_hdmi_plat_data *plat_data);
> +#endif /* __IMX_HDMI_H__ */
regards
Philipp
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v13 07/12] drm: bridge/dw_hdmi: add support for multi-byte register width access
Am Freitag, den 28.11.2014, 17:43 +0800 schrieb Andy Yan:
> Hi Zabel:
> On 2014年11月27日 00:34, Philipp Zabel wrote:
> > Am Mittwoch, den 26.11.2014, 21:32 +0800 schrieb Andy Yan:
> >> On rockchip rk3288, only word(32-bit) accesses are
> >> permitted for hdmi registers. Byte width accesses (writeb,
> >> readb) generate an imprecise external abort.
> >>
> >> Signed-off-by: Andy Yan
> >>
> >> ---
> >>
> >> Changes in v13: None
> >> Changes in v12: None
> >> Changes in v11: None
> >> Changes in v10: None
> >> Changes in v9: None
> >> Changes in v8: None
> >> Changes in v7: None
> >> Changes in v6:
> >> - refactor register access without reg_shift
> >>
> >> Changes in v5:
> >> - refactor reg-io-width
> >>
> >> Changes in v4: None
> >> Changes in v3:
> >> - split multi-register access to one indepent patch
> >>
> >> drivers/gpu/drm/bridge/dw_hdmi.c | 57
> >> +++-
> >> 1 file changed, 51 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/bridge/dw_hdmi.c
> >> b/drivers/gpu/drm/bridge/dw_hdmi.c
> >> index a53bf63..5e88c8d 100644
> >> --- a/drivers/gpu/drm/bridge/dw_hdmi.c
> >> +++ b/drivers/gpu/drm/bridge/dw_hdmi.c
> >> @@ -100,6 +100,11 @@ struct hdmi_data_info {
> >>struct hdmi_vmode video_mode;
> >> };
> >>
> >> +union dw_reg_ptr {
> >> + u32 __iomem *p32;
> >> + u8 __iomem *p8;
> >> +};
> > I see no need to introduce this. Just explicitly multiply the offset in
> > dw_hdmi_writel.
> >
> Is there any disadvantage to do like this?
> The compiler can help us do the explicitly multiply by this way.
Four additional lines, a new defined type, a few more changes to struct
dw_hdmi and dw_hdmi_bind necessary.
Technically I see no problem to let the compiler do the multiplication,
my issue is that it ever so slightly obfuscates the code. Instead of
just writing "* 4" in two functions, we get a new union that you need to
know about when looking at struct dw_hdmi and dw_hdmi_bind, regs.p8 is
used but never assigned directly, it's just a tiny bit of additional
effort needed to understand the code. But when the cost to avoid that is
so small...
regards
Philipp
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v5 1/6] of: Decrement refcount of previous endpoint in of_graph_get_next_endpoint
Am Sonntag, den 09.11.2014, 16:51 +0100 schrieb Guennadi Liakhovetski:
> On Sun, 9 Nov 2014, Philipp Zabel wrote:
>
> > Hi Guennadi,
> >
> > On Fri, Nov 07, 2014 at 11:06:21PM +0100, Guennadi Liakhovetski wrote:
> > > Hi Philipp,
> > >
> > > Thanks for the patch and sorry for a late reply. I did look at your
> > > patches earlier too, but maybe not attentively enough, or maybe I'm
> > > misunderstanding something now. In the scan_of_host() function in
> > > soc_camera.c as of current -next I see:
> > >
> > > epn = of_graph_get_next_endpoint(np, epn);
> > >
> > > which already looks like a refcount leak to me. If epn != NULL, its
> > > refcount is incremented, but then immediately the variable gets
> > > overwritten, and there's no extra copy of that variable to fix this. If
> > > I'm right, then that bug in itself should be fixed, ideally before your
> > > patch is applied. But in fact, your patch fixes this, since it modifies
> > > of_graph_get_next_endpoint() to return with prev's refcount not
> > > incremented, right? Whereas the of_node_put(epn) later down in
> > > scan_of_host() decrements refcount of the _next_ endpoint, not the
> > > previous one, so, it should be left alone? I.e. AFAICT your modification
> > > to of_graph_get_next_endpoint() fixes soc_camera.c with no further
> > > modifications to it required?
> >
> > You are right. With the old implementation, you'd have to do the
> > epn = of_graph_get_next_endpoint(np, prev); of_node_put(prev); prev = epn;
> > dance to avoid leaking a reference to the first endpoint. This series
> > accidentally fixes soc_camera by changing of_graph_get_next_endpoint
> > to decrement the reference count itself.
>
> Right, so, the patch has to be adjusted not to touch soc_camera.c at all.
No. As of the current media-tree we still need move the of_node_put(epn)
out of the loop:
diff --git a/drivers/media/platform/soc_camera/soc_camera.c
b/drivers/media/platform/soc_camera/soc_camera.c
index f4308fe..619b2d4 100644
--- a/drivers/media/platform/soc_camera/soc_camera.c
+++ b/drivers/media/platform/soc_camera/soc_camera.c
@@ -1696,7 +1696,6 @@ static void scan_of_host(struct soc_camera_host *ici)
if (!i)
soc_of_bind(ici, epn, ren->parent);
- of_node_put(epn);
of_node_put(ren);
if (i) {
@@ -1704,6 +1703,8 @@ static void scan_of_host(struct soc_camera_host *ici)
break;
}
}
+
+ of_node_put(epn);
}
#else
We can do this in two steps (step 1 fixing the current status quo, step
2 as part of this series). Step 1:
--- a/drivers/media/platform/soc_camera/soc_camera.c
+++ b/drivers/media/platform/soc_camera/soc_camera.c
@@ -1691,11 +1691,13 @@ static void scan_of_host(struct soc_camera_host *ici)
{
struct device *dev = ici->v4l2_dev.dev;
struct device_node *np = dev->of_node;
- struct device_node *epn = NULL, *ren;
+ struct device_node *epn, *prev = NULL, *ren;
unsigned int i;
for (i = 0; ; i++) {
- epn = of_graph_get_next_endpoint(np, epn);
+ epn = of_graph_get_next_endpoint(np, prev);
+ of_node_put(prev);
+ prev = epn;
if (!epn)
break;
@@ -1710,7 +1712,6 @@ static void scan_of_host(struct soc_camera_host *ici)
if (!i)
soc_of_bind(ici, epn, ren->parent);
- of_node_put(epn);
of_node_put(ren);
if (i) {
And step 2:
diff --git a/drivers/media/platform/soc_camera/soc_camera.c
b/drivers/media/platform/soc_camera/soc_camera.c
index 3d44773..1de62bf 100644
--- a/drivers/media/platform/soc_camera/soc_camera.c
+++ b/drivers/media/platform/soc_camera/soc_camera.c
@@ -1691,13 +1691,11 @@ static void scan_of_host(struct soc_camera_host *ici)
{
struct device *dev = ici->v4l2_dev.dev;
struct device_node *np = dev->of_node;
- struct device_node *epn, *prev = NULL, *ren;
+ struct device_node *epn = NULL, *ren;
unsigned int i;
for (i = 0; ; i++) {
- epn = of_graph_get_next_endpoint(np, prev);
- of_node_put(prev);
- prev = epn;
+ epn = of_graph_get_next_endpoint(np, epn);
if (!epn)
break;
@@ -1719,6 +1717,8 @@ static void scan_of_host(struct soc_camera_host *ici)
break;
}
}
+
+ of_node_put(epn);
}
#else
Would you prefer that option?
regards
Philipp
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v15 12/12] drm: bridge/dw_hdmi: add rockchip rk3288 support
Hi Andy,
Am Dienstag, den 02.12.2014, 15:45 +0800 schrieb Andy Yan:
[...]
> +static int dw_hdmi_rockchip_bind(struct device *dev, struct device *master,
> + void *data)
> +{
> + struct platform_device *pdev = to_platform_device(dev);
> + const struct dw_hdmi_plat_data *plat_data;
> + const struct of_device_id *match;
> + struct drm_device *drm = data;
> + struct drm_encoder *encoder;
> + struct rockchip_hdmi *hdmi;
> + int ret;
> +
> + if (!pdev->dev.of_node)
> + return -ENODEV;
> +
> + hdmi = devm_kzalloc(&pdev->dev, sizeof(*hdmi), GFP_KERNEL);
> + if (!hdmi)
> + return -ENOMEM;
> +
> + match = of_match_node(dw_hdmi_rockchip_ids, pdev->dev.of_node);
> + plat_data = match->data;
> + hdmi->dev = &pdev->dev;
> + encoder = &hdmi->encoder;
> + platform_set_drvdata(pdev, hdmi);
> +
> + ret = rockchip_hdmi_parse_dt(hdmi);
> + if (ret) {
> + dev_err(hdmi->dev, "Unable to parse OF data\n");
> + return ret;
> + }
> +
> + ret = clk_prepare_enable(hdmi->clk);
> + if (ret) {
> + dev_err(hdmi->dev, "Cannot enable HDMI clock: %d\n", ret);
> + return ret;
> + }
> +
> + ret = clk_prepare_enable(hdmi->hdcp_clk);
> + if (ret) {
> + dev_err(hdmi->dev, "Cannot enable HDMI hdcp clock: %d\n", ret);
> + return ret;
> + }
Could we have a look at the clocks again? Basically the Rockchip clock
handling is exactly the same, except the clocks are called by other
names.
On i.MX6, according to the reference manual, the HDMI TX module has four
clock inputs: "iahbclk" (bus clock), "icecclk" (32 kHz CEC clock),
"ihclk" (module clock), and "isfrclk" (27 MHz internal SFR clock).
The "iahbclk" and "ihclk" are both sourced from the SoC AHB root clock,
the 32 kHz reference input can't be gated, and the "isfrclk" has its own
gate.
Does the HDMI TX implementation on Rockchip still have the separate
external sfr bus and module clock inputs? I assume that your "clk" input
is a single gate bit for bus and module clocks at the same time?
If possible, I'd prefer to find a common binding for the clocks with
some of the clocks being optional, but for that we need to know the
actual clock inputs to the HDMI TX module.
regards
Philipp
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v15 12/12] drm: bridge/dw_hdmi: add rockchip rk3288 support
Hi Andy,
Am Dienstag, den 02.12.2014, 20:34 +0800 schrieb Andy Yan:
> Hi Philipp:
> On 2014年12月02日 18:24, Philipp Zabel wrote:
> > Hi Andy,
> >
> > Am Dienstag, den 02.12.2014, 15:45 +0800 schrieb Andy Yan:
> > [...]
> >> +static int dw_hdmi_rockchip_bind(struct device *dev, struct device
> >> *master,
> >> + void *data)
> >> +{
> >> + struct platform_device *pdev = to_platform_device(dev);
> >> + const struct dw_hdmi_plat_data *plat_data;
> >> + const struct of_device_id *match;
> >> + struct drm_device *drm = data;
> >> + struct drm_encoder *encoder;
> >> + struct rockchip_hdmi *hdmi;
> >> + int ret;
> >> +
> >> + if (!pdev->dev.of_node)
> >> + return -ENODEV;
> >> +
> >> + hdmi = devm_kzalloc(&pdev->dev, sizeof(*hdmi), GFP_KERNEL);
> >> + if (!hdmi)
> >> + return -ENOMEM;
> >> +
> >> + match = of_match_node(dw_hdmi_rockchip_ids, pdev->dev.of_node);
> >> + plat_data = match->data;
> >> + hdmi->dev = &pdev->dev;
> >> + encoder = &hdmi->encoder;
> >> + platform_set_drvdata(pdev, hdmi);
> >> +
> >> + ret = rockchip_hdmi_parse_dt(hdmi);
> >> + if (ret) {
> >> + dev_err(hdmi->dev, "Unable to parse OF data\n");
> >> + return ret;
> >> + }
> >> +
> >> + ret = clk_prepare_enable(hdmi->clk);
> >> + if (ret) {
> >> + dev_err(hdmi->dev, "Cannot enable HDMI clock: %d\n", ret);
> >> + return ret;
> >> + }
> >> +
> >> + ret = clk_prepare_enable(hdmi->hdcp_clk);
> >> + if (ret) {
> >> + dev_err(hdmi->dev, "Cannot enable HDMI hdcp clock: %d\n", ret);
> >> + return ret;
> >> + }
> > Could we have a look at the clocks again? Basically the Rockchip clock
> > handling is exactly the same, except the clocks are called by other
> > names.
> >
> > On i.MX6, according to the reference manual, the HDMI TX module has four
> > clock inputs: "iahbclk" (bus clock), "icecclk" (32 kHz CEC clock),
> > "ihclk" (module clock), and "isfrclk" (27 MHz internal SFR clock).
> > The "iahbclk" and "ihclk" are both sourced from the SoC AHB root clock,
> > the 32 kHz reference input can't be gated, and the "isfrclk" has its own
> > gate.
> >
> > Does the HDMI TX implementation on Rockchip still have the separate
> > external sfr bus and module clock inputs? I assume that your "clk" input
> > is a single gate bit for bus and module clocks at the same time?
> > If possible, I'd prefer to find a common binding for the clocks with
> > some of the clocks being optional, but for that we need to know the
> > actual clock inputs to the HDMI TX module.
> >
> > regards
> > Philipp
> >
> There are three individual clock inputs on Rockchip RK3288 HDMI:
> "hdmi_ctrl_clk",
> "hdmi_cec_clk", "hdmi_hdcp_clk", the three clocks are responsible
> for different
> functions as their name described, and have their own private gate
> bit. That is
> to say, the cec_clk and hdcp_clk can all be disabled if we don't
> need hdcp and cec
> function.
> So I think it's better to make the clk control platform independent.
My question is not about the available gates at the SoC level, but about
the actual clock inputs from point of view of the HDMI TX IP.
It could be that the hdmi_ctrl_clk gates all inputs to the module and
bus clocks together. If so, you could just reuse "isfr" and "iahb" and
set it to the same clock. If not, we'd need to think of something else.
Unfortunately I don't have any Synopsys documentation of the HDMI TX at
that level.
regards
Philipp
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v15 06/12] dt-bindings: add document for dw_hdmi
Hi Andy,
Am Dienstag, den 02.12.2014, 15:42 +0800 schrieb Andy Yan:
> diff --git a/Documentation/devicetree/bindings/drm/bridge/dw_hdmi.txt
> b/Documentation/devicetree/bindings/drm/bridge/dw_hdmi.txt
> new file mode 100644
> index 000..107c1ca
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/drm/bridge/dw_hdmi.txt
> @@ -0,0 +1,40 @@
> +DesignWare HDMI bridge bindings
> +
> +Required properities:
> +- compatible: platform specific such as:
> + * "fsl,imx6q-hdmi"
> + * "fsl,imx6dl-hdmi"
> + * "rockchip,rk3288-dw-hdmi"
I think we should add a common compatible value "snps,dw-hdmi-tx" here:
compatible = "fsl,imx6q-hdmi", "snps,dw-hdmi-tx";
> +- reg: Physical base address and length of the controller's registers.
> +- ddc-i2c-bus: phandle of an I2C controller used for DDC EDID probing
Better make ddc-i2c-bus optional, see the other thread about the ddc i2c
master.
> +- interrupts: The HDMI interrupt number
> +
> +Optional properties
> +- reg-io-width: the width of the reg:1,4, default set to 1 if not present
> +
> +Example:
> + hdmi: hdmi@012 {
> + compatible = "fsl,imx6q-hdmi";
> + reg = <0x0012 0x9000>;
> + interrupts = <0 115 0x04>;
> + gpr = <&gpr>;
> + clocks = <&clks 123>, <&clks 124>;
> + clock-names = "iahb", "isfr";
> + ddc-i2c-bus = <&i2c2>;
> +
> + port@0 {
> + reg = <0>;
> +
> + hdmi_mux_0: endpoint {
> + remote-endpoint = <&ipu1_di0_hdmi>;
> + };
> + };
> +
> + port@1 {
> + reg = <1>;
> +
> + hdmi_mux_1: endpoint {
> + remote-endpoint = <&ipu1_di1_hdmi>;
> + };
> + };
> + };
regards
Philipp
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v15 06/12] dt-bindings: add document for dw_hdmi
Hi Andy, Am Mittwoch, den 03.12.2014, 08:54 +0800 schrieb Andy Yan: > >> +Required properities: > >> +- compatible: platform specific such as: > >> + * "fsl,imx6q-hdmi" > >> + * "fsl,imx6dl-hdmi" > >> + * "rockchip,rk3288-dw-hdmi" > > I think we should add a common compatible value "snps,dw-hdmi-tx" here: > > > > compatible = "fsl,imx6q-hdmi", "snps,dw-hdmi-tx"; > > > How about "snps,dw-hdmi", because the driver is not only about >hdmi tx, but also include hdmi phy. Synopsys call the whole module "DesignWare HDMI Transmitter (TX) IP Solution": https://www.synopsys.com/dw/ipdir.php?ds=dwc_hdmi_14_csds_tx https://www.synopsys.com/dw/ipdir.php?ds=dwc_hdmi_20_csds_tx That includes the PHY. I'd prefer keeping the -tx in there to differentiate from a possible future "snps,dw-hdmi-rx": https://www.synopsys.com/dw/ipdir.php?ds=dwc_hdmi_14_csds_rx https://www.synopsys.com/dw/ipdir.php?ds=dwc_hdmi_20_csds_rx >If we add such compatible value, do we have to implement another > platform driver like dw_hdmi-pltfm.c with the > compatible="snps,dw-hdmi" , > or just include the compatible value in dw_hdmi-imx.c and > dw_hdmi-rockchip.c? That common compatible doesn't have to be used by any driver. It's just there to show these are the same/similar IP core. If a common driver without any SoC specific knowledge could be written, that one would match against the common compatible. regards Philipp ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v15 06/12] dt-bindings: add document for dw_hdmi
Am Mittwoch, den 03.12.2014, 17:46 +0800 schrieb Andy Yan: > On 2014年12月03日 02:23, Philipp Zabel wrote: > > Hi Andy, > > > > Am Dienstag, den 02.12.2014, 15:42 +0800 schrieb Andy Yan: > >> diff --git a/Documentation/devicetree/bindings/drm/bridge/dw_hdmi.txt > >> b/Documentation/devicetree/bindings/drm/bridge/dw_hdmi.txt > >> new file mode 100644 > >> index 000..107c1ca > >> --- /dev/null > >> +++ b/Documentation/devicetree/bindings/drm/bridge/dw_hdmi.txt > >> @@ -0,0 +1,40 @@ > >> +DesignWare HDMI bridge bindings > >> + > >> +Required properities: > >> +- compatible: platform specific such as: > >> + * "fsl,imx6q-hdmi" > >> + * "fsl,imx6dl-hdmi" > >> + * "rockchip,rk3288-dw-hdmi" > > I think we should add a common compatible value "snps,dw-hdmi-tx" here: > > > > compatible = "fsl,imx6q-hdmi", "snps,dw-hdmi-tx"; > > > >> +- reg: Physical base address and length of the controller's registers. > >> +- ddc-i2c-bus: phandle of an I2C controller used for DDC EDID probing > > Better make ddc-i2c-bus optional, see the other thread about the ddc i2c > > master. > I have the same idea too, but the patch about ddc i2c master has not > landed yet, can we change the ddc-i2c-bus to optional after the ddc > i2c master > patch land? Check out Documentation/devicetree/bindings/drm/imx/hdmi.txt, it was already marked as optional. We can't make it required now. regards Philipp ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v15 12/12] drm: bridge/dw_hdmi: add rockchip rk3288 support
Hi Andy,
Am Mittwoch, den 03.12.2014, 20:32 +0800 schrieb Andy Yan:
> > My question is not about the available gates at the SoC level, but about
> > the actual clock inputs from point of view of the HDMI TX IP.
> >
> > It could be that the hdmi_ctrl_clk gates all inputs to the module and
> > bus clocks together. If so, you could just reuse "isfr" and "iahb" and
> > set it to the same clock. If not, we'd need to think of something else.
> > Unfortunately I don't have any Synopsys documentation of the HDMI TX at
> > that level.
>
> After confirming with the IC designer, we finally make clear that
> Rockchip RK3288 almost use the same clock design with imx:
> clk-iahbclk, used for hdmi module and bus
> hdcp_clk-isfrclk, used for hdcp and i2cm
> cecclk -cecclk, but this clk can be gated on rockchip, this is
> different with imx,
> but we don't handle the cec stuff now. So i will try to reuse the
> imx clk binds. do you
>think that is ok?
Thank you for taking the time to verify this. So we should move the
clock handling out of the soc specific parts into the common driver and
reuse the existing clock bindings ("iahb", "isfr").
I'd suggest to add the "cec" clock now to the binding document as an
optional clock, then you can already specify it in the rockchip dtsi.
regards
Philipp
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v16 03/12] drm: imx: imx-hdmi: convert imx-hdmi to drm_bridge mode
Hi Andy,
Am Donnerstag, den 04.12.2014, 00:04 +0800 schrieb Andy Yan:
> On 2014年12月03日 23:38, Russell King - ARM Linux wrote:
> > On Wed, Dec 03, 2014 at 11:29:26PM +0800, Andy Yan wrote:
> >> +int imx_hdmi_bind(struct device *dev, struct device *master,
> >> +void *data, struct drm_encoder *encoder,
> >> +const struct imx_hdmi_plat_data *plat_data)
> >> {
> >>struct platform_device *pdev = to_platform_device(dev);
> >> - const struct of_device_id *of_id =
> >> - of_match_device(imx_hdmi_dt_ids, dev);
> >>struct drm_device *drm = data;
> >>struct device_node *np = dev->of_node;
> >>struct device_node *ddc_node;
> >> @@ -1594,19 +1566,16 @@ static int imx_hdmi_bind(struct device *dev,
> >> struct device *master, void *data)
> >>struct resource *iores;
> >>int ret, irq;
> >>
> >> - hdmi = devm_kzalloc(dev, sizeof(*hdmi), GFP_KERNEL);
> >> + hdmi = devm_kzalloc(&pdev->dev, sizeof(*hdmi), GFP_KERNEL);
> >>if (!hdmi)
> >>return -ENOMEM;
> >>
> >> - hdmi->dev = dev;
> >> + hdmi->plat_data = plat_data;
> >> + hdmi->dev = &pdev->dev;
> >> + hdmi->dev_type = plat_data->dev_type;
> >>hdmi->sample_rate = 48000;
> >>hdmi->ratio = 100;
> >> -
> >> - if (of_id) {
> >> - const struct platform_device_id *device_id = of_id->data;
> >> -
> >> - hdmi->dev_type = device_id->driver_data;
> >> - }
> >> + hdmi->encoder = encoder;
> > I'd suggest changing imx_hdmi_bind() to take the struct resource and irq
> > number, and avoiding the platform device stuff altogether in here.
> >
> Actually this is what the current code do: the resource and irq number
> are all handled in imx_hdmi_bind
It would be better if the bind function would not have to care about
platform resources, that should be handled in the probe function. I had
a patch to move them:
http://lists.freedesktop.org/archives/dri-devel/2014-May/059630.html
Maybe you could incorporate something like this?
regards
Philipp
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v16 03/12] drm: imx: imx-hdmi: convert imx-hdmi to drm_bridge mode
Hi Russell, Am Mittwoch, den 03.12.2014, 16:30 + schrieb Russell King - ARM Linux: > On Wed, Dec 03, 2014 at 05:20:15PM +0100, Philipp Zabel wrote: > > Hi Andy, > > > > It would be better if the bind function would not have to care about > > platform resources, that should be handled in the probe function. I had > > a patch to move them: > > > > http://lists.freedesktop.org/archives/dri-devel/2014-May/059630.html > > > > Maybe you could incorporate something like this? > > Personally, I hate this idea. Having a two-layered setup means that > the when the bind() method is called, the state of struct imx_hdmi is > indeterminant. > > If it's called immediately from probe, most of the structure will be > zeroed, and only those members initialised by the probe function will > be set to non-zero values. > > However, if the HDMI interface has been previously bound, and is > subsequently re-bound, then the structure will most definitely no > longer be in a known state on the second bind() call. > > This is fragile. > > Now, people have tried to tell me that this isn't fragile, but, I now > have proof that it is as fragile as I fear. The component helper > doesn't yet have that many users, and already we have one user (okay, > it's not part of the mainline kernel - it's etnaviv) which contained > exactly this kind of bug: it expected its private structures to be > zeroed on the bind() call. > > So, I /really/ hate this idea. If you really want to do this, then > please ensure that the bind() call explicitly zeros the bits of the > struct which aren't initialised by the probe() call, so we know that > the driver will always start off with a known initial state. You are right, no I don't want this. When I initially wrote this patch I was under the impression that the memory allocated by devm_kzalloc in bind() wouldn't be freed on unbind(). I remember I stopped pursuing this patch when I got aware of the devres_open/close/remove_group dance in the component framework code, but somehow forgot to drop it altogether locally. regards Philipp ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v17 06/12] dt-bindings: add document for dw_hdmi
Hi Andy,
Am Donnerstag, den 04.12.2014, 18:06 +0800 schrieb Andy Yan:
> Signed-off-by: Andy Yan
>
> ---
>
> Changes in v17: None
> Changes in v16:
> - describe ddc-i2c-bus as optional
> - add common clocks bindings
>
> Changes in v15: None
> Changes in v14: None
> Changes in v13: None
> Changes in v12: None
> Changes in v11: None
> Changes in v10: None
> Changes in v9: None
> Changes in v8:
> - correct some spelling mistake
> - modify ddc-i2c-bus and interrupt description
>
> Changes in v7: None
> Changes in v6: None
> Changes in v5: None
> Changes in v4: None
> Changes in v3: None
>
> .../devicetree/bindings/drm/bridge/dw_hdmi.txt | 45
> ++
> 1 file changed, 45 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/drm/bridge/dw_hdmi.txt
>
> diff --git a/Documentation/devicetree/bindings/drm/bridge/dw_hdmi.txt
> b/Documentation/devicetree/bindings/drm/bridge/dw_hdmi.txt
> new file mode 100644
> index 000..fb14730
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/drm/bridge/dw_hdmi.txt
> @@ -0,0 +1,45 @@
> +DesignWare HDMI bridge bindings
> +
> +Required properities:
> +- compatible: platform specific such as:
> + * "snps,dw-hdmi-tx"
> + * "fsl,imx6q-hdmi"
> + * "fsl,imx6dl-hdmi"
> + * "rockchip,rk3288-dw-hdmi"
> +- reg: Physical base address and length of the controller's registers.
> +- interrupts: The HDMI interrupt number
> +- clocks, clock-names : must have the phandles to the HDMI iahb and isrf
> clocks,
> + as described in Documentation/devicetree/bindings/clock/clock-bindings.txt,
> + the clock-names should be "iahb", "isfr"
This is missing the port bindings, I think it should mention the port
property here and refer to the soc specific binding document.
- port@...: SoC specific port nodes with endpoint definitions as defined
in Documentation/devicetree/bindings/media/video-interfaces.txt,
please refer to the SoC specific binding document:
* Documentation/devicetree/bindings/drm/imx/hdmi.txt
* Documentation/devicetree/bindings/video/dw_hdmi-rockchip.txt
This also makes be wonder, shouldn't dw_hdmi-rockchip be under drm/?
> +Optional properties
> +- reg-io-width: the width of the reg:1,4, default set to 1 if not present
> +- ddc-i2c-bus: phandle of an I2C controller used for DDC EDID probing
> +- clkocks, clock-names: phandle to the HDMI cec clock, name should be "cec"
s/clkocks/clocks/, and I'd uppercase the HDMI CEC clock for consistency.
> +
> +Example:
> + hdmi: hdmi@012 {
> + compatible = "fsl,imx6q-hdmi";
Could you change this example to:
compatible = "fsl,imx6q-hdmi", "snps,dw-hdmi-tx"
> + reg = <0x0012 0x9000>;
> + interrupts = <0 115 0x04>;
> + gpr = <&gpr>;
> + clocks = <&clks 123>, <&clks 124>;
> + clock-names = "iahb", "isfr";
> + ddc-i2c-bus = <&i2c2>;
> +
> + port@0 {
> + reg = <0>;
> +
> + hdmi_mux_0: endpoint {
> + remote-endpoint = <&ipu1_di0_hdmi>;
> + };
> + };
> +
> + port@1 {
> + reg = <1>;
> +
> + hdmi_mux_1: endpoint {
> + remote-endpoint = <&ipu1_di1_hdmi>;
> + };
> + };
> + };
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v18 06/12] dt-bindings: add document for dw_hdmi
Am Freitag, den 05.12.2014, 14:27 +0800 schrieb Andy Yan:
> Signed-off-by: Andy Yan
This binding is mostly a copy of the existing
Documentation/devicetree/bindings/drm/imx/hdmi.txt, but there is a new
reg-io-width property to configure the register access bus width and we
have added new compatibles "rockchip,rk3288-dw-hdmi" and the common
"snps,dw-hdmi-tx". Could we get an Ack for this and patch 11 by the
device tree maintainers?
regards
Philipp
> ---
>
> Changes in v18:
> - add port bindings
> - correct some spelling mistakes in dw_hdmi bindings doc
>
> Changes in v17: None
> Changes in v16:
> - describe ddc-i2c-bus as optional
> - add common clocks bindings
>
> Changes in v15: None
> Changes in v14: None
> Changes in v13: None
> Changes in v12: None
> Changes in v11: None
> Changes in v10: None
> Changes in v9: None
> Changes in v8:
> - correct some spelling mistake
> - modify ddc-i2c-bus and interrupt description
>
> Changes in v7: None
> Changes in v6: None
> Changes in v5: None
> Changes in v4: None
> Changes in v3: None
>
> .../devicetree/bindings/drm/bridge/dw_hdmi.txt | 50
> ++
> 1 file changed, 50 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/drm/bridge/dw_hdmi.txt
>
> diff --git a/Documentation/devicetree/bindings/drm/bridge/dw_hdmi.txt
> b/Documentation/devicetree/bindings/drm/bridge/dw_hdmi.txt
> new file mode 100644
> index 000..a905c14
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/drm/bridge/dw_hdmi.txt
> @@ -0,0 +1,50 @@
> +DesignWare HDMI bridge bindings
> +
> +Required properties:
> +- compatible: platform specific such as:
> + * "snps,dw-hdmi-tx"
> + * "fsl,imx6q-hdmi"
> + * "fsl,imx6dl-hdmi"
> + * "rockchip,rk3288-dw-hdmi"
> +- reg: Physical base address and length of the controller's registers.
> +- interrupts: The HDMI interrupt number
> +- clocks, clock-names : must have the phandles to the HDMI iahb and isfr
> clocks,
> + as described in Documentation/devicetree/bindings/clock/clock-bindings.txt,
> + the clocks are soc specific, the clock-names should be "iahb", "isfr"
> +-port@[X]: SoC specific port nodes with endpoint definitions as defined
> + in Documentation/devicetree/bindings/media/video-interfaces.txt,
> + please refer to the SoC specific binding document:
> +* Documentation/devicetree/bindings/drm/imx/hdmi.txt
> +* Documentation/devicetree/bindings/video/dw_hdmi-rockchip.txt
> +
> +Optional properties
> +- reg-io-width: the width of the reg:1,4, default set to 1 if not present
> +- ddc-i2c-bus: phandle of an I2C controller used for DDC EDID probing
> +- clocks, clock-names: phandle to the HDMI CEC clock, name should be "cec"
> +
> +Example:
> + hdmi: hdmi@012 {
> + compatible = "fsl,imx6q-hdmi";
> + reg = <0x0012 0x9000>;
> + interrupts = <0 115 0x04>;
> + gpr = <&gpr>;
> + clocks = <&clks 123>, <&clks 124>;
> + clock-names = "iahb", "isfr";
> + ddc-i2c-bus = <&i2c2>;
> +
> + port@0 {
> + reg = <0>;
> +
> + hdmi_mux_0: endpoint {
> + remote-endpoint = <&ipu1_di0_hdmi>;
> + };
> + };
> +
> + port@1 {
> + reg = <1>;
> +
> + hdmi_mux_1: endpoint {
> + remote-endpoint = <&ipu1_di1_hdmi>;
> + };
> + };
> + };
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v18 0/12] dw-hdmi: convert imx hdmi to bridge/dw_hdmi
Hi Andy, Am Freitag, den 05.12.2014, 14:22 +0800 schrieb Andy Yan: > We found Freescale imx6 and Rockchip rk3288 and Ingenic JZ4780 (Xburst/MIPS) > use the interface compatible Designware HDMI IP, but they also have some > lightly differences, such as phy pll configuration, register width(imx hdmi > register is one byte, but rk3288 is 4 bytes width and can only be accessed > by word), 4K support(imx6 doesn't support 4k, but rk3288 does), and HDMI2.0 > support. > > To reuse the imx-hdmi driver, we make this patch set: > (1): fix some CodingStyle warning to make checkpatch happy > (2): convert imx-hdmi to drm_bridge > (3): split platform specific code > (4): move imx-hdmi to bridge/dw_hdmi > (5): extend dw_hdmi.c to support rk3288 hdmi > (6): add rockchip rk3288 platform specific code dw_hdmi-rockchip.c > > Changes in v18: > - remove a multiple blank lines in imx-hdmi.c > - fix a checkpatch warning in imx-hdmi_pltfm.c > - add port bindings > - correct some spelling mistakes in dw_hdmi bindings doc > - correct some spelling mistakes in dw_hdmi-rockchip bindings doc [...] I am happy with the series so far. Pending Acks from the device tree maintainers for the new binding documents, I'd like to apply either the whole of it on top of git://git.pengutronix.de/git/pza/linux.git imx-drm/next or take at least the i.MX specific patches (1-5) because of the dependency on the imx-drm OF helper conversion. regards Philipp ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v18 0/12] dw-hdmi: convert imx hdmi to bridge/dw_hdmi
Hi Heiko, Am Dienstag, den 06.01.2015, 12:49 +0100 schrieb Heiko Stübner: > Hi Philipp, > > Am Samstag, 6. Dezember 2014, 00:31:46 schrieb Andy Yan: > > > I am happy with the series so far. Pending Acks from the device tree > > > maintainers for the new binding documents, I'd like to apply either the > > > whole of it on top of > > > > > > git://git.pengutronix.de/git/pza/linux.git imx-drm/next > > > > > > or take at least the i.MX specific patches (1-5) because of the > > > dependency on the imx-drm OF helper conversion. > > do you still want to take this series? > > As for the devicetree ACK, there is this (unwritten) rule that if the > dt-maintainers do not respond after 3 weeks (and a ping mail to them) > it should be considered acked. As this version of the series is sitting > on the lists for a month now and nobody complained during the other > 17 versions as well, I think we should be on the safe side :-) Alright, let's assume silent approval. > There is one slight catch. Patch 3 needs a little modification, as the > THIS_MODULE ower of the imx_hdmi got meanwhile cleaned up. > > So the patch would need to be modified as shown by the diff at the > bottom. I'm not sure if you want Andy to repost the whole series again > but I'll just post my fixed variant as reply to the origin v18 of patch 3 > for convenience. No need to post this again, I can apply the patches and fix up the remaining issues you and Russell pointed out in the process. regards Philipp ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v8 5/5] media: imx: Try colorimetry at both sink and source pads
Hi Steve, On Tue, 2019-05-21 at 18:03 -0700, Steve Longerbeam wrote: > Retask imx_media_fill_default_mbus_fields() to try colorimetry parameters, > renaming it to to imx_media_try_colorimetry(), and call it at both sink and > source pad try_fmt's. The unrelated check for uninitialized field value is > moved out to appropriate places in each subdev try_fmt. > > The IC now supports Rec.709 and BT.601 Y'CbCr encoding, and both limited > and full range quantization for both YUV and RGB space, so allow those > for pipelines that route through the IC. > > Signed-off-by: Steve Longerbeam I've applied them on the imx-drm/next branch with Hans' Acked-by on 5/5. regards Philipp ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] media: staging/imx: Fix NULL deref in find_pipeline_entity()
On Wed, 2019-06-26 at 11:52 -0700, Steve Longerbeam wrote:
> Fix a cut&paste error in find_pipeline_entity(). The start entity must be
> passed to media_entity_to_video_device() in find_pipeline_entity(), not
> pad->entity. The pad is only put to use later, after determining the start
> entity is not the entity being searched for.
>
> Fixes: 3ef46bc97ca2 ("media: staging/imx: Improve pipeline searching")
>
> Reported-by: Colin Ian King
> Signed-off-by: Steve Longerbeam
> ---
> drivers/staging/media/imx/imx-media-utils.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/media/imx/imx-media-utils.c
> b/drivers/staging/media/imx/imx-media-utils.c
> index b5b8a3b7730a..6fb88c22ee27 100644
> --- a/drivers/staging/media/imx/imx-media-utils.c
> +++ b/drivers/staging/media/imx/imx-media-utils.c
> @@ -842,7 +842,7 @@ find_pipeline_entity(struct media_entity *start, u32
> grp_id,
> if (sd->grp_id & grp_id)
> return &sd->entity;
> } else if (buftype && is_media_entity_v4l2_video_device(start)) {
> - vfd = media_entity_to_video_device(pad->entity);
> + vfd = media_entity_to_video_device(start);
> if (buftype == vfd->queue->type)
> return &vfd->entity;
> }
Reviewed-by: Philipp Zabel
regards
Philipp
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] imx-csi: fix burst size
Hi Russell, On Fri, 2017-09-29 at 22:41 +0100, Russell King wrote: > Setting a burst size of "8" doesn't work for IMX219 with 8-bit bayer, > but a burst size of "16" does. Fix this. Do larger bursts work as well, if the width is divisible by the burst length? Since the Bayer format can't pass through the IC direct path anyway, 32-byte or 64-byte bursts may be possible. > Signed-off-by: Russell King > --- > drivers/staging/media/imx/imx-media-csi.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/staging/media/imx/imx-media-csi.c > b/drivers/staging/media/imx/imx-media-csi.c > index 6d856118c223..e27bcdb63973 100644 > --- a/drivers/staging/media/imx/imx-media-csi.c > +++ b/drivers/staging/media/imx/imx-media-csi.c > @@ -341,7 +341,7 @@ static int csi_idmac_setup_channel(struct csi_priv > *priv) > case V4L2_PIX_FMT_SGBRG8: > case V4L2_PIX_FMT_SGRBG8: > case V4L2_PIX_FMT_SRGGB8: > - burst_size = 8; > + burst_size = 16; > passthrough = true; > passthrough_bits = 8; > break; regards Philipp ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] imx-csi: fix burst size
On Fri, 2017-09-29 at 22:41 +0100, Russell King wrote: > Setting a burst size of "8" doesn't work for IMX219 with 8-bit bayer, > but a burst size of "16" does. Fix this. > > Signed-off-by: Russell King Oh, well, this isn't for me to apply after all. Acked-by: Philipp Zabel regards Philipp > --- > drivers/staging/media/imx/imx-media-csi.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/staging/media/imx/imx-media-csi.c > b/drivers/staging/media/imx/imx-media-csi.c > index 6d856118c223..e27bcdb63973 100644 > --- a/drivers/staging/media/imx/imx-media-csi.c > +++ b/drivers/staging/media/imx/imx-media-csi.c > @@ -341,7 +341,7 @@ static int csi_idmac_setup_channel(struct csi_priv *priv) > case V4L2_PIX_FMT_SGBRG8: > case V4L2_PIX_FMT_SGRBG8: > case V4L2_PIX_FMT_SRGGB8: > - burst_size = 8; > + burst_size = 16; > passthrough = true; > passthrough_bits = 8; > break; ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/7] media: atomisp: fix ident for assert/return
Hi Mauro,
On Tue, 2017-10-31 at 12:04 -0400, Mauro Carvalho Chehab wrote:
> On lots of places, assert/return are starting at the first
> column, causing indentation issues, as complained by spatch:
>
> drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_common/host/irq_private.h:32
> irq_reg_store() warn: inconsistent indenting
>
> Used this small script to fix such occurrences:
>
> for i in $(git grep -l -E "^(assert|return)" drivers/staging/media/); do perl
> -ne 's/^(assert|return)/\t$1/; print $_' <$i >a && mv a $i; done
This also catches labels that start with "return". Adding some
whitespace to the regular expression may avoid these false positives.
> Signed-off-by: Mauro Carvalho Chehab
> ---
> .../pci/atomisp2/css2400/camera/util/src/util.c| 2 +-
> .../hive_isp_css_common/host/event_fifo_private.h | 2 +-
> .../host/fifo_monitor_private.h| 28 +-
> .../css2400/hive_isp_css_common/host/gdc.c | 8 +--
> .../css2400/hive_isp_css_common/host/gp_device.c | 2 +-
> .../hive_isp_css_common/host/gp_device_private.h | 16 +++---
> .../hive_isp_css_common/host/gpio_private.h| 4 +-
> .../hive_isp_css_common/host/hmem_private.h| 4 +-
> .../host/input_formatter_private.h | 16 +++---
> .../hive_isp_css_common/host/input_system.c| 28 +-
> .../host/input_system_private.h| 64
> +++---
> .../css2400/hive_isp_css_common/host/irq.c | 30 +-
> .../css2400/hive_isp_css_common/host/irq_private.h | 12 ++--
> .../css2400/hive_isp_css_common/host/isp.c | 4 +-
> .../css2400/hive_isp_css_common/host/mmu.c | 6 +-
> .../css2400/hive_isp_css_common/host/mmu_private.h | 12 ++--
> .../css2400/hive_isp_css_common/host/sp_private.h | 60 ++--
> .../atomisp/pci/atomisp2/css2400/sh_css_hrt.c | 2 +-
> drivers/staging/media/imx/imx-media-capture.c | 2 +-
[...]
> diff --git a/drivers/staging/media/imx/imx-media-capture.c
> b/drivers/staging/media/imx/imx-media-capture.c
> index ea145bafb880..149f0e1753a1 100644
> --- a/drivers/staging/media/imx/imx-media-capture.c
> +++ b/drivers/staging/media/imx/imx-media-capture.c
> @@ -463,7 +463,7 @@ static int capture_start_streaming(struct vb2_queue *vq,
> unsigned int count)
>
> return 0;
>
> -return_bufs:
> + return_bufs:
> spin_lock_irqsave(&priv->q_lock, flags);
> list_for_each_entry_safe(buf, tmp, &priv->ready_q, list) {
> list_del(&buf->list);
This label should stay at the first column.
regards
Philipp
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 3/8] media: v4l2-async: simplify v4l2_async_subdev structure
On Mon, 2017-12-18 at 17:53 -0200, Mauro Carvalho Chehab wrote:
> The V4L2_ASYNC_MATCH_FWNODE match criteria requires just one
> struct to be filled (struct fwnode_handle). The V4L2_ASYNC_MATCH_DEVNAME
> match criteria requires just a device name.
>
> So, it doesn't make sense to enclose those into structs,
> as the criteria can go directly into the union.
>
> That makes easier to document it, as we don't need to document
> weird senseless structs.
>
> At drivers, this makes even clearer about the match criteria.
>
> Acked-by: Sylwester Nawrocki
> Signed-off-by: Mauro Carvalho Chehab
Thanks, this does improve readability in the drivers. For imx-media,
Acked-by: Philipp Zabel
regards
Philipp
> ---
> drivers/media/platform/am437x/am437x-vpfe.c| 6 +++---
> drivers/media/platform/atmel/atmel-isc.c | 2 +-
> drivers/media/platform/atmel/atmel-isi.c | 2 +-
> drivers/media/platform/davinci/vpif_capture.c | 4 ++--
> drivers/media/platform/exynos4-is/media-dev.c | 4 ++--
> drivers/media/platform/pxa_camera.c| 2 +-
> drivers/media/platform/qcom/camss-8x16/camss.c | 2 +-
> drivers/media/platform/rcar-vin/rcar-core.c| 2 +-
> drivers/media/platform/rcar_drif.c | 4 ++--
> drivers/media/platform/soc_camera/soc_camera.c | 2 +-
> drivers/media/platform/stm32/stm32-dcmi.c | 2 +-
> drivers/media/platform/ti-vpe/cal.c| 2 +-
> drivers/media/platform/xilinx/xilinx-vipp.c| 2 +-
> drivers/media/v4l2-core/v4l2-async.c | 16
> drivers/media/v4l2-core/v4l2-fwnode.c | 10 +-
> drivers/staging/media/imx/imx-media-dev.c | 4 ++--
> include/media/v4l2-async.h | 8 ++--
> 17 files changed, 35 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/media/platform/am437x/am437x-vpfe.c
> b/drivers/media/platform/am437x/am437x-vpfe.c
> index 0997c640191d..601ae6487617 100644
> --- a/drivers/media/platform/am437x/am437x-vpfe.c
> +++ b/drivers/media/platform/am437x/am437x-vpfe.c
> @@ -2304,8 +2304,8 @@ vpfe_async_bound(struct v4l2_async_notifier *notifier,
> vpfe_dbg(1, vpfe, "vpfe_async_bound\n");
>
> for (i = 0; i < ARRAY_SIZE(vpfe->cfg->asd); i++) {
> - if (vpfe->cfg->asd[i]->match.fwnode.fwnode ==
> - asd[i].match.fwnode.fwnode) {
> + if (vpfe->cfg->asd[i]->match.fwnode ==
> + asd[i].match.fwnode) {
> sdinfo = &vpfe->cfg->sub_devs[i];
> vpfe->sd[i] = subdev;
> vpfe->sd[i]->grp_id = sdinfo->grp_id;
> @@ -2510,7 +2510,7 @@ vpfe_get_pdata(struct platform_device *pdev)
> }
>
> pdata->asd[i]->match_type = V4L2_ASYNC_MATCH_FWNODE;
> - pdata->asd[i]->match.fwnode.fwnode = of_fwnode_handle(rem);
> + pdata->asd[i]->match.fwnode = of_fwnode_handle(rem);
> of_node_put(rem);
> }
>
> diff --git a/drivers/media/platform/atmel/atmel-isc.c
> b/drivers/media/platform/atmel/atmel-isc.c
> index 0c2635647f69..34676409ca08 100644
> --- a/drivers/media/platform/atmel/atmel-isc.c
> +++ b/drivers/media/platform/atmel/atmel-isc.c
> @@ -2088,7 +2088,7 @@ static int isc_parse_dt(struct device *dev, struct
> isc_device *isc)
> subdev_entity->pfe_cfg0 |= ISC_PFE_CFG0_PPOL_LOW;
>
> subdev_entity->asd->match_type = V4L2_ASYNC_MATCH_FWNODE;
> - subdev_entity->asd->match.fwnode.fwnode =
> + subdev_entity->asd->match.fwnode =
> of_fwnode_handle(rem);
> list_add_tail(&subdev_entity->list, &isc->subdev_entities);
> }
> diff --git a/drivers/media/platform/atmel/atmel-isi.c
> b/drivers/media/platform/atmel/atmel-isi.c
> index e900995143a3..9958918e2449 100644
> --- a/drivers/media/platform/atmel/atmel-isi.c
> +++ b/drivers/media/platform/atmel/atmel-isi.c
> @@ -1128,7 +1128,7 @@ static int isi_graph_parse(struct atmel_isi *isi,
> struct device_node *node)
> /* Remote node to connect */
> isi->entity.node = remote;
> isi->entity.asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
> - isi->entity.asd.match.fwnode.fwnode = of_fwnode_handle(remote);
> + isi->entity.asd.match.fwnode = of_fwnode_handle(remote);
> return 0;
> }
> }
> diff --git a/drivers/media/platform/davinci/vpif_capture.c
> b/drivers/media/platform/davinci/vpif_capture.c
> index e45916f69def..e1c273c8b9a6 100644
> ---
Re: [PATCH v2 5/8] media: v4l2-mediabus: convert flags to enums and document them
On Tue, 2017-12-19 at 09:18 -0200, Mauro Carvalho Chehab wrote: > There is a mess with media bus flags: there are two sets of > flags, one used by parallel and ITU-R BT.656 outputs, > and another one for CSI2. > > Depending on the type, the same bit has different meanings. > > That's very confusing, and counter-intuitive. So, split them > into two sets of flags, inside an enum. > > This way, it becomes clearer that there are two separate sets > of flags. It also makes easier if CSI1, CSP, CSI3, etc. would > need a different set of flags. > > As a side effect, enums can be documented via kernel-docs, > so there will be an improvement at flags documentation. > > Unfortunately, soc_camera and pxa_camera do a mess with > the flags, using either one set of flags without proper > checks about the type. That could be fixed, but, as both drivers > are obsolete and in the process of cleanings, I opted to just > keep the behavior, using an unsigned int inside those two > drivers. > > Acked-by: Hans Verkuil > Signed-off-by: Mauro Carvalho Chehab For imx-media, Acked-by: Philipp Zabel thanks Philipp ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 21/24] media: imx: Add MIPI CSI-2 Receiver subdev driver
Hi Steve, On Thu, 2017-02-09 at 15:51 -0800, Steve Longerbeam wrote: > > On 02/09/2017 03:49 PM, Steve Longerbeam wrote: > > > > > > On 02/08/2017 03:42 PM, Russell King - ARM Linux wrote: > >> On Wed, Feb 08, 2017 at 03:23:53PM -0800, Steve Longerbeam wrote: > >>>> Actually, this exact function already exists as > >>>> dw_mipi_dsi_phy_write in > >>>> drivers/gpu/drm/rockchip/dw-mipi-dsi.c, and it looks like the D-PHY > >>>> register 0x44 might contain a field called HSFREQRANGE_SEL. > >>> Thanks for pointing out drivers/gpu/drm/rockchip/dw-mipi-dsi.c. > >>> It's clear from that driver that there probably needs to be a fuller > >>> treatment of the D-PHY programming here, but I don't know where > >>> to find the MIPI CSI-2 D-PHY documentation for the i.MX6. The code > >>> in imxcsi2_reset() was also pulled from FSL, and that's all I really > >>> have > >>> to go on for the D-PHY programming. I assume the D-PHY is also a > >>> Synopsys core, like the host controller, but the i.MX6 manual doesn't > >>> cover it. > >> Why exactly? What problems are you seeing that would necessitate a > >> more detailed programming of the D-PHY? From my testing, I can wind > >> a 2-lane MIPI bus on iMX6D from 912Mbps per lane down to (eg) 308Mbps > >> per lane with your existing code without any issues. > > > > That's good to hear. > > > > Just from my experience with struggles to get the CSI2 receiver > > up and running with an active clock lane, and my suspicions that > > some of that could be due to my lack of understanding of the D-PHY > > programming. > > But I should add that after a re-org of the sequence, it looks more stable > now. Tested on both the SabreSD and SabreLite with the OV5640. It seems the OV5640 driver never puts its the CSI-2 lanes into stop state while not streaming. With the recent s_stream reordering, streaming from TC358743 does not work anymore, since imx6-mipi-csi2 s_stream is called before tc358743 s_stream, while all lanes are still in stop state. Then it waits for the clock lane to become active and fails. I have applied the following patch to revert the reordering locally to get it to work again. The initialization order, as Russell pointed out, should be: 1. reset the D-PHY. 2. place MIPI sensor in LP-11 state 3. perform D-PHY initialisation 4. configure CSI2 lanes and de-assert resets and shutdown signals So csi2_s_stream should wait for stop state on all lanes (the result of 2.) before dphy_init (3.), not wait for active clock afterwards. That should happen only after sensor_s_stream was called. So unless we are allowed to reorder steps 1. and 2., we might need the prepare_stream callback after all. More specifically, the chapter 40.3.1 "Startup Sequence" in the i.MX6DQ reference manual states: 1. Deassert presetn signal (global reset) - This is probably the APB global reset, so not something we have to care about. 2. Configure MIPI Camera Sensor to put all Tx lanes in PL-11 state 3. D-PHY initialization (write 0x14 to address 0x44) 4. CSI2 Controller programming - Set N_LANES - Deassert PHY_SHUTDOWNZ - Deassert PHY_RSTZ - Deassert CSI2_RESETN 5. Read the PHY status register (PHY_STATE) to confirm that all data and clock lanes of the D-PHY are in Stop State 6. Configure the MIPI Camera Sensor to start transmitting a clock on the D-PHY clock lane 7. CSI2 Controller programming - Read the PHY status register (PHY_STATE) to confirm that the D-PHY is receiving a clock on the D-PHY clock lane 2. can be done in sensor s_power (which tc358743 currently does) only if the sensor can still be configured in step 6. Also, 3./4./5. are csi2 code, 6. is sensor code, and 7. is csi2 code again. For reliable startup we need csi2 receiver code to be called on both sides of the sensor enabling its clock lane. --8<-- >From f12758caa3e1d05980cd2ac07587d125449fc860 Mon Sep 17 00:00:00 2001 From: Philipp Zabel Date: Mon, 13 Feb 2017 09:28:27 +0100 Subject: [PATCH] media: imx: revert streamon sequence change Without this patch, starting streaming from a TC358743 MIPI CSI-2 source fails with the following error messages: imx6-mipi-csi2: clock lane timeout, phy_state = 0x06f0 ipu1_csi0: pipeline_set_stream failed with -110 The phy state above has the stopstateclk, rxulpsclknot, and stopstatedata[3:0] bits set: at this point all lanes are in stop state, no lane is in ultra low power state or active. This is no suprise, since tc358743 s_stream(sd, 1) has not been called due to the recently changed ordering. The imx6-mipi-csi2 s_stream does wait for the clock lane to become active, so csi2_s_stream must happen after tc
Re: [PATCH v3 21/24] media: imx: Add MIPI CSI-2 Receiver subdev driver
Hi Steve,
On Mon, 2017-02-13 at 15:20 -0800, Steve Longerbeam wrote:
[...]
> > It seems the OV5640 driver never puts its the CSI-2 lanes into stop
> > state while not streaming.
>
> Yes I found that as well.
>
> But good news, I finally managed to coax the OV5640's clock lane
> into LP-11 state! It was accomplished by setting bit 5 in OV5640 register
> 0x4800. The datasheet describes this bit as "Gate clock lane when no
> packet to transmit". But I may have also got this to work with the
> additional
> write 1 to bits 4-6 in register 0x3019 ("MIPI CLK/data lane state in sleep
> mode" - setting 1 to these bits selects LP-11 for sleep mode).
>
> So I am now finally able to call csi2_dphy_wait_stopstate() in
> csi2_s_stream(ON).
That's good news.
> So for the TC35874, you shouldn't see a timeout in csi2_s_stream(ON)
> any longer.
>
> I have updated both ov5640.c and imx6-mipi-csi2.c in the wip branch.
> Can you try again? I have not applied your patch below, because I
> don't think that is needed anymore.
Thanks, I'll test tomorrow.
> And speaking of the TC35874, I received this module for the SabreLite
> from Boundary Devices (thanks BD!). So I can finally help you with
> debug/test. Are there any patches you need to send to me (against
> wip branch) for this support, or can I use what exists now in media
> tree? Also any scripts or help with running.
That's even better news. I have pushed my my wip branch, which contains
some colorspace work and experiments to pass through query/g_/s_std
subdev calls so bypassing the pipeline can be avoided. Also, there's the
Nitrogen6X device tree that I've been using to test:
https://git.pengutronix.de/git/pza/linux imx-media-staging-md-wip
> > With the recent s_stream reordering,
> > streaming from TC358743 does not work anymore, since imx6-mipi-csi2
> > s_stream is called before tc358743 s_stream, while all lanes are still
> > in stop state. Then it waits for the clock lane to become active and
> > fails. I have applied the following patch to revert the reordering
> > locally to get it to work again.
> >
> > The initialization order, as Russell pointed out, should be:
> >
> > 1. reset the D-PHY.
> > 2. place MIPI sensor in LP-11 state
> > 3. perform D-PHY initialisation
> > 4. configure CSI2 lanes and de-assert resets and shutdown signals
> >
> > So csi2_s_stream should wait for stop state on all lanes (the result of
> > 2.) before dphy_init (3.), not wait for active clock afterwards. That
> > should happen only after sensor_s_stream was called. So unless we are
> > allowed to reorder steps 1. and 2., we might need the prepare_stream
> > callback after all.
>
> Agreed!
>
> See my new FIXME comment in imx6-mipi-csi2.c.
Looks good. I wonder if enabling the phy clock isn't part of step 3.
though.
> I agree we might need a new subdev op .prepare_stream(). This
> op would be implemented by imx6-mipi-csi2.c, and would carry
> out steps 3, 4, 5 (instead of currently by csi2_s_stream()). Then
> step 6 would finally become available as csi2_s_stream().
>
> And then we must re-order stream on to start sensor first, then
> csi2, as in your patch below.
regards
Philipp
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 00/24] i.MX Media Driver
Hi Steve,
On Tue, 2017-02-14 at 18:27 -0800, Steve Longerbeam wrote:
[...]
> >
> > # Provide an EDID to the HDMI source
> > v4l2-ctl -d /dev/v4l-subdev2 --set-edid=file=edid-1080p.hex
>
> I can probably generate this Intel hex file myself from sysfs
> edid outputs, but for convenience do you mind sending me this
> file? I have a 1080p HDMI source I can plug into the tc358743.
I copied the EDID off of some random 1080p HDMI monitor,
probably using something like:
xxd -g1 /sys/class/drm/card0-HDMI-A-1/edid | cut -c 9-56
--8<--
00 ff ff ff ff ff ff 00 09 d1 89 78 45 54 00 00
2a 14 01 03 80 35 1e 78 2e b8 45 a1 59 55 9f 28
0d 50 54 a5 6b 80 81 c0 81 00 81 80 a9 c0 b3 00
d1 c0 01 01 01 01 02 3a 80 18 71 38 2d 40 58 2c
45 00 13 2a 21 00 00 1e 00 00 00 ff 00 4e 41 41
30 36 32 39 36 53 4c 30 0a 20 00 00 00 fd 00 32
4c 1e 53 11 00 0a 20 20 20 20 20 20 00 00 00 fc
00 42 65 6e 51 20 47 4c 32 34 34 30 48 0a 01 18
02 03 22 f1 4f 90 05 04 03 02 01 11 12 13 14 06
07 15 16 1f 23 09 07 07 65 03 0c 00 10 00 83 01
00 00 02 3a 80 18 71 38 2d 40 58 2c 45 00 13 2a
21 00 00 1f 01 1d 80 18 71 1c 16 20 58 2c 25 00
13 2a 21 00 00 9f 01 1d 00 72 51 d0 1e 20 6e 28
55 00 13 2a 21 00 00 1e 8c 0a d0 8a 20 e0 2d 10
10 3e 96 00 13 2a 21 00 00 18 00 00 00 00 00 00
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 eb
-->8--
> The other problem here is that my version of v4l2-ctl, built from
> master branch of g...@github.com:gjasny/v4l-utils.git, does not
> support taking a subdev node:
>
> root@mx6q:~# v4l2-ctl -d /dev/v4l-subdev15 --get-edid=format=hex
> VIDIOC_QUERYCAP: failed: Inappropriate ioctl for device
> /dev/v4l-subdev15: not a v4l2 node
>
> Is this something you added yourself, or where can I find this version
> of v4l2-ctrl?
Ah right, I still have no proper fix for that. v4l-ctl bails out if it
can't VIDIOC_QUERYCAP, which is an ioctl not supported on subdevices.
I have just patched it out locally:
--8<--
diff --git a/utils/v4l2-ctl/v4l2-ctl.cpp b/utils/v4l2-ctl/v4l2-ctl.cpp
index 886a91d093ae..fa15a49375ae 100644
--- a/utils/v4l2-ctl/v4l2-ctl.cpp
+++ b/utils/v4l2-ctl/v4l2-ctl.cpp
@@ -1214,10 +1214,7 @@ int main(int argc, char **argv)
}
verbose = options[OptVerbose];
- if (doioctl(fd, VIDIOC_QUERYCAP, &vcap)) {
- fprintf(stderr, "%s: not a v4l2 node\n", device);
- exit(1);
- }
+ doioctl(fd, VIDIOC_QUERYCAP, &vcap);
capabilities = vcap.capabilities;
if (capabilities & V4L2_CAP_DEVICE_CAPS)
capabilities = vcap.device_caps;
-->8--
Note that setting the EDID is not necessary if you can force the mode on
your HDMI source.
regards
Philipp
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 30/36] media: imx: update capture dev format on IDMAC output pad set_fmt
On Wed, 2017-02-15 at 18:19 -0800, Steve Longerbeam wrote:
> When configuring the IDMAC output pad formats (in ipu_csi,
> ipu_ic_prpenc, and ipu_ic_prpvf subdevs), the attached capture
> device format must also be updated.
>
> Signed-off-by: Steve Longerbeam
> Suggested-by: Philipp Zabel
> ---
> drivers/staging/media/imx/imx-ic-prpencvf.c | 9 +
> drivers/staging/media/imx/imx-media-csi.c | 9 +
> 2 files changed, 18 insertions(+)
>
> diff --git a/drivers/staging/media/imx/imx-ic-prpencvf.c
> b/drivers/staging/media/imx/imx-ic-prpencvf.c
> index 2be8845..6e45975 100644
> --- a/drivers/staging/media/imx/imx-ic-prpencvf.c
> +++ b/drivers/staging/media/imx/imx-ic-prpencvf.c
> @@ -739,6 +739,7 @@ static int prp_set_fmt(struct v4l2_subdev *sd,
> struct v4l2_subdev_format *sdformat)
> {
> struct prp_priv *priv = sd_to_priv(sd);
> + struct imx_media_video_dev *vdev = priv->vdev;
> const struct imx_media_pixfmt *cc;
> struct v4l2_mbus_framefmt *infmt;
> u32 code;
> @@ -800,6 +801,14 @@ static int prp_set_fmt(struct v4l2_subdev *sd,
> } else {
> priv->format_mbus[sdformat->pad] = sdformat->format;
> priv->cc[sdformat->pad] = cc;
> + if (sdformat->pad == PRPENCVF_SRC_PAD) {
> + /*
> + * update the capture device format if this is
> + * the IDMAC output pad
> + */
> + imx_media_mbus_fmt_to_pix_fmt(&vdev->fmt.fmt.pix,
> + &sdformat->format, cc);
> + }
This is replaced again by patch 36. These should probably be squashed
together.
regards
Philipp
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 36/36] media: imx: propagate sink pad formats to source pads
On Wed, 2017-02-15 at 18:19 -0800, Steve Longerbeam wrote:
[...]
> diff --git a/drivers/staging/media/imx/imx-ic-prpencvf.c
> b/drivers/staging/media/imx/imx-ic-prpencvf.c
> index dd9d499..c43f85f 100644
> --- a/drivers/staging/media/imx/imx-ic-prpencvf.c
> +++ b/drivers/staging/media/imx/imx-ic-prpencvf.c
> @@ -806,16 +806,22 @@ static int prp_set_fmt(struct v4l2_subdev *sd,
> if (sdformat->which == V4L2_SUBDEV_FORMAT_TRY) {
> cfg->try_fmt = sdformat->format;
> } else {
> - priv->format_mbus[sdformat->pad] = sdformat->format;
> + struct v4l2_mbus_framefmt *f =
> + &priv->format_mbus[sdformat->pad];
> + struct v4l2_mbus_framefmt *outf =
> + &priv->format_mbus[PRPENCVF_SRC_PAD];
> +
> + *f = sdformat->format;
> priv->cc[sdformat->pad] = cc;
> - if (sdformat->pad == PRPENCVF_SRC_PAD) {
> - /*
> - * update the capture device format if this is
> - * the IDMAC output pad
> - */
> - imx_media_mbus_fmt_to_pix_fmt(&vdev->fmt.fmt.pix,
> - &sdformat->format, cc);
> +
> + /* propagate format to source pad */
> + if (sdformat->pad == PRPENCVF_SINK_PAD) {
> + outf->width = f->width;
> + outf->height = f->height;
What about media bus format, field, and colorimetry?
> }
> +
> + /* update the capture device format from output pad */
> + imx_media_mbus_fmt_to_pix_fmt(&vdev->fmt.fmt.pix, outf, cc);
> }
>
> return 0;
> diff --git a/drivers/staging/media/imx/imx-media-csi.c
> b/drivers/staging/media/imx/imx-media-csi.c
> index 3e6b607..9d9ec03 100644
> --- a/drivers/staging/media/imx/imx-media-csi.c
> +++ b/drivers/staging/media/imx/imx-media-csi.c
> @@ -1161,19 +1161,27 @@ static int csi_set_fmt(struct v4l2_subdev *sd,
> if (sdformat->which == V4L2_SUBDEV_FORMAT_TRY) {
> cfg->try_fmt = sdformat->format;
> } else {
> + struct v4l2_mbus_framefmt *f_direct, *f_idmac;
> +
> priv->format_mbus[sdformat->pad] = sdformat->format;
> priv->cc[sdformat->pad] = cc;
> - /* Reset the crop window if this is the input pad */
> - if (sdformat->pad == CSI_SINK_PAD)
> +
> + f_direct = &priv->format_mbus[CSI_SRC_PAD_DIRECT];
> + f_idmac = &priv->format_mbus[CSI_SRC_PAD_IDMAC];
> +
> + if (sdformat->pad == CSI_SINK_PAD) {
> + /* reset the crop window */
> priv->crop = crop;
> - else if (sdformat->pad == CSI_SRC_PAD_IDMAC) {
> - /*
> - * update the capture device format if this is
> - * the IDMAC output pad
> - */
> - imx_media_mbus_fmt_to_pix_fmt(&vdev->fmt.fmt.pix,
> - &sdformat->format, cc);
> +
> + /* propagate format to source pads */
> + f_direct->width = crop.width;
> + f_direct->height = crop.height;
> + f_idmac->width = crop.width;
> + f_idmac->height = crop.height;
This is missing also media bus format, field and colorimetry
propagation.
> }
> +
> + /* update the capture device format from IDMAC output pad */
> + imx_media_mbus_fmt_to_pix_fmt(&vdev->fmt.fmt.pix, f_idmac, cc);
> }
>
> return 0;
> diff --git a/drivers/staging/media/imx/imx-media-vdic.c
> b/drivers/staging/media/imx/imx-media-vdic.c
> index 61e6017..55fb522 100644
> --- a/drivers/staging/media/imx/imx-media-vdic.c
> +++ b/drivers/staging/media/imx/imx-media-vdic.c
> @@ -649,8 +649,21 @@ static int vdic_set_fmt(struct v4l2_subdev *sd,
> if (sdformat->which == V4L2_SUBDEV_FORMAT_TRY) {
> cfg->try_fmt = sdformat->format;
> } else {
> - priv->format_mbus[sdformat->pad] = sdformat->format;
> + struct v4l2_mbus_framefmt *f =
> + &priv->format_mbus[sdformat->pad];
> + struct v4l2_mbus_framefmt *outf =
> + &priv->format_mbus[VDIC_SRC_PAD_DIRECT];
> +
> + *f = sdformat->format;
> priv->cc[sdformat->pad] = cc;
> +
> + /* propagate format to source pad */
> + if (sdformat->pad == VDIC_SINK_PAD_DIRECT ||
> + sdformat->pad == VDIC_SINK_PAD_IDMAC) {
> + outf->width = f->width;
> + outf->height = f->height;
> + outf->field = V4L2_FIELD_NONE;
This is missing colorimetry, too.
regards
Philipp
___
devel mailing list
de...@linuxdriverproj
Re: [PATCH v4 33/36] media: imx: redo pixel format enumeration and negotiation
On Wed, 2017-02-15 at 18:19 -0800, Steve Longerbeam wrote: > The previous API and negotiation of mbus codes and pixel formats > was broken, and has been completely redone. > > The negotiation of media bus codes should be as follows: > > CSI: > > sink pad direct src pad IDMAC src pad > - > RGB (any)IPU RGB RGB (any) > YUV (any)IPU YUV YUV (any) > Bayer N/A must be same bayer code as sink The IDMAC src pad should also use the internal 32-bit RGB / YUV format, except if bayer/raw mode is selected, in which case the attached capture video device should only allow a single mode corresponding to the output pad media bus format. regards Philipp ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 17/36] media: Add userspace header file for i.MX
On Wed, 2017-02-15 at 18:19 -0800, Steve Longerbeam wrote:
> This adds a header file for use by userspace programs wanting to interact
> with the i.MX media driver. It defines custom v4l2 controls and events
> generated by the i.MX v4l2 subdevices.
>
> Signed-off-by: Steve Longerbeam
> ---
> include/uapi/media/Kbuild | 1 +
> include/uapi/media/imx.h | 29 +
> 2 files changed, 30 insertions(+)
> create mode 100644 include/uapi/media/imx.h
>
> diff --git a/include/uapi/media/Kbuild b/include/uapi/media/Kbuild
> index aafaa5a..fa78958 100644
> --- a/include/uapi/media/Kbuild
> +++ b/include/uapi/media/Kbuild
> @@ -1 +1,2 @@
> # UAPI Header export list
> +header-y += imx.h
> diff --git a/include/uapi/media/imx.h b/include/uapi/media/imx.h
> new file mode 100644
> index 000..1fdd1c1
> --- /dev/null
> +++ b/include/uapi/media/imx.h
> @@ -0,0 +1,29 @@
> +/*
> + * Copyright (c) 2014-2015 Mentor Graphics Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by the
> + * Free Software Foundation; either version 2 of the
> + * License, or (at your option) any later version
> + */
> +
> +#ifndef __UAPI_MEDIA_IMX_H__
> +#define __UAPI_MEDIA_IMX_H__
> +
> +/*
> + * events from the subdevs
> + */
> +#define V4L2_EVENT_IMX_CLASS V4L2_EVENT_PRIVATE_START
> +#define V4L2_EVENT_IMX_NFB4EOF(V4L2_EVENT_IMX_CLASS + 1)
> +#define V4L2_EVENT_IMX_FRAME_INTERVAL (V4L2_EVENT_IMX_CLASS + 2)
These events are still i.MX specific. I think they shouldn't be.
> +enum imx_ctrl_id {
> + V4L2_CID_IMX_MOTION = (V4L2_CID_USER_IMX_BASE + 0),
> + V4L2_CID_IMX_FIM_ENABLE,
> + V4L2_CID_IMX_FIM_NUM,
> + V4L2_CID_IMX_FIM_TOLERANCE_MIN,
> + V4L2_CID_IMX_FIM_TOLERANCE_MAX,
> + V4L2_CID_IMX_FIM_NUM_SKIP,
> +};
> +
> +#endif
regards
Philipp
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 01/36] [media] dt-bindings: Add bindings for i.MX media driver
On Wed, 2017-02-15 at 18:19 -0800, Steve Longerbeam wrote:
> Add bindings documentation for the i.MX media driver.
>
> Signed-off-by: Steve Longerbeam
> ---
> Documentation/devicetree/bindings/media/imx.txt | 66
> +
> 1 file changed, 66 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/media/imx.txt
>
> diff --git a/Documentation/devicetree/bindings/media/imx.txt
> b/Documentation/devicetree/bindings/media/imx.txt
> new file mode 100644
> index 000..fd5af50
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/imx.txt
> @@ -0,0 +1,66 @@
> +Freescale i.MX Media Video Device
> +=
> +
> +Video Media Controller node
> +---
> +
> +This is the media controller node for video capture support. It is a
> +virtual device that lists the camera serial interface nodes that the
> +media device will control.
> +
> +Required properties:
> +- compatible : "fsl,imx-capture-subsystem";
> +- ports : Should contain a list of phandles pointing to camera
> + sensor interface ports of IPU devices
> +
> +example:
> +
> +capture-subsystem {
> + compatible = "fsl,capture-subsystem";
"fsl,imx-capture-subsystem"
regards
Philipp
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 18/36] media: Add i.MX media core driver
stream restart which are not > +attributed to adv718x sending a corrupt field, so this is used to > +skip those frames to prevent unnecessary restarts. > + > + > +SabreSD with MIPI CSI-2 OV5640 > +-- > + > +Similarly to SabreLite, the SabreSD supports a parallel interface > +OV5642 module on IPU1 CSI0, and a MIPI CSI-2 OV5640 module. The OV5642 > +connects to i2c bus 1 and the OV5640 to i2c bus 2. > + > +The device tree for SabreSD includes OF graphs for both the parallel > +OV5642 and the MIPI CSI-2 OV5640, but as of this writing only the MIPI > +CSI-2 OV5640 has been tested, so the OV5642 node is currently disabled. > +The OV5640 module connects to MIPI connector J5 (sorry I don't have the > +compatible module part number or URL). > + > +The following example configures a direct conversion pipeline to capture > +from the OV5640. $sensorfmt can be any format supported by the OV5640. > +$sensordim is the frame dimension part of $sensorfmt (minus the mbus > +pixel code). $outputfmt can be any format supported by the > +ipu1_ic_prpenc entity at its output pad: > + > +.. code-block:: none > + > + # Setup links > + media-ctl -l '"ov5640_mipi 1-003c":0 -> "imx6-mipi-csi2":0[1]' > + media-ctl -l '"imx6-mipi-csi2":2 -> "ipu1_csi1":0[1]' > + media-ctl -l '"ipu1_csi1":1 -> "ipu1_ic_prp":0[1]' > + media-ctl -l '"ipu1_ic_prp":1 -> "ipu1_ic_prpenc":0[1]' > + media-ctl -l '"ipu1_ic_prpenc":1 -> "ipu1_ic_prpenc capture":0[1]' > + # Configure pads > + media-ctl -V "\"ov5640_mipi 1-003c\":0 [fmt:$sensorfmt field:none]" > + media-ctl -V "\"imx6-mipi-csi2\":2 [fmt:$sensorfmt field:none]" > + media-ctl -V "\"ipu1_csi1\":1 [fmt:AYUV32/$sensordim field:none]" > + media-ctl -V "\"ipu1_ic_prp\":1 [fmt:AYUV32/$sensordim field:none]" > + media-ctl -V "\"ipu1_ic_prpenc\":1 [fmt:$outputfmt field:none]" > + > +Streaming can then begin on "ipu1_ic_prpenc capture" node. > + > + > + > +Known Issues > + > + > +1. When using 90 or 270 degree rotation control at capture resolutions > + near the IC resizer limit of 1024x1024, and combined with planar > + pixel formats (YUV420, YUV422p), frame capture will often fail with > + no end-of-frame interrupts from the IDMAC channel. To work around > + this, use lower resolution and/or packed formats (YUYV, RGB3, etc.) > + when 90 or 270 rotations are needed. > + > + > +File list > +- > + > +drivers/staging/media/imx/ > +include/media/imx.h > +include/uapi/media/imx.h > + > +References > +-- > + > +[1] "i.MX 6Dual/6Quad Applications Processor Reference Manual" > +[2] "i.MX 6Solo/6DualLite Applications Processor Reference Manual" > + > + > +Authors > +--- > +Steve Longerbeam > +Philipp Zabel > +Russell King - ARM Linux > + > +Copyright (C) 2012-2017 Mentor Graphics Inc. > diff --git a/drivers/staging/media/Kconfig b/drivers/staging/media/Kconfig > index ffb8fa7..05b55a8 100644 > --- a/drivers/staging/media/Kconfig > +++ b/drivers/staging/media/Kconfig > @@ -25,6 +25,8 @@ source "drivers/staging/media/cxd2099/Kconfig" > > source "drivers/staging/media/davinci_vpfe/Kconfig" > > +source "drivers/staging/media/imx/Kconfig" > + > source "drivers/staging/media/omap4iss/Kconfig" > > source "drivers/staging/media/s5p-cec/Kconfig" > diff --git a/drivers/staging/media/Makefile b/drivers/staging/media/Makefile > index a28e82c..6f50ddd 100644 > --- a/drivers/staging/media/Makefile > +++ b/drivers/staging/media/Makefile > @@ -1,6 +1,7 @@ > obj-$(CONFIG_I2C_BCM2048)+= bcm2048/ > obj-$(CONFIG_VIDEO_SAMSUNG_S5P_CEC) += s5p-cec/ > obj-$(CONFIG_DVB_CXD2099)+= cxd2099/ > +obj-$(CONFIG_VIDEO_IMX_MEDIA)+= imx/ > obj-$(CONFIG_LIRC_STAGING) += lirc/ > obj-$(CONFIG_VIDEO_DM365_VPFE) += davinci_vpfe/ > obj-$(CONFIG_VIDEO_OMAP4)+= omap4iss/ > diff --git a/drivers/staging/media/imx/Kconfig > b/drivers/staging/media/imx/Kconfig > new file mode 100644 > index 000..722ed55 > --- /dev/null > +++ b/drivers/staging/media/imx/Kconfig > @@ -0,0 +1,7 @@ > +config VIDEO_IMX_MEDIA > + tristate "i.MX5/6 V4L2 media core driver" > + depends on MEDIA_CONTROLLER && VIDEO_V4L2 && ARCH_MXC && IMX_IPUV3_CORE > + ---help--- > + Say yes here to enable support for video4linux media controlle
Re: [PATCH v4 18/36] media: Add i.MX media core driver
On Thu, 2017-02-16 at 17:33 -0800, Steve Longerbeam wrote:
>
> On 02/16/2017 05:02 AM, Philipp Zabel wrote:
> > On Wed, 2017-02-15 at 18:19 -0800, Steve Longerbeam wrote:
>
> >> +
> >> +- Clean up and move the ov5642 subdev driver to drivers/media/i2c, and
> >> + create the binding docs for it.
> >
> > This is done already, right?
>
>
> I cleaned up ov5640 and moved it to drivers/media/i2c with binding docs,
> but not the ov5642 yet.
Ok, thanks.
> >> +- The Frame Interval Monitor could be exported to v4l2-core for
> >> + general use.
> >> +
> >> +- The subdev that is the original source of video data (referred to as
> >> + the "sensor" in the code), is called from various subdevs in the
> >> + pipeline in order to set/query the video standard ({g|s|enum}_std)
> >> + and to get/set the original frame interval from the capture interface
> >> + ([gs]_parm). Instead, the entities that need this info should call its
> >> + direct neighbor, and the neighbor should propagate the call to its
> >> + neighbor in turn if necessary.
> >
> > Especially the [gs]_parm fix is necessary to present userspace with the
> > correct frame interval in case of frame skipping in the CSI.
>
>
> Right, understood. I've added this to list of fixes for version 5.
>
> What a pain though! It means propagating every call to g_frame_interval
> upstream until a subdev "that cares" returns ret == 0 or
> ret != -ENOIOCTLCMD. And that goes for any other chained subdev call
> as well.
Not at all. Since the frame interval is a property of the pad, that had
to be propagated downstream by media-ctl along with media bus format,
frame size, and colorimetry earlier.
regards
Philipp
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 00/36] i.MX Media Driver
On Thu, 2017-02-16 at 22:57 +, Russell King - ARM Linux wrote: > On Thu, Feb 16, 2017 at 02:27:41PM -0800, Steve Longerbeam wrote: > > > > > > On 02/16/2017 02:20 PM, Russell King - ARM Linux wrote: > > >On Wed, Feb 15, 2017 at 06:19:02PM -0800, Steve Longerbeam wrote: > > >>In version 4: > > > > > >With this version, I get: > > > > > >[28762.892053] imx6-mipi-csi2: LP-11 timeout, phy_state = 0x > > >[28762.899409] ipu1_csi0: pipeline_set_stream failed with -110 > > > > > > > Right, in the imx219, on exit from s_power(), the clock and data lanes > > must be placed in the LP-11 state. This has been done in the ov5640 and > > tc358743 subdevs. > > The only way to do that is to enable streaming from the sensor, wait > an initialisation time, and then disable streaming, and wait for the > current line to finish. There is _no_ other way to get the sensor to > place its clock and data lines into LP-11 state. I thought going through LP-11 is part of the D-PHY transmitter initialization, during the LP->HS wakeup sequence. But then I have no access to MIPI specs. It is unfortunate that the i.MX6 MIPI CSI-2 core needs software assistance here, but could it be possible to trigger that sequence in the sensor and then without waiting switching to polling for LP-11 state in the i.MX6 MIPI CSI-2 receiver? > For that to happen, we need to program the sensor a bit more than we > currently do at power on (to a minimal resolution, and setting up the > PLLs), and introduce another 4ms on top of the 8ms or so that the > runtime resume function already takes. > > Looking at the SMIA driver, things are worse, and I suspect that it also > will not work with the current setup - the SMIA spec shows that the CSI > clock and data lines are tristated while the sensor is not streaming, > which means they aren't held at a guaranteed LP-11 state, even if that > driver momentarily enabled streaming. Hence, Freescale's (or is it > Synopsis') requirement may actually be difficult to satisfy. > > However, I regard runtime PM broken with the current imx-capture setup. > At the moment, power is controlled at the sensor by whether the media > links are enabled. So, if you have an enabled link coming off the > sensor, the sensor will be powered up, whether you're using it or not. > > Given that the number of applications out there that know about the > media subdevs is really quite small, this combination makes having > runtime PM in sensor devices completely pointless - they can't sleep > as long as they have an enabled link, which could be persistent over > many days or weeks. regards Philipp ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 23/36] media: imx: Add MIPI CSI-2 Receiver subdev driver
On Wed, 2017-02-15 at 18:19 -0800, Steve Longerbeam wrote:
> Adds MIPI CSI-2 Receiver subdev driver. This subdev is required
> for sensors with a MIPI CSI2 interface.
>
> Signed-off-by: Steve Longerbeam
> ---
> drivers/staging/media/imx/Makefile | 1 +
> drivers/staging/media/imx/imx6-mipi-csi2.c | 573
> +
> 2 files changed, 574 insertions(+)
> create mode 100644 drivers/staging/media/imx/imx6-mipi-csi2.c
>
> diff --git a/drivers/staging/media/imx/Makefile
> b/drivers/staging/media/imx/Makefile
> index 878a126..3569625 100644
> --- a/drivers/staging/media/imx/Makefile
> +++ b/drivers/staging/media/imx/Makefile
> @@ -9,3 +9,4 @@ obj-$(CONFIG_VIDEO_IMX_MEDIA) += imx-media-vdic.o
> obj-$(CONFIG_VIDEO_IMX_MEDIA) += imx-media-ic.o
>
> obj-$(CONFIG_VIDEO_IMX_CSI) += imx-media-csi.o
> +obj-$(CONFIG_VIDEO_IMX_CSI) += imx6-mipi-csi2.o
> diff --git a/drivers/staging/media/imx/imx6-mipi-csi2.c
> b/drivers/staging/media/imx/imx6-mipi-csi2.c
> new file mode 100644
> index 000..23dca80
> --- /dev/null
> +++ b/drivers/staging/media/imx/imx6-mipi-csi2.c
> @@ -0,0 +1,573 @@
> +/*
> + * MIPI CSI-2 Receiver Subdev for Freescale i.MX6 SOC.
> + *
> + * Copyright (c) 2012-2017 Mentor Graphics Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include "imx-media.h"
> +
> +/*
> + * there must be 5 pads: 1 input pad from sensor, and
> + * the 4 virtual channel output pads
> + */
> +#define CSI2_SINK_PAD 0
> +#define CSI2_NUM_SINK_PADS 1
> +#define CSI2_NUM_SRC_PADS 4
> +#define CSI2_NUM_PADS 5
> +
> +struct csi2_dev {
> + struct device *dev;
> + struct v4l2_subdev sd;
> + struct media_pad pad[CSI2_NUM_PADS];
> + struct v4l2_mbus_framefmt format_mbus;
> + struct clk *dphy_clk;
> + struct clk *cfg_clk;
> + struct clk *pix_clk; /* what is this? */
> + void __iomem *base;
> + struct v4l2_of_bus_mipi_csi2 bus;
> + boolon;
> + boolstream_on;
> + boolsrc_linked;
> + boolsink_linked[CSI2_NUM_SRC_PADS];
> +};
> +
> +#define DEVICE_NAME "imx6-mipi-csi2"
> +
> +/* Register offsets */
> +#define CSI2_VERSION0x000
> +#define CSI2_N_LANES0x004
> +#define CSI2_PHY_SHUTDOWNZ 0x008
> +#define CSI2_DPHY_RSTZ 0x00c
> +#define CSI2_RESETN 0x010
> +#define CSI2_PHY_STATE 0x014
> +#define PHY_STOPSTATEDATA_BIT 4
> +#define PHY_STOPSTATEDATA(n)BIT(PHY_STOPSTATEDATA_BIT + (n))
> +#define PHY_RXCLKACTIVEHS BIT(8)
> +#define PHY_RXULPSCLKNOTBIT(9)
> +#define PHY_STOPSTATECLKBIT(10)
> +#define CSI2_DATA_IDS_1 0x018
> +#define CSI2_DATA_IDS_2 0x01c
> +#define CSI2_ERR1 0x020
> +#define CSI2_ERR2 0x024
> +#define CSI2_MSK1 0x028
> +#define CSI2_MSK2 0x02c
> +#define CSI2_PHY_TST_CTRL0 0x030
> +#define PHY_TESTCLR BIT(0)
> +#define PHY_TESTCLK BIT(1)
> +#define CSI2_PHY_TST_CTRL1 0x034
> +#define PHY_TESTEN BIT(16)
> +#define CSI2_SFT_RESET 0xf00
> +
> +static inline struct csi2_dev *sd_to_dev(struct v4l2_subdev *sdev)
> +{
> + return container_of(sdev, struct csi2_dev, sd);
> +}
> +
> +static void csi2_enable(struct csi2_dev *csi2, bool enable)
> +{
> + if (enable) {
> + writel(0x1, csi2->base + CSI2_PHY_SHUTDOWNZ);
> + writel(0x1, csi2->base + CSI2_DPHY_RSTZ);
> + writel(0x1, csi2->base + CSI2_RESETN);
> + } else {
> + writel(0x0, csi2->base + CSI2_PHY_SHUTDOWNZ);
> + writel(0x0, csi2->base + CSI2_DPHY_RSTZ);
> + writel(0x0, csi2->base + CSI2_RESETN);
> + }
> +}
> +
> +static void csi2_set_lanes(struct csi2_dev *csi2)
> +{
> + int lanes = csi2->bus.num_data_lanes;
> +
> + writel(lanes - 1, csi2->base + CSI2_N_LANES);
> +}
> +
> +static void dw_mipi_csi2_phy_write(struct csi2_dev *csi2,
> +u32 test_code, u32 test_data)
> +{
> + /* Clear PHY test interface */
> + writel(PHY_TESTCLR, csi2->base + CSI2_PHY_TST_CTRL0);
> + writel(0x0, csi2->base + CSI2_PHY_TST_CTRL1);
> + writel(0x0, csi2->base + CSI2_PHY_TST_CTRL0);
> +
> + /* Raise test interface strobe signal */
> + writel(PHY_TESTCLK, csi2->base + CSI2_PHY_TST_CTRL0);
> +
> + /* Configure address write on falling edge and lower strobe signal */
> + writel(PHY_TESTEN | test_code, csi2->base + CSI2_PHY_TST_CTRL1);
> + writel(0x
Re: [PATCH v4 00/36] i.MX Media Driver
On Fri, 2017-02-17 at 10:56 +, Russell King - ARM Linux wrote: > On Fri, Feb 17, 2017 at 11:39:11AM +0100, Philipp Zabel wrote: > > On Thu, 2017-02-16 at 22:57 +, Russell King - ARM Linux wrote: > > > On Thu, Feb 16, 2017 at 02:27:41PM -0800, Steve Longerbeam wrote: > > > > > > > > > > > > On 02/16/2017 02:20 PM, Russell King - ARM Linux wrote: > > > > >On Wed, Feb 15, 2017 at 06:19:02PM -0800, Steve Longerbeam wrote: > > > > >>In version 4: > > > > > > > > > >With this version, I get: > > > > > > > > > >[28762.892053] imx6-mipi-csi2: LP-11 timeout, phy_state = 0x > > > > >[28762.899409] ipu1_csi0: pipeline_set_stream failed with -110 > > > > > > > > > > > > > Right, in the imx219, on exit from s_power(), the clock and data lanes > > > > must be placed in the LP-11 state. This has been done in the ov5640 and > > > > tc358743 subdevs. > > > > > > The only way to do that is to enable streaming from the sensor, wait > > > an initialisation time, and then disable streaming, and wait for the > > > current line to finish. There is _no_ other way to get the sensor to > > > place its clock and data lines into LP-11 state. > > > > I thought going through LP-11 is part of the D-PHY transmitter > > initialization, during the LP->HS wakeup sequence. But then I have no > > access to MIPI specs. > > The D-PHY transmitter initialisation *only* happens as part of the > wake-up from standby to streaming mode. That is because Sony expect > that you program the sensor, and then when you switch it to streaming > mode, it computes the D-PHY parameters from the PLL, input clock rate > (you have to tell it the clock rate in 1/256 MHz units), number of > lanes, and other parameters. > > It is possible to program the D-PHY parameters manually, but that > doesn't change the above sequence in any way (it just avoids the > chip computing the values, it doesn't result in any change of > behaviour on the bus.) > > The IMX219 specifications are clear: the clock and data lines are > held low (LP-00 state) after releasing the hardware enable signal. > There's a period of chip initialisation, and then you can access the > I2C bus and configure it. There's a further period of initialisation > where charge pumps are getting to their operating state. Then, you > set the streaming bit, and a load more initialisation happens before > the CSI bus enters LP-11 state and the first frame pops out. When > entering standby, the last frame is completed, and then the CSI bus > enters LP-11 state. How about firing off a thread in imx6-mipi-csi2 prepare_stream that spins on the LP-11 check and then continues with the receiver D-PHY initialization once the condition is met? I think we should have at least 100 us to do this, but maybe the IMX219 can be programmed to stay in LP-11 for a longer time. > SMIA are slightly different - mostly following what I've said above, > but the clock and data lines are tristated after releasing the > xshutdown signal, and they remain tristated until the clock line > starts toggling before the first frame appears. There appears to > be no point that the clock line enters LP-11 state before it starts > toggling. When entering standby, the last frame is completed, and > the CSI bus enters tristate mode (so floating.) There is no way to > get these sensors into LP-11 state. I have no idea what to do about those. regards Philipp ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 23/36] media: imx: Add MIPI CSI-2 Receiver subdev driver
On Fri, 2017-02-17 at 11:06 +, Russell King - ARM Linux wrote:
> On Fri, Feb 17, 2017 at 11:47:59AM +0100, Philipp Zabel wrote:
> > On Wed, 2017-02-15 at 18:19 -0800, Steve Longerbeam wrote:
> > > +static void csi2_dphy_init(struct csi2_dev *csi2)
> > > +{
> > > + /*
> > > + * FIXME: 0x14 is derived from a fixed D-PHY reference
> > > + * clock from the HSI_TX PLL, and a fixed target lane max
> > > + * bandwidth of 300 Mbps. This value should be derived
> >
> > If the table in https://community.nxp.com/docs/DOC-94312 is correct,
> > this should be 850 Mbps. Where does this 300 Mbps value come from?
>
> I thought you had some code to compute the correct value, although
> I guess we've lost the ability to know how fast the sensor is going
> to drive the link.
I had code to calculate the number of needed lanes from the bit rate and
link frequency. I did not actually change the D-PHY register value.
And as you pointed out, calculating the number of lanes is not useful
without input from the sensor driver, as some lane configurations might
not be supported.
> Note that the IMX219 currently drives the data lanes at 912Mbps almost
> exclusively, as I've yet to finish working out how to derive the PLL
> parameters. (I have something that works, but it currently takes on
> the order of 100k iterations to derive the parameters. gcd() doesn't
> help you in this instance.)
The tc358743 also currently only implements a fixed rate (of 594 Mbps).
regards
Philipp
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 00/36] i.MX Media Driver
On Thu, 2017-02-16 at 14:27 -0800, Steve Longerbeam wrote: > > On 02/16/2017 02:20 PM, Russell King - ARM Linux wrote: > > On Wed, Feb 15, 2017 at 06:19:02PM -0800, Steve Longerbeam wrote: > >> In version 4: > > > > With this version, I get: > > > > [28762.892053] imx6-mipi-csi2: LP-11 timeout, phy_state = 0x > > [28762.899409] ipu1_csi0: pipeline_set_stream failed with -110 > > > > Right, in the imx219, on exit from s_power(), the clock and data lanes > must be placed in the LP-11 state. This has been done in the ov5640 and > tc358743 subdevs. > > If we want to bring in the patch that adds a .prepare_stream() op, > the csi-2 bus would need to be placed in LP-11 in that op instead. > > Philipp, should I go ahead and add your .prepare_stream() patch? I think with Russell's explanation of how the imx219 sensor operates, we'll have to do something before calling the sensor s_stream, but right now I'm still unsure what exactly. regards Philipp ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 23/36] media: imx: Add MIPI CSI-2 Receiver subdev driver
On Fri, 2017-02-17 at 11:47 +0100, Philipp Zabel wrote:
> On Wed, 2017-02-15 at 18:19 -0800, Steve Longerbeam wrote:
> > Adds MIPI CSI-2 Receiver subdev driver. This subdev is required
> > for sensors with a MIPI CSI2 interface.
> >
> > Signed-off-by: Steve Longerbeam
> > ---
> > drivers/staging/media/imx/Makefile | 1 +
> > drivers/staging/media/imx/imx6-mipi-csi2.c | 573
> > +
> > 2 files changed, 574 insertions(+)
> > create mode 100644 drivers/staging/media/imx/imx6-mipi-csi2.c
> >
> > diff --git a/drivers/staging/media/imx/Makefile
> > b/drivers/staging/media/imx/Makefile
> > index 878a126..3569625 100644
> > --- a/drivers/staging/media/imx/Makefile
> > +++ b/drivers/staging/media/imx/Makefile
> > @@ -9,3 +9,4 @@ obj-$(CONFIG_VIDEO_IMX_MEDIA) += imx-media-vdic.o
> > obj-$(CONFIG_VIDEO_IMX_MEDIA) += imx-media-ic.o
> >
> > obj-$(CONFIG_VIDEO_IMX_CSI) += imx-media-csi.o
> > +obj-$(CONFIG_VIDEO_IMX_CSI) += imx6-mipi-csi2.o
> > diff --git a/drivers/staging/media/imx/imx6-mipi-csi2.c
> > b/drivers/staging/media/imx/imx6-mipi-csi2.c
> > new file mode 100644
> > index 000..23dca80
> > --- /dev/null
> > +++ b/drivers/staging/media/imx/imx6-mipi-csi2.c
> > @@ -0,0 +1,573 @@
> > +/*
> > + * MIPI CSI-2 Receiver Subdev for Freescale i.MX6 SOC.
> > + *
> > + * Copyright (c) 2012-2017 Mentor Graphics Inc.
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + */
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include "imx-media.h"
> > +
> > +/*
> > + * there must be 5 pads: 1 input pad from sensor, and
> > + * the 4 virtual channel output pads
> > + */
> > +#define CSI2_SINK_PAD 0
> > +#define CSI2_NUM_SINK_PADS 1
> > +#define CSI2_NUM_SRC_PADS 4
> > +#define CSI2_NUM_PADS 5
> > +
> > +struct csi2_dev {
> > + struct device *dev;
> > + struct v4l2_subdev sd;
> > + struct media_pad pad[CSI2_NUM_PADS];
> > + struct v4l2_mbus_framefmt format_mbus;
> > + struct clk *dphy_clk;
> > + struct clk *cfg_clk;
> > + struct clk *pix_clk; /* what is this? */
> > + void __iomem *base;
> > + struct v4l2_of_bus_mipi_csi2 bus;
> > + boolon;
> > + boolstream_on;
> > + boolsrc_linked;
> > + boolsink_linked[CSI2_NUM_SRC_PADS];
> > +};
> > +
> > +#define DEVICE_NAME "imx6-mipi-csi2"
> > +
> > +/* Register offsets */
> > +#define CSI2_VERSION0x000
> > +#define CSI2_N_LANES0x004
> > +#define CSI2_PHY_SHUTDOWNZ 0x008
> > +#define CSI2_DPHY_RSTZ 0x00c
> > +#define CSI2_RESETN 0x010
> > +#define CSI2_PHY_STATE 0x014
> > +#define PHY_STOPSTATEDATA_BIT 4
> > +#define PHY_STOPSTATEDATA(n)BIT(PHY_STOPSTATEDATA_BIT + (n))
> > +#define PHY_RXCLKACTIVEHS BIT(8)
> > +#define PHY_RXULPSCLKNOTBIT(9)
> > +#define PHY_STOPSTATECLKBIT(10)
> > +#define CSI2_DATA_IDS_1 0x018
> > +#define CSI2_DATA_IDS_2 0x01c
> > +#define CSI2_ERR1 0x020
> > +#define CSI2_ERR2 0x024
> > +#define CSI2_MSK1 0x028
> > +#define CSI2_MSK2 0x02c
> > +#define CSI2_PHY_TST_CTRL0 0x030
> > +#define PHY_TESTCLRBIT(0)
> > +#define PHY_TESTCLKBIT(1)
> > +#define CSI2_PHY_TST_CTRL1 0x034
> > +#define PHY_TESTEN BIT(16)
> > +#define CSI2_SFT_RESET 0xf00
> > +
> > +static inline struct csi2_dev *sd_to_dev(struct v4l2_subdev *sdev)
> > +{
> > + return container_of(sdev, struct csi2_dev, sd);
> > +}
> > +
> > +static void csi2_enable(struct csi2_dev *csi2, bool enable)
> > +{
> > + if (enable) {
> > + writel(0x1, csi2->base + CSI2_PHY_SHUTDOWNZ);
> > + writel(0x1, csi2->base + CSI2_DPHY_RSTZ);
> > + writel(0x1, csi2->base + CSI2_RESETN);
> > + } else {
>
Re: [PATCH v4 00/36] i.MX Media Driver
On Fri, 2017-02-17 at 14:22 +0200, Sakari Ailus wrote:
> Hi Philipp, Steve and Russell,
>
> On Fri, Feb 17, 2017 at 12:43:38PM +0100, Philipp Zabel wrote:
> > On Thu, 2017-02-16 at 14:27 -0800, Steve Longerbeam wrote:
> > >
> > > On 02/16/2017 02:20 PM, Russell King - ARM Linux wrote:
> > > > On Wed, Feb 15, 2017 at 06:19:02PM -0800, Steve Longerbeam wrote:
> > > >> In version 4:
> > > >
> > > > With this version, I get:
> > > >
> > > > [28762.892053] imx6-mipi-csi2: LP-11 timeout, phy_state = 0x
> > > > [28762.899409] ipu1_csi0: pipeline_set_stream failed with -110
> > > >
> > >
> > > Right, in the imx219, on exit from s_power(), the clock and data lanes
> > > must be placed in the LP-11 state. This has been done in the ov5640 and
> > > tc358743 subdevs.
> > >
> > > If we want to bring in the patch that adds a .prepare_stream() op,
> > > the csi-2 bus would need to be placed in LP-11 in that op instead.
> > >
> > > Philipp, should I go ahead and add your .prepare_stream() patch?
> >
> > I think with Russell's explanation of how the imx219 sensor operates,
> > we'll have to do something before calling the sensor s_stream, but right
> > now I'm still unsure what exactly.
>
> Indeed there appears to be no other way to achieve the LP-11 state than
> going through the streaming state for this particular sensor, apart from
> starting streaming.
>
> Is there a particular reason why you're waiting for the transmitter to
> transfer to LP-11 state? That appears to be the last step which is done in
> the csi2_s_stream() callback.
>
> What the sensor does next is to start streaming, and the first thing it does
> in that process is to switch to LP-11 state.
>
> Have you tried what happens if you simply drop the LP-11 check? To me that
> would seem the right thing to do.
Removing the wait for LP-11 alone might not be an issue in my case, as
the TC358743 is known to be in stop state all along. So I just have to
make sure that the time between s_stream(csi2) starting the receiver and
s_stream(tc358743) causing LP-11 to be changed to the next state is long
enough for the receiver to detect LP-11 (which I really can't, I just
have to pray I2C transmissions are slow enough).
The problems start if we have to enable the D-PHY and deassert resets
either before the sensor enters LP-11 state or after it already started
streaming, because we don't know when the sensor drives that state on
the bus.
The latter case I is simulated easily by again changing the order so
that the "sensor" (tc358743) is enabled before the CSI-2 receiver D-PHY
initialization. The result is that captures time out, presumably because
the receiver never entered HS mode because it didn't see LP-11. The
PHY_STATE register contains 0x200, meaning RXCLKACTIVEHS (which we
should wait for in step 7.) is never set.
I tried to test the former by instead modifying the tc358743 driver a
bit:
--8<--
diff --git a/drivers/media/i2c/tc358743.c b/drivers/media/i2c/tc358743.c
index 39d4cdd328c0f..43df80903215b 100644
--- a/drivers/media/i2c/tc358743.c
+++ b/drivers/media/i2c/tc358743.c
@@ -1378,8 +1378,6 @@ static int tc358743_s_dv_timings(struct v4l2_subdev *sd,
state->timings = *timings;
enable_stream(sd, false);
- tc358743_set_pll(sd);
- tc358743_set_csi(sd);
return 0;
}
@@ -1469,6 +1467,11 @@ static int tc358743_g_mbus_config(struct v4l2_subdev *sd,
static int tc358743_s_stream(struct v4l2_subdev *sd, int enable)
{
+ if (enable) {
+ tc358743_set_pll(sd);
+ tc358743_set_csi(sd);
+ tc358743_set_csi_color_space(sd);
+ }
enable_stream(sd, enable);
if (!enable) {
/* Put all lanes in PL-11 state (STOPSTATE) */
@@ -1657,9 +1660,6 @@ static int tc358743_set_fmt(struct v4l2_subdev *sd,
state->vout_color_sel = vout_color_sel;
enable_stream(sd, false);
- tc358743_set_pll(sd);
- tc358743_set_csi(sd);
- tc358743_set_csi_color_space(sd);
return 0;
}
-->8--
That should enable the CSI-2 Tx and put it in LP-11 only after the CSI-2
receiver is enabled, right before starting streaming.
That did seem to work the few times I tested, but I have no idea how
this will behave with other chips that do something else to the bus
while not streaming, and whether it is ok to enable the CSI right after
the sensor without waiting for the CSI-2 bus to settle.
regards
Philipp
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 29/36] media: imx: mipi-csi2: enable setting and getting of frame rates
On Mon, 2017-02-20 at 16:18 -0800, Steve Longerbeam wrote:
>
> On 02/20/2017 04:13 PM, Russell King - ARM Linux wrote:
> > On Tue, Feb 21, 2017 at 12:04:10AM +0200, Sakari Ailus wrote:
> >> On Wed, Feb 15, 2017 at 06:19:31PM -0800, Steve Longerbeam wrote:
> >>> From: Russell King
> >>>
> >>> Setting and getting frame rates is part of the negotiation mechanism
> >>> between subdevs. The lack of support means that a frame rate at the
> >>> sensor can't be negotiated through the subdev path.
> >>
> >> Just wondering --- what do you need this for?
> >
> > The v4l2 documentation contradicts the media-ctl implementation.
> >
> > While v4l2 documentation says:
> >
> > These ioctls are used to get and set the frame interval at specific
> > subdev pads in the image pipeline. The frame interval only makes sense
> > for sub-devices that can control the frame period on their own. This
> > includes, for instance, image sensors and TV tuners. Sub-devices that
> > don't support frame intervals must not implement these ioctls.
> >
> > However, when trying to configure the pipeline using media-ctl, eg:
> >
> > media-ctl -d /dev/media1 --set-v4l2 '"imx219 pixel
> > 0-0010":0[crop:(0,0)/3264x2464]'
> > media-ctl -d /dev/media1 --set-v4l2 '"imx219
> > 0-0010":1[fmt:SRGGB10/3264x2464@1/30]'
> > media-ctl -d /dev/media1 --set-v4l2 '"imx219
> > 0-0010":0[fmt:SRGGB8/816x616@1/30]'
> > media-ctl -d /dev/media1 --set-v4l2
> > '"imx6-mipi-csi2":1[fmt:SRGGB8/816x616@1/30]'
> > Unable to setup formats: Inappropriate ioctl for device (25)
> > media-ctl -d /dev/media1 --set-v4l2
> > '"ipu1_csi0_mux":2[fmt:SRGGB8/816x616@1/30]'
> > media-ctl -d /dev/media1 --set-v4l2 '"ipu1_csi0":2[fmt:SRGGB8/816x616@1/30]'
> >
> > The problem there is that the format setting for the csi2 does not get
> > propagated forward:
> >
> > $ strace media-ctl -d /dev/media1 --set-v4l2
> > '"imx6-mipi-csi2":1[fmt:SRGGB8/816x616@1/30]'
> > ...
> > open("/dev/v4l-subdev16", O_RDWR) = 3
> > ioctl(3, VIDIOC_SUBDEV_S_FMT, 0xbec16244) = 0
> > ioctl(3, VIDIOC_SUBDEV_S_FRAME_INTERVAL, 0xbec162a4) = -1 ENOTTY
> > (Inappropriate
> > ioctl for device)
> > fstat64(1, {st_mode=S_IFCHR|0600, st_rdev=makedev(136, 2), ...}) = 0
> > write(1, "Unable to setup formats: Inappro"..., 61) = 61
> > Unable to setup formats: Inappropriate ioctl for device (25)
> > close(3)= 0
> > exit_group(1) = ?
> > +++ exited with 1 +++
> >
> > because media-ctl exits as soon as it encouters the error while trying
> > to set the frame rate.
> >
> > This makes implementing setup of the media pipeline in shell scripts
> > unnecessarily difficult - as you need to then know whether an entity
> > is likely not to support the VIDIOC_SUBDEV_S_FRAME_INTERVAL call,
> > and either avoid specifying a frame rate:
> >
> > $ strace media-ctl -d /dev/media1 --set-v4l2
> > '"imx6-mipi-csi2":1[fmt:SRGGB8/816x616]'
> > ...
> > open("/dev/v4l-subdev16", O_RDWR) = 3
> > ioctl(3, VIDIOC_SUBDEV_S_FMT, 0xbeb1a254) = 0
> > open("/dev/v4l-subdev0", O_RDWR)= 4
> > ioctl(4, VIDIOC_SUBDEV_S_FMT, 0xbeb1a254) = 0
> > close(4)= 0
> > close(3)= 0
> > exit_group(0) = ?
> > +++ exited with 0 +++
> >
> > or manually setting the format on the sink.
> >
> > Allowing the S_FRAME_INTERVAL call seems to me to be more in keeping
> > with the negotiation mechanism that is implemented in subdevs, and
> > IMHO should be implemented inside the kernel as a pad operation along
> > with the format negotiation, especially so as frame skipping is
> > defined as scaling, in just the same way as the frame size is also
> > scaling:
> >
> >- ``MEDIA_ENT_F_PROC_VIDEO_SCALER``
> >
> >- Video scaler. An entity capable of video scaling must have
> > at least one sink pad and one source pad, and scale the
> > video frame(s) received on its sink pad(s) to a different
> > resolution output on its source pad(s). The range of
> > supported scaling ratios is entity-specific and can differ
> > between the horizontal and vertical directions (in particular
> > scaling can be supported in one direction only). Binning and
> > skipping are considered as scaling.
> >
> > Although, this is vague, as it doesn't define what it means by "skipping",
> > whether that's skipping pixels (iow, sub-sampling) or whether that's
> > frame skipping.
I'd interpret this as meaning pixel skipping, not frame skipping.
> > Then there's the issue where, if you have this setup:
> >
> > camera --> csi2 receiver --> csi --> capture
> >
> > and the "csi" subdev can skip frames, you need to know (a) at the CSI
> > sink pad what the frame rate is of the source (b) what the desired
> > source pad frame rate is, so you can configure the frame skipping.
> > So, does the csi subdev have to walk back through the media graph
>
Re: [PATCH v4 15/36] platform: add video-multiplexer subdevice driver
On Sun, 2017-02-19 at 23:02 +0100, Pavel Machek wrote:
> Hi!
>
> > From: Philipp Zabel
> >
> > This driver can handle SoC internal and external video bus multiplexers,
> > controlled either by register bit fields or by a GPIO. The subdevice
> > passes through frame interval and mbus configuration of the active input
> > to the output side.
> >
> > Signed-off-by: Sascha Hauer
> > Signed-off-by: Philipp Zabel
> > --
> >
>
> Again, this is slightly non-standard format. Normally changes from v1
> go below ---, but in your case it would cut off the signoff...
>
> > diff --git a/Documentation/devicetree/bindings/media/video-multiplexer.txt
> > b/Documentation/devicetree/bindings/media/video-multiplexer.txt
> > new file mode 100644
> > index 000..9d133d9
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/video-multiplexer.txt
> > @@ -0,0 +1,59 @@
> > +Video Multiplexer
> > +=
> > +
> > +Video multiplexers allow to select between multiple input ports. Video
> > received
> > +on the active input port is passed through to the output port. Muxes
> > described
> > +by this binding may be controlled by a syscon register bitfield or by a
> > GPIO.
> > +
> > +Required properties:
> > +- compatible : should be "video-multiplexer"
> > +- reg: should be register base of the register containing the control
> > bitfield
> > +- bit-mask: bitmask of the control bitfield in the control register
> > +- bit-shift: bit offset of the control bitfield in the control register
> > +- gpios: alternatively to reg, bit-mask, and bit-shift, a single GPIO
> > phandle
> > + may be given to switch between two inputs
> > +- #address-cells: should be <1>
> > +- #size-cells: should be <0>
> > +- port@*: at least three port nodes containing endpoints connecting to the
> > + source and sink devices according to of_graph bindings. The last port is
> > + the output port, all others are inputs.
>
> At least three? I guess it is exactly three with the gpio?
Yes. With the mmio bitfield muxes there can be more.
> Plus you might want to describe which port correspond to which gpio
> state/bitfield values...
>
> > +struct vidsw {
>
> I knew it: it is secretely a switch! :-).
This driver started as a two-input gpio controlled bus switch.
I changed the name when adding support for bitfield controlled
multiplexers with more than two inputs.
> > +static void vidsw_set_active(struct vidsw *vidsw, int active)
> > +{
> > + vidsw->active = active;
> > + if (active < 0)
> > + return;
> > +
> > + dev_dbg(vidsw->subdev.dev, "setting %d active\n", active);
> > +
> > + if (vidsw->field)
> > + regmap_field_write(vidsw->field, active);
> > + else if (vidsw->gpio)
> > + gpiod_set_value(vidsw->gpio, active);
>
> else dev_err()...?
If neither field nor gpio are set, probe will have failed and this will
never be called.
> > +static int vidsw_async_init(struct vidsw *vidsw, struct device_node *node)
> > +{
> > + struct device_node *ep;
> > + u32 portno;
> > + int numports;
>
> numbports is int, so I guess portno should be, too?
We could change both to unsigned int, as both vidsw->num_pads and
endpoint.base.port are unsigned int, and they are only compared/assigned
to those and each other.
> > + portno = endpoint.base.port;
> > + if (portno >= numports - 1)
> > + continue;
>
I.
> > + if (!pad) {
> > + /* Mirror the input side on the output side */
> > + cfg->type = vidsw->endpoint[vidsw->active].bus_type;
> > + if (cfg->type == V4L2_MBUS_PARALLEL ||
> > + cfg->type == V4L2_MBUS_BT656)
> > + cfg->flags =
> > vidsw->endpoint[vidsw->active].bus.parallel.flags;
> > + }
>
> Will this need support for other V4L2_MBUS_ values?
To support CSI-2 multiplexers, yes.
> > +MODULE_AUTHOR("Sascha Hauer, Pengutronix");
> > +MODULE_AUTHOR("Philipp Zabel, Pengutronix");
>
> Normally, MODULE_AUTHOR contains comma separated names of authors,
> perhaps with . Not sure two MODULE_AUTHORs per file
> will work.
>
> Thanks,
> Pavel
regards
Philipp
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 33/36] media: imx: redo pixel format enumeration and negotiation
Hi Steve, On Wed, 2017-02-22 at 15:52 -0800, Steve Longerbeam wrote: > Hi Philipp, > > > On 02/16/2017 03:32 AM, Philipp Zabel wrote: > > On Wed, 2017-02-15 at 18:19 -0800, Steve Longerbeam wrote: > >> The previous API and negotiation of mbus codes and pixel formats > >> was broken, and has been completely redone. > >> > >> The negotiation of media bus codes should be as follows: > >> > >> CSI: > >> > >> sink pad direct src pad IDMAC src pad > >> - > >> RGB (any)IPU RGB RGB (any) > >> YUV (any)IPU YUV YUV (any) > >> Bayer N/A must be same bayer code as sink > > > > The IDMAC src pad should also use the internal 32-bit RGB / YUV format, > > except if bayer/raw mode is selected, in which case the attached capture > > video device should only allow a single mode corresponding to the output > > pad media bus format. > > The IDMAC source pad is going to memory, so it has left the IPU. > Are you sure it should be an internal IPU format? I realize it > is linked to a capture device node, and the IPU format could then > be translated to a v4l2 fourcc by the capture device, but IMHO this > pad is external to the IPU. The CSI IDMAC source pad should describe the format at the connection between the CSI and the IDMAC, just as the icprpvf and icprpenc source pads. The format outside of the IPU is the memory format written by the IDMAC, but that is a memory pixel format and not a media bus format at all. That would also make it more straightforward to enumerate the memory pixel formats in the capture device: If the source pad media bus format is 32-bit YUV, enumerate all YUV formats, if it is 32-bit RGB or RGB565, enumerate all rgb formats, otherwise (bayer/raw mode) only allow the specific memory format matching the bus format. regards Philipp ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 29/36] media: imx: mipi-csi2: enable setting and getting of frame rates
On Mon, 2017-03-13 at 13:27 +, Russell King - ARM Linux wrote: > On Mon, Mar 13, 2017 at 03:16:48PM +0200, Sakari Ailus wrote: > > The vast majority of existing drivers do not implement them nor the user > > space expects having to set them. Making that mandatory would break existing > > user space. > > > > In addition, that does not belong to link validation either: link validation > > should only include static properties of the link that are required for > > correct hardware operation. Frame rate is not such property: hardware that > > supports the MC interface generally does not recognise such concept (with > > the exception of some sensors). Additionally, it is dynamic: the frame rate > > can change during streaming, making its validation at streamon time useless. > > So how do we configure the CSI, which can do frame skipping? > > With what you're proposing, it means it's possible to configure the > camera sensor source pad to do 50fps. Configure the CSI sink pad to > an arbitary value, such as 30fps, and configure the CSI source pad to > 15fps. > > What you actually get out of the CSI is 25fps, which bears very little > with the actual values used on the CSI source pad. > > You could say "CSI should ask the camera sensor" - well, that's fine > if it's immediately downstream, but otherwise we'd need to go walking > down the graph to find something that resembles its source - there may > be mux and CSI2 interface subdev blocks in that path. Or we just accept > that frame rates are completely arbitary and bear no useful meaning what > so ever. Which would include the frame interval returned by VIDIOC_G_PARM on the connected video device, as that gets its information from the CSI output pad's frame interval. regards Philipp ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 29/36] media: imx: mipi-csi2: enable setting and getting of frame rates
On Tue, 2017-03-14 at 08:34 +0100, Hans Verkuil wrote: > On 03/13/2017 10:03 PM, Sakari Ailus wrote: > > Hi Steve, > > > > On Mon, Mar 13, 2017 at 11:06:22AM -0700, Steve Longerbeam wrote: > >> > >> > >> On 03/13/2017 06:55 AM, Philipp Zabel wrote: > >>> On Mon, 2017-03-13 at 13:27 +, Russell King - ARM Linux wrote: > >>>> On Mon, Mar 13, 2017 at 03:16:48PM +0200, Sakari Ailus wrote: > >>>>> The vast majority of existing drivers do not implement them nor the user > >>>>> space expects having to set them. Making that mandatory would break > >>>>> existing > >>>>> user space. > >>>>> > >>>>> In addition, that does not belong to link validation either: link > >>>>> validation > >>>>> should only include static properties of the link that are required for > >>>>> correct hardware operation. Frame rate is not such property: hardware > >>>>> that > >>>>> supports the MC interface generally does not recognise such concept > >>>>> (with > >>>>> the exception of some sensors). Additionally, it is dynamic: the frame > >>>>> rate > >>>>> can change during streaming, making its validation at streamon time > >>>>> useless. > >>>> > >>>> So how do we configure the CSI, which can do frame skipping? > >>>> > >>>> With what you're proposing, it means it's possible to configure the > >>>> camera sensor source pad to do 50fps. Configure the CSI sink pad to > >>>> an arbitary value, such as 30fps, and configure the CSI source pad to > >>>> 15fps. > >>>> > >>>> What you actually get out of the CSI is 25fps, which bears very little > >>>> with the actual values used on the CSI source pad. > >>>> > >>>> You could say "CSI should ask the camera sensor" - well, that's fine > >>>> if it's immediately downstream, but otherwise we'd need to go walking > >>>> down the graph to find something that resembles its source - there may > >>>> be mux and CSI2 interface subdev blocks in that path. Or we just accept > >>>> that frame rates are completely arbitary and bear no useful meaning what > >>>> so ever. > >>> > >>> Which would include the frame interval returned by VIDIOC_G_PARM on the > >>> connected video device, as that gets its information from the CSI output > >>> pad's frame interval. > >>> > >> > >> I'm kinda in the middle on this topic. I agree with Sakari that > >> frame rate can fluctuate, but that should only be temporary. If > >> the frame rate permanently shifts from what a subdev reports via > >> g_frame_interval, then that is a system problem. So I agree with > >> Phillip and Russell that a link validation of frame interval still > >> makes sense. > >> > >> But I also have to agree with Sakari that a subdev that has no > >> control over frame rate has no business implementing those ops. > >> > >> And then I agree with Russell that for subdevs that do have control > >> over frame rate, they would have to walk the graph to find the frame > >> rate source. > >> > >> So we're stuck in a broken situation: either the subdevs have to walk > >> the graph to find the source of frame rate, or s_frame_interval > >> would have to be mandatory and validated between pads, same as set_fmt. > > > > It's not broken; what we are missing though is documentation on how to > > control devices that can change the frame rate i.e. presumably drop frames > > occasionally. > > > > If you're doing something that hasn't been done before, it may be that new > > documentation needs to be written to accomodate that use case. As we have an > > existing interface (VIDIOC_SUBDEV_[GS]_FRAME_INTERVAL) it does make sense > > to use that. What is not possible, though, is to mandate its use in link > > validation everywhere. > > > > If you had a hardware limitation that would require that the frame rate is > > constant, then we'd need to handle that in link validation for that > > particular piece of hardware. But there really is no case for doing that for > > everything else. > > > > General note: I would strongly recommend that g/s_parm support is removed in > v4l2_subdev in favor of g/s_frame_interval. > > g/s_parm is an abomination... Agreed. Just in this specific case I was talking about G_PARM on the /dev/video node, not the v4l2_subdev nodes. This is currently used by non-subdev-aware userspace to obtain the framerate from the video capture device. > There seem to be only a few i2c drivers that use g/s_parm, so this shouldn't > be a lot of work. > > Having two APIs for the same thing is always very bad. > > Regards, > > Hans > regards Philipp ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: media / v4l2-mc: wishlist for complex cameras (was Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline)
On Wed, 2017-03-15 at 14:55 -0400, Nicolas Dufresne wrote: > Le mercredi 15 mars 2017 à 11:50 +0100, Philippe De Muyter a écrit : > > > I would say: camorama, xawtv3, zbar, google talk, skype. If it runs > > > with those, it will likely run with any other application. > > > > > > > I would like to add the 'v4l2src' plugin of gstreamer, and on the > > imx6 its > > While it would be nice if somehow you would get v4l2src to work (in > some legacy/emulation mode through libv4l2), v4l2src works just fine, provided the pipeline is configured manually in advance via media-ctl. > the longer plan is to > implement smart bin that handle several v4l2src, that can do the > required interactions so we can expose similar level of controls as > found in Android Camera HAL3, and maybe even further assuming userspace > can change the media tree at run-time. We might be a long way from > there, specially that some of the features depends on how much the > hardware can do. Just being able to figure-out how to build the MC tree > dynamically seems really hard when thinking of generic mechanism. Also, > Request API will be needed. > > I think for this one, we'll need some userspace driver that enable the > features (not hide them), and that's what I'd be looking for from > libv4l2 in this regard. regards Philipp ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: media / v4l2-mc: wishlist for complex cameras (was Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline)
On Thu, 2017-03-16 at 10:47 +0100, Philippe De Muyter wrote: > On Thu, Mar 16, 2017 at 10:26:00AM +0100, Philipp Zabel wrote: > > On Wed, 2017-03-15 at 14:55 -0400, Nicolas Dufresne wrote: > > > Le mercredi 15 mars 2017 à 11:50 +0100, Philippe De Muyter a écrit : > > > > > I would say: camorama, xawtv3, zbar, google talk, skype. If it runs > > > > > with those, it will likely run with any other application. > > > > > > > > > > > > > I would like to add the 'v4l2src' plugin of gstreamer, and on the > > > > imx6 its > > > > > > While it would be nice if somehow you would get v4l2src to work (in > > > some legacy/emulation mode through libv4l2), > > > > v4l2src works just fine, provided the pipeline is configured manually in > > advance via media-ctl. > > Including choosing the framerate ? Sorry, I have no time these days > to test it myself. No, the framerate is set with media-ctl on the CSI output pad. To really choose the framerate, the element would indeed need a deeper understanding of the pipeline, as the resulting framerate depends on at least the source v4l2_subdevice (sensor) framerate and the CSI frame skipping. > And I cited imxv4l2videosrc for its ability to provide the physical address > of the image buffers for further processing by other (not necessarily next > in gstreamer pipeline, or for all frames) hardware-accelerated plugins likes > the h.264 video encoder. As I am stuck with fsl/nxp kernel and driver on that > matter, I don't know how the interfaces have evolved in current linux kernels. The physical address of the image buffers is hidden from userspace by dma-buf objects, but those can be passed around to the next driver without copying the image data. regards Philipp ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline
On Fri, 2017-03-17 at 11:42 +, Russell King - ARM Linux wrote: > On Tue, Mar 14, 2017 at 08:55:36AM +0100, Hans Verkuil wrote: > > We're all very driver-development-driven, and userspace gets very little > > attention in general. So before just throwing in the towel we should take > > a good look at the reasons why there has been little or no development: is > > it because of fundamental design defects, or because nobody paid attention > > to it? > > > > I strongly suspect it is the latter. > > > > In addition, I suspect end-users of these complex devices don't really care > > about a plugin: they want full control and won't typically use generic > > applications. If they would need support for that, we'd have seen much more > > interest. The main reason for having a plugin is to simplify testing and > > if this is going to be used on cheap hobbyist devkits. > > I think you're looking at it with a programmers hat on, not a users hat. > > Are you really telling me that requiring users to 'su' to root, and then > use media-ctl to manually configure the capture device is what most > users "want" ? > > Hasn't the way technology has moved towards graphical interfaces, > particularly smart phones, taught us that the vast majority of users > want is intuitive, easy to use interfaces, and not the command line > with reams of documentation? > > Why are smart phones soo popular - it's partly because they're flashy, > but also because of the wealth of apps, and apps which follow the > philosophy of "do one job, do it well" (otherwise they get bad reviews.) > > An additional complication is simply that it is hard to find fully supported > > MC hardware. omap3 boards are hard to find these days, renesas boards are > > not > > easy to get, freescale isn't the most popular either. Allwinner, mediatek, > > amlogic, broadcom and qualcomm all have closed source implementations or no > > implementation at all. > > Right, and that in itself tells us something - the problem that we're > trying to solve is not one that commonly exists in the real world. > > Yes, the hardware we have in front of us may be very complex, but if > there's very few systems out there which are capable of making use of > all that complexity, then we're trying to solve a problem that isn't > the common case - and if it's going to take years to solve it (it > already has taken years) then it's the wrong problem to be solved. > > I bet most of the problem can be eliminated if, rather than exposing > all this complexity, we instead expose a simpler capture system where > the board designer gets to "wire up" the capture system. > > I'll go back to my Bayer example, because that's the simplest. As > I've already said many times in these threads, there is only one > possible path through the iMX6 device that such a source can be used > with - it's a fixed path. The actual path depends on the CSI2 > virtual channel that the camera has been _configured_ to use, but > apart from that, it's effectively a well known set of blocks. Such > a configuration could be placed in DT. > > For RGB connected to a single parallel CSI, things get a little more > complex - capture through the CSI or through two other capture devices > for de-interlacing or other features. However, I'm not convinced that > exposing multiple /dev/video* devices for different features for the > same video source is a sane approach - I think that's a huge usability > problem. (The user is expected to select the capture device on iMX6 > depending on the features they want, and if they want to change features, > they're expected to shut down their application and start it up on a > different capture device.) For the most part on iMX6, there's one > path down to the CSI block, and then there's optional routing through > the rest of the IPU depending on what features you want (such as > de-interlacing.) > > The complex case is a CSI2 connected camera which produces multiple > streams through differing virtual channels - and that's IMHO the only > case where we need multiple different /dev/video* capture devices to > be present. I wanted to have the IC PRP outputs separate because the IC PRP should support running both the VF and ENC tasks with different parameters from the same input. That would allow to capture two different resolutions (up to 1024x1024) at the same time. I think most of the simple, fixed pipeline use cases could be handled by libv4l2, by allowing to pass a v4l2 subdevice path to v4l2_open. If that function internally would set up the media links to the nearest /dev/video interface, propagate format, resolution and frame intervals if necessary, and return an fd to the video device, there'd be no additional complexity for the users beyond selecting the v4l2_subdev instead of the video device. regards Philipp ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo
Re: [PATCH v4 14/36] [media] v4l2-mc: add a function to inherit controls from a pipeline
On Fri, 2017-03-17 at 10:24 -0300, Mauro Carvalho Chehab wrote: [...] > The big question, waiting for an answer on the last 8 years is > who would do that? Such person would need to have several different > hardware from different vendors, in order to ensure that it has > a generic solution. > > It is a way more feasible that the Kernel developers that already > have a certain hardware on their hands to add support inside the > driver to forward the controls through the pipeline and to setup > a "default" pipeline that would cover the common use cases at > driver's probe. Actually, would setting pipeline via libv4l2 plugin and letting drivers provide a sane enabled default pipeline configuration be mutually exclusive? Not sure about the control forwarding, but at least a simple link setup and format forwarding would also be possible in the kernel without hindering userspace from doing it themselves later. regards Philipp ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v5 38/39] media: imx: csi: fix crop rectangle reset in sink set_fmt
On Sun, 2017-03-19 at 12:08 -0700, Steve Longerbeam wrote: > > On 03/19/2017 08:22 AM, Russell King - ARM Linux wrote: > > On Thu, Mar 09, 2017 at 08:53:18PM -0800, Steve Longerbeam wrote: > >> From: Philipp Zabel > >> > >> The csi_try_crop call in set_fmt should compare the cropping rectangle > >> to the currently set input format, not to the previous input format. > > Are we really sure that the cropping support is implemented correctly? > > > > I came across this while looking at what we're doing with the > > V4L2_SEL_FLAG_KEEP_CONFIG flag. > > > > Documentation/media/uapi/v4l/dev-subdev.rst defines the behaviour of > > the user API, and "Order of configuration and format propagation" says: > > > >The coordinates to a step always refer to the actual size of the > >previous step. The exception to this rule is the source compose > >rectangle, which refers to the sink compose bounds rectangle --- if it > >is supported by the hardware. > > > >1. Sink pad format. The user configures the sink pad format. This format > > defines the parameters of the image the entity receives through the > > pad for further processing. > > > >2. Sink pad actual crop selection. The sink pad crop defines the crop > > performed to the sink pad format. > > > >3. Sink pad actual compose selection. The size of the sink pad compose > > rectangle defines the scaling ratio compared to the size of the sink > > pad crop rectangle. The location of the compose rectangle specifies > > the location of the actual sink compose rectangle in the sink compose > > bounds rectangle. > > > >4. Source pad actual crop selection. Crop on the source pad defines crop > > performed to the image in the sink compose bounds rectangle. > > > >5. Source pad format. The source pad format defines the output pixel > > format of the subdev, as well as the other parameters with the > > exception of the image width and height. Width and height are defined > > by the size of the source pad actual crop selection. > > > >Accessing any of the above rectangles not supported by the subdev will > >return ``EINVAL``. Any rectangle referring to a previous unsupported > >rectangle coordinates will instead refer to the previous supported > >rectangle. For example, if sink crop is not supported, the compose > >selection will refer to the sink pad format dimensions instead. > > > > Note step 3 above: scaling is defined by the ratio of the _sink_ crop > > rectangle to the _sink_ compose rectangle. The above paragraph suggests we skip any rectangles that are not supported. In our case that would be 3. and 4., since the CSI can't compose into a larger frame. I hadn't realised that the crop selection currently happens on the source pad. The hardware actually only supports cropping of the input (the crop rectangle we write into the window registers are before downscaling). So the crop rectangle should be moved to the sink pad. > > So, lets say that the camera produces a 1280x720 image, and the sink > > pad format is configured with 1280x720. That's step 1. > > > > The sink crop operates within that rectangle, cropping it to an area. > > Let's say we're only interested in its centre, so we'd chose 640x360 > > with the top-left as 320,180. This is step 2. >> > > Then, if we want to down-scale by a factor of two, we'd set the sink > > compose selection to 320x180. Except when composing is not supported. If the sink compose and source crop rectangles are not supported, the source pad format takes their place in determining the scaling output resolution. At least that's how I read the documentation. > > This seems to be at odds with how the scaling is done in CSI at > > present: the selection implementations all reject attempts to > > configure the sink pad, instead only supporting crop rectangles on > > the source, > > Correct. Currently cropping is only supported at the source pad > (step 4). > > Initially the CSI didn't support down-scaling, so step 3 is not supported, > so the sink pad format/crop selection rectangle/crop compose rectangle > are collapsed into the same sink pad format rectangle. > > Philipp later added support for /2 downscaling, but we didn't put this in > the correct API, looks like this needs to move into the selection API at > step 3 (sink pad compose rectangle). I am not sure about this. Wouldn't moving the input crop to the sink pad
Re: [PATCH v5 00/39] i.MX Media Driver
On Sat, 2017-03-18 at 12:58 -0700, Steve Longerbeam wrote: > > On 03/18/2017 12:22 PM, Russell King - ARM Linux wrote: > > Hi Steve, > > > > I've just been trying to get gstreamer to capture and h264 encode > > video from my camera at various frame rates, and what I've discovered > > does not look good. > > > > 1) when setting frame rates, media-ctl _always_ calls > > VIDIOC_SUBDEV_S_FRAME_INTERVAL with pad=0. To allow setting pad > 0: https://patchwork.linuxtv.org/patch/39348/ > > 2) media-ctl never retrieves the frame interval information, so there's > > no way to read it back with standard tools, and no indication that > > this is going on... > > I think Philipp Zabel submitted a patch which addresses these > in media-ctl. Check with him. To read back and propagate the frame interval: https://patchwork.linuxtv.org/patch/39349/ https://patchwork.linuxtv.org/patch/39351/ regards Philipp ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v5 00/39] i.MX Media Driver
On Sun, 2017-03-19 at 12:14 +, Russell King - ARM Linux wrote: > On Sat, Mar 18, 2017 at 12:58:27PM -0700, Steve Longerbeam wrote: > > On 03/18/2017 12:22 PM, Russell King - ARM Linux wrote: > > >0:00:01.955927879 20954 0x15ffe90 INFOv4l2 > > >gstv4l2object.c:3811:gst_v4l2_object_get_caps: probed caps: > > >video/x-bayer, format=(string)rggb, framerate=(fraction)3/1001, > > >width=(int)816, height=(int)616, pixel-aspect-ratio=(fraction)1/1; > > >video/x-raw, format=(string)I420, framerate=(fraction)3/1001, > > >width=(int)816, height=(int)616, interlace-mode=(string)progressive, > > >pixel-aspect-ratio=(fraction)1/1; video/x-raw, format=(string)YV12, > > >framerate=(fraction)3/1001, width=(int)816, height=(int)616, > > >interlace-mode=(string)progressive, pixel-aspect-ratio=(fraction)1/1; > > >video/x-raw, format=(string)BGR, framerate=(fraction)3/1001, > > >width=(int)816, height=(int)616, interlace-mode=(string)progressive, > > >pixel-aspect-ratio=(fraction)1/1; video/x-raw, format=(string)RGB, > > >framerate=(fraction)3/1001, width=(int)816, height=(int)616, > > >interlace-mode=(string)progressive, pixel-aspect-ratio=(fraction)1/1 > > > > > >despite the media pipeline actually being configured for 60fps. > > > > > >Forcing it by adjusting the pipeline only results in gstreamer > > >failing, because it believes that v4l2 is unable to operate at > > >60fps. > > > > > >Also note the complaints from v4l2src about the non-compliance... > > > > Thanks, I've fixed most of v4l2-compliance issues, but this is not > > done yet. Is that something you can help with? > > I've looked at this, and IMHO it's yet another media control API mess. > > - media-ctl itself allows setting the format on subdev pads via > struct v4l2_subdev_format. > > - struct v4l2_subdev_format contains a struct v4l2_mbus_framefmt. > > - struct v4l2_mbus_framefmt contains: > * @width: frame width > * @height: frame height > * @code: data format code (from enum v4l2_mbus_pixelcode) > * @field: used interlacing type (from enum v4l2_field) > * @colorspace: colorspace of the data (from enum v4l2_colorspace) > * @ycbcr_enc: YCbCr encoding of the data (from enum v4l2_ycbcr_encoding) > * @quantization: quantization of the data (from enum v4l2_quantization) > * @xfer_func: transfer function of the data (from enum v4l2_xfer_func) > > - media-ctl sets width, height, code and field, but nothing else. > > We're already agreed that the fields that media-ctl are part of the > format negotiation between the ultimate source, flowing down to the > capture device. However, there's no support in media-ctl to deal > with these other fields - so media-ctl in itself is only half- > implemented. To set and read colorimetry information: https://patchwork.linuxtv.org/patch/39350/ regards Philipp ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v5 38/39] media: imx: csi: fix crop rectangle reset in sink set_fmt
On Mon, 2017-03-20 at 12:08 +, Russell King - ARM Linux wrote: > On Mon, Mar 20, 2017 at 12:55:26PM +0100, Philipp Zabel wrote: > > The above paragraph suggests we skip any rectangles that are not > > supported. In our case that would be 3. and 4., since the CSI can't > > compose into a larger frame. I hadn't realised that the crop selection > > currently happens on the source pad. > > I'd recommend viewing the documentation in its post-processed version, > because then you get the examples as pictures, and they say that a > picture is worth 1000 words. See > > https://linuxtv.org/downloads/v4l-dvb-apis/uapi/v4l/dev-subdev.html > > There is almost an exact example of what we're trying to do - it's > figure 4.6. Here, we have a sink pad with a cropping rectangle on > the input, which is then scaled to a composition rectangle (there's > no bounds rectangle, and it's specified that in such a case the > top,left of the composition rectangle will always be 0,0 - see quote > below). > > Where it differs is that the example also supports source cropping > for two source pads. We don't support that. > > The same document says: > > Scaling support is optional. When supported by a subdev, the crop > rectangle on the subdev's sink pad is scaled to the size configured > using the > :ref:`VIDIOC_SUBDEV_S_SELECTION ` IOCTL > using ``V4L2_SEL_TGT_COMPOSE`` selection target on the same pad. If the > subdev supports scaling but not composing, the top and left values are > not used and must always be set to zero. Right, this sentence does imply that when scaling is supported, there must be a sink compose rectangle, even when composing is not. I have previously set up scaling like this: media-ctl --set-v4l2 "'ipu1_csi0_mux':2[fmt:UYVY2X8/1920x1080@1/60]" media-ctl --set-v4l2 "'ipu1_csi0':2[fmt:AYUV32/960x540@1/30]" Does this mean, it should work like this instead? media-ctl --set-v4l2 "'ipu1_csi0_mux':2[fmt:UYVY2X8/1920x1080@1/60]" media-ctl --set-v4l2 "'ipu1_csi0':0[fmt:UYVY2X8/1920x1080@1/60,compose:(0,0)/960x540]" media-ctl --set-v4l2 "'ipu1_csi0':2[fmt:AYUV32/960x540@1/30]" I suppose setting the source pad format should not be allowed to modify the sink compose rectangle. regards Philipp ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v5 00/39] i.MX Media Driver
On Mon, 2017-03-20 at 15:43 +, Russell King - ARM Linux wrote:
> On Mon, Mar 20, 2017 at 02:20:16PM +0100, Philipp Zabel wrote:
> > To set and read colorimetry information:
> > https://patchwork.linuxtv.org/patch/39350/
>
> Thanks, I've applied all four of your patches, but there's a side effect
> from that. Old media-ctl (modified by me):
>
> - entity 53: imx219 0-0010 (2 pads, 2 links)
> type V4L2 subdev subtype Unknown flags 0
> device node name /dev/v4l-subdev9
> pad0: Source
> [fmt:SRGGB8/816x616 field:none
> frame_interval:1/25]
> -> "imx6-mipi-csi2":0 [ENABLED]
> pad1: Sink
> [fmt:SRGGB10/3280x2464 field:none
> crop.bounds:(0,0)/3280x2464
> crop:(0,0)/3264x2464
> compose.bounds:(0,0)/3264x2464
> compose:(0,0)/816x616]
> <- "imx219 pixel 0-0010":0 [ENABLED,IMMUTABLE]
>
> New media-ctl:
>
> - entity 53: imx219 0-0010 (2 pads, 2 links)
> type V4L2 subdev subtype Unknown flags 0
> device node name /dev/v4l-subdev9
> pad0: Source
> [fmt:SRGGB8_1X8/816x616@1/25 field:none colorspace:srgb
> xfer:srgb]
> -> "imx6-mipi-csi2":0 [ENABLED]
> pad1: Sink
> <- "imx219 pixel 0-0010":0 [ENABLED,IMMUTABLE]
>
> It looks like we successfully retrieve the frame interval for pad 0
> and print it, but when we try to retrieve the frame interval for pad 1,
> we get EINVAL (because that's what I'm returning, but I'm wondering if
> that's the correct thing to do...) and that prevents _all_ format
> information being output.
According to the documentation [1], you are doing the right thing:
The struct v4l2_subdev_frame_interval pad references a non-existing
pad, or the pad doesn’t support frame intervals.
But v4l2_subdev_call returns -ENOIOCTLCMD if the g_frame_interval op is
not implemented at all, which is turned into -ENOTTY by video_usercopy.
[1]
https://linuxtv.org/downloads/v4l-dvb-apis/uapi/v4l/vidioc-subdev-g-frame-interval.html#return-value
> Maybe something like the following would be a better idea?
>
> utils/media-ctl/media-ctl.c | 10 +-
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/utils/media-ctl/media-ctl.c b/utils/media-ctl/media-ctl.c
> index f61963a..a50a559 100644
> --- a/utils/media-ctl/media-ctl.c
> +++ b/utils/media-ctl/media-ctl.c
> @@ -81,22 +81,22 @@ static void v4l2_subdev_print_format(struct media_entity
> *entity,
> struct v4l2_mbus_framefmt format;
> struct v4l2_fract interval = { 0, 0 };
> struct v4l2_rect rect;
> - int ret;
> + int ret, err_fi;
>
> ret = v4l2_subdev_get_format(entity, &format, pad, which);
> if (ret != 0)
> return;
>
> - ret = v4l2_subdev_get_frame_interval(entity, &interval, pad);
> - if (ret != 0 && ret != -ENOTTY)
> - return;
> + err_fi = v4l2_subdev_get_frame_interval(entity, &interval, pad);
Not supporting frame intervals doesn't warrant a visible error message,
I think -EINVAL should also be ignored above, if the spec is to be
believed.
>
> printf("\t\t[fmt:%s/%ux%u",
> v4l2_subdev_pixelcode_to_string(format.code),
> format.width, format.height);
>
> - if (interval.numerator || interval.denominator)
> + if (err_fi == 0 && (interval.numerator || interval.denominator))
> printf("@%u/%u", interval.numerator, interval.denominator);
> + else if (err_fi != -ENOTTY)
> + printf("@", strerror(-err_fi));
Or here.
>
> if (format.field)
> printf(" field:%s", v4l2_subdev_field_to_string(format.field));
>
>
regards
Philipp
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v5 38/39] media: imx: csi: fix crop rectangle reset in sink set_fmt
Hi Steve, Russell,
On Mon, 2017-03-20 at 14:17 +, Russell King - ARM Linux wrote:
> On Mon, Mar 20, 2017 at 03:00:51PM +0100, Philipp Zabel wrote:
> > On Mon, 2017-03-20 at 12:08 +, Russell King - ARM Linux wrote:
> > > The same document says:
> > >
> > > Scaling support is optional. When supported by a subdev, the crop
> > > rectangle on the subdev's sink pad is scaled to the size configured
> > > using the
> > > :ref:`VIDIOC_SUBDEV_S_SELECTION ` IOCTL
> > > using ``V4L2_SEL_TGT_COMPOSE`` selection target on the same pad. If the
> > > subdev supports scaling but not composing, the top and left values are
> > > not used and must always be set to zero.
> >
> > Right, this sentence does imply that when scaling is supported, there
> > must be a sink compose rectangle, even when composing is not.
> >
> > I have previously set up scaling like this:
> >
> > media-ctl --set-v4l2 "'ipu1_csi0_mux':2[fmt:UYVY2X8/1920x1080@1/60]"
> > media-ctl --set-v4l2 "'ipu1_csi0':2[fmt:AYUV32/960x540@1/30]"
> >
> > Does this mean, it should work like this instead?
> >
> > media-ctl --set-v4l2 "'ipu1_csi0_mux':2[fmt:UYVY2X8/1920x1080@1/60]"
> > media-ctl --set-v4l2
> > "'ipu1_csi0':0[fmt:UYVY2X8/1920x1080@1/60,compose:(0,0)/960x540]"
> > media-ctl --set-v4l2 "'ipu1_csi0':2[fmt:AYUV32/960x540@1/30]"
> >
> > I suppose setting the source pad format should not be allowed to modify
> > the sink compose rectangle.
>
> That is what I believe having read these documents several times, but
> we need v4l2 people to confirm.
What do you think of this:
--8<--
>From 2830aebc404bdfc9d7fc1ec94e5282d0b668e8f6 Mon Sep 17 00:00:00 2001
From: Philipp Zabel
Date: Mon, 20 Mar 2017 17:10:21 +0100
Subject: [PATCH] media: imx: csi: add sink selection rectangles
Move the crop rectangle to the sink pad and add a sink compose rectangle
to configure scaling. Also propagate rectangles from sink pad to crop
rectangle, to compose rectangle, and to the source pads both in ACTIVE
and TRY variants of set_fmt/selection, and initialize the default crop
and compose rectangles.
Signed-off-by: Philipp Zabel
---
drivers/staging/media/imx/imx-media-csi.c | 216 +-
1 file changed, 152 insertions(+), 64 deletions(-)
diff --git a/drivers/staging/media/imx/imx-media-csi.c
b/drivers/staging/media/imx/imx-media-csi.c
index 560da3abdd70b..b026a5d602ddf 100644
--- a/drivers/staging/media/imx/imx-media-csi.c
+++ b/drivers/staging/media/imx/imx-media-csi.c
@@ -79,6 +79,7 @@ struct csi_priv {
const struct imx_media_pixfmt *cc[CSI_NUM_PADS];
struct v4l2_fract frame_interval;
struct v4l2_rect crop;
+ struct v4l2_rect compose;
const struct csi_skip_desc *skip[CSI_NUM_PADS - 1];
/* active vb2 buffers to send to video dev sink */
@@ -574,8 +575,8 @@ static int csi_setup(struct csi_priv *priv)
ipu_csi_set_window(priv->csi, &priv->crop);
ipu_csi_set_downsize(priv->csi,
-priv->crop.width == 2 * outfmt->width,
-priv->crop.height == 2 * outfmt->height);
+priv->crop.width == 2 * priv->compose.width,
+priv->crop.height == 2 * priv->compose.height);
ipu_csi_init_interface(priv->csi, &sensor_mbus_cfg, &if_fmt);
@@ -1001,6 +1002,27 @@ __csi_get_fmt(struct csi_priv *priv, struct
v4l2_subdev_pad_config *cfg,
return &priv->format_mbus[pad];
}
+static struct v4l2_rect *
+__csi_get_crop(struct csi_priv *priv, struct v4l2_subdev_pad_config *cfg,
+ enum v4l2_subdev_format_whence which)
+{
+ if (which == V4L2_SUBDEV_FORMAT_TRY)
+ return v4l2_subdev_get_try_crop(&priv->sd, cfg, CSI_SINK_PAD);
+ else
+ return &priv->crop;
+}
+
+static struct v4l2_rect *
+__csi_get_compose(struct csi_priv *priv, struct v4l2_subdev_pad_config *cfg,
+ enum v4l2_subdev_format_whence which)
+{
+ if (which == V4L2_SUBDEV_FORMAT_TRY)
+ return v4l2_subdev_get_try_compose(&priv->sd, cfg,
+ CSI_SINK_PAD);
+ else
+ return &priv->compose;
+}
+
static void csi_try_crop(struct csi_priv *priv,
struct v4l2_rect *crop,
struct v4l2_subdev_pad_config *cfg,
@@ -1159,6 +1181,7 @@ static void csi_try_fmt(struct csi_priv *priv,
struct v4l2_subdev_pad_config *cfg,
struct v4l2_subdev_format *sdformat,
Re: [PATCH v5 38/39] media: imx: csi: fix crop rectangle reset in sink set_fmt
On Mon, 2017-03-20 at 14:17 +, Russell King - ARM Linux wrote: > On Mon, Mar 20, 2017 at 03:00:51PM +0100, Philipp Zabel wrote: > > On Mon, 2017-03-20 at 12:08 +, Russell King - ARM Linux wrote: > > > The same document says: > > > > > > Scaling support is optional. When supported by a subdev, the crop > > > rectangle on the subdev's sink pad is scaled to the size configured > > > using the > > > :ref:`VIDIOC_SUBDEV_S_SELECTION ` IOCTL > > > using ``V4L2_SEL_TGT_COMPOSE`` selection target on the same pad. If the > > > subdev supports scaling but not composing, the top and left values are > > > not used and must always be set to zero. > > > > Right, this sentence does imply that when scaling is supported, there > > must be a sink compose rectangle, even when composing is not. > > > > I have previously set up scaling like this: > > > > media-ctl --set-v4l2 "'ipu1_csi0_mux':2[fmt:UYVY2X8/1920x1080@1/60]" > > media-ctl --set-v4l2 "'ipu1_csi0':2[fmt:AYUV32/960x540@1/30]" > > > > Does this mean, it should work like this instead? > > > > media-ctl --set-v4l2 "'ipu1_csi0_mux':2[fmt:UYVY2X8/1920x1080@1/60]" > > media-ctl --set-v4l2 > > "'ipu1_csi0':0[fmt:UYVY2X8/1920x1080@1/60,compose:(0,0)/960x540]" > > media-ctl --set-v4l2 "'ipu1_csi0':2[fmt:AYUV32/960x540@1/30]" > > > > I suppose setting the source pad format should not be allowed to modify > > the sink compose rectangle. > > That is what I believe having read these documents several times, but > we need v4l2 people to confirm. > > Note that setting the format on 'ipu1_csi0':0 should already be done by > the previous media-ctl command, so it should be possible to simplify > that to: > > media-ctl --set-v4l2 "'ipu1_csi0_mux':2[fmt:UYVY2X8/1920x1080@1/60]" > media-ctl --set-v4l2 "'ipu1_csi0':0[compose:(0,0)/960x540]" > media-ctl --set-v4l2 "'ipu1_csi0':2[fmt:AYUV32/960x540@1/30]" Thanks, that works, too. > I have tripped over a bug in media-ctl when specifying both a crop and > compose rectangle - the --help output suggests that "," should be used > to separate them. media-ctl rejects that, telling me the character at > the "," should be "]". Replacing the "," with " " allows media-ctl to > accept it and set both rectangles, so it sounds like a parser bug - I've > not looked into this any further yet. I can confirm this. I don't see any place in v4l2_subdev_parse_pad_format that handles the "," separator. There's just whitespace skipping between the v4l2-properties. regards Philipp ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
