Hi,
On 02-12-16 11:31, Jani Nikula wrote:
On Thu, 01 Dec 2016, Hans de Goede <hdegoede@xxxxxxxxxx> 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.
True, I did this because adding a callback per init step to the
drm_panel interface felt like it will result in a game of
whack a mole (adding more and more callbacks). See the end of
this mail for a proposal with leaves the drm_panel interface
in place, while also avoiding this problem.
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.
I see, I was not aware of this history.
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.
Is intel_dsi.c the only user of the drm_panel interface ? The name
suggests it is not intel specific.
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.
How about we add a "drm_panel_exec_sequence" callback to the
drm_panel interface, which takes an enum argument which (de)init
step to do ?
Then we can easily add extra init steps later by extending the
enum, and panel implementations which do not care about certain
steps can simply treat these as nops. This avoids the need
to add a ton of callbacks to the drm_panel struct.
If there are no other users, we could then also remove the
enable/disable and prepare/unprepare pairs from drm_panel
now, I left those in place assuming that intel_dsi.c was
not the only user of drm_panel.
Regards,
Hans
BR,
Jani.
Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
---
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,
};
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
