Re: [PATCH v6 3/3] drm/rockchip: Add ROCKCHIP DW MIPI DSI controller driver

[Nickey Yang]

Hi Brian,


On 2017å12æ07æ 05:52, Brian Norris wrote:
> 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@xxxxxxxxxxxxxx>
> > Signed-off-by: Brian Norris <briannorris@xxxxxxxxxxxx>
> > Reviewed-by: Brian Norris <briannorris@xxxxxxxxxxxx>
> > Reviewed-by: Sean Paul <seanpaul@xxxxxxxxxxxx>
> > ---
> > 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?
So should we change this to
1ãremove dw_mipi_dsi_encoder_disable, and in
dw_mipi_dsi_encoder_mode_set:
 drm_of_encoder_active_endpoint_id ->
ÂÂ ÂÂ clk_prepare_enable ->
ÂÂÂÂÂÂ Â pm_runtime_get_sync ->
ÂÂÂÂÂÂÂÂÂÂ grf_config ->
ÂÂÂÂÂÂ Â pm_runtime_put_sync ->
ÂÂ ÂÂ clk_prepare_disable?
or
2ãuse dw_mipi_dsi_encoder_disable, and in
dw_mipi_dsi_encoder_mode_set:
ÂÂ pm_runtime_get_sync ->
 drm_of_encoder_active_endpoint_id ->
ÂÂ Â ÂÂ clk_prepare_enable ->
ÂÂÂÂÂÂÂÂÂÂ grf_config ->
ÂÂ ÂÂÂÂ clk_prepare_disable?
and call pm_runtime_put_sync if there is a failure in
drm_of_encoder_active_endpoint_id or clk_prepare_enable

> 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