| Message ID | 20241203191111.47B56F7@mail.steuer-voss.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drm: bridge: fsl-ldb: fixup mode on freq mismatch | expand |
On 12/3/24 8:09 PM, Nikolaus Voss wrote: > LDB clock has to be a fixed multiple of the pixel clock. > As LDB and pixel clock are derived from different clock sources Can you please share the content of /sys/kernel/debug/clk/clk_summary ? LDB and matching LCDIF should use the same PLL on MX8MP , else you might really run into odd issues.
Hi Marek, On 03.12.2024 21:15, Marek Vasut wrote: > On 12/3/24 8:09 PM, Nikolaus Voss wrote: >> LDB clock has to be a fixed multiple of the pixel clock. >> As LDB and pixel clock are derived from different clock sources > > Can you please share the content of /sys/kernel/debug/clk/clk_summary ? Sure. Without my patch: video_pll1_ref_sel 1 1 0 24000000 0 0 50000 Y deviceless no_connection_id video_pll1 1 1 0 1039500000 0 0 50000 Y deviceless no_connection_id video_pll1_bypass 1 1 0 1039500000 0 0 50000 Y deviceless no_connection_id video_pll1_out 2 2 0 1039500000 0 0 50000 Y deviceless no_connection_id media_ldb 1 1 0 346500000 0 0 50000 Y 32ec0000.blk-ctrl:bridge@5c ldb deviceless no_connection_id media_ldb_root_clk 0 0 0 346500000 0 0 50000 Y deviceless no_connection_id media_disp2_pix 1 1 0 51975000 0 0 50000 Y deviceless no_connection_id media_disp2_pix_root_clk 1 1 0 51975000 0 0 50000 Y 32e90000.display-controller pix Here 346500000 (media_ldb) != 7 * 51975000 (media_disp2_pix) -> distorted panel image (if any). The requested panel pixel clock from EDID is 51200000. This is the same with my patch: video_pll1_ref_sel 1 1 0 24000000 0 0 50000 Y deviceless no_connection_id video_pll1 1 1 0 1039500000 0 0 50000 Y deviceless no_connection_id video_pll1_bypass 1 1 0 1039500000 0 0 50000 Y deviceless no_connection_id video_pll1_out 2 2 0 1039500000 0 0 50000 Y deviceless no_connection_id media_ldb 1 1 0 346500000 0 0 50000 Y 32ec0000.blk-ctrl:bridge@5c ldb deviceless no_connection_id media_ldb_root_clk 0 0 0 346500000 0 0 50000 Y deviceless no_connection_id media_disp2_pix 1 1 0 49500000 0 0 50000 Y deviceless no_connection_id media_disp2_pix_root_clk 1 1 0 49500000 0 0 50000 Y 32e90000.display-controller pix So, here 346500000 (media_ldb) = 7 * 49500000 (media_disp2_pix). -> stable panel image, but pixel clock reduced to 49.5 MHz from requested 51.2 MHz. My conclusion: The clock source is the same, nevertheless the ldb/pixel clock constraint cannot be satisfied without either modifying the pll clock or the pixel clock.
Hi Nikolaus, kernel test robot noticed the following build warnings: [auto build test WARNING on linus/master] [also build test WARNING on drm-intel/for-linux-next drm-intel/for-linux-next-fixes drm-tip/drm-tip v6.13-rc1 next-20241203] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Nikolaus-Voss/drm-bridge-fsl-ldb-fixup-mode-on-freq-mismatch/20241204-115306 base: linus/master patch link: https://lore.kernel.org/r/20241203191111.47B56F7%40mail.steuer-voss.de patch subject: [PATCH v2] drm: bridge: fsl-ldb: fixup mode on freq mismatch config: arm-randconfig-003-20241204 (https://download.01.org/0day-ci/archive/20241204/202412042024.4mJmO3sM-lkp@intel.com/config) compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241204/202412042024.4mJmO3sM-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202412042024.4mJmO3sM-lkp@intel.com/ All warnings (new ones prefixed by >>): >> drivers/gpu/drm/bridge/fsl-ldb.c:125:30: warning: omitting the parameter name in a function definition is a C2x extension [-Wc2x-extensions] struct drm_bridge_state *, ^ drivers/gpu/drm/bridge/fsl-ldb.c:127:33: warning: omitting the parameter name in a function definition is a C2x extension [-Wc2x-extensions] struct drm_connector_state *) ^ 2 warnings generated. vim +125 drivers/gpu/drm/bridge/fsl-ldb.c 123 124 static int fsl_ldb_atomic_check(struct drm_bridge *bridge, > 125 struct drm_bridge_state *, 126 struct drm_crtc_state *crtc_state, 127 struct drm_connector_state *) 128 { 129 struct fsl_ldb *fsl_ldb = to_fsl_ldb(bridge); 130 const struct drm_display_mode *mode = &crtc_state->mode; 131 unsigned long requested_link_freq = 132 fsl_ldb_link_frequency(fsl_ldb, mode->clock); 133 unsigned long freq = clk_round_rate(fsl_ldb->clk, requested_link_freq); 134 135 if (freq != requested_link_freq) { 136 /* 137 * this will lead to flicker and incomplete lines on 138 * the attached display, adjust the CRTC clock 139 * accordingly. 140 */ 141 struct drm_display_mode *adjusted_mode = &crtc_state->adjusted_mode; 142 int pclk = freq / fsl_ldb_link_frequency(fsl_ldb, 1); 143 144 if (adjusted_mode->clock != pclk) { 145 dev_warn(fsl_ldb->dev, "Adjusted pixel clk to match LDB clk (%d kHz -> %d kHz)!\n", 146 adjusted_mode->clock, pclk); 147 148 adjusted_mode->clock = pclk; 149 adjusted_mode->crtc_clock = pclk; 150 } 151 } 152 153 return 0; 154 } 155
Hi Nikolaus,
kernel test robot noticed the following build warnings:
[auto build test WARNING on linus/master]
[also build test WARNING on drm-intel/for-linux-next drm-intel/for-linux-next-fixes drm-tip/drm-tip v6.13-rc1 next-20241204]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Nikolaus-Voss/drm-bridge-fsl-ldb-fixup-mode-on-freq-mismatch/20241204-115306
base: linus/master
patch link: https://lore.kernel.org/r/20241203191111.47B56F7%40mail.steuer-voss.de
patch subject: [PATCH v2] drm: bridge: fsl-ldb: fixup mode on freq mismatch
config: s390-allmodconfig (https://download.01.org/0day-ci/archive/20241205/202412050521.d87GwldA-lkp@intel.com/config)
compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project 592c0fe55f6d9a811028b5f3507be91458ab2713)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241205/202412050521.d87GwldA-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202412050521.d87GwldA-lkp@intel.com/
All warnings (new ones prefixed by >>):
In file included from drivers/gpu/drm/bridge/fsl-ldb.c:9:
In file included from include/linux/module.h:19:
In file included from include/linux/elf.h:6:
In file included from arch/s390/include/asm/elf.h:181:
In file included from arch/s390/include/asm/mmu_context.h:11:
In file included from arch/s390/include/asm/pgalloc.h:18:
In file included from include/linux/mm.h:2223:
include/linux/vmstat.h:504:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
504 | return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~ ^
505 | item];
| ~~~~
include/linux/vmstat.h:511:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
511 | return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~ ^
512 | NR_VM_NUMA_EVENT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~~
include/linux/vmstat.h:518:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
518 | return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
| ~~~~~~~~~~~ ^ ~~~
include/linux/vmstat.h:524:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
524 | return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~ ^
525 | NR_VM_NUMA_EVENT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~~
>> drivers/gpu/drm/bridge/fsl-ldb.c:125:30: warning: omitting the parameter name in a function definition is a C23 extension [-Wc23-extensions]
125 | struct drm_bridge_state *,
| ^
drivers/gpu/drm/bridge/fsl-ldb.c:127:33: warning: omitting the parameter name in a function definition is a C23 extension [-Wc23-extensions]
127 | struct drm_connector_state *)
| ^
6 warnings generated.
vim +125 drivers/gpu/drm/bridge/fsl-ldb.c
123
124 static int fsl_ldb_atomic_check(struct drm_bridge *bridge,
> 125 struct drm_bridge_state *,
126 struct drm_crtc_state *crtc_state,
127 struct drm_connector_state *)
128 {
129 struct fsl_ldb *fsl_ldb = to_fsl_ldb(bridge);
130 const struct drm_display_mode *mode = &crtc_state->mode;
131 unsigned long requested_link_freq =
132 fsl_ldb_link_frequency(fsl_ldb, mode->clock);
133 unsigned long freq = clk_round_rate(fsl_ldb->clk, requested_link_freq);
134
135 if (freq != requested_link_freq) {
136 /*
137 * this will lead to flicker and incomplete lines on
138 * the attached display, adjust the CRTC clock
139 * accordingly.
140 */
141 struct drm_display_mode *adjusted_mode = &crtc_state->adjusted_mode;
142 int pclk = freq / fsl_ldb_link_frequency(fsl_ldb, 1);
143
144 if (adjusted_mode->clock != pclk) {
145 dev_warn(fsl_ldb->dev, "Adjusted pixel clk to match LDB clk (%d kHz -> %d kHz)!\n",
146 adjusted_mode->clock, pclk);
147
148 adjusted_mode->clock = pclk;
149 adjusted_mode->crtc_clock = pclk;
150 }
151 }
152
153 return 0;
154 }
155
Hi Nikolaus, On 03/12/2024 at 20:09:52 +01, Nikolaus Voss <nv@vosn.de> wrote: > LDB clock has to be a fixed multiple of the pixel clock. Not only, IIUC it also needs to be synchronized, ie. share the same source. > As LDB and pixel clock are derived from different clock sources > (at least on imx8mp), Wait, what? I am sorry but that is not at all recommended, both should come from video_pll1 which the de-facto versatile PLL to use, no? Am I missing something here? > this constraint cannot be satisfied for > any pixel clock, which leads to flickering and incomplete > lines on the attached display. > > To overcome this, check this condition in .atomic_check() and > adapt the pixel clock accordingly. > > Cc: <stable@vger.kernel.org> > Fixes: 463db5c2ed4a ("drm: bridge: ldb: Implement simple Freescale i.MX8MP LDB bridge") > Nit: No \n here. > Signed-off-by: Nikolaus Voss <nv@vosn.de> Thanks, Miquèl
Hi Miquèl, On 06.12.2024 15:08, Miquel Raynal wrote: > On 03/12/2024 at 20:09:52 +01, Nikolaus Voss <nv@vosn.de> wrote: > >> LDB clock has to be a fixed multiple of the pixel clock. > > Not only, IIUC it also needs to be synchronized, ie. share the same > source. > >> As LDB and pixel clock are derived from different clock sources >> (at least on imx8mp), > > Wait, what? I am sorry but that is not at all recommended, both should > come from video_pll1 which the de-facto versatile PLL to use, no? Am I > missing something here? No, I was wrong :-). I've corrected the log text in the v3.
On 12/4/24 11:40 AM, Nikolaus Voss wrote: Hi, >>> LDB clock has to be a fixed multiple of the pixel clock. >>> As LDB and pixel clock are derived from different clock sources >> >> Can you please share the content of /sys/kernel/debug/clk/clk_summary ? > > Sure. Without my patch: > > video_pll1_ref_sel 1 1 0 24000000 > 0 0 50000 Y deviceless no_connection_id > video_pll1 1 1 0 1039500000 > 0 0 50000 Y deviceless no_connection_id > video_pll1_bypass 1 1 0 1039500000 > 0 0 50000 Y deviceless > no_connection_id > video_pll1_out 2 2 0 1039500000 > 0 0 50000 Y deviceless > no_connection_id > media_ldb 1 1 0 > 346500000 0 0 50000 Y 32ec0000.blk- > ctrl:bridge@5c ldb > deviceless > no_connection_id > media_ldb_root_clk 0 0 0 346500000 > 0 0 50000 Y deviceless > no_connection_id > media_disp2_pix 1 1 0 51975000 > 0 0 50000 Y deviceless > no_connection_id > media_disp2_pix_root_clk 1 1 0 > 51975000 0 0 50000 Y 32e90000.display- > controller pix > > Here 346500000 (media_ldb) != 7 * 51975000 (media_disp2_pix) > -> distorted panel image (if any). > The requested panel pixel clock from EDID is 51200000. Right, this is what Miquel is trying to solve with their series. > This is the same with my patch: > > video_pll1_ref_sel 1 1 0 24000000 > 0 0 50000 Y deviceless no_connection_id > video_pll1 1 1 0 1039500000 > 0 0 50000 Y deviceless no_connection_id > video_pll1_bypass 1 1 0 1039500000 > 0 0 50000 Y deviceless > no_connection_id > video_pll1_out 2 2 0 1039500000 > 0 0 50000 Y deviceless > no_connection_id > media_ldb 1 1 0 > 346500000 0 0 50000 Y 32ec0000.blk- > ctrl:bridge@5c ldb > deviceless > no_connection_id > media_ldb_root_clk 0 0 0 346500000 > 0 0 50000 Y deviceless > no_connection_id > media_disp2_pix 1 1 0 49500000 > 0 0 50000 Y deviceless > no_connection_id > media_disp2_pix_root_clk 1 1 0 > 49500000 0 0 50000 Y 32e90000.display- > controller pix > > So, here 346500000 (media_ldb) = 7 * 49500000 (media_disp2_pix). > -> stable panel image, but pixel clock reduced to 49.5 MHz from > requested 51.2 MHz. Inaccurate pixel clock and non-60Hz frame rate is not a win either. > My conclusion: The clock source is the same I agree . You wrote "derived from different clock sources" above, keyword:different, which is not correct. > , nevertheless the > ldb/pixel clock constraint cannot be satisfied without either > modifying the pll clock or the pixel clock. In this particular case, you surely do want to modify the PLL settings to achieve accurate pixel clock.
On 07.12.2024 12:46, Marek Vasut wrote: > On 12/4/24 11:40 AM, Nikolaus Voss wrote: >>>> LDB clock has to be a fixed multiple of the pixel clock. >>>> As LDB and pixel clock are derived from different clock sources >>> >>> Can you please share the content of /sys/kernel/debug/clk/clk_summary >>> ? >> >> Sure. Without my patch: >> >> video_pll1_ref_sel 1 1 0 >> 24000000 0 0 50000 Y deviceless >> no_connection_id >> video_pll1 1 1 0 1039500000 >> 0 0 50000 Y deviceless >> no_connection_id >> video_pll1_bypass 1 1 0 1039500000 >> 0 0 50000 Y deviceless >> no_connection_id >> video_pll1_out 2 2 0 1039500000 >> 0 0 50000 Y deviceless >> no_connection_id >> media_ldb 1 1 0 >> 346500000 0 0 50000 Y 32ec0000.blk- >> ctrl:bridge@5c ldb >> deviceless >> no_connection_id >> media_ldb_root_clk 0 0 0 346500000 >> 0 0 50000 Y deviceless >> no_connection_id >> media_disp2_pix 1 1 0 >> 51975000 0 0 50000 Y deviceless >> no_connection_id >> media_disp2_pix_root_clk 1 1 0 >> 51975000 0 0 50000 Y 32e90000.display- >> controller pix >> >> Here 346500000 (media_ldb) != 7 * 51975000 (media_disp2_pix) >> -> distorted panel image (if any). >> The requested panel pixel clock from EDID is 51200000. > > Right, this is what Miquel is trying to solve with their series. > >> This is the same with my patch: >> >> video_pll1_ref_sel 1 1 0 >> 24000000 0 0 50000 Y deviceless >> no_connection_id >> video_pll1 1 1 0 1039500000 >> 0 0 50000 Y deviceless >> no_connection_id >> video_pll1_bypass 1 1 0 1039500000 >> 0 0 50000 Y deviceless >> no_connection_id >> video_pll1_out 2 2 0 1039500000 >> 0 0 50000 Y deviceless >> no_connection_id >> media_ldb 1 1 0 >> 346500000 0 0 50000 Y 32ec0000.blk- >> ctrl:bridge@5c ldb >> deviceless >> no_connection_id >> media_ldb_root_clk 0 0 0 346500000 >> 0 0 50000 Y deviceless >> no_connection_id >> media_disp2_pix 1 1 0 >> 49500000 0 0 50000 Y deviceless >> no_connection_id >> media_disp2_pix_root_clk 1 1 0 >> 49500000 0 0 50000 Y 32e90000.display- >> controller pix >> >> So, here 346500000 (media_ldb) = 7 * 49500000 (media_disp2_pix). >> -> stable panel image, but pixel clock reduced to 49.5 MHz from >> requested 51.2 MHz. > > Inaccurate pixel clock and non-60Hz frame rate is not a win either. Some percents of deviation is usually not visible. > >> My conclusion: The clock source is the same > > I agree . > > You wrote "derived from different clock sources" above, > keyword:different, which is not correct. > >> , nevertheless the >> ldb/pixel clock constraint cannot be satisfied without either >> modifying the pll clock or the pixel clock. > In this particular case, you surely do want to modify the PLL settings > to achieve accurate pixel clock. No, in this case there is a 3 percent deviation, resulting in 58 Hz frame rate instead of 60 Hz.
On 12/9/24 10:27 AM, Nikolaus Voss wrote: > On 07.12.2024 12:46, Marek Vasut wrote: >> On 12/4/24 11:40 AM, Nikolaus Voss wrote: >>>>> LDB clock has to be a fixed multiple of the pixel clock. >>>>> As LDB and pixel clock are derived from different clock sources >>>> >>>> Can you please share the content of /sys/kernel/debug/clk/clk_summary ? >>> >>> Sure. Without my patch: >>> >>> video_pll1_ref_sel 1 1 0 24000000 >>> 0 0 50000 Y deviceless no_connection_id >>> video_pll1 1 1 0 1039500000 >>> 0 0 50000 Y deviceless no_connection_id >>> video_pll1_bypass 1 1 0 1039500000 >>> 0 0 50000 Y deviceless no_connection_id >>> video_pll1_out 2 2 0 1039500000 >>> 0 0 50000 Y deviceless no_connection_id >>> media_ldb 1 1 0 346500000 >>> 0 0 50000 Y 32ec0000.blk- ctrl:bridge@5c ldb >>> deviceless >>> no_connection_id >>> media_ldb_root_clk 0 0 0 346500000 >>> 0 0 50000 Y deviceless >>> no_connection_id >>> media_disp2_pix 1 1 0 51975000 >>> 0 0 50000 Y deviceless >>> no_connection_id >>> media_disp2_pix_root_clk 1 1 0 >>> 51975000 0 0 50000 Y 32e90000.display- >>> controller pix >>> >>> Here 346500000 (media_ldb) != 7 * 51975000 (media_disp2_pix) >>> -> distorted panel image (if any). >>> The requested panel pixel clock from EDID is 51200000. >> >> Right, this is what Miquel is trying to solve with their series. >> >>> This is the same with my patch: >>> >>> video_pll1_ref_sel 1 1 0 24000000 >>> 0 0 50000 Y deviceless no_connection_id >>> video_pll1 1 1 0 1039500000 >>> 0 0 50000 Y deviceless no_connection_id >>> video_pll1_bypass 1 1 0 1039500000 >>> 0 0 50000 Y deviceless no_connection_id >>> video_pll1_out 2 2 0 1039500000 >>> 0 0 50000 Y deviceless no_connection_id >>> media_ldb 1 1 0 346500000 >>> 0 0 50000 Y 32ec0000.blk- ctrl:bridge@5c ldb >>> deviceless >>> no_connection_id >>> media_ldb_root_clk 0 0 0 346500000 >>> 0 0 50000 Y deviceless >>> no_connection_id >>> media_disp2_pix 1 1 0 49500000 >>> 0 0 50000 Y deviceless >>> no_connection_id >>> media_disp2_pix_root_clk 1 1 0 >>> 49500000 0 0 50000 Y 32e90000.display- >>> controller pix >>> >>> So, here 346500000 (media_ldb) = 7 * 49500000 (media_disp2_pix). >>> -> stable panel image, but pixel clock reduced to 49.5 MHz from >>> requested 51.2 MHz. >> >> Inaccurate pixel clock and non-60Hz frame rate is not a win either. > > Some percents of deviation is usually not visible. The PLL is accurate, so this kind of non-60 Hz frame rate compromise really should not be necessary. >>> My conclusion: The clock source is the same >> >> I agree . >> >> You wrote "derived from different clock sources" above, >> keyword:different, which is not correct. >> >>> , nevertheless the >>> ldb/pixel clock constraint cannot be satisfied without either >>> modifying the pll clock or the pixel clock. >> In this particular case, you surely do want to modify the PLL settings >> to achieve accurate pixel clock. > > No, in this case there is a 3 percent deviation, resulting in 58 Hz > frame rate instead of 60 Hz. Consider e.g. 60 FPS video playback, on 58 Hz refresh panel it will suffer from some stutter . It is better to aim for the 60 Hz then .
Hi Marek, On 09.12.2024 22:51, Marek Vasut wrote: > On 12/9/24 10:27 AM, Nikolaus Voss wrote: >> On 07.12.2024 12:46, Marek Vasut wrote: >>> On 12/4/24 11:40 AM, Nikolaus Voss wrote: >>>>>> LDB clock has to be a fixed multiple of the pixel clock. >>>>>> As LDB and pixel clock are derived from different clock sources >>>>> >>>>> Can you please share the content of >>>>> /sys/kernel/debug/clk/clk_summary ? >>>> >>>> Sure. Without my patch: >>>> >>>> video_pll1_ref_sel 1 1 0 24000000 >>>> 0 0 50000 Y deviceless no_connection_id >>>> video_pll1 1 1 0 1039500000 >>>> 0 0 50000 Y deviceless no_connection_id >>>> video_pll1_bypass 1 1 0 1039500000 >>>> 0 0 50000 Y deviceless no_connection_id >>>> video_pll1_out 2 2 0 1039500000 >>>> 0 0 50000 Y deviceless >>>> no_connection_id >>>> media_ldb 1 1 0 346500000 >>>> 0 0 50000 Y 32ec0000.blk- ctrl:bridge@5c ldb >>>> deviceless >>>> no_connection_id >>>> media_ldb_root_clk 0 0 0 346500000 >>>> 0 0 50000 Y deviceless >>>> no_connection_id >>>> media_disp2_pix 1 1 0 51975000 >>>> 0 0 50000 Y deviceless >>>> no_connection_id >>>> media_disp2_pix_root_clk 1 1 0 >>>> 51975000 0 0 50000 Y 32e90000.display- >>>> controller pix >>>> >>>> Here 346500000 (media_ldb) != 7 * 51975000 (media_disp2_pix) >>>> -> distorted panel image (if any). >>>> The requested panel pixel clock from EDID is 51200000. >>> >>> Right, this is what Miquel is trying to solve with their series. >>> >>>> This is the same with my patch: >>>> >>>> video_pll1_ref_sel 1 1 0 24000000 >>>> 0 0 50000 Y deviceless no_connection_id >>>> video_pll1 1 1 0 1039500000 >>>> 0 0 50000 Y deviceless no_connection_id >>>> video_pll1_bypass 1 1 0 1039500000 >>>> 0 0 50000 Y deviceless no_connection_id >>>> video_pll1_out 2 2 0 1039500000 >>>> 0 0 50000 Y deviceless >>>> no_connection_id >>>> media_ldb 1 1 0 346500000 >>>> 0 0 50000 Y 32ec0000.blk- ctrl:bridge@5c ldb >>>> deviceless >>>> no_connection_id >>>> media_ldb_root_clk 0 0 0 346500000 >>>> 0 0 50000 Y deviceless >>>> no_connection_id >>>> media_disp2_pix 1 1 0 49500000 >>>> 0 0 50000 Y deviceless >>>> no_connection_id >>>> media_disp2_pix_root_clk 1 1 0 >>>> 49500000 0 0 50000 Y 32e90000.display- >>>> controller pix >>>> >>>> So, here 346500000 (media_ldb) = 7 * 49500000 (media_disp2_pix). >>>> -> stable panel image, but pixel clock reduced to 49.5 MHz from >>>> requested 51.2 MHz. >>> >>> Inaccurate pixel clock and non-60Hz frame rate is not a win either. >> >> Some percents of deviation is usually not visible. > > The PLL is accurate, so this kind of non-60 Hz frame rate compromise > really should not be necessary. > >>>> My conclusion: The clock source is the same >>> >>> I agree . >>> >>> You wrote "derived from different clock sources" above, >>> keyword:different, which is not correct. >>> >>>> , nevertheless the >>>> ldb/pixel clock constraint cannot be satisfied without either >>>> modifying the pll clock or the pixel clock. >>> In this particular case, you surely do want to modify the PLL >>> settings >>> to achieve accurate pixel clock. >> >> No, in this case there is a 3 percent deviation, resulting in 58 Hz >> frame rate instead of 60 Hz. > Consider e.g. 60 FPS video playback, on 58 Hz refresh panel it will > suffer from some stutter . It is better to aim for the 60 Hz then . This is a relevant use case, I agree. But even with a dedicated video PLL (what a luxury in the embedded world!) you will eventually have to drop or double a frame as the clocks are asynchronous. And the sync problem still exists with 25 or 50 FPS videos.
diff --git a/drivers/gpu/drm/bridge/fsl-ldb.c b/drivers/gpu/drm/bridge/fsl-ldb.c index 0e4bac7dd04ff..5b09529564609 100644 --- a/drivers/gpu/drm/bridge/fsl-ldb.c +++ b/drivers/gpu/drm/bridge/fsl-ldb.c @@ -121,6 +121,38 @@ static int fsl_ldb_attach(struct drm_bridge *bridge, bridge, flags); } +static int fsl_ldb_atomic_check(struct drm_bridge *bridge, + struct drm_bridge_state *, + struct drm_crtc_state *crtc_state, + struct drm_connector_state *) +{ + struct fsl_ldb *fsl_ldb = to_fsl_ldb(bridge); + const struct drm_display_mode *mode = &crtc_state->mode; + unsigned long requested_link_freq = + fsl_ldb_link_frequency(fsl_ldb, mode->clock); + unsigned long freq = clk_round_rate(fsl_ldb->clk, requested_link_freq); + + if (freq != requested_link_freq) { + /* + * this will lead to flicker and incomplete lines on + * the attached display, adjust the CRTC clock + * accordingly. + */ + struct drm_display_mode *adjusted_mode = &crtc_state->adjusted_mode; + int pclk = freq / fsl_ldb_link_frequency(fsl_ldb, 1); + + if (adjusted_mode->clock != pclk) { + dev_warn(fsl_ldb->dev, "Adjusted pixel clk to match LDB clk (%d kHz -> %d kHz)!\n", + adjusted_mode->clock, pclk); + + adjusted_mode->clock = pclk; + adjusted_mode->crtc_clock = pclk; + } + } + + return 0; +} + static void fsl_ldb_atomic_enable(struct drm_bridge *bridge, struct drm_bridge_state *old_bridge_state) { @@ -280,6 +312,7 @@ fsl_ldb_mode_valid(struct drm_bridge *bridge, static const struct drm_bridge_funcs funcs = { .attach = fsl_ldb_attach, + .atomic_check = fsl_ldb_atomic_check, .atomic_enable = fsl_ldb_atomic_enable, .atomic_disable = fsl_ldb_atomic_disable, .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
LDB clock has to be a fixed multiple of the pixel clock. As LDB and pixel clock are derived from different clock sources (at least on imx8mp), this constraint cannot be satisfied for any pixel clock, which leads to flickering and incomplete lines on the attached display. To overcome this, check this condition in .atomic_check() and adapt the pixel clock accordingly. Cc: <stable@vger.kernel.org> Fixes: 463db5c2ed4a ("drm: bridge: ldb: Implement simple Freescale i.MX8MP LDB bridge") Signed-off-by: Nikolaus Voss <nv@vosn.de> --- v2: - use .atomic_check() instead of .mode_fixup() (Dmitry Baryshkov) - add Fixes tag (Liu Ying) - use fsl_ldb_link_frequency() and drop const qualifier for struct fsl_ldb* (Liu Ying) drivers/gpu/drm/bridge/fsl-ldb.c | 33 ++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+)