Re: [PATCH 1/3] Move FIMD register headers to include/video/

[Tomasz Figa]

Hi,

On Tuesday 31 of July 2012 at 09:47:57, Jingoo Han wrote:
 On Monday, July 30, 2012 8:16 PM, Leela Krishna Amudala wrote:
  Hello Jingoo Han,
  
  On Mon, Jul 30, 2012 at 2:23 PM, Jingoo Han jg1@samsung.com wrote:
   On Monday, July 30, 2012 5:45 PM, Leela Krishna Amudala wrote:
   Moved the contents of regs-fb-v4.h and regs-fb.h from arch side
   to include/video/samsung_fimd.h
   
   Signed-off-by: Leela Krishna Amudala l.kris...@samsung.com
   ---
   
arch/arm/plat-samsung/include/plat/regs-fb-v4.h |  159 ---
arch/arm/plat-samsung/include/plat/regs-fb.h|  403
-
include/video/samsung_fimd.h|  533
+++ 3 files changed, 533 insertions(+), 562
deletions(-)
delete mode 100644 arch/arm/plat-samsung/include/plat/regs-fb-v4.h
delete mode 100644 arch/arm/plat-samsung/include/plat/regs-fb.h
create mode 100644 include/video/samsung_fimd.h
   
   +*/
   +
   +/*FIMD V8 REG OFFSET */
   +#define FIMD_V8_VIDTCON0 (0x20010)
   +#define FIMD_V8_VIDTCON1 (0x20014)
   +#define FIMD_V8_VIDTCON2 (0x20018)
   +#define FIMD_V8_VIDTCON3 (0x2001C)
   +#define FIMD_V8_VIDCON1  (0x20004)
 
 How about using soc_is_exynos5250()?
 
 +#define VIDTCON0 (soc_is_exynos5250() ? \
 + (0x20010) : (0x10))
 
 In this case, the FIMD driver does not need to change.
 Also, one binary is available.
 

This would look good indeed, but there are some drawbacks:
- it would not scale nicely for future SoCs using the new FIMD
- it would add the overhead of checking SoC ID for every access to affected 
registers (at least 1 load, 1 AND, 1 compare, 1 move and 1 conditional OR, so 
5 instructions in total, possibly even more, as opposed to a single load from 
a variant struct).

I would stay with the way used in s3c-fb driver, using variant structs 
describing FIMD revisions.

Best regards,
Tomasz Figa

 
 Best regards,
 Jingoo Han
 
   CC'ed Marek.
   
   To Leela Krishna Amudala,
   
   Don't add these definitions for FIMD_V8_xxx registers, which arenot
   related to current regs-fb-v4.h 
  and regs-fb.h.
  
   Just move and merge regs-fb-v4.h and regs-fb.h to one header file,
   not add new definitions. If you want to add these definitions, please
   make new patch for this. 
  Will do it in the suggested way,
  
   Also, #define FIMD_V8_xxx is ugly.
   I think that there is better way.
   Please, find other way.
  
  I used FIMD_V8_xxx instead of  EXYNOS5_FIMD_*, because in future,
  there is a possibility that version 8 FIMD can be used in other
  application processors also.
  Thanks for reviewing the patch.
  
  Best Wishes,
  Leela Krishna.
  
   --
   1.7.0.4
   
   --
   To unsubscribe from this list: send the line unsubscribe linux-fbdev
   in
   the body of a message to majord...@vger.kernel.org
   More majordomo info at  http://vger.kernel.org/majordomo-info.html
 
 --
 To unsubscribe from this list: send the line unsubscribe linux-samsung-soc
 in the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH V3 1/2] drm/exynos: add platform_device_id table and driver data for exynos5 drm fimd

[Tomasz Figa]

Hi Leela,

See my comments inline.

On Thursday 16 of August 2012 12:08:08 Leela Krishna Amudala wrote:
 +enum fimd_version_type {
 + VERSION_8, /* FIMD_VERSION8 */
 +};
 +
 +struct drm_fimd_driver_data {
 + enum fimd_version_type fimd_ver;
 +};
 +
 +struct drm_fimd_driver_data exynos5_drm_fimd_driver_data = {
 + .fimd_ver = VERSION_8,
 +};

I think that the approach with timing_base, as suggested by Joonyoung Shim, 
would be much cleaner.

 +static struct platform_device_id exynos_drm_fimd_driver_ids[] = {
 + {
 + .name   = exynos4-fb,
 + }, {
 + .name   = exynos5-drm-fimd,
 + .driver_data= (unsigned long)exynos5_drm_fimd_driver_data,
 + },
 + {},
 +};
 +MODULE_DEVICE_TABLE(platform, exynos_drm_fimd_driver_ids);

If I see correctly, this will crash on a null pointer dereference on 
Exynos4 without DT, because of NULL driver_data.

P.S. I think you should CC linux-arm-kernel and linux-samsung-soc lists 
when submitting patches related to ARM and Samsung SoCs.

Best regards,
-- 
Tomasz Figa
Samsung Poland RD Center

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC v2 0/5] Common Display Framework

[Tomasz Figa]

Hi Vikas,

On Tuesday 18 of December 2012 08:31:30 Vikas Sajjan wrote:
  Hi Laurent,
 
 Thanks for the reply.
 
 On 17 December 2012 20:55, Laurent Pinchart 
 
 laurent.pinch...@ideasonboard.com wrote:
  Hi Vikas,
  
  Sorry for the late reply. I now have more time to work on CDF, so
  delays should be much shorter.
  
  On Thursday 06 December 2012 10:51:15 Vikas Sajjan wrote:
   Hi Laurent,
   
   I was thinking of porting CDF to samsung EXYNOS 5250 platform, what
   I
  
  found
  
   is that, the exynos display controller is MIPI DSI based controller.
   
   But if I look at CDF patches, it has only support for MIPI DBI based
  
  Display
  
   controller.
   
   So my question is, do we have any generic framework for MIPI DSI
   based
   display controller? basically I wanted to know, how to go about
   porting
  
  CDF
  
   for such kind of display controller.
  
  MIPI DSI support is not available yet. The only reason for that is
  that I don't have any MIPI DSI hardware to write and test the code
  with :-)
  
  The common display framework should definitely support MIPI DSI. I
  think the
  existing MIPI DBI code could be used as a base, so the implementation
  shouldn't be too high.
  
  Yeah, i was also thinking in similar lines, below is my though for
  MIPI
 
 DSI support in CDF.
 
 o   MIPI DSI support as part of CDF framework will expose
 
 §  mipi_dsi_register_device(mpi_device) (will be called mach-xxx-dt.c
 file )
 
 §  mipi_dsi_register_driver(mipi_driver, bus ops) (will be called from
 platform specific init driver call )
 
 ·bus ops will be
 
 o   read data
 
 o   write data
 
 o   write command
 
 §  MIPI DSI will be registered as bus_register()
 
 When MIPI DSI probe is called, it (e.g., Exynos or OMAP MIPI DSI) will
 initialize the MIPI DSI HW IP.
 
  This probe will also parse the DT file for MIPI DSI based panel, add
 the panel device (device_add() ) to kernel and register the display
 entity with its control and  video ops with CDF.
 
 I can give this a try.

I am currently in progress of reworking Exynos MIPI DSIM code and s6e8ax0 
LCD driver to use the v2 RFC of Common Display Framework. I have most of 
the work done, I have just to solve several remaining problems.

Best regards,
-- 
Tomasz Figa
Samsung Poland RD Center
SW Solution Development, Linux Platform

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC v2 0/5] Common Display Framework

[Tomasz Figa]

Hi Laurent,

On Monday 24 of December 2012 15:12:28 Laurent Pinchart wrote:
 Hi Tomasz,
 
 On Friday 21 December 2012 11:00:52 Tomasz Figa wrote:
  On Tuesday 18 of December 2012 08:31:30 Vikas Sajjan wrote:
   On 17 December 2012 20:55, Laurent Pinchart wrote:
Hi Vikas,

Sorry for the late reply. I now have more time to work on CDF, so
delays should be much shorter.

On Thursday 06 December 2012 10:51:15 Vikas Sajjan wrote:
 Hi Laurent,
 
 I was thinking of porting CDF to samsung EXYNOS 5250 platform,
 what
 I found is that, the exynos display controller is MIPI DSI based
 controller.
 
 But if I look at CDF patches, it has only support for MIPI DBI
 based
 Display controller.
 
 So my question is, do we have any generic framework for MIPI DSI
 based display controller? basically I wanted to know, how to go
 about
 porting CDF for such kind of display controller.

MIPI DSI support is not available yet. The only reason for that is
that I don't have any MIPI DSI hardware to write and test the code
with :-)

The common display framework should definitely support MIPI DSI. I
think the existing MIPI DBI code could be used as a base, so the
implementation shouldn't be too high.

Yeah, i was also thinking in similar lines, below is my though for
MIPI DSI support in CDF.
   
   o   MIPI DSI support as part of CDF framework will expose
   §  mipi_dsi_register_device(mpi_device) (will be called
   mach-xxx-dt.c
   file )
   §  mipi_dsi_register_driver(mipi_driver, bus ops) (will be called
   from
   platform specific init driver call )
   ·bus ops will be
   o   read data
   o   write data
   o   write command
   §  MIPI DSI will be registered as bus_register()
   
   When MIPI DSI probe is called, it (e.g., Exynos or OMAP MIPI DSI)
   will
   initialize the MIPI DSI HW IP.
   
   This probe will also parse the DT file for MIPI DSI based panel, add
   the panel device (device_add() ) to kernel and register the display
   entity with its control and  video ops with CDF.
   
   I can give this a try.
  
  I am currently in progress of reworking Exynos MIPI DSIM code and
  s6e8ax0 LCD driver to use the v2 RFC of Common Display Framework. I
  have most of the work done, I have just to solve several remaining
  problems.
 Do you already have code that you can publish ? I'm particularly
 interested (and I think Tomi Valkeinen would be as well) in looking at
 the DSI operations you expose to DSI sinks (panels, transceivers, ...).

Well, I'm afraid this might be little below your expectations, but here's 
an initial RFC of the part defining just the DSI bus. I need a bit more 
time for patches for Exynos MIPI DSI master and s6e8ax0 LCD.

The implementation is very simple and heavily based on your MIPI DBI 
support and existing Exynos MIPI DSIM framework. Provided operation set is 
based on operation set used by Exynos s6e8ax0 LCD driver. Unfortunately 
this is my only source of information about MIPI DSI.

Best regards,
-- 
Tomasz Figa
Samsung Poland RD Center
SW Solution Development, Linux Platform

From bad07d8bdce0ff76cbc81a9da597c0d01e5244f7 Mon Sep 17 00:00:00 2001
From: Tomasz Figa t.f...@samsung.com
Date: Thu, 27 Dec 2012 12:36:15 +0100
Subject: [RFC] video: display: Add generic MIPI DSI bus

Signed-off-by: Tomasz Figa t.f...@samsung.com
---
 drivers/video/display/Kconfig|   4 +
 drivers/video/display/Makefile   |   1 +
 drivers/video/display/mipi-dsi-bus.c | 214 
+++
 include/video/display.h  |   1 +
 include/video/mipi-dsi-bus.h |  98 
 5 files changed, 318 insertions(+)
 create mode 100644 drivers/video/display/mipi-dsi-bus.c
 create mode 100644 include/video/mipi-dsi-bus.h

diff --git a/drivers/video/display/Kconfig b/drivers/video/display/Kconfig
index 13b6aaf..dbaff9d 100644
--- a/drivers/video/display/Kconfig
+++ b/drivers/video/display/Kconfig
@@ -9,6 +9,10 @@ config DISPLAY_MIPI_DBI
tristate
default n
 
+config DISPLAY_MIPI_DSI
+   tristate
+   default n
+
 config DISPLAY_PANEL_DPI
tristate DPI (Parallel) Display Panels
---help---
diff --git a/drivers/video/display/Makefile 
b/drivers/video/display/Makefile
index 482bec7..429b3ac8 100644
--- a/drivers/video/display/Makefile
+++ b/drivers/video/display/Makefile
@@ -1,5 +1,6 @@
 obj-$(CONFIG_DISPLAY_CORE) += display-core.o
 obj-$(CONFIG_DISPLAY_MIPI_DBI) += mipi-dbi-bus.o
+obj-$(CONFIG_DISPLAY_MIPI_DSI) += mipi-dsi-bus.o
 obj-$(CONFIG_DISPLAY_PANEL_DPI) += panel-dpi.o
 obj-$(CONFIG_DISPLAY_PANEL_R61505) += panel-r61505.o
 obj-$(CONFIG_DISPLAY_PANEL_R61517) += panel-r61517.o
diff --git a/drivers/video/display/mipi-dsi-bus.c 
b/drivers/video/display/mipi-dsi-bus.c
new file mode 100644
index 000..2998522
--- /dev/null
+++ b/drivers/video/display/mipi-dsi-bus.c
@@ -0,0 +1,214 @@
+/*
+ * MIPI DSI Bus

Re: [RFC 0/6] Common Display Framework-T

[Tomasz Figa]

. Bus services to access
   connected devices are provided by the video source, wrapper
   functions will be used to handle serialization and locking, and
   possibly to offer higher level services (such as DCS for instance).
   
   One drawback of using the platform bus is that PM relationships
   between
   the bus master and slaves will not be taken into account during
   suspend/resume. However, a similar issue exists for DPI panels, and
   PM
   relationships at the video bus level for DBI and DSI are not handled
   by
   the DBI/DSI busses either. As we need a generic solution to handle
   those (likely through early suspend and late resume), the same
   solution can be used to handle DBI and DSI control bus PM
   relationships without requiring a Linux DBI or DSI bus.
   
   Even though I still like the idea of DBI and DSI busses, I agree
   with Tomi that they're not strictly needed and I will drop them.
   
   Entity model
   
   
   Tomi's proposal split the display entities into video sources
   (struct
   video_source) and display entities (struct display_entity). To make
   generic pipeline operations easier, we agreed to merge the video
   source
   and the display entity back. struct display_entity thus models a
   display entity that has any number of sink and/or source ports,
   modeled as struct display_entity_port instances.
  
  Looking at Tomi's patchset, he has considered panel as display
  entity
  and  MIPI DSI as video source entity. So if we are planning to merge
  it back how should we treat panel and MIPI DSI. i mean should we
  consider both panel  and MIPI DSI has 2 different display entities.
  i.e, during the probe of each of these drivers, should we register a
  display entity with CDF.
 
 Both the DSI encoder and the DSI panel would be modeled as display
 entities. The DSI encoder would have a source port that models its DSI
 video source, and the DSI panel would have a sink port.
 
   Video stream operations will be exposed by the display entity as
   function pointers and will take a port reference as argument (this
   could take the form of struct display_entity * and port index, or
   struct
   display_entity_port *). The DVI and DSI operations model proposed by
   Tomi in this patch series will be kept.
  
  so you mean you will be adding these ops as part of struct display
  entity rather than  video source ops,
 
 That's correct.
 
  static const struct dsi_video_source_ops dsi_dsi_ops = {
  
  .update = dsi_bus_update,
  .dcs_write = dsi_bus_dcs_write,
  .dcs_read = dsi_bus_dcs_read,
  .configure_pins = dsi_bus_configure_pins,
  .set_clocks = dsi_bus_set_clocks,
  .enable = dsi_bus_enable,
  .disable = dsi_bus_disable,
  .set_size = dsi_bus_set_size,
  .set_operation_mode = dsi_bus_set_operation_mode,
  .set_pixel_format = dsi_bus_set_pixel_format,
  .enable_hs = dsi_bus_enable_hs,
  
  };
  
  if you can post CDF v3 patches early, it will give us more clarity
  w.r.t to discussions you and Tomi had.
 
 I'm working on that.

May I ask you to add me on CC in v3?

This would allow me to track the development, without missing anything 
like it happened with the discussion about bus-less design.

Best regards,
-- 
Tomasz Figa
Samsung Poland RD Center
SW Solution Development, Linux Platform

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] [RFC] video: display: Adding frame related ops to MIPI DSI video source struct

[Tomasz Figa]

Hi Vikas,

On Wednesday 02 of January 2013 18:47:22 Vikas C Sajjan wrote:
 From: Vikas Sajjan vikas.saj...@linaro.org
 
 Signed-off-by: Vikas Sajjan vikas.saj...@linaro.org
 ---
  include/video/display.h |6 ++
  1 file changed, 6 insertions(+)
 
 diff --git a/include/video/display.h b/include/video/display.h
 index b639fd0..fb2f437 100644
 --- a/include/video/display.h
 +++ b/include/video/display.h
 @@ -117,6 +117,12 @@ struct dsi_video_source_ops {
 
   void (*enable_hs)(struct video_source *src, bool enable);
 
 + /* frame related */
 + int (*get_frame_done)(struct video_source *src);
 + int (*clear_frame_done)(struct video_source *src);
 + int (*set_early_blank_mode)(struct video_source *src, int power);
 + int (*set_blank_mode)(struct video_source *src, int power);
 +

I'm not sure if all those extra ops are needed in any way.

Looking and Exynos MIPI DSIM driver, set_blank_mode is handling only 
FB_BLANK_UNBLANK status, which basically equals to the already existing 
enable operation, while set_early_blank mode handles only 
FB_BLANK_POWERDOWN, being equal to disable callback.

Both get_frame_done and clear_frame_done do not look at anything used at 
the moment and if frame done status monitoring will be ever needed, I 
think a better way should be implemented.

Best regards,
-- 
Tomasz Figa
Samsung Poland RD Center
SW Solution Development, Linux Platform

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC v2 0/5] Common Display Framework

[Tomasz Figa]

On Tuesday 08 of January 2013 11:12:26 Marcus Lorentzon wrote:
 On 01/08/2013 09:18 AM, Laurent Pinchart wrote:
  On Thursday 27 December 2012 15:43:34 Tomasz Figa wrote:
On Monday 24 of December 2012 15:12:28 Laurent Pinchart wrote:
  On Friday 21 December 2012 11:00:52 Tomasz Figa wrote:
On Tuesday 18 of December 2012 08:31:30 Vikas Sajjan 
wrote:
  On 17 December 2012 20:55, Laurent Pinchart wrote:
Hi Vikas,

Sorry for the late reply. I now have more time to
work on CDF, so
delays should be much shorter.

On Thursday 06 December 2012 10:51:15 Vikas Sajjan 
wrote:
  Hi Laurent,
  
  I was thinking of porting CDF to samsung
  EXYNOS 5250 platform,
  what I found is that, the exynos display
  controller is MIPI DSI
  based controller.
  
  But if I look at CDF patches, it has only
  support for MIPI DBI
  based Display controller.
  
  So my question is, do we have any generic
  framework for MIPI DSI
  based display controller? basically I wanted
  to know, how to go
  about porting CDF for such kind of display
  controller.

MIPI DSI support is not available yet. The only
reason for that is
that I don't have any MIPI DSI hardware to write
and test the code
with:-)

The common display framework should definitely
support MIPI DSI. I
think the existing MIPI DBI code could be used as
a base, so the
implementation shouldn't be too high.

Yeah, i was also thinking in similar lines, below
is my though for
MIPI DSI support in CDF.
  
  o   MIPI DSI support as part of CDF framework will
  expose
  §  mipi_dsi_register_device(mpi_device) (will be
  called mach-xxx-dt.c
  file )
  §  mipi_dsi_register_driver(mipi_driver, bus ops)
  (will be called
  from platform specific init driver call )
  ·bus ops will be
  o   read data
  o   write data
  o   write command
  §  MIPI DSI will be registered as bus_register()
  
  When MIPI DSI probe is called, it (e.g., Exynos or
  OMAP MIPI DSI)
  will initialize the MIPI DSI HW IP.
  
  This probe will also parse the DT file for MIPI DSI
  based panel, add
  the panel device (device_add() ) to kernel and
  register the display
  entity with its control and  video ops with CDF.
  
  I can give this a try.

I am currently in progress of reworking Exynos MIPI DSIM
code and
s6e8ax0 LCD driver to use the v2 RFC of Common Display
Framework. I
have most of the work done, I have just to solve several
remaining
problems.
  
  Do you already have code that you can publish ? I'm
  particularly
  interested (and I think Tomi Valkeinen would be as well) in
  looking at
  the DSI operations you expose to DSI sinks (panels,
  transceivers, ...).

Well, I'm afraid this might be little below your expectations, but
here's an initial RFC of the part defining just the DSI bus. I
need a bit more time for patches for Exynos MIPI DSI master and
s6e8ax0 LCD.
  
  No worries. I was particularly interested in the DSI operations you
  needed to export, they seem pretty simple. Thank you for sharing the
  code.
 FYI,
 here is STE DSI API:
 http://www.igloocommunity.org/gitweb/?p=kernel/igloo-kernel.git;a=blob;f
 =include/video/mcde.h;h=499ce5cfecc9ad77593e761cdcc1624502f28432;hb=HEAD
 #l361
 
 But it is not perfect. After a couple of products we realized that most
 panel drivers want an easy way to send a bunch of init commands in one
 go. So I think it should be an op for sending an array of commands at
 once. Something like
 
 struct dsi_cmd {
  enum mipi_pkt_type type; /* MIPI DSI, DCS, SetPacketLen, ... */
  u8 cmd;
  int dataLen;
  u8 *data;
 }
 struct dsi_ops {
  int dsi_write(source, int num_cmds, struct dsi_cmd *cmds);
  ...
 }

Yes, this should be flexible enough to cover most of (or even whole) DSI 
specification.

However I'm not sure whether the dsi_write name would be appropriate, 
since DSI packet types include also read and special transactions. So, 
according to DSI terminology, maybe dsi_transaction would be better?

 
 The rest of DSI write API could be made helpers on top of this one op.
 This grouping also allows driver to describe intent to send a bunch of
 commands together which might be of interest with mode set (if you need

[RFC PATCH 0/4] Common Display Framework-TF

[Tomasz Figa]

Hi,

After pretty long time of trying to adapt Exynos-specific DSI display support
to Common Display Framework I'm ready to show some preliminary RFC patches.
This series shows some ideas for CDF that came to my mind during my work,
some changes based on comments received by Tomi's edition of CDF and also
preliminarys version of Exynos DSI (video source part only, still with some
FIXMEs) and Samsung S6E8AX0 DSI panel drivers.

It is heavily based on Tomi's work which can be found here:
http://thread.gmane.org/gmane.comp.video.dri.devel/78227

The code might be a bit hacky in places, as I wanted to get it to a kind
of complete and working state first. However I hope that some of the ideas
might useful for further works.

So, here comes the TF edition of Common Clock Framework.


Changes done to Tomi's edition of CDF:

 - Replaced set_operation_mode, set_pixel_format and set_size video source
   operations with get_params display entity operation, as it was originally
   in Laurent's version.

   We have discussed this matter on the mailing list
   and decided that it would be better to have the source ask the sink for
   its parameter structure and do whatever appropriate with it.

 - Defined a preliminary set of DSI bus parameters.

   Following parameters are defined:

   1. Pixel format used for video data transmission.
   2. Mode flags (several bit flags ORed together):
 a) DSI_MODE_VIDEO - entity uses video mode (as opposed to command mode),
following DSI_MODE_VIDEO_* flags are relevant only if this flag is set.
b) DSI_MODE_VIDEO_BURST - entity uses burst transfer for video data
c) DSI_MODE_VIDEO_SYNC_PULSE - entity uses sync pulses (as opposed
   to sync events)
d) DSI_MODE_VIDEO_AUTO_VERT - entity uses automatic video mode
   detection
e) DSI_MODE_VIDEO_HSE - entity needs horizontal sync end packets
f) DSI_MODE_VIDEO_HFP - entity needs horizontal front porch area
g) DSI_MODE_VIDEO_HBP - entity needs horizontal back porch area
h) DSI_MODE_VIDEO_HSA - entity needs horizontal sync active area
 i) DSI_MODE_VSYNC_FLUSH - vertical sync pulse flushes video data
 j) DSI_MODE_EOT_PACKET - entity needs EoT packets
   3. Bit (serial) clock frequency in HS mode.
   4. Escape mode clock frequency.
   5. Mask of used data lanes (each bit represents single lane).
   6. Command allow area in video mode - amount of lines after transmitting
  video data when generic commands are accepted.

   Feel free to suggest anything missing or wrong, as the list of
   parameters is based merely on what was used in original Exynos MIPI
   DSIM driver.

 - Redesigned source-entity matching.

   Now each source has name string and integer id and each entity has
   source name and source id. In addition, they can have of_node specified,
   which is used for Device Tree-based matching.

   The matching procedure is as follows:

   1. Matching takes place when display entity registers.
 a) If there is of_node specified for the entity then video-source
phandle of this node is being parsed and list of registered sources
is traversed in search of a source with of_node received from
parsing the phandle.
 b) Otherwise the matching key is a pair of source name and id.
   2. If no matching source is found, display_entity_register returns
  -EPROBE_DEFER error which causes the entity driver to defer its
  probe until the source registers.
   3. Otherwise an optional bind operation of video source is called,
  sink field of source and source field of entity are set and the
  matching ends successfully.

 - Some initial concept of source-entity cross-locking.

   Only video source is protected at the moment, as I still have to think
   a bit about locking the entity in a way where removing it by user request
   is still possible.

 - Dropped any panels and chips that I can't test.

   They are irrelevant for this series, so there is no point in including them.

 - Added Exynos DSI video source driver.

   This is a new driver for the DSI controller found in Exynos SoCs. It still
   needs some work, but in current state can be considered an example of DSI
   video source implementation for my version of CDF.

 - Added Samsung S6E8AX0 DSI panel driver.

   This is the old Exynos-specific driver, just migrated to CDF and with
   some hacks to provide control over entity state to userspace using
   lcd device. However it can be used to demonstrate video source ops in use.

Feel free to comment as much as you can.

Tomasz Figa (4):
  video: add display-core
  video: add makefile  kconfig
  video: display: Add exynos-dsi video source driver
  video: display: Add Samsung s6e8ax0 display panel driver

 drivers/video/Kconfig |1 +
 drivers/video/Makefile|1 +
 drivers/video/display/Kconfig |   16

[RFC PATCH 3/4] video: display: Add exynos-dsi video source driver

[Tomasz Figa]

This patch adds new driver for DSI master block available in Samsung
Exynos SoCs. The driver is designed and written specifically for Common
Display Framework.

Signed-off-by: Tomasz Figa t.f...@samsung.com
Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
---
 drivers/video/display/Kconfig |3 +
 drivers/video/display/Makefile|1 +
 drivers/video/display/source-exynos_dsi.c | 1313 +
 include/video/exynos_dsi.h|   41 +
 4 files changed, 1358 insertions(+)
 create mode 100644 drivers/video/display/source-exynos_dsi.c
 create mode 100644 include/video/exynos_dsi.h

diff --git a/drivers/video/display/Kconfig b/drivers/video/display/Kconfig
index 477192d..b14527a 100644
--- a/drivers/video/display/Kconfig
+++ b/drivers/video/display/Kconfig
@@ -6,5 +6,8 @@ menuconfig DISPLAY_CORE
 if DISPLAY_CORE
 
 
+config DISPLAY_SOURCE_EXYNOS_DSI
+   tristate Samsung SoC MIPI DSI Master
+
 
 endif # DISPLAY_CORE
diff --git a/drivers/video/display/Makefile b/drivers/video/display/Makefile
index bd93496..40a283a 100644
--- a/drivers/video/display/Makefile
+++ b/drivers/video/display/Makefile
@@ -1 +1,2 @@
 obj-$(CONFIG_DISPLAY_CORE) += display-core.o
+obj-$(CONFIG_DISPLAY_SOURCE_EXYNOS_DSI) += source-exynos_dsi.o
diff --git a/drivers/video/display/source-exynos_dsi.c 
b/drivers/video/display/source-exynos_dsi.c
new file mode 100644
index 000..30b15bf
--- /dev/null
+++ b/drivers/video/display/source-exynos_dsi.c
@@ -0,0 +1,1313 @@
+/*
+ * Samsung SoC MIPI DSI Master driver.
+ *
+ * Copyright (c) 2012 Samsung Electronics Co., Ltd
+ *
+ * Contacts: Tomasz Figa t.f...@samsung.com
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+*/
+
+#include linux/clk.h
+#include linux/completion.h
+#include linux/delay.h
+#include linux/errno.h
+#include linux/fb.h
+#include linux/interrupt.h
+#include linux/io.h
+#include linux/irq.h
+#include linux/kernel.h
+#include linux/list.h
+#include linux/memory.h
+#include linux/mm.h
+#include linux/module.h
+#include linux/of.h
+#include linux/platform_device.h
+#include linux/pm_runtime.h
+#include linux/regulator/consumer.h
+#include linux/spinlock.h
+#include linux/videomode.h
+#include linux/wait.h
+
+#include video/display.h
+#include video/exynos_dsi.h
+#include video/mipi_display.h
+
+#define DSIM_STATUS_REG0x0 /* Status register */
+#define DSIM_SWRST_REG 0x4 /* Software reset register */
+#define DSIM_CLKCTRL_REG   0x8 /* Clock control register */
+#define DSIM_TIMEOUT_REG   0xc /* Time out register */
+#define DSIM_CONFIG_REG0x10/* Configuration register */
+#define DSIM_ESCMODE_REG   0x14/* Escape mode register */
+
+/* Main display image resolution register */
+#define DSIM_MDRESOL_REG   0x18
+#define DSIM_MVPORCH_REG   0x1c/* Main display Vporch register */
+#define DSIM_MHPORCH_REG   0x20/* Main display Hporch register */
+#define DSIM_MSYNC_REG 0x24/* Main display sync area register */
+
+/* Sub display image resolution register */
+#define DSIM_SDRESOL_REG   0x28
+#define DSIM_INTSRC_REG0x2c/* Interrupt source register */
+#define DSIM_INTMSK_REG0x30/* Interrupt mask register */
+#define DSIM_PKTHDR_REG0x34/* Packet Header FIFO register 
*/
+#define DSIM_PAYLOAD_REG   0x38/* Payload FIFO register */
+#define DSIM_RXFIFO_REG0x3c/* Read FIFO register */
+#define DSIM_FIFOTHLD_REG  0x40/* FIFO threshold level register */
+#define DSIM_FIFOCTRL_REG  0x44/* FIFO status and control register */
+
+/* FIFO memory AC characteristic register */
+#define DSIM_PLLCTRL_REG   0x4c/* PLL control register */
+#define DSIM_PLLTMR_REG0x50/* PLL timer register */
+#define DSIM_PHYACCHR_REG  0x54/* D-PHY AC characteristic register */
+#define DSIM_PHYACCHR1_REG 0x58/* D-PHY AC characteristic register1 */
+
+/* DSIM_STATUS */
+#define DSIM_STOP_STATE_DAT(x) (((x)  0xf)  0)
+#define DSIM_STOP_STATE_CLK(1  8)
+#define DSIM_TX_READY_HS_CLK   (1  10)
+#define DSIM_PLL_STABLE(1  31)
+
+/* DSIM_SWRST */
+#define DSIM_FUNCRST   (1  16)
+#define DSIM_SWRST (1  0)
+
+/* DSIM_TIMEOUT */
+#define DSIM_LPDR_TOUT(x)  ((x)  0)
+#define DSIM_BTA_TOUT(x)   ((x)  16)
+
+/* DSIM_CLKCTRL */
+#define DSIM_ESC_PRESCALER(x)  (((x)  0x)  0)
+#define DSIM_ESC_PRESCALER_MASK(0x  0)
+#define DSIM_LANE_ESC_CLK_EN_CLK   (1  19)
+#define DSIM_LANE_ESC_CLK_EN_DATA(x)   (((x)  0xf)  20)
+#define DSIM_LANE_ESC_CLK_EN_DATA_MASK (0xf  20)
+#define DSIM_BYTE_CLKEN(1  24)
+#define

[RFC PATCH 4/4] video: display: Add Samsung s6e8ax0 display panel driver

[Tomasz Figa]

This patch adds Common Display Framework driver for Samsung s6e8ax0
MIPI DSI display panel.

Signed-off-by: Tomasz Figa t.f...@samsung.com
Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
---
 drivers/video/display/Kconfig |3 +
 drivers/video/display/Makefile|1 +
 drivers/video/display/panel-s6e8ax0.c | 1027 +
 include/video/panel-s6e8ax0.h |   41 ++
 4 files changed, 1072 insertions(+)
 create mode 100644 drivers/video/display/panel-s6e8ax0.c
 create mode 100644 include/video/panel-s6e8ax0.h

diff --git a/drivers/video/display/Kconfig b/drivers/video/display/Kconfig
index b14527a..f19ec04 100644
--- a/drivers/video/display/Kconfig
+++ b/drivers/video/display/Kconfig
@@ -5,6 +5,9 @@ menuconfig DISPLAY_CORE
 
 if DISPLAY_CORE
 
+config DISPLAY_PANEL_S6E8AX0
+   tristate S6E8AX0 DSI video mode panel
+   select OF_VIDEOMODE
 
 config DISPLAY_SOURCE_EXYNOS_DSI
tristate Samsung SoC MIPI DSI Master
diff --git a/drivers/video/display/Makefile b/drivers/video/display/Makefile
index 40a283a..0f7fdc2 100644
--- a/drivers/video/display/Makefile
+++ b/drivers/video/display/Makefile
@@ -1,2 +1,3 @@
 obj-$(CONFIG_DISPLAY_CORE) += display-core.o
+obj-$(CONFIG_DISPLAY_PANEL_S6E8AX0) += panel-s6e8ax0.o
 obj-$(CONFIG_DISPLAY_SOURCE_EXYNOS_DSI) += source-exynos_dsi.o
diff --git a/drivers/video/display/panel-s6e8ax0.c 
b/drivers/video/display/panel-s6e8ax0.c
new file mode 100644
index 000..4c09fe2
--- /dev/null
+++ b/drivers/video/display/panel-s6e8ax0.c
@@ -0,0 +1,1027 @@
+/* linux/drivers/video/exynos/s6e8ax0.c
+ *
+ * MIPI-DSI based s6e8ax0 AMOLED lcd 4.65 inch panel driver.
+ *
+ * Inki Dae, inki@samsung.com
+ * Donghwa Lee, dh09@samsung.com
+ * Tomasz Figa, t.f...@samsung.com
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include linux/module.h
+#include linux/kernel.h
+#include linux/errno.h
+#include linux/mutex.h
+#include linux/wait.h
+#include linux/ctype.h
+#include linux/io.h
+#include linux/delay.h
+#include linux/irq.h
+#include linux/interrupt.h
+
+#include linux/fb.h
+#include linux/gpio.h
+#include linux/lcd.h
+#include linux/of.h
+#include linux/of_gpio.h
+#include linux/of_videomode.h
+#include linux/platform_device.h
+#include linux/pm_runtime.h
+#include linux/backlight.h
+#include linux/regulator/consumer.h
+
+#include video/display.h
+#include video/mipi_display.h
+#include video/panel-s6e8ax0.h
+
+#define LDI_MTP_LENGTH 24
+#define DSIM_PM_STABLE_TIME10
+#define MIN_BRIGHTNESS 0
+#define MAX_BRIGHTNESS 24
+#define GAMMA_TABLE_COUNT  26
+
+struct s6e8ax0 {
+   struct display_entity entity;
+   struct device *dev;
+
+   struct s6e8ax0_platform_data *pdata;
+   struct backlight_device *bd;
+   struct lcd_device *ld;
+   struct regulator_bulk_data supplies[2];
+
+   bool enabled;
+   unsigned int id;
+   unsigned int gamma;
+   unsigned int acl_enable;
+   unsigned int cur_acl;
+   int power;
+
+   unsigned int reset_gpio;
+};
+
+#define to_panel(p)container_of(p, struct s6e8ax0, entity)
+
+static const unsigned char s6e8ax0_22_gamma_30[] = {
+   0xfa, 0x01, 0x60, 0x10, 0x60, 0xf5, 0x00, 0xff, 0xad, 0xaf,
+   0xbA, 0xc3, 0xd8, 0xc5, 0x9f, 0xc6, 0x9e, 0xc1, 0xdc, 0xc0,
+   0x00, 0x61, 0x00, 0x5a, 0x00, 0x74,
+};
+
+static const unsigned char s6e8ax0_22_gamma_50[] = {
+   0xfa, 0x01, 0x60, 0x10, 0x60, 0xe8, 0x1f, 0xf7, 0xad, 0xc0,
+   0xb5, 0xc4, 0xdc, 0xc4, 0x9e, 0xc6, 0x9c, 0xbb, 0xd8, 0xbb,
+   0x00, 0x70, 0x00, 0x68, 0x00, 0x86,
+};
+
+static const unsigned char s6e8ax0_22_gamma_60[] = {
+   0xfa, 0x01, 0x60, 0x10, 0x60, 0xde, 0x1f, 0xef, 0xad, 0xc4,
+   0xb3, 0xc3, 0xdd, 0xc4, 0x9e, 0xc6, 0x9c, 0xbc, 0xd6, 0xba,
+   0x00, 0x75, 0x00, 0x6e, 0x00, 0x8d,
+};
+
+static const unsigned char s6e8ax0_22_gamma_70[] = {
+   0xfa, 0x01, 0x60, 0x10, 0x60, 0xd8, 0x1f, 0xe7, 0xaf, 0xc8,
+   0xb4, 0xc4, 0xdd, 0xc3, 0x9d, 0xc6, 0x9c, 0xbb, 0xd6, 0xb9,
+   0x00, 0x7a, 0x00, 0x72, 0x00, 0x93,
+};
+
+static const unsigned char s6e8ax0_22_gamma_80[] = {
+   0xfa, 0x01, 0x60, 0x10, 0x60, 0xc9, 0x1f, 0xde, 0xae, 0xc9,
+   0xb1, 0xc3, 0xdd, 0xc2, 0x9d, 0xc5, 0x9b, 0xbc, 0xd6, 0xbb,
+   0x00, 0x7f, 0x00, 0x77, 0x00, 0x99,
+};
+
+static const unsigned char s6e8ax0_22_gamma_90[] = {
+   0xfa, 0x01, 0x60, 0x10, 0x60, 0xc7, 0x1f, 0xd9, 0xb0, 0xcc,
+   0xb2, 0xc3, 0xdc, 0xc1, 0x9c, 0xc6, 0x9c, 0xbc, 0xd4, 0xb9,
+   0x00, 0x83, 0x00, 0x7b, 0x00, 0x9e,
+};
+
+static const unsigned char s6e8ax0_22_gamma_100[] = {
+   0xfa, 0x01, 0x60, 0x10, 0x60, 0xbd, 0x80, 0xcd, 0xba, 0xce,
+   0xb3, 0xc4, 0xde, 0xc3, 0x9c, 0xc4, 0x9, 0xb8, 0xd3, 0xb6,
+   0x00, 0x88, 0x00, 0x80, 0x00, 0xa5,
+};
+
+static const unsigned char

Re: [RFC PATCH 0/4] Common Display Framework-TF

[Tomasz Figa]

Hi Laurent,

On Saturday 02 of February 2013 11:39:59 Laurent Pinchart wrote:
 Hi Tomasz,
 
 Thank you for your RFC.
 
 On Wednesday 30 January 2013 16:38:59 Tomasz Figa wrote:
  Hi,
  
  After pretty long time of trying to adapt Exynos-specific DSI display
  support to Common Display Framework I'm ready to show some preliminary
  RFC patches. This series shows some ideas for CDF that came to my
  mind during my work, some changes based on comments received by
  Tomi's edition of CDF and also preliminarys version of Exynos DSI
  (video source part only, still with some FIXMEs) and Samsung S6E8AX0
  DSI panel drivers.
  
  It is heavily based on Tomi's work which can be found here:
  http://thread.gmane.org/gmane.comp.video.dri.devel/78227
  
  The code might be a bit hacky in places, as I wanted to get it to a
  kind of complete and working state first. However I hope that some of
  the ideas might useful for further works.
  
  So, here comes the TF edition of Common Clock Framework.
  
  
  Changes done to Tomi's edition of CDF:
   - Replaced set_operation_mode, set_pixel_format and set_size video
   source  
 operations with get_params display entity operation, as it was
 originally in Laurent's version.
 
 We have discussed this matter on the mailing list and decided that
 it
 would be better to have the source ask the sink for its parameter
 structure and do whatever appropriate with it.
 
 Could you briefly outline the rationale behind this ?

Display interfaces may be described by an arbitrary number of parameters. 
Some will require just video timings, others like DSI will require a 
significant number of additional bus-specific ones.

Separating all the parameters (all of which are usually set at the same 
time) into a lot of ops setting single parameter will just add unnecessary 
complexity.

 I'm wondering whether get_params could limit us if a device needs to
 modify parameters at runtime. For instance a panel might need to change
 clock frequencies or number of used data lane depending on various
 runtime conditions. I don't have a real use case here, so it might just
 be a bit of over-engineering.

Hmm, isn't it in the opposite direction (the user requests the display 
interface to change, for example, video mode, which in turn reconfigures 
the link and requests the panel to update its settings)?

However it might be reasonable to split the parameters into constant and 
variable ones. In case of DSI bus, there is a lot of parameters that are 
considered just at link initialization time (the whole dsi params struct I 
defined). Video mode, however, is a variable parameter that can be changed 
on some displays.

 
   - Defined a preliminary set of DSI bus parameters.
   
 Following parameters are defined:
 
 1. Pixel format used for video data transmission.
 
 2. Mode flags (several bit flags ORed together):
   a) DSI_MODE_VIDEO - entity uses video mode (as opposed to command
   
  mode), following DSI_MODE_VIDEO_* flags are relevant only if
  this
  flag is set.
  b) DSI_MODE_VIDEO_BURST - entity uses burst transfer for video
  data
  c) DSI_MODE_VIDEO_SYNC_PULSE - entity uses sync pulses (as
  opposed
  
 to sync events)
  
  d) DSI_MODE_VIDEO_AUTO_VERT - entity uses automatic video mode
  
 detection
  
  e) DSI_MODE_VIDEO_HSE - entity needs horizontal sync end
  packets
  f) DSI_MODE_VIDEO_HFP - entity needs horizontal front porch
  area
  g) DSI_MODE_VIDEO_HBP - entity needs horizontal back porch
  area
  h) DSI_MODE_VIDEO_HSA - entity needs horizontal sync active
  area
   
   i) DSI_MODE_VSYNC_FLUSH - vertical sync pulse flushes video data
   j) DSI_MODE_EOT_PACKET - entity needs EoT packets
 
 3. Bit (serial) clock frequency in HS mode.
 4. Escape mode clock frequency.
 5. Mask of used data lanes (each bit represents single lane).
 
 From my experience with MIPI CSI (Camera Serial Interface, similar to
 DSI) some transceivers can reroute data lanes internally. Is that true
 for DSI as well ? In that case we would need a list of data lanes, not
 just a mask.

Hmm, I don't remember at the moment, will have to look to the 
specification. Exynos DSI master doesn't support such feature.

 6. Command allow area in video mode - amount of lines after
 transmitting
video data when generic commands are accepted.
 
 Feel free to suggest anything missing or wrong, as the list of
 parameters is based merely on what was used in original Exynos MIPI
 DSIM driver.
   
   - Redesigned source-entity matching.
   
 Now each source has name string and integer id and each entity has
 source name and source id. In addition, they can have of_node

Re: [RFC PATCH 4/4] video: display: Add Samsung s6e8ax0 display panel driver

[Tomasz Figa]

On Thursday 07 of February 2013 15:04:30 Vikas Sajjan wrote:
 Hi Figa,
 
 On Wed, Jan 30, 2013 at 9:09 PM, Tomasz Figa t.f...@samsung.com wrote:
  This patch adds Common Display Framework driver for Samsung s6e8ax0
  MIPI DSI display panel.
  
  Signed-off-by: Tomasz Figa t.f...@samsung.com
  Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
  ---
  
   drivers/video/display/Kconfig |3 +
   drivers/video/display/Makefile|1 +
   drivers/video/display/panel-s6e8ax0.c | 1027
   + include/video/panel-s6e8ax0.h 
  |   41 ++
   4 files changed, 1072 insertions(+)
   create mode 100644 drivers/video/display/panel-s6e8ax0.c
   create mode 100644 include/video/panel-s6e8ax0.h
  
[snip]
  +   lcd-ld = lcd_device_register(s6e8ax0, pdev-dev, lcd,
  +   s6e8ax0_lcd_ops);
  +   if (IS_ERR(lcd-ld)) {
  +   dev_err(pdev-dev, failed to register lcd ops.\n);
  +   ret = PTR_ERR(lcd-ld);
  +   goto err_lcd_register;
  +   }
  +
  +   lcd-bd = backlight_device_register(s6e8ax0-bl, pdev-dev,
  lcd, +   s6e8ax0_backlight_ops, NULL);
  +   if (IS_ERR(lcd-bd)) {
  +   dev_err(pdev-dev, failed to register backlight
  ops.\n); +   ret = PTR_ERR(lcd-bd);
  +   goto err_backlight_register;
  +   }
  +
 
 I think we should try to remove the dependency with LCD framework and
 Backlight framework, and incorporate those functionality as par of
 CDF.
 you can refer to my similar patch Make s6e8ax0 panel driver compliant
 with CDF (
 http://comments.gmane.org/gmane.linux.drivers.video-input-infrastructur
 e/59187 ) which i had posted couple of weeks back, where I made an
 attempt to remove lcd_ops dependency.

Yes, I have written in the cover letter that those interfaces is just a 
hack to be able to control the display from userspace in current state of 
CDF.

I agree that CDF will have to be extended with backlight/brightness 
control. However currently CDF does not expose any interface to userspace.

Laurent, what's your opinion on this?

P.S. Tomasz is my first name.

Best regards,
-- 
Tomasz Figa
Samsung Poland RD Center
SW Solution Development, Linux Platform

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4] drm/exynos: prepare FIMD clocks

[Tomasz Figa]

Hi,

On Monday 08 of April 2013 16:41:54 Viresh Kumar wrote:
 On 8 April 2013 16:37, Vikas Sajjan vikas.saj...@linaro.org wrote:
  While migrating to common clock framework (CCF), I found that the FIMD
  clocks were pulled down by the CCF.
  If CCF finds any clock(s) which has NOT been claimed by any of the
  drivers, then such clock(s) are PULLed low by CCF.
  
  Calling clk_prepare() for FIMD clocks fixes the issue.
  
  This patch also replaces clk_disable() with clk_unprepare() during
  exit, since clk_prepare() is called in fimd_probe().
 
 I asked you about fixing your commit log too.. It still looks incorrect
 to me.
 
 This patch doesn't have anything to do with CCF pulling clocks down, but
 calling clk_prepare() before clk_enable() is must now.. that's it..
 nothing more.
 

I fully agree.

The message should be something like:

Common Clock Framework introduced the need to prepare clocks before 
enabling them, otherwise clk_enable() fails. This patch adds clk_prepare 
calls to the driver.

and that's all.

What you are observing as CCF pulling clocks down is the fact that 
clk_enable() fails if the clock is not prepared and so the clock is not 
enabled in result.

Another thing is that CCF is not pulling anything down. GPIO pins can be 
pulled down (or up or not pulled), but clocks can be masked, gated or 
simply disabled - this does not imply their signal level.

  Signed-off-by: Vikas Sajjan vikas.saj...@linaro.org
  ---
  
  Changes since v3:
  - added clk_prepare() in fimd_probe() and clk_unprepare() in
  fimd_remove() 
   as suggested by Viresh Kumar viresh.ku...@linaro.org
  
  Changes since v2:
  - moved clk_prepare_enable() and clk_disable_unprepare() from
  fimd_probe() to fimd_clock() as suggested by Inki Dae
  inki@samsung.com 
  Changes since v1:
  - added error checking for clk_prepare_enable() and also
  replaced
  clk_disable() with clk_disable_unprepare() during exit.
  
  ---
  
   drivers/gpu/drm/exynos/exynos_drm_fimd.c |   14 --
   1 file changed, 12 insertions(+), 2 deletions(-)
  
  diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
  b/drivers/gpu/drm/exynos/exynos_drm_fimd.c index 9537761..aa22370
  100644
  --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
  +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
  @@ -934,6 +934,16 @@ static int fimd_probe(struct platform_device
  *pdev) 
  return ret;
  
  }
  
  +   ret = clk_prepare(ctx-bus_clk);
  +   if (ret  0)
  +   return ret;
  +
  +   ret = clk_prepare(ctx-lcd_clk);
  +   if  (ret  0) {
  +   clk_unprepare(ctx-bus_clk);
  +   return ret;
  +   }
  +

Why not just simply use clk_prepare_enable() instead of all calls to 
clk_enable() in the driver?

Same goes for s/clk_disable/clk_disable_unprepare/ .

  
  ctx-vidcon0 = pdata-vidcon0;
  ctx-vidcon1 = pdata-vidcon1;
  ctx-default_win = pdata-default_win;
  
  @@ -981,8 +991,8 @@ static int fimd_remove(struct platform_device
  *pdev) 
  if (ctx-suspended)
  
  goto out;
  
  -   clk_disable(ctx-lcd_clk);
  -   clk_disable(ctx-bus_clk);
  +   clk_unprepare(ctx-lcd_clk);
  +   clk_unprepare(ctx-bus_clk);
 
 This looks wrong again.. You still need to call clk_disable() to make
 clk enabled
 count zero...

Viresh is right again here.

Best regards,
Tomasz

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4] drm/exynos: prepare FIMD clocks

[Tomasz Figa]

On Sunday 21 of April 2013 13:23:10 Viresh Kumar wrote:
 On 20 April 2013 20:56, Inki Dae inki@samsung.com wrote:
  Sorry for being late. I think clk_prepare/unprepare are nothing to do
  yet in case of Exynos but those might be used for in the future so
  your patch looks good to me as is.
  
  Applied. :)
 

Hmm. Now, after searching for this thread in dri-devel archives, I'm 
wondering why I haven't received some of messages from this thread through 
linux-samsung-soc mailing list...

I believe linux-samsung-soc list exists to collect all threads related to 
Samsung SoCs for people that don't want to subscribe to lists like dri-
devel, on which there is a lot of threads irrelevant to them, with the 
risk of missing the important ones.

Please always make sure that any discussion about Samsung SoCs (patches in 
particular) is going through linux-samsung-soc as well.

Thanks in advance.

Best regards,
Tomasz

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4] drm/exynos: prepare FIMD clocks

[Tomasz Figa]

Hi Inki,

On Sunday 21 of April 2013 23:05:45 Inki Dae wrote:
 2013/4/21 Tomasz Figa tomasz.f...@gmail.com
 
  On Sunday 21 of April 2013 13:23:10 Viresh Kumar wrote:
   On 20 April 2013 20:56, Inki Dae inki@samsung.com wrote:
Sorry for being late. I think clk_prepare/unprepare are nothing to
do
yet in case of Exynos but those might be used for in the future so
your patch looks good to me as is.

Applied. :)
  
  Hmm. Now, after searching for this thread in dri-devel archives, I'm
  wondering why I haven't received some of messages from this thread
  through linux-samsung-soc mailing list...
  
  I believe linux-samsung-soc list exists to collect all threads related
  to Samsung SoCs for people that don't want to subscribe to lists like
  dri- devel, on which there is a lot of threads irrelevant to them,
  with the risk of missing the important ones.
  
  Please always make sure that any discussion about Samsung SoCs
  (patches in particular) is going through linux-samsung-soc as well.
 
 Thanks for your advice. As you said, some people might not want to
 subscribe to some mainling lists they don't want. And I think that the
 main mailing list on this patch is dri-devel so you must receive this
 email thread if you subscribed to the dri-devel.

I agree that dri-devel is the target mailing list for DRM patches, but 
AFAIK all threads related to Samsung SoCs should be sent to linux-samsung-
soc as well.

For example, I don't subscribe dri-devel, but I do linux-samsung-soc, 
because all I want to follow is all the works related to Samsung SoCs.
Remaining threads on dri-devel are outside of my competencies.

 Anyway it would be
 best to share this all mailing lists included in this email thread but
 if so, I have no doubt to receive email bumb. :(

Hmm, you don't have to subscribe to a mailing list to post to it.

Actually I'm wondering if the fact that your previous messages did not get 
to the linux-samsung-soc list was not caused by presence of HTML part in 
your messages, which is strongly discouraged on all mailing lists and 
actually blocked on vger.kernel.org where linux-samsung-soc is hosted.

Best regards,
Tomasz

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4] drm/exynos: prepare FIMD clocks

[Tomasz Figa]

Hi Inki,

On Sunday 21 of April 2013 22:36:08 Inki Dae wrote:
 2013/4/21 Tomasz Figa tomasz.f...@gmail.com
 
  Hi,
  
  On Monday 08 of April 2013 16:41:54 Viresh Kumar wrote:
   On 8 April 2013 16:37, Vikas Sajjan vikas.saj...@linaro.org wrote:
While migrating to common clock framework (CCF), I found that the
FIMD
clocks were pulled down by the CCF.
If CCF finds any clock(s) which has NOT been claimed by any of the
drivers, then such clock(s) are PULLed low by CCF.

Calling clk_prepare() for FIMD clocks fixes the issue.

This patch also replaces clk_disable() with clk_unprepare() during
exit, since clk_prepare() is called in fimd_probe().
   
   I asked you about fixing your commit log too.. It still looks
   incorrect
   to me.
   
   This patch doesn't have anything to do with CCF pulling clocks down,
   but calling clk_prepare() before clk_enable() is must now.. that's
   it.. nothing more.
  
  I fully agree.
  
  The message should be something like:
  
  Common Clock Framework introduced the need to prepare clocks before
  enabling them, otherwise clk_enable() fails. This patch adds
  clk_prepare calls to the driver.
  
  and that's all.
  
  What you are observing as CCF pulling clocks down is the fact that
  clk_enable() fails if the clock is not prepared and so the clock is
  not
  enabled in result.
  
  Another thing is that CCF is not pulling anything down. GPIO pins can
  be pulled down (or up or not pulled), but clocks can be masked, gated
  or simply disabled - this does not imply their signal level.
  
Signed-off-by: Vikas Sajjan vikas.saj...@linaro.org
---

Changes since v3:
- added clk_prepare() in fimd_probe() and clk_unprepare()
in
fimd_remove()

 as suggested by Viresh Kumar viresh.ku...@linaro.org

Changes since v2:
- moved clk_prepare_enable() and clk_disable_unprepare()
from
fimd_probe() to fimd_clock() as suggested by Inki Dae
inki@samsung.com

Changes since v1:
- added error checking for clk_prepare_enable() and also
replaced
clk_disable() with clk_disable_unprepare() during exit.

---

 drivers/gpu/drm/exynos/exynos_drm_fimd.c |   14 --
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
b/drivers/gpu/drm/exynos/exynos_drm_fimd.c index 9537761..aa22370
100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
@@ -934,6 +934,16 @@ static int fimd_probe(struct platform_device
*pdev)

return ret;

}

+   ret = clk_prepare(ctx-bus_clk);
+   if (ret  0)
+   return ret;
+
+   ret = clk_prepare(ctx-lcd_clk);
+   if  (ret  0) {
+   clk_unprepare(ctx-bus_clk);
+   return ret;
+   }
+
  
  Why not just simply use clk_prepare_enable() instead of all calls to
  clk_enable() in the driver?
  
  Same goes for s/clk_disable/clk_disable_unprepare/ .
 
 I agree with you. Using clk_prepare_enable() is more clear. Actually I
 had already commented on this. Please see the patch v2. But this way
 also looks good to me.

Well, both versions are technically correct and will have the same effect 
for Exynos SoC clocks, since only enable/disable ops change hardware 
state.

However if we look at general meaning of those generic ops, the clock will 
remain prepared for all the time the driver is loaded, even if the device 
is runtime suspended. Again on Exynos SoCs this won't have any effect, but 
I think we should respect general Common Clock Framework semantics anyway.

ctx-vidcon0 = pdata-vidcon0;
ctx-vidcon1 = pdata-vidcon1;
ctx-default_win = pdata-default_win;

@@ -981,8 +991,8 @@ static int fimd_remove(struct platform_device
*pdev)

if (ctx-suspended)

goto out;

-   clk_disable(ctx-lcd_clk);
-   clk_disable(ctx-bus_clk);
+   clk_unprepare(ctx-lcd_clk);
+   clk_unprepare(ctx-bus_clk);
   
   This looks wrong again.. You still need to call clk_disable() to
   make
   clk enabled
   count zero...
  
  Viresh is right again here.
 
 Ok, you two guys say together this looks wrong so I'd like to take more
 checking. I thought that clk-clk_enable is 1 at here and it would be 0
 by pm_runtimg_put_sync(). Is there any my missing point?

You're reasoning is correct, but only assuming that runtime PM is enabled. 
When it is disabled, pm_runtime_put_sync() is a no-op.

Well, after digging into the exynos_drm_fimd driver a bit more, it seems 
like its power management code needs a serious rework, because I was able 
to find more problems:

1) fimd_activate

[PATCH] MAINTAINERS: Add linux-samsung-soc list to all related entries

[Tomasz Figa]

Several entries in MAINTAINERS file related to Samsung SoCs do not point
to linux-samsung-soc mailing list, which is supposed to collect all
Samsung SoC-related threads, to ease following of Samsung SoC-related
work. This leads to a problem with people skipping this mailing list in
their posts, even though they are related to Samsung SoCs.

This patch adds pointers to linux-samsung-soc mailing list to affected
entries of MAINTAINERS file.

Signed-off-by: Tomasz Figa tomasz.f...@gmail.com
---
 MAINTAINERS | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 6c0d68b..07cb8da 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1139,6 +1139,7 @@ F:arch/arm/mach-exynos*/
 ARM/SAMSUNG MOBILE MACHINE SUPPORT
 M: Kyungmin Park kyungmin.p...@samsung.com
 L: linux-arm-ker...@lists.infradead.org (moderated for non-subscribers)
+L: linux-samsung-...@vger.kernel.org (moderated for non-subscribers)
 S: Maintained
 F: arch/arm/mach-s5pv210/mach-aquila.c
 F: arch/arm/mach-s5pv210/mach-goni.c
@@ -1150,6 +1151,7 @@ M:Kyungmin Park kyungmin.p...@samsung.com
 M: Kamil Debski k.deb...@samsung.com
 L: linux-arm-ker...@lists.infradead.org
 L: linux-me...@vger.kernel.org
+L: linux-samsung-...@vger.kernel.org (moderated for non-subscribers)
 S: Maintained
 F: drivers/media/platform/s5p-g2d/
 
@@ -1158,6 +1160,7 @@ M:Kyungmin Park kyungmin.p...@samsung.com
 M: Sylwester Nawrocki s.nawro...@samsung.com
 L: linux-arm-ker...@lists.infradead.org
 L: linux-me...@vger.kernel.org
+L: linux-samsung-...@vger.kernel.org (moderated for non-subscribers)
 S: Maintained
 F: arch/arm/plat-samsung/include/plat/*fimc*
 F: drivers/media/platform/s5p-fimc/
@@ -1168,6 +1171,7 @@ M:Kamil Debski k.deb...@samsung.com
 M: Jeongtae Park jtp.p...@samsung.com
 L: linux-arm-ker...@lists.infradead.org
 L: linux-me...@vger.kernel.org
+L: linux-samsung-...@vger.kernel.org (moderated for non-subscribers)
 S: Maintained
 F: arch/arm/plat-samsung/s5p-dev-mfc.c
 F: drivers/media/platform/s5p-mfc/
@@ -1177,6 +1181,7 @@ M:Kyungmin Park kyungmin.p...@samsung.com
 M: Tomasz Stanislawski t.stanisl...@samsung.com
 L: linux-arm-ker...@lists.infradead.org
 L: linux-me...@vger.kernel.org
+L: linux-samsung-...@vger.kernel.org (moderated for non-subscribers)
 S: Maintained
 F: drivers/media/platform/s5p-tv/
 
@@ -2679,6 +2684,7 @@ M:Joonyoung Shim jy0922.s...@samsung.com
 M: Seung-Woo Kim sw0312@samsung.com
 M: Kyungmin Park kyungmin.p...@samsung.com
 L: dri-devel@lists.freedesktop.org
+L: linux-samsung-...@vger.kernel.org (moderated for non-subscribers)
 T: git git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git
 S: Supported
 F: drivers/gpu/drm/exynos
@@ -3142,6 +3148,7 @@ F:Documentation/extcon/
 EXYNOS DP DRIVER
 M: Jingoo Han jg1@samsung.com
 L: linux-fb...@vger.kernel.org
+L: linux-samsung-...@vger.kernel.org (moderated for non-subscribers)
 S: Maintained
 F: drivers/video/exynos/exynos_dp*
 F: include/video/exynos_dp*
@@ -3151,6 +3158,7 @@ M:Inki Dae inki@samsung.com
 M: Donghwa Lee dh09@samsung.com
 M: Kyungmin Park kyungmin.p...@samsung.com
 L: linux-fb...@vger.kernel.org
+L: linux-samsung-...@vger.kernel.org (moderated for non-subscribers)
 S: Maintained
 F: drivers/video/exynos/exynos_mipi*
 F: include/video/exynos_mipi*
@@ -6892,12 +6900,14 @@ F:  drivers/platform/x86/samsung-laptop.c
 SAMSUNG AUDIO (ASoC) DRIVERS
 M: Sangbeom Kim sbki...@samsung.com
 L: alsa-de...@alsa-project.org (moderated for non-subscribers)
+L: linux-samsung-...@vger.kernel.org (moderated for non-subscribers)
 S: Supported
 F: sound/soc/samsung
 
 SAMSUNG FRAMEBUFFER DRIVER
 M: Jingoo Han jg1@samsung.com
 L: linux-fb...@vger.kernel.org
+L: linux-samsung-...@vger.kernel.org (moderated for non-subscribers)
 S: Maintained
 F: drivers/video/s3c-fb.c
 
@@ -7087,6 +7097,7 @@ F:drivers/mmc/host/sdhci-pltfm.[ch]
 SECURE DIGITAL HOST CONTROLLER INTERFACE (SDHCI) SAMSUNG DRIVER
 M: Ben Dooks ben-li...@fluff.org
 L: linux-...@vger.kernel.org
+L: linux-samsung-...@vger.kernel.org (moderated for non-subscribers)
 S: Maintained
 F: drivers/mmc/host/sdhci-s3c.c
 
-- 
1.8.2.1

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4] drm/exynos: prepare FIMD clocks

[Tomasz Figa]

On Sunday 21 of April 2013 22:36:08 Inki Dae wrote:
   2013/4/21 Tomasz Figa tomasz.f...@gmail.com
  
Hi,
   
On Monday 08 of April 2013 16:41:54 Viresh Kumar wrote:
 On 8 April 2013 16:37, Vikas Sajjan vikas.saj...@linaro.org wrote:
  While migrating to common clock framework (CCF), I found that the
  FIMD
  clocks were pulled down by the CCF.
  If CCF finds any clock(s) which has NOT been claimed by any of the
  drivers, then such clock(s) are PULLed low by CCF.
 
  Calling clk_prepare() for FIMD clocks fixes the issue.
 
  This patch also replaces clk_disable() with clk_unprepare() during
  exit, since clk_prepare() is called in fimd_probe().

 I asked you about fixing your commit log too.. It still looks
 incorrect
 to me.

 This patch doesn't have anything to do with CCF pulling clocks down,
 but calling clk_prepare() before clk_enable() is must now.. that's
 it.. nothing more.
   
I fully agree.
   
The message should be something like:
   
Common Clock Framework introduced the need to prepare clocks before
enabling them, otherwise clk_enable() fails. This patch adds
clk_prepare calls to the driver.
   
and that's all.
   
What you are observing as CCF pulling clocks down is the fact that
clk_enable() fails if the clock is not prepared and so the clock is
not
enabled in result.
   
Another thing is that CCF is not pulling anything down. GPIO pins can
be pulled down (or up or not pulled), but clocks can be masked, gated
or simply disabled - this does not imply their signal level.
   
  Signed-off-by: Vikas Sajjan vikas.saj...@linaro.org
  ---
 
  Changes since v3:
  - added clk_prepare() in fimd_probe() and clk_unprepare()
  in
  fimd_remove()
 
   as suggested by Viresh Kumar viresh.ku...@linaro.org
 
  Changes since v2:
  - moved clk_prepare_enable() and clk_disable_unprepare()
  from
  fimd_probe() to fimd_clock() as suggested by Inki Dae
  inki@samsung.com
 
  Changes since v1:
  - added error checking for clk_prepare_enable() and also
  replaced
  clk_disable() with clk_disable_unprepare() during exit.
 
  ---
 
   drivers/gpu/drm/exynos/exynos_drm_fimd.c |   14 --
   1 file changed, 12 insertions(+), 2 deletions(-)
 
  diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
  b/drivers/gpu/drm/exynos/exynos_drm_fimd.c index 9537761..aa22370
  100644
  --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
  +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
  @@ -934,6 +934,16 @@ static int fimd_probe(struct platform_device
  *pdev)
 
  return ret;
 
  }
 
  +   ret = clk_prepare(ctx-bus_clk);
  +   if (ret  0)
  +   return ret;
  +
  +   ret = clk_prepare(ctx-lcd_clk);
  +   if  (ret  0) {
  +   clk_unprepare(ctx-bus_clk);
  +   return ret;
  +   }
  +
   
Why not just simply use clk_prepare_enable() instead of all calls to
clk_enable() in the driver?
   
Same goes for s/clk_disable/clk_disable_unprepare/ .
  
   I agree with you. Using clk_prepare_enable() is more clear. Actually I
   had already commented on this. Please see the patch v2. But this way
   also looks good to me.
  
  
  Well, both versions are technically correct and will have the same effect
  for Exynos SoC clocks, since only enable/disable ops change hardware
  state.
  
  However if we look at general meaning of those generic ops, the clock will
  remain prepared for all the time the driver is loaded, even if the device
  
  
  
  Right, so I said previous one is more clear. I gonna revert current one 
and then merge previous one(v3)
  
  
   
  is runtime suspended. Again on Exynos SoCs this won't have any effect, but
  I think we should respect general Common Clock Framework semantics anyway.
  
  
  ctx-vidcon0 = pdata-vidcon0;
  ctx-vidcon1 = pdata-vidcon1;
  ctx-default_win = pdata-default_win;
 
  @@ -981,8 +991,8 @@ static int fimd_remove(struct platform_device
  *pdev)
 
  if (ctx-suspended)
 
  goto out;
 
  -   clk_disable(ctx-lcd_clk);
  -   clk_disable(ctx-bus_clk);
  +   clk_unprepare(ctx-lcd_clk);
  +   clk_unprepare(ctx-bus_clk);

 This looks wrong again.. You still need to call clk_disable() to
 make
 clk enabled
 count zero...
   
Viresh is right again here.
  
   Ok, you two guys say together this looks wrong so I'd like to take more
   checking. I thought that clk-clk_enable is 1 at here and it would be 0
   by pm_runtimg_put_sync(). Is there any my missing

Re: [PATCH v4] drm/exynos: prepare FIMD clocks

[Tomasz Figa]

On Monday 22 of April 2013 10:44:00 Viresh Kumar wrote:
 On 21 April 2013 20:13, Tomasz Figa tomasz.f...@gmail.com wrote:
  3) after those two changes, all that remains is to fix compliance with
  Common Clock Framework, in other words:
  
  s/clk_enable/clk_prepare_enable/
  
  and
  
  s/clk_disable/clk_disable_unprepare/
 
 We don't have to call  clk_{un}prepare() everytime for your platform as
 you aren't doing anything in it. So just call them once at probe/remove and
 call clk_enable/disable everywhere else.

Can you assure that in future SoCs, on which this driver will be used, this 
assumption will still hold true or even that in current Exynos driver this 
behavior won't be changed?

Best regards,
-- 
Tomasz Figa
Samsung Poland RD Center
SW Solution Development, Kernel and System Framework

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4] drm/exynos: prepare FIMD clocks

[Tomasz Figa]

On Monday 22 of April 2013 12:17:39 Sylwester Nawrocki wrote:
 On 04/22/2013 12:03 PM, Inki Dae wrote:
   Also looks good to me. But what if power domain was disabled without
   pm
   runtime? In this case, you must enable the power domain at machine
   code or
   bootloader somewhere. This way would not only need some hard codes
   to turn
   the power domain on but also not manage power management fully. This
   is same as only the use of pm runtime interface(needing some hard
   codes without pm runtime) so I don't prefer to add
   clk_enable/disable to fimd probe(). I quite tend to force only the
   use of pm runtime as possible. So please add the hard codes to
   machine code or bootloader like you did for power domain if you
   want to use drm fimd without pm runtime.
  
  That's not how the runtime PM, clock subsystems work:
  
  1) When CONFIG_PM_RUNTIME is disabled, all the used hardware must be
  kept
  powered on all the time.
  
  2) Common Clock Framework will always gate all clocks that have zero
  enable_count. Note that CCF support for Exynos is already merged for
  3.10 and it will be the only available clock support method for
  Exynos.
  
  AFAIK, drivers must work correctly in both cases, with
  CONFIG_PM_RUNTIME
  enabled and disabled.
  
  Then is the driver worked correctly if the power domain to this device was
  disabled at bootloader without CONFIG_PM_RUNTIME and with clk_enable()?  I
  think, in this case, the device wouldn't be worked correctly because the
  power of the device remains off. So you must enable the power domain
  somewhere. What is the difference between these two cases?
 
 How about making the driver dependant on PM_RUNTIME and making it always
 use pm_runtime_* API, regardless if the platform actually implements runtime
 PM or not ? Is there any issue in using the Runtime PM core always, rather
 than coding any workarounds in drivers when PM_RUNTIME is disabled ?

I don't think this is a good idea. This would mean that any user that from 
some reasons don't want to use PM_RUNTIME, would not be able to use the driver 
anymore.

Rafael, Kevin, do you have any opinion on this?

Best regards,
-- 
Tomasz Figa
Samsung Poland RD Center
SW Solution Development, Kernel and System Framework

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4] drm/exynos: prepare FIMD clocks

[Tomasz Figa]

On Monday 22 of April 2013 12:05:49 Sylwester Nawrocki wrote:
 On 04/22/2013 11:56 AM, Tomasz Figa wrote:
  On Monday 22 of April 2013 10:44:00 Viresh Kumar wrote:
  On 21 April 2013 20:13, Tomasz Figa tomasz.f...@gmail.com wrote:
  3) after those two changes, all that remains is to fix compliance with
  Common Clock Framework, in other words:
  
  s/clk_enable/clk_prepare_enable/
  
  and
  
  s/clk_disable/clk_disable_unprepare/
  
  We don't have to call  clk_{un}prepare() everytime for your platform as
  you aren't doing anything in it. So just call them once at probe/remove
  and
  call clk_enable/disable everywhere else.
 
 Yes, I agree with that. Additionally clk_(un)prepare must not be called in
 atomic context, so some drivers will have to work like this anyway.
 Or the clocks could be prepared/unprepared in the device open/close file op
 for instance.

Well, I don't think drivers should make any assumptions how particular clk ops 
are implemented on particular platform.

Instead, generic semantics of Common Clock Framework should be obeyed, which 
AFAIK are:
1) Each clock must be prepared before enabling.
2) clk_prepare() can not be called from atomic contexts.
3) clk_prepare_enable() can be used instead of clk_prepare() + clk_enable() 
when the driver does not need to enable the clock from atomic context.

Since the Exynos DRM FIMD driver does not need to do call any clock operations 
in atomic contexts, the approach keeping the clock handling as simple as 
possible would be to just replace all clk_{enable,disable} with 
clk_{prepare_enable,disable_unprepare}, as I suggested.

CCing Mike, the maintainer of Common Clock Framework, since he's the right 
person to pass any judgements when it is about clocks.

Best regards,
-- 
Tomasz Figa
Samsung Poland RD Center
SW Solution Development, Kernel and System Framework

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/exynos: fix multiple definition build error

[Tomasz Figa]

Hi Inki,

On Friday 26 of April 2013 14:03:10 Inki Dae wrote:
 This patch fixes multiple definition error like below when building it
 as moudle with device tree support.
 
 drivers/gpu/drm/exynos/exynos_drm_g2d.o: In function `.LANCHOR1':
 exynos_drm_g2d.c:(.rodata+0x6c): multiple definition of
 `__mod_of_device_table'
 drivers/gpu/drm/exynos/exynos_drm_fimd.o:exynos_drm_fimd.c:(.rodata+0x1
 44): first defined here
 
 Signed-off-by: Inki Dae inki@samsung.com
 Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
 ---
  drivers/gpu/drm/exynos/exynos_drm_fimd.c |2 +-
  drivers/gpu/drm/exynos/exynos_drm_g2d.c  |2 +-
  2 files changed, 2 insertions(+), 2 deletions(-)
 
 diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
 b/drivers/gpu/drm/exynos/exynos_drm_fimd.c index 746b282..1e02d13
 100644
 --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
 +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
 @@ -117,7 +117,7 @@ static const struct of_device_id
 fimd_driver_dt_match[] = { .data = exynos5_fimd_driver_data },
   {},
  };
 -MODULE_DEVICE_TABLE(of, fimd_driver_dt_match);
 +MODULE_DEVICE_TABLE(of_fimd, fimd_driver_dt_match);

I wonder if this change wouldn't break the purpose of having 
MODULE_DEVICE_TABLE at all.

As far as I remember, this is used to create a symbol with well known name 
that userspace tools can use to identify what devices are handled in this 
module. For example

MODULE_DEVICE_TABLE(of, fimd_driver_dt_match);

results in creation of __mod_of_device_table symbol, of which tools, such 
as depmod are aware and can build a list of supported devices.

Your change will result in creation of __mod_of_fimd_device_table, which 
is unknown and won't be of any use.

By the way, looking at the definition of MODULE_DEVICE_TABLE, which is

139 #define MODULE_DEVICE_TABLE(type,name)  \
140   MODULE_GENERIC_TABLE(type##_device,name)

and then MODULE_GENERIC_TABLE

 85 #ifdef MODULE
 86 #define MODULE_GENERIC_TABLE(gtype,name)\
 87 extern const struct gtype##_id __mod_##gtype##_table\
 88   __attribute__ ((unused, alias(__stringify(name
 89 
 90 #else  /* !MODULE */
 91 #define MODULE_GENERIC_TABLE(gtype,name)
 92 #endif

it seems like the exact line that will be generated after your change, 
will be

extern const struct of_fimd_device_id __mod_of_fimd_device_table
__attribute__ ((unused, alias(__stringify(name;

which seems wrong, because of_fimd_device_id is not a correct struct type.

Best regards,
Tomasz

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/exynos: fix multiple definition build error

[Tomasz Figa]

On Friday 26 of April 2013 11:48:50 Sylwester Nawrocki wrote:
 On 04/26/2013 10:20 AM, Inki Dae wrote:
  Exactly right. it's my mistake. But now it seems that
  __mode_of_device_table is multi defined at fimd and g2d side so there
  still is module build error. :(
 Since all drivers seem to be linked into single a single module, you
 likely need to create a separate table of struct of_device_id just for
 the purpose of MODULE_DEVICE_TABLE(of, ...). This table would contain
 'compatible' strings for all devices. Or choose of_device_id for just
 one device and define MODULE_DEVICE_TABLE() for it in some common place,
 e.g. exynos_drm_drv.c. I believe all devices should be listed though.

IMHO, the most proper solution would be to split the module into parent 
exynos_drm module and per-device submodules, which would depend on the 
parent module.

This way you would be able to load dynamically any submodule you want, 
without recompiling the modules.

Best regards,
Tomasz

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 0/4] drm/exynos: fimd: Add support for S3C6400/S3C6410

[Tomasz Figa]

Much of the code in Exynos DRM subsystem is generic enough to use
it for older (non-Exynos) Samsung SoCs as well, after minor modifications.

This series starts adding support for previous SoCs to Exynos DRM by
introducing S3C64xx support to exynos_drm_fimd driver.

Adding support for remaining SoCs between S3C64xx and Exynos4 (S5PC100,
S5P64x0 and S5PV210) should be rather straightforward, but at the moment
I can test only on S3C6410, so I limited my changes only to this SoC.

On Tiny6410 (Mini6410-compatible) board based on Samsung S3C6410 SoC,
using my to-be-resubmitted patches for Device Tree support:

Tested-by: Tomasz Figa tomasz.f...@gmail.com

Tomasz Figa (4):
  drm/exynos: fimd: Hold pointer to driver data in context struct
  drm/exynos: fimd: Add support for FIMD versions without SHADOWCON
register
  drm/exynos: fimd: Add support for FIMD variants with clock selection
  drm/exynos: fimd: Add support for S3C64xx SoCs

 drivers/gpu/drm/exynos/exynos_drm_fimd.c | 90 
 1 file changed, 68 insertions(+), 22 deletions(-)

-- 
1.8.2.1

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/4] drm/exynos: fimd: Hold pointer to driver data in context struct

[Tomasz Figa]

This patch adds pointer to driver data to fimd_context structure, to
remove the need to call drm_fimd_get_driver_data() each time access to
driver data is necessary.

Signed-off-by: Tomasz Figa tomasz.f...@gmail.com
---
 drivers/gpu/drm/exynos/exynos_drm_fimd.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c 
b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
index 746b282..264434f 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
@@ -107,6 +107,7 @@ struct fimd_context {
atomic_twait_vsync_event;
 
struct exynos_drm_panel_info *panel;
+   struct fimd_driver_data *driver_data;
 };
 
 #ifdef CONFIG_OF
@@ -239,10 +240,9 @@ static void fimd_commit(struct device *dev)
struct exynos_drm_panel_info *panel = ctx-panel;
struct fb_videomode *timing = panel-timing;
struct fimd_driver_data *driver_data;
-   struct platform_device *pdev = to_platform_device(dev);
u32 val;
 
-   driver_data = drm_fimd_get_driver_data(pdev);
+   driver_data = ctx-driver_data;
if (ctx-suspended)
return;
 
@@ -949,6 +949,7 @@ static int fimd_probe(struct platform_device *pdev)
return ret;
}
 
+   ctx-driver_data = drm_fimd_get_driver_data(pdev);
ctx-vidcon0 = pdata-vidcon0;
ctx-vidcon1 = pdata-vidcon1;
ctx-default_win = pdata-default_win;
-- 
1.8.2.1

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/4] drm/exynos: fimd: Add support for FIMD versions without SHADOWCON register

[Tomasz Figa]

Some platforms that can be supported with this driver have PRTCON
register instead of SHADOWCON, which requires slightly different
handling.

This patch factors out all register shadow control code from the driver
and adds a function to control register shadowing appropriately,
depending on driver data.

Signed-off-by: Tomasz Figa tomasz.f...@gmail.com
---
 drivers/gpu/drm/exynos/exynos_drm_fimd.c | 69 +++-
 1 file changed, 49 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c 
b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
index 264434f..a5559f6 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
@@ -63,14 +63,18 @@
 
 struct fimd_driver_data {
unsigned int timing_base;
+
+   unsigned int has_shadowcon:1;
 };
 
 static struct fimd_driver_data exynos4_fimd_driver_data = {
.timing_base = 0x0,
+   .has_shadowcon = 1,
 };
 
 static struct fimd_driver_data exynos5_fimd_driver_data = {
.timing_base = 0x2,
+   .has_shadowcon = 1,
 };
 
 struct fimd_win_data {
@@ -489,6 +493,33 @@ static void fimd_win_set_colkey(struct device *dev, 
unsigned int win)
writel(keycon1, ctx-regs + WKEYCON1_BASE(win));
 }
 
+/**
+ * shadow_protect_win() - disable updating values from shadow registers at 
vsync
+ *
+ * @win: window to protect registers for
+ * @protect: 1 to protect (disable updates)
+ */
+static void fimd_shadow_protect_win(struct fimd_context *ctx,
+   int win, bool protect)
+{
+   u32 reg, bits, val;
+
+   if (ctx-driver_data-has_shadowcon) {
+   reg = SHADOWCON;
+   bits = SHADOWCON_WINx_PROTECT(win);
+   } else {
+   reg = PRTCON;
+   bits = PRTCON_PROTECT;
+   }
+
+   val = readl(ctx-regs + reg);
+   if (protect)
+   val |= bits;
+   else
+   val = ~bits;
+   writel(val, ctx-regs + reg);
+}
+
 static void fimd_win_commit(struct device *dev, int zpos)
 {
struct fimd_context *ctx = get_fimd_context(dev);
@@ -512,7 +543,7 @@ static void fimd_win_commit(struct device *dev, int zpos)
win_data = ctx-win_data[win];
 
/*
-* SHADOWCON register is used for enabling timing.
+* SHADOWCON/PRTCON register is used for enabling timing.
 *
 * for example, once only width value of a register is set,
 * if the dma is started then fimd hardware could malfunction so
@@ -522,9 +553,7 @@ static void fimd_win_commit(struct device *dev, int zpos)
 */
 
/* protect windows */
-   val = readl(ctx-regs + SHADOWCON);
-   val |= SHADOWCON_WINx_PROTECT(win);
-   writel(val, ctx-regs + SHADOWCON);
+   fimd_shadow_protect_win(ctx, win, true);
 
/* buffer start address */
val = (unsigned long)win_data-dma_addr;
@@ -602,10 +631,13 @@ static void fimd_win_commit(struct device *dev, int zpos)
writel(val, ctx-regs + WINCON(win));
 
/* Enable DMA channel and unprotect windows */
-   val = readl(ctx-regs + SHADOWCON);
-   val |= SHADOWCON_CHx_ENABLE(win);
-   val = ~SHADOWCON_WINx_PROTECT(win);
-   writel(val, ctx-regs + SHADOWCON);
+   fimd_shadow_protect_win(ctx, win, false);
+
+   if (ctx-driver_data-has_shadowcon) {
+   val = readl(ctx-regs + SHADOWCON);
+   val |= SHADOWCON_CHx_ENABLE(win);
+   writel(val, ctx-regs + SHADOWCON);
+   }
 
win_data-enabled = true;
 }
@@ -634,9 +666,7 @@ static void fimd_win_disable(struct device *dev, int zpos)
}
 
/* protect windows */
-   val = readl(ctx-regs + SHADOWCON);
-   val |= SHADOWCON_WINx_PROTECT(win);
-   writel(val, ctx-regs + SHADOWCON);
+   fimd_shadow_protect_win(ctx, win, true);
 
/* wincon */
val = readl(ctx-regs + WINCON(win));
@@ -644,10 +674,13 @@ static void fimd_win_disable(struct device *dev, int zpos)
writel(val, ctx-regs + WINCON(win));
 
/* unprotect windows */
-   val = readl(ctx-regs + SHADOWCON);
-   val = ~SHADOWCON_CHx_ENABLE(win);
-   val = ~SHADOWCON_WINx_PROTECT(win);
-   writel(val, ctx-regs + SHADOWCON);
+   if (ctx-driver_data-has_shadowcon) {
+   val = readl(ctx-regs + SHADOWCON);
+   val = ~SHADOWCON_CHx_ENABLE(win);
+   writel(val, ctx-regs + SHADOWCON);
+   }
+
+   fimd_shadow_protect_win(ctx, win, false);
 
win_data-enabled = false;
 }
@@ -777,8 +810,6 @@ static int fimd_calc_clkdiv(struct fimd_context *ctx,
 
 static void fimd_clear_win(struct fimd_context *ctx, int win)
 {
-   u32 val;
-
DRM_DEBUG_KMS(%s\n, __FILE__);
 
writel(0, ctx-regs + WINCON(win));
@@ -789,9 +820,7 @@ static void fimd_clear_win(struct fimd_context *ctx, int 
win)
if (win == 1 || win == 2)
writel(0, ctx-regs + VIDOSD_D

[PATCH 3/4] drm/exynos: fimd: Add support for FIMD variants with clock selection

[Tomasz Figa]

Some platforms that can be supported this driver has additional clock
source selection bits in VIDCON0 register that allows to select which
clock should be used to drive the pixel clock: bus clock or special
clock.

Since this driver assumes that special clock always drives the pixel
clock, this patch sets the selection bitfield to use the special clock.

Signed-off-by: Tomasz Figa tomasz.f...@gmail.com
---
 drivers/gpu/drm/exynos/exynos_drm_fimd.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c 
b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
index a5559f6..a2e385d 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
@@ -65,6 +65,7 @@ struct fimd_driver_data {
unsigned int timing_base;
 
unsigned int has_shadowcon:1;
+   unsigned int has_clksel:1;
 };
 
 static struct fimd_driver_data exynos4_fimd_driver_data = {
@@ -278,6 +279,11 @@ static void fimd_commit(struct device *dev)
val = ctx-vidcon0;
val = ~(VIDCON0_CLKVAL_F_MASK | VIDCON0_CLKDIR);
 
+   if (ctx-driver_data-has_clksel) {
+   val = ~VIDCON0_CLKSEL_MASK;
+   val |= VIDCON0_CLKSEL_LCD;
+   }
+
if (ctx-clkdiv  1)
val |= VIDCON0_CLKVAL_F(ctx-clkdiv - 1) | VIDCON0_CLKDIR;
else
-- 
1.8.2.1

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 4/4] drm/exynos: fimd: Add support for S3C64xx SoCs

[Tomasz Figa]

The FIMD block present on S3C6400/S3C6410 SoCs is compatible with this
driver, so it can be supported by it as well.

This patch adds appropriate device IDs and driver data to enable this
driver for S3C64xx SoCs.

Signed-off-by: Tomasz Figa tomasz.f...@gmail.com
---
 drivers/gpu/drm/exynos/exynos_drm_fimd.c | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c 
b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
index a2e385d..a1669d4 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
@@ -68,6 +68,11 @@ struct fimd_driver_data {
unsigned int has_clksel:1;
 };
 
+static struct fimd_driver_data s3c64xx_fimd_driver_data = {
+   .timing_base = 0x0,
+   .has_clksel = 1,
+};
+
 static struct fimd_driver_data exynos4_fimd_driver_data = {
.timing_base = 0x0,
.has_shadowcon = 1,
@@ -117,6 +122,8 @@ struct fimd_context {
 
 #ifdef CONFIG_OF
 static const struct of_device_id fimd_driver_dt_match[] = {
+   { .compatible = samsung,s3c6400-fimd,
+ .data = s3c64xx_fimd_driver_data },
{ .compatible = samsung,exynos4210-fimd,
  .data = exynos4_fimd_driver_data },
{ .compatible = samsung,exynos5250-fimd,
@@ -1108,6 +1115,9 @@ static int fimd_runtime_resume(struct device *dev)
 
 static struct platform_device_id fimd_driver_ids[] = {
{
+   .name   = s3c64xx-fb,
+   .driver_data= (unsigned long)s3c64xx_fimd_driver_data,
+   }, {
.name   = exynos4-fb,
.driver_data= (unsigned long)exynos4_fimd_driver_data,
}, {
-- 
1.8.2.1

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/exynos: fimd: Get signal polarities from device tree

[Tomasz Figa]

This patch modifies the driver to perform two stage parsing of video
timings from device tree, to get timing information as struct videomode,
which contains more data than struct fb_videomode.

Thanks to this change, information about polarity of control signals
(VSYNC, HSYNC, VDEN, VCLK) can be retrieved, in addition to standard
video timings.

Signed-off-by: Tomasz Figa tomasz.f...@gmail.com
---
 drivers/gpu/drm/exynos/exynos_drm_fimd.c | 18 --
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c 
b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
index a1669d4..9023efa 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
@@ -21,7 +21,9 @@
 #include linux/pm_runtime.h
 
 #include video/of_display_timing.h
+#include video/of_videomode.h
 #include video/samsung_fimd.h
+#include video/videomode.h
 #include drm/exynos_drm.h
 
 #include exynos_drm_drv.h
@@ -928,18 +930,30 @@ static int fimd_probe(struct platform_device *pdev)
DRM_DEBUG_KMS(%s\n, __FILE__);
 
if (pdev-dev.of_node) {
+   struct videomode vm;
+
pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
if (!pdata) {
DRM_ERROR(memory allocation for pdata failed\n);
return -ENOMEM;
}
 
-   ret = of_get_fb_videomode(dev-of_node, pdata-panel.timing,
-   OF_USE_NATIVE_MODE);
+   ret = of_get_videomode(dev-of_node, vm, OF_USE_NATIVE_MODE);
if (ret) {
DRM_ERROR(failed: of_get_fb_videomode() : %d\n, ret);
return ret;
}
+
+   fb_videomode_from_videomode(vm, pdata-panel.timing);
+
+   if (vm.flags  DISPLAY_FLAGS_VSYNC_LOW)
+   pdata-vidcon1 |= VIDCON1_INV_VSYNC;
+   if (vm.flags  DISPLAY_FLAGS_HSYNC_LOW)
+   pdata-vidcon1 |= VIDCON1_INV_HSYNC;
+   if (vm.flags  DISPLAY_FLAGS_DE_LOW)
+   pdata-vidcon1 |= VIDCON1_INV_VDEN;
+   if (vm.flags  DISPLAY_FLAGS_PIXDATA_NEGEDGE)
+   pdata-vidcon1 |= VIDCON1_INV_VCLK;
} else {
pdata = pdev-dev.platform_data;
if (!pdata) {
-- 
1.8.2.1

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

DMA mapping API abuse in exynos-drm

[Tomasz Figa]

Hi,

Recently I've been working a bit on a DRM driver for the GPU of Samsung 
S3C6410 SoCs, which required me to familiarize a bit with exynos-drm, as 
it already contains a KMS driver which is compatible with the SoC I'm 
working with, making it a good place to put my driver in.

Reading through exynos_drm_buf.c I stumbled across following (a bit long) 
piece of code:

dma_set_attr(DMA_ATTR_NO_KERNEL_MAPPING, buf-dma_attrs);

nr_pages = buf-size  PAGE_SHIFT;

if (!is_drm_iommu_supported(dev)) {
dma_addr_t start_addr;
unsigned int i = 0;

buf-pages = kzalloc(sizeof(struct page) * nr_pages,
GFP_KERNEL);
if (!buf-pages) {
DRM_ERROR(failed to allocate pages.\n);
return -ENOMEM;
}

buf-kvaddr = dma_alloc_attrs(dev-dev, buf-size,
buf-dma_addr, GFP_KERNEL,
buf-dma_attrs);
if (!buf-kvaddr) {
DRM_ERROR(failed to allocate buffer.\n);
kfree(buf-pages);
return -ENOMEM;
}

start_addr = buf-dma_addr;
while (i  nr_pages) {
buf-pages[i] = phys_to_page(start_addr);
start_addr += PAGE_SIZE;
i++;
}
} else {

buf-pages = dma_alloc_attrs(dev-dev, buf-size,
buf-dma_addr, GFP_KERNEL,
buf-dma_attrs);
if (!buf-pages) {
DRM_ERROR(failed to allocate buffer.\n);
return -ENOMEM;
}
}

buf-sgt = drm_prime_pages_to_sg(buf-pages, nr_pages);
if (!buf-sgt) {
DRM_ERROR(failed to get sg table.\n);
ret = -ENOMEM;
goto err_free_attrs;
}

Notice that the value returned by dma_alloc_attrs() is assumed by this 
code to be either a kernel virtual address (in !is_drm_iommu_supported() 
case) or struct pages ** (in is_drm_iommu_supported() case), which seemed 
a bit worrying to me, making me do a more in depth research on how 
dma_alloc_attrs works.

This, in turn, led me to following excerpt of Documentation/DMA-
attributes.txt :

DMA_ATTR_NO_KERNEL_MAPPING
--

DMA_ATTR_NO_KERNEL_MAPPING lets the platform to avoid creating a kernel
virtual mapping for the allocated buffer. On some architectures creating
such mapping is non-trivial task and consumes very limited resources
(like kernel virtual address space or dma consistent address space).
Buffers allocated with this attribute can be only passed to user space
by calling dma_mmap_attrs(). By using this API, you are guaranteeing
that you won't dereference the pointer returned by dma_alloc_attr(). You
can threat it as a cookie that must be passed to dma_mmap_attrs() and
dma_free_attrs(). Make sure that both of these also get this attribute
set on each call.

of which the following sentence is the most relevant:

By using this API, you are guaranteeing that you won't dereference the 
pointer returned by dma_alloc_attr().

This statement is obviously ignored by buffer allocation code of exynos-
drm.

A simple git blame revealed that this brokenness has been introduced by 
commit:

4744ad2 drm/exynos: use DMA_ATTR_NO_KERNEL_MAPPING attribute


and later fixed a bit by following depending patches (because the original 
patch apparently broke any allocations without IOMMU):

c704f1b drm/exynos: consider no iommu support to console framebuffer
694be45 drm/exynos: consider buffer allocation without iommu


Now, the real question is whether my reasoning is incorrect (sorry for the 
noise then) or this is really a bug which needs to be fixed?

Best regards,
Tomasz

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Introduce a new helper framework for buffer synchronization

[Tomasz Figa]

Hi,

On Monday 13 of May 2013 20:24:01 Inki Dae wrote:
  -Original Message-
  From: Maarten Lankhorst [mailto:maarten.lankho...@canonical.com]
  Sent: Monday, May 13, 2013 6:52 PM
  To: Inki Dae
  Cc: 'Rob Clark'; 'Daniel Vetter'; 'DRI mailing list'; linux-arm-
  ker...@lists.infradead.org; linux-me...@vger.kernel.org;
  'linux-fbdev';
  'Kyungmin Park'; 'myungjoo.ham'; 'YoungJun Cho'
  Subject: Re: Introduce a new helper framework for buffer
  synchronization 
  Op 13-05-13 11:21, Inki Dae schreef:
   -Original Message-
   From: Maarten Lankhorst [mailto:maarten.lankho...@canonical.com]
   Sent: Monday, May 13, 2013 5:01 PM
   To: Inki Dae
   Cc: Rob Clark; Daniel Vetter; DRI mailing list; linux-arm-
   ker...@lists.infradead.org; linux-me...@vger.kernel.org;
   linux-fbdev;
   Kyungmin Park; myungjoo.ham; YoungJun Cho
   Subject: Re: Introduce a new helper framework for buffer
  
  synchronization
  
   Op 09-05-13 09:33, Inki Dae schreef:
   Hi all,
   
   This post introduces a new helper framework based on dma fence.
   And
  
  the
  
   purpose of this post is to collect other opinions and advices
   before
  
  RFC
  
   posting.
   
   First of all, this helper framework, called fence helper, is in
  
  progress
  
   yet so might not have enough comments in codes and also might need
   to
  
  be
  
   more cleaned up. Moreover, we might be missing some parts of the
   dma
   
   fence.
   
   However, I'd like to say that all things mentioned below has been
  
  tested
  
   with Linux platform and worked well.
   
   
   And tutorial for user process.
   
   just before cpu access
   
   struct dma_buf_fence *df;
   
   df-type = DMA_BUF_ACCESS_READ or
 
 DMA_BUF_ACCESS_WRITE;
 
   ioctl(fd, DMA_BUF_GET_FENCE, df);
   
   after memset or memcpy
   
   ioctl(fd, DMA_BUF_PUT_FENCE, df);
   
   NAK.
   
   Userspace doesn't need to trigger fences. It can do a buffer idle
   wait,
   and postpone submitting new commands until after it's done using
   the
   buffer.
   
   Hi Maarten,
   
   It seems that you say user should wait for a buffer like KDS does:
   KDS
  
  uses
  
   select() to postpone submitting new commands. But I think this way
  
  assumes
  
   that every data flows a DMA device to a CPU. For example, a CPU
   should
  
  keep
  
   polling for the completion of a buffer access by a DMA device. This
  
  means
  
   that the this way isn't considered for data flow to opposite case;
   CPU
  
  to
  
   DMA device.
  
  Not really. You do both things the same way. You first wait for the bo
  to be idle, this could be implemented by adding poll support to the
  dma-buf fd.
  Then you either do your read or write. Since userspace is supposed to
  be the one controlling the bo it should stay idle at that point. If
  you have another thread queueing
  the buffer againbefore your thread is done that's a bug in the
 
 application,
 
  and can be solved with userspace locking primitives. No need for the
  kernel to get involved.
 
 Yes, that is how we have synchronized buffer between CPU and DMA device
 until now without buffer synchronization mechanism. I thought that it's
 best to make user not considering anything: user can access a buffer
 regardless of any DMA device controlling and the buffer synchronization
 is performed in kernel level. Moreover, I think we could optimize
 graphics and multimedia hardware performance because hardware can do
 more works: one thread accesses a shared buffer and the other controls
 DMA device with the shared buffer in parallel.

Could you explain this point? I thought that if there is a shared buffer 
accessible for user and DMA device, only one of them can use it at the 
moment, i.e. the buffer is useless for the reading user (or read DMA) 
until (write) DMA (or writing user) finishes writing for it. Is it 
incorrect? Or this is not the point here?

I'm not an expert here and I'm just trying to understand the idea, so 
correct me if I'm wrong. Thanks in advance.

Best regards,
Tomasz

 Thus, we could avoid
 sequential processing and that is my intention. Aren't you think about
 that we could improve hardware utilization with such way or other? of
 course, there could be better way.
 
   Kernel space doesn't need the root hole you created by giving a
   dereferencing a pointer passed from userspace.
   Your next exercise should be to write a security exploit from the
   api
  
  you
  
   created here. It's the only way to learn how to write safe code.
   Hint:
   df.ctx = mmap(..);
   
   Also I'm not clear to use our way yet and that is why I posted. As
   you
   mentioned, it seems like that using mmap() is more safe. But there
   is
  
  one
  
   issue it makes me confusing. For your hint, df.ctx = mmap(..), the
   issue 
  is
  
   that dmabuf mmap can be used to map a dmabuf with user space. And
   the
  

Re: [PATCH 0/4] drm/exynos: fimd: Add support for S3C6400/S3C6410

[Tomasz Figa]

Hi,

On Wednesday 01 of May 2013 21:02:25 Tomasz Figa wrote:
 Much of the code in Exynos DRM subsystem is generic enough to use
 it for older (non-Exynos) Samsung SoCs as well, after minor
 modifications.
 
 This series starts adding support for previous SoCs to Exynos DRM by
 introducing S3C64xx support to exynos_drm_fimd driver.
 
 Adding support for remaining SoCs between S3C64xx and Exynos4 (S5PC100,
 S5P64x0 and S5PV210) should be rather straightforward, but at the moment
 I can test only on S3C6410, so I limited my changes only to this SoC.
 
 On Tiny6410 (Mini6410-compatible) board based on Samsung S3C6410 SoC,
 using my to-be-resubmitted patches for Device Tree support:
 
 Tested-by: Tomasz Figa tomasz.f...@gmail.com
 
 Tomasz Figa (4):
   drm/exynos: fimd: Hold pointer to driver data in context struct
   drm/exynos: fimd: Add support for FIMD versions without SHADOWCON
 register
   drm/exynos: fimd: Add support for FIMD variants with clock selection
   drm/exynos: fimd: Add support for S3C64xx SoCs
 
  drivers/gpu/drm/exynos/exynos_drm_fimd.c | 90
  1 file changed, 68 insertions(+), 22
 deletions(-)

Any comments for this series?

Best regards,
Tomasz

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/exynos: fimd: Get signal polarities from device tree

[Tomasz Figa]

Hi,

On Wednesday 01 of May 2013 22:00:25 Daniel Vetter wrote:
 On Wed, May 01, 2013 at 09:06:09PM +0200, Tomasz Figa wrote:
  This patch modifies the driver to perform two stage parsing of video
  timings from device tree, to get timing information as struct
  videomode, which contains more data than struct fb_videomode.
  
  Thanks to this change, information about polarity of control signals
  (VSYNC, HSYNC, VDEN, VCLK) can be retrieved, in addition to standard
  video timings.
  
  Signed-off-by: Tomasz Figa tomasz.f...@gmail.com
 
 Since the drm mode struct also contains flags for sync polarity ... why
 is there no direct of - drm_mode function? Going through an fb
 videomode in a kms drm driver looks _really_ backwards to me.
 
 Cc'in Dave for the fun of it ;-)

Struct fb_videomode is what exynos_drm_fimd driver uses internally. Sure 
it should use drm_mode, but this is not really related to this patch, 
because the code added in this patch only fills in the pdata struct, which 
for compatibility reasons (the same structure is used for both fbdev and 
drm drivers) contains struct fb_videomode.

OK, now after having a bit of fun, could we merge this patch to at least 
have usable support of parallel displays using this driver?

Best regards,
Tomasz

 Cheers, Daniel
 
  ---
  
   drivers/gpu/drm/exynos/exynos_drm_fimd.c | 18 --
   1 file changed, 16 insertions(+), 2 deletions(-)
  
  diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
  b/drivers/gpu/drm/exynos/exynos_drm_fimd.c index a1669d4..9023efa
  100644
  --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
  +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
  @@ -21,7 +21,9 @@
  
   #include linux/pm_runtime.h
   
   #include video/of_display_timing.h
  
  +#include video/of_videomode.h
  
   #include video/samsung_fimd.h
  
  +#include video/videomode.h
  
   #include drm/exynos_drm.h
   
   #include exynos_drm_drv.h
  
  @@ -928,18 +930,30 @@ static int fimd_probe(struct platform_device
  *pdev) 
  DRM_DEBUG_KMS(%s\n, __FILE__);
  
  if (pdev-dev.of_node) {
  
  +   struct videomode vm;
  +
  
  pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
  if (!pdata) {
  
  DRM_ERROR(memory allocation for pdata failed\n);
  return -ENOMEM;
  
  }
  
  -   ret = of_get_fb_videomode(dev-of_node, pdata-
panel.timing,
  -   OF_USE_NATIVE_MODE);
  +   ret = of_get_videomode(dev-of_node, vm, 
OF_USE_NATIVE_MODE);
  
  if (ret) {
  
  DRM_ERROR(failed: of_get_fb_videomode() : %d\n, 
ret);
  return ret;
  
  }
  
  +
  +   fb_videomode_from_videomode(vm, pdata-panel.timing);
  +
  +   if (vm.flags  DISPLAY_FLAGS_VSYNC_LOW)
  +   pdata-vidcon1 |= VIDCON1_INV_VSYNC;
  +   if (vm.flags  DISPLAY_FLAGS_HSYNC_LOW)
  +   pdata-vidcon1 |= VIDCON1_INV_HSYNC;
  +   if (vm.flags  DISPLAY_FLAGS_DE_LOW)
  +   pdata-vidcon1 |= VIDCON1_INV_VDEN;
  +   if (vm.flags  DISPLAY_FLAGS_PIXDATA_NEGEDGE)
  +   pdata-vidcon1 |= VIDCON1_INV_VCLK;
  
  } else {
  
  pdata = pdev-dev.platform_data;
  if (!pdata) {
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/exynos: wait for the completion of pending page flip

[Tomasz Figa]

Hi Inki,

On Wednesday 22 of May 2013 15:37:14 Inki Dae wrote:
 2013/5/22 Stéphane Marchesin stephane.marche...@gmail.com
 
  On Tue, May 21, 2013 at 9:22 PM, Inki Dae inki@samsung.com 
wrote:
   2013/5/22 Stéphane Marchesin stephane.marche...@gmail.com
   
   On Tue, May 21, 2013 at 1:08 AM, Inki Dae inki@samsung.com 
wrote:
This patch fixes the issue that drm_vblank_get() is failed.

The issus occurs when next page flip request is tried
if previous page flip event wasn't completed yet and then
dpms became off.

So this patch make sure that page flip event is completed
before dpms goes to off.
   
   Hi,
   
   This patch is a squash of the two following patches from the Chrome
   OS
  
   tree with the KDS bits removed and the dpms off bit added:
  http://git.chromium.org/gitweb/?p=chromiumos/third_party/kernel-next.g
  it;a=commitdiff;h=2e77cd4e423967862ca01b1af82aa8b5b7429fc4;hp=aba002da
  4c6e5efec4d43e1ce33930a79269349a
  
  
  http://git.chromium.org/gitweb/?p=chromiumos/third_party/kernel-next.g
  it;a=commitdiff;h=b4ec8bfa750ef43a43c2da746c8afdbb51002882;hp=4f28b9a7
  5c928f229443d7c6c3163159ceb6903a 
   Please keep proper attribution.
   
   Those patches are just for Chrome OS. Please post them if you want
   for
  
  those
  
   to be considered so that they can be reviewed.
  
  We intend to, once they are rebased onto latest kernel. But what I'm
  pointing out is that you're removing proper attribution from Chrome OS
  patches, and this is the second time it has happened.
 
 What is mean? does mainline kernel have the attribution? if not so, we
 don't need to consider it. And please know that I can not be aware of
 you have such patch set without any posting.
 

at·tri·bu·tion 
n.
1. The act of attributing, especially the act of establishing a particular 
person as the creator of a work of art
[1]

So yes, mainline kernel has attribution. Actually posting something as 
work of someone that is not the author of the posted work is considered 
bad everywhere, isn't it?

However looking at those two patches linked by Stéphane, I'm not really 
sure this patch is really just a squash of them. Stéphane, are you 100% 
sure that your claims are true?

Best regards,
Tomasz

[1] http://www.thefreedictionary.com/attribution

   That is why we attend open
   source. One more comment, please do not abuse
   exynos_drm_crtc_page_flip() 
  What do you mean by abuse?
  
   Signed-off-by: Inki Dae inki@samsung.com
   Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
   ---
   
drivers/gpu/drm/exynos/exynos_drm_crtc.c |   16 
1 files changed, 16 insertions(+), 0 deletions(-)
   
   diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
   b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
   index e8894bc..69a77e9 100644
   --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
   +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
   @@ -48,6 +48,8 @@ struct exynos_drm_crtc {
   
   unsigned intpipe;
   unsigned intdpms;
   enum exynos_crtc_mode   mode;
   
   +   wait_queue_head_t   pending_flip_queue;
   +   atomic_tpending_flip;
   
};

static void exynos_drm_crtc_dpms(struct drm_crtc *crtc, int mode)
   
   @@ -61,6 +63,13 @@ static void exynos_drm_crtc_dpms(struct drm_crtc
  
  *crtc,
  
   int mode)
   
   return;
   
   }
   
   +   if (mode  DRM_MODE_DPMS_ON) {
   +   /* wait for the completion of page flip. */
   +   wait_event(exynos_crtc-pending_flip_queue,
   +  
   atomic_read(exynos_crtc-pending_flip)
  
  ==
  
   0);
   +   drm_vblank_off(crtc-dev, exynos_crtc-pipe);
   
   You should be using vblank_put/get.
   
   No, drm_vblank_put should be called by
   exynos_drm_crtc_finish_pageflip(). And know that this patch makes
   sure that pended page flip event is 
  completed
  
   before dpms goes to off.
  
  You need to do vblank_put/get while you're waiting. Otherwise you
  don't know for sure that flips will happen. This is especially bad as
  you don't use a timeout.
 
 Understood. Right, drm_vblank_get call before wait_event and
 drm_vblank_put call after wait_event.
 
 Thanks,
 Inki Dae
 
  Stéphane
  
   Thanks,
   Inki Dae
   
   Stéphane
   
+   }
+

exynos_drm_fn_encoder(crtc, mode,

exynos_drm_encoder_crtc_dpms);

exynos_crtc-dpms = mode;
 
 }

@@ -225,6 +234,7 @@ static int exynos_drm_crtc_page_flip(struct
  
  drm_crtc
  
*crtc,

spin_lock_irq(dev-event_lock);
list_add_tail(event-base.link,

dev_priv-pageflip_event_list);

+   atomic_set(exynos_crtc-pending_flip, 1);

Re: [PATCH] drm/exynos: fimd: Get signal polarities from device tree

[Tomasz Figa]

On Wednesday 01 of May 2013 21:06:09 Tomasz Figa wrote:
 This patch modifies the driver to perform two stage parsing of video
 timings from device tree, to get timing information as struct videomode,
 which contains more data than struct fb_videomode.
 
 Thanks to this change, information about polarity of control signals
 (VSYNC, HSYNC, VDEN, VCLK) can be retrieved, in addition to standard
 video timings.
 
 Signed-off-by: Tomasz Figa tomasz.f...@gmail.com
 ---
  drivers/gpu/drm/exynos/exynos_drm_fimd.c | 18 --
  1 file changed, 16 insertions(+), 2 deletions(-)
 
 diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
 b/drivers/gpu/drm/exynos/exynos_drm_fimd.c index a1669d4..9023efa
 100644
 --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
 +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
 @@ -21,7 +21,9 @@
  #include linux/pm_runtime.h
 
  #include video/of_display_timing.h
 +#include video/of_videomode.h
  #include video/samsung_fimd.h
 +#include video/videomode.h
  #include drm/exynos_drm.h
 
  #include exynos_drm_drv.h
 @@ -928,18 +930,30 @@ static int fimd_probe(struct platform_device
 *pdev) DRM_DEBUG_KMS(%s\n, __FILE__);
 
   if (pdev-dev.of_node) {
 + struct videomode vm;
 +
   pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
   if (!pdata) {
   DRM_ERROR(memory allocation for pdata failed\n);
   return -ENOMEM;
   }
 
 - ret = of_get_fb_videomode(dev-of_node, pdata-
panel.timing,
 - OF_USE_NATIVE_MODE);
 + ret = of_get_videomode(dev-of_node, vm, 
OF_USE_NATIVE_MODE);
   if (ret) {
   DRM_ERROR(failed: of_get_fb_videomode() : %d\n, 
ret);
   return ret;
   }
 +
 + fb_videomode_from_videomode(vm, pdata-panel.timing);
 +
 + if (vm.flags  DISPLAY_FLAGS_VSYNC_LOW)
 + pdata-vidcon1 |= VIDCON1_INV_VSYNC;
 + if (vm.flags  DISPLAY_FLAGS_HSYNC_LOW)
 + pdata-vidcon1 |= VIDCON1_INV_HSYNC;
 + if (vm.flags  DISPLAY_FLAGS_DE_LOW)
 + pdata-vidcon1 |= VIDCON1_INV_VDEN;
 + if (vm.flags  DISPLAY_FLAGS_PIXDATA_NEGEDGE)
 + pdata-vidcon1 |= VIDCON1_INV_VCLK;
   } else {
   pdata = pdev-dev.platform_data;
   if (!pdata) {

Ping.

Best regards,
Tomasz

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/4] drm/exynos: fimd: Add support for S3C6400/S3C6410

[Tomasz Figa]

On Sunday 19 of May 2013 13:26:57 Tomasz Figa wrote:
 Hi,
 
 On Wednesday 01 of May 2013 21:02:25 Tomasz Figa wrote:
  Much of the code in Exynos DRM subsystem is generic enough to use
  it for older (non-Exynos) Samsung SoCs as well, after minor
  modifications.
  
  This series starts adding support for previous SoCs to Exynos DRM by
  introducing S3C64xx support to exynos_drm_fimd driver.
  
  Adding support for remaining SoCs between S3C64xx and Exynos4
  (S5PC100,
  S5P64x0 and S5PV210) should be rather straightforward, but at the
  moment I can test only on S3C6410, so I limited my changes only to
  this SoC.
  
  On Tiny6410 (Mini6410-compatible) board based on Samsung S3C6410 SoC,
  using my to-be-resubmitted patches for Device Tree support:
  
  Tested-by: Tomasz Figa tomasz.f...@gmail.com
  
  Tomasz Figa (4):
drm/exynos: fimd: Hold pointer to driver data in context struct
drm/exynos: fimd: Add support for FIMD versions without SHADOWCON

  register

drm/exynos: fimd: Add support for FIMD variants with clock selection
drm/exynos: fimd: Add support for S3C64xx SoCs
   
   drivers/gpu/drm/exynos/exynos_drm_fimd.c | 90
  
   1 file changed, 68 insertions(+), 22
  deletions(-)
 
 Any comments for this series?
 
 Best regards,
 Tomasz

Ping.

Best regards,
Tomasz

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/exynos: fimd: Get signal polarities from device tree

[Tomasz Figa]

Hi Joonyoung,

On Thursday 06 of June 2013 13:30:49 Joonyoung Shim wrote:
 On 05/19/2013 08:32 PM, Tomasz Figa wrote:
  Hi,
  
  On Wednesday 01 of May 2013 22:00:25 Daniel Vetter wrote:
  On Wed, May 01, 2013 at 09:06:09PM +0200, Tomasz Figa wrote:
  This patch modifies the driver to perform two stage parsing of video
  timings from device tree, to get timing information as struct
  videomode, which contains more data than struct fb_videomode.
  
  Thanks to this change, information about polarity of control signals
  (VSYNC, HSYNC, VDEN, VCLK) can be retrieved, in addition to standard
  video timings.
  
  Signed-off-by: Tomasz Figa tomasz.f...@gmail.com
  
  Since the drm mode struct also contains flags for sync polarity ...
  why
  is there no direct of - drm_mode function? Going through an fb
  videomode in a kms drm driver looks _really_ backwards to me.
  
  Cc'in Dave for the fun of it ;-)
  
  Struct fb_videomode is what exynos_drm_fimd driver uses internally.
  Sure it should use drm_mode, but this is not really related to this
  patch, because the code added in this patch only fills in the pdata
  struct, which for compatibility reasons (the same structure is used
  for both fbdev and drm drivers) contains struct fb_videomode.
  
  OK, now after having a bit of fun, could we merge this patch to at
  least have usable support of parallel displays using this driver?
 
 I think it's better to use struct display_timings instead of struct
 fb_videomode in exynos_drm.

I agree that fb_videomode struct is a bit unfortunate here, but it seems 
to be widely used in the exynos_drm_fimd driver.

Actually, if I understood it properly, the correct struct to use in DRM 
drivers is drm_display_mode, but there is no conversion function from 
struct display_timing to it.

IMHO a separate patch introducing such conversion and then another one 
which modifies the driver to use drm_display_mode everywhere would be the 
best solution.

Best regards,
Tomasz

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/4] drm/exynos: rename compatible strings for hdmi subsystem

[Tomasz Figa]

Hi Rahul,

On Tuesday 18 of June 2013 18:19:35 Rahul Sharma wrote:
 This patch renames the combatible strings for hdmi, mixer, ddc
 and hdmiphy. It follows the convention of using compatible string
 which represent the SoC in which the IP was added for the first
 time.
 
 Signed-off-by: Rahul Sharma rahul.sha...@samsung.com
 ---
  Documentation/devicetree/bindings/video/exynos_hdmi.txt|6
 -- Documentation/devicetree/bindings/video/exynos_hdmiddc.txt |   
 4 ++-- Documentation/devicetree/bindings/video/exynos_hdmiphy.txt |   
 6 -- Documentation/devicetree/bindings/video/exynos_mixer.txt   |  
  7 +-- drivers/gpu/drm/exynos/exynos_ddc.c|
2 +- drivers/gpu/drm/exynos/exynos_hdmi.c   |   
 2 +- drivers/gpu/drm/exynos/exynos_hdmiphy.c|4
 +++- drivers/gpu/drm/exynos/exynos_mixer.c  |   12
 ++-- 8 files changed, 26 insertions(+), 17 deletions(-)
 
 diff --git a/Documentation/devicetree/bindings/video/exynos_hdmi.txt
 b/Documentation/devicetree/bindings/video/exynos_hdmi.txt index
 589edee..2ac01ca 100644
 --- a/Documentation/devicetree/bindings/video/exynos_hdmi.txt
 +++ b/Documentation/devicetree/bindings/video/exynos_hdmi.txt
 @@ -1,7 +1,9 @@
  Device-Tree bindings for drm hdmi driver
 
  Required properties:
 -- compatible: value should be samsung,exynos5-hdmi.
 +- compatible: value should be one among the following:
 + 1) samsung,exynos4210-hdmi
 + 2) samsung,exynos4212-hdmi
  - reg: physical base address of the hdmi and length of memory mapped
   region.
  - interrupts: interrupt number to the cpu.
 @@ -15,7 +17,7 @@ Required properties:
  Example:
 
   hdmi {
 - compatible = samsung,exynos5-hdmi;
 + compatible = samsung,exynos4212-hdmi;

Sorry, but it's a NAK from me.

DeviceTree bindings are considered an ABI. This is to allow older dtbs to 
work with new kernels.

If you just change the binding this way, you break all the existing users 
of this compatible value.

In addition you are doing it in a way that breaks bisection:
 - patch 1/4 breaks existing in-tree users of current compatible values,
 - after patch 2 and 3 it is still broken,
 - and eventually all in-tree users are fixed by patch 4 (but you can't 
fix out-of-tree users).

Please do it without changing existing compatible values. Even if they are 
misleading, this is all can be described in the documentation - just list 
SoCs that can be used with each compatible value there.

Best regards,
Tomasz

   reg = 0x1453 0x10;
   interrupts = 0 95 0;
   hpd-gpio = gpx3 7 0xf 1 3;
 diff --git a/Documentation/devicetree/bindings/video/exynos_hdmiddc.txt
 b/Documentation/devicetree/bindings/video/exynos_hdmiddc.txt index
 fa166d9..c1bd2ea 100644
 --- a/Documentation/devicetree/bindings/video/exynos_hdmiddc.txt
 +++ b/Documentation/devicetree/bindings/video/exynos_hdmiddc.txt
 @@ -1,12 +1,12 @@
  Device-Tree bindings for hdmiddc driver
 
  Required properties:
 -- compatible: value should be samsung,exynos5-hdmiddc.
 +- compatible: value should be samsung,exynos4210-hdmiddc.
  - reg: I2C address of the hdmiddc device.
 
  Example:
 
   hdmiddc {
 - compatible = samsung,exynos5-hdmiddc;
 + compatible = samsung,exynos4210-hdmiddc;
   reg = 0x50;
   };
 diff --git a/Documentation/devicetree/bindings/video/exynos_hdmiphy.txt
 b/Documentation/devicetree/bindings/video/exynos_hdmiphy.txt index
 858f4f9..e59d793 100644
 --- a/Documentation/devicetree/bindings/video/exynos_hdmiphy.txt
 +++ b/Documentation/devicetree/bindings/video/exynos_hdmiphy.txt
 @@ -1,12 +1,14 @@
  Device-Tree bindings for hdmiphy driver
 
  Required properties:
 -- compatible: value should be samsung,exynos5-hdmiphy.
 +- compatible: value should be
 + 1) samsung,exynos4210-hdmiphy.
 + 2) samsung,exynos4212-hdmiphy.
  - reg: I2C address of the hdmiphy device.
 
  Example:
 
   hdmiphy {
 - compatible = samsung,exynos5-hdmiphy;
 + compatible = samsung,exynos4210-hdmiphy;
   reg = 0x38;
   };
 diff --git a/Documentation/devicetree/bindings/video/exynos_mixer.txt
 b/Documentation/devicetree/bindings/video/exynos_mixer.txt index
 9b2ea03..a8b063f 100644
 --- a/Documentation/devicetree/bindings/video/exynos_mixer.txt
 +++ b/Documentation/devicetree/bindings/video/exynos_mixer.txt
 @@ -1,7 +1,10 @@
  Device-Tree bindings for mixer driver
 
  Required properties:
 -- compatible: value should be samsung,exynos5-mixer.
 +- compatible: value should be:
 + 1) samsung,exynos4210-mixer
 + 2) samsung,exynos5250-mixer
 +
  - reg: physical base address of the mixer and length of memory mapped
   region.
  - interrupts: interrupt number to the cpu.
 @@ -9,7 +12,7 @@ Required properties:
  Example:
 
   mixer {
 - compatible = samsung,exynos5-mixer;
 + compatible = 

Re: [PATCH 1/4] drm/exynos: rename compatible strings for hdmi subsystem

[Tomasz Figa]

Hi Rahul,

On Wednesday 19 of June 2013 15:02:59 Rahul Sharma wrote:
 Hi All,
 
 On Wed, Jun 19, 2013 at 1:57 PM, Inki Dae inki@samsung.com wrote:
  -Original Message-
  From: dri-devel-bounces+inki.dae=samsung@lists.freedesktop.org
  [mailto:dri-devel-bounces+inki.dae=samsung@lists.freedesktop.org] On
  Behalf Of Lucas Stach
  Sent: Wednesday, June 19, 2013 4:59 PM
  To: Tomasz Figa
  Cc: kgene@samsung.com; devicetree-disc...@lists.ozlabs.org;
  sw0312@samsung.com; jo...@samsung.com;
  
  dri-devel@lists.freedesktop.org;
  
  linux-samsung-...@vger.kernel.org; rob.herr...@calxeda.com;
  s.nawro...@samsung.com; grant.lik...@linaro.org; Rahul Sharma
  Subject: Re: [PATCH 1/4] drm/exynos: rename compatible strings for hdmi
  subsystem
  
  Am Mittwoch, den 19.06.2013, 09:52 +0200 schrieb Tomasz Figa:
   Hi Rahul,
   
   On Tuesday 18 of June 2013 18:19:35 Rahul Sharma wrote:
This patch renames the combatible strings for hdmi, mixer, ddc
and hdmiphy. It follows the convention of using compatible string
which represent the SoC in which the IP was added for the first
time.

Signed-off-by: Rahul Sharma rahul.sha...@samsung.com
---

 Documentation/devicetree/bindings/video/exynos_hdmi.txt|6

-- Documentation/devicetree/bindings/video/exynos_hdmiddc.txt |
4 ++-- Documentation/devicetree/bindings/video/exynos_hdmiphy.txt |
6 -- Documentation/devicetree/bindings/video/exynos_mixer.txt   |

 7 +-- drivers/gpu/drm/exynos/exynos_ddc.c
 
   2 +- drivers/gpu/drm/exynos/exynos_hdmi.c   |

2 +- drivers/gpu/drm/exynos/exynos_hdmiphy.c|   
4
+++- drivers/gpu/drm/exynos/exynos_mixer.c  |  
12
++-- 8 files changed, 26 insertions(+), 17 deletions(-)

diff --git a/Documentation/devicetree/bindings/video/exynos_hdmi.txt
b/Documentation/devicetree/bindings/video/exynos_hdmi.txt index
589edee..2ac01ca 100644
--- a/Documentation/devicetree/bindings/video/exynos_hdmi.txt
+++ b/Documentation/devicetree/bindings/video/exynos_hdmi.txt
@@ -1,7 +1,9 @@

 Device-Tree bindings for drm hdmi driver

 Required properties:
-- compatible: value should be samsung,exynos5-hdmi.
+- compatible: value should be one among the following:
+ 1) samsung,exynos4210-hdmi
+ 2) samsung,exynos4212-hdmi

 - reg: physical base address of the hdmi and length of memory mapped
 
  region.
 
 - interrupts: interrupt number to the cpu.

@@ -15,7 +17,7 @@ Required properties:
 Example:
  hdmi {

- compatible = samsung,exynos5-hdmi;
+ compatible = samsung,exynos4212-hdmi;
   
   Sorry, but it's a NAK from me.
   
   DeviceTree bindings are considered an ABI. This is to allow older dtbs
  
  to
  
   work with new kernels.
   
   If you just change the binding this way, you break all the existing
  
  users
  
   of this compatible value.
   
   In addition you are doing it in a way that breaks bisection:
- patch 1/4 breaks existing in-tree users of current compatible
values,
- after patch 2 and 3 it is still broken,
- and eventually all in-tree users are fixed by patch 4 (but you can't
   
   fix out-of-tree users).
 
 @Tomasz, I understand your point but how is it possible to change
 compatible types in driver as well as in dtbs by not breaking either of them
 other then putting changes in a single patch.

It's very easy. (Let's forget about the fact that DT bindings are an ABI 
temporarily) You can simply add new compatible values to the driver in first 
patch, then modify dts files in second one and then remove old compatibles 
values from the driver in third patch. (Now we remember about DT being ABI 
again.)

 I ensured that hdmi stuff is
 intact with whole series merged in either tree (drm or arch). Please
 suggest a better way.
 
 The Only existing user is Exynos5250, which is modified in the same patch
 set.

This is not true. You have modified only the existing _in_ _tree_ users.

Keep in mind that device tree is not a part of the kernel. It is currently 
located in the same tree, but it is _not_ a part of the kernel.

Now think about existing boards (like exynos5250-snow) that have a dtb built 
from older dts sources stored in their flash memory. You need to maintain 
compatibilty with this old dtb in new kernels as well.

   Please do it without changing existing compatible values. Even if they
  
  are
  
   misleading, this is all can be described in the documentation - just
  
  list
  
   SoCs that can be used with each compatible value there.
  
  Or you could just introduce the new compatible value and make all
  in-tree users use this, but keep the old values around and still accept
  them in the drivers. This way you get the goodness of the cleaner new
  symbols without breaking existing users. Just mark

Re: [PATCH v3 1/3] drm/exynos: add new compatible strings for hdmi subsystem

[Tomasz Figa]

Hi Rahul,

On Thursday 20 of June 2013 07:41:53 Rahul Sharma wrote:
 Hi Tomasz, Lucas,
 
 How does this patch look to you ? Please share your views.

Looks fine now. Have my

Reviewed-by: Tomasz Figa t.f...@samsung.com

for the whole series.

Best regards,
Tomasz

 regards,
 Rahul Sharma.
 
 On Wed, Jun 19, 2013 at 6:21 PM, Rahul Sharma rahul.sha...@samsung.com 
wrote:
  This patch adds new combatible strings for hdmi, mixer, ddc
  and hdmiphy. It follows the convention of using compatible string
  which represent the SoC in which the IP was added for the first
  time.
  
  Drivers continue to support the previous compatible strings
  but further addition of these compatible strings in device tree
  is deprecated.
  
  Signed-off-by: Rahul Sharma rahul.sha...@samsung.com
  ---
  
   Documentation/devicetree/bindings/video/exynos_hdmi.txt   |7
   +-- .../devicetree/bindings/video/exynos_hdmiddc.txt  | 
 7 +-- .../devicetree/bindings/video/exynos_hdmiphy.txt
|7 +--
   Documentation/devicetree/bindings/video/exynos_mixer.txt  |8
   ++-- drivers/gpu/drm/exynos/exynos_ddc.c   |
  2 ++ drivers/gpu/drm/exynos/exynos_hdmi.c  | 
 3 +++ drivers/gpu/drm/exynos/exynos_hdmiphy.c   | 
 4  drivers/gpu/drm/exynos/exynos_mixer.c |
 13 - 8 files changed, 38 insertions(+), 13
   deletions(-)
  
  diff --git a/Documentation/devicetree/bindings/video/exynos_hdmi.txt
  b/Documentation/devicetree/bindings/video/exynos_hdmi.txt index
  589edee..c71d0f0 100644
  --- a/Documentation/devicetree/bindings/video/exynos_hdmi.txt
  +++ b/Documentation/devicetree/bindings/video/exynos_hdmi.txt
  @@ -1,7 +1,10 @@
  
   Device-Tree bindings for drm hdmi driver
  
   Required properties:
  -- compatible: value should be samsung,exynos5-hdmi.
  +- compatible: value should be one among the following:
  +   1) samsung,exynos5-hdmi DEPRECATED
  +   2) samsung,exynos4210-hdmi
  +   3) samsung,exynos4212-hdmi
  
   - reg: physical base address of the hdmi and length of memory mapped
   
  region.
   
   - interrupts: interrupt number to the cpu.
  
  @@ -15,7 +18,7 @@ Required properties:
   Example:
  hdmi {
  
  -   compatible = samsung,exynos5-hdmi;
  +   compatible = samsung,exynos4212-hdmi;
  
  reg = 0x1453 0x10;
  interrupts = 0 95 0;
  hpd-gpio = gpx3 7 0xf 1 3;
  
  diff --git
  a/Documentation/devicetree/bindings/video/exynos_hdmiddc.txt
  b/Documentation/devicetree/bindings/video/exynos_hdmiddc.txt index
  fa166d9..41eee97 100644
  --- a/Documentation/devicetree/bindings/video/exynos_hdmiddc.txt
  +++ b/Documentation/devicetree/bindings/video/exynos_hdmiddc.txt
  @@ -1,12 +1,15 @@
  
   Device-Tree bindings for hdmiddc driver
  
   Required properties:
  -- compatible: value should be samsung,exynos5-hdmiddc.
  +- compatible: value should be one of the following
  +   1) samsung,exynos5-hdmiddc DEPRECATED
  +   2) samsung,exynos4210-hdmiddc
  +
  
   - reg: I2C address of the hdmiddc device.
   
   Example:
  hdmiddc {
  
  -   compatible = samsung,exynos5-hdmiddc;
  +   compatible = samsung,exynos4210-hdmiddc;
  
  reg = 0x50;
  
  };
  
  diff --git
  a/Documentation/devicetree/bindings/video/exynos_hdmiphy.txt
  b/Documentation/devicetree/bindings/video/exynos_hdmiphy.txt index
  858f4f9..162f641 100644
  --- a/Documentation/devicetree/bindings/video/exynos_hdmiphy.txt
  +++ b/Documentation/devicetree/bindings/video/exynos_hdmiphy.txt
  @@ -1,12 +1,15 @@
  
   Device-Tree bindings for hdmiphy driver
  
   Required properties:
  -- compatible: value should be samsung,exynos5-hdmiphy.
  +- compatible: value should be one of the following:
  +   1) samsung,exynos5-hdmiphy DEPRECATED
  +   2) samsung,exynos4210-hdmiphy.
  +   3) samsung,exynos4212-hdmiphy.
  
   - reg: I2C address of the hdmiphy device.
   
   Example:
  hdmiphy {
  
  -   compatible = samsung,exynos5-hdmiphy;
  +   compatible = samsung,exynos4210-hdmiphy;
  
  reg = 0x38;
  
  };
  
  diff --git a/Documentation/devicetree/bindings/video/exynos_mixer.txt
  b/Documentation/devicetree/bindings/video/exynos_mixer.txt index
  9b2ea03..9131b99 100644
  --- a/Documentation/devicetree/bindings/video/exynos_mixer.txt
  +++ b/Documentation/devicetree/bindings/video/exynos_mixer.txt
  @@ -1,7 +1,11 @@
  
   Device-Tree bindings for mixer driver
  
   Required properties:
  -- compatible: value should be samsung,exynos5-mixer.
  +- compatible: value should be one of the following:
  +   1) samsung,exynos5-mixer DEPRECATED
  +   2) samsung,exynos4210-mixer
  +   3) samsung,exynos5250-mixer
  +
  
   - reg: physical base address of the mixer and length of memory

Re: [PATCH] drm/exynos: Add missing includes

[Tomasz Figa]

On Tuesday 02 of July 2013 16:04:49 Mark Brown wrote:
 On Tue, Jul 02, 2013 at 09:21:32PM +0900, Inki Dae wrote:
   Ensure that all externally accessed functions are correctly
   prototyped
   when defined in each file by making sure the headers with the
   protoypes
   are included in the file with the definition.
  
  I don't see why this patch is needed. it seems like including
  unnecessary headers so it makes the code size enlarged.
 
 Well, aside from it being basic good practice and allowing the compiler
 to check for errors in the prototypes this is also something that sparse
 warns about.
 
 If the resulting binary size is changed by having the headers included
 then that indicates a bug in the headers - they *really* shouldn't be
 doing anything substantial here.  None of the headers in question looked
 at all worrying.

+1

Best regards,
Tomasz

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/exynos: exynos_drm_ipp: fix return value check

[Tomasz Figa]

On Thursday 04 of July 2013 21:35:00 Wei Yongjun wrote:
 From: Wei Yongjun yongjun_...@trendmicro.com.cn
 
 In case of error, the function ipp_find_obj() returns ERR_PTR()
 and never returns NULL. The NULL test in the return value check
 should be replaced with IS_ERR().
 
 Signed-off-by: Wei Yongjun yongjun_...@trendmicro.com.cn
 ---
  drivers/gpu/drm/exynos/exynos_drm_ipp.c | 12 ++--
  1 file changed, 6 insertions(+), 6 deletions(-)
 

Reviewed-by: Tomasz Figa t.f...@samsung.com

Best regards,
Tomasz

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: DT binding review for Armada display subsystem

[Tomasz Figa]

Hi,

On Sunday 14 of July 2013 00:09:55 Russell King - ARM Linux wrote:
 On Sun, Jul 14, 2013 at 12:16:58AM +0200, Sylwester Nawrocki wrote:
  On 07/13/2013 11:02 PM, Russell King - ARM Linux wrote:
  On Sat, Jul 13, 2013 at 10:43:29PM +0200, Sylwester Nawrocki wrote:
  I wasn't aware of it, thanks. I've seen a patch from Jiada Wang, it
  seems they're working on v4 with clock object reference counting.
  Presumably we need both clk_get() to be taking reference on the
  module and reference counted clk free, e.g. in cases where clock
  provider is a hot-pluggable device. It might be too paranoid
  though, I haven't seen hardware configurations where a clock source
  could be unplugged safely when whole system is running.
  
  I'm not going to accept refcounting being thrown into clk_get().  The
  clkdev API already has refcounting, as much as it needs to.  It just
  needs people to use the hooks that I provided back in 2008 when I
  created the clkdev API for doing _precisely_ this job.
  
  Have a read through these commits, which backup my statement above:
  
  0318e693d3a56836632bf1a2cfdafb7f34bcc703 - initial commit of the
  clkdev API d72fbdf01fc77628c0b837d0dd2fd564fa26ede6 - converting
  Integrator to clkdev API
  
  and it will show you how to do refcounting.  The common clk API just
  needs to stop defining __clk_get() and __clk_put() to be an empty
  function and implement them appropriately for it's clk
  implementation,
  like they were always meant to be.
  
  Sure, I've already seen the above commits. This is how I understood it
  as well - __clk_get(), __clk_put() need to be defined by the common
  clk
  API, since it is going to replace all the arch specific
  implementations. 
  __clk_get() and __clk_put() are the clk-implementation specific parts
  of the clkdev API, because the clkdev API is utterly divorsed from
  the
  internals of what a 'struct clk' actually is.  clkdev just treats a
  'struct clk' as a completely opaque type and never bothers poking
  about inside it.
  
  OK, but at the clock's implementation side we may need, e.g. to use
  kref to keep track of the clock's state, and free it only when it is
  unprepared, all its children are unregistered, etc. ? Is it not what
  you stated in your comment to patch [1] ?
 
 If you want to do refcounting on a clock (which you run into problems
 as I highlighted earlier in this thread) then yes, you need to use a
 kref, and take a reference in __clk_get() and drop it in __clk_put()
 to make things safe.
 
 Whether you also take a reference on the module supplying the clock or
 not depends whether you need to keep the code around to manipulate that
 clock while there are users of it.
 
 As I've already said - consider the possibilities with this scenario.
 Here's one:
 
   A clock consumer is using a clock, but the clock supplier has been
   removed.  The clock consumer wants to change the state of the clock
   in some way - eg, system is suspending.  clk_disable() doesn't fail,
   but on resume, clk_enable() does... (and how many people assume that
   clk_enable() never fails?)  And who knows what rate the clock is now
   producing or whether it's even producing anything...
 
 I'm not saying don't do the refcounting thing I mentioned back in June.
 I'm merely pointing out the issues that there are.  There isn't a one
 right answer here because each has their own advantages and
 disadvantages (and problems.)

What about having Mike on CC for such clock-related discussion?

Best regards,
Tomasz

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm/exynos: add dt-binding documentation for rotator

[Tomasz Figa]

On Monday 22 of July 2013 16:00:22 Sylwester Nawrocki wrote:
 On 07/22/2013 03:31 PM, Inki Dae wrote:
  ---Original Message-
  
   From: linux-samsung-soc-ow...@vger.kernel.org
   [mailto:linux-samsung-soc- ow...@vger.kernel.org] On Behalf Of
   Lucas Stach
   Sent: Monday, July 22, 2013 9:47 PM
   To: Inki Dae
   Cc: 'Mark Rutland'; 'Chanho Park';
   linux-samsung-...@vger.kernel.org;
   devicetree-disc...@lists.ozlabs.org; sw0312@samsung.com; dri-
   de...@lists.freedesktop.org; kyungmin.p...@samsung.com;
   kgene@samsung.com; linux-arm-ker...@lists.infradead.org
   Subject: Re: [PATCH 2/3] drm/exynos: add dt-binding documentation
   for
   rotator
   
   Am Montag, den 22.07.2013, 21:37 +0900 schrieb Inki Dae:
 -Original Message-
 From: Mark Rutland [mailto:mark.rutl...@arm.com]
 Sent: Monday, July 22, 2013 5:48 PM
 To: Chanho Park
 Cc: inki@samsung.com; kgene@samsung.com;
 linux-samsung-
 s...@vger.kernel.org; jy0922.s...@samsung.com; devicetree-
 disc...@lists.ozlabs.org; sw0312@samsung.com; dri-
 de...@lists.freedesktop.org; kyungmin.p...@samsung.com;
 linux-arm-
 ker...@lists.infradead.org
 Subject: Re: [PATCH 2/3] drm/exynos: add dt-binding
 documentation for
 rotator
 
 On Mon, Jul 22, 2013 at 07:49:26AM +0100, Chanho Park wrote:
  This patch adds a dt-binding document for exynos rotator.
  It
   
   describes
   
 which
 
  nodes should be defined to use the rotator.
  
  Signed-off-by: Chanho Park chanho61.p...@samsung.com
  Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
  ---
  
   .../bindings/drm/exynos/samsung-rotator.txt|   35
 
 
 
   1 file changed, 35 insertions(+)
   create mode 100644
 
 Documentation/devicetree/bindings/drm/exynos/samsung-rotator.
 txt
 
  diff --git
  a/Documentation/devicetree/bindings/drm/exynos/samsung-
 
 rotator.txt
 b/Documentation/devicetree/bindings/drm/exynos/samsung-
 rotator.txt
 
  new file mode 100644
  index 000..6b1d704
  --- /dev/null
  +++
  b/Documentation/devicetree/bindings/drm/exynos/samsung-
   
   rotator.txt
   
  @@ -0,0 +1,35 @@
  +* Samsung Image Rotator
  +
  +Required properties:
  +  - compatible : value should be the
  samsung,exynos4210.

Please, add more compatible strings for other SoC.

  +  - reg : Physical base address of the IP registers and
  length of
 
 memory
 
  + mapped region.
  +  - interrupts : interrupt number to the CPU.
  +  - clocks : clock number of exynos4 rotator clock.
  +  - clocks : clock name of rotator
 
 clock-names?
 
  +  - status : okay or disabled
  +  - limit table for image formats :
  min_w/min_h/max_w/max_h for
   
   min/max
   
 of image
 
 Limit table? This doesn't seem to be a well-defined binding,
 and it
 seems like a relatively generic thing to describe.
 
  +
  +Example:
  +   rotator: rotator@1281 {
  +   compatible = samsung,exynos4210-rotator;
  +   reg = 0x1281 0x1000;
  +   interrupts = 0 83 0;
  +   clocks = clock 278;
  +   clock-names = rotator;
  +   status = disabled;
  +   ycbcr420_2p {
 
 Which names are allowed for these subnodes?
 
  +   min_w = 32;
  +   min_h = 32;
  +   max_w = 32768;
  +   max_h = 32768;
  +   align = 3;
 
 min-width, min-height, max-width, max-height? What units are
 they in?
 
 What does alignment specify exactly?
 
 Are these a configurable part of the rotator hardware, or are
 these
 values always the same?

Right, that seems like configurable part. At least, min_w/h and
max_w/h
   
   can
   
be different values according to SoC and pixel formats so they
should be described enough in this dt-binding document file.
   
   Is there really this much configurability? Could each of those
   values be a different on different SoCs
  
  Not much but Yes. Rotator devices of Exynos SoC support various pixel
  formats; RGB888, RGB565, YUV422, YUV420-2Plane, and YUV420-3Plane,
  and each of these pixel formats could be different limitation to
  minimum and maximum sizes. For example, the below shows the
  limitation to Rotator device of Exynos4412 SoC according to pixel
  formats, 
   Image format  Minimum size  Maximum size
   
RGB8888x8 8kx8k
RGB56516x16  16kx16k
YUV42216x16  

Re: [PATCH 1/3] drm/exynos: add device tree support for rotator

[Tomasz Figa]

On Monday 22 of July 2013 15:49:25 Chanho Park wrote:
 The exynos4 platform is only dt-based since 3.10, we should convert
 driver data and ids to dt-based parsing methods. The rotator driver has
 a limit table to get size limit. The minimum size of RGB888 format is 8
 x 8 and maximum size is 8K x 8K. The other format, YCbCr420 2-Plane has
 32 x 32 min size and 32K x 32K max size. Each format should be multiple
 of 'align' value.
 
 Signed-off-by: Chanho Park chanho61.p...@samsung.com
 Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
 ---
  drivers/gpu/drm/exynos/exynos_drm_rotator.c |  110
 +++ 1 file changed, 80 insertions(+), 30
 deletions(-)
 
 diff --git a/drivers/gpu/drm/exynos/exynos_drm_rotator.c
 b/drivers/gpu/drm/exynos/exynos_drm_rotator.c index 427640a..b353a10
 100644
 --- a/drivers/gpu/drm/exynos/exynos_drm_rotator.c
 +++ b/drivers/gpu/drm/exynos/exynos_drm_rotator.c
 @@ -96,7 +96,7 @@ struct rot_context {
   struct resource *regs_res;
   void __iomem*regs;
   struct clk  *clock;
 - struct rot_limit_table  *limit_tbl;
 + struct rot_limit_table  limit_tbl;
   int irq;
   int cur_buf_id[EXYNOS_DRM_OPS_MAX];
   boolsuspended;
 @@ -167,7 +167,7 @@ static irqreturn_t rotator_irq_handler(int irq, void
 *arg) static void rotator_align_size(struct rot_context *rot, u32 fmt,
 u32 *hsize, u32 *vsize)
  {
 - struct rot_limit_table *limit_tbl = rot-limit_tbl;
 + struct rot_limit_table *limit_tbl = rot-limit_tbl;
   struct rot_limit *limit;
   u32 mask, val;
 
 @@ -632,6 +632,72 @@ static int rotator_ippdrv_start(struct device *dev,
 enum drm_exynos_ipp_cmd cmd) return 0;
  }
 
 +static const struct of_device_id exynos_rotator_match[] = {
 + {
 + .compatible = samsung,exynos4210-rotator,
 + },
 + {},
 +};
 +MODULE_DEVICE_TABLE(of, exynos_rotator_match);
 +
 +static int rotator_parse_dt_tbl(struct device_node *np, struct
 rot_limit *rlim) +{
 + int ret;
 +
 + ret = of_property_read_u32(np, min_w, rlim-min_w);
 + if (ret)
 + return ret;
 +
 + ret = of_property_read_u32(np, min_h, rlim-min_h);
 + if (ret)
 + return ret;
 +
 + ret = of_property_read_u32(np, max_w, rlim-max_w);
 + if (ret)
 + return ret;
 +
 + ret = of_property_read_u32(np, max_h, rlim-max_h);
 + if (ret)
 + return ret;
 +
 + ret = of_property_read_u32(np, align, rlim-align);
 + if (ret)
 + return ret;
 +
 + return 0;
 +}
 +
 +static int rotator_parse_dt(struct device *dev, struct rot_context
 *rot) +{
 + struct device_node *ycbcr_node, *rgb888_node;
 + int ret;
 +
 + ycbcr_node = of_get_child_by_name(dev-of_node, ycbcr420_2p);
 + if (!ycbcr_node) {
 + dev_err(dev, can't find ycbcr420_2p node\n);
 + return -ENODEV;
 + }
 +
 + rgb888_node = of_get_child_by_name(dev-of_node, rgb888);
 + if (!rgb888_node) {
 + dev_err(dev, can't find rgb888 node\n);
 + return -ENODEV;
 + }
 +
 + ret = rotator_parse_dt_tbl(ycbcr_node, rot-
limit_tbl.ycbcr420_2p);
 + if (ret) {
 + dev_err(dev, failed to parse ycbcr420 data\n);
 + return ret;
 + }
 + ret = rotator_parse_dt_tbl(rgb888_node, rot-limit_tbl.rgb888);
 + if (ret) {
 + dev_err(dev, failed to parse rgb888 data\n);
 + return ret;
 + }
 +
 + return 0;
 +}
 +
  static int rotator_probe(struct platform_device *pdev)
  {
   struct device *dev = pdev-dev;
 @@ -639,14 +705,23 @@ static int rotator_probe(struct platform_device
 *pdev) struct exynos_drm_ippdrv *ippdrv;
   int ret;
 
 + if (!dev-of_node) {
 + dev_err(dev, Cannot find device tree node\n);
 + return -ENODEV;
 + }
 +
   rot = devm_kzalloc(dev, sizeof(*rot), GFP_KERNEL);
   if (!rot) {
   dev_err(dev, failed to allocate rot\n);
   return -ENOMEM;
   }
 
 - rot-limit_tbl = (struct rot_limit_table *)
 - platform_get_device_id(pdev)-driver_data;
 + ret = rotator_parse_dt(dev, rot);
 + if (ret) {
 + dev_err(dev, failed parse dt info\n);
 + devm_kfree(dev, rot);
 + return ret;
 + }
 
   rot-regs_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
   rot-regs = devm_ioremap_resource(dev, rot-regs_res);
 @@ -718,31 +793,6 @@ static int rotator_remove(struct platform_device
 *pdev) return 0;
  }
 
 -static struct rot_limit_table rot_limit_tbl = {
 - .ycbcr420_2p = {
 - .min_w = 32,
 - .min_h = 32,
 - .max_w = SZ_32K,
 - .max_h = SZ_32K,
 - .align = 3,
 - },
 - .rgb888 = {
 - .min_w = 8,
 - .min_h = 8,
 - .max_w = SZ_8K,
 - .max_h = SZ_8K,
 - .align = 2,
 - },
 -};
 -
 -static struct 

Re: [PATCH] drm/exynos: Add check for IOMMU while passing physically continous memory flag

[Tomasz Figa]

Hi Vikas,

On Thursday 01 of August 2013 16:49:32 Vikas Sajjan wrote:
 While trying to get boot-logo up on exynos5420 SMDK which has eDP panel
 connected with resolution 2560x1600, following error occured even with
 IOMMU enabled:
 [0.88] [drm:lowlevel_buffer_allocate] *ERROR* failed to allocate
 buffer. [0.89] [drm] Initialized exynos 1.0.0 20110530 on minor 0
 
 This patch fixes the issue by adding a check for IOMMU.
 
 Signed-off-by: Vikas Sajjan vikas.saj...@linaro.org
 Signed-off-by: Arun Kumar arun...@samsung.com
 ---
  drivers/gpu/drm/exynos/exynos_drm_fbdev.c |9 -
  1 file changed, 8 insertions(+), 1 deletion(-)
 
 diff --git a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
 b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c index 8e60bd6..2a8
 100644
 --- a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
 +++ b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
 @@ -16,6 +16,7 @@
  #include drm/drm_crtc.h
  #include drm/drm_fb_helper.h
  #include drm/drm_crtc_helper.h
 +#include drm/exynos_drm.h
 
  #include exynos_drm_drv.h
  #include exynos_drm_fb.h
 @@ -143,6 +144,7 @@ static int exynos_drm_fbdev_create(struct
 drm_fb_helper *helper, struct platform_device *pdev = dev-platformdev;
   unsigned long size;
   int ret;
 + unsigned int flag;
 
   DRM_DEBUG_KMS(surface width(%d), height(%d) and bpp(%d\n,
   sizes-surface_width, sizes-surface_height,
 @@ -166,7 +168,12 @@ static int exynos_drm_fbdev_create(struct
 drm_fb_helper *helper, size = mode_cmd.pitches[0] * mode_cmd.height;
 
   /* 0 means to allocate physically continuous memory */
 - exynos_gem_obj = exynos_drm_gem_create(dev, 0, size);
 + if (!is_drm_iommu_supported(dev))
 + flag = 0;
 + else
 + flag = EXYNOS_BO_NONCONTIG;

While noncontig memory might be used for devices that support IOMMU, there 
should be no problem with using contig memory for them, so this seems more 
like masking the original problem rather than tracking it down.

Could you check why the allocation fails when requesting contiguous 
memory?

Best regards,
Tomasz

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/exynos: Add check for IOMMU while passing physically continous memory flag

[Tomasz Figa]

Hi Vikas,

On Friday 02 of August 2013 12:08:52 Vikas Sajjan wrote:
 Hi Rob,
 
 On 2 August 2013 06:03, Rob Clark robdcl...@gmail.com wrote:
  On Thu, Aug 1, 2013 at 7:20 PM, Tomasz Figa tomasz.f...@gmail.com 
wrote:
  Hi Vikas,
  
  On Thursday 01 of August 2013 16:49:32 Vikas Sajjan wrote:
  While trying to get boot-logo up on exynos5420 SMDK which has eDP
  panel
  connected with resolution 2560x1600, following error occured even
  with
  IOMMU enabled:
  [0.88] [drm:lowlevel_buffer_allocate] *ERROR* failed to allocate
  buffer. [0.89] [drm] Initialized exynos 1.0.0 20110530 on minor
  0
  
  This patch fixes the issue by adding a check for IOMMU.
  
  Signed-off-by: Vikas Sajjan vikas.saj...@linaro.org
  Signed-off-by: Arun Kumar arun...@samsung.com
  ---
  
   drivers/gpu/drm/exynos/exynos_drm_fbdev.c |9 -
   1 file changed, 8 insertions(+), 1 deletion(-)
[snip]
  @@ -166,7 +168,12 @@ static int exynos_drm_fbdev_create(struct
  drm_fb_helper *helper, size = mode_cmd.pitches[0] * mode_cmd.height;
  
/* 0 means to allocate physically continuous memory */
  
  - exynos_gem_obj = exynos_drm_gem_create(dev, 0, size);
  + if (!is_drm_iommu_supported(dev))
  + flag = 0;
  + else
  + flag = EXYNOS_BO_NONCONTIG;
  
  While noncontig memory might be used for devices that support IOMMU,
  there should be no problem with using contig memory for them, so
  this seems more like masking the original problem rather than
  tracking it down. 
  it is probably a good idea to not require contig memory when it is not
  needed for performance or functionality (and if it is only
  performance, then fallback gracefully to non-contig).. but yeah, would
  be good to know if this is masking another issue all the same
 
 Whats happening with CONTIG flag and with IOMMU,  is
 
  __iommu_alloc_buffer() --- dma_alloc_from_contiguous() and in this
 function it fails at
 this condition check  if (pageno = cma-count)
 
 So I tried increasing the CONFIG_CMA_SIZE_MBYTES to 24,  this check
 succeeds and it works well without my patch.
 
 But what about the case where CONFIG_CMA is disabled , yet i want
 bigger memory for a device.
  I think using IOMMU we can achieve this.

  correct me, if i am wrong.

This is probably fine. I'm not sure about performance aspects of using 
noncontig memory as framebuffer, though. This needs to be checked and if 
there is some performance penalty, I would make noncontig allocation a 
fallback case, if contig fails, as Rob has suggested.

Also I think you should adjust the commit message to say that non-
contiguous memory can be used when IOMMU is supported, so there is no need 
to force contiguous allocations, since this is not a bug fix, but rather a 
feature this patch is adding.

Best regards,
Tomasz
 
  BR,
  -R
  
  Could you check why the allocation fails when requesting contiguous
  memory?
  
  Best regards,
  Tomasz
  
  --
  To unsubscribe from this list: send the line unsubscribe
  linux-media in the body of a message to majord...@vger.kernel.org
  More majordomo info at  http://vger.kernel.org/majordomo-info.html
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH V2] drm/exynos: Add fallback option to get non physically continous memory for fb

[Tomasz Figa]

On Monday 05 of August 2013 15:14:42 Vikas Sajjan wrote:
 While trying to get boot-logo up on exynos5420 SMDK which has eDP panel
 connected with resolution 2560x1600, following error occured even with
 IOMMU enabled:
 [0.88] [drm:lowlevel_buffer_allocate] *ERROR* failed to allocate
 buffer. [0.89] [drm] Initialized exynos 1.0.0 20110530 on minor 0
 
 To address the case where physically continous memory MAY NOT be a
 mandatory requirement for fb, the patch adds a feature to get non
 physically continous memory for fb if IOMMU is supported and if CONTIG
 memory allocation fails.
 
 Signed-off-by: Vikas Sajjan vikas.saj...@linaro.org
 Signed-off-by: Arun Kumar arun...@samsung.com
 ---
 changes since v1:
- Modified to add the fallback patch if CONTIG alloc fails as suggested
 by Rob Clark robdcl...@gmail.com and Tomasz Figa
 tomasz.f...@gmail.com.
 
- changed the commit message.
 ---
  drivers/gpu/drm/exynos/exynos_drm_fbdev.c |   19 +++
  1 file changed, 15 insertions(+), 4 deletions(-)
 
 diff --git a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
 b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c index 8e60bd6..9a4b886
 100644
 --- a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
 +++ b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
 @@ -16,6 +16,7 @@
  #include drm/drm_crtc.h
  #include drm/drm_fb_helper.h
  #include drm/drm_crtc_helper.h
 +#include drm/exynos_drm.h
 
  #include exynos_drm_drv.h
  #include exynos_drm_fb.h
 @@ -165,11 +166,21 @@ static int exynos_drm_fbdev_create(struct
 drm_fb_helper *helper,
 
   size = mode_cmd.pitches[0] * mode_cmd.height;
 
 - /* 0 means to allocate physically continuous memory */
 - exynos_gem_obj = exynos_drm_gem_create(dev, 0, size);
 + exynos_gem_obj = exynos_drm_gem_create(dev, EXYNOS_BO_CONTIG, size);

You can put the fallback here like this:
if (IS_ERR(exynos_gem_obj)  is_drm_iommu_supported(dev)) {
/*
 * If IOMMU is supported then try to get buffer from
 * non-continous memory area
 */
dev_warn(pdev-dev, contiguous FB allocation failed, falling 
back to non-contiguous\n);
exynos_gem_obj = exynos_drm_gem_create(dev,

EXYNOS_BO_NONCONTIG, size);
}

   if (IS_ERR(exynos_gem_obj)) {
 - ret = PTR_ERR(exynos_gem_obj);
 - goto err_release_framebuffer;

And then you can leave this original check untouched, reducing the
diffstat and unnecessary code indentation.

 + /*
 +  * If IOMMU is supported then try to get buffer from
 +  * non-continous memory area
 +  */
 + if (is_drm_iommu_supported(dev))
 + exynos_gem_obj = exynos_drm_gem_create(dev,
 + EXYNOS_BO_NONCONTIG, size);
 + if (IS_ERR(exynos_gem_obj)) {
 + ret = PTR_ERR(exynos_gem_obj);
 + goto err_release_framebuffer;
 + }
 + dev_warn(pdev-dev, exynos_gem_obj for FB is allocated with\n
 + non physically continuous memory\n);

Please don't split messages into multiple lines, because this makes
grepping for them harder (and checkpatch complains).

Best regards,
Tomasz

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2 1/5] drm/exynos: add device tree support for rotator

[Tomasz Figa]

Hi Chanho,

On Friday 09 of August 2013 16:40:49 Chanho Park wrote:
 The exynos4 platform is only dt-based since 3.10, we should convert
 driver data and ids to dt-based parsing methods. The rotator driver has
 a limit table to get size limit of input picture. Each SoCs has slightly
 different limit value compared with any others.
 For example, exynos4210's max_size of RGB888 is 16k x 16k. But, others
 have 8k x 8k. Another example the exynos5250 should have multiple of 2
 pixel size for its X/Y axis. Thus, we should keep different tables for
 each of them.
 
 Signed-off-by: Chanho Park chanho61.p...@samsung.com
 Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
 ---
  drivers/gpu/drm/exynos/exynos_drm_rotator.c |  109
 --- 1 file changed, 81 insertions(+), 28
 deletions(-)
 
 diff --git a/drivers/gpu/drm/exynos/exynos_drm_rotator.c
 b/drivers/gpu/drm/exynos/exynos_drm_rotator.c index 427640a..39b09e0
 100644
 --- a/drivers/gpu/drm/exynos/exynos_drm_rotator.c
 +++ b/drivers/gpu/drm/exynos/exynos_drm_rotator.c
 @@ -632,6 +632,73 @@ static int rotator_ippdrv_start(struct device *dev,
 enum drm_exynos_ipp_cmd cmd) return 0;
  }
 
 +static struct rot_limit_table rot_limit_tbl_4210 = {
 + .ycbcr420_2p = {
 + .min_w = 32,
 + .min_h = 32,
 + .max_w = SZ_64K,
 + .max_h = SZ_64K,
 + .align = 3,
 + },
 + .rgb888 = {
 + .min_w = 8,
 + .min_h = 8,
 + .max_w = SZ_16K,
 + .max_h = SZ_16K,
 + .align = 2,
 + },
 +};
 +
 +static struct rot_limit_table rot_limit_tbl_4x12 = {
 + .ycbcr420_2p = {
 + .min_w = 32,
 + .min_h = 32,
 + .max_w = SZ_32K,
 + .max_h = SZ_32K,
 + .align = 3,
 + },
 + .rgb888 = {
 + .min_w = 8,
 + .min_h = 8,
 + .max_w = SZ_8K,
 + .max_h = SZ_8K,
 + .align = 2,
 + },
 +};
 +
 +static struct rot_limit_table rot_limit_tbl_5250 = {
 + .ycbcr420_2p = {
 + .min_w = 32,
 + .min_h = 32,
 + .max_w = SZ_32K,
 + .max_h = SZ_32K,
 + .align = 3,
 + },
 + .rgb888 = {
 + .min_w = 8,
 + .min_h = 8,
 + .max_w = SZ_8K,
 + .max_h = SZ_8K,
 + .align = 1,
 + },
 +};
 +
 +static const struct of_device_id exynos_rotator_match[] = {
 + {
 + .compatible = samsung,exynos4210-rotator,
 + .data = rot_limit_tbl_4210,
 + },
 + {
 + .compatible = samsung,exynos4212-rotator,
 + .data = rot_limit_tbl_4x12,
 + },
 + {
 + .compatible = samsung,exynos5250-rotator,
 + .data = rot_limit_tbl_5250,
 + },
 + {},
 +};
 +
  static int rotator_probe(struct platform_device *pdev)
  {
   struct device *dev = pdev-dev;
 @@ -645,8 +712,19 @@ static int rotator_probe(struct platform_device
 *pdev) return -ENOMEM;
   }
 
 - rot-limit_tbl = (struct rot_limit_table *)
 - platform_get_device_id(pdev)-driver_data;
 + if (dev-of_node) {
 + const struct of_device_id *match;
 + match = of_match_node(of_match_ptr(exynos_rotator_match),
 + dev-of_node);
 + if (match == NULL) {
 + dev_err(dev, failed to match node\n);
 + return -ENODEV;
 + }
 + rot-limit_tbl = (struct rot_limit_table *)match-data;
 + } else {
 + dev_err(dev, cannot find binding\n);

What about having a check for !dev-of_node at the beginning of probe, to 
not complicate further code?

Also the error message is confusing. It should be something closer to 
device does not have of_node.

 + return -ENODEV;
 + }
 
   rot-regs_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
   rot-regs = devm_ioremap_resource(dev, rot-regs_res);
 @@ -718,31 +796,6 @@ static int rotator_remove(struct platform_device
 *pdev) return 0;
  }
 
 -static struct rot_limit_table rot_limit_tbl = {
 - .ycbcr420_2p = {
 - .min_w = 32,
 - .min_h = 32,
 - .max_w = SZ_32K,
 - .max_h = SZ_32K,
 - .align = 3,
 - },
 - .rgb888 = {
 - .min_w = 8,
 - .min_h = 8,
 - .max_w = SZ_8K,
 - .max_h = SZ_8K,
 - .align = 2,
 - },
 -};
 -
 -static struct platform_device_id rotator_driver_ids[] = {
 - {
 - .name   = exynos-rot,
 - .driver_data= (unsigned long)rot_limit_tbl,
 - },
 - {},
 -};
 -
  static int rotator_clk_crtl(struct rot_context *rot, bool enable)
  {
   if (enable) {
 @@ -804,10 +857,10 @@ static const struct dev_pm_ops rotator_pm_ops = {
  struct platform_driver rotator_driver = {
   .probe 

Re: [PATCHv2 2/5] ARM: dts: Add rotator node for exynos4210

[Tomasz Figa]

On Friday 09 of August 2013 16:40:50 Chanho Park wrote:
 This patch adds a rotator node for exynos4210. The exynos4210 has
 different limitation of image size compared with later chips.
 
 Signed-off-by: Chanho Park chanho61.p...@samsung.com
 Cc: Thomas Abraham thomas.abra...@linaro.org
 Cc: Kukjin Kim kgene@samsung.com
 Cc: Inki Dae inki@samsung.com
 Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
 ---
  arch/arm/boot/dts/exynos4.dtsi |9 +
  1 file changed, 9 insertions(+)
 
 diff --git a/arch/arm/boot/dts/exynos4.dtsi
 b/arch/arm/boot/dts/exynos4.dtsi index 597cfcf..002b2b8 100644
 --- a/arch/arm/boot/dts/exynos4.dtsi
 +++ b/arch/arm/boot/dts/exynos4.dtsi
 @@ -243,6 +243,15 @@
   status = disabled;
   };
 
 + rotator: rotator@1281 {
 + compatible = samsung,exynos4210-rotator;
 + reg = 0x1281 0x1000;
 + interrupts = 0 83 0;
 + clocks = clock 278;
 + clock-names = rotator;
 + status = disabled;

Does the rotator need any board-specific information to work? If no, it 
should not be disabled by default. Same comment goes to patch 4/5.

Best regards,
Tomasz

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2 5/5] ARM: dts: Add dt binding documentation for exynos rotator

[Tomasz Figa]

Hi Chanho,

On Friday 09 of August 2013 16:40:53 Chanho Park wrote:
 This patch describes each nodes of rotator and specifies a example how to
 bind it.
 
 Signed-off-by: Chanho Park chanho61.p...@samsung.com
 Cc: Thomas Abraham thomas.abra...@linaro.org
 Cc: Kukjin Kim kgene@samsung.com
 Cc: Inki Dae inki@samsung.com
 Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
 ---
  .../devicetree/bindings/gpu/samsung-rotator.txt|   26
  1 file changed, 26 insertions(+)
  create mode 100644
 Documentation/devicetree/bindings/gpu/samsung-rotator.txt
 
 diff --git a/Documentation/devicetree/bindings/gpu/samsung-rotator.txt
 b/Documentation/devicetree/bindings/gpu/samsung-rotator.txt new file
 mode 100644
 index 000..31ee7b5
 --- /dev/null
 +++ b/Documentation/devicetree/bindings/gpu/samsung-rotator.txt
 @@ -0,0 +1,26 @@
 +* Samsung Image Rotator
 +
 +Required properties:
 +  - compatible : value should be following:
 + (a) samsung,exynos4210-rotator for Rotator IP in Exynos4210
 + (b) samsung,exynos4212-rotator for Rotator IP in Exynos4212/4412
 + (c) samsung,exynos5250-rotator for Rotator IP in Exynos5250
 +
 +  - reg : Physical base address of the IP registers and length of memory
 +   mapped region.
 +
 +  - interrupts : Interrupt number of Rotator

What about: interrupt specifier for rotator interrupt, according to format 
specific to interrupt parent.

 +
 +  - clocks : Clock number described in
 Documentations/devicetree/binding/clock. +

Perhaps: clock specifier for rotator clock, according to generic clock 
bindings.

 +  - clock-names : Clock name.

Names of clocks specified in clocks property. For exynos rotator this 
property should contain only one name: rotator.

 +Example:
 + rotator: rotator@1280 {

I wonder if we shouldn't use a more generic name here, according to ePAPR 
node naming recommendation.

 + compatible = samsung,exynos4210-rotator;
 + reg = 0x1281 0x1000;

Typo. Node has 1280 in unit-address suffix, but this property has 
1281 instead.

 + interrupts = 0 83 0;
 + clocks = clock 278;
 + clock-names = rotator;
 + status = disabled;

Status property should be omitted from this example, as it's not a part of 
this binding.

Best regards,
Tomasz

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2 5/5] ARM: dts: Add dt binding documentation for exynos rotator

[Tomasz Figa]

On Friday 09 of August 2013 10:38:30 Stephen Warren wrote:
 On 08/09/2013 07:15 AM, Tomasz Figa wrote:
  Hi Chanho,
  
  On Friday 09 of August 2013 16:40:53 Chanho Park wrote:
  This patch describes each nodes of rotator and specifies a example how
  to bind it.
  
  diff --git a/Documentation/devicetree/bindings/gpu/samsung-rotator.txt
  
  +* Samsung Image Rotator
  +
  +Required properties:
  +  - compatible : value should be following:
  +  (a) samsung,exynos4210-rotator for Rotator IP in Exynos4210
  +  (b) samsung,exynos4212-rotator for Rotator IP in Exynos4212/4412
  +  (c) samsung,exynos5250-rotator for Rotator IP in Exynos5250
 
 Is rotator the name that the HW documentation uses to refer to this
 block?

Yes, it is. More specifically it is referred to as Image Rotator.

Best regards,
Tomasz

 If so, those compatible values seem fine. If not, they seem
 slightly too generic for me; perhaps better to use the raw HW block name?
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/3] Add dt support for exynos hdmiphy settings

[Tomasz Figa]

Hi Shirish,

On Tuesday 13 of August 2013 12:39:27 Shirish S wrote:
 For various revision of chipset if the signal pattern is changed for
 every board, then the phy setting need to be updated correspondingly by
 measuring the signal.
 With the hdmiphy settings fixed in the driver the only way currently is
 to either add a new structure or add compile time option.
 To avoid this, we can move the same to dt, wherin we can have different
 dt files for every revision. This patchset can be considered as an
 initiative towards achieving the same for exynos 5250 and can be later
 extended to future chipsets.
 Also this patchset moves the entire structure to dt file as-is in the
 driver and hence we can find all the hex values, which are not logically
 explained similar to driver.
 
 Shirish S (3):
   ARM: dts: smdk5250: Add hdmi phy settings
   ARM: dts: arndale:  Add hdmi phy settings
   drm: exynos: hdmi: Add dt support for hdmiphy settings
 
  .../devicetree/bindings/video/exynos_hdmi.txt  |   18 +-
  arch/arm/boot/dts/exynos5250-arndale.dts   |  120 
  arch/arm/boot/dts/exynos5250-smdk5250.dts  |  120 
  drivers/gpu/drm/exynos/exynos_hdmi.c   |  191
 +++- 4 files changed, 320 insertions(+), 129
 deletions(-)

Since this series is related to Exynos platform files, please also Cc the 
linux-samsung-soc mailing list and Kukjin Kim.

Same goes for device tree bindings. If you change anything related to 
device tree, you need to post your patches to devicet...@vger.kernel.org 
mailing list. Ccing device tree maintainers is also a good practice.

Please resend this series with above two issues addressed.

Best regards,
Tomasz

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/2] update pixel format setting to fimd driver

[Tomasz Figa]

Hi Inki,

On Tuesday 20 of August 2013 14:45:10 Inki Dae wrote:
 This patch series fix pixel format setting according to
 drm_framebuffer's pixel_format, and check if a given window
 supports alpha channel or not.
 
 Inki Dae (2):
   drm/exynos: fix fimd pixel format setting
   drm/exynos: check a pixel format to a particular window layer
 
  drivers/gpu/drm/exynos/exynos_drm_fimd.c |   45
 -- 1 files changed, 24 insertions(+), 21
 deletions(-)

For the whole series:

Reviewed-by: Tomasz Figa t.f...@samsung.com

Best regards,
Tomasz

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 6/6] drm/exynos: Make Exynos DRM drivers depend on OF

[Tomasz Figa]

Hi Sachin,

On Thursday 22 of August 2013 11:15:23 Sachin Kamat wrote:
 Exynos is a DT-only platform. Add this info to Kconfig.
 
 Signed-off-by: Sachin Kamat sachin.ka...@linaro.org
 ---
  drivers/gpu/drm/exynos/Kconfig |4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)
 
 diff --git a/drivers/gpu/drm/exynos/Kconfig
 b/drivers/gpu/drm/exynos/Kconfig index 772c62a..80a251a 100644
 --- a/drivers/gpu/drm/exynos/Kconfig
 +++ b/drivers/gpu/drm/exynos/Kconfig
 @@ -1,6 +1,6 @@
  config DRM_EXYNOS
   tristate DRM Support for Samsung SoC EXYNOS Series
 - depends on DRM  (PLAT_SAMSUNG || ARCH_MULTIPLATFORM)
 + depends on OF  DRM  (PLAT_SAMSUNG || ARCH_MULTIPLATFORM)
   select DRM_KMS_HELPER
   select FB_CFB_FILLRECT
   select FB_CFB_COPYAREA
 @@ -24,7 +24,7 @@ config DRM_EXYNOS_DMABUF
 
  config DRM_EXYNOS_FIMD
   bool Exynos DRM FIMD
 - depends on OF  DRM_EXYNOS  !FB_S3C  !ARCH_MULTIPLATFORM
 + depends on DRM_EXYNOS  !FB_S3C  !ARCH_MULTIPLATFORM
   select FB_MODE_HELPERS
   select VIDEOMODE_HELPERS
   help

Shouldn't this patch be first in the series?

Best regards,
Tomasz

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/7] drm/exynos: move hdmiphy related code to hdmiphy driver

[Tomasz Figa]

On Monday 30 of September 2013 00:08:46 Sylwester Nawrocki wrote:
 On 09/28/2013 06:10 PM, Inki Dae wrote:
  Any opinion from Device-Tree folks?
  
  IMO, we should have same consensus on Shirish patches before
  proceeding. 
  Rahul, it seems that DT people have no interest in this issue. So
  let's
  have a consensus about this issue internally.
  
  To Mr. Kyungmin, Sylwester, Kukjin Kim, and Tomasz,
  How about keeping hdmiphy config data in each board dts file?
 
 Please don't use HTML and quote only relevant part of e-mails. Otherwise
 there are good chances your messages end up in people's spam box.
 
 It often helps to Cc a DT binding maintainer directly.
 
 Then, you consider moving the HDMI phy configuration to the device tree.
 As Sean suggested in this thread:
 
  +static struct hdmiphy_config hdmiphy_4210_configs[] = {

I'd like to only add that patches introducing or modifying a device tree 
binding need to be acked by at least one DT binding maintainer to be 
merged.

  +   {
  +   .pixel_clock = 2700,
  +   .conf = {
  +   0x01, 0x05, 0x00, 0xD8, 0x10, 0x1C, 0x30,
  0x40,
  +   0x6B, 0x10, 0x02, 0x51, 0xDF, 0xF2, 0x54,
  0x87,
  +   0x84, 0x00, 0x30, 0x38, 0x00, 0x08, 0x10,
  0xE0,
  +   0x22, 0x40, 0xE3, 0x26, 0x00, 0x00, 0x00,
  0x00,
  +   },
  +   },
 
 [trimmed couple more entries]
 
  +};
  
  Are you aware of the effort to move these to dt? Since these are
  board-specific values, it seems incorrect to apply them universally.
  Shirish has uploaded a patch to the chromium review site to push these
  into dt (https://chromium-review.googlesource.com/#/c/65581). Maybe
  you can work that into your patch set?
 
 The configuration data is 64 bytes of the register values IIUC. Would it
 be possible to figure out exact meaning of each byte ?

This is definitely something that I would go for. Then for board specific 
data appropriate device tree properties could be defined, not just a 
binary blob.

Best regards,
Tomasz

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 00/23] drm/exynos: Refactor parts of the exynos driver

[Tomasz Figa]

Hi Sean,

On Thursday 10 of October 2013 20:30:13 Sean Paul wrote:
 This patchset refactors parts of the exynos driver to move it closer to a 
 proper
 drm driver (rather than just implementing a drm layer on top of the hardware
 drivers). The hope is to get to a point where the dp/hdmi drivers can 
 implement
 drm_connector/drm_encoder directly, and fimd/mixer can directly implement
 drm_crtc.

This looks like a really great series. I did not have time yet to do
a thorough review, but at first glance it looks fine. Let me try to
take a deeper look into this next week.

Best regards,
Tomasz

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/4] drm/rockchip: Do not use DMA mapping API if attached to IOMMU domain

[Tomasz Figa]

Hi Mark,

Thanks for reviving this series and sorry for not taking care of it
myself. Please see some comments inline.

On Tue, Feb 7, 2017 at 3:09 PM, Mark Yao <mark@rock-chips.com> wrote:
> From: Tomasz Figa <tf...@chromium.org>
>
> The API is not suitable for subsystems consisting of multiple devices
> and requires severe hacks to use it. To mitigate this, this patch
> implements allocation and address space management locally by using
> helpers provided by DRM framework, like other DRM drivers do, e.g.
> Tegra.
>
> This patch should not introduce any functional changes until the driver
> is made to attach subdevices into an IOMMU domain with the generic IOMMU
> API, which will happen in following patch. Based heavily on GEM
> implementation of Tegra DRM driver.
>
> Signed-off-by: Tomasz Figa <tf...@chromium.org>
> Signed-off-by: Shunqian Zheng <zhen...@rock-chips.com>
> Acked-by: Mark Yao <mark@rock-chips.com>

I believe you need your Signed-off-by here.

> ---
>  drivers/gpu/drm/rockchip/rockchip_drm_drv.h |   4 +-
>  drivers/gpu/drm/rockchip/rockchip_drm_gem.c | 217 
> ++--
>  drivers/gpu/drm/rockchip/rockchip_drm_gem.h |   8 +
>  3 files changed, 219 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h 
> b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
> index fb6226c..7c123d9 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
> @@ -30,6 +30,7 @@
>
>  struct drm_device;
>  struct drm_connector;
> +struct iommu_domain;
>
>  /*
>   * Rockchip drm private crtc funcs.
> @@ -60,7 +61,8 @@ struct rockchip_drm_private {
> struct drm_gem_object *fbdev_bo;
> const struct rockchip_crtc_funcs *crtc_funcs[ROCKCHIP_MAX_CRTC];
> struct drm_atomic_state *state;
> -
> +   struct iommu_domain *domain;
> +   struct drm_mm mm;
> struct list_head psr_list;
> spinlock_t psr_list_lock;
>  };
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c 
> b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
> index b70f942..5209392 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
> @@ -16,11 +16,135 @@
>  #include 
>  #include 
>  #include 
> +#include 
>
>  #include "rockchip_drm_drv.h"
>  #include "rockchip_drm_gem.h"
>
> -static int rockchip_gem_alloc_buf(struct rockchip_gem_object *rk_obj,
> +static int rockchip_gem_iommu_map(struct rockchip_gem_object *rk_obj)
> +{
> +   struct drm_device *drm = rk_obj->base.dev;
> +   struct rockchip_drm_private *private = drm->dev_private;
> +   int prot = IOMMU_READ | IOMMU_WRITE;
> +   ssize_t ret;
> +
> +   ret = drm_mm_insert_node_generic(>mm, _obj->mm,
> +rk_obj->base.size, PAGE_SIZE,
> +0, 0);
> +   if (ret < 0) {
> +   DRM_ERROR("out of I/O virtual memory: %zd\n", ret);
> +   return ret;
> +   }
> +
> +   rk_obj->dma_addr = rk_obj->mm.start;
> +
> +   ret = iommu_map_sg(private->domain, rk_obj->dma_addr, 
> rk_obj->sgt->sgl,
> +  rk_obj->sgt->nents, prot);
> +   if (ret < 0) {
> +   DRM_ERROR("failed to map buffer: %zd\n", ret);
> +   goto err_remove_node;
> +   }
> +
> +   rk_obj->size = ret;
> +
> +   return 0;
> +
> +err_remove_node:
> +   drm_mm_remove_node(_obj->mm);
> +
> +   return ret;
> +}
> +
> +static int rockchip_gem_iommu_unmap(struct rockchip_gem_object *rk_obj)
> +{
> +   struct drm_device *drm = rk_obj->base.dev;
> +   struct rockchip_drm_private *private = drm->dev_private;
> +
> +   iommu_unmap(private->domain, rk_obj->dma_addr, rk_obj->size);
> +   drm_mm_remove_node(_obj->mm);
> +
> +   return 0;
> +}
> +
> +static int rockchip_gem_get_pages(struct rockchip_gem_object *rk_obj)
> +{
> +   struct drm_device *drm = rk_obj->base.dev;
> +   int ret, i;
> +   struct scatterlist *s;
> +
> +   rk_obj->pages = drm_gem_get_pages(_obj->base);
> +   if (IS_ERR(rk_obj->pages))
> +   return PTR_ERR(rk_obj->pages);
> +
> +   rk_obj->num_pages = rk_obj->base.size >> PAGE_SHIFT;
> +
> +   rk_obj->sgt = drm_prime_pages_to_sg(rk_obj->pages, rk_obj->num_pages);
> +   if (IS_ERR(rk_obj->sgt)) {
> +   ret = PTR_ERR(rk_obj->sgt);

Re: [PATCH v2 7/7] drm/rockchip: Call drm_gem_object_release() to destroy GEM base

[Tomasz Figa]

Hi Mark,

On Tue, Feb 7, 2017 at 9:37 PM, Thierry Reding <thierry.red...@gmail.com> wrote:
> On Tue, Feb 07, 2017 at 04:39:33PM +0800, Mark Yao wrote:
>> From: Tomasz Figa <tf...@chromium.org>
>>
>> When converting the driver to use shmem-backed GEMs for IOMMU-enabled
>> systems, we forgot to add calls to drm_gem_object_release(), which gave
>> us a quite nice memory leak. This patch adds the missing calls.
>>
>> Fixes: f11d5f0 ("FROMLIST: drm/rockchip: Do not use DMA mapping API if
>> attached to IOMMU domain")

Since the patch being fixed is also a part of this series, the fix
could be just squashed directly (with sign-off lists merged). Same for
patch 6/7.

Best regards,
Tomasz
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH libdrm] tests: Use -pthread in CFLAGS instead of -lpthread

[Tomasz Figa]

-lpthread is not always a valid flag to pull pthread support, especially
on Android it will fail to link due to a missing libpthread.so. The more
generic way to build-in pthread support is to use the -pthread CFLAG, so
let's use it instead.

Signed-off-by: Tomasz Figa <tf...@chromium.org>
---
 tests/exynos/Makefile.am   | 4 ++--
 tests/modetest/Makefile.am | 4 ++--
 tests/nouveau/Makefile.am  | 3 ++-
 3 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/tests/exynos/Makefile.am b/tests/exynos/Makefile.am
index 357d6b8c..b6361727 100644
--- a/tests/exynos/Makefile.am
+++ b/tests/exynos/Makefile.am
@@ -1,4 +1,5 @@
 AM_CFLAGS = \
+   -pthread \
$(WARN_CFLAGS)\
-I $(top_srcdir)/include/drm \
-I $(top_srcdir)/libkms/ \
@@ -34,8 +35,7 @@ exynos_fimg2d_perf_LDADD = \
 
 exynos_fimg2d_event_LDADD = \
$(top_builddir)/libdrm.la \
-   $(top_builddir)/exynos/libdrm_exynos.la \
-   -lpthread
+   $(top_builddir)/exynos/libdrm_exynos.la
 
 exynos_fimg2d_test_LDADD = \
$(top_builddir)/libdrm.la \
diff --git a/tests/modetest/Makefile.am b/tests/modetest/Makefile.am
index 9686ccb9..4b296c83 100644
--- a/tests/modetest/Makefile.am
+++ b/tests/modetest/Makefile.am
@@ -3,6 +3,7 @@ include Makefile.sources
 AM_CFLAGS = $(filter-out -Wpointer-arith, $(WARN_CFLAGS))
 
 AM_CFLAGS += \
+   -pthread \
-I$(top_srcdir)/include/drm \
-I$(top_srcdir)/tests \
-I$(top_srcdir)
@@ -20,5 +21,4 @@ modetest_SOURCES = $(MODETEST_FILES)
 modetest_LDADD = \
$(top_builddir)/libdrm.la \
$(top_builddir)/tests/util/libutil.la \
-   $(CAIRO_LIBS) \
-   -lpthread
+   $(CAIRO_LIBS)
diff --git a/tests/nouveau/Makefile.am b/tests/nouveau/Makefile.am
index c4f6e299..593acef0 100644
--- a/tests/nouveau/Makefile.am
+++ b/tests/nouveau/Makefile.am
@@ -1,4 +1,5 @@
 AM_CPPFLAGS = \
+   -pthread \
-I$(top_srcdir)/include/drm \
-I$(top_srcdir)/nouveau \
-I$(top_srcdir)
@@ -8,7 +9,7 @@ AM_CFLAGS = $(WARN_CFLAGS)
 LDADD = \
../../nouveau/libdrm_nouveau.la \
../../libdrm.la \
-   -ldl -lpthread
+   -ldl
 
 TESTS = threaded
 
-- 
2.11.0.483.g087da7b7c-goog

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/bridge: analogix_dp: Don't return -EBUSY when msg->size is 0 in aux transaction

[Tomasz Figa]

Hi Zain,

On Mon, Feb 13, 2017 at 6:27 PM, zain wang  wrote:
>
> The analogix_dp_transfer() will return -EBUSY if num_transferred is zero.
> But sometimes we will send a bare address packet to start the transaction,
> like drm_dp_i2c_xfer() show:
> ..
> /* Send a bare address packet to start the transaction.
>  * Zero sized messages specify an address only (bare
>  * address) transaction.
>  */
> msg.buffer = NULL;
> msg.size = 0;
> err = drm_dp_i2c_do_msg(aux, );
> ..
>
> In this case, the msg->size is zero, so the num_transferred will be zero too.
> We can't return -EBUSY here, let's we return num_transferred if 
> num_transferred
> equals msg->size.
>

Please see my question inline.

> Signed-off-by: zain wang 
> ---
>  drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c 
> b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> index 303083a..5384aca 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> @@ -1162,5 +1162,5 @@ ssize_t analogix_dp_transfer(struct analogix_dp_device 
> *dp,
>  (msg->request & ~DP_AUX_I2C_MOT) == DP_AUX_NATIVE_READ)
> msg->reply = DP_AUX_NATIVE_REPLY_ACK;
>
> -   return num_transferred > 0 ? num_transferred : -EBUSY;
> +   return (num_transferred == msg->size) ? num_transferred : -EBUSY;

I might be missing something but, looking at the code, I don't see any
possibility of num_transferred ever being different than msg->size. To
be honest, it doesn't seem to even make any sense keeping the local
variable there, because msg->size can be simply always returned, as
errors are handled by jumping to aux_error label.

Best regards,
Tomasz
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/bridge: analogix_dp: Don't return -EBUSY when msg->size is 0 in aux transaction

[Tomasz Figa]

On Mon, Feb 20, 2017 at 1:04 PM, Zain Wang <w...@rock-chips.com> wrote:
> 在 2017/2/20 10:40, Tomasz Figa 写道:
>> On Mon, Feb 13, 2017 at 6:27 PM, zain wang <w...@rock-chips.com> wrote:
>>> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
>>> b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
>>> index 303083a..5384aca 100644
>>> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
>>> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
>>> @@ -1162,5 +1162,5 @@ ssize_t analogix_dp_transfer(struct
>>> analogix_dp_device *dp,
>>>   (msg->request & ~DP_AUX_I2C_MOT) == DP_AUX_NATIVE_READ)
>>>  msg->reply = DP_AUX_NATIVE_REPLY_ACK;
>>>
>>> -   return num_transferred > 0 ? num_transferred : -EBUSY;
>>> +   return (num_transferred == msg->size) ? num_transferred : -EBUSY;
>>
>> I might be missing something but, looking at the code, I don't see any
>> possibility of num_transferred ever being different than msg->size. To
>> be honest, it doesn't seem to even make any sense keeping the local
>> variable there, because msg->size can be simply always returned, as
>> errors are handled by jumping to aux_error label.
>
> Yeah, I agree with you.
> The better way to fix this issue is to revert the changes
> https://patchwork.kernel.org/patch/9411711/
> (returning num_transferred directly may be better here)

I think it's still not enough to clean up the code completely. It
should be just enough to remove num_transferred completely and simply
return msg->size at the end.

Best regards,
Tomasz
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 0/8] drm/rockchip: Flip wait clean-up

[Tomasz Figa]

The display controller found on Rockchip SoCs supported by Rockchip DRM
driver (VOP) is a bit problematic, because it does not provide hardware
vblank counter. Because vblank interrupt is used to feed the software
counter, the driver had custom code to wait for flip completion to avoid
race between atomic flush and vblank interrupt handler.

This, however, brought a different set of issues. In fact, even with the
custom wait code, there is stil a race between the handler and driver
state update (of the values used to compare with registers to determine
if flip has completed). On top of that, legacy cursor updates are not
implemented properly and have to wait for vblank to complete, which
is against the API specification.

This series attempts to clean up the driver from this custom waiting code,
eliminating related races and bringing correct handling of legacy cursor
plane. It also gives a nice effect of more than 100 lines of code removed.

This is a forward port of patches from 4.4 kernel used by ChromiumOS and
tested there. Even though the code base has not changed significantly, it
would be nice if someone with proper testing environment could give them
a try.

Based on for-next branch of Sean Paul's dogwood tree:
https://cgit.freedesktop.org/~seanpaul/dogwood/log/?h=for-next
git://people.freedesktop.org/~seanpaul/dogwood

Tomasz Figa (8):
  drm/rockchip: Clear interrupt status bits before enabling
  drm/rockchip: Get rid of some unnecessary code
  drm/rockchip: Avoid race with vblank count increment
  drm/rockchip: Unreference framebuffers from flip work
  drm/rockchip: Replace custom wait_for_vblanks with helper
  drm/rockchip: Do not enable vblank without event
  drm/rockchip: Always signal event in next vblank after cfg_done
  drm/rockchip: Kill vop_plane_state

 drivers/gpu/drm/rockchip/rockchip_drm_drv.h |   1 -
 drivers/gpu/drm/rockchip/rockchip_drm_fb.c  |  64 +--
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 247 +++-
 3 files changed, 95 insertions(+), 217 deletions(-)

-- 
2.8.0.rc3.226.g39d4020

[PATCH 1/8] drm/rockchip: Clear interrupt status bits before enabling

[Tomasz Figa]

The enable register only masks the raw status bits to signal CPU
interrupt only for enabled interrupts. The status bits are activated
regardless of the enable register. This means that we might have an old
interrupt event queued, which we are not interested in. To avoid getting
a spurious interrupt signalled, we have to clear the old bit before we
update the enable register.

Signed-off-by: Tomasz Figa 
---
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c 
b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index 209167b..7e811cf 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -414,6 +414,7 @@ static void vop_dsp_hold_valid_irq_enable(struct vop *vop)

spin_lock_irqsave(>irq_lock, flags);

+   VOP_INTR_SET_TYPE(vop, clear, DSP_HOLD_VALID_INTR, 1);
VOP_INTR_SET_TYPE(vop, enable, DSP_HOLD_VALID_INTR, 1);

spin_unlock_irqrestore(>irq_lock, flags);
@@ -479,6 +480,7 @@ static void vop_line_flag_irq_enable(struct vop *vop, int 
line_num)
spin_lock_irqsave(>irq_lock, flags);

VOP_CTRL_SET(vop, line_flag_num[0], line_num);
+   VOP_INTR_SET_TYPE(vop, clear, LINE_FLAG_INTR, 1);
VOP_INTR_SET_TYPE(vop, enable, LINE_FLAG_INTR, 1);

spin_unlock_irqrestore(>irq_lock, flags);
@@ -921,6 +923,7 @@ static int vop_crtc_enable_vblank(struct drm_crtc *crtc)

spin_lock_irqsave(>irq_lock, flags);

+   VOP_INTR_SET_TYPE(vop, clear, FS_INTR, 1);
VOP_INTR_SET_TYPE(vop, enable, FS_INTR, 1);

spin_unlock_irqrestore(>irq_lock, flags);
-- 
2.8.0.rc3.226.g39d4020

[PATCH 5/8] drm/rockchip: Replace custom wait_for_vblanks with helper

[Tomasz Figa]

Currently the driver uses a custom function to wait for flip to complete
after an atomic commit. It was needed before because of two problems:
 - there is no hardware vblank counter, so the original helper would
   have a race condition with the vblank interrupt,
 - the driver didn't support unreferencing cursor framebuffers
   asynchronously to the commit, which was what the helper expected.
Since both problems have been solved by previous patches, we can now
make the driver use the generic helper and remove custom waiting code.

Signed-off-by: Tomasz Figa 
---
 drivers/gpu/drm/rockchip/rockchip_drm_drv.h |  1 -
 drivers/gpu/drm/rockchip/rockchip_drm_fb.c  | 64 +
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 14 ---
 3 files changed, 1 insertion(+), 78 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h 
b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
index 5c69845..fb6226c 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
@@ -39,7 +39,6 @@ struct drm_connector;
 struct rockchip_crtc_funcs {
int (*enable_vblank)(struct drm_crtc *crtc);
void (*disable_vblank)(struct drm_crtc *crtc);
-   void (*wait_for_update)(struct drm_crtc *crtc);
 };

 struct rockchip_crtc_state {
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c 
b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
index 9890ecc..0f6eda0 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
@@ -174,68 +174,6 @@ static void rockchip_drm_output_poll_changed(struct 
drm_device *dev)
drm_fb_helper_hotplug_event(fb_helper);
 }

-static void rockchip_crtc_wait_for_update(struct drm_crtc *crtc)
-{
-   struct rockchip_drm_private *priv = crtc->dev->dev_private;
-   int pipe = drm_crtc_index(crtc);
-   const struct rockchip_crtc_funcs *crtc_funcs = priv->crtc_funcs[pipe];
-
-   if (crtc_funcs && crtc_funcs->wait_for_update)
-   crtc_funcs->wait_for_update(crtc);
-}
-
-/*
- * We can't use drm_atomic_helper_wait_for_vblanks() because rk3288 and rk3066
- * have hardware counters for neither vblanks nor scanlines, which results in
- * a race where:
- * | <-- HW vsync irq and reg take effect
- *plane_commit --> |
- * get_vblank and wait --> |
- * | <-- handle_vblank, vblank->count + 1
- *  cleanup_fb --> |
- * iommu crash --> |
- * | <-- HW vsync irq and reg take effect
- *
- * This function is equivalent but uses rockchip_crtc_wait_for_update() instead
- * of waiting for vblank_count to change.
- */
-static void
-rockchip_atomic_wait_for_complete(struct drm_device *dev, struct 
drm_atomic_state *old_state)
-{
-   struct drm_crtc_state *old_crtc_state;
-   struct drm_crtc *crtc;
-   int i, ret;
-
-   for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) {
-   /* No one cares about the old state, so abuse it for tracking
-* and store whether we hold a vblank reference (and should do a
-* vblank wait) in the ->enable boolean.
-*/
-   old_crtc_state->enable = false;
-
-   if (!crtc->state->active)
-   continue;
-
-   if (!drm_atomic_helper_framebuffer_changed(dev,
-   old_state, crtc))
-   continue;
-
-   ret = drm_crtc_vblank_get(crtc);
-   if (ret != 0)
-   continue;
-
-   old_crtc_state->enable = true;
-   }
-
-   for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) {
-   if (!old_crtc_state->enable)
-   continue;
-
-   rockchip_crtc_wait_for_update(crtc);
-   drm_crtc_vblank_put(crtc);
-   }
-}
-
 static void
 rockchip_atomic_commit_tail(struct drm_atomic_state *state)
 {
@@ -250,7 +188,7 @@ rockchip_atomic_commit_tail(struct drm_atomic_state *state)

drm_atomic_helper_commit_hw_done(state);

-   rockchip_atomic_wait_for_complete(dev, state);
+   drm_atomic_helper_wait_for_vblanks(dev, state);

drm_atomic_helper_cleanup_planes(dev, state);
 }
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c 
b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index 86829f5..af9ddbe 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -122,7 +122,6 @@ struct vop {
struct mutex vsync_mutex;
bool vsync_work_pending;
struct completion dsp_hold_completion;
-   struct completion wait_update_complete;

/* protected by dev->event_lock */
struct drm_pending_vblank_event *event;
@@ -937,18 +936,9 @@ static void vop_crtc_disable_vblank(struct drm_crtc 

[PATCH 2/8] drm/rockchip: Get rid of some unnecessary code

[Tomasz Figa]

Current code implements prepare_fb and cleanup_fb callbacks only to
grab/release fb references, which is already done by atomic framework
when creating/destryoing plane state. Let's remove these
unused bits.

Signed-off-by: Tomasz Figa 
---
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 18 --
 1 file changed, 18 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c 
b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index 7e811cf..68988c6 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -641,22 +641,6 @@ static void vop_plane_destroy(struct drm_plane *plane)
drm_plane_cleanup(plane);
 }

-static int vop_plane_prepare_fb(struct drm_plane *plane,
-   struct drm_plane_state *new_state)
-{
-   if (plane->state->fb)
-   drm_framebuffer_reference(plane->state->fb);
-
-   return 0;
-}
-
-static void vop_plane_cleanup_fb(struct drm_plane *plane,
-struct drm_plane_state *old_state)
-{
-   if (old_state->fb)
-   drm_framebuffer_unreference(old_state->fb);
-}
-
 static int vop_plane_atomic_check(struct drm_plane *plane,
   struct drm_plane_state *state)
 {
@@ -849,8 +833,6 @@ static void vop_plane_atomic_update(struct drm_plane *plane,
 }

 static const struct drm_plane_helper_funcs plane_helper_funcs = {
-   .prepare_fb = vop_plane_prepare_fb,
-   .cleanup_fb = vop_plane_cleanup_fb,
.atomic_check = vop_plane_atomic_check,
.atomic_update = vop_plane_atomic_update,
.atomic_disable = vop_plane_atomic_disable,
-- 
2.8.0.rc3.226.g39d4020

[PATCH 3/8] drm/rockchip: Avoid race with vblank count increment

[Tomasz Figa]

Since VOP does not have a hardware vblank count register, the ongoing
commit might be racing with a requested vblank interrupt, which would
increment the software vblank counter before the changes being committed
actually happen.

To avoid this, we can extend .atomic_flush(), so after it sets cfg_done
bit, it polls the vblank interrupt bit until it's inactive to make sure
that any old vblank interrupt gets to the handler and then uses
synchronize_irq(vop->irq) to make sure the handler finishes running.

The polling case should happen very rarely, but even if, the total wait
time should be relatively low and in practice almost equal to the vop
hardirq handler running time.

Signed-off-by: Tomasz Figa 
---
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 34 +
 1 file changed, 34 insertions(+)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c 
b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index 68988c6..f989440 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -23,6 +23,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -1063,6 +1064,32 @@ static void vop_crtc_enable(struct drm_crtc *crtc)
rockchip_drm_psr_activate(>crtc);
 }

+static bool vop_fs_irq_is_pending(struct vop *vop)
+{
+   return VOP_INTR_GET_TYPE(vop, status, FS_INTR);
+}
+
+static void vop_wait_for_irq_handler(struct vop *vop)
+{
+   bool pending;
+   int ret;
+
+   /*
+* Spin until frame start interrupt status bit goes low, which means
+* that interrupt handler was invoked and cleared it. The timeout of
+* 10 msecs is really too long, but it is just a safety measure if
+* something goes really wrong. The wait will only happen in the very
+* unlikely case of a vblank happening exactly at the same time and
+* shouldn't exceed microseconds range.
+*/
+   ret = readx_poll_timeout_atomic(vop_fs_irq_is_pending, vop, pending,
+   !pending, 0, 10 * 1000);
+   if (ret)
+   DRM_DEV_ERROR(vop->dev, "VOP vblank IRQ stuck for 10 ms\n");
+
+   synchronize_irq(vop->irq);
+}
+
 static void vop_crtc_atomic_flush(struct drm_crtc *crtc,
  struct drm_crtc_state *old_crtc_state)
 {
@@ -1076,6 +1103,13 @@ static void vop_crtc_atomic_flush(struct drm_crtc *crtc,
vop_cfg_done(vop);

spin_unlock(>reg_lock);
+
+   /*
+* There is a (rather unlikely) possiblity that a vblank interrupt
+* fired before we set the cfg_done bit. To avoid spuriously
+* signalling flip completion we need to wait for it to finish.
+*/
+   vop_wait_for_irq_handler(vop);
 }

 static void vop_crtc_atomic_begin(struct drm_crtc *crtc,
-- 
2.8.0.rc3.226.g39d4020

[PATCH 4/8] drm/rockchip: Unreference framebuffers from flip work

[Tomasz Figa]

Currently the driver waits for vblank and then unreferences old
framebuffers from atomic commit code path. This is however breaking the
legacy cursor API, which requires the updates to be fully asynchronous.
Instead of just adding a special case for cursor, we can have actually
smaller amount of code to unreference any changed framebuffer from a
flip work.

Signed-off-by: Tomasz Figa 
---
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 41 +
 1 file changed, 41 insertions(+)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c 
b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index f989440..86829f5 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -17,6 +17,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 

 #include 
@@ -89,6 +90,10 @@
 #define to_vop_win(x) container_of(x, struct vop_win, base)
 #define to_vop_plane_state(x) container_of(x, struct vop_plane_state, base)

+enum vop_pending {
+   VOP_PENDING_FB_UNREF,
+};
+
 struct vop_plane_state {
struct drm_plane_state base;
int format;
@@ -122,6 +127,9 @@ struct vop {
/* protected by dev->event_lock */
struct drm_pending_vblank_event *event;

+   struct drm_flip_work fb_unref_work;
+   unsigned long pending;
+
struct completion line_flag_completion;

const struct vop_data *data;
@@ -1093,7 +1101,11 @@ static void vop_wait_for_irq_handler(struct vop *vop)
 static void vop_crtc_atomic_flush(struct drm_crtc *crtc,
  struct drm_crtc_state *old_crtc_state)
 {
+   struct drm_atomic_state *old_state = old_crtc_state->state;
+   struct drm_plane_state *old_plane_state;
struct vop *vop = to_vop(crtc);
+   struct drm_plane *plane;
+   int i;

if (WARN_ON(!vop->is_enabled))
return;
@@ -1110,6 +1122,19 @@ static void vop_crtc_atomic_flush(struct drm_crtc *crtc,
 * signalling flip completion we need to wait for it to finish.
 */
vop_wait_for_irq_handler(vop);
+
+   for_each_plane_in_state(old_state, plane, old_plane_state, i) {
+   if (!old_plane_state->fb)
+   continue;
+
+   if (old_plane_state->fb == plane->state->fb)
+   continue;
+
+   drm_framebuffer_reference(old_plane_state->fb);
+   drm_flip_work_queue(>fb_unref_work, old_plane_state->fb);
+   set_bit(VOP_PENDING_FB_UNREF, >pending);
+   WARN_ON(drm_crtc_vblank_get(crtc) != 0);
+   }
 }

 static void vop_crtc_atomic_begin(struct drm_crtc *crtc,
@@ -1185,6 +1210,15 @@ static const struct drm_crtc_funcs vop_crtc_funcs = {
.atomic_destroy_state = vop_crtc_destroy_state,
 };

+static void vop_fb_unref_worker(struct drm_flip_work *work, void *val)
+{
+   struct vop *vop = container_of(work, struct vop, fb_unref_work);
+   struct drm_framebuffer *fb = val;
+
+   drm_crtc_vblank_put(>crtc);
+   drm_framebuffer_unreference(fb);
+}
+
 static bool vop_win_pending_is_complete(struct vop_win *vop_win)
 {
dma_addr_t yrgb_mst;
@@ -1223,6 +1257,9 @@ static void vop_handle_vblank(struct vop *vop)

if (!completion_done(>wait_update_complete))
complete(>wait_update_complete);
+
+   if (test_and_clear_bit(VOP_PENDING_FB_UNREF, >pending))
+   drm_flip_work_commit(>fb_unref_work, system_unbound_wq);
 }

 static irqreturn_t vop_isr(int irq, void *data)
@@ -1361,6 +1398,9 @@ static int vop_create_crtc(struct vop *vop)
goto err_cleanup_crtc;
}

+   drm_flip_work_init(>fb_unref_work, "fb_unref",
+  vop_fb_unref_worker);
+
init_completion(>dsp_hold_completion);
init_completion(>wait_update_complete);
init_completion(>line_flag_completion);
@@ -1404,6 +1444,7 @@ static void vop_destroy_crtc(struct vop *vop)
 * references the CRTC.
 */
drm_crtc_cleanup(crtc);
+   drm_flip_work_cleanup(>fb_unref_work);
 }

 static int vop_initial(struct vop *vop)
-- 
2.8.0.rc3.226.g39d4020

[PATCH 6/8] drm/rockchip: Do not enable vblank without event

[Tomasz Figa]

Originally we needed to enable vblank for any atomic commit to kick the
PSR machine, but that was changed and we no longer need to do so from
a vblank interrupt. Let's return to original behavior of enabling
vblank only if it is really necessary.

This essentially reverts commit 5b6804034ae9 ("drm/rockchip: Enable
vblank without event").

Signed-off-by: Tomasz Figa 
---
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 15 ---
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c 
b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index af9ddbe..bb7a865 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -116,7 +116,6 @@ struct vop {
struct device *dev;
struct drm_device *drm_dev;
bool is_enabled;
-   bool vblank_active;

/* mutex vsync_ work */
struct mutex vsync_mutex;
@@ -1135,11 +1134,10 @@ static void vop_crtc_atomic_begin(struct drm_crtc *crtc,
rockchip_drm_psr_flush(crtc);

spin_lock_irq(>dev->event_lock);
-   vop->vblank_active = true;
-   WARN_ON(drm_crtc_vblank_get(crtc) != 0);
-   WARN_ON(vop->event);
-
if (crtc->state->event) {
+   WARN_ON(drm_crtc_vblank_get(crtc) != 0);
+   WARN_ON(vop->event);
+
vop->event = crtc->state->event;
crtc->state->event = NULL;
}
@@ -1236,12 +1234,8 @@ static void vop_handle_vblank(struct vop *vop)
spin_lock_irqsave(>event_lock, flags);
if (vop->event) {
drm_crtc_send_vblank_event(crtc, vop->event);
-   vop->event = NULL;
-
-   }
-   if (vop->vblank_active) {
-   vop->vblank_active = false;
drm_crtc_vblank_put(crtc);
+   vop->event = NULL;
}
spin_unlock_irqrestore(>event_lock, flags);

@@ -1518,7 +1512,6 @@ static int vop_initial(struct vop *vop)
clk_disable(vop->aclk);

vop->is_enabled = false;
-   vop->vblank_active = false;

return 0;

-- 
2.8.0.rc3.226.g39d4020

[PATCH 7/8] drm/rockchip: Always signal event in next vblank after cfg_done

[Tomasz Figa]

This patch makes the driver send the pending vblank event in next vblank
following the commit, relying on vblank signalling improvements done in
previous patches. This gives us vblank events that always represent the
real moment of changes hitting on the screen (which was the case only
for complete FB changes before) and lets us remove the manual window
update check.

Signed-off-by: Tomasz Figa 
---
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 54 ++---
 1 file changed, 10 insertions(+), 44 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c 
b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index bb7a865..cacdffb 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -105,10 +105,6 @@ struct vop_win {
struct drm_plane base;
const struct vop_win_data *data;
struct vop *vop;
-
-   /* protected by dev->event_lock */
-   bool enable;
-   dma_addr_t yrgb_mst;
 };

 struct vop {
@@ -716,11 +712,6 @@ static void vop_plane_atomic_disable(struct drm_plane 
*plane,
if (!old_state->crtc)
return;

-   spin_lock_irq(>dev->event_lock);
-   vop_win->enable = false;
-   vop_win->yrgb_mst = 0;
-   spin_unlock_irq(>dev->event_lock);
-
spin_lock(>reg_lock);

VOP_WIN_SET(vop, win, enable, 0);
@@ -784,11 +775,6 @@ static void vop_plane_atomic_update(struct drm_plane 
*plane,
offset += (src->y1 >> 16) * fb->pitches[0];
vop_plane_state->yrgb_mst = rk_obj->dma_addr + offset + fb->offsets[0];

-   spin_lock_irq(>dev->event_lock);
-   vop_win->enable = true;
-   vop_win->yrgb_mst = vop_plane_state->yrgb_mst;
-   spin_unlock_irq(>dev->event_lock);
-
spin_lock(>reg_lock);

VOP_WIN_SET(vop, win, format, vop_plane_state->format);
@@ -1112,6 +1098,16 @@ static void vop_crtc_atomic_flush(struct drm_crtc *crtc,
 */
vop_wait_for_irq_handler(vop);

+   spin_lock_irq(>dev->event_lock);
+   if (crtc->state->event) {
+   WARN_ON(drm_crtc_vblank_get(crtc) != 0);
+   WARN_ON(vop->event);
+
+   vop->event = crtc->state->event;
+   crtc->state->event = NULL;
+   }
+   spin_unlock_irq(>dev->event_lock);
+
for_each_plane_in_state(old_state, plane, old_plane_state, i) {
if (!old_plane_state->fb)
continue;
@@ -1129,19 +1125,7 @@ static void vop_crtc_atomic_flush(struct drm_crtc *crtc,
 static void vop_crtc_atomic_begin(struct drm_crtc *crtc,
  struct drm_crtc_state *old_crtc_state)
 {
-   struct vop *vop = to_vop(crtc);
-
rockchip_drm_psr_flush(crtc);
-
-   spin_lock_irq(>dev->event_lock);
-   if (crtc->state->event) {
-   WARN_ON(drm_crtc_vblank_get(crtc) != 0);
-   WARN_ON(vop->event);
-
-   vop->event = crtc->state->event;
-   crtc->state->event = NULL;
-   }
-   spin_unlock_irq(>dev->event_lock);
 }

 static const struct drm_crtc_helper_funcs vop_crtc_helper_funcs = {
@@ -1207,29 +1191,11 @@ static void vop_fb_unref_worker(struct drm_flip_work 
*work, void *val)
drm_framebuffer_unreference(fb);
 }

-static bool vop_win_pending_is_complete(struct vop_win *vop_win)
-{
-   dma_addr_t yrgb_mst;
-
-   if (!vop_win->enable)
-   return VOP_WIN_GET(vop_win->vop, vop_win->data, enable) == 0;
-
-   yrgb_mst = VOP_WIN_GET_YRGBADDR(vop_win->vop, vop_win->data);
-
-   return yrgb_mst == vop_win->yrgb_mst;
-}
-
 static void vop_handle_vblank(struct vop *vop)
 {
struct drm_device *drm = vop->drm_dev;
struct drm_crtc *crtc = >crtc;
unsigned long flags;
-   int i;
-
-   for (i = 0; i < vop->data->win_size; i++) {
-   if (!vop_win_pending_is_complete(>win[i]))
-   return;
-   }

spin_lock_irqsave(>event_lock, flags);
if (vop->event) {
-- 
2.8.0.rc3.226.g39d4020

[PATCH 8/8] drm/rockchip: Kill vop_plane_state

[Tomasz Figa]

After changes introduced by last patches, there is no useful data stored
in vop_plane_state struct.  Let's remove it and make the driver use
generic plane state alone.

Signed-off-by: Tomasz Figa 
---
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 94 +
 1 file changed, 15 insertions(+), 79 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c 
b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index cacdffb..57650c9 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -88,19 +88,11 @@

 #define to_vop(x) container_of(x, struct vop, crtc)
 #define to_vop_win(x) container_of(x, struct vop_win, base)
-#define to_vop_plane_state(x) container_of(x, struct vop_plane_state, base)

 enum vop_pending {
VOP_PENDING_FB_UNREF,
 };

-struct vop_plane_state {
-   struct drm_plane_state base;
-   int format;
-   dma_addr_t yrgb_mst;
-   bool enable;
-};
-
 struct vop_win {
struct drm_plane base;
const struct vop_win_data *data;
@@ -651,7 +643,6 @@ static int vop_plane_atomic_check(struct drm_plane *plane,
struct drm_crtc_state *crtc_state;
struct drm_framebuffer *fb = state->fb;
struct vop_win *vop_win = to_vop_win(plane);
-   struct vop_plane_state *vop_plane_state = to_vop_plane_state(state);
const struct vop_win_data *win = vop_win->data;
int ret;
struct drm_rect clip;
@@ -661,7 +652,7 @@ static int vop_plane_atomic_check(struct drm_plane *plane,
DRM_PLANE_HELPER_NO_SCALING;

if (!crtc || !fb)
-   goto out_disable;
+   return 0;

crtc_state = drm_atomic_get_existing_crtc_state(state->state, crtc);
if (WARN_ON(!crtc_state))
@@ -679,11 +670,11 @@ static int vop_plane_atomic_check(struct drm_plane *plane,
return ret;

if (!state->visible)
-   goto out_disable;
+   return 0;

-   vop_plane_state->format = vop_convert_format(fb->pixel_format);
-   if (vop_plane_state->format < 0)
-   return vop_plane_state->format;
+   ret = vop_convert_format(fb->pixel_format);
+   if (ret < 0)
+   return ret;

/*
 * Src.x1 can be odd when do clip, but yuv plane start point
@@ -692,19 +683,12 @@ static int vop_plane_atomic_check(struct drm_plane *plane,
if (is_yuv_support(fb->pixel_format) && ((state->src.x1 >> 16) % 2))
return -EINVAL;

-   vop_plane_state->enable = true;
-
-   return 0;
-
-out_disable:
-   vop_plane_state->enable = false;
return 0;
 }

 static void vop_plane_atomic_disable(struct drm_plane *plane,
 struct drm_plane_state *old_state)
 {
-   struct vop_plane_state *vop_plane_state = to_vop_plane_state(old_state);
struct vop_win *vop_win = to_vop_win(plane);
const struct vop_win_data *win = vop_win->data;
struct vop *vop = to_vop(old_state->crtc);
@@ -717,8 +701,6 @@ static void vop_plane_atomic_disable(struct drm_plane 
*plane,
VOP_WIN_SET(vop, win, enable, 0);

spin_unlock(>reg_lock);
-
-   vop_plane_state->enable = false;
 }

 static void vop_plane_atomic_update(struct drm_plane *plane,
@@ -727,7 +709,6 @@ static void vop_plane_atomic_update(struct drm_plane *plane,
struct drm_plane_state *state = plane->state;
struct drm_crtc *crtc = state->crtc;
struct vop_win *vop_win = to_vop_win(plane);
-   struct vop_plane_state *vop_plane_state = to_vop_plane_state(state);
const struct vop_win_data *win = vop_win->data;
struct vop *vop = to_vop(state->crtc);
struct drm_framebuffer *fb = state->fb;
@@ -742,6 +723,7 @@ static void vop_plane_atomic_update(struct drm_plane *plane,
dma_addr_t dma_addr;
uint32_t val;
bool rb_swap;
+   int format;

/*
 * can't update plane when vop is disabled.
@@ -752,7 +734,7 @@ static void vop_plane_atomic_update(struct drm_plane *plane,
if (WARN_ON(!vop->is_enabled))
return;

-   if (!vop_plane_state->enable) {
+   if (!state->visible) {
vop_plane_atomic_disable(plane, old_state);
return;
}
@@ -773,13 +755,15 @@ static void vop_plane_atomic_update(struct drm_plane 
*plane,

offset = (src->x1 >> 16) * drm_format_plane_cpp(fb->pixel_format, 0);
offset += (src->y1 >> 16) * fb->pitches[0];
-   vop_plane_state->yrgb_mst = rk_obj->dma_addr + offset + fb->offsets[0];
+   dma_addr = rk_obj->dma_addr + offset + fb->offsets[0];
+
+   format = vop_convert_format(fb->pixel_format);

spin_lock(>reg_lock);

-   VOP_WIN_SET(vop, win, format, vop_plane_state->for

[PATCH] drm/rockchip: Respect page offset for PRIME mmap calls

[Tomasz Figa]

From: Ørjan Eide <orjan.e...@arm.com>

When mapping external DMA-bufs through the PRIME mmap call, we might be
given an offset which has to be respected. However for the internal DRM
GEM mmap path, we have to ignore the fake mmap offset used to identify
the buffer only. Currently the code always zeroes out vma->vm_pgoff,
which breaks the former.

This patch fixes the problem by moving the vm_pgoff assignment to a
function that is used only for GEM mmap path, so that the PRIME path
retains the original offset.

Fixes: a8594f2 ("drm/rockchip: unset pgoff when mmap'ing gems")

Signed-off-by: Ørjan Eide 
Signed-off-by: Tomasz Figa 
Cc: stable at vger.kernel.org
---
 drivers/gpu/drm/rockchip/rockchip_drm_gem.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c 
b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
index b70f942..cab4d60 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
@@ -64,7 +64,6 @@ static int rockchip_drm_gem_object_mmap(struct drm_gem_object 
*obj,
 * VM_PFNMAP flag that was set by drm_gem_mmap_obj()/drm_gem_mmap().
 */
vma->vm_flags &= ~VM_PFNMAP;
-   vma->vm_pgoff = 0;

ret = dma_mmap_attrs(drm->dev, vma, rk_obj->kvaddr, rk_obj->dma_addr,
 obj->size, rk_obj->dma_attrs);
@@ -96,6 +95,12 @@ int rockchip_gem_mmap(struct file *filp, struct 
vm_area_struct *vma)
if (ret)
return ret;

+   /*
+* Set vm_pgoff (used as a fake buffer offset by DRM) to 0 and map the
+* whole buffer from the start.
+*/
+   vma->vm_pgoff = 0;
+
obj = vma->vm_private_data;

return rockchip_drm_gem_object_mmap(obj, vma);
-- 
2.8.0.rc3.226.g39d4020

[PATCH 2/8] drm/rockchip: Get rid of some unnecessary code

[Tomasz Figa]

Hi Mark,

On Sun, Sep 18, 2016 at 10:50 AM, Mark yao  wrote:
> On 2016年09月14日 20:54, Tomasz Figa wrote:
>>
>> Current code implements prepare_fb and cleanup_fb callbacks only to
>> grab/release fb references, which is already done by atomic framework
>> when creating/destryoing plane state. Let's remove these
>> unused bits.
>>
>> Signed-off-by: Tomasz Figa 
>> ---
>>   drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 18 --
>>   1 file changed, 18 deletions(-)
>
>
> Hi Tomasz
>
> I think we can't get rid of the prepare_fb and cleanup_fb

I think I have to disagree. Please see below for detailed explanation.

>
> see the reason:
> commit 44d0237a26395ac94160cf23f32769013b365590
> Author: Mark Yao 
> Date:   Fri Apr 29 11:37:20 2016 +0800
>
> drm/rockchip: vop: fix iommu crash with async atomic
>
> After async atomic_commit callback, drm_atomic_clean_old_fb will
> clean all old fb, but because async, the old fb may be also on
> the vop hardware, dma will access the old fb buffer, clean old
> fb will cause iommu page fault.

I think the above is not quite right. Atomic plane state holds a
reference to its fb and old state is not supposed to be destroyed
until the flip completes.

Indeed current rockchip_atomic_commit implementation has following
order of calls: rockchip_atomic_wait_for_complete(),
drm_atomic_helper_cleanup_planes(), drm_atomic_state_free(). This
means that .cleanup_fb() is called from
drm_atomic_helper_cleanup_planes() just before drm_atomic_state_free()
will release references by destroying old plane states. Note that both
are called already after rockchip_atomic_wait_for_complete(), so it
should be already safe to free the old fbs.

So the above fix doesn't really do anything, possibly just covers the
race condition of the original wait for vblank function by delaying
drm_atomic_state_free() a bit.

Moreover, the whole series has been thoroughly tested in Chrome OS 4.4
kernel, including async commits. (There is still a possibility some
newer upstream changes slightly modified the semantics, but I couldn't
find such difference. Actually one of the advantages of atomic helpers
was to avoid manually refcounting the fbs from the driver.)

Best regards,
Tomasz

[PATCH v2 1/5] drm/rockchip: sort registers define by chip's number

[Tomasz Figa]

Hi Mark,

Mark Yao  rock-chips.com> writes:
> 
> No functional changes, sort the vop registers to make
> code more readable.

I might have found a typo. I guess it could be just fixed in this patch,
if it's already moving the code around. Please see the comments inline.

> 
> Signed-off-by: Mark Yao  rock-chips.com>
> ---
>  drivers/gpu/drm/rockchip/rockchip_vop_reg.c |  166 +
--
>  drivers/gpu/drm/rockchip/rockchip_vop_reg.h |   88 +++---
>  2 files changed, 127 insertions(+), 127 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/rockchip_vop_reg.c 
b/drivers/gpu/drm/rockchip/rockchip_vop_reg.c
> index 3166b46..e75b2b8 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_vop_reg.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_vop_reg.c
>  -50,6 +50,87  static const uint32_t 
formats_win_lite[] = {
>   DRM_FORMAT_BGR565,
>  };
> 
> +static const struct vop_scl_regs rk3066_win_scl = {

Why is this rk3066 and not rk3036? It doesn't match the sorting order
introduced by this patch, by the way. Typo?

> + .scale_yrgb_x = VOP_REG(RK3036_WIN0_SCL_FACTOR_YRGB, 0x, 0x0),
> + .scale_yrgb_y = VOP_REG(RK3036_WIN0_SCL_FACTOR_YRGB, 0x, 16),
> + .scale_cbcr_x = VOP_REG(RK3036_WIN0_SCL_FACTOR_CBR, 0x, 0x0),
> + .scale_cbcr_y = VOP_REG(RK3036_WIN0_SCL_FACTOR_CBR, 0x, 16),
> +};
> +
> +static const struct vop_win_phy rk3036_win0_data = {
> + .scl = _win_scl,

Ditto.

Best regards,
Tomasz

[PATCH v2 3/5] drm/rockchip: vop: introduce VOP_REG_MASK

[Tomasz Figa]

Hi Mark,

Mark Yao  rock-chips.com> writes:

> 
> Some new vop register support mask, bit[16-31] is mask,
> bit[0-15] is value, the mask is correspond to the value.

Please see my comments inline.

> 
> Signed-off-by: Mark Yao  rock-chips.com>
> ---
>  drivers/gpu/drm/rockchip/rockchip_drm_vop.c |   45 ++---
--
>  drivers/gpu/drm/rockchip/rockchip_drm_vop.h |1 +
>  drivers/gpu/drm/rockchip/rockchip_vop_reg.c |9 +-
>  3 files changed, 32 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c 
b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> index 28596e7..59f24cd 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
[snip]
> -static inline void vop_mask_write_relaxed(struct vop *vop, uint32_t 
offset,
> -   uint32_t mask, uint32_t v)
> -{
> - if (mask) {
> + if (write_mask) {
> + v = (v << shift) | (mask << (shift + 16));

If the caller gives "v" too big, it might get over the bit 16 and affect
the mask. IMHO it would be much safer to mask (v << shift) with 0x
just in case.

> + } else {
>   uint32_t cached_val = vop->regsbak[offset >> 2];
> 
> - cached_val = (cached_val & ~mask) | v;
> - writel_relaxed(cached_val, vop->regs + offset);
> - vop->regsbak[offset >> 2] = cached_val;
> + v = (cached_val & ~(mask << shift)) | (v << shift);

Should we also mask "v" with "mask" for better safety?

Best regards,
Tomasz

[PATCH v2 4/5] drm/rockchip: vop: add rk3399 vop support

[Tomasz Figa]

Hi Mark,

Mark Yao  rock-chips.com> writes:

> 
> There are two VOP in rk3399 chip, respectively VOP_BIG and VOP_LIT.
> most registers layout of this two vop is same, their framework are both
> VOP_FULL, the Major differences of this two is that:

Reviewed-by: Tomasz Figa 

Best regards,
Tomasz

[PATCH v2 5/5] dt-bindings: add documentation for Rockchip rk3399 display controllers

[Tomasz Figa]

Hi Mark,

Mark Yao  rock-chips.com> writes:

> 
> Cc: Rob Herring <robh+dt  kernel.org>
> Cc: Pawel Moll  arm.com>
> Cc: Mark Rutland  arm.com>
> Cc: Ian Campbell <ijc+devicetree  hellion.org.uk>
> Cc: Kumar Gala  codeaurora.org>
> 
> Signed-off-by: Mark Yao  rock-chips.com>
> ---
>  .../bindings/display/rockchip/rockchip-vop.txt |5 +
>  1 file changed, 5 insertions(+)

Please see my comment inline.

> 
> diff --git a/Documentation/devicetree/bindings/display/rockchip/rockchip-
vop.txt b/Documentation/devicetree/bindings/display/rockchip/rockchip-
vop.txt
> index 196121f..6bf8473 100644
> --- a/Documentation/devicetree/bindings/display/rockchip/rockchip-vop.txt
> +++ b/Documentation/devicetree/bindings/display/rockchip/rockchip-vop.txt
>  -8,6 +8,11  Required properties:
>  - compatible: value should be one of the following
>   "rockchip,rk3036-vop";
>   "rockchip,rk3288-vop";
> + "rockchip,rk3399-vop-big";
> + "rockchip,rk3399-vop-lit";
> +Document compatible values for rk3399 display controllers.
> +Big and little display controllers are not identical and have differing
> +feature sets on the rk3399.

I think this doesn't really need any explanation. If two separate 
compatible strings are given, it means that the hardware is different. :)

Other than that,

Reviewed-by: Tomasz Figa 

Best regards,
Tomasz

[PATCH v2 1/7] iommu/rockchip: fix devm_{request, free}_irq parameter

[Tomasz Figa]

Hi,

On Wed, Jun 8, 2016 at 10:26 PM, Shunqian Zheng  
wrote:
> From: Simon Xue 
>
> Even though the iommu shares irq with its master, using the *dev of iommu
> instead of master's *dev for devm_{request,free}_irq makes things clear.
>
> Signed-off-by: Simon Xue 
> Signed-off-by: Shunqian Zheng 
> ---
>  drivers/iommu/rockchip-iommu.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>

Reviewed-by: Tomasz Figa 

Best regards,
Tomasz

[PATCH v2 2/7] iommu/rockchip: add map_sg callback for rk_iommu_ops

[Tomasz Figa]

Hi,

On Wed, Jun 8, 2016 at 10:26 PM, Shunqian Zheng  
wrote:
> From: Simon Xue 
>
> The iommu_dma_alloc() in iommu/dma-iommu.c calls iommu_map_sg()
> that requires the callback iommu_ops .map_sg(). Adding the
> default_iommu_map_sg() to rockchip iommu accordingly.
>
> Signed-off-by: Simon Xue 
> Signed-off-by: Shunqian Zheng 
> ---
>  drivers/iommu/rockchip-iommu.c | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-by: Tomasz Figa 

Best regards,
Tomasz

[PATCH v2 4/7] ARM: dts: rockchip: add virtual iommu for display

[Tomasz Figa]

Hi,

On Wed, Jun 8, 2016 at 10:26 PM, Shunqian Zheng  
wrote:
> An virtual iommu without reg or interrupts for display.
> Adding this according to iommu driver changes.
>
> Signed-off-by: Shunqian Zheng 
> ---
>  arch/arm/boot/dts/rk3288.dtsi | 6 ++
>  1 file changed, 6 insertions(+)
>
> diff --git a/arch/arm/boot/dts/rk3288.dtsi b/arch/arm/boot/dts/rk3288.dtsi
> index 7fa932f..4cd535f 100644
> --- a/arch/arm/boot/dts/rk3288.dtsi
> +++ b/arch/arm/boot/dts/rk3288.dtsi
> @@ -219,9 +219,15 @@
> clock-names = "timer", "pclk";
> };
>
> +   display_mmu: virtual-iommu {
> +   compatible = "rockchip,iommu";
> +   #iommu-cells = <0>;
> +   };
> +

Device tree should describe real hardware and so it isn't really a
good idea to add such virtual iommu, especially using a compatible
string of a real device.

Please see my comments to patch 3/7 for an alternative idea.

Best regards,
Tomasz

[PATCH v2 3/7] iommu/rockchip: support virtual iommu slave device

[Tomasz Figa]

Hi,

On Wed, Jun 8, 2016 at 10:26 PM, Shunqian Zheng  
wrote:
> An virtual master device like DRM need to attach to iommu
> domain to share the domain with VOP(the one with actual
> iommu slave). We currently check the group is NULL to indicate
> a virtual master, which is not true since we decide to use
> the common iommu api to attach device in DRM.
>
> With this patch, we can probe a virtual iommu device and
> allow the DRM attaching to it. The virtual iommu is needed also
> because we want convert to use DMA API for map/unmap, cache flush,
> so that DRM buffer alloc still work even VOP is disabled.

I'm not really convinced that this is a good idea. This will require
creating fake devices in the system and generally looks really hacky.
Please see my alternative proposal inline.

>
> Signed-off-by: Shunqian Zheng 
> ---
>  drivers/iommu/rockchip-iommu.c | 37 +
>  1 file changed, 25 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
> index 3c16ec3..d6c3051 100644
> --- a/drivers/iommu/rockchip-iommu.c
> +++ b/drivers/iommu/rockchip-iommu.c
> @@ -75,6 +75,11 @@
>
>  #define IOMMU_REG_POLL_COUNT_FAST 1000
>
> +/* A virtual iommu in device-tree registered without reg or
> + * interrupts, so the num_mmu is zero.
> + */
> +#define RK_IOMMU_IS_VIRTUAL(iommu) (iommu->num_mmu == 0)
> +
>  struct rk_iommu_domain {
> struct list_head iommus;
> u32 *dt; /* page directory table */
> @@ -789,13 +794,13 @@ static int rk_iommu_attach_device(struct iommu_domain 
> *domain,
> int ret, i;
> phys_addr_t dte_addr;
>
> -   /*
> -* Allow 'virtual devices' (e.g., drm) to attach to domain.
> -* Such a device does not belong to an iommu group.
> -*/
> iommu = rk_iommu_from_dev(dev);
> -   if (!iommu)

Could we instead allocate such virtual rk_iommu struct here (dev could
be used as iommu->dev for logging purposes and a fake group could be
allocated too)?

> +
> +   iommu->domain = domain;
> +   if (RK_IOMMU_IS_VIRTUAL(iommu)) {
> +   dev_dbg(dev, "Attach virtual device to iommu domain\n");
> return 0;
> +   }
>
> ret = rk_iommu_enable_stall(iommu);
> if (ret)
> @@ -805,7 +810,6 @@ static int rk_iommu_attach_device(struct iommu_domain 
> *domain,
> if (ret)
> return ret;
>
> -   iommu->domain = domain;
>
> ret = devm_request_irq(iommu->dev, iommu->irq, rk_iommu_irq,
>IRQF_SHARED, dev_name(dev), iommu);
> @@ -842,10 +846,13 @@ static void rk_iommu_detach_device(struct iommu_domain 
> *domain,
> unsigned long flags;
> int i;
>
> -   /* Allow 'virtual devices' (eg drm) to detach from domain */
> iommu = rk_iommu_from_dev(dev);
> -   if (!iommu)
> +
> +   iommu->domain = NULL;

I don't think it's a good idea to set the domain to NULL before
disabling the real IOMMU. It might still trigger an interrupt at this
point and things won't behave correctly. I guess the original line
could be left as is and simply same assignment added under the if
below.

Best regards,
Tomasz

[PATCH v2 5/7] drm: rockchip: use common iommu api to attach iommu

[Tomasz Figa]

Hi,

On Wed, Jun 8, 2016 at 10:26 PM, Shunqian Zheng  
wrote:
> Rockchip DRM used the arm special API, arm_iommu_*(), to attach
> iommu for ARM32 SoCs. This patch convert to common iommu API
> so it would support ARM64 like RK3399.
>
> The general idea is domain_alloc(), attach_device() and
> arch_setup_dma_ops() to set dma_ops manually for DRM at the last.
>
> Signed-off-by: Shunqian Zheng 
> ---
>  drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 130 
> +++-
>  drivers/gpu/drm/rockchip/rockchip_drm_drv.h |   1 +
>  2 files changed, 89 insertions(+), 42 deletions(-)
>

Please see my comments inline.

> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c 
> b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> index f5a68fc..7965a66 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> @@ -14,8 +14,6 @@
>   * GNU General Public License for more details.
>   */
>
> -#include 
> -
>  #include 
>  #include 
>  #include 
> @@ -24,6 +22,8 @@
>  #include 
>  #include 
>  #include 
> +#include 
> +#include 
>
>  #include "rockchip_drm_drv.h"
>  #include "rockchip_drm_fb.h"
> @@ -46,7 +46,8 @@ static bool is_support_iommu = true;
>  int rockchip_drm_dma_attach_device(struct drm_device *drm_dev,
>struct device *dev)
>  {
> -   struct dma_iommu_mapping *mapping = drm_dev->dev->archdata.mapping;
> +   struct rockchip_drm_private *private = drm_dev->dev_private;
> +   struct iommu_domain *domain = private->domain;
> int ret;
>
> if (!is_support_iommu)
> @@ -58,16 +59,25 @@ int rockchip_drm_dma_attach_device(struct drm_device 
> *drm_dev,
>
> dma_set_max_seg_size(dev, DMA_BIT_MASK(32));
>
> -   return arm_iommu_attach_device(dev, mapping);
> +   ret = iommu_attach_device(domain, dev);
> +

nit: Unnecessary blank line.

> +   if (ret) {
> +   dev_err(dev, "Failed to attach iommu device\n");
> +   return ret;
> +   }

nit: On the other hand, a blank line here would improve readability.

> +   arch_setup_dma_ops(dev, 0x, SZ_2G,
> +  (struct iommu_ops *)dev->bus->iommu_ops, false);

This is casting a const pointer to a non-const pointer. which isn't
really a good idea. I can see that arch_setup_dma_ops() requires a
writable pointer, though. Looking at the implementations of
arch_setup_dma_ops() around the platforms (namely arm and arm64...),
it makes me wonder if the prototype shouldn't be changed to const
instead.

> +   return 0;
>  }
>
>  void rockchip_drm_dma_detach_device(struct drm_device *drm_dev,
> struct device *dev)
>  {
> -   if (!is_support_iommu)
> -   return;
> +   struct rockchip_drm_private *private = drm_dev->dev_private;
> +   struct iommu_domain *domain = private->domain;
>
> -   arm_iommu_detach_device(dev);
> +   if (is_support_iommu)
> +   iommu_detach_device(domain, dev);
>  }
>
>  int rockchip_register_crtc_funcs(struct drm_crtc *crtc,
> @@ -132,10 +142,70 @@ static void rockchip_drm_crtc_disable_vblank(struct 
> drm_device *dev,
> priv->crtc_funcs[pipe]->disable_vblank(crtc);
>  }
>
> +static int rockchip_drm_init_iommu(struct drm_device *drm_dev)
> +{
> +   struct rockchip_drm_private *private = drm_dev->dev_private;
> +   struct device *dev = drm_dev->dev;
> +   int ret;
> +
> +   dev->dma_parms = devm_kzalloc(dev, sizeof(*dev->dma_parms),
> + GFP_KERNEL);
> +   if (!dev->dma_parms) {
> +   ret = -ENOMEM;
> +   return ret;

nit: return -ENOMEM;

> +   }
> +
> +   ret = dma_set_coherent_mask(dev, DMA_BIT_MASK(32));
> +   if (ret) {
> +   dev_err(dev, "Failed to set coherent mask\n");
> +   return ret;
> +   }
> +
> +   dma_set_max_seg_size(dev, DMA_BIT_MASK(32));
> +
> +   private->domain = iommu_domain_alloc(_bus_type);
> +   if (!private->domain)
> +   return -ENOMEM;
> +
> +   ret = iommu_get_dma_cookie(private->domain);
> +   if (ret) {
> +   dev_err(dev, "Failed to get dma cookie\n");
> +   goto err_free_domain;
> +   }
> +
> +   ret = iommu_dma_init_domain(private->domain, 0x, SZ_2G);

I guess djkurtz's TODO comment could be preserved here.

Best regards,
Tomasz

[PATCH v2 7/7] iommu/rockchip: enable rockchip iommu on ARM64 platform

[Tomasz Figa]

Hi,

On Wed, Jun 8, 2016 at 10:26 PM, Shunqian Zheng  
wrote:
> From: Simon Xue 
>

It is usual a good practice to include at least a single sentence
here, even if the patch is trivial. In this case it could say

"This patch makes it possible to compile the rockchip-iommu driver on
ARM64 platform to be used with 64-bit SoCs equipped with this type of
IOMMU."

> Signed-off-by: Simon Xue 
> Signed-off-by: Shunqian Zheng 
> ---
>  drivers/iommu/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>

Assuming that the above is fixed:

Reviewed-by: Tomasz Figa 

Best regards,
Tomasz

[PATCH v2 6/7] iommu/rockchip: use DMA API to map, to flush cache

[Tomasz Figa]

Hi,

On Wed, Jun 8, 2016 at 10:26 PM, Shunqian Zheng  
wrote:
> Use DMA API instead of architecture internal functions like
> __cpuc_flush_dcache_area() etc.
>
> To support the virtual device like DRM the virtual slave iommu
> added in the previous patch, attaching to which the DRM can use
> it own domain->dev for dma_map_*(), dma_sync_*() even VOP is disabled.
>
> With this patch, this driver is available for ARM64 like RK3399.
>

Could we instead simply allocate coherent memory for page tables using
dma_alloc_coherent() and skip any flushing on CPU side completely? If
I'm looking correctly, the driver only reads back the page directory
when checking if there is a need to allocate new page table, so there
shouldn't be any significant penalty for disabling the cache.

Other than that, please see some comments inline.

> Signed-off-by: Shunqian Zheng 
> ---
>  drivers/iommu/rockchip-iommu.c | 113 
> ++---
>  1 file changed, 71 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
> index d6c3051..aafea6e 100644
> --- a/drivers/iommu/rockchip-iommu.c
> +++ b/drivers/iommu/rockchip-iommu.c
> @@ -4,8 +4,6 @@
>   * published by the Free Software Foundation.
>   */
>
> -#include 
> -#include 
>  #include 
>  #include 
>  #include 
> @@ -61,8 +59,7 @@
>  #define RK_MMU_IRQ_BUS_ERROR 0x02  /* bus read error */
>  #define RK_MMU_IRQ_MASK  (RK_MMU_IRQ_PAGE_FAULT | 
> RK_MMU_IRQ_BUS_ERROR)
>
> -#define NUM_DT_ENTRIES 1024
> -#define NUM_PT_ENTRIES 1024
> +#define NUM_TLB_ENTRIES 1024 /* for both DT and PT */

Is it necessary to change this in this patch? In general, it's not a
good idea to mix multiple logical changes together.

>
>  #define SPAGE_ORDER 12
>  #define SPAGE_SIZE (1 << SPAGE_ORDER)
> @@ -82,7 +79,9 @@
>
>  struct rk_iommu_domain {
> struct list_head iommus;
> +   struct device *dev;
> u32 *dt; /* page directory table */
> +   dma_addr_t dt_dma;
> spinlock_t iommus_lock; /* lock for iommus list */
> spinlock_t dt_lock; /* lock for modifying page directory table */
>
> @@ -98,14 +97,12 @@ struct rk_iommu {
> struct iommu_domain *domain; /* domain to which iommu is attached */
>  };
>
> -static inline void rk_table_flush(u32 *va, unsigned int count)
> +static inline void rk_table_flush(struct device *dev, dma_addr_t dma,
> + unsigned int count)
>  {
> -   phys_addr_t pa_start = virt_to_phys(va);
> -   phys_addr_t pa_end = virt_to_phys(va + count);
> -   size_t size = pa_end - pa_start;
> +   size_t size = count * 4;

It would be a good idea to specify what "count" is. I'm a bit confused
that before it meant bytes and now some multiple of 4?

Best regards,
Tomasz

[PATCH v2 6/7] iommu/rockchip: use DMA API to map, to flush cache

[Tomasz Figa]

On Mon, Jun 13, 2016 at 6:56 PM, Shunqian Zheng
 wrote:
> Hi
>
> On 2016年06月10日 17:10, Tomasz Figa wrote:
>>
>> Hi,
>>
>> On Wed, Jun 8, 2016 at 10:26 PM, Shunqian Zheng 
>> wrote:
>>>
>>> Use DMA API instead of architecture internal functions like
>>> __cpuc_flush_dcache_area() etc.
>>>
>>> To support the virtual device like DRM the virtual slave iommu
>>> added in the previous patch, attaching to which the DRM can use
>>> it own domain->dev for dma_map_*(), dma_sync_*() even VOP is disabled.
>>>
>>> With this patch, this driver is available for ARM64 like RK3399.
>>>
>> Could we instead simply allocate coherent memory for page tables using
>> dma_alloc_coherent() and skip any flushing on CPU side completely? If
>> I'm looking correctly, the driver only reads back the page directory
>> when checking if there is a need to allocate new page table, so there
>> shouldn't be any significant penalty for disabling the cache.
>
> I try to use dma_alloc_coherent() to replace the dma_map_single(),
> but it doesn't work for me properly.
> Because the DRM uses the iommu_dma_ops instead the swiotlb_dma_ops after
> attaching
> to iommu, so when the iommu domain need to alloc a new page in
> rk_iommu_map(),
> it would call:
> rk_iommu_map()  --> dma_alloc_coherent()  --> ops->alloc()  --> iommu_map()
> --> rk_iommu_map()

It shouldn't call iommu_map(), because the IOMMU is not behind another
IOMMU. Are you sure you called dma_alloc_coherent() on behalf of the
IOMMU struct device and not the DRM device?

Best regards,
Tomasz

[PATCH v2 6/7] iommu/rockchip: use DMA API to map, to flush cache

[Tomasz Figa]

On Mon, Jun 13, 2016 at 7:31 PM, Shunqian Zheng
 wrote:
> HI,
>
>
> On 2016年06月13日 18:21, Tomasz Figa wrote:
>>
>> On Mon, Jun 13, 2016 at 6:56 PM, Shunqian Zheng
>>  wrote:
>>>
>>> Hi
>>>
>>> On 2016年06月10日 17:10, Tomasz Figa wrote:
>>>>
>>>> Hi,
>>>>
>>>> On Wed, Jun 8, 2016 at 10:26 PM, Shunqian Zheng 
>>>> wrote:
>>>>>
>>>>> Use DMA API instead of architecture internal functions like
>>>>> __cpuc_flush_dcache_area() etc.
>>>>>
>>>>> To support the virtual device like DRM the virtual slave iommu
>>>>> added in the previous patch, attaching to which the DRM can use
>>>>> it own domain->dev for dma_map_*(), dma_sync_*() even VOP is disabled.
>>>>>
>>>>> With this patch, this driver is available for ARM64 like RK3399.
>>>>>
>>>> Could we instead simply allocate coherent memory for page tables using
>>>> dma_alloc_coherent() and skip any flushing on CPU side completely? If
>>>> I'm looking correctly, the driver only reads back the page directory
>>>> when checking if there is a need to allocate new page table, so there
>>>> shouldn't be any significant penalty for disabling the cache.
>>>
>>> I try to use dma_alloc_coherent() to replace the dma_map_single(),
>>> but it doesn't work for me properly.
>>> Because the DRM uses the iommu_dma_ops instead the swiotlb_dma_ops after
>>> attaching
>>> to iommu, so when the iommu domain need to alloc a new page in
>>> rk_iommu_map(),
>>> it would call:
>>> rk_iommu_map()  --> dma_alloc_coherent()  --> ops->alloc()  -->
>>> iommu_map()
>>> --> rk_iommu_map()
>>
>> It shouldn't call iommu_map(), because the IOMMU is not behind another
>> IOMMU. Are you sure you called dma_alloc_coherent() on behalf of the
>> IOMMU struct device and not the DRM device?
>
> I called dma_alloc_coherent() with DRM device but not the IOMMU device,
> because DRM didn't attach to any iommu. Even allocating an virtual one when
> attaching, the iommu->dev
> is DRM device though.
> Am I right here?

What I meant, is that even though rk_iommu_map() is called for DRM
device, the page table allocation happening inside should be called
for the IOMMU device itself, because it's the consumer of these page
tables.

Best regards,
Tomasz

[PATCH v3 05/10] drm/rockchip: analogix_dp: add rk3399 eDP support

[Tomasz Figa]

Hi Yakir,

Yakir Yang  rock-chips.com> writes:
> >>   Required properties:
> >> -- compatible: "rockchip,rk3288-edp";
> >> +- compatible: "rockchip,rk3288-edp",
> >> + "rockchip,rk3399-edp";
> > As commented by Tomasz on gerrit, there is a pre-existing typo here.
> > Specifically "rockchip,rk3288-edp" should be "rockchip,rk3288-dp".
> >
> > The typo is pre-existing so I'm not sure you would need to spin this
> > series to fix it, but if you were spinning it anyway it wouldn't hurt
> > to fix.  Could also just send out a followon patch to fix this after
> > the series lands...
> 
> My bad, forget to fix that :(
> it would be better to merge that fix into this series, would send a 
> follow patch soon.

No need to respin the whole series just for this typo. It's better to fix it 
in a separate patch later at this point, especially since this revision of 
the series looks quite good.

Best regards,
Tomasz

[PATCH v3 0/10]

[Tomasz Figa]

Hi Yakir,

Yakir Yang  rock-chips.com> writes:
> 
> RK3399 and RK3288 shared the same eDP IP controller, only some light
> difference with VOP configure and GRF configure.
> 
> Also same misc fix to analogix_dp driver:
> - Hotplug invalid which report by Dan Carpenter
> - Make panel detect to an optional action
> - correct the register bit define error in ANALOGIX_DP_PLL_REG_1

This version looks good to me. For all patches in the series:

Reviewed-by: Tomasz Figa 

Best regards,
Tomasz

[PATCH v3 3/6] iommu/rockchip: support virtual iommu slave device

[Tomasz Figa]

Hi Shunqian,

On Wed, Jun 15, 2016 at 9:04 PM, Shunqian Zheng  
wrote:
> An virtual master device like DRM need to attach to iommu
> domain to share the mapping with VOPs(the one with actual
> iommu slaves).
> DRM attaches to iommu and allocates buffers before VOPs enabled,
> which means there may have not real iommu devices can be used
> to do dma mapping.
>
> This patch creates a iommu when virtual master(group is NULL)
> attaching, so it can use this iommu even the real iommus disabled.
>
> Changes of V3:
> - Instead of registering virtual iommu in DTS, this patch
>   creates a iommu when attaching.
>
> Signed-off-by: Shunqian Zheng 
> Suggested-by: Tomasz Figa 

To clarify, I don't really like the idea of virtual IOMMU, however it
is registered, dts or manually, but I don't think there is any other
reasonable way of dealing with allocations for subsystems such as DRM,
where there are multiple devices in one IOMMU domain. Having said
that, please see some minor comments inline.

> ---
>  drivers/iommu/rockchip-iommu.c | 133 
> +
>  1 file changed, 122 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
> index 3c16ec3..82ecc99 100644
> --- a/drivers/iommu/rockchip-iommu.c
> +++ b/drivers/iommu/rockchip-iommu.c
[snip]
> @@ -878,6 +975,9 @@ static struct iommu_domain 
> *rk_iommu_domain_alloc(unsigned type)
> if (!rk_domain)
> return NULL;
>
> +   if (iommu_get_dma_cookie(_domain->domain))
> +   goto err_dt;
> +

Shouldn't this belong to a separate patch? Actually, is this required?
If so, how this worked before?

> /*
>  * rk32xx iommus use a 2 level pagetable.
>  * Each level1 (dt) and level2 (pt) table has 1024 4-byte entries.
> @@ -917,6 +1017,9 @@ static void rk_iommu_domain_free(struct iommu_domain 
> *domain)
> }
>
> free_page((unsigned long)rk_domain->dt);
> +
> +   iommu_put_dma_cookie(_domain->domain);
> +

See above.

Otherwise, with the above addressed, you can add my Reviewed-by.

Best regards,
Tomasz

[PATCH v3 4/6] drm: rockchip: use common iommu api to attach iommu

[Tomasz Figa]

Hi Shunqian,

On Wed, Jun 15, 2016 at 9:04 PM, Shunqian Zheng  
wrote:
> Rockchip DRM used the arm special API, arm_iommu_*(), to attach
> iommu for ARM32 SoCs. This patch convert to common iommu API
> so it would support ARM64 like RK3399.
>
> The general idea is domain_alloc(), attach_device() and
> arch_setup_dma_ops() to set dma_ops manually for DRM at the last.
>
> Signed-off-by: Shunqian Zheng 
> ---
>  drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 112 
> +---
>  drivers/gpu/drm/rockchip/rockchip_drm_drv.h |   1 +
>  2 files changed, 71 insertions(+), 42 deletions(-)

Please see my comment inline.

> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c 
> b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> index f5a68fc..b52c38d 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> @@ -14,16 +14,16 @@
>   * GNU General Public License for more details.
>   */
>
> -#include 
> -
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
>  #include 
>  #include 
> +#include 
>
>  #include "rockchip_drm_drv.h"
>  #include "rockchip_drm_fb.h"
> @@ -46,7 +46,8 @@ static bool is_support_iommu = true;
>  int rockchip_drm_dma_attach_device(struct drm_device *drm_dev,
>struct device *dev)
>  {
> -   struct dma_iommu_mapping *mapping = drm_dev->dev->archdata.mapping;
> +   struct rockchip_drm_private *private = drm_dev->dev_private;
> +   struct iommu_domain *domain = private->domain;
> int ret;
>
> if (!is_support_iommu)
> @@ -58,16 +59,25 @@ int rockchip_drm_dma_attach_device(struct drm_device 
> *drm_dev,
>
> dma_set_max_seg_size(dev, DMA_BIT_MASK(32));
>
> -   return arm_iommu_attach_device(dev, mapping);
> +   ret = iommu_attach_device(domain, dev);
> +   if (ret) {
> +   dev_err(dev, "Failed to attach iommu device\n");
> +   return ret;
> +   }
> +
> +   /* TODO(djkurtz): fetch the mapping start/size from somewhere */
> +   arch_setup_dma_ops(dev, 0x, SZ_2G, dev->bus->iommu_ops, 
> false);
> +   return 0;
>  }
>
>  void rockchip_drm_dma_detach_device(struct drm_device *drm_dev,
> struct device *dev)
>  {
> -   if (!is_support_iommu)
> -   return;
> +   struct rockchip_drm_private *private = drm_dev->dev_private;
> +   struct iommu_domain *domain = private->domain;
>
> -   arm_iommu_detach_device(dev);
> +   if (is_support_iommu)
> +   iommu_detach_device(domain, dev);
>  }
>
>  int rockchip_register_crtc_funcs(struct drm_crtc *crtc,
> @@ -132,10 +142,52 @@ static void rockchip_drm_crtc_disable_vblank(struct 
> drm_device *dev,
> priv->crtc_funcs[pipe]->disable_vblank(crtc);
>  }
>
> +static int rockchip_drm_init_iommu(struct drm_device *drm_dev)
> +{
> +   struct rockchip_drm_private *private = drm_dev->dev_private;
> +   struct device *dev = drm_dev->dev;
> +   int ret;
> +
> +   dev->dma_parms = devm_kzalloc(dev, sizeof(*dev->dma_parms),
> + GFP_KERNEL);
> +   if (!dev->dma_parms)
> +   return -ENOMEM;
> +
> +   private->domain = iommu_domain_alloc(_bus_type);
> +   if (!private->domain)
> +   return -ENOMEM;
> +
> +   /* TODO(djkurtz): fetch the mapping start/size from somewhere */
> +   ret = iommu_dma_init_domain(private->domain, 0x, SZ_2G);
> +   if (ret) {
> +   dev_err(dev, "Failed to init domain\n");
> +   goto err_free_domain;
> +   }
> +
> +   ret = rockchip_drm_dma_attach_device(drm_dev, dev);
> +   if (ret) {
> +   dev_err(dev, "Failed to attach device\n");
> +   goto err_free_domain;
> +   }
> +
> +   return 0;
> +
> +err_free_domain:
> +   iommu_domain_free(private->domain);
> +
> +   return ret;
> +}
> +
> +static void rockchip_iommu_cleanup(struct drm_device *drm_dev)
> +{
> +   struct rockchip_drm_private *private = drm_dev->dev_private;
> +

No need to call rockchip_drm_dma_detach_device() here?

> +   iommu_domain_free(private->domain);
> +}

Otherwise looks good, so after addressing the above, feel free to add
my Reviewed-by.

Best regards,
Tomasz

[PATCH v3 5/6] iommu/rockchip: use DMA API to map, to flush cache

[Tomasz Figa]

Hi Shunqian,

On Wed, Jun 15, 2016 at 9:04 PM, Shunqian Zheng  
wrote:
> Use DMA API instead of architecture internal functions like
> __cpuc_flush_dcache_area() etc.
>
> To support the virtual device like DRM the virtual slave iommu
> added in the previous patch, attaching to which the DRM can use
> it own domain->dev for dma_map_*(), dma_sync_*() even VOP is disabled.
>
> With this patch, this driver is available for ARM64 like RK3399.
>
> Signed-off-by: Shunqian Zheng 
> ---
>  drivers/iommu/rockchip-iommu.c | 113 
> +++--
>  1 file changed, 76 insertions(+), 37 deletions(-)

In general looks good to me, but still have some concern about
attaching and detaching. Please see inline.

> diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
> index 82ecc99..b60b29e 100644
> --- a/drivers/iommu/rockchip-iommu.c
> +++ b/drivers/iommu/rockchip-iommu.c
[snip]
> @@ -886,6 +901,22 @@ static int rk_iommu_attach_device(struct iommu_domain 
> *domain,
> return -ENOMEM;
> }
>
> +   /* Set the domain dev to virtual one if exist */
> +   if (rk_iommu_is_virtual(iommu) || !rk_domain->dev)

Does it matter if the iommu is virtual or not? This condition will
always evaluate to true on first attach anyway, so there might be
times when rk_domain->dev points to a real device.

> +   rk_domain->dev = iommu->dev;
> +
[snip]
>  static void rk_iommu_detach_device(struct iommu_domain *domain,
> @@ -987,8 +1023,6 @@ static struct iommu_domain 
> *rk_iommu_domain_alloc(unsigned type)
> if (!rk_domain->dt)
> goto err_dt;
>
> -   rk_table_flush(rk_domain->dt, NUM_DT_ENTRIES);
> -
> spin_lock_init(_domain->iommus_lock);
> spin_lock_init(_domain->dt_lock);
> INIT_LIST_HEAD(_domain->iommus);
> @@ -1012,10 +1046,15 @@ static void rk_iommu_domain_free(struct iommu_domain 
> *domain)
> if (rk_dte_is_pt_valid(dte)) {
> phys_addr_t pt_phys = rk_dte_pt_address(dte);
> u32 *page_table = phys_to_virt(pt_phys);
> +   dma_unmap_single(rk_domain->dev, pt_phys,
> +SPAGE_SIZE, DMA_TO_DEVICE);
> free_page((unsigned long)page_table);
> }
> }
>
> +   if (!rk_domain->dt_dma)
> +   dma_unmap_single(rk_domain->dev, rk_domain->dt_dma,
> +SPAGE_SIZE, DMA_TO_DEVICE);
> free_page((unsigned long)rk_domain->dt);
>
> iommu_put_dma_cookie(_domain->domain);

If we detach here a device whose IOMMU is currently pointed by
rk_domain->dev, then the pointer will not point to anything valid
anymore. To be honest, I don't see any good solution for this. Maybe
we should keep all the IOMMUs in a list and set the ->dev pointer to
any from the list here?

Best regards,
Tomasz

[PATCH] drm/rockchip: Finish initialization before registering DRM device

[Tomasz Figa]

Currently the driver calls drm_dev_register() directly after allocating
the DRM device and then continues with further initialization. This is
incorrect, because drm_dev_register() is supposed to be called after all
initialization is done. This problem was masked by the fact that
drm_dev_register() did not use to do anything special before, but
recently it started to call drm_connector_register_all(), which leads to
a crash if the driver is not fully initialized.

This patch fixes the problem by moving the call to drm_dev_register() to
the end of the initialization sequence and also removing the, now
unnecessary, call to drm_connector_register_all() from driver code.

Fixes: f706974a69b6 ("drm/rockchip: Drop drm_driver.load/unload callbacks")

Signed-off-by: Tomasz Figa 
---
 drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 18 +++---
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c 
b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
index 0d28455..e2c31d3 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
@@ -146,16 +146,12 @@ static int rockchip_drm_bind(struct device *dev)
if (!drm_dev)
return -ENOMEM;

-   ret = drm_dev_register(drm_dev, 0);
-   if (ret)
-   goto err_free;
-
dev_set_drvdata(dev, drm_dev);

private = devm_kzalloc(drm_dev->dev, sizeof(*private), GFP_KERNEL);
if (!private) {
ret = -ENOMEM;
-   goto err_unregister;
+   goto err_free;
}

drm_dev->dev_private = private;
@@ -197,12 +193,6 @@ static int rockchip_drm_bind(struct device *dev)
if (ret)
goto err_detach_device;

-   ret = drm_connector_register_all(drm_dev);
-   if (ret) {
-   dev_err(dev, "failed to register connectors\n");
-   goto err_unbind;
-   }
-
/* init kms poll for handling hpd */
drm_kms_helper_poll_init(drm_dev);

@@ -222,9 +212,15 @@ static int rockchip_drm_bind(struct device *dev)
if (ret)
goto err_vblank_cleanup;

+   ret = drm_dev_register(drm_dev, 0);
+   if (ret)
+   goto err_fbdev_fini;
+
if (is_support_iommu)
arm_iommu_release_mapping(mapping);
return 0;
+err_fbdev_fini:
+   rockchip_drm_fbdev_fini(drm_dev);
 err_vblank_cleanup:
drm_vblank_cleanup(drm_dev);
 err_kms_helper_poll_fini:
-- 
2.8.0.rc3.226.g39d4020

[PATCH v4 0/8] iommu/rockchip: Fix bugs and enable on ARM64

[Tomasz Figa]

This series intends mostly to enable support for ARM64 architecture
in the rockchip-iommu driver. On the way to do so, some bugs are also
fixed.

The most important changes here are:
 - making the Rockchip IOMMU driver use DMA API for managing cache
   coherency of page tables,
 - making the Rockchip DRM driver not use DMA API on behalf of a virtual
   device (behind a virtual IOMMU) to allocate and map buffers, but
   instead proper DRM helpers and IOMMU API directly.

Changes since v3:
 - Drop the idea of virtual IOMMU. Instead replace hacky allocation code
   in DRM driver, with proper management of IOMMU domain.
 - Add one more fix for allocation of IOMMU register base addresses.
Changes since v2:
 - Instead of registering virtual IOMMU from DTS, create it when
   attaching.
 - Fix some bugs found in internal review.

Shunqian Zheng (4):
  iommu/rockchip: Fix allocation of bases array in driver probe
  iommu/rockchip: Use DMA API to manage coherency
  iommu/rockchip: Prepare to support generic DMA mapping
  drm/rockchip: Use common IOMMU API to attach devices

Simon Xue (3):
  iommu/rockchip: Fix devm_{request,free}_irq parameter
  iommu/rockchip: Add map_sg callback for rk_iommu_ops
  iommu/rockchip: Enable Rockchip IOMMU on ARM64

Tomasz Figa (1):
  drm/rockchip: Do not use DMA mapping API if attached to IOMMU domain

 drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 100 ++--
 drivers/gpu/drm/rockchip/rockchip_drm_drv.h |   3 +
 drivers/gpu/drm/rockchip/rockchip_drm_gem.c | 236 ++--
 drivers/gpu/drm/rockchip/rockchip_drm_gem.h |   9 ++
 drivers/iommu/Kconfig   |   2 +-
 drivers/iommu/rockchip-iommu.c  | 180 +++--
 6 files changed, 427 insertions(+), 103 deletions(-)

-- 
2.8.0.rc3.226.g39d4020

[PATCH v4 1/8] iommu/rockchip: Fix devm_{request,free}_irq parameter

[Tomasz Figa]

From: Simon Xue <x...@rock-chips.com>

Even though the IOMMU shares IRQ with its master, the struct device
passed to {request,free}_irq is supposed to represent the device that is
signalling the interrupt. This patch makes the driver use IOMMU device
instead of master's device to make things clear.

Signed-off-by: Simon Xue 
Signed-off-by: Shunqian Zheng 
Reviewed-on: https://chromium-review.googlesource.com/346325
Reviewed-by: Douglas Anderson 
Signed-off-by: Tomasz Figa 
---
 drivers/iommu/rockchip-iommu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
index 25b4627..5a9659a 100644
--- a/drivers/iommu/rockchip-iommu.c
+++ b/drivers/iommu/rockchip-iommu.c
@@ -807,7 +807,7 @@ static int rk_iommu_attach_device(struct iommu_domain 
*domain,

iommu->domain = domain;

-   ret = devm_request_irq(dev, iommu->irq, rk_iommu_irq,
+   ret = devm_request_irq(iommu->dev, iommu->irq, rk_iommu_irq,
   IRQF_SHARED, dev_name(dev), iommu);
if (ret)
return ret;
@@ -860,7 +860,7 @@ static void rk_iommu_detach_device(struct iommu_domain 
*domain,
}
rk_iommu_disable_stall(iommu);

-   devm_free_irq(dev, iommu->irq, iommu);
+   devm_free_irq(iommu->dev, iommu->irq, iommu);

iommu->domain = NULL;

-- 
2.8.0.rc3.226.g39d4020

[PATCH v4 3/8] iommu/rockchip: Fix allocation of bases array in driver probe

[Tomasz Figa]

From: Shunqian Zheng <zhen...@rock-chips.com>

In .probe(), devm_kzalloc() is called with size == 0 and works only
by luck, due to internal behavior of the allocator and the fact
that the proper allocation size is small. Let's use proper value for
calculating the size.

Fixes: cd6438c5f844 ("iommu/rockchip: Reconstruct to support multi slaves")

Signed-off-by: Shunqian Zheng 
Signed-off-by: Tomasz Figa 
---
 drivers/iommu/rockchip-iommu.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
index 53fa0d9..8a5bac7 100644
--- a/drivers/iommu/rockchip-iommu.c
+++ b/drivers/iommu/rockchip-iommu.c
@@ -1034,6 +1034,7 @@ static int rk_iommu_probe(struct platform_device *pdev)
struct device *dev = >dev;
struct rk_iommu *iommu;
struct resource *res;
+   int num_res = pdev->num_resources;
int i;

iommu = devm_kzalloc(dev, sizeof(*iommu), GFP_KERNEL);
@@ -1043,12 +1044,13 @@ static int rk_iommu_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, iommu);
iommu->dev = dev;
iommu->num_mmu = 0;
-   iommu->bases = devm_kzalloc(dev, sizeof(*iommu->bases) * iommu->num_mmu,
+
+   iommu->bases = devm_kzalloc(dev, sizeof(*iommu->bases) * num_res,
GFP_KERNEL);
if (!iommu->bases)
return -ENOMEM;

-   for (i = 0; i < pdev->num_resources; i++) {
+   for (i = 0; i < num_res; i++) {
res = platform_get_resource(pdev, IORESOURCE_MEM, i);
if (!res)
continue;
-- 
2.8.0.rc3.226.g39d4020

[PATCH v4 6/8] drm/rockchip: Do not use DMA mapping API if attached to IOMMU domain

[Tomasz Figa]

The API is not suitable for subsystems consisting of multiple devices
and requires severe hacks to use it. To mitigate this, this patch
implements allocation and address space management locally by using
helpers provided by DRM framework, like other DRM drivers do, e.g.
Tegra.

This patch should not introduce any functional changes until the driver
is made to attach subdevices into an IOMMU domain with the generic IOMMU
API, which will happen in following patch. Based heavily on GEM
implementation of Tegra DRM driver.

Signed-off-by: Tomasz Figa 
---
 drivers/gpu/drm/rockchip/rockchip_drm_drv.h |   3 +
 drivers/gpu/drm/rockchip/rockchip_drm_gem.c | 236 ++--
 drivers/gpu/drm/rockchip/rockchip_drm_gem.h |   9 ++
 3 files changed, 237 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h 
b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
index ea39329..5ab1223 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
@@ -30,6 +30,7 @@

 struct drm_device;
 struct drm_connector;
+struct iommu_domain;

 /*
  * Rockchip drm private crtc funcs.
@@ -61,6 +62,8 @@ struct rockchip_drm_private {
struct drm_gem_object *fbdev_bo;
const struct rockchip_crtc_funcs *crtc_funcs[ROCKCHIP_MAX_CRTC];
struct drm_atomic_state *state;
+   struct iommu_domain *domain;
+   struct drm_mm mm;
 };

 int rockchip_register_crtc_funcs(struct drm_crtc *crtc,
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c 
b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
index 059e902..fc0e6c1 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
@@ -18,11 +18,147 @@
 #include 

 #include 
+#include 

 #include "rockchip_drm_drv.h"
 #include "rockchip_drm_gem.h"

-static int rockchip_gem_alloc_buf(struct rockchip_gem_object *rk_obj,
+static int rockchip_gem_iommu_map(struct rockchip_drm_private *private,
+ struct rockchip_gem_object *rk_obj)
+{
+   int prot = IOMMU_READ | IOMMU_WRITE;
+   ssize_t ret;
+
+   if (rk_obj->mm)
+   return -EBUSY;
+
+   rk_obj->mm = kzalloc(sizeof(*rk_obj->mm), GFP_KERNEL);
+   if (!rk_obj->mm)
+   return -ENOMEM;
+
+   ret = drm_mm_insert_node_generic(>mm, rk_obj->mm,
+rk_obj->base.size, PAGE_SIZE,
+0, 0, 0);
+   if (ret < 0) {
+   DRM_ERROR("out of I/O virtual memory: %zd\n", ret);
+   goto err_free_mm;
+   }
+
+   rk_obj->dma_addr = rk_obj->mm->start;
+
+   ret = iommu_map_sg(private->domain, rk_obj->dma_addr, rk_obj->sgt->sgl,
+  rk_obj->sgt->nents, prot);
+   if (ret < 0) {
+   DRM_ERROR("failed to map buffer: %zd\n", ret);
+   goto err_remove_node;
+   }
+
+   rk_obj->size = ret;
+
+   return 0;
+
+err_remove_node:
+   drm_mm_remove_node(rk_obj->mm);
+err_free_mm:
+   kfree(rk_obj->mm);
+   return ret;
+}
+
+static int rockchip_gem_iommu_unmap(struct rockchip_drm_private *private,
+   struct rockchip_gem_object *rk_obj)
+{
+   if (!rk_obj->mm)
+   return 0;
+
+   iommu_unmap(private->domain, rk_obj->dma_addr, rk_obj->size);
+   drm_mm_remove_node(rk_obj->mm);
+   kfree(rk_obj->mm);
+
+   return 0;
+}
+
+static int rockchip_gem_get_pages(struct drm_device *drm,
+ struct rockchip_gem_object *rk_obj)
+{
+   int ret, i;
+   struct scatterlist *s;
+
+   rk_obj->pages = drm_gem_get_pages(_obj->base);
+   if (IS_ERR(rk_obj->pages))
+   return PTR_ERR(rk_obj->pages);
+
+   rk_obj->num_pages = rk_obj->base.size >> PAGE_SHIFT;
+
+   rk_obj->sgt = drm_prime_pages_to_sg(rk_obj->pages, rk_obj->num_pages);
+   if (IS_ERR(rk_obj->sgt)) {
+   ret = PTR_ERR(rk_obj->sgt);
+   goto err_put_pages;
+   }
+
+   /*
+* Fake up the SG table so that dma_sync_sg_for_device() can be used
+* to flush the pages associated with it.
+*
+* TODO: Replace this by drm_clflash_sg() once it can be implemented
+* without relying on symbols that are not exported.
+*/
+   for_each_sg(rk_obj->sgt->sgl, s, rk_obj->sgt->nents, i)
+   sg_dma_address(s) = sg_phys(s);
+
+   dma_sync_sg_for_device(drm->dev, rk_obj->sgt->sgl, rk_obj->sgt->nents,
+  DMA_TO_DEVICE);
+
+   return 0;
+
+err_put_pages:
+   drm_gem_put_pages(_obj->base, rk_obj->pages, false, false);
+   return ret;
+}
+
+static void rockchip_gem_put_pages(struct drm_device *drm,
+  

[PATCH v4 4/8] iommu/rockchip: Use DMA API to manage coherency

[Tomasz Figa]

From: Shunqian Zheng <zhen...@rock-chips.com>

Use DMA API instead of architecture internal functions like
__cpuc_flush_dcache_area() etc.

The biggest difficulty here is that dma_map and _sync calls require some
struct device, while there is no real 1:1 relation between an IOMMU
domain and some device. To overcome this, a simple platform device is
registered for each allocated IOMMU domain.

With this patch, this driver can be used on both ARM and ARM64
platforms, such as RK3288 and RK3399 respectively.

Signed-off-by: Shunqian Zheng 
Signed-off-by: Tomasz Figa 
---
 drivers/iommu/rockchip-iommu.c | 161 +++--
 1 file changed, 122 insertions(+), 39 deletions(-)

diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
index 8a5bac7..0551146 100644
--- a/drivers/iommu/rockchip-iommu.c
+++ b/drivers/iommu/rockchip-iommu.c
@@ -4,11 +4,10 @@
  * published by the Free Software Foundation.
  */

-#include 
-#include 
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -77,7 +76,9 @@

 struct rk_iommu_domain {
struct list_head iommus;
+   struct platform_device *pdev;
u32 *dt; /* page directory table */
+   dma_addr_t dt_dma;
spinlock_t iommus_lock; /* lock for iommus list */
spinlock_t dt_lock; /* lock for modifying page directory table */

@@ -93,14 +94,12 @@ struct rk_iommu {
struct iommu_domain *domain; /* domain to which iommu is attached */
 };

-static inline void rk_table_flush(u32 *va, unsigned int count)
+static inline void rk_table_flush(struct rk_iommu_domain *dom, dma_addr_t dma,
+ unsigned int count)
 {
-   phys_addr_t pa_start = virt_to_phys(va);
-   phys_addr_t pa_end = virt_to_phys(va + count);
-   size_t size = pa_end - pa_start;
+   size_t size = count * 4; /* count of entry, 4 bytes per entry */

-   __cpuc_flush_dcache_area(va, size);
-   outer_flush_range(pa_start, pa_end);
+   dma_sync_single_for_device(>pdev->dev, dma, size, DMA_TO_DEVICE);
 }

 static struct rk_iommu_domain *to_rk_domain(struct iommu_domain *dom)
@@ -183,10 +182,9 @@ static inline bool rk_dte_is_pt_valid(u32 dte)
return dte & RK_DTE_PT_VALID;
 }

-static u32 rk_mk_dte(u32 *pt)
+static inline u32 rk_mk_dte(dma_addr_t pt_dma)
 {
-   phys_addr_t pt_phys = virt_to_phys(pt);
-   return (pt_phys & RK_DTE_PT_ADDRESS_MASK) | RK_DTE_PT_VALID;
+   return (pt_dma & RK_DTE_PT_ADDRESS_MASK) | RK_DTE_PT_VALID;
 }

 /*
@@ -603,13 +601,16 @@ static void rk_iommu_zap_iova_first_last(struct 
rk_iommu_domain *rk_domain,
 static u32 *rk_dte_get_page_table(struct rk_iommu_domain *rk_domain,
  dma_addr_t iova)
 {
+   struct device *dev = _domain->pdev->dev;
u32 *page_table, *dte_addr;
-   u32 dte;
+   u32 dte_index, dte;
phys_addr_t pt_phys;
+   dma_addr_t pt_dma;

assert_spin_locked(_domain->dt_lock);

-   dte_addr = _domain->dt[rk_iova_dte_index(iova)];
+   dte_index = rk_iova_dte_index(iova);
+   dte_addr = _domain->dt[dte_index];
dte = *dte_addr;
if (rk_dte_is_pt_valid(dte))
goto done;
@@ -618,19 +619,26 @@ static u32 *rk_dte_get_page_table(struct rk_iommu_domain 
*rk_domain,
if (!page_table)
return ERR_PTR(-ENOMEM);

-   dte = rk_mk_dte(page_table);
-   *dte_addr = dte;
+   pt_dma = dma_map_single(dev, page_table, SPAGE_SIZE, DMA_TO_DEVICE);
+   if (dma_mapping_error(dev, pt_dma)) {
+   dev_err(dev, "DMA mapping error while allocating page table\n");
+   free_page((unsigned long)page_table);
+   return ERR_PTR(-ENOMEM);
+   }

-   rk_table_flush(page_table, NUM_PT_ENTRIES);
-   rk_table_flush(dte_addr, 1);
+   dte = rk_mk_dte(pt_dma);
+   *dte_addr = dte;

+   rk_table_flush(rk_domain, pt_dma, NUM_PT_ENTRIES);
+   rk_table_flush(rk_domain, rk_domain->dt_dma + dte_index * 4, 1);
 done:
pt_phys = rk_dte_pt_address(dte);
return (u32 *)phys_to_virt(pt_phys);
 }

 static size_t rk_iommu_unmap_iova(struct rk_iommu_domain *rk_domain,
- u32 *pte_addr, dma_addr_t iova, size_t size)
+ u32 *pte_addr, dma_addr_t pte_dma,
+ size_t size)
 {
unsigned int pte_count;
unsigned int pte_total = size / SPAGE_SIZE;
@@ -645,14 +653,14 @@ static size_t rk_iommu_unmap_iova(struct rk_iommu_domain 
*rk_domain,
pte_addr[pte_count] = rk_mk_pte_invalid(pte);
}

-   rk_table_flush(pte_addr, pte_count);
+   rk_table_flush(rk_domain, pte_dma, pte_count);

return pte_count * SPAGE_SIZE;
 }

 static int rk_iommu_map_iova(struct rk_iommu_domain *rk_domain, u32 *pte_addr,
-dma_addr_t iova, phys_addr_t paddr

[PATCH v4 7/8] drm/rockchip: Use common IOMMU API to attach devices

[Tomasz Figa]

From: Shunqian Zheng <zhen...@rock-chips.com>

Rockchip DRM used the arm special API, arm_iommu_*(), to attach
iommu for ARM32 SoCs. This patch convert to common iommu API
so it would support ARM64 like RK3399.

Since previous patch added support for direct IOMMU address space
management, there is no need to use DMA API anymore and this patch wires
things to use the new method.

Signed-off-by: Shunqian Zheng 
Signed-off-by: Tomasz Figa 
---
 drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 100 +++-
 1 file changed, 53 insertions(+), 47 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c 
b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
index e2c31d3..2793ac9 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
@@ -14,18 +14,18 @@
  * GNU General Public License for more details.
  */

-#include 
-
 #include 
 #include 
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
 #include 
 #include 
+#include 

 #include "rockchip_drm_drv.h"
 #include "rockchip_drm_fb.h"
@@ -49,28 +49,31 @@ static struct drm_driver rockchip_drm_driver;
 int rockchip_drm_dma_attach_device(struct drm_device *drm_dev,
   struct device *dev)
 {
-   struct dma_iommu_mapping *mapping = drm_dev->dev->archdata.mapping;
+   struct rockchip_drm_private *private = drm_dev->dev_private;
int ret;

if (!is_support_iommu)
return 0;

-   ret = dma_set_coherent_mask(dev, DMA_BIT_MASK(32));
-   if (ret)
+   ret = iommu_attach_device(private->domain, dev);
+   if (ret) {
+   dev_err(dev, "Failed to attach iommu device\n");
return ret;
+   }

-   dma_set_max_seg_size(dev, DMA_BIT_MASK(32));
-
-   return arm_iommu_attach_device(dev, mapping);
+   return 0;
 }

 void rockchip_drm_dma_detach_device(struct drm_device *drm_dev,
struct device *dev)
 {
+   struct rockchip_drm_private *private = drm_dev->dev_private;
+   struct iommu_domain *domain = private->domain;
+
if (!is_support_iommu)
return;

-   arm_iommu_detach_device(dev);
+   iommu_detach_device(domain, dev);
 }

 int rockchip_register_crtc_funcs(struct drm_crtc *crtc,
@@ -135,11 +138,45 @@ static void rockchip_drm_crtc_disable_vblank(struct 
drm_device *dev,
priv->crtc_funcs[pipe]->disable_vblank(crtc);
 }

+static int rockchip_drm_init_iommu(struct drm_device *drm_dev)
+{
+   struct rockchip_drm_private *private = drm_dev->dev_private;
+   struct iommu_domain_geometry *geometry;
+   u64 start, end;
+
+   if (!is_support_iommu)
+   return 0;
+
+   private->domain = iommu_domain_alloc(_bus_type);
+   if (!private->domain)
+   return -ENOMEM;
+
+   geometry = >domain->geometry;
+   start = geometry->aperture_start;
+   end = geometry->aperture_end;
+
+   DRM_DEBUG("IOMMU context initialized (aperture: %#llx-%#llx)\n",
+ start, end);
+   drm_mm_init(>mm, start, end - start + 1);
+
+   return 0;
+}
+
+static void rockchip_iommu_cleanup(struct drm_device *drm_dev)
+{
+   struct rockchip_drm_private *private = drm_dev->dev_private;
+
+   if (!is_support_iommu)
+   return;
+
+   drm_mm_takedown(>mm);
+   iommu_domain_free(private->domain);
+}
+
 static int rockchip_drm_bind(struct device *dev)
 {
struct drm_device *drm_dev;
struct rockchip_drm_private *private;
-   struct dma_iommu_mapping *mapping = NULL;
int ret;

drm_dev = drm_dev_alloc(_drm_driver, dev);
@@ -160,38 +197,14 @@ static int rockchip_drm_bind(struct device *dev)

rockchip_drm_mode_config_init(drm_dev);

-   dev->dma_parms = devm_kzalloc(dev, sizeof(*dev->dma_parms),
- GFP_KERNEL);
-   if (!dev->dma_parms) {
-   ret = -ENOMEM;
+   ret = rockchip_drm_init_iommu(drm_dev);
+   if (ret)
goto err_config_cleanup;
-   }
-
-   if (is_support_iommu) {
-   /* TODO(djkurtz): fetch the mapping start/size from somewhere */
-   mapping = arm_iommu_create_mapping(_bus_type,
-  0x,
-  SZ_2G);
-   if (IS_ERR(mapping)) {
-   ret = PTR_ERR(mapping);
-   goto err_config_cleanup;
-   }
-
-   ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
-   if (ret)
-   goto err_release_mapping;
-
-   dma_set_max_seg_size(dev, DMA_BIT_MASK(32));
-
-   ret = arm_iommu_attach_device(dev, mapping);
-   if (ret)
-  

[PATCH v4 8/8] iommu/rockchip: Enable Rockchip IOMMU on ARM64

[Tomasz Figa]

From: Simon Xue <x...@rock-chips.com>

This patch makes it possible to compile the rockchip-iommu driver on
ARM64, so that it can be used with 64-bit SoCs equipped with this type
of IOMMU.

Signed-off-by: Simon Xue 
Signed-off-by: Shunqian Zheng 
Signed-off-by: Tomasz Figa 
---
 drivers/iommu/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index ad08603..5572621 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -218,7 +218,7 @@ config OMAP_IOMMU_DEBUG

 config ROCKCHIP_IOMMU
bool "Rockchip IOMMU Support"
-   depends on ARM
+   depends on ARM || ARM64
depends on ARCH_ROCKCHIP || COMPILE_TEST
select IOMMU_API
select ARM_DMA_USE_IOMMU
-- 
2.8.0.rc3.226.g39d4020

[PATCH v4 2/8] iommu/rockchip: Add map_sg callback for rk_iommu_ops

[Tomasz Figa]

From: Simon Xue <x...@rock-chips.com>

The iommu_dma_alloc() in iommu/dma-iommu.c calls iommu_map_sg()
that requires the callback iommu_ops .map_sg(). Adding the
default_iommu_map_sg() to Rockchip IOMMU accordingly.

Signed-off-by: Simon Xue 
Signed-off-by: Shunqian Zheng 
Reviewed-on: https://chromium-review.googlesource.com/346326
Reviewed-by: Douglas Anderson 
Signed-off-by: Tomasz Figa 
---
 drivers/iommu/rockchip-iommu.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
index 5a9659a..53fa0d9 100644
--- a/drivers/iommu/rockchip-iommu.c
+++ b/drivers/iommu/rockchip-iommu.c
@@ -1022,6 +1022,7 @@ static const struct iommu_ops rk_iommu_ops = {
.detach_dev = rk_iommu_detach_device,
.map = rk_iommu_map,
.unmap = rk_iommu_unmap,
+   .map_sg = default_iommu_map_sg,
.add_device = rk_iommu_add_device,
.remove_device = rk_iommu_remove_device,
.iova_to_phys = rk_iommu_iova_to_phys,
-- 
2.8.0.rc3.226.g39d4020