On Fri, 02 Dec 2016, Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
> 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.
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@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,
>>> };
>>
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
