Re: [PATCH v5 6/6] drm/sprd: add Unisoc's drm mipi dsi&dphy driver

[Maxime Ripard <maxime@cerno.tech>]

[-- Attachment #1: Type: text/plain, Size: 5064 bytes --]

On Wed, May 12, 2021 at 09:53:06PM +0800, Kevin Tang wrote:
> > > +struct dsi_reg {
> > > +     union _0x00 {
> > > +             u32 val;
> > > +             struct _DSI_VERSION {
> > > +             u32 dsi_version: 16;
> > > +             u32 reserved: 16;
> > > +             } bits;
> > > +     } DSI_VERSION;
> >
> > Using unions and structures to define the register is really frowned
> > upon in favor of defines, like you rightfully did in the crtc driver.
>
> This workload is too big, this design has been used for many years,
> so I actually want to keep it the same, but if it really doesn’t meet
> the current design.
>
> I can change the design, but it may take a lot of time......

There's no rush :)

> > > +static int sprd_dsi_find_panel(struct sprd_dsi *dsi)
> > > +{
> > > +     struct device *dev = dsi->host.dev;
> > > +     struct device_node *child, *lcds_node;
> > > +     struct drm_panel *panel;
> > > +
> > > +     /* search /lcds child node first */
> > > +     lcds_node = of_find_node_by_path("/lcds");
> > > +     for_each_child_of_node(lcds_node, child) {
> > > +             panel = of_drm_find_panel(child);
> > > +             if (!IS_ERR(panel)) {
> > > +                     dsi->panel = panel;
> > > +                     return 0;
> > > +             }
> > > +     }
> >
> > That's not part of your binding
> Ok, i will remove it.
> >
> > > +
> > > +     /*
> > > +      * If /lcds child node search failed, we search
> > > +      * the child of dsi host node.
> > > +      */
> > > +     for_each_child_of_node(dev->of_node, child) {
> > > +             panel = of_drm_find_panel(child);
> > > +             if (!IS_ERR(panel)) {
> > > +                     dsi->panel = panel;
> > > +                     return 0;
> > > +             }
> > > +     }
> >
> > And you don't need this either. You'll register a mipi_dsi_host,
> > that will in turn probe all the devices under that bus, and will
> > then call the .attach callback.
>
> This should be move to the .attach callback?

The panel pointer assignment can. The rest is useless.

> > > +     drm_err(dsi->drm, "of_drm_find_panel() failed\n");
> > > +     return -ENODEV;
> > > +}
> > > +
> > > +static int sprd_dsi_host_attach(struct mipi_dsi_host *host,
> > > +                        struct mipi_dsi_device *slave)
> > > +{
> > > +     struct sprd_dsi *dsi = host_to_dsi(host);
> > > +     struct dsi_context *ctx = &dsi->ctx;
> > > +     int ret;
> > > +
> > > +     dsi->slave = slave;
> > > +     ctx->lanes = slave->lanes;
> > > +     ctx->format = slave->format;
> > > +     ctx->byte_clk = slave->hs_rate / 8;
> > > +     ctx->esc_clk = slave->lp_rate;
> > > +
> > > +     if (slave->mode_flags & MIPI_DSI_MODE_VIDEO)
> > > +             ctx->work_mode = DSI_MODE_VIDEO;
> > > +     else
> > > +             ctx->work_mode = DSI_MODE_CMD;
> > > +
> > > +     if (slave->mode_flags & MIPI_DSI_MODE_VIDEO_BURST)
> > > +             ctx->burst_mode = VIDEO_BURST_WITH_SYNC_PULSES;
> > > +     else if (slave->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE)
> > > +             ctx->burst_mode = VIDEO_NON_BURST_WITH_SYNC_PULSES;
> > > +     else
> > > +             ctx->burst_mode = VIDEO_NON_BURST_WITH_SYNC_EVENTS;
> > > +
> > > +     if (slave->mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS)
> > > +             ctx->nc_clk_en = true;
> >
> > I'm not sure why you need to duplicate all this, can't you just
> > dereference the dsi->slave pointer when you need it?
>
> Sorry, can you help me with a demo?

In your sprd_dsi_encoder_enable function, you call sprd_dphy_init which
takes a pointer to struct dsi_context, and will call, say,
dsi_phy_datalane_en, using ctx->lanes.

Since you have access to the struct sprd_dsi in sprd_dsi_encoder_enable,
why not pass it and the mipi_dsi_device pointer to sprd_dphy_init, and
dereference slave->lanes in dsi_phy_datalane_en?

This will greatly reduce the size of the dsi_context structure.

> > > +static enum drm_mode_status
> > > +sprd_dsi_connector_mode_valid(struct drm_connector *connector,
> > > +                      struct drm_display_mode *mode)
> > > +{
> > > +     struct sprd_dsi *dsi = connector_to_dsi(connector);
> > > +
> > > +     drm_dbg(dsi->drm, "%s() mode: "DRM_MODE_FMT"\n", __func__, DRM_MODE_ARG(mode));
> > > +
> > > +     if (mode->type & DRM_MODE_TYPE_PREFERRED) {
> > > +             dsi->mode = mode;
> > > +             drm_display_mode_to_videomode(dsi->mode, &dsi->ctx.vm);
> > > +     }
> >
> > Again, what happens if the mode isn't the preferred one?
>
> We hope to restore the low-resolution image to the original resolution
> through the scaling algorithm while keeping the resolution unchanged.
> So whether it's dpu or dsi, must be keeping on preferred mode.

Is there any particular reason to do so? Why do you need to take the
preferred mode into account in the first place? Can't you just use
whatever drm_display_mode is being passed?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]