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