[PATCH] drm/crtc: Add a helper func to get a registered crtc from its index

[Xinliang Liu]

On 25 August 2015 at 17:36, Daniel Vetter <daniel at ffwll.ch> wrote:
Hi Daniel,
Thank you very much for reply. Sorry, I just come back from vacation.
Very happy that you have a good idea to solve the mess.
Looking forward to see your patch soon!

On Tue, Aug 25, 2015 at 11:13:51AM +0800, Xinliang Liu wrote:
> > This patch add a helper func to get a registered crtc from its index.
> > In some case, where we know the crtc's index and we want to know the
> > crtc too.
> >
> > For example, the enable_vblank func of struct drm_driver:
> > In the implementation of this func, we know the index of the crtc but
> > we want to know the crtc. This helper func can get the crtc easily.
> > A sample impelmentation of enable_vblank is as shown as bellow:
> >
> > int hisi_drm_crtc_enable_vblank(struct drm_device *dev, int c)
> > {
> >       struct drm_crtc *crtc = drm_get_crtc_from_index(dev, c);
> >       struct hisi_crtc *hcrtc = to_hisi_crtc(crtc);
> >       struct hisi_crtc_ops *ops = hcrtc->ops;
> >       int ret = 0;
> >
> >       if (ops->enable_vblank)
> >               ret = ops->enable_vblank(hcrtc);
> >
> >       return ret;
> > }
> >
> > Signed-off-by: Xinliang Liu <xinliang.liu at linaro.org>
>
> Yeah unfortunately drm_irq.c is still stick in the old pre-KMS days. I
> think we should go a bit further here though to allow new drivers to be
> completely free of int pipe:
>
> - add a new array pointer dev->mode_conifg.crtc_arr, which is
>   (re-)allocated in drm_crtc_init_with_planes. Then a pipe->crtc lookup
>   will be just
>
>         crtc = dev->mode_config.crtc_arr[pipe];
>
> - add new hooks for vblank handling int drm_crtc_helper_funcs for
>   enable_vblanke, disable_vblank, get_vblank_timestamp and get_scanoutpos.
>   Ofc also anotate the docs for the existing hooks and make it clear new
>   drivers should use the new ones. Ofc these new hooks should directly
>   take a struct drm_crtc * instead of inte pipe.
>
I agree too.  It seems that vblank is a part of crtc.


> - change the code in drm_irq.c to wrap all callbacks and first check
>   whether the new ones are there and only if that's not the case call the
>   old ones.
>
> With these changes drivers can be completely free of int pipe and use
> struct drm_crtc exclusivly I think, and the mess would be fully restricted
> to drm_irq.c.
>
For the drm_irq_install(struct drm_device *dev, int irq) function,  I
suggest to add one param such as "void *data" to pass crtc
so that in the irq_handler we can find crtc easily.

>
> Cheers, Daniel
>
> > ---
> >  drivers/gpu/drm/drm_crtc.c | 25 +++++++++++++++++++++++++
> >  include/drm/drm_crtc.h     |  2 ++
> >  2 files changed, 27 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> > index b9ba061..8764765 100644
> > --- a/drivers/gpu/drm/drm_crtc.c
> > +++ b/drivers/gpu/drm/drm_crtc.c
> > @@ -747,6 +747,31 @@ unsigned int drm_crtc_index(struct drm_crtc *crtc)
> >  }
> >  EXPORT_SYMBOL(drm_crtc_index);
> >
> > +/**
> > + * drm_get_crtc_from_index - find a registered CRTC from the index
> > + * @dev: DRM device
> > + * @index: index of a registered CRTC
> > + *
> > + * Given a index, return the registered CRTC within a DRM
> > + * device's list of CRTCs.
> > + */
> > +struct drm_crtc *drm_get_crtc_from_index(struct drm_device *dev,
> > +                                      unsigned int index)
> > +{
> > +     unsigned int index_tmp = 0;
> > +     struct drm_crtc *crtc;
> > +
> > +     list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
> > +             if (index_tmp == index)
> > +                     return crtc;
> > +
> > +             index_tmp++;
> > +     }
> > +
> > +     BUG();
> > +}
> > +EXPORT_SYMBOL(drm_get_crtc_from_index);
> > +
> >  /*
> >   * drm_mode_remove - remove and free a mode
> >   * @connector: connector list to modify
> > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> > index 57ca8cc..3a46d39d 100644
> > --- a/include/drm/drm_crtc.h
> > +++ b/include/drm/drm_crtc.h
> > @@ -1225,6 +1225,8 @@ extern int drm_crtc_init_with_planes(struct
> drm_device *dev,
> >                                    const struct drm_crtc_funcs *funcs);
> >  extern void drm_crtc_cleanup(struct drm_crtc *crtc);
> >  extern unsigned int drm_crtc_index(struct drm_crtc *crtc);
> > +extern struct drm_crtc *drm_get_crtc_from_index(struct drm_device *dev,
> > +                                             unsigned int index);
> >
> >  /**
> >   * drm_crtc_mask - find the mask of a registered CRTC
> > --
> > 1.9.1
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20150910/3f0c0596/attachment-0001.html>