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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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 ?

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.

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

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

One of the main locking issues here are cross-references. The display entity 
holds a reference to the video source (for video operations), and the display 
controller driver holds a reference to the display entity (for control 
operations), resulting in a cross-references lock situation. One possible 
solution would be to first unbind the display entity device from its driver to 
break the cycle.

>  - 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 +
>  drivers/video/display/Makefile            |    3 +
>  drivers/video/display/display-core.c      |  295 +++++++
>  drivers/video/display/panel-s6e8ax0.c     | 1027 ++++++++++++++++++++++
>  drivers/video/display/source-exynos_dsi.c | 1313 ++++++++++++++++++++++++++
>  include/video/display.h                   |  230 +++++
>  include/video/exynos_dsi.h                |   41 +
>  include/video/panel-s6e8ax0.h             |   41 +
>  10 files changed, 2968 insertions(+)
>  create mode 100644 drivers/video/display/Kconfig
>  create mode 100644 drivers/video/display/Makefile
>  create mode 100644 drivers/video/display/display-core.c
>  create mode 100644 drivers/video/display/panel-s6e8ax0.c
>  create mode 100644 drivers/video/display/source-exynos_dsi.c
>  create mode 100644 include/video/display.h
>  create mode 100644 include/video/exynos_dsi.h
>  create mode 100644 include/video/panel-s6e8ax0.h
-- 
Regards,

Laurent Pinchart