Messages in this thread | | | Date | Wed, 2 Aug 2023 21:51:14 +0300 | Subject | Re: [PATCH 2/4] drm/bridge-connector: handle subconnector types | From | Dmitry Baryshkov <> |
| |
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. >
-- With best wishes Dmitry
| |