Den 11.08.2019 16.16, skrev Sam Ravnborg: > Hi Noralf. > > I really like how this allows us to have a single file for all > the uses of a driver IC. > And this patch-set is much easier to grasp than the first RFC. > > General things: > > - The current design have a tight connection between the display > controller and the panel. Will this hurt in the long run? > In other words, should we try to add a panel_bridge in-between? > For now I think this would just make something simple more > complicated. > So this note was to say - no I think we should not use panel_bridge. >
I did look at panel_bridge but it didn't give me anything I needed, it would only be a 1:1 passthrough layer. > - drm_panel has proper support for modes. > This is today duplicated in mipi_dbi. > Could we make it so that when a panel is used then the panel > has the mode info - as we then use the panel more in the way we do > in other cases? > As it is now the mode is specified in mipi_dbi_dev_init() > The drm_connector would then, if a panel is used, ask the panel for > the mode. > I did not really think to the end of this, but it seems wrong that > we introduce drm_panel and then keep modes in mipi_dbi. > I considered that, but it would would just generate duplicate code for the connector. It would make sense to refactor this when/if all mipi dbi drivers are turned into panel drivers. > - For backlight support please move this to drm_panel. > I have patches that makes it simple to use backlight with drm_panel, > and this will then benefit from it. > The drm_panel backlight supports requires that a backlight > phandle is specified in the DT node of the panel. > I can fix that when I respin if those patches have landed by then. > Some more specific comments in the following. > > Sam > > On Thu, Aug 01, 2019 at 03:52:46PM +0200, Noralf Trønnes wrote: >> This adds a function that registers a DRM driver for use with MIPI DBI >> panels in command mode. That is, framebuffer upload over DBI. >> >> Signed-off-by: Noralf Trønnes <nor...@tronnes.org> >> --- >> drivers/gpu/drm/drm_mipi_dbi.c | 92 ++++++++++++++++++++++++++++++++++ >> include/drm/drm_mipi_dbi.h | 34 +++++++++++++ >> 2 files changed, 126 insertions(+) >> >> diff --git a/drivers/gpu/drm/drm_mipi_dbi.c b/drivers/gpu/drm/drm_mipi_dbi.c >> index 1961f713aaab..797a20e3a017 100644 >> --- a/drivers/gpu/drm/drm_mipi_dbi.c >> +++ b/drivers/gpu/drm/drm_mipi_dbi.c >> @@ -17,11 +17,13 @@ >> #include <drm/drm_damage_helper.h> >> #include <drm/drm_drv.h> >> #include <drm/drm_gem_cma_helper.h> >> +#include <drm/drm_fb_helper.h> >> #include <drm/drm_format_helper.h> >> #include <drm/drm_fourcc.h> >> #include <drm/drm_gem_framebuffer_helper.h> >> #include <drm/drm_mipi_dbi.h> >> #include <drm/drm_modes.h> >> +#include <drm/drm_panel.h> >> #include <drm/drm_probe_helper.h> >> #include <drm/drm_rect.h> >> #include <drm/drm_vblank.h> >> @@ -597,6 +599,96 @@ void mipi_dbi_release(struct drm_device *drm) >> } >> EXPORT_SYMBOL(mipi_dbi_release); >> >> +static void drm_mipi_dbi_panel_pipe_enable(struct drm_simple_display_pipe >> *pipe, >> + struct drm_crtc_state *crtc_state, >> + struct drm_plane_state *plane_state) >> +{ >> + struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(pipe->crtc.dev); >> + struct drm_panel *panel = dbidev->panel; >> + int ret, idx; >> + >> + if (!drm_dev_enter(pipe->crtc.dev, &idx)) >> + return; >> + >> + DRM_DEBUG_KMS("\n"); > Still usefull? > No I think this can go (it was in the code I copied from the driver). >> + >> + ret = drm_panel_prepare(panel); >> + if (ret) >> + goto out_exit; >> + >> + mipi_dbi_enable_flush(dbidev, crtc_state, plane_state); >> + >> + drm_panel_enable(panel); >> +out_exit: > nit - blank line above label? > >> + drm_dev_exit(idx); >> +} >> + >> +static void drm_mipi_dbi_panel_pipe_disable(struct drm_simple_display_pipe >> *pipe) >> +{ >> + struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(pipe->crtc.dev); >> + struct drm_panel *panel = dbidev->panel; >> + >> + if (!dbidev->enabled) >> + return; >> + >> + DRM_DEBUG_KMS("\n"); > Still usefull? >> + >> + dbidev->enabled = false; >> + drm_panel_disable(panel); >> + drm_panel_unprepare(panel); >> +} >> + >> +static const struct drm_simple_display_pipe_funcs drm_mipi_dbi_pipe_funcs = >> { >> + .enable = drm_mipi_dbi_panel_pipe_enable, >> + .disable = drm_mipi_dbi_panel_pipe_disable, >> + .update = mipi_dbi_pipe_update, >> + .prepare_fb = drm_gem_fb_simple_display_pipe_prepare_fb, >> +}; >> + >> +/** >> + * drm_mipi_dbi_panel_register - Register a MIPI DBI DRM driver >> + * @panel: DRM Panel >> + * @dbidev: MIPI DBI device structure to initialize >> + * @mode: Display mode >> + * >> + * This function registeres a DRM driver with @panel attached. >> + * >> + * Returns: >> + * Zero on success, negative error code on failure. >> + */ >> +int drm_mipi_dbi_panel_register(struct drm_panel *panel, struct >> mipi_dbi_dev *dbidev, >> + struct drm_driver *driver, const struct >> drm_display_mode *mode, >> + u32 rotation) > Can we make this use enum drm_panel_orientation - so we can use > of_drm_get_panel_orientation() in the callers? > of_drm_get_panel_orientation() is not merged yet, but we could do so if > this patch-set needs it. > > I know that this would require mipi_dbi_dev_init() and all users to be > updated. But it is a simpler interface so worth it. > That would break rotation on userspace that doesn't know about the panel orientation property which is a recent addition. These panels are mostly used in the embedded world not desktop. It also would break fbdev 90/270 rotation (see drm_client_rotation()). Noralf. >> +{ >> + struct device *dev = panel->dev; >> + struct drm_device *drm; >> + int ret; >> + >> + dbidev->panel = panel; >> + >> + drm = &dbidev->drm; >> + ret = devm_drm_dev_init(dev, drm, driver); >> + if (ret) { >> + kfree(dbidev); >> + return ret; >> + } >> + >> + drm_mode_config_init(drm); >> + >> + ret = mipi_dbi_dev_init(dbidev, &drm_mipi_dbi_pipe_funcs, mode, >> rotation); >> + >> + drm_mode_config_reset(drm); >> + >> + ret = drm_dev_register(drm, 0); >> + if (ret) >> + return ret; >> + >> + drm_fbdev_generic_setup(drm, 16); >> + >> + return 0; >> +} >> +EXPORT_SYMBOL(drm_mipi_dbi_panel_register); >> + >> /** >> * mipi_dbi_hw_reset - Hardware reset of controller >> * @dbi: MIPI DBI structure _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel