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