[PATCH v1 3/9] drm: add drm_atomic_helper_bridge_dsi_input_bus_fmt

[Doug Anderson]

Hi,

On Sun, Feb 6, 2022 at 7:44 AM Sam Ravnborg <sam at 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 at 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