Re: [PATCH 2/7] drm/dp_mst: Register AUX devices for MST ports
On Thu, 2019-06-27 at 22:21 +, Li, Sun peng (Leo) wrote:
> Sorry for the late response! just jumping back on this now.
>
> On 2019-05-16 5:40 p.m., Lyude Paul wrote:
> > [CAUTION: External Email]
> >
> > So a couple of things:
> >
> > On Thu, 2019-05-16 at 11:17 -0400, sunpeng...@amd.com wrote:
> > > From: Ville Syrjälä
> > >
> > > All available downstream ports - physical and logical - are exposed for
> > > each MST device. They are listed in /dev/, following the same naming
> > > scheme as SST devices by appending an incremental ID.
> > >
> > > Although all downstream ports are exposed, only some will work as
> > > expected. Consider the following topology:
> > >
> > > +-+
> > > | ASIC |
> > > +-+
> > >Conn-0|
> > > |
> > > +v+
> > >+| MST HUB |+
> > >|+-+|
> > >| |
> > >|Port-1 Port-2|
> > > +-v-+ +-v-+
> > > | MST | | SST |
> > > | Display | | Display |
> > > +---+ +---+
> > >|Port-1
> > >x
> > >
> > > MST Path | MST Device
> > > --+--
> > > sst:0 | MST Hub
> > > mst:0-1 | MST Display
> > > mst:0-1-1 | MST Display's disconnected DP out
> > > mst:0-1-8 | MST Display's internal sink
> > > mst:0-2 | SST Display
> > >
> > > On certain MST displays, the upstream physical port will ACK DPCD reads.
> > > However, reads on the local logical port to the internal sink will
> > > *NAK*. i.e. reading mst:0-1 ACKs, but mst:0-1-8 NAKs.
> > >
> > > There may also be duplicates. Some displays will return the same GUID
> > > when reading DPCD from both mst:0-1 and mst:0-1-8.
> > >
> > > There are some device-dependent behavior as well. The MST hub used
> > > during testing will actually *ACK* read requests on a disconnected
> > > physical port, whereas the MST displays will NAK.
> > >
> > > In light of these discrepancies, it's simpler to expose all downstream
> > > ports - both physical and logical - and let the user decide what to use.
> > >
> > > Cc: Lyude Paul
> > > Signed-off-by: Ville Syrjälä
> > > Signed-off-by: Leo Li
> > > ---
> > > drivers/gpu/drm/drm_dp_aux_dev.c | 14 -
> > > drivers/gpu/drm/drm_dp_mst_topology.c | 103
> > > +
> > > -
> > > include/drm/drm_dp_helper.h | 4 ++
> > > include/drm/drm_dp_mst_helper.h | 6 ++
> > > 4 files changed, 112 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_dp_aux_dev.c
> > > b/drivers/gpu/drm/drm_dp_aux_dev.c
> > > index 6d84611..01c02b9 100644
> > > --- a/drivers/gpu/drm/drm_dp_aux_dev.c
> > > +++ b/drivers/gpu/drm/drm_dp_aux_dev.c
> > > @@ -34,6 +34,7 @@
> > > #include
> > > #include
> > > #include
> > > +#include
> > > #include
> > > #include
> > >
> > > @@ -114,6 +115,7 @@ static ssize_t name_show(struct device *dev,
> > >
> > >return res;
> > > }
> > > +
> > > static DEVICE_ATTR_RO(name);
> > >
> > > static struct attribute *drm_dp_aux_attrs[] = {
> > > @@ -160,7 +162,11 @@ static ssize_t auxdev_read_iter(struct kiocb *iocb,
> > > struct iov_iter *to)
> > >break;
> > >}
> > >
> > > - res = drm_dp_dpcd_read(aux_dev->aux, pos, buf, todo);
> > > + if (aux_dev->aux->is_remote)
> > > + res = drm_dp_mst_dpcd_read(aux_dev->aux, pos, buf,
> > > todo);
> > > + else
> > > + res = drm_dp_dpcd_read(aux_dev->aux, pos, buf,
> > > todo);
> > > +
> >
> > It's still not at all clear to me why we're trying to avoid specifying
> > actual
> > callbacks for the aux device. We should remove this part entirely, remove
> > the
> > is_remote entry from struct drm_dp_aux, and then just specify our own hook
> > in
> > struct drm_dp_aux->transfer().
> >
>
> I'm not sure if this would work well. The existing policy does retries
> around the ->transfer() call. Using the same hook will cause a nested
> retry - once when calling remote_aux->transfer, and another when calling
> real_aux->transfer. The difference is the scope of the retry. The former
> replays the entire aux transaction, while the latter replays just the
> failed sideband packet. I think having the retry at the packet level
> makes more sense. Either way, it shouldn't happen in a nested manner.
>
> In general, we need a way to determine whether a message needs to be
> sent via sideband. I'm not sure if there's a better way than setting a
> 'is_remote' flag?
oh-this is a very good point actually! I suppose we would certainly want to be
able to retry on a packet level instead. Since that's the case, I think going
with the current is_remote solution should be fine then.
Re: [PATCH 2/7] drm/dp_mst: Register AUX devices for MST ports
Sorry for the late response! just jumping back on this now.
On 2019-05-16 5:40 p.m., Lyude Paul wrote:
> [CAUTION: External Email]
>
> So a couple of things:
>
> On Thu, 2019-05-16 at 11:17 -0400, sunpeng...@amd.com wrote:
>> From: Ville Syrjälä
>>
>> All available downstream ports - physical and logical - are exposed for
>> each MST device. They are listed in /dev/, following the same naming
>> scheme as SST devices by appending an incremental ID.
>>
>> Although all downstream ports are exposed, only some will work as
>> expected. Consider the following topology:
>>
>> +-+
>> | ASIC |
>> +-+
>>Conn-0|
>> |
>> +v+
>>+| MST HUB |+
>>|+-+|
>>| |
>>|Port-1 Port-2|
>> +-v-+ +-v-+
>> | MST | | SST |
>> | Display | | Display |
>> +---+ +---+
>>|Port-1
>>x
>>
>> MST Path | MST Device
>> --+--
>> sst:0 | MST Hub
>> mst:0-1 | MST Display
>> mst:0-1-1 | MST Display's disconnected DP out
>> mst:0-1-8 | MST Display's internal sink
>> mst:0-2 | SST Display
>>
>> On certain MST displays, the upstream physical port will ACK DPCD reads.
>> However, reads on the local logical port to the internal sink will
>> *NAK*. i.e. reading mst:0-1 ACKs, but mst:0-1-8 NAKs.
>>
>> There may also be duplicates. Some displays will return the same GUID
>> when reading DPCD from both mst:0-1 and mst:0-1-8.
>>
>> There are some device-dependent behavior as well. The MST hub used
>> during testing will actually *ACK* read requests on a disconnected
>> physical port, whereas the MST displays will NAK.
>>
>> In light of these discrepancies, it's simpler to expose all downstream
>> ports - both physical and logical - and let the user decide what to use.
>>
>> Cc: Lyude Paul
>> Signed-off-by: Ville Syrjälä
>> Signed-off-by: Leo Li
>> ---
>> drivers/gpu/drm/drm_dp_aux_dev.c | 14 -
>> drivers/gpu/drm/drm_dp_mst_topology.c | 103 +
>> -
>> include/drm/drm_dp_helper.h | 4 ++
>> include/drm/drm_dp_mst_helper.h | 6 ++
>> 4 files changed, 112 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_dp_aux_dev.c
>> b/drivers/gpu/drm/drm_dp_aux_dev.c
>> index 6d84611..01c02b9 100644
>> --- a/drivers/gpu/drm/drm_dp_aux_dev.c
>> +++ b/drivers/gpu/drm/drm_dp_aux_dev.c
>> @@ -34,6 +34,7 @@
>> #include
>> #include
>> #include
>> +#include
>> #include
>> #include
>>
>> @@ -114,6 +115,7 @@ static ssize_t name_show(struct device *dev,
>>
>>return res;
>> }
>> +
>> static DEVICE_ATTR_RO(name);
>>
>> static struct attribute *drm_dp_aux_attrs[] = {
>> @@ -160,7 +162,11 @@ static ssize_t auxdev_read_iter(struct kiocb *iocb,
>> struct iov_iter *to)
>>break;
>>}
>>
>> - res = drm_dp_dpcd_read(aux_dev->aux, pos, buf, todo);
>> + if (aux_dev->aux->is_remote)
>> + res = drm_dp_mst_dpcd_read(aux_dev->aux, pos, buf,
>> todo);
>> + else
>> + res = drm_dp_dpcd_read(aux_dev->aux, pos, buf, todo);
>> +
>
> It's still not at all clear to me why we're trying to avoid specifying actual
> callbacks for the aux device. We should remove this part entirely, remove the
> is_remote entry from struct drm_dp_aux, and then just specify our own hook in
> struct drm_dp_aux->transfer().
>
I'm not sure if this would work well. The existing policy does retries
around the ->transfer() call. Using the same hook will cause a nested
retry - once when calling remote_aux->transfer, and another when calling
real_aux->transfer. The difference is the scope of the retry. The former
replays the entire aux transaction, while the latter replays just the
failed sideband packet. I think having the retry at the packet level
makes more sense. Either way, it shouldn't happen in a nested manner.
In general, we need a way to determine whether a message needs to be
sent via sideband. I'm not sure if there's a better way than setting a
'is_remote' flag?
Leo
>>if (res <= 0)
>>break;
>>
>> @@ -207,7 +213,11 @@ static ssize_t auxdev_write_iter(struct kiocb *iocb,
>> struct iov_iter *from)
>>break;
>>}
>>
>> - res = drm_dp_dpcd_write(aux_dev->aux, pos, buf, todo);
>> + if (aux_dev->aux->is_remote)
>> + res = drm_dp_mst_dpcd_write(aux_dev->aux, pos, buf,
>> todo);
>> + else
>> + res = drm_dp_dpcd_write(aux_dev->aux, pos, buf, todo);
>> +
>>if (res <= 0)
>>break;
>>
>> diff --git a/drivers/gpu/
Re: [PATCH 2/7] drm/dp_mst: Register AUX devices for MST ports
On Thu, 2019-06-06 at 19:41 +, Li, Sun peng (Leo) wrote: > > On 2019-06-03 3:28 p.m., Lyude Paul wrote: > > > I'm reproducing this just by reloading i915 on a machine with some MST > > > displays connected. I uploaded a copy of the script that I use to do > > > this > > > here: > > > > > > https://people.freedesktop.org/~lyudess/archive/06-03-2019/unloadgpumod.sh > > oops-almost forgot to mention. The argument you pass to make it reload > > instead > > of just unloading is --reload > > > > Thanks for the script! > > So, the warning has to do with: > > 1. Having the aux device as a child of connector device, and > 2. During driver unload, drm_connector_unregister() is called before > drm_dp_mst_topology_put_port() > > Which means that connector_unregister() will recursively remove the > child aux device, before put_port() can properly unregister it. Any > further attempts to remove after the first will throw a "not found" warning. > > Call-stacks for reference: > >*drm_connector_unregister*+0x37/0x60 [drm] >drm_connector_unregister_all+0x30/0x60 [drm] >drm_modeset_unregister_all+0xe/0x30 [drm] >drm_dev_unregister+0x9c/0xb0 [drm] >i915_driver_unload+0x73/0x120 [i915] > >drm_dp_aux_unregister_devnode+0xf5/0x180 [drm_kms_helper] >*drm_dp_mst_topology_put_port*+0x4e/0xf0 [drm_kms_helper] >drm_dp_mst_topology_put_mstb+0x91/0x160 [drm_kms_helper] >drm_dp_mst_topology_mgr_set_mst+0x12b/0x2b0 [drm_kms_helper] >? __finish_swait+0x10/0x40 >drm_dp_mst_topology_mgr_destroy+0x11/0xa0 [drm_kms_helper] >intel_dp_encoder_flush_work+0x32/0xb0 [i915] >intel_ddi_encoder_destroy+0x32/0x60 [i915] >drm_mode_config_cleanup+0x51/0x2e0 [drm] >intel_modeset_cleanup+0xc8/0x140 [i915] >i915_driver_unload+0xa0/0x120 [i915] > > A solution is to unregister the aux device immediately before the > connector device is unregistered - if we are to keep the aux device as a > child. Following current scheme with SST, it looks like > drm_connector_funcs->early_unregister() is the right place to do so. > To keep the balance, aux registration will then happen in > drm_connector_funcs->late_register(). This will leave the SDP > transaction handling part in DRM still, but pass the responsibility of > creating and removing remote (fake) aux devices to the driver. > > I have a WIP patch here for you to take a look. It should apply on top > of the existing patchset: > https://pastebin.com/1YJZhL4C > This approach seems fine to me! I was thinking we might end up doing something like this. My only thought is that we can probably write up a drm_dp_mst_connector_late_register() and drm_dp_mst_connector_early_unregister() that drivers can call from their →late_register()/→early_unregister() callbacks that handles printing the debug message, setting the parent and registering/unregistering the aux device. I'd imagine someday in the future we'll probably have more things to add to those callbacks for mst as well. > I'd like to hear your thoughts, before I go and modify other drivers :) > > Thanks, > Leo -- Cheers, Lyude Paul ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/7] drm/dp_mst: Register AUX devices for MST ports
On 2019-06-03 3:28 p.m., Lyude Paul wrote: >> I'm reproducing this just by reloading i915 on a machine with some MST >> displays connected. I uploaded a copy of the script that I use to do this >> here: >> >> https://people.freedesktop.org/~lyudess/archive/06-03-2019/unloadgpumod.sh > oops-almost forgot to mention. The argument you pass to make it reload instead > of just unloading is --reload > Thanks for the script! So, the warning has to do with: 1. Having the aux device as a child of connector device, and 2. During driver unload, drm_connector_unregister() is called before drm_dp_mst_topology_put_port() Which means that connector_unregister() will recursively remove the child aux device, before put_port() can properly unregister it. Any further attempts to remove after the first will throw a "not found" warning. Call-stacks for reference: *drm_connector_unregister*+0x37/0x60 [drm] drm_connector_unregister_all+0x30/0x60 [drm] drm_modeset_unregister_all+0xe/0x30 [drm] drm_dev_unregister+0x9c/0xb0 [drm] i915_driver_unload+0x73/0x120 [i915] drm_dp_aux_unregister_devnode+0xf5/0x180 [drm_kms_helper] *drm_dp_mst_topology_put_port*+0x4e/0xf0 [drm_kms_helper] drm_dp_mst_topology_put_mstb+0x91/0x160 [drm_kms_helper] drm_dp_mst_topology_mgr_set_mst+0x12b/0x2b0 [drm_kms_helper] ? __finish_swait+0x10/0x40 drm_dp_mst_topology_mgr_destroy+0x11/0xa0 [drm_kms_helper] intel_dp_encoder_flush_work+0x32/0xb0 [i915] intel_ddi_encoder_destroy+0x32/0x60 [i915] drm_mode_config_cleanup+0x51/0x2e0 [drm] intel_modeset_cleanup+0xc8/0x140 [i915] i915_driver_unload+0xa0/0x120 [i915] A solution is to unregister the aux device immediately before the connector device is unregistered - if we are to keep the aux device as a child. Following current scheme with SST, it looks like drm_connector_funcs->early_unregister() is the right place to do so. To keep the balance, aux registration will then happen in drm_connector_funcs->late_register(). This will leave the SDP transaction handling part in DRM still, but pass the responsibility of creating and removing remote (fake) aux devices to the driver. I have a WIP patch here for you to take a look. It should apply on top of the existing patchset: https://pastebin.com/1YJZhL4C I'd like to hear your thoughts, before I go and modify other drivers :) Thanks, Leo ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/7] drm/dp_mst: Register AUX devices for MST ports
On Mon, 2019-06-03 at 15:25 -0400, Lyude Paul wrote:
> On Thu, 2019-05-30 at 18:20 +, Li, Sun peng (Leo) wrote:
> > Hey, sorry for my late response!
> >
> > On 2019-05-16 5:40 p.m., Lyude Paul wrote:
> >
> > > >if (old_pdt != port->pdt && !port->input) {
> > > > @@ -1220,6 +1268,8 @@ static void drm_dp_add_port(struct
> > > > drm_dp_mst_branch
> > > > *mstb,
> > > >drm_connector_set_tile_property(port-
> > > > >connector);
> > > >}
> > > >(*mstb->mgr->cbs->register_connector)(port->connector);
> > > This is wrong: we always want to setup everything in the connector first
> > > before trying to register it, not after. Otherwise, things explode like
> > > so:
> >
> > **snip**
> >
> > > [ 452.424461] [ cut here ]
> > > [ 452.424464] sysfs group 'power' not found for kobject 'drm_dp_aux5'
> > > [ 452.424471] WARNING: CPU: 3 PID: 1887 at fs/sysfs/group.c:256
> > > sysfs_remove_group+0x76/0x80
> > > [ 452.424473] Modules linked in: vfat fat elan_i2c i915(O) intel_rapl
> > > x86_pkg_temp_thermal intel_powerclamp coretemp kvm_intel iTCO_wdt kvm
> > > mei_wdt irqbypass iTCO_vendor_support crct10dif_pclmul wmi_bmof
> > > intel_wmi_thunderbolt crc32_pclmul i2c_algo_bit ghash_clmulni_intel
> > > intel_cstate drm_kms_helper(O) intel_uncore intel_rapl_perf btusb btrtl
> > > btbcm syscopyarea btintel sysfillrect sysimgblt fb_sys_fops bluetooth
> > > drm(O) joydev mei_me idma64 ucsi_acpi thunderbolt ecdh_generic i2c_i801
> > > intel_xhci_usb_role_switch processor_thermal_device typec_ucsi
> > > intel_lpss_pci intel_soc_dts_iosf mei roles intel_lpss typec
> > > intel_pch_thermal wmi thinkpad_acpi ledtrig_audio rfkill int3403_thermal
> > > int340x_thermal_zone int3400_thermal acpi_thermal_rel acpi_pad video
> > > pcc_cpufreq crc32c_intel nvme serio_raw uas e1000e nvme_core usb_storage
> > > i2c_dev
> > > [ 452.424492] CPU: 3 PID: 1887 Comm: unloadgpumod Tainted:
> > > G O 5.1.0Lyude-Test+ #1
> > > [ 452.424494] Hardware name: LENOVO 20L8S2N800/20L8S2N800, BIOS
> > > N22ET35W
> > > (1.12 ) 04/09/2018
> > > [ 452.424496] RIP: 0010:sysfs_remove_group+0x76/0x80
> > > [ 452.424498] Code: 48 89 df 5b 5d 41 5c e9 f8 bc ff ff 48 89 df e8 d0
> > > bc
> > > ff ff eb cb 49 8b 14 24 48 8b 75 00 48 c7 c7 08 a5 0c aa e8 44 00 d6 ff
> > > <0f> 0b 5b 5d 41 5c c3 0f 1f 00 0f 1f 44 00 00 48 85 f6 74 31 41 54
> > > [ 452.424500] RSP: 0018:a8bb81b5fb28 EFLAGS: 00010282
> > > [ 452.424501] RAX: RBX: RCX:
> > > 0006
> > > [ 452.424502] RDX: 0007 RSI: 0086 RDI:
> > > 981fde2d5a00
> > > [ 452.424503] RBP: a9ea12e0 R08: 0792 R09:
> > > 0046
> > > [ 452.424504] R10: 0727 R11: a8bb81b5f9d0 R12:
> > > 981fd5f77010
> > > [ 452.424505] R13: 981fd6ebbedc R14: dead0200 R15:
> > > dead0100
> > > [ 452.424507] FS: 7f8cc1d8c740() GS:981fde2c()
> > > knlGS:
> > > [ 452.424508] CS: 0010 DS: ES: CR0: 80050033
> > > [ 452.424509] CR2: 55b19d079a08 CR3: 00043b2a0002 CR4:
> > > 003606e0
> > > [ 452.424510] DR0: DR1: DR2:
> > >
> > > [ 452.424511] DR3: DR6: fffe0ff0 DR7:
> > > 0400
> > > [ 452.424512] Call Trace:
> > > [ 452.424516] device_del+0x75/0x360
> > > [ 452.424518] ? class_find_device+0x96/0xf0
> > > [ 452.424520] device_unregister+0x16/0x60
> > > [ 452.424521] device_destroy+0x3a/0x40
> > > [ 452.424525] drm_dp_aux_unregister_devnode+0xea/0x180
> > > [drm_kms_helper]
> > > [ 452.424534] ? drm_dbg+0x87/0x90 [drm]
> > > [ 452.424538] drm_dp_mst_topology_put_port+0x5b/0x110 [drm_kms_helper]
> > > [ 452.424543] drm_dp_mst_topology_put_mstb+0xb6/0x180 [drm_kms_helper]
> > > [ 452.424547] drm_dp_mst_topology_mgr_set_mst+0x233/0x2b0
> > > [drm_kms_helper]
> > > [ 452.424551] drm_dp_mst_topology_mgr_destroy+0x18/0xa0
> > > [drm_kms_helper]
> > > [ 452.424571] intel_dp_encoder_flush_work+0x32/0xb0 [i915]
> > > [ 452.424592] intel_ddi_encoder_destroy+0x32/0x70 [i915]
> > > [ 452.424600] drm_mode_config_cleanup+0x51/0x2e0 [drm]
> > > [ 452.424621] intel_modeset_cleanup+0xc8/0x140 [i915]
> > > [ 452.424633] i915_driver_unload+0xa8/0x130 [i915]
> > > [ 452.424645] i915_pci_remove+0x1e/0x40 [i915]
> > > [ 452.424647] pci_device_remove+0x3b/0xc0
> > > [ 452.424649] device_release_driver_internal+0xe4/0x1d0
> > > [ 452.424651] pci_stop_bus_device+0x69/0x90
> > > [ 452.424653] pci_stop_and_remove_bus_device_locked+0x16/0x30
> > > [ 452.424655] remove_store+0x75/0x90
> > > [ 452.424656] kernfs_fop_write+0x116/0x190
> > > [ 452.424658] vfs_write+0xa5/0x1a0
> > > [ 452.424660] ksys_write+0x57/0xd0
> > > [ 452.424663] do_syscall_64+0x55/0x150
> > > [ 452.424665] entry_SYSCALL_64_after_hwframe
Re: [PATCH 2/7] drm/dp_mst: Register AUX devices for MST ports
On Thu, 2019-05-30 at 18:20 +, Li, Sun peng (Leo) wrote:
> Hey, sorry for my late response!
>
> On 2019-05-16 5:40 p.m., Lyude Paul wrote:
>
> > >if (old_pdt != port->pdt && !port->input) {
> > > @@ -1220,6 +1268,8 @@ static void drm_dp_add_port(struct
> > > drm_dp_mst_branch
> > > *mstb,
> > >drm_connector_set_tile_property(port->connector);
> > >}
> > >(*mstb->mgr->cbs->register_connector)(port->connector);
> > This is wrong: we always want to setup everything in the connector first
> > before trying to register it, not after. Otherwise, things explode like
> > so:
>
> **snip**
>
> > [ 452.424461] [ cut here ]
> > [ 452.424464] sysfs group 'power' not found for kobject 'drm_dp_aux5'
> > [ 452.424471] WARNING: CPU: 3 PID: 1887 at fs/sysfs/group.c:256
> > sysfs_remove_group+0x76/0x80
> > [ 452.424473] Modules linked in: vfat fat elan_i2c i915(O) intel_rapl
> > x86_pkg_temp_thermal intel_powerclamp coretemp kvm_intel iTCO_wdt kvm
> > mei_wdt irqbypass iTCO_vendor_support crct10dif_pclmul wmi_bmof
> > intel_wmi_thunderbolt crc32_pclmul i2c_algo_bit ghash_clmulni_intel
> > intel_cstate drm_kms_helper(O) intel_uncore intel_rapl_perf btusb btrtl
> > btbcm syscopyarea btintel sysfillrect sysimgblt fb_sys_fops bluetooth
> > drm(O) joydev mei_me idma64 ucsi_acpi thunderbolt ecdh_generic i2c_i801
> > intel_xhci_usb_role_switch processor_thermal_device typec_ucsi
> > intel_lpss_pci intel_soc_dts_iosf mei roles intel_lpss typec
> > intel_pch_thermal wmi thinkpad_acpi ledtrig_audio rfkill int3403_thermal
> > int340x_thermal_zone int3400_thermal acpi_thermal_rel acpi_pad video
> > pcc_cpufreq crc32c_intel nvme serio_raw uas e1000e nvme_core usb_storage
> > i2c_dev
> > [ 452.424492] CPU: 3 PID: 1887 Comm: unloadgpumod Tainted:
> > G O 5.1.0Lyude-Test+ #1
> > [ 452.424494] Hardware name: LENOVO 20L8S2N800/20L8S2N800, BIOS N22ET35W
> > (1.12 ) 04/09/2018
> > [ 452.424496] RIP: 0010:sysfs_remove_group+0x76/0x80
> > [ 452.424498] Code: 48 89 df 5b 5d 41 5c e9 f8 bc ff ff 48 89 df e8 d0 bc
> > ff ff eb cb 49 8b 14 24 48 8b 75 00 48 c7 c7 08 a5 0c aa e8 44 00 d6 ff
> > <0f> 0b 5b 5d 41 5c c3 0f 1f 00 0f 1f 44 00 00 48 85 f6 74 31 41 54
> > [ 452.424500] RSP: 0018:a8bb81b5fb28 EFLAGS: 00010282
> > [ 452.424501] RAX: RBX: RCX:
> > 0006
> > [ 452.424502] RDX: 0007 RSI: 0086 RDI:
> > 981fde2d5a00
> > [ 452.424503] RBP: a9ea12e0 R08: 0792 R09:
> > 0046
> > [ 452.424504] R10: 0727 R11: a8bb81b5f9d0 R12:
> > 981fd5f77010
> > [ 452.424505] R13: 981fd6ebbedc R14: dead0200 R15:
> > dead0100
> > [ 452.424507] FS: 7f8cc1d8c740() GS:981fde2c()
> > knlGS:
> > [ 452.424508] CS: 0010 DS: ES: CR0: 80050033
> > [ 452.424509] CR2: 55b19d079a08 CR3: 00043b2a0002 CR4:
> > 003606e0
> > [ 452.424510] DR0: DR1: DR2:
> >
> > [ 452.424511] DR3: DR6: fffe0ff0 DR7:
> > 0400
> > [ 452.424512] Call Trace:
> > [ 452.424516] device_del+0x75/0x360
> > [ 452.424518] ? class_find_device+0x96/0xf0
> > [ 452.424520] device_unregister+0x16/0x60
> > [ 452.424521] device_destroy+0x3a/0x40
> > [ 452.424525] drm_dp_aux_unregister_devnode+0xea/0x180 [drm_kms_helper]
> > [ 452.424534] ? drm_dbg+0x87/0x90 [drm]
> > [ 452.424538] drm_dp_mst_topology_put_port+0x5b/0x110 [drm_kms_helper]
> > [ 452.424543] drm_dp_mst_topology_put_mstb+0xb6/0x180 [drm_kms_helper]
> > [ 452.424547] drm_dp_mst_topology_mgr_set_mst+0x233/0x2b0
> > [drm_kms_helper]
> > [ 452.424551] drm_dp_mst_topology_mgr_destroy+0x18/0xa0 [drm_kms_helper]
> > [ 452.424571] intel_dp_encoder_flush_work+0x32/0xb0 [i915]
> > [ 452.424592] intel_ddi_encoder_destroy+0x32/0x70 [i915]
> > [ 452.424600] drm_mode_config_cleanup+0x51/0x2e0 [drm]
> > [ 452.424621] intel_modeset_cleanup+0xc8/0x140 [i915]
> > [ 452.424633] i915_driver_unload+0xa8/0x130 [i915]
> > [ 452.424645] i915_pci_remove+0x1e/0x40 [i915]
> > [ 452.424647] pci_device_remove+0x3b/0xc0
> > [ 452.424649] device_release_driver_internal+0xe4/0x1d0
> > [ 452.424651] pci_stop_bus_device+0x69/0x90
> > [ 452.424653] pci_stop_and_remove_bus_device_locked+0x16/0x30
> > [ 452.424655] remove_store+0x75/0x90
> > [ 452.424656] kernfs_fop_write+0x116/0x190
> > [ 452.424658] vfs_write+0xa5/0x1a0
> > [ 452.424660] ksys_write+0x57/0xd0
> > [ 452.424663] do_syscall_64+0x55/0x150
> > [ 452.424665] entry_SYSCALL_64_after_hwframe+0x44/0xa9
> > [ 452.424667] RIP: 0033:0x7f8cc1e7d038
> > [ 452.424668] Code: 89 02 48 c7 c0 ff ff ff ff eb b3 0f 1f 80 00 00 00 00
> > f3 0f 1e fa 48 8d 05 e5 76 0d 00 8b 00 85 c0 75 17 b8 01 00 00 00 0f 05
> > <48> 3d 00 f0 ff ff 77 58 c3 0f 1f 80 00 00 00 00 41 5
Re: [PATCH 2/7] drm/dp_mst: Register AUX devices for MST ports
Hey, sorry for my late response!
On 2019-05-16 5:40 p.m., Lyude Paul wrote:
>>if (old_pdt != port->pdt && !port->input) {
>> @@ -1220,6 +1268,8 @@ static void drm_dp_add_port(struct drm_dp_mst_branch
>> *mstb,
>>drm_connector_set_tile_property(port->connector);
>>}
>>(*mstb->mgr->cbs->register_connector)(port->connector);
> This is wrong: we always want to setup everything in the connector first
> before trying to register it, not after. Otherwise, things explode like so:
**snip**
> [ 452.424461] [ cut here ]
> [ 452.424464] sysfs group 'power' not found for kobject 'drm_dp_aux5'
> [ 452.424471] WARNING: CPU: 3 PID: 1887 at fs/sysfs/group.c:256
> sysfs_remove_group+0x76/0x80
> [ 452.424473] Modules linked in: vfat fat elan_i2c i915(O) intel_rapl
> x86_pkg_temp_thermal intel_powerclamp coretemp kvm_intel iTCO_wdt kvm mei_wdt
> irqbypass iTCO_vendor_support crct10dif_pclmul wmi_bmof intel_wmi_thunderbolt
> crc32_pclmul i2c_algo_bit ghash_clmulni_intel intel_cstate drm_kms_helper(O)
> intel_uncore intel_rapl_perf btusb btrtl btbcm syscopyarea btintel
> sysfillrect sysimgblt fb_sys_fops bluetooth drm(O) joydev mei_me idma64
> ucsi_acpi thunderbolt ecdh_generic i2c_i801 intel_xhci_usb_role_switch
> processor_thermal_device typec_ucsi intel_lpss_pci intel_soc_dts_iosf mei
> roles intel_lpss typec intel_pch_thermal wmi thinkpad_acpi ledtrig_audio
> rfkill int3403_thermal int340x_thermal_zone int3400_thermal acpi_thermal_rel
> acpi_pad video pcc_cpufreq crc32c_intel nvme serio_raw uas e1000e nvme_core
> usb_storage i2c_dev
> [ 452.424492] CPU: 3 PID: 1887 Comm: unloadgpumod Tainted: G O
> 5.1.0Lyude-Test+ #1
> [ 452.424494] Hardware name: LENOVO 20L8S2N800/20L8S2N800, BIOS N22ET35W
> (1.12 ) 04/09/2018
> [ 452.424496] RIP: 0010:sysfs_remove_group+0x76/0x80
> [ 452.424498] Code: 48 89 df 5b 5d 41 5c e9 f8 bc ff ff 48 89 df e8 d0 bc ff
> ff eb cb 49 8b 14 24 48 8b 75 00 48 c7 c7 08 a5 0c aa e8 44 00 d6 ff <0f> 0b
> 5b 5d 41 5c c3 0f 1f 00 0f 1f 44 00 00 48 85 f6 74 31 41 54
> [ 452.424500] RSP: 0018:a8bb81b5fb28 EFLAGS: 00010282
> [ 452.424501] RAX: RBX: RCX:
> 0006
> [ 452.424502] RDX: 0007 RSI: 0086 RDI:
> 981fde2d5a00
> [ 452.424503] RBP: a9ea12e0 R08: 0792 R09:
> 0046
> [ 452.424504] R10: 0727 R11: a8bb81b5f9d0 R12:
> 981fd5f77010
> [ 452.424505] R13: 981fd6ebbedc R14: dead0200 R15:
> dead0100
> [ 452.424507] FS: 7f8cc1d8c740() GS:981fde2c()
> knlGS:
> [ 452.424508] CS: 0010 DS: ES: CR0: 80050033
> [ 452.424509] CR2: 55b19d079a08 CR3: 00043b2a0002 CR4:
> 003606e0
> [ 452.424510] DR0: DR1: DR2:
>
> [ 452.424511] DR3: DR6: fffe0ff0 DR7:
> 0400
> [ 452.424512] Call Trace:
> [ 452.424516] device_del+0x75/0x360
> [ 452.424518] ? class_find_device+0x96/0xf0
> [ 452.424520] device_unregister+0x16/0x60
> [ 452.424521] device_destroy+0x3a/0x40
> [ 452.424525] drm_dp_aux_unregister_devnode+0xea/0x180 [drm_kms_helper]
> [ 452.424534] ? drm_dbg+0x87/0x90 [drm]
> [ 452.424538] drm_dp_mst_topology_put_port+0x5b/0x110 [drm_kms_helper]
> [ 452.424543] drm_dp_mst_topology_put_mstb+0xb6/0x180 [drm_kms_helper]
> [ 452.424547] drm_dp_mst_topology_mgr_set_mst+0x233/0x2b0 [drm_kms_helper]
> [ 452.424551] drm_dp_mst_topology_mgr_destroy+0x18/0xa0 [drm_kms_helper]
> [ 452.424571] intel_dp_encoder_flush_work+0x32/0xb0 [i915]
> [ 452.424592] intel_ddi_encoder_destroy+0x32/0x70 [i915]
> [ 452.424600] drm_mode_config_cleanup+0x51/0x2e0 [drm]
> [ 452.424621] intel_modeset_cleanup+0xc8/0x140 [i915]
> [ 452.424633] i915_driver_unload+0xa8/0x130 [i915]
> [ 452.424645] i915_pci_remove+0x1e/0x40 [i915]
> [ 452.424647] pci_device_remove+0x3b/0xc0
> [ 452.424649] device_release_driver_internal+0xe4/0x1d0
> [ 452.424651] pci_stop_bus_device+0x69/0x90
> [ 452.424653] pci_stop_and_remove_bus_device_locked+0x16/0x30
> [ 452.424655] remove_store+0x75/0x90
> [ 452.424656] kernfs_fop_write+0x116/0x190
> [ 452.424658] vfs_write+0xa5/0x1a0
> [ 452.424660] ksys_write+0x57/0xd0
> [ 452.424663] do_syscall_64+0x55/0x150
> [ 452.424665] entry_SYSCALL_64_after_hwframe+0x44/0xa9
> [ 452.424667] RIP: 0033:0x7f8cc1e7d038
> [ 452.424668] Code: 89 02 48 c7 c0 ff ff ff ff eb b3 0f 1f 80 00 00 00 00 f3
> 0f 1e fa 48 8d 05 e5 76 0d 00 8b 00 85 c0 75 17 b8 01 00 00 00 0f 05 <48> 3d
> 00 f0 ff ff 77 58 c3 0f 1f 80 00 00 00 00 41 54 49 89 d4 55
> [ 452.424670] RSP: 002b:7ffce4321218 EFLAGS: 0246 ORIG_RAX:
> 0001
> [ 452.424672] RAX: ffda RBX: 0002 RCX:
> 7f8cc1e7d038
> [ 452.424673] RDX: 0002 RSI: 00
Re: [PATCH 2/7] drm/dp_mst: Register AUX devices for MST ports
So a couple of things:
On Thu, 2019-05-16 at 11:17 -0400, sunpeng...@amd.com wrote:
> From: Ville Syrjälä
>
> All available downstream ports - physical and logical - are exposed for
> each MST device. They are listed in /dev/, following the same naming
> scheme as SST devices by appending an incremental ID.
>
> Although all downstream ports are exposed, only some will work as
> expected. Consider the following topology:
>
>+-+
>| ASIC |
>+-+
> Conn-0|
> |
>+v+
> +| MST HUB |+
> |+-+|
> | |
> |Port-1 Port-2|
> +-v-+ +-v-+
> | MST | | SST |
> | Display | | Display |
> +---+ +---+
> |Port-1
> x
>
> MST Path | MST Device
> --+--
> sst:0 | MST Hub
> mst:0-1 | MST Display
> mst:0-1-1 | MST Display's disconnected DP out
> mst:0-1-8 | MST Display's internal sink
> mst:0-2 | SST Display
>
> On certain MST displays, the upstream physical port will ACK DPCD reads.
> However, reads on the local logical port to the internal sink will
> *NAK*. i.e. reading mst:0-1 ACKs, but mst:0-1-8 NAKs.
>
> There may also be duplicates. Some displays will return the same GUID
> when reading DPCD from both mst:0-1 and mst:0-1-8.
>
> There are some device-dependent behavior as well. The MST hub used
> during testing will actually *ACK* read requests on a disconnected
> physical port, whereas the MST displays will NAK.
>
> In light of these discrepancies, it's simpler to expose all downstream
> ports - both physical and logical - and let the user decide what to use.
>
> Cc: Lyude Paul
> Signed-off-by: Ville Syrjälä
> Signed-off-by: Leo Li
> ---
> drivers/gpu/drm/drm_dp_aux_dev.c | 14 -
> drivers/gpu/drm/drm_dp_mst_topology.c | 103 +
> -
> include/drm/drm_dp_helper.h | 4 ++
> include/drm/drm_dp_mst_helper.h | 6 ++
> 4 files changed, 112 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_dp_aux_dev.c
> b/drivers/gpu/drm/drm_dp_aux_dev.c
> index 6d84611..01c02b9 100644
> --- a/drivers/gpu/drm/drm_dp_aux_dev.c
> +++ b/drivers/gpu/drm/drm_dp_aux_dev.c
> @@ -34,6 +34,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
>
> @@ -114,6 +115,7 @@ static ssize_t name_show(struct device *dev,
>
> return res;
> }
> +
> static DEVICE_ATTR_RO(name);
>
> static struct attribute *drm_dp_aux_attrs[] = {
> @@ -160,7 +162,11 @@ static ssize_t auxdev_read_iter(struct kiocb *iocb,
> struct iov_iter *to)
> break;
> }
>
> - res = drm_dp_dpcd_read(aux_dev->aux, pos, buf, todo);
> + if (aux_dev->aux->is_remote)
> + res = drm_dp_mst_dpcd_read(aux_dev->aux, pos, buf,
> todo);
> + else
> + res = drm_dp_dpcd_read(aux_dev->aux, pos, buf, todo);
> +
It's still not at all clear to me why we're trying to avoid specifying actual
callbacks for the aux device. We should remove this part entirely, remove the
is_remote entry from struct drm_dp_aux, and then just specify our own hook in
struct drm_dp_aux->transfer().
> if (res <= 0)
> break;
>
> @@ -207,7 +213,11 @@ static ssize_t auxdev_write_iter(struct kiocb *iocb,
> struct iov_iter *from)
> break;
> }
>
> - res = drm_dp_dpcd_write(aux_dev->aux, pos, buf, todo);
> + if (aux_dev->aux->is_remote)
> + res = drm_dp_mst_dpcd_write(aux_dev->aux, pos, buf,
> todo);
> + else
> + res = drm_dp_dpcd_write(aux_dev->aux, pos, buf, todo);
> +
> if (res <= 0)
> break;
>
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 2ab16c9..54da68e 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -35,6 +35,8 @@
> #include
> #include
>
> +#include "drm_crtc_helper_internal.h"
> +
Unless I'm mistaken, this looks like some sort of ditritus.
> /**
> * DOC: dp mst helper
> *
> @@ -52,6 +54,9 @@ static int drm_dp_dpcd_write_payload(struct
> drm_dp_mst_topology_mgr *mgr,
>int id,
>struct drm_dp_payload *payload);
>
> +static int drm_dp_send_dpcd_read(struct drm_dp_mst_topology_mgr *mgr,
> + struct drm_dp_mst_port *port,
> + int offset, int size, u8 *bytes);
> static int drm_dp_send_dpcd_write(struct drm_dp_mst_topology_mgr *mgr,
> struct drm_dp_mst_port *port,
>
