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 at thurning-instruments.co.uk> >>>> Tested-by: Mark Fortescue <mark at 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 at lists.freedesktop.org >>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel >>>
