RE: [PATCH v3 7/9] drm/atomic-helper: Add select_output_bus_format callback
Hi Maxime, Thank you for the review. > -Original Message- > From: Maxime Ripard > Sent: Friday, March 22, 2024 2:45 AM > To: Klymenko, Anatoliy > Cc: Laurent Pinchart ; Maarten Lankhorst > ; Thomas Zimmermann > ; David Airlie ; Daniel Vetter > ; Simek, Michal ; Andrzej Hajda > ; Neil Armstrong ; Robert > Foss ; Jonas Karlman ; Jernej Skrabec > ; Rob Herring ; Krzysztof > Kozlowski ; Conor Dooley > ; Mauro Carvalho Chehab ; Tomi > Valkeinen ; dri-devel@lists.freedesktop.org; > linux-arm-ker...@lists.infradead.org; linux-ker...@vger.kernel.org; > devicet...@vger.kernel.org; linux-me...@vger.kernel.org > Subject: Re: [PATCH v3 7/9] drm/atomic-helper: Add select_output_bus_format > callback > > On Thu, Mar 21, 2024 at 01:43:45PM -0700, Anatoliy Klymenko wrote: > > diff --git a/drivers/gpu/drm/drm_crtc_helper.c > > b/drivers/gpu/drm/drm_crtc_helper.c > > index 2dafc39a27cb..f2e12a3c4e5f 100644 > > --- a/drivers/gpu/drm/drm_crtc_helper.c > > +++ b/drivers/gpu/drm/drm_crtc_helper.c > > @@ -1055,3 +1055,39 @@ int drm_helper_force_disable_all(struct > drm_device *dev) > > return ret; > > } > > EXPORT_SYMBOL(drm_helper_force_disable_all); > > + > > +/** > > + * drm_helper_crtc_select_output_bus_format - Select output media bus > > +format > > + * @crtc: The CRTC to query > > + * @crtc_state: The new CRTC state > > + * @supported_fmts: List of media bus format options to pick from > > + * @num_supported_fmts: Number of media bus formats in > > +@supported_fmts list > > + * > > + * Encoder drivers may call this helper to give the connected CRTC a > > +chance to > > + * select compatible or preffered media bus format to use over the > > +CRTC encoder > > + * link. Encoders should provide list of supported input > > +MEDIA_BUS_FMT_* for > > + * CRTC to pick from. CRTC driver is expected to select preferred > > +media bus > > + * format from the list and, once enabled, generate the signal accordingly. > > + * > > + * Returns: > > + * Selected preferred media bus format or 0 if CRTC does not support > > +any from > > + * @supported_fmts list. > > + */ > > +u32 drm_helper_crtc_select_output_bus_format(struct drm_crtc *crtc, > > +struct drm_crtc_state *crtc_state, > > +const u32 *supported_fmts, > > +unsigned int num_supported_fmts) { > > + if (!crtc || !supported_fmts || !num_supported_fmts) > > + return 0; > > + > > + if (!crtc->helper_private || > > + !crtc->helper_private->select_output_bus_format) > > + return supported_fmts[0]; > > + > > + return crtc->helper_private->select_output_bus_format(crtc, > > + crtc_state, > > + supported_fmts, > > + num_supported_fmts); > > +} > > Again, the output of that selection must be found in the CRTC state, otherwise > CRTCs have no way to know which have been selected. > Yes, now I got it - thank you. I'll fix this in the next version. > Maxime Thank you, Anatoliy
Re: [PATCH v3 7/9] drm/atomic-helper: Add select_output_bus_format callback
On Thu, Mar 21, 2024 at 01:43:45PM -0700, Anatoliy Klymenko wrote: > diff --git a/drivers/gpu/drm/drm_crtc_helper.c > b/drivers/gpu/drm/drm_crtc_helper.c > index 2dafc39a27cb..f2e12a3c4e5f 100644 > --- a/drivers/gpu/drm/drm_crtc_helper.c > +++ b/drivers/gpu/drm/drm_crtc_helper.c > @@ -1055,3 +1055,39 @@ int drm_helper_force_disable_all(struct drm_device > *dev) > return ret; > } > EXPORT_SYMBOL(drm_helper_force_disable_all); > + > +/** > + * drm_helper_crtc_select_output_bus_format - Select output media bus format > + * @crtc: The CRTC to query > + * @crtc_state: The new CRTC state > + * @supported_fmts: List of media bus format options to pick from > + * @num_supported_fmts: Number of media bus formats in @supported_fmts list > + * > + * Encoder drivers may call this helper to give the connected CRTC a chance > to > + * select compatible or preffered media bus format to use over the CRTC > encoder > + * link. Encoders should provide list of supported input MEDIA_BUS_FMT_* for > + * CRTC to pick from. CRTC driver is expected to select preferred media bus > + * format from the list and, once enabled, generate the signal accordingly. > + * > + * Returns: > + * Selected preferred media bus format or 0 if CRTC does not support any from > + * @supported_fmts list. > + */ > +u32 drm_helper_crtc_select_output_bus_format(struct drm_crtc *crtc, > + struct drm_crtc_state *crtc_state, > + const u32 *supported_fmts, > + unsigned int num_supported_fmts) > +{ > + if (!crtc || !supported_fmts || !num_supported_fmts) > + return 0; > + > + if (!crtc->helper_private || > + !crtc->helper_private->select_output_bus_format) > + return supported_fmts[0]; > + > + return crtc->helper_private->select_output_bus_format(crtc, > + crtc_state, > + supported_fmts, > + num_supported_fmts); > +} Again, the output of that selection must be found in the CRTC state, otherwise CRTCs have no way to know which have been selected. Maxime signature.asc Description: PGP signature
[PATCH v3 7/9] drm/atomic-helper: Add select_output_bus_format callback
Add optional drm_crtc_helper_funcs.select_output_bus_format callback. This callback allows to negotiate compatible media bus format on the link between CRTC and connected DRM encoder or DRM bridge chain. A good usage example is the CRTC implemented as FPGA soft IP. This kind of CRTC will most certainly support a single output media bus format, as supporting multiple runtime options consumes extra FPGA resources. A variety of options for the FPGA designs are usually achieved by synthesizing IP with different parameters. Add drm_helper_crtc_select_output_bus_format that wraps drm_crtc_helper_funcs.select_output_bus_format. Incorporate select_output_bus_format callback into the format negotiation stage to fix the input bus format of the first DRM bridge in the chain. Signed-off-by: Anatoliy Klymenko --- drivers/gpu/drm/drm_bridge.c | 14 +++-- drivers/gpu/drm/drm_crtc_helper.c| 36 include/drm/drm_crtc_helper.h| 5 + include/drm/drm_modeset_helper_vtables.h | 30 ++ 4 files changed, 83 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index 521a71c61b16..955ca108cd4b 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -28,6 +28,7 @@ #include #include +#include #include #include #include @@ -879,7 +880,8 @@ static int select_bus_fmt_recursive(struct drm_bridge *first_bridge, unsigned int i, num_in_bus_fmts = 0; struct drm_bridge_state *cur_state; struct drm_bridge *prev_bridge; - u32 *in_bus_fmts; + struct drm_crtc *crtc = crtc_state->crtc; + u32 *in_bus_fmts, in_fmt; int ret; prev_bridge = drm_bridge_get_prev_bridge(cur_bridge); @@ -933,7 +935,15 @@ static int select_bus_fmt_recursive(struct drm_bridge *first_bridge, return -ENOMEM; if (first_bridge == cur_bridge) { - cur_state->input_bus_cfg.format = in_bus_fmts[0]; + in_fmt = drm_helper_crtc_select_output_bus_format(crtc, + crtc_state, + in_bus_fmts, + num_in_bus_fmts); + if (!in_fmt) { + kfree(in_bus_fmts); + return -ENOTSUPP; + } + cur_state->input_bus_cfg.format = in_fmt; cur_state->output_bus_cfg.format = out_bus_fmt; kfree(in_bus_fmts); return 0; diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c index 2dafc39a27cb..f2e12a3c4e5f 100644 --- a/drivers/gpu/drm/drm_crtc_helper.c +++ b/drivers/gpu/drm/drm_crtc_helper.c @@ -1055,3 +1055,39 @@ int drm_helper_force_disable_all(struct drm_device *dev) return ret; } EXPORT_SYMBOL(drm_helper_force_disable_all); + +/** + * drm_helper_crtc_select_output_bus_format - Select output media bus format + * @crtc: The CRTC to query + * @crtc_state: The new CRTC state + * @supported_fmts: List of media bus format options to pick from + * @num_supported_fmts: Number of media bus formats in @supported_fmts list + * + * Encoder drivers may call this helper to give the connected CRTC a chance to + * select compatible or preffered media bus format to use over the CRTC encoder + * link. Encoders should provide list of supported input MEDIA_BUS_FMT_* for + * CRTC to pick from. CRTC driver is expected to select preferred media bus + * format from the list and, once enabled, generate the signal accordingly. + * + * Returns: + * Selected preferred media bus format or 0 if CRTC does not support any from + * @supported_fmts list. + */ +u32 drm_helper_crtc_select_output_bus_format(struct drm_crtc *crtc, +struct drm_crtc_state *crtc_state, +const u32 *supported_fmts, +unsigned int num_supported_fmts) +{ + if (!crtc || !supported_fmts || !num_supported_fmts) + return 0; + + if (!crtc->helper_private || + !crtc->helper_private->select_output_bus_format) + return supported_fmts[0]; + + return crtc->helper_private->select_output_bus_format(crtc, + crtc_state, + supported_fmts, + num_supported_fmts); +} +EXPORT_SYMBOL(drm_helper_crtc_select_output_bus_format); diff --git a/include/drm/drm_crtc_helper.h b/include/drm/drm_crtc_helper.h index 8c886fc46ef2..b7eb52f3ce41 100644 --- a/include/drm/drm_crtc_helper.h +++ b/include/drm/drm_crtc_helper.h @@ -38,6 +38,7 @@ struct drm_atomic_state; struct drm_connector; struct drm_crtc; +struct drm_crtc_state;