| Message ID | bf11ec1253ea5938c0a047ce01d07fc686bba020.1421410274.git.jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 01/16/2015 05:57 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. > > Signed-off-by: Jani Nikula <jani.nikula@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 | 179 ++++++++++++++++++----------- > 4 files changed, 156 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 fc218b7754b3..19a9955eab0e 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); > + drm_panel_get_modes(intel_dsi->panel); Should be inside the config mutex_lock below. > + > + mutex_lock(&dev->mode_config.mutex); > + 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..204f54df8fe1 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,12 @@ 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]; > + 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; You just missed vbt_panel->intel_dsi = intel_dsi; before returning Regards Shobhit > } > - > -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, > -}; >
On Fri, Jan 23, 2015 at 04:27:46PM +0530, Shobhit Kumar wrote: > On 01/16/2015 05:57 PM, Jani Nikula wrote: > >@@ -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); > >+ drm_panel_get_modes(intel_dsi->panel); > > Should be inside the config mutex_lock below. Why? > >+ > >+ mutex_lock(&dev->mode_config.mutex); > >+ 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); Generally this is single-threaded init code, no one else can touch it. Which means you also never ever need locks. We tend to occasionally sprinkle them around to satisfy general locking checks which are correct at runtime. But at least in tricky cases that means we also stick a comment next to them (see e.g. the various places in i915_irq.c). Merged up to the previous patch, thanks a lot. -Daniel
On 01/23/2015 09:01 PM, Daniel Vetter wrote: > On Fri, Jan 23, 2015 at 04:27:46PM +0530, Shobhit Kumar wrote: >> On 01/16/2015 05:57 PM, Jani Nikula wrote: >>> @@ -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); >>> + drm_panel_get_modes(intel_dsi->panel); >> >> Should be inside the config mutex_lock below. > > Why? Because usually the drm_mode_probed_add gets called in the drm flow with correct locking, but in this case calling get_modes here results in drm_mode_probed_add being called which expects the lock to be taken. Else we will see a big WARN dump. I agree with your comment below but this was mainly to avoid the warning dump. > >>> + >>> + mutex_lock(&dev->mode_config.mutex); >>> + 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); > > Generally this is single-threaded init code, no one else can touch it. > Which means you also never ever need locks. We tend to occasionally > sprinkle them around to satisfy general locking checks which are correct > at runtime. But at least in tricky cases that means we also stick a > comment next to them (see e.g. the various places in i915_irq.c). > > Merged up to the previous patch, thanks a lot. > -Daniel > Regards Shobhit
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 fc218b7754b3..19a9955eab0e 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); + drm_panel_get_modes(intel_dsi->panel); + + mutex_lock(&dev->mode_config.mutex); + 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..204f54df8fe1 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,12 @@ 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]; + 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, -};
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. Signed-off-by: Jani Nikula <jani.nikula@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 | 179 ++++++++++++++++++----------- 4 files changed, 156 insertions(+), 119 deletions(-)