Re: [PATCH v3 06/32] drm/exynos: Pass exynos_drm_manager in manager ops instead of dev

From: Sean Paul <seanpaul@chromium.org>
To: Tomasz Figa <tomasz.figa@gmail.com>
Cc: dri-devel <dri-devel@lists.freedesktop.org>,
	"Stéphane Marchesin" <marcheu@chromium.org>
Subject: Re: [PATCH v3 06/32] drm/exynos: Pass exynos_drm_manager in manager ops instead of dev
Date: Fri, 1 Nov 2013 16:01:23 -0400	[thread overview]
Message-ID: <CAOw6vbJWT5giDGAUWMRGB+HW2f6QDVSOmyouPJvyRXMEsC9mNQ@mail.gmail.com> (raw)
In-Reply-To: <15548958.EEHG2RulkY@flatron>

On Thu, Oct 31, 2013 at 8:19 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> Hi Sean,
>
> On Tuesday 29 of October 2013 12:12:52 Sean Paul wrote:
>> This patch changes the manager ops callbacks from accepting the subdrv
>> device pointer to taking a pointer to the manager. This will allow us
>> to move closer to decoupling manager/display from subdrv, and
>> subsequently decoupling the crtc/plane from the encoder.
>
> The idea of changing callbacks argument itself is fine for me, but I
> wonder if by the way we couldn't refactor the code in a way that would
> allow type checking of context structures. This would make the code a bit
> less error-prone (or maybe I'm just a bit too paranoid...).
>
> Anyway, please see my remaining comments inline.
>
>> Signed-off-by: Sean Paul <seanpaul@chromium.org>
>> ---
>>
>> Changes in v2:
>>       - Instead of passing context, pass manager
>>       - Properly assign ctx->dev in fimd driver
>> Changes in v3:
>>       - Added vidi implementation
>>
>>  drivers/gpu/drm/exynos/exynos_drm_connector.c |   2 +-
>>  drivers/gpu/drm/exynos/exynos_drm_drv.h       |  35 ++++----
>>  drivers/gpu/drm/exynos/exynos_drm_encoder.c   |  27 +++---
>>  drivers/gpu/drm/exynos/exynos_drm_fimd.c      | 114
>> ++++++++++++++------------ drivers/gpu/drm/exynos/exynos_drm_hdmi.c
>>  |  72 ++++++++-------- drivers/gpu/drm/exynos/exynos_drm_vidi.c      |
>>  83 ++++++++++--------- 6 files changed, 180 insertions(+), 153
>> deletions(-)
> [snip]
>> @@ -182,16 +184,16 @@ static struct exynos_drm_display_ops
>> fimd_display_ops = { .power_on = fimd_display_power_on,
>>  };
>>
>> -static void fimd_win_mode_set(struct device *dev,
>> -                           struct exynos_drm_overlay *overlay)
>> +static void fimd_win_mode_set(struct exynos_drm_manager *mgr,
>> +                     struct exynos_drm_overlay *overlay)
>>  {
>> -     struct fimd_context *ctx = get_fimd_context(dev);
>> +     struct fimd_context *ctx = mgr->ctx;
>>       struct fimd_win_data *win_data;
>>       int win;
>>       unsigned long offset;
>>
>>       if (!overlay) {
>> -             dev_err(dev, "overlay is NULL\n");
>> +             DRM_ERROR("overlay is NULL\n");
>
> This change does not seem to be related to $subject.
>

It is. fimd_win_mode_set does not take dev as an argument any longer,
as such it's undefined.

>>               return;
>>       }
>>
>> @@ -231,9 +233,8 @@ static void fimd_win_mode_set(struct device *dev,
>>                       overlay->fb_width, overlay->crtc_width);
>>  }
>>
>> -static void fimd_win_set_pixfmt(struct device *dev, unsigned int win)
>> +static void fimd_win_set_pixfmt(struct fimd_context *ctx, unsigned int
>> win) {
>
> Again not really related to $subject. Maybe this should be done in a
> preparatory patch preceeding this one? (+ same comment for several
> identical changes below)
>

I think it's directly related to the subject. We no longer pass dev as
an argument, so that has indirect effects on other functions.

>> -     struct fimd_context *ctx = get_fimd_context(dev);
>>       struct fimd_win_data *win_data = &ctx->win_data[win];
>>       unsigned long val;
>>
> [snip]
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_hdmi.c
>> b/drivers/gpu/drm/exynos/exynos_drm_hdmi.c index aebcc0e..ca0a87f
>> 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_hdmi.c
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_hdmi.c
>> @@ -129,11 +129,9 @@ static struct edid *drm_hdmi_get_edid(struct device
>> *dev,
>>
>>       return NULL;
>>  }
>> -
>> -static int drm_hdmi_check_mode(struct device *dev,
>> +static int drm_hdmi_check_mode_ctx(struct drm_hdmi_context *ctx,
>>               struct drm_display_mode *mode)
>>  {
>> -     struct drm_hdmi_context *ctx = to_context(dev);
>>       int ret = 0;
>>
>>       /*
>> @@ -153,6 +151,14 @@ static int drm_hdmi_check_mode(struct device *dev,
>>       return 0;
>>  }
>>
>> +static int drm_hdmi_check_mode(struct device *dev,
>> +             struct drm_display_mode *mode)
>> +{
>> +     struct drm_hdmi_context *ctx = to_context(dev);
>> +
>> +     return drm_hdmi_check_mode_ctx(ctx, mode);
>> +}
>> +
>
> nit: I don't think such wrapper is necessary.
>
> It seems to be easy enough to get from dev to ctx, so depending on the
> amount of user of drm_hdmi_check_mode() it might be better to simply
> change them to pass ctx instead of dev.
>

This is a display_op that is defined to accept dev. It's changed later
in the patchset to accept display, at which point the wrapper goes
away.

> [snip]
>> @@ -403,19 +404,23 @@ static void vidi_subdrv_remove(struct drm_device
>> *drm_dev, struct device *dev) /* TODO. */
>>  }
>>
>> -static int vidi_power_on(struct vidi_context *ctx, bool enable)
>> +static int vidi_power_on(struct exynos_drm_manager *mgr, bool enable)
>>  {
>> -     struct exynos_drm_subdrv *subdrv = &ctx->subdrv;
>> -     struct device *dev = subdrv->dev;
>> +     struct vidi_context *ctx = mgr->ctx;
>> +
>> +     DRM_DEBUG_KMS("%s\n", __FILE__);
>> +
>> +     if (enable != false && enable != true)
>> +             return -EINVAL;
>
> Huh? What value would you expect a bool to have if not false or true?
>
> Anyway, this shouldn't really matter, as the check bellow assumes that
> anything non-zero is true.
>

This is pre-existing vidi code, I just moved it.

Sean

>>
>>       if (enable) {
>>               ctx->suspended = false;
>>
>
> Just for clarity, this is the check I mentioned above.
>
> Best regards,
> Tomasz
>