From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tomasz Figa Subject: Re: [PATCH v3 06/32] drm/exynos: Pass exynos_drm_manager in manager ops instead of dev Date: Fri, 01 Nov 2013 21:11:29 +0100 Message-ID: <1848645.t6SHkpZ8l6@flatron> References: <1383063198-10526-1-git-send-email-seanpaul@chromium.org> <15548958.EEHG2RulkY@flatron> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-ee0-f50.google.com (mail-ee0-f50.google.com [74.125.83.50]) by gabe.freedesktop.org (Postfix) with ESMTP id D3395F079C for ; Fri, 1 Nov 2013 13:11:27 -0700 (PDT) Received: by mail-ee0-f50.google.com with SMTP id l10so872eei.37 for ; Fri, 01 Nov 2013 13:11:26 -0700 (PDT) In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces@lists.freedesktop.org Errors-To: dri-devel-bounces@lists.freedesktop.org To: Sean Paul Cc: dri-devel , =?ISO-8859-1?Q?St=E9phane?= Marchesin List-Id: dri-devel@lists.freedesktop.org On Friday 01 of November 2013 16:01:23 Sean Paul wrote: > On Thu, Oct 31, 2013 at 8:19 PM, Tomasz Figa wrote: > > Hi Sean, > > > > On Tuesday 29 of October 2013 12:12:52 Sean Paul wrote: [snip] > >> -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. Right, I have overlooked this. > >> 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. Fine. > >> - 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. Fair enough. > > [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. I don't see the hunk removing it from another location. Are you sure? Best regards, Tomasz