Re: [PATCH v2] drm/i915/dsi: switch to drm_panel interface

[Shobhit Kumar <shobhit.kumar@linux.intel.com>]

On 01/23/2015 07:00 PM, Jani Nikula wrote:
> Replace intel_dsi_device and intel_dsi_dev_ops with drm_panel and
> drm_panel_funcs. They are adequate for what we have now, and if we end
> up needing more than this we should improve drm_panel. This will keep us
> better aligned with the drm core infrastructure.
>
> The panel driver initialization changes a bit. It still remains hideous,
> but fixing that is beyond the scope here.
>
> v2: extend mode config mutex to cover drm_panel_get_modes (Shobhit)
>      vbt_panel->intel_dsi = intel_dsi in vbt panel init (Shobhit)
>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

Reviewed-By: Shobhit Kumar <shobhit.kumar@intel.com>

> ---
>   drivers/gpu/drm/i915/Kconfig               |   1 +
>   drivers/gpu/drm/i915/intel_dsi.c           |  68 +++++++----
>   drivers/gpu/drm/i915/intel_dsi.h           |  27 +----
>   drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 180 ++++++++++++++++++-----------
>   4 files changed, 157 insertions(+), 119 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig
> index 4e39ab34eb1c..da196cd07263 100644
> --- a/drivers/gpu/drm/i915/Kconfig
> +++ b/drivers/gpu/drm/i915/Kconfig
> @@ -11,6 +11,7 @@ config DRM_I915
>   	select SHMEM
>   	select TMPFS
>   	select DRM_KMS_HELPER
> +	select DRM_PANEL
>   	# i915 depends on ACPI_VIDEO when ACPI is enabled
>   	# but for select to work, need to select ACPI_VIDEO's dependencies, ick
>   	select BACKLIGHT_LCD_SUPPORT if ACPI
> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> index c56c4150fc13..52c5fabe284a 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.c
> +++ b/drivers/gpu/drm/i915/intel_dsi.c
> @@ -27,18 +27,20 @@
>   #include <drm/drm_crtc.h>
>   #include <drm/drm_edid.h>
>   #include <drm/i915_drm.h>
> +#include <drm/drm_panel.h>
>   #include <linux/slab.h>
>   #include "i915_drv.h"
>   #include "intel_drv.h"
>   #include "intel_dsi.h"
>   #include "intel_dsi_cmd.h"
>
> -/* the sub-encoders aka panel drivers */
> -static const struct intel_dsi_device intel_dsi_devices[] = {
> +static const struct {
> +	u16 panel_id;
> +	struct drm_panel * (*init)(struct intel_dsi *intel_dsi, u16 panel_id);
> +} intel_dsi_drivers[] = {
>   	{
>   		.panel_id = MIPI_DSI_GENERIC_PANEL_ID,
> -		.name = "vbt-generic-dsi-vid-mode-display",
> -		.dev_ops = &vbt_generic_dsi_display_ops,
> +		.init = vbt_panel_init,
>   	},
>   };
>
> @@ -214,8 +216,7 @@ static void intel_dsi_enable(struct intel_encoder *encoder)
>   			dpi_send_cmd(intel_dsi, TURN_ON, DPI_LP_MODE_EN, port);
>   		msleep(100);
>
> -		if (intel_dsi->dev.dev_ops->enable)
> -			intel_dsi->dev.dev_ops->enable(&intel_dsi->dev);
> +		drm_panel_enable(intel_dsi->panel);
>
>   		for_each_dsi_port(port, intel_dsi->ports)
>   			wait_for_dsi_fifo_empty(intel_dsi, port);
> @@ -255,8 +256,7 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder)
>
>   	msleep(intel_dsi->panel_on_delay);
>
> -	if (intel_dsi->dev.dev_ops->panel_reset)
> -		intel_dsi->dev.dev_ops->panel_reset(&intel_dsi->dev);
> +	drm_panel_prepare(intel_dsi->panel);
>
>   	for_each_dsi_port(port, intel_dsi->ports)
>   		wait_for_dsi_fifo_empty(intel_dsi, port);
> @@ -329,8 +329,7 @@ static void intel_dsi_disable(struct intel_encoder *encoder)
>   	}
>   	/* if disable packets are sent before sending shutdown packet then in
>   	 * some next enable sequence send turn on packet error is observed */
> -	if (intel_dsi->dev.dev_ops->disable)
> -		intel_dsi->dev.dev_ops->disable(&intel_dsi->dev);
> +	drm_panel_disable(intel_dsi->panel);
>
>   	for_each_dsi_port(port, intel_dsi->ports)
>   		wait_for_dsi_fifo_empty(intel_dsi, port);
> @@ -395,8 +394,7 @@ static void intel_dsi_post_disable(struct intel_encoder *encoder)
>   	val &= ~DPOUNIT_CLOCK_GATE_DISABLE;
>   	I915_WRITE(DSPCLK_GATE_D, val);
>
> -	if (intel_dsi->dev.dev_ops->disable_panel_power)
> -		intel_dsi->dev.dev_ops->disable_panel_power(&intel_dsi->dev);
> +	drm_panel_unprepare(intel_dsi->panel);
>
>   	msleep(intel_dsi->panel_off_delay);
>   	msleep(intel_dsi->panel_pwr_cycle_delay);
> @@ -760,7 +758,7 @@ static int intel_dsi_get_modes(struct drm_connector *connector)
>   	return 1;
>   }
>
> -static void intel_dsi_destroy(struct drm_connector *connector)
> +static void intel_dsi_connector_destroy(struct drm_connector *connector)
>   {
>   	struct intel_connector *intel_connector = to_intel_connector(connector);
>
> @@ -770,8 +768,20 @@ static void intel_dsi_destroy(struct drm_connector *connector)
>   	kfree(connector);
>   }
>
> +static void intel_dsi_encoder_destroy(struct drm_encoder *encoder)
> +{
> +	struct intel_dsi *intel_dsi = enc_to_intel_dsi(encoder);
> +
> +	if (intel_dsi->panel) {
> +		drm_panel_detach(intel_dsi->panel);
> +		/* XXX: Logically this call belongs in the panel driver. */
> +		drm_panel_remove(intel_dsi->panel);
> +	}
> +	intel_encoder_destroy(encoder);
> +}
> +
>   static const struct drm_encoder_funcs intel_dsi_funcs = {
> -	.destroy = intel_encoder_destroy,
> +	.destroy = intel_dsi_encoder_destroy,
>   };
>
>   static const struct drm_connector_helper_funcs intel_dsi_connector_helper_funcs = {
> @@ -783,7 +793,7 @@ static const struct drm_connector_helper_funcs intel_dsi_connector_helper_funcs
>   static const struct drm_connector_funcs intel_dsi_connector_funcs = {
>   	.dpms = intel_connector_dpms,
>   	.detect = intel_dsi_detect,
> -	.destroy = intel_dsi_destroy,
> +	.destroy = intel_dsi_connector_destroy,
>   	.fill_modes = drm_helper_probe_single_connector_modes,
>   };
>
> @@ -794,9 +804,8 @@ void intel_dsi_init(struct drm_device *dev)
>   	struct drm_encoder *encoder;
>   	struct intel_connector *intel_connector;
>   	struct drm_connector *connector;
> -	struct drm_display_mode *fixed_mode = NULL;
> +	struct drm_display_mode *scan, *fixed_mode = NULL;
>   	struct drm_i915_private *dev_priv = dev->dev_private;
> -	const struct intel_dsi_device *dsi;
>   	unsigned int i;
>
>   	DRM_DEBUG_KMS("\n");
> @@ -853,15 +862,14 @@ void intel_dsi_init(struct drm_device *dev)
>   		intel_dsi->ports = (1 << PORT_C);
>   	}
>
> -	for (i = 0; i < ARRAY_SIZE(intel_dsi_devices); i++) {
> -		dsi = &intel_dsi_devices[i];
> -		intel_dsi->dev = *dsi;
> -
> -		if (dsi->dev_ops->init(&intel_dsi->dev))
> +	for (i = 0; i < ARRAY_SIZE(intel_dsi_drivers); i++) {
> +		intel_dsi->panel = intel_dsi_drivers[i].init(intel_dsi,
> +							     intel_dsi_drivers[i].panel_id);
> +		if (intel_dsi->panel)
>   			break;
>   	}
>
> -	if (i == ARRAY_SIZE(intel_dsi_devices)) {
> +	if (!intel_dsi->panel) {
>   		DRM_DEBUG_KMS("no device found\n");
>   		goto err;
>   	}
> @@ -881,13 +889,23 @@ void intel_dsi_init(struct drm_device *dev)
>
>   	drm_connector_register(connector);
>
> -	fixed_mode = dsi->dev_ops->get_modes(&intel_dsi->dev);
> +	drm_panel_attach(intel_dsi->panel, connector);
> +
> +	mutex_lock(&dev->mode_config.mutex);
> +	drm_panel_get_modes(intel_dsi->panel);
> +	list_for_each_entry(scan, &connector->probed_modes, head) {
> +		if ((scan->type & DRM_MODE_TYPE_PREFERRED)) {
> +			fixed_mode = drm_mode_duplicate(dev, scan);
> +			break;
> +		}
> +	}
> +	mutex_unlock(&dev->mode_config.mutex);
> +
>   	if (!fixed_mode) {
>   		DRM_DEBUG_KMS("no fixed mode\n");
>   		goto err;
>   	}
>
> -	fixed_mode->type |= DRM_MODE_TYPE_PREFERRED;
>   	intel_panel_init(&intel_connector->panel, fixed_mode, NULL);
>
>   	return;
> diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h
> index 22f87036a256..fc0b2b8d90f1 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.h
> +++ b/drivers/gpu/drm/i915/intel_dsi.h
> @@ -33,33 +33,10 @@
>   #define DSI_DUAL_LINK_FRONT_BACK	1
>   #define DSI_DUAL_LINK_PIXEL_ALT		2
>
> -struct intel_dsi_device {
> -	unsigned int panel_id;
> -	const char *name;
> -	const struct intel_dsi_dev_ops *dev_ops;
> -	void *dev_priv;
> -};
> -
> -struct intel_dsi_dev_ops {
> -	bool (*init)(struct intel_dsi_device *dsi);
> -
> -	void (*panel_reset)(struct intel_dsi_device *dsi);
> -
> -	void (*disable_panel_power)(struct intel_dsi_device *dsi);
> -
> -	/* This callback must be able to assume DSI commands can be sent */
> -	void (*enable)(struct intel_dsi_device *dsi);
> -
> -	/* This callback must be able to assume DSI commands can be sent */
> -	void (*disable)(struct intel_dsi_device *dsi);
> -
> -	struct drm_display_mode *(*get_modes)(struct intel_dsi_device *dsi);
> -};
> -
>   struct intel_dsi {
>   	struct intel_encoder base;
>
> -	struct intel_dsi_device dev;
> +	struct drm_panel *panel;
>
>   	struct intel_connector *attached_connector;
>
> @@ -130,6 +107,6 @@ extern void vlv_enable_dsi_pll(struct intel_encoder *encoder);
>   extern void vlv_disable_dsi_pll(struct intel_encoder *encoder);
>   extern u32 vlv_get_dsi_pclk(struct intel_encoder *encoder, int pipe_bpp);
>
> -extern struct intel_dsi_dev_ops vbt_generic_dsi_display_ops;
> +struct drm_panel *vbt_panel_init(struct intel_dsi *intel_dsi, u16 panel_id);
>
>   #endif /* _INTEL_DSI_H */
> diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> index 561ec2981dfd..ac7a24dcf7f7 100644
> --- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> +++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> @@ -28,6 +28,7 @@
>   #include <drm/drm_crtc.h>
>   #include <drm/drm_edid.h>
>   #include <drm/i915_drm.h>
> +#include <drm/drm_panel.h>
>   #include <linux/slab.h>
>   #include <video/mipi_display.h>
>   #include <asm/intel-mid.h>
> @@ -37,6 +38,16 @@
>   #include "intel_dsi.h"
>   #include "intel_dsi_cmd.h"
>
> +struct vbt_panel {
> +	struct drm_panel panel;
> +	struct intel_dsi *intel_dsi;
> +};
> +
> +static inline struct vbt_panel *to_vbt_panel(struct drm_panel *panel)
> +{
> +	return container_of(panel, struct vbt_panel, panel);
> +}
> +
>   #define MIPI_TRANSFER_MODE_SHIFT	0
>   #define MIPI_VIRTUAL_CHANNEL_SHIFT	1
>   #define MIPI_PORT_SHIFT			3
> @@ -272,14 +283,103 @@ static void generic_exec_sequence(struct intel_dsi *intel_dsi, const u8 *data)
>   	}
>   }
>
> -static bool generic_init(struct intel_dsi_device *dsi)
> +static int vbt_panel_prepare(struct drm_panel *panel)
> +{
> +	struct vbt_panel *vbt_panel = to_vbt_panel(panel);
> +	struct intel_dsi *intel_dsi = vbt_panel->intel_dsi;
> +	struct drm_device *dev = intel_dsi->base.base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	const u8 *sequence;
> +
> +	sequence = dev_priv->vbt.dsi.sequence[MIPI_SEQ_ASSERT_RESET];
> +	generic_exec_sequence(intel_dsi, sequence);
> +
> +	sequence = dev_priv->vbt.dsi.sequence[MIPI_SEQ_INIT_OTP];
> +	generic_exec_sequence(intel_dsi, sequence);
> +
> +	return 0;
> +}
> +
> +static int vbt_panel_unprepare(struct drm_panel *panel)
> +{
> +	struct vbt_panel *vbt_panel = to_vbt_panel(panel);
> +	struct intel_dsi *intel_dsi = vbt_panel->intel_dsi;
> +	struct drm_device *dev = intel_dsi->base.base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	const u8 *sequence;
> +
> +	sequence = dev_priv->vbt.dsi.sequence[MIPI_SEQ_DEASSERT_RESET];
> +	generic_exec_sequence(intel_dsi, sequence);
> +
> +	return 0;
> +}
> +
> +static int vbt_panel_enable(struct drm_panel *panel)
> +{
> +	struct vbt_panel *vbt_panel = to_vbt_panel(panel);
> +	struct intel_dsi *intel_dsi = vbt_panel->intel_dsi;
> +	struct drm_device *dev = intel_dsi->base.base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	const u8 *sequence;
> +
> +	sequence = dev_priv->vbt.dsi.sequence[MIPI_SEQ_DISPLAY_ON];
> +	generic_exec_sequence(intel_dsi, sequence);
> +
> +	return 0;
> +}
> +
> +static int vbt_panel_disable(struct drm_panel *panel)
> +{
> +	struct vbt_panel *vbt_panel = to_vbt_panel(panel);
> +	struct intel_dsi *intel_dsi = vbt_panel->intel_dsi;
> +	struct drm_device *dev = intel_dsi->base.base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	const u8 *sequence;
> +
> +	sequence = dev_priv->vbt.dsi.sequence[MIPI_SEQ_DISPLAY_OFF];
> +	generic_exec_sequence(intel_dsi, sequence);
> +
> +	return 0;
> +}
> +
> +static int vbt_panel_get_modes(struct drm_panel *panel)
> +{
> +	struct vbt_panel *vbt_panel = to_vbt_panel(panel);
> +	struct intel_dsi *intel_dsi = vbt_panel->intel_dsi;
> +	struct drm_device *dev = intel_dsi->base.base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct drm_display_mode *mode;
> +
> +	if (!panel->connector)
> +		return 0;
> +
> +	mode = drm_mode_duplicate(dev, dev_priv->vbt.lfp_lvds_vbt_mode);
> +	if (!mode)
> +		return 0;
> +
> +	mode->type |= DRM_MODE_TYPE_PREFERRED;
> +
> +	drm_mode_probed_add(panel->connector, mode);
> +
> +	return 1;
> +}
> +
> +static const struct drm_panel_funcs vbt_panel_funcs = {
> +	.disable = vbt_panel_disable,
> +	.unprepare = vbt_panel_unprepare,
> +	.prepare = vbt_panel_prepare,
> +	.enable = vbt_panel_enable,
> +	.get_modes = vbt_panel_get_modes,
> +};
> +
> +struct drm_panel *vbt_panel_init(struct intel_dsi *intel_dsi, u16 panel_id)
>   {
> -	struct intel_dsi *intel_dsi = container_of(dsi, struct intel_dsi, dev);
>   	struct drm_device *dev = intel_dsi->base.base.dev;
>   	struct drm_i915_private *dev_priv = dev->dev_private;
>   	struct mipi_config *mipi_config = dev_priv->vbt.dsi.config;
>   	struct mipi_pps_data *pps = dev_priv->vbt.dsi.pps;
>   	struct drm_display_mode *mode = dev_priv->vbt.lfp_lvds_vbt_mode;
> +	struct vbt_panel *vbt_panel;
>   	u32 bits_per_pixel = 24;
>   	u32 tlpx_ns, extra_byte_count, bitrate, tlpx_ui;
>   	u32 ui_num, ui_den;
> @@ -346,7 +446,7 @@ static bool generic_init(struct intel_dsi_device *dsi)
>   			if (mipi_config->target_burst_mode_freq <
>   								computed_ddr) {
>   				DRM_ERROR("Burst mode freq is less than computed\n");
> -				return false;
> +				return NULL;
>   			}
>
>   			burst_mode_ratio = DIV_ROUND_UP(
> @@ -356,7 +456,7 @@ static bool generic_init(struct intel_dsi_device *dsi)
>   			pclk = DIV_ROUND_UP(pclk * burst_mode_ratio, 100);
>   		} else {
>   			DRM_ERROR("Burst mode target is not set\n");
> -			return false;
> +			return NULL;
>   		}
>   	} else
>   		burst_mode_ratio = 100;
> @@ -557,71 +657,13 @@ static bool generic_init(struct intel_dsi_device *dsi)
>   	intel_dsi->panel_off_delay = pps->panel_off_delay / 10;
>   	intel_dsi->panel_pwr_cycle_delay = pps->panel_power_cycle_delay / 10;
>
> -	return true;
> -}
> -
> -static void generic_panel_reset(struct intel_dsi_device *dsi)
> -{
> -	struct intel_dsi *intel_dsi = container_of(dsi, struct intel_dsi, dev);
> -	struct drm_device *dev = intel_dsi->base.base.dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -
> -	char *sequence = dev_priv->vbt.dsi.sequence[MIPI_SEQ_ASSERT_RESET];
> -
> -	generic_exec_sequence(intel_dsi, sequence);
> -
> -	sequence = dev_priv->vbt.dsi.sequence[MIPI_SEQ_INIT_OTP];
> -	generic_exec_sequence(intel_dsi, sequence);
> -}
> -
> -static void generic_disable_panel_power(struct intel_dsi_device *dsi)
> -{
> -	struct intel_dsi *intel_dsi = container_of(dsi, struct intel_dsi, dev);
> -	struct drm_device *dev = intel_dsi->base.base.dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -
> -	char *sequence = dev_priv->vbt.dsi.sequence[MIPI_SEQ_DEASSERT_RESET];
> -
> -	generic_exec_sequence(intel_dsi, sequence);
> -}
> -
> -static void generic_enable(struct intel_dsi_device *dsi)
> -{
> -	struct intel_dsi *intel_dsi = container_of(dsi, struct intel_dsi, dev);
> -	struct drm_device *dev = intel_dsi->base.base.dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> +	/* This is cheating a bit with the cleanup. */
> +	vbt_panel = devm_kzalloc(dev->dev, sizeof(*vbt_panel), GFP_KERNEL);
>
> -	char *sequence = dev_priv->vbt.dsi.sequence[MIPI_SEQ_DISPLAY_ON];
> +	vbt_panel->intel_dsi = intel_dsi;
> +	drm_panel_init(&vbt_panel->panel);
> +	vbt_panel->panel.funcs = &vbt_panel_funcs;
> +	drm_panel_add(&vbt_panel->panel);
>
> -	generic_exec_sequence(intel_dsi, sequence);
> +	return &vbt_panel->panel;
>   }
> -
> -static void generic_disable(struct intel_dsi_device *dsi)
> -{
> -	struct intel_dsi *intel_dsi = container_of(dsi, struct intel_dsi, dev);
> -	struct drm_device *dev = intel_dsi->base.base.dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -
> -	char *sequence = dev_priv->vbt.dsi.sequence[MIPI_SEQ_DISPLAY_OFF];
> -
> -	generic_exec_sequence(intel_dsi, sequence);
> -}
> -
> -static struct drm_display_mode *generic_get_modes(struct intel_dsi_device *dsi)
> -{
> -	struct intel_dsi *intel_dsi = container_of(dsi, struct intel_dsi, dev);
> -	struct drm_device *dev = intel_dsi->base.base.dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -
> -	dev_priv->vbt.lfp_lvds_vbt_mode->type |= DRM_MODE_TYPE_PREFERRED;
> -	return dev_priv->vbt.lfp_lvds_vbt_mode;
> -}
> -
> -struct intel_dsi_dev_ops vbt_generic_dsi_display_ops = {
> -	.init = generic_init,
> -	.panel_reset = generic_panel_reset,
> -	.disable_panel_power = generic_disable_panel_power,
> -	.enable = generic_enable,
> -	.disable = generic_disable,
> -	.get_modes = generic_get_modes,
> -};
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx