Re: [PATCH v3 14/32] drm/exynos: Remove exynos_drm_hdmi shim

[Tomasz Figa <tomasz.figa@gmail.com>]

Hi Sean,

On Tuesday 29 of October 2013 12:13:00 Sean Paul wrote:
> This patch trims exynos_drm_hdmi out of the driver. The reason it
> existed in the first place was to make up for the mixture of
> display/overlay/manager ops being spread across hdmi and mixer. With
> that code now rationalized, mixer and hdmi map directly to
> exynos_drm_crtc and exynos_drm_encoder, respectively. Since there is a
> 1:1 mapping, we no longer need this layer.
> 
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> ---
> 
> Changes in v2:
> 	- hdmi/mixer ops now take display/manager instead of context
> Changes in v3:
> 	- Moved removal of exynos_drm_hdmi.c file here
> 
>  drivers/gpu/drm/exynos/Makefile          |   3 +-
>  drivers/gpu/drm/exynos/exynos_drm_drv.c  |  13 -
>  drivers/gpu/drm/exynos/exynos_drm_hdmi.c | 416 -------------------------------
>  drivers/gpu/drm/exynos/exynos_drm_hdmi.h |  69 -----
>  drivers/gpu/drm/exynos/exynos_hdmi.c     | 162 +++++++-----
>  drivers/gpu/drm/exynos/exynos_mixer.c    | 204 ++++++++-------
>  drivers/gpu/drm/exynos/exynos_mixer.h    |  20 ++
>  7 files changed, 222 insertions(+), 665 deletions(-)
>  delete mode 100644 drivers/gpu/drm/exynos/exynos_drm_hdmi.c
>  delete mode 100644 drivers/gpu/drm/exynos/exynos_drm_hdmi.h
>  create mode 100644 drivers/gpu/drm/exynos/exynos_mixer.h
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c
> index e106309..9e12fb7 100644
> --- a/drivers/gpu/drm/exynos/exynos_hdmi.c
> +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
[snip]
> +static struct exynos_drm_display hdmi_display = {
> +	.type = EXYNOS_DISPLAY_TYPE_HDMI,
> +	.ops = &hdmi_display_ops,
> +};
> +
>  static irqreturn_t hdmi_irq_thread(int irq, void *arg)
>  {
> -	struct exynos_drm_hdmi_context *ctx = arg;
> -	struct hdmi_context *hdata = ctx->ctx;
> +	struct hdmi_context *hdata = arg;
>  
>  	mutex_lock(&hdata->hdmi_mutex);
>  	hdata->hpd = gpio_get_value(hdata->hpd_gpio);
> @@ -1887,7 +1944,6 @@ static struct of_device_id hdmi_match_types[] = {
>  static int hdmi_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> -	struct exynos_drm_hdmi_context *drm_hdmi_ctx;
>  	struct hdmi_context *hdata;
>  	struct s5p_hdmi_platform_data *pdata;
>  	struct resource *res;
> @@ -1902,20 +1958,13 @@ static int hdmi_probe(struct platform_device *pdev)
>  	if (!pdata)
>  		return -EINVAL;
>  
> -	drm_hdmi_ctx = devm_kzalloc(dev, sizeof(*drm_hdmi_ctx), GFP_KERNEL);
> -	if (!drm_hdmi_ctx)
> -		return -ENOMEM;
> -
>  	hdata = devm_kzalloc(dev, sizeof(struct hdmi_context), GFP_KERNEL);
>  	if (!hdata)
>  		return -ENOMEM;
>  
>  	mutex_init(&hdata->hdmi_mutex);
>  
> -	drm_hdmi_ctx->ctx = (void *)hdata;
> -	hdata->parent_ctx = (void *)drm_hdmi_ctx;
> -
> -	platform_set_drvdata(pdev, drm_hdmi_ctx);
> +	platform_set_drvdata(pdev, &hdmi_display);

This looks like a step backwards. Originally the driver had kind of per
instance data, while now it has a single global struct, making it harder
to properly support multiple HDMI instances, what is not unlikely to show
up in future SoCs.

So instead, I would embed an exynos_drm_display struct inside struct
hdmi_context and initialize it at runtime and use it to get access to
hdata, using container_of.

> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
> index 60935c4..39aed3e 100644
> --- a/drivers/gpu/drm/exynos/exynos_mixer.c
> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c
[snip]
> @@ -1184,21 +1190,17 @@ static struct of_device_id mixer_match_types[] = {
>  static int mixer_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> -	struct exynos_drm_hdmi_context *drm_hdmi_ctx;
>  	struct mixer_context *ctx;
>  	struct mixer_drv_data *drv;
>  	int ret;
>  
>  	dev_info(dev, "probe start\n");
>  
> -	drm_hdmi_ctx = devm_kzalloc(dev, sizeof(*drm_hdmi_ctx),
> -								GFP_KERNEL);
> -	if (!drm_hdmi_ctx)
> -		return -ENOMEM;
> -
> -	ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
> -	if (!ctx)
> +	ctx = devm_kzalloc(&pdev->dev, sizeof(*ctx), GFP_KERNEL);
> +	if (!ctx) {
> +		DRM_ERROR("failed to alloc mixer context.\n");
>  		return -ENOMEM;
> +	}
>  
>  	mutex_init(&ctx->mixer_mutex);
>  
> @@ -1211,18 +1213,14 @@ static int mixer_probe(struct platform_device *pdev)
>  			platform_get_device_id(pdev)->driver_data;
>  	}
>  
> -	ctx->dev = dev;
> -	ctx->parent_ctx = (void *)drm_hdmi_ctx;
> -	drm_hdmi_ctx->ctx = (void *)ctx;
> +	ctx->dev = &pdev->dev;
>  	ctx->vp_enabled = drv->is_vp_enabled;
>  	ctx->mxr_ver = drv->version;
>  	DRM_INIT_WAITQUEUE(&ctx->wait_vsync_queue);
>  	atomic_set(&ctx->wait_vsync_event, 0);
>  
> -	platform_set_drvdata(pdev, drm_hdmi_ctx);
> -
>  	/* acquire resources: regs, irqs, clocks */
> -	ret = mixer_resources_init(drm_hdmi_ctx, pdev);
> +	ret = mixer_resources_init(ctx, pdev);
>  	if (ret) {
>  		DRM_ERROR("mixer_resources_init failed\n");
>  		goto fail;
> @@ -1230,18 +1228,16 @@ static int mixer_probe(struct platform_device *pdev)
>  
>  	if (ctx->vp_enabled) {
>  		/* acquire vp resources: regs, irqs, clocks */
> -		ret = vp_resources_init(drm_hdmi_ctx, pdev);
> +		ret = vp_resources_init(ctx, pdev);
>  		if (ret) {
>  			DRM_ERROR("vp_resources_init failed\n");
>  			goto fail;
>  		}
>  	}
>  
> -	/* attach mixer driver to common hdmi. */
> -	exynos_mixer_drv_attach(drm_hdmi_ctx);
> -
> -	/* register specific callback point to common hdmi. */
> -	exynos_mixer_ops_register(&mixer_ops);
> +	mixer_manager.ctx = ctx;
> +	platform_set_drvdata(pdev, &mixer_manager);
> +	exynos_drm_manager_register(&mixer_manager);

Ditto.

Best regards,
Tomasz