| Message ID | 20200206191834.6125-5-narmstrong@baylibre.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Neil Armstrong |
| Headers | show |
| Series | drm/bridge: dw-hdmi: implement bus-format negotiation and YUV420 support | expand |
On Thu, 6 Feb 2020 20:18:27 +0100 Neil Armstrong <narmstrong@baylibre.com> wrote: > Add the atomic_get_output_bus_fmts, atomic_get_input_bus_fmts to negociate ^ hooks? > the possible output and input formats for the current mode and monitor, > and use the negotiated formats in a basic atomic_check callback. > > Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> > --- > + > +/* Can return a maximum of 4 possible input formats for an output format */ > +#define MAX_INPUT_SEL_FORMATS 4 It seems to only be 3 in practice (based on the code) unless I missed something. > + > +static u32 *dw_hdmi_bridge_atomic_get_input_bus_fmts(struct drm_bridge *bridge, > + struct drm_bridge_state *bridge_state, > + struct drm_crtc_state *crtc_state, > + struct drm_connector_state *conn_state, > + u32 output_fmt, > + unsigned int *num_input_fmts) > +{ > + u32 *input_fmts; > + int i = 0; > + > + *num_input_fmts = 0; > + > + input_fmts = kcalloc(MAX_INPUT_SEL_FORMATS, sizeof(*input_fmts), > + GFP_KERNEL); > + if (!input_fmts) > + return NULL; > + > + switch (output_fmt) { > + /* 8bit */ > + case MEDIA_BUS_FMT_RGB888_1X24: > + input_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24; > + input_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24; > + input_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16; > + break; > + case MEDIA_BUS_FMT_YUV8_1X24: > + input_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24; > + input_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16; > + input_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24; > + break; > + case MEDIA_BUS_FMT_UYVY8_1X16: > + input_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16; > + input_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24; > + input_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24; > + break; > + > + /* 10bit */ > + case MEDIA_BUS_FMT_RGB101010_1X30: > + input_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30; > + input_fmts[i++] = MEDIA_BUS_FMT_YUV10_1X30; > + input_fmts[i++] = MEDIA_BUS_FMT_UYVY10_1X20; > + break; > + case MEDIA_BUS_FMT_YUV10_1X30: > + input_fmts[i++] = MEDIA_BUS_FMT_YUV10_1X30; > + input_fmts[i++] = MEDIA_BUS_FMT_UYVY10_1X20; > + input_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30; > + break; > + case MEDIA_BUS_FMT_UYVY10_1X20: > + input_fmts[i++] = MEDIA_BUS_FMT_UYVY10_1X20; > + input_fmts[i++] = MEDIA_BUS_FMT_YUV10_1X30; > + input_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30; > + break; > + > + /* 12bit */ > + case MEDIA_BUS_FMT_RGB121212_1X36: > + input_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36; > + input_fmts[i++] = MEDIA_BUS_FMT_YUV12_1X36; > + input_fmts[i++] = MEDIA_BUS_FMT_UYVY12_1X24; > + break; > + case MEDIA_BUS_FMT_YUV12_1X36: > + input_fmts[i++] = MEDIA_BUS_FMT_YUV12_1X36; > + input_fmts[i++] = MEDIA_BUS_FMT_UYVY12_1X24; > + input_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36; > + break; > + case MEDIA_BUS_FMT_UYVY12_1X24: > + input_fmts[i++] = MEDIA_BUS_FMT_UYVY12_1X24; > + input_fmts[i++] = MEDIA_BUS_FMT_YUV12_1X36; > + input_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36; > + break; > + > + /* 16bit */ > + case MEDIA_BUS_FMT_RGB161616_1X48: > + input_fmts[i++] = MEDIA_BUS_FMT_RGB161616_1X48; > + input_fmts[i++] = MEDIA_BUS_FMT_YUV16_1X48; > + break; > + case MEDIA_BUS_FMT_YUV16_1X48: > + input_fmts[i++] = MEDIA_BUS_FMT_YUV16_1X48; > + input_fmts[i++] = MEDIA_BUS_FMT_RGB161616_1X48; > + break; > + > + /* 420 */ > + case MEDIA_BUS_FMT_UYYVYY8_0_5X24: > + case MEDIA_BUS_FMT_UYYVYY10_0_5X30: > + case MEDIA_BUS_FMT_UYYVYY12_0_5X36: > + case MEDIA_BUS_FMT_UYYVYY16_0_5X48: > + input_fmts[i++] = output_fmt; > + break; > + } > + > + *num_input_fmts = i; > + > + if (*num_input_fmts == 0) { > + kfree(input_fmts); > + input_fmts = NULL; > + } > + > + return input_fmts; > +} > + > +static int dw_hdmi_bridge_atomic_check(struct drm_bridge *bridge, > + struct drm_bridge_state *bridge_state, > + struct drm_crtc_state *crtc_state, > + struct drm_connector_state *conn_state) > +{ > + struct dw_hdmi *hdmi = bridge->driver_private; > + > + dev_dbg(hdmi->dev, "selected output format %x\n", > + bridge_state->output_bus_cfg.format); Nit: not aligned on the open parens. > + > + hdmi->hdmi_data.enc_out_bus_format = > + bridge_state->output_bus_cfg.format; > + > + dev_dbg(hdmi->dev, "selected input format %x\n", > + bridge_state->input_bus_cfg.format); > + > + hdmi->hdmi_data.enc_in_bus_format = > + bridge_state->input_bus_cfg.format; > + > + return 0; > +} > + > static int dw_hdmi_bridge_attach(struct drm_bridge *bridge) > { > struct dw_hdmi *hdmi = bridge->driver_private; > @@ -2499,6 +2759,9 @@ static const struct drm_bridge_funcs dw_hdmi_bridge_funcs = { > .atomic_reset = drm_atomic_helper_bridge_reset, > .attach = dw_hdmi_bridge_attach, > .detach = dw_hdmi_bridge_detach, > + .atomic_check = dw_hdmi_bridge_atomic_check, > + .atomic_get_output_bus_fmts = dw_hdmi_bridge_atomic_get_output_bus_fmts, > + .atomic_get_input_bus_fmts = dw_hdmi_bridge_atomic_get_input_bus_fmts, > .enable = dw_hdmi_bridge_enable, > .disable = dw_hdmi_bridge_disable, > .mode_set = dw_hdmi_bridge_mode_set, > @@ -2963,6 +3226,7 @@ __dw_hdmi_probe(struct platform_device *pdev, > > hdmi->bridge.driver_private = hdmi; > hdmi->bridge.funcs = &dw_hdmi_bridge_funcs; > + Nit: not sure this has to be part of that patch. Looks good otherwise. Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com> > #ifdef CONFIG_OF > hdmi->bridge.of_node = pdev->dev.of_node; > #endif
On 07/02/2020 12:02, Boris Brezillon wrote: > On Thu, 6 Feb 2020 20:18:27 +0100 > Neil Armstrong <narmstrong@baylibre.com> wrote: > >> Add the atomic_get_output_bus_fmts, atomic_get_input_bus_fmts to negociate > > ^ hooks? > >> the possible output and input formats for the current mode and monitor, >> and use the negotiated formats in a basic atomic_check callback. >> >> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> >> --- > >> + >> +/* Can return a maximum of 4 possible input formats for an output format */ >> +#define MAX_INPUT_SEL_FORMATS 4 > > It seems to only be 3 in practice (based on the code) unless I missed > something. You're right... > >> + >> +static u32 *dw_hdmi_bridge_atomic_get_input_bus_fmts(struct drm_bridge *bridge, >> + struct drm_bridge_state *bridge_state, >> + struct drm_crtc_state *crtc_state, >> + struct drm_connector_state *conn_state, >> + u32 output_fmt, >> + unsigned int *num_input_fmts) >> +{ >> + u32 *input_fmts; >> + int i = 0; >> + >> + *num_input_fmts = 0; >> + >> + input_fmts = kcalloc(MAX_INPUT_SEL_FORMATS, sizeof(*input_fmts), >> + GFP_KERNEL); >> + if (!input_fmts) >> + return NULL; >> + >> + switch (output_fmt) { >> + /* 8bit */ >> + case MEDIA_BUS_FMT_RGB888_1X24: >> + input_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24; >> + input_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24; >> + input_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16; >> + break; >> + case MEDIA_BUS_FMT_YUV8_1X24: >> + input_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24; >> + input_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16; >> + input_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24; >> + break; >> + case MEDIA_BUS_FMT_UYVY8_1X16: >> + input_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16; >> + input_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24; >> + input_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24; >> + break; >> + >> + /* 10bit */ >> + case MEDIA_BUS_FMT_RGB101010_1X30: >> + input_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30; >> + input_fmts[i++] = MEDIA_BUS_FMT_YUV10_1X30; >> + input_fmts[i++] = MEDIA_BUS_FMT_UYVY10_1X20; >> + break; >> + case MEDIA_BUS_FMT_YUV10_1X30: >> + input_fmts[i++] = MEDIA_BUS_FMT_YUV10_1X30; >> + input_fmts[i++] = MEDIA_BUS_FMT_UYVY10_1X20; >> + input_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30; >> + break; >> + case MEDIA_BUS_FMT_UYVY10_1X20: >> + input_fmts[i++] = MEDIA_BUS_FMT_UYVY10_1X20; >> + input_fmts[i++] = MEDIA_BUS_FMT_YUV10_1X30; >> + input_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30; >> + break; >> + >> + /* 12bit */ >> + case MEDIA_BUS_FMT_RGB121212_1X36: >> + input_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36; >> + input_fmts[i++] = MEDIA_BUS_FMT_YUV12_1X36; >> + input_fmts[i++] = MEDIA_BUS_FMT_UYVY12_1X24; >> + break; >> + case MEDIA_BUS_FMT_YUV12_1X36: >> + input_fmts[i++] = MEDIA_BUS_FMT_YUV12_1X36; >> + input_fmts[i++] = MEDIA_BUS_FMT_UYVY12_1X24; >> + input_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36; >> + break; >> + case MEDIA_BUS_FMT_UYVY12_1X24: >> + input_fmts[i++] = MEDIA_BUS_FMT_UYVY12_1X24; >> + input_fmts[i++] = MEDIA_BUS_FMT_YUV12_1X36; >> + input_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36; >> + break; >> + >> + /* 16bit */ >> + case MEDIA_BUS_FMT_RGB161616_1X48: >> + input_fmts[i++] = MEDIA_BUS_FMT_RGB161616_1X48; >> + input_fmts[i++] = MEDIA_BUS_FMT_YUV16_1X48; >> + break; >> + case MEDIA_BUS_FMT_YUV16_1X48: >> + input_fmts[i++] = MEDIA_BUS_FMT_YUV16_1X48; >> + input_fmts[i++] = MEDIA_BUS_FMT_RGB161616_1X48; >> + break; >> + >> + /* 420 */ >> + case MEDIA_BUS_FMT_UYYVYY8_0_5X24: >> + case MEDIA_BUS_FMT_UYYVYY10_0_5X30: >> + case MEDIA_BUS_FMT_UYYVYY12_0_5X36: >> + case MEDIA_BUS_FMT_UYYVYY16_0_5X48: >> + input_fmts[i++] = output_fmt; >> + break; >> + } >> + >> + *num_input_fmts = i; >> + >> + if (*num_input_fmts == 0) { >> + kfree(input_fmts); >> + input_fmts = NULL; >> + } >> + >> + return input_fmts; >> +} >> + >> +static int dw_hdmi_bridge_atomic_check(struct drm_bridge *bridge, >> + struct drm_bridge_state *bridge_state, >> + struct drm_crtc_state *crtc_state, >> + struct drm_connector_state *conn_state) >> +{ >> + struct dw_hdmi *hdmi = bridge->driver_private; >> + >> + dev_dbg(hdmi->dev, "selected output format %x\n", >> + bridge_state->output_bus_cfg.format); > > Nit: not aligned on the open parens. > >> + >> + hdmi->hdmi_data.enc_out_bus_format = >> + bridge_state->output_bus_cfg.format; >> + >> + dev_dbg(hdmi->dev, "selected input format %x\n", >> + bridge_state->input_bus_cfg.format); >> + >> + hdmi->hdmi_data.enc_in_bus_format = >> + bridge_state->input_bus_cfg.format; >> + >> + return 0; >> +} >> + >> static int dw_hdmi_bridge_attach(struct drm_bridge *bridge) >> { >> struct dw_hdmi *hdmi = bridge->driver_private; >> @@ -2499,6 +2759,9 @@ static const struct drm_bridge_funcs dw_hdmi_bridge_funcs = { >> .atomic_reset = drm_atomic_helper_bridge_reset, >> .attach = dw_hdmi_bridge_attach, >> .detach = dw_hdmi_bridge_detach, >> + .atomic_check = dw_hdmi_bridge_atomic_check, >> + .atomic_get_output_bus_fmts = dw_hdmi_bridge_atomic_get_output_bus_fmts, >> + .atomic_get_input_bus_fmts = dw_hdmi_bridge_atomic_get_input_bus_fmts, >> .enable = dw_hdmi_bridge_enable, >> .disable = dw_hdmi_bridge_disable, >> .mode_set = dw_hdmi_bridge_mode_set, >> @@ -2963,6 +3226,7 @@ __dw_hdmi_probe(struct platform_device *pdev, >> >> hdmi->bridge.driver_private = hdmi; >> hdmi->bridge.funcs = &dw_hdmi_bridge_funcs; >> + > > Nit: not sure this has to be part of that patch. > > Looks good otherwise. > > Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com> Thanks, will fixup in a v5 or while applying. Neil > >> #ifdef CONFIG_OF >> hdmi->bridge.of_node = pdev->dev.of_node; >> #endif >
Hi Neil! Dne četrtek, 06. februar 2020 ob 20:18:27 CET je Neil Armstrong napisal(a): > Add the atomic_get_output_bus_fmts, atomic_get_input_bus_fmts to negociate > the possible output and input formats for the current mode and monitor, > and use the negotiated formats in a basic atomic_check callback. > > Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> > --- > drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 272 +++++++++++++++++++++- > 1 file changed, 268 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index > fec4a4bcd1fe..15048ad694bc 100644 > --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > @@ -2095,11 +2095,10 @@ 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; > > - /* TOFIX: Get input format from plat data or fallback to RGB888 */ > if (hdmi->plat_data->input_bus_format) > hdmi->hdmi_data.enc_in_bus_format = > hdmi->plat_data->input_bus_format; > - else > + else if (hdmi->hdmi_data.enc_in_bus_format == MEDIA_BUS_FMT_FIXED) > hdmi->hdmi_data.enc_in_bus_format = MEDIA_BUS_FMT_RGB888_1X24; > > /* TOFIX: Get input encoding from plat data or fallback to none */ > @@ -2109,8 +2108,8 @@ static int dw_hdmi_setup(struct dw_hdmi *hdmi, struct > drm_display_mode *mode) else > hdmi->hdmi_data.enc_in_encoding = V4L2_YCBCR_ENC_DEFAULT; > > - /* TOFIX: Default to RGB888 output format */ > - hdmi->hdmi_data.enc_out_bus_format = MEDIA_BUS_FMT_RGB888_1X24; > + if (hdmi->hdmi_data.enc_out_bus_format == MEDIA_BUS_FMT_FIXED) > + hdmi->hdmi_data.enc_out_bus_format = MEDIA_BUS_FMT_RGB888_1X24; > > hdmi->hdmi_data.pix_repet_factor = 0; > hdmi->hdmi_data.hdcp_enable = 0; > @@ -2388,6 +2387,267 @@ static const struct drm_connector_helper_funcs > dw_hdmi_connector_helper_funcs = .atomic_check = > dw_hdmi_connector_atomic_check, > }; > > +/* > + * Possible output formats : > + * - MEDIA_BUS_FMT_UYYVYY16_0_5X48, > + * - MEDIA_BUS_FMT_UYYVYY12_0_5X36, > + * - MEDIA_BUS_FMT_UYYVYY10_0_5X30, > + * - MEDIA_BUS_FMT_UYYVYY8_0_5X24, > + * - MEDIA_BUS_FMT_YUV16_1X48, > + * - MEDIA_BUS_FMT_RGB161616_1X48, > + * - MEDIA_BUS_FMT_UYVY12_1X24, > + * - MEDIA_BUS_FMT_YUV12_1X36, > + * - MEDIA_BUS_FMT_RGB121212_1X36, > + * - MEDIA_BUS_FMT_UYVY10_1X20, > + * - MEDIA_BUS_FMT_YUV10_1X30, > + * - MEDIA_BUS_FMT_RGB101010_1X30, > + * - MEDIA_BUS_FMT_UYVY8_1X16, > + * - MEDIA_BUS_FMT_YUV8_1X24, > + * - MEDIA_BUS_FMT_RGB888_1X24, > + */ > + > +/* Can return a maximum of 12 possible output formats for a mode/connector > */ +#define MAX_OUTPUT_SEL_FORMATS 12 > + > +static u32 *dw_hdmi_bridge_atomic_get_output_bus_fmts(struct drm_bridge > *bridge, + struct drm_bridge_state *bridge_state, > + struct drm_crtc_state *crtc_state, > + struct drm_connector_state *conn_state, > + unsigned int *num_output_fmts) > +{ > + struct drm_connector *conn = conn_state->connector; > + struct drm_display_info *info = &conn->display_info; > + struct drm_display_mode *mode = &crtc_state->mode; > + u8 max_bpc = conn_state->max_requested_bpc; > + bool is_hdmi2_sink = info->hdmi.scdc.supported || > + (info->color_formats & DRM_COLOR_FORMAT_YCRCB420); > + u32 *output_fmts; > + int i = 0; > + > + *num_output_fmts = 0; > + > + output_fmts = kcalloc(MAX_OUTPUT_SEL_FORMATS, sizeof(*output_fmts), > + GFP_KERNEL); > + if (!output_fmts) > + return NULL; > + > + /* > + * If the current mode enforces 4:2:0, force the output but format > + * to 4:2:0 and do not add the YUV422/444/RGB formats > + */ > + if (conn->ycbcr_420_allowed && > + (drm_mode_is_420_only(info, mode) || > + ())) { > + > + /* Order bus formats from 16bit to 8bit if supported */ > + if (max_bpc >= 16 && info->bpc == 16 && > + (info->hdmi.y420_dc_modes & DRM_EDID_YCBCR420_DC_48)) > + output_fmts[i++] = MEDIA_BUS_FMT_UYYVYY16_0_5X48; > + > + if (max_bpc >= 12 && info->bpc >= 12 && > + (info->hdmi.y420_dc_modes & DRM_EDID_YCBCR420_DC_36)) > + output_fmts[i++] = MEDIA_BUS_FMT_UYYVYY12_0_5X36; > + > + if (max_bpc >= 10 && info->bpc >= 10 && > + (info->hdmi.y420_dc_modes & DRM_EDID_YCBCR420_DC_30)) > + output_fmts[i++] = MEDIA_BUS_FMT_UYYVYY10_0_5X30; > + > + /* Default 8bit fallback */ > + output_fmts[i++] = MEDIA_BUS_FMT_UYYVYY8_0_5X24; > + > + *num_output_fmts = i; > + > + return output_fmts; Driver shouldn't return just yet for case "is_hdmi2_sink && drm_mode_is_420_also(info, mode)", because monitor/TV also supports YCbCr 4:4:4 in that case. IMO YCbCr 4:4:4 should be even prefered. What do you think? Best regards, Jernej > + } > + > + /* > + * Order bus formats from 16bit to 8bit and from YUV422 to RGB > + * if supported. In any case the default RGB888 format is added > + */ > + > + if (max_bpc >= 16 && info->bpc == 16) { > + if (info->color_formats & DRM_COLOR_FORMAT_YCRCB444) > + output_fmts[i++] = MEDIA_BUS_FMT_YUV16_1X48; > + > + output_fmts[i++] = MEDIA_BUS_FMT_RGB161616_1X48; > + } > + > + if (max_bpc >= 12 && info->bpc >= 12) { > + if (info->color_formats & DRM_COLOR_FORMAT_YCRCB422) > + output_fmts[i++] = MEDIA_BUS_FMT_UYVY12_1X24; > + > + if (info->color_formats & DRM_COLOR_FORMAT_YCRCB444) > + output_fmts[i++] = MEDIA_BUS_FMT_YUV12_1X36; > + > + output_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36; > + } > + > + if (max_bpc >= 10 && info->bpc >= 10) { > + if (info->color_formats & DRM_COLOR_FORMAT_YCRCB422) > + output_fmts[i++] = MEDIA_BUS_FMT_UYVY10_1X20; > + > + if (info->color_formats & DRM_COLOR_FORMAT_YCRCB444) > + output_fmts[i++] = MEDIA_BUS_FMT_YUV10_1X30; > + > + output_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30; > + } > + > + if (info->color_formats & DRM_COLOR_FORMAT_YCRCB422) > + output_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16; > + > + if (info->color_formats & DRM_COLOR_FORMAT_YCRCB444) > + output_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24; > + > + /* Default 8bit RGB fallback */ > + output_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24; > + > + *num_output_fmts = i; > + > + return output_fmts; > +} > + > +/* > + * Possible input formats : > + * - MEDIA_BUS_FMT_RGB888_1X24 > + * - MEDIA_BUS_FMT_YUV8_1X24 > + * - MEDIA_BUS_FMT_UYVY8_1X16 > + * - MEDIA_BUS_FMT_UYYVYY8_0_5X24 > + * - MEDIA_BUS_FMT_RGB101010_1X30 > + * - MEDIA_BUS_FMT_YUV10_1X30 > + * - MEDIA_BUS_FMT_UYVY10_1X20 > + * - MEDIA_BUS_FMT_UYYVYY10_0_5X30 > + * - MEDIA_BUS_FMT_RGB121212_1X36 > + * - MEDIA_BUS_FMT_YUV12_1X36 > + * - MEDIA_BUS_FMT_UYVY12_1X24 > + * - MEDIA_BUS_FMT_UYYVYY12_0_5X36 > + * - MEDIA_BUS_FMT_RGB161616_1X48 > + * - MEDIA_BUS_FMT_YUV16_1X48 > + * - MEDIA_BUS_FMT_UYYVYY16_0_5X48 > + */ > + > +/* Can return a maximum of 4 possible input formats for an output format */ > +#define MAX_INPUT_SEL_FORMATS 4 > + > +static u32 *dw_hdmi_bridge_atomic_get_input_bus_fmts(struct drm_bridge > *bridge, + struct drm_bridge_state *bridge_state, > + struct drm_crtc_state *crtc_state, > + struct drm_connector_state *conn_state, > + u32 output_fmt, > + unsigned int *num_input_fmts) > +{ > + u32 *input_fmts; > + int i = 0; > + > + *num_input_fmts = 0; > + > + input_fmts = kcalloc(MAX_INPUT_SEL_FORMATS, sizeof(*input_fmts), > + GFP_KERNEL); > + if (!input_fmts) > + return NULL; > + > + switch (output_fmt) { > + /* 8bit */ > + case MEDIA_BUS_FMT_RGB888_1X24: > + input_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24; > + input_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24; > + input_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16; > + break; > + case MEDIA_BUS_FMT_YUV8_1X24: > + input_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24; > + input_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16; > + input_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24; > + break; > + case MEDIA_BUS_FMT_UYVY8_1X16: > + input_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16; > + input_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24; > + input_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24; > + break; > + > + /* 10bit */ > + case MEDIA_BUS_FMT_RGB101010_1X30: > + input_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30; > + input_fmts[i++] = MEDIA_BUS_FMT_YUV10_1X30; > + input_fmts[i++] = MEDIA_BUS_FMT_UYVY10_1X20; > + break; > + case MEDIA_BUS_FMT_YUV10_1X30: > + input_fmts[i++] = MEDIA_BUS_FMT_YUV10_1X30; > + input_fmts[i++] = MEDIA_BUS_FMT_UYVY10_1X20; > + input_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30; > + break; > + case MEDIA_BUS_FMT_UYVY10_1X20: > + input_fmts[i++] = MEDIA_BUS_FMT_UYVY10_1X20; > + input_fmts[i++] = MEDIA_BUS_FMT_YUV10_1X30; > + input_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30; > + break; > + > + /* 12bit */ > + case MEDIA_BUS_FMT_RGB121212_1X36: > + input_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36; > + input_fmts[i++] = MEDIA_BUS_FMT_YUV12_1X36; > + input_fmts[i++] = MEDIA_BUS_FMT_UYVY12_1X24; > + break; > + case MEDIA_BUS_FMT_YUV12_1X36: > + input_fmts[i++] = MEDIA_BUS_FMT_YUV12_1X36; > + input_fmts[i++] = MEDIA_BUS_FMT_UYVY12_1X24; > + input_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36; > + break; > + case MEDIA_BUS_FMT_UYVY12_1X24: > + input_fmts[i++] = MEDIA_BUS_FMT_UYVY12_1X24; > + input_fmts[i++] = MEDIA_BUS_FMT_YUV12_1X36; > + input_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36; > + break; > + > + /* 16bit */ > + case MEDIA_BUS_FMT_RGB161616_1X48: > + input_fmts[i++] = MEDIA_BUS_FMT_RGB161616_1X48; > + input_fmts[i++] = MEDIA_BUS_FMT_YUV16_1X48; > + break; > + case MEDIA_BUS_FMT_YUV16_1X48: > + input_fmts[i++] = MEDIA_BUS_FMT_YUV16_1X48; > + input_fmts[i++] = MEDIA_BUS_FMT_RGB161616_1X48; > + break; > + > + /* 420 */ > + case MEDIA_BUS_FMT_UYYVYY8_0_5X24: > + case MEDIA_BUS_FMT_UYYVYY10_0_5X30: > + case MEDIA_BUS_FMT_UYYVYY12_0_5X36: > + case MEDIA_BUS_FMT_UYYVYY16_0_5X48: > + input_fmts[i++] = output_fmt; > + break; > + } > + > + *num_input_fmts = i; > + > + if (*num_input_fmts == 0) { > + kfree(input_fmts); > + input_fmts = NULL; > + } > + > + return input_fmts; > +} > + > +static int dw_hdmi_bridge_atomic_check(struct drm_bridge *bridge, > + struct drm_bridge_state *bridge_state, > + struct drm_crtc_state *crtc_state, > + struct drm_connector_state *conn_state) > +{ > + struct dw_hdmi *hdmi = bridge->driver_private; > + > + dev_dbg(hdmi->dev, "selected output format %x\n", > + bridge_state->output_bus_cfg.format); > + > + hdmi->hdmi_data.enc_out_bus_format = > + bridge_state->output_bus_cfg.format; > + > + dev_dbg(hdmi->dev, "selected input format %x\n", > + bridge_state->input_bus_cfg.format); > + > + hdmi->hdmi_data.enc_in_bus_format = > + bridge_state->input_bus_cfg.format; > + > + return 0; > +} > + > static int dw_hdmi_bridge_attach(struct drm_bridge *bridge) > { > struct dw_hdmi *hdmi = bridge->driver_private; > @@ -2499,6 +2759,9 @@ static const struct drm_bridge_funcs > dw_hdmi_bridge_funcs = { .atomic_reset = drm_atomic_helper_bridge_reset, > .attach = dw_hdmi_bridge_attach, > .detach = dw_hdmi_bridge_detach, > + .atomic_check = dw_hdmi_bridge_atomic_check, > + .atomic_get_output_bus_fmts = dw_hdmi_bridge_atomic_get_output_bus_fmts, > + .atomic_get_input_bus_fmts = dw_hdmi_bridge_atomic_get_input_bus_fmts, > .enable = dw_hdmi_bridge_enable, > .disable = dw_hdmi_bridge_disable, > .mode_set = dw_hdmi_bridge_mode_set, > @@ -2963,6 +3226,7 @@ __dw_hdmi_probe(struct platform_device *pdev, > > hdmi->bridge.driver_private = hdmi; > hdmi->bridge.funcs = &dw_hdmi_bridge_funcs; > + > #ifdef CONFIG_OF > hdmi->bridge.of_node = pdev->dev.of_node; > #endif
Hi Jernej, On 2020-02-29 08:42, Jernej Škrabec wrote: > Hi Neil! > > Dne četrtek, 06. februar 2020 ob 20:18:27 CET je Neil Armstrong napisal(a): >> Add the atomic_get_output_bus_fmts, atomic_get_input_bus_fmts to negociate >> the possible output and input formats for the current mode and monitor, >> and use the negotiated formats in a basic atomic_check callback. >> >> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> >> --- >> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 272 +++++++++++++++++++++- >> 1 file changed, 268 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c >> b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index >> fec4a4bcd1fe..15048ad694bc 100644 >> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c >> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c >> @@ -2095,11 +2095,10 @@ 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; >> >> - /* TOFIX: Get input format from plat data or fallback to RGB888 */ >> if (hdmi->plat_data->input_bus_format) >> hdmi->hdmi_data.enc_in_bus_format = >> hdmi->plat_data->input_bus_format; >> - else >> + else if (hdmi->hdmi_data.enc_in_bus_format == MEDIA_BUS_FMT_FIXED) >> hdmi->hdmi_data.enc_in_bus_format = > MEDIA_BUS_FMT_RGB888_1X24; >> >> /* TOFIX: Get input encoding from plat data or fallback to none */ >> @@ -2109,8 +2108,8 @@ static int dw_hdmi_setup(struct dw_hdmi *hdmi, struct >> drm_display_mode *mode) else >> hdmi->hdmi_data.enc_in_encoding = > V4L2_YCBCR_ENC_DEFAULT; >> >> - /* TOFIX: Default to RGB888 output format */ >> - hdmi->hdmi_data.enc_out_bus_format = MEDIA_BUS_FMT_RGB888_1X24; >> + if (hdmi->hdmi_data.enc_out_bus_format == MEDIA_BUS_FMT_FIXED) >> + hdmi->hdmi_data.enc_out_bus_format = > MEDIA_BUS_FMT_RGB888_1X24; >> >> hdmi->hdmi_data.pix_repet_factor = 0; >> hdmi->hdmi_data.hdcp_enable = 0; >> @@ -2388,6 +2387,267 @@ static const struct drm_connector_helper_funcs >> dw_hdmi_connector_helper_funcs = .atomic_check = >> dw_hdmi_connector_atomic_check, >> }; >> >> +/* >> + * Possible output formats : >> + * - MEDIA_BUS_FMT_UYYVYY16_0_5X48, >> + * - MEDIA_BUS_FMT_UYYVYY12_0_5X36, >> + * - MEDIA_BUS_FMT_UYYVYY10_0_5X30, >> + * - MEDIA_BUS_FMT_UYYVYY8_0_5X24, >> + * - MEDIA_BUS_FMT_YUV16_1X48, >> + * - MEDIA_BUS_FMT_RGB161616_1X48, >> + * - MEDIA_BUS_FMT_UYVY12_1X24, >> + * - MEDIA_BUS_FMT_YUV12_1X36, >> + * - MEDIA_BUS_FMT_RGB121212_1X36, >> + * - MEDIA_BUS_FMT_UYVY10_1X20, >> + * - MEDIA_BUS_FMT_YUV10_1X30, >> + * - MEDIA_BUS_FMT_RGB101010_1X30, >> + * - MEDIA_BUS_FMT_UYVY8_1X16, >> + * - MEDIA_BUS_FMT_YUV8_1X24, >> + * - MEDIA_BUS_FMT_RGB888_1X24, >> + */ >> + >> +/* Can return a maximum of 12 possible output formats for a mode/connector >> */ +#define MAX_OUTPUT_SEL_FORMATS 12 >> + >> +static u32 *dw_hdmi_bridge_atomic_get_output_bus_fmts(struct drm_bridge >> *bridge, + struct > drm_bridge_state *bridge_state, >> + struct drm_crtc_state > *crtc_state, >> + struct > drm_connector_state *conn_state, >> + unsigned int > *num_output_fmts) >> +{ >> + struct drm_connector *conn = conn_state->connector; >> + struct drm_display_info *info = &conn->display_info; >> + struct drm_display_mode *mode = &crtc_state->mode; >> + u8 max_bpc = conn_state->max_requested_bpc; >> + bool is_hdmi2_sink = info->hdmi.scdc.supported || >> + (info->color_formats & > DRM_COLOR_FORMAT_YCRCB420); >> + u32 *output_fmts; >> + int i = 0; >> + >> + *num_output_fmts = 0; >> + >> + output_fmts = kcalloc(MAX_OUTPUT_SEL_FORMATS, sizeof(*output_fmts), >> + GFP_KERNEL); >> + if (!output_fmts) >> + return NULL; >> + >> + /* >> + * If the current mode enforces 4:2:0, force the output but format >> + * to 4:2:0 and do not add the YUV422/444/RGB formats >> + */ >> + if (conn->ycbcr_420_allowed && >> + (drm_mode_is_420_only(info, mode) || >> + ())) { >> + >> + /* Order bus formats from 16bit to 8bit if supported */ >> + if (max_bpc >= 16 && info->bpc == 16 && >> + (info->hdmi.y420_dc_modes & > DRM_EDID_YCBCR420_DC_48)) >> + output_fmts[i++] = > MEDIA_BUS_FMT_UYYVYY16_0_5X48; >> + >> + if (max_bpc >= 12 && info->bpc >= 12 && >> + (info->hdmi.y420_dc_modes & > DRM_EDID_YCBCR420_DC_36)) >> + output_fmts[i++] = > MEDIA_BUS_FMT_UYYVYY12_0_5X36; >> + >> + if (max_bpc >= 10 && info->bpc >= 10 && >> + (info->hdmi.y420_dc_modes & > DRM_EDID_YCBCR420_DC_30)) >> + output_fmts[i++] = > MEDIA_BUS_FMT_UYYVYY10_0_5X30; >> + >> + /* Default 8bit fallback */ >> + output_fmts[i++] = MEDIA_BUS_FMT_UYYVYY8_0_5X24; >> + >> + *num_output_fmts = i; >> + >> + return output_fmts; > > Driver shouldn't return just yet for case "is_hdmi2_sink && > drm_mode_is_420_also(info, mode)", because monitor/TV also supports YCbCr > 4:4:4 in that case. IMO YCbCr 4:4:4 should be even prefered. What do you > think? I think we need to have some way for controller driver and userspace to control what hdmi output format gets selected. I know for a fact that some Samsung TV have issues with 444 YCbCr modes at 4k 50/60hz but have no problems with 420 modes. The Samsung TV edid lie and/or the TV is not fully following HDMI specs. From a personal and mediaplayer userspace perspective I would like to prefer 420/444 YCbCr mode as soon as any yuv drm plane is active and rgb 444 anytime else. On Rockchip SoCs the display controller cannot output yuv422 to dw-hdmi block, the optimal output format selection in such case should put yuv422 last. Maybe dw-hdmi can call a dw-hdmi glue driver callback to get the preferred output format order? On a side note but related issue, the dw-hdmi format negotiation code should probably also filter modes on tmds rate, something like [1]. It is needed to filter out deep color modes that is not supported by the sink or hdmi spec. [1] https://github.com/Kwiboo/linux-rockchip/commit/fc3df6903384e764ab6ac59879c489cbef55fcbe Best regards, Jonas > > Best regards, > Jernej > >> + } >> + >> + /* >> + * Order bus formats from 16bit to 8bit and from YUV422 to RGB >> + * if supported. In any case the default RGB888 format is added >> + */ >> + >> + if (max_bpc >= 16 && info->bpc == 16) { >> + if (info->color_formats & DRM_COLOR_FORMAT_YCRCB444) >> + output_fmts[i++] = MEDIA_BUS_FMT_YUV16_1X48; >> + >> + output_fmts[i++] = MEDIA_BUS_FMT_RGB161616_1X48; >> + } >> + >> + if (max_bpc >= 12 && info->bpc >= 12) { >> + if (info->color_formats & DRM_COLOR_FORMAT_YCRCB422) >> + output_fmts[i++] = MEDIA_BUS_FMT_UYVY12_1X24; >> + >> + if (info->color_formats & DRM_COLOR_FORMAT_YCRCB444) >> + output_fmts[i++] = MEDIA_BUS_FMT_YUV12_1X36; >> + >> + output_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36; >> + } >> + >> + if (max_bpc >= 10 && info->bpc >= 10) { >> + if (info->color_formats & DRM_COLOR_FORMAT_YCRCB422) >> + output_fmts[i++] = MEDIA_BUS_FMT_UYVY10_1X20; >> + >> + if (info->color_formats & DRM_COLOR_FORMAT_YCRCB444) >> + output_fmts[i++] = MEDIA_BUS_FMT_YUV10_1X30; >> + >> + output_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30; >> + } >> + >> + if (info->color_formats & DRM_COLOR_FORMAT_YCRCB422) >> + output_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16; >> + >> + if (info->color_formats & DRM_COLOR_FORMAT_YCRCB444) >> + output_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24; >> + >> + /* Default 8bit RGB fallback */ >> + output_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24; >> + >> + *num_output_fmts = i; >> + >> + return output_fmts; >> +} >> + >> +/* >> + * Possible input formats : >> + * - MEDIA_BUS_FMT_RGB888_1X24 >> + * - MEDIA_BUS_FMT_YUV8_1X24 >> + * - MEDIA_BUS_FMT_UYVY8_1X16 >> + * - MEDIA_BUS_FMT_UYYVYY8_0_5X24 >> + * - MEDIA_BUS_FMT_RGB101010_1X30 >> + * - MEDIA_BUS_FMT_YUV10_1X30 >> + * - MEDIA_BUS_FMT_UYVY10_1X20 >> + * - MEDIA_BUS_FMT_UYYVYY10_0_5X30 >> + * - MEDIA_BUS_FMT_RGB121212_1X36 >> + * - MEDIA_BUS_FMT_YUV12_1X36 >> + * - MEDIA_BUS_FMT_UYVY12_1X24 >> + * - MEDIA_BUS_FMT_UYYVYY12_0_5X36 >> + * - MEDIA_BUS_FMT_RGB161616_1X48 >> + * - MEDIA_BUS_FMT_YUV16_1X48 >> + * - MEDIA_BUS_FMT_UYYVYY16_0_5X48 >> + */ >> + >> +/* Can return a maximum of 4 possible input formats for an output format */ >> +#define MAX_INPUT_SEL_FORMATS 4 >> + >> +static u32 *dw_hdmi_bridge_atomic_get_input_bus_fmts(struct drm_bridge >> *bridge, + struct > drm_bridge_state *bridge_state, >> + struct drm_crtc_state > *crtc_state, >> + struct > drm_connector_state *conn_state, >> + u32 output_fmt, >> + unsigned int > *num_input_fmts) >> +{ >> + u32 *input_fmts; >> + int i = 0; >> + >> + *num_input_fmts = 0; >> + >> + input_fmts = kcalloc(MAX_INPUT_SEL_FORMATS, sizeof(*input_fmts), >> + GFP_KERNEL); >> + if (!input_fmts) >> + return NULL; >> + >> + switch (output_fmt) { >> + /* 8bit */ >> + case MEDIA_BUS_FMT_RGB888_1X24: >> + input_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24; >> + input_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24; >> + input_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16; >> + break; >> + case MEDIA_BUS_FMT_YUV8_1X24: >> + input_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24; >> + input_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16; >> + input_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24; >> + break; >> + case MEDIA_BUS_FMT_UYVY8_1X16: >> + input_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16; >> + input_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24; >> + input_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24; >> + break; >> + >> + /* 10bit */ >> + case MEDIA_BUS_FMT_RGB101010_1X30: >> + input_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30; >> + input_fmts[i++] = MEDIA_BUS_FMT_YUV10_1X30; >> + input_fmts[i++] = MEDIA_BUS_FMT_UYVY10_1X20; >> + break; >> + case MEDIA_BUS_FMT_YUV10_1X30: >> + input_fmts[i++] = MEDIA_BUS_FMT_YUV10_1X30; >> + input_fmts[i++] = MEDIA_BUS_FMT_UYVY10_1X20; >> + input_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30; >> + break; >> + case MEDIA_BUS_FMT_UYVY10_1X20: >> + input_fmts[i++] = MEDIA_BUS_FMT_UYVY10_1X20; >> + input_fmts[i++] = MEDIA_BUS_FMT_YUV10_1X30; >> + input_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30; >> + break; >> + >> + /* 12bit */ >> + case MEDIA_BUS_FMT_RGB121212_1X36: >> + input_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36; >> + input_fmts[i++] = MEDIA_BUS_FMT_YUV12_1X36; >> + input_fmts[i++] = MEDIA_BUS_FMT_UYVY12_1X24; >> + break; >> + case MEDIA_BUS_FMT_YUV12_1X36: >> + input_fmts[i++] = MEDIA_BUS_FMT_YUV12_1X36; >> + input_fmts[i++] = MEDIA_BUS_FMT_UYVY12_1X24; >> + input_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36; >> + break; >> + case MEDIA_BUS_FMT_UYVY12_1X24: >> + input_fmts[i++] = MEDIA_BUS_FMT_UYVY12_1X24; >> + input_fmts[i++] = MEDIA_BUS_FMT_YUV12_1X36; >> + input_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36; >> + break; >> + >> + /* 16bit */ >> + case MEDIA_BUS_FMT_RGB161616_1X48: >> + input_fmts[i++] = MEDIA_BUS_FMT_RGB161616_1X48; >> + input_fmts[i++] = MEDIA_BUS_FMT_YUV16_1X48; >> + break; >> + case MEDIA_BUS_FMT_YUV16_1X48: >> + input_fmts[i++] = MEDIA_BUS_FMT_YUV16_1X48; >> + input_fmts[i++] = MEDIA_BUS_FMT_RGB161616_1X48; >> + break; >> + >> + /* 420 */ >> + case MEDIA_BUS_FMT_UYYVYY8_0_5X24: >> + case MEDIA_BUS_FMT_UYYVYY10_0_5X30: >> + case MEDIA_BUS_FMT_UYYVYY12_0_5X36: >> + case MEDIA_BUS_FMT_UYYVYY16_0_5X48: >> + input_fmts[i++] = output_fmt; >> + break; >> + } >> + >> + *num_input_fmts = i; >> + >> + if (*num_input_fmts == 0) { >> + kfree(input_fmts); >> + input_fmts = NULL; >> + } >> + >> + return input_fmts; >> +} >> + >> +static int dw_hdmi_bridge_atomic_check(struct drm_bridge *bridge, >> + struct drm_bridge_state > *bridge_state, >> + struct drm_crtc_state > *crtc_state, >> + struct drm_connector_state > *conn_state) >> +{ >> + struct dw_hdmi *hdmi = bridge->driver_private; >> + >> + dev_dbg(hdmi->dev, "selected output format %x\n", >> + bridge_state->output_bus_cfg.format); >> + >> + hdmi->hdmi_data.enc_out_bus_format = >> + bridge_state->output_bus_cfg.format; >> + >> + dev_dbg(hdmi->dev, "selected input format %x\n", >> + bridge_state->input_bus_cfg.format); >> + >> + hdmi->hdmi_data.enc_in_bus_format = >> + bridge_state->input_bus_cfg.format; >> + >> + return 0; >> +} >> + >> static int dw_hdmi_bridge_attach(struct drm_bridge *bridge) >> { >> struct dw_hdmi *hdmi = bridge->driver_private; >> @@ -2499,6 +2759,9 @@ static const struct drm_bridge_funcs >> dw_hdmi_bridge_funcs = { .atomic_reset = drm_atomic_helper_bridge_reset, >> .attach = dw_hdmi_bridge_attach, >> .detach = dw_hdmi_bridge_detach, >> + .atomic_check = dw_hdmi_bridge_atomic_check, >> + .atomic_get_output_bus_fmts = > dw_hdmi_bridge_atomic_get_output_bus_fmts, >> + .atomic_get_input_bus_fmts = > dw_hdmi_bridge_atomic_get_input_bus_fmts, >> .enable = dw_hdmi_bridge_enable, >> .disable = dw_hdmi_bridge_disable, >> .mode_set = dw_hdmi_bridge_mode_set, >> @@ -2963,6 +3226,7 @@ __dw_hdmi_probe(struct platform_device *pdev, >> >> hdmi->bridge.driver_private = hdmi; >> hdmi->bridge.funcs = &dw_hdmi_bridge_funcs; >> + >> #ifdef CONFIG_OF >> hdmi->bridge.of_node = pdev->dev.of_node; >> #endif > > > >
Dne sobota, 29. februar 2020 ob 11:09:14 CET je Jonas Karlman napisal(a): > Hi Jernej, > > On 2020-02-29 08:42, Jernej Škrabec wrote: > > Hi Neil! > > > > Dne četrtek, 06. februar 2020 ob 20:18:27 CET je Neil Armstrong napisal(a): > >> Add the atomic_get_output_bus_fmts, atomic_get_input_bus_fmts to > >> negociate > >> the possible output and input formats for the current mode and monitor, > >> and use the negotiated formats in a basic atomic_check callback. > >> > >> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> > >> --- > >> > >> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 272 +++++++++++++++++++++- > >> 1 file changed, 268 insertions(+), 4 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > >> b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index > >> fec4a4bcd1fe..15048ad694bc 100644 > >> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > >> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > >> @@ -2095,11 +2095,10 @@ 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; > >> > >> - /* TOFIX: Get input format from plat data or fallback to RGB888 */ > >> > >> if (hdmi->plat_data->input_bus_format) > >> > >> hdmi->hdmi_data.enc_in_bus_format = > >> > >> hdmi->plat_data->input_bus_format; > >> > >> - else > >> + else if (hdmi->hdmi_data.enc_in_bus_format == MEDIA_BUS_FMT_FIXED) > >> > >> hdmi->hdmi_data.enc_in_bus_format = > > > > MEDIA_BUS_FMT_RGB888_1X24; > > > >> /* TOFIX: Get input encoding from plat data or fallback to none */ > >> > >> @@ -2109,8 +2108,8 @@ static int dw_hdmi_setup(struct dw_hdmi *hdmi, > >> struct > >> drm_display_mode *mode) else > >> > >> hdmi->hdmi_data.enc_in_encoding = > > > > V4L2_YCBCR_ENC_DEFAULT; > > > >> - /* TOFIX: Default to RGB888 output format */ > >> - hdmi->hdmi_data.enc_out_bus_format = MEDIA_BUS_FMT_RGB888_1X24; > >> + if (hdmi->hdmi_data.enc_out_bus_format == MEDIA_BUS_FMT_FIXED) > >> + hdmi->hdmi_data.enc_out_bus_format = > > > > MEDIA_BUS_FMT_RGB888_1X24; > > > >> hdmi->hdmi_data.pix_repet_factor = 0; > >> hdmi->hdmi_data.hdcp_enable = 0; > >> > >> @@ -2388,6 +2387,267 @@ static const struct drm_connector_helper_funcs > >> dw_hdmi_connector_helper_funcs = .atomic_check = > >> dw_hdmi_connector_atomic_check, > >> > >> }; > >> > >> +/* > >> + * Possible output formats : > >> + * - MEDIA_BUS_FMT_UYYVYY16_0_5X48, > >> + * - MEDIA_BUS_FMT_UYYVYY12_0_5X36, > >> + * - MEDIA_BUS_FMT_UYYVYY10_0_5X30, > >> + * - MEDIA_BUS_FMT_UYYVYY8_0_5X24, > >> + * - MEDIA_BUS_FMT_YUV16_1X48, > >> + * - MEDIA_BUS_FMT_RGB161616_1X48, > >> + * - MEDIA_BUS_FMT_UYVY12_1X24, > >> + * - MEDIA_BUS_FMT_YUV12_1X36, > >> + * - MEDIA_BUS_FMT_RGB121212_1X36, > >> + * - MEDIA_BUS_FMT_UYVY10_1X20, > >> + * - MEDIA_BUS_FMT_YUV10_1X30, > >> + * - MEDIA_BUS_FMT_RGB101010_1X30, > >> + * - MEDIA_BUS_FMT_UYVY8_1X16, > >> + * - MEDIA_BUS_FMT_YUV8_1X24, > >> + * - MEDIA_BUS_FMT_RGB888_1X24, > >> + */ > >> + > >> +/* Can return a maximum of 12 possible output formats for a > >> mode/connector > >> */ +#define MAX_OUTPUT_SEL_FORMATS 12 > >> + > >> +static u32 *dw_hdmi_bridge_atomic_get_output_bus_fmts(struct drm_bridge > >> *bridge, + struct > > > > drm_bridge_state *bridge_state, > > > >> + struct drm_crtc_state > > > > *crtc_state, > > > >> + struct > > > > drm_connector_state *conn_state, > > > >> + unsigned int > > > > *num_output_fmts) > > > >> +{ > >> + struct drm_connector *conn = conn_state->connector; > >> + struct drm_display_info *info = &conn->display_info; > >> + struct drm_display_mode *mode = &crtc_state->mode; > >> + u8 max_bpc = conn_state->max_requested_bpc; > >> + bool is_hdmi2_sink = info->hdmi.scdc.supported || > >> + (info->color_formats & > > > > DRM_COLOR_FORMAT_YCRCB420); > > > >> + u32 *output_fmts; > >> + int i = 0; > >> + > >> + *num_output_fmts = 0; > >> + > >> + output_fmts = kcalloc(MAX_OUTPUT_SEL_FORMATS, sizeof(*output_fmts), > >> + GFP_KERNEL); > >> + if (!output_fmts) > >> + return NULL; > >> + > >> + /* > >> + * If the current mode enforces 4:2:0, force the output but format > >> + * to 4:2:0 and do not add the YUV422/444/RGB formats > >> + */ > >> + if (conn->ycbcr_420_allowed && > >> + (drm_mode_is_420_only(info, mode) || > >> + ())) { > >> + > >> + /* Order bus formats from 16bit to 8bit if supported */ > >> + if (max_bpc >= 16 && info->bpc == 16 && > >> + (info->hdmi.y420_dc_modes & > > > > DRM_EDID_YCBCR420_DC_48)) > > > >> + output_fmts[i++] = > > > > MEDIA_BUS_FMT_UYYVYY16_0_5X48; > > > >> + > >> + if (max_bpc >= 12 && info->bpc >= 12 && > >> + (info->hdmi.y420_dc_modes & > > > > DRM_EDID_YCBCR420_DC_36)) > > > >> + output_fmts[i++] = > > > > MEDIA_BUS_FMT_UYYVYY12_0_5X36; > > > >> + > >> + if (max_bpc >= 10 && info->bpc >= 10 && > >> + (info->hdmi.y420_dc_modes & > > > > DRM_EDID_YCBCR420_DC_30)) > > > >> + output_fmts[i++] = > > > > MEDIA_BUS_FMT_UYYVYY10_0_5X30; > > > >> + > >> + /* Default 8bit fallback */ > >> + output_fmts[i++] = MEDIA_BUS_FMT_UYYVYY8_0_5X24; > >> + > >> + *num_output_fmts = i; > >> + > >> + return output_fmts; > > > > Driver shouldn't return just yet for case "is_hdmi2_sink && > > drm_mode_is_420_also(info, mode)", because monitor/TV also supports YCbCr > > 4:4:4 in that case. IMO YCbCr 4:4:4 should be even prefered. What do you > > think? > > I think we need to have some way for controller driver and userspace to > control what hdmi output format gets selected. I know for a fact that some > Samsung TV have issues with 444 YCbCr modes at 4k 50/60hz but have no > problems with 420 modes. The Samsung TV edid lie and/or the TV is not fully > following HDMI specs Interesting, maybe just some bandwith issues? I guess we should have a blacklist for such cases. I know that at least Allwinner BSP driver for DW HDMI has a blacklist, in this case for 2 monitors which claim to support YCbCr 4:4:4 mode but they not. > > From a personal and mediaplayer userspace perspective I would like to prefer > 420/444 YCbCr mode as soon as any yuv drm plane is active and rgb 444 > anytime else. I would argue that YCbCr is always prefered: - CEA 861 prefers it for all CEA modes - avoid dealing with quantization range - it's always limited range for < hdmi 2.0 and selectable on some hdmi 2.0 capable sinks Anyway, there is no universal solution to avoid color space conversion in all cases. For example, due to design of Allwinner Display Engine 2, it can only work in RGB format internally, but it can convert final output to YCbCr right before it's feeded to DW HDMI. On the other hand, you have meson display pipeline which works in YCbCr format internally and relies on DW HDMI CSC unit to convert output to RGB. Fortunately, there are also display pipelines which can work in any colorspace internally, like Allwinner Display Engine 3. Not sure in which category Rockchip display pipeline falls into. > > On Rockchip SoCs the display controller cannot output yuv422 to dw-hdmi > block, the optimal output format selection in such case should put yuv422 > last. Note that DW HDMI has decimation support which converts 444 to 422. This is part of CSC unit. Side note: CSC unit is optional feature of DW-HDMI and presence is indicated by config register. However, it seems that CSC support is broken in DW HDMI controller on 40nm Allwinner SoCs. If it is enabled, TV loses signal. I have to investigate if only CSC is broken or decimation also don't work. > > Maybe dw-hdmi can call a dw-hdmi glue driver callback to get the preferred > output format order? > > On a side note but related issue, the dw-hdmi format negotiation code should > probably also filter modes on tmds rate, something like [1]. > It is needed to filter out deep color modes that is not supported by the > sink or hdmi spec. Ah, you mean on TMDS rates supported by the sink. Could this avoid Samsung deep color issues? If so, we don't need blacklist then. Best regards, Jernej > > [1] > https://github.com/Kwiboo/linux-rockchip/commit/fc3df6903384e764ab6ac59879c > 489cbef55fcbe > > Best regards, > Jonas > > > Best regards, > > Jernej > > > >> + } > >> + > >> + /* > >> + * Order bus formats from 16bit to 8bit and from YUV422 to RGB > >> + * if supported. In any case the default RGB888 format is added > >> + */ > >> + > >> + if (max_bpc >= 16 && info->bpc == 16) { > >> + if (info->color_formats & DRM_COLOR_FORMAT_YCRCB444) > >> + output_fmts[i++] = MEDIA_BUS_FMT_YUV16_1X48; > >> + > >> + output_fmts[i++] = MEDIA_BUS_FMT_RGB161616_1X48; > >> + } > >> + > >> + if (max_bpc >= 12 && info->bpc >= 12) { > >> + if (info->color_formats & DRM_COLOR_FORMAT_YCRCB422) > >> + output_fmts[i++] = MEDIA_BUS_FMT_UYVY12_1X24; > >> + > >> + if (info->color_formats & DRM_COLOR_FORMAT_YCRCB444) > >> + output_fmts[i++] = MEDIA_BUS_FMT_YUV12_1X36; > >> + > >> + output_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36; > >> + } > >> + > >> + if (max_bpc >= 10 && info->bpc >= 10) { > >> + if (info->color_formats & DRM_COLOR_FORMAT_YCRCB422) > >> + output_fmts[i++] = MEDIA_BUS_FMT_UYVY10_1X20; > >> + > >> + if (info->color_formats & DRM_COLOR_FORMAT_YCRCB444) > >> + output_fmts[i++] = MEDIA_BUS_FMT_YUV10_1X30; > >> + > >> + output_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30; > >> + } > >> + > >> + if (info->color_formats & DRM_COLOR_FORMAT_YCRCB422) > >> + output_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16; > >> + > >> + if (info->color_formats & DRM_COLOR_FORMAT_YCRCB444) > >> + output_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24; > >> + > >> + /* Default 8bit RGB fallback */ > >> + output_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24; > >> + > >> + *num_output_fmts = i; > >> + > >> + return output_fmts; > >> +} > >> + > >> +/* > >> + * Possible input formats : > >> + * - MEDIA_BUS_FMT_RGB888_1X24 > >> + * - MEDIA_BUS_FMT_YUV8_1X24 > >> + * - MEDIA_BUS_FMT_UYVY8_1X16 > >> + * - MEDIA_BUS_FMT_UYYVYY8_0_5X24 > >> + * - MEDIA_BUS_FMT_RGB101010_1X30 > >> + * - MEDIA_BUS_FMT_YUV10_1X30 > >> + * - MEDIA_BUS_FMT_UYVY10_1X20 > >> + * - MEDIA_BUS_FMT_UYYVYY10_0_5X30 > >> + * - MEDIA_BUS_FMT_RGB121212_1X36 > >> + * - MEDIA_BUS_FMT_YUV12_1X36 > >> + * - MEDIA_BUS_FMT_UYVY12_1X24 > >> + * - MEDIA_BUS_FMT_UYYVYY12_0_5X36 > >> + * - MEDIA_BUS_FMT_RGB161616_1X48 > >> + * - MEDIA_BUS_FMT_YUV16_1X48 > >> + * - MEDIA_BUS_FMT_UYYVYY16_0_5X48 > >> + */ > >> + > >> +/* Can return a maximum of 4 possible input formats for an output format > >> */ +#define MAX_INPUT_SEL_FORMATS 4 > >> + > >> +static u32 *dw_hdmi_bridge_atomic_get_input_bus_fmts(struct drm_bridge > >> *bridge, + struct > > > > drm_bridge_state *bridge_state, > > > >> + struct drm_crtc_state > > > > *crtc_state, > > > >> + struct > > > > drm_connector_state *conn_state, > > > >> + u32 output_fmt, > >> + unsigned int > > > > *num_input_fmts) > > > >> +{ > >> + u32 *input_fmts; > >> + int i = 0; > >> + > >> + *num_input_fmts = 0; > >> + > >> + input_fmts = kcalloc(MAX_INPUT_SEL_FORMATS, sizeof(*input_fmts), > >> + GFP_KERNEL); > >> + if (!input_fmts) > >> + return NULL; > >> + > >> + switch (output_fmt) { > >> + /* 8bit */ > >> + case MEDIA_BUS_FMT_RGB888_1X24: > >> + input_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24; > >> + input_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24; > >> + input_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16; > >> + break; > >> + case MEDIA_BUS_FMT_YUV8_1X24: > >> + input_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24; > >> + input_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16; > >> + input_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24; > >> + break; > >> + case MEDIA_BUS_FMT_UYVY8_1X16: > >> + input_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16; > >> + input_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24; > >> + input_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24; > >> + break; > >> + > >> + /* 10bit */ > >> + case MEDIA_BUS_FMT_RGB101010_1X30: > >> + input_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30; > >> + input_fmts[i++] = MEDIA_BUS_FMT_YUV10_1X30; > >> + input_fmts[i++] = MEDIA_BUS_FMT_UYVY10_1X20; > >> + break; > >> + case MEDIA_BUS_FMT_YUV10_1X30: > >> + input_fmts[i++] = MEDIA_BUS_FMT_YUV10_1X30; > >> + input_fmts[i++] = MEDIA_BUS_FMT_UYVY10_1X20; > >> + input_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30; > >> + break; > >> + case MEDIA_BUS_FMT_UYVY10_1X20: > >> + input_fmts[i++] = MEDIA_BUS_FMT_UYVY10_1X20; > >> + input_fmts[i++] = MEDIA_BUS_FMT_YUV10_1X30; > >> + input_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30; > >> + break; > >> + > >> + /* 12bit */ > >> + case MEDIA_BUS_FMT_RGB121212_1X36: > >> + input_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36; > >> + input_fmts[i++] = MEDIA_BUS_FMT_YUV12_1X36; > >> + input_fmts[i++] = MEDIA_BUS_FMT_UYVY12_1X24; > >> + break; > >> + case MEDIA_BUS_FMT_YUV12_1X36: > >> + input_fmts[i++] = MEDIA_BUS_FMT_YUV12_1X36; > >> + input_fmts[i++] = MEDIA_BUS_FMT_UYVY12_1X24; > >> + input_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36; > >> + break; > >> + case MEDIA_BUS_FMT_UYVY12_1X24: > >> + input_fmts[i++] = MEDIA_BUS_FMT_UYVY12_1X24; > >> + input_fmts[i++] = MEDIA_BUS_FMT_YUV12_1X36; > >> + input_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36; > >> + break; > >> + > >> + /* 16bit */ > >> + case MEDIA_BUS_FMT_RGB161616_1X48: > >> + input_fmts[i++] = MEDIA_BUS_FMT_RGB161616_1X48; > >> + input_fmts[i++] = MEDIA_BUS_FMT_YUV16_1X48; > >> + break; > >> + case MEDIA_BUS_FMT_YUV16_1X48: > >> + input_fmts[i++] = MEDIA_BUS_FMT_YUV16_1X48; > >> + input_fmts[i++] = MEDIA_BUS_FMT_RGB161616_1X48; > >> + break; > >> + > >> + /* 420 */ > >> + case MEDIA_BUS_FMT_UYYVYY8_0_5X24: > >> + case MEDIA_BUS_FMT_UYYVYY10_0_5X30: > >> + case MEDIA_BUS_FMT_UYYVYY12_0_5X36: > >> + case MEDIA_BUS_FMT_UYYVYY16_0_5X48: > >> + input_fmts[i++] = output_fmt; > >> + break; > >> + } > >> + > >> + *num_input_fmts = i; > >> + > >> + if (*num_input_fmts == 0) { > >> + kfree(input_fmts); > >> + input_fmts = NULL; > >> + } > >> + > >> + return input_fmts; > >> +} > >> + > >> +static int dw_hdmi_bridge_atomic_check(struct drm_bridge *bridge, > >> + struct drm_bridge_state > > > > *bridge_state, > > > >> + struct drm_crtc_state > > > > *crtc_state, > > > >> + struct drm_connector_state > > > > *conn_state) > > > >> +{ > >> + struct dw_hdmi *hdmi = bridge->driver_private; > >> + > >> + dev_dbg(hdmi->dev, "selected output format %x\n", > >> + bridge_state->output_bus_cfg.format); > >> + > >> + hdmi->hdmi_data.enc_out_bus_format = > >> + bridge_state->output_bus_cfg.format; > >> + > >> + dev_dbg(hdmi->dev, "selected input format %x\n", > >> + bridge_state->input_bus_cfg.format); > >> + > >> + hdmi->hdmi_data.enc_in_bus_format = > >> + bridge_state->input_bus_cfg.format; > >> + > >> + return 0; > >> +} > >> + > >> > >> static int dw_hdmi_bridge_attach(struct drm_bridge *bridge) > >> { > >> > >> struct dw_hdmi *hdmi = bridge->driver_private; > >> > >> @@ -2499,6 +2759,9 @@ static const struct drm_bridge_funcs > >> dw_hdmi_bridge_funcs = { .atomic_reset = drm_atomic_helper_bridge_reset, > >> > >> .attach = dw_hdmi_bridge_attach, > >> .detach = dw_hdmi_bridge_detach, > >> > >> + .atomic_check = dw_hdmi_bridge_atomic_check, > >> + .atomic_get_output_bus_fmts = > > > > dw_hdmi_bridge_atomic_get_output_bus_fmts, > > > >> + .atomic_get_input_bus_fmts = > > > > dw_hdmi_bridge_atomic_get_input_bus_fmts, > > > >> .enable = dw_hdmi_bridge_enable, > >> .disable = dw_hdmi_bridge_disable, > >> .mode_set = dw_hdmi_bridge_mode_set, > >> > >> @@ -2963,6 +3226,7 @@ __dw_hdmi_probe(struct platform_device *pdev, > >> > >> hdmi->bridge.driver_private = hdmi; > >> hdmi->bridge.funcs = &dw_hdmi_bridge_funcs; > >> > >> + > >> > >> #ifdef CONFIG_OF > >> > >> hdmi->bridge.of_node = pdev->dev.of_node; > >> > >> #endif
On 2020-02-29 12:07, Jernej Škrabec wrote: > Dne sobota, 29. februar 2020 ob 11:09:14 CET je Jonas Karlman napisal(a): >> Hi Jernej, >> >> On 2020-02-29 08:42, Jernej Škrabec wrote: >>> Hi Neil! >>> >>> Dne četrtek, 06. februar 2020 ob 20:18:27 CET je Neil Armstrong > napisal(a): >>>> Add the atomic_get_output_bus_fmts, atomic_get_input_bus_fmts to >>>> negociate >>>> the possible output and input formats for the current mode and monitor, >>>> and use the negotiated formats in a basic atomic_check callback. >>>> >>>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> >>>> --- >>>> >>>> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 272 +++++++++++++++++++++- >>>> 1 file changed, 268 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c >>>> b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index >>>> fec4a4bcd1fe..15048ad694bc 100644 >>>> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c >>>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c >>>> @@ -2095,11 +2095,10 @@ 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; >>>> >>>> - /* TOFIX: Get input format from plat data or fallback to RGB888 */ >>>> >>>> if (hdmi->plat_data->input_bus_format) >>>> >>>> hdmi->hdmi_data.enc_in_bus_format = >>>> >>>> hdmi->plat_data->input_bus_format; >>>> >>>> - else >>>> + else if (hdmi->hdmi_data.enc_in_bus_format == MEDIA_BUS_FMT_FIXED) >>>> >>>> hdmi->hdmi_data.enc_in_bus_format = >>> >>> MEDIA_BUS_FMT_RGB888_1X24; >>> >>>> /* TOFIX: Get input encoding from plat data or fallback to none */ >>>> >>>> @@ -2109,8 +2108,8 @@ static int dw_hdmi_setup(struct dw_hdmi *hdmi, >>>> struct >>>> drm_display_mode *mode) else >>>> >>>> hdmi->hdmi_data.enc_in_encoding = >>> >>> V4L2_YCBCR_ENC_DEFAULT; >>> >>>> - /* TOFIX: Default to RGB888 output format */ >>>> - hdmi->hdmi_data.enc_out_bus_format = MEDIA_BUS_FMT_RGB888_1X24; >>>> + if (hdmi->hdmi_data.enc_out_bus_format == MEDIA_BUS_FMT_FIXED) >>>> + hdmi->hdmi_data.enc_out_bus_format = >>> >>> MEDIA_BUS_FMT_RGB888_1X24; >>> >>>> hdmi->hdmi_data.pix_repet_factor = 0; >>>> hdmi->hdmi_data.hdcp_enable = 0; >>>> >>>> @@ -2388,6 +2387,267 @@ static const struct drm_connector_helper_funcs >>>> dw_hdmi_connector_helper_funcs = .atomic_check = >>>> dw_hdmi_connector_atomic_check, >>>> >>>> }; >>>> >>>> +/* >>>> + * Possible output formats : >>>> + * - MEDIA_BUS_FMT_UYYVYY16_0_5X48, >>>> + * - MEDIA_BUS_FMT_UYYVYY12_0_5X36, >>>> + * - MEDIA_BUS_FMT_UYYVYY10_0_5X30, >>>> + * - MEDIA_BUS_FMT_UYYVYY8_0_5X24, >>>> + * - MEDIA_BUS_FMT_YUV16_1X48, >>>> + * - MEDIA_BUS_FMT_RGB161616_1X48, >>>> + * - MEDIA_BUS_FMT_UYVY12_1X24, >>>> + * - MEDIA_BUS_FMT_YUV12_1X36, >>>> + * - MEDIA_BUS_FMT_RGB121212_1X36, >>>> + * - MEDIA_BUS_FMT_UYVY10_1X20, >>>> + * - MEDIA_BUS_FMT_YUV10_1X30, >>>> + * - MEDIA_BUS_FMT_RGB101010_1X30, >>>> + * - MEDIA_BUS_FMT_UYVY8_1X16, >>>> + * - MEDIA_BUS_FMT_YUV8_1X24, >>>> + * - MEDIA_BUS_FMT_RGB888_1X24, >>>> + */ >>>> + >>>> +/* Can return a maximum of 12 possible output formats for a >>>> mode/connector >>>> */ +#define MAX_OUTPUT_SEL_FORMATS 12 >>>> + >>>> +static u32 *dw_hdmi_bridge_atomic_get_output_bus_fmts(struct drm_bridge >>>> *bridge, + struct >>> >>> drm_bridge_state *bridge_state, >>> >>>> + struct drm_crtc_state >>> >>> *crtc_state, >>> >>>> + struct >>> >>> drm_connector_state *conn_state, >>> >>>> + unsigned int >>> >>> *num_output_fmts) >>> >>>> +{ >>>> + struct drm_connector *conn = conn_state->connector; >>>> + struct drm_display_info *info = &conn->display_info; >>>> + struct drm_display_mode *mode = &crtc_state->mode; >>>> + u8 max_bpc = conn_state->max_requested_bpc; >>>> + bool is_hdmi2_sink = info->hdmi.scdc.supported || >>>> + (info->color_formats & >>> >>> DRM_COLOR_FORMAT_YCRCB420); >>> >>>> + u32 *output_fmts; >>>> + int i = 0; >>>> + >>>> + *num_output_fmts = 0; >>>> + >>>> + output_fmts = kcalloc(MAX_OUTPUT_SEL_FORMATS, > sizeof(*output_fmts), >>>> + GFP_KERNEL); >>>> + if (!output_fmts) >>>> + return NULL; >>>> + >>>> + /* >>>> + * If the current mode enforces 4:2:0, force the output but format >>>> + * to 4:2:0 and do not add the YUV422/444/RGB formats >>>> + */ >>>> + if (conn->ycbcr_420_allowed && >>>> + (drm_mode_is_420_only(info, mode) || >>>> + ())) { >>>> + >>>> + /* Order bus formats from 16bit to 8bit if supported */ >>>> + if (max_bpc >= 16 && info->bpc == 16 && >>>> + (info->hdmi.y420_dc_modes & >>> >>> DRM_EDID_YCBCR420_DC_48)) >>> >>>> + output_fmts[i++] = >>> >>> MEDIA_BUS_FMT_UYYVYY16_0_5X48; >>> >>>> + >>>> + if (max_bpc >= 12 && info->bpc >= 12 && >>>> + (info->hdmi.y420_dc_modes & >>> >>> DRM_EDID_YCBCR420_DC_36)) >>> >>>> + output_fmts[i++] = >>> >>> MEDIA_BUS_FMT_UYYVYY12_0_5X36; >>> >>>> + >>>> + if (max_bpc >= 10 && info->bpc >= 10 && >>>> + (info->hdmi.y420_dc_modes & >>> >>> DRM_EDID_YCBCR420_DC_30)) >>> >>>> + output_fmts[i++] = >>> >>> MEDIA_BUS_FMT_UYYVYY10_0_5X30; >>> >>>> + >>>> + /* Default 8bit fallback */ >>>> + output_fmts[i++] = MEDIA_BUS_FMT_UYYVYY8_0_5X24; >>>> + >>>> + *num_output_fmts = i; >>>> + >>>> + return output_fmts; >>> >>> Driver shouldn't return just yet for case "is_hdmi2_sink && >>> drm_mode_is_420_also(info, mode)", because monitor/TV also supports YCbCr >>> 4:4:4 in that case. IMO YCbCr 4:4:4 should be even prefered. What do you >>> think? >> >> I think we need to have some way for controller driver and userspace to >> control what hdmi output format gets selected. I know for a fact that some >> Samsung TV have issues with 444 YCbCr modes at 4k 50/60hz but have no >> problems with 420 modes. The Samsung TV edid lie and/or the TV is not fully >> following HDMI specs > > Interesting, maybe just some bandwith issues? I guess we should have a > blacklist for such cases. I know that at least Allwinner BSP driver for DW > HDMI has a blacklist, in this case for 2 monitors which claim to support YCbCr > 4:4:4 mode but they not. > >> >> From a personal and mediaplayer userspace perspective I would like to prefer >> 420/444 YCbCr mode as soon as any yuv drm plane is active and rgb 444 >> anytime else. > > I would argue that YCbCr is always prefered: > - CEA 861 prefers it for all CEA modes > - avoid dealing with quantization range - it's always limited range for < hdmi > 2.0 and selectable on some hdmi 2.0 capable sinks This is probably true if your sink is a TV, for monitors with HDMI connections I would expect or at least like to have an option to use rgb444 full range mode for any desktop or gaming or similar use-case. > > Anyway, there is no universal solution to avoid color space conversion in all > cases. For example, due to design of Allwinner Display Engine 2, it can only > work in RGB format internally, but it can convert final output to YCbCr right > before it's feeded to DW HDMI. On the other hand, you have meson display > pipeline which works in YCbCr format internally and relies on DW HDMI CSC unit > to convert output to RGB. Fortunately, there are also display pipelines which > can work in any colorspace internally, like Allwinner Display Engine 3. Not > sure in which category Rockchip display pipeline falls into. Agree, and for Rockchip to my knowledge RK3288 VOP (Video Output Processor) can only output 8/10-bit rgb (full range) to DW-HDMI. And the VOP in RK322x/RK3328/RK3399 can output 8/10-bit rgb and 444/420 yuv. I do not know how the VOP handles internal color conversion. > >> >> On Rockchip SoCs the display controller cannot output yuv422 to dw-hdmi >> block, the optimal output format selection in such case should put yuv422 >> last. > > Note that DW HDMI has decimation support which converts 444 to 422. This is > part of CSC unit. This works as it should (after a patch to disable color conversion for 444 to 422), the issue I see is that the format negotiation code will currently favor 444 to 422 decimation over using 444 output. Note: 422 will not be selected over 444 currently in case sink report deep color support due to drm edid parsing code following HDMI 1.3 spec, see [1]. [1] https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/drm_edid.c#L4801-L4806 > > Side note: CSC unit is optional feature of DW-HDMI and presence is indicated > by config register. However, it seems that CSC support is broken in DW HDMI > controller on 40nm Allwinner SoCs. If it is enabled, TV loses signal. I have > to investigate if only CSC is broken or decimation also don't work. > >> >> Maybe dw-hdmi can call a dw-hdmi glue driver callback to get the preferred >> output format order? >> >> On a side note but related issue, the dw-hdmi format negotiation code should >> probably also filter modes on tmds rate, something like [1]. >> It is needed to filter out deep color modes that is not supported by the >> sink or hdmi spec. > > Ah, you mean on TMDS rates supported by the sink. Could this avoid Samsung > deep color issues? If so, we don't need blacklist then. I do not think so, the initial issue was reported with intel graphics but looking at the edid and comparing to the manual of supported modes they do not fully match. See manual info at [2] and edid when UHD mode is off at [3] and when on at [4]. [2] https://gitlab.freedesktop.org/drm/intel/uploads/36630c8a727bf8c66a53fd7470c88383/MU7000_manual_-_UHD_colour_modes.PNG [3] http://ix.io/1H25 [4] http://ix.io/1H2e > > Best regards, > Jernej > >> >> [1] >> https://github.com/Kwiboo/linux-rockchip/commit/fc3df6903384e764ab6ac59879c >> 489cbef55fcbe >> >> Best regards, >> Jonas >> >>> Best regards, >>> Jernej >>> >>>> + } >>>> + >>>> + /* >>>> + * Order bus formats from 16bit to 8bit and from YUV422 to RGB >>>> + * if supported. In any case the default RGB888 format is added >>>> + */ >>>> + >>>> + if (max_bpc >= 16 && info->bpc == 16) { >>>> + if (info->color_formats & DRM_COLOR_FORMAT_YCRCB444) >>>> + output_fmts[i++] = MEDIA_BUS_FMT_YUV16_1X48; >>>> + >>>> + output_fmts[i++] = MEDIA_BUS_FMT_RGB161616_1X48; >>>> + } >>>> + >>>> + if (max_bpc >= 12 && info->bpc >= 12) { >>>> + if (info->color_formats & DRM_COLOR_FORMAT_YCRCB422) >>>> + output_fmts[i++] = > MEDIA_BUS_FMT_UYVY12_1X24; >>>> + >>>> + if (info->color_formats & DRM_COLOR_FORMAT_YCRCB444) >>>> + output_fmts[i++] = MEDIA_BUS_FMT_YUV12_1X36; >>>> + >>>> + output_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36; >>>> + } >>>> + >>>> + if (max_bpc >= 10 && info->bpc >= 10) { >>>> + if (info->color_formats & DRM_COLOR_FORMAT_YCRCB422) >>>> + output_fmts[i++] = > MEDIA_BUS_FMT_UYVY10_1X20; >>>> + >>>> + if (info->color_formats & DRM_COLOR_FORMAT_YCRCB444) >>>> + output_fmts[i++] = MEDIA_BUS_FMT_YUV10_1X30; >>>> + >>>> + output_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30; >>>> + } >>>> + >>>> + if (info->color_formats & DRM_COLOR_FORMAT_YCRCB422) >>>> + output_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16; >>>> + >>>> + if (info->color_formats & DRM_COLOR_FORMAT_YCRCB444) >>>> + output_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24; >>>> + >>>> + /* Default 8bit RGB fallback */ >>>> + output_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24; >>>> + >>>> + *num_output_fmts = i; >>>> + >>>> + return output_fmts; >>>> +} >>>> + >>>> +/* >>>> + * Possible input formats : >>>> + * - MEDIA_BUS_FMT_RGB888_1X24 >>>> + * - MEDIA_BUS_FMT_YUV8_1X24 >>>> + * - MEDIA_BUS_FMT_UYVY8_1X16 >>>> + * - MEDIA_BUS_FMT_UYYVYY8_0_5X24 >>>> + * - MEDIA_BUS_FMT_RGB101010_1X30 >>>> + * - MEDIA_BUS_FMT_YUV10_1X30 >>>> + * - MEDIA_BUS_FMT_UYVY10_1X20 >>>> + * - MEDIA_BUS_FMT_UYYVYY10_0_5X30 >>>> + * - MEDIA_BUS_FMT_RGB121212_1X36 >>>> + * - MEDIA_BUS_FMT_YUV12_1X36 >>>> + * - MEDIA_BUS_FMT_UYVY12_1X24 >>>> + * - MEDIA_BUS_FMT_UYYVYY12_0_5X36 >>>> + * - MEDIA_BUS_FMT_RGB161616_1X48 >>>> + * - MEDIA_BUS_FMT_YUV16_1X48 >>>> + * - MEDIA_BUS_FMT_UYYVYY16_0_5X48 >>>> + */ >>>> + >>>> +/* Can return a maximum of 4 possible input formats for an output format >>>> */ +#define MAX_INPUT_SEL_FORMATS 4 >>>> + >>>> +static u32 *dw_hdmi_bridge_atomic_get_input_bus_fmts(struct drm_bridge >>>> *bridge, + struct >>> >>> drm_bridge_state *bridge_state, >>> >>>> + struct drm_crtc_state >>> >>> *crtc_state, >>> >>>> + struct >>> >>> drm_connector_state *conn_state, >>> >>>> + u32 output_fmt, >>>> + unsigned int >>> >>> *num_input_fmts) >>> >>>> +{ >>>> + u32 *input_fmts; >>>> + int i = 0; >>>> + >>>> + *num_input_fmts = 0; >>>> + >>>> + input_fmts = kcalloc(MAX_INPUT_SEL_FORMATS, sizeof(*input_fmts), >>>> + GFP_KERNEL); >>>> + if (!input_fmts) >>>> + return NULL; >>>> + >>>> + switch (output_fmt) { >>>> + /* 8bit */ >>>> + case MEDIA_BUS_FMT_RGB888_1X24: >>>> + input_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24; >>>> + input_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24; >>>> + input_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16; >>>> + break; >>>> + case MEDIA_BUS_FMT_YUV8_1X24: >>>> + input_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24; >>>> + input_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16; >>>> + input_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24; >>>> + break; >>>> + case MEDIA_BUS_FMT_UYVY8_1X16: >>>> + input_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16; >>>> + input_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24; >>>> + input_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24; >>>> + break; >>>> + >>>> + /* 10bit */ >>>> + case MEDIA_BUS_FMT_RGB101010_1X30: >>>> + input_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30; >>>> + input_fmts[i++] = MEDIA_BUS_FMT_YUV10_1X30; >>>> + input_fmts[i++] = MEDIA_BUS_FMT_UYVY10_1X20; >>>> + break; >>>> + case MEDIA_BUS_FMT_YUV10_1X30: >>>> + input_fmts[i++] = MEDIA_BUS_FMT_YUV10_1X30; >>>> + input_fmts[i++] = MEDIA_BUS_FMT_UYVY10_1X20; >>>> + input_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30; >>>> + break; >>>> + case MEDIA_BUS_FMT_UYVY10_1X20: >>>> + input_fmts[i++] = MEDIA_BUS_FMT_UYVY10_1X20; >>>> + input_fmts[i++] = MEDIA_BUS_FMT_YUV10_1X30; >>>> + input_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30; >>>> + break; >>>> + >>>> + /* 12bit */ >>>> + case MEDIA_BUS_FMT_RGB121212_1X36: >>>> + input_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36; >>>> + input_fmts[i++] = MEDIA_BUS_FMT_YUV12_1X36; >>>> + input_fmts[i++] = MEDIA_BUS_FMT_UYVY12_1X24; >>>> + break; >>>> + case MEDIA_BUS_FMT_YUV12_1X36: >>>> + input_fmts[i++] = MEDIA_BUS_FMT_YUV12_1X36; >>>> + input_fmts[i++] = MEDIA_BUS_FMT_UYVY12_1X24; >>>> + input_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36; >>>> + break; >>>> + case MEDIA_BUS_FMT_UYVY12_1X24: >>>> + input_fmts[i++] = MEDIA_BUS_FMT_UYVY12_1X24; >>>> + input_fmts[i++] = MEDIA_BUS_FMT_YUV12_1X36; >>>> + input_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36; >>>> + break; >>>> + >>>> + /* 16bit */ >>>> + case MEDIA_BUS_FMT_RGB161616_1X48: >>>> + input_fmts[i++] = MEDIA_BUS_FMT_RGB161616_1X48; >>>> + input_fmts[i++] = MEDIA_BUS_FMT_YUV16_1X48; >>>> + break; >>>> + case MEDIA_BUS_FMT_YUV16_1X48: >>>> + input_fmts[i++] = MEDIA_BUS_FMT_YUV16_1X48; >>>> + input_fmts[i++] = MEDIA_BUS_FMT_RGB161616_1X48; >>>> + break; >>>> + >>>> + /* 420 */ >>>> + case MEDIA_BUS_FMT_UYYVYY8_0_5X24: >>>> + case MEDIA_BUS_FMT_UYYVYY10_0_5X30: >>>> + case MEDIA_BUS_FMT_UYYVYY12_0_5X36: >>>> + case MEDIA_BUS_FMT_UYYVYY16_0_5X48: >>>> + input_fmts[i++] = output_fmt; >>>> + break; >>>> + } >>>> + >>>> + *num_input_fmts = i; >>>> + >>>> + if (*num_input_fmts == 0) { >>>> + kfree(input_fmts); >>>> + input_fmts = NULL; >>>> + } >>>> + >>>> + return input_fmts; >>>> +} >>>> + >>>> +static int dw_hdmi_bridge_atomic_check(struct drm_bridge *bridge, >>>> + struct drm_bridge_state >>> >>> *bridge_state, >>> >>>> + struct drm_crtc_state >>> >>> *crtc_state, >>> >>>> + struct drm_connector_state >>> >>> *conn_state) >>> >>>> +{ >>>> + struct dw_hdmi *hdmi = bridge->driver_private; >>>> + >>>> + dev_dbg(hdmi->dev, "selected output format %x\n", >>>> + bridge_state->output_bus_cfg.format); >>>> + >>>> + hdmi->hdmi_data.enc_out_bus_format = >>>> + bridge_state->output_bus_cfg.format; >>>> + >>>> + dev_dbg(hdmi->dev, "selected input format %x\n", >>>> + bridge_state->input_bus_cfg.format); >>>> + >>>> + hdmi->hdmi_data.enc_in_bus_format = >>>> + bridge_state->input_bus_cfg.format; >>>> + >>>> + return 0; >>>> +} >>>> + >>>> >>>> static int dw_hdmi_bridge_attach(struct drm_bridge *bridge) >>>> { >>>> >>>> struct dw_hdmi *hdmi = bridge->driver_private; >>>> >>>> @@ -2499,6 +2759,9 @@ static const struct drm_bridge_funcs >>>> dw_hdmi_bridge_funcs = { .atomic_reset = drm_atomic_helper_bridge_reset, >>>> >>>> .attach = dw_hdmi_bridge_attach, >>>> .detach = dw_hdmi_bridge_detach, >>>> >>>> + .atomic_check = dw_hdmi_bridge_atomic_check, >>>> + .atomic_get_output_bus_fmts = >>> >>> dw_hdmi_bridge_atomic_get_output_bus_fmts, >>> >>>> + .atomic_get_input_bus_fmts = >>> >>> dw_hdmi_bridge_atomic_get_input_bus_fmts, >>> >>>> .enable = dw_hdmi_bridge_enable, >>>> .disable = dw_hdmi_bridge_disable, >>>> .mode_set = dw_hdmi_bridge_mode_set, >>>> >>>> @@ -2963,6 +3226,7 @@ __dw_hdmi_probe(struct platform_device *pdev, >>>> >>>> hdmi->bridge.driver_private = hdmi; >>>> hdmi->bridge.funcs = &dw_hdmi_bridge_funcs; >>>> >>>> + >>>> >>>> #ifdef CONFIG_OF >>>> >>>> hdmi->bridge.of_node = pdev->dev.of_node; >>>> >>>> #endif > > > >
Dne sobota, 29. februar 2020 ob 13:15:26 CET je Jonas Karlman napisal(a): > On 2020-02-29 12:07, Jernej Škrabec wrote: > > Dne sobota, 29. februar 2020 ob 11:09:14 CET je Jonas Karlman napisal(a): > >> Hi Jernej, > >> > >> On 2020-02-29 08:42, Jernej Škrabec wrote: > >>> Hi Neil! > >>> > >>> Dne četrtek, 06. februar 2020 ob 20:18:27 CET je Neil Armstrong > > > > napisal(a): > >>>> Add the atomic_get_output_bus_fmts, atomic_get_input_bus_fmts to > >>>> negociate > >>>> the possible output and input formats for the current mode and monitor, > >>>> and use the negotiated formats in a basic atomic_check callback. > >>>> > >>>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> > >>>> --- > >>>> > >>>> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 272 +++++++++++++++++++++- > >>>> 1 file changed, 268 insertions(+), 4 deletions(-) > >>>> > >>>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > >>>> b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index > >>>> fec4a4bcd1fe..15048ad694bc 100644 > >>>> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > >>>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > >>>> @@ -2095,11 +2095,10 @@ 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; > >>>> > >>>> - /* TOFIX: Get input format from plat data or fallback to RGB888 */ > >>>> > >>>> if (hdmi->plat_data->input_bus_format) > >>>> > >>>> hdmi->hdmi_data.enc_in_bus_format = > >>>> > >>>> hdmi->plat_data->input_bus_format; > >>>> > >>>> - else > >>>> + else if (hdmi->hdmi_data.enc_in_bus_format == MEDIA_BUS_FMT_FIXED) > >>>> > >>>> hdmi->hdmi_data.enc_in_bus_format = > >>> > >>> MEDIA_BUS_FMT_RGB888_1X24; > >>> > >>>> /* TOFIX: Get input encoding from plat data or fallback to none */ > >>>> > >>>> @@ -2109,8 +2108,8 @@ static int dw_hdmi_setup(struct dw_hdmi *hdmi, > >>>> struct > >>>> drm_display_mode *mode) else > >>>> > >>>> hdmi->hdmi_data.enc_in_encoding = > >>> > >>> V4L2_YCBCR_ENC_DEFAULT; > >>> > >>>> - /* TOFIX: Default to RGB888 output format */ > >>>> - hdmi->hdmi_data.enc_out_bus_format = MEDIA_BUS_FMT_RGB888_1X24; > >>>> + if (hdmi->hdmi_data.enc_out_bus_format == MEDIA_BUS_FMT_FIXED) > >>>> + hdmi->hdmi_data.enc_out_bus_format = > >>> > >>> MEDIA_BUS_FMT_RGB888_1X24; > >>> > >>>> hdmi->hdmi_data.pix_repet_factor = 0; > >>>> hdmi->hdmi_data.hdcp_enable = 0; > >>>> > >>>> @@ -2388,6 +2387,267 @@ static const struct drm_connector_helper_funcs > >>>> dw_hdmi_connector_helper_funcs = .atomic_check = > >>>> dw_hdmi_connector_atomic_check, > >>>> > >>>> }; > >>>> > >>>> +/* > >>>> + * Possible output formats : > >>>> + * - MEDIA_BUS_FMT_UYYVYY16_0_5X48, > >>>> + * - MEDIA_BUS_FMT_UYYVYY12_0_5X36, > >>>> + * - MEDIA_BUS_FMT_UYYVYY10_0_5X30, > >>>> + * - MEDIA_BUS_FMT_UYYVYY8_0_5X24, > >>>> + * - MEDIA_BUS_FMT_YUV16_1X48, > >>>> + * - MEDIA_BUS_FMT_RGB161616_1X48, > >>>> + * - MEDIA_BUS_FMT_UYVY12_1X24, > >>>> + * - MEDIA_BUS_FMT_YUV12_1X36, > >>>> + * - MEDIA_BUS_FMT_RGB121212_1X36, > >>>> + * - MEDIA_BUS_FMT_UYVY10_1X20, > >>>> + * - MEDIA_BUS_FMT_YUV10_1X30, > >>>> + * - MEDIA_BUS_FMT_RGB101010_1X30, > >>>> + * - MEDIA_BUS_FMT_UYVY8_1X16, > >>>> + * - MEDIA_BUS_FMT_YUV8_1X24, > >>>> + * - MEDIA_BUS_FMT_RGB888_1X24, > >>>> + */ > >>>> + > >>>> +/* Can return a maximum of 12 possible output formats for a > >>>> mode/connector > >>>> */ +#define MAX_OUTPUT_SEL_FORMATS 12 > >>>> + > >>>> +static u32 *dw_hdmi_bridge_atomic_get_output_bus_fmts(struct > >>>> drm_bridge > >>>> *bridge, + struct > >>> > >>> drm_bridge_state *bridge_state, > >>> > >>>> + struct drm_crtc_state > >>> > >>> *crtc_state, > >>> > >>>> + struct > >>> > >>> drm_connector_state *conn_state, > >>> > >>>> + unsigned int > >>> > >>> *num_output_fmts) > >>> > >>>> +{ > >>>> + struct drm_connector *conn = conn_state->connector; > >>>> + struct drm_display_info *info = &conn->display_info; > >>>> + struct drm_display_mode *mode = &crtc_state->mode; > >>>> + u8 max_bpc = conn_state->max_requested_bpc; > >>>> + bool is_hdmi2_sink = info->hdmi.scdc.supported || > >>>> + (info->color_formats & > >>> > >>> DRM_COLOR_FORMAT_YCRCB420); > >>> > >>>> + u32 *output_fmts; > >>>> + int i = 0; > >>>> + > >>>> + *num_output_fmts = 0; > >>>> + > >>>> + output_fmts = kcalloc(MAX_OUTPUT_SEL_FORMATS, > > > > sizeof(*output_fmts), > > > >>>> + GFP_KERNEL); > >>>> + if (!output_fmts) > >>>> + return NULL; > >>>> + > >>>> + /* > >>>> + * If the current mode enforces 4:2:0, force the output but format > >>>> + * to 4:2:0 and do not add the YUV422/444/RGB formats > >>>> + */ > >>>> + if (conn->ycbcr_420_allowed && > >>>> + (drm_mode_is_420_only(info, mode) || > >>>> + ())) { > >>>> + > >>>> + /* Order bus formats from 16bit to 8bit if supported */ > >>>> + if (max_bpc >= 16 && info->bpc == 16 && > >>>> + (info->hdmi.y420_dc_modes & > >>> > >>> DRM_EDID_YCBCR420_DC_48)) > >>> > >>>> + output_fmts[i++] = > >>> > >>> MEDIA_BUS_FMT_UYYVYY16_0_5X48; > >>> > >>>> + > >>>> + if (max_bpc >= 12 && info->bpc >= 12 && > >>>> + (info->hdmi.y420_dc_modes & > >>> > >>> DRM_EDID_YCBCR420_DC_36)) > >>> > >>>> + output_fmts[i++] = > >>> > >>> MEDIA_BUS_FMT_UYYVYY12_0_5X36; > >>> > >>>> + > >>>> + if (max_bpc >= 10 && info->bpc >= 10 && > >>>> + (info->hdmi.y420_dc_modes & > >>> > >>> DRM_EDID_YCBCR420_DC_30)) > >>> > >>>> + output_fmts[i++] = > >>> > >>> MEDIA_BUS_FMT_UYYVYY10_0_5X30; > >>> > >>>> + > >>>> + /* Default 8bit fallback */ > >>>> + output_fmts[i++] = MEDIA_BUS_FMT_UYYVYY8_0_5X24; > >>>> + > >>>> + *num_output_fmts = i; > >>>> + > >>>> + return output_fmts; > >>> > >>> Driver shouldn't return just yet for case "is_hdmi2_sink && > >>> drm_mode_is_420_also(info, mode)", because monitor/TV also supports > >>> YCbCr > >>> 4:4:4 in that case. IMO YCbCr 4:4:4 should be even prefered. What do you > >>> think? > >> > >> I think we need to have some way for controller driver and userspace to > >> control what hdmi output format gets selected. I know for a fact that > >> some > >> Samsung TV have issues with 444 YCbCr modes at 4k 50/60hz but have no > >> problems with 420 modes. The Samsung TV edid lie and/or the TV is not > >> fully > >> following HDMI specs > > > > Interesting, maybe just some bandwith issues? I guess we should have a > > blacklist for such cases. I know that at least Allwinner BSP driver for DW > > HDMI has a blacklist, in this case for 2 monitors which claim to support > > YCbCr 4:4:4 mode but they not. > > > >> From a personal and mediaplayer userspace perspective I would like to > >> prefer 420/444 YCbCr mode as soon as any yuv drm plane is active and rgb > >> 444 anytime else. > > > > I would argue that YCbCr is always prefered: > > - CEA 861 prefers it for all CEA modes > > - avoid dealing with quantization range - it's always limited range for < > > hdmi 2.0 and selectable on some hdmi 2.0 capable sinks > > This is probably true if your sink is a TV, for monitors with HDMI > connections I would expect or at least like to have an option to use rgb444 > full range mode for any desktop or gaming or similar use-case. Well, thing is that CEA 861 clearly states that for CEA modes (720p, 1080p, 2160p, etc.) limited range should be also used for RGB color space. With implemented logic to detect if RGB limited range is needed or not and indicating that in AVI frame, I finally get proper (better?) color reproduction with PC monitor and TV at the same time. I'm not sure if there is any way to detect "full range RGB" capable displays. However, we can always implement connector property like Intel did in i915 driver "Broadcast RGB", which can override RGB quantization mode. > > > Anyway, there is no universal solution to avoid color space conversion in > > all cases. For example, due to design of Allwinner Display Engine 2, it > > can only work in RGB format internally, but it can convert final output > > to YCbCr right before it's feeded to DW HDMI. On the other hand, you have > > meson display pipeline which works in YCbCr format internally and relies > > on DW HDMI CSC unit to convert output to RGB. Fortunately, there are also > > display pipelines which can work in any colorspace internally, like > > Allwinner Display Engine 3. Not sure in which category Rockchip display > > pipeline falls into. > > Agree, and for Rockchip to my knowledge RK3288 VOP (Video Output Processor) > can only output 8/10-bit rgb (full range) to DW-HDMI. > And the VOP in RK322x/RK3328/RK3399 can output 8/10-bit rgb and 444/420 yuv. > I do not know how the VOP handles internal color conversion. > > >> On Rockchip SoCs the display controller cannot output yuv422 to dw-hdmi > >> block, the optimal output format selection in such case should put yuv422 > >> last. > > > > Note that DW HDMI has decimation support which converts 444 to 422. This > > is > > part of CSC unit. > > This works as it should (after a patch to disable color conversion for 444 > to 422), Yeah, I noticed that too, I plan to send a patch for that soon. > the issue I see is that the format negotiation code will currently > favor 444 to 422 decimation over using 444 output. > Note: 422 will not be selected over 444 currently in case sink report deep > color support due to drm edid parsing code following HDMI 1.3 spec, see > [1]. > > [1] > https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/drm_edid.c#L4 > 801-L4806 > > Side note: CSC unit is optional feature of DW-HDMI and presence is > > indicated by config register. However, it seems that CSC support is > > broken in DW HDMI controller on 40nm Allwinner SoCs. If it is enabled, TV > > loses signal. I have to investigate if only CSC is broken or decimation > > also don't work.> Scratch above, DW HDMI CSC unit works ok on Allwinner SoCs, I just forgot to enable CSC clock. > >> Maybe dw-hdmi can call a dw-hdmi glue driver callback to get the > >> preferred > >> output format order? > >> > >> On a side note but related issue, the dw-hdmi format negotiation code > >> should probably also filter modes on tmds rate, something like [1]. > >> It is needed to filter out deep color modes that is not supported by the > >> sink or hdmi spec. > > > > Ah, you mean on TMDS rates supported by the sink. Could this avoid Samsung > > deep color issues? If so, we don't need blacklist then. > > I do not think so, the initial issue was reported with intel graphics but > looking at the edid and comparing to the manual of supported modes they do > not fully match. See manual info at [2] and edid when UHD mode is off at > [3] and when on at [4]. Which part doesn't match? I don't see any issue. Best regards, Jernej > > [2] > https://gitlab.freedesktop.org/drm/intel/uploads/36630c8a727bf8c66a53fd7470 > c88383/MU7000_manual_-_UHD_colour_modes.PNG [3] http://ix.io/1H25 > [4] http://ix.io/1H2e > > > Best regards, > > Jernej > > > >> [1] > >> https://github.com/Kwiboo/linux-rockchip/commit/fc3df6903384e764ab6ac5987 > >> 9c > >> 489cbef55fcbe > >> > >> Best regards, > >> Jonas > >> > >>> Best regards, > >>> Jernej > >>> > >>>> + } > >>>> + > >>>> + /* > >>>> + * Order bus formats from 16bit to 8bit and from YUV422 to RGB > >>>> + * if supported. In any case the default RGB888 format is added > >>>> + */ > >>>> + > >>>> + if (max_bpc >= 16 && info->bpc == 16) { > >>>> + if (info->color_formats & DRM_COLOR_FORMAT_YCRCB444) > >>>> + output_fmts[i++] = MEDIA_BUS_FMT_YUV16_1X48; > >>>> + > >>>> + output_fmts[i++] = MEDIA_BUS_FMT_RGB161616_1X48; > >>>> + } > >>>> + > >>>> + if (max_bpc >= 12 && info->bpc >= 12) { > >>>> + if (info->color_formats & DRM_COLOR_FORMAT_YCRCB422) > >>>> + output_fmts[i++] = > > > > MEDIA_BUS_FMT_UYVY12_1X24; > > > >>>> + > >>>> + if (info->color_formats & DRM_COLOR_FORMAT_YCRCB444) > >>>> + output_fmts[i++] = MEDIA_BUS_FMT_YUV12_1X36; > >>>> + > >>>> + output_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36; > >>>> + } > >>>> + > >>>> + if (max_bpc >= 10 && info->bpc >= 10) { > >>>> + if (info->color_formats & DRM_COLOR_FORMAT_YCRCB422) > >>>> + output_fmts[i++] = > > > > MEDIA_BUS_FMT_UYVY10_1X20; > > > >>>> + > >>>> + if (info->color_formats & DRM_COLOR_FORMAT_YCRCB444) > >>>> + output_fmts[i++] = MEDIA_BUS_FMT_YUV10_1X30; > >>>> + > >>>> + output_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30; > >>>> + } > >>>> + > >>>> + if (info->color_formats & DRM_COLOR_FORMAT_YCRCB422) > >>>> + output_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16; > >>>> + > >>>> + if (info->color_formats & DRM_COLOR_FORMAT_YCRCB444) > >>>> + output_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24; > >>>> + > >>>> + /* Default 8bit RGB fallback */ > >>>> + output_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24; > >>>> + > >>>> + *num_output_fmts = i; > >>>> + > >>>> + return output_fmts; > >>>> +} > >>>> + > >>>> +/* > >>>> + * Possible input formats : > >>>> + * - MEDIA_BUS_FMT_RGB888_1X24 > >>>> + * - MEDIA_BUS_FMT_YUV8_1X24 > >>>> + * - MEDIA_BUS_FMT_UYVY8_1X16 > >>>> + * - MEDIA_BUS_FMT_UYYVYY8_0_5X24 > >>>> + * - MEDIA_BUS_FMT_RGB101010_1X30 > >>>> + * - MEDIA_BUS_FMT_YUV10_1X30 > >>>> + * - MEDIA_BUS_FMT_UYVY10_1X20 > >>>> + * - MEDIA_BUS_FMT_UYYVYY10_0_5X30 > >>>> + * - MEDIA_BUS_FMT_RGB121212_1X36 > >>>> + * - MEDIA_BUS_FMT_YUV12_1X36 > >>>> + * - MEDIA_BUS_FMT_UYVY12_1X24 > >>>> + * - MEDIA_BUS_FMT_UYYVYY12_0_5X36 > >>>> + * - MEDIA_BUS_FMT_RGB161616_1X48 > >>>> + * - MEDIA_BUS_FMT_YUV16_1X48 > >>>> + * - MEDIA_BUS_FMT_UYYVYY16_0_5X48 > >>>> + */ > >>>> + > >>>> +/* Can return a maximum of 4 possible input formats for an output > >>>> format > >>>> */ +#define MAX_INPUT_SEL_FORMATS 4 > >>>> + > >>>> +static u32 *dw_hdmi_bridge_atomic_get_input_bus_fmts(struct drm_bridge > >>>> *bridge, + struct > >>> > >>> drm_bridge_state *bridge_state, > >>> > >>>> + struct drm_crtc_state > >>> > >>> *crtc_state, > >>> > >>>> + struct > >>> > >>> drm_connector_state *conn_state, > >>> > >>>> + u32 output_fmt, > >>>> + unsigned int > >>> > >>> *num_input_fmts) > >>> > >>>> +{ > >>>> + u32 *input_fmts; > >>>> + int i = 0; > >>>> + > >>>> + *num_input_fmts = 0; > >>>> + > >>>> + input_fmts = kcalloc(MAX_INPUT_SEL_FORMATS, sizeof(*input_fmts), > >>>> + GFP_KERNEL); > >>>> + if (!input_fmts) > >>>> + return NULL; > >>>> + > >>>> + switch (output_fmt) { > >>>> + /* 8bit */ > >>>> + case MEDIA_BUS_FMT_RGB888_1X24: > >>>> + input_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24; > >>>> + input_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24; > >>>> + input_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16; > >>>> + break; > >>>> + case MEDIA_BUS_FMT_YUV8_1X24: > >>>> + input_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24; > >>>> + input_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16; > >>>> + input_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24; > >>>> + break; > >>>> + case MEDIA_BUS_FMT_UYVY8_1X16: > >>>> + input_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16; > >>>> + input_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24; > >>>> + input_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24; > >>>> + break; > >>>> + > >>>> + /* 10bit */ > >>>> + case MEDIA_BUS_FMT_RGB101010_1X30: > >>>> + input_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30; > >>>> + input_fmts[i++] = MEDIA_BUS_FMT_YUV10_1X30; > >>>> + input_fmts[i++] = MEDIA_BUS_FMT_UYVY10_1X20; > >>>> + break; > >>>> + case MEDIA_BUS_FMT_YUV10_1X30: > >>>> + input_fmts[i++] = MEDIA_BUS_FMT_YUV10_1X30; > >>>> + input_fmts[i++] = MEDIA_BUS_FMT_UYVY10_1X20; > >>>> + input_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30; > >>>> + break; > >>>> + case MEDIA_BUS_FMT_UYVY10_1X20: > >>>> + input_fmts[i++] = MEDIA_BUS_FMT_UYVY10_1X20; > >>>> + input_fmts[i++] = MEDIA_BUS_FMT_YUV10_1X30; > >>>> + input_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30; > >>>> + break; > >>>> + > >>>> + /* 12bit */ > >>>> + case MEDIA_BUS_FMT_RGB121212_1X36: > >>>> + input_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36; > >>>> + input_fmts[i++] = MEDIA_BUS_FMT_YUV12_1X36; > >>>> + input_fmts[i++] = MEDIA_BUS_FMT_UYVY12_1X24; > >>>> + break; > >>>> + case MEDIA_BUS_FMT_YUV12_1X36: > >>>> + input_fmts[i++] = MEDIA_BUS_FMT_YUV12_1X36; > >>>> + input_fmts[i++] = MEDIA_BUS_FMT_UYVY12_1X24; > >>>> + input_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36; > >>>> + break; > >>>> + case MEDIA_BUS_FMT_UYVY12_1X24: > >>>> + input_fmts[i++] = MEDIA_BUS_FMT_UYVY12_1X24; > >>>> + input_fmts[i++] = MEDIA_BUS_FMT_YUV12_1X36; > >>>> + input_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36; > >>>> + break; > >>>> + > >>>> + /* 16bit */ > >>>> + case MEDIA_BUS_FMT_RGB161616_1X48: > >>>> + input_fmts[i++] = MEDIA_BUS_FMT_RGB161616_1X48; > >>>> + input_fmts[i++] = MEDIA_BUS_FMT_YUV16_1X48; > >>>> + break; > >>>> + case MEDIA_BUS_FMT_YUV16_1X48: > >>>> + input_fmts[i++] = MEDIA_BUS_FMT_YUV16_1X48; > >>>> + input_fmts[i++] = MEDIA_BUS_FMT_RGB161616_1X48; > >>>> + break; > >>>> + > >>>> + /* 420 */ > >>>> + case MEDIA_BUS_FMT_UYYVYY8_0_5X24: > >>>> + case MEDIA_BUS_FMT_UYYVYY10_0_5X30: > >>>> + case MEDIA_BUS_FMT_UYYVYY12_0_5X36: > >>>> + case MEDIA_BUS_FMT_UYYVYY16_0_5X48: > >>>> + input_fmts[i++] = output_fmt; > >>>> + break; > >>>> + } > >>>> + > >>>> + *num_input_fmts = i; > >>>> + > >>>> + if (*num_input_fmts == 0) { > >>>> + kfree(input_fmts); > >>>> + input_fmts = NULL; > >>>> + } > >>>> + > >>>> + return input_fmts; > >>>> +} > >>>> + > >>>> +static int dw_hdmi_bridge_atomic_check(struct drm_bridge *bridge, > >>>> + struct drm_bridge_state > >>> > >>> *bridge_state, > >>> > >>>> + struct drm_crtc_state > >>> > >>> *crtc_state, > >>> > >>>> + struct drm_connector_state > >>> > >>> *conn_state) > >>> > >>>> +{ > >>>> + struct dw_hdmi *hdmi = bridge->driver_private; > >>>> + > >>>> + dev_dbg(hdmi->dev, "selected output format %x\n", > >>>> + bridge_state->output_bus_cfg.format); > >>>> + > >>>> + hdmi->hdmi_data.enc_out_bus_format = > >>>> + bridge_state->output_bus_cfg.format; > >>>> + > >>>> + dev_dbg(hdmi->dev, "selected input format %x\n", > >>>> + bridge_state->input_bus_cfg.format); > >>>> + > >>>> + hdmi->hdmi_data.enc_in_bus_format = > >>>> + bridge_state->input_bus_cfg.format; > >>>> + > >>>> + return 0; > >>>> +} > >>>> + > >>>> > >>>> static int dw_hdmi_bridge_attach(struct drm_bridge *bridge) > >>>> { > >>>> > >>>> struct dw_hdmi *hdmi = bridge->driver_private; > >>>> > >>>> @@ -2499,6 +2759,9 @@ static const struct drm_bridge_funcs > >>>> dw_hdmi_bridge_funcs = { .atomic_reset = > >>>> drm_atomic_helper_bridge_reset, > >>>> > >>>> .attach = dw_hdmi_bridge_attach, > >>>> .detach = dw_hdmi_bridge_detach, > >>>> > >>>> + .atomic_check = dw_hdmi_bridge_atomic_check, > >>>> + .atomic_get_output_bus_fmts = > >>> > >>> dw_hdmi_bridge_atomic_get_output_bus_fmts, > >>> > >>>> + .atomic_get_input_bus_fmts = > >>> > >>> dw_hdmi_bridge_atomic_get_input_bus_fmts, > >>> > >>>> .enable = dw_hdmi_bridge_enable, > >>>> .disable = dw_hdmi_bridge_disable, > >>>> .mode_set = dw_hdmi_bridge_mode_set, > >>>> > >>>> @@ -2963,6 +3226,7 @@ __dw_hdmi_probe(struct platform_device *pdev, > >>>> > >>>> hdmi->bridge.driver_private = hdmi; > >>>> hdmi->bridge.funcs = &dw_hdmi_bridge_funcs; > >>>> > >>>> + > >>>> > >>>> #ifdef CONFIG_OF > >>>> > >>>> hdmi->bridge.of_node = pdev->dev.of_node; > >>>> > >>>> #endif
On 29/02/2020 08:42, Jernej Škrabec wrote: > Hi Neil! > > Dne četrtek, 06. februar 2020 ob 20:18:27 CET je Neil Armstrong napisal(a): >> Add the atomic_get_output_bus_fmts, atomic_get_input_bus_fmts to negociate >> the possible output and input formats for the current mode and monitor, >> and use the negotiated formats in a basic atomic_check callback. >> >> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> >> --- >> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 272 +++++++++++++++++++++- >> 1 file changed, 268 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c >> b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index >> fec4a4bcd1fe..15048ad694bc 100644 >> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c >> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c >> @@ -2095,11 +2095,10 @@ 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; >> >> - /* TOFIX: Get input format from plat data or fallback to RGB888 */ >> if (hdmi->plat_data->input_bus_format) >> hdmi->hdmi_data.enc_in_bus_format = >> hdmi->plat_data->input_bus_format; >> - else >> + else if (hdmi->hdmi_data.enc_in_bus_format == MEDIA_BUS_FMT_FIXED) >> hdmi->hdmi_data.enc_in_bus_format = > MEDIA_BUS_FMT_RGB888_1X24; >> >> /* TOFIX: Get input encoding from plat data or fallback to none */ >> @@ -2109,8 +2108,8 @@ static int dw_hdmi_setup(struct dw_hdmi *hdmi, struct >> drm_display_mode *mode) else >> hdmi->hdmi_data.enc_in_encoding = > V4L2_YCBCR_ENC_DEFAULT; >> >> - /* TOFIX: Default to RGB888 output format */ >> - hdmi->hdmi_data.enc_out_bus_format = MEDIA_BUS_FMT_RGB888_1X24; >> + if (hdmi->hdmi_data.enc_out_bus_format == MEDIA_BUS_FMT_FIXED) >> + hdmi->hdmi_data.enc_out_bus_format = > MEDIA_BUS_FMT_RGB888_1X24; >> >> hdmi->hdmi_data.pix_repet_factor = 0; >> hdmi->hdmi_data.hdcp_enable = 0; >> @@ -2388,6 +2387,267 @@ static const struct drm_connector_helper_funcs >> dw_hdmi_connector_helper_funcs = .atomic_check = >> dw_hdmi_connector_atomic_check, >> }; >> >> +/* >> + * Possible output formats : >> + * - MEDIA_BUS_FMT_UYYVYY16_0_5X48, >> + * - MEDIA_BUS_FMT_UYYVYY12_0_5X36, >> + * - MEDIA_BUS_FMT_UYYVYY10_0_5X30, >> + * - MEDIA_BUS_FMT_UYYVYY8_0_5X24, >> + * - MEDIA_BUS_FMT_YUV16_1X48, >> + * - MEDIA_BUS_FMT_RGB161616_1X48, >> + * - MEDIA_BUS_FMT_UYVY12_1X24, >> + * - MEDIA_BUS_FMT_YUV12_1X36, >> + * - MEDIA_BUS_FMT_RGB121212_1X36, >> + * - MEDIA_BUS_FMT_UYVY10_1X20, >> + * - MEDIA_BUS_FMT_YUV10_1X30, >> + * - MEDIA_BUS_FMT_RGB101010_1X30, >> + * - MEDIA_BUS_FMT_UYVY8_1X16, >> + * - MEDIA_BUS_FMT_YUV8_1X24, >> + * - MEDIA_BUS_FMT_RGB888_1X24, >> + */ >> + >> +/* Can return a maximum of 12 possible output formats for a mode/connector >> */ +#define MAX_OUTPUT_SEL_FORMATS 12 >> + >> +static u32 *dw_hdmi_bridge_atomic_get_output_bus_fmts(struct drm_bridge >> *bridge, + struct > drm_bridge_state *bridge_state, >> + struct drm_crtc_state > *crtc_state, >> + struct > drm_connector_state *conn_state, >> + unsigned int > *num_output_fmts) >> +{ >> + struct drm_connector *conn = conn_state->connector; >> + struct drm_display_info *info = &conn->display_info; >> + struct drm_display_mode *mode = &crtc_state->mode; >> + u8 max_bpc = conn_state->max_requested_bpc; >> + bool is_hdmi2_sink = info->hdmi.scdc.supported || >> + (info->color_formats & > DRM_COLOR_FORMAT_YCRCB420); >> + u32 *output_fmts; >> + int i = 0; >> + >> + *num_output_fmts = 0; >> + >> + output_fmts = kcalloc(MAX_OUTPUT_SEL_FORMATS, sizeof(*output_fmts), >> + GFP_KERNEL); >> + if (!output_fmts) >> + return NULL; >> + >> + /* >> + * If the current mode enforces 4:2:0, force the output but format >> + * to 4:2:0 and do not add the YUV422/444/RGB formats >> + */ >> + if (conn->ycbcr_420_allowed && >> + (drm_mode_is_420_only(info, mode) || >> + ())) { >> + >> + /* Order bus formats from 16bit to 8bit if supported */ >> + if (max_bpc >= 16 && info->bpc == 16 && >> + (info->hdmi.y420_dc_modes & > DRM_EDID_YCBCR420_DC_48)) >> + output_fmts[i++] = > MEDIA_BUS_FMT_UYYVYY16_0_5X48; >> + >> + if (max_bpc >= 12 && info->bpc >= 12 && >> + (info->hdmi.y420_dc_modes & > DRM_EDID_YCBCR420_DC_36)) >> + output_fmts[i++] = > MEDIA_BUS_FMT_UYYVYY12_0_5X36; >> + >> + if (max_bpc >= 10 && info->bpc >= 10 && >> + (info->hdmi.y420_dc_modes & > DRM_EDID_YCBCR420_DC_30)) >> + output_fmts[i++] = > MEDIA_BUS_FMT_UYYVYY10_0_5X30; >> + >> + /* Default 8bit fallback */ >> + output_fmts[i++] = MEDIA_BUS_FMT_UYYVYY8_0_5X24; >> + >> + *num_output_fmts = i; >> + >> + return output_fmts; > > Driver shouldn't return just yet for case "is_hdmi2_sink && > drm_mode_is_420_also(info, mode)", because monitor/TV also supports YCbCr > 4:4:4 in that case. IMO YCbCr 4:4:4 should be even prefered. What do you > think? The code is right. If drm_mode_is_420_only(info, mode), then the selected mode is 4:2:0 only, so only 4:2:0 bus formats will be allowed in this case. In this 4:2:0 only mode, the TV only support 4:2:0 4k60 modes and doesn't support SCDC and high tmds frequency pattern & co. Neil > > Best regards, > Jernej > >> + } >> + >> + /* >> + * Order bus formats from 16bit to 8bit and from YUV422 to RGB >> + * if supported. In any case the default RGB888 format is added >> + */ >> + >> + if (max_bpc >= 16 && info->bpc == 16) { >> + if (info->color_formats & DRM_COLOR_FORMAT_YCRCB444) >> + output_fmts[i++] = MEDIA_BUS_FMT_YUV16_1X48; >> + >> + output_fmts[i++] = MEDIA_BUS_FMT_RGB161616_1X48; >> + } >> + >> + if (max_bpc >= 12 && info->bpc >= 12) { >> + if (info->color_formats & DRM_COLOR_FORMAT_YCRCB422) >> + output_fmts[i++] = MEDIA_BUS_FMT_UYVY12_1X24; >> + >> + if (info->color_formats & DRM_COLOR_FORMAT_YCRCB444) >> + output_fmts[i++] = MEDIA_BUS_FMT_YUV12_1X36; >> + >> + output_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36; >> + } >> + >> + if (max_bpc >= 10 && info->bpc >= 10) { >> + if (info->color_formats & DRM_COLOR_FORMAT_YCRCB422) >> + output_fmts[i++] = MEDIA_BUS_FMT_UYVY10_1X20; >> + >> + if (info->color_formats & DRM_COLOR_FORMAT_YCRCB444) >> + output_fmts[i++] = MEDIA_BUS_FMT_YUV10_1X30; >> + >> + output_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30; >> + } >> + >> + if (info->color_formats & DRM_COLOR_FORMAT_YCRCB422) >> + output_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16; >> + >> + if (info->color_formats & DRM_COLOR_FORMAT_YCRCB444) >> + output_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24; >> + >> + /* Default 8bit RGB fallback */ >> + output_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24; >> + >> + *num_output_fmts = i; >> + >> + return output_fmts; >> +} >> + >> +/* >> + * Possible input formats : >> + * - MEDIA_BUS_FMT_RGB888_1X24 >> + * - MEDIA_BUS_FMT_YUV8_1X24 >> + * - MEDIA_BUS_FMT_UYVY8_1X16 >> + * - MEDIA_BUS_FMT_UYYVYY8_0_5X24 >> + * - MEDIA_BUS_FMT_RGB101010_1X30 >> + * - MEDIA_BUS_FMT_YUV10_1X30 >> + * - MEDIA_BUS_FMT_UYVY10_1X20 >> + * - MEDIA_BUS_FMT_UYYVYY10_0_5X30 >> + * - MEDIA_BUS_FMT_RGB121212_1X36 >> + * - MEDIA_BUS_FMT_YUV12_1X36 >> + * - MEDIA_BUS_FMT_UYVY12_1X24 >> + * - MEDIA_BUS_FMT_UYYVYY12_0_5X36 >> + * - MEDIA_BUS_FMT_RGB161616_1X48 >> + * - MEDIA_BUS_FMT_YUV16_1X48 >> + * - MEDIA_BUS_FMT_UYYVYY16_0_5X48 >> + */ >> + >> +/* Can return a maximum of 4 possible input formats for an output format */ >> +#define MAX_INPUT_SEL_FORMATS 4 >> + >> +static u32 *dw_hdmi_bridge_atomic_get_input_bus_fmts(struct drm_bridge >> *bridge, + struct > drm_bridge_state *bridge_state, >> + struct drm_crtc_state > *crtc_state, >> + struct > drm_connector_state *conn_state, >> + u32 output_fmt, >> + unsigned int > *num_input_fmts) >> +{ >> + u32 *input_fmts; >> + int i = 0; >> + >> + *num_input_fmts = 0; >> + >> + input_fmts = kcalloc(MAX_INPUT_SEL_FORMATS, sizeof(*input_fmts), >> + GFP_KERNEL); >> + if (!input_fmts) >> + return NULL; >> + >> + switch (output_fmt) { >> + /* 8bit */ >> + case MEDIA_BUS_FMT_RGB888_1X24: >> + input_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24; >> + input_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24; >> + input_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16; >> + break; >> + case MEDIA_BUS_FMT_YUV8_1X24: >> + input_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24; >> + input_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16; >> + input_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24; >> + break; >> + case MEDIA_BUS_FMT_UYVY8_1X16: >> + input_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16; >> + input_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24; >> + input_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24; >> + break; >> + >> + /* 10bit */ >> + case MEDIA_BUS_FMT_RGB101010_1X30: >> + input_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30; >> + input_fmts[i++] = MEDIA_BUS_FMT_YUV10_1X30; >> + input_fmts[i++] = MEDIA_BUS_FMT_UYVY10_1X20; >> + break; >> + case MEDIA_BUS_FMT_YUV10_1X30: >> + input_fmts[i++] = MEDIA_BUS_FMT_YUV10_1X30; >> + input_fmts[i++] = MEDIA_BUS_FMT_UYVY10_1X20; >> + input_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30; >> + break; >> + case MEDIA_BUS_FMT_UYVY10_1X20: >> + input_fmts[i++] = MEDIA_BUS_FMT_UYVY10_1X20; >> + input_fmts[i++] = MEDIA_BUS_FMT_YUV10_1X30; >> + input_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30; >> + break; >> + >> + /* 12bit */ >> + case MEDIA_BUS_FMT_RGB121212_1X36: >> + input_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36; >> + input_fmts[i++] = MEDIA_BUS_FMT_YUV12_1X36; >> + input_fmts[i++] = MEDIA_BUS_FMT_UYVY12_1X24; >> + break; >> + case MEDIA_BUS_FMT_YUV12_1X36: >> + input_fmts[i++] = MEDIA_BUS_FMT_YUV12_1X36; >> + input_fmts[i++] = MEDIA_BUS_FMT_UYVY12_1X24; >> + input_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36; >> + break; >> + case MEDIA_BUS_FMT_UYVY12_1X24: >> + input_fmts[i++] = MEDIA_BUS_FMT_UYVY12_1X24; >> + input_fmts[i++] = MEDIA_BUS_FMT_YUV12_1X36; >> + input_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36; >> + break; >> + >> + /* 16bit */ >> + case MEDIA_BUS_FMT_RGB161616_1X48: >> + input_fmts[i++] = MEDIA_BUS_FMT_RGB161616_1X48; >> + input_fmts[i++] = MEDIA_BUS_FMT_YUV16_1X48; >> + break; >> + case MEDIA_BUS_FMT_YUV16_1X48: >> + input_fmts[i++] = MEDIA_BUS_FMT_YUV16_1X48; >> + input_fmts[i++] = MEDIA_BUS_FMT_RGB161616_1X48; >> + break; >> + >> + /* 420 */ >> + case MEDIA_BUS_FMT_UYYVYY8_0_5X24: >> + case MEDIA_BUS_FMT_UYYVYY10_0_5X30: >> + case MEDIA_BUS_FMT_UYYVYY12_0_5X36: >> + case MEDIA_BUS_FMT_UYYVYY16_0_5X48: >> + input_fmts[i++] = output_fmt; >> + break; >> + } >> + >> + *num_input_fmts = i; >> + >> + if (*num_input_fmts == 0) { >> + kfree(input_fmts); >> + input_fmts = NULL; >> + } >> + >> + return input_fmts; >> +} >> + >> +static int dw_hdmi_bridge_atomic_check(struct drm_bridge *bridge, >> + struct drm_bridge_state > *bridge_state, >> + struct drm_crtc_state > *crtc_state, >> + struct drm_connector_state > *conn_state) >> +{ >> + struct dw_hdmi *hdmi = bridge->driver_private; >> + >> + dev_dbg(hdmi->dev, "selected output format %x\n", >> + bridge_state->output_bus_cfg.format); >> + >> + hdmi->hdmi_data.enc_out_bus_format = >> + bridge_state->output_bus_cfg.format; >> + >> + dev_dbg(hdmi->dev, "selected input format %x\n", >> + bridge_state->input_bus_cfg.format); >> + >> + hdmi->hdmi_data.enc_in_bus_format = >> + bridge_state->input_bus_cfg.format; >> + >> + return 0; >> +} >> + >> static int dw_hdmi_bridge_attach(struct drm_bridge *bridge) >> { >> struct dw_hdmi *hdmi = bridge->driver_private; >> @@ -2499,6 +2759,9 @@ static const struct drm_bridge_funcs >> dw_hdmi_bridge_funcs = { .atomic_reset = drm_atomic_helper_bridge_reset, >> .attach = dw_hdmi_bridge_attach, >> .detach = dw_hdmi_bridge_detach, >> + .atomic_check = dw_hdmi_bridge_atomic_check, >> + .atomic_get_output_bus_fmts = > dw_hdmi_bridge_atomic_get_output_bus_fmts, >> + .atomic_get_input_bus_fmts = > dw_hdmi_bridge_atomic_get_input_bus_fmts, >> .enable = dw_hdmi_bridge_enable, >> .disable = dw_hdmi_bridge_disable, >> .mode_set = dw_hdmi_bridge_mode_set, >> @@ -2963,6 +3226,7 @@ __dw_hdmi_probe(struct platform_device *pdev, >> >> hdmi->bridge.driver_private = hdmi; >> hdmi->bridge.funcs = &dw_hdmi_bridge_funcs; >> + >> #ifdef CONFIG_OF >> hdmi->bridge.of_node = pdev->dev.of_node; >> #endif > > > >
On 29/02/2020 11:09, Jonas Karlman wrote: > Hi Jernej, > > On 2020-02-29 08:42, Jernej Škrabec wrote: >> Hi Neil! >> >> Dne četrtek, 06. februar 2020 ob 20:18:27 CET je Neil Armstrong napisal(a): >>> Add the atomic_get_output_bus_fmts, atomic_get_input_bus_fmts to negociate >>> the possible output and input formats for the current mode and monitor, >>> and use the negotiated formats in a basic atomic_check callback. >>> >>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> >>> --- >>> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 272 +++++++++++++++++++++- >>> 1 file changed, 268 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c >>> b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index >>> fec4a4bcd1fe..15048ad694bc 100644 >>> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c >>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c >>> @@ -2095,11 +2095,10 @@ 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; >>> >>> - /* TOFIX: Get input format from plat data or fallback to RGB888 */ >>> if (hdmi->plat_data->input_bus_format) >>> hdmi->hdmi_data.enc_in_bus_format = >>> hdmi->plat_data->input_bus_format; >>> - else >>> + else if (hdmi->hdmi_data.enc_in_bus_format == MEDIA_BUS_FMT_FIXED) >>> hdmi->hdmi_data.enc_in_bus_format = >> MEDIA_BUS_FMT_RGB888_1X24; >>> >>> /* TOFIX: Get input encoding from plat data or fallback to none */ >>> @@ -2109,8 +2108,8 @@ static int dw_hdmi_setup(struct dw_hdmi *hdmi, struct >>> drm_display_mode *mode) else >>> hdmi->hdmi_data.enc_in_encoding = >> V4L2_YCBCR_ENC_DEFAULT; >>> >>> - /* TOFIX: Default to RGB888 output format */ >>> - hdmi->hdmi_data.enc_out_bus_format = MEDIA_BUS_FMT_RGB888_1X24; >>> + if (hdmi->hdmi_data.enc_out_bus_format == MEDIA_BUS_FMT_FIXED) >>> + hdmi->hdmi_data.enc_out_bus_format = >> MEDIA_BUS_FMT_RGB888_1X24; >>> >>> hdmi->hdmi_data.pix_repet_factor = 0; >>> hdmi->hdmi_data.hdcp_enable = 0; >>> @@ -2388,6 +2387,267 @@ static const struct drm_connector_helper_funcs >>> dw_hdmi_connector_helper_funcs = .atomic_check = >>> dw_hdmi_connector_atomic_check, >>> }; >>> >>> +/* >>> + * Possible output formats : >>> + * - MEDIA_BUS_FMT_UYYVYY16_0_5X48, >>> + * - MEDIA_BUS_FMT_UYYVYY12_0_5X36, >>> + * - MEDIA_BUS_FMT_UYYVYY10_0_5X30, >>> + * - MEDIA_BUS_FMT_UYYVYY8_0_5X24, >>> + * - MEDIA_BUS_FMT_YUV16_1X48, >>> + * - MEDIA_BUS_FMT_RGB161616_1X48, >>> + * - MEDIA_BUS_FMT_UYVY12_1X24, >>> + * - MEDIA_BUS_FMT_YUV12_1X36, >>> + * - MEDIA_BUS_FMT_RGB121212_1X36, >>> + * - MEDIA_BUS_FMT_UYVY10_1X20, >>> + * - MEDIA_BUS_FMT_YUV10_1X30, >>> + * - MEDIA_BUS_FMT_RGB101010_1X30, >>> + * - MEDIA_BUS_FMT_UYVY8_1X16, >>> + * - MEDIA_BUS_FMT_YUV8_1X24, >>> + * - MEDIA_BUS_FMT_RGB888_1X24, >>> + */ >>> + >>> +/* Can return a maximum of 12 possible output formats for a mode/connector >>> */ +#define MAX_OUTPUT_SEL_FORMATS 12 >>> + >>> +static u32 *dw_hdmi_bridge_atomic_get_output_bus_fmts(struct drm_bridge >>> *bridge, + struct >> drm_bridge_state *bridge_state, >>> + struct drm_crtc_state >> *crtc_state, >>> + struct >> drm_connector_state *conn_state, >>> + unsigned int >> *num_output_fmts) >>> +{ >>> + struct drm_connector *conn = conn_state->connector; >>> + struct drm_display_info *info = &conn->display_info; >>> + struct drm_display_mode *mode = &crtc_state->mode; >>> + u8 max_bpc = conn_state->max_requested_bpc; >>> + bool is_hdmi2_sink = info->hdmi.scdc.supported || >>> + (info->color_formats & >> DRM_COLOR_FORMAT_YCRCB420); >>> + u32 *output_fmts; >>> + int i = 0; >>> + >>> + *num_output_fmts = 0; >>> + >>> + output_fmts = kcalloc(MAX_OUTPUT_SEL_FORMATS, sizeof(*output_fmts), >>> + GFP_KERNEL); >>> + if (!output_fmts) >>> + return NULL; >>> + >>> + /* >>> + * If the current mode enforces 4:2:0, force the output but format >>> + * to 4:2:0 and do not add the YUV422/444/RGB formats >>> + */ >>> + if (conn->ycbcr_420_allowed && >>> + (drm_mode_is_420_only(info, mode) || >>> + ())) { >>> + >>> + /* Order bus formats from 16bit to 8bit if supported */ >>> + if (max_bpc >= 16 && info->bpc == 16 && >>> + (info->hdmi.y420_dc_modes & >> DRM_EDID_YCBCR420_DC_48)) >>> + output_fmts[i++] = >> MEDIA_BUS_FMT_UYYVYY16_0_5X48; >>> + >>> + if (max_bpc >= 12 && info->bpc >= 12 && >>> + (info->hdmi.y420_dc_modes & >> DRM_EDID_YCBCR420_DC_36)) >>> + output_fmts[i++] = >> MEDIA_BUS_FMT_UYYVYY12_0_5X36; >>> + >>> + if (max_bpc >= 10 && info->bpc >= 10 && >>> + (info->hdmi.y420_dc_modes & >> DRM_EDID_YCBCR420_DC_30)) >>> + output_fmts[i++] = >> MEDIA_BUS_FMT_UYYVYY10_0_5X30; >>> + >>> + /* Default 8bit fallback */ >>> + output_fmts[i++] = MEDIA_BUS_FMT_UYYVYY8_0_5X24; >>> + >>> + *num_output_fmts = i; >>> + >>> + return output_fmts; >> >> Driver shouldn't return just yet for case "is_hdmi2_sink && >> drm_mode_is_420_also(info, mode)", because monitor/TV also supports YCbCr >> 4:4:4 in that case. IMO YCbCr 4:4:4 should be even prefered. What do you >> think? > > I think we need to have some way for controller driver and userspace to control what > hdmi output format gets selected. I know for a fact that some Samsung TV have issues > with 444 YCbCr modes at 4k 50/60hz but have no problems with 420 modes. > The Samsung TV edid lie and/or the TV is not fully following HDMI specs. In this case this must be handled in the EDID quirks, by disabling SCDC & co for this TV. > > From a personal and mediaplayer userspace perspective I would like to prefer > 420/444 YCbCr mode as soon as any yuv drm plane is active and rgb 444 anytime else. > > On Rockchip SoCs the display controller cannot output yuv422 to dw-hdmi block, > the optimal output format selection in such case should put yuv422 last. I think 4:2:2 should be moved after 4:4:4 indeed, but on another side, 10bit cannot be done in 4k60 4:4:4, so only 4:2:2 is possible when the tv is not 4:2:0 capable. > > Maybe dw-hdmi can call a dw-hdmi glue driver callback to get the preferred output format order? This seems complex to handle... > > On a side note but related issue, the dw-hdmi format negotiation code should > probably also filter modes on tmds rate, something like [1]. > It is needed to filter out deep color modes that is not supported by the sink or hdmi spec. Indeed. > > [1] https://github.com/Kwiboo/linux-rockchip/commit/fc3df6903384e764ab6ac59879c489cbef55fcbe > > Best regards, > Jonas Neil > >> >> Best regards, >> Jernej >> >>> + } >>> + >>> + /* >>> + * Order bus formats from 16bit to 8bit and from YUV422 to RGB >>> + * if supported. In any case the default RGB888 format is added >>> + */ >>> + >>> + if (max_bpc >= 16 && info->bpc == 16) { >>> + if (info->color_formats & DRM_COLOR_FORMAT_YCRCB444) >>> + output_fmts[i++] = MEDIA_BUS_FMT_YUV16_1X48; >>> + >>> + output_fmts[i++] = MEDIA_BUS_FMT_RGB161616_1X48; >>> + } >>> + >>> + if (max_bpc >= 12 && info->bpc >= 12) { >>> + if (info->color_formats & DRM_COLOR_FORMAT_YCRCB422) >>> + output_fmts[i++] = MEDIA_BUS_FMT_UYVY12_1X24; >>> + >>> + if (info->color_formats & DRM_COLOR_FORMAT_YCRCB444) >>> + output_fmts[i++] = MEDIA_BUS_FMT_YUV12_1X36; >>> + >>> + output_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36; >>> + } >>> + >>> + if (max_bpc >= 10 && info->bpc >= 10) { >>> + if (info->color_formats & DRM_COLOR_FORMAT_YCRCB422) >>> + output_fmts[i++] = MEDIA_BUS_FMT_UYVY10_1X20; >>> + >>> + if (info->color_formats & DRM_COLOR_FORMAT_YCRCB444) >>> + output_fmts[i++] = MEDIA_BUS_FMT_YUV10_1X30; >>> + >>> + output_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30; >>> + } >>> + >>> + if (info->color_formats & DRM_COLOR_FORMAT_YCRCB422) >>> + output_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16; >>> + >>> + if (info->color_formats & DRM_COLOR_FORMAT_YCRCB444) >>> + output_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24; >>> + >>> + /* Default 8bit RGB fallback */ >>> + output_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24; >>> + >>> + *num_output_fmts = i; >>> + >>> + return output_fmts; >>> +} >>> + >>> +/* >>> + * Possible input formats : >>> + * - MEDIA_BUS_FMT_RGB888_1X24 >>> + * - MEDIA_BUS_FMT_YUV8_1X24 >>> + * - MEDIA_BUS_FMT_UYVY8_1X16 >>> + * - MEDIA_BUS_FMT_UYYVYY8_0_5X24 >>> + * - MEDIA_BUS_FMT_RGB101010_1X30 >>> + * - MEDIA_BUS_FMT_YUV10_1X30 >>> + * - MEDIA_BUS_FMT_UYVY10_1X20 >>> + * - MEDIA_BUS_FMT_UYYVYY10_0_5X30 >>> + * - MEDIA_BUS_FMT_RGB121212_1X36 >>> + * - MEDIA_BUS_FMT_YUV12_1X36 >>> + * - MEDIA_BUS_FMT_UYVY12_1X24 >>> + * - MEDIA_BUS_FMT_UYYVYY12_0_5X36 >>> + * - MEDIA_BUS_FMT_RGB161616_1X48 >>> + * - MEDIA_BUS_FMT_YUV16_1X48 >>> + * - MEDIA_BUS_FMT_UYYVYY16_0_5X48 >>> + */ >>> + >>> +/* Can return a maximum of 4 possible input formats for an output format */ >>> +#define MAX_INPUT_SEL_FORMATS 4 >>> + >>> +static u32 *dw_hdmi_bridge_atomic_get_input_bus_fmts(struct drm_bridge >>> *bridge, + struct >> drm_bridge_state *bridge_state, >>> + struct drm_crtc_state >> *crtc_state, >>> + struct >> drm_connector_state *conn_state, >>> + u32 output_fmt, >>> + unsigned int >> *num_input_fmts) >>> +{ >>> + u32 *input_fmts; >>> + int i = 0; >>> + >>> + *num_input_fmts = 0; >>> + >>> + input_fmts = kcalloc(MAX_INPUT_SEL_FORMATS, sizeof(*input_fmts), >>> + GFP_KERNEL); >>> + if (!input_fmts) >>> + return NULL; >>> + >>> + switch (output_fmt) { >>> + /* 8bit */ >>> + case MEDIA_BUS_FMT_RGB888_1X24: >>> + input_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24; >>> + input_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24; >>> + input_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16; >>> + break; >>> + case MEDIA_BUS_FMT_YUV8_1X24: >>> + input_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24; >>> + input_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16; >>> + input_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24; >>> + break; >>> + case MEDIA_BUS_FMT_UYVY8_1X16: >>> + input_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16; >>> + input_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24; >>> + input_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24; >>> + break; >>> + >>> + /* 10bit */ >>> + case MEDIA_BUS_FMT_RGB101010_1X30: >>> + input_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30; >>> + input_fmts[i++] = MEDIA_BUS_FMT_YUV10_1X30; >>> + input_fmts[i++] = MEDIA_BUS_FMT_UYVY10_1X20; >>> + break; >>> + case MEDIA_BUS_FMT_YUV10_1X30: >>> + input_fmts[i++] = MEDIA_BUS_FMT_YUV10_1X30; >>> + input_fmts[i++] = MEDIA_BUS_FMT_UYVY10_1X20; >>> + input_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30; >>> + break; >>> + case MEDIA_BUS_FMT_UYVY10_1X20: >>> + input_fmts[i++] = MEDIA_BUS_FMT_UYVY10_1X20; >>> + input_fmts[i++] = MEDIA_BUS_FMT_YUV10_1X30; >>> + input_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30; >>> + break; >>> + >>> + /* 12bit */ >>> + case MEDIA_BUS_FMT_RGB121212_1X36: >>> + input_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36; >>> + input_fmts[i++] = MEDIA_BUS_FMT_YUV12_1X36; >>> + input_fmts[i++] = MEDIA_BUS_FMT_UYVY12_1X24; >>> + break; >>> + case MEDIA_BUS_FMT_YUV12_1X36: >>> + input_fmts[i++] = MEDIA_BUS_FMT_YUV12_1X36; >>> + input_fmts[i++] = MEDIA_BUS_FMT_UYVY12_1X24; >>> + input_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36; >>> + break; >>> + case MEDIA_BUS_FMT_UYVY12_1X24: >>> + input_fmts[i++] = MEDIA_BUS_FMT_UYVY12_1X24; >>> + input_fmts[i++] = MEDIA_BUS_FMT_YUV12_1X36; >>> + input_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36; >>> + break; >>> + >>> + /* 16bit */ >>> + case MEDIA_BUS_FMT_RGB161616_1X48: >>> + input_fmts[i++] = MEDIA_BUS_FMT_RGB161616_1X48; >>> + input_fmts[i++] = MEDIA_BUS_FMT_YUV16_1X48; >>> + break; >>> + case MEDIA_BUS_FMT_YUV16_1X48: >>> + input_fmts[i++] = MEDIA_BUS_FMT_YUV16_1X48; >>> + input_fmts[i++] = MEDIA_BUS_FMT_RGB161616_1X48; >>> + break; >>> + >>> + /* 420 */ >>> + case MEDIA_BUS_FMT_UYYVYY8_0_5X24: >>> + case MEDIA_BUS_FMT_UYYVYY10_0_5X30: >>> + case MEDIA_BUS_FMT_UYYVYY12_0_5X36: >>> + case MEDIA_BUS_FMT_UYYVYY16_0_5X48: >>> + input_fmts[i++] = output_fmt; >>> + break; >>> + } >>> + >>> + *num_input_fmts = i; >>> + >>> + if (*num_input_fmts == 0) { >>> + kfree(input_fmts); >>> + input_fmts = NULL; >>> + } >>> + >>> + return input_fmts; >>> +} >>> + >>> +static int dw_hdmi_bridge_atomic_check(struct drm_bridge *bridge, >>> + struct drm_bridge_state >> *bridge_state, >>> + struct drm_crtc_state >> *crtc_state, >>> + struct drm_connector_state >> *conn_state) >>> +{ >>> + struct dw_hdmi *hdmi = bridge->driver_private; >>> + >>> + dev_dbg(hdmi->dev, "selected output format %x\n", >>> + bridge_state->output_bus_cfg.format); >>> + >>> + hdmi->hdmi_data.enc_out_bus_format = >>> + bridge_state->output_bus_cfg.format; >>> + >>> + dev_dbg(hdmi->dev, "selected input format %x\n", >>> + bridge_state->input_bus_cfg.format); >>> + >>> + hdmi->hdmi_data.enc_in_bus_format = >>> + bridge_state->input_bus_cfg.format; >>> + >>> + return 0; >>> +} >>> + >>> static int dw_hdmi_bridge_attach(struct drm_bridge *bridge) >>> { >>> struct dw_hdmi *hdmi = bridge->driver_private; >>> @@ -2499,6 +2759,9 @@ static const struct drm_bridge_funcs >>> dw_hdmi_bridge_funcs = { .atomic_reset = drm_atomic_helper_bridge_reset, >>> .attach = dw_hdmi_bridge_attach, >>> .detach = dw_hdmi_bridge_detach, >>> + .atomic_check = dw_hdmi_bridge_atomic_check, >>> + .atomic_get_output_bus_fmts = >> dw_hdmi_bridge_atomic_get_output_bus_fmts, >>> + .atomic_get_input_bus_fmts = >> dw_hdmi_bridge_atomic_get_input_bus_fmts, >>> .enable = dw_hdmi_bridge_enable, >>> .disable = dw_hdmi_bridge_disable, >>> .mode_set = dw_hdmi_bridge_mode_set, >>> @@ -2963,6 +3226,7 @@ __dw_hdmi_probe(struct platform_device *pdev, >>> >>> hdmi->bridge.driver_private = hdmi; >>> hdmi->bridge.funcs = &dw_hdmi_bridge_funcs; >>> + >>> #ifdef CONFIG_OF >>> hdmi->bridge.of_node = pdev->dev.of_node; >>> #endif >> >> >> >>
Hi Neil, Thank you for the patch. On Thu, Feb 06, 2020 at 08:18:27PM +0100, Neil Armstrong wrote: > Add the atomic_get_output_bus_fmts, atomic_get_input_bus_fmts to negociate > the possible output and input formats for the current mode and monitor, > and use the negotiated formats in a basic atomic_check callback. > > Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> > --- > drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 272 +++++++++++++++++++++- > 1 file changed, 268 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > index fec4a4bcd1fe..15048ad694bc 100644 > --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > @@ -2095,11 +2095,10 @@ 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; > > - /* TOFIX: Get input format from plat data or fallback to RGB888 */ > if (hdmi->plat_data->input_bus_format) > hdmi->hdmi_data.enc_in_bus_format = > hdmi->plat_data->input_bus_format; > - else > + else if (hdmi->hdmi_data.enc_in_bus_format == MEDIA_BUS_FMT_FIXED) > hdmi->hdmi_data.enc_in_bus_format = MEDIA_BUS_FMT_RGB888_1X24; > > /* TOFIX: Get input encoding from plat data or fallback to none */ > @@ -2109,8 +2108,8 @@ static int dw_hdmi_setup(struct dw_hdmi *hdmi, struct drm_display_mode *mode) > else > hdmi->hdmi_data.enc_in_encoding = V4L2_YCBCR_ENC_DEFAULT; > > - /* TOFIX: Default to RGB888 output format */ > - hdmi->hdmi_data.enc_out_bus_format = MEDIA_BUS_FMT_RGB888_1X24; > + if (hdmi->hdmi_data.enc_out_bus_format == MEDIA_BUS_FMT_FIXED) > + hdmi->hdmi_data.enc_out_bus_format = MEDIA_BUS_FMT_RGB888_1X24; > > hdmi->hdmi_data.pix_repet_factor = 0; > hdmi->hdmi_data.hdcp_enable = 0; > @@ -2388,6 +2387,267 @@ static const struct drm_connector_helper_funcs dw_hdmi_connector_helper_funcs = > .atomic_check = dw_hdmi_connector_atomic_check, > }; > > +/* > + * Possible output formats : > + * - MEDIA_BUS_FMT_UYYVYY16_0_5X48, > + * - MEDIA_BUS_FMT_UYYVYY12_0_5X36, > + * - MEDIA_BUS_FMT_UYYVYY10_0_5X30, > + * - MEDIA_BUS_FMT_UYYVYY8_0_5X24, > + * - MEDIA_BUS_FMT_YUV16_1X48, > + * - MEDIA_BUS_FMT_RGB161616_1X48, > + * - MEDIA_BUS_FMT_UYVY12_1X24, > + * - MEDIA_BUS_FMT_YUV12_1X36, > + * - MEDIA_BUS_FMT_RGB121212_1X36, > + * - MEDIA_BUS_FMT_UYVY10_1X20, > + * - MEDIA_BUS_FMT_YUV10_1X30, > + * - MEDIA_BUS_FMT_RGB101010_1X30, > + * - MEDIA_BUS_FMT_UYVY8_1X16, > + * - MEDIA_BUS_FMT_YUV8_1X24, > + * - MEDIA_BUS_FMT_RGB888_1X24, > + */ I'd drop this comment as I don't think it brings much, except for a risk of getting out of sync with the implementation below :-) > + > +/* Can return a maximum of 12 possible output formats for a mode/connector */ > +#define MAX_OUTPUT_SEL_FORMATS 12 I count 11 below. > + > +static u32 *dw_hdmi_bridge_atomic_get_output_bus_fmts(struct drm_bridge *bridge, > + struct drm_bridge_state *bridge_state, > + struct drm_crtc_state *crtc_state, > + struct drm_connector_state *conn_state, > + unsigned int *num_output_fmts) > +{ > + struct drm_connector *conn = conn_state->connector; > + struct drm_display_info *info = &conn->display_info; > + struct drm_display_mode *mode = &crtc_state->mode; > + u8 max_bpc = conn_state->max_requested_bpc; > + bool is_hdmi2_sink = info->hdmi.scdc.supported || > + (info->color_formats & DRM_COLOR_FORMAT_YCRCB420); > + u32 *output_fmts; > + int i = 0; i is never negative, you can make it an unsigned int. > + > + *num_output_fmts = 0; > + > + output_fmts = kcalloc(MAX_OUTPUT_SEL_FORMATS, sizeof(*output_fmts), > + GFP_KERNEL); > + if (!output_fmts) > + return NULL; > + > + /* > + * If the current mode enforces 4:2:0, force the output but format > + * to 4:2:0 and do not add the YUV422/444/RGB formats > + */ > + if (conn->ycbcr_420_allowed && > + (drm_mode_is_420_only(info, mode) || > + (is_hdmi2_sink && drm_mode_is_420_also(info, mode)))) { > + > + /* Order bus formats from 16bit to 8bit if supported */ > + if (max_bpc >= 16 && info->bpc == 16 && > + (info->hdmi.y420_dc_modes & DRM_EDID_YCBCR420_DC_48)) > + output_fmts[i++] = MEDIA_BUS_FMT_UYYVYY16_0_5X48; > + > + if (max_bpc >= 12 && info->bpc >= 12 && > + (info->hdmi.y420_dc_modes & DRM_EDID_YCBCR420_DC_36)) > + output_fmts[i++] = MEDIA_BUS_FMT_UYYVYY12_0_5X36; > + > + if (max_bpc >= 10 && info->bpc >= 10 && > + (info->hdmi.y420_dc_modes & DRM_EDID_YCBCR420_DC_30)) > + output_fmts[i++] = MEDIA_BUS_FMT_UYYVYY10_0_5X30; > + > + /* Default 8bit fallback */ > + output_fmts[i++] = MEDIA_BUS_FMT_UYYVYY8_0_5X24; > + > + *num_output_fmts = i; > + > + return output_fmts; > + } > + > + /* > + * Order bus formats from 16bit to 8bit and from YUV422 to RGB > + * if supported. In any case the default RGB888 format is added > + */ > + > + if (max_bpc >= 16 && info->bpc == 16) { > + if (info->color_formats & DRM_COLOR_FORMAT_YCRCB444) > + output_fmts[i++] = MEDIA_BUS_FMT_YUV16_1X48; > + > + output_fmts[i++] = MEDIA_BUS_FMT_RGB161616_1X48; > + } > + > + if (max_bpc >= 12 && info->bpc >= 12) { > + if (info->color_formats & DRM_COLOR_FORMAT_YCRCB422) > + output_fmts[i++] = MEDIA_BUS_FMT_UYVY12_1X24; > + > + if (info->color_formats & DRM_COLOR_FORMAT_YCRCB444) > + output_fmts[i++] = MEDIA_BUS_FMT_YUV12_1X36; > + > + output_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36; > + } > + > + if (max_bpc >= 10 && info->bpc >= 10) { > + if (info->color_formats & DRM_COLOR_FORMAT_YCRCB422) > + output_fmts[i++] = MEDIA_BUS_FMT_UYVY10_1X20; > + > + if (info->color_formats & DRM_COLOR_FORMAT_YCRCB444) > + output_fmts[i++] = MEDIA_BUS_FMT_YUV10_1X30; > + > + output_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30; > + } > + > + if (info->color_formats & DRM_COLOR_FORMAT_YCRCB422) > + output_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16; > + > + if (info->color_formats & DRM_COLOR_FORMAT_YCRCB444) > + output_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24; > + > + /* Default 8bit RGB fallback */ > + output_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24; > + > + *num_output_fmts = i; > + > + return output_fmts; > +} > + > +/* > + * Possible input formats : > + * - MEDIA_BUS_FMT_RGB888_1X24 > + * - MEDIA_BUS_FMT_YUV8_1X24 > + * - MEDIA_BUS_FMT_UYVY8_1X16 > + * - MEDIA_BUS_FMT_UYYVYY8_0_5X24 > + * - MEDIA_BUS_FMT_RGB101010_1X30 > + * - MEDIA_BUS_FMT_YUV10_1X30 > + * - MEDIA_BUS_FMT_UYVY10_1X20 > + * - MEDIA_BUS_FMT_UYYVYY10_0_5X30 > + * - MEDIA_BUS_FMT_RGB121212_1X36 > + * - MEDIA_BUS_FMT_YUV12_1X36 > + * - MEDIA_BUS_FMT_UYVY12_1X24 > + * - MEDIA_BUS_FMT_UYYVYY12_0_5X36 > + * - MEDIA_BUS_FMT_RGB161616_1X48 > + * - MEDIA_BUS_FMT_YUV16_1X48 > + * - MEDIA_BUS_FMT_UYYVYY16_0_5X48 > + */ Same here. > + > +/* Can return a maximum of 4 possible input formats for an output format */ > +#define MAX_INPUT_SEL_FORMATS 4 As Boris pointed out, that should be 3. > + > +static u32 *dw_hdmi_bridge_atomic_get_input_bus_fmts(struct drm_bridge *bridge, > + struct drm_bridge_state *bridge_state, > + struct drm_crtc_state *crtc_state, > + struct drm_connector_state *conn_state, > + u32 output_fmt, > + unsigned int *num_input_fmts) > +{ > + u32 *input_fmts; > + int i = 0; i is never negative, you can make it an unsigned int. > + > + *num_input_fmts = 0; > + > + input_fmts = kcalloc(MAX_INPUT_SEL_FORMATS, sizeof(*input_fmts), > + GFP_KERNEL); > + if (!input_fmts) > + return NULL; > + > + switch (output_fmt) { > + /* 8bit */ > + case MEDIA_BUS_FMT_RGB888_1X24: > + input_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24; > + input_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24; > + input_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16; > + break; > + case MEDIA_BUS_FMT_YUV8_1X24: > + input_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24; > + input_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16; > + input_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24; > + break; > + case MEDIA_BUS_FMT_UYVY8_1X16: > + input_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16; > + input_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24; > + input_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24; > + break; > + > + /* 10bit */ > + case MEDIA_BUS_FMT_RGB101010_1X30: > + input_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30; > + input_fmts[i++] = MEDIA_BUS_FMT_YUV10_1X30; > + input_fmts[i++] = MEDIA_BUS_FMT_UYVY10_1X20; > + break; > + case MEDIA_BUS_FMT_YUV10_1X30: > + input_fmts[i++] = MEDIA_BUS_FMT_YUV10_1X30; > + input_fmts[i++] = MEDIA_BUS_FMT_UYVY10_1X20; > + input_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30; > + break; > + case MEDIA_BUS_FMT_UYVY10_1X20: > + input_fmts[i++] = MEDIA_BUS_FMT_UYVY10_1X20; > + input_fmts[i++] = MEDIA_BUS_FMT_YUV10_1X30; > + input_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30; > + break; > + > + /* 12bit */ > + case MEDIA_BUS_FMT_RGB121212_1X36: > + input_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36; > + input_fmts[i++] = MEDIA_BUS_FMT_YUV12_1X36; > + input_fmts[i++] = MEDIA_BUS_FMT_UYVY12_1X24; > + break; > + case MEDIA_BUS_FMT_YUV12_1X36: > + input_fmts[i++] = MEDIA_BUS_FMT_YUV12_1X36; > + input_fmts[i++] = MEDIA_BUS_FMT_UYVY12_1X24; > + input_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36; > + break; > + case MEDIA_BUS_FMT_UYVY12_1X24: > + input_fmts[i++] = MEDIA_BUS_FMT_UYVY12_1X24; > + input_fmts[i++] = MEDIA_BUS_FMT_YUV12_1X36; > + input_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36; > + break; > + > + /* 16bit */ > + case MEDIA_BUS_FMT_RGB161616_1X48: > + input_fmts[i++] = MEDIA_BUS_FMT_RGB161616_1X48; > + input_fmts[i++] = MEDIA_BUS_FMT_YUV16_1X48; > + break; > + case MEDIA_BUS_FMT_YUV16_1X48: > + input_fmts[i++] = MEDIA_BUS_FMT_YUV16_1X48; > + input_fmts[i++] = MEDIA_BUS_FMT_RGB161616_1X48; > + break; > + > + /* 420 */ s/420/YUV 4:2:0/ ? > + case MEDIA_BUS_FMT_UYYVYY8_0_5X24: > + case MEDIA_BUS_FMT_UYYVYY10_0_5X30: > + case MEDIA_BUS_FMT_UYYVYY12_0_5X36: > + case MEDIA_BUS_FMT_UYYVYY16_0_5X48: > + input_fmts[i++] = output_fmt; > + break; > + } > + > + *num_input_fmts = i; > + > + if (*num_input_fmts == 0) { > + kfree(input_fmts); > + input_fmts = NULL; > + } > + > + return input_fmts; > +} > + > +static int dw_hdmi_bridge_atomic_check(struct drm_bridge *bridge, > + struct drm_bridge_state *bridge_state, > + struct drm_crtc_state *crtc_state, > + struct drm_connector_state *conn_state) > +{ > + struct dw_hdmi *hdmi = bridge->driver_private; > + > + dev_dbg(hdmi->dev, "selected output format %x\n", s/%x/0x%04x/ > + bridge_state->output_bus_cfg.format); Misalignment ? > + > + hdmi->hdmi_data.enc_out_bus_format = > + bridge_state->output_bus_cfg.format; > + > + dev_dbg(hdmi->dev, "selected input format %x\n", > + bridge_state->input_bus_cfg.format); Same here. I would combine both messages: dev_dbg(hdmi->dev, "input format: 0x%04x, output format: 0x04x\n", bridge_state->input_bus_cfg.format, bridge_state->output_bus_cfg.format); > + > + hdmi->hdmi_data.enc_in_bus_format = > + bridge_state->input_bus_cfg.format; > + > + return 0; > +} > + > static int dw_hdmi_bridge_attach(struct drm_bridge *bridge) > { > struct dw_hdmi *hdmi = bridge->driver_private; > @@ -2499,6 +2759,9 @@ static const struct drm_bridge_funcs dw_hdmi_bridge_funcs = { > .atomic_reset = drm_atomic_helper_bridge_reset, > .attach = dw_hdmi_bridge_attach, > .detach = dw_hdmi_bridge_detach, > + .atomic_check = dw_hdmi_bridge_atomic_check, > + .atomic_get_output_bus_fmts = dw_hdmi_bridge_atomic_get_output_bus_fmts, > + .atomic_get_input_bus_fmts = dw_hdmi_bridge_atomic_get_input_bus_fmts, > .enable = dw_hdmi_bridge_enable, > .disable = dw_hdmi_bridge_disable, > .mode_set = dw_hdmi_bridge_mode_set, > @@ -2963,6 +3226,7 @@ __dw_hdmi_probe(struct platform_device *pdev, > > hdmi->bridge.driver_private = hdmi; > hdmi->bridge.funcs = &dw_hdmi_bridge_funcs; > + This seems unrelated. > #ifdef CONFIG_OF > hdmi->bridge.of_node = pdev->dev.of_node; > #endif
On 02/03/2020 11:18, Laurent Pinchart wrote: > Hi Neil, > > Thank you for the patch. > > On Thu, Feb 06, 2020 at 08:18:27PM +0100, Neil Armstrong wrote: >> Add the atomic_get_output_bus_fmts, atomic_get_input_bus_fmts to negociate >> the possible output and input formats for the current mode and monitor, >> and use the negotiated formats in a basic atomic_check callback. >> >> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> >> --- >> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 272 +++++++++++++++++++++- >> 1 file changed, 268 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c >> index fec4a4bcd1fe..15048ad694bc 100644 >> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c >> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c >> @@ -2095,11 +2095,10 @@ 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; >> >> - /* TOFIX: Get input format from plat data or fallback to RGB888 */ >> if (hdmi->plat_data->input_bus_format) >> hdmi->hdmi_data.enc_in_bus_format = >> hdmi->plat_data->input_bus_format; >> - else >> + else if (hdmi->hdmi_data.enc_in_bus_format == MEDIA_BUS_FMT_FIXED) >> hdmi->hdmi_data.enc_in_bus_format = MEDIA_BUS_FMT_RGB888_1X24; >> >> /* TOFIX: Get input encoding from plat data or fallback to none */ >> @@ -2109,8 +2108,8 @@ static int dw_hdmi_setup(struct dw_hdmi *hdmi, struct drm_display_mode *mode) >> else >> hdmi->hdmi_data.enc_in_encoding = V4L2_YCBCR_ENC_DEFAULT; >> >> - /* TOFIX: Default to RGB888 output format */ >> - hdmi->hdmi_data.enc_out_bus_format = MEDIA_BUS_FMT_RGB888_1X24; >> + if (hdmi->hdmi_data.enc_out_bus_format == MEDIA_BUS_FMT_FIXED) >> + hdmi->hdmi_data.enc_out_bus_format = MEDIA_BUS_FMT_RGB888_1X24; >> >> hdmi->hdmi_data.pix_repet_factor = 0; >> hdmi->hdmi_data.hdcp_enable = 0; >> @@ -2388,6 +2387,267 @@ static const struct drm_connector_helper_funcs dw_hdmi_connector_helper_funcs = >> .atomic_check = dw_hdmi_connector_atomic_check, >> }; >> >> +/* >> + * Possible output formats : >> + * - MEDIA_BUS_FMT_UYYVYY16_0_5X48, >> + * - MEDIA_BUS_FMT_UYYVYY12_0_5X36, >> + * - MEDIA_BUS_FMT_UYYVYY10_0_5X30, >> + * - MEDIA_BUS_FMT_UYYVYY8_0_5X24, >> + * - MEDIA_BUS_FMT_YUV16_1X48, >> + * - MEDIA_BUS_FMT_RGB161616_1X48, >> + * - MEDIA_BUS_FMT_UYVY12_1X24, >> + * - MEDIA_BUS_FMT_YUV12_1X36, >> + * - MEDIA_BUS_FMT_RGB121212_1X36, >> + * - MEDIA_BUS_FMT_UYVY10_1X20, >> + * - MEDIA_BUS_FMT_YUV10_1X30, >> + * - MEDIA_BUS_FMT_RGB101010_1X30, >> + * - MEDIA_BUS_FMT_UYVY8_1X16, >> + * - MEDIA_BUS_FMT_YUV8_1X24, >> + * - MEDIA_BUS_FMT_RGB888_1X24, >> + */ > > I'd drop this comment as I don't think it brings much, except for a risk > of getting out of sync with the implementation below :-) I know it's redundant, but I'll prefer having it here to clarify which bus formats are handled. > >> + >> +/* Can return a maximum of 12 possible output formats for a mode/connector */ >> +#define MAX_OUTPUT_SEL_FORMATS 12 > > I count 11 below. Exact ! > >> + >> +static u32 *dw_hdmi_bridge_atomic_get_output_bus_fmts(struct drm_bridge *bridge, >> + struct drm_bridge_state *bridge_state, >> + struct drm_crtc_state *crtc_state, >> + struct drm_connector_state *conn_state, >> + unsigned int *num_output_fmts) >> +{ >> + struct drm_connector *conn = conn_state->connector; >> + struct drm_display_info *info = &conn->display_info; >> + struct drm_display_mode *mode = &crtc_state->mode; >> + u8 max_bpc = conn_state->max_requested_bpc; >> + bool is_hdmi2_sink = info->hdmi.scdc.supported || >> + (info->color_formats & DRM_COLOR_FORMAT_YCRCB420); >> + u32 *output_fmts; >> + int i = 0; > > i is never negative, you can make it an unsigned int. ok > >> + >> + *num_output_fmts = 0; >> + >> + output_fmts = kcalloc(MAX_OUTPUT_SEL_FORMATS, sizeof(*output_fmts), >> + GFP_KERNEL); >> + if (!output_fmts) >> + return NULL; >> + >> + /* >> + * If the current mode enforces 4:2:0, force the output but format >> + * to 4:2:0 and do not add the YUV422/444/RGB formats >> + */ >> + if (conn->ycbcr_420_allowed && >> + (drm_mode_is_420_only(info, mode) || >> + (is_hdmi2_sink && drm_mode_is_420_also(info, mode)))) { >> + >> + /* Order bus formats from 16bit to 8bit if supported */ >> + if (max_bpc >= 16 && info->bpc == 16 && >> + (info->hdmi.y420_dc_modes & DRM_EDID_YCBCR420_DC_48)) >> + output_fmts[i++] = MEDIA_BUS_FMT_UYYVYY16_0_5X48; >> + >> + if (max_bpc >= 12 && info->bpc >= 12 && >> + (info->hdmi.y420_dc_modes & DRM_EDID_YCBCR420_DC_36)) >> + output_fmts[i++] = MEDIA_BUS_FMT_UYYVYY12_0_5X36; >> + >> + if (max_bpc >= 10 && info->bpc >= 10 && >> + (info->hdmi.y420_dc_modes & DRM_EDID_YCBCR420_DC_30)) >> + output_fmts[i++] = MEDIA_BUS_FMT_UYYVYY10_0_5X30; >> + >> + /* Default 8bit fallback */ >> + output_fmts[i++] = MEDIA_BUS_FMT_UYYVYY8_0_5X24; >> + >> + *num_output_fmts = i; >> + >> + return output_fmts; >> + } >> + >> + /* >> + * Order bus formats from 16bit to 8bit and from YUV422 to RGB >> + * if supported. In any case the default RGB888 format is added >> + */ >> + >> + if (max_bpc >= 16 && info->bpc == 16) { >> + if (info->color_formats & DRM_COLOR_FORMAT_YCRCB444) >> + output_fmts[i++] = MEDIA_BUS_FMT_YUV16_1X48; >> + >> + output_fmts[i++] = MEDIA_BUS_FMT_RGB161616_1X48; >> + } >> + >> + if (max_bpc >= 12 && info->bpc >= 12) { >> + if (info->color_formats & DRM_COLOR_FORMAT_YCRCB422) >> + output_fmts[i++] = MEDIA_BUS_FMT_UYVY12_1X24; >> + >> + if (info->color_formats & DRM_COLOR_FORMAT_YCRCB444) >> + output_fmts[i++] = MEDIA_BUS_FMT_YUV12_1X36; >> + >> + output_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36; >> + } >> + >> + if (max_bpc >= 10 && info->bpc >= 10) { >> + if (info->color_formats & DRM_COLOR_FORMAT_YCRCB422) >> + output_fmts[i++] = MEDIA_BUS_FMT_UYVY10_1X20; >> + >> + if (info->color_formats & DRM_COLOR_FORMAT_YCRCB444) >> + output_fmts[i++] = MEDIA_BUS_FMT_YUV10_1X30; >> + >> + output_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30; >> + } >> + >> + if (info->color_formats & DRM_COLOR_FORMAT_YCRCB422) >> + output_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16; >> + >> + if (info->color_formats & DRM_COLOR_FORMAT_YCRCB444) >> + output_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24; >> + >> + /* Default 8bit RGB fallback */ >> + output_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24; >> + >> + *num_output_fmts = i; >> + >> + return output_fmts; >> +} >> + >> +/* >> + * Possible input formats : >> + * - MEDIA_BUS_FMT_RGB888_1X24 >> + * - MEDIA_BUS_FMT_YUV8_1X24 >> + * - MEDIA_BUS_FMT_UYVY8_1X16 >> + * - MEDIA_BUS_FMT_UYYVYY8_0_5X24 >> + * - MEDIA_BUS_FMT_RGB101010_1X30 >> + * - MEDIA_BUS_FMT_YUV10_1X30 >> + * - MEDIA_BUS_FMT_UYVY10_1X20 >> + * - MEDIA_BUS_FMT_UYYVYY10_0_5X30 >> + * - MEDIA_BUS_FMT_RGB121212_1X36 >> + * - MEDIA_BUS_FMT_YUV12_1X36 >> + * - MEDIA_BUS_FMT_UYVY12_1X24 >> + * - MEDIA_BUS_FMT_UYYVYY12_0_5X36 >> + * - MEDIA_BUS_FMT_RGB161616_1X48 >> + * - MEDIA_BUS_FMT_YUV16_1X48 >> + * - MEDIA_BUS_FMT_UYYVYY16_0_5X48 >> + */ > > Same here. I'll prefer keeping these aswell > >> + >> +/* Can return a maximum of 4 possible input formats for an output format */ >> +#define MAX_INPUT_SEL_FORMATS 4 > > As Boris pointed out, that should be 3. Yep > >> + >> +static u32 *dw_hdmi_bridge_atomic_get_input_bus_fmts(struct drm_bridge *bridge, >> + struct drm_bridge_state *bridge_state, >> + struct drm_crtc_state *crtc_state, >> + struct drm_connector_state *conn_state, >> + u32 output_fmt, >> + unsigned int *num_input_fmts) >> +{ >> + u32 *input_fmts; >> + int i = 0; > > i is never negative, you can make it an unsigned int. ok > >> + >> + *num_input_fmts = 0; >> + >> + input_fmts = kcalloc(MAX_INPUT_SEL_FORMATS, sizeof(*input_fmts), >> + GFP_KERNEL); >> + if (!input_fmts) >> + return NULL; >> + >> + switch (output_fmt) { >> + /* 8bit */ >> + case MEDIA_BUS_FMT_RGB888_1X24: >> + input_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24; >> + input_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24; >> + input_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16; >> + break; >> + case MEDIA_BUS_FMT_YUV8_1X24: >> + input_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24; >> + input_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16; >> + input_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24; >> + break; >> + case MEDIA_BUS_FMT_UYVY8_1X16: >> + input_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16; >> + input_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24; >> + input_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24; >> + break; >> + >> + /* 10bit */ >> + case MEDIA_BUS_FMT_RGB101010_1X30: >> + input_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30; >> + input_fmts[i++] = MEDIA_BUS_FMT_YUV10_1X30; >> + input_fmts[i++] = MEDIA_BUS_FMT_UYVY10_1X20; >> + break; >> + case MEDIA_BUS_FMT_YUV10_1X30: >> + input_fmts[i++] = MEDIA_BUS_FMT_YUV10_1X30; >> + input_fmts[i++] = MEDIA_BUS_FMT_UYVY10_1X20; >> + input_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30; >> + break; >> + case MEDIA_BUS_FMT_UYVY10_1X20: >> + input_fmts[i++] = MEDIA_BUS_FMT_UYVY10_1X20; >> + input_fmts[i++] = MEDIA_BUS_FMT_YUV10_1X30; >> + input_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30; >> + break; >> + >> + /* 12bit */ >> + case MEDIA_BUS_FMT_RGB121212_1X36: >> + input_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36; >> + input_fmts[i++] = MEDIA_BUS_FMT_YUV12_1X36; >> + input_fmts[i++] = MEDIA_BUS_FMT_UYVY12_1X24; >> + break; >> + case MEDIA_BUS_FMT_YUV12_1X36: >> + input_fmts[i++] = MEDIA_BUS_FMT_YUV12_1X36; >> + input_fmts[i++] = MEDIA_BUS_FMT_UYVY12_1X24; >> + input_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36; >> + break; >> + case MEDIA_BUS_FMT_UYVY12_1X24: >> + input_fmts[i++] = MEDIA_BUS_FMT_UYVY12_1X24; >> + input_fmts[i++] = MEDIA_BUS_FMT_YUV12_1X36; >> + input_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36; >> + break; >> + >> + /* 16bit */ >> + case MEDIA_BUS_FMT_RGB161616_1X48: >> + input_fmts[i++] = MEDIA_BUS_FMT_RGB161616_1X48; >> + input_fmts[i++] = MEDIA_BUS_FMT_YUV16_1X48; >> + break; >> + case MEDIA_BUS_FMT_YUV16_1X48: >> + input_fmts[i++] = MEDIA_BUS_FMT_YUV16_1X48; >> + input_fmts[i++] = MEDIA_BUS_FMT_RGB161616_1X48; >> + break; >> + >> + /* 420 */ > > s/420/YUV 4:2:0/ ? Looks better > >> + case MEDIA_BUS_FMT_UYYVYY8_0_5X24: >> + case MEDIA_BUS_FMT_UYYVYY10_0_5X30: >> + case MEDIA_BUS_FMT_UYYVYY12_0_5X36: >> + case MEDIA_BUS_FMT_UYYVYY16_0_5X48: >> + input_fmts[i++] = output_fmt; >> + break; >> + } >> + >> + *num_input_fmts = i; >> + >> + if (*num_input_fmts == 0) { >> + kfree(input_fmts); >> + input_fmts = NULL; >> + } >> + >> + return input_fmts; >> +} >> + >> +static int dw_hdmi_bridge_atomic_check(struct drm_bridge *bridge, >> + struct drm_bridge_state *bridge_state, >> + struct drm_crtc_state *crtc_state, >> + struct drm_connector_state *conn_state) >> +{ >> + struct dw_hdmi *hdmi = bridge->driver_private; >> + >> + dev_dbg(hdmi->dev, "selected output format %x\n", > > s/%x/0x%04x/ > >> + bridge_state->output_bus_cfg.format); > > Misalignment ? Fixed > >> + >> + hdmi->hdmi_data.enc_out_bus_format = >> + bridge_state->output_bus_cfg.format; >> + >> + dev_dbg(hdmi->dev, "selected input format %x\n", >> + bridge_state->input_bus_cfg.format); > > Same here. I would combine both messages: > > dev_dbg(hdmi->dev, "input format: 0x%04x, output format: 0x04x\n", > bridge_state->input_bus_cfg.format, > bridge_state->output_bus_cfg.format); Looks better > >> + >> + hdmi->hdmi_data.enc_in_bus_format = >> + bridge_state->input_bus_cfg.format; >> + >> + return 0; >> +} >> + >> static int dw_hdmi_bridge_attach(struct drm_bridge *bridge) >> { >> struct dw_hdmi *hdmi = bridge->driver_private; >> @@ -2499,6 +2759,9 @@ static const struct drm_bridge_funcs dw_hdmi_bridge_funcs = { >> .atomic_reset = drm_atomic_helper_bridge_reset, >> .attach = dw_hdmi_bridge_attach, >> .detach = dw_hdmi_bridge_detach, >> + .atomic_check = dw_hdmi_bridge_atomic_check, >> + .atomic_get_output_bus_fmts = dw_hdmi_bridge_atomic_get_output_bus_fmts, >> + .atomic_get_input_bus_fmts = dw_hdmi_bridge_atomic_get_input_bus_fmts, >> .enable = dw_hdmi_bridge_enable, >> .disable = dw_hdmi_bridge_disable, >> .mode_set = dw_hdmi_bridge_mode_set, >> @@ -2963,6 +3226,7 @@ __dw_hdmi_probe(struct platform_device *pdev, >> >> hdmi->bridge.driver_private = hdmi; >> hdmi->bridge.funcs = &dw_hdmi_bridge_funcs; >> + > > This seems unrelated. Yep > >> #ifdef CONFIG_OF >> hdmi->bridge.of_node = pdev->dev.of_node; >> #endif > Thanks, Neil
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index fec4a4bcd1fe..15048ad694bc 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c @@ -2095,11 +2095,10 @@ 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; - /* TOFIX: Get input format from plat data or fallback to RGB888 */ if (hdmi->plat_data->input_bus_format) hdmi->hdmi_data.enc_in_bus_format = hdmi->plat_data->input_bus_format; - else + else if (hdmi->hdmi_data.enc_in_bus_format == MEDIA_BUS_FMT_FIXED) hdmi->hdmi_data.enc_in_bus_format = MEDIA_BUS_FMT_RGB888_1X24; /* TOFIX: Get input encoding from plat data or fallback to none */ @@ -2109,8 +2108,8 @@ static int dw_hdmi_setup(struct dw_hdmi *hdmi, struct drm_display_mode *mode) else hdmi->hdmi_data.enc_in_encoding = V4L2_YCBCR_ENC_DEFAULT; - /* TOFIX: Default to RGB888 output format */ - hdmi->hdmi_data.enc_out_bus_format = MEDIA_BUS_FMT_RGB888_1X24; + if (hdmi->hdmi_data.enc_out_bus_format == MEDIA_BUS_FMT_FIXED) + hdmi->hdmi_data.enc_out_bus_format = MEDIA_BUS_FMT_RGB888_1X24; hdmi->hdmi_data.pix_repet_factor = 0; hdmi->hdmi_data.hdcp_enable = 0; @@ -2388,6 +2387,267 @@ static const struct drm_connector_helper_funcs dw_hdmi_connector_helper_funcs = .atomic_check = dw_hdmi_connector_atomic_check, }; +/* + * Possible output formats : + * - MEDIA_BUS_FMT_UYYVYY16_0_5X48, + * - MEDIA_BUS_FMT_UYYVYY12_0_5X36, + * - MEDIA_BUS_FMT_UYYVYY10_0_5X30, + * - MEDIA_BUS_FMT_UYYVYY8_0_5X24, + * - MEDIA_BUS_FMT_YUV16_1X48, + * - MEDIA_BUS_FMT_RGB161616_1X48, + * - MEDIA_BUS_FMT_UYVY12_1X24, + * - MEDIA_BUS_FMT_YUV12_1X36, + * - MEDIA_BUS_FMT_RGB121212_1X36, + * - MEDIA_BUS_FMT_UYVY10_1X20, + * - MEDIA_BUS_FMT_YUV10_1X30, + * - MEDIA_BUS_FMT_RGB101010_1X30, + * - MEDIA_BUS_FMT_UYVY8_1X16, + * - MEDIA_BUS_FMT_YUV8_1X24, + * - MEDIA_BUS_FMT_RGB888_1X24, + */ + +/* Can return a maximum of 12 possible output formats for a mode/connector */ +#define MAX_OUTPUT_SEL_FORMATS 12 + +static u32 *dw_hdmi_bridge_atomic_get_output_bus_fmts(struct drm_bridge *bridge, + struct drm_bridge_state *bridge_state, + struct drm_crtc_state *crtc_state, + struct drm_connector_state *conn_state, + unsigned int *num_output_fmts) +{ + struct drm_connector *conn = conn_state->connector; + struct drm_display_info *info = &conn->display_info; + struct drm_display_mode *mode = &crtc_state->mode; + u8 max_bpc = conn_state->max_requested_bpc; + bool is_hdmi2_sink = info->hdmi.scdc.supported || + (info->color_formats & DRM_COLOR_FORMAT_YCRCB420); + u32 *output_fmts; + int i = 0; + + *num_output_fmts = 0; + + output_fmts = kcalloc(MAX_OUTPUT_SEL_FORMATS, sizeof(*output_fmts), + GFP_KERNEL); + if (!output_fmts) + return NULL; + + /* + * If the current mode enforces 4:2:0, force the output but format + * to 4:2:0 and do not add the YUV422/444/RGB formats + */ + if (conn->ycbcr_420_allowed && + (drm_mode_is_420_only(info, mode) || + (is_hdmi2_sink && drm_mode_is_420_also(info, mode)))) { + + /* Order bus formats from 16bit to 8bit if supported */ + if (max_bpc >= 16 && info->bpc == 16 && + (info->hdmi.y420_dc_modes & DRM_EDID_YCBCR420_DC_48)) + output_fmts[i++] = MEDIA_BUS_FMT_UYYVYY16_0_5X48; + + if (max_bpc >= 12 && info->bpc >= 12 && + (info->hdmi.y420_dc_modes & DRM_EDID_YCBCR420_DC_36)) + output_fmts[i++] = MEDIA_BUS_FMT_UYYVYY12_0_5X36; + + if (max_bpc >= 10 && info->bpc >= 10 && + (info->hdmi.y420_dc_modes & DRM_EDID_YCBCR420_DC_30)) + output_fmts[i++] = MEDIA_BUS_FMT_UYYVYY10_0_5X30; + + /* Default 8bit fallback */ + output_fmts[i++] = MEDIA_BUS_FMT_UYYVYY8_0_5X24; + + *num_output_fmts = i; + + return output_fmts; + } + + /* + * Order bus formats from 16bit to 8bit and from YUV422 to RGB + * if supported. In any case the default RGB888 format is added + */ + + if (max_bpc >= 16 && info->bpc == 16) { + if (info->color_formats & DRM_COLOR_FORMAT_YCRCB444) + output_fmts[i++] = MEDIA_BUS_FMT_YUV16_1X48; + + output_fmts[i++] = MEDIA_BUS_FMT_RGB161616_1X48; + } + + if (max_bpc >= 12 && info->bpc >= 12) { + if (info->color_formats & DRM_COLOR_FORMAT_YCRCB422) + output_fmts[i++] = MEDIA_BUS_FMT_UYVY12_1X24; + + if (info->color_formats & DRM_COLOR_FORMAT_YCRCB444) + output_fmts[i++] = MEDIA_BUS_FMT_YUV12_1X36; + + output_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36; + } + + if (max_bpc >= 10 && info->bpc >= 10) { + if (info->color_formats & DRM_COLOR_FORMAT_YCRCB422) + output_fmts[i++] = MEDIA_BUS_FMT_UYVY10_1X20; + + if (info->color_formats & DRM_COLOR_FORMAT_YCRCB444) + output_fmts[i++] = MEDIA_BUS_FMT_YUV10_1X30; + + output_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30; + } + + if (info->color_formats & DRM_COLOR_FORMAT_YCRCB422) + output_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16; + + if (info->color_formats & DRM_COLOR_FORMAT_YCRCB444) + output_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24; + + /* Default 8bit RGB fallback */ + output_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24; + + *num_output_fmts = i; + + return output_fmts; +} + +/* + * Possible input formats : + * - MEDIA_BUS_FMT_RGB888_1X24 + * - MEDIA_BUS_FMT_YUV8_1X24 + * - MEDIA_BUS_FMT_UYVY8_1X16 + * - MEDIA_BUS_FMT_UYYVYY8_0_5X24 + * - MEDIA_BUS_FMT_RGB101010_1X30 + * - MEDIA_BUS_FMT_YUV10_1X30 + * - MEDIA_BUS_FMT_UYVY10_1X20 + * - MEDIA_BUS_FMT_UYYVYY10_0_5X30 + * - MEDIA_BUS_FMT_RGB121212_1X36 + * - MEDIA_BUS_FMT_YUV12_1X36 + * - MEDIA_BUS_FMT_UYVY12_1X24 + * - MEDIA_BUS_FMT_UYYVYY12_0_5X36 + * - MEDIA_BUS_FMT_RGB161616_1X48 + * - MEDIA_BUS_FMT_YUV16_1X48 + * - MEDIA_BUS_FMT_UYYVYY16_0_5X48 + */ + +/* Can return a maximum of 4 possible input formats for an output format */ +#define MAX_INPUT_SEL_FORMATS 4 + +static u32 *dw_hdmi_bridge_atomic_get_input_bus_fmts(struct drm_bridge *bridge, + struct drm_bridge_state *bridge_state, + struct drm_crtc_state *crtc_state, + struct drm_connector_state *conn_state, + u32 output_fmt, + unsigned int *num_input_fmts) +{ + u32 *input_fmts; + int i = 0; + + *num_input_fmts = 0; + + input_fmts = kcalloc(MAX_INPUT_SEL_FORMATS, sizeof(*input_fmts), + GFP_KERNEL); + if (!input_fmts) + return NULL; + + switch (output_fmt) { + /* 8bit */ + case MEDIA_BUS_FMT_RGB888_1X24: + input_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24; + input_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24; + input_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16; + break; + case MEDIA_BUS_FMT_YUV8_1X24: + input_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24; + input_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16; + input_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24; + break; + case MEDIA_BUS_FMT_UYVY8_1X16: + input_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16; + input_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24; + input_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24; + break; + + /* 10bit */ + case MEDIA_BUS_FMT_RGB101010_1X30: + input_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30; + input_fmts[i++] = MEDIA_BUS_FMT_YUV10_1X30; + input_fmts[i++] = MEDIA_BUS_FMT_UYVY10_1X20; + break; + case MEDIA_BUS_FMT_YUV10_1X30: + input_fmts[i++] = MEDIA_BUS_FMT_YUV10_1X30; + input_fmts[i++] = MEDIA_BUS_FMT_UYVY10_1X20; + input_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30; + break; + case MEDIA_BUS_FMT_UYVY10_1X20: + input_fmts[i++] = MEDIA_BUS_FMT_UYVY10_1X20; + input_fmts[i++] = MEDIA_BUS_FMT_YUV10_1X30; + input_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30; + break; + + /* 12bit */ + case MEDIA_BUS_FMT_RGB121212_1X36: + input_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36; + input_fmts[i++] = MEDIA_BUS_FMT_YUV12_1X36; + input_fmts[i++] = MEDIA_BUS_FMT_UYVY12_1X24; + break; + case MEDIA_BUS_FMT_YUV12_1X36: + input_fmts[i++] = MEDIA_BUS_FMT_YUV12_1X36; + input_fmts[i++] = MEDIA_BUS_FMT_UYVY12_1X24; + input_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36; + break; + case MEDIA_BUS_FMT_UYVY12_1X24: + input_fmts[i++] = MEDIA_BUS_FMT_UYVY12_1X24; + input_fmts[i++] = MEDIA_BUS_FMT_YUV12_1X36; + input_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36; + break; + + /* 16bit */ + case MEDIA_BUS_FMT_RGB161616_1X48: + input_fmts[i++] = MEDIA_BUS_FMT_RGB161616_1X48; + input_fmts[i++] = MEDIA_BUS_FMT_YUV16_1X48; + break; + case MEDIA_BUS_FMT_YUV16_1X48: + input_fmts[i++] = MEDIA_BUS_FMT_YUV16_1X48; + input_fmts[i++] = MEDIA_BUS_FMT_RGB161616_1X48; + break; + + /* 420 */ + case MEDIA_BUS_FMT_UYYVYY8_0_5X24: + case MEDIA_BUS_FMT_UYYVYY10_0_5X30: + case MEDIA_BUS_FMT_UYYVYY12_0_5X36: + case MEDIA_BUS_FMT_UYYVYY16_0_5X48: + input_fmts[i++] = output_fmt; + break; + } + + *num_input_fmts = i; + + if (*num_input_fmts == 0) { + kfree(input_fmts); + input_fmts = NULL; + } + + return input_fmts; +} + +static int dw_hdmi_bridge_atomic_check(struct drm_bridge *bridge, + struct drm_bridge_state *bridge_state, + struct drm_crtc_state *crtc_state, + struct drm_connector_state *conn_state) +{ + struct dw_hdmi *hdmi = bridge->driver_private; + + dev_dbg(hdmi->dev, "selected output format %x\n", + bridge_state->output_bus_cfg.format); + + hdmi->hdmi_data.enc_out_bus_format = + bridge_state->output_bus_cfg.format; + + dev_dbg(hdmi->dev, "selected input format %x\n", + bridge_state->input_bus_cfg.format); + + hdmi->hdmi_data.enc_in_bus_format = + bridge_state->input_bus_cfg.format; + + return 0; +} + static int dw_hdmi_bridge_attach(struct drm_bridge *bridge) { struct dw_hdmi *hdmi = bridge->driver_private; @@ -2499,6 +2759,9 @@ static const struct drm_bridge_funcs dw_hdmi_bridge_funcs = { .atomic_reset = drm_atomic_helper_bridge_reset, .attach = dw_hdmi_bridge_attach, .detach = dw_hdmi_bridge_detach, + .atomic_check = dw_hdmi_bridge_atomic_check, + .atomic_get_output_bus_fmts = dw_hdmi_bridge_atomic_get_output_bus_fmts, + .atomic_get_input_bus_fmts = dw_hdmi_bridge_atomic_get_input_bus_fmts, .enable = dw_hdmi_bridge_enable, .disable = dw_hdmi_bridge_disable, .mode_set = dw_hdmi_bridge_mode_set, @@ -2963,6 +3226,7 @@ __dw_hdmi_probe(struct platform_device *pdev, hdmi->bridge.driver_private = hdmi; hdmi->bridge.funcs = &dw_hdmi_bridge_funcs; + #ifdef CONFIG_OF hdmi->bridge.of_node = pdev->dev.of_node; #endif
Add the atomic_get_output_bus_fmts, atomic_get_input_bus_fmts to negociate the possible output and input formats for the current mode and monitor, and use the negotiated formats in a basic atomic_check callback. Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> --- drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 272 +++++++++++++++++++++- 1 file changed, 268 insertions(+), 4 deletions(-)