| Message ID | 1594760265-11618-1-git-send-email-anitha.chrisanthus@intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Add support for KeemBay DRM driver | expand |
Hi Anitha On Tue, Jul 14, 2020 at 01:56:46PM -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. > > 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. > > Device tree patches are under review here > https://lore.kernel.org/linux-arm-kernel/20200708175020.194436-1-daniele.alessandrelli@linux.intel.com/T/ Cool, new driver, thanks a lot for submitting. > Changes since v1: > - Removed redundant license text, updated license > - Rearranged include blocks > - renamed global vars and removed extern in c > - Used upclassing for dev_private > - Used drm_dev_init in drm device create (will be updated to use > devm_drm_dev_alloc() in a separate patch later as kmb driver is currently > developed on 5.4 kernel) drm moves fairly quickly, please develop the upstream submission on top of linux-next or similar. We constantly add new helpers to simplify drivers, and we expect new driver submissions to be up to date with all that. Another thing: From your description it sounds like it's a very simple driver, just a single plane/crtc, nothing fancy, plus adv bridge output. Is the driver already using simple display pipeline helpers? I think that would be an ideal fit and probably greatly simplifies the code. > - minor cleanups The patch series looks like it contains the entire development history, or at least large chunks of it. That's useful for you, but for upstreaming the main focues (especially for smaller drivers) is whether your driver uses all the available helpers and integrations correctly. And for that it's much easier if the history is cleaned up, and all intermediate steps removed. I think once that's done I can do a quick pass and drop suggestions for cleanup and stuff like that, and then we should (usually at least) be able to pull in the driver fairly quickly. Another thing to consider is where/how this driver will be maintained. Preferred option is as part of drm-misc so that we have redudancy and all that in a fairly big group. Works with commit rights, so maybe check out some of our docs about that too. https://drm.pages.freedesktop.org/maintainer-tools/drm-misc.html The committer model comes with a full set of scripts and docs to avoid oopsies in maintainership. Generally works really well. Cheers, Daniel > > 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 | 226 +++++ > drivers/gpu/drm/kmb/kmb_crtc.h | 41 + > drivers/gpu/drm/kmb/kmb_drv.c | 809 ++++++++++++++++ > drivers/gpu/drm/kmb/kmb_drv.h | 176 ++++ > drivers/gpu/drm/kmb/kmb_dsi.c | 1927 +++++++++++++++++++++++++++++++++++++++ > drivers/gpu/drm/kmb/kmb_dsi.h | 370 ++++++++ > drivers/gpu/drm/kmb/kmb_plane.c | 518 +++++++++++ > drivers/gpu/drm/kmb/kmb_plane.h | 124 +++ > drivers/gpu/drm/kmb/kmb_regs.h | 738 +++++++++++++++ > 13 files changed, 4946 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 > > -- > 2.7.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Jul 15, 2020 at 05:05:49PM +0200, Daniel Vetter wrote: > Hi Anitha > > On Tue, Jul 14, 2020 at 01:56:46PM -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. > > > > 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. > > > > Device tree patches are under review here > > https://lore.kernel.org/linux-arm-kernel/20200708175020.194436-1-daniele.alessandrelli@linux.intel.com/T/ > > Cool, new driver, thanks a lot for submitting. > > > Changes since v1: > > - Removed redundant license text, updated license > > - Rearranged include blocks > > - renamed global vars and removed extern in c > > - Used upclassing for dev_private > > - Used drm_dev_init in drm device create (will be updated to use > > devm_drm_dev_alloc() in a separate patch later as kmb driver is currently > > developed on 5.4 kernel) > > drm moves fairly quickly, please develop the upstream submission on top of > linux-next or similar. We constantly add new helpers to simplify drivers, > and we expect new driver submissions to be up to date with all that. > > Another thing: From your description it sounds like it's a very simple > driver, just a single plane/crtc, nothing fancy, plus adv bridge output. > Is the driver already using simple display pipeline helpers? I think that > would be an ideal fit and probably greatly simplifies the code. > > > - minor cleanups > > The patch series looks like it contains the entire development history, or > at least large chunks of it. That's useful for you, but for upstreaming > the main focues (especially for smaller drivers) is whether your driver > uses all the available helpers and integrations correctly. And for that > it's much easier if the history is cleaned up, and all intermediate steps > removed. > > I think once that's done I can do a quick pass and drop suggestions for > cleanup and stuff like that, and then we should (usually at least) be able > to pull in the driver fairly quickly. > > Another thing to consider is where/how this driver will be maintained. > Preferred option is as part of drm-misc so that we have redudancy and all > that in a fairly big group. Works with commit rights, so maybe check out > some of our docs about that too. > > https://drm.pages.freedesktop.org/maintainer-tools/drm-misc.html > > The committer model comes with a full set of scripts and docs to avoid > oopsies in maintainership. Generally works really well. Oh if you have a git branch of this all somewhere I could do a quick high-level pass and see whether there's anything big. -Daniel > > Cheers, Daniel > > > > > > 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 | 226 +++++ > > drivers/gpu/drm/kmb/kmb_crtc.h | 41 + > > drivers/gpu/drm/kmb/kmb_drv.c | 809 ++++++++++++++++ > > drivers/gpu/drm/kmb/kmb_drv.h | 176 ++++ > > drivers/gpu/drm/kmb/kmb_dsi.c | 1927 +++++++++++++++++++++++++++++++++++++++ > > drivers/gpu/drm/kmb/kmb_dsi.h | 370 ++++++++ > > drivers/gpu/drm/kmb/kmb_plane.c | 518 +++++++++++ > > drivers/gpu/drm/kmb/kmb_plane.h | 124 +++ > > drivers/gpu/drm/kmb/kmb_regs.h | 738 +++++++++++++++ > > 13 files changed, 4946 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 > > > > -- > > 2.7.4 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
Hi Anitha. On Tue, Jul 14, 2020 at 01:56:46PM -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. > > 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. > > Device tree patches are under review here > https://lore.kernel.org/linux-arm-kernel/20200708175020.194436-1-daniele.alessandrelli@linux.intel.com/T/ > > Changes since v1: > - Removed redundant license text, updated license > - Rearranged include blocks > - renamed global vars and removed extern in c > - Used upclassing for dev_private > - Used drm_dev_init in drm device create (will be updated to use > devm_drm_dev_alloc() in a separate patch later as kmb driver is currently > developed on 5.4 kernel) > - minor cleanups Could you please include a TODO or something. It is not obvious if some points from last review are pending or they were skipped. From a quick look maybe half of the poitns were addressed which is good. But some points are yet to be done. Sam > > 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 | 226 +++++ > drivers/gpu/drm/kmb/kmb_crtc.h | 41 + > drivers/gpu/drm/kmb/kmb_drv.c | 809 ++++++++++++++++ > drivers/gpu/drm/kmb/kmb_drv.h | 176 ++++ > drivers/gpu/drm/kmb/kmb_dsi.c | 1927 +++++++++++++++++++++++++++++++++++++++ > drivers/gpu/drm/kmb/kmb_dsi.h | 370 ++++++++ > drivers/gpu/drm/kmb/kmb_plane.c | 518 +++++++++++ > drivers/gpu/drm/kmb/kmb_plane.h | 124 +++ > drivers/gpu/drm/kmb/kmb_regs.h | 738 +++++++++++++++ > 13 files changed, 4946 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 > > -- > 2.7.4 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Wed, Jul 15, 2020 at 05:05:49PM +0200, Daniel Vetter wrote: > Hi Anitha > > On Tue, Jul 14, 2020 at 01:56:46PM -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. > > > > 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. > > > > Device tree patches are under review here > > https://lore.kernel.org/linux-arm-kernel/20200708175020.194436-1-daniele.alessandrelli@linux.intel.com/T/ > > Cool, new driver, thanks a lot for submitting. > > > Changes since v1: > > - Removed redundant license text, updated license > > - Rearranged include blocks > > - renamed global vars and removed extern in c > > - Used upclassing for dev_private > > - Used drm_dev_init in drm device create (will be updated to use > > devm_drm_dev_alloc() in a separate patch later as kmb driver is currently > > developed on 5.4 kernel) > > drm moves fairly quickly, please develop the upstream submission on top of > linux-next or similar. We constantly add new helpers to simplify drivers, > and we expect new driver submissions to be up to date with all that. Seconded! > > Another thing: From your description it sounds like it's a very simple > driver, just a single plane/crtc, nothing fancy, plus adv bridge output. > Is the driver already using simple display pipeline helpers? I think that > would be an ideal fit and probably greatly simplifies the code. > > > - minor cleanups > > The patch series looks like it contains the entire development history, or > at least large chunks of it. That's useful for you, but for upstreaming > the main focues (especially for smaller drivers) is whether your driver > uses all the available helpers and integrations correctly. And for that > it's much easier if the history is cleaned up, and all intermediate steps > removed. And also agree on this point. The submission could be split up in a few patches where the split is file based. So only with the latest patch, containing Makefile + Kconfig,the driver i buildable. This would ease review as one looses focus when trying to review 1000+ lines in one mail. You will loose some of the internal history - but if important keep relevant parts in sensible comments. Sam
Hi Sam and Daniel, Thank you very much for reviewing the code. I will squish all the commits and send version 3, so it will be easier for you to review. Anitha > -----Original Message----- > From: Sam Ravnborg <sam@ravnborg.org> > Sent: Wednesday, July 15, 2020 10:07 AM > To: Daniel Vetter <daniel@ffwll.ch> > Cc: Chrisanthus, Anitha <anitha.chrisanthus@intel.com>; Vetter, Daniel > <daniel.vetter@intel.com>; intel-gfx@lists.freedesktop.org; Dea, Edmund J > <edmund.j.dea@intel.com>; dri-devel@lists.freedesktop.org > Subject: Re: [Intel-gfx] [PATCH v2 00/59] Add support for KeemBay DRM driver > > On Wed, Jul 15, 2020 at 05:05:49PM +0200, Daniel Vetter wrote: > > Hi Anitha > > > > On Tue, Jul 14, 2020 at 01:56:46PM -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. > > > > > > 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. > > > > > > Device tree patches are under review here > > > https://lore.kernel.org/linux-arm-kernel/20200708175020.194436-1- > daniele.alessandrelli@linux.intel.com/T/ > > > > Cool, new driver, thanks a lot for submitting. > > > > > Changes since v1: > > > - Removed redundant license text, updated license > > > - Rearranged include blocks > > > - renamed global vars and removed extern in c > > > - Used upclassing for dev_private > > > - Used drm_dev_init in drm device create (will be updated to use > > > devm_drm_dev_alloc() in a separate patch later as kmb driver is currently > > > developed on 5.4 kernel) > > > > drm moves fairly quickly, please develop the upstream submission on top of > > linux-next or similar. We constantly add new helpers to simplify drivers, > > and we expect new driver submissions to be up to date with all that. > Seconded! > > > > > Another thing: From your description it sounds like it's a very simple > > driver, just a single plane/crtc, nothing fancy, plus adv bridge output. > > Is the driver already using simple display pipeline helpers? I think that > > would be an ideal fit and probably greatly simplifies the code. > > > > > - minor cleanups > > > > The patch series looks like it contains the entire development history, or > > at least large chunks of it. That's useful for you, but for upstreaming > > the main focues (especially for smaller drivers) is whether your driver > > uses all the available helpers and integrations correctly. And for that > > it's much easier if the history is cleaned up, and all intermediate steps > > removed. > And also agree on this point. > The submission could be split up in a few patches where the split is > file based. So only with the latest patch, containing Makefile + > Kconfig,the driver i buildable. > This would ease review as one looses focus when trying to review 1000+ > lines in one mail. > > You will loose some of the internal history - but if important keep > relevant parts in sensible comments. > > Sam
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. Device tree patches are under review here https://lore.kernel.org/linux-arm-kernel/20200708175020.194436-1-daniele.alessandrelli@linux.intel.com/T/ Changes since v1: - Removed redundant license text, updated license - Rearranged include blocks - renamed global vars and removed extern in c - Used upclassing for dev_private - Used drm_dev_init in drm device create (will be updated to use devm_drm_dev_alloc() in a separate patch later as kmb driver is currently developed on 5.4 kernel) - minor cleanups 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 | 226 +++++ drivers/gpu/drm/kmb/kmb_crtc.h | 41 + drivers/gpu/drm/kmb/kmb_drv.c | 809 ++++++++++++++++ drivers/gpu/drm/kmb/kmb_drv.h | 176 ++++ drivers/gpu/drm/kmb/kmb_dsi.c | 1927 +++++++++++++++++++++++++++++++++++++++ drivers/gpu/drm/kmb/kmb_dsi.h | 370 ++++++++ drivers/gpu/drm/kmb/kmb_plane.c | 518 +++++++++++ drivers/gpu/drm/kmb/kmb_plane.h | 124 +++ drivers/gpu/drm/kmb/kmb_regs.h | 738 +++++++++++++++ 13 files changed, 4946 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