Re: [PATCH RFC 17/19] drm/bridge: lvds-encoder: Implement bus format negotiation for sn75lvds83

[Boris Brezillon <boris.brezillon@collabora.com>]

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);  
> 

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