by Krzysztof Kozlowski

[permalink] [raw]

Subject: Re: [PATCH v2 7/8] dt-bindings: xlnx: Add VTC and TPG bindings

On 13/03/2024 01:55, Anatoliy Klymenko wrote:
> DO NOT MERGE. REFERENCE ONLY.

Please add it to the PATCH title, so [PATCH DO NOT MERGE]... but better
just don't send it in one thread. Many maintainers apply entire thread,
thus whatever you send here, would be applied.

I am not going to review this patch, because as you said, should not be
merged, so just for formality:

NAK.

Best regards,
Krzysztof

2024-03-14 12:05:22

[permalink] [raw]

Subject: RE: [PATCH v2 4/8] drm: xlnx: zynqmp_dpsub: Minimize usage of global flag

Hi Laurent,

Thank you for the review.

> -----Original Message-----
> From: Laurent Pinchart <[email protected]>
> Sent: Monday, March 18, 2024 5:13 PM
> To: Klymenko, Anatoliy <[email protected]>
> Cc: Maarten Lankhorst <[email protected]>; Maxime Ripard
> <[email protected]>; Thomas Zimmermann <[email protected]>; David
> Airlie <[email protected]>; Daniel Vetter <[email protected]>; Simek, Michal
> <[email protected]>; Andrzej Hajda <[email protected]>; Neil
> Armstrong <[email protected]>; Robert Foss <[email protected]>; Jonas
> Karlman <[email protected]>; Jernej Skrabec <[email protected]>; Rob
> Herring <[email protected]>; Krzysztof Kozlowski
> <[email protected]>; Conor Dooley <[email protected]>;
> Mauro Carvalho Chehab <[email protected]>; dri-
> [email protected]; [email protected]; linux-
> [email protected]; [email protected]; linux-
> [email protected]
> Subject: Re: [PATCH v2 4/8] drm: xlnx: zynqmp_dpsub: Minimize usage of global
> flag
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> Hi Anatoliy,
>
> Thank you for the patch.
>
> On Tue, Mar 12, 2024 at 05:55:01PM -0700, Anatoliy Klymenko wrote:
> > Avoid usage of global zynqmp_dpsub.dma_enabled flag in DPSUB layer
> > context. This flag signals whether the driver runs in DRM CRTC or DRM
> > bridge mode, assuming that all display layers share the same live or
> > non-live mode of operation. Using per-layer mode instead of global
> > flag will siplify future support of the hybrid scenario.
>
> s/siplify/simplify/
>
> > Signed-off-by: Anatoliy Klymenko <[email protected]>
> > ---
> > drivers/gpu/drm/xlnx/zynqmp_disp.c | 11 ++++-------
> > 1 file changed, 4 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c
> > b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> > index af851190f447..dd48fa60fa9a 100644
> > --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
> > +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> > @@ -975,7 +975,7 @@ void zynqmp_disp_layer_disable(struct
> > zynqmp_disp_layer *layer) {
> > unsigned int i;
> >
> > - if (layer->disp->dpsub->dma_enabled) {
> > + if (layer->mode == ZYNQMP_DPSUB_LAYER_NONLIVE) {
> > for (i = 0; i < layer->drm_fmt->num_planes; i++)
> > dmaengine_terminate_sync(layer->dmas[i].chan);
> > }
> > @@ -1001,7 +1001,7 @@ void zynqmp_disp_layer_set_format(struct
> > zynqmp_disp_layer *layer,
> >
> > zynqmp_disp_avbuf_set_format(layer->disp, layer,
> > layer->disp_fmt);
> >
> > - if (!layer->disp->dpsub->dma_enabled)
> > + if (layer->mode == ZYNQMP_DPSUB_LAYER_LIVE)
> > return;
> >
> > /*
> > @@ -1039,7 +1039,7 @@ int zynqmp_disp_layer_update(struct
> zynqmp_disp_layer *layer,
> > const struct drm_format_info *info = layer->drm_fmt;
> > unsigned int i;
> >
> > - if (!layer->disp->dpsub->dma_enabled)
> > + if (layer->mode == ZYNQMP_DPSUB_LAYER_LIVE)
> > return 0;
>
> The above changes look nice.
>
> >
> > for (i = 0; i < info->num_planes; i++) { @@ -1089,7 +1089,7 @@
> > static void zynqmp_disp_layer_release_dma(struct zynqmp_disp *disp, {
> > unsigned int i;
> >
> > - if (!layer->info || !disp->dpsub->dma_enabled)
> > + if (!layer->info)
>
> This, however, doesn't seem right, as this function is called unconditionally from
> the remove path. The change below seems weird too.
> If I'm missing something, it should at least be explained in the commit message.
>

Actually, this whole condition should be removed, as now we're setting layer.info for all types of layers. On top of that, we're setting the number of DMA channels to zero for the live layers, which in turn prevents any DMA channel initialization or release. You are right - that probably should be mentioned explicitly in the commit message. I'll update it.

> > return;
> >
> > for (i = 0; i < layer->info->num_channels; i++) { @@ -1132,9
> > +1132,6 @@ static int zynqmp_disp_layer_request_dma(struct zynqmp_disp
> *disp,
> > unsigned int i;
> > int ret;
> >
> > - if (!disp->dpsub->dma_enabled)
> > - return 0;
> > -
> > for (i = 0; i < layer->info->num_channels; i++) {
> > struct zynqmp_disp_layer_dma *dma = &layer->dmas[i];
> > char dma_channel_name[16];
> >
>
> --
> Regards,
>
> Laurent Pinchart

Thank you,
Anatoliy

2024-03-20 01:25:57

by Anatoliy Klymenko

[permalink] [raw]

Subject: RE: [PATCH v2 5/8] drm: xlnx: zynqmp_dpsub: Set input live format

Hi Laurent,

Thanks a lot for your review.

> -----Original Message-----
> From: Laurent Pinchart <[email protected]>
> Sent: Monday, March 18, 2024 5:35 PM
> To: Klymenko, Anatoliy <[email protected]>
> Cc: Maarten Lankhorst <[email protected]>; Maxime Ripard
> <[email protected]>; Thomas Zimmermann <[email protected]>; David
> Airlie <[email protected]>; Daniel Vetter <[email protected]>; Simek, Michal
> <[email protected]>; Andrzej Hajda <[email protected]>; Neil
> Armstrong <[email protected]>; Robert Foss <[email protected]>; Jonas
> Karlman <[email protected]>; Jernej Skrabec <[email protected]>; Rob
> Herring <[email protected]>; Krzysztof Kozlowski
> <[email protected]>; Conor Dooley <[email protected]>;
> Mauro Carvalho Chehab <[email protected]>; dri-
> [email protected]; [email protected]; linux-
> [email protected]; [email protected]; linux-
> [email protected]
> Subject: Re: [PATCH v2 5/8] drm: xlnx: zynqmp_dpsub: Set input live format
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> 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.
>

Got it - will fix. Thank you.

> > Update zynqmp_disp_layer_set_format() API to fit both live and
> > non-live layer types.
> >
> > Signed-off-by: Anatoliy Klymenko <[email protected]>
> > ---
> > 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 ?

Yep, slipped between my fingers during the rebase. I will update this in the next version.

>
> > .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.
>

I was trying to preserve symmetry in struct zynqmp_disp_format, having only relevant information there for both layer modes, but this ended up in this ugly lookup. Probably the proper fix would be adding blender register values to this struct, but that would be a rather big change. So, I think your proposal is a good compromise. This will require a few layer.mode checks though. Thank you.

> > + };
> > + 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

Thank you,
Anatoliy