| Message ID | 20190826152649.13820-18-boris.brezillon@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: Add support for bus-format negotiation | expand |
Hi Boris, On Mon, 2019-08-26 at 17:26 +0200, Boris Brezillon wrote: > Now that bridges can expose the bus format/flags they expect, we can > use those instead of the relying on the display_info provided by the > connector (which is only valid if the encoder is directly connected > to bridge element driving the panel/display). > > We also explicitly expose the bus formats supported by our encoder by > filling encoder->output_bus_caps with proper info. > > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com> > --- > Changes in v2: > * Adjust things to match the new bus-format negotiation infra > --- > drivers/gpu/drm/imx/parallel-display.c | 136 ++++++++++++++++++++++--- > 1 file changed, 120 insertions(+), 16 deletions(-) > > diff --git a/drivers/gpu/drm/imx/parallel-display.c b/drivers/gpu/drm/imx/parallel-display.c > index 35518e5de356..df47cb74759f 100644 > --- a/drivers/gpu/drm/imx/parallel-display.c > +++ b/drivers/gpu/drm/imx/parallel-display.c > @@ -89,37 +89,139 @@ static struct drm_encoder *imx_pd_connector_best_encoder( > return &imxpd->encoder; > } > > -static void imx_pd_encoder_enable(struct drm_encoder *encoder) > +static void imx_pd_bridge_enable(struct drm_bridge *bridge) > { > + struct drm_encoder *encoder = bridge_to_encoder(bridge); > struct imx_parallel_display *imxpd = enc_to_imxpd(encoder); > > drm_panel_prepare(imxpd->panel); > drm_panel_enable(imxpd->panel); > } > > -static void imx_pd_encoder_disable(struct drm_encoder *encoder) > +static void imx_pd_bridge_disable(struct drm_bridge *bridge) > { > + struct drm_encoder *encoder = bridge_to_encoder(bridge); > struct imx_parallel_display *imxpd = enc_to_imxpd(encoder); > > drm_panel_disable(imxpd->panel); > drm_panel_unprepare(imxpd->panel); > } > > -static int imx_pd_encoder_atomic_check(struct drm_encoder *encoder, > - struct drm_crtc_state *crtc_state, > - struct drm_connector_state *conn_state) > +static const u32 imx_pd_bus_fmts[] = { > + MEDIA_BUS_FMT_RGB888_1X24, > + MEDIA_BUS_FMT_RGB565_1X16, > + MEDIA_BUS_FMT_RGB666_1X18, > + MEDIA_BUS_FMT_RGB666_1X24_CPADHI, ipu-dc.c also supports MEDIA_BUS_FMT_BGR888_1X24. > +}; > + > +static void > +imx_pd_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, > + u32 *output_fmts) > { > + struct drm_display_info *di = &conn_state->connector->display_info; > + struct drm_encoder *encoder = bridge_to_encoder(bridge); > + struct imx_parallel_display *imxpd = enc_to_imxpd(encoder); > + > + *num_output_fmts = 1; > + if (imxpd->bus_format) { > + if (output_fmts) > + output_fmts[0] = imxpd->bus_format; This is the legacy DT configured interface-pix-fmt. Maybe this should be moved to the last place. > + } else if (di->num_bus_formats) { > + *num_output_fmts = 1; num_output_fmts is already set to one above. > + if (output_fmts) > + output_fmts[0] = di->bus_formats[0]; > + } else { > + *num_output_fmts = ARRAY_SIZE(imx_pd_bus_fmts); > + if (output_fmts) > + memcpy(output_fmts, imx_pd_bus_fmts, > + ARRAY_SIZE(imx_pd_bus_fmts)); > + } > +} > + > +static void > +imx_pd_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) > +{ > + struct drm_encoder *encoder = bridge_to_encoder(bridge); > + struct imx_parallel_display *imxpd = enc_to_imxpd(encoder); > + > + *num_input_fmts = 0; > + if (output_fmt == MEDIA_BUS_FMT_FIXED) { > + *num_input_fmts = 1; I don't understand this. The bus format stored in imx_crtc_state by atomic_check is used to configure the DC later. This should never be MEDIA_BUS_FMT_FIXED, that would trigger a WARN_ON in ipu_bus_format_to_map(). > + } else if (!imxpd->bus_format) { > + unsigned int i; > + > + for (i = 0; i < ARRAY_SIZE(imx_pd_bus_fmts); i++) { > + if (imx_pd_bus_fmts[i] == output_fmt) { > + *num_input_fmts = 1; > + break; > + } > + } > + } else if (imxpd->bus_format == output_fmt) { > + *num_input_fmts = 1; > + } > + > + if (*num_input_fmts && input_fmts) > + input_fmts[0] = MEDIA_BUS_FMT_FIXED; The parallel-display driver basically represents the wiring, pinmux, and drivers between the IPU DI and the DISP pads. The input format should always be identical to the output format. > +} > + > +static int imx_pd_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 drm_encoder *encoder = bridge_to_encoder(bridge); > struct imx_crtc_state *imx_crtc_state = to_imx_crtc_state(crtc_state); > struct drm_display_info *di = &conn_state->connector->display_info; > struct imx_parallel_display *imxpd = enc_to_imxpd(encoder); > + struct drm_bridge_state *next_bridge_state = NULL; > + struct drm_bridge *next_bridge; > + u32 bus_flags, bus_fmt; > + unsigned int i; > > - if (!imxpd->bus_format && di->num_bus_formats) { > - imx_crtc_state->bus_flags = di->bus_flags; > - imx_crtc_state->bus_format = di->bus_formats[0]; > - } else { > - imx_crtc_state->bus_flags = imxpd->bus_flags; > - imx_crtc_state->bus_format = imxpd->bus_format; > + next_bridge = drm_bridge_chain_get_next_bridge(bridge); > + if (next_bridge) > + next_bridge_state = drm_atomic_get_new_bridge_state(crtc_state->state, > + next_bridge); > + > + bus_fmt = bridge_state->output_bus_cfg.fmt; > + if (bus_fmt == MEDIA_BUS_FMT_FIXED) > + bus_fmt = 0; I would expect this to return with -EINVAL if bridge_state- >output_bus_cfg.fmt is not in the list of supported formats. > + > + if (next_bridge_state) > + bus_flags = next_bridge_state->input_bus_cfg.flags; > + else if (!imxpd->bus_format && di->num_bus_formats) > + bus_flags = di->bus_flags; > + else > + bus_flags = imxpd->bus_flags; > + > + for (i = 0; bus_fmt && i < ARRAY_SIZE(imx_pd_bus_fmts); i++) { > + if (imx_pd_bus_fmts[i] == bridge_state->output_bus_cfg.fmt) > + break; > } > + > + if (i == ARRAY_SIZE(imx_pd_bus_fmts)) > + return -EINVAL; > + > + if (bus_flags & > + ~(DRM_BUS_FLAG_DE_LOW | DRM_BUS_FLAG_DE_HIGH | > + DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE | > + DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE)) > + return -EINVAL; This could allow DRM_BUS_FLAG_SYNC_DRIVE_POSEDGE/NEGEDGE as well, if they are not opposite to PIXDATA_DRIVE. > + bridge_state->output_bus_cfg.flags = bus_flags; > + bridge_state->input_bus_cfg.flags = bus_flags; > + imx_crtc_state->bus_flags = bus_flags; > + imx_crtc_state->bus_format = bridge_state->output_bus_cfg.fmt; This should be the input bus format. > imx_crtc_state->di_hsync_pin = 2; > imx_crtc_state->di_vsync_pin = 3; > > @@ -143,10 +245,12 @@ static const struct drm_encoder_funcs imx_pd_encoder_funcs = { > .destroy = imx_drm_encoder_destroy, > }; > > -static const struct drm_encoder_helper_funcs imx_pd_encoder_helper_funcs = { > - .enable = imx_pd_encoder_enable, > - .disable = imx_pd_encoder_disable, > - .atomic_check = imx_pd_encoder_atomic_check, > +static const struct drm_bridge_funcs imx_pd_bridge_funcs = { > + .enable = imx_pd_bridge_enable, > + .disable = imx_pd_bridge_disable, > + .atomic_check = imx_pd_bridge_atomic_check, > + .atomic_get_input_bus_fmts = imx_pd_bridge_atomic_get_input_bus_fmts, > + .atomic_get_output_bus_fmts = imx_pd_bridge_atomic_get_output_bus_fmts, > }; > > static int imx_pd_register(struct drm_device *drm, > @@ -166,7 +270,7 @@ static int imx_pd_register(struct drm_device *drm, > */ > imxpd->connector.dpms = DRM_MODE_DPMS_OFF; > > - drm_encoder_helper_add(encoder, &imx_pd_encoder_helper_funcs); > + encoder->bridge.funcs = &imx_pd_bridge_funcs; > drm_encoder_init(drm, encoder, &imx_pd_encoder_funcs, > DRM_MODE_ENCODER_NONE, NULL); regards Philipp
On Tue, 27 Aug 2019 10:15:19 +0200 Philipp Zabel <p.zabel@pengutronix.de> wrote: > Hi Boris, > > On Mon, 2019-08-26 at 17:26 +0200, Boris Brezillon wrote: > > Now that bridges can expose the bus format/flags they expect, we can > > use those instead of the relying on the display_info provided by the > > connector (which is only valid if the encoder is directly connected > > to bridge element driving the panel/display). > > > > We also explicitly expose the bus formats supported by our encoder by > > filling encoder->output_bus_caps with proper info. > > > > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com> > > --- > > Changes in v2: > > * Adjust things to match the new bus-format negotiation infra > > --- > > drivers/gpu/drm/imx/parallel-display.c | 136 ++++++++++++++++++++++--- > > 1 file changed, 120 insertions(+), 16 deletions(-) > > > > diff --git a/drivers/gpu/drm/imx/parallel-display.c b/drivers/gpu/drm/imx/parallel-display.c > > index 35518e5de356..df47cb74759f 100644 > > --- a/drivers/gpu/drm/imx/parallel-display.c > > +++ b/drivers/gpu/drm/imx/parallel-display.c > > @@ -89,37 +89,139 @@ static struct drm_encoder *imx_pd_connector_best_encoder( > > return &imxpd->encoder; > > } > > > > -static void imx_pd_encoder_enable(struct drm_encoder *encoder) > > +static void imx_pd_bridge_enable(struct drm_bridge *bridge) > > { > > + struct drm_encoder *encoder = bridge_to_encoder(bridge); > > struct imx_parallel_display *imxpd = enc_to_imxpd(encoder); > > > > drm_panel_prepare(imxpd->panel); > > drm_panel_enable(imxpd->panel); > > } > > > > -static void imx_pd_encoder_disable(struct drm_encoder *encoder) > > +static void imx_pd_bridge_disable(struct drm_bridge *bridge) > > { > > + struct drm_encoder *encoder = bridge_to_encoder(bridge); > > struct imx_parallel_display *imxpd = enc_to_imxpd(encoder); > > > > drm_panel_disable(imxpd->panel); > > drm_panel_unprepare(imxpd->panel); > > } > > > > -static int imx_pd_encoder_atomic_check(struct drm_encoder *encoder, > > - struct drm_crtc_state *crtc_state, > > - struct drm_connector_state *conn_state) > > +static const u32 imx_pd_bus_fmts[] = { > > + MEDIA_BUS_FMT_RGB888_1X24, > > + MEDIA_BUS_FMT_RGB565_1X16, > > + MEDIA_BUS_FMT_RGB666_1X18, > > + MEDIA_BUS_FMT_RGB666_1X24_CPADHI, > > ipu-dc.c also supports MEDIA_BUS_FMT_BGR888_1X24. Will add that one to the list. > > > +}; > > + > > +static void > > +imx_pd_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, > > + u32 *output_fmts) > > { > > + struct drm_display_info *di = &conn_state->connector->display_info; > > + struct drm_encoder *encoder = bridge_to_encoder(bridge); > > + struct imx_parallel_display *imxpd = enc_to_imxpd(encoder); > > + > > + *num_output_fmts = 1; > > + if (imxpd->bus_format) { > > + if (output_fmts) > > + output_fmts[0] = imxpd->bus_format; > > This is the legacy DT configured interface-pix-fmt. Maybe this should be > moved to the last place. Hm, shouldn't we restrict things to a single format when we have the DT prop defined? > > > + } else if (di->num_bus_formats) { > > + *num_output_fmts = 1; > > num_output_fmts is already set to one above. Correct, I'll get rid of this assignment. > > > + if (output_fmts) > > + output_fmts[0] = di->bus_formats[0]; > > + } else { > > + *num_output_fmts = ARRAY_SIZE(imx_pd_bus_fmts); > > + if (output_fmts) > > + memcpy(output_fmts, imx_pd_bus_fmts, > > + ARRAY_SIZE(imx_pd_bus_fmts)); > > + } > > +} > > + > > +static void > > +imx_pd_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) > > +{ > > + struct drm_encoder *encoder = bridge_to_encoder(bridge); > > + struct imx_parallel_display *imxpd = enc_to_imxpd(encoder); > > + > > + *num_input_fmts = 0; > > + if (output_fmt == MEDIA_BUS_FMT_FIXED) { > > + *num_input_fmts = 1; > > I don't understand this. The bus format stored in imx_crtc_state by > atomic_check is used to configure the DC later. This should never be > MEDIA_BUS_FMT_FIXED, that would trigger a WARN_ON in > ipu_bus_format_to_map(). MEDIA_BUS_FMT_FIXED is the default value used when the next bridge in the chain does not support bus format negotiation. When the driver receives this value, it should pick a default value (which I might have gotten wrong here) so that it keeps working even if some elements of the bridge chain don't support negotiating the bus format yet. > > > + } else if (!imxpd->bus_format) { > > + unsigned int i; > > + > > + for (i = 0; i < ARRAY_SIZE(imx_pd_bus_fmts); i++) { > > + if (imx_pd_bus_fmts[i] == output_fmt) { > > + *num_input_fmts = 1; > > + break; > > + } > > + } > > + } else if (imxpd->bus_format == output_fmt) { > > + *num_input_fmts = 1; > > + } > > + > > + if (*num_input_fmts && input_fmts) > > + input_fmts[0] = MEDIA_BUS_FMT_FIXED; > > The parallel-display driver basically represents the wiring, pinmux, and > drivers between the IPU DI and the DISP pads. The input format should > always be identical to the output format. I can do that if you like. Note that we are forwarding the ->output_bus_cfg.fmt value to the IPU DI, not ->input_bus_cfg.fmt. I just assumed that input format wouldn't be used in the dummy bridge element (the one embedded in the encoder) since encoders only have an output end (input port is likely to be a SoC specific link between the CRTC and the encoder which we probably don't need/want to expose). > > > +} > > + > > +static int imx_pd_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 drm_encoder *encoder = bridge_to_encoder(bridge); > > struct imx_crtc_state *imx_crtc_state = to_imx_crtc_state(crtc_state); > > struct drm_display_info *di = &conn_state->connector->display_info; > > struct imx_parallel_display *imxpd = enc_to_imxpd(encoder); > > + struct drm_bridge_state *next_bridge_state = NULL; > > + struct drm_bridge *next_bridge; > > + u32 bus_flags, bus_fmt; > > + unsigned int i; > > > > - if (!imxpd->bus_format && di->num_bus_formats) { > > - imx_crtc_state->bus_flags = di->bus_flags; > > - imx_crtc_state->bus_format = di->bus_formats[0]; > > - } else { > > - imx_crtc_state->bus_flags = imxpd->bus_flags; > > - imx_crtc_state->bus_format = imxpd->bus_format; > > + next_bridge = drm_bridge_chain_get_next_bridge(bridge); > > + if (next_bridge) > > + next_bridge_state = drm_atomic_get_new_bridge_state(crtc_state->state, > > + next_bridge); > > + > > + bus_fmt = bridge_state->output_bus_cfg.fmt; > > + if (bus_fmt == MEDIA_BUS_FMT_FIXED) > > + bus_fmt = 0; > > I would expect this to return with -EINVAL if bridge_state- > >output_bus_cfg.fmt is not in the list of supported formats. As said above, we need a way to support bridge chains where not all elements support negotiating the bus format, that's what this fallback is for, but maybe 0 is not an appropriate value to mean 'pick the default format'. > > > + > > + if (next_bridge_state) > > + bus_flags = next_bridge_state->input_bus_cfg.flags; > > + else if (!imxpd->bus_format && di->num_bus_formats) > > + bus_flags = di->bus_flags; > > + else > > + bus_flags = imxpd->bus_flags; > > + > > + for (i = 0; bus_fmt && i < ARRAY_SIZE(imx_pd_bus_fmts); i++) { > > + if (imx_pd_bus_fmts[i] == bridge_state->output_bus_cfg.fmt) > > + break; > > } > > + > > + if (i == ARRAY_SIZE(imx_pd_bus_fmts)) > > + return -EINVAL; > > + > > + if (bus_flags & > > + ~(DRM_BUS_FLAG_DE_LOW | DRM_BUS_FLAG_DE_HIGH | > > + DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE | > > + DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE)) > > + return -EINVAL; > > This could allow DRM_BUS_FLAG_SYNC_DRIVE_POSEDGE/NEGEDGE as well, if > they are not opposite to PIXDATA_DRIVE. Okay, will add those flags and check that PIXDATA and SYNC are consistent.
On Tue, 2019-08-27 at 10:43 +0200, Boris Brezillon wrote: [...] > > > +static void > > > +imx_pd_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, > > > + u32 *output_fmts) > > > { > > > + struct drm_display_info *di = &conn_state->connector->display_info; > > > + struct drm_encoder *encoder = bridge_to_encoder(bridge); > > > + struct imx_parallel_display *imxpd = enc_to_imxpd(encoder); > > > + > > > + *num_output_fmts = 1; > > > + if (imxpd->bus_format) { > > > + if (output_fmts) > > > + output_fmts[0] = imxpd->bus_format; > > > > This is the legacy DT configured interface-pix-fmt. Maybe this should be > > moved to the last place. > > Hm, shouldn't we restrict things to a single format when we have the > DT prop defined? Absolutely. This was just a cosmetic remark. I'm suggesting to put this branch below the other two, to indicate its relative importance. [...] > > > +static void > > > +imx_pd_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) > > > +{ > > > + struct drm_encoder *encoder = bridge_to_encoder(bridge); > > > + struct imx_parallel_display *imxpd = enc_to_imxpd(encoder); > > > + > > > + *num_input_fmts = 0; > > > + if (output_fmt == MEDIA_BUS_FMT_FIXED) { > > > + *num_input_fmts = 1; > > > > I don't understand this. The bus format stored in imx_crtc_state by > > atomic_check is used to configure the DC later. This should never be > > MEDIA_BUS_FMT_FIXED, that would trigger a WARN_ON in > > ipu_bus_format_to_map(). > > MEDIA_BUS_FMT_FIXED is the default value used when the next bridge in > the chain does not support bus format negotiation. When the driver > receives this value, it should pick a default value (which I might have > gotten wrong here) so that it keeps working even if some elements of > the bridge chain don't support negotiating the bus format yet. That was unexpected, as MEDIA_BUS_FMT_FIXED is documented as: "MEDIA_BUS_FMT_FIXED shall be used by host-client pairs, where the data format is fixed." in include/uapi/linux/media-bus-format.h. I read this as something for a fixed link between two known devices where there is only one possible format. Here we can't possibly know what the other side expects. Is this something that is planned to be removed in the future, once negotiation support is commonplace? > > > > > + } else if (!imxpd->bus_format) { > > > + unsigned int i; > > > + > > > + for (i = 0; i < ARRAY_SIZE(imx_pd_bus_fmts); i++) { > > > + if (imx_pd_bus_fmts[i] == output_fmt) { > > > + *num_input_fmts = 1; > > > + break; > > > + } > > > + } > > > + } else if (imxpd->bus_format == output_fmt) { > > > + *num_input_fmts = 1; > > > + } > > > + > > > + if (*num_input_fmts && input_fmts) > > > + input_fmts[0] = MEDIA_BUS_FMT_FIXED; > > > > The parallel-display driver basically represents the wiring, pinmux, and > > drivers between the IPU DI and the DISP pads. The input format should > > always be identical to the output format. > > I can do that if you like. Note that we are forwarding > the ->output_bus_cfg.fmt value to the IPU DI, not ->input_bus_cfg.fmt. > I just assumed that input format wouldn't be used in the dummy bridge > element (the one embedded in the encoder) since encoders only have an > output end (input port is likely to be a SoC specific link between the > CRTC and the encoder which we probably don't need/want to expose). Then why (would this driver have to) implement get_input_fmts at all? I'd like this to be consistent. If encoder-bridges don't use the input bus format in general, I'm fine with this. I was just confused that the bridge takes part in input format negotiation and then carries on using the output format to configure its input. > > > +static int imx_pd_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 drm_encoder *encoder = bridge_to_encoder(bridge); > > > struct imx_crtc_state *imx_crtc_state = to_imx_crtc_state(crtc_state); > > > struct drm_display_info *di = &conn_state->connector->display_info; > > > struct imx_parallel_display *imxpd = enc_to_imxpd(encoder); > > > + struct drm_bridge_state *next_bridge_state = NULL; > > > + struct drm_bridge *next_bridge; > > > + u32 bus_flags, bus_fmt; > > > + unsigned int i; > > > > > > - if (!imxpd->bus_format && di->num_bus_formats) { > > > - imx_crtc_state->bus_flags = di->bus_flags; > > > - imx_crtc_state->bus_format = di->bus_formats[0]; > > > - } else { > > > - imx_crtc_state->bus_flags = imxpd->bus_flags; > > > - imx_crtc_state->bus_format = imxpd->bus_format; > > > + next_bridge = drm_bridge_chain_get_next_bridge(bridge); > > > + if (next_bridge) > > > + next_bridge_state = drm_atomic_get_new_bridge_state(crtc_state->state, > > > + next_bridge); > > > + > > > + bus_fmt = bridge_state->output_bus_cfg.fmt; > > > + if (bus_fmt == MEDIA_BUS_FMT_FIXED) > > > + bus_fmt = 0; > > > > I would expect this to return with -EINVAL if bridge_state- > > > output_bus_cfg.fmt is not in the list of supported formats. > > As said above, we need a way to support bridge chains where not all > elements support negotiating the bus format, that's what this fallback > is for, but maybe 0 is not an appropriate value to mean 'pick the > default format'. We'd actually have to pick one here. If set, that should be imxpd->bus_format. regards Philipp
On Tue, 27 Aug 2019 11:23:02 +0200 Philipp Zabel <p.zabel@pengutronix.de> wrote: > On Tue, 2019-08-27 at 10:43 +0200, Boris Brezillon wrote: > [...] > > > > +static void > > > > +imx_pd_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, > > > > + u32 *output_fmts) > > > > { > > > > + struct drm_display_info *di = &conn_state->connector->display_info; > > > > + struct drm_encoder *encoder = bridge_to_encoder(bridge); > > > > + struct imx_parallel_display *imxpd = enc_to_imxpd(encoder); > > > > + > > > > + *num_output_fmts = 1; > > > > + if (imxpd->bus_format) { > > > > + if (output_fmts) > > > > + output_fmts[0] = imxpd->bus_format; > > > > > > This is the legacy DT configured interface-pix-fmt. Maybe this should be > > > moved to the last place. > > > > Hm, shouldn't we restrict things to a single format when we have the > > DT prop defined? > > Absolutely. This was just a cosmetic remark. I'm suggesting to put this > branch below the other two, to indicate its relative importance. Okay, something like that? *num_output_fmts = 1; if (!imxpd->bus_format && di->num_bus_formats) { if (output_fmts) output_fmts[0] = di->bus_formats[0]; } else if (!imxpd->bus_format) { *num_output_fmts = ARRAY_SIZE(imx_pd_bus_fmts); if (output_fmts) memcpy(output_fmts, imx_pd_bus_fmts, ARRAY_SIZE(imx_pd_bus_fmts)); } else if (output_fmts) { output_fmts[0] = imxpd->bus_format; } > > [...] > > > > +static void > > > > +imx_pd_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) > > > > +{ > > > > + struct drm_encoder *encoder = bridge_to_encoder(bridge); > > > > + struct imx_parallel_display *imxpd = enc_to_imxpd(encoder); > > > > + > > > > + *num_input_fmts = 0; > > > > + if (output_fmt == MEDIA_BUS_FMT_FIXED) { > > > > + *num_input_fmts = 1; > > > > > > I don't understand this. The bus format stored in imx_crtc_state by > > > atomic_check is used to configure the DC later. This should never be > > > MEDIA_BUS_FMT_FIXED, that would trigger a WARN_ON in > > > ipu_bus_format_to_map(). > > > > MEDIA_BUS_FMT_FIXED is the default value used when the next bridge in > > the chain does not support bus format negotiation. When the driver > > receives this value, it should pick a default value (which I might have > > gotten wrong here) so that it keeps working even if some elements of > > the bridge chain don't support negotiating the bus format yet. > > That was unexpected, as MEDIA_BUS_FMT_FIXED is documented as: > > "MEDIA_BUS_FMT_FIXED shall be used by host-client pairs, > where the data format is fixed." > > in include/uapi/linux/media-bus-format.h. I read this as something for a > fixed link between two known devices where there is only one possible > format. Here we can't possibly know what the other side expects. Well, it's more or less what happens right now without the bus format negotiation: bridge elements consider that the link uses a fixed format that's known by both ends in advance. > > Is this something that is planned to be removed in the future, once > negotiation support is commonplace? Ideally yes. > > > > > > > > + } else if (!imxpd->bus_format) { > > > > + unsigned int i; > > > > + > > > > + for (i = 0; i < ARRAY_SIZE(imx_pd_bus_fmts); i++) { > > > > + if (imx_pd_bus_fmts[i] == output_fmt) { > > > > + *num_input_fmts = 1; > > > > + break; > > > > + } > > > > + } > > > > + } else if (imxpd->bus_format == output_fmt) { > > > > + *num_input_fmts = 1; > > > > + } > > > > + > > > > + if (*num_input_fmts && input_fmts) > > > > + input_fmts[0] = MEDIA_BUS_FMT_FIXED; > > > > > > The parallel-display driver basically represents the wiring, pinmux, and > > > drivers between the IPU DI and the DISP pads. The input format should > > > always be identical to the output format. > > > > I can do that if you like. Note that we are forwarding > > the ->output_bus_cfg.fmt value to the IPU DI, not ->input_bus_cfg.fmt. > > I just assumed that input format wouldn't be used in the dummy bridge > > element (the one embedded in the encoder) since encoders only have an > > output end (input port is likely to be a SoC specific link between the > > CRTC and the encoder which we probably don't need/want to expose). > > Then why (would this driver have to) implement get_input_fmts at all? That's the only way we can check if an output format is supported: bus format negotiation is based on a trial and error logic that starts from the end of the pipeline (last bridge element) and goes backward until it reaches the first bridge (the dummy encoder bridge). A bus format setting is validated when all ->get_input_bus_fmts() hooks returned at least one possible format (*num_input_formats > 0) for the output format being tested by the next element in the chain. And that's why even the dummy encoder bridge has to implement this hook unless it only supports one output format (the core returns MEDIA_BUS_FMT_FIXED when ->get_input_bus_fmts is NULL). > > I'd like this to be consistent. If encoder-bridges don't use the input > bus format in general, I'm fine with this. Propagating output to input doesn't hurt, and if you think that's clearer this way I'm perfectly fine adjusting the driver accordingly. > > I was just confused that the bridge takes part in input format > negotiation and then carries on using the output format to configure its > input. Maybe I'm wrong, but I thought it was the parallel (AKA DPI) encoder output we were configuring here. Anyway, I'll choose one semantic and document it. > > > > > +static int imx_pd_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 drm_encoder *encoder = bridge_to_encoder(bridge); > > > > struct imx_crtc_state *imx_crtc_state = to_imx_crtc_state(crtc_state); > > > > struct drm_display_info *di = &conn_state->connector->display_info; > > > > struct imx_parallel_display *imxpd = enc_to_imxpd(encoder); > > > > + struct drm_bridge_state *next_bridge_state = NULL; > > > > + struct drm_bridge *next_bridge; > > > > + u32 bus_flags, bus_fmt; > > > > + unsigned int i; > > > > > > > > - if (!imxpd->bus_format && di->num_bus_formats) { > > > > - imx_crtc_state->bus_flags = di->bus_flags; > > > > - imx_crtc_state->bus_format = di->bus_formats[0]; > > > > - } else { > > > > - imx_crtc_state->bus_flags = imxpd->bus_flags; > > > > - imx_crtc_state->bus_format = imxpd->bus_format; > > > > + next_bridge = drm_bridge_chain_get_next_bridge(bridge); > > > > + if (next_bridge) > > > > + next_bridge_state = drm_atomic_get_new_bridge_state(crtc_state->state, > > > > + next_bridge); > > > > + > > > > + bus_fmt = bridge_state->output_bus_cfg.fmt; > > > > + if (bus_fmt == MEDIA_BUS_FMT_FIXED) > > > > + bus_fmt = 0; > > > > > > I would expect this to return with -EINVAL if bridge_state- > > > > output_bus_cfg.fmt is not in the list of supported formats. > > > > As said above, we need a way to support bridge chains where not all > > elements support negotiating the bus format, that's what this fallback > > is for, but maybe 0 is not an appropriate value to mean 'pick the > > default format'. > > We'd actually have to pick one here. If set, that should be > imxpd->bus_format. What if imxpd->bus_format is 0? Should I return -EINVAL?
On Tue, 2019-08-27 at 11:57 +0200, Boris Brezillon wrote: > On Tue, 27 Aug 2019 11:23:02 +0200 > Philipp Zabel <p.zabel@pengutronix.de> wrote: > > On Tue, 2019-08-27 at 10:43 +0200, Boris Brezillon wrote: [...] > > Absolutely. This was just a cosmetic remark. I'm suggesting to put this > > branch below the other two, to indicate its relative importance. > > Okay, something like that? > > *num_output_fmts = 1; > if (!imxpd->bus_format && di->num_bus_formats) { > if (output_fmts) > output_fmts[0] = di->bus_formats[0]; > } else if (!imxpd->bus_format) { > *num_output_fmts = ARRAY_SIZE(imx_pd_bus_fmts); > if (output_fmts) > memcpy(output_fmts, imx_pd_bus_fmts, > ARRAY_SIZE(imx_pd_bus_fmts)); > } else if (output_fmts) { > output_fmts[0] = imxpd->bus_format; > } Yes, thank you. [...] > > That was unexpected, as MEDIA_BUS_FMT_FIXED is documented as: > > > > "MEDIA_BUS_FMT_FIXED shall be used by host-client pairs, > > where the data format is fixed." > > > > in include/uapi/linux/media-bus-format.h. I read this as something for a > > fixed link between two known devices where there is only one possible > > format. Here we can't possibly know what the other side expects. > > Well, it's more or less what happens right now without the bus format > negotiation: bridge elements consider that the link uses a fixed format > that's known by both ends in advance. Ok. And with the legacy DT provided bus format we can even choose a sensible format. [...] > > > I can do that if you like. Note that we are forwarding > > > the ->output_bus_cfg.fmt value to the IPU DI, not ->input_bus_cfg.fmt. > > > I just assumed that input format wouldn't be used in the dummy bridge > > > element (the one embedded in the encoder) since encoders only have an > > > output end (input port is likely to be a SoC specific link between the > > > CRTC and the encoder which we probably don't need/want to expose). > > > > Then why (would this driver have to) implement get_input_fmts at all? > > That's the only way we can check if an output format is supported: bus > format negotiation is based on a trial and error logic that starts from > the end of the pipeline (last bridge element) and goes backward until > it reaches the first bridge (the dummy encoder bridge). A bus format > setting is validated when all ->get_input_bus_fmts() hooks returned at > least one possible format (*num_input_formats > 0) for the output format > being tested by the next element in the chain. And that's why even the > dummy encoder bridge has to implement this hook unless it only supports > one output format (the core returns MEDIA_BUS_FMT_FIXED when > ->get_input_bus_fmts is NULL). This function (currently) also only returns MEDIA_BUS_FMT_FIXED, so there is no difference in return value (if queried with a supported output_fmt). select_bus_fmt_recursive could just check atomic_get_output_bus_fmts if that is implemented, but atomic_get_input_bus_fmts isn't. That point is moot though, if we propagate the output format to the input format. > > I'd like this to be consistent. If encoder-bridges don't use the input > > bus format in general, I'm fine with this. > > Propagating output to input doesn't hurt, and if you think that's > clearer this way I'm perfectly fine adjusting the driver accordingly. Yes, I'd like that. > > I was just confused that the bridge takes part in input format > > negotiation and then carries on using the output format to configure its > > input. > > Maybe I'm wrong, but I thought it was the parallel (AKA DPI) encoder > output we were configuring here. > Anyway, I'll choose one semantic and document it. Semantics :) You are not wrong, the IPU DIs technically output DPI compatible signals, internally. Those are fed through muxes either into the HDMI/LVDS/TV encoders, or directly into the IOMUX block, which drives them onto the DISP pads. The IPU driver is from a time before bridges and of-graph bindings were established. We chose to consider the whole IDMAC/DP/DC/DI path as crtc, and the HDMI / LVDS / IOMUX/DISP blocks as encoder, because the crtc- encoder assignment mapped well to configuring the muxes between the components. Technically we could just as well consider IDMAC/DP/part-of-DC to be the crtc and part-of-DC/DI to be a DPI encoder to which the HDMI/LVDS/TV bridges are connected, but today we'd still miss the possibility to multiplex several encoders into the same bridge. [...] > > > As said above, we need a way to support bridge chains where not all > > > elements support negotiating the bus format, that's what this fallback > > > is for, but maybe 0 is not an appropriate value to mean 'pick the > > > default format'. > > > > We'd actually have to pick one here. If set, that should be > > imxpd->bus_format. > > What if imxpd->bus_format is 0? Should I return -EINVAL? I think so. That would certainly be easier to debug than "strange colours" when hooking up a bridge that is incompatible with whatever we'd choose. regards Philipp
On Tue, 27 Aug 2019 14:51:20 +0200 Philipp Zabel <p.zabel@pengutronix.de> wrote: > [...] > > > > I can do that if you like. Note that we are forwarding > > > > the ->output_bus_cfg.fmt value to the IPU DI, not ->input_bus_cfg.fmt. > > > > I just assumed that input format wouldn't be used in the dummy bridge > > > > element (the one embedded in the encoder) since encoders only have an > > > > output end (input port is likely to be a SoC specific link between the > > > > CRTC and the encoder which we probably don't need/want to expose). > > > > > > Then why (would this driver have to) implement get_input_fmts at all? > > > > That's the only way we can check if an output format is supported: bus > > format negotiation is based on a trial and error logic that starts from > > the end of the pipeline (last bridge element) and goes backward until > > it reaches the first bridge (the dummy encoder bridge). A bus format > > setting is validated when all ->get_input_bus_fmts() hooks returned at > > least one possible format (*num_input_formats > 0) for the output format > > being tested by the next element in the chain. And that's why even the > > dummy encoder bridge has to implement this hook unless it only supports > > one output format (the core returns MEDIA_BUS_FMT_FIXED when > > ->get_input_bus_fmts is NULL). > > This function (currently) also only returns MEDIA_BUS_FMT_FIXED, so > there is no difference in return value (if queried with a supported > output_fmt). There's a small difference actually: when output_fmt != MEDIA_BUS_FMT_FIXED, we make sure output_fmt is something we support. If you don't implement ->get_input_bus_fmts() you'll just accept any format requested by the next element in the pipeline. > select_bus_fmt_recursive could just check atomic_get_output_bus_fmts if > that is implemented, but atomic_get_input_bus_fmts isn't. I'd like to use ->get_output_bus_fmts() only to retrieve supported output formats on the last bridge element, otherwise it makes the whole thing even more complex. > > That point is moot though, if we propagate the output format to the > input format. I'll do that then (and mandate that all encoder drivers do the same). > > [...] > > > > As said above, we need a way to support bridge chains where not all > > > > elements support negotiating the bus format, that's what this fallback > > > > is for, but maybe 0 is not an appropriate value to mean 'pick the > > > > default format'. > > > > > > We'd actually have to pick one here. If set, that should be > > > imxpd->bus_format. > > > > What if imxpd->bus_format is 0? Should I return -EINVAL? > > I think so. That would certainly be easier to debug than "strange > colours" when hooking up a bridge that is incompatible with whatever > we'd choose. Okay.
On Tue, 2019-08-27 at 15:20 +0200, Boris Brezillon wrote: > On Tue, 27 Aug 2019 14:51:20 +0200 > Philipp Zabel <p.zabel@pengutronix.de> wrote: > > > [...] > > > > > I can do that if you like. Note that we are forwarding > > > > > the ->output_bus_cfg.fmt value to the IPU DI, not ->input_bus_cfg.fmt. > > > > > I just assumed that input format wouldn't be used in the dummy bridge > > > > > element (the one embedded in the encoder) since encoders only have an > > > > > output end (input port is likely to be a SoC specific link between the > > > > > CRTC and the encoder which we probably don't need/want to expose). > > > > > > > > Then why (would this driver have to) implement get_input_fmts at all? > > > > > > That's the only way we can check if an output format is supported: bus > > > format negotiation is based on a trial and error logic that starts from > > > the end of the pipeline (last bridge element) and goes backward until > > > it reaches the first bridge (the dummy encoder bridge). A bus format > > > setting is validated when all ->get_input_bus_fmts() hooks returned at > > > least one possible format (*num_input_formats > 0) for the output format > > > being tested by the next element in the chain. And that's why even the > > > dummy encoder bridge has to implement this hook unless it only supports > > > one output format (the core returns MEDIA_BUS_FMT_FIXED when > > > ->get_input_bus_fmts is NULL). > > > > This function (currently) also only returns MEDIA_BUS_FMT_FIXED, so > > there is no difference in return value (if queried with a supported > > output_fmt). > > There's a small difference actually: when output_fmt != > MEDIA_BUS_FMT_FIXED, we make sure output_fmt is something we support. If > you don't implement ->get_input_bus_fmts() you'll just accept any format > requested by the next element in the pipeline. I see. My whole point was predicated on the incorrect assumption that get_input_bus_fmts would not have to be fed usupported output_fmts. > > select_bus_fmt_recursive could just check atomic_get_output_bus_fmts if > > that is implemented, but atomic_get_input_bus_fmts isn't. > > I'd like to use ->get_output_bus_fmts() only to retrieve supported > output formats on the last bridge element, otherwise it makes the > whole thing even more complex. Ok, I should have paid more attention to the documentation in patch 16. > > That point is moot though, if we propagate the output format to the > > input format. > > I'll do that then (and mandate that all encoder drivers do the same). Sounds good. At least then there aren't two classes of bridges that implement the negotiation callbacks differently. regards Philipp
diff --git a/drivers/gpu/drm/imx/parallel-display.c b/drivers/gpu/drm/imx/parallel-display.c index 35518e5de356..df47cb74759f 100644 --- a/drivers/gpu/drm/imx/parallel-display.c +++ b/drivers/gpu/drm/imx/parallel-display.c @@ -89,37 +89,139 @@ static struct drm_encoder *imx_pd_connector_best_encoder( return &imxpd->encoder; } -static void imx_pd_encoder_enable(struct drm_encoder *encoder) +static void imx_pd_bridge_enable(struct drm_bridge *bridge) { + struct drm_encoder *encoder = bridge_to_encoder(bridge); struct imx_parallel_display *imxpd = enc_to_imxpd(encoder); drm_panel_prepare(imxpd->panel); drm_panel_enable(imxpd->panel); } -static void imx_pd_encoder_disable(struct drm_encoder *encoder) +static void imx_pd_bridge_disable(struct drm_bridge *bridge) { + struct drm_encoder *encoder = bridge_to_encoder(bridge); struct imx_parallel_display *imxpd = enc_to_imxpd(encoder); drm_panel_disable(imxpd->panel); drm_panel_unprepare(imxpd->panel); } -static int imx_pd_encoder_atomic_check(struct drm_encoder *encoder, - struct drm_crtc_state *crtc_state, - struct drm_connector_state *conn_state) +static const u32 imx_pd_bus_fmts[] = { + MEDIA_BUS_FMT_RGB888_1X24, + MEDIA_BUS_FMT_RGB565_1X16, + MEDIA_BUS_FMT_RGB666_1X18, + MEDIA_BUS_FMT_RGB666_1X24_CPADHI, +}; + +static void +imx_pd_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, + u32 *output_fmts) { + struct drm_display_info *di = &conn_state->connector->display_info; + struct drm_encoder *encoder = bridge_to_encoder(bridge); + struct imx_parallel_display *imxpd = enc_to_imxpd(encoder); + + *num_output_fmts = 1; + if (imxpd->bus_format) { + if (output_fmts) + output_fmts[0] = imxpd->bus_format; + } else if (di->num_bus_formats) { + *num_output_fmts = 1; + if (output_fmts) + output_fmts[0] = di->bus_formats[0]; + } else { + *num_output_fmts = ARRAY_SIZE(imx_pd_bus_fmts); + if (output_fmts) + memcpy(output_fmts, imx_pd_bus_fmts, + ARRAY_SIZE(imx_pd_bus_fmts)); + } +} + +static void +imx_pd_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) +{ + struct drm_encoder *encoder = bridge_to_encoder(bridge); + struct imx_parallel_display *imxpd = enc_to_imxpd(encoder); + + *num_input_fmts = 0; + if (output_fmt == MEDIA_BUS_FMT_FIXED) { + *num_input_fmts = 1; + } else if (!imxpd->bus_format) { + unsigned int i; + + for (i = 0; i < ARRAY_SIZE(imx_pd_bus_fmts); i++) { + if (imx_pd_bus_fmts[i] == output_fmt) { + *num_input_fmts = 1; + break; + } + } + } else if (imxpd->bus_format == output_fmt) { + *num_input_fmts = 1; + } + + if (*num_input_fmts && input_fmts) + input_fmts[0] = MEDIA_BUS_FMT_FIXED; +} + +static int imx_pd_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 drm_encoder *encoder = bridge_to_encoder(bridge); struct imx_crtc_state *imx_crtc_state = to_imx_crtc_state(crtc_state); struct drm_display_info *di = &conn_state->connector->display_info; struct imx_parallel_display *imxpd = enc_to_imxpd(encoder); + struct drm_bridge_state *next_bridge_state = NULL; + struct drm_bridge *next_bridge; + u32 bus_flags, bus_fmt; + unsigned int i; - if (!imxpd->bus_format && di->num_bus_formats) { - imx_crtc_state->bus_flags = di->bus_flags; - imx_crtc_state->bus_format = di->bus_formats[0]; - } else { - imx_crtc_state->bus_flags = imxpd->bus_flags; - imx_crtc_state->bus_format = imxpd->bus_format; + next_bridge = drm_bridge_chain_get_next_bridge(bridge); + if (next_bridge) + next_bridge_state = drm_atomic_get_new_bridge_state(crtc_state->state, + next_bridge); + + bus_fmt = bridge_state->output_bus_cfg.fmt; + if (bus_fmt == MEDIA_BUS_FMT_FIXED) + bus_fmt = 0; + + if (next_bridge_state) + bus_flags = next_bridge_state->input_bus_cfg.flags; + else if (!imxpd->bus_format && di->num_bus_formats) + bus_flags = di->bus_flags; + else + bus_flags = imxpd->bus_flags; + + for (i = 0; bus_fmt && i < ARRAY_SIZE(imx_pd_bus_fmts); i++) { + if (imx_pd_bus_fmts[i] == bridge_state->output_bus_cfg.fmt) + break; } + + if (i == ARRAY_SIZE(imx_pd_bus_fmts)) + return -EINVAL; + + if (bus_flags & + ~(DRM_BUS_FLAG_DE_LOW | DRM_BUS_FLAG_DE_HIGH | + DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE | + DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE)) + return -EINVAL; + + bridge_state->output_bus_cfg.flags = bus_flags; + bridge_state->input_bus_cfg.flags = bus_flags; + imx_crtc_state->bus_flags = bus_flags; + imx_crtc_state->bus_format = bridge_state->output_bus_cfg.fmt; imx_crtc_state->di_hsync_pin = 2; imx_crtc_state->di_vsync_pin = 3; @@ -143,10 +245,12 @@ static const struct drm_encoder_funcs imx_pd_encoder_funcs = { .destroy = imx_drm_encoder_destroy, }; -static const struct drm_encoder_helper_funcs imx_pd_encoder_helper_funcs = { - .enable = imx_pd_encoder_enable, - .disable = imx_pd_encoder_disable, - .atomic_check = imx_pd_encoder_atomic_check, +static const struct drm_bridge_funcs imx_pd_bridge_funcs = { + .enable = imx_pd_bridge_enable, + .disable = imx_pd_bridge_disable, + .atomic_check = imx_pd_bridge_atomic_check, + .atomic_get_input_bus_fmts = imx_pd_bridge_atomic_get_input_bus_fmts, + .atomic_get_output_bus_fmts = imx_pd_bridge_atomic_get_output_bus_fmts, }; static int imx_pd_register(struct drm_device *drm, @@ -166,7 +270,7 @@ static int imx_pd_register(struct drm_device *drm, */ imxpd->connector.dpms = DRM_MODE_DPMS_OFF; - drm_encoder_helper_add(encoder, &imx_pd_encoder_helper_funcs); + encoder->bridge.funcs = &imx_pd_bridge_funcs; drm_encoder_init(drm, encoder, &imx_pd_encoder_funcs, DRM_MODE_ENCODER_NONE, NULL);
Now that bridges can expose the bus format/flags they expect, we can use those instead of the relying on the display_info provided by the connector (which is only valid if the encoder is directly connected to bridge element driving the panel/display). We also explicitly expose the bus formats supported by our encoder by filling encoder->output_bus_caps with proper info. Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com> --- Changes in v2: * Adjust things to match the new bus-format negotiation infra --- drivers/gpu/drm/imx/parallel-display.c | 136 ++++++++++++++++++++++--- 1 file changed, 120 insertions(+), 16 deletions(-)