| Message ID | 20181018073327.64942-1-icenowy@aosc.io |
|---|---|
| Headers | show |
| Series | Analogix ANX6345 RGB-(e)DP bridge support | expand |
Hi Icenowy, Thank you for the patch. On Thursday, 18 October 2018 10:33:19 EEST Icenowy Zheng wrote: > As ANA78xx chips are designed and produced by Analogix Semiconductor, > Inc, move their driver codes into analogix subdirectory. > > Signed-off-by: Icenowy Zheng <icenowy@aosc.io> > --- > drivers/gpu/drm/bridge/Kconfig | 10 ---------- > drivers/gpu/drm/bridge/Makefile | 4 ++-- > drivers/gpu/drm/bridge/analogix/Kconfig | 10 ++++++++++ > drivers/gpu/drm/bridge/analogix/Makefile | 1 + > .../gpu/drm/bridge/{ => analogix}/analogix-anx78xx.c | 0 > .../gpu/drm/bridge/{ => analogix}/analogix-anx78xx.h | 0 > 6 files changed, 13 insertions(+), 12 deletions(-) > rename drivers/gpu/drm/bridge/{ => analogix}/analogix-anx78xx.c (100%) > rename drivers/gpu/drm/bridge/{ => analogix}/analogix-anx78xx.h (100%) > > diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig > index 9eeb8ef0b174..8a7ffb3256d8 100644 > --- a/drivers/gpu/drm/bridge/Kconfig > +++ b/drivers/gpu/drm/bridge/Kconfig > @@ -15,16 +15,6 @@ config DRM_PANEL_BRIDGE > menu "Display Interface Bridges" > depends on DRM && DRM_BRIDGE > > -config DRM_ANALOGIX_ANX78XX > - tristate "Analogix ANX78XX bridge" > - select DRM_KMS_HELPER > - select REGMAP_I2C > - ---help--- > - ANX78XX is an ultra-low Full-HD SlimPort transmitter > - designed for portable devices. The ANX78XX transforms > - the HDMI output of an application processor to MyDP > - or DisplayPort. > - > config DRM_CDNS_DSI > tristate "Cadence DPI/DSI bridge" > select DRM_KMS_HELPER > diff --git a/drivers/gpu/drm/bridge/Makefile > b/drivers/gpu/drm/bridge/Makefile index 4934fcf5a6f8..a6c7dd7727ea 100644 > --- a/drivers/gpu/drm/bridge/Makefile > +++ b/drivers/gpu/drm/bridge/Makefile > @@ -1,5 +1,4 @@ > # SPDX-License-Identifier: GPL-2.0 > -obj-$(CONFIG_DRM_ANALOGIX_ANX78XX) += analogix-anx78xx.o > obj-$(CONFIG_DRM_CDNS_DSI) += cdns-dsi.o > obj-$(CONFIG_DRM_DUMB_VGA_DAC) += dumb-vga-dac.o > obj-$(CONFIG_DRM_LVDS_ENCODER) += lvds-encoder.o > @@ -12,8 +11,9 @@ obj-$(CONFIG_DRM_SII9234) += sii9234.o > obj-$(CONFIG_DRM_THINE_THC63LVD1024) += thc63lvd1024.o > obj-$(CONFIG_DRM_TOSHIBA_TC358764) += tc358764.o > obj-$(CONFIG_DRM_TOSHIBA_TC358767) += tc358767.o > -obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix/ > obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511/ > obj-$(CONFIG_DRM_TI_SN65DSI86) += ti-sn65dsi86.o > obj-$(CONFIG_DRM_TI_TFP410) += ti-tfp410.o > + > +obj-y += analogix/ > obj-y += synopsys/ > diff --git a/drivers/gpu/drm/bridge/analogix/Kconfig > b/drivers/gpu/drm/bridge/analogix/Kconfig index 80f286fa3a69..27b37aa2ea77 > 100644 > --- a/drivers/gpu/drm/bridge/analogix/Kconfig > +++ b/drivers/gpu/drm/bridge/analogix/Kconfig > @@ -1,3 +1,13 @@ > config DRM_ANALOGIX_DP > tristate > depends on DRM > + > +config DRM_ANALOGIX_ANX78XX > + tristate "Analogix ANX78XX bridge" > + select DRM_KMS_HELPER > + select REGMAP_I2C > + ---help--- > + ANX78XX is an ultra-low Full-HD SlimPort transmitter > + designed for portable devices. The ANX78XX transforms > + the HDMI output of an application processor to MyDP > + or DisplayPort. > diff --git a/drivers/gpu/drm/bridge/analogix/Makefile > b/drivers/gpu/drm/bridge/analogix/Makefile index cd4010ba6890..eb41be845055 > 100644 > --- a/drivers/gpu/drm/bridge/analogix/Makefile > +++ b/drivers/gpu/drm/bridge/analogix/Makefile > @@ -1,2 +1,3 @@ > analogix_dp-objs := analogix_dp_core.o analogix_dp_reg.o > obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix_dp.o > +obj-$(CONFIG_DRM_ANALOGIX_ANX78XX) += analogix-anx78xx.o This makes sense to me. I would have tried to keep the Kconfig and Makefile entries alphabetically sorted, but that's not a big deal. With or without the sorting, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > diff --git a/drivers/gpu/drm/bridge/analogix-anx78xx.c > b/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c similarity index 100% > rename from drivers/gpu/drm/bridge/analogix-anx78xx.c > rename to drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c > diff --git a/drivers/gpu/drm/bridge/analogix-anx78xx.h > b/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.h similarity index 100% > rename from drivers/gpu/drm/bridge/analogix-anx78xx.h > rename to drivers/gpu/drm/bridge/analogix/analogix-anx78xx.h
Hi Icenowy, Thank you for the patch. On Thursday, 18 October 2018 10:33:27 EEST Icenowy Zheng wrote: > From: Chen-Yu Tsai <wens@csie.org> > > The panels shipped with Allwinner devices are very "generic", i.e. > they do not have model numbers or reliable sources of information > for the timings (that we know of) other than the fex files shipped > on them. The dot clock frequency provided in the fex files have all > been rounded to the nearest MHz, as that is the unit used in them. > > We were using the simple panel "urt,umsh-8596md-t" as a substitute > for the A13 Q8 tablets in the absence of a specific model for what > may be many different but otherwise timing compatible panels. This > was usable without any visual artifacts or side effects, until the > dot clock rate check was added in commit bb43d40d7c83 ("drm/sun4i: > rgb: Validate the clock rate"). > > The reason this check fails is because the dotclock frequency for > this model is 33.26 MHz, which is not achievable with our dot clock > hardware, and the rate returned by clk_round_rate deviates slightly, > causing the driver to reject the display mode. > > The LCD panels have some tolerance on the dot clock frequency, even > if it's not specified in their datasheets. > > This patch adds a 5% tolerence to the dot clock check. Why do you think this shouldn't be merged ? > Signed-off-by: Chen-Yu Tsai <wens@csie.org> > --- > drivers/gpu/drm/sun4i/sun4i_rgb.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/sun4i/sun4i_rgb.c > b/drivers/gpu/drm/sun4i/sun4i_rgb.c index bf068da6b12e..23bdc449eacc 100644 > --- a/drivers/gpu/drm/sun4i/sun4i_rgb.c > +++ b/drivers/gpu/drm/sun4i/sun4i_rgb.c > @@ -92,13 +92,14 @@ static enum drm_mode_status sun4i_rgb_mode_valid(struct > drm_encoder *crtc, > > DRM_DEBUG_DRIVER("Vertical parameters OK\n"); > > + /* Check against a 5% tolerance for the dot clock */ > 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 * 19 / 20 ) > return MODE_CLOCK_LOW; > > - if (rounded_rate > rate) > + if (rounded_rate > rate * 21 / 20) > return MODE_CLOCK_HIGH; > > DRM_DEBUG_DRIVER("Clock rate OK\n");
On Thu, Oct 18, 2018 at 10:55 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Icenowy, > > Thank you for the patch. > > On Thursday, 18 October 2018 10:33:27 EEST Icenowy Zheng wrote: > > From: Chen-Yu Tsai <wens@csie.org> > > > > The panels shipped with Allwinner devices are very "generic", i.e. > > they do not have model numbers or reliable sources of information > > for the timings (that we know of) other than the fex files shipped > > on them. The dot clock frequency provided in the fex files have all > > been rounded to the nearest MHz, as that is the unit used in them. > > > > We were using the simple panel "urt,umsh-8596md-t" as a substitute > > for the A13 Q8 tablets in the absence of a specific model for what > > may be many different but otherwise timing compatible panels. This > > was usable without any visual artifacts or side effects, until the > > dot clock rate check was added in commit bb43d40d7c83 ("drm/sun4i: > > rgb: Validate the clock rate"). > > > > The reason this check fails is because the dotclock frequency for > > this model is 33.26 MHz, which is not achievable with our dot clock > > hardware, and the rate returned by clk_round_rate deviates slightly, > > causing the driver to reject the display mode. > > > > The LCD panels have some tolerance on the dot clock frequency, even > > if it's not specified in their datasheets. > > > > This patch adds a 5% tolerence to the dot clock check. > > Why do you think this shouldn't be merged ? It pisses of a lot of people who really insist upon accurate timing. I think a better fix would be to have a dotclock range in drm_panel, and some magic to figure out which one of these we can actually do. Then tell userspace that this is the mode is should request. That way userspace knows what the actual dotclock/refresh rate is, and the panel still works. -Daniel > > > Signed-off-by: Chen-Yu Tsai <wens@csie.org> > > --- > > drivers/gpu/drm/sun4i/sun4i_rgb.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/sun4i/sun4i_rgb.c > > b/drivers/gpu/drm/sun4i/sun4i_rgb.c index bf068da6b12e..23bdc449eacc 100644 > > --- a/drivers/gpu/drm/sun4i/sun4i_rgb.c > > +++ b/drivers/gpu/drm/sun4i/sun4i_rgb.c > > @@ -92,13 +92,14 @@ static enum drm_mode_status sun4i_rgb_mode_valid(struct > > drm_encoder *crtc, > > > > DRM_DEBUG_DRIVER("Vertical parameters OK\n"); > > > > + /* Check against a 5% tolerance for the dot clock */ > > 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 * 19 / 20 ) > > return MODE_CLOCK_LOW; > > > > - if (rounded_rate > rate) > > + if (rounded_rate > rate * 21 / 20) > > return MODE_CLOCK_HIGH; > > > > DRM_DEBUG_DRIVER("Clock rate OK\n"); > > -- > Regards, > > Laurent Pinchart > > > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Thu, Oct 18, 2018 at 11:18:06AM +0200, Daniel Vetter wrote: > On Thu, Oct 18, 2018 at 10:55 AM Laurent Pinchart > <laurent.pinchart@ideasonboard.com> wrote: > > > > Hi Icenowy, > > > > Thank you for the patch. > > > > On Thursday, 18 October 2018 10:33:27 EEST Icenowy Zheng wrote: > > > From: Chen-Yu Tsai <wens@csie.org> > > > > > > The panels shipped with Allwinner devices are very "generic", i.e. > > > they do not have model numbers or reliable sources of information > > > for the timings (that we know of) other than the fex files shipped > > > on them. The dot clock frequency provided in the fex files have all > > > been rounded to the nearest MHz, as that is the unit used in them. > > > > > > We were using the simple panel "urt,umsh-8596md-t" as a substitute > > > for the A13 Q8 tablets in the absence of a specific model for what > > > may be many different but otherwise timing compatible panels. This > > > was usable without any visual artifacts or side effects, until the > > > dot clock rate check was added in commit bb43d40d7c83 ("drm/sun4i: > > > rgb: Validate the clock rate"). > > > > > > The reason this check fails is because the dotclock frequency for > > > this model is 33.26 MHz, which is not achievable with our dot clock > > > hardware, and the rate returned by clk_round_rate deviates slightly, > > > causing the driver to reject the display mode. > > > > > > The LCD panels have some tolerance on the dot clock frequency, even > > > if it's not specified in their datasheets. > > > > > > This patch adds a 5% tolerence to the dot clock check. > > > > Why do you think this shouldn't be merged ? > > It pisses of a lot of people who really insist upon accurate timing. It's not just about accurate timings. That 5% is a made-up limit, that never have really been confirmed by looking at the typical tolerancies of panels. And while being to relaxed might make some panels work that are not working currently, it might also break some panels that currently work and won't now, and ideally, we should really be able to let those panels work if they can, and filter out resolutions if they can't. > I think a better fix would be to have a dotclock range in drm_panel, > and some magic to figure out which one of these we can actually > do. Then tell userspace that this is the mode is should > request. That way userspace knows what the actual dotclock/refresh > rate is, and the panel still works. It's not just about panels, but also bridges with EDID where the tolerancy is not exposed. Maxime
Hello, On Thursday, 18 October 2018 12:42:58 EEST Maxime Ripard wrote: > On Thu, Oct 18, 2018 at 11:18:06AM +0200, Daniel Vetter wrote: > > On Thu, Oct 18, 2018 at 10:55 AM Laurent Pinchart wrote: > >> On Thursday, 18 October 2018 10:33:27 EEST Icenowy Zheng wrote: > >>> From: Chen-Yu Tsai <wens@csie.org> > >>> > >>> The panels shipped with Allwinner devices are very "generic", i.e. > >>> they do not have model numbers or reliable sources of information > >>> for the timings (that we know of) other than the fex files shipped > >>> on them. The dot clock frequency provided in the fex files have all > >>> been rounded to the nearest MHz, as that is the unit used in them. > >>> > >>> We were using the simple panel "urt,umsh-8596md-t" as a substitute > >>> for the A13 Q8 tablets in the absence of a specific model for what > >>> may be many different but otherwise timing compatible panels. This > >>> was usable without any visual artifacts or side effects, until the > >>> dot clock rate check was added in commit bb43d40d7c83 ("drm/sun4i: > >>> rgb: Validate the clock rate"). > >>> > >>> The reason this check fails is because the dotclock frequency for > >>> this model is 33.26 MHz, which is not achievable with our dot clock > >>> hardware, and the rate returned by clk_round_rate deviates slightly, > >>> causing the driver to reject the display mode. > >>> > >>> The LCD panels have some tolerance on the dot clock frequency, even > >>> if it's not specified in their datasheets. > >>> > >>> This patch adds a 5% tolerence to the dot clock check. > >> > >> Why do you think this shouldn't be merged ? > > > > It pisses of a lot of people who really insist upon accurate timing. > > It's not just about accurate timings. That 5% is a made-up limit, that > never have really been confirmed by looking at the typical tolerancies > of panels. > > And while being to relaxed might make some panels work that are not > working currently, it might also break some panels that currently work > and won't now, and ideally, we should really be able to let those > panels work if they can, and filter out resolutions if they can't. > > > I think a better fix would be to have a dotclock range in drm_panel, > > and some magic to figure out which one of these we can actually > > do. Then tell userspace that this is the mode is should > > request. That way userspace knows what the actual dotclock/refresh > > rate is, and the panel still works. > > It's not just about panels, but also bridges with EDID where the > tolerancy is not exposed. Given that the tolerance is a property of the panel or bridge, I agree with Daniel that it should be implemented there, or at least in cooperation with drm_panel and drm_bridge. Semi-related information, I think the CEA and VESA standards allow a 0.5% clock tolerance. What is the maximum clock frequency deviation required for this platform ?
On Thu, Oct 18, 2018 at 02:31:01PM +0300, Laurent Pinchart wrote: > Hello, > > On Thursday, 18 October 2018 12:42:58 EEST Maxime Ripard wrote: > > On Thu, Oct 18, 2018 at 11:18:06AM +0200, Daniel Vetter wrote: > > > On Thu, Oct 18, 2018 at 10:55 AM Laurent Pinchart wrote: > > >> On Thursday, 18 October 2018 10:33:27 EEST Icenowy Zheng wrote: > > >>> From: Chen-Yu Tsai <wens@csie.org> > > >>> > > >>> The panels shipped with Allwinner devices are very "generic", i.e. > > >>> they do not have model numbers or reliable sources of information > > >>> for the timings (that we know of) other than the fex files shipped > > >>> on them. The dot clock frequency provided in the fex files have all > > >>> been rounded to the nearest MHz, as that is the unit used in them. > > >>> > > >>> We were using the simple panel "urt,umsh-8596md-t" as a substitute > > >>> for the A13 Q8 tablets in the absence of a specific model for what > > >>> may be many different but otherwise timing compatible panels. This > > >>> was usable without any visual artifacts or side effects, until the > > >>> dot clock rate check was added in commit bb43d40d7c83 ("drm/sun4i: > > >>> rgb: Validate the clock rate"). > > >>> > > >>> The reason this check fails is because the dotclock frequency for > > >>> this model is 33.26 MHz, which is not achievable with our dot clock > > >>> hardware, and the rate returned by clk_round_rate deviates slightly, > > >>> causing the driver to reject the display mode. > > >>> > > >>> The LCD panels have some tolerance on the dot clock frequency, even > > >>> if it's not specified in their datasheets. > > >>> > > >>> This patch adds a 5% tolerence to the dot clock check. > > >> > > >> Why do you think this shouldn't be merged ? > > > > > > It pisses of a lot of people who really insist upon accurate timing. > > > > It's not just about accurate timings. That 5% is a made-up limit, that > > never have really been confirmed by looking at the typical tolerancies > > of panels. > > > > And while being to relaxed might make some panels work that are not > > working currently, it might also break some panels that currently work > > and won't now, and ideally, we should really be able to let those > > panels work if they can, and filter out resolutions if they can't. > > > > > I think a better fix would be to have a dotclock range in drm_panel, > > > and some magic to figure out which one of these we can actually > > > do. Then tell userspace that this is the mode is should > > > request. That way userspace knows what the actual dotclock/refresh > > > rate is, and the panel still works. > > > > It's not just about panels, but also bridges with EDID where the > > tolerancy is not exposed. > > Given that the tolerance is a property of the panel or bridge, I agree with > Daniel that it should be implemented there, or at least in cooperation with > drm_panel and drm_bridge. How are we supposed to deal with panels without any documentation then? > Semi-related information, I think the CEA and VESA standards allow a 0.5% > clock tolerance. What is the maximum clock frequency deviation required for > this platform ? Looks like it does indeed. That's definetely good to know. Thanks! Maxime
在 2018-10-18四的 14:18 +0200，Maxime Ripard写道： > On Thu, Oct 18, 2018 at 02:31:01PM +0300, Laurent Pinchart wrote: > > Hello, > > > > On Thursday, 18 October 2018 12:42:58 EEST Maxime Ripard wrote: > > > On Thu, Oct 18, 2018 at 11:18:06AM +0200, Daniel Vetter wrote: > > > > On Thu, Oct 18, 2018 at 10:55 AM Laurent Pinchart wrote: > > > > > On Thursday, 18 October 2018 10:33:27 EEST Icenowy Zheng > > > > > wrote: > > > > > > From: Chen-Yu Tsai <wens@csie.org> > > > > > > > > > > > > The panels shipped with Allwinner devices are very > > > > > > "generic", i.e. > > > > > > they do not have model numbers or reliable sources of > > > > > > information > > > > > > for the timings (that we know of) other than the fex files > > > > > > shipped > > > > > > on them. The dot clock frequency provided in the fex files > > > > > > have all > > > > > > been rounded to the nearest MHz, as that is the unit used > > > > > > in them. > > > > > > > > > > > > We were using the simple panel "urt,umsh-8596md-t" as a > > > > > > substitute > > > > > > for the A13 Q8 tablets in the absence of a specific model > > > > > > for what > > > > > > may be many different but otherwise timing compatible > > > > > > panels. This > > > > > > was usable without any visual artifacts or side effects, > > > > > > until the > > > > > > dot clock rate check was added in commit bb43d40d7c83 > > > > > > ("drm/sun4i: > > > > > > rgb: Validate the clock rate"). > > > > > > > > > > > > The reason this check fails is because the dotclock > > > > > > frequency for > > > > > > this model is 33.26 MHz, which is not achievable with our > > > > > > dot clock > > > > > > hardware, and the rate returned by clk_round_rate deviates > > > > > > slightly, > > > > > > causing the driver to reject the display mode. > > > > > > > > > > > > The LCD panels have some tolerance on the dot clock > > > > > > frequency, even > > > > > > if it's not specified in their datasheets. > > > > > > > > > > > > This patch adds a 5% tolerence to the dot clock check. > > > > > > > > > > Why do you think this shouldn't be merged ? > > > > > > > > It pisses of a lot of people who really insist upon accurate > > > > timing. > > > > > > It's not just about accurate timings. That 5% is a made-up limit, > > > that > > > never have really been confirmed by looking at the typical > > > tolerancies > > > of panels. > > > > > > And while being to relaxed might make some panels work that are > > > not > > > working currently, it might also break some panels that currently > > > work > > > and won't now, and ideally, we should really be able to let those > > > panels work if they can, and filter out resolutions if they > > > can't. > > > > > > > I think a better fix would be to have a dotclock range in > > > > drm_panel, > > > > and some magic to figure out which one of these we can actually > > > > do. Then tell userspace that this is the mode is should > > > > request. That way userspace knows what the actual > > > > dotclock/refresh > > > > rate is, and the panel still works. > > > > > > It's not just about panels, but also bridges with EDID where the > > > tolerancy is not exposed. > > > > Given that the tolerance is a property of the panel or bridge, I > > agree with > > Daniel that it should be implemented there, or at least in > > cooperation with > > drm_panel and drm_bridge. > > How are we supposed to deal with panels without any documentation > then? > > > Semi-related information, I think the CEA and VESA standards allow > > a 0.5% > > clock tolerance. What is the maximum clock frequency deviation > > required for > > this platform ? > > Looks like it does indeed. That's definetely good to know. Then maybe we can choose to allow 0.5% error when a bridge is attached? > > Thanks! > Maxime >
On Thu, Oct 18, 2018 at 08:50:15PM +0800, Icenowy Zheng wrote: > 在 2018-10-18四的 14:18 +0200，Maxime Ripard写道： > > On Thu, Oct 18, 2018 at 02:31:01PM +0300, Laurent Pinchart wrote: > > > Hello, > > > > > > On Thursday, 18 October 2018 12:42:58 EEST Maxime Ripard wrote: > > > > On Thu, Oct 18, 2018 at 11:18:06AM +0200, Daniel Vetter wrote: > > > > > On Thu, Oct 18, 2018 at 10:55 AM Laurent Pinchart wrote: > > > > > > On Thursday, 18 October 2018 10:33:27 EEST Icenowy Zheng > > > > > > wrote: > > > > > > > From: Chen-Yu Tsai <wens@csie.org> > > > > > > > > > > > > > > The panels shipped with Allwinner devices are very > > > > > > > "generic", i.e. > > > > > > > they do not have model numbers or reliable sources of > > > > > > > information > > > > > > > for the timings (that we know of) other than the fex files > > > > > > > shipped > > > > > > > on them. The dot clock frequency provided in the fex files > > > > > > > have all > > > > > > > been rounded to the nearest MHz, as that is the unit used > > > > > > > in them. > > > > > > > > > > > > > > We were using the simple panel "urt,umsh-8596md-t" as a > > > > > > > substitute > > > > > > > for the A13 Q8 tablets in the absence of a specific model > > > > > > > for what > > > > > > > may be many different but otherwise timing compatible > > > > > > > panels. This > > > > > > > was usable without any visual artifacts or side effects, > > > > > > > until the > > > > > > > dot clock rate check was added in commit bb43d40d7c83 > > > > > > > ("drm/sun4i: > > > > > > > rgb: Validate the clock rate"). > > > > > > > > > > > > > > The reason this check fails is because the dotclock > > > > > > > frequency for > > > > > > > this model is 33.26 MHz, which is not achievable with our > > > > > > > dot clock > > > > > > > hardware, and the rate returned by clk_round_rate deviates > > > > > > > slightly, > > > > > > > causing the driver to reject the display mode. > > > > > > > > > > > > > > The LCD panels have some tolerance on the dot clock > > > > > > > frequency, even > > > > > > > if it's not specified in their datasheets. > > > > > > > > > > > > > > This patch adds a 5% tolerence to the dot clock check. > > > > > > > > > > > > Why do you think this shouldn't be merged ? > > > > > > > > > > It pisses of a lot of people who really insist upon accurate > > > > > timing. > > > > > > > > It's not just about accurate timings. That 5% is a made-up limit, > > > > that > > > > never have really been confirmed by looking at the typical > > > > tolerancies > > > > of panels. > > > > > > > > And while being to relaxed might make some panels work that are > > > > not > > > > working currently, it might also break some panels that currently > > > > work > > > > and won't now, and ideally, we should really be able to let those > > > > panels work if they can, and filter out resolutions if they > > > > can't. > > > > > > > > > I think a better fix would be to have a dotclock range in > > > > > drm_panel, > > > > > and some magic to figure out which one of these we can actually > > > > > do. Then tell userspace that this is the mode is should > > > > > request. That way userspace knows what the actual > > > > > dotclock/refresh > > > > > rate is, and the panel still works. > > > > > > > > It's not just about panels, but also bridges with EDID where the > > > > tolerancy is not exposed. > > > > > > Given that the tolerance is a property of the panel or bridge, I > > > agree with > > > Daniel that it should be implemented there, or at least in > > > cooperation with > > > drm_panel and drm_bridge. > > > > How are we supposed to deal with panels without any documentation > > then? > > > > > Semi-related information, I think the CEA and VESA standards allow > > > a 0.5% > > > clock tolerance. What is the maximum clock frequency deviation > > > required for > > > this platform ? > > > > Looks like it does indeed. That's definetely good to know. > > Then maybe we can choose to allow 0.5% error when a bridge is attached? It doesn't fix the panel case, but that sounds reasonable yes Maxime
Hi Maxime, On Thursday, 18 October 2018 15:18:19 EEST Maxime Ripard wrote: > On Thu, Oct 18, 2018 at 02:31:01PM +0300, Laurent Pinchart wrote: > > On Thursday, 18 October 2018 12:42:58 EEST Maxime Ripard wrote: > >> On Thu, Oct 18, 2018 at 11:18:06AM +0200, Daniel Vetter wrote: > >>> On Thu, Oct 18, 2018 at 10:55 AM Laurent Pinchart wrote: > >>>> On Thursday, 18 October 2018 10:33:27 EEST Icenowy Zheng wrote: > >>>>> From: Chen-Yu Tsai <wens@csie.org> > >>>>> > >>>>> The panels shipped with Allwinner devices are very "generic", i.e. > >>>>> they do not have model numbers or reliable sources of information > >>>>> for the timings (that we know of) other than the fex files shipped > >>>>> on them. The dot clock frequency provided in the fex files have all > >>>>> been rounded to the nearest MHz, as that is the unit used in them. > >>>>> > >>>>> We were using the simple panel "urt,umsh-8596md-t" as a substitute > >>>>> for the A13 Q8 tablets in the absence of a specific model for what > >>>>> may be many different but otherwise timing compatible panels. This > >>>>> was usable without any visual artifacts or side effects, until the > >>>>> dot clock rate check was added in commit bb43d40d7c83 ("drm/sun4i: > >>>>> rgb: Validate the clock rate"). > >>>>> > >>>>> The reason this check fails is because the dotclock frequency for > >>>>> this model is 33.26 MHz, which is not achievable with our dot clock > >>>>> hardware, and the rate returned by clk_round_rate deviates slightly, > >>>>> causing the driver to reject the display mode. > >>>>> > >>>>> The LCD panels have some tolerance on the dot clock frequency, even > >>>>> if it's not specified in their datasheets. > >>>>> > >>>>> This patch adds a 5% tolerence to the dot clock check. > >>>> > >>>> Why do you think this shouldn't be merged ? > >>> > >>> It pisses of a lot of people who really insist upon accurate timing. > >> > >> It's not just about accurate timings. That 5% is a made-up limit, that > >> never have really been confirmed by looking at the typical tolerancies > >> of panels. > >> > >> And while being to relaxed might make some panels work that are not > >> working currently, it might also break some panels that currently work > >> and won't now, and ideally, we should really be able to let those > >> panels work if they can, and filter out resolutions if they can't. > >> > >>> I think a better fix would be to have a dotclock range in drm_panel, > >>> and some magic to figure out which one of these we can actually > >>> do. Then tell userspace that this is the mode is should > >>> request. That way userspace knows what the actual dotclock/refresh > >>> rate is, and the panel still works. > >> > >> It's not just about panels, but also bridges with EDID where the > >> tolerancy is not exposed. > > > > Given that the tolerance is a property of the panel or bridge, I agree > > with Daniel that it should be implemented there, or at least in > > cooperation with drm_panel and drm_bridge. > > How are we supposed to deal with panels without any documentation then? We can only guess, but if the guess comes from the panel driver, it will at least not affect all panels and bridges, like this patch does. > > Semi-related information, I think the CEA and VESA standards allow a 0.5% > > clock tolerance. What is the maximum clock frequency deviation required > > for this platform ? > > Looks like it does indeed. That's definetely good to know.
Hi Icenowy, On Thursday, October 18, 2018 12:33:25 AM PDT Icenowy Zheng wrote: > Pinebook has an ANX6345 bridge connected to the RGB666 LCD output, and > the I2C controlling signals are connected to R_I2C bus. > > Enable it in the device tree, and add a usable EDID from the panel's > datasheet (at least 14" Pinebook used a panel without EDID). There's no EDID in dts and 14" Pinebook uses a panel with EDID. Is it leftover of old comment? Regards, Vasily > > Signed-off-by: Icenowy Zheng <icenowy@aosc.io> > --- > .../dts/allwinner/sun50i-a64-pinebook.dts | 43 +++++++++++++++++++ > 1 file changed, 43 insertions(+) > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinebook.dts > b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinebook.dts index > 77fac84797e9..d7c14d0d61f9 100644 > --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinebook.dts > +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinebook.dts > @@ -66,6 +66,10 @@ > }; > }; > > +&de { > + status = "okay"; > +}; > + > &ehci0 { > phys = <&usbphy 0>; > phy-names = "usb"; > @@ -76,6 +80,10 @@ > status = "okay"; > }; > > +&mixer0 { > + status = "okay"; > +}; > + > &mmc0 { > pinctrl-names = "default"; > pinctrl-0 = <&mmc0_pins>; > @@ -127,6 +135,27 @@ > status = "okay"; > }; > > +&r_i2c { > + pinctrl-names = "default"; > + pinctrl-0 = <&r_i2c_pins_a>; > + status = "okay"; > + > + anx6345: anx6345@38 { > + compatible = "analogix,anx6345"; > + reg = <0x38>; > + reset-gpios = <&pio 3 24 GPIO_ACTIVE_HIGH>; /* PD24 */ > + panel-supply = <®_dc1sw>; > + dvdd25-supply = <®_dldo2>; > + dvdd12-supply = <®_fldo1>; > + > + port { > + anx6345_in: endpoint { > + remote-endpoint = <&tcon0_out_anx6345>; > + }; > + }; > + }; > +}; > + > &r_rsb { > status = "okay"; > > @@ -267,6 +296,20 @@ > vcc-hdmi-supply = <®_dldo1>; > }; > > +&tcon0 { > + pinctrl-names = "default"; > + pinctrl-0 = <&lcd_rgb666_pins>; > + > + status = "okay"; > +}; > + > +&tcon0_out { > + tcon0_out_anx6345: endpoint@0 { > + reg = <0>; > + remote-endpoint = <&anx6345_in>; > + }; > +}; > + > &uart0 { > pinctrl-names = "default"; > pinctrl-0 = <&uart0_pb_pins>;
于 2018年10月18日 GMT+08:00 下午11:17:35, Vasily Khoruzhick <anarsoul@gmail.com> 写到: >Hi Icenowy, > >On Thursday, October 18, 2018 12:33:25 AM PDT Icenowy Zheng wrote: >> Pinebook has an ANX6345 bridge connected to the RGB666 LCD output, >and >> the I2C controlling signals are connected to R_I2C bus. >> >> Enable it in the device tree, and add a usable EDID from the panel's >> datasheet (at least 14" Pinebook used a panel without EDID). > >There's no EDID in dts and 14" Pinebook uses a panel with EDID. Is it >leftover >of old comment? Yes, it is. > >Regards, >Vasily > >> >> Signed-off-by: Icenowy Zheng <icenowy@aosc.io> >> --- >> .../dts/allwinner/sun50i-a64-pinebook.dts | 43 >+++++++++++++++++++ >> 1 file changed, 43 insertions(+) >> >> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinebook.dts >> b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinebook.dts index >> 77fac84797e9..d7c14d0d61f9 100644 >> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pinebook.dts >> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pinebook.dts >> @@ -66,6 +66,10 @@ >> }; >> }; >> >> +&de { >> + status = "okay"; >> +}; >> + >> &ehci0 { >> phys = <&usbphy 0>; >> phy-names = "usb"; >> @@ -76,6 +80,10 @@ >> status = "okay"; >> }; >> >> +&mixer0 { >> + status = "okay"; >> +}; >> + >> &mmc0 { >> pinctrl-names = "default"; >> pinctrl-0 = <&mmc0_pins>; >> @@ -127,6 +135,27 @@ >> status = "okay"; >> }; >> >> +&r_i2c { >> + pinctrl-names = "default"; >> + pinctrl-0 = <&r_i2c_pins_a>; >> + status = "okay"; >> + >> + anx6345: anx6345@38 { >> + compatible = "analogix,anx6345"; >> + reg = <0x38>; >> + reset-gpios = <&pio 3 24 GPIO_ACTIVE_HIGH>; /* PD24 */ >> + panel-supply = <®_dc1sw>; >> + dvdd25-supply = <®_dldo2>; >> + dvdd12-supply = <®_fldo1>; >> + >> + port { >> + anx6345_in: endpoint { >> + remote-endpoint = <&tcon0_out_anx6345>; >> + }; >> + }; >> + }; >> +}; >> + >> &r_rsb { >> status = "okay"; >> >> @@ -267,6 +296,20 @@ >> vcc-hdmi-supply = <®_dldo1>; >> }; >> >> +&tcon0 { >> + pinctrl-names = "default"; >> + pinctrl-0 = <&lcd_rgb666_pins>; >> + >> + status = "okay"; >> +}; >> + >> +&tcon0_out { >> + tcon0_out_anx6345: endpoint@0 { >> + reg = <0>; >> + remote-endpoint = <&anx6345_in>; >> + }; >> +}; >> + >> &uart0 { >> pinctrl-names = "default"; >> pinctrl-0 = <&uart0_pb_pins>;
On Thu, Oct 18, 2018 at 12:35 AM Icenowy Zheng <icenowy@aosc.io> wrote: > > This patchset brings the support for Analogix ANX6345 RGB-(e)DP bridge, > which is used by some Allwinner A64 laptops, such as Pinebook and Olimex > TERES-I. > > It reuses some definitions from the ANX78xx driver that already exists > in the kernel tree, but the driver code itself is rewritten, because the > big difference between ANX6345 and ANX78xx. > > This patchset also enables the bridge on Pinebook and TERES-I, and a > temporary workaround patch (do not merge) for the dot clock accuracy > problem of sun4i-drm. > > This patchset assumes some fixes ([1], [2] and [3]) are already > applied, without them the patchset cannot be tested on the A64 devices > mentioned above. For whole series: Tested-by: Vasily Khoruzhick <anarsoul@gmail.com> > > [1] https://patchwork.kernel.org/patch/10628827/ > [2] https://patchwork.kernel.org/patch/10628825/ > [3] https://patchwork.kernel.org/patch/10646791/ > > Chen-Yu Tsai (1): > [DO NOT MERGE] drm/sun4i: rgb: Add 5% tolerance to dot clock frequency > check > > Icenowy Zheng (8): > drm/bridge: move ANA78xx driver to analogix subdirectory > drm/bridge: split some definitions of ANX78xx to dedicated headers > drm/bridge: extract some Analogix I2C DP common code > dt-bindings: Add ANX6345 DP/eDP transmitter binding > drm/bridge: Add Analogix anx6345 support > arm64: allwinner: a64: add pinmux for RGB666 LCD > arm64: allwinner: a64: enable ANX6345 bridge on Pinebook > arm64: allwinner: a64: enable ANX6345 bridge on TERES-I > > .../bindings/display/bridge/anx6345.txt | 39 + > .../dts/allwinner/sun50i-a64-pinebook.dts | 43 + > .../boot/dts/allwinner/sun50i-a64-teres-i.dts | 40 +- > arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 9 + > drivers/gpu/drm/bridge/Kconfig | 10 - > drivers/gpu/drm/bridge/Makefile | 4 +- > drivers/gpu/drm/bridge/analogix-anx78xx.h | 719 --------------- > drivers/gpu/drm/bridge/analogix/Kconfig | 25 + > drivers/gpu/drm/bridge/analogix/Makefile | 4 + > .../drm/bridge/analogix/analogix-anx6345.c | 862 ++++++++++++++++++ > .../bridge/{ => analogix}/analogix-anx78xx.c | 146 +-- > .../drm/bridge/analogix/analogix-anx78xx.h | 265 ++++++ > .../drm/bridge/analogix/analogix-i2c-dptx.c | 169 ++++ > .../drm/bridge/analogix/analogix-i2c-dptx.h | 258 ++++++ > .../bridge/analogix/analogix-i2c-txcommon.h | 240 +++++ > drivers/gpu/drm/sun4i/sun4i_rgb.c | 5 +- > 16 files changed, 1956 insertions(+), 882 deletions(-) > create mode 100644 Documentation/devicetree/bindings/display/bridge/anx6345.txt > delete mode 100644 drivers/gpu/drm/bridge/analogix-anx78xx.h > create mode 100644 drivers/gpu/drm/bridge/analogix/analogix-anx6345.c > rename drivers/gpu/drm/bridge/{ => analogix}/analogix-anx78xx.c (90%) > create mode 100644 drivers/gpu/drm/bridge/analogix/analogix-anx78xx.h > create mode 100644 drivers/gpu/drm/bridge/analogix/analogix-i2c-dptx.c > create mode 100644 drivers/gpu/drm/bridge/analogix/analogix-i2c-dptx.h > create mode 100644 drivers/gpu/drm/bridge/analogix/analogix-i2c-txcommon.h > > -- > 2.18.1 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Thu, Oct 18, 2018 at 4:31 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > Given that the tolerance is a property of the panel or bridge, I agree with > Daniel that it should be implemented there, or at least in cooperation with > drm_panel and drm_bridge. Clock tolerance is not specified in ANX6345 datasheet. > Semi-related information, I think the CEA and VESA standards allow a 0.5% > clock tolerance. What is the maximum clock frequency deviation required for > this platform ? This particular platform requires ~1% deviation. E.g. on Pinebook with 768p panel: requested clock: 73.0 MHz, real clock: 72.296296 MHz (0.96%) on Pinebook with 1080p panel: requested clock: 138.5 MHz, real clock: 138.461538 MHz (0.03%) So unfortunately 0.5% is not enough. Regards, Vasily
On Thu, Oct 18, 2018 at 03:33:18PM +0800, Icenowy Zheng wrote: > This patchset brings the support for Analogix ANX6345 RGB-(e)DP bridge, > which is used by some Allwinner A64 laptops, such as Pinebook and Olimex > TERES-I. > So what's the status here? I'm working on the Teres-I and I find myself recreating the chunks in this patchset almost verbatim (only DT so far), one by one, so there must be something right about them ;-) Whose turn is it? Torsten
On Mon, Feb 4, 2019 at 4:22 AM Torsten Duwe <duwe@lst.de> wrote: > > On Thu, Oct 18, 2018 at 03:33:18PM +0800, Icenowy Zheng wrote: > > This patchset brings the support for Analogix ANX6345 RGB-(e)DP bridge, > > which is used by some Allwinner A64 laptops, such as Pinebook and Olimex > > TERES-I. > > > > So what's the status here? I'm working on the Teres-I and I find myself > recreating the chunks in this patchset almost verbatim (only DT so far), > one by one, so there must be something right about them ;-) > > Whose turn is it? I've sent v2 yesterday, however I tested it only on Pinebook. > > Torsten > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
First thing that struck me is that the chip's reset is actually low active
reset-gpios = <&pio 3 24 GPIO_ACTIVE_LOW>; /* PD24 */
^^^^
(please correct this in patches 11 and 12)
Consequently, you're using inverted values here in the driver:
> +static void anx6345_poweron(struct anx6345 *anx6345)
> +{
[...]
> + gpiod_set_value_cansleep(pdata->gpiod_reset, 0);
0 = reset on, ok.
> + usleep_range(1000, 2000);
> +
> + gpiod_set_value_cansleep(pdata->gpiod_reset, 1);
1 = reset off, also fine.
> +
> + /* Power on registers module */
> + anx6345_set_bits(anx6345->map[I2C_IDX_TXCOM], SP_POWERDOWN_CTRL_REG,
> + SP_HDCP_PD | SP_AUDIO_PD | SP_VIDEO_PD | SP_LINK_PD);
> + anx6345_clear_bits(anx6345->map[I2C_IDX_TXCOM], SP_POWERDOWN_CTRL_REG,
> + SP_REGISTER_PD | SP_TOTAL_PD);
> +
> + anx6345->powered = true;
> +}
> +
> +static void anx6345_poweroff(struct anx6345 *anx6345)
> +{
> + struct anx6345_platform_data *pdata = &anx6345->pdata;
> + int err;
> +
> + if (WARN_ON(!anx6345->powered))
> + return;
> +
> + gpiod_set_value_cansleep(pdata->gpiod_reset, 1);
> + usleep_range(1000, 2000);
This one got me a bit confused. Assert or deassert reset (again) before
poweroff?
Please stick to the logical value of the reset line.
Torsten
This patchset brings the support for Analogix ANX6345 RGB-(e)DP bridge, which is used by some Allwinner A64 laptops, such as Pinebook and Olimex TERES-I. It reuses some definitions from the ANX78xx driver that already exists in the kernel tree, but the driver code itself is rewritten, because the big difference between ANX6345 and ANX78xx. This patchset also enables the bridge on Pinebook and TERES-I, and a temporary workaround patch (do not merge) for the dot clock accuracy problem of sun4i-drm. This patchset assumes some fixes ([1], [2] and [3]) are already applied, without them the patchset cannot be tested on the A64 devices mentioned above. [1] https://patchwork.kernel.org/patch/10628827/ [2] https://patchwork.kernel.org/patch/10628825/ [3] https://patchwork.kernel.org/patch/10646791/ Chen-Yu Tsai (1): [DO NOT MERGE] drm/sun4i: rgb: Add 5% tolerance to dot clock frequency check Icenowy Zheng (8): drm/bridge: move ANA78xx driver to analogix subdirectory drm/bridge: split some definitions of ANX78xx to dedicated headers drm/bridge: extract some Analogix I2C DP common code dt-bindings: Add ANX6345 DP/eDP transmitter binding drm/bridge: Add Analogix anx6345 support arm64: allwinner: a64: add pinmux for RGB666 LCD arm64: allwinner: a64: enable ANX6345 bridge on Pinebook arm64: allwinner: a64: enable ANX6345 bridge on TERES-I .../bindings/display/bridge/anx6345.txt | 39 + .../dts/allwinner/sun50i-a64-pinebook.dts | 43 + .../boot/dts/allwinner/sun50i-a64-teres-i.dts | 40 +- arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 9 + drivers/gpu/drm/bridge/Kconfig | 10 - drivers/gpu/drm/bridge/Makefile | 4 +- drivers/gpu/drm/bridge/analogix-anx78xx.h | 719 --------------- drivers/gpu/drm/bridge/analogix/Kconfig | 25 + drivers/gpu/drm/bridge/analogix/Makefile | 4 + .../drm/bridge/analogix/analogix-anx6345.c | 862 ++++++++++++++++++ .../bridge/{ => analogix}/analogix-anx78xx.c | 146 +-- .../drm/bridge/analogix/analogix-anx78xx.h | 265 ++++++ .../drm/bridge/analogix/analogix-i2c-dptx.c | 169 ++++ .../drm/bridge/analogix/analogix-i2c-dptx.h | 258 ++++++ .../bridge/analogix/analogix-i2c-txcommon.h | 240 +++++ drivers/gpu/drm/sun4i/sun4i_rgb.c | 5 +- 16 files changed, 1956 insertions(+), 882 deletions(-) create mode 100644 Documentation/devicetree/bindings/display/bridge/anx6345.txt delete mode 100644 drivers/gpu/drm/bridge/analogix-anx78xx.h create mode 100644 drivers/gpu/drm/bridge/analogix/analogix-anx6345.c rename drivers/gpu/drm/bridge/{ => analogix}/analogix-anx78xx.c (90%) create mode 100644 drivers/gpu/drm/bridge/analogix/analogix-anx78xx.h create mode 100644 drivers/gpu/drm/bridge/analogix/analogix-i2c-dptx.c create mode 100644 drivers/gpu/drm/bridge/analogix/analogix-i2c-dptx.h create mode 100644 drivers/gpu/drm/bridge/analogix/analogix-i2c-txcommon.h