Re: [PATCH 1/4] drm/mipi-dbi: Support command mode panel drivers

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.

- 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.

- 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.

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 noralf@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?

• 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.

+{

• 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

diff --git a/include/drm/drm_mipi_dbi.h b/include/drm/drm_mipi_dbi.h index 67c66f5ee591..f41ee0d31871 100644 --- a/include/drm/drm_mipi_dbi.h +++ b/include/drm/drm_mipi_dbi.h @@ -12,6 +12,7 @@ #include <drm/drm_device.h> #include <drm/drm_simple_kms_helper.h>

+struct drm_panel; struct drm_rect; struct spi_device; struct gpio_desc; @@ -123,6 +124,11 @@ struct mipi_dbi_dev { * @dbi: MIPI DBI interface */ struct mipi_dbi dbi;

• /**

• * @panel: Attached DRM panel. See drm_mipi_dbi_panel_register().
  

• */
  

• struct drm_panel *panel;

};

static inline struct mipi_dbi_dev *drm_to_mipi_dbi_dev(struct drm_device *drm) @@ -140,6 +146,34 @@ int mipi_dbi_dev_init_with_formats(struct mipi_dbi_dev *dbidev, int mipi_dbi_dev_init(struct mipi_dbi_dev *dbidev, const struct drm_simple_display_pipe_funcs *funcs, const struct drm_display_mode *mode, unsigned int rotation);

+/**

• • DEFINE_DRM_MIPI_DBI_PANEL_DRIVER - Define a DRM driver structure
• • @_name: Name
• • @_desc: Description
• • @_date: Date
• • This macro defines a &drm_driver for MIPI DBI panel drivers.
• */

+#define DEFINE_DRM_MIPI_DBI_PANEL_DRIVER(_name, _desc, _date) \

• DEFINE_DRM_GEM_CMA_FOPS(_name ## _fops); \
• static struct drm_driver _name ## _drm_driver = { \

• .driver_features	= DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC, \
  

• .fops			= & _name ## _fops, \
  

• .release		= mipi_dbi_release, \
  

• DRM_GEM_CMA_VMAP_DRIVER_OPS, \
  

• .debugfs_init		= mipi_dbi_debugfs_init, \
  

• .name			= #_name, \
  

• .desc			= _desc, \
  

• .date			= _date, \
  

• .major			= 1, \
  

• .minor			= 0, \
  

• }

+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);
  

void mipi_dbi_release(struct drm_device *drm); void mipi_dbi_pipe_update(struct drm_simple_display_pipe *pipe, struct drm_plane_state *old_state); -- 2.20.1