Re: [PATCH 0/6] drm/vc4: dsi: Conversion to bridge
On Wed, 07 Dec 2022 11:22:43 +0100, Maxime Ripard wrote: > This series converts the vc4 DSI driver to a bridge. It's been in use for a > while on the downstream tree. > > Let me know what you think, > Maxime > > > [...] Applied to drm/drm-misc (drm-misc-next). Thanks! Maxime
Re: (subset) [PATCH] drm/vc4: Improve the KUnit documentation
On Thu, 8 Dec 2022 10:47:27 +0100, Maxime Ripard wrote: > The command-line can be expressed using a code-block, and we were > missing which architectures were available. > > Applied to drm/drm-misc (drm-misc-next). Thanks! Maxime
[GIT PULL] fbdev fixes and updates for v6.2-rc1
Hi Linus, please pull fbdev fixes and updates for kernel 6.2-rc1. Most relevant are the patches from Dmitry Torokhov to switch omapfb to the gpiod API. The other patches are small and fix e.g. UML build issues, improve the error paths and cleanup code. Thanks, Helge The following changes since commit c7020e1b346d5840e93b58cc4f2c67fc645d8df9: Merge tag 'pci-v6.2-changes' of git://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci (2022-12-14 09:54:10 -0800) are available in the Git repository at: http://git.kernel.org/pub/scm/linux/kernel/git/deller/linux-fbdev.git tags/fbdev-for-6.2-rc1 for you to fetch changes up to 3c3bfb8586f848317ceba5d777e11204ba3e5758: fbdev: fbcon: release buffer when fbcon_do_set_font() failed (2022-12-14 20:01:51 +0100) fbdev updates and fixes for kernel 6.2-rc1: Switch omapfb to the gpiod API, some failure path fixes and UML build fixes. Andy Shevchenko (2): fbdev: ssd1307fb: Drop optional dependency fbdev: ssd1307fb: Drop duplicate NULL checks for PWM APIs Christophe JAILLET (2): fbdev: uvesafb: Fixes an error handling path in uvesafb_probe() fbdev: uvesafb: Simplify uvesafb_remove() Colin Ian King (1): fbdev: omapfb: remove redundant variable checksum Dmitry Torokhov (13): fbdev: omapfb: connector-hdmi: switch to using gpiod API fbdev: omapfb: panel-sony-acx565akm: remove support for platform data fbdev: omapfb: panel-sony-acx565akm: switch to using gpiod API fbdev: omapfb: encoder-tfp410: switch to using gpiod API fbdev: omapfb: panel-dsi-cm: switch to using gpiod API fbdev: omapfb: panel-tpo-td043mtea1: switch to using gpiod API fbdev: omapfb: panel-nec-nl8048hl11: switch to using gpiod API fbdev: omapfb: panel-dpi: remove support for platform data fbdev: omapfb: connector-analog-tv: remove support for platform data fbdev: omapfb: encoder-opa362: fix included headers fbdev: omapfb: panel-lgphilips-lb035q02: remove backlight GPIO handling fbdev: omapfb: panel-tpo-td028ttec1: stop including gpio.h fbdev: omapfb: panel-sharp-ls037v7dw01: fix included headers Dongliang Mu (2): fbdev: smscufx: fix error handling code in ufx_usb_probe fbdev: da8xx-fb: add missing regulator_disable() in fb_probe Gaosheng Cui (1): fbdev: ep93xx-fb: Add missing clk_disable_unprepare in ep93xxfb_probe() Randy Dunlap (2): fbdev: geode: don't build on UML fbdev: uvesafb: don't build on UML Shang XiaoJing (1): fbdev: via: Fix error in via_core_init() Tetsuo Handa (1): fbdev: fbcon: release buffer when fbcon_do_set_font() failed Uwe Kleine-König (1): fbdev: matroxfb: Convert to i2c's .probe_new() Xiongfeng Wang (1): fbdev: vermilion: decrease reference count in error path Yang Yingliang (1): fbdev: pm2fb: fix missing pci_disable_device() Yu Zhe (1): fbdev: controlfb: fix spelling mistake "paramaters"->"parameters" wangkail...@jari.cn (1): fbdev: pxafb: Remove unnecessary print function dev_err() ye xingchen (2): fbdev: uvesafb: use sysfs_emit() to instead of scnprintf() fbdev: sh_mobile_lcdcfb: use sysfs_emit() to instead of scnprintf() drivers/video/fbdev/Kconfig| 2 +- drivers/video/fbdev/controlfb.c| 2 +- drivers/video/fbdev/core/fbcon.c | 3 +- drivers/video/fbdev/da8xx-fb.c | 7 +- drivers/video/fbdev/ep93xx-fb.c| 4 +- drivers/video/fbdev/geode/Kconfig | 1 + drivers/video/fbdev/matrox/matroxfb_maven.c| 5 +- .../omap2/omapfb/displays/connector-analog-tv.c| 60 ++- .../fbdev/omap2/omapfb/displays/connector-hdmi.c | 49 +++-- .../fbdev/omap2/omapfb/displays/encoder-opa362.c | 4 +- .../fbdev/omap2/omapfb/displays/encoder-tfp410.c | 67 .../video/fbdev/omap2/omapfb/displays/panel-dpi.c | 83 ++- .../fbdev/omap2/omapfb/displays/panel-dsi-cm.c | 116 - .../omapfb/displays/panel-lgphilips-lb035q02.c | 21 +--- .../omap2/omapfb/displays/panel-nec-nl8048hl11.c | 72 - .../omapfb/displays/panel-sharp-ls037v7dw01.c | 3 +- .../omap2/omapfb/displays/panel-sony-acx565akm.c | 105 ++- .../omap2/omapfb/displays/panel-tpo-td028ttec1.c | 1 - .../omap2/omapfb/displays/panel-tpo-td043mtea1.c | 59 +++ drivers/video/fbdev/omap2/omapfb/dss/hdmi5_core.c | 2 - drivers/video/fbdev/pm2fb.c| 9 +- drivers/video/fbdev/pxafb.c| 1 - drivers/video/fbdev/sh_mobile_lcdcfb.c | 8 +- drivers/video/fbdev/smscufx.c | 46 +--- drivers/video/fbdev/ssd1307fb.c| 12 +--
[PATCH] backlight: backlight: Fix doc for backlight_device_get_by_name
backlight_put() has been dropped, we should call put_device() to drop the reference taken by backlight_device_get_by_name(). Fixes: 0f6a3256fd81 ("backlight: backlight: Drop backlight_put()") Signed-off-by: Miaoqian Lin --- drivers/video/backlight/backlight.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c index b788ff3d0f45..6eea72aa8dbf 100644 --- a/drivers/video/backlight/backlight.c +++ b/drivers/video/backlight/backlight.c @@ -501,7 +501,7 @@ EXPORT_SYMBOL(backlight_device_get_by_type); * * This function looks up a backlight device by its name. It obtains a reference * on the backlight device and it is the caller's responsibility to drop the - * reference by calling backlight_put(). + * reference by calling put_device(). * * Returns: * A pointer to the backlight device if found, otherwise NULL. -- 2.25.1
Re: [PATCH] drivers: staging: fbtft: Replace usage of udelay
On Thu, Dec 15, 2022 at 01:37:46AM +, Haris M. Bhatti wrote: > checkpatch highlighted that use of udelay should be replaced by > usleep_range. > --- > drivers/staging/fbtft/fb_ra8875.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/staging/fbtft/fb_ra8875.c > b/drivers/staging/fbtft/fb_ra8875.c > index 398bdbf53c9a..75cf3bb18414 100644 > --- a/drivers/staging/fbtft/fb_ra8875.c > +++ b/drivers/staging/fbtft/fb_ra8875.c > @@ -217,7 +217,7 @@ static void write_reg8_bus8(struct fbtft_par *par, int > len, ...) > } > len--; > > - udelay(100); > + usleep_range(100, 101); > > if (len) { > buf = (u8 *)par->buf; > @@ -238,7 +238,7 @@ static void write_reg8_bus8(struct fbtft_par *par, int > len, ...) > > /* restore user spi-speed */ > par->fbtftops.write = fbtft_write_spi; > - udelay(100); > + usleep_range(100, 101); > } > > static int write_vmem16_bus8(struct fbtft_par *par, size_t offset, size_t > len) > -- > 2.38.1 > > Hi, This is the friendly patch-bot of Greg Kroah-Hartman. You have sent him a patch that has triggered this response. He used to manually respond to these common problems, but in order to save his sanity (he kept writing the same thing over and over, yet to different people), I was created. Hopefully you will not take offence and will fix the problem in your patch and resubmit it so that it can be accepted into the Linux kernel tree. You are receiving this message because of the following common error(s) as indicated below: - Your patch does not have a Signed-off-by: line. Please read the kernel file, Documentation/SubmittingPatches and resend it after adding that line. Note, the line needs to be in the body of the email, before the patch, not at the bottom of the patch or in the email signature. - You did not specify a description of why the patch is needed, or possibly, any description at all, in the email body. Please read the section entitled "The canonical patch format" in the kernel file, Documentation/SubmittingPatches for what is needed in order to properly describe the change. - You did not write a descriptive Subject: for the patch, allowing Greg, and everyone else, to know what this patch is all about. Please read the section entitled "The canonical patch format" in the kernel file, Documentation/SubmittingPatches for what a proper Subject: line should look like. If you wish to discuss this problem further, or you have questions about how to resolve this issue, please feel free to respond to this email and Greg will reply once he has dug out from the pending patches received from other developers. thanks, greg k-h's patch email bot
Re: renesas/master bisection: igt-kms-rockchip.kms_vblank.pipe-A-wait-forked on rk3399-gru-kevin
On Tue, Dec 13, 2022 at 04:51:11PM +, Mark Brown wrote: > On Tue, Dec 13, 2022 at 05:56:30AM -0800, KernelCI bot wrote: > > The KernelCI bisection bot found regressions in at least two KMS tests > in the Renesas tree on rk3399-gru-kevin just after the Renesas tree > merged up mainline: > >igt-kms-rockchip.kms_vblank.pipe-A-wait-forked >igt-kms-rockchip.kms_vblank.pipe-A-query-busy > > which it bisected to ca871659ec16 ("drm/bridge: analogix_dp: Support > PSR-exit to disable transition"). I'm not *100%* sure I trust the > bisection but it sure is suspicous that two separate bisects for related > issues landed on the same commit. > > Below is the full report for the bisect for the first test, the bisect > for the latter looks identical. It's got links to full logs for the > test run and a Reported-by for the bot - I do see some backtraces from > userspace in the output, the first is: I think the backtraces are just because IGT calls assert(). > | IGT-Version: 1.26-gf8a4a0b (aarch64) (Linux: 6.1.0 aarch64) > | <14>[ 35.48] [IGT] drm_read: starting subtest short-buffer-wakeup > | Starting subtest: short-buffer-wakeup > | > | (| drm_read:350) CRITICAL: Test assertion failure function generate_event, > file ../tests/drm_read.c:65: > | (drm_read:350) CRITICAL: <14>[ 36.155642] [IGT] drm_read: exiting, ret=98 > | Failed assertion: kmstest_get_vblank(fd, pipe, DRM_VBLANK_EVENT) > | > | (drm_read:350) CRITICAL: Last errno: 22, Invalid argument > | Stack trace: > | > | #0 ../lib/igt_core.c:1933 __igt_fail_assert() > | #1 [+0xd5362770] > | #2 [+0xd536193c] > | #3 [__libc_start_main+0xe8] > | #4 [+0xd5361974] > | #5 [[ 36.162851] Console: switching to colour frame buffer > device 300x100>+0xd5361974] > | Subtest short-buffer-wakeup failed. > > Unfortunately we don't have current results from mainline or -next. Thanks for the notification. I'm running: 6e516faf0431 drm/panfrost: Job should reference MMU not file_priv which is allegedly a "good" kernel. And running this: ### First kill my display manager, etc. ## Then: IGT_FORCE_DRIVER=rockchip /usr/libexec/igt-gpu-tools/kms_vblank --run-subtest pipe-A-wait-forked Gives the appended log [1]. If I'm looking right, it seems like it's failing the same way as the "regression." I also tested this: # the 5.10.x backport, and its parent: dbe04e874d4fbd56be64fdfcb29410241b6ad08a^ -- i.e.: 61297ee0c329 Input: bcm5974 - set missing URB_NO_TRANSFER_DMA_MAP urb flag and saw the same failures, and I also see the failures I was trying to avoid in this series (see e54a4424925a ("drm/atomic: Force bridge self-refresh-exit on CRTC switch")). Perhaps that's because of the "First kill my display manager, etc." -- because that step means we might end up switching CRTCs (VOPs) when the test starts, and triggering the bug. I'll look some more, but this might be a difference of test setup, such that my setup has the issue before and after that commit, but your setup sees a regression. Small tangent: It's possible this is similar to stuff I had to debug a while back in this space: atomictest: Disable CRTCs before test https://chromium-review.googlesource.com/c/chromiumos/platform/drm-tests/+/3309960 null_platform_test: Disable CRTCs first https://chromium-review.googlesource.com/c/chromiumos/platform/drm-tests/+/3315478 In that case, there was actually an underlying kernel regression due to: 846c7dfc1193 drm/atomic: Try to preserve the crtc enabled state in drm_atomic_remove_fb, v2. But our tests were broken too (assuming an initial state that wasn't guaranteed), so we just fixed the tests. Anyway, I'll confirm when I get some fresh eyes and can try a few more things (like neutering the ChromeOS display framework for my tests). Brian [1] IGT-Version: 1.26-gf8a4a0b5 (arm) (Linux: 5.18.0-rc6+ aarch64) Starting subtest: pipe-A-wait-forked Beginning pipe-A-wait-forked on pipe A, connector eDP-1 (kms_vblank:2532) CRITICAL: Test assertion failure function vblank_wait, file ../igt-gpu-tools-1.25/tests/kms_vblank.c:319: (kms_vblank:2535) CRITICAL: Test assertion failure function vblank_wait, file ../igt-gpu-tools-1.25/tests/kms_vblank.c:319: (kms_vblank:2531) CRITICAL: Test assertion failure function vblank_wait, file ../igt-gpu-tools-1.25/tests/kms_vblank.c:319: (kms_vblank:2532) CRITICAL: Failed assertion: wait_vblank(fd, ) == 0 (kms_vblank:2534) CRITICAL: Test assertion failure function vblank_wait, file ../igt-gpu-tools-1.25/tests/kms_vblank.c:319: (kms_vblank:2535) CRITICAL: Failed assertion: wait_vblank(fd, ) == 0 (kms_vblank:2531) CRITICAL: Failed assertion: wait_vblank(fd, ) == 0 (kms_vblank:2536) CRITICAL: Test assertion failure function vblank_wait, file ../igt-gpu-tools-1.25/tests/kms_vblank.c:319: (kms_vblank:2532) CRITICAL: Last errno: 22, Invalid argument (kms_vblank:2534) CRITICAL: Failed assertion: wait_vblank(fd, ) == 0 (kms_vblank:2536) CRITICAL: Failed assertion: wait_vblank(fd, ) == 0
[RESEND PATCH] drm/plane-helper: Add the missing declaration of drm_atomic_state
Add the missing declaration of struct drm_atomic_state to fix the compile error below: error: 'struct drm_atomic_state' declared inside parameter list will not be visible outside of this definition or declaration [-Werror] Signed-off-by: Ma Jun --- include/drm/drm_plane_helper.h | 1 + 1 file changed, 1 insertion(+) diff --git a/include/drm/drm_plane_helper.h b/include/drm/drm_plane_helper.h index b00ad36cf5b6..530f88176db4 100644 --- a/include/drm/drm_plane_helper.h +++ b/include/drm/drm_plane_helper.h @@ -30,6 +30,7 @@ struct drm_crtc; struct drm_framebuffer; struct drm_modeset_acquire_ctx; struct drm_plane; +struct drm_atomic_state; int drm_plane_helper_update_primary(struct drm_plane *plane, struct drm_crtc *crtc, struct drm_framebuffer *fb, -- 2.25.1
Re: [PATCH] drm/msm/dp: do not complete dp_aux_cmd_fifo_tx() if irq is not for aux transfer
Hi Stephen On 12/14/2022 4:29 PM, Stephen Boyd wrote: Quoting Doug Anderson (2022-12-14 16:14:42) Hi, On Wed, Dec 14, 2022 at 3:46 PM Abhinav Kumar wrote: Hi Doug On 12/14/2022 2:29 PM, Doug Anderson wrote: Hi, On Wed, Dec 14, 2022 at 1:21 PM Kuogee Hsieh wrote: There are 3 possible interrupt sources are handled by DP controller, HPDstatus, Controller state changes and Aux read/write transaction. At every irq, DP controller have to check isr status of every interrupt sources and service the interrupt if its isr status bits shows interrupts are pending. There is potential race condition may happen at current aux isr handler implementation since it is always complete dp_aux_cmd_fifo_tx() even irq is not for aux read or write transaction. This may cause aux read transaction return premature if host aux data read is in the middle of waiting for sink to complete transferring data to host while irq happen. This will cause host's receiving buffer contains unexpected data. This patch fixes this problem by checking aux isr and return immediately at aux isr handler if there are no any isr status bits set. Follows are the signature at kernel logs when problem happen, EDID has corrupt header panel-simple-dp-aux aux-aea.edp: Couldn't identify panel via EDID panel-simple-dp-aux aux-aea.edp: error -EIO: Couldn't detect panel nor find a fallback Signed-off-by: Kuogee Hsieh --- drivers/gpu/drm/msm/dp/dp_aux.c | 7 +++ 1 file changed, 7 insertions(+) diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c index d030a93..8f8b12a 100644 --- a/drivers/gpu/drm/msm/dp/dp_aux.c +++ b/drivers/gpu/drm/msm/dp/dp_aux.c @@ -423,6 +423,13 @@ void dp_aux_isr(struct drm_dp_aux *dp_aux) isr = dp_catalog_aux_get_irq(aux->catalog); + /* +* if this irq is not for aux transfer, +* then return immediately +*/ Why do you need 4 lines for a comment that fits on one line? Yes, we can fit this to one line. + if (!isr) + return; I can confirm that this works for me. I could reproduce the EDID problems in the past and I can't after this patch. ...so I could give a: Tested-by: Douglas Anderson I'm not an expert on this part of the code, so feel free to ignore my other comments if everyone else thinks this patch is fine as-is, but to me something here feels a little fragile. It feels a little weird that we'll "complete" for _any_ interrupt that comes through now rather than relying on dp_aux_native_handler() / dp_aux_i2c_handler() to specifically identify interrupts that caused the end of the transfer. I guess that idea is that every possible interrupt we get causes the end of the transfer? -Doug So this turned out to be more tricky and was a good finding from kuogee. In the bad EDID case, it was technically not bad EDID. What was happening was, the VIDEO_READY interrupt was continuously firing. Ideally, this should fire only once but due to some error condition it kept firing. We dont exactly know why yet what was the error condition making it continuously fire. This is a great detail that is missing from the commit text. Yup, we should update this. In the DP ISR, the dp_aux_isr() gets called even if it was not an aux interrupt which fired (so the call flow in this case was dp_display_irq_handler (triggered for VIDEO_READY) ---> dp_aux_isr() So we should certainly have some protection to return early from this routine if there was no aux interrupt which fired. I'm not sure that's a race condition though, more like a problem where the completion is called unconditionally? hmm ... True. Which is what this fix is doing. Its not completing any interrupt, its just returning early if no aux interrupt fired. ...but the whole problem was that it was doing the complete() at the end, right? Kuogee even mentioned that in the commit message. Specifically, I checked dp_aux_native_handler() and dp_aux_i2c_handler(), both of which are passed the "isr". Unless I messed up, both functions already were no-ops if the ISR was 0, even before Kuogee's patch. That means that the only thing Kuogee's patch does is to prevent the call to "complete(>comp)" at the end of "dp_aux_isr()". ...and it makes sense not to call the complete() if no "isr" is 0. ...but what I'm saying is that _any_ non-zero value of ISR will still cause the complete() to be called after Kuogee's patch. That means that if any of the 32-bits in the "isr" variable are set, that we will call complete(). I'm asking if you're sure that every single bit of the "isr" means that we're ready to call complete(). It feels like it would be less fragile if dp_aux_native_handler() and dp_aux_i2c_handler() (which both already look at the ISR) returned some value saying whether the "isr" contained a bit that meant that complete() should be called. I'm almost certain I've asked for this before, but I can't find it anymore. Can we also simplify the aux handlers to be
Re: [RFC PATCH 3/6] drm/msm/dpu1: Wire up DSC mask for active CTL configuration
On 14/12/2022 21:30, Marijn Suijten wrote: On 2022-12-14 20:43:29, Dmitry Baryshkov wrote: On 14/12/2022 01:22, Marijn Suijten wrote: Active CTLs have to configure what DSC block(s) have to be enabled, and what DSC block(s) have to be flushed; this value was initialized to zero resulting in the necessary register writes to never happen (or would write zero otherwise). This seems to have gotten lost in the DSC v4->v5 series while refactoring how the combination with merge_3d was handled. Fixes: 58dca9810749 ("drm/msm/disp/dpu1: Add support for DSC in encoder") Signed-off-by: Marijn Suijten --- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 1 + drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 1 + drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c | 2 ++ 3 files changed, 4 insertions(+) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c index ae28b2b93e69..35791f93c33d 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c @@ -61,6 +61,7 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg( intf_cfg.intf_mode_sel = DPU_CTL_MODE_SEL_CMD; intf_cfg.stream_sel = cmd_enc->stream_sel; intf_cfg.mode_3d = dpu_encoder_helper_get_3d_blend_mode(phys_enc); + intf_cfg.dsc = dpu_encoder_helper_get_dsc(phys_enc); ctl->ops.setup_intf_cfg(ctl, _cfg); /* setup which pp blk will connect to this intf */ diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c index 0f71e8fe7be7..9ee3a7306a5f 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c @@ -274,6 +274,7 @@ static void dpu_encoder_phys_vid_setup_timing_engine( intf_cfg.intf_mode_sel = DPU_CTL_MODE_SEL_VID; intf_cfg.stream_sel = 0; /* Don't care value for video mode */ intf_cfg.mode_3d = dpu_encoder_helper_get_3d_blend_mode(phys_enc); + intf_cfg.dsc = dpu_encoder_helper_get_dsc(phys_enc); if (phys_enc->hw_pp->merge_3d) intf_cfg.merge_3d = phys_enc->hw_pp->merge_3d->idx; diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c index 7cbcef6efe17..92ddf9995b37 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c @@ -209,6 +209,7 @@ static void dpu_encoder_phys_wb_setup_cdp(struct dpu_encoder_phys *phys_enc) intf_cfg.intf = DPU_NONE; intf_cfg.wb = hw_wb->idx; + intf_cfg.dsc = dpu_encoder_helper_get_dsc(phys_enc); We usually don't have DSC with the writeback, don't we? I am unsure so ended up adding them in writeback regardless. Downstream uses a separate callback to process intf_cfg.dsc instead of going through setup_intf_cfg(). To prevent these from being missed again (in the case of copy), how about instead having some function that sets up intf_cfg with these default values from a phys_enc? That way most of this remains oblivious to the caller. I'm not sure this is possible. E.g. intf_cfg.dsc should not be set for the WB. On the same note, that callback on non-DPU_CTL_ACTIVE_CFG hardware doesn't use the intf_cfg.dsc member anyway, but it was again added to keep the blocks somewhat consistent (in case it ever becomes used?). if (mode_3d && hw_pp && hw_pp->merge_3d) intf_cfg.merge_3d = hw_pp->merge_3d->idx; @@ -230,6 +231,7 @@ static void dpu_encoder_phys_wb_setup_cdp(struct dpu_encoder_phys *phys_enc) intf_cfg.wb = hw_wb->idx; intf_cfg.mode_3d = dpu_encoder_helper_get_3d_blend_mode(phys_enc); + intf_cfg.dsc = dpu_encoder_helper_get_dsc(phys_enc); phys_enc->hw_ctl->ops.setup_intf_cfg(phys_enc->hw_ctl, _cfg); } } - Marijn -- With best wishes Dmitry
Re: [RFC PATCH 0/6] drm/msm: DSC Electric Boogaloo for sm8[12]50
On 14/12/2022 21:23, Marijn Suijten wrote: On 2022-12-14 20:40:06, Dmitry Baryshkov wrote: On 14/12/2022 01:22, Marijn Suijten wrote: This preliminary Display Stream Compression support package for (initially tested on) sm8[12]50 is based on comparing DSC behaviour between downstream and mainline. Some new callbacks are added (for binding blocks on active CTLs), logic bugs are corrected, zeroed struct members are now assigned proper values, and RM allocation and hw block retrieval now hand out (or not) DSC blocks without causing null-pointer dereferences. Unfortunately it is not yet enough to get rid of completely corrupted display output on the boards I tested here: - Sony Xperia 1 (sm8150), 1644x3840 or 1096x2560 pixels; - Sony Xperia 5II (sm8250), 1080x2520, at 60 or 120Hz; - (can include more Xperia boards if desired) Both devices use the DUALPIPE_DSCMERGE topology downstream: dual LM, PP and DSC, but only a single INTF/encoder/DSI-link. Hopefully this spawns some community/upstream interest to help rootcause our corruption issues (after we open a drm/msm report on GitLab for more appropriate tracking). The Sony Xperia XZ3 (sdm845) was fully tested and validated with this series to not cause any regressions (an one of the math fixes now allows us to change slice_count in the panel driver, which would corrupt previously). Marijn Suijten (6): drm/msm/dpu1: Implement DSC binding to PP block for CTL V1 drm/msm/dpu1: Add DSC config for sm8150 and sm8250 drm/msm/dpu1: Wire up DSC mask for active CTL configuration drm/msm/dsi: Use DSC slice(s) packet size to compute word count drm/msm/dsi: Flip greater-than check for slice_count and slice_per_intf drm/msm/dpu: Disallow unallocated (DSC) resources to be returned General comment: patches with Fixes ideally should come first. Usually they are picked into -fixes and/or stable kernels. If the Fixes patches are in the middle of the series, one can not be sure that they do not have dependencies on previous patches. If there is one, it should probably be stated clearly to ease work on backporting them. Ack, I may have rushed these RFC patches straight off my branches onto the lists in hopes of sparking some suggestions on what may still be broken or missing to get DSC working on sm[12]50, but will keep this in mind for v2 after receiving some more review. That said, any suggestions? From what I've noticed lately: - set dsc_version_major/dsc_version_minor - try using dsc params from 1.2 rater than 1.1 version spec (there is small difference there) -- With best wishes Dmitry
Re: [PATCH v12 2/5] dt-bindings: msm/dp: add data-lanes and link-frequencies property
Quoting Kuogee Hsieh (2022-12-14 14:56:23) > > On 12/13/2022 3:06 PM, Stephen Boyd wrote: > > Quoting Kuogee Hsieh (2022-12-13 13:44:05) > >> Add both data-lanes and link-frequencies property into endpoint > > Why do we care? Please tell us why it's important. Any response? > >> @@ -193,6 +217,8 @@ examples: > >> reg = <1>; > >> endpoint { > >> remote-endpoint = <>; > >> +data-lanes = <0 1>; > >> +link-frequencies = /bits/ 64 <162000 27 > >> 54 81>; > >> }; > > So far we haven't used the output port on the DP controller in DT. > > > > I'm still not clear on what we should do in general for DP because > > there's a PHY that actually controls a lane count and lane mapping. In > > my mental model of the SoC, this DP controller's output port is > > connected to the DP PHY, which then sends the DP lanes out of the SoC to > > the next downstream device (i.e. a DP connector or type-c muxer). Having > > a remote-endpoint property with a phandle to typec doesn't fit my mental > > model. I'd expect it to be the typec PHY. > ack > > > > That brings up the question: when we have 2 lanes vs. 4 lanes will we > > duplicate the data-lanes property in the PHY binding? I suspect we'll > > have to. Hopefully that sort of duplication is OK? > Current we have limitation by reserve 2 data lanes for usb2, i am not > sure duplication to 4 lanes will work automatically. > > > > Similarly, we may have a redriver that limits the link-frequencies > > property further (e.g. only support <= 2.7GHz). Having multiple > > link-frequencies along the graph is OK, right? And isn't the > > link-frequencies property known here by fact that the DP controller > > tells us which SoC this controller is for, and thus we already know the > > supported link frequencies? > > > > Finally, I wonder if we should put any of this in the DP controller's > > output endpoint, or if we can put these sorts of properties in the DP > > PHY binding directly? Can't we do that and then when the DP controller > > tries to set 4 lanes, the PHY immediately fails the call and the link > > training algorithm does its thing and tries fewer lanes? And similarly, > > if link-frequencies were in the PHY's binding, the PHY could fail to set > > those frequencies during link training, returning an error to the DP > > controller, letting the training move on to a lower frequency. If we did > > that this patch series would largely be about modifying the PHY binding, > > updating the PHY driver to enforce constraints, and handling errors > > during link training in the DP controller (which may already be done? I > > didn't check). > > > phy/pll have different configuration base on link lanes and rate. > > it has to be set up before link training can start. > > Once link training start, then there are no any interactions between > controller and phy during link training session. What do you mean? The DP controller calls phy_configure() and changes the link rate. The return value from phy_configure() should be checked and link training should skip link rates that aren't supported and/or number of lanes that aren't supported. > > Link training only happen between dp controller and sink since link > status is reported by sink (read back from sink's dpcd register directly). > > T achieve link symbol locked, link training will start from reduce link > rate until lowest rate, if it still failed, then it will reduce lanes > with highest rate and start training again. > > it will repeat same process until lowest lane (one lane), if it still > failed, then it will give up and declare link training failed. Yes, that describes the link training algorithm. I don't see why phy_configure() return value can't be checked and either number of lanes or link frequencies be checked. If only two lanes are supported, then phy_configure() will fail for the 4 link rates and the algorithm will reduce the number of lanes and go back to the highest rate. Then when the highest rate isn't supported it will drop link rate until the link rate is supported. > > Therefore I think add data-lanes and link-frequencies properties in the > DP PHY binding directly will not helps. > I didn't follow your logic. Sorry.
Re: [Freedreno] [PATCH] drm/msm/dp: do not complete dp_aux_cmd_fifo_tx() if irq is not for aux transfer
Hi Doug On 12/14/2022 4:14 PM, Doug Anderson wrote: Hi, On Wed, Dec 14, 2022 at 3:46 PM Abhinav Kumar wrote: Hi Doug On 12/14/2022 2:29 PM, Doug Anderson wrote: Hi, On Wed, Dec 14, 2022 at 1:21 PM Kuogee Hsieh wrote: There are 3 possible interrupt sources are handled by DP controller, HPDstatus, Controller state changes and Aux read/write transaction. At every irq, DP controller have to check isr status of every interrupt sources and service the interrupt if its isr status bits shows interrupts are pending. There is potential race condition may happen at current aux isr handler implementation since it is always complete dp_aux_cmd_fifo_tx() even irq is not for aux read or write transaction. This may cause aux read transaction return premature if host aux data read is in the middle of waiting for sink to complete transferring data to host while irq happen. This will cause host's receiving buffer contains unexpected data. This patch fixes this problem by checking aux isr and return immediately at aux isr handler if there are no any isr status bits set. Follows are the signature at kernel logs when problem happen, EDID has corrupt header panel-simple-dp-aux aux-aea.edp: Couldn't identify panel via EDID panel-simple-dp-aux aux-aea.edp: error -EIO: Couldn't detect panel nor find a fallback Signed-off-by: Kuogee Hsieh --- drivers/gpu/drm/msm/dp/dp_aux.c | 7 +++ 1 file changed, 7 insertions(+) diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c index d030a93..8f8b12a 100644 --- a/drivers/gpu/drm/msm/dp/dp_aux.c +++ b/drivers/gpu/drm/msm/dp/dp_aux.c @@ -423,6 +423,13 @@ void dp_aux_isr(struct drm_dp_aux *dp_aux) isr = dp_catalog_aux_get_irq(aux->catalog); + /* +* if this irq is not for aux transfer, +* then return immediately +*/ Why do you need 4 lines for a comment that fits on one line? Yes, we can fit this to one line. + if (!isr) + return; I can confirm that this works for me. I could reproduce the EDID problems in the past and I can't after this patch. ...so I could give a: Tested-by: Douglas Anderson I'm not an expert on this part of the code, so feel free to ignore my other comments if everyone else thinks this patch is fine as-is, but to me something here feels a little fragile. It feels a little weird that we'll "complete" for _any_ interrupt that comes through now rather than relying on dp_aux_native_handler() / dp_aux_i2c_handler() to specifically identify interrupts that caused the end of the transfer. I guess that idea is that every possible interrupt we get causes the end of the transfer? -Doug So this turned out to be more tricky and was a good finding from kuogee. In the bad EDID case, it was technically not bad EDID. What was happening was, the VIDEO_READY interrupt was continuously firing. Ideally, this should fire only once but due to some error condition it kept firing. We dont exactly know why yet what was the error condition making it continuously fire. In the DP ISR, the dp_aux_isr() gets called even if it was not an aux interrupt which fired (so the call flow in this case was dp_display_irq_handler (triggered for VIDEO_READY) ---> dp_aux_isr() So we should certainly have some protection to return early from this routine if there was no aux interrupt which fired. Which is what this fix is doing. Its not completing any interrupt, its just returning early if no aux interrupt fired. ...but the whole problem was that it was doing the complete() at the end, right? Kuogee even mentioned that in the commit message. Specifically, I checked dp_aux_native_handler() and dp_aux_i2c_handler(), both of which are passed the "isr". Unless I messed up, both functions already were no-ops if the ISR was 0, even before Kuogee's patch. That means that the only thing Kuogee's patch does is to prevent the call to "complete(>comp)" at the end of "dp_aux_isr()". ...and it makes sense not to call the complete() if no "isr" is 0. ...but what I'm saying is that _any_ non-zero value of ISR will still cause the complete() to be called after Kuogee's patch. That means that if any of the 32-bits in the "isr" variable are set, that we will call complete(). I'm asking if you're sure that every single bit of the "isr" means that we're ready to call complete(). It feels like it would be less fragile if dp_aux_native_handler() and dp_aux_i2c_handler() (which both already look at the ISR) returned some value saying whether the "isr" contained a bit that meant that complete() should be called. Yes, so other than the "transfer done" bits, the other bits we listen to are below: 29 #define DP_INTERRUPT_STATUS1 \ 30 (DP_INTR_AUX_I2C_DONE| \ 31 DP_INTR_WRONG_ADDR | DP_INTR_TIMEOUT | \ 32 DP_INTR_NACK_DEFER | DP_INTR_WRONG_DATA_CNT | \ 33 DP_INTR_I2C_NACK | DP_INTR_I2C_DEFER | \ 34 DP_INTR_PLL_UNLOCKED | DP_INTR_AUX_ERROR All of these, if they fire, will be
Re: [PATCH] drm/msm/dp: do not complete dp_aux_cmd_fifo_tx() if irq is not for aux transfer
Quoting Doug Anderson (2022-12-14 16:14:42) > Hi, > > On Wed, Dec 14, 2022 at 3:46 PM Abhinav Kumar > wrote: > > > > Hi Doug > > > > On 12/14/2022 2:29 PM, Doug Anderson wrote: > > > Hi, > > > > > > On Wed, Dec 14, 2022 at 1:21 PM Kuogee Hsieh > > > wrote: > > >> > > >> There are 3 possible interrupt sources are handled by DP controller, > > >> HPDstatus, Controller state changes and Aux read/write transaction. > > >> At every irq, DP controller have to check isr status of every interrupt > > >> sources and service the interrupt if its isr status bits shows interrupts > > >> are pending. There is potential race condition may happen at current aux > > >> isr handler implementation since it is always complete > > >> dp_aux_cmd_fifo_tx() > > >> even irq is not for aux read or write transaction. This may cause aux > > >> read > > >> transaction return premature if host aux data read is in the middle of > > >> waiting for sink to complete transferring data to host while irq happen. > > >> This will cause host's receiving buffer contains unexpected data. This > > >> patch fixes this problem by checking aux isr and return immediately at > > >> aux isr handler if there are no any isr status bits set. > > >> > > >> Follows are the signature at kernel logs when problem happen, > > >> EDID has corrupt header > > >> panel-simple-dp-aux aux-aea.edp: Couldn't identify panel via EDID > > >> panel-simple-dp-aux aux-aea.edp: error -EIO: Couldn't detect panel > > >> nor find a fallback > > >> > > >> Signed-off-by: Kuogee Hsieh > > >> --- > > >> drivers/gpu/drm/msm/dp/dp_aux.c | 7 +++ > > >> 1 file changed, 7 insertions(+) > > >> > > >> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c > > >> b/drivers/gpu/drm/msm/dp/dp_aux.c > > >> index d030a93..8f8b12a 100644 > > >> --- a/drivers/gpu/drm/msm/dp/dp_aux.c > > >> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c > > >> @@ -423,6 +423,13 @@ void dp_aux_isr(struct drm_dp_aux *dp_aux) > > >> > > >> isr = dp_catalog_aux_get_irq(aux->catalog); > > >> > > >> + /* > > >> +* if this irq is not for aux transfer, > > >> +* then return immediately > > >> +*/ > > > > > > Why do you need 4 lines for a comment that fits on one line? > > Yes, we can fit this to one line. > > > > > >> + if (!isr) > > >> + return; > > > > > > I can confirm that this works for me. I could reproduce the EDID > > > problems in the past and I can't after this patch. ...so I could give > > > a: > > > > > > Tested-by: Douglas Anderson > > > > > > I'm not an expert on this part of the code, so feel free to ignore my > > > other comments if everyone else thinks this patch is fine as-is, but > > > to me something here feels a little fragile. It feels a little weird > > > that we'll "complete" for _any_ interrupt that comes through now > > > rather than relying on dp_aux_native_handler() / dp_aux_i2c_handler() > > > to specifically identify interrupts that caused the end of the > > > transfer. I guess that idea is that every possible interrupt we get > > > causes the end of the transfer? > > > > > > -Doug > > > > So this turned out to be more tricky and was a good finding from kuogee. > > > > In the bad EDID case, it was technically not bad EDID. > > > > What was happening was, the VIDEO_READY interrupt was continuously > > firing. Ideally, this should fire only once but due to some error > > condition it kept firing. We dont exactly know why yet what was the > > error condition making it continuously fire. This is a great detail that is missing from the commit text. > > > > In the DP ISR, the dp_aux_isr() gets called even if it was not an aux > > interrupt which fired (so the call flow in this case was > > dp_display_irq_handler (triggered for VIDEO_READY) ---> dp_aux_isr() > > So we should certainly have some protection to return early from this > > routine if there was no aux interrupt which fired. I'm not sure that's a race condition though, more like a problem where the completion is called unconditionally? > > > > Which is what this fix is doing. > > > > Its not completing any interrupt, its just returning early if no aux > > interrupt fired. > > ...but the whole problem was that it was doing the complete() at the > end, right? Kuogee even mentioned that in the commit message. > Specifically, I checked dp_aux_native_handler() and > dp_aux_i2c_handler(), both of which are passed the "isr". Unless I > messed up, both functions already were no-ops if the ISR was 0, even > before Kuogee's patch. That means that the only thing Kuogee's patch > does is to prevent the call to "complete(>comp)" at the end of > "dp_aux_isr()". > > ...and it makes sense not to call the complete() if no "isr" is 0. > ...but what I'm saying is that _any_ non-zero value of ISR will still > cause the complete() to be called after Kuogee's patch. That means > that if any of the 32-bits in the "isr" variable are set, that we will > call complete(). I'm asking if you're
Re: [PATCH] drm/msm/dp: do not complete dp_aux_cmd_fifo_tx() if irq is not for aux transfer
Hi, On Wed, Dec 14, 2022 at 3:46 PM Abhinav Kumar wrote: > > Hi Doug > > On 12/14/2022 2:29 PM, Doug Anderson wrote: > > Hi, > > > > On Wed, Dec 14, 2022 at 1:21 PM Kuogee Hsieh > > wrote: > >> > >> There are 3 possible interrupt sources are handled by DP controller, > >> HPDstatus, Controller state changes and Aux read/write transaction. > >> At every irq, DP controller have to check isr status of every interrupt > >> sources and service the interrupt if its isr status bits shows interrupts > >> are pending. There is potential race condition may happen at current aux > >> isr handler implementation since it is always complete dp_aux_cmd_fifo_tx() > >> even irq is not for aux read or write transaction. This may cause aux read > >> transaction return premature if host aux data read is in the middle of > >> waiting for sink to complete transferring data to host while irq happen. > >> This will cause host's receiving buffer contains unexpected data. This > >> patch fixes this problem by checking aux isr and return immediately at > >> aux isr handler if there are no any isr status bits set. > >> > >> Follows are the signature at kernel logs when problem happen, > >> EDID has corrupt header > >> panel-simple-dp-aux aux-aea.edp: Couldn't identify panel via EDID > >> panel-simple-dp-aux aux-aea.edp: error -EIO: Couldn't detect panel nor > >> find a fallback > >> > >> Signed-off-by: Kuogee Hsieh > >> --- > >> drivers/gpu/drm/msm/dp/dp_aux.c | 7 +++ > >> 1 file changed, 7 insertions(+) > >> > >> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c > >> b/drivers/gpu/drm/msm/dp/dp_aux.c > >> index d030a93..8f8b12a 100644 > >> --- a/drivers/gpu/drm/msm/dp/dp_aux.c > >> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c > >> @@ -423,6 +423,13 @@ void dp_aux_isr(struct drm_dp_aux *dp_aux) > >> > >> isr = dp_catalog_aux_get_irq(aux->catalog); > >> > >> + /* > >> +* if this irq is not for aux transfer, > >> +* then return immediately > >> +*/ > > > > Why do you need 4 lines for a comment that fits on one line? > Yes, we can fit this to one line. > > > >> + if (!isr) > >> + return; > > > > I can confirm that this works for me. I could reproduce the EDID > > problems in the past and I can't after this patch. ...so I could give > > a: > > > > Tested-by: Douglas Anderson > > > > I'm not an expert on this part of the code, so feel free to ignore my > > other comments if everyone else thinks this patch is fine as-is, but > > to me something here feels a little fragile. It feels a little weird > > that we'll "complete" for _any_ interrupt that comes through now > > rather than relying on dp_aux_native_handler() / dp_aux_i2c_handler() > > to specifically identify interrupts that caused the end of the > > transfer. I guess that idea is that every possible interrupt we get > > causes the end of the transfer? > > > > -Doug > > So this turned out to be more tricky and was a good finding from kuogee. > > In the bad EDID case, it was technically not bad EDID. > > What was happening was, the VIDEO_READY interrupt was continuously > firing. Ideally, this should fire only once but due to some error > condition it kept firing. We dont exactly know why yet what was the > error condition making it continuously fire. > > In the DP ISR, the dp_aux_isr() gets called even if it was not an aux > interrupt which fired (so the call flow in this case was > dp_display_irq_handler (triggered for VIDEO_READY) ---> dp_aux_isr() > So we should certainly have some protection to return early from this > routine if there was no aux interrupt which fired. > > Which is what this fix is doing. > > Its not completing any interrupt, its just returning early if no aux > interrupt fired. ...but the whole problem was that it was doing the complete() at the end, right? Kuogee even mentioned that in the commit message. Specifically, I checked dp_aux_native_handler() and dp_aux_i2c_handler(), both of which are passed the "isr". Unless I messed up, both functions already were no-ops if the ISR was 0, even before Kuogee's patch. That means that the only thing Kuogee's patch does is to prevent the call to "complete(>comp)" at the end of "dp_aux_isr()". ...and it makes sense not to call the complete() if no "isr" is 0. ...but what I'm saying is that _any_ non-zero value of ISR will still cause the complete() to be called after Kuogee's patch. That means that if any of the 32-bits in the "isr" variable are set, that we will call complete(). I'm asking if you're sure that every single bit of the "isr" means that we're ready to call complete(). It feels like it would be less fragile if dp_aux_native_handler() and dp_aux_i2c_handler() (which both already look at the ISR) returned some value saying whether the "isr" contained a bit that meant that complete() should be called. -Doug
Re: [PATCH] drm/msm/dp: do not complete dp_aux_cmd_fifo_tx() if irq is not for aux transfer
Hi Doug On 12/14/2022 2:29 PM, Doug Anderson wrote: Hi, On Wed, Dec 14, 2022 at 1:21 PM Kuogee Hsieh wrote: There are 3 possible interrupt sources are handled by DP controller, HPDstatus, Controller state changes and Aux read/write transaction. At every irq, DP controller have to check isr status of every interrupt sources and service the interrupt if its isr status bits shows interrupts are pending. There is potential race condition may happen at current aux isr handler implementation since it is always complete dp_aux_cmd_fifo_tx() even irq is not for aux read or write transaction. This may cause aux read transaction return premature if host aux data read is in the middle of waiting for sink to complete transferring data to host while irq happen. This will cause host's receiving buffer contains unexpected data. This patch fixes this problem by checking aux isr and return immediately at aux isr handler if there are no any isr status bits set. Follows are the signature at kernel logs when problem happen, EDID has corrupt header panel-simple-dp-aux aux-aea.edp: Couldn't identify panel via EDID panel-simple-dp-aux aux-aea.edp: error -EIO: Couldn't detect panel nor find a fallback Signed-off-by: Kuogee Hsieh --- drivers/gpu/drm/msm/dp/dp_aux.c | 7 +++ 1 file changed, 7 insertions(+) diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c index d030a93..8f8b12a 100644 --- a/drivers/gpu/drm/msm/dp/dp_aux.c +++ b/drivers/gpu/drm/msm/dp/dp_aux.c @@ -423,6 +423,13 @@ void dp_aux_isr(struct drm_dp_aux *dp_aux) isr = dp_catalog_aux_get_irq(aux->catalog); + /* +* if this irq is not for aux transfer, +* then return immediately +*/ Why do you need 4 lines for a comment that fits on one line? Yes, we can fit this to one line. + if (!isr) + return; I can confirm that this works for me. I could reproduce the EDID problems in the past and I can't after this patch. ...so I could give a: Tested-by: Douglas Anderson I'm not an expert on this part of the code, so feel free to ignore my other comments if everyone else thinks this patch is fine as-is, but to me something here feels a little fragile. It feels a little weird that we'll "complete" for _any_ interrupt that comes through now rather than relying on dp_aux_native_handler() / dp_aux_i2c_handler() to specifically identify interrupts that caused the end of the transfer. I guess that idea is that every possible interrupt we get causes the end of the transfer? -Doug So this turned out to be more tricky and was a good finding from kuogee. In the bad EDID case, it was technically not bad EDID. What was happening was, the VIDEO_READY interrupt was continuously firing. Ideally, this should fire only once but due to some error condition it kept firing. We dont exactly know why yet what was the error condition making it continuously fire. In the DP ISR, the dp_aux_isr() gets called even if it was not an aux interrupt which fired (so the call flow in this case was dp_display_irq_handler (triggered for VIDEO_READY) ---> dp_aux_isr() So we should certainly have some protection to return early from this routine if there was no aux interrupt which fired. Which is what this fix is doing. Its not completing any interrupt, its just returning early if no aux interrupt fired. So based on the logs I have seen and given that its fixing this stability issue. Fixes: c943b4948b58 ("drm/msm/dp: add displayPort driver support") Reviewed-by: Abhinav Kumar
Re: [PATCH] drm/radeon: Fix screen corruption (v2)
On 2022-12-14 22:02, Alex Deucher wrote: On Wed, Dec 14, 2022 at 4:54 PM Robin Murphy wrote: On 2022-12-12 02:08, Luben Tuikov wrote: Fix screen corruption on older 32-bit systems using AGP chips. On older systems with little memory, for instance 1.5 GiB, using an AGP chip, the device's DMA mask is 0x, but the memory mask is 0x7FF, and subsequently dma_addressing_limited() returns 0x < 0x7FFF, false. As such the result of this static inline isn't suitable for the last argument to ttm_device_init()--it simply needs to now whether to use GFP_DMA32 when allocating DMA buffers. This sounds wrong to me. If the issues happen on systems without PAE it clearly can't have anything to with the actual DMA address size. Not to mention that AFAICS 32-bit x86 doesn't even have ZONE_DMA32, so GFP_DMA32 would be functionally meaningless anyway. Although the reported symptoms initially sounded like they could be caused by DMA going to the wrong place, that is also equally consistent with a loss of cache coherency. My (limited) understanding of AGP is that the GART can effectively alias memory to a second physical address, so I could well believe that something somewhere in the driver stack needs to perform some cache maintenance to avoid coherency issues, and that in these particular setups whatever that is might be assuming the memory is direct-mapped and thus going wrong for highmem pages. So as I said before, I really think this is not about using GFP_DMA32 at all, but about *not* using GFP_HIGHUSER. One of the wonderful features of AGP is that it has to be used with uncached memory. The aperture basically just provides a remapping of physical pages into a linear aperture that you point the GPU at. TTM has to jump through quite a few hoops to get uncached memory in the first place, so it's likely that that somehow isn't compatible with HIGHMEM. Can you get uncached HIGHMEM? I guess in principle yes, if you're careful not to use regular kmap()/kmap_atomic(), and always use pgprot_noncached() for userspace/vmalloc mappings, but clearly that leaves lots of scope for slipping up. Working backwards from primitives like set_memory_uc(), I see various paths in TTM where manipulating the caching state is skipped for highmem pages, but I wouldn't even know where to start looking for whether the right state is propagated to all the places where they might eventually be mapped somewhere. Cheers, Robin.
Re: [PATCH v12 2/5] dt-bindings: msm/dp: add data-lanes and link-frequencies property
On 12/13/2022 3:06 PM, Stephen Boyd wrote: Quoting Kuogee Hsieh (2022-12-13 13:44:05) Add both data-lanes and link-frequencies property into endpoint Why do we care? Please tell us why it's important. Changes in v7: -- split yaml out of dtsi patch -- link-frequencies from link rate to symbol rate -- deprecation of old data-lanes property Changes in v8: -- correct Bjorn mail address to kernel.org Changes in v10: -- add menu item to data-lanes and link-frequecnis Changes in v11: -- add endpoint property at port@1 Changes in v12: -- use enum for item at data-lanes and link-frequencies Signed-off-by: Kuogee Hsieh ` ^ Stray ` here? -/ --- .../bindings/display/msm/dp-controller.yaml| 30 -- 1 file changed, 28 insertions(+), 2 deletions(-) diff --git a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml index f2515af..8fb9fa5 100644 --- a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml +++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml @@ -96,14 +97,37 @@ properties: ports: $ref: /schemas/graph.yaml#/properties/ports + properties: port@0: -$ref: /schemas/graph.yaml#/properties/port +$ref: "/schemas/graph.yaml#/$defs/port-base" description: Input endpoint of the controller +properties: + endpoint: +$ref: /schemas/media/video-interfaces.yaml# port@1: -$ref: /schemas/graph.yaml#/properties/port +$ref: "/schemas/graph.yaml#/$defs/port-base" I thought the quotes weren't needed? description: Output endpoint of the controller +properties: + endpoint: +$ref: /schemas/media/video-interfaces.yaml# Does this need 'unevaluatedProperties: false' here? +properties: + data-lanes: +minItems: 1 +maxItems: 4 +items: + enum: [ 0, 1, 2, 3 ] + + link-frequencies: +minItems: 1 +maxItems: 4 +items: + enum: [ 162000, 27, 54, 81 ] + +required: + - port@0 + - port@1 required: - compatible @@ -193,6 +217,8 @@ examples: reg = <1>; endpoint { remote-endpoint = <>; +data-lanes = <0 1>; +link-frequencies = /bits/ 64 <162000 27 54 81>; }; So far we haven't used the output port on the DP controller in DT. I'm still not clear on what we should do in general for DP because there's a PHY that actually controls a lane count and lane mapping. In my mental model of the SoC, this DP controller's output port is connected to the DP PHY, which then sends the DP lanes out of the SoC to the next downstream device (i.e. a DP connector or type-c muxer). Having a remote-endpoint property with a phandle to typec doesn't fit my mental model. I'd expect it to be the typec PHY. ack That brings up the question: when we have 2 lanes vs. 4 lanes will we duplicate the data-lanes property in the PHY binding? I suspect we'll have to. Hopefully that sort of duplication is OK? Current we have limitation by reserve 2 data lanes for usb2, i am not sure duplication to 4 lanes will work automatically. Similarly, we may have a redriver that limits the link-frequencies property further (e.g. only support <= 2.7GHz). Having multiple link-frequencies along the graph is OK, right? And isn't the link-frequencies property known here by fact that the DP controller tells us which SoC this controller is for, and thus we already know the supported link frequencies? Finally, I wonder if we should put any of this in the DP controller's output endpoint, or if we can put these sorts of properties in the DP PHY binding directly? Can't we do that and then when the DP controller tries to set 4 lanes, the PHY immediately fails the call and the link training algorithm does its thing and tries fewer lanes? And similarly, if link-frequencies were in the PHY's binding, the PHY could fail to set those frequencies during link training, returning an error to the DP controller, letting the training move on to a lower frequency. If we did that this patch series would largely be about modifying the PHY binding, updating the PHY driver to enforce constraints, and handling errors during link training in the DP controller (which may already be done? I didn't check). phy/pll have different configuration base on link lanes and rate. it has to be set up before link training can start. Once link training start, then there are no any interactions between controller and phy during link training session. Link training only happen between
Re: [PATCH] drm/msm/dp: do not complete dp_aux_cmd_fifo_tx() if irq is not for aux transfer
Hi, On Wed, Dec 14, 2022 at 1:21 PM Kuogee Hsieh wrote: > > There are 3 possible interrupt sources are handled by DP controller, > HPDstatus, Controller state changes and Aux read/write transaction. > At every irq, DP controller have to check isr status of every interrupt > sources and service the interrupt if its isr status bits shows interrupts > are pending. There is potential race condition may happen at current aux > isr handler implementation since it is always complete dp_aux_cmd_fifo_tx() > even irq is not for aux read or write transaction. This may cause aux read > transaction return premature if host aux data read is in the middle of > waiting for sink to complete transferring data to host while irq happen. > This will cause host's receiving buffer contains unexpected data. This > patch fixes this problem by checking aux isr and return immediately at > aux isr handler if there are no any isr status bits set. > > Follows are the signature at kernel logs when problem happen, > EDID has corrupt header > panel-simple-dp-aux aux-aea.edp: Couldn't identify panel via EDID > panel-simple-dp-aux aux-aea.edp: error -EIO: Couldn't detect panel nor > find a fallback > > Signed-off-by: Kuogee Hsieh > --- > drivers/gpu/drm/msm/dp/dp_aux.c | 7 +++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c > index d030a93..8f8b12a 100644 > --- a/drivers/gpu/drm/msm/dp/dp_aux.c > +++ b/drivers/gpu/drm/msm/dp/dp_aux.c > @@ -423,6 +423,13 @@ void dp_aux_isr(struct drm_dp_aux *dp_aux) > > isr = dp_catalog_aux_get_irq(aux->catalog); > > + /* > +* if this irq is not for aux transfer, > +* then return immediately > +*/ Why do you need 4 lines for a comment that fits on one line? > + if (!isr) > + return; I can confirm that this works for me. I could reproduce the EDID problems in the past and I can't after this patch. ...so I could give a: Tested-by: Douglas Anderson I'm not an expert on this part of the code, so feel free to ignore my other comments if everyone else thinks this patch is fine as-is, but to me something here feels a little fragile. It feels a little weird that we'll "complete" for _any_ interrupt that comes through now rather than relying on dp_aux_native_handler() / dp_aux_i2c_handler() to specifically identify interrupts that caused the end of the transfer. I guess that idea is that every possible interrupt we get causes the end of the transfer? -Doug
Re: [PATCH] drm/i915/gt: Reset twice
Hi Rodrigo, On Tue, Dec 13, 2022 at 01:18:48PM +, Vivi, Rodrigo wrote: > On Tue, 2022-12-13 at 00:08 +0100, Andi Shyti wrote: > > Hi Rodrigo, > > > > On Mon, Dec 12, 2022 at 11:55:10AM -0500, Rodrigo Vivi wrote: > > > On Mon, Dec 12, 2022 at 05:13:38PM +0100, Andi Shyti wrote: > > > > From: Chris Wilson > > > > > > > > After applying an engine reset, on some platforms like > > > > Jasperlake, we > > > > occasionally detect that the engine state is not cleared until > > > > shortly > > > > after the resume. As we try to resume the engine with volatile > > > > internal > > > > state, the first request fails with a spurious CS event (it looks > > > > like > > > > it reports a lite-restore to the hung context, instead of the > > > > expected > > > > idle->active context switch). > > > > > > > > Signed-off-by: Chris Wilson > > > > > > There's a typo in the signature email I'm afraid... > > > > oh yes, I forgot the 'C' :) > > you forgot? > who signed it off? Chris, but as I was copy/pasting SoB's I might have unintentionally removed the 'c'. > > > Other than that, have we checked the possibility of using the > > > driver-initiated-flr bit > > > instead of this second loop? That should be the right way to > > > guarantee everything is > > > cleared on gen11+... > > > > maybe I am misinterpreting it, but is FLR the same as resetting > > hardware domains individually? > > No, it is bigger than that... almost the PCI FLR with some exceptions: > > https://lists.freedesktop.org/archives/intel-gfx/2022-December/313956.html yes, exactly... I would use FLR feedback if I was performing an FLR reset. But here I'm not doing that, here I'm simply gating off some power domains. It happens that those power domains turn on and off engines making them reset. FLR doesn't have anything to do here, also because if you want to reset a single engine you go through this function, instead of resetting the whole GPU with whatever is annexed. This patch is not fixing the "reset" concept of i915, but it's fixing a missing feedback that happens in one single platform when trying to gate on/off a domain. Maybe I am completely off track, but I don't see connection with FLR here. (besides FLR might not be present in all the platforms) Thanks a lot for your inputs, Andi > > How am I supposed to use driver_initiated_flr() in this context? > > Some drivers are not even using this gt reset anymore and going > directly with the driver-initiated FLR in case that guc reset failed. > > I believe we can still keep the gt reset in our case as we currently > have, but instead of keep retrying it until it succeeds we probably > should go to the next level and do the driver initiated FLR when the GT > reset failed. > > > > > Thanks, > > Andi > > > > > > Cc: sta...@vger.kernel.org > > > > Cc: Mika Kuoppala > > > > Signed-off-by: Andi Shyti > > > > --- > > > > drivers/gpu/drm/i915/gt/intel_reset.c | 34 > > > > ++- > > > > 1 file changed, 28 insertions(+), 6 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/gt/intel_reset.c > > > > b/drivers/gpu/drm/i915/gt/intel_reset.c > > > > index ffde89c5835a4..88dfc0c5316ff 100644 > > > > --- a/drivers/gpu/drm/i915/gt/intel_reset.c > > > > +++ b/drivers/gpu/drm/i915/gt/intel_reset.c > > > > @@ -268,6 +268,7 @@ static int ilk_do_reset(struct intel_gt *gt, > > > > intel_engine_mask_t engine_mask, > > > > static int gen6_hw_domain_reset(struct intel_gt *gt, u32 > > > > hw_domain_mask) > > > > { > > > > struct intel_uncore *uncore = gt->uncore; > > > > + int loops = 2; > > > > int err; > > > > > > > > /* > > > > @@ -275,18 +276,39 @@ static int gen6_hw_domain_reset(struct > > > > intel_gt *gt, u32 hw_domain_mask) > > > > * for fifo space for the write or forcewake the chip for > > > > * the read > > > > */ > > > > - intel_uncore_write_fw(uncore, GEN6_GDRST, > > > > hw_domain_mask); > > > > + do { > > > > + intel_uncore_write_fw(uncore, GEN6_GDRST, > > > > hw_domain_mask); > > > > > > > > - /* Wait for the device to ack the reset requests */ > > > > - err = __intel_wait_for_register_fw(uncore, > > > > - GEN6_GDRST, > > > > hw_domain_mask, 0, > > > > - 500, 0, > > > > - NULL); > > > > + /* > > > > + * Wait for the device to ack the reset requests. > > > > + * > > > > + * On some platforms, e.g. Jasperlake, we see see > > > > that the > > > > + * engine register state is not cleared until > > > > shortly after > > > > + * GDRST reports completion, causing a failure as > > > > we try > > > > + * to immediately resume while the internal state > > > > is still > > > > + * in flux. If we immediately repeat the reset, > >
Re: [PATCH] drm/radeon: Fix screen corruption (v2)
On Wed, Dec 14, 2022 at 4:54 PM Robin Murphy wrote: > > On 2022-12-12 02:08, Luben Tuikov wrote: > > Fix screen corruption on older 32-bit systems using AGP chips. > > > > On older systems with little memory, for instance 1.5 GiB, using an AGP > > chip, > > the device's DMA mask is 0x, but the memory mask is 0x7FF, and > > subsequently dma_addressing_limited() returns 0x < 0x7FFF, > > false. As such the result of this static inline isn't suitable for the last > > argument to ttm_device_init()--it simply needs to now whether to use > > GFP_DMA32 > > when allocating DMA buffers. > > This sounds wrong to me. If the issues happen on systems without PAE it > clearly can't have anything to with the actual DMA address size. Not to > mention that AFAICS 32-bit x86 doesn't even have ZONE_DMA32, so > GFP_DMA32 would be functionally meaningless anyway. Although the > reported symptoms initially sounded like they could be caused by DMA > going to the wrong place, that is also equally consistent with a loss of > cache coherency. > > My (limited) understanding of AGP is that the GART can effectively alias > memory to a second physical address, so I could well believe that > something somewhere in the driver stack needs to perform some cache > maintenance to avoid coherency issues, and that in these particular > setups whatever that is might be assuming the memory is direct-mapped > and thus going wrong for highmem pages. > > So as I said before, I really think this is not about using GFP_DMA32 at > all, but about *not* using GFP_HIGHUSER. One of the wonderful features of AGP is that it has to be used with uncached memory. The aperture basically just provides a remapping of physical pages into a linear aperture that you point the GPU at. TTM has to jump through quite a few hoops to get uncached memory in the first place, so it's likely that that somehow isn't compatible with HIGHMEM. Can you get uncached HIGHMEM? Alex > > Thanks, > Robin. > > > Partially reverts commit 33b3ad3788aba846fc8b9a065fe2685a0b64f713. > > > > v2: Amend the commit description. > > > > Cc: Mikhail Krylov > > Cc: Alex Deucher > > Cc: Robin Murphy > > Cc: Direct Rendering Infrastructure - Development > > > > Cc: AMD Graphics > > Fixes: 33b3ad3788aba8 ("drm/radeon: handle PCIe root ports with addressing > > limitations") > > Signed-off-by: Luben Tuikov > > --- > > drivers/gpu/drm/radeon/radeon.h| 1 + > > drivers/gpu/drm/radeon/radeon_device.c | 2 +- > > drivers/gpu/drm/radeon/radeon_ttm.c| 2 +- > > 3 files changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/radeon/radeon.h > > b/drivers/gpu/drm/radeon/radeon.h > > index 37dec92339b16a..4fe38fd9be3267 100644 > > --- a/drivers/gpu/drm/radeon/radeon.h > > +++ b/drivers/gpu/drm/radeon/radeon.h > > @@ -2426,6 +2426,7 @@ struct radeon_device { > > struct radeon_wbwb; > > struct radeon_dummy_pagedummy_page; > > boolshutdown; > > + boolneed_dma32; > > boolneed_swiotlb; > > boolaccel_working; > > boolfastfb_working; /* IGP feature*/ > > diff --git a/drivers/gpu/drm/radeon/radeon_device.c > > b/drivers/gpu/drm/radeon/radeon_device.c > > index 6344454a772172..3643a3cfe061bd 100644 > > --- a/drivers/gpu/drm/radeon/radeon_device.c > > +++ b/drivers/gpu/drm/radeon/radeon_device.c > > @@ -1370,7 +1370,7 @@ int radeon_device_init(struct radeon_device *rdev, > > if (rdev->family == CHIP_CEDAR) > > dma_bits = 32; > > #endif > > - > > + rdev->need_dma32 = dma_bits == 32; > > r = dma_set_mask_and_coherent(>pdev->dev, > > DMA_BIT_MASK(dma_bits)); > > if (r) { > > pr_warn("radeon: No suitable DMA available\n"); > > diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c > > b/drivers/gpu/drm/radeon/radeon_ttm.c > > index bdb4c0e0736ba2..3debaeb720d173 100644 > > --- a/drivers/gpu/drm/radeon/radeon_ttm.c > > +++ b/drivers/gpu/drm/radeon/radeon_ttm.c > > @@ -696,7 +696,7 @@ int radeon_ttm_init(struct radeon_device *rdev) > > rdev->ddev->anon_inode->i_mapping, > > rdev->ddev->vma_offset_manager, > > rdev->need_swiotlb, > > -dma_addressing_limited(>pdev->dev)); > > +rdev->need_dma32); > > if (r) { > > DRM_ERROR("failed initializing buffer object driver(%d).\n", > > r); > > return r; > > > > base-commit: 20e03e7f6e8efd42168db6d3fe044b804e0ede8f
Re: [PATCH] drm/radeon: Fix screen corruption (v2)
On 2022-12-12 02:08, Luben Tuikov wrote: Fix screen corruption on older 32-bit systems using AGP chips. On older systems with little memory, for instance 1.5 GiB, using an AGP chip, the device's DMA mask is 0x, but the memory mask is 0x7FF, and subsequently dma_addressing_limited() returns 0x < 0x7FFF, false. As such the result of this static inline isn't suitable for the last argument to ttm_device_init()--it simply needs to now whether to use GFP_DMA32 when allocating DMA buffers. This sounds wrong to me. If the issues happen on systems without PAE it clearly can't have anything to with the actual DMA address size. Not to mention that AFAICS 32-bit x86 doesn't even have ZONE_DMA32, so GFP_DMA32 would be functionally meaningless anyway. Although the reported symptoms initially sounded like they could be caused by DMA going to the wrong place, that is also equally consistent with a loss of cache coherency. My (limited) understanding of AGP is that the GART can effectively alias memory to a second physical address, so I could well believe that something somewhere in the driver stack needs to perform some cache maintenance to avoid coherency issues, and that in these particular setups whatever that is might be assuming the memory is direct-mapped and thus going wrong for highmem pages. So as I said before, I really think this is not about using GFP_DMA32 at all, but about *not* using GFP_HIGHUSER. Thanks, Robin. Partially reverts commit 33b3ad3788aba846fc8b9a065fe2685a0b64f713. v2: Amend the commit description. Cc: Mikhail Krylov Cc: Alex Deucher Cc: Robin Murphy Cc: Direct Rendering Infrastructure - Development Cc: AMD Graphics Fixes: 33b3ad3788aba8 ("drm/radeon: handle PCIe root ports with addressing limitations") Signed-off-by: Luben Tuikov --- drivers/gpu/drm/radeon/radeon.h| 1 + drivers/gpu/drm/radeon/radeon_device.c | 2 +- drivers/gpu/drm/radeon/radeon_ttm.c| 2 +- 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h index 37dec92339b16a..4fe38fd9be3267 100644 --- a/drivers/gpu/drm/radeon/radeon.h +++ b/drivers/gpu/drm/radeon/radeon.h @@ -2426,6 +2426,7 @@ struct radeon_device { struct radeon_wbwb; struct radeon_dummy_pagedummy_page; boolshutdown; + boolneed_dma32; boolneed_swiotlb; boolaccel_working; boolfastfb_working; /* IGP feature*/ diff --git a/drivers/gpu/drm/radeon/radeon_device.c b/drivers/gpu/drm/radeon/radeon_device.c index 6344454a772172..3643a3cfe061bd 100644 --- a/drivers/gpu/drm/radeon/radeon_device.c +++ b/drivers/gpu/drm/radeon/radeon_device.c @@ -1370,7 +1370,7 @@ int radeon_device_init(struct radeon_device *rdev, if (rdev->family == CHIP_CEDAR) dma_bits = 32; #endif - + rdev->need_dma32 = dma_bits == 32; r = dma_set_mask_and_coherent(>pdev->dev, DMA_BIT_MASK(dma_bits)); if (r) { pr_warn("radeon: No suitable DMA available\n"); diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c index bdb4c0e0736ba2..3debaeb720d173 100644 --- a/drivers/gpu/drm/radeon/radeon_ttm.c +++ b/drivers/gpu/drm/radeon/radeon_ttm.c @@ -696,7 +696,7 @@ int radeon_ttm_init(struct radeon_device *rdev) rdev->ddev->anon_inode->i_mapping, rdev->ddev->vma_offset_manager, rdev->need_swiotlb, - dma_addressing_limited(>pdev->dev)); + rdev->need_dma32); if (r) { DRM_ERROR("failed initializing buffer object driver(%d).\n", r); return r; base-commit: 20e03e7f6e8efd42168db6d3fe044b804e0ede8f
Re: [PATCH 1/3] drm/display/dp_mst: Fix down/up message handling after sink disconnect
For the whole series: Reviewed-by: Lyude Paul Thanks! On Wed, 2022-12-14 at 20:42 +0200, Imre Deak wrote: > If the sink gets disconnected during receiving a multi-packet DP MST AUX > down-reply/up-request sideband message, the state keeping track of which > packets have been received already is not reset. This results in a failed > sanity check for the subsequent message packet received after a sink is > reconnected (due to the pending message not yet completed with an > end-of-message-transfer packet), indicated by the > > "sideband msg set header failed" > > error. > > Fix the above by resetting the up/down message reception state after a > disconnect event. > > Cc: Lyude Paul > Cc: # v3.17+ > Signed-off-by: Imre Deak > --- > drivers/gpu/drm/display/drm_dp_mst_topology.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c > b/drivers/gpu/drm/display/drm_dp_mst_topology.c > index 51a46689cda70..90819fff2c9ba 100644 > --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c > +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c > @@ -3641,6 +3641,9 @@ int drm_dp_mst_topology_mgr_set_mst(struct > drm_dp_mst_topology_mgr *mgr, bool ms > drm_dp_dpcd_writeb(mgr->aux, DP_MSTM_CTRL, 0); > ret = 0; > mgr->payload_id_table_cleared = false; > + > + memset(>down_rep_recv, 0, sizeof(mgr->down_rep_recv)); > + memset(>up_req_recv, 0, sizeof(mgr->up_req_recv)); > } > > out_unlock: -- Cheers, Lyude Paul (she/her) Software Engineer at Red Hat
[PATCH] drm/msm/dp: do not complete dp_aux_cmd_fifo_tx() if irq is not for aux transfer
There are 3 possible interrupt sources are handled by DP controller, HPDstatus, Controller state changes and Aux read/write transaction. At every irq, DP controller have to check isr status of every interrupt sources and service the interrupt if its isr status bits shows interrupts are pending. There is potential race condition may happen at current aux isr handler implementation since it is always complete dp_aux_cmd_fifo_tx() even irq is not for aux read or write transaction. This may cause aux read transaction return premature if host aux data read is in the middle of waiting for sink to complete transferring data to host while irq happen. This will cause host's receiving buffer contains unexpected data. This patch fixes this problem by checking aux isr and return immediately at aux isr handler if there are no any isr status bits set. Follows are the signature at kernel logs when problem happen, EDID has corrupt header panel-simple-dp-aux aux-aea.edp: Couldn't identify panel via EDID panel-simple-dp-aux aux-aea.edp: error -EIO: Couldn't detect panel nor find a fallback Signed-off-by: Kuogee Hsieh --- drivers/gpu/drm/msm/dp/dp_aux.c | 7 +++ 1 file changed, 7 insertions(+) diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c index d030a93..8f8b12a 100644 --- a/drivers/gpu/drm/msm/dp/dp_aux.c +++ b/drivers/gpu/drm/msm/dp/dp_aux.c @@ -423,6 +423,13 @@ void dp_aux_isr(struct drm_dp_aux *dp_aux) isr = dp_catalog_aux_get_irq(aux->catalog); + /* +* if this irq is not for aux transfer, +* then return immediately +*/ + if (!isr) + return; + if (!aux->cmd_busy) return; -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Re: [v10] drm/msm/disp/dpu1: add support for dspp sub block flush in sc7280
On 2022-12-12 11:35:15, Kalyan Thota wrote: > [..] > >> + if (ctx->pending_dspp_flush_mask[dspp - DSPP_0]) > >> + DPU_REG_WRITE(>hw, CTL_DSPP_n_FLUSH(dspp - > >> DSPP_0), > >> + ctx->pending_dspp_flush_mask[dspp - > >> + DSPP_0]); > > > >Shouldn't this loop as a whole check if _any_ DSPP flush is requested via > >`pending_flush_mask & BIT(29)`? The other flushes don't check the per-block > >mask value either (and could write zero that way) but only base this check > >on the > >presence of a global flush mask for that block. > > > BIT(29) enables dspp flush only from DPU rev 7.x.x where hierarchal flush is > introduced. For other targets that supports CTL_ACTIVE, it's a NOP. The only way this patch ever writes pending_dspp_flush_mask is followed by unconditionally setting BIT(29) in pending_flush_mask. I was under the assumption that pending_dspp_flush_mask should be considered invalid or irrelevant unless BIT(29) is set. > With the check "pending_flush_mask & BIT(29)", unintended DSPP registers for > that CTL path will be programmed to "0" which is not correct IMO. You can also keep the second `if` to guard against that; as said the code above does exactly this though, but I think we could assume that if a pending sub-block flush is set, pending_dspp_flush_mask is nonzero? > Secondly "pending_flush_mask & BIT(29)" although will not be true for DPU > 6.x.x versions but can be confusing w.r.t code readability. > Let me know your thoughts. Ack, it is /super/ confusing that BIT(29) is used for DSPP (sub-block) flush, but also to flash INTF_2?? In fact there are many overlapping flush bits used for different components. Only few are clarified via a #define. Can you confirm whether this is correct? And whether these should all be pulled out into numerically-sorted defines to improve readability and document intentional overlap? - Marijn
Re: [PATCH V5 2/4] dt-bindings: display: panel: Add Magnachip D53E6EA8966
On Wed, 14 Dec 2022 12:06:09 -0600, Chris Morgan wrote: > From: Chris Morgan > > Add documentation for Magnachip D53E6EA8966 based panels such as the > Samsung AMS495QA01 panel. > > Signed-off-by: Chris Morgan > Signed-off-by: Maya Matuszczyk > --- > .../display/panel/magnachip,d53e6ea8966.yaml | 62 +++ > 1 file changed, 62 insertions(+) > create mode 100644 > Documentation/devicetree/bindings/display/panel/magnachip,d53e6ea8966.yaml > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13): yamllint warnings/errors: dtschema/dtc warnings/errors: ./Documentation/devicetree/bindings/display/panel/magnachip,d53e6ea8966.yaml: $id: relative path/filename doesn't match actual path or filename expected: http://devicetree.org/schemas/display/panel/magnachip,d53e6ea8966.yaml# doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20221214180611.109651-3-macroalph...@gmail.com The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema.
Re: [PATCH 06/16] drm/connector: Allow drivers to pass list of supported colorspaces
On Wed, Dec 14, 2022 at 3:56 AM Pekka Paalanen wrote: > > On Tue, 13 Dec 2022 11:32:01 -0500 > Harry Wentland wrote: > > > On 12/13/22 05:23, Pekka Paalanen wrote: > > > On Mon, 12 Dec 2022 13:21:27 -0500 > > > Harry Wentland wrote: > > > > > >> Drivers might not support all colorspaces defined in > > >> dp_colorspaces and hdmi_colorspaces. This results in > > >> undefined behavior when userspace is setting an > > >> unsupported colorspace. > > >> > > >> Allow drivers to pass the list of supported colorspaces > > >> when creating the colorspace property. > > > > > > Hi Harry, > > > > > > what is there for drivers to support? Isn't this just infoframe data > > > that shall be sent down to the sink as-is with no other effect? > > > > > > > You have a good point. > > > > Right now the supported colorspaces de-facto depend on driver > > implementations > > as you can see in [1] for i915 and [2] for amdgpu. The amdgpu driver will > > also program the MSA [3] for DP connections, and a bunch of other things > > which > > are deeper in the driver. > > > > [1] > > https://gitlab.freedesktop.org/hwentland/linux/-/blob/hdr-colorimetry/drivers/gpu/drm/i915/display/intel_dp.c#L1741 > > [2] > > https://gitlab.freedesktop.org/hwentland/linux/-/blob/hdr-colorimetry/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c#L5155 > > [3] > > https://gitlab.freedesktop.org/hwentland/linux/-/blob/hdr-colorimetry/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_stream_encoder.c#L368 > > > > I don't know why the DP VSC stuff needs to be in drivers. It should be > > common. The MSA packing would likely have to be driver specific since the > > mechanism of packing it is HW specific. > > What's MSA? I think it's Main Stream Attribute data. Part of DP. See slide 31 of this document: https://www.vesa.org/wp-content/uploads/2011/01/ICCE-Presentation-on-VESA-DisplayPort.pdf Alex > > I don't see it in > https://www.kernel.org/doc/html/latest/gpu/amdgpu/display/dc-glossary.html > or anywhere under Documentation/gpu or in CTA-861-H. > > > I'll have a closer look and see if we can eliminate the "driver supported" > > bit. If we can't we'll probably need to describe the reasoning better. > > That would be nice, thanks! > > > Will it be a problem if the list of supported colorspaces differs between > > drivers? > > I do not think so. It's just normal KMS UAPI that one must always > inspect an enumeration to see what values are possible. Userspace > cannot use a header with pre-defined numerical values, they always need > to be introspected first like everything else about KMS properties. > > I know there were some opinions about hard-coding enum numerical values > in headers, but I think in the end everyone agreed to the introspection > even if it didn't seem useful at the time. > > Besides, if a driver never supported a given value but misbehaved or > refused, I don't think that counts as a kernel regression? > > > Thanks, > pq > > > > > Harry > > > > > Is the driver confusing colorimetry with color-representation (the > > > RGB-YCbCr conversion)? Or is this property defining both? > > > > > > I feel that the documentation of "Colorspace" KMS connector property > > > needs clarification, and a list of potentially available values with > > > explanations, more than just a reference to CTA-816-H which it does not > > > even do yet. > > > > > > Perhaps a table, where for each enum drm_colorspace entry has a row > > > explaining: > > > > > > > > > Thanks, > > > pq > > > > > > > > >> Signed-off-by: Harry Wentland > > >> Cc: Pekka Paalanen > > >> Cc: Sebastian Wick > > >> Cc: vitaly.pros...@amd.com > > >> Cc: Uma Shankar > > >> Cc: Ville Syrjälä > > >> Cc: Joshua Ashton > > >> Cc: dri-devel@lists.freedesktop.org > > >> Cc: amd-...@lists.freedesktop.org > > >> --- > > >> drivers/gpu/drm/drm_connector.c | 140 +- > > >> .../gpu/drm/i915/display/intel_connector.c| 4 +- > > >> drivers/gpu/drm/vc4/vc4_hdmi.c| 2 +- > > >> include/drm/drm_connector.h | 8 +- > > >> 4 files changed, 83 insertions(+), 71 deletions(-) > > >>
Re: [RFC PATCH 6/6] drm/msm/dpu: Disallow unallocated (DSC) resources to be returned
On 2022-12-14 20:56:30, Dmitry Baryshkov wrote: > On 14/12/2022 01:22, Marijn Suijten wrote: > > In the event that the topology requests resources that have not been > > created by the system (because they are typically not represented in > > dpu_mdss_cfg ^1), the resource(s) in global_state (in this case DSC > > blocks) remain NULL but will still be returned out of > > dpu_rm_get_assigned_resources, where the caller expects to get an array > > containing num_blks valid pointers (but instead gets these NULLs). > > > > To prevent this from happening, where null-pointer dereferences > > typically result in a hard-to-debug platform lockup, num_blks shouldn't > > increase past NULL blocks and will print an error and break instead. > > After all, max_blks represents the static size of the maximum number of > > blocks whereas the actual amount varies per platform. > > > > In the specific case of DSC initial resource allocation should behave > > more like LMs and CTLs where NULL resources are skipped. The current > > hardcoded mapping of DSC blocks should be loosened separately as DPU > > 5.0.0 introduced a crossbar where DSC blocks can be "somewhat" freely > > bound to any PP and CTL, but that hardcoding currently means that we > > will return an error when the topology reserves a DSC that isn't > > available, instead of looking for the next free one. > > > > ^1: which can happen after a git rebase ended up moving additions to > > _dpu_cfg to a different struct which has the same patch context. > > > > Signed-off-by: Marijn Suijten > > --- > > drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 10 ++ > > 1 file changed, 10 insertions(+) > > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c > > index 73b3442e7467..dcbf03d2940a 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c > > @@ -496,6 +496,11 @@ static int _dpu_rm_reserve_dsc(struct dpu_rm *rm, > > > > /* check if DSC required are allocated or not */ > > for (i = 0; i < num_dsc; i++) { > > + if (!rm->dsc_blks[i]) { > > + DPU_ERROR("DSC %d does not exist\n", i); > > + return -EIO; > > + } > > + > > if (global_state->dsc_to_enc_id[i]) { > > DPU_ERROR("DSC %d is already allocated\n", i); > > return -EIO; > > @@ -660,6 +665,11 @@ int dpu_rm_get_assigned_resources(struct dpu_rm *rm, > > blks_size, enc_id); > > break; > > } > > + if (!hw_blks[i]) { > > + DPU_ERROR("No more resource %d available to assign to > > enc %d\n", > > + type, enc_id); > > + break; > > + } > > blks[num_blks++] = hw_blks[i]; > > } > > > > These two chunks should come as two separate patches, each having it's > own Fixes tag. Ack. They are indeed addressing different issues (with the same outcome) with differing "backportability". Will address in v2, thanks for pointing it out (and missing a Fixes: in the first place, of which we already have so many...). - Marijn
Re: [RFC PATCH 3/6] drm/msm/dpu1: Wire up DSC mask for active CTL configuration
On 2022-12-14 20:43:29, Dmitry Baryshkov wrote: > On 14/12/2022 01:22, Marijn Suijten wrote: > > Active CTLs have to configure what DSC block(s) have to be enabled, and > > what DSC block(s) have to be flushed; this value was initialized to zero > > resulting in the necessary register writes to never happen (or would > > write zero otherwise). This seems to have gotten lost in the DSC v4->v5 > > series while refactoring how the combination with merge_3d was handled. > > > > Fixes: 58dca9810749 ("drm/msm/disp/dpu1: Add support for DSC in encoder") > > Signed-off-by: Marijn Suijten > > --- > > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 1 + > > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 1 + > > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c | 2 ++ > > 3 files changed, 4 insertions(+) > > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c > > index ae28b2b93e69..35791f93c33d 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c > > @@ -61,6 +61,7 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg( > > intf_cfg.intf_mode_sel = DPU_CTL_MODE_SEL_CMD; > > intf_cfg.stream_sel = cmd_enc->stream_sel; > > intf_cfg.mode_3d = dpu_encoder_helper_get_3d_blend_mode(phys_enc); > > + intf_cfg.dsc = dpu_encoder_helper_get_dsc(phys_enc); > > ctl->ops.setup_intf_cfg(ctl, _cfg); > > > > /* setup which pp blk will connect to this intf */ > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c > > index 0f71e8fe7be7..9ee3a7306a5f 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c > > @@ -274,6 +274,7 @@ static void dpu_encoder_phys_vid_setup_timing_engine( > > intf_cfg.intf_mode_sel = DPU_CTL_MODE_SEL_VID; > > intf_cfg.stream_sel = 0; /* Don't care value for video mode */ > > intf_cfg.mode_3d = dpu_encoder_helper_get_3d_blend_mode(phys_enc); > > + intf_cfg.dsc = dpu_encoder_helper_get_dsc(phys_enc); > > if (phys_enc->hw_pp->merge_3d) > > intf_cfg.merge_3d = phys_enc->hw_pp->merge_3d->idx; > > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c > > index 7cbcef6efe17..92ddf9995b37 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c > > @@ -209,6 +209,7 @@ static void dpu_encoder_phys_wb_setup_cdp(struct > > dpu_encoder_phys *phys_enc) > > > > intf_cfg.intf = DPU_NONE; > > intf_cfg.wb = hw_wb->idx; > > + intf_cfg.dsc = dpu_encoder_helper_get_dsc(phys_enc); > > We usually don't have DSC with the writeback, don't we? I am unsure so ended up adding them in writeback regardless. Downstream uses a separate callback to process intf_cfg.dsc instead of going through setup_intf_cfg(). To prevent these from being missed again (in the case of copy), how about instead having some function that sets up intf_cfg with these default values from a phys_enc? That way most of this remains oblivious to the caller. On the same note, that callback on non-DPU_CTL_ACTIVE_CFG hardware doesn't use the intf_cfg.dsc member anyway, but it was again added to keep the blocks somewhat consistent (in case it ever becomes used?). > > if (mode_3d && hw_pp && hw_pp->merge_3d) > > intf_cfg.merge_3d = hw_pp->merge_3d->idx; > > @@ -230,6 +231,7 @@ static void dpu_encoder_phys_wb_setup_cdp(struct > > dpu_encoder_phys *phys_enc) > > intf_cfg.wb = hw_wb->idx; > > intf_cfg.mode_3d = > > dpu_encoder_helper_get_3d_blend_mode(phys_enc); > > + intf_cfg.dsc = dpu_encoder_helper_get_dsc(phys_enc); > > phys_enc->hw_ctl->ops.setup_intf_cfg(phys_enc->hw_ctl, > > _cfg); > > } > > } - Marijn
Re: [RFC PATCH 0/6] drm/msm: DSC Electric Boogaloo for sm8[12]50
On 2022-12-14 20:40:06, Dmitry Baryshkov wrote: > On 14/12/2022 01:22, Marijn Suijten wrote: > > This preliminary Display Stream Compression support package for > > (initially tested on) sm8[12]50 is based on comparing DSC behaviour > > between downstream and mainline. Some new callbacks are added (for > > binding blocks on active CTLs), logic bugs are corrected, zeroed struct > > members are now assigned proper values, and RM allocation and hw block > > retrieval now hand out (or not) DSC blocks without causing null-pointer > > dereferences. > > > > Unfortunately it is not yet enough to get rid of completely corrupted > > display output on the boards I tested here: > > - Sony Xperia 1 (sm8150), 1644x3840 or 1096x2560 pixels; > > - Sony Xperia 5II (sm8250), 1080x2520, at 60 or 120Hz; > > - (can include more Xperia boards if desired) > > > > Both devices use the DUALPIPE_DSCMERGE topology downstream: dual LM, PP > > and DSC, but only a single INTF/encoder/DSI-link. > > > > Hopefully this spawns some community/upstream interest to help rootcause > > our corruption issues (after we open a drm/msm report on GitLab for more > > appropriate tracking). > > > > The Sony Xperia XZ3 (sdm845) was fully tested and validated with this > > series to not cause any regressions (an one of the math fixes now allows > > us to change slice_count in the panel driver, which would corrupt > > previously). > > > > Marijn Suijten (6): > >drm/msm/dpu1: Implement DSC binding to PP block for CTL V1 > >drm/msm/dpu1: Add DSC config for sm8150 and sm8250 > >drm/msm/dpu1: Wire up DSC mask for active CTL configuration > >drm/msm/dsi: Use DSC slice(s) packet size to compute word count > >drm/msm/dsi: Flip greater-than check for slice_count and > > slice_per_intf > >drm/msm/dpu: Disallow unallocated (DSC) resources to be returned > > General comment: patches with Fixes ideally should come first. Usually > they are picked into -fixes and/or stable kernels. If the Fixes patches > are in the middle of the series, one can not be sure that they do not > have dependencies on previous patches. If there is one, it should > probably be stated clearly to ease work on backporting them. Ack, I may have rushed these RFC patches straight off my branches onto the lists in hopes of sparking some suggestions on what may still be broken or missing to get DSC working on sm[12]50, but will keep this in mind for v2 after receiving some more review. That said, any suggestions? - Marijn
Re: [PATCH 06/16] drm/connector: Allow drivers to pass list of supported colorspaces
On 12/14/22 03:55, Pekka Paalanen wrote: > On Tue, 13 Dec 2022 11:32:01 -0500 > Harry Wentland wrote: > >> On 12/13/22 05:23, Pekka Paalanen wrote: >>> On Mon, 12 Dec 2022 13:21:27 -0500 >>> Harry Wentland wrote: >>> Drivers might not support all colorspaces defined in dp_colorspaces and hdmi_colorspaces. This results in undefined behavior when userspace is setting an unsupported colorspace. Allow drivers to pass the list of supported colorspaces when creating the colorspace property. >>> >>> Hi Harry, >>> >>> what is there for drivers to support? Isn't this just infoframe data >>> that shall be sent down to the sink as-is with no other effect? >>> >> >> You have a good point. >> >> Right now the supported colorspaces de-facto depend on driver implementations >> as you can see in [1] for i915 and [2] for amdgpu. The amdgpu driver will >> also program the MSA [3] for DP connections, and a bunch of other things >> which >> are deeper in the driver. >> >> [1] >> https://gitlab.freedesktop.org/hwentland/linux/-/blob/hdr-colorimetry/drivers/gpu/drm/i915/display/intel_dp.c#L1741 >> [2] >> https://gitlab.freedesktop.org/hwentland/linux/-/blob/hdr-colorimetry/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c#L5155 >> [3] >> https://gitlab.freedesktop.org/hwentland/linux/-/blob/hdr-colorimetry/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_stream_encoder.c#L368 >> >> I don't know why the DP VSC stuff needs to be in drivers. It should be >> common. The MSA packing would likely have to be driver specific since the >> mechanism of packing it is HW specific. > > What's MSA? This is from the DP spec. It stands for Main Stream Attribute. It's a packet similar to an SDP that is encoded in the DP main signal stream (as opposed to an AUX message). > > I don't see it in > https://www.kernel.org/doc/html/latest/gpu/amdgpu/display/dc-glossary.html > or anywhere under Documentation/gpu or in CTA-861-H. > >> I'll have a closer look and see if we can eliminate the "driver supported" >> bit. If we can't we'll probably need to describe the reasoning better. > > That would be nice, thanks! > >> Will it be a problem if the list of supported colorspaces differs between >> drivers? > > I do not think so. It's just normal KMS UAPI that one must always > inspect an enumeration to see what values are possible. Userspace > cannot use a header with pre-defined numerical values, they always need > to be introspected first like everything else about KMS properties. > > I know there were some opinions about hard-coding enum numerical values > in headers, but I think in the end everyone agreed to the introspection > even if it didn't seem useful at the time. > > Besides, if a driver never supported a given value but misbehaved or > refused, I don't think that counts as a kernel regression? > True, but I would imagine that can be confusing for developers of DRM clients when enabling support for a new feature. I would guess it's much less confusing if drivers only exposed functionality that are expected to work (and are hopefully tested regularly). Harry > > Thanks, > pq > >> >> Harry >> >>> Is the driver confusing colorimetry with color-representation (the >>> RGB-YCbCr conversion)? Or is this property defining both? >>> >>> I feel that the documentation of "Colorspace" KMS connector property >>> needs clarification, and a list of potentially available values with >>> explanations, more than just a reference to CTA-816-H which it does not >>> even do yet. >>> >>> Perhaps a table, where for each enum drm_colorspace entry has a row >>> explaining: >>> >>> >>> Thanks, >>> pq >>> >>> Signed-off-by: Harry Wentland Cc: Pekka Paalanen Cc: Sebastian Wick Cc: vitaly.pros...@amd.com Cc: Uma Shankar Cc: Ville Syrjälä Cc: Joshua Ashton Cc: dri-devel@lists.freedesktop.org Cc: amd-...@lists.freedesktop.org --- drivers/gpu/drm/drm_connector.c | 140 +- .../gpu/drm/i915/display/intel_connector.c| 4 +- drivers/gpu/drm/vc4/vc4_hdmi.c| 2 +- include/drm/drm_connector.h | 8 +- 4 files changed, 83 insertions(+), 71 deletions(-)
Re: [RFC PATCH 6/6] drm/msm/dpu: Disallow unallocated (DSC) resources to be returned
On 14/12/2022 01:22, Marijn Suijten wrote: In the event that the topology requests resources that have not been created by the system (because they are typically not represented in dpu_mdss_cfg ^1), the resource(s) in global_state (in this case DSC blocks) remain NULL but will still be returned out of dpu_rm_get_assigned_resources, where the caller expects to get an array containing num_blks valid pointers (but instead gets these NULLs). To prevent this from happening, where null-pointer dereferences typically result in a hard-to-debug platform lockup, num_blks shouldn't increase past NULL blocks and will print an error and break instead. After all, max_blks represents the static size of the maximum number of blocks whereas the actual amount varies per platform. In the specific case of DSC initial resource allocation should behave more like LMs and CTLs where NULL resources are skipped. The current hardcoded mapping of DSC blocks should be loosened separately as DPU 5.0.0 introduced a crossbar where DSC blocks can be "somewhat" freely bound to any PP and CTL, but that hardcoding currently means that we will return an error when the topology reserves a DSC that isn't available, instead of looking for the next free one. ^1: which can happen after a git rebase ended up moving additions to _dpu_cfg to a different struct which has the same patch context. Signed-off-by: Marijn Suijten --- drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 10 ++ 1 file changed, 10 insertions(+) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c index 73b3442e7467..dcbf03d2940a 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c @@ -496,6 +496,11 @@ static int _dpu_rm_reserve_dsc(struct dpu_rm *rm, /* check if DSC required are allocated or not */ for (i = 0; i < num_dsc; i++) { + if (!rm->dsc_blks[i]) { + DPU_ERROR("DSC %d does not exist\n", i); + return -EIO; + } + if (global_state->dsc_to_enc_id[i]) { DPU_ERROR("DSC %d is already allocated\n", i); return -EIO; @@ -660,6 +665,11 @@ int dpu_rm_get_assigned_resources(struct dpu_rm *rm, blks_size, enc_id); break; } + if (!hw_blks[i]) { + DPU_ERROR("No more resource %d available to assign to enc %d\n", + type, enc_id); + break; + } blks[num_blks++] = hw_blks[i]; } These two chunks should come as two separate patches, each having it's own Fixes tag. -- With best wishes Dmitry
Re: [RFC PATCH 5/6] drm/msm/dsi: Flip greater-than check for slice_count and slice_per_intf
On 14/12/2022 01:22, Marijn Suijten wrote: According to downstream /and the comment copied from it/ this comparison should be the other way around. In other words, when the panel driver requests to use more slices per packet than what could be sent over this interface, it is bumped down to only use a single slice per packet (and strangely not the number of slices that could fit on the interface). Fixes: 08802f515c3c ("drm/msm/dsi: Add support for DSC configuration") --- drivers/gpu/drm/msm/dsi/dsi_host.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) With SoB in place: Reviewed-by: Dmitry Baryshkov -- With best wishes Dmitry
Re: [RFC PATCH 4/6] drm/msm/dsi: Use DSC slice(s) packet size to compute word count
On 14/12/2022 01:22, Marijn Suijten wrote: According to downstream the value to use for WORD_COUNT is bytes_per_pkt, which denotes the number of bytes in a packet based on how many slices have been configured by the panel driver times the width of a slice times the number of bytes per pixel. The DSC panels seen thus far use one byte per pixel, only one slice per packet, and a slice width of half the panel width leading to the desired bytes_per_pkt+1 value to be equal to hdisplay/2+1. This however isn't the case anymore for panels that configure two slices per packet, where the value should now be hdisplay+1. Note that the aforementioned panel (on a Sony Xperia XZ3, sdm845) with slice_count=1 has also been tested to successfully accept slice_count=2, which would have shown corrupted output previously. Fixes: 08802f515c3c ("drm/msm/dsi: Add support for DSC configuration") Signed-off-by: Marijn Suijten --- drivers/gpu/drm/msm/dsi/dsi_host.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) Reviewed-by: Dmitry Baryshkov -- With best wishes Dmitry
Re: [RFC PATCH 3/6] drm/msm/dpu1: Wire up DSC mask for active CTL configuration
On 14/12/2022 01:22, Marijn Suijten wrote: Active CTLs have to configure what DSC block(s) have to be enabled, and what DSC block(s) have to be flushed; this value was initialized to zero resulting in the necessary register writes to never happen (or would write zero otherwise). This seems to have gotten lost in the DSC v4->v5 series while refactoring how the combination with merge_3d was handled. Fixes: 58dca9810749 ("drm/msm/disp/dpu1: Add support for DSC in encoder") Signed-off-by: Marijn Suijten --- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 1 + drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 1 + drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c | 2 ++ 3 files changed, 4 insertions(+) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c index ae28b2b93e69..35791f93c33d 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c @@ -61,6 +61,7 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg( intf_cfg.intf_mode_sel = DPU_CTL_MODE_SEL_CMD; intf_cfg.stream_sel = cmd_enc->stream_sel; intf_cfg.mode_3d = dpu_encoder_helper_get_3d_blend_mode(phys_enc); + intf_cfg.dsc = dpu_encoder_helper_get_dsc(phys_enc); ctl->ops.setup_intf_cfg(ctl, _cfg); /* setup which pp blk will connect to this intf */ diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c index 0f71e8fe7be7..9ee3a7306a5f 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c @@ -274,6 +274,7 @@ static void dpu_encoder_phys_vid_setup_timing_engine( intf_cfg.intf_mode_sel = DPU_CTL_MODE_SEL_VID; intf_cfg.stream_sel = 0; /* Don't care value for video mode */ intf_cfg.mode_3d = dpu_encoder_helper_get_3d_blend_mode(phys_enc); + intf_cfg.dsc = dpu_encoder_helper_get_dsc(phys_enc); if (phys_enc->hw_pp->merge_3d) intf_cfg.merge_3d = phys_enc->hw_pp->merge_3d->idx; diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c index 7cbcef6efe17..92ddf9995b37 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c @@ -209,6 +209,7 @@ static void dpu_encoder_phys_wb_setup_cdp(struct dpu_encoder_phys *phys_enc) intf_cfg.intf = DPU_NONE; intf_cfg.wb = hw_wb->idx; + intf_cfg.dsc = dpu_encoder_helper_get_dsc(phys_enc); We usually don't have DSC with the writeback, don't we? if (mode_3d && hw_pp && hw_pp->merge_3d) intf_cfg.merge_3d = hw_pp->merge_3d->idx; @@ -230,6 +231,7 @@ static void dpu_encoder_phys_wb_setup_cdp(struct dpu_encoder_phys *phys_enc) intf_cfg.wb = hw_wb->idx; intf_cfg.mode_3d = dpu_encoder_helper_get_3d_blend_mode(phys_enc); + intf_cfg.dsc = dpu_encoder_helper_get_dsc(phys_enc); phys_enc->hw_ctl->ops.setup_intf_cfg(phys_enc->hw_ctl, _cfg); } } -- With best wishes Dmitry
[PATCH 3/3] drm/display/dp_mst: Fix payload addition on a disconnected sink
If an MST stream is enabled on a disconnected sink, the payload for the stream is not created and the MST manager's payload count/next start VC slot is not updated. Since the payload's start VC slot may still contain a valid value (!= -1) the subsequent disabling of such a stream could cause an incorrect decrease of the payload count/next start VC slot in drm_dp_remove_payload() and hence later payload additions will fail. Fix the above by marking the payload as invalid in the above case, so that it's skipped during payload removal. While at it add a debug print for this case. Cc: Lyude Paul Cc: # v6.1+ Signed-off-by: Imre Deak --- drivers/gpu/drm/display/drm_dp_mst_topology.c | 7 ++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c index 01350510244f2..5861b0a6247bc 100644 --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c @@ -3309,8 +3309,13 @@ int drm_dp_add_payload_part1(struct drm_dp_mst_topology_mgr *mgr, int ret; port = drm_dp_mst_topology_get_port_validated(mgr, payload->port); - if (!port) + if (!port) { + drm_dbg_kms(mgr->dev, + "VCPI %d for port %p not in topology, not creating a payload\n", + payload->vcpi, payload->port); + payload->vc_start_slot = -1; return 0; + } if (mgr->payload_count == 0) mgr->next_start_slot = mst_state->start_slot; -- 2.37.1
[PATCH 2/3] drm/display/dp_mst: Fix down message handling after a packet reception error
After an error during receiving a packet for a multi-packet DP MST sideband message, the state tracking which packets have been received already is not reset. This prevents the reception of subsequent down messages (due to the pending message not yet completed with an end-of-message-transfer packet). Fix the above by resetting the reception state after a packet error. Cc: Lyude Paul Cc: # v3.17+ Signed-off-by: Imre Deak --- drivers/gpu/drm/display/drm_dp_mst_topology.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c index 90819fff2c9ba..01350510244f2 100644 --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c @@ -3856,7 +3856,7 @@ static int drm_dp_mst_handle_down_rep(struct drm_dp_mst_topology_mgr *mgr) struct drm_dp_sideband_msg_rx *msg = >down_rep_recv; if (!drm_dp_get_one_sb_msg(mgr, false, )) - goto out; + goto out_clear_reply; /* Multi-packet message transmission, don't clear the reply */ if (!msg->have_eomt) -- 2.37.1
[PATCH 1/3] drm/display/dp_mst: Fix down/up message handling after sink disconnect
If the sink gets disconnected during receiving a multi-packet DP MST AUX down-reply/up-request sideband message, the state keeping track of which packets have been received already is not reset. This results in a failed sanity check for the subsequent message packet received after a sink is reconnected (due to the pending message not yet completed with an end-of-message-transfer packet), indicated by the "sideband msg set header failed" error. Fix the above by resetting the up/down message reception state after a disconnect event. Cc: Lyude Paul Cc: # v3.17+ Signed-off-by: Imre Deak --- drivers/gpu/drm/display/drm_dp_mst_topology.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c index 51a46689cda70..90819fff2c9ba 100644 --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c @@ -3641,6 +3641,9 @@ int drm_dp_mst_topology_mgr_set_mst(struct drm_dp_mst_topology_mgr *mgr, bool ms drm_dp_dpcd_writeb(mgr->aux, DP_MSTM_CTRL, 0); ret = 0; mgr->payload_id_table_cleared = false; + + memset(>down_rep_recv, 0, sizeof(mgr->down_rep_recv)); + memset(>up_req_recv, 0, sizeof(mgr->up_req_recv)); } out_unlock: -- 2.37.1
Re: [RFC PATCH 2/6] drm/msm/dpu1: Add DSC config for sm8150 and sm8250
On 14/12/2022 01:22, Marijn Suijten wrote: These blocks on CTL V1 support setting a PINGPONG idx to send pixel output to. Signed-off-by: Marijn Suijten --- .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c| 23 ++- 1 file changed, 17 insertions(+), 6 deletions(-) Reviewed-by: Dmitry Baryshkov -- With best wishes Dmitry
Re: [RFC PATCH 1/6] drm/msm/dpu1: Implement DSC binding to PP block for CTL V1
On 14/12/2022 01:22, Marijn Suijten wrote: All V1 CTL blocks (active CTLs) explicitly bind the pixel output from a DSC block to a PINGPONG block by setting the PINGPONG idx in a DSC hardware register. Signed-off-by: Marijn Suijten --- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 3 +++ .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h| 9 +++ drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c| 27 +++ drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h| 4 +++ 4 files changed, 43 insertions(+) Reviewed-by: Dmitry Baryshkov -- With best wishes Dmitry
Re: [RFC PATCH 0/6] drm/msm: DSC Electric Boogaloo for sm8[12]50
On 14/12/2022 01:22, Marijn Suijten wrote: This preliminary Display Stream Compression support package for (initially tested on) sm8[12]50 is based on comparing DSC behaviour between downstream and mainline. Some new callbacks are added (for binding blocks on active CTLs), logic bugs are corrected, zeroed struct members are now assigned proper values, and RM allocation and hw block retrieval now hand out (or not) DSC blocks without causing null-pointer dereferences. Unfortunately it is not yet enough to get rid of completely corrupted display output on the boards I tested here: - Sony Xperia 1 (sm8150), 1644x3840 or 1096x2560 pixels; - Sony Xperia 5II (sm8250), 1080x2520, at 60 or 120Hz; - (can include more Xperia boards if desired) Both devices use the DUALPIPE_DSCMERGE topology downstream: dual LM, PP and DSC, but only a single INTF/encoder/DSI-link. Hopefully this spawns some community/upstream interest to help rootcause our corruption issues (after we open a drm/msm report on GitLab for more appropriate tracking). The Sony Xperia XZ3 (sdm845) was fully tested and validated with this series to not cause any regressions (an one of the math fixes now allows us to change slice_count in the panel driver, which would corrupt previously). Marijn Suijten (6): drm/msm/dpu1: Implement DSC binding to PP block for CTL V1 drm/msm/dpu1: Add DSC config for sm8150 and sm8250 drm/msm/dpu1: Wire up DSC mask for active CTL configuration drm/msm/dsi: Use DSC slice(s) packet size to compute word count drm/msm/dsi: Flip greater-than check for slice_count and slice_per_intf drm/msm/dpu: Disallow unallocated (DSC) resources to be returned General comment: patches with Fixes ideally should come first. Usually they are picked into -fixes and/or stable kernels. If the Fixes patches are in the middle of the series, one can not be sure that they do not have dependencies on previous patches. If there is one, it should probably be stated clearly to ease work on backporting them. drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 3 +++ .../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 1 + .../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 1 + .../drm/msm/disp/dpu1/dpu_encoder_phys_wb.c | 2 ++ .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c| 23 +++- .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h| 9 +++ drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c| 27 +++ drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h| 4 +++ drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c| 10 +++ drivers/gpu/drm/msm/dsi/dsi_host.c| 6 ++--- 10 files changed, 77 insertions(+), 9 deletions(-) -- 2.38.1 -- With best wishes Dmitry
Re: [PATCH 1/5] Renaming weak prng invocations - prandom_bytes_state, prandom_u32_state
On Wed, Dec 14, 2022 at 05:21:17PM +0100, Stanislaw Gruszka wrote: > On Wed, Dec 14, 2022 at 04:15:49PM +0100, Eric Dumazet wrote: > > On Wed, Dec 14, 2022 at 1:34 PM Stanislaw Gruszka > > wrote: > > > > > > On Mon, Dec 12, 2022 at 03:35:20PM +0100, Jason A. Donenfeld wrote: > > > > Please CC me on future revisions. > > > > > > > > As of 6.2, the prandom namespace is *only* for predictable randomness. > > > > There's no need to rename anything. So nack on this patch 1/5. > > > > > > It is not obvious (for casual developers like me) that p in prandom > > > stands for predictable. Some renaming would be useful IMHO. I disagree. pseudo-random has *always* menat "predictable". And the 'p' in prandom was originally "pseudo-random". In userspace, random(3) is also pseudo-random, and is ***utterly*** predictable. So the original use of prandom() was a bit more of an explicit nod to the fact that prandom is something which is inherently predictable. So I don't think it's needed to rename it, whether it's to "predictable_rng_prandom_u32", or "no_you_idiot_dont_you_dare_use_it_for_cryptographi_purposes_prandom_u32". I think we need to assume a certain base level of competence, especially for someone who is messing with security psensitive kernel code. If a developer doesn't know that a prng is predictable, that's probably the *least* of the sort of mistakes that they might make. - Ted
Re: [PATCH v4 1/7] accel/ivpu: Introduce a new DRM driver for Intel VPU
On Wed, Dec 14, 2022 at 5:07 PM Jeffrey Hugo wrote: > > On 12/14/2022 6:57 AM, Oded Gabbay wrote: > > On Thu, Dec 8, 2022 at 1:08 PM Jacek Lawrynowicz > > wrote: > >> + > >> +static inline int ivpu_hw_info_init(struct ivpu_device *vdev) > >> +{ > >> + return vdev->hw->ops->info_init(vdev); > >> +}; > >> + > >> +static inline int ivpu_hw_power_up(struct ivpu_device *vdev) > >> +{ > >> + ivpu_dbg(vdev, PM, "HW power up\n"); > >> + > >> + return vdev->hw->ops->power_up(vdev); > >> +}; > >> + > >> +static inline int ivpu_hw_boot_fw(struct ivpu_device *vdev) > >> +{ > >> + return vdev->hw->ops->boot_fw(vdev); > >> +}; > >> + > >> +static inline bool ivpu_hw_is_idle(struct ivpu_device *vdev) > >> +{ > >> + return vdev->hw->ops->is_idle(vdev); > >> +}; > >> + > >> +static inline int ivpu_hw_power_down(struct ivpu_device *vdev) > >> +{ > >> + ivpu_dbg(vdev, PM, "HW power down\n"); > >> + > >> + return vdev->hw->ops->power_down(vdev); > >> +}; > >> + > >> +static inline void ivpu_hw_wdt_disable(struct ivpu_device *vdev) > >> +{ > >> + vdev->hw->ops->wdt_disable(vdev); > >> +}; > >> + > >> +/* Register indirect accesses */ > >> +static inline u32 ivpu_hw_reg_pll_freq_get(struct ivpu_device *vdev) > >> +{ > >> + return vdev->hw->ops->reg_pll_freq_get(vdev); > >> +}; > >> + > >> +static inline u32 ivpu_hw_reg_telemetry_offset_get(struct ivpu_device > >> *vdev) > >> +{ > >> + return vdev->hw->ops->reg_telemetry_offset_get(vdev); > >> +}; > >> + > >> +static inline u32 ivpu_hw_reg_telemetry_size_get(struct ivpu_device *vdev) > >> +{ > >> + return vdev->hw->ops->reg_telemetry_size_get(vdev); > >> +}; > >> + > >> +static inline u32 ivpu_hw_reg_telemetry_enable_get(struct ivpu_device > >> *vdev) > >> +{ > >> + return vdev->hw->ops->reg_telemetry_enable_get(vdev); > >> +}; > >> + > >> +static inline void ivpu_hw_reg_db_set(struct ivpu_device *vdev, u32 db_id) > >> +{ > >> + vdev->hw->ops->reg_db_set(vdev, db_id); > >> +}; > >> + > >> +static inline u32 ivpu_hw_reg_ipc_rx_addr_get(struct ivpu_device *vdev) > >> +{ > >> + return vdev->hw->ops->reg_ipc_rx_addr_get(vdev); > >> +}; > >> + > >> +static inline u32 ivpu_hw_reg_ipc_rx_count_get(struct ivpu_device *vdev) > >> +{ > >> + return vdev->hw->ops->reg_ipc_rx_count_get(vdev); > >> +}; > >> + > >> +static inline void ivpu_hw_reg_ipc_tx_set(struct ivpu_device *vdev, u32 > >> vpu_addr) > >> +{ > >> + vdev->hw->ops->reg_ipc_tx_set(vdev, vpu_addr); > >> +}; > >> + > >> +static inline void ivpu_hw_irq_clear(struct ivpu_device *vdev) > >> +{ > >> + vdev->hw->ops->irq_clear(vdev); > >> +}; > >> + > >> +static inline void ivpu_hw_irq_enable(struct ivpu_device *vdev) > >> +{ > >> + vdev->hw->ops->irq_enable(vdev); > >> +}; > >> + > >> +static inline void ivpu_hw_irq_disable(struct ivpu_device *vdev) > >> +{ > >> + vdev->hw->ops->irq_disable(vdev); > >> +}; > > Why hide all the calls to the hw ops functions inside inline functions ? > > I mean, it just makes it one step longer to read the code... > > Why not directly call vdev->hw->ops->operation ? > > > >> diff --git a/include/uapi/drm/ivpu_drm.h b/include/uapi/drm/ivpu_drm.h > >> new file mode 100644 > >> index ..922cbf30ce34 > >> --- /dev/null > >> +++ b/include/uapi/drm/ivpu_drm.h > >> @@ -0,0 +1,95 @@ > >> +/* SPDX-License-Identifier: GPL-2.0-only WITH Linux-syscall-note */ > >> +/* > >> + * Copyright (C) 2020-2022 Intel Corporation > >> + */ > >> + > >> +#ifndef __UAPI_IVPU_DRM_H__ > >> +#define __UAPI_IVPU_DRM_H__ > >> + > >> +#include "drm.h" > >> + > >> +#if defined(__cplusplus) > >> +extern "C" { > >> +#endif > >> + > >> +#define DRM_IVPU_DRIVER_MAJOR 1 > >> +#define DRM_IVPU_DRIVER_MINOR 0 > > I would prefer not to define versions for the driver. Don't get in > > this trap of trying to keep a version > > number updated over time. It breaks down the moment you get the code > > merged into the kernel tree as the driver is what is in the kernel, not > > separate from it. > > > > Also think of the stable kernels, you will backport fixes to those for > > the driver as part of the development process. So what "version" are > > they now? They are some mis-match of the old one with new fixes, and as > > such, the version number now means nothing. > > > > Just stick to the version number of the kernel itself, that's the only > > sane way. > > This seems to assume that the user is running whole kernels and not a > backport of the driver (similar to the backports project that is popular > with net). That's what the kernel upstream community usually assumes ;) > > Lets assume this gets merged into 6.3. End user is on say Ubuntu 20.04 > which GA'd with a 5.4 kernel and is maintaining that. Intel here > backports the 6.3 driver to 5.4 to support this end user and tries to do > the "right thing" by using the upstream code instead of some downstream > fork. Now the kernel version is meaningless because
[PATCH V4 4/4] arm64: dts: rockchip: add display to RG503
From: Chris Morgan Add Samsung AMS495QA01 panel to RG503. Signed-off-by: Chris Morgan Signed-off-by: Maya Matuszczyk --- .../dts/rockchip/rk3566-anbernic-rg503.dts| 55 +++ 1 file changed, 55 insertions(+) diff --git a/arch/arm64/boot/dts/rockchip/rk3566-anbernic-rg503.dts b/arch/arm64/boot/dts/rockchip/rk3566-anbernic-rg503.dts index 5dafcc86296b..b4b2df821cba 100644 --- a/arch/arm64/boot/dts/rockchip/rk3566-anbernic-rg503.dts +++ b/arch/arm64/boot/dts/rockchip/rk3566-anbernic-rg503.dts @@ -47,6 +47,21 @@ gpio_spi: spi { mosi-gpios = < RK_PB0 GPIO_ACTIVE_HIGH>; cs-gpios = < RK_PA7 GPIO_ACTIVE_HIGH>; num-chipselects = <0>; + + panel@0 { + compatible = "samsung,ams495qa01"; + reg = <0>; + pinctrl-names = "default"; + pinctrl-0 = <_reset>; + reset-gpios = < RK_PA0 GPIO_ACTIVE_LOW>; + vdd-supply = <_3v3>; + + port { + mipi_in_panel: endpoint { + remote-endpoint = <_out_panel>; + }; + }; + }; }; /* Channels reversed for both headphones and speakers. */ @@ -94,6 +109,32 @@ { assigned-clock-rates = <12>, <2>, <5>; }; +_dphy0 { + status = "okay"; +}; + + { + status = "okay"; + + ports { + dsi0_in: port@0 { + reg = <0>; + + dsi0_in_vp1: endpoint { + remote-endpoint = <_out_dsi0>; + }; + }; + + dsi0_out: port@1 { + reg = <1>; + + mipi_out_panel: endpoint { + remote-endpoint = <_in_panel>; + }; + }; + }; +}; + _keys_control { button-a { gpios = < RK_PC1 GPIO_ACTIVE_LOW>; @@ -146,6 +187,13 @@ spk_amp_enable_h: spk-amp-enable-h { }; }; + gpio-lcd { + lcd_reset: lcd-reset { + rockchip,pins = + <4 RK_PA0 RK_FUNC_GPIO _pull_none>; + }; + }; + gpio-spi { spi_pins: spi-pins { rockchip,pins = @@ -164,3 +212,10 @@ rk817_charger: charger { rockchip,sleep-filter-current-microamp = <10>; }; }; + + { + vp1_out_dsi0: endpoint@ROCKCHIP_VOP2_EP_MIPI0 { + reg = ; + remote-endpoint = <_in_vp1>; + }; +}; -- 2.34.1
[PATCH V5 3/4] drm/panel: Add Magnachip D53E6EA8966 Panel Driver
From: Chris Morgan Support Magnachip D53E6EA8966 based panels such as the Samsung AMS495QA01 panel as found on the Anbernic RG503. Note this driver supports only the AMS495QA01 today which receives video signals via DSI, however it receives commands via 3-wire SPI. Signed-off-by: Chris Morgan Signed-off-by: Maya Matuszczyk --- drivers/gpu/drm/panel/Kconfig | 10 + drivers/gpu/drm/panel/Makefile| 1 + .../drm/panel/panel-magnachip-d53e6ea8966.c | 515 ++ 3 files changed, 526 insertions(+) create mode 100644 drivers/gpu/drm/panel/panel-magnachip-d53e6ea8966.c diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig index a582ddd583c2..1f81fe8a2f8a 100644 --- a/drivers/gpu/drm/panel/Kconfig +++ b/drivers/gpu/drm/panel/Kconfig @@ -288,6 +288,16 @@ config DRM_PANEL_LG_LG4573 Say Y here if you want to enable support for LG4573 RGB panel. To compile this driver as a module, choose M here. +config DRM_PANEL_MAGNACHIP_D53E6EA8966 + tristate "Magnachip D53E6EA8966 DSI panel" + depends on OF && SPI + depends on DRM_MIPI_DSI + select DRM_MIPI_DBI + help + DRM panel driver for the Samsung AMS495QA01 panel controlled + with the Magnachip D53E6EA8966 panel IC. This panel receives + video data via DSI but commands via 3-Wire 9-bit SPI. + config DRM_PANEL_NEC_NL8048HL11 tristate "NEC NL8048HL11 RGB panel" depends on GPIOLIB && OF && SPI diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile index 34e717382dbb..7c09cd480c69 100644 --- a/drivers/gpu/drm/panel/Makefile +++ b/drivers/gpu/drm/panel/Makefile @@ -26,6 +26,7 @@ obj-$(CONFIG_DRM_PANEL_LEADTEK_LTK050H3146W) += panel-leadtek-ltk050h3146w.o obj-$(CONFIG_DRM_PANEL_LEADTEK_LTK500HD1829) += panel-leadtek-ltk500hd1829.o obj-$(CONFIG_DRM_PANEL_LG_LB035Q02) += panel-lg-lb035q02.o obj-$(CONFIG_DRM_PANEL_LG_LG4573) += panel-lg-lg4573.o +obj-$(CONFIG_DRM_PANEL_MAGNACHIP_D53E6EA8966) += panel-magnachip-d53e6ea8966.o obj-$(CONFIG_DRM_PANEL_NEC_NL8048HL11) += panel-nec-nl8048hl11.o obj-$(CONFIG_DRM_PANEL_NEWVISION_NV3051D) += panel-newvision-nv3051d.o obj-$(CONFIG_DRM_PANEL_NEWVISION_NV3052C) += panel-newvision-nv3052c.o diff --git a/drivers/gpu/drm/panel/panel-magnachip-d53e6ea8966.c b/drivers/gpu/drm/panel/panel-magnachip-d53e6ea8966.c new file mode 100644 index ..c1b80f2f296c --- /dev/null +++ b/drivers/gpu/drm/panel/panel-magnachip-d53e6ea8966.c @@ -0,0 +1,515 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Magnachip d53e6ea8966 MIPI-DSI panel driver + * Copyright (C) 2022 Chris Morgan + */ + +#include +#include +#include +#include +#include + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include + +/* Forward declaration for use in backlight function */ +struct d53e6ea8966; + +/* Panel info, unique to each panel */ +struct d53e6ea8966_panel_info { + /** @display_modes: the supported display modes */ + const struct drm_display_mode *display_modes; + /** @num_modes: the number of supported display modes */ + unsigned int num_modes; + /** @width_mm: panel width in mm */ + u16 width_mm; + /** @height_mm: panel height in mm */ + u16 height_mm; + /** @bus_flags: drm bus flags for panel */ + u32 bus_flags; + /** @panel_funcs: panel functions for panel */ + const struct drm_panel_funcs *panel_funcs; + /** @backlight: panel backlight registration or NULL */ + int (*backlight_register)(struct d53e6ea8966 *db); +}; + +struct d53e6ea8966 { + /** @dev: the container device */ + struct device *dev; + /** @dbi: the DBI bus abstraction handle */ + struct mipi_dbi dbi; + /** @panel: the DRM panel instance for this device */ + struct drm_panel panel; + /** @reset: reset GPIO line */ + struct gpio_desc *reset; + /** @enable: enable GPIO line */ + struct gpio_desc *enable; + /** @reg_vdd: VDD supply regulator for panel logic */ + struct regulator *reg_vdd; + /** @reg_elvdd: ELVDD supply regulator for panel display */ + struct regulator *reg_elvdd; + /** @dsi_dev: DSI child device (panel) */ + struct mipi_dsi_device *dsi_dev; + /** @bl_dev: pseudo-backlight device for oled panel */ + struct backlight_device *bl_dev; + /** @panel_info: struct containing panel timing and info */ + const struct d53e6ea8966_panel_info *panel_info; +}; + +#define NUM_GAMMA_LEVELS 16 +#define GAMMA_TABLE_COUNT 23 +#define MAX_BRIGHTNESS (NUM_GAMMA_LEVELS - 1) + +#define MCS_ELVSS_ON 0xb1 +#define MCS_TEMP_SWIRE 0xb2 +#define MCS_PASSWORD_0 0xf0 +#define MCS_PASSWORD_1 0xf1 +#define MCS_ANALOG_PWR_CTL_0 0xf4 +#define
[PATCH V5 1/4] drm: of: Add drm_of_get_dsi_bus helper function
From: Chris Morgan Add helper function to find DSI host for devices where DSI panel is not a minor of a DSI bus (such as the Samsung AMS495QA01 panel or the official Raspberry Pi touchscreen display). Signed-off-by: Chris Morgan Signed-off-by: Maya Matuszczyk --- drivers/gpu/drm/drm_of.c | 62 include/drm/drm_of.h | 11 +++ 2 files changed, 73 insertions(+) diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c index 7bbcb999bb75..7d89ac164069 100644 --- a/drivers/gpu/drm/drm_of.c +++ b/drivers/gpu/drm/drm_of.c @@ -10,6 +10,7 @@ #include #include #include +#include #include #include @@ -493,3 +494,64 @@ int drm_of_get_data_lanes_count_ep(const struct device_node *port, return ret; } EXPORT_SYMBOL_GPL(drm_of_get_data_lanes_count_ep); + +/** + * drm_of_get_dsi_bus - find the DSI bus for a given device + * @dev: parent device of display (SPI, I2C) + * @dsi_host: DSI host to be populated + * @info: DSI device info to be updated with correct DSI node + * + * Given a panel device parented to a non-DSI device, follow the + * devicetree to find the correct DSI host node and populate the + * dsi_host with the correct host and info with the correct node. + * Returns zero if successful, -EPROBE_DEFER if the DSI host is + * found but not available, or -ENODEV otherwise. + */ +int drm_of_get_dsi_bus(struct device *dev, + struct mipi_dsi_host **dsi_host, + struct mipi_dsi_device_info *info) +{ + struct device_node *endpoint, *dsi_host_node; + + /* +* Get first endpoint child from device. +*/ + endpoint = of_graph_get_next_endpoint(dev->of_node, NULL); + if (!endpoint) + return -ENODEV; + + /* +* Follow the first endpoint to get the DSI host node. +*/ + dsi_host_node = of_graph_get_remote_port_parent(endpoint); + if (!dsi_host_node) + goto error; + + /* +* Get the DSI host from the DSI host node. If we get an error +* or the return is null assume we're not ready to probe just +* yet. Release the DSI host node since we're done with it. +*/ + *dsi_host = of_find_mipi_dsi_host_by_node(dsi_host_node); + of_node_put(dsi_host_node); + if (IS_ERR_OR_NULL(*dsi_host)) { + of_node_put(endpoint); + return -EPROBE_DEFER; + } + + /* +* Set the node of the mipi_dsi_device_info to the correct node +* and then release the endpoint node since we're done with it. +*/ + info->node = of_graph_get_remote_port(endpoint); + if (IS_ERR_OR_NULL(info->node)) + goto error; + + of_node_put(endpoint); + return 0; + +error: + of_node_put(endpoint); + return -ENODEV; +} +EXPORT_SYMBOL_GPL(drm_of_get_dsi_bus); diff --git a/include/drm/drm_of.h b/include/drm/drm_of.h index 10ab58c40746..a694ce71c3b2 100644 --- a/include/drm/drm_of.h +++ b/include/drm/drm_of.h @@ -15,6 +15,8 @@ struct drm_encoder; struct drm_panel; struct drm_bridge; struct device_node; +struct mipi_dsi_device_info; +struct mipi_dsi_host; /** * enum drm_lvds_dual_link_pixels - Pixel order of an LVDS dual-link connection @@ -56,6 +58,9 @@ int drm_of_get_data_lanes_count_ep(const struct device_node *port, int port_reg, int reg, const unsigned int min, const unsigned int max); +int drm_of_get_dsi_bus(struct device *dev, + struct mipi_dsi_host **dsi_host, + struct mipi_dsi_device_info *info); #else static inline uint32_t drm_of_crtc_port_mask(struct drm_device *dev, struct device_node *port) @@ -127,6 +132,12 @@ drm_of_get_data_lanes_count_ep(const struct device_node *port, { return -EINVAL; } +static int drm_of_get_dsi_bus(struct device *dev, + struct mipi_dsi_host **dsi_host, + struct mipi_dsi_device_info *info); +{ + return -EINVAL; +} #endif /* -- 2.34.1
[PATCH V5 2/4] dt-bindings: display: panel: Add Magnachip D53E6EA8966
From: Chris Morgan Add documentation for Magnachip D53E6EA8966 based panels such as the Samsung AMS495QA01 panel. Signed-off-by: Chris Morgan Signed-off-by: Maya Matuszczyk --- .../display/panel/magnachip,d53e6ea8966.yaml | 62 +++ 1 file changed, 62 insertions(+) create mode 100644 Documentation/devicetree/bindings/display/panel/magnachip,d53e6ea8966.yaml diff --git a/Documentation/devicetree/bindings/display/panel/magnachip,d53e6ea8966.yaml b/Documentation/devicetree/bindings/display/panel/magnachip,d53e6ea8966.yaml new file mode 100644 index ..0a57a303a758 --- /dev/null +++ b/Documentation/devicetree/bindings/display/panel/magnachip,d53e6ea8966.yaml @@ -0,0 +1,62 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/display/panel/samsung,ams495qa01.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Magnachip D53E6EA8966 based display panel + +description: | + Magnachip D53E6EA8966 based display panels, such as the one found on + the Anbernic RG503 that is combined with a 940x544 OLED Samsung + AMS495QA01. + +maintainers: + - Chris Morgan + +allOf: + - $ref: panel-common.yaml# + +properties: + compatible: +const: samsung,ams495qa01 + + reg: true + reset-gpios: +description: reset gpio, must be GPIO_ACTIVE_LOW + elvdd-supply: +description: regulator that supplies voltage to the panel display + enable-gpios: true + port: true + vdd-supply: +description: regulator that supplies voltage to panel logic + +required: + - compatible + - reg + - reset-gpios + - vdd-supply + +additionalProperties: false + +examples: + - | +#include +spi { +#address-cells = <1>; +#size-cells = <0>; +panel@0 { +compatible = "samsung,ams495qa01"; +reg = <0>; +reset-gpios = < 0 GPIO_ACTIVE_LOW>; +vdd-supply = <_3v3>; + +port { +mipi_in_panel: endpoint { + remote-endpoint = <_out_panel>; +}; +}; +}; +}; + +... -- 2.34.1
[PATCH V5 0/4] drm/panel: Add Magnachip D53E6EA8966 Panel Controller
From: Chris Morgan Add the Magnachip D53E6EA8966 panel IC controller for display panels such as the Samsung AMS495QA01 panel as found on the Anbernic RG503. This panel uses DSI to receive video signals, but 3-wire SPI to receive command signals. Changes since V4: - Renamed driver from the panel model to the panel IC controller per DRM team. - Added a drm_of helper function of drm_of_get_dsi_bus() to handle finding and populating the DSI node when the DSI node is not the parent of the DSI controlled display. - Converted the documented commands to constants to make it more readable. - Reset GPIO is now required and documented as GPIO_ACTIVE_LOW. - Removed "prepared" logic from panel. Changes since V3: - Updated documentation to add spi-peripheral-props.yaml per updates made for similar devices. Note that I removed a "Reviewed-by" tag from Rob Herring since this change probably needs to be confirmed. - Added binding for RG503, since this device is now accepted with this request: https://lore.kernel.org/linux-rockchip/166274831283.21181.6861718157177507544.b4...@sntech.de/ Changes since V2: - Added 50hz mode at request of userspace devs. - Renamed "dupa" to panel name. Good catch Maya. - Added Maya's Signed-off-by. - Removed check for max backlight, since it is already done by backlight_device_set_brightness. - Fixed minor formatting issues on devicetree binding documentation and added port to provided example. Changes since V1: - Removed errant reference to backlight in documentation. This is an OLED panel. - Made elvss regulator optional. In my case its hard wired and not controllable. - Added "prepared" enum to track panel status to prevent unbalanced regulator enable/disable. Chris Morgan (4): drm: of: Add drm_of_get_dsi_bus helper function dt-bindings: display: panel: Add Magnachip D53E6EA8966 drm/panel: Add Magnachip D53E6EA8966 Panel Driver arm64: dts: rockchip: add display to RG503 .../display/panel/magnachip,d53e6ea8966.yaml | 62 +++ .../dts/rockchip/rk3566-anbernic-rg503.dts| 55 ++ drivers/gpu/drm/drm_of.c | 62 +++ drivers/gpu/drm/panel/Kconfig | 10 + drivers/gpu/drm/panel/Makefile| 1 + .../drm/panel/panel-magnachip-d53e6ea8966.c | 515 ++ include/drm/drm_of.h | 11 + 7 files changed, 716 insertions(+) create mode 100644 Documentation/devicetree/bindings/display/panel/magnachip,d53e6ea8966.yaml create mode 100644 drivers/gpu/drm/panel/panel-magnachip-d53e6ea8966.c -- 2.34.1
Re: [Intel-gfx] [PATCH] drm/i915/dg2: Return Wa_22012654132 to just specific steppings
On Tue, Dec 13, 2022 at 03:41:19PM -0800, Matt Roper wrote: > Programming of the ENABLE_PREFETCH_INTO_IC bit originally showed up in > both the general DG2 tuning guide (applicable to all DG2 > variants/steppings) and under Wa_22012654132 (applicable only to > specific steppings). It has now been removed from the tuning guide, and > the guidance is to only program it in the specific steppings associated > with the workaround. > > Bspec: 68331 Reviewed-by: Matt Atwood > Signed-off-by: Matt Roper > --- > drivers/gpu/drm/i915/gt/intel_workarounds.c | 27 ++--- > 1 file changed, 13 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c > b/drivers/gpu/drm/i915/gt/intel_workarounds.c > index 7d71f5bbddc8..bf84efb3f15f 100644 > --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c > +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c > @@ -2913,20 +2913,6 @@ add_render_compute_tuning_settings(struct > drm_i915_private *i915, > if (IS_DG2(i915)) { > wa_mcr_write_or(wal, XEHP_L3SCQREG7, > BLEND_FILL_CACHING_OPT_DIS); > wa_mcr_write_clr_set(wal, RT_CTRL, STACKID_CTRL, > STACKID_CTRL_512); > - > - /* > - * This is also listed as Wa_22012654132 for certain DG2 > - * steppings, but the tuning setting programming is a superset > - * since it applies to all DG2 variants and steppings. > - * > - * Note that register 0xE420 is write-only and cannot be read > - * back for verification on DG2 (due to Wa_14012342262), so > - * we need to explicitly skip the readback. > - */ > - wa_mcr_add(wal, GEN10_CACHE_MODE_SS, 0, > -_MASKED_BIT_ENABLE(ENABLE_PREFETCH_INTO_IC), > -0 /* write-only, so skip validation */, > -true); > } > > /* > @@ -3022,6 +3008,19 @@ general_render_compute_wa_init(struct intel_engine_cs > *engine, struct i915_wa_li > /* Wa_18017747507:dg2 */ > wa_masked_en(wal, VFG_PREEMPTION_CHICKEN, > POLYGON_TRIFAN_LINELOOP_DISABLE); > } > + > + if (IS_DG2_GRAPHICS_STEP(i915, G10, STEP_A0, STEP_C0) || > IS_DG2_G11(i915)) > + /* > + * Wa_22012654132 > + * > + * Note that register 0xE420 is write-only and cannot be read > + * back for verification on DG2 (due to Wa_14012342262), so > + * we need to explicitly skip the readback. > + */ > + wa_mcr_add(wal, GEN10_CACHE_MODE_SS, 0, > +_MASKED_BIT_ENABLE(ENABLE_PREFETCH_INTO_IC), > +0 /* write-only, so skip validation */, > +true); > } > > static void > -- > 2.38.1 >
Re: [git pull] drm for 6.2-rc1
On Wed, Dec 14, 2022 at 12:05 AM Christian König wrote: > > Anyway we need to re-apply b09d6acba1d9 which should be trivial. Note that my resolution did exactly that (*), it's just that when I double-checked against Dave's suggested merge that I noticed I'd done things differently than he did. (*) Well, when I say "did exactly that" I don't actually know some of the other fencing changes that happened, so there may be a reason why something further should still be done. So I can only point to my merge commit a594533df0f6 and ask people to verify. It does all at least work for me. Knock wood. Linus
Re: [PATCH] drm/bridge: it6505: Add caching for EDID
On Tue, 15 Nov 2022 19:27:20 +0800, Pin-yen Lin wrote: > Add caching when EDID is read, and invalidate the cache until the > bridge detects HPD low or sink count changes on HPD_IRQ. > > It takes 1.2s for IT6505 bridge to read a 3-block EDID, and skipping > one EDID read would be a notable difference on user experience. > > > [...] Applied, thanks! Repo: https://cgit.freedesktop.org/drm/drm-misc/ [1/1] drm/bridge: it6505: Add caching for EDID (no commit info) rob
Re: [PATCH 1/5] Renaming weak prng invocations - prandom_bytes_state, prandom_u32_state
On Wed, Dec 14, 2022 at 04:15:49PM +0100, Eric Dumazet wrote: > On Wed, Dec 14, 2022 at 1:34 PM Stanislaw Gruszka > wrote: > > > > On Mon, Dec 12, 2022 at 03:35:20PM +0100, Jason A. Donenfeld wrote: > > > Please CC me on future revisions. > > > > > > As of 6.2, the prandom namespace is *only* for predictable randomness. > > > There's no need to rename anything. So nack on this patch 1/5. > > > > It is not obvious (for casual developers like me) that p in prandom > > stands for predictable. Some renaming would be useful IMHO. > > Renaming makes backports more complicated, because stable teams will > have to 'undo' name changes. > Stable teams are already overwhelmed by the amount of backports, and > silly merge conflicts. Since when backporting problems is valid argument for stop making changes? That's new for me. > linux kernel is not for casual readers. Sure. Regards Stanislaw
Re: [PATCH v13 2/5] dt-bindings: msm/dp: add data-lanes and link-frequencies property
On Tue, Dec 13, 2022 at 02:56:18PM -0800, Kuogee Hsieh wrote: > Add both data-lanes and link-frequencies property into endpoint > > Changes in v7: > -- split yaml out of dtsi patch > -- link-frequencies from link rate to symbol rate > -- deprecation of old data-lanes property > > Changes in v8: > -- correct Bjorn mail address to kernel.org > > Changes in v10: > -- add menu item to data-lanes and link-frequecnis > > Changes in v11: > -- add endpoint property at port@1 > > Changes in v12: > -- use enum for item at data-lanes and link-frequencies > > Changes in v13: > -- revised changes at port@0 > -- use correct ref schemas for both port@0 and port@1 > -- mark both port@0 and port@1 are required > -- add line between data-lanes and link-frequencies properties > > Signed-off-by: Kuogee Hsieh ` > --- > .../bindings/display/msm/dp-controller.yaml| 26 > -- > 1 file changed, 24 insertions(+), 2 deletions(-) > > diff --git a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml > b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml > index f2515af..9d002de 100644 > --- a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml > +++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml > @@ -81,6 +81,7 @@ properties: > >data-lanes: > $ref: /schemas/types.yaml#/definitions/uint32-array > +deprecated: true > minItems: 1 > maxItems: 4 > items: > @@ -98,12 +99,31 @@ properties: > $ref: /schemas/graph.yaml#/properties/ports > properties: >port@0: > -$ref: /schemas/graph.yaml#/properties/port > +$ref: "/schemas/graph.yaml#/$defs/port-base" This means you have extra properties to add in the endpoint, but you didn't define any. > description: Input endpoint of the controller > >port@1: > -$ref: /schemas/graph.yaml#/properties/port > +$ref: "/schemas/graph.yaml#/$defs/port-base" Don't need quotes. Why did you add them? > description: Output endpoint of the controller > +properties: > + endpoint: > +$ref: /schemas/media/video-interfaces.yaml# > +properties: > + data-lanes: > +minItems: 1 > +maxItems: 4 > +items: > + enum: [ 0, 1, 2, 3 ] > + > + link-frequencies: > +minItems: 1 > +maxItems: 4 > +items: > + enum: [ 162000, 27, 54, 81 ] > + > +required: > + - port@0 > + - port@1 > > required: >- compatible > @@ -193,6 +213,8 @@ examples: > reg = <1>; > endpoint { > remote-endpoint = <>; > +data-lanes = <0 1>; > +link-frequencies = /bits/ 64 <162000 27 > 54 81>; > }; > }; > }; > -- > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > a Linux Foundation Collaborative Project > >
Re: [PATCH 1/5] Renaming weak prng invocations - prandom_bytes_state, prandom_u32_state
On Wed, Dec 14, 2022 at 05:53:52PM +0200, Andy Shevchenko wrote: > On Wed, Dec 14, 2022 at 04:15:49PM +0100, Eric Dumazet wrote: > > On Wed, Dec 14, 2022 at 1:34 PM Stanislaw Gruszka > > wrote: > > > On Mon, Dec 12, 2022 at 03:35:20PM +0100, Jason A. Donenfeld wrote: > > > > Please CC me on future revisions. > > > > > > > > As of 6.2, the prandom namespace is *only* for predictable randomness. > > > > There's no need to rename anything. So nack on this patch 1/5. > > > > > > It is not obvious (for casual developers like me) that p in prandom > > > stands for predictable. Some renaming would be useful IMHO. > > > > Renaming makes backports more complicated, because stable teams will > > have to 'undo' name changes. > > Stable teams are already overwhelmed by the amount of backports, and > > silly merge conflicts. > > > > Take another example : > > > > u64 timecounter_read(struct timecounter *tc) > > > > You would think this function would read the timecounter, right ? > > > > Well, it _updates_ many fields from @tc, so a 'better name' would also > > be useful. > > Right, at some point we become into the world of > > #define true 0 > > because... (read below) > > > linux kernel is not for casual readers. > > P.S. I believe you applied a common sense and in some cases > the renames are necessary. And before you become to a wrong conclusion by reading between the lines, no, I'm not taking either side (to rename or not to rename) in this case. -- With Best Regards, Andy Shevchenko
Re: [PATCH 1/5] Renaming weak prng invocations - prandom_bytes_state, prandom_u32_state
On Wed, Dec 14, 2022 at 04:15:49PM +0100, Eric Dumazet wrote: > On Wed, Dec 14, 2022 at 1:34 PM Stanislaw Gruszka > wrote: > > On Mon, Dec 12, 2022 at 03:35:20PM +0100, Jason A. Donenfeld wrote: > > > Please CC me on future revisions. > > > > > > As of 6.2, the prandom namespace is *only* for predictable randomness. > > > There's no need to rename anything. So nack on this patch 1/5. > > > > It is not obvious (for casual developers like me) that p in prandom > > stands for predictable. Some renaming would be useful IMHO. > > Renaming makes backports more complicated, because stable teams will > have to 'undo' name changes. > Stable teams are already overwhelmed by the amount of backports, and > silly merge conflicts. > > Take another example : > > u64 timecounter_read(struct timecounter *tc) > > You would think this function would read the timecounter, right ? > > Well, it _updates_ many fields from @tc, so a 'better name' would also > be useful. Right, at some point we become into the world of #define true 0 because... (read below) > linux kernel is not for casual readers. P.S. I believe you applied a common sense and in some cases the renames are necessary. -- With Best Regards, Andy Shevchenko
Re: [PATCH 16/16] drm/amd/display: Don't restrict bpc to 8 bpc
On Wed, Dec 14, 2022 at 4:01 AM Pekka Paalanen wrote: > > On Tue, 13 Dec 2022 18:20:59 +0100 > Michel Dänzer wrote: > > > On 12/12/22 19:21, Harry Wentland wrote: > > > This will let us pass kms_hdr.bpc_switch. > > > > > > I don't see any good reasons why we still need to > > > limit bpc to 8 bpc and doing so is problematic when > > > we enable HDR. > > > > > > If I remember correctly there might have been some > > > displays out there where the advertised link bandwidth > > > was not large enough to drive the default timing at > > > max bpc. This would leave to an atomic commit/check > > > failure which should really be handled in compositors > > > with some sort of fallback mechanism. > > > > > > If this somehow turns out to still be an issue I > > > suggest we add a module parameter to allow users to > > > limit the max_bpc to a desired value. > > > > While leaving the fallback for user space to handle makes some sense > > in theory, in practice most KMS display servers likely won't handle > > it. > > > > Another issue is that if mode validation is based on the maximum bpc > > value, it may reject modes which would work with lower bpc. > > > > > > What Ville (CC'd) suggested before instead (and what i915 seems to be > > doing already) is that the driver should do mode validation based on > > the *minimum* bpc, and automatically make the effective bpc lower > > than the maximum as needed to make the rest of the atomic state work. > > A driver is always allowed to choose a bpc lower than max_bpc, so it > very well should do so when necessary due to *known* hardware etc. > limitations. > In the amdgpu case, it's more of a preference thing. The driver would enable higher bpcs at the expense of refresh rate and it seemed most users want higher refresh rates than higher bpc. I guess the driver can select a lower bpc at its discretion to produce what it thinks is the best default, but what about users that don't want the default? Alex > So things like mode validation cannot just look at a single max or min > bpc, but it needs to figure out if there is any usable bpc value that > makes the mode work. > > The max_bpc knob exists only for the cases where the sink undetectably > malfunctions unless the bpc is artificially limited more than seems > necessary. That malfunction requires a human to detect, and reconfigure > their system as we don't have a quirk database for this I think. > > The question of userspace wanting a specific bpc is a different matter > and an unsolved one. It also ties to userspace wanting to use the > current mode to avoid a mode switch between e.g. hand-off from firmware > boot splash to proper userspace. That's also unsolved AFAIK. > > OTOH, we have the discussion that concluded as > https://gitlab.freedesktop.org/wayland/weston/-/issues/612#note_1359898 > which really puts userspace in charge of max_bpc, so the driver-chosen > default value does not have much impact as long as it makes the > firmware-chosen video mode to continue, as requested in > https://gitlab.freedesktop.org/wayland/weston/-/merge_requests/995 > given that userspace cannot know what the actual bpc currently is nor > set the exact bpc to keep it the same. > > > Thanks, > pq
Re: [PATCH v4 1/7] accel/ivpu: Introduce a new DRM driver for Intel VPU
On Wed, Dec 14, 2022 at 03:57:54PM +0200, Oded Gabbay wrote: > On Thu, Dec 8, 2022 at 1:08 PM Jacek Lawrynowicz > wrote: > > + > > +static inline int ivpu_hw_info_init(struct ivpu_device *vdev) > > +{ > > + return vdev->hw->ops->info_init(vdev); > > +}; > > + > > +static inline int ivpu_hw_power_up(struct ivpu_device *vdev) > > +{ > > + ivpu_dbg(vdev, PM, "HW power up\n"); > > + > > + return vdev->hw->ops->power_up(vdev); > > +}; > > + > > +static inline int ivpu_hw_boot_fw(struct ivpu_device *vdev) > > +{ > > + return vdev->hw->ops->boot_fw(vdev); > > +}; > > + > > +static inline bool ivpu_hw_is_idle(struct ivpu_device *vdev) > > +{ > > + return vdev->hw->ops->is_idle(vdev); > > +}; > > + > > +static inline int ivpu_hw_power_down(struct ivpu_device *vdev) > > +{ > > + ivpu_dbg(vdev, PM, "HW power down\n"); > > + > > + return vdev->hw->ops->power_down(vdev); > > +}; > > + > > +static inline void ivpu_hw_wdt_disable(struct ivpu_device *vdev) > > +{ > > + vdev->hw->ops->wdt_disable(vdev); > > +}; > > + > > +/* Register indirect accesses */ > > +static inline u32 ivpu_hw_reg_pll_freq_get(struct ivpu_device *vdev) > > +{ > > + return vdev->hw->ops->reg_pll_freq_get(vdev); > > +}; > > + > > +static inline u32 ivpu_hw_reg_telemetry_offset_get(struct ivpu_device > > *vdev) > > +{ > > + return vdev->hw->ops->reg_telemetry_offset_get(vdev); > > +}; > > + > > +static inline u32 ivpu_hw_reg_telemetry_size_get(struct ivpu_device *vdev) > > +{ > > + return vdev->hw->ops->reg_telemetry_size_get(vdev); > > +}; > > + > > +static inline u32 ivpu_hw_reg_telemetry_enable_get(struct ivpu_device > > *vdev) > > +{ > > + return vdev->hw->ops->reg_telemetry_enable_get(vdev); > > +}; > > + > > +static inline void ivpu_hw_reg_db_set(struct ivpu_device *vdev, u32 db_id) > > +{ > > + vdev->hw->ops->reg_db_set(vdev, db_id); > > +}; > > + > > +static inline u32 ivpu_hw_reg_ipc_rx_addr_get(struct ivpu_device *vdev) > > +{ > > + return vdev->hw->ops->reg_ipc_rx_addr_get(vdev); > > +}; > > + > > +static inline u32 ivpu_hw_reg_ipc_rx_count_get(struct ivpu_device *vdev) > > +{ > > + return vdev->hw->ops->reg_ipc_rx_count_get(vdev); > > +}; > > + > > +static inline void ivpu_hw_reg_ipc_tx_set(struct ivpu_device *vdev, u32 > > vpu_addr) > > +{ > > + vdev->hw->ops->reg_ipc_tx_set(vdev, vpu_addr); > > +}; > > + > > +static inline void ivpu_hw_irq_clear(struct ivpu_device *vdev) > > +{ > > + vdev->hw->ops->irq_clear(vdev); > > +}; > > + > > +static inline void ivpu_hw_irq_enable(struct ivpu_device *vdev) > > +{ > > + vdev->hw->ops->irq_enable(vdev); > > +}; > > + > > +static inline void ivpu_hw_irq_disable(struct ivpu_device *vdev) > > +{ > > + vdev->hw->ops->irq_disable(vdev); > > +}; > Why hide all the calls to the hw ops functions inside inline functions ? > I mean, it just makes it one step longer to read the code... > Why not directly call vdev->hw->ops->operation ? It allows to extend the calls for code that are common between individual implementations. > > diff --git a/include/uapi/drm/ivpu_drm.h b/include/uapi/drm/ivpu_drm.h > > new file mode 100644 > > index ..922cbf30ce34 > > --- /dev/null > > +++ b/include/uapi/drm/ivpu_drm.h > > @@ -0,0 +1,95 @@ > > +/* SPDX-License-Identifier: GPL-2.0-only WITH Linux-syscall-note */ > > +/* > > + * Copyright (C) 2020-2022 Intel Corporation > > + */ > > + > > +#ifndef __UAPI_IVPU_DRM_H__ > > +#define __UAPI_IVPU_DRM_H__ > > + > > +#include "drm.h" > > + > > +#if defined(__cplusplus) > > +extern "C" { > > +#endif > > + > > +#define DRM_IVPU_DRIVER_MAJOR 1 > > +#define DRM_IVPU_DRIVER_MINOR 0 > I would prefer not to define versions for the driver. Don't get in > this trap of trying to keep a version > number updated over time. It breaks down the moment you get the code > merged into the kernel tree as the driver is what is in the kernel, not > separate from it. > > Also think of the stable kernels, you will backport fixes to those for > the driver as part of the development process. So what "version" are > they now? They are some mis-match of the old one with new fixes, and as > such, the version number now means nothing. Yes, it's not optimal to identify features using major/minor version and we do not relay heavily on this. For now we expect that some features are present and working since particular major/minor version and mark feature tests failed or skipped accordingly. > Just stick to the version number of the kernel itself, that's the only > sane way. > > btw, I talked to Daniel about this and he told me this > major/minor/patchlevel is legacy in drm and should be deprecated, so > I'm going to send a patch to document that it is deprecated and how to > use getcap ioctl to find out the features the driver support. Cool. Regards Stanislaw
Re: [PATCH v9 01/15] drm/msm/disp/dpu: clear dpu_assign_crtc and get crtc from connector state instead of dpu_enc
On 14/12/2022 12:05, Vinod Polimera wrote: Update crtc retrieval from dpu_enc to dpu_enc connector state, since new links get set as part of the dpu enc virt mode set. The dpu_enc->crtc cache is no more needed, hence cleaning it as part of this change. This patch is dependent on the series: https://patchwork.freedesktop.org/series/110969/ Signed-off-by: Vinod Polimera --- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c| 4 --- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 42 + drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 8 -- 3 files changed, 13 insertions(+), 41 deletions(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c index 3f72d38..289d51e 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c @@ -1029,7 +1029,6 @@ static void dpu_crtc_disable(struct drm_crtc *crtc, */ if (dpu_encoder_get_intf_mode(encoder) == INTF_MODE_VIDEO) release_bandwidth = true; - dpu_encoder_assign_crtc(encoder, NULL); } /* wait for frame_event_done completion */ @@ -1099,9 +1098,6 @@ static void dpu_crtc_enable(struct drm_crtc *crtc, trace_dpu_crtc_enable(DRMID(crtc), true, dpu_crtc); dpu_crtc->enabled = true; - drm_for_each_encoder_mask(encoder, crtc->dev, crtc->state->encoder_mask) - dpu_encoder_assign_crtc(encoder, crtc); - /* Enable/restore vblank irq handling */ drm_crtc_vblank_on(crtc); } diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c index a585036..b9b254d 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c @@ -132,11 +132,6 @@ enum dpu_enc_rc_states { * @intfs_swapped:Whether or not the phys_enc interfaces have been swapped *for partial update right-only cases, such as pingpong *split where virtual pingpong does not generate IRQs - * @crtc: Pointer to the currently assigned crtc. Normally you - * would use crtc->state->encoder_mask to determine the - * link between encoder/crtc. However in this case we need - * to track crtc in the disable() hook which is called - * _after_ encoder_mask is cleared. * @connector:If a mode is set, cached pointer to the active connector * @crtc_kickoff_cb: Callback into CRTC that will flush & start *all CTL paths @@ -181,7 +176,6 @@ struct dpu_encoder_virt { bool intfs_swapped; - struct drm_crtc *crtc; struct drm_connector *connector; struct dentry *debugfs_root; @@ -1317,7 +1311,7 @@ static void dpu_encoder_vblank_callback(struct drm_encoder *drm_enc, struct dpu_encoder_phys *phy_enc) { struct dpu_encoder_virt *dpu_enc = NULL; - unsigned long lock_flags; + struct drm_crtc *crtc; if (!drm_enc || !phy_enc) return; @@ -1325,12 +1319,13 @@ static void dpu_encoder_vblank_callback(struct drm_encoder *drm_enc, DPU_ATRACE_BEGIN("encoder_vblank_callback"); dpu_enc = to_dpu_encoder_virt(drm_enc); - atomic_inc(_enc->vsync_cnt); + if (!dpu_enc->connector || !dpu_enc->connector->state || + !dpu_enc->connector->state->crtc) + return; - spin_lock_irqsave(_enc->enc_spinlock, lock_flags); - if (dpu_enc->crtc) - dpu_crtc_vblank_callback(dpu_enc->crtc); - spin_unlock_irqrestore(_enc->enc_spinlock, lock_flags); + atomic_inc(_enc->vsync_cnt); + crtc = dpu_enc->connector->state->crtc; + dpu_crtc_vblank_callback(crtc); DPU_ATRACE_END("encoder_vblank_callback"); } @@ -1353,33 +1348,22 @@ static void dpu_encoder_underrun_callback(struct drm_encoder *drm_enc, DPU_ATRACE_END("encoder_underrun_callback"); } -void dpu_encoder_assign_crtc(struct drm_encoder *drm_enc, struct drm_crtc *crtc) -{ - struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc); - unsigned long lock_flags; - - spin_lock_irqsave(_enc->enc_spinlock, lock_flags); - /* crtc should always be cleared before re-assigning */ - WARN_ON(crtc && dpu_enc->crtc); - dpu_enc->crtc = crtc; - spin_unlock_irqrestore(_enc->enc_spinlock, lock_flags); -} - void dpu_encoder_toggle_vblank_for_crtc(struct drm_encoder *drm_enc, struct drm_crtc *crtc, bool enable) { struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc); - unsigned long lock_flags; + struct drm_crtc *new_crtc; int i; trace_dpu_enc_vblank_cb(DRMID(drm_enc), enable); - spin_lock_irqsave(_enc->enc_spinlock, lock_flags); - if (dpu_enc->crtc != crtc) { -
Re: [PATCH 1/5] Renaming weak prng invocations - prandom_bytes_state, prandom_u32_state
On Wed, Dec 14, 2022 at 1:34 PM Stanislaw Gruszka wrote: > > On Mon, Dec 12, 2022 at 03:35:20PM +0100, Jason A. Donenfeld wrote: > > Please CC me on future revisions. > > > > As of 6.2, the prandom namespace is *only* for predictable randomness. > > There's no need to rename anything. So nack on this patch 1/5. > > It is not obvious (for casual developers like me) that p in prandom > stands for predictable. Some renaming would be useful IMHO. Renaming makes backports more complicated, because stable teams will have to 'undo' name changes. Stable teams are already overwhelmed by the amount of backports, and silly merge conflicts. Take another example : u64 timecounter_read(struct timecounter *tc) You would think this function would read the timecounter, right ? Well, it _updates_ many fields from @tc, so a 'better name' would also be useful. linux kernel is not for casual readers.
[Bug 216806] [Raven Ridge] console disappears after framebuffer device loads
https://bugzilla.kernel.org/show_bug.cgi?id=216806 --- Comment #3 from Balazs Vinarz (viniba...@gmail.com) --- It looks like this: https://www.youtube.com/watch?v=8oUUBjmo0_I -- You may reply to this email to add a comment. You are receiving this mail because: You are watching the assignee of the bug.
Re: [PATCH v4 1/7] accel/ivpu: Introduce a new DRM driver for Intel VPU
On 12/14/2022 6:57 AM, Oded Gabbay wrote: On Thu, Dec 8, 2022 at 1:08 PM Jacek Lawrynowicz wrote: + +static inline int ivpu_hw_info_init(struct ivpu_device *vdev) +{ + return vdev->hw->ops->info_init(vdev); +}; + +static inline int ivpu_hw_power_up(struct ivpu_device *vdev) +{ + ivpu_dbg(vdev, PM, "HW power up\n"); + + return vdev->hw->ops->power_up(vdev); +}; + +static inline int ivpu_hw_boot_fw(struct ivpu_device *vdev) +{ + return vdev->hw->ops->boot_fw(vdev); +}; + +static inline bool ivpu_hw_is_idle(struct ivpu_device *vdev) +{ + return vdev->hw->ops->is_idle(vdev); +}; + +static inline int ivpu_hw_power_down(struct ivpu_device *vdev) +{ + ivpu_dbg(vdev, PM, "HW power down\n"); + + return vdev->hw->ops->power_down(vdev); +}; + +static inline void ivpu_hw_wdt_disable(struct ivpu_device *vdev) +{ + vdev->hw->ops->wdt_disable(vdev); +}; + +/* Register indirect accesses */ +static inline u32 ivpu_hw_reg_pll_freq_get(struct ivpu_device *vdev) +{ + return vdev->hw->ops->reg_pll_freq_get(vdev); +}; + +static inline u32 ivpu_hw_reg_telemetry_offset_get(struct ivpu_device *vdev) +{ + return vdev->hw->ops->reg_telemetry_offset_get(vdev); +}; + +static inline u32 ivpu_hw_reg_telemetry_size_get(struct ivpu_device *vdev) +{ + return vdev->hw->ops->reg_telemetry_size_get(vdev); +}; + +static inline u32 ivpu_hw_reg_telemetry_enable_get(struct ivpu_device *vdev) +{ + return vdev->hw->ops->reg_telemetry_enable_get(vdev); +}; + +static inline void ivpu_hw_reg_db_set(struct ivpu_device *vdev, u32 db_id) +{ + vdev->hw->ops->reg_db_set(vdev, db_id); +}; + +static inline u32 ivpu_hw_reg_ipc_rx_addr_get(struct ivpu_device *vdev) +{ + return vdev->hw->ops->reg_ipc_rx_addr_get(vdev); +}; + +static inline u32 ivpu_hw_reg_ipc_rx_count_get(struct ivpu_device *vdev) +{ + return vdev->hw->ops->reg_ipc_rx_count_get(vdev); +}; + +static inline void ivpu_hw_reg_ipc_tx_set(struct ivpu_device *vdev, u32 vpu_addr) +{ + vdev->hw->ops->reg_ipc_tx_set(vdev, vpu_addr); +}; + +static inline void ivpu_hw_irq_clear(struct ivpu_device *vdev) +{ + vdev->hw->ops->irq_clear(vdev); +}; + +static inline void ivpu_hw_irq_enable(struct ivpu_device *vdev) +{ + vdev->hw->ops->irq_enable(vdev); +}; + +static inline void ivpu_hw_irq_disable(struct ivpu_device *vdev) +{ + vdev->hw->ops->irq_disable(vdev); +}; Why hide all the calls to the hw ops functions inside inline functions ? I mean, it just makes it one step longer to read the code... Why not directly call vdev->hw->ops->operation ? diff --git a/include/uapi/drm/ivpu_drm.h b/include/uapi/drm/ivpu_drm.h new file mode 100644 index ..922cbf30ce34 --- /dev/null +++ b/include/uapi/drm/ivpu_drm.h @@ -0,0 +1,95 @@ +/* SPDX-License-Identifier: GPL-2.0-only WITH Linux-syscall-note */ +/* + * Copyright (C) 2020-2022 Intel Corporation + */ + +#ifndef __UAPI_IVPU_DRM_H__ +#define __UAPI_IVPU_DRM_H__ + +#include "drm.h" + +#if defined(__cplusplus) +extern "C" { +#endif + +#define DRM_IVPU_DRIVER_MAJOR 1 +#define DRM_IVPU_DRIVER_MINOR 0 I would prefer not to define versions for the driver. Don't get in this trap of trying to keep a version number updated over time. It breaks down the moment you get the code merged into the kernel tree as the driver is what is in the kernel, not separate from it. Also think of the stable kernels, you will backport fixes to those for the driver as part of the development process. So what "version" are they now? They are some mis-match of the old one with new fixes, and as such, the version number now means nothing. Just stick to the version number of the kernel itself, that's the only sane way. This seems to assume that the user is running whole kernels and not a backport of the driver (similar to the backports project that is popular with net). Lets assume this gets merged into 6.3. End user is on say Ubuntu 20.04 which GA'd with a 5.4 kernel and is maintaining that. Intel here backports the 6.3 driver to 5.4 to support this end user and tries to do the "right thing" by using the upstream code instead of some downstream fork. Now the kernel version is meaningless because 5.4 never had this driver. Of course if the user reports an issue to Intel, it would be handy to know exactly what version of the driver the user has. The src version hash that modinfo provides is meaningless because it changes based on the entirety of the build, which includes things like the compiler used. If there is an alternative solution for this, I'd love to hear it.
Re: renesas/master bisection: igt-kms-rockchip.kms_vblank.pipe-A-wait-forked on rk3399-gru-kevin
Hi Guillaume, On Wed, Dec 14, 2022 at 3:16 PM Guillaume Tucker wrote: > On 14/12/2022 15:03, Geert Uytterhoeven wrote: > > On Wed, Dec 14, 2022 at 1:54 PM Guillaume Tucker > > wrote: > >> On 14/12/2022 11:06, Geert Uytterhoeven wrote: > >>> On Tue, Dec 13, 2022 at 5:58 PM Mark Brown wrote: > On Tue, Dec 13, 2022 at 05:56:30AM -0800, KernelCI bot wrote: > The KernelCI bisection bot found regressions in at least two KMS tests > in the Renesas tree on rk3399-gru-kevin just after the Renesas tree > merged up mainline: > > igt-kms-rockchip.kms_vblank.pipe-A-wait-forked > igt-kms-rockchip.kms_vblank.pipe-A-query-busy > > which it bisected to ca871659ec16 ("drm/bridge: analogix_dp: Support > PSR-exit to disable transition"). I'm not *100%* sure I trust the > bisection but it sure is suspicous that two separate bisects for related > issues landed on the same commit. > >>> > >>> ... which is an old commit, added in v5.19-rc2, and which did not > >>> enter through the renesas tree at all? > >> > >> Do you mean this report shouldn't have been sent to you? > > > > Exactly. > As you can see, Geert is not listed there. Sorry, it was indeed not sent explicitly to me, but I was mentioned, so I noticed. > >> FYI The renesas tree got rebased and inherited a regression which > >> got bisected. > > > > Renesas/master is (usually) never rebased. > > So when did this rebase happen, and why is it relevant? > > Sorry then I guess it wasn't rebased but if mainline was merged > into the branch then it inherited the regressions from mainline > at that point. Yep. > >> The same thing probably happened to other trees. > > > > Indeed, I expect any tree that merged v5.19-rc2 or later to contain > > this regression. That doesn't mean it's a good idea to tell all > > consumers of v5.19-rc2 and later they got a regression (and make it > > sound like the problem was introduced in their trees). > > No, the issues aren't reported more than once. And again, the > tree used to do the bisection is not relevant as what matters is > the commit that it found. > > >> Yes it would be good to have 2nd order regressions based on a > >> reference branch (e.g. mainline in this case) in KernelCI but > > > > Sorry, I don't know what is a "2nd order regression". > > Regressions are basically a delta between results in a given > revision and the previous one on the same branch (new failures). > What I call "2nd order" regressions are a delta of these > regressions compared to another reference branch. For example, > regressions that are in a particular tree and aren't also in > mainline such as the one at hand which isn't specific to renesas. This regression must also be present in mainline (in v6.1). > >> that's for next year. Regardless, it found a commit and the > >> bisection looks legit. > > > > Good. So please report it to the relevant parties. > > > > While bisecting, as soon as you happen to arrive at a commit > > that is already upstream... > > > > > git bisect start > > > # good: [997b2d66ff4e40ef6a5acf76452e8c21104416f7] Merge branch > > 'renesas-next' into renesas-devel > > > git bisect good 997b2d66ff4e40ef6a5acf76452e8c21104416f7 > > > # bad: [710ce3a8a6fbfcd81d8ad361dc9d43c6a785f25e] Merge tag > > 'v6.1' into renesas-devel > > > git bisect bad 710ce3a8a6fbfcd81d8ad361dc9d43c6a785f25e > > > # bad: [044610f8e4155ec0374f7c8307b725b7d01d750c] Merge tag > > 'ata-6.0-rc2' of > > git://git.kernel.org/pub/scm/linux/kernel/git/dlemoal/libata > > > git bisect bad 044610f8e4155ec0374f7c8307b725b7d01d750c > > (which happens here ) > > > > ... there is no point in "blaming" any of the bisection points before. > > > > The git command to check this is (assumed "linus" is a remote pointing > > to Linus' tree): > > > > git branch -a --contains 044610f8e4155ec0374f7c8307b725b7d01d750c > > linus/master > > > > You can use similar commands to find the maintainer's tree for commits > > that are in linux-next and in a for-next branch, but not yet upstream. > > I think it won't be to implement this as soon as we start > tracking regressions specific to each tree since we'll have valid > good/bad revisions that are relevant to the tree in the first > place (if I understand correctly what you mean here). My point is that regressions should be reported against the tree where they truly originated, not against a random tree that merged upstream. Thanks! Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
[Bug 216806] [Raven Ridge] console disappears after framebuffer device loads
https://bugzilla.kernel.org/show_bug.cgi?id=216806 --- Comment #2 from Balazs Vinarz (viniba...@gmail.com) --- Hello Alex, it's set: $ zgrep DRM_FBDEV /proc/config.gz CONFIG_DRM_FBDEV_EMULATION=y CONFIG_DRM_FBDEV_OVERALLOC=100 I just got a hint to use the iommu=pt, but that one didn't work too. -- You may reply to this email to add a comment. You are receiving this mail because: You are watching the assignee of the bug.
Re: renesas/master bisection: igt-kms-rockchip.kms_vblank.pipe-A-wait-forked on rk3399-gru-kevin
Hi Mark, On Wed, Dec 14, 2022 at 3:39 PM Mark Brown wrote: > On Wed, Dec 14, 2022 at 03:16:33PM +0100, Guillaume Tucker wrote: > > Mark, how did you get the list of recipients? > > > There's a command for this btw, which was used when the reports > > were automatically sent to the recipients before we reverted to > > manual filtering to reduce the noise: > > My standard thing is to look at who touched the commit, possibly also > adding seemingly relevant maintainers depending on how good the list > from the commit was (IIRC in this case the commit went entirely through > ChromeOS people so I added relevant DRM submaintainers which turned out > to be a surprisingly large number of people), and relevant lists. > > > As you can see, Geert is not listed there. > > I didn't send the report to Geert as far as I can see, I imagine he saw > it as a result of it going to one of the lists and noticed the mention > of Renesas as the tree, possibly he's got some filter set up to find > things that mention it. The recipient list I have is: > > | To: kernelci-resu...@groups.io, b...@kernelci.org, Brian Norris > | , Sean Paul , > Douglas > | Anderson > | Cc: gtuc...@collabora.com, dri-devel@lists.freedesktop.org, > | linux-arm-ker...@lists.infradead.org, Andrzej Hajda > | , Neil Armstrong > , > | Robert Foss , Laurent Pinchart > | , Jonas Karlman > , > | Jernej Skrabec > > which doesn't mention him at all. Right. I noticed the email because my name was in the body (it's part of the git repo name). The "Re: renesas/master bisection" in the subject immediately triggered my interest. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Re: renesas/master bisection: igt-kms-rockchip.kms_vblank.pipe-A-wait-forked on rk3399-gru-kevin
On Wed, Dec 14, 2022 at 03:27:56PM +0100, Guillaume Tucker wrote: > On 14/12/2022 14:50, Mark Brown wrote: > > As a developer I tend to find this unhelpful, it makes it much more > > likely that the mail will get missed. As a reporter it means there's > > more information to copy into the report. > Well it's up to you or anyone reporting the bisection result. > Base on my personal experience, I always got very quick replies > when doing this. For me on the recipient side it's more a question of if you get any at all. > I don't see your point about copying more information though, I > would just open the mbox in my mail client to reply and paste the > content of the bisection report. With a bit more work this could > be fully automated but that should be part of the bisection > rework using the new API & pipeline so sometime later in 2023... If I'm manually pasing stuff I either have to quote it by hand or feel like I need to edit the automatically generated bits. > > I do notice that the Renesas tree tends to get a *lot* of the bisection > > reports generated for some reason (vastly more than any other tree > > including mainline or -next), however this wasn't sent based on the tree > > at all - I just looked at the people involved with the commit. > In the past month, there were 15 bisection reports on renesas, 7 > on linux-next and 28 on mainline for a total of 79 so 29 in other > trees. So it's true renesas is getting quite a lot of them, it's > not entirely clear to me why that's the case but it's worth > investigating a bit. Yeah, that's vastly more than I'd expect and the overwhelming majority of them are quite clearly not specific to the Renesas tree (things like bootrr failures for non-Renesas boards). signature.asc Description: PGP signature
Re: renesas/master bisection: igt-kms-rockchip.kms_vblank.pipe-A-wait-forked on rk3399-gru-kevin
On Wed, Dec 14, 2022 at 03:16:33PM +0100, Guillaume Tucker wrote: > Mark, how did you get the list of recipients? > There's a command for this btw, which was used when the reports > were automatically sent to the recipients before we reverted to > manual filtering to reduce the noise: My standard thing is to look at who touched the commit, possibly also adding seemingly relevant maintainers depending on how good the list from the commit was (IIRC in this case the commit went entirely through ChromeOS people so I added relevant DRM submaintainers which turned out to be a surprisingly large number of people), and relevant lists. > As you can see, Geert is not listed there. I didn't send the report to Geert as far as I can see, I imagine he saw it as a result of it going to one of the lists and noticed the mention of Renesas as the tree, possibly he's got some filter set up to find things that mention it. The recipient list I have is: | To: kernelci-resu...@groups.io, b...@kernelci.org, Brian Norris | , Sean Paul , Douglas | Anderson | Cc: gtuc...@collabora.com, dri-devel@lists.freedesktop.org, | linux-arm-ker...@lists.infradead.org, Andrzej Hajda | , Neil Armstrong , | Robert Foss , Laurent Pinchart | , Jonas Karlman , | Jernej Skrabec which doesn't mention him at all. > >> Yes it would be good to have 2nd order regressions based on a > >> reference branch (e.g. mainline in this case) in KernelCI but > > > > Sorry, I don't know what is a "2nd order regression". > > Regressions are basically a delta between results in a given > revision and the previous one on the same branch (new failures). > What I call "2nd order" regressions are a delta of these > regressions compared to another reference branch. For example, > regressions that are in a particular tree and aren't also in > mainline such as the one at hand which isn't specific to renesas. Like I said in the other mail there is something going on which means that a *very* lerge proportion of our bisection results are found in the Renesas tree. > >> that's for next year. Regardless, it found a commit and the > >> bisection looks legit. > > Good. So please report it to the relevant parties. That's what I did... > > While bisecting, as soon as you happen to arrive at a commit > > that is already upstream... > > 'ata-6.0-rc2' of > > git://git.kernel.org/pub/scm/linux/kernel/git/dlemoal/libata > > > git bisect bad 044610f8e4155ec0374f7c8307b725b7d01d750c > > (which happens here ) > > ... there is no point in "blaming" any of the bisection points before. I'm not sure I understand the logic here? The goal with a bisection is to identify which commit caused an issue to aid in resolving whatever the problem is. It doesn't really matter if this happens before or after the commit lands in Linus' tree. I do agree that the tree mentioned becomes a bit misleading though. signature.asc Description: PGP signature
Re: [PATCH v2 0/3] drm/tiny: panel-mipi-dbi: Support separate I/O voltage supply
Den 01.12.2022 17.02, skrev Otto Pflüger: > As stated in > Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yml, > the MIPI DBI specification defines two power supplies, one for powering > the panel and one for the I/O voltage. The panel-mipi-dbi driver > currently only supports specifying a single "power-supply" in the > device tree. > > Add support for a second power supply defined in a new "io-supply" > device tree property to make the driver properly configure the voltage > regulators on platforms where separate supplies are used. > > Changes in v2: > - Don't list power-supply in the properties section of >panel-mipi-dbi-spi.yaml because it is already in panel-common.yaml > > Otto Pflüger (3): > drm/mipi-dbi: Support separate I/O regulator > drm/tiny: panel-mipi-dbi: Read I/O supply from DT > dt-bindings: display: panel: mipi-dbi-spi: Add io-supply > Series applied to drm-misc-next. Noralf.
Re: renesas/master bisection: igt-kms-rockchip.kms_vblank.pipe-A-wait-forked on rk3399-gru-kevin
On 14/12/2022 14:50, Mark Brown wrote: > On Wed, Dec 14, 2022 at 01:55:03PM +0100, Guillaume Tucker wrote: > >> Maybe you could retrieve the original thread and rely to it with >> the report? That's the ideal way of following up on a patch I >> think. You can get the mbox file this way: > >> ./kci_bisect get_mbox \ >> --commit ca871659ec1606d33b1e76de8d4cf924cf627e34 \ >> --kdir ~/src/linux > > As a developer I tend to find this unhelpful, it makes it much more > likely that the mail will get missed. As a reporter it means there's > more information to copy into the report. Well it's up to you or anyone reporting the bisection result. Base on my personal experience, I always got very quick replies when doing this. I don't see your point about copying more information though, I would just open the mbox in my mail client to reply and paste the content of the bisection report. With a bit more work this could be fully automated but that should be part of the bisection rework using the new API & pipeline so sometime later in 2023... >>> ... which is an old commit, added in v5.19-rc2, and which did not >>> enter through the renesas tree at all? > >> Do you mean this report shouldn't have been sent to you? > > I do notice that the Renesas tree tends to get a *lot* of the bisection > reports generated for some reason (vastly more than any other tree > including mainline or -next), however this wasn't sent based on the tree > at all - I just looked at the people involved with the commit. In the past month, there were 15 bisection reports on renesas, 7 on linux-next and 28 on mainline for a total of 79 so 29 in other trees. So it's true renesas is getting quite a lot of them, it's not entirely clear to me why that's the case but it's worth investigating a bit. See my other email about "kci_bisect get_recipients". Thanks, Guillaume
Re: renesas/master bisection: igt-kms-rockchip.kms_vblank.pipe-A-wait-forked on rk3399-gru-kevin
On 14/12/2022 15:03, Geert Uytterhoeven wrote: > Hi Guillaume, > > On Wed, Dec 14, 2022 at 1:54 PM Guillaume Tucker > wrote: >> On 14/12/2022 11:06, Geert Uytterhoeven wrote: >>> On Tue, Dec 13, 2022 at 5:58 PM Mark Brown wrote: On Tue, Dec 13, 2022 at 05:56:30AM -0800, KernelCI bot wrote: The KernelCI bisection bot found regressions in at least two KMS tests in the Renesas tree on rk3399-gru-kevin just after the Renesas tree merged up mainline: igt-kms-rockchip.kms_vblank.pipe-A-wait-forked igt-kms-rockchip.kms_vblank.pipe-A-query-busy which it bisected to ca871659ec16 ("drm/bridge: analogix_dp: Support PSR-exit to disable transition"). I'm not *100%* sure I trust the bisection but it sure is suspicous that two separate bisects for related issues landed on the same commit. >>> >>> ... which is an old commit, added in v5.19-rc2, and which did not >>> enter through the renesas tree at all? >> >> Do you mean this report shouldn't have been sent to you? > > Exactly. OK. Mark, how did you get the list of recipients? There's a command for this btw, which was used when the reports were automatically sent to the recipients before we reverted to manual filtering to reduce the noise: $ ./kci_bisect get_recipients --kdir ~/src/linux --commit ca871659ec1606d33b1e76de8d4cf924cf627e34 To: Douglas Anderson Brian Norris Sean Paul Cc: Miaoqian Lin Andrzej Hajda Jonas Karlman Daniel Vetter Robert Foss Jernej Skrabec Heiko Stuebner Sasha Levin linux-ker...@vger.kernel.org dri-devel@lists.freedesktop.org Laurent Pinchart Neil Armstrong Greg Kroah-Hartman David Airlie As you can see, Geert is not listed there. >> FYI The renesas tree got rebased and inherited a regression which >> got bisected. > > Renesas/master is (usually) never rebased. > So when did this rebase happen, and why is it relevant? Sorry then I guess it wasn't rebased but if mainline was merged into the branch then it inherited the regressions from mainline at that point. >> The same thing probably happened to other trees. > > Indeed, I expect any tree that merged v5.19-rc2 or later to contain > this regression. That doesn't mean it's a good idea to tell all > consumers of v5.19-rc2 and later they got a regression (and make it > sound like the problem was introduced in their trees). No, the issues aren't reported more than once. And again, the tree used to do the bisection is not relevant as what matters is the commit that it found. >> Yes it would be good to have 2nd order regressions based on a >> reference branch (e.g. mainline in this case) in KernelCI but > > Sorry, I don't know what is a "2nd order regression". Regressions are basically a delta between results in a given revision and the previous one on the same branch (new failures). What I call "2nd order" regressions are a delta of these regressions compared to another reference branch. For example, regressions that are in a particular tree and aren't also in mainline such as the one at hand which isn't specific to renesas. >> that's for next year. Regardless, it found a commit and the >> bisection looks legit. > > Good. So please report it to the relevant parties. > > While bisecting, as soon as you happen to arrive at a commit > that is already upstream... > > > git bisect start > > # good: [997b2d66ff4e40ef6a5acf76452e8c21104416f7] Merge branch > 'renesas-next' into renesas-devel > > git bisect good 997b2d66ff4e40ef6a5acf76452e8c21104416f7 > > # bad: [710ce3a8a6fbfcd81d8ad361dc9d43c6a785f25e] Merge tag > 'v6.1' into renesas-devel > > git bisect bad 710ce3a8a6fbfcd81d8ad361dc9d43c6a785f25e > > # bad: [044610f8e4155ec0374f7c8307b725b7d01d750c] Merge tag > 'ata-6.0-rc2' of > git://git.kernel.org/pub/scm/linux/kernel/git/dlemoal/libata > > git bisect bad 044610f8e4155ec0374f7c8307b725b7d01d750c > (which happens here ) > > ... there is no point in "blaming" any of the bisection points before. > > The git command to check this is (assumed "linus" is a remote pointing > to Linus' tree): > > git branch -a --contains 044610f8e4155ec0374f7c8307b725b7d01d750c > linus/master > > You can use similar commands to find the maintainer's tree for commits > that are in linux-next and in a for-next branch, but not yet upstream. I think it won't be to implement this as soon as we start tracking regressions specific to each tree since we'll have valid good/bad revisions that are relevant to the tree in the first place (if I understand correctly what you mean here). Thanks, Guillaume
Re: renesas/master bisection: igt-kms-rockchip.kms_vblank.pipe-A-wait-forked on rk3399-gru-kevin
Hi Guillaume, On Wed, Dec 14, 2022 at 1:54 PM Guillaume Tucker wrote: > On 14/12/2022 11:06, Geert Uytterhoeven wrote: > > On Tue, Dec 13, 2022 at 5:58 PM Mark Brown wrote: > >> On Tue, Dec 13, 2022 at 05:56:30AM -0800, KernelCI bot wrote: > >> The KernelCI bisection bot found regressions in at least two KMS tests > >> in the Renesas tree on rk3399-gru-kevin just after the Renesas tree > >> merged up mainline: > >> > >>igt-kms-rockchip.kms_vblank.pipe-A-wait-forked > >>igt-kms-rockchip.kms_vblank.pipe-A-query-busy > >> > >> which it bisected to ca871659ec16 ("drm/bridge: analogix_dp: Support > >> PSR-exit to disable transition"). I'm not *100%* sure I trust the > >> bisection but it sure is suspicous that two separate bisects for related > >> issues landed on the same commit. > > > > ... which is an old commit, added in v5.19-rc2, and which did not > > enter through the renesas tree at all? > > Do you mean this report shouldn't have been sent to you? Exactly. > FYI The renesas tree got rebased and inherited a regression which > got bisected. Renesas/master is (usually) never rebased. So when did this rebase happen, and why is it relevant? > The same thing probably happened to other trees. Indeed, I expect any tree that merged v5.19-rc2 or later to contain this regression. That doesn't mean it's a good idea to tell all consumers of v5.19-rc2 and later they got a regression (and make it sound like the problem was introduced in their trees). > Yes it would be good to have 2nd order regressions based on a > reference branch (e.g. mainline in this case) in KernelCI but Sorry, I don't know what is a "2nd order regression". > that's for next year. Regardless, it found a commit and the > bisection looks legit. Good. So please report it to the relevant parties. While bisecting, as soon as you happen to arrive at a commit that is already upstream... > git bisect start > # good: [997b2d66ff4e40ef6a5acf76452e8c21104416f7] Merge branch 'renesas-next' into renesas-devel > git bisect good 997b2d66ff4e40ef6a5acf76452e8c21104416f7 > # bad: [710ce3a8a6fbfcd81d8ad361dc9d43c6a785f25e] Merge tag 'v6.1' into renesas-devel > git bisect bad 710ce3a8a6fbfcd81d8ad361dc9d43c6a785f25e > # bad: [044610f8e4155ec0374f7c8307b725b7d01d750c] Merge tag 'ata-6.0-rc2' of git://git.kernel.org/pub/scm/linux/kernel/git/dlemoal/libata > git bisect bad 044610f8e4155ec0374f7c8307b725b7d01d750c (which happens here ) ... there is no point in "blaming" any of the bisection points before. The git command to check this is (assumed "linus" is a remote pointing to Linus' tree): git branch -a --contains 044610f8e4155ec0374f7c8307b725b7d01d750c linus/master You can use similar commands to find the maintainer's tree for commits that are in linux-next and in a for-next branch, but not yet upstream. Thanks! Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Re: [PATCH v4 1/7] accel/ivpu: Introduce a new DRM driver for Intel VPU
On Thu, Dec 8, 2022 at 1:08 PM Jacek Lawrynowicz wrote: > + > +static inline int ivpu_hw_info_init(struct ivpu_device *vdev) > +{ > + return vdev->hw->ops->info_init(vdev); > +}; > + > +static inline int ivpu_hw_power_up(struct ivpu_device *vdev) > +{ > + ivpu_dbg(vdev, PM, "HW power up\n"); > + > + return vdev->hw->ops->power_up(vdev); > +}; > + > +static inline int ivpu_hw_boot_fw(struct ivpu_device *vdev) > +{ > + return vdev->hw->ops->boot_fw(vdev); > +}; > + > +static inline bool ivpu_hw_is_idle(struct ivpu_device *vdev) > +{ > + return vdev->hw->ops->is_idle(vdev); > +}; > + > +static inline int ivpu_hw_power_down(struct ivpu_device *vdev) > +{ > + ivpu_dbg(vdev, PM, "HW power down\n"); > + > + return vdev->hw->ops->power_down(vdev); > +}; > + > +static inline void ivpu_hw_wdt_disable(struct ivpu_device *vdev) > +{ > + vdev->hw->ops->wdt_disable(vdev); > +}; > + > +/* Register indirect accesses */ > +static inline u32 ivpu_hw_reg_pll_freq_get(struct ivpu_device *vdev) > +{ > + return vdev->hw->ops->reg_pll_freq_get(vdev); > +}; > + > +static inline u32 ivpu_hw_reg_telemetry_offset_get(struct ivpu_device *vdev) > +{ > + return vdev->hw->ops->reg_telemetry_offset_get(vdev); > +}; > + > +static inline u32 ivpu_hw_reg_telemetry_size_get(struct ivpu_device *vdev) > +{ > + return vdev->hw->ops->reg_telemetry_size_get(vdev); > +}; > + > +static inline u32 ivpu_hw_reg_telemetry_enable_get(struct ivpu_device *vdev) > +{ > + return vdev->hw->ops->reg_telemetry_enable_get(vdev); > +}; > + > +static inline void ivpu_hw_reg_db_set(struct ivpu_device *vdev, u32 db_id) > +{ > + vdev->hw->ops->reg_db_set(vdev, db_id); > +}; > + > +static inline u32 ivpu_hw_reg_ipc_rx_addr_get(struct ivpu_device *vdev) > +{ > + return vdev->hw->ops->reg_ipc_rx_addr_get(vdev); > +}; > + > +static inline u32 ivpu_hw_reg_ipc_rx_count_get(struct ivpu_device *vdev) > +{ > + return vdev->hw->ops->reg_ipc_rx_count_get(vdev); > +}; > + > +static inline void ivpu_hw_reg_ipc_tx_set(struct ivpu_device *vdev, u32 > vpu_addr) > +{ > + vdev->hw->ops->reg_ipc_tx_set(vdev, vpu_addr); > +}; > + > +static inline void ivpu_hw_irq_clear(struct ivpu_device *vdev) > +{ > + vdev->hw->ops->irq_clear(vdev); > +}; > + > +static inline void ivpu_hw_irq_enable(struct ivpu_device *vdev) > +{ > + vdev->hw->ops->irq_enable(vdev); > +}; > + > +static inline void ivpu_hw_irq_disable(struct ivpu_device *vdev) > +{ > + vdev->hw->ops->irq_disable(vdev); > +}; Why hide all the calls to the hw ops functions inside inline functions ? I mean, it just makes it one step longer to read the code... Why not directly call vdev->hw->ops->operation ? > diff --git a/include/uapi/drm/ivpu_drm.h b/include/uapi/drm/ivpu_drm.h > new file mode 100644 > index ..922cbf30ce34 > --- /dev/null > +++ b/include/uapi/drm/ivpu_drm.h > @@ -0,0 +1,95 @@ > +/* SPDX-License-Identifier: GPL-2.0-only WITH Linux-syscall-note */ > +/* > + * Copyright (C) 2020-2022 Intel Corporation > + */ > + > +#ifndef __UAPI_IVPU_DRM_H__ > +#define __UAPI_IVPU_DRM_H__ > + > +#include "drm.h" > + > +#if defined(__cplusplus) > +extern "C" { > +#endif > + > +#define DRM_IVPU_DRIVER_MAJOR 1 > +#define DRM_IVPU_DRIVER_MINOR 0 I would prefer not to define versions for the driver. Don't get in this trap of trying to keep a version number updated over time. It breaks down the moment you get the code merged into the kernel tree as the driver is what is in the kernel, not separate from it. Also think of the stable kernels, you will backport fixes to those for the driver as part of the development process. So what "version" are they now? They are some mis-match of the old one with new fixes, and as such, the version number now means nothing. Just stick to the version number of the kernel itself, that's the only sane way. btw, I talked to Daniel about this and he told me this major/minor/patchlevel is legacy in drm and should be deprecated, so I'm going to send a patch to document that it is deprecated and how to use getcap ioctl to find out the features the driver support. Thanks, Oded
Re: renesas/master bisection: igt-kms-rockchip.kms_vblank.pipe-A-wait-forked on rk3399-gru-kevin
On Wed, Dec 14, 2022 at 01:55:03PM +0100, Guillaume Tucker wrote: > Maybe you could retrieve the original thread and rely to it with > the report? That's the ideal way of following up on a patch I > think. You can get the mbox file this way: > ./kci_bisect get_mbox \ > --commit ca871659ec1606d33b1e76de8d4cf924cf627e34 \ > --kdir ~/src/linux As a developer I tend to find this unhelpful, it makes it much more likely that the mail will get missed. As a reporter it means there's more information to copy into the report. > > ... which is an old commit, added in v5.19-rc2, and which did not > > enter through the renesas tree at all? > Do you mean this report shouldn't have been sent to you? I do notice that the Renesas tree tends to get a *lot* of the bisection reports generated for some reason (vastly more than any other tree including mainline or -next), however this wasn't sent based on the tree at all - I just looked at the people involved with the commit. signature.asc Description: PGP signature
[PATCH v10 18/18] drm: bridge: samsung-dsim: Add i.MX8M Plus support
From: Marek Vasut Add extras to support i.MX8M Plus. The main change is the removal of HS/VS/DE signal inversion in the LCDIFv3-DSIM glue logic, otherwise the implementation of this IP in i.MX8M Plus is very much compatible with the i.MX8M Mini/Nano one. Signed-off-by: Marek Vasut Signed-off-by: Jagan Teki --- Changes for v10: - none Changes for v9: - added im8mp in DSIM_STATE_REINITIALIZED check - drop previous = NULL check drivers/gpu/drm/bridge/samsung-dsim.c | 23 +++ include/drm/bridge/samsung-dsim.h | 1 + 2 files changed, 24 insertions(+) diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c index b14efb6fca91..13ce21af3b4c 100644 --- a/drivers/gpu/drm/bridge/samsung-dsim.c +++ b/drivers/gpu/drm/bridge/samsung-dsim.c @@ -479,6 +479,7 @@ samsung_dsim_types[DSIM_TYPE_COUNT] = { [DSIM_TYPE_EXYNOS5422] = _dsi_driver_data, [DSIM_TYPE_EXYNOS5433] = _dsi_driver_data, [DSIM_TYPE_IMX8MM] = _dsi_driver_data, + [DSIM_TYPE_IMX8MP] = _dsi_driver_data, }; static inline struct samsung_dsim *host_to_dsi(struct mipi_dsi_host *h) @@ -1461,10 +1462,17 @@ static int samsung_dsim_atomic_check(struct drm_bridge *bridge, * 13.6.2.7.2 RGB interface * both claim "Vsync, Hsync, and VDEN are active high signals.", the * LCDIF must generate inverted HS/VS/DE signals, i.e. active LOW. +* +* The i.MX8M Plus glue logic between LCDIFv3 and DSIM does not +* implement the same behavior, therefore LCDIFv3 must generate +* HS/VS/DE signals active HIGH. */ if (dsi->plat_data->hw_type == DSIM_TYPE_IMX8MM) { adjusted_mode->flags |= (DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC); adjusted_mode->flags &= ~(DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC); + } else if (dsi->plat_data->hw_type == DSIM_TYPE_IMX8MP) { + adjusted_mode->flags &= ~(DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC); + adjusted_mode->flags |= (DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC); } return 0; @@ -1639,6 +1647,10 @@ static const struct samsung_dsim_host_ops generic_dsim_host_ops = { .unregister_host = generic_dsim_unregister_host, }; +static const struct drm_bridge_timings samsung_dsim_bridge_timings_de_high = { + .input_bus_flags = DRM_BUS_FLAG_DE_HIGH, +}; + static const struct drm_bridge_timings samsung_dsim_bridge_timings_de_low = { .input_bus_flags = DRM_BUS_FLAG_DE_LOW, }; @@ -1728,6 +1740,8 @@ int samsung_dsim_probe(struct platform_device *pdev) /* DE_LOW: i.MX8M Mini/Nano LCDIF-DSIM glue logic inverts HS/VS/DE */ if (dsi->plat_data->hw_type == DSIM_TYPE_IMX8MM) dsi->bridge.timings = _dsim_bridge_timings_de_low; + else + dsi->bridge.timings = _dsim_bridge_timings_de_high; if (dsi->plat_data->host_ops && dsi->plat_data->host_ops->register_host) ret = dsi->plat_data->host_ops->register_host(dsi); @@ -1833,11 +1847,20 @@ static const struct samsung_dsim_plat_data samsung_dsim_imx8mm_pdata = { .host_ops = _dsim_host_ops, }; +static const struct samsung_dsim_plat_data samsung_dsim_imx8mp_pdata = { + .hw_type = DSIM_TYPE_IMX8MP, + .host_ops = _dsim_host_ops, +}; + static const struct of_device_id samsung_dsim_of_match[] = { { .compatible = "fsl,imx8mm-mipi-dsim", .data = _dsim_imx8mm_pdata, }, + { + .compatible = "fsl,imx8mp-mipi-dsim", + .data = _dsim_imx8mp_pdata, + }, { /* sentinel. */ } }; MODULE_DEVICE_TABLE(of, samsung_dsim_of_match); diff --git a/include/drm/bridge/samsung-dsim.h b/include/drm/bridge/samsung-dsim.h index e54e62f5f632..5b3dfcaf9fac 100644 --- a/include/drm/bridge/samsung-dsim.h +++ b/include/drm/bridge/samsung-dsim.h @@ -27,6 +27,7 @@ enum samsung_dsim_type { DSIM_TYPE_EXYNOS5422, DSIM_TYPE_EXYNOS5433, DSIM_TYPE_IMX8MM, + DSIM_TYPE_IMX8MP, DSIM_TYPE_COUNT, }; -- 2.25.1
[PATCH v10 14/18] drm: bridge: Generalize Exynos-DSI driver into a Samsung DSIM bridge
Samsung MIPI DSIM controller is common DSI IP that can be used in various SoCs like Exynos, i.MX8M Mini/Nano. In order to access this DSI controller between various platform SoCs, the ideal way to incorporate this in the drm stack is via the drm bridge driver. We already have a consolidated code for supporting component and bridge based DRM drivers, so keep the exynos component based code in existing exynos_drm_dsi.c and move generic bridge code as part of samsung-dsim.c Signed-off-by: Marek Szyprowski Signed-off-by: Jagan Teki --- Changes for v10: - don't add new code - move the files and update samsung_dsim names - update commit message Changes for v9: - drop the bridge attach fix for Exynos Changes for v8: - update the commit message head Changes for v7: - fix the drm bridge attach chain for exynos drm dsi driver - fix the hw_type checking logic Changes for v6: - handle previous bridge pointer for exynos dsi Changes for v5: - [mszyprow] reworked driver initialization using the same approach as in drivers/gpu/drm/{exynos/exynos_dp.c, bridge/analogix/analogix_dp_core.c}, removed weak functions, moved exynos_dsi_driver back to exynos_drm_dsi.c and restored integration with exynos_drm custom initialization. - updated the commit message [Jagan] Changes for v4: - include Inki Dae in MAINTAINERS - remove dsi_driver probe in exynos_drm_drv to support multi-arch build Changes for v3: - restore gpio related fixes - restore proper bridge chain - rework initialization issue - fix header includes in proper way Changes for v2: - fixed exynos dsi driver conversion (Marek Szyprowski) - updated commit message - updated MAINTAINERS file Changes for v1: - don't maintain component_ops in bridge driver - don't maintain platform glue code in bridge driver - add platform-specific glue code and make a common bridge MAINTAINERS |9 + drivers/gpu/drm/bridge/Kconfig | 12 + drivers/gpu/drm/bridge/Makefile |1 + drivers/gpu/drm/bridge/samsung-dsim.c | 1816 drivers/gpu/drm/exynos/Kconfig |1 + drivers/gpu/drm/exynos/exynos_drm_dsi.c | 2007 +-- include/drm/bridge/samsung-dsim.h | 118 ++ 7 files changed, 2027 insertions(+), 1937 deletions(-) create mode 100644 drivers/gpu/drm/bridge/samsung-dsim.c create mode 100644 include/drm/bridge/samsung-dsim.h diff --git a/MAINTAINERS b/MAINTAINERS index f3edff6b1cad..9a37cce05062 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -6675,6 +6675,15 @@ T: git git://anongit.freedesktop.org/drm/drm-misc F: Documentation/devicetree/bindings/display/panel/samsung,lms397kf04.yaml F: drivers/gpu/drm/panel/panel-samsung-db7430.c +DRM DRIVER FOR SAMSUNG MIPI DSIM BRIDGE +M: Jagan Teki +M: Marek Szyprowski +M: Inki Dae S: Maintained diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig index 57946d80b02d..8e85dac9f53e 100644 --- a/drivers/gpu/drm/bridge/Kconfig +++ b/drivers/gpu/drm/bridge/Kconfig @@ -231,6 +231,18 @@ config DRM_PARADE_PS8640 The PS8640 is a high-performance and low-power MIPI DSI to eDP converter +config DRM_SAMSUNG_DSIM + tristate "Samsung MIPI DSIM bridge driver" + depends on COMMON_CLK + depends on OF && HAS_IOMEM + select DRM_KMS_HELPER + select DRM_MIPI_DSI + select DRM_PANEL_BRIDGE + help + The Samsung MIPI DSIM bridge controller driver. + This MIPI DSIM bridge can be found it on Exynos SoCs and + NXP's i.MX8M Mini/Nano. + config DRM_SIL_SII8620 tristate "Silicon Image SII8620 HDMI/MHL bridge" depends on OF diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile index 1884803c6860..dae843723991 100644 --- a/drivers/gpu/drm/bridge/Makefile +++ b/drivers/gpu/drm/bridge/Makefile @@ -15,6 +15,7 @@ obj-$(CONFIG_DRM_MEGACHIPS_STDP_GE_B850V3_FW) += megachips-stdp-ge-b850v obj-$(CONFIG_DRM_NXP_PTN3460) += nxp-ptn3460.o obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o obj-$(CONFIG_DRM_PARADE_PS8640) += parade-ps8640.o +obj-$(CONFIG_DRM_SAMSUNG_DSIM) += samsung-dsim.o obj-$(CONFIG_DRM_SIL_SII8620) += sil-sii8620.o obj-$(CONFIG_DRM_SII902X) += sii902x.o obj-$(CONFIG_DRM_SII9234) += sii9234.o diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c new file mode 100644 index ..dd27935081a3 --- /dev/null +++ b/drivers/gpu/drm/bridge/samsung-dsim.c @@ -0,0 +1,1816 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Samsung MIPI DSIM bridge driver. + * + * Copyright (C) 2021 Amarula Solutions(India) + * Copyright (c) 2014 Samsung Electronics Co., Ltd + * Author: Jagan Teki + * + * Based on exynos_drm_dsi from + * Tomasz Figa + */ + +#include + +#include +#include +#include +#include +#include +#include + +#include + +#include +#include +#include + +/* returns true iff both arguments logically differs */
[PATCH v10 16/18] drm: bridge: samsung-dsim: Add i.MX8M Mini/Nano support
Samsung MIPI DSIM master can also be found in i.MX8M Mini/Nano SoC. Add compatible and associated driver_data for it. Reviewed-by: Laurent Pinchart Signed-off-by: Marek Szyprowski Signed-off-by: Jagan Teki --- Changes for v10, v9: - none Changed for v8: - fix and update the comment Changes for v7, v6: - none Changes for v3: - enable DSIM_QUIRK_FIXUP_SYNC_POL quirk Changes for v5: - [mszyprow] rebased and adjusted to the new driver initialization - drop quirk Changes for v4: - none Changes for v3: - enable DSIM_QUIRK_FIXUP_SYNC_POL quirk Changes for v2: - collect Laurent r-b Changes for v1: - none drivers/gpu/drm/bridge/samsung-dsim.c | 44 +++ 1 file changed, 44 insertions(+) diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c index dd27935081a3..b14efb6fca91 100644 --- a/drivers/gpu/drm/bridge/samsung-dsim.c +++ b/drivers/gpu/drm/bridge/samsung-dsim.c @@ -376,6 +376,24 @@ static const unsigned int exynos5433_reg_values[] = { [PHYTIMING_HS_TRAIL] = DSIM_PHYTIMING2_HS_TRAIL(0x0c), }; +static const unsigned int imx8mm_dsim_reg_values[] = { + [RESET_TYPE] = DSIM_SWRST, + [PLL_TIMER] = 500, + [STOP_STATE_CNT] = 0xf, + [PHYCTRL_ULPS_EXIT] = 0, + [PHYCTRL_VREG_LP] = 0, + [PHYCTRL_SLEW_UP] = 0, + [PHYTIMING_LPX] = DSIM_PHYTIMING_LPX(0x06), + [PHYTIMING_HS_EXIT] = DSIM_PHYTIMING_HS_EXIT(0x0b), + [PHYTIMING_CLK_PREPARE] = DSIM_PHYTIMING1_CLK_PREPARE(0x07), + [PHYTIMING_CLK_ZERO] = DSIM_PHYTIMING1_CLK_ZERO(0x26), + [PHYTIMING_CLK_POST] = DSIM_PHYTIMING1_CLK_POST(0x0d), + [PHYTIMING_CLK_TRAIL] = DSIM_PHYTIMING1_CLK_TRAIL(0x08), + [PHYTIMING_HS_PREPARE] = DSIM_PHYTIMING2_HS_PREPARE(0x08), + [PHYTIMING_HS_ZERO] = DSIM_PHYTIMING2_HS_ZERO(0x0d), + [PHYTIMING_HS_TRAIL] = DSIM_PHYTIMING2_HS_TRAIL(0x0b), +}; + static const struct samsung_dsim_driver_data exynos3_dsi_driver_data = { .reg_ofs = exynos_reg_ofs, .plltmr_reg = 0x50, @@ -437,6 +455,22 @@ static const struct samsung_dsim_driver_data exynos5422_dsi_driver_data = { .reg_values = exynos5422_reg_values, }; +static const struct samsung_dsim_driver_data imx8mm_dsi_driver_data = { + .reg_ofs = exynos5433_reg_ofs, + .plltmr_reg = 0xa0, + .has_clklane_stop = 1, + .num_clks = 2, + .max_freq = 2100, + .wait_for_reset = 0, + .num_bits_resol = 12, + /* +* Unlike Exynos, PLL_P(PMS_P) offset 14 is used in i.MX8M Mini/Nano/Plus +* downstream driver - drivers/gpu/drm/bridge/sec-dsim.c +*/ + .pll_p_offset = 14, + .reg_values = imx8mm_dsim_reg_values, +}; + static const struct samsung_dsim_driver_data * samsung_dsim_types[DSIM_TYPE_COUNT] = { [DSIM_TYPE_EXYNOS3250] = _dsi_driver_data, @@ -444,6 +478,7 @@ samsung_dsim_types[DSIM_TYPE_COUNT] = { [DSIM_TYPE_EXYNOS5410] = _dsi_driver_data, [DSIM_TYPE_EXYNOS5422] = _dsi_driver_data, [DSIM_TYPE_EXYNOS5433] = _dsi_driver_data, + [DSIM_TYPE_IMX8MM] = _dsi_driver_data, }; static inline struct samsung_dsim *host_to_dsi(struct mipi_dsi_host *h) @@ -1793,7 +1828,16 @@ const struct dev_pm_ops samsung_dsim_pm_ops = { }; EXPORT_SYMBOL_GPL(samsung_dsim_pm_ops); +static const struct samsung_dsim_plat_data samsung_dsim_imx8mm_pdata = { + .hw_type = DSIM_TYPE_IMX8MM, + .host_ops = _dsim_host_ops, +}; + static const struct of_device_id samsung_dsim_of_match[] = { + { + .compatible = "fsl,imx8mm-mipi-dsim", + .data = _dsim_imx8mm_pdata, + }, { /* sentinel. */ } }; MODULE_DEVICE_TABLE(of, samsung_dsim_of_match); -- 2.25.1
[PATCH 10/10] drm: bridge: it66121: Add support for the IT6610
Add support for the IT6610 HDMI encoder. The hardware is very similar, and therefore the driver did not require too many changes. Some bits are only available on the IT66121, and vice-versa. Also, the IT6610 requires specific polarities on the DE and pixel lines. Signed-off-by: Paul Cercueil --- drivers/gpu/drm/bridge/ite-it66121.c | 108 +-- 1 file changed, 86 insertions(+), 22 deletions(-) diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c index 43b027b85b8e..b34860871627 100644 --- a/drivers/gpu/drm/bridge/ite-it66121.c +++ b/drivers/gpu/drm/bridge/ite-it66121.c @@ -68,6 +68,7 @@ #define IT66121_AFE_XP_ENO BIT(4) #define IT66121_AFE_XP_RESETB BIT(3) #define IT66121_AFE_XP_PWDIBIT(2) +#define IT6610_AFE_XP_BYPASS BIT(0) #define IT66121_AFE_IP_REG 0x64 #define IT66121_AFE_IP_GAINBIT BIT(7) @@ -284,7 +285,13 @@ #define IT66121_AFE_CLK_HIGH 8 /* Khz */ +enum chip_id { + ID_IT6610, + ID_IT66121, +}; + struct it66121_chip_info { + enum chip_id id; u16 vid, pid; }; @@ -391,16 +398,22 @@ static int it66121_configure_afe(struct it66121_ctx *ctx, ret = regmap_write_bits(ctx->regmap, IT66121_AFE_IP_REG, IT66121_AFE_IP_GAINBIT | - IT66121_AFE_IP_ER0 | - IT66121_AFE_IP_EC1, + IT66121_AFE_IP_ER0, IT66121_AFE_IP_GAINBIT); if (ret) return ret; - ret = regmap_write_bits(ctx->regmap, IT66121_AFE_XP_EC1_REG, - IT66121_AFE_XP_EC1_LOWCLK, 0x80); - if (ret) - return ret; + if (ctx->info->id == ID_IT66121) { + ret = regmap_write_bits(ctx->regmap, IT66121_AFE_IP_REG, + IT66121_AFE_IP_EC1, 0); + if (ret) + return ret; + + ret = regmap_write_bits(ctx->regmap, IT66121_AFE_XP_EC1_REG, + IT66121_AFE_XP_EC1_LOWCLK, 0x80); + if (ret) + return ret; + } } else { ret = regmap_write_bits(ctx->regmap, IT66121_AFE_XP_REG, IT66121_AFE_XP_GAINBIT | @@ -411,17 +424,24 @@ static int it66121_configure_afe(struct it66121_ctx *ctx, ret = regmap_write_bits(ctx->regmap, IT66121_AFE_IP_REG, IT66121_AFE_IP_GAINBIT | - IT66121_AFE_IP_ER0 | - IT66121_AFE_IP_EC1, IT66121_AFE_IP_ER0 | - IT66121_AFE_IP_EC1); + IT66121_AFE_IP_ER0, + IT66121_AFE_IP_ER0); if (ret) return ret; - ret = regmap_write_bits(ctx->regmap, IT66121_AFE_XP_EC1_REG, - IT66121_AFE_XP_EC1_LOWCLK, - IT66121_AFE_XP_EC1_LOWCLK); - if (ret) - return ret; + if (ctx->info->id == ID_IT66121) { + ret = regmap_write_bits(ctx->regmap, IT66121_AFE_IP_REG, + IT66121_AFE_IP_EC1, + IT66121_AFE_IP_EC1); + if (ret) + return ret; + + ret = regmap_write_bits(ctx->regmap, IT66121_AFE_XP_EC1_REG, + IT66121_AFE_XP_EC1_LOWCLK, + IT66121_AFE_XP_EC1_LOWCLK); + if (ret) + return ret; + } } /* Clear reset flags */ @@ -430,6 +450,14 @@ static int it66121_configure_afe(struct it66121_ctx *ctx, if (ret) return ret; + if (ctx->info->id == ID_IT6610) { + ret = regmap_write_bits(ctx->regmap, IT66121_AFE_XP_REG, + IT6610_AFE_XP_BYPASS, + IT6610_AFE_XP_BYPASS); + if (ret) + return ret; + } + return it66121_fire_afe(ctx); } @@ -491,7 +519,6 @@ static int it66121_get_edid_block(void *context, u8 *buf, unsigned int block, size_t len) { struct it66121_ctx *ctx = context; - unsigned int val; int remain = len;
[PATCH v10 15/18] dt-bindings: display: exynos: dsim: Add NXP i.MX8M Mini/Nano support
Samsung MIPI DSIM bridge can also be found in i.MX8M Mini/Nano SoC. Add dt-bingings for it. Cc: devicet...@vger.kernel.org Acked-by: Rob Herring Signed-off-by: Jagan Teki --- Changes for v10, v9: - none Changes for v8: - add comment to include i.MX8M Nano. Changes for v7, v6, v5, v4: - none Changes for v3: - collect Rob Acked-by Changes for v2: - updated comments Changes for v1: - new patch Documentation/devicetree/bindings/display/exynos/exynos_dsim.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/Documentation/devicetree/bindings/display/exynos/exynos_dsim.txt b/Documentation/devicetree/bindings/display/exynos/exynos_dsim.txt index be377786e8cd..5133d4d39190 100644 --- a/Documentation/devicetree/bindings/display/exynos/exynos_dsim.txt +++ b/Documentation/devicetree/bindings/display/exynos/exynos_dsim.txt @@ -7,6 +7,7 @@ Required properties: "samsung,exynos5410-mipi-dsi" /* for Exynos5410/5420/5440 SoCs */ "samsung,exynos5422-mipi-dsi" /* for Exynos5422/5800 SoCs */ "samsung,exynos5433-mipi-dsi" /* for Exynos5433 SoCs */ + "fsl,imx8mm-mipi-dsim" /* for i.MX8M Mini/Nano SoCs */ - reg: physical base address and length of the registers set for the device - interrupts: should contain DSI interrupt - clocks: list of clock specifiers, must contain an entry for each required -- 2.25.1
[PATCH v10 17/18] dt-bindings: display: exynos: dsim: Add NXP i.MX8M Plus support
Samsung MIPI DSIM bridge can also be found in i.MX8M Plus SoC. Add dt-bingings for it. Cc: devicet...@vger.kernel.org Cc: Rob Herring Signed-off-by: Jagan Teki --- Changes for v10, v9: - none Documentation/devicetree/bindings/display/exynos/exynos_dsim.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/Documentation/devicetree/bindings/display/exynos/exynos_dsim.txt b/Documentation/devicetree/bindings/display/exynos/exynos_dsim.txt index 5133d4d39190..2a5f0889ec32 100644 --- a/Documentation/devicetree/bindings/display/exynos/exynos_dsim.txt +++ b/Documentation/devicetree/bindings/display/exynos/exynos_dsim.txt @@ -8,6 +8,7 @@ Required properties: "samsung,exynos5422-mipi-dsi" /* for Exynos5422/5800 SoCs */ "samsung,exynos5433-mipi-dsi" /* for Exynos5433 SoCs */ "fsl,imx8mm-mipi-dsim" /* for i.MX8M Mini/Nano SoCs */ + "fsl,imx8mp-mipi-dsim" /* for i.MX8M Plus SoCs */ - reg: physical base address and length of the registers set for the device - interrupts: should contain DSI interrupt - clocks: list of clock specifiers, must contain an entry for each required -- 2.25.1
[PATCH 09/10] drm: bridge: it66121: Move VID/PID to new it66121_chip_info structure
This will make it easier later to introduce support for new chips in this driver. Signed-off-by: Paul Cercueil --- drivers/gpu/drm/bridge/ite-it66121.c | 27 +++ 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c index 7972003d4776..43b027b85b8e 100644 --- a/drivers/gpu/drm/bridge/ite-it66121.c +++ b/drivers/gpu/drm/bridge/ite-it66121.c @@ -35,10 +35,6 @@ #define IT66121_DEVICE_ID0_REG 0x02 #define IT66121_DEVICE_ID1_REG 0x03 -#define IT66121_VENDOR_ID0 0x54 -#define IT66121_VENDOR_ID1 0x49 -#define IT66121_DEVICE_ID0 0x12 -#define IT66121_DEVICE_ID1 0x06 #define IT66121_REVISION_MASK GENMASK(7, 4) #define IT66121_DEVICE_ID1_MASKGENMASK(3, 0) @@ -286,13 +282,12 @@ #define IT66121_AUD_SWL_16BIT 0x2 #define IT66121_AUD_SWL_NOT_INDICATED 0x0 -#define IT66121_VENDOR_ID0 0x54 -#define IT66121_VENDOR_ID1 0x49 -#define IT66121_DEVICE_ID0 0x12 -#define IT66121_DEVICE_ID1 0x06 -#define IT66121_DEVICE_MASK0x0F #define IT66121_AFE_CLK_HIGH 8 /* Khz */ +struct it66121_chip_info { + u16 vid, pid; +}; + struct it66121_ctx { struct regmap *regmap; struct drm_bridge bridge; @@ -311,6 +306,7 @@ struct it66121_ctx { u8 swl; bool auto_cts; } audio; + const struct it66121_chip_info *info; }; static const struct regmap_range_cfg it66121_regmap_banks[] = { @@ -1451,6 +1447,7 @@ static const char * const it66121_supplies[] = { static int it66121_probe(struct i2c_client *client) { + const struct i2c_device_id *id = i2c_client_get_device_id(client); u32 revision_id, vendor_ids[2] = { 0 }, device_ids[2] = { 0 }; struct device_node *ep; int ret; @@ -1472,6 +1469,7 @@ static int it66121_probe(struct i2c_client *client) ctx->dev = dev; ctx->client = client; + ctx->info = (const struct it66121_chip_info *) id->driver_data; of_property_read_u32(ep, "bus-width", >bus_width); of_node_put(ep); @@ -1523,8 +1521,8 @@ static int it66121_probe(struct i2c_client *client) revision_id = FIELD_GET(IT66121_REVISION_MASK, device_ids[1]); device_ids[1] &= IT66121_DEVICE_ID1_MASK; - if (vendor_ids[0] != IT66121_VENDOR_ID0 || vendor_ids[1] != IT66121_VENDOR_ID1 || - device_ids[0] != IT66121_DEVICE_ID0 || device_ids[1] != IT66121_DEVICE_ID1) { + if ((vendor_ids[1] << 8 | vendor_ids[0]) != ctx->info->vid || + (device_ids[1] << 8 | device_ids[0]) != ctx->info->pid) { return -ENODEV; } @@ -1563,8 +1561,13 @@ static const struct of_device_id it66121_dt_match[] = { }; MODULE_DEVICE_TABLE(of, it66121_dt_match); +static const struct it66121_chip_info it66121_chip_info = { + .vid = 0x4954, + .pid = 0x0612, +}; + static const struct i2c_device_id it66121_id[] = { - { "it66121", 0 }, + { "it66121", (kernel_ulong_t) _chip_info }, { } }; MODULE_DEVICE_TABLE(i2c, it66121_id); -- 2.35.1
[PATCH v10 13/18] drm: exynos: dsi: Add Exynos based host irq hooks
Enable and disable of te_gpio's are Exynos platform specific irq handling, so add the exynos based irq operations and hook them for exynos plat_data. Signed-off-by: Jagan Teki --- Changes for v10: - split from previous series patch "drm: bridge: Generalize Exynos-DSI driver into a Samsung DSIM bridge" drivers/gpu/drm/exynos/exynos_drm_dsi.c | 55 + 1 file changed, 47 insertions(+), 8 deletions(-) diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c index 819131a36b96..580e06595b37 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c @@ -291,9 +291,15 @@ struct exynos_dsim_host_ops { int (*detach)(struct exynos_dsi *dsim, struct mipi_dsi_device *device); }; +struct exynos_dsim_irq_ops { + void (*enable)(struct exynos_dsi *dsim); + void (*disable)(struct exynos_dsi *dsim); +}; + struct exynos_dsi_plat_data { enum exynos_dsi_type hw_type; const struct exynos_dsim_host_ops *host_ops; + const struct exynos_dsim_irq_ops *irq_ops; }; struct exynos_dsi { @@ -308,7 +314,6 @@ struct exynos_dsi { struct clk **clks; struct regulator_bulk_data supplies[2]; int irq; - struct gpio_desc *te_gpio; u32 pll_clk_rate; u32 burst_clk_rate; @@ -332,6 +337,7 @@ struct exynos_dsi { struct exynos_dsi_enc { struct drm_encoder encoder; + struct gpio_desc *te_gpio; }; #define host_to_dsi(host) container_of(host, struct exynos_dsi, dsi_host) @@ -1345,18 +1351,38 @@ static irqreturn_t exynos_dsi_te_irq_handler(int irq, void *dev_id) return IRQ_HANDLED; } -static void exynos_dsi_enable_irq(struct exynos_dsi *dsi) +static void _exynos_dsi_enable_irq(struct exynos_dsi *dsim) { - enable_irq(dsi->irq); + struct _exynos_dsi *dsi = dsim->priv; if (dsi->te_gpio) enable_irq(gpiod_to_irq(dsi->te_gpio)); } -static void exynos_dsi_disable_irq(struct exynos_dsi *dsi) +static void _exynos_dsi_disable_irq(struct exynos_dsi *dsim) { + struct _exynos_dsi *dsi = dsim->priv; + if (dsi->te_gpio) disable_irq(gpiod_to_irq(dsi->te_gpio)); +} + +static void exynos_dsi_enable_irq(struct exynos_dsi *dsi) +{ + const struct exynos_dsi_plat_data *pdata = dsi->plat_data; + + enable_irq(dsi->irq); + + if (pdata->irq_ops && pdata->irq_ops->enable) + pdata->irq_ops->enable(dsi); +} + +static void exynos_dsi_disable_irq(struct exynos_dsi *dsi) +{ + const struct exynos_dsi_plat_data *pdata = dsi->plat_data; + + if (pdata->irq_ops && pdata->irq_ops->disable) + pdata->irq_ops->disable(dsi); disable_irq(dsi->irq); } @@ -1385,9 +1411,10 @@ static int exynos_dsi_init(struct exynos_dsi *dsi) return 0; } -static int exynos_dsi_register_te_irq(struct exynos_dsi *dsi, +static int exynos_dsi_register_te_irq(struct exynos_dsi *dsim, struct device *panel) { + struct _exynos_dsi *dsi = dsim->priv; int ret; int te_gpio_irq; @@ -1395,7 +1422,7 @@ static int exynos_dsi_register_te_irq(struct exynos_dsi *dsi, if (!dsi->te_gpio) { return 0; } else if (IS_ERR(dsi->te_gpio)) { - dev_err(dsi->dev, "gpio request failed with %ld\n", + dev_err(dsim->dev, "gpio request failed with %ld\n", PTR_ERR(dsi->te_gpio)); return PTR_ERR(dsi->te_gpio); } @@ -1405,7 +1432,7 @@ static int exynos_dsi_register_te_irq(struct exynos_dsi *dsi, ret = request_threaded_irq(te_gpio_irq, exynos_dsi_te_irq_handler, NULL, IRQF_TRIGGER_RISING | IRQF_NO_AUTOEN, "TE", dsi); if (ret) { - dev_err(dsi->dev, "request interrupt failed with %d\n", ret); + dev_err(dsim->dev, "request interrupt failed with %d\n", ret); gpiod_put(dsi->te_gpio); return ret; } @@ -1413,8 +1440,10 @@ static int exynos_dsi_register_te_irq(struct exynos_dsi *dsi, return 0; } -static void exynos_dsi_unregister_te_irq(struct exynos_dsi *dsi) +static void exynos_dsi_unregister_te_irq(struct exynos_dsi *dsim) { + struct _exynos_dsi *dsi = dsim->priv; + if (dsi->te_gpio) { free_irq(gpiod_to_irq(dsi->te_gpio), dsi); gpiod_put(dsi->te_gpio); @@ -2035,6 +2064,11 @@ static const struct dev_pm_ops exynos_dsi_pm_ops = { pm_runtime_force_resume) }; +static const struct exynos_dsim_irq_ops exynos_dsi_irq_ops = { + .enable = _exynos_dsi_enable_irq, + .disable = _exynos_dsi_disable_irq, +}; + static const struct exynos_dsim_host_ops exynos_dsi_host_ops = { .register_host = exynos_dsi_register_host, .unregister_host = exynos_dsi_unregister_host, @@ -2045,26
[PATCH v10 11/18] drm: exynos: dsi: Add atomic_get_input_bus_fmts
Finding the right input bus format throughout the pipeline is hard so add atomic_get_input_bus_fmts callback and initialize with the proper input format from list of supported output formats. This format can be used in pipeline for negotiating bus format between the DSI-end of this bridge and the other component closer to pipeline components. List of Pixel formats are taken from, AN13573 i.MX 8/RT MIPI DSI/CSI-2, Rev. 0, 21 March 2022 3.7.4 Pixel formats Table 14. DSI pixel packing formats Tested-by: Marek Szyprowski Signed-off-by: Jagan Teki --- Changes for v10: - none Changes for v9: - added MEDIA_BUS_FMT_FIXED - return MEDIA_BUS_FMT_RGB888_1X24 output_fmt if supported output_fmt list is unsupported. - added MEDIA_BUS_FMT_YUYV10_1X20, MEDIA_BUS_FMT_YUYV12_1X24 Changes for v8: - added pixel formats supported by NXP AN13573 i.MX 8/RT MIPI DSI/CSI-2 Changes for v7 - v4: - none Changes for v3: - include media-bus-format.h Changes for v2: - none Changes for v1: - new patch drivers/gpu/drm/exynos/exynos_drm_dsi.c | 69 + 1 file changed, 69 insertions(+) diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c index 36820a7b5884..bb3d6a7fa84e 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c @@ -12,6 +12,7 @@ #include #include #include +#include #include #include #include @@ -1467,6 +1468,73 @@ static void exynos_dsi_atomic_post_disable(struct drm_bridge *bridge, pm_runtime_put_sync(dsi->dev); } +/* + * This pixel output formats list referenced from, + * AN13573 i.MX 8/RT MIPI DSI/CSI-2, Rev. 0, 21 March 2022 + * 3.7.4 Pixel formats + * Table 14. DSI pixel packing formats + */ +static const u32 exynos_dsi_pixel_output_fmts[] = { + MEDIA_BUS_FMT_YUYV10_1X20, + MEDIA_BUS_FMT_YUYV12_1X24, + MEDIA_BUS_FMT_UYVY8_1X16, + MEDIA_BUS_FMT_RGB101010_1X30, + MEDIA_BUS_FMT_RGB121212_1X36, + MEDIA_BUS_FMT_RGB565_1X16, + MEDIA_BUS_FMT_RGB666_1X18, + MEDIA_BUS_FMT_RGB888_1X24, + + MEDIA_BUS_FMT_FIXED, +}; + +static bool exynos_dsi_pixel_output_fmt_supported(u32 fmt) +{ + int i; + + for (i = 0; i < ARRAY_SIZE(exynos_dsi_pixel_output_fmts); i++) { + if (exynos_dsi_pixel_output_fmts[i] == fmt) + return true; + } + + return false; +} + +static u32 * +exynos_dsi_atomic_get_input_bus_fmts(struct drm_bridge *bridge, +struct drm_bridge_state *bridge_state, +struct drm_crtc_state *crtc_state, +struct drm_connector_state *conn_state, +u32 output_fmt, +unsigned int *num_input_fmts) +{ + u32 *input_fmts; + + if (!exynos_dsi_pixel_output_fmt_supported(output_fmt)) + /* +* Some bridge/display drivers are still not able to pass the +* correct format, so handle those pipelines by falling back +* to the default format till the supported formats finalized. +*/ + output_fmt = MEDIA_BUS_FMT_RGB888_1X24; + + input_fmts = kmalloc(sizeof(*input_fmts), GFP_KERNEL); + if (!input_fmts) + return NULL; + + switch (output_fmt) { + case MEDIA_BUS_FMT_FIXED: + input_fmts[0] = MEDIA_BUS_FMT_RGB888_1X24; + break; + default: + input_fmts[0] = output_fmt; + break; + } + + *num_input_fmts = 1; + + return input_fmts; +} + static int exynos_dsi_atomic_check(struct drm_bridge *bridge, struct drm_bridge_state *bridge_state, struct drm_crtc_state *crtc_state, @@ -1515,6 +1583,7 @@ static const struct drm_bridge_funcs exynos_dsi_bridge_funcs = { .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state, .atomic_reset = drm_atomic_helper_bridge_reset, + .atomic_get_input_bus_fmts = exynos_dsi_atomic_get_input_bus_fmts, .atomic_check = exynos_dsi_atomic_check, .atomic_pre_enable = exynos_dsi_atomic_pre_enable, .atomic_enable = exynos_dsi_atomic_enable, -- 2.25.1
[PATCH v10 09/18] drm: exynos: dsi: Add atomic check
Look like an explicit fixing up of mode_flags is required for DSIM IP present in i.MX8M Mini/Nano SoCs. At least the LCDIF + DSIM needs active low sync polarities in order to correlate the correct sync flags of the surrounding components in the chain to make sure the whole pipeline can work properly. On the other hand the i.MX 8M Mini Applications Processor Reference Manual, Rev. 3, 11/2020 says. "13.6.3.5.2 RGB interface Vsync, Hsync, and VDEN are active high signals." i.MX 8M Mini Applications Processor Reference Manual Rev. 3, 11/2020 3.6.3.5.2 RGB interface i.MX 8M Nano Applications Processor Reference Manual Rev. 2, 07/2022 13.6.2.7.2 RGB interface both claim "Vsync, Hsync, and VDEN are active high signals.", the LCDIF must generate inverted HS/VS/DE signals, i.e. active LOW. No clear evidence about whether it can be documentation issues or something, so added a comment FIXME for this and updated the active low sync polarities using SAMSUNG_DSIM_TYPE_IMX8MM hw_type. Comments are suggested by Marek Vasut. Signed-off-by: Jagan Teki --- Changes for v10, v9: - none Changes for v8: - update the comments about sync signals polarities - added clear commit message by including i.MX8M Nano details Changes for v7: - fix the hw_type checking logic Changes for v6: - none Changes for v5: - rebase based new bridge changes [mszyprow] - remove DSIM_QUIRK_FIXUP_SYNC_POL - add hw_type check for sync polarities change. Changes for v4: - none Changes for v3: - add DSIM_QUIRK_FIXUP_SYNC_POL to handle mode_flasg fixup Changes for v2: - none Changes for v1: - fix mode flags in atomic_check instead of mode_fixup drivers/gpu/drm/exynos/exynos_drm_dsi.c | 28 + 1 file changed, 28 insertions(+) diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c index 5792f9883cee..12a40576f937 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c @@ -263,6 +263,7 @@ enum exynos_dsi_type { DSIM_TYPE_EXYNOS5410, DSIM_TYPE_EXYNOS5422, DSIM_TYPE_EXYNOS5433, + DSIM_TYPE_IMX8MM, DSIM_TYPE_COUNT, }; @@ -1466,6 +1467,32 @@ static void exynos_dsi_atomic_post_disable(struct drm_bridge *bridge, pm_runtime_put_sync(dsi->dev); } +static int exynos_dsi_atomic_check(struct drm_bridge *bridge, + struct drm_bridge_state *bridge_state, + struct drm_crtc_state *crtc_state, + struct drm_connector_state *conn_state) +{ + struct exynos_dsi *dsi = bridge_to_dsi(bridge); + struct drm_display_mode *adjusted_mode = _state->adjusted_mode; + + /* +* The i.MX8M Mini/Nano glue logic between LCDIF and DSIM +* inverts HS/VS/DE sync signals polarity, therefore, while +* i.MX 8M Mini Applications Processor Reference Manual Rev. 3, 11/2020 +* 13.6.3.5.2 RGB interface +* i.MX 8M Nano Applications Processor Reference Manual Rev. 2, 07/2022 +* 13.6.2.7.2 RGB interface +* both claim "Vsync, Hsync, and VDEN are active high signals.", the +* LCDIF must generate inverted HS/VS/DE signals, i.e. active LOW. +*/ + if (dsi->plat_data->hw_type == DSIM_TYPE_IMX8MM) { + adjusted_mode->flags |= (DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC); + adjusted_mode->flags &= ~(DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC); + } + + return 0; +} + static void exynos_dsi_mode_set(struct drm_bridge *bridge, const struct drm_display_mode *mode, const struct drm_display_mode *adjusted_mode) @@ -1488,6 +1515,7 @@ static const struct drm_bridge_funcs exynos_dsi_bridge_funcs = { .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state, .atomic_reset = drm_atomic_helper_bridge_reset, + .atomic_check = exynos_dsi_atomic_check, .atomic_pre_enable = exynos_dsi_atomic_pre_enable, .atomic_enable = exynos_dsi_atomic_enable, .atomic_disable = exynos_dsi_atomic_disable, -- 2.25.1
[PATCH v10 12/18] drm: exynos: dsi: Consolidate component and bridge
DSI host registration, attach and detach operations are quite different for the component and bridge-based DRM drivers. Supporting generic bridge driver to use both component and bridge based DRM drivers can be tricky and would require additional host related operation hooks. Add host operation hooks for registering and unregistering Exynos and generic drivers, where Exynos hooks are used in existing Exynos component based DRM drivers and generic hooks are used in i.MX8M bridge based DRM drivers. Add host attach and detach operation hooks for Exynos component DRM drivers and those get invoked while DSI core host attach and detach gets called. Signed-off-by: Marek Szyprowski Signed-off-by: Jagan Teki --- Changes for v10: - split from previous series patch "drm: bridge: Generalize Exynos-DSI driver into a Samsung DSIM bridge" drivers/gpu/drm/exynos/exynos_drm_dsi.c | 179 ++-- 1 file changed, 140 insertions(+), 39 deletions(-) diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c index bb3d6a7fa84e..819131a36b96 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c @@ -250,6 +250,8 @@ struct exynos_dsi_transfer { u16 rx_done; }; +struct exynos_dsi; + #define DSIM_STATE_ENABLED BIT(0) #define DSIM_STATE_INITIALIZED BIT(1) #define DSIM_STATE_CMD_LPM BIT(2) @@ -282,12 +284,19 @@ struct exynos_dsi_driver_data { const unsigned int *reg_values; }; +struct exynos_dsim_host_ops { + int (*register_host)(struct exynos_dsi *dsim); + void (*unregister_host)(struct exynos_dsi *dsim); + int (*attach)(struct exynos_dsi *dsim, struct mipi_dsi_device *device); + int (*detach)(struct exynos_dsi *dsim, struct mipi_dsi_device *device); +}; + struct exynos_dsi_plat_data { enum exynos_dsi_type hw_type; + const struct exynos_dsim_host_ops *host_ops; }; struct exynos_dsi { - struct drm_encoder encoder; struct mipi_dsi_host dsi_host; struct drm_bridge bridge; struct drm_bridge *out_bridge; @@ -317,6 +326,12 @@ struct exynos_dsi { const struct exynos_dsi_driver_data *driver_data; const struct exynos_dsi_plat_data *plat_data; + + void *priv; +}; + +struct exynos_dsi_enc { + struct drm_encoder encoder; }; #define host_to_dsi(host) container_of(host, struct exynos_dsi, dsi_host) @@ -1320,10 +1335,11 @@ static irqreturn_t exynos_dsi_irq(int irq, void *dev_id) static irqreturn_t exynos_dsi_te_irq_handler(int irq, void *dev_id) { - struct exynos_dsi *dsi = (struct exynos_dsi *)dev_id; + struct exynos_dsi *dsim = (struct exynos_dsi *)dev_id; + struct exynos_dsi_enc *dsi = dsim->priv; struct drm_encoder *encoder = >encoder; - if (dsi->state & DSIM_STATE_VIDOUT_AVAILABLE) + if (dsim->state & DSIM_STATE_VIDOUT_AVAILABLE) exynos_drm_crtc_te_handler(encoder->crtc); return IRQ_HANDLED; @@ -1597,9 +1613,8 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host, struct mipi_dsi_device *device) { struct exynos_dsi *dsi = host_to_dsi(host); + const struct exynos_dsi_plat_data *pdata = dsi->plat_data; struct device *dev = dsi->dev; - struct drm_encoder *encoder = >encoder; - struct drm_device *drm = encoder->dev; int ret; dsi->out_bridge = devm_drm_of_dsi_get_bridge(dev, dev->of_node, 1, 0); @@ -1613,35 +1628,15 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host, drm_bridge_add(>bridge); - drm_bridge_attach(encoder, >bridge, - list_first_entry_or_null(>bridge_chain, - struct drm_bridge, - chain_node), 0); - - /* -* This is a temporary solution and should be made by more generic way. -* -* If attached panel device is for command mode one, dsi should register -* TE interrupt handler. -*/ - if (!(device->mode_flags & MIPI_DSI_MODE_VIDEO)) { - ret = exynos_dsi_register_te_irq(dsi, >dev); - if (ret) + if (pdata->host_ops && pdata->host_ops->attach) { + ret = pdata->host_ops->attach(dsi, device); + if (ret < 0) return ret; } - mutex_lock(>mode_config.mutex); - dsi->lanes = device->lanes; dsi->format = device->format; dsi->mode_flags = device->mode_flags; - exynos_drm_crtc_get_by_type(drm, EXYNOS_DISPLAY_TYPE_LCD)->i80_mode = - !(dsi->mode_flags & MIPI_DSI_MODE_VIDEO); - - mutex_unlock(>mode_config.mutex); - - if (drm->mode_config.poll_enabled) - drm_kms_helper_hotplug_event(drm); return 0; } @@ -1650,12 +1645,14 @@ static int
[PATCH v10 10/18] drm: exynos: dsi: Add input_bus_flags
LCDIF-DSIM glue logic inverts the HS/VS/DE signals and expecting the i.MX8M Mini/Nano DSI host to add additional Data Enable signal active low (DE_LOW). This makes the valid data transfer on each horizontal line. So, add additional bus flags DE_LOW setting via input_bus_flags for i.MX8M Mini/Nano platforms. Suggested-by: Marek Vasut Signed-off-by: Jagan Teki --- Changes for v10, v9: - none Changes for v8: - add DE_LOW for i.MX8M Mini/Nano platforms. Changes for v7, v6: - none Changes for v5: - rebased based on updated bridge changes Changes for v4 - v1: - none drivers/gpu/drm/exynos/exynos_drm_dsi.c | 8 1 file changed, 8 insertions(+) diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c index 12a40576f937..36820a7b5884 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c @@ -1692,6 +1692,10 @@ static const struct component_ops exynos_dsi_component_ops = { .unbind = exynos_dsi_unbind, }; +static const struct drm_bridge_timings dsim_bridge_timings_de_low = { + .input_bus_flags = DRM_BUS_FLAG_DE_LOW, +}; + static int exynos_dsi_probe(struct platform_device *pdev) { struct device *dev = >dev; @@ -1778,6 +1782,10 @@ static int exynos_dsi_probe(struct platform_device *pdev) dsi->bridge.type = DRM_MODE_CONNECTOR_DSI; dsi->bridge.pre_enable_prev_first = true; + /* DE_LOW: i.MX8M Mini/Nano LCDIF-DSIM glue logic inverts HS/VS/DE */ + if (dsi->plat_data->hw_type == DSIM_TYPE_IMX8MM) + dsi->bridge.timings = _bridge_timings_de_low; + ret = component_add(dev, _dsi_component_ops); if (ret) goto err_disable_runtime; -- 2.25.1
[PATCH v10 08/18] drm: exynos: dsi: Handle proper host initialization
From: Marek Szyprowski Host transfer() in the DSI master will invoke only when the DSI commands are sent from DSI devices like DSI Panel or DSI bridges and this host the transfer wouldn't invoke for I2C-based-DSI bridge drivers. Handling DSI host initialization in transfer calls misses the controller setup for I2C configured DSI bridges. This patch updates the DSI host initialization by calling host to init from bridge pre_enable as the bridge pre_enable API is invoked by core as it is common across all classes of DSI device drivers. The host init during pre_enable is conditional and not invoked for Exynos as existing downstream drm panels and bridges in Exynos are expecting the host initialization during DSI transfer. Signed-off-by: Jagan Teki Signed-off-by: Marek Szyprowski --- Changes for v10: - update the to simple logic to handle all platforms Changs for v9 - v8: - none Changes for v2: - check initialized state in samsung_dsim_init Changes for v1: - keep DSI init in host transfer drivers/gpu/drm/exynos/exynos_drm_dsi.c | 27 +++-- 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c index fdaf514b39f2..5792f9883cee 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c @@ -254,6 +254,9 @@ struct exynos_dsi_transfer { #define DSIM_STATE_CMD_LPM BIT(2) #define DSIM_STATE_VIDOUT_AVAILABLEBIT(3) +#define exynos_dsi_hw_is_exynos(hw) \ + ((hw) >= DSIM_TYPE_EXYNOS3250 && (hw) <= DSIM_TYPE_EXYNOS5433) + enum exynos_dsi_type { DSIM_TYPE_EXYNOS3250, DSIM_TYPE_EXYNOS4210, @@ -1344,6 +1347,9 @@ static int exynos_dsi_init(struct exynos_dsi *dsi) { const struct exynos_dsi_driver_data *driver_data = dsi->driver_data; + if (dsi->state & DSIM_STATE_INITIALIZED) + return 0; + exynos_dsi_reset(dsi); exynos_dsi_enable_irq(dsi); @@ -1356,6 +1362,8 @@ static int exynos_dsi_init(struct exynos_dsi *dsi) exynos_dsi_set_phy_ctrl(dsi); exynos_dsi_init_link(dsi); + dsi->state |= DSIM_STATE_INITIALIZED; + return 0; } @@ -1411,6 +1419,16 @@ static void exynos_dsi_atomic_pre_enable(struct drm_bridge *bridge, } dsi->state |= DSIM_STATE_ENABLED; + + /* +* For Exynos-DSIM the downstream bridge, or panel are expecting +* the host initialization during DSI transfer. +*/ + if (!exynos_dsi_hw_is_exynos(dsi->plat_data->hw_type)) { + ret = exynos_dsi_init(dsi); + if (ret) + return; + } } static void exynos_dsi_atomic_enable(struct drm_bridge *bridge, @@ -1557,12 +1575,9 @@ static ssize_t exynos_dsi_host_transfer(struct mipi_dsi_host *host, if (!(dsi->state & DSIM_STATE_ENABLED)) return -EINVAL; - if (!(dsi->state & DSIM_STATE_INITIALIZED)) { - ret = exynos_dsi_init(dsi); - if (ret) - return ret; - dsi->state |= DSIM_STATE_INITIALIZED; - } + ret = exynos_dsi_init(dsi); + if (ret) + return ret; ret = mipi_dsi_create_packet(, msg); if (ret < 0) -- 2.25.1
[PATCH v10 06/18] drm: exynos: dsi: Add platform PLL_P (PMS_P) offset
Look like PLL PMS_P offset value varies between platforms that have Samsung DSIM IP. However, there is no clear evidence for it as both Exynos and i.MX 8M Mini Application Processor Reference Manual is still referring the PMS_P offset as 13. The offset 13 is not working for i.MX8M Mini SoCs but the downstream NXP sec-dsim.c driver is using offset 14 for i.MX8M Mini SoC platforms [1] [2]. PMS_P value set in sec_mipi_dsim_check_pll_out using PLLCTRL_SET_P() with offset 13 and then an additional offset of one bit added in sec_mipi_dsim_config_pll via PLLCTRL_SET_PMS(). Not sure whether it is reference manual documentation or something else but this patch trusts the downstream code and handle PLL_P offset via platform driver data so-that imx8mm driver data shall use pll_p_offset to 14. Similar to Mini the i.MX8M Nano/Plus also has P=14, unlike Exynos. [1] https://source.codeaurora.org/external/imx/linux-imx/tree/drivers/gpu/drm/bridge/sec-dsim.c?h=imx_5.4.47_2.2.0#n210 [2] https://source.codeaurora.org/external/imx/linux-imx/tree/drivers/gpu/drm/bridge/sec-dsim.c?h=imx_5.4.47_2.2.0#n211 Reviewed-by: Marek Vasut Signed-off-by: Frieder Schrempf Signed-off-by: Jagan Teki --- Changes for v10, v9: - none Changes for v8: - updated commit message for 8M Nano/Plus Changes for v7, v6: - none Changes for v5: - updated clear commit message Changes for v4, v3, v2: - none Changes for v1: - updated commit message - add downstream driver link drivers/gpu/drm/exynos/exynos_drm_dsi.c | 11 +-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c index 5918d31127aa..7a845badb1b2 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c @@ -194,7 +194,7 @@ /* DSIM_PLLCTRL */ #define DSIM_FREQ_BAND(x) ((x) << 24) #define DSIM_PLL_EN(1 << 23) -#define DSIM_PLL_P(x) ((x) << 13) +#define DSIM_PLL_P(x, offset) ((x) << (offset)) #define DSIM_PLL_M(x) ((x) << 4) #define DSIM_PLL_S(x) ((x) << 1) @@ -263,6 +263,7 @@ struct exynos_dsi_driver_data { unsigned int max_freq; unsigned int wait_for_reset; unsigned int num_bits_resol; + unsigned int pll_p_offset; const unsigned int *reg_values; }; @@ -471,6 +472,7 @@ static const struct exynos_dsi_driver_data exynos3_dsi_driver_data = { .max_freq = 1000, .wait_for_reset = 1, .num_bits_resol = 11, + .pll_p_offset = 13, .reg_values = reg_values, }; @@ -483,6 +485,7 @@ static const struct exynos_dsi_driver_data exynos4_dsi_driver_data = { .max_freq = 1000, .wait_for_reset = 1, .num_bits_resol = 11, + .pll_p_offset = 13, .reg_values = reg_values, }; @@ -493,6 +496,7 @@ static const struct exynos_dsi_driver_data exynos5_dsi_driver_data = { .max_freq = 1000, .wait_for_reset = 1, .num_bits_resol = 11, + .pll_p_offset = 13, .reg_values = reg_values, }; @@ -504,6 +508,7 @@ static const struct exynos_dsi_driver_data exynos5433_dsi_driver_data = { .max_freq = 1500, .wait_for_reset = 0, .num_bits_resol = 12, + .pll_p_offset = 13, .reg_values = exynos5433_reg_values, }; @@ -515,6 +520,7 @@ static const struct exynos_dsi_driver_data exynos5422_dsi_driver_data = { .max_freq = 1500, .wait_for_reset = 1, .num_bits_resol = 12, + .pll_p_offset = 13, .reg_values = exynos5422_reg_values, }; @@ -628,7 +634,8 @@ static unsigned long exynos_dsi_set_pll(struct exynos_dsi *dsi, writel(driver_data->reg_values[PLL_TIMER], dsi->reg_base + driver_data->plltmr_reg); - reg = DSIM_PLL_EN | DSIM_PLL_P(p) | DSIM_PLL_M(m) | DSIM_PLL_S(s); + reg = DSIM_PLL_EN | DSIM_PLL_P(p, driver_data->pll_p_offset) | + DSIM_PLL_M(m) | DSIM_PLL_S(s); if (driver_data->has_freqband) { static const unsigned long freq_bands[] = { -- 2.25.1
[PATCH v10 07/18] drm: exynos: dsi: Introduce hw_type platform data
Samsung MIPI DSIM controller is common DSI IP that can be used in various SoCs like Exynos, i.MX8M Mini/Nano/Plus. Add hw_type enum via platform_data so that accessing the different controller data between various platforms becomes easy and meaningful. Suggested-by: Marek Szyprowski Signed-off-by: Jagan Teki --- Changes for v10: - split from previous series patch "drm: bridge: Generalize Exynos-DSI driver into a Samsung DSIM bridge" - update enum type names drivers/gpu/drm/exynos/exynos_drm_dsi.c | 84 - 1 file changed, 69 insertions(+), 15 deletions(-) diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c index 7a845badb1b2..fdaf514b39f2 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c @@ -254,6 +254,16 @@ struct exynos_dsi_transfer { #define DSIM_STATE_CMD_LPM BIT(2) #define DSIM_STATE_VIDOUT_AVAILABLEBIT(3) +enum exynos_dsi_type { + DSIM_TYPE_EXYNOS3250, + DSIM_TYPE_EXYNOS4210, + DSIM_TYPE_EXYNOS5410, + DSIM_TYPE_EXYNOS5422, + DSIM_TYPE_EXYNOS5433, + + DSIM_TYPE_COUNT, +}; + struct exynos_dsi_driver_data { const unsigned int *reg_ofs; unsigned int plltmr_reg; @@ -267,6 +277,10 @@ struct exynos_dsi_driver_data { const unsigned int *reg_values; }; +struct exynos_dsi_plat_data { + enum exynos_dsi_type hw_type; +}; + struct exynos_dsi { struct drm_encoder encoder; struct mipi_dsi_host dsi_host; @@ -297,6 +311,7 @@ struct exynos_dsi { struct list_head transfer_list; const struct exynos_dsi_driver_data *driver_data; + const struct exynos_dsi_plat_data *plat_data; }; #define host_to_dsi(host) container_of(host, struct exynos_dsi, dsi_host) @@ -524,18 +539,13 @@ static const struct exynos_dsi_driver_data exynos5422_dsi_driver_data = { .reg_values = exynos5422_reg_values, }; -static const struct of_device_id exynos_dsi_of_match[] = { - { .compatible = "samsung,exynos3250-mipi-dsi", - .data = _dsi_driver_data }, - { .compatible = "samsung,exynos4210-mipi-dsi", - .data = _dsi_driver_data }, - { .compatible = "samsung,exynos5410-mipi-dsi", - .data = _dsi_driver_data }, - { .compatible = "samsung,exynos5422-mipi-dsi", - .data = _dsi_driver_data }, - { .compatible = "samsung,exynos5433-mipi-dsi", - .data = _dsi_driver_data }, - { } +static const struct exynos_dsi_driver_data * +exynos_dsi_types[DSIM_TYPE_COUNT] = { + [DSIM_TYPE_EXYNOS3250] = _dsi_driver_data, + [DSIM_TYPE_EXYNOS4210] = _dsi_driver_data, + [DSIM_TYPE_EXYNOS5410] = _dsi_driver_data, + [DSIM_TYPE_EXYNOS5422] = _dsi_driver_data, + [DSIM_TYPE_EXYNOS5433] = _dsi_driver_data, }; static void exynos_dsi_wait_for_reset(struct exynos_dsi *dsi) @@ -1468,8 +1478,6 @@ static const struct drm_bridge_funcs exynos_dsi_bridge_funcs = { .attach = exynos_dsi_attach, }; -MODULE_DEVICE_TABLE(of, exynos_dsi_of_match); - static int exynos_dsi_host_attach(struct mipi_dsi_host *host, struct mipi_dsi_device *device) { @@ -1659,7 +1667,8 @@ static int exynos_dsi_probe(struct platform_device *pdev) dsi->dsi_host.dev = dev; dsi->dev = dev; - dsi->driver_data = of_device_get_match_data(dev); + dsi->plat_data = of_device_get_match_data(dev); + dsi->driver_data = exynos_dsi_types[dsi->plat_data->hw_type]; dsi->supplies[0].supply = "vddcore"; dsi->supplies[1].supply = "vddio"; @@ -1817,6 +1826,51 @@ static const struct dev_pm_ops exynos_dsi_pm_ops = { pm_runtime_force_resume) }; +static const struct exynos_dsi_plat_data exynos3250_dsi_pdata = { + .hw_type = DSIM_TYPE_EXYNOS3250, +}; + +static const struct exynos_dsi_plat_data exynos4210_dsi_pdata = { + .hw_type = DSIM_TYPE_EXYNOS4210, +}; + +static const struct exynos_dsi_plat_data exynos5410_dsi_pdata = { + .hw_type = DSIM_TYPE_EXYNOS5410, +}; + +static const struct exynos_dsi_plat_data exynos5422_dsi_pdata = { + .hw_type = DSIM_TYPE_EXYNOS5422, +}; + +static const struct exynos_dsi_plat_data exynos5433_dsi_pdata = { + .hw_type = DSIM_TYPE_EXYNOS5433, +}; + +static const struct of_device_id exynos_dsi_of_match[] = { + { + .compatible = "samsung,exynos3250-mipi-dsi", + .data = _dsi_pdata, + }, + { + .compatible = "samsung,exynos4210-mipi-dsi", + .data = _dsi_pdata, + }, + { + .compatible = "samsung,exynos5410-mipi-dsi", + .data = _dsi_pdata, + }, + { + .compatible = "samsung,exynos5422-mipi-dsi", + .data = _dsi_pdata, + }, + { + .compatible = "samsung,exynos5433-mipi-dsi", + .data =
[PATCH v10 05/18] drm: exynos: dsi: Mark PHY as optional
The same Samsung MIPI DSIM master can also be used in NXP's i.MX8M Mini/Nano/Plus SoC. In i.MX8M Mini/Nano/Plus SoC the DSI Phy requires a MIPI DPHY bit to reset in order to activate the PHY and that can be done via upstream i.MX8M blk-ctrl driver. So, mark the phy get as optional. Reviewed-by: Marek Vasut Signed-off-by: Jagan Teki --- Changes for v10: - add Plus in commit message - collect Marek RB Changes for v9, v8, v7, v6, v5, v4, v3, v2: - none Changes for v1: - new patch drivers/gpu/drm/exynos/exynos_drm_dsi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c index 4a165764121d..5918d31127aa 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c @@ -1687,7 +1687,7 @@ static int exynos_dsi_probe(struct platform_device *pdev) if (IS_ERR(dsi->reg_base)) return PTR_ERR(dsi->reg_base); - dsi->phy = devm_phy_get(dev, "dsim"); + dsi->phy = devm_phy_optional_get(dev, "dsim"); if (IS_ERR(dsi->phy)) { dev_info(dev, "failed to get dsim phy\n"); return PTR_ERR(dsi->phy); -- 2.25.1
[PATCH v10 02/18] drm: bridge: panel: Add devm_drm_of_dsi_get_bridge helper
Add devm OF helper to return the next DSI bridge in the chain. Unlike general bridge return helper devm_drm_of_get_bridge, this helper uses the dsi specific panel_or_bridge helper to find the next DSI device in the pipeline. Helper lookup a given child DSI node or a DT node's port and endpoint number, find the connected node and return either the associated struct drm_panel or drm_bridge device. Cc: Maxime Ripard Cc: Laurent Pinchart Cc: Linus Walleij Cc: Maarten Lankhorst Signed-off-by: Jagan Teki --- Changes for v10: - new patch drivers/gpu/drm/bridge/panel.c | 34 ++ include/drm/drm_bridge.h | 2 ++ 2 files changed, 36 insertions(+) diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c index 03c3274dc3d9..50ea5de45197 100644 --- a/drivers/gpu/drm/bridge/panel.c +++ b/drivers/gpu/drm/bridge/panel.c @@ -496,4 +496,38 @@ struct drm_bridge *drmm_of_get_bridge(struct drm_device *drm, } EXPORT_SYMBOL(drmm_of_get_bridge); +/** + * devm_drm_of_dsi_get_bridge - Return next DSI bridge in the chain + * @dev: device to tie the bridge lifetime to + * @np: device tree node containing encoder output ports + * @port: port in the device tree node + * @endpoint: endpoint in the device tree node + * + * Lookup a given child DSI node or a DT node's port and endpoint number, + * find the connected node and return either the associated struct drm_panel + * or drm_bridge device. Either @panel or @bridge must not be NULL. + * + * Returns a pointer to the bridge if successful, or an error pointer + * otherwise. + */ +struct drm_bridge *devm_drm_of_dsi_get_bridge(struct device *dev, + struct device_node *np, + u32 port, u32 endpoint) +{ + struct drm_bridge *bridge; + struct drm_panel *panel; + int ret; + + ret = drm_of_dsi_find_panel_or_bridge(np, port, endpoint, + , ); + if (ret) + return ERR_PTR(ret); + + if (panel) + bridge = devm_drm_panel_bridge_add(dev, panel); + + return bridge; +} +EXPORT_SYMBOL(devm_drm_of_dsi_get_bridge); + #endif diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h index 42f86327b40a..ccb14e361d3f 100644 --- a/include/drm/drm_bridge.h +++ b/include/drm/drm_bridge.h @@ -931,6 +931,8 @@ struct drm_bridge *devm_drm_of_get_bridge(struct device *dev, struct device_node u32 port, u32 endpoint); struct drm_bridge *drmm_of_get_bridge(struct drm_device *drm, struct device_node *node, u32 port, u32 endpoint); +struct drm_bridge *devm_drm_of_dsi_get_bridge(struct device *dev, struct device_node *node, + u32 port, u32 endpoint); #else static inline struct drm_bridge *devm_drm_of_get_bridge(struct device *dev, struct device_node *node, -- 2.25.1
[PATCH v10 01/18] drm: of: Lookup if child node has DSI panel or bridge
Devices can also be child nodes when we also control that device through the upstream device (ie, MIPI-DCS for a MIPI-DSI device). Unlike the drm_of_find_panel_or_bridge helper it requires a special case to lookup a child node of the given parent that isn't either port or ports. Lookup for a child DSI node of the given parent that isn't either port or ports. If it is found then it will directly find the panel or bridge otherwise lookup for the child node with a given port and endpoint number as drm_of_find_panel_or_bridge does. Supporting this feature via existing drm_of_find_panel_or_bridge found several issues while handling usecases. Here is the previously failed attempt of similar and the same has been reverted later. commit <80253168dbfd> ("drm: of: Lookup if child node has panel or bridge") So, add a separate helper to handle this DSI use case. Example OF graph representation of DSI host, which has port but not has ports and has child panel node. dsi { compatible = "allwinner,sun6i-a31-mipi-dsi"; #address-cells = <1>; #size-cells = <0>; port { dsi_in_tcon0: endpoint { remote-endpoint = ; }; panel@0 { reg = <0>; }; }; Example OF graph representation of DSI host, which has ports but not has port and has child panel node. dsi { compatible = "samsung,exynos5433-mipi-dsi"; #address-cells = <1>; #size-cells = <0>; ports { #address-cells = <1>; #size-cells = <0>; port@0 { reg = <0>; dsi_to_mic: endpoint { remote-endpoint = <_to_dsi>; }; }; }; panel@0 { reg = <0>; }; }; Example OF graph representation of DSI host, which has neither a port nor a ports but has child panel node. dsi0 { compatible = "ste,mcde-dsi"; #address-cells = <1>; #size-cells = <0>; panel@0 { reg = <0>; }; }; Cc: Maxime Ripard Cc: Laurent Pinchart Cc: Linus Walleij Cc: Maarten Lankhorst Signed-off-by: Jagan Teki --- Changes for v10: - new patch drivers/gpu/drm/drm_of.c | 113 --- include/drm/drm_of.h | 12 + 2 files changed, 105 insertions(+), 20 deletions(-) diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c index 7bbcb999bb75..020457444dfe 100644 --- a/drivers/gpu/drm/drm_of.c +++ b/drivers/gpu/drm/drm_of.c @@ -216,6 +216,36 @@ int drm_of_encoder_active_endpoint(struct device_node *node, } EXPORT_SYMBOL_GPL(drm_of_encoder_active_endpoint); +static int of_drm_find_panel_or_bridge(struct device_node *remote, + struct drm_panel **panel, + struct drm_bridge **bridge) +{ + int ret = -EPROBE_DEFER; + + if (panel) { + *panel = of_drm_find_panel(remote); + if (!IS_ERR(*panel)) + ret = 0; + else + *panel = NULL; + } + + /* No panel found yet, check for a bridge next. */ + if (bridge) { + if (ret) { + *bridge = of_drm_find_bridge(remote); + if (*bridge) + ret = 0; + } else { + *bridge = NULL; + } + + } + + of_node_put(remote); + return ret; +} + /** * drm_of_find_panel_or_bridge - return connected panel or bridge device * @np: device tree node containing encoder output ports @@ -238,7 +268,6 @@ int drm_of_find_panel_or_bridge(const struct device_node *np, struct drm_panel **panel, struct drm_bridge **bridge) { - int ret = -EPROBE_DEFER; struct device_node *remote; if (!panel && !bridge) @@ -259,30 +288,74 @@ int drm_of_find_panel_or_bridge(const struct device_node *np, if (!remote) return -ENODEV; - if (panel) { - *panel = of_drm_find_panel(remote); - if (!IS_ERR(*panel)) - ret = 0; - else - *panel = NULL; - } + return of_drm_find_panel_or_bridge(remote, panel, bridge); +} +EXPORT_SYMBOL_GPL(drm_of_find_panel_or_bridge); - /* No panel found yet, check for a bridge next. */ - if (bridge) { - if (ret) { - *bridge = of_drm_find_bridge(remote); - if (*bridge) - ret = 0; - } else { - *bridge = NULL; - } +/** + * drm_of_dsi_find_panel_or_bridge - return connected DSI panel or bridge device + * @np: device tree node containing encoder output ports + *
[PATCH v10 03/18] drm: exynos: dsi: Drop explicit call to bridge detach
Exynos DSI already converted into a bridge driver, so bridge detach will suppose happened during bridge chain removal done by the bridge core. Drop the explicit call chain to detach the bridge. Signed-off-by: Jagan Teki --- Changes for v10: - new patch drivers/gpu/drm/exynos/exynos_drm_dsi.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c index 06d6513ddaae..df15501b1075 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c @@ -1531,8 +1531,6 @@ static int exynos_dsi_host_detach(struct mipi_dsi_host *host, struct exynos_dsi *dsi = host_to_dsi(host); struct drm_device *drm = dsi->encoder.dev; - if (dsi->out_bridge->funcs->detach) - dsi->out_bridge->funcs->detach(dsi->out_bridge); dsi->out_bridge = NULL; if (drm->mode_config.poll_enabled) -- 2.25.1
[PATCH v10 04/18] drm: exynos: dsi: Switch to devm_drm_of_dsi_get_bridge
devm_drm_of_dsi_get_bridge is capable of looking up the downstream DSI bridge and panel and trying to add a panel bridge if the panel is found. Replace explicit finding calls with devm_drm_of_dsi_get_bridge. Signed-off-by: Jagan Teki --- Changes for v10: - new patch drivers/gpu/drm/exynos/exynos_drm_dsi.c | 13 + 1 file changed, 1 insertion(+), 12 deletions(-) diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c index df15501b1075..4a165764121d 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c @@ -1470,18 +1470,9 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host, struct device *dev = dsi->dev; struct drm_encoder *encoder = >encoder; struct drm_device *drm = encoder->dev; - struct drm_panel *panel; int ret; - panel = of_drm_find_panel(device->dev.of_node); - if (!IS_ERR(panel)) { - dsi->out_bridge = devm_drm_panel_bridge_add(dev, panel); - } else { - dsi->out_bridge = of_drm_find_bridge(device->dev.of_node); - if (!dsi->out_bridge) - dsi->out_bridge = ERR_PTR(-EINVAL); - } - + dsi->out_bridge = devm_drm_of_dsi_get_bridge(dev, dev->of_node, 1, 0); if (IS_ERR(dsi->out_bridge)) { ret = PTR_ERR(dsi->out_bridge); DRM_DEV_ERROR(dev, "failed to find the bridge: %d\n", ret); @@ -1531,8 +1522,6 @@ static int exynos_dsi_host_detach(struct mipi_dsi_host *host, struct exynos_dsi *dsi = host_to_dsi(host); struct drm_device *drm = dsi->encoder.dev; - dsi->out_bridge = NULL; - if (drm->mode_config.poll_enabled) drm_kms_helper_hotplug_event(drm); -- 2.25.1
[PATCH 07/10] drm: bridge: it66121: Don't clear DDC FIFO twice
The DDC FIFO was cleared before the loop in it66121_get_edid_block(), and at the beginning of each iteration; which means that it did not have to be cleared before the loop. Signed-off-by: Paul Cercueil --- drivers/gpu/drm/bridge/ite-it66121.c | 16 1 file changed, 16 deletions(-) diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c index 06fa59ae5ffc..5335d4abd7c5 100644 --- a/drivers/gpu/drm/bridge/ite-it66121.c +++ b/drivers/gpu/drm/bridge/ite-it66121.c @@ -456,18 +456,6 @@ static inline int it66121_wait_ddc_ready(struct it66121_ctx *ctx) return 0; } -static int it66121_clear_ddc_fifo(struct it66121_ctx *ctx) -{ - int ret; - - ret = it66121_preamble_ddc(ctx); - if (ret) - return ret; - - return regmap_write(ctx->regmap, IT66121_DDC_COMMAND_REG, - IT66121_DDC_COMMAND_FIFO_CLR); -} - static int it66121_abort_ddc_ops(struct it66121_ctx *ctx) { int ret; @@ -515,10 +503,6 @@ static int it66121_get_edid_block(void *context, u8 *buf, offset = (block % 2) * len; block = block / 2; - ret = it66121_clear_ddc_fifo(ctx); - if (ret) - return ret; - while (remain > 0) { cnt = (remain > IT66121_EDID_FIFO_SIZE) ? IT66121_EDID_FIFO_SIZE : remain; -- 2.35.1
[PATCH 08/10] drm: bridge: it66121: Set DDC preamble only once before reading EDID
The DDC preamble and target address only need to be set once before reading the EDID, even if multiple segments have to be read. Signed-off-by: Paul Cercueil --- drivers/gpu/drm/bridge/ite-it66121.c | 28 1 file changed, 16 insertions(+), 12 deletions(-) diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c index 5335d4abd7c5..7972003d4776 100644 --- a/drivers/gpu/drm/bridge/ite-it66121.c +++ b/drivers/gpu/drm/bridge/ite-it66121.c @@ -506,9 +506,6 @@ static int it66121_get_edid_block(void *context, u8 *buf, while (remain > 0) { cnt = (remain > IT66121_EDID_FIFO_SIZE) ? IT66121_EDID_FIFO_SIZE : remain; - ret = it66121_preamble_ddc(ctx); - if (ret) - return ret; ret = regmap_write(ctx->regmap, IT66121_DDC_COMMAND_REG, IT66121_DDC_COMMAND_FIFO_CLR); @@ -519,15 +516,6 @@ static int it66121_get_edid_block(void *context, u8 *buf, if (ret) return ret; - ret = it66121_preamble_ddc(ctx); - if (ret) - return ret; - - ret = regmap_write(ctx->regmap, IT66121_DDC_HEADER_REG, - IT66121_DDC_HEADER_EDID); - if (ret) - return ret; - ret = regmap_write(ctx->regmap, IT66121_DDC_OFFSET_REG, offset); if (ret) return ret; @@ -842,9 +830,25 @@ static struct edid *it66121_bridge_get_edid(struct drm_bridge *bridge, { struct it66121_ctx *ctx = container_of(bridge, struct it66121_ctx, bridge); struct edid *edid; + int ret; mutex_lock(>lock); + ret = it66121_preamble_ddc(ctx); + if (ret) { + edid = ERR_PTR(ret); + goto out_unlock; + } + + ret = regmap_write(ctx->regmap, IT66121_DDC_HEADER_REG, + IT66121_DDC_HEADER_EDID); + if (ret) { + edid = ERR_PTR(ret); + goto out_unlock; + } + edid = drm_do_get_edid(connector, it66121_get_edid_block, ctx); + +out_unlock: mutex_unlock(>lock); return edid; -- 2.35.1
[PATCH 06/10] drm: bridge: it66121: Don't use DDC error IRQs
The DDC error IRQs will fire on the IT6610 every time the FIFO is empty, which is not very helpful. To resolve this, we can simply disable them, and handle DDC errors in it66121_wait_ddc_ready(). Signed-off-by: Paul Cercueil --- drivers/gpu/drm/bridge/ite-it66121.c | 49 ++-- 1 file changed, 10 insertions(+), 39 deletions(-) diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c index bfb9c87a7019..06fa59ae5ffc 100644 --- a/drivers/gpu/drm/bridge/ite-it66121.c +++ b/drivers/gpu/drm/bridge/ite-it66121.c @@ -515,16 +515,6 @@ static int it66121_get_edid_block(void *context, u8 *buf, offset = (block % 2) * len; block = block / 2; - ret = regmap_read(ctx->regmap, IT66121_INT_STATUS1_REG, ); - if (ret) - return ret; - - if (val & IT66121_INT_STATUS1_DDC_BUSHANG) { - ret = it66121_abort_ddc_ops(ctx); - if (ret) - return ret; - } - ret = it66121_clear_ddc_fifo(ctx); if (ret) return ret; @@ -545,16 +535,6 @@ static int it66121_get_edid_block(void *context, u8 *buf, if (ret) return ret; - ret = regmap_read(ctx->regmap, IT66121_INT_STATUS1_REG, ); - if (ret) - return ret; - - if (val & IT66121_INT_STATUS1_DDC_BUSHANG) { - ret = it66121_abort_ddc_ops(ctx); - if (ret) - return ret; - } - ret = it66121_preamble_ddc(ctx); if (ret) return ret; @@ -585,8 +565,10 @@ static int it66121_get_edid_block(void *context, u8 *buf, remain -= cnt; ret = it66121_wait_ddc_ready(ctx); - if (ret) + if (ret) { + it66121_abort_ddc_ops(ctx); return ret; + } ret = regmap_noinc_read(ctx->regmap, IT66121_DDC_RD_FIFO_REG, buf, cnt); @@ -671,11 +653,7 @@ static int it66121_bridge_attach(struct drm_bridge *bridge, /* Per programming manual, sleep here for bridge to settle */ msleep(50); - /* Start interrupts */ - return regmap_write_bits(ctx->regmap, IT66121_INT_MASK1_REG, -IT66121_INT_MASK1_DDC_NOACK | -IT66121_INT_MASK1_DDC_FIFOERR | -IT66121_INT_MASK1_DDC_BUSHANG, 0); + return 0; } static int it66121_set_mute(struct it66121_ctx *ctx, bool mute) @@ -926,21 +904,14 @@ static irqreturn_t it66121_irq_threaded_handler(int irq, void *dev_id) ret = regmap_read(ctx->regmap, IT66121_INT_STATUS1_REG, ); if (ret) { dev_err(dev, "Cannot read STATUS1_REG %d\n", ret); - } else { - if (val & IT66121_INT_STATUS1_DDC_FIFOERR) - it66121_clear_ddc_fifo(ctx); - if (val & (IT66121_INT_STATUS1_DDC_BUSHANG | - IT66121_INT_STATUS1_DDC_NOACK)) - it66121_abort_ddc_ops(ctx); - if (val & IT66121_INT_STATUS1_HPD_STATUS) { - regmap_write_bits(ctx->regmap, IT66121_INT_CLR1_REG, - IT66121_INT_CLR1_HPD, IT66121_INT_CLR1_HPD); + } else if (val & IT66121_INT_STATUS1_HPD_STATUS) { + regmap_write_bits(ctx->regmap, IT66121_INT_CLR1_REG, + IT66121_INT_CLR1_HPD, IT66121_INT_CLR1_HPD); - status = it66121_is_hpd_detect(ctx) ? connector_status_connected - : connector_status_disconnected; + status = it66121_is_hpd_detect(ctx) ? connector_status_connected + : connector_status_disconnected; - event = true; - } + event = true; } regmap_write_bits(ctx->regmap, IT66121_SYS_STATUS_REG, -- 2.35.1
[PATCH v10 00/18] drm: Add Samsung MIPI DSIM bridge
This series supports common bridge support for Samsung MIPI DSIM which is used in Exynos and i.MX8MM SoC's. The final bridge supports both the Exynos and i.MX8M Mini/Nano/Plus. Patch 0001 - 0004: adding devm_drm_of_dsi_get_bridge Patch 0005 - 0006: optional PHY, PMS_P offset Patch 0007 : introduce hw_type Patch 0008 : fixing host init Patch 0009 : atomic_check Patch 0010 : input_bus_flags Patch 0011 : atomic_get_input_bus_fmts Patch 0012 - 0013: component vs bridge Patch 0014 : DSIM bridge Patch 0015 - 0016: i.MX8M Mini/Nano Patch 0017 - 0018: i.MX8M Plus Changes for v10: - rebase on drm-misc-next - add drm_of_dsi_find_panel_or_bridge - add devm_drm_of_dsi_get_bridge - fix host initialization (Thanks to Marek Szyprowski) - rearrange the tiny patches for easy to review - update simple names for enum hw_type - add is_hw_exynos macro - rework on commit messages Changes for v9: - rebase on drm-misc-next - drop drm bridge attach fix for Exynos - added prepare_prev_first flag - added pre_enable_prev_first flag - fix bridge chain order for exynos - added fix for Exynos host init for first DSI transfer - added MEDIA_BUS_FMT_FIXED - return MEDIA_BUS_FMT_RGB888_1X24 output_fmt if supported output_fmt list is unsupported. - added MEDIA_BUS_FMT_YUYV10_1X20 - added MEDIA_BUS_FMT_YUYV12_1X24 Changes for v8: * fixed comment lines * fixed commit messages * fixed video mode bits * collect Marek Ack * fixed video mode bit names * update input formats logic * added imx8mplus support Changes for v7: * fix the drm bridge attach chain for exynos drm dsi driver * fix the hw_type checking logic Changes for v6: * handle previous bridge for exynos dsi while attaching bridge Changes for v5: * bridge changes to support multi-arch * updated and clear commit messages * add hw_type via plat data * removed unneeded quirk * rebased on linux-next Changes for v4: * include Inki Dae in MAINTAINERS * remove dsi_driver probe in exynos_drm_drv to support multi-arch build * update init handling to ensure host init done on first cmd transfer Changes for v3: * fix the mult-arch build * fix dsi host init * updated commit messages Changes for v2: * fix bridge handling * fix dsi host init * correct the commit messages Tested in Engicam i.Core MX8M Mini SoM. Repo: https://gitlab.com/openedev/kernel/-/commits/imx8mm-dsi-v10 v9: https://lore.kernel.org/all/20221209152343.180139-1-ja...@amarulasolutions.com/ Any inputs? Jagan. Jagan Teki (16): drm: of: Lookup if child node has DSI panel or bridge drm: bridge: panel: Add devm_drm_of_dsi_get_bridge helper drm: exynos: dsi: Drop explicit call to bridge detach drm: exynos: dsi: Switch to devm_drm_of_dsi_get_bridge drm: exynos: dsi: Mark PHY as optional drm: exynos: dsi: Add platform PLL_P (PMS_P) offset drm: exynos: dsi: Introduce hw_type platform data drm: exynos: dsi: Add atomic check drm: exynos: dsi: Add input_bus_flags drm: exynos: dsi: Add atomic_get_input_bus_fmts drm: exynos: dsi: Consolidate component and bridge drm: exynos: dsi: Add Exynos based host irq hooks drm: bridge: Generalize Exynos-DSI driver into a Samsung DSIM bridge dt-bindings: display: exynos: dsim: Add NXP i.MX8M Mini/Nano support drm: bridge: samsung-dsim: Add i.MX8M Mini/Nano support dt-bindings: display: exynos: dsim: Add NXP i.MX8M Plus support Marek Szyprowski (1): drm: exynos: dsi: Handle proper host initialization Marek Vasut (1): drm: bridge: samsung-dsim: Add i.MX8M Plus support .../bindings/display/exynos/exynos_dsim.txt |2 + MAINTAINERS |9 + drivers/gpu/drm/bridge/Kconfig| 12 + drivers/gpu/drm/bridge/Makefile |1 + drivers/gpu/drm/bridge/panel.c| 34 + drivers/gpu/drm/bridge/samsung-dsim.c | 1883 + drivers/gpu/drm/drm_of.c | 113 +- drivers/gpu/drm/exynos/Kconfig|1 + drivers/gpu/drm/exynos/exynos_drm_dsi.c | 1793 +--- include/drm/bridge/samsung-dsim.h | 119 ++ include/drm/drm_bridge.h |2 + include/drm/drm_of.h | 12 + 12 files changed, 2285 insertions(+), 1696 deletions(-) create mode 100644 drivers/gpu/drm/bridge/samsung-dsim.c create mode 100644 include/drm/bridge/samsung-dsim.h -- 2.25.1
[PATCH 05/10] drm: bridge: it66121: Fix wait for DDC ready
The function it66121_wait_ddc_ready() would previously read the status register until "true", which means it never actually polled anything and would just read the register once. Now, it will properly wait until the DDC hardware is ready or until it reported an error. The 'busy' variable was also renamed to 'error' since these bits are set on error and not when the DDC hardware is busy. Since the DDC ready function is now working properly, the msleep(20) can be removed. Signed-off-by: Paul Cercueil --- drivers/gpu/drm/bridge/ite-it66121.c | 15 +++ 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c index 0a4fdfd7af44..bfb9c87a7019 100644 --- a/drivers/gpu/drm/bridge/ite-it66121.c +++ b/drivers/gpu/drm/bridge/ite-it66121.c @@ -440,15 +440,17 @@ static int it66121_configure_afe(struct it66121_ctx *ctx, static inline int it66121_wait_ddc_ready(struct it66121_ctx *ctx) { int ret, val; - u32 busy = IT66121_DDC_STATUS_NOACK | IT66121_DDC_STATUS_WAIT_BUS | - IT66121_DDC_STATUS_ARBI_LOSE; + u32 error = IT66121_DDC_STATUS_NOACK | IT66121_DDC_STATUS_WAIT_BUS | + IT66121_DDC_STATUS_ARBI_LOSE; + u32 done = IT66121_DDC_STATUS_TX_DONE; - ret = regmap_read_poll_timeout(ctx->regmap, IT66121_DDC_STATUS_REG, val, true, - IT66121_EDID_SLEEP_US, IT66121_EDID_TIMEOUT_US); + ret = regmap_read_poll_timeout(ctx->regmap, IT66121_DDC_STATUS_REG, val, + val & (error | done), IT66121_EDID_SLEEP_US, + IT66121_EDID_TIMEOUT_US); if (ret) return ret; - if (val & busy) + if (val & error) return -EAGAIN; return 0; @@ -582,9 +584,6 @@ static int it66121_get_edid_block(void *context, u8 *buf, offset += cnt; remain -= cnt; - /* Per programming manual, sleep here before emptying the FIFO */ - msleep(20); - ret = it66121_wait_ddc_ready(ctx); if (ret) return ret; -- 2.35.1
[PATCH 04/10] drm: bridge: it66121: Write AVI infoframe with regmap_bulk_write()
Since all AVI infoframe registers are contiguous in the address space, the AVI infoframe can be written in one go with regmap_bulk_write(). Signed-off-by: Paul Cercueil --- drivers/gpu/drm/bridge/ite-it66121.c | 27 +++ 1 file changed, 7 insertions(+), 20 deletions(-) diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c index 1840df30..0a4fdfd7af44 100644 --- a/drivers/gpu/drm/bridge/ite-it66121.c +++ b/drivers/gpu/drm/bridge/ite-it66121.c @@ -773,24 +773,9 @@ void it66121_bridge_mode_set(struct drm_bridge *bridge, const struct drm_display_mode *mode, const struct drm_display_mode *adjusted_mode) { - int ret, i; u8 buf[HDMI_INFOFRAME_SIZE(AVI)]; struct it66121_ctx *ctx = container_of(bridge, struct it66121_ctx, bridge); - const u16 aviinfo_reg[HDMI_AVI_INFOFRAME_SIZE] = { - IT66121_AVIINFO_DB1_REG, - IT66121_AVIINFO_DB2_REG, - IT66121_AVIINFO_DB3_REG, - IT66121_AVIINFO_DB4_REG, - IT66121_AVIINFO_DB5_REG, - IT66121_AVIINFO_DB6_REG, - IT66121_AVIINFO_DB7_REG, - IT66121_AVIINFO_DB8_REG, - IT66121_AVIINFO_DB9_REG, - IT66121_AVIINFO_DB10_REG, - IT66121_AVIINFO_DB11_REG, - IT66121_AVIINFO_DB12_REG, - IT66121_AVIINFO_DB13_REG - }; + int ret; mutex_lock(>lock); @@ -810,10 +795,12 @@ void it66121_bridge_mode_set(struct drm_bridge *bridge, } /* Write new AVI infoframe packet */ - for (i = 0; i < HDMI_AVI_INFOFRAME_SIZE; i++) { - if (regmap_write(ctx->regmap, aviinfo_reg[i], buf[i + HDMI_INFOFRAME_HEADER_SIZE])) - goto unlock; - } + ret = regmap_bulk_write(ctx->regmap, IT66121_AVIINFO_DB1_REG, + [HDMI_INFOFRAME_HEADER_SIZE], + HDMI_AVI_INFOFRAME_SIZE); + if (ret) + goto unlock; + if (regmap_write(ctx->regmap, IT66121_AVIINFO_CSUM_REG, buf[3])) goto unlock; -- 2.35.1
[PATCH 02/10] drm: bridge: it66121: Use devm_regulator_bulk_get_enable()
Simplify the code of the driver by using devm_regulator_bulk_get_enable(), which will handle powering up the regulators, and disabling them on probe error or module removal. Signed-off-by: Paul Cercueil --- drivers/gpu/drm/bridge/ite-it66121.c | 34 +++- 1 file changed, 8 insertions(+), 26 deletions(-) diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c index 7476cfbf9585..a698eec8f250 100644 --- a/drivers/gpu/drm/bridge/ite-it66121.c +++ b/drivers/gpu/drm/bridge/ite-it66121.c @@ -301,7 +301,6 @@ struct it66121_ctx { struct device *dev; struct gpio_desc *gpio_reset; struct i2c_client *client; - struct regulator_bulk_data supplies[3]; u32 bus_width; struct mutex lock; /* Protects fields below and device registers */ struct hdmi_avi_infoframe hdmi_avi_infoframe; @@ -342,16 +341,6 @@ static void it66121_hw_reset(struct it66121_ctx *ctx) gpiod_set_value(ctx->gpio_reset, 0); } -static inline int ite66121_power_on(struct it66121_ctx *ctx) -{ - return regulator_bulk_enable(ARRAY_SIZE(ctx->supplies), ctx->supplies); -} - -static inline int ite66121_power_off(struct it66121_ctx *ctx) -{ - return regulator_bulk_disable(ARRAY_SIZE(ctx->supplies), ctx->supplies); -} - static inline int it66121_preamble_ddc(struct it66121_ctx *ctx) { return regmap_write(ctx->regmap, IT66121_MASTER_SEL_REG, IT66121_MASTER_SEL_HOST); @@ -1512,6 +1501,10 @@ static int it66121_audio_codec_init(struct it66121_ctx *ctx, struct device *dev) return PTR_ERR_OR_ZERO(ctx->audio.pdev); } +static const char * const it66121_supplies[] = { + "vcn33", "vcn18", "vrf12" +}; + static int it66121_probe(struct i2c_client *client) { u32 revision_id, vendor_ids[2] = { 0 }, device_ids[2] = { 0 }; @@ -1564,26 +1557,18 @@ static int it66121_probe(struct i2c_client *client) i2c_set_clientdata(client, ctx); mutex_init(>lock); - ctx->supplies[0].supply = "vcn33"; - ctx->supplies[1].supply = "vcn18"; - ctx->supplies[2].supply = "vrf12"; - ret = devm_regulator_bulk_get(ctx->dev, 3, ctx->supplies); + ret = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(it66121_supplies), +it66121_supplies); if (ret) { - dev_err(ctx->dev, "regulator_bulk failed\n"); + dev_err(dev, "Failed to enable power supplies\n"); return ret; } - ret = ite66121_power_on(ctx); - if (ret) - return ret; - it66121_hw_reset(ctx); ctx->regmap = devm_regmap_init_i2c(client, _regmap_config); - if (IS_ERR(ctx->regmap)) { - ite66121_power_off(ctx); + if (IS_ERR(ctx->regmap)) return PTR_ERR(ctx->regmap); - } regmap_read(ctx->regmap, IT66121_VENDOR_ID0_REG, _ids[0]); regmap_read(ctx->regmap, IT66121_VENDOR_ID1_REG, _ids[1]); @@ -1596,7 +1581,6 @@ static int it66121_probe(struct i2c_client *client) if (vendor_ids[0] != IT66121_VENDOR_ID0 || vendor_ids[1] != IT66121_VENDOR_ID1 || device_ids[0] != IT66121_DEVICE_ID0 || device_ids[1] != IT66121_DEVICE_ID1) { - ite66121_power_off(ctx); return -ENODEV; } @@ -1609,7 +1593,6 @@ static int it66121_probe(struct i2c_client *client) IRQF_ONESHOT, dev_name(dev), ctx); if (ret < 0) { dev_err(dev, "Failed to request irq %d:%d\n", client->irq, ret); - ite66121_power_off(ctx); return ret; } @@ -1626,7 +1609,6 @@ static void it66121_remove(struct i2c_client *client) { struct it66121_ctx *ctx = i2c_get_clientdata(client); - ite66121_power_off(ctx); drm_bridge_remove(>bridge); mutex_destroy(>lock); } -- 2.35.1
[PATCH 03/10] drm: bridge: it66121: Use regmap_noinc_read()
Use regmap_noinc_read() instead of reading the data from the DDC FIFO one byte at a time. Signed-off-by: Paul Cercueil --- drivers/gpu/drm/bridge/ite-it66121.c | 13 ++--- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c index a698eec8f250..1840df30 100644 --- a/drivers/gpu/drm/bridge/ite-it66121.c +++ b/drivers/gpu/drm/bridge/ite-it66121.c @@ -589,13 +589,12 @@ static int it66121_get_edid_block(void *context, u8 *buf, if (ret) return ret; - do { - ret = regmap_read(ctx->regmap, IT66121_DDC_RD_FIFO_REG, ); - if (ret) - return ret; - *(buf++) = val; - cnt--; - } while (cnt > 0); + ret = regmap_noinc_read(ctx->regmap, IT66121_DDC_RD_FIFO_REG, + buf, cnt); + if (ret) + return ret; + + buf += cnt; } return 0; -- 2.35.1
[PATCH 01/10] dt-bindings: display: bridge: it66121: Add compatible string for IT6610
Add a new ite,it6610 compatible string to the IT66121 binding documentation, since the two chips are very similar. Signed-off-by: Paul Cercueil --- .../devicetree/bindings/display/bridge/ite,it66121.yaml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml b/Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml index 1b2185be92cd..72957be0ba3c 100644 --- a/Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml +++ b/Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml @@ -17,7 +17,9 @@ description: | properties: compatible: -const: ite,it66121 +enum: + - ite,it66121 + - ite,it6610 reg: maxItems: 1 -- 2.35.1