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

[Andrzej Hajda <a.hajda@samsung.com>]

On 03/02/2015 11:57 AM, Ajay kumar wrote:
> On Mon, Mar 2, 2015 at 1:38 PM, Andrzej Hajda <a.hajda@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@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;
}

Regards
Andrzej

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