* [RFC/RFT PATCH 0/4] drm/bridge: dw-hdmi: Add support for Custom PHYs @ 2017-01-17 12:31 Neil Armstrong 2017-01-17 12:31 ` [RFC/RFT PATCH 1/4] drm/bridge: dw-hdmi: Switch to regmap for register access Neil Armstrong ` (3 more replies) 0 siblings, 4 replies; 21+ messages in thread From: Neil Armstrong @ 2017-01-17 12:31 UTC (permalink / raw) To: dri-devel, laurent.pinchart+renesas Cc: Neil Armstrong, Jose.Abreu, kieran.bingham, linux-amlogic, linux-kernel The Amlogic GX SoCs implements a Synopsys DesignWare HDMI TX Controller in combination with a very custom PHY. Thanks to Laurent Pinchart's changes, the HW report the following : Detected HDMI TX controller v2.01a with HDCP (Vendor PHY) The following differs from common PHY integration as managed in the current driver : - Amlogic PHY is not configured through the internal I2C link - Amlogic PHY do not use the ENTMDS, SVSRET, PDDQ, ... signals from the controller - Amlogic PHY do not export HPD ands RxSense signals to the controller And finally, concerning the controller integration : - the Controller registers are not flat memory-mapped, and uses an addr+read/write register pair to write all registers. - Inputs only YUV444 pixel data This is why the following patchset implements : - Conversion to regmap for register access - Add more callbacks ops to handle Custom PHYs - Configure the Input format from the plat_data - Fixes a bug that considers the input to be always RBG and sends bad pixel format to a DVI sink by disabling CSC This patchset makes the Amlogix GX SoCs HDMI output successfully work, but I do not have access to Renesas, i.MX or RockChip SoCs to test against potentiel regressions, like the regmap conversion. This patchset is based on the latest Laurent Pinchart dw-hdmi serie at [1]. [1] http://lkml.kernel.org/r/20170117082910.27023-1-laurent.pinchart+renesas@ideasonboard.com Neil Armstrong (4): drm/bridge: dw-hdmi: Switch to regmap for register access drm/bridge: dw-hdmi: Add support for custom PHY handling drm/bridge: dw-hdmi: Enable CSC even for DVI drm/bridge: dw-hdmi: Take input format from plat_data drivers/gpu/drm/bridge/dw-hdmi.c | 194 +++++++++++++++++++++++++-------------- include/drm/bridge/dw_hdmi.h | 18 ++++ 2 files changed, 143 insertions(+), 69 deletions(-) -- 1.9.1 ^ permalink raw reply [flat|nested] 21+ messages in thread
* [RFC/RFT PATCH 1/4] drm/bridge: dw-hdmi: Switch to regmap for register access 2017-01-17 12:31 [RFC/RFT PATCH 0/4] drm/bridge: dw-hdmi: Add support for Custom PHYs Neil Armstrong @ 2017-01-17 12:31 ` Neil Armstrong 2017-01-17 14:39 ` Laurent Pinchart 2017-01-17 12:31 ` [RFC/RFT PATCH 2/4] drm/bridge: dw-hdmi: Add support for custom PHY handling Neil Armstrong ` (2 subsequent siblings) 3 siblings, 1 reply; 21+ messages in thread From: Neil Armstrong @ 2017-01-17 12:31 UTC (permalink / raw) To: dri-devel, laurent.pinchart+renesas Cc: Neil Armstrong, Jose.Abreu, kieran.bingham, linux-amlogic, linux-kernel The Synopsys Designware HDMI TX Controller does not enforce register access on platforms instanciating it. The current driver supports two different types of memory-mapped flat register access, but in order to support the Amlogic Meson SoCs integration, and provide a more generic way to handle all sorts of register mapping, switch the register access to use the regmap infrastructure. In the case of the registers are not flat memory-mapped or does not conform at the actual driver implementation, a regmap struct can be given in the plat_data and be used at probe or bind. Since the AHB audio driver only uses direct memory access, using regmap only allows the I2S audio driver to be registered. Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> --- drivers/gpu/drm/bridge/dw-hdmi.c | 105 +++++++++++++++++++++------------------ include/drm/bridge/dw_hdmi.h | 1 + 2 files changed, 57 insertions(+), 49 deletions(-) diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c index ca9d0ce..13747fe 100644 --- a/drivers/gpu/drm/bridge/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/dw-hdmi.c @@ -20,6 +20,7 @@ #include <linux/mutex.h> #include <linux/of_device.h> #include <linux/spinlock.h> +#include <linux/regmap.h> #include <drm/drm_of.h> #include <drm/drmP.h> @@ -167,8 +168,7 @@ struct dw_hdmi { unsigned int audio_n; bool audio_enable; - void (*write)(struct dw_hdmi *hdmi, u8 val, int offset); - u8 (*read)(struct dw_hdmi *hdmi, int offset); + struct regmap *regm; }; #define HDMI_IH_PHY_STAT0_RX_SENSE \ @@ -179,42 +179,23 @@ struct dw_hdmi { (HDMI_PHY_RX_SENSE0 | HDMI_PHY_RX_SENSE1 | \ HDMI_PHY_RX_SENSE2 | HDMI_PHY_RX_SENSE3) -static void dw_hdmi_writel(struct dw_hdmi *hdmi, u8 val, int offset) -{ - writel(val, hdmi->regs + (offset << 2)); -} - -static u8 dw_hdmi_readl(struct dw_hdmi *hdmi, int offset) -{ - return readl(hdmi->regs + (offset << 2)); -} - -static void dw_hdmi_writeb(struct dw_hdmi *hdmi, u8 val, int offset) -{ - writeb(val, hdmi->regs + offset); -} - -static u8 dw_hdmi_readb(struct dw_hdmi *hdmi, int offset) -{ - return readb(hdmi->regs + offset); -} - static inline void hdmi_writeb(struct dw_hdmi *hdmi, u8 val, int offset) { - hdmi->write(hdmi, val, offset); + regmap_write(hdmi->regm, offset, val); } static inline u8 hdmi_readb(struct dw_hdmi *hdmi, int offset) { - return hdmi->read(hdmi, offset); + unsigned int val = 0; + + regmap_read(hdmi->regm, offset, &val); + + return val; } static void hdmi_modb(struct dw_hdmi *hdmi, u8 data, u8 mask, unsigned reg) { - u8 val = hdmi_readb(hdmi, reg) & ~mask; - - val |= data & mask; - hdmi_writeb(hdmi, val, reg); + regmap_update_bits(hdmi->regm, reg, mask, data); } static void hdmi_mask_writeb(struct dw_hdmi *hdmi, u8 data, unsigned int reg, @@ -1949,6 +1930,21 @@ static int dw_hdmi_detect_phy(struct dw_hdmi *hdmi) return 0; } +static struct regmap_config hdmi_regmap_8bit_config = { + .reg_bits = 32, + .val_bits = 8, + .reg_stride = 1, + .max_register = HDMI_I2CM_FS_SCL_LCNT_0_ADDR, +}; + +static struct regmap_config hdmi_regmap_32bit_config = { + .reg_bits = 8, + .pad_bits = 24, + .val_bits = 32, + .reg_stride = 4, + .max_register = HDMI_I2CM_FS_SCL_LCNT_0_ADDR, +}; + static struct dw_hdmi * __dw_hdmi_probe(struct platform_device *pdev, const struct dw_hdmi_plat_data *plat_data) @@ -1958,7 +1954,8 @@ static int dw_hdmi_detect_phy(struct dw_hdmi *hdmi) struct platform_device_info pdevinfo; struct device_node *ddc_node; struct dw_hdmi *hdmi; - struct resource *iores; + struct regmap_config *reg_config = &hdmi_regmap_8bit_config; + struct resource *iores = NULL; int irq; int ret; u32 val = 1; @@ -1982,20 +1979,21 @@ static int dw_hdmi_detect_phy(struct dw_hdmi *hdmi) mutex_init(&hdmi->audio_mutex); spin_lock_init(&hdmi->audio_lock); - of_property_read_u32(np, "reg-io-width", &val); - - switch (val) { - case 4: - hdmi->write = dw_hdmi_writel; - hdmi->read = dw_hdmi_readl; - break; - case 1: - hdmi->write = dw_hdmi_writeb; - hdmi->read = dw_hdmi_readb; - break; - default: - dev_err(dev, "reg-io-width must be 1 or 4\n"); - return ERR_PTR(-EINVAL); + if (plat_data->regm) + hdmi->regm = plat_data->regm; + else { + of_property_read_u32(np, "reg-io-width", &val); + switch (val) { + case 4: + reg_config = &hdmi_regmap_32bit_config; + break; + case 1: + reg_config = &hdmi_regmap_8bit_config; + break; + default: + dev_err(dev, "reg-io-width must be 1 or 4\n"); + return ERR_PTR(-EINVAL); + } } ddc_node = of_parse_phandle(np, "ddc-i2c-bus", 0); @@ -2011,11 +2009,20 @@ static int dw_hdmi_detect_phy(struct dw_hdmi *hdmi) dev_dbg(hdmi->dev, "no ddc property found\n"); } - iores = platform_get_resource(pdev, IORESOURCE_MEM, 0); - hdmi->regs = devm_ioremap_resource(dev, iores); - if (IS_ERR(hdmi->regs)) { - ret = PTR_ERR(hdmi->regs); - goto err_res; + if (!plat_data->regm) { + iores = platform_get_resource(pdev, IORESOURCE_MEM, 0); + hdmi->regs = devm_ioremap_resource(dev, iores); + if (IS_ERR(hdmi->regs)) { + ret = PTR_ERR(hdmi->regs); + goto err_res; + } + + hdmi->regm = devm_regmap_init_mmio(dev, hdmi->regs, reg_config); + if (IS_ERR(hdmi->regm)) { + dev_err(dev, "Failed to configure regmap\n"); + ret = PTR_ERR(hdmi->regm); + goto err_res; + } } hdmi->isfr_clk = devm_clk_get(hdmi->dev, "isfr"); @@ -2123,7 +2130,7 @@ static int dw_hdmi_detect_phy(struct dw_hdmi *hdmi) config0 = hdmi_readb(hdmi, HDMI_CONFIG0_ID); config3 = hdmi_readb(hdmi, HDMI_CONFIG3_ID); - if (config3 & HDMI_CONFIG3_AHBAUDDMA) { + if (iores && config3 & HDMI_CONFIG3_AHBAUDDMA) { struct dw_hdmi_audio_data audio; audio.phys = iores->start; diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h index 735a8ab..163842d 100644 --- a/include/drm/bridge/dw_hdmi.h +++ b/include/drm/bridge/dw_hdmi.h @@ -60,6 +60,7 @@ struct dw_hdmi_plat_data { unsigned long mpixelclock); enum drm_mode_status (*mode_valid)(struct drm_connector *connector, struct drm_display_mode *mode); + struct regmap *regm; }; int dw_hdmi_probe(struct platform_device *pdev, -- 1.9.1 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [RFC/RFT PATCH 1/4] drm/bridge: dw-hdmi: Switch to regmap for register access 2017-01-17 12:31 ` [RFC/RFT PATCH 1/4] drm/bridge: dw-hdmi: Switch to regmap for register access Neil Armstrong @ 2017-01-17 14:39 ` Laurent Pinchart 2017-01-20 15:12 ` Neil Armstrong 0 siblings, 1 reply; 21+ messages in thread From: Laurent Pinchart @ 2017-01-17 14:39 UTC (permalink / raw) To: Neil Armstrong Cc: dri-devel, laurent.pinchart+renesas, Jose.Abreu, kieran.bingham, linux-amlogic, linux-kernel Hi Neil, Thank you for the patch. On Tuesday 17 Jan 2017 13:31:31 Neil Armstrong wrote: > The Synopsys Designware HDMI TX Controller does not enforce register access > on platforms instanciating it. > The current driver supports two different types of memory-mapped flat > register access, but in order to support the Amlogic Meson SoCs integration, > and provide a more generic way to handle all sorts of register mapping, > switch the register access to use the regmap infrastructure. > > In the case of the registers are not flat memory-mapped or does not conform s/does/do/ > at the actual driver implementation, a regmap struct can be given in the s/at the actual/to the current/ ? > plat_data and be used at probe or bind. > > Since the AHB audio driver only uses direct memory access, using regmap only > allows the I2S audio driver to be registered. This sounds a bit unclear to me, how about "[...], only allow the I2C audio driver to be registered if the device is directly memory-mapped." ? > Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> > --- > drivers/gpu/drm/bridge/dw-hdmi.c | 105 +++++++++++++++++++----------------- > include/drm/bridge/dw_hdmi.h | 1 + > 2 files changed, 57 insertions(+), 49 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c > b/drivers/gpu/drm/bridge/dw-hdmi.c index ca9d0ce..13747fe 100644 > --- a/drivers/gpu/drm/bridge/dw-hdmi.c > +++ b/drivers/gpu/drm/bridge/dw-hdmi.c > @@ -20,6 +20,7 @@ > #include <linux/mutex.h> > #include <linux/of_device.h> > #include <linux/spinlock.h> > +#include <linux/regmap.h> Could you please keep the headers alphabetically sorted ? > #include <drm/drm_of.h> > #include <drm/drmP.h> > @@ -167,8 +168,7 @@ struct dw_hdmi { > unsigned int audio_n; > bool audio_enable; > > - void (*write)(struct dw_hdmi *hdmi, u8 val, int offset); > - u8 (*read)(struct dw_hdmi *hdmi, int offset); > + struct regmap *regm; > }; > > #define HDMI_IH_PHY_STAT0_RX_SENSE \ > @@ -179,42 +179,23 @@ struct dw_hdmi { > (HDMI_PHY_RX_SENSE0 | HDMI_PHY_RX_SENSE1 | \ > HDMI_PHY_RX_SENSE2 | HDMI_PHY_RX_SENSE3) > > -static void dw_hdmi_writel(struct dw_hdmi *hdmi, u8 val, int offset) > -{ > - writel(val, hdmi->regs + (offset << 2)); > -} > - > -static u8 dw_hdmi_readl(struct dw_hdmi *hdmi, int offset) > -{ > - return readl(hdmi->regs + (offset << 2)); > -} > - > -static void dw_hdmi_writeb(struct dw_hdmi *hdmi, u8 val, int offset) > -{ > - writeb(val, hdmi->regs + offset); > -} > - > -static u8 dw_hdmi_readb(struct dw_hdmi *hdmi, int offset) > -{ > - return readb(hdmi->regs + offset); > -} > - > static inline void hdmi_writeb(struct dw_hdmi *hdmi, u8 val, int offset) Not related to this patch, but the value, offset order of arguments to the write function has been making me cringe since the very first time I read the code. I wonder if modifying this would be accepted. > { > - hdmi->write(hdmi, val, offset); > + regmap_write(hdmi->regm, offset, val); > } > > static inline u8 hdmi_readb(struct dw_hdmi *hdmi, int offset) > { > - return hdmi->read(hdmi, offset); > + unsigned int val = 0; > + > + regmap_read(hdmi->regm, offset, &val); > + > + return val; > } > > static void hdmi_modb(struct dw_hdmi *hdmi, u8 data, u8 mask, unsigned reg) > { > - u8 val = hdmi_readb(hdmi, reg) & ~mask; > - > - val |= data & mask; > - hdmi_writeb(hdmi, val, reg); > + regmap_update_bits(hdmi->regm, reg, mask, data); > } > > static void hdmi_mask_writeb(struct dw_hdmi *hdmi, u8 data, unsigned int > reg, @@ -1949,6 +1930,21 @@ static int dw_hdmi_detect_phy(struct dw_hdmi > *hdmi) return 0; > } > > +static struct regmap_config hdmi_regmap_8bit_config = { > + .reg_bits = 32, > + .val_bits = 8, > + .reg_stride = 1, > + .max_register = HDMI_I2CM_FS_SCL_LCNT_0_ADDR, > +}; > + > +static struct regmap_config hdmi_regmap_32bit_config = { > + .reg_bits = 8, > + .pad_bits = 24, > + .val_bits = 32, > + .reg_stride = 4, > + .max_register = HDMI_I2CM_FS_SCL_LCNT_0_ADDR, > +}; I believe you can make these const. > static struct dw_hdmi * > __dw_hdmi_probe(struct platform_device *pdev, > const struct dw_hdmi_plat_data *plat_data) > @@ -1958,7 +1954,8 @@ static int dw_hdmi_detect_phy(struct dw_hdmi *hdmi) > struct platform_device_info pdevinfo; > struct device_node *ddc_node; > struct dw_hdmi *hdmi; > - struct resource *iores; > + struct regmap_config *reg_config = &hdmi_regmap_8bit_config; No need to assign a value at declaration time (unless the compiler is not smart enough and complains, or is smarter than me and finds a problem where I don't). > + struct resource *iores = NULL; > int irq; > int ret; > u32 val = 1; > @@ -1982,20 +1979,21 @@ static int dw_hdmi_detect_phy(struct dw_hdmi *hdmi) > mutex_init(&hdmi->audio_mutex); > spin_lock_init(&hdmi->audio_lock); > > - of_property_read_u32(np, "reg-io-width", &val); > - > - switch (val) { > - case 4: > - hdmi->write = dw_hdmi_writel; > - hdmi->read = dw_hdmi_readl; > - break; > - case 1: > - hdmi->write = dw_hdmi_writeb; > - hdmi->read = dw_hdmi_readb; > - break; > - default: > - dev_err(dev, "reg-io-width must be 1 or 4\n"); > - return ERR_PTR(-EINVAL); > + if (plat_data->regm) > + hdmi->regm = plat_data->regm; You need curly braces around this statement. > + else { > + of_property_read_u32(np, "reg-io-width", &val); > + switch (val) { > + case 4: > + reg_config = &hdmi_regmap_32bit_config; > + break; > + case 1: > + reg_config = &hdmi_regmap_8bit_config; > + break; > + default: > + dev_err(dev, "reg-io-width must be 1 or 4\n"); > + return ERR_PTR(-EINVAL); > + } > } > > ddc_node = of_parse_phandle(np, "ddc-i2c-bus", 0); > @@ -2011,11 +2009,20 @@ static int dw_hdmi_detect_phy(struct dw_hdmi *hdmi) > dev_dbg(hdmi->dev, "no ddc property found\n"); > } > > - iores = platform_get_resource(pdev, IORESOURCE_MEM, 0); > - hdmi->regs = devm_ioremap_resource(dev, iores); > - if (IS_ERR(hdmi->regs)) { > - ret = PTR_ERR(hdmi->regs); > - goto err_res; > + if (!plat_data->regm) { > + iores = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + hdmi->regs = devm_ioremap_resource(dev, iores); > + if (IS_ERR(hdmi->regs)) { > + ret = PTR_ERR(hdmi->regs); > + goto err_res; > + } > + > + hdmi->regm = devm_regmap_init_mmio(dev, hdmi->regs, reg_config); > + if (IS_ERR(hdmi->regm)) { > + dev_err(dev, "Failed to configure regmap\n"); > + ret = PTR_ERR(hdmi->regm); > + goto err_res; > + } > } > > hdmi->isfr_clk = devm_clk_get(hdmi->dev, "isfr"); > @@ -2123,7 +2130,7 @@ static int dw_hdmi_detect_phy(struct dw_hdmi *hdmi) > config0 = hdmi_readb(hdmi, HDMI_CONFIG0_ID); > config3 = hdmi_readb(hdmi, HDMI_CONFIG3_ID); > > - if (config3 & HDMI_CONFIG3_AHBAUDDMA) { > + if (iores && config3 & HDMI_CONFIG3_AHBAUDDMA) { You test !plat->regm above, and iores here. How about standardizing that ? If you test for !plat->regm here, you won't have to initialize iores to NULL. Apart from these small issues the patch looks good to me. > struct dw_hdmi_audio_data audio; > > audio.phys = iores->start; > diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h > index 735a8ab..163842d 100644 > --- a/include/drm/bridge/dw_hdmi.h > +++ b/include/drm/bridge/dw_hdmi.h > @@ -60,6 +60,7 @@ struct dw_hdmi_plat_data { > unsigned long mpixelclock); > enum drm_mode_status (*mode_valid)(struct drm_connector *connector, > struct drm_display_mode *mode); > + struct regmap *regm; > }; > > int dw_hdmi_probe(struct platform_device *pdev, -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC/RFT PATCH 1/4] drm/bridge: dw-hdmi: Switch to regmap for register access 2017-01-17 14:39 ` Laurent Pinchart @ 2017-01-20 15:12 ` Neil Armstrong 0 siblings, 0 replies; 21+ messages in thread From: Neil Armstrong @ 2017-01-20 15:12 UTC (permalink / raw) To: Laurent Pinchart Cc: dri-devel, laurent.pinchart+renesas, Jose.Abreu, kieran.bingham, linux-amlogic, linux-kernel, Mark Brown On 01/17/2017 03:39 PM, Laurent Pinchart wrote: > Hi Neil, > > Thank you for the patch. > > On Tuesday 17 Jan 2017 13:31:31 Neil Armstrong wrote: >> The Synopsys Designware HDMI TX Controller does not enforce register access >> on platforms instanciating it. >> The current driver supports two different types of memory-mapped flat >> register access, but in order to support the Amlogic Meson SoCs integration, >> and provide a more generic way to handle all sorts of register mapping, >> switch the register access to use the regmap infrastructure. >> >> In the case of the registers are not flat memory-mapped or does not conform > > s/does/do/ > >> at the actual driver implementation, a regmap struct can be given in the > > s/at the actual/to the current/ ? > >> plat_data and be used at probe or bind. >> >> Since the AHB audio driver only uses direct memory access, using regmap only >> allows the I2S audio driver to be registered. > > This sounds a bit unclear to me, how about "[...], only allow the I2C audio > driver to be registered if the device is directly memory-mapped." ? > >> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> >> --- >> drivers/gpu/drm/bridge/dw-hdmi.c | 105 +++++++++++++++++++----------------- >> include/drm/bridge/dw_hdmi.h | 1 + >> 2 files changed, 57 insertions(+), 49 deletions(-) >> >> diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c >> b/drivers/gpu/drm/bridge/dw-hdmi.c index ca9d0ce..13747fe 100644 >> --- a/drivers/gpu/drm/bridge/dw-hdmi.c >> +++ b/drivers/gpu/drm/bridge/dw-hdmi.c >> @@ -20,6 +20,7 @@ >> #include <linux/mutex.h> >> #include <linux/of_device.h> >> #include <linux/spinlock.h> >> +#include <linux/regmap.h> > > Could you please keep the headers alphabetically sorted ? > >> #include <drm/drm_of.h> >> #include <drm/drmP.h> >> @@ -167,8 +168,7 @@ struct dw_hdmi { >> unsigned int audio_n; >> bool audio_enable; >> >> - void (*write)(struct dw_hdmi *hdmi, u8 val, int offset); >> - u8 (*read)(struct dw_hdmi *hdmi, int offset); >> + struct regmap *regm; >> }; >> >> #define HDMI_IH_PHY_STAT0_RX_SENSE \ >> @@ -179,42 +179,23 @@ struct dw_hdmi { >> (HDMI_PHY_RX_SENSE0 | HDMI_PHY_RX_SENSE1 | \ >> HDMI_PHY_RX_SENSE2 | HDMI_PHY_RX_SENSE3) >> >> -static void dw_hdmi_writel(struct dw_hdmi *hdmi, u8 val, int offset) >> -{ >> - writel(val, hdmi->regs + (offset << 2)); >> -} >> - >> -static u8 dw_hdmi_readl(struct dw_hdmi *hdmi, int offset) >> -{ >> - return readl(hdmi->regs + (offset << 2)); >> -} >> - >> -static void dw_hdmi_writeb(struct dw_hdmi *hdmi, u8 val, int offset) >> -{ >> - writeb(val, hdmi->regs + offset); >> -} >> - >> -static u8 dw_hdmi_readb(struct dw_hdmi *hdmi, int offset) >> -{ >> - return readb(hdmi->regs + offset); >> -} >> - >> static inline void hdmi_writeb(struct dw_hdmi *hdmi, u8 val, int offset) > > Not related to this patch, but the value, offset order of arguments to the > write function has been making me cringe since the very first time I read the > code. I wonder if modifying this would be accepted. > >> { >> - hdmi->write(hdmi, val, offset); >> + regmap_write(hdmi->regm, offset, val); >> } >> >> static inline u8 hdmi_readb(struct dw_hdmi *hdmi, int offset) >> { >> - return hdmi->read(hdmi, offset); >> + unsigned int val = 0; >> + >> + regmap_read(hdmi->regm, offset, &val); >> + >> + return val; >> } >> >> static void hdmi_modb(struct dw_hdmi *hdmi, u8 data, u8 mask, unsigned reg) >> { >> - u8 val = hdmi_readb(hdmi, reg) & ~mask; >> - >> - val |= data & mask; >> - hdmi_writeb(hdmi, val, reg); >> + regmap_update_bits(hdmi->regm, reg, mask, data); >> } >> >> static void hdmi_mask_writeb(struct dw_hdmi *hdmi, u8 data, unsigned int >> reg, @@ -1949,6 +1930,21 @@ static int dw_hdmi_detect_phy(struct dw_hdmi >> *hdmi) return 0; >> } >> >> +static struct regmap_config hdmi_regmap_8bit_config = { >> + .reg_bits = 32, >> + .val_bits = 8, >> + .reg_stride = 1, >> + .max_register = HDMI_I2CM_FS_SCL_LCNT_0_ADDR, >> +}; >> + >> +static struct regmap_config hdmi_regmap_32bit_config = { >> + .reg_bits = 8, >> + .pad_bits = 24, >> + .val_bits = 32, >> + .reg_stride = 4, >> + .max_register = HDMI_I2CM_FS_SCL_LCNT_0_ADDR, >> +}; > > I believe you can make these const. > >> static struct dw_hdmi * >> __dw_hdmi_probe(struct platform_device *pdev, >> const struct dw_hdmi_plat_data *plat_data) >> @@ -1958,7 +1954,8 @@ static int dw_hdmi_detect_phy(struct dw_hdmi *hdmi) >> struct platform_device_info pdevinfo; >> struct device_node *ddc_node; >> struct dw_hdmi *hdmi; >> - struct resource *iores; >> + struct regmap_config *reg_config = &hdmi_regmap_8bit_config; > > No need to assign a value at declaration time (unless the compiler is not > smart enough and complains, or is smarter than me and finds a problem where I > don't). > >> + struct resource *iores = NULL; >> int irq; >> int ret; >> u32 val = 1; >> @@ -1982,20 +1979,21 @@ static int dw_hdmi_detect_phy(struct dw_hdmi *hdmi) >> mutex_init(&hdmi->audio_mutex); >> spin_lock_init(&hdmi->audio_lock); >> >> - of_property_read_u32(np, "reg-io-width", &val); >> - >> - switch (val) { >> - case 4: >> - hdmi->write = dw_hdmi_writel; >> - hdmi->read = dw_hdmi_readl; >> - break; >> - case 1: >> - hdmi->write = dw_hdmi_writeb; >> - hdmi->read = dw_hdmi_readb; >> - break; >> - default: >> - dev_err(dev, "reg-io-width must be 1 or 4\n"); >> - return ERR_PTR(-EINVAL); >> + if (plat_data->regm) >> + hdmi->regm = plat_data->regm; > > You need curly braces around this statement. > >> + else { >> + of_property_read_u32(np, "reg-io-width", &val); >> + switch (val) { >> + case 4: >> + reg_config = &hdmi_regmap_32bit_config; >> + break; >> + case 1: >> + reg_config = &hdmi_regmap_8bit_config; >> + break; >> + default: >> + dev_err(dev, "reg-io-width must be 1 or 4\n"); >> + return ERR_PTR(-EINVAL); >> + } >> } >> >> ddc_node = of_parse_phandle(np, "ddc-i2c-bus", 0); >> @@ -2011,11 +2009,20 @@ static int dw_hdmi_detect_phy(struct dw_hdmi *hdmi) >> dev_dbg(hdmi->dev, "no ddc property found\n"); >> } >> >> - iores = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> - hdmi->regs = devm_ioremap_resource(dev, iores); >> - if (IS_ERR(hdmi->regs)) { >> - ret = PTR_ERR(hdmi->regs); >> - goto err_res; >> + if (!plat_data->regm) { >> + iores = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> + hdmi->regs = devm_ioremap_resource(dev, iores); >> + if (IS_ERR(hdmi->regs)) { >> + ret = PTR_ERR(hdmi->regs); >> + goto err_res; >> + } >> + >> + hdmi->regm = devm_regmap_init_mmio(dev, hdmi->regs, > reg_config); >> + if (IS_ERR(hdmi->regm)) { >> + dev_err(dev, "Failed to configure regmap\n"); >> + ret = PTR_ERR(hdmi->regm); >> + goto err_res; >> + } >> } >> >> hdmi->isfr_clk = devm_clk_get(hdmi->dev, "isfr"); >> @@ -2123,7 +2130,7 @@ static int dw_hdmi_detect_phy(struct dw_hdmi *hdmi) >> config0 = hdmi_readb(hdmi, HDMI_CONFIG0_ID); >> config3 = hdmi_readb(hdmi, HDMI_CONFIG3_ID); >> >> - if (config3 & HDMI_CONFIG3_AHBAUDDMA) { >> + if (iores && config3 & HDMI_CONFIG3_AHBAUDDMA) { > > You test !plat->regm above, and iores here. How about standardizing that ? If > you test for !plat->regm here, you won't have to initialize iores to NULL. > > Apart from these small issues the patch looks good to me. > >> struct dw_hdmi_audio_data audio; >> >> audio.phys = iores->start; >> diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h >> index 735a8ab..163842d 100644 >> --- a/include/drm/bridge/dw_hdmi.h >> +++ b/include/drm/bridge/dw_hdmi.h >> @@ -60,6 +60,7 @@ struct dw_hdmi_plat_data { >> unsigned long mpixelclock); >> enum drm_mode_status (*mode_valid)(struct drm_connector *connector, >> struct drm_display_mode *mode); >> + struct regmap *regm; >> }; >> >> int dw_hdmi_probe(struct platform_device *pdev, > Hi All, The actual 4bytes regmap config actually fails on rk3288 because regmap bypasses all modification of the reg address because of the regmap_mmio implementation. The only remaining way is to keep a reg_shift in in the hdmi context, see the patch below. With such change, on rk3288 : Tested-by: Neil Armstrong <narmstrong@baylibre.com> " [ 10.400294] dwhdmi-rockchip ff980000.hdmi: Detected HDMI TX controller v2.00a with HDCP (DWC MHL PHY) " Neil -><------------------ diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c index 8b35df5..563647f 100644 --- a/drivers/gpu/drm/bridge/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/dw-hdmi.c @@ -168,6 +168,7 @@ struct dw_hdmi { unsigned int audio_n; bool audio_enable; + unsigned int reg_shift; struct regmap *regm; }; @@ -181,21 +182,21 @@ struct dw_hdmi { static inline void hdmi_writeb(struct dw_hdmi *hdmi, u8 val, int offset) { - regmap_write(hdmi->regm, offset, val); + regmap_write(hdmi->regm, offset << hdmi->reg_shift, val); } static inline u8 hdmi_readb(struct dw_hdmi *hdmi, int offset) { unsigned int val = 0; - regmap_read(hdmi->regm, offset, &val); + regmap_read(hdmi->regm, offset << hdmi->reg_shift, &val); return val; } static void hdmi_modb(struct dw_hdmi *hdmi, u8 data, u8 mask, unsigned reg) { - regmap_update_bits(hdmi->regm, reg, mask, data); + regmap_update_bits(hdmi->regm, reg << hdmi->reg_shift, mask, data); } static void hdmi_mask_writeb(struct dw_hdmi *hdmi, u8 data, unsigned int reg, @@ -1984,11 +1985,10 @@ static const struct regmap_config hdmi_regmap_8bit_config = { }; static const struct regmap_config hdmi_regmap_32bit_config = { - .reg_bits = 8, - .pad_bits = 24, + .reg_bits = 32, .val_bits = 32, .reg_stride = 4, - .max_register = HDMI_I2CM_FS_SCL_LCNT_0_ADDR, + .max_register = HDMI_I2CM_FS_SCL_LCNT_0_ADDR << 2, }; static struct dw_hdmi * @@ -2044,6 +2044,7 @@ __dw_hdmi_probe(struct platform_device *pdev, switch (val) { case 4: reg_config = &hdmi_regmap_32bit_config; + hdmi->reg_shift = 2; break; case 1: reg_config = &hdmi_regmap_8bit_config; ^ permalink raw reply related [flat|nested] 21+ messages in thread
* [RFC/RFT PATCH 2/4] drm/bridge: dw-hdmi: Add support for custom PHY handling 2017-01-17 12:31 [RFC/RFT PATCH 0/4] drm/bridge: dw-hdmi: Add support for Custom PHYs Neil Armstrong 2017-01-17 12:31 ` [RFC/RFT PATCH 1/4] drm/bridge: dw-hdmi: Switch to regmap for register access Neil Armstrong @ 2017-01-17 12:31 ` Neil Armstrong 2017-01-17 14:54 ` Laurent Pinchart 2017-01-18 10:40 ` Jose Abreu 2017-01-17 12:31 ` [RFC/RFT PATCH 3/4] drm/bridge: dw-hdmi: Enable CSC even for DVI Neil Armstrong 2017-01-17 12:31 ` [RFC/RFT PATCH 4/4] drm/bridge: dw-hdmi: Take input format from plat_data Neil Armstrong 3 siblings, 2 replies; 21+ messages in thread From: Neil Armstrong @ 2017-01-17 12:31 UTC (permalink / raw) To: dri-devel, laurent.pinchart+renesas Cc: Neil Armstrong, Jose.Abreu, kieran.bingham, linux-amlogic, linux-kernel The Synopsys DesignWare HDMI TX Controller support various Transceivers (PHY) attached to the controller, but also allows fully custom PHYs to be connected. Add PHY init, disable functions in plat_data to handle fully custom PHY init. Some custom PHYs also handles the HPD and RxSense separately and do not use the internal signals exported in the Controller's IRQ stat, so a read_hpd is provided to switch to HPD status polling. To complete the implementation, add a function to mimic the Controllers interrupt HPD and RxSense handling from the vendor PHY wrapper code. Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> --- drivers/gpu/drm/bridge/dw-hdmi.c | 78 +++++++++++++++++++++++++++++++--------- include/drm/bridge/dw_hdmi.h | 8 +++++ 2 files changed, 70 insertions(+), 16 deletions(-) diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c index 13747fe..923e250 100644 --- a/drivers/gpu/drm/bridge/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/dw-hdmi.c @@ -1434,9 +1434,18 @@ static int dw_hdmi_setup(struct dw_hdmi *hdmi, struct drm_display_mode *mode) hdmi_av_composer(hdmi, mode); /* HDMI Initializateion Step B.2 */ - ret = dw_hdmi_phy_init(hdmi); - if (ret) - return ret; + if (hdmi->plat_data->hdmi_phy_init) { + ret = hdmi->plat_data->hdmi_phy_init(hdmi, hdmi->plat_data, + &hdmi->previous_mode); + if (ret) + return ret; + + hdmi->phy_enabled = true; + } else { + ret = dw_hdmi_phy_init(hdmi); + if (ret) + return ret; + } /* HDMI Initialization Step B.3 */ dw_hdmi_enable_video_path(hdmi); @@ -1551,7 +1560,11 @@ static void dw_hdmi_poweron(struct dw_hdmi *hdmi) static void dw_hdmi_poweroff(struct dw_hdmi *hdmi) { - dw_hdmi_phy_disable(hdmi); + if (hdmi->phy_enabled && hdmi->plat_data->hdmi_phy_disable) { + hdmi->plat_data->hdmi_phy_disable(hdmi, hdmi->plat_data); + hdmi->phy_enabled = false; + } else + dw_hdmi_phy_disable(hdmi); hdmi->bridge_is_on = false; } @@ -1593,6 +1606,9 @@ static void dw_hdmi_update_phy_mask(struct dw_hdmi *hdmi) { u8 old_mask = hdmi->phy_mask; + if (hdmi->plat_data && hdmi->plat_data->hdmi_read_hpd) + return; + if (hdmi->force || hdmi->disabled || !hdmi->rxsense) hdmi->phy_mask |= HDMI_PHY_RX_SENSE; else @@ -1614,6 +1630,11 @@ static void dw_hdmi_update_phy_mask(struct dw_hdmi *hdmi) dw_hdmi_update_phy_mask(hdmi); mutex_unlock(&hdmi->mutex); + if (hdmi->plat_data && hdmi->plat_data->hdmi_read_hpd) + return hdmi->plat_data->hdmi_read_hpd(hdmi, hdmi->plat_data) ? + connector_status_connected : + connector_status_disconnected; + return hdmi_readb(hdmi, HDMI_PHY_STAT0) & HDMI_PHY_HPD ? connector_status_connected : connector_status_disconnected; } @@ -1901,6 +1922,26 @@ static irqreturn_t dw_hdmi_irq(int irq, void *dev_id) } }; +void dw_hdmi_setup_rx_sense(struct device *dev, bool hpd, bool rx_sense) +{ + struct dw_hdmi *hdmi = dev_get_drvdata(dev); + + mutex_lock(&hdmi->mutex); + + if (!hdmi->disabled && !hdmi->force) { + if (!rx_sense) + hdmi->rxsense = false; + + if (hpd) + hdmi->rxsense = true; + + dw_hdmi_update_power(hdmi); + dw_hdmi_update_phy_mask(hdmi); + } + mutex_unlock(&hdmi->mutex); +} +EXPORT_SYMBOL_GPL(dw_hdmi_setup_rx_sense); + static int dw_hdmi_detect_phy(struct dw_hdmi *hdmi) { unsigned int i; @@ -1921,7 +1962,9 @@ static int dw_hdmi_detect_phy(struct dw_hdmi *hdmi) return -ENODEV; } - if (!hdmi->phy->configure && !hdmi->plat_data->configure_phy) { + if (!hdmi->phy->configure && + !hdmi->plat_data->configure_phy && + !hdmi->plat_data->hdmi_phy_init) { dev_err(hdmi->dev, "%s requires platform support\n", hdmi->phy->name); return -ENODEV; @@ -2101,15 +2144,17 @@ static int dw_hdmi_detect_phy(struct dw_hdmi *hdmi) hdmi->ddc = NULL; } - /* - * Configure registers related to HDMI interrupt - * generation before registering IRQ. - */ - hdmi_writeb(hdmi, HDMI_PHY_HPD | HDMI_PHY_RX_SENSE, HDMI_PHY_POL0); + if (!hdmi->plat_data || !hdmi->plat_data->hdmi_read_hpd) { + /* + * Configure registers related to HDMI interrupt + * generation before registering IRQ. + */ + hdmi_writeb(hdmi, HDMI_PHY_HPD | HDMI_PHY_RX_SENSE, HDMI_PHY_POL0); - /* Clear Hotplug interrupts */ - hdmi_writeb(hdmi, HDMI_IH_PHY_STAT0_HPD | HDMI_IH_PHY_STAT0_RX_SENSE, - HDMI_IH_PHY_STAT0); + /* Clear Hotplug interrupts */ + hdmi_writeb(hdmi, HDMI_IH_PHY_STAT0_HPD | HDMI_IH_PHY_STAT0_RX_SENSE, + HDMI_IH_PHY_STAT0); + } hdmi->bridge.driver_private = hdmi; hdmi->bridge.funcs = &dw_hdmi_bridge_funcs; @@ -2119,9 +2164,10 @@ static int dw_hdmi_detect_phy(struct dw_hdmi *hdmi) if (ret) goto err_iahb; - /* Unmute interrupts */ - hdmi_writeb(hdmi, ~(HDMI_IH_PHY_STAT0_HPD | HDMI_IH_PHY_STAT0_RX_SENSE), - HDMI_IH_MUTE_PHY_STAT0); + if (!hdmi->plat_data || !hdmi->plat_data->hdmi_read_hpd) + /* Unmute interrupts */ + hdmi_writeb(hdmi, ~(HDMI_IH_PHY_STAT0_HPD | HDMI_IH_PHY_STAT0_RX_SENSE), + HDMI_IH_MUTE_PHY_STAT0); memset(&pdevinfo, 0, sizeof(pdevinfo)); pdevinfo.parent = dev; diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h index 163842d..d6a0ab3 100644 --- a/include/drm/bridge/dw_hdmi.h +++ b/include/drm/bridge/dw_hdmi.h @@ -61,6 +61,13 @@ struct dw_hdmi_plat_data { enum drm_mode_status (*mode_valid)(struct drm_connector *connector, struct drm_display_mode *mode); struct regmap *regm; + int (*hdmi_phy_init)(struct dw_hdmi *hdmi, + const struct dw_hdmi_plat_data *data, + struct drm_display_mode *mode); + void (*hdmi_phy_disable)(struct dw_hdmi *hdmi, + const struct dw_hdmi_plat_data *data); + bool (*hdmi_read_hpd)(struct dw_hdmi *hdmi, + const struct dw_hdmi_plat_data *data); }; int dw_hdmi_probe(struct platform_device *pdev, @@ -70,6 +77,7 @@ int dw_hdmi_probe(struct platform_device *pdev, int dw_hdmi_bind(struct platform_device *pdev, struct drm_encoder *encoder, const struct dw_hdmi_plat_data *plat_data); +void dw_hdmi_setup_rx_sense(struct device *dev, bool hpd, bool rx_sense); void dw_hdmi_set_sample_rate(struct dw_hdmi *hdmi, unsigned int rate); void dw_hdmi_audio_enable(struct dw_hdmi *hdmi); void dw_hdmi_audio_disable(struct dw_hdmi *hdmi); -- 1.9.1 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [RFC/RFT PATCH 2/4] drm/bridge: dw-hdmi: Add support for custom PHY handling 2017-01-17 12:31 ` [RFC/RFT PATCH 2/4] drm/bridge: dw-hdmi: Add support for custom PHY handling Neil Armstrong @ 2017-01-17 14:54 ` Laurent Pinchart 2017-01-18 10:40 ` Jose Abreu 1 sibling, 0 replies; 21+ messages in thread From: Laurent Pinchart @ 2017-01-17 14:54 UTC (permalink / raw) To: Neil Armstrong Cc: dri-devel, laurent.pinchart+renesas, Jose.Abreu, kieran.bingham, linux-amlogic, linux-kernel Hi Neil, Thank you for the patch. On Tuesday 17 Jan 2017 13:31:32 Neil Armstrong wrote: > The Synopsys DesignWare HDMI TX Controller support various Transceivers > (PHY) attached to the controller, but also allows fully custom PHYs to be > connected. > > Add PHY init, disable functions in plat_data to handle fully custom PHY > init. > > Some custom PHYs also handles the HPD and RxSense separately and do not use > the internal signals exported in the Controller's IRQ stat, so a read_hpd > is provided to switch to HPD status polling. > To complete the implementation, add a function to mimic the Controllers > interrupt HPD and RxSense handling from the vendor PHY wrapper code. I believe this goes in the right direction. PHY handling needs to be decoupled from the TX controller. As you've noticed I've taken a first step in that direction with "drm: bridge: dw-hdmi: Refactor PHY power handling", but that's not enough. Issues were reported with that patch which I plan to rework. If that's fine with you, I'll rebase it on top of this patch (that I will likely modify) and plan to get the result ready for v4.12. > Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> > --- > drivers/gpu/drm/bridge/dw-hdmi.c | 78 +++++++++++++++++++++++++++++-------- > include/drm/bridge/dw_hdmi.h | 8 +++++ > 2 files changed, 70 insertions(+), 16 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c > b/drivers/gpu/drm/bridge/dw-hdmi.c index 13747fe..923e250 100644 > --- a/drivers/gpu/drm/bridge/dw-hdmi.c > +++ b/drivers/gpu/drm/bridge/dw-hdmi.c > @@ -1434,9 +1434,18 @@ static int dw_hdmi_setup(struct dw_hdmi *hdmi, struct > drm_display_mode *mode) hdmi_av_composer(hdmi, mode); > > /* HDMI Initializateion Step B.2 */ > - ret = dw_hdmi_phy_init(hdmi); > - if (ret) > - return ret; > + if (hdmi->plat_data->hdmi_phy_init) { > + ret = hdmi->plat_data->hdmi_phy_init(hdmi, hdmi->plat_data, > + &hdmi->previous_mode); > + if (ret) > + return ret; > + > + hdmi->phy_enabled = true; > + } else { > + ret = dw_hdmi_phy_init(hdmi); > + if (ret) > + return ret; > + } > > /* HDMI Initialization Step B.3 */ > dw_hdmi_enable_video_path(hdmi); > @@ -1551,7 +1560,11 @@ static void dw_hdmi_poweron(struct dw_hdmi *hdmi) > > static void dw_hdmi_poweroff(struct dw_hdmi *hdmi) > { > - dw_hdmi_phy_disable(hdmi); > + if (hdmi->phy_enabled && hdmi->plat_data->hdmi_phy_disable) { > + hdmi->plat_data->hdmi_phy_disable(hdmi, hdmi->plat_data); > + hdmi->phy_enabled = false; > + } else > + dw_hdmi_phy_disable(hdmi); > hdmi->bridge_is_on = false; > } > > @@ -1593,6 +1606,9 @@ static void dw_hdmi_update_phy_mask(struct dw_hdmi > *hdmi) { > u8 old_mask = hdmi->phy_mask; > > + if (hdmi->plat_data && hdmi->plat_data->hdmi_read_hpd) > + return; > + > if (hdmi->force || hdmi->disabled || !hdmi->rxsense) > hdmi->phy_mask |= HDMI_PHY_RX_SENSE; > else > @@ -1614,6 +1630,11 @@ static void dw_hdmi_update_phy_mask(struct dw_hdmi > *hdmi) dw_hdmi_update_phy_mask(hdmi); > mutex_unlock(&hdmi->mutex); > > + if (hdmi->plat_data && hdmi->plat_data->hdmi_read_hpd) > + return hdmi->plat_data->hdmi_read_hpd(hdmi, hdmi->plat_data) ? > + connector_status_connected : > + connector_status_disconnected; > + > return hdmi_readb(hdmi, HDMI_PHY_STAT0) & HDMI_PHY_HPD ? > connector_status_connected : connector_status_disconnected; > } > @@ -1901,6 +1922,26 @@ static irqreturn_t dw_hdmi_irq(int irq, void *dev_id) > } > }; > > +void dw_hdmi_setup_rx_sense(struct device *dev, bool hpd, bool rx_sense) > +{ > + struct dw_hdmi *hdmi = dev_get_drvdata(dev); > + > + mutex_lock(&hdmi->mutex); > + > + if (!hdmi->disabled && !hdmi->force) { > + if (!rx_sense) > + hdmi->rxsense = false; > + > + if (hpd) > + hdmi->rxsense = true; > + > + dw_hdmi_update_power(hdmi); > + dw_hdmi_update_phy_mask(hdmi); > + } > + mutex_unlock(&hdmi->mutex); > +} > +EXPORT_SYMBOL_GPL(dw_hdmi_setup_rx_sense); > + > static int dw_hdmi_detect_phy(struct dw_hdmi *hdmi) > { > unsigned int i; > @@ -1921,7 +1962,9 @@ static int dw_hdmi_detect_phy(struct dw_hdmi *hdmi) > return -ENODEV; > } > > - if (!hdmi->phy->configure && !hdmi->plat_data->configure_phy) { > + if (!hdmi->phy->configure && > + !hdmi->plat_data->configure_phy && > + !hdmi->plat_data->hdmi_phy_init) { > dev_err(hdmi->dev, "%s requires platform support\n", > hdmi->phy->name); > return -ENODEV; > @@ -2101,15 +2144,17 @@ static int dw_hdmi_detect_phy(struct dw_hdmi *hdmi) > hdmi->ddc = NULL; > } > > - /* > - * Configure registers related to HDMI interrupt > - * generation before registering IRQ. > - */ > - hdmi_writeb(hdmi, HDMI_PHY_HPD | HDMI_PHY_RX_SENSE, HDMI_PHY_POL0); > + if (!hdmi->plat_data || !hdmi->plat_data->hdmi_read_hpd) { > + /* > + * Configure registers related to HDMI interrupt > + * generation before registering IRQ. > + */ > + hdmi_writeb(hdmi, HDMI_PHY_HPD | HDMI_PHY_RX_SENSE, HDMI_PHY_POL0); > > - /* Clear Hotplug interrupts */ > - hdmi_writeb(hdmi, HDMI_IH_PHY_STAT0_HPD | HDMI_IH_PHY_STAT0_RX_SENSE, > - HDMI_IH_PHY_STAT0); > + /* Clear Hotplug interrupts */ > + hdmi_writeb(hdmi, HDMI_IH_PHY_STAT0_HPD | HDMI_IH_PHY_STAT0_RX_SENSE, > + HDMI_IH_PHY_STAT0); > + } > > hdmi->bridge.driver_private = hdmi; > hdmi->bridge.funcs = &dw_hdmi_bridge_funcs; > @@ -2119,9 +2164,10 @@ static int dw_hdmi_detect_phy(struct dw_hdmi *hdmi) > if (ret) > goto err_iahb; > > - /* Unmute interrupts */ > - hdmi_writeb(hdmi, ~(HDMI_IH_PHY_STAT0_HPD | HDMI_IH_PHY_STAT0_RX_SENSE), > - HDMI_IH_MUTE_PHY_STAT0); > + if (!hdmi->plat_data || !hdmi->plat_data->hdmi_read_hpd) > + /* Unmute interrupts */ > + hdmi_writeb(hdmi, ~(HDMI_IH_PHY_STAT0_HPD | HDMI_IH_PHY_STAT0_RX_SENSE), > + HDMI_IH_MUTE_PHY_STAT0); > > memset(&pdevinfo, 0, sizeof(pdevinfo)); > pdevinfo.parent = dev; > diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h > index 163842d..d6a0ab3 100644 > --- a/include/drm/bridge/dw_hdmi.h > +++ b/include/drm/bridge/dw_hdmi.h > @@ -61,6 +61,13 @@ struct dw_hdmi_plat_data { > enum drm_mode_status (*mode_valid)(struct drm_connector *connector, > struct drm_display_mode *mode); > struct regmap *regm; > + int (*hdmi_phy_init)(struct dw_hdmi *hdmi, > + const struct dw_hdmi_plat_data *data, > + struct drm_display_mode *mode); > + void (*hdmi_phy_disable)(struct dw_hdmi *hdmi, > + const struct dw_hdmi_plat_data *data); > + bool (*hdmi_read_hpd)(struct dw_hdmi *hdmi, > + const struct dw_hdmi_plat_data *data); > }; > > int dw_hdmi_probe(struct platform_device *pdev, > @@ -70,6 +77,7 @@ int dw_hdmi_probe(struct platform_device *pdev, > int dw_hdmi_bind(struct platform_device *pdev, struct drm_encoder *encoder, > const struct dw_hdmi_plat_data *plat_data); > > +void dw_hdmi_setup_rx_sense(struct device *dev, bool hpd, bool rx_sense); > void dw_hdmi_set_sample_rate(struct dw_hdmi *hdmi, unsigned int rate); > void dw_hdmi_audio_enable(struct dw_hdmi *hdmi); > void dw_hdmi_audio_disable(struct dw_hdmi *hdmi); -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC/RFT PATCH 2/4] drm/bridge: dw-hdmi: Add support for custom PHY handling 2017-01-17 12:31 ` [RFC/RFT PATCH 2/4] drm/bridge: dw-hdmi: Add support for custom PHY handling Neil Armstrong 2017-01-17 14:54 ` Laurent Pinchart @ 2017-01-18 10:40 ` Jose Abreu 2017-01-18 11:20 ` Neil Armstrong 1 sibling, 1 reply; 21+ messages in thread From: Jose Abreu @ 2017-01-18 10:40 UTC (permalink / raw) To: Neil Armstrong, dri-devel, laurent.pinchart+renesas Cc: Jose.Abreu, linux-amlogic, kieran.bingham, linux-kernel Hi Neil, On 17-01-2017 12:31, Neil Armstrong wrote: > @@ -1434,9 +1434,18 @@ static int dw_hdmi_setup(struct dw_hdmi *hdmi, struct drm_display_mode *mode) > hdmi_av_composer(hdmi, mode); > > /* HDMI Initializateion Step B.2 */ > - ret = dw_hdmi_phy_init(hdmi); > - if (ret) > - return ret; > + if (hdmi->plat_data->hdmi_phy_init) { > + ret = hdmi->plat_data->hdmi_phy_init(hdmi, hdmi->plat_data, > @@ -1551,7 +1560,11 @@ static void dw_hdmi_poweron(struct dw_hdmi *hdmi) > > static void dw_hdmi_poweroff(struct dw_hdmi *hdmi) > { > - dw_hdmi_phy_disable(hdmi); > + if (hdmi->phy_enabled && hdmi->plat_data->hdmi_phy_disable) { > + hdmi->plat_data->hdmi_phy_disable(hdmi, hdmi->plat_data); > + hdmi->phy_enabled = false; > + } else > + dw_hdmi_phy_disable(hdmi); > hdmi->bridge_is_on = false; > } > > @@ -1921,7 +1962,9 @@ static int dw_hdmi_detect_phy(struct dw_hdmi *hdmi) > return -ENODEV; > } > > - if (!hdmi->phy->configure && !hdmi->plat_data->configure_phy) { > + if (!hdmi->phy->configure && > + !hdmi->plat_data->configure_phy && > + !hdmi->plat_data->hdmi_phy_init) { > dev_err(hdmi->dev, "%s requires platform support\n", > hdmi->phy->name); > return -ENODEV; > @@ -2119,9 +2164,10 @@ static int dw_hdmi_detect_phy(struct dw_hdmi *hdmi) > + if (!hdmi->plat_data || !hdmi->plat_data->hdmi_read_hpd) > + /* Unmute interrupts */ > + hdmi_writeb(hdmi, ~(HDMI_IH_PHY_STAT0_HPD | HDMI_IH_PHY_STAT0_RX_SENSE), > + HDMI_IH_MUTE_PHY_STAT0); > > [sniped some parts of the patch] I personally don't like this kind of 'if plat_data ... else ...' Can't we just abstract all of the phy configuration (for a Synopsys Phy) to a new file and then pass it always in pdata as default? Then overwrite it with custom one if needed. Much like what you did with the regmap. Or even leave it in dw-hdmi.c but have a generic pdata structure with generic phy init/disable functions. Best regards, Jose Miguel Abreu ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC/RFT PATCH 2/4] drm/bridge: dw-hdmi: Add support for custom PHY handling 2017-01-18 10:40 ` Jose Abreu @ 2017-01-18 11:20 ` Neil Armstrong 2017-01-19 14:20 ` Jose Abreu 0 siblings, 1 reply; 21+ messages in thread From: Neil Armstrong @ 2017-01-18 11:20 UTC (permalink / raw) To: Jose Abreu, dri-devel, laurent.pinchart+renesas Cc: linux-amlogic, kieran.bingham, linux-kernel Hi Jose, On 01/18/2017 11:40 AM, Jose Abreu wrote: > Hi Neil, > > > On 17-01-2017 12:31, Neil Armstrong wrote: >> @@ -1434,9 +1434,18 @@ static int dw_hdmi_setup(struct dw_hdmi *hdmi, struct drm_display_mode *mode) >> hdmi_av_composer(hdmi, mode); >> >> /* HDMI Initializateion Step B.2 */ >> - ret = dw_hdmi_phy_init(hdmi); >> - if (ret) >> - return ret; >> + if (hdmi->plat_data->hdmi_phy_init) { >> + ret = hdmi->plat_data->hdmi_phy_init(hdmi, hdmi->plat_data, >> @@ -1551,7 +1560,11 @@ static void dw_hdmi_poweron(struct dw_hdmi *hdmi) >> >> static void dw_hdmi_poweroff(struct dw_hdmi *hdmi) >> { >> - dw_hdmi_phy_disable(hdmi); >> + if (hdmi->phy_enabled && hdmi->plat_data->hdmi_phy_disable) { >> + hdmi->plat_data->hdmi_phy_disable(hdmi, hdmi->plat_data); >> + hdmi->phy_enabled = false; >> + } else >> + dw_hdmi_phy_disable(hdmi); >> hdmi->bridge_is_on = false; >> } >> >> @@ -1921,7 +1962,9 @@ static int dw_hdmi_detect_phy(struct dw_hdmi *hdmi) >> return -ENODEV; >> } >> >> - if (!hdmi->phy->configure && !hdmi->plat_data->configure_phy) { >> + if (!hdmi->phy->configure && >> + !hdmi->plat_data->configure_phy && >> + !hdmi->plat_data->hdmi_phy_init) { >> dev_err(hdmi->dev, "%s requires platform support\n", >> hdmi->phy->name); >> return -ENODEV; >> @@ -2119,9 +2164,10 @@ static int dw_hdmi_detect_phy(struct dw_hdmi *hdmi) >> + if (!hdmi->plat_data || !hdmi->plat_data->hdmi_read_hpd) >> + /* Unmute interrupts */ >> + hdmi_writeb(hdmi, ~(HDMI_IH_PHY_STAT0_HPD | HDMI_IH_PHY_STAT0_RX_SENSE), >> + HDMI_IH_MUTE_PHY_STAT0); >> >> > > [sniped some parts of the patch] > > I personally don't like this kind of 'if plat_data ... else ...' > Can't we just abstract all of the phy configuration (for a > Synopsys Phy) to a new file and then pass it always in pdata as > default? Then overwrite it with custom one if needed. Much like > what you did with the regmap. Or even leave it in dw-hdmi.c but > have a generic pdata structure with generic phy init/disable > functions. It's the idea we discussed with Laurent. Keeping the Synopsys PHY code inside the dw-hdmi driver would be simpler. But don't you think adding a proper "ops" structure apart from the plat_data should be necessary ? Neil > > Best regards, > Jose Miguel Abreu > ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC/RFT PATCH 2/4] drm/bridge: dw-hdmi: Add support for custom PHY handling 2017-01-18 11:20 ` Neil Armstrong @ 2017-01-19 14:20 ` Jose Abreu 0 siblings, 0 replies; 21+ messages in thread From: Jose Abreu @ 2017-01-19 14:20 UTC (permalink / raw) To: Neil Armstrong, Jose Abreu, dri-devel, laurent.pinchart+renesas Cc: linux-amlogic, kieran.bingham, linux-kernel Hi Neil, On 18-01-2017 11:20, Neil Armstrong wrote: > > It's the idea we discussed with Laurent. > Keeping the Synopsys PHY code inside the dw-hdmi driver would be simpler. > > But don't you think adding a proper "ops" structure apart from the plat_data > should be necessary ? > > Neil > An "ops" structure seems fine by me :) We could have one for the generic phy's (the ones that dw-hdmi already supports) and let the developer specify other one if needed. Best regards, Jose Miguel Abreu ^ permalink raw reply [flat|nested] 21+ messages in thread
* [RFC/RFT PATCH 3/4] drm/bridge: dw-hdmi: Enable CSC even for DVI 2017-01-17 12:31 [RFC/RFT PATCH 0/4] drm/bridge: dw-hdmi: Add support for Custom PHYs Neil Armstrong 2017-01-17 12:31 ` [RFC/RFT PATCH 1/4] drm/bridge: dw-hdmi: Switch to regmap for register access Neil Armstrong 2017-01-17 12:31 ` [RFC/RFT PATCH 2/4] drm/bridge: dw-hdmi: Add support for custom PHY handling Neil Armstrong @ 2017-01-17 12:31 ` Neil Armstrong 2017-01-17 14:40 ` Laurent Pinchart 2017-01-18 10:22 ` Jose Abreu 2017-01-17 12:31 ` [RFC/RFT PATCH 4/4] drm/bridge: dw-hdmi: Take input format from plat_data Neil Armstrong 3 siblings, 2 replies; 21+ messages in thread From: Neil Armstrong @ 2017-01-17 12:31 UTC (permalink / raw) To: dri-devel, laurent.pinchart+renesas Cc: Neil Armstrong, Jose.Abreu, kieran.bingham, linux-amlogic, linux-kernel If the input pixel format is not RGB, the CSC must be enabled in order to provide valid pixel to DVI sinks. This patch removes the hdmi only dependency on the CSC enabling. Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> --- drivers/gpu/drm/bridge/dw-hdmi.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c index 923e250..8a6a183 100644 --- a/drivers/gpu/drm/bridge/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/dw-hdmi.c @@ -1331,8 +1331,8 @@ static void dw_hdmi_enable_video_path(struct dw_hdmi *hdmi) hdmi_writeb(hdmi, clkdis, HDMI_MC_CLKDIS); } - /* Enable color space conversion if needed (for HDMI sinks only). */ - if (hdmi->sink_is_hdmi && is_color_space_conversion(hdmi)) + /* Enable color space conversion if needed */ + if (is_color_space_conversion(hdmi)) hdmi_writeb(hdmi, HDMI_MC_FLOWCTRL_FEED_THROUGH_OFF_CSC_IN_PATH, HDMI_MC_FLOWCTRL); else -- 1.9.1 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [RFC/RFT PATCH 3/4] drm/bridge: dw-hdmi: Enable CSC even for DVI 2017-01-17 12:31 ` [RFC/RFT PATCH 3/4] drm/bridge: dw-hdmi: Enable CSC even for DVI Neil Armstrong @ 2017-01-17 14:40 ` Laurent Pinchart 2017-01-18 10:22 ` Jose Abreu 1 sibling, 0 replies; 21+ messages in thread From: Laurent Pinchart @ 2017-01-17 14:40 UTC (permalink / raw) To: Neil Armstrong Cc: dri-devel, laurent.pinchart+renesas, Jose.Abreu, kieran.bingham, linux-amlogic, linux-kernel Hi Neil, Thank you for the patch. On Tuesday 17 Jan 2017 13:31:33 Neil Armstrong wrote: > If the input pixel format is not RGB, the CSC must be enabled in order to > provide valid pixel to DVI sinks. > This patch removes the hdmi only dependency on the CSC enabling. > > Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/gpu/drm/bridge/dw-hdmi.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c > b/drivers/gpu/drm/bridge/dw-hdmi.c index 923e250..8a6a183 100644 > --- a/drivers/gpu/drm/bridge/dw-hdmi.c > +++ b/drivers/gpu/drm/bridge/dw-hdmi.c > @@ -1331,8 +1331,8 @@ static void dw_hdmi_enable_video_path(struct dw_hdmi > *hdmi) hdmi_writeb(hdmi, clkdis, HDMI_MC_CLKDIS); > } > > - /* Enable color space conversion if needed (for HDMI sinks only). */ > - if (hdmi->sink_is_hdmi && is_color_space_conversion(hdmi)) > + /* Enable color space conversion if needed */ > + if (is_color_space_conversion(hdmi)) > hdmi_writeb(hdmi, HDMI_MC_FLOWCTRL_FEED_THROUGH_OFF_CSC_IN_PATH, > HDMI_MC_FLOWCTRL); > else -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC/RFT PATCH 3/4] drm/bridge: dw-hdmi: Enable CSC even for DVI 2017-01-17 12:31 ` [RFC/RFT PATCH 3/4] drm/bridge: dw-hdmi: Enable CSC even for DVI Neil Armstrong 2017-01-17 14:40 ` Laurent Pinchart @ 2017-01-18 10:22 ` Jose Abreu 2017-01-18 11:15 ` Neil Armstrong 1 sibling, 1 reply; 21+ messages in thread From: Jose Abreu @ 2017-01-18 10:22 UTC (permalink / raw) To: Neil Armstrong, dri-devel, laurent.pinchart+renesas Cc: Jose.Abreu, kieran.bingham, linux-amlogic, linux-kernel Hi Neil, On 17-01-2017 12:31, Neil Armstrong wrote: > If the input pixel format is not RGB, the CSC must be enabled in order to > provide valid pixel to DVI sinks. > This patch removes the hdmi only dependency on the CSC enabling. > > Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> I was going to say that you should make sure the output colorspace is RGB or else DVI will fail to work, but I just noticed the output colorspace is fixed in the code to RGB. Though, that may change in the future. Reviewed-by: Jose Abreu <joabreu@synopsys.com> Best regards, Jose Miguel Abreu > --- > drivers/gpu/drm/bridge/dw-hdmi.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c > index 923e250..8a6a183 100644 > --- a/drivers/gpu/drm/bridge/dw-hdmi.c > +++ b/drivers/gpu/drm/bridge/dw-hdmi.c > @@ -1331,8 +1331,8 @@ static void dw_hdmi_enable_video_path(struct dw_hdmi *hdmi) > hdmi_writeb(hdmi, clkdis, HDMI_MC_CLKDIS); > } > > - /* Enable color space conversion if needed (for HDMI sinks only). */ > - if (hdmi->sink_is_hdmi && is_color_space_conversion(hdmi)) > + /* Enable color space conversion if needed */ > + if (is_color_space_conversion(hdmi)) > hdmi_writeb(hdmi, HDMI_MC_FLOWCTRL_FEED_THROUGH_OFF_CSC_IN_PATH, > HDMI_MC_FLOWCTRL); > else ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC/RFT PATCH 3/4] drm/bridge: dw-hdmi: Enable CSC even for DVI 2017-01-18 10:22 ` Jose Abreu @ 2017-01-18 11:15 ` Neil Armstrong 0 siblings, 0 replies; 21+ messages in thread From: Neil Armstrong @ 2017-01-18 11:15 UTC (permalink / raw) To: Jose Abreu, dri-devel, laurent.pinchart+renesas Cc: kieran.bingham, linux-amlogic, linux-kernel Hi jose, On 01/18/2017 11:22 AM, Jose Abreu wrote: > Hi Neil, > > > On 17-01-2017 12:31, Neil Armstrong wrote: >> If the input pixel format is not RGB, the CSC must be enabled in order to >> provide valid pixel to DVI sinks. >> This patch removes the hdmi only dependency on the CSC enabling. >> >> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> > > I was going to say that you should make sure the output > colorspace is RGB or else DVI will fail to work, but I just > noticed the output colorspace is fixed in the code to RGB. > Though, that may change in the future. Thanks for the review, It would certainly need to enforce RGB output colorspace when in DVI mode. Neil > > Reviewed-by: Jose Abreu <joabreu@synopsys.com> > > Best regards, > Jose Miguel Abreu > >> --- >> drivers/gpu/drm/bridge/dw-hdmi.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c >> index 923e250..8a6a183 100644 >> --- a/drivers/gpu/drm/bridge/dw-hdmi.c >> +++ b/drivers/gpu/drm/bridge/dw-hdmi.c >> @@ -1331,8 +1331,8 @@ static void dw_hdmi_enable_video_path(struct dw_hdmi *hdmi) >> hdmi_writeb(hdmi, clkdis, HDMI_MC_CLKDIS); >> } >> >> - /* Enable color space conversion if needed (for HDMI sinks only). */ >> - if (hdmi->sink_is_hdmi && is_color_space_conversion(hdmi)) >> + /* Enable color space conversion if needed */ >> + if (is_color_space_conversion(hdmi)) >> hdmi_writeb(hdmi, HDMI_MC_FLOWCTRL_FEED_THROUGH_OFF_CSC_IN_PATH, >> HDMI_MC_FLOWCTRL); >> else > ^ permalink raw reply [flat|nested] 21+ messages in thread
* [RFC/RFT PATCH 4/4] drm/bridge: dw-hdmi: Take input format from plat_data 2017-01-17 12:31 [RFC/RFT PATCH 0/4] drm/bridge: dw-hdmi: Add support for Custom PHYs Neil Armstrong ` (2 preceding siblings ...) 2017-01-17 12:31 ` [RFC/RFT PATCH 3/4] drm/bridge: dw-hdmi: Enable CSC even for DVI Neil Armstrong @ 2017-01-17 12:31 ` Neil Armstrong 2017-01-17 14:48 ` Laurent Pinchart 2017-01-18 10:28 ` Jose Abreu 3 siblings, 2 replies; 21+ messages in thread From: Neil Armstrong @ 2017-01-17 12:31 UTC (permalink / raw) To: dri-devel, laurent.pinchart+renesas Cc: Neil Armstrong, Jose.Abreu, kieran.bingham, linux-amlogic, linux-kernel Some display pipelines can only provide non-RBG input pixels to the HDMI TX Controller, this patch takes the pixel format from the plat_data if provided. Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> --- drivers/gpu/drm/bridge/dw-hdmi.c | 7 +++++-- include/drm/bridge/dw_hdmi.h | 9 +++++++++ 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c index 8a6a183..fa4147c 100644 --- a/drivers/gpu/drm/bridge/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/dw-hdmi.c @@ -1420,8 +1420,11 @@ static int dw_hdmi_setup(struct dw_hdmi *hdmi, struct drm_display_mode *mode) hdmi->hdmi_data.video_mode.mpixelrepetitionoutput = 0; hdmi->hdmi_data.video_mode.mpixelrepetitioninput = 0; - /* TODO: Get input format from IPU (via FB driver interface) */ - hdmi->hdmi_data.enc_in_format = RGB; + /* Get input format from plat data or fallback to RGB */ + if (hdmi->plat_data->input_fmt >= 0) + hdmi->hdmi_data.enc_in_format = hdmi->plat_data->input_fmt; + else + hdmi->hdmi_data.enc_in_format = RGB; hdmi->hdmi_data.enc_out_format = RGB; diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h index d6a0ab3..4f426c3 100644 --- a/include/drm/bridge/dw_hdmi.h +++ b/include/drm/bridge/dw_hdmi.h @@ -21,6 +21,14 @@ enum { DW_HDMI_RES_MAX, }; +enum { + DW_HDMI_INPUT_FMT_RGB = 0, + DW_HDMI_INPUT_FMT_YCBCR444, + DW_HDMI_INPUT_FMT_YCBCR422_16BITS, + DW_HDMI_INPUT_FMT_YCBCR422_8BITS, + DW_HDMI_INPUT_FMT_XVYCC444, +}; + enum dw_hdmi_phy_type { DW_HDMI_PHY_DWC_HDMI_TX_PHY = 0x00, DW_HDMI_PHY_DWC_MHL_PHY_HEAC = 0xb2, @@ -68,6 +76,7 @@ struct dw_hdmi_plat_data { const struct dw_hdmi_plat_data *data); bool (*hdmi_read_hpd)(struct dw_hdmi *hdmi, const struct dw_hdmi_plat_data *data); + int input_fmt; }; int dw_hdmi_probe(struct platform_device *pdev, -- 1.9.1 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [RFC/RFT PATCH 4/4] drm/bridge: dw-hdmi: Take input format from plat_data 2017-01-17 12:31 ` [RFC/RFT PATCH 4/4] drm/bridge: dw-hdmi: Take input format from plat_data Neil Armstrong @ 2017-01-17 14:48 ` Laurent Pinchart 2017-01-18 10:28 ` Jose Abreu 1 sibling, 0 replies; 21+ messages in thread From: Laurent Pinchart @ 2017-01-17 14:48 UTC (permalink / raw) To: Neil Armstrong Cc: dri-devel, laurent.pinchart+renesas, Jose.Abreu, kieran.bingham, linux-amlogic, linux-kernel Hi Neil, Thank you for the patch. On Tuesday 17 Jan 2017 13:31:34 Neil Armstrong wrote: > Some display pipelines can only provide non-RBG input pixels to the HDMI TX > Controller, this patch takes the pixel format from the plat_data if > provided. > > Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> > --- > drivers/gpu/drm/bridge/dw-hdmi.c | 7 +++++-- > include/drm/bridge/dw_hdmi.h | 9 +++++++++ > 2 files changed, 14 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c > b/drivers/gpu/drm/bridge/dw-hdmi.c index 8a6a183..fa4147c 100644 > --- a/drivers/gpu/drm/bridge/dw-hdmi.c > +++ b/drivers/gpu/drm/bridge/dw-hdmi.c > @@ -1420,8 +1420,11 @@ static int dw_hdmi_setup(struct dw_hdmi *hdmi, struct > drm_display_mode *mode) hdmi->hdmi_data.video_mode.mpixelrepetitionoutput = > 0; > hdmi->hdmi_data.video_mode.mpixelrepetitioninput = 0; > > - /* TODO: Get input format from IPU (via FB driver interface) */ > - hdmi->hdmi_data.enc_in_format = RGB; > + /* Get input format from plat data or fallback to RGB */ > + if (hdmi->plat_data->input_fmt >= 0) > + hdmi->hdmi_data.enc_in_format = hdmi->plat_data->input_fmt; > + else > + hdmi->hdmi_data.enc_in_format = RGB; This should ideally be queried dynamically. I believe we need to extend the DRM bridge API for this purpose. I might be willing to accept a pdata-based solution in the meantime though. > hdmi->hdmi_data.enc_out_format = RGB; > > diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h > index d6a0ab3..4f426c3 100644 > --- a/include/drm/bridge/dw_hdmi.h > +++ b/include/drm/bridge/dw_hdmi.h > @@ -21,6 +21,14 @@ enum { > DW_HDMI_RES_MAX, > }; > > +enum { > + DW_HDMI_INPUT_FMT_RGB = 0, > + DW_HDMI_INPUT_FMT_YCBCR444, > + DW_HDMI_INPUT_FMT_YCBCR422_16BITS, > + DW_HDMI_INPUT_FMT_YCBCR422_8BITS, > + DW_HDMI_INPUT_FMT_XVYCC444, > +}; How about giving the enum a name and dropping the #define RGB 0 #define YCBCR444 1 #define YCBCR422_16BITS 2 #define YCBCR422_8BITS 3 #define XVYCC444 4 macros from the driver ? Even better, how about using a media bus format code from include/uapi/linux/media-bus-format.h ? We would need an additional colorspace field to express the difference between the YCBCR and XVYCC formats, as both of them are YUV 4:4:4 and map to the same bus format. > enum dw_hdmi_phy_type { > DW_HDMI_PHY_DWC_HDMI_TX_PHY = 0x00, > DW_HDMI_PHY_DWC_MHL_PHY_HEAC = 0xb2, > @@ -68,6 +76,7 @@ struct dw_hdmi_plat_data { > const struct dw_hdmi_plat_data *data); > bool (*hdmi_read_hpd)(struct dw_hdmi *hdmi, > const struct dw_hdmi_plat_data *data); > + int input_fmt; > }; > > int dw_hdmi_probe(struct platform_device *pdev, -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC/RFT PATCH 4/4] drm/bridge: dw-hdmi: Take input format from plat_data 2017-01-17 12:31 ` [RFC/RFT PATCH 4/4] drm/bridge: dw-hdmi: Take input format from plat_data Neil Armstrong 2017-01-17 14:48 ` Laurent Pinchart @ 2017-01-18 10:28 ` Jose Abreu 2017-01-18 11:24 ` Neil Armstrong 1 sibling, 1 reply; 21+ messages in thread From: Jose Abreu @ 2017-01-18 10:28 UTC (permalink / raw) To: Neil Armstrong, dri-devel, laurent.pinchart+renesas Cc: Jose.Abreu, kieran.bingham, linux-amlogic, linux-kernel Hi Neil, On 17-01-2017 12:31, Neil Armstrong wrote: > Some display pipelines can only provide non-RBG input pixels to the HDMI TX > Controller, this patch takes the pixel format from the plat_data if provided. > > Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> > --- > drivers/gpu/drm/bridge/dw-hdmi.c | 7 +++++-- > include/drm/bridge/dw_hdmi.h | 9 +++++++++ > 2 files changed, 14 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c > index 8a6a183..fa4147c 100644 > --- a/drivers/gpu/drm/bridge/dw-hdmi.c > +++ b/drivers/gpu/drm/bridge/dw-hdmi.c > @@ -1420,8 +1420,11 @@ static int dw_hdmi_setup(struct dw_hdmi *hdmi, struct drm_display_mode *mode) > hdmi->hdmi_data.video_mode.mpixelrepetitionoutput = 0; > hdmi->hdmi_data.video_mode.mpixelrepetitioninput = 0; > > - /* TODO: Get input format from IPU (via FB driver interface) */ > - hdmi->hdmi_data.enc_in_format = RGB; > + /* Get input format from plat data or fallback to RGB */ > + if (hdmi->plat_data->input_fmt >= 0) > + hdmi->hdmi_data.enc_in_format = hdmi->plat_data->input_fmt; Also not a big fan of this approach, but its better than it was. BTW see relevant discussion about colorspace (this is more in the output path) here [1]. [1] https://patchwork.kernel.org/patch/9495153/ > + else > + hdmi->hdmi_data.enc_in_format = RGB; > > hdmi->hdmi_data.enc_out_format = RGB; > > diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h > index d6a0ab3..4f426c3 100644 > --- a/include/drm/bridge/dw_hdmi.h > +++ b/include/drm/bridge/dw_hdmi.h > @@ -21,6 +21,14 @@ enum { > DW_HDMI_RES_MAX, > }; > > +enum { > + DW_HDMI_INPUT_FMT_RGB = 0, > + DW_HDMI_INPUT_FMT_YCBCR444, > + DW_HDMI_INPUT_FMT_YCBCR422_16BITS, > + DW_HDMI_INPUT_FMT_YCBCR422_8BITS, Hmm, did you test these two? I'm not sure if deep color can be converted using CSC. Best regards, Jose Miguel Abreu > + DW_HDMI_INPUT_FMT_XVYCC444, > +}; > + > enum dw_hdmi_phy_type { > DW_HDMI_PHY_DWC_HDMI_TX_PHY = 0x00, > DW_HDMI_PHY_DWC_MHL_PHY_HEAC = 0xb2, > @@ -68,6 +76,7 @@ struct dw_hdmi_plat_data { > const struct dw_hdmi_plat_data *data); > bool (*hdmi_read_hpd)(struct dw_hdmi *hdmi, > const struct dw_hdmi_plat_data *data); > + int input_fmt; > }; > > int dw_hdmi_probe(struct platform_device *pdev, ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC/RFT PATCH 4/4] drm/bridge: dw-hdmi: Take input format from plat_data 2017-01-18 10:28 ` Jose Abreu @ 2017-01-18 11:24 ` Neil Armstrong 2017-01-18 20:49 ` Laurent Pinchart 0 siblings, 1 reply; 21+ messages in thread From: Neil Armstrong @ 2017-01-18 11:24 UTC (permalink / raw) To: Jose Abreu, dri-devel, laurent.pinchart+renesas Cc: kieran.bingham, linux-amlogic, linux-kernel Hi Jose, On 01/18/2017 11:28 AM, Jose Abreu wrote: > Hi Neil, > > > On 17-01-2017 12:31, Neil Armstrong wrote: >> Some display pipelines can only provide non-RBG input pixels to the HDMI TX >> Controller, this patch takes the pixel format from the plat_data if provided. >> >> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> >> --- >> drivers/gpu/drm/bridge/dw-hdmi.c | 7 +++++-- >> include/drm/bridge/dw_hdmi.h | 9 +++++++++ >> 2 files changed, 14 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c >> index 8a6a183..fa4147c 100644 >> --- a/drivers/gpu/drm/bridge/dw-hdmi.c >> +++ b/drivers/gpu/drm/bridge/dw-hdmi.c >> @@ -1420,8 +1420,11 @@ static int dw_hdmi_setup(struct dw_hdmi *hdmi, struct drm_display_mode *mode) >> hdmi->hdmi_data.video_mode.mpixelrepetitionoutput = 0; >> hdmi->hdmi_data.video_mode.mpixelrepetitioninput = 0; >> >> - /* TODO: Get input format from IPU (via FB driver interface) */ >> - hdmi->hdmi_data.enc_in_format = RGB; >> + /* Get input format from plat data or fallback to RGB */ >> + if (hdmi->plat_data->input_fmt >= 0) >> + hdmi->hdmi_data.enc_in_format = hdmi->plat_data->input_fmt; > > Also not a big fan of this approach, but its better than it was. > BTW see relevant discussion about colorspace (this is more in the > output path) here [1]. > > [1] https://patchwork.kernel.org/patch/9495153/ > >> + else >> + hdmi->hdmi_data.enc_in_format = RGB; >> >> hdmi->hdmi_data.enc_out_format = RGB; >> >> diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h >> index d6a0ab3..4f426c3 100644 >> --- a/include/drm/bridge/dw_hdmi.h >> +++ b/include/drm/bridge/dw_hdmi.h >> @@ -21,6 +21,14 @@ enum { >> DW_HDMI_RES_MAX, >> }; >> >> +enum { >> + DW_HDMI_INPUT_FMT_RGB = 0, >> + DW_HDMI_INPUT_FMT_YCBCR444, >> + DW_HDMI_INPUT_FMT_YCBCR422_16BITS, >> + DW_HDMI_INPUT_FMT_YCBCR422_8BITS, > > Hmm, did you test these two? I'm not sure if deep color can be > converted using CSC. I simply copied the dw-hdmi internal values. Proper handling of input formats capabilities and output format pixel clock handling should be added sometime. In the meantime, it's worth adding which input is supported. This approach is very limited, should I add a bitmask with all support input formats and select between RGB, YUV444 or YUV422 in dw_hdmi_setup ? > > Best regards, > Jose Miguel Abreu > >> + DW_HDMI_INPUT_FMT_XVYCC444, >> +}; >> + >> enum dw_hdmi_phy_type { >> DW_HDMI_PHY_DWC_HDMI_TX_PHY = 0x00, >> DW_HDMI_PHY_DWC_MHL_PHY_HEAC = 0xb2, >> @@ -68,6 +76,7 @@ struct dw_hdmi_plat_data { >> const struct dw_hdmi_plat_data *data); >> bool (*hdmi_read_hpd)(struct dw_hdmi *hdmi, >> const struct dw_hdmi_plat_data *data); >> + int input_fmt; >> }; >> >> int dw_hdmi_probe(struct platform_device *pdev, > Thanks, Neil ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC/RFT PATCH 4/4] drm/bridge: dw-hdmi: Take input format from plat_data 2017-01-18 11:24 ` Neil Armstrong @ 2017-01-18 20:49 ` Laurent Pinchart 2017-01-19 15:21 ` Jose Abreu 0 siblings, 1 reply; 21+ messages in thread From: Laurent Pinchart @ 2017-01-18 20:49 UTC (permalink / raw) To: Neil Armstrong Cc: Jose Abreu, dri-devel, laurent.pinchart+renesas, kieran.bingham, linux-amlogic, linux-kernel, Hans Verkuil Hi Neil, (CC'ing Hans Verkuil) On Wednesday 18 Jan 2017 12:24:22 Neil Armstrong wrote: > On 01/18/2017 11:28 AM, Jose Abreu wrote: > > On 17-01-2017 12:31, Neil Armstrong wrote: > >> Some display pipelines can only provide non-RBG input pixels to the HDMI > >> TX Controller, this patch takes the pixel format from the plat_data if > >> provided. > >> > >> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> > >> --- > >> > >> drivers/gpu/drm/bridge/dw-hdmi.c | 7 +++++-- > >> include/drm/bridge/dw_hdmi.h | 9 +++++++++ > >> 2 files changed, 14 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c > >> b/drivers/gpu/drm/bridge/dw-hdmi.c index 8a6a183..fa4147c 100644 > >> --- a/drivers/gpu/drm/bridge/dw-hdmi.c > >> +++ b/drivers/gpu/drm/bridge/dw-hdmi.c > >> @@ -1420,8 +1420,11 @@ static int dw_hdmi_setup(struct dw_hdmi *hdmi, > >> struct drm_display_mode *mode) > >> hdmi->hdmi_data.video_mode.mpixelrepetitionoutput = 0; > >> hdmi->hdmi_data.video_mode.mpixelrepetitioninput = 0; > >> > >> - /* TODO: Get input format from IPU (via FB driver interface) */ > >> - hdmi->hdmi_data.enc_in_format = RGB; > >> + /* Get input format from plat data or fallback to RGB */ > >> + if (hdmi->plat_data->input_fmt >= 0) > >> + hdmi->hdmi_data.enc_in_format = hdmi->plat_data->input_fmt; > > > > Also not a big fan of this approach, but its better than it was. > > BTW see relevant discussion about colorspace (this is more in the > > output path) here [1]. > > > > [1] https://patchwork.kernel.org/patch/9495153/ > > > >> + else > >> + hdmi->hdmi_data.enc_in_format = RGB; > >> > >> hdmi->hdmi_data.enc_out_format = RGB; > >> > >> diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h > >> index d6a0ab3..4f426c3 100644 > >> --- a/include/drm/bridge/dw_hdmi.h > >> +++ b/include/drm/bridge/dw_hdmi.h > >> @@ -21,6 +21,14 @@ enum { > >> > >> DW_HDMI_RES_MAX, > >> > >> }; > >> > >> +enum { > >> + DW_HDMI_INPUT_FMT_RGB = 0, > >> + DW_HDMI_INPUT_FMT_YCBCR444, > >> + DW_HDMI_INPUT_FMT_YCBCR422_16BITS, > >> + DW_HDMI_INPUT_FMT_YCBCR422_8BITS, > > > > Hmm, did you test these two? I'm not sure if deep color can be > > converted using CSC. > > I simply copied the dw-hdmi internal values. > Proper handling of input formats capabilities and > output format pixel clock handling should be added sometime. > In the meantime, it's worth adding which input is supported. > > This approach is very limited, should I add a bitmask with all > support input formats and select between RGB, YUV444 or YUV422 > in dw_hdmi_setup ? Ideally the bridge mode set operation should be extended to take format and colorspace information (or another bridge operation should be created for that purpose, I'm still undecided). As that's quite a big change, I'm fine if you start by passing a fixed format and colorspace through platform data. I would however like the format to use the media bus format codes defined in include/uapi/linux/media-bus-format.h. As far as I'm aware we have no colorspace definitions in DRM, so we would need to fix that. V4L2 handles colorspaces, and the API went through several iterations before we got it working, with stupid mistakes that we don't want to repeat here. Jose pointed to https://patchwork.kernel.org/patch/9495153/ as a starting point, and I would like to point out the format and colorspace are two different things. A colorspace is a combination of an encoding and quantization and transfer functions. Hans Verkuil has researched this topic for V4L2 (see https://gstreamer.freedesktop.org/data/events/gstreamer-conference/2015/Hans Verkuil - Colorspaces and HDMI.pdf and https://linuxtv.org/downloads/v4l-dvb-apis/uapi/v4l/colorspaces.html), we *really* want to involve him in this discussion. I believe colorspace information should be shared between V4L2 and DRM the same way we share the media bus formats (We should have done so for 4CCs as well but it's unfortunately too late for that). > >> + DW_HDMI_INPUT_FMT_XVYCC444, > >> +}; > >> + > >> enum dw_hdmi_phy_type { > >> DW_HDMI_PHY_DWC_HDMI_TX_PHY = 0x00, > >> DW_HDMI_PHY_DWC_MHL_PHY_HEAC = 0xb2, > >> @@ -68,6 +76,7 @@ struct dw_hdmi_plat_data { > >> const struct dw_hdmi_plat_data *data); > >> bool (*hdmi_read_hpd)(struct dw_hdmi *hdmi, > >> const struct dw_hdmi_plat_data *data); > >> + int input_fmt; > >> }; > >> > >> int dw_hdmi_probe(struct platform_device *pdev, -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC/RFT PATCH 4/4] drm/bridge: dw-hdmi: Take input format from plat_data 2017-01-18 20:49 ` Laurent Pinchart @ 2017-01-19 15:21 ` Jose Abreu 2017-01-19 15:30 ` Hans Verkuil 0 siblings, 1 reply; 21+ messages in thread From: Jose Abreu @ 2017-01-19 15:21 UTC (permalink / raw) To: Laurent Pinchart, Neil Armstrong Cc: Jose Abreu, dri-devel, laurent.pinchart+renesas, kieran.bingham, linux-amlogic, linux-kernel, Hans Verkuil Hi Laurent, On 18-01-2017 20:49, Laurent Pinchart wrote: > > Ideally the bridge mode set operation should be extended to take format and > colorspace information (or another bridge operation should be created for that > purpose, I'm still undecided). As that's quite a big change, I'm fine if you > start by passing a fixed format and colorspace through platform data. I would > however like the format to use the media bus format codes defined in > include/uapi/linux/media-bus-format.h. > > As far as I'm aware we have no colorspace definitions in DRM, so we would need > to fix that. V4L2 handles colorspaces, and the API went through several > iterations before we got it working, with stupid mistakes that we don't want > to repeat here. > > Jose pointed to https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.kernel.org_patch_9495153_&d=DwICAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=BkyajeUCra1JNwll2fmexnvnkIDglOZPocC1ibkiNCQ&s=V21eO0BUlcWrbddiKml5I6ylkiX2ALOHPHmble7kxwI&e= as a starting > point, and I would like to point out the format and colorspace are two > different things. A colorspace is a combination of an encoding and > quantization and transfer functions. Hans Verkuil has researched this topic > for V4L2 (see https://urldefense.proofpoint.com/v2/url?u=https-3A__gstreamer.freedesktop.org_data_events_gstreamer-2Dconference_2015_Hans&d=DwICAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=BkyajeUCra1JNwll2fmexnvnkIDglOZPocC1ibkiNCQ&s=z9IMQn5gPepKFfsLUwUXiG8MA2s1SBB1jyQDE0JIKdk&e= Verkuil - Colorspaces and HDMI.pdf and > https://urldefense.proofpoint.com/v2/url?u=https-3A__linuxtv.org_downloads_v4l-2Ddvb-2Dapis_uapi_v4l_colorspaces.html&d=DwICAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=BkyajeUCra1JNwll2fmexnvnkIDglOZPocC1ibkiNCQ&s=QReIV1CxgeALvbP5QObIvfwIh7hjEL8J4fxiATaMVYc&e= ), we > *really* want to involve him in this discussion. > > I believe colorspace information should be shared between V4L2 and DRM the > same way we share the media bus formats (We should have done so for 4CCs as > well but it's unfortunately too late for that). > > Hmm, I am always confusing this :/ So, what I was refering as "color space" is in reality the encoding (pixel encoding). In HDMI the pixel encoding can be RGB 4:4:4, YCbCr 4:4:4, YCbCr 4:2:2 and YCbCr 4:2:0. We also have color depth which can be from 8-bit to 16-bit. Besides this there is also colorimetry. I've used media-bus-format.h to successfully pass information across a V4L2 driver but I've used it in BGR24 only, which is RGB 4:4:4 8 bit, which in media-bust-format.h would correspond to MEDIA_BUS_FMT_BGR888_1X24. So, I don't know exactly what is the support for other encodings and what if there's support for color depth. If there is then great, but if not then support for this should also be added. DRM already parses successfully the supported encodings from EDID and stores them in a structure. Note that this is the output encoding, not the input one. We can still have RGB as input and then output at YCbCr using CSC. The missing point is the selection of encoding (either from userspace or from some sort of automatic mechanism by the kernel). Best regards, Jose Miguel Abreu ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC/RFT PATCH 4/4] drm/bridge: dw-hdmi: Take input format from plat_data 2017-01-19 15:21 ` Jose Abreu @ 2017-01-19 15:30 ` Hans Verkuil 2017-01-19 16:45 ` Laurent Pinchart 0 siblings, 1 reply; 21+ messages in thread From: Hans Verkuil @ 2017-01-19 15:30 UTC (permalink / raw) To: Jose Abreu, Laurent Pinchart, Neil Armstrong Cc: dri-devel, laurent.pinchart+renesas, kieran.bingham, linux-amlogic, linux-kernel, Hans Verkuil On 01/19/17 16:21, Jose Abreu wrote: > Hi Laurent, > > > On 18-01-2017 20:49, Laurent Pinchart wrote: >> >> Ideally the bridge mode set operation should be extended to take format and >> colorspace information (or another bridge operation should be created for that >> purpose, I'm still undecided). As that's quite a big change, I'm fine if you >> start by passing a fixed format and colorspace through platform data. I would >> however like the format to use the media bus format codes defined in >> include/uapi/linux/media-bus-format.h. >> >> As far as I'm aware we have no colorspace definitions in DRM, so we would need >> to fix that. V4L2 handles colorspaces, and the API went through several >> iterations before we got it working, with stupid mistakes that we don't want >> to repeat here. >> >> Jose pointed to https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.kernel.org_patch_9495153_&d=DwICAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=BkyajeUCra1JNwll2fmexnvnkIDglOZPocC1ibkiNCQ&s=V21eO0BUlcWrbddiKml5I6ylkiX2ALOHPHmble7kxwI&e= as a starting >> point, and I would like to point out the format and colorspace are two >> different things. A colorspace is a combination of an encoding and >> quantization and transfer functions. Hans Verkuil has researched this topic >> for V4L2 (see https://urldefense.proofpoint.com/v2/url?u=https-3A__gstreamer.freedesktop.org_data_events_gstreamer-2Dconference_2015_Hans&d=DwICAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=BkyajeUCra1JNwll2fmexnvnkIDglOZPocC1ibkiNCQ&s=z9IMQn5gPepKFfsLUwUXiG8MA2s1SBB1jyQDE0JIKdk&e= Verkuil - Colorspaces and HDMI.pdf and >> https://urldefense.proofpoint.com/v2/url?u=https-3A__linuxtv.org_downloads_v4l-2Ddvb-2Dapis_uapi_v4l_colorspaces.html&d=DwICAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=BkyajeUCra1JNwll2fmexnvnkIDglOZPocC1ibkiNCQ&s=QReIV1CxgeALvbP5QObIvfwIh7hjEL8J4fxiATaMVYc&e= ), we >> *really* want to involve him in this discussion. >> >> I believe colorspace information should be shared between V4L2 and DRM the >> same way we share the media bus formats (We should have done so for 4CCs as >> well but it's unfortunately too late for that). >> >> > > Hmm, I am always confusing this :/ So, what I was refering as > "color space" is in reality the encoding (pixel encoding). In > HDMI the pixel encoding can be RGB 4:4:4, YCbCr 4:4:4, YCbCr > 4:2:2 and YCbCr 4:2:0. We also have color depth which can be from > 8-bit to 16-bit. Besides this there is also colorimetry. > > I've used media-bus-format.h to successfully pass information > across a V4L2 driver but I've used it in BGR24 only, which is RGB > 4:4:4 8 bit, which in media-bust-format.h would correspond to > MEDIA_BUS_FMT_BGR888_1X24. So, I don't know exactly what is the > support for other encodings and what if there's support for color > depth. If there is then great, but if not then support for this > should also be added. > > DRM already parses successfully the supported encodings from EDID > and stores them in a structure. Note that this is the output > encoding, not the input one. We can still have RGB as input and > then output at YCbCr using CSC. The missing point is the > selection of encoding (either from userspace or from some sort of > automatic mechanism by the kernel). The list of supported encodings for video capture depends on the HW capabilities. So the driver will list the supported formats (ENUMFMT) and userspace then selects a format (S_FMT) from that list. Most HDMI receivers can convert the input to various RGB/YCbCr formats. Deep color support for HDMI has not been added (surprisingly nobody needed it apparently), but it is pretty easy: new V4L2_PIX_FMT defines should be added for those. When talking about RGB/YCbCr I prefer the term 'color encoding', since this has nothing to do with colorspaces (sRGB. AdobeRGB, BT.2020, etc.) Regards, Hans ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC/RFT PATCH 4/4] drm/bridge: dw-hdmi: Take input format from plat_data 2017-01-19 15:30 ` Hans Verkuil @ 2017-01-19 16:45 ` Laurent Pinchart 0 siblings, 0 replies; 21+ messages in thread From: Laurent Pinchart @ 2017-01-19 16:45 UTC (permalink / raw) To: Hans Verkuil Cc: Jose Abreu, Neil Armstrong, dri-devel, laurent.pinchart+renesas, kieran.bingham, linux-amlogic, linux-kernel, Hans Verkuil Hi Hans and Jose, On Thursday 19 Jan 2017 16:30:57 Hans Verkuil wrote: > On 01/19/17 16:21, Jose Abreu wrote: > > On 18-01-2017 20:49, Laurent Pinchart wrote: > >> Ideally the bridge mode set operation should be extended to take format > >> and colorspace information (or another bridge operation should be created > >> for that purpose, I'm still undecided). As that's quite a big change, I'm > >> fine if you start by passing a fixed format and colorspace through > >> platform data. I would however like the format to use the media bus > >> format codes defined in include/uapi/linux/media-bus-format.h. > >> > >> As far as I'm aware we have no colorspace definitions in DRM, so we would > >> need to fix that. V4L2 handles colorspaces, and the API went through > >> several iterations before we got it working, with stupid mistakes that > >> we don't want to repeat here. > >> > >> Jose pointed to > >> https://patchwork.kernel.org/patch/9495153/ as a starting point, and I > >> would like to point out the format and colorspace are two different > >> things. A colorspace is a combination of an encoding and quantization > >> and transfer functions. Hans Verkuil has researched this topic for V4L2 > >> (see https://gstreamer.freedesktop.org/data/events/gstreamer-> >> conference/2015/Hans Verkuil - Colorspaces and HDMI.pdf and > >> https://linuxtv.org/downloads/v4l-dvb-apis/uapi/v4l/colorspaces.html), we > >> *really* want to involve him in this discussion. > >> > >> I believe colorspace information should be shared between V4L2 and DRM > >> the same way we share the media bus formats (We should have done so for > >> 4CCs as well but it's unfortunately too late for that). > > > > Hmm, I am always confusing this :/ So, what I was refering as > > "color space" is in reality the encoding (pixel encoding). In > > HDMI the pixel encoding can be RGB 4:4:4, YCbCr 4:4:4, YCbCr > > 4:2:2 and YCbCr 4:2:0. We also have color depth which can be from > > 8-bit to 16-bit. Besides this there is also colorimetry. There's two separate concepts here, the color encoding and the pixel format. The color encoding defines how non-linear R'G'B' is transformed to non-linear Y'CbCr (or whether to stick to R'G'B' of course). It's a mathematical formula, and along with the quantization defines how the numerical Y'CbCr values are obtained. The pixel format then defines the number of bits per sample, as well as the subsampling factors. The media bus codes thus roughly correspond to pixel formats (although they also define whether the color encoding uses RGB or YCbCr, but don't define the color encoding itself or the quantization method). > > I've used media-bus-format.h to successfully pass information > > across a V4L2 driver but I've used it in BGR24 only, which is RGB > > 4:4:4 8 bit, which in media-bust-format.h would correspond to > > MEDIA_BUS_FMT_BGR888_1X24. So, I don't know exactly what is the > > support for other encodings and what if there's support for color > > depth. If there is then great, but if not then support for this > > should also be added. We have a bunch of YUV media bus formats defined in the kernel (see https://linuxtv.org/downloads/v4l-dvb-apis/uapi/v4l/subdev-formats.html#v4l2-mbus-pixelcode). We're missing some that would be needed here (namely YUV 4:4:4 in 12bpp and I believe the 4:2:0 formats), but it's just a matter of adding new definitions with the related documentation. > > DRM already parses successfully the supported encodings from EDID > > and stores them in a structure. Note that this is the output > > encoding, not the input one. We can still have RGB as input and > > then output at YCbCr using CSC. The missing point is the > > selection of encoding (either from userspace or from some sort of > > automatic mechanism by the kernel). We need to select both the input and output formats and encodings, and, yes, I believe that at least the output format and encoding should come from userspace. > The list of supported encodings for video capture depends on the HW > capabilities. So the driver will list the supported formats (ENUMFMT) > and userspace then selects a format (S_FMT) from that list. Please note that this is about HDMI encoders and the DRM/KMS subsystem :-) > Most HDMI receivers can convert the input to various RGB/YCbCr formats. > > Deep color support for HDMI has not been added (surprisingly nobody needed > it apparently), but it is pretty easy: new V4L2_PIX_FMT defines should be > added for those. New media bus formats in this case. > When talking about RGB/YCbCr I prefer the term 'color encoding', since > this has nothing to do with colorspaces (sRGB. AdobeRGB, BT.2020, etc.) -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2017-01-20 15:19 UTC | newest] Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-01-17 12:31 [RFC/RFT PATCH 0/4] drm/bridge: dw-hdmi: Add support for Custom PHYs Neil Armstrong 2017-01-17 12:31 ` [RFC/RFT PATCH 1/4] drm/bridge: dw-hdmi: Switch to regmap for register access Neil Armstrong 2017-01-17 14:39 ` Laurent Pinchart 2017-01-20 15:12 ` Neil Armstrong 2017-01-17 12:31 ` [RFC/RFT PATCH 2/4] drm/bridge: dw-hdmi: Add support for custom PHY handling Neil Armstrong 2017-01-17 14:54 ` Laurent Pinchart 2017-01-18 10:40 ` Jose Abreu 2017-01-18 11:20 ` Neil Armstrong 2017-01-19 14:20 ` Jose Abreu 2017-01-17 12:31 ` [RFC/RFT PATCH 3/4] drm/bridge: dw-hdmi: Enable CSC even for DVI Neil Armstrong 2017-01-17 14:40 ` Laurent Pinchart 2017-01-18 10:22 ` Jose Abreu 2017-01-18 11:15 ` Neil Armstrong 2017-01-17 12:31 ` [RFC/RFT PATCH 4/4] drm/bridge: dw-hdmi: Take input format from plat_data Neil Armstrong 2017-01-17 14:48 ` Laurent Pinchart 2017-01-18 10:28 ` Jose Abreu 2017-01-18 11:24 ` Neil Armstrong 2017-01-18 20:49 ` Laurent Pinchart 2017-01-19 15:21 ` Jose Abreu 2017-01-19 15:30 ` Hans Verkuil 2017-01-19 16:45 ` Laurent Pinchart
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).