On Thu, 01 Dec 2016, Hans de Goede <hdegoede at redhat.com> wrote:
> The drm_panel_enable/disable and drm_panel_prepare/unprepare calls are
> not fine grained enough to abstract all the different steps we need to
> take (and VBT sequences we need to exec) properly. So simply remove the
> panel _enable/disable and prepare/unprepare callbacks and instead
> export intel_dsi_exec_vbt_sequence() from intel_dsi_panel_vbt.c
> and call that from intel_dsi_enable/disable().
>
> No functional changes.
No functional changes, but this is quite a big design change between
intel_dsi.c and intel_dsi_panel_vbt.c. Originally the idea was to
separate the dsi core code and the panel specific code, letting one
write a panel driver that was not based on values in VBT. This patch
changes that.
Now I'm not sure if there'll ever be a panel driver not based on VBT,
and perhaps the current drm_panel based interface between two is not
enough in the end, nor prettiest. But after this patch, we might just as
well throw out the drm_panel interface, and refactor the division
between the two files completely. Indeed, if we accept the direction set
in this patch, that refactoring would be the logical next step.
I have not made up my mind about this yet. An alternative would be to
amend the drm_panel interface to achieve the kind of granularity you're
after in the follow-up patches. Indeed, such patches have been sent in
the past.
BR,
Jani.
>
> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
> ---
> drivers/gpu/drm/i915/intel_dsi.c | 14 +++++++---
> drivers/gpu/drm/i915/intel_dsi.h | 3 +++
> drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 43
> ++----------------------------
> 3 files changed, 15 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dsi.c
> b/drivers/gpu/drm/i915/intel_dsi.c
> index 85b748d..cf761e8 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.c
> +++ b/drivers/gpu/drm/i915/intel_dsi.c
> @@ -656,7 +656,10 @@ static void intel_dsi_pre_enable(struct intel_encoder
> *encoder,
> /* put device in ready state */
> intel_dsi_device_ready(encoder);
>
> - drm_panel_prepare(intel_dsi->panel);
> + intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_ASSERT_RESET);
> + intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_POWER_ON);
> + intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DEASSERT_RESET);
> + intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_INIT_OTP);
>
> /* Enable port in pre-enable phase itself because as per hw team
> * recommendation, port should be enabled befor plane & pipe */
> @@ -669,7 +672,8 @@ static void intel_dsi_pre_enable(struct intel_encoder
> *encoder,
> dpi_send_cmd(intel_dsi, TURN_ON, false, port);
> msleep(100);
>
> - drm_panel_enable(intel_dsi->panel);
> + intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DISPLAY_ON);
> + intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_BACKLIGHT_ON);
>
> intel_dsi_port_enable(encoder);
> }
> @@ -732,7 +736,8 @@ static void intel_dsi_post_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
> */
> - drm_panel_disable(intel_dsi->panel);
> + intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_BACKLIGHT_OFF);
> + intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DISPLAY_OFF);
>
> intel_dsi_clear_device_ready(encoder);
>
> @@ -746,7 +751,8 @@ static void intel_dsi_post_disable(struct intel_encoder
> *encoder,
> I915_WRITE(DSPCLK_GATE_D, val);
> }
>
> - drm_panel_unprepare(intel_dsi->panel);
> + intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_ASSERT_RESET);
> + intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_POWER_OFF);
>
> msleep(intel_dsi->panel_off_delay);
>
> diff --git a/drivers/gpu/drm/i915/intel_dsi.h
> b/drivers/gpu/drm/i915/intel_dsi.h
> index d567823..5486491 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.h
> +++ b/drivers/gpu/drm/i915/intel_dsi.h
> @@ -132,6 +132,9 @@ static inline struct intel_dsi *enc_to_intel_dsi(struct
> drm_encoder *encoder)
>
> void wait_for_dsi_fifo_empty(struct intel_dsi *intel_dsi, enum port port);
>
> +void intel_dsi_exec_vbt_sequence(struct intel_dsi *intel_dsi,
> + enum mipi_seq seq_id);
> +
> bool intel_dsi_pll_is_enabled(struct drm_i915_private *dev_priv);
> int intel_compute_dsi_pll(struct intel_encoder *encoder,
> struct intel_crtc_state *config);
> diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> index b5a02c6..f71f913 100644
> --- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> +++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> @@ -400,10 +400,9 @@ static const char *sequence_name(enum mipi_seq seq_id)
> return "(unknown)";
> }
>
> -static void generic_exec_sequence(struct drm_panel *panel, enum mipi_seq
> seq_id)
> +void intel_dsi_exec_vbt_sequence(struct intel_dsi *intel_dsi,
> + enum mipi_seq seq_id)
> {
> - struct vbt_panel *vbt_panel = to_vbt_panel(panel);
> - struct intel_dsi *intel_dsi = vbt_panel->intel_dsi;
> struct drm_i915_private *dev_priv = to_i915(intel_dsi->base.base.dev);
> const u8 *data;
> fn_mipi_elem_exec mipi_elem_exec;
> @@ -467,40 +466,6 @@ static void generic_exec_sequence(struct drm_panel
> *panel, enum mipi_seq seq_id)
> }
> }
>
> -static int vbt_panel_prepare(struct drm_panel *panel)
> -{
> - generic_exec_sequence(panel, MIPI_SEQ_ASSERT_RESET);
> - generic_exec_sequence(panel, MIPI_SEQ_POWER_ON);
> - generic_exec_sequence(panel, MIPI_SEQ_DEASSERT_RESET);
> - generic_exec_sequence(panel, MIPI_SEQ_INIT_OTP);
> -
> - return 0;
> -}
> -
> -static int vbt_panel_unprepare(struct drm_panel *panel)
> -{
> - generic_exec_sequence(panel, MIPI_SEQ_ASSERT_RESET);
> - generic_exec_sequence(panel, MIPI_SEQ_POWER_OFF);
> -
> - return 0;
> -}
> -
> -static int vbt_panel_enable(struct drm_panel *panel)
> -{
> - generic_exec_sequence(panel, MIPI_SEQ_DISPLAY_ON);
> - generic_exec_sequence(panel, MIPI_SEQ_BACKLIGHT_ON);
> -
> - return 0;
> -}
> -
> -static int vbt_panel_disable(struct drm_panel *panel)
> -{
> - generic_exec_sequence(panel, MIPI_SEQ_BACKLIGHT_OFF);
> - generic_exec_sequence(panel, MIPI_SEQ_DISPLAY_OFF);
> -
> - return 0;
> -}
> -
> static int vbt_panel_get_modes(struct drm_panel *panel)
> {
> struct vbt_panel *vbt_panel = to_vbt_panel(panel);
> @@ -524,10 +489,6 @@ static int vbt_panel_get_modes(struct drm_panel *panel)
> }
>
> 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,
> };
--
Jani Nikula, Intel Open Source Technology Center
