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

[Tomasz Figa <tomasz.figa@gmail.com>]

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.

>  		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)

> -	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.

[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.

> 
>  	if (enable) {
>  		ctx->suspended = false;
> 

Just for clarity, this is the check I mentioned above.

Best regards,
Tomasz