<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On 10 September 2015 at 17:46, Daniel Vetter <span dir="ltr"><<a href="mailto:daniel@ffwll.ch" target="_blank">daniel@ffwll.ch</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class=""><div class="h5">On Thu, Sep 10, 2015 at 04:07:16PM +0800, Xinliang Liu wrote:<br>
> On 25 August 2015 at 17:36, Daniel Vetter <<a href="mailto:daniel@ffwll.ch">daniel@ffwll.ch</a>> wrote:<br>
> Hi Daniel,<br>
> Thank you very much for reply. Sorry, I just come back from vacation.<br>
> Very happy that you have a good idea to solve the mess.<br>
> Looking forward to see your patch soon!<br>
><br>
> On Tue, Aug 25, 2015 at 11:13:51AM +0800, Xinliang Liu wrote:<br>
> > > This patch add a helper func to get a registered crtc from its index.<br>
> > > In some case, where we know the crtc's index and we want to know the<br>
> > > crtc too.<br>
> > ><br>
> > > For example, the enable_vblank func of struct drm_driver:<br>
> > > In the implementation of this func, we know the index of the crtc but<br>
> > > we want to know the crtc. This helper func can get the crtc easily.<br>
> > > A sample impelmentation of enable_vblank is as shown as bellow:<br>
> > ><br>
> > > int hisi_drm_crtc_enable_vblank(struct drm_device *dev, int c)<br>
> > > {<br>
> > >       struct drm_crtc *crtc = drm_get_crtc_from_index(dev, c);<br>
> > >       struct hisi_crtc *hcrtc = to_hisi_crtc(crtc);<br>
> > >       struct hisi_crtc_ops *ops = hcrtc->ops;<br>
> > >       int ret = 0;<br>
> > ><br>
> > >       if (ops->enable_vblank)<br>
> > >               ret = ops->enable_vblank(hcrtc);<br>
> > ><br>
> > >       return ret;<br>
> > > }<br>
> > ><br>
> > > Signed-off-by: Xinliang Liu <<a href="mailto:xinliang.liu@linaro.org">xinliang.liu@linaro.org</a>><br>
> ><br>
> > Yeah unfortunately drm_irq.c is still stick in the old pre-KMS days. I<br>
> > think we should go a bit further here though to allow new drivers to be<br>
> > completely free of int pipe:<br>
> ><br>
> > - add a new array pointer dev->mode_conifg.crtc_arr, which is<br>
> >   (re-)allocated in drm_crtc_init_with_planes. Then a pipe->crtc lookup<br>
> >   will be just<br>
> ><br>
> >         crtc = dev->mode_config.crtc_arr[pipe];<br>
> ><br>
> > - add new hooks for vblank handling int drm_crtc_helper_funcs for<br>
> >   enable_vblanke, disable_vblank, get_vblank_timestamp and get_scanoutpos.<br>
> >   Ofc also anotate the docs for the existing hooks and make it clear new<br>
> >   drivers should use the new ones. Ofc these new hooks should directly<br>
> >   take a struct drm_crtc * instead of inte pipe.<br>
> ><br>
> I agree too.  It seems that vblank is a part of crtc.<br>
><br>
><br>
> > - change the code in drm_irq.c to wrap all callbacks and first check<br>
> >   whether the new ones are there and only if that's not the case call the<br>
> >   old ones.<br>
> ><br>
> > With these changes drivers can be completely free of int pipe and use<br>
> > struct drm_crtc exclusivly I think, and the mess would be fully restricted<br>
> > to drm_irq.c.<br>
> ><br>
> For the drm_irq_install(struct drm_device *dev, int irq) function,  I<br>
> suggest to add one param such as "void *data" to pass crtc<br>
> so that in the irq_handler we can find crtc easily.<br>
<br>
</div></div>Imo there's no need to change drm_irq_install at all, this is just a<br>
convenience wrapper for (mostly pci) devices which only have 1 interrupt.<br>
If this doesn't fit your hw (e.g. per-crtc interrupt sources) then just<br>
don't use it, it's purely optional and there's a bunch of drivers which<br>
don't use it.<br>
<br></blockquote><div>Thank you so much for your suggestion.  I think I should not use drm_irq_install in my new driver.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
I have a long-term plan to split the vblank handling code out from the<br>
optional irq handling parts into a new drm_vblank.c file, so that this<br>
split between optional irq helpers and core vblank infrastructure is<br>
clearer.<br></blockquote><div>Thank you so much for you work! I am really looking forward to see the vblank improvement. </div><div><br></div><div>-Xinliang</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<span class=""><font color="#888888">-Daniel<br>
</font></span><div class=""><div class="h5">--<br>
Daniel Vetter<br>
Software Engineer, Intel Corporation<br>
<a href="http://blog.ffwll.ch" rel="noreferrer" target="_blank">http://blog.ffwll.ch</a><br>
</div></div></blockquote></div><br></div></div>