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

[Xinliang Liu]

On 10 September 2015 at 17:46, Daniel Vetter <daniel at ffwll.ch> wrote:

> On Thu, Sep 10, 2015 at 04:07:16PM +0800, Xinliang Liu wrote:
> > 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.
>
> Imo there's no need to change drm_irq_install at all, this is just a
> convenience wrapper for (mostly pci) devices which only have 1 interrupt.
> If this doesn't fit your hw (e.g. per-crtc interrupt sources) then just
> don't use it, it's purely optional and there's a bunch of drivers which
> don't use it.
>
> Thank you so much for your suggestion.  I think I should not use
drm_irq_install in my new driver.


> I have a long-term plan to split the vblank handling code out from the
> optional irq handling parts into a new drm_vblank.c file, so that this
> split between optional irq helpers and core vblank infrastructure is
> clearer.
>
Thank you so much for you work! I am really looking forward to see the
vblank improvement.

-Xinliang

> -Daniel
> --
> 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/20150911/782d9bb9/attachment-0001.html>