Dne ponedeljek, 25. september 2023 ob 09:57:22 CEST je Maxime Ripard napisal(a):
> Hi,
>
> On Sun, Sep 24, 2023 at 09:26:00PM +0200, Jernej Skrabec wrote:
> > Since ddc-en property handling was moved from sun8i dw-hdmi driver to
> > display connector driver, probe order of drivers determines if EDID is
> > properly read at boot time or not.
> >
> > In order to fix this, let's switch to bridge functions which allows us
> > to build proper chain and defer execution until all drivers are probed.
> >
> > Fixes: 920169041baa ("drm/sun4i: dw-hdmi: Fix ddc-en GPIO consumer conflict")
> > Signed-off-by: Jernej Skrabec <jernej.skrabec@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c | 114 +++++++++++++++++++++++++-
> > drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h | 5 ++
> > 2 files changed, 117 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
> > index 8f8d3bdba5ce..93831cdf1917 100644
> > --- a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
> > +++ b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c
> > @@ -8,14 +8,82 @@
> > #include <linux/of.h>
> > #include <linux/platform_device.h>
> >
> > +#include <drm/drm_atomic_state_helper.h>
> > +#include <drm/drm_bridge_connector.h>
> > #include <drm/drm_managed.h>
> > #include <drm/drm_modeset_helper_vtables.h>
> > #include <drm/drm_of.h>
> > #include <drm/drm_simple_kms_helper.h>
> >
> > +#include <media/cec-notifier.h>
> > +
> > #include "sun8i_dw_hdmi.h"
> > #include "sun8i_tcon_top.h"
> >
> > +#define bridge_to_sun8i_dw_hdmi(x) \
> > + container_of(x, struct sun8i_dw_hdmi, enc_bridge)
> > +
> > +static int sun8i_hdmi_enc_attach(struct drm_bridge *bridge,
> > + enum drm_bridge_attach_flags flags)
> > +{
> > + struct sun8i_dw_hdmi *hdmi = bridge_to_sun8i_dw_hdmi(bridge);
> > +
> > + return drm_bridge_attach(&hdmi->encoder, hdmi->hdmi_bridge,
> > + &hdmi->enc_bridge, flags);
> > +}
> > +
> > +static void sun8i_hdmi_enc_detach(struct drm_bridge *bridge)
> > +{
> > + struct sun8i_dw_hdmi *hdmi = bridge_to_sun8i_dw_hdmi(bridge);
> > +
> > + cec_notifier_conn_unregister(hdmi->cec_notifier);
> > + hdmi->cec_notifier = NULL;
> > +}
> > +
> > +static void sun8i_hdmi_enc_hpd_notify(struct drm_bridge *bridge,
> > + enum drm_connector_status status)
> > +{
> > + struct sun8i_dw_hdmi *hdmi = bridge_to_sun8i_dw_hdmi(bridge);
> > + struct edid *edid;
> > +
> > + if (!hdmi->cec_notifier)
> > + return;
> > +
> > + if (status == connector_status_connected) {
> > + edid = drm_bridge_get_edid(hdmi->hdmi_bridge, hdmi->connector);
> > + if (edid)
> > + cec_notifier_set_phys_addr_from_edid(hdmi->cec_notifier,
> > + edid);
> > + } else {
> > + cec_notifier_phys_addr_invalidate(hdmi->cec_notifier);
> > + }
> > +}
> > +
> > +static int sun8i_hdmi_enc_atomic_check(struct drm_bridge *bridge,
> > + struct drm_bridge_state *bridge_state,
> > + struct drm_crtc_state *crtc_state,
> > + struct drm_connector_state *conn_state)
> > +{
> > + struct drm_connector_state *old_conn_state =
> > + drm_atomic_get_old_connector_state(conn_state->state,
> > + conn_state->connector);
> > +
> > + if (!drm_connector_atomic_hdr_metadata_equal(old_conn_state, conn_state))
> > + crtc_state->mode_changed = true;
> > +
> > + return 0;
> > +}
> > +
> > +static const struct drm_bridge_funcs sun8i_hdmi_enc_bridge_funcs = {
> > + .attach = sun8i_hdmi_enc_attach,
> > + .detach = sun8i_hdmi_enc_detach,
> > + .hpd_notify = sun8i_hdmi_enc_hpd_notify,
> > + .atomic_check = sun8i_hdmi_enc_atomic_check,
> > + .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
> > + .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
> > + .atomic_reset = drm_atomic_helper_bridge_reset,
> > +};
> > +
> > static void sun8i_dw_hdmi_encoder_mode_set(struct drm_encoder *encoder,
> > struct drm_display_mode *mode,
> > struct drm_display_mode *adj_mode)
> > @@ -99,6 +167,8 @@ static int sun8i_dw_hdmi_bind(struct device *dev, struct device *master,
> > {
> > struct platform_device *pdev = to_platform_device(dev);
> > struct dw_hdmi_plat_data *plat_data;
> > + struct cec_connector_info conn_info;
> > + struct drm_connector *connector;
> > struct drm_device *drm = data;
> > struct device_node *phy_node;
> > struct drm_encoder *encoder;
> > @@ -187,18 +257,57 @@ static int sun8i_dw_hdmi_bind(struct device *dev, struct device *master,
> >
> > plat_data->mode_valid = hdmi->quirks->mode_valid;
> > plat_data->use_drm_infoframe = hdmi->quirks->use_drm_infoframe;
> > + plat_data->output_port = 1;
> > sun8i_hdmi_phy_set_ops(hdmi->phy, plat_data);
> >
> > platform_set_drvdata(pdev, hdmi);
> >
> > - hdmi->hdmi = dw_hdmi_bind(pdev, encoder, plat_data);
> > + hdmi->hdmi = dw_hdmi_probe(pdev, plat_data);
> > if (IS_ERR(hdmi->hdmi)) {
> > ret = PTR_ERR(hdmi->hdmi);
> > goto err_deinit_phy;
> > }
> >
> > + hdmi->hdmi_bridge = of_drm_find_bridge(dev->of_node);
>
> So you're also adding child bridge support? This should be mentioned in
> the commit log.
>
> > + hdmi->enc_bridge.funcs = &sun8i_hdmi_enc_bridge_funcs;
> > + hdmi->enc_bridge.type = DRM_MODE_CONNECTOR_HDMIA;
> > + hdmi->enc_bridge.interlace_allowed = true;
> > +
> > + drm_bridge_add(&hdmi->enc_bridge);
> > +
> > + ret = drm_bridge_attach(encoder, &hdmi->enc_bridge, NULL,
> > + DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> > + if (ret)
> > + goto err_remove_dw_hdmi;
> > +
> > + connector = drm_bridge_connector_init(drm, encoder);
> > + if (IS_ERR(connector)) {
> > + dev_err(dev, "Unable to create HDMI bridge connector\n");
> > + ret = PTR_ERR(connector);
> > + goto err_remove_dw_hdmi;
> > + }
> > +
> > + hdmi->connector = connector;
> > + drm_connector_attach_encoder(connector, encoder);
> > +
> > + if (hdmi->quirks->use_drm_infoframe)
> > + drm_connector_attach_hdr_output_metadata_property(connector);
> > +
> > + cec_fill_conn_info_from_drm(&conn_info, connector);
> > +
> > + hdmi->cec_notifier = cec_notifier_conn_register(&pdev->dev, NULL,
> > + &conn_info);
> > + if (!hdmi->cec_notifier) {
> > + ret = -ENOMEM;
> > + goto err_remove_dw_hdmi;
> > + }
> > +
> > return 0;
>
> Yeah, I'm not sure. We now have two different yet redundant set of
> operations with no clear definition wrt what belongs where. I'm really
> not impressed with the use of bridges for things that are clearly not
> bridges.
DRM bridges should be taken as more broad concept than just a physical
bridge.
>
> The "nothing happens until all drivers are loaded" argument is also
> supposed to be covered by the component framework, so why do we even
> have to use a bridge here?
There is more than one reason:
- Display connector driver, which sets ddc-en gpio, is purely bridge driver.
- In long term, I plan to add format negotiation between display, dw-hdmi
and DE3 (I already have WIP code). This is already implemented in bridge
infrastructure.
- There is a plan to remove connector handling from DW HDMI handling
and use display connector driver instead. This again involves bridges.
sun4i-drm and rockchip-drm are the only remaining drivers, which are
not yet converted.
>
> You were saying that it's an issue of probe order going wrong, could you
> explain a bit more what goes wrong so we can try to figure something out
> that doesn't involve a bridge?
Not sure how to make this clearer than in cover letter and this commit
message. ddc-en pin is responsibility of display-connector driver (see
commit mentioned in fixes tag), so it must probe before sun8i dw hdmi.
Why are you so against bridges? As I explained above, format negotiation
is really implemented only there, so they are needed at some point
anyway.
Best regards,
Jernej
