| Message ID | 1440472431-50874-1-git-send-email-xinliang.liu@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. - 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. 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
On Tue, Aug 25, 2015 at 11:36:18AM +0200, Daniel Vetter wrote: > 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: Of course you meant to say /unsigned/ 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 have a couple patches to address this partially, which came about as a result of the int crtc/crtc_id/pipe/whatever -> unsigned int pipe conversion work that I've been doing. > - 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. I like the idea of moving the callbacks to drm_crtc_helper_funcs. That allows us to introduce this step by step, without a flag date when every driver needs to switch the drm_driver functions over. Thierry
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 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. 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. -Daniel
On 26 August 2015 at 19:04, Thierry Reding <thierry.reding@gmail.com> wrote: > On Tue, Aug 25, 2015 at 11:36:18AM +0200, Daniel Vetter wrote: > > 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: > > Of course you meant to say /unsigned/ 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 have a couple patches to address this partially, which came about as a > result of the int crtc/crtc_id/pipe/whatever -> unsigned int pipe > conversion work that I've been doing. > Hi Thierry, I wonder what your patches look like. Could you share them to me. > > - 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. > > I like the idea of moving the callbacks to drm_crtc_helper_funcs. That > allows us to introduce this step by step, without a flag date when every > driver needs to switch the drm_driver functions over. > > Thierry >
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(+)