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

[Noralf Trønnes]

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