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

[Inki Dae <inki.dae@samsung.com>]

Hi,

2015-03-02 19:57 GMT+09:00 Ajay kumar <ajaynumb@gmail.com>:
> 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(-)
>>>

-- snip --

>>>  static void decon_commit(struct exynos_drm_crtc *crtc)
>>>  {
>>>       struct decon_context *ctx = crtc->ctx;
>>> +     struct decon_driver_data *drv_data = ctx->driver_data;
>>>       struct drm_display_mode *mode = &crtc->base.mode;
>>> +     struct decon_ext_videomode *my_mode;
>>>       u32 val, clkdiv;
>>>
>>>       if (ctx->suspended)
>>> @@ -200,6 +306,38 @@ static void decon_commit(struct exynos_drm_crtc *crtc)
>>>       if (mode->htotal == 0 || mode->vtotal == 0)
>>>               return;
>>>
>>> +     if (drv_data->decon_type == DECON_EXT) {
>>> +             if ((mode->hdisplay == 720) && (mode->vdisplay == 480) &&
>>> +                                             (mode->vrefresh == 60))
>>> +                     my_mode = &decon_ext_porchs[MODE_720_480_60];
>>> +             if ((mode->hdisplay == 720) && (mode->vdisplay == 576) &&
>>> +                                             (mode->vrefresh == 50))
>>> +                     my_mode = &decon_ext_porchs[MODE_720_576_50];
>>> +             if ((mode->hdisplay == 1280) && (mode->vdisplay == 720) &&
>>> +                                             (mode->vrefresh == 60))
>>> +                     my_mode = &decon_ext_porchs[MODE_1280_720_60];
>>> +             if ((mode->hdisplay == 1280) && (mode->vdisplay == 720) &&
>>> +                                             (mode->vrefresh == 50))
>>> +                     my_mode = &decon_ext_porchs[MODE_1280_720_50];
>>> +             if ((mode->hdisplay == 1920) && (mode->vdisplay == 1080) &&
>>> +                                             (mode->vrefresh == 60))
>>> +                     my_mode = &decon_ext_porchs[MODE_1920_1080_60];
>>> +             if ((mode->hdisplay == 1920) && (mode->vdisplay == 1080) &&
>>> +                                             (mode->vrefresh == 50))
>>> +                     my_mode = &decon_ext_porchs[MODE_1920_1080_50];
>>> +             if ((mode->hdisplay == 1920) && (mode->vdisplay == 1080) &&
>>> +                                             (mode->vrefresh == 30))
>>> +                     my_mode = &decon_ext_porchs[MODE_1920_1080_30];
>>> +             if ((mode->hdisplay == 1920) && (mode->vdisplay == 1080) &&
>>> +                                             (mode->vrefresh == 25))
>>> +                     my_mode = &decon_ext_porchs[MODE_1920_1080_25];
>>> +             if ((mode->hdisplay == 1920) && (mode->vdisplay == 1080) &&
>>> +                                             (mode->vrefresh == 24))
>>> +                     my_mode = &decon_ext_porchs[MODE_1920_1080_24];
>>> +
>>> +             decon_ext_set_timing(mode, my_mode);
>>> +     }
>>> +
>>
>> I guess these ugly ifs could be replaced by simple loop:
>>         for (i = 0; i < ARRAY_SIZE(decon_ext_porchs); ++i) {
>>                 if (mode->hdisplay != decon_ext_porchs[i].hdisplay || ...)
>>                         continue;
>>                 decon_ext_set_timing(mode, &decon_ext_porchs[i]);
>>                 break;
>>         }
> Ok, Though the number of checks is the same, your code at least looks better. :)
>
>> Anyway I have doubts if commit should modify crtc.base.mode I guess
>> better place for it can be in *_fixup callback.
> Inki? What is your suggestion?

Agree with Andrzej. The fixup callback would be a right place to find
a valid mode. By the way, what if mode->hdisplay/vdisplay/vrefresh are
not supported for all DECON_EXT modes?

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