diff mbox

[2/4] drm/exynos: implement display-mode-check callback in mixer driver

Message ID 1356608328-5847-3-git-send-email-rahul.sharma@samsung.com (mailing list archive)
State New, archived
Headers show

Commit Message

Rahul Sharma Dec. 27, 2012, 11:38 a.m. UTC 
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@samsung.com>
Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/exynos/exynos_mixer.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

Comments

Sean Paul Jan. 2, 2013, 5:15 p.m. UTC | #1 
On Thu, Dec 27, 2012 at 6:38 AM, Rahul Sharma <rahul.sharma@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@samsung.com>
> Signed-off-by: Sean Paul <seanpaul@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?

> +       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;

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
>
Rahul Sharma Jan. 3, 2013, 3:37 a.m. UTC | #2 
On Wed, Jan 2, 2013 at 10:45 PM, Sean Paul <seanpaul@google.com> wrote:
> On Thu, Dec 27, 2012 at 6:38 AM, Rahul Sharma <rahul.sharma@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@samsung.com>
>> Signed-off-by: Sean Paul <seanpaul@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.
>
>> +       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.

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
>>
Sean Paul Jan. 3, 2013, 3:14 p.m. UTC | #3 
On Wed, Jan 2, 2013 at 10:37 PM, Rahul Sharma <r.sh.open@gmail.com> wrote:
> On Wed, Jan 2, 2013 at 10:45 PM, Sean Paul <seanpaul@google.com> wrote:
>> On Thu, Dec 27, 2012 at 6:38 AM, Rahul Sharma <rahul.sharma@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@samsung.com>
>>> Signed-off-by: Sean Paul <seanpaul@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
>>>
diff mbox

Patch

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;
+	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;
+	} 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 */