| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <5464f068-d157-36b3-a1f1-a92ca993511a@linaro.org>
Date: Wed, 2 Aug 2023 21:51:14 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc: neil.armstrong@...aro.org, David Airlie <airlied@...il.com>,
Daniel Vetter <daniel@...ll.ch>,
Andrzej Hajda <andrzej.hajda@...el.com>,
Robert Foss <rfoss@...nel.org>,
Jonas Karlman <jonas@...boo.se>,
Jernej Skrabec <jernej.skrabec@...il.com>,
Andy Gross <agross@...nel.org>,
Bjorn Andersson <andersson@...nel.org>,
Konrad Dybcio <konrad.dybcio@...aro.org>,
Simon Ser <contact@...rsion.fr>, Janne Grunau <j@...nau.net>,
Alex Deucher <alexander.deucher@....com>,
Christian König <christian.koenig@....com>,
"Pan, Xinhui" <Xinhui.Pan@....com>,
Harry Wentland <harry.wentland@....com>,
Leo Li <sunpeng.li@....com>,
Rodrigo Siqueira <Rodrigo.Siqueira@....com>,
Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
Maxime Ripard <mripard@...nel.org>,
Thomas Zimmermann <tzimmermann@...e.de>,
Jani Nikula <jani.nikula@...ux.intel.com>,
Joonas Lahtinen <joonas.lahtinen@...ux.intel.com>,
Rodrigo Vivi <rodrigo.vivi@...el.com>,
Tvrtko Ursulin <tvrtko.ursulin@...ux.intel.com>,
amd-gfx@...ts.freedesktop.org, dri-devel@...ts.freedesktop.org,
linux-kernel@...r.kernel.org, intel-gfx@...ts.freedesktop.org
Subject: Re: [PATCH 2/4] drm/bridge-connector: handle subconnector types
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@...aro.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
Powered by blists - more mailing lists