| Message ID | 20190808151150.16336-18-boris.brezillon@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: Add support for bus-format negotiation | expand |
Hi Boris, Thank you for the patch. On Thu, Aug 08, 2019 at 05:11:48PM +0200, Boris Brezillon wrote: > The SN75LVDS83 bridge support several input formats except the input > format is directly related to the expected output format. Let's express > that constraint by setting the bridge_state->output_bus_cfg.fmt field > to the appropriate value. > > The previous element in the chain might be able to use this information > to pick the appropriate output bus format. > > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com> This driver is supposed to only support LVDS encoders that require no special configuration, and thus not contain device-specific code. I'm not sure I like departing from this :-S Would there be a way to make this more generic ? Do you think the code below could be generalized, or is it really device-specific ? > --- > drivers/gpu/drm/bridge/lvds-encoder.c | 48 +++++++++++++++++++++++++++ > 1 file changed, 48 insertions(+) > > diff --git a/drivers/gpu/drm/bridge/lvds-encoder.c b/drivers/gpu/drm/bridge/lvds-encoder.c > index da7013c5faf1..8d399d1828b0 100644 > --- a/drivers/gpu/drm/bridge/lvds-encoder.c > +++ b/drivers/gpu/drm/bridge/lvds-encoder.c > @@ -159,9 +159,57 @@ static int lvds_encoder_remove(struct platform_device *pdev) > return 0; > } > > +static int sn75lvds83_atomic_check(struct drm_bridge *bridge, > + struct drm_bridge_state *bridge_state, > + struct drm_crtc_state *crtc_state, > + struct drm_connector_state *conn_state) > +{ > + int ret; > + > + ret = drm_atomic_bridge_choose_output_bus_cfg(bridge_state, crtc_state, > + conn_state); > + if (ret) > + return ret; > + > + /* > + * The output bus format has a direct impact on the expected input bus > + * format. > + */ > + switch (bridge_state->output_bus_cfg.fmt) { > + case MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA: > + case MEDIA_BUS_FMT_RGB888_1X7X4_SPWG: > + /* > + * JEIDA and SPWG variants theoretically require different pin > + * mapping, but MEDIA_BUS_FMT_ definitions do not allow > + * fined-grained pin placement definition, and this is > + * something we expect to be taken care of at board design > + * time, so let's ignore this for now. > + * If it becomes a problem, we can always add a way to override > + * the bus format with a FW property. > + */ > + bridge_state->input_bus_cfg.fmt = MEDIA_BUS_FMT_RGB888_1X24; > + break; > + case MEDIA_BUS_FMT_RGB666_1X7X3_SPWG: > + bridge_state->input_bus_cfg.fmt = MEDIA_BUS_FMT_RGB666_1X18; > + break; > + default: > + bridge_state->input_bus_cfg.fmt = 0; > + break; > + } These seems a bit fragile. The SN75LVDS83 has a 28-bit input data bus, and maps those bits to 3x8 RGB data + 1x4 control signals. It serialises the data and outputs them on 4 LVDS pairs. When a panel uses MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA or MEDIA_BUS_FMT_RGB888_1X7X4_SPWG, the 4 LVDS pairs are used, and MEDIA_BUS_FMT_RGB888_1X24 is required. When outputting MEDIA_BUS_FMT_RGB666_1X7X3_SPWG, the fourth LVDS pair is ignored, but the 3x8 RGB input bus can still receive MEDIA_BUS_FMT_RGB888_1X24, the 2 LSBs of each component will just be discarded. The source can thus ignore those 3x2 LSBs, but it still has to map the 3x6 MSBs to the high order bits of the 3x8 signals. *However*, if the hardware designs wires the 3x6 MSBs of the SN75LVDS83 to the 3x6 LSBs of the source, then the source will have to produce MEDIA_BUS_FMT_RGB666_1X18. Hardcoding the mapping as done here thus seems to be system-specific. I think you need to check how signals are connected (through DT properties, bus-width and data-shift come to mind). > + > + /* Propagate the bus_flags. */ > + bridge_state->input_bus_cfg.flags = bridge_state->output_bus_cfg.flags; > + return 0; > +} > + > +static const struct lvds_encoder_ops sn75lvds83_ops = { > + .atomic_check = sn75lvds83_atomic_check, > +}; > + > static const struct of_device_id lvds_encoder_match[] = { > { .compatible = "lvds-encoder" }, > { .compatible = "thine,thc63lvdm83d" }, > + { .compatible = "ti,sn75lvds83", .data = &sn75lvds83_ops }, > {}, > }; > MODULE_DEVICE_TABLE(of, lvds_encoder_match);
On Thu, 22 Aug 2019 03:32:10 +0300 Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > Hi Boris, > > Thank you for the patch. > > On Thu, Aug 08, 2019 at 05:11:48PM +0200, Boris Brezillon wrote: > > The SN75LVDS83 bridge support several input formats except the input > > format is directly related to the expected output format. Let's express > > that constraint by setting the bridge_state->output_bus_cfg.fmt field > > to the appropriate value. > > > > The previous element in the chain might be able to use this information > > to pick the appropriate output bus format. > > > > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com> > > This driver is supposed to only support LVDS encoders that require no > special configuration, and thus not contain device-specific code. I'm > not sure I like departing from this :-S > > Would there be a way to make this more generic ? Do you think the code > below could be generalized, or is it really device-specific ? The list of supported formats is likely to differ, so you'd at least need to attach that to the compat. Also not sure the RGB -> LVDS format conversion always follow the same logic, hence the decision to let bridges implement ->atomic_check() if they want/need to. Should I create a new driver for this bridge? > > > --- > > drivers/gpu/drm/bridge/lvds-encoder.c | 48 +++++++++++++++++++++++++++ > > 1 file changed, 48 insertions(+) > > > > diff --git a/drivers/gpu/drm/bridge/lvds-encoder.c b/drivers/gpu/drm/bridge/lvds-encoder.c > > index da7013c5faf1..8d399d1828b0 100644 > > --- a/drivers/gpu/drm/bridge/lvds-encoder.c > > +++ b/drivers/gpu/drm/bridge/lvds-encoder.c > > @@ -159,9 +159,57 @@ static int lvds_encoder_remove(struct platform_device *pdev) > > return 0; > > } > > > > +static int sn75lvds83_atomic_check(struct drm_bridge *bridge, > > + struct drm_bridge_state *bridge_state, > > + struct drm_crtc_state *crtc_state, > > + struct drm_connector_state *conn_state) > > +{ > > + int ret; > > + > > + ret = drm_atomic_bridge_choose_output_bus_cfg(bridge_state, crtc_state, > > + conn_state); > > + if (ret) > > + return ret; > > + > > + /* > > + * The output bus format has a direct impact on the expected input bus > > + * format. > > + */ > > + switch (bridge_state->output_bus_cfg.fmt) { > > + case MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA: > > + case MEDIA_BUS_FMT_RGB888_1X7X4_SPWG: > > + /* > > + * JEIDA and SPWG variants theoretically require different pin > > + * mapping, but MEDIA_BUS_FMT_ definitions do not allow > > + * fined-grained pin placement definition, and this is > > + * something we expect to be taken care of at board design > > + * time, so let's ignore this for now. > > + * If it becomes a problem, we can always add a way to override > > + * the bus format with a FW property. > > + */ > > + bridge_state->input_bus_cfg.fmt = MEDIA_BUS_FMT_RGB888_1X24; > > + break; > > + case MEDIA_BUS_FMT_RGB666_1X7X3_SPWG: > > + bridge_state->input_bus_cfg.fmt = MEDIA_BUS_FMT_RGB666_1X18; > > + break; > > + default: > > + bridge_state->input_bus_cfg.fmt = 0; > > + break; > > + } > > These seems a bit fragile. The SN75LVDS83 has a 28-bit input data bus, > and maps those bits to 3x8 RGB data + 1x4 control signals. It serialises > the data and outputs them on 4 LVDS pairs. When a panel uses > MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA or MEDIA_BUS_FMT_RGB888_1X7X4_SPWG, the > 4 LVDS pairs are used, and MEDIA_BUS_FMT_RGB888_1X24 is required. When > outputting MEDIA_BUS_FMT_RGB666_1X7X3_SPWG, the fourth LVDS pair is > ignored, but the 3x8 RGB input bus can still receive > MEDIA_BUS_FMT_RGB888_1X24, the 2 LSBs of each component will just be > discarded. The source can thus ignore those 3x2 LSBs, but it still has > to map the 3x6 MSBs to the high order bits of the 3x8 signals. > *However*, if the hardware designs wires the 3x6 MSBs of the SN75LVDS83 > to the 3x6 LSBs of the source, then the source will have to produce > MEDIA_BUS_FMT_RGB666_1X18. Hardcoding the mapping as done here thus > seems to be system-specific. I think you need to check how signals are > connected (through DT properties, bus-width and data-shift come to > mind). True, I just wanted to keep it simple until the need for the other case arises. Should we make the bus-width/data-shift mandatory for that bridge, or can we have a default behavior (like the one proposed here) when the props are missing? > > > + > > + /* Propagate the bus_flags. */ > > + bridge_state->input_bus_cfg.flags = bridge_state->output_bus_cfg.flags; > > + return 0; > > +} > > + > > +static const struct lvds_encoder_ops sn75lvds83_ops = { > > + .atomic_check = sn75lvds83_atomic_check, > > +}; > > + > > static const struct of_device_id lvds_encoder_match[] = { > > { .compatible = "lvds-encoder" }, > > { .compatible = "thine,thc63lvdm83d" }, > > + { .compatible = "ti,sn75lvds83", .data = &sn75lvds83_ops }, > > {}, > > }; > > MODULE_DEVICE_TABLE(of, lvds_encoder_match); >
diff --git a/drivers/gpu/drm/bridge/lvds-encoder.c b/drivers/gpu/drm/bridge/lvds-encoder.c index da7013c5faf1..8d399d1828b0 100644 --- a/drivers/gpu/drm/bridge/lvds-encoder.c +++ b/drivers/gpu/drm/bridge/lvds-encoder.c @@ -159,9 +159,57 @@ static int lvds_encoder_remove(struct platform_device *pdev) return 0; } +static int sn75lvds83_atomic_check(struct drm_bridge *bridge, + struct drm_bridge_state *bridge_state, + struct drm_crtc_state *crtc_state, + struct drm_connector_state *conn_state) +{ + int ret; + + ret = drm_atomic_bridge_choose_output_bus_cfg(bridge_state, crtc_state, + conn_state); + if (ret) + return ret; + + /* + * The output bus format has a direct impact on the expected input bus + * format. + */ + switch (bridge_state->output_bus_cfg.fmt) { + case MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA: + case MEDIA_BUS_FMT_RGB888_1X7X4_SPWG: + /* + * JEIDA and SPWG variants theoretically require different pin + * mapping, but MEDIA_BUS_FMT_ definitions do not allow + * fined-grained pin placement definition, and this is + * something we expect to be taken care of at board design + * time, so let's ignore this for now. + * If it becomes a problem, we can always add a way to override + * the bus format with a FW property. + */ + bridge_state->input_bus_cfg.fmt = MEDIA_BUS_FMT_RGB888_1X24; + break; + case MEDIA_BUS_FMT_RGB666_1X7X3_SPWG: + bridge_state->input_bus_cfg.fmt = MEDIA_BUS_FMT_RGB666_1X18; + break; + default: + bridge_state->input_bus_cfg.fmt = 0; + break; + } + + /* Propagate the bus_flags. */ + bridge_state->input_bus_cfg.flags = bridge_state->output_bus_cfg.flags; + return 0; +} + +static const struct lvds_encoder_ops sn75lvds83_ops = { + .atomic_check = sn75lvds83_atomic_check, +}; + static const struct of_device_id lvds_encoder_match[] = { { .compatible = "lvds-encoder" }, { .compatible = "thine,thc63lvdm83d" }, + { .compatible = "ti,sn75lvds83", .data = &sn75lvds83_ops }, {}, }; MODULE_DEVICE_TABLE(of, lvds_encoder_match);
The SN75LVDS83 bridge support several input formats except the input format is directly related to the expected output format. Let's express that constraint by setting the bridge_state->output_bus_cfg.fmt field to the appropriate value. The previous element in the chain might be able to use this information to pick the appropriate output bus format. Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com> --- drivers/gpu/drm/bridge/lvds-encoder.c | 48 +++++++++++++++++++++++++++ 1 file changed, 48 insertions(+)