| Message ID | 20240623-drm-bridge-connector-fix-hdmi-reset-v2-1-8590d44912ce@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: fix two issues related to HDMI Connector implementation | expand |
On 6/22/2024 10:40 PM, Dmitry Baryshkov wrote: > On HDMI connectors which use drm_bridge_connector and DRM_BRIDGE_OP_HDMI > IGT chokes on the max_bpc property in several kms_properties tests due > to the the drm_bridge_connector failing to reset HDMI-related > properties. > > Call __drm_atomic_helper_connector_hdmi_reset() if there is a > the drm_bridge_connector has bridge_hdmi. > > Note, the __drm_atomic_helper_connector_hdmi_reset() is moved to > drm_atomic_state_helper.c because drm_bridge_connector.c can not depend > on DRM_DISPLAY_HDMI_STATE_HELPER. At the same time it is impossible to > call this function from HDMI bridges, there is is no function that > corresponds to the drm_connector_funcs::reset(). > > Fixes: 6b4468b0c6ba ("drm/bridge-connector: implement glue code for HDMI connector") > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > drivers/gpu/drm/display/drm_hdmi_state_helper.c | 21 --------------------- > drivers/gpu/drm/drm_atomic_state_helper.c | 21 +++++++++++++++++++++ > drivers/gpu/drm/drm_bridge_connector.c | 13 ++++++++++++- > include/drm/display/drm_hdmi_state_helper.h | 3 --- > include/drm/drm_atomic_state_helper.h | 2 ++ > 5 files changed, 35 insertions(+), 25 deletions(-) > > diff --git a/drivers/gpu/drm/display/drm_hdmi_state_helper.c b/drivers/gpu/drm/display/drm_hdmi_state_helper.c > index 2dab3ad8ce64..67f39857b0b4 100644 > --- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c > +++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c > @@ -8,27 +8,6 @@ > #include <drm/display/drm_hdmi_helper.h> > #include <drm/display/drm_hdmi_state_helper.h> > > -/** > - * __drm_atomic_helper_connector_hdmi_reset() - Initializes all HDMI @drm_connector_state resources > - * @connector: DRM connector > - * @new_conn_state: connector state to reset > - * > - * Initializes all HDMI resources from a @drm_connector_state without > - * actually allocating it. This is useful for HDMI drivers, in > - * combination with __drm_atomic_helper_connector_reset() or > - * drm_atomic_helper_connector_reset(). > - */ > -void __drm_atomic_helper_connector_hdmi_reset(struct drm_connector *connector, > - struct drm_connector_state *new_conn_state) > -{ > - unsigned int max_bpc = connector->max_bpc; > - > - new_conn_state->max_bpc = max_bpc; > - new_conn_state->max_requested_bpc = max_bpc; > - new_conn_state->hdmi.broadcast_rgb = DRM_HDMI_BROADCAST_RGB_AUTO; > -} > -EXPORT_SYMBOL(__drm_atomic_helper_connector_hdmi_reset); > - > static const struct drm_display_mode * > connector_state_get_mode(const struct drm_connector_state *conn_state) > { > diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c > index 519228eb1095..1518ada81b45 100644 > --- a/drivers/gpu/drm/drm_atomic_state_helper.c > +++ b/drivers/gpu/drm/drm_atomic_state_helper.c > @@ -478,6 +478,27 @@ void drm_atomic_helper_connector_reset(struct drm_connector *connector) > } > EXPORT_SYMBOL(drm_atomic_helper_connector_reset); > > +/** > + * __drm_atomic_helper_connector_hdmi_reset() - Initializes all HDMI @drm_connector_state resources > + * @connector: DRM connector > + * @new_conn_state: connector state to reset > + * > + * Initializes all HDMI resources from a @drm_connector_state without > + * actually allocating it. This is useful for HDMI drivers, in > + * combination with __drm_atomic_helper_connector_reset() or > + * drm_atomic_helper_connector_reset(). > + */ > +void __drm_atomic_helper_connector_hdmi_reset(struct drm_connector *connector, > + struct drm_connector_state *new_conn_state) > +{ > + unsigned int max_bpc = connector->max_bpc; > + > + new_conn_state->max_bpc = max_bpc; > + new_conn_state->max_requested_bpc = max_bpc; I understand this is just code propagation but do we need a max_bpc local variable? We can just do new_conn_state->max_bpc = connector->max_bpc; new_conn_state->max_requested_bpc = connector->max_bpc; But apart from that nit, this LGTM. Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
On Tue, 25 Jun 2024 at 01:28, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > > > On 6/22/2024 10:40 PM, Dmitry Baryshkov wrote: > > On HDMI connectors which use drm_bridge_connector and DRM_BRIDGE_OP_HDMI > > IGT chokes on the max_bpc property in several kms_properties tests due > > to the the drm_bridge_connector failing to reset HDMI-related > > properties. > > > > Call __drm_atomic_helper_connector_hdmi_reset() if there is a > > the drm_bridge_connector has bridge_hdmi. > > > > Note, the __drm_atomic_helper_connector_hdmi_reset() is moved to > > drm_atomic_state_helper.c because drm_bridge_connector.c can not depend > > on DRM_DISPLAY_HDMI_STATE_HELPER. At the same time it is impossible to > > call this function from HDMI bridges, there is is no function that > > corresponds to the drm_connector_funcs::reset(). > > > > Fixes: 6b4468b0c6ba ("drm/bridge-connector: implement glue code for HDMI connector") > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > --- > > drivers/gpu/drm/display/drm_hdmi_state_helper.c | 21 --------------------- > > drivers/gpu/drm/drm_atomic_state_helper.c | 21 +++++++++++++++++++++ > > drivers/gpu/drm/drm_bridge_connector.c | 13 ++++++++++++- > > include/drm/display/drm_hdmi_state_helper.h | 3 --- > > include/drm/drm_atomic_state_helper.h | 2 ++ > > 5 files changed, 35 insertions(+), 25 deletions(-) > > > > diff --git a/drivers/gpu/drm/display/drm_hdmi_state_helper.c b/drivers/gpu/drm/display/drm_hdmi_state_helper.c > > index 2dab3ad8ce64..67f39857b0b4 100644 > > --- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c > > +++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c > > @@ -8,27 +8,6 @@ > > #include <drm/display/drm_hdmi_helper.h> > > #include <drm/display/drm_hdmi_state_helper.h> > > > > -/** > > - * __drm_atomic_helper_connector_hdmi_reset() - Initializes all HDMI @drm_connector_state resources > > - * @connector: DRM connector > > - * @new_conn_state: connector state to reset > > - * > > - * Initializes all HDMI resources from a @drm_connector_state without > > - * actually allocating it. This is useful for HDMI drivers, in > > - * combination with __drm_atomic_helper_connector_reset() or > > - * drm_atomic_helper_connector_reset(). > > - */ > > -void __drm_atomic_helper_connector_hdmi_reset(struct drm_connector *connector, > > - struct drm_connector_state *new_conn_state) > > -{ > > - unsigned int max_bpc = connector->max_bpc; > > - > > - new_conn_state->max_bpc = max_bpc; > > - new_conn_state->max_requested_bpc = max_bpc; > > - new_conn_state->hdmi.broadcast_rgb = DRM_HDMI_BROADCAST_RGB_AUTO; > > -} > > -EXPORT_SYMBOL(__drm_atomic_helper_connector_hdmi_reset); > > - > > static const struct drm_display_mode * > > connector_state_get_mode(const struct drm_connector_state *conn_state) > > { > > diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c > > index 519228eb1095..1518ada81b45 100644 > > --- a/drivers/gpu/drm/drm_atomic_state_helper.c > > +++ b/drivers/gpu/drm/drm_atomic_state_helper.c > > @@ -478,6 +478,27 @@ void drm_atomic_helper_connector_reset(struct drm_connector *connector) > > } > > EXPORT_SYMBOL(drm_atomic_helper_connector_reset); > > > > +/** > > + * __drm_atomic_helper_connector_hdmi_reset() - Initializes all HDMI @drm_connector_state resources > > + * @connector: DRM connector > > + * @new_conn_state: connector state to reset > > + * > > + * Initializes all HDMI resources from a @drm_connector_state without > > + * actually allocating it. This is useful for HDMI drivers, in > > + * combination with __drm_atomic_helper_connector_reset() or > > + * drm_atomic_helper_connector_reset(). > > + */ > > +void __drm_atomic_helper_connector_hdmi_reset(struct drm_connector *connector, > > + struct drm_connector_state *new_conn_state) > > +{ > > + unsigned int max_bpc = connector->max_bpc; > > + > > + new_conn_state->max_bpc = max_bpc; > > + new_conn_state->max_requested_bpc = max_bpc; > > I understand this is just code propagation but do we need a max_bpc > local variable? > > We can just do > > new_conn_state->max_bpc = connector->max_bpc; > new_conn_state->max_requested_bpc = connector->max_bpc; Possible implementation, but I was really just doing code move. > > But apart from that nit, this LGTM. > > Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
Hi, On Sun, Jun 23, 2024 at 08:40:12AM GMT, Dmitry Baryshkov wrote: > On HDMI connectors which use drm_bridge_connector and DRM_BRIDGE_OP_HDMI > IGT chokes on the max_bpc property in several kms_properties tests due > to the the drm_bridge_connector failing to reset HDMI-related > properties. > > Call __drm_atomic_helper_connector_hdmi_reset() if there is a > the drm_bridge_connector has bridge_hdmi. > > Note, the __drm_atomic_helper_connector_hdmi_reset() is moved to > drm_atomic_state_helper.c because drm_bridge_connector.c can not depend > on DRM_DISPLAY_HDMI_STATE_HELPER. At the same time it is impossible to > call this function from HDMI bridges, there is is no function that > corresponds to the drm_connector_funcs::reset(). Why can't it depend on DRM_DISPLAY_HDMI_STATE_HELPER? Maxime
On Tue, 25 Jun 2024 at 18:02, Maxime Ripard <mripard@kernel.org> wrote: > > Hi, > > On Sun, Jun 23, 2024 at 08:40:12AM GMT, Dmitry Baryshkov wrote: > > On HDMI connectors which use drm_bridge_connector and DRM_BRIDGE_OP_HDMI > > IGT chokes on the max_bpc property in several kms_properties tests due > > to the the drm_bridge_connector failing to reset HDMI-related > > properties. > > > > Call __drm_atomic_helper_connector_hdmi_reset() if there is a > > the drm_bridge_connector has bridge_hdmi. > > > > Note, the __drm_atomic_helper_connector_hdmi_reset() is moved to > > drm_atomic_state_helper.c because drm_bridge_connector.c can not depend > > on DRM_DISPLAY_HDMI_STATE_HELPER. At the same time it is impossible to > > call this function from HDMI bridges, there is is no function that > > corresponds to the drm_connector_funcs::reset(). > > Why can't it depend on DRM_DISPLAY_HDMI_STATE_HELPER? Is it okay to have DRM_KMS_HELPER to depend unconditionally or select DRM_DISPLAY_HDMI_STATE_HELPER?
On Tue, Jun 25, 2024 at 06:05:33PM GMT, Dmitry Baryshkov wrote: > On Tue, 25 Jun 2024 at 18:02, Maxime Ripard <mripard@kernel.org> wrote: > > > > Hi, > > > > On Sun, Jun 23, 2024 at 08:40:12AM GMT, Dmitry Baryshkov wrote: > > > On HDMI connectors which use drm_bridge_connector and DRM_BRIDGE_OP_HDMI > > > IGT chokes on the max_bpc property in several kms_properties tests due > > > to the the drm_bridge_connector failing to reset HDMI-related > > > properties. > > > > > > Call __drm_atomic_helper_connector_hdmi_reset() if there is a > > > the drm_bridge_connector has bridge_hdmi. > > > > > > Note, the __drm_atomic_helper_connector_hdmi_reset() is moved to > > > drm_atomic_state_helper.c because drm_bridge_connector.c can not depend > > > on DRM_DISPLAY_HDMI_STATE_HELPER. At the same time it is impossible to > > > call this function from HDMI bridges, there is is no function that > > > corresponds to the drm_connector_funcs::reset(). > > > > Why can't it depend on DRM_DISPLAY_HDMI_STATE_HELPER? > > Is it okay to have DRM_KMS_HELPER to depend unconditionally or select > DRM_DISPLAY_HDMI_STATE_HELPER? No, but it's not clear to me why drm_bridge_connector is part of DRM_KMS_HELPER? It doesn't seem to be called from the core but only drivers, just like DRM_PANEL_BRIDGE which has a separate config option Maxime
On Tue, Jun 25, 2024 at 05:49:54PM GMT, Maxime Ripard wrote: > On Tue, Jun 25, 2024 at 06:05:33PM GMT, Dmitry Baryshkov wrote: > > On Tue, 25 Jun 2024 at 18:02, Maxime Ripard <mripard@kernel.org> wrote: > > > > > > Hi, > > > > > > On Sun, Jun 23, 2024 at 08:40:12AM GMT, Dmitry Baryshkov wrote: > > > > On HDMI connectors which use drm_bridge_connector and DRM_BRIDGE_OP_HDMI > > > > IGT chokes on the max_bpc property in several kms_properties tests due > > > > to the the drm_bridge_connector failing to reset HDMI-related > > > > properties. > > > > > > > > Call __drm_atomic_helper_connector_hdmi_reset() if there is a > > > > the drm_bridge_connector has bridge_hdmi. > > > > > > > > Note, the __drm_atomic_helper_connector_hdmi_reset() is moved to > > > > drm_atomic_state_helper.c because drm_bridge_connector.c can not depend > > > > on DRM_DISPLAY_HDMI_STATE_HELPER. At the same time it is impossible to > > > > call this function from HDMI bridges, there is is no function that > > > > corresponds to the drm_connector_funcs::reset(). > > > > > > Why can't it depend on DRM_DISPLAY_HDMI_STATE_HELPER? > > > > Is it okay to have DRM_KMS_HELPER to depend unconditionally or select > > DRM_DISPLAY_HDMI_STATE_HELPER? > > No, but it's not clear to me why drm_bridge_connector is part of > DRM_KMS_HELPER? It doesn't seem to be called from the core but only > drivers, just like DRM_PANEL_BRIDGE which has a separate config option But then DRM_PANEL_BRIDGE is also linked into drm_kms_helper.ko. I can't really say that I understand the definition of various helper modules, but my taste tells me that code from drm/*.c should not depend directly on the code from drm/display/*.c.
On Tue, Jun 25, 2024 at 07:38:22PM GMT, Dmitry Baryshkov wrote: > On Tue, Jun 25, 2024 at 05:49:54PM GMT, Maxime Ripard wrote: > > On Tue, Jun 25, 2024 at 06:05:33PM GMT, Dmitry Baryshkov wrote: > > > On Tue, 25 Jun 2024 at 18:02, Maxime Ripard <mripard@kernel.org> wrote: > > > > > > > > Hi, > > > > > > > > On Sun, Jun 23, 2024 at 08:40:12AM GMT, Dmitry Baryshkov wrote: > > > > > On HDMI connectors which use drm_bridge_connector and DRM_BRIDGE_OP_HDMI > > > > > IGT chokes on the max_bpc property in several kms_properties tests due > > > > > to the the drm_bridge_connector failing to reset HDMI-related > > > > > properties. > > > > > > > > > > Call __drm_atomic_helper_connector_hdmi_reset() if there is a > > > > > the drm_bridge_connector has bridge_hdmi. > > > > > > > > > > Note, the __drm_atomic_helper_connector_hdmi_reset() is moved to > > > > > drm_atomic_state_helper.c because drm_bridge_connector.c can not depend > > > > > on DRM_DISPLAY_HDMI_STATE_HELPER. At the same time it is impossible to > > > > > call this function from HDMI bridges, there is is no function that > > > > > corresponds to the drm_connector_funcs::reset(). > > > > > > > > Why can't it depend on DRM_DISPLAY_HDMI_STATE_HELPER? > > > > > > Is it okay to have DRM_KMS_HELPER to depend unconditionally or select > > > DRM_DISPLAY_HDMI_STATE_HELPER? > > > > No, but it's not clear to me why drm_bridge_connector is part of > > DRM_KMS_HELPER? It doesn't seem to be called from the core but only > > drivers, just like DRM_PANEL_BRIDGE which has a separate config option > > But then DRM_PANEL_BRIDGE is also linked into drm_kms_helper.ko. And we don't seem to have any reason for that, it could be a separate module afaik. > I can't really say that I understand the definition of various helper > modules, but my taste tells me that code from drm/*.c should not > depend directly on the code from drm/display/*.c. Sure, but the bridge connector could be under bridge just as well, or part of the display helpers. It's really a "leaf" driver as far as helpers are concerned. Maxime
diff --git a/drivers/gpu/drm/display/drm_hdmi_state_helper.c b/drivers/gpu/drm/display/drm_hdmi_state_helper.c index 2dab3ad8ce64..67f39857b0b4 100644 --- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c +++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c @@ -8,27 +8,6 @@ #include <drm/display/drm_hdmi_helper.h> #include <drm/display/drm_hdmi_state_helper.h> -/** - * __drm_atomic_helper_connector_hdmi_reset() - Initializes all HDMI @drm_connector_state resources - * @connector: DRM connector - * @new_conn_state: connector state to reset - * - * Initializes all HDMI resources from a @drm_connector_state without - * actually allocating it. This is useful for HDMI drivers, in - * combination with __drm_atomic_helper_connector_reset() or - * drm_atomic_helper_connector_reset(). - */ -void __drm_atomic_helper_connector_hdmi_reset(struct drm_connector *connector, - struct drm_connector_state *new_conn_state) -{ - unsigned int max_bpc = connector->max_bpc; - - new_conn_state->max_bpc = max_bpc; - new_conn_state->max_requested_bpc = max_bpc; - new_conn_state->hdmi.broadcast_rgb = DRM_HDMI_BROADCAST_RGB_AUTO; -} -EXPORT_SYMBOL(__drm_atomic_helper_connector_hdmi_reset); - static const struct drm_display_mode * connector_state_get_mode(const struct drm_connector_state *conn_state) { diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c index 519228eb1095..1518ada81b45 100644 --- a/drivers/gpu/drm/drm_atomic_state_helper.c +++ b/drivers/gpu/drm/drm_atomic_state_helper.c @@ -478,6 +478,27 @@ void drm_atomic_helper_connector_reset(struct drm_connector *connector) } EXPORT_SYMBOL(drm_atomic_helper_connector_reset); +/** + * __drm_atomic_helper_connector_hdmi_reset() - Initializes all HDMI @drm_connector_state resources + * @connector: DRM connector + * @new_conn_state: connector state to reset + * + * Initializes all HDMI resources from a @drm_connector_state without + * actually allocating it. This is useful for HDMI drivers, in + * combination with __drm_atomic_helper_connector_reset() or + * drm_atomic_helper_connector_reset(). + */ +void __drm_atomic_helper_connector_hdmi_reset(struct drm_connector *connector, + struct drm_connector_state *new_conn_state) +{ + unsigned int max_bpc = connector->max_bpc; + + new_conn_state->max_bpc = max_bpc; + new_conn_state->max_requested_bpc = max_bpc; + new_conn_state->hdmi.broadcast_rgb = DRM_HDMI_BROADCAST_RGB_AUTO; +} +EXPORT_SYMBOL(__drm_atomic_helper_connector_hdmi_reset); + /** * drm_atomic_helper_connector_tv_margins_reset - Resets TV connector properties * @connector: DRM connector diff --git a/drivers/gpu/drm/drm_bridge_connector.c b/drivers/gpu/drm/drm_bridge_connector.c index 0869b663f17e..7ebb35438459 100644 --- a/drivers/gpu/drm/drm_bridge_connector.c +++ b/drivers/gpu/drm/drm_bridge_connector.c @@ -216,8 +216,19 @@ static void drm_bridge_connector_debugfs_init(struct drm_connector *connector, } } +static void drm_bridge_connector_reset(struct drm_connector *connector) +{ + struct drm_bridge_connector *bridge_connector = + to_drm_bridge_connector(connector); + + drm_atomic_helper_connector_reset(connector); + if (bridge_connector->bridge_hdmi) + __drm_atomic_helper_connector_hdmi_reset(connector, + connector->state); +} + static const struct drm_connector_funcs drm_bridge_connector_funcs = { - .reset = drm_atomic_helper_connector_reset, + .reset = drm_bridge_connector_reset, .detect = drm_bridge_connector_detect, .fill_modes = drm_helper_probe_single_connector_modes, .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, diff --git a/include/drm/display/drm_hdmi_state_helper.h b/include/drm/display/drm_hdmi_state_helper.h index 285f366cf716..454319b40f17 100644 --- a/include/drm/display/drm_hdmi_state_helper.h +++ b/include/drm/display/drm_hdmi_state_helper.h @@ -8,9 +8,6 @@ struct drm_connector; struct drm_connector_state; struct hdmi_audio_infoframe; -void __drm_atomic_helper_connector_hdmi_reset(struct drm_connector *connector, - struct drm_connector_state *new_conn_state); - int drm_atomic_helper_connector_hdmi_check(struct drm_connector *connector, struct drm_atomic_state *state); diff --git a/include/drm/drm_atomic_state_helper.h b/include/drm/drm_atomic_state_helper.h index b9740edb2658..adf300e6b4eb 100644 --- a/include/drm/drm_atomic_state_helper.h +++ b/include/drm/drm_atomic_state_helper.h @@ -71,6 +71,8 @@ void __drm_atomic_helper_connector_state_reset(struct drm_connector_state *conn_ void __drm_atomic_helper_connector_reset(struct drm_connector *connector, struct drm_connector_state *conn_state); void drm_atomic_helper_connector_reset(struct drm_connector *connector); +void __drm_atomic_helper_connector_hdmi_reset(struct drm_connector *connector, + struct drm_connector_state *new_conn_state); void drm_atomic_helper_connector_tv_reset(struct drm_connector *connector); int drm_atomic_helper_connector_tv_check(struct drm_connector *connector, struct drm_atomic_state *state);
On HDMI connectors which use drm_bridge_connector and DRM_BRIDGE_OP_HDMI IGT chokes on the max_bpc property in several kms_properties tests due to the the drm_bridge_connector failing to reset HDMI-related properties. Call __drm_atomic_helper_connector_hdmi_reset() if there is a the drm_bridge_connector has bridge_hdmi. Note, the __drm_atomic_helper_connector_hdmi_reset() is moved to drm_atomic_state_helper.c because drm_bridge_connector.c can not depend on DRM_DISPLAY_HDMI_STATE_HELPER. At the same time it is impossible to call this function from HDMI bridges, there is is no function that corresponds to the drm_connector_funcs::reset(). Fixes: 6b4468b0c6ba ("drm/bridge-connector: implement glue code for HDMI connector") Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> --- drivers/gpu/drm/display/drm_hdmi_state_helper.c | 21 --------------------- drivers/gpu/drm/drm_atomic_state_helper.c | 21 +++++++++++++++++++++ drivers/gpu/drm/drm_bridge_connector.c | 13 ++++++++++++- include/drm/display/drm_hdmi_state_helper.h | 3 --- include/drm/drm_atomic_state_helper.h | 2 ++ 5 files changed, 35 insertions(+), 25 deletions(-)