<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On 25 August 2015 at 17:36, Daniel Vetter <span dir="ltr"><<a href="mailto:daniel@ffwll.ch" target="_blank">daniel@ffwll.ch</a>></span> wrote:</div><div class="gmail_quote">Hi Daniel,</div><div class="gmail_quote">Thank you very much for reply. Sorry, I just come back from vacation.</div><div class="gmail_quote">Very happy that you have a good idea to solve the mess. </div><div class="gmail_quote">Looking forward to see your patch soon!</div><div class="gmail_quote"><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"><span class="">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>
</span>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></blockquote><div>I agree too. It seems that vblank is a part of crtc.</div><div><br></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">
<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></blockquote><div>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</div><div>so that in the irq_handler we can find crtc easily.</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">
<br>
Cheers, Daniel<br>
<div><div class="h5"><br>
> ---<br>
> drivers/gpu/drm/drm_crtc.c | 25 +++++++++++++++++++++++++<br>
> include/drm/drm_crtc.h | 2 ++<br>
> 2 files changed, 27 insertions(+)<br>
><br>
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c<br>
> index b9ba061..8764765 100644<br>
> --- a/drivers/gpu/drm/drm_crtc.c<br>
> +++ b/drivers/gpu/drm/drm_crtc.c<br>
> @@ -747,6 +747,31 @@ unsigned int drm_crtc_index(struct drm_crtc *crtc)<br>
> }<br>
> EXPORT_SYMBOL(drm_crtc_index);<br>
><br>
> +/**<br>
> + * drm_get_crtc_from_index - find a registered CRTC from the index<br>
> + * @dev: DRM device<br>
> + * @index: index of a registered CRTC<br>
> + *<br>
> + * Given a index, return the registered CRTC within a DRM<br>
> + * device's list of CRTCs.<br>
> + */<br>
> +struct drm_crtc *drm_get_crtc_from_index(struct drm_device *dev,<br>
> + unsigned int index)<br>
> +{<br>
> + unsigned int index_tmp = 0;<br>
> + struct drm_crtc *crtc;<br>
> +<br>
> + list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {<br>
> + if (index_tmp == index)<br>
> + return crtc;<br>
> +<br>
> + index_tmp++;<br>
> + }<br>
> +<br>
> + BUG();<br>
> +}<br>
> +EXPORT_SYMBOL(drm_get_crtc_from_index);<br>
> +<br>
> /*<br>
> * drm_mode_remove - remove and free a mode<br>
> * @connector: connector list to modify<br>
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h<br>
> index 57ca8cc..3a46d39d 100644<br>
> --- a/include/drm/drm_crtc.h<br>
> +++ b/include/drm/drm_crtc.h<br>
> @@ -1225,6 +1225,8 @@ extern int drm_crtc_init_with_planes(struct drm_device *dev,<br>
> const struct drm_crtc_funcs *funcs);<br>
> extern void drm_crtc_cleanup(struct drm_crtc *crtc);<br>
> extern unsigned int drm_crtc_index(struct drm_crtc *crtc);<br>
> +extern struct drm_crtc *drm_get_crtc_from_index(struct drm_device *dev,<br>
> + unsigned int index);<br>
><br>
> /**<br>
> * drm_crtc_mask - find the mask of a registered CRTC<br>
> --<br>
> 1.9.1<br>
><br>
</div></div>> _______________________________________________<br>
> dri-devel mailing list<br>
> <a href="mailto:dri-devel@lists.freedesktop.org">dri-devel@lists.freedesktop.org</a><br>
> <a href="http://lists.freedesktop.org/mailman/listinfo/dri-devel" rel="noreferrer" target="_blank">http://lists.freedesktop.org/mailman/listinfo/dri-devel</a><br>
<span class=""><font color="#888888"><br>
--<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>
</font></span></blockquote></div><br></div></div>