Hi Anatoliy,
Thank you for the patch.
On Tue, Mar 12, 2024 at 05:55:00PM -0700, Anatoliy Klymenko wrote:
> DPSUB in bridge mode supports multiple input media bus formats.
>
> Announce the list of supported input media bus formats via
> drm_bridge.atomic_get_input_bus_fmts callback.
> Introduce a set of live input formats, supported by DPSUB.
> Rename zynqmp_disp_layer_drm_formats() to zynqmp_disp_layer_formats() to
> reflect semantics for both live and non-live layer format lists.
>
> Signed-off-by: Anatoliy Klymenko <anatoliy.klyme...@amd.com>
> ---
> drivers/gpu/drm/xlnx/zynqmp_disp.c | 67
> +++++++++++++++++++++++++++++++++-----
> drivers/gpu/drm/xlnx/zynqmp_disp.h | 4 +--
> drivers/gpu/drm/xlnx/zynqmp_dp.c | 26 +++++++++++++++
> drivers/gpu/drm/xlnx/zynqmp_kms.c | 2 +-
> 4 files changed, 88 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c
> b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> index e6d26ef60e89..af851190f447 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> @@ -18,6 +18,7 @@
> #include <linux/dma/xilinx_dpdma.h>
> #include <linux/dma-mapping.h>
> #include <linux/dmaengine.h>
> +#include <linux/media-bus-format.h>
> #include <linux/module.h>
> #include <linux/of.h>
> #include <linux/platform_device.h>
> @@ -77,12 +78,16 @@ enum zynqmp_dpsub_layer_mode {
> /**
> * struct zynqmp_disp_format - Display subsystem format information
> * @drm_fmt: DRM format (4CC)
> + * @bus_fmt: Media bus format
> * @buf_fmt: AV buffer format
> * @swap: Flag to swap R & B for RGB formats, and U & V for YUV formats
> * @sf: Scaling factors for color components
> */
> struct zynqmp_disp_format {
> - u32 drm_fmt;
> + union {
> + u32 drm_fmt;
> + u32 bus_fmt;
> + };
I'm not a big fan of the union, but I can live with it.
> u32 buf_fmt;
> bool swap;
> const u32 *sf;
> @@ -182,6 +187,12 @@ static const u32 scaling_factors_565[] = {
> ZYNQMP_DISP_AV_BUF_5BIT_SF,
> };
>
> +static const u32 scaling_factors_666[] = {
> + ZYNQMP_DISP_AV_BUF_6BIT_SF,
> + ZYNQMP_DISP_AV_BUF_6BIT_SF,
> + ZYNQMP_DISP_AV_BUF_6BIT_SF,
> +};
> +
> static const u32 scaling_factors_888[] = {
> ZYNQMP_DISP_AV_BUF_8BIT_SF,
> ZYNQMP_DISP_AV_BUF_8BIT_SF,
> @@ -364,6 +375,36 @@ static const struct zynqmp_disp_format avbuf_gfx_fmts[]
> = {
> },
> };
>
> +/* List of live video layer formats */
> +static const struct zynqmp_disp_format avbuf_live_fmts[] = {
> + {
> + .bus_fmt = MEDIA_BUS_FMT_RGB666_1X18,
> + .buf_fmt = ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_6 |
> + ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_RGB,
> + .sf = scaling_factors_666,
> + }, {
> + .bus_fmt = MEDIA_BUS_FMT_UYVY8_1X24,
> + .buf_fmt = ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_8 |
> + ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_RGB,
> + .sf = scaling_factors_888,
> + }, {
> + .bus_fmt = MEDIA_BUS_FMT_UYVY8_1X16,
> + .buf_fmt = ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_8 |
> + ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV422,
> + .sf = scaling_factors_888,
> + }, {
> + .bus_fmt = MEDIA_BUS_FMT_VUY8_1X24,
> + .buf_fmt = ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_8 |
> + ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV444,
> + .sf = scaling_factors_888,
> + }, {
> + .bus_fmt = MEDIA_BUS_FMT_UYVY10_1X20,
> + .buf_fmt = ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_10 |
> + ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_YUV422,
> + .sf = scaling_factors_101010,
> + },
> +};
> +
> static u32 zynqmp_disp_avbuf_read(struct zynqmp_disp *disp, int reg)
> {
> return readl(disp->avbuf.base + reg);
> @@ -883,16 +924,17 @@ zynqmp_disp_layer_find_format(struct zynqmp_disp_layer
> *layer,
> }
>
> /**
> - * zynqmp_disp_layer_drm_formats - Return the DRM formats supported by the
> layer
> + * zynqmp_disp_layer_formats - Return DRM or media bus formats supported by
> + * the layer
> * @layer: The layer
> * @num_formats: Pointer to the returned number of formats
> *
> - * Return: A newly allocated u32 array that stores all the DRM formats
> + * Return: A newly allocated u32 array that stores all DRM or media bus
> formats
> * supported by the layer. The number of formats in the array is returned
> * through the num_formats argument.
> */
> -u32 *zynqmp_disp_layer_drm_formats(struct zynqmp_disp_layer *layer,
> - unsigned int *num_formats)
> +u32 *zynqmp_disp_layer_formats(struct zynqmp_disp_layer *layer,
> + unsigned int *num_formats)
> {
> unsigned int i;
> u32 *formats;
> @@ -1131,6 +1173,11 @@ static int zynqmp_disp_create_layers(struct
> zynqmp_disp *disp)
> .num_channels = 1,
> },
> };
> + static const struct zynqmp_disp_layer_info live_layer_info = {
> + .formats = avbuf_live_fmts,
> + .num_formats = ARRAY_SIZE(avbuf_live_fmts),
> + .num_channels = 0,
> + };
>
> unsigned int i;
> int ret;
> @@ -1140,12 +1187,16 @@ static int zynqmp_disp_create_layers(struct
> zynqmp_disp *disp)
>
> layer->id = i;
> layer->disp = disp;
> - layer->info = &layer_info[i];
> /* For now assume dpsub works in either live or non-live mode
> for both layers.
While are it, could you please turn this into
/*
* For now assume dpsub works in either live or non-live mode
for both layers.
with a blank line just above it ?
> * Hybrid mode is not supported yet.
> */
> - layer->mode = disp->dpsub->dma_enabled ?
> ZYNQMP_DPSUB_LAYER_NONLIVE
> - :
> ZYNQMP_DPSUB_LAYER_LIVE;
> + if (disp->dpsub->dma_enabled) {
> + layer->mode = ZYNQMP_DPSUB_LAYER_NONLIVE;
> + layer->info = &layer_info[i];
> + } else {
> + layer->mode = ZYNQMP_DPSUB_LAYER_LIVE;
> + layer->info = &live_layer_info;
> + }
>
> ret = zynqmp_disp_layer_request_dma(disp, layer);
> if (ret)
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.h
> b/drivers/gpu/drm/xlnx/zynqmp_disp.h
> index 9b8b202224d9..88c285a12e23 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_disp.h
> +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.h
> @@ -50,8 +50,8 @@ int zynqmp_disp_setup_clock(struct zynqmp_disp *disp,
> void zynqmp_disp_blend_set_global_alpha(struct zynqmp_disp *disp,
> bool enable, u32 alpha);
>
> -u32 *zynqmp_disp_layer_drm_formats(struct zynqmp_disp_layer *layer,
> - unsigned int *num_formats);
> +u32 *zynqmp_disp_layer_formats(struct zynqmp_disp_layer *layer,
> + unsigned int *num_formats);
> void zynqmp_disp_layer_enable(struct zynqmp_disp_layer *layer);
> void zynqmp_disp_layer_disable(struct zynqmp_disp_layer *layer);
> void zynqmp_disp_layer_set_format(struct zynqmp_disp_layer *layer,
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> index 04b6bcac3b07..a0d169ac48c0 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> @@ -1568,6 +1568,31 @@ static const struct drm_edid
> *zynqmp_dp_bridge_edid_read(struct drm_bridge *brid
> return drm_edid_read_ddc(connector, &dp->aux.ddc);
> }
>
> +static u32 *
> +zynqmp_dp_bridge_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 zynqmp_dp *dp = bridge_to_dp(bridge);
> + struct zynqmp_disp_layer *layer;
> + enum zynqmp_dpsub_layer_id layer_id;
> +
> + if (dp->dpsub->connected_ports & BIT(ZYNQMP_DPSUB_PORT_LIVE_VIDEO))
> + layer_id = ZYNQMP_DPSUB_LAYER_VID;
> + else if (dp->dpsub->connected_ports & BIT(ZYNQMP_DPSUB_PORT_LIVE_GFX))
> + layer_id = ZYNQMP_DPSUB_LAYER_GFX;
> + else {
> + *num_input_fmts = 0;
> + return NULL;
> + }
You need curly braces around all branches if one of them has multiple
statements.
Given that the above pattern is repeated twice already, a helper
function that returns the layer pointer would be useful. Then you could
simply write
layer = ...(dp);
if (!layer) {
*num_input_fmts = 0;
return NULL;
}
With these small issues addressed,
Reviewed-by: Laurent Pinchart <laurent.pinch...@ideasonboard.com>
> + layer = dp->dpsub->layers[layer_id];
> +
> + return zynqmp_disp_layer_formats(layer, num_input_fmts);
> +}
> +
> static const struct drm_bridge_funcs zynqmp_dp_bridge_funcs = {
> .attach = zynqmp_dp_bridge_attach,
> .detach = zynqmp_dp_bridge_detach,
> @@ -1580,6 +1605,7 @@ static const struct drm_bridge_funcs
> zynqmp_dp_bridge_funcs = {
> .atomic_check = zynqmp_dp_bridge_atomic_check,
> .detect = zynqmp_dp_bridge_detect,
> .edid_read = zynqmp_dp_bridge_edid_read,
> + .atomic_get_input_bus_fmts = zynqmp_dp_bridge_get_input_bus_fmts,
> };
>
> /*
> -----------------------------------------------------------------------------
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_kms.c
> b/drivers/gpu/drm/xlnx/zynqmp_kms.c
> index 43bf416b33d5..bf9fba01df0e 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_kms.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_kms.c
> @@ -152,7 +152,7 @@ static int zynqmp_dpsub_create_planes(struct zynqmp_dpsub
> *dpsub)
> unsigned int num_formats;
> u32 *formats;
>
> - formats = zynqmp_disp_layer_drm_formats(layer, &num_formats);
> + formats = zynqmp_disp_layer_formats(layer, &num_formats);
> if (!formats)
> return -ENOMEM;
>
>
--
Regards,
Laurent Pinchart
