Re: [PATCH v7 09/11] drm/mediatek: dp: Add support for embedded DisplayPort aux-bus

[Nícolas F. R. A. Prado]

Hi,

On Fri, Aug 25, 2023 at 02:01:09PM +0200, Michael Walle wrote:
> Hi AngeloGioacchino,
> 
> > For the eDP case we can support using aux-bus on MediaTek DP: this
> > gives us the possibility to declare our panel as generic "panel-edp"
> > which will automatically configure the timings and available modes
> > via the EDID that we read from it.
> > 
> > To do this, move the panel parsing at the end of the probe function
> > so that the hardware is initialized beforehand and also initialize
> > the DPTX AUX block and power both on as, when we populate the
> > aux-bus, the panel driver will trigger an EDID read to perform
> > panel detection.
> > 
> > Last but not least, since now the AUX transfers can happen in the
> > separated aux-bus, it was necessary to add an exclusion for the
> > cable_plugged_in check in `mtk_dp_aux_transfer()` and the easiest
> > way to do this is to simply ignore checking that when the bridge
> > type is eDP.
> 
> This patch breaks my board based on the MT8195 which only has one
> DisplayPort output port. I suspect it might also break the mt8195-cherry
> board.

Do you mean that your board does not have an internal display, only the one
output port? If so, why are you enabling the nodes for the internal display path
in your board specific DT?

> 
> While the mediatek-dpi driver finds the DP port:
> [    3.131645] mediatek-dpi 1c113000.dp-intf: Found bridge node: /soc/dp-tx@1c600000
> 
> The probing of the eDP is deferred:
> [   13.289009] platform 1c015000.dp-intf: deferred probe pending
> 
> So I don't know why, but to make dp_intf1 work, it seems that dp_intf0
> must be probed successfully. After this patch, the edp (which is
> connected to the dp_intf1) probe will return with an -ENODEV and
> the previous call to devm_drm_bridge_add() will be rolled back.

The MediaTek DRM driver uses the component framework, so it waits for all of its
components to register until it binds them all (which includes both intf0 and
intf1, unless they're disabled on the DT).

It's true that before this patch no panel being found for edp-tx wouldn't
prevent it to probe, but it really should.

Thanks,
Nícolas

> 
> Before this patch, bridge_add() was called in any case (in the
> error case with next_bridge = NULL) and the mediatek-dpi probed
> like that:
> 
> [    3.121011] mediatek-dpi 1c015000.dp-intf: Found bridge node: /soc/edp-tx@1c500000
> [    3.122111] mediatek-dpi 1c113000.dp-intf: Found bridge node: /soc/dp-tx@1c600000
> 
> Eventually resulting in a framebuffer device:
> [    4.451081] mediatek-drm mediatek-drm.8.auto: [drm] fb0: mediatekdrmfb frame buffer device
> 
> 
> NB, somehow this series broke the initial display output. I always have
> to replug the DisplayPort to get some output. I'll dig deeper into that
> later.
> 
> ..
> 
> > @@ -2519,21 +2553,14 @@ static int mtk_dp_probe(struct platform_device *pdev)
> >  		return dev_err_probe(dev, mtk_dp->irq,
> >  				     "failed to request dp irq resource\n");
> >  
> > -	mtk_dp->next_bridge = devm_drm_of_get_bridge(dev, dev->of_node, 1, 0);
> > -	if (IS_ERR(mtk_dp->next_bridge) &&
> > -	    PTR_ERR(mtk_dp->next_bridge) == -ENODEV)
> > -		mtk_dp->next_bridge = NULL;
> 
> In my case, this branch was taken.
> 
> -michael
> 
> > -	else if (IS_ERR(mtk_dp->next_bridge))
> > -		return dev_err_probe(dev, PTR_ERR(mtk_dp->next_bridge),
> > -				     "Failed to get bridge\n");
> > -
> >  	ret = mtk_dp_dt_parse(mtk_dp, pdev);
> >  	if (ret)
> >  		return dev_err_probe(dev, ret, "Failed to parse dt\n");
> >  
> > -	drm_dp_aux_init(&mtk_dp->aux);
> >  	mtk_dp->aux.name = "aux_mtk_dp";
> > +	mtk_dp->aux.dev = dev;
> >  	mtk_dp->aux.transfer = mtk_dp_aux_transfer;
> > +	drm_dp_aux_init(&mtk_dp->aux);
> >  
> >  	spin_lock_init(&mtk_dp->irq_thread_lock);
> >