* [PATCH 001/001] drivers/gpu/radeon: NULL pointer deference workaround
@ 2016-09-07 17:38 Mark Fortescue
2016-09-08 7:14 ` Christian König
0 siblings, 1 reply; 8+ messages in thread
From: Mark Fortescue @ 2016-09-07 17:38 UTC (permalink / raw)
To: David Airlie; +Cc: Alex Deucher, Christian König, dri-devel, linux-kernel
On an LV-683 (AMD Dual-core G-T56N) Mini-ITX board, I get a Kernel Oops
because Connector 0 (LCD Panel interface) does not have DDC.
Ubuntu 16.04 LTS Kernel (4.4 series):
...
[ 8.262990] [drm] ib test on ring 5 succeeded
[ 8.288897] [drm] Radeon Display Connectors
[ 8.293175] [drm] Connector 0:
[ 8.296252] [drm] DP-1
[ 8.298791] [drm] Encoders:
[ 8.301770] [drm] DFP1: INTERNAL_UNIPHY
[ 8.305973] [drm] Connector 1:
[ 8.309043] [drm] DP-2
[ 8.311598] [drm] HPD2
[ 8.314169] [drm] DDC: 0x6440 0x6440 0x6444 0x6444 0x6448 0x6448 0x644c
0x644c
[ 8.321609] [drm] Encoders:
[ 8.324589] [drm] DFP2: INTERNAL_UNIPHY
[ 8.328793] [drm] Connector 2:
[ 8.331856] [drm] VGA-1
[ 8.342947] [drm] DDC: 0x64d8 0x64d8 0x64dc 0x64dc 0x64e0 0x64e0 0x64e4
0x64e4
[ 8.350341] [drm] Encoders:
[ 8.353310] [drm] CRT1: INTERNAL_KLDSCP_DAC1
[ 8.358195] BUG: unable to handle kernel NULL pointer dereference at
0000000000000409
[ 8.409733] [<ffffffffc02024da>] radeon_dp_getsinktype+0x1a/0x30 [radeon]
[ 8.416805] PGD 0
[ 8.418841] Oops: 0000 [#1] SMP
...
This patch prevents Kernel failures due to a connector not having a DDC
interface by changing the code so that ddc_bus is always checked before use.
The problem was first identified using the uBuntu MATE 14.04 LTS (3.16
series kernels) but not dealt with at that time. On attempting to
install uBuntu MATE 16.04 LTS (4.4 series kernels), it became clear that
using various workarounds to allow the issue to be ignored were not
viable so more effort was put in to sorting the issue resulting in this
patch. See https://bugs.launchpad.net/bugs/1587885 for more details.
Signed-off-by: Mark Fortescue <mark@thurning-instruments.co.uk>
Tested-by: Mark Fortescue <mark@thurning-instruments.co.uk>
---
Looks like Thunderbird may have made a mess of the patch when pasting
the contents into the mail message - my alternate mail client (pine)
also has strict line length handling and trashes non-MIME encoded patches.
This may not be the correct approach to solving the issue but it is
clean in that it ensures that ddc_bus is never used when NULL regardless
of how the code ended up at the point of use.
If it helps with back porting, I have patches for the uBuntu 14.04 LTS
[3.13 series], uBuntu MATE 14.04 LTS [3.16 series] and uBuntu 16.04 LTS
[4.4 series] kernels.
Test Hardware:
Commell LV-683 Mini-ITX with onboard AMD Dual-core G-T56N
4G Ram, 2x1TB Disk, HANNS-G HC194D 1280x1024 LCD (VGA).
4.8.0-rc5 with patch boots without error.
drivers/gpu/drm/radeon/atombios_dp.c | 60 ++++++++++++-------
drivers/gpu/drm/radeon/radeon_connectors.c | 46 +++++++-------
drivers/gpu/drm/radeon/radeon_dp_mst.c | 9 ++
drivers/gpu/drm/radeon/radeon_i2c.c | 3
4 files changed, 73 insertions(+), 45 deletions(-)
Patch:
diff --git a/drivers/gpu/drm/radeon/atombios_dp.c
b/drivers/gpu/drm/radeon/atombios_dp.c
index cead089a..98b3c0e 100644
--- a/drivers/gpu/drm/radeon/atombios_dp.c
+++ b/drivers/gpu/drm/radeon/atombios_dp.c
@@ -232,6 +232,9 @@ void radeon_dp_aux_init(struct radeon_connector
*radeon_connector)
struct radeon_device *rdev = dev->dev_private;
int ret;
+ if (!radeon_connector->ddc_bus)
+ return;
+
radeon_connector->ddc_bus->rec.hpd = radeon_connector->hpd.hpd;
radeon_connector->ddc_bus->aux.dev = radeon_connector->base.kdev;
if (ASIC_IS_DCE5(rdev)) {
@@ -364,6 +367,9 @@ u8 radeon_dp_getsinktype(struct radeon_connector
*radeon_connector)
struct drm_device *dev = radeon_connector->base.dev;
struct radeon_device *rdev = dev->dev_private;
+ if (!radeon_connector->ddc_bus)
+ return 0;
+
return radeon_dp_encoder_service(rdev, ATOM_DP_ACTION_GET_SINK_TYPE, 0,
radeon_connector->ddc_bus->rec.i2c_id, 0);
}
@@ -376,6 +382,9 @@ static void radeon_dp_probe_oui(struct
radeon_connector *radeon_connector)
if (!(dig_connector->dpcd[DP_DOWN_STREAM_PORT_COUNT] & DP_OUI_SUPPORT))
return;
+ if (!radeon_connector->ddc_bus)
+ return;
+
if (drm_dp_dpcd_read(&radeon_connector->ddc_bus->aux, DP_SINK_OUI,
buf, 3) == 3)
DRM_DEBUG_KMS("Sink OUI: %02hx%02hx%02hx\n",
buf[0], buf[1], buf[2]);
@@ -391,6 +400,9 @@ bool radeon_dp_getdpcd(struct radeon_connector
*radeon_connector)
u8 msg[DP_DPCD_SIZE];
int ret, i;
+ if (!radeon_connector->ddc_bus)
+ return false;
+
for (i = 0; i < 7; i++) {
ret = drm_dp_dpcd_read(&radeon_connector->ddc_bus->aux,
DP_DPCD_REV, msg,
DP_DPCD_SIZE);
@@ -428,24 +440,26 @@ int radeon_dp_get_panel_mode(struct drm_encoder
*encoder,
dig_connector = radeon_connector->con_priv;
- if (dp_bridge != ENCODER_OBJECT_ID_NONE) {
- /* DP bridge chips */
- if (drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux,
- DP_EDP_CONFIGURATION_CAP, &tmp) == 1) {
- if (tmp & 1)
- panel_mode = DP_PANEL_MODE_INTERNAL_DP2_MODE;
- else if ((dp_bridge == ENCODER_OBJECT_ID_NUTMEG) ||
- (dp_bridge == ENCODER_OBJECT_ID_TRAVIS))
- panel_mode = DP_PANEL_MODE_INTERNAL_DP1_MODE;
- else
- panel_mode = DP_PANEL_MODE_EXTERNAL_DP_MODE;
- }
- } else if (connector->connector_type == DRM_MODE_CONNECTOR_eDP) {
- /* eDP */
- if (drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux,
- DP_EDP_CONFIGURATION_CAP, &tmp) == 1) {
- if (tmp & 1)
- panel_mode = DP_PANEL_MODE_INTERNAL_DP2_MODE;
+ if (radeon_connector->ddc_bus) {
+ if (dp_bridge != ENCODER_OBJECT_ID_NONE) {
+ /* DP bridge chips */
+ if (drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux,
+ DP_EDP_CONFIGURATION_CAP, &tmp) == 1) {
+ if (tmp & 1)
+ panel_mode = DP_PANEL_MODE_INTERNAL_DP2_MODE;
+ else if ((dp_bridge == ENCODER_OBJECT_ID_NUTMEG) ||
+ (dp_bridge == ENCODER_OBJECT_ID_TRAVIS))
+ panel_mode = DP_PANEL_MODE_INTERNAL_DP1_MODE;
+ else
+ panel_mode = DP_PANEL_MODE_EXTERNAL_DP_MODE;
+ }
+ } else if (connector->connector_type == DRM_MODE_CONNECTOR_eDP) {
+ /* eDP */
+ if (drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux,
+ DP_EDP_CONFIGURATION_CAP, &tmp) == 1) {
+ if (tmp & 1)
+ panel_mode = DP_PANEL_MODE_INTERNAL_DP2_MODE;
+ }
}
}
@@ -511,6 +525,9 @@ bool radeon_dp_needs_link_train(struct
radeon_connector *radeon_connector)
u8 link_status[DP_LINK_STATUS_SIZE];
struct radeon_connector_atom_dig *dig = radeon_connector->con_priv;
+ if (!radeon_connector->ddc_bus)
+ return false;
+
if (drm_dp_dpcd_read_link_status(&radeon_connector->ddc_bus->aux,
link_status)
<= 0)
return false;
@@ -531,7 +548,7 @@ void radeon_dp_set_rx_power_state(struct
drm_connector *connector,
dig_connector = radeon_connector->con_priv;
/* power up/down the sink */
- if (dig_connector->dpcd[0] >= 0x11) {
+ if (radeon_connector->ddc_bus && dig_connector->dpcd[0] >= 0x11) {
drm_dp_dpcd_writeb(&radeon_connector->ddc_bus->aux,
DP_SET_POWER, power_state);
usleep_range(1000, 2000);
@@ -834,7 +851,8 @@ void radeon_dp_link_train(struct drm_encoder *encoder,
else
dp_info.enc_id |= ATOM_DP_CONFIG_LINK_A;
- if (drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux,
DP_MAX_LANE_COUNT, &tmp)
+ if (radeon_connector->ddc_bus &&
+ drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux,
DP_MAX_LANE_COUNT, &tmp)
== 1) {
if (ASIC_IS_DCE5(rdev) && (tmp & DP_TPS3_SUPPORTED))
dp_info.tp3_supported = true;
@@ -850,7 +868,7 @@ void radeon_dp_link_train(struct drm_encoder *encoder,
dp_info.connector = connector;
dp_info.dp_lane_count = dig_connector->dp_lane_count;
dp_info.dp_clock = dig_connector->dp_clock;
- dp_info.aux = &radeon_connector->ddc_bus->aux;
+ dp_info.aux = radeon_connector->ddc_bus ?
&radeon_connector->ddc_bus->aux : 0;
if (radeon_dp_link_train_init(&dp_info))
goto done;
diff --git a/drivers/gpu/drm/radeon/radeon_connectors.c
b/drivers/gpu/drm/radeon/radeon_connectors.c
index b79f3b0..cec30c9 100644
--- a/drivers/gpu/drm/radeon/radeon_connectors.c
+++ b/drivers/gpu/drm/radeon/radeon_connectors.c
@@ -328,31 +328,32 @@ static void radeon_connector_get_edid(struct
drm_connector *connector)
if (radeon_connector->router.ddc_valid)
radeon_router_select_ddc_port(radeon_connector);
- if ((radeon_connector_encoder_get_dp_bridge_encoder_id(connector) !=
- ENCODER_OBJECT_ID_NONE) &&
- radeon_connector->ddc_bus->has_aux) {
- radeon_connector->edid = drm_get_edid(connector,
- &radeon_connector->ddc_bus->aux.ddc);
- } else if ((connector->connector_type ==
DRM_MODE_CONNECTOR_DisplayPort) ||
- (connector->connector_type == DRM_MODE_CONNECTOR_eDP)) {
- struct radeon_connector_atom_dig *dig = radeon_connector->con_priv;
-
- if ((dig->dp_sink_type == CONNECTOR_OBJECT_ID_DISPLAYPORT ||
- dig->dp_sink_type == CONNECTOR_OBJECT_ID_eDP) &&
- radeon_connector->ddc_bus->has_aux)
- radeon_connector->edid = drm_get_edid(&radeon_connector->base,
+ if (radeon_connector->ddc_bus) {
+ if ((radeon_connector_encoder_get_dp_bridge_encoder_id(connector) !=
+ ENCODER_OBJECT_ID_NONE) &&
+ radeon_connector->ddc_bus->has_aux) {
+ radeon_connector->edid = drm_get_edid(connector,
&radeon_connector->ddc_bus->aux.ddc);
- else if (radeon_connector->ddc_bus)
+ } else if ((connector->connector_type ==
DRM_MODE_CONNECTOR_DisplayPort) ||
+ (connector->connector_type == DRM_MODE_CONNECTOR_eDP)) {
+ struct radeon_connector_atom_dig *dig = radeon_connector->con_priv;
+
+ if ((dig->dp_sink_type == CONNECTOR_OBJECT_ID_DISPLAYPORT ||
+ dig->dp_sink_type == CONNECTOR_OBJECT_ID_eDP) &&
+ radeon_connector->ddc_bus->has_aux)
+ radeon_connector->edid = drm_get_edid(&radeon_connector->base,
+ &radeon_connector->ddc_bus->aux.ddc);
+ else
+ radeon_connector->edid = drm_get_edid(&radeon_connector->base,
+ &radeon_connector->ddc_bus->adapter);
+ } else if (vga_switcheroo_handler_flags() &
VGA_SWITCHEROO_CAN_SWITCH_DDC &&
+ connector->connector_type == DRM_MODE_CONNECTOR_LVDS) {
+ radeon_connector->edid =
drm_get_edid_switcheroo(&radeon_connector->base,
+ &radeon_connector->ddc_bus->adapter);
+ } else {
radeon_connector->edid = drm_get_edid(&radeon_connector->base,
&radeon_connector->ddc_bus->adapter);
- } else if (vga_switcheroo_handler_flags() &
VGA_SWITCHEROO_CAN_SWITCH_DDC &&
- connector->connector_type == DRM_MODE_CONNECTOR_LVDS &&
- radeon_connector->ddc_bus) {
- radeon_connector->edid = drm_get_edid_switcheroo(&radeon_connector->base,
- &radeon_connector->ddc_bus->adapter);
- } else if (radeon_connector->ddc_bus) {
- radeon_connector->edid = drm_get_edid(&radeon_connector->base,
- &radeon_connector->ddc_bus->adapter);
+ }
}
if (!radeon_connector->edid) {
@@ -1312,6 +1313,7 @@ radeon_dvi_detect(struct drm_connector *connector,
bool force)
continue;
list_radeon_connector = to_radeon_connector(list_connector);
if (list_radeon_connector->shared_ddc &&
+ radeon_connector->ddc_bus &&
(list_radeon_connector->ddc_bus->rec.i2c_id ==
radeon_connector->ddc_bus->rec.i2c_id)) {
/* cases where both connectors are digital */
diff --git a/drivers/gpu/drm/radeon/radeon_dp_mst.c
b/drivers/gpu/drm/radeon/radeon_dp_mst.c
index de504ea..89e91f3 100644
--- a/drivers/gpu/drm/radeon/radeon_dp_mst.c
+++ b/drivers/gpu/drm/radeon/radeon_dp_mst.c
@@ -661,7 +661,7 @@ radeon_dp_mst_init(struct radeon_connector
*radeon_connector)
{
struct drm_device *dev = radeon_connector->base.dev;
- if (!radeon_connector->ddc_bus->has_aux)
+ if (!radeon_connector->ddc_bus || !radeon_connector->ddc_bus->has_aux)
return 0;
radeon_connector->mst_mgr.cbs = &mst_cbs;
@@ -688,6 +688,9 @@ radeon_dp_mst_probe(struct radeon_connector
*radeon_connector)
if (dig_connector->dpcd[DP_DPCD_REV] < 0x12)
return 0;
+ if (!radeon_connector->ddc_bus || !radeon_connector->ddc_bus->has_aux)
+ return 0;
+
ret = drm_dp_dpcd_read(&radeon_connector->ddc_bus->aux, DP_MSTM_CAP,
msg,
1);
if (ret) {
@@ -711,7 +714,9 @@ radeon_dp_mst_check_status(struct radeon_connector
*radeon_connector)
struct radeon_connector_atom_dig *dig_connector =
radeon_connector->con_priv;
int retry;
- if (dig_connector->is_mst) {
+ if (dig_connector->is_mst &&
+ radeon_connector->ddc_bus &&
+ radeon_connector->ddc_bus->has_aux) {
u8 esi[16] = { 0 };
int dret;
int ret = 0;
diff --git a/drivers/gpu/drm/radeon/radeon_i2c.c
b/drivers/gpu/drm/radeon/radeon_i2c.c
index 9590bcd..c18f7ba 100644
--- a/drivers/gpu/drm/radeon/radeon_i2c.c
+++ b/drivers/gpu/drm/radeon/radeon_i2c.c
@@ -63,6 +63,9 @@ bool radeon_ddc_probe(struct radeon_connector
*radeon_connector, bool use_aux)
if (radeon_connector->router.ddc_valid)
radeon_router_select_ddc_port(radeon_connector);
+ if (!radeon_connector->ddc_bus)
+ return false;
+
if (use_aux) {
ret = i2c_transfer(&radeon_connector->ddc_bus->aux.ddc, msgs, 2);
} else {
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 001/001] drivers/gpu/radeon: NULL pointer deference workaround
2016-09-07 17:38 [PATCH 001/001] drivers/gpu/radeon: NULL pointer deference workaround Mark Fortescue
@ 2016-09-08 7:14 ` Christian König
2016-09-08 9:09 ` Mark Fortescue
0 siblings, 1 reply; 8+ messages in thread
From: Christian König @ 2016-09-08 7:14 UTC (permalink / raw)
To: Mark Fortescue, David Airlie
Cc: Alex Deucher, Christian König, dri-devel, linux-kernel
Am 07.09.2016 um 19:38 schrieb Mark Fortescue:
>
> On an LV-683 (AMD Dual-core G-T56N) Mini-ITX board, I get a Kernel
> Oops because Connector 0 (LCD Panel interface) does not have DDC.
I'm not an expert on this, but that is really odd cause even LCD Panels
should have a DDC interface.
>
> Ubuntu 16.04 LTS Kernel (4.4 series):
>
> ...
> [ 8.262990] [drm] ib test on ring 5 succeeded
> [ 8.288897] [drm] Radeon Display Connectors
> [ 8.293175] [drm] Connector 0:
> [ 8.296252] [drm] DP-1
Especially since the BIOS claims that this is a displayport connector
and there is no physical way to have a DP without an DDC as far as I know.
Please open a bug report on FDO and attach you BIOS image.
Alex can probably take a look when he's back from vacation.
Regards,
Christian.
> [ 8.298791] [drm] Encoders:
> [ 8.301770] [drm] DFP1: INTERNAL_UNIPHY
> [ 8.305973] [drm] Connector 1:
> [ 8.309043] [drm] DP-2
> [ 8.311598] [drm] HPD2
> [ 8.314169] [drm] DDC: 0x6440 0x6440 0x6444 0x6444 0x6448 0x6448
> 0x644c 0x644c
> [ 8.321609] [drm] Encoders:
> [ 8.324589] [drm] DFP2: INTERNAL_UNIPHY
> [ 8.328793] [drm] Connector 2:
> [ 8.331856] [drm] VGA-1
> [ 8.342947] [drm] DDC: 0x64d8 0x64d8 0x64dc 0x64dc 0x64e0 0x64e0
> 0x64e4 0x64e4
> [ 8.350341] [drm] Encoders:
> [ 8.353310] [drm] CRT1: INTERNAL_KLDSCP_DAC1
> [ 8.358195] BUG: unable to handle kernel NULL pointer dereference at
> 0000000000000409
> [ 8.409733] [<ffffffffc02024da>] radeon_dp_getsinktype+0x1a/0x30 [radeon]
> [ 8.416805] PGD 0
> [ 8.418841] Oops: 0000 [#1] SMP
> ...
>
> This patch prevents Kernel failures due to a connector not having a
> DDC interface by changing the code so that ddc_bus is always checked
> before use.
> The problem was first identified using the uBuntu MATE 14.04 LTS (3.16
> series kernels) but not dealt with at that time. On attempting to
> install uBuntu MATE 16.04 LTS (4.4 series kernels), it became clear
> that using various workarounds to allow the issue to be ignored were
> not viable so more effort was put in to sorting the issue resulting in
> this patch. See https://bugs.launchpad.net/bugs/1587885 for more details.
>
> Signed-off-by: Mark Fortescue <mark@thurning-instruments.co.uk>
> Tested-by: Mark Fortescue <mark@thurning-instruments.co.uk>
>
> ---
>
> Looks like Thunderbird may have made a mess of the patch when pasting
> the contents into the mail message - my alternate mail client (pine)
> also has strict line length handling and trashes non-MIME encoded
> patches.
>
> This may not be the correct approach to solving the issue but it is
> clean in that it ensures that ddc_bus is never used when NULL
> regardless of how the code ended up at the point of use.
>
> If it helps with back porting, I have patches for the uBuntu 14.04 LTS
> [3.13 series], uBuntu MATE 14.04 LTS [3.16 series] and uBuntu 16.04
> LTS [4.4 series] kernels.
>
> Test Hardware:
> Commell LV-683 Mini-ITX with onboard AMD Dual-core G-T56N
> 4G Ram, 2x1TB Disk, HANNS-G HC194D 1280x1024 LCD (VGA).
> 4.8.0-rc5 with patch boots without error.
>
> drivers/gpu/drm/radeon/atombios_dp.c | 60 ++++++++++++-------
> drivers/gpu/drm/radeon/radeon_connectors.c | 46 +++++++-------
> drivers/gpu/drm/radeon/radeon_dp_mst.c | 9 ++
> drivers/gpu/drm/radeon/radeon_i2c.c | 3
> 4 files changed, 73 insertions(+), 45 deletions(-)
>
> Patch:
> diff --git a/drivers/gpu/drm/radeon/atombios_dp.c
> b/drivers/gpu/drm/radeon/atombios_dp.c
> index cead089a..98b3c0e 100644
> --- a/drivers/gpu/drm/radeon/atombios_dp.c
> +++ b/drivers/gpu/drm/radeon/atombios_dp.c
> @@ -232,6 +232,9 @@ void radeon_dp_aux_init(struct radeon_connector
> *radeon_connector)
> struct radeon_device *rdev = dev->dev_private;
> int ret;
>
> + if (!radeon_connector->ddc_bus)
> + return;
> +
> radeon_connector->ddc_bus->rec.hpd = radeon_connector->hpd.hpd;
> radeon_connector->ddc_bus->aux.dev = radeon_connector->base.kdev;
> if (ASIC_IS_DCE5(rdev)) {
> @@ -364,6 +367,9 @@ u8 radeon_dp_getsinktype(struct radeon_connector
> *radeon_connector)
> struct drm_device *dev = radeon_connector->base.dev;
> struct radeon_device *rdev = dev->dev_private;
>
> + if (!radeon_connector->ddc_bus)
> + return 0;
> +
> return radeon_dp_encoder_service(rdev,
> ATOM_DP_ACTION_GET_SINK_TYPE, 0,
> radeon_connector->ddc_bus->rec.i2c_id, 0);
> }
> @@ -376,6 +382,9 @@ static void radeon_dp_probe_oui(struct
> radeon_connector *radeon_connector)
> if (!(dig_connector->dpcd[DP_DOWN_STREAM_PORT_COUNT] &
> DP_OUI_SUPPORT))
> return;
>
> + if (!radeon_connector->ddc_bus)
> + return;
> +
> if (drm_dp_dpcd_read(&radeon_connector->ddc_bus->aux,
> DP_SINK_OUI, buf, 3) == 3)
> DRM_DEBUG_KMS("Sink OUI: %02hx%02hx%02hx\n",
> buf[0], buf[1], buf[2]);
> @@ -391,6 +400,9 @@ bool radeon_dp_getdpcd(struct radeon_connector
> *radeon_connector)
> u8 msg[DP_DPCD_SIZE];
> int ret, i;
>
> + if (!radeon_connector->ddc_bus)
> + return false;
> +
> for (i = 0; i < 7; i++) {
> ret = drm_dp_dpcd_read(&radeon_connector->ddc_bus->aux,
> DP_DPCD_REV, msg,
> DP_DPCD_SIZE);
> @@ -428,24 +440,26 @@ int radeon_dp_get_panel_mode(struct drm_encoder
> *encoder,
>
> dig_connector = radeon_connector->con_priv;
>
> - if (dp_bridge != ENCODER_OBJECT_ID_NONE) {
> - /* DP bridge chips */
> - if (drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux,
> - DP_EDP_CONFIGURATION_CAP, &tmp) == 1) {
> - if (tmp & 1)
> - panel_mode = DP_PANEL_MODE_INTERNAL_DP2_MODE;
> - else if ((dp_bridge == ENCODER_OBJECT_ID_NUTMEG) ||
> - (dp_bridge == ENCODER_OBJECT_ID_TRAVIS))
> - panel_mode = DP_PANEL_MODE_INTERNAL_DP1_MODE;
> - else
> - panel_mode = DP_PANEL_MODE_EXTERNAL_DP_MODE;
> - }
> - } else if (connector->connector_type == DRM_MODE_CONNECTOR_eDP) {
> - /* eDP */
> - if (drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux,
> - DP_EDP_CONFIGURATION_CAP, &tmp) == 1) {
> - if (tmp & 1)
> - panel_mode = DP_PANEL_MODE_INTERNAL_DP2_MODE;
> + if (radeon_connector->ddc_bus) {
> + if (dp_bridge != ENCODER_OBJECT_ID_NONE) {
> + /* DP bridge chips */
> + if (drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux,
> + DP_EDP_CONFIGURATION_CAP, &tmp) == 1) {
> + if (tmp & 1)
> + panel_mode = DP_PANEL_MODE_INTERNAL_DP2_MODE;
> + else if ((dp_bridge == ENCODER_OBJECT_ID_NUTMEG) ||
> + (dp_bridge == ENCODER_OBJECT_ID_TRAVIS))
> + panel_mode = DP_PANEL_MODE_INTERNAL_DP1_MODE;
> + else
> + panel_mode = DP_PANEL_MODE_EXTERNAL_DP_MODE;
> + }
> + } else if (connector->connector_type ==
> DRM_MODE_CONNECTOR_eDP) {
> + /* eDP */
> + if (drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux,
> + DP_EDP_CONFIGURATION_CAP, &tmp) == 1) {
> + if (tmp & 1)
> + panel_mode = DP_PANEL_MODE_INTERNAL_DP2_MODE;
> + }
> }
> }
>
> @@ -511,6 +525,9 @@ bool radeon_dp_needs_link_train(struct
> radeon_connector *radeon_connector)
> u8 link_status[DP_LINK_STATUS_SIZE];
> struct radeon_connector_atom_dig *dig = radeon_connector->con_priv;
>
> + if (!radeon_connector->ddc_bus)
> + return false;
> +
> if
> (drm_dp_dpcd_read_link_status(&radeon_connector->ddc_bus->aux,
> link_status)
> <= 0)
> return false;
> @@ -531,7 +548,7 @@ void radeon_dp_set_rx_power_state(struct
> drm_connector *connector,
> dig_connector = radeon_connector->con_priv;
>
> /* power up/down the sink */
> - if (dig_connector->dpcd[0] >= 0x11) {
> + if (radeon_connector->ddc_bus && dig_connector->dpcd[0] >= 0x11) {
> drm_dp_dpcd_writeb(&radeon_connector->ddc_bus->aux,
> DP_SET_POWER, power_state);
> usleep_range(1000, 2000);
> @@ -834,7 +851,8 @@ void radeon_dp_link_train(struct drm_encoder
> *encoder,
> else
> dp_info.enc_id |= ATOM_DP_CONFIG_LINK_A;
>
> - if (drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux,
> DP_MAX_LANE_COUNT, &tmp)
> + if (radeon_connector->ddc_bus &&
> + drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux,
> DP_MAX_LANE_COUNT, &tmp)
> == 1) {
> if (ASIC_IS_DCE5(rdev) && (tmp & DP_TPS3_SUPPORTED))
> dp_info.tp3_supported = true;
> @@ -850,7 +868,7 @@ void radeon_dp_link_train(struct drm_encoder
> *encoder,
> dp_info.connector = connector;
> dp_info.dp_lane_count = dig_connector->dp_lane_count;
> dp_info.dp_clock = dig_connector->dp_clock;
> - dp_info.aux = &radeon_connector->ddc_bus->aux;
> + dp_info.aux = radeon_connector->ddc_bus ?
> &radeon_connector->ddc_bus->aux : 0;
>
> if (radeon_dp_link_train_init(&dp_info))
> goto done;
> diff --git a/drivers/gpu/drm/radeon/radeon_connectors.c
> b/drivers/gpu/drm/radeon/radeon_connectors.c
> index b79f3b0..cec30c9 100644
> --- a/drivers/gpu/drm/radeon/radeon_connectors.c
> +++ b/drivers/gpu/drm/radeon/radeon_connectors.c
> @@ -328,31 +328,32 @@ static void radeon_connector_get_edid(struct
> drm_connector *connector)
> if (radeon_connector->router.ddc_valid)
> radeon_router_select_ddc_port(radeon_connector);
>
> - if ((radeon_connector_encoder_get_dp_bridge_encoder_id(connector) !=
> - ENCODER_OBJECT_ID_NONE) &&
> - radeon_connector->ddc_bus->has_aux) {
> - radeon_connector->edid = drm_get_edid(connector,
> - &radeon_connector->ddc_bus->aux.ddc);
> - } else if ((connector->connector_type ==
> DRM_MODE_CONNECTOR_DisplayPort) ||
> - (connector->connector_type == DRM_MODE_CONNECTOR_eDP)) {
> - struct radeon_connector_atom_dig *dig =
> radeon_connector->con_priv;
> -
> - if ((dig->dp_sink_type == CONNECTOR_OBJECT_ID_DISPLAYPORT ||
> - dig->dp_sink_type == CONNECTOR_OBJECT_ID_eDP) &&
> - radeon_connector->ddc_bus->has_aux)
> - radeon_connector->edid =
> drm_get_edid(&radeon_connector->base,
> + if (radeon_connector->ddc_bus) {
> + if
> ((radeon_connector_encoder_get_dp_bridge_encoder_id(connector) !=
> + ENCODER_OBJECT_ID_NONE) &&
> + radeon_connector->ddc_bus->has_aux) {
> + radeon_connector->edid = drm_get_edid(connector,
> &radeon_connector->ddc_bus->aux.ddc);
> - else if (radeon_connector->ddc_bus)
> + } else if ((connector->connector_type ==
> DRM_MODE_CONNECTOR_DisplayPort) ||
> + (connector->connector_type == DRM_MODE_CONNECTOR_eDP)) {
> + struct radeon_connector_atom_dig *dig =
> radeon_connector->con_priv;
> +
> + if ((dig->dp_sink_type == CONNECTOR_OBJECT_ID_DISPLAYPORT ||
> + dig->dp_sink_type == CONNECTOR_OBJECT_ID_eDP) &&
> + radeon_connector->ddc_bus->has_aux)
> + radeon_connector->edid =
> drm_get_edid(&radeon_connector->base,
> + &radeon_connector->ddc_bus->aux.ddc);
> + else
> + radeon_connector->edid =
> drm_get_edid(&radeon_connector->base,
> + &radeon_connector->ddc_bus->adapter);
> + } else if (vga_switcheroo_handler_flags() &
> VGA_SWITCHEROO_CAN_SWITCH_DDC &&
> + connector->connector_type == DRM_MODE_CONNECTOR_LVDS) {
> + radeon_connector->edid =
> drm_get_edid_switcheroo(&radeon_connector->base,
> + &radeon_connector->ddc_bus->adapter);
> + } else {
> radeon_connector->edid =
> drm_get_edid(&radeon_connector->base,
> &radeon_connector->ddc_bus->adapter);
> - } else if (vga_switcheroo_handler_flags() &
> VGA_SWITCHEROO_CAN_SWITCH_DDC &&
> - connector->connector_type == DRM_MODE_CONNECTOR_LVDS &&
> - radeon_connector->ddc_bus) {
> - radeon_connector->edid =
> drm_get_edid_switcheroo(&radeon_connector->base,
> - &radeon_connector->ddc_bus->adapter);
> - } else if (radeon_connector->ddc_bus) {
> - radeon_connector->edid = drm_get_edid(&radeon_connector->base,
> - &radeon_connector->ddc_bus->adapter);
> + }
> }
>
> if (!radeon_connector->edid) {
> @@ -1312,6 +1313,7 @@ radeon_dvi_detect(struct drm_connector
> *connector, bool force)
> continue;
> list_radeon_connector =
> to_radeon_connector(list_connector);
> if (list_radeon_connector->shared_ddc &&
> + radeon_connector->ddc_bus &&
> (list_radeon_connector->ddc_bus->rec.i2c_id ==
> radeon_connector->ddc_bus->rec.i2c_id)) {
> /* cases where both connectors are digital */
> diff --git a/drivers/gpu/drm/radeon/radeon_dp_mst.c
> b/drivers/gpu/drm/radeon/radeon_dp_mst.c
> index de504ea..89e91f3 100644
> --- a/drivers/gpu/drm/radeon/radeon_dp_mst.c
> +++ b/drivers/gpu/drm/radeon/radeon_dp_mst.c
> @@ -661,7 +661,7 @@ radeon_dp_mst_init(struct radeon_connector
> *radeon_connector)
> {
> struct drm_device *dev = radeon_connector->base.dev;
>
> - if (!radeon_connector->ddc_bus->has_aux)
> + if (!radeon_connector->ddc_bus ||
> !radeon_connector->ddc_bus->has_aux)
> return 0;
>
> radeon_connector->mst_mgr.cbs = &mst_cbs;
> @@ -688,6 +688,9 @@ radeon_dp_mst_probe(struct radeon_connector
> *radeon_connector)
> if (dig_connector->dpcd[DP_DPCD_REV] < 0x12)
> return 0;
>
> + if (!radeon_connector->ddc_bus ||
> !radeon_connector->ddc_bus->has_aux)
> + return 0;
> +
> ret = drm_dp_dpcd_read(&radeon_connector->ddc_bus->aux,
> DP_MSTM_CAP, msg,
> 1);
> if (ret) {
> @@ -711,7 +714,9 @@ radeon_dp_mst_check_status(struct radeon_connector
> *radeon_connector)
> struct radeon_connector_atom_dig *dig_connector =
> radeon_connector->con_priv;
> int retry;
>
> - if (dig_connector->is_mst) {
> + if (dig_connector->is_mst &&
> + radeon_connector->ddc_bus &&
> + radeon_connector->ddc_bus->has_aux) {
> u8 esi[16] = { 0 };
> int dret;
> int ret = 0;
> diff --git a/drivers/gpu/drm/radeon/radeon_i2c.c
> b/drivers/gpu/drm/radeon/radeon_i2c.c
> index 9590bcd..c18f7ba 100644
> --- a/drivers/gpu/drm/radeon/radeon_i2c.c
> +++ b/drivers/gpu/drm/radeon/radeon_i2c.c
> @@ -63,6 +63,9 @@ bool radeon_ddc_probe(struct radeon_connector
> *radeon_connector, bool use_aux)
> if (radeon_connector->router.ddc_valid)
> radeon_router_select_ddc_port(radeon_connector);
>
> + if (!radeon_connector->ddc_bus)
> + return false;
> +
> if (use_aux) {
> ret = i2c_transfer(&radeon_connector->ddc_bus->aux.ddc,
> msgs, 2);
> } else {
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 001/001] drivers/gpu/radeon: NULL pointer deference workaround
2016-09-08 7:14 ` Christian König
@ 2016-09-08 9:09 ` Mark Fortescue
2016-09-08 10:25 ` Christian König
0 siblings, 1 reply; 8+ messages in thread
From: Mark Fortescue @ 2016-09-08 9:09 UTC (permalink / raw)
To: Christian König, David Airlie; +Cc: Alex Deucher, dri-devel, linux-kernel
Hi Christian,
Thank you for the feedback.
On 08/09/16 08:14, Christian König wrote:
> Am 07.09.2016 um 19:38 schrieb Mark Fortescue:
>>
>> On an LV-683 (AMD Dual-core G-T56N) Mini-ITX board, I get a Kernel
>> Oops because Connector 0 (LCD Panel interface) does not have DDC.
>
> I'm not an expert on this, but that is really odd cause even LCD Panels
> should have a DDC interface.
>
>>
>> Ubuntu 16.04 LTS Kernel (4.4 series):
>>
>> ...
>> [ 8.262990] [drm] ib test on ring 5 succeeded
>> [ 8.288897] [drm] Radeon Display Connectors
>> [ 8.293175] [drm] Connector 0:
>> [ 8.296252] [drm] DP-1
>
> Especially since the BIOS claims that this is a displayport connector
> and there is no physical way to have a DP without an DDC as far as I know.
>
> Please open a bug report on FDO and attach you BIOS image.
FDO ? I am not familiar with this. Please can you enlighten me.
I do not have a BIOS image so will need some assistance in understanding
what is required here and how I extract the BIOS information you are after.
On this motherboard, DP-1 is a single channel 18bit LVDS LCD panel
interface and DP-2 is a DVI interface (which I can connect to my monitor
if testing this is useful). There are no displayport connectors.
On industrial motherboards, I have noticed that it is not uncommon to
hard code the information for the LCD panel into the BIOS so no DDC is
required. In this case, there is no LCD panel connected to the interface
anyway.
See http://www.commell.com.tw/product/SBC/LV-683.HTM for more
information on the board. Looking at the web site, there is a BIOS image
available form Commell if that is of use.
>
> Alex can probably take a look when he's back from vacation.
>
> Regards,
> Christian.
>
>> [ 8.298791] [drm] Encoders:
>> [ 8.301770] [drm] DFP1: INTERNAL_UNIPHY
>> [ 8.305973] [drm] Connector 1:
>> [ 8.309043] [drm] DP-2
>> [ 8.311598] [drm] HPD2
>> [ 8.314169] [drm] DDC: 0x6440 0x6440 0x6444 0x6444 0x6448 0x6448
>> 0x644c 0x644c
>> [ 8.321609] [drm] Encoders:
>> [ 8.324589] [drm] DFP2: INTERNAL_UNIPHY
>> [ 8.328793] [drm] Connector 2:
>> [ 8.331856] [drm] VGA-1
>> [ 8.342947] [drm] DDC: 0x64d8 0x64d8 0x64dc 0x64dc 0x64e0 0x64e0
>> 0x64e4 0x64e4
>> [ 8.350341] [drm] Encoders:
>> [ 8.353310] [drm] CRT1: INTERNAL_KLDSCP_DAC1
>> [ 8.358195] BUG: unable to handle kernel NULL pointer dereference at
>> 0000000000000409
>> [ 8.409733] [<ffffffffc02024da>] radeon_dp_getsinktype+0x1a/0x30 [radeon]
>> [ 8.416805] PGD 0
>> [ 8.418841] Oops: 0000 [#1] SMP
>> ...
>>
>> This patch prevents Kernel failures due to a connector not having a
>> DDC interface by changing the code so that ddc_bus is always checked
>> before use.
>> The problem was first identified using the uBuntu MATE 14.04 LTS (3.16
>> series kernels) but not dealt with at that time. On attempting to
>> install uBuntu MATE 16.04 LTS (4.4 series kernels), it became clear
>> that using various workarounds to allow the issue to be ignored were
>> not viable so more effort was put in to sorting the issue resulting in
>> this patch. See https://bugs.launchpad.net/bugs/1587885 for more details.
>>
>> Signed-off-by: Mark Fortescue <mark@thurning-instruments.co.uk>
>> Tested-by: Mark Fortescue <mark@thurning-instruments.co.uk>
>>
>> ---
>>
>> Looks like Thunderbird may have made a mess of the patch when pasting
>> the contents into the mail message - my alternate mail client (pine)
>> also has strict line length handling and trashes non-MIME encoded
>> patches.
>>
>> This may not be the correct approach to solving the issue but it is
>> clean in that it ensures that ddc_bus is never used when NULL
>> regardless of how the code ended up at the point of use.
>>
>> If it helps with back porting, I have patches for the uBuntu 14.04 LTS
>> [3.13 series], uBuntu MATE 14.04 LTS [3.16 series] and uBuntu 16.04
>> LTS [4.4 series] kernels.
>>
>> Test Hardware:
>> Commell LV-683 Mini-ITX with onboard AMD Dual-core G-T56N
>> 4G Ram, 2x1TB Disk, HANNS-G HC194D 1280x1024 LCD (VGA).
>> 4.8.0-rc5 with patch boots without error.
>>
>> drivers/gpu/drm/radeon/atombios_dp.c | 60 ++++++++++++-------
>> drivers/gpu/drm/radeon/radeon_connectors.c | 46 +++++++-------
>> drivers/gpu/drm/radeon/radeon_dp_mst.c | 9 ++
>> drivers/gpu/drm/radeon/radeon_i2c.c | 3
>> 4 files changed, 73 insertions(+), 45 deletions(-)
>>
>> Patch:
>> diff --git a/drivers/gpu/drm/radeon/atombios_dp.c
>> b/drivers/gpu/drm/radeon/atombios_dp.c
>> index cead089a..98b3c0e 100644
>> --- a/drivers/gpu/drm/radeon/atombios_dp.c
>> +++ b/drivers/gpu/drm/radeon/atombios_dp.c
>> @@ -232,6 +232,9 @@ void radeon_dp_aux_init(struct radeon_connector
>> *radeon_connector)
>> struct radeon_device *rdev = dev->dev_private;
>> int ret;
>>
>> + if (!radeon_connector->ddc_bus)
>> + return;
>> +
>> radeon_connector->ddc_bus->rec.hpd = radeon_connector->hpd.hpd;
>> radeon_connector->ddc_bus->aux.dev = radeon_connector->base.kdev;
>> if (ASIC_IS_DCE5(rdev)) {
>> @@ -364,6 +367,9 @@ u8 radeon_dp_getsinktype(struct radeon_connector
>> *radeon_connector)
>> struct drm_device *dev = radeon_connector->base.dev;
>> struct radeon_device *rdev = dev->dev_private;
>>
>> + if (!radeon_connector->ddc_bus)
>> + return 0;
>> +
>> return radeon_dp_encoder_service(rdev,
>> ATOM_DP_ACTION_GET_SINK_TYPE, 0,
>> radeon_connector->ddc_bus->rec.i2c_id, 0);
>> }
>> @@ -376,6 +382,9 @@ static void radeon_dp_probe_oui(struct
>> radeon_connector *radeon_connector)
>> if (!(dig_connector->dpcd[DP_DOWN_STREAM_PORT_COUNT] &
>> DP_OUI_SUPPORT))
>> return;
>>
>> + if (!radeon_connector->ddc_bus)
>> + return;
>> +
>> if (drm_dp_dpcd_read(&radeon_connector->ddc_bus->aux,
>> DP_SINK_OUI, buf, 3) == 3)
>> DRM_DEBUG_KMS("Sink OUI: %02hx%02hx%02hx\n",
>> buf[0], buf[1], buf[2]);
>> @@ -391,6 +400,9 @@ bool radeon_dp_getdpcd(struct radeon_connector
>> *radeon_connector)
>> u8 msg[DP_DPCD_SIZE];
>> int ret, i;
>>
>> + if (!radeon_connector->ddc_bus)
>> + return false;
>> +
>> for (i = 0; i < 7; i++) {
>> ret = drm_dp_dpcd_read(&radeon_connector->ddc_bus->aux,
>> DP_DPCD_REV, msg,
>> DP_DPCD_SIZE);
>> @@ -428,24 +440,26 @@ int radeon_dp_get_panel_mode(struct drm_encoder
>> *encoder,
>>
>> dig_connector = radeon_connector->con_priv;
>>
>> - if (dp_bridge != ENCODER_OBJECT_ID_NONE) {
>> - /* DP bridge chips */
>> - if (drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux,
>> - DP_EDP_CONFIGURATION_CAP, &tmp) == 1) {
>> - if (tmp & 1)
>> - panel_mode = DP_PANEL_MODE_INTERNAL_DP2_MODE;
>> - else if ((dp_bridge == ENCODER_OBJECT_ID_NUTMEG) ||
>> - (dp_bridge == ENCODER_OBJECT_ID_TRAVIS))
>> - panel_mode = DP_PANEL_MODE_INTERNAL_DP1_MODE;
>> - else
>> - panel_mode = DP_PANEL_MODE_EXTERNAL_DP_MODE;
>> - }
>> - } else if (connector->connector_type == DRM_MODE_CONNECTOR_eDP) {
>> - /* eDP */
>> - if (drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux,
>> - DP_EDP_CONFIGURATION_CAP, &tmp) == 1) {
>> - if (tmp & 1)
>> - panel_mode = DP_PANEL_MODE_INTERNAL_DP2_MODE;
>> + if (radeon_connector->ddc_bus) {
>> + if (dp_bridge != ENCODER_OBJECT_ID_NONE) {
>> + /* DP bridge chips */
>> + if (drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux,
>> + DP_EDP_CONFIGURATION_CAP, &tmp) == 1) {
>> + if (tmp & 1)
>> + panel_mode = DP_PANEL_MODE_INTERNAL_DP2_MODE;
>> + else if ((dp_bridge == ENCODER_OBJECT_ID_NUTMEG) ||
>> + (dp_bridge == ENCODER_OBJECT_ID_TRAVIS))
>> + panel_mode = DP_PANEL_MODE_INTERNAL_DP1_MODE;
>> + else
>> + panel_mode = DP_PANEL_MODE_EXTERNAL_DP_MODE;
>> + }
>> + } else if (connector->connector_type ==
>> DRM_MODE_CONNECTOR_eDP) {
>> + /* eDP */
>> + if (drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux,
>> + DP_EDP_CONFIGURATION_CAP, &tmp) == 1) {
>> + if (tmp & 1)
>> + panel_mode = DP_PANEL_MODE_INTERNAL_DP2_MODE;
>> + }
>> }
>> }
>>
>> @@ -511,6 +525,9 @@ bool radeon_dp_needs_link_train(struct
>> radeon_connector *radeon_connector)
>> u8 link_status[DP_LINK_STATUS_SIZE];
>> struct radeon_connector_atom_dig *dig = radeon_connector->con_priv;
>>
>> + if (!radeon_connector->ddc_bus)
>> + return false;
>> +
>> if
>> (drm_dp_dpcd_read_link_status(&radeon_connector->ddc_bus->aux,
>> link_status)
>> <= 0)
>> return false;
>> @@ -531,7 +548,7 @@ void radeon_dp_set_rx_power_state(struct
>> drm_connector *connector,
>> dig_connector = radeon_connector->con_priv;
>>
>> /* power up/down the sink */
>> - if (dig_connector->dpcd[0] >= 0x11) {
>> + if (radeon_connector->ddc_bus && dig_connector->dpcd[0] >= 0x11) {
>> drm_dp_dpcd_writeb(&radeon_connector->ddc_bus->aux,
>> DP_SET_POWER, power_state);
>> usleep_range(1000, 2000);
>> @@ -834,7 +851,8 @@ void radeon_dp_link_train(struct drm_encoder
>> *encoder,
>> else
>> dp_info.enc_id |= ATOM_DP_CONFIG_LINK_A;
>>
>> - if (drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux,
>> DP_MAX_LANE_COUNT, &tmp)
>> + if (radeon_connector->ddc_bus &&
>> + drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux,
>> DP_MAX_LANE_COUNT, &tmp)
>> == 1) {
>> if (ASIC_IS_DCE5(rdev) && (tmp & DP_TPS3_SUPPORTED))
>> dp_info.tp3_supported = true;
>> @@ -850,7 +868,7 @@ void radeon_dp_link_train(struct drm_encoder
>> *encoder,
>> dp_info.connector = connector;
>> dp_info.dp_lane_count = dig_connector->dp_lane_count;
>> dp_info.dp_clock = dig_connector->dp_clock;
>> - dp_info.aux = &radeon_connector->ddc_bus->aux;
>> + dp_info.aux = radeon_connector->ddc_bus ?
>> &radeon_connector->ddc_bus->aux : 0;
>>
>> if (radeon_dp_link_train_init(&dp_info))
>> goto done;
>> diff --git a/drivers/gpu/drm/radeon/radeon_connectors.c
>> b/drivers/gpu/drm/radeon/radeon_connectors.c
>> index b79f3b0..cec30c9 100644
>> --- a/drivers/gpu/drm/radeon/radeon_connectors.c
>> +++ b/drivers/gpu/drm/radeon/radeon_connectors.c
>> @@ -328,31 +328,32 @@ static void radeon_connector_get_edid(struct
>> drm_connector *connector)
>> if (radeon_connector->router.ddc_valid)
>> radeon_router_select_ddc_port(radeon_connector);
>>
>> - if ((radeon_connector_encoder_get_dp_bridge_encoder_id(connector) !=
>> - ENCODER_OBJECT_ID_NONE) &&
>> - radeon_connector->ddc_bus->has_aux) {
>> - radeon_connector->edid = drm_get_edid(connector,
>> - &radeon_connector->ddc_bus->aux.ddc);
>> - } else if ((connector->connector_type ==
>> DRM_MODE_CONNECTOR_DisplayPort) ||
>> - (connector->connector_type == DRM_MODE_CONNECTOR_eDP)) {
>> - struct radeon_connector_atom_dig *dig =
>> radeon_connector->con_priv;
>> -
>> - if ((dig->dp_sink_type == CONNECTOR_OBJECT_ID_DISPLAYPORT ||
>> - dig->dp_sink_type == CONNECTOR_OBJECT_ID_eDP) &&
>> - radeon_connector->ddc_bus->has_aux)
>> - radeon_connector->edid =
>> drm_get_edid(&radeon_connector->base,
>> + if (radeon_connector->ddc_bus) {
>> + if
>> ((radeon_connector_encoder_get_dp_bridge_encoder_id(connector) !=
>> + ENCODER_OBJECT_ID_NONE) &&
>> + radeon_connector->ddc_bus->has_aux) {
>> + radeon_connector->edid = drm_get_edid(connector,
>> &radeon_connector->ddc_bus->aux.ddc);
>> - else if (radeon_connector->ddc_bus)
>> + } else if ((connector->connector_type ==
>> DRM_MODE_CONNECTOR_DisplayPort) ||
>> + (connector->connector_type == DRM_MODE_CONNECTOR_eDP)) {
>> + struct radeon_connector_atom_dig *dig =
>> radeon_connector->con_priv;
>> +
>> + if ((dig->dp_sink_type == CONNECTOR_OBJECT_ID_DISPLAYPORT ||
>> + dig->dp_sink_type == CONNECTOR_OBJECT_ID_eDP) &&
>> + radeon_connector->ddc_bus->has_aux)
>> + radeon_connector->edid =
>> drm_get_edid(&radeon_connector->base,
>> + &radeon_connector->ddc_bus->aux.ddc);
>> + else
>> + radeon_connector->edid =
>> drm_get_edid(&radeon_connector->base,
>> + &radeon_connector->ddc_bus->adapter);
>> + } else if (vga_switcheroo_handler_flags() &
>> VGA_SWITCHEROO_CAN_SWITCH_DDC &&
>> + connector->connector_type == DRM_MODE_CONNECTOR_LVDS) {
>> + radeon_connector->edid =
>> drm_get_edid_switcheroo(&radeon_connector->base,
>> + &radeon_connector->ddc_bus->adapter);
>> + } else {
>> radeon_connector->edid =
>> drm_get_edid(&radeon_connector->base,
>> &radeon_connector->ddc_bus->adapter);
>> - } else if (vga_switcheroo_handler_flags() &
>> VGA_SWITCHEROO_CAN_SWITCH_DDC &&
>> - connector->connector_type == DRM_MODE_CONNECTOR_LVDS &&
>> - radeon_connector->ddc_bus) {
>> - radeon_connector->edid =
>> drm_get_edid_switcheroo(&radeon_connector->base,
>> - &radeon_connector->ddc_bus->adapter);
>> - } else if (radeon_connector->ddc_bus) {
>> - radeon_connector->edid = drm_get_edid(&radeon_connector->base,
>> - &radeon_connector->ddc_bus->adapter);
>> + }
>> }
>>
>> if (!radeon_connector->edid) {
>> @@ -1312,6 +1313,7 @@ radeon_dvi_detect(struct drm_connector
>> *connector, bool force)
>> continue;
>> list_radeon_connector =
>> to_radeon_connector(list_connector);
>> if (list_radeon_connector->shared_ddc &&
>> + radeon_connector->ddc_bus &&
>> (list_radeon_connector->ddc_bus->rec.i2c_id ==
>> radeon_connector->ddc_bus->rec.i2c_id)) {
>> /* cases where both connectors are digital */
>> diff --git a/drivers/gpu/drm/radeon/radeon_dp_mst.c
>> b/drivers/gpu/drm/radeon/radeon_dp_mst.c
>> index de504ea..89e91f3 100644
>> --- a/drivers/gpu/drm/radeon/radeon_dp_mst.c
>> +++ b/drivers/gpu/drm/radeon/radeon_dp_mst.c
>> @@ -661,7 +661,7 @@ radeon_dp_mst_init(struct radeon_connector
>> *radeon_connector)
>> {
>> struct drm_device *dev = radeon_connector->base.dev;
>>
>> - if (!radeon_connector->ddc_bus->has_aux)
>> + if (!radeon_connector->ddc_bus ||
>> !radeon_connector->ddc_bus->has_aux)
>> return 0;
>>
>> radeon_connector->mst_mgr.cbs = &mst_cbs;
>> @@ -688,6 +688,9 @@ radeon_dp_mst_probe(struct radeon_connector
>> *radeon_connector)
>> if (dig_connector->dpcd[DP_DPCD_REV] < 0x12)
>> return 0;
>>
>> + if (!radeon_connector->ddc_bus ||
>> !radeon_connector->ddc_bus->has_aux)
>> + return 0;
>> +
>> ret = drm_dp_dpcd_read(&radeon_connector->ddc_bus->aux,
>> DP_MSTM_CAP, msg,
>> 1);
>> if (ret) {
>> @@ -711,7 +714,9 @@ radeon_dp_mst_check_status(struct radeon_connector
>> *radeon_connector)
>> struct radeon_connector_atom_dig *dig_connector =
>> radeon_connector->con_priv;
>> int retry;
>>
>> - if (dig_connector->is_mst) {
>> + if (dig_connector->is_mst &&
>> + radeon_connector->ddc_bus &&
>> + radeon_connector->ddc_bus->has_aux) {
>> u8 esi[16] = { 0 };
>> int dret;
>> int ret = 0;
>> diff --git a/drivers/gpu/drm/radeon/radeon_i2c.c
>> b/drivers/gpu/drm/radeon/radeon_i2c.c
>> index 9590bcd..c18f7ba 100644
>> --- a/drivers/gpu/drm/radeon/radeon_i2c.c
>> +++ b/drivers/gpu/drm/radeon/radeon_i2c.c
>> @@ -63,6 +63,9 @@ bool radeon_ddc_probe(struct radeon_connector
>> *radeon_connector, bool use_aux)
>> if (radeon_connector->router.ddc_valid)
>> radeon_router_select_ddc_port(radeon_connector);
>>
>> + if (!radeon_connector->ddc_bus)
>> + return false;
>> +
>> if (use_aux) {
>> ret = i2c_transfer(&radeon_connector->ddc_bus->aux.ddc,
>> msgs, 2);
>> } else {
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 001/001] drivers/gpu/radeon: NULL pointer deference workaround
2016-09-08 9:09 ` Mark Fortescue
@ 2016-09-08 10:25 ` Christian König
2016-09-08 11:18 ` Mark Fortescue
0 siblings, 1 reply; 8+ messages in thread
From: Christian König @ 2016-09-08 10:25 UTC (permalink / raw)
To: Mark Fortescue, David Airlie; +Cc: Alex Deucher, dri-devel, linux-kernel
Am 08.09.2016 um 11:09 schrieb Mark Fortescue:
> Hi Christian,
>
> Thank you for the feedback.
>
> On 08/09/16 08:14, Christian König wrote:
>> Am 07.09.2016 um 19:38 schrieb Mark Fortescue:
>>>
>>> On an LV-683 (AMD Dual-core G-T56N) Mini-ITX board, I get a Kernel
>>> Oops because Connector 0 (LCD Panel interface) does not have DDC.
>>
>> I'm not an expert on this, but that is really odd cause even LCD Panels
>> should have a DDC interface.
>>
>>>
>>> Ubuntu 16.04 LTS Kernel (4.4 series):
>>>
>>> ...
>>> [ 8.262990] [drm] ib test on ring 5 succeeded
>>> [ 8.288897] [drm] Radeon Display Connectors
>>> [ 8.293175] [drm] Connector 0:
>>> [ 8.296252] [drm] DP-1
>>
>> Especially since the BIOS claims that this is a displayport connector
>> and there is no physical way to have a DP without an DDC as far as I
>> know.
>>
>> Please open a bug report on FDO and attach you BIOS image.
>
> FDO ? I am not familiar with this. Please can you enlighten me.
>
See here: http://bugs.freedesktop.org/
> I do not have a BIOS image so will need some assistance in
> understanding what is required here and how I extract the BIOS
> information you are after.
>
Sorry my fault. Mullins is an APU, so you don't have a dedicated video
BIOS. As usually I didn't got enough sleep :) But please open up a bug
report anyway.
> On this motherboard, DP-1 is a single channel 18bit LVDS LCD panel
> interface and DP-2 is a DVI interface (which I can connect to my
> monitor if testing this is useful). There are no displayport connectors.
Yeah, but from the driver point of view there are only DP connectors on
the chip. The LVDS and DVI are probably realized with external DP to
whatever converter ICs.
>
> On industrial motherboards, I have noticed that it is not uncommon to
> hard code the information for the LCD panel into the BIOS so no DDC is
> required. In this case, there is no LCD panel connected to the
> interface anyway.
>
That is correct and as far as I know well supported by Radeon, but the
crux is there should still be a DDC channel even if there isn't anything
attached to it.
See with displayport you got four LVDS channels to submit the actual
picture and an AUX channel to configure and query the device. The DDC is
just represented as certain packets over the AUX channel.
If the AUX channel doesn't work or isn't connect then the link training
wouldn't be possible as well and so you wouldn't be able to get any
picture on the LVDS.
> See http://www.commell.com.tw/product/SBC/LV-683.HTM for more
> information on the board. Looking at the web site, there is a BIOS
> image available form Commell if that is of use.
Alex clearly needs to take a look on this. I think for the time being
you could hack together a patch which just ignores DP connectors during
probing if they don't have an associated DDC instead of changing the
code everywhere the DDC object is required.
Regards,
Christian.
>
>>
>> Alex can probably take a look when he's back from vacation.
>>
>> Regards,
>> Christian.
>>
>>> [ 8.298791] [drm] Encoders:
>>> [ 8.301770] [drm] DFP1: INTERNAL_UNIPHY
>>> [ 8.305973] [drm] Connector 1:
>>> [ 8.309043] [drm] DP-2
>>> [ 8.311598] [drm] HPD2
>>> [ 8.314169] [drm] DDC: 0x6440 0x6440 0x6444 0x6444 0x6448 0x6448
>>> 0x644c 0x644c
>>> [ 8.321609] [drm] Encoders:
>>> [ 8.324589] [drm] DFP2: INTERNAL_UNIPHY
>>> [ 8.328793] [drm] Connector 2:
>>> [ 8.331856] [drm] VGA-1
>>> [ 8.342947] [drm] DDC: 0x64d8 0x64d8 0x64dc 0x64dc 0x64e0 0x64e0
>>> 0x64e4 0x64e4
>>> [ 8.350341] [drm] Encoders:
>>> [ 8.353310] [drm] CRT1: INTERNAL_KLDSCP_DAC1
>>> [ 8.358195] BUG: unable to handle kernel NULL pointer dereference at
>>> 0000000000000409
>>> [ 8.409733] [<ffffffffc02024da>] radeon_dp_getsinktype+0x1a/0x30
>>> [radeon]
>>> [ 8.416805] PGD 0
>>> [ 8.418841] Oops: 0000 [#1] SMP
>>> ...
>>>
>>> This patch prevents Kernel failures due to a connector not having a
>>> DDC interface by changing the code so that ddc_bus is always checked
>>> before use.
>>> The problem was first identified using the uBuntu MATE 14.04 LTS (3.16
>>> series kernels) but not dealt with at that time. On attempting to
>>> install uBuntu MATE 16.04 LTS (4.4 series kernels), it became clear
>>> that using various workarounds to allow the issue to be ignored were
>>> not viable so more effort was put in to sorting the issue resulting in
>>> this patch. See https://bugs.launchpad.net/bugs/1587885 for more
>>> details.
>>>
>>> Signed-off-by: Mark Fortescue <mark@thurning-instruments.co.uk>
>>> Tested-by: Mark Fortescue <mark@thurning-instruments.co.uk>
>>>
>>> ---
>>>
>>> Looks like Thunderbird may have made a mess of the patch when pasting
>>> the contents into the mail message - my alternate mail client (pine)
>>> also has strict line length handling and trashes non-MIME encoded
>>> patches.
>>>
>>> This may not be the correct approach to solving the issue but it is
>>> clean in that it ensures that ddc_bus is never used when NULL
>>> regardless of how the code ended up at the point of use.
>>>
>>> If it helps with back porting, I have patches for the uBuntu 14.04 LTS
>>> [3.13 series], uBuntu MATE 14.04 LTS [3.16 series] and uBuntu 16.04
>>> LTS [4.4 series] kernels.
>>>
>>> Test Hardware:
>>> Commell LV-683 Mini-ITX with onboard AMD Dual-core G-T56N
>>> 4G Ram, 2x1TB Disk, HANNS-G HC194D 1280x1024 LCD (VGA).
>>> 4.8.0-rc5 with patch boots without error.
>>>
>>> drivers/gpu/drm/radeon/atombios_dp.c | 60 ++++++++++++-------
>>> drivers/gpu/drm/radeon/radeon_connectors.c | 46 +++++++-------
>>> drivers/gpu/drm/radeon/radeon_dp_mst.c | 9 ++
>>> drivers/gpu/drm/radeon/radeon_i2c.c | 3
>>> 4 files changed, 73 insertions(+), 45 deletions(-)
>>>
>>> Patch:
>>> diff --git a/drivers/gpu/drm/radeon/atombios_dp.c
>>> b/drivers/gpu/drm/radeon/atombios_dp.c
>>> index cead089a..98b3c0e 100644
>>> --- a/drivers/gpu/drm/radeon/atombios_dp.c
>>> +++ b/drivers/gpu/drm/radeon/atombios_dp.c
>>> @@ -232,6 +232,9 @@ void radeon_dp_aux_init(struct radeon_connector
>>> *radeon_connector)
>>> struct radeon_device *rdev = dev->dev_private;
>>> int ret;
>>>
>>> + if (!radeon_connector->ddc_bus)
>>> + return;
>>> +
>>> radeon_connector->ddc_bus->rec.hpd = radeon_connector->hpd.hpd;
>>> radeon_connector->ddc_bus->aux.dev = radeon_connector->base.kdev;
>>> if (ASIC_IS_DCE5(rdev)) {
>>> @@ -364,6 +367,9 @@ u8 radeon_dp_getsinktype(struct radeon_connector
>>> *radeon_connector)
>>> struct drm_device *dev = radeon_connector->base.dev;
>>> struct radeon_device *rdev = dev->dev_private;
>>>
>>> + if (!radeon_connector->ddc_bus)
>>> + return 0;
>>> +
>>> return radeon_dp_encoder_service(rdev,
>>> ATOM_DP_ACTION_GET_SINK_TYPE, 0,
>>> radeon_connector->ddc_bus->rec.i2c_id, 0);
>>> }
>>> @@ -376,6 +382,9 @@ static void radeon_dp_probe_oui(struct
>>> radeon_connector *radeon_connector)
>>> if (!(dig_connector->dpcd[DP_DOWN_STREAM_PORT_COUNT] &
>>> DP_OUI_SUPPORT))
>>> return;
>>>
>>> + if (!radeon_connector->ddc_bus)
>>> + return;
>>> +
>>> if (drm_dp_dpcd_read(&radeon_connector->ddc_bus->aux,
>>> DP_SINK_OUI, buf, 3) == 3)
>>> DRM_DEBUG_KMS("Sink OUI: %02hx%02hx%02hx\n",
>>> buf[0], buf[1], buf[2]);
>>> @@ -391,6 +400,9 @@ bool radeon_dp_getdpcd(struct radeon_connector
>>> *radeon_connector)
>>> u8 msg[DP_DPCD_SIZE];
>>> int ret, i;
>>>
>>> + if (!radeon_connector->ddc_bus)
>>> + return false;
>>> +
>>> for (i = 0; i < 7; i++) {
>>> ret = drm_dp_dpcd_read(&radeon_connector->ddc_bus->aux,
>>> DP_DPCD_REV, msg,
>>> DP_DPCD_SIZE);
>>> @@ -428,24 +440,26 @@ int radeon_dp_get_panel_mode(struct drm_encoder
>>> *encoder,
>>>
>>> dig_connector = radeon_connector->con_priv;
>>>
>>> - if (dp_bridge != ENCODER_OBJECT_ID_NONE) {
>>> - /* DP bridge chips */
>>> - if (drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux,
>>> - DP_EDP_CONFIGURATION_CAP, &tmp) == 1) {
>>> - if (tmp & 1)
>>> - panel_mode = DP_PANEL_MODE_INTERNAL_DP2_MODE;
>>> - else if ((dp_bridge == ENCODER_OBJECT_ID_NUTMEG) ||
>>> - (dp_bridge == ENCODER_OBJECT_ID_TRAVIS))
>>> - panel_mode = DP_PANEL_MODE_INTERNAL_DP1_MODE;
>>> - else
>>> - panel_mode = DP_PANEL_MODE_EXTERNAL_DP_MODE;
>>> - }
>>> - } else if (connector->connector_type == DRM_MODE_CONNECTOR_eDP) {
>>> - /* eDP */
>>> - if (drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux,
>>> - DP_EDP_CONFIGURATION_CAP, &tmp) == 1) {
>>> - if (tmp & 1)
>>> - panel_mode = DP_PANEL_MODE_INTERNAL_DP2_MODE;
>>> + if (radeon_connector->ddc_bus) {
>>> + if (dp_bridge != ENCODER_OBJECT_ID_NONE) {
>>> + /* DP bridge chips */
>>> + if (drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux,
>>> + DP_EDP_CONFIGURATION_CAP, &tmp) == 1) {
>>> + if (tmp & 1)
>>> + panel_mode = DP_PANEL_MODE_INTERNAL_DP2_MODE;
>>> + else if ((dp_bridge == ENCODER_OBJECT_ID_NUTMEG) ||
>>> + (dp_bridge == ENCODER_OBJECT_ID_TRAVIS))
>>> + panel_mode = DP_PANEL_MODE_INTERNAL_DP1_MODE;
>>> + else
>>> + panel_mode = DP_PANEL_MODE_EXTERNAL_DP_MODE;
>>> + }
>>> + } else if (connector->connector_type ==
>>> DRM_MODE_CONNECTOR_eDP) {
>>> + /* eDP */
>>> + if (drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux,
>>> + DP_EDP_CONFIGURATION_CAP, &tmp) == 1) {
>>> + if (tmp & 1)
>>> + panel_mode = DP_PANEL_MODE_INTERNAL_DP2_MODE;
>>> + }
>>> }
>>> }
>>>
>>> @@ -511,6 +525,9 @@ bool radeon_dp_needs_link_train(struct
>>> radeon_connector *radeon_connector)
>>> u8 link_status[DP_LINK_STATUS_SIZE];
>>> struct radeon_connector_atom_dig *dig =
>>> radeon_connector->con_priv;
>>>
>>> + if (!radeon_connector->ddc_bus)
>>> + return false;
>>> +
>>> if
>>> (drm_dp_dpcd_read_link_status(&radeon_connector->ddc_bus->aux,
>>> link_status)
>>> <= 0)
>>> return false;
>>> @@ -531,7 +548,7 @@ void radeon_dp_set_rx_power_state(struct
>>> drm_connector *connector,
>>> dig_connector = radeon_connector->con_priv;
>>>
>>> /* power up/down the sink */
>>> - if (dig_connector->dpcd[0] >= 0x11) {
>>> + if (radeon_connector->ddc_bus && dig_connector->dpcd[0] >= 0x11) {
>>> drm_dp_dpcd_writeb(&radeon_connector->ddc_bus->aux,
>>> DP_SET_POWER, power_state);
>>> usleep_range(1000, 2000);
>>> @@ -834,7 +851,8 @@ void radeon_dp_link_train(struct drm_encoder
>>> *encoder,
>>> else
>>> dp_info.enc_id |= ATOM_DP_CONFIG_LINK_A;
>>>
>>> - if (drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux,
>>> DP_MAX_LANE_COUNT, &tmp)
>>> + if (radeon_connector->ddc_bus &&
>>> + drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux,
>>> DP_MAX_LANE_COUNT, &tmp)
>>> == 1) {
>>> if (ASIC_IS_DCE5(rdev) && (tmp & DP_TPS3_SUPPORTED))
>>> dp_info.tp3_supported = true;
>>> @@ -850,7 +868,7 @@ void radeon_dp_link_train(struct drm_encoder
>>> *encoder,
>>> dp_info.connector = connector;
>>> dp_info.dp_lane_count = dig_connector->dp_lane_count;
>>> dp_info.dp_clock = dig_connector->dp_clock;
>>> - dp_info.aux = &radeon_connector->ddc_bus->aux;
>>> + dp_info.aux = radeon_connector->ddc_bus ?
>>> &radeon_connector->ddc_bus->aux : 0;
>>>
>>> if (radeon_dp_link_train_init(&dp_info))
>>> goto done;
>>> diff --git a/drivers/gpu/drm/radeon/radeon_connectors.c
>>> b/drivers/gpu/drm/radeon/radeon_connectors.c
>>> index b79f3b0..cec30c9 100644
>>> --- a/drivers/gpu/drm/radeon/radeon_connectors.c
>>> +++ b/drivers/gpu/drm/radeon/radeon_connectors.c
>>> @@ -328,31 +328,32 @@ static void radeon_connector_get_edid(struct
>>> drm_connector *connector)
>>> if (radeon_connector->router.ddc_valid)
>>> radeon_router_select_ddc_port(radeon_connector);
>>>
>>> - if
>>> ((radeon_connector_encoder_get_dp_bridge_encoder_id(connector) !=
>>> - ENCODER_OBJECT_ID_NONE) &&
>>> - radeon_connector->ddc_bus->has_aux) {
>>> - radeon_connector->edid = drm_get_edid(connector,
>>> - &radeon_connector->ddc_bus->aux.ddc);
>>> - } else if ((connector->connector_type ==
>>> DRM_MODE_CONNECTOR_DisplayPort) ||
>>> - (connector->connector_type == DRM_MODE_CONNECTOR_eDP)) {
>>> - struct radeon_connector_atom_dig *dig =
>>> radeon_connector->con_priv;
>>> -
>>> - if ((dig->dp_sink_type == CONNECTOR_OBJECT_ID_DISPLAYPORT ||
>>> - dig->dp_sink_type == CONNECTOR_OBJECT_ID_eDP) &&
>>> - radeon_connector->ddc_bus->has_aux)
>>> - radeon_connector->edid =
>>> drm_get_edid(&radeon_connector->base,
>>> + if (radeon_connector->ddc_bus) {
>>> + if
>>> ((radeon_connector_encoder_get_dp_bridge_encoder_id(connector) !=
>>> + ENCODER_OBJECT_ID_NONE) &&
>>> + radeon_connector->ddc_bus->has_aux) {
>>> + radeon_connector->edid = drm_get_edid(connector,
>>> &radeon_connector->ddc_bus->aux.ddc);
>>> - else if (radeon_connector->ddc_bus)
>>> + } else if ((connector->connector_type ==
>>> DRM_MODE_CONNECTOR_DisplayPort) ||
>>> + (connector->connector_type ==
>>> DRM_MODE_CONNECTOR_eDP)) {
>>> + struct radeon_connector_atom_dig *dig =
>>> radeon_connector->con_priv;
>>> +
>>> + if ((dig->dp_sink_type ==
>>> CONNECTOR_OBJECT_ID_DISPLAYPORT ||
>>> + dig->dp_sink_type == CONNECTOR_OBJECT_ID_eDP) &&
>>> + radeon_connector->ddc_bus->has_aux)
>>> + radeon_connector->edid =
>>> drm_get_edid(&radeon_connector->base,
>>> + &radeon_connector->ddc_bus->aux.ddc);
>>> + else
>>> + radeon_connector->edid =
>>> drm_get_edid(&radeon_connector->base,
>>> + &radeon_connector->ddc_bus->adapter);
>>> + } else if (vga_switcheroo_handler_flags() &
>>> VGA_SWITCHEROO_CAN_SWITCH_DDC &&
>>> + connector->connector_type == DRM_MODE_CONNECTOR_LVDS) {
>>> + radeon_connector->edid =
>>> drm_get_edid_switcheroo(&radeon_connector->base,
>>> + &radeon_connector->ddc_bus->adapter);
>>> + } else {
>>> radeon_connector->edid =
>>> drm_get_edid(&radeon_connector->base,
>>> &radeon_connector->ddc_bus->adapter);
>>> - } else if (vga_switcheroo_handler_flags() &
>>> VGA_SWITCHEROO_CAN_SWITCH_DDC &&
>>> - connector->connector_type == DRM_MODE_CONNECTOR_LVDS &&
>>> - radeon_connector->ddc_bus) {
>>> - radeon_connector->edid =
>>> drm_get_edid_switcheroo(&radeon_connector->base,
>>> - &radeon_connector->ddc_bus->adapter);
>>> - } else if (radeon_connector->ddc_bus) {
>>> - radeon_connector->edid = drm_get_edid(&radeon_connector->base,
>>> - &radeon_connector->ddc_bus->adapter);
>>> + }
>>> }
>>>
>>> if (!radeon_connector->edid) {
>>> @@ -1312,6 +1313,7 @@ radeon_dvi_detect(struct drm_connector
>>> *connector, bool force)
>>> continue;
>>> list_radeon_connector =
>>> to_radeon_connector(list_connector);
>>> if (list_radeon_connector->shared_ddc &&
>>> + radeon_connector->ddc_bus &&
>>> (list_radeon_connector->ddc_bus->rec.i2c_id ==
>>> radeon_connector->ddc_bus->rec.i2c_id)) {
>>> /* cases where both connectors are digital */
>>> diff --git a/drivers/gpu/drm/radeon/radeon_dp_mst.c
>>> b/drivers/gpu/drm/radeon/radeon_dp_mst.c
>>> index de504ea..89e91f3 100644
>>> --- a/drivers/gpu/drm/radeon/radeon_dp_mst.c
>>> +++ b/drivers/gpu/drm/radeon/radeon_dp_mst.c
>>> @@ -661,7 +661,7 @@ radeon_dp_mst_init(struct radeon_connector
>>> *radeon_connector)
>>> {
>>> struct drm_device *dev = radeon_connector->base.dev;
>>>
>>> - if (!radeon_connector->ddc_bus->has_aux)
>>> + if (!radeon_connector->ddc_bus ||
>>> !radeon_connector->ddc_bus->has_aux)
>>> return 0;
>>>
>>> radeon_connector->mst_mgr.cbs = &mst_cbs;
>>> @@ -688,6 +688,9 @@ radeon_dp_mst_probe(struct radeon_connector
>>> *radeon_connector)
>>> if (dig_connector->dpcd[DP_DPCD_REV] < 0x12)
>>> return 0;
>>>
>>> + if (!radeon_connector->ddc_bus ||
>>> !radeon_connector->ddc_bus->has_aux)
>>> + return 0;
>>> +
>>> ret = drm_dp_dpcd_read(&radeon_connector->ddc_bus->aux,
>>> DP_MSTM_CAP, msg,
>>> 1);
>>> if (ret) {
>>> @@ -711,7 +714,9 @@ radeon_dp_mst_check_status(struct radeon_connector
>>> *radeon_connector)
>>> struct radeon_connector_atom_dig *dig_connector =
>>> radeon_connector->con_priv;
>>> int retry;
>>>
>>> - if (dig_connector->is_mst) {
>>> + if (dig_connector->is_mst &&
>>> + radeon_connector->ddc_bus &&
>>> + radeon_connector->ddc_bus->has_aux) {
>>> u8 esi[16] = { 0 };
>>> int dret;
>>> int ret = 0;
>>> diff --git a/drivers/gpu/drm/radeon/radeon_i2c.c
>>> b/drivers/gpu/drm/radeon/radeon_i2c.c
>>> index 9590bcd..c18f7ba 100644
>>> --- a/drivers/gpu/drm/radeon/radeon_i2c.c
>>> +++ b/drivers/gpu/drm/radeon/radeon_i2c.c
>>> @@ -63,6 +63,9 @@ bool radeon_ddc_probe(struct radeon_connector
>>> *radeon_connector, bool use_aux)
>>> if (radeon_connector->router.ddc_valid)
>>> radeon_router_select_ddc_port(radeon_connector);
>>>
>>> + if (!radeon_connector->ddc_bus)
>>> + return false;
>>> +
>>> if (use_aux) {
>>> ret = i2c_transfer(&radeon_connector->ddc_bus->aux.ddc,
>>> msgs, 2);
>>> } else {
>>> _______________________________________________
>>> dri-devel mailing list
>>> dri-devel@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 001/001] drivers/gpu/radeon: NULL pointer deference workaround
2016-09-08 10:25 ` Christian König
@ 2016-09-08 11:18 ` Mark Fortescue
2016-09-09 8:15 ` Christian König
0 siblings, 1 reply; 8+ messages in thread
From: Mark Fortescue @ 2016-09-08 11:18 UTC (permalink / raw)
To: Christian König, David Airlie; +Cc: Alex Deucher, dri-devel, linux-kernel
On 08/09/16 11:25, Christian König wrote:
> Am 08.09.2016 um 11:09 schrieb Mark Fortescue:
>> Hi Christian,
>>
>> Thank you for the feedback.
>>
>> On 08/09/16 08:14, Christian König wrote:
>>> Am 07.09.2016 um 19:38 schrieb Mark Fortescue:
>>>>
>>>> On an LV-683 (AMD Dual-core G-T56N) Mini-ITX board, I get a Kernel
>>>> Oops because Connector 0 (LCD Panel interface) does not have DDC.
>>>
>>> I'm not an expert on this, but that is really odd cause even LCD Panels
>>> should have a DDC interface.
>>>
>>>>
>>>> Ubuntu 16.04 LTS Kernel (4.4 series):
>>>>
>>>> ...
>>>> [ 8.262990] [drm] ib test on ring 5 succeeded
>>>> [ 8.288897] [drm] Radeon Display Connectors
>>>> [ 8.293175] [drm] Connector 0:
>>>> [ 8.296252] [drm] DP-1
>>>
>>> Especially since the BIOS claims that this is a displayport connector
>>> and there is no physical way to have a DP without an DDC as far as I
>>> know.
>>>
>>> Please open a bug report on FDO and attach you BIOS image.
>>
>> FDO ? I am not familiar with this. Please can you enlighten me.
>>
>
> See here: http://bugs.freedesktop.org/
>
>> I do not have a BIOS image so will need some assistance in
>> understanding what is required here and how I extract the BIOS
>> information you are after.
>>
>
> Sorry my fault. Mullins is an APU, so you don't have a dedicated video
> BIOS. As usually I didn't got enough sleep :) But please open up a bug
> report anyway.
Know the problem of being awake too long :). I will raise a bug.
>
>> On this motherboard, DP-1 is a single channel 18bit LVDS LCD panel
>> interface and DP-2 is a DVI interface (which I can connect to my
>> monitor if testing this is useful). There are no displayport connectors.
>
> Yeah, but from the driver point of view there are only DP connectors on
> the chip. The LVDS and DVI are probably realized with external DP to
> whatever converter ICs.
>
That would explain why some similar boards have 24bit LVDS and others
only 18bit LVDS.
>>
>> On industrial motherboards, I have noticed that it is not uncommon to
>> hard code the information for the LCD panel into the BIOS so no DDC is
>> required. In this case, there is no LCD panel connected to the
>> interface anyway.
>>
>
> That is correct and as far as I know well supported by Radeon, but the
> crux is there should still be a DDC channel even if there isn't anything
> attached to it.
>
> See with displayport you got four LVDS channels to submit the actual
> picture and an AUX channel to configure and query the device. The DDC is
> just represented as certain packets over the AUX channel.
>
> If the AUX channel doesn't work or isn't connect then the link training
> wouldn't be possible as well and so you wouldn't be able to get any
> picture on the LVDS.
>
Interesting.
>> See http://www.commell.com.tw/product/SBC/LV-683.HTM for more
>> information on the board. Looking at the web site, there is a BIOS
>> image available form Commell if that is of use.
>
> Alex clearly needs to take a look on this. I think for the time being
> you could hack together a patch which just ignores DP connectors during
> probing if they don't have an associated DDC instead of changing the
> code everywhere the DDC object is required.
>
I will try to find the time to look at this in more detail. GPU/DRM is
not something I have done any real work on in the past so I do not know
how it all fits together. The overkill on the code changes is the
result, as that way I don't need to understand where and why the helper
function (radeon_dp_getsinktype) gets called or what other code paths
can be executed that could fail.
I will wait for Alex to have a think about it and respond before I do
any more - that way I can finish the work I am paid to do first :).
> Regards,
> Christian.
>
Regards
Mark.
>>
>>>
>>> Alex can probably take a look when he's back from vacation.
>>>
>>> Regards,
>>> Christian.
>>>
>>>> [ 8.298791] [drm] Encoders:
>>>> [ 8.301770] [drm] DFP1: INTERNAL_UNIPHY
>>>> [ 8.305973] [drm] Connector 1:
>>>> [ 8.309043] [drm] DP-2
>>>> [ 8.311598] [drm] HPD2
>>>> [ 8.314169] [drm] DDC: 0x6440 0x6440 0x6444 0x6444 0x6448 0x6448
>>>> 0x644c 0x644c
>>>> [ 8.321609] [drm] Encoders:
>>>> [ 8.324589] [drm] DFP2: INTERNAL_UNIPHY
>>>> [ 8.328793] [drm] Connector 2:
>>>> [ 8.331856] [drm] VGA-1
>>>> [ 8.342947] [drm] DDC: 0x64d8 0x64d8 0x64dc 0x64dc 0x64e0 0x64e0
>>>> 0x64e4 0x64e4
>>>> [ 8.350341] [drm] Encoders:
>>>> [ 8.353310] [drm] CRT1: INTERNAL_KLDSCP_DAC1
>>>> [ 8.358195] BUG: unable to handle kernel NULL pointer dereference at
>>>> 0000000000000409
>>>> [ 8.409733] [<ffffffffc02024da>] radeon_dp_getsinktype+0x1a/0x30
>>>> [radeon]
>>>> [ 8.416805] PGD 0
>>>> [ 8.418841] Oops: 0000 [#1] SMP
>>>> ...
>>>>
>>>> This patch prevents Kernel failures due to a connector not having a
>>>> DDC interface by changing the code so that ddc_bus is always checked
>>>> before use.
>>>> The problem was first identified using the uBuntu MATE 14.04 LTS (3.16
>>>> series kernels) but not dealt with at that time. On attempting to
>>>> install uBuntu MATE 16.04 LTS (4.4 series kernels), it became clear
>>>> that using various workarounds to allow the issue to be ignored were
>>>> not viable so more effort was put in to sorting the issue resulting in
>>>> this patch. See https://bugs.launchpad.net/bugs/1587885 for more
>>>> details.
>>>>
>>>> Signed-off-by: Mark Fortescue <mark@thurning-instruments.co.uk>
>>>> Tested-by: Mark Fortescue <mark@thurning-instruments.co.uk>
>>>>
>>>> ---
>>>>
>>>> Looks like Thunderbird may have made a mess of the patch when pasting
>>>> the contents into the mail message - my alternate mail client (pine)
>>>> also has strict line length handling and trashes non-MIME encoded
>>>> patches.
>>>>
>>>> This may not be the correct approach to solving the issue but it is
>>>> clean in that it ensures that ddc_bus is never used when NULL
>>>> regardless of how the code ended up at the point of use.
>>>>
>>>> If it helps with back porting, I have patches for the uBuntu 14.04 LTS
>>>> [3.13 series], uBuntu MATE 14.04 LTS [3.16 series] and uBuntu 16.04
>>>> LTS [4.4 series] kernels.
>>>>
>>>> Test Hardware:
>>>> Commell LV-683 Mini-ITX with onboard AMD Dual-core G-T56N
>>>> 4G Ram, 2x1TB Disk, HANNS-G HC194D 1280x1024 LCD (VGA).
>>>> 4.8.0-rc5 with patch boots without error.
>>>>
>>>> drivers/gpu/drm/radeon/atombios_dp.c | 60 ++++++++++++-------
>>>> drivers/gpu/drm/radeon/radeon_connectors.c | 46 +++++++-------
>>>> drivers/gpu/drm/radeon/radeon_dp_mst.c | 9 ++
>>>> drivers/gpu/drm/radeon/radeon_i2c.c | 3
>>>> 4 files changed, 73 insertions(+), 45 deletions(-)
>>>>
>>>> Patch:
>>>> diff --git a/drivers/gpu/drm/radeon/atombios_dp.c
>>>> b/drivers/gpu/drm/radeon/atombios_dp.c
>>>> index cead089a..98b3c0e 100644
>>>> --- a/drivers/gpu/drm/radeon/atombios_dp.c
>>>> +++ b/drivers/gpu/drm/radeon/atombios_dp.c
>>>> @@ -232,6 +232,9 @@ void radeon_dp_aux_init(struct radeon_connector
>>>> *radeon_connector)
>>>> struct radeon_device *rdev = dev->dev_private;
>>>> int ret;
>>>>
>>>> + if (!radeon_connector->ddc_bus)
>>>> + return;
>>>> +
>>>> radeon_connector->ddc_bus->rec.hpd = radeon_connector->hpd.hpd;
>>>> radeon_connector->ddc_bus->aux.dev = radeon_connector->base.kdev;
>>>> if (ASIC_IS_DCE5(rdev)) {
>>>> @@ -364,6 +367,9 @@ u8 radeon_dp_getsinktype(struct radeon_connector
>>>> *radeon_connector)
>>>> struct drm_device *dev = radeon_connector->base.dev;
>>>> struct radeon_device *rdev = dev->dev_private;
>>>>
>>>> + if (!radeon_connector->ddc_bus)
>>>> + return 0;
>>>> +
>>>> return radeon_dp_encoder_service(rdev,
>>>> ATOM_DP_ACTION_GET_SINK_TYPE, 0,
>>>> radeon_connector->ddc_bus->rec.i2c_id, 0);
>>>> }
>>>> @@ -376,6 +382,9 @@ static void radeon_dp_probe_oui(struct
>>>> radeon_connector *radeon_connector)
>>>> if (!(dig_connector->dpcd[DP_DOWN_STREAM_PORT_COUNT] &
>>>> DP_OUI_SUPPORT))
>>>> return;
>>>>
>>>> + if (!radeon_connector->ddc_bus)
>>>> + return;
>>>> +
>>>> if (drm_dp_dpcd_read(&radeon_connector->ddc_bus->aux,
>>>> DP_SINK_OUI, buf, 3) == 3)
>>>> DRM_DEBUG_KMS("Sink OUI: %02hx%02hx%02hx\n",
>>>> buf[0], buf[1], buf[2]);
>>>> @@ -391,6 +400,9 @@ bool radeon_dp_getdpcd(struct radeon_connector
>>>> *radeon_connector)
>>>> u8 msg[DP_DPCD_SIZE];
>>>> int ret, i;
>>>>
>>>> + if (!radeon_connector->ddc_bus)
>>>> + return false;
>>>> +
>>>> for (i = 0; i < 7; i++) {
>>>> ret = drm_dp_dpcd_read(&radeon_connector->ddc_bus->aux,
>>>> DP_DPCD_REV, msg,
>>>> DP_DPCD_SIZE);
>>>> @@ -428,24 +440,26 @@ int radeon_dp_get_panel_mode(struct drm_encoder
>>>> *encoder,
>>>>
>>>> dig_connector = radeon_connector->con_priv;
>>>>
>>>> - if (dp_bridge != ENCODER_OBJECT_ID_NONE) {
>>>> - /* DP bridge chips */
>>>> - if (drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux,
>>>> - DP_EDP_CONFIGURATION_CAP, &tmp) == 1) {
>>>> - if (tmp & 1)
>>>> - panel_mode = DP_PANEL_MODE_INTERNAL_DP2_MODE;
>>>> - else if ((dp_bridge == ENCODER_OBJECT_ID_NUTMEG) ||
>>>> - (dp_bridge == ENCODER_OBJECT_ID_TRAVIS))
>>>> - panel_mode = DP_PANEL_MODE_INTERNAL_DP1_MODE;
>>>> - else
>>>> - panel_mode = DP_PANEL_MODE_EXTERNAL_DP_MODE;
>>>> - }
>>>> - } else if (connector->connector_type == DRM_MODE_CONNECTOR_eDP) {
>>>> - /* eDP */
>>>> - if (drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux,
>>>> - DP_EDP_CONFIGURATION_CAP, &tmp) == 1) {
>>>> - if (tmp & 1)
>>>> - panel_mode = DP_PANEL_MODE_INTERNAL_DP2_MODE;
>>>> + if (radeon_connector->ddc_bus) {
>>>> + if (dp_bridge != ENCODER_OBJECT_ID_NONE) {
>>>> + /* DP bridge chips */
>>>> + if (drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux,
>>>> + DP_EDP_CONFIGURATION_CAP, &tmp) == 1) {
>>>> + if (tmp & 1)
>>>> + panel_mode = DP_PANEL_MODE_INTERNAL_DP2_MODE;
>>>> + else if ((dp_bridge == ENCODER_OBJECT_ID_NUTMEG) ||
>>>> + (dp_bridge == ENCODER_OBJECT_ID_TRAVIS))
>>>> + panel_mode = DP_PANEL_MODE_INTERNAL_DP1_MODE;
>>>> + else
>>>> + panel_mode = DP_PANEL_MODE_EXTERNAL_DP_MODE;
>>>> + }
>>>> + } else if (connector->connector_type ==
>>>> DRM_MODE_CONNECTOR_eDP) {
>>>> + /* eDP */
>>>> + if (drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux,
>>>> + DP_EDP_CONFIGURATION_CAP, &tmp) == 1) {
>>>> + if (tmp & 1)
>>>> + panel_mode = DP_PANEL_MODE_INTERNAL_DP2_MODE;
>>>> + }
>>>> }
>>>> }
>>>>
>>>> @@ -511,6 +525,9 @@ bool radeon_dp_needs_link_train(struct
>>>> radeon_connector *radeon_connector)
>>>> u8 link_status[DP_LINK_STATUS_SIZE];
>>>> struct radeon_connector_atom_dig *dig =
>>>> radeon_connector->con_priv;
>>>>
>>>> + if (!radeon_connector->ddc_bus)
>>>> + return false;
>>>> +
>>>> if
>>>> (drm_dp_dpcd_read_link_status(&radeon_connector->ddc_bus->aux,
>>>> link_status)
>>>> <= 0)
>>>> return false;
>>>> @@ -531,7 +548,7 @@ void radeon_dp_set_rx_power_state(struct
>>>> drm_connector *connector,
>>>> dig_connector = radeon_connector->con_priv;
>>>>
>>>> /* power up/down the sink */
>>>> - if (dig_connector->dpcd[0] >= 0x11) {
>>>> + if (radeon_connector->ddc_bus && dig_connector->dpcd[0] >= 0x11) {
>>>> drm_dp_dpcd_writeb(&radeon_connector->ddc_bus->aux,
>>>> DP_SET_POWER, power_state);
>>>> usleep_range(1000, 2000);
>>>> @@ -834,7 +851,8 @@ void radeon_dp_link_train(struct drm_encoder
>>>> *encoder,
>>>> else
>>>> dp_info.enc_id |= ATOM_DP_CONFIG_LINK_A;
>>>>
>>>> - if (drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux,
>>>> DP_MAX_LANE_COUNT, &tmp)
>>>> + if (radeon_connector->ddc_bus &&
>>>> + drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux,
>>>> DP_MAX_LANE_COUNT, &tmp)
>>>> == 1) {
>>>> if (ASIC_IS_DCE5(rdev) && (tmp & DP_TPS3_SUPPORTED))
>>>> dp_info.tp3_supported = true;
>>>> @@ -850,7 +868,7 @@ void radeon_dp_link_train(struct drm_encoder
>>>> *encoder,
>>>> dp_info.connector = connector;
>>>> dp_info.dp_lane_count = dig_connector->dp_lane_count;
>>>> dp_info.dp_clock = dig_connector->dp_clock;
>>>> - dp_info.aux = &radeon_connector->ddc_bus->aux;
>>>> + dp_info.aux = radeon_connector->ddc_bus ?
>>>> &radeon_connector->ddc_bus->aux : 0;
>>>>
>>>> if (radeon_dp_link_train_init(&dp_info))
>>>> goto done;
>>>> diff --git a/drivers/gpu/drm/radeon/radeon_connectors.c
>>>> b/drivers/gpu/drm/radeon/radeon_connectors.c
>>>> index b79f3b0..cec30c9 100644
>>>> --- a/drivers/gpu/drm/radeon/radeon_connectors.c
>>>> +++ b/drivers/gpu/drm/radeon/radeon_connectors.c
>>>> @@ -328,31 +328,32 @@ static void radeon_connector_get_edid(struct
>>>> drm_connector *connector)
>>>> if (radeon_connector->router.ddc_valid)
>>>> radeon_router_select_ddc_port(radeon_connector);
>>>>
>>>> - if
>>>> ((radeon_connector_encoder_get_dp_bridge_encoder_id(connector) !=
>>>> - ENCODER_OBJECT_ID_NONE) &&
>>>> - radeon_connector->ddc_bus->has_aux) {
>>>> - radeon_connector->edid = drm_get_edid(connector,
>>>> - &radeon_connector->ddc_bus->aux.ddc);
>>>> - } else if ((connector->connector_type ==
>>>> DRM_MODE_CONNECTOR_DisplayPort) ||
>>>> - (connector->connector_type == DRM_MODE_CONNECTOR_eDP)) {
>>>> - struct radeon_connector_atom_dig *dig =
>>>> radeon_connector->con_priv;
>>>> -
>>>> - if ((dig->dp_sink_type == CONNECTOR_OBJECT_ID_DISPLAYPORT ||
>>>> - dig->dp_sink_type == CONNECTOR_OBJECT_ID_eDP) &&
>>>> - radeon_connector->ddc_bus->has_aux)
>>>> - radeon_connector->edid =
>>>> drm_get_edid(&radeon_connector->base,
>>>> + if (radeon_connector->ddc_bus) {
>>>> + if
>>>> ((radeon_connector_encoder_get_dp_bridge_encoder_id(connector) !=
>>>> + ENCODER_OBJECT_ID_NONE) &&
>>>> + radeon_connector->ddc_bus->has_aux) {
>>>> + radeon_connector->edid = drm_get_edid(connector,
>>>> &radeon_connector->ddc_bus->aux.ddc);
>>>> - else if (radeon_connector->ddc_bus)
>>>> + } else if ((connector->connector_type ==
>>>> DRM_MODE_CONNECTOR_DisplayPort) ||
>>>> + (connector->connector_type ==
>>>> DRM_MODE_CONNECTOR_eDP)) {
>>>> + struct radeon_connector_atom_dig *dig =
>>>> radeon_connector->con_priv;
>>>> +
>>>> + if ((dig->dp_sink_type ==
>>>> CONNECTOR_OBJECT_ID_DISPLAYPORT ||
>>>> + dig->dp_sink_type == CONNECTOR_OBJECT_ID_eDP) &&
>>>> + radeon_connector->ddc_bus->has_aux)
>>>> + radeon_connector->edid =
>>>> drm_get_edid(&radeon_connector->base,
>>>> + &radeon_connector->ddc_bus->aux.ddc);
>>>> + else
>>>> + radeon_connector->edid =
>>>> drm_get_edid(&radeon_connector->base,
>>>> + &radeon_connector->ddc_bus->adapter);
>>>> + } else if (vga_switcheroo_handler_flags() &
>>>> VGA_SWITCHEROO_CAN_SWITCH_DDC &&
>>>> + connector->connector_type == DRM_MODE_CONNECTOR_LVDS) {
>>>> + radeon_connector->edid =
>>>> drm_get_edid_switcheroo(&radeon_connector->base,
>>>> + &radeon_connector->ddc_bus->adapter);
>>>> + } else {
>>>> radeon_connector->edid =
>>>> drm_get_edid(&radeon_connector->base,
>>>> &radeon_connector->ddc_bus->adapter);
>>>> - } else if (vga_switcheroo_handler_flags() &
>>>> VGA_SWITCHEROO_CAN_SWITCH_DDC &&
>>>> - connector->connector_type == DRM_MODE_CONNECTOR_LVDS &&
>>>> - radeon_connector->ddc_bus) {
>>>> - radeon_connector->edid =
>>>> drm_get_edid_switcheroo(&radeon_connector->base,
>>>> - &radeon_connector->ddc_bus->adapter);
>>>> - } else if (radeon_connector->ddc_bus) {
>>>> - radeon_connector->edid = drm_get_edid(&radeon_connector->base,
>>>> - &radeon_connector->ddc_bus->adapter);
>>>> + }
>>>> }
>>>>
>>>> if (!radeon_connector->edid) {
>>>> @@ -1312,6 +1313,7 @@ radeon_dvi_detect(struct drm_connector
>>>> *connector, bool force)
>>>> continue;
>>>> list_radeon_connector =
>>>> to_radeon_connector(list_connector);
>>>> if (list_radeon_connector->shared_ddc &&
>>>> + radeon_connector->ddc_bus &&
>>>> (list_radeon_connector->ddc_bus->rec.i2c_id ==
>>>> radeon_connector->ddc_bus->rec.i2c_id)) {
>>>> /* cases where both connectors are digital */
>>>> diff --git a/drivers/gpu/drm/radeon/radeon_dp_mst.c
>>>> b/drivers/gpu/drm/radeon/radeon_dp_mst.c
>>>> index de504ea..89e91f3 100644
>>>> --- a/drivers/gpu/drm/radeon/radeon_dp_mst.c
>>>> +++ b/drivers/gpu/drm/radeon/radeon_dp_mst.c
>>>> @@ -661,7 +661,7 @@ radeon_dp_mst_init(struct radeon_connector
>>>> *radeon_connector)
>>>> {
>>>> struct drm_device *dev = radeon_connector->base.dev;
>>>>
>>>> - if (!radeon_connector->ddc_bus->has_aux)
>>>> + if (!radeon_connector->ddc_bus ||
>>>> !radeon_connector->ddc_bus->has_aux)
>>>> return 0;
>>>>
>>>> radeon_connector->mst_mgr.cbs = &mst_cbs;
>>>> @@ -688,6 +688,9 @@ radeon_dp_mst_probe(struct radeon_connector
>>>> *radeon_connector)
>>>> if (dig_connector->dpcd[DP_DPCD_REV] < 0x12)
>>>> return 0;
>>>>
>>>> + if (!radeon_connector->ddc_bus ||
>>>> !radeon_connector->ddc_bus->has_aux)
>>>> + return 0;
>>>> +
>>>> ret = drm_dp_dpcd_read(&radeon_connector->ddc_bus->aux,
>>>> DP_MSTM_CAP, msg,
>>>> 1);
>>>> if (ret) {
>>>> @@ -711,7 +714,9 @@ radeon_dp_mst_check_status(struct radeon_connector
>>>> *radeon_connector)
>>>> struct radeon_connector_atom_dig *dig_connector =
>>>> radeon_connector->con_priv;
>>>> int retry;
>>>>
>>>> - if (dig_connector->is_mst) {
>>>> + if (dig_connector->is_mst &&
>>>> + radeon_connector->ddc_bus &&
>>>> + radeon_connector->ddc_bus->has_aux) {
>>>> u8 esi[16] = { 0 };
>>>> int dret;
>>>> int ret = 0;
>>>> diff --git a/drivers/gpu/drm/radeon/radeon_i2c.c
>>>> b/drivers/gpu/drm/radeon/radeon_i2c.c
>>>> index 9590bcd..c18f7ba 100644
>>>> --- a/drivers/gpu/drm/radeon/radeon_i2c.c
>>>> +++ b/drivers/gpu/drm/radeon/radeon_i2c.c
>>>> @@ -63,6 +63,9 @@ bool radeon_ddc_probe(struct radeon_connector
>>>> *radeon_connector, bool use_aux)
>>>> if (radeon_connector->router.ddc_valid)
>>>> radeon_router_select_ddc_port(radeon_connector);
>>>>
>>>> + if (!radeon_connector->ddc_bus)
>>>> + return false;
>>>> +
>>>> if (use_aux) {
>>>> ret = i2c_transfer(&radeon_connector->ddc_bus->aux.ddc,
>>>> msgs, 2);
>>>> } else {
>>>> _______________________________________________
>>>> dri-devel mailing list
>>>> dri-devel@lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 001/001] drivers/gpu/radeon: NULL pointer deference workaround
2016-09-08 11:18 ` Mark Fortescue
@ 2016-09-09 8:15 ` Christian König
2016-09-09 13:20 ` Deucher, Alexander
0 siblings, 1 reply; 8+ messages in thread
From: Christian König @ 2016-09-09 8:15 UTC (permalink / raw)
To: Mark Fortescue, Christian König, David Airlie
Cc: Alex Deucher, linux-kernel, dri-devel
>> On this motherboard, DP-1 is a single channel 18bit LVDS LCD panel
>> interface and DP-2 is a DVI interface (which I can connect to my
>> monitor if testing this is useful). There are no displayport connectors.
>
> Yeah, but from the driver point of view there are only DP connectors on
> the chip. The LVDS and DVI are probably realized with external DP to
> whatever converter ICs.
>
>
> That would explain why some similar boards have 24bit LVDS and others
> only 18bit LVDS.
It could be that this is actually a configuration we don't support yet
with radeon and/or the display stack.
E.g. that the connector only uses the displayport LVDS lanes, but not
actual the displayport protocol to train those lanes.
Instead it rather expects a fixed training which is configured somewhere
in the BIOS.
Anyway Alex needs to take a look, he knows displayport much better than
I do (and hates it passionately :).
Cheers,
Christian.
Am 08.09.2016 um 13:18 schrieb Mark Fortescue:
> On 08/09/16 11:25, Christian König wrote:
>> Am 08.09.2016 um 11:09 schrieb Mark Fortescue:
>>> Hi Christian,
>>>
>>> Thank you for the feedback.
>>>
>>> On 08/09/16 08:14, Christian König wrote:
>>>> Am 07.09.2016 um 19:38 schrieb Mark Fortescue:
>>>>>
>>>>> On an LV-683 (AMD Dual-core G-T56N) Mini-ITX board, I get a Kernel
>>>>> Oops because Connector 0 (LCD Panel interface) does not have DDC.
>>>>
>>>> I'm not an expert on this, but that is really odd cause even LCD
>>>> Panels
>>>> should have a DDC interface.
>>>>
>>>>>
>>>>> Ubuntu 16.04 LTS Kernel (4.4 series):
>>>>>
>>>>> ...
>>>>> [ 8.262990] [drm] ib test on ring 5 succeeded
>>>>> [ 8.288897] [drm] Radeon Display Connectors
>>>>> [ 8.293175] [drm] Connector 0:
>>>>> [ 8.296252] [drm] DP-1
>>>>
>>>> Especially since the BIOS claims that this is a displayport connector
>>>> and there is no physical way to have a DP without an DDC as far as I
>>>> know.
>>>>
>>>> Please open a bug report on FDO and attach you BIOS image.
>>>
>>> FDO ? I am not familiar with this. Please can you enlighten me.
>>>
>>
>> See here: http://bugs.freedesktop.org/
>>
>>> I do not have a BIOS image so will need some assistance in
>>> understanding what is required here and how I extract the BIOS
>>> information you are after.
>>>
>>
>> Sorry my fault. Mullins is an APU, so you don't have a dedicated video
>> BIOS. As usually I didn't got enough sleep :) But please open up a bug
>> report anyway.
>
> Know the problem of being awake too long :). I will raise a bug.
>
>>
>>> On this motherboard, DP-1 is a single channel 18bit LVDS LCD panel
>>> interface and DP-2 is a DVI interface (which I can connect to my
>>> monitor if testing this is useful). There are no displayport
>>> connectors.
>>
>> Yeah, but from the driver point of view there are only DP connectors on
>> the chip. The LVDS and DVI are probably realized with external DP to
>> whatever converter ICs.
>>
>
> That would explain why some similar boards have 24bit LVDS and others
> only 18bit LVDS.
>
>>>
>>> On industrial motherboards, I have noticed that it is not uncommon to
>>> hard code the information for the LCD panel into the BIOS so no DDC is
>>> required. In this case, there is no LCD panel connected to the
>>> interface anyway.
>>>
>>
>> That is correct and as far as I know well supported by Radeon, but the
>> crux is there should still be a DDC channel even if there isn't anything
>> attached to it.
>>
>> See with displayport you got four LVDS channels to submit the actual
>> picture and an AUX channel to configure and query the device. The DDC is
>> just represented as certain packets over the AUX channel.
>>
>> If the AUX channel doesn't work or isn't connect then the link training
>> wouldn't be possible as well and so you wouldn't be able to get any
>> picture on the LVDS.
>>
>
> Interesting.
>
>>> See http://www.commell.com.tw/product/SBC/LV-683.HTM for more
>>> information on the board. Looking at the web site, there is a BIOS
>>> image available form Commell if that is of use.
>>
>> Alex clearly needs to take a look on this. I think for the time being
>> you could hack together a patch which just ignores DP connectors during
>> probing if they don't have an associated DDC instead of changing the
>> code everywhere the DDC object is required.
>>
>
> I will try to find the time to look at this in more detail. GPU/DRM is
> not something I have done any real work on in the past so I do not
> know how it all fits together. The overkill on the code changes is the
> result, as that way I don't need to understand where and why the
> helper function (radeon_dp_getsinktype) gets called or what other code
> paths can be executed that could fail.
>
> I will wait for Alex to have a think about it and respond before I do
> any more - that way I can finish the work I am paid to do first :).
>
>> Regards,
>> Christian.
>>
> Regards
> Mark.
>>>
>>>>
>>>> Alex can probably take a look when he's back from vacation.
>>>>
>>>> Regards,
>>>> Christian.
>>>>
>>>>> [ 8.298791] [drm] Encoders:
>>>>> [ 8.301770] [drm] DFP1: INTERNAL_UNIPHY
>>>>> [ 8.305973] [drm] Connector 1:
>>>>> [ 8.309043] [drm] DP-2
>>>>> [ 8.311598] [drm] HPD2
>>>>> [ 8.314169] [drm] DDC: 0x6440 0x6440 0x6444 0x6444 0x6448 0x6448
>>>>> 0x644c 0x644c
>>>>> [ 8.321609] [drm] Encoders:
>>>>> [ 8.324589] [drm] DFP2: INTERNAL_UNIPHY
>>>>> [ 8.328793] [drm] Connector 2:
>>>>> [ 8.331856] [drm] VGA-1
>>>>> [ 8.342947] [drm] DDC: 0x64d8 0x64d8 0x64dc 0x64dc 0x64e0 0x64e0
>>>>> 0x64e4 0x64e4
>>>>> [ 8.350341] [drm] Encoders:
>>>>> [ 8.353310] [drm] CRT1: INTERNAL_KLDSCP_DAC1
>>>>> [ 8.358195] BUG: unable to handle kernel NULL pointer dereference at
>>>>> 0000000000000409
>>>>> [ 8.409733] [<ffffffffc02024da>] radeon_dp_getsinktype+0x1a/0x30
>>>>> [radeon]
>>>>> [ 8.416805] PGD 0
>>>>> [ 8.418841] Oops: 0000 [#1] SMP
>>>>> ...
>>>>>
>>>>> This patch prevents Kernel failures due to a connector not having a
>>>>> DDC interface by changing the code so that ddc_bus is always checked
>>>>> before use.
>>>>> The problem was first identified using the uBuntu MATE 14.04 LTS
>>>>> (3.16
>>>>> series kernels) but not dealt with at that time. On attempting to
>>>>> install uBuntu MATE 16.04 LTS (4.4 series kernels), it became clear
>>>>> that using various workarounds to allow the issue to be ignored were
>>>>> not viable so more effort was put in to sorting the issue
>>>>> resulting in
>>>>> this patch. See https://bugs.launchpad.net/bugs/1587885 for more
>>>>> details.
>>>>>
>>>>> Signed-off-by: Mark Fortescue <mark@thurning-instruments.co.uk>
>>>>> Tested-by: Mark Fortescue <mark@thurning-instruments.co.uk>
>>>>>
>>>>> ---
>>>>>
>>>>> Looks like Thunderbird may have made a mess of the patch when pasting
>>>>> the contents into the mail message - my alternate mail client (pine)
>>>>> also has strict line length handling and trashes non-MIME encoded
>>>>> patches.
>>>>>
>>>>> This may not be the correct approach to solving the issue but it is
>>>>> clean in that it ensures that ddc_bus is never used when NULL
>>>>> regardless of how the code ended up at the point of use.
>>>>>
>>>>> If it helps with back porting, I have patches for the uBuntu 14.04
>>>>> LTS
>>>>> [3.13 series], uBuntu MATE 14.04 LTS [3.16 series] and uBuntu 16.04
>>>>> LTS [4.4 series] kernels.
>>>>>
>>>>> Test Hardware:
>>>>> Commell LV-683 Mini-ITX with onboard AMD Dual-core G-T56N
>>>>> 4G Ram, 2x1TB Disk, HANNS-G HC194D 1280x1024 LCD (VGA).
>>>>> 4.8.0-rc5 with patch boots without error.
>>>>>
>>>>> drivers/gpu/drm/radeon/atombios_dp.c | 60
>>>>> ++++++++++++-------
>>>>> drivers/gpu/drm/radeon/radeon_connectors.c | 46 +++++++-------
>>>>> drivers/gpu/drm/radeon/radeon_dp_mst.c | 9 ++
>>>>> drivers/gpu/drm/radeon/radeon_i2c.c | 3
>>>>> 4 files changed, 73 insertions(+), 45 deletions(-)
>>>>>
>>>>> Patch:
>>>>> diff --git a/drivers/gpu/drm/radeon/atombios_dp.c
>>>>> b/drivers/gpu/drm/radeon/atombios_dp.c
>>>>> index cead089a..98b3c0e 100644
>>>>> --- a/drivers/gpu/drm/radeon/atombios_dp.c
>>>>> +++ b/drivers/gpu/drm/radeon/atombios_dp.c
>>>>> @@ -232,6 +232,9 @@ void radeon_dp_aux_init(struct radeon_connector
>>>>> *radeon_connector)
>>>>> struct radeon_device *rdev = dev->dev_private;
>>>>> int ret;
>>>>>
>>>>> + if (!radeon_connector->ddc_bus)
>>>>> + return;
>>>>> +
>>>>> radeon_connector->ddc_bus->rec.hpd = radeon_connector->hpd.hpd;
>>>>> radeon_connector->ddc_bus->aux.dev =
>>>>> radeon_connector->base.kdev;
>>>>> if (ASIC_IS_DCE5(rdev)) {
>>>>> @@ -364,6 +367,9 @@ u8 radeon_dp_getsinktype(struct radeon_connector
>>>>> *radeon_connector)
>>>>> struct drm_device *dev = radeon_connector->base.dev;
>>>>> struct radeon_device *rdev = dev->dev_private;
>>>>>
>>>>> + if (!radeon_connector->ddc_bus)
>>>>> + return 0;
>>>>> +
>>>>> return radeon_dp_encoder_service(rdev,
>>>>> ATOM_DP_ACTION_GET_SINK_TYPE, 0,
>>>>> radeon_connector->ddc_bus->rec.i2c_id, 0);
>>>>> }
>>>>> @@ -376,6 +382,9 @@ static void radeon_dp_probe_oui(struct
>>>>> radeon_connector *radeon_connector)
>>>>> if (!(dig_connector->dpcd[DP_DOWN_STREAM_PORT_COUNT] &
>>>>> DP_OUI_SUPPORT))
>>>>> return;
>>>>>
>>>>> + if (!radeon_connector->ddc_bus)
>>>>> + return;
>>>>> +
>>>>> if (drm_dp_dpcd_read(&radeon_connector->ddc_bus->aux,
>>>>> DP_SINK_OUI, buf, 3) == 3)
>>>>> DRM_DEBUG_KMS("Sink OUI: %02hx%02hx%02hx\n",
>>>>> buf[0], buf[1], buf[2]);
>>>>> @@ -391,6 +400,9 @@ bool radeon_dp_getdpcd(struct radeon_connector
>>>>> *radeon_connector)
>>>>> u8 msg[DP_DPCD_SIZE];
>>>>> int ret, i;
>>>>>
>>>>> + if (!radeon_connector->ddc_bus)
>>>>> + return false;
>>>>> +
>>>>> for (i = 0; i < 7; i++) {
>>>>> ret = drm_dp_dpcd_read(&radeon_connector->ddc_bus->aux,
>>>>> DP_DPCD_REV, msg,
>>>>> DP_DPCD_SIZE);
>>>>> @@ -428,24 +440,26 @@ int radeon_dp_get_panel_mode(struct drm_encoder
>>>>> *encoder,
>>>>>
>>>>> dig_connector = radeon_connector->con_priv;
>>>>>
>>>>> - if (dp_bridge != ENCODER_OBJECT_ID_NONE) {
>>>>> - /* DP bridge chips */
>>>>> - if (drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux,
>>>>> - DP_EDP_CONFIGURATION_CAP, &tmp) == 1) {
>>>>> - if (tmp & 1)
>>>>> - panel_mode = DP_PANEL_MODE_INTERNAL_DP2_MODE;
>>>>> - else if ((dp_bridge == ENCODER_OBJECT_ID_NUTMEG) ||
>>>>> - (dp_bridge == ENCODER_OBJECT_ID_TRAVIS))
>>>>> - panel_mode = DP_PANEL_MODE_INTERNAL_DP1_MODE;
>>>>> - else
>>>>> - panel_mode = DP_PANEL_MODE_EXTERNAL_DP_MODE;
>>>>> - }
>>>>> - } else if (connector->connector_type ==
>>>>> DRM_MODE_CONNECTOR_eDP) {
>>>>> - /* eDP */
>>>>> - if (drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux,
>>>>> - DP_EDP_CONFIGURATION_CAP, &tmp) == 1) {
>>>>> - if (tmp & 1)
>>>>> - panel_mode = DP_PANEL_MODE_INTERNAL_DP2_MODE;
>>>>> + if (radeon_connector->ddc_bus) {
>>>>> + if (dp_bridge != ENCODER_OBJECT_ID_NONE) {
>>>>> + /* DP bridge chips */
>>>>> + if (drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux,
>>>>> + DP_EDP_CONFIGURATION_CAP, &tmp) == 1) {
>>>>> + if (tmp & 1)
>>>>> + panel_mode = DP_PANEL_MODE_INTERNAL_DP2_MODE;
>>>>> + else if ((dp_bridge == ENCODER_OBJECT_ID_NUTMEG) ||
>>>>> + (dp_bridge == ENCODER_OBJECT_ID_TRAVIS))
>>>>> + panel_mode = DP_PANEL_MODE_INTERNAL_DP1_MODE;
>>>>> + else
>>>>> + panel_mode = DP_PANEL_MODE_EXTERNAL_DP_MODE;
>>>>> + }
>>>>> + } else if (connector->connector_type ==
>>>>> DRM_MODE_CONNECTOR_eDP) {
>>>>> + /* eDP */
>>>>> + if (drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux,
>>>>> + DP_EDP_CONFIGURATION_CAP, &tmp) == 1) {
>>>>> + if (tmp & 1)
>>>>> + panel_mode = DP_PANEL_MODE_INTERNAL_DP2_MODE;
>>>>> + }
>>>>> }
>>>>> }
>>>>>
>>>>> @@ -511,6 +525,9 @@ bool radeon_dp_needs_link_train(struct
>>>>> radeon_connector *radeon_connector)
>>>>> u8 link_status[DP_LINK_STATUS_SIZE];
>>>>> struct radeon_connector_atom_dig *dig =
>>>>> radeon_connector->con_priv;
>>>>>
>>>>> + if (!radeon_connector->ddc_bus)
>>>>> + return false;
>>>>> +
>>>>> if
>>>>> (drm_dp_dpcd_read_link_status(&radeon_connector->ddc_bus->aux,
>>>>> link_status)
>>>>> <= 0)
>>>>> return false;
>>>>> @@ -531,7 +548,7 @@ void radeon_dp_set_rx_power_state(struct
>>>>> drm_connector *connector,
>>>>> dig_connector = radeon_connector->con_priv;
>>>>>
>>>>> /* power up/down the sink */
>>>>> - if (dig_connector->dpcd[0] >= 0x11) {
>>>>> + if (radeon_connector->ddc_bus && dig_connector->dpcd[0] >=
>>>>> 0x11) {
>>>>> drm_dp_dpcd_writeb(&radeon_connector->ddc_bus->aux,
>>>>> DP_SET_POWER, power_state);
>>>>> usleep_range(1000, 2000);
>>>>> @@ -834,7 +851,8 @@ void radeon_dp_link_train(struct drm_encoder
>>>>> *encoder,
>>>>> else
>>>>> dp_info.enc_id |= ATOM_DP_CONFIG_LINK_A;
>>>>>
>>>>> - if (drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux,
>>>>> DP_MAX_LANE_COUNT, &tmp)
>>>>> + if (radeon_connector->ddc_bus &&
>>>>> + drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux,
>>>>> DP_MAX_LANE_COUNT, &tmp)
>>>>> == 1) {
>>>>> if (ASIC_IS_DCE5(rdev) && (tmp & DP_TPS3_SUPPORTED))
>>>>> dp_info.tp3_supported = true;
>>>>> @@ -850,7 +868,7 @@ void radeon_dp_link_train(struct drm_encoder
>>>>> *encoder,
>>>>> dp_info.connector = connector;
>>>>> dp_info.dp_lane_count = dig_connector->dp_lane_count;
>>>>> dp_info.dp_clock = dig_connector->dp_clock;
>>>>> - dp_info.aux = &radeon_connector->ddc_bus->aux;
>>>>> + dp_info.aux = radeon_connector->ddc_bus ?
>>>>> &radeon_connector->ddc_bus->aux : 0;
>>>>>
>>>>> if (radeon_dp_link_train_init(&dp_info))
>>>>> goto done;
>>>>> diff --git a/drivers/gpu/drm/radeon/radeon_connectors.c
>>>>> b/drivers/gpu/drm/radeon/radeon_connectors.c
>>>>> index b79f3b0..cec30c9 100644
>>>>> --- a/drivers/gpu/drm/radeon/radeon_connectors.c
>>>>> +++ b/drivers/gpu/drm/radeon/radeon_connectors.c
>>>>> @@ -328,31 +328,32 @@ static void radeon_connector_get_edid(struct
>>>>> drm_connector *connector)
>>>>> if (radeon_connector->router.ddc_valid)
>>>>> radeon_router_select_ddc_port(radeon_connector);
>>>>>
>>>>> - if
>>>>> ((radeon_connector_encoder_get_dp_bridge_encoder_id(connector) !=
>>>>> - ENCODER_OBJECT_ID_NONE) &&
>>>>> - radeon_connector->ddc_bus->has_aux) {
>>>>> - radeon_connector->edid = drm_get_edid(connector,
>>>>> - &radeon_connector->ddc_bus->aux.ddc);
>>>>> - } else if ((connector->connector_type ==
>>>>> DRM_MODE_CONNECTOR_DisplayPort) ||
>>>>> - (connector->connector_type == DRM_MODE_CONNECTOR_eDP)) {
>>>>> - struct radeon_connector_atom_dig *dig =
>>>>> radeon_connector->con_priv;
>>>>> -
>>>>> - if ((dig->dp_sink_type == CONNECTOR_OBJECT_ID_DISPLAYPORT ||
>>>>> - dig->dp_sink_type == CONNECTOR_OBJECT_ID_eDP) &&
>>>>> - radeon_connector->ddc_bus->has_aux)
>>>>> - radeon_connector->edid =
>>>>> drm_get_edid(&radeon_connector->base,
>>>>> + if (radeon_connector->ddc_bus) {
>>>>> + if
>>>>> ((radeon_connector_encoder_get_dp_bridge_encoder_id(connector) !=
>>>>> + ENCODER_OBJECT_ID_NONE) &&
>>>>> + radeon_connector->ddc_bus->has_aux) {
>>>>> + radeon_connector->edid = drm_get_edid(connector,
>>>>> &radeon_connector->ddc_bus->aux.ddc);
>>>>> - else if (radeon_connector->ddc_bus)
>>>>> + } else if ((connector->connector_type ==
>>>>> DRM_MODE_CONNECTOR_DisplayPort) ||
>>>>> + (connector->connector_type ==
>>>>> DRM_MODE_CONNECTOR_eDP)) {
>>>>> + struct radeon_connector_atom_dig *dig =
>>>>> radeon_connector->con_priv;
>>>>> +
>>>>> + if ((dig->dp_sink_type ==
>>>>> CONNECTOR_OBJECT_ID_DISPLAYPORT ||
>>>>> + dig->dp_sink_type == CONNECTOR_OBJECT_ID_eDP) &&
>>>>> + radeon_connector->ddc_bus->has_aux)
>>>>> + radeon_connector->edid =
>>>>> drm_get_edid(&radeon_connector->base,
>>>>> + &radeon_connector->ddc_bus->aux.ddc);
>>>>> + else
>>>>> + radeon_connector->edid =
>>>>> drm_get_edid(&radeon_connector->base,
>>>>> + &radeon_connector->ddc_bus->adapter);
>>>>> + } else if (vga_switcheroo_handler_flags() &
>>>>> VGA_SWITCHEROO_CAN_SWITCH_DDC &&
>>>>> + connector->connector_type ==
>>>>> DRM_MODE_CONNECTOR_LVDS) {
>>>>> + radeon_connector->edid =
>>>>> drm_get_edid_switcheroo(&radeon_connector->base,
>>>>> + &radeon_connector->ddc_bus->adapter);
>>>>> + } else {
>>>>> radeon_connector->edid =
>>>>> drm_get_edid(&radeon_connector->base,
>>>>> &radeon_connector->ddc_bus->adapter);
>>>>> - } else if (vga_switcheroo_handler_flags() &
>>>>> VGA_SWITCHEROO_CAN_SWITCH_DDC &&
>>>>> - connector->connector_type == DRM_MODE_CONNECTOR_LVDS &&
>>>>> - radeon_connector->ddc_bus) {
>>>>> - radeon_connector->edid =
>>>>> drm_get_edid_switcheroo(&radeon_connector->base,
>>>>> - &radeon_connector->ddc_bus->adapter);
>>>>> - } else if (radeon_connector->ddc_bus) {
>>>>> - radeon_connector->edid =
>>>>> drm_get_edid(&radeon_connector->base,
>>>>> - &radeon_connector->ddc_bus->adapter);
>>>>> + }
>>>>> }
>>>>>
>>>>> if (!radeon_connector->edid) {
>>>>> @@ -1312,6 +1313,7 @@ radeon_dvi_detect(struct drm_connector
>>>>> *connector, bool force)
>>>>> continue;
>>>>> list_radeon_connector =
>>>>> to_radeon_connector(list_connector);
>>>>> if (list_radeon_connector->shared_ddc &&
>>>>> + radeon_connector->ddc_bus &&
>>>>> (list_radeon_connector->ddc_bus->rec.i2c_id ==
>>>>> radeon_connector->ddc_bus->rec.i2c_id)) {
>>>>> /* cases where both connectors are
>>>>> digital */
>>>>> diff --git a/drivers/gpu/drm/radeon/radeon_dp_mst.c
>>>>> b/drivers/gpu/drm/radeon/radeon_dp_mst.c
>>>>> index de504ea..89e91f3 100644
>>>>> --- a/drivers/gpu/drm/radeon/radeon_dp_mst.c
>>>>> +++ b/drivers/gpu/drm/radeon/radeon_dp_mst.c
>>>>> @@ -661,7 +661,7 @@ radeon_dp_mst_init(struct radeon_connector
>>>>> *radeon_connector)
>>>>> {
>>>>> struct drm_device *dev = radeon_connector->base.dev;
>>>>>
>>>>> - if (!radeon_connector->ddc_bus->has_aux)
>>>>> + if (!radeon_connector->ddc_bus ||
>>>>> !radeon_connector->ddc_bus->has_aux)
>>>>> return 0;
>>>>>
>>>>> radeon_connector->mst_mgr.cbs = &mst_cbs;
>>>>> @@ -688,6 +688,9 @@ radeon_dp_mst_probe(struct radeon_connector
>>>>> *radeon_connector)
>>>>> if (dig_connector->dpcd[DP_DPCD_REV] < 0x12)
>>>>> return 0;
>>>>>
>>>>> + if (!radeon_connector->ddc_bus ||
>>>>> !radeon_connector->ddc_bus->has_aux)
>>>>> + return 0;
>>>>> +
>>>>> ret = drm_dp_dpcd_read(&radeon_connector->ddc_bus->aux,
>>>>> DP_MSTM_CAP, msg,
>>>>> 1);
>>>>> if (ret) {
>>>>> @@ -711,7 +714,9 @@ radeon_dp_mst_check_status(struct
>>>>> radeon_connector
>>>>> *radeon_connector)
>>>>> struct radeon_connector_atom_dig *dig_connector =
>>>>> radeon_connector->con_priv;
>>>>> int retry;
>>>>>
>>>>> - if (dig_connector->is_mst) {
>>>>> + if (dig_connector->is_mst &&
>>>>> + radeon_connector->ddc_bus &&
>>>>> + radeon_connector->ddc_bus->has_aux) {
>>>>> u8 esi[16] = { 0 };
>>>>> int dret;
>>>>> int ret = 0;
>>>>> diff --git a/drivers/gpu/drm/radeon/radeon_i2c.c
>>>>> b/drivers/gpu/drm/radeon/radeon_i2c.c
>>>>> index 9590bcd..c18f7ba 100644
>>>>> --- a/drivers/gpu/drm/radeon/radeon_i2c.c
>>>>> +++ b/drivers/gpu/drm/radeon/radeon_i2c.c
>>>>> @@ -63,6 +63,9 @@ bool radeon_ddc_probe(struct radeon_connector
>>>>> *radeon_connector, bool use_aux)
>>>>> if (radeon_connector->router.ddc_valid)
>>>>> radeon_router_select_ddc_port(radeon_connector);
>>>>>
>>>>> + if (!radeon_connector->ddc_bus)
>>>>> + return false;
>>>>> +
>>>>> if (use_aux) {
>>>>> ret = i2c_transfer(&radeon_connector->ddc_bus->aux.ddc,
>>>>> msgs, 2);
>>>>> } else {
>>>>> _______________________________________________
>>>>> dri-devel mailing list
>>>>> dri-devel@lists.freedesktop.org
>>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH 001/001] drivers/gpu/radeon: NULL pointer deference workaround
2016-09-09 8:15 ` Christian König
@ 2016-09-09 13:20 ` Deucher, Alexander
2016-09-09 13:32 ` Mark Fortescue
0 siblings, 1 reply; 8+ messages in thread
From: Deucher, Alexander @ 2016-09-09 13:20 UTC (permalink / raw)
To: Koenig, Christian, Mark Fortescue, David Airlie; +Cc: linux-kernel, dri-devel
> -----Original Message-----
> From: Koenig, Christian
> Sent: Friday, September 09, 2016 4:16 AM
> To: Mark Fortescue; Koenig, Christian; David Airlie
> Cc: Deucher, Alexander; linux-kernel@vger.kernel.org; dri-
> devel@lists.freedesktop.org
> Subject: Re: [PATCH 001/001] drivers/gpu/radeon: NULL pointer deference
> workaround
>
> >> On this motherboard, DP-1 is a single channel 18bit LVDS LCD panel
> >> interface and DP-2 is a DVI interface (which I can connect to my
> >> monitor if testing this is useful). There are no displayport connectors.
> >
> > Yeah, but from the driver point of view there are only DP connectors on
> > the chip. The LVDS and DVI are probably realized with external DP to
> > whatever converter ICs.
> >
> >
> > That would explain why some similar boards have 24bit LVDS and others
> > only 18bit LVDS.
>
> It could be that this is actually a configuration we don't support yet
> with radeon and/or the display stack.
>
> E.g. that the connector only uses the displayport LVDS lanes, but not
> actual the displayport protocol to train those lanes.
>
> Instead it rather expects a fixed training which is configured somewhere
> in the BIOS.
>
> Anyway Alex needs to take a look, he knows displayport much better than
> I do (and hates it passionately :).
Generally these sorts of setups are supported by exposing an LVDS or eDP connector rather than a regular DP. In the case of LVDS and eDP, we have code to fallback to a hardcoded EDID in the vbios if DDC is not available. I'll take a closer look when I'm back.
Alex
>
> Cheers,
> Christian.
>
> Am 08.09.2016 um 13:18 schrieb Mark Fortescue:
> > On 08/09/16 11:25, Christian König wrote:
> >> Am 08.09.2016 um 11:09 schrieb Mark Fortescue:
> >>> Hi Christian,
> >>>
> >>> Thank you for the feedback.
> >>>
> >>> On 08/09/16 08:14, Christian König wrote:
> >>>> Am 07.09.2016 um 19:38 schrieb Mark Fortescue:
> >>>>>
> >>>>> On an LV-683 (AMD Dual-core G-T56N) Mini-ITX board, I get a Kernel
> >>>>> Oops because Connector 0 (LCD Panel interface) does not have DDC.
> >>>>
> >>>> I'm not an expert on this, but that is really odd cause even LCD
> >>>> Panels
> >>>> should have a DDC interface.
> >>>>
> >>>>>
> >>>>> Ubuntu 16.04 LTS Kernel (4.4 series):
> >>>>>
> >>>>> ...
> >>>>> [ 8.262990] [drm] ib test on ring 5 succeeded
> >>>>> [ 8.288897] [drm] Radeon Display Connectors
> >>>>> [ 8.293175] [drm] Connector 0:
> >>>>> [ 8.296252] [drm] DP-1
> >>>>
> >>>> Especially since the BIOS claims that this is a displayport connector
> >>>> and there is no physical way to have a DP without an DDC as far as I
> >>>> know.
> >>>>
> >>>> Please open a bug report on FDO and attach you BIOS image.
> >>>
> >>> FDO ? I am not familiar with this. Please can you enlighten me.
> >>>
> >>
> >> See here: http://bugs.freedesktop.org/
> >>
> >>> I do not have a BIOS image so will need some assistance in
> >>> understanding what is required here and how I extract the BIOS
> >>> information you are after.
> >>>
> >>
> >> Sorry my fault. Mullins is an APU, so you don't have a dedicated video
> >> BIOS. As usually I didn't got enough sleep :) But please open up a bug
> >> report anyway.
> >
> > Know the problem of being awake too long :). I will raise a bug.
> >
> >>
> >>> On this motherboard, DP-1 is a single channel 18bit LVDS LCD panel
> >>> interface and DP-2 is a DVI interface (which I can connect to my
> >>> monitor if testing this is useful). There are no displayport
> >>> connectors.
> >>
> >> Yeah, but from the driver point of view there are only DP connectors on
> >> the chip. The LVDS and DVI are probably realized with external DP to
> >> whatever converter ICs.
> >>
> >
> > That would explain why some similar boards have 24bit LVDS and others
> > only 18bit LVDS.
> >
> >>>
> >>> On industrial motherboards, I have noticed that it is not uncommon to
> >>> hard code the information for the LCD panel into the BIOS so no DDC is
> >>> required. In this case, there is no LCD panel connected to the
> >>> interface anyway.
> >>>
> >>
> >> That is correct and as far as I know well supported by Radeon, but the
> >> crux is there should still be a DDC channel even if there isn't anything
> >> attached to it.
> >>
> >> See with displayport you got four LVDS channels to submit the actual
> >> picture and an AUX channel to configure and query the device. The DDC is
> >> just represented as certain packets over the AUX channel.
> >>
> >> If the AUX channel doesn't work or isn't connect then the link training
> >> wouldn't be possible as well and so you wouldn't be able to get any
> >> picture on the LVDS.
> >>
> >
> > Interesting.
> >
> >>> See http://www.commell.com.tw/product/SBC/LV-683.HTM for more
> >>> information on the board. Looking at the web site, there is a BIOS
> >>> image available form Commell if that is of use.
> >>
> >> Alex clearly needs to take a look on this. I think for the time being
> >> you could hack together a patch which just ignores DP connectors during
> >> probing if they don't have an associated DDC instead of changing the
> >> code everywhere the DDC object is required.
> >>
> >
> > I will try to find the time to look at this in more detail. GPU/DRM is
> > not something I have done any real work on in the past so I do not
> > know how it all fits together. The overkill on the code changes is the
> > result, as that way I don't need to understand where and why the
> > helper function (radeon_dp_getsinktype) gets called or what other code
> > paths can be executed that could fail.
> >
> > I will wait for Alex to have a think about it and respond before I do
> > any more - that way I can finish the work I am paid to do first :).
> >
> >> Regards,
> >> Christian.
> >>
> > Regards
> > Mark.
> >>>
> >>>>
> >>>> Alex can probably take a look when he's back from vacation.
> >>>>
> >>>> Regards,
> >>>> Christian.
> >>>>
> >>>>> [ 8.298791] [drm] Encoders:
> >>>>> [ 8.301770] [drm] DFP1: INTERNAL_UNIPHY
> >>>>> [ 8.305973] [drm] Connector 1:
> >>>>> [ 8.309043] [drm] DP-2
> >>>>> [ 8.311598] [drm] HPD2
> >>>>> [ 8.314169] [drm] DDC: 0x6440 0x6440 0x6444 0x6444 0x6448 0x6448
> >>>>> 0x644c 0x644c
> >>>>> [ 8.321609] [drm] Encoders:
> >>>>> [ 8.324589] [drm] DFP2: INTERNAL_UNIPHY
> >>>>> [ 8.328793] [drm] Connector 2:
> >>>>> [ 8.331856] [drm] VGA-1
> >>>>> [ 8.342947] [drm] DDC: 0x64d8 0x64d8 0x64dc 0x64dc 0x64e0 0x64e0
> >>>>> 0x64e4 0x64e4
> >>>>> [ 8.350341] [drm] Encoders:
> >>>>> [ 8.353310] [drm] CRT1: INTERNAL_KLDSCP_DAC1
> >>>>> [ 8.358195] BUG: unable to handle kernel NULL pointer dereference at
> >>>>> 0000000000000409
> >>>>> [ 8.409733] [<ffffffffc02024da>] radeon_dp_getsinktype+0x1a/0x30
> >>>>> [radeon]
> >>>>> [ 8.416805] PGD 0
> >>>>> [ 8.418841] Oops: 0000 [#1] SMP
> >>>>> ...
> >>>>>
> >>>>> This patch prevents Kernel failures due to a connector not having a
> >>>>> DDC interface by changing the code so that ddc_bus is always checked
> >>>>> before use.
> >>>>> The problem was first identified using the uBuntu MATE 14.04 LTS
> >>>>> (3.16
> >>>>> series kernels) but not dealt with at that time. On attempting to
> >>>>> install uBuntu MATE 16.04 LTS (4.4 series kernels), it became clear
> >>>>> that using various workarounds to allow the issue to be ignored were
> >>>>> not viable so more effort was put in to sorting the issue
> >>>>> resulting in
> >>>>> this patch. See https://bugs.launchpad.net/bugs/1587885 for more
> >>>>> details.
> >>>>>
> >>>>> Signed-off-by: Mark Fortescue <mark@thurning-instruments.co.uk>
> >>>>> Tested-by: Mark Fortescue <mark@thurning-instruments.co.uk>
> >>>>>
> >>>>> ---
> >>>>>
> >>>>> Looks like Thunderbird may have made a mess of the patch when
> pasting
> >>>>> the contents into the mail message - my alternate mail client (pine)
> >>>>> also has strict line length handling and trashes non-MIME encoded
> >>>>> patches.
> >>>>>
> >>>>> This may not be the correct approach to solving the issue but it is
> >>>>> clean in that it ensures that ddc_bus is never used when NULL
> >>>>> regardless of how the code ended up at the point of use.
> >>>>>
> >>>>> If it helps with back porting, I have patches for the uBuntu 14.04
> >>>>> LTS
> >>>>> [3.13 series], uBuntu MATE 14.04 LTS [3.16 series] and uBuntu 16.04
> >>>>> LTS [4.4 series] kernels.
> >>>>>
> >>>>> Test Hardware:
> >>>>> Commell LV-683 Mini-ITX with onboard AMD Dual-core G-T56N
> >>>>> 4G Ram, 2x1TB Disk, HANNS-G HC194D 1280x1024 LCD (VGA).
> >>>>> 4.8.0-rc5 with patch boots without error.
> >>>>>
> >>>>> drivers/gpu/drm/radeon/atombios_dp.c | 60
> >>>>> ++++++++++++-------
> >>>>> drivers/gpu/drm/radeon/radeon_connectors.c | 46 +++++++-------
> >>>>> drivers/gpu/drm/radeon/radeon_dp_mst.c | 9 ++
> >>>>> drivers/gpu/drm/radeon/radeon_i2c.c | 3
> >>>>> 4 files changed, 73 insertions(+), 45 deletions(-)
> >>>>>
> >>>>> Patch:
> >>>>> diff --git a/drivers/gpu/drm/radeon/atombios_dp.c
> >>>>> b/drivers/gpu/drm/radeon/atombios_dp.c
> >>>>> index cead089a..98b3c0e 100644
> >>>>> --- a/drivers/gpu/drm/radeon/atombios_dp.c
> >>>>> +++ b/drivers/gpu/drm/radeon/atombios_dp.c
> >>>>> @@ -232,6 +232,9 @@ void radeon_dp_aux_init(struct
> radeon_connector
> >>>>> *radeon_connector)
> >>>>> struct radeon_device *rdev = dev->dev_private;
> >>>>> int ret;
> >>>>>
> >>>>> + if (!radeon_connector->ddc_bus)
> >>>>> + return;
> >>>>> +
> >>>>> radeon_connector->ddc_bus->rec.hpd = radeon_connector-
> >hpd.hpd;
> >>>>> radeon_connector->ddc_bus->aux.dev =
> >>>>> radeon_connector->base.kdev;
> >>>>> if (ASIC_IS_DCE5(rdev)) {
> >>>>> @@ -364,6 +367,9 @@ u8 radeon_dp_getsinktype(struct
> radeon_connector
> >>>>> *radeon_connector)
> >>>>> struct drm_device *dev = radeon_connector->base.dev;
> >>>>> struct radeon_device *rdev = dev->dev_private;
> >>>>>
> >>>>> + if (!radeon_connector->ddc_bus)
> >>>>> + return 0;
> >>>>> +
> >>>>> return radeon_dp_encoder_service(rdev,
> >>>>> ATOM_DP_ACTION_GET_SINK_TYPE, 0,
> >>>>> radeon_connector->ddc_bus->rec.i2c_id, 0);
> >>>>> }
> >>>>> @@ -376,6 +382,9 @@ static void radeon_dp_probe_oui(struct
> >>>>> radeon_connector *radeon_connector)
> >>>>> if (!(dig_connector->dpcd[DP_DOWN_STREAM_PORT_COUNT] &
> >>>>> DP_OUI_SUPPORT))
> >>>>> return;
> >>>>>
> >>>>> + if (!radeon_connector->ddc_bus)
> >>>>> + return;
> >>>>> +
> >>>>> if (drm_dp_dpcd_read(&radeon_connector->ddc_bus->aux,
> >>>>> DP_SINK_OUI, buf, 3) == 3)
> >>>>> DRM_DEBUG_KMS("Sink OUI: %02hx%02hx%02hx\n",
> >>>>> buf[0], buf[1], buf[2]);
> >>>>> @@ -391,6 +400,9 @@ bool radeon_dp_getdpcd(struct
> radeon_connector
> >>>>> *radeon_connector)
> >>>>> u8 msg[DP_DPCD_SIZE];
> >>>>> int ret, i;
> >>>>>
> >>>>> + if (!radeon_connector->ddc_bus)
> >>>>> + return false;
> >>>>> +
> >>>>> for (i = 0; i < 7; i++) {
> >>>>> ret = drm_dp_dpcd_read(&radeon_connector->ddc_bus->aux,
> >>>>> DP_DPCD_REV, msg,
> >>>>> DP_DPCD_SIZE);
> >>>>> @@ -428,24 +440,26 @@ int radeon_dp_get_panel_mode(struct
> drm_encoder
> >>>>> *encoder,
> >>>>>
> >>>>> dig_connector = radeon_connector->con_priv;
> >>>>>
> >>>>> - if (dp_bridge != ENCODER_OBJECT_ID_NONE) {
> >>>>> - /* DP bridge chips */
> >>>>> - if (drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux,
> >>>>> - DP_EDP_CONFIGURATION_CAP, &tmp) == 1) {
> >>>>> - if (tmp & 1)
> >>>>> - panel_mode = DP_PANEL_MODE_INTERNAL_DP2_MODE;
> >>>>> - else if ((dp_bridge == ENCODER_OBJECT_ID_NUTMEG) ||
> >>>>> - (dp_bridge == ENCODER_OBJECT_ID_TRAVIS))
> >>>>> - panel_mode = DP_PANEL_MODE_INTERNAL_DP1_MODE;
> >>>>> - else
> >>>>> - panel_mode = DP_PANEL_MODE_EXTERNAL_DP_MODE;
> >>>>> - }
> >>>>> - } else if (connector->connector_type ==
> >>>>> DRM_MODE_CONNECTOR_eDP) {
> >>>>> - /* eDP */
> >>>>> - if (drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux,
> >>>>> - DP_EDP_CONFIGURATION_CAP, &tmp) == 1) {
> >>>>> - if (tmp & 1)
> >>>>> - panel_mode = DP_PANEL_MODE_INTERNAL_DP2_MODE;
> >>>>> + if (radeon_connector->ddc_bus) {
> >>>>> + if (dp_bridge != ENCODER_OBJECT_ID_NONE) {
> >>>>> + /* DP bridge chips */
> >>>>> + if (drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux,
> >>>>> + DP_EDP_CONFIGURATION_CAP, &tmp) == 1) {
> >>>>> + if (tmp & 1)
> >>>>> + panel_mode = DP_PANEL_MODE_INTERNAL_DP2_MODE;
> >>>>> + else if ((dp_bridge == ENCODER_OBJECT_ID_NUTMEG) ||
> >>>>> + (dp_bridge == ENCODER_OBJECT_ID_TRAVIS))
> >>>>> + panel_mode = DP_PANEL_MODE_INTERNAL_DP1_MODE;
> >>>>> + else
> >>>>> + panel_mode = DP_PANEL_MODE_EXTERNAL_DP_MODE;
> >>>>> + }
> >>>>> + } else if (connector->connector_type ==
> >>>>> DRM_MODE_CONNECTOR_eDP) {
> >>>>> + /* eDP */
> >>>>> + if (drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux,
> >>>>> + DP_EDP_CONFIGURATION_CAP, &tmp) == 1) {
> >>>>> + if (tmp & 1)
> >>>>> + panel_mode = DP_PANEL_MODE_INTERNAL_DP2_MODE;
> >>>>> + }
> >>>>> }
> >>>>> }
> >>>>>
> >>>>> @@ -511,6 +525,9 @@ bool radeon_dp_needs_link_train(struct
> >>>>> radeon_connector *radeon_connector)
> >>>>> u8 link_status[DP_LINK_STATUS_SIZE];
> >>>>> struct radeon_connector_atom_dig *dig =
> >>>>> radeon_connector->con_priv;
> >>>>>
> >>>>> + if (!radeon_connector->ddc_bus)
> >>>>> + return false;
> >>>>> +
> >>>>> if
> >>>>> (drm_dp_dpcd_read_link_status(&radeon_connector->ddc_bus-
> >aux,
> >>>>> link_status)
> >>>>> <= 0)
> >>>>> return false;
> >>>>> @@ -531,7 +548,7 @@ void radeon_dp_set_rx_power_state(struct
> >>>>> drm_connector *connector,
> >>>>> dig_connector = radeon_connector->con_priv;
> >>>>>
> >>>>> /* power up/down the sink */
> >>>>> - if (dig_connector->dpcd[0] >= 0x11) {
> >>>>> + if (radeon_connector->ddc_bus && dig_connector->dpcd[0] >=
> >>>>> 0x11) {
> >>>>> drm_dp_dpcd_writeb(&radeon_connector->ddc_bus->aux,
> >>>>> DP_SET_POWER, power_state);
> >>>>> usleep_range(1000, 2000);
> >>>>> @@ -834,7 +851,8 @@ void radeon_dp_link_train(struct
> drm_encoder
> >>>>> *encoder,
> >>>>> else
> >>>>> dp_info.enc_id |= ATOM_DP_CONFIG_LINK_A;
> >>>>>
> >>>>> - if (drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux,
> >>>>> DP_MAX_LANE_COUNT, &tmp)
> >>>>> + if (radeon_connector->ddc_bus &&
> >>>>> + drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux,
> >>>>> DP_MAX_LANE_COUNT, &tmp)
> >>>>> == 1) {
> >>>>> if (ASIC_IS_DCE5(rdev) && (tmp & DP_TPS3_SUPPORTED))
> >>>>> dp_info.tp3_supported = true;
> >>>>> @@ -850,7 +868,7 @@ void radeon_dp_link_train(struct
> drm_encoder
> >>>>> *encoder,
> >>>>> dp_info.connector = connector;
> >>>>> dp_info.dp_lane_count = dig_connector->dp_lane_count;
> >>>>> dp_info.dp_clock = dig_connector->dp_clock;
> >>>>> - dp_info.aux = &radeon_connector->ddc_bus->aux;
> >>>>> + dp_info.aux = radeon_connector->ddc_bus ?
> >>>>> &radeon_connector->ddc_bus->aux : 0;
> >>>>>
> >>>>> if (radeon_dp_link_train_init(&dp_info))
> >>>>> goto done;
> >>>>> diff --git a/drivers/gpu/drm/radeon/radeon_connectors.c
> >>>>> b/drivers/gpu/drm/radeon/radeon_connectors.c
> >>>>> index b79f3b0..cec30c9 100644
> >>>>> --- a/drivers/gpu/drm/radeon/radeon_connectors.c
> >>>>> +++ b/drivers/gpu/drm/radeon/radeon_connectors.c
> >>>>> @@ -328,31 +328,32 @@ static void
> radeon_connector_get_edid(struct
> >>>>> drm_connector *connector)
> >>>>> if (radeon_connector->router.ddc_valid)
> >>>>> radeon_router_select_ddc_port(radeon_connector);
> >>>>>
> >>>>> - if
> >>>>>
> ((radeon_connector_encoder_get_dp_bridge_encoder_id(connector) !=
> >>>>> - ENCODER_OBJECT_ID_NONE) &&
> >>>>> - radeon_connector->ddc_bus->has_aux) {
> >>>>> - radeon_connector->edid = drm_get_edid(connector,
> >>>>> - &radeon_connector->ddc_bus->aux.ddc);
> >>>>> - } else if ((connector->connector_type ==
> >>>>> DRM_MODE_CONNECTOR_DisplayPort) ||
> >>>>> - (connector->connector_type ==
> DRM_MODE_CONNECTOR_eDP)) {
> >>>>> - struct radeon_connector_atom_dig *dig =
> >>>>> radeon_connector->con_priv;
> >>>>> -
> >>>>> - if ((dig->dp_sink_type ==
> CONNECTOR_OBJECT_ID_DISPLAYPORT ||
> >>>>> - dig->dp_sink_type == CONNECTOR_OBJECT_ID_eDP) &&
> >>>>> - radeon_connector->ddc_bus->has_aux)
> >>>>> - radeon_connector->edid =
> >>>>> drm_get_edid(&radeon_connector->base,
> >>>>> + if (radeon_connector->ddc_bus) {
> >>>>> + if
> >>>>>
> ((radeon_connector_encoder_get_dp_bridge_encoder_id(connector) !=
> >>>>> + ENCODER_OBJECT_ID_NONE) &&
> >>>>> + radeon_connector->ddc_bus->has_aux) {
> >>>>> + radeon_connector->edid = drm_get_edid(connector,
> >>>>> &radeon_connector->ddc_bus->aux.ddc);
> >>>>> - else if (radeon_connector->ddc_bus)
> >>>>> + } else if ((connector->connector_type ==
> >>>>> DRM_MODE_CONNECTOR_DisplayPort) ||
> >>>>> + (connector->connector_type ==
> >>>>> DRM_MODE_CONNECTOR_eDP)) {
> >>>>> + struct radeon_connector_atom_dig *dig =
> >>>>> radeon_connector->con_priv;
> >>>>> +
> >>>>> + if ((dig->dp_sink_type ==
> >>>>> CONNECTOR_OBJECT_ID_DISPLAYPORT ||
> >>>>> + dig->dp_sink_type == CONNECTOR_OBJECT_ID_eDP) &&
> >>>>> + radeon_connector->ddc_bus->has_aux)
> >>>>> + radeon_connector->edid =
> >>>>> drm_get_edid(&radeon_connector->base,
> >>>>> + &radeon_connector->ddc_bus->aux.ddc);
> >>>>> + else
> >>>>> + radeon_connector->edid =
> >>>>> drm_get_edid(&radeon_connector->base,
> >>>>> + &radeon_connector->ddc_bus->adapter);
> >>>>> + } else if (vga_switcheroo_handler_flags() &
> >>>>> VGA_SWITCHEROO_CAN_SWITCH_DDC &&
> >>>>> + connector->connector_type ==
> >>>>> DRM_MODE_CONNECTOR_LVDS) {
> >>>>> + radeon_connector->edid =
> >>>>> drm_get_edid_switcheroo(&radeon_connector->base,
> >>>>> + &radeon_connector->ddc_bus->adapter);
> >>>>> + } else {
> >>>>> radeon_connector->edid =
> >>>>> drm_get_edid(&radeon_connector->base,
> >>>>> &radeon_connector->ddc_bus->adapter);
> >>>>> - } else if (vga_switcheroo_handler_flags() &
> >>>>> VGA_SWITCHEROO_CAN_SWITCH_DDC &&
> >>>>> - connector->connector_type ==
> DRM_MODE_CONNECTOR_LVDS &&
> >>>>> - radeon_connector->ddc_bus) {
> >>>>> - radeon_connector->edid =
> >>>>> drm_get_edid_switcheroo(&radeon_connector->base,
> >>>>> - &radeon_connector->ddc_bus->adapter);
> >>>>> - } else if (radeon_connector->ddc_bus) {
> >>>>> - radeon_connector->edid =
> >>>>> drm_get_edid(&radeon_connector->base,
> >>>>> - &radeon_connector->ddc_bus->adapter);
> >>>>> + }
> >>>>> }
> >>>>>
> >>>>> if (!radeon_connector->edid) {
> >>>>> @@ -1312,6 +1313,7 @@ radeon_dvi_detect(struct drm_connector
> >>>>> *connector, bool force)
> >>>>> continue;
> >>>>> list_radeon_connector =
> >>>>> to_radeon_connector(list_connector);
> >>>>> if (list_radeon_connector->shared_ddc &&
> >>>>> + radeon_connector->ddc_bus &&
> >>>>> (list_radeon_connector->ddc_bus->rec.i2c_id ==
> >>>>> radeon_connector->ddc_bus->rec.i2c_id)) {
> >>>>> /* cases where both connectors are
> >>>>> digital */
> >>>>> diff --git a/drivers/gpu/drm/radeon/radeon_dp_mst.c
> >>>>> b/drivers/gpu/drm/radeon/radeon_dp_mst.c
> >>>>> index de504ea..89e91f3 100644
> >>>>> --- a/drivers/gpu/drm/radeon/radeon_dp_mst.c
> >>>>> +++ b/drivers/gpu/drm/radeon/radeon_dp_mst.c
> >>>>> @@ -661,7 +661,7 @@ radeon_dp_mst_init(struct radeon_connector
> >>>>> *radeon_connector)
> >>>>> {
> >>>>> struct drm_device *dev = radeon_connector->base.dev;
> >>>>>
> >>>>> - if (!radeon_connector->ddc_bus->has_aux)
> >>>>> + if (!radeon_connector->ddc_bus ||
> >>>>> !radeon_connector->ddc_bus->has_aux)
> >>>>> return 0;
> >>>>>
> >>>>> radeon_connector->mst_mgr.cbs = &mst_cbs;
> >>>>> @@ -688,6 +688,9 @@ radeon_dp_mst_probe(struct
> radeon_connector
> >>>>> *radeon_connector)
> >>>>> if (dig_connector->dpcd[DP_DPCD_REV] < 0x12)
> >>>>> return 0;
> >>>>>
> >>>>> + if (!radeon_connector->ddc_bus ||
> >>>>> !radeon_connector->ddc_bus->has_aux)
> >>>>> + return 0;
> >>>>> +
> >>>>> ret = drm_dp_dpcd_read(&radeon_connector->ddc_bus->aux,
> >>>>> DP_MSTM_CAP, msg,
> >>>>> 1);
> >>>>> if (ret) {
> >>>>> @@ -711,7 +714,9 @@ radeon_dp_mst_check_status(struct
> >>>>> radeon_connector
> >>>>> *radeon_connector)
> >>>>> struct radeon_connector_atom_dig *dig_connector =
> >>>>> radeon_connector->con_priv;
> >>>>> int retry;
> >>>>>
> >>>>> - if (dig_connector->is_mst) {
> >>>>> + if (dig_connector->is_mst &&
> >>>>> + radeon_connector->ddc_bus &&
> >>>>> + radeon_connector->ddc_bus->has_aux) {
> >>>>> u8 esi[16] = { 0 };
> >>>>> int dret;
> >>>>> int ret = 0;
> >>>>> diff --git a/drivers/gpu/drm/radeon/radeon_i2c.c
> >>>>> b/drivers/gpu/drm/radeon/radeon_i2c.c
> >>>>> index 9590bcd..c18f7ba 100644
> >>>>> --- a/drivers/gpu/drm/radeon/radeon_i2c.c
> >>>>> +++ b/drivers/gpu/drm/radeon/radeon_i2c.c
> >>>>> @@ -63,6 +63,9 @@ bool radeon_ddc_probe(struct
> radeon_connector
> >>>>> *radeon_connector, bool use_aux)
> >>>>> if (radeon_connector->router.ddc_valid)
> >>>>> radeon_router_select_ddc_port(radeon_connector);
> >>>>>
> >>>>> + if (!radeon_connector->ddc_bus)
> >>>>> + return false;
> >>>>> +
> >>>>> if (use_aux) {
> >>>>> ret = i2c_transfer(&radeon_connector->ddc_bus->aux.ddc,
> >>>>> msgs, 2);
> >>>>> } else {
> >>>>> _______________________________________________
> >>>>> dri-devel mailing list
> >>>>> dri-devel@lists.freedesktop.org
> >>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >>>>
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 001/001] drivers/gpu/radeon: NULL pointer deference workaround
2016-09-09 13:20 ` Deucher, Alexander
@ 2016-09-09 13:32 ` Mark Fortescue
0 siblings, 0 replies; 8+ messages in thread
From: Mark Fortescue @ 2016-09-09 13:32 UTC (permalink / raw)
To: Deucher, Alexander
Cc: Koenig, Christian, David Airlie, linux-kernel, dri-devel
On 09/09/16 13:20, Deucher, Alexander wrote:
>> -----Original Message-----
>> From: Koenig, Christian
>> Sent: Friday, September 09, 2016 4:16 AM
>> To: Mark Fortescue; Koenig, Christian; David Airlie
>> Cc: Deucher, Alexander; linux-kernel@vger.kernel.org; dri-
>> devel@lists.freedesktop.org
>> Subject: Re: [PATCH 001/001] drivers/gpu/radeon: NULL pointer deference
>> workaround
>>
>>>> On this motherboard, DP-1 is a single channel 18bit LVDS LCD panel
>>>> interface and DP-2 is a DVI interface (which I can connect to my
>>>> monitor if testing this is useful). There are no displayport connectors.
>>>
>>> Yeah, but from the driver point of view there are only DP connectors on
>>> the chip. The LVDS and DVI are probably realized with external DP to
>>> whatever converter ICs.
>>>
>>>
>>> That would explain why some similar boards have 24bit LVDS and others
>>> only 18bit LVDS.
>>
>> It could be that this is actually a configuration we don't support yet
>> with radeon and/or the display stack.
>>
>> E.g. that the connector only uses the displayport LVDS lanes, but not
>> actual the displayport protocol to train those lanes.
>>
>> Instead it rather expects a fixed training which is configured somewhere
>> in the BIOS.
>>
>> Anyway Alex needs to take a look, he knows displayport much better than
>> I do (and hates it passionately :).
>
> Generally these sorts of setups are supported by exposing an LVDS or eDP connector rather than a regular DP. In the case of LVDS and eDP, we have code to fallback to a hardcoded EDID in the vbios if DDC is not available. I'll take a closer look when I'm back.
>
> Alex
Thanks Alex,
I look forward to hearing from you when you have finished recovering
from your holiday and the associated work backlog :).
I raised a bug (https://bugs.freedesktop.org/show_bug.cgi?id=97637) -
probably on the wrong component but fixing that should be easy.
It has a more complete copy of the Opps.
Regards
Mark.
>
>>
>> Cheers,
>> Christian.
>>
>> Am 08.09.2016 um 13:18 schrieb Mark Fortescue:
>>> On 08/09/16 11:25, Christian König wrote:
>>>> Am 08.09.2016 um 11:09 schrieb Mark Fortescue:
>>>>> Hi Christian,
>>>>>
>>>>> Thank you for the feedback.
>>>>>
>>>>> On 08/09/16 08:14, Christian König wrote:
>>>>>> Am 07.09.2016 um 19:38 schrieb Mark Fortescue:
>>>>>>>
>>>>>>> On an LV-683 (AMD Dual-core G-T56N) Mini-ITX board, I get a Kernel
>>>>>>> Oops because Connector 0 (LCD Panel interface) does not have DDC.
>>>>>>
>>>>>> I'm not an expert on this, but that is really odd cause even LCD
>>>>>> Panels
>>>>>> should have a DDC interface.
>>>>>>
>>>>>>>
>>>>>>> Ubuntu 16.04 LTS Kernel (4.4 series):
>>>>>>>
>>>>>>> ...
>>>>>>> [ 8.262990] [drm] ib test on ring 5 succeeded
>>>>>>> [ 8.288897] [drm] Radeon Display Connectors
>>>>>>> [ 8.293175] [drm] Connector 0:
>>>>>>> [ 8.296252] [drm] DP-1
>>>>>>
>>>>>> Especially since the BIOS claims that this is a displayport connector
>>>>>> and there is no physical way to have a DP without an DDC as far as I
>>>>>> know.
>>>>>>
>>>>>> Please open a bug report on FDO and attach you BIOS image.
>>>>>
>>>>> FDO ? I am not familiar with this. Please can you enlighten me.
>>>>>
>>>>
>>>> See here: http://bugs.freedesktop.org/
>>>>
>>>>> I do not have a BIOS image so will need some assistance in
>>>>> understanding what is required here and how I extract the BIOS
>>>>> information you are after.
>>>>>
>>>>
>>>> Sorry my fault. Mullins is an APU, so you don't have a dedicated video
>>>> BIOS. As usually I didn't got enough sleep :) But please open up a bug
>>>> report anyway.
>>>
>>> Know the problem of being awake too long :). I will raise a bug.
>>>
>>>>
>>>>> On this motherboard, DP-1 is a single channel 18bit LVDS LCD panel
>>>>> interface and DP-2 is a DVI interface (which I can connect to my
>>>>> monitor if testing this is useful). There are no displayport
>>>>> connectors.
>>>>
>>>> Yeah, but from the driver point of view there are only DP connectors on
>>>> the chip. The LVDS and DVI are probably realized with external DP to
>>>> whatever converter ICs.
>>>>
>>>
>>> That would explain why some similar boards have 24bit LVDS and others
>>> only 18bit LVDS.
>>>
>>>>>
>>>>> On industrial motherboards, I have noticed that it is not uncommon to
>>>>> hard code the information for the LCD panel into the BIOS so no DDC is
>>>>> required. In this case, there is no LCD panel connected to the
>>>>> interface anyway.
>>>>>
>>>>
>>>> That is correct and as far as I know well supported by Radeon, but the
>>>> crux is there should still be a DDC channel even if there isn't anything
>>>> attached to it.
>>>>
>>>> See with displayport you got four LVDS channels to submit the actual
>>>> picture and an AUX channel to configure and query the device. The DDC is
>>>> just represented as certain packets over the AUX channel.
>>>>
>>>> If the AUX channel doesn't work or isn't connect then the link training
>>>> wouldn't be possible as well and so you wouldn't be able to get any
>>>> picture on the LVDS.
>>>>
>>>
>>> Interesting.
>>>
>>>>> See http://www.commell.com.tw/product/SBC/LV-683.HTM for more
>>>>> information on the board. Looking at the web site, there is a BIOS
>>>>> image available form Commell if that is of use.
>>>>
>>>> Alex clearly needs to take a look on this. I think for the time being
>>>> you could hack together a patch which just ignores DP connectors during
>>>> probing if they don't have an associated DDC instead of changing the
>>>> code everywhere the DDC object is required.
>>>>
>>>
>>> I will try to find the time to look at this in more detail. GPU/DRM is
>>> not something I have done any real work on in the past so I do not
>>> know how it all fits together. The overkill on the code changes is the
>>> result, as that way I don't need to understand where and why the
>>> helper function (radeon_dp_getsinktype) gets called or what other code
>>> paths can be executed that could fail.
>>>
>>> I will wait for Alex to have a think about it and respond before I do
>>> any more - that way I can finish the work I am paid to do first :).
>>>
>>>> Regards,
>>>> Christian.
>>>>
>>> Regards
>>> Mark.
>>>>>
>>>>>>
>>>>>> Alex can probably take a look when he's back from vacation.
>>>>>>
>>>>>> Regards,
>>>>>> Christian.
>>>>>>
>>>>>>> [ 8.298791] [drm] Encoders:
>>>>>>> [ 8.301770] [drm] DFP1: INTERNAL_UNIPHY
>>>>>>> [ 8.305973] [drm] Connector 1:
>>>>>>> [ 8.309043] [drm] DP-2
>>>>>>> [ 8.311598] [drm] HPD2
>>>>>>> [ 8.314169] [drm] DDC: 0x6440 0x6440 0x6444 0x6444 0x6448 0x6448
>>>>>>> 0x644c 0x644c
>>>>>>> [ 8.321609] [drm] Encoders:
>>>>>>> [ 8.324589] [drm] DFP2: INTERNAL_UNIPHY
>>>>>>> [ 8.328793] [drm] Connector 2:
>>>>>>> [ 8.331856] [drm] VGA-1
>>>>>>> [ 8.342947] [drm] DDC: 0x64d8 0x64d8 0x64dc 0x64dc 0x64e0 0x64e0
>>>>>>> 0x64e4 0x64e4
>>>>>>> [ 8.350341] [drm] Encoders:
>>>>>>> [ 8.353310] [drm] CRT1: INTERNAL_KLDSCP_DAC1
>>>>>>> [ 8.358195] BUG: unable to handle kernel NULL pointer dereference at
>>>>>>> 0000000000000409
>>>>>>> [ 8.409733] [<ffffffffc02024da>] radeon_dp_getsinktype+0x1a/0x30
>>>>>>> [radeon]
>>>>>>> [ 8.416805] PGD 0
>>>>>>> [ 8.418841] Oops: 0000 [#1] SMP
>>>>>>> ...
>>>>>>>
>>>>>>> This patch prevents Kernel failures due to a connector not having a
>>>>>>> DDC interface by changing the code so that ddc_bus is always checked
>>>>>>> before use.
>>>>>>> The problem was first identified using the uBuntu MATE 14.04 LTS
>>>>>>> (3.16
>>>>>>> series kernels) but not dealt with at that time. On attempting to
>>>>>>> install uBuntu MATE 16.04 LTS (4.4 series kernels), it became clear
>>>>>>> that using various workarounds to allow the issue to be ignored were
>>>>>>> not viable so more effort was put in to sorting the issue
>>>>>>> resulting in
>>>>>>> this patch. See https://bugs.launchpad.net/bugs/1587885 for more
>>>>>>> details.
>>>>>>>
>>>>>>> Signed-off-by: Mark Fortescue <mark@thurning-instruments.co.uk>
>>>>>>> Tested-by: Mark Fortescue <mark@thurning-instruments.co.uk>
>>>>>>>
>>>>>>> ---
>>>>>>>
>>>>>>> Looks like Thunderbird may have made a mess of the patch when
>> pasting
>>>>>>> the contents into the mail message - my alternate mail client (pine)
>>>>>>> also has strict line length handling and trashes non-MIME encoded
>>>>>>> patches.
>>>>>>>
>>>>>>> This may not be the correct approach to solving the issue but it is
>>>>>>> clean in that it ensures that ddc_bus is never used when NULL
>>>>>>> regardless of how the code ended up at the point of use.
>>>>>>>
>>>>>>> If it helps with back porting, I have patches for the uBuntu 14.04
>>>>>>> LTS
>>>>>>> [3.13 series], uBuntu MATE 14.04 LTS [3.16 series] and uBuntu 16.04
>>>>>>> LTS [4.4 series] kernels.
>>>>>>>
>>>>>>> Test Hardware:
>>>>>>> Commell LV-683 Mini-ITX with onboard AMD Dual-core G-T56N
>>>>>>> 4G Ram, 2x1TB Disk, HANNS-G HC194D 1280x1024 LCD (VGA).
>>>>>>> 4.8.0-rc5 with patch boots without error.
>>>>>>>
>>>>>>> drivers/gpu/drm/radeon/atombios_dp.c | 60
>>>>>>> ++++++++++++-------
>>>>>>> drivers/gpu/drm/radeon/radeon_connectors.c | 46 +++++++-------
>>>>>>> drivers/gpu/drm/radeon/radeon_dp_mst.c | 9 ++
>>>>>>> drivers/gpu/drm/radeon/radeon_i2c.c | 3
>>>>>>> 4 files changed, 73 insertions(+), 45 deletions(-)
>>>>>>>
>>>>>>> Patch:
>>>>>>> diff --git a/drivers/gpu/drm/radeon/atombios_dp.c
>>>>>>> b/drivers/gpu/drm/radeon/atombios_dp.c
>>>>>>> index cead089a..98b3c0e 100644
>>>>>>> --- a/drivers/gpu/drm/radeon/atombios_dp.c
>>>>>>> +++ b/drivers/gpu/drm/radeon/atombios_dp.c
>>>>>>> @@ -232,6 +232,9 @@ void radeon_dp_aux_init(struct
>> radeon_connector
>>>>>>> *radeon_connector)
>>>>>>> struct radeon_device *rdev = dev->dev_private;
>>>>>>> int ret;
>>>>>>>
>>>>>>> + if (!radeon_connector->ddc_bus)
>>>>>>> + return;
>>>>>>> +
>>>>>>> radeon_connector->ddc_bus->rec.hpd = radeon_connector-
>>> hpd.hpd;
>>>>>>> radeon_connector->ddc_bus->aux.dev =
>>>>>>> radeon_connector->base.kdev;
>>>>>>> if (ASIC_IS_DCE5(rdev)) {
>>>>>>> @@ -364,6 +367,9 @@ u8 radeon_dp_getsinktype(struct
>> radeon_connector
>>>>>>> *radeon_connector)
>>>>>>> struct drm_device *dev = radeon_connector->base.dev;
>>>>>>> struct radeon_device *rdev = dev->dev_private;
>>>>>>>
>>>>>>> + if (!radeon_connector->ddc_bus)
>>>>>>> + return 0;
>>>>>>> +
>>>>>>> return radeon_dp_encoder_service(rdev,
>>>>>>> ATOM_DP_ACTION_GET_SINK_TYPE, 0,
>>>>>>> radeon_connector->ddc_bus->rec.i2c_id, 0);
>>>>>>> }
>>>>>>> @@ -376,6 +382,9 @@ static void radeon_dp_probe_oui(struct
>>>>>>> radeon_connector *radeon_connector)
>>>>>>> if (!(dig_connector->dpcd[DP_DOWN_STREAM_PORT_COUNT] &
>>>>>>> DP_OUI_SUPPORT))
>>>>>>> return;
>>>>>>>
>>>>>>> + if (!radeon_connector->ddc_bus)
>>>>>>> + return;
>>>>>>> +
>>>>>>> if (drm_dp_dpcd_read(&radeon_connector->ddc_bus->aux,
>>>>>>> DP_SINK_OUI, buf, 3) == 3)
>>>>>>> DRM_DEBUG_KMS("Sink OUI: %02hx%02hx%02hx\n",
>>>>>>> buf[0], buf[1], buf[2]);
>>>>>>> @@ -391,6 +400,9 @@ bool radeon_dp_getdpcd(struct
>> radeon_connector
>>>>>>> *radeon_connector)
>>>>>>> u8 msg[DP_DPCD_SIZE];
>>>>>>> int ret, i;
>>>>>>>
>>>>>>> + if (!radeon_connector->ddc_bus)
>>>>>>> + return false;
>>>>>>> +
>>>>>>> for (i = 0; i < 7; i++) {
>>>>>>> ret = drm_dp_dpcd_read(&radeon_connector->ddc_bus->aux,
>>>>>>> DP_DPCD_REV, msg,
>>>>>>> DP_DPCD_SIZE);
>>>>>>> @@ -428,24 +440,26 @@ int radeon_dp_get_panel_mode(struct
>> drm_encoder
>>>>>>> *encoder,
>>>>>>>
>>>>>>> dig_connector = radeon_connector->con_priv;
>>>>>>>
>>>>>>> - if (dp_bridge != ENCODER_OBJECT_ID_NONE) {
>>>>>>> - /* DP bridge chips */
>>>>>>> - if (drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux,
>>>>>>> - DP_EDP_CONFIGURATION_CAP, &tmp) == 1) {
>>>>>>> - if (tmp & 1)
>>>>>>> - panel_mode = DP_PANEL_MODE_INTERNAL_DP2_MODE;
>>>>>>> - else if ((dp_bridge == ENCODER_OBJECT_ID_NUTMEG) ||
>>>>>>> - (dp_bridge == ENCODER_OBJECT_ID_TRAVIS))
>>>>>>> - panel_mode = DP_PANEL_MODE_INTERNAL_DP1_MODE;
>>>>>>> - else
>>>>>>> - panel_mode = DP_PANEL_MODE_EXTERNAL_DP_MODE;
>>>>>>> - }
>>>>>>> - } else if (connector->connector_type ==
>>>>>>> DRM_MODE_CONNECTOR_eDP) {
>>>>>>> - /* eDP */
>>>>>>> - if (drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux,
>>>>>>> - DP_EDP_CONFIGURATION_CAP, &tmp) == 1) {
>>>>>>> - if (tmp & 1)
>>>>>>> - panel_mode = DP_PANEL_MODE_INTERNAL_DP2_MODE;
>>>>>>> + if (radeon_connector->ddc_bus) {
>>>>>>> + if (dp_bridge != ENCODER_OBJECT_ID_NONE) {
>>>>>>> + /* DP bridge chips */
>>>>>>> + if (drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux,
>>>>>>> + DP_EDP_CONFIGURATION_CAP, &tmp) == 1) {
>>>>>>> + if (tmp & 1)
>>>>>>> + panel_mode = DP_PANEL_MODE_INTERNAL_DP2_MODE;
>>>>>>> + else if ((dp_bridge == ENCODER_OBJECT_ID_NUTMEG) ||
>>>>>>> + (dp_bridge == ENCODER_OBJECT_ID_TRAVIS))
>>>>>>> + panel_mode = DP_PANEL_MODE_INTERNAL_DP1_MODE;
>>>>>>> + else
>>>>>>> + panel_mode = DP_PANEL_MODE_EXTERNAL_DP_MODE;
>>>>>>> + }
>>>>>>> + } else if (connector->connector_type ==
>>>>>>> DRM_MODE_CONNECTOR_eDP) {
>>>>>>> + /* eDP */
>>>>>>> + if (drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux,
>>>>>>> + DP_EDP_CONFIGURATION_CAP, &tmp) == 1) {
>>>>>>> + if (tmp & 1)
>>>>>>> + panel_mode = DP_PANEL_MODE_INTERNAL_DP2_MODE;
>>>>>>> + }
>>>>>>> }
>>>>>>> }
>>>>>>>
>>>>>>> @@ -511,6 +525,9 @@ bool radeon_dp_needs_link_train(struct
>>>>>>> radeon_connector *radeon_connector)
>>>>>>> u8 link_status[DP_LINK_STATUS_SIZE];
>>>>>>> struct radeon_connector_atom_dig *dig =
>>>>>>> radeon_connector->con_priv;
>>>>>>>
>>>>>>> + if (!radeon_connector->ddc_bus)
>>>>>>> + return false;
>>>>>>> +
>>>>>>> if
>>>>>>> (drm_dp_dpcd_read_link_status(&radeon_connector->ddc_bus-
>>> aux,
>>>>>>> link_status)
>>>>>>> <= 0)
>>>>>>> return false;
>>>>>>> @@ -531,7 +548,7 @@ void radeon_dp_set_rx_power_state(struct
>>>>>>> drm_connector *connector,
>>>>>>> dig_connector = radeon_connector->con_priv;
>>>>>>>
>>>>>>> /* power up/down the sink */
>>>>>>> - if (dig_connector->dpcd[0] >= 0x11) {
>>>>>>> + if (radeon_connector->ddc_bus && dig_connector->dpcd[0] >=
>>>>>>> 0x11) {
>>>>>>> drm_dp_dpcd_writeb(&radeon_connector->ddc_bus->aux,
>>>>>>> DP_SET_POWER, power_state);
>>>>>>> usleep_range(1000, 2000);
>>>>>>> @@ -834,7 +851,8 @@ void radeon_dp_link_train(struct
>> drm_encoder
>>>>>>> *encoder,
>>>>>>> else
>>>>>>> dp_info.enc_id |= ATOM_DP_CONFIG_LINK_A;
>>>>>>>
>>>>>>> - if (drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux,
>>>>>>> DP_MAX_LANE_COUNT, &tmp)
>>>>>>> + if (radeon_connector->ddc_bus &&
>>>>>>> + drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux,
>>>>>>> DP_MAX_LANE_COUNT, &tmp)
>>>>>>> == 1) {
>>>>>>> if (ASIC_IS_DCE5(rdev) && (tmp & DP_TPS3_SUPPORTED))
>>>>>>> dp_info.tp3_supported = true;
>>>>>>> @@ -850,7 +868,7 @@ void radeon_dp_link_train(struct
>> drm_encoder
>>>>>>> *encoder,
>>>>>>> dp_info.connector = connector;
>>>>>>> dp_info.dp_lane_count = dig_connector->dp_lane_count;
>>>>>>> dp_info.dp_clock = dig_connector->dp_clock;
>>>>>>> - dp_info.aux = &radeon_connector->ddc_bus->aux;
>>>>>>> + dp_info.aux = radeon_connector->ddc_bus ?
>>>>>>> &radeon_connector->ddc_bus->aux : 0;
>>>>>>>
>>>>>>> if (radeon_dp_link_train_init(&dp_info))
>>>>>>> goto done;
>>>>>>> diff --git a/drivers/gpu/drm/radeon/radeon_connectors.c
>>>>>>> b/drivers/gpu/drm/radeon/radeon_connectors.c
>>>>>>> index b79f3b0..cec30c9 100644
>>>>>>> --- a/drivers/gpu/drm/radeon/radeon_connectors.c
>>>>>>> +++ b/drivers/gpu/drm/radeon/radeon_connectors.c
>>>>>>> @@ -328,31 +328,32 @@ static void
>> radeon_connector_get_edid(struct
>>>>>>> drm_connector *connector)
>>>>>>> if (radeon_connector->router.ddc_valid)
>>>>>>> radeon_router_select_ddc_port(radeon_connector);
>>>>>>>
>>>>>>> - if
>>>>>>>
>> ((radeon_connector_encoder_get_dp_bridge_encoder_id(connector) !=
>>>>>>> - ENCODER_OBJECT_ID_NONE) &&
>>>>>>> - radeon_connector->ddc_bus->has_aux) {
>>>>>>> - radeon_connector->edid = drm_get_edid(connector,
>>>>>>> - &radeon_connector->ddc_bus->aux.ddc);
>>>>>>> - } else if ((connector->connector_type ==
>>>>>>> DRM_MODE_CONNECTOR_DisplayPort) ||
>>>>>>> - (connector->connector_type ==
>> DRM_MODE_CONNECTOR_eDP)) {
>>>>>>> - struct radeon_connector_atom_dig *dig =
>>>>>>> radeon_connector->con_priv;
>>>>>>> -
>>>>>>> - if ((dig->dp_sink_type ==
>> CONNECTOR_OBJECT_ID_DISPLAYPORT ||
>>>>>>> - dig->dp_sink_type == CONNECTOR_OBJECT_ID_eDP) &&
>>>>>>> - radeon_connector->ddc_bus->has_aux)
>>>>>>> - radeon_connector->edid =
>>>>>>> drm_get_edid(&radeon_connector->base,
>>>>>>> + if (radeon_connector->ddc_bus) {
>>>>>>> + if
>>>>>>>
>> ((radeon_connector_encoder_get_dp_bridge_encoder_id(connector) !=
>>>>>>> + ENCODER_OBJECT_ID_NONE) &&
>>>>>>> + radeon_connector->ddc_bus->has_aux) {
>>>>>>> + radeon_connector->edid = drm_get_edid(connector,
>>>>>>> &radeon_connector->ddc_bus->aux.ddc);
>>>>>>> - else if (radeon_connector->ddc_bus)
>>>>>>> + } else if ((connector->connector_type ==
>>>>>>> DRM_MODE_CONNECTOR_DisplayPort) ||
>>>>>>> + (connector->connector_type ==
>>>>>>> DRM_MODE_CONNECTOR_eDP)) {
>>>>>>> + struct radeon_connector_atom_dig *dig =
>>>>>>> radeon_connector->con_priv;
>>>>>>> +
>>>>>>> + if ((dig->dp_sink_type ==
>>>>>>> CONNECTOR_OBJECT_ID_DISPLAYPORT ||
>>>>>>> + dig->dp_sink_type == CONNECTOR_OBJECT_ID_eDP) &&
>>>>>>> + radeon_connector->ddc_bus->has_aux)
>>>>>>> + radeon_connector->edid =
>>>>>>> drm_get_edid(&radeon_connector->base,
>>>>>>> + &radeon_connector->ddc_bus->aux.ddc);
>>>>>>> + else
>>>>>>> + radeon_connector->edid =
>>>>>>> drm_get_edid(&radeon_connector->base,
>>>>>>> + &radeon_connector->ddc_bus->adapter);
>>>>>>> + } else if (vga_switcheroo_handler_flags() &
>>>>>>> VGA_SWITCHEROO_CAN_SWITCH_DDC &&
>>>>>>> + connector->connector_type ==
>>>>>>> DRM_MODE_CONNECTOR_LVDS) {
>>>>>>> + radeon_connector->edid =
>>>>>>> drm_get_edid_switcheroo(&radeon_connector->base,
>>>>>>> + &radeon_connector->ddc_bus->adapter);
>>>>>>> + } else {
>>>>>>> radeon_connector->edid =
>>>>>>> drm_get_edid(&radeon_connector->base,
>>>>>>> &radeon_connector->ddc_bus->adapter);
>>>>>>> - } else if (vga_switcheroo_handler_flags() &
>>>>>>> VGA_SWITCHEROO_CAN_SWITCH_DDC &&
>>>>>>> - connector->connector_type ==
>> DRM_MODE_CONNECTOR_LVDS &&
>>>>>>> - radeon_connector->ddc_bus) {
>>>>>>> - radeon_connector->edid =
>>>>>>> drm_get_edid_switcheroo(&radeon_connector->base,
>>>>>>> - &radeon_connector->ddc_bus->adapter);
>>>>>>> - } else if (radeon_connector->ddc_bus) {
>>>>>>> - radeon_connector->edid =
>>>>>>> drm_get_edid(&radeon_connector->base,
>>>>>>> - &radeon_connector->ddc_bus->adapter);
>>>>>>> + }
>>>>>>> }
>>>>>>>
>>>>>>> if (!radeon_connector->edid) {
>>>>>>> @@ -1312,6 +1313,7 @@ radeon_dvi_detect(struct drm_connector
>>>>>>> *connector, bool force)
>>>>>>> continue;
>>>>>>> list_radeon_connector =
>>>>>>> to_radeon_connector(list_connector);
>>>>>>> if (list_radeon_connector->shared_ddc &&
>>>>>>> + radeon_connector->ddc_bus &&
>>>>>>> (list_radeon_connector->ddc_bus->rec.i2c_id ==
>>>>>>> radeon_connector->ddc_bus->rec.i2c_id)) {
>>>>>>> /* cases where both connectors are
>>>>>>> digital */
>>>>>>> diff --git a/drivers/gpu/drm/radeon/radeon_dp_mst.c
>>>>>>> b/drivers/gpu/drm/radeon/radeon_dp_mst.c
>>>>>>> index de504ea..89e91f3 100644
>>>>>>> --- a/drivers/gpu/drm/radeon/radeon_dp_mst.c
>>>>>>> +++ b/drivers/gpu/drm/radeon/radeon_dp_mst.c
>>>>>>> @@ -661,7 +661,7 @@ radeon_dp_mst_init(struct radeon_connector
>>>>>>> *radeon_connector)
>>>>>>> {
>>>>>>> struct drm_device *dev = radeon_connector->base.dev;
>>>>>>>
>>>>>>> - if (!radeon_connector->ddc_bus->has_aux)
>>>>>>> + if (!radeon_connector->ddc_bus ||
>>>>>>> !radeon_connector->ddc_bus->has_aux)
>>>>>>> return 0;
>>>>>>>
>>>>>>> radeon_connector->mst_mgr.cbs = &mst_cbs;
>>>>>>> @@ -688,6 +688,9 @@ radeon_dp_mst_probe(struct
>> radeon_connector
>>>>>>> *radeon_connector)
>>>>>>> if (dig_connector->dpcd[DP_DPCD_REV] < 0x12)
>>>>>>> return 0;
>>>>>>>
>>>>>>> + if (!radeon_connector->ddc_bus ||
>>>>>>> !radeon_connector->ddc_bus->has_aux)
>>>>>>> + return 0;
>>>>>>> +
>>>>>>> ret = drm_dp_dpcd_read(&radeon_connector->ddc_bus->aux,
>>>>>>> DP_MSTM_CAP, msg,
>>>>>>> 1);
>>>>>>> if (ret) {
>>>>>>> @@ -711,7 +714,9 @@ radeon_dp_mst_check_status(struct
>>>>>>> radeon_connector
>>>>>>> *radeon_connector)
>>>>>>> struct radeon_connector_atom_dig *dig_connector =
>>>>>>> radeon_connector->con_priv;
>>>>>>> int retry;
>>>>>>>
>>>>>>> - if (dig_connector->is_mst) {
>>>>>>> + if (dig_connector->is_mst &&
>>>>>>> + radeon_connector->ddc_bus &&
>>>>>>> + radeon_connector->ddc_bus->has_aux) {
>>>>>>> u8 esi[16] = { 0 };
>>>>>>> int dret;
>>>>>>> int ret = 0;
>>>>>>> diff --git a/drivers/gpu/drm/radeon/radeon_i2c.c
>>>>>>> b/drivers/gpu/drm/radeon/radeon_i2c.c
>>>>>>> index 9590bcd..c18f7ba 100644
>>>>>>> --- a/drivers/gpu/drm/radeon/radeon_i2c.c
>>>>>>> +++ b/drivers/gpu/drm/radeon/radeon_i2c.c
>>>>>>> @@ -63,6 +63,9 @@ bool radeon_ddc_probe(struct
>> radeon_connector
>>>>>>> *radeon_connector, bool use_aux)
>>>>>>> if (radeon_connector->router.ddc_valid)
>>>>>>> radeon_router_select_ddc_port(radeon_connector);
>>>>>>>
>>>>>>> + if (!radeon_connector->ddc_bus)
>>>>>>> + return false;
>>>>>>> +
>>>>>>> if (use_aux) {
>>>>>>> ret = i2c_transfer(&radeon_connector->ddc_bus->aux.ddc,
>>>>>>> msgs, 2);
>>>>>>> } else {
>>>>>>> _______________________________________________
>>>>>>> dri-devel mailing list
>>>>>>> dri-devel@lists.freedesktop.org
>>>>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>>>>
>>> _______________________________________________
>>> dri-devel mailing list
>>> dri-devel@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2016-09-09 13:33 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-09-07 17:38 [PATCH 001/001] drivers/gpu/radeon: NULL pointer deference workaround Mark Fortescue
2016-09-08 7:14 ` Christian König
2016-09-08 9:09 ` Mark Fortescue
2016-09-08 10:25 ` Christian König
2016-09-08 11:18 ` Mark Fortescue
2016-09-09 8:15 ` Christian König
2016-09-09 13:20 ` Deucher, Alexander
2016-09-09 13:32 ` Mark Fortescue
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).