On Thu, 22 Aug 2019 03:32:10 +0300
Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
>
> 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);
>
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
