| Message ID | 20230729004913.215872-3-dmitry.baryshkov@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/bridge-connector: simplify handling of USB-C DP | expand |
On 29/07/2023 02:49, Dmitry Baryshkov wrote: > If the created connector type supports subconnector type property, > create and attach corresponding it. The default subtype value is 0, > which maps to the DRM_MODE_SUBCONNECTOR_Unknown type. > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > drivers/gpu/drm/drm_bridge_connector.c | 33 +++++++++++++++++++++++++- > include/drm/drm_bridge.h | 4 ++++ > 2 files changed, 36 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_bridge_connector.c b/drivers/gpu/drm/drm_bridge_connector.c > index 07b5930b1282..a7b92f0d2430 100644 > --- a/drivers/gpu/drm/drm_bridge_connector.c > +++ b/drivers/gpu/drm/drm_bridge_connector.c > @@ -329,7 +329,9 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm, > struct drm_connector *connector; > struct i2c_adapter *ddc = NULL; > struct drm_bridge *bridge, *panel_bridge = NULL; > + enum drm_mode_subconnector subconnector; > int connector_type; > + int ret; > > bridge_connector = kzalloc(sizeof(*bridge_connector), GFP_KERNEL); > if (!bridge_connector) > @@ -365,8 +367,10 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm, > if (bridge->ops & DRM_BRIDGE_OP_MODES) > bridge_connector->bridge_modes = bridge; > > - if (!drm_bridge_get_next_bridge(bridge)) > + if (!drm_bridge_get_next_bridge(bridge)) { > connector_type = bridge->type; > + subconnector = bridge->subtype; > + } > > #ifdef CONFIG_OF > if (!drm_bridge_get_next_bridge(bridge) && > @@ -399,6 +403,33 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm, > if (panel_bridge) > drm_panel_bridge_set_orientation(connector, panel_bridge); > > + if (connector_type == DRM_MODE_CONNECTOR_DisplayPort) { > + drm_connector_attach_dp_subconnector_property(connector, subconnector); > + } else if (connector_type == DRM_MODE_CONNECTOR_DVII) { > + ret = drm_mode_create_dvi_i_properties(drm); > + if (ret) > + return ERR_PTR(ret); > + > + drm_object_attach_property(&connector->base, > + drm->mode_config.dvi_i_subconnector_property, > + subconnector); > + } else if (connector_type == DRM_MODE_CONNECTOR_TV) { > + ret = drm_mode_create_tv_properties(drm, > + BIT(DRM_MODE_TV_MODE_NTSC) | > + BIT(DRM_MODE_TV_MODE_NTSC_443) | > + BIT(DRM_MODE_TV_MODE_NTSC_J) | > + BIT(DRM_MODE_TV_MODE_PAL) | > + BIT(DRM_MODE_TV_MODE_PAL_M) | > + BIT(DRM_MODE_TV_MODE_PAL_N) | > + BIT(DRM_MODE_TV_MODE_SECAM)); > + if (ret) > + return ERR_PTR(ret); I don't think this is right, this should be called from the appropriate encoder device depending on the analog tv mode capabilities. > + > + drm_object_attach_property(&connector->base, > + drm->mode_config.tv_subconnector_property, > + subconnector); Here, only add the property if drm->mode_config.tv_subconnector_property exists, and perhaps add a warning if not. AFAIK same for DRM_MODE_CONNECTOR_DVII. > + } > + > return connector; > } > EXPORT_SYMBOL_GPL(drm_bridge_connector_init); > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h > index bf964cdfb330..68b14ac5ac0d 100644 > --- a/include/drm/drm_bridge.h > +++ b/include/drm/drm_bridge.h > @@ -739,6 +739,10 @@ struct drm_bridge { > * identifies the type of connected display. > */ > int type; > + /** > + * @subtype: the subtype of the connector for the DP/TV/DVI-I cases. > + */ > + enum drm_mode_subconnector subtype; > /** > * @interlace_allowed: Indicate that the bridge can handle interlaced > * modes.
On Wed, 2 Aug 2023 at 11:35, Neil Armstrong <neil.armstrong@linaro.org> wrote: > > On 29/07/2023 02:49, Dmitry Baryshkov wrote: > > If the created connector type supports subconnector type property, > > create and attach corresponding it. The default subtype value is 0, > > which maps to the DRM_MODE_SUBCONNECTOR_Unknown type. > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > --- > > drivers/gpu/drm/drm_bridge_connector.c | 33 +++++++++++++++++++++++++- > > include/drm/drm_bridge.h | 4 ++++ > > 2 files changed, 36 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/drm_bridge_connector.c b/drivers/gpu/drm/drm_bridge_connector.c > > index 07b5930b1282..a7b92f0d2430 100644 > > --- a/drivers/gpu/drm/drm_bridge_connector.c > > +++ b/drivers/gpu/drm/drm_bridge_connector.c > > @@ -329,7 +329,9 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm, > > struct drm_connector *connector; > > struct i2c_adapter *ddc = NULL; > > struct drm_bridge *bridge, *panel_bridge = NULL; > > + enum drm_mode_subconnector subconnector; > > int connector_type; > > + int ret; > > > > bridge_connector = kzalloc(sizeof(*bridge_connector), GFP_KERNEL); > > if (!bridge_connector) > > @@ -365,8 +367,10 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm, > > if (bridge->ops & DRM_BRIDGE_OP_MODES) > > bridge_connector->bridge_modes = bridge; > > > > - if (!drm_bridge_get_next_bridge(bridge)) > > + if (!drm_bridge_get_next_bridge(bridge)) { > > connector_type = bridge->type; > > + subconnector = bridge->subtype; > > + } > > > > #ifdef CONFIG_OF > > if (!drm_bridge_get_next_bridge(bridge) && > > @@ -399,6 +403,33 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm, > > if (panel_bridge) > > drm_panel_bridge_set_orientation(connector, panel_bridge); > > > > + if (connector_type == DRM_MODE_CONNECTOR_DisplayPort) { > > + drm_connector_attach_dp_subconnector_property(connector, subconnector); > > + } else if (connector_type == DRM_MODE_CONNECTOR_DVII) { > > + ret = drm_mode_create_dvi_i_properties(drm); > > + if (ret) > > + return ERR_PTR(ret); > > + > > + drm_object_attach_property(&connector->base, > > + drm->mode_config.dvi_i_subconnector_property, > > + subconnector); > > + } else if (connector_type == DRM_MODE_CONNECTOR_TV) { > > + ret = drm_mode_create_tv_properties(drm, > > + BIT(DRM_MODE_TV_MODE_NTSC) | > > + BIT(DRM_MODE_TV_MODE_NTSC_443) | > > + BIT(DRM_MODE_TV_MODE_NTSC_J) | > > + BIT(DRM_MODE_TV_MODE_PAL) | > > + BIT(DRM_MODE_TV_MODE_PAL_M) | > > + BIT(DRM_MODE_TV_MODE_PAL_N) | > > + BIT(DRM_MODE_TV_MODE_SECAM)); > > + if (ret) > > + return ERR_PTR(ret); > > I don't think this is right, this should be called from the appropriate encoder > device depending on the analog tv mode capabilities. Good question. My logic was the following: the DRM device can have different TV out ports with different capabilities (yeah, pure theoretical construct). In this case it might be impossible to create a single subset of values. Thus it is more correct to create the property listing all possible values. The property is immutable anyway (and so the user doesn't have control over the value). > > + > > + drm_object_attach_property(&connector->base, > > + drm->mode_config.tv_subconnector_property, > > + subconnector); > > Here, only add the property if drm->mode_config.tv_subconnector_property exists, > and perhaps add a warning if not. This property is created in the previous call, drm_mode_create_tv_properties() -> drm_mode_create_tv_properties_legacy(). > > AFAIK same for DRM_MODE_CONNECTOR_DVII. > > > + } > > + > > return connector; > > } > > EXPORT_SYMBOL_GPL(drm_bridge_connector_init); > > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h > > index bf964cdfb330..68b14ac5ac0d 100644 > > --- a/include/drm/drm_bridge.h > > +++ b/include/drm/drm_bridge.h > > @@ -739,6 +739,10 @@ struct drm_bridge { > > * identifies the type of connected display. > > */ > > int type; > > + /** > > + * @subtype: the subtype of the connector for the DP/TV/DVI-I cases. > > + */ > > + enum drm_mode_subconnector subtype; > > /** > > * @interlace_allowed: Indicate that the bridge can handle interlaced > > * modes. >
On Wed, Aug 02, 2023 at 12:05:50PM +0300, Dmitry Baryshkov wrote: > On Wed, 2 Aug 2023 at 11:35, Neil Armstrong wrote: > > On 29/07/2023 02:49, Dmitry Baryshkov wrote: > > > If the created connector type supports subconnector type property, > > > create and attach corresponding it. The default subtype value is 0, > > > which maps to the DRM_MODE_SUBCONNECTOR_Unknown type. > > > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > > --- > > > drivers/gpu/drm/drm_bridge_connector.c | 33 +++++++++++++++++++++++++- > > > include/drm/drm_bridge.h | 4 ++++ > > > 2 files changed, 36 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/drm_bridge_connector.c b/drivers/gpu/drm/drm_bridge_connector.c > > > index 07b5930b1282..a7b92f0d2430 100644 > > > --- a/drivers/gpu/drm/drm_bridge_connector.c > > > +++ b/drivers/gpu/drm/drm_bridge_connector.c > > > @@ -329,7 +329,9 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm, > > > struct drm_connector *connector; > > > struct i2c_adapter *ddc = NULL; > > > struct drm_bridge *bridge, *panel_bridge = NULL; > > > + enum drm_mode_subconnector subconnector; > > > int connector_type; > > > + int ret; > > > > > > bridge_connector = kzalloc(sizeof(*bridge_connector), GFP_KERNEL); > > > if (!bridge_connector) > > > @@ -365,8 +367,10 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm, > > > if (bridge->ops & DRM_BRIDGE_OP_MODES) > > > bridge_connector->bridge_modes = bridge; > > > > > > - if (!drm_bridge_get_next_bridge(bridge)) > > > + if (!drm_bridge_get_next_bridge(bridge)) { > > > connector_type = bridge->type; > > > + subconnector = bridge->subtype; > > > + } > > > > > > #ifdef CONFIG_OF > > > if (!drm_bridge_get_next_bridge(bridge) && > > > @@ -399,6 +403,33 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm, > > > if (panel_bridge) > > > drm_panel_bridge_set_orientation(connector, panel_bridge); > > > > > > + if (connector_type == DRM_MODE_CONNECTOR_DisplayPort) { > > > + drm_connector_attach_dp_subconnector_property(connector, subconnector); > > > + } else if (connector_type == DRM_MODE_CONNECTOR_DVII) { > > > + ret = drm_mode_create_dvi_i_properties(drm); > > > + if (ret) > > > + return ERR_PTR(ret); > > > + > > > + drm_object_attach_property(&connector->base, > > > + drm->mode_config.dvi_i_subconnector_property, > > > + subconnector); > > > + } else if (connector_type == DRM_MODE_CONNECTOR_TV) { > > > + ret = drm_mode_create_tv_properties(drm, > > > + BIT(DRM_MODE_TV_MODE_NTSC) | > > > + BIT(DRM_MODE_TV_MODE_NTSC_443) | > > > + BIT(DRM_MODE_TV_MODE_NTSC_J) | > > > + BIT(DRM_MODE_TV_MODE_PAL) | > > > + BIT(DRM_MODE_TV_MODE_PAL_M) | > > > + BIT(DRM_MODE_TV_MODE_PAL_N) | > > > + BIT(DRM_MODE_TV_MODE_SECAM)); > > > + if (ret) > > > + return ERR_PTR(ret); > > > > I don't think this is right, this should be called from the appropriate encoder > > device depending on the analog tv mode capabilities. > > Good question. My logic was the following: the DRM device can have > different TV out ports with different capabilities (yeah, pure > theoretical construct). In this case it might be impossible to create > a single subset of values. Thus it is more correct to create the > property listing all possible values. The property is immutable anyway > (and so the user doesn't have control over the value). Those ports would correspond to different connectors, so I agree with Neil, I don't think it's right to create a single property with all modes and attach it to all analog output connectors. If you want to support multiple analog outputs that have different capabilities, this will need changes to drm_mode_create_tv_properties() to allow creating multiple properties. If you don't want to do so now, and prefer limiting support to devices where all ports support the same modes (which includes devices with a single analog output), then the modes should reflect what the device supports. > > > + > > > + drm_object_attach_property(&connector->base, > > > + drm->mode_config.tv_subconnector_property, > > > + subconnector); > > > > Here, only add the property if drm->mode_config.tv_subconnector_property exists, > > and perhaps add a warning if not. > > This property is created in the previous call, > drm_mode_create_tv_properties() -> > drm_mode_create_tv_properties_legacy(). > > > AFAIK same for DRM_MODE_CONNECTOR_DVII. > > > > > + } > > > + > > > return connector; > > > } > > > EXPORT_SYMBOL_GPL(drm_bridge_connector_init); > > > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h > > > index bf964cdfb330..68b14ac5ac0d 100644 > > > --- a/include/drm/drm_bridge.h > > > +++ b/include/drm/drm_bridge.h > > > @@ -739,6 +739,10 @@ struct drm_bridge { > > > * identifies the type of connected display. > > > */ > > > int type; > > > + /** > > > + * @subtype: the subtype of the connector for the DP/TV/DVI-I cases. > > > + */ > > > + enum drm_mode_subconnector subtype; > > > /** > > > * @interlace_allowed: Indicate that the bridge can handle interlaced > > > * modes.
On 02/08/2023 21:46, Laurent Pinchart wrote: > On Wed, Aug 02, 2023 at 12:05:50PM +0300, Dmitry Baryshkov wrote: >> On Wed, 2 Aug 2023 at 11:35, Neil Armstrong wrote: >>> On 29/07/2023 02:49, Dmitry Baryshkov wrote: >>>> If the created connector type supports subconnector type property, >>>> create and attach corresponding it. The default subtype value is 0, >>>> which maps to the DRM_MODE_SUBCONNECTOR_Unknown type. >>>> >>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>>> --- >>>> drivers/gpu/drm/drm_bridge_connector.c | 33 +++++++++++++++++++++++++- >>>> include/drm/drm_bridge.h | 4 ++++ >>>> 2 files changed, 36 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/gpu/drm/drm_bridge_connector.c b/drivers/gpu/drm/drm_bridge_connector.c >>>> index 07b5930b1282..a7b92f0d2430 100644 >>>> --- a/drivers/gpu/drm/drm_bridge_connector.c >>>> +++ b/drivers/gpu/drm/drm_bridge_connector.c >>>> @@ -329,7 +329,9 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm, >>>> struct drm_connector *connector; >>>> struct i2c_adapter *ddc = NULL; >>>> struct drm_bridge *bridge, *panel_bridge = NULL; >>>> + enum drm_mode_subconnector subconnector; >>>> int connector_type; >>>> + int ret; >>>> >>>> bridge_connector = kzalloc(sizeof(*bridge_connector), GFP_KERNEL); >>>> if (!bridge_connector) >>>> @@ -365,8 +367,10 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm, >>>> if (bridge->ops & DRM_BRIDGE_OP_MODES) >>>> bridge_connector->bridge_modes = bridge; >>>> >>>> - if (!drm_bridge_get_next_bridge(bridge)) >>>> + if (!drm_bridge_get_next_bridge(bridge)) { >>>> connector_type = bridge->type; >>>> + subconnector = bridge->subtype; >>>> + } >>>> >>>> #ifdef CONFIG_OF >>>> if (!drm_bridge_get_next_bridge(bridge) && >>>> @@ -399,6 +403,33 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm, >>>> if (panel_bridge) >>>> drm_panel_bridge_set_orientation(connector, panel_bridge); >>>> >>>> + if (connector_type == DRM_MODE_CONNECTOR_DisplayPort) { >>>> + drm_connector_attach_dp_subconnector_property(connector, subconnector); >>>> + } else if (connector_type == DRM_MODE_CONNECTOR_DVII) { >>>> + ret = drm_mode_create_dvi_i_properties(drm); >>>> + if (ret) >>>> + return ERR_PTR(ret); >>>> + >>>> + drm_object_attach_property(&connector->base, >>>> + drm->mode_config.dvi_i_subconnector_property, >>>> + subconnector); >>>> + } else if (connector_type == DRM_MODE_CONNECTOR_TV) { >>>> + ret = drm_mode_create_tv_properties(drm, >>>> + BIT(DRM_MODE_TV_MODE_NTSC) | >>>> + BIT(DRM_MODE_TV_MODE_NTSC_443) | >>>> + BIT(DRM_MODE_TV_MODE_NTSC_J) | >>>> + BIT(DRM_MODE_TV_MODE_PAL) | >>>> + BIT(DRM_MODE_TV_MODE_PAL_M) | >>>> + BIT(DRM_MODE_TV_MODE_PAL_N) | >>>> + BIT(DRM_MODE_TV_MODE_SECAM)); >>>> + if (ret) >>>> + return ERR_PTR(ret); >>> >>> I don't think this is right, this should be called from the appropriate encoder >>> device depending on the analog tv mode capabilities. >> >> Good question. My logic was the following: the DRM device can have >> different TV out ports with different capabilities (yeah, pure >> theoretical construct). In this case it might be impossible to create >> a single subset of values. Thus it is more correct to create the >> property listing all possible values. The property is immutable anyway >> (and so the user doesn't have control over the value). > > Those ports would correspond to different connectors, so I agree with > Neil, I don't think it's right to create a single property with all > modes and attach it to all analog output connectors. I agree that this case is mishandled currently (as current design assumes a single property that fits for the complete device). > > If you want to support multiple analog outputs that have different > capabilities, this will need changes to drm_mode_create_tv_properties() > to allow creating multiple properties. If you don't want to do so now, > and prefer limiting support to devices where all ports support the same > modes (which includes devices with a single analog output), then the > modes should reflect what the device supports. Ack, I'll drop the create call and check for the property existence before attaching it. > >>>> + >>>> + drm_object_attach_property(&connector->base, >>>> + drm->mode_config.tv_subconnector_property, >>>> + subconnector); >>> >>> Here, only add the property if drm->mode_config.tv_subconnector_property exists, >>> and perhaps add a warning if not. >> >> This property is created in the previous call, >> drm_mode_create_tv_properties() -> >> drm_mode_create_tv_properties_legacy(). >> >>> AFAIK same for DRM_MODE_CONNECTOR_DVII. >>> >>>> + } >>>> + >>>> return connector; >>>> } >>>> EXPORT_SYMBOL_GPL(drm_bridge_connector_init); >>>> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h >>>> index bf964cdfb330..68b14ac5ac0d 100644 >>>> --- a/include/drm/drm_bridge.h >>>> +++ b/include/drm/drm_bridge.h >>>> @@ -739,6 +739,10 @@ struct drm_bridge { >>>> * identifies the type of connected display. >>>> */ >>>> int type; >>>> + /** >>>> + * @subtype: the subtype of the connector for the DP/TV/DVI-I cases. >>>> + */ >>>> + enum drm_mode_subconnector subtype; >>>> /** >>>> * @interlace_allowed: Indicate that the bridge can handle interlaced >>>> * modes. >
diff --git a/drivers/gpu/drm/drm_bridge_connector.c b/drivers/gpu/drm/drm_bridge_connector.c index 07b5930b1282..a7b92f0d2430 100644 --- a/drivers/gpu/drm/drm_bridge_connector.c +++ b/drivers/gpu/drm/drm_bridge_connector.c @@ -329,7 +329,9 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm, struct drm_connector *connector; struct i2c_adapter *ddc = NULL; struct drm_bridge *bridge, *panel_bridge = NULL; + enum drm_mode_subconnector subconnector; int connector_type; + int ret; bridge_connector = kzalloc(sizeof(*bridge_connector), GFP_KERNEL); if (!bridge_connector) @@ -365,8 +367,10 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm, if (bridge->ops & DRM_BRIDGE_OP_MODES) bridge_connector->bridge_modes = bridge; - if (!drm_bridge_get_next_bridge(bridge)) + if (!drm_bridge_get_next_bridge(bridge)) { connector_type = bridge->type; + subconnector = bridge->subtype; + } #ifdef CONFIG_OF if (!drm_bridge_get_next_bridge(bridge) && @@ -399,6 +403,33 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm, if (panel_bridge) drm_panel_bridge_set_orientation(connector, panel_bridge); + if (connector_type == DRM_MODE_CONNECTOR_DisplayPort) { + drm_connector_attach_dp_subconnector_property(connector, subconnector); + } else if (connector_type == DRM_MODE_CONNECTOR_DVII) { + ret = drm_mode_create_dvi_i_properties(drm); + if (ret) + return ERR_PTR(ret); + + drm_object_attach_property(&connector->base, + drm->mode_config.dvi_i_subconnector_property, + subconnector); + } else if (connector_type == DRM_MODE_CONNECTOR_TV) { + ret = drm_mode_create_tv_properties(drm, + BIT(DRM_MODE_TV_MODE_NTSC) | + BIT(DRM_MODE_TV_MODE_NTSC_443) | + BIT(DRM_MODE_TV_MODE_NTSC_J) | + BIT(DRM_MODE_TV_MODE_PAL) | + BIT(DRM_MODE_TV_MODE_PAL_M) | + BIT(DRM_MODE_TV_MODE_PAL_N) | + BIT(DRM_MODE_TV_MODE_SECAM)); + if (ret) + return ERR_PTR(ret); + + drm_object_attach_property(&connector->base, + drm->mode_config.tv_subconnector_property, + subconnector); + } + return connector; } EXPORT_SYMBOL_GPL(drm_bridge_connector_init); diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h index bf964cdfb330..68b14ac5ac0d 100644 --- a/include/drm/drm_bridge.h +++ b/include/drm/drm_bridge.h @@ -739,6 +739,10 @@ struct drm_bridge { * identifies the type of connected display. */ int type; + /** + * @subtype: the subtype of the connector for the DP/TV/DVI-I cases. + */ + enum drm_mode_subconnector subtype; /** * @interlace_allowed: Indicate that the bridge can handle interlaced * modes.
If the created connector type supports subconnector type property, create and attach corresponding it. The default subtype value is 0, which maps to the DRM_MODE_SUBCONNECTOR_Unknown type. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> --- drivers/gpu/drm/drm_bridge_connector.c | 33 +++++++++++++++++++++++++- include/drm/drm_bridge.h | 4 ++++ 2 files changed, 36 insertions(+), 1 deletion(-)