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

[Sam Ravnborg <sam@ravnborg.org>]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel