| Message ID | 20161201202925.12220-10-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 01 Dec 2016, Hans de Goede <hdegoede@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@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, > };
Hi, On 02-12-16 11:31, Jani Nikula wrote: > On Thu, 01 Dec 2016, Hans de Goede <hdegoede@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. 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@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, >> }; >
On Fri, 02 Dec 2016, Hans de Goede <hdegoede@redhat.com> wrote: > Hi, > > On 02-12-16 11:31, Jani Nikula wrote: >> On Thu, 01 Dec 2016, Hans de Goede <hdegoede@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. > > 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. Just a quick note here, it's not i915 specific, it's used in other drivers. BR, Jani. > >> 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@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, >>> }; >>
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, };
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. Signed-off-by: Hans de Goede <hdegoede@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(-)