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

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

On Friday 01 of November 2013 16:01:23 Sean Paul wrote:
> 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:
[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