Re: [PATCH] Revert "drm/exynos/decon5433: implement frame counter"

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Oct 05, 2018 at 04:03:25PM +0900, Inki Dae wrote:
> 
> 
> On 2018년 10월 05일 15:26, Daniel Vetter wrote:
> > On Fri, Oct 5, 2018 at 8:22 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> >>
> >> On Fri, Oct 5, 2018 at 4:59 AM Inki Dae <inki.dae@samsung.com> wrote:
> >>>
> >>> This reverts commit 0586feba322e1de05075700eb4b835c8b683e62b
> >>>
> >>> This patch makes it to need get_vblank_counter callback in crtc
> >>> to get frame counter from decon driver.
> >>>
> >>> However, drm_dev->max_vblank_count is a member unique to
> >>> vendor's DRM driver but in case of ARM DRM, some CRTC devices
> >>> don't provide the frame counter value. As a result, this patch
> >>> made extension and clone mode not working.
> >>>
> >>> Instead of this patch, we may need separated max_vblank_count
> >>> which belongs to each CRTC device, or need to implement frame
> >>> counter emulation for them who don't support HW frame counter.
> >>
> >> I think addin drm_crtc->max_vblank_count would make tons of sense, we've moved a bunch of the other vblank things in there too in the past. The logic in drm_vblank.c could be
> > 
> > Oops, hit send to early. I meant:
> > 
> >     max_vblank_count = crtc->max_vblank_count ? crtc->max_vblank_count
> > : dev->max_vblank_count
> > 
> > The problem with this is that most functions in drm_vblank.c still
> > only work with the (dev, pipe) pair, and not yet natively with (struct
> > drm_crtc *crtc). Might be quite some refactoring needed to push the
> 
> drm_update_vblank_count function provides pipe correspoding to a crtc
> which is invariant over the litetime of the crtc.  So I think we could
> resolve this problem easily by referring to
> crtc->max_vblank_count given when vendor driver created a crtc after
> getting the crtc object using drm_crtc_from_index function.

Well, only if your a kms driver. That's the entire problem. The vblank
code is also used by non-kms drivers, and we can't just break those.

I think a simpler solution is to add it to struct drm_vblank_crtc. That
one we will have available in both cases.
-Daniel

> 
> Thanks,
> Inki Dae
> 
> > crtc pointer down to all the places where we'll need it. We might also
> > need to split drm_vblank.c completely into new-style functions for kms
> > drivers (using struct drm_crtc *), and the legacy code for old
> > ums-only drivers (still using the dev, pipe pair throughout).
> > -Daniel
> > 
> >>
> >>
> >> -Daniel
> >>
> >>>
> >>> Signed-off-by: Inki Dae <inki.dae@samsung.com>
> >>> ---
> >>>  drivers/gpu/drm/exynos/exynos5433_drm_decon.c |  9 ---------
> >>>  drivers/gpu/drm/exynos/exynos_drm_crtc.c      | 11 -----------
> >>>  drivers/gpu/drm/exynos/exynos_drm_drv.h       |  1 -
> >>>  3 files changed, 21 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/exynos/exynos5433_drm_decon.c b/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
> >>> index 94529aa..aef487d 100644
> >>> --- a/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
> >>> +++ b/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
> >>> @@ -164,13 +164,6 @@ static u32 decon_get_frame_count(struct decon_context *ctx, bool end)
> >>>         return frm;
> >>>  }
> >>>
> >>> -static u32 decon_get_vblank_counter(struct exynos_drm_crtc *crtc)
> >>> -{
> >>> -       struct decon_context *ctx = crtc->ctx;
> >>> -
> >>> -       return decon_get_frame_count(ctx, false);
> >>> -}
> >>> -
> >>>  static void decon_setup_trigger(struct decon_context *ctx)
> >>>  {
> >>>         if (!ctx->crtc->i80_mode && !(ctx->out_type & I80_HW_TRG))
> >>> @@ -536,7 +529,6 @@ static const struct exynos_drm_crtc_ops decon_crtc_ops = {
> >>>         .disable                = decon_disable,
> >>>         .enable_vblank          = decon_enable_vblank,
> >>>         .disable_vblank         = decon_disable_vblank,
> >>> -       .get_vblank_counter     = decon_get_vblank_counter,
> >>>         .atomic_begin           = decon_atomic_begin,
> >>>         .update_plane           = decon_update_plane,
> >>>         .disable_plane          = decon_disable_plane,
> >>> @@ -554,7 +546,6 @@ static int decon_bind(struct device *dev, struct device *master, void *data)
> >>>         int ret;
> >>>
> >>>         ctx->drm_dev = drm_dev;
> >>> -       drm_dev->max_vblank_count = 0xffffffff;
> >>>
> >>>         for (win = ctx->first_win; win < WINDOWS_NR; win++) {
> >>>                 ctx->configs[win].pixel_formats = decon_formats;
> >>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> >>> index eea9025..2696289 100644
> >>> --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> >>> +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> >>> @@ -162,16 +162,6 @@ static void exynos_drm_crtc_disable_vblank(struct drm_crtc *crtc)
> >>>                 exynos_crtc->ops->disable_vblank(exynos_crtc);
> >>>  }
> >>>
> >>> -static u32 exynos_drm_crtc_get_vblank_counter(struct drm_crtc *crtc)
> >>> -{
> >>> -       struct exynos_drm_crtc *exynos_crtc = to_exynos_crtc(crtc);
> >>> -
> >>> -       if (exynos_crtc->ops->get_vblank_counter)
> >>> -               return exynos_crtc->ops->get_vblank_counter(exynos_crtc);
> >>> -
> >>> -       return 0;
> >>> -}
> >>> -
> >>>  static const struct drm_crtc_funcs exynos_crtc_funcs = {
> >>>         .set_config     = drm_atomic_helper_set_config,
> >>>         .page_flip      = drm_atomic_helper_page_flip,
> >>> @@ -181,7 +171,6 @@ static const struct drm_crtc_funcs exynos_crtc_funcs = {
> >>>         .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
> >>>         .enable_vblank = exynos_drm_crtc_enable_vblank,
> >>>         .disable_vblank = exynos_drm_crtc_disable_vblank,
> >>> -       .get_vblank_counter = exynos_drm_crtc_get_vblank_counter,
> >>>  };
> >>>
> >>>  struct exynos_drm_crtc *exynos_drm_crtc_create(struct drm_device *drm_dev,
> >>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h b/drivers/gpu/drm/exynos/exynos_drm_drv.h
> >>> index ec9604f..5e61e70 100644
> >>> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.h
> >>> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h
> >>> @@ -135,7 +135,6 @@ struct exynos_drm_crtc_ops {
> >>>         void (*disable)(struct exynos_drm_crtc *crtc);
> >>>         int (*enable_vblank)(struct exynos_drm_crtc *crtc);
> >>>         void (*disable_vblank)(struct exynos_drm_crtc *crtc);
> >>> -       u32 (*get_vblank_counter)(struct exynos_drm_crtc *crtc);
> >>>         enum drm_mode_status (*mode_valid)(struct exynos_drm_crtc *crtc,
> >>>                 const struct drm_display_mode *mode);
> >>>         bool (*mode_fixup)(struct exynos_drm_crtc *crtc,
> >>> --
> >>> 2.7.4
> >>>
> >>> _______________________________________________
> >>> dri-devel mailing list
> >>> dri-devel@lists.freedesktop.org
> >>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >>
> >>
> >>
> >> --
> >> Daniel Vetter
> >> Software Engineer, Intel Corporation
> >> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> > 
> > 
> > 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel