| Message ID | 20190215050957.20755-7-anarsoul@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Analogix ANX6345 RGB-(e)DP bridge support | expand |
On Thu, Feb 14, 2019 at 09:09:52PM -0800, Vasily Khoruzhick wrote: > Clock rate check that was added in commit bb43d40d7c83 ("drm/sun4i: rgb: > Validate the clock rate") prevents some panel and bridges from working with > sun4i driver. Sounds lile a regression that should be reverted. The fix is not a backwards compatible change either. > > Unfortunately, dotclock frequency for some modes are not achievable on > sunxi hardware, and there's a slight deviation in rate returned by > clk_round_rate(), so they fail this check. > > Experiments show that panels and bridges work fine with this slight > deviation, e.g. Pinebook that uses ANX6345 bridge with 768p eDP panel > requests 73 MHz, gets 72.296MHz instead (0.96% difference) and works just > fine. > > This patch adds DT property to disable strict clock rate check > > Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com> > --- > .../devicetree/bindings/display/sunxi/sun4i-drm.txt | 2 ++ > drivers/gpu/drm/sun4i/sun4i_rgb.c | 5 +++++ > drivers/gpu/drm/sun4i/sun4i_tcon.c | 3 +++ > drivers/gpu/drm/sun4i/sun4i_tcon.h | 1 + > 4 files changed, 11 insertions(+) > > diff --git a/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt b/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt > index f426bdb42f18..18c8b053a28d 100644 > --- a/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt > +++ b/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt > @@ -63,6 +63,8 @@ Required properties: > Documentation/devicetree/bindings/media/video-interfaces.txt. The > first port should be the input endpoint. The second should be the > output, usually to an HDMI connector. > + - no-strict-clock-check: don't reject timings if exact dot clock can't be > + reached. This should be the default IMO. Most panels are a single timing, so if we reject it the fallback no display? I thought we had some mechanism already to allow some range of frequencies. I think the chromeos guys needed something IIRC. Rob
On Mon, Feb 18, 2019 at 10:26 AM Rob Herring <robh@kernel.org> wrote: > > On Thu, Feb 14, 2019 at 09:09:52PM -0800, Vasily Khoruzhick wrote: > > Clock rate check that was added in commit bb43d40d7c83 ("drm/sun4i: rgb: > > Validate the clock rate") prevents some panel and bridges from working with > > sun4i driver. > > Sounds lile a regression that should be reverted. The fix is not a > backwards compatible change either. anx6345 driver isn't mainlined yet and I'm not sure if this change breaks any mainlined boards. So likely there's not enough justification to revert it. > > Unfortunately, dotclock frequency for some modes are not achievable on > > sunxi hardware, and there's a slight deviation in rate returned by > > clk_round_rate(), so they fail this check. > > > > Experiments show that panels and bridges work fine with this slight > > deviation, e.g. Pinebook that uses ANX6345 bridge with 768p eDP panel > > requests 73 MHz, gets 72.296MHz instead (0.96% difference) and works just > > fine. > > > > This patch adds DT property to disable strict clock rate check > > > > Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com> > > --- > > .../devicetree/bindings/display/sunxi/sun4i-drm.txt | 2 ++ > > drivers/gpu/drm/sun4i/sun4i_rgb.c | 5 +++++ > > drivers/gpu/drm/sun4i/sun4i_tcon.c | 3 +++ > > drivers/gpu/drm/sun4i/sun4i_tcon.h | 1 + > > 4 files changed, 11 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt b/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt > > index f426bdb42f18..18c8b053a28d 100644 > > --- a/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt > > +++ b/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt > > @@ -63,6 +63,8 @@ Required properties: > > Documentation/devicetree/bindings/media/video-interfaces.txt. The > > first port should be the input endpoint. The second should be the > > output, usually to an HDMI connector. > > + - no-strict-clock-check: don't reject timings if exact dot clock can't be > > + reached. > > This should be the default IMO. Most panels are a single timing, so if > we reject it the fallback no display? As far as I remember the change was introduced to reject some modes for which dotclock can't be reached when driver is used with VGA bridge. So if we make it default it'll break boards with VGA bridge and old DT. > I thought we had some mechanism already to allow some range of > frequencies. I think the chromeos guys needed something IIRC. You can specify frequency range for panels, but there's nothing for bridges. In my case EDID doesn't specify clock tolerance. > > Rob
On Mon, Feb 18, 2019 at 11:33:05AM -0800, Vasily Khoruzhick wrote: > On Mon, Feb 18, 2019 at 10:26 AM Rob Herring <robh@kernel.org> wrote: > > > > On Thu, Feb 14, 2019 at 09:09:52PM -0800, Vasily Khoruzhick wrote: > > > Clock rate check that was added in commit bb43d40d7c83 ("drm/sun4i: rgb: > > > Validate the clock rate") prevents some panel and bridges from working with > > > sun4i driver. > > > > Sounds lile a regression that should be reverted. The fix is not a > > backwards compatible change either. > > anx6345 driver isn't mainlined yet and I'm not sure if this change > breaks any mainlined boards. So likely there's not enough > justification to revert it. > > > > Unfortunately, dotclock frequency for some modes are not achievable on > > > sunxi hardware, and there's a slight deviation in rate returned by > > > clk_round_rate(), so they fail this check. > > > > > > Experiments show that panels and bridges work fine with this slight > > > deviation, e.g. Pinebook that uses ANX6345 bridge with 768p eDP panel > > > requests 73 MHz, gets 72.296MHz instead (0.96% difference) and works just > > > fine. > > > > > > This patch adds DT property to disable strict clock rate check > > > > > > Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com> > > > --- > > > .../devicetree/bindings/display/sunxi/sun4i-drm.txt | 2 ++ > > > drivers/gpu/drm/sun4i/sun4i_rgb.c | 5 +++++ > > > drivers/gpu/drm/sun4i/sun4i_tcon.c | 3 +++ > > > drivers/gpu/drm/sun4i/sun4i_tcon.h | 1 + > > > 4 files changed, 11 insertions(+) > > > > > > diff --git a/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt b/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt > > > index f426bdb42f18..18c8b053a28d 100644 > > > --- a/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt > > > +++ b/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt > > > @@ -63,6 +63,8 @@ Required properties: > > > Documentation/devicetree/bindings/media/video-interfaces.txt. The > > > first port should be the input endpoint. The second should be the > > > output, usually to an HDMI connector. > > > + - no-strict-clock-check: don't reject timings if exact dot clock can't be > > > + reached. > > > > This should be the default IMO. Most panels are a single timing, so if > > we reject it the fallback no display? > > As far as I remember the change was introduced to reject some modes > for which dotclock can't be reached when driver is used with VGA > bridge. So if we make it default it'll break boards with VGA bridge > and old DT. > > > I thought we had some mechanism already to allow some range of > > frequencies. I think the chromeos guys needed something IIRC. > > You can specify frequency range for panels, but there's nothing for > bridges. In my case EDID doesn't specify clock tolerance. I gave it some more though, and came up with the following patch. The basic idea is to leave the boundary check for the bridges that will have EDID and we need to filter out the modes that have no chance of being supported. The tolerancy used is the one defined in VESA specs, but I added a module parameter if you wanted to tune that. And finally, since most of our panels are single timings without any tolerancy, we just try our best in this case and that's it, while leaving the door open to support display_timings and being able to do more once we have an idea of what the tolerancies are. If that works for you, I'll submit it. Maxime --- >8 --- diff --git a/drivers/gpu/drm/sun4i/sun4i_rgb.c b/drivers/gpu/drm/sun4i/sun4i_rgb.c index f4a22689eb54..0460146aab75 100644 --- a/drivers/gpu/drm/sun4i/sun4i_rgb.c +++ b/drivers/gpu/drm/sun4i/sun4i_rgb.c @@ -43,6 +43,25 @@ drm_encoder_to_sun4i_rgb(struct drm_encoder *encoder) encoder); } +static inline struct drm_connector * +sun4i_rgb_get_connector_from_encoder(const struct drm_encoder *encoder) +{ + struct drm_connector *connector = NULL, *tmp; + struct drm_connector_list_iter iter; + + drm_connector_list_iter_begin(encoder->dev, &iter); + drm_for_each_connector_iter(tmp, &iter) + if (tmp->encoder == encoder) { + connector = tmp; + goto out; + } + +out: + drm_connector_list_iter_end(&iter); + + return connector; +} + static int sun4i_rgb_get_modes(struct drm_connector *connector) { struct sun4i_rgb *rgb = @@ -52,11 +71,22 @@ static int sun4i_rgb_get_modes(struct drm_connector *connector) return drm_panel_get_modes(tcon->panel); } +/* + * VESA DMT defines a tolerancy of 0.5% on the pixel clock, while the + * CVT spec reuses that tolerancy in its examples, so it looks to be a + * good default tolerancy for the EDID-based modes. + */ +static unsigned int clock_tolerancy = 5; +module_param(clock_tolerancy, uint, 0644); +MODULE_PARM_DESC(clock_tolerancy, + "Tolerancy of the pixel clock in per mille"); + static enum drm_mode_status sun4i_rgb_mode_valid(struct drm_encoder *crtc, const struct drm_display_mode *mode) { struct sun4i_rgb *rgb = drm_encoder_to_sun4i_rgb(crtc); struct sun4i_tcon *tcon = rgb->tcon; + struct drm_connector *connector = sun4i_rgb_get_connector_from_encoder(crtc); u32 hsync = mode->hsync_end - mode->hsync_start; u32 vsync = mode->vsync_end - mode->vsync_start; unsigned long rate = mode->clock * 1000; @@ -92,15 +122,27 @@ static enum drm_mode_status sun4i_rgb_mode_valid(struct drm_encoder *crtc, DRM_DEBUG_DRIVER("Vertical parameters OK\n"); + /* + * TODO: We should use the struct display_timing if available + * and / or trying to stretch the timings within that + * tolerancy to take care of panels that we wouldn't be able + * to have a exact match for. + */ + if (connector->connector_type == DRM_MODE_CONNECTOR_Unknown) { + DRM_DEBUG_DRIVER("RGB panel used, skipping clock rate checks"); + goto out; + } + tcon->dclk_min_div = 6; tcon->dclk_max_div = 127; rounded_rate = clk_round_rate(tcon->dclk, rate); - if (rounded_rate < rate) + if (rounded_rate < (rate * (1000 - clock_tolerancy) / 1000)) return MODE_CLOCK_LOW; - if (rounded_rate > rate) + if (rounded_rate > (rate * (1000 + clock_tolerancy) / 1000)) return MODE_CLOCK_HIGH; +out: DRM_DEBUG_DRIVER("Clock rate OK\n"); return MODE_OK; --- >8 ---
On Tue, Feb 19, 2019 at 12:56 AM Maxime Ripard <maxime.ripard@bootlin.com> wrote: > > On Mon, Feb 18, 2019 at 11:33:05AM -0800, Vasily Khoruzhick wrote: > > On Mon, Feb 18, 2019 at 10:26 AM Rob Herring <robh@kernel.org> wrote: > > > > > > On Thu, Feb 14, 2019 at 09:09:52PM -0800, Vasily Khoruzhick wrote: > > > > Clock rate check that was added in commit bb43d40d7c83 ("drm/sun4i: rgb: > > > > Validate the clock rate") prevents some panel and bridges from working with > > > > sun4i driver. > > > > > > Sounds lile a regression that should be reverted. The fix is not a > > > backwards compatible change either. > > > > anx6345 driver isn't mainlined yet and I'm not sure if this change > > breaks any mainlined boards. So likely there's not enough > > justification to revert it. > > > > > > Unfortunately, dotclock frequency for some modes are not achievable on > > > > sunxi hardware, and there's a slight deviation in rate returned by > > > > clk_round_rate(), so they fail this check. > > > > > > > > Experiments show that panels and bridges work fine with this slight > > > > deviation, e.g. Pinebook that uses ANX6345 bridge with 768p eDP panel > > > > requests 73 MHz, gets 72.296MHz instead (0.96% difference) and works just > > > > fine. > > > > > > > > This patch adds DT property to disable strict clock rate check > > > > > > > > Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com> > > > > --- > > > > .../devicetree/bindings/display/sunxi/sun4i-drm.txt | 2 ++ > > > > drivers/gpu/drm/sun4i/sun4i_rgb.c | 5 +++++ > > > > drivers/gpu/drm/sun4i/sun4i_tcon.c | 3 +++ > > > > drivers/gpu/drm/sun4i/sun4i_tcon.h | 1 + > > > > 4 files changed, 11 insertions(+) > > > > > > > > diff --git a/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt b/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt > > > > index f426bdb42f18..18c8b053a28d 100644 > > > > --- a/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt > > > > +++ b/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt > > > > @@ -63,6 +63,8 @@ Required properties: > > > > Documentation/devicetree/bindings/media/video-interfaces.txt. The > > > > first port should be the input endpoint. The second should be the > > > > output, usually to an HDMI connector. > > > > + - no-strict-clock-check: don't reject timings if exact dot clock can't be > > > > + reached. > > > > > > This should be the default IMO. Most panels are a single timing, so if > > > we reject it the fallback no display? > > > > As far as I remember the change was introduced to reject some modes > > for which dotclock can't be reached when driver is used with VGA > > bridge. So if we make it default it'll break boards with VGA bridge > > and old DT. > > > > > I thought we had some mechanism already to allow some range of > > > frequencies. I think the chromeos guys needed something IIRC. > > > > You can specify frequency range for panels, but there's nothing for > > bridges. In my case EDID doesn't specify clock tolerance. > > I gave it some more though, and came up with the following patch. The > basic idea is to leave the boundary check for the bridges that will > have EDID and we need to filter out the modes that have no chance of > being supported. The tolerancy used is the one defined in VESA specs, > but I added a module parameter if you wanted to tune that. > > And finally, since most of our panels are single timings without any > tolerancy, we just try our best in this case and that's it, while > leaving the door open to support display_timings and being able to do > more once we have an idea of what the tolerancies are. > > If that works for you, I'll submit it. Maxime, thanks for your patch but it doesn't work for me. Pinebook needs 1% tolerance. Having it as a module parameter means that no distro will be able to boot on Pinebook out of the box. > Maxime > > --- >8 --- > diff --git a/drivers/gpu/drm/sun4i/sun4i_rgb.c b/drivers/gpu/drm/sun4i/sun4i_rgb.c > index f4a22689eb54..0460146aab75 100644 > --- a/drivers/gpu/drm/sun4i/sun4i_rgb.c > +++ b/drivers/gpu/drm/sun4i/sun4i_rgb.c > @@ -43,6 +43,25 @@ drm_encoder_to_sun4i_rgb(struct drm_encoder *encoder) > encoder); > } > > +static inline struct drm_connector * > +sun4i_rgb_get_connector_from_encoder(const struct drm_encoder *encoder) > +{ > + struct drm_connector *connector = NULL, *tmp; > + struct drm_connector_list_iter iter; > + > + drm_connector_list_iter_begin(encoder->dev, &iter); > + drm_for_each_connector_iter(tmp, &iter) > + if (tmp->encoder == encoder) { > + connector = tmp; > + goto out; > + } > + > +out: > + drm_connector_list_iter_end(&iter); > + > + return connector; > +} > + > static int sun4i_rgb_get_modes(struct drm_connector *connector) > { > struct sun4i_rgb *rgb = > @@ -52,11 +71,22 @@ static int sun4i_rgb_get_modes(struct drm_connector *connector) > return drm_panel_get_modes(tcon->panel); > } > > +/* > + * VESA DMT defines a tolerancy of 0.5% on the pixel clock, while the > + * CVT spec reuses that tolerancy in its examples, so it looks to be a > + * good default tolerancy for the EDID-based modes. > + */ > +static unsigned int clock_tolerancy = 5; > +module_param(clock_tolerancy, uint, 0644); > +MODULE_PARM_DESC(clock_tolerancy, > + "Tolerancy of the pixel clock in per mille"); > + > static enum drm_mode_status sun4i_rgb_mode_valid(struct drm_encoder *crtc, > const struct drm_display_mode *mode) > { > struct sun4i_rgb *rgb = drm_encoder_to_sun4i_rgb(crtc); > struct sun4i_tcon *tcon = rgb->tcon; > + struct drm_connector *connector = sun4i_rgb_get_connector_from_encoder(crtc); > u32 hsync = mode->hsync_end - mode->hsync_start; > u32 vsync = mode->vsync_end - mode->vsync_start; > unsigned long rate = mode->clock * 1000; > @@ -92,15 +122,27 @@ static enum drm_mode_status sun4i_rgb_mode_valid(struct drm_encoder *crtc, > > DRM_DEBUG_DRIVER("Vertical parameters OK\n"); > > + /* > + * TODO: We should use the struct display_timing if available > + * and / or trying to stretch the timings within that > + * tolerancy to take care of panels that we wouldn't be able > + * to have a exact match for. > + */ > + if (connector->connector_type == DRM_MODE_CONNECTOR_Unknown) { > + DRM_DEBUG_DRIVER("RGB panel used, skipping clock rate checks"); > + goto out; > + } > + > tcon->dclk_min_div = 6; > tcon->dclk_max_div = 127; > rounded_rate = clk_round_rate(tcon->dclk, rate); > - if (rounded_rate < rate) > + if (rounded_rate < (rate * (1000 - clock_tolerancy) / 1000)) > return MODE_CLOCK_LOW; > > - if (rounded_rate > rate) > + if (rounded_rate > (rate * (1000 + clock_tolerancy) / 1000)) > return MODE_CLOCK_HIGH; > > +out: > DRM_DEBUG_DRIVER("Clock rate OK\n"); > > return MODE_OK; > --- >8 --- > > > -- > Maxime Ripard, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com
On Tue, Feb 19, 2019 at 07:44:56AM -0800, Vasily Khoruzhick wrote: > On Tue, Feb 19, 2019 at 12:56 AM Maxime Ripard > <maxime.ripard@bootlin.com> wrote: > > > > On Mon, Feb 18, 2019 at 11:33:05AM -0800, Vasily Khoruzhick wrote: > > > On Mon, Feb 18, 2019 at 10:26 AM Rob Herring <robh@kernel.org> wrote: > > > > > > > > On Thu, Feb 14, 2019 at 09:09:52PM -0800, Vasily Khoruzhick wrote: > > > > > Clock rate check that was added in commit bb43d40d7c83 ("drm/sun4i: rgb: > > > > > Validate the clock rate") prevents some panel and bridges from working with > > > > > sun4i driver. > > > > > > > > Sounds lile a regression that should be reverted. The fix is not a > > > > backwards compatible change either. > > > > > > anx6345 driver isn't mainlined yet and I'm not sure if this change > > > breaks any mainlined boards. So likely there's not enough > > > justification to revert it. > > > > > > > > Unfortunately, dotclock frequency for some modes are not achievable on > > > > > sunxi hardware, and there's a slight deviation in rate returned by > > > > > clk_round_rate(), so they fail this check. > > > > > > > > > > Experiments show that panels and bridges work fine with this slight > > > > > deviation, e.g. Pinebook that uses ANX6345 bridge with 768p eDP panel > > > > > requests 73 MHz, gets 72.296MHz instead (0.96% difference) and works just > > > > > fine. > > > > > > > > > > This patch adds DT property to disable strict clock rate check > > > > > > > > > > Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com> > > > > > --- > > > > > .../devicetree/bindings/display/sunxi/sun4i-drm.txt | 2 ++ > > > > > drivers/gpu/drm/sun4i/sun4i_rgb.c | 5 +++++ > > > > > drivers/gpu/drm/sun4i/sun4i_tcon.c | 3 +++ > > > > > drivers/gpu/drm/sun4i/sun4i_tcon.h | 1 + > > > > > 4 files changed, 11 insertions(+) > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt b/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt > > > > > index f426bdb42f18..18c8b053a28d 100644 > > > > > --- a/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt > > > > > +++ b/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt > > > > > @@ -63,6 +63,8 @@ Required properties: > > > > > Documentation/devicetree/bindings/media/video-interfaces.txt. The > > > > > first port should be the input endpoint. The second should be the > > > > > output, usually to an HDMI connector. > > > > > + - no-strict-clock-check: don't reject timings if exact dot clock can't be > > > > > + reached. > > > > > > > > This should be the default IMO. Most panels are a single timing, so if > > > > we reject it the fallback no display? > > > > > > As far as I remember the change was introduced to reject some modes > > > for which dotclock can't be reached when driver is used with VGA > > > bridge. So if we make it default it'll break boards with VGA bridge > > > and old DT. > > > > > > > I thought we had some mechanism already to allow some range of > > > > frequencies. I think the chromeos guys needed something IIRC. > > > > > > You can specify frequency range for panels, but there's nothing for > > > bridges. In my case EDID doesn't specify clock tolerance. > > > > I gave it some more though, and came up with the following patch. The > > basic idea is to leave the boundary check for the bridges that will > > have EDID and we need to filter out the modes that have no chance of > > being supported. The tolerancy used is the one defined in VESA specs, > > but I added a module parameter if you wanted to tune that. > > > > And finally, since most of our panels are single timings without any > > tolerancy, we just try our best in this case and that's it, while > > leaving the door open to support display_timings and being able to do > > more once we have an idea of what the tolerancies are. > > > > If that works for you, I'll submit it. > > Maxime, thanks for your patch but it doesn't work for me. Pinebook > needs 1% tolerance. Having it as a module parameter means that no > distro will be able to boot on Pinebook out of the box. I don't really know what to tell you, the VESA spec defines everywhere that tolerance, and if we're not able to provide that, then we're not compliant and I don't want us to not be compliant just because one panel needed to be a bit more flexible, and especially since what could work on one panel might fail on another one. If you want alternate solutions, then please answer to: http://lists.infradead.org/pipermail/linux-arm-kernel/2019-February/630441.html or provide the EDID blob. Maxime
On Wed, Feb 20, 2019 at 2:33 AM Maxime Ripard <maxime.ripard@bootlin.com> wrote: > > On Tue, Feb 19, 2019 at 07:44:56AM -0800, Vasily Khoruzhick wrote: > > On Tue, Feb 19, 2019 at 12:56 AM Maxime Ripard > > Maxime, thanks for your patch but it doesn't work for me. Pinebook > > needs 1% tolerance. Having it as a module parameter means that no > > distro will be able to boot on Pinebook out of the box. > > I don't really know what to tell you, the VESA spec defines everywhere > that tolerance, and if we're not able to provide that, then we're not > compliant and I don't want us to not be compliant just because one > panel needed to be a bit more flexible, and especially since what > could work on one panel might fail on another one. > > If you want alternate solutions, then please answer to: > http://lists.infradead.org/pipermail/linux-arm-kernel/2019-February/630441.html > > or provide the EDID blob. Uh, I spoke to early, my bad. I made a mistake when I calculated rate deviation for 768p last time - I took 73MHz as dotclock, but actual requested rate 72.3 MHz and it gets 72.296296 MHz, so it's within 0.5% tolerance. 1080p pinebook needs 138.5 MHz and gets 138.461538 MHz - also within 0.5%. However your patch has an issue, I'll respond to your previous email. > Maxime > > -- > Maxime Ripard, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com
On Tue, Feb 19, 2019 at 12:56 AM Maxime Ripard <maxime.ripard@bootlin.com> wrote: > > On Mon, Feb 18, 2019 at 11:33:05AM -0800, Vasily Khoruzhick wrote: > > On Mon, Feb 18, 2019 at 10:26 AM Rob Herring <robh@kernel.org> wrote: > > > > > > On Thu, Feb 14, 2019 at 09:09:52PM -0800, Vasily Khoruzhick wrote: > > > > Clock rate check that was added in commit bb43d40d7c83 ("drm/sun4i: rgb: > > > > Validate the clock rate") prevents some panel and bridges from working with > > > > sun4i driver. > > > > > > Sounds lile a regression that should be reverted. The fix is not a > > > backwards compatible change either. > > > > anx6345 driver isn't mainlined yet and I'm not sure if this change > > breaks any mainlined boards. So likely there's not enough > > justification to revert it. > > > > > > Unfortunately, dotclock frequency for some modes are not achievable on > > > > sunxi hardware, and there's a slight deviation in rate returned by > > > > clk_round_rate(), so they fail this check. > > > > > > > > Experiments show that panels and bridges work fine with this slight > > > > deviation, e.g. Pinebook that uses ANX6345 bridge with 768p eDP panel > > > > requests 73 MHz, gets 72.296MHz instead (0.96% difference) and works just > > > > fine. > > > > > > > > This patch adds DT property to disable strict clock rate check > > > > > > > > Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com> > > > > --- > > > > .../devicetree/bindings/display/sunxi/sun4i-drm.txt | 2 ++ > > > > drivers/gpu/drm/sun4i/sun4i_rgb.c | 5 +++++ > > > > drivers/gpu/drm/sun4i/sun4i_tcon.c | 3 +++ > > > > drivers/gpu/drm/sun4i/sun4i_tcon.h | 1 + > > > > 4 files changed, 11 insertions(+) > > > > > > > > diff --git a/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt b/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt > > > > index f426bdb42f18..18c8b053a28d 100644 > > > > --- a/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt > > > > +++ b/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt > > > > @@ -63,6 +63,8 @@ Required properties: > > > > Documentation/devicetree/bindings/media/video-interfaces.txt. The > > > > first port should be the input endpoint. The second should be the > > > > output, usually to an HDMI connector. > > > > + - no-strict-clock-check: don't reject timings if exact dot clock can't be > > > > + reached. > > > > > > This should be the default IMO. Most panels are a single timing, so if > > > we reject it the fallback no display? > > > > As far as I remember the change was introduced to reject some modes > > for which dotclock can't be reached when driver is used with VGA > > bridge. So if we make it default it'll break boards with VGA bridge > > and old DT. > > > > > I thought we had some mechanism already to allow some range of > > > frequencies. I think the chromeos guys needed something IIRC. > > > > You can specify frequency range for panels, but there's nothing for > > bridges. In my case EDID doesn't specify clock tolerance. > > I gave it some more though, and came up with the following patch. The > basic idea is to leave the boundary check for the bridges that will > have EDID and we need to filter out the modes that have no chance of > being supported. The tolerancy used is the one defined in VESA specs, > but I added a module parameter if you wanted to tune that. > > And finally, since most of our panels are single timings without any > tolerancy, we just try our best in this case and that's it, while > leaving the door open to support display_timings and being able to do > more once we have an idea of what the tolerancies are. > > If that works for you, I'll submit it. > > Maxime > > --- >8 --- > diff --git a/drivers/gpu/drm/sun4i/sun4i_rgb.c b/drivers/gpu/drm/sun4i/sun4i_rgb.c > index f4a22689eb54..0460146aab75 100644 > --- a/drivers/gpu/drm/sun4i/sun4i_rgb.c > +++ b/drivers/gpu/drm/sun4i/sun4i_rgb.c > @@ -43,6 +43,25 @@ drm_encoder_to_sun4i_rgb(struct drm_encoder *encoder) > encoder); > } > > +static inline struct drm_connector * > +sun4i_rgb_get_connector_from_encoder(const struct drm_encoder *encoder) > +{ > + struct drm_connector *connector = NULL, *tmp; > + struct drm_connector_list_iter iter; > + > + drm_connector_list_iter_begin(encoder->dev, &iter); > + drm_for_each_connector_iter(tmp, &iter) > + if (tmp->encoder == encoder) { > + connector = tmp; > + goto out; > + } > + > +out: > + drm_connector_list_iter_end(&iter); > + > + return connector; > +} > + > static int sun4i_rgb_get_modes(struct drm_connector *connector) > { > struct sun4i_rgb *rgb = > @@ -52,11 +71,22 @@ static int sun4i_rgb_get_modes(struct drm_connector *connector) > return drm_panel_get_modes(tcon->panel); > } > > +/* > + * VESA DMT defines a tolerancy of 0.5% on the pixel clock, while the > + * CVT spec reuses that tolerancy in its examples, so it looks to be a > + * good default tolerancy for the EDID-based modes. > + */ > +static unsigned int clock_tolerancy = 5; > +module_param(clock_tolerancy, uint, 0644); > +MODULE_PARM_DESC(clock_tolerancy, > + "Tolerancy of the pixel clock in per mille"); > + > static enum drm_mode_status sun4i_rgb_mode_valid(struct drm_encoder *crtc, > const struct drm_display_mode *mode) > { > struct sun4i_rgb *rgb = drm_encoder_to_sun4i_rgb(crtc); > struct sun4i_tcon *tcon = rgb->tcon; > + struct drm_connector *connector = sun4i_rgb_get_connector_from_encoder(crtc); > u32 hsync = mode->hsync_end - mode->hsync_start; > u32 vsync = mode->vsync_end - mode->vsync_start; > unsigned long rate = mode->clock * 1000; > @@ -92,15 +122,27 @@ static enum drm_mode_status sun4i_rgb_mode_valid(struct drm_encoder *crtc, > > DRM_DEBUG_DRIVER("Vertical parameters OK\n"); > > + /* > + * TODO: We should use the struct display_timing if available > + * and / or trying to stretch the timings within that > + * tolerancy to take care of panels that we wouldn't be able > + * to have a exact match for. > + */ > + if (connector->connector_type == DRM_MODE_CONNECTOR_Unknown) { I applied your patch onto 5.0-rc6 and connector is NULL for me on Pinebook. I guess you need to check whether it's NULL before dereferencing it. > + DRM_DEBUG_DRIVER("RGB panel used, skipping clock rate checks"); > + goto out; > + } > + > tcon->dclk_min_div = 6; > tcon->dclk_max_div = 127; > rounded_rate = clk_round_rate(tcon->dclk, rate); > - if (rounded_rate < rate) > + if (rounded_rate < (rate * (1000 - clock_tolerancy) / 1000)) > return MODE_CLOCK_LOW; > > - if (rounded_rate > rate) > + if (rounded_rate > (rate * (1000 + clock_tolerancy) / 1000)) > return MODE_CLOCK_HIGH; > > +out: > DRM_DEBUG_DRIVER("Clock rate OK\n"); > > return MODE_OK; > --- >8 --- > > > -- > Maxime Ripard, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com
diff --git a/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt b/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt index f426bdb42f18..18c8b053a28d 100644 --- a/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt +++ b/Documentation/devicetree/bindings/display/sunxi/sun4i-drm.txt @@ -63,6 +63,8 @@ Required properties: Documentation/devicetree/bindings/media/video-interfaces.txt. The first port should be the input endpoint. The second should be the output, usually to an HDMI connector. + - no-strict-clock-check: don't reject timings if exact dot clock can't be + reached. DWC HDMI TX Encoder ------------------- diff --git a/drivers/gpu/drm/sun4i/sun4i_rgb.c b/drivers/gpu/drm/sun4i/sun4i_rgb.c index f4a22689eb54..b392d4b1ebd3 100644 --- a/drivers/gpu/drm/sun4i/sun4i_rgb.c +++ b/drivers/gpu/drm/sun4i/sun4i_rgb.c @@ -94,7 +94,12 @@ static enum drm_mode_status sun4i_rgb_mode_valid(struct drm_encoder *crtc, tcon->dclk_min_div = 6; tcon->dclk_max_div = 127; + + if (tcon->no_strict_clock_check) + return MODE_OK; + rounded_rate = clk_round_rate(tcon->dclk, rate); + if (rounded_rate < rate) return MODE_CLOCK_LOW; diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c index cf45d0f940f9..f5f7f479332b 100644 --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c @@ -1108,6 +1108,9 @@ static int sun4i_tcon_bind(struct device *dev, struct device *master, tcon->id = engine->id; tcon->quirks = of_device_get_match_data(dev); + tcon->no_strict_clock_check = of_property_read_bool(dev->of_node, + "no-strict-clock-check"); + tcon->lcd_rst = devm_reset_control_get(dev, "lcd"); if (IS_ERR(tcon->lcd_rst)) { dev_err(dev, "Couldn't get our reset line\n"); diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.h b/drivers/gpu/drm/sun4i/sun4i_tcon.h index b5214d71610f..ab510b80004a 100644 --- a/drivers/gpu/drm/sun4i/sun4i_tcon.h +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.h @@ -258,6 +258,7 @@ struct sun4i_tcon { struct reset_control *lvds_rst; struct drm_panel *panel; + bool no_strict_clock_check; /* Platform adjustments */ const struct sun4i_tcon_quirks *quirks;
Clock rate check that was added in commit bb43d40d7c83 ("drm/sun4i: rgb: Validate the clock rate") prevents some panel and bridges from working with sun4i driver. Unfortunately, dotclock frequency for some modes are not achievable on sunxi hardware, and there's a slight deviation in rate returned by clk_round_rate(), so they fail this check. Experiments show that panels and bridges work fine with this slight deviation, e.g. Pinebook that uses ANX6345 bridge with 768p eDP panel requests 73 MHz, gets 72.296MHz instead (0.96% difference) and works just fine. This patch adds DT property to disable strict clock rate check Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com> --- .../devicetree/bindings/display/sunxi/sun4i-drm.txt | 2 ++ drivers/gpu/drm/sun4i/sun4i_rgb.c | 5 +++++ drivers/gpu/drm/sun4i/sun4i_tcon.c | 3 +++ drivers/gpu/drm/sun4i/sun4i_tcon.h | 1 + 4 files changed, 11 insertions(+)