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

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

[Xinliang Liu]

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(+)

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

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

[Daniel Vetter]

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2545 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch