Hi Fabrizio,
Thank you for the patch.
On Wed, Aug 28, 2019 at 07:36:38PM +0100, Fabrizio Castro wrote:
> The driver doesn't support dual-link LVDS displays, and the way
> it identifies bridges won't allow for dual-LVDS displays to be
> connected. Also, it's not possible to swap even and odd pixels
> around in case the wiring isn't taking advantage of the default
> hardware configuration. Further more, the "mode" of the companion
> encoder should be same as the mode of the primary encoder.
>
> Rework the driver to improve all of the above, so that it can
> support dual-LVDS displays.
That's lots of changes in one patch, could it be split to ease review ?
Also, should the commit message be reworded to explain what the patch
does, instead of enumerating issues ? When there's a single issue being
addressed in a patch it's usually fine, but there the change is larger,
without an explanation of how you intend to fix the issues I can't tell
if the code really matches your intent.
> Signed-off-by: Fabrizio Castro <fabrizio.castro@xxxxxxxxxxxxxx>
>
> ---
> v2->v3:
> * reworked to take advantange of the new dt-bindings
> * squashed in the patche for fixing the companion's mode
>
> Laurent,
>
> unfortunately the best way to get the companion encoder to use
> the same mode as the primary encoder is setting the mode directly
> without calling into rcar_lvds_mode_set for the companion encoder,
> as the below test fails for the companion encoder in
> rcar_lvds_get_lvds_mode:
> if (!info->num_bus_formats || !info->bus_formats)
Would "[PATCH] drm: rcar-du: lvds: Get mode from state" help here ?
Maybe you could review that patch, I could then include it in my -next
branch, your work would be simplified, and everybody would be happy ?
:-)
> Anyhow, setting the mode for the companion encoder doesn't seem
> to be mandary according to the experiments I have been running,
> but the HW User's Manual doesn't really say much about this,
> therefore I think the safest option is still to set the mode for
> the companion encoder.
I agree it should be done.
> ---
> drivers/gpu/drm/rcar-du/rcar_lvds.c | 110 +++++++++++++++++++++---------------
> 1 file changed, 65 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> index 3fe0b86..dfec5e7 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> @@ -20,6 +20,8 @@
> #include <drm/drm_atomic.h>
> #include <drm/drm_atomic_helper.h>
> #include <drm/drm_bridge.h>
> +#include <drm/drm_bus_timings.h>
> +#include <drm/drm_of.h>
> #include <drm/drm_panel.h>
> #include <drm/drm_probe_helper.h>
>
> @@ -69,6 +71,7 @@ struct rcar_lvds {
>
> struct drm_bridge *companion;
> bool dual_link;
> + bool stripe_swap_data;
> };
>
> #define bridge_to_rcar_lvds(b) \
> @@ -439,12 +442,20 @@ static void rcar_lvds_enable(struct drm_bridge *bridge)
> rcar_lvds_write(lvds, LVDCHCR, lvdhcr);
>
> if (lvds->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK) {
> - /*
> - * Configure vertical stripe based on the mode of operation of
> - * the connected device.
> - */
> - rcar_lvds_write(lvds, LVDSTRIPE,
> - lvds->dual_link ? LVDSTRIPE_ST_ON : 0);
> + u32 lvdstripe = 0;
> +
> + if (lvds->dual_link)
> + /*
> + * Configure vertical stripe based on the mode of
> + * operation of the connected device.
> + *
> + * ST_SWAP from LVD1STRIPE is reserved, do not set
> + * in the companion LVDS
> + */
> + lvdstripe = LVDSTRIPE_ST_ON |
> + (lvds->companion && lvds->stripe_swap_data ?
> + LVDSTRIPE_ST_SWAP : 0);
> + rcar_lvds_write(lvds, LVDSTRIPE, lvdstripe);
> }
>
> /*
> @@ -603,6 +614,11 @@ static void rcar_lvds_mode_set(struct drm_bridge *bridge,
> lvds->display_mode = *adjusted_mode;
>
> rcar_lvds_get_lvds_mode(lvds);
> + if (lvds->companion) {
> + struct rcar_lvds *companion_lvds = bridge_to_rcar_lvds(
> + lvds->companion);
> + companion_lvds->mode = lvds->mode;
> + }
> }
>
> static int rcar_lvds_attach(struct drm_bridge *bridge)
> @@ -667,9 +683,10 @@ EXPORT_SYMBOL_GPL(rcar_lvds_dual_link);
> static int rcar_lvds_parse_dt_companion(struct rcar_lvds *lvds)
> {
> const struct of_device_id *match;
> - struct device_node *companion;
> + struct device_node *companion, *p0 = NULL, *p1 = NULL;
> struct device *dev = lvds->dev;
> - int ret = 0;
> + struct rcar_lvds *companion_lvds;
> + int ret = 0, dual_link;
>
> /* Locate the companion LVDS encoder for dual-link operation, if any. */
> companion = of_parse_phandle(dev->of_node, "renesas,companion", 0);
> @@ -687,16 +704,50 @@ static int rcar_lvds_parse_dt_companion(struct rcar_lvds *lvds)
> goto done;
> }
>
> + /*
> + * We need to work out if the sink is expecting us to function in
> + * dual-link mode. We do this by looking at the DT port nodes we are
> + * connected to, if they are marked as expecting even pixels and
> + * odd pixels than we need to enable vertical stripe output
> + */
> + p0 = of_graph_get_port_by_id(dev->of_node, 1);
> + p1 = of_graph_get_port_by_id(companion, 1);
> + dual_link = drm_of_lvds_get_dual_link_configuration(p0, p1);
You can call of_node_put(p0) and of_node_put(p1) here instead of adding
them at the end of the function.
> + if (dual_link >= DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS) {
> + dev_dbg(dev, "Dual-link configuration detected\n");
> + lvds->dual_link = true;
> + } else {
> + /* dual-link mode is not required */
> + dev_dbg(dev, "Single-link configuration detected\n");
> + goto done;
> + }
Missing blank line here.
> + /*
> + * We may need to swap even and odd pixels around in case the wiring
> + * doesn't match the default configuration.
> + * By default we generate even pixels from this encoder and odd pixels
> + * from the companion encoder, but if p0 is connected to the port
> + * expecting ood pixels, and p1 is connected to the port expecting even
> + * pixels, then we need to swap even and odd pixels around
> + */
> + if (dual_link == DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS) {
> + dev_dbg(dev, "Data swapping required\n");
> + lvds->stripe_swap_data = true;
> + }
> +
> lvds->companion = of_drm_find_bridge(companion);
> if (!lvds->companion) {
> ret = -EPROBE_DEFER;
> goto done;
> }
> + companion_lvds = bridge_to_rcar_lvds(lvds->companion);
> + companion_lvds->dual_link = lvds->dual_link;
I don't like poking directly in the companion like this :-( Can't we let
the companion detect dual link mode itself ?
>
> dev_dbg(dev, "Found companion encoder %pOF\n", companion);
>
> done:
> of_node_put(companion);
> + of_node_put(p0);
> + of_node_put(p1);
>
> return ret;
> }
> @@ -704,10 +755,7 @@ static int rcar_lvds_parse_dt_companion(struct rcar_lvds *lvds)
> static int rcar_lvds_parse_dt(struct rcar_lvds *lvds)
> {
> struct device_node *local_output = NULL;
> - struct device_node *remote_input = NULL;
> struct device_node *remote = NULL;
> - struct device_node *node;
> - bool is_bridge = false;
> int ret = 0;
>
> local_output = of_graph_get_endpoint_by_regs(lvds->dev->of_node, 1, 0);
> @@ -735,45 +783,17 @@ static int rcar_lvds_parse_dt(struct rcar_lvds *lvds)
> goto done;
> }
>
I think you can also drop all the code above.
> - remote_input = of_graph_get_remote_endpoint(local_output);
> -
> - for_each_endpoint_of_node(remote, node) {
> - if (node != remote_input) {
> - /*
> - * We've found one endpoint other than the input, this
> - * must be a bridge.
> - */
> - is_bridge = true;
> - of_node_put(node);
> - break;
> - }
> - }
> -
> - if (is_bridge) {
> - lvds->next_bridge = of_drm_find_bridge(remote);
> - if (!lvds->next_bridge) {
> - ret = -EPROBE_DEFER;
> - goto done;
> - }
> -
> - if (lvds->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK)
> - lvds->dual_link = lvds->next_bridge->timings
> - ? lvds->next_bridge->timings->dual_link
> - : false;
Aren't you breaking backward compatibility with this change ? Unless I'm
mistaken you're now requiring the new DT properties, and the existing DT
that include a thc63lvd1024 won't have them.
> - } else {
> - lvds->panel = of_drm_find_panel(remote);
> - if (IS_ERR(lvds->panel)) {
> - ret = PTR_ERR(lvds->panel);
> - goto done;
> - }
> + ret = drm_of_find_panel_or_bridge(lvds->dev->of_node, 1, 0,
> + &lvds->panel, &lvds->next_bridge);
> + if (ret) {
> + ret = -EPROBE_DEFER;
Shouldn't you return ret instead of overriding it ?
> + goto done;
> }
> -
> - if (lvds->dual_link)
> + if (lvds->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK)
> ret = rcar_lvds_parse_dt_companion(lvds);
>
> done:
> of_node_put(local_output);
> - of_node_put(remote_input);
> of_node_put(remote);
>
> /*
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
