| Message ID | 20220206154405.1243333-4-sam@ravnborg.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/bridge: ps8640 and ti-sn65dsi86 updates | expand |
Hi, On Sun, Feb 6, 2022 at 7:44 AM Sam Ravnborg <sam@ravnborg.org> wrote: > > There is a number of bridge drivers that supports a single media bus > format for DSI. Add a helper to avoid duplicating the code. > > Signed-off-by: Sam Ravnborg <sam@ravnborg.org> > --- > drivers/gpu/drm/drm_atomic_helper.c | 41 +++++++++++++++++++++++++++++ > include/drm/drm_atomic_helper.h | 7 +++++ > 2 files changed, 48 insertions(+) > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > index a7a05e1e26bb..94f313dc196f 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -3549,3 +3549,44 @@ drm_atomic_helper_bridge_propagate_bus_fmt(struct drm_bridge *bridge, > return input_fmts; > } > EXPORT_SYMBOL(drm_atomic_helper_bridge_propagate_bus_fmt); > + > +/** > + * drm_atomic_helper_bridge_dsi_input_bus_fmt - Define one DSI output format Is the description right? It's called "input" format but it defines an output format? > + * @bridge: bridge control structure > + * @bridge_state: new bridge state > + * @crtc_state: new CRTC state > + * @conn_state: new connector state > + * @output_fmt: tested output bus format > + * @num_input_fmts: will contain the size of the returned array Maybe indicate that it's always 1 in the comments? > + * > + * This helper is an implementation of the > + * &drm_bridge_funcs.atomic_get_input_bus_fmts operation for bridges that supports > + * a single DSI media bus format MEDIA_BUS_FMT_RGB888_1X24. > + * > + * RETURNS kernel-doc can't parse this return syntax and warns: warning: No description found for return value of 'drm_atomic_helper_bridge_dsi_input_bus_fmt' > + * A format array with one entry containing MEDIA_BUS_FMT_RGB888_1X24, > + * or NULL if the allocation failed > + */ > +u32 * > +drm_atomic_helper_bridge_dsi_input_bus_fmt(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; > + > + *num_input_fmts = 0; > + > + input_fmts = kcalloc(MAX_INPUT_SEL_FORMATS, sizeof(*input_fmts), GFP_KERNEL); I probably wouldn't have bothered with `kcalloc` for something that's always just one value and you're setting it. Why not just input_fmts = kmalloc(sizeof(*input_fmts), GFP_KERNEL); ...also MAX_INPUT_SEL_FORMATS isn't defined. I guess that's why you said it didn't compile? Also: if it was common for others to want to provide fixed formats, I wonder about adding a helper function that did most of the work here? Dunno what it would be named since it's already a bit a of handful, but I'd expect to call it like: static const u32 formats[] = { MEDIA_BUS_FMT_RGB888_1X24 }; return my_neat_helper(formats, ARRAY_SIZE(formats), num_output_formats) Then my_neat_helper() could do kmemdup() on the array passed and fill in "num_output_formats" to be either the array size of 0 (if the kmemdup failed). > + if (!input_fmts) > + return NULL; > + > + /* This is the DSI-end bus format */ > + input_fmts[0] = MEDIA_BUS_FMT_RGB888_1X24; I'm not an expert, but I'm curious. Can't DSI run in other formats? ...or maybe I'm misunderstanding what this is for. I guess I'm not sure how it relates to: enum mipi_dsi_pixel_format { MIPI_DSI_FMT_RGB888, MIPI_DSI_FMT_RGB666, MIPI_DSI_FMT_RGB666_PACKED, MIPI_DSI_FMT_RGB565, }; -Doug
Hello, On Mon, Feb 07, 2022 at 02:32:45PM -0800, Doug Anderson wrote: > On Sun, Feb 6, 2022 at 7:44 AM Sam Ravnborg <sam@ravnborg.org> wrote: > > > > There is a number of bridge drivers that supports a single media bus > > format for DSI. Add a helper to avoid duplicating the code. > > > > Signed-off-by: Sam Ravnborg <sam@ravnborg.org> > > --- > > drivers/gpu/drm/drm_atomic_helper.c | 41 +++++++++++++++++++++++++++++ > > include/drm/drm_atomic_helper.h | 7 +++++ > > 2 files changed, 48 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > > index a7a05e1e26bb..94f313dc196f 100644 > > --- a/drivers/gpu/drm/drm_atomic_helper.c > > +++ b/drivers/gpu/drm/drm_atomic_helper.c > > @@ -3549,3 +3549,44 @@ drm_atomic_helper_bridge_propagate_bus_fmt(struct drm_bridge *bridge, > > return input_fmts; > > } > > EXPORT_SYMBOL(drm_atomic_helper_bridge_propagate_bus_fmt); > > + > > +/** > > + * drm_atomic_helper_bridge_dsi_input_bus_fmt - Define one DSI output format > > Is the description right? It's called "input" format but it defines an > output format? > > > > + * @bridge: bridge control structure > > + * @bridge_state: new bridge state > > + * @crtc_state: new CRTC state > > + * @conn_state: new connector state > > + * @output_fmt: tested output bus format > > + * @num_input_fmts: will contain the size of the returned array > > Maybe indicate that it's always 1 in the comments? > > > + * > > + * This helper is an implementation of the > > + * &drm_bridge_funcs.atomic_get_input_bus_fmts operation for bridges that supports > > + * a single DSI media bus format MEDIA_BUS_FMT_RGB888_1X24. > > + * > > + * RETURNS > > kernel-doc can't parse this return syntax and warns: > > warning: No description found for return value of > 'drm_atomic_helper_bridge_dsi_input_bus_fmt' > > > > + * A format array with one entry containing MEDIA_BUS_FMT_RGB888_1X24, > > + * or NULL if the allocation failed > > + */ > > +u32 * > > +drm_atomic_helper_bridge_dsi_input_bus_fmt(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; > > + > > + *num_input_fmts = 0; > > + > > + input_fmts = kcalloc(MAX_INPUT_SEL_FORMATS, sizeof(*input_fmts), GFP_KERNEL); > > I probably wouldn't have bothered with `kcalloc` for something that's > always just one value and you're setting it. Why not just > > input_fmts = kmalloc(sizeof(*input_fmts), GFP_KERNEL); > > ...also MAX_INPUT_SEL_FORMATS isn't defined. I guess that's why you > said it didn't compile? > > Also: if it was common for others to want to provide fixed formats, I > wonder about adding a helper function that did most of the work here? > Dunno what it would be named since it's already a bit a of handful, > but I'd expect to call it like: > > static const u32 formats[] = { MEDIA_BUS_FMT_RGB888_1X24 }; > return my_neat_helper(formats, ARRAY_SIZE(formats), num_output_formats) > > Then my_neat_helper() could do kmemdup() on the array passed and fill > in "num_output_formats" to be either the array size of 0 (if the > kmemdup failed). I quite like that approach. We could even have a wrapper macro that adds the ARRAY_SIZE() argument automatically. > > + if (!input_fmts) > > + return NULL; > > + > > + /* This is the DSI-end bus format */ > > + input_fmts[0] = MEDIA_BUS_FMT_RGB888_1X24; > > I'm not an expert, but I'm curious. Can't DSI run in other formats? > ...or maybe I'm misunderstanding what this is for. I guess I'm not > sure how it relates to: > > enum mipi_dsi_pixel_format { > MIPI_DSI_FMT_RGB888, > MIPI_DSI_FMT_RGB666, > MIPI_DSI_FMT_RGB666_PACKED, > MIPI_DSI_FMT_RGB565, > };
On Tue, Feb 08, 2022 at 02:44:25AM +0200, Laurent Pinchart wrote: > Hello, > > On Mon, Feb 07, 2022 at 02:32:45PM -0800, Doug Anderson wrote: > > On Sun, Feb 6, 2022 at 7:44 AM Sam Ravnborg <sam@ravnborg.org> wrote: > > > > > > There is a number of bridge drivers that supports a single media bus > > > format for DSI. Add a helper to avoid duplicating the code. > > > > > > Signed-off-by: Sam Ravnborg <sam@ravnborg.org> > > > --- > > > drivers/gpu/drm/drm_atomic_helper.c | 41 +++++++++++++++++++++++++++++ > > > include/drm/drm_atomic_helper.h | 7 +++++ > > > 2 files changed, 48 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > > > index a7a05e1e26bb..94f313dc196f 100644 > > > --- a/drivers/gpu/drm/drm_atomic_helper.c > > > +++ b/drivers/gpu/drm/drm_atomic_helper.c > > > @@ -3549,3 +3549,44 @@ drm_atomic_helper_bridge_propagate_bus_fmt(struct drm_bridge *bridge, > > > return input_fmts; > > > } > > > EXPORT_SYMBOL(drm_atomic_helper_bridge_propagate_bus_fmt); > > > + > > > +/** > > > + * drm_atomic_helper_bridge_dsi_input_bus_fmt - Define one DSI output format > > > > Is the description right? It's called "input" format but it defines an > > output format? > > > > > > > + * @bridge: bridge control structure > > > + * @bridge_state: new bridge state > > > + * @crtc_state: new CRTC state > > > + * @conn_state: new connector state > > > + * @output_fmt: tested output bus format > > > + * @num_input_fmts: will contain the size of the returned array > > > > Maybe indicate that it's always 1 in the comments? > > > > > + * > > > + * This helper is an implementation of the > > > + * &drm_bridge_funcs.atomic_get_input_bus_fmts operation for bridges that supports > > > + * a single DSI media bus format MEDIA_BUS_FMT_RGB888_1X24. > > > + * > > > + * RETURNS > > > > kernel-doc can't parse this return syntax and warns: > > > > warning: No description found for return value of > > 'drm_atomic_helper_bridge_dsi_input_bus_fmt' > > > > > > > + * A format array with one entry containing MEDIA_BUS_FMT_RGB888_1X24, > > > + * or NULL if the allocation failed > > > + */ > > > +u32 * > > > +drm_atomic_helper_bridge_dsi_input_bus_fmt(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; > > > + > > > + *num_input_fmts = 0; > > > + > > > + input_fmts = kcalloc(MAX_INPUT_SEL_FORMATS, sizeof(*input_fmts), GFP_KERNEL); > > > > I probably wouldn't have bothered with `kcalloc` for something that's > > always just one value and you're setting it. Why not just > > > > input_fmts = kmalloc(sizeof(*input_fmts), GFP_KERNEL); > > > > ...also MAX_INPUT_SEL_FORMATS isn't defined. I guess that's why you > > said it didn't compile? > > > > Also: if it was common for others to want to provide fixed formats, I > > wonder about adding a helper function that did most of the work here? > > Dunno what it would be named since it's already a bit a of handful, > > but I'd expect to call it like: > > > > static const u32 formats[] = { MEDIA_BUS_FMT_RGB888_1X24 }; > > return my_neat_helper(formats, ARRAY_SIZE(formats), num_output_formats) > > > > Then my_neat_helper() could do kmemdup() on the array passed and fill > > in "num_output_formats" to be either the array size of 0 (if the > > kmemdup failed). > > I quite like that approach. We could even have a wrapper macro that adds > the ARRAY_SIZE() argument automatically. Agree, will try to give that a spin. And will then also process all the nice feedback from Douglas. Sam
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index a7a05e1e26bb..94f313dc196f 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -3549,3 +3549,44 @@ drm_atomic_helper_bridge_propagate_bus_fmt(struct drm_bridge *bridge, return input_fmts; } EXPORT_SYMBOL(drm_atomic_helper_bridge_propagate_bus_fmt); + +/** + * drm_atomic_helper_bridge_dsi_input_bus_fmt - Define one DSI output format + * + * @bridge: bridge control structure + * @bridge_state: new bridge state + * @crtc_state: new CRTC state + * @conn_state: new connector state + * @output_fmt: tested output bus format + * @num_input_fmts: will contain the size of the returned array + * + * This helper is an implementation of the + * &drm_bridge_funcs.atomic_get_input_bus_fmts operation for bridges that supports + * a single DSI media bus format MEDIA_BUS_FMT_RGB888_1X24. + * + * RETURNS + * A format array with one entry containing MEDIA_BUS_FMT_RGB888_1X24, + * or NULL if the allocation failed + */ +u32 * +drm_atomic_helper_bridge_dsi_input_bus_fmt(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; + + *num_input_fmts = 0; + + input_fmts = kcalloc(MAX_INPUT_SEL_FORMATS, sizeof(*input_fmts), GFP_KERNEL); + if (!input_fmts) + return NULL; + + /* This is the DSI-end bus format */ + input_fmts[0] = MEDIA_BUS_FMT_RGB888_1X24; + *num_input_fmts = 1; + + return input_fmts; +} diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h index 4045e2507e11..0e81b6f637d6 100644 --- a/include/drm/drm_atomic_helper.h +++ b/include/drm/drm_atomic_helper.h @@ -231,4 +231,11 @@ drm_atomic_helper_bridge_propagate_bus_fmt(struct drm_bridge *bridge, u32 output_fmt, unsigned int *num_input_fmts); +u32 * +drm_atomic_helper_bridge_dsi_input_bus_fmt(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); #endif /* DRM_ATOMIC_HELPER_H_ */
There is a number of bridge drivers that supports a single media bus format for DSI. Add a helper to avoid duplicating the code. Signed-off-by: Sam Ravnborg <sam@ravnborg.org> --- drivers/gpu/drm/drm_atomic_helper.c | 41 +++++++++++++++++++++++++++++ include/drm/drm_atomic_helper.h | 7 +++++ 2 files changed, 48 insertions(+)