Re: [PATCH 00/59] Add support for Keem Bay DRM driver
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 >
RE: [PATCH 00/59] Add support for Keem Bay DRM driver
Thanks much Sam for reviewing the code so quickly. I'll address your reviews in v2. Anitha > -Original Message- > From: Sam Ravnborg > Sent: Wednesday, July 1, 2020 12:01 AM > To: Chrisanthus, Anitha > Cc: dri-devel@lists.freedesktop.org; Paauwe, Bob J ; > Dea, Edmund J ; Vetter, Daniel > ; intel-...@lists.freedesktop.org; Vivi, Rodrigo > > 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 > > #include > > #include > > #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 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
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 #include #include #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 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
