[PATCH] drm/vc4: Enable the DSI module and link before other enables.

[PATCH] drm/vc4: Enable the DSI module and link before other enables.

[Eric Anholt]

We want the DSI PHY to be enabled and the DSI module clocked before a
panel or bridge's prepare() function, since most DSI panel drivers
with a prepare() hook are doing DCS transactions inside of them.

Signed-off-by: Eric Anholt <eric@anholt.net>
Cc: Kevin Quigley <kevin@kquigley.co.uk>
Cc: James Hughes <james.hughes@raspberrypi.org>
Cc: Boris Brezillon <boris.brezillon@bootlin.com>
---

I'm not sure it makes sense to enable CRTCs before encoders on vc4 at
all.  Why start scanning pixels before the encoder is ready to hear
about VSTART?  However, this patch will hopefully unblock people
trying to attach other panels to rpi

 drivers/gpu/drm/vc4/vc4_drv.h |  1 +
 drivers/gpu/drm/vc4/vc4_dsi.c |  3 +--
 drivers/gpu/drm/vc4/vc4_kms.c | 25 +++++++++++++++++++++++++
 3 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
index 554a4e810d5b..e7d7bfc75acd 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.h
+++ b/drivers/gpu/drm/vc4/vc4_drv.h
@@ -711,6 +711,7 @@ int vc4_dpi_debugfs_regs(struct seq_file *m, void *unused);
 /* vc4_dsi.c */
 extern struct platform_driver vc4_dsi_driver;
 int vc4_dsi_debugfs_regs(struct seq_file *m, void *unused);
+void vc4_dsi_prepare(struct drm_encoder *encoder);
 
 /* vc4_fence.c */
 extern const struct dma_fence_ops vc4_fence_ops;
diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c
index 8aa897835118..88471731e066 100644
--- a/drivers/gpu/drm/vc4/vc4_dsi.c
+++ b/drivers/gpu/drm/vc4/vc4_dsi.c
@@ -875,7 +875,7 @@ static bool vc4_dsi_encoder_mode_fixup(struct drm_encoder *encoder,
 	return true;
 }
 
-static void vc4_dsi_encoder_enable(struct drm_encoder *encoder)
+void vc4_dsi_prepare(struct drm_encoder *encoder)
 {
 	struct drm_display_mode *mode = &encoder->crtc->state->adjusted_mode;
 	struct vc4_dsi_encoder *vc4_encoder = to_vc4_dsi_encoder(encoder);
@@ -1345,7 +1345,6 @@ static const struct mipi_dsi_host_ops vc4_dsi_host_ops = {
 
 static const struct drm_encoder_helper_funcs vc4_dsi_encoder_helper_funcs = {
 	.disable = vc4_dsi_encoder_disable,
-	.enable = vc4_dsi_encoder_enable,
 	.mode_fixup = vc4_dsi_encoder_mode_fixup,
 };
 
diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index 8a411e5f8776..7e9b52ba3448 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -140,6 +140,9 @@ vc4_atomic_complete_commit(struct drm_atomic_state *state)
 {
 	struct drm_device *dev = state->dev;
 	struct vc4_dev *vc4 = to_vc4_dev(dev);
+	struct drm_connector *connector;
+	struct drm_connector_state *new_conn_state;
+	int i;
 
 	drm_atomic_helper_wait_for_fences(dev, state, false);
 
@@ -151,6 +154,28 @@ vc4_atomic_complete_commit(struct drm_atomic_state *state)
 
 	drm_atomic_helper_commit_planes(dev, state, 0);
 
+	/* Enable DSI link. */
+	for_each_new_connector_in_state(state, connector, new_conn_state, i) {
+		struct drm_encoder *encoder;
+		struct vc4_encoder *vc4_encoder;
+
+		if (!new_conn_state->best_encoder)
+			continue;
+
+		if (!new_conn_state->crtc->state->active ||
+		    !drm_atomic_crtc_needs_modeset(new_conn_state->crtc->state))
+			continue;
+
+		(void)connector;
+		encoder = new_conn_state->best_encoder;
+		vc4_encoder = to_vc4_encoder(encoder);
+
+		if (vc4_encoder->type == VC4_ENCODER_TYPE_DSI0 ||
+		    vc4_encoder->type == VC4_ENCODER_TYPE_DSI1) {
+			vc4_dsi_prepare(encoder);
+		}
+	}
+
 	drm_atomic_helper_commit_modeset_enables(dev, state);
 
 	/* Make sure that drm_atomic_helper_wait_for_vblanks()
-- 
2.17.0

Re: [PATCH] drm/vc4: Enable the DSI module and link before other enables.

[Andrzej Hajda]

On 04.06.2018 21:44, Eric Anholt wrote:
> We want the DSI PHY to be enabled and the DSI module clocked before a
> panel or bridge's prepare() function, since most DSI panel drivers
> with a prepare() hook are doing DCS transactions inside of them.
>
> Signed-off-by: Eric Anholt <eric@anholt.net>
> Cc: Kevin Quigley <kevin@kquigley.co.uk>
> Cc: James Hughes <james.hughes@raspberrypi.org>
> Cc: Boris Brezillon <boris.brezillon@bootlin.com>
> ---
>
> I'm not sure it makes sense to enable CRTCs before encoders on vc4 at
> all.  Why start scanning pixels before the encoder is ready to hear
> about VSTART?  However, this patch will hopefully unblock people
> trying to attach other panels to rpi

But this patch is about enabling encoder before panel/bridge prepare. Or
am I missing something.
Anyway I would be more precise here, MIPI-DSI bus is used for two things:
- control bus - accessing DSI device (configuration/detection,...),
- video data bus - sending video stream.

I suspect in prepare/pre_enable phase of DSI panel/bridge only control
bus should be functional, video stream transfer does not make sense.
So the best solution (I guess) would be to split DSI-encoder enable
sequence into control bus enable and video transfer enable parts and
call the former before 1st transfer request from device and the latter
in encoder enable callback. Of course there will be a problem then with
disabling sequence, ie stopping video stream should be performed in
encoder's disable, but when we should disable control bus? If we do it
in encoder's disable callback we could not perform transfers in
post_disable cb of the bridge - in most cases (maybe all currently
present in kernel) it will work, but it does not look ideal.
All this recalls me discussion that hardwiring bridge callbacks into drm
core is problematic and maybe it would be better to call downstream
callbacks explicitly from upstream element - the callback order should
depend on the local bus protocol, and should not be fixed in drm core.

Regards
Andrzej

>
>  drivers/gpu/drm/vc4/vc4_drv.h |  1 +
>  drivers/gpu/drm/vc4/vc4_dsi.c |  3 +--
>  drivers/gpu/drm/vc4/vc4_kms.c | 25 +++++++++++++++++++++++++
>  3 files changed, 27 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
> index 554a4e810d5b..e7d7bfc75acd 100644
> --- a/drivers/gpu/drm/vc4/vc4_drv.h
> +++ b/drivers/gpu/drm/vc4/vc4_drv.h
> @@ -711,6 +711,7 @@ int vc4_dpi_debugfs_regs(struct seq_file *m, void *unused);
>  /* vc4_dsi.c */
>  extern struct platform_driver vc4_dsi_driver;
>  int vc4_dsi_debugfs_regs(struct seq_file *m, void *unused);
> +void vc4_dsi_prepare(struct drm_encoder *encoder);
>  
>  /* vc4_fence.c */
>  extern const struct dma_fence_ops vc4_fence_ops;
> diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c
> index 8aa897835118..88471731e066 100644
> --- a/drivers/gpu/drm/vc4/vc4_dsi.c
> +++ b/drivers/gpu/drm/vc4/vc4_dsi.c
> @@ -875,7 +875,7 @@ static bool vc4_dsi_encoder_mode_fixup(struct drm_encoder *encoder,
>  	return true;
>  }
>  
> -static void vc4_dsi_encoder_enable(struct drm_encoder *encoder)
> +void vc4_dsi_prepare(struct drm_encoder *encoder)
>  {
>  	struct drm_display_mode *mode = &encoder->crtc->state->adjusted_mode;
>  	struct vc4_dsi_encoder *vc4_encoder = to_vc4_dsi_encoder(encoder);
> @@ -1345,7 +1345,6 @@ static const struct mipi_dsi_host_ops vc4_dsi_host_ops = {
>  
>  static const struct drm_encoder_helper_funcs vc4_dsi_encoder_helper_funcs = {
>  	.disable = vc4_dsi_encoder_disable,
> -	.enable = vc4_dsi_encoder_enable,
>  	.mode_fixup = vc4_dsi_encoder_mode_fixup,
>  };
>  
> diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
> index 8a411e5f8776..7e9b52ba3448 100644
> --- a/drivers/gpu/drm/vc4/vc4_kms.c
> +++ b/drivers/gpu/drm/vc4/vc4_kms.c
> @@ -140,6 +140,9 @@ vc4_atomic_complete_commit(struct drm_atomic_state *state)
>  {
>  	struct drm_device *dev = state->dev;
>  	struct vc4_dev *vc4 = to_vc4_dev(dev);
> +	struct drm_connector *connector;
> +	struct drm_connector_state *new_conn_state;
> +	int i;
>  
>  	drm_atomic_helper_wait_for_fences(dev, state, false);
>  
> @@ -151,6 +154,28 @@ vc4_atomic_complete_commit(struct drm_atomic_state *state)
>  
>  	drm_atomic_helper_commit_planes(dev, state, 0);
>  
> +	/* Enable DSI link. */
> +	for_each_new_connector_in_state(state, connector, new_conn_state, i) {
> +		struct drm_encoder *encoder;
> +		struct vc4_encoder *vc4_encoder;
> +
> +		if (!new_conn_state->best_encoder)
> +			continue;
> +
> +		if (!new_conn_state->crtc->state->active ||
> +		    !drm_atomic_crtc_needs_modeset(new_conn_state->crtc->state))
> +			continue;
> +
> +		(void)connector;
> +		encoder = new_conn_state->best_encoder;
> +		vc4_encoder = to_vc4_encoder(encoder);
> +
> +		if (vc4_encoder->type == VC4_ENCODER_TYPE_DSI0 ||
> +		    vc4_encoder->type == VC4_ENCODER_TYPE_DSI1) {
> +			vc4_dsi_prepare(encoder);
> +		}
> +	}
> +
>  	drm_atomic_helper_commit_modeset_enables(dev, state);
>  
>  	/* Make sure that drm_atomic_helper_wait_for_vblanks()

Re: [PATCH] drm/vc4: Enable the DSI module and link before other enables.

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 2804 bytes --]

Andrzej Hajda <a.hajda@samsung.com> writes:

> On 04.06.2018 21:44, Eric Anholt wrote:
>> We want the DSI PHY to be enabled and the DSI module clocked before a
>> panel or bridge's prepare() function, since most DSI panel drivers
>> with a prepare() hook are doing DCS transactions inside of them.
>>
>> Signed-off-by: Eric Anholt <eric@anholt.net>
>> Cc: Kevin Quigley <kevin@kquigley.co.uk>
>> Cc: James Hughes <james.hughes@raspberrypi.org>
>> Cc: Boris Brezillon <boris.brezillon@bootlin.com>
>> ---
>>
>> I'm not sure it makes sense to enable CRTCs before encoders on vc4 at
>> all.  Why start scanning pixels before the encoder is ready to hear
>> about VSTART?  However, this patch will hopefully unblock people
>> trying to attach other panels to rpi
>
> But this patch is about enabling encoder before panel/bridge prepare. Or
> am I missing something.
> Anyway I would be more precise here, MIPI-DSI bus is used for two things:
> - control bus - accessing DSI device (configuration/detection,...),
> - video data bus - sending video stream.
>
> I suspect in prepare/pre_enable phase of DSI panel/bridge only control
> bus should be functional, video stream transfer does not make sense.
> So the best solution (I guess) would be to split DSI-encoder enable
> sequence into control bus enable and video transfer enable parts and
> call the former before 1st transfer request from device and the latter
> in encoder enable callback. Of course there will be a problem then with
> disabling sequence, ie stopping video stream should be performed in
> encoder's disable, but when we should disable control bus? If we do it
> in encoder's disable callback we could not perform transfers in
> post_disable cb of the bridge - in most cases (maybe all currently
> present in kernel) it will work, but it does not look ideal.
> All this recalls me discussion that hardwiring bridge callbacks into drm
> core is problematic and maybe it would be better to call downstream
> callbacks explicitly from upstream element - the callback order should
> depend on the local bus protocol, and should not be fixed in drm core.

It does seem like for DSI encoders they generally would need to be able
to control when the call down to the bridge happens.  However, from my
experience with panels, drivers are bad about calling both prepare and
enable, if their particular panel only cares about one of them.  So, how
about:

- encoders can call drm_bridge_disable_midlayer_calls() (any naming
  suggestions?) to flag this bridge as not wanting the calls from the
  atomic helpers.

- atomic helpers WARN if bridge midlayer_calls flag was set and
  drm_bridge_pre_enable/enable/disable/post_disable failed to be called
  by the encoder

- drm_bridge_detach() clears disable_midlayer_calls flag for the next
  encoder

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/vc4: Enable the DSI module and link before other enables.

[Eric Anholt]

[-- Attachment #1: Type: text/plain, Size: 2804 bytes --]

Andrzej Hajda <a.hajda@samsung.com> writes:

> On 04.06.2018 21:44, Eric Anholt wrote:
>> We want the DSI PHY to be enabled and the DSI module clocked before a
>> panel or bridge's prepare() function, since most DSI panel drivers
>> with a prepare() hook are doing DCS transactions inside of them.
>>
>> Signed-off-by: Eric Anholt <eric@anholt.net>
>> Cc: Kevin Quigley <kevin@kquigley.co.uk>
>> Cc: James Hughes <james.hughes@raspberrypi.org>
>> Cc: Boris Brezillon <boris.brezillon@bootlin.com>
>> ---
>>
>> I'm not sure it makes sense to enable CRTCs before encoders on vc4 at
>> all.  Why start scanning pixels before the encoder is ready to hear
>> about VSTART?  However, this patch will hopefully unblock people
>> trying to attach other panels to rpi
>
> But this patch is about enabling encoder before panel/bridge prepare. Or
> am I missing something.
> Anyway I would be more precise here, MIPI-DSI bus is used for two things:
> - control bus - accessing DSI device (configuration/detection,...),
> - video data bus - sending video stream.
>
> I suspect in prepare/pre_enable phase of DSI panel/bridge only control
> bus should be functional, video stream transfer does not make sense.
> So the best solution (I guess) would be to split DSI-encoder enable
> sequence into control bus enable and video transfer enable parts and
> call the former before 1st transfer request from device and the latter
> in encoder enable callback. Of course there will be a problem then with
> disabling sequence, ie stopping video stream should be performed in
> encoder's disable, but when we should disable control bus? If we do it
> in encoder's disable callback we could not perform transfers in
> post_disable cb of the bridge - in most cases (maybe all currently
> present in kernel) it will work, but it does not look ideal.
> All this recalls me discussion that hardwiring bridge callbacks into drm
> core is problematic and maybe it would be better to call downstream
> callbacks explicitly from upstream element - the callback order should
> depend on the local bus protocol, and should not be fixed in drm core.

It does seem like for DSI encoders they generally would need to be able
to control when the call down to the bridge happens.  However, from my
experience with panels, drivers are bad about calling both prepare and
enable, if their particular panel only cares about one of them.  So, how
about:

- encoders can call drm_bridge_disable_midlayer_calls() (any naming
  suggestions?) to flag this bridge as not wanting the calls from the
  atomic helpers.

- atomic helpers WARN if bridge midlayer_calls flag was set and
  drm_bridge_pre_enable/enable/disable/post_disable failed to be called
  by the encoder

- drm_bridge_detach() clears disable_midlayer_calls flag for the next
  encoder

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH] drm/vc4: Enable the DSI module and link before other enables.

[Andrzej Hajda]

On 05.06.2018 20:49, Eric Anholt wrote:
> Andrzej Hajda <a.hajda@samsung.com> writes:
>
>> On 04.06.2018 21:44, Eric Anholt wrote:
>>> We want the DSI PHY to be enabled and the DSI module clocked before a
>>> panel or bridge's prepare() function, since most DSI panel drivers
>>> with a prepare() hook are doing DCS transactions inside of them.
>>>
>>> Signed-off-by: Eric Anholt <eric@anholt.net>
>>> Cc: Kevin Quigley <kevin@kquigley.co.uk>
>>> Cc: James Hughes <james.hughes@raspberrypi.org>
>>> Cc: Boris Brezillon <boris.brezillon@bootlin.com>
>>> ---
>>>
>>> I'm not sure it makes sense to enable CRTCs before encoders on vc4 at
>>> all.  Why start scanning pixels before the encoder is ready to hear
>>> about VSTART?  However, this patch will hopefully unblock people
>>> trying to attach other panels to rpi
>> But this patch is about enabling encoder before panel/bridge prepare. Or
>> am I missing something.
>> Anyway I would be more precise here, MIPI-DSI bus is used for two things:
>> - control bus - accessing DSI device (configuration/detection,...),
>> - video data bus - sending video stream.
>>
>> I suspect in prepare/pre_enable phase of DSI panel/bridge only control
>> bus should be functional, video stream transfer does not make sense.
>> So the best solution (I guess) would be to split DSI-encoder enable
>> sequence into control bus enable and video transfer enable parts and
>> call the former before 1st transfer request from device and the latter
>> in encoder enable callback. Of course there will be a problem then with
>> disabling sequence, ie stopping video stream should be performed in
>> encoder's disable, but when we should disable control bus? If we do it
>> in encoder's disable callback we could not perform transfers in
>> post_disable cb of the bridge - in most cases (maybe all currently
>> present in kernel) it will work, but it does not look ideal.
>> All this recalls me discussion that hardwiring bridge callbacks into drm
>> core is problematic and maybe it would be better to call downstream
>> callbacks explicitly from upstream element - the callback order should
>> depend on the local bus protocol, and should not be fixed in drm core.
> It does seem like for DSI encoders they generally would need to be able
> to control when the call down to the bridge happens.  However, from my
> experience with panels, drivers are bad about calling both prepare and
> enable, if their particular panel only cares about one of them.  So, how
> about:
>
> - encoders can call drm_bridge_disable_midlayer_calls() (any naming
>   suggestions?) to flag this bridge as not wanting the calls from the
>   atomic helpers.
>
> - atomic helpers WARN if bridge midlayer_calls flag was set and
>   drm_bridge_pre_enable/enable/disable/post_disable failed to be called
>   by the encoder
>
> - drm_bridge_detach() clears disable_midlayer_calls flag for the next
>   encoder


I like the idea, I guess the flag should be placed in "struct
drm_bridge". Since I plan to propose flag to avoid device links in
panels/bridges maybe it would be good to set flags directly, or by
helper similar to irq_set_status_flags, instead of creating separate
helper for each flag.
I am not sure about warns from atomic helpers, maybe it would be enough
to track and verify state transitions of bridges similarly to the ones
proposed (and abandoned?) by Sean [1].

And little off-topic: all these duplication of
ideas/code/functionalities added to/from panels from/to bridges and this
crazy code:
    sink = drm_of_find_panel_or_bridge...
    if (sink is panel)
        do_something
    else if (sink is bridge)
        do_something_similar_but_with_different_functions
laying in multiple encoders/bridges, provokes me to raise again a
question, wouldn't be better to merge drm_bridge and drm_panel into one
drm_sink object.

[1]: https://marc.info/?l=dri-devel&m=150827480716580

Regards
Andrzej

RE: [PATCH] drm/vc4: Enable the DSI module and link before other enables.

[Kevin Quigley]

Hi,

When we talk about control bus enable and video transfer enable for DSI,  there is a mode in which control commands can be sent, interleaved with the video.  This is often done at the end-of-frame and/or end of line.  I've seen this handled in some other DSI device drivers.  In these cases, the driver has to look at the frame timing in the video and carefully select the number of control bytes which can be sent during the blanking periods.  The number of bytes which can be sent, is very much dependant on the panel-size and clock frequencies... (which determines the blanking time).    I'm not sure how widely this is a feature in the DSI encoder HW, and/or implemented purely in the drivers.

I have seen panel drivers - that send commands along DSI, whilst video is streaming. They do this for e.g. to change panel brightness, etc...  (which is usually only a small number of bytes).   For info, a typical panel start-up scenario is:
- Enable/configure panel HW
- Start Video frames (allowing for the HW & panel's internal regulators to "settle)
- Switch on the Display

To conclude:  I think that DSI encoder devices need to be able to support sending control data (LPDA) - concurrently with video frames.

This might make a difference when talking about how to setup and tear down these devices.   For one panel we're working with,  we have 250-300 bytes of configuration data to send down; so we would prefer to ensure that's sent -before- we enable the video stream to the panel device.


Regards


Kevin


-----Original Message-----
From: Andrzej Hajda [mailto:a.hajda@samsung.com] 
Sent: 05 June 2018 09:26
To: Eric Anholt; dri-devel@lists.freedesktop.org
Cc: Kevin Quigley; Boris Brezillon; linux-kernel@vger.kernel.org; James Hughes; Archit Taneja
Subject: Re: [PATCH] drm/vc4: Enable the DSI module and link before other enables.

On 04.06.2018 21:44, Eric Anholt wrote:
> We want the DSI PHY to be enabled and the DSI module clocked before a 
> panel or bridge's prepare() function, since most DSI panel drivers 
> with a prepare() hook are doing DCS transactions inside of them.
>
> Signed-off-by: Eric Anholt <eric@anholt.net>
> Cc: Kevin Quigley <kevin@kquigley.co.uk>
> Cc: James Hughes <james.hughes@raspberrypi.org>
> Cc: Boris Brezillon <boris.brezillon@bootlin.com>
> ---
>
> I'm not sure it makes sense to enable CRTCs before encoders on vc4 at 
> all.  Why start scanning pixels before the encoder is ready to hear 
> about VSTART?  However, this patch will hopefully unblock people 
> trying to attach other panels to rpi

But this patch is about enabling encoder before panel/bridge prepare. Or am I missing something.
Anyway I would be more precise here, MIPI-DSI bus is used for two things:
- control bus - accessing DSI device (configuration/detection,...),
- video data bus - sending video stream.

I suspect in prepare/pre_enable phase of DSI panel/bridge only control bus should be functional, video stream transfer does not make sense.
So the best solution (I guess) would be to split DSI-encoder enable sequence into control bus enable and video transfer enable parts and call the former before 1st transfer request from device and the latter in encoder enable callback. Of course there will be a problem then with disabling sequence, ie stopping video stream should be performed in encoder's disable, but when we should disable control bus? If we do it in encoder's disable callback we could not perform transfers in post_disable cb of the bridge - in most cases (maybe all currently present in kernel) it will work, but it does not look ideal.
All this recalls me discussion that hardwiring bridge callbacks into drm core is problematic and maybe it would be better to call downstream callbacks explicitly from upstream element - the callback order should depend on the local bus protocol, and should not be fixed in drm core.

Regards
Andrzej

>
>  drivers/gpu/drm/vc4/vc4_drv.h |  1 +
>  drivers/gpu/drm/vc4/vc4_dsi.c |  3 +--  drivers/gpu/drm/vc4/vc4_kms.c 
> | 25 +++++++++++++++++++++++++
>  3 files changed, 27 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_drv.h 
> b/drivers/gpu/drm/vc4/vc4_drv.h index 554a4e810d5b..e7d7bfc75acd 
> 100644
> --- a/drivers/gpu/drm/vc4/vc4_drv.h
> +++ b/drivers/gpu/drm/vc4/vc4_drv.h
> @@ -711,6 +711,7 @@ int vc4_dpi_debugfs_regs(struct seq_file *m, void 
> *unused);
>  /* vc4_dsi.c */
>  extern struct platform_driver vc4_dsi_driver;  int 
> vc4_dsi_debugfs_regs(struct seq_file *m, void *unused);
> +void vc4_dsi_prepare(struct drm_encoder *encoder);
>  
>  /* vc4_fence.c */
>  extern const struct dma_fence_ops vc4_fence_ops; diff --git 
> a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c index 
> 8aa897835118..88471731e066 100644
> --- a/drivers/gpu/drm/vc4/vc4_dsi.c
> +++ b/drivers/gpu/drm/vc4/vc4_dsi.c
> @@ -875,7 +875,7 @@ static bool vc4_dsi_encoder_mode_fixup(struct drm_encoder *encoder,
>  	return true;
>  }
>  
> -static void vc4_dsi_encoder_enable(struct drm_encoder *encoder)
> +void vc4_dsi_prepare(struct drm_encoder *encoder)
>  {
>  	struct drm_display_mode *mode = &encoder->crtc->state->adjusted_mode;
>  	struct vc4_dsi_encoder *vc4_encoder = to_vc4_dsi_encoder(encoder); 
> @@ -1345,7 +1345,6 @@ static const struct mipi_dsi_host_ops 
> vc4_dsi_host_ops = {
>  
>  static const struct drm_encoder_helper_funcs vc4_dsi_encoder_helper_funcs = {
>  	.disable = vc4_dsi_encoder_disable,
> -	.enable = vc4_dsi_encoder_enable,
>  	.mode_fixup = vc4_dsi_encoder_mode_fixup,  };
>  
> diff --git a/drivers/gpu/drm/vc4/vc4_kms.c 
> b/drivers/gpu/drm/vc4/vc4_kms.c index 8a411e5f8776..7e9b52ba3448 
> 100644
> --- a/drivers/gpu/drm/vc4/vc4_kms.c
> +++ b/drivers/gpu/drm/vc4/vc4_kms.c
> @@ -140,6 +140,9 @@ vc4_atomic_complete_commit(struct drm_atomic_state 
> *state)  {
>  	struct drm_device *dev = state->dev;
>  	struct vc4_dev *vc4 = to_vc4_dev(dev);
> +	struct drm_connector *connector;
> +	struct drm_connector_state *new_conn_state;
> +	int i;
>  
>  	drm_atomic_helper_wait_for_fences(dev, state, false);
>  
> @@ -151,6 +154,28 @@ vc4_atomic_complete_commit(struct 
> drm_atomic_state *state)
>  
>  	drm_atomic_helper_commit_planes(dev, state, 0);
>  
> +	/* Enable DSI link. */
> +	for_each_new_connector_in_state(state, connector, new_conn_state, i) {
> +		struct drm_encoder *encoder;
> +		struct vc4_encoder *vc4_encoder;
> +
> +		if (!new_conn_state->best_encoder)
> +			continue;
> +
> +		if (!new_conn_state->crtc->state->active ||
> +		    !drm_atomic_crtc_needs_modeset(new_conn_state->crtc->state))
> +			continue;
> +
> +		(void)connector;
> +		encoder = new_conn_state->best_encoder;
> +		vc4_encoder = to_vc4_encoder(encoder);
> +
> +		if (vc4_encoder->type == VC4_ENCODER_TYPE_DSI0 ||
> +		    vc4_encoder->type == VC4_ENCODER_TYPE_DSI1) {
> +			vc4_dsi_prepare(encoder);
> +		}
> +	}
> +
>  	drm_atomic_helper_commit_modeset_enables(dev, state);
>  
>  	/* Make sure that drm_atomic_helper_wait_for_vblanks()

RE: [PATCH] drm/vc4: Enable the DSI module and link before other enables.

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 997 bytes --]

Kevin Quigley <kevin@kquigley.co.uk> writes:

> Hi,
>
> When we talk about control bus enable and video transfer enable for
> DSI, there is a mode in which control commands can be sent,
> interleaved with the video.  This is often done at the end-of-frame
> and/or end of line.  I've seen this handled in some other DSI device
> drivers.  In these cases, the driver has to look at the frame timing
> in the video and carefully select the number of control bytes which
> can be sent during the blanking periods.  The number of bytes which
> can be sent, is very much dependant on the panel-size and clock
> frequencies... (which determines the blanking time).  I'm not sure how
> widely this is a feature in the DSI encoder HW, and/or implemented
> purely in the drivers.

That scheduling is done by the VC4 DSI encoder.

> To conclude: I think that DSI encoder devices need to be able to
> support sending control data (LPDA) - concurrently with video frames.

I believe that should work already.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

RE: [PATCH] drm/vc4: Enable the DSI module and link before other enables.

[Eric Anholt]

[-- Attachment #1: Type: text/plain, Size: 997 bytes --]

Kevin Quigley <kevin@kquigley.co.uk> writes:

> Hi,
>
> When we talk about control bus enable and video transfer enable for
> DSI, there is a mode in which control commands can be sent,
> interleaved with the video.  This is often done at the end-of-frame
> and/or end of line.  I've seen this handled in some other DSI device
> drivers.  In these cases, the driver has to look at the frame timing
> in the video and carefully select the number of control bytes which
> can be sent during the blanking periods.  The number of bytes which
> can be sent, is very much dependant on the panel-size and clock
> frequencies... (which determines the blanking time).  I'm not sure how
> widely this is a feature in the DSI encoder HW, and/or implemented
> purely in the drivers.

That scheduling is done by the VC4 DSI encoder.

> To conclude: I think that DSI encoder devices need to be able to
> support sending control data (LPDA) - concurrently with video frames.

I believe that should work already.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]