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
> Sent: Monday, March 18, 2024 5:35 PM
> To: Klymenko, Anatoliy
> Cc: Maarten Lankhorst ; Maxime Ripard
> ; Thomas Zimmermann ; David
> Airlie ; Daniel Vetter ; Simek, Michal
> ; Andrzej Hajda ; Neil
> Armstrong ; Robert Foss ; Jonas
> Karlman ; Jernej Skrabec ; Rob
> Herring ; Krzysztof Kozlowski
> ; Conor Dooley ;
> Mauro Carvalho Chehab ; dri-
> de...@lists.freedesktop.org; linux-arm-ker...@lists.infradead.org; linux-
> ker...@vger.kernel.org; devicet...@vger.kernel.org; linux-
> me...@vger.kernel.org
> 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
> > ---
> > 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(laye
Re: [PATCH v2 5/8] drm: xlnx: zynqmp_dpsub: Set input live format
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
> ---
> 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 =
[PATCH v2 5/8] drm: xlnx: zynqmp_dpsub: Set input live format
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.
Update zynqmp_disp_layer_set_format() API to fit both live and non-live
layer types.
Signed-off-by: Anatoliy Klymenko
---
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,
.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,
+ },
+ };
+ 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;
+}
+
/**
*
