Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20190820230626.23253-1-john.stultz@linaro.org> <20190820230626.23253-20-john.stultz@linaro.org>
In-Reply-To: <20190820230626.23253-20-john.stultz@linaro.org>
From:   Daniel Vetter <daniel@ffwll.ch>
Date:   Tue, 7 Apr 2020 10:02:29 +0200
Message-ID: <CAKMK7uHr5U-pPsxdQ4MpfK5v8iLjphDFug_3VTiUAf06nhS=yQ@mail.gmail.com>
Subject: Re: [PATCH v5 19/25] drm: kirin: Add register connect helper
 functions in drm init
To:     John Stultz <john.stultz@linaro.org>
Cc:     lkml <linux-kernel@vger.kernel.org>,
        Xu YiPing <xuyiping@hisilicon.com>,
        Rongrong Zou <zourongrong@gmail.com>,
        Xinliang Liu <z.liuxinliang@hisilicon.com>,
        David Airlie <airlied@linux.ie>,
        dri-devel <dri-devel@lists.freedesktop.org>,
        Sam Ravnborg <sam@ravnborg.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, Aug 21, 2019 at 1:06 AM John Stultz <john.stultz@linaro.org> wrote:
>
> From: Xu YiPing <xuyiping@hisilicon.com>
>
> As part of refactoring the kirin driver to better support
> different hardware revisions, this patch adds a flag to the
> device specific driver data so that we can conditionally
> register the connectors at init.
>
> Cc: Rongrong Zou <zourongrong@gmail.com>
> Cc: Xinliang Liu <z.liuxinliang@hisilicon.com>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: dri-devel <dri-devel@lists.freedesktop.org>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> Acked-by: Xinliang Liu <z.liuxinliang@hisilicon.com>
> Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
> Signed-off-by: Xu YiPing <xuyiping@hisilicon.com>
> [jstultz: reworded commit message]
> Signed-off-by: John Stultz <john.stultz@linaro.org>

Whatever it is you're doing, this is nonsense. drm_dev_register does
register connectors for you. If you hotplug connectors later on, you
have to register them (manually), but definitely that should happen in
the hotplug code. General rule: If it's not a hotplugged dp mst
connector, calling drm_connector_register from drivers is pointless
(and yes I know there's tons of that stuff around, for historical
reasons).

Further the flag isn't even set, so this is just dead code.

Please revert.

Thanks, Daniel

> ---
> v5: checkpatch --strict whitespace fixups noticed by Sam
> ---
>  .../gpu/drm/hisilicon/kirin/kirin_drm_ade.c   |  1 +
>  .../gpu/drm/hisilicon/kirin/kirin_drm_drv.c   | 43 +++++++++++++++++++
>  .../gpu/drm/hisilicon/kirin/kirin_drm_drv.h   |  1 +
>  3 files changed, 45 insertions(+)
>
> diff --git a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
> index 21c5d457a97d..89bdc0388138 100644
> --- a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
> +++ b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c
> @@ -1073,6 +1073,7 @@ static struct drm_driver ade_driver = {
>  };
>
>  struct kirin_drm_data ade_driver_data = {
> +       .register_connects = false,
>         .num_planes = ADE_CH_NUM,
>         .prim_plane = ADE_CH1,
>         .channel_formats = channel_formats,
> diff --git a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c
> index c9faaa848cc6..68de8838da3c 100644
> --- a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c
> +++ b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c
> @@ -98,6 +98,40 @@ static int compare_of(struct device *dev, void *data)
>         return dev->of_node == data;
>  }
>
> +static int kirin_drm_connectors_register(struct drm_device *dev)
> +{
> +       struct drm_connector *connector;
> +       struct drm_connector *failed_connector;
> +       struct drm_connector_list_iter conn_iter;
> +       int ret;
> +
> +       mutex_lock(&dev->mode_config.mutex);
> +       drm_connector_list_iter_begin(dev, &conn_iter);
> +       drm_for_each_connector_iter(connector, &conn_iter) {
> +               ret = drm_connector_register(connector);
> +               if (ret) {
> +                       failed_connector = connector;
> +                       goto err;
> +               }
> +       }
> +       drm_connector_list_iter_end(&conn_iter);
> +       mutex_unlock(&dev->mode_config.mutex);
> +
> +       return 0;
> +
> +err:
> +       drm_connector_list_iter_begin(dev, &conn_iter);
> +       drm_for_each_connector_iter(connector, &conn_iter) {
> +               if (failed_connector == connector)
> +                       break;
> +               drm_connector_unregister(connector);
> +       }
> +       drm_connector_list_iter_end(&conn_iter);
> +       mutex_unlock(&dev->mode_config.mutex);
> +
> +       return ret;
> +}
> +
>  static int kirin_drm_bind(struct device *dev)
>  {
>         struct drm_device *drm_dev;
> @@ -117,8 +151,17 @@ static int kirin_drm_bind(struct device *dev)
>
>         drm_fbdev_generic_setup(drm_dev, 32);
>
> +       /* connectors should be registered after drm device register */
> +       if (driver_data->register_connects) {
> +               ret = kirin_drm_connectors_register(drm_dev);
> +               if (ret)
> +                       goto err_drm_dev_unregister;
> +       }
> +
>         return 0;
>
> +err_drm_dev_unregister:
> +       drm_dev_unregister(drm_dev);
>  err_kms_cleanup:
>         kirin_drm_kms_cleanup(drm_dev);
>  err_drm_dev_put:
> diff --git a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.h b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.h
> index fdbfc4a90f22..95f56c9960d5 100644
> --- a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.h
> +++ b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.h
> @@ -39,6 +39,7 @@ struct kirin_drm_data {
>         u32 channel_formats_cnt;
>         int config_max_width;
>         int config_max_height;
> +       bool register_connects;
>         u32 num_planes;
>         u32 prim_plane;
>
> --
> 2.17.1
>


-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch