[PATCH v2 0/8] drm: zynqmp_dp: Misc. patches and debugfs support

[PATCH v2 0/8] drm: zynqmp_dp: Misc. patches and debugfs support

[Sean Anderson]

This series adds debugfs support for the zynqmp_dp driver. The intent is
to enable compliance testing or to help debug signal-integrity issues.

The first four patches are general improvements (and could be applied
independently), while the last four add debugfs support.

Changes in v2:
- Fix kerneldoc
- Rearrange zynqmp_dp for better padding
- Document hpd_irq_work
- Split off the HPD IRQ work into another commit
- Expand the commit message
- 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: xlnx: Fix kerneldoc
  drm: zynqmp_dp: Downgrade log level for aux retries message
  drm: zynqmp_dp: Adjust training values per-lane
  drm: zynqmp_dp: Rearrange zynqmp_dp for better padding
  drm: zynqmp_dp: Don't retrain the link in our IRQ
  drm: zynqmp_dp: Add locking
  drm: zynqmp_dp: Split off several helper functions
  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_disp.c  |   6 +-
 drivers/gpu/drm/xlnx/zynqmp_dp.c    | 836 +++++++++++++++++++++++++---
 drivers/gpu/drm/xlnx/zynqmp_dpsub.h |   1 +
 drivers/gpu/drm/xlnx/zynqmp_kms.h   |   4 +-
 7 files changed, 931 insertions(+), 67 deletions(-)
 create mode 100644 Documentation/gpu/zynqmp.rst

-- 
2.35.1.1320.gc452695387.dirty


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 1/8] drm: xlnx: Fix kerneldoc

[Sean Anderson]

Fix a few errors in the kerneldoc. Mostly this addresses missing/renamed
members.

Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
---

Changes in v2:
- New

 drivers/gpu/drm/xlnx/zynqmp_disp.c  | 6 +++---
 drivers/gpu/drm/xlnx/zynqmp_dpsub.h | 1 +
 drivers/gpu/drm/xlnx/zynqmp_kms.h   | 4 ++--
 3 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c b/drivers/gpu/drm/xlnx/zynqmp_disp.c
index 407bc07cec69..f79bf3fb8110 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
@@ -128,9 +128,9 @@ struct zynqmp_disp_layer {
  * struct zynqmp_disp - Display controller
  * @dev: Device structure
  * @dpsub: Display subsystem
- * @blend.base: Register I/O base address for the blender
- * @avbuf.base: Register I/O base address for the audio/video buffer manager
- * @audio.base: Registers I/O base address for the audio mixer
+ * @blend: Register I/O base address for the blender
+ * @avbuf: Register I/O base address for the audio/video buffer manager
+ * @audio: Registers I/O base address for the audio mixer
  * @layers: Layers (planes)
  */
 struct zynqmp_disp {
diff --git a/drivers/gpu/drm/xlnx/zynqmp_dpsub.h b/drivers/gpu/drm/xlnx/zynqmp_dpsub.h
index 09ea01878f2a..b18554467e9c 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_dpsub.h
+++ b/drivers/gpu/drm/xlnx/zynqmp_dpsub.h
@@ -53,6 +53,7 @@ enum zynqmp_dpsub_format {
  * @drm: The DRM/KMS device data
  * @bridge: The DP encoder bridge
  * @disp: The display controller
+ * @layers: Video and graphics layers
  * @dp: The DisplayPort controller
  * @dma_align: DMA alignment constraint (must be a power of 2)
  */
diff --git a/drivers/gpu/drm/xlnx/zynqmp_kms.h b/drivers/gpu/drm/xlnx/zynqmp_kms.h
index 01be96b00e3f..cb13c6b8008e 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_kms.h
+++ b/drivers/gpu/drm/xlnx/zynqmp_kms.h
@@ -22,9 +22,9 @@
 struct zynqmp_dpsub;
 
 /**
- * struct zynqmp_dpsub - ZynqMP DisplayPort Subsystem DRM/KMS data
+ * struct zynqmp_dpsub_drm - ZynqMP DisplayPort Subsystem DRM/KMS data
  * @dpsub: Backpointer to the DisplayPort subsystem
- * @drm: The DRM/KMS device
+ * @dev: The DRM/KMS device
  * @planes: The DRM planes
  * @crtc: The DRM CRTC
  * @encoder: The dummy DRM encoder
-- 
2.35.1.1320.gc452695387.dirty


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/8] drm: xlnx: Fix kerneldoc

[Tomi Valkeinen]

On 20/03/2024 00:51, Sean Anderson wrote:
> Fix a few errors in the kerneldoc. Mostly this addresses missing/renamed
> members.
> 
> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
> ---
> 
> Changes in v2:
> - New
> 
>   drivers/gpu/drm/xlnx/zynqmp_disp.c  | 6 +++---
>   drivers/gpu/drm/xlnx/zynqmp_dpsub.h | 1 +
>   drivers/gpu/drm/xlnx/zynqmp_kms.h   | 4 ++--
>   3 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> index 407bc07cec69..f79bf3fb8110 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> @@ -128,9 +128,9 @@ struct zynqmp_disp_layer {
>    * struct zynqmp_disp - Display controller
>    * @dev: Device structure
>    * @dpsub: Display subsystem
> - * @blend.base: Register I/O base address for the blender
> - * @avbuf.base: Register I/O base address for the audio/video buffer manager
> - * @audio.base: Registers I/O base address for the audio mixer
> + * @blend: Register I/O base address for the blender
> + * @avbuf: Register I/O base address for the audio/video buffer manager
> + * @audio: Registers I/O base address for the audio mixer

Afaics, the kernel doc guide:

https://docs.kernel.org/doc-guide/kernel-doc.html#nested-structs-unions

says that the current version is correct. Or is the issue that while, 
say, 'base' is documented, 'blend' was not?

  Tomi


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/8] drm: xlnx: Fix kerneldoc

[Randy Dunlap]

On 3/19/24 22:42, Tomi Valkeinen wrote:
> On 20/03/2024 00:51, Sean Anderson wrote:
>> Fix a few errors in the kerneldoc. Mostly this addresses missing/renamed
>> members.
>>
>> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
>> ---
>>
>> Changes in v2:
>> - New
>>
>>   drivers/gpu/drm/xlnx/zynqmp_disp.c  | 6 +++---
>>   drivers/gpu/drm/xlnx/zynqmp_dpsub.h | 1 +
>>   drivers/gpu/drm/xlnx/zynqmp_kms.h   | 4 ++--
>>   3 files changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c b/drivers/gpu/drm/xlnx/zynqmp_disp.c
>> index 407bc07cec69..f79bf3fb8110 100644
>> --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
>> +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
>> @@ -128,9 +128,9 @@ struct zynqmp_disp_layer {
>>    * struct zynqmp_disp - Display controller
>>    * @dev: Device structure
>>    * @dpsub: Display subsystem
>> - * @blend.base: Register I/O base address for the blender
>> - * @avbuf.base: Register I/O base address for the audio/video buffer manager
>> - * @audio.base: Registers I/O base address for the audio mixer
>> + * @blend: Register I/O base address for the blender
>> + * @avbuf: Register I/O base address for the audio/video buffer manager
>> + * @audio: Registers I/O base address for the audio mixer
> 
> Afaics, the kernel doc guide:
> 
> https://docs.kernel.org/doc-guide/kernel-doc.html#nested-structs-unions
> 
> says that the current version is correct. Or is the issue that while, say, 'base' is documented, 'blend' was not?

Hi,

I would do it more like so:

---
 drivers/gpu/drm/xlnx/zynqmp_disp.c |    3 +++
 1 file changed, 3 insertions(+)

diff -- a/drivers/gpu/drm/xlnx/zynqmp_disp.c b/drivers/gpu/drm/xlnx/zynqmp_disp.c
--- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
@@ -128,8 +128,11 @@ struct zynqmp_disp_layer {
  * struct zynqmp_disp - Display controller
  * @dev: Device structure
  * @dpsub: Display subsystem
+ * @blend: blender iomem info
  * @blend.base: Register I/O base address for the blender
+ * @avbuf: audio/video buffer iomem info
  * @avbuf.base: Register I/O base address for the audio/video buffer manager
+ * @audio: audio mixer iomem info
  * @audio.base: Registers I/O base address for the audio mixer
  * @layers: Layers (planes)
  */


but in my testing, Sean's way or my way result in no warning/errors.

-- 
#Randy

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/8] drm: xlnx: Fix kerneldoc

[Sean Anderson]

On 3/20/24 02:05, Randy Dunlap wrote:
> 
> 
> On 3/19/24 22:42, Tomi Valkeinen wrote:
>> On 20/03/2024 00:51, Sean Anderson wrote:
>>> Fix a few errors in the kerneldoc. Mostly this addresses missing/renamed
>>> members.
>>>
>>> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
>>> ---
>>>
>>> Changes in v2:
>>> - New
>>>
>>>   drivers/gpu/drm/xlnx/zynqmp_disp.c  | 6 +++---
>>>   drivers/gpu/drm/xlnx/zynqmp_dpsub.h | 1 +
>>>   drivers/gpu/drm/xlnx/zynqmp_kms.h   | 4 ++--
>>>   3 files changed, 6 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c b/drivers/gpu/drm/xlnx/zynqmp_disp.c
>>> index 407bc07cec69..f79bf3fb8110 100644
>>> --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
>>> +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
>>> @@ -128,9 +128,9 @@ struct zynqmp_disp_layer {
>>>    * struct zynqmp_disp - Display controller
>>>    * @dev: Device structure
>>>    * @dpsub: Display subsystem
>>> - * @blend.base: Register I/O base address for the blender
>>> - * @avbuf.base: Register I/O base address for the audio/video buffer manager
>>> - * @audio.base: Registers I/O base address for the audio mixer
>>> + * @blend: Register I/O base address for the blender
>>> + * @avbuf: Register I/O base address for the audio/video buffer manager
>>> + * @audio: Registers I/O base address for the audio mixer
>> 
>> Afaics, the kernel doc guide:
>> 
>> https://docs.kernel.org/doc-guide/kernel-doc.html#nested-structs-unions
>> 
>> says that the current version is correct. Or is the issue that while, say, 'base' is documented, 'blend' was not?
> 
> Hi,
> 
> I would do it more like so:
> 
> ---
>  drivers/gpu/drm/xlnx/zynqmp_disp.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> diff -- a/drivers/gpu/drm/xlnx/zynqmp_disp.c b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> @@ -128,8 +128,11 @@ struct zynqmp_disp_layer {
>   * struct zynqmp_disp - Display controller
>   * @dev: Device structure
>   * @dpsub: Display subsystem
> + * @blend: blender iomem info
>   * @blend.base: Register I/O base address for the blender
> + * @avbuf: audio/video buffer iomem info
>   * @avbuf.base: Register I/O base address for the audio/video buffer manager
> + * @audio: audio mixer iomem info
>   * @audio.base: Registers I/O base address for the audio mixer
>   * @layers: Layers (planes)
>   */
> 
> 
> but in my testing, Sean's way or my way result in no warning/errors.
> 

The specific errors are:

../drivers/gpu/drm/xlnx/zynqmp_disp.c:151: warning: Function parameter or struct member 'blend' not described in 'zynqmp_disp'
../drivers/gpu/drm/xlnx/zynqmp_disp.c:151: warning: Function parameter or struct member 'avbuf' not described in 'zynqmp_disp'
../drivers/gpu/drm/xlnx/zynqmp_disp.c:151: warning: Function parameter or struct member 'audio' not described in 'zynqmp_disp'

I don't see the need to document a single-member struct twice. Actually,
maybe it would be better to just lift the .base member to live in
zynqmp_disp. But I think that would be better in another series.

--Sean

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/8] drm: xlnx: Fix kerneldoc

[Tomi Valkeinen]

On 21/03/2024 17:33, Sean Anderson wrote:
> On 3/20/24 02:05, Randy Dunlap wrote:
>>
>>
>> On 3/19/24 22:42, Tomi Valkeinen wrote:
>>> On 20/03/2024 00:51, Sean Anderson wrote:
>>>> Fix a few errors in the kerneldoc. Mostly this addresses missing/renamed
>>>> members.
>>>>
>>>> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
>>>> ---
>>>>
>>>> Changes in v2:
>>>> - New
>>>>
>>>>    drivers/gpu/drm/xlnx/zynqmp_disp.c  | 6 +++---
>>>>    drivers/gpu/drm/xlnx/zynqmp_dpsub.h | 1 +
>>>>    drivers/gpu/drm/xlnx/zynqmp_kms.h   | 4 ++--
>>>>    3 files changed, 6 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c b/drivers/gpu/drm/xlnx/zynqmp_disp.c
>>>> index 407bc07cec69..f79bf3fb8110 100644
>>>> --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
>>>> +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
>>>> @@ -128,9 +128,9 @@ struct zynqmp_disp_layer {
>>>>     * struct zynqmp_disp - Display controller
>>>>     * @dev: Device structure
>>>>     * @dpsub: Display subsystem
>>>> - * @blend.base: Register I/O base address for the blender
>>>> - * @avbuf.base: Register I/O base address for the audio/video buffer manager
>>>> - * @audio.base: Registers I/O base address for the audio mixer
>>>> + * @blend: Register I/O base address for the blender
>>>> + * @avbuf: Register I/O base address for the audio/video buffer manager
>>>> + * @audio: Registers I/O base address for the audio mixer
>>>
>>> Afaics, the kernel doc guide:
>>>
>>> https://docs.kernel.org/doc-guide/kernel-doc.html#nested-structs-unions
>>>
>>> says that the current version is correct. Or is the issue that while, say, 'base' is documented, 'blend' was not?
>>
>> Hi,
>>
>> I would do it more like so:
>>
>> ---
>>   drivers/gpu/drm/xlnx/zynqmp_disp.c |    3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff -- a/drivers/gpu/drm/xlnx/zynqmp_disp.c b/drivers/gpu/drm/xlnx/zynqmp_disp.c
>> --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
>> +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
>> @@ -128,8 +128,11 @@ struct zynqmp_disp_layer {
>>    * struct zynqmp_disp - Display controller
>>    * @dev: Device structure
>>    * @dpsub: Display subsystem
>> + * @blend: blender iomem info
>>    * @blend.base: Register I/O base address for the blender
>> + * @avbuf: audio/video buffer iomem info
>>    * @avbuf.base: Register I/O base address for the audio/video buffer manager
>> + * @audio: audio mixer iomem info
>>    * @audio.base: Registers I/O base address for the audio mixer
>>    * @layers: Layers (planes)
>>    */
>>
>>
>> but in my testing, Sean's way or my way result in no warning/errors.
>>
> 
> The specific errors are:
> 
> ../drivers/gpu/drm/xlnx/zynqmp_disp.c:151: warning: Function parameter or struct member 'blend' not described in 'zynqmp_disp'
> ../drivers/gpu/drm/xlnx/zynqmp_disp.c:151: warning: Function parameter or struct member 'avbuf' not described in 'zynqmp_disp'
> ../drivers/gpu/drm/xlnx/zynqmp_disp.c:151: warning: Function parameter or struct member 'audio' not described in 'zynqmp_disp'
> 
> I don't see the need to document a single-member struct twice. Actually,

But if only the struct is documented, then we're documenting the wrong 
thing. A tool showing to the user what blend.base is would miss that 
documentation.

> maybe it would be better to just lift the .base member to live in
> zynqmp_disp. But I think that would be better in another series.

Yes, there's not much point with the structs.

  Tomi


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/8] drm: xlnx: Fix kerneldoc

[Sean Anderson]

On 3/22/24 01:50, Tomi Valkeinen wrote:
> On 21/03/2024 17:33, Sean Anderson wrote:
>> On 3/20/24 02:05, Randy Dunlap wrote:
>>>
>>>
>>> On 3/19/24 22:42, Tomi Valkeinen wrote:
>>>> On 20/03/2024 00:51, Sean Anderson wrote:
>>>>> Fix a few errors in the kerneldoc. Mostly this addresses missing/renamed
>>>>> members.
>>>>>
>>>>> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
>>>>> ---
>>>>>
>>>>> Changes in v2:
>>>>> - New
>>>>>
>>>>>    drivers/gpu/drm/xlnx/zynqmp_disp.c  | 6 +++---
>>>>>    drivers/gpu/drm/xlnx/zynqmp_dpsub.h | 1 +
>>>>>    drivers/gpu/drm/xlnx/zynqmp_kms.h   | 4 ++--
>>>>>    3 files changed, 6 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c b/drivers/gpu/drm/xlnx/zynqmp_disp.c
>>>>> index 407bc07cec69..f79bf3fb8110 100644
>>>>> --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
>>>>> +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
>>>>> @@ -128,9 +128,9 @@ struct zynqmp_disp_layer {
>>>>>     * struct zynqmp_disp - Display controller
>>>>>     * @dev: Device structure
>>>>>     * @dpsub: Display subsystem
>>>>> - * @blend.base: Register I/O base address for the blender
>>>>> - * @avbuf.base: Register I/O base address for the audio/video buffer manager
>>>>> - * @audio.base: Registers I/O base address for the audio mixer
>>>>> + * @blend: Register I/O base address for the blender
>>>>> + * @avbuf: Register I/O base address for the audio/video buffer manager
>>>>> + * @audio: Registers I/O base address for the audio mixer
>>>>
>>>> Afaics, the kernel doc guide:
>>>>
>>>> https://docs.kernel.org/doc-guide/kernel-doc.html#nested-structs-unions
>>>>
>>>> says that the current version is correct. Or is the issue that while, say, 'base' is documented, 'blend' was not?
>>>
>>> Hi,
>>>
>>> I would do it more like so:
>>>
>>> ---
>>>   drivers/gpu/drm/xlnx/zynqmp_disp.c |    3 +++
>>>   1 file changed, 3 insertions(+)
>>>
>>> diff -- a/drivers/gpu/drm/xlnx/zynqmp_disp.c b/drivers/gpu/drm/xlnx/zynqmp_disp.c
>>> --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
>>> +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
>>> @@ -128,8 +128,11 @@ struct zynqmp_disp_layer {
>>>    * struct zynqmp_disp - Display controller
>>>    * @dev: Device structure
>>>    * @dpsub: Display subsystem
>>> + * @blend: blender iomem info
>>>    * @blend.base: Register I/O base address for the blender
>>> + * @avbuf: audio/video buffer iomem info
>>>    * @avbuf.base: Register I/O base address for the audio/video buffer manager
>>> + * @audio: audio mixer iomem info
>>>    * @audio.base: Registers I/O base address for the audio mixer
>>>    * @layers: Layers (planes)
>>>    */
>>>
>>>
>>> but in my testing, Sean's way or my way result in no warning/errors.
>>>
>>
>> The specific errors are:
>>
>> ../drivers/gpu/drm/xlnx/zynqmp_disp.c:151: warning: Function parameter or struct member 'blend' not described in 'zynqmp_disp'
>> ../drivers/gpu/drm/xlnx/zynqmp_disp.c:151: warning: Function parameter or struct member 'avbuf' not described in 'zynqmp_disp'
>> ../drivers/gpu/drm/xlnx/zynqmp_disp.c:151: warning: Function parameter or struct member 'audio' not described in 'zynqmp_disp'
>>
>> I don't see the need to document a single-member struct twice. Actually,
> 
> But if only the struct is documented, then we're documenting the wrong thing. A tool showing to the user what blend.base is would miss that documentation.

Are there any such tools? kerneldoc e.g. just prints the definition and
then a list of members with documentation. So from the user's
perspective the only thing which changes is the name.

--Sean

>> maybe it would be better to just lift the .base member to live in
>> zynqmp_disp. But I think that would be better in another series.
> 
> Yes, there's not much point with the structs.
> 
>  Tomi
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 2/8] drm: zynqmp_dp: Downgrade log level for aux retries message

[Sean Anderson]

Enable this message for verbose debugging only as it is otherwise
printed after every AUX message, quickly filling the log buffer.

Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---

(no changes since v1)

 drivers/gpu/drm/xlnx/zynqmp_dp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
index a0606fab0e22..98a32e6a0459 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
@@ -1006,7 +1006,7 @@ zynqmp_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
 					       msg->buffer, msg->size,
 					       &msg->reply);
 		if (!ret) {
-			dev_dbg(dp->dev, "aux %d retries\n", i);
+			dev_vdbg(dp->dev, "aux %d retries\n", i);
 			return msg->size;
 		}
 
-- 
2.35.1.1320.gc452695387.dirty


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/8] drm: zynqmp_dp: Downgrade log level for aux retries message

[Tomi Valkeinen]

On 20/03/2024 00:51, Sean Anderson wrote:
> Enable this message for verbose debugging only as it is otherwise
> printed after every AUX message, quickly filling the log buffer.
> 
> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> 
> (no changes since v1)
> 
>   drivers/gpu/drm/xlnx/zynqmp_dp.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> index a0606fab0e22..98a32e6a0459 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> @@ -1006,7 +1006,7 @@ zynqmp_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
>   					       msg->buffer, msg->size,
>   					       &msg->reply);
>   		if (!ret) {
> -			dev_dbg(dp->dev, "aux %d retries\n", i);
> +			dev_vdbg(dp->dev, "aux %d retries\n", i);
>   			return msg->size;
>   		}
>   

Yes, these are annoying... In my work branch I had added "if (i)" there, 
so that this is only printed if there actually are retries.

But this is fine too (or even dropping the print totally), so:

Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

  Tomi


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 3/8] drm: zynqmp_dp: Adjust training values per-lane

[Sean Anderson]

The feedback we get from the DPRX is per-lane. Make changes using this
information, instead of picking the maximum values from all lanes. This
results in more-consistent training on marginal links.

Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
---

(no changes since v1)

 drivers/gpu/drm/xlnx/zynqmp_dp.c | 23 ++++++++---------------
 1 file changed, 8 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
index 98a32e6a0459..8635b5673386 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
@@ -605,28 +605,21 @@ static void zynqmp_dp_adjust_train(struct zynqmp_dp *dp,
 				   u8 link_status[DP_LINK_STATUS_SIZE])
 {
 	u8 *train_set = dp->train_set;
-	u8 voltage = 0, preemphasis = 0;
 	u8 i;
 
 	for (i = 0; i < dp->mode.lane_cnt; i++) {
-		u8 v = drm_dp_get_adjust_request_voltage(link_status, i);
-		u8 p = drm_dp_get_adjust_request_pre_emphasis(link_status, i);
+		u8 voltage = drm_dp_get_adjust_request_voltage(link_status, i);
+		u8 preemphasis =
+			drm_dp_get_adjust_request_pre_emphasis(link_status, i);
 
-		if (v > voltage)
-			voltage = v;
+		if (voltage >= DP_TRAIN_VOLTAGE_SWING_LEVEL_3)
+			voltage |= DP_TRAIN_MAX_SWING_REACHED;
 
-		if (p > preemphasis)
-			preemphasis = p;
-	}
+		if (preemphasis >= DP_TRAIN_PRE_EMPH_LEVEL_2)
+			preemphasis |= DP_TRAIN_MAX_PRE_EMPHASIS_REACHED;
 
-	if (voltage >= DP_TRAIN_VOLTAGE_SWING_LEVEL_3)
-		voltage |= DP_TRAIN_MAX_SWING_REACHED;
-
-	if (preemphasis >= DP_TRAIN_PRE_EMPH_LEVEL_2)
-		preemphasis |= DP_TRAIN_MAX_PRE_EMPHASIS_REACHED;
-
-	for (i = 0; i < dp->mode.lane_cnt; i++)
 		train_set[i] = voltage | preemphasis;
+	}
 }
 
 /**
-- 
2.35.1.1320.gc452695387.dirty


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 3/8] drm: zynqmp_dp: Adjust training values per-lane

[Tomi Valkeinen]

On 20/03/2024 00:51, Sean Anderson wrote:
> The feedback we get from the DPRX is per-lane. Make changes using this
> information, instead of picking the maximum values from all lanes. This
> results in more-consistent training on marginal links.
> 
> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
> ---
> 
> (no changes since v1)
> 
>   drivers/gpu/drm/xlnx/zynqmp_dp.c | 23 ++++++++---------------
>   1 file changed, 8 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> index 98a32e6a0459..8635b5673386 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> @@ -605,28 +605,21 @@ static void zynqmp_dp_adjust_train(struct zynqmp_dp *dp,
>   				   u8 link_status[DP_LINK_STATUS_SIZE])
>   {
>   	u8 *train_set = dp->train_set;
> -	u8 voltage = 0, preemphasis = 0;
>   	u8 i;
>   
>   	for (i = 0; i < dp->mode.lane_cnt; i++) {
> -		u8 v = drm_dp_get_adjust_request_voltage(link_status, i);
> -		u8 p = drm_dp_get_adjust_request_pre_emphasis(link_status, i);
> +		u8 voltage = drm_dp_get_adjust_request_voltage(link_status, i);
> +		u8 preemphasis =
> +			drm_dp_get_adjust_request_pre_emphasis(link_status, i);
>   
> -		if (v > voltage)
> -			voltage = v;
> +		if (voltage >= DP_TRAIN_VOLTAGE_SWING_LEVEL_3)
> +			voltage |= DP_TRAIN_MAX_SWING_REACHED;
>   
> -		if (p > preemphasis)
> -			preemphasis = p;
> -	}
> +		if (preemphasis >= DP_TRAIN_PRE_EMPH_LEVEL_2)
> +			preemphasis |= DP_TRAIN_MAX_PRE_EMPHASIS_REACHED;
>   
> -	if (voltage >= DP_TRAIN_VOLTAGE_SWING_LEVEL_3)
> -		voltage |= DP_TRAIN_MAX_SWING_REACHED;
> -
> -	if (preemphasis >= DP_TRAIN_PRE_EMPH_LEVEL_2)
> -		preemphasis |= DP_TRAIN_MAX_PRE_EMPHASIS_REACHED;
> -
> -	for (i = 0; i < dp->mode.lane_cnt; i++)
>   		train_set[i] = voltage | preemphasis;
> +	}
>   }
>   
>   /**

Looks fine to me, but a few cosmetic suggestions, feel free to ignore if 
not to your liking:

1)

u8 voltage, preemphasis;

voltage = drm_dp_get_adjust_request_voltage(link_status, i);
preemphasis = drm_dp_get_adjust_request_pre_emphasis(link_status, i);

2)

for (unsigned int i = 0; i < dp->mode.lane_cnt; i++)

3)

dp->train_set[i] = voltage | preemphasis;


Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

  Tomi


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 3/8] drm: zynqmp_dp: Adjust training values per-lane

[Sean Anderson]

On 3/20/24 01:57, Tomi Valkeinen wrote:
> On 20/03/2024 00:51, Sean Anderson wrote:
>> The feedback we get from the DPRX is per-lane. Make changes using this
>> information, instead of picking the maximum values from all lanes. This
>> results in more-consistent training on marginal links.
>>
>> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
>> ---
>>
>> (no changes since v1)
>>
>>   drivers/gpu/drm/xlnx/zynqmp_dp.c | 23 ++++++++---------------
>>   1 file changed, 8 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
>> index 98a32e6a0459..8635b5673386 100644
>> --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
>> +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
>> @@ -605,28 +605,21 @@ static void zynqmp_dp_adjust_train(struct zynqmp_dp *dp,
>>                      u8 link_status[DP_LINK_STATUS_SIZE])
>>   {
>>       u8 *train_set = dp->train_set;
>> -    u8 voltage = 0, preemphasis = 0;
>>       u8 i;
>>         for (i = 0; i < dp->mode.lane_cnt; i++) {
>> -        u8 v = drm_dp_get_adjust_request_voltage(link_status, i);
>> -        u8 p = drm_dp_get_adjust_request_pre_emphasis(link_status, i);
>> +        u8 voltage = drm_dp_get_adjust_request_voltage(link_status, i);
>> +        u8 preemphasis =
>> +            drm_dp_get_adjust_request_pre_emphasis(link_status, i);
>>   -        if (v > voltage)
>> -            voltage = v;
>> +        if (voltage >= DP_TRAIN_VOLTAGE_SWING_LEVEL_3)
>> +            voltage |= DP_TRAIN_MAX_SWING_REACHED;
>>   -        if (p > preemphasis)
>> -            preemphasis = p;
>> -    }
>> +        if (preemphasis >= DP_TRAIN_PRE_EMPH_LEVEL_2)
>> +            preemphasis |= DP_TRAIN_MAX_PRE_EMPHASIS_REACHED;
>>   -    if (voltage >= DP_TRAIN_VOLTAGE_SWING_LEVEL_3)
>> -        voltage |= DP_TRAIN_MAX_SWING_REACHED;
>> -
>> -    if (preemphasis >= DP_TRAIN_PRE_EMPH_LEVEL_2)
>> -        preemphasis |= DP_TRAIN_MAX_PRE_EMPHASIS_REACHED;
>> -
>> -    for (i = 0; i < dp->mode.lane_cnt; i++)
>>           train_set[i] = voltage | preemphasis;
>> +    }
>>   }
>>     /**
> 
> Looks fine to me, but a few cosmetic suggestions, feel free to ignore if not to your liking:
> 
> 1)
> 
> u8 voltage, preemphasis;
> 
> voltage = drm_dp_get_adjust_request_voltage(link_status, i);
> preemphasis = drm_dp_get_adjust_request_pre_emphasis(link_status, i);

If the comment here is about the line break, I agree that this looks
better but the second line is over 80 characters.

> 2)
> 
> for (unsigned int i = 0; i < dp->mode.lane_cnt; i++)

Is this allowed now?

> 3)
> 
> dp->train_set[i] = voltage | preemphasis;

This would be undone in patch 7/8.

--Sean

> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> 
>  Tomi
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 4/8] drm: zynqmp_dp: Rearrange zynqmp_dp for better padding

[Sean Anderson]

Sort the members of struct zynqmp_dp to reduce padding necessary for
alignment.

Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
---

Changes in v2:
- New

 drivers/gpu/drm/xlnx/zynqmp_dp.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
index 8635b5673386..f1834c8e3c02 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
@@ -255,10 +255,10 @@ struct zynqmp_dp_link_config {
  * @fmt: format identifier string
  */
 struct zynqmp_dp_mode {
-	u8 bw_code;
-	u8 lane_cnt;
-	int pclock;
 	const char *fmt;
+	int pclock;
+	u8 bw_code;
+	u8 lane_cnt;
 };
 
 /**
@@ -295,27 +295,27 @@ struct zynqmp_dp_config {
  * @train_set: set of training data
  */
 struct zynqmp_dp {
+	struct drm_dp_aux aux;
+	struct drm_bridge bridge;
+	struct delayed_work hpd_work;
+
+	struct drm_bridge *next_bridge;
 	struct device *dev;
 	struct zynqmp_dpsub *dpsub;
 	void __iomem *iomem;
 	struct reset_control *reset;
-	int irq;
-
-	struct drm_bridge bridge;
-	struct drm_bridge *next_bridge;
-
-	struct zynqmp_dp_config config;
-	struct drm_dp_aux aux;
 	struct phy *phy[ZYNQMP_DP_MAX_LANES];
-	u8 num_lanes;
-	struct delayed_work hpd_work;
+
 	enum drm_connector_status status;
+	int irq;
 	bool enabled;
 
-	u8 dpcd[DP_RECEIVER_CAP_SIZE];
-	struct zynqmp_dp_link_config link_config;
 	struct zynqmp_dp_mode mode;
+	struct zynqmp_dp_link_config link_config;
+	struct zynqmp_dp_config config;
+	u8 dpcd[DP_RECEIVER_CAP_SIZE];
 	u8 train_set[ZYNQMP_DP_MAX_LANES];
+	u8 num_lanes;
 };
 
 static inline struct zynqmp_dp *bridge_to_dp(struct drm_bridge *bridge)
-- 
2.35.1.1320.gc452695387.dirty


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 4/8] drm: zynqmp_dp: Rearrange zynqmp_dp for better padding

[Tomi Valkeinen]

On 20/03/2024 00:51, Sean Anderson wrote:
> Sort the members of struct zynqmp_dp to reduce padding necessary for
> alignment.
> 
> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
> ---
> 
> Changes in v2:
> - New
> 
>   drivers/gpu/drm/xlnx/zynqmp_dp.c | 28 ++++++++++++++--------------
>   1 file changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> index 8635b5673386..f1834c8e3c02 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> @@ -255,10 +255,10 @@ struct zynqmp_dp_link_config {
>    * @fmt: format identifier string
>    */
>   struct zynqmp_dp_mode {
> -	u8 bw_code;
> -	u8 lane_cnt;
> -	int pclock;
>   	const char *fmt;
> +	int pclock;
> +	u8 bw_code;
> +	u8 lane_cnt;
>   };
>   
>   /**
> @@ -295,27 +295,27 @@ struct zynqmp_dp_config {
>    * @train_set: set of training data
>    */
>   struct zynqmp_dp {
> +	struct drm_dp_aux aux;
> +	struct drm_bridge bridge;
> +	struct delayed_work hpd_work;
> +
> +	struct drm_bridge *next_bridge;
>   	struct device *dev;
>   	struct zynqmp_dpsub *dpsub;
>   	void __iomem *iomem;
>   	struct reset_control *reset;
> -	int irq;
> -
> -	struct drm_bridge bridge;
> -	struct drm_bridge *next_bridge;
> -
> -	struct zynqmp_dp_config config;
> -	struct drm_dp_aux aux;
>   	struct phy *phy[ZYNQMP_DP_MAX_LANES];
> -	u8 num_lanes;
> -	struct delayed_work hpd_work;
> +
>   	enum drm_connector_status status;
> +	int irq;
>   	bool enabled;
>   
> -	u8 dpcd[DP_RECEIVER_CAP_SIZE];
> -	struct zynqmp_dp_link_config link_config;
>   	struct zynqmp_dp_mode mode;
> +	struct zynqmp_dp_link_config link_config;
> +	struct zynqmp_dp_config config;
> +	u8 dpcd[DP_RECEIVER_CAP_SIZE];
>   	u8 train_set[ZYNQMP_DP_MAX_LANES];
> +	u8 num_lanes;
>   };
>   
>   static inline struct zynqmp_dp *bridge_to_dp(struct drm_bridge *bridge)

If you change the order of the fields, you should change the order in 
the kernel doc accordingly.

To be honest, I'm not sure if I like this patch. We have usually one 
instance of these structs allocated. How many bytes do we save?

I'm fine with getting easy savings by changing the field order in some 
cases, but I think the "human" side of the order is important too: 
usually the fields are grouped in some way, and ordered so that the more 
base or generic ones are first, and fields for some specific feature are 
later. And fields protected by a lock should be grouped together, with 
their lock being first/last in that group.

Looking at the zynqmp_dp struct with this patch, I get an urge to start 
moving things around: dev, dpsub, iomem, etc first, hpd somewhere later. 
Base config fields like config, num_lanes, irq would be grouped 
together. Etc.

  Tomi


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 4/8] drm: zynqmp_dp: Rearrange zynqmp_dp for better padding

[Sean Anderson]

On 3/20/24 02:14, Tomi Valkeinen wrote:
> On 20/03/2024 00:51, Sean Anderson wrote:
>> Sort the members of struct zynqmp_dp to reduce padding necessary for
>> alignment.
>>
>> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
>> ---
>>
>> Changes in v2:
>> - New
>>
>>   drivers/gpu/drm/xlnx/zynqmp_dp.c | 28 ++++++++++++++--------------
>>   1 file changed, 14 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
>> index 8635b5673386..f1834c8e3c02 100644
>> --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
>> +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
>> @@ -255,10 +255,10 @@ struct zynqmp_dp_link_config {
>>    * @fmt: format identifier string
>>    */
>>   struct zynqmp_dp_mode {
>> -    u8 bw_code;
>> -    u8 lane_cnt;
>> -    int pclock;
>>       const char *fmt;
>> +    int pclock;
>> +    u8 bw_code;
>> +    u8 lane_cnt;
>>   };
>>     /**
>> @@ -295,27 +295,27 @@ struct zynqmp_dp_config {
>>    * @train_set: set of training data
>>    */
>>   struct zynqmp_dp {
>> +    struct drm_dp_aux aux;
>> +    struct drm_bridge bridge;
>> +    struct delayed_work hpd_work;
>> +
>> +    struct drm_bridge *next_bridge;
>>       struct device *dev;
>>       struct zynqmp_dpsub *dpsub;
>>       void __iomem *iomem;
>>       struct reset_control *reset;
>> -    int irq;
>> -
>> -    struct drm_bridge bridge;
>> -    struct drm_bridge *next_bridge;
>> -
>> -    struct zynqmp_dp_config config;
>> -    struct drm_dp_aux aux;
>>       struct phy *phy[ZYNQMP_DP_MAX_LANES];
>> -    u8 num_lanes;
>> -    struct delayed_work hpd_work;
>> +
>>       enum drm_connector_status status;
>> +    int irq;
>>       bool enabled;
>>   -    u8 dpcd[DP_RECEIVER_CAP_SIZE];
>> -    struct zynqmp_dp_link_config link_config;
>>       struct zynqmp_dp_mode mode;
>> +    struct zynqmp_dp_link_config link_config;
>> +    struct zynqmp_dp_config config;
>> +    u8 dpcd[DP_RECEIVER_CAP_SIZE];
>>       u8 train_set[ZYNQMP_DP_MAX_LANES];
>> +    u8 num_lanes;
>>   };
>>     static inline struct zynqmp_dp *bridge_to_dp(struct drm_bridge *bridge)
> 
> If you change the order of the fields, you should change the order in the kernel doc accordingly.

The kernel doc is documentation, so it should continue to group similar
functionality together.

> To be honest, I'm not sure if I like this patch. We have usually one instance of these structs allocated. How many bytes do we save?

Actually, the main reason is to make it easy to determine where to
insert new members. Stick the pointers with the pointers, u8s with u8s,
etc.

> I'm fine with getting easy savings by changing the field order in some cases, but I think the "human" side of the order is important too: usually the fields are grouped in some way, and ordered so that the more base or generic ones are first, and fields for some specific feature are later. And fields protected by a lock should be grouped together, with their lock being first/last in that group.
> 
> Looking at the zynqmp_dp struct with this patch, I get an urge to start moving things around: dev, dpsub, iomem, etc first, hpd somewhere later. Base config fields like config, num_lanes, irq would be grouped together. Etc.

--Sean

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 5/8] drm: zynqmp_dp: Don't retrain the link in our IRQ

[Sean Anderson]

Retraining the link can take a while, and might involve waiting for
DPCD reads/writes to complete. This is inappropriate for an IRQ handler.
Just schedule this work for later completion. This is racy, but will be
fixed in the next commit.

Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
---
Actually, on second look this IRQ is threaded. So why do we have a
workqueue for HPD events? Maybe we should make it unthreaded?

Changes in v2:
- Document hpd_irq_work
- Split this off from the lcoking changes

 drivers/gpu/drm/xlnx/zynqmp_dp.c | 43 ++++++++++++++++++++------------
 1 file changed, 27 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
index f1834c8e3c02..f3fcdbf662fa 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
@@ -287,6 +287,7 @@ struct zynqmp_dp_config {
  * @phy: PHY handles for DP lanes
  * @num_lanes: number of enabled phy lanes
  * @hpd_work: hot plug detection worker
+ * @hpd_irq_work: hot plug detection IRQ worker
  * @status: connection status
  * @enabled: flag to indicate if the device is enabled
  * @dpcd: DP configuration data from currently connected sink device
@@ -298,6 +299,7 @@ struct zynqmp_dp {
 	struct drm_dp_aux aux;
 	struct drm_bridge bridge;
 	struct delayed_work hpd_work;
+	struct delayed_work hpd_irq_work;
 
 	struct drm_bridge *next_bridge;
 	struct device *dev;
@@ -1611,6 +1613,27 @@ static void zynqmp_dp_hpd_work_func(struct work_struct *work)
 	drm_bridge_hpd_notify(&dp->bridge, status);
 }
 
+static void zynqmp_dp_hpd_irq_work_func(struct work_struct *work)
+{
+	struct zynqmp_dp *dp = container_of(work, struct zynqmp_dp,
+					    hpd_irq_work.work);
+	u8 status[DP_LINK_STATUS_SIZE + 2];
+	int err;
+
+	err = drm_dp_dpcd_read(&dp->aux, DP_SINK_COUNT, status,
+			       DP_LINK_STATUS_SIZE + 2);
+	if (err < 0) {
+		dev_dbg_ratelimited(dp->dev,
+				    "could not read sink status: %d\n", err);
+	} else {
+		if (status[4] & DP_LINK_STATUS_UPDATED ||
+		    !drm_dp_clock_recovery_ok(&status[2], dp->mode.lane_cnt) ||
+		    !drm_dp_channel_eq_ok(&status[2], dp->mode.lane_cnt)) {
+			zynqmp_dp_train_loop(dp);
+		}
+	}
+}
+
 static irqreturn_t zynqmp_dp_irq_handler(int irq, void *data)
 {
 	struct zynqmp_dp *dp = (struct zynqmp_dp *)data;
@@ -1635,23 +1658,9 @@ static irqreturn_t zynqmp_dp_irq_handler(int irq, void *data)
 	if (status & ZYNQMP_DP_INT_HPD_EVENT)
 		schedule_delayed_work(&dp->hpd_work, 0);
 
-	if (status & ZYNQMP_DP_INT_HPD_IRQ) {
-		int ret;
-		u8 status[DP_LINK_STATUS_SIZE + 2];
+	if (status & ZYNQMP_DP_INT_HPD_IRQ)
+		schedule_delayed_work(&dp->hpd_irq_work, 0);
 
-		ret = drm_dp_dpcd_read(&dp->aux, DP_SINK_COUNT, status,
-				       DP_LINK_STATUS_SIZE + 2);
-		if (ret < 0)
-			goto handled;
-
-		if (status[4] & DP_LINK_STATUS_UPDATED ||
-		    !drm_dp_clock_recovery_ok(&status[2], dp->mode.lane_cnt) ||
-		    !drm_dp_channel_eq_ok(&status[2], dp->mode.lane_cnt)) {
-			zynqmp_dp_train_loop(dp);
-		}
-	}
-
-handled:
 	return IRQ_HANDLED;
 }
 
@@ -1676,6 +1685,7 @@ int zynqmp_dp_probe(struct zynqmp_dpsub *dpsub)
 	dp->status = connector_status_disconnected;
 
 	INIT_DELAYED_WORK(&dp->hpd_work, zynqmp_dp_hpd_work_func);
+	INIT_DELAYED_WORK(&dp->hpd_irq_work, zynqmp_dp_hpd_irq_work_func);
 
 	/* Acquire all resources (IOMEM, IRQ and PHYs). */
 	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dp");
@@ -1775,6 +1785,7 @@ void zynqmp_dp_remove(struct zynqmp_dpsub *dpsub)
 	zynqmp_dp_write(dp, ZYNQMP_DP_INT_DS, ZYNQMP_DP_INT_ALL);
 	disable_irq(dp->irq);
 
+	cancel_delayed_work_sync(&dp->hpd_irq_work);
 	cancel_delayed_work_sync(&dp->hpd_work);
 
 	zynqmp_dp_write(dp, ZYNQMP_DP_TRANSMITTER_ENABLE, 0);
-- 
2.35.1.1320.gc452695387.dirty


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 5/8] drm: zynqmp_dp: Don't retrain the link in our IRQ

[Tomi Valkeinen]

On 20/03/2024 00:51, Sean Anderson wrote:
> Retraining the link can take a while, and might involve waiting for
> DPCD reads/writes to complete. This is inappropriate for an IRQ handler.
> Just schedule this work for later completion. This is racy, but will be
> fixed in the next commit.

You should add the locks first, and use them here, rather than first 
adding a buggy commit and fixing it in the next one.

> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
> ---
> Actually, on second look this IRQ is threaded. So why do we have a
> workqueue for HPD events? Maybe we should make it unthreaded?

Indeed, there's not much work being done in the IRQ handler. I don't 
know why it's threaded.

We could move the queued work to be inside the threaded irq handler, but 
with a quick look, the HPD work has lines like "msleep(100)" (and that's 
inside a for loop...), which is probably not a good thing to do even in 
threaded irq handler.

Although I'm not sure if that code is good to have anywhere. Why do we 
even have such code in the HPD work path... We already got the HPD 
interrupt. What does "It takes some delay (ex, 100 ~ 500 msec) to get 
the HPD signal with some monitors" even mean...

Would it be possible to clean up the work funcs a bit (I haven't looked 
a the new work func yet), to remove the worst extra sleeps, and just do 
all that inside the threaded irq handler?

Do we need to handle interrupts while either delayed work is being done?

If we do need a delayed work, would just one work be enough which 
handles both HPD_EVENT and HPD_IRQ, instead of two?

  Tomi


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 5/8] drm: zynqmp_dp: Don't retrain the link in our IRQ

[Sean Anderson]

On 3/20/24 02:53, Tomi Valkeinen wrote:
> On 20/03/2024 00:51, Sean Anderson wrote:
>> Retraining the link can take a while, and might involve waiting for
>> DPCD reads/writes to complete. This is inappropriate for an IRQ handler.
>> Just schedule this work for later completion. This is racy, but will be
>> fixed in the next commit.
> 
> You should add the locks first, and use them here, rather than first
> adding a buggy commit and fixing it in the next one.

I didn't think I could add the locks first since I only noticed the IRQ
was threaded right before sending out this series. So yeah, we could add
locking, add the workqueue, and then unthread the IRQ.

>> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
>> ---
>> Actually, on second look this IRQ is threaded. So why do we have a
>> workqueue for HPD events? Maybe we should make it unthreaded?
> 
> Indeed, there's not much work being done in the IRQ handler. I don't know why it's threaded.
> 
> We could move the queued work to be inside the threaded irq handler,
> but with a quick look, the HPD work has lines like "msleep(100)" (and
> that's inside a for loop...), which is probably not a good thing to do
> even in threaded irq handler.
> 
> Although I'm not sure if that code is good to have anywhere. Why do we
> even have such code in the HPD work path... We already got the HPD
> interrupt. What does "It takes some delay (ex, 100 ~ 500 msec) to get
> the HPD signal with some monitors" even mean...

The documentation for this bit is

| HPD_STATE	0	ro	0x0	Contains the raw state of the HPD pin on the DisplayPort connector.

So I think the idea is to perform some debouncing.

> Would it be possible to clean up the work funcs a bit (I haven't
> looked a the new work func yet), to remove the worst extra sleeps, and
> just do all that inside the threaded irq handler?

Probably not, since a HPD IRQ results in link retraining, which can take a while.

> Do we need to handle interrupts while either delayed work is being done?

Probably not.

> If we do need a delayed work, would just one work be enough which
> handles both HPD_EVENT and HPD_IRQ, instead of two?

Maybe, but then we need to determine which pending events we need to
handle. I think since we have only two events it will be easier to just
have separate workqueues.

--Sean

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 5/8] drm: zynqmp_dp: Don't retrain the link in our IRQ

[Tomi Valkeinen]

On 21/03/2024 17:52, Sean Anderson wrote:
> On 3/20/24 02:53, Tomi Valkeinen wrote:
>> On 20/03/2024 00:51, Sean Anderson wrote:
>>> Retraining the link can take a while, and might involve waiting for
>>> DPCD reads/writes to complete. This is inappropriate for an IRQ handler.
>>> Just schedule this work for later completion. This is racy, but will be
>>> fixed in the next commit.
>>
>> You should add the locks first, and use them here, rather than first
>> adding a buggy commit and fixing it in the next one.
> 
> I didn't think I could add the locks first since I only noticed the IRQ
> was threaded right before sending out this series. So yeah, we could add
> locking, add the workqueue, and then unthread the IRQ.
> 
>>> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
>>> ---
>>> Actually, on second look this IRQ is threaded. So why do we have a
>>> workqueue for HPD events? Maybe we should make it unthreaded?
>>
>> Indeed, there's not much work being done in the IRQ handler. I don't know why it's threaded.
>>
>> We could move the queued work to be inside the threaded irq handler,
>> but with a quick look, the HPD work has lines like "msleep(100)" (and
>> that's inside a for loop...), which is probably not a good thing to do
>> even in threaded irq handler.
>>
>> Although I'm not sure if that code is good to have anywhere. Why do we
>> even have such code in the HPD work path... We already got the HPD
>> interrupt. What does "It takes some delay (ex, 100 ~ 500 msec) to get
>> the HPD signal with some monitors" even mean...
> 
> The documentation for this bit is
> 
> | HPD_STATE	0	ro	0x0	Contains the raw state of the HPD pin on the DisplayPort connector.
> 
> So I think the idea is to perform some debouncing.

Hmm, it just looks a bit odd to me. It can sleep for a second. And the 
wording "It takes some delay (ex, 100 ~ 500 msec) to get the HPD signal 
with some monitors" makes it sound like some kind of a hack...

The docs mention debounce once:

https://docs.amd.com/r/en-US/pg299-v-dp-txss1/Hot-Plug-Detection

But it's not immediately obvious what the SW must do and what's done by 
the HW. Debounce is not mentioned later, e.g. in the HPD Event Handling. 
But if debounce is needed, wouldn't it be perhaps in a few milliseconds, 
instead of hundreds of milliseconds...

zynqmp_dp_bridge_detect() is used for drm_bridge_funcs.detect(), and if 
the cable is not connected, it'll sleep for 1 second (probably more) 
until returning not connected. It just doesn't sound correct to me.

Well, it's not part of this patch as such, but related to the amount of 
time we spend in the interrupt handler (and also the detect()).

>> Would it be possible to clean up the work funcs a bit (I haven't
>> looked a the new work func yet), to remove the worst extra sleeps, and
>> just do all that inside the threaded irq handler?
> 
> Probably not, since a HPD IRQ results in link retraining, which can take a while.

But is it any different if you have a workqueue? Isn't a threaded 
interrupt handler basically the same thing?

Probably at least the zynqmp_dp_hpd_work_func() could be done in the 
threaded irq just fine, if the insane 1s sleep can be dropped.

>> Do we need to handle interrupts while either delayed work is being done?
> 
> Probably not.
> 
>> If we do need a delayed work, would just one work be enough which
>> handles both HPD_EVENT and HPD_IRQ, instead of two?
> 
> Maybe, but then we need to determine which pending events we need to
> handle. I think since we have only two events it will be easier to just
> have separate workqueues.

The less concurrency, the better...Which is why it would be nice to do 
it all in the threaded irq.

  Tomi


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 5/8] drm: zynqmp_dp: Don't retrain the link in our IRQ

[Sean Anderson]

On 3/21/24 13:25, Tomi Valkeinen wrote:
> On 21/03/2024 17:52, Sean Anderson wrote:
>> On 3/20/24 02:53, Tomi Valkeinen wrote:
>>> On 20/03/2024 00:51, Sean Anderson wrote:
>>>> Retraining the link can take a while, and might involve waiting for
>>>> DPCD reads/writes to complete. This is inappropriate for an IRQ handler.
>>>> Just schedule this work for later completion. This is racy, but will be
>>>> fixed in the next commit.
>>>
>>> You should add the locks first, and use them here, rather than first
>>> adding a buggy commit and fixing it in the next one.
>>
>> I didn't think I could add the locks first since I only noticed the IRQ
>> was threaded right before sending out this series. So yeah, we could add
>> locking, add the workqueue, and then unthread the IRQ.
>>
>>>> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
>>>> ---
>>>> Actually, on second look this IRQ is threaded. So why do we have a
>>>> workqueue for HPD events? Maybe we should make it unthreaded?
>>>
>>> Indeed, there's not much work being done in the IRQ handler. I don't know why it's threaded.
>>>
>>> We could move the queued work to be inside the threaded irq handler,
>>> but with a quick look, the HPD work has lines like "msleep(100)" (and
>>> that's inside a for loop...), which is probably not a good thing to do
>>> even in threaded irq handler.
>>>
>>> Although I'm not sure if that code is good to have anywhere. Why do we
>>> even have such code in the HPD work path... We already got the HPD
>>> interrupt. What does "It takes some delay (ex, 100 ~ 500 msec) to get
>>> the HPD signal with some monitors" even mean...
>>
>> The documentation for this bit is
>>
>> | HPD_STATE    0    ro    0x0    Contains the raw state of the HPD pin on the DisplayPort connector.
>>
>> So I think the idea is to perform some debouncing.
> 
> Hmm, it just looks a bit odd to me. It can sleep for a second. And the wording "It takes some delay (ex, 100 ~ 500 msec) to get the HPD signal with some monitors" makes it sound like some kind of a hack...
> 
> The docs mention debounce once:
> 
> https://docs.amd.com/r/en-US/pg299-v-dp-txss1/Hot-Plug-Detection

Are you sure this is the right document? This seems to be documentation for [1]. Is that instantiated as a hard block on the ZynqMP?

[1] https://www.xilinx.com/products/intellectual-property/ef-di-displayport.html

> But it's not immediately obvious what the SW must do and what's done by the HW. Debounce is not mentioned later, e.g. in the HPD Event Handling. But if debounce is needed, wouldn't it be perhaps in a few milliseconds, instead of hundreds of milliseconds...

Well, the DP spec says

| If the HPD is the result of a new device being connected, either
| directly to the Source device (signaled by a long HPD), –or– downstream
| of a Branch device (indicated by incrementing the DFP_COUNT field value
| in the DOWN_STREAM_PORT_COUNT register (DPCD 00007h[3:0]) and signaled
| by an IRQ_HPD pulse), the Source device shall read the new DisplayID or
| legacy EDID that has been made available to it to ensure that content
| being transmitted over the link is able to be properly received and
| rendered.
|
| Informative Note: If the HPD signal toggling (or bouncing) is the
|                   result of the Hot Unplug followed by Hot Plug of a
|                   cable-connector assembly, the HPD signal is likely
|                   to remain unstable during the de-bouncing period,
|                   which is in the order of tens of milliseconds. The
|                   Source device may either check the HPD signal’s
|                   stability before initiating an AUX read transaction,
|                   –or– immediately initiate the AUX read transaction
|                   after each HPD rising edge.

So a 100 ms delay seems plausible for some monitors.

That said, maybe we can just skip this and always read the DPCD.

> zynqmp_dp_bridge_detect() is used for drm_bridge_funcs.detect(), and if the cable is not connected, it'll sleep for 1 second (probably more) until returning not connected. It just doesn't sound correct to me.
> 
> Well, it's not part of this patch as such, but related to the amount of time we spend in the interrupt handler (and also the detect()).
> 
>>> Would it be possible to clean up the work funcs a bit (I haven't
>>> looked a the new work func yet), to remove the worst extra sleeps, and
>>> just do all that inside the threaded irq handler?
>>
>> Probably not, since a HPD IRQ results in link retraining, which can take a while.
> 
> But is it any different if you have a workqueue? Isn't a threaded interrupt handler basically the same thing?
> 
> Probably at least the zynqmp_dp_hpd_work_func() could be done in the threaded irq just fine, if the insane 1s sleep can be dropped.

Anything involving AUX shouldn't been in an IRQ, since
zynqmp_dp_aux_transfer will retry for up to 50ms by default.

>>> Do we need to handle interrupts while either delayed work is being done?
>>
>> Probably not.
>>
>>> If we do need a delayed work, would just one work be enough which
>>> handles both HPD_EVENT and HPD_IRQ, instead of two?
>>
>> Maybe, but then we need to determine which pending events we need to
>> handle. I think since we have only two events it will be easier to just
>> have separate workqueues.
> 
> The less concurrency, the better...Which is why it would be nice to do it all in the threaded irq.

Yeah, but we can use a mutex for this which means there is not too much
interesting going on.

--Sean

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 5/8] drm: zynqmp_dp: Don't retrain the link in our IRQ

[Tomi Valkeinen]

On 21/03/2024 20:01, Sean Anderson wrote:
> On 3/21/24 13:25, Tomi Valkeinen wrote:
>> On 21/03/2024 17:52, Sean Anderson wrote:
>>> On 3/20/24 02:53, Tomi Valkeinen wrote:
>>>> On 20/03/2024 00:51, Sean Anderson wrote:
>>>>> Retraining the link can take a while, and might involve waiting for
>>>>> DPCD reads/writes to complete. This is inappropriate for an IRQ handler.
>>>>> Just schedule this work for later completion. This is racy, but will be
>>>>> fixed in the next commit.
>>>>
>>>> You should add the locks first, and use them here, rather than first
>>>> adding a buggy commit and fixing it in the next one.
>>>
>>> I didn't think I could add the locks first since I only noticed the IRQ
>>> was threaded right before sending out this series. So yeah, we could add
>>> locking, add the workqueue, and then unthread the IRQ.
>>>
>>>>> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
>>>>> ---
>>>>> Actually, on second look this IRQ is threaded. So why do we have a
>>>>> workqueue for HPD events? Maybe we should make it unthreaded?
>>>>
>>>> Indeed, there's not much work being done in the IRQ handler. I don't know why it's threaded.
>>>>
>>>> We could move the queued work to be inside the threaded irq handler,
>>>> but with a quick look, the HPD work has lines like "msleep(100)" (and
>>>> that's inside a for loop...), which is probably not a good thing to do
>>>> even in threaded irq handler.
>>>>
>>>> Although I'm not sure if that code is good to have anywhere. Why do we
>>>> even have such code in the HPD work path... We already got the HPD
>>>> interrupt. What does "It takes some delay (ex, 100 ~ 500 msec) to get
>>>> the HPD signal with some monitors" even mean...
>>>
>>> The documentation for this bit is
>>>
>>> | HPD_STATE    0    ro    0x0    Contains the raw state of the HPD pin on the DisplayPort connector.
>>>
>>> So I think the idea is to perform some debouncing.
>>
>> Hmm, it just looks a bit odd to me. It can sleep for a second. And the wording "It takes some delay (ex, 100 ~ 500 msec) to get the HPD signal with some monitors" makes it sound like some kind of a hack...
>>
>> The docs mention debounce once:
>>
>> https://docs.amd.com/r/en-US/pg299-v-dp-txss1/Hot-Plug-Detection
> 
> Are you sure this is the right document? This seems to be documentation for [1]. Is that instantiated as a hard block on the ZynqMP?
> 
> [1] https://www.xilinx.com/products/intellectual-property/ef-di-displayport.html

You're right, wrong document. The registers and bitfield names I looked 
at just matched, so I didn't think it through...

The right doc says even less:

https://docs.amd.com/r/en-US/ug1085-zynq-ultrascale-trm/Upon-HPD-Assertion

>> But it's not immediately obvious what the SW must do and what's done by the HW. Debounce is not mentioned later, e.g. in the HPD Event Handling. But if debounce is needed, wouldn't it be perhaps in a few milliseconds, instead of hundreds of milliseconds...
> 
> Well, the DP spec says
> 
> | If the HPD is the result of a new device being connected, either
> | directly to the Source device (signaled by a long HPD), –or– downstream
> | of a Branch device (indicated by incrementing the DFP_COUNT field value
> | in the DOWN_STREAM_PORT_COUNT register (DPCD 00007h[3:0]) and signaled
> | by an IRQ_HPD pulse), the Source device shall read the new DisplayID or
> | legacy EDID that has been made available to it to ensure that content
> | being transmitted over the link is able to be properly received and
> | rendered.
> |
> | Informative Note: If the HPD signal toggling (or bouncing) is the
> |                   result of the Hot Unplug followed by Hot Plug of a
> |                   cable-connector assembly, the HPD signal is likely
> |                   to remain unstable during the de-bouncing period,
> |                   which is in the order of tens of milliseconds. The
> |                   Source device may either check the HPD signal’s
> |                   stability before initiating an AUX read transaction,
> |                   –or– immediately initiate the AUX read transaction
> |                   after each HPD rising edge.
> 
> So a 100 ms delay seems plausible for some monitors.

I read the text above as "it may take tens of milliseconds for HPD to 
stabilize". So polling it for total of 100ms sounds fine, but we're 
polling it for 1000ms.

And I think checking for stability is fine, but for detect() I think it 
goes overboard: if the cable is disconnected, every detect call spends a 
second checking for HPD, even if we haven't seen any sign of an HPD =).

And if we're checking the HPD stability, wouldn't we, say, poll the HPD 
for some time, and see if it stays the same? At the moment the code 
proceeds right away if HPD is high, but keeps polling if HPD is low.

> That said, maybe we can just skip this and always read the DPCD.

If the HPD is bouncing, is the AUX line also unstable?

I don't mind a HPD stability check, I think it makes sense as (I think) 
the HW doesn't handle de-bouncing here. I think think it could be much 
much shorter than what it is now, and that it would make sense to 
observe the HPD for a period, instead of just waiting for the HPD to go 
high.

But this could also be left for later, I don't think it matters in the 
context of this series.

>> zynqmp_dp_bridge_detect() is used for drm_bridge_funcs.detect(), and if the cable is not connected, it'll sleep for 1 second (probably more) until returning not connected. It just doesn't sound correct to me.
>>
>> Well, it's not part of this patch as such, but related to the amount of time we spend in the interrupt handler (and also the detect()).
>>
>>>> Would it be possible to clean up the work funcs a bit (I haven't
>>>> looked a the new work func yet), to remove the worst extra sleeps, and
>>>> just do all that inside the threaded irq handler?
>>>
>>> Probably not, since a HPD IRQ results in link retraining, which can take a while.
>>
>> But is it any different if you have a workqueue? Isn't a threaded interrupt handler basically the same thing?
>>
>> Probably at least the zynqmp_dp_hpd_work_func() could be done in the threaded irq just fine, if the insane 1s sleep can be dropped.
> 
> Anything involving AUX shouldn't been in an IRQ, since
> zynqmp_dp_aux_transfer will retry for up to 50ms by default.

Perhaps. I'm still not sure if that's a problem. If a threaded irq is 
essentially a workqueue dedicated for this device, and we don't need to 
handle other irqs while the work is being done, then... What's the 
difference with a threaded irq and a workqueue?

Oh, but we do need to handle irqs, we have the vblank irq in there. We 
don't want the vblanks to stop if there's a HPD IRQ.

Btw, looks like zynqmp_dpsub_drm_handle_vblank() can sleep, so we can't 
move to non-threaded irq.

>>>> Do we need to handle interrupts while either delayed work is being done?
>>>
>>> Probably not.
>>>
>>>> If we do need a delayed work, would just one work be enough which
>>>> handles both HPD_EVENT and HPD_IRQ, instead of two?
>>>
>>> Maybe, but then we need to determine which pending events we need to
>>> handle. I think since we have only two events it will be easier to just
>>> have separate workqueues.
>>
>> The less concurrency, the better...Which is why it would be nice to do it all in the threaded irq.
> 
> Yeah, but we can use a mutex for this which means there is not too much
> interesting going on.

Ok. Yep, if we get (hopefully) a single mutex with clearly defined 
fields that it protects, I'm ok with workqueues.

I'd still prefer just one workqueue, though...

  Tomi


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 5/8] drm: zynqmp_dp: Don't retrain the link in our IRQ

[Sean Anderson]

On 3/21/24 15:08, Tomi Valkeinen wrote:
> On 21/03/2024 20:01, Sean Anderson wrote:
>> On 3/21/24 13:25, Tomi Valkeinen wrote:
>>> On 21/03/2024 17:52, Sean Anderson wrote:
>>>> On 3/20/24 02:53, Tomi Valkeinen wrote:
>>>>> On 20/03/2024 00:51, Sean Anderson wrote:
>>>>>> Retraining the link can take a while, and might involve waiting for
>>>>>> DPCD reads/writes to complete. This is inappropriate for an IRQ handler.
>>>>>> Just schedule this work for later completion. This is racy, but will be
>>>>>> fixed in the next commit.
>>>>>
>>>>> You should add the locks first, and use them here, rather than first
>>>>> adding a buggy commit and fixing it in the next one.
>>>>
>>>> I didn't think I could add the locks first since I only noticed the IRQ
>>>> was threaded right before sending out this series. So yeah, we could add
>>>> locking, add the workqueue, and then unthread the IRQ.
>>>>
>>>>>> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
>>>>>> ---
>>>>>> Actually, on second look this IRQ is threaded. So why do we have a
>>>>>> workqueue for HPD events? Maybe we should make it unthreaded?
>>>>>
>>>>> Indeed, there's not much work being done in the IRQ handler. I don't know why it's threaded.
>>>>>
>>>>> We could move the queued work to be inside the threaded irq handler,
>>>>> but with a quick look, the HPD work has lines like "msleep(100)" (and
>>>>> that's inside a for loop...), which is probably not a good thing to do
>>>>> even in threaded irq handler.
>>>>>
>>>>> Although I'm not sure if that code is good to have anywhere. Why do we
>>>>> even have such code in the HPD work path... We already got the HPD
>>>>> interrupt. What does "It takes some delay (ex, 100 ~ 500 msec) to get
>>>>> the HPD signal with some monitors" even mean...
>>>>
>>>> The documentation for this bit is
>>>>
>>>> | HPD_STATE    0    ro    0x0    Contains the raw state of the HPD pin on the DisplayPort connector.
>>>>
>>>> So I think the idea is to perform some debouncing.
>>>
>>> Hmm, it just looks a bit odd to me. It can sleep for a second. And the wording "It takes some delay (ex, 100 ~ 500 msec) to get the HPD signal with some monitors" makes it sound like some kind of a hack...
>>>
>>> The docs mention debounce once:
>>>
>>> https://docs.amd.com/r/en-US/pg299-v-dp-txss1/Hot-Plug-Detection
>>
>> Are you sure this is the right document? This seems to be documentation for [1]. Is that instantiated as a hard block on the ZynqMP?
>>
>> [1] https://www.xilinx.com/products/intellectual-property/ef-di-displayport.html
> 
> You're right, wrong document. The registers and bitfield names I looked at just matched, so I didn't think it through...
> 
> The right doc says even less:
> 
> https://docs.amd.com/r/en-US/ug1085-zynq-ultrascale-trm/Upon-HPD-Assertion
> 
>>> But it's not immediately obvious what the SW must do and what's done by the HW. Debounce is not mentioned later, e.g. in the HPD Event Handling. But if debounce is needed, wouldn't it be perhaps in a few milliseconds, instead of hundreds of milliseconds...
>>
>> Well, the DP spec says
>>
>> | If the HPD is the result of a new device being connected, either
>> | directly to the Source device (signaled by a long HPD), –or– downstream
>> | of a Branch device (indicated by incrementing the DFP_COUNT field value
>> | in the DOWN_STREAM_PORT_COUNT register (DPCD 00007h[3:0]) and signaled
>> | by an IRQ_HPD pulse), the Source device shall read the new DisplayID or
>> | legacy EDID that has been made available to it to ensure that content
>> | being transmitted over the link is able to be properly received and
>> | rendered.
>> |
>> | Informative Note: If the HPD signal toggling (or bouncing) is the
>> |                   result of the Hot Unplug followed by Hot Plug of a
>> |                   cable-connector assembly, the HPD signal is likely
>> |                   to remain unstable during the de-bouncing period,
>> |                   which is in the order of tens of milliseconds. The
>> |                   Source device may either check the HPD signal’s
>> |                   stability before initiating an AUX read transaction,
>> |                   –or– immediately initiate the AUX read transaction
>> |                   after each HPD rising edge.
>>
>> So a 100 ms delay seems plausible for some monitors.
> 
> I read the text above as "it may take tens of milliseconds for HPD to stabilize". So polling it for total of 100ms sounds fine, but we're polling it for 1000ms.
> 
> And I think checking for stability is fine, but for detect() I think it goes overboard: if the cable is disconnected, every detect call spends a second checking for HPD, even if we haven't seen any sign of an HPD =).
> 
> And if we're checking the HPD stability, wouldn't we, say, poll the HPD for some time, and see if it stays the same? At the moment the code proceeds right away if HPD is high, but keeps polling if HPD is low.
> 
>> That said, maybe we can just skip this and always read the DPCD.
> 
> If the HPD is bouncing, is the AUX line also unstable?
> 
> I don't mind a HPD stability check, I think it makes sense as (I think) the HW doesn't handle de-bouncing here. I think think it could be much much shorter than what it is now, and that it would make sense to observe the HPD for a period, instead of just waiting for the HPD to go high.
> 
> But this could also be left for later, I don't think it matters in the context of this series.
> 
>>> zynqmp_dp_bridge_detect() is used for drm_bridge_funcs.detect(), and if the cable is not connected, it'll sleep for 1 second (probably more) until returning not connected. It just doesn't sound correct to me.
>>>
>>> Well, it's not part of this patch as such, but related to the amount of time we spend in the interrupt handler (and also the detect()).
>>>
>>>>> Would it be possible to clean up the work funcs a bit (I haven't
>>>>> looked a the new work func yet), to remove the worst extra sleeps, and
>>>>> just do all that inside the threaded irq handler?
>>>>
>>>> Probably not, since a HPD IRQ results in link retraining, which can take a while.
>>>
>>> But is it any different if you have a workqueue? Isn't a threaded interrupt handler basically the same thing?
>>>
>>> Probably at least the zynqmp_dp_hpd_work_func() could be done in the threaded irq just fine, if the insane 1s sleep can be dropped.
>>
>> Anything involving AUX shouldn't been in an IRQ, since
>> zynqmp_dp_aux_transfer will retry for up to 50ms by default.
> 
> Perhaps. I'm still not sure if that's a problem. If a threaded irq is essentially a workqueue dedicated for this device, and we don't need to handle other irqs while the work is being done, then... What's the difference with a threaded irq and a workqueue?
> 
> Oh, but we do need to handle irqs, we have the vblank irq in there. We don't want the vblanks to stop if there's a HPD IRQ.
> 
> Btw, looks like zynqmp_dpsub_drm_handle_vblank() can sleep, so we can't move to non-threaded irq.

I don't see that. We have

zynqmp_dpsub_drm_handle_vblank
  drm_crtc_handle_vblank
    drm_handle_vblank
      spin_lock_irqsave(...)
      ...
      spin_lock_irqsave(...)
      vblank_disable_fn(...)
        spin_lock_irqsave(...)
        ...
        spin_lock_irqrestore(...)

so no sleeping AFAICT.

--Sean

>>>>> Do we need to handle interrupts while either delayed work is being done?
>>>>
>>>> Probably not.
>>>>
>>>>> If we do need a delayed work, would just one work be enough which
>>>>> handles both HPD_EVENT and HPD_IRQ, instead of two?
>>>>
>>>> Maybe, but then we need to determine which pending events we need to
>>>> handle. I think since we have only two events it will be easier to just
>>>> have separate workqueues.
>>>
>>> The less concurrency, the better...Which is why it would be nice to do it all in the threaded irq.
>>
>> Yeah, but we can use a mutex for this which means there is not too much
>> interesting going on.
> 
> Ok. Yep, if we get (hopefully) a single mutex with clearly defined fields that it protects, I'm ok with workqueues.
> 
> I'd still prefer just one workqueue, though...

Yeah, but then we need a spinlock or something to tell the workqueue what it should do.

--Sean


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 5/8] drm: zynqmp_dp: Don't retrain the link in our IRQ

[Tomi Valkeinen]

On 21/03/2024 21:17, Sean Anderson wrote:
> On 3/21/24 15:08, Tomi Valkeinen wrote:
>> On 21/03/2024 20:01, Sean Anderson wrote:
>>> On 3/21/24 13:25, Tomi Valkeinen wrote:
>>>> On 21/03/2024 17:52, Sean Anderson wrote:
>>>>> On 3/20/24 02:53, Tomi Valkeinen wrote:
>>>>>> On 20/03/2024 00:51, Sean Anderson wrote:
>>>>>>> Retraining the link can take a while, and might involve waiting for
>>>>>>> DPCD reads/writes to complete. This is inappropriate for an IRQ handler.
>>>>>>> Just schedule this work for later completion. This is racy, but will be
>>>>>>> fixed in the next commit.
>>>>>>
>>>>>> You should add the locks first, and use them here, rather than first
>>>>>> adding a buggy commit and fixing it in the next one.
>>>>>
>>>>> I didn't think I could add the locks first since I only noticed the IRQ
>>>>> was threaded right before sending out this series. So yeah, we could add
>>>>> locking, add the workqueue, and then unthread the IRQ.
>>>>>
>>>>>>> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
>>>>>>> ---
>>>>>>> Actually, on second look this IRQ is threaded. So why do we have a
>>>>>>> workqueue for HPD events? Maybe we should make it unthreaded?
>>>>>>
>>>>>> Indeed, there's not much work being done in the IRQ handler. I don't know why it's threaded.
>>>>>>
>>>>>> We could move the queued work to be inside the threaded irq handler,
>>>>>> but with a quick look, the HPD work has lines like "msleep(100)" (and
>>>>>> that's inside a for loop...), which is probably not a good thing to do
>>>>>> even in threaded irq handler.
>>>>>>
>>>>>> Although I'm not sure if that code is good to have anywhere. Why do we
>>>>>> even have such code in the HPD work path... We already got the HPD
>>>>>> interrupt. What does "It takes some delay (ex, 100 ~ 500 msec) to get
>>>>>> the HPD signal with some monitors" even mean...
>>>>>
>>>>> The documentation for this bit is
>>>>>
>>>>> | HPD_STATE    0    ro    0x0    Contains the raw state of the HPD pin on the DisplayPort connector.
>>>>>
>>>>> So I think the idea is to perform some debouncing.
>>>>
>>>> Hmm, it just looks a bit odd to me. It can sleep for a second. And the wording "It takes some delay (ex, 100 ~ 500 msec) to get the HPD signal with some monitors" makes it sound like some kind of a hack...
>>>>
>>>> The docs mention debounce once:
>>>>
>>>> https://docs.amd.com/r/en-US/pg299-v-dp-txss1/Hot-Plug-Detection
>>>
>>> Are you sure this is the right document? This seems to be documentation for [1]. Is that instantiated as a hard block on the ZynqMP?
>>>
>>> [1] https://www.xilinx.com/products/intellectual-property/ef-di-displayport.html
>>
>> You're right, wrong document. The registers and bitfield names I looked at just matched, so I didn't think it through...
>>
>> The right doc says even less:
>>
>> https://docs.amd.com/r/en-US/ug1085-zynq-ultrascale-trm/Upon-HPD-Assertion
>>
>>>> But it's not immediately obvious what the SW must do and what's done by the HW. Debounce is not mentioned later, e.g. in the HPD Event Handling. But if debounce is needed, wouldn't it be perhaps in a few milliseconds, instead of hundreds of milliseconds...
>>>
>>> Well, the DP spec says
>>>
>>> | If the HPD is the result of a new device being connected, either
>>> | directly to the Source device (signaled by a long HPD), –or– downstream
>>> | of a Branch device (indicated by incrementing the DFP_COUNT field value
>>> | in the DOWN_STREAM_PORT_COUNT register (DPCD 00007h[3:0]) and signaled
>>> | by an IRQ_HPD pulse), the Source device shall read the new DisplayID or
>>> | legacy EDID that has been made available to it to ensure that content
>>> | being transmitted over the link is able to be properly received and
>>> | rendered.
>>> |
>>> | Informative Note: If the HPD signal toggling (or bouncing) is the
>>> |                   result of the Hot Unplug followed by Hot Plug of a
>>> |                   cable-connector assembly, the HPD signal is likely
>>> |                   to remain unstable during the de-bouncing period,
>>> |                   which is in the order of tens of milliseconds. The
>>> |                   Source device may either check the HPD signal’s
>>> |                   stability before initiating an AUX read transaction,
>>> |                   –or– immediately initiate the AUX read transaction
>>> |                   after each HPD rising edge.
>>>
>>> So a 100 ms delay seems plausible for some monitors.
>>
>> I read the text above as "it may take tens of milliseconds for HPD to stabilize". So polling it for total of 100ms sounds fine, but we're polling it for 1000ms.
>>
>> And I think checking for stability is fine, but for detect() I think it goes overboard: if the cable is disconnected, every detect call spends a second checking for HPD, even if we haven't seen any sign of an HPD =).
>>
>> And if we're checking the HPD stability, wouldn't we, say, poll the HPD for some time, and see if it stays the same? At the moment the code proceeds right away if HPD is high, but keeps polling if HPD is low.
>>
>>> That said, maybe we can just skip this and always read the DPCD.
>>
>> If the HPD is bouncing, is the AUX line also unstable?
>>
>> I don't mind a HPD stability check, I think it makes sense as (I think) the HW doesn't handle de-bouncing here. I think think it could be much much shorter than what it is now, and that it would make sense to observe the HPD for a period, instead of just waiting for the HPD to go high.
>>
>> But this could also be left for later, I don't think it matters in the context of this series.
>>
>>>> zynqmp_dp_bridge_detect() is used for drm_bridge_funcs.detect(), and if the cable is not connected, it'll sleep for 1 second (probably more) until returning not connected. It just doesn't sound correct to me.
>>>>
>>>> Well, it's not part of this patch as such, but related to the amount of time we spend in the interrupt handler (and also the detect()).
>>>>
>>>>>> Would it be possible to clean up the work funcs a bit (I haven't
>>>>>> looked a the new work func yet), to remove the worst extra sleeps, and
>>>>>> just do all that inside the threaded irq handler?
>>>>>
>>>>> Probably not, since a HPD IRQ results in link retraining, which can take a while.
>>>>
>>>> But is it any different if you have a workqueue? Isn't a threaded interrupt handler basically the same thing?
>>>>
>>>> Probably at least the zynqmp_dp_hpd_work_func() could be done in the threaded irq just fine, if the insane 1s sleep can be dropped.
>>>
>>> Anything involving AUX shouldn't been in an IRQ, since
>>> zynqmp_dp_aux_transfer will retry for up to 50ms by default.
>>
>> Perhaps. I'm still not sure if that's a problem. If a threaded irq is essentially a workqueue dedicated for this device, and we don't need to handle other irqs while the work is being done, then... What's the difference with a threaded irq and a workqueue?
>>
>> Oh, but we do need to handle irqs, we have the vblank irq in there. We don't want the vblanks to stop if there's a HPD IRQ.
>>
>> Btw, looks like zynqmp_dpsub_drm_handle_vblank() can sleep, so we can't move to non-threaded irq.
> 
> I don't see that. We have
> 
> zynqmp_dpsub_drm_handle_vblank
>    drm_crtc_handle_vblank
>      drm_handle_vblank
>        spin_lock_irqsave(...)
>        ...
>        spin_lock_irqsave(...)
>        vblank_disable_fn(...)
>          spin_lock_irqsave(...)
>          ...
>          spin_lock_irqrestore(...)
> 
> so no sleeping AFAICT.

Sorry, I don't know what code-path I was following where I saw mutexes. 
I shouldn't look at code so late at night...

>>>>>> Do we need to handle interrupts while either delayed work is being done?
>>>>>
>>>>> Probably not.
>>>>>
>>>>>> If we do need a delayed work, would just one work be enough which
>>>>>> handles both HPD_EVENT and HPD_IRQ, instead of two?
>>>>>
>>>>> Maybe, but then we need to determine which pending events we need to
>>>>> handle. I think since we have only two events it will be easier to just
>>>>> have separate workqueues.
>>>>
>>>> The less concurrency, the better...Which is why it would be nice to do it all in the threaded irq.
>>>
>>> Yeah, but we can use a mutex for this which means there is not too much
>>> interesting going on.
>>
>> Ok. Yep, if we get (hopefully) a single mutex with clearly defined fields that it protects, I'm ok with workqueues.
>>
>> I'd still prefer just one workqueue, though...
> 
> Yeah, but then we need a spinlock or something to tell the workqueue what it should do.

Yep. We could also always look at the HPD (if we drop the big sleeps) in 
the wq, and have a flag for the HPD IRQ, which would reduce the state to 
a single bit.

  Tomi


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 5/8] drm: zynqmp_dp: Don't retrain the link in our IRQ

[Sean Anderson]

On 3/22/24 01:32, Tomi Valkeinen wrote:
> On 21/03/2024 21:17, Sean Anderson wrote:
>> On 3/21/24 15:08, Tomi Valkeinen wrote:
>>> On 21/03/2024 20:01, Sean Anderson wrote:
>>>> On 3/21/24 13:25, Tomi Valkeinen wrote:
>>>>> On 21/03/2024 17:52, Sean Anderson wrote:
>>>>>> On 3/20/24 02:53, Tomi Valkeinen wrote:
>>>>>>> On 20/03/2024 00:51, Sean Anderson wrote:
>>>>>>> Do we need to handle interrupts while either delayed work is being done?
>>>>>>
>>>>>> Probably not.
>>>>>>
>>>>>>> If we do need a delayed work, would just one work be enough which
>>>>>>> handles both HPD_EVENT and HPD_IRQ, instead of two?
>>>>>>
>>>>>> Maybe, but then we need to determine which pending events we need to
>>>>>> handle. I think since we have only two events it will be easier to just
>>>>>> have separate workqueues.
>>>>>
>>>>> The less concurrency, the better...Which is why it would be nice to do it all in the threaded irq.
>>>>
>>>> Yeah, but we can use a mutex for this which means there is not too much
>>>> interesting going on.
>>>
>>> Ok. Yep, if we get (hopefully) a single mutex with clearly defined fields that it protects, I'm ok with workqueues.
>>>
>>> I'd still prefer just one workqueue, though...
>>
>> Yeah, but then we need a spinlock or something to tell the workqueue what it should do.
> 
> Yep. We could also always look at the HPD (if we drop the big sleeps) in the wq, and have a flag for the HPD IRQ, which would reduce the state to a single bit.

How about something like

zynqmp_dp_irq_handler(...)
{
	/* Read status and handle underflow/overflow/vblank */

	status &= ZYNQMP_DP_INT_HPD_EVENT | ZYNQMP_DP_INT_HPD_IRQ;
	if (status) {
		atomic_or(status, &dp->status);
		return IRQ_WAKE_THREAD;
	}

	return IRQ_HANDLED;
}

zynqmp_dp_thread_handler(...)
{
	status = atomic_xchg(&dp->status, 0);
	/* process HPD stuff */
}

which gets rid of the workqueue too.

--Sean

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 5/8] drm: zynqmp_dp: Don't retrain the link in our IRQ

[Tomi Valkeinen]

On 22/03/2024 18:18, Sean Anderson wrote:
> On 3/22/24 01:32, Tomi Valkeinen wrote:
>> On 21/03/2024 21:17, Sean Anderson wrote:
>>> On 3/21/24 15:08, Tomi Valkeinen wrote:
>>>> On 21/03/2024 20:01, Sean Anderson wrote:
>>>>> On 3/21/24 13:25, Tomi Valkeinen wrote:
>>>>>> On 21/03/2024 17:52, Sean Anderson wrote:
>>>>>>> On 3/20/24 02:53, Tomi Valkeinen wrote:
>>>>>>>> On 20/03/2024 00:51, Sean Anderson wrote:
>>>>>>>> Do we need to handle interrupts while either delayed work is being done?
>>>>>>>
>>>>>>> Probably not.
>>>>>>>
>>>>>>>> If we do need a delayed work, would just one work be enough which
>>>>>>>> handles both HPD_EVENT and HPD_IRQ, instead of two?
>>>>>>>
>>>>>>> Maybe, but then we need to determine which pending events we need to
>>>>>>> handle. I think since we have only two events it will be easier to just
>>>>>>> have separate workqueues.
>>>>>>
>>>>>> The less concurrency, the better...Which is why it would be nice to do it all in the threaded irq.
>>>>>
>>>>> Yeah, but we can use a mutex for this which means there is not too much
>>>>> interesting going on.
>>>>
>>>> Ok. Yep, if we get (hopefully) a single mutex with clearly defined fields that it protects, I'm ok with workqueues.
>>>>
>>>> I'd still prefer just one workqueue, though...
>>>
>>> Yeah, but then we need a spinlock or something to tell the workqueue what it should do.
>>
>> Yep. We could also always look at the HPD (if we drop the big sleeps) in the wq, and have a flag for the HPD IRQ, which would reduce the state to a single bit.
> 
> How about something like
> 
> zynqmp_dp_irq_handler(...)
> {
> 	/* Read status and handle underflow/overflow/vblank */
> 
> 	status &= ZYNQMP_DP_INT_HPD_EVENT | ZYNQMP_DP_INT_HPD_IRQ;
> 	if (status) {
> 		atomic_or(status, &dp->status);
> 		return IRQ_WAKE_THREAD;
> 	}
> 
> 	return IRQ_HANDLED;
> }
> 
> zynqmp_dp_thread_handler(...)
> {
> 	status = atomic_xchg(&dp->status, 0);
> 	/* process HPD stuff */
> }
> 
> which gets rid of the workqueue too.

I like it. We can't use IRQF_ONESHOT, as that would keep the irq masked 
while the threaded handler is being ran. I don't think that's a problem, 
but just something to keep in mind that both handlers can run concurrently.

  Tomi


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 5/8] drm: zynqmp_dp: Don't retrain the link in our IRQ

[Sean Anderson]

On 3/22/24 14:09, Tomi Valkeinen wrote:
> On 22/03/2024 18:18, Sean Anderson wrote:
>> On 3/22/24 01:32, Tomi Valkeinen wrote:
>>> On 21/03/2024 21:17, Sean Anderson wrote:
>>>> On 3/21/24 15:08, Tomi Valkeinen wrote:
>>>>> On 21/03/2024 20:01, Sean Anderson wrote:
>>>>>> On 3/21/24 13:25, Tomi Valkeinen wrote:
>>>>>>> On 21/03/2024 17:52, Sean Anderson wrote:
>>>>>>>> On 3/20/24 02:53, Tomi Valkeinen wrote:
>>>>>>>>> On 20/03/2024 00:51, Sean Anderson wrote:
>>>>>>>>> Do we need to handle interrupts while either delayed work is being done?
>>>>>>>>
>>>>>>>> Probably not.
>>>>>>>>
>>>>>>>>> If we do need a delayed work, would just one work be enough which
>>>>>>>>> handles both HPD_EVENT and HPD_IRQ, instead of two?
>>>>>>>>
>>>>>>>> Maybe, but then we need to determine which pending events we need to
>>>>>>>> handle. I think since we have only two events it will be easier to just
>>>>>>>> have separate workqueues.
>>>>>>>
>>>>>>> The less concurrency, the better...Which is why it would be nice to do it all in the threaded irq.
>>>>>>
>>>>>> Yeah, but we can use a mutex for this which means there is not too much
>>>>>> interesting going on.
>>>>>
>>>>> Ok. Yep, if we get (hopefully) a single mutex with clearly defined fields that it protects, I'm ok with workqueues.
>>>>>
>>>>> I'd still prefer just one workqueue, though...
>>>>
>>>> Yeah, but then we need a spinlock or something to tell the workqueue what it should do.
>>>
>>> Yep. We could also always look at the HPD (if we drop the big sleeps) in the wq, and have a flag for the HPD IRQ, which would reduce the state to a single bit.
>>
>> How about something like
>>
>> zynqmp_dp_irq_handler(...)
>> {
>>     /* Read status and handle underflow/overflow/vblank */
>>
>>     status &= ZYNQMP_DP_INT_HPD_EVENT | ZYNQMP_DP_INT_HPD_IRQ;
>>     if (status) {
>>         atomic_or(status, &dp->status);
>>         return IRQ_WAKE_THREAD;
>>     }
>>
>>     return IRQ_HANDLED;
>> }
>>
>> zynqmp_dp_thread_handler(...)
>> {
>>     status = atomic_xchg(&dp->status, 0);
>>     /* process HPD stuff */
>> }
>>
>> which gets rid of the workqueue too.
> 
> I like it. We can't use IRQF_ONESHOT, as that would keep the irq masked while the threaded handler is being ran. I don't think that's a problem, but just something to keep in mind that both handlers can run concurrently.

Actually, I'm not sure we can do it like this. Imagine we have something
like

CPU 0                      CPU 1
zynqmp_dp_thread_handler()
  atomic_xchg()                      
			   __handle_irq_event_percpu 
                             zynqmp_dp_irq_handler()
                               atomic_or()
                               return IRQ_WAIT_THREAD
                             __irq_wake_thread()
                               test_and_set_bit(IRQTF_RUNTHREAD, ...)
                               return
  return IRQ_HANDLED

and whoops we now have bits set in dp->status but the thread isn't
running. I don't think there's a way to fix this without locking (or two
works). TBH I am leaning towards just having two works; it is a clean
implementation. We can also convert to use work_struct instead of
delayed_work, since we never set a delay.

--Sean

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 5/8] drm: zynqmp_dp: Don't retrain the link in our IRQ

[Tomi Valkeinen]

On 22/03/2024 23:22, Sean Anderson wrote:
> On 3/22/24 14:09, Tomi Valkeinen wrote:
>> On 22/03/2024 18:18, Sean Anderson wrote:
>>> On 3/22/24 01:32, Tomi Valkeinen wrote:
>>>> On 21/03/2024 21:17, Sean Anderson wrote:
>>>>> On 3/21/24 15:08, Tomi Valkeinen wrote:
>>>>>> On 21/03/2024 20:01, Sean Anderson wrote:
>>>>>>> On 3/21/24 13:25, Tomi Valkeinen wrote:
>>>>>>>> On 21/03/2024 17:52, Sean Anderson wrote:
>>>>>>>>> On 3/20/24 02:53, Tomi Valkeinen wrote:
>>>>>>>>>> On 20/03/2024 00:51, Sean Anderson wrote:
>>>>>>>>>> Do we need to handle interrupts while either delayed work is being done?
>>>>>>>>>
>>>>>>>>> Probably not.
>>>>>>>>>
>>>>>>>>>> If we do need a delayed work, would just one work be enough which
>>>>>>>>>> handles both HPD_EVENT and HPD_IRQ, instead of two?
>>>>>>>>>
>>>>>>>>> Maybe, but then we need to determine which pending events we need to
>>>>>>>>> handle. I think since we have only two events it will be easier to just
>>>>>>>>> have separate workqueues.
>>>>>>>>
>>>>>>>> The less concurrency, the better...Which is why it would be nice to do it all in the threaded irq.
>>>>>>>
>>>>>>> Yeah, but we can use a mutex for this which means there is not too much
>>>>>>> interesting going on.
>>>>>>
>>>>>> Ok. Yep, if we get (hopefully) a single mutex with clearly defined fields that it protects, I'm ok with workqueues.
>>>>>>
>>>>>> I'd still prefer just one workqueue, though...
>>>>>
>>>>> Yeah, but then we need a spinlock or something to tell the workqueue what it should do.
>>>>
>>>> Yep. We could also always look at the HPD (if we drop the big sleeps) in the wq, and have a flag for the HPD IRQ, which would reduce the state to a single bit.
>>>
>>> How about something like
>>>
>>> zynqmp_dp_irq_handler(...)
>>> {
>>>      /* Read status and handle underflow/overflow/vblank */
>>>
>>>      status &= ZYNQMP_DP_INT_HPD_EVENT | ZYNQMP_DP_INT_HPD_IRQ;
>>>      if (status) {
>>>          atomic_or(status, &dp->status);
>>>          return IRQ_WAKE_THREAD;
>>>      }
>>>
>>>      return IRQ_HANDLED;
>>> }
>>>
>>> zynqmp_dp_thread_handler(...)
>>> {
>>>      status = atomic_xchg(&dp->status, 0);
>>>      /* process HPD stuff */
>>> }
>>>
>>> which gets rid of the workqueue too.
>>
>> I like it. We can't use IRQF_ONESHOT, as that would keep the irq masked while the threaded handler is being ran. I don't think that's a problem, but just something to keep in mind that both handlers can run concurrently.
> 
> Actually, I'm not sure we can do it like this. Imagine we have something
> like
> 
> CPU 0                      CPU 1
> zynqmp_dp_thread_handler()
>    atomic_xchg()
> 			   __handle_irq_event_percpu
>                               zynqmp_dp_irq_handler()
>                                 atomic_or()
>                                 return IRQ_WAIT_THREAD
>                               __irq_wake_thread()
>                                 test_and_set_bit(IRQTF_RUNTHREAD, ...)
>                                 return
>    return IRQ_HANDLED
> 
> and whoops we now have bits set in dp->status but the thread isn't
> running. I don't think there's a way to fix this without locking (or two

In your example above, the IRQTF_RUNTHREAD has been cleared by the 
threaded-irq before calling zynqmp_dp_thread_handler. So the hard-irq 
will set that flag before the zynqmp_dp_thread_handler() returns.

When zynqmp_dp_thread_handler() returns, the execution will go to 
irq_wait_for_interrupt(). That function will notice the IRQTF_RUNTHREAD 
flag (and clear it), and run the zynqmp_dp_thread_handler() again.

So if I'm not mistaken, when the hard-irq function returns 
IRQ_WAKE_THREAD, it's always guaranteed that a "fresh" run of the 
threaded handler will be ran.

I think that makes sense, as I'm not sure how threaded handlers without 
IRQF_ONESHOT could be used if that were not the case. I hope I'm right 
in my analysis =).

  Tomi


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 6/8] drm: zynqmp_dp: Add locking

[Sean Anderson]

Add some locking to prevent the IRQ/workers/bridge API calls from stepping
on each other's toes. This lock protects:

- Non-atomic registers configuring the link. That is, everything but the
  IRQ registers (since these are accessed in an atomic fashion), and the DP
  AUX registers (since these don't affect the link). We also access AUX
  while holding this lock, so it would be very tricky to support.
- Link configuration. This is effectively everything in zynqmp_dp which
  isn't read-only after probe time. So from next_bridge onward.

This lock is designed to protect configuration changes so we don't have to
do anything tricky. Configuration should never be in the hot path, so I'm
not worried about performance.

Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
---

Changes in v2:
- Split off the HPD IRQ work into another commit
- Expand the commit message

 drivers/gpu/drm/xlnx/zynqmp_dp.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
index f3fcdbf662fa..a90ab5c0f5cf 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
@@ -279,6 +279,7 @@ struct zynqmp_dp_config {
  * @dpsub: Display subsystem
  * @iomem: device I/O memory for register access
  * @reset: reset controller
+ * @lock: Mutex protecting this struct and register access (but not AUX)
  * @irq: irq
  * @bridge: DRM bridge for the DP encoder
  * @next_bridge: The downstream bridge
@@ -294,12 +295,17 @@ struct zynqmp_dp_config {
  * @link_config: common link configuration between IP core and sink device
  * @mode: current mode between IP core and sink device
  * @train_set: set of training data
+ *
+ * @lock covers the link configuration in this struct and the device's
+ * registers. It does not cover @aux. It is not strictly required for any of
+ * the members which are only modified at probe/remove time (e.g. @dev).
  */
 struct zynqmp_dp {
 	struct drm_dp_aux aux;
 	struct drm_bridge bridge;
 	struct delayed_work hpd_work;
 	struct delayed_work hpd_irq_work;
+	struct mutex lock;
 
 	struct drm_bridge *next_bridge;
 	struct device *dev;
@@ -1373,8 +1379,10 @@ zynqmp_dp_bridge_mode_valid(struct drm_bridge *bridge,
 	}
 
 	/* Check with link rate and lane count */
+	mutex_lock(&dp->lock);
 	rate = zynqmp_dp_max_rate(dp->link_config.max_rate,
 				  dp->link_config.max_lanes, dp->config.bpp);
+	mutex_unlock(&dp->lock);
 	if (mode->clock > rate) {
 		dev_dbg(dp->dev, "filtered mode %s for high pixel rate\n",
 			mode->name);
@@ -1401,6 +1409,7 @@ static void zynqmp_dp_bridge_atomic_enable(struct drm_bridge *bridge,
 
 	pm_runtime_get_sync(dp->dev);
 
+	mutex_lock(&dp->lock);
 	zynqmp_dp_disp_enable(dp, old_bridge_state);
 
 	/*
@@ -1461,6 +1470,7 @@ static void zynqmp_dp_bridge_atomic_enable(struct drm_bridge *bridge,
 	zynqmp_dp_write(dp, ZYNQMP_DP_SOFTWARE_RESET,
 			ZYNQMP_DP_SOFTWARE_RESET_ALL);
 	zynqmp_dp_write(dp, ZYNQMP_DP_MAIN_STREAM_ENABLE, 1);
+	mutex_unlock(&dp->lock);
 }
 
 static void zynqmp_dp_bridge_atomic_disable(struct drm_bridge *bridge,
@@ -1468,6 +1478,7 @@ static void zynqmp_dp_bridge_atomic_disable(struct drm_bridge *bridge,
 {
 	struct zynqmp_dp *dp = bridge_to_dp(bridge);
 
+	mutex_lock(&dp->lock);
 	dp->enabled = false;
 	cancel_delayed_work(&dp->hpd_work);
 	zynqmp_dp_write(dp, ZYNQMP_DP_MAIN_STREAM_ENABLE, 0);
@@ -1478,6 +1489,7 @@ static void zynqmp_dp_bridge_atomic_disable(struct drm_bridge *bridge,
 		zynqmp_dp_write(dp, ZYNQMP_DP_TX_AUDIO_CONTROL, 0);
 
 	zynqmp_dp_disp_disable(dp, old_bridge_state);
+	mutex_unlock(&dp->lock);
 
 	pm_runtime_put_sync(dp->dev);
 }
@@ -1520,6 +1532,8 @@ static enum drm_connector_status zynqmp_dp_bridge_detect(struct drm_bridge *brid
 	u32 state, i;
 	int ret;
 
+	mutex_lock(&dp->lock);
+
 	/*
 	 * This is from heuristic. It takes some delay (ex, 100 ~ 500 msec) to
 	 * get the HPD signal with some monitors.
@@ -1547,11 +1561,13 @@ static enum drm_connector_status zynqmp_dp_bridge_detect(struct drm_bridge *brid
 					       dp->num_lanes);
 
 		dp->status = connector_status_connected;
+		mutex_unlock(&dp->lock);
 		return connector_status_connected;
 	}
 
 disconnected:
 	dp->status = connector_status_disconnected;
+	mutex_unlock(&dp->lock);
 	return connector_status_disconnected;
 }
 
@@ -1620,6 +1636,7 @@ static void zynqmp_dp_hpd_irq_work_func(struct work_struct *work)
 	u8 status[DP_LINK_STATUS_SIZE + 2];
 	int err;
 
+	mutex_lock(&dp->lock);
 	err = drm_dp_dpcd_read(&dp->aux, DP_SINK_COUNT, status,
 			       DP_LINK_STATUS_SIZE + 2);
 	if (err < 0) {
@@ -1632,6 +1649,7 @@ static void zynqmp_dp_hpd_irq_work_func(struct work_struct *work)
 			zynqmp_dp_train_loop(dp);
 		}
 	}
+	mutex_unlock(&dp->lock);
 }
 
 static irqreturn_t zynqmp_dp_irq_handler(int irq, void *data)
@@ -1683,6 +1701,7 @@ int zynqmp_dp_probe(struct zynqmp_dpsub *dpsub)
 	dp->dev = &pdev->dev;
 	dp->dpsub = dpsub;
 	dp->status = connector_status_disconnected;
+	mutex_init(&dp->lock);
 
 	INIT_DELAYED_WORK(&dp->hpd_work, zynqmp_dp_hpd_work_func);
 	INIT_DELAYED_WORK(&dp->hpd_irq_work, zynqmp_dp_hpd_irq_work_func);
@@ -1793,4 +1812,5 @@ void zynqmp_dp_remove(struct zynqmp_dpsub *dpsub)
 
 	zynqmp_dp_phy_exit(dp);
 	zynqmp_dp_reset(dp, true);
+	mutex_destroy(&dp->lock);
 }
-- 
2.35.1.1320.gc452695387.dirty


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 7/8] drm: zynqmp_dp: Split off several helper functions

[Sean Anderson]

In preparation for supporting compliance testing, split off several
helper functions. No functional change intended.

Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---

(no changes since v1)

 drivers/gpu/drm/xlnx/zynqmp_dp.c | 49 ++++++++++++++++++++++----------
 1 file changed, 34 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
index a90ab5c0f5cf..6e04dcf61e19 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
@@ -633,6 +633,7 @@ static void zynqmp_dp_adjust_train(struct zynqmp_dp *dp,
 /**
  * zynqmp_dp_update_vs_emph - Update the training values
  * @dp: DisplayPort IP core structure
+ * @train_set: A set of training values
  *
  * Update the training values based on the request from sink. The mapped values
  * are predefined, and values(vs, pe, pc) are from the device manual.
@@ -640,12 +641,12 @@ static void zynqmp_dp_adjust_train(struct zynqmp_dp *dp,
  * Return: 0 if vs and emph are updated successfully, or the error code returned
  * by drm_dp_dpcd_write().
  */
-static int zynqmp_dp_update_vs_emph(struct zynqmp_dp *dp)
+static int zynqmp_dp_update_vs_emph(struct zynqmp_dp *dp, u8 *train_set)
 {
 	unsigned int i;
 	int ret;
 
-	ret = drm_dp_dpcd_write(&dp->aux, DP_TRAINING_LANE0_SET, dp->train_set,
+	ret = drm_dp_dpcd_write(&dp->aux, DP_TRAINING_LANE0_SET, train_set,
 				dp->mode.lane_cnt);
 	if (ret < 0)
 		return ret;
@@ -653,7 +654,7 @@ static int zynqmp_dp_update_vs_emph(struct zynqmp_dp *dp)
 	for (i = 0; i < dp->mode.lane_cnt; i++) {
 		u32 reg = ZYNQMP_DP_SUB_TX_PHY_PRECURSOR_LANE_0 + i * 4;
 		union phy_configure_opts opts = { 0 };
-		u8 train = dp->train_set[i];
+		u8 train = train_set[i];
 
 		opts.dp.voltage[0] = (train & DP_TRAIN_VOLTAGE_SWING_MASK)
 				   >> DP_TRAIN_VOLTAGE_SWING_SHIFT;
@@ -697,7 +698,7 @@ static int zynqmp_dp_link_train_cr(struct zynqmp_dp *dp)
 	 * So, This loop should exit before 512 iterations
 	 */
 	for (max_tries = 0; max_tries < 512; max_tries++) {
-		ret = zynqmp_dp_update_vs_emph(dp);
+		ret = zynqmp_dp_update_vs_emph(dp, dp->train_set);
 		if (ret)
 			return ret;
 
@@ -762,7 +763,7 @@ static int zynqmp_dp_link_train_ce(struct zynqmp_dp *dp)
 		return ret;
 
 	for (tries = 0; tries < DP_MAX_TRAINING_TRIES; tries++) {
-		ret = zynqmp_dp_update_vs_emph(dp);
+		ret = zynqmp_dp_update_vs_emph(dp, dp->train_set);
 		if (ret)
 			return ret;
 
@@ -785,28 +786,29 @@ static int zynqmp_dp_link_train_ce(struct zynqmp_dp *dp)
 }
 
 /**
- * zynqmp_dp_train - Train the link
+ * zynqmp_dp_setup() - Set up major link parameters
  * @dp: DisplayPort IP core structure
+ * @bw_code: The link bandwidth as a multiple of 270 MHz
+ * @lane_cnt: The number of lanes to use
+ * @enhanced: Use enhanced framing
+ * @downspread: Enable spread-spectrum clocking
  *
- * Return: 0 if all trains are done successfully, or corresponding error code.
+ * Return: 0 on success, or -errno on failure
  */
-static int zynqmp_dp_train(struct zynqmp_dp *dp)
+static int zynqmp_dp_setup(struct zynqmp_dp *dp, u8 bw_code, u8 lane_cnt,
+			   bool enhanced, bool downspread)
 {
 	u32 reg;
-	u8 bw_code = dp->mode.bw_code;
-	u8 lane_cnt = dp->mode.lane_cnt;
 	u8 aux_lane_cnt = lane_cnt;
-	bool enhanced;
 	int ret;
 
 	zynqmp_dp_write(dp, ZYNQMP_DP_LANE_COUNT_SET, lane_cnt);
-	enhanced = drm_dp_enhanced_frame_cap(dp->dpcd);
 	if (enhanced) {
 		zynqmp_dp_write(dp, ZYNQMP_DP_ENHANCED_FRAME_EN, 1);
 		aux_lane_cnt |= DP_LANE_COUNT_ENHANCED_FRAME_EN;
 	}
 
-	if (dp->dpcd[3] & 0x1) {
+	if (downspread) {
 		zynqmp_dp_write(dp, ZYNQMP_DP_DOWNSPREAD_CTL, 1);
 		drm_dp_dpcd_writeb(&dp->aux, DP_DOWNSPREAD_CTRL,
 				   DP_SPREAD_AMP_0_5);
@@ -849,8 +851,25 @@ static int zynqmp_dp_train(struct zynqmp_dp *dp)
 	}
 
 	zynqmp_dp_write(dp, ZYNQMP_DP_PHY_CLOCK_SELECT, reg);
-	ret = zynqmp_dp_phy_ready(dp);
-	if (ret < 0)
+	return zynqmp_dp_phy_ready(dp);
+}
+
+
+/**
+ * zynqmp_dp_train - Train the link
+ * @dp: DisplayPort IP core structure
+ *
+ * Return: 0 if all trains are done successfully, or corresponding error code.
+ */
+static int zynqmp_dp_train(struct zynqmp_dp *dp)
+{
+	int ret;
+
+	ret = zynqmp_dp_setup(dp, dp->mode.bw_code, dp->mode.lane_cnt,
+			      drm_dp_enhanced_frame_cap(dp->dpcd),
+			      dp->dpcd[DP_MAX_DOWNSPREAD] &
+			      DP_MAX_DOWNSPREAD_0_5);
+	if (ret)
 		return ret;
 
 	zynqmp_dp_write(dp, ZYNQMP_DP_SCRAMBLING_DISABLE, 1);
-- 
2.35.1.1320.gc452695387.dirty


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 7/8] drm: zynqmp_dp: Split off several helper functions

[Tomi Valkeinen]

On 20/03/2024 00:51, Sean Anderson wrote:
> In preparation for supporting compliance testing, split off several
> helper functions. No functional change intended.
> 
> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> 
> (no changes since v1)
> 
>   drivers/gpu/drm/xlnx/zynqmp_dp.c | 49 ++++++++++++++++++++++----------
>   1 file changed, 34 insertions(+), 15 deletions(-)
> 

Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

  Tomi


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 8/8] drm: zynqmp_dp: Add debugfs interface for compliance testing

[Sean Anderson]

Add a debugfs interface for exercising the various test modes supported
by the DisplayPort controller. This allows performing compliance
testing, or performing signal integrity measurements on a failing link.
At the moment, we do not support sink-driven link quality testing,
although such support would be fairly easy to add.

Additionally, add some debugfs files for ignoring AUX errors and HPD
events, as this can allow testing with equipment that cannot emulate a
DPRX.

Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
---

Changes in v2:
- Document debugfs files
- Add ignore_aux_errors and ignore_hpd debugfs files to replace earlier
  implicit functionality
- Attempt to fix unreproducable, spurious build warning

 Documentation/gpu/drivers.rst    |   1 +
 Documentation/gpu/zynqmp.rst     | 149 +++++++
 MAINTAINERS                      |   1 +
 drivers/gpu/drm/xlnx/zynqmp_dp.c | 675 ++++++++++++++++++++++++++++++-
 4 files changed, 823 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/gpu/zynqmp.rst

diff --git a/Documentation/gpu/drivers.rst b/Documentation/gpu/drivers.rst
index b899cbc5c2b4..187201aedbe5 100644
--- a/Documentation/gpu/drivers.rst
+++ b/Documentation/gpu/drivers.rst
@@ -22,6 +22,7 @@ GPU Driver Documentation
    afbc
    komeda-kms
    panfrost
+   zynqmp
 
 .. only::  subproject and html
 
diff --git a/Documentation/gpu/zynqmp.rst b/Documentation/gpu/zynqmp.rst
new file mode 100644
index 000000000000..4173a79972a2
--- /dev/null
+++ b/Documentation/gpu/zynqmp.rst
@@ -0,0 +1,149 @@
+.. SPDX-License-Identifier: GPL-2.0+
+
+===============================================
+Xilinx ZynqMP Ultrascale+ DisplayPort Subsystem
+===============================================
+
+This subsystem handles DisplayPort video and audio output on the ZynqMP. It
+supports in-memory framebuffers with the DisplayPort DMA controller
+(xilinx-dpdma), as well as "live" video and audio from the programmable logic
+(PL). This subsystem can perform several transformations, including color space
+conversion, alpha blending, and audio mixing, although not all features are
+currently supported.
+
+debugfs
+-------
+
+To support debugging and compliance testing, several test modes can be enabled
+though debugfs. The following files in /sys/kernel/debug/dri/X/DP-1/test/
+control the DisplayPort test modes:
+
+active:
+        Writing a 1 to this file will activate test mode, and writing a 0 will
+        deactivate test mode. Writing a 1 or 0 when the test mode is already
+        active/inactive will re-activate/re-deactivate test mode. When test
+        mode is inactive, changes made to other files will have no (immediate)
+        effect, although the settings will be saved for when test mode is
+        activated. When test mode is active, changes made to other files will
+        apply immediately.
+
+custom:
+        Custom test pattern value
+
+downspread:
+        Enable/disable clock downspreading (spread-spectrum clocking) by
+        writing 1/0
+
+enhanced:
+        Enable/disable enhanced framing
+
+ignore_aux_errors:
+        Ignore AUX errors when set to 1. Writes to this file take effect
+        immediately (regardless of whether test mode is active) and affect all
+        AUX transfers.
+
+ignore_hpd:
+        Ignore hotplug events (such as cable removals or monitor link
+        retraining requests) when set to 1. Writes to this file take effect
+        immediately (regardless of whether test mode is active).
+
+laneX_preemphasis:
+        Preemphasis from 0 (lowest) to 2 (most) for lane X
+
+laneX_swing:
+        Voltage swing from 0 (lowest) to 3 (most) for lane X
+
+lanes:
+        Number of lanes to use (1, 2, or 4)
+
+pattern:
+        Test pattern. May be one of:
+
+                video
+                        Use regular video input
+
+                symbol-error
+                        Symbol error measurement pattern
+
+                prbs7
+                        Output of the PRBS7 (x^7 + x^6 + 1) polynomial
+
+                80bit-custom
+                        A custom 80-bit pattern
+
+                cp2520
+                        HBR2 compliance eye pattern
+
+                tps1
+                        Link training symbol pattern TPS1 (/D10.2/)
+
+                tps2
+                        Link training symbol pattern TPS2
+
+                tps3
+                        Link training symbol pattern TPS3 (for HBR2)
+
+rate:
+        Rate in hertz. One of
+
+                * 5400000000 (HBR2)
+                * 2700000000 (HBR)
+                * 1620000000 (RBR)
+
+You can dump the displayport test settings with the following command::
+
+        for prop in /sys/kernel/debug/dri/1/DP-1/test/*; do
+                printf '%-17s ' ${prop##*/}
+                if [ ${prop##*/} = custom ]; then
+                        hexdump -C $prop | head -1
+                else
+                        cat $prop
+                fi
+        done
+
+The output could look something like::
+
+        active            1
+        custom            00000000  00 00 00 00 00 00 00 00  00 00                    |..........|
+        downspread        0
+        enhanced          1
+        ignore_aux_errors 1
+        ignore_hpd        1
+        lane0_preemphasis 0
+        lane0_swing       3
+        lane1_preemphasis 0
+        lane1_swing       3
+        lanes             2
+        pattern           prbs7
+        rate              1620000000
+
+The recommended test procedure is to connect the board to a monitor,
+configure test mode, activate test mode, and then disconnect the cable
+and connect it to your test equipment of choice. For example, one
+sequence of commands could be::
+
+        echo 1 > /sys/kernel/debug/dri/1/DP-1/test/enhanced
+        echo tps1 > /sys/kernel/debug/dri/1/DP-1/test/pattern
+        echo 1620000000 > /sys/kernel/debug/dri/1/DP-1/test/rate
+        echo 1 > /sys/kernel/debug/dri/1/DP-1/test/ignore_aux_errors
+        echo 1 > /sys/kernel/debug/dri/1/DP-1/test/ignore_hpd
+        echo 1 > /sys/kernel/debug/dri/1/DP-1/test/active
+
+at which point the cable could be disconnected from the monitor.
+
+Internals
+---------
+
+.. kernel-doc:: drivers/gpu/drm/xlnx/zynqmp_disp.h
+
+.. kernel-doc:: drivers/gpu/drm/xlnx/zynqmp_dpsub.h
+
+.. kernel-doc:: drivers/gpu/drm/xlnx/zynqmp_kms.h
+
+.. kernel-doc:: drivers/gpu/drm/xlnx/zynqmp_disp.c
+
+.. kernel-doc:: drivers/gpu/drm/xlnx/zynqmp_dp.c
+
+.. kernel-doc:: drivers/gpu/drm/xlnx/zynqmp_dpsub.c
+
+.. kernel-doc:: drivers/gpu/drm/xlnx/zynqmp_kms.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 8d1052fa6a69..78680180aefc 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7305,6 +7305,7 @@ L:	dri-devel@lists.freedesktop.org
 S:	Maintained
 T:	git git://anongit.freedesktop.org/drm/drm-misc
 F:	Documentation/devicetree/bindings/display/xlnx/
+F:	Documentation/gpu/zynqmp.rst
 F:	drivers/gpu/drm/xlnx/
 
 DRM GPU SCHEDULER
diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
index 6e04dcf61e19..6a84a96544e5 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
@@ -18,7 +18,9 @@
 #include <drm/drm_modes.h>
 #include <drm/drm_of.h>
 
+#include <linux/bitfield.h>
 #include <linux/clk.h>
+#include <linux/debugfs.h>
 #include <linux/delay.h>
 #include <linux/device.h>
 #include <linux/io.h>
@@ -50,6 +52,7 @@ MODULE_PARM_DESC(power_on_delay_ms, "DP power on delay in msec (default: 4)");
 #define ZYNQMP_DP_LANE_COUNT_SET			0x4
 #define ZYNQMP_DP_ENHANCED_FRAME_EN			0x8
 #define ZYNQMP_DP_TRAINING_PATTERN_SET			0xc
+#define ZYNQMP_DP_LINK_QUAL_PATTERN_SET			0x10
 #define ZYNQMP_DP_SCRAMBLING_DISABLE			0x14
 #define ZYNQMP_DP_DOWNSPREAD_CTL			0x18
 #define ZYNQMP_DP_SOFTWARE_RESET			0x1c
@@ -63,6 +66,9 @@ MODULE_PARM_DESC(power_on_delay_ms, "DP power on delay in msec (default: 4)");
 							 ZYNQMP_DP_SOFTWARE_RESET_STREAM3 | \
 							 ZYNQMP_DP_SOFTWARE_RESET_STREAM4 | \
 							 ZYNQMP_DP_SOFTWARE_RESET_AUX)
+#define ZYNQMP_DP_COMP_PATTERN_80BIT_1			0x20
+#define ZYNQMP_DP_COMP_PATTERN_80BIT_2			0x24
+#define ZYNQMP_DP_COMP_PATTERN_80BIT_3			0x28
 
 /* Core enable registers */
 #define ZYNQMP_DP_TRANSMITTER_ENABLE			0x80
@@ -206,6 +212,7 @@ MODULE_PARM_DESC(power_on_delay_ms, "DP power on delay in msec (default: 4)");
 #define ZYNQMP_DP_TX_PHY_POWER_DOWN_LANE_2		BIT(2)
 #define ZYNQMP_DP_TX_PHY_POWER_DOWN_LANE_3		BIT(3)
 #define ZYNQMP_DP_TX_PHY_POWER_DOWN_ALL			0xf
+#define ZYNQMP_DP_TRANSMIT_PRBS7			0x230
 #define ZYNQMP_DP_PHY_PRECURSOR_LANE_0			0x23c
 #define ZYNQMP_DP_PHY_PRECURSOR_LANE_1			0x240
 #define ZYNQMP_DP_PHY_PRECURSOR_LANE_2			0x244
@@ -273,6 +280,69 @@ struct zynqmp_dp_config {
 	u8 bpp;
 };
 
+/**
+ * enum test_pattern - Test patterns for test testing
+ * @TEST_VIDEO: Use regular video input
+ * @TEST_SYMBOL_ERROR: Symbol error measurement pattern
+ * @TEST_PRBS7: Output of the PRBS7 (x^7 + x^6 + 1) polynomial
+ * @TEST_80BIT_CUSTOM: A custom 80-bit pattern
+ * @TEST_CP2520: HBR2 compliance eye pattern
+ * @TEST_TPS1: Link training symbol pattern TPS1 (/D10.2/)
+ * @TEST_TPS2: Link training symbol pattern TPS2
+ * @TEST_TPS3: Link training symbol pattern TPS3 (for HBR2)
+ */
+enum test_pattern {
+	TEST_VIDEO,
+	TEST_TPS1,
+	TEST_TPS2,
+	TEST_TPS3,
+	TEST_SYMBOL_ERROR,
+	TEST_PRBS7,
+	TEST_80BIT_CUSTOM,
+	TEST_CP2520,
+};
+
+static const char *const test_pattern_str[] = {
+	[TEST_VIDEO] = "video",
+	[TEST_TPS1] = "tps1",
+	[TEST_TPS2] = "tps2",
+	[TEST_TPS3] = "tps3",
+	[TEST_SYMBOL_ERROR] = "symbol-error",
+	[TEST_PRBS7] = "prbs7",
+	[TEST_80BIT_CUSTOM] = "80bit-custom",
+	[TEST_CP2520] = "cp2520",
+};
+
+/**
+ * struct zynqmp_dp_test - Configuration for test mode
+ * @pattern: The test pattern
+ * @enhanced: Use enhanced framing
+ * @downspread: Use SSC
+ * @active: Whether test mode is active
+ * @custom: Custom pattern for %TEST_80BIT_CUSTOM
+ * @train_set: Voltage/preemphasis settings
+ * @bw_code: Bandwidth code for the link
+ * @link_cnt: Number of lanes
+ */
+struct zynqmp_dp_test {
+	enum test_pattern pattern;
+	bool enhanced, downspread, active;
+	u8 custom[10];
+	u8 train_set[ZYNQMP_DP_MAX_LANES];
+	u8 bw_code;
+	u8 link_cnt;
+};
+
+/**
+ * struct zynqmp_dp_train_set_priv - Private data for train_set debugfs files
+ * @dp: DisplayPort IP core structure
+ * @lane: The lane for this file
+ */
+struct zynqmp_dp_train_set_priv {
+	struct zynqmp_dp *dp;
+	int lane;
+};
+
 /**
  * struct zynqmp_dp - Xilinx DisplayPort core
  * @dev: device structure
@@ -283,22 +353,28 @@ struct zynqmp_dp_config {
  * @irq: irq
  * @bridge: DRM bridge for the DP encoder
  * @next_bridge: The downstream bridge
+ * @test: Configuration for test mode
  * @config: IP core configuration from DTS
  * @aux: aux channel
+ * @ignore_aux_errors: If set, AUX errors are suppressed
  * @phy: PHY handles for DP lanes
  * @num_lanes: number of enabled phy lanes
  * @hpd_work: hot plug detection worker
  * @hpd_irq_work: hot plug detection IRQ worker
+ * @ignore_hpd: If set, HPD events and IRQs are ignored
  * @status: connection status
  * @enabled: flag to indicate if the device is enabled
  * @dpcd: DP configuration data from currently connected sink device
  * @link_config: common link configuration between IP core and sink device
  * @mode: current mode between IP core and sink device
  * @train_set: set of training data
+ * @debugfs_train_set: Debugfs private data for @train_set
  *
  * @lock covers the link configuration in this struct and the device's
- * registers. It does not cover @aux. It is not strictly required for any of
- * the members which are only modified at probe/remove time (e.g. @dev).
+ * registers. It does not cover @aux or @ignore_aux_errors. It is not strictly
+ * required for any of the members which are only modified at probe/remove time
+ * (e.g. @dev). @ignore_hpd is also not covered, since it is independent of the
+ * rest of the configuration.
  */
 struct zynqmp_dp {
 	struct drm_dp_aux aux;
@@ -317,9 +393,13 @@ struct zynqmp_dp {
 	enum drm_connector_status status;
 	int irq;
 	bool enabled;
+	bool ignore_aux_errors;
+	bool ignore_hpd;
 
+	struct zynqmp_dp_train_set_priv debugfs_train_set[ZYNQMP_DP_MAX_LANES];
 	struct zynqmp_dp_mode mode;
 	struct zynqmp_dp_link_config link_config;
+	struct zynqmp_dp_test test;
 	struct zynqmp_dp_config config;
 	u8 dpcd[DP_RECEIVER_CAP_SIZE];
 	u8 train_set[ZYNQMP_DP_MAX_LANES];
@@ -1032,6 +1112,8 @@ zynqmp_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
 
 		if (dp->status == connector_status_disconnected) {
 			dev_dbg(dp->dev, "no connected aux device\n");
+			if (dp->ignore_aux_errors)
+				goto fake_response;
 			return -ENODEV;
 		}
 
@@ -1040,7 +1122,13 @@ zynqmp_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
 
 	dev_dbg(dp->dev, "failed to do aux transfer (%d)\n", ret);
 
-	return ret;
+	if (!dp->ignore_aux_errors)
+		return ret;
+
+fake_response:
+	msg->reply = DP_AUX_NATIVE_REPLY_ACK;
+	memset(msg->buffer, 0, msg->size);
+	return msg->size;
 }
 
 /**
@@ -1598,6 +1686,580 @@ static struct edid *zynqmp_dp_bridge_get_edid(struct drm_bridge *bridge,
 	return drm_get_edid(connector, &dp->aux.ddc);
 }
 
+/* -----------------------------------------------------------------------------
+ * debugfs
+ */
+
+/**
+ * zynqmp_dp_set_test_pattern() - Configure the link for a test pattern
+ * @dp: DisplayPort IP core structure
+ * @pattern: The test pattern to configure
+ * @custom: The custom pattern to use if @pattern is %TEST_80BIT_CUSTOM
+ *
+ * Return: 0 on success, or negative errno on (DPCD) failure
+ */
+static int zynqmp_dp_set_test_pattern(struct zynqmp_dp *dp,
+				      enum test_pattern pattern,
+				      u8 *const custom)
+{
+	bool scramble = false;
+	u32 train_pattern = 0;
+	u32 link_pattern = 0;
+	u8 dpcd_train = 0;
+	u8 dpcd_link = 0;
+	int ret;
+
+	switch (pattern) {
+	case TEST_TPS1:
+		train_pattern = 1;
+		break;
+	case TEST_TPS2:
+		train_pattern = 2;
+		break;
+	case TEST_TPS3:
+		train_pattern = 3;
+		break;
+	case TEST_SYMBOL_ERROR:
+		scramble = true;
+		link_pattern = DP_PHY_TEST_PATTERN_ERROR_COUNT;
+		break;
+	case TEST_PRBS7:
+		/* We use a dedicated register to enable PRBS7 */
+		dpcd_link = DP_LINK_QUAL_PATTERN_ERROR_RATE;
+		break;
+	case TEST_80BIT_CUSTOM: {
+		const u8 *p = custom;
+
+		link_pattern = DP_LINK_QUAL_PATTERN_80BIT_CUSTOM;
+
+		zynqmp_dp_write(dp, ZYNQMP_DP_COMP_PATTERN_80BIT_1,
+				(p[3] << 24) | (p[2] << 16) | (p[1] << 8) | p[0]);
+		zynqmp_dp_write(dp, ZYNQMP_DP_COMP_PATTERN_80BIT_2,
+				(p[7] << 24) | (p[6] << 16) | (p[5] << 8) | p[4]);
+		zynqmp_dp_write(dp, ZYNQMP_DP_COMP_PATTERN_80BIT_3,
+				(p[9] << 8) | p[8]);
+		break;
+	}
+	case TEST_CP2520:
+		link_pattern = DP_LINK_QUAL_PATTERN_CP2520_PAT_1;
+		break;
+	default:
+		WARN_ON_ONCE(1);
+		fallthrough;
+	case TEST_VIDEO:
+		scramble = true;
+	}
+
+	zynqmp_dp_write(dp, ZYNQMP_DP_SCRAMBLING_DISABLE, !scramble);
+	zynqmp_dp_write(dp, ZYNQMP_DP_TRAINING_PATTERN_SET, train_pattern);
+	zynqmp_dp_write(dp, ZYNQMP_DP_LINK_QUAL_PATTERN_SET, link_pattern);
+	zynqmp_dp_write(dp, ZYNQMP_DP_TRANSMIT_PRBS7, pattern == TEST_PRBS7);
+
+	dpcd_link = dpcd_link ?: link_pattern;
+	dpcd_train = train_pattern;
+	if (!scramble)
+		dpcd_train |= DP_LINK_SCRAMBLING_DISABLE;
+
+	if (dp->dpcd[DP_DPCD_REV] < 0x12) {
+		if (pattern == TEST_CP2520)
+			dev_warn(dp->dev,
+				"can't set sink link quality pattern to CP2520 for DPCD < r1.2; error counters will be invalid\n");
+		else
+			dpcd_train |= FIELD_PREP(DP_LINK_QUAL_PATTERN_11_MASK,
+						 dpcd_link);
+	} else {
+		u8 dpcd_link_lane[ZYNQMP_DP_MAX_LANES];
+
+		memset(dpcd_link_lane, dpcd_link, ZYNQMP_DP_MAX_LANES);
+		ret = drm_dp_dpcd_write(&dp->aux, DP_LINK_QUAL_LANE0_SET,
+					dpcd_link_lane, ZYNQMP_DP_MAX_LANES);
+		if (ret < 0)
+			return ret;
+	}
+
+	ret = drm_dp_dpcd_writeb(&dp->aux, DP_TRAINING_PATTERN_SET, dpcd_train);
+	return ret < 0 ? ret : 0;
+}
+
+static int zynqmp_dp_test_setup(struct zynqmp_dp *dp)
+{
+	return zynqmp_dp_setup(dp, dp->test.bw_code, dp->test.link_cnt,
+			       dp->test.enhanced, dp->test.downspread);
+}
+
+static ssize_t zynqmp_dp_pattern_read(struct file *file, char __user *user_buf,
+				      size_t count, loff_t *ppos)
+{
+	struct dentry *dentry = file->f_path.dentry;
+	struct zynqmp_dp *dp = file->private_data;
+	char buf[16];
+	ssize_t ret;
+
+	ret = debugfs_file_get(dentry);
+	if (unlikely(ret))
+		return ret;
+
+	mutex_lock(&dp->lock);
+	ret = snprintf(buf, sizeof(buf), "%s\n",
+		       test_pattern_str[dp->test.pattern]);
+	mutex_unlock(&dp->lock);
+
+	debugfs_file_put(dentry);
+	return simple_read_from_buffer(user_buf, count, ppos, buf, ret);
+}
+
+static ssize_t zynqmp_dp_pattern_write(struct file *file,
+				       const char __user *user_buf,
+				       size_t count, loff_t *ppos)
+{
+
+	struct dentry *dentry = file->f_path.dentry;
+	struct zynqmp_dp *dp = file->private_data;
+	char buf[16];
+	ssize_t ret;
+	int pattern;
+
+	ret = debugfs_file_get(dentry);
+	if (unlikely(ret))
+		return ret;
+
+	ret = simple_write_to_buffer(buf, sizeof(buf) - 1, ppos, user_buf,
+				     count);
+	if (ret < 0)
+		goto out;
+	buf[ret] = '\0';
+
+	pattern = sysfs_match_string(test_pattern_str, buf);
+	if (pattern < 0) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	mutex_lock(&dp->lock);
+	dp->test.pattern = pattern;
+	if (dp->test.active)
+		ret = zynqmp_dp_set_test_pattern(dp, dp->test.pattern,
+						 dp->test.custom) ?: ret;
+	mutex_unlock(&dp->lock);
+
+out:
+	debugfs_file_put(dentry);
+	return ret;
+}
+
+static const struct file_operations fops_zynqmp_dp_pattern = {
+	.read = zynqmp_dp_pattern_read,
+	.write = zynqmp_dp_pattern_write,
+	.open = simple_open,
+	.llseek = noop_llseek,
+};
+
+static int zynqmp_dp_enhanced_get(void *data, u64 *val)
+{
+	struct zynqmp_dp *dp = data;
+
+	mutex_lock(&dp->lock);
+	*val = dp->test.enhanced;
+	mutex_unlock(&dp->lock);
+	return 0;
+}
+
+static int zynqmp_dp_enhanced_set(void *data, u64 val)
+{
+	struct zynqmp_dp *dp = data;
+	int ret = 0;
+
+	mutex_lock(&dp->lock);
+	dp->test.enhanced = val;
+	if (dp->test.active)
+		ret = zynqmp_dp_test_setup(dp);
+	mutex_unlock(&dp->lock);
+
+	return ret;
+}
+
+DEFINE_DEBUGFS_ATTRIBUTE(fops_zynqmp_dp_enhanced, zynqmp_dp_enhanced_get,
+			 zynqmp_dp_enhanced_set, "%llu\n");
+
+static int zynqmp_dp_downspread_get(void *data, u64 *val)
+{
+	struct zynqmp_dp *dp = data;
+
+	mutex_lock(&dp->lock);
+	*val = dp->test.downspread;
+	mutex_unlock(&dp->lock);
+	return 0;
+}
+
+static int zynqmp_dp_downspread_set(void *data, u64 val)
+{
+	struct zynqmp_dp *dp = data;
+	int ret = 0;
+
+	mutex_lock(&dp->lock);
+	dp->test.downspread = val;
+	if (dp->test.active)
+		ret = zynqmp_dp_test_setup(dp);
+	mutex_unlock(&dp->lock);
+
+	return ret;
+}
+
+DEFINE_DEBUGFS_ATTRIBUTE(fops_zynqmp_dp_downspread, zynqmp_dp_downspread_get,
+			 zynqmp_dp_downspread_set, "%llu\n");
+
+static int zynqmp_dp_active_get(void *data, u64 *val)
+{
+	struct zynqmp_dp *dp = data;
+
+	mutex_lock(&dp->lock);
+	*val = dp->test.active;
+	mutex_unlock(&dp->lock);
+	return 0;
+}
+
+static int zynqmp_dp_active_set(void *data, u64 val)
+{
+	struct zynqmp_dp *dp = data;
+	int ret = 0;
+
+	mutex_lock(&dp->lock);
+	if (val) {
+		if (val < 2) {
+			ret = zynqmp_dp_test_setup(dp);
+			if (ret)
+				goto out;
+		}
+
+		ret = zynqmp_dp_set_test_pattern(dp, dp->test.pattern,
+						 dp->test.custom);
+		if (ret)
+			goto out;
+
+		ret = zynqmp_dp_update_vs_emph(dp, dp->test.train_set);
+		if (ret)
+			goto out;
+
+		dp->test.active = true;
+	} else {
+		int err;
+
+		dp->test.active = false;
+		err = zynqmp_dp_set_test_pattern(dp, TEST_VIDEO, NULL);
+		if (err)
+			dev_warn(dp->dev, "could not clear test pattern: %d\n",
+				 err);
+		zynqmp_dp_train_loop(dp);
+	}
+out:
+	mutex_unlock(&dp->lock);
+
+	return ret;
+}
+
+DEFINE_DEBUGFS_ATTRIBUTE(fops_zynqmp_dp_active, zynqmp_dp_active_get,
+			 zynqmp_dp_active_set, "%llu\n");
+
+static ssize_t zynqmp_dp_custom_read(struct file *file, char __user *user_buf,
+				      size_t count, loff_t *ppos)
+{
+	struct dentry *dentry = file->f_path.dentry;
+	struct zynqmp_dp *dp = file->private_data;
+	ssize_t ret;
+
+	ret = debugfs_file_get(dentry);
+	if (unlikely(ret))
+		return ret;
+
+	mutex_lock(&dp->lock);
+	ret = simple_read_from_buffer(user_buf, count, ppos, &dp->test.custom,
+				      sizeof(dp->test.custom));
+	mutex_unlock(&dp->lock);
+
+	debugfs_file_put(dentry);
+	return ret;
+}
+
+static ssize_t zynqmp_dp_custom_write(struct file *file,
+				      const char __user *user_buf,
+				      size_t count, loff_t *ppos)
+{
+
+	struct dentry *dentry = file->f_path.dentry;
+	struct zynqmp_dp *dp = file->private_data;
+	ssize_t ret;
+	char buf[sizeof(dp->test.custom)];
+
+	ret = debugfs_file_get(dentry);
+	if (unlikely(ret))
+		return ret;
+
+	ret = simple_write_to_buffer(buf, sizeof(buf), ppos, user_buf, count);
+	if (ret < 0)
+		goto out;
+
+	mutex_lock(&dp->lock);
+	memcpy(dp->test.custom, buf, ret);
+	if (dp->test.active)
+		ret = zynqmp_dp_set_test_pattern(dp, dp->test.pattern,
+						 dp->test.custom) ?: ret;
+	mutex_unlock(&dp->lock);
+
+out:
+	debugfs_file_put(dentry);
+	return ret;
+}
+
+static const struct file_operations fops_zynqmp_dp_custom = {
+	.read = zynqmp_dp_custom_read,
+	.write = zynqmp_dp_custom_write,
+	.open = simple_open,
+	.llseek = noop_llseek,
+};
+
+static int zynqmp_dp_swing_get(void *data, u64 *val)
+{
+	struct zynqmp_dp_train_set_priv *priv = data;
+	struct zynqmp_dp *dp = priv->dp;
+
+	mutex_lock(&dp->lock);
+	*val = dp->test.train_set[priv->lane] & DP_TRAIN_VOLTAGE_SWING_MASK;
+	mutex_unlock(&dp->lock);
+	return 0;
+}
+
+static int zynqmp_dp_swing_set(void *data, u64 val)
+{
+	struct zynqmp_dp_train_set_priv *priv = data;
+	struct zynqmp_dp *dp = priv->dp;
+	u8 *train_set = &dp->test.train_set[priv->lane];
+	int ret = 0;
+
+	if (val > 3)
+		return -EINVAL;
+
+	mutex_lock(&dp->lock);
+	*train_set &= ~(DP_TRAIN_MAX_SWING_REACHED |
+			DP_TRAIN_VOLTAGE_SWING_MASK);
+	*train_set |= val;
+	if (val == 3)
+		*train_set |= DP_TRAIN_MAX_SWING_REACHED;
+
+	if (dp->test.active)
+		ret = zynqmp_dp_update_vs_emph(dp, dp->test.train_set);
+	mutex_unlock(&dp->lock);
+
+	return ret;
+}
+
+DEFINE_DEBUGFS_ATTRIBUTE(fops_zynqmp_dp_swing, zynqmp_dp_swing_get,
+			 zynqmp_dp_swing_set, "%llu\n");
+
+static int zynqmp_dp_preemphasis_get(void *data, u64 *val)
+{
+	struct zynqmp_dp_train_set_priv *priv = data;
+	struct zynqmp_dp *dp = priv->dp;
+
+	mutex_lock(&dp->lock);
+	*val = FIELD_GET(DP_TRAIN_PRE_EMPHASIS_MASK,
+			 dp->test.train_set[priv->lane]);
+	mutex_unlock(&dp->lock);
+	return 0;
+}
+
+static int zynqmp_dp_preemphasis_set(void *data, u64 val)
+{
+	struct zynqmp_dp_train_set_priv *priv = data;
+	struct zynqmp_dp *dp = priv->dp;
+	u8 *train_set = &dp->test.train_set[priv->lane];
+	int ret = 0;
+
+	if (val > 2)
+		return -EINVAL;
+
+	mutex_lock(&dp->lock);
+	*train_set &= ~(DP_TRAIN_MAX_PRE_EMPHASIS_REACHED |
+			DP_TRAIN_PRE_EMPHASIS_MASK);
+	*train_set |= val;
+	if (val == 2)
+		*train_set |= DP_TRAIN_MAX_PRE_EMPHASIS_REACHED;
+
+	if (dp->test.active)
+		ret = zynqmp_dp_update_vs_emph(dp, dp->test.train_set);
+	mutex_unlock(&dp->lock);
+
+	return ret;
+}
+
+DEFINE_DEBUGFS_ATTRIBUTE(fops_zynqmp_dp_preemphasis, zynqmp_dp_preemphasis_get,
+			 zynqmp_dp_preemphasis_set, "%llu\n");
+
+static int zynqmp_dp_lanes_get(void *data, u64 *val)
+{
+	struct zynqmp_dp *dp = data;
+
+	mutex_lock(&dp->lock);
+	*val = dp->test.link_cnt;
+	mutex_unlock(&dp->lock);
+	return 0;
+}
+
+static int zynqmp_dp_lanes_set(void *data, u64 val)
+{
+	struct zynqmp_dp *dp = data;
+	int ret = 0;
+
+	if (val > ZYNQMP_DP_MAX_LANES)
+		return -EINVAL;
+
+	mutex_lock(&dp->lock);
+	if (val > dp->num_lanes) {
+		ret = -EINVAL;
+	} else {
+		dp->test.link_cnt = val;
+		if (dp->test.active)
+			ret = zynqmp_dp_test_setup(dp);
+	}
+	mutex_unlock(&dp->lock);
+
+	return ret;
+}
+
+DEFINE_DEBUGFS_ATTRIBUTE(fops_zynqmp_dp_lanes, zynqmp_dp_lanes_get,
+			 zynqmp_dp_lanes_set, "%llu\n");
+
+static int zynqmp_dp_rate_get(void *data, u64 *val)
+{
+	struct zynqmp_dp *dp = data;
+
+	mutex_lock(&dp->lock);
+	*val = drm_dp_bw_code_to_link_rate(dp->test.bw_code) * 10000;
+	mutex_unlock(&dp->lock);
+	return 0;
+}
+
+static int zynqmp_dp_rate_set(void *data, u64 val)
+{
+	struct zynqmp_dp *dp = data;
+	int link_rate;
+	int ret = 0;
+	u8 bw_code;
+
+	if (do_div(val, 10000))
+		return -EINVAL;
+
+	bw_code = drm_dp_link_rate_to_bw_code(val);
+	link_rate = drm_dp_bw_code_to_link_rate(bw_code);
+	if (val != link_rate)
+		return -EINVAL;
+
+	if (bw_code != DP_LINK_BW_1_62 && bw_code != DP_LINK_BW_2_7 &&
+	    bw_code != DP_LINK_BW_5_4)
+		return -EINVAL;
+
+	mutex_lock(&dp->lock);
+	dp->test.bw_code = bw_code;
+	if (dp->test.active)
+		ret = zynqmp_dp_test_setup(dp);
+	mutex_unlock(&dp->lock);
+
+	return ret;
+}
+
+DEFINE_DEBUGFS_ATTRIBUTE(fops_zynqmp_dp_rate, zynqmp_dp_rate_get,
+			 zynqmp_dp_rate_set, "%llu\n");
+
+static int zynqmp_dp_ignore_aux_errors_get(void *data, u64 *val)
+{
+	struct zynqmp_dp *dp = data;
+
+	mutex_lock(&dp->aux.hw_mutex);
+	*val = dp->ignore_aux_errors;
+	mutex_unlock(&dp->aux.hw_mutex);
+	return 0;
+}
+
+static int zynqmp_dp_ignore_aux_errors_set(void *data, u64 val)
+{
+	struct zynqmp_dp *dp = data;
+
+	if (val != !!val)
+		return -EINVAL;
+
+	mutex_lock(&dp->aux.hw_mutex);
+	dp->ignore_aux_errors = val;
+	mutex_unlock(&dp->aux.hw_mutex);
+	return 0;
+}
+
+DEFINE_DEBUGFS_ATTRIBUTE(fops_zynqmp_dp_ignore_aux_errors,
+			 zynqmp_dp_ignore_aux_errors_get,
+			 zynqmp_dp_ignore_aux_errors_set, "%llu\n");
+
+static int zynqmp_dp_ignore_hpd_get(void *data, u64 *val)
+{
+	struct zynqmp_dp *dp = data;
+
+	*val = READ_ONCE(dp->ignore_hpd);
+	return 0;
+}
+
+static int zynqmp_dp_ignore_hpd_set(void *data, u64 val)
+{
+	struct zynqmp_dp *dp = data;
+
+	if (val != !!val)
+		return -EINVAL;
+
+	WRITE_ONCE(dp->ignore_hpd, val);
+	return 0;
+}
+
+DEFINE_DEBUGFS_ATTRIBUTE(fops_zynqmp_dp_ignore_hpd, zynqmp_dp_ignore_hpd_get,
+			 zynqmp_dp_ignore_hpd_set, "%llu\n");
+
+static void zynqmp_dp_bridge_debugfs_init(struct drm_bridge *bridge,
+					  struct dentry *root)
+{
+	struct zynqmp_dp *dp = bridge_to_dp(bridge);
+	struct dentry *test;
+	int i;
+
+	dp->test.bw_code = DP_LINK_BW_5_4;
+	dp->test.link_cnt = dp->num_lanes;
+
+	test = debugfs_create_dir("test", root);
+#define CREATE_FILE(name) \
+	debugfs_create_file(#name, 0600, test, dp, &fops_zynqmp_dp_##name)
+	CREATE_FILE(pattern);
+	CREATE_FILE(enhanced);
+	CREATE_FILE(downspread);
+	CREATE_FILE(active);
+	CREATE_FILE(custom);
+	CREATE_FILE(rate);
+	CREATE_FILE(lanes);
+	CREATE_FILE(ignore_aux_errors);
+	CREATE_FILE(ignore_hpd);
+
+	for (i = 0; i < dp->num_lanes; i++) {
+		static const char fmt[] = "lane%d_preemphasis";
+		char name[sizeof(fmt)];
+
+		dp->debugfs_train_set[i].dp = dp;
+		dp->debugfs_train_set[i].lane = i;
+
+		snprintf(name, sizeof(name), fmt, i);
+		debugfs_create_file(name, 0600, test,
+				    &dp->debugfs_train_set[i],
+				    &fops_zynqmp_dp_preemphasis);
+
+		snprintf(name, sizeof(name), "lane%d_swing", i);
+		debugfs_create_file(name, 0600, test,
+				    &dp->debugfs_train_set[i],
+				    &fops_zynqmp_dp_swing);
+	}
+}
+
 static const struct drm_bridge_funcs zynqmp_dp_bridge_funcs = {
 	.attach = zynqmp_dp_bridge_attach,
 	.detach = zynqmp_dp_bridge_detach,
@@ -1610,6 +2272,7 @@ static const struct drm_bridge_funcs zynqmp_dp_bridge_funcs = {
 	.atomic_check = zynqmp_dp_bridge_atomic_check,
 	.detect = zynqmp_dp_bridge_detect,
 	.get_edid = zynqmp_dp_bridge_get_edid,
+	.debugfs_init = zynqmp_dp_bridge_debugfs_init,
 };
 
 /* -----------------------------------------------------------------------------
@@ -1644,6 +2307,9 @@ static void zynqmp_dp_hpd_work_func(struct work_struct *work)
 					    hpd_work.work);
 	enum drm_connector_status status;
 
+	if (READ_ONCE(dp->ignore_hpd))
+		return;
+
 	status = zynqmp_dp_bridge_detect(&dp->bridge);
 	drm_bridge_hpd_notify(&dp->bridge, status);
 }
@@ -1655,6 +2321,9 @@ static void zynqmp_dp_hpd_irq_work_func(struct work_struct *work)
 	u8 status[DP_LINK_STATUS_SIZE + 2];
 	int err;
 
+	if (READ_ONCE(dp->ignore_hpd))
+		return;
+
 	mutex_lock(&dp->lock);
 	err = drm_dp_dpcd_read(&dp->aux, DP_SINK_COUNT, status,
 			       DP_LINK_STATUS_SIZE + 2);
-- 
2.35.1.1320.gc452695387.dirty


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 8/8] drm: zynqmp_dp: Add debugfs interface for compliance testing

[Tomi Valkeinen]

On 20/03/2024 00:51, Sean Anderson wrote:

> +/**
> + * enum test_pattern - Test patterns for test testing

"for test testing"? =)

> @@ -1655,6 +2321,9 @@ static void zynqmp_dp_hpd_irq_work_func(struct work_struct *work)
>   	u8 status[DP_LINK_STATUS_SIZE + 2];
>   	int err;
>   
> +	if (READ_ONCE(dp->ignore_hpd))
> +		return;
> +
>   	mutex_lock(&dp->lock);
>   	err = drm_dp_dpcd_read(&dp->aux, DP_SINK_COUNT, status,
>   			       DP_LINK_STATUS_SIZE + 2);

Why do you need READ/WRITE_ONCE() for ignore_hpd?

  Tomi


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 8/8] drm: zynqmp_dp: Add debugfs interface for compliance testing

[Sean Anderson]

On 3/20/24 03:49, Tomi Valkeinen wrote:
> On 20/03/2024 00:51, Sean Anderson wrote:
> 
>> +/**
>> + * enum test_pattern - Test patterns for test testing
> 
> "for test testing"? =)
> 
>> @@ -1655,6 +2321,9 @@ static void zynqmp_dp_hpd_irq_work_func(struct work_struct *work)
>>       u8 status[DP_LINK_STATUS_SIZE + 2];
>>       int err;
>>   +    if (READ_ONCE(dp->ignore_hpd))
>> +        return;
>> +
>>       mutex_lock(&dp->lock);
>>       err = drm_dp_dpcd_read(&dp->aux, DP_SINK_COUNT, status,
>>                      DP_LINK_STATUS_SIZE + 2);
> 
> Why do you need READ/WRITE_ONCE() for ignore_hpd?

It's not protected by dp->lock so we don't have to take it for
zynqmp_dp_hpd_work_func. Although maybe we should make a version of
zynqmp_dp_bridge_detect which assumes we already hold the lock.

--Sean

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 8/8] drm: zynqmp_dp: Add debugfs interface for compliance testing

[Tomi Valkeinen]

On 21/03/2024 18:08, Sean Anderson wrote:
> On 3/20/24 03:49, Tomi Valkeinen wrote:
>> On 20/03/2024 00:51, Sean Anderson wrote:
>>
>>> +/**
>>> + * enum test_pattern - Test patterns for test testing
>>
>> "for test testing"? =)
>>
>>> @@ -1655,6 +2321,9 @@ static void zynqmp_dp_hpd_irq_work_func(struct work_struct *work)
>>>        u8 status[DP_LINK_STATUS_SIZE + 2];
>>>        int err;
>>>    +    if (READ_ONCE(dp->ignore_hpd))
>>> +        return;
>>> +
>>>        mutex_lock(&dp->lock);
>>>        err = drm_dp_dpcd_read(&dp->aux, DP_SINK_COUNT, status,
>>>                       DP_LINK_STATUS_SIZE + 2);
>>
>> Why do you need READ/WRITE_ONCE() for ignore_hpd?
> 
> It's not protected by dp->lock so we don't have to take it for
> zynqmp_dp_hpd_work_func. Although maybe we should make a version of
> zynqmp_dp_bridge_detect which assumes we already hold the lock.

Does using the macros solve some potential issue, or is it just for 
documenting that this variable is accessed without lock?

  Tomi


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 8/8] drm: zynqmp_dp: Add debugfs interface for compliance testing

[Sean Anderson]

On 3/21/24 12:31, Tomi Valkeinen wrote:
> On 21/03/2024 18:08, Sean Anderson wrote:
>> On 3/20/24 03:49, Tomi Valkeinen wrote:
>>> On 20/03/2024 00:51, Sean Anderson wrote:
>>>
>>>> +/**
>>>> + * enum test_pattern - Test patterns for test testing
>>>
>>> "for test testing"? =)
>>>
>>>> @@ -1655,6 +2321,9 @@ static void zynqmp_dp_hpd_irq_work_func(struct work_struct *work)
>>>>        u8 status[DP_LINK_STATUS_SIZE + 2];
>>>>        int err;
>>>>    +    if (READ_ONCE(dp->ignore_hpd))
>>>> +        return;
>>>> +
>>>>        mutex_lock(&dp->lock);
>>>>        err = drm_dp_dpcd_read(&dp->aux, DP_SINK_COUNT, status,
>>>>                       DP_LINK_STATUS_SIZE + 2);
>>>
>>> Why do you need READ/WRITE_ONCE() for ignore_hpd?
>>
>> It's not protected by dp->lock so we don't have to take it for
>> zynqmp_dp_hpd_work_func. Although maybe we should make a version of
>> zynqmp_dp_bridge_detect which assumes we already hold the lock.
> 
> Does using the macros solve some potential issue, or is it just for documenting that this variable is accessed without lock?

Without this the compiler is free to issue multiple loads for this
variable, which could be incorrect.

--Sean


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 0/8] drm: zynqmp_dp: Misc. patches and debugfs support

[Sean Anderson]

On 3/19/24 18:51, Sean Anderson wrote:
> This series adds debugfs support for the zynqmp_dp driver. The intent is
> to enable compliance testing or to help debug signal-integrity issues.
> 
> The first four patches are general improvements (and could be applied
> independently), while the last four add debugfs support.
> 
> Changes in v2:
> - Fix kerneldoc
> - Rearrange zynqmp_dp for better padding
> - Document hpd_irq_work
> - Split off the HPD IRQ work into another commit
> - Expand the commit message
> - 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: xlnx: Fix kerneldoc
>   drm: zynqmp_dp: Downgrade log level for aux retries message
>   drm: zynqmp_dp: Adjust training values per-lane
>   drm: zynqmp_dp: Rearrange zynqmp_dp for better padding
>   drm: zynqmp_dp: Don't retrain the link in our IRQ
>   drm: zynqmp_dp: Add locking
>   drm: zynqmp_dp: Split off several helper functions
>   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_disp.c  |   6 +-
>  drivers/gpu/drm/xlnx/zynqmp_dp.c    | 836 +++++++++++++++++++++++++---
>  drivers/gpu/drm/xlnx/zynqmp_dpsub.h |   1 +
>  drivers/gpu/drm/xlnx/zynqmp_kms.h   |   4 +-
>  7 files changed, 931 insertions(+), 67 deletions(-)
>  create mode 100644 Documentation/gpu/zynqmp.rst
> 

+CC Dmitry, Tomi, and Anatoliy who I forgot to CC

Let me know if you want to be taken off CC for future revisions

--Sean

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel