| Message ID | 20211014152606.2289528-2-narmstrong@baylibre.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Neil Armstrong |
| Headers | show |
| Series | drm/meson: rework encoders to pass ATTACH_NO_CONNECTOR | expand |
Hi Neil, code looks fine. A few improvement proposals to the comments. With the include order fixed and the comments considered: Reviewed-by: Sam Ravnborg <sam@ravnborg.org> Sam On Thu, Oct 14, 2021 at 05:26:00PM +0200, Neil Armstrong wrote: > Since this bridge is tied to the connector, it acts like a passthrough, > so concerning the output & input bus formats, either pass the bus formats from the > previous bridge or return fallback data like done in the bridge function: > drm_atomic_bridge_chain_select_bus_fmts() & select_bus_fmt_recursive. > > This permits avoiding skipping the negociation if the remaining bridge chain has > all the bits in place. > > Without this bus fmt negociation breaks on drm/meson HDMI pipeline when attaching > dw-hdmi with DRM_BRIDGE_ATTACH_NO_CONNECTOR, because the last bridge of the > display-connector doesn't implement buf fmt callbacks and MEDIA_BUS_FMT_FIXED > is used leading to select an unsupported default bus format from dw-hdmi. > > Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> > --- > drivers/gpu/drm/bridge/display-connector.c | 88 ++++++++++++++++++++++ > 1 file changed, 88 insertions(+) > > diff --git a/drivers/gpu/drm/bridge/display-connector.c b/drivers/gpu/drm/bridge/display-connector.c > index 05eb759da6fc..9697ac173157 100644 > --- a/drivers/gpu/drm/bridge/display-connector.c > +++ b/drivers/gpu/drm/bridge/display-connector.c > @@ -14,6 +14,7 @@ > #include <linux/regulator/consumer.h> > > #include <drm/drm_bridge.h> > +#include <drm/drm_atomic_helper.h> > #include <drm/drm_edid.h> Alphabetic order. > > struct display_connector { > @@ -87,10 +88,97 @@ static struct edid *display_connector_get_edid(struct drm_bridge *bridge, > return drm_get_edid(connector, conn->bridge.ddc); > } > > +/* > + * Since this bridge is tied to the connector, it acts like a passthrough, > + * so concerning the output bus formats, either pass the bus formats from the > + * previous bridge or return fallback data like done in the bridge function: > + * drm_atomic_bridge_chain_select_bus_fmts(). > + * This permits avoiding skipping the negociation if the bridge chain has all > + * bits in place. negociation if => negotiation of Consider the following wording: This supports negotiation if the bridge chain has all bits in place. > + */ > +static u32 *display_connector_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_bridge *prev_bridge = drm_bridge_get_prev_bridge(bridge); > + struct drm_bridge_state *prev_bridge_state; > + > + if (!prev_bridge || !prev_bridge->funcs->atomic_get_output_bus_fmts) { > + struct drm_connector *conn = conn_state->connector; > + u32 *out_bus_fmts; > + > + *num_output_fmts = 1; > + out_bus_fmts = kmalloc(sizeof(*out_bus_fmts), GFP_KERNEL); > + if (!out_bus_fmts) > + return NULL; > + > + if (conn->display_info.num_bus_formats && > + conn->display_info.bus_formats) > + out_bus_fmts[0] = conn->display_info.bus_formats[0]; > + else > + out_bus_fmts[0] = MEDIA_BUS_FMT_FIXED; > + > + return out_bus_fmts; > + } > + > + prev_bridge_state = drm_atomic_get_new_bridge_state(crtc_state->state, > + prev_bridge); > + > + return prev_bridge->funcs->atomic_get_output_bus_fmts(prev_bridge, prev_bridge_state, > + crtc_state, conn_state, > + num_output_fmts); > +} > + > +/* > + * Since this bridge is tied to the connector, it acts like a passthrough, > + * so concerning the input bus formats, either pass the bus formats from the > + * previous bridge or return fallback data like done in the bridge function: > + * select_bus_fmt_recursive() when atomic_get_input_bus_fmts is not supported. Maybe use this this: from the previous bridge or MEDIA_BUS_FMT_FIXED (like select_bus_fmt_recursive()) > + * This permits avoiding skipping the negociation if the bridge chain has all > + * bits in place. Like above > + */ > +static u32 *display_connector_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) > +{ > + struct drm_bridge *prev_bridge = drm_bridge_get_prev_bridge(bridge); > + struct drm_bridge_state *prev_bridge_state; > + > + if (!prev_bridge || !prev_bridge->funcs->atomic_get_input_bus_fmts) { > + u32 *in_bus_fmts; > + > + *num_input_fmts = 1; > + in_bus_fmts = kmalloc(sizeof(*in_bus_fmts), GFP_KERNEL); > + if (!in_bus_fmts) > + return NULL; > + > + in_bus_fmts[0] = MEDIA_BUS_FMT_FIXED; > + > + return in_bus_fmts; > + } > + > + prev_bridge_state = drm_atomic_get_new_bridge_state(crtc_state->state, > + prev_bridge); > + > + return prev_bridge->funcs->atomic_get_input_bus_fmts(prev_bridge, prev_bridge_state, > + crtc_state, conn_state, output_fmt, > + num_input_fmts); > +} > + > static const struct drm_bridge_funcs display_connector_bridge_funcs = { > .attach = display_connector_attach, > .detect = display_connector_detect, > .get_edid = display_connector_get_edid, > + .atomic_get_output_bus_fmts = display_connector_get_output_bus_fmts, > + .atomic_get_input_bus_fmts = display_connector_get_input_bus_fmts, > + .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, > + .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state, > + .atomic_reset = drm_atomic_helper_bridge_reset, > }; > > static irqreturn_t display_connector_hpd_irq(int irq, void *arg) > -- > 2.25.1
diff --git a/drivers/gpu/drm/bridge/display-connector.c b/drivers/gpu/drm/bridge/display-connector.c index 05eb759da6fc..9697ac173157 100644 --- a/drivers/gpu/drm/bridge/display-connector.c +++ b/drivers/gpu/drm/bridge/display-connector.c @@ -14,6 +14,7 @@ #include <linux/regulator/consumer.h> #include <drm/drm_bridge.h> +#include <drm/drm_atomic_helper.h> #include <drm/drm_edid.h> struct display_connector { @@ -87,10 +88,97 @@ static struct edid *display_connector_get_edid(struct drm_bridge *bridge, return drm_get_edid(connector, conn->bridge.ddc); } +/* + * Since this bridge is tied to the connector, it acts like a passthrough, + * so concerning the output bus formats, either pass the bus formats from the + * previous bridge or return fallback data like done in the bridge function: + * drm_atomic_bridge_chain_select_bus_fmts(). + * This permits avoiding skipping the negociation if the bridge chain has all + * bits in place. + */ +static u32 *display_connector_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_bridge *prev_bridge = drm_bridge_get_prev_bridge(bridge); + struct drm_bridge_state *prev_bridge_state; + + if (!prev_bridge || !prev_bridge->funcs->atomic_get_output_bus_fmts) { + struct drm_connector *conn = conn_state->connector; + u32 *out_bus_fmts; + + *num_output_fmts = 1; + out_bus_fmts = kmalloc(sizeof(*out_bus_fmts), GFP_KERNEL); + if (!out_bus_fmts) + return NULL; + + if (conn->display_info.num_bus_formats && + conn->display_info.bus_formats) + out_bus_fmts[0] = conn->display_info.bus_formats[0]; + else + out_bus_fmts[0] = MEDIA_BUS_FMT_FIXED; + + return out_bus_fmts; + } + + prev_bridge_state = drm_atomic_get_new_bridge_state(crtc_state->state, + prev_bridge); + + return prev_bridge->funcs->atomic_get_output_bus_fmts(prev_bridge, prev_bridge_state, + crtc_state, conn_state, + num_output_fmts); +} + +/* + * Since this bridge is tied to the connector, it acts like a passthrough, + * so concerning the input bus formats, either pass the bus formats from the + * previous bridge or return fallback data like done in the bridge function: + * select_bus_fmt_recursive() when atomic_get_input_bus_fmts is not supported. + * This permits avoiding skipping the negociation if the bridge chain has all + * bits in place. + */ +static u32 *display_connector_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) +{ + struct drm_bridge *prev_bridge = drm_bridge_get_prev_bridge(bridge); + struct drm_bridge_state *prev_bridge_state; + + if (!prev_bridge || !prev_bridge->funcs->atomic_get_input_bus_fmts) { + u32 *in_bus_fmts; + + *num_input_fmts = 1; + in_bus_fmts = kmalloc(sizeof(*in_bus_fmts), GFP_KERNEL); + if (!in_bus_fmts) + return NULL; + + in_bus_fmts[0] = MEDIA_BUS_FMT_FIXED; + + return in_bus_fmts; + } + + prev_bridge_state = drm_atomic_get_new_bridge_state(crtc_state->state, + prev_bridge); + + return prev_bridge->funcs->atomic_get_input_bus_fmts(prev_bridge, prev_bridge_state, + crtc_state, conn_state, output_fmt, + num_input_fmts); +} + static const struct drm_bridge_funcs display_connector_bridge_funcs = { .attach = display_connector_attach, .detect = display_connector_detect, .get_edid = display_connector_get_edid, + .atomic_get_output_bus_fmts = display_connector_get_output_bus_fmts, + .atomic_get_input_bus_fmts = display_connector_get_input_bus_fmts, + .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, + .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state, + .atomic_reset = drm_atomic_helper_bridge_reset, }; static irqreturn_t display_connector_hpd_irq(int irq, void *arg)
Since this bridge is tied to the connector, it acts like a passthrough, so concerning the output & input bus formats, either pass the bus formats from the previous bridge or return fallback data like done in the bridge function: drm_atomic_bridge_chain_select_bus_fmts() & select_bus_fmt_recursive. This permits avoiding skipping the negociation if the remaining bridge chain has all the bits in place. Without this bus fmt negociation breaks on drm/meson HDMI pipeline when attaching dw-hdmi with DRM_BRIDGE_ATTACH_NO_CONNECTOR, because the last bridge of the display-connector doesn't implement buf fmt callbacks and MEDIA_BUS_FMT_FIXED is used leading to select an unsupported default bus format from dw-hdmi. Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> --- drivers/gpu/drm/bridge/display-connector.c | 88 ++++++++++++++++++++++ 1 file changed, 88 insertions(+)