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