| Message ID | 20230415104104.5537-5-aford173@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/6] drm: bridge: samsung-dsim: Support multi-lane calculations | expand |
On 4/15/23 12:41, Adam Ford wrote: > The high-speed clock is hard-coded to the burst-clock > frequency specified in the device tree. However, when > using devices like certain bridge chips without burst mode > and varying resolutions and refresh rates, it may be > necessary to set the high-speed clock dynamically based > on the desired pixel clock for the connected device. The link rate negotiation should happen internally between the nearest bridge and DSIM, so please add that to DRM core instead of hacking around it by tweaking the HS clock again.
On Sun, Apr 16, 2023 at 5:13 PM Marek Vasut <marex@denx.de> wrote: > > On 4/15/23 12:41, Adam Ford wrote: > > The high-speed clock is hard-coded to the burst-clock > > frequency specified in the device tree. However, when > > using devices like certain bridge chips without burst mode > > and varying resolutions and refresh rates, it may be > > necessary to set the high-speed clock dynamically based > > on the desired pixel clock for the connected device. > > The link rate negotiation should happen internally between the nearest > bridge and DSIM, so please add that to DRM core instead of hacking > around it by tweaking the HS clock again. I thought you tried to add something like this before and had some resistance. The Pixel clock is set by the bridge already without any new code added to the DRM core.. I am just reading that value that's there, and setting the clock accordingly. I don't see how this is a hack. adam
On 4/17/23 13:57, Adam Ford wrote: > On Sun, Apr 16, 2023 at 5:13 PM Marek Vasut <marex@denx.de> wrote: >> >> On 4/15/23 12:41, Adam Ford wrote: >>> The high-speed clock is hard-coded to the burst-clock >>> frequency specified in the device tree. However, when >>> using devices like certain bridge chips without burst mode >>> and varying resolutions and refresh rates, it may be >>> necessary to set the high-speed clock dynamically based >>> on the desired pixel clock for the connected device. >> >> The link rate negotiation should happen internally between the nearest >> bridge and DSIM, so please add that to DRM core instead of hacking >> around it by tweaking the HS clock again. > > I thought you tried to add something like this before and had some resistance. Yes, all my attempts were rejected by a single reviewer. I suspended my efforts in that area for now. > The Pixel clock is set by the bridge already without any new code > added to the DRM core.. I am just reading that value that's there, > and setting the clock accordingly. I don't see how this is a hack. Assume you have a DSI-to-HDMI bridge attached to your DSIM bridge, it operates in non-burst mode, like ADV7533 . How would you configure the HS clock rate for such a bridge in DT ? (hint: you cannot, because the required clock comes from the EDID, which may not be available just yet)
On Mon, Apr 17, 2023 at 3:08 PM Marek Vasut <marex@denx.de> wrote: > > On 4/17/23 13:57, Adam Ford wrote: > > On Sun, Apr 16, 2023 at 5:13 PM Marek Vasut <marex@denx.de> wrote: > >> > >> On 4/15/23 12:41, Adam Ford wrote: > >>> The high-speed clock is hard-coded to the burst-clock > >>> frequency specified in the device tree. However, when > >>> using devices like certain bridge chips without burst mode > >>> and varying resolutions and refresh rates, it may be > >>> necessary to set the high-speed clock dynamically based > >>> on the desired pixel clock for the connected device. > >> > >> The link rate negotiation should happen internally between the nearest > >> bridge and DSIM, so please add that to DRM core instead of hacking > >> around it by tweaking the HS clock again. > > > > I thought you tried to add something like this before and had some resistance. > > Yes, all my attempts were rejected by a single reviewer. I suspended my > efforts in that area for now. > > > The Pixel clock is set by the bridge already without any new code > > added to the DRM core.. I am just reading that value that's there, > > and setting the clock accordingly. I don't see how this is a hack. > > Assume you have a DSI-to-HDMI bridge attached to your DSIM bridge, it > operates in non-burst mode, like ADV7533 . How would you configure the I have an ADV7535 > HS clock rate for such a bridge in DT ? (hint: you cannot, because the > required clock comes from the EDID, which may not be available just yet) The whole idea is that you wouldn't want to or need to configure the clock speed in the device tree because it comes from the EDID->bridge->DSI. I've tested this configuration on imx8mm, imx8mn, and imx8mp and I can change the resolution and refresh rate on the fly and the DSI will automatically readjust accordingly. If you fixed the clock in the device tree, you wouldn't be able to do that, and that was the point of this patch. adam
On 4/18/23 00:24, Adam Ford wrote: > On Mon, Apr 17, 2023 at 3:08 PM Marek Vasut <marex@denx.de> wrote: >> >> On 4/17/23 13:57, Adam Ford wrote: >>> On Sun, Apr 16, 2023 at 5:13 PM Marek Vasut <marex@denx.de> wrote: >>>> >>>> On 4/15/23 12:41, Adam Ford wrote: >>>>> The high-speed clock is hard-coded to the burst-clock >>>>> frequency specified in the device tree. However, when >>>>> using devices like certain bridge chips without burst mode >>>>> and varying resolutions and refresh rates, it may be >>>>> necessary to set the high-speed clock dynamically based >>>>> on the desired pixel clock for the connected device. >>>> >>>> The link rate negotiation should happen internally between the nearest >>>> bridge and DSIM, so please add that to DRM core instead of hacking >>>> around it by tweaking the HS clock again. >>> >>> I thought you tried to add something like this before and had some resistance. >> >> Yes, all my attempts were rejected by a single reviewer. I suspended my >> efforts in that area for now. >> >>> The Pixel clock is set by the bridge already without any new code >>> added to the DRM core.. I am just reading that value that's there, >>> and setting the clock accordingly. I don't see how this is a hack. >> >> Assume you have a DSI-to-HDMI bridge attached to your DSIM bridge, it >> operates in non-burst mode, like ADV7533 . How would you configure the > > I have an ADV7535 > >> HS clock rate for such a bridge in DT ? (hint: you cannot, because the >> required clock comes from the EDID, which may not be available just yet) > > The whole idea is that you wouldn't want to or need to configure the > clock speed in the device tree because it comes from the > EDID->bridge->DSI. > > I've tested this configuration on imx8mm, imx8mn, and imx8mp and I can > change the resolution and refresh rate on the fly and the DSI will > automatically readjust accordingly. If you fixed the clock in the > device tree, you wouldn't be able to do that, and that was the point > of this patch. Uh, I retract my comment, I was clearly confused here and we're talking about the same thing.
On Mon, Apr 17, 2023 at 6:23 PM Marek Vasut <marex@denx.de> wrote: > > On 4/18/23 00:24, Adam Ford wrote: > > On Mon, Apr 17, 2023 at 3:08 PM Marek Vasut <marex@denx.de> wrote: > >> > >> On 4/17/23 13:57, Adam Ford wrote: > >>> On Sun, Apr 16, 2023 at 5:13 PM Marek Vasut <marex@denx.de> wrote: > >>>> > >>>> On 4/15/23 12:41, Adam Ford wrote: > >>>>> The high-speed clock is hard-coded to the burst-clock > >>>>> frequency specified in the device tree. However, when > >>>>> using devices like certain bridge chips without burst mode > >>>>> and varying resolutions and refresh rates, it may be > >>>>> necessary to set the high-speed clock dynamically based > >>>>> on the desired pixel clock for the connected device. > >>>> > >>>> The link rate negotiation should happen internally between the nearest > >>>> bridge and DSIM, so please add that to DRM core instead of hacking > >>>> around it by tweaking the HS clock again. > >>> > >>> I thought you tried to add something like this before and had some resistance. > >> > >> Yes, all my attempts were rejected by a single reviewer. I suspended my > >> efforts in that area for now. > >> > >>> The Pixel clock is set by the bridge already without any new code > >>> added to the DRM core.. I am just reading that value that's there, > >>> and setting the clock accordingly. I don't see how this is a hack. > >> > >> Assume you have a DSI-to-HDMI bridge attached to your DSIM bridge, it > >> operates in non-burst mode, like ADV7533 . How would you configure the > > > > I have an ADV7535 > > > >> HS clock rate for such a bridge in DT ? (hint: you cannot, because the > >> required clock comes from the EDID, which may not be available just yet) > > > > The whole idea is that you wouldn't want to or need to configure the > > clock speed in the device tree because it comes from the > > EDID->bridge->DSI. > > > > I've tested this configuration on imx8mm, imx8mn, and imx8mp and I can > > change the resolution and refresh rate on the fly and the DSI will > > automatically readjust accordingly. If you fixed the clock in the > > device tree, you wouldn't be able to do that, and that was the point > > of this patch. > > Uh, I retract my comment, I was clearly confused here and we're talking > about the same thing. I'm working on a V2 for this series. Are you OK with this if I update the commit message a bit to make it more clear? adam
diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c index c48db27adafe..5aa3a44f15ec 100644 --- a/drivers/gpu/drm/bridge/samsung-dsim.c +++ b/drivers/gpu/drm/bridge/samsung-dsim.c @@ -659,11 +659,21 @@ static unsigned long samsung_dsim_set_pll(struct samsung_dsim *dsi, static int samsung_dsim_enable_clock(struct samsung_dsim *dsi) { - unsigned long hs_clk, byte_clk, esc_clk; + unsigned long hs_clk, byte_clk, esc_clk, pix_clk; unsigned long esc_div; u32 reg; + struct drm_display_mode *m = &dsi->mode; + int bpp = mipi_dsi_pixel_format_to_bpp(dsi->format); + + /* m->clock is in KHz */ + pix_clk = m->clock * 1000; + + /* Use burst_clk_rate for burst mode, otherwise use the pix_clk */ + if ((dsi->mode_flags & MIPI_DSI_MODE_VIDEO_BURST) && dsi->burst_clk_rate) + hs_clk = samsung_dsim_set_pll(dsi, dsi->burst_clk_rate); + else + hs_clk = samsung_dsim_set_pll(dsi, DIV_ROUND_UP(pix_clk * bpp, dsi->lanes)); - hs_clk = samsung_dsim_set_pll(dsi, dsi->burst_clk_rate); if (!hs_clk) { dev_err(dsi->dev, "failed to configure DSI PLL\n"); return -EFAULT;
The high-speed clock is hard-coded to the burst-clock frequency specified in the device tree. However, when using devices like certain bridge chips without burst mode and varying resolutions and refresh rates, it may be necessary to set the high-speed clock dynamically based on the desired pixel clock for the connected device. Signed-off-by: Adam Ford <aford173@gmail.com> --- drivers/gpu/drm/bridge/samsung-dsim.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)