Hi Anatoliy,
Thank you for the patch.
On Tue, Mar 12, 2024 at 05:55:02PM -0700, Anatoliy Klymenko wrote:
> Program live video input format according to selected media bus format.
>
> In the bridge mode of operation, DPSUB is connected to FPGA CRTC which
> almost certainly supports a single media bus format as its output. Expect
> this to be delivered via the new bridge atomic state. Program DPSUB
> registers accordingly.
No line breaks within paragraphs. Add a blank line if you want to
paragraphs, remove the line break otherwise.
> Update zynqmp_disp_layer_set_format() API to fit both live and non-live
> layer types.
>
> Signed-off-by: Anatoliy Klymenko <anatoliy.klymenko@xxxxxxx>
> ---
> drivers/gpu/drm/xlnx/zynqmp_disp.c | 93 +++++++++++++++++++++++++++++++-------
> drivers/gpu/drm/xlnx/zynqmp_disp.h | 2 +-
> drivers/gpu/drm/xlnx/zynqmp_dp.c | 13 ++++--
> drivers/gpu/drm/xlnx/zynqmp_kms.c | 2 +-
> 4 files changed, 87 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> index dd48fa60fa9a..0cacd597f4b8 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> @@ -383,7 +383,7 @@ static const struct zynqmp_disp_format avbuf_live_fmts[] = {
> ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_RGB,
> .sf = scaling_factors_666,
> }, {
> - .bus_fmt = MEDIA_BUS_FMT_UYVY8_1X24,
> + .bus_fmt = MEDIA_BUS_FMT_RBG888_1X24,
Does this belong to a previous patch ?
> .buf_fmt = ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_BPC_8 |
> ZYNQMP_DISP_AV_BUF_LIVE_CONFIG_FMT_RGB,
> .sf = scaling_factors_888,
> @@ -433,19 +433,28 @@ static void zynqmp_disp_avbuf_set_format(struct zynqmp_disp *disp,
> const struct zynqmp_disp_format *fmt)
> {
> unsigned int i;
> - u32 val;
> + u32 val, reg;
>
> - val = zynqmp_disp_avbuf_read(disp, ZYNQMP_DISP_AV_BUF_FMT);
> - val &= zynqmp_disp_layer_is_video(layer)
> - ? ~ZYNQMP_DISP_AV_BUF_FMT_NL_VID_MASK
> - : ~ZYNQMP_DISP_AV_BUF_FMT_NL_GFX_MASK;
> - val |= fmt->buf_fmt;
> - zynqmp_disp_avbuf_write(disp, ZYNQMP_DISP_AV_BUF_FMT, val);
> + layer->disp_fmt = fmt;
> + if (layer->mode == ZYNQMP_DPSUB_LAYER_NONLIVE) {
> + reg = ZYNQMP_DISP_AV_BUF_FMT;
> + val = zynqmp_disp_avbuf_read(disp, ZYNQMP_DISP_AV_BUF_FMT);
> + val &= zynqmp_disp_layer_is_video(layer)
> + ? ~ZYNQMP_DISP_AV_BUF_FMT_NL_VID_MASK
> + : ~ZYNQMP_DISP_AV_BUF_FMT_NL_GFX_MASK;
> + val |= fmt->buf_fmt;
> + } else {
> + reg = zynqmp_disp_layer_is_video(layer)
> + ? ZYNQMP_DISP_AV_BUF_LIVE_VID_CONFIG
> + : ZYNQMP_DISP_AV_BUF_LIVE_GFX_CONFIG;
> + val = fmt->buf_fmt;
> + }
> + zynqmp_disp_avbuf_write(disp, reg, val);
>
> for (i = 0; i < ZYNQMP_DISP_AV_BUF_NUM_SF; i++) {
> - unsigned int reg = zynqmp_disp_layer_is_video(layer)
> - ? ZYNQMP_DISP_AV_BUF_VID_COMP_SF(i)
> - : ZYNQMP_DISP_AV_BUF_GFX_COMP_SF(i);
> + reg = zynqmp_disp_layer_is_video(layer)
> + ? ZYNQMP_DISP_AV_BUF_VID_COMP_SF(i)
> + : ZYNQMP_DISP_AV_BUF_GFX_COMP_SF(i);
>
> zynqmp_disp_avbuf_write(disp, reg, fmt->sf[i]);
> }
> @@ -984,23 +993,73 @@ void zynqmp_disp_layer_disable(struct zynqmp_disp_layer *layer)
> zynqmp_disp_blend_layer_disable(layer->disp, layer);
> }
>
> +struct zynqmp_disp_bus_to_drm {
> + u32 bus_fmt;
> + u32 drm_fmt;
> +};
> +
> +/**
> + * zynqmp_disp_reference_drm_format - Get reference DRM format corresponding
> + * to the given media bus format.
> + * @bus_format: Media bus format
> + *
> + * Map media bus format to some DRM format that represents the same color space
> + * and chroma subsampling as the @bus_format video signal. These DRM format
> + * properties are required to program the blender.
> + *
> + * Return: DRM format code corresponding to @bus_format
> + */
> +static u32 zynqmp_disp_reference_drm_format(u32 bus_format)
> +{
> + static const struct zynqmp_disp_bus_to_drm format_map[] = {
> + {
> + .bus_fmt = MEDIA_BUS_FMT_RGB666_1X18,
> + .drm_fmt = DRM_FORMAT_RGB565,
> + }, {
> + .bus_fmt = MEDIA_BUS_FMT_RBG888_1X24,
> + .drm_fmt = DRM_FORMAT_RGB888,
> + }, {
> + .bus_fmt = MEDIA_BUS_FMT_UYVY8_1X16,
> + .drm_fmt = DRM_FORMAT_YUV422,
> + }, {
> + .bus_fmt = MEDIA_BUS_FMT_VUY8_1X24,
> + .drm_fmt = DRM_FORMAT_YUV444,
> + }, {
> + .bus_fmt = MEDIA_BUS_FMT_UYVY10_1X20,
> + .drm_fmt = DRM_FORMAT_P210,
> + },
Hmmmm... Looking at this, I think you should have both bus_fmt and
drm_fmt in zynqmp_disp_format. It seems it would simplify the code flow
and make things more readable. If you would like me to give it a try,
please let me know.
> + };
> + unsigned int i;
> +
> + for (i = 0; i < ARRAY_SIZE(format_map); ++i)
> + if (format_map[i].bus_fmt == bus_format)
> + return format_map[i].drm_fmt;
> +
> + return DRM_FORMAT_INVALID;
> +}
> +
> /**
> * zynqmp_disp_layer_set_format - Set the layer format
> * @layer: The layer
> - * @info: The format info
> + * @drm_or_bus_format: DRM or media bus format
> *
> * Set the format for @layer to @info. The layer must be disabled.
> */
> void zynqmp_disp_layer_set_format(struct zynqmp_disp_layer *layer,
> - const struct drm_format_info *info)
> + u32 drm_or_bus_format)
> {
> unsigned int i;
>
> - layer->disp_fmt = zynqmp_disp_layer_find_format(layer, info->format);
> - layer->drm_fmt = info;
> -
> + layer->disp_fmt = zynqmp_disp_layer_find_format(layer, drm_or_bus_format);
> zynqmp_disp_avbuf_set_format(layer->disp, layer, layer->disp_fmt);
>
> + if (layer->mode == ZYNQMP_DPSUB_LAYER_LIVE)
> + drm_or_bus_format = zynqmp_disp_reference_drm_format(drm_or_bus_format);
> +
> + layer->drm_fmt = drm_format_info(drm_or_bus_format);
> + if (!layer->drm_fmt)
> + return;
> +
> if (layer->mode == ZYNQMP_DPSUB_LAYER_LIVE)
> return;
>
> @@ -1008,7 +1067,7 @@ void zynqmp_disp_layer_set_format(struct zynqmp_disp_layer *layer,
> * Set pconfig for each DMA channel to indicate they're part of a
> * video group.
> */
> - for (i = 0; i < info->num_planes; i++) {
> + for (i = 0; i < layer->drm_fmt->num_planes; i++) {
> struct zynqmp_disp_layer_dma *dma = &layer->dmas[i];
> struct xilinx_dpdma_peripheral_config pconfig = {
> .video_group = true,
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.h b/drivers/gpu/drm/xlnx/zynqmp_disp.h
> index 88c285a12e23..9f9a5f50ffbc 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_disp.h
> +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.h
> @@ -55,7 +55,7 @@ u32 *zynqmp_disp_layer_formats(struct zynqmp_disp_layer *layer,
> 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,
> - const struct drm_format_info *info);
> + u32 drm_or_bus_format);
> int zynqmp_disp_layer_update(struct zynqmp_disp_layer *layer,
> struct drm_plane_state *state);
>
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> index a0d169ac48c0..fc6b1d783c28 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> @@ -1281,7 +1281,8 @@ static void zynqmp_dp_disp_enable(struct zynqmp_dp *dp,
> {
> enum zynqmp_dpsub_layer_id layer_id;
> struct zynqmp_disp_layer *layer;
> - const struct drm_format_info *info;
> + struct drm_bridge_state *bridge_state;
> + u32 bus_fmt;
>
> if (dp->dpsub->connected_ports & BIT(ZYNQMP_DPSUB_PORT_LIVE_VIDEO))
> layer_id = ZYNQMP_DPSUB_LAYER_VID;
> @@ -1291,10 +1292,14 @@ static void zynqmp_dp_disp_enable(struct zynqmp_dp *dp,
> return;
>
> layer = dp->dpsub->layers[layer_id];
> + bridge_state = drm_atomic_get_new_bridge_state(old_bridge_state->base.state,
> + old_bridge_state->bridge);
> + if (WARN_ON(!bridge_state))
> + return;
> +
> + bus_fmt = bridge_state->input_bus_cfg.format;
> + zynqmp_disp_layer_set_format(layer, bus_fmt);
>
> - /* TODO: Make the format configurable. */
> - info = drm_format_info(DRM_FORMAT_YUV422);
> - zynqmp_disp_layer_set_format(layer, info);
> zynqmp_disp_layer_enable(layer);
>
> if (layer_id == ZYNQMP_DPSUB_LAYER_GFX)
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_kms.c b/drivers/gpu/drm/xlnx/zynqmp_kms.c
> index bf9fba01df0e..d96b3f3f2e3a 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_kms.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_kms.c
> @@ -111,7 +111,7 @@ static void zynqmp_dpsub_plane_atomic_update(struct drm_plane *plane,
> if (old_state->fb)
> zynqmp_disp_layer_disable(layer);
>
> - zynqmp_disp_layer_set_format(layer, new_state->fb->format);
> + zynqmp_disp_layer_set_format(layer, new_state->fb->format->format);
> }
>
> zynqmp_disp_layer_update(layer, new_state);
>
--
Regards,
Laurent Pinchart
