| Message ID | 20241206-drm-bridge-hdmi-connector-v6-9-50dc145a9c06@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: add DRM HDMI Codec framework | expand |
Hi, On Fri, Dec 06, 2024 at 12:16:03PM +0200, Dmitry Baryshkov wrote: > The vc4_hdmi_connector_detect_ctx() via vc4_hdmi_handle_hotplug() > already reads EDID and propagates it to the drm_connector. Stop > rereading EDID as a part of the .get_modes() callback and just update > the list of modes. This matches the behaviour of the i915 driver. > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > drivers/gpu/drm/vc4/vc4_hdmi.c | 21 --------------------- > 1 file changed, 21 deletions(-) > > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c > index e5ab42f72f618b90f956482db6c9c8074c1e3bf1..3364ef90968dad3074800f02926300ffceb75c69 100644 > --- a/drivers/gpu/drm/vc4/vc4_hdmi.c > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c > @@ -470,31 +470,10 @@ static int vc4_hdmi_connector_detect_ctx(struct drm_connector *connector, > > static int vc4_hdmi_connector_get_modes(struct drm_connector *connector) > { > - struct vc4_hdmi *vc4_hdmi = connector_to_vc4_hdmi(connector); > struct vc4_dev *vc4 = to_vc4_dev(connector->dev); > - const struct drm_edid *drm_edid; > int ret = 0; > > - /* > - * NOTE: This function should really take vc4_hdmi->mutex, but doing so > - * results in reentrancy issues since cec_s_phys_addr() might call > - * .adap_enable, which leads to that funtion being called with our mutex > - * held. > - * > - * Concurrency isn't an issue at the moment since we don't share > - * any state with any of the other frameworks so we can ignore > - * the lock for now. > - */ > - > - drm_edid = drm_edid_read_ddc(connector, vc4_hdmi->ddc); > - drm_edid_connector_update(connector, drm_edid); > - cec_s_phys_addr(vc4_hdmi->cec_adap, > - connector->display_info.source_physical_address, false); > - if (!drm_edid) > - return 0; > - > ret = drm_edid_connector_add_modes(connector); > - drm_edid_free(drm_edid); I don't think that works though, for mostly two reasons: 1) We're not sure we'll even have an hotplug interrupt if the system boots with the connector plugged in for example. 2) afaik, the get_modes ioctl directly calls get_modes too. Maxime
On Fri, Dec 06, 2024 at 03:34:52PM +0100, Maxime Ripard wrote: > Hi, > > On Fri, Dec 06, 2024 at 12:16:03PM +0200, Dmitry Baryshkov wrote: > > The vc4_hdmi_connector_detect_ctx() via vc4_hdmi_handle_hotplug() > > already reads EDID and propagates it to the drm_connector. Stop > > rereading EDID as a part of the .get_modes() callback and just update > > the list of modes. This matches the behaviour of the i915 driver. > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > --- > > drivers/gpu/drm/vc4/vc4_hdmi.c | 21 --------------------- > > 1 file changed, 21 deletions(-) > > > > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c > > index e5ab42f72f618b90f956482db6c9c8074c1e3bf1..3364ef90968dad3074800f02926300ffceb75c69 100644 > > --- a/drivers/gpu/drm/vc4/vc4_hdmi.c > > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c > > @@ -470,31 +470,10 @@ static int vc4_hdmi_connector_detect_ctx(struct drm_connector *connector, > > > > static int vc4_hdmi_connector_get_modes(struct drm_connector *connector) > > { > > - struct vc4_hdmi *vc4_hdmi = connector_to_vc4_hdmi(connector); > > struct vc4_dev *vc4 = to_vc4_dev(connector->dev); > > - const struct drm_edid *drm_edid; > > int ret = 0; > > > > - /* > > - * NOTE: This function should really take vc4_hdmi->mutex, but doing so > > - * results in reentrancy issues since cec_s_phys_addr() might call > > - * .adap_enable, which leads to that funtion being called with our mutex > > - * held. > > - * > > - * Concurrency isn't an issue at the moment since we don't share > > - * any state with any of the other frameworks so we can ignore > > - * the lock for now. > > - */ > > - > > - drm_edid = drm_edid_read_ddc(connector, vc4_hdmi->ddc); > > - drm_edid_connector_update(connector, drm_edid); > > - cec_s_phys_addr(vc4_hdmi->cec_adap, > > - connector->display_info.source_physical_address, false); > > - if (!drm_edid) > > - return 0; > > - > > ret = drm_edid_connector_add_modes(connector); > > - drm_edid_free(drm_edid); > > I don't think that works though, for mostly two reasons: > > 1) We're not sure we'll even have an hotplug interrupt if the system > boots with the connector plugged in for example. > > 2) afaik, the get_modes ioctl directly calls get_modes too. I think both paths use .fill_modes, not get_modes. And fill_modes explicitly calls .detect / .detect_ctx. It would be nice if somebody can verify the change on the acual hw. But as I wrote, i915 driver does exactly the same: the EDID is read / updated in .detect_ctx(), while .get_modes() only performs drm_edid_connector_add_modes(). Okay, there is one significant difference which we should probably patch out: the .force() is also implemented to update EDID in the forced case. I should probably do the same for the drivers being handled in this patchset.
On Sun, Dec 08, 2024 at 01:06:46PM +0200, Dmitry Baryshkov wrote: > On Fri, Dec 06, 2024 at 03:34:52PM +0100, Maxime Ripard wrote: > > Hi, > > > > On Fri, Dec 06, 2024 at 12:16:03PM +0200, Dmitry Baryshkov wrote: > > > The vc4_hdmi_connector_detect_ctx() via vc4_hdmi_handle_hotplug() > > > already reads EDID and propagates it to the drm_connector. Stop > > > rereading EDID as a part of the .get_modes() callback and just update > > > the list of modes. This matches the behaviour of the i915 driver. > > > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > > --- > > > drivers/gpu/drm/vc4/vc4_hdmi.c | 21 --------------------- > > > 1 file changed, 21 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c > > > index e5ab42f72f618b90f956482db6c9c8074c1e3bf1..3364ef90968dad3074800f02926300ffceb75c69 100644 > > > --- a/drivers/gpu/drm/vc4/vc4_hdmi.c > > > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c > > > @@ -470,31 +470,10 @@ static int vc4_hdmi_connector_detect_ctx(struct drm_connector *connector, > > > > > > static int vc4_hdmi_connector_get_modes(struct drm_connector *connector) > > > { > > > - struct vc4_hdmi *vc4_hdmi = connector_to_vc4_hdmi(connector); > > > struct vc4_dev *vc4 = to_vc4_dev(connector->dev); > > > - const struct drm_edid *drm_edid; > > > int ret = 0; > > > > > > - /* > > > - * NOTE: This function should really take vc4_hdmi->mutex, but doing so > > > - * results in reentrancy issues since cec_s_phys_addr() might call > > > - * .adap_enable, which leads to that funtion being called with our mutex > > > - * held. > > > - * > > > - * Concurrency isn't an issue at the moment since we don't share > > > - * any state with any of the other frameworks so we can ignore > > > - * the lock for now. > > > - */ > > > - > > > - drm_edid = drm_edid_read_ddc(connector, vc4_hdmi->ddc); > > > - drm_edid_connector_update(connector, drm_edid); > > > - cec_s_phys_addr(vc4_hdmi->cec_adap, > > > - connector->display_info.source_physical_address, false); > > > - if (!drm_edid) > > > - return 0; > > > - > > > ret = drm_edid_connector_add_modes(connector); > > > - drm_edid_free(drm_edid); > > > > I don't think that works though, for mostly two reasons: > > > > 1) We're not sure we'll even have an hotplug interrupt if the system > > boots with the connector plugged in for example. > > > > 2) afaik, the get_modes ioctl directly calls get_modes too. > > I think both paths use .fill_modes, not get_modes. And fill_modes > explicitly calls .detect / .detect_ctx. It would be nice if somebody can > verify the change on the acual hw. But as I wrote, i915 driver does > exactly the same: the EDID is read / updated in .detect_ctx(), while > .get_modes() only performs drm_edid_connector_add_modes(). > Okay, there is one significant difference which we should probably patch > out: the .force() is also implemented to update EDID in the forced case. > I should probably do the same for the drivers being handled in this > patchset. Maxime, will .force() cover your concerns? I probably fail to see a path when .get_mode() is called without calling .detect() / .force() first.
On Sun, Dec 08, 2024 at 01:06:46PM +0200, Dmitry Baryshkov wrote: > On Fri, Dec 06, 2024 at 03:34:52PM +0100, Maxime Ripard wrote: > > Hi, > > > > On Fri, Dec 06, 2024 at 12:16:03PM +0200, Dmitry Baryshkov wrote: > > > The vc4_hdmi_connector_detect_ctx() via vc4_hdmi_handle_hotplug() > > > already reads EDID and propagates it to the drm_connector. Stop > > > rereading EDID as a part of the .get_modes() callback and just update > > > the list of modes. This matches the behaviour of the i915 driver. > > > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > > --- > > > drivers/gpu/drm/vc4/vc4_hdmi.c | 21 --------------------- > > > 1 file changed, 21 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c > > > index e5ab42f72f618b90f956482db6c9c8074c1e3bf1..3364ef90968dad3074800f02926300ffceb75c69 100644 > > > --- a/drivers/gpu/drm/vc4/vc4_hdmi.c > > > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c > > > @@ -470,31 +470,10 @@ static int vc4_hdmi_connector_detect_ctx(struct drm_connector *connector, > > > > > > static int vc4_hdmi_connector_get_modes(struct drm_connector *connector) > > > { > > > - struct vc4_hdmi *vc4_hdmi = connector_to_vc4_hdmi(connector); > > > struct vc4_dev *vc4 = to_vc4_dev(connector->dev); > > > - const struct drm_edid *drm_edid; > > > int ret = 0; > > > > > > - /* > > > - * NOTE: This function should really take vc4_hdmi->mutex, but doing so > > > - * results in reentrancy issues since cec_s_phys_addr() might call > > > - * .adap_enable, which leads to that funtion being called with our mutex > > > - * held. > > > - * > > > - * Concurrency isn't an issue at the moment since we don't share > > > - * any state with any of the other frameworks so we can ignore > > > - * the lock for now. > > > - */ > > > - > > > - drm_edid = drm_edid_read_ddc(connector, vc4_hdmi->ddc); > > > - drm_edid_connector_update(connector, drm_edid); > > > - cec_s_phys_addr(vc4_hdmi->cec_adap, > > > - connector->display_info.source_physical_address, false); > > > - if (!drm_edid) > > > - return 0; > > > - > > > ret = drm_edid_connector_add_modes(connector); > > > - drm_edid_free(drm_edid); > > > > I don't think that works though, for mostly two reasons: > > > > 1) We're not sure we'll even have an hotplug interrupt if the system > > boots with the connector plugged in for example. > > > > 2) afaik, the get_modes ioctl directly calls get_modes too. > > I think both paths use .fill_modes, not get_modes. The standard fill_modes helper calls get_modes. > And fill_modes explicitly calls .detect / .detect_ctx. It would be > nice if somebody can verify the change on the acual hw. But as I > wrote, i915 driver does exactly the same: the EDID is read / updated > in .detect_ctx(), while .get_modes() only performs > drm_edid_connector_add_modes(). Okay, there is one significant > difference which we should probably patch out: the .force() is also > implemented to update EDID in the forced case. I should probably do > the same for the drivers being handled in this patchset. Ack. Worst case scenario we can fix it later on. Maxime
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index e5ab42f72f618b90f956482db6c9c8074c1e3bf1..3364ef90968dad3074800f02926300ffceb75c69 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -470,31 +470,10 @@ static int vc4_hdmi_connector_detect_ctx(struct drm_connector *connector, static int vc4_hdmi_connector_get_modes(struct drm_connector *connector) { - struct vc4_hdmi *vc4_hdmi = connector_to_vc4_hdmi(connector); struct vc4_dev *vc4 = to_vc4_dev(connector->dev); - const struct drm_edid *drm_edid; int ret = 0; - /* - * NOTE: This function should really take vc4_hdmi->mutex, but doing so - * results in reentrancy issues since cec_s_phys_addr() might call - * .adap_enable, which leads to that funtion being called with our mutex - * held. - * - * Concurrency isn't an issue at the moment since we don't share - * any state with any of the other frameworks so we can ignore - * the lock for now. - */ - - drm_edid = drm_edid_read_ddc(connector, vc4_hdmi->ddc); - drm_edid_connector_update(connector, drm_edid); - cec_s_phys_addr(vc4_hdmi->cec_adap, - connector->display_info.source_physical_address, false); - if (!drm_edid) - return 0; - ret = drm_edid_connector_add_modes(connector); - drm_edid_free(drm_edid); if (!vc4->hvs->vc5_hdmi_enable_hdmi_20) { struct drm_device *drm = connector->dev;
The vc4_hdmi_connector_detect_ctx() via vc4_hdmi_handle_hotplug() already reads EDID and propagates it to the drm_connector. Stop rereading EDID as a part of the .get_modes() callback and just update the list of modes. This matches the behaviour of the i915 driver. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> --- drivers/gpu/drm/vc4/vc4_hdmi.c | 21 --------------------- 1 file changed, 21 deletions(-)