[Intel-gfx] [PATCH 2/4] drm/bridge-connector: handle subconnector types

[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 at 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