lkml.org 
[lkml]   [2023]   [Aug]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH 2/4] drm/bridge-connector: handle subconnector types
From
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

\
 
 \ /
  Last update: 2023-08-02 20:52    [W:0.286 / U:0.080 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site