Date: Wed, 6 Dec 2017 13:52:43 -0800
From: Brian Norris <briannorris@chromium.org>
To: Nickey Yang <nickey.yang@rock-chips.com>
Cc: robh+dt@kernel.org, heiko@sntech.de, mark.rutland@arm.com,
        airlied@linux.ie, linux-kernel@vger.kernel.org,
        dri-devel@lists.freedesktop.org, linux-rockchip@lists.infradead.org,
        laurent.pinchart@ideasonboard.com, seanpaul@chromium.org,
        mka@chromium.org, hoegsberg@gmail.com, architt@codeaurora.org,
        philippe.cornu@st.com, yannick.fertre@st.com, hl@rock-chips.com,
        zyw@rock-chips.com, xbl@rock-chips.com
Subject: Re: [PATCH v6 3/3] drm/rockchip: Add ROCKCHIP DW MIPI DSI controller
 driver
Message-ID: <20171206215243.GB14257@google.com>
References: <1512551301-12946-1-git-send-email-nickey.yang@rock-chips.com>
 <1512551301-12946-4-git-send-email-nickey.yang@rock-chips.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <1512551301-12946-4-git-send-email-nickey.yang@rock-chips.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 6165
Lines: 177

Hi Nickey, others,

I just want to highlight a thing or two here. Otherwise, my
'Reviewed-by' still basically stands (FWIW).

On Wed, Dec 06, 2017 at 05:08:21PM +0800, Nickey Yang wrote:
> Add the ROCKCHIP DSI controller driver that uses the Synopsys DesignWare
> MIPI DSI host controller bridge.
> 
> Signed-off-by: Nickey Yang <nickey.yang@rock-chips.com>
> Signed-off-by: Brian Norris <briannorris@chromium.org>
> Reviewed-by: Brian Norris <briannorris@chromium.org>
> Reviewed-by: Sean Paul <seanpaul@chromium.org>
> ---
> change:
> 
> v2:
>    add err_pllref, remove unnecessary encoder.enable & disable
>    correct spelling mistakes
> v3:
>    call dw_mipi_dsi_unbind() in dw_mipi_dsi_rockchip_unbind()
>    fix typo, use of_device_get_match_data(),
>    change some ‘bind()’ logic into 'probe()'
>    add 'dev_set_drvdata()'
> v4:
>   return -EINVAL when can not get best_freq
>   add a clarifying comment when get vco
>   add review tag
> v5:
>   keep our power domain enabled while touching GRF
> v6:
>   change func dw_mipi_encoder_disable name to
>   dw_mipi_dsi_encoder_disable

We noticed a regression w.r.t. pm_runtime_*() handling using this patch,
hence the pm_runtime changes in v5/v6. We actually need to keep our
power domain enabled in the mode_set() function, where we start to
configure some Rockchip-specific registers (GRF). More on that below.

> 
>  drivers/gpu/drm/rockchip/Kconfig                |    2 +-
>  drivers/gpu/drm/rockchip/Makefile               |    2 +-
>  drivers/gpu/drm/rockchip/dw-mipi-dsi.c          | 1349 -----------------------
>  drivers/gpu/drm/rockchip/dw-mipi-dsi_rockchip.c |  785 +++++++++++++
>  drivers/gpu/drm/rockchip/rockchip_drm_drv.c     |    2 +-
>  drivers/gpu/drm/rockchip/rockchip_drm_drv.h     |    2 +-
>  6 files changed, 789 insertions(+), 1353 deletions(-)
>  delete mode 100644 drivers/gpu/drm/rockchip/dw-mipi-dsi.c
>  create mode 100644 drivers/gpu/drm/rockchip/dw-mipi-dsi_rockchip.c
> 

...

> diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi_rockchip.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi_rockchip.c
> new file mode 100644
> index 0000000..66ab6fe
> --- /dev/null
> +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi_rockchip.c
> @@ -0,0 +1,785 @@

...

> +static void dw_mipi_dsi_encoder_mode_set(struct drm_encoder *encoder,
> +					 struct drm_display_mode *mode,
> +					 struct drm_display_mode *adjusted)
> +{
> +	struct dw_mipi_dsi_rockchip *dsi = to_dsi(encoder);
> +	const struct rockchip_dw_dsi_chip_data *cdata = dsi->cdata;
> +	int val, ret, mux;
> +
> +	mux = drm_of_encoder_active_endpoint_id(dsi->dev->of_node,
> +						&dsi->encoder);
> +	if (mux < 0)
> +		return;
> +	/*
> +	 * For the RK3399, the clk of grf must be enabled before writing grf
> +	 * register. And for RK3288 or other soc, this grf_clk must be NULL,
> +	 * the clk_prepare_enable return true directly.
> +	 */
> +	ret = clk_prepare_enable(dsi->grf_clk);
> +	if (ret) {
> +		DRM_DEV_ERROR(dsi->dev, "Failed to enable grf_clk: %d\n", ret);
> +		return;
> +	}
> +	pm_runtime_get_sync(dsi->dev);

What happens if there's a clk_prepare_enable() failure or failure to
retrieve the endpoint ID earlier in this function? You won't call
pm_runtime_get_*()...but might we still see a call to
dw_mipi_dsi_encoder_disable(), which would mean we get unbalanced
runtime PM status?

Also (and more importantly), is it fair to do all of this in mode_set()?
I believe Archit asked about this before, and the reason we're doing
this stuff in mode_set() now (where previously, the Rockchip driver was
doing it in ->enable()) is because when Philippe extracted the synopsys
bridge driver, that code migrated to ->mode_set().

But, I'm reading the comments on drm_encoder_helper_funcs::mode_set, and
I see:

        /**
         * @mode_set:
         *
         * This callback is used to update the display mode of an encoder.
         *
         * Note that the display pipe is completely off when this function is
         * called. Drivers which need hardware to be running before they program
         * the new display mode (because they implement runtime PM) should not
         * use this hook, because the helper library calls it only once and not
         * every time the display pipeline is suspend using either DPMS or the
         * new "ACTIVE" property. Such drivers should instead move all their
         * encoder setup into the @enable callback.

That sounds like perhaps the synopsys driver should be moving to use
->enable() instead, so the Rockchip driver can do that as well?

At any rate, I haven't found any real problem with using mode_set() so
far, but I was just curious what the recommended practice was.

> +	val = cdata->dsi0_en_bit << 16;
> +	if (mux)
> +		val |= cdata->dsi0_en_bit;
> +	regmap_write(dsi->grf_regmap, cdata->grf_switch_reg, val);
> +
> +	if (cdata->grf_dsi0_mode_reg)
> +		regmap_write(dsi->grf_regmap, cdata->grf_dsi0_mode_reg,
> +			     cdata->grf_dsi0_mode);
> +
> +	clk_disable_unprepare(dsi->grf_clk);
> +}
> +
> +static int
> +dw_mipi_dsi_encoder_atomic_check(struct drm_encoder *encoder,
> +				 struct drm_crtc_state *crtc_state,
> +				 struct drm_connector_state *conn_state)
> +{
> +	struct rockchip_crtc_state *s = to_rockchip_crtc_state(crtc_state);
> +	struct dw_mipi_dsi_rockchip *dsi = to_dsi(encoder);
> +
> +	switch (dsi->format) {
> +	case MIPI_DSI_FMT_RGB888:
> +		s->output_mode = ROCKCHIP_OUT_MODE_P888;
> +		break;
> +	case MIPI_DSI_FMT_RGB666:
> +		s->output_mode = ROCKCHIP_OUT_MODE_P666;
> +		break;
> +	case MIPI_DSI_FMT_RGB565:
> +		s->output_mode = ROCKCHIP_OUT_MODE_P565;
> +		break;
> +	default:
> +		WARN_ON(1);
> +		return -EINVAL;
> +	}
> +
> +	s->output_type = DRM_MODE_CONNECTOR_DSI;
> +
> +	return 0;
> +}
> +
> +static void dw_mipi_dsi_encoder_disable(struct drm_encoder *encoder)
> +{
> +	struct dw_mipi_dsi_rockchip *dsi = to_dsi(encoder);
> +
> +	pm_runtime_put(dsi->dev);
> +}
> +
> +static const struct drm_encoder_helper_funcs
> +dw_mipi_dsi_encoder_helper_funcs = {
> +	.mode_set = dw_mipi_dsi_encoder_mode_set,
> +	.atomic_check = dw_mipi_dsi_encoder_atomic_check,
> +	.disable = dw_mipi_dsi_encoder_disable,
> +};

... 

Brian