[PATCH v2] drm: bridge: fsl-ldb: fixup mode on freq mismatch

[PATCH v2] drm: bridge: fsl-ldb: fixup mode on freq mismatch

[Nikolaus Voss]

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(+)

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,
-- 
2.43.0

Re: [PATCH v2] drm: bridge: fsl-ldb: fixup mode on freq mismatch

[Marek Vasut]

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.

Re: [PATCH v2] drm: bridge: fsl-ldb: fixup mode on freq mismatch

[Nikolaus Voss]

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.

-- 
Nikolaus Voss

Re: [PATCH v2] drm: bridge: fsl-ldb: fixup mode on freq mismatch

[Marek Vasut]

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.

Re: [PATCH v2] drm: bridge: fsl-ldb: fixup mode on freq mismatch

[Nikolaus Voss]

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.

-- 
Nikolaus Voss

Re: [PATCH v2] drm: bridge: fsl-ldb: fixup mode on freq mismatch

[Marek Vasut]

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 .

Re: [PATCH v2] drm: bridge: fsl-ldb: fixup mode on freq mismatch

[Nikolaus Voss]

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.

-- 
Nikolaus Voss

Re: [PATCH v2] drm: bridge: fsl-ldb: fixup mode on freq mismatch

[kernel test robot]

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	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v2] drm: bridge: fsl-ldb: fixup mode on freq mismatch

[kernel test robot]

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	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v2] drm: bridge: fsl-ldb: fixup mode on freq mismatch

[Miquel Raynal]

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

Re: [PATCH v2] drm: bridge: fsl-ldb: fixup mode on freq mismatch

[Nikolaus Voss]

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.

-- 
Nikolaus Voss