| Message ID | 20241112020737.335297-2-marex@denx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/2] drm/bridge/panel: Add drm_bridge_get_panel to extract panel from last bridge | expand |
Hi Marek, Thank you for the patch. On Tue, Nov 12, 2024 at 03:05:37AM +0100, Marek Vasut wrote: > The Pixel PLL is not very capable and may come up with wildly inaccurate > clock. Since DPI panels are often tolerant to slightly higher pixel clock > without being operated outside of specification, calculate two Pixel PLL > from either mode clock or display_timing .pixelclock.max , whichever is > higher. Maybe this is a leftover from v1 in the commit message, but I don't think the code computes two pixel PLL. > Since the Pixel PLL output clock frequency calculation always > returns lower frequency than the requested clock frequency, passing in > the higher clock frequency should result in output clock frequency which > is closer to the expected pixel clock. Is that guaranteed ? > For the Chefree CH101 panel with 13 MHz Xtal input clock, the frequency > without this patch is 65 MHz which is out of the panel specification of > 68.9..73.4 MHz, while with this patch it is 71.5 MHz which is well within > the specification and far more accurate. I'm a bit concerned that this patch is quite a bit of a hack, but fixing the problem correctly would be too much yak shaving :-S > > Keep the change isolated to DPI output. > > Signed-off-by: Marek Vasut <marex@denx.de> > --- > Cc: Andrzej Hajda <andrzej.hajda@intel.com> > Cc: David Airlie <airlied@gmail.com> > Cc: Jernej Skrabec <jernej.skrabec@gmail.com> > Cc: Jonas Karlman <jonas@kwiboo.se> > Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Maxime Ripard <mripard@kernel.org> > Cc: Neil Armstrong <neil.armstrong@linaro.org> > Cc: Robert Foss <rfoss@kernel.org> > Cc: Simona Vetter <simona@ffwll.ch> > Cc: Thomas Zimmermann <tzimmermann@suse.de> > Cc: dri-devel@lists.freedesktop.org > --- > V2: - Isolate the change to DPI only, split tc_bridge_mode_set() > - Look up display_timings and use .pixelclock.max as input > into the PLL calculation if available. That should yield > more accurate results for DPI panels. > --- > drivers/gpu/drm/bridge/tc358767.c | 47 +++++++++++++++++++++++++------ > 1 file changed, 39 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c > index 0d523322fdd8e..fe9ab06d82d91 100644 > --- a/drivers/gpu/drm/bridge/tc358767.c > +++ b/drivers/gpu/drm/bridge/tc358767.c > @@ -39,6 +39,8 @@ > #include <drm/drm_print.h> > #include <drm/drm_probe_helper.h> > > +#include <video/display_timing.h> > + > /* Registers */ > > /* DSI D-PHY Layer registers */ > @@ -1681,13 +1683,33 @@ static int tc_dpi_atomic_check(struct drm_bridge *bridge, > struct drm_crtc_state *crtc_state, > struct drm_connector_state *conn_state) > { > + u32 mode_clock = crtc_state->mode.clock * 1000; > struct tc_data *tc = bridge_to_tc(bridge); > - int adjusted_clock = 0; > + struct drm_bridge *nb = bridge; > + struct display_timing timings; > + struct drm_panel *panel; > + int adjusted_clock; > int ret; > > + do { > + if (!drm_bridge_is_panel(nb)) drm_bridge_get_panel() already checks if the bridge is related to a panel, so I think you can drop this check. > + continue; > + > + panel = drm_bridge_get_panel(nb); > + if (!panel || !panel->funcs || !panel->funcs->get_timings) > + continue; > + > + ret = panel->funcs->get_timings(panel, 1, &timings); > + if (ret <= 0) > + break; > + > + if (timings.pixelclock.max > mode_clock) > + mode_clock = timings.pixelclock.max; > + break; > + } while ((nb = drm_bridge_get_next_bridge(nb))); Can the panel be anything that the last bridge in the chain ? > + > ret = tc_pxl_pll_calc(tc, clk_get_rate(tc->refclk), > - crtc_state->mode.clock * 1000, > - &adjusted_clock, NULL); > + mode_clock, &adjusted_clock, NULL); > if (ret) > return ret; > > @@ -1758,9 +1780,18 @@ tc_edp_mode_valid(struct drm_bridge *bridge, > return MODE_OK; > } > > -static void tc_bridge_mode_set(struct drm_bridge *bridge, > - const struct drm_display_mode *mode, > - const struct drm_display_mode *adj) > +static void tc_dpi_bridge_mode_set(struct drm_bridge *bridge, > + const struct drm_display_mode *mode, > + const struct drm_display_mode *adj) > +{ > + struct tc_data *tc = bridge_to_tc(bridge); > + > + drm_mode_copy(&tc->mode, adj); > +} > + > +static void tc_edp_bridge_mode_set(struct drm_bridge *bridge, > + const struct drm_display_mode *mode, > + const struct drm_display_mode *adj) > { > struct tc_data *tc = bridge_to_tc(bridge); > > @@ -1977,7 +2008,7 @@ tc_edp_atomic_get_output_bus_fmts(struct drm_bridge *bridge, > static const struct drm_bridge_funcs tc_dpi_bridge_funcs = { > .attach = tc_dpi_bridge_attach, > .mode_valid = tc_dpi_mode_valid, > - .mode_set = tc_bridge_mode_set, > + .mode_set = tc_dpi_bridge_mode_set, > .atomic_check = tc_dpi_atomic_check, > .atomic_enable = tc_dpi_bridge_atomic_enable, > .atomic_disable = tc_dpi_bridge_atomic_disable, > @@ -1991,7 +2022,7 @@ static const struct drm_bridge_funcs tc_edp_bridge_funcs = { > .attach = tc_edp_bridge_attach, > .detach = tc_edp_bridge_detach, > .mode_valid = tc_edp_mode_valid, > - .mode_set = tc_bridge_mode_set, > + .mode_set = tc_edp_bridge_mode_set, > .atomic_check = tc_edp_atomic_check, > .atomic_enable = tc_edp_bridge_atomic_enable, > .atomic_disable = tc_edp_bridge_atomic_disable,
On Tue, Nov 12, 2024 at 03:05:37AM +0100, Marek Vasut wrote: > The Pixel PLL is not very capable and may come up with wildly inaccurate > clock. Since DPI panels are often tolerant to slightly higher pixel clock > without being operated outside of specification, calculate two Pixel PLL > from either mode clock or display_timing .pixelclock.max , whichever is > higher. Since the Pixel PLL output clock frequency calculation always > returns lower frequency than the requested clock frequency, passing in > the higher clock frequency should result in output clock frequency which > is closer to the expected pixel clock. > > For the Chefree CH101 panel with 13 MHz Xtal input clock, the frequency > without this patch is 65 MHz which is out of the panel specification of > 68.9..73.4 MHz, while with this patch it is 71.5 MHz which is well within > the specification and far more accurate. Granted that most of the panel drivers do not implement get_timings() and granted that there are no current users of that interface, I think we should move away from it (and maybe even drop it completely from drm_panel). What about achieving the same via slightly different approach: register a non-preferred mode with the clock equal to the max clock allowed. The bridge driver can then use the default and the "max" mode to select PLL clock. I understand that this suggestion doesn't follow the DPI panel specifics, which are closer to the continuous timings rather than fixed set of modes, however I really don't think that it's worth keeping the interface for the sake of a single driver. Original commit 2938931f3732 ("drm/panel: Add display timing support") from 2014 mentions using those timings for .mode_fixup(), but I couldn't find a trace of the corresponding implementation. Another possible option might be to impletent adjusting modes in .atomic_check() / .mode_fixup(). > > Keep the change isolated to DPI output. > > Signed-off-by: Marek Vasut <marex@denx.de> > --- > Cc: Andrzej Hajda <andrzej.hajda@intel.com> > Cc: David Airlie <airlied@gmail.com> > Cc: Jernej Skrabec <jernej.skrabec@gmail.com> > Cc: Jonas Karlman <jonas@kwiboo.se> > Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Maxime Ripard <mripard@kernel.org> > Cc: Neil Armstrong <neil.armstrong@linaro.org> > Cc: Robert Foss <rfoss@kernel.org> > Cc: Simona Vetter <simona@ffwll.ch> > Cc: Thomas Zimmermann <tzimmermann@suse.de> > Cc: dri-devel@lists.freedesktop.org > --- > V2: - Isolate the change to DPI only, split tc_bridge_mode_set() > - Look up display_timings and use .pixelclock.max as input > into the PLL calculation if available. That should yield > more accurate results for DPI panels. > --- > drivers/gpu/drm/bridge/tc358767.c | 47 +++++++++++++++++++++++++------ > 1 file changed, 39 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c > index 0d523322fdd8e..fe9ab06d82d91 100644 > --- a/drivers/gpu/drm/bridge/tc358767.c > +++ b/drivers/gpu/drm/bridge/tc358767.c > @@ -39,6 +39,8 @@ > #include <drm/drm_print.h> > #include <drm/drm_probe_helper.h> > > +#include <video/display_timing.h> > + > /* Registers */ > > /* DSI D-PHY Layer registers */ > @@ -1681,13 +1683,33 @@ static int tc_dpi_atomic_check(struct drm_bridge *bridge, > struct drm_crtc_state *crtc_state, > struct drm_connector_state *conn_state) > { > + u32 mode_clock = crtc_state->mode.clock * 1000; > struct tc_data *tc = bridge_to_tc(bridge); > - int adjusted_clock = 0; > + struct drm_bridge *nb = bridge; > + struct display_timing timings; > + struct drm_panel *panel; > + int adjusted_clock; > int ret; > > + do { > + if (!drm_bridge_is_panel(nb)) > + continue; > + > + panel = drm_bridge_get_panel(nb); > + if (!panel || !panel->funcs || !panel->funcs->get_timings) > + continue; > + > + ret = panel->funcs->get_timings(panel, 1, &timings); > + if (ret <= 0) > + break; > + > + if (timings.pixelclock.max > mode_clock) > + mode_clock = timings.pixelclock.max; > + break; > + } while ((nb = drm_bridge_get_next_bridge(nb))); > + > ret = tc_pxl_pll_calc(tc, clk_get_rate(tc->refclk), > - crtc_state->mode.clock * 1000, > - &adjusted_clock, NULL); > + mode_clock, &adjusted_clock, NULL); > if (ret) > return ret; > > @@ -1758,9 +1780,18 @@ tc_edp_mode_valid(struct drm_bridge *bridge, > return MODE_OK; > } > > -static void tc_bridge_mode_set(struct drm_bridge *bridge, > - const struct drm_display_mode *mode, > - const struct drm_display_mode *adj) > +static void tc_dpi_bridge_mode_set(struct drm_bridge *bridge, > + const struct drm_display_mode *mode, > + const struct drm_display_mode *adj) > +{ > + struct tc_data *tc = bridge_to_tc(bridge); > + > + drm_mode_copy(&tc->mode, adj); > +} > + > +static void tc_edp_bridge_mode_set(struct drm_bridge *bridge, > + const struct drm_display_mode *mode, > + const struct drm_display_mode *adj) > { > struct tc_data *tc = bridge_to_tc(bridge); > > @@ -1977,7 +2008,7 @@ tc_edp_atomic_get_output_bus_fmts(struct drm_bridge *bridge, > static const struct drm_bridge_funcs tc_dpi_bridge_funcs = { > .attach = tc_dpi_bridge_attach, > .mode_valid = tc_dpi_mode_valid, > - .mode_set = tc_bridge_mode_set, > + .mode_set = tc_dpi_bridge_mode_set, > .atomic_check = tc_dpi_atomic_check, > .atomic_enable = tc_dpi_bridge_atomic_enable, > .atomic_disable = tc_dpi_bridge_atomic_disable, > @@ -1991,7 +2022,7 @@ static const struct drm_bridge_funcs tc_edp_bridge_funcs = { > .attach = tc_edp_bridge_attach, > .detach = tc_edp_bridge_detach, > .mode_valid = tc_edp_mode_valid, > - .mode_set = tc_bridge_mode_set, > + .mode_set = tc_edp_bridge_mode_set, > .atomic_check = tc_edp_atomic_check, > .atomic_enable = tc_edp_bridge_atomic_enable, > .atomic_disable = tc_edp_bridge_atomic_disable, > -- > 2.45.2 >
On Fri, Nov 22, 2024 at 03:32:57PM +0200, Dmitry Baryshkov wrote: > On Tue, Nov 12, 2024 at 03:05:37AM +0100, Marek Vasut wrote: > > The Pixel PLL is not very capable and may come up with wildly inaccurate > > clock. Since DPI panels are often tolerant to slightly higher pixel clock > > without being operated outside of specification, calculate two Pixel PLL > > from either mode clock or display_timing .pixelclock.max , whichever is > > higher. Since the Pixel PLL output clock frequency calculation always > > returns lower frequency than the requested clock frequency, passing in > > the higher clock frequency should result in output clock frequency which > > is closer to the expected pixel clock. > > > > For the Chefree CH101 panel with 13 MHz Xtal input clock, the frequency > > without this patch is 65 MHz which is out of the panel specification of > > 68.9..73.4 MHz, while with this patch it is 71.5 MHz which is well within > > the specification and far more accurate. > > Granted that most of the panel drivers do not implement get_timings() > and granted that there are no current users of that interface, I think > we should move away from it (and maybe even drop it completely from > drm_panel). I think we should do the opposite :) Panels usually have a pretty large operating window, and the current construct only works because nobody really uses the same panels (or we're hiding that behind different compatibles) across SoCs or generation. Or we're working around it. It's kind of a mess, and it gets messy in encoders too: some will filter out panel modes, some won't. Some will adjust timings to fit their clocks, some won't. etc. Going forward, switching everyone to a timing-like interface and providing a set of helpers to adjust timings within possible boundaries to fit a clock rate is doable and should be done imo. > What about achieving the same via slightly different approach: register > a non-preferred mode with the clock equal to the max clock allowed. The > bridge driver can then use the default and the "max" mode to select PLL > clock. > > I understand that this suggestion doesn't follow the DPI panel > specifics, which are closer to the continuous timings rather than fixed > set of modes, however I really don't think that it's worth keeping the > interface for the sake of a single driver. Original commit 2938931f3732 > ("drm/panel: Add display timing support") from 2014 mentions using those > timings for .mode_fixup(), but I couldn't find a trace of the > corresponding implementation. > > Another possible option might be to impletent adjusting modes in > .atomic_check() / .mode_fixup(). It's unused because we don't have an easy API for encoders to use it. We should fix *that*. Maxime
On Mon, 25 Nov 2024 at 15:17, Maxime Ripard <mripard@kernel.org> wrote: > > On Fri, Nov 22, 2024 at 03:32:57PM +0200, Dmitry Baryshkov wrote: > > On Tue, Nov 12, 2024 at 03:05:37AM +0100, Marek Vasut wrote: > > > The Pixel PLL is not very capable and may come up with wildly inaccurate > > > clock. Since DPI panels are often tolerant to slightly higher pixel clock > > > without being operated outside of specification, calculate two Pixel PLL > > > from either mode clock or display_timing .pixelclock.max , whichever is > > > higher. Since the Pixel PLL output clock frequency calculation always > > > returns lower frequency than the requested clock frequency, passing in > > > the higher clock frequency should result in output clock frequency which > > > is closer to the expected pixel clock. > > > > > > For the Chefree CH101 panel with 13 MHz Xtal input clock, the frequency > > > without this patch is 65 MHz which is out of the panel specification of > > > 68.9..73.4 MHz, while with this patch it is 71.5 MHz which is well within > > > the specification and far more accurate. > > > > Granted that most of the panel drivers do not implement get_timings() > > and granted that there are no current users of that interface, I think > > we should move away from it (and maybe even drop it completely from > > drm_panel). > > I think we should do the opposite :) > > Panels usually have a pretty large operating window, and the current > construct only works because nobody really uses the same panels (or > we're hiding that behind different compatibles) across SoCs or > generation. Or we're working around it. > > It's kind of a mess, and it gets messy in encoders too: some will filter > out panel modes, some won't. Some will adjust timings to fit their > clocks, some won't. etc. Well... I think it's messy because we have so many different interfaces. Some encoders can poke directly into the panel, some drivers use bridge chains and panel bridge. Some do even a messier thing and try both at the same time. I think that in the long-term nobody should be using the drm_panel interface directly. > > Going forward, switching everyone to a timing-like interface and > providing a set of helpers to adjust timings within possible boundaries > to fit a clock rate is doable and should be done imo. ... and then it might help with the command-mode DSI panels with DSC... > > > What about achieving the same via slightly different approach: register > > a non-preferred mode with the clock equal to the max clock allowed. The > > bridge driver can then use the default and the "max" mode to select PLL > > clock. > > > > I understand that this suggestion doesn't follow the DPI panel > > specifics, which are closer to the continuous timings rather than fixed > > set of modes, however I really don't think that it's worth keeping the > > interface for the sake of a single driver. Original commit 2938931f3732 > > ("drm/panel: Add display timing support") from 2014 mentions using those > > timings for .mode_fixup(), but I couldn't find a trace of the > > corresponding implementation. > > > > Another possible option might be to impletent adjusting modes in > > .atomic_check() / .mode_fixup(). > > It's unused because we don't have an easy API for encoders to use it. We > should fix *that*. Sounds good to me too. I'm not sure what it should look like though. We barely scratched our heads when looking at the DSC / CMD stuff, but nothing came out of it. Ideally... there should be some kind of get_timings being available through the full bridge chain, so that the encoders can use it. But I'm not sure how that should work, because some bridges would like to manipulate those timings. And some bridges will completely drop get_timings() and produce raw modes even after consuming the timings set.
On 11/22/24 2:32 PM, Dmitry Baryshkov wrote: > On Tue, Nov 12, 2024 at 03:05:37AM +0100, Marek Vasut wrote: >> The Pixel PLL is not very capable and may come up with wildly inaccurate >> clock. Since DPI panels are often tolerant to slightly higher pixel clock >> without being operated outside of specification, calculate two Pixel PLL >> from either mode clock or display_timing .pixelclock.max , whichever is >> higher. Since the Pixel PLL output clock frequency calculation always >> returns lower frequency than the requested clock frequency, passing in >> the higher clock frequency should result in output clock frequency which >> is closer to the expected pixel clock. >> >> For the Chefree CH101 panel with 13 MHz Xtal input clock, the frequency >> without this patch is 65 MHz which is out of the panel specification of >> 68.9..73.4 MHz, while with this patch it is 71.5 MHz which is well within >> the specification and far more accurate. > > Granted that most of the panel drivers do not implement get_timings() > and granted that there are no current users of that interface, I think > we should move away from it (and maybe even drop it completely from > drm_panel). It does fit DPI and LVDS panels and their descriptions in datasheets the best. > What about achieving the same via slightly different approach: register > a non-preferred mode with the clock equal to the max clock allowed. The > bridge driver can then use the default and the "max" mode to select PLL > clock. > > I understand that this suggestion doesn't follow the DPI panel > specifics, which are closer to the continuous timings rather than fixed > set of modes, however I really don't think that it's worth keeping the > interface for the sake of a single driver. Original commit 2938931f3732 > ("drm/panel: Add display timing support") from 2014 mentions using those > timings for .mode_fixup(), but I couldn't find a trace of the > corresponding implementation. > > Another possible option might be to impletent adjusting modes in > .atomic_check() / .mode_fixup(). Something like this ? static const struct display_timing chefree_ch101olhlwh_002_timing = { .pixelclock = { 68900000, 71100000, 73400000 }, ... }; static const struct panel_desc chefree_ch101olhlwh_002 = { .timings = &chefree_ch101olhlwh_002_timing, .num_timings = 1, ... }; ... would turn into ... static const struct drm_display_mode chefree_ch101olhlwh_002_mode[3] = { { .clock = 68900000, ... }, { .clock = 71100000, ... }, { .clock = 73400000, ... } }; static const struct panel_desc chefree_ch101olhlwh_002 = { .modes = &chefree_ch101olhlwh_002_mode, .num_timings = 3, ... }; ? Maybe some mode flag to specify which mode is MIN/TYP/MAX would be useful with that change too ? Finally, the TC358767 driver would test all modes and find the most accurate one ?
On 11/25/24 2:17 PM, Maxime Ripard wrote: > On Fri, Nov 22, 2024 at 03:32:57PM +0200, Dmitry Baryshkov wrote: >> On Tue, Nov 12, 2024 at 03:05:37AM +0100, Marek Vasut wrote: >>> The Pixel PLL is not very capable and may come up with wildly inaccurate >>> clock. Since DPI panels are often tolerant to slightly higher pixel clock >>> without being operated outside of specification, calculate two Pixel PLL >>> from either mode clock or display_timing .pixelclock.max , whichever is >>> higher. Since the Pixel PLL output clock frequency calculation always >>> returns lower frequency than the requested clock frequency, passing in >>> the higher clock frequency should result in output clock frequency which >>> is closer to the expected pixel clock. >>> >>> For the Chefree CH101 panel with 13 MHz Xtal input clock, the frequency >>> without this patch is 65 MHz which is out of the panel specification of >>> 68.9..73.4 MHz, while with this patch it is 71.5 MHz which is well within >>> the specification and far more accurate. >> >> Granted that most of the panel drivers do not implement get_timings() >> and granted that there are no current users of that interface, I think >> we should move away from it (and maybe even drop it completely from >> drm_panel). > > I think we should do the opposite :) > > Panels usually have a pretty large operating window, and the current > construct only works because nobody really uses the same panels (or > we're hiding that behind different compatibles) across SoCs or > generation. Or we're working around it. I am using the same panels on NXP MX6/MX8 and STM32MP1 with wildly different pipeline setups too. > It's kind of a mess, and it gets messy in encoders too: some will filter > out panel modes, some won't. Some will adjust timings to fit their > clocks, some won't. etc. > > Going forward, switching everyone to a timing-like interface and > providing a set of helpers to adjust timings within possible boundaries > to fit a clock rate is doable and should be done imo. Maybe it would be better to have multiple modes and mark them with MIN/TYP/MAX flag , so at least there wouldn't be two sets of structures describing the same thing, but only one structure (drm_display_mode) ?
On Tue, Nov 26, 2024 at 12:48:20AM +0100, Marek Vasut wrote: > On 11/22/24 2:32 PM, Dmitry Baryshkov wrote: > > On Tue, Nov 12, 2024 at 03:05:37AM +0100, Marek Vasut wrote: > > > The Pixel PLL is not very capable and may come up with wildly inaccurate > > > clock. Since DPI panels are often tolerant to slightly higher pixel clock > > > without being operated outside of specification, calculate two Pixel PLL > > > from either mode clock or display_timing .pixelclock.max , whichever is > > > higher. Since the Pixel PLL output clock frequency calculation always > > > returns lower frequency than the requested clock frequency, passing in > > > the higher clock frequency should result in output clock frequency which > > > is closer to the expected pixel clock. > > > > > > For the Chefree CH101 panel with 13 MHz Xtal input clock, the frequency > > > without this patch is 65 MHz which is out of the panel specification of > > > 68.9..73.4 MHz, while with this patch it is 71.5 MHz which is well within > > > the specification and far more accurate. > > > > Granted that most of the panel drivers do not implement get_timings() > > and granted that there are no current users of that interface, I think > > we should move away from it (and maybe even drop it completely from > > drm_panel). > > It does fit DPI and LVDS panels and their descriptions in datasheets the > best. > > > What about achieving the same via slightly different approach: register > > a non-preferred mode with the clock equal to the max clock allowed. The > > bridge driver can then use the default and the "max" mode to select PLL > > clock. > > > > I understand that this suggestion doesn't follow the DPI panel > > specifics, which are closer to the continuous timings rather than fixed > > set of modes, however I really don't think that it's worth keeping the > > interface for the sake of a single driver. Original commit 2938931f3732 > > ("drm/panel: Add display timing support") from 2014 mentions using those > > timings for .mode_fixup(), but I couldn't find a trace of the > > corresponding implementation. > > > > Another possible option might be to impletent adjusting modes in > > .atomic_check() / .mode_fixup(). > Something like this ? > > static const struct display_timing chefree_ch101olhlwh_002_timing = { > .pixelclock = { 68900000, 71100000, 73400000 }, > ... > }; > > static const struct panel_desc chefree_ch101olhlwh_002 = { > .timings = &chefree_ch101olhlwh_002_timing, > .num_timings = 1, > ... > }; > > ... would turn into ... > > static const struct drm_display_mode chefree_ch101olhlwh_002_mode[3] = { > { > .clock = 68900000, > ... > }, { > .clock = 71100000, > ... > }, { > .clock = 73400000, > ... > } > }; > > static const struct panel_desc chefree_ch101olhlwh_002 = { > .modes = &chefree_ch101olhlwh_002_mode, > .num_timings = 3, > ... > }; > > ? Except that doesn't work if you want to keep your driver at the expected framerate. To reduce the pixel clock, you also need to reduce the blanking period within the acceptable boundaries if you want to keep the same framerate. Maxime
On Tue, Nov 26, 2024 at 12:07:10AM +0200, Dmitry Baryshkov wrote: > On Mon, 25 Nov 2024 at 15:17, Maxime Ripard <mripard@kernel.org> wrote: > > > > On Fri, Nov 22, 2024 at 03:32:57PM +0200, Dmitry Baryshkov wrote: > > > On Tue, Nov 12, 2024 at 03:05:37AM +0100, Marek Vasut wrote: > > > > The Pixel PLL is not very capable and may come up with wildly inaccurate > > > > clock. Since DPI panels are often tolerant to slightly higher pixel clock > > > > without being operated outside of specification, calculate two Pixel PLL > > > > from either mode clock or display_timing .pixelclock.max , whichever is > > > > higher. Since the Pixel PLL output clock frequency calculation always > > > > returns lower frequency than the requested clock frequency, passing in > > > > the higher clock frequency should result in output clock frequency which > > > > is closer to the expected pixel clock. > > > > > > > > For the Chefree CH101 panel with 13 MHz Xtal input clock, the frequency > > > > without this patch is 65 MHz which is out of the panel specification of > > > > 68.9..73.4 MHz, while with this patch it is 71.5 MHz which is well within > > > > the specification and far more accurate. > > > > > > Granted that most of the panel drivers do not implement get_timings() > > > and granted that there are no current users of that interface, I think > > > we should move away from it (and maybe even drop it completely from > > > drm_panel). > > > > I think we should do the opposite :) > > > > Panels usually have a pretty large operating window, and the current > > construct only works because nobody really uses the same panels (or > > we're hiding that behind different compatibles) across SoCs or > > generation. Or we're working around it. > > > > It's kind of a mess, and it gets messy in encoders too: some will filter > > out panel modes, some won't. Some will adjust timings to fit their > > clocks, some won't. etc. > > Well... I think it's messy because we have so many different > interfaces. Some encoders can poke directly into the panel, some > drivers use bridge chains and panel bridge. Some do even a messier > thing and try both at the same time. > I think that in the long-term nobody should be using the drm_panel > interface directly. There's a few corner cases that are doable with the panel API that aren't possible with the bridge API still. Being able to get the pixel clock you're going to get from the encoder and adjust the timings to match the panel tolerance is one for example, and we have a couple of drivers doing that. > > Going forward, switching everyone to a timing-like interface and > > providing a set of helpers to adjust timings within possible boundaries > > to fit a clock rate is doable and should be done imo. > > ... and then it might help with the command-mode DSI panels with DSC... How so? > > > > > What about achieving the same via slightly different approach: register > > > a non-preferred mode with the clock equal to the max clock allowed. The > > > bridge driver can then use the default and the "max" mode to select PLL > > > clock. > > > > > > I understand that this suggestion doesn't follow the DPI panel > > > specifics, which are closer to the continuous timings rather than fixed > > > set of modes, however I really don't think that it's worth keeping the > > > interface for the sake of a single driver. Original commit 2938931f3732 > > > ("drm/panel: Add display timing support") from 2014 mentions using those > > > timings for .mode_fixup(), but I couldn't find a trace of the > > > corresponding implementation. > > > > > > Another possible option might be to impletent adjusting modes in > > > .atomic_check() / .mode_fixup(). > > > > It's unused because we don't have an easy API for encoders to use it. We > > should fix *that*. > > Sounds good to me too. > I'm not sure what it should look like though. We barely scratched our > heads when looking at the DSC / CMD stuff, but nothing came out of it. > > Ideally... there should be some kind of get_timings being available > through the full bridge chain, so that the encoders can use it. But > I'm not sure how that should work, because some bridges would like to > manipulate those timings. And some bridges will completely drop > get_timings() and produce raw modes even after consuming the timings > set. the drm_display_mode -> timings conversion is fairly easy to do: just use the same values for min, typ and max. Maxime
On 11/26/24 4:56 PM, Maxime Ripard wrote: > On Tue, Nov 26, 2024 at 12:48:20AM +0100, Marek Vasut wrote: >> On 11/22/24 2:32 PM, Dmitry Baryshkov wrote: >>> On Tue, Nov 12, 2024 at 03:05:37AM +0100, Marek Vasut wrote: >>>> The Pixel PLL is not very capable and may come up with wildly inaccurate >>>> clock. Since DPI panels are often tolerant to slightly higher pixel clock >>>> without being operated outside of specification, calculate two Pixel PLL >>>> from either mode clock or display_timing .pixelclock.max , whichever is >>>> higher. Since the Pixel PLL output clock frequency calculation always >>>> returns lower frequency than the requested clock frequency, passing in >>>> the higher clock frequency should result in output clock frequency which >>>> is closer to the expected pixel clock. >>>> >>>> For the Chefree CH101 panel with 13 MHz Xtal input clock, the frequency >>>> without this patch is 65 MHz which is out of the panel specification of >>>> 68.9..73.4 MHz, while with this patch it is 71.5 MHz which is well within >>>> the specification and far more accurate. >>> >>> Granted that most of the panel drivers do not implement get_timings() >>> and granted that there are no current users of that interface, I think >>> we should move away from it (and maybe even drop it completely from >>> drm_panel). >> >> It does fit DPI and LVDS panels and their descriptions in datasheets the >> best. >> >>> What about achieving the same via slightly different approach: register >>> a non-preferred mode with the clock equal to the max clock allowed. The >>> bridge driver can then use the default and the "max" mode to select PLL >>> clock. >>> >>> I understand that this suggestion doesn't follow the DPI panel >>> specifics, which are closer to the continuous timings rather than fixed >>> set of modes, however I really don't think that it's worth keeping the >>> interface for the sake of a single driver. Original commit 2938931f3732 >>> ("drm/panel: Add display timing support") from 2014 mentions using those >>> timings for .mode_fixup(), but I couldn't find a trace of the >>> corresponding implementation. >>> >>> Another possible option might be to impletent adjusting modes in >>> .atomic_check() / .mode_fixup(). >> Something like this ? >> >> static const struct display_timing chefree_ch101olhlwh_002_timing = { >> .pixelclock = { 68900000, 71100000, 73400000 }, >> ... >> }; >> >> static const struct panel_desc chefree_ch101olhlwh_002 = { >> .timings = &chefree_ch101olhlwh_002_timing, >> .num_timings = 1, >> ... >> }; >> >> ... would turn into ... >> >> static const struct drm_display_mode chefree_ch101olhlwh_002_mode[3] = { >> { >> .clock = 68900000, >> ... >> }, { >> .clock = 71100000, >> ... >> }, { >> .clock = 73400000, >> ... >> } >> }; >> >> static const struct panel_desc chefree_ch101olhlwh_002 = { >> .modes = &chefree_ch101olhlwh_002_mode, >> .num_timings = 3, >> ... >> }; >> >> ? > > Except that doesn't work if you want to keep your driver at the expected > framerate. To reduce the pixel clock, you also need to reduce the > blanking period within the acceptable boundaries if you want to keep the > same framerate. Each mode entry would have to have its own full set of timings of course, not just .clock . Those can be converted from display_timing just like clock.
On Tue, 26 Nov 2024 at 18:00, Maxime Ripard <mripard@kernel.org> wrote: > > On Tue, Nov 26, 2024 at 12:07:10AM +0200, Dmitry Baryshkov wrote: > > On Mon, 25 Nov 2024 at 15:17, Maxime Ripard <mripard@kernel.org> wrote: > > > > > > On Fri, Nov 22, 2024 at 03:32:57PM +0200, Dmitry Baryshkov wrote: > > > > On Tue, Nov 12, 2024 at 03:05:37AM +0100, Marek Vasut wrote: > > > > > The Pixel PLL is not very capable and may come up with wildly inaccurate > > > > > clock. Since DPI panels are often tolerant to slightly higher pixel clock > > > > > without being operated outside of specification, calculate two Pixel PLL > > > > > from either mode clock or display_timing .pixelclock.max , whichever is > > > > > higher. Since the Pixel PLL output clock frequency calculation always > > > > > returns lower frequency than the requested clock frequency, passing in > > > > > the higher clock frequency should result in output clock frequency which > > > > > is closer to the expected pixel clock. > > > > > > > > > > For the Chefree CH101 panel with 13 MHz Xtal input clock, the frequency > > > > > without this patch is 65 MHz which is out of the panel specification of > > > > > 68.9..73.4 MHz, while with this patch it is 71.5 MHz which is well within > > > > > the specification and far more accurate. > > > > > > > > Granted that most of the panel drivers do not implement get_timings() > > > > and granted that there are no current users of that interface, I think > > > > we should move away from it (and maybe even drop it completely from > > > > drm_panel). > > > > > > I think we should do the opposite :) > > > > > > Panels usually have a pretty large operating window, and the current > > > construct only works because nobody really uses the same panels (or > > > we're hiding that behind different compatibles) across SoCs or > > > generation. Or we're working around it. > > > > > > It's kind of a mess, and it gets messy in encoders too: some will filter > > > out panel modes, some won't. Some will adjust timings to fit their > > > clocks, some won't. etc. > > > > Well... I think it's messy because we have so many different > > interfaces. Some encoders can poke directly into the panel, some > > drivers use bridge chains and panel bridge. Some do even a messier > > thing and try both at the same time. > > I think that in the long-term nobody should be using the drm_panel > > interface directly. > > There's a few corner cases that are doable with the panel API that > aren't possible with the bridge API still. Being able to get the pixel > clock you're going to get from the encoder and adjust the timings to > match the panel tolerance is one for example, and we have a couple of > drivers doing that. My initial point was that only panel-simple (and panel-edp) implement the get_timings(). And for the existing encoder drivers I think we should be migrating to using bridge API instead of panel API. > > > > Going forward, switching everyone to a timing-like interface and > > > providing a set of helpers to adjust timings within possible boundaries > > > to fit a clock rate is doable and should be done imo. > > > > ... and then it might help with the command-mode DSI panels with DSC... > > How so? We had troubles with CMD panels, because we need to get the mode timings based on FPS and compression ratio (so it can not be pregrorammed, like we usually do for most of the panels). > > > > > > > > What about achieving the same via slightly different approach: register > > > > a non-preferred mode with the clock equal to the max clock allowed. The > > > > bridge driver can then use the default and the "max" mode to select PLL > > > > clock. > > > > > > > > I understand that this suggestion doesn't follow the DPI panel > > > > specifics, which are closer to the continuous timings rather than fixed > > > > set of modes, however I really don't think that it's worth keeping the > > > > interface for the sake of a single driver. Original commit 2938931f3732 > > > > ("drm/panel: Add display timing support") from 2014 mentions using those > > > > timings for .mode_fixup(), but I couldn't find a trace of the > > > > corresponding implementation. > > > > > > > > Another possible option might be to impletent adjusting modes in > > > > .atomic_check() / .mode_fixup(). > > > > > > It's unused because we don't have an easy API for encoders to use it. We > > > should fix *that*. > > > > Sounds good to me too. > > I'm not sure what it should look like though. We barely scratched our > > heads when looking at the DSC / CMD stuff, but nothing came out of it. > > > > Ideally... there should be some kind of get_timings being available > > through the full bridge chain, so that the encoders can use it. But > > I'm not sure how that should work, because some bridges would like to > > manipulate those timings. And some bridges will completely drop > > get_timings() and produce raw modes even after consuming the timings > > set. > > the drm_display_mode -> timings conversion is fairly easy to do: just > use the same values for min, typ and max.
diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c index 0d523322fdd8e..fe9ab06d82d91 100644 --- a/drivers/gpu/drm/bridge/tc358767.c +++ b/drivers/gpu/drm/bridge/tc358767.c @@ -39,6 +39,8 @@ #include <drm/drm_print.h> #include <drm/drm_probe_helper.h> +#include <video/display_timing.h> + /* Registers */ /* DSI D-PHY Layer registers */ @@ -1681,13 +1683,33 @@ static int tc_dpi_atomic_check(struct drm_bridge *bridge, struct drm_crtc_state *crtc_state, struct drm_connector_state *conn_state) { + u32 mode_clock = crtc_state->mode.clock * 1000; struct tc_data *tc = bridge_to_tc(bridge); - int adjusted_clock = 0; + struct drm_bridge *nb = bridge; + struct display_timing timings; + struct drm_panel *panel; + int adjusted_clock; int ret; + do { + if (!drm_bridge_is_panel(nb)) + continue; + + panel = drm_bridge_get_panel(nb); + if (!panel || !panel->funcs || !panel->funcs->get_timings) + continue; + + ret = panel->funcs->get_timings(panel, 1, &timings); + if (ret <= 0) + break; + + if (timings.pixelclock.max > mode_clock) + mode_clock = timings.pixelclock.max; + break; + } while ((nb = drm_bridge_get_next_bridge(nb))); + ret = tc_pxl_pll_calc(tc, clk_get_rate(tc->refclk), - crtc_state->mode.clock * 1000, - &adjusted_clock, NULL); + mode_clock, &adjusted_clock, NULL); if (ret) return ret; @@ -1758,9 +1780,18 @@ tc_edp_mode_valid(struct drm_bridge *bridge, return MODE_OK; } -static void tc_bridge_mode_set(struct drm_bridge *bridge, - const struct drm_display_mode *mode, - const struct drm_display_mode *adj) +static void tc_dpi_bridge_mode_set(struct drm_bridge *bridge, + const struct drm_display_mode *mode, + const struct drm_display_mode *adj) +{ + struct tc_data *tc = bridge_to_tc(bridge); + + drm_mode_copy(&tc->mode, adj); +} + +static void tc_edp_bridge_mode_set(struct drm_bridge *bridge, + const struct drm_display_mode *mode, + const struct drm_display_mode *adj) { struct tc_data *tc = bridge_to_tc(bridge); @@ -1977,7 +2008,7 @@ tc_edp_atomic_get_output_bus_fmts(struct drm_bridge *bridge, static const struct drm_bridge_funcs tc_dpi_bridge_funcs = { .attach = tc_dpi_bridge_attach, .mode_valid = tc_dpi_mode_valid, - .mode_set = tc_bridge_mode_set, + .mode_set = tc_dpi_bridge_mode_set, .atomic_check = tc_dpi_atomic_check, .atomic_enable = tc_dpi_bridge_atomic_enable, .atomic_disable = tc_dpi_bridge_atomic_disable, @@ -1991,7 +2022,7 @@ static const struct drm_bridge_funcs tc_edp_bridge_funcs = { .attach = tc_edp_bridge_attach, .detach = tc_edp_bridge_detach, .mode_valid = tc_edp_mode_valid, - .mode_set = tc_bridge_mode_set, + .mode_set = tc_edp_bridge_mode_set, .atomic_check = tc_edp_atomic_check, .atomic_enable = tc_edp_bridge_atomic_enable, .atomic_disable = tc_edp_bridge_atomic_disable,
The Pixel PLL is not very capable and may come up with wildly inaccurate clock. Since DPI panels are often tolerant to slightly higher pixel clock without being operated outside of specification, calculate two Pixel PLL from either mode clock or display_timing .pixelclock.max , whichever is higher. Since the Pixel PLL output clock frequency calculation always returns lower frequency than the requested clock frequency, passing in the higher clock frequency should result in output clock frequency which is closer to the expected pixel clock. For the Chefree CH101 panel with 13 MHz Xtal input clock, the frequency without this patch is 65 MHz which is out of the panel specification of 68.9..73.4 MHz, while with this patch it is 71.5 MHz which is well within the specification and far more accurate. Keep the change isolated to DPI output. Signed-off-by: Marek Vasut <marex@denx.de> --- Cc: Andrzej Hajda <andrzej.hajda@intel.com> Cc: David Airlie <airlied@gmail.com> Cc: Jernej Skrabec <jernej.skrabec@gmail.com> Cc: Jonas Karlman <jonas@kwiboo.se> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Cc: Maxime Ripard <mripard@kernel.org> Cc: Neil Armstrong <neil.armstrong@linaro.org> Cc: Robert Foss <rfoss@kernel.org> Cc: Simona Vetter <simona@ffwll.ch> Cc: Thomas Zimmermann <tzimmermann@suse.de> Cc: dri-devel@lists.freedesktop.org --- V2: - Isolate the change to DPI only, split tc_bridge_mode_set() - Look up display_timings and use .pixelclock.max as input into the PLL calculation if available. That should yield more accurate results for DPI panels. --- drivers/gpu/drm/bridge/tc358767.c | 47 +++++++++++++++++++++++++------ 1 file changed, 39 insertions(+), 8 deletions(-)