Re: [PATCH v3 29/32] drm/exynos: Implement drm_connector directly in dp driver

From: Inki Dae <inki.dae@samsung.com>
To: Sean Paul <seanpaul@chromium.org>
Cc: "Stéphane Marchesin" <marcheu@chromium.org>,
	"DRI mailing list" <dri-devel@lists.freedesktop.org>
Subject: Re: [PATCH v3 29/32] drm/exynos: Implement drm_connector directly in dp driver
Date: Tue, 3 Dec 2013 13:45:44 +0900	[thread overview]
Message-ID: <CAAQKjZMQYN3sbH=sNov70=mugaCEzq9abb1Ru3K1GfoMLHweFQ@mail.gmail.com> (raw)
In-Reply-To: <1383063198-10526-30-git-send-email-seanpaul@chromium.org>

Hi Sean,

2013/10/30 Sean Paul <seanpaul@chromium.org>:
> This patch implements drm_connector directly in the dp driver, this will
> allow us to move away from the exynos_drm_connector layer.
>
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> ---
>
> Changes in v3:
>         - Added to the patchset
>
>  drivers/gpu/drm/exynos/exynos_dp_core.c | 99 ++++++++++++++++++++++++++++++---
>  drivers/gpu/drm/exynos/exynos_dp_core.h |  4 ++
>  2 files changed, 94 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/exynos/exynos_dp_core.c b/drivers/gpu/drm/exynos/exynos_dp_core.c
> index 476d3b0..139ea15 100644
> --- a/drivers/gpu/drm/exynos/exynos_dp_core.c
> +++ b/drivers/gpu/drm/exynos/exynos_dp_core.c
> @@ -22,10 +22,15 @@
>  #include <video/of_videomode.h>
>
>  #include <drm/drmP.h>
> +#include <drm/drm_crtc.h>
> +#include <drm/drm_crtc_helper.h>
>
>  #include "exynos_drm_drv.h"
>  #include "exynos_dp_core.h"
>
> +#define ctx_from_connector(c)  container_of(c, struct exynos_dp_device, \
> +                                       connector)
> +
>  static int exynos_dp_init_dp(struct exynos_dp_device *dp)
>  {
>         exynos_dp_reset(dp);
> @@ -896,21 +901,98 @@ static void exynos_dp_hotplug(struct work_struct *work)
>                 dev_err(dp->dev, "unable to config video\n");
>  }
>
> -static bool exynos_dp_display_is_connected(struct exynos_drm_display *display)
> +static enum drm_connector_status exynos_dp_detect(
> +                               struct drm_connector *connector, bool force)
>  {
> -       return true;
> +       return connector_status_connected;

No, eDP can be hot-plugged. Check if lcd panel is connected or not,
and if connected then return connector_status_connected else
connector_status_disconnected. See the interrupt handler.

>  }
>
> -static void *exynos_dp_get_panel(struct exynos_drm_display *display)
> +static void exynos_dp_connector_destroy(struct drm_connector *connector)
>  {
> -       struct exynos_dp_device *dp = display->ctx;
> +}
> +
> +static struct drm_connector_funcs exynos_dp_connector_funcs = {
> +       .dpms = drm_helper_connector_dpms,
> +       .fill_modes = drm_helper_probe_single_connector_modes,
> +       .detect = exynos_dp_detect,
> +       .destroy = exynos_dp_connector_destroy,
> +};
> +
> +static int exynos_dp_get_modes(struct drm_connector *connector)
> +{
> +       struct exynos_dp_device *dp = ctx_from_connector(connector);
> +       struct drm_display_mode *mode;
> +
> +       mode = drm_mode_create(connector->dev);
> +       if (!mode) {
> +               DRM_ERROR("failed to create a new display mode.\n");
> +               return 0;
> +       }
>
> -       return &dp->panel;
> +       drm_display_mode_from_videomode(&dp->panel.vm, mode);
> +       mode->width_mm = dp->panel.width_mm;
> +       mode->height_mm = dp->panel.height_mm;
> +       connector->display_info.width_mm = mode->width_mm;
> +       connector->display_info.height_mm = mode->height_mm;
> +
> +       mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
> +       drm_mode_set_name(mode);
> +       drm_mode_probed_add(connector, mode);
> +
> +       return 1;
>  }
>
> -static int exynos_dp_check_mode(struct exynos_drm_display *display,
> +static int exynos_dp_mode_valid(struct drm_connector *connector,
>                         struct drm_display_mode *mode)
>  {
> +       return MODE_OK;
> +}
> +
> +static struct drm_encoder *exynos_dp_best_encoder(
> +                       struct drm_connector *connector)
> +{
> +       struct exynos_dp_device *dp = ctx_from_connector(connector);
> +
> +       return dp->encoder;

eDP context doesn't need a encoder as its member. And you can get best
encoder through connector->encoder.

> +}
> +
> +static struct drm_connector_helper_funcs exynos_dp_connector_helper_funcs = {
> +       .get_modes = exynos_dp_get_modes,
> +       .mode_valid = exynos_dp_mode_valid,
> +       .best_encoder = exynos_dp_best_encoder,
> +};
> +
> +static int exynos_dp_initialize(struct exynos_drm_display *display,
> +                               struct drm_device *drm_dev)
> +{
> +       struct exynos_dp_device *dp = display->ctx;
> +
> +       dp->drm_dev = drm_dev;
> +
> +       return 0;
> +}
> +
> +static int exynos_dp_create_connector(struct exynos_drm_display *display,
> +                               struct drm_encoder *encoder)
> +{
> +       struct exynos_dp_device *dp = display->ctx;
> +       struct drm_connector *connector = &dp->connector;
> +       int ret;
> +
> +       dp->encoder = encoder;

Unnecessary line, Add connector->encoder = encoder instead.

> +       connector->polled = DRM_CONNECTOR_POLL_HPD;
> +
> +       ret = drm_connector_init(dp->drm_dev, connector,
> +                       &exynos_dp_connector_funcs, DRM_MODE_CONNECTOR_eDP);
> +       if (ret) {
> +               DRM_ERROR("Failed to initialize connector with drm\n");
> +               return ret;
> +       }
> +
> +       drm_connector_helper_add(connector, &exynos_dp_connector_helper_funcs);
> +       drm_sysfs_connector_add(connector);
> +       drm_mode_connector_attach_encoder(connector, encoder);
> +
>         return 0;
>  }
>
> @@ -986,9 +1068,8 @@ static void exynos_dp_dpms(struct exynos_drm_display *display, int mode)
>  }
>
>  static struct exynos_drm_display_ops exynos_dp_display_ops = {
> -       .is_connected = exynos_dp_display_is_connected,
> -       .get_panel = exynos_dp_get_panel,
> -       .check_mode = exynos_dp_check_mode,
> +       .initialize = exynos_dp_initialize,
> +       .create_connector = exynos_dp_create_connector,
>         .dpms = exynos_dp_dpms,
>  };
>
> diff --git a/drivers/gpu/drm/exynos/exynos_dp_core.h b/drivers/gpu/drm/exynos/exynos_dp_core.h
> index 0c67c19..da3fa7e 100644
> --- a/drivers/gpu/drm/exynos/exynos_dp_core.h
> +++ b/drivers/gpu/drm/exynos/exynos_dp_core.h
> @@ -13,6 +13,7 @@
>  #ifndef _EXYNOS_DP_CORE_H
>  #define _EXYNOS_DP_CORE_H
>
> +#include <drm/drm_crtc.h>
>  #include <drm/exynos_drm.h>
>  #include <video/exynos_dp.h>
>
> @@ -36,6 +37,9 @@ struct link_train {
>
>  struct exynos_dp_device {
>         struct device           *dev;
> +       struct drm_device       *drm_dev;
> +       struct drm_connector    connector;
> +       struct drm_encoder      *encoder;

I don't see why eDP context needs encoder pointer.

>         struct clk              *clock;
>         unsigned int            irq;
>         void __iomem            *reg_base;
> --
> 1.8.4
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel