| Message ID | 20240809193600.3360015-1-sean.anderson@linux.dev (mailing list archive) |
|---|---|
| Headers | show |
| Series | drm: zynqmp_dp: IRQ cleanups and debugfs support | expand |
On 8/9/24 15:35, Sean Anderson wrote: > This series cleans up the zyqnmp_dp IRQ and locking situation. Once > that's done, it adds debugfs support. The intent is to enable compliance > testing or to help debug signal-integrity issues. > > Previously, I discussed converting the HPD work(s) to a threaded IRQ. I > did not end up doing that for this series since the steps would be > > - Add locking > - Move link retraining to a work function > - Harden the IRQ > - Merge the works into a threaded IRQ (omitted) > > Which with the exception of the final step is the same as leaving those > works as-is. Conversion to a threaded IRQ can be done as a follow-up. > > Changes in v6: > - Unplug DRM device before removal > - Fix hang upon driver removal > - Rebase onto drm-misc/drm-misc-next > > Changes in v5: > - Rebase onto drm-misc/drm-misc-next > > Changes in v4: > - Rebase onto drm/drm-next > > Changes in v3: > - Convert to a hard IRQ > - Use AUX IRQs instead of polling > - Take dp->lock in zynqmp_dp_hpd_work_func > > Changes in v2: > - Split off the HPD IRQ work into another commit > - Expand the commit message > - Document hpd_irq_work > - Document debugfs files > - Add ignore_aux_errors and ignore_hpd debugfs files to replace earlier > implicit functionality > - Attempt to fix unreproducable, spurious build warning > - Drop "Optionally ignore DPCD errors" in favor of a debugfs file > directly affecting zynqmp_dp_aux_transfer. > > Sean Anderson (8): > drm: zynqmp_kms: Unplug DRM device before removal > drm: zynqmp_dp: Add locking > drm: zynqmp_dp: Don't retrain the link in our IRQ > drm: zynqmp_dp: Convert to a hard IRQ > drm: zynqmp_dp: Use AUX IRQs instead of polling > drm: zynqmp_dp: Split off several helper functions > drm: zynqmp_dp: Take dp->lock in zynqmp_dp_hpd_work_func > drm: zynqmp_dp: Add debugfs interface for compliance testing > > Documentation/gpu/drivers.rst | 1 + > Documentation/gpu/zynqmp.rst | 149 ++++++ > MAINTAINERS | 1 + > drivers/gpu/drm/xlnx/zynqmp_dp.c | 846 ++++++++++++++++++++++++++++-- > drivers/gpu/drm/xlnx/zynqmp_kms.c | 2 +- > 5 files changed, 951 insertions(+), 48 deletions(-) > create mode 100644 Documentation/gpu/zynqmp.rst > ping --Sean
Hi, On 01/10/2024 21:31, Sean Anderson wrote: > On 8/9/24 15:35, Sean Anderson wrote: >> This series cleans up the zyqnmp_dp IRQ and locking situation. Once >> that's done, it adds debugfs support. The intent is to enable compliance >> testing or to help debug signal-integrity issues. I think the patches 1-7 look fine, and I think I can pick those already to drm-misc if you're ok with that. I'm a bit unsure about patch 8, probably mainly because I don't have experience with the compliance testing. How have you tested this? With some DP analyzer/tester, I presume? I think none of this (patch 8) is needed by almost anybody. Even among zynqmp_dp developers I assume it's very rare to have the hardware for this. I wonder if it would make sense to have the debugfs and related code behind a compile option (which would be nice as the code wouldn't even compiled in), or maybe a module parameter (which would be nice as then "anyone" can easily enable it for compliance testing). What do you think? I also somehow recall that there was some discussion earlier about how/if other drivers support compliance testing. But I can't find the discussion. Do you remember if there was such discussion, and what was the conclusion? With a quick look, everything in the debugfs looks generic, not xilinx specific. Tomi
On 10/2/24 10:50, Tomi Valkeinen wrote: > Hi, > > On 01/10/2024 21:31, Sean Anderson wrote: >> On 8/9/24 15:35, Sean Anderson wrote: >>> This series cleans up the zyqnmp_dp IRQ and locking situation. Once >>> that's done, it adds debugfs support. The intent is to enable compliance >>> testing or to help debug signal-integrity issues. > > I think the patches 1-7 look fine, and I think I can pick those already to drm-misc if you're ok with that. > > I'm a bit unsure about patch 8, probably mainly because I don't have experience with the compliance testing. > > How have you tested this? With some DP analyzer/tester, I presume? For my test setup I used an oscilloscope hooked up to the displayport output using a fixture that broke the signals out to SMA. Since the oscilloscope cannot emulate a sink, I first had the output connected to a monitor. Then I disabled HPD and reconnected the output to my fixture. This process is described in more detail in the documentation. > I think none of this (patch 8) is needed by almost anybody. Well, I found it very useful for debugging a signal integrity issue I was having. Once I could have a look at the signals it was very clear what the problem was. > Even among zynqmp_dp developers I assume it's very rare to have the > hardware for this. I wonder if it would make sense to have the debugfs > and related code behind a compile option (which would be nice as the > code wouldn't even compiled in), or maybe a module parameter (which > would be nice as then "anyone" can easily enable it for compliance > testing). What do you think? Other drivers with these features just enabled it unconditionally, so I didn't bother with any special config. > I also somehow recall that there was some discussion earlier about > how/if other drivers support compliance testing. But I can't find the > discussion. Do you remember if there was such discussion, and what was > the conclusion? With a quick look, everything in the debugfs looks > generic, not xilinx specific. The last it got discussed was back in [1], but I never got any further response. I agree that some of this is generic, and could probably be reworked into some internal helpers. But I don't have the bandwidth at the moment to do that work. --Sean [1] http://lore.kernel.org/dri-devel/cda22b0c-8d7c-4ce2-9a7c-3b5ab540fa1f@linux.dev
Hi Tomi, On 10/3/24 10:53, Sean Anderson wrote: > On 10/2/24 10:50, Tomi Valkeinen wrote: >> Hi, >> >> On 01/10/2024 21:31, Sean Anderson wrote: >>> On 8/9/24 15:35, Sean Anderson wrote: >>>> This series cleans up the zyqnmp_dp IRQ and locking situation. Once >>>> that's done, it adds debugfs support. The intent is to enable compliance >>>> testing or to help debug signal-integrity issues. >> >> I think the patches 1-7 look fine, and I think I can pick those already to drm-misc if you're ok with that. >> >> I'm a bit unsure about patch 8, probably mainly because I don't have experience with the compliance testing. >> >> How have you tested this? With some DP analyzer/tester, I presume? > > For my test setup I used an oscilloscope hooked up to the displayport > output using a fixture that broke the signals out to SMA. Since the > oscilloscope cannot emulate a sink, I first had the output connected to > a monitor. Then I disabled HPD and reconnected the output to my fixture. > This process is described in more detail in the documentation. > >> I think none of this (patch 8) is needed by almost anybody. > > Well, I found it very useful for debugging a signal integrity issue I > was having. Once I could have a look at the signals it was very clear > what the problem was. > >> Even among zynqmp_dp developers I assume it's very rare to have the >> hardware for this. I wonder if it would make sense to have the debugfs >> and related code behind a compile option (which would be nice as the >> code wouldn't even compiled in), or maybe a module parameter (which >> would be nice as then "anyone" can easily enable it for compliance >> testing). What do you think? > > Other drivers with these features just enabled it unconditionally, so I > didn't bother with any special config. > >> I also somehow recall that there was some discussion earlier about >> how/if other drivers support compliance testing. But I can't find the >> discussion. Do you remember if there was such discussion, and what was >> the conclusion? With a quick look, everything in the debugfs looks >> generic, not xilinx specific. > > The last it got discussed was back in [1], but I never got any further > response. I agree that some of this is generic, and could probably be > reworked into some internal helpers. But I don't have the bandwidth at > the moment to do that work. > > --Sean > > [1] http://lore.kernel.org/dri-devel/cda22b0c-8d7c-4ce2-9a7c-3b5ab540fa1f@linux.dev Does this all make sense to you? At the moment I don't believe I have any changes I need to resend for (although this series is archived in patchwork [1] for some reason). --Sean [1] https://patchwork.kernel.org/project/dri-devel/list/?series=878338&archive=both
Hi, On 25/10/2024 17:58, Sean Anderson wrote: > Hi Tomi, > > On 10/3/24 10:53, Sean Anderson wrote: >> On 10/2/24 10:50, Tomi Valkeinen wrote: >>> Hi, >>> >>> On 01/10/2024 21:31, Sean Anderson wrote: >>>> On 8/9/24 15:35, Sean Anderson wrote: >>>>> This series cleans up the zyqnmp_dp IRQ and locking situation. Once >>>>> that's done, it adds debugfs support. The intent is to enable compliance >>>>> testing or to help debug signal-integrity issues. >>> >>> I think the patches 1-7 look fine, and I think I can pick those already to drm-misc if you're ok with that. >>> >>> I'm a bit unsure about patch 8, probably mainly because I don't have experience with the compliance testing. >>> >>> How have you tested this? With some DP analyzer/tester, I presume? >> >> For my test setup I used an oscilloscope hooked up to the displayport >> output using a fixture that broke the signals out to SMA. Since the >> oscilloscope cannot emulate a sink, I first had the output connected to >> a monitor. Then I disabled HPD and reconnected the output to my fixture. >> This process is described in more detail in the documentation. >> >>> I think none of this (patch 8) is needed by almost anybody. >> >> Well, I found it very useful for debugging a signal integrity issue I >> was having. Once I could have a look at the signals it was very clear >> what the problem was. >> >>> Even among zynqmp_dp developers I assume it's very rare to have the >>> hardware for this. I wonder if it would make sense to have the debugfs >>> and related code behind a compile option (which would be nice as the >>> code wouldn't even compiled in), or maybe a module parameter (which >>> would be nice as then "anyone" can easily enable it for compliance >>> testing). What do you think? >> >> Other drivers with these features just enabled it unconditionally, so I >> didn't bother with any special config. >> >>> I also somehow recall that there was some discussion earlier about >>> how/if other drivers support compliance testing. But I can't find the >>> discussion. Do you remember if there was such discussion, and what was >>> the conclusion? With a quick look, everything in the debugfs looks >>> generic, not xilinx specific. >> >> The last it got discussed was back in [1], but I never got any further >> response. I agree that some of this is generic, and could probably be >> reworked into some internal helpers. But I don't have the bandwidth at >> the moment to do that work. >> >> --Sean >> >> [1] http://lore.kernel.org/dri-devel/cda22b0c-8d7c-4ce2-9a7c-3b5ab540fa1f@linux.dev > > Does this all make sense to you? At the moment I don't believe I have any > changes I need to resend for (although this series is archived in patchwork [1] > for some reason). > > --Sean > > [1] https://patchwork.kernel.org/project/dri-devel/list/?series=878338&archive=both I was hoping to get tested-by from amd, as I can't test this properly, but it's probably pointless to wait. The biggest hesitation I have is what I mentioned earlier: this adds a lot of code which is not for normal use. It would be nice to split this into a separate file, maybe behind a compile option, but I fear that'll require a more restructuring of the driver. So, I think it's fine, I'll apply this tomorrow. Tomi
Hi Sean, On 28/10/2024 17:04, Tomi Valkeinen wrote: > Hi, > > On 25/10/2024 17:58, Sean Anderson wrote: >> Hi Tomi, >> >> On 10/3/24 10:53, Sean Anderson wrote: >>> On 10/2/24 10:50, Tomi Valkeinen wrote: >>>> Hi, >>>> >>>> On 01/10/2024 21:31, Sean Anderson wrote: >>>>> On 8/9/24 15:35, Sean Anderson wrote: >>>>>> This series cleans up the zyqnmp_dp IRQ and locking situation. Once >>>>>> that's done, it adds debugfs support. The intent is to enable >>>>>> compliance >>>>>> testing or to help debug signal-integrity issues. >>>> >>>> I think the patches 1-7 look fine, and I think I can pick those >>>> already to drm-misc if you're ok with that. >>>> >>>> I'm a bit unsure about patch 8, probably mainly because I don't have >>>> experience with the compliance testing. >>>> >>>> How have you tested this? With some DP analyzer/tester, I presume? >>> >>> For my test setup I used an oscilloscope hooked up to the displayport >>> output using a fixture that broke the signals out to SMA. Since the >>> oscilloscope cannot emulate a sink, I first had the output connected to >>> a monitor. Then I disabled HPD and reconnected the output to my fixture. >>> This process is described in more detail in the documentation. >>> >>>> I think none of this (patch 8) is needed by almost anybody. >>> >>> Well, I found it very useful for debugging a signal integrity issue I >>> was having. Once I could have a look at the signals it was very clear >>> what the problem was. >>> >>>> Even among zynqmp_dp developers I assume it's very rare to have the >>>> hardware for this. I wonder if it would make sense to have the debugfs >>>> and related code behind a compile option (which would be nice as the >>>> code wouldn't even compiled in), or maybe a module parameter (which >>>> would be nice as then "anyone" can easily enable it for compliance >>>> testing). What do you think? >>> >>> Other drivers with these features just enabled it unconditionally, so I >>> didn't bother with any special config. >>> >>>> I also somehow recall that there was some discussion earlier about >>>> how/if other drivers support compliance testing. But I can't find the >>>> discussion. Do you remember if there was such discussion, and what was >>>> the conclusion? With a quick look, everything in the debugfs looks >>>> generic, not xilinx specific. >>> >>> The last it got discussed was back in [1], but I never got any further >>> response. I agree that some of this is generic, and could probably be >>> reworked into some internal helpers. But I don't have the bandwidth at >>> the moment to do that work. >>> >>> --Sean >>> >>> [1] http://lore.kernel.org/dri-devel/ >>> cda22b0c-8d7c-4ce2-9a7c-3b5ab540fa1f@linux.dev >> >> Does this all make sense to you? At the moment I don't believe I have any >> changes I need to resend for (although this series is archived in >> patchwork [1] >> for some reason). >> >> --Sean >> >> [1] https://patchwork.kernel.org/project/dri-devel/list/? >> series=878338&archive=both > > I was hoping to get tested-by from amd, as I can't test this properly, > but it's probably pointless to wait. > > The biggest hesitation I have is what I mentioned earlier: this adds a > lot of code which is not for normal use. It would be nice to split this > into a separate file, maybe behind a compile option, but I fear that'll > require a more restructuring of the driver. > > So, I think it's fine, I'll apply this tomorrow. > > Tomi > Thanks, pushed. Tomi