Re: [RFC v2 PATCH v3 10/14] drm/panel: add S6E3FA0 driver

[YoungJun Cho <yj44.cho@samsung.com>]

Hi Andrzej, Laurent, Thierry.

CCing Steffen Trumtrar,

On 04/29/2014 05:35 PM, YoungJun Cho wrote:
> On 04/29/2014 03:02 PM, YoungJun Cho wrote:
>> Hi Laurent,
>>
>> Thank you for sharing your idea.
>>
>> On 04/29/2014 12:05 AM, Laurent Pinchart wrote:
>>> Hi YoungJun,
>>>
>>> On Tuesday 22 April 2014 10:24:39 YoungJun Cho wrote:
>>>> On 04/22/2014 08:00 AM, Laurent Pinchart wrote:
>>>>> Hi YoungJun,
>>>>>
>>>>> Thank you for the patch.
>>>>>
>>>>> On Monday 21 April 2014 21:28:37 YoungJun Cho wrote:
>>>>>> This patch adds MIPI-DSI command mode based S6E3FA0 AMOLED LCD Panel
>>>>>> driver.
>>>>>>
>>>>>> Changelog v2:
>>>>>> - Declares delay, size properties in probe routine instead of DT
>>>>>> Changelog v3:
>>>>>> - Moves CPU timings relevant properties from FIMD DT
>>>>>>
>>>>>>     (commented by Laurent Pinchart, Andrzej Hajda)
>>>>>>
>>>>>> Signed-off-by: YoungJun Cho <yj44.cho@samsung.com>
>>>>>> Acked-by: Inki Dae <inki.dae@samsung.com>
>>>>>> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
>>>>>> ---
>>>>>>
>>>>>>    drivers/gpu/drm/panel/Kconfig         |    7 +
>>>>>>    drivers/gpu/drm/panel/Makefile        |    1 +
>>>>>>    drivers/gpu/drm/panel/panel-s6e3fa0.c |  569
>>>>>> ++++++++++++++++++++++++++
>>>>>>    3 files changed, 577 insertions(+)
>>>>>>    create mode 100644 drivers/gpu/drm/panel/panel-s6e3fa0.c
>>>>>
>>>>> [snip]
>>>>>
>>>>>> diff --git a/drivers/gpu/drm/panel/panel-s6e3fa0.c
>>>>>> b/drivers/gpu/drm/panel/panel-s6e3fa0.c new file mode 100644
>>>>>> index 0000000..1282678
>>>>>> --- /dev/null
>>>>>> +++ b/drivers/gpu/drm/panel/panel-s6e3fa0.c
>>>>>> @@ -0,0 +1,569 @@
>>>>>
>>>>> [snip]
>>>>>
>>>>>> +static int s6e3fa0_get_modes(struct drm_panel *panel)
>>>>>> +{
>>>>>> +    struct drm_connector *connector = panel->connector;
>>>>>> +    struct s6e3fa0 *ctx = panel_to_s6e3fa0(panel);
>>>>>> +    struct drm_display_mode *mode;
>>>>>> +
>>>>>> +    mode = drm_mode_create(connector->dev);
>>>>>> +    if (!mode) {
>>>>>> +        DRM_ERROR("failed to create a new display mode\n");
>>>>>> +        return 0;
>>>>>> +    }
>>>>>> +
>>>>>> +    drm_display_mode_from_videomode(&ctx->vm, mode);
>>>>>> +    mode->width_mm = ctx->width_mm;
>>>>>> +    mode->height_mm = ctx->height_mm;
>>>>>> +    connector->display_info.width_mm = mode->width_mm;
>>>>>> +    connector->display_info.height_mm = mode->height_mm;
>>>>>> +
>>>>>> +    mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
>>>>>> +    mode->private = (void *)&ctx->cpu_timings;
>>>>>
>>>>> Wouldn't it make sense to create a new get_interface_params (or
>>>>> similar)
>>>>> operation for drm_panel to query interface configuration parameters
>>>>> instead of shoving it in the mode private field ?
>>>>
>>>> You mean "new get_interface_params operation" is different from
>>>> get_modes() ?
>>>>
>>>> Till now, struct drm_display_mode and most of mode relevant APIs are
>>>> only for video interface.
>>>> And CPU interface also needs video mode configurations.
>>>>
>>>> I have a plan to implement the CPU interface relevant APIs like video
>>>> mode ones, but I think they should be used under current DRM mode APIs
>>>> like fill_modes, get_modes and so on.
>>>> So after that implementation, this private field will be replaced by
>>>> new ones.
>>>>
>>>> Could you explain it in more detail?
>>>
>>> The idea is that the interface parameters (RD/WR signals timings in
>>> this case,
>>> but this could also include MIPI DSI lane configuration or any other
>>> kind of
>>> physical interface parameters) are distinct from the video modes.
>>
>> Yes. The RD/WR signals timings are distinct from the video modes,
>> but in my opinion, others are covered by video mode already.
>>
>>>
>>> Do you see a need to tie tie interface parameters with
>>> drm_display_mode ? Can
>>> they be mode-specific ? In any case I'd like not to use the private
>>> field of
>>> drm_display_mode. If we need to tie both information together then it
>>> should
>>> be done in a standard way.
>>
>> I think this cpu-mode-timings is in struct drm_display_mode
>> (NOT by *private) and requires drm_display_mode_from_cpumode()
>> because the drm_display_mode_from_videomode() covers only video mode.
>>
>> I'll try implement it as soon as possible.
>>
>
> For your information, there are two modes in MIPI DSI specification,
> which are video mode and command mode.
> And CS, RD/WR timings are related with MIPI DBI specification,
> VSYNC, HSYNC timings are related with MIPI DPI specification.
>
> So I think all things relevant with command mode should be arranged
> the name of command mode(NOT CPU mode, like video mode NOT RGB mode)
> in MIPI DSI, and we don't need to consider MIPI DBI like we do MIPI DPI
> for video mode.
>

First, for MIPI command mode, hactive and vactive are only required.
Sorry Andrzej for confusing reply.
I have to modify exynos fimd pixel clock calculating function.

Second, for generic MIPI command mode timing support, commandmode.c and
of_commandmode.c are required in drivers/video.
And the struct drm_panel_command_mode_timings should be moved into 
commandmode.h as struct commandmode.

And the last(this is important and I need your ideas), adds this command 
mode timings into struct display_timing.
 From hierarchical aspects, display_timing(s) should contain command 
mode timings too, but it only contains video mode timings.
If I do it, current video mode relevant DTs will be broken.

So IMHO it is required to rename of_*_display_timing() as 
of_*_videomode_timing() and add of_*_commandmode_timing() into 
of_display_timing.c.
And should add command mode timings into DT like display-timings-command
to distinguish it from display-timings(for videomode).

What do you think about it?

Thank you.
Best regards YJ

> Thank you.
> Best regards YJ
>
>> Thank you,
>> Best regards YJ.
>>>
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>