| Message ID | 20200812083543.4231-1-algea.cao@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Support change dw-hdmi output color | expand |
Hi Algea, Thank you for the patch. On Wed, Aug 12, 2020 at 04:35:43PM +0800, Algea Cao wrote: > Introduce struct dw_hdmi_property_ops in plat_data to support > vendor hdmi property. > > Implement hdmi vendor properties color_depth_property and > hdmi_output_property to config hdmi output color depth and > color format. > > The property "hdmi_output_format", the possible value > could be: > - RGB > - YCBCR 444 > - YCBCR 422 > - YCBCR 420 > > Default value of the property is set to 0 = RGB, so no changes if you > don't set the property. > > The property "hdmi_output_depth" possible value could be > - Automatic > This indicates prefer highest color depth, it is > 30bit on rockcip platform. > - 24bit > - 30bit > The default value of property is 24bit. > > Signed-off-by: Algea Cao <algea.cao@rock-chips.com> > --- > > drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 174 ++++++++++++++++++++ > include/drm/bridge/dw_hdmi.h | 22 +++ > 2 files changed, 196 insertions(+) > > diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c > index 23de359a1dec..8f22d9a566db 100644 > --- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c > +++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c > @@ -52,6 +52,27 @@ > > #define HIWORD_UPDATE(val, mask) (val | (mask) << 16) > > +/* HDMI output pixel format */ > +enum drm_hdmi_output_type { > + DRM_HDMI_OUTPUT_DEFAULT_RGB, /* default RGB */ > + DRM_HDMI_OUTPUT_YCBCR444, /* YCBCR 444 */ > + DRM_HDMI_OUTPUT_YCBCR422, /* YCBCR 422 */ > + DRM_HDMI_OUTPUT_YCBCR420, /* YCBCR 420 */ > + DRM_HDMI_OUTPUT_YCBCR_HQ, /* Highest subsampled YUV */ > + DRM_HDMI_OUTPUT_YCBCR_LQ, /* Lowest subsampled YUV */ > + DRM_HDMI_OUTPUT_INVALID, /* Guess what ? */ > +}; Vendor-specific properties shouldn't use names starting with drm_ or DRM_, that's for the DRM core. But this doesn't seem specific to Rockchip at all, it should be a standard property. Additionally, new properties need to come with a userspace implementation showing their usage, in X.org, Weston, the Android DRM/KMS HW composer, or another relevant upstream project (a test tool is usually not enough). > + > +enum dw_hdmi_rockchip_color_depth { > + ROCKCHIP_HDMI_DEPTH_8, > + ROCKCHIP_HDMI_DEPTH_10, > + ROCKCHIP_HDMI_DEPTH_12, > + ROCKCHIP_HDMI_DEPTH_16, > + ROCKCHIP_HDMI_DEPTH_420_10, > + ROCKCHIP_HDMI_DEPTH_420_12, > + ROCKCHIP_HDMI_DEPTH_420_16 > +}; > + > /** > * struct rockchip_hdmi_chip_data - splite the grf setting of kind of chips > * @lcdsel_grf_reg: grf register offset of lcdc select > @@ -73,6 +94,12 @@ struct rockchip_hdmi { > struct clk *grf_clk; > struct dw_hdmi *hdmi; > struct phy *phy; > + > + struct drm_property *color_depth_property; > + struct drm_property *hdmi_output_property; > + > + unsigned int colordepth; > + enum drm_hdmi_output_type hdmi_output; > }; > > #define to_rockchip_hdmi(x) container_of(x, struct rockchip_hdmi, x) > @@ -327,6 +354,150 @@ static void dw_hdmi_rockchip_genphy_disable(struct dw_hdmi *dw_hdmi, void *data) > phy_power_off(hdmi->phy); > } > > +static const struct drm_prop_enum_list color_depth_enum_list[] = { > + { 0, "Automatic" }, /* Prefer highest color depth */ > + { 8, "24bit" }, > + { 10, "30bit" }, > +}; > + > +static const struct drm_prop_enum_list drm_hdmi_output_enum_list[] = { > + { DRM_HDMI_OUTPUT_DEFAULT_RGB, "output_rgb" }, > + { DRM_HDMI_OUTPUT_YCBCR444, "output_ycbcr444" }, > + { DRM_HDMI_OUTPUT_YCBCR422, "output_ycbcr422" }, > + { DRM_HDMI_OUTPUT_YCBCR420, "output_ycbcr420" }, > + { DRM_HDMI_OUTPUT_YCBCR_HQ, "output_ycbcr_high_subsampling" }, > + { DRM_HDMI_OUTPUT_YCBCR_LQ, "output_ycbcr_low_subsampling" }, > + { DRM_HDMI_OUTPUT_INVALID, "invalid_output" }, > +}; > + > +static void > +dw_hdmi_rockchip_attach_properties(struct drm_connector *connector, > + unsigned int color, int version, > + void *data) > +{ > + struct rockchip_hdmi *hdmi = (struct rockchip_hdmi *)data; > + struct drm_property *prop; > + > + switch (color) { > + case MEDIA_BUS_FMT_RGB101010_1X30: > + hdmi->hdmi_output = DRM_HDMI_OUTPUT_DEFAULT_RGB; > + hdmi->colordepth = 10; > + break; > + case MEDIA_BUS_FMT_YUV8_1X24: > + hdmi->hdmi_output = DRM_HDMI_OUTPUT_YCBCR444; > + hdmi->colordepth = 8; > + break; > + case MEDIA_BUS_FMT_YUV10_1X30: > + hdmi->hdmi_output = DRM_HDMI_OUTPUT_YCBCR444; > + hdmi->colordepth = 10; > + break; > + case MEDIA_BUS_FMT_UYVY10_1X20: > + hdmi->hdmi_output = DRM_HDMI_OUTPUT_YCBCR422; > + hdmi->colordepth = 10; > + break; > + case MEDIA_BUS_FMT_UYVY8_1X16: > + hdmi->hdmi_output = DRM_HDMI_OUTPUT_YCBCR422; > + hdmi->colordepth = 8; > + break; > + case MEDIA_BUS_FMT_UYYVYY8_0_5X24: > + hdmi->hdmi_output = DRM_HDMI_OUTPUT_YCBCR420; > + hdmi->colordepth = 8; > + break; > + case MEDIA_BUS_FMT_UYYVYY10_0_5X30: > + hdmi->hdmi_output = DRM_HDMI_OUTPUT_YCBCR420; > + hdmi->colordepth = 10; > + break; > + default: > + hdmi->hdmi_output = DRM_HDMI_OUTPUT_DEFAULT_RGB; > + hdmi->colordepth = 8; > + } > + > + prop = drm_property_create_enum(connector->dev, 0, > + "hdmi_output_depth", > + color_depth_enum_list, > + ARRAY_SIZE(color_depth_enum_list)); > + if (prop) { > + hdmi->color_depth_property = prop; > + drm_object_attach_property(&connector->base, prop, 0); > + } > + > + prop = drm_property_create_enum(connector->dev, 0, "hdmi_output_format", > + drm_hdmi_output_enum_list, > + ARRAY_SIZE(drm_hdmi_output_enum_list)); > + if (prop) { > + hdmi->hdmi_output_property = prop; > + drm_object_attach_property(&connector->base, prop, 0); > + } > +} > + > +static void > +dw_hdmi_rockchip_destroy_properties(struct drm_connector *connector, > + void *data) > +{ > + struct rockchip_hdmi *hdmi = (struct rockchip_hdmi *)data; > + > + if (hdmi->color_depth_property) { > + drm_property_destroy(connector->dev, > + hdmi->color_depth_property); > + hdmi->color_depth_property = NULL; > + } > + > + if (hdmi->hdmi_output_property) { > + drm_property_destroy(connector->dev, > + hdmi->hdmi_output_property); > + hdmi->hdmi_output_property = NULL; > + } > +} > + > +static int > +dw_hdmi_rockchip_set_property(struct drm_connector *connector, > + struct drm_connector_state *state, > + struct drm_property *property, > + u64 val, > + void *data) > +{ > + struct rockchip_hdmi *hdmi = (struct rockchip_hdmi *)data; > + > + if (property == hdmi->color_depth_property) { > + hdmi->colordepth = val; > + return 0; > + } else if (property == hdmi->hdmi_output_property) { > + hdmi->hdmi_output = val; > + return 0; > + } > + > + DRM_ERROR("failed to set rockchip hdmi connector property\n"); > + return -EINVAL; > +} > + > +static int > +dw_hdmi_rockchip_get_property(struct drm_connector *connector, > + const struct drm_connector_state *state, > + struct drm_property *property, > + u64 *val, > + void *data) > +{ > + struct rockchip_hdmi *hdmi = (struct rockchip_hdmi *)data; > + > + if (property == hdmi->color_depth_property) { > + *val = hdmi->colordepth; > + return 0; > + } else if (property == hdmi->hdmi_output_property) { > + *val = hdmi->hdmi_output; > + return 0; > + } > + > + DRM_ERROR("failed to get rockchip hdmi connector property\n"); > + return -EINVAL; > +} > + > +static const struct dw_hdmi_property_ops dw_hdmi_rockchip_property_ops = { > + .attach_properties = dw_hdmi_rockchip_attach_properties, > + .destroy_properties = dw_hdmi_rockchip_destroy_properties, > + .set_property = dw_hdmi_rockchip_set_property, > + .get_property = dw_hdmi_rockchip_get_property, > +}; > + > static void dw_hdmi_rk3228_setup_hpd(struct dw_hdmi *dw_hdmi, void *data) > { > struct rockchip_hdmi *hdmi = (struct rockchip_hdmi *)data; > @@ -511,6 +682,9 @@ static int dw_hdmi_rockchip_bind(struct device *dev, struct device *master, > hdmi->dev = &pdev->dev; > hdmi->chip_data = plat_data->phy_data; > plat_data->phy_data = hdmi; > + > + plat_data->property_ops = &dw_hdmi_rockchip_property_ops; > + > encoder = &hdmi->encoder; > > encoder->possible_crtcs = drm_of_find_possible_crtcs(drm, dev->of_node); > diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h > index ea34ca146b82..dc561ebe7a9b 100644 > --- a/include/drm/bridge/dw_hdmi.h > +++ b/include/drm/bridge/dw_hdmi.h > @@ -6,6 +6,7 @@ > #ifndef __DW_HDMI__ > #define __DW_HDMI__ > > +#include <drm/drm_property.h> > #include <sound/hdmi-codec.h> > > struct drm_display_info; > @@ -123,6 +124,24 @@ struct dw_hdmi_phy_ops { > void (*setup_hpd)(struct dw_hdmi *hdmi, void *data); > }; > > +struct dw_hdmi_property_ops { > + void (*attach_properties)(struct drm_connector *connector, > + unsigned int color, int version, > + void *data); > + void (*destroy_properties)(struct drm_connector *connector, > + void *data); > + int (*set_property)(struct drm_connector *connector, > + struct drm_connector_state *state, > + struct drm_property *property, > + u64 val, > + void *data); > + int (*get_property)(struct drm_connector *connector, > + const struct drm_connector_state *state, > + struct drm_property *property, > + u64 *val, > + void *data); > +}; > + > struct dw_hdmi_plat_data { > struct regmap *regm; > > @@ -141,6 +160,9 @@ struct dw_hdmi_plat_data { > const struct drm_display_info *info, > const struct drm_display_mode *mode); > > + /* Vendor Property support */ > + const struct dw_hdmi_property_ops *property_ops; > + > /* Vendor PHY support */ > const struct dw_hdmi_phy_ops *phy_ops; > const char *phy_name;
Hi Algea, On Wed, Aug 12, 2020 at 07:08:10PM +0800, crj wrote: > 在 2020/8/12 17:33, Laurent Pinchart 写道: > > On Wed, Aug 12, 2020 at 04:35:43PM +0800, Algea Cao wrote: > >> Introduce struct dw_hdmi_property_ops in plat_data to support > >> vendor hdmi property. > >> > >> Implement hdmi vendor properties color_depth_property and > >> hdmi_output_property to config hdmi output color depth and > >> color format. > >> > >> The property "hdmi_output_format", the possible value > >> could be: > >> - RGB > >> - YCBCR 444 > >> - YCBCR 422 > >> - YCBCR 420 > >> > >> Default value of the property is set to 0 = RGB, so no changes if you > >> don't set the property. > >> > >> The property "hdmi_output_depth" possible value could be > >> - Automatic > >> This indicates prefer highest color depth, it is > >> 30bit on rockcip platform. > >> - 24bit > >> - 30bit > >> The default value of property is 24bit. > >> > >> Signed-off-by: Algea Cao <algea.cao@rock-chips.com> > >> --- > >> > >> drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 174 ++++++++++++++++++++ > >> include/drm/bridge/dw_hdmi.h | 22 +++ > >> 2 files changed, 196 insertions(+) > >> > >> diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c > >> index 23de359a1dec..8f22d9a566db 100644 > >> --- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c > >> +++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c > >> @@ -52,6 +52,27 @@ > >> > >> #define HIWORD_UPDATE(val, mask) (val | (mask) << 16) > >> > >> +/* HDMI output pixel format */ > >> +enum drm_hdmi_output_type { > >> + DRM_HDMI_OUTPUT_DEFAULT_RGB, /* default RGB */ > >> + DRM_HDMI_OUTPUT_YCBCR444, /* YCBCR 444 */ > >> + DRM_HDMI_OUTPUT_YCBCR422, /* YCBCR 422 */ > >> + DRM_HDMI_OUTPUT_YCBCR420, /* YCBCR 420 */ > >> + DRM_HDMI_OUTPUT_YCBCR_HQ, /* Highest subsampled YUV */ > >> + DRM_HDMI_OUTPUT_YCBCR_LQ, /* Lowest subsampled YUV */ > >> + DRM_HDMI_OUTPUT_INVALID, /* Guess what ? */ > >> +}; > > > > Vendor-specific properties shouldn't use names starting with drm_ or > > DRM_, that's for the DRM core. But this doesn't seem specific to > > Rockchip at all, it should be a standard property. Additionally, new > > properties need to come with a userspace implementation showing their > > usage, in X.org, Weston, the Android DRM/KMS HW composer, or another > > relevant upstream project (a test tool is usually not enough). > > We use these properties only in Android HW composer, But we can't upstream > > our HW composer code right now. Can we use this properties as private > property > > and do not upstream HW composer for the time being? It's not my decision, it's a policy of the DRM subsystem to require an open implementation in userspace to validate all API additions. In any case, I think selection of the output format should be done through a standard API (very likely a standard DRM property), possibly related to drm_mode_create_hdmi_colorspace_property(). There's nothing Rockchip-specific here. > >> + > >> +enum dw_hdmi_rockchip_color_depth { > >> + ROCKCHIP_HDMI_DEPTH_8, > >> + ROCKCHIP_HDMI_DEPTH_10, > >> + ROCKCHIP_HDMI_DEPTH_12, > >> + ROCKCHIP_HDMI_DEPTH_16, > >> + ROCKCHIP_HDMI_DEPTH_420_10, > >> + ROCKCHIP_HDMI_DEPTH_420_12, > >> + ROCKCHIP_HDMI_DEPTH_420_16 > >> +}; > >> + > >> /** > >> * struct rockchip_hdmi_chip_data - splite the grf setting of kind of chips > >> * @lcdsel_grf_reg: grf register offset of lcdc select > >> @@ -73,6 +94,12 @@ struct rockchip_hdmi { > >> struct clk *grf_clk; > >> struct dw_hdmi *hdmi; > >> struct phy *phy; > >> + > >> + struct drm_property *color_depth_property; > >> + struct drm_property *hdmi_output_property; > >> + > >> + unsigned int colordepth; > >> + enum drm_hdmi_output_type hdmi_output; > >> }; > >> > >> #define to_rockchip_hdmi(x) container_of(x, struct rockchip_hdmi, x) > >> @@ -327,6 +354,150 @@ static void dw_hdmi_rockchip_genphy_disable(struct dw_hdmi *dw_hdmi, void *data) > >> phy_power_off(hdmi->phy); > >> } > >> > >> +static const struct drm_prop_enum_list color_depth_enum_list[] = { > >> + { 0, "Automatic" }, /* Prefer highest color depth */ > >> + { 8, "24bit" }, > >> + { 10, "30bit" }, > >> +}; > >> + > >> +static const struct drm_prop_enum_list drm_hdmi_output_enum_list[] = { > >> + { DRM_HDMI_OUTPUT_DEFAULT_RGB, "output_rgb" }, > >> + { DRM_HDMI_OUTPUT_YCBCR444, "output_ycbcr444" }, > >> + { DRM_HDMI_OUTPUT_YCBCR422, "output_ycbcr422" }, > >> + { DRM_HDMI_OUTPUT_YCBCR420, "output_ycbcr420" }, > >> + { DRM_HDMI_OUTPUT_YCBCR_HQ, "output_ycbcr_high_subsampling" }, > >> + { DRM_HDMI_OUTPUT_YCBCR_LQ, "output_ycbcr_low_subsampling" }, > >> + { DRM_HDMI_OUTPUT_INVALID, "invalid_output" }, > >> +}; > >> + > >> +static void > >> +dw_hdmi_rockchip_attach_properties(struct drm_connector *connector, > >> + unsigned int color, int version, > >> + void *data) > >> +{ > >> + struct rockchip_hdmi *hdmi = (struct rockchip_hdmi *)data; > >> + struct drm_property *prop; > >> + > >> + switch (color) { > >> + case MEDIA_BUS_FMT_RGB101010_1X30: > >> + hdmi->hdmi_output = DRM_HDMI_OUTPUT_DEFAULT_RGB; > >> + hdmi->colordepth = 10; > >> + break; > >> + case MEDIA_BUS_FMT_YUV8_1X24: > >> + hdmi->hdmi_output = DRM_HDMI_OUTPUT_YCBCR444; > >> + hdmi->colordepth = 8; > >> + break; > >> + case MEDIA_BUS_FMT_YUV10_1X30: > >> + hdmi->hdmi_output = DRM_HDMI_OUTPUT_YCBCR444; > >> + hdmi->colordepth = 10; > >> + break; > >> + case MEDIA_BUS_FMT_UYVY10_1X20: > >> + hdmi->hdmi_output = DRM_HDMI_OUTPUT_YCBCR422; > >> + hdmi->colordepth = 10; > >> + break; > >> + case MEDIA_BUS_FMT_UYVY8_1X16: > >> + hdmi->hdmi_output = DRM_HDMI_OUTPUT_YCBCR422; > >> + hdmi->colordepth = 8; > >> + break; > >> + case MEDIA_BUS_FMT_UYYVYY8_0_5X24: > >> + hdmi->hdmi_output = DRM_HDMI_OUTPUT_YCBCR420; > >> + hdmi->colordepth = 8; > >> + break; > >> + case MEDIA_BUS_FMT_UYYVYY10_0_5X30: > >> + hdmi->hdmi_output = DRM_HDMI_OUTPUT_YCBCR420; > >> + hdmi->colordepth = 10; > >> + break; > >> + default: > >> + hdmi->hdmi_output = DRM_HDMI_OUTPUT_DEFAULT_RGB; > >> + hdmi->colordepth = 8; > >> + } > >> + > >> + prop = drm_property_create_enum(connector->dev, 0, > >> + "hdmi_output_depth", > >> + color_depth_enum_list, > >> + ARRAY_SIZE(color_depth_enum_list)); > >> + if (prop) { > >> + hdmi->color_depth_property = prop; > >> + drm_object_attach_property(&connector->base, prop, 0); > >> + } > >> + > >> + prop = drm_property_create_enum(connector->dev, 0, "hdmi_output_format", > >> + drm_hdmi_output_enum_list, > >> + ARRAY_SIZE(drm_hdmi_output_enum_list)); > >> + if (prop) { > >> + hdmi->hdmi_output_property = prop; > >> + drm_object_attach_property(&connector->base, prop, 0); > >> + } > >> +} > >> + > >> +static void > >> +dw_hdmi_rockchip_destroy_properties(struct drm_connector *connector, > >> + void *data) > >> +{ > >> + struct rockchip_hdmi *hdmi = (struct rockchip_hdmi *)data; > >> + > >> + if (hdmi->color_depth_property) { > >> + drm_property_destroy(connector->dev, > >> + hdmi->color_depth_property); > >> + hdmi->color_depth_property = NULL; > >> + } > >> + > >> + if (hdmi->hdmi_output_property) { > >> + drm_property_destroy(connector->dev, > >> + hdmi->hdmi_output_property); > >> + hdmi->hdmi_output_property = NULL; > >> + } > >> +} > >> + > >> +static int > >> +dw_hdmi_rockchip_set_property(struct drm_connector *connector, > >> + struct drm_connector_state *state, > >> + struct drm_property *property, > >> + u64 val, > >> + void *data) > >> +{ > >> + struct rockchip_hdmi *hdmi = (struct rockchip_hdmi *)data; > >> + > >> + if (property == hdmi->color_depth_property) { > >> + hdmi->colordepth = val; > >> + return 0; > >> + } else if (property == hdmi->hdmi_output_property) { > >> + hdmi->hdmi_output = val; > >> + return 0; > >> + } > >> + > >> + DRM_ERROR("failed to set rockchip hdmi connector property\n"); > >> + return -EINVAL; > >> +} > >> + > >> +static int > >> +dw_hdmi_rockchip_get_property(struct drm_connector *connector, > >> + const struct drm_connector_state *state, > >> + struct drm_property *property, > >> + u64 *val, > >> + void *data) > >> +{ > >> + struct rockchip_hdmi *hdmi = (struct rockchip_hdmi *)data; > >> + > >> + if (property == hdmi->color_depth_property) { > >> + *val = hdmi->colordepth; > >> + return 0; > >> + } else if (property == hdmi->hdmi_output_property) { > >> + *val = hdmi->hdmi_output; > >> + return 0; > >> + } > >> + > >> + DRM_ERROR("failed to get rockchip hdmi connector property\n"); > >> + return -EINVAL; > >> +} > >> + > >> +static const struct dw_hdmi_property_ops dw_hdmi_rockchip_property_ops = { > >> + .attach_properties = dw_hdmi_rockchip_attach_properties, > >> + .destroy_properties = dw_hdmi_rockchip_destroy_properties, > >> + .set_property = dw_hdmi_rockchip_set_property, > >> + .get_property = dw_hdmi_rockchip_get_property, > >> +}; > >> + > >> static void dw_hdmi_rk3228_setup_hpd(struct dw_hdmi *dw_hdmi, void *data) > >> { > >> struct rockchip_hdmi *hdmi = (struct rockchip_hdmi *)data; > >> @@ -511,6 +682,9 @@ static int dw_hdmi_rockchip_bind(struct device *dev, struct device *master, > >> hdmi->dev = &pdev->dev; > >> hdmi->chip_data = plat_data->phy_data; > >> plat_data->phy_data = hdmi; > >> + > >> + plat_data->property_ops = &dw_hdmi_rockchip_property_ops; > >> + > >> encoder = &hdmi->encoder; > >> > >> encoder->possible_crtcs = drm_of_find_possible_crtcs(drm, dev->of_node); > >> diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h > >> index ea34ca146b82..dc561ebe7a9b 100644 > >> --- a/include/drm/bridge/dw_hdmi.h > >> +++ b/include/drm/bridge/dw_hdmi.h > >> @@ -6,6 +6,7 @@ > >> #ifndef __DW_HDMI__ > >> #define __DW_HDMI__ > >> > >> +#include <drm/drm_property.h> > >> #include <sound/hdmi-codec.h> > >> > >> struct drm_display_info; > >> @@ -123,6 +124,24 @@ struct dw_hdmi_phy_ops { > >> void (*setup_hpd)(struct dw_hdmi *hdmi, void *data); > >> }; > >> > >> +struct dw_hdmi_property_ops { > >> + void (*attach_properties)(struct drm_connector *connector, > >> + unsigned int color, int version, > >> + void *data); > >> + void (*destroy_properties)(struct drm_connector *connector, > >> + void *data); > >> + int (*set_property)(struct drm_connector *connector, > >> + struct drm_connector_state *state, > >> + struct drm_property *property, > >> + u64 val, > >> + void *data); > >> + int (*get_property)(struct drm_connector *connector, > >> + const struct drm_connector_state *state, > >> + struct drm_property *property, > >> + u64 *val, > >> + void *data); > >> +}; > >> + > >> struct dw_hdmi_plat_data { > >> struct regmap *regm; > >> > >> @@ -141,6 +160,9 @@ struct dw_hdmi_plat_data { > >> const struct drm_display_info *info, > >> const struct drm_display_mode *mode); > >> > >> + /* Vendor Property support */ > >> + const struct dw_hdmi_property_ops *property_ops; > >> + > >> /* Vendor PHY support */ > >> const struct dw_hdmi_phy_ops *phy_ops; > >> const char *phy_name;
On Wed, 12 Aug 2020 16:30:17 +0300 Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > Hi Algea, > > On Wed, Aug 12, 2020 at 07:08:10PM +0800, crj wrote: > > 在 2020/8/12 17:33, Laurent Pinchart 写道: > > > On Wed, Aug 12, 2020 at 04:35:43PM +0800, Algea Cao wrote: > > >> Introduce struct dw_hdmi_property_ops in plat_data to support > > >> vendor hdmi property. > > >> > > >> Implement hdmi vendor properties color_depth_property and > > >> hdmi_output_property to config hdmi output color depth and > > >> color format. > > >> > > >> The property "hdmi_output_format", the possible value > > >> could be: > > >> - RGB > > >> - YCBCR 444 > > >> - YCBCR 422 > > >> - YCBCR 420 > > >> > > >> Default value of the property is set to 0 = RGB, so no changes if you > > >> don't set the property. > > >> > > >> The property "hdmi_output_depth" possible value could be > > >> - Automatic > > >> This indicates prefer highest color depth, it is > > >> 30bit on rockcip platform. > > >> - 24bit > > >> - 30bit > > >> The default value of property is 24bit. > > >> > > >> Signed-off-by: Algea Cao <algea.cao@rock-chips.com> > > >> --- > > >> > > >> drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 174 ++++++++++++++++++++ > > >> include/drm/bridge/dw_hdmi.h | 22 +++ > > >> 2 files changed, 196 insertions(+) > > >> > > >> diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c > > >> index 23de359a1dec..8f22d9a566db 100644 > > >> --- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c > > >> +++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c > > >> @@ -52,6 +52,27 @@ > > >> > > >> #define HIWORD_UPDATE(val, mask) (val | (mask) << 16) > > >> > > >> +/* HDMI output pixel format */ > > >> +enum drm_hdmi_output_type { > > >> + DRM_HDMI_OUTPUT_DEFAULT_RGB, /* default RGB */ > > >> + DRM_HDMI_OUTPUT_YCBCR444, /* YCBCR 444 */ > > >> + DRM_HDMI_OUTPUT_YCBCR422, /* YCBCR 422 */ > > >> + DRM_HDMI_OUTPUT_YCBCR420, /* YCBCR 420 */ > > >> + DRM_HDMI_OUTPUT_YCBCR_HQ, /* Highest subsampled YUV */ > > >> + DRM_HDMI_OUTPUT_YCBCR_LQ, /* Lowest subsampled YUV */ > > >> + DRM_HDMI_OUTPUT_INVALID, /* Guess what ? */ > > >> +}; > > > > > > Vendor-specific properties shouldn't use names starting with drm_ or > > > DRM_, that's for the DRM core. But this doesn't seem specific to > > > Rockchip at all, it should be a standard property. Additionally, new > > > properties need to come with a userspace implementation showing their > > > usage, in X.org, Weston, the Android DRM/KMS HW composer, or another > > > relevant upstream project (a test tool is usually not enough). > > > > We use these properties only in Android HW composer, But we can't upstream > > > > our HW composer code right now. Can we use this properties as private > > property > > > > and do not upstream HW composer for the time being? > > It's not my decision, it's a policy of the DRM subsystem to require an > open implementation in userspace to validate all API additions. Also read https://www.kernel.org/doc/html/latest/gpu/drm-uapi.html#open-source-userspace-requirements very carefully: it calls for a FOSS userspace project's proper upstream to have reviewed and accepted the patches to use the new UAPI, but those patches must NOT be MERGED at that time yet. Thanks, pq
On Thu, Aug 13, 2020 at 10:42:28AM +0300, Pekka Paalanen wrote: > On Wed, 12 Aug 2020 16:30:17 +0300 Laurent Pinchart wrote: > > On Wed, Aug 12, 2020 at 07:08:10PM +0800, crj wrote: > > > 在 2020/8/12 17:33, Laurent Pinchart 写道: > > > > On Wed, Aug 12, 2020 at 04:35:43PM +0800, Algea Cao wrote: > > > >> Introduce struct dw_hdmi_property_ops in plat_data to support > > > >> vendor hdmi property. > > > >> > > > >> Implement hdmi vendor properties color_depth_property and > > > >> hdmi_output_property to config hdmi output color depth and > > > >> color format. > > > >> > > > >> The property "hdmi_output_format", the possible value > > > >> could be: > > > >> - RGB > > > >> - YCBCR 444 > > > >> - YCBCR 422 > > > >> - YCBCR 420 > > > >> > > > >> Default value of the property is set to 0 = RGB, so no changes if you > > > >> don't set the property. > > > >> > > > >> The property "hdmi_output_depth" possible value could be > > > >> - Automatic > > > >> This indicates prefer highest color depth, it is > > > >> 30bit on rockcip platform. > > > >> - 24bit > > > >> - 30bit > > > >> The default value of property is 24bit. > > > >> > > > >> Signed-off-by: Algea Cao <algea.cao@rock-chips.com> > > > >> --- > > > >> > > > >> drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 174 ++++++++++++++++++++ > > > >> include/drm/bridge/dw_hdmi.h | 22 +++ > > > >> 2 files changed, 196 insertions(+) > > > >> > > > >> diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c > > > >> index 23de359a1dec..8f22d9a566db 100644 > > > >> --- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c > > > >> +++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c > > > >> @@ -52,6 +52,27 @@ > > > >> > > > >> #define HIWORD_UPDATE(val, mask) (val | (mask) << 16) > > > >> > > > >> +/* HDMI output pixel format */ > > > >> +enum drm_hdmi_output_type { > > > >> + DRM_HDMI_OUTPUT_DEFAULT_RGB, /* default RGB */ > > > >> + DRM_HDMI_OUTPUT_YCBCR444, /* YCBCR 444 */ > > > >> + DRM_HDMI_OUTPUT_YCBCR422, /* YCBCR 422 */ > > > >> + DRM_HDMI_OUTPUT_YCBCR420, /* YCBCR 420 */ > > > >> + DRM_HDMI_OUTPUT_YCBCR_HQ, /* Highest subsampled YUV */ > > > >> + DRM_HDMI_OUTPUT_YCBCR_LQ, /* Lowest subsampled YUV */ > > > >> + DRM_HDMI_OUTPUT_INVALID, /* Guess what ? */ > > > >> +}; > > > > > > > > Vendor-specific properties shouldn't use names starting with drm_ or > > > > DRM_, that's for the DRM core. But this doesn't seem specific to > > > > Rockchip at all, it should be a standard property. Additionally, new > > > > properties need to come with a userspace implementation showing their > > > > usage, in X.org, Weston, the Android DRM/KMS HW composer, or another > > > > relevant upstream project (a test tool is usually not enough). > > > > > > We use these properties only in Android HW composer, But we can't upstream > > > > > > our HW composer code right now. Can we use this properties as private > > > property > > > > > > and do not upstream HW composer for the time being? > > > > It's not my decision, it's a policy of the DRM subsystem to require an > > open implementation in userspace to validate all API additions. > > Also read > https://www.kernel.org/doc/html/latest/gpu/drm-uapi.html#open-source-userspace-requirements > very carefully: it calls for a FOSS userspace project's proper upstream > to have reviewed and accepted the patches to use the new UAPI, but > those patches must NOT be MERGED at that time yet. Correct. Many userspace projects wouldn't merge a patch before the kernel API is merged, so that would create a chicken and egg problem :-)
On Thu, 13 Aug 2020 13:45:22 +0300 Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > On Thu, Aug 13, 2020 at 10:42:28AM +0300, Pekka Paalanen wrote: > > On Wed, 12 Aug 2020 16:30:17 +0300 Laurent Pinchart wrote: > > > On Wed, Aug 12, 2020 at 07:08:10PM +0800, crj wrote: > > > > 在 2020/8/12 17:33, Laurent Pinchart 写道: > > > > > On Wed, Aug 12, 2020 at 04:35:43PM +0800, Algea Cao wrote: > > > > >> Introduce struct dw_hdmi_property_ops in plat_data to support > > > > >> vendor hdmi property. > > > > >> > > > > >> Implement hdmi vendor properties color_depth_property and > > > > >> hdmi_output_property to config hdmi output color depth and > > > > >> color format. > > > > >> > > > > >> The property "hdmi_output_format", the possible value > > > > >> could be: > > > > >> - RGB > > > > >> - YCBCR 444 > > > > >> - YCBCR 422 > > > > >> - YCBCR 420 > > > > >> > > > > >> Default value of the property is set to 0 = RGB, so no changes if you > > > > >> don't set the property. > > > > >> > > > > >> The property "hdmi_output_depth" possible value could be > > > > >> - Automatic > > > > >> This indicates prefer highest color depth, it is > > > > >> 30bit on rockcip platform. > > > > >> - 24bit > > > > >> - 30bit > > > > >> The default value of property is 24bit. > > > > >> > > > > >> Signed-off-by: Algea Cao <algea.cao@rock-chips.com> > > > > >> --- > > > > >> > > > > >> drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 174 ++++++++++++++++++++ > > > > >> include/drm/bridge/dw_hdmi.h | 22 +++ > > > > >> 2 files changed, 196 insertions(+) > > > > >> > > > > >> diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c > > > > >> index 23de359a1dec..8f22d9a566db 100644 > > > > >> --- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c > > > > >> +++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c > > > > >> @@ -52,6 +52,27 @@ > > > > >> > > > > >> #define HIWORD_UPDATE(val, mask) (val | (mask) << 16) > > > > >> > > > > >> +/* HDMI output pixel format */ > > > > >> +enum drm_hdmi_output_type { > > > > >> + DRM_HDMI_OUTPUT_DEFAULT_RGB, /* default RGB */ > > > > >> + DRM_HDMI_OUTPUT_YCBCR444, /* YCBCR 444 */ > > > > >> + DRM_HDMI_OUTPUT_YCBCR422, /* YCBCR 422 */ > > > > >> + DRM_HDMI_OUTPUT_YCBCR420, /* YCBCR 420 */ > > > > >> + DRM_HDMI_OUTPUT_YCBCR_HQ, /* Highest subsampled YUV */ > > > > >> + DRM_HDMI_OUTPUT_YCBCR_LQ, /* Lowest subsampled YUV */ > > > > >> + DRM_HDMI_OUTPUT_INVALID, /* Guess what ? */ > > > > >> +}; > > > > > > > > > > Vendor-specific properties shouldn't use names starting with drm_ or > > > > > DRM_, that's for the DRM core. But this doesn't seem specific to > > > > > Rockchip at all, it should be a standard property. Additionally, new > > > > > properties need to come with a userspace implementation showing their > > > > > usage, in X.org, Weston, the Android DRM/KMS HW composer, or another > > > > > relevant upstream project (a test tool is usually not enough). > > > > > > > > We use these properties only in Android HW composer, But we can't upstream > > > > > > > > our HW composer code right now. Can we use this properties as private > > > > property > > > > > > > > and do not upstream HW composer for the time being? > > > > > > It's not my decision, it's a policy of the DRM subsystem to require an > > > open implementation in userspace to validate all API additions. > > > > Also read > > https://www.kernel.org/doc/html/latest/gpu/drm-uapi.html#open-source-userspace-requirements > > very carefully: it calls for a FOSS userspace project's proper upstream > > to have reviewed and accepted the patches to use the new UAPI, but > > those patches must NOT be MERGED at that time yet. > > Correct. Many userspace projects wouldn't merge a patch before the > kernel API is merged, so that would create a chicken and egg problem :-) I wouldn't be so sure that absolutely everyone knows that rule. It only takes just one userspace project to merge and release with it to potentially require renaming everything if any change is needed after the kernel review process. Actually, if I remember right, I may have seen such merging happen. After all, "accepted" is usually a synonym for "merged". Thanks, pq
diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c index 23de359a1dec..8f22d9a566db 100644 --- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c +++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c @@ -52,6 +52,27 @@ #define HIWORD_UPDATE(val, mask) (val | (mask) << 16) +/* HDMI output pixel format */ +enum drm_hdmi_output_type { + DRM_HDMI_OUTPUT_DEFAULT_RGB, /* default RGB */ + DRM_HDMI_OUTPUT_YCBCR444, /* YCBCR 444 */ + DRM_HDMI_OUTPUT_YCBCR422, /* YCBCR 422 */ + DRM_HDMI_OUTPUT_YCBCR420, /* YCBCR 420 */ + DRM_HDMI_OUTPUT_YCBCR_HQ, /* Highest subsampled YUV */ + DRM_HDMI_OUTPUT_YCBCR_LQ, /* Lowest subsampled YUV */ + DRM_HDMI_OUTPUT_INVALID, /* Guess what ? */ +}; + +enum dw_hdmi_rockchip_color_depth { + ROCKCHIP_HDMI_DEPTH_8, + ROCKCHIP_HDMI_DEPTH_10, + ROCKCHIP_HDMI_DEPTH_12, + ROCKCHIP_HDMI_DEPTH_16, + ROCKCHIP_HDMI_DEPTH_420_10, + ROCKCHIP_HDMI_DEPTH_420_12, + ROCKCHIP_HDMI_DEPTH_420_16 +}; + /** * struct rockchip_hdmi_chip_data - splite the grf setting of kind of chips * @lcdsel_grf_reg: grf register offset of lcdc select @@ -73,6 +94,12 @@ struct rockchip_hdmi { struct clk *grf_clk; struct dw_hdmi *hdmi; struct phy *phy; + + struct drm_property *color_depth_property; + struct drm_property *hdmi_output_property; + + unsigned int colordepth; + enum drm_hdmi_output_type hdmi_output; }; #define to_rockchip_hdmi(x) container_of(x, struct rockchip_hdmi, x) @@ -327,6 +354,150 @@ static void dw_hdmi_rockchip_genphy_disable(struct dw_hdmi *dw_hdmi, void *data) phy_power_off(hdmi->phy); } +static const struct drm_prop_enum_list color_depth_enum_list[] = { + { 0, "Automatic" }, /* Prefer highest color depth */ + { 8, "24bit" }, + { 10, "30bit" }, +}; + +static const struct drm_prop_enum_list drm_hdmi_output_enum_list[] = { + { DRM_HDMI_OUTPUT_DEFAULT_RGB, "output_rgb" }, + { DRM_HDMI_OUTPUT_YCBCR444, "output_ycbcr444" }, + { DRM_HDMI_OUTPUT_YCBCR422, "output_ycbcr422" }, + { DRM_HDMI_OUTPUT_YCBCR420, "output_ycbcr420" }, + { DRM_HDMI_OUTPUT_YCBCR_HQ, "output_ycbcr_high_subsampling" }, + { DRM_HDMI_OUTPUT_YCBCR_LQ, "output_ycbcr_low_subsampling" }, + { DRM_HDMI_OUTPUT_INVALID, "invalid_output" }, +}; + +static void +dw_hdmi_rockchip_attach_properties(struct drm_connector *connector, + unsigned int color, int version, + void *data) +{ + struct rockchip_hdmi *hdmi = (struct rockchip_hdmi *)data; + struct drm_property *prop; + + switch (color) { + case MEDIA_BUS_FMT_RGB101010_1X30: + hdmi->hdmi_output = DRM_HDMI_OUTPUT_DEFAULT_RGB; + hdmi->colordepth = 10; + break; + case MEDIA_BUS_FMT_YUV8_1X24: + hdmi->hdmi_output = DRM_HDMI_OUTPUT_YCBCR444; + hdmi->colordepth = 8; + break; + case MEDIA_BUS_FMT_YUV10_1X30: + hdmi->hdmi_output = DRM_HDMI_OUTPUT_YCBCR444; + hdmi->colordepth = 10; + break; + case MEDIA_BUS_FMT_UYVY10_1X20: + hdmi->hdmi_output = DRM_HDMI_OUTPUT_YCBCR422; + hdmi->colordepth = 10; + break; + case MEDIA_BUS_FMT_UYVY8_1X16: + hdmi->hdmi_output = DRM_HDMI_OUTPUT_YCBCR422; + hdmi->colordepth = 8; + break; + case MEDIA_BUS_FMT_UYYVYY8_0_5X24: + hdmi->hdmi_output = DRM_HDMI_OUTPUT_YCBCR420; + hdmi->colordepth = 8; + break; + case MEDIA_BUS_FMT_UYYVYY10_0_5X30: + hdmi->hdmi_output = DRM_HDMI_OUTPUT_YCBCR420; + hdmi->colordepth = 10; + break; + default: + hdmi->hdmi_output = DRM_HDMI_OUTPUT_DEFAULT_RGB; + hdmi->colordepth = 8; + } + + prop = drm_property_create_enum(connector->dev, 0, + "hdmi_output_depth", + color_depth_enum_list, + ARRAY_SIZE(color_depth_enum_list)); + if (prop) { + hdmi->color_depth_property = prop; + drm_object_attach_property(&connector->base, prop, 0); + } + + prop = drm_property_create_enum(connector->dev, 0, "hdmi_output_format", + drm_hdmi_output_enum_list, + ARRAY_SIZE(drm_hdmi_output_enum_list)); + if (prop) { + hdmi->hdmi_output_property = prop; + drm_object_attach_property(&connector->base, prop, 0); + } +} + +static void +dw_hdmi_rockchip_destroy_properties(struct drm_connector *connector, + void *data) +{ + struct rockchip_hdmi *hdmi = (struct rockchip_hdmi *)data; + + if (hdmi->color_depth_property) { + drm_property_destroy(connector->dev, + hdmi->color_depth_property); + hdmi->color_depth_property = NULL; + } + + if (hdmi->hdmi_output_property) { + drm_property_destroy(connector->dev, + hdmi->hdmi_output_property); + hdmi->hdmi_output_property = NULL; + } +} + +static int +dw_hdmi_rockchip_set_property(struct drm_connector *connector, + struct drm_connector_state *state, + struct drm_property *property, + u64 val, + void *data) +{ + struct rockchip_hdmi *hdmi = (struct rockchip_hdmi *)data; + + if (property == hdmi->color_depth_property) { + hdmi->colordepth = val; + return 0; + } else if (property == hdmi->hdmi_output_property) { + hdmi->hdmi_output = val; + return 0; + } + + DRM_ERROR("failed to set rockchip hdmi connector property\n"); + return -EINVAL; +} + +static int +dw_hdmi_rockchip_get_property(struct drm_connector *connector, + const struct drm_connector_state *state, + struct drm_property *property, + u64 *val, + void *data) +{ + struct rockchip_hdmi *hdmi = (struct rockchip_hdmi *)data; + + if (property == hdmi->color_depth_property) { + *val = hdmi->colordepth; + return 0; + } else if (property == hdmi->hdmi_output_property) { + *val = hdmi->hdmi_output; + return 0; + } + + DRM_ERROR("failed to get rockchip hdmi connector property\n"); + return -EINVAL; +} + +static const struct dw_hdmi_property_ops dw_hdmi_rockchip_property_ops = { + .attach_properties = dw_hdmi_rockchip_attach_properties, + .destroy_properties = dw_hdmi_rockchip_destroy_properties, + .set_property = dw_hdmi_rockchip_set_property, + .get_property = dw_hdmi_rockchip_get_property, +}; + static void dw_hdmi_rk3228_setup_hpd(struct dw_hdmi *dw_hdmi, void *data) { struct rockchip_hdmi *hdmi = (struct rockchip_hdmi *)data; @@ -511,6 +682,9 @@ static int dw_hdmi_rockchip_bind(struct device *dev, struct device *master, hdmi->dev = &pdev->dev; hdmi->chip_data = plat_data->phy_data; plat_data->phy_data = hdmi; + + plat_data->property_ops = &dw_hdmi_rockchip_property_ops; + encoder = &hdmi->encoder; encoder->possible_crtcs = drm_of_find_possible_crtcs(drm, dev->of_node); diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h index ea34ca146b82..dc561ebe7a9b 100644 --- a/include/drm/bridge/dw_hdmi.h +++ b/include/drm/bridge/dw_hdmi.h @@ -6,6 +6,7 @@ #ifndef __DW_HDMI__ #define __DW_HDMI__ +#include <drm/drm_property.h> #include <sound/hdmi-codec.h> struct drm_display_info; @@ -123,6 +124,24 @@ struct dw_hdmi_phy_ops { void (*setup_hpd)(struct dw_hdmi *hdmi, void *data); }; +struct dw_hdmi_property_ops { + void (*attach_properties)(struct drm_connector *connector, + unsigned int color, int version, + void *data); + void (*destroy_properties)(struct drm_connector *connector, + void *data); + int (*set_property)(struct drm_connector *connector, + struct drm_connector_state *state, + struct drm_property *property, + u64 val, + void *data); + int (*get_property)(struct drm_connector *connector, + const struct drm_connector_state *state, + struct drm_property *property, + u64 *val, + void *data); +}; + struct dw_hdmi_plat_data { struct regmap *regm; @@ -141,6 +160,9 @@ struct dw_hdmi_plat_data { const struct drm_display_info *info, const struct drm_display_mode *mode); + /* Vendor Property support */ + const struct dw_hdmi_property_ops *property_ops; + /* Vendor PHY support */ const struct dw_hdmi_phy_ops *phy_ops; const char *phy_name;
Introduce struct dw_hdmi_property_ops in plat_data to support vendor hdmi property. Implement hdmi vendor properties color_depth_property and hdmi_output_property to config hdmi output color depth and color format. The property "hdmi_output_format", the possible value could be: - RGB - YCBCR 444 - YCBCR 422 - YCBCR 420 Default value of the property is set to 0 = RGB, so no changes if you don't set the property. The property "hdmi_output_depth" possible value could be - Automatic This indicates prefer highest color depth, it is 30bit on rockcip platform. - 24bit - 30bit The default value of property is 24bit. Signed-off-by: Algea Cao <algea.cao@rock-chips.com> --- drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 174 ++++++++++++++++++++ include/drm/bridge/dw_hdmi.h | 22 +++ 2 files changed, 196 insertions(+)