Re: [PATCH v9 03/14] drm/mediatek: Add DSI sub driver

[Philipp Zabel <p.zabel@pengutronix.de>]

Hi Daniel,

Am Dienstag, den 02.02.2016, 21:32 +0800 schrieb Daniel Kurtz:
> Hi Philipp,
> 
> I ran into some issues when trying to bring up just the DSI path of
> the Mediatek DRM driver.
> Things were failing in probe/bind that triggered some oopses in the
> unbind/error paths.
> This resulted in the following review of the dsi patch...
> 
> On Tue, Jan 12, 2016 at 11:15 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > From: CK Hu <ck.hu@mediatek.com>
> >
> > This patch add a drm encoder/connector driver for the MIPI DSI function
> > block of the Mediatek display subsystem and a phy driver for the MIPI TX
> > D-PHY control module.
> >
> > Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > ---
> >  drivers/gpu/drm/mediatek/Kconfig       |   3 +
> >  drivers/gpu/drm/mediatek/Makefile      |   4 +-
> >  drivers/gpu/drm/mediatek/mtk_drm_drv.c |   2 +
> >  drivers/gpu/drm/mediatek/mtk_drm_drv.h |   2 +
> >  drivers/gpu/drm/mediatek/mtk_dsi.c     | 847 +++++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/mediatek/mtk_dsi.h     |  58 +++
> >  drivers/gpu/drm/mediatek/mtk_mipi_tx.c | 487 +++++++++++++++++++
> >  7 files changed, 1402 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/gpu/drm/mediatek/mtk_dsi.c
> >  create mode 100644 drivers/gpu/drm/mediatek/mtk_dsi.h
> >  create mode 100644 drivers/gpu/drm/mediatek/mtk_mipi_tx.c
> >
[...]
> > diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > new file mode 100644
> > index 0000000..6ab5a31
> > --- /dev/null
> > +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
[...]
> > +#define DSI_PHY_TIMECON3       0x11c
> > +#define CLK_HS_PRPR                    (0xff << 0)
> > +#define CLK_HS_POST                    (0xff << 8)
> > +#define CLK_HS_EXIT                    (0xff << 16)
> > +
> > +#define NS_TO_CYCLE(n, c)    ((n) / c + (((n) % c) ? 1 : 0))
> 
> () around each of the two 'c' in the macro.

Ok.

[...]
> > +static int mtk_dsi_poweron(struct mtk_dsi *dsi)
> > +{
> > +       struct device *dev = dsi->dev;
> > +       int ret;
> > +
> > +       if (++dsi->refcount != 1)
> > +               return 0;
> > +
> > +       /**
> > +        * data_rate = (pixel_clock / 1000) * pixel_dipth * mipi_ratio;
> > +        * pixel_clock unit is Khz, data_rata unit is MHz, so need divide 1000.
> > +        * mipi_ratio is mipi clk coefficient for balance the pixel clk in mipi.
> > +        * we set mipi_ratio is 1.05.
> > +        */
> > +       dsi->data_rate = dsi->vm.pixelclock * 3 * 21 / (1 * 1000 * 10);
> > +
> > +       ret = clk_set_rate(dsi->hs_clk, dsi->data_rate * 1000000);
> > +       if (ret < 0)
> > +               dev_err(dev, "Failed to set data rate: %d\n", ret);
> 
> Should we error out here?

Probably yes. At least we should check clk_get_rate before we try to go
ahead anyway. I'll return the error here.

> > +static void mtk_output_dsi_disable(struct mtk_dsi *dsi)
> > +{
> > +       if (!dsi->enabled)
> > +               return;
> > +
> > +       if (dsi->panel) {
> > +               if (drm_panel_disable(dsi->panel)) {
> > +                       DRM_ERROR("failed to disable the panel\n");
> > +                       return;
> > +               }
> > +       }
> > +
> > +       mtk_dsi_poweroff(dsi);
> 
> The order is a bit suspicious here; I would expect to poweroff dsi
> before the panel to mirror the turn on order.

CK, could you comment on this?

I can reorder this, but I'm not sure about the reasoning (what happens
hardware wise if we just cut panel power vs. if the DSI panel first sees
the ULP transition). Further, I don't have a panel to test, just the
PS8640.

> > +
> > +       dsi->enabled = false;
> > +}
[...]
> > +static int mtk_drm_attach_lcm_bridge(struct drm_bridge *bridge,
> 
> What is "lcm"?

No idea, I'll drop it.

> > +                                    struct drm_encoder *encoder)
> > +{
> > +       int ret;
> > +
> > +       encoder->bridge = bridge;
> > +       bridge->encoder = encoder;
> > +       ret = drm_bridge_attach(encoder->dev, bridge);
> > +       if (ret) {
> > +               DRM_ERROR("Failed to attach bridge to drm\n");
> > +               return ret;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +static int mtk_dsi_create_conn_enc(struct drm_device *drm, struct mtk_dsi *dsi)
> > +{
> > +       int ret;
> > +
> > +       ret = drm_encoder_init(drm, &dsi->encoder, &mtk_dsi_encoder_funcs,
> > +                              DRM_MODE_ENCODER_DSI);
> > +       if (ret) {
> > +               DRM_ERROR("Failed to encoder init to drm\n");
> > +               return ret;
> > +       }
> > +       drm_encoder_helper_add(&dsi->encoder, &mtk_dsi_encoder_helper_funcs);
> > +
> > +       dsi->encoder.possible_crtcs = 1;
> 
> This doesn't look right.  If there are N crtcs in the system, how can
> we know this DSI will always be associated with CRTC:0 ?

This is related to the comment in mtk_drm_drv.c:

	/*
	 * We currently support two fixed data streams, each statically
	 * assigned to a crtc:
	 * OVL0 -> COLOR0 -> AAL -> OD -> RDMA0 -> UFOE -> DSI0 ...
	 * ... and OVL1 -> COLOR1 -> GAMMA -> RDMA1 -> DPI0.
	 */

In theory the DSI0 sink can be driven by any of the OVL0/1, RDMA0/1/2
sources, depending on the display path configuration, but how those
sources should correspond to the crtcs in the dynamic case hasn't been
worked out yet.

> > +
> > +       /* Pre-empt DP connector creation if there's a bridge */
> > +       ret = mtk_drm_attach_lcm_bridge(dsi->bridge, &dsi->encoder);
> > +       if (!ret)
> > +               return 0;
> 
> Tricky.  The "no-error" case here is to return 0 and skip the rest of
> this function?
> In particular, this skips initializing and registering the connector.

Yes. There is no DSI connector in a system that has a DSI-to-eDP bridge
connected to the SoCs DSI output, so we leave it to the bridge driver to
register an eDP connector itself.

> Unfortunately, afaict, there is no way to tell that we skipped this,
> so in mtk_dsi_unbind() we will always call mtk_dsi_destroy_conn_enc(),
> which will try to cleanup the connector that we didn't init.

See below, drm_connector_init sets dsi->conn.dev, so we can use that to
detect the bridge case.
Although if the bridge driver should register the connector and then
fails for some other reason, we'll happily try to register our own.
Hm...

> > +
> > +       ret = drm_connector_init(drm, &dsi->conn, &mtk_dsi_connector_funcs,
> > +                                DRM_MODE_CONNECTOR_DSI);
> > +       if (ret) {
> > +               DRM_ERROR("Failed to connector init to drm\n");
> > +               goto errconnector;
> > +       }
> > +
> > +       drm_connector_helper_add(&dsi->conn, &mtk_dsi_connector_helper_funcs);
> > +
> > +       ret = drm_connector_register(&dsi->conn);
> > +       if (ret) {
> > +               DRM_ERROR("Failed to connector register to drm\n");
> > +               goto errconnectorreg;
> > +       }
> > +
> > +       dsi->conn.dpms = DRM_MODE_DPMS_OFF;
> > +       drm_mode_connector_attach_encoder(&dsi->conn, &dsi->encoder);
> > +
> > +       if (dsi->panel) {
> > +               ret = drm_panel_attach(dsi->panel, &dsi->conn);
> > +               if (ret) {
> > +                       DRM_ERROR("Failed to attact panel to drm\n");
> > +                       return ret;
> > +               }
> > +       }
> > +       return 0;
> > +
> > +errconnector:
> > +       drm_encoder_cleanup(&dsi->encoder);
> > +errconnectorreg:
> > +       drm_connector_cleanup(&dsi->conn);
> 
> The cleanup order is backwards; clean up encoder last to invert init order.
> Also, these labels are not very readable.

I'll reorder these and rename the labels.

[...]
> > +static void mtk_dsi_destroy_conn_enc(struct mtk_dsi *dsi)
> > +{
> > +       drm_encoder_cleanup(&dsi->encoder);
> > +       drm_connector_unregister(&dsi->conn);
> > +       drm_connector_cleanup(&dsi->conn);
> 
> unregistering / cleaning up the connector will oops here if we took
> the "Pre-empt DP connector creation if there's a bridge" case in
> mtk_dsi_create_conn_enc and our connector is not initialized.

I'll wrap the connector unregister/cleanup:

	/* Skip connector cleanup if creation was delegated to the bridge */
	if (dsi->conn.dev) {
		drm_connector_unregister(&dsi->conn);
		drm_connector_cleanup(&dsi->conn);
	}

[...]
> > +static void mtk_dsi_unbind(struct device *dev, struct device *master,
> > +                          void *data)
> > +{
> > +       struct drm_device *drm = data;
> > +       struct mtk_dsi *dsi;
> > +
> > +       dsi = platform_get_drvdata(to_platform_device(dev));
> 
> Just:
>        struct mtk_dsi *dsi = dev_get_drvdata(dev);

Ok.

[...]
> > +static int mtk_dsi_probe(struct platform_device *pdev)
> > +{
> > +       struct mtk_dsi *dsi;
> > +       struct device *dev = &pdev->dev;
> > +       struct device_node *remote_node, *endpoint;
> > +       struct resource *regs;
> > +       int comp_id;
> > +       int ret;
> > +
> > +       dsi = devm_kzalloc(dev, sizeof(*dsi), GFP_KERNEL);
> 
> Always NULL check allocations.

Ok.

> > +       dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_SYNC_PULSE;
> > +       dsi->format = MIPI_DSI_FMT_RGB888;
> > +       dsi->lanes = 4;
> 
> As far as I can tell, these three fields are constants, which means either we
> are missing code to parse these from somewhere, or we can hard code this
> throughout and simplify the driver.

These usually come from the struct panel_desc_dsi and are filled into
struct mipi_dsi_device for DSI bus probed panels. I'm not sure how we
can get to this information for platform device simple panels or I2C bus
probed encoders. In fact the PS8640 encoder used on oak doesn't even
have a fixed mode, but supports several from RGB565 to RGB101010.
This should probably be chosen dynamically, depending on the mode set.

[...]
> > diff --git a/drivers/gpu/drm/mediatek/mtk_mipi_tx.c b/drivers/gpu/drm/mediatek/mtk_mipi_tx.c
> > new file mode 100644
> > index 0000000..3b91a36
> > --- /dev/null
> > +++ b/drivers/gpu/drm/mediatek/mtk_mipi_tx.c
[...]
> > +static struct clk_ops mtk_mipi_tx_pll_ops = {
> 
> static const
[...]
> > +static struct phy_ops mtk_mipi_tx_ops = {
> 
> static const

Ok.

thanks
Philipp


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel