Re: [PATCH] imx-drm: imx-hdmi: fix hdmi hotplug detection initial state
On Tue, Jun 10, 2014 at 04:13:06PM +0100, Russell King - ARM Linux wrote: where 'M' is the IPU DI clock muxer. However, we're currently setting this up as: LM --+ LDB serial `- /7 -+ LDB DI clock IPM --- /N IM --- IPU DI clock and hoping that the LDB and IPU DI clocks are appropriately synchronised. I've just found that we do indeed do this - but there's nothing to switch the configuration back when the LDB is no longer using a particular DI. Also, I'm having a hard time working out why we have the LDB being given all sorts of clocks... LM --+- LDB serial (clks 33, aka di0_pll) `- /7 -+- LDB DI clock(clks 135, aka di0) `- IM --- IPU DI clock(clks 39, aka di0_sel) The LDB is given all of these to play with, including reprogramming the IM, and there's nothing which ever programs IM to anything but the LDB DI clock once it's set there. Not only does this feel horribly unclean from the DT perspective, but it's also a horrid violation of reasonable layering. What if we wanted to fix this by adding control of di0_sel to the HDMI interface too? We then need to list yet again the IPU DI clock and the desired input clock there, and make the imx-hdmi code aware of that. Wouldn't it be better to have the ipuv3-crtc, or even the IPU DI code be in control of its external clock mux, and request the IPU DI code to select a particular input clock? In other words, have one central place where the IPU DI clock is controlled, rather than ending up with it spread through lots of different sub-drivers? -- FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] imx-drm: imx-hdmi: fix hdmi hotplug detection initial state
On Tue, Jun 10, 2014 at 05:14:21PM +0100, Russell King - ARM Linux wrote: On Tue, Jun 10, 2014 at 04:13:06PM +0100, Russell King - ARM Linux wrote: where 'M' is the IPU DI clock muxer. However, we're currently setting this up as: LM --+ LDB serial `- /7 -+ LDB DI clock IPM --- /N IM --- IPU DI clock and hoping that the LDB and IPU DI clocks are appropriately synchronised. I've just found that we do indeed do this - but there's nothing to switch the configuration back when the LDB is no longer using a particular DI. Also, I'm having a hard time working out why we have the LDB being given all sorts of clocks... LM --+- LDB serial(clks 33, aka di0_pll) `- /7 -+- LDB DI clock (clks 135, aka di0) `- IM --- IPU DI clock (clks 39, aka di0_sel) The LDB is given all of these to play with, including reprogramming the IM, and there's nothing which ever programs IM to anything but the LDB DI clock once it's set there. *Sigh*... is the clock tree represented in Linux even correct? --|\ --| | --| |--+- --| | ^ ldb_di0_sel | --|/ (clks 33) | `-- /3.5 /2 -- G -+-- ^ ^ ldb_di0_podf | ^ ldb_di0 ldb_di0_div_3_5 | .--' | '--|\ (ldb_di1)| | (ipp_di0)| |- G (ipp_di1)| | ^ ^ ipu1_di0 (ipu1_di0_pre)|/ ipu1_di0_sel This diagram is drawn from the code in clk-imx6.c, and it does not agree with what is in the SoC manuals - this is the representation redrawn from the manuals: --|\ --| | --| |--+-- G --| | ^ ldb_di0_sel |^ ldb serial --|/ (clks 33) | `-- /3.5 /2 -+--- ^ ^ ldb_di0_podf | ^ ldb di ldb_di0_div_3_5| .-' | '--|\ (ldb_di1)| | (ipp_di0)| |- G (ipp_di1)| | ^ ^ ipu1_di0 (ipu1_di0_pre)|/ ipu1_di0_sel The difference is, there is no clock gate between the LDB DI clock and the /7 divider, but there is a clock gate on the LDB serial clock. In another location, the iMX6QDL manual suggests that it may be more like this: --|\ --| | --| |--- cg ---+- --| | ^ ldb_di0_sel |^ ldb serial --|/ (clks 33) | `-- /3.5 /2 -+--- ^ ^ ldb_di0_podf | ^ ldb di ldb_di0_div_3_5| .-' | '--|\ (ldb_di1)| | (ipp_di0)| | (ipp_di1)| | ^ ^ ipu1_di0 (ipu1_di0_pre)-- cg --|/ ipu1_di0_sel although cg is not defined what it is. Another place seems to confirm the above diagram, saying that the ldb_di0_clk_enable gating bits controls both ch_0_serial_clk (presumably the ldb serial clock) and di_0_clk_nc (presumably the ldb di clock. If that's correct cg refers to the clock gating via the CCM_CCGR registers, which appear in the CCM clock tree diagram under LPCG. So... I wonder which one of these three is actually the right one... -- FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v14 01/10] [media] v4l2: add new V4L2_PIX_FMT_RGB666 pixel format.
On Mon, Jun 16, 2014 at 12:11:15PM +0200, Denis Carikli wrote: That new macro is needed by the imx_drm staging driver for supporting the QVGA display of the eukrea-cpuimx51 board. As I said probably around v10 time, I already have this patch queued, and I was going to send it to Greg before the previous merge window, but due to the number of patches I was already carrying, it was lost amongst the trees. -- FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v14 02/10] imx-drm: Add RGB666 support for parallel display.
On Mon, Jun 16, 2014 at 12:11:16PM +0200, Denis Carikli wrote: Signed-off-by: Denis Carikli de...@eukrea.com Acked-by: Philipp Zabel p.za...@pengutronix.de As I said probably around v10 time, I already have this patch queued, and I was going to send it to Greg before the previous merge window, but due to the number of patches I was already carrying, it was lost amongst the trees. -- FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v14 03/10] imx-drm: Correct BGR666 and the board's dts that use them.
On Mon, Jun 16, 2014 at 12:11:17PM +0200, Denis Carikli wrote: The current BGR666 is not consistent with the other color mapings like BGR24. BGR666 should be in the same byte order than BGR24. Signed-off-by: Denis Carikli de...@eukrea.com Acked-by: Philipp Zabel p.za...@pengutronix.de As I said probably around v10 time, I already have this patch queued, and I was going to send it to Greg before the previous merge window, but due to the number of patches I was already carrying, it was lost amongst the trees. -- FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] imx-drm: imx-hdmi: fix hdmi hotplug detection initial state
On Mon, Jun 16, 2014 at 11:13:02AM -0300, Fabio Estevam wrote: On Wed, Jun 11, 2014 at 5:17 AM, Russell King - ARM Linux li...@arm.linux.org.uk wrote: The problem here is that we need more inteligence from CCF in order to do that - we need it to be able to reprogram the dividers so that the IPU DI0 clock remains at 148.5MHz while increasing the output of pll5_video_div three-fold. Another solution would be to source the LDB clock from PLL3 at 480MHz, this gives a pixel clock of 68.6MHz (3sf). The other options are Ok, I have tried this approach: --- a/arch/arm/mach-imx/clk-imx6q.c +++ b/arch/arm/mach-imx/clk-imx6q.c @@ -441,8 +441,8 @@ static void __init imx6q_clocks_init(struct device_node *ccm if ((imx_get_soc_revision() != IMX_CHIP_REVISION_1_0) || cpu_is_imx6dl()) { - clk_set_parent(clk[ldb_di0_sel], clk[pll5_video_div]); - clk_set_parent(clk[ldb_di1_sel], clk[pll5_video_div]); + clk_set_parent(clk[ldb_di0_sel], clk[pll3_usb_otg]); + clk_set_parent(clk[ldb_di1_sel], clk[pll3_usb_otg]); } and it allows HDMI and LVDS to be displayed if I boot with the HDMI kernel connected. Would this be an acceptable solution in the meantime? I have no objection to that as an interim solution, but it does leave me wondering whether this causes LDB to change the USB OTG clocks. Might it be worth printing something, just in case someone finds USB OTG breaks and wonders why? -- FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v14 06/10] drm: drm_display_mode: add signal polarity flags
On Mon, Jun 16, 2014 at 12:11:20PM +0200, Denis Carikli wrote: We need a way to pass signal polarity informations between DRM panels, and the display drivers. To do that, a pol_flags field was added to drm_display_mode. Signed-off-by: Denis Carikli de...@eukrea.com This patch needs an ack from the DRM people - can someone review it please? This series has now been round 14 revisions and it's about time it was properly reviewed - or a statement made if it's unacceptable. --- ChangeLog v13-v14: - Fixed DRM_MODE_FLAG_POL_DE_HIGH's description. ChangeLog v12-v13: - Added Docbook documentation for pol_flags the struct field. - Removed the _PRESERVE defines: it was used by patches against the imx_drm driver. Now theses patches have been adapted not to require that defines. ChangeLog v11-v12: - Rebased: This patch now applies against drm_modes.h - Rebased: It now uses the new DRM_MODE_FLAG_POL_DE flags defines names ChangeLog v10-v11: - Since the imx-drm won't be able to retrive its regulators from the device tree when using display-timings nodes, and that I was told that the drm simple-panel driver already supported that, I then, instead, added what was lacking to make the eukrea displays work with the drm-simple-panel driver. That required a way to get back the display polarity informations from the imx-drm driver without affecting userspace. --- Documentation/DocBook/drm.tmpl | 30 ++ include/drm/drm_modes.h|6 ++ 2 files changed, 36 insertions(+) diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl index 7df3134..22d435f 100644 --- a/Documentation/DocBook/drm.tmpl +++ b/Documentation/DocBook/drm.tmpl @@ -2292,6 +2292,36 @@ void intel_crt_init(struct drm_device *dev) and structfieldheight_mm/structfield fields are only used internally during EDID parsing and should not be set when creating modes manually. /para + para +The structfieldpol_flags/structfield value represents the display +signal polarity flags, it can be a combination of +variablelist + varlistentry +termDRM_MODE_FLAG_POL_PIXDATA_NEGEDGE/term + listitempara + drive pixel data on falling edge, sample data on rising edge. + /para/listitem + /varlistentry + varlistentry +termDRM_MODE_FLAG_POL_PIXDATA_POSEDGE/term +listitempara + Drive pixel data on rising edge, sample data on falling edge. +/para/listitem + /varlistentry + varlistentry +termDRM_MODE_FLAG_POL_DE_LOW/term +listitempara + data-enable pulse is active low +/para/listitem + /varlistentry + varlistentry +termDRM_MODE_FLAG_POL_DE_HIGH/term +listitempara + data-enable pulse is active high +/para/listitem + /varlistentry +/variablelist + /para /listitem listitem synopsisint (*mode_valid)(struct drm_connector *connector, diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h index 91d0582..c5cbe31 100644 --- a/include/drm/drm_modes.h +++ b/include/drm/drm_modes.h @@ -93,6 +93,11 @@ enum drm_mode_status { #define DRM_MODE_FLAG_3D_MAX DRM_MODE_FLAG_3D_SIDE_BY_SIDE_HALF +#define DRM_MODE_FLAG_POL_PIXDATA_NEGEDGEBIT(1) +#define DRM_MODE_FLAG_POL_PIXDATA_POSEDGEBIT(2) +#define DRM_MODE_FLAG_POL_DE_LOW BIT(3) +#define DRM_MODE_FLAG_POL_DE_HIGHBIT(4) + struct drm_display_mode { /* Header */ struct list_head head; @@ -144,6 +149,7 @@ struct drm_display_mode { int vrefresh; /* in Hz */ int hsync; /* in kHz */ enum hdmi_picture_aspect picture_aspect_ratio; + unsigned int pol_flags; }; /* mode specified on the command line */ -- 1.7.9.5 -- FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v14 08/10] drm/panel: Add Eukrea mbimxsd51 displays.
Denis, This patch creates binding documentation. Any patch which does so should be copied to the DT people so they can review the bindings and give appropriate acks. It would be better if you separate the binding documentation updates from the other functional changes too. I've added them on this reply to see whether they'll feel friendly enough to comment on the patch as it stands to avoid having to go through two more rounds on this already-fourteen revision patch set. On Mon, Jun 16, 2014 at 12:11:22PM +0200, Denis Carikli wrote: Signed-off-by: Denis Carikli de...@eukrea.com --- ChangeLog v13-v14: - None ChangeLog v12-v13: - Added a note explaining why the size is zero in the eukrea_mbimxsd51_dvi(s)vga structs. ChangeLog v11-v12: - Rebased: It now uses the new DRM_MODE_FLAG_POL_DE flags defines names ChangeLog v10-v11: - New patch. --- .../bindings/panel/eukrea,mbimxsd51-cmo-qvga.txt |7 ++ .../bindings/panel/eukrea,mbimxsd51-dvi-svga.txt |7 ++ .../bindings/panel/eukrea,mbimxsd51-dvi-vga.txt|7 ++ drivers/gpu/drm/panel/panel-simple.c | 83 4 files changed, 104 insertions(+) create mode 100644 Documentation/devicetree/bindings/panel/eukrea,mbimxsd51-cmo-qvga.txt create mode 100644 Documentation/devicetree/bindings/panel/eukrea,mbimxsd51-dvi-svga.txt create mode 100644 Documentation/devicetree/bindings/panel/eukrea,mbimxsd51-dvi-vga.txt diff --git a/Documentation/devicetree/bindings/panel/eukrea,mbimxsd51-cmo-qvga.txt b/Documentation/devicetree/bindings/panel/eukrea,mbimxsd51-cmo-qvga.txt new file mode 100644 index 000..03679d0 --- /dev/null +++ b/Documentation/devicetree/bindings/panel/eukrea,mbimxsd51-cmo-qvga.txt @@ -0,0 +1,7 @@ +Eukrea CMO-QVGA (320x240 pixels) TFT LCD panel + +Required properties: +- compatible: should be eukrea,mbimxsd51-cmo-qvga + +This binding is compatible with the simple-panel binding, which is specified +in simple-panel.txt in this directory. diff --git a/Documentation/devicetree/bindings/panel/eukrea,mbimxsd51-dvi-svga.txt b/Documentation/devicetree/bindings/panel/eukrea,mbimxsd51-dvi-svga.txt new file mode 100644 index 000..f408c9a --- /dev/null +++ b/Documentation/devicetree/bindings/panel/eukrea,mbimxsd51-dvi-svga.txt @@ -0,0 +1,7 @@ +Eukrea DVI-SVGA (800x600 pixels) DVI output. + +Required properties: +- compatible: should be eukrea,mbimxsd51-dvi-svga + +This binding is compatible with the simple-panel binding, which is specified +in simple-panel.txt in this directory. diff --git a/Documentation/devicetree/bindings/panel/eukrea,mbimxsd51-dvi-vga.txt b/Documentation/devicetree/bindings/panel/eukrea,mbimxsd51-dvi-vga.txt new file mode 100644 index 000..8ea90da --- /dev/null +++ b/Documentation/devicetree/bindings/panel/eukrea,mbimxsd51-dvi-vga.txt @@ -0,0 +1,7 @@ +Eukrea DVI-VGA (640x480 pixels) DVI output. + +Required properties: +- compatible: should be eukrea,mbimxsd51-dvi-vga + +This binding is compatible with the simple-panel binding, which is specified +in simple-panel.txt in this directory. diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c index a251361..adc40a7 100644 --- a/drivers/gpu/drm/panel/panel-simple.c +++ b/drivers/gpu/drm/panel/panel-simple.c @@ -403,6 +403,80 @@ static const struct panel_desc edt_etm0700g0dh6 = { }, }; +static const struct drm_display_mode eukrea_mbimxsd51_cmoqvga_mode = { + .clock = 6500, + .hdisplay = 320, + .hsync_start = 320 + 38, + .hsync_end = 320 + 38 + 20, + .htotal = 320 + 38 + 20 + 30, + .vdisplay = 240, + .vsync_start = 240 + 15, + .vsync_end = 240 + 15 + 4, + .vtotal = 240 + 15 + 4 + 3, + .vrefresh = 60, + .pol_flags = DRM_MODE_FLAG_POL_PIXDATA_NEGEDGE | + DRM_MODE_FLAG_POL_DE_LOW, +}; + +static const struct panel_desc eukrea_mbimxsd51_cmoqvga = { + .modes = eukrea_mbimxsd51_cmoqvga_mode, + .num_modes = 1, + .size = { + .width = 73, + .height = 56, + }, +}; + +static const struct drm_display_mode eukrea_mbimxsd51_dvisvga_mode = { + .clock = 44333, + .hdisplay = 800, + .hsync_start = 800 + 112, + .hsync_end = 800 + 112 + 32, + .htotal = 800 + 112 + 32 + 80, + .vdisplay = 600, + .vsync_start = 600 + 3, + .vsync_end = 600 + 3 + 17, + .vtotal = 600 + 3 + 17 + 4, + .vrefresh = 60, + .pol_flags = DRM_MODE_FLAG_POL_PIXDATA_POSEDGE | + DRM_MODE_FLAG_POL_DE_HIGH, +}; + +static const struct panel_desc eukrea_mbimxsd51_dvisvga = { + .modes = eukrea_mbimxsd51_dvisvga_mode, + .num_modes = 1, + /* This is a DVI adapter for external displays */ + .size = { + .width = 0, + .height = 0, + }, +}; + +static const struct drm_display_mode eukrea_mbimxsd51_dvivga_mode = { +
Re: [PATCH v14 04/10] imx-drm: use defines for clock polarity settings
On Mon, Jun 16, 2014 at 12:11:18PM +0200, Denis Carikli wrote: Signed-off-by: Denis Carikli de...@eukrea.com It would be nice to have a little more explanation in the commit messages for these patches. If you'd like to send me better commit messages for these patches, I'll add them to what I already have: imx-drm: use defines for clock polarity settings imx-drm: add RGB666 support for parallel display. It may also be worth describing the RGB666 format in the commit message for: v4l2: add new V4L2_PIX_FMT_RGB666 pixel format. And... getting some more acks for these patches would be very useful, I think I'd like to see Sascha's ack for these... Sascha? -- FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v14 04/10] imx-drm: use defines for clock polarity settings
On Tue, Jun 24, 2014 at 06:25:19PM +0200, Denis Carikli wrote: On 06/24/2014 05:13 PM, Russell King - ARM Linux wrote: [...] If you'd like to send me better commit messages for these patches, I'll add them to what I already have: imx-drm: use defines for clock polarity settings The comment of the clk_pol field of the ipu_di_signal_cfg struct was inverted. Instead of merely inverting the comment, the values of clk_pol were defined. s/inverting/fixing/ imx-drm: add RGB666 support for parallel display. This permits to drive parallel displays that expect the RGB666 color format. This allows imx-drm to drive ... It may also be worth describing the RGB666 format in the commit message for: v4l2: add new V4L2_PIX_FMT_RGB666 pixel format. The RGB666 color format encodes 6 bits for each color(red, green and blue), linearly. It looks like this in memory: 017 RRGGBB Thanks! I've tweaked them very slightly as detailed above so they read a bit better. -- FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v14 04/10] imx-drm: use defines for clock polarity settings
On Wed, Jun 25, 2014 at 06:48:45AM +0200, Sascha Hauer wrote: On Mon, Jun 16, 2014 at 12:11:18PM +0200, Denis Carikli wrote: + /* * Bitfield of Display Interface signal polarities. */ @@ -37,7 +43,7 @@ struct ipu_di_signal_cfg { unsigned clksel_en:1; unsigned clkidle_en:1; unsigned data_pol:1;/* true = inverted */ - unsigned clk_pol:1; /* true = rising edge */ + unsigned clk_pol:1; unsigned enable_pol:1; unsigned Hsync_pol:1; /* true = active high */ unsigned Vsync_pol:1; ...can we rename the flags to more meaningful names instead? unsigned clk_pol_rising_edge:1; unsigned enable_pol_high:1; unsigned hsync_active_high:1; unsigned vsync_active_high:1; Now look at patch 7, where these become tri-state: - don't change - rising edge/active high - falling edge/active low So your suggestion is not a good idea. -- FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v14 07/10] imx-drm: Use drm_display_mode timings flags.
On Mon, Jun 16, 2014 at 12:11:21PM +0200, Denis Carikli wrote: The previous hardware behaviour was kept if the flags are not set. I'd like to throw in a patch that I've been carrying for a bit now. It conflicts with your patches, but I'm happy to fix that conflict locally (and have been doing so for a while now.) This is related to a slightly different issue - knowing which types of bridges are bound to a particualar DI. This matters in part for selecting the clock routing - as things currently stand, the last bridge to call imx_drm_panel_format*() gets its way with this. With this change, we can see which bridges are bound, and make the appropriate decision. At the moment, we are saved from things going awry as we don't support cloning outputs. The relevence to your patch set is that some bridges require clk_pol to be configured appropriately - HDMI requires clk_pol = 0 in order to work correctly (with the opposite edge, the image is noisy.) While this approach only allows us to identify the types of bridges connected to a DI rather than uniquely identifing the bridges themselves, I think this is not only an improvement, but also a simplification of the current code, and allows better decisions about things like clk_pol to be made. I'm sending it here because it is relevent to Denis' patch set - I will also send it out separately if people want it separately, though that will go to a reduced Cc list. From: Russell King rmk+ker...@arm.linux.org.uk Subject: [PATCH] imx-drm: core: handling of DI clock flags to ipu_crtc_mode_set() We do not need to track the state of the IPU DI's clock flags by having each display bridge calling back into imx-drm-core, and then back out into ipuv3-crtc.c. ipuv3-crtc can instead just scan the list of encoders to retrieve their type, and build up a picture of which types of encoders are attached. We can then use this information to configure the IPU DI clocking mode without any uncertainty - if we have multiple bridges connected to the same DI, if one of them requires a synchronous DI clock, that's what we must use. Signed-off-by: Russell King rmk+ker...@arm.linux.org.uk --- drivers/staging/imx-drm/imx-drm-core.c | 3 +-- drivers/staging/imx-drm/imx-drm.h | 2 +- drivers/staging/imx-drm/ipuv3-crtc.c | 40 +++--- 3 files changed, 25 insertions(+), 20 deletions(-) diff --git a/drivers/staging/imx-drm/imx-drm-core.c b/drivers/staging/imx-drm/imx-drm-core.c index def8280d7ee6..6d9376c760ad 100644 --- a/drivers/staging/imx-drm/imx-drm-core.c +++ b/drivers/staging/imx-drm/imx-drm-core.c @@ -115,8 +115,7 @@ int imx_drm_panel_format_pins(struct drm_encoder *encoder, helper = imx_crtc-imx_drm_helper_funcs; if (helper-set_interface_pix_fmt) return helper-set_interface_pix_fmt(encoder-crtc, - encoder-encoder_type, interface_pix_fmt, - hsync_pin, vsync_pin); + interface_pix_fmt, hsync_pin, vsync_pin); return 0; } EXPORT_SYMBOL_GPL(imx_drm_panel_format_pins); diff --git a/drivers/staging/imx-drm/imx-drm.h b/drivers/staging/imx-drm/imx-drm.h index 7453ae00c412..3c559ccd6af0 100644 --- a/drivers/staging/imx-drm/imx-drm.h +++ b/drivers/staging/imx-drm/imx-drm.h @@ -17,7 +17,7 @@ int imx_drm_crtc_id(struct imx_drm_crtc *crtc); struct imx_drm_crtc_helper_funcs { int (*enable_vblank)(struct drm_crtc *crtc); void (*disable_vblank)(struct drm_crtc *crtc); - int (*set_interface_pix_fmt)(struct drm_crtc *crtc, u32 encoder_type, + int (*set_interface_pix_fmt)(struct drm_crtc *crtc, u32 pix_fmt, int hsync_pin, int vsync_pin); const struct drm_crtc_helper_funcs *crtc_helper_funcs; const struct drm_crtc_funcs *crtc_funcs; diff --git a/drivers/staging/imx-drm/ipuv3-crtc.c b/drivers/staging/imx-drm/ipuv3-crtc.c index 7fec438d8c54..af09032aedb0 100644 --- a/drivers/staging/imx-drm/ipuv3-crtc.c +++ b/drivers/staging/imx-drm/ipuv3-crtc.c @@ -51,7 +51,6 @@ struct ipu_crtc { struct drm_framebuffer *newfb; int irq; u32 interface_pix_fmt; - unsigned long di_clkflags; int di_hsync_pin; int di_vsync_pin; }; @@ -146,10 +145,13 @@ static int ipu_crtc_mode_set(struct drm_crtc *crtc, int x, int y, struct drm_framebuffer *old_fb) { + struct drm_device *dev = crtc-dev; + struct drm_encoder *encoder; struct ipu_crtc *ipu_crtc = to_ipu_crtc(crtc); - int ret; struct ipu_di_signal_cfg sig_cfg = {}; + unsigned long encoder_types = 0; u32 out_pixel_fmt; + int ret; dev_dbg(ipu_crtc-dev, %s: mode-hdisplay: %d\n, __func__, mode-hdisplay); @@ -165,6 +167,24 @@ static int
Re: [PATCH RFC v2 3/8] component: add support for component match array
On Thu, Jun 26, 2014 at 02:34:17PM +0200, Philipp Zabel wrote: Hi Russell, On Tue, Jun 24, 2014 at 9:29 PM, Russell King rmk+ker...@arm.linux.org.uk wrote: [...] +/* + * Add a component to be matched. + * + * The match array is first created or extended if necessary. + */ +void component_match_add(struct device *dev, struct component_match **matchptr, + int (*compare)(struct device *, void *), void *compare_data) +{ + struct component_match *match = *matchptr; + + if (IS_ERR(match)) + return; + + if (!match || match-num == match-alloc) { + size_t new_size = match ? match-alloc + 16 : 15; + + match = component_match_realloc(dev, match, new_size); + + *matchptr = match; + + if (IS_ERR(match)) + return; + } + + match-compare[match-num].fn = compare; + match-compare[match-num].data = compare_data; + match-num++; +} component_match_add should be exported. Fixed, thanks. -- FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v14 04/10] imx-drm: use defines for clock polarity settings
On Wed, Jun 25, 2014 at 11:44:47AM +0200, Denis Carikli wrote: On 06/25/2014 06:48 AM, Sascha Hauer wrote: +#define ENABLE_POL_LOW 0 +#define ENABLE_POL_HIGH1 Adding defines without a proper namespace (IPU_) outside a driver private header file is not nice. Anyway, instead of adding the defines ... Fixed in imx-drm: use defines for clock polarity settings and in imx-drm: Use drm_display_mode timings flags.. Denis, can you send just this one updated patch, so I can update the one I have here with this change. Once you've done that, I'll send the first four off to Greg. Thanks. -- FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 0/2] imx-drm: two unbind bugs
The following two patches fix a couple of oopses I've found while re-testing the unbind support for imx-drm. Can I suggest that people check that it's possible to successfully unbind and rebind the driver when they add stuff (like the ipu plane support)? I have these queued for Greg to pull. Thanks. drivers/staging/imx-drm/imx-ldb.c | 3 +++ drivers/staging/imx-drm/ipuv3-plane.c | 3 ++- 2 files changed, 5 insertions(+), 1 deletion(-) -- FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH RFC v2 3/8] component: add support for component match array
On Thu, Jun 26, 2014 at 03:46:01PM +0100, Russell King - ARM Linux wrote: On Thu, Jun 26, 2014 at 02:34:17PM +0200, Philipp Zabel wrote: Hi Russell, On Tue, Jun 24, 2014 at 9:29 PM, Russell King rmk+ker...@arm.linux.org.uk wrote: [...] +/* + * Add a component to be matched. + * + * The match array is first created or extended if necessary. + */ +void component_match_add(struct device *dev, struct component_match **matchptr, + int (*compare)(struct device *, void *), void *compare_data) +{ + struct component_match *match = *matchptr; + + if (IS_ERR(match)) + return; + + if (!match || match-num == match-alloc) { + size_t new_size = match ? match-alloc + 16 : 15; + + match = component_match_realloc(dev, match, new_size); + + *matchptr = match; + + if (IS_ERR(match)) + return; + } + + match-compare[match-num].fn = compare; + match-compare[match-num].data = compare_data; + match-num++; +} component_match_add should be exported. Fixed, thanks. As there's no further comments, and Inki Dae has not responded, I'm going to send these out without the RFC tag in the hope that people will provide acks. This allows us to move forward with this despite the Exynos DRM blockage. The ultimate plan is for patches 1 to 3 inclusive to be merged into Greg's driver tree, 1 to 3 and 5 into Greg's staging tree, and 1 to 3 and 4 for David Airlie's DRM tree - patches 1 to 3 are needed for both patches 4 and 5. -- FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v3 0/8] component helper improvements
A while back, Laurent raised some comments about the component helper, which this patch set starts to address. The first point it addresses is the repeated parsing inefficiency when deferred probing occurs. When DT is used, the structure of the component helper today means that masters end up parsing the device tree for each attempt to re-bind the driver. We remove this inefficiency by creating an array of matching data and functions, which the component helper can use internally to match up components to their master. The second point was the inefficiency of destroying the list of components each time we saw a failure. We did this to ensure that we kept things correctly ordered: component bind order matters. As we have an array instead, the array is already ordered, so we use this array to store the component pointers instead of a list, and remember which are duplicates (and thus should be avoided.) Avoiding the right duplicates matters as we walk the array in the opposite direction at tear down. I hope that this is the final submission in patch form. Ultimately, these patches need to be handled carefully: Patch Trees 1, 2, 3 Driver, Staging, DRM 4 Driver, DRM 5 Driver, Staging 6, 7, 8 To be held back until all users are updated (Exynos DRM) drivers/base/component.c | 249 ++--- drivers/gpu/drm/msm/msm_drv.c | 83 +-- drivers/staging/imx-drm/imx-drm-core.c | 57 +--- include/linux/component.h | 8 +- 4 files changed, 208 insertions(+), 189 deletions(-) -- FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it. ___ linux-arm-kernel mailing list linux-arm-ker...@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [RFC PATCH 0/8] component helper improvements
On Wed, May 14, 2014 at 08:42:17PM +0200, Thierry Reding wrote: I've been looking at converting the Tegra DRM driver to the component helpers for a while now and had to make some changes to make it work for that particular use-case. While updating the imx-drm and msm DRM drivers for those changes I noticed an oddity. Both of the existing drivers use the following pattern: static int driver_component_bind(struct device *dev, struct device *master, void *data) { allocate memory request resources ... hook up to subsystem ... enable hardware } static const struct component_ops driver_component_ops = { .bind = driver_component_bind, }; static int driver_probe(struct platform_device *pdev) { return component_add(pdev-dev, driver_component_ops); } While converting Tegra DRM, what I intuitively did (I didn't actually look at the other drivers for inspiration) was something more along the lines of the following: static int driver_component_bind(struct device *dev, struct device *master, void *data) { hook up to subsystem ... enable hardware } static const struct component_ops driver_component_ops = { .bind = driver_component_bind, }; static int driver_probe(struct platform_device *pdev) { allocate memory request resources ... return component_add(pdev-dev, driver_component_ops); } Since usually deferred probing is caused by resource allocations failing this has the side-effect of handling deferred probing before the master device is even bound (the component_add() happens as the very last step) and therefore there is less risk for component_bind_all() to fail. I've actually never seen it fail at all. Failure at that point is almost certainly irrecoverable anyway. It isn't irrecoverable - that case is handled. I really don't like two-stage driver initialisation - it increases the chances of bugs creeping in. Take for example this code: probe() { priv = devm_kzalloc(dev, whatever); priv-mem = devm_ioremap_resource(dev, res); dev_set_drvdata(dev, priv); return component_add(dev, ops); } So far so good, not much can go wrong at that point - we know exactly what state the 'priv' structure is at the point where the component_add call is made. Now, when the ops' bind method is called, we retrieve the private data. At this point, we can no longer rely on the initialisation state of many of the members. We can't assume that they were zero when we're called, because we can have this sequence of events: - driver is probed - component is bound - component is unbound - component is bound At this point, the private data will be dirty. This actually makes the use of devm_kzalloc() a joke in the probe function - although it does initialise all members to zero, we can't rely on that at all when the component is bound. While the driver itself may be coded for this to be safe, can we say the same for any structures which are embedded into the private data, which may be private to other subsystems? By way of illustration, ASoC can also have this two stage approach. I'll draw your attention to SGTL5000, and the recent patch I submitted (which I don't think will be taken.) This driver suffers badly if the ASoC card is bound, then unbound, and an attempt to rebind it again. That's because the driver gets some managed resources in both the first stage and the second stage, and expects them to be automatically released in when the second stage is torn down. This bug has existed for a very long time, and has gone unnoticed (it will be unnoticed until you try to debug by removing modules and trying to load replacements, which is how I found it.) That exact bug can't happen with the component helpers, because I explicitly thought about the handling of managed resources, and added the necessary support to deal with these correctly. However, it serves as an example that, despite comments from people saying that my fear is unlikely to happen, we already have code which suffers from issues with two-stage initialisation. The unfortunate thing is that validation testing for Linux tends not to venture much past does it boot, are my devices present and can I run some programs. It doesn't cover system shutdown/reboot very often (we've had bugs which have been present for ages there - my test farm explicitly does a power off after boot testing now) and it hardly ever covers drivers being unbound or module removal. -- FTTC broadband for 0.8mile line: now at 9.7Mbps
Re: [PATCH RFC v2 3/8] component: add support for component match array
On Thu, Jul 03, 2014 at 12:26:39AM +0900, Inki Dae wrote: It's has been just a week. I will check and look into your patch series. I think Exynos drm should also be considered for the use of component match array. Actually, this series has been around for much longer than just a week. Your new usage introduced in the recent merge window is what has resulted in you becoming aware of this series. It was developed before April through discussions, and then shared with those people. Then, April 27th, it was posted publically to all the recipients except yourself (because Exynos hadn't visibly been converted). At that time, two reviewed-by tags were given. It was also sent out last week, with yourself added to the recepient list because there is now a dependency with some of your work. However, what I'm asking for is *not* the entire series to be merged at this point - that would break Exynos DRM. I'm asking for the first three to be merged initially by Greg, which gets the new interface in place without breaking existing users. We can then convert existing users at a slower rate, and remove the old interface once everyone has caught up. So, I'd ask that you give priority to looking at the first three patches and deciding whether you find them acceptable as a replacement interface, rather than trying to review the entire set of six core patches (1,2,3,6,7,8). What I'm trying to avoid here is for all these patches to be delayed past the next merge window, and pushed into the next cycle. That's likely to end up in the same scenario as exists with Exynos DRM today, only with other new users of the existing interface - and then have to repeat this whole try to get the new users to review this set of changes cycle again. We're half way through -rc3 right now. -final occurs anytime between -rc6 and -rc9, which could be just three and a half weeks away. I have other changes which I need to get out onto the list(s) which depend on this too (for DRM) which I'm hoping to also make this coming merge window, but I can't start that process until I know where I stand with these. -- FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 0/8] component helper improvements
On Thu, Jul 03, 2014 at 01:51:19AM +0200, Laurent Pinchart wrote: On Wednesday 02 July 2014 15:59:04 Russell King - ARM Linux wrote: On Tue, Jul 01, 2014 at 03:40:11PM +0100, Russell King - ARM Linux wrote: A while back, Laurent raised some comments about the component helper, which this patch set starts to address. I looked back over the two other times which this series has posted, and noticed that two patches had been reviewed, so I've added those tags. Unless there's any objections from anyone, I'll send the first three off to Greg either tonight or tomorrow night, which at least gets us moving forward on this. If anyone has any objections, please shout ASAP. Thanks. No objection from my side at all, this is a nice improvement. For the first three patches, Acked-by: Laurent Pinchart laurent.pinch...@ideasonboard.com Thanks, ack added. -- FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [GIT PULL] component helper updates for 3.17
On Thu, Jul 03, 2014 at 01:20:03PM -0700, Greg Kroah-Hartman wrote: On Thu, Jul 03, 2014 at 03:10:40PM +0100, Russell King wrote: Greg, Please incorporate the latest component updates, which can be found at: git://ftp.arm.linux.org.uk/~rmk/linux-arm.git component-for-driver with SHA1 6955b58254c2bcee8a7b55ce06468a645dc98ec5. Pulled into my driver-core-next branch of my driver-core.git tree and pushed out. Many thanks! I'll send the other two (one for staging, and one for DRM) tomorrow. -- FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 3/8] component: add support for component match array
On Fri, Jul 04, 2014 at 04:17:35PM +0530, Sachin Kamat wrote: Hi Russell +int component_master_add_with_match(struct device *dev, + const struct component_master_ops *ops, + struct component_match *match) { struct master *master; int ret; + if (ops-add_components match) + return -EINVAL; + + /* Reallocate the match array for its true size */ + match = component_match_realloc(dev, match, match-num); ^ This gives a NULL pointer dereference error when match is NULL (as passed by component_master_add() below). Observed this while testing linux-next kernel (next-20140704) on Exynos based board with DRM enabled. Thanks for your report. Please verify that the patch below resolves it for you. Thanks. drivers/base/component.c | 10 ++ 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/drivers/base/component.c b/drivers/base/component.c index b4236daed4fa..f748430bb654 100644 --- a/drivers/base/component.c +++ b/drivers/base/component.c @@ -293,10 +293,12 @@ int component_master_add_with_match(struct device *dev, if (ops-add_components match) return -EINVAL; - /* Reallocate the match array for its true size */ - match = component_match_realloc(dev, match, match-num); - if (IS_ERR(match)) - return PTR_ERR(match); + if (match) { + /* Reallocate the match array for its true size */ + match = component_match_realloc(dev, match, match-num); + if (IS_ERR(match)) + return PTR_ERR(match); + } master = kzalloc(sizeof(*master), GFP_KERNEL); if (!master) -- FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 3/8] component: add support for component match array
On Fri, Jul 04, 2014 at 05:00:36PM +0530, Sachin Kamat wrote: On Fri, Jul 4, 2014 at 4:22 PM, Russell King - ARM Linux li...@arm.linux.org.uk wrote: On Fri, Jul 04, 2014 at 04:17:35PM +0530, Sachin Kamat wrote: Hi Russell +int component_master_add_with_match(struct device *dev, + const struct component_master_ops *ops, + struct component_match *match) { struct master *master; int ret; + if (ops-add_components match) + return -EINVAL; + + /* Reallocate the match array for its true size */ + match = component_match_realloc(dev, match, match-num); ^ This gives a NULL pointer dereference error when match is NULL (as passed by component_master_add() below). Observed this while testing linux-next kernel (next-20140704) on Exynos based board with DRM enabled. Thanks for your report. Please verify that the patch below resolves it for you. Thanks. Yes, the below patch fixes the crash. Thanks for the fix. Thanks. I'll add a tested-by and reported-by for your address when committing this patch. Let me know if you want something different. -- FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v5 06/11] imx-drm: imx-tve: Fix DDC I2C bus property
On Wed, Mar 05, 2014 at 10:20:57AM +0100, Philipp Zabel wrote: This patch fixes the TV Encoder DDC I2C bus property to use the common 'ddc-i2c-bus' property name instead of 'ddc'. Looking at both hdmi and tve, the ddc part is very similar. The difference is how the probe is handled: imx-hdmi: ddc_node = of_parse_phandle(np, ddc, 0); if (ddc_node) { hdmi-ddc = of_find_i2c_adapter_by_node(ddc_node); if (!hdmi-ddc) dev_dbg(hdmi-dev, failed to read ddc node\n); of_node_put(ddc_node); } else { dev_dbg(hdmi-dev, no ddc property found\n); } imx-tve: ddc_node = of_parse_phandle(np, ddc, 0); if (ddc_node) { tve-ddc = of_find_i2c_adapter_by_node(ddc_node); of_node_put(ddc_node); } It appears to differ only by debug prints - is there any reason we couldn't unify the DDC backend part? I've tinkered with this idea, and already have a patch, though it needs a little rework. Any thoughts? -- FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v9][ 4/8] imx-drm: Match ipu_di_signal_cfg's clk_pol with its description.
On Thu, Mar 06, 2014 at 05:04:25PM +0100, Denis Carikli wrote: According to the datasheet, setting the di0_polarity_disp_clk field in the GENERAL di register sets the output clock polarity to active high. Signed-off-by: Denis Carikli de...@eukrea.com --- ChangeLog v8-v9: - New patch that is now needed by the staging: imx-drm: Use de-active and pixelclk-active patch. --- drivers/staging/imx-drm/ipu-v3/ipu-di.c |2 +- drivers/staging/imx-drm/ipuv3-crtc.c|2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/staging/imx-drm/ipu-v3/ipu-di.c b/drivers/staging/imx-drm/ipu-v3/ipu-di.c index 82a9eba..849b3e1 100644 --- a/drivers/staging/imx-drm/ipu-v3/ipu-di.c +++ b/drivers/staging/imx-drm/ipu-v3/ipu-di.c @@ -595,7 +595,7 @@ int ipu_di_init_sync_panel(struct ipu_di *di, struct ipu_di_signal_cfg *sig) } } - if (!sig-clk_pol) + if (sig-clk_pol) di_gen |= DI_GEN_POLARITY_DISP_CLK; ipu_di_write(di, di_gen, DI_GENERAL); diff --git a/drivers/staging/imx-drm/ipuv3-crtc.c b/drivers/staging/imx-drm/ipuv3-crtc.c index e646017..f506075 100644 --- a/drivers/staging/imx-drm/ipuv3-crtc.c +++ b/drivers/staging/imx-drm/ipuv3-crtc.c @@ -158,7 +158,7 @@ static int ipu_crtc_mode_set(struct drm_crtc *crtc, sig_cfg.Vsync_pol = 1; sig_cfg.enable_pol = 1; - sig_cfg.clk_pol = 1; + sig_cfg.clk_pol = 0; sig_cfg.width = mode-hdisplay; sig_cfg.height = mode-vdisplay; sig_cfg.pixel_fmt = out_pixel_fmt; I brought this up a while back: http://archive.arm.linux.org.uk/lurker/message/20131015.103500.0c058eb9.en.html it looks like it was never properly addressed, so yes, I think this is the right solution, and brings the kernel inline with the code which was in uboot back in October, and the value of sig_cfg.clk_pol now matches the register bit. However, I think an even better solution would be to have the clk_pol values to be defined: CLK_POL_ACTIVE_HIGH and CLK_POL_ACTIVE_LOW. This makes the actual value used irrelevant, and helps readability. Maybe something to consider for a future patch? -- FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/2] imx-drm: imx-ldb: Use OF graph to find connected panel
On Thu, Mar 06, 2014 at 02:54:40PM +0100, Philipp Zabel wrote: @@ -566,9 +566,18 @@ static int imx_ldb_bind(struct device *dev, struct device *master, void *data) return -EINVAL; } - panel_node = of_parse_phandle(child, fsl,panel, 0); - if (panel_node) - channel-panel = of_drm_find_panel(panel_node); + /* The output port is port@4 with mux or port@1 without mux */ + port = of_graph_get_port_by_id(child, imx_ldb-lvds_mux ? 4 : 1); I guess we're holding off on these two patches while the last bits of the of-graph get sorted - the above function doesn't currently exist so causes a build failure. Thanks. -- FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/2] imx-drm: imx-ldb: Add drm_panel support
On Thu, Mar 06, 2014 at 02:54:39PM +0100, Philipp Zabel wrote: This patch allows to optionally attach the lvds-channel to a panel supported by a drm_panel driver instead of supplying the modes via device tree. Hmm, I think something may be missing in this patch... you're introducing calls into the drm panel code, but there's nothign which ensures that code gets built alongside imx-ldb.c. With imx-ldb built as a module, I get: ERROR: drm_panel_attach [drivers/staging/imx-drm/imx-ldb.ko] undefined! ERROR: of_drm_find_panel [drivers/staging/imx-drm/imx-ldb.ko] undefined! -- FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v5 00/11] imx-drm dt bindings
On Wed, Mar 05, 2014 at 10:20:51AM +0100, Philipp Zabel wrote: Hi, this latest version of the imx-drm DT binding patches applies on top of staging-next and also depends on the OF graph binding patchset that moves the v4l2_of helpers to drivers/of. Currently, the two patchsets are also available at: git://git.pengutronix.de/git/pza/linux.git topic/of-graph git://git.pengutronix.de/git/pza/linux.git topic/imx-drm-dt Okay, having looked at the second tree, pulling that will result in pulling in all of the staging tree here, which I'd rather not do. Unless there's any objection, I'd like to take these as patches on top of the imx-drm stuff which I sent to Greg plus the of-graph stuff which they depend upon. In other words, exactly how I've been testing it today. Any objections? -- FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v5 00/11] imx-drm dt bindings
On Fri, Mar 07, 2014 at 07:57:51PM +0100, Philipp Zabel wrote: [Added Shawn to Cc:] On Fri, Mar 7, 2014 at 7:28 PM, Greg Kroah-Hartman gre...@linuxfoundation.org wrote: On Fri, Mar 07, 2014 at 05:56:12PM +, Russell King - ARM Linux wrote: On Wed, Mar 05, 2014 at 10:20:51AM +0100, Philipp Zabel wrote: Hi, this latest version of the imx-drm DT binding patches applies on top of staging-next and also depends on the OF graph binding patchset that moves the v4l2_of helpers to drivers/of. Currently, the two patchsets are also available at: git://git.pengutronix.de/git/pza/linux.git topic/of-graph git://git.pengutronix.de/git/pza/linux.git topic/imx-drm-dt Okay, having looked at the second tree, pulling that will result in pulling in all of the staging tree here, which I'd rather not do. Unless there's any objection, I'd like to take these as patches on top of the imx-drm stuff which I sent to Greg plus the of-graph stuff which they depend upon. In other words, exactly how I've been testing it today. Any objections? None from me! :) Nor from me. But I'd like to point out that there already is one merge issue with Shawn's for-next branch in arch/arm/boot/dts/imx53-qsb.dts between d5eb195 ARM: dts: i.MX53: move common QSB nodes to new file and these two patches: 17b5001 imx-drm: convert to componentised device support. ARM: dts: imx53: Add IPU DI ports and endpoints, move imx-drm node to dtsi The first one already is in staging-next. Yes, I'm aware of that (since I've been encountering it when creating the build tree for my autobuilder.) Thanks. -- FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 07/12] drm: drm_display_mode: add signal polarity flags
On Tue, Mar 18, 2014 at 08:50:30AM +0100, Lothar Waßmann wrote: Hi, Laurent Pinchart wrote: Hi Lothar, On Monday 17 March 2014 16:14:36 Lothar Waßmann wrote: DE is not a clock signal, but an 'Enable' signal whose value (high or low) defines the window in which the pixel data is valid. The flag defines whether data is valid during the HIGH or LOW period of DE. The DRM_MODE_FLAG_POL_DE_(LOW|HIGH) do, by my impression of the proposed new DRM_MODE_FLAG_POL_DE_*EDGE flags is that they define sampling clock edges, not active levels. The current naming of the flags gives the impression that they describe the sampling edges of a clock signal. But the DE signal in fact is not a clock signal but a level sensitive gating signal. +1 -- FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 07/12] drm: drm_display_mode: add signal polarity flags
On Tue, Mar 18, 2014 at 01:41:54PM +0100, Laurent Pinchart wrote: Hi Lothar, That's not my point. I *know* that DE is a data gating signal with a polarity already defined by the DRM_MODE_FLAG_POL_DE_(LOW|HIGH) flags. Like all other signals it gets generated on a clock edge and is sampled on a clock edge. The DRM_MODE_FLAG_POL_DE_*EDGE flags proposed above describe seem to describe just that, *not* the signal polarity. I thus want to know whether there are systems where the data signals and the DE signal need to be sampled on different edges of the pixel clock. That's not relevant to this patch series though. This patch series is about adding configuration which will allow iMX6 SoCs to be properly configured for their displays. iMX has the ability to: - define the polarity of the clock signal - define the polarity of the other signals It does not have the ability to define which clock edge individual signals like vsync (frame clock), hsync (line clock), disable enable change on independently. So, it doesn't make sense _here_ for the display enable to be defined by an edge - it's not something that can be changed here. What can only be changed is it's active level. Of course, there may be some which can do this, and that's a separate problem that would need to be addressed when there's hardware that does support it. The objection which Lothar is raising is that _this_ definition does not match the _hardware_ capabilities which it is intended to be used with, which is a very valid objection. -- FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH RFC 26/46] drivers/base: provide an infrastructure for componentised subsystems
On Wed, Mar 19, 2014 at 06:22:14PM +0100, Laurent Pinchart wrote: Hi Russell, (CC'ing Philipp Zabel who might be able to provide feedback as a user of the component framework) Could you please have a look at the questions below and provide an answer when you'll have time ? I'd like to bridge the gap between the component and the V4L2 asynchronous registration implementations. I have a reply partly prepared, but I'm snowed under by the L2 cache stuff at the moment, sorry. -- FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH RFC 26/46] drivers/base: provide an infrastructure for componentised subsystems
On Fri, Mar 07, 2014 at 12:24:33AM +0100, Laurent Pinchart wrote: However, we (as in the V4L2 community, and me personally) would have appreciated to be CC'ed on the proposal. As you might know we already had a solution for this problem, albeit V4L2-specific, in drivers/media/v4l2- core/v4l2-async.c. There's a lot of people who would have liked to be on the Cc, but there's two problems: 1. the Cc list would be too big for mailing lists to accept the message, and 2. finding everyone who should be on the Cc list is quite an impossible task. The topic is particularly hot given that a similar solution was also proposed as part of the now defunct (or at least hibernating) common display framework. Yes, I am aware of CDF. However, the annoying thing is that it's another case of the bigger picture not being looked at - which is that we don't need yet another subsystem specific solution to a problem which is not subsystem specific. The fact of the matter is that /anyone/ has the opportunity to come up with a generic solution to this problem, and no one did... instead, more solutions were generated - the proof is we solved this in CDF with a CDF specific solution. :p If I had replied to this mail thread without sleeping on it first I might not have known better and have got half-paranoid, wondereding whether there had been a deliberate attempt to fast-track this API before the V4L2 developers noticed. To be perfectly clear, there is *NO* implicit or explicit such accusation here, as I know better. What would have happened is that CDF would have been raised, and there would be a big long discussion with no real resolution. The problem would not have been solved (even partially). We'd be sitting here right now still without any kind of solution that anyone can use. Instead, what we have right now is the opportunity for people to start making use of this and solving the real problems they have with driver initialisation. For example, the IPU on iMX locks up after a number of mode changes, and it's useful to be able to unload the driver, poke about in the hardware, and reload it. Without this problem fixed, that's impossible without rebooting the kernel, because removing the driver oopses the kernel due to the broken work-arounds that it has to do - and it has to do those because this problem has not been solved, despite it being known about for /years/. Accordingly, I would like to comment on the component API, despite the fact that it has been merged in mainline already. The first thing that I believe is missing is documentation. Do you have any pending patch for that, either as kerneldoc or as a text file for Documentation/ ? As I've read the code to understand it I might have missed so design goals, so please bear with the stupid questions that may follow. There's lots of things in the kernel which you just have to read the code for - and this is one of them at the moment. :) (Another is PM domains...) What I know is that this will not satisfy all your requirements - I don't want it to initially satisfy everyone's requirements, because that's just far too big a job, but it satisfies the common problem that most people are suffering from and have already implemented many badly written driver specific solutions. In other words - this is designed to _improve_ the current situation where we have lots of buggy implementions trying to work around this problem, factor that code out, and fix up those problems. Briefly, the idea is: - there is a master device - lots of these subsystems doing this already have that, whether that be ALSA or DRM based stuff. - then there are the individual component devices and their drivers. Subsystems like ALSA and DRM are not component based subsystems. These subsystems have two states - either they're initialised and the entire card system is known about, or they're not initialised. There is no possibility of a piecemeal approach, where they partially come up and additional stuff gets added later. With DRM, that's especially true because of how the userspace API works - to change that probably means changing stuff all the way through things like the X server and its xrandr application interface. This is probably the reason why David said at KS that DRM isn't going to do the hotplugging of components. The master device has a privileged position - it gets to make the decision about which component devices are relevant to it, and when the card system is fully known. As far as DT goes, we've had a long discussion about this approach in the past, and we've accepted this approach - we have the sound node which doesn't actually refer to any hardware block, it's a node which describes how the hardware blocks are connected together, which gets translated into a platform device. When a master device gets added, it gets added to the list of master devices, and then it's asked whether all the components that it needs are present. Since we
Re: [PATCH v5 00/11] imx-drm dt bindings
On Mon, Apr 07, 2014 at 12:23:37PM +0800, Shawn Guo wrote: And I see another HDMI regression with my testing on mainline kernel. I can have my HDMI work at 1920x1080 with v3.14 kernel, but it can only probes 1024x768 with the mainline today. Works for me here. [20.606] (II) LoadModule: modesetting [20.607] (II) Loading /usr/lib/xorg/modules/drivers/modesetting_drv.so [20.609] (II) Module modesetting: vendor=X.Org Foundation [20.609] compiled for 1.12.1.902, module version = 0.3.0 [20.610] Module class: X.Org Video Driver [20.610] ABI class: X.Org Video Driver, version 12.0 [20.610] (II) modesetting: Driver for Modesetting Kernel Drivers: kms [20.610] (++) using VT number 7 [20.624] (WW) Falling back to old probe method for modesetting [20.624] (II) modesetting(0): using default device [20.627] (II) modesetting(0): Creating default Display subsection in Screen section Default Screen Section for depth/fbbpp 24/32 [20.627] (==) modesetting(0): Depth 24, (==) framebuffer bpp 32 [20.628] (==) modesetting(0): RGB weight 888 [20.628] (==) modesetting(0): Default visual is TrueColor [20.628] (II) modesetting(0): ShadowFB: preferred NO, enabled NO [20.628] (II) modesetting(0): Output HDMI-0 has no monitor section [20.629] (II) modesetting(0): EDID for output HDMI-0 [20.629] (II) modesetting(0): Printing probed modes for output HDMI-0 So no EDID. Did you update the ddc property to ddc-i2c-bus ? -- FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v12][ 03/12] imx-drm: Correct BGR666 and the board's dts that use them.
On Mon, Apr 07, 2014 at 02:44:42PM +0200, Denis Carikli wrote: arch/arm/boot/dts/imx51-apf51dev.dts|2 +- arch/arm/boot/dts/imx53-m53evk.dts |2 +- drivers/staging/imx-drm/imx-ldb.c |4 ++-- drivers/staging/imx-drm/ipu-v3/ipu-dc.c |4 ++-- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/arch/arm/boot/dts/imx51-apf51dev.dts b/arch/arm/boot/dts/imx51-apf51dev.dts index c5a9a24..7b3851d 100644 --- a/arch/arm/boot/dts/imx51-apf51dev.dts +++ b/arch/arm/boot/dts/imx51-apf51dev.dts @@ -18,7 +18,7 @@ display@di1 { compatible = fsl,imx-parallel-display; - interface-pix-fmt = bgr666; + interface-pix-fmt = rgb666; ... /* bgr666 */ ipu_dc_map_clear(priv, IPU_DC_MAP_BGR666); - ipu_dc_map_config(priv, IPU_DC_MAP_BGR666, 0, 5, 0xfc); /* blue */ + ipu_dc_map_config(priv, IPU_DC_MAP_BGR666, 0, 17, 0xfc); /* blue */ ipu_dc_map_config(priv, IPU_DC_MAP_BGR666, 1, 11, 0xfc); /* green */ - ipu_dc_map_config(priv, IPU_DC_MAP_BGR666, 2, 17, 0xfc); /* red */ + ipu_dc_map_config(priv, IPU_DC_MAP_BGR666, 2, 5, 0xfc); /* red */ arm-soc people have become very obtuse with respect to changes to any patches to arch/arm/boot/dts, and complain loudly if any changes to that directory do not go through them as separate patches. What this means is that your patch is unacceptable and needs to be split up. The choices seem to be to either break imx-drm by splitting the patch in two, thereby ending up with the two dependent changes being merged entirely separate, resulting in breakage while one or other is merged, or to add the RGB666 support, wait for that to hit mainline, then update the DT files, wait for that to hit mainline, then fix the BGR666 support. That'll take around six to nine months (two to three months per release cycle.) Or arm-soc could come to their senses and realise that they do not have sole ownership over arch/arm/boot/dts. -- FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/4] staging: imx-hdmi: correct connector detect and hotplug
On Fri, Apr 11, 2014 at 04:13:33PM +0200, Lucas Stach wrote: Make sure that we probe for a display on detect regardless of previous hotplug events. Don't handle connector hotplug state ourselves, but let DRM do the right thing for us. This brings our hotplug handling in line with what other DRM drivers do. Why should working setups have to pay the price for faulty setups when we can adequately detect the hotplug signal on iMX SoCs when it's correctly wired? By price I mean - if we end up having to poll the connector, we end up calling the i2c functions, and the i2c functions on iMX use a fixed timeout of 100ms. That means the context which runs the imx_hdmi_connector_detect() function is forced to sleep for 100ms. If that's being run as part of a softirq (eg, via a work struct), that's bad news because that could be any thread in the system. The price should only be paid by those implementations where the hotplug signal is not correctly wired. -- FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 3/4] staging: imx-hdmi: clear all hotplug IRQs on bind
On Fri, Apr 11, 2014 at 04:13:34PM +0200, Lucas Stach wrote: Makes sure we don't receive a stray IRQ on startup. Signed-off-by: Lucas Stach l.st...@pengutronix.de --- drivers/staging/imx-drm/imx-hdmi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/imx-drm/imx-hdmi.c b/drivers/staging/imx-drm/imx-hdmi.c index 22cfdfc5ef74..7d407e917786 100644 --- a/drivers/staging/imx-drm/imx-hdmi.c +++ b/drivers/staging/imx-drm/imx-hdmi.c @@ -1705,7 +1705,7 @@ static int imx_hdmi_bind(struct device *dev, struct device *master, void *data) hdmi_writeb(hdmi, hdmi-sink_detect_polarity, HDMI_PHY_POL0); /* Clear Hotplug interrupts */ - hdmi_writeb(hdmi, hdmi-sink_detect_status, HDMI_IH_PHY_STAT0); + hdmi_writeb(hdmi, 0x3d, HDMI_IH_PHY_STAT0); If we are only unmasking hdmi-sink_detect_status interrupts via a write to HDMI_IH_MUTE_PHY_STAT0, then why do we need to clear these other interrupts? Please add additional explanation to the commit message giving the reasoning for this change. Thanks. -- FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/4] staging: imx-hdmi: correct connector detect and hotplug
On Mon, Apr 14, 2014 at 10:42:32AM +0200, Lucas Stach wrote: Am Sonntag, den 13.04.2014, 15:58 +0100 schrieb Russell King - ARM Linux: On Fri, Apr 11, 2014 at 04:13:33PM +0200, Lucas Stach wrote: Make sure that we probe for a display on detect regardless of previous hotplug events. Don't handle connector hotplug state ourselves, but let DRM do the right thing for us. This brings our hotplug handling in line with what other DRM drivers do. Why should working setups have to pay the price for faulty setups when we can adequately detect the hotplug signal on iMX SoCs when it's correctly wired? By price I mean - if we end up having to poll the connector, we end up calling the i2c functions, and the i2c functions on iMX use a fixed timeout of 100ms. That means the context which runs the imx_hdmi_connector_detect() function is forced to sleep for 100ms. If that's being run as part of a softirq (eg, via a work struct), that's bad news because that could be any thread in the system. The price should only be paid by those implementations where the hotplug signal is not correctly wired. This change is not related to broken systems. It just uses the DRM framework as intended. The detect() callback, which triggers the EDID fetch will only be called by DRM when a hotplug event was received, or if someone (e.g. kms_fb_helper, or userspace) explicitly requests to poll the connector. Not doing so is working around the DRM framework, not using it. So as mentioned this change just brings us in line with what other DRM drivers do to handle hotplug and connector detect. I totally disagree with that. What we're doing today using HPD to detect connection is entirely in keeping with DRM and the HDMI spec, and is more correct than your solution using EDID to detect the presence of a connection. HPD in HDMI indicates that the EDID is available for reading. There is no need what so ever to try reading the EDID to detect whether a device is present. Moreover, the HDMI spec does not say what state the DDC signals will be when the sink is powered off - it seems to me that it is entirely reasonable when HPD is lowered due to the sink being powered off that the DDC signals may be clamped to logic zero by ESD diodes in the sink, which would cause problems when trying to detect by reading the EDID. Moreover, it is quite legal for a sink to modify the contents of its EEPROM - and it can do this by manipulating the DDC signals itself. Polling the EDID would open the possibilities of races, reading the EDID before the sink had finished updating it. -- FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/4] staging: imx-hdmi: correct connector detect and hotplug
On Mon, Apr 14, 2014 at 11:38:43AM +0200, Lucas Stach wrote: Am Montag, den 14.04.2014, 10:10 +0100 schrieb Russell King - ARM Linux: On Mon, Apr 14, 2014 at 10:42:32AM +0200, Lucas Stach wrote: Am Sonntag, den 13.04.2014, 15:58 +0100 schrieb Russell King - ARM Linux: On Fri, Apr 11, 2014 at 04:13:33PM +0200, Lucas Stach wrote: Make sure that we probe for a display on detect regardless of previous hotplug events. Don't handle connector hotplug state ourselves, but let DRM do the right thing for us. This brings our hotplug handling in line with what other DRM drivers do. Why should working setups have to pay the price for faulty setups when we can adequately detect the hotplug signal on iMX SoCs when it's correctly wired? By price I mean - if we end up having to poll the connector, we end up calling the i2c functions, and the i2c functions on iMX use a fixed timeout of 100ms. That means the context which runs the imx_hdmi_connector_detect() function is forced to sleep for 100ms. If that's being run as part of a softirq (eg, via a work struct), that's bad news because that could be any thread in the system. The price should only be paid by those implementations where the hotplug signal is not correctly wired. This change is not related to broken systems. It just uses the DRM framework as intended. The detect() callback, which triggers the EDID fetch will only be called by DRM when a hotplug event was received, or if someone (e.g. kms_fb_helper, or userspace) explicitly requests to poll the connector. Not doing so is working around the DRM framework, not using it. So as mentioned this change just brings us in line with what other DRM drivers do to handle hotplug and connector detect. I totally disagree with that. What we're doing today using HPD to detect connection is entirely in keeping with DRM and the HDMI spec, and is more correct than your solution using EDID to detect the presence of a connection. HPD in HDMI indicates that the EDID is available for reading. There is no need what so ever to try reading the EDID to detect whether a device is present. Moreover, the HDMI spec does not say what state the DDC signals will be when the sink is powered off - it seems to me that it is entirely reasonable when HPD is lowered due to the sink being powered off that the DDC signals may be clamped to logic zero by ESD diodes in the sink, which would cause problems when trying to detect by reading the EDID. Moreover, it is quite legal for a sink to modify the contents of its EEPROM - and it can do this by manipulating the DDC signals itself. Polling the EDID would open the possibilities of races, reading the EDID before the sink had finished updating it. And that's exactly what happens now. We do not poll the EDID in any way, until we are explicitly asked to do so, which happens only very few occasions. Please go back and read the code after this patch. What we do now in the regular case (nobody is calling detect() explicitly) is the following: Now *you* please go back and read what you said about kms/userspace being able to poll the connector, thereby causing an EDID read attempt while HPD may not be active. -- FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 0/7] Reorder i.MX IPU display enable/disable sequence
On Mon, Apr 14, 2014 at 05:21:28PM +0200, Philipp Zabel wrote: Repeatedly enabling and disabling the display currently can lead to a state in which the IPU doesn't produce a valid signal anymore because we disable IPU submodules before they can finish their interaction. Well done at finding the cause of this - I've encounted this several times, while testing imx-hdmi and it's good that it should hopefully be fixed! -- FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/4] staging: imx-hdmi: correct connector detect and hotplug
On Mon, Apr 14, 2014 at 12:24:45PM +0200, Lucas Stach wrote: Am Montag, den 14.04.2014, 11:09 +0100 schrieb Russell King - ARM Linux: Now *you* please go back and read what you said about kms/userspace being able to poll the connector, thereby causing an EDID read attempt while HPD may not be active. Yes, userspace may trigger an explicit detect because it might suspect that a sink is present while it has not received any HP event. What userspace expects to happen in this situation is an explicit poll of the connector regardless of the HP status. So, if you issue this poll, and the sink has lowered the HPD signal because it wants to update the EDID EEPROM, and is in the middle of doing so. Meanwhile you start an I2C transaction in the DDC bus. Maybe you win the arbitration, maybe you gain access because you manage to get your transaction in while the sink is between two I2C transactions. The result is you can end up reading inconsistent EDID data from the sink. There is no race free way to do this - HPD is the indication on HDMI that the sink is available, and that the EDID can be read by the source. If HPD is not active, then the EDID should *not* be read. If you then just report the connector as disconnected because you didn't receive a HP event before, you break the use-case for which userspace is calling an explicit detect in the first place. What is wrong is that we store the interrupt-generated cached state, rather than reporting the actual HPD signal state when the detect method is used. We need to be reporting the real live state of the HPD signal in that function - or, in the case where HPD has not been correctly wired, your fallback of the RXSENSE bits. HPD *is* the signal which says the HDMI sink is *properly* connected and the EDID data is available for you to read when it is asserted. When HPD is not asserted, the HDMI sink is saying that the EDID data is not available. -- FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 0/7] Reorder i.MX IPU display enable/disable sequence
On Mon, Apr 14, 2014 at 05:21:28PM +0200, Philipp Zabel wrote: Repeatedly enabling and disabling the display currently can lead to a state in which the IPU doesn't produce a valid signal anymore because we disable IPU submodules before they can finish their interaction. I'm afraid to say that after adding these patches, I find that when Xorg starts, the thing appears to lock up. Bisecting points at: imx-drm: ipu-dc: Wait for DC_FC_1 / DP_SF_END interrupt being the culpret - booting with the patches up to and including: imx-drm: ipu-dmfc: Wait for FIFOs to run empty before disabling works fine. -- FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 4/7] imx-drm: ipu-dc: Wait for DC_FC_1 / DP_SF_END interrupt
On Mon, Apr 14, 2014 at 05:21:32PM +0200, Philipp Zabel wrote: Wait for the DC Frame Complete or DP Sync Flow End interrupts before disabling DC channels. Signed-off-by: Philipp Zabel p.za...@pengutronix.de --- drivers/staging/imx-drm/ipu-v3/ipu-dc.c | 71 +++-- 1 file changed, 50 insertions(+), 21 deletions(-) diff --git a/drivers/staging/imx-drm/ipu-v3/ipu-dc.c b/drivers/staging/imx-drm/ipu-v3/ipu-dc.c index d5de8bb..13b7538 100644 --- a/drivers/staging/imx-drm/ipu-v3/ipu-dc.c +++ b/drivers/staging/imx-drm/ipu-v3/ipu-dc.c @@ -18,6 +18,7 @@ #include linux/types.h #include linux/errno.h #include linux/delay.h +#include linux/interrupt.h #include linux/io.h #include ../imx-drm.h @@ -110,6 +111,9 @@ struct ipu_dc_priv { struct device *dev; struct ipu_dc channels[IPU_DC_NUM_CHANNELS]; struct mutexmutex; + struct completion comp; + int dc_irq; + int dp_irq; }; static void dc_link_event(struct ipu_dc *dc, int event, int addr, int priority) @@ -239,38 +243,46 @@ void ipu_dc_enable_channel(struct ipu_dc *dc) } EXPORT_SYMBOL_GPL(ipu_dc_enable_channel); +static irqreturn_t dc_irq_handler(int irq, void *dev_id) +{ + struct ipu_dc *dc = dev_id; + u32 reg; + + reg = readl(dc-base + DC_WR_CH_CONF); + reg = ~DC_WR_CH_CONF_PROG_TYPE_MASK; + writel(reg, dc-base + DC_WR_CH_CONF); + + /* The Freescale BSP kernel clears DIx_COUNTER_RELEASE here */ + + disable_irq(irq); Shouldn't this be disable_irq_nosync() ? -- FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 0/8] Reorder i.MX IPU display enable/disable sequence
On Mon, Apr 14, 2014 at 11:53:15PM +0200, Philipp Zabel wrote: Repeatedly enabling and disabling the display currently can lead to a state in which the IPU doesn't produce a valid signal anymore because we disable IPU submodules before they can finish their interaction. Yes, this appears to not lockup as the last series did. I'll see about squeezing in some further testing over the coming days. Thanks. -- FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 0/8] Reorder i.MX IPU display enable/disable sequence
On Mon, Apr 14, 2014 at 11:53:15PM +0200, Philipp Zabel wrote: Repeatedly enabling and disabling the display currently can lead to a state in which the IPU doesn't produce a valid signal anymore because we disable IPU submodules before they can finish their interaction. This series reorders the enable/disable sequence so that we first wait for the DC/DP to finish processing the current frame, then stop the DI and IDMAC, and only then disable clocks to the submodules. Also from now on we really disable the DC when it is not in use. Okay, I'm going to queue these up in a couple of days, but there's something which I'd prefer to be fixed... there's one in particular that is excessively long. Philipp Zabel (8): imx-drm: ipu-common: add ipu_map_irq to request non-IDMAC interrupts imx-drm: ipu-common: Add helpers to check for a busy IDMAC channel and to busywait for an interrupt imx-drm: ipu-dmfc: Wait for FIFOs to run empty before disabling imx-drm: ipu-dc: Wait for DC_FC_1 / DP_SF_END interrupt imx-drm: ipu-dp: Split disabling the DP foreground channel from disabling the DP module imx-drm: imx-dp: When disabling the DP foreground channel, wait until the DP background channel is finished before disabling the IDMAC channel imx-drm: imx-dp: disable DP fg channel after DP bg channel has finished maybe? imx-drm: ipuv3-crtc: Change display enable/disable order imx-drm: ipu-dc: Disable DC module when not in use -- FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] imx-drm: fix hdmi hotplug detection initial state
On Thu, Apr 24, 2014 at 02:00:49PM -0700, Tim Harvey wrote: I'm still seeing issues with HDMI detect on powerup, at least on my Gateworks Ventana boards (which have no voltage devider or anything else on the HPD line to the IMX6 other than a TVS). I'm currently using your latest imx-drm-staging branch and have applied this patch on top of it. When I enable debug in imx-hdmi.c I see the following: So it's a similar setup to the Cubox-i. on powerup with HDMI attached: [6.291082] imx-ipuv3 240.ipu: IPUv3H probed [6.298370] imx-ipuv3 280.ipu: IPUv3H probed [6.310356] [drm] Supports vblank timestamp caching Rev 2 (21.10.2013). [6.317087] [drm] No driver support for vblank timestamp query. [6.324249] imx-drm display-subsystem.11: bound imx-ipuv3-crtc.0 (ops ipu_crtc_ops) [6.332475] imx-drm display-subsystem.11: bound imx-ipuv3-crtc.1 (ops ipu_crtc_ops) [6.340616] imx-drm display-subsystem.11: bound imx-ipuv3-crtc.2 (ops ipu_crtc_ops) [6.349221] imx-drm display-subsystem.11: bound imx-ipuv3-crtc.3 (ops ipu_crtc_ops) [6.358098] imx-hdmi 12.hdmi: Detected HDMI controller 0x13:0xa:0xa0:0xc1 [6.365366] hdmi_compute_cts: freq: 48000 pixel_clk: 7425 ratio: 100 [6.372169] imx-hdmi 12.hdmi: hdmi_set_clk_regenerator: samplerate=48000 ratio=100 pixelclk=7425 N=6144 cts=74250 [6.383971] imx-drm display-subsystem.11: bound 12.hdmi (ops hdmi_ops) [6.383998] imx-hdmi 12.hdmi: EVENT=plugin [6.384011] imx-hdmi 12.hdmi: imx_hdmi_poweron [6.384022] imx-hdmi 12.hdmi: imx_hdmi_setup vic=0 [6.384031] imx-hdmi 12.hdmi: Non-CEA mode used in HDMI [6.384038] imx-hdmi 12.hdmi: final pixclk = 0 [6.402148] imx-hdmi 12.hdmi: imx_hdmi_setup DVI mode [6.428804] imx-drm display-subsystem.11: bound ldb.10 (ops imx_ldb_ops) [6.490101] imx-hdmi 12.hdmi: got edid: width[70] x height[39] [6.506191] imx-drm display-subsystem.11: fb0: frame buffer device [6.512596] imx-drm display-subsystem.11: registered panic notifier [6.518947] [drm] Initialized imx-drm 1.0.0 20120507 on minor 0 So at boot, I see this: [1.282549] imx-ipuv3 240.ipu: IPUv3H probed [1.295494] [drm] Supports vblank timestamp caching Rev 2 (21.10.2013). [1.302197] [drm] No driver support for vblank timestamp query. [1.308315] imx-drm display-subsystem.10: bound imx-ipuv3-crtc.0 (ops ipu_crtc_ops) [1.326728] imx-drm display-subsystem.10: bound imx-ipuv3-crtc.1 (ops ipu_crtc_ops) [1.336741] imx-hdmi 12.hdmi: Detected HDMI controller 0x13:0x1a:0xa0:0xc1 [1.346109] hdmi_compute_cts: freq: 48000 pixel_clk: 7425 ratio: 100 [1.346128] imx-hdmi 12.hdmi: hdmi_set_clk_regenerator: samplerate=48000 ratio=100 pixelclk=7425 N=6144 cts=74250 [1.346276] imx-hdmi 12.hdmi: EVENT=plugin [1.346291] imx-hdmi 12.hdmi: Non-CEA mode used in HDMI [1.346301] imx-hdmi 12.hdmi: final pixclk = 0 [1.364422] imx-hdmi 12.hdmi: imx_hdmi_setup DVI mode [1.365896] imx-drm display-subsystem.10: bound 12.hdmi (ops hdmi_ops) [1.428629] imx-hdmi 12.hdmi: got edid: width[128] x height[72] [1.435752] imx-hdmi 12.hdmi: CEA mode used vic=31 [1.435761] imx-hdmi 12.hdmi: final pixclk = 14850 [1.453879] imx-hdmi 12.hdmi: imx_hdmi_setup CEA mode [1.453886] hdmi_compute_cts: freq: 48000 pixel_clk: 14850 ratio: 100 [1.453896] imx-hdmi 12.hdmi: hdmi_set_clk_regenerator: samplerate=48000 ratio=100 pixelclk=14850 N=6144 cts=148500 [1.454549] imx-hdmi 12.hdmi: CEA mode used vic=31 [1.454555] imx-hdmi 12.hdmi: final pixclk = 14850 [1.472685] imx-hdmi 12.hdmi: imx_hdmi_setup CEA mode [1.472691] hdmi_compute_cts: freq: 48000 pixel_clk: 14850 ratio: 100 [1.472702] imx-hdmi 12.hdmi: hdmi_set_clk_regenerator: samplerate=48000 ratio=100 pixelclk=14850 N=6144 cts=148500 [1.489418] Console: switching to colour frame buffer device 240x67 [1.511370] imx-drm display-subsystem.10: fb0: frame buffer device [1.517770] imx-drm display-subsystem.10: registered panic notifier [1.524160] [drm] Initialized imx-drm 1.0.0 20120507 on minor 0 which is only with imx-hdmi bound, no ldb. The difference you will notice is that there's the Console: switching ... line, which is there because I have fbcon enabled. Therefore, I suspect that it is working as it should - if you enable fbcon, it should automatically initialise. If you don't have fbcon enabled, then it'll probably wait for a userspace DRM driver to bring it up. -- FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] imx-drm: fix hdmi hotplug detection initial state
On Thu, Apr 24, 2014 at 03:57:27PM -0700, Tim Harvey wrote: On Thu, Apr 24, 2014 at 3:07 PM, Russell King - ARM Linux li...@arm.linux.org.uk wrote: On Thu, Apr 24, 2014 at 02:00:49PM -0700, Tim Harvey wrote: I'm still seeing issues with HDMI detect on powerup, at least on my Gateworks Ventana boards (which have no voltage devider or anything else on the HPD line to the IMX6 other than a TVS). I'm currently using your latest imx-drm-staging branch and have applied this patch on top of it. When I enable debug in imx-hdmi.c I see the following: So it's a similar setup to the Cubox-i. snip which is only with imx-hdmi bound, no ldb. The difference you will notice is that there's the Console: switching ... line, which is there because I have fbcon enabled. Therefore, I suspect that it is working as it should - if you enable fbcon, it should automatically initialise. If you don't have fbcon enabled, then it'll probably wait for a userspace DRM driver to bring it up. Russell, Yes, your correct. If I enable fbcon it comes up at boot. But what if I don't want fbcon? I have CONFIG_LOGO=y and I would expect that to come up without needing to hotswap. Something must still be missing that is getting taken care of by fbcon. Hmm. I don't know - tracing the code paths, towards the end of the imx-drm initialisation, we call drm_fbdev_cma_init(), which should configure the initial modes - this will only happen if CONFIG_DRM_IMX_FB_HELPER is enabled, which you seem to have set already in your boot logs. drm_fbdev_cma_init() will call drm_fb_helper_initial_config() to set an initial configuration. It would seem to think there are available modes, otherwise you'd get a No connectors reported connected with modes message. That creates a fb device of the desired size for the mode by calling back into drm_fbdev_cma_create(). That creates the fbdev device, and back in drm_fb_helper_single_fb_probe(), the fbdev device is registered. And at that point, we're into the depths of the fb device layers to decide what to do... and it's up to them to set an initial mode. So, the question I'd put to you is: do you get an initial mode when trying the same configuration but with a pure fb driver? I suspect you'll be in the same situation: without fbcon, fbdev doesn't initialise registered framebuffers with an initial mode. -- FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [RFC PATCH 0/8] component helper improvements
On Sun, Apr 27, 2014 at 02:51:30PM +0200, Daniel Vetter wrote: On Sun, Apr 27, 2014 at 1:00 AM, Russell King - ARM Linux li...@arm.linux.org.uk wrote: A while back, Laurent raised some comments about the component helper, which this patch set starts to address. The first point it addresses is the repeated parsing inefficiency when deferred probing occurs. When DT is used, the structure of the component helper today means that masters end up parsing the device tree for each attempt to re-bind the driver. We remove this inefficiency by creating an array of matching data and functions, which the component helper can use internally to match up components to their master. The second point was the inefficiency of destroying the list of components each time we saw a failure. We did this to ensure that we kept things correctly ordered: component bind order matters. As we have an array instead, the array is already ordered, so we use this array to store the component pointers instead of a list, and remember which are duplicates (and thus should be avoided.) Avoiding the right duplicates matters as we walk the array in the opposite direction at tear down. I would like to see patches 1-5 scheduled for the next merge window, with 6-8 for the following window - this gives us grace of one kernel cycle to ensure that any new component helper users are properly converted. Afaict the actual patches haven't made it to dri-devel, only to linux-arm-kernel. Are they stuck somewhere? The patches themselves end up being Cc'd depending on their content and the contents of the MAINTAINERS file, unless I specifically tell my scripts that the patches are to be sent to/cc people - generally I do that for the primary recipients of the series. That means only the patch(es) which touch DRM stuff were copied to dri-devel, in this case, that being the MSM one. -- FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/1] staging: imx-drm: Lines over 80 characters fixed.
On Tue, Aug 19, 2014 at 05:36:10PM +0300, Yannis Damigos wrote: diff --git a/drivers/staging/imx-drm/ipuv3-crtc.c b/drivers/staging/imx-drm/ipuv3-crtc.c index 720868b..d6657a0 100644 --- a/drivers/staging/imx-drm/ipuv3-crtc.c +++ b/drivers/staging/imx-drm/ipuv3-crtc.c @@ -201,9 +201,10 @@ static int ipu_crtc_mode_set(struct drm_crtc *crtc, return ret; } - return ipu_plane_mode_set(ipu_crtc-plane[0], crtc, mode, crtc-primary-fb, - 0, 0, mode-hdisplay, mode-vdisplay, - x, y, mode-hdisplay, mode-vdisplay); + return ipu_plane_mode_set(ipu_crtc-plane[0], crtc, mode, + crtc-primary-fb, + 0, 0, mode-hdisplay, mode-vdisplay, + x, y, mode-hdisplay, mode-vdisplay); Why change the indentation like this? There is a wisdom which suggests that it's better to align the arguments with the function call's first argument, unless there is a strong reason not to. In this case, I can't see a strong reason to change the indentation to some different style here. } static void ipu_crtc_handle_pageflip(struct ipu_crtc *ipu_crtc) @@ -227,7 +228,8 @@ static irqreturn_t ipu_irq_handler(int irq, void *dev_id) if (ipu_crtc-newfb) { ipu_crtc-newfb = NULL; - ipu_plane_set_base(ipu_crtc-plane[0], ipu_crtc-base.primary-fb, + ipu_plane_set_base(ipu_crtc-plane[0], + ipu_crtc-base.primary-fb, ipu_crtc-plane[0]-x, ipu_crtc-plane[0]-y); What may make better sense here is: struct ipu_plane *plane = ipu_crtc-plane[0]; ipu_crtc-newfb = NULL; ipu_plane_set_base(plane, ipu_crtc-base.primary-fb, plane-x, plane-y); which to me looks loads nicer. -- FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up according to speedtest.net. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH RFC v2 3/8] component: add support for component match array
On Thu, Jul 03, 2014 at 12:26:39AM +0900, Inki Dae wrote: 2014-07-01 23:22 GMT+09:00 Russell King - ARM Linux li...@arm.linux.org.uk: On Thu, Jun 26, 2014 at 03:46:01PM +0100, Russell King - ARM Linux wrote: On Thu, Jun 26, 2014 at 02:34:17PM +0200, Philipp Zabel wrote: Hi Russell, On Tue, Jun 24, 2014 at 9:29 PM, Russell King rmk+ker...@arm.linux.org.uk wrote: [...] +/* + * Add a component to be matched. + * + * The match array is first created or extended if necessary. + */ +void component_match_add(struct device *dev, struct component_match **matchptr, + int (*compare)(struct device *, void *), void *compare_data) +{ + struct component_match *match = *matchptr; + + if (IS_ERR(match)) + return; + + if (!match || match-num == match-alloc) { + size_t new_size = match ? match-alloc + 16 : 15; + + match = component_match_realloc(dev, match, new_size); + + *matchptr = match; + + if (IS_ERR(match)) + return; + } + + match-compare[match-num].fn = compare; + match-compare[match-num].data = compare_data; + match-num++; +} component_match_add should be exported. Fixed, thanks. As there's no further comments, and Inki Dae has not responded, I'm It's has been just a week. I will check and look into your patch series. I think Exynos drm should also be considered for the use of component match array. It has now been almost two months. What's happening on this? Please note that I'm planning to push the rest of the component updates during the next merge window, which will result in unconverted drivers breaking. Thanks. -- FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up according to speedtest.net. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v18 12/12] drm: bridge/dw_hdmi: add rockchip rk3288 support
On Thu, Dec 11, 2014 at 12:24:15PM +0100, Heiko Stübner wrote: Past practices suggest that having the dw in the name is a sane solution too, like in dw_mmc-foo (mmc/host), dwmac-foo (net/ethernet/stmicro/stmmac). And personally I'd keep to this already established naming scheme ... i.e. not hiding the dw heritage. And also it looks like other involved parties like Philipp and Russell seemed to be ok with the naming through the revisions till now. I don't have much of a preference when it comes to this. I was disappointed that the original imx-hdmi driver did not use dw in its filename, as the documentation clearly stated in several places that it was a designware part, and as we all know, they're a company which sells IP, so their designs are going to crop up in different places. So I welcome this patch set - and I've also tested it on a SolidRun Hummingboard i2ex along with all my CEC and audio patches, where it seems to be fine. So for the set: Tested-by: Russell King rmk+ker...@arm.linux.org.uk Apart from the two minor items I've pointed out in separate replies: Acked-by: Russell King rmk+ker...@arm.linux.org.uk Thanks. -- FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up according to speedtest.net. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v18 04/12] drm: imx: imx-hdmi: split phy configuration to platform driver
On Fri, Dec 05, 2014 at 02:25:50PM +0800, Andy Yan wrote: hdmi phy configuration is platform specific, which can be adusted Minor typo: adjusted -- FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up according to speedtest.net. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v18.1 03/12] drm: imx: imx-hdmi: convert imx-hdmi to drm_bridge mode
On Tue, Jan 06, 2015 at 12:52:24PM +0100, Heiko Stübner wrote: +static void imx_hdmi_bridge_nope(struct drm_bridge *bridge) _nope ? As in No? Or should this be _nop for no-operation ? -- FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up according to speedtest.net. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH/RFC 5/6] staging: board: Add support for devices with complex dependencies
On Fri, Apr 03, 2015 at 03:57:27PM +0300, Dan Carpenter wrote: On Fri, Apr 03, 2015 at 02:42:02PM +0200, Geert Uytterhoeven wrote: +int __init board_staging_register_clock(const struct board_staging_clk *bsc) +{ + struct clk *clk; + int error; + + pr_debug(Registering clock %s for con_id %s dev_id %s\n, bsc-clk, +bsc-con_id, bsc-dev_id); + clk = clk_get(NULL, bsc-clk); + if (IS_ERR(clk)) { + error = PTR_ERR(clk); + pr_err(Failed to get clock %s (%d)\n, bsc-clk, error); + return error; + } + + error = clk_register_clkdev(clk, bsc-con_id, bsc-dev_id); + if (error) + pr_err(Failed to register clock %s (%d)\n, bsc-clk, error); + return error; Missing curly braces. Also it's weird that don't we need a clk_put() on the error patch as well as the success path? What's also concerning is that this is an abuse of this. clk_register_clkdev() is supposed to be used with clocks created with the CCF functions, it's not for creating aliases. We have clk_add_alias() which does *everything* that this function does, only in a less buggy way. -- FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up according to speedtest.net. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH/RFC 5/6] staging: board: Add support for devices with complex dependencies
On Fri, Apr 03, 2015 at 03:27:40PM +0200, Geert Uytterhoeven wrote: On Fri, Apr 3, 2015 at 2:57 PM, Dan Carpenter dan.carpen...@oracle.com wrote: + error = clk_register_clkdev(clk, bsc-con_id, bsc-dev_id); + if (error) + pr_err(Failed to register clock %s (%d)\n, bsc-clk, error); + return error; Missing curly braces. Also it's weird that don't we need a clk_put() on the error patch as well as the success path? Thanks! So it worked only by accident: with the new per-user struct clk instances clk_put() must not be called if clk_register_clkdev() succeeded. Yes, that's because the per-user struct clk messed quite a lot of things up - the patches were /not/ well tested before they went in. That's no excuse to work around the breakage they caused though. That said, I never did post the work I did earlier this month to fix the problems in clkdev which those patches caused... so, I guess it's time to post them and rush them in for the 4.1 merge window... (frankly, the per-user struct clk patches should've been reverted.) -- FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up according to speedtest.net. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] arch:arm:mm:Correction in the boundary check for module end address.
On Tue, Nov 10, 2015 at 12:35:50AM +0900, Jungseung Lee wrote: > Hi, > > 2015-11-09 16:57 GMT+09:00 Shailendra Verma: > > From: Shailendra Verma > > > > The module end boundary check is not proper.The out of bound value > > of module end can produce undesired results. > > > > Signed-off-by: Shailendra Verma > > Reviewed-by: Ravikant Bijendra Sharma > > --- > > linux-4.3-rc6/arch/arm/mm/pageattr.c |2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/linux-4.3-rc6/arch/arm/mm/pageattr.c > > b/linux-4.3-rc6/arch/arm/mm/pageattr.c > > index cf30daf..be7fe4b 100644 > > --- a/linux-4.3-rc6/arch/arm/mm/pageattr.c > > +++ b/linux-4.3-rc6/arch/arm/mm/pageattr.c > > @@ -52,7 +52,7 @@ static int change_memory_common(unsigned long addr, int > > numpages, > > if (start < MODULES_VADDR || start >= MODULES_END) > > return -EINVAL; > > > > - if (end < MODULES_VADDR || start >= MODULES_END) > > + if (end < MODULES_VADDR || end >= MODULES_END) > > return -EINVAL; > > > > data.set_mask = set_mask; > > -- > > 1.7.9.5 > > > Same patch with proper format is already submitted by Hillf. > https://lkml.org/lkml/2015/5/3/202 What happened to that patch? It should at least have had a: Fixes: f2ca09f381a5 ("ARM: 8311/1: Don't use is_module_addr in setting page attributes") tag on it, and it needs to find its way into the patch system. As no one has reported a crash (I don't think it's crash-causing, but is merely a correctness issue) I don't see any reason to backport it to stable trees. Other changes are needed here as well - the original commit creating this contains: + if (!IS_ALIGNED(addr, PAGE_SIZE)) { + start &= PAGE_MASK; + end = start + size; + WARN_ON_ONCE(1); + } which is truely amazing. Fine, it may not be intended to work with non-aligned addresses, but if we're going to round the start down, then rounding the end down as well like that is also buggy. unsigned long start = addr; unsigned long size = PAGE_SIZE * numpages; unsigned long end = start + size; if (WARN_ON_ONCE(!IS_ALIGNED(addr, PAGE_SIZE)) { start &= PAGE_MASK; end = PAGE_ALIGN(end); } would be much better - this will round 'end' up, so we encompass the entire requested region. -- FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [RFC][PATCH 1/3] staging: ion: Move away from the DMA APIs for cache flushing
On Thu, May 26, 2016 at 10:58:35AM +0100, Liviu Dudau wrote: > On Wed, May 25, 2016 at 12:48:02PM -0700, Laura Abbott wrote: > > > > Ion is currently using the DMA APIs in non-compliant ways for cache > > maintaince. The issue is Ion needs to do cache operations outside of > > the regular DMA model. The Ion model matches more closely with the > > DRM model which calls cache APIs directly. Add an appropriate > > abstraction layer for Ion to call cache operations outside of the > > DMA API. I _really_ hate seeing architecture internal functions being abused in drivers - architecture internal functions are there to implement the official kernel APIs and are not for drivers to poke about with. I've had this happen several times, and each time it makes maintanence of architecture code harder than it should be. In any case, the functions you are using are probably not appropriate - the way I've defined the architecture internal functions is for each to have a specific purpose. Eg, if caches need flushing when page tables change, then the function gets implemented, otherwise it's a no-op. Using a function which _seems_ to do the right thing today in a way which is against its purpose is a recipe for your code breaking. If you need something from the architecture which isn't already provided, then you need to talk to architecture people about proposing an official interface to that functionality, rather than trying to bolt per- architecture shims into drivers. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: ion: make the pte default none PTE_RDONLY
On Fri, Jan 15, 2016 at 03:03:42PM -0800, Laura Abbott wrote: > (adding linux-arm and a few people) > > On 01/14/2016 06:42 PM, Chen Feng wrote: > >The page is already alloc at ion_alloc function, > >ion_mmap map the alloced pages to user-space. > > > >The default prot can be PTE_RDONLY. Take a look at > >here: > >set_pte_at() > >arch/arm64/include/asm: > > if (pte_dirty(pte) && pte_write(pte)) > > pte_val(pte) &= ~PTE_RDONLY; > > else > > pte_val(pte) |= PTE_RDONLY; > > > >So with the dirty bit,it can improve the efficiency > >and donnot need to handle memory fault when use access. > > > >Signed-off-by: Chen Feng> >Signed-off-by: Wei Dong > >Reviewed-by: Zhuangluan Su > >--- > > drivers/staging/android/ion/ion.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > >diff --git a/drivers/staging/android/ion/ion.c > >b/drivers/staging/android/ion/ion.c > >index e237e9f..dba5942 100644 > >--- a/drivers/staging/android/ion/ion.c > >+++ b/drivers/staging/android/ion/ion.c > >@@ -1026,6 +1026,9 @@ static int ion_mmap(struct dma_buf *dmabuf, struct > >vm_area_struct *vma) > > if (!(buffer->flags & ION_FLAG_CACHED)) > > vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot); > > > >+/*Default writeable*/ > >+vma->vm_page_prot = pte_mkdirty(vma->vm_page_prot); > >+ > > mutex_lock(>lock); > > /* now map it to userspace */ > > ret = buffer->heap->ops->map_user(buffer->heap, buffer, vma); > > > > The extra fault is unfortunate but I'm skeptical about just setting > pte_mkdirty. > > Catalin/Will, do you have any thoughts? Right now it seems like any > range mapped with remap_pfn_range will have this extra fault > behavior. Is marking the range dirty the best solution? What happens if the mapping requested was read only - at the very least, I don't think this should be done unconditionally. -- RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [RFCv2][PATCH 2/5] arm: Implement ARCH_HAS_FORCE_CACHE
On Mon, Aug 08, 2016 at 10:49:34AM -0700, Laura Abbott wrote: > +/* > + * Make an area consistent for devices. > + * Note: Drivers should NOT use this function directly, as it will break > + * platforms with CONFIG_DMABOUNCE. > + * Use the driver DMA support - see dma-mapping.h (dma_sync_*) > + */ > +void __dma_page_cpu_to_dev(struct page *page, unsigned long off, > + size_t size, enum dma_data_direction dir) > +{ > + phys_addr_t paddr; > + > + dma_cache_maint_page(page, off, size, dir, dmac_map_area); > + > + paddr = page_to_phys(page) + off; > + if (dir == DMA_FROM_DEVICE) { > + outer_inv_range(paddr, paddr + size); > + } else { > + outer_clean_range(paddr, paddr + size); > + } > + /* FIXME: non-speculating: flush on bidirectional mappings? */ > +} > + > +void __dma_page_dev_to_cpu(struct page *page, unsigned long off, > + size_t size, enum dma_data_direction dir) > +{ > + phys_addr_t paddr = page_to_phys(page) + off; > + > + /* FIXME: non-speculating: not required */ > + /* in any case, don't bother invalidating if DMA to device */ > + if (dir != DMA_TO_DEVICE) { > + outer_inv_range(paddr, paddr + size); > + > + dma_cache_maint_page(page, off, size, dir, dmac_unmap_area); > + } > + > + /* > + * Mark the D-cache clean for these pages to avoid extra flushing. > + */ > + if (dir != DMA_TO_DEVICE && size >= PAGE_SIZE) { > + unsigned long pfn; > + size_t left = size; > + > + pfn = page_to_pfn(page) + off / PAGE_SIZE; > + off %= PAGE_SIZE; > + if (off) { > + pfn++; > + left -= PAGE_SIZE - off; > + } > + while (left >= PAGE_SIZE) { > + page = pfn_to_page(pfn++); > + set_bit(PG_dcache_clean, >flags); > + left -= PAGE_SIZE; > + } > + } > +} I _really_ don't want these exposed in any shape or form to driver code. I've seen too many hacks out there where people have gone under the cover of the APIs they should be using, and headed straight for the low-level functionality - adding function prototypes to get at stuff they have no business doing. Moving this here is just asking for it to be abused. > + > +void kernel_force_cache_clean(struct page *page, size_t size) > +{ > + __dma_page_cpu_to_dev(page, 0, size, DMA_BIDIRECTIONAL); > +} > + > +void kernel_force_cache_invalidate(struct page *page, size_t size) > +{ > + __dma_page_dev_to_cpu(page, 0, size, DMA_BIDIRECTIONAL); > +} Nothing in our implementation of these DMA operations guarantees that those mean "clean" and "invalidate". The DMA operations are there so that CPUs can implement whatever they need at the map and unmap times - and I've been very careful not to specify which cache operations are involved. For example, on older CPUs, __dma_page_dev_to_cpu() is almost always a no-op. If you want something that does something specific, then we need something designed to do something specific. Please don't re-use what you think will fit. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. ___ 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
On Fri, Jan 06, 2017 at 06:11:39PM -0800, Steve Longerbeam wrote: > +static int imxcsi2_get_fmt(struct v4l2_subdev *sd, > +struct v4l2_subdev_pad_config *cfg, > +struct v4l2_subdev_format *sdformat) > +{ > + struct imxcsi2_dev *csi2 = sd_to_dev(sd); > + > + sdformat->format = csi2->format_mbus; > + > + return 0; > +} Hi Steve, This isn't correct, and I suspect the other get_fmt implementations are the same - I've just checked imx-csi.c, and that also appears to have the same issue. When get_fmt() is called with sdformat->which == V4L2_SUBDEV_FORMAT_TRY, you need to return the try format rather than the current format. See the second paragraph of Documentation/media/uapi/v4l/dev-subdev.rst's "Format Negotiation" section, where it talks about using V4L2_SUBDEV_FORMAT_TRY with both VIDIOC_SUBDEV_G_FMT and VIDIOC_SUBDEV_S_FMT. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 00/24] i.MX Media Driver
On Thu, Feb 02, 2017 at 10:26:55AM -0800, Steve Longerbeam wrote: > On 02/02/2017 09:56 AM, Russell King - ARM Linux wrote: > >and for whatever reason we end up falling out through free_ring. This > >is VERY bad news, because it means that the ring which SMFC took a copy > >of is now freed beneath its feet. > > Yes, that is bad. That was a bug, if imx_media_dma_buf_queue_from_vb() > returned error, the ring should not have been freed, it should have only > returned the error. And further bad stuff happens from that point on. > > But all of this is gone in version 4. I think there's an error in how you think the queue_setup() works. camif_queue_setup() always returns the number of buffers between IMX_MEDIA_MIN_RING_BUFS and IMX_MEDIA_MAX_RING_BUFS. However, it seems that, looking through the videobuf2-core.c code, that the value is passed to __vb2_queue_alloc() to allocate the specified number of _additional_ buffers over and on-top of the existing q->num_buffers: static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory, unsigned int num_buffers, unsigned int num_planes, const unsigned plane_sizes[VB2_MAX_PLANES]) { for (buffer = 0; buffer < num_buffers; ++buffer) { ... vb->index = q->num_buffers + buffer; and int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory, unsigned int *count) { unsigned int num_buffers, allocated_buffers, num_planes = 0; ... num_buffers = min_t(unsigned int, *count, VB2_MAX_FRAME); num_buffers = max_t(unsigned int, num_buffers, q->min_buffers_needed); ... /* * Ask the driver how many buffers and planes per buffer it requires. * Driver also sets the size and allocator context for each plane. */ ret = call_qop(q, queue_setup, q, _buffers, _planes, plane_sizes, q->alloc_devs); if (ret) return ret; /* Finally, allocate buffers and video memory */ allocated_buffers = __vb2_queue_alloc(q, memory, num_buffers, num_planes, plane_sizes); or: int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory, unsigned int *count, unsigned requested_planes, const unsigned requested_sizes[]) { unsigned int num_planes = 0, num_buffers, allocated_buffers; ... num_buffers = min(*count, VB2_MAX_FRAME - q->num_buffers); if (requested_planes && requested_sizes) { num_planes = requested_planes; ... /* * Ask the driver, whether the requested number of buffers, planes per * buffer and their sizes are acceptable */ ret = call_qop(q, queue_setup, q, _buffers, _planes, plane_sizes, q->alloc_devs); if (ret) return ret; /* Finally, allocate buffers and video memory */ allocated_buffers = __vb2_queue_alloc(q, memory, num_buffers, num_planes, plane_sizes); It seems to me that if you don't take account of the existing queue size, your camif_queue_setup() has the side effect that each time either of these are called. Hence, the vb2 queue increases by the same amount each time, which is probably what you don't want. The documentation on queue_setup() leaves much to be desired: * @queue_setup:called from VIDIOC_REQBUFS() and VIDIOC_CREATE_BUFS() * handlers before memory allocation. It can be called * twice: if the original number of requested buffers * could not be allocated, then it will be called a * second time with the actually allocated number of * buffers to verify if that is OK. * The driver should return the required number of buffers * in \*num_buffers, the required number of planes per * buffer in \*num_planes, the size of each plane should be * set in the sizes\[\] array and optional per-plane * allocator specific device in the alloc_devs\[\] array. * When called from VIDIOC_REQBUFS,() \*num_planes == 0, * the driver has to use the currently configured format to * determine the plane sizes and \*num_buffers is the total * number of buffers that are being allocated. When called * from VIDIOC_CREATE_BUFS,() \*num_planes != 0 and it * describes the requested number of planes and sizes\[\] * contains the requested plane sizes. If either * \*num_planes or the requested sizes are invalid callback * must return %-EINVAL.
Re: [PATCH v3 00/24] i.MX Media Driver
On Thu, Feb 02, 2017 at 11:12:41AM -0800, Steve Longerbeam wrote: > Here is the current .queue_setup() op in imx-media-capture.c: > > static int capture_queue_setup(struct vb2_queue *vq, >unsigned int *nbuffers, >unsigned int *nplanes, >unsigned int sizes[], >struct device *alloc_devs[]) > { > struct capture_priv *priv = vb2_get_drv_priv(vq); > struct v4l2_pix_format *pix = >vdev.fmt.fmt.pix; > unsigned int count = *nbuffers; > > if (vq->type != V4L2_BUF_TYPE_VIDEO_CAPTURE) > return -EINVAL; > > if (*nplanes) { > if (*nplanes != 1 || sizes[0] < pix->sizeimage) > return -EINVAL; > count += vq->num_buffers; > } > > while (pix->sizeimage * count > VID_MEM_LIMIT) > count--; That's a weird way of writing: unsigned int max_num = VID_MEM_LIMIT / pix->sizeimage; count = max(count, max_num); -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 16/24] media: Add i.MX media core driver
On Fri, Jan 06, 2017 at 06:11:34PM -0800, Steve Longerbeam wrote: > +struct imx_media_dev { > + struct media_device md; > + struct v4l2_device v4l2_dev; This is similarly buggy. struct v4l2_device { struct device *dev; #if defined(CONFIG_MEDIA_CONTROLLER) struct media_device *mdev; #endif struct list_head subdevs; spinlock_t lock; char name[V4L2_DEVICE_NAME_SIZE]; void (*notify)(struct v4l2_subdev *sd, unsigned int notification, void *arg); struct v4l2_ctrl_handler *ctrl_handler; struct v4l2_prio_state prio; struct kref ref; void (*release)(struct v4l2_device *v4l2_dev); }; Notice the kref and release function. This is the only way the memory backing "struct v4l2_device" may be released. If you wish to embed this structure into another structure, then the lifetime of that other structure is determined by this one. IOW, when this release function is called, only then may you kfree() the memory backing struct imx_media_dev. > + struct device *dev; And... do you need all these struct device pointers? imxmd->dev = dev; imxmd->md.dev = dev; As media_device already contains a pointer, can't you re-use that? -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 16/24] media: Add i.MX media core driver
On Fri, Jan 06, 2017 at 06:11:34PM -0800, Steve Longerbeam wrote: > +/* register an internal subdev as a platform device */ > +static struct imx_media_subdev * > +add_internal_subdev(struct imx_media_dev *imxmd, > + const struct internal_subdev *isd, > + int ipu_id) > +{ > + struct imx_media_internal_sd_platformdata pdata; > + struct platform_device_info pdevinfo = {0}; > + struct imx_media_subdev *imxsd; > + struct platform_device *pdev; > + > + switch (isd->id->grp_id) { > + case IMX_MEDIA_GRP_ID_CAMIF0...IMX_MEDIA_GRP_ID_CAMIF1: > + pdata.grp_id = isd->id->grp_id + > + ((2 * ipu_id) << IMX_MEDIA_GRP_ID_CAMIF_BIT); > + break; > + default: > + pdata.grp_id = isd->id->grp_id; > + break; > + } > + > + /* the id of IPU this subdev will control */ > + pdata.ipu_id = ipu_id; > + > + /* create subdev name */ > + imx_media_grp_id_to_sd_name(pdata.sd_name, sizeof(pdata.sd_name), > + pdata.grp_id, ipu_id); > + > + pdevinfo.name = isd->id->name; > + pdevinfo.id = ipu_id * num_isd + isd->id->index; > + pdevinfo.parent = imxmd->dev; > + pdevinfo.data = > + pdevinfo.size_data = sizeof(pdata); > + pdevinfo.dma_mask = DMA_BIT_MASK(32); > + > + pdev = platform_device_register_full(); > + if (IS_ERR(pdev)) > + return ERR_CAST(pdev); > + > + imxsd = imx_media_add_async_subdev(imxmd, NULL, dev_name(>dev)); > + if (IS_ERR(imxsd)) > + return imxsd; > + > + imxsd->num_sink_pads = isd->num_sink_pads; > + imxsd->num_src_pads = isd->num_src_pads; > + > + return imxsd; > +} You seem to create platform devices here, but I see nowhere that you ever remove them - so if you get to the lucky point of being able to rmmod imx-media and then try to re-insert it, you end up with a load of kernel warnings, one for each device created this way, and platform_device_register_full() fails: WARNING: CPU: 0 PID: 2143 at /home/rmk/git/linux-rmk/fs/sysfs/dir.c:31 sysfs_warn_dup+0x64/0x80 sysfs: cannot create duplicate filename '/devices/soc0/soc/soc:media@0/imx-ipuv3-smfc.2' Modules linked in: imx_media(C+) rfcomm bnep bluetooth nfsd imx_camif(C) imx_ic(C) imx_smfc(C) caam_jr snd_soc_imx_sgtl5000 snd_soc_fsl_asoc_card uvcvideo snd_soc_imx_spdif imx_mipi_csi2(C) imx_media_common(C) snd_soc_imx_audmux imx219 snd_soc_sgtl5000 video_multiplexer imx_sdma caam imx2_wdt rc_cec coda v4l2_mem2mem videobuf2_v4l2 snd_soc_fsl_ssi snd_soc_fsl_spdif videobuf2_dma_contig imx_pcm_dma videobuf2_core videobuf2_vmalloc videobuf2_memops imx_thermal dw_hdmi_cec dw_hdmi_ahb_audio etnaviv fuse rc_pinnacle_pctv_hd [last unloaded: imx_media] CPU: 0 PID: 2143 Comm: modprobe Tainted: G C 4.10.0-rc6+ #2103 Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree) Backtrace: [] (dump_backtrace) from [] (show_stack+0x18/0x1c) r6:6013 r5: r4: r3: [] (show_stack) from [] (dump_stack+0xa4/0xdc) [] (dump_stack) from [] (__warn+0xdc/0x108) r6:c08ad998 r5: r4:cf4e9a78 r3:ec984980 [] (__warn) from [] (warn_slowpath_fmt+0x40/0x48) r10:e5800010 r8:ef1fa818 r7:ef1fa810 r6:ef202300 r5:e9809000 r4:ee868000 [] (warn_slowpath_fmt) from [] (sysfs_warn_dup+0x64/0x80) r3:ee868000 r2:c08ad9c0 [] (sysfs_warn_dup) from [] (sysfs_create_dir_ns+0x90/0x98) r6:ffef r5:ef202300 r4:eb5e3418 [] (sysfs_create_dir_ns) from [] (kobject_add_internal+0xa4/0x2d8) r6:ef1fa818 r5: r4:eb5e3418 [] (kobject_add_internal) from [] (kobject_add+0x50/0x98) r8:bf1f9018 r7:ef1fa810 r6:ef1fa818 r5: r4:eb5e3418 [] (kobject_add) from [] (device_add+0xc8/0x538) r3:ef1fa834 r2: r6:eb5e3418 r5: r4:eb5e3410 [] (device_add) from [] (platform_device_add+0xb0/0x214) r10:e5800010 r9: r8:bf1f9018 r7:e5806fd4 r6:eb5e3410 r5:eb5e3400 r4: [] (platform_device_add) from [] (platform_device_register_full+0xe8/0x110) r7:e5806fd4 r6:0002 r5:eb5e3400 r4:cf4e9c18 [] (platform_device_register_full) from [] (add_ipu_internal_subdevs+0x128/0x2c8 [imx_media]) r5:bf1f9000 r4:0918 [] (add_ipu_internal_subdevs [imx_media]) from [] (imx_media_add_internal_subdevs+0x2c/0x70 [imx_media]) r10:0048 r9:f31ceef8 r8:ef7f1d94 r7:e5800230 r6:e5800200 r5:e5800010 r4:cf4e9c90 [] (imx_media_add_internal_subdevs [imx_media]) from [] (imx_media_probe+0xc4/0x1c0 [imx_media]) r5: r4:e5800010 [] (imx_media_probe [imx_media]) from [] (platform_drv_probe+0x58/0xb8) r8: r7:bf1fbd48 r6:fdfb r5:ef1fa810 r4:fffe [] (platform_drv_probe) from [] (driver_probe_device+0x204/0x2c8) r7:bf1fbd48 r6: r5:c1419de8 r4:ef1fa810 [] (driver_probe_device) from [] (__driver_attach+0xbc/0xc0) r10:0124 r8:0001 r7: r6:ef1fa844 r5:bf1fbd48 r4:ef1fa810 [] (__driver_attach) from [] (bus_for_each_dev+0x5c/0x90) r6:c0418d54 r5:bf1fbd48 r4:
Re: [PATCH v3 20/24] media: imx: Add Camera Interface subdev driver
On Fri, Jan 06, 2017 at 06:11:38PM -0800, Steve Longerbeam wrote: > +struct camif_priv { > + struct device *dev; > + struct video_devicevfd; You can't do this. > +static struct video_device camif_videodev = { > + .fops = _fops, > + .ioctl_ops = _ioctl_ops, > + .minor = -1, > + .release= video_device_release, > + .vfl_dir= VFL_DIR_RX, > + .tvnorms= V4L2_STD_NTSC | V4L2_STD_PAL | V4L2_STD_SECAM, > +}; > +static int camif_probe(struct platform_device *pdev) > +{ > + struct imx_media_internal_sd_platformdata *pdata; > + struct camif_priv *priv; > + struct video_device *vfd; > + int ret; > + > + priv = devm_kzalloc(>dev, sizeof(*priv), GFP_KERNEL); > + if (!priv) > + return -ENOMEM; You kmalloc this structure, so this structure has the lifetime of the driver being bound to the platform device. > + vfd = >vfd; > + *vfd = camif_videodev; However, "*vfd" contains a struct device, and you _correctly_ set the release function for "*vfd" to video_device_release via camif_videodev. However, if you try to rmmod imx-media, then you end up with a kernel warning that you're freeing memory containing a held lock, and later chaos ensues because kmalloc has been corrupted. The root cause of this is embedding the device structure within the video_device into the driver's private data. *Any* structure what so ever that contains a kref is reference counted, and that includes struct device, and therefore also includes struct video_device. What that means is that its lifetime is _not_ under _your_ control, and you may not free it except through its release function (which is video_device_release().) However, that also tries to kfree (with an offset of 4) your private data, which results in the warning and the corrupted kmalloc free lists. The solution is simple, make "vfd" a pointer in your private data structure and kmalloc() it separately, letting video_device_release() kfree() that data when it needs to. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 00/24] i.MX Media Driver
On Mon, Jan 30, 2017 at 05:22:01PM -0800, Steve Longerbeam wrote: > Edit: I see a subdev that is missing: the video mux. Did you enable > CONFIG_VIDEO_MULTIPLEXER? Yes, and that's where the problem is - the video-multiplexer is missing the module aliases to allow it to be automatically loaded. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 17/24] media: imx: Add CSI subdev driver
On Fri, Jan 06, 2017 at 06:11:35PM -0800, Steve Longerbeam wrote: > This is a media entity subdevice for the i.MX Camera > Serial Interface module. > > Signed-off-by: Steve Longerbeam> --- The lack of s_frame_interval/g_frame_interval in this driver means: media-ctl -v -d /dev/media1 --set-v4l2 '"imx6-mipi-csi2":1[fmt:SGBRG8/512x512@1/30]' Opening media device /dev/media1 Enumerating entities Found 29 entities Enumerating pads and links Setting up format SGBRG8 512x512 on pad imx6-mipi-csi2/1 Format set: SGBRG8 512x512 Setting up frame interval 1/30 on entity imx6-mipi-csi2 Unable to set frame interval: Inappropriate ioctl for device (-25)Unable to setup formats: Inappropriate ioctl for device (25) which causes the setup of the next element in the chain to also fail (because the above media-ctl command doesn't set the sink on the ipu1 csi0 mux.) It seems to me that without the frame interval supported throughout the chain, there's no way for an application to properly negotiate the capture parameters via the "try" mechanism, since it has no idea whether the frame rate it wants is supported throughout the subdev chain. Eg, the sensor may be able to do 100fps but there could be something in the pipeline that restricts it due to bandwidth. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 00/24] i.MX Media Driver
On Mon, Jan 30, 2017 at 05:22:01PM -0800, Steve Longerbeam wrote: > I'm also having trouble finding a datasheet for it, but from what > I've read, it has a MIPI CSI-2 interface. It should work fine as long > as it presents a single source pad, registers asynchronously, and > sets its entity function to MEDIA_ENT_F_CAM_SENSOR. Yes, it is MIPI CSI-2, and yes it has a single source pad, registers asynchronously, but that's about as far as it goes. The structure is a camera sensor followed by some processing. So just like the smiapp code, I've ended up with multiple subdevs describing each stage of the sensors pipeline. Just like smiapp, the camera sensor block (which is the very far end of the pipeline) is marked with MEDIA_ENT_F_CAM_SENSOR. However, in front of that is the binner, which just like smiapp gets a separate entity. It's this entity which is connected to the mipi-csi2 subdev. Unlike smiapp, which does not set an entity function, I set my binner entity as MEDIA_ENT_F_PROC_VIDEO_SCALER on the basis that that is what V4L2 documentation recommend: - .. row 27 .. _MEDIA-ENT-F-PROC-VIDEO-SCALER: - ``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. This causes attempts to configure the ipu1_csi0 interface to fail: media-ctl -v -d /dev/media1 --set-v4l2 '"ipu1_csi0":1[fmt:SGBRG8/512x512@1/30]' Opening media device /dev/media1 Enumerating entities Found 29 entities Enumerating pads and links Setting up format SGBRG8 512x512 on pad ipu1_csi0/1 Unable to set format: No such device (-19) Unable to setup formats: No such device (19) and in the kernel log: ipu1_csi0: no sensor attached And yes, I already know that my next problem is going to be that the bayer formats are not supported in your driver (just like Philipp's driver) but adding them should not be difficult... but only once this issue is resolved. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 17/24] media: imx: Add CSI subdev driver
On Fri, Jan 06, 2017 at 06:11:35PM -0800, Steve Longerbeam wrote: > +static int csi_link_validate(struct v4l2_subdev *sd, > + struct media_link *link, > + struct v4l2_subdev_format *source_fmt, > + struct v4l2_subdev_format *sink_fmt) > +{ > + struct csi_priv *priv = v4l2_get_subdevdata(sd); > + bool is_csi2; > + int ret; > + > + ret = v4l2_subdev_link_validate_default(sd, link, source_fmt, sink_fmt); > + if (ret) > + return ret; > + > + priv->sensor = __imx_media_find_sensor(priv->md, >sd.entity); > + if (IS_ERR(priv->sensor)) { > + v4l2_err(>sd, "no sensor attached\n"); > + ret = PTR_ERR(priv->sensor); > + priv->sensor = NULL; > + return ret; > + } > + > + ret = v4l2_subdev_call(priv->sensor->sd, video, g_mbus_config, > +>sensor_mbus_cfg); > + if (ret) > + return ret; > + > + is_csi2 = (priv->sensor_mbus_cfg.type == V4L2_MBUS_CSI2); > + > + if (is_csi2) { > + int vc_num = 0; > + /* > + * NOTE! It seems the virtual channels from the mipi csi-2 > + * receiver are used only for routing by the video mux's, > + * or for hard-wired routing to the CSI's. Once the stream > + * enters the CSI's however, they are treated internally > + * in the IPU as virtual channel 0. > + */ > +#if 0 > + vc_num = imx_media_find_mipi_csi2_channel(priv->md, > + >sd.entity); > + if (vc_num < 0) > + return vc_num; > +#endif > + ipu_csi_set_mipi_datatype(priv->csi, vc_num, > + >format_mbus[priv->input_pad]); This seems (at least to me) to go against the spirit of the subdev negotiation. Here, you seem to bypass the negotiation performed between the CSI and the attached device, wanting to grab the format from the CSI2 sensor directly. That bypasses negotiation performed at the CSI2 subdev and video muxes. The same goes for the frame rate monitoring code - imho, the frame rate should be something that results from negotiation with the neighbouring element, not by poking at some entity that is several entities away. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. ___ 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 Tue, Jan 31, 2017 at 05:02:40PM -0800, Steve Longerbeam wrote: > But this also puts a requirement on MIPI sensors that s_power(ON) > should only place the D_PHY in LP-11, and _not_ start the clock lane. > But perhaps that is correct behavior anyway. If the CSI2 DPHY is held in reset state, it shouldn't matter what the sensor does. In the case of IMX219, it needs a full setup of the device, including enabling it to stream (so it starts the clock lane etc) in order to get it into LP-11 state. Merely disabling the XCLR signal leaves the lanes grounded. I do seem to remember reading in one of the MIPI specs that this is rather expected behaviour, though I can't point at a paragraph this late in the night. So, the only way to satisfy these requirements is this order: - assert PHY reset signals (so blocking any activity on the CSI lanes) - initialise the sensor (including allowing it to start streaming and then stopping the stream - at that point, the lanes will be in LP-11.) - deassert the resets as per the iMX6 documentation and follow the remaining procedure. I'll look at your other points tomorrow. Thanks. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 17/24] media: imx: Add CSI subdev driver
On Tue, Jan 31, 2017 at 04:31:55PM -0800, Steve Longerbeam wrote: > > > On 01/31/2017 03:20 AM, Russell King - ARM Linux wrote: > >On Fri, Jan 06, 2017 at 06:11:35PM -0800, Steve Longerbeam wrote: > >>+static int csi_link_validate(struct v4l2_subdev *sd, > >>+struct media_link *link, > >>+struct v4l2_subdev_format *source_fmt, > >>+struct v4l2_subdev_format *sink_fmt) > >>+{ > >>+ struct csi_priv *priv = v4l2_get_subdevdata(sd); > >>+ bool is_csi2; > >>+ int ret; > >>+ > >>+ ret = v4l2_subdev_link_validate_default(sd, link, source_fmt, sink_fmt); > >>+ if (ret) > >>+ return ret; > >>+ > >>+ priv->sensor = __imx_media_find_sensor(priv->md, >sd.entity); > >>+ if (IS_ERR(priv->sensor)) { > >>+ v4l2_err(>sd, "no sensor attached\n"); > >>+ ret = PTR_ERR(priv->sensor); > >>+ priv->sensor = NULL; > >>+ return ret; > >>+ } > >>+ > >>+ ret = v4l2_subdev_call(priv->sensor->sd, video, g_mbus_config, > >>+ >sensor_mbus_cfg); > >>+ if (ret) > >>+ return ret; > >>+ > >>+ is_csi2 = (priv->sensor_mbus_cfg.type == V4L2_MBUS_CSI2); > >>+ > >>+ if (is_csi2) { > >>+ int vc_num = 0; > >>+ /* > >>+* NOTE! It seems the virtual channels from the mipi csi-2 > >>+* receiver are used only for routing by the video mux's, > >>+* or for hard-wired routing to the CSI's. Once the stream > >>+* enters the CSI's however, they are treated internally > >>+* in the IPU as virtual channel 0. > >>+*/ > >>+#if 0 > >>+ vc_num = imx_media_find_mipi_csi2_channel(priv->md, > >>+ >sd.entity); > >>+ if (vc_num < 0) > >>+ return vc_num; > >>+#endif > >>+ ipu_csi_set_mipi_datatype(priv->csi, vc_num, > >>+ >format_mbus[priv->input_pad]); > >This seems (at least to me) to go against the spirit of the subdev > >negotiation. Here, you seem to bypass the negotiation performed > >between the CSI and the attached device, wanting to grab the > >format from the CSI2 sensor directly. That bypasses negotiation > >performed at the CSI2 subdev and video muxes. > > This isn't so much grabbing a pad format, it is determining > which source pad at the imx6-mipi-csi2 receiver subdev is > reached from this CSI (which determines the virtual channel > the CSI is receiving). > > If there was a way to determine the vc# via a status register > in the CSI, that would be perfect, but there isn't. In fact, the CSIs > seem to be ignoring the meta-data bus sent by the CSI2IPU gasket > which contains this info, or that bus is not being routed to the CSIs. > As the note says, the CSIs only accept vc0 at the CSI_MIPI_DI register. > Any other value programmed there results in no data from the CSI. > > And even the presence of CSI_MIPI_DI register makes no sense to me, > the CSIs are given the vc# and MIPI datatype from the CSI2IPU meta-data > bus, so I don't understand why it needs to be told. The CSI_MIPI_DI register selects the data stream we want from the CSI2 camera. CSI2 cameras can produce many different data streams - for example, a CSI2 camera can, for the same image, produce a YUV encoded frame and a jpeg-encoded frame. These are sent over the CSI2 serial bus using two different data types. The CSI2IPU converts the serial data streams into a parallel data stream, feeding that into the CSI layer. The CSI layer, in conjunction with the CSI_MIPI_DI register, selects one of these streams to pass on to the SMFC and other blocks. >From what I've read, the CSI can also be programmed to pass other streams on as well, but I've never tried that. In my particular case, the IMX219 camera produces a complete frame using the RAW8 or RAW10 MIPI data type, and also produces two lines of register data per frame, encoded using another data type. I think it should be possible to program the CSI to pass this other data on as "generic data" through a different FIFO and have it written to memory, which makes it possible to know the camera settings for each frame. (This isn't something I'm interested in though, I'm just using it as an example of why, possibly, there's that register in the CSI block.) -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 00/24] i.MX Media Driver
On Tue, Jan 31, 2017 at 03:43:22PM -0800, Steve Longerbeam wrote: > > > On 01/31/2017 03:00 AM, Russell King - ARM Linux wrote: > >Just like smiapp, the camera sensor block (which is the very far end > >of the pipeline) is marked with MEDIA_ENT_F_CAM_SENSOR. However, in > >front of that is the binner, which just like smiapp gets a separate > >entity. It's this entity which is connected to the mipi-csi2 subdev. > > wow, ok got it. > > So the sensor pipeline and binner, and the OF graph connecting > them, are described in the device tree I presume. No - because the binner and sensor are on the same die, it's even one single device, there's no real separation of the two devices. The reason there's no real separation is because the binning is done as part of the process of reading the array - sometimes before the analog voltage is converted to its digital pixel value representation. The separation comes because of the requirements of the v4l2 media subdevs, which requires scalers to have a sink pad and a source pad. (Please see the v4l2 documentation, I think I've already quoted this: .. _MEDIA-ENT-F-PROC-VIDEO-SCALER: - ``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. (Oh yes, I see it was the mail to which you were replying to...) So, in order to configure the scaling (which can be none, /2 and /4) we have to expose two subdevs - one which is the scaler, and has a source pad connected to the upstream (in this case CSI2 receiver) and a sink pad immutably connected to the camera sensor. Since the split is entirely down to the V4L2 implementation requirements, it's not something that should be reflected in DT - we don't put implementation details in DT. It's just the same (as I've already said) as the SMIAPP camera driver, the reason I'm pointing you towards that is because this is an already mainlined camera driver which nicely illustrates how my driver is structured from the v4l2 subdev API point of view. > The OF graph AFAIK, has no information about which ports are sinks > and which are sources, so of_parse_subdev() tries to determine that > based on the compatible string of the device node. So ATM > of_parse_subdev() assumes there is nothing but the imx6-mipi-csi2, > video-multiplexer, and camera sensors upstream from the CSI ports > in the OF graph. > > I realize that's not a robust solution, and is the reason for the > "no sensor attached" below. > > Is there any way to determine from the OF graph the data-direction > of a port (whether it is a sink or a source)? If so it will make > of_parse_subdev() much more robust. I'm not sure why you need to know the data direction. I think the issue here is how you're going about dealing with the subdevs. Here's the subdev setup: +-camera+ | pixel array -> binner |---> csi2 --> ipu1csi0 mux --> csi0 --> smfc --> idmac +---+ How the subdevs are supposed to work is that you start from the first subdev in sequence (in this case the pixel array) and negotiate with the driver through the TRY formats on its source pad, as well as negotiating with the sink pad of the directly neighbouring subdev. The neighbouring subdev propagates the configuration in a driver specific manner from its source pad to the sink pad, giving a default configuration at its source. This process repeats throughout the chain all the way up to the bridge device. Now, where things go wrong is that you want to know what each type of these subdevs are, and the reason you want that is so you can do this (for example - I know similar stuff happens with the "sensor" stuff further up the chain as well): +-camera+ | pixel array -> binner |---> csi2 --> ipu1csi0 mux --> csi0 --> smfc --> idmac +---+| ^--I-want-your-bus-format-and-frame-rate---' which goes against the negotiation mechanism of v4l2 subdevs. This is broken - it bypass the subdev negotiation that has been performed on the intervening subdevs between the "sensor" and the csi0 subdevs, so if there were a subdev in that chain that (eg) reduced the frame rate by discarding the odd frames, you'd be working with the wrong frame interval for your frame interval monitor at csi. Note that the "MEDIA_ENT_F_PROC_VIDEO_SCALER" subde
Re: [PATCH v3 00/24] i.MX Media Driver
On Wed, Feb 01, 2017 at 10:30:57AM +0100, Philipp Zabel wrote: > On Tue, 2017-01-31 at 17:26 -0800, Steve Longerbeam wrote: > [...] > > right, need to fix that. Probably by poking the attached > > source subdev (csi or prpenc/vf) for its supported formats. > > You are right, in bayer/raw mode only one specific format should be > listed, depending on the CSI output pad format. It depends what Steve means by "source subdev". It should be the next sub-device below the bridge - if we have this setup of subdev's: ---> CSI ---> SMFC ---> IDMAC then the format configured at the SMFC's output pad is what matters, not what was configured at CSI. It's the responsibility of SMFC and CSI to make sure that their source pads are configured with a compatible format for their corresponding source pad, and it's also the sink subdev's responsibility to check that the configuration across a link is valid (possibly via v4l2_subdev_link_validate(), or a more intensive or relaxed test if required.) For example: - when CSI's source pad is configured with a RGGB output format, userspace media-ctl will also set that on SMFC's sink pad. - when SMFC's sink pad is configured, SMFC should configure it's source pad with an identical format (RGGB). - when SMFC's source pad is configured, it should refuse to change the format, because SMFC can't modify pixel the format - it's just a buffer. When starting to stream (Documentation/media/kapi/v4l2-subdev.rst) the link validation function is called, and: - the SMFC driver's link_validate function will be called to validate the CSI -> SMFC link. This allows the SMFC to be sure that there's a compatible configuration - and, since the link does not allow format conversion, it should verify that the format on the CSI's source pad is the same as SMFC's sink pad. Not only does this match what the hardware's doing, it also means that, because there's no format conversion between the CSI's hardware output and IDMAC, we don't need to care about trying to fetch the CSI's source pad configuration from the IDMAC end - we can fetch that information from our neighbour's SMFC's source pad _or_ our own sink pad if we have one. To see why this is an important, consider what the effect would be if SMFC did have the capability to change the pixel format. That means the format presented to the IDMAC block would depend on the configuration of SMFC, and the CSI's source pad format is no longer relevant to IDMAC. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 00/24] i.MX Media Driver
On Tue, Jan 31, 2017 at 05:54:52PM -0800, Steve Longerbeam wrote: > On 01/31/2017 04:23 PM, Russell King - ARM Linux wrote: > First, thank you for the explanation, it clears up a lot. > > But of_parse_subdev() is used to parse the OF graph starting > from the CSI ports, to discover all the nodes to add to subdev > async registration. It also forms the media link info to be used > later to create the media graph, after all discovered subdevs > have come online (registered themselves). This happens at > driver load time, it doesn't have anything to do with pad > negotiation. Right, but I'm discussing why you need to know which is the sensor subdev, and why you need to get parameters from the sensor subdev. > > I think the > >issue here is how you're going about dealing with the subdevs. > >Here's the subdev setup: > > > >+-camera+ > >| pixel array -> binner |---> csi2 --> ipu1csi0 mux --> csi0 --> smfc --> > >idmac > >+---+ > > > >How the subdevs are supposed to work is that you start from the first > >subdev in sequence (in this case the pixel array) and negotiate with > >the driver through the TRY formats on its source pad, as well as > >negotiating with the sink pad of the directly neighbouring subdev. > > > >The neighbouring subdev propagates the configuration in a driver > >specific manner from its source pad to the sink pad, giving a default > >configuration at its source. > > Let me try to re-phrase. You mean the subdev's set_fmt(), when > configured its source pad(s), should call set_fmt() at the connected > sink subdev to automatically propagate the format to the sink's pad? No. For any individual subdev, if you configure it's _sink_ then it should propagate the configuration to its _source_, potentially modifying the configuration according to its function. It should never forward the configuration to the other side of any links. The responsibility for setting up the neighbours source pad is the userspace media application. See Documentation/media/uapi/v4l/dev-subdev.rst and the section named "Format Negotiation". -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 00/24] i.MX Media Driver
On Tue, Jan 31, 2017 at 11:09:24AM +0100, Philipp Zabel wrote: > On Mon, 2017-01-30 at 13:06 +0000, Russell King - ARM Linux wrote: > > To help illustrate my point, consider the difference between > > MEDIA_BUS_FMT_RGB565_1X16 and MEDIA_BUS_FMT_RGB565_2X8_BE or > > MEDIA_BUS_FMT_RGB565_2X8_LE. RGB565_1X16 means 1 pixel over an effective > > 16-bit wide bus (if it's not 16-bit, then it has to be broken up into > > separate "samples".) RGB565_2X8 means 1 pixel as two 8-bit samples. > > > > So, the 10-bit bayer is 1 pixel as 1.25 bytes. Or is it, over a serial > > bus. Using the RGB565 case, 10-bit bayer over a 4 lane CSI bus becomes > > interesting: > > > > first byte 2nd 3rd > > lane 1 P0 9:2 S0 P7 9:2 > > lane 2 P1 9:2 P4 9:2 S1 > > lane 3 P2 9:2 P5 9:2 P8 9:2 > > lane 4 P3 9:2 P6 9:2 P9 9:2 > > > > S0 = P0/P1/P2/P3 least significant two bits > > S1 = P4/P5/P6/P7 least significant two bits > > > > or 2 lane CSI: > > first byte 2nd 3rd 4th 5th > > lane 1 P0 9:2 P2 S0 P5 P7 > > lane 2 P1 9:2 P3 P4 P6 S1 > > > > or 1 lane CSI: > > lane 1 P0 P1 P2 P3 S0 P4 P5 P6 P7 S1 P8 P9 ... > > These do look like three different media bus formats to me. This isn't limited to the serial side - the parallel bus side between the CSI2 interface and CSI2IPU wrapper, and the CSI2IPU wrapper and the CS0/1 interfaces is much the same with 10-bit bayer. Think of the CSI2 <-> CSI2IPU bit as the 4-lane case, lane 0 ending up on the least significant 8 bits of the 32-bit bus, lane 3 on the top 8-bits. Post CSI2IPU, it talks about a 16-bit bus in the diagrams, so that's kind of the 2-lane case above... -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 20/24] media: imx: Add Camera Interface subdev driver
On Fri, Jan 20, 2017 at 03:38:28PM +0100, Hans Verkuil wrote: > Should be set to something like 'platform:imx-media-camif'. v4l2-compliance > should complain about this. ... and more. Driver Info: Driver name : imx-media-camif Card type : imx-media-camif Bus info : Driver version: 4.10.0 Capabilities : 0x8421 Video Capture Streaming Extended Pix Format Device Capabilities Device Caps : 0x0421 Video Capture Streaming Extended Pix Format Compliance test for device /dev/video3 (not using libv4l2): Required ioctls: fail: v4l2-compliance.cpp(244): string empty fail: v4l2-compliance.cpp(297): check_ustring(vcap.bus_info, sizeof(vcap.bus_info)) test VIDIOC_QUERYCAP: FAIL Allow for multiple opens: test second video open: OK fail: v4l2-compliance.cpp(244): string empty fail: v4l2-compliance.cpp(297): check_ustring(vcap.bus_info, sizeof(vcap.bus_info)) test VIDIOC_QUERYCAP: FAIL test VIDIOC_G/S_PRIORITY: OK Debug ioctls: test VIDIOC_DBG_G/S_REGISTER: OK test VIDIOC_LOG_STATUS: OK (Not Supported) Input ioctls: test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported) test VIDIOC_G/S_FREQUENCY: OK (Not Supported) test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported) test VIDIOC_ENUMAUDIO: OK (Not Supported) fail: v4l2-test-input-output.cpp(382): std == 0 fail: v4l2-test-input-output.cpp(437): invalid attributes for input 0 test VIDIOC_G/S/ENUMINPUT: FAIL test VIDIOC_G/S_AUDIO: OK (Not Supported) Inputs: 0 Audio Inputs: 0 Tuners: 0 Output ioctls: test VIDIOC_G/S_MODULATOR: OK (Not Supported) test VIDIOC_G/S_FREQUENCY: OK (Not Supported) test VIDIOC_ENUMAUDOUT: OK (Not Supported) test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported) test VIDIOC_G/S_AUDOUT: OK (Not Supported) Outputs: 0 Audio Outputs: 0 Modulators: 0 Input/Output configuration ioctls: test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported) test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported) test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported) test VIDIOC_G/S_EDID: OK (Not Supported) Control ioctls: test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK test VIDIOC_QUERYCTRL: OK test VIDIOC_G/S_CTRL: OK test VIDIOC_G/S/TRY_EXT_CTRLS: OK fail: v4l2-test-controls.cpp(779): subscribe event for control 'Camera Controls' failed test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: FAIL test VIDIOC_G/S_JPEGCOMP: OK (Not Supported) Standard Controls: 13 Private Controls: 0 Format ioctls: test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK test VIDIOC_G/S_PARM: OK (Not Supported) test VIDIOC_G_FBUF: OK (Not Supported) fail: v4l2-test-formats.cpp(414): unknown pixelformat 42474752 for buftype 1 test VIDIOC_G_FMT: FAIL test VIDIOC_TRY_FMT: OK (Not Supported) test VIDIOC_S_FMT: OK (Not Supported) test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported) Codec ioctls: test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported) test VIDIOC_G_ENC_INDEX: OK (Not Supported) test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported) Buffer ioctls: test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK fail: v4l2-test-buffers.cpp(500): q.has_expbuf(node) test VIDIOC_EXPBUF: FAIL Total: 39, Succeeded: 33, Failed: 6, Warnings: 0 Not all of these may be a result of Steve's code - this is running against my gradually modified version to support bayer formats (which seems to be the cause of the v4l2-test-formats.cpp failure... for some reason the driver isn't enumerating all the formats.) And that reason is the way that the formats are enumerated: static int camif_enum_fmt_vid_cap(struct file *file, void *fh, struct v4l2_fmtdesc *f) { const struct imx_media_pixfmt *cc; u32 code; int ret; ret = imx_media_enum_format(, f->index, true, true); if (ret) return ret; cc = imx_media_find_format(0, code, true, true); if (!cc) return -EINVAL; When imx_media_enum_format() hits this entry in the table: }, { .fourcc = V4L2_PIX_FMT_BGR24, .cs = IPUV3_COLORSPACE_RGB, .bpp= 24, }, { becaues there's no .codes defined: int imx_media_enum_format(u32 *code, u32 index, bool allow_rgb, bool allow_planar) {
Re: [PATCH v3 00/24] i.MX Media Driver
On Tue, Jan 31, 2017 at 02:35:00PM +0100, Philipp Zabel wrote: > On Tue, 2017-01-31 at 13:14 +0000, Russell King - ARM Linux wrote: > > This isn't limited to the serial side - the parallel bus side between > > the CSI2 interface and CSI2IPU wrapper, and the CSI2IPU wrapper and > > the CS0/1 interfaces is much the same with 10-bit bayer. > > > > Think of the CSI2 <-> CSI2IPU bit as the 4-lane case, lane 0 ending > > up on the least significant 8 bits of the 32-bit bus, lane 3 on the > > top 8-bits. > > > > Post CSI2IPU, it talks about a 16-bit bus in the diagrams, so that's > > kind of the 2-lane case above... > > You are right, on the parallel buses the format most definitely is not > MEDIA_BUS_FMT_SBGGR10_1X10. We don't have any representation of the > 32-bit bus between CSI2 host and CSI2IPU gasket because we model the two > as a single entity, but the four 16-bit parallel buses between the > CSI2IPU gasket and the IPU1/2 CSI0/1 probably should be set to a custom > format describing this accurately. Yep. I should also point out that there's a very odd transformation going on somewhere, and I don't yet know where. The sensor is definitely outputting GBRG format, but what seems to get written into memory is RGGB format. It's somewhere post CSI, because when I was using the (broken) CSI compander with 10 bit bayer, the red compander channel affected the red channel output from the camera, but changed the green component written to memory... it's very much like either the first line gets lost somewhere, or the odd/even lines are transposed. It could also be a gstreamer bug. As I say, it's not something I've looked into deeply enough yet... too many other issues to chase down! -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 00/24] i.MX Media Driver
On Tue, Jan 31, 2017 at 03:25:10PM +0100, Philipp Zabel wrote: > On Tue, 2017-01-31 at 14:54 +0100, Philipp Zabel wrote: > > Hi Steve, > > > > I have just tested the imx-media-staging-md-wip branch on a Nitrogen6X > > with a tc358743 (BD_HDMI_MIPI HDMI to MIPI CSI-2 receiver board). Some > > observations: > > > > # Link pipeline > > media-ctl -l "'tc358743 1-000f':0->'imx6-mipi-csi2':0[1]" > > media-ctl -l "'imx6-mipi-csi2':1->'ipu1_csi0_mux':0[1]" > > media-ctl -l "'ipu1_csi0_mux':2->'ipu1_csi0':0[1]" > > media-ctl -l "'ipu1_csi0':2->'ipu1_csi0 capture':0[1]" > > > > # Provide an EDID to the HDMI source > > v4l2-ctl -d /dev/v4l-subdev2 --set-edid=file=edid-1080p.hex > > # At this point the HDMI source is enabled and sends a 1080p60 signal > > # Configure detected DV timings > > media-ctl --set-dv "'tc358743 1-000f':0" > > > > # Set pad formats > > media-ctl --set-v4l2 "'tc358743 1-000f':0[fmt:UYVY/1920x1080]" > > media-ctl --set-v4l2 "'imx6-mipi-csi2':1[fmt:UYVY2X8/1920x1080]" > > media-ctl --set-v4l2 "'ipu1_csi0_mux':2[fmt:UYVY2X8/1920x1080]" > > I noticed this seems to get ignored. The format is incorrectly set to > UYVY even though I request UYVY2X8 (see CSI2IPU chapter, Figure 19-10. > YUV422-8 data reception in the reference manual), but it seems to work > anyway. That's because the driver at imx-csi level bypasses the proper media pad formats on its sink pads, and instead goes poking about at the "sensor" to get the format. This is one of the reasons it wants to know which entity is the "sensor". The "sensor" stuff in there needs to be scrapped... -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 00/24] i.MX Media Driver
On Wed, Feb 01, 2017 at 11:42:31AM +0100, Philipp Zabel wrote: > On Wed, 2017-02-01 at 10:11 +0000, Russell King - ARM Linux wrote: > Right, it's just that in the latest version there is no v4l2_subdev for > fifos and idmac. There is the capture interface entity that represents > one of the IDMAC write channels, but that doesn't have a pad and format > configuration. > The SMFC entity was removed because the fifo can be considered part of > the link between CSI and IDMAC. There is no manual configuration > necessary as the SMFC itself can't do anything to the data that flows > through it. There is no reason to present it to userspace as a no-op > entity. > So in the direct CSI -> SMFC -> IDMAC case, the CSI source pad now is > the nearest neighbor pad to the IDMAC capture video device. Ok, that sounds fine then. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. ___ 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
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. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 06/24] ARM: dts: imx6-sabrelite: add OV5642 and OV5640 camera sensors
On Sun, Feb 05, 2017 at 05:24:30PM +0200, Laurent Pinchart wrote: > Hi Russell, > > On Monday 30 Jan 2017 22:51:33 Russell King - ARM Linux wrote: > > > + ov5640_to_mipi_csi: endpoint@1 { > > > + reg = <1>; > > > + remote-endpoint = > <_csi_from_mipi_sensor>; > > > + data-lanes = <0 1>; > > > + clock-lanes = <2>; > > > > How do you envision a four-lane sensor being described? > > > > data-lanes = <0 1 3 4>; > > clock-lanes = <2>; > > > > ? > > > > The binding document for video-interfaces.txt says: > > > > - clock-lanes: an array of physical clock lane indexes. Position of an entry > > determines the logical lane number, while the value of an entry indicates > > physical lane, e.g. for a MIPI CSI-2 bus we could have "clock-lanes = > > <0>;", which places the clock lane on hardware lane 0. This property is > > valid for serial busses only (e.g. MIPI CSI-2). Note that for the MIPI > > CSI-2 bus this array contains only one entry. > > > > So I think you need to have a good reason to make the clock lane non-zero. > > The purpose of the data-lanes and clock-lanes properties is to describe lane > assignment for hardware that supports lane routing. As far as I know the > OV5640 doesn't support lane routing and has dedicated pins for the clock and > data lanes. The data-lanes and clock-lanes properties should probably not be > specified at all. You need at least data-lanes so you know how many data lanes are wired between the camera and the mipi csi2 receiver. Just because a camera has (eg) four lanes does not mean you need to wire all four, and in some cases less than four will be wired. If data-lanes does not describe that, then all existing users of the binding are abusing it: $ grep data_lanes drivers/media/i2c -r drivers/media/i2c/s5k5baf.c:state->nlanes = ep.bus.mipi_csi2.num_data_lanes; drivers/media/i2c/s5c73m3/s5c73m3-core.c: if (ep.bus.mipi_csi2.num_data_lanes != S5C73M3_MIPI_DATA_LANES) drivers/media/i2c/tc358743.c: endpoint->bus.mipi_csi2.num_data_lanes == 0 || drivers/media/i2c/smiapp/smiapp-core.c: hwcfg->lanes = bus_cfg->bus.mipi_csi2.num_data_lanes; So all those drivers are using it for the _number_ of CSI2 lanes, and are not touching the mapping in any way (not even checking that it is an identity mapping.) You could specify any mapping to these drivers, as long as num_data_lanes came out right. And... there's no point having a property in a binding if no one is using it... and even more silly not to have a property that everyone needs... -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 00/24] i.MX Media Driver
I seem to be getting some sort of memory corruption with this driver. I've had two instances now of uninitialised spinlocks in imx_media_dma_buf_get_active() which show that the spinlock being taken in this function is all-zeros. That very quickly leads to an oops, where I've seen buf->ring is NULL in imx_media_dma_buf_set_active(). Not quite sure what's going on, but the trigger (at least for me) is to change my gstreamer pipeline from: DISPLAY=:0 gst-launch-1.0 -v v4l2src device=/dev/video3 ! bayer2rgbneon ! xvimagesink to DISPLAY=:0 gst-launch-1.0 -v v4l2src device=/dev/video3 ! queue ! bayer2rgbneon ! xvimagesink and it seems to take as little as two or three attempts to provoke the kernel to totally die. I've just tried a third time. I can run the first gstreamer command five times. The I ran the second command and immediately got this: INFO: trying to register non-static key. the code is fine but needs lockdep annotation. turning off the locking correctness validator. CPU: 0 PID: 1008 Comm: Xorg Tainted: G C 4.10.0-rc6+ #2103 Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree) Backtrace: [] (dump_backtrace) from [] (show_stack+0x18/0x1c) r6:600f0193 r5: r4: r3: [] (show_stack) from [] (dump_stack+0xa4/0xdc) [] (dump_stack) from [] (register_lock_class+0x1d4/0x554) r6:c1400408 r5: r4: r3:ee47a4c0 [] (register_lock_class) from [] (__lock_acquire+0x80/0x17b0) r10:d995f760 r9:c0a70384 r8: r7:c0a38680 r6: r5:ee47a4c0 r4:c1400408 [] (__lock_acquire) from [] (lock_acquire+0xd8/0x250) r10: r9:c0a70384 r8: r7: r6:d995f760 r5:600f0193 r4: [] (lock_acquire) from [] (_raw_spin_lock_irqsave+0x4c/0x60) r10:ed501e64 r9:c09e04ec r8: r7:0139 r6:bf0d7a8c r5:600f0193 r4:d995f750 [] (_raw_spin_lock_irqsave) from [] (imx_media_dma_buf_get_active+0x1c/0x94 [imx_media_common]) r6:e98b2c10 r5:d995f750 r4:d995f600 [] (imx_media_dma_buf_get_active [imx_media_common]) from [] (imx_smfc_eof_interrupt+0x60/0x124 [imx_smfc]) r5:ee935dc4 r4:ee935c10 [] (imx_smfc_eof_interrupt [imx_smfc]) from [] (__handle_irq_event_percpu+0xa4/0x428) r6:e98b2c10 r5:e98b2c00 r4:ebfb6d40 r3:bf12c458 [] (__handle_irq_event_percpu) from [] (handle_irq_event_percpu+0x24/0x60) r10:ed501fb0 r9:f4001100 r8:0009 r7: r6:e98b2c10 r5:e98b2c00 r4:e98b2c00 [] (handle_irq_event_percpu) from [] (handle_irq_event+0x40/0x64) r5:e98b2c60 r4:e98b2c00 [] (handle_irq_event) from [] (handle_level_irq+0xb0/0x138) r6:e98b2c10 r5:e98b2c60 r4:e98b2c00 r3:c09d0418 [] (handle_level_irq) from [] (generic_handle_irq+0x20/0x30) r6:ee4a3010 r5:ed501f08 r4: r3:c00a30c4 [] (generic_handle_irq) from [] (ipu_irq_handle+0xa8/0xd8) [] (ipu_irq_handle) from [] (ipu_irq_handler+0x5c/0xb4) r8:ef008400 r7:0026 r6:ee4a3010 r5:c09e756c r4:ef1efc10 [] (ipu_irq_handler) from [] (generic_handle_irq+0x20/0x30) r6: r5: r4:c09d52d0 [] (generic_handle_irq) from [] (__handle_domain_irq+0x5c/0xb8) [] (__handle_domain_irq) from [] (gic_handle_irq+0x4c/0x9c) r8:c0a38a78 r7:03eb r6:c09e0af0 r5:f400010c r4:f4000100 r3:ed501fb0 [] (gic_handle_irq) from [] (__irq_usr+0x58/0x80) Exception stack(0xed501fb0 to 0xed501ff8) 1fa0: b698b4e0 0042c000 b698c708 1fc0: 0010 81231b10 81231b18 80e89670 b698b4e0 8114957c 7f79b000 81149438 1fe0: 7f79b248 bee08b98 7f708609 b6904220 600f0030 r10:7f79b000 r9:8114957c r8:10c5387d r7:10c5387d r6: r5:600f0030 r4:b6904220 r3:ee47a4c0 [ cut here ] WARNING: CPU: 0 PID: 1008 at /home/rmk/git/linux-rmk/drivers/staging/media/imx/imx-smfc.c:159 imx_smfc_eof_interrupt+0x118/0x124 [imx_smfc] Modules linked in: imx_csi(C) rfcomm bnep bluetooth nfsd imx_camif(C) imx_ic(C) imx_smfc(C) caam_jr snd_soc_imx_sgtl5000 uvcvideo snd_soc_fsl_asoc_card snd_soc_imx_spdif imx_media(C) imx_mipi_csi2(C) imx_media_common(C) snd_soc_imx_audmux imx219 snd_soc_sgtl5000 caam video_multiplexer imx_sdma imx2_wdt rc_cec snd_soc_fsl_ssi coda v4l2_mem2mem videobuf2_v4l2 videobuf2_dma_contig videobuf2_core snd_soc_fsl_spdif imx_pcm_dma videobuf2_vmalloc dw_hdmi_ahb_audio dw_hdmi_cec videobuf2_memops imx_thermal etnaviv fuse rc_pinnacle_pctv_hd CPU: 0 PID: 1008 Comm: Xorg Tainted: G C 4.10.0-rc6+ #2103 Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree) Backtrace: [] (dump_backtrace) from [] (show_stack+0x18/0x1c) r6:600f0193 r5: r4: r3: [] (show_stack) from [] (dump_stack+0xa4/0xdc) [] (dump_stack) from [] (__warn+0xdc/0x108) r6:bf12d004 r5: r4: r3:ee47a4c0 [] (__warn) from [] (warn_slowpath_null+0x28/0x30) r10:ed501e64 r8: r7:0139 r6:e98b2c10 r5:ee935dc4 r4:ee935c10 [] (warn_slowpath_null) from [] (imx_smfc_eof_interrupt+0x118/0x124 [imx_smfc]) [] (imx_smfc_eof_interrupt [imx_smfc]) from [] (__handle_irq_event_percpu+0xa4/0x428)
Re: [PATCH v3 18/24] media: imx: Add SMFC subdev driver
Hi Steve, On Fri, Jan 06, 2017 at 06:11:36PM -0800, Steve Longerbeam wrote: > +/* > + * Min/Max supported width and heights. > + * > + * We allow planar output from the SMFC, so we have to align > + * output width by 16 pixels to meet IDMAC alignment requirements, > + * which also means input width must have the same alignment. > + */ > +#define MIN_W 176 > +#define MIN_H 144 > +#define MAX_W 8192 > +#define MAX_H 4096 > +#define W_ALIGN4 /* multiple of 16 pixels */ Does this only apply to planar formats? I notice Philipp's driver allows 8 pixel alignment. If it's only for planar formats, it ought to determine the alignment based on the requested format rather than hard-coding it to the maximum alignment of all the supported formats. Thanks. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 20/24] media: imx: Add Camera Interface subdev driver
On Tue, Jan 31, 2017 at 10:21:26AM -0800, Steve Longerbeam wrote: > On 01/31/2017 05:42 AM, Russell King - ARM Linux wrote: > >On Fri, Jan 20, 2017 at 03:38:28PM +0100, Hans Verkuil wrote: > >>Should be set to something like 'platform:imx-media-camif'. v4l2-compliance > >>should complain about this. > >... and more. > > Right, in version 3 that you are working with, no v4l2-compliance fixes were > in yet. A lot of the compliance errors are fixed, please look in latest > branch > imx-media-staging-md-wip at g...@github.com:slongerbeam/mediatree.git. Sorry, I'm not prepared to pull random trees from github as there's no easy way to see what's in the branch. I've always disliked github because its web interface makes it soo difficult to navigate around git trees hosted there. You can see a commit, you can see a diff of the commit. You can get a list of branches. But there seems to be no way to get a list of commits similar to "git log" or even a one-line summary of each commit on a branch. If there is, it's completely non-obvious (which I think is much of the problem with github, it's web interface is horrendous.) Or you can clone/pull the tree without knowing what you're fetching (eg, what the tree is based upon.) Or you can waste time clicking repeatedly on the "parent" commit link on each patch working your way back through the history... Well, it looks like it's bsaed on 4.10-rc1 with who-knows-what work from the linux-media tree (I didn't try and go back any further.) As I don't want to take a whole pile of other changes into my tree, I'm certainly not going to pull from your github tree. Sorry. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 00/24] i.MX Media Driver
On Thu, Feb 02, 2017 at 05:22:46PM +, Russell King - ARM Linux wrote: > I thought, maybe, it's the IPU overwriting past the end of the buffer, > but I've added checks and that doesn't seem to have fired. I also > wondered if it was some kind of use-after-free of the ring, so I made > imx_media_free_dma_buf_ring() memset the ring to 0x5a5a5a5a before > kfree()ing it... doesn't look like it's that either. I'm going to > continue poking to see if I can figure out what's going on. I take that back... here's a use-after-free of that buffer, on the very first run: Alignment trap: not handling instruction e1921f9f at [] Unhandled fault: alignment exception (0x001) at 0x5a5a5b5e pgd = c0004000 [5a5a5b5e] *pgd= Internal error: : 1 [#1] SMP ARM Modules linked in: imx_csi(C) rfcomm bnep bluetooth nfsd imx_camif(C) imx_ic(C) imx_smfc(C) caam_jr snd_soc_imx_spdif snd_soc_imx_sgtl5000 snd_soc_fsl_asoc_card imx_media(C) uvcvideo imx_mipi_csi2(C) imx_media_common(C) imx219 snd_soc_sgtl5000 snd_soc_imx_audmux caam video_multiplexer imx_sdma imx2_wdt coda v4l2_mem2mem videobuf2_v4l2 videobuf2_dma_contig videobuf2_core rc_cec snd_soc_fsl_ssi snd_soc_fsl_spdif videobuf2_vmalloc videobuf2_memops imx_pcm_dma imx_thermal dw_hdmi_ahb_audio dw_hdmi_cec etnaviv fuse rc_pinnacle_pctv_hd CPU: 0 PID: 99 Comm: kworker/0:3 Tainted: G C 4.10.0-rc6+ #2103 Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree) Workqueue: lru-add-drain wq_barrier_func task: ee4e24c0 task.stack: ee6da000 PC is at __lock_acquire+0xd4/0x17b0 LR is at lock_acquire+0xd8/0x250 pc : []lr : []psr: 20070193 sp : ee6dbb60 ip : 0001 fp : ee6dbbe4 r10: e9efad60 r9 : c0a70384 r8 : r7 : c0a38680 r6 : r5 : ee4e24c0 r4 : c1400408 r3 : r2 : 5a5a5b5e r1 : r0 : 5a5a5a5a Flags: nzCv IRQs off FIQs on Mode SVC_32 ISA ARM Segment none Control: 10c5387d Table: 3d7ec04a DAC: 0051 Process kworker/0:3 (pid: 99, stack limit = 0xee6da210) Stack: (0xee6dbb60 to 0xee6dc000) bb60: c0a38680 0002 c0b9d8c4 ee4e29a8 ee6dbc04 ee6dbb80 c0089708 c0088d44 bb80: ee6dbb9c 050f c00867fc c0086728 ee6dbbf4 ee6dbba0 87eba239 c035aa2f bba0: 0001 ee4e29a8 c00c4f84 0001 0026 0560e36b bbc0: 60070193 e9efad60 c0a70384 ee6dbc3c ee6dbbe8 bbe0: c008b108 c0089400 0001 0080 bf0d2a8c bc00: c008b108 c0089400 0001 c09e04ec e9efad50 60070193 bf0d2a8c bc20: 0139 c09e04ec ee6dbcec ee6dbc6c ee6dbc40 c07016f4 c008b03c bc40: 0001 bf0d2a8c ee6dbcec ee6dbc84 e9efac00 e9efad50 ee9785c4 bc60: ee6dbc84 ee6dbc70 bf0d2a8c c07016b4 ee978410 e9efb400 ee6dbca4 ee6dbc88 bc80: bf1224b8 bf0d2a7c bf122458 ee88d4c0 e9efb400 e9efb410 ee6dbce4 ee6dbca8 bca0: c009f5dc bf122464 0001 c09e04ec e9efb400 c009f9f8 e9efb400 bcc0: e9efb400 e9efb410 0009 f4001100 ee6dbe38 ee6dbd04 ee6dbce8 bce0: c009f984 c009f544 c0701d10 e9efb400 e9efb460 ee6dbd24 ee6dbd08 bd00: c009fa00 c009f96c c09d0418 e9efb400 e9efb460 e9efb410 ee6dbd44 ee6dbd28 bd20: c00a3174 c009f9cc c00a30c4 ee6dbd90 ee4a3010 ee6dbd54 ee6dbd48 bd40: c009ecf0 c00a30d0 ee6dbd84 ee6dbd58 c0409328 c009ecdc c09d0448 0001 bd60: 0026 ef1efc10 c09e756c ee4a3010 0026 ef008400 ee6dbdcc ee6dbd88 bd80: c0409458 c040928c 0001 0001 0002 0003 000a bda0: 000b 000c 000d 000e ee6dbdcc c09d52d0 bdc0: ee6dbddc ee6dbdd0 c009ecf0 c0409408 ee6dbe04 ee6dbde0 c009ee24 c009ecdc bde0: ee6dbe38 f4000100 f400010c c09e0af0 03eb c0a38a78 ee6dbe34 ee6dbe08 be00: c00094c8 c009edd4 ee4e24c0 c0701d50 20070013 ee6dbe6c ef7be600 be20: ee6da000 c09f5dc6 ee6dbe9c ee6dbe38 c00149f0 c0009488 0001 ee4e2988 be40: 60070093 20070013 ddb9799c 20070013 ee6dbef0 ef7be600 c09e04ec be60: c09f5dc6 ee6dbe9c 0288 ee6dbe88 c008b60c c0701d50 20070013 be80: 0051 ddb9799c ddb97998 ee6dbebc ee6dbea0 c0083824 c0701d20 bea0: c004e9c4 ee6e6d80 ddb97978 ef7ba940 ee6dbecc ee6dbec0 c004e9d8 c00837e8 bec0: ee6dbf2c ee6dbed0 c0050958 c004e9d0 0001 c0050898 bee0: c0701d8c ee4e24c0 000f c0a73e7c c0bc8834 c08947f8 bf00: 0008 ee6e6d80 ee6e6d98 ee6e6d98 0008 ef7ba940 ef7ba940 c09dd900 bf20: ee6dbf44 ee6dbf30 c0050e78 c0050774 ee6e6d80 ef7ba974 ee6dbf7c ee6dbf48 bf40: c0051094 c0050e54 ee6e8ac0 ee509eb8 ee509e80 ee6e8ac0 bf60: ee509eb8 ee6e6d80 ef0c9e58 c0050e88 ee6dbfac ee6dbf80 c0057b90 c0050e94 bf80: ee6da000 ee6e8ac0 c0057a88 bfa0: ee6dbfb0 c000fdf0 c0057a94 bfc0: bfe0: 0013 3fffd861 3fffdc61 Backtrace: [] (__lock_acquire) from [] (lock_acquir
Re: [PATCH v3 20/24] media: imx: Add Camera Interface subdev driver
On Tue, Jan 31, 2017 at 09:55:29PM +, Ian Arkver wrote: > On 31/01/17 20:33, Russell King - ARM Linux wrote: > >On Tue, Jan 31, 2017 at 10:21:26AM -0800, Steve Longerbeam wrote: > >>On 01/31/2017 05:42 AM, Russell King - ARM Linux wrote: > >>>On Fri, Jan 20, 2017 at 03:38:28PM +0100, Hans Verkuil wrote: > >>>>Should be set to something like 'platform:imx-media-camif'. > >>>>v4l2-compliance > >>>>should complain about this. > >>>... and more. > >> > >>Right, in version 3 that you are working with, no v4l2-compliance fixes were > >>in yet. A lot of the compliance errors are fixed, please look in latest > >>branch > >>imx-media-staging-md-wip at g...@github.com:slongerbeam/mediatree.git. > > > >Sorry, I'm not prepared to pull random trees from github as there's > >no easy way to see what's in the branch. > > > >I've always disliked github because its web interface makes it soo > >difficult to navigate around git trees hosted there. You can see > >a commit, you can see a diff of the commit. You can get a list of > >branches. But there seems to be no way to get a list of commits > >similar to "git log" or even a one-line summary of each commit on > >a branch. If there is, it's completely non-obvious (which I think is > >much of the problem with github, it's web interface is horrendous.) > > > >Or you can clone/pull the tree without knowing what you're fetching > >(eg, what the tree is based upon.) > > > >Or you can waste time clicking repeatedly on the "parent" commit link > >on each patch working your way back through the history... > > > >Well, it looks like it's bsaed on 4.10-rc1 with who-knows-what work > >from the linux-media tree (I didn't try and go back any further.) > >As I don't want to take a whole pile of other changes into my tree, > >I'm certainly not going to pull from your github tree. Sorry. > > > > https://github.com/slongerbeam/mediatree/compare/master...imx-media-staging-md-wip > > It's under the "Compare" button from the main view. It would be nice though > if the first commit's parent was some clearly tagged start point. I don't want master though, I want v4.10-rc1, and if I ask for that it tells me it knows nothing about v4.10-rc1, despite the fact that's a tag in the mainline kernel repository which was merged into the linux-media tree that this tree is based upon. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 20/24] media: imx: Add Camera Interface subdev driver
On Tue, Jan 31, 2017 at 02:36:53PM -0800, Steve Longerbeam wrote: > On 01/31/2017 02:04 PM, Russell King - ARM Linux wrote: > >I don't want master though, I want v4.10-rc1, and if I ask for that > >it tells me it knows nothing about v4.10-rc1, despite the fact that's > >a tag in the mainline kernel repository which was merged into the > >linux-media tree that this tree is based upon. > > Hi Russell, yes g...@github.com:slongerbeam/mediatree.git is a fork > of the linux-media tree, and the imx-media-staging-md-wip branch > is up-to-date with master, currently at 4.10-rc1. "up to date" is different from "contains other stuff other than is in 4.10-rc1". What I see in your tree is that your code is based off a merge commit between something called "patchwork" (which I assume is a branch in the media tree containing stuff commited from patch work) and v4.10-rc1. Now, you don't get a commit when merging unless there's changes that aren't in the commit you're merging - if "patchwork" was up to date with v4.10-rc1, then git would have done a "fast forward" to v4.10-rc1. Therefore, while it may be "up to date" with v4.10-rc1 in so far that it's had v4.10-rc1 merged into it, that's not what I've been saying. There are other changes below that merge commit which aren't in v4.10-rc1. It's those other changes that I'm talking about, and it's those other changes I do not want without knowing what they are. It may be that those other changes have since been merged into v4.10-rc6 - but github's web interface can't show me that. In fact, github's web interface is pretty damned useless as far as this stuff goes. So, what I'll get if I clone or pull your imx-media-staging-md-wip branch is, yes, a copy of all your changes, but _also_ all the changes that are in the media tree that _aren't_ in mainline at the point that v4.10-rc1 was merged. > You don't need to use the web interface, just git clone the repo. You're assuming I want to work off the top of your commits. I don't. I've got other dependencies. Then there's yet another problem - lets say that I get a copy of your patches that haven't been on the mailing list, and I then want to make a comment about it. I can't reply to a patch that hasn't been on the mailing list. So, the long established mechanism by which the Linux community does patch review breaks down. So no, sorry, I'm not fetching your tree, and I will persist with your v3 patch set for the time being. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 00/24] i.MX Media Driver
> The central issue seems to be that I think media pad links / media bus > formats should describe physical links, such as parallel or serial > buses, and the formats of pixels flowing through them, whereas Steve > would like to extend them to describe software transports and in-memory > formats. This probably isn't the right place to attach this comment in this thread, but... the issue of media bus formats matching physical buses is an argument that I think is already lost. For example, take the 10-bit bayer formats: #define MEDIA_BUS_FMT_SBGGR10_1X10 0x3007 #define MEDIA_BUS_FMT_SGBRG10_1X10 0x300e #define MEDIA_BUS_FMT_SGRBG10_1X10 0x300a #define MEDIA_BUS_FMT_SRGGB10_1X10 0x300f These are commonly used on CSI serial buses (see the smiapp driver for example). From the description at the top of the file, it says the 1X10 means that one pixel is transferred as one 10-bit sample. However, the format on wire is somewhat different - four pixels are transmitted over five bytes: P0 P1 P2 P3 P0 P1 P2 P3 8-bit 8-bit 8-bit 8-bit 2-bit 2-bit 2-bit 2-bit This gives two problems: 1) it doesn't fit in any sensible kind of "one pixel transferred as N M-bit samples" description because the pixel/sample values (depending how you look at them) are broken up. 2) changing this will probably be a user visible change, as things like smiapp are already in use. So, I think what we actually have is the media bus formats describing the _logical_ bus format. Yes, one pixel is transferred as one 10-bit sample in this case. To help illustrate my point, consider the difference between MEDIA_BUS_FMT_RGB565_1X16 and MEDIA_BUS_FMT_RGB565_2X8_BE or MEDIA_BUS_FMT_RGB565_2X8_LE. RGB565_1X16 means 1 pixel over an effective 16-bit wide bus (if it's not 16-bit, then it has to be broken up into separate "samples".) RGB565_2X8 means 1 pixel as two 8-bit samples. So, the 10-bit bayer is 1 pixel as 1.25 bytes. Or is it, over a serial bus. Using the RGB565 case, 10-bit bayer over a 4 lane CSI bus becomes interesting: first byte 2nd 3rd lane 1 P0 9:2 S0 P7 9:2 lane 2 P1 9:2 P4 9:2 S1 lane 3 P2 9:2 P5 9:2 P8 9:2 lane 4 P3 9:2 P6 9:2 P9 9:2 S0 = P0/P1/P2/P3 least significant two bits S1 = P4/P5/P6/P7 least significant two bits or 2 lane CSI: first byte 2nd 3rd 4th 5th lane 1 P0 9:2 P2 S0 P5 P7 lane 2 P1 9:2 P3 P4 P6 S1 or 1 lane CSI: lane 1 P0 P1 P2 P3 S0 P4 P5 P6 P7 S1 P8 P9 ... etc. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 16/24] media: Add i.MX media core driver
On Mon, Jan 23, 2017 at 12:13:26PM +0100, Philipp Zabel wrote: > Hi Steve, > > On Sun, 2017-01-22 at 18:31 -0800, Steve Longerbeam wrote: > > Second, ignoring the above locking issue for a moment, > > v4l2_pipeline_pm_use() > > will call s_power on the sensor _first_, then the mipi csi-2 s_power, > > when executing > > media-ctl -l '"ov5640 1-003c":0 -> "imx6-mipi-csi2":0[1]'. Which is the > > wrong order. > > In my version which enforces the correct power on order, the mipi csi-2 > > s_power > > is called first in that link setup, followed by the sensor. > > I don't understand why you want to power up subdevs as soon as the links > are established. Shouldn't that rather be done for all subdevices in the > pipeline when the corresponding capture device is opened? > It seems to me that powering up the pipeline should be the last step > before userspace actually starts the capture. I agree with Philipp here - configuration of the software pipeline shouldn't result in hardware being forced to be powered up. That's more of a decision for the individual sub-driver than for core. Executing media-ctl to enable a link between two sub-device endpoints should really be a matter of setting the software state, and when the video device is opened for streaming, surely that's when the hardware in the chain between the source and the capture device should be powered up and programmed. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 19/24] media: imx: Add IC subdev drivers
On Fri, Jan 06, 2017 at 06:11:37PM -0800, Steve Longerbeam wrote: > This is a set of three media entity subdevice drivers for the i.MX > Image Converter. The i.MX IC module contains three independent > "tasks": > > - Pre-processing Encode task: video frames are routed directly from > the CSI and can be scaled, color-space converted, and rotated. > Scaled output is limited to 1024x1024 resolution. Output frames > are routed to the camera interface entities (camif). > > - Pre-processing Viewfinder task: this task can perform the same > conversions as the pre-process encode task, but in addition can > be used for hardware motion compensated deinterlacing. Frames can > come either directly from the CSI or from the SMFC entities (memory > buffers via IDMAC channels). Scaled output is limited to 1024x1024 > resolution. Output frames can be routed to various sinks including > the post-processing task entities. > > - Post-processing task: same conversions as pre-process encode. However > this entity sends frames to the i.MX IPU image converter which supports > image tiling, which allows scaled output up to 4096x4096 resolution. > Output frames can be routed to the camera interfaces. > > Signed-off-by: Steve LongerbeamApplying: media: imx: Add IC subdev drivers .git/rebase-apply/patch:3054: new blank line at EOF. + warning: 1 line adds whitespace errors. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 17/24] media: imx: Add CSI subdev driver
On Fri, Jan 06, 2017 at 06:11:35PM -0800, Steve Longerbeam wrote: > This is a media entity subdevice for the i.MX Camera > Serial Interface module. > > Signed-off-by: Steve Longerbeamwarning: 3 lines add whitespace errors. Applying: media: imx: Add CSI subdev driver .git/rebase-apply/patch:38: new blank line at EOF. + warning: 1 line adds whitespace errors. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. ___ 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
On Fri, Jan 06, 2017 at 06:11:39PM -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 LongerbeamApplying: media: imx: Add MIPI CSI-2 Receiver subdev driver .git/rebase-apply/patch:522: new blank line at EOF. + warning: 1 line adds whitespace errors. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 06/24] ARM: dts: imx6-sabrelite: add OV5642 and OV5640 camera sensors
On Fri, Jan 06, 2017 at 06:11:24PM -0800, Steve Longerbeam wrote: > + ov5640: camera@40 { > + compatible = "ovti,ov5640"; > + pinctrl-names = "default"; > + pinctrl-0 = <_ov5640>; > + clocks = <_xclk>; > + clock-names = "xclk"; > + reg = <0x40>; > + xclk = <2200>; > + reset-gpios = < 5 GPIO_ACTIVE_LOW>; /* NANDF_D5 */ > + pwdn-gpios = < 9 GPIO_ACTIVE_HIGH>; /* NANDF_WP_B */ > + > + port { > + #address-cells = <1>; > + #size-cells = <0>; > + > + ov5640_to_mipi_csi: endpoint@1 { > + reg = <1>; > + remote-endpoint = <_csi_from_mipi_sensor>; > + data-lanes = <0 1>; > + clock-lanes = <2>; How do you envision a four-lane sensor being described? data-lanes = <0 1 3 4>; clock-lanes = <2>; ? The binding document for video-interfaces.txt says: - clock-lanes: an array of physical clock lane indexes. Position of an entry determines the logical lane number, while the value of an entry indicates physical lane, e.g. for a MIPI CSI-2 bus we could have "clock-lanes = <0>;", which places the clock lane on hardware lane 0. This property is valid for serial busses only (e.g. MIPI CSI-2). Note that for the MIPI CSI-2 bus this array contains only one entry. So I think you need to have a good reason to make the clock lane non-zero. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 06/24] ARM: dts: imx6-sabrelite: add OV5642 and OV5640 camera sensors
On Fri, Jan 06, 2017 at 06:11:24PM -0800, Steve Longerbeam wrote: > diff --git a/arch/arm/boot/dts/imx6q-sabrelite.dts > b/arch/arm/boot/dts/imx6q-sabrelite.dts > index 66d10d8..9e2d26d 100644 > --- a/arch/arm/boot/dts/imx6q-sabrelite.dts > +++ b/arch/arm/boot/dts/imx6q-sabrelite.dts > @@ -52,3 +52,9 @@ > { > status = "okay"; > }; > + > +_csi1_from_mipi_vc1 { > + data-lanes = <0 1>; > + clock-lanes = <2>; > +}; > + > diff --git a/arch/arm/boot/dts/imx6qdl-sabrelite.dtsi > b/arch/arm/boot/dts/imx6qdl-sabrelite.dtsi > index 795b5a5..bca9fed 100644 > --- a/arch/arm/boot/dts/imx6qdl-sabrelite.dtsi > +++ b/arch/arm/boot/dts/imx6qdl-sabrelite.dtsi ... > +/* Incoming port from sensor */ > +_csi_from_mipi_sensor { > +remote-endpoint = <_to_mipi_csi>; > +data-lanes = <0 1>; > +clock-lanes = <2>; > +}; > + Applying: ARM: dts: imx6-sabrelite: add OV5642 and OV5640 camera sensors .git/rebase-apply/patch:33: new blank line at EOF. + .git/rebase-apply/patch:201: new blank line at EOF. + warning: 2 lines add whitespace errors. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 16/24] media: Add i.MX media core driver
On Fri, Jan 06, 2017 at 06:11:34PM -0800, Steve Longerbeam wrote: > Add the core media driver for i.MX SOC. > > Signed-off-by: Steve LongerbeamApplying: media: Add i.MX media core driver .git/rebase-apply/patch:516: new blank line at EOF. + .git/rebase-apply/patch:528: new blank line at EOF. + .git/rebase-apply/patch:556: new blank line at EOF. + warning: 3 lines add whitespace errors. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. ___ 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
On Fri, Jan 06, 2017 at 06:11:39PM -0800, Steve Longerbeam wrote: > +++ b/drivers/staging/media/imx/imx-mipi-csi2.c ... > +#define DEVICE_NAME "imx6-mipi-csi2" Why is the device/driver named imx6-mipi-csi2, but the module named imx-mipi-csi2 - could there be some consistency here please? Thanks. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. ___ 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
On Fri, Jan 06, 2017 at 06:11:39PM -0800, Steve Longerbeam wrote: > +static void imxcsi2_enable(struct imxcsi2_dev *csi2, bool enable) > +{ > + if (enable) { > + imxcsi2_write(csi2, 0x, CSI2_PHY_SHUTDOWNZ); > + imxcsi2_write(csi2, 0x, CSI2_DPHY_RSTZ); > + imxcsi2_write(csi2, 0x, CSI2_RESETN); > + } else { > + imxcsi2_write(csi2, 0x0, CSI2_PHY_SHUTDOWNZ); > + imxcsi2_write(csi2, 0x0, CSI2_DPHY_RSTZ); > + imxcsi2_write(csi2, 0x0, CSI2_RESETN); > + } > +} > + > +static void imxcsi2_reset(struct imxcsi2_dev *csi2) > +{ > + imxcsi2_enable(csi2, false); > + > + imxcsi2_write(csi2, 0x0001, CSI2_PHY_TST_CTRL0); > + imxcsi2_write(csi2, 0x, CSI2_PHY_TST_CTRL1); > + imxcsi2_write(csi2, 0x, CSI2_PHY_TST_CTRL0); > + imxcsi2_write(csi2, 0x0002, CSI2_PHY_TST_CTRL0); > + imxcsi2_write(csi2, 0x00010044, CSI2_PHY_TST_CTRL1); > + imxcsi2_write(csi2, 0x, CSI2_PHY_TST_CTRL0); > + imxcsi2_write(csi2, 0x0014, CSI2_PHY_TST_CTRL1); > + imxcsi2_write(csi2, 0x0002, CSI2_PHY_TST_CTRL0); > + imxcsi2_write(csi2, 0x, CSI2_PHY_TST_CTRL0); > + > + imxcsi2_enable(csi2, true); > +} > + > +static int imxcsi2_dphy_wait(struct imxcsi2_dev *csi2) > +{ > + u32 reg; > + int i; > + > + /* wait for mipi sensor ready */ > + for (i = 0; i < 50; i++) { > + reg = imxcsi2_read(csi2, CSI2_PHY_STATE); > + if (reg != 0x200) > + break; > + usleep_range(1, 2); > + } > + > + if (i >= 50) { > + v4l2_err(>sd, > + "wait for clock lane timeout, phy_state = 0x%08x\n", > + reg); > + return -ETIME; > + } > + > + /* wait for mipi stable */ > + for (i = 0; i < 50; i++) { > + reg = imxcsi2_read(csi2, CSI2_ERR1); > + if (reg == 0x0) > + break; > + usleep_range(1, 2); > + } > + > + if (i >= 50) { > + v4l2_err(>sd, > + "wait for controller timeout, err1 = 0x%08x\n", > + reg); > + return -ETIME; > + } > + > + /* finally let's wait for active clock on the clock lane */ > + for (i = 0; i < 50; i++) { > + reg = imxcsi2_read(csi2, CSI2_PHY_STATE); > + if (reg & (1 << 8)) > + break; > + usleep_range(1, 2); > + } > + > + if (i >= 50) { > + v4l2_err(>sd, > + "wait for active clock timeout, phy_state = 0x%08x\n", > + reg); > + return -ETIME; > + } > + > + v4l2_info(>sd, "ready, dphy version 0x%x\n", > + imxcsi2_read(csi2, CSI2_VERSION)); > + > + return 0; > +} ... > +static int imxcsi2_s_power(struct v4l2_subdev *sd, int on) > +{ > + struct imxcsi2_dev *csi2 = sd_to_dev(sd); > + > + if (on && !csi2->on) { > + v4l2_info(>sd, "power ON\n"); > + clk_prepare_enable(csi2->cfg_clk); > + clk_prepare_enable(csi2->dphy_clk); > + imxcsi2_set_lanes(csi2); > + imxcsi2_reset(csi2); The iMX6 manuals call for a very specific seven sequence of initialisation for CSI2, which begins with: 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 Since you reset the CSI2 at power up and then release it, how do you guarantee that the published sequence is followed? With Philipp's driver, this is easy, because there is a prepare_stream callback which gives the sensor an opportunity to get everything correctly configured according to the negotiated parameters, and place the sensor in LP-11 state. Some sensors do not power up in LP-11 state, but need to be programmed fully before being asked to momentarily stream. Only at that point is the sensor guaranteed to be in the required LP-11 state. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 00/24] i.MX Media Driver
On Tue, Jan 31, 2017 at 12:45:11AM +, Russell King - ARM Linux wrote: > Trying this driver with an imx219 camera (which works with Philipp's > driver) results in not much happening... no /dev/media* node for it, > no subdevs, no nothing. No clues as to what's missing either. Only > messages from imx-media are from registering the various subdevs. Another issue: imx_csi 5491 4 imx_camif 11654 4 imx_ic 23961 8 imx_smfc6639 4 imx_media 23308 1 imx_csi imx_mipi_csi2 5544 1 imx_media_common 12701 6 imx_csi,imx_smfc,imx_media,imx_mipi_csi2,imx_camif,imx_ic imx219 21205 2 So how does one remove any of these modules, say, while developing a camera driver? Having to reboot to test an update makes it painfully slow for testing. Philipp's driver can do this (once the unload bugs are fixed, which I have patches for). -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 22/24] media: imx: Add MIPI CSI-2 OV5640 sensor subdev driver
On Fri, Jan 06, 2017 at 06:11:40PM -0800, Steve Longerbeam wrote: > +config IMX_OV5640_MIPI > + tristate "OmniVision OV5640 MIPI CSI-2 camera support" > + depends on GPIOLIB && VIDEO_IMX_CAMERA > + select IMX_MIPI_CSI2 > + default y Why is this defaulting to y? New drivers should not default to enabled unless they are replacing some already pre-existing functionality. Ditto for the other camera driver. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 00/24] i.MX Media Driver
On Fri, Jan 06, 2017 at 06:11:18PM -0800, Steve Longerbeam wrote: > Philipp Zabel (3): > ARM: dts: imx6qdl: Add mipi_ipu1/2 multiplexers, mipi_csi, and their > connections > add mux and video interface bridge entity functions > platform: add video-multiplexer subdevice driver > > Steve Longerbeam (21): > [media] dt-bindings: Add bindings for i.MX media driver > ARM: dts: imx6qdl: Add compatible, clocks, irqs to MIPI CSI-2 node > ARM: dts: imx6qdl: add media device > ARM: dts: imx6qdl-sabrelite: remove erratum ERR006687 workaround > ARM: dts: imx6-sabrelite: add OV5642 and OV5640 camera sensors > ARM: dts: imx6-sabresd: add OV5642 and OV5640 camera sensors > ARM: dts: imx6-sabreauto: create i2cmux for i2c3 > ARM: dts: imx6-sabreauto: add reset-gpios property for max7310_b > ARM: dts: imx6-sabreauto: add pinctrl for gpt input capture > ARM: dts: imx6-sabreauto: add the ADV7180 video decoder > UAPI: Add media UAPI Kbuild file > media: Add userspace header file for i.MX > media: Add i.MX media core driver > media: imx: Add CSI subdev driver > media: imx: Add SMFC subdev driver > media: imx: Add IC subdev drivers > media: imx: Add Camera Interface subdev driver > media: imx: Add MIPI CSI-2 Receiver subdev driver > media: imx: Add MIPI CSI-2 OV5640 sensor subdev driver > media: imx: Add Parallel OV5642 sensor subdev driver > ARM: imx_v6_v7_defconfig: Enable staging video4linux drivers Hi, Trying this driver with an imx219 camera (which works with Philipp's driver) results in not much happening... no /dev/media* node for it, no subdevs, no nothing. No clues as to what's missing either. Only messages from imx-media are from registering the various subdevs. [ 37.444877] imx-media: Registered subdev imx6-mipi-csi2 [ 37.444973] imx-media: Registered subdev imx219 0-0010 [ 38.868740] imx-media: Registered subdev ipu1_ic_prpenc [ 38.869265] imx-media: Registered subdev ipu1_ic_prpvf [ 38.869425] imx-media: Registered subdev ipu1_ic_pp0 [ 38.870086] imx-media: Registered subdev ipu1_ic_pp1 [ 38.871510] imx-media: Registered subdev ipu2_ic_prpenc [ 38.871743] imx-media: Registered subdev ipu1_smfc0 [ 38.873043] imx-media: Registered subdev ipu1_smfc1 [ 38.873225] imx-media: Registered subdev ipu2_ic_prpvf [ 38.875027] imx-media: Registered subdev ipu2_smfc0 [ 38.875320] imx-media: Registered subdev ipu2_ic_pp0 [ 38.877148] imx-media: Registered subdev ipu2_smfc1 [ 38.877436] imx-media: Registered subdev ipu2_ic_pp1 [ 38.932089] imx-media: Registered subdev camif0 [ 38.956538] imx-media: Registered subdev camif1 [ 38.959148] imx-media: Registered subdev camif2 [ 38.964353] imx-media: Registered subdev camif3 [ 206.502077] imx-media: Registered subdev ipu1_csi0 [ 206.503304] imx-media: Registered subdev ipu1_csi1 [ 206.503814] imx-media: Registered subdev ipu2_csi0 [ 206.504281] imx-media: Registered subdev ipu2_csi1 I also get: [ 37.200072] imx6-mipi-csi2: data lanes: 2 [ 37.200077] imx6-mipi-csi2: flags: 0x0200 and from what I can see, all modules from drivers/staging/media/imx/ are loaded (had to load imx-csi by hand because of the brokenness in the drivers/gpu/ipu code attaching an device_node pointer after registering the platform device, which changes what userspace sees in the modalias file.) Any clues at what to look at? -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel