| Message ID | 20241114065759.3341908-3-victor.liu@nxp.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add ITE IT6263 LVDS to HDMI converter support | expand |
> Subject: [PATCH v7 2/7] Revert "clk: imx: clk-imx8mp: Allow > media_disp pixel clock reconfigure parent rate" > > This reverts commit ff06ea04e4cf3ba2f025024776e83bfbdfa05155. > > media_disp1_pix clock is the pixel clock of the first i.MX8MP LCDIFv3 > display controller, while media_disp2_pix clock is the pixel clock of the > second i.MX8MP LCDIFv3 display controller. The two display > controllers connect with Samsung MIPI DSI controller and LVDS Display > Bridge(LDB) respectively. Since the two display controllers are driven > by separate DRM driver instances and the two pixel clocks may be > derived from the same video_pll1_out clock(sys_pll3_out clock could > be already used to derive audio_axi clock), there is no way to negotiate > a dynamically changeable video_pll1_out clock rate to satisfy both of > the two display controllers. In this case, the only solution to drive > them with the single video_pll1_out clock is to assign a > sensible/unchangeable clock rate for video_pll1_out clock. Thus, there > is no need to set the CLK_SET_RATE_PARENT flag for > media_disp{1,2}_pix clocks, drop it then. > > Fixes: ff06ea04e4cf ("clk: imx: clk-imx8mp: Allow media_disp pixel > clock reconfigure parent rate") > Signed-off-by: Liu Ying <victor.liu@nxp.com> > --- Acked-by: Peng Fan <peng.fan@nxp.com>
On 11/14/24 7:57 AM, Liu Ying wrote: > This reverts commit ff06ea04e4cf3ba2f025024776e83bfbdfa05155. > > media_disp1_pix clock is the pixel clock of the first i.MX8MP LCDIFv3 > display controller, while media_disp2_pix clock is the pixel clock of > the second i.MX8MP LCDIFv3 display controller. The two display > controllers connect with Samsung MIPI DSI controller and LVDS Display > Bridge(LDB) respectively. Since the two display controllers are driven > by separate DRM driver instances and the two pixel clocks may be derived > from the same video_pll1_out clock(sys_pll3_out clock could be already > used to derive audio_axi clock), there is no way to negotiate a dynamically > changeable video_pll1_out clock rate to satisfy both of the two display > controllers. In this case, the only solution to drive them with the > single video_pll1_out clock is to assign a sensible/unchangeable clock > rate for video_pll1_out clock. Thus, there is no need to set the > CLK_SET_RATE_PARENT flag for media_disp{1,2}_pix clocks, drop it then. > > Fixes: ff06ea04e4cf ("clk: imx: clk-imx8mp: Allow media_disp pixel clock reconfigure parent rate") > Signed-off-by: Liu Ying <victor.liu@nxp.com> Uh, I almost missed this revert between all the LDB patches. This revert will break my usecase on MX8MP where I need to operate two disparate panels attached to LVDS and TC358767 DSI-to-DP bridge and I need accurate pixel clock for both. Not being able to configure accurate pixel clock will make the displays not work, so from my side, this is a NAK, sorry. There has to be some better solution which still allows the PLL reconfiguration to achieve accurate pixel clock.
Hi Marek, On 11/15/2024, Marek Vasut wrote: > On 11/14/24 7:57 AM, Liu Ying wrote: > > This reverts commit ff06ea04e4cf3ba2f025024776e83bfbdfa05155. > > > > media_disp1_pix clock is the pixel clock of the first i.MX8MP LCDIFv3 > > display controller, while media_disp2_pix clock is the pixel clock of > > the second i.MX8MP LCDIFv3 display controller. The two display > > controllers connect with Samsung MIPI DSI controller and LVDS Display > > Bridge(LDB) respectively. Since the two display controllers are driven > > by separate DRM driver instances and the two pixel clocks may be derived > > from the same video_pll1_out clock(sys_pll3_out clock could be already > > used to derive audio_axi clock), there is no way to negotiate a dynamically > > changeable video_pll1_out clock rate to satisfy both of the two display > > controllers. In this case, the only solution to drive them with the > > single video_pll1_out clock is to assign a sensible/unchangeable clock > > rate for video_pll1_out clock. Thus, there is no need to set the > > CLK_SET_RATE_PARENT flag for media_disp{1,2}_pix clocks, drop it then. > > > > Fixes: ff06ea04e4cf ("clk: imx: clk-imx8mp: Allow media_disp pixel clock > reconfigure parent rate") > > Signed-off-by: Liu Ying <victor.liu@nxp.com> > Uh, I almost missed this revert between all the LDB patches. > > This revert will break my usecase on MX8MP where I need to operate two > disparate panels attached to LVDS and TC358767 DSI-to-DP bridge and I > need accurate pixel clock for both. Not being able to configure accurate > pixel clock will make the displays not work, so from my side, this is a > NAK, sorry. Is your usecase in upstream kernel? If yes, which DT file implements the usecase? I guess it's im8mp-dhcom-som.dtsi authored by you, but it only contains the DT node for TC358767, but not LVDS panel. Can you please elaborate about the failure case? You still may assign an accurate PLL rate in DT. This patch only makes the PLL rate be unchangeable dynamically in runtime. That means the existing imx8m-dhcom-som.dtsi would use IMX8MP_VIDEO_PLL1_OUT(running at 1.0395GHz) as the parent clock of IMX8MP_CLK_MEDIA_DISP1_PIX (for LCDIF1/DSI), since it includes imx8mp.dsti. I assume it should be able to support typical video modes like 1080p60 video mode with 148.5MHz pixel clock at least with 1.0395GHz PLL rate. Granted that less video modes read from DP monitor would be supported without dynamically changeable PLL rates, this is something we have to accept because some i.MX8MP platforms(like i.MX8MP EVK) have to share IMX8MP_VIDEO_PLL1_OUT between LVDS and MIPI DSI display pipelines. The missing part is that we need to do mode validation for the MIPI DSI display pipeline either in samsung-dsim.c or lcdif_kms.c to filter unsupported video mode out. Is this missing mode validation the cause of your failure case? > > There has to be some better solution which still allows the PLL > reconfiguration to achieve accurate pixel clock. As I mentioned in cover letter, the only solution to support LVDS and MIPI DSI displays on all i.MX8MP platforms is to assign a sensible and unchangeable PLL rate in DT. Some platforms may use two separate PLLs for the LVDS and MIPI DSI display pipelines, while some others have to use only the single IMX8MP_VIDEO_PLL1_OUT because all other eligible PLLs are used up. That's all fine, just being platforms dependent. The only limitation of the solution is that some platforms couldn't support some particular LVDS and MIPI DSI displays at the same time due to lack of PLLs, but this has to be accepted since the shared IMX8MP_VIDEO_PLL1_OUT case needs to be supported and the two display pipelines are not aware of each other from kernel's point of view. I hope that we can agree on this solution first before spreading discussions across different threads and eventually the NAK can be taken back. Regards, Liu Ying
On 11/18/24 4:54 AM, Ying Liu wrote: > Hi Marek, Hi, >>> media_disp1_pix clock is the pixel clock of the first i.MX8MP LCDIFv3 >>> display controller, while media_disp2_pix clock is the pixel clock of >>> the second i.MX8MP LCDIFv3 display controller. The two display >>> controllers connect with Samsung MIPI DSI controller and LVDS Display >>> Bridge(LDB) respectively. Since the two display controllers are driven >>> by separate DRM driver instances and the two pixel clocks may be derived >>> from the same video_pll1_out clock(sys_pll3_out clock could be already >>> used to derive audio_axi clock), there is no way to negotiate a dynamically >>> changeable video_pll1_out clock rate to satisfy both of the two display >>> controllers. In this case, the only solution to drive them with the >>> single video_pll1_out clock is to assign a sensible/unchangeable clock >>> rate for video_pll1_out clock. Thus, there is no need to set the >>> CLK_SET_RATE_PARENT flag for media_disp{1,2}_pix clocks, drop it then. >>> >>> Fixes: ff06ea04e4cf ("clk: imx: clk-imx8mp: Allow media_disp pixel clock >> reconfigure parent rate") >>> Signed-off-by: Liu Ying <victor.liu@nxp.com> >> Uh, I almost missed this revert between all the LDB patches. >> >> This revert will break my usecase on MX8MP where I need to operate two >> disparate panels attached to LVDS and TC358767 DSI-to-DP bridge and I >> need accurate pixel clock for both. Not being able to configure accurate >> pixel clock will make the displays not work, so from my side, this is a >> NAK, sorry. > > Is your usecase in upstream kernel? If yes, which DT file implements the > usecase? I guess it's im8mp-dhcom-som.dtsi authored by you, but it only > contains the DT node for TC358767, but not LVDS panel. > > Can you please elaborate about the failure case? The TC9595 can drive an DP output, for that the clock which have to be set on the LCDIF cannot be predicted, as that information comes from the monitor EDID/DPCD. That is why the LCDIF has to be able to configure the Video PLL1 clock to accurate clock frequency. For the LVDS LDB, the use case is the other way around -- the pixel clock which should be generated by LCDIF and fed to LDB are known from the panel type listed in DT, but they should still be accurate. > You still may assign an accurate PLL rate in DT. > This patch only makes the PLL rate be unchangeable dynamically in > runtime. That means the existing imx8m-dhcom-som.dtsi would use > IMX8MP_VIDEO_PLL1_OUT(running at 1.0395GHz) as the parent clock > of IMX8MP_CLK_MEDIA_DISP1_PIX (for LCDIF1/DSI), since it includes > imx8mp.dsti. I assume it should be able to support typical video modes > like 1080p60 video mode with 148.5MHz pixel clock at least with 1.0395GHz > PLL rate. This will break multiple DP monitors I tested so far I'm afraid. And I spent a LOT of time wrestling with the TC9595 bridge to make sure it actually does work well. > Granted that less video modes read from DP monitor would > be supported without dynamically changeable PLL rates, this is something > we have to accept because some i.MX8MP platforms(like i.MX8MP EVK) > have to share IMX8MP_VIDEO_PLL1_OUT between LVDS and MIPI DSI > display pipelines. What I need is the use of two full PLL1443x (like Video PLL and Audio PLL1/2) , one for each display output, and those PLLs have to be fully configurable to produce accurate pixel clock for each connected panel. Otherwise I cannot make proper use of the video output capabilities of the MX8MP SoC. > The missing part is that we need to do mode validation > for the MIPI DSI display pipeline either in samsung-dsim.c or lcdif_kms.c > to filter unsupported video mode out. Is this missing mode validation > the cause of your failure case? I do want to support the various modes, I do not want to filter them out. They can be supported, the only "problem" is the shared Video PLL which is not really an actual problem in my case, because I do not use shared Video PLL, I use two separate PLLs. I think what is needed is for the LCDIF1/LCDIF2/LDB to figure out whether they share the Video PLL at all (you already suggested the clock subsystem can provide that information), and then if: - yes, agree on some sort of middle-ground frequency to configure into the Video PLL, frequency which somehow fits all three consumers (LCDIF1,LCDIF2,LDB) - no, configure each consumer upstream clock to generate accurate pixel clock for that consumer Something like ^ would make MX8MP EVK (the "yes" case) with shared Video PLL work, without breaking my use case (the "no" case), right ? (*) >> There has to be some better solution which still allows the PLL >> reconfiguration to achieve accurate pixel clock. > > As I mentioned in cover letter, the only solution to support LVDS and > MIPI DSI displays on all i.MX8MP platforms is to assign a sensible and > unchangeable PLL rate in DT. I am currently using Video PLL and Audio PLL to drive DSI and LVDS outputs from each, so no, fixed Video PLL assignment in DT is not the only solution. > Some platforms may use two separate > PLLs for the LVDS and MIPI DSI display pipelines, while some others > have to use only the single IMX8MP_VIDEO_PLL1_OUT because > all other eligible PLLs are used up. That's all fine, just being platforms > dependent. The only limitation of the solution is that some platforms > couldn't support some particular LVDS and MIPI DSI displays at the > same time due to lack of PLLs, but this has to be accepted since > the shared IMX8MP_VIDEO_PLL1_OUT case needs to be supported and > the two display pipelines are not aware of each other from kernel's > point of view. Can something like (*) above be implemented instead, so both Shared and separate PLLs would be supported ? That should solve both of our use cases, right ? > I hope that we can agree on this solution first before spreading > discussions across different threads and eventually the NAK can be > taken back. I cannot really agree on a solution which breaks one of my use cases, but maybe there is an alternative how to support both options, see (*) above ?
On 11/19/24, Marek Vasut wrote: > On 11/18/24 4:54 AM, Ying Liu wrote: > > Hi Marek, > > Hi, > > >>> media_disp1_pix clock is the pixel clock of the first i.MX8MP LCDIFv3 > >>> display controller, while media_disp2_pix clock is the pixel clock of > >>> the second i.MX8MP LCDIFv3 display controller. The two display > >>> controllers connect with Samsung MIPI DSI controller and LVDS Display > >>> Bridge(LDB) respectively. Since the two display controllers are driven > >>> by separate DRM driver instances and the two pixel clocks may be > derived > >>> from the same video_pll1_out clock(sys_pll3_out clock could be already > >>> used to derive audio_axi clock), there is no way to negotiate a > dynamically > >>> changeable video_pll1_out clock rate to satisfy both of the two display > >>> controllers. In this case, the only solution to drive them with the > >>> single video_pll1_out clock is to assign a sensible/unchangeable clock > >>> rate for video_pll1_out clock. Thus, there is no need to set the > >>> CLK_SET_RATE_PARENT flag for media_disp{1,2}_pix clocks, drop it then. > >>> > >>> Fixes: ff06ea04e4cf ("clk: imx: clk-imx8mp: Allow media_disp pixel clock > >> reconfigure parent rate") > >>> Signed-off-by: Liu Ying <victor.liu@nxp.com> > >> Uh, I almost missed this revert between all the LDB patches. > >> > >> This revert will break my usecase on MX8MP where I need to operate two > >> disparate panels attached to LVDS and TC358767 DSI-to-DP bridge and I > >> need accurate pixel clock for both. Not being able to configure accurate > >> pixel clock will make the displays not work, so from my side, this is a > >> NAK, sorry. > > > > Is your usecase in upstream kernel? If yes, which DT file implements the > > usecase? I guess it's im8mp-dhcom-som.dtsi authored by you, but it only > > contains the DT node for TC358767, but not LVDS panel. > > > > Can you please elaborate about the failure case? > > The TC9595 can drive an DP output, for that the clock which have to be > set on the LCDIF cannot be predicted, as that information comes from the > monitor EDID/DPCD. That is why the LCDIF has to be able to configure the > Video PLL1 clock to accurate clock frequency. > > For the LVDS LDB, the use case is the other way around -- the pixel > clock which should be generated by LCDIF and fed to LDB are known from > the panel type listed in DT, but they should still be accurate. Thanks for the information. I think the key question is whether the alternative solution(*) you mentioned below stands or not, in other words, whether LCDIF1/LCDIF2/LDB drivers know that they are sharing a PLL or not. > > > You still may assign an accurate PLL rate in DT. > > This patch only makes the PLL rate be unchangeable dynamically in > > runtime. That means the existing imx8m-dhcom-som.dtsi would use > > IMX8MP_VIDEO_PLL1_OUT(running at 1.0395GHz) as the parent clock > > of IMX8MP_CLK_MEDIA_DISP1_PIX (for LCDIF1/DSI), since it includes > > imx8mp.dsti. I assume it should be able to support typical video modes > > like 1080p60 video mode with 148.5MHz pixel clock at least with 1.0395GHz > > PLL rate. > > This will break multiple DP monitors I tested so far I'm afraid. And I > spent a LOT of time wrestling with the TC9595 bridge to make sure it > actually does work well. If the DP monitors support typical video modes like 1080p60 with 148.5MHz pixel clock rate, I assume these typical video modes work still ok with this patch at least. Please help confirm this, since if the alternative solution(*) doesn't stand, we would know those video modes still work ok with my solution(fixed PLL rate). > > > Granted that less video modes read from DP monitor would > > be supported without dynamically changeable PLL rates, this is something > > we have to accept because some i.MX8MP platforms(like i.MX8MP EVK) > > have to share IMX8MP_VIDEO_PLL1_OUT between LVDS and MIPI DSI > > display pipelines. > > What I need is the use of two full PLL1443x (like Video PLL and Audio > PLL1/2) , one for each display output, and those PLLs have to be fully > configurable to produce accurate pixel clock for each connected panel. > Otherwise I cannot make proper use of the video output capabilities of > the MX8MP SoC. Yeah, I understand your requirements. However, it still depends on whether the alternative solution(*) stands or not. > > > The missing part is that we need to do mode validation > > for the MIPI DSI display pipeline either in samsung-dsim.c or lcdif_kms.c > > to filter unsupported video mode out. Is this missing mode validation > > the cause of your failure case? > > I do want to support the various modes, I do not want to filter them > out. They can be supported, the only "problem" is the shared Video PLL > which is not really an actual problem in my case, because I do not use > shared Video PLL, I use two separate PLLs. > > I think what is needed is for the LCDIF1/LCDIF2/LDB to figure out > whether they share the Video PLL at all (you already suggested the clock > subsystem can provide that information), and then if: But, how to let LCDIF1/LCDIF2/LDB drivers to figure out that? I didn't suggest that the clock subsystem can provide that information. > - yes, agree on some sort of middle-ground frequency to configure into > the Video PLL, frequency which somehow fits all three consumers > (LCDIF1,LCDIF2,LDB) > - no, configure each consumer upstream clock to generate accurate pixel > clock for that consumer > > Something like ^ would make MX8MP EVK (the "yes" case) with shared Video > PLL work, without breaking my use case (the "no" case), right ? (*) In an ideal world, right. But, again how to let LCDIF1/LCDIF2/LDB drivers to figure out that they are sharing a PLL? > > >> There has to be some better solution which still allows the PLL > >> reconfiguration to achieve accurate pixel clock. > > > > As I mentioned in cover letter, the only solution to support LVDS and > > MIPI DSI displays on all i.MX8MP platforms is to assign a sensible and > > unchangeable PLL rate in DT. > > I am currently using Video PLL and Audio PLL to drive DSI and LVDS > outputs from each, so no, fixed Video PLL assignment in DT is not the > only solution. > > > Some platforms may use two separate > > PLLs for the LVDS and MIPI DSI display pipelines, while some others > > have to use only the single IMX8MP_VIDEO_PLL1_OUT because > > all other eligible PLLs are used up. That's all fine, just being platforms > > dependent. The only limitation of the solution is that some platforms > > couldn't support some particular LVDS and MIPI DSI displays at the > > same time due to lack of PLLs, but this has to be accepted since > > the shared IMX8MP_VIDEO_PLL1_OUT case needs to be supported and > > the two display pipelines are not aware of each other from kernel's > > point of view. > > Can something like (*) above be implemented instead, so both Shared and > separate PLLs would be supported ? That should solve both of our use > cases, right ? I don't see any clear way to implement something like(*). Take the 3 i.MX8MP LCDIFs as one graphic card driven by one imx-lcdif DRM instance? Would it be too intrusive? Use clk_get_parent() to determine if the pixel clocks of LCDIF1&2 are sharing PLL(note clk_get_parent() implementation contains a TODO: Create a per-user clk.)? How to do mode validation for the shared PLL case(note mode_valid() callback is supposed to look at nothing more than passed-in mode)? Use clk_set_rate_range() to fix the PLL rate(min == max)? > > > I hope that we can agree on this solution first before spreading > > discussions across different threads and eventually the NAK can be > > taken back. > > I cannot really agree on a solution which breaks one of my use cases, > but maybe there is an alternative how to support both options, see (*) > above ? I tend to say there is no any alternative solution to satisfy both separate PLLs and shared PLL use cases, or even if there is one, it won't be easy to implement. If you know one, please shout it out. Regards, Liu Ying
On 11/19/24 9:18 AM, Ying Liu wrote: [...] >> The TC9595 can drive an DP output, for that the clock which have to be >> set on the LCDIF cannot be predicted, as that information comes from the >> monitor EDID/DPCD. That is why the LCDIF has to be able to configure the >> Video PLL1 clock to accurate clock frequency. >> >> For the LVDS LDB, the use case is the other way around -- the pixel >> clock which should be generated by LCDIF and fed to LDB are known from >> the panel type listed in DT, but they should still be accurate. > > Thanks for the information. I think the key question is whether the > alternative solution(*) you mentioned below stands or not, in other words, > whether LCDIF1/LCDIF2/LDB drivers know that they are sharing a PLL > or not. I'll continue at the end ... >>> You still may assign an accurate PLL rate in DT. >>> This patch only makes the PLL rate be unchangeable dynamically in >>> runtime. That means the existing imx8m-dhcom-som.dtsi would use >>> IMX8MP_VIDEO_PLL1_OUT(running at 1.0395GHz) as the parent clock >>> of IMX8MP_CLK_MEDIA_DISP1_PIX (for LCDIF1/DSI), since it includes >>> imx8mp.dsti. I assume it should be able to support typical video modes >>> like 1080p60 video mode with 148.5MHz pixel clock at least with 1.0395GHz >>> PLL rate. >> >> This will break multiple DP monitors I tested so far I'm afraid. And I >> spent a LOT of time wrestling with the TC9595 bridge to make sure it >> actually does work well. > > If the DP monitors support typical video modes like 1080p60 with > 148.5MHz pixel clock rate, I assume these typical video modes work > still ok with this patch at least. Please help confirm this, since if the > alternative solution(*) doesn't stand, we would know those video > modes still work ok with my solution(fixed PLL rate). They do not work with the fixed PLL setting. >>> Granted that less video modes read from DP monitor would >>> be supported without dynamically changeable PLL rates, this is something >>> we have to accept because some i.MX8MP platforms(like i.MX8MP EVK) >>> have to share IMX8MP_VIDEO_PLL1_OUT between LVDS and MIPI DSI >>> display pipelines. >> >> What I need is the use of two full PLL1443x (like Video PLL and Audio >> PLL1/2) , one for each display output, and those PLLs have to be fully >> configurable to produce accurate pixel clock for each connected panel. >> Otherwise I cannot make proper use of the video output capabilities of >> the MX8MP SoC. > > Yeah, I understand your requirements. However, it still depends on > whether the alternative solution(*) stands or not. I'll continue at the end ... >>> The missing part is that we need to do mode validation >>> for the MIPI DSI display pipeline either in samsung-dsim.c or lcdif_kms.c >>> to filter unsupported video mode out. Is this missing mode validation >>> the cause of your failure case? >> >> I do want to support the various modes, I do not want to filter them >> out. They can be supported, the only "problem" is the shared Video PLL >> which is not really an actual problem in my case, because I do not use >> shared Video PLL, I use two separate PLLs. >> >> I think what is needed is for the LCDIF1/LCDIF2/LDB to figure out >> whether they share the Video PLL at all (you already suggested the clock >> subsystem can provide that information), and then if: > > But, how to let LCDIF1/LCDIF2/LDB drivers to figure out that? > > I didn't suggest that the clock subsystem can provide that information. ... by end I mean here. One really nasty way I can think of is -- use find_node_by_compatible(), look up all the relevant DT nodes, parse their clock properties, and check whether they all point to the Video PLL or not. Maybe the clock subsystem has a better way, like list "neighbor" consumers of some specific parent clock or something like that. [...] >> Can something like (*) above be implemented instead, so both Shared and >> separate PLLs would be supported ? That should solve both of our use >> cases, right ? > > I don't see any clear way to implement something like(*). > > Take the 3 i.MX8MP LCDIFs as one graphic card driven by one imx-lcdif > DRM instance? Would it be too intrusive? Yes, and I think unnecessary, one can simply traverse and parse the DT to determine the clock assignment? > Use clk_get_parent() to determine if the pixel clocks of LCDIF1&2 are > sharing PLL(note clk_get_parent() implementation contains a TODO: > Create a per-user clk.)? Maybe not necessary for this case. > How to do mode validation for the shared PLL case(note mode_valid() > callback is supposed to look at nothing more than passed-in mode)? > Use clk_set_rate_range() to fix the PLL rate(min == max)? This is a good question -- we can use fixed frequency set in DT for the PLL in case it is shared, and set whatever optimal frequency if the PLL is not shared. That would be a good first step I think (**). The next step would be to find a way to negotiate acceptable PLL frequency between LCDIF1/LCDIF2/LDB in case the PLL is shared, but I do agree this is non-trivial, hence next step. >>> I hope that we can agree on this solution first before spreading >>> discussions across different threads and eventually the NAK can be >>> taken back. >> >> I cannot really agree on a solution which breaks one of my use cases, >> but maybe there is an alternative how to support both options, see (*) >> above ? > > I tend to say there is no any alternative solution to satisfy both > separate PLLs and shared PLL use cases, or even if there is one, it won't > be easy to implement. If you know one, please shout it out. Maybe (*) with first step (**) would be doable ?
On 11/20/24, Marek Vasut wrote: > On 11/19/24 9:18 AM, Ying Liu wrote: > > [...] > > >> The TC9595 can drive an DP output, for that the clock which have to be > >> set on the LCDIF cannot be predicted, as that information comes from the > >> monitor EDID/DPCD. That is why the LCDIF has to be able to configure the > >> Video PLL1 clock to accurate clock frequency. > >> > >> For the LVDS LDB, the use case is the other way around -- the pixel > >> clock which should be generated by LCDIF and fed to LDB are known from > >> the panel type listed in DT, but they should still be accurate. > > > > Thanks for the information. I think the key question is whether the > > alternative solution(*) you mentioned below stands or not, in other words, > > whether LCDIF1/LCDIF2/LDB drivers know that they are sharing a PLL > > or not. > > I'll continue at the end ... > > >>> You still may assign an accurate PLL rate in DT. > >>> This patch only makes the PLL rate be unchangeable dynamically in > >>> runtime. That means the existing imx8m-dhcom-som.dtsi would use > >>> IMX8MP_VIDEO_PLL1_OUT(running at 1.0395GHz) as the parent clock > >>> of IMX8MP_CLK_MEDIA_DISP1_PIX (for LCDIF1/DSI), since it includes > >>> imx8mp.dsti. I assume it should be able to support typical video modes > >>> like 1080p60 video mode with 148.5MHz pixel clock at least with > 1.0395GHz > >>> PLL rate. > >> > >> This will break multiple DP monitors I tested so far I'm afraid. And I > >> spent a LOT of time wrestling with the TC9595 bridge to make sure it > >> actually does work well. > > > > If the DP monitors support typical video modes like 1080p60 with > > 148.5MHz pixel clock rate, I assume these typical video modes work > > still ok with this patch at least. Please help confirm this, since if the > > alternative solution(*) doesn't stand, we would know those video > > modes still work ok with my solution(fixed PLL rate). > > They do not work with the fixed PLL setting. Why? Did you assign a sensible fixed PLL rate in DT? Can you please compare clk_summary output for the failing cases before and after this patch is applied? I assume that if you use the fixed PLL rate same to the rate which works before this patch is applied, the typical video modes still just work after this patch is applied. > > >>> Granted that less video modes read from DP monitor would > >>> be supported without dynamically changeable PLL rates, this is > something > >>> we have to accept because some i.MX8MP platforms(like i.MX8MP EVK) > >>> have to share IMX8MP_VIDEO_PLL1_OUT between LVDS and MIPI DSI > >>> display pipelines. > >> > >> What I need is the use of two full PLL1443x (like Video PLL and Audio > >> PLL1/2) , one for each display output, and those PLLs have to be fully > >> configurable to produce accurate pixel clock for each connected panel. > >> Otherwise I cannot make proper use of the video output capabilities of > >> the MX8MP SoC. > > > > Yeah, I understand your requirements. However, it still depends on > > whether the alternative solution(*) stands or not. > > I'll continue at the end ... > > >>> The missing part is that we need to do mode validation > >>> for the MIPI DSI display pipeline either in samsung-dsim.c or lcdif_kms.c > >>> to filter unsupported video mode out. Is this missing mode validation > >>> the cause of your failure case? > >> > >> I do want to support the various modes, I do not want to filter them > >> out. They can be supported, the only "problem" is the shared Video PLL > >> which is not really an actual problem in my case, because I do not use > >> shared Video PLL, I use two separate PLLs. > >> > >> I think what is needed is for the LCDIF1/LCDIF2/LDB to figure out > >> whether they share the Video PLL at all (you already suggested the clock > >> subsystem can provide that information), and then if: > > > > But, how to let LCDIF1/LCDIF2/LDB drivers to figure out that? > > > > I didn't suggest that the clock subsystem can provide that information. > > ... by end I mean here. > > One really nasty way I can think of is -- use find_node_by_compatible(), > look up all the relevant DT nodes, parse their clock properties, and > check whether they all point to the Video PLL or not. That's nasty. It looks even more nasty when considering the fact that i.MX93 LCDIF is also driven by imx-lcdif DRM while only i.MX8MP LCDIF needs the nasty check, because i.MX93 SoC embeds only one LCDIF. > > Maybe the clock subsystem has a better way, like list "neighbor" > consumers of some specific parent clock or something like that. What will imx-lcdif DRM look like by using this way? Get the ancestor PLL clock of pixel clock(media_disp{1,2}_pix_root_clk), list all child clocks (media_disp1_pix and/or media_disp2_pix + other possible clocks) of the PLL clock in a string array and find media_disp1_pix + media_disp2_pix in it? Doesn't look nice, either. > > [...] > > >> Can something like (*) above be implemented instead, so both Shared > and > >> separate PLLs would be supported ? That should solve both of our use > >> cases, right ? > > > > I don't see any clear way to implement something like(*). > > > > Take the 3 i.MX8MP LCDIFs as one graphic card driven by one imx-lcdif > > DRM instance? Would it be too intrusive? > > Yes, and I think unnecessary, one can simply traverse and parse the DT > to determine the clock assignment? Yes, people can traverse and parse DT, but it's nasty. In addition, one may argue that now that CLK_SET_RATE_PARENT flag is set for the pixel clocks, all potential video modes read from EDID should be supported when only either LVDS display pipeline or MIPI DSI display pipeline is active in the shared PLL case. This requires one single DRM instance to detect single or dual active display pipelines dynamically, hence this single DRM instance becomes necessary. > > > Use clk_get_parent() to determine if the pixel clocks of LCDIF1&2 are > > sharing PLL(note clk_get_parent() implementation contains a TODO: > > Create a per-user clk.)? > > Maybe not necessary for this case. > > > How to do mode validation for the shared PLL case(note mode_valid() > > callback is supposed to look at nothing more than passed-in mode)? > > Use clk_set_rate_range() to fix the PLL rate(min == max)? > > This is a good question -- we can use fixed frequency set in DT for the > PLL in case it is shared, and set whatever optimal frequency if the PLL > is not shared. That would be a good first step I think (**). The above requirement of dynamical active display pipeline number detection defeats the fixed PLL rate for in the shared PLL case. And, it makes mode validation kind of undoable, because mode_valid() is not supposed to know that active number. > > The next step would be to find a way to negotiate acceptable PLL > frequency between LCDIF1/LCDIF2/LDB in case the PLL is shared, but I do > agree this is non-trivial, hence next step. > > >>> I hope that we can agree on this solution first before spreading > >>> discussions across different threads and eventually the NAK can be > >>> taken back. > >> > >> I cannot really agree on a solution which breaks one of my use cases, > >> but maybe there is an alternative how to support both options, see (*) > >> above ? > > > > I tend to say there is no any alternative solution to satisfy both > > separate PLLs and shared PLL use cases, or even if there is one, it won't > > be easy to implement. If you know one, please shout it out. > Maybe (*) with first step (**) would be doable ? Maybe it's not doable if the above requirement of dynamical active display pipeline number detection needs to be supported. Considering 1) the way of getting separate/shared PLL information and 2) mode validation, I don't think your overall alternative solution is good. Regards, Liu Ying
On 11/20/24 7:38 AM, Ying Liu wrote: [...] >>> If the DP monitors support typical video modes like 1080p60 with >>> 148.5MHz pixel clock rate, I assume these typical video modes work >>> still ok with this patch at least. Please help confirm this, since if the >>> alternative solution(*) doesn't stand, we would know those video >>> modes still work ok with my solution(fixed PLL rate). >> >> They do not work with the fixed PLL setting. > > Why? Did you assign a sensible fixed PLL rate in DT? Whatever was in imx8mp.dtsi does not really work for all the panels. Please keep in mind that the use case I have does not include only 1920x1080 "standard" panels, but also other resolutions. > Can you please compare clk_summary output for the failing cases > before and after this patch is applied? I assume that if you use > the fixed PLL rate same to the rate which works before this patch is > applied, the typical video modes still just work after this patch is > applied. I'm afraid I do not need to support only typical video modes, but also the other "atypical" modes. [...] >> One really nasty way I can think of is -- use find_node_by_compatible(), >> look up all the relevant DT nodes, parse their clock properties, and >> check whether they all point to the Video PLL or not. > > That's nasty. It looks even more nasty when considering the fact that > i.MX93 LCDIF is also driven by imx-lcdif DRM while only i.MX8MP LCDIF > needs the nasty check, because i.MX93 SoC embeds only one LCDIF. The check can be skipped based on compatible string. I agree it is nasty, but it is a start. Are there better ideas ? >> Maybe the clock subsystem has a better way, like list "neighbor" >> consumers of some specific parent clock or something like that. > > What will imx-lcdif DRM look like by using this way? Get the ancestor PLL > clock of pixel clock(media_disp{1,2}_pix_root_clk), list all child clocks > (media_disp1_pix and/or media_disp2_pix + other possible clocks) of the > PLL clock in a string array and find media_disp1_pix + media_disp2_pix > in it? > > Doesn't look nice, either. One other option came to my mind -- place a virtual clock between the Video PLL and consumers (LCDIF1/2/LDB), and then have the virtual clock driver do the clock rate negotiation in some .round_rate callback. That is also nasty, but it is another idea. If there is a clock specifically implemented to negotiate best upstream clock rate for all of its consumers, and it is aware of the consumer behavior details and requirements, maybe that could work ? >> [...] >> >>>> Can something like (*) above be implemented instead, so both Shared >> and >>>> separate PLLs would be supported ? That should solve both of our use >>>> cases, right ? >>> >>> I don't see any clear way to implement something like(*). >>> >>> Take the 3 i.MX8MP LCDIFs as one graphic card driven by one imx-lcdif >>> DRM instance? Would it be too intrusive? >> >> Yes, and I think unnecessary, one can simply traverse and parse the DT >> to determine the clock assignment? > > Yes, people can traverse and parse DT, but it's nasty. > > In addition, one may argue that now that CLK_SET_RATE_PARENT flag > is set for the pixel clocks, all potential video modes read from EDID > should be supported when only either LVDS display pipeline or MIPI DSI > display pipeline is active in the shared PLL case. This requires one > single DRM instance to detect single or dual active display pipelines > dynamically, hence this single DRM instance becomes necessary. Would single virtual clock which do the frequency negotiation between multiple DRM consumers work too ? I do not have much to add to the points below.
On 11/22/24, Marek Vasut wrote: > On 11/20/24 7:38 AM, Ying Liu wrote: > > [...] > > >>> If the DP monitors support typical video modes like 1080p60 with > >>> 148.5MHz pixel clock rate, I assume these typical video modes work > >>> still ok with this patch at least. Please help confirm this, since if the > >>> alternative solution(*) doesn't stand, we would know those video > >>> modes still work ok with my solution(fixed PLL rate). > >> > >> They do not work with the fixed PLL setting. > > > > Why? Did you assign a sensible fixed PLL rate in DT? > > Whatever was in imx8mp.dtsi does not really work for all the panels. > Please keep in mind that the use case I have does not include only > 1920x1080 "standard" panels, but also other resolutions. It looks like you are still sticking to the idea of supporting all potentially valid video modes by trying to find an "alternative" solution, while neglecting that the solution *could be* never working. > > > Can you please compare clk_summary output for the failing cases > > before and after this patch is applied? I assume that if you use > > the fixed PLL rate same to the rate which works before this patch is > > applied, the typical video modes still just work after this patch is > > applied. > > I'm afraid I do not need to support only typical video modes, but also > the other "atypical" modes. If the "alternative" solution doesn't work, we'll end up using the "fixed PLL rate" solution. It that case, some video modes would be filtered out as a sacrifice. > > [...] > > >> One really nasty way I can think of is -- use find_node_by_compatible(), > >> look up all the relevant DT nodes, parse their clock properties, and > >> check whether they all point to the Video PLL or not. > > > > That's nasty. It looks even more nasty when considering the fact that > > i.MX93 LCDIF is also driven by imx-lcdif DRM while only i.MX8MP LCDIF > > needs the nasty check, because i.MX93 SoC embeds only one LCDIF. > > The check can be skipped based on compatible string. > > I agree it is nasty, but it is a start. Are there better ideas ? No good idea from me. > > >> Maybe the clock subsystem has a better way, like list "neighbor" > >> consumers of some specific parent clock or something like that. > > > > What will imx-lcdif DRM look like by using this way? Get the ancestor PLL > > clock of pixel clock(media_disp{1,2}_pix_root_clk), list all child clocks > > (media_disp1_pix and/or media_disp2_pix + other possible clocks) of the > > PLL clock in a string array and find media_disp1_pix + media_disp2_pix > > in it? > > > > Doesn't look nice, either. > > One other option came to my mind -- place a virtual clock between the > Video PLL and consumers (LCDIF1/2/LDB), and then have the virtual clock > driver do the clock rate negotiation in some .round_rate callback. That > is also nasty, but it is another idea. If there is a clock specifically > implemented to negotiate best upstream clock rate for all of its > consumers, and it is aware of the consumer behavior details and > requirements, maybe that could work ? A mighty virtual clock? I'm not sure if that would work or not. > > >> [...] > >> > >>>> Can something like (*) above be implemented instead, so both Shared > >> and > >>>> separate PLLs would be supported ? That should solve both of our use > >>>> cases, right ? > >>> > >>> I don't see any clear way to implement something like(*). > >>> > >>> Take the 3 i.MX8MP LCDIFs as one graphic card driven by one imx-lcdif > >>> DRM instance? Would it be too intrusive? > >> > >> Yes, and I think unnecessary, one can simply traverse and parse the DT > >> to determine the clock assignment? > > > > Yes, people can traverse and parse DT, but it's nasty. > > > > In addition, one may argue that now that CLK_SET_RATE_PARENT flag > > is set for the pixel clocks, all potential video modes read from EDID > > should be supported when only either LVDS display pipeline or MIPI DSI > > display pipeline is active in the shared PLL case. This requires one > > single DRM instance to detect single or dual active display pipelines > > dynamically, hence this single DRM instance becomes necessary. > > Would single virtual clock which do the frequency negotiation between > multiple DRM consumers work too ? Not sure if it would work or not, but I'm sure that one single DRM instance means atomic check/commit for the display pipelines as a whole, hence awareness of active display pipeline number in an atomic way. > > I do not have much to add to the points below. Regards, Liu Ying
diff --git a/drivers/clk/imx/clk-imx8mp.c b/drivers/clk/imx/clk-imx8mp.c index 516dbd170c8a..e561ff7b135f 100644 --- a/drivers/clk/imx/clk-imx8mp.c +++ b/drivers/clk/imx/clk-imx8mp.c @@ -547,7 +547,7 @@ static int imx8mp_clocks_probe(struct platform_device *pdev) hws[IMX8MP_CLK_AHB] = imx8m_clk_hw_composite_bus_critical("ahb_root", imx8mp_ahb_sels, ccm_base + 0x9000); hws[IMX8MP_CLK_AUDIO_AHB] = imx8m_clk_hw_composite_bus("audio_ahb", imx8mp_audio_ahb_sels, ccm_base + 0x9100); hws[IMX8MP_CLK_MIPI_DSI_ESC_RX] = imx8m_clk_hw_composite_bus("mipi_dsi_esc_rx", imx8mp_mipi_dsi_esc_rx_sels, ccm_base + 0x9200); - hws[IMX8MP_CLK_MEDIA_DISP2_PIX] = imx8m_clk_hw_composite_bus_flags("media_disp2_pix", imx8mp_media_disp_pix_sels, ccm_base + 0x9300, CLK_SET_RATE_PARENT); + hws[IMX8MP_CLK_MEDIA_DISP2_PIX] = imx8m_clk_hw_composite_bus("media_disp2_pix", imx8mp_media_disp_pix_sels, ccm_base + 0x9300); hws[IMX8MP_CLK_IPG_ROOT] = imx_clk_hw_divider2("ipg_root", "ahb_root", ccm_base + 0x9080, 0, 1); @@ -609,7 +609,7 @@ static int imx8mp_clocks_probe(struct platform_device *pdev) hws[IMX8MP_CLK_USDHC3] = imx8m_clk_hw_composite("usdhc3", imx8mp_usdhc3_sels, ccm_base + 0xbc80); hws[IMX8MP_CLK_MEDIA_CAM1_PIX] = imx8m_clk_hw_composite("media_cam1_pix", imx8mp_media_cam1_pix_sels, ccm_base + 0xbd00); hws[IMX8MP_CLK_MEDIA_MIPI_PHY1_REF] = imx8m_clk_hw_composite("media_mipi_phy1_ref", imx8mp_media_mipi_phy1_ref_sels, ccm_base + 0xbd80); - hws[IMX8MP_CLK_MEDIA_DISP1_PIX] = imx8m_clk_hw_composite_bus_flags("media_disp1_pix", imx8mp_media_disp_pix_sels, ccm_base + 0xbe00, CLK_SET_RATE_PARENT); + hws[IMX8MP_CLK_MEDIA_DISP1_PIX] = imx8m_clk_hw_composite("media_disp1_pix", imx8mp_media_disp_pix_sels, ccm_base + 0xbe00); hws[IMX8MP_CLK_MEDIA_CAM2_PIX] = imx8m_clk_hw_composite("media_cam2_pix", imx8mp_media_cam2_pix_sels, ccm_base + 0xbe80); hws[IMX8MP_CLK_MEDIA_LDB] = imx8m_clk_hw_composite("media_ldb", imx8mp_media_ldb_sels, ccm_base + 0xbf00); hws[IMX8MP_CLK_MEMREPAIR] = imx8m_clk_hw_composite_critical("mem_repair", imx8mp_memrepair_sels, ccm_base + 0xbf80); diff --git a/drivers/clk/imx/clk.h b/drivers/clk/imx/clk.h index aa5202f284f3..adb7ad649a0d 100644 --- a/drivers/clk/imx/clk.h +++ b/drivers/clk/imx/clk.h @@ -442,10 +442,6 @@ struct clk_hw *__imx8m_clk_hw_composite(const char *name, _imx8m_clk_hw_composite(name, parent_names, reg, \ IMX_COMPOSITE_BUS, IMX_COMPOSITE_CLK_FLAGS_DEFAULT) -#define imx8m_clk_hw_composite_bus_flags(name, parent_names, reg, flags) \ - _imx8m_clk_hw_composite(name, parent_names, reg, \ - IMX_COMPOSITE_BUS, IMX_COMPOSITE_CLK_FLAGS_DEFAULT | flags) - #define imx8m_clk_hw_composite_bus_critical(name, parent_names, reg) \ _imx8m_clk_hw_composite(name, parent_names, reg, \ IMX_COMPOSITE_BUS, IMX_COMPOSITE_CLK_FLAGS_CRITICAL)
This reverts commit ff06ea04e4cf3ba2f025024776e83bfbdfa05155. media_disp1_pix clock is the pixel clock of the first i.MX8MP LCDIFv3 display controller, while media_disp2_pix clock is the pixel clock of the second i.MX8MP LCDIFv3 display controller. The two display controllers connect with Samsung MIPI DSI controller and LVDS Display Bridge(LDB) respectively. Since the two display controllers are driven by separate DRM driver instances and the two pixel clocks may be derived from the same video_pll1_out clock(sys_pll3_out clock could be already used to derive audio_axi clock), there is no way to negotiate a dynamically changeable video_pll1_out clock rate to satisfy both of the two display controllers. In this case, the only solution to drive them with the single video_pll1_out clock is to assign a sensible/unchangeable clock rate for video_pll1_out clock. Thus, there is no need to set the CLK_SET_RATE_PARENT flag for media_disp{1,2}_pix clocks, drop it then. Fixes: ff06ea04e4cf ("clk: imx: clk-imx8mp: Allow media_disp pixel clock reconfigure parent rate") Signed-off-by: Liu Ying <victor.liu@nxp.com> --- v7: * No change. v6: * New patch. drivers/clk/imx/clk-imx8mp.c | 4 ++-- drivers/clk/imx/clk.h | 4 ---- 2 files changed, 2 insertions(+), 6 deletions(-)