On Wed, Jan 2, 2013 at 10:37 PM, Rahul Sharma <r.sh.open at gmail.com> wrote:
> On Wed, Jan 2, 2013 at 10:45 PM, Sean Paul <seanpaul at google.com> wrote:
>> On Thu, Dec 27, 2012 at 6:38 AM, Rahul Sharma <rahul.sharma at samsung.com>
>> wrote:
>>> This patch adds the implementation of check_mode callback in the mixer
>>> driver. Based on the mixer version, correct set of restrictions will be
>>> exposed by the mixer driver. A resolution will be acceptable only if passes
>>> the criteria set by mixer and hdmi IPs.
>>>
>>> Signed-off-by: Rahul Sharma <rahul.sharma at samsung.com>
>>> Signed-off-by: Sean Paul <seanpaul at chromium.org>
>>> ---
>>> drivers/gpu/drm/exynos/exynos_mixer.c | 30 ++++++++++++++++++++++++++++++
>>> 1 file changed, 30 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c
>>> b/drivers/gpu/drm/exynos/exynos_mixer.c
>>> index 21db895..093b374 100644
>>> --- a/drivers/gpu/drm/exynos/exynos_mixer.c
>>> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c
>>> @@ -810,6 +810,33 @@ static void mixer_win_disable(void *ctx, int win)
>>> mixer_ctx->win_data[win].enabled = false;
>>> }
>>>
>>> +int mixer_check_mode(void *ctx, void *mode)
>>> +{
>>> + struct mixer_context *mixer_ctx = ctx;
>>> + struct fb_videomode *check_mode = mode;
>>
>> Why not just pass fb_videomode in the first place?
>
> I kept the prototype same with check_timing in exynos_hdmi_ops or I
> should change in
> both the places.
Yeah, I think it should change in both places. The type doesn't need
to be opaque, IMO.
>>
>>> + unsigned int w, h;
>>> +
>>> + w = check_mode->xres;
>>> + h = check_mode->yres;
>>> +
>>> + DRM_DEBUG_KMS("%s : xres=%d, yres=%d, refresh=%d, intl=%d\n",
>>> + __func__, check_mode->xres, check_mode->yres,
>>> + check_mode->refresh, (check_mode->vmode &
>>> + FB_VMODE_INTERLACED) ? true : false);
>>> +
>>> + if (mixer_ctx->mxr_ver == MXR_VER_16_0_33_0) {
>>> + if ((w >= 464 && w <= 720 && h >= 261 && h <= 576) ||
>>> + (w >= 1024 && w <= 1280 &&
>>> + h >= 576 && h <= 720) ||
>>> + (w >= 1664 && w <= 1920 &&
>>> + h >= 936 && h <= 1080))
>>> + return 0;
>>
>> You could eliminate some of this spaghetti by doing:
>> if (mixer_ctx->mxr_ver != MXR_VER_16_0_33_0)
>> return 0;
>>
> I will do that.
>
> I have also added checks for min vertical lines (h >= 936, h >= 576,
> ..) which was
> not present in original patches. Due to this, 1920x540P was getting
> selected which
> is actually not supported by exy5 mixer.
>
> Above values are calculated by Min Horz lines *9 / 16.
> Please verify this part.
>
Yeah, I noticed that. I had assumed that the mixer could generate any
number of vertical lines from 0 to N where N is an upper bound defined
by the horizontal columns (from the HDMI Resolution Restriction
document). It's surprising to me that it can't generate that
resolution.
At any rate, you must have some information I don't, so it is what it is :)
Sean
> regards,
> Rahul Sharma.
>
>> Then remove one level of indent from the big if statement.
>>
>> Sean
>>
>>> + } else {
>>> + return 0;
>>> + }
>>> +
>>> + return -EINVAL;
>>> +}
>>> static void mixer_wait_for_vblank(void *ctx)
>>> {
>>> struct mixer_context *mixer_ctx = ctx;
>>> @@ -947,6 +974,9 @@ static struct exynos_mixer_ops mixer_ops = {
>>> .win_mode_set = mixer_win_mode_set,
>>> .win_commit = mixer_win_commit,
>>> .win_disable = mixer_win_disable,
>>> +
>>> + /* display */
>>> + .check_mode = mixer_check_mode,
>>> };
>>>
>>> /* for pageflip event */
>>> --
>>> 1.8.0
>>>
