| Message ID | 20230123151212.269082-1-jagan@amarulasolutions.com |
|---|---|
| Headers | show |
| Series | drm: Add Samsung MIPI DSIM bridge | expand |
On Mon, Jan 23, 2023 at 8:42 PM Jagan Teki <jagan@amarulasolutions.com> wrote: > > This series supports common bridge support for Samsung MIPI DSIM > which is used in Exynos and i.MX8MM SoC's. > > The final bridge supports both the Exynos and i.MX8M Mini/Nano/Plus. > > Patch 0001 - 0004: adding devm_drm_of_dsi_get_bridge > > Patch 0005 - 0006: optional PHY, PMS_P offset > > Patch 0007 : introduce hw_type > > Patch 0008 : fixing host init > > Patch 0009 : atomic_check > > Patch 0010 : input_bus_flags > > Patch 0011 : atomic_get_input_bus_fmts > > Patch 0012 - 0013: component vs bridge > > Patch 0014 : DSIM bridge > > Patch 0015 - 0016: i.MX8M Mini/Nano > > Patch 0017 - 0018: i.MX8M Plus > > Changes for v11: > - collect RB from Frieder Schrempf > - collect ACK from Rob > - collect ACK from Robert > - fix BIT macro replacements > - fix checkpatch --strict warnings > - fix unneeded commit text > - drop extra lines > > Changes for v10: > - rebase on drm-misc-next > - add drm_of_dsi_find_panel_or_bridge > - add devm_drm_of_dsi_get_bridge > - fix host initialization (Thanks to Marek Szyprowski) > - rearrange the tiny patches for easy to review > - update simple names for enum hw_type > - add is_hw_exynos macro > - rework on commit messages > > Changes for v9: > - rebase on drm-misc-next > - drop drm bridge attach fix for Exynos > - added prepare_prev_first flag > - added pre_enable_prev_first flag > - fix bridge chain order for exynos > - added fix for Exynos host init for first DSI transfer > - added MEDIA_BUS_FMT_FIXED > - return MEDIA_BUS_FMT_RGB888_1X24 output_fmt if supported output_fmt > list is unsupported. > - added MEDIA_BUS_FMT_YUYV10_1X20 > - added MEDIA_BUS_FMT_YUYV12_1X24 > > Changes for v8: > * fixed comment lines > * fixed commit messages > * fixed video mode bits > * collect Marek Ack > * fixed video mode bit names > * update input formats logic > * added imx8mplus support > > Changes for v7: > * fix the drm bridge attach chain for exynos drm dsi driver > * fix the hw_type checking logic > > Changes for v6: > * handle previous bridge for exynos dsi while attaching bridge > > Changes for v5: > * bridge changes to support multi-arch > * updated and clear commit messages > * add hw_type via plat data > * removed unneeded quirk > * rebased on linux-next > > Changes for v4: > * include Inki Dae in MAINTAINERS > * remove dsi_driver probe in exynos_drm_drv to support multi-arch build > * update init handling to ensure host init done on first cmd transfer > > Changes for v3: > * fix the mult-arch build > * fix dsi host init > * updated commit messages > > Changes for v2: > * fix bridge handling > * fix dsi host init > * correct the commit messages > > Tested in Engicam i.Core MX8M Mini SoM. > > Repo: > https://github.com/openedev/kernel/tree/imx8mm-dsi-v11 > > v10: > https://lore.kernel.org/all/20221214125907.376148-1-jagan@amarulasolutions.com/ > > Any inputs? > Jagan. > > Jagan Teki (16): > drm: of: Lookup if child node has DSI panel or bridge > drm: bridge: panel: Add devm_drm_of_dsi_get_bridge helper > drm: exynos: dsi: Drop explicit call to bridge detach > drm: exynos: dsi: Switch to devm_drm_of_dsi_get_bridge > drm: exynos: dsi: Mark PHY as optional > drm: exynos: dsi: Add platform PLL_P (PMS_P) offset > drm: exynos: dsi: Introduce hw_type platform data > drm: exynos: dsi: Add atomic check > drm: exynos: dsi: Add input_bus_flags > drm: exynos: dsi: Add atomic_get_input_bus_fmts > drm: exynos: dsi: Consolidate component and bridge > drm: exynos: dsi: Add Exynos based host irq hooks > drm: bridge: Generalize Exynos-DSI driver into a Samsung DSIM bridge > dt-bindings: display: exynos: dsim: Add NXP i.MX8M Mini/Nano support > drm: bridge: samsung-dsim: Add i.MX8M Mini/Nano support > dt-bindings: display: exynos: dsim: Add NXP i.MX8M Plus support > > Marek Szyprowski (1): > drm: exynos: dsi: Handle proper host initialization > > Marek Vasut (1): > drm: bridge: samsung-dsim: Add i.MX8M Plus support Can anyone pick this series? Thanks, Jagan.
On 1/23/23 16:12, Jagan Teki wrote: [...] > +static bool exynos_dsi_pixel_output_fmt_supported(u32 fmt) > +{ > + int i; if (fmt == MEDIA_BUS_FMT_FIXED) return false; > + for (i = 0; i < ARRAY_SIZE(exynos_dsi_pixel_output_fmts); i++) { > + if (exynos_dsi_pixel_output_fmts[i] == fmt) > + return true; > + } > + > + return false; > +} > + > +static u32 * > +exynos_dsi_atomic_get_input_bus_fmts(struct drm_bridge *bridge, > + struct drm_bridge_state *bridge_state, > + struct drm_crtc_state *crtc_state, > + struct drm_connector_state *conn_state, > + u32 output_fmt, > + unsigned int *num_input_fmts) > +{ > + u32 *input_fmts; > + > + if (!exynos_dsi_pixel_output_fmt_supported(output_fmt)) > + /* > + * Some bridge/display drivers are still not able to pass the > + * correct format, so handle those pipelines by falling back > + * to the default format till the supported formats finalized. > + */ > + output_fmt = MEDIA_BUS_FMT_RGB888_1X24; You can move this ^ past the kmalloc() call, so in unlikely case the kmalloc() fails, it would fail first. > + input_fmts = kmalloc(sizeof(*input_fmts), GFP_KERNEL); > + if (!input_fmts) > + return NULL; Delete from here ... > + switch (output_fmt) { > + case MEDIA_BUS_FMT_FIXED: > + input_fmts[0] = MEDIA_BUS_FMT_RGB888_1X24; > + break; > + default: > + input_fmts[0] = output_fmt; > + break; > + } ... until here, and do simple: input_fmts[0] = output_fmt; The effect should be the same, the code should be simpler and faster. > + *num_input_fmts = 1; [...]
On 1/23/23 16:12, Jagan Teki wrote: > Enable and disable of te_gpio's are Exynos platform specific > irq handling, so add the exynos based irq operations and hook > them for exynos plat_data. If this is just an optional generic GPIO IRQ, why not keep it in the core code ? TE (tearing enable?) should be available on MX8M too.
On 1/23/23 16:12, Jagan Teki wrote: > Samsung MIPI DSIM controller is common DSI IP that can be used > in various SoCs like Exynos, i.MX8M Mini/Nano/Plus. > > Add hw_type enum via platform_data so that accessing the different > controller data between various platforms becomes easy and meaningful. > > Reviewed-by: Frieder Schrempf <frieder.schrempf@kontron.de> > Suggested-by: Marek Szyprowski <m.szyprowski@samsung.com> > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> Reviewed-by: Marek Vasut <marex@denx.de>
On 1/23/23 16:12, Jagan Teki wrote: > Look like an explicit fixing up of mode_flags is required for DSIM IP > present in i.MX8M Mini/Nano SoCs. > > At least the LCDIF + DSIM needs active low sync polarities in order > to correlate the correct sync flags of the surrounding components in > the chain to make sure the whole pipeline can work properly. > > On the other hand the i.MX 8M Mini Applications Processor Reference Manual, > Rev. 3, 11/2020 says. > "13.6.3.5.2 RGB interface > Vsync, Hsync, and VDEN are active high signals." > > i.MX 8M Mini Applications Processor Reference Manual Rev. 3, 11/2020 > 3.6.3.5.2 RGB interface > i.MX 8M Nano Applications Processor Reference Manual Rev. 2, 07/2022 > 13.6.2.7.2 RGB interface > both claim "Vsync, Hsync, and VDEN are active high signals.", the > LCDIF must generate inverted HS/VS/DE signals, i.e. active LOW. > > No clear evidence about whether it can be documentation issues or > something, so added proper comments on the code. > > Comments are suggested by Marek Vasut. > > Reviewed-by: Frieder Schrempf <frieder.schrempf@kontron.de> > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> Reviewed-by: Marek Vasut <marex@denx.de>
On 1/23/23 16:12, Jagan Teki wrote: > Samsung MIPI DSIM master can also be found in i.MX8M Mini/Nano SoC. > > Add compatible and associated driver_data for it. > > Reviewed-by: Frieder Schrempf <frieder.schrempf@kontron.de> > Acked-by: Robert Foss <robert.foss@linaro.org> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> Reviewed-by: Marek Vasut <marex@denx.de>
On 1/23/23 16:12, Jagan Teki wrote: [...] > @@ -6738,6 +6738,15 @@ T: git git://anongit.freedesktop.org/drm/drm-misc > F: Documentation/devicetree/bindings/display/panel/samsung,lms397kf04.yaml > F: drivers/gpu/drm/panel/panel-samsung-db7430.c > > +DRM DRIVER FOR SAMSUNG MIPI DSIM BRIDGE > +M: Jagan Teki <jagan@amarulasolutions.com> > +M: Marek Szyprowski <m.szyprowski@samsung.com> > +M: Inki Dae <inki.dae@samsung.com Keep the list sorted. > +S: Maintained > +T: git git://anongit.freedesktop.org/drm/drm-misc > +F: drivers/gpu/drm/bridge/samsung-dsim.c > +F: include/drm/bridge/samsung-dsim.h > + > DRM DRIVER FOR SAMSUNG S6D27A1 PANELS > M: Markuss Broks <markuss.broks@gmail.com> > S: Maintained [...] With that fixed, Reviewed-by: Marek Vasut <marex@denx.de>
On 1/23/23 16:12, Jagan Teki wrote: > From: Marek Vasut <marex@denx.de> > > Add extras to support i.MX8M Plus. The main change is the removal of > HS/VS/DE signal inversion in the LCDIFv3-DSIM glue logic, otherwise > the implementation of this IP in i.MX8M Plus is very much compatible > with the i.MX8M Mini/Nano one. > > Reviewed-by: Frieder Schrempf <frieder.schrempf@kontron.de> > Acked-by: Robert Foss <robert.foss@linaro.org> > Signed-off-by: Marek Vasut <marex@denx.de> > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> Self-review for completeness: Reviewed-by: Marek Vasut <marex@denx.de>
On Wed, Jan 25, 2023 at 2:18 AM Marek Vasut <marex@denx.de> wrote: > > On 1/23/23 16:12, Jagan Teki wrote: > > Enable and disable of te_gpio's are Exynos platform specific > > irq handling, so add the exynos based irq operations and hook > > them for exynos plat_data. > > If this is just an optional generic GPIO IRQ, why not keep it in the > core code ? TE (tearing enable?) should be available on MX8M too. So far the discussion (since from initial versions) with Marek Szyprowski, seems to be available in Exynos. So, I keep it separate from the DSIM core. Jagan.
On 1/24/23 22:01, Jagan Teki wrote: > On Wed, Jan 25, 2023 at 2:18 AM Marek Vasut <marex@denx.de> wrote: >> >> On 1/23/23 16:12, Jagan Teki wrote: >>> Enable and disable of te_gpio's are Exynos platform specific >>> irq handling, so add the exynos based irq operations and hook >>> them for exynos plat_data. >> >> If this is just an optional generic GPIO IRQ, why not keep it in the >> core code ? TE (tearing enable?) should be available on MX8M too. > > So far the discussion (since from initial versions) with Marek > Szyprowski, seems to be available in Exynos. So, I keep it separate > from the DSIM core. Isn't TE a generic GPIO IRQ ? If so, it is available also on i.MX8M .
On 1/23/23 16:11, Jagan Teki wrote: > This series supports common bridge support for Samsung MIPI DSIM > which is used in Exynos and i.MX8MM SoC's. > > The final bridge supports both the Exynos and i.MX8M Mini/Nano/Plus. > > Patch 0001 - 0004: adding devm_drm_of_dsi_get_bridge > > Patch 0005 - 0006: optional PHY, PMS_P offset > > Patch 0007 : introduce hw_type > > Patch 0008 : fixing host init > > Patch 0009 : atomic_check > > Patch 0010 : input_bus_flags > > Patch 0011 : atomic_get_input_bus_fmts > > Patch 0012 - 0013: component vs bridge > > Patch 0014 : DSIM bridge > > Patch 0015 - 0016: i.MX8M Mini/Nano > > Patch 0017 - 0018: i.MX8M Plus Please drop chen.fang@nxp.com, narmstrong@linaro.org, jy0922.shim@samsung.com from CC, they bounce.
On Wed, Jan 25, 2023 at 2:15 AM Marek Vasut <marex@denx.de> wrote: > > On 1/23/23 16:12, Jagan Teki wrote: > > [...] > > > +static bool exynos_dsi_pixel_output_fmt_supported(u32 fmt) > > +{ > > + int i; > > if (fmt == MEDIA_BUS_FMT_FIXED) > return false; > > > + for (i = 0; i < ARRAY_SIZE(exynos_dsi_pixel_output_fmts); i++) { > > + if (exynos_dsi_pixel_output_fmts[i] == fmt) > > + return true; > > + } > > + > > + return false; > > +} > > + > > +static u32 * > > +exynos_dsi_atomic_get_input_bus_fmts(struct drm_bridge *bridge, > > + struct drm_bridge_state *bridge_state, > > + struct drm_crtc_state *crtc_state, > > + struct drm_connector_state *conn_state, > > + u32 output_fmt, > > + unsigned int *num_input_fmts) > > +{ > > + u32 *input_fmts; > > + > > + if (!exynos_dsi_pixel_output_fmt_supported(output_fmt)) > > + /* > > + * Some bridge/display drivers are still not able to pass the > > + * correct format, so handle those pipelines by falling back > > + * to the default format till the supported formats finalized. > > + */ > > + output_fmt = MEDIA_BUS_FMT_RGB888_1X24; > > You can move this ^ past the kmalloc() call, so in unlikely case the > kmalloc() fails, it would fail first. I didn't get this point, what do we need to do if exynos_dsi_pixel_output_fmt_supported returns false? Jagan.
On 1/24/23 22:16, Jagan Teki wrote: > On Wed, Jan 25, 2023 at 2:15 AM Marek Vasut <marex@denx.de> wrote: >> >> On 1/23/23 16:12, Jagan Teki wrote: >> >> [...] >> >>> +static bool exynos_dsi_pixel_output_fmt_supported(u32 fmt) >>> +{ >>> + int i; >> >> if (fmt == MEDIA_BUS_FMT_FIXED) >> return false; >> >>> + for (i = 0; i < ARRAY_SIZE(exynos_dsi_pixel_output_fmts); i++) { >>> + if (exynos_dsi_pixel_output_fmts[i] == fmt) >>> + return true; >>> + } >>> + >>> + return false; >>> +} >>> + >>> +static u32 * >>> +exynos_dsi_atomic_get_input_bus_fmts(struct drm_bridge *bridge, >>> + struct drm_bridge_state *bridge_state, >>> + struct drm_crtc_state *crtc_state, >>> + struct drm_connector_state *conn_state, >>> + u32 output_fmt, >>> + unsigned int *num_input_fmts) >>> +{ >>> + u32 *input_fmts; >>> + >>> + if (!exynos_dsi_pixel_output_fmt_supported(output_fmt)) >>> + /* >>> + * Some bridge/display drivers are still not able to pass the >>> + * correct format, so handle those pipelines by falling back >>> + * to the default format till the supported formats finalized. >>> + */ >>> + output_fmt = MEDIA_BUS_FMT_RGB888_1X24; >> >> You can move this ^ past the kmalloc() call, so in unlikely case the >> kmalloc() fails, it would fail first. > > I didn't get this point, what do we need to do if > exynos_dsi_pixel_output_fmt_supported returns false? { u32 *input_fmts; input_fmts = kmalloc(sizeof(*input_fmts), GFP_KERNEL); if (!input_fmts) return NULL; if (!exynos_dsi_pixel_output_fmt_supported(output_fmt)) /* ... the comment ... */ output_fmt = MEDIA_BUS_FMT_RGB888_1X24; input_fmts[0] = output_fmt; *num_input_fmts = 1; return input_fmts; }
On Wed, Jan 25, 2023 at 2:49 AM Marek Vasut <marex@denx.de> wrote: > > On 1/24/23 22:16, Jagan Teki wrote: > > On Wed, Jan 25, 2023 at 2:15 AM Marek Vasut <marex@denx.de> wrote: > >> > >> On 1/23/23 16:12, Jagan Teki wrote: > >> > >> [...] > >> > >>> +static bool exynos_dsi_pixel_output_fmt_supported(u32 fmt) > >>> +{ > >>> + int i; > >> > >> if (fmt == MEDIA_BUS_FMT_FIXED) > >> return false; > >> > >>> + for (i = 0; i < ARRAY_SIZE(exynos_dsi_pixel_output_fmts); i++) { > >>> + if (exynos_dsi_pixel_output_fmts[i] == fmt) > >>> + return true; > >>> + } > >>> + > >>> + return false; > >>> +} > >>> + > >>> +static u32 * > >>> +exynos_dsi_atomic_get_input_bus_fmts(struct drm_bridge *bridge, > >>> + struct drm_bridge_state *bridge_state, > >>> + struct drm_crtc_state *crtc_state, > >>> + struct drm_connector_state *conn_state, > >>> + u32 output_fmt, > >>> + unsigned int *num_input_fmts) > >>> +{ > >>> + u32 *input_fmts; > >>> + > >>> + if (!exynos_dsi_pixel_output_fmt_supported(output_fmt)) > >>> + /* > >>> + * Some bridge/display drivers are still not able to pass the > >>> + * correct format, so handle those pipelines by falling back > >>> + * to the default format till the supported formats finalized. > >>> + */ > >>> + output_fmt = MEDIA_BUS_FMT_RGB888_1X24; > >> > >> You can move this ^ past the kmalloc() call, so in unlikely case the > >> kmalloc() fails, it would fail first. > > > > I didn't get this point, what do we need to do if > > exynos_dsi_pixel_output_fmt_supported returns false? > > { > u32 *input_fmts; > > input_fmts = kmalloc(sizeof(*input_fmts), GFP_KERNEL); > if (!input_fmts) > return NULL; > > if (!exynos_dsi_pixel_output_fmt_supported(output_fmt)) > /* ... the comment ... */ > output_fmt = MEDIA_BUS_FMT_RGB888_1X24; > > input_fmts[0] = output_fmt; > *num_input_fmts = 1; > > return input_fmts; > } Got it, thanks!
On Wed, Jan 25, 2023 at 2:42 AM Marek Vasut <marex@denx.de> wrote: > > On 1/24/23 22:01, Jagan Teki wrote: > > On Wed, Jan 25, 2023 at 2:18 AM Marek Vasut <marex@denx.de> wrote: > >> > >> On 1/23/23 16:12, Jagan Teki wrote: > >>> Enable and disable of te_gpio's are Exynos platform specific > >>> irq handling, so add the exynos based irq operations and hook > >>> them for exynos plat_data. > >> > >> If this is just an optional generic GPIO IRQ, why not keep it in the > >> core code ? TE (tearing enable?) should be available on MX8M too. > > > > So far the discussion (since from initial versions) with Marek > > Szyprowski, seems to be available in Exynos. So, I keep it separate > > from the DSIM core. > > Isn't TE a generic GPIO IRQ ? If so, it is available also on i.MX8M .
On Wed, Jan 25, 2023 at 2:54 AM Jagan Teki <jagan@amarulasolutions.com> wrote: > > On Wed, Jan 25, 2023 at 2:42 AM Marek Vasut <marex@denx.de> wrote: > > > > On 1/24/23 22:01, Jagan Teki wrote: > > > On Wed, Jan 25, 2023 at 2:18 AM Marek Vasut <marex@denx.de> wrote: > > >> > > >> On 1/23/23 16:12, Jagan Teki wrote: > > >>> Enable and disable of te_gpio's are Exynos platform specific > > >>> irq handling, so add the exynos based irq operations and hook > > >>> them for exynos plat_data. > > >> > > >> If this is just an optional generic GPIO IRQ, why not keep it in the > > >> core code ? TE (tearing enable?) should be available on MX8M too. > > > > > > So far the discussion (since from initial versions) with Marek > > > Szyprowski, seems to be available in Exynos. So, I keep it separate > > > from the DSIM core. > > > > Isn't TE a generic GPIO IRQ ? If so, it is available also on i.MX8M . I will check this. Jagan.
On Wed, Jan 25, 2023 at 2:54 AM Jagan Teki <jagan@amarulasolutions.com> wrote: > > On Wed, Jan 25, 2023 at 2:54 AM Jagan Teki <jagan@amarulasolutions.com> wrote: > > > > On Wed, Jan 25, 2023 at 2:42 AM Marek Vasut <marex@denx.de> wrote: > > > > > > On 1/24/23 22:01, Jagan Teki wrote: > > > > On Wed, Jan 25, 2023 at 2:18 AM Marek Vasut <marex@denx.de> wrote: > > > >> > > > >> On 1/23/23 16:12, Jagan Teki wrote: > > > >>> Enable and disable of te_gpio's are Exynos platform specific > > > >>> irq handling, so add the exynos based irq operations and hook > > > >>> them for exynos plat_data. > > > >> > > > >> If this is just an optional generic GPIO IRQ, why not keep it in the > > > >> core code ? TE (tearing enable?) should be available on MX8M too. > > > > > > > > So far the discussion (since from initial versions) with Marek > > > > Szyprowski, seems to be available in Exynos. So, I keep it separate > > > > from the DSIM core. > > > > > > Isn't TE a generic GPIO IRQ ? If so, it is available also on i.MX8M . > > I will check this. In order to use TE_GPIO we need te handler implementation, right now Exynos CRTC DRM drivers have implementation for this. That is the main reason to keep the TE_GPIO handling in exynos, maybe if we handle that generically then it is a viable option to move TE_GPIO to the DSIM core. Jagan.
On 1/25/23 07:54, Jagan Teki wrote: > On Wed, Jan 25, 2023 at 2:54 AM Jagan Teki <jagan@amarulasolutions.com> wrote: >> >> On Wed, Jan 25, 2023 at 2:54 AM Jagan Teki <jagan@amarulasolutions.com> wrote: >>> >>> On Wed, Jan 25, 2023 at 2:42 AM Marek Vasut <marex@denx.de> wrote: >>>> >>>> On 1/24/23 22:01, Jagan Teki wrote: >>>>> On Wed, Jan 25, 2023 at 2:18 AM Marek Vasut <marex@denx.de> wrote: >>>>>> >>>>>> On 1/23/23 16:12, Jagan Teki wrote: >>>>>>> Enable and disable of te_gpio's are Exynos platform specific >>>>>>> irq handling, so add the exynos based irq operations and hook >>>>>>> them for exynos plat_data. >>>>>> >>>>>> If this is just an optional generic GPIO IRQ, why not keep it in the >>>>>> core code ? TE (tearing enable?) should be available on MX8M too. >>>>> >>>>> So far the discussion (since from initial versions) with Marek >>>>> Szyprowski, seems to be available in Exynos. So, I keep it separate >>>>> from the DSIM core. >>>> >>>> Isn't TE a generic GPIO IRQ ? If so, it is available also on i.MX8M . >> >> I will check this. > > In order to use TE_GPIO we need te handler implementation, right now > Exynos CRTC DRM drivers have implementation for this. That is the main > reason to keep the TE_GPIO handling in exynos, maybe if we handle that > generically then it is a viable option to move TE_GPIO to the DSIM > core. I think you can do this exactly the same way exynos does it -- check whether te_handler() callback is implemented by the glue code (the one you already have for various exynos and imx8mm/imx8mm SoCs) and if so, call it. If it is not implemented, do not call anything in the TE IRQ handler.
On Wed, Jan 25, 2023 at 7:23 PM Marek Vasut <marex@denx.de> wrote: > > On 1/25/23 07:54, Jagan Teki wrote: > > On Wed, Jan 25, 2023 at 2:54 AM Jagan Teki <jagan@amarulasolutions.com> wrote: > >> > >> On Wed, Jan 25, 2023 at 2:54 AM Jagan Teki <jagan@amarulasolutions.com> wrote: > >>> > >>> On Wed, Jan 25, 2023 at 2:42 AM Marek Vasut <marex@denx.de> wrote: > >>>> > >>>> On 1/24/23 22:01, Jagan Teki wrote: > >>>>> On Wed, Jan 25, 2023 at 2:18 AM Marek Vasut <marex@denx.de> wrote: > >>>>>> > >>>>>> On 1/23/23 16:12, Jagan Teki wrote: > >>>>>>> Enable and disable of te_gpio's are Exynos platform specific > >>>>>>> irq handling, so add the exynos based irq operations and hook > >>>>>>> them for exynos plat_data. > >>>>>> > >>>>>> If this is just an optional generic GPIO IRQ, why not keep it in the > >>>>>> core code ? TE (tearing enable?) should be available on MX8M too. > >>>>> > >>>>> So far the discussion (since from initial versions) with Marek > >>>>> Szyprowski, seems to be available in Exynos. So, I keep it separate > >>>>> from the DSIM core. > >>>> > >>>> Isn't TE a generic GPIO IRQ ? If so, it is available also on i.MX8M . > >> > >> I will check this. > > > > In order to use TE_GPIO we need te handler implementation, right now > > Exynos CRTC DRM drivers have implementation for this. That is the main > > reason to keep the TE_GPIO handling in exynos, maybe if we handle that > > generically then it is a viable option to move TE_GPIO to the DSIM > > core. > > I think you can do this exactly the same way exynos does it -- check > whether te_handler() callback is implemented by the glue code (the one > you already have for various exynos and imx8mm/imx8mm SoCs) and if so, > call it. If it is not implemented, do not call anything in the TE IRQ > handler. I need to understand how i.MX8MM handles this on TE IRQ in the DSIM host side, Can I do this in future patch set as it might involve bindings changes as well if it's part of DSIM? Jagan.
On Wed, Jan 25, 2023 at 2:42 AM Marek Vasut <marex@denx.de> wrote: > > On 1/24/23 22:01, Jagan Teki wrote: > > On Wed, Jan 25, 2023 at 2:18 AM Marek Vasut <marex@denx.de> wrote: > >> > >> On 1/23/23 16:12, Jagan Teki wrote: > >>> Enable and disable of te_gpio's are Exynos platform specific > >>> irq handling, so add the exynos based irq operations and hook > >>> them for exynos plat_data. > >> > >> If this is just an optional generic GPIO IRQ, why not keep it in the > >> core code ? TE (tearing enable?) should be available on MX8M too. > > > > So far the discussion (since from initial versions) with Marek > > Szyprowski, seems to be available in Exynos. So, I keep it separate > > from the DSIM core. > > Isn't TE a generic GPIO IRQ ? If so, it is available also on i.MX8M . I didn't find this in the DSIM part in i.MX8M Manual nor in the i.MX 8/RT MIPI DSI/CSI-2 or bsp kernel [1], did you find anywhere in i.MX8M part? Look like TE GPIO means tearing effect signal handle on the panel side. from, Documentation/devicetree/bindings/display/panel/panel-common.yaml te-gpios: maxItems: 1 description: GPIO spec for the tearing effect synchronization signal. The tearing effect signal is active high. Active low signals can be supported by inverting the GPIO specifier polarity flag. Maybe Exynos hack this gpio on the host side instead of on the panel side for some reason, not sure about it - Marek Szypeowski any comments please? [1] https://source.codeaurora.org/external/imx/linux-imx/tree/drivers/gpu/drm/bridge/sec-dsim.c?h=imx_5.4.47_2.2.0 Thanks, Jagan.
On 1/25/23 15:04, Jagan Teki wrote: > On Wed, Jan 25, 2023 at 7:23 PM Marek Vasut <marex@denx.de> wrote: >> >> On 1/25/23 07:54, Jagan Teki wrote: >>> On Wed, Jan 25, 2023 at 2:54 AM Jagan Teki <jagan@amarulasolutions.com> wrote: >>>> >>>> On Wed, Jan 25, 2023 at 2:54 AM Jagan Teki <jagan@amarulasolutions.com> wrote: >>>>> >>>>> On Wed, Jan 25, 2023 at 2:42 AM Marek Vasut <marex@denx.de> wrote: >>>>>> >>>>>> On 1/24/23 22:01, Jagan Teki wrote: >>>>>>> On Wed, Jan 25, 2023 at 2:18 AM Marek Vasut <marex@denx.de> wrote: >>>>>>>> >>>>>>>> On 1/23/23 16:12, Jagan Teki wrote: >>>>>>>>> Enable and disable of te_gpio's are Exynos platform specific >>>>>>>>> irq handling, so add the exynos based irq operations and hook >>>>>>>>> them for exynos plat_data. >>>>>>>> >>>>>>>> If this is just an optional generic GPIO IRQ, why not keep it in the >>>>>>>> core code ? TE (tearing enable?) should be available on MX8M too. >>>>>>> >>>>>>> So far the discussion (since from initial versions) with Marek >>>>>>> Szyprowski, seems to be available in Exynos. So, I keep it separate >>>>>>> from the DSIM core. >>>>>> >>>>>> Isn't TE a generic GPIO IRQ ? If so, it is available also on i.MX8M . >>>> >>>> I will check this. >>> >>> In order to use TE_GPIO we need te handler implementation, right now >>> Exynos CRTC DRM drivers have implementation for this. That is the main >>> reason to keep the TE_GPIO handling in exynos, maybe if we handle that >>> generically then it is a viable option to move TE_GPIO to the DSIM >>> core. >> >> I think you can do this exactly the same way exynos does it -- check >> whether te_handler() callback is implemented by the glue code (the one >> you already have for various exynos and imx8mm/imx8mm SoCs) and if so, >> call it. If it is not implemented, do not call anything in the TE IRQ >> handler. > > I need to understand how i.MX8MM handles this on TE IRQ in the DSIM > host side, Can I do this in future patch set as it might involve > bindings changes as well if it's part of DSIM? Why not leave an empty te_handler implementation on MX8M for now ? You can fill that implementation in future patchset, but the generic part of the code would be in place .
On Wed, Jan 25, 2023 at 10:16 PM Marek Vasut <marex@denx.de> wrote: > > On 1/25/23 15:04, Jagan Teki wrote: > > On Wed, Jan 25, 2023 at 7:23 PM Marek Vasut <marex@denx.de> wrote: > >> > >> On 1/25/23 07:54, Jagan Teki wrote: > >>> On Wed, Jan 25, 2023 at 2:54 AM Jagan Teki <jagan@amarulasolutions.com> wrote: > >>>> > >>>> On Wed, Jan 25, 2023 at 2:54 AM Jagan Teki <jagan@amarulasolutions.com> wrote: > >>>>> > >>>>> On Wed, Jan 25, 2023 at 2:42 AM Marek Vasut <marex@denx.de> wrote: > >>>>>> > >>>>>> On 1/24/23 22:01, Jagan Teki wrote: > >>>>>>> On Wed, Jan 25, 2023 at 2:18 AM Marek Vasut <marex@denx.de> wrote: > >>>>>>>> > >>>>>>>> On 1/23/23 16:12, Jagan Teki wrote: > >>>>>>>>> Enable and disable of te_gpio's are Exynos platform specific > >>>>>>>>> irq handling, so add the exynos based irq operations and hook > >>>>>>>>> them for exynos plat_data. > >>>>>>>> > >>>>>>>> If this is just an optional generic GPIO IRQ, why not keep it in the > >>>>>>>> core code ? TE (tearing enable?) should be available on MX8M too. > >>>>>>> > >>>>>>> So far the discussion (since from initial versions) with Marek > >>>>>>> Szyprowski, seems to be available in Exynos. So, I keep it separate > >>>>>>> from the DSIM core. > >>>>>> > >>>>>> Isn't TE a generic GPIO IRQ ? If so, it is available also on i.MX8M . > >>>> > >>>> I will check this. > >>> > >>> In order to use TE_GPIO we need te handler implementation, right now > >>> Exynos CRTC DRM drivers have implementation for this. That is the main > >>> reason to keep the TE_GPIO handling in exynos, maybe if we handle that > >>> generically then it is a viable option to move TE_GPIO to the DSIM > >>> core. > >> > >> I think you can do this exactly the same way exynos does it -- check > >> whether te_handler() callback is implemented by the glue code (the one > >> you already have for various exynos and imx8mm/imx8mm SoCs) and if so, > >> call it. If it is not implemented, do not call anything in the TE IRQ > >> handler. > > > > I need to understand how i.MX8MM handles this on TE IRQ in the DSIM > > host side, Can I do this in future patch set as it might involve > > bindings changes as well if it's part of DSIM? > > Why not leave an empty te_handler implementation on MX8M for now ? > You can fill that implementation in future patchset, but the generic > part of the code would be in place . Look like we have one issue to move regster te_irq into samsung-dsim. exynos_dsi_register_te_irq is done after the bridge attach is done in Exynos, here bridge attach is triggered in the component ops bind call, since samsung-dsim is a pure bridge w/o any component ops. https://github.com/openedev/kernel/blob/imx8mm-dsi-v12/drivers/gpu/drm/bridge/samsung-dsim.c#L1527 https://github.com/openedev/kernel/blob/imx8mm-dsi-v12/drivers/gpu/drm/exynos/exynos_drm_dsi.c#L112 Any suggestion on how to handle this? Thanks, Jagan.
On 1/25/23 18:12, Jagan Teki wrote: > On Wed, Jan 25, 2023 at 10:16 PM Marek Vasut <marex@denx.de> wrote: >> >> On 1/25/23 15:04, Jagan Teki wrote: >>> On Wed, Jan 25, 2023 at 7:23 PM Marek Vasut <marex@denx.de> wrote: >>>> >>>> On 1/25/23 07:54, Jagan Teki wrote: >>>>> On Wed, Jan 25, 2023 at 2:54 AM Jagan Teki <jagan@amarulasolutions.com> wrote: >>>>>> >>>>>> On Wed, Jan 25, 2023 at 2:54 AM Jagan Teki <jagan@amarulasolutions.com> wrote: >>>>>>> >>>>>>> On Wed, Jan 25, 2023 at 2:42 AM Marek Vasut <marex@denx.de> wrote: >>>>>>>> >>>>>>>> On 1/24/23 22:01, Jagan Teki wrote: >>>>>>>>> On Wed, Jan 25, 2023 at 2:18 AM Marek Vasut <marex@denx.de> wrote: >>>>>>>>>> >>>>>>>>>> On 1/23/23 16:12, Jagan Teki wrote: >>>>>>>>>>> Enable and disable of te_gpio's are Exynos platform specific >>>>>>>>>>> irq handling, so add the exynos based irq operations and hook >>>>>>>>>>> them for exynos plat_data. >>>>>>>>>> >>>>>>>>>> If this is just an optional generic GPIO IRQ, why not keep it in the >>>>>>>>>> core code ? TE (tearing enable?) should be available on MX8M too. >>>>>>>>> >>>>>>>>> So far the discussion (since from initial versions) with Marek >>>>>>>>> Szyprowski, seems to be available in Exynos. So, I keep it separate >>>>>>>>> from the DSIM core. >>>>>>>> >>>>>>>> Isn't TE a generic GPIO IRQ ? If so, it is available also on i.MX8M . >>>>>> >>>>>> I will check this. >>>>> >>>>> In order to use TE_GPIO we need te handler implementation, right now >>>>> Exynos CRTC DRM drivers have implementation for this. That is the main >>>>> reason to keep the TE_GPIO handling in exynos, maybe if we handle that >>>>> generically then it is a viable option to move TE_GPIO to the DSIM >>>>> core. >>>> >>>> I think you can do this exactly the same way exynos does it -- check >>>> whether te_handler() callback is implemented by the glue code (the one >>>> you already have for various exynos and imx8mm/imx8mm SoCs) and if so, >>>> call it. If it is not implemented, do not call anything in the TE IRQ >>>> handler. >>> >>> I need to understand how i.MX8MM handles this on TE IRQ in the DSIM >>> host side, Can I do this in future patch set as it might involve >>> bindings changes as well if it's part of DSIM? >> >> Why not leave an empty te_handler implementation on MX8M for now ? >> You can fill that implementation in future patchset, but the generic >> part of the code would be in place . > > Look like we have one issue to move regster te_irq into samsung-dsim. > > exynos_dsi_register_te_irq is done after the bridge attach is done in > Exynos, here bridge attach is triggered in the component ops bind > call, since samsung-dsim is a pure bridge w/o any component ops. > https://github.com/openedev/kernel/blob/imx8mm-dsi-v12/drivers/gpu/drm/bridge/samsung-dsim.c#L1527 > https://github.com/openedev/kernel/blob/imx8mm-dsi-v12/drivers/gpu/drm/exynos/exynos_drm_dsi.c#L112 > > Any suggestion on how to handle this? Why isn't the generic code calling drm_bridge_attach() in samsung_dsim_host_attach(), like the exynos one ?
On Wed, Jan 25, 2023 at 10:57 PM Marek Vasut <marex@denx.de> wrote: > > On 1/25/23 18:12, Jagan Teki wrote: > > On Wed, Jan 25, 2023 at 10:16 PM Marek Vasut <marex@denx.de> wrote: > >> > >> On 1/25/23 15:04, Jagan Teki wrote: > >>> On Wed, Jan 25, 2023 at 7:23 PM Marek Vasut <marex@denx.de> wrote: > >>>> > >>>> On 1/25/23 07:54, Jagan Teki wrote: > >>>>> On Wed, Jan 25, 2023 at 2:54 AM Jagan Teki <jagan@amarulasolutions.com> wrote: > >>>>>> > >>>>>> On Wed, Jan 25, 2023 at 2:54 AM Jagan Teki <jagan@amarulasolutions.com> wrote: > >>>>>>> > >>>>>>> On Wed, Jan 25, 2023 at 2:42 AM Marek Vasut <marex@denx.de> wrote: > >>>>>>>> > >>>>>>>> On 1/24/23 22:01, Jagan Teki wrote: > >>>>>>>>> On Wed, Jan 25, 2023 at 2:18 AM Marek Vasut <marex@denx.de> wrote: > >>>>>>>>>> > >>>>>>>>>> On 1/23/23 16:12, Jagan Teki wrote: > >>>>>>>>>>> Enable and disable of te_gpio's are Exynos platform specific > >>>>>>>>>>> irq handling, so add the exynos based irq operations and hook > >>>>>>>>>>> them for exynos plat_data. > >>>>>>>>>> > >>>>>>>>>> If this is just an optional generic GPIO IRQ, why not keep it in the > >>>>>>>>>> core code ? TE (tearing enable?) should be available on MX8M too. > >>>>>>>>> > >>>>>>>>> So far the discussion (since from initial versions) with Marek > >>>>>>>>> Szyprowski, seems to be available in Exynos. So, I keep it separate > >>>>>>>>> from the DSIM core. > >>>>>>>> > >>>>>>>> Isn't TE a generic GPIO IRQ ? If so, it is available also on i.MX8M . > >>>>>> > >>>>>> I will check this. > >>>>> > >>>>> In order to use TE_GPIO we need te handler implementation, right now > >>>>> Exynos CRTC DRM drivers have implementation for this. That is the main > >>>>> reason to keep the TE_GPIO handling in exynos, maybe if we handle that > >>>>> generically then it is a viable option to move TE_GPIO to the DSIM > >>>>> core. > >>>> > >>>> I think you can do this exactly the same way exynos does it -- check > >>>> whether te_handler() callback is implemented by the glue code (the one > >>>> you already have for various exynos and imx8mm/imx8mm SoCs) and if so, > >>>> call it. If it is not implemented, do not call anything in the TE IRQ > >>>> handler. > >>> > >>> I need to understand how i.MX8MM handles this on TE IRQ in the DSIM > >>> host side, Can I do this in future patch set as it might involve > >>> bindings changes as well if it's part of DSIM? > >> > >> Why not leave an empty te_handler implementation on MX8M for now ? > >> You can fill that implementation in future patchset, but the generic > >> part of the code would be in place . > > > > Look like we have one issue to move regster te_irq into samsung-dsim. > > > > exynos_dsi_register_te_irq is done after the bridge attach is done in > > Exynos, here bridge attach is triggered in the component ops bind > > call, since samsung-dsim is a pure bridge w/o any component ops. > > https://github.com/openedev/kernel/blob/imx8mm-dsi-v12/drivers/gpu/drm/bridge/samsung-dsim.c#L1527 > > https://github.com/openedev/kernel/blob/imx8mm-dsi-v12/drivers/gpu/drm/exynos/exynos_drm_dsi.c#L112 > > > > Any suggestion on how to handle this? > > Why isn't the generic code calling drm_bridge_attach() in > samsung_dsim_host_attach(), like the exynos one ? Exynos drm drivers follow component ops and generic dsim is a pure drm bridge whose downstream bridge will attach in bridge ops attach and the component-based drivers require an initial bridge attach (whose previous is NULL) call in the component bind hook for establishing the bridge chain. Jagan.
On 1/25/23 18:35, Jagan Teki wrote: [...] >>> exynos_dsi_register_te_irq is done after the bridge attach is done in >>> Exynos, here bridge attach is triggered in the component ops bind >>> call, since samsung-dsim is a pure bridge w/o any component ops. >>> https://github.com/openedev/kernel/blob/imx8mm-dsi-v12/drivers/gpu/drm/bridge/samsung-dsim.c#L1527 >>> https://github.com/openedev/kernel/blob/imx8mm-dsi-v12/drivers/gpu/drm/exynos/exynos_drm_dsi.c#L112 >>> >>> Any suggestion on how to handle this? >> >> Why isn't the generic code calling drm_bridge_attach() in >> samsung_dsim_host_attach(), like the exynos one ? > > Exynos drm drivers follow component ops and generic dsim is a pure drm > bridge whose downstream bridge will attach in bridge ops attach and > the component-based drivers require an initial bridge attach (whose > previous is NULL) call in the component bind hook for establishing the > bridge chain. Well in that case, call the exynos optional host_attach and register the TE IRQ handler at the end, that should work just fine too, right ? If so, then you can also move the IRQ handler registration into the generic part of the driver.
On Wed, Jan 25, 2023 at 11:33 PM Marek Vasut <marex@denx.de> wrote: > > On 1/25/23 18:35, Jagan Teki wrote: > > [...] > > >>> exynos_dsi_register_te_irq is done after the bridge attach is done in > >>> Exynos, here bridge attach is triggered in the component ops bind > >>> call, since samsung-dsim is a pure bridge w/o any component ops. > >>> https://github.com/openedev/kernel/blob/imx8mm-dsi-v12/drivers/gpu/drm/bridge/samsung-dsim.c#L1527 > >>> https://github.com/openedev/kernel/blob/imx8mm-dsi-v12/drivers/gpu/drm/exynos/exynos_drm_dsi.c#L112 > >>> > >>> Any suggestion on how to handle this? > >> > >> Why isn't the generic code calling drm_bridge_attach() in > >> samsung_dsim_host_attach(), like the exynos one ? > > > > Exynos drm drivers follow component ops and generic dsim is a pure drm > > bridge whose downstream bridge will attach in bridge ops attach and > > the component-based drivers require an initial bridge attach (whose > > previous is NULL) call in the component bind hook for establishing the > > bridge chain. > > Well in that case, call the exynos optional host_attach and register the > TE IRQ handler at the end, that should work just fine too, right ? If > so, then you can also move the IRQ handler registration into the generic > part of the driver. Something like this? samsung_dsim_host_attach() { drm_bridge_add(&dsi->bridge); if (pdata->host_ops && pdata->host_ops->attach) pdata->host_ops->attach(dsi, device); exynos_dsi_register_te_irq dsi->lanes = device->lanes; dsi->format = device->format; dsi->mode_flags = device->mode_flags; } Jagan.
On 1/25/23 20:24, Jagan Teki wrote: > On Wed, Jan 25, 2023 at 11:33 PM Marek Vasut <marex@denx.de> wrote: >> >> On 1/25/23 18:35, Jagan Teki wrote: >> >> [...] >> >>>>> exynos_dsi_register_te_irq is done after the bridge attach is done in >>>>> Exynos, here bridge attach is triggered in the component ops bind >>>>> call, since samsung-dsim is a pure bridge w/o any component ops. >>>>> https://github.com/openedev/kernel/blob/imx8mm-dsi-v12/drivers/gpu/drm/bridge/samsung-dsim.c#L1527 >>>>> https://github.com/openedev/kernel/blob/imx8mm-dsi-v12/drivers/gpu/drm/exynos/exynos_drm_dsi.c#L112 >>>>> >>>>> Any suggestion on how to handle this? >>>> >>>> Why isn't the generic code calling drm_bridge_attach() in >>>> samsung_dsim_host_attach(), like the exynos one ? >>> >>> Exynos drm drivers follow component ops and generic dsim is a pure drm >>> bridge whose downstream bridge will attach in bridge ops attach and >>> the component-based drivers require an initial bridge attach (whose >>> previous is NULL) call in the component bind hook for establishing the >>> bridge chain. >> >> Well in that case, call the exynos optional host_attach and register the >> TE IRQ handler at the end, that should work just fine too, right ? If >> so, then you can also move the IRQ handler registration into the generic >> part of the driver. > > Something like this? > > samsung_dsim_host_attach() > { > drm_bridge_add(&dsi->bridge); > > if (pdata->host_ops && pdata->host_ops->attach) > pdata->host_ops->attach(dsi, device); > > exynos_dsi_register_te_irq > > dsi->lanes = device->lanes; > dsi->format = device->format; > dsi->mode_flags = device->mode_flags; > } Yes, I think that should work .