Re: [PATCH RFC 2/2] pmdomain: ti-sci: Support retaining PD boot time state
Hi, On 03/05/2024 16:45, Ulf Hansson wrote: + Abel, Saravanna, Stephen On Mon, 15 Apr 2024 at 19:17, Tomi Valkeinen wrote: On 15/04/2024 19:00, Tomi Valkeinen wrote: Add a new flag, TI_SCI_PD_KEEP_BOOT_STATE, which can be set in the dts when referring to power domains. When this flag is set, the ti-sci driver will check if the PD is currently enabled in the HW, and if so, set the GENPD_FLAG_ALWAYS_ON flag so that the PD will stay enabled. The main issue I'm trying to solve here is this: If the Display Subsystem (DSS) has been enabled by the bootloader, the related PD has also been enabled in the HW. When the tidss driver probes, the driver framework will automatically enable the PD. While executing the probe function it is very common for the probe to return EPROBE_DEFER, and, in rarer cases, an actual error. When this happens (probe() returns an error), the driver framework will automatically disable the related PD. Powering off the PD while the DSS is enabled and displaying a picture will cause the DSS HW to enter a bad state, from which (afaik) it can't be woken up except with full power-cycle. Trying to access the DSS in this state (e.g. when retrying the probe) will usually cause the board to hang sooner or later. Even if we wouldn't have this board-hangs issue, it's nice to be able to keep the DSS PD enabled: we want to keep the DSS enabled when the bootloader has enabled the screen. If, instead, we disable the PD at the first EPROBE_DEFER, the screen will (probably) go black. A few things occurred to me. The driver is supposed to clear the GENPD_FLAG_ALWAYS_ON when the driver has probed successfully. There are two possible issues with that: - Afaics, there's no API to do that, and currently I just clear the bit in genpd->flags. There's a clear race there, so some locking would be required. - This uses the GENPD_FLAG_ALWAYS_ON flag to say "PD is always on, until the driver has started". If the PD would have GENPD_FLAG_ALWAYS_ON set for other reasons, the driver would still go and clear the flag, which might break things. Also, unrelated to the above and not a problem in practice at the very moment, but I think clocks should also be dealt with somehow. Something, at early-ish boot stage, should mark the relevant clocks as in use, so that there's no chance they would be turned off when the main kernel has started (the main display driver is often a module). It would be nice to deal with all the above in a single place. I wonder if the tidss driver itself could somehow be split into two parts, an early part that would probe with minimal dependencies, mainly to reserve the core resources without doing any kind of DRM init. And a main part which would (somehow) finish the initialization at a later point, when we have the filesystem (for firmware) and the other bridge/panel drivers have probed. That can be somewhat achieved with simplefb or simpledrm, though, but we can't do any TI DSS specific things there, and it also creates a requirement to have either of those drivers built-in, and the related DT nodes to be added. Without going into too much detail, this and similar problems have been discussed in the past. With the fw_devlink and the ->sync_state() callback we are getting closer to a solution, but for genpd a solution is still pending. If you want to read up on earlier discussions and join us moving forward, that would be great. The last attempt for genpd to move this forward was posted by Abel Vesa: https://lore.kernel.org/linux-pm/20230621144019.3219858-1-abel.v...@linaro.org/ Beyond that, we have also discussed various solutions at the last LPC in Richmond. I think the consensus at that point was that Saravana targeted to post something for clocks - and when that was done, we should do the similar thing for genpd. Anyway, I have looped them into this thread, so they can share any updates on their side of the matter. If I understand the series correctly, it has an issue at least for this case/platform. The devlinks are between the consumer devices and the PD provider device. TI SCI PD provider has quite a lot of PDs, and all the consumers would have to be probed before any of the PDs could be disabled. So, to get the display PD disabled, I would have to load, e.g., the GPU driver (which I don't even have). I believe this is the case for the clocks also. Perhaps that can be considered a feature, but I fear that in practice it would mean that most of the time for most users all the boot-time enabled powerdomains would be always on. Nevertheless, I believe the series would fix the issue mentioned in this patch, so I'll see if I can get the series working on the TI platform to get a bit more experience on this whole issue. Tomi Tomi Kind regards Uffe Another option here would perhaps be to change the driver framework (drivers/base/platform.c) which attaches and detaches the PD, and make it somehow optional, allowing the d
Re: [BUG] drm: zynqmp_dp: Lockup in zynqmp_dp_bridge_detect when device is unbound
Hi,
On 04/05/2024 00:54, Sean Anderson wrote:
Hi,
I have discovered a bug in the displayport driver on drm-misc-next. To
trigger it, run
echo fd4a.display > /sys/bus/platform/drivers/zynqmp-dpsub/unbind
The system will become unresponsive and (after a bit) splat with a hard
LOCKUP. One core will be unresponsive at the first zynqmp_dp_read in
zynqmp_dp_bridge_detect.
I believe the issue is due the registers being unmapped and the block
put into reset in zynqmp_dp_remove instead of zynqmp_dpsub_release. This
could be resolved by deferring things until zynqmp_dpsub_release
(requiring us to skip devm_*), or by adding a flag to struct zynqmp_dp
and checking it before each callback. A subsystem-level implementation
might be better for the latter.
For a better traceback, try applying the below patch and running the
following commands before triggering the lockup:
echo 4 > /sys/module/drm/parameters/debug
echo 8 > /proc/sys/kernel/printk
I wasn't able to reproduce. Where does the detect call come in your
case? Looking at the code, afaics, it shouldn't happen.
Tomi
diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
index 9df068a413f3..17b477b14ab5 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
@@ -296,6 +296,7 @@ struct zynqmp_dp_config {
* @train_set: set of training data
*/
struct zynqmp_dp {
+ unsigned long long magic;
struct device *dev;
struct zynqmp_dpsub *dpsub;
void __iomem *iomem;
@@ -1533,6 +1534,8 @@ static enum drm_connector_status
zynqmp_dp_bridge_detect(struct drm_bridge *brid
u32 state, i;
int ret;
+ WARN_ON(dp->magic != 0x0123456789abcdefULL);
+
/*
* This is from heuristic. It takes some delay (ex, 100 ~ 500 msec) to
* get the HPD signal with some monitors.
@@ -1723,6 +1726,7 @@ int zynqmp_dp_probe(struct zynqmp_dpsub *dpsub)
if (!dp)
return -ENOMEM;
+ dp->magic = 0x0123456789abcdefULL;
dp->dev = >dev;
dp->dpsub = dpsub;
dp->status = connector_status_disconnected;
@@ -1839,4 +1843,5 @@ void zynqmp_dp_remove(struct zynqmp_dpsub *dpsub)
zynqmp_dp_phy_exit(dp);
zynqmp_dp_reset(dp, true);
+ dp->magic = 0xdeadbeefdeadbeefULL;
}
Re: [PATCH v3 0/2] Fix Kernel CI issues
On 03/05/2024 19:27, Nathan Chancellor wrote:
Hi Tomi,
On Sat, Apr 27, 2024 at 10:48:16AM +0300, Tomi Valkeinen wrote:
On 26/04/2024 22:27, Anatoliy Klymenko wrote:
Fix number of CI reported W=1 build issues.
Patch 1/2: Fix function arguments description.
Closes:
https://lore.kernel.org/oe-kbuild-all/202404260616.kfgdpcdn-...@intel.com/
Patch 2/2: Fix clang compilation error.
Closes:
https://lore.kernel.org/oe-kbuild-all/202404260946.4ozxvhd2-...@intel.com/
Signed-off-by: Anatoliy Klymenko
---
Changes in v3:
- Add Signed-off-by tag.
- Link to v2:
https://lore.kernel.org/r/20240425-dp-live-fmt-fix-v2-0-6048e8121...@amd.com
Changes in v2:
- Compilation error fix added.
- Link to v1:
https://lore.kernel.org/r/20240425-dp-live-fmt-fix-v1-1-405f352d3...@amd.com
---
Anatoliy Klymenko (2):
drm: xlnx: zynqmp_dpsub: Fix few function comments
drm: xlnx: zynqmp_dpsub: Fix compilation error
drivers/gpu/drm/xlnx/zynqmp_disp.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
---
base-commit: 2bdb481bf7a93c22b9fea8daefa2834aab23a70f
change-id: 20240425-dp-live-fmt-fix-a10bf7973596
Best regards,
Thanks, pushed to drm-misc-next.
I think the second patch also needs to go to drm-misc-next-fixes? The
clang warning fixed by it has returned in next-20240503 because it
appears that for-linux-next was switch from drm-misc-next to
drm-misc-next-fixes, as I see for-linux-next was pointing to commit
235e60653f8d ("drm/debugfs: Drop conditionals around of_node pointers")
on drm-misc-next in next-20240502 but it is now pointing to commit
be3f3042391d ("drm: zynqmp_dpsub: Always register bridge") on
drm-misc-next-fixes in next-20240503.
Oh. Hmm, did I just hit the feature-freeze point with the fixes...
Now I'm unsure where I should push these (if anywhere), as they already
are in drm-misc-next.
DRM Misc maintainers, can you give me a hint? =)
Tomi
Re: [PATCH v3 0/2] Fix Kernel CI issues
On 26/04/2024 22:27, Anatoliy Klymenko wrote: Fix number of CI reported W=1 build issues. Patch 1/2: Fix function arguments description. Closes: https://lore.kernel.org/oe-kbuild-all/202404260616.kfgdpcdn-...@intel.com/ Patch 2/2: Fix clang compilation error. Closes: https://lore.kernel.org/oe-kbuild-all/202404260946.4ozxvhd2-...@intel.com/ Signed-off-by: Anatoliy Klymenko --- Changes in v3: - Add Signed-off-by tag. - Link to v2: https://lore.kernel.org/r/20240425-dp-live-fmt-fix-v2-0-6048e8121...@amd.com Changes in v2: - Compilation error fix added. - Link to v1: https://lore.kernel.org/r/20240425-dp-live-fmt-fix-v1-1-405f352d3...@amd.com --- Anatoliy Klymenko (2): drm: xlnx: zynqmp_dpsub: Fix few function comments drm: xlnx: zynqmp_dpsub: Fix compilation error drivers/gpu/drm/xlnx/zynqmp_disp.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) --- base-commit: 2bdb481bf7a93c22b9fea8daefa2834aab23a70f change-id: 20240425-dp-live-fmt-fix-a10bf7973596 Best regards, Thanks, pushed to drm-misc-next. Tomi
Re: [PATCH] drm: zynqmp_dpsub: Always register bridge
On 26/04/2024 12:30, Laurent Pinchart wrote:
On Fri, Mar 22, 2024 at 08:01:44AM +0200, Tomi Valkeinen wrote:
On 08/03/2024 22:47, Sean Anderson wrote:
We must always register the DRM bridge, since zynqmp_dp_hpd_work_func
calls drm_bridge_hpd_notify, which in turn expects hpd_mutex to be
initialized. We do this before zynqmp_dpsub_drm_init since that calls
drm_bridge_attach. This fixes the following lockdep warning:
[ 19.217084] [ cut here ]
[ 19.227530] DEBUG_LOCKS_WARN_ON(lock->magic != lock)
[ 19.227768] WARNING: CPU: 0 PID: 140 at kernel/locking/mutex.c:582
__mutex_lock+0x4bc/0x550
[ 19.241696] Modules linked in:
[ 19.244937] CPU: 0 PID: 140 Comm: kworker/0:4 Not tainted 6.6.20+ #96
[ 19.252046] Hardware name: xlnx,zynqmp (DT)
[ 19.256421] Workqueue: events zynqmp_dp_hpd_work_func
[ 19.261795] pstate: 6005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[ 19.269104] pc : __mutex_lock+0x4bc/0x550
[ 19.273364] lr : __mutex_lock+0x4bc/0x550
[ 19.277592] sp : ffc085c5bbe0
[ 19.281066] x29: ffc085c5bbe0 x28: x27: ff88009417f8
[ 19.288624] x26: ff8800941788 x25: ff8800020008 x24: ffc082aa3000
[ 19.296227] x23: ffc080d90e3c x22: 0002 x21:
[ 19.303744] x20: x19: ff88002f5210 x18:
[ 19.311295] x17: 6c707369642e3030 x16: 3030613464662072 x15: 0720072007200720
[ 19.318922] x14: x13: 284e4f5f4e524157 x12: 0001
[ 19.326442] x11: 0001ffc085c5b940 x10: 0001ff88003f388b x9 : 0001ff88003f3888
[ 19.334003] x8 : 0001ff88003f3888 x7 : x6 :
[ 19.341537] x5 : x4 : 1668 x3 :
[ 19.349054] x2 : x1 : x0 : ff88003f3880
[ 19.356581] Call trace:
[ 19.359160] __mutex_lock+0x4bc/0x550
[ 19.363032] mutex_lock_nested+0x24/0x30
[ 19.367187] drm_bridge_hpd_notify+0x2c/0x6c
[ 19.371698] zynqmp_dp_hpd_work_func+0x44/0x54
[ 19.376364] process_one_work+0x3ac/0x988
[ 19.380660] worker_thread+0x398/0x694
[ 19.384736] kthread+0x1bc/0x1c0
[ 19.388241] ret_from_fork+0x10/0x20
[ 19.392031] irq event stamp: 183
[ 19.395450] hardirqs last enabled at (183): []
finish_task_switch.isra.0+0xa8/0x2d4
[ 19.405140] hardirqs last disabled at (182): []
__schedule+0x714/0xd04
[ 19.413612] softirqs last enabled at (114): []
srcu_invoke_callbacks+0x158/0x23c
[ 19.423128] softirqs last disabled at (110): []
srcu_invoke_callbacks+0x158/0x23c
[ 19.432614] ---[ end trace ]---
Fixes: eb2d64bfcc17 ("drm: xlnx: zynqmp_dpsub: Report HPD through the bridge")
Signed-off-by: Sean Anderson
---
drivers/gpu/drm/xlnx/zynqmp_dpsub.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
b/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
index 88eb33acd5f0..639fff2c693f 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
@@ -256,12 +256,11 @@ static int zynqmp_dpsub_probe(struct platform_device
*pdev)
if (ret)
goto err_dp;
+ drm_bridge_add(dpsub->bridge);
A blank line here would be nice.
if (dpsub->dma_enabled) {
ret = zynqmp_dpsub_drm_init(dpsub);
if (ret)
goto err_disp;
- } else {
- drm_bridge_add(dpsub->bridge);
}
dev_info(>dev, "ZynqMP DisplayPort Subsystem driver probed");
@@ -288,9 +287,8 @@ static void zynqmp_dpsub_remove(struct platform_device
*pdev)
if (dpsub->drm)
zynqmp_dpsub_drm_cleanup(dpsub);
- else
- drm_bridge_remove(dpsub->bridge);
+ drm_bridge_remove(dpsub->bridge);
zynqmp_disp_remove(dpsub);
zynqmp_dp_remove(dpsub);
I sent a similar patch:
https://lore.kernel.org/all/20240312-xilinx-dp-lock-fix-v1-1-1698f9f03...@ideasonboard.com/
I have the drm_bridge_add() call in zynqmp_dp_probe(), as that's where
the bridge is set up, so it felt like a logical place. You add it later,
just before the bridge is used the first time.
I like mine a bit more as it has all the bridge code in the same place,
but I also wonder if there might be some risks in adding the bridge
early (before zynqmp_disp_probe()), although I can't see any issue right
away...
Seems we have the same concerns :-) I've reviewed your patch and wrote
pretty much the same. I would be more comfortable with this version,
even if I like gathering all bridge code in the same location.
I guess there's no reason to take the risk here, so I have pushed this
one to drm-misc-next.
Tomi
In any case, as this works for me too:
Reviewed-by: Tomi Valkeinen
Reviewed-by: Laurent Pinchart
Re: [PATCH v4 00/13] drm: zynqmp_dp: IRQ cleanups and debugfs support
On 25/04/2024 18:17, Sean Anderson wrote: On 4/24/24 14:54, Tomi Valkeinen wrote: Hi Sean, On 23/04/2024 20:18, Sean Anderson wrote: This series cleans up the zyqnmp_dp IRQ and locking situation. Once that's done, it adds debugfs support. The intent is to enable compliance testing or to help debug signal-integrity issues. Last time I discussed converting the HPD work(s) to a threaded IRQ. I did not end up doing that for this series since the steps would be - Add locking - Move link retraining to a work function - Harden the IRQ - Merge the works into a threaded IRQ (omitted) Which with the exception of the final step is the same as leaving those works as-is. Conversion to a threaded IRQ can be done as a follow-up. If it's ok to you, I'd like to pick the first four patches to drm-misc already. Fine by me. I have pushed the first four to drm-misc. I'll try to look at the rest of the patches next week. Tomi
Re: [PATCH v2 2/2] drm: xlnx: zynqmp_dpsub: Fix compilation error
On 26/04/2024 04:46, Anatoliy Klymenko wrote:
Fix W=1 clang 19 compilation error in zynqmp_disp_layer_drm_formats().
Reported-by: kernel test robot
Closes:
https://lore.kernel.org/oe-kbuild-all/202404260946.4ozxvhd2-...@intel.com/
---
This is missing your signed-off-by.
Tomi
drivers/gpu/drm/xlnx/zynqmp_disp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c
b/drivers/gpu/drm/xlnx/zynqmp_disp.c
index 423f5f4943cc..c9fb432d4cbd 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
@@ -981,7 +981,7 @@ u32 *zynqmp_disp_layer_drm_formats(struct zynqmp_disp_layer
*layer,
unsigned int i;
u32 *formats;
- if (WARN_ON(!layer->mode == ZYNQMP_DPSUB_LAYER_NONLIVE)) {
+ if (WARN_ON(layer->mode != ZYNQMP_DPSUB_LAYER_NONLIVE)) {
*num_formats = 0;
return NULL;
}
Re: [PATCH v4 00/13] drm: zynqmp_dp: IRQ cleanups and debugfs support
Hi Sean, On 23/04/2024 20:18, Sean Anderson wrote: This series cleans up the zyqnmp_dp IRQ and locking situation. Once that's done, it adds debugfs support. The intent is to enable compliance testing or to help debug signal-integrity issues. Last time I discussed converting the HPD work(s) to a threaded IRQ. I did not end up doing that for this series since the steps would be - Add locking - Move link retraining to a work function - Harden the IRQ - Merge the works into a threaded IRQ (omitted) Which with the exception of the final step is the same as leaving those works as-is. Conversion to a threaded IRQ can be done as a follow-up. If it's ok to you, I'd like to pick the first four patches to drm-misc already. Tomi Changes in v4: - Rebase onto drm/drm-next Changes in v3: - Store base pointers in zynqmp_disp directly - Don't delay work - Convert to a hard IRQ - Use AUX IRQs instead of polling - Take dp->lock in zynqmp_dp_hpd_work_func Changes in v2: - Fix kerneldoc - Rearrange zynqmp_dp for better padding - Split off the HPD IRQ work into another commit - Expand the commit message - Document hpd_irq_work - Document debugfs files - Add ignore_aux_errors and ignore_hpd debugfs files to replace earlier implicit functionality - Attempt to fix unreproducable, spurious build warning - Drop "Optionally ignore DPCD errors" in favor of a debugfs file directly affecting zynqmp_dp_aux_transfer. Sean Anderson (13): drm: xlnx: Store base pointers in zynqmp_disp directly 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 delay work drm: zynqmp_dp: Add locking drm: zynqmp_dp: Don't retrain the link in our IRQ drm: zynqmp_dp: Convert to a hard IRQ drm: zynqmp_dp: Use AUX IRQs instead of polling drm: zynqmp_dp: Split off several helper functions drm: zynqmp_dp: Take dp->lock in zynqmp_dp_hpd_work_func drm: zynqmp_dp: Add debugfs interface for compliance testing Documentation/gpu/drivers.rst | 1 + Documentation/gpu/zynqmp.rst| 149 + MAINTAINERS | 1 + drivers/gpu/drm/xlnx/zynqmp_disp.c | 44 +- drivers/gpu/drm/xlnx/zynqmp_dp.c| 908 +--- drivers/gpu/drm/xlnx/zynqmp_dpsub.h | 1 + drivers/gpu/drm/xlnx/zynqmp_kms.h | 4 +- 7 files changed, 999 insertions(+), 109 deletions(-) create mode 100644 Documentation/gpu/zynqmp.rst
Re: [PATCH v4 0/7] Managing live video input format for ZynqMP DPSUB
Hi, On 16/04/2024 23:31, Anatoliy Klymenko wrote: Implement live video input format setting for ZynqMP DPSUB. ZynqMP DPSUB can operate in 2 modes: DMA-based and live. In the live mode, DPSUB receives a live video signal from FPGA-based CRTC. DPSUB acts as a DRM encoder bridge in such a scenario. To properly tune into the incoming video signal, DPSUB should be programmed with the proper media bus format. This patch series addresses this task. Patch 1/7: Set the DPSUB layer mode of operation prior to enabling the layer. Allows to use layer operational mode before its enablement. Patch 2/7: Update some IP register defines. Patch 3/7: Factor out some code into a helper function. Patch 4/7: Announce supported input media bus formats via drm_bridge_funcs.atomic_get_input_bus_fmts callback. Patch 5/7: Minimize usage of a global flag. Minor improvement. Patch 6/7: Program DPSUB live video input format based on selected bus config in the new atomic bridge state. Patch 7/7: New optional CRTC atomic helper proposal that will allow to negotiate video signal format between CRTC and connected encoder. Incorporate this callback into the DRM bridge format negotiation process. Save negotiated output format in drm_crtc_state. Reference usage of this API is available here: https://github.com/onotole/linux/tree/dpsub-live-in The patches up to and including patch 6 look ready to me. I'll pick them up to drm-misc. Tomi To: Laurent Pinchart To: Maarten Lankhorst To: Maxime Ripard To: Thomas Zimmermann To: David Airlie To: Daniel Vetter To: Michal Simek To: Andrzej Hajda To: Neil Armstrong To: Robert Foss To: Jonas Karlman To: Jernej Skrabec To: Rob Herring To: Krzysztof Kozlowski To: Conor Dooley To: Mauro Carvalho Chehab Cc: Tomi Valkeinen Cc: dri-devel@lists.freedesktop.org Cc: linux-arm-ker...@lists.infradead.org Cc: linux-ker...@vger.kernel.org Cc: devicet...@vger.kernel.org Cc: linux-me...@vger.kernel.org Signed-off-by: Anatoliy Klymenko Changes in v4: - Replace controversial reference driver patches with the private repository link. - Split display layer format manipulation functions into 2 separate cases for diferet layer modes. - Address misc review comments (typos, comments, etc.) Link to v3: https://lore.kernel.org/r/20240321-dp-live-fmt-v3-0-d5090d796...@amd.com Changes in v3: - Add connected live layer helper - Include reference DRM format in zynqmp_disp_format for live layerss. - Add default bus format list for non-live case. - Explain removal of redundant checks in the commit message. - Minor fixes and improvements from review comments. Link to v2: https://lore.kernel.org/r/20240312-dp-live-fmt-v2-0-a9c35dc5c...@amd.com Changes in v2: - Factor out register defines update into separate patch. - Add some improvements minimizing ithe usage of a global flag. - Reuse existing format setting API instead of introducing new versions. - Add warning around NULL check on new bridge state within atomic enable callback. - Add drm_helper_crtc_select_output_bus_format() that wraps drm_crtc_helper_funcs.select_output_bus_format(). - Update API comments per review recommendations. - Address some minor review comments. - Add reference CRTC driver that demonstrates the usage of the proposed drm_crtc_helper_funcs.select_output_bus_format() API. - Link to v1: https://lore.kernel.org/r/20240226-dp-live-fmt-v1-0-b78c3f69c...@amd.com --- Anatoliy Klymenko (7): drm: xlnx: zynqmp_dpsub: Set layer mode during creation drm: xlnx: zynqmp_dpsub: Update live format defines drm: xlnx: zynqmp_dpsub: Add connected live layer helper drm: xlnx: zynqmp_dpsub: Anounce supported input formats drm: xlnx: zynqmp_dpsub: Minimize usage of global flag drm: xlnx: zynqmp_dpsub: Set input live format drm/atomic-helper: Add select_output_bus_format callback drivers/gpu/drm/drm_bridge.c | 14 +- drivers/gpu/drm/drm_crtc_helper.c| 38 + drivers/gpu/drm/xlnx/zynqmp_disp.c | 231 +++ drivers/gpu/drm/xlnx/zynqmp_disp.h | 17 +-- drivers/gpu/drm/xlnx/zynqmp_disp_regs.h | 8 +- drivers/gpu/drm/xlnx/zynqmp_dp.c | 81 --- drivers/gpu/drm/xlnx/zynqmp_kms.c| 2 +- include/drm/drm_crtc.h | 11 ++ include/drm/drm_crtc_helper.h| 5 + include/drm/drm_modeset_helper_vtables.h | 30 10 files changed, 372 insertions(+), 65 deletions(-) --- base-commit: bfa4437fd3938ae2e186e7664b2db65bb8775670 change-id: 20240226-dp-live-fmt-6415773b5a68 Best regards,
Re: [PATCH v4 10/13] drm: zynqmp_dp: Use AUX IRQs instead of polling
On 23/04/2024 20:18, Sean Anderson wrote:
Instead of polling the status register for the AUX status, just enable
the IRQs and signal a completion.
Signed-off-by: Sean Anderson
---
This one seems to cause a hang when I unload the modules. I didn't debug
it further yet, but most likely we get an AUX interrupt when disabling
the hardware, and the driver hasn't disabled the IRQ handler.
Tomi
(no changes since v3)
Changes in v3:
- New
drivers/gpu/drm/xlnx/zynqmp_dp.c | 35 +++-
1 file changed, 25 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
index 9d61b6b8f2d4..863668642190 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
@@ -285,6 +285,7 @@ struct zynqmp_dp_config {
* @next_bridge: The downstream bridge
* @config: IP core configuration from DTS
* @aux: aux channel
+ * @aux_done: Completed when we get an AUX reply or timeout
* @phy: PHY handles for DP lanes
* @num_lanes: number of enabled phy lanes
* @hpd_work: hot plug detection worker
@@ -305,6 +306,7 @@ struct zynqmp_dp {
struct drm_bridge bridge;
struct work_struct hpd_work;
struct work_struct hpd_irq_work;
+ struct completion aux_done;
struct mutex lock;
struct drm_bridge *next_bridge;
@@ -941,12 +943,15 @@ static int zynqmp_dp_aux_cmd_submit(struct zynqmp_dp *dp,
u32 cmd, u16 addr,
u8 *buf, u8 bytes, u8 *reply)
{
bool is_read = (cmd & AUX_READ_BIT) ? true : false;
+ unsigned long time_left;
u32 reg, i;
reg = zynqmp_dp_read(dp, ZYNQMP_DP_INTERRUPT_SIGNAL_STATE);
if (reg & ZYNQMP_DP_INTERRUPT_SIGNAL_STATE_REQUEST)
return -EBUSY;
+ reinit_completion(>aux_done);
+
zynqmp_dp_write(dp, ZYNQMP_DP_AUX_ADDRESS, addr);
if (!is_read)
for (i = 0; i < bytes; i++)
@@ -961,17 +966,14 @@ static int zynqmp_dp_aux_cmd_submit(struct zynqmp_dp *dp,
u32 cmd, u16 addr,
zynqmp_dp_write(dp, ZYNQMP_DP_AUX_COMMAND, reg);
/* Wait for reply to be delivered upto 2ms */
- for (i = 0; ; i++) {
- reg = zynqmp_dp_read(dp, ZYNQMP_DP_INTERRUPT_SIGNAL_STATE);
- if (reg & ZYNQMP_DP_INTERRUPT_SIGNAL_STATE_REPLY)
- break;
+ time_left = wait_for_completion_timeout(>aux_done,
+ msecs_to_jiffies(2));
+ if (!time_left)
+ return -ETIMEDOUT;
- if (reg & ZYNQMP_DP_INTERRUPT_SIGNAL_STATE_REPLY_TIMEOUT ||
- i == 2)
- return -ETIMEDOUT;
-
- usleep_range(1000, 1100);
- }
+ reg = zynqmp_dp_read(dp, ZYNQMP_DP_INTERRUPT_SIGNAL_STATE);
+ if (reg & ZYNQMP_DP_INTERRUPT_SIGNAL_STATE_REPLY_TIMEOUT)
+ return -ETIMEDOUT;
reg = zynqmp_dp_read(dp, ZYNQMP_DP_AUX_REPLY_CODE);
if (reply)
@@ -1055,6 +1057,9 @@ static int zynqmp_dp_aux_init(struct zynqmp_dp *dp)
(w << ZYNQMP_DP_AUX_CLK_DIVIDER_AUX_FILTER_SHIFT) |
(rate / (1000 * 1000)));
+ zynqmp_dp_write(dp, ZYNQMP_DP_INT_EN, ZYNQMP_DP_INT_REPLY_RECEIVED |
+ ZYNQMP_DP_INT_REPLY_TIMEOUT);
+
dp->aux.name = "ZynqMP DP AUX";
dp->aux.dev = dp->dev;
dp->aux.drm_dev = dp->bridge.dev;
@@ -1072,6 +1077,9 @@ static int zynqmp_dp_aux_init(struct zynqmp_dp *dp)
static void zynqmp_dp_aux_cleanup(struct zynqmp_dp *dp)
{
drm_dp_aux_unregister(>aux);
+
+ zynqmp_dp_write(dp, ZYNQMP_DP_INT_DS, ZYNQMP_DP_INT_REPLY_RECEIVED |
+ ZYNQMP_DP_INT_REPLY_TIMEOUT);
}
/* -
@@ -1685,6 +1693,12 @@ static irqreturn_t zynqmp_dp_irq_handler(int irq, void
*data)
if (status & ZYNQMP_DP_INT_HPD_IRQ)
schedule_work(>hpd_irq_work);
+ if (status & ZYNQMP_DP_INTERRUPT_SIGNAL_STATE_REPLY)
+ complete(>aux_done);
+
+ if (status & ZYNQMP_DP_INTERRUPT_SIGNAL_STATE_REPLY_TIMEOUT)
+ complete(>aux_done);
+
return IRQ_HANDLED;
}
@@ -1708,6 +1722,7 @@ int zynqmp_dp_probe(struct zynqmp_dpsub *dpsub)
dp->dpsub = dpsub;
dp->status = connector_status_disconnected;
mutex_init(>lock);
+ init_completion(>aux_done);
INIT_WORK(>hpd_work, zynqmp_dp_hpd_work_func);
INIT_WORK(>hpd_irq_work, zynqmp_dp_hpd_irq_work_func);
Re: [PATCH] drm: zynqmp_dpsub: Always register bridge
On 23/04/2024 23:50, Sean Anderson wrote:
Hi,
On 3/22/24 02:01, Tomi Valkeinen wrote:
Hi,
On 08/03/2024 22:47, Sean Anderson wrote:
We must always register the DRM bridge, since zynqmp_dp_hpd_work_func
calls drm_bridge_hpd_notify, which in turn expects hpd_mutex to be
initialized. We do this before zynqmp_dpsub_drm_init since that calls
drm_bridge_attach. This fixes the following lockdep warning:
[ 19.217084] [ cut here ]
[ 19.227530] DEBUG_LOCKS_WARN_ON(lock->magic != lock)
[ 19.227768] WARNING: CPU: 0 PID: 140 at kernel/locking/mutex.c:582
__mutex_lock+0x4bc/0x550
[ 19.241696] Modules linked in:
[ 19.244937] CPU: 0 PID: 140 Comm: kworker/0:4 Not tainted 6.6.20+ #96
[ 19.252046] Hardware name: xlnx,zynqmp (DT)
[ 19.256421] Workqueue: events zynqmp_dp_hpd_work_func
[ 19.261795] pstate: 6005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[ 19.269104] pc : __mutex_lock+0x4bc/0x550
[ 19.273364] lr : __mutex_lock+0x4bc/0x550
[ 19.277592] sp : ffc085c5bbe0
[ 19.281066] x29: ffc085c5bbe0 x28: x27: ff88009417f8
[ 19.288624] x26: ff8800941788 x25: ff8800020008 x24: ffc082aa3000
[ 19.296227] x23: ffc080d90e3c x22: 0002 x21:
[ 19.303744] x20: x19: ff88002f5210 x18:
[ 19.311295] x17: 6c707369642e3030 x16: 3030613464662072 x15: 0720072007200720
[ 19.318922] x14: x13: 284e4f5f4e524157 x12: 0001
[ 19.326442] x11: 0001ffc085c5b940 x10: 0001ff88003f388b x9 : 0001ff88003f3888
[ 19.334003] x8 : 0001ff88003f3888 x7 : x6 :
[ 19.341537] x5 : x4 : 1668 x3 :
[ 19.349054] x2 : x1 : x0 : ff88003f3880
[ 19.356581] Call trace:
[ 19.359160] __mutex_lock+0x4bc/0x550
[ 19.363032] mutex_lock_nested+0x24/0x30
[ 19.367187] drm_bridge_hpd_notify+0x2c/0x6c
[ 19.371698] zynqmp_dp_hpd_work_func+0x44/0x54
[ 19.376364] process_one_work+0x3ac/0x988
[ 19.380660] worker_thread+0x398/0x694
[ 19.384736] kthread+0x1bc/0x1c0
[ 19.388241] ret_from_fork+0x10/0x20
[ 19.392031] irq event stamp: 183
[ 19.395450] hardirqs last enabled at (183): []
finish_task_switch.isra.0+0xa8/0x2d4
[ 19.405140] hardirqs last disabled at (182): []
__schedule+0x714/0xd04
[ 19.413612] softirqs last enabled at (114): []
srcu_invoke_callbacks+0x158/0x23c
[ 19.423128] softirqs last disabled at (110): []
srcu_invoke_callbacks+0x158/0x23c
[ 19.432614] ---[ end trace ]---
Fixes: eb2d64bfcc17 ("drm: xlnx: zynqmp_dpsub: Report HPD through the bridge")
Signed-off-by: Sean Anderson
---
drivers/gpu/drm/xlnx/zynqmp_dpsub.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
b/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
index 88eb33acd5f0..639fff2c693f 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
@@ -256,12 +256,11 @@ static int zynqmp_dpsub_probe(struct platform_device
*pdev)
if (ret)
goto err_dp;
+ drm_bridge_add(dpsub->bridge);
if (dpsub->dma_enabled) {
ret = zynqmp_dpsub_drm_init(dpsub);
if (ret)
goto err_disp;
- } else {
- drm_bridge_add(dpsub->bridge);
}
dev_info(>dev, "ZynqMP DisplayPort Subsystem driver probed");
@@ -288,9 +287,8 @@ static void zynqmp_dpsub_remove(struct platform_device
*pdev)
if (dpsub->drm)
zynqmp_dpsub_drm_cleanup(dpsub);
- else
- drm_bridge_remove(dpsub->bridge);
+ drm_bridge_remove(dpsub->bridge);
zynqmp_disp_remove(dpsub);
zynqmp_dp_remove(dpsub);
I sent a similar patch:
https://lore.kernel.org/all/20240312-xilinx-dp-lock-fix-v1-1-1698f9f03...@ideasonboard.com/
I have the drm_bridge_add() call in zynqmp_dp_probe(), as that's where the
bridge is set up, so it felt like a logical place. You add it later, just
before the bridge is used the first time.
I like mine a bit more as it has all the bridge code in the same place, but I
also wonder if there might be some risks in adding the bridge early (before
zynqmp_disp_probe()), although I can't see any issue right away...
In any case, as this works for me too:
Reviewed-by: Tomi Valkeinen
Tomi
Can someone pick up this fix before the release? I am still running into this
bug on linux-next/master.
Can you check my version (link above)? After looking at both versions
today, I'd rather push mine: it adds and removes the bridge in the same
file where all the bridge code resides, and it'd be nice to manage the
bridge in that file as much as possible.
Tomi
Re: [PATCH v3 00/13] drm: zynqmp_dp: IRQ cleanups and debugfs support
On 23/04/2024 17:59, Sean Anderson wrote: On 4/23/24 09:33, Tomi Valkeinen wrote: Hi Sean, On 22/04/2024 21:45, Sean Anderson wrote: This series cleans up the zyqnmp_dp IRQ and locking situation. Once that's done, it adds debugfs support. The intent is to enable compliance testing or to help debug signal-integrity issues. Last time I discussed converting the HPD work(s) to a threaded IRQ. I did not end up doing that for this series since the steps would be - Add locking - Move link retraining to a work function - Harden the IRQ - Merge the works into a threaded IRQ (omitted) Which with the exception of the final step is the same as leaving those works as-is. Conversion to a threaded IRQ can be done as a follow-up. What is the base for this series? I'm having trouble applying it. I managed to mostly apply it, but I see the board hang when I unload the modules. I didn't debug it as it might as well be caused by my conflict resolution. The base is v6.8-rc1, but it should probably be v6.9. I can rebase and resend. Did you have something extra in your branch before the series? I got "error: sha1 information is lacking or useless". Tomi
Re: [PATCH v3 00/13] drm: zynqmp_dp: IRQ cleanups and debugfs support
Hi Sean, On 22/04/2024 21:45, Sean Anderson wrote: This series cleans up the zyqnmp_dp IRQ and locking situation. Once that's done, it adds debugfs support. The intent is to enable compliance testing or to help debug signal-integrity issues. Last time I discussed converting the HPD work(s) to a threaded IRQ. I did not end up doing that for this series since the steps would be - Add locking - Move link retraining to a work function - Harden the IRQ - Merge the works into a threaded IRQ (omitted) Which with the exception of the final step is the same as leaving those works as-is. Conversion to a threaded IRQ can be done as a follow-up. What is the base for this series? I'm having trouble applying it. I managed to mostly apply it, but I see the board hang when I unload the modules. I didn't debug it as it might as well be caused by my conflict resolution. Tomi Changes in v3: - Store base pointers in zynqmp_disp directly - Don't delay work - Convert to a hard IRQ - Use AUX IRQs instead of polling - Take dp->lock in zynqmp_dp_hpd_work_func Changes in v2: - Fix kerneldoc - Rearrange zynqmp_dp for better padding - Split off the HPD IRQ work into another commit - Expand the commit message - Document hpd_irq_work - Document debugfs files - Add ignore_aux_errors and ignore_hpd debugfs files to replace earlier implicit functionality - Attempt to fix unreproducable, spurious build warning - Drop "Optionally ignore DPCD errors" in favor of a debugfs file directly affecting zynqmp_dp_aux_transfer. Sean Anderson (13): drm: xlnx: Store base pointers in zynqmp_disp directly 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 delay work drm: zynqmp_dp: Add locking drm: zynqmp_dp: Don't retrain the link in our IRQ drm: zynqmp_dp: Convert to a hard IRQ drm: zynqmp_dp: Use AUX IRQs instead of polling drm: zynqmp_dp: Split off several helper functions drm: zynqmp_dp: Take dp->lock in zynqmp_dp_hpd_work_func drm: zynqmp_dp: Add debugfs interface for compliance testing Documentation/gpu/drivers.rst | 1 + Documentation/gpu/zynqmp.rst| 149 + MAINTAINERS | 1 + drivers/gpu/drm/xlnx/zynqmp_disp.c | 44 +- drivers/gpu/drm/xlnx/zynqmp_dp.c| 909 +--- drivers/gpu/drm/xlnx/zynqmp_dpsub.h | 1 + drivers/gpu/drm/xlnx/zynqmp_kms.h | 4 +- 7 files changed, 1000 insertions(+), 109 deletions(-) create mode 100644 Documentation/gpu/zynqmp.rst
Re: [PATCH v3 06/13] drm: zynqmp_dp: Don't delay work
On 22/04/2024 21:45, Sean Anderson wrote:
We always call scheduled_delayed_work with no delay, so just use a
non-delayed work_struct instead.
Signed-off-by: Sean Anderson
---
Reviewed-by: Tomi Valkeinen
Tomi
Changes in v3:
- New
drivers/gpu/drm/xlnx/zynqmp_dp.c | 13 ++---
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
index f1834c8e3c02..59fed00a8f89 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
@@ -297,7 +297,7 @@ struct zynqmp_dp_config {
struct zynqmp_dp {
struct drm_dp_aux aux;
struct drm_bridge bridge;
- struct delayed_work hpd_work;
+ struct work_struct hpd_work;
struct drm_bridge *next_bridge;
struct device *dev;
@@ -1467,7 +1467,7 @@ static void zynqmp_dp_bridge_atomic_disable(struct
drm_bridge *bridge,
struct zynqmp_dp *dp = bridge_to_dp(bridge);
dp->enabled = false;
- cancel_delayed_work(>hpd_work);
+ cancel_work(>hpd_work);
zynqmp_dp_write(dp, ZYNQMP_DP_MAIN_STREAM_ENABLE, 0);
drm_dp_dpcd_writeb(>aux, DP_SET_POWER, DP_SET_POWER_D3);
zynqmp_dp_write(dp, ZYNQMP_DP_TX_PHY_POWER_DOWN,
@@ -1603,8 +1603,7 @@ void zynqmp_dp_disable_vblank(struct zynqmp_dp *dp)
static void zynqmp_dp_hpd_work_func(struct work_struct *work)
{
- struct zynqmp_dp *dp = container_of(work, struct zynqmp_dp,
- hpd_work.work);
+ struct zynqmp_dp *dp = container_of(work, struct zynqmp_dp, hpd_work);
enum drm_connector_status status;
status = zynqmp_dp_bridge_detect(>bridge);
@@ -1633,7 +1632,7 @@ static irqreturn_t zynqmp_dp_irq_handler(int irq, void
*data)
zynqmp_dpsub_drm_handle_vblank(dp->dpsub);
if (status & ZYNQMP_DP_INT_HPD_EVENT)
- schedule_delayed_work(>hpd_work, 0);
+ schedule_work(>hpd_work);
if (status & ZYNQMP_DP_INT_HPD_IRQ) {
int ret;
@@ -1675,7 +1674,7 @@ int zynqmp_dp_probe(struct zynqmp_dpsub *dpsub)
dp->dpsub = dpsub;
dp->status = connector_status_disconnected;
- INIT_DELAYED_WORK(>hpd_work, zynqmp_dp_hpd_work_func);
+ INIT_WORK(>hpd_work, zynqmp_dp_hpd_work_func);
/* Acquire all resources (IOMEM, IRQ and PHYs). */
res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dp");
@@ -1775,7 +1774,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(>hpd_work);
+ cancel_work_sync(>hpd_work);
zynqmp_dp_write(dp, ZYNQMP_DP_TRANSMITTER_ENABLE, 0);
zynqmp_dp_write(dp, ZYNQMP_DP_INT_DS, 0x);
Re: [PATCH v3 02/13] drm: xlnx: Fix kerneldoc
On 22/04/2024 21:45, Sean Anderson wrote:
Fix a few errors in the kerneldoc. Mostly this addresses missing/renamed
members.
Signed-off-by: Sean Anderson
---
Reviewed-by: Tomi Valkeinen
Tomi
Changes in v3:
- Split off documentation for base pointers to previous commit
Changes in v2:
- New
drivers/gpu/drm/xlnx/zynqmp_dpsub.h | 1 +
drivers/gpu/drm/xlnx/zynqmp_kms.h | 4 ++--
2 files changed, 3 insertions(+), 2 deletions(-)
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
Re: [PATCH v3 01/13] drm: xlnx: Store base pointers in zynqmp_disp directly
On 22/04/2024 21:45, Sean Anderson wrote:
The blend, avbuf, and audio members of zynqmp_disp are anonymous structs
with only one member each. This is rather pointless, so move the members
up a level.
Signed-off-by: Sean Anderson
---
Changes in v3:
- New
I would have renamed the fields to, e.g., "blend_base", but it doesn't
really matter as they are accessed only in a couple of places.
Reviewed-by: Tomi Valkeinen
Tomi
drivers/gpu/drm/xlnx/zynqmp_disp.c | 44 +-
1 file changed, 19 insertions(+), 25 deletions(-)
diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c
b/drivers/gpu/drm/xlnx/zynqmp_disp.c
index 407bc07cec69..94a3ac046373 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
@@ -128,24 +128,18 @@ 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 {
struct device *dev;
struct zynqmp_dpsub *dpsub;
- struct {
- void __iomem *base;
- } blend;
- struct {
- void __iomem *base;
- } avbuf;
- struct {
- void __iomem *base;
- } audio;
+ void __iomem *blend;
+ void __iomem *avbuf;
+ void __iomem *audio;
struct zynqmp_disp_layer layers[ZYNQMP_DPSUB_NUM_LAYERS];
};
@@ -356,12 +350,12 @@ static const struct zynqmp_disp_format avbuf_gfx_fmts[] =
{
static u32 zynqmp_disp_avbuf_read(struct zynqmp_disp *disp, int reg)
{
- return readl(disp->avbuf.base + reg);
+ return readl(disp->avbuf + reg);
}
static void zynqmp_disp_avbuf_write(struct zynqmp_disp *disp, int reg, u32 val)
{
- writel(val, disp->avbuf.base + reg);
+ writel(val, disp->avbuf + reg);
}
static bool zynqmp_disp_layer_is_video(const struct zynqmp_disp_layer *layer)
@@ -587,7 +581,7 @@ static void zynqmp_disp_avbuf_disable(struct zynqmp_disp
*disp)
static void zynqmp_disp_blend_write(struct zynqmp_disp *disp, int reg, u32 val)
{
- writel(val, disp->blend.base + reg);
+ writel(val, disp->blend + reg);
}
/*
@@ -813,7 +807,7 @@ static void zynqmp_disp_blend_layer_disable(struct
zynqmp_disp *disp,
static void zynqmp_disp_audio_write(struct zynqmp_disp *disp, int reg, u32 val)
{
- writel(val, disp->audio.base + reg);
+ writel(val, disp->audio + reg);
}
/**
@@ -1237,21 +1231,21 @@ int zynqmp_disp_probe(struct zynqmp_dpsub *dpsub)
disp->dev = >dev;
disp->dpsub = dpsub;
- disp->blend.base = devm_platform_ioremap_resource_byname(pdev, "blend");
- if (IS_ERR(disp->blend.base)) {
- ret = PTR_ERR(disp->blend.base);
+ disp->blend = devm_platform_ioremap_resource_byname(pdev, "blend");
+ if (IS_ERR(disp->blend)) {
+ ret = PTR_ERR(disp->blend);
goto error;
}
- disp->avbuf.base = devm_platform_ioremap_resource_byname(pdev, "av_buf");
- if (IS_ERR(disp->avbuf.base)) {
- ret = PTR_ERR(disp->avbuf.base);
+ disp->avbuf = devm_platform_ioremap_resource_byname(pdev, "av_buf");
+ if (IS_ERR(disp->avbuf)) {
+ ret = PTR_ERR(disp->avbuf);
goto error;
}
- disp->audio.base = devm_platform_ioremap_resource_byname(pdev, "aud");
- if (IS_ERR(disp->audio.base)) {
- ret = PTR_ERR(disp->audio.base);
+ disp->audio = devm_platform_ioremap_resource_byname(pdev, "aud");
+ if (IS_ERR(disp->audio)) {
+ ret = PTR_ERR(disp->audio);
goto error;
}
Re: [PATCH v4 6/7] drm: xlnx: zynqmp_dpsub: Set input live format
On 16/04/2024 23:31, Anatoliy Klymenko wrote:
Program live video input format according to selected media bus format.
In the bridge mode of operation, DPSUB is connected to FPGA CRTC which
almost certainly supports a single media bus format as its output. Expect
this to be delivered via the new bridge atomic state. Program DPSUB
registers accordingly.
Signed-off-by: Anatoliy Klymenko
---
drivers/gpu/drm/xlnx/zynqmp_disp.c | 92 --
drivers/gpu/drm/xlnx/zynqmp_disp.h | 2 +
drivers/gpu/drm/xlnx/zynqmp_dp.c | 13 --
3 files changed, 90 insertions(+), 17 deletions(-)
Reviewed-by: Tomi Valkeinen
Tomi
diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c
b/drivers/gpu/drm/xlnx/zynqmp_disp.c
index 8cdd74a9b772..13157da0089e 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
@@ -436,19 +436,29 @@ static void zynqmp_disp_avbuf_set_format(struct
zynqmp_disp *disp,
const struct zynqmp_disp_format *fmt)
{
unsigned int i;
- u32 val;
+ u32 val, reg;
- val = zynqmp_disp_avbuf_read(disp, ZYNQMP_DISP_AV_BUF_FMT);
- val &= zynqmp_disp_layer_is_video(layer)
- ? ~ZYNQMP_DISP_AV_BUF_FMT_NL_VID_MASK
- : ~ZYNQMP_DISP_AV_BUF_FMT_NL_GFX_MASK;
- val |= fmt->buf_fmt;
- zynqmp_disp_avbuf_write(disp, ZYNQMP_DISP_AV_BUF_FMT, val);
+ layer->disp_fmt = fmt;
+ if (layer->mode == ZYNQMP_DPSUB_LAYER_NONLIVE) {
+ reg = ZYNQMP_DISP_AV_BUF_FMT;
+ val = zynqmp_disp_avbuf_read(disp, ZYNQMP_DISP_AV_BUF_FMT);
+ val &= zynqmp_disp_layer_is_video(layer)
+ ? ~ZYNQMP_DISP_AV_BUF_FMT_NL_VID_MASK
+ : ~ZYNQMP_DISP_AV_BUF_FMT_NL_GFX_MASK;
+ val |= fmt->buf_fmt;
+ zynqmp_disp_avbuf_write(disp, reg, val);
+ } else {
+ reg = zynqmp_disp_layer_is_video(layer)
+ ? ZYNQMP_DISP_AV_BUF_LIVE_VID_CONFIG
+ : ZYNQMP_DISP_AV_BUF_LIVE_GFX_CONFIG;
+ val = fmt->buf_fmt;
+ zynqmp_disp_avbuf_write(disp, reg, val);
+ }
for (i = 0; i < ZYNQMP_DISP_AV_BUF_NUM_SF; i++) {
- unsigned int reg = zynqmp_disp_layer_is_video(layer)
-? ZYNQMP_DISP_AV_BUF_VID_COMP_SF(i)
-: ZYNQMP_DISP_AV_BUF_GFX_COMP_SF(i);
+ reg = zynqmp_disp_layer_is_video(layer)
+ ? ZYNQMP_DISP_AV_BUF_VID_COMP_SF(i)
+ : ZYNQMP_DISP_AV_BUF_GFX_COMP_SF(i);
zynqmp_disp_avbuf_write(disp, reg, fmt->sf[i]);
}
@@ -926,6 +936,31 @@ zynqmp_disp_layer_find_format(struct zynqmp_disp_layer
*layer,
return NULL;
}
+/**
+ * zynqmp_disp_layer_find_live_format - Find format information for given
+ * media bus format
+ * @layer: The layer
+ * @drm_fmt: Media bus format to search
+ *
+ * Search display subsystem format information corresponding to the given media
+ * bus format @media_bus_format for the @layer, and return a pointer to the
+ * format descriptor.
+ *
+ * Return: A pointer to the format descriptor if found, NULL otherwise
+ */
+static const struct zynqmp_disp_format *
+zynqmp_disp_layer_find_live_format(struct zynqmp_disp_layer *layer,
+ u32 media_bus_format)
+{
+ unsigned int i;
+
+ for (i = 0; i < layer->info->num_formats; i++)
+ if (layer->info->formats[i].bus_fmt == media_bus_format)
+ return >info->formats[i];
+
+ return NULL;
+}
+
/**
* zynqmp_disp_layer_drm_formats - Return the DRM formats supported by the
layer
* @layer: The layer
@@ -1040,6 +1075,9 @@ void zynqmp_disp_layer_disable(struct zynqmp_disp_layer
*layer)
* @layer: The layer
* @info: The format info
*
+ * NOTE: Use zynqmp_disp_layer_set_live_format() to set media bus format for
+ * live video layers.
+ *
* Set the format for @layer to @info. The layer must be disabled.
*/
void zynqmp_disp_layer_set_format(struct zynqmp_disp_layer *layer,
@@ -1047,14 +1085,16 @@ void zynqmp_disp_layer_set_format(struct
zynqmp_disp_layer *layer,
{
unsigned int i;
+ if (WARN_ON(layer->mode != ZYNQMP_DPSUB_LAYER_NONLIVE))
+ return;
+
layer->disp_fmt = zynqmp_disp_layer_find_format(layer, info->format);
+ if (WARN_ON(!layer->disp_fmt))
+ return;
layer->drm_fmt = info;
zynqmp_disp_avbuf_set_format(layer->disp, layer, layer->disp_fmt);
- if (layer->mode == ZYNQMP_DPSUB_LAYER_LIVE)
- return;
-
/*
* Set pconfig for each DMA channel to indicate they're part of a
* video group.
@@ -1074,6 +1114,32 @@ void zynqmp_disp_layer_set_format(struct
zynqmp_disp_layer *layer,
}
}
+/**
+ * zynqmp_disp_layer_set_liv
Re: [PATCH v4 4/7] drm: xlnx: zynqmp_dpsub: Anounce supported input formats
On 16/04/2024 23:31, Anatoliy Klymenko wrote:
DPSUB in bridge mode supports multiple input media bus formats.
Announce the list of supported input media bus formats via
drm_bridge.atomic_get_input_bus_fmts callback. Introduce a set of live
input formats supported by DPSUB. Add safeguards to format list functions
to prevent their misuse in the different layer modes contexts.
Reviewed-by: Laurent Pinchart
Signed-off-by: Anatoliy Klymenko
---
drivers/gpu/drm/xlnx/zynqmp_disp.c | 110 +++--
drivers/gpu/drm/xlnx/zynqmp_disp.h | 2 +
drivers/gpu/drm/xlnx/zynqmp_dp.c | 31 +++
3 files changed, 139 insertions(+), 4 deletions(-)
Reviewed-by: Tomi Valkeinen
Tomi
diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c
b/drivers/gpu/drm/xlnx/zynqmp_disp.c
index 891577475349..24f1f367b1d3 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
@@ -18,6 +18,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -77,12 +78,14 @@ enum zynqmp_dpsub_layer_mode {
/**
* struct zynqmp_disp_format - Display subsystem format information
* @drm_fmt: DRM format (4CC)
+ * @bus_fmt: Media bus format
* @buf_fmt: AV buffer format
* @swap: Flag to swap R & B for RGB formats, and U & V for YUV formats
* @sf: Scaling factors for color components
*/
struct zynqmp_disp_format {
u32 drm_fmt;
+ u32 bus_fmt;
u32 buf_fmt;
bool swap;
const u32 *sf;
@@ -182,6 +185,12 @@ static const u32 scaling_factors_565[] = {
ZYNQMP_DISP_AV_BUF_5BIT_SF,
};
+static const u32 scaling_factors_666[] = {
+ ZYNQMP_DISP_AV_BUF_6BIT_SF,
+ ZYNQMP_DISP_AV_BUF_6BIT_SF,
+ ZYNQMP_DISP_AV_BUF_6BIT_SF,
+};
+
static const u32 scaling_factors_888[] = {
ZYNQMP_DISP_AV_BUF_8BIT_SF,
ZYNQMP_DISP_AV_BUF_8BIT_SF,
@@ -364,6 +373,41 @@ static const struct zynqmp_disp_format avbuf_gfx_fmts[] = {
},
};
+/* List of live video layer formats */
+static const struct zynqmp_disp_format avbuf_live_fmts[] = {
+ {
+ .drm_fmt= DRM_FORMAT_RGB565,
+ .bus_fmt= MEDIA_BUS_FMT_RGB666_1X18,
+ .buf_fmt= ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_6 |
+ ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_RGB,
+ .sf = scaling_factors_666,
+ }, {
+ .drm_fmt= DRM_FORMAT_RGB888,
+ .bus_fmt= MEDIA_BUS_FMT_RGB888_1X24,
+ .buf_fmt= ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_8 |
+ ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_RGB,
+ .sf = scaling_factors_888,
+ }, {
+ .drm_fmt= DRM_FORMAT_YUV422,
+ .bus_fmt= MEDIA_BUS_FMT_UYVY8_1X16,
+ .buf_fmt= ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_8 |
+ ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV422,
+ .sf = scaling_factors_888,
+ }, {
+ .drm_fmt= DRM_FORMAT_YUV444,
+ .bus_fmt= MEDIA_BUS_FMT_VUY8_1X24,
+ .buf_fmt= ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_8 |
+ ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV444,
+ .sf = scaling_factors_888,
+ }, {
+ .drm_fmt= DRM_FORMAT_P210,
+ .bus_fmt= MEDIA_BUS_FMT_UYVY10_1X20,
+ .buf_fmt= ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_10 |
+ ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV422,
+ .sf = scaling_factors_101010,
+ },
+};
+
static u32 zynqmp_disp_avbuf_read(struct zynqmp_disp *disp, int reg)
{
return readl(disp->avbuf.base + reg);
@@ -887,6 +931,11 @@ zynqmp_disp_layer_find_format(struct zynqmp_disp_layer
*layer,
* @layer: The layer
* @num_formats: Pointer to the returned number of formats
*
+ * NOTE: This function doesn't make sense for live video layers and will
+ * always return an empty list in such cases. zynqmp_disp_live_layer_formats()
+ * should be used to query a list of media bus formats supported by the live
+ * video input layer.
+ *
* Return: A newly allocated u32 array that stores all the DRM formats
* supported by the layer. The number of formats in the array is returned
* through the num_formats argument.
@@ -897,10 +946,17 @@ u32 *zynqmp_disp_layer_drm_formats(struct
zynqmp_disp_layer *layer,
unsigned int i;
u32 *formats;
+ if (WARN_ON(!layer->mode == ZYNQMP_DPSUB_LAYER_NONLIVE)) {
+ *num_formats = 0;
+ return NULL;
+ }
+
formats = kcalloc(layer->info->num_formats, sizeof(*formats),
GFP_KERNEL);
- if (!formats)
+ if (!formats) {
+
Re: [PATCH v4 2/7] drm: xlnx: zynqmp_dpsub: Update live format defines
On 16/04/2024 23:31, Anatoliy Klymenko wrote: Update live format defines to match DPSUB AV_BUF_LIVE_VID_CONFIG register layout. These defines were never referenced before, so no other changes required. Reviewed-by: Laurent Pinchart Signed-off-by: Anatoliy Klymenko --- drivers/gpu/drm/xlnx/zynqmp_disp_regs.h | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h b/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h index f92a006d5070..fa3935384834 100644 --- a/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h +++ b/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h @@ -165,10 +165,10 @@ #define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_10 0x2 #define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_12 0x3 #define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_MASK GENMASK(2, 0) -#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_RGB 0x0 -#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV444 0x1 -#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV422 0x2 -#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YONLY 0x3 +#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_RGB (0x0 << 4) +#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV444 (0x1 << 4) +#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV422 (0x2 << 4) +#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YONLY (0x3 << 4) #define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_MASK GENMASK(5, 4) #define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_CB_FIRST BIT(8) #define ZYNQMP_DISP_AV_BUF_PALETTE_MEMORY 0x400 Reviewed-by: Tomi Valkeinen Tomi
Re: [PATCH RFC 2/2] pmdomain: ti-sci: Support retaining PD boot time state
On 15/04/2024 19:00, Tomi Valkeinen wrote: Add a new flag, TI_SCI_PD_KEEP_BOOT_STATE, which can be set in the dts when referring to power domains. When this flag is set, the ti-sci driver will check if the PD is currently enabled in the HW, and if so, set the GENPD_FLAG_ALWAYS_ON flag so that the PD will stay enabled. The main issue I'm trying to solve here is this: If the Display Subsystem (DSS) has been enabled by the bootloader, the related PD has also been enabled in the HW. When the tidss driver probes, the driver framework will automatically enable the PD. While executing the probe function it is very common for the probe to return EPROBE_DEFER, and, in rarer cases, an actual error. When this happens (probe() returns an error), the driver framework will automatically disable the related PD. Powering off the PD while the DSS is enabled and displaying a picture will cause the DSS HW to enter a bad state, from which (afaik) it can't be woken up except with full power-cycle. Trying to access the DSS in this state (e.g. when retrying the probe) will usually cause the board to hang sooner or later. Even if we wouldn't have this board-hangs issue, it's nice to be able to keep the DSS PD enabled: we want to keep the DSS enabled when the bootloader has enabled the screen. If, instead, we disable the PD at the first EPROBE_DEFER, the screen will (probably) go black. A few things occurred to me. The driver is supposed to clear the GENPD_FLAG_ALWAYS_ON when the driver has probed successfully. There are two possible issues with that: - Afaics, there's no API to do that, and currently I just clear the bit in genpd->flags. There's a clear race there, so some locking would be required. - This uses the GENPD_FLAG_ALWAYS_ON flag to say "PD is always on, until the driver has started". If the PD would have GENPD_FLAG_ALWAYS_ON set for other reasons, the driver would still go and clear the flag, which might break things. Also, unrelated to the above and not a problem in practice at the very moment, but I think clocks should also be dealt with somehow. Something, at early-ish boot stage, should mark the relevant clocks as in use, so that there's no chance they would be turned off when the main kernel has started (the main display driver is often a module). It would be nice to deal with all the above in a single place. I wonder if the tidss driver itself could somehow be split into two parts, an early part that would probe with minimal dependencies, mainly to reserve the core resources without doing any kind of DRM init. And a main part which would (somehow) finish the initialization at a later point, when we have the filesystem (for firmware) and the other bridge/panel drivers have probed. That can be somewhat achieved with simplefb or simpledrm, though, but we can't do any TI DSS specific things there, and it also creates a requirement to have either of those drivers built-in, and the related DT nodes to be added. Tomi Another option here would perhaps be to change the driver framework (drivers/base/platform.c) which attaches and detaches the PD, and make it somehow optional, allowing the driver the manage the PD. That option has two downsides: 1) the driver _has_ to manage the PD, which would rule out the use of simplefb and simpledrm, and 2) it would leave the PD in off state from Linux's perspective until a driver enables the PD, and that might mean that the PD gets actually disabled as part of normal system wide power management (disabling unused resources). Yet another option would be to do this outside the ti_sci_pm_domains driver: a piece of code that would somehow be ran after the ti_sci_pm_domains driver has probed (so that we have the PDs), but before tidss/simplefb/simpledrm probes. The problem here is the "somehow" part. Also, this would partly have the same issue 2) as mentioned above. TODO: If this approach is ok, sci-pm-domain.yaml needs to be extended. Also, it sounds a bit like the cell value is not a bit-mask, so maybe adding TI_SCI_PD_KEEP_BOOT_STATE flag this way is not fine. Signed-off-by: Tomi Valkeinen --- drivers/pmdomain/ti/ti_sci_pm_domains.c| 27 +-- include/dt-bindings/soc/ti,sci_pm_domain.h | 1 + 2 files changed, 26 insertions(+), 2 deletions(-) diff --git a/drivers/pmdomain/ti/ti_sci_pm_domains.c b/drivers/pmdomain/ti/ti_sci_pm_domains.c index 1510d5ddae3d..b71b390aaa39 100644 --- a/drivers/pmdomain/ti/ti_sci_pm_domains.c +++ b/drivers/pmdomain/ti/ti_sci_pm_domains.c @@ -103,7 +103,7 @@ static struct generic_pm_domain *ti_sci_pd_xlate( return ERR_PTR(-ENOENT); genpd_to_ti_sci_pd(genpd_data->domains[idx])->exclusive = - genpdspec->args[1]; + genpdspec->args[1] & TI_SCI_PD_EXCLUSIVE; return genpd_data->domains[idx]; } @@ -161,6 +161,8 @@ static int ti_sci_pm_domain_probe(struct platform_device *p
[PATCH RFC 0/2] pmdomain: ti-sci: Handling DSS boot splash
The first patch here is a fix for ti_sci_pm_domains to handle the case where the dts has two nodes which refer to the same PD. The second is the interesting one, and very much an RFC. Tomi Signed-off-by: Tomi Valkeinen --- Tomi Valkeinen (2): pmdomain: ti-sci: Fix duplicate PD referrals pmdomain: ti-sci: Support retaining PD boot time state drivers/pmdomain/ti/ti_sci_pm_domains.c| 47 -- include/dt-bindings/soc/ti,sci_pm_domain.h | 1 + 2 files changed, 45 insertions(+), 3 deletions(-) --- base-commit: 0bbac3facb5d6cc0171c45c9873a2dc96bea9680 change-id: 20240415-ti-sci-pd-33b39f6b0586 Best regards, -- Tomi Valkeinen
[PATCH RFC 1/2] pmdomain: ti-sci: Fix duplicate PD referrals
When the dts file has multiple referrers to a single PD (e.g.
simple-framebuffer and dss nodes both point to the DSS power-domain) the
ti-sci driver will create two power domains, both with the same ID, and
that will cause problems as one of the power domains will hide the other
one.
Fix this checking if a PD with the ID has already been created, and only
create a PD for new IDs.
Fixes: efa5c01cd7ee ("soc: ti: ti_sci_pm_domains: switch to use multiple genpds
instead of one")
Signed-off-by: Tomi Valkeinen
---
drivers/pmdomain/ti/ti_sci_pm_domains.c | 20 +++-
1 file changed, 19 insertions(+), 1 deletion(-)
diff --git a/drivers/pmdomain/ti/ti_sci_pm_domains.c
b/drivers/pmdomain/ti/ti_sci_pm_domains.c
index 9dddf227a3a6..1510d5ddae3d 100644
--- a/drivers/pmdomain/ti/ti_sci_pm_domains.c
+++ b/drivers/pmdomain/ti/ti_sci_pm_domains.c
@@ -114,6 +114,18 @@ static const struct of_device_id
ti_sci_pm_domain_matches[] = {
};
MODULE_DEVICE_TABLE(of, ti_sci_pm_domain_matches);
+static bool ti_sci_pm_idx_exists(struct ti_sci_genpd_provider *pd_provider,
u32 idx)
+{
+ struct ti_sci_pm_domain *pd;
+
+ list_for_each_entry(pd, _provider->pd_list, node) {
+ if (pd->idx == idx)
+ return true;
+ }
+
+ return false;
+}
+
static int ti_sci_pm_domain_probe(struct platform_device *pdev)
{
struct device *dev = >dev;
@@ -149,8 +161,14 @@ static int ti_sci_pm_domain_probe(struct platform_device
*pdev)
break;
if (args.args_count >= 1 && args.np == dev->of_node) {
- if (args.args[0] > max_id)
+ if (args.args[0] > max_id) {
max_id = args.args[0];
+ } else {
+ if (ti_sci_pm_idx_exists(pd_provider,
args.args[0])) {
+ index++;
+ continue;
+ }
+ }
pd = devm_kzalloc(dev, sizeof(*pd), GFP_KERNEL);
if (!pd) {
--
2.34.1
[PATCH RFC 2/2] pmdomain: ti-sci: Support retaining PD boot time state
Add a new flag, TI_SCI_PD_KEEP_BOOT_STATE, which can be set in the dts
when referring to power domains. When this flag is set, the ti-sci
driver will check if the PD is currently enabled in the HW, and if so,
set the GENPD_FLAG_ALWAYS_ON flag so that the PD will stay enabled.
The main issue I'm trying to solve here is this:
If the Display Subsystem (DSS) has been enabled by the bootloader, the
related PD has also been enabled in the HW. When the tidss driver
probes, the driver framework will automatically enable the PD. While
executing the probe function it is very common for the probe to return
EPROBE_DEFER, and, in rarer cases, an actual error. When this happens
(probe() returns an error), the driver framework will automatically
disable the related PD.
Powering off the PD while the DSS is enabled and displaying a picture
will cause the DSS HW to enter a bad state, from which (afaik) it can't
be woken up except with full power-cycle. Trying to access the DSS in
this state (e.g. when retrying the probe) will usually cause the board
to hang sooner or later.
Even if we wouldn't have this board-hangs issue, it's nice to be able to
keep the DSS PD enabled: we want to keep the DSS enabled when the
bootloader has enabled the screen. If, instead, we disable the PD at the
first EPROBE_DEFER, the screen will (probably) go black.
Another option here would perhaps be to change the driver framework
(drivers/base/platform.c) which attaches and detaches the PD, and make
it somehow optional, allowing the driver the manage the PD. That option
has two downsides: 1) the driver _has_ to manage the PD, which would
rule out the use of simplefb and simpledrm, and 2) it would leave the PD
in off state from Linux's perspective until a driver enables the PD, and
that might mean that the PD gets actually disabled as part of normal
system wide power management (disabling unused resources).
Yet another option would be to do this outside the ti_sci_pm_domains
driver: a piece of code that would somehow be ran after the
ti_sci_pm_domains driver has probed (so that we have the PDs), but
before tidss/simplefb/simpledrm probes. The problem here is the
"somehow" part. Also, this would partly have the same issue 2) as
mentioned above.
TODO: If this approach is ok, sci-pm-domain.yaml needs to be extended.
Also, it sounds a bit like the cell value is not a bit-mask, so maybe
adding TI_SCI_PD_KEEP_BOOT_STATE flag this way is not fine.
Signed-off-by: Tomi Valkeinen
---
drivers/pmdomain/ti/ti_sci_pm_domains.c| 27 +--
include/dt-bindings/soc/ti,sci_pm_domain.h | 1 +
2 files changed, 26 insertions(+), 2 deletions(-)
diff --git a/drivers/pmdomain/ti/ti_sci_pm_domains.c
b/drivers/pmdomain/ti/ti_sci_pm_domains.c
index 1510d5ddae3d..b71b390aaa39 100644
--- a/drivers/pmdomain/ti/ti_sci_pm_domains.c
+++ b/drivers/pmdomain/ti/ti_sci_pm_domains.c
@@ -103,7 +103,7 @@ static struct generic_pm_domain *ti_sci_pd_xlate(
return ERR_PTR(-ENOENT);
genpd_to_ti_sci_pd(genpd_data->domains[idx])->exclusive =
- genpdspec->args[1];
+ genpdspec->args[1] & TI_SCI_PD_EXCLUSIVE;
return genpd_data->domains[idx];
}
@@ -161,6 +161,8 @@ static int ti_sci_pm_domain_probe(struct platform_device
*pdev)
break;
if (args.args_count >= 1 && args.np == dev->of_node) {
+ bool is_on = false;
+
if (args.args[0] > max_id) {
max_id = args.args[0];
} else {
@@ -189,7 +191,28 @@ static int ti_sci_pm_domain_probe(struct platform_device
*pdev)
pd->idx = args.args[0];
pd->parent = pd_provider;
- pm_genpd_init(>pd, NULL, true);
+ /*
+* If TI_SCI_PD_KEEP_BOOT_STATE is set and the
+* PD has been enabled by the bootloader, set
+* the PD to GENPD_FLAG_ALWAYS_ON. This will
+* make sure the PD stays enabled until a driver
+* takes over and clears the
GENPD_FLAG_ALWAYS_ON
+* flag.
+*/
+ if (args.args_count > 1 &&
+ args.args[1] & TI_SCI_PD_KEEP_BOOT_STATE) {
+ /*
+* We ignore any error here, and in case
+* of error just assume the PD is off.
+*/
+
pd_provider->ti_sci->ops.dev_ops.is_on(pd_provider
Re: [PATCH 12/21] drm/tilcdc: Allow build without __iowmb()
On 10/04/2024 20:04, Ville Syrjälä wrote: On Wed, Apr 10, 2024 at 06:25:17PM +0300, Ville Syrjälä wrote: On Wed, Apr 10, 2024 at 12:06:29PM +0300, Tomi Valkeinen wrote: On 08/04/2024 20:04, Ville Syrjala wrote: From: Ville Syrjälä __iowmb() isn't available on most architectures. Make its use optional so that the driver can be built on other architectures with COMPILE_TEST=y. Cc: Jyri Sarha Cc: Tomi Valkeinen Signed-off-by: Ville Syrjälä --- drivers/gpu/drm/tilcdc/tilcdc_regs.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/gpu/drm/tilcdc/tilcdc_regs.h b/drivers/gpu/drm/tilcdc/tilcdc_regs.h index f90e2dc3457c..44e4ada30fba 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_regs.h +++ b/drivers/gpu/drm/tilcdc/tilcdc_regs.h @@ -125,7 +125,9 @@ static inline void tilcdc_write64(struct drm_device *dev, u32 reg, u64 data) #if defined(iowrite64) && !defined(iowrite64_is_nonatomic) iowrite64(data, addr); #else +#ifdef __iowmb __iowmb(); +#endif /* This compiles to strd (=64-bit write) on ARM7 */ *(volatile u64 __force *)addr = __cpu_to_le64(data); #endif As the memory barrier is an important part there, would it be better to ifdef based on COMPILE_TEST, to make it clear why it's being done? I can do that if you prefer. What if someone tries to actually boot a kernel built with COMPILE_TEST=y on a machine with this hardware? Ah, right... #ifndef __iowmb #define __iowmb BUG #endif ? =) Maybe go with the original one, but with a comment like "allow compilation without __iowmb() for COMPILE_TEST" or such. Tomi
Re: [PATCH 12/21] drm/tilcdc: Allow build without __iowmb()
On 10/04/2024 18:25, Ville Syrjälä wrote: On Wed, Apr 10, 2024 at 12:06:29PM +0300, Tomi Valkeinen wrote: On 08/04/2024 20:04, Ville Syrjala wrote: From: Ville Syrjälä __iowmb() isn't available on most architectures. Make its use optional so that the driver can be built on other architectures with COMPILE_TEST=y. Cc: Jyri Sarha Cc: Tomi Valkeinen Signed-off-by: Ville Syrjälä --- drivers/gpu/drm/tilcdc/tilcdc_regs.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/gpu/drm/tilcdc/tilcdc_regs.h b/drivers/gpu/drm/tilcdc/tilcdc_regs.h index f90e2dc3457c..44e4ada30fba 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_regs.h +++ b/drivers/gpu/drm/tilcdc/tilcdc_regs.h @@ -125,7 +125,9 @@ static inline void tilcdc_write64(struct drm_device *dev, u32 reg, u64 data) #if defined(iowrite64) && !defined(iowrite64_is_nonatomic) iowrite64(data, addr); #else +#ifdef __iowmb __iowmb(); +#endif /* This compiles to strd (=64-bit write) on ARM7 */ *(volatile u64 __force *)addr = __cpu_to_le64(data); #endif As the memory barrier is an important part there, would it be better to ifdef based on COMPILE_TEST, to make it clear why it's being done? I can do that if you prefer. I suppose the real question is why iowrite64() doesn't work if a hand rolled version does work? If I recall right, there is (was?) no iowrite64. The bus is 32 bit anyway. But the two 32 bit registers written with the tilcdc_write64() have an annoying HW race, so we tried to find a method of writing them that reduces the chance of race to a minimum. Tomi
Re: [PATCH] drm: tilcdc: don't use devm_pinctrl_get_select_default() in probe
On 22/09/2023 10:37, Wolfram Sang wrote:
Since commit ab78029ecc34 ("drivers/pinctrl: grab default handles from
device core"), we can rely on device core for setting the default pins.
Signed-off-by: Wolfram Sang
---
drivers/gpu/drm/tilcdc/tilcdc_panel.c | 6 --
1 file changed, 6 deletions(-)
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_panel.c
b/drivers/gpu/drm/tilcdc/tilcdc_panel.c
index 9aefd010acde..68093d6b6b16 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_panel.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_panel.c
@@ -6,7 +6,6 @@
#include
#include
-#include
#include
#include
@@ -308,7 +307,6 @@ static int panel_probe(struct platform_device *pdev)
struct backlight_device *backlight;
struct panel_module *panel_mod;
struct tilcdc_module *mod;
- struct pinctrl *pinctrl;
int ret;
/* bail out early if no DT data: */
@@ -342,10 +340,6 @@ static int panel_probe(struct platform_device *pdev)
tilcdc_module_init(mod, "panel", _module_ops);
- pinctrl = devm_pinctrl_get_select_default(>dev);
- if (IS_ERR(pinctrl))
- dev_warn(>dev, "pins are not configured\n");
-
panel_mod->timings = of_get_display_timings(node);
if (!panel_mod->timings) {
dev_err(>dev, "could not get panel timings\n");
Thanks, applying to drm-misc-next.
Tomi
Re: [PATCH] drm/omap: dmm_tiler: drop driver owner assignment
On 30/03/2024 22:28, Krzysztof Kozlowski wrote:
Core in platform_driver_register() already sets the .owner, so driver
does not need to. Whatever is set here will be anyway overwritten by
main driver calling platform_driver_register().
Signed-off-by: Krzysztof Kozlowski
---
drivers/gpu/drm/omapdrm/omap_dmm_tiler.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
index 9753c1e1f994..1aca3060333e 100644
--- a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
+++ b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
@@ -1212,7 +1212,6 @@ struct platform_driver omap_dmm_driver = {
.probe = omap_dmm_probe,
.remove_new = omap_dmm_remove,
.driver = {
- .owner = THIS_MODULE,
.name = DMM_DRIVER_NAME,
.of_match_table = of_match_ptr(dmm_of_match),
.pm = _dmm_pm_ops,
Thanks, applying to drm-misc-next.
Tomi
Re: [PATCH] drm: xlnx: db: fix a memory leak in probe
On 04/04/2024 10:32, Dan Carpenter wrote:
Free "dp" before returning.
Fixes: be318d01a903 ("drm: xlnx: dp: Reset DisplayPort IP")
Signed-off-by: Dan Carpenter
---
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 5a40aa1d4283..8a15d18a65a6 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
@@ -1716,7 +1716,7 @@ int zynqmp_dp_probe(struct zynqmp_dpsub *dpsub)
ret = zynqmp_dp_reset(dp, true);
if (ret < 0)
- return ret;
+ goto err_free;
ret = zynqmp_dp_reset(dp, false);
if (ret < 0)
Thanks, applying to drm-misc-next.
Tomi
Re: [PATCH 15/21] drm/omap: Allow build with COMPILE_TEST=y
On 08/04/2024 20:04, Ville Syrjala wrote: From: Ville Syrjälä Allow omapdrm to be built with COMPILE_TEST=y for greater coverage. FIXME: Still borked due to ? Cc: Tomi Valkeinen Signed-off-by: Ville Syrjälä --- drivers/gpu/drm/omapdrm/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/omapdrm/Kconfig b/drivers/gpu/drm/omapdrm/Kconfig index 6c49270cb290..85ed92042b74 100644 --- a/drivers/gpu/drm/omapdrm/Kconfig +++ b/drivers/gpu/drm/omapdrm/Kconfig @@ -2,7 +2,7 @@ config DRM_OMAP tristate "OMAP DRM" depends on DRM && OF - depends on ARCH_OMAP2PLUS + depends on ARCH_OMAP2PLUS || COMPILE_TEST select DRM_KMS_HELPER select FB_DMAMEM_HELPERS_DEFERRED if DRM_FBDEV_EMULATION select VIDEOMODE_HELPERS Reviewed-by: Tomi Valkeinen Tomi
Re: [PATCH 14/21] drm/omap: Open code phys_to_page()
On 08/04/2024 20:04, Ville Syrjala wrote:
From: Ville Syrjälä
phys_to_page() is not available on most architectures.
Just open code it like msm does. Allows COMPILE_TEST=y
builds of omapdrm on other architectures.
Cc: Tomi Valkeinen
Signed-off-by: Ville Syrjälä
---
drivers/gpu/drm/omapdrm/omap_gem.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c
b/drivers/gpu/drm/omapdrm/omap_gem.c
index 3421e8389222..c4454e7f1c94 100644
--- a/drivers/gpu/drm/omapdrm/omap_gem.c
+++ b/drivers/gpu/drm/omapdrm/omap_gem.c
@@ -1022,8 +1022,8 @@ struct sg_table *omap_gem_get_sg(struct drm_gem_object
*obj,
if (addr) {
for_each_sg(sgt->sgl, sg, count, i) {
- sg_set_page(sg, phys_to_page(addr), len,
- offset_in_page(addr));
+ sg_set_page(sg, pfn_to_page(__phys_to_pfn(addr)),
+ len, offset_in_page(addr));
sg_dma_address(sg) = addr;
sg_dma_len(sg) = len;
Reviewed-by: Tomi Valkeinen
Tomi
Re: [PATCH 13/21] drm/tilcdc: Allow build with COMPILE_TEST=y
On 08/04/2024 20:04, Ville Syrjala wrote:
From: Ville Syrjälä
Allow tilcdc to be built with COMPILE_TEST=y for greater
coverage. Builds fine on x86/x86_64 at least.
Cc: Jyri Sarha
Cc: Tomi Valkeinen
Signed-off-by: Ville Syrjälä
---
drivers/gpu/drm/tilcdc/Kconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/tilcdc/Kconfig b/drivers/gpu/drm/tilcdc/Kconfig
index d3bd2d7a181e..1897ef91c70b 100644
--- a/drivers/gpu/drm/tilcdc/Kconfig
+++ b/drivers/gpu/drm/tilcdc/Kconfig
@@ -1,7 +1,7 @@
# SPDX-License-Identifier: GPL-2.0-only
config DRM_TILCDC
tristate "DRM Support for TI LCDC Display Controller"
- depends on DRM && OF && ARM
+ depends on DRM && OF && (ARM || COMPILE_TEST)
select DRM_KMS_HELPER
select DRM_GEM_DMA_HELPER
select DRM_BRIDGE
Reviewed-by: Tomi Valkeinen
Tomi
Re: [PATCH 12/21] drm/tilcdc: Allow build without __iowmb()
On 08/04/2024 20:04, Ville Syrjala wrote: From: Ville Syrjälä __iowmb() isn't available on most architectures. Make its use optional so that the driver can be built on other architectures with COMPILE_TEST=y. Cc: Jyri Sarha Cc: Tomi Valkeinen Signed-off-by: Ville Syrjälä --- drivers/gpu/drm/tilcdc/tilcdc_regs.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/gpu/drm/tilcdc/tilcdc_regs.h b/drivers/gpu/drm/tilcdc/tilcdc_regs.h index f90e2dc3457c..44e4ada30fba 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_regs.h +++ b/drivers/gpu/drm/tilcdc/tilcdc_regs.h @@ -125,7 +125,9 @@ static inline void tilcdc_write64(struct drm_device *dev, u32 reg, u64 data) #if defined(iowrite64) && !defined(iowrite64_is_nonatomic) iowrite64(data, addr); #else +#ifdef __iowmb __iowmb(); +#endif /* This compiles to strd (=64-bit write) on ARM7 */ *(volatile u64 __force *)addr = __cpu_to_le64(data); #endif As the memory barrier is an important part there, would it be better to ifdef based on COMPILE_TEST, to make it clear why it's being done? Tomi
Re: [PATCH v3 6/9] drm: xlnx: zynqmp_dpsub: Set input live format
On 21/03/2024 22:43, Anatoliy Klymenko wrote:
Program live video input format according to selected media bus format.
In the bridge mode of operation, DPSUB is connected to FPGA CRTC which
almost certainly supports a single media bus format as its output. Expect
this to be delivered via the new bridge atomic state. Program DPSUB
registers accordingly. Update zynqmp_disp_layer_set_format() API to fit
both live and non-live layer types.
Signed-off-by: Anatoliy Klymenko
---
drivers/gpu/drm/xlnx/zynqmp_disp.c | 66 +-
drivers/gpu/drm/xlnx/zynqmp_disp.h | 2 +-
drivers/gpu/drm/xlnx/zynqmp_dp.c | 13 +---
drivers/gpu/drm/xlnx/zynqmp_kms.c | 2 +-
4 files changed, 55 insertions(+), 28 deletions(-)
diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c
b/drivers/gpu/drm/xlnx/zynqmp_disp.c
index 0c2b3f4bffa6..a385d22d428e 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
@@ -436,19 +436,28 @@ static void zynqmp_disp_avbuf_set_format(struct
zynqmp_disp *disp,
const struct zynqmp_disp_format *fmt)
{
unsigned int i;
- u32 val;
+ u32 val, reg;
- val = zynqmp_disp_avbuf_read(disp, ZYNQMP_DISP_AV_BUF_FMT);
- val &= zynqmp_disp_layer_is_video(layer)
- ? ~ZYNQMP_DISP_AV_BUF_FMT_NL_VID_MASK
- : ~ZYNQMP_DISP_AV_BUF_FMT_NL_GFX_MASK;
- val |= fmt->buf_fmt;
- zynqmp_disp_avbuf_write(disp, ZYNQMP_DISP_AV_BUF_FMT, val);
+ layer->disp_fmt = fmt;
+ if (layer->mode == ZYNQMP_DPSUB_LAYER_NONLIVE) {
+ reg = ZYNQMP_DISP_AV_BUF_FMT;
+ val = zynqmp_disp_avbuf_read(disp, ZYNQMP_DISP_AV_BUF_FMT);
+ val &= zynqmp_disp_layer_is_video(layer)
+ ? ~ZYNQMP_DISP_AV_BUF_FMT_NL_VID_MASK
+ : ~ZYNQMP_DISP_AV_BUF_FMT_NL_GFX_MASK;
+ val |= fmt->buf_fmt;
+ } else {
+ reg = zynqmp_disp_layer_is_video(layer)
+ ? ZYNQMP_DISP_AV_BUF_LIVE_VID_CONFIG
+ : ZYNQMP_DISP_AV_BUF_LIVE_GFX_CONFIG;
+ val = fmt->buf_fmt;
+ }
+ zynqmp_disp_avbuf_write(disp, reg, val);
Just write the registers inside the above if-else blocks.
for (i = 0; i < ZYNQMP_DISP_AV_BUF_NUM_SF; i++) {
- unsigned int reg = zynqmp_disp_layer_is_video(layer)
-? ZYNQMP_DISP_AV_BUF_VID_COMP_SF(i)
-: ZYNQMP_DISP_AV_BUF_GFX_COMP_SF(i);
+ reg = zynqmp_disp_layer_is_video(layer)
+ ? ZYNQMP_DISP_AV_BUF_VID_COMP_SF(i)
+ : ZYNQMP_DISP_AV_BUF_GFX_COMP_SF(i);
zynqmp_disp_avbuf_write(disp, reg, fmt->sf[i]);
}
@@ -902,25 +911,33 @@ static void zynqmp_disp_audio_disable(struct zynqmp_disp
*disp)
*/
/**
- * zynqmp_disp_layer_find_format - Find format information for a DRM format
+ * zynqmp_disp_layer_find_format - Find format information for a DRM or media
+ * bus format
* @layer: The layer
- * @drm_fmt: DRM format to search
+ * @drm_or_bus_format: DRM or media bus format
*
* Search display subsystem format information corresponding to the given DRM
- * format @drm_fmt for the @layer, and return a pointer to the format
- * descriptor.
+ * or media bus format @drm_or_bus_format for the @layer, and return a pointer
+ * to the format descriptor. Search key choice depends on @layer mode, for live
+ * layers search is done by zynqmp_disp_format.bus_fmt, and for non-live layers
+ * zynqmp_disp_format.drm_fmt is used.
Here also I recommend creating separate funcs for the fourcc and mbus
versions. They are different types, even if they happen to fit into u32.
*
* Return: A pointer to the format descriptor if found, NULL otherwise
*/
static const struct zynqmp_disp_format *
zynqmp_disp_layer_find_format(struct zynqmp_disp_layer *layer,
- u32 drm_fmt)
+ u32 drm_or_bus_format)
{
unsigned int i;
+ const struct zynqmp_disp_format *disp_format;
for (i = 0; i < layer->info->num_formats; i++) {
- if (layer->info->formats[i].drm_fmt == drm_fmt)
- return >info->formats[i];
+ disp_format = >info->formats[i];
+ if ((layer->mode == ZYNQMP_DPSUB_LAYER_LIVE &&
+disp_format->bus_fmt == drm_or_bus_format) ||
+ (layer->mode == ZYNQMP_DPSUB_LAYER_NONLIVE &&
+disp_format->drm_fmt == drm_or_bus_format))
+ return disp_format;
}
return NULL;
@@ -992,20 +1009,25 @@ void zynqmp_disp_layer_disable(struct zynqmp_disp_layer
*layer)
/**
* zynqmp_disp_layer_set_format - Set the layer format
* @layer: The layer
- * @info: The format info
+ * @drm_or_bus_format: DRM or media bus format
*
* Set the format for @layer to @info. The layer must be
Re: [PATCH v3 5/9] drm: xlnx: zynqmp_dpsub: Minimize usage of global flag
On 21/03/2024 22:43, Anatoliy Klymenko wrote:
Avoid usage of global zynqmp_dpsub.dma_enabled flag in DPSUB layer
context. This flag signals whether the driver runs in DRM CRTC or DRM
bridge mode, assuming that all display layers share the same live or
non-live mode of operation. Using per-layer mode instead of global flag
will simplify future support of the hybrid scenario.
Remove redundant checks in DMA request/release contexts as
zynqmp_disp_layer.info is well-defined for all layer types, including the
correct number of DMA channels required for each particular layer.
Signed-off-by: Anatoliy Klymenko
---
drivers/gpu/drm/xlnx/zynqmp_disp.c | 12 +++-
1 file changed, 3 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c
b/drivers/gpu/drm/xlnx/zynqmp_disp.c
index abdc3971b193..0c2b3f4bffa6 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
@@ -980,7 +980,7 @@ void zynqmp_disp_layer_disable(struct zynqmp_disp_layer
*layer)
{
unsigned int i;
- if (layer->disp->dpsub->dma_enabled) {
+ if (layer->mode == ZYNQMP_DPSUB_LAYER_NONLIVE) {
for (i = 0; i < layer->drm_fmt->num_planes; i++)
dmaengine_terminate_sync(layer->dmas[i].chan);
}
@@ -1006,7 +1006,7 @@ void zynqmp_disp_layer_set_format(struct
zynqmp_disp_layer *layer,
zynqmp_disp_avbuf_set_format(layer->disp, layer, layer->disp_fmt);
- if (!layer->disp->dpsub->dma_enabled)
+ if (layer->mode == ZYNQMP_DPSUB_LAYER_LIVE)
return;
/*
@@ -1044,7 +1044,7 @@ int zynqmp_disp_layer_update(struct zynqmp_disp_layer
*layer,
const struct drm_format_info *info = layer->drm_fmt;
unsigned int i;
- if (!layer->disp->dpsub->dma_enabled)
+ if (layer->mode == ZYNQMP_DPSUB_LAYER_LIVE)
return 0;
for (i = 0; i < info->num_planes; i++) {
@@ -1094,9 +1094,6 @@ static void zynqmp_disp_layer_release_dma(struct
zynqmp_disp *disp,
{
unsigned int i;
- if (!layer->info || !disp->dpsub->dma_enabled)
- return;
-
for (i = 0; i < layer->info->num_channels; i++) {
struct zynqmp_disp_layer_dma *dma = >dmas[i];
@@ -1137,9 +1134,6 @@ static int zynqmp_disp_layer_request_dma(struct zynqmp_disp *disp,
unsigned int i;
int ret;
- if (!disp->dpsub->dma_enabled)
- return 0;
-
for (i = 0; i < layer->info->num_channels; i++) {
struct zynqmp_disp_layer_dma *dma = >dmas[i];
char dma_channel_name[16];
Reviewed-by: Tomi Valkeinen
Tomi
Re: [PATCH v3 4/9] drm: xlnx: zynqmp_dpsub: Anounce supported input formats
On 21/03/2024 22:43, Anatoliy Klymenko wrote:
DPSUB in bridge mode supports multiple input media bus formats.
Announce the list of supported input media bus formats via
drm_bridge.atomic_get_input_bus_fmts callback.
Introduce a set of live input formats, supported by DPSUB.
Rename zynqmp_disp_layer_drm_formats() to zynqmp_disp_layer_formats() to
reflect semantics for both live and non-live layer format lists.
Reviewed-by: Laurent Pinchart
Signed-off-by: Anatoliy Klymenko
---
drivers/gpu/drm/xlnx/zynqmp_disp.c | 76 +-
drivers/gpu/drm/xlnx/zynqmp_disp.h | 4 +-
drivers/gpu/drm/xlnx/zynqmp_dp.c | 31
drivers/gpu/drm/xlnx/zynqmp_kms.c | 2 +-
4 files changed, 101 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c
b/drivers/gpu/drm/xlnx/zynqmp_disp.c
index e6d26ef60e89..abdc3971b193 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
@@ -18,6 +18,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -77,12 +78,14 @@ enum zynqmp_dpsub_layer_mode {
/**
* struct zynqmp_disp_format - Display subsystem format information
* @drm_fmt: DRM format (4CC)
+ * @bus_fmt: Media bus format
* @buf_fmt: AV buffer format
* @swap: Flag to swap R & B for RGB formats, and U & V for YUV formats
* @sf: Scaling factors for color components
*/
struct zynqmp_disp_format {
u32 drm_fmt;
+ u32 bus_fmt;
u32 buf_fmt;
bool swap;
const u32 *sf;
@@ -182,6 +185,12 @@ static const u32 scaling_factors_565[] = {
ZYNQMP_DISP_AV_BUF_5BIT_SF,
};
+static const u32 scaling_factors_666[] = {
+ ZYNQMP_DISP_AV_BUF_6BIT_SF,
+ ZYNQMP_DISP_AV_BUF_6BIT_SF,
+ ZYNQMP_DISP_AV_BUF_6BIT_SF,
+};
+
static const u32 scaling_factors_888[] = {
ZYNQMP_DISP_AV_BUF_8BIT_SF,
ZYNQMP_DISP_AV_BUF_8BIT_SF,
@@ -364,6 +373,41 @@ static const struct zynqmp_disp_format avbuf_gfx_fmts[] = {
},
};
+/* List of live video layer formats */
+static const struct zynqmp_disp_format avbuf_live_fmts[] = {
+ {
+ .drm_fmt= DRM_FORMAT_RGB565,
+ .bus_fmt= MEDIA_BUS_FMT_RGB666_1X18,
+ .buf_fmt= ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_6 |
+ ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_RGB,
+ .sf = scaling_factors_666,
+ }, {
+ .drm_fmt= DRM_FORMAT_RGB888,
+ .bus_fmt= MEDIA_BUS_FMT_RGB888_1X24,
+ .buf_fmt= ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_8 |
+ ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_RGB,
+ .sf = scaling_factors_888,
+ }, {
+ .drm_fmt= DRM_FORMAT_YUV422,
+ .bus_fmt= MEDIA_BUS_FMT_UYVY8_1X16,
+ .buf_fmt= ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_8 |
+ ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV422,
+ .sf = scaling_factors_888,
+ }, {
+ .drm_fmt= DRM_FORMAT_YUV444,
+ .bus_fmt= MEDIA_BUS_FMT_VUY8_1X24,
+ .buf_fmt= ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_8 |
+ ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV444,
+ .sf = scaling_factors_888,
+ }, {
+ .drm_fmt= DRM_FORMAT_P210,
+ .bus_fmt= MEDIA_BUS_FMT_UYVY10_1X20,
+ .buf_fmt= ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_10 |
+ ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV422,
+ .sf = scaling_factors_101010,
+ },
+};
+
static u32 zynqmp_disp_avbuf_read(struct zynqmp_disp *disp, int reg)
{
return readl(disp->avbuf.base + reg);
@@ -883,16 +927,17 @@ zynqmp_disp_layer_find_format(struct zynqmp_disp_layer
*layer,
}
/**
- * zynqmp_disp_layer_drm_formats - Return the DRM formats supported by the
layer
+ * zynqmp_disp_layer_formats - Return DRM or media bus formats supported by
+ * the layer
* @layer: The layer
* @num_formats: Pointer to the returned number of formats
*
- * Return: A newly allocated u32 array that stores all the DRM formats
+ * Return: A newly allocated u32 array that stores all DRM or media bus formats
* supported by the layer. The number of formats in the array is returned
* through the num_formats argument.
*/
-u32 *zynqmp_disp_layer_drm_formats(struct zynqmp_disp_layer *layer,
- unsigned int *num_formats)
+u32 *zynqmp_disp_layer_formats(struct zynqmp_disp_layer *layer,
+ unsigned int *num_formats)
{
unsigned int i;
u32 *formats;
@@ -903,7 +948,9 @@ u32 *zynqmp_disp_layer_drm_formats(struct zynqmp_disp_layer
*layer,
return NULL;
Re: [PATCH v3 1/9] drm: xlnx: zynqmp_dpsub: Set layer mode during creation
On 21/03/2024 22:43, Anatoliy Klymenko wrote:
Set layer mode of operation (live or dma-based) during layer creation.
Each DPSUB layer mode of operation is defined by corresponding DT node port
connection, so it is possible to assign it during layer object creation.
Previously it was set in layer enable functions, although it is too late
as setting layer format depends on layer mode, and should be done before
given layer enabled.
Signed-off-by: Anatoliy Klymenko
Reviewed-by: Laurent Pinchart
---
drivers/gpu/drm/xlnx/zynqmp_disp.c | 20
drivers/gpu/drm/xlnx/zynqmp_disp.h | 13 +
drivers/gpu/drm/xlnx/zynqmp_dp.c | 2 +-
drivers/gpu/drm/xlnx/zynqmp_kms.c | 2 +-
4 files changed, 19 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c
b/drivers/gpu/drm/xlnx/zynqmp_disp.c
index 8a39b3accce5..e6d26ef60e89 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
@@ -64,6 +64,16 @@
#define ZYNQMP_DISP_MAX_NUM_SUB_PLANES 3
+/**
+ * enum zynqmp_dpsub_layer_mode - Layer mode
+ * @ZYNQMP_DPSUB_LAYER_NONLIVE: non-live (memory) mode
+ * @ZYNQMP_DPSUB_LAYER_LIVE: live (stream) mode
+ */
+enum zynqmp_dpsub_layer_mode {
+ ZYNQMP_DPSUB_LAYER_NONLIVE,
+ ZYNQMP_DPSUB_LAYER_LIVE,
+};
+
/**
* struct zynqmp_disp_format - Display subsystem format information
* @drm_fmt: DRM format (4CC)
@@ -902,15 +912,12 @@ u32 *zynqmp_disp_layer_drm_formats(struct
zynqmp_disp_layer *layer,
/**
* zynqmp_disp_layer_enable - Enable a layer
* @layer: The layer
- * @mode: Operating mode of layer
*
* Enable the @layer in the audio/video buffer manager and the blender. DMA
* channels are started separately by zynqmp_disp_layer_update().
*/
-void zynqmp_disp_layer_enable(struct zynqmp_disp_layer *layer,
- enum zynqmp_dpsub_layer_mode mode)
+void zynqmp_disp_layer_enable(struct zynqmp_disp_layer *layer)
{
- layer->mode = mode;
zynqmp_disp_avbuf_enable_video(layer->disp, layer);
zynqmp_disp_blend_layer_enable(layer->disp, layer);
}
@@ -1134,6 +1141,11 @@ static int zynqmp_disp_create_layers(struct zynqmp_disp
*disp)
layer->id = i;
layer->disp = disp;
layer->info = _info[i];
+ /* For now assume dpsub works in either live or non-live mode
for both layers.
+* Hybrid mode is not supported yet.
+*/
This comment style is not according to the style guide, and in fact you
fix it in the patch 4. So please fix it here instead.
Tomi
+ layer->mode = disp->dpsub->dma_enabled ?
ZYNQMP_DPSUB_LAYER_NONLIVE
+ :
ZYNQMP_DPSUB_LAYER_LIVE;
ret = zynqmp_disp_layer_request_dma(disp, layer);
if (ret)
diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.h
b/drivers/gpu/drm/xlnx/zynqmp_disp.h
index 123cffac08be..9b8b202224d9 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_disp.h
+++ b/drivers/gpu/drm/xlnx/zynqmp_disp.h
@@ -42,16 +42,6 @@ enum zynqmp_dpsub_layer_id {
ZYNQMP_DPSUB_LAYER_GFX,
};
-/**
- * enum zynqmp_dpsub_layer_mode - Layer mode
- * @ZYNQMP_DPSUB_LAYER_NONLIVE: non-live (memory) mode
- * @ZYNQMP_DPSUB_LAYER_LIVE: live (stream) mode
- */
-enum zynqmp_dpsub_layer_mode {
- ZYNQMP_DPSUB_LAYER_NONLIVE,
- ZYNQMP_DPSUB_LAYER_LIVE,
-};
-
void zynqmp_disp_enable(struct zynqmp_disp *disp);
void zynqmp_disp_disable(struct zynqmp_disp *disp);
int zynqmp_disp_setup_clock(struct zynqmp_disp *disp,
@@ -62,8 +52,7 @@ void zynqmp_disp_blend_set_global_alpha(struct zynqmp_disp
*disp,
u32 *zynqmp_disp_layer_drm_formats(struct zynqmp_disp_layer *layer,
unsigned int *num_formats);
-void zynqmp_disp_layer_enable(struct zynqmp_disp_layer *layer,
- enum zynqmp_dpsub_layer_mode mode);
+void zynqmp_disp_layer_enable(struct zynqmp_disp_layer *layer);
void zynqmp_disp_layer_disable(struct zynqmp_disp_layer *layer);
void zynqmp_disp_layer_set_format(struct zynqmp_disp_layer *layer,
const struct drm_format_info *info);
diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
index 1846c4971fd8..04b6bcac3b07 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
@@ -1295,7 +1295,7 @@ static void zynqmp_dp_disp_enable(struct zynqmp_dp *dp,
/* TODO: Make the format configurable. */
info = drm_format_info(DRM_FORMAT_YUV422);
zynqmp_disp_layer_set_format(layer, info);
- zynqmp_disp_layer_enable(layer, ZYNQMP_DPSUB_LAYER_LIVE);
+ zynqmp_disp_layer_enable(layer);
if (layer_id == ZYNQMP_DPSUB_LAYER_GFX)
zynqmp_disp_blend_set_global_alpha(dp->dpsub->disp, true, 255);
diff --git a/drivers/gpu/drm/xlnx/zynqmp_kms.c
b/drivers/gpu/drm/xlnx/zynqmp_kms.c
index
Re: [PATCH v3 3/9] drm: xlnx: zynqmp_dpsub: Add connected live layer helper
On 21/03/2024 22:43, Anatoliy Klymenko wrote:
Add a helper function capturing the first connected live display layer
discovery logic.
Signed-off-by: Anatoliy Klymenko
---
drivers/gpu/drm/xlnx/zynqmp_dp.c | 37 +++--
1 file changed, 23 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
index 04b6bcac3b07..4faafdd76798 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
@@ -1276,28 +1276,40 @@ static void zynqmp_dp_encoder_mode_set_stream(struct
zynqmp_dp *dp,
* DISP Configuration
*/
+/**
+ * zynqmp_dp_disp_connected_live_layer - Return the first connected live layer
+ * @dp: DisplayPort IP core structure
+ *
+ * Return: The first connected live display layer or NULL if none of the live
+ * layer is connected.
"layers"
Reviewed-by: Tomi Valkeinen
Tomi
+ */
+static struct zynqmp_disp_layer *
+zynqmp_dp_disp_connected_live_layer(struct zynqmp_dp *dp)
+{
+ if (dp->dpsub->connected_ports & BIT(ZYNQMP_DPSUB_PORT_LIVE_VIDEO))
+ return dp->dpsub->layers[ZYNQMP_DPSUB_LAYER_VID];
+ else if (dp->dpsub->connected_ports & BIT(ZYNQMP_DPSUB_PORT_LIVE_GFX))
+ return dp->dpsub->layers[ZYNQMP_DPSUB_LAYER_GFX];
+ else
+ return NULL;
+}
+
static void zynqmp_dp_disp_enable(struct zynqmp_dp *dp,
struct drm_bridge_state *old_bridge_state)
{
- enum zynqmp_dpsub_layer_id layer_id;
struct zynqmp_disp_layer *layer;
const struct drm_format_info *info;
- if (dp->dpsub->connected_ports & BIT(ZYNQMP_DPSUB_PORT_LIVE_VIDEO))
- layer_id = ZYNQMP_DPSUB_LAYER_VID;
- else if (dp->dpsub->connected_ports & BIT(ZYNQMP_DPSUB_PORT_LIVE_GFX))
- layer_id = ZYNQMP_DPSUB_LAYER_GFX;
- else
+ layer = zynqmp_dp_disp_connected_live_layer(dp);
+ if (!layer)
return;
- layer = dp->dpsub->layers[layer_id];
-
/* TODO: Make the format configurable. */
info = drm_format_info(DRM_FORMAT_YUV422);
zynqmp_disp_layer_set_format(layer, info);
zynqmp_disp_layer_enable(layer);
- if (layer_id == ZYNQMP_DPSUB_LAYER_GFX)
+ if (layer == dp->dpsub->layers[ZYNQMP_DPSUB_LAYER_GFX])
zynqmp_disp_blend_set_global_alpha(dp->dpsub->disp, true, 255);
else
zynqmp_disp_blend_set_global_alpha(dp->dpsub->disp, false, 0);
@@ -1310,11 +1322,8 @@ static void zynqmp_dp_disp_disable(struct zynqmp_dp *dp,
{
struct zynqmp_disp_layer *layer;
- if (dp->dpsub->connected_ports & BIT(ZYNQMP_DPSUB_PORT_LIVE_VIDEO))
- layer = dp->dpsub->layers[ZYNQMP_DPSUB_LAYER_VID];
- else if (dp->dpsub->connected_ports & BIT(ZYNQMP_DPSUB_PORT_LIVE_GFX))
- layer = dp->dpsub->layers[ZYNQMP_DPSUB_LAYER_GFX];
- else
+ layer = zynqmp_dp_disp_connected_live_layer(dp);
+ if (!layer)
return;
zynqmp_disp_disable(dp->dpsub->disp);
Re: [PATCH v3 2/9] drm: xlnx: zynqmp_dpsub: Update live format defines
On 21/03/2024 22:43, Anatoliy Klymenko wrote: Update live format defines to match DPSUB AV_BUF_LIVE_VID_CONFIG register layout. I think this description needs a bit more. Mention that the defines are not currently used, so we can change them like this without any other change. Tomi Reviewed-by: Laurent Pinchart Signed-off-by: Anatoliy Klymenko --- drivers/gpu/drm/xlnx/zynqmp_disp_regs.h | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h b/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h index f92a006d5070..fa3935384834 100644 --- a/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h +++ b/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h @@ -165,10 +165,10 @@ #define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_10 0x2 #define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_12 0x3 #define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_MASK GENMASK(2, 0) -#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_RGB 0x0 -#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV444 0x1 -#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV422 0x2 -#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YONLY 0x3 +#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_RGB (0x0 << 4) +#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV444 (0x1 << 4) +#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV422 (0x2 << 4) +#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YONLY (0x3 << 4) #define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_MASK GENMASK(5, 4) #define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_CB_FIRST BIT(8) #define ZYNQMP_DISP_AV_BUF_PALETTE_MEMORY 0x400
Re: [PATCH v3 1/9] drm: xlnx: zynqmp_dpsub: Set layer mode during creation
On 21/03/2024 22:43, Anatoliy Klymenko wrote:
Set layer mode of operation (live or dma-based) during layer creation.
Each DPSUB layer mode of operation is defined by corresponding DT node port
connection, so it is possible to assign it during layer object creation.
Previously it was set in layer enable functions, although it is too late
as setting layer format depends on layer mode, and should be done before
given layer enabled.
Signed-off-by: Anatoliy Klymenko
Reviewed-by: Laurent Pinchart
---
drivers/gpu/drm/xlnx/zynqmp_disp.c | 20
drivers/gpu/drm/xlnx/zynqmp_disp.h | 13 +
drivers/gpu/drm/xlnx/zynqmp_dp.c | 2 +-
drivers/gpu/drm/xlnx/zynqmp_kms.c | 2 +-
4 files changed, 19 insertions(+), 18 deletions(-)
Reviewed-by: Tomi Valkeinen
Tomi
diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c
b/drivers/gpu/drm/xlnx/zynqmp_disp.c
index 8a39b3accce5..e6d26ef60e89 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
@@ -64,6 +64,16 @@
#define ZYNQMP_DISP_MAX_NUM_SUB_PLANES 3
+/**
+ * enum zynqmp_dpsub_layer_mode - Layer mode
+ * @ZYNQMP_DPSUB_LAYER_NONLIVE: non-live (memory) mode
+ * @ZYNQMP_DPSUB_LAYER_LIVE: live (stream) mode
+ */
+enum zynqmp_dpsub_layer_mode {
+ ZYNQMP_DPSUB_LAYER_NONLIVE,
+ ZYNQMP_DPSUB_LAYER_LIVE,
+};
+
/**
* struct zynqmp_disp_format - Display subsystem format information
* @drm_fmt: DRM format (4CC)
@@ -902,15 +912,12 @@ u32 *zynqmp_disp_layer_drm_formats(struct
zynqmp_disp_layer *layer,
/**
* zynqmp_disp_layer_enable - Enable a layer
* @layer: The layer
- * @mode: Operating mode of layer
*
* Enable the @layer in the audio/video buffer manager and the blender. DMA
* channels are started separately by zynqmp_disp_layer_update().
*/
-void zynqmp_disp_layer_enable(struct zynqmp_disp_layer *layer,
- enum zynqmp_dpsub_layer_mode mode)
+void zynqmp_disp_layer_enable(struct zynqmp_disp_layer *layer)
{
- layer->mode = mode;
zynqmp_disp_avbuf_enable_video(layer->disp, layer);
zynqmp_disp_blend_layer_enable(layer->disp, layer);
}
@@ -1134,6 +1141,11 @@ static int zynqmp_disp_create_layers(struct zynqmp_disp
*disp)
layer->id = i;
layer->disp = disp;
layer->info = _info[i];
+ /* For now assume dpsub works in either live or non-live mode
for both layers.
+* Hybrid mode is not supported yet.
+*/
+ layer->mode = disp->dpsub->dma_enabled ?
ZYNQMP_DPSUB_LAYER_NONLIVE
+ :
ZYNQMP_DPSUB_LAYER_LIVE;
ret = zynqmp_disp_layer_request_dma(disp, layer);
if (ret)
diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.h
b/drivers/gpu/drm/xlnx/zynqmp_disp.h
index 123cffac08be..9b8b202224d9 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_disp.h
+++ b/drivers/gpu/drm/xlnx/zynqmp_disp.h
@@ -42,16 +42,6 @@ enum zynqmp_dpsub_layer_id {
ZYNQMP_DPSUB_LAYER_GFX,
};
-/**
- * enum zynqmp_dpsub_layer_mode - Layer mode
- * @ZYNQMP_DPSUB_LAYER_NONLIVE: non-live (memory) mode
- * @ZYNQMP_DPSUB_LAYER_LIVE: live (stream) mode
- */
-enum zynqmp_dpsub_layer_mode {
- ZYNQMP_DPSUB_LAYER_NONLIVE,
- ZYNQMP_DPSUB_LAYER_LIVE,
-};
-
void zynqmp_disp_enable(struct zynqmp_disp *disp);
void zynqmp_disp_disable(struct zynqmp_disp *disp);
int zynqmp_disp_setup_clock(struct zynqmp_disp *disp,
@@ -62,8 +52,7 @@ void zynqmp_disp_blend_set_global_alpha(struct zynqmp_disp
*disp,
u32 *zynqmp_disp_layer_drm_formats(struct zynqmp_disp_layer *layer,
unsigned int *num_formats);
-void zynqmp_disp_layer_enable(struct zynqmp_disp_layer *layer,
- enum zynqmp_dpsub_layer_mode mode);
+void zynqmp_disp_layer_enable(struct zynqmp_disp_layer *layer);
void zynqmp_disp_layer_disable(struct zynqmp_disp_layer *layer);
void zynqmp_disp_layer_set_format(struct zynqmp_disp_layer *layer,
const struct drm_format_info *info);
diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
index 1846c4971fd8..04b6bcac3b07 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
@@ -1295,7 +1295,7 @@ static void zynqmp_dp_disp_enable(struct zynqmp_dp *dp,
/* TODO: Make the format configurable. */
info = drm_format_info(DRM_FORMAT_YUV422);
zynqmp_disp_layer_set_format(layer, info);
- zynqmp_disp_layer_enable(layer, ZYNQMP_DPSUB_LAYER_LIVE);
+ zynqmp_disp_layer_enable(layer);
if (layer_id == ZYNQMP_DPSUB_LAYER_GFX)
zynqmp_disp_blend_set_global_alpha(dp->dpsub->disp, true, 255);
diff --git a/drivers/gpu/drm/xlnx/zynqmp_kms.c
b/drivers/gpu/drm/xlnx/zynqmp_kms.c
index db3bb4afbfc4..43bf416b33d5 100644
--- a/drivers/gpu/drm/xlnx/zynqmp
[PATCH] MAINTAINERS: Add myself as maintainer for Xilinx DRM drivers
Add myself as a co-maintainer for Xilinx DRM drivers to help Laurent. Signed-off-by: Tomi Valkeinen --- MAINTAINERS | 1 + 1 file changed, 1 insertion(+) diff --git a/MAINTAINERS b/MAINTAINERS index 1aabf1c15bb3..79ef5a6bf21b 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -7306,6 +7306,7 @@ F:drivers/gpu/drm/xen/ DRM DRIVERS FOR XILINX M: Laurent Pinchart +M: Tomi Valkeinen L: dri-devel@lists.freedesktop.org S: Maintained T: git git://anongit.freedesktop.org/drm/drm-misc --- base-commit: e8f897f4afef0031fe618a8e94127a0934896aba change-id: 20240327-xilinx-maintainer-f6020f6cba4d Best regards, -- Tomi Valkeinen
Re: [PATCH v2 5/8] drm: zynqmp_dp: Don't retrain the link in our IRQ
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, >status);
return IRQ_WAKE_THREAD;
}
return IRQ_HANDLED;
}
zynqmp_dp_thread_handler(...)
{
status = atomic_xchg(>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
Re: [PATCH v2 5/8] drm: zynqmp_dp: Don't retrain the link in our IRQ
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, >status);
return IRQ_WAKE_THREAD;
}
return IRQ_HANDLED;
}
zynqmp_dp_thread_handler(...)
{
status = atomic_xchg(>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
Re: [PATCH] drm: zynqmp_dpsub: Always register bridge
Hi,
On 08/03/2024 22:47, Sean Anderson wrote:
We must always register the DRM bridge, since zynqmp_dp_hpd_work_func
calls drm_bridge_hpd_notify, which in turn expects hpd_mutex to be
initialized. We do this before zynqmp_dpsub_drm_init since that calls
drm_bridge_attach. This fixes the following lockdep warning:
[ 19.217084] [ cut here ]
[ 19.227530] DEBUG_LOCKS_WARN_ON(lock->magic != lock)
[ 19.227768] WARNING: CPU: 0 PID: 140 at kernel/locking/mutex.c:582
__mutex_lock+0x4bc/0x550
[ 19.241696] Modules linked in:
[ 19.244937] CPU: 0 PID: 140 Comm: kworker/0:4 Not tainted 6.6.20+ #96
[ 19.252046] Hardware name: xlnx,zynqmp (DT)
[ 19.256421] Workqueue: events zynqmp_dp_hpd_work_func
[ 19.261795] pstate: 6005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[ 19.269104] pc : __mutex_lock+0x4bc/0x550
[ 19.273364] lr : __mutex_lock+0x4bc/0x550
[ 19.277592] sp : ffc085c5bbe0
[ 19.281066] x29: ffc085c5bbe0 x28: x27: ff88009417f8
[ 19.288624] x26: ff8800941788 x25: ff8800020008 x24: ffc082aa3000
[ 19.296227] x23: ffc080d90e3c x22: 0002 x21:
[ 19.303744] x20: x19: ff88002f5210 x18:
[ 19.311295] x17: 6c707369642e3030 x16: 3030613464662072 x15: 0720072007200720
[ 19.318922] x14: x13: 284e4f5f4e524157 x12: 0001
[ 19.326442] x11: 0001ffc085c5b940 x10: 0001ff88003f388b x9 : 0001ff88003f3888
[ 19.334003] x8 : 0001ff88003f3888 x7 : x6 :
[ 19.341537] x5 : x4 : 1668 x3 :
[ 19.349054] x2 : x1 : x0 : ff88003f3880
[ 19.356581] Call trace:
[ 19.359160] __mutex_lock+0x4bc/0x550
[ 19.363032] mutex_lock_nested+0x24/0x30
[ 19.367187] drm_bridge_hpd_notify+0x2c/0x6c
[ 19.371698] zynqmp_dp_hpd_work_func+0x44/0x54
[ 19.376364] process_one_work+0x3ac/0x988
[ 19.380660] worker_thread+0x398/0x694
[ 19.384736] kthread+0x1bc/0x1c0
[ 19.388241] ret_from_fork+0x10/0x20
[ 19.392031] irq event stamp: 183
[ 19.395450] hardirqs last enabled at (183): []
finish_task_switch.isra.0+0xa8/0x2d4
[ 19.405140] hardirqs last disabled at (182): []
__schedule+0x714/0xd04
[ 19.413612] softirqs last enabled at (114): []
srcu_invoke_callbacks+0x158/0x23c
[ 19.423128] softirqs last disabled at (110): []
srcu_invoke_callbacks+0x158/0x23c
[ 19.432614] ---[ end trace ]---
Fixes: eb2d64bfcc17 ("drm: xlnx: zynqmp_dpsub: Report HPD through the bridge")
Signed-off-by: Sean Anderson
---
drivers/gpu/drm/xlnx/zynqmp_dpsub.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
b/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
index 88eb33acd5f0..639fff2c693f 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
@@ -256,12 +256,11 @@ static int zynqmp_dpsub_probe(struct platform_device
*pdev)
if (ret)
goto err_dp;
+ drm_bridge_add(dpsub->bridge);
if (dpsub->dma_enabled) {
ret = zynqmp_dpsub_drm_init(dpsub);
if (ret)
goto err_disp;
- } else {
- drm_bridge_add(dpsub->bridge);
}
dev_info(>dev, "ZynqMP DisplayPort Subsystem driver probed");
@@ -288,9 +287,8 @@ static void zynqmp_dpsub_remove(struct platform_device
*pdev)
if (dpsub->drm)
zynqmp_dpsub_drm_cleanup(dpsub);
- else
- drm_bridge_remove(dpsub->bridge);
+ drm_bridge_remove(dpsub->bridge);
zynqmp_disp_remove(dpsub);
zynqmp_dp_remove(dpsub);
I sent a similar patch:
https://lore.kernel.org/all/20240312-xilinx-dp-lock-fix-v1-1-1698f9f03...@ideasonboard.com/
I have the drm_bridge_add() call in zynqmp_dp_probe(), as that's where
the bridge is set up, so it felt like a logical place. You add it later,
just before the bridge is used the first time.
I like mine a bit more as it has all the bridge code in the same place,
but I also wonder if there might be some risks in adding the bridge
early (before zynqmp_disp_probe()), although I can't see any issue right
away...
In any case, as this works for me too:
Reviewed-by: Tomi Valkeinen
Tomi
Re: [PATCH v2 1/8] drm: xlnx: Fix kerneldoc
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
---
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
Re: [PATCH v2 5/8] drm: zynqmp_dp: Don't retrain the link in our IRQ
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 --- 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 7h[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 tha
Re: [PATCH v2 5/8] drm: zynqmp_dp: Don't retrain the link in our IRQ
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 --- 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_STATE0ro0x0Contains 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 7h[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 th
Re: [PATCH v2 5/8] drm: zynqmp_dp: Don't retrain the link in our IRQ
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 --- 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
Re: [PATCH v2 8/8] drm: zynqmp_dp: Add debugfs interface for compliance testing
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(>lock); err = drm_dp_dpcd_read(>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
Re: [PATCH v2 2/4] dt-bindings: display/xlnx/zynqmp-dpsub: Add audio DMAs
On 20/03/2024 17:37, Rob Herring wrote: On Tue, Mar 19, 2024 at 10:22:37AM +0200, Tomi Valkeinen wrote: The DP subsystem for ZynqMP support audio via two channels, and the DP DMA has dma-engines for those channels. For some reason the DT binding has not specified those channels, even if the picture included in xlnx,zynqmp-dpsub.yaml shows "2 x aud" DMAs. New required entries is an ABI change. This message kind of indicates it was a mistake, but should be a lot more explicit. Are things broken without the entries? Need 'Fixes'? I'll improve the desc for the next version. So, yes, it's an ABI change, and as far as I can guess (I can't figure out any other reason), the audio DMAs were left out by mistake or misunderstanding. The Linux driver has not supported audio, so this has not been an issue and nothing is broken. Now that this series adds the audio support, I had to add the audio DMAs. I considered making the DMAs optional in the DT, but that doesn't sound right, even if that would keep the ABI compatibility (wouldn't it?). The driver I add in this series does consider the audio DMAs as optional, though. If they're not present, the driver will continue without audio support. So, strictly speaking I think this is a fix to the original commit that adds the DT node, but as the driver using the audio DMAs comes in only now, I think there's no need for the 'Fixes' and backporting. I'm happy to change the approach if you think some other way is better. Tomi Add the two audio DMAs to the binding. Signed-off-by: Tomi Valkeinen --- .../devicetree/bindings/display/xlnx/xlnx,zynqmp-dpsub.yaml| 10 -- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/Documentation/devicetree/bindings/display/xlnx/xlnx,zynqmp-dpsub.yaml b/Documentation/devicetree/bindings/display/xlnx/xlnx,zynqmp-dpsub.yaml index 554f9d5809d4..6b754d4f260e 100644 --- a/Documentation/devicetree/bindings/display/xlnx/xlnx,zynqmp-dpsub.yaml +++ b/Documentation/devicetree/bindings/display/xlnx/xlnx,zynqmp-dpsub.yaml @@ -100,12 +100,16 @@ properties: - description: Video layer, plane 1 (U/V or U) - description: Video layer, plane 2 (V) - description: Graphics layer + - description: Audio channel 0 + - description: Audio channel 1 dma-names: items: - const: vid0 - const: vid1 - const: vid2 - const: gfx0 + - const: aud0 + - const: aud1 phys: description: PHYs for the DP data lanes @@ -194,11 +198,13 @@ examples: power-domains = <_dp>; resets = < ZYNQMP_RESET_DP>; -dma-names = "vid0", "vid1", "vid2", "gfx0"; +dma-names = "vid0", "vid1", "vid2", "gfx0", "aud0", "aud1"; dmas = <_dpdma 0>, <_dpdma 1>, <_dpdma 2>, - <_dpdma 3>; + <_dpdma 3>, + <_dpdma 4>, + <_dpdma 5>; phys = < 1 PHY_TYPE_DP 0 3>, < 0 PHY_TYPE_DP 1 3>; -- 2.34.1
Re: [PATCH v2 8/8] drm: zynqmp_dp: Add debugfs interface for compliance testing
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(>lock); err = drm_dp_dpcd_read(>aux, DP_SINK_COUNT, status, DP_LINK_STATUS_SIZE + 2); Why do you need READ/WRITE_ONCE() for ignore_hpd? Tomi
Re: [PATCH v2 7/8] drm: zynqmp_dp: Split off several helper functions
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 Reviewed-by: Laurent Pinchart --- (no changes since v1) drivers/gpu/drm/xlnx/zynqmp_dp.c | 49 ++-- 1 file changed, 34 insertions(+), 15 deletions(-) Reviewed-by: Tomi Valkeinen Tomi
Re: [PATCH v2 5/8] drm: zynqmp_dp: Don't retrain the link in our IRQ
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 --- 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
Re: [PATCH v2 4/8] drm: zynqmp_dp: Rearrange zynqmp_dp for better padding
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
---
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
Re: [PATCH v2 3/8] drm: zynqmp_dp: Adjust training values per-lane
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
---
(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
Re: [PATCH v2 2/8] drm: zynqmp_dp: Downgrade log level for aux retries message
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
Reviewed-by: Laurent Pinchart
---
(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,
>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
Re: [PATCH v2 1/8] drm: xlnx: Fix kerneldoc
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
---
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
[PATCH v2 4/4] drm: xlnx: zynqmp_dpsub: Add DP audio support
Add basic DisplayPort audio support.
Support non-live audio playback from two PCMs (DMA channels), and the
volume control in the audio mixer.
As older dtb files may not have the audio DMA channels defined, the
driver will just mark the audio support as disabled if the audio DMA is
missing, and will continue with only display support.
Note: Reset doesn't seem to work (ZYNQMP_DISP_AUD_SOFT_RESET). If we do
a reset, audio playback won't start again even if, afaics, we do set up
all the necessary registers. So, at the moment, resetting the audio
block in dp_dai_hw_free() is commented out.
Signed-off-by: Tomi Valkeinen
---
drivers/gpu/drm/xlnx/Kconfig| 9 +
drivers/gpu/drm/xlnx/Makefile | 1 +
drivers/gpu/drm/xlnx/zynqmp_disp.c | 50
drivers/gpu/drm/xlnx/zynqmp_disp_regs.h | 7 +-
drivers/gpu/drm/xlnx/zynqmp_dp.c| 54 ++--
drivers/gpu/drm/xlnx/zynqmp_dp.h| 7 +
drivers/gpu/drm/xlnx/zynqmp_dp_audio.c | 461
drivers/gpu/drm/xlnx/zynqmp_dpsub.c | 39 +--
drivers/gpu/drm/xlnx/zynqmp_dpsub.h | 15 +-
9 files changed, 540 insertions(+), 103 deletions(-)
diff --git a/drivers/gpu/drm/xlnx/Kconfig b/drivers/gpu/drm/xlnx/Kconfig
index 68ee897de9d7..d88cfbaf2863 100644
--- a/drivers/gpu/drm/xlnx/Kconfig
+++ b/drivers/gpu/drm/xlnx/Kconfig
@@ -15,3 +15,12 @@ config DRM_ZYNQMP_DPSUB
This is a DRM/KMS driver for ZynqMP DisplayPort controller. Choose
this option if you have a Xilinx ZynqMP SoC with DisplayPort
subsystem.
+
+config DRM_ZYNQMP_DPSUB_AUDIO
+ bool "ZynqMP DisplayPort Audio Support"
+ depends on DRM_ZYNQMP_DPSUB
+ depends on SND && SND_SOC
+ select SND_SOC_GENERIC_DMAENGINE_PCM
+ help
+ Choose this option to enable DisplayPort audio support in the ZynqMP
+ DisplayPort driver.
diff --git a/drivers/gpu/drm/xlnx/Makefile b/drivers/gpu/drm/xlnx/Makefile
index ea1422a39502..ab6e2ffd7e8d 100644
--- a/drivers/gpu/drm/xlnx/Makefile
+++ b/drivers/gpu/drm/xlnx/Makefile
@@ -1,2 +1,3 @@
zynqmp-dpsub-y := zynqmp_disp.o zynqmp_dpsub.o zynqmp_dp.o zynqmp_kms.o
+zynqmp-dpsub-$(CONFIG_DRM_ZYNQMP_DPSUB_AUDIO) += zynqmp_dp_audio.o
obj-$(CONFIG_DRM_ZYNQMP_DPSUB) += zynqmp-dpsub.o
diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c
b/drivers/gpu/drm/xlnx/zynqmp_disp.c
index 407bc07cec69..d2bf0e2d0135 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
@@ -130,7 +130,6 @@ struct zynqmp_disp_layer {
* @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
* @layers: Layers (planes)
*/
struct zynqmp_disp {
@@ -143,9 +142,6 @@ struct zynqmp_disp {
struct {
void __iomem *base;
} avbuf;
- struct {
- void __iomem *base;
- } audio;
struct zynqmp_disp_layer layers[ZYNQMP_DPSUB_NUM_LAYERS];
};
@@ -807,42 +803,6 @@ static void zynqmp_disp_blend_layer_disable(struct
zynqmp_disp *disp,
csc_zero_offsets);
}
-/*
-
- * Audio Mixer
- */
-
-static void zynqmp_disp_audio_write(struct zynqmp_disp *disp, int reg, u32 val)
-{
- writel(val, disp->audio.base + reg);
-}
-
-/**
- * zynqmp_disp_audio_enable - Enable the audio mixer
- * @disp: Display controller
- *
- * Enable the audio mixer by de-asserting the soft reset. The audio state is
set to
- * default values by the reset, set the default mixer volume explicitly.
- */
-static void zynqmp_disp_audio_enable(struct zynqmp_disp *disp)
-{
- /* Clear the audio soft reset register as it's an non-reset flop. */
- zynqmp_disp_audio_write(disp, ZYNQMP_DISP_AUD_SOFT_RESET, 0);
- zynqmp_disp_audio_write(disp, ZYNQMP_DISP_AUD_MIXER_VOLUME,
- ZYNQMP_DISP_AUD_MIXER_VOLUME_NO_SCALE);
-}
-
-/**
- * zynqmp_disp_audio_disable - Disable the audio mixer
- * @disp: Display controller
- *
- * Disable the audio mixer by asserting its soft reset.
- */
-static void zynqmp_disp_audio_disable(struct zynqmp_disp *disp)
-{
- zynqmp_disp_audio_write(disp, ZYNQMP_DISP_AUD_SOFT_RESET,
- ZYNQMP_DISP_AUD_SOFT_RESET_AUD_SRST);
-}
-
/*
-
* ZynqMP Display Layer & DRM Plane
*/
@@ -1169,8 +1129,6 @@ void zynqmp_disp_enable(struct zynqmp_disp *disp)
true);
zynqmp_disp_avbuf_enable_channels(disp);
zynqmp_disp_avbuf_enable_audio(disp);
-
- zynqmp_disp_audio_enable(disp);
}
/**
@@ -1179,8 +1137,6 @@ void zynqmp_disp_enable(struct zynqmp_disp *disp)
*/
void zynqmp_disp_disable
[PATCH v2 3/4] arm64: dts: zynqmp: Add DMA for DP audio
Add the two DMA channels used for the DisplayPort audio to the
zynqmp_dpsub node.
Signed-off-by: Tomi Valkeinen
---
arch/arm64/boot/dts/xilinx/zynqmp.dtsi | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
b/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
index eaba466804bc..811d80cbf4c5 100644
--- a/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
+++ b/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
@@ -1036,11 +1036,14 @@ zynqmp_dpsub: display@fd4a {
"dp_vtc_pixel_clk_in";
power-domains = <_firmware PD_DP>;
resets = <_reset ZYNQMP_RESET_DP>;
- dma-names = "vid0", "vid1", "vid2", "gfx0";
+ dma-names = "vid0", "vid1", "vid2", "gfx0",
+ "aud0", "aud1";
dmas = <_dpdma ZYNQMP_DPDMA_VIDEO0>,
<_dpdma ZYNQMP_DPDMA_VIDEO1>,
<_dpdma ZYNQMP_DPDMA_VIDEO2>,
- <_dpdma ZYNQMP_DPDMA_GRAPHICS>;
+ <_dpdma ZYNQMP_DPDMA_GRAPHICS>,
+ <_dpdma ZYNQMP_DPDMA_AUDIO0>,
+ <_dpdma ZYNQMP_DPDMA_AUDIO1>;
ports {
#address-cells = <1>;
--
2.34.1
[PATCH v2 2/4] dt-bindings: display/xlnx/zynqmp-dpsub: Add audio DMAs
The DP subsystem for ZynqMP support audio via two channels, and the DP DMA has dma-engines for those channels. For some reason the DT binding has not specified those channels, even if the picture included in xlnx,zynqmp-dpsub.yaml shows "2 x aud" DMAs. Add the two audio DMAs to the binding. Signed-off-by: Tomi Valkeinen --- .../devicetree/bindings/display/xlnx/xlnx,zynqmp-dpsub.yaml| 10 -- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/Documentation/devicetree/bindings/display/xlnx/xlnx,zynqmp-dpsub.yaml b/Documentation/devicetree/bindings/display/xlnx/xlnx,zynqmp-dpsub.yaml index 554f9d5809d4..6b754d4f260e 100644 --- a/Documentation/devicetree/bindings/display/xlnx/xlnx,zynqmp-dpsub.yaml +++ b/Documentation/devicetree/bindings/display/xlnx/xlnx,zynqmp-dpsub.yaml @@ -100,12 +100,16 @@ properties: - description: Video layer, plane 1 (U/V or U) - description: Video layer, plane 2 (V) - description: Graphics layer + - description: Audio channel 0 + - description: Audio channel 1 dma-names: items: - const: vid0 - const: vid1 - const: vid2 - const: gfx0 + - const: aud0 + - const: aud1 phys: description: PHYs for the DP data lanes @@ -194,11 +198,13 @@ examples: power-domains = <_dp>; resets = < ZYNQMP_RESET_DP>; -dma-names = "vid0", "vid1", "vid2", "gfx0"; +dma-names = "vid0", "vid1", "vid2", "gfx0", "aud0", "aud1"; dmas = <_dpdma 0>, <_dpdma 1>, <_dpdma 2>, - <_dpdma 3>; + <_dpdma 3>, + <_dpdma 4>, + <_dpdma 5>; phys = < 1 PHY_TYPE_DP 0 3>, < 0 PHY_TYPE_DP 1 3>; -- 2.34.1
[PATCH v2 1/4] ASoC: dmaengine_pcm: Allow passing component name via config
At the moment we cannot instantiate two dmaengine_pcms with the same
parent device, as the components will be named the same, leading to
conflicts.
Add 'name' field to the snd_dmaengine_pcm_config, and use that (if
defined) as the component name instead of deriving the component name
from the device.
Signed-off-by: Tomi Valkeinen
---
include/sound/dmaengine_pcm.h | 2 ++
sound/soc/soc-core.c | 8 +---
sound/soc/soc-generic-dmaengine-pcm.c | 3 +++
3 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/include/sound/dmaengine_pcm.h b/include/sound/dmaengine_pcm.h
index d70c55f17df7..c11aaf8079fb 100644
--- a/include/sound/dmaengine_pcm.h
+++ b/include/sound/dmaengine_pcm.h
@@ -118,6 +118,7 @@ int snd_dmaengine_pcm_refine_runtime_hwparams(
* which do not use devicetree.
* @process: Callback used to apply processing on samples transferred from/to
* user space.
+ * @name: Component name. If null, dev_name will be used.
* @compat_filter_fn: Will be used as the filter function when requesting a
* channel for platforms which do not use devicetree. The filter parameter
* will be the DAI's DMA data.
@@ -143,6 +144,7 @@ struct snd_dmaengine_pcm_config {
int (*process)(struct snd_pcm_substream *substream,
int channel, unsigned long hwoff,
unsigned long bytes);
+ const char *name;
dma_filter_fn compat_filter_fn;
struct device *dma_dev;
const char *chan_names[SNDRV_PCM_STREAM_LAST + 1];
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index 516350533e73..772d67065611 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -2792,10 +2792,12 @@ int snd_soc_component_initialize(struct
snd_soc_component *component,
INIT_LIST_HEAD(>list);
mutex_init(>io_mutex);
- component->name = fmt_single_name(dev, >id);
if (!component->name) {
- dev_err(dev, "ASoC: Failed to allocate name\n");
- return -ENOMEM;
+ component->name = fmt_single_name(dev, >id);
+ if (!component->name) {
+ dev_err(dev, "ASoC: Failed to allocate name\n");
+ return -ENOMEM;
+ }
}
component->dev = dev;
diff --git a/sound/soc/soc-generic-dmaengine-pcm.c
b/sound/soc/soc-generic-dmaengine-pcm.c
index 092ca09f3631..83db1a83d8ba 100644
--- a/sound/soc/soc-generic-dmaengine-pcm.c
+++ b/sound/soc/soc-generic-dmaengine-pcm.c
@@ -441,6 +441,9 @@ int snd_dmaengine_pcm_register(struct device *dev,
pcm->config = config;
pcm->flags = flags;
+ if (config->name)
+ pcm->component.name = config->name;
+
ret = dmaengine_pcm_request_chan_of(pcm, dev, config);
if (ret)
goto err_free_dma;
--
2.34.1
[PATCH v2 0/4] drm: xlnx: zynqmp: Add DP audio support
Add DisplayPort audio support for Xilinx ZynqMP platforms. This depends on patch adding cyclic DMA mode for DPDMA driver: https://lore.kernel.org/all/20240228042124.3074044-3-vishal.sa...@amd.com/ If that patch is missing, starting an audio playback will fail with an ASoC error. The current DT is, for some reason, missing the DMA channels for the audio. This series adds that to the bindings and the dts file, but to support older dtb files without the audio DMA, the driver will not fail if the audio DMA is missing, but will just mark the audio support as disabled. The series also includes an improvement to the soc-generic-dmaengine-pcm.c, required to support two dmaengine_pcms. Signed-off-by: Tomi Valkeinen --- Changes in v2: - Fix a missing double-quote in the DT binding - Link to v1: https://lore.kernel.org/r/20240312-xilinx-dp-audio-v1-0-696c79fac...@ideasonboard.com --- Tomi Valkeinen (4): ASoC: dmaengine_pcm: Allow passing component name via config dt-bindings: display/xlnx/zynqmp-dpsub: Add audio DMAs arm64: dts: zynqmp: Add DMA for DP audio drm: xlnx: zynqmp_dpsub: Add DP audio support .../bindings/display/xlnx/xlnx,zynqmp-dpsub.yaml | 10 +- arch/arm64/boot/dts/xilinx/zynqmp.dtsi | 7 +- drivers/gpu/drm/xlnx/Kconfig | 9 + drivers/gpu/drm/xlnx/Makefile | 1 + drivers/gpu/drm/xlnx/zynqmp_disp.c | 50 --- drivers/gpu/drm/xlnx/zynqmp_disp_regs.h| 7 +- drivers/gpu/drm/xlnx/zynqmp_dp.c | 54 ++- drivers/gpu/drm/xlnx/zynqmp_dp.h | 7 + drivers/gpu/drm/xlnx/zynqmp_dp_audio.c | 461 + drivers/gpu/drm/xlnx/zynqmp_dpsub.c| 39 +- drivers/gpu/drm/xlnx/zynqmp_dpsub.h| 15 +- include/sound/dmaengine_pcm.h | 2 + sound/soc/soc-core.c | 8 +- sound/soc/soc-generic-dmaengine-pcm.c | 3 + 14 files changed, 563 insertions(+), 110 deletions(-) --- base-commit: e8f897f4afef0031fe618a8e94127a0934896aba change-id: 20240312-xilinx-dp-audio-468ad12f9f64 Best regards, -- Tomi Valkeinen
Re: [PATCH v3 0/2] Fixes for omapdrm console
Hi, On 28/02/2024 08:35, Tony Lindgren wrote: Here are two fixes for omapdrm for missing drm_framebuffer_funcs.dirty that needs to be paired with omap_framebuffer_dirty(), and to add FB_GEN_DEFAULT_DEFERRED_DMAMEM_OPS so things behave as earlier with drm_fb_helper_sys_write(). Without these fixes, the console won't update for the command mode displays. And likely mmap() using writes can miss updates as noted by Thomas. Regards, Tony Changes since v2: - Fix cache issue noted by Tomi using custom omap_fbdev_fb_mmap() as suggested by Thomas - Add FB_DMAMEM_HELPERS_DEFERRED Kconfig option and use it for omapdrm as noted by Thomas Changes since v1: - Add FB_GEN_DEFAULT_DEFERRED_DMAMEM_OPS to use with FB_DEFAULT_DEFERRED_OPS as suggested by Thomas Tony Lindgren (2): drm/omapdrm: Fix console by implementing fb_dirty drm/omapdrm: Fix console with deferred ops drivers/gpu/drm/omapdrm/Kconfig | 2 +- drivers/gpu/drm/omapdrm/omap_fbdev.c | 40 +++- drivers/video/fbdev/core/Kconfig | 6 + include/linux/fb.h | 4 +++ 4 files changed, 45 insertions(+), 7 deletions(-) Looks fine to me, I'll apply to drm-misc-next. Tomi
[PATCH 4/4] drm: xlnx: zynqmp_dpsub: Add DP audio support
Add basic DisplayPort audio support.
Support non-live audio playback from two PCMs (DMA channels), and the
volume control in the audio mixer.
As older dtb files may not have the audio DMA channels defined, the
driver will just mark the audio support as disabled if the audio DMA is
missing, and will continue with only display support.
Note: Reset doesn't seem to work (ZYNQMP_DISP_AUD_SOFT_RESET). If we do
a reset, audio playback won't start again even if, afaics, we do set up
all the necessary registers. So, at the moment, resetting the audio
block in dp_dai_hw_free() is commented out.
Signed-off-by: Tomi Valkeinen
---
drivers/gpu/drm/xlnx/Kconfig| 9 +
drivers/gpu/drm/xlnx/Makefile | 1 +
drivers/gpu/drm/xlnx/zynqmp_disp.c | 50
drivers/gpu/drm/xlnx/zynqmp_disp_regs.h | 7 +-
drivers/gpu/drm/xlnx/zynqmp_dp.c| 54 ++--
drivers/gpu/drm/xlnx/zynqmp_dp.h| 7 +
drivers/gpu/drm/xlnx/zynqmp_dp_audio.c | 461
drivers/gpu/drm/xlnx/zynqmp_dpsub.c | 39 +--
drivers/gpu/drm/xlnx/zynqmp_dpsub.h | 15 +-
9 files changed, 540 insertions(+), 103 deletions(-)
diff --git a/drivers/gpu/drm/xlnx/Kconfig b/drivers/gpu/drm/xlnx/Kconfig
index 68ee897de9d7..d88cfbaf2863 100644
--- a/drivers/gpu/drm/xlnx/Kconfig
+++ b/drivers/gpu/drm/xlnx/Kconfig
@@ -15,3 +15,12 @@ config DRM_ZYNQMP_DPSUB
This is a DRM/KMS driver for ZynqMP DisplayPort controller. Choose
this option if you have a Xilinx ZynqMP SoC with DisplayPort
subsystem.
+
+config DRM_ZYNQMP_DPSUB_AUDIO
+ bool "ZynqMP DisplayPort Audio Support"
+ depends on DRM_ZYNQMP_DPSUB
+ depends on SND && SND_SOC
+ select SND_SOC_GENERIC_DMAENGINE_PCM
+ help
+ Choose this option to enable DisplayPort audio support in the ZynqMP
+ DisplayPort driver.
diff --git a/drivers/gpu/drm/xlnx/Makefile b/drivers/gpu/drm/xlnx/Makefile
index ea1422a39502..ab6e2ffd7e8d 100644
--- a/drivers/gpu/drm/xlnx/Makefile
+++ b/drivers/gpu/drm/xlnx/Makefile
@@ -1,2 +1,3 @@
zynqmp-dpsub-y := zynqmp_disp.o zynqmp_dpsub.o zynqmp_dp.o zynqmp_kms.o
+zynqmp-dpsub-$(CONFIG_DRM_ZYNQMP_DPSUB_AUDIO) += zynqmp_dp_audio.o
obj-$(CONFIG_DRM_ZYNQMP_DPSUB) += zynqmp-dpsub.o
diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c
b/drivers/gpu/drm/xlnx/zynqmp_disp.c
index 407bc07cec69..d2bf0e2d0135 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
@@ -130,7 +130,6 @@ struct zynqmp_disp_layer {
* @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
* @layers: Layers (planes)
*/
struct zynqmp_disp {
@@ -143,9 +142,6 @@ struct zynqmp_disp {
struct {
void __iomem *base;
} avbuf;
- struct {
- void __iomem *base;
- } audio;
struct zynqmp_disp_layer layers[ZYNQMP_DPSUB_NUM_LAYERS];
};
@@ -807,42 +803,6 @@ static void zynqmp_disp_blend_layer_disable(struct
zynqmp_disp *disp,
csc_zero_offsets);
}
-/*
-
- * Audio Mixer
- */
-
-static void zynqmp_disp_audio_write(struct zynqmp_disp *disp, int reg, u32 val)
-{
- writel(val, disp->audio.base + reg);
-}
-
-/**
- * zynqmp_disp_audio_enable - Enable the audio mixer
- * @disp: Display controller
- *
- * Enable the audio mixer by de-asserting the soft reset. The audio state is
set to
- * default values by the reset, set the default mixer volume explicitly.
- */
-static void zynqmp_disp_audio_enable(struct zynqmp_disp *disp)
-{
- /* Clear the audio soft reset register as it's an non-reset flop. */
- zynqmp_disp_audio_write(disp, ZYNQMP_DISP_AUD_SOFT_RESET, 0);
- zynqmp_disp_audio_write(disp, ZYNQMP_DISP_AUD_MIXER_VOLUME,
- ZYNQMP_DISP_AUD_MIXER_VOLUME_NO_SCALE);
-}
-
-/**
- * zynqmp_disp_audio_disable - Disable the audio mixer
- * @disp: Display controller
- *
- * Disable the audio mixer by asserting its soft reset.
- */
-static void zynqmp_disp_audio_disable(struct zynqmp_disp *disp)
-{
- zynqmp_disp_audio_write(disp, ZYNQMP_DISP_AUD_SOFT_RESET,
- ZYNQMP_DISP_AUD_SOFT_RESET_AUD_SRST);
-}
-
/*
-
* ZynqMP Display Layer & DRM Plane
*/
@@ -1169,8 +1129,6 @@ void zynqmp_disp_enable(struct zynqmp_disp *disp)
true);
zynqmp_disp_avbuf_enable_channels(disp);
zynqmp_disp_avbuf_enable_audio(disp);
-
- zynqmp_disp_audio_enable(disp);
}
/**
@@ -1179,8 +1137,6 @@ void zynqmp_disp_enable(struct zynqmp_disp *disp)
*/
void zynqmp_disp_disable
[PATCH 3/4] arm64: dts: zynqmp: Add DMA for DP audio
Add the two DMA channels used for the DisplayPort audio to the
zynqmp_dpsub node.
Signed-off-by: Tomi Valkeinen
---
arch/arm64/boot/dts/xilinx/zynqmp.dtsi | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
b/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
index eaba466804bc..811d80cbf4c5 100644
--- a/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
+++ b/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
@@ -1036,11 +1036,14 @@ zynqmp_dpsub: display@fd4a {
"dp_vtc_pixel_clk_in";
power-domains = <_firmware PD_DP>;
resets = <_reset ZYNQMP_RESET_DP>;
- dma-names = "vid0", "vid1", "vid2", "gfx0";
+ dma-names = "vid0", "vid1", "vid2", "gfx0",
+ "aud0", "aud1";
dmas = <_dpdma ZYNQMP_DPDMA_VIDEO0>,
<_dpdma ZYNQMP_DPDMA_VIDEO1>,
<_dpdma ZYNQMP_DPDMA_VIDEO2>,
- <_dpdma ZYNQMP_DPDMA_GRAPHICS>;
+ <_dpdma ZYNQMP_DPDMA_GRAPHICS>,
+ <_dpdma ZYNQMP_DPDMA_AUDIO0>,
+ <_dpdma ZYNQMP_DPDMA_AUDIO1>;
ports {
#address-cells = <1>;
--
2.34.1
[PATCH 2/4] dt-bindings: display/xlnx/zynqmp-dpsub: Add audio DMAs
The DP subsystem for ZynqMP support audio via two channels, and the DP DMA has dma-engines for those channels. For some reason the DT binding has not specified those channels, even if the picture included in xlnx,zynqmp-dpsub.yaml shows "2 x aud" DMAs. Add the two audio DMAs to the binding. Signed-off-by: Tomi Valkeinen --- .../devicetree/bindings/display/xlnx/xlnx,zynqmp-dpsub.yaml| 10 -- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/Documentation/devicetree/bindings/display/xlnx/xlnx,zynqmp-dpsub.yaml b/Documentation/devicetree/bindings/display/xlnx/xlnx,zynqmp-dpsub.yaml index 554f9d5809d4..8a56ab923cca 100644 --- a/Documentation/devicetree/bindings/display/xlnx/xlnx,zynqmp-dpsub.yaml +++ b/Documentation/devicetree/bindings/display/xlnx/xlnx,zynqmp-dpsub.yaml @@ -100,12 +100,16 @@ properties: - description: Video layer, plane 1 (U/V or U) - description: Video layer, plane 2 (V) - description: Graphics layer + - description: Audio channel 0 + - description: Audio channel 1 dma-names: items: - const: vid0 - const: vid1 - const: vid2 - const: gfx0 + - const: aud0 + - const: aud1 phys: description: PHYs for the DP data lanes @@ -194,11 +198,13 @@ examples: power-domains = <_dp>; resets = < ZYNQMP_RESET_DP>; -dma-names = "vid0", "vid1", "vid2", "gfx0"; +dma-names = "vid0", "vid1", "vid2", "gfx0", "aud0", "aud1; dmas = <_dpdma 0>, <_dpdma 1>, <_dpdma 2>, - <_dpdma 3>; + <_dpdma 3>, + <_dpdma 4>, + <_dpdma 5>; phys = < 1 PHY_TYPE_DP 0 3>, < 0 PHY_TYPE_DP 1 3>; -- 2.34.1
[PATCH 1/4] ASoC: dmaengine_pcm: Allow passing component name via config
At the moment we cannot instantiate two dmaengine_pcms with the same
parent device, as the components will be named the same, leading to
conflicts.
Add 'name' field to the snd_dmaengine_pcm_config, and use that (if
defined) as the component name instead of deriving the component name
from the device.
Signed-off-by: Tomi Valkeinen
---
include/sound/dmaengine_pcm.h | 2 ++
sound/soc/soc-core.c | 8 +---
sound/soc/soc-generic-dmaengine-pcm.c | 3 +++
3 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/include/sound/dmaengine_pcm.h b/include/sound/dmaengine_pcm.h
index d70c55f17df7..c11aaf8079fb 100644
--- a/include/sound/dmaengine_pcm.h
+++ b/include/sound/dmaengine_pcm.h
@@ -118,6 +118,7 @@ int snd_dmaengine_pcm_refine_runtime_hwparams(
* which do not use devicetree.
* @process: Callback used to apply processing on samples transferred from/to
* user space.
+ * @name: Component name. If null, dev_name will be used.
* @compat_filter_fn: Will be used as the filter function when requesting a
* channel for platforms which do not use devicetree. The filter parameter
* will be the DAI's DMA data.
@@ -143,6 +144,7 @@ struct snd_dmaengine_pcm_config {
int (*process)(struct snd_pcm_substream *substream,
int channel, unsigned long hwoff,
unsigned long bytes);
+ const char *name;
dma_filter_fn compat_filter_fn;
struct device *dma_dev;
const char *chan_names[SNDRV_PCM_STREAM_LAST + 1];
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index 516350533e73..772d67065611 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -2792,10 +2792,12 @@ int snd_soc_component_initialize(struct
snd_soc_component *component,
INIT_LIST_HEAD(>list);
mutex_init(>io_mutex);
- component->name = fmt_single_name(dev, >id);
if (!component->name) {
- dev_err(dev, "ASoC: Failed to allocate name\n");
- return -ENOMEM;
+ component->name = fmt_single_name(dev, >id);
+ if (!component->name) {
+ dev_err(dev, "ASoC: Failed to allocate name\n");
+ return -ENOMEM;
+ }
}
component->dev = dev;
diff --git a/sound/soc/soc-generic-dmaengine-pcm.c
b/sound/soc/soc-generic-dmaengine-pcm.c
index 092ca09f3631..83db1a83d8ba 100644
--- a/sound/soc/soc-generic-dmaengine-pcm.c
+++ b/sound/soc/soc-generic-dmaengine-pcm.c
@@ -441,6 +441,9 @@ int snd_dmaengine_pcm_register(struct device *dev,
pcm->config = config;
pcm->flags = flags;
+ if (config->name)
+ pcm->component.name = config->name;
+
ret = dmaengine_pcm_request_chan_of(pcm, dev, config);
if (ret)
goto err_free_dma;
--
2.34.1
[PATCH 0/4] drm: xlnx: zynqmp: Add DP audio support
Add DisplayPort audio support for Xilinx ZynqMP platforms. This depends on patch adding cyclic DMA mode for DPDMA driver: https://lore.kernel.org/all/20240228042124.3074044-3-vishal.sa...@amd.com/ If that patch is missing, starting an audio playback will fail with an ASoC error. The current DT is, for some reason, missing the DMA channels for the audio. This series adds that to the bindings and the dts file, but to support older dtb files without the audio DMA, the driver will not fail if the audio DMA is missing, but will just mark the audio support as disabled. The series also includes an improvement to the soc-generic-dmaengine-pcm.c, required to support two dmaengine_pcms. Signed-off-by: Tomi Valkeinen --- Tomi Valkeinen (4): ASoC: dmaengine_pcm: Allow passing component name via config dt-bindings: display/xlnx/zynqmp-dpsub: Add audio DMAs arm64: dts: zynqmp: Add DMA for DP audio drm: xlnx: zynqmp_dpsub: Add DP audio support .../bindings/display/xlnx/xlnx,zynqmp-dpsub.yaml | 10 +- arch/arm64/boot/dts/xilinx/zynqmp.dtsi | 7 +- drivers/gpu/drm/xlnx/Kconfig | 9 + drivers/gpu/drm/xlnx/Makefile | 1 + drivers/gpu/drm/xlnx/zynqmp_disp.c | 50 --- drivers/gpu/drm/xlnx/zynqmp_disp_regs.h| 7 +- drivers/gpu/drm/xlnx/zynqmp_dp.c | 54 ++- drivers/gpu/drm/xlnx/zynqmp_dp.h | 7 + drivers/gpu/drm/xlnx/zynqmp_dp_audio.c | 461 + drivers/gpu/drm/xlnx/zynqmp_dpsub.c| 39 +- drivers/gpu/drm/xlnx/zynqmp_dpsub.h| 15 +- include/sound/dmaengine_pcm.h | 2 + sound/soc/soc-core.c | 8 +- sound/soc/soc-generic-dmaengine-pcm.c | 3 + 14 files changed, 563 insertions(+), 110 deletions(-) --- base-commit: e8f897f4afef0031fe618a8e94127a0934896aba change-id: 20240312-xilinx-dp-audio-468ad12f9f64 Best regards, -- Tomi Valkeinen
[PATCH] drm: xlnx: zynqmp_dpsub: Fix missing drm_bridge_add() call
The driver creates a bridge, but never calls drm_bridge_add() when
non-live input is used. This leaves the bridge's hpd_mutex
uninitialized, leading to:
WARNING: CPU: 0 PID: 9 at kernel/locking/mutex.c:582 __mutex_lock+0x708/0x840
Add the bridge add & remove calls so that the bridge gets managed
correctly.
Signed-off-by: Tomi Valkeinen
Fixes: 561671612394 ("drm: xlnx: zynqmp_dpsub: Add support for live video
input")
---
drivers/gpu/drm/xlnx/zynqmp_dp.c| 4
drivers/gpu/drm/xlnx/zynqmp_dpsub.c | 4
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
index a0606fab0e22..9f750740dfb8 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
@@ -1761,6 +1761,8 @@ int zynqmp_dp_probe(struct zynqmp_dpsub *dpsub)
dpsub->dp = dp;
+ drm_bridge_add(dpsub->bridge);
+
dev_dbg(dp->dev, "ZynqMP DisplayPort Tx probed with %u lanes\n",
dp->num_lanes);
@@ -1789,4 +1791,6 @@ void zynqmp_dp_remove(struct zynqmp_dpsub *dpsub)
zynqmp_dp_phy_exit(dp);
zynqmp_dp_reset(dp, true);
+
+ drm_bridge_remove(dpsub->bridge);
}
diff --git a/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
b/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
index 88eb33acd5f0..3933c4f1a44f 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
@@ -260,8 +260,6 @@ static int zynqmp_dpsub_probe(struct platform_device *pdev)
ret = zynqmp_dpsub_drm_init(dpsub);
if (ret)
goto err_disp;
- } else {
- drm_bridge_add(dpsub->bridge);
}
dev_info(>dev, "ZynqMP DisplayPort Subsystem driver probed");
@@ -288,8 +286,6 @@ static void zynqmp_dpsub_remove(struct platform_device
*pdev)
if (dpsub->drm)
zynqmp_dpsub_drm_cleanup(dpsub);
- else
- drm_bridge_remove(dpsub->bridge);
zynqmp_disp_remove(dpsub);
zynqmp_dp_remove(dpsub);
---
base-commit: e8f897f4afef0031fe618a8e94127a0934896aba
change-id: 20240312-xilinx-dp-lock-fix-cf68f43a7bab
Best regards,
--
Tomi Valkeinen
Re: [PATCH] drm: xlnx: dp: Reset DisplayPort IP
On 29/02/2024 11:05, Laurent Pinchart wrote: Tomi, could you push this through drm-misc ? On Wed, Feb 28, 2024 at 06:22:25PM +0200, Laurent Pinchart wrote: Hello Rohit, Thank you for the patch. On Fri, Feb 16, 2024 at 04:40:43AM -0800, Rohit Visavalia wrote: Assert DisplayPort reset signal before deasserting, it is to clear out any registers programmed before booting kernel. Signed-off-by: Rohit Visavalia --- drivers/gpu/drm/xlnx/zynqmp_dp.c | 4 1 file changed, 4 insertions(+) diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c index 1846c4971fd8..5a40aa1d4283 100644 --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c @@ -1714,6 +1714,10 @@ int zynqmp_dp_probe(struct zynqmp_dpsub *dpsub) goto err_free; } + ret = zynqmp_dp_reset(dp, true); + if (ret < 0) + return ret; + This looks fine to me, so Reviewed-by: Laurent Pinchart Thanks, tested and pushed to drm-misc-next. Tomi
Re: [PATCH v2 0/2] Fixes for omapdrm console
On 26/02/2024 10:26, Tomi Valkeinen wrote: Hi Tony, On 25/02/2024 08:46, Tony Lindgren wrote: Here are two fixes for omapdrm console. How is it broken? I don't usually use the console (or fbdev) but enabling it now, it seems to work fine for me, on DRA76 EVM with HDMI output. After applying your patches, I see a lot of cache-related artifacts on the screen when updating the fb. Tomi
Re: [PATCH v2 0/2] Fixes for omapdrm console
Hi Tony, On 25/02/2024 08:46, Tony Lindgren wrote: Here are two fixes for omapdrm console. How is it broken? I don't usually use the console (or fbdev) but enabling it now, it seems to work fine for me, on DRA76 EVM with HDMI output. Tomi Regards, Tony Changes since v1: - Add FB_GEN_DEFAULT_DEFERRED_DMAMEM_OPS to use with FB_DEFAULT_DEFERRED_OPS as suggested by Thomas Tony Lindgren (2): drm/omapdrm: Fix console by implementing fb_dirty drm/omapdrm: Fix console with deferred ops drivers/gpu/drm/omapdrm/omap_fbdev.c | 39 +++- include/linux/fb.h | 4 +++ 2 files changed, 31 insertions(+), 12 deletions(-)
Re: [PATCH v2 0/2] drm/bridge: tc358767: Fix DRM_BRIDGE_ATTACH_NO_CONNECTOR case
On 15/02/2024 11:03, Alexander Stein wrote: Hi everyone, Am Donnerstag, 15. Februar 2024, 09:53:54 CET schrieb Jan Kiszka: On 11.12.23 09:07, Aradhya Bhatia wrote: On 06/12/23 17:41, Tomi Valkeinen wrote: Hi, On 08/11/2023 14:45, Alexander Stein wrote: Hi Tomi, Am Mittwoch, 8. November 2023, 12:27:21 CET schrieb Tomi Valkeinen: These two patches are needed to make tc358767 work in the DRM_BRIDGE_ATTACH_NO_CONNECTOR case, at least when using a DP connector. I have tested this with TI AM654 EVM with a tc358767 add-on card connected to a DP monitor. Just a question regarding the usage of this DSI-DP bridge. What is the state of the DSI lanes after the DSI host has been initialized, but before calling atomic_pre_enable? AFAIK this bridge requires LP-11 on DSI at any time for accessing the AUX channel. + Marek Marek, Alexander, A quick grep tells me that you have added devicetree for tc358767 in DSI to (e)DP mode on other platforms. Could you please test these patches and report if you find any issue? Sorry, I can't provide any feedback here. I've yet to setup the DSI-DP correctly. Ok. Does anyone have a worry that these patches make the situation worse for the DSI case than it was before? Afaics, if the DSI lanes are not set up early enough by the DSI host, the driver would break with and without these patches. These do fix the driver for DRM_BRIDGE_ATTACH_NO_CONNECTOR and DPI, so I'd like to merge these unless these cause a regression with the DSI case. Tomi
Re: [PATCH 1/2] drm/tidss: Fix initial plane zpos values
Hi, On 13/02/2024 13:39, Daniel Stone wrote: Hi, On Tue, 13 Feb 2024 at 10:18, Marius Vlad wrote: On Tue, Feb 13, 2024 at 11:57:59AM +0200, Tomi Valkeinen wrote: I haven't. I'm quite unfamiliar with Weston, and Randolph from TI (cc'd) has been working on the Weston side of things. I also don't know if there's something TI specific here, as the use case is with non-mainline GPU drivers and non-mainline Mesa. I should have been a bit clearer in the patch description, as I didn't mean that upstream Weston has a bug (maybe it has, maybe it has not). Don't worry about it. We've had bugs in the past and I'm sure we'll have more. :) Either way, it's definitely better to have the kernel expose sensible behaviour rather than weird workarounds, unless they've been around for so long that they're basically baked into ABI. Yeah, that's always a worry. I do hope that no user of tidss expects the plane zpos values to be the current funny ones. But we'll probably find out when I merge this =). The issue seen is that when Weston decides to use DRM planes for composition, the plane zpositions are not configured correctly (or at all?). Afaics, this leads to e.g. weston showing a window with a DRM "overlay" plane that is behind the "primary" root plane, so the window is not visible. And as Weston thinks that the area supposedly covered by the overlay plane does not need to be rendered on the root plane, there are also artifacts on that area. Also, the Weston I used is a bit older one (10.0.1), as I needed to go back in my buildroot versions to get all that non-mainline GPU stuff compiled and working. A more recent Weston may behave differently. Right after Weston 10, we had a few minor changes related to the zpos-sorting list of planes and how we parse the plan list without having a temporary zpos ordered list to pick planes from. And there's another fix for missing out to set out the zpos for scanout to the minimum available - which seems like a good candidate to explain what happens in the issue described above. So if trying Weston again, please try with at least Weston 12, which should have those changes in. Specifically, you probably want commits 4cde507be6a1 and 58dde0e0c000. I think the window of breakage was small enough that - assuming either those commits or an upgrade to Weston 12/13 fixes it - we can just ask people to upgrade to a fixed Weston. Presuming this is not related to any TI specific code, I guess it's a regression in the sense that at some point Weston added the support to use planes for composition, so previously with only a single plane per display there was no issue. That point was 12 years ago, so not that novel. ;) Hmm, so do I understand it right, the plane code from 12 years back supposedly works ok, but somewhere around Weston 10 something broke, but was fixed with the commits you mention above? Tomi
Re: [PATCH v5 0/4] Add common1 region for AM62, AM62A & AM65x
On 16/02/2024 08:24, Devarsh Thakkar wrote: This adds DSS common1 region for respective SoCs supporting it. Changelog: V2 : Remove do-not-merge tag and add am62a dss common1 reion V3 : Add Fixes tag to each commit V4 : Add Reviewed-by tag and AM62A SoC TRM Link V5 : Split dts patch to separate patches for each SoC Devarsh Thakkar (4): dt-bindings: display: ti,am65x-dss: Add support for common1 region arm64: dts: ti: Add common1 register space for AM65x SoC arm64: dts: ti: Add common1 register space for AM62x SoC arm64: dts: ti: Add common1 register space for AM62A SoC .../devicetree/bindings/display/ti/ti,am65x-dss.yaml | 7 +-- arch/arm64/boot/dts/ti/k3-am62-main.dtsi | 5 +++-- arch/arm64/boot/dts/ti/k3-am62a-main.dtsi | 5 +++-- arch/arm64/boot/dts/ti/k3-am65-main.dtsi | 5 +++-- 4 files changed, 14 insertions(+), 8 deletions(-) For the series: Reviewed-by: Tomi Valkeinen Tomi
Re: [PATCH 1/2] dt-bindings: display: ti,am65x-dss: Add support for common1 region
On 14/02/2024 11:10, Tomi Valkeinen wrote: Hi, On 15/01/2024 14:57, Devarsh Thakkar wrote: TI keystone display subsystem present in AM65 and other SoCs such as AM62 support two separate register spaces namely "common" and "common1" which can be used by two separate hosts to program the display controller as described in respective Technical Reference Manuals [1]. The common1 register space has similar set of configuration registers as supported in common register space except the global configuration registers which are exclusive to common region. This adds binding for "common1" register region too as supported by the hardware. [1]: AM62x TRM: https://www.ti.com/lit/pdf/spruiv7 (Section 14.8.9.1 DSS Registers) AM65x TRM: https://www.ti.com/lit/pdf/spruid7 (Section 12.6.5 DSS Registers) Signed-off-by: Devarsh Thakkar --- .../devicetree/bindings/display/ti/ti,am65x-dss.yaml | 7 +-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml index b6767ef0d24d..55e3e490d0e6 100644 --- a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml +++ b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml @@ -37,6 +37,7 @@ properties: - description: OVR2 overlay manager for vp2 - description: VP1 video port 1 - description: VP2 video port 2 + - description: common1 DSS register area reg-names: items: @@ -47,6 +48,7 @@ properties: - const: ovr2 - const: vp1 - const: vp2 + - const: common1 clocks: items: @@ -147,9 +149,10 @@ examples: <0x04a07000 0x1000>, /* ovr1 */ <0x04a08000 0x1000>, /* ovr2 */ <0x04a0a000 0x1000>, /* vp1 */ - <0x04a0b000 0x1000>; /* vp2 */ + <0x04a0b000 0x1000>, /* vp2 */ + <0x04a01000 0x1000>; /* common1 */ reg-names = "common", "vidl1", "vid", - "ovr1", "ovr2", "vp1", "vp2"; + "ovr1", "ovr2", "vp1", "vp2", "common1"; ti,am65x-oldi-io-ctrl = <_oldi_io_ctrl>; power-domains = <_pds 67 TI_SCI_PD_EXCLUSIVE>; clocks = <_clks 67 1>, Looks fine to me, I'll apply to drm-misc-next. Hmm, now thinking about this, doesn't this cause dtb checks to start failing, as the dtbs are missing one entry? Is it better to merge these kind of changes with the dts changes? Or does it matter? Tomi
Re: [PATCH 1/2] dt-bindings: display: ti,am65x-dss: Add support for common1 region
Hi, On 15/01/2024 14:57, Devarsh Thakkar wrote: TI keystone display subsystem present in AM65 and other SoCs such as AM62 support two separate register spaces namely "common" and "common1" which can be used by two separate hosts to program the display controller as described in respective Technical Reference Manuals [1]. The common1 register space has similar set of configuration registers as supported in common register space except the global configuration registers which are exclusive to common region. This adds binding for "common1" register region too as supported by the hardware. [1]: AM62x TRM: https://www.ti.com/lit/pdf/spruiv7 (Section 14.8.9.1 DSS Registers) AM65x TRM: https://www.ti.com/lit/pdf/spruid7 (Section 12.6.5 DSS Registers) Signed-off-by: Devarsh Thakkar --- .../devicetree/bindings/display/ti/ti,am65x-dss.yaml | 7 +-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml index b6767ef0d24d..55e3e490d0e6 100644 --- a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml +++ b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml @@ -37,6 +37,7 @@ properties: - description: OVR2 overlay manager for vp2 - description: VP1 video port 1 - description: VP2 video port 2 + - description: common1 DSS register area reg-names: items: @@ -47,6 +48,7 @@ properties: - const: ovr2 - const: vp1 - const: vp2 + - const: common1 clocks: items: @@ -147,9 +149,10 @@ examples: <0x04a07000 0x1000>, /* ovr1 */ <0x04a08000 0x1000>, /* ovr2 */ <0x04a0a000 0x1000>, /* vp1 */ -<0x04a0b000 0x1000>; /* vp2 */ +<0x04a0b000 0x1000>, /* vp2 */ +<0x04a01000 0x1000>; /* common1 */ reg-names = "common", "vidl1", "vid", -"ovr1", "ovr2", "vp1", "vp2"; +"ovr1", "ovr2", "vp1", "vp2", "common1"; ti,am65x-oldi-io-ctrl = <_oldi_io_ctrl>; power-domains = <_pds 67 TI_SCI_PD_EXCLUSIVE>; clocks =<_clks 67 1>, Looks fine to me, I'll apply to drm-misc-next. Tomi
Re: [PATCH 1/2] drm/tidss: Fix initial plane zpos values
Hi,
On 13/02/2024 11:04, Pekka Paalanen wrote:
On Tue, 13 Feb 2024 10:16:36 +0200
Tomi Valkeinen wrote:
When the driver sets up the zpos property it sets the default zpos value
to the HW id of the plane. That is fine as such, but as on many DSS
versions the driver arranges the DRM planes in a different order than
the HW planes (to keep the non-scalable planes first), this leads to odd
initial zpos values. An example is J721e, where the initial zpos values
for DRM planes are 1, 3, 0, 2.
In theory the userspace should configure the zpos values properly when
using multiple planes, and in that sense the initial zpos values
shouldn't matter, but there's really no reason not to fix this and help
the userspace apps which don't handle zpos perfectly. In particular,
Weston seems to have issues dealing with the planes with the current
default zpos values.
So let's change the zpos values for the DRM planes to 0, 1, 2, 3.
Another option would be to configure the planes marked as primary planes
to zpos 0. On a two display system this would give us plane zpos values
of 0, 0, 1, 2. The end result and behavior would be very similar in this
option, and I'm not aware that this would actually help us in any way.
So, to keep the code simple, I opted for the 0, 1, 2, 3 values.
Signed-off-by: Tomi Valkeinen
Fixes: 32a1795f57ee ("drm/tidss: New driver for TI Keystone platform Display
SubSystem")
Hi Tomi,
have you reported this to Weston? What exactly is the problem?
I haven't. I'm quite unfamiliar with Weston, and Randolph from TI (cc'd)
has been working on the Weston side of things. I also don't know if
there's something TI specific here, as the use case is with non-mainline
GPU drivers and non-mainline Mesa. I should have been a bit clearer in
the patch description, as I didn't mean that upstream Weston has a bug
(maybe it has, maybe it has not).
The issue seen is that when Weston decides to use DRM planes for
composition, the plane zpositions are not configured correctly (or at
all?). Afaics, this leads to e.g. weston showing a window with a DRM
"overlay" plane that is behind the "primary" root plane, so the window
is not visible. And as Weston thinks that the area supposedly covered by
the overlay plane does not need to be rendered on the root plane, there
are also artifacts on that area.
Also, the Weston I used is a bit older one (10.0.1), as I needed to go
back in my buildroot versions to get all that non-mainline GPU stuff
compiled and working. A more recent Weston may behave differently.
It doesn't seem like a good idea to work around userspace bugs
(non-regression, I presume?) with kernel changes.
Presuming this is not related to any TI specific code, I guess it's a
regression in the sense that at some point Weston added the support to
use planes for composition, so previously with only a single plane per
display there was no issue.
I agree with you, this patch shouldn't be merged to "fix" issues with
tidss + Weston. However, the current default zpos values really don't
make sense, so I think this patch can stand on its own, and should be
merged just to make the tidss behavior a bit saner.
But even if this patch merged, the issue with Weston should be looked at
(*poke* Randolph =).
Tomi
Thanks,
pq
---
drivers/gpu/drm/tidss/tidss_plane.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/tidss/tidss_plane.c
b/drivers/gpu/drm/tidss/tidss_plane.c
index e1c0ef0c3894..68fed531f6a7 100644
--- a/drivers/gpu/drm/tidss/tidss_plane.c
+++ b/drivers/gpu/drm/tidss/tidss_plane.c
@@ -213,7 +213,7 @@ struct tidss_plane *tidss_plane_create(struct tidss_device
*tidss,
drm_plane_helper_add(>plane, _plane_helper_funcs);
- drm_plane_create_zpos_property(>plane, hw_plane_id, 0,
+ drm_plane_create_zpos_property(>plane, tidss->num_planes, 0,
num_planes - 1);
ret = drm_plane_create_color_properties(>plane,
[PATCH 2/2] drm/tidss: Fix sync-lost issue with two displays
A sync lost issue can be observed with two displays, when moving a plane
from one disabled display to an another disabled display, and then
enabling the display to which the plane was moved to. The exact
requirements for this to trigger are not clear.
It looks like the issue is that the layers are left enabled in the first
display's OVR registers. Even if the corresponding VP is disabled, it
still causes an issue, as if the disabled VP and its OVR would still be
in use, leading to the same VID being used by two OVRs. However, this is
just speculation based on testing the DSS behavior.
Experimentation shows that as a workaround, we can disable all the
layers in the OVR when disabling a VP. There should be no downside to
this, as the OVR is anyway effectively disabled if its VP is disabled,
and it seems to solve the sync lost issue.
However, there may be a bigger issue in play here, related to J721e
erratum i2097 ("DSS: Disabling a Layer Connected to Overlay May Result
in Synclost During the Next Frame"). Experimentation also shows that the
OVR's CHANNELIN field has similar issue. So we may need to revisit this
when we find out more about the core issue.
Signed-off-by: Tomi Valkeinen
Fixes: 32a1795f57ee ("drm/tidss: New driver for TI Keystone platform Display
SubSystem")
---
drivers/gpu/drm/tidss/tidss_crtc.c | 10 ++
1 file changed, 10 insertions(+)
diff --git a/drivers/gpu/drm/tidss/tidss_crtc.c
b/drivers/gpu/drm/tidss/tidss_crtc.c
index 5f838980c7a1..94f8e3178df5 100644
--- a/drivers/gpu/drm/tidss/tidss_crtc.c
+++ b/drivers/gpu/drm/tidss/tidss_crtc.c
@@ -265,6 +265,16 @@ static void tidss_crtc_atomic_disable(struct drm_crtc
*crtc,
reinit_completion(>framedone_completion);
+ /*
+* If a layer is left enabled when the videoport is disabled, and the
+* vid pipeline that was used for the layer is taken into use on
+* another videoport, the DSS will report sync lost issues. Disable all
+* the layers here as a work-around.
+*/
+ for (u32 layer = 0; layer < tidss->feat->num_planes; layer++)
+ dispc_ovr_enable_layer(tidss->dispc, tcrtc->hw_videoport, layer,
+ false);
+
dispc_vp_disable(tidss->dispc, tcrtc->hw_videoport);
if (!wait_for_completion_timeout(>framedone_completion,
--
2.34.1
[PATCH 1/2] drm/tidss: Fix initial plane zpos values
When the driver sets up the zpos property it sets the default zpos value
to the HW id of the plane. That is fine as such, but as on many DSS
versions the driver arranges the DRM planes in a different order than
the HW planes (to keep the non-scalable planes first), this leads to odd
initial zpos values. An example is J721e, where the initial zpos values
for DRM planes are 1, 3, 0, 2.
In theory the userspace should configure the zpos values properly when
using multiple planes, and in that sense the initial zpos values
shouldn't matter, but there's really no reason not to fix this and help
the userspace apps which don't handle zpos perfectly. In particular,
Weston seems to have issues dealing with the planes with the current
default zpos values.
So let's change the zpos values for the DRM planes to 0, 1, 2, 3.
Another option would be to configure the planes marked as primary planes
to zpos 0. On a two display system this would give us plane zpos values
of 0, 0, 1, 2. The end result and behavior would be very similar in this
option, and I'm not aware that this would actually help us in any way.
So, to keep the code simple, I opted for the 0, 1, 2, 3 values.
Signed-off-by: Tomi Valkeinen
Fixes: 32a1795f57ee ("drm/tidss: New driver for TI Keystone platform Display
SubSystem")
---
drivers/gpu/drm/tidss/tidss_plane.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/tidss/tidss_plane.c
b/drivers/gpu/drm/tidss/tidss_plane.c
index e1c0ef0c3894..68fed531f6a7 100644
--- a/drivers/gpu/drm/tidss/tidss_plane.c
+++ b/drivers/gpu/drm/tidss/tidss_plane.c
@@ -213,7 +213,7 @@ struct tidss_plane *tidss_plane_create(struct tidss_device
*tidss,
drm_plane_helper_add(>plane, _plane_helper_funcs);
- drm_plane_create_zpos_property(>plane, hw_plane_id, 0,
+ drm_plane_create_zpos_property(>plane, tidss->num_planes, 0,
num_planes - 1);
ret = drm_plane_create_color_properties(>plane,
--
2.34.1
[PATCH 0/2] drm/tidss: Fixes for zpos and multi-display
Two fixes for tidss: The first one helps Weston deal with the planes, the second fixes a possible sync-lost issue with multiple displays. Signed-off-by: Tomi Valkeinen --- Tomi Valkeinen (2): drm/tidss: Fix initial plane zpos values drm/tidss: Fix sync-lost issue with two displays drivers/gpu/drm/tidss/tidss_crtc.c | 10 ++ drivers/gpu/drm/tidss/tidss_plane.c | 2 +- 2 files changed, 11 insertions(+), 1 deletion(-) --- base-commit: 423af970da74db7eed1b14f2b7f115714a67aeb8 change-id: 20240213-tidss-fixes-cc3741cd604c Best regards, -- Tomi Valkeinen
Re: [PATCH v3 0/5] Fixing live video input in ZynqMP DPSUB
On 24/01/2024 04:53, Anatoliy Klymenko wrote: Add few missing pieces to support ZynqMP DPSUB live video in mode. ZynqMP DPSUB supports 2 modes of operations in regard to video data input. In the first mode, DPSUB uses DMA engine to pull video data from memory buffers. To support this the driver implements CRTC and DRM bridge representing DP encoder. In the second mode, DPSUB acquires video data pushed from FPGA and passes it downstream to DP output. This mode of operation is modeled in the driver as a DRM bridge that should be attached to some external CRTC. Patches 1/5,2/5,3/5,4/5 are minor fixes. DPSUB requires input live video format to be configured. Patch 5/5: The DP Subsystem requires the input live video format to be configured. In this patch, we are assuming that the CRTC's bus format is fixed (typical for FPGA CRTC) and comes from the device tree. This is a proposed solution, as there is no API to query CRTC output bus format or negotiate it in any other way. Changes in v2: - Address reviewers' comments: - More elaborate and consistent comments / commit messages - Fix includes' order - Replace of_property_read_u32_index() with of_property_read_u32() Changes in v3: - Split patch #3 into 3) moving status register clear immediately after read; 4) masking status against interrupts' mask Link to v1: https://lore.kernel.org/all/20240112234222.913138-1-anatoliy.klyme...@amd.com/ Link to v2: https://lore.kernel.org/all/20240119055437.2549149-1-anatoliy.klyme...@amd.com/ Anatoliy Klymenko (5): drm: xlnx: zynqmp_dpsub: Make drm bridge discoverable drm: xlnx: zynqmp_dpsub: Fix timing for live mode drm: xlnx: zynqmp_dpsub: Clear status register ASAP drm: xlnx: zynqmp_dpsub: Filter interrupts against mask drm: xlnx: zynqmp_dpsub: Set live video in format drivers/gpu/drm/xlnx/zynqmp_disp.c | 111 +--- drivers/gpu/drm/xlnx/zynqmp_disp.h | 3 +- drivers/gpu/drm/xlnx/zynqmp_disp_regs.h | 8 +- drivers/gpu/drm/xlnx/zynqmp_dp.c| 16 +++- drivers/gpu/drm/xlnx/zynqmp_kms.c | 2 +- 5 files changed, 119 insertions(+), 21 deletions(-) Thanks! I have pushed patches 1 to 4 to drm-misc-next. As Laurent said, the fifth one needs some more work. Tomi
Re: [PATCH 4/4] drm: xlnx: zynqmp_dpsub: Set live video in format
On 19/01/2024 07:54, Klymenko, Anatoliy wrote: diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h b/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h index f92a006d5070..926e07c255bb 100644 --- a/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h +++ b/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h @@ -165,10 +165,10 @@ #define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_10 0x2 #define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_12 0x3 #define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_MASK GENMASK(2, 0) -#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_RGB 0x0 -#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV4440x1 -#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV4220x2 -#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YONLY 0x3 +#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_RGB 0x00 +#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV4440x10 +#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV4220x20 +#define ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YONLY 0x30 What's this about? Were these wrong before? Sounds like a separate patch needed for these. It is an embedded bit shift that corresponds to DPSUB live video / gfx format register layout. Original values are technically correct but would require extra bit shifts to operate with. The current patch is the first instance of actual use of those defines. Do you think it's worth to factor those changes out into a separate patch? The value for the defines should then be something like (0x3 << 4), to make it clearer that it's shifted to the right position. Tomi
Re: [PATCH v3 4/5] drm: xlnx: zynqmp_dpsub: Filter interrupts against mask
On 24/01/2024 04:54, Anatoliy Klymenko wrote: Filter out status register against the interrupts' mask. Some events are being reported via DP status register, even if corresponding interrupts have been disabled. One instance of such event leads to generation of VBLANK when the driver is in DRM bridge mode, which in turn results in NULL pointer dereferencing. We should avoid processing such events in an interrupt handler context. This problem is less noticeable when the driver operates in DMA mode, as in this case we have DRM CRTC object instantiated and DRM framework simply discards unwanted VBLANKs in drm_handle_vblank(). Signed-off-by: Anatoliy Klymenko --- drivers/gpu/drm/xlnx/zynqmp_dp.c | 9 - 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c index 5a3335e1fffa..9f48e5bbcdec 100644 --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c @@ -1627,7 +1627,14 @@ static irqreturn_t zynqmp_dp_irq_handler(int irq, void *data) /* clear status register as soon as we read it */ zynqmp_dp_write(dp, ZYNQMP_DP_INT_STATUS, status); mask = zynqmp_dp_read(dp, ZYNQMP_DP_INT_MASK); - if (!(status & ~mask)) + + /* +* Status register may report some events, which corresponding interrupts +* have been disabled. Filter out those events against interrupts' mask. +*/ + status &= ~mask; + + if (!status) return IRQ_NONE; /* dbg for diagnostic, but not much that the driver can do */ Reviewed-by: Tomi Valkeinen Tomi
Re: [PATCH v3 3/5] drm: xlnx: zynqmp_dpsub: Clear status register ASAP
On 24/01/2024 04:54, Anatoliy Klymenko wrote: Clear status register as soon as we read it. Addressing comments from https://lore.kernel.org/dri-devel/beb551c7-bb7e-4cd0-b166-e9aad90c4...@ideasonboard.com/ Signed-off-by: Anatoliy Klymenko --- drivers/gpu/drm/xlnx/zynqmp_dp.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c index d60b7431603f..5a3335e1fffa 100644 --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c @@ -1624,6 +1624,8 @@ static irqreturn_t zynqmp_dp_irq_handler(int irq, void *data) u32 status, mask; status = zynqmp_dp_read(dp, ZYNQMP_DP_INT_STATUS); + /* clear status register as soon as we read it */ + zynqmp_dp_write(dp, ZYNQMP_DP_INT_STATUS, status); mask = zynqmp_dp_read(dp, ZYNQMP_DP_INT_MASK); if (!(status & ~mask)) return IRQ_NONE; @@ -1634,8 +1636,6 @@ static irqreturn_t zynqmp_dp_irq_handler(int irq, void *data) if (status & ZYNQMP_DP_INT_CHBUF_OVERFLW_MASK) dev_dbg_ratelimited(dp->dev, "overflow interrupt\n"); - zynqmp_dp_write(dp, ZYNQMP_DP_INT_STATUS, status); - if (status & ZYNQMP_DP_INT_VBLANK_START) zynqmp_dpsub_drm_handle_vblank(dp->dpsub); Reviewed-by: Tomi Valkeinen Tomi
Re: [RFC PATCH 3/3] arm64: dts: ti: k3-am62x: Add overlay to use DSS in display sharing mode
Hi,
On 16/01/2024 15:41, Devarsh Thakkar wrote:
This overlay needs to be used with display sharing supported device
manager firmware only.
Remote core running this firmware has write access to "common" register
space, VIDL pipeline, OVR1 overlay and VP1 videoport.
The processing core running Linux is provided write access to VID
pipeline and "common1" register space.
The VP1 video port is shared between Linux and remote core with remote
core configuring the overlay manager to set Zorder 1 for VID pipeline
and Zorder 2 for VIDL pipeline.
Add reserved memory region for framebuffer region used by remote core in
dss shared mode overlay file so that Linux does not re-use the same
while allocating memory.
I don't understand this one. Why is RAM used by RTOS accessible by Linux
in the first place?
Tomi
Also add a label for reserved memory region in base device-tree file so
that it can be referred back in overlay file.
Signed-off-by: Devarsh Thakkar
---
arch/arm64/boot/dts/ti/Makefile | 1 +
.../arm64/boot/dts/ti/k3-am62x-sk-common.dtsi | 2 +-
.../dts/ti/k3-am62x-sk-dss-shared-mode.dtso | 33 +++
3 files changed, 35 insertions(+), 1 deletion(-)
create mode 100644 arch/arm64/boot/dts/ti/k3-am62x-sk-dss-shared-mode.dtso
diff --git a/arch/arm64/boot/dts/ti/Makefile b/arch/arm64/boot/dts/ti/Makefile
index 52c1dc910308..ff832741b367 100644
--- a/arch/arm64/boot/dts/ti/Makefile
+++ b/arch/arm64/boot/dts/ti/Makefile
@@ -35,6 +35,7 @@ dtb-$(CONFIG_ARCH_K3) += k3-am62x-sk-csi2-ov5640.dtbo
dtb-$(CONFIG_ARCH_K3) += k3-am62x-sk-csi2-tevi-ov5640.dtbo
dtb-$(CONFIG_ARCH_K3) += k3-am62x-sk-csi2-imx219.dtbo
dtb-$(CONFIG_ARCH_K3) += k3-am62x-sk-hdmi-audio.dtbo
+dtb-$(CONFIG_ARCH_K3) += k3-am62x-sk-dss-shared-mode.dtbo
# Boards with AM64x SoC
dtb-$(CONFIG_ARCH_K3) += k3-am642-evm.dtb
diff --git a/arch/arm64/boot/dts/ti/k3-am62x-sk-common.dtsi
b/arch/arm64/boot/dts/ti/k3-am62x-sk-common.dtsi
index 33768c02d8eb..8b55ca10102f 100644
--- a/arch/arm64/boot/dts/ti/k3-am62x-sk-common.dtsi
+++ b/arch/arm64/boot/dts/ti/k3-am62x-sk-common.dtsi
@@ -34,7 +34,7 @@ memory@8000 {
reg = <0x 0x8000 0x 0x8000>;
};
- reserved-memory {
+ reserved_memory: reserved-memory {
#address-cells = <2>;
#size-cells = <2>;
ranges;
diff --git a/arch/arm64/boot/dts/ti/k3-am62x-sk-dss-shared-mode.dtso
b/arch/arm64/boot/dts/ti/k3-am62x-sk-dss-shared-mode.dtso
new file mode 100644
index ..02153748a5c2
--- /dev/null
+++ b/arch/arm64/boot/dts/ti/k3-am62x-sk-dss-shared-mode.dtso
@@ -0,0 +1,33 @@
+// SPDX-License-Identifier: GPL-2.0
+/**
+ * DT overlay to enable display sharing mode for AM62P DSS0
+ * This is compatible with custom AM62 Device Manager firmware
+ *
+ * Copyright (C) 2023 Texas Instruments Incorporated - http://www.ti.com/
+ */
+
+/dts-v1/;
+/plugin/;
+
+#include
+#include
+
+ {
+ ti,dss-shared-mode;
+ ti,dss-shared-mode-vp = "vp1";
+ ti,dss-shared-mode-vp-owned = <0>;
+ ti,dss-shared-mode-common = "common1";
+ ti,dss-shared-mode-planes = "vid";
+ ti,dss-shared-mode-plane-zorder = <0>;
+ interrupts = ;
+};
+
+_memory {
+ #address-cells = <2>;
+ #size-cells = <2>;
+ rtos_framebuffer_memory_region: rtos-framebuffer-memory@9480 {
+ compatible = "shared-dma-pool";
+ reg = <0x00 0x9480 0x00 0x0800>;
+ no-map;
+ };
+};
Re: [RFC PATCH 2/3] drm/tidss: Add support for display sharing
Hi,
On 16/01/2024 15:41, Devarsh Thakkar wrote:
Display subsystem present in TI Keystone family of devices supports sharing
of display between multiple hosts as it provides separate register space
(common* region) for each host to programming display controller and also a
unique interrupt line for each host.
This adds support for display sharing, by allowing partitioning of
resources either at video port level or at video plane level as
described below :
1) Linux can own (i.e have write access) completely one or more of video
ports along with corresponding resources (viz. overlay managers,
video planes) used by Linux in context of those video ports.
Even if Linux is owning
these video ports it can still share this video port with a remote core
which can own one or more video planes associated with this video port.
2) Linux owns one or more of the video planes with video port
(along with corresponding overlay manager) associated with these planes
being owned and controlled by a remote core. Linux still has read-only
access to the associated video port and overlay managers so that it can
parse the settings made by remote core.
For both the cases, the resources used in context of processing core
running Linux along with ownership information are exposed by user as
part of device-tree blob and driver uses an updated feature list tailored
for this shared mode accordingly. The driver also auto-populates
matching overlay managers and output types from shared video
port list provided in device-tree blob.
In dispc_feature struct remove const access specfier for output_type
array as it is required to be updated dynamically in run-time for shared
mode.
For 2) where Linux is only owning a set of video planes with
corresponding video port and overlay manager controlled by a remote
core, separate set of CRTC callbacks are used which just latch on
to the preset mode set by remote core, thus avoiding any reconfiguration
of associated video ports, overlay managers and clocks.
For this case, it is also checked that Linux controlled video planes
don't exceed screen size set by remote core while running the display.
Display clocks and OLDI related fields are also not
populated for this scenario as remote core is owning those resources.
For 1), where Linux owns only a set of video port and associated
planes with rest of resources owned completely by remote cores,
only those set of resources are exposed to Linux and programmed using
traditional CRTC helpers and rest of video ports and associated resources
are removed from feature list accordingly.
Signed-off-by: Devarsh Thakkar
---
drivers/gpu/drm/tidss/tidss_crtc.c | 120 -
drivers/gpu/drm/tidss/tidss_dispc.c | 254 +---
drivers/gpu/drm/tidss/tidss_dispc.h | 2 +-
drivers/gpu/drm/tidss/tidss_drv.c | 33 ++--
drivers/gpu/drm/tidss/tidss_drv.h | 6 +
5 files changed, 375 insertions(+), 40 deletions(-)
diff --git a/drivers/gpu/drm/tidss/tidss_crtc.c
b/drivers/gpu/drm/tidss/tidss_crtc.c
index 5f838980c7a1..f6a877ff4c6c 100644
--- a/drivers/gpu/drm/tidss/tidss_crtc.c
+++ b/drivers/gpu/drm/tidss/tidss_crtc.c
@@ -31,13 +31,19 @@ static void tidss_crtc_finish_page_flip(struct tidss_crtc
*tcrtc)
/*
* New settings are taken into use at VFP, and GO bit is cleared at
* the same time. This happens before the vertical blank interrupt.
-* So there is a small change that the driver sets GO bit after VFP, but
+* So there is a small chance that the driver sets GO bit after VFP, but
* before vblank, and we have to check for that case here.
+*
+* For a video port shared between Linux and remote core but owned by
remote core,
+* this is not required since Linux just attaches to mode that was
preset by remote
+* core with which display is being shared.
*/
- busy = dispc_vp_go_busy(tidss->dispc, tcrtc->hw_videoport);
- if (busy) {
- spin_unlock_irqrestore(>event_lock, flags);
- return;
+ if (!tidss->shared_mode ||
tidss->shared_mode_owned_vps[tcrtc->hw_videoport]) {
You test this in multiple places. I think it would be better to combine
those, in one way or another. Either a helper function, or maybe invert
the shared_mode_owned_vps, i.e. rather have something like "foreign_vps"
(better name needed), so that when !shared_mode, the default of False in
foreign_vps array will just work. Then the above test will be just if
(!tidss->foreign_vps[tcrtc->hw_videoport])
+ busy = dispc_vp_go_busy(tidss->dispc, tcrtc->hw_videoport);
+ if (busy) {
+ spin_unlock_irqrestore(>event_lock, flags);
+ return;
+ }
}
event = tcrtc->event;
@@ -208,6 +214,44 @@ static void tidss_crtc_atomic_flush(struct drm_crtc *crtc,
spin_unlock_irqrestore(>event_lock, flags);
}
+static void
Re: [PATCH 4/4] drm: xlnx: zynqmp_dpsub: Set live video in format
Hi Anatoliy,
On 13/01/2024 01:42, Anatoliy Klymenko wrote:
Live video input format is expected to be set as
"bus-format" property in connected remote endpoint.
Program live video input format DPSUB registers.
Set display layer mode in layer creation context.
Some comments inline below. But one thing to improve is the commit desc.
I think this needs more explanation on what's the issue here. So
basically something like what's the feature in question, why it's not
working or what's missing, and what does this patch do to get it working.
And while often it's reasonable to expect some level of understanding of
the HW in question, it doesn't hurt to give some clarifications on the
names used (here the "live video input").
Signed-off-by: Anatoliy Klymenko
---
drivers/gpu/drm/xlnx/zynqmp_disp.c | 109 ++--
drivers/gpu/drm/xlnx/zynqmp_disp.h | 3 +-
drivers/gpu/drm/xlnx/zynqmp_disp_regs.h | 8 +-
drivers/gpu/drm/xlnx/zynqmp_dp.c| 2 +-
drivers/gpu/drm/xlnx/zynqmp_kms.c | 2 +-
5 files changed, 107 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c
b/drivers/gpu/drm/xlnx/zynqmp_disp.c
index 8a39b3accce5..83af3ad9cdb5 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
@@ -20,8 +20,10 @@
#include
#include
#include
+#include
#include
#include
+#include
Alphabetical order, please.
#include "zynqmp_disp.h"
#include "zynqmp_disp_regs.h"
@@ -67,12 +69,16 @@
/**
* struct zynqmp_disp_format - Display subsystem format information
* @drm_fmt: DRM format (4CC)
+ * @bus_fmt: Live video media bus format
* @buf_fmt: AV buffer format
* @swap: Flag to swap R & B for RGB formats, and U & V for YUV formats
* @sf: Scaling factors for color components
*/
struct zynqmp_disp_format {
- u32 drm_fmt;
+ union {
+ u32 drm_fmt;
+ u32 bus_fmt;
+ };
u32 buf_fmt;
bool swap;
const u32 *sf;
@@ -354,6 +360,16 @@ static const struct zynqmp_disp_format avbuf_gfx_fmts[] = {
},
};
+/* TODO: add support for different formats */
+static const struct zynqmp_disp_format avbuf_live_vid_fmts[] = {
+ {
+ .bus_fmt= MEDIA_BUS_FMT_UYVY8_1X16,
+ .buf_fmt= ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_8 |
+ ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV422,
+ .sf = scaling_factors_888,
+ }
+};
+
static u32 zynqmp_disp_avbuf_read(struct zynqmp_disp *disp, int reg)
{
return readl(disp->avbuf.base + reg);
@@ -369,6 +385,34 @@ static bool zynqmp_disp_layer_is_video(const struct
zynqmp_disp_layer *layer)
return layer->id == ZYNQMP_DPSUB_LAYER_VID;
}
+/**
+ * zynqmp_disp_avbuf_set_live_format - Set live input format for a layer
+ * @disp: Display controller
+ * @layer: The layer
+ * @fmt: The format information
+ *
+ * Set the live video input format for @layer to @fmt.
+ */
+static void zynqmp_disp_avbuf_set_live_format(struct zynqmp_disp *disp,
+ struct zynqmp_disp_layer *layer,
+ const struct zynqmp_disp_format
*fmt)
+{
+ u32 reg, i;
+
+ reg = zynqmp_disp_layer_is_video(layer)
+ ? ZYNQMP_DISP_AV_BUF_LIVE_VID_CONFIG
+ : ZYNQMP_DISP_AV_BUF_LIVE_GFX_CONFIG;
+ zynqmp_disp_avbuf_write(disp, reg, fmt->buf_fmt);
+
+ for (i = 0; i < ZYNQMP_DISP_AV_BUF_NUM_SF; ++i) {
+ reg = zynqmp_disp_layer_is_video(layer)
+ ? ZYNQMP_DISP_AV_BUF_LIVD_VID_COMP_SF(i)
+ : ZYNQMP_DISP_AV_BUF_LIVD_GFX_COMP_SF(i);
+ zynqmp_disp_avbuf_write(disp, reg, fmt->sf[i]);
+ }
+ layer->disp_fmt = fmt;
+}
+
/**
* zynqmp_disp_avbuf_set_format - Set the input format for a layer
* @disp: Display controller
@@ -902,15 +946,12 @@ u32 *zynqmp_disp_layer_drm_formats(struct
zynqmp_disp_layer *layer,
/**
* zynqmp_disp_layer_enable - Enable a layer
* @layer: The layer
- * @mode: Operating mode of layer
*
* Enable the @layer in the audio/video buffer manager and the blender. DMA
* channels are started separately by zynqmp_disp_layer_update().
*/
-void zynqmp_disp_layer_enable(struct zynqmp_disp_layer *layer,
- enum zynqmp_dpsub_layer_mode mode)
+void zynqmp_disp_layer_enable(struct zynqmp_disp_layer *layer)
{
- layer->mode = mode;
zynqmp_disp_avbuf_enable_video(layer->disp, layer);
zynqmp_disp_blend_layer_enable(layer->disp, layer);
}
@@ -950,11 +991,12 @@ void zynqmp_disp_layer_set_format(struct
zynqmp_disp_layer *layer,
layer->disp_fmt = zynqmp_disp_layer_find_format(layer, info->format);
layer->drm_fmt = info;
- zynqmp_disp_avbuf_set_format(layer->disp, layer, layer->disp_fmt);
-
- if
Re: [PATCH 3/4] drm: xlnx: zynqmp_dpsub: Don't generate vblank in live mode
On 13/01/2024 01:42, Anatoliy Klymenko wrote: Filter out status register against interrupts' mask. Some events are being reported via DP status register, even if corresponding interrupts have been disabled. Avoid processing of such events in interrupt handler context. The subject talks about vblank and live mode, the the description doesn't. Can you elaborate in the desc a bit about when this is an issue and why it wasn't before? Signed-off-by: Anatoliy Klymenko --- drivers/gpu/drm/xlnx/zynqmp_dp.c | 11 +-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c index d60b7431603f..571c5dbc97e5 100644 --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c @@ -1624,8 +1624,16 @@ static irqreturn_t zynqmp_dp_irq_handler(int irq, void *data) u32 status, mask; status = zynqmp_dp_read(dp, ZYNQMP_DP_INT_STATUS); + zynqmp_dp_write(dp, ZYNQMP_DP_INT_STATUS, status); mask = zynqmp_dp_read(dp, ZYNQMP_DP_INT_MASK); - if (!(status & ~mask)) + + /* +* Status register may report some events, which corresponding interrupts +* have been disabled. Filter out those events against interrupts' mask. +*/ + status &= ~mask; + + if (!status) return IRQ_NONE; /* dbg for diagnostic, but not much that the driver can do */ @@ -1634,7 +1642,6 @@ static irqreturn_t zynqmp_dp_irq_handler(int irq, void *data) if (status & ZYNQMP_DP_INT_CHBUF_OVERFLW_MASK) dev_dbg_ratelimited(dp->dev, "overflow interrupt\n"); - zynqmp_dp_write(dp, ZYNQMP_DP_INT_STATUS, status); if (status & ZYNQMP_DP_INT_VBLANK_START) zynqmp_dpsub_drm_handle_vblank(dp->dpsub); Moving the zynqmp_dp_write() is not related to this fix, is it? I think it should be in a separate patch. Tomi
Re: [PATCH 2/4] drm: xlnx: zynqmp_dpsub: Fix timing for live mode
On 13/01/2024 01:42, Anatoliy Klymenko wrote: Expect external video timing in live video input mode, program DPSUB accordingly. Signed-off-by: Anatoliy Klymenko --- drivers/gpu/drm/xlnx/zynqmp_disp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c b/drivers/gpu/drm/xlnx/zynqmp_disp.c index 407bc07cec69..8a39b3accce5 100644 --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c @@ -1166,7 +1166,7 @@ void zynqmp_disp_enable(struct zynqmp_disp *disp) /* Choose clock source based on the DT clock handle. */ zynqmp_disp_avbuf_set_clocks_sources(disp, disp->dpsub->vid_clk_from_ps, disp->dpsub->aud_clk_from_ps, -true); +disp->dpsub->vid_clk_from_ps); zynqmp_disp_avbuf_enable_channels(disp); zynqmp_disp_avbuf_enable_audio(disp); Reviewed-by: Tomi Valkeinen Tomi
Re: [PATCH 1/4] drm: xlnx: zynqmp_dpsub: Make drm bridge discoverable
On 13/01/2024 01:42, Anatoliy Klymenko wrote: Assign device of node to bridge prior registering it. This will make said bridge discoverable by separate crtc driver. I think a few words on why this is needed (and why it wasn't needed before) would be nice. Other than that: Reviewed-by: Tomi Valkeinen Tomi Signed-off-by: Anatoliy Klymenko --- drivers/gpu/drm/xlnx/zynqmp_dp.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c index a0606fab0e22..d60b7431603f 100644 --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c @@ -1721,6 +1721,7 @@ int zynqmp_dp_probe(struct zynqmp_dpsub *dpsub) bridge->ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID | DRM_BRIDGE_OP_HPD; bridge->type = DRM_MODE_CONNECTOR_DisplayPort; + bridge->of_node = dp->dev->of_node; dpsub->bridge = bridge; /*
[PATCH 2/2] drm/bridge: sii902x: Fix audio codec unregistration
The driver never unregisters the audio codec platform device, which can
lead to a crash on module reloading, nor does it handle the return value
from sii902x_audio_codec_init().
Signed-off-by: Tomi Valkeinen
Fixes: ff5781634c41 ("drm/bridge: sii902x: Implement HDMI audio support")
Cc: Jyri Sarha
---
drivers/gpu/drm/bridge/sii902x.c | 21 +
1 file changed, 17 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c
index 69da73e414a9..4560ae9cbce1 100644
--- a/drivers/gpu/drm/bridge/sii902x.c
+++ b/drivers/gpu/drm/bridge/sii902x.c
@@ -1080,7 +1080,9 @@ static int sii902x_init(struct sii902x *sii902x)
return ret;
}
- sii902x_audio_codec_init(sii902x, dev);
+ ret = sii902x_audio_codec_init(sii902x, dev);
+ if (ret)
+ return ret;
i2c_set_clientdata(sii902x->i2c, sii902x);
@@ -1088,13 +1090,15 @@ static int sii902x_init(struct sii902x *sii902x)
1, 0, I2C_MUX_GATE,
sii902x_i2c_bypass_select,
sii902x_i2c_bypass_deselect);
- if (!sii902x->i2cmux)
- return -ENOMEM;
+ if (!sii902x->i2cmux) {
+ ret = -ENOMEM;
+ goto err_unreg_audio;
+ }
sii902x->i2cmux->priv = sii902x;
ret = i2c_mux_add_adapter(sii902x->i2cmux, 0, 0, 0);
if (ret)
- return ret;
+ goto err_unreg_audio;
sii902x->bridge.funcs = _bridge_funcs;
sii902x->bridge.of_node = dev->of_node;
@@ -1107,6 +,12 @@ static int sii902x_init(struct sii902x *sii902x)
drm_bridge_add(>bridge);
return 0;
+
+err_unreg_audio:
+ if (!PTR_ERR_OR_ZERO(sii902x->audio.pdev))
+ platform_device_unregister(sii902x->audio.pdev);
+
+ return ret;
}
static int sii902x_probe(struct i2c_client *client)
@@ -1179,6 +1189,9 @@ static void sii902x_remove(struct i2c_client *client)
drm_bridge_remove(>bridge);
i2c_mux_del_adapters(sii902x->i2cmux);
+
+ if (!PTR_ERR_OR_ZERO(sii902x->audio.pdev))
+ platform_device_unregister(sii902x->audio.pdev);
}
static const struct of_device_id sii902x_dt_ids[] = {
--
2.34.1
[PATCH 1/2] drm/bridge: sii902x: Fix probing race issue
A null pointer dereference crash has been observed rarely on TI
platforms using sii9022 bridge:
[ 53.271356] sii902x_get_edid+0x34/0x70 [sii902x]
[ 53.276066] sii902x_bridge_get_edid+0x14/0x20 [sii902x]
[ 53.281381] drm_bridge_get_edid+0x20/0x34 [drm]
[ 53.286305] drm_bridge_connector_get_modes+0x8c/0xcc [drm_kms_helper]
[ 53.292955] drm_helper_probe_single_connector_modes+0x190/0x538
[drm_kms_helper]
[ 53.300510] drm_client_modeset_probe+0x1f0/0xbd4 [drm]
[ 53.305958] __drm_fb_helper_initial_config_and_unlock+0x50/0x510
[drm_kms_helper]
[ 53.313611] drm_fb_helper_initial_config+0x48/0x58 [drm_kms_helper]
[ 53.320039] drm_fbdev_dma_client_hotplug+0x84/0xd4 [drm_dma_helper]
[ 53.326401] drm_client_register+0x5c/0xa0 [drm]
[ 53.331216] drm_fbdev_dma_setup+0xc8/0x13c [drm_dma_helper]
[ 53.336881] tidss_probe+0x128/0x264 [tidss]
[ 53.341174] platform_probe+0x68/0xc4
[ 53.344841] really_probe+0x188/0x3c4
[ 53.348501] __driver_probe_device+0x7c/0x16c
[ 53.352854] driver_probe_device+0x3c/0x10c
[ 53.357033] __device_attach_driver+0xbc/0x158
[ 53.361472] bus_for_each_drv+0x88/0xe8
[ 53.365303] __device_attach+0xa0/0x1b4
[ 53.369135] device_initial_probe+0x14/0x20
[ 53.373314] bus_probe_device+0xb0/0xb4
[ 53.377145] deferred_probe_work_func+0xcc/0x124
[ 53.381757] process_one_work+0x1f0/0x518
[ 53.385770] worker_thread+0x1e8/0x3dc
[ 53.389519] kthread+0x11c/0x120
[ 53.392750] ret_from_fork+0x10/0x20
The issue here is as follows:
- tidss probes, but is deferred as sii902x is still missing.
- sii902x starts probing and enters sii902x_init().
- sii902x calls drm_bridge_add(). Now the sii902x bridge is ready from
DRM's perspective.
- sii902x calls sii902x_audio_codec_init() and
platform_device_register_data()
- The registration of the audio platform device causes probing of the
deferred devices.
- tidss probes, which eventually causes sii902x_bridge_get_edid() to be
called.
- sii902x_bridge_get_edid() tries to use the i2c to read the edid.
However, the sii902x driver has not set up the i2c part yet, leading
to the crash.
Fix this by moving the drm_bridge_add() to the end of the
sii902x_init(), which is also at the very end of sii902x_probe().
Signed-off-by: Tomi Valkeinen
Fixes: 21d808405fe4 ("drm/bridge/sii902x: Fix EDID readback")
---
drivers/gpu/drm/bridge/sii902x.c | 29 -
1 file changed, 16 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c
index 2bdc5b439beb..69da73e414a9 100644
--- a/drivers/gpu/drm/bridge/sii902x.c
+++ b/drivers/gpu/drm/bridge/sii902x.c
@@ -1080,16 +1080,6 @@ static int sii902x_init(struct sii902x *sii902x)
return ret;
}
- sii902x->bridge.funcs = _bridge_funcs;
- sii902x->bridge.of_node = dev->of_node;
- sii902x->bridge.timings = _sii902x_timings;
- sii902x->bridge.ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID;
-
- if (sii902x->i2c->irq > 0)
- sii902x->bridge.ops |= DRM_BRIDGE_OP_HPD;
-
- drm_bridge_add(>bridge);
-
sii902x_audio_codec_init(sii902x, dev);
i2c_set_clientdata(sii902x->i2c, sii902x);
@@ -1102,7 +1092,21 @@ static int sii902x_init(struct sii902x *sii902x)
return -ENOMEM;
sii902x->i2cmux->priv = sii902x;
- return i2c_mux_add_adapter(sii902x->i2cmux, 0, 0, 0);
+ ret = i2c_mux_add_adapter(sii902x->i2cmux, 0, 0, 0);
+ if (ret)
+ return ret;
+
+ sii902x->bridge.funcs = _bridge_funcs;
+ sii902x->bridge.of_node = dev->of_node;
+ sii902x->bridge.timings = _sii902x_timings;
+ sii902x->bridge.ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID;
+
+ if (sii902x->i2c->irq > 0)
+ sii902x->bridge.ops |= DRM_BRIDGE_OP_HPD;
+
+ drm_bridge_add(>bridge);
+
+ return 0;
}
static int sii902x_probe(struct i2c_client *client)
@@ -1170,12 +1174,11 @@ static int sii902x_probe(struct i2c_client *client)
}
static void sii902x_remove(struct i2c_client *client)
-
{
struct sii902x *sii902x = i2c_get_clientdata(client);
- i2c_mux_del_adapters(sii902x->i2cmux);
drm_bridge_remove(>bridge);
+ i2c_mux_del_adapters(sii902x->i2cmux);
}
static const struct of_device_id sii902x_dt_ids[] = {
--
2.34.1
[PATCH 0/2] drm/bridge: sii902x: Crash fixes
Two small fixes to sii902x for crashes. Signed-off-by: Tomi Valkeinen --- Tomi Valkeinen (2): drm/bridge: sii902x: Fix probing race issue drm/bridge: sii902x: Fix audio codec unregistration drivers/gpu/drm/bridge/sii902x.c | 42 +++- 1 file changed, 29 insertions(+), 13 deletions(-) --- base-commit: 0c75d52190b8bfa22cdb66e07148aea599c4535d change-id: 20240103-si902x-fixes-468d7153b8ee Best regards, -- Tomi Valkeinen
Re: [PATCH 0/2] drm/bridge: tc358767: Fix DRM_BRIDGE_ATTACH_NO_CONNECTOR case
On 15/12/2023 15:33, Jan Kiszka wrote: On 31.10.23 14:26, Tomi Valkeinen wrote: These two patches are needed to make tc358767 work in the DRM_BRIDGE_ATTACH_NO_CONNECTOR case, at least when using a DP connector. The first patch, "drm/bridge: tc358767: Support input format negotiation hook" was already sent separately, but I included it here for completeness, as both are needed to get a working display. I have tested this with TI AM654 EVM with a tc358767 add-on card. Signed-off-by: Tomi Valkeinen --- Aradhya Bhatia (1): drm/bridge: tc358767: Support input format negotiation hook Tomi Valkeinen (1): drm/bridge: tc358767: Fix link properties discovery drivers/gpu/drm/bridge/tc358767.c | 32 1 file changed, 32 insertions(+) --- base-commit: 79d94360d50fcd487edcfe118a47a2881534923f change-id: 20231031-tc358767-58e3ebdf95f0 Best regards, What's the plan for these fixes? I'm not yet seeing them in Linus' tree, thus also not in stable. Jan The discussion continued in v2 of the series. There are some open question in the tc358767 behavior. Tomi
Re: [PATCH v2 0/4] drm: Fix errors about uninitialized/wrong variables
Hi all, On 03/11/2023 15:14, Tomi Valkeinen wrote: Fix cases where smatch reports a use of an uninitialized variable, and one where the variable is initialized but contains wrong value. Tomi Signed-off-by: Tomi Valkeinen --- Changes in v2: - Added two more fixes - Link to v1: https://lore.kernel.org/r/20230804-uninit-fixes-v1-0-a60772c04...@ideasonboard.com --- Tomi Valkeinen (4): drm/drm_file: fix use of uninitialized variable drm/framebuffer: Fix use of uninitialized variable drm/bridge: cdns-mhdp8546: Fix use of uninitialized variable drm/bridge: tc358767: Fix return value on error case drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-hdcp.c | 3 ++- drivers/gpu/drm/bridge/tc358767.c | 2 +- drivers/gpu/drm/drm_file.c | 2 +- drivers/gpu/drm/drm_framebuffer.c | 2 +- 4 files changed, 5 insertions(+), 4 deletions(-) --- base-commit: 9d7c8c066916f231ca0ed4e4fce6c4b58ca3e451 change-id: 20230804-uninit-fixes-188f92d60ac3 Best regards, Ping on this (or I'll forget the series...). Tomi
Re: [PATCH] drm/mipi-dsi: Fix detach call without attach
Hi mipi dsi maintainers (I'm not sure who that is =),
On 21/09/2023 13:50, Tomi Valkeinen wrote:
It's been reported that DSI host driver's detach can be called without
the attach ever happening:
https://lore.kernel.org/all/20230412073954.20601-1-t...@atomide.com/
After reading the code, I think this is what happens:
We have a DSI host defined in the device tree and a DSI peripheral under
that host (i.e. an i2c device using the DSI as data bus doesn't exhibit
this behavior).
The host driver calls mipi_dsi_host_register(), which causes (via a few
functions) mipi_dsi_device_add() to be called for the DSI peripheral. So
now we have a DSI device under the host, but attach hasn't been called.
Normally the probing of the devices continues, and eventually the DSI
peripheral's driver will call mipi_dsi_attach(), attaching the
peripheral.
However, if the host driver's probe encounters an error after calling
mipi_dsi_host_register(), and before the peripheral has called
mipi_dsi_attach(), the host driver will do cleanups and return an error
from its probe function. The cleanups include calling
mipi_dsi_host_unregister().
mipi_dsi_host_unregister() will call two functions for all its DSI
peripheral devices: mipi_dsi_detach() and mipi_dsi_device_unregister().
The latter makes sense, as the device exists, but the former may be
wrong as attach has not necessarily been done.
To fix this, track the attached state of the peripheral, and only detach
from mipi_dsi_host_unregister() if the peripheral was attached.
Note that I have only tested this with a board with an i2c DSI
peripheral, not with a "pure" DSI peripheral.
However, slightly related, the unregister machinery still seems broken.
E.g. if the DSI host driver is unbound, it'll detach and unregister the
DSI peripherals. After that, when the DSI peripheral driver unbound
it'll call detach either directly or using the devm variant, leading to
a crash. And probably the driver will crash if it happens, for some
reason, to try to send a message via the DSI bus.
But that's another topic.
Signed-off-by: Tomi Valkeinen
---
Any comments? I can push this via drm-misc, but I'd like an ack.
Tomi
drivers/gpu/drm/drm_mipi_dsi.c | 17 +++--
include/drm/drm_mipi_dsi.h | 2 ++
2 files changed, 17 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
index 14201f73aab1..843a6dbda93a 100644
--- a/drivers/gpu/drm/drm_mipi_dsi.c
+++ b/drivers/gpu/drm/drm_mipi_dsi.c
@@ -347,7 +347,8 @@ static int mipi_dsi_remove_device_fn(struct device *dev,
void *priv)
{
struct mipi_dsi_device *dsi = to_mipi_dsi_device(dev);
- mipi_dsi_detach(dsi);
+ if (dsi->attached)
+ mipi_dsi_detach(dsi);
mipi_dsi_device_unregister(dsi);
return 0;
@@ -370,11 +371,18 @@ EXPORT_SYMBOL(mipi_dsi_host_unregister);
int mipi_dsi_attach(struct mipi_dsi_device *dsi)
{
const struct mipi_dsi_host_ops *ops = dsi->host->ops;
+ int ret;
if (!ops || !ops->attach)
return -ENOSYS;
- return ops->attach(dsi->host, dsi);
+ ret = ops->attach(dsi->host, dsi);
+ if (ret)
+ return ret;
+
+ dsi->attached = true;
+
+ return 0;
}
EXPORT_SYMBOL(mipi_dsi_attach);
@@ -386,9 +394,14 @@ int mipi_dsi_detach(struct mipi_dsi_device *dsi)
{
const struct mipi_dsi_host_ops *ops = dsi->host->ops;
+ if (WARN_ON(!dsi->attached))
+ return -EINVAL;
+
if (!ops || !ops->detach)
return -ENOSYS;
+ dsi->attached = false;
+
return ops->detach(dsi->host, dsi);
}
EXPORT_SYMBOL(mipi_dsi_detach);
diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
index c9df0407980c..c0aec0d4d664 100644
--- a/include/drm/drm_mipi_dsi.h
+++ b/include/drm/drm_mipi_dsi.h
@@ -168,6 +168,7 @@ struct mipi_dsi_device_info {
* struct mipi_dsi_device - DSI peripheral device
* @host: DSI host for this peripheral
* @dev: driver model device node for this peripheral
+ * @attached: the DSI device has been successfully attached
* @name: DSI peripheral chip type
* @channel: virtual channel assigned to the peripheral
* @format: pixel format for video mode
@@ -184,6 +185,7 @@ struct mipi_dsi_device_info {
struct mipi_dsi_device {
struct mipi_dsi_host *host;
struct device dev;
+ bool attached;
char name[DSI_DEV_NAME_SIZE];
unsigned int channel;
---
base-commit: 9fc75c40faa29df14ba16066be6bdfaea9f39ce4
change-id: 20230921-dsi-detach-fix-6736f7a48ba7
Best regards,
Re: [PATCH v2 0/2] drm/bridge: tc358767: Fix DRM_BRIDGE_ATTACH_NO_CONNECTOR case
Hi, On 08/11/2023 14:45, Alexander Stein wrote: Hi Tomi, Am Mittwoch, 8. November 2023, 12:27:21 CET schrieb Tomi Valkeinen: These two patches are needed to make tc358767 work in the DRM_BRIDGE_ATTACH_NO_CONNECTOR case, at least when using a DP connector. I have tested this with TI AM654 EVM with a tc358767 add-on card connected to a DP monitor. Just a question regarding the usage of this DSI-DP bridge. What is the state of the DSI lanes after the DSI host has been initialized, but before calling atomic_pre_enable? AFAIK this bridge requires LP-11 on DSI at any time for accessing the AUX channel. We haven't received any test reports for the DSI-DP case... I was looking at the datasheet, and I wonder, why do you say the bridge requires DSI to be up for the AUX transactions? Tomi Best regards, Alexander Signed-off-by: Tomi Valkeinen --- Changes in v2: - Update the format negotiation patch as discussed in https://lore.kernel.org/all/7ddf0edb-2925-4b7c-ad07-27c030dd0...@ti.com/ - Link to v1: https://lore.kernel.org/r/20231031-tc358767-v1-0-392081ad9f4b@ideasonboard. com --- Aradhya Bhatia (1): drm/bridge: tc358767: Add format negotiation hooks for DPI/DSI to (e)DP Tomi Valkeinen (1): drm/bridge: tc358767: Fix link properties discovery drivers/gpu/drm/bridge/tc358767.c | 32 1 file changed, 32 insertions(+) --- base-commit: 9d7c8c066916f231ca0ed4e4fce6c4b58ca3e451 change-id: 20231031-tc358767-58e3ebdf95f0 Best regards,
Re: [PATCH v3 0/2] Add DSS support for TI AM62A7 SoC
On 08/11/2023 19:16, Aradhya Bhatia wrote: This patch series adds a new compatible for the Display SubSystem (DSS) controller on TI's AM62A7 SoC. It further adds the required support, for the same, in the tidss driver. The DSS controller is similar to the recently added AM625 DSS, with the key difference being the absence of VP1 output on the SoC. The VP1 in AM62A7 DSS is tied off and cannot be used, unlike in AM625, where the VP1 was connected to 2 OLDI TXes. The video pipeline that corresponds to VP1 still exists and can be used to overlay planes on the VP2's primary plane. This can be done using the overlay managers inside the SoC. Moreover, DSS VP2 can output Full-HD RGB888 DPI video signals. I have tested these patches on AM62A7 SK-EVM, which converts DPI signals to HDMI on the platform using the Sil9022A HDMI transmitter. All the patches, required to enable display on AM62A7-SK, can be found on my github fork[0] in the branch "next_am62a-v3". Regards Aradhya [0]: https://github.com/aradhya07/linux-ab/tree/next_am62a-v3 Change Log: V2 -> V3: - Add Krzysztof Kozlowski's R-b in patch 1/2. - Add new DISPC_VP_TIED_OFF for tied-off video-ports in patch 2/2. V1 -> V2: - Correctly sort DISPC_AM62A7 macro after DISPC_AM625 in patch 2/2. Previous Versions: V1: https://lore.kernel.org/all/20230818131750.4779-1-a-bhat...@ti.com/ V2: https://lore.kernel.org/all/20230818142124.8561-1-a-bhat...@ti.com/ Aradhya Bhatia (2): dt-bindings: display: ti: Add support for am62a7 dss drivers/tidss: Add support for AM62A7 DSS .../bindings/display/ti/ti,am65x-dss.yaml | 14 + drivers/gpu/drm/tidss/tidss_dispc.c | 59 +++ drivers/gpu/drm/tidss/tidss_dispc.h | 3 + drivers/gpu/drm/tidss/tidss_drv.c | 1 + 4 files changed, 77 insertions(+) base-commit: 2220f68f4504aa1ccce0fac721ccdb301e9da32f Thanks, I'm applying to drm-misc. Tomi
Re: [PATCH v3] drm: omapdrm: Improve check for contiguous buffers
On 13/11/2023 22:55, Andrew Davis wrote:
While a scatter-gather table having only 1 entry does imply it is
contiguous, it is a logic error to assume the inverse. Tables can have
more than 1 entry and still be contiguous. Use a proper check here.
Signed-off-by: Andrew Davis
---
Thanks, I'll pick this up.
Tomi
Changes from v2:
- Double check that these multi-segment SGTs are handled correctly elsewhere
in the driver
- Rebase on v6.7-rc1
Changes from v1:
- Sent correct version of patch :)
drivers/gpu/drm/omapdrm/omap_gem.c | 14 ++
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c
b/drivers/gpu/drm/omapdrm/omap_gem.c
index c48fa531ca321..3421e8389222a 100644
--- a/drivers/gpu/drm/omapdrm/omap_gem.c
+++ b/drivers/gpu/drm/omapdrm/omap_gem.c
@@ -48,7 +48,7 @@ struct omap_gem_object {
* OMAP_BO_MEM_DMA_API flag set)
*
* - buffers imported from dmabuf (with the OMAP_BO_MEM_DMABUF flag set)
-* if they are physically contiguous (when sgt->orig_nents == 1)
+* if they are physically contiguous
*
* - buffers mapped through the TILER when pin_cnt is not zero, in which
* case the DMA address points to the TILER aperture
@@ -148,12 +148,18 @@ u64 omap_gem_mmap_offset(struct drm_gem_object *obj)
return drm_vma_node_offset_addr(>vma_node);
}
+static bool omap_gem_sgt_is_contiguous(struct sg_table *sgt, size_t size)
+{
+ return !(drm_prime_get_contiguous_size(sgt) < size);
+}
+
static bool omap_gem_is_contiguous(struct omap_gem_object *omap_obj)
{
if (omap_obj->flags & OMAP_BO_MEM_DMA_API)
return true;
- if ((omap_obj->flags & OMAP_BO_MEM_DMABUF) && omap_obj->sgt->nents == 1)
+ if ((omap_obj->flags & OMAP_BO_MEM_DMABUF) &&
+ omap_gem_sgt_is_contiguous(omap_obj->sgt, omap_obj->base.size))
return true;
return false;
@@ -1385,7 +1391,7 @@ struct drm_gem_object *omap_gem_new_dmabuf(struct
drm_device *dev, size_t size,
union omap_gem_size gsize;
/* Without a DMM only physically contiguous buffers can be supported. */
- if (sgt->orig_nents != 1 && !priv->has_dmm)
+ if (!omap_gem_sgt_is_contiguous(sgt, size) && !priv->has_dmm)
return ERR_PTR(-EINVAL);
gsize.bytes = PAGE_ALIGN(size);
@@ -1399,7 +1405,7 @@ struct drm_gem_object *omap_gem_new_dmabuf(struct
drm_device *dev, size_t size,
omap_obj->sgt = sgt;
- if (sgt->orig_nents == 1) {
+ if (omap_gem_sgt_is_contiguous(sgt, size)) {
omap_obj->dma_addr = sg_dma_address(sgt->sgl);
} else {
/* Create pages list from sgt */
Re: [PATCH v3 0/2] Add DSS support for TI AM62A7 SoC
Hi, On 08/11/2023 19:16, Aradhya Bhatia wrote: This patch series adds a new compatible for the Display SubSystem (DSS) controller on TI's AM62A7 SoC. It further adds the required support, for the same, in the tidss driver. The DSS controller is similar to the recently added AM625 DSS, with the key difference being the absence of VP1 output on the SoC. The VP1 in AM62A7 DSS is tied off and cannot be used, unlike in AM625, where the VP1 was connected to 2 OLDI TXes. The video pipeline that corresponds to VP1 still exists and can be used to overlay planes on the VP2's primary plane. This can be done using the overlay managers inside the SoC. Moreover, DSS VP2 can output Full-HD RGB888 DPI video signals. I have tested these patches on AM62A7 SK-EVM, which converts DPI signals to HDMI on the platform using the Sil9022A HDMI transmitter. All the patches, required to enable display on AM62A7-SK, can be found on my github fork[0] in the branch "next_am62a-v3". Regards Aradhya [0]: https://github.com/aradhya07/linux-ab/tree/next_am62a-v3 Change Log: V2 -> V3: - Add Krzysztof Kozlowski's R-b in patch 1/2. - Add new DISPC_VP_TIED_OFF for tied-off video-ports in patch 2/2. V1 -> V2: - Correctly sort DISPC_AM62A7 macro after DISPC_AM625 in patch 2/2. Previous Versions: V1: https://lore.kernel.org/all/20230818131750.4779-1-a-bhat...@ti.com/ V2: https://lore.kernel.org/all/20230818142124.8561-1-a-bhat...@ti.com/ Aradhya Bhatia (2): dt-bindings: display: ti: Add support for am62a7 dss drivers/tidss: Add support for AM62A7 DSS .../bindings/display/ti/ti,am65x-dss.yaml | 14 + drivers/gpu/drm/tidss/tidss_dispc.c | 59 +++ drivers/gpu/drm/tidss/tidss_dispc.h | 3 + drivers/gpu/drm/tidss/tidss_drv.c | 1 + 4 files changed, 77 insertions(+) base-commit: 2220f68f4504aa1ccce0fac721ccdb301e9da32f For the series: Reviewed-by: Tomi Valkeinen Tomi
