| Message ID | 20250209225843.7412-1-mwen@igalia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/amd/display: restore edid reading from a given i2c adapter | expand |
On 2/9/2025 16:50, Melissa Wen wrote: > When switching to drm_edid, we slightly changed how to get edid by > removing the possibility of getting them from dc_link when in aux > transaction mode. As MST doesn't initialize the connector with > `drm_connector_init_with_ddc()`, restore the original behavior to avoid > functional changes. > > Fixes: 48edb2a4256e ("drm/amd/display: switch amdgpu_dm_connector to use struct drm_edid") > Signed-off-by: Melissa Wen <mwen@igalia.com> > --- > > > Hi, > > So far, there is no reports about an issue related to this but I noticed > this potential functional change when investigating the previous > freesync problem. I'm not 100% clear if MST takes this path without > initializating connector->ddc, but I propose here to restore the > original behavior to avoid regressions. > > Melissa I think this looks reasonable. Charlie, Can this get included in this week's promotion tests? Thanks, > > > .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 17 +++++++++++++++-- > 1 file changed, 15 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > index a8421c07b160..0cd22a6686a3 100644 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > @@ -7269,8 +7269,14 @@ static void amdgpu_dm_connector_funcs_force(struct drm_connector *connector) > struct dc_link *dc_link = aconnector->dc_link; > struct dc_sink *dc_em_sink = aconnector->dc_em_sink; > const struct drm_edid *drm_edid; > + struct i2c_adapter *ddc; > > - drm_edid = drm_edid_read(connector); > + if (dc_link->aux_mode) > + ddc = &aconnector->dm_dp_aux.aux.ddc; > + else > + ddc = &aconnector->i2c->base; > + > + drm_edid = drm_edid_read_ddc(connector, ddc); > drm_edid_connector_update(connector, drm_edid); > if (!drm_edid) { > DRM_ERROR("No EDID found on connector: %s.\n", connector->name); > @@ -7315,14 +7321,21 @@ static int get_modes(struct drm_connector *connector) > static void create_eml_sink(struct amdgpu_dm_connector *aconnector) > { > struct drm_connector *connector = &aconnector->base; > + struct dc_link *dc_link = aconnector->dc_link; > struct dc_sink_init_data init_params = { > .link = aconnector->dc_link, > .sink_signal = SIGNAL_TYPE_VIRTUAL > }; > const struct drm_edid *drm_edid; > const struct edid *edid; > + struct i2c_adapter *ddc; > > - drm_edid = drm_edid_read(connector); > + if (dc_link->aux_mode) > + ddc = &aconnector->dm_dp_aux.aux.ddc; > + else > + ddc = &aconnector->i2c->base; > + > + drm_edid = drm_edid_read_ddc(connector, ddc); > drm_edid_connector_update(connector, drm_edid); > if (!drm_edid) { > DRM_ERROR("No EDID found on connector: %s.\n", connector->name);
On 2/10/25 15:18, Mario Limonciello wrote: > On 2/9/2025 16:50, Melissa Wen wrote: >> When switching to drm_edid, we slightly changed how to get edid by >> removing the possibility of getting them from dc_link when in aux >> transaction mode. As MST doesn't initialize the connector with >> `drm_connector_init_with_ddc()`, restore the original behavior to avoid >> functional changes. >> >> Fixes: 48edb2a4256e ("drm/amd/display: switch amdgpu_dm_connector to >> use struct drm_edid") >> Signed-off-by: Melissa Wen <mwen@igalia.com> >> --- >> >> >> Hi, >> >> So far, there is no reports about an issue related to this but I noticed >> this potential functional change when investigating the previous >> freesync problem. I'm not 100% clear if MST takes this path without >> initializating connector->ddc, but I propose here to restore the >> original behavior to avoid regressions. >> >> Melissa > > I think this looks reasonable. > > Charlie, > > Can this get included in this week's promotion tests? I just shared this patch to this week's promoter, Roman, and this patch will be included in promotion test. > > Thanks, > >> >> >> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 17 +++++++++++++++-- >> 1 file changed, 15 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/ >> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c >> index a8421c07b160..0cd22a6686a3 100644 >> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c >> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c >> @@ -7269,8 +7269,14 @@ static void >> amdgpu_dm_connector_funcs_force(struct drm_connector *connector) >> struct dc_link *dc_link = aconnector->dc_link; >> struct dc_sink *dc_em_sink = aconnector->dc_em_sink; >> const struct drm_edid *drm_edid; >> + struct i2c_adapter *ddc; >> - drm_edid = drm_edid_read(connector); >> + if (dc_link->aux_mode) >> + ddc = &aconnector->dm_dp_aux.aux.ddc; >> + else >> + ddc = &aconnector->i2c->base; >> + >> + drm_edid = drm_edid_read_ddc(connector, ddc); >> drm_edid_connector_update(connector, drm_edid); >> if (!drm_edid) { >> DRM_ERROR("No EDID found on connector: %s.\n", connector- >> >name); >> @@ -7315,14 +7321,21 @@ static int get_modes(struct drm_connector >> *connector) >> static void create_eml_sink(struct amdgpu_dm_connector *aconnector) >> { >> struct drm_connector *connector = &aconnector->base; >> + struct dc_link *dc_link = aconnector->dc_link; >> struct dc_sink_init_data init_params = { >> .link = aconnector->dc_link, >> .sink_signal = SIGNAL_TYPE_VIRTUAL >> }; >> const struct drm_edid *drm_edid; >> const struct edid *edid; >> + struct i2c_adapter *ddc; >> - drm_edid = drm_edid_read(connector); >> + if (dc_link->aux_mode) >> + ddc = &aconnector->dm_dp_aux.aux.ddc; >> + else >> + ddc = &aconnector->i2c->base; >> + >> + drm_edid = drm_edid_read_ddc(connector, ddc); >> drm_edid_connector_update(connector, drm_edid); >> if (!drm_edid) { >> DRM_ERROR("No EDID found on connector: %s.\n", connector- >> >name); >
Hi Melissa,
kernel test robot noticed the following build warnings:
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Melissa-Wen/drm-amd-display-restore-edid-reading-from-a-given-i2c-adapter/20250210-070016
base: https://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git exynos-drm-next
patch link: https://lore.kernel.org/r/20250209225843.7412-1-mwen%40igalia.com
patch subject: [PATCH] drm/amd/display: restore edid reading from a given i2c adapter
config: x86_64-randconfig-161-20250211 (https://download.01.org/0day-ci/archive/20250212/202502121000.EBCEdoo9-lkp@intel.com/config)
compiler: clang version 19.1.3 (https://github.com/llvm/llvm-project ab51eccf88f5321e7c60591c5546b254b6afab99)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202502121000.EBCEdoo9-lkp@intel.com/
New smatch warnings:
drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:7201 amdgpu_dm_connector_funcs_force() warn: variable dereferenced before check 'dc_link' (see line 7187)
Old smatch warnings:
drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:12110 parse_edid_displayid_vrr() warn: variable dereferenced before check 'edid_ext' (see line 12106)
vim +/dc_link +7201 drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c
dae343b343ff741 Arnd Bergmann 2023-05-01 7179 static void amdgpu_dm_connector_funcs_force(struct drm_connector *connector)
0ba4a784a14592a Alex Hung 2023-04-05 7180 {
0ba4a784a14592a Alex Hung 2023-04-05 7181 struct amdgpu_dm_connector *aconnector = to_amdgpu_dm_connector(connector);
0ba4a784a14592a Alex Hung 2023-04-05 7182 struct dc_link *dc_link = aconnector->dc_link;
0ba4a784a14592a Alex Hung 2023-04-05 7183 struct dc_sink *dc_em_sink = aconnector->dc_em_sink;
48edb2a4256eedf Melissa Wen 2024-09-27 7184 const struct drm_edid *drm_edid;
be1aae525741575 Melissa Wen 2025-02-09 7185 struct i2c_adapter *ddc;
0ba4a784a14592a Alex Hung 2023-04-05 7186
be1aae525741575 Melissa Wen 2025-02-09 @7187 if (dc_link->aux_mode)
^^^^^^^^^^^^^^^^^
Unchecked dereference
be1aae525741575 Melissa Wen 2025-02-09 7188 ddc = &aconnector->dm_dp_aux.aux.ddc;
be1aae525741575 Melissa Wen 2025-02-09 7189 else
be1aae525741575 Melissa Wen 2025-02-09 7190 ddc = &aconnector->i2c->base;
be1aae525741575 Melissa Wen 2025-02-09 7191
be1aae525741575 Melissa Wen 2025-02-09 7192 drm_edid = drm_edid_read_ddc(connector, ddc);
48edb2a4256eedf Melissa Wen 2024-09-27 7193 drm_edid_connector_update(connector, drm_edid);
48edb2a4256eedf Melissa Wen 2024-09-27 7194 if (!drm_edid) {
0e859faf8670a78 Alex Hung 2023-08-25 7195 DRM_ERROR("No EDID found on connector: %s.\n", connector->name);
0ba4a784a14592a Alex Hung 2023-04-05 7196 return;
0e859faf8670a78 Alex Hung 2023-08-25 7197 }
0ba4a784a14592a Alex Hung 2023-04-05 7198
48edb2a4256eedf Melissa Wen 2024-09-27 7199 aconnector->drm_edid = drm_edid;
0ba4a784a14592a Alex Hung 2023-04-05 7200 /* Update emulated (virtual) sink's EDID */
0ba4a784a14592a Alex Hung 2023-04-05 @7201 if (dc_em_sink && dc_link) {
^^^^^^^
This code assumes dc_link can be NULL
48edb2a4256eedf Melissa Wen 2024-09-27 7202 // FIXME: Get rid of drm_edid_raw()
48edb2a4256eedf Melissa Wen 2024-09-27 7203 const struct edid *edid = drm_edid_raw(drm_edid);
48edb2a4256eedf Melissa Wen 2024-09-27 7204
0ba4a784a14592a Alex Hung 2023-04-05 7205 memset(&dc_em_sink->edid_caps, 0, sizeof(struct dc_edid_caps));
48edb2a4256eedf Melissa Wen 2024-09-27 7206 memmove(dc_em_sink->dc_edid.raw_edid, edid,
48edb2a4256eedf Melissa Wen 2024-09-27 7207 (edid->extensions + 1) * EDID_LENGTH);
0ba4a784a14592a Alex Hung 2023-04-05 7208 dm_helpers_parse_edid_caps(
0ba4a784a14592a Alex Hung 2023-04-05 7209 dc_link,
0ba4a784a14592a Alex Hung 2023-04-05 7210 &dc_em_sink->dc_edid,
0ba4a784a14592a Alex Hung 2023-04-05 7211 &dc_em_sink->edid_caps);
0ba4a784a14592a Alex Hung 2023-04-05 7212 }
0ba4a784a14592a Alex Hung 2023-04-05 7213 }
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c index a8421c07b160..0cd22a6686a3 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -7269,8 +7269,14 @@ static void amdgpu_dm_connector_funcs_force(struct drm_connector *connector) struct dc_link *dc_link = aconnector->dc_link; struct dc_sink *dc_em_sink = aconnector->dc_em_sink; const struct drm_edid *drm_edid; + struct i2c_adapter *ddc; - drm_edid = drm_edid_read(connector); + if (dc_link->aux_mode) + ddc = &aconnector->dm_dp_aux.aux.ddc; + else + ddc = &aconnector->i2c->base; + + drm_edid = drm_edid_read_ddc(connector, ddc); drm_edid_connector_update(connector, drm_edid); if (!drm_edid) { DRM_ERROR("No EDID found on connector: %s.\n", connector->name); @@ -7315,14 +7321,21 @@ static int get_modes(struct drm_connector *connector) static void create_eml_sink(struct amdgpu_dm_connector *aconnector) { struct drm_connector *connector = &aconnector->base; + struct dc_link *dc_link = aconnector->dc_link; struct dc_sink_init_data init_params = { .link = aconnector->dc_link, .sink_signal = SIGNAL_TYPE_VIRTUAL }; const struct drm_edid *drm_edid; const struct edid *edid; + struct i2c_adapter *ddc; - drm_edid = drm_edid_read(connector); + if (dc_link->aux_mode) + ddc = &aconnector->dm_dp_aux.aux.ddc; + else + ddc = &aconnector->i2c->base; + + drm_edid = drm_edid_read_ddc(connector, ddc); drm_edid_connector_update(connector, drm_edid); if (!drm_edid) { DRM_ERROR("No EDID found on connector: %s.\n", connector->name);
When switching to drm_edid, we slightly changed how to get edid by removing the possibility of getting them from dc_link when in aux transaction mode. As MST doesn't initialize the connector with `drm_connector_init_with_ddc()`, restore the original behavior to avoid functional changes. Fixes: 48edb2a4256e ("drm/amd/display: switch amdgpu_dm_connector to use struct drm_edid") Signed-off-by: Melissa Wen <mwen@igalia.com> --- Hi, So far, there is no reports about an issue related to this but I noticed this potential functional change when investigating the previous freesync problem. I'm not 100% clear if MST takes this path without initializating connector->ddc, but I propose here to restore the original behavior to avoid regressions. Melissa .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-)