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