[PATCH 2/2] drm/exynos: decon: Add support for DECON-EXT

[Ajay kumar]

On Wed, Mar 4, 2015 at 7:44 PM, Andrzej Hajda <a.hajda at samsung.com> wrote:
> On 03/02/2015 11:57 AM, Ajay kumar wrote:
>> On Mon, Mar 2, 2015 at 1:38 PM, Andrzej Hajda <a.hajda at samsung.com> wrote:
>>> On 02/26/2015 04:24 PM, Ajay Kumar wrote:
>>>> * Modify DECON-INT driver to support DECON-EXT.
>>>> * Add a table of porch values needed to set timing registers of DECON-EXT.
>>>> * DECON-EXT supports only H/w Triggered COMMAND mode.
>>>> * DECON-EXT supports only one DMA window(window 1), so modify
>>>>   all window management routines to support 2 windows of DECON-INT
>>>>   and 1 window of DECON-EXT.
>>>>
>>>> Signed-off-by: Ajay Kumar <ajaykumar.rs at samsung.com>
>>>> ---
>>>>  .../devicetree/bindings/video/exynos7-decon.txt    |    8 +-
>>>>  drivers/gpu/drm/exynos/exynos7_drm_decon.c         |  229 ++++++++++++++++++--
>>>>  include/video/exynos7_decon.h                      |   13 ++
>>>>  3 files changed, 230 insertions(+), 20 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/video/exynos7-decon.txt b/Documentation/devicetree/bindings/video/exynos7-decon.txt
>>>> index f5f9c8d..87350c0 100644
>>>> --- a/Documentation/devicetree/bindings/video/exynos7-decon.txt
>>>> +++ b/Documentation/devicetree/bindings/video/exynos7-decon.txt
>>>> @@ -2,10 +2,14 @@ Device-Tree bindings for Samsung Exynos7 SoC display controller (DECON)
>>>>
>>>>  DECON (Display and Enhancement Controller) is the Display Controller for the
>>>>  Exynos7 series of SoCs which transfers the image data from a video memory
>>>> -buffer to an external LCD interface.
>>>> +buffer to an external LCD/HDMI interface.
>>>> +
>>>> +Exynos7 supports DECON-INT for generating video signals for LCD.
>>>> +Exynos7 supports DECON-EXT for generating video signals for HDMI.
>>>>
>>>>  Required properties:
>>>> -- compatible: value should be "samsung,exynos7-decon";
>>>> +- compatible: value should be "samsung,exynos7-decon-int" for DECON-INT;
>>>> +           value should be "samsung,exynos7-decon-ext" for DECON-EXT;
>>>>
>>>>  - reg: physical base address and length of the DECON registers set.
>>>>
>>>> diff --git a/drivers/gpu/drm/exynos/exynos7_drm_decon.c b/drivers/gpu/drm/exynos/exynos7_drm_decon.c
>>>> index 63f02e2..9e2d083 100644
>>>> --- a/drivers/gpu/drm/exynos/exynos7_drm_decon.c
>>>> +++ b/drivers/gpu/drm/exynos/exynos7_drm_decon.c
>>>> @@ -41,6 +41,28 @@
>>>>
>>>>  #define WINDOWS_NR   2
>>> Maybe it would be better to rename it to MAX_WINDOWS_NR
>> Ok.
>>>> +#define DECON_EXT_CH_MAPPING 0x432105
>>> I am not familiar with decon, could you explain what exactly
>>> this mapping means and why is it fixed in the driver to this value,
>>> not default one. By the way my specs says bits 0-3 are reserverd
>>> and here it seems you are using them.
>> I reused the value from the code which hardware team shared with me.
>> It tries to map a input hardware channel(IDMA or VPP channel) onto
>> a hardware window in DECON.
>> Channel_N is mapped onto window_N in case of DECON-INT.
>> In case of DECON-EXT, Channel 0 should be mapped to window 1 of DECON-EXT.
>> So, basically for the changes I have made, I should ideally set only
>> bits [4:6] to 0x1,
>> and leave other bits untouched, though modifying other bits wouldn't
>> affect in anyway.
>>>> +
>>>> +enum decon_type {
>>>> +     DECON_INT,
>>>> +     DECON_EXT,
>>>> +} decon_type;
>>>> +
>>>> +struct decon_driver_data {
>>>> +     enum decon_type                 decon_type;
>>>> +     unsigned int                    nr_windows;
>>>> +};
>>>> +
>>>> +static struct decon_driver_data exynos7_decon_int_driver_data = {
>>>> +     .decon_type = DECON_INT,
>>> I wonder if it wouldn't be better to use different fields to describe
>>> each capability/property instead of decon_type. For example
>>>         .display_type = EXYNOS_DISPLAY_TYPE_LCD,
>>>         .map_channels = 0,
>> Ok, let me list down the differences between INT and EXT.
>> 1) Only h/w triggered command mode for DECON-EXT.
>> 2) Need to feed modified porch values(decon_ext_timings)
>> 3) Input channel to H/w window mapping(WINCHMAP)
>> 4) default_window - 0 for DECON-INT and 1 for DECON-EXT
>>
>> Out of the above differences, the first 3 can somehow be converted
>> to capability/property and embedded into driver_data.
>> But the 4th difference is bothering me.
>> I tried using something like start_window, end_window and tried to make
>> The code common for DECON-INT and DECON-EXT, and it just doesn't work.
>> ex:
>> start_window = 0, end_window = 1 for DECON-INT
>> start_window = 1, end_window = 1 for DECON-EXT
>>
>> When win_commit gets called for window 0 from crtc_commit/plane_commit:
>> Configure start_window(0  for DECON-INT and 1 for DECON-EXT)
>>
>> When win_commit is called from decon_apply, it is called for window 1 also.
>> That time win_commit can skip updating actual window 1.
>> How do I take care of this ambiguity? This case happens with
>> win_disable routine also!
>
> I think clear distinction where are we using hw windows and where sw
> windows should be enough.
> For example the loop iterating over all windows can look like:
>
>         for (win = 0; win < drv_data->nr_windows; win++) {
>                 int hw_win = get_hw_win(ctx, win);
>
>                 val = readl(ctx->regs + WINCON(hw_win));
>         }
>
> Where get_hw_win can look like:
>
> static inline int get_hw_win(ctx, win)
> {
>     return ctx->driver_data->skip_windows + win;
> }
OK, you just want it in a static wrapper.
skip_windows = 0 for DECON-INT
skip_windows = 1 for DECON-EXT

Regards,
Ajay