Date: Tue, 25 Aug 2015 11:36:18 +0200
From: Daniel Vetter <daniel@ffwll.ch>
To: Xinliang Liu <xinliang.liu@linaro.org>
Cc: airlied@linux.ie, dri-devel@lists.freedesktop.org, liguozhu@hisilicon.com,
        linux-kernel@vger.kernel.org, benjamin.gaignard@linaro.org
Subject: Re: [PATCH] drm/crtc: Add a helper func to get a registered crtc
 from its index
Message-ID: <20150825093618.GA20434@phenom.ffwll.local>
Mail-Followup-To: Xinliang Liu <xinliang.liu@linaro.org>, airlied@linux.ie,
	dri-devel@lists.freedesktop.org, liguozhu@hisilicon.com,
	linux-kernel@vger.kernel.org, benjamin.gaignard@linaro.org
References: <1440472431-50874-1-git-send-email-xinliang.liu@linaro.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1440472431-50874-1-git-send-email-xinliang.liu@linaro.org>
User-Agent: Mutt/1.5.23 (2014-03-12)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4149
Lines: 122

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/