mbox series

[00/59] Add support for Keem Bay DRM driver

Message ID 1593552491-23698-1-git-send-email-anitha.chrisanthus@intel.com (mailing list archive)
Headers show
Series Add support for Keem Bay DRM driver | expand

Message

Chrisanthus, Anitha June 30, 2020, 9:27 p.m. UTC 
This is a new DRM driver for Intel's KeemBay SOC.
The SoC couples an ARM Cortex A53 CPU with an Intel
Movidius VPU.

This driver is tested with the KMB EVM board which is the refernce baord
for Keem Bay SOC. The SOC's display pipeline is as follows

+--------------+    +---------+    +-----------------------+
|LCD controller| -> |Mipi DSI | -> |Mipi to HDMI Converter |
+--------------+    +---------+    +-----------------------+

LCD controller and Mipi DSI transmitter are part of the SOC and 
mipi to HDMI converter is ADV7535 for KMB EVM board.

The DRM driver is a basic KMS atomic modesetting display driver and
has no 2D or 3D graphics.It calls into the ADV bridge driver at 
the connector level.

Only 1080p resolution and single plane is supported at this time.
The usecase is for debugging video and camera outputs.

Since we are just starting the upstream process, the KMB EVM board is not in
mainline and so Device tree changes are missing.

Anitha Chrisanthus (52):
  drm/kmb: Add support for KeemBay Display
  drm/kmb: Added id to kmb_plane
  drm/kmb: Set correct values in the LAYERn_CFG register
  drm/kmb: Use biwise operators for register definitions
  drm/kmb: Updated kmb_plane_atomic_check
  drm/kmb: Initial check-in for Mipi DSI
  drm/kmb: Set OUT_FORMAT_CFG register
  drm/kmb: Added mipi_dsi_host initialization
  drm/kmb: Part 1 of Mipi Tx Initialization
  drm/kmb: Part 2 of Mipi Tx Initialization
  drm/kmb: Use correct mmio offset from data book
  drm/kmb: Part3 of Mipi Tx initialization
  drm/kmb: Part4 of Mipi Tx Initialization
  drm/kmb: Correct address offsets for mipi registers
  drm/kmb: Part5 of Mipi Tx Intitialization
  drm/kmb: Part6 of Mipi Tx Initialization
  drm/kmb: Part7 of Mipi Tx Initialization
  drm/kmb: Part8 of Mipi Tx Initialization
  drm/kmb: Added ioremap/iounmap for register access
  drm/kmb: Register IRQ for LCD
  drm/kmb: IRQ handlers for LCD and mipi dsi
  drm/kmb: Set hardcoded values to LCD_VSYNC_START
  drm/kmb: Additional register programming to update_plane
  drm/kmb: Add ADV7535 bridge
  drm/kmb: Display clock enable/disable
  drm/kmb: rebase to newer kernel version
  drm/kmb: minor name change to match device tree
  drm/kmb: Changed MMIO size
  drm/kmb: Defer Probe
  drm/kmb: call bridge init in the very beginning
  drm/kmb: Enable MSS_CAM_CLK_CTRL for LCD and MIPI
  drm/kmb: Set MSS_CAM_RSTN_CTRL along with enable
  drm/kmb: Mipi DPHY initialization changes
  drm/kmb: Fixed driver unload
  drm/kmb: Added LCD_TEST config
  drm/kmb: Changes for LCD to Mipi
  drm/kmb: Update LCD programming to match MIPI
  drm/kmb: Changed name of driver to kmb-drm
  drm/kmb: Mipi settings from input timings
  drm/kmb: Enable LCD interrupts
  drm/kmb: Enable LCD interrupts during modeset
  drm/kmb: Don’t inadvertantly disable LCD controller
  drm/kmb: SWAP R and B LCD Layer order
  drm/kmb: Disable ping pong mode
  drm/kmb: Do the layer initializations only once
  drm/kmb: disable the LCD layer in EOF irq handler
  drm/kmb: Initialize uninitialized variables
  drm/kmb: Added useful messages in LCD ISR
  kmb/drm: Prune unsupported modes
  drm/kmb: workaround for dma undeflow issue
  drm/kmb: Get System Clock from SCMI
  drm/kmb: work around for planar formats

Edmund Dea (7):
  drm/kmb: Cleanup probe functions
  drm/kmb: Revert dsi_host back to a static variable
  drm/kmb: Initialize clocks for clk_msscam, clk_mipi_ecfg, &
    clk_mipi_cfg.
  drm/kmb: Remove declaration of irq_lcd/irq_mipi
  drm/kmb: Enable MIPI TX HS Test Pattern Generation
  drm/kmb: Write to LCD_LAYERn_CFG only once
  drm/kmb: Cleaned up code

 drivers/gpu/drm/Kconfig         |    2 +
 drivers/gpu/drm/Makefile        |    1 +
 drivers/gpu/drm/kmb/Kconfig     |   12 +
 drivers/gpu/drm/kmb/Makefile    |    2 +
 drivers/gpu/drm/kmb/kmb_crtc.c  |  243 +++++
 drivers/gpu/drm/kmb/kmb_crtc.h  |   61 ++
 drivers/gpu/drm/kmb/kmb_drv.c   |  828 +++++++++++++++++
 drivers/gpu/drm/kmb/kmb_drv.h   |  196 ++++
 drivers/gpu/drm/kmb/kmb_dsi.c   | 1950 +++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/kmb/kmb_dsi.h   |  390 ++++++++
 drivers/gpu/drm/kmb/kmb_plane.c |  538 +++++++++++
 drivers/gpu/drm/kmb/kmb_plane.h |  142 +++
 drivers/gpu/drm/kmb/kmb_regs.h  |  758 +++++++++++++++
 13 files changed, 5123 insertions(+)
 create mode 100644 drivers/gpu/drm/kmb/Kconfig
 create mode 100644 drivers/gpu/drm/kmb/Makefile
 create mode 100644 drivers/gpu/drm/kmb/kmb_crtc.c
 create mode 100644 drivers/gpu/drm/kmb/kmb_crtc.h
 create mode 100644 drivers/gpu/drm/kmb/kmb_drv.c
 create mode 100644 drivers/gpu/drm/kmb/kmb_drv.h
 create mode 100644 drivers/gpu/drm/kmb/kmb_dsi.c
 create mode 100644 drivers/gpu/drm/kmb/kmb_dsi.h
 create mode 100644 drivers/gpu/drm/kmb/kmb_plane.c
 create mode 100644 drivers/gpu/drm/kmb/kmb_plane.h
 create mode 100644 drivers/gpu/drm/kmb/kmb_regs.h

Comments

Sam Ravnborg July 1, 2020, 7:01 a.m. UTC | #1 
Hi Anitha.

On Tue, Jun 30, 2020 at 02:27:12PM -0700, Anitha Chrisanthus wrote:
> This is a new DRM driver for Intel's KeemBay SOC.
> The SoC couples an ARM Cortex A53 CPU with an Intel
> Movidius VPU.
...
Nice and informative intro - thanks.

The patchset looks more like a dump of current state and less like a
logical set of changes - as the few I looked at changed code introduced
in earlier patches.
So I ended up applying all patches to a local branch.
Very good to post an WIP version so you can capture some early
feedback.
It is never fun to think something is finished and then address a lot of
feedback.


Some general remarks after reading/browsing some of the code:
- Embeds drm_device - good
- Uses SPDX, good. But includes also a license text. Can we
  get rid of the redundandt license text?
- Some of the inline functions in kmb_drv.h is too large
  (kmb_write_bits_mipi())
- There is stray code commented out using "//" here and there - shall be
  dropped.
- Please arrange include files in blocks:
#include <linux/...>

#include <video/...>

#include <drm/...>

#include "kmb_*"

Within each block sort alphabetially.

- Use a kmb_ prefix or similar for global variables - like under_flow
- no extern in .c files - plane_status
- Consider using an array for the clk's
- In general use drm_info(), drm_err() for logging.
  Yes, you will need to pass kmb_drm_private around to do so
- Do not use drm_device->dev_private, it is deprecated. Use upclassing
- kmb_driver can be slimmed a lot. See what recent drivers uses. There is
  some nice defines so it is obvious what is not standard.
  DRIVER_HAVE_IRQ is not relevant btw.
- Start using drmm_* support. The way kmb_drm_private is allocated
  is sub-optimal

The above was my fist drive-by comments - but do not be discouraged.
For the most part they should be easy to address.
Looking forward for other feedback and for v2.

Let me know if the above triggers any questions.

> +--------------+    +---------+    +-----------------------+
> |LCD controller| -> |Mipi DSI | -> |Mipi to HDMI Converter |
> +--------------+    +---------+    +-----------------------+

Question to dri-devel people:
Would the Mipi DSI be better represented outside the display driver?
If yes, how?

	Sam
Chrisanthus, Anitha July 1, 2020, 9:53 p.m. UTC | #2 
Thanks much Sam for reviewing the code so quickly. I'll address your reviews in v2.

Anitha

> -----Original Message-----
> From: Sam Ravnborg <sam@ravnborg.org>
> Sent: Wednesday, July 1, 2020 12:01 AM
> To: Chrisanthus, Anitha <anitha.chrisanthus@intel.com>
> Cc: dri-devel@lists.freedesktop.org; Paauwe, Bob J <bob.j.paauwe@intel.com>;
> Dea, Edmund J <edmund.j.dea@intel.com>; Vetter, Daniel
> <daniel.vetter@intel.com>; intel-gfx@lists.freedesktop.org; Vivi, Rodrigo
> <rodrigo.vivi@intel.com>
> Subject: Re: [PATCH 00/59] Add support for Keem Bay DRM driver
> 
> Hi Anitha.
> 
> On Tue, Jun 30, 2020 at 02:27:12PM -0700, Anitha Chrisanthus wrote:
> > This is a new DRM driver for Intel's KeemBay SOC.
> > The SoC couples an ARM Cortex A53 CPU with an Intel
> > Movidius VPU.
> ...
> Nice and informative intro - thanks.
> 
> The patchset looks more like a dump of current state and less like a
> logical set of changes - as the few I looked at changed code introduced
> in earlier patches.
> So I ended up applying all patches to a local branch.
> Very good to post an WIP version so you can capture some early
> feedback.
> It is never fun to think something is finished and then address a lot of
> feedback.
> 
> 
> Some general remarks after reading/browsing some of the code:
> - Embeds drm_device - good
> - Uses SPDX, good. But includes also a license text. Can we
>   get rid of the redundandt license text?
> - Some of the inline functions in kmb_drv.h is too large
>   (kmb_write_bits_mipi())
> - There is stray code commented out using "//" here and there - shall be
>   dropped.
> - Please arrange include files in blocks:
> #include <linux/...>
> 
> #include <video/...>
> 
> #include <drm/...>
> 
> #include "kmb_*"
> 
> Within each block sort alphabetially.
> 
> - Use a kmb_ prefix or similar for global variables - like under_flow
> - no extern in .c files - plane_status
> - Consider using an array for the clk's
> - In general use drm_info(), drm_err() for logging.
>   Yes, you will need to pass kmb_drm_private around to do so
> - Do not use drm_device->dev_private, it is deprecated. Use upclassing
> - kmb_driver can be slimmed a lot. See what recent drivers uses. There is
>   some nice defines so it is obvious what is not standard.
>   DRIVER_HAVE_IRQ is not relevant btw.
> - Start using drmm_* support. The way kmb_drm_private is allocated
>   is sub-optimal
> 
> The above was my fist drive-by comments - but do not be discouraged.
> For the most part they should be easy to address.
> Looking forward for other feedback and for v2.
> 
> Let me know if the above triggers any questions.
> 
> > +--------------+    +---------+    +-----------------------+
> > |LCD controller| -> |Mipi DSI | -> |Mipi to HDMI Converter |
> > +--------------+    +---------+    +-----------------------+
> 
> Question to dri-devel people:
> Would the Mipi DSI be better represented outside the display driver?
> If yes, how?
> 
> 	Sam
Neil Armstrong July 2, 2020, 10:19 a.m. UTC | #3 
Hi,

On 30/06/2020 23:27, Anitha Chrisanthus wrote:
> This is a new DRM driver for Intel's KeemBay SOC.
> The SoC couples an ARM Cortex A53 CPU with an Intel
> Movidius VPU.
> 
> This driver is tested with the KMB EVM board which is the refernce baord
> for Keem Bay SOC. The SOC's display pipeline is as follows
> 
> +--------------+    +---------+    +-----------------------+
> |LCD controller| -> |Mipi DSI | -> |Mipi to HDMI Converter |
> +--------------+    +---------+    +-----------------------+
> 
> LCD controller and Mipi DSI transmitter are part of the SOC and 
> mipi to HDMI converter is ADV7535 for KMB EVM board.
> 
> The DRM driver is a basic KMS atomic modesetting display driver and
> has no 2D or 3D graphics.It calls into the ADV bridge driver at 
> the connector level.
> 
> Only 1080p resolution and single plane is supported at this time.
> The usecase is for debugging video and camera outputs.
> 
> Since we are just starting the upstream process, the KMB EVM board is not in
> mainline and so Device tree changes are missing.

A proper Yaml bindings file is then necessary even if the platform is not
mainline.

Neil

> 
> Anitha Chrisanthus (52):
>   drm/kmb: Add support for KeemBay Display
>   drm/kmb: Added id to kmb_plane
>   drm/kmb: Set correct values in the LAYERn_CFG register
>   drm/kmb: Use biwise operators for register definitions
>   drm/kmb: Updated kmb_plane_atomic_check
>   drm/kmb: Initial check-in for Mipi DSI
>   drm/kmb: Set OUT_FORMAT_CFG register
>   drm/kmb: Added mipi_dsi_host initialization
>   drm/kmb: Part 1 of Mipi Tx Initialization
>   drm/kmb: Part 2 of Mipi Tx Initialization
>   drm/kmb: Use correct mmio offset from data book
>   drm/kmb: Part3 of Mipi Tx initialization
>   drm/kmb: Part4 of Mipi Tx Initialization
>   drm/kmb: Correct address offsets for mipi registers
>   drm/kmb: Part5 of Mipi Tx Intitialization
>   drm/kmb: Part6 of Mipi Tx Initialization
>   drm/kmb: Part7 of Mipi Tx Initialization
>   drm/kmb: Part8 of Mipi Tx Initialization
>   drm/kmb: Added ioremap/iounmap for register access
>   drm/kmb: Register IRQ for LCD
>   drm/kmb: IRQ handlers for LCD and mipi dsi
>   drm/kmb: Set hardcoded values to LCD_VSYNC_START
>   drm/kmb: Additional register programming to update_plane
>   drm/kmb: Add ADV7535 bridge
>   drm/kmb: Display clock enable/disable
>   drm/kmb: rebase to newer kernel version
>   drm/kmb: minor name change to match device tree
>   drm/kmb: Changed MMIO size
>   drm/kmb: Defer Probe
>   drm/kmb: call bridge init in the very beginning
>   drm/kmb: Enable MSS_CAM_CLK_CTRL for LCD and MIPI
>   drm/kmb: Set MSS_CAM_RSTN_CTRL along with enable
>   drm/kmb: Mipi DPHY initialization changes
>   drm/kmb: Fixed driver unload
>   drm/kmb: Added LCD_TEST config
>   drm/kmb: Changes for LCD to Mipi
>   drm/kmb: Update LCD programming to match MIPI
>   drm/kmb: Changed name of driver to kmb-drm
>   drm/kmb: Mipi settings from input timings
>   drm/kmb: Enable LCD interrupts
>   drm/kmb: Enable LCD interrupts during modeset
>   drm/kmb: Don’t inadvertantly disable LCD controller
>   drm/kmb: SWAP R and B LCD Layer order
>   drm/kmb: Disable ping pong mode
>   drm/kmb: Do the layer initializations only once
>   drm/kmb: disable the LCD layer in EOF irq handler
>   drm/kmb: Initialize uninitialized variables
>   drm/kmb: Added useful messages in LCD ISR
>   kmb/drm: Prune unsupported modes
>   drm/kmb: workaround for dma undeflow issue
>   drm/kmb: Get System Clock from SCMI
>   drm/kmb: work around for planar formats
> 
> Edmund Dea (7):
>   drm/kmb: Cleanup probe functions
>   drm/kmb: Revert dsi_host back to a static variable
>   drm/kmb: Initialize clocks for clk_msscam, clk_mipi_ecfg, &
>     clk_mipi_cfg.
>   drm/kmb: Remove declaration of irq_lcd/irq_mipi
>   drm/kmb: Enable MIPI TX HS Test Pattern Generation
>   drm/kmb: Write to LCD_LAYERn_CFG only once
>   drm/kmb: Cleaned up code
> 
>  drivers/gpu/drm/Kconfig         |    2 +
>  drivers/gpu/drm/Makefile        |    1 +
>  drivers/gpu/drm/kmb/Kconfig     |   12 +
>  drivers/gpu/drm/kmb/Makefile    |    2 +
>  drivers/gpu/drm/kmb/kmb_crtc.c  |  243 +++++
>  drivers/gpu/drm/kmb/kmb_crtc.h  |   61 ++
>  drivers/gpu/drm/kmb/kmb_drv.c   |  828 +++++++++++++++++
>  drivers/gpu/drm/kmb/kmb_drv.h   |  196 ++++
>  drivers/gpu/drm/kmb/kmb_dsi.c   | 1950 +++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/kmb/kmb_dsi.h   |  390 ++++++++
>  drivers/gpu/drm/kmb/kmb_plane.c |  538 +++++++++++
>  drivers/gpu/drm/kmb/kmb_plane.h |  142 +++
>  drivers/gpu/drm/kmb/kmb_regs.h  |  758 +++++++++++++++
>  13 files changed, 5123 insertions(+)
>  create mode 100644 drivers/gpu/drm/kmb/Kconfig
>  create mode 100644 drivers/gpu/drm/kmb/Makefile
>  create mode 100644 drivers/gpu/drm/kmb/kmb_crtc.c
>  create mode 100644 drivers/gpu/drm/kmb/kmb_crtc.h
>  create mode 100644 drivers/gpu/drm/kmb/kmb_drv.c
>  create mode 100644 drivers/gpu/drm/kmb/kmb_drv.h
>  create mode 100644 drivers/gpu/drm/kmb/kmb_dsi.c
>  create mode 100644 drivers/gpu/drm/kmb/kmb_dsi.h
>  create mode 100644 drivers/gpu/drm/kmb/kmb_plane.c
>  create mode 100644 drivers/gpu/drm/kmb/kmb_plane.h
>  create mode 100644 drivers/gpu/drm/kmb/kmb_regs.h
>