Re: [PATCH] imx-drm: imx-hdmi: fix hdmi hotplug detection initial state

[Russell King - ARM Linux]

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

[Russell King - ARM Linux]

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.

[Russell King - ARM Linux]

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.

[Russell King - ARM Linux]

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.

[Russell King - ARM Linux]

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

[Russell King - ARM Linux]

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

[Russell King - ARM Linux]

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.

[Russell King - ARM Linux]

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

[Russell King - ARM Linux]

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

[Russell King - ARM Linux]

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

[Russell King - ARM Linux]

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.

[Russell King - ARM Linux]

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

[Russell King - ARM Linux]

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

[Russell King - ARM Linux]

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

[Russell King - ARM Linux]

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

[Russell King - ARM Linux]

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

[Russell King - ARM Linux]

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

[Russell King - ARM Linux]

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

[Russell King - ARM Linux]

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

[Russell King - ARM Linux]

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

[Russell King - ARM Linux]

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

[Russell King - ARM Linux]

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

[Russell King - ARM Linux]

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

[Russell King - ARM Linux]

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.

[Russell King - ARM Linux]

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

[Russell King - ARM Linux]

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

[Russell King - ARM Linux]

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

[Russell King - ARM Linux]

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

[Russell King - ARM Linux]

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

[Russell King - ARM Linux]

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

[Russell King - ARM Linux]

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

[Russell King - ARM Linux]

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

[Russell King - ARM Linux]

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

[Russell King - ARM Linux]

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.

[Russell King - ARM Linux]

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

[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.

-- 
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

[Russell King - ARM Linux]

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

[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.

-- 
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

[Russell King - ARM Linux]

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

[Russell King - ARM Linux]

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

[Russell King - ARM Linux]

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

[Russell King - ARM Linux]

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

[Russell King - ARM Linux]

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

[Russell King - ARM Linux]

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

[Russell King - ARM Linux]

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

[Russell King - ARM Linux]

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

[Russell King - ARM Linux]

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

[Russell King - ARM Linux]

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.

[Russell King - ARM Linux]

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

[Russell King - ARM Linux]

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

[Russell King - ARM Linux]

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

[Russell King - ARM Linux]

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

[Russell King - ARM Linux]

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

[Russell King - ARM Linux]

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

[Russell King - ARM Linux]

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.

[Russell King - ARM Linux]

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

[Russell King - ARM Linux]

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

[Russell King - ARM Linux]

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

[Russell King - ARM Linux]

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

[Russell King - ARM Linux]

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

[Russell King - ARM Linux]

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

[Russell King - ARM Linux]

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

[Russell King - ARM Linux]

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

[Russell King - ARM Linux]

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

[Russell King - ARM Linux]

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

[Russell King - ARM Linux]

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

[Russell King - ARM Linux]

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

[Russell King - ARM Linux]

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

[Russell King - ARM Linux]

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

[Russell King - ARM Linux]

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

[Russell King - ARM Linux]

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

[Russell King - ARM Linux]

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

[Russell King - ARM Linux]

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

[Russell King - ARM Linux]

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

[Russell King - ARM Linux]

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

[Russell King - ARM Linux]

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

[Russell King - ARM Linux]

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

[Russell King - ARM Linux]

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

[Russell King - ARM Linux]

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

[Russell King - ARM Linux]

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

[Russell King - ARM Linux]

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

[Russell King - ARM Linux]

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

[Russell King - ARM Linux]

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

[Russell King - ARM Linux]

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

[Russell King - ARM Linux]

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

[Russell King - ARM Linux]

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

[Russell King - ARM Linux]

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

[Russell King - ARM Linux]

> 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

[Russell King - ARM Linux]

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

[Russell King - ARM Linux]

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 Longerbeam 

Applying: 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

[Russell King - ARM Linux]

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 

warning: 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

[Russell King - ARM Linux]

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 Longerbeam 

Applying: 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

[Russell King - ARM Linux]

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

[Russell King - ARM Linux]

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

[Russell King - ARM Linux]

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 Longerbeam 

Applying: 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

[Russell King - ARM Linux]

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

[Russell King - ARM Linux]

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

[Russell King - ARM Linux]

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

[Russell King - ARM Linux]

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

[Russell King - ARM Linux]

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