| Message ID | 1440472431-50874-1-git-send-email-xinliang.liu@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 25 August 2015 at 17:36, Daniel Vetter <daniel@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@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@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/dri-devel > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch >
On 10 September 2015 at 17:46, Daniel Vetter <daniel@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@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@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 >
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
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@linaro.org> --- drivers/gpu/drm/drm_crtc.c | 25 +++++++++++++++++++++++++ include/drm/drm_crtc.h | 2 ++ 2 files changed, 27 insertions(+)