Hi Thierry,
On 07/14/2014 06:41 PM, Thierry Reding wrote:
> On Mon, Jul 14, 2014 at 06:22:39PM +0900, YoungJun Cho wrote:
>> Hi Thierry,
>>
>> Thank you for comment.
>>
>> On 07/10/2014 04:38 PM, Thierry Reding wrote:
>>> On Thu, Jul 10, 2014 at 10:06:07AM +0900, YoungJun Cho wrote:
>>>> On 07/10/2014 12:22 AM, Thierry Reding wrote:
>>>>> On Tue, Jul 08, 2014 at 09:39:38AM +0900, YoungJun Cho wrote:
>>>>>> To support LCD I80 interface, the DSI host calls this function
>>>>>> to notify the panel tearing effect synchronization signal to
>>>>>> the CRTC device manager to trigger to transfer video image.
>>>>>>
>>>>>> Signed-off-by: YoungJun Cho <yj44.cho at samsung.com>
>>>>>> Acked-by: Inki Dae <inki.dae at samsung.com>
>>>>>> Acked-by: Kyungmin Park <kyungmin.park at samsung.com>
>>>>>> ---
>>>>>> drivers/gpu/drm/exynos/exynos_drm_dsi.c | 11 +++++++++++
>>>>>> include/drm/drm_mipi_dsi.h | 7 +++++++
>>>>>> 2 files changed, 18 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>>>>>> b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>>>>>> index dad543a..76e34ca 100644
>>>>>> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>>>>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>>>>>> @@ -24,6 +24,7 @@
>>>>>> #include <video/mipi_display.h>
>>>>>> #include <video/videomode.h>
>>>>>>
>>>>>> +#include "exynos_drm_crtc.h"
>>>>>> #include "exynos_drm_drv.h"
>>>>>>
>>>>>> /* returns true iff both arguments logically differs */
>>>>>> @@ -1041,10 +1042,20 @@ static ssize_t exynos_dsi_host_transfer(struct
>>>>>> mipi_dsi_host *host,
>>>>>> return (ret < 0) ? ret : xfer.rx_done;
>>>>>> }
>>>>>>
>>>>>> +static void exynos_dsi_host_pass_te(struct mipi_dsi_host *host)
>>>>>> +{
>>>>>> + struct exynos_dsi *dsi = host_to_dsi(host);
>>>>>> + struct drm_encoder *encoder = dsi->encoder;
>>>>>> +
>>>>>> + if (dsi->state & DSIM_STATE_ENABLED)
>>>>>> + exynos_drm_crtc_te_handler(encoder->crtc);
>>>>>> +}
>>>>>> +
>>>>>> static const struct mipi_dsi_host_ops exynos_dsi_ops = {
>>>>>> .attach = exynos_dsi_host_attach,
>>>>>> .detach = exynos_dsi_host_detach,
>>>>>> .transfer = exynos_dsi_host_transfer,
>>>>>> + .pass_te = exynos_dsi_host_pass_te,
>>>>>> };
>>>>>>
>>>>>> static int exynos_dsi_poweron(struct exynos_dsi *dsi)
>>>>>> diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
>>>>>> index 944f33f..3f21bea 100644
>>>>>> --- a/include/drm/drm_mipi_dsi.h
>>>>>> +++ b/include/drm/drm_mipi_dsi.h
>>>>>> @@ -49,6 +49,12 @@ struct mipi_dsi_msg {
>>>>>> * @detach: detach DSI device from DSI host
>>>>>> * @transfer: send and/or receive DSI packet, return number of
>>>>>> received bytes,
>>>>>> * or error
>>>>>> + * @pass_te: call the crtc te_handler() callback from DSI host.
>>>>>> + * The panel generates tearing effect synchronization signal
>>>>>> between
>>>>>> + * MCU and FB to display video images. And the display
>>>>>> controller
>>>>>> + * should trigger to transfer video image at this signal. So
>>>>>> the panel
>>>>>> + * receives the TE IRQ, then calls this function to notify it
>>>>>> to the
>>>>>> + * display controller.
>>>>>> */
>>>>>> struct mipi_dsi_host_ops {
>>>>>> int (*attach)(struct mipi_dsi_host *host,
>>>>>> @@ -57,6 +63,7 @@ struct mipi_dsi_host_ops {
>>>>>> struct mipi_dsi_device *dsi);
>>>>>> ssize_t (*transfer)(struct mipi_dsi_host *host,
>>>>>> struct mipi_dsi_msg *msg);
>>>>>> + void (*pass_te)(struct mipi_dsi_host *host);
>>>>>
>>>>> I've objected to this particular change before and that objection still
>>>>> stands. I don't see how this is related to DSI. It seems like an
>>>>> implementation detail of this particular setup and I think it should be
>>>>> handled differently (within the Exynos DSI controller implementation
>>>>> possibly).
>>>>>
>>>>
>>>> Okay, I understand what you mean.
>>>> As you know, this function is called by panel TE interrupt handler, so it
>>>> could be accessed by panel.
>>>> Do you have any good idea for panel to access exynos_drm_dsi directly
>>>> without mipi_dis_host_ops?
>>>
>>> I've gone through the DSI specification again and the only mention of
>>> the tearing effect is in section 8.12 "TE Signaling in DSI". That says
>>> the following:
>>>
>>> A Command Mode display module has its own timing controller and
>>> local frame buffer for display refresh. In some cases the host
>>> processor needs to be notified of timing events on the display
>>> module, e.g. the start of vertical blanking or similar timing
>>> information. In a traditional parallel-bus interface like DBI-2,
>>> a dedicated signal wire labeled TE (Tearing Effect) is provided
>>> to convey such timing information to the host processor. In a
>>> DSI system, the same information, with reasonably low latency,
>>> shall be transmitted from the display module to the host
>>> processor when requested, using the bidirectional Data Lane.
>>>
>>> My interpretation of that is that a DSI peripheral doesn't have a
>>> dedicated TE signal. Now the panel that you want to support here seems
>>> to have one, so I'm wondering if maybe it isn't a DSI panel at all but
>>> rather DBI.
>>
>> Uhm, this panel is DSI panel right. It provides TE external pad to transfer
>> TE pulse to host AP and it also provides TE relevant 3 DCS, so host AP could
>> choose either of them.
>> But I think it's better to use IRQ instead of polling method.
>
> According to the specification you don't have to rely on polling. You
> can simply pass control of the bus to the peripheral (via a BTA
> sequence) and then wait for the peripheral to signal TE.
>
I need to check that the Exynos DSIM driver supports this BTA sequence.
> That said, I've been doing some research and it seems like we have a
> somewhat similar feature on Tegra. What happens there is that there are
> three GPIO pins that can be repurposed for TE signalling. But as opposed
> to using them as interrupts the display controller can be configured to
> use them, upon which it will automatically handle the TE signal by
> sending the next frame.
Could you explain more detail how the Tegra display controller could be
configured with this GPIO pins?
I have no idea except that the display controller registers this GPIO as
an IRQ.
>
> So we have at least two very different implementations of this on two
> different SoCs. Further the specification explicitly recommends using
> the BTA sequence and DSI protocol to wait for TE. So I still think that
> controllers that provide an additional, non-spec compliant method to
> signal TE should handle it separately rather than within DSI. Otherwise
> we essentially need to make the DSI "core" aware of all these quirks,
> and I'd rather avoid that.
You mean, the DSI specification guides to use BTA, so it's better to use
display controller rather than DSIM, right?
>
>> As Inki commented before, I'll try to use remote-endpoint property of DSI
>> device node in exynos DSIM driver and call FIMD notifier.
>
> Sounds like it matches what I said above. I'm not a huge fan of
> notifiers, but if it works for you I suppose that's fine. The
> alternative would be to directly call a FIMD function, which is
> somewhat more explicit than a notifier.
Yes, I also like explicit call, so I want to use dsi_host_ops and calls
it in panel. But there is an objection to use dis_host_ops, we think
notifier in exynos dsim for fimd(display controller).
Thank you.
Best regards YJ
>
> Thierry
>
