Re: [PATCH 1/3] drm/mediatek: Refactor pixel format logic
On 02/02/2023 21:41, Justin Green wrote: Yes, I had a comment on the naming in that patch. Never the less, I think if we don't need to "overwrite" the value, we should use just one struct for the values instead of copying them to the different .c files and give them SoC specific names. I don't have a very strong opinion about this, and in fact that is how v1 of the patch worked, but Chun-Kuang specifically suggested moving that struct into the .c files a few versions back. I think it makes sense if we expect additional skew between the different components and what pixel formats they support. Ok, if Chun-Kuang asked to do it this way, then I won't object. In the end he is the maintainer of the driver. Regards, Matthias
Re: [PATCH 1/3] drm/mediatek: Refactor pixel format logic
> Yes, I had a comment on the naming in that patch. Never the less, I think if > we > don't need to "overwrite" the value, we should use just one struct for the > values instead of copying them to the different .c files and give them SoC > specific names. I don't have a very strong opinion about this, and in fact that is how v1 of the patch worked, but Chun-Kuang specifically suggested moving that struct into the .c files a few versions back. I think it makes sense if we expect additional skew between the different components and what pixel formats they support.
Re: [PATCH 1/3] drm/mediatek: Refactor pixel format logic
On 02/02/2023 19:59, Justin Green wrote: Hi Matthias, mt8173_formats are the same as the old struct formats. Maybe we should use that and only overwrite where we actually use a different array. I think this was sort of how the original patch worked, but we wanted to add some flexibility to allow different components to support different formats. In patch 3 of the series, we actually overwrite this field with mt8195_formats. Yes, I had a comment on the naming in that patch. Never the less, I think if we don't need to "overwrite" the value, we should use just one struct for the values instead of copying them to the different .c files and give them SoC specific names. Why can't we use ARRAY_SIZE(formats) here like we did before? I think ARRAY_SIZE is just a macro for getting the length of statically allocated arrays. Because we won't know until runtime which list of pixel formats we will be using, I'm not sure we can use that in this circumstance? You are probably right. Regards, Matthias
Re: [PATCH 1/3] drm/mediatek: Refactor pixel format logic
Hi Matthias, > mt8173_formats are the same as the old struct formats. Maybe we should use > that > and only overwrite where we actually use a different array. I think this was sort of how the original patch worked, but we wanted to add some flexibility to allow different components to support different formats. In patch 3 of the series, we actually overwrite this field with mt8195_formats. > Why can't we use ARRAY_SIZE(formats) here like we did before? I think ARRAY_SIZE is just a macro for getting the length of statically allocated arrays. Because we won't know until runtime which list of pixel formats we will be using, I'm not sure we can use that in this circumstance? Regards, Justin
Re: [PATCH 1/3] drm/mediatek: Refactor pixel format logic
On 01/02/2023 18:02, Justin Green wrote:
Add an DDP component interface for querying pixel format support and move list
of supported pixel formats into DDP components instead of mtk_drm_plane.c
Tested by running Chrome on an MT8195.
Signed-off-by: Justin Green
---
drivers/gpu/drm/mediatek/mtk_disp_drv.h | 4 ++
drivers/gpu/drm/mediatek/mtk_disp_ovl.c | 44 +
drivers/gpu/drm/mediatek/mtk_disp_rdma.c| 38 ++
drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 4 +-
drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c | 4 ++
drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h | 20 ++
drivers/gpu/drm/mediatek/mtk_drm_plane.c| 24 ---
drivers/gpu/drm/mediatek/mtk_drm_plane.h| 3 +-
8 files changed, 123 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/mediatek/mtk_disp_drv.h
b/drivers/gpu/drm/mediatek/mtk_disp_drv.h
index 33e61a136bbc..0df6a06defb8 100644
--- a/drivers/gpu/drm/mediatek/mtk_disp_drv.h
+++ b/drivers/gpu/drm/mediatek/mtk_disp_drv.h
@@ -96,6 +96,8 @@ void mtk_ovl_register_vblank_cb(struct device *dev,
void mtk_ovl_unregister_vblank_cb(struct device *dev);
void mtk_ovl_enable_vblank(struct device *dev);
void mtk_ovl_disable_vblank(struct device *dev);
+const u32 *mtk_ovl_get_formats(struct device *dev);
+size_t mtk_ovl_get_num_formats(struct device *dev);
void mtk_rdma_bypass_shadow(struct device *dev);
int mtk_rdma_clk_enable(struct device *dev);
@@ -115,6 +117,8 @@ void mtk_rdma_register_vblank_cb(struct device *dev,
void mtk_rdma_unregister_vblank_cb(struct device *dev);
void mtk_rdma_enable_vblank(struct device *dev);
void mtk_rdma_disable_vblank(struct device *dev);
+const u32 *mtk_rdma_get_formats(struct device *dev);
+size_t mtk_rdma_get_num_formats(struct device *dev);
int mtk_mdp_rdma_clk_enable(struct device *dev);
void mtk_mdp_rdma_clk_disable(struct device *dev);
diff --git a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
index 84daeaffab6a..8743c8047dc9 100644
--- a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
+++ b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
@@ -66,6 +66,20 @@
#define OVL_CON_VIRT_FLIP BIT(9)
#define OVL_CON_HORZ_FLIP BIT(10)
+static const u32 mt8173_formats[] = {
+ DRM_FORMAT_XRGB,
+ DRM_FORMAT_ARGB,
+ DRM_FORMAT_BGRX,
+ DRM_FORMAT_BGRA,
+ DRM_FORMAT_ABGR,
+ DRM_FORMAT_XBGR,
+ DRM_FORMAT_RGB888,
+ DRM_FORMAT_BGR888,
+ DRM_FORMAT_RGB565,
+ DRM_FORMAT_UYVY,
+ DRM_FORMAT_YUYV,
+};
+
struct mtk_disp_ovl_data {
unsigned int addr;
unsigned int gmc_bits;
@@ -73,6 +87,8 @@ struct mtk_disp_ovl_data {
bool fmt_rgb565_is_0;
bool smi_id_en;
bool supports_afbc;
+ const u32 *formats;
+ size_t num_formats;
};
/*
@@ -138,6 +154,20 @@ void mtk_ovl_disable_vblank(struct device *dev)
writel_relaxed(0x0, ovl->regs + DISP_REG_OVL_INTEN);
}
+const u32 *mtk_ovl_get_formats(struct device *dev)
+{
+ struct mtk_disp_ovl *ovl = dev_get_drvdata(dev);
+
+ return ovl->data->formats;
+}
+
+size_t mtk_ovl_get_num_formats(struct device *dev)
+{
+ struct mtk_disp_ovl *ovl = dev_get_drvdata(dev);
+
+ return ovl->data->num_formats;
+}
+
int mtk_ovl_clk_enable(struct device *dev)
{
struct mtk_disp_ovl *ovl = dev_get_drvdata(dev);
@@ -495,6 +525,8 @@ static const struct mtk_disp_ovl_data
mt2701_ovl_driver_data = {
.gmc_bits = 8,
.layer_nr = 4,
.fmt_rgb565_is_0 = false,
+ .formats = mt8173_formats,
+ .num_formats = ARRAY_SIZE(mt8173_formats),
};
static const struct mtk_disp_ovl_data mt8173_ovl_driver_data = {
@@ -502,6 +534,8 @@ static const struct mtk_disp_ovl_data
mt8173_ovl_driver_data = {
.gmc_bits = 8,
.layer_nr = 4,
.fmt_rgb565_is_0 = true,
+ .formats = mt8173_formats,
+ .num_formats = ARRAY_SIZE(mt8173_formats),
mt8173_formats are the same as the old struct formats. Maybe we should use that
and only overwrite where we actually use a different array.
Regarding num_formats, see my comment below.
};
static const struct mtk_disp_ovl_data mt8183_ovl_driver_data = {
@@ -509,6 +543,8 @@ static const struct mtk_disp_ovl_data
mt8183_ovl_driver_data = {
.gmc_bits = 10,
.layer_nr = 4,
.fmt_rgb565_is_0 = true,
+ .formats = mt8173_formats,
+ .num_formats = ARRAY_SIZE(mt8173_formats),
};
static const struct mtk_disp_ovl_data mt8183_ovl_2l_driver_data = {
@@ -516,6 +552,8 @@ static const struct mtk_disp_ovl_data
mt8183_ovl_2l_driver_data = {
.gmc_bits = 10,
.layer_nr = 2,
.fmt_rgb565_is_0 = true,
+ .formats = mt8173_formats,
+ .num_formats = ARRAY_SIZE(mt8173_formats),
};
static const struct mtk_disp_ovl_data mt8192_ovl_driver_data = {
@@ -524,6 +562,8 @@ static
[PATCH 1/3] drm/mediatek: Refactor pixel format logic
Add an DDP component interface for querying pixel format support and move list
of supported pixel formats into DDP components instead of mtk_drm_plane.c
Tested by running Chrome on an MT8195.
Signed-off-by: Justin Green
---
drivers/gpu/drm/mediatek/mtk_disp_drv.h | 4 ++
drivers/gpu/drm/mediatek/mtk_disp_ovl.c | 44 +
drivers/gpu/drm/mediatek/mtk_disp_rdma.c| 38 ++
drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 4 +-
drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c | 4 ++
drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h | 20 ++
drivers/gpu/drm/mediatek/mtk_drm_plane.c| 24 ---
drivers/gpu/drm/mediatek/mtk_drm_plane.h| 3 +-
8 files changed, 123 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/mediatek/mtk_disp_drv.h
b/drivers/gpu/drm/mediatek/mtk_disp_drv.h
index 33e61a136bbc..0df6a06defb8 100644
--- a/drivers/gpu/drm/mediatek/mtk_disp_drv.h
+++ b/drivers/gpu/drm/mediatek/mtk_disp_drv.h
@@ -96,6 +96,8 @@ void mtk_ovl_register_vblank_cb(struct device *dev,
void mtk_ovl_unregister_vblank_cb(struct device *dev);
void mtk_ovl_enable_vblank(struct device *dev);
void mtk_ovl_disable_vblank(struct device *dev);
+const u32 *mtk_ovl_get_formats(struct device *dev);
+size_t mtk_ovl_get_num_formats(struct device *dev);
void mtk_rdma_bypass_shadow(struct device *dev);
int mtk_rdma_clk_enable(struct device *dev);
@@ -115,6 +117,8 @@ void mtk_rdma_register_vblank_cb(struct device *dev,
void mtk_rdma_unregister_vblank_cb(struct device *dev);
void mtk_rdma_enable_vblank(struct device *dev);
void mtk_rdma_disable_vblank(struct device *dev);
+const u32 *mtk_rdma_get_formats(struct device *dev);
+size_t mtk_rdma_get_num_formats(struct device *dev);
int mtk_mdp_rdma_clk_enable(struct device *dev);
void mtk_mdp_rdma_clk_disable(struct device *dev);
diff --git a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
index 84daeaffab6a..8743c8047dc9 100644
--- a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
+++ b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
@@ -66,6 +66,20 @@
#defineOVL_CON_VIRT_FLIP BIT(9)
#defineOVL_CON_HORZ_FLIP BIT(10)
+static const u32 mt8173_formats[] = {
+ DRM_FORMAT_XRGB,
+ DRM_FORMAT_ARGB,
+ DRM_FORMAT_BGRX,
+ DRM_FORMAT_BGRA,
+ DRM_FORMAT_ABGR,
+ DRM_FORMAT_XBGR,
+ DRM_FORMAT_RGB888,
+ DRM_FORMAT_BGR888,
+ DRM_FORMAT_RGB565,
+ DRM_FORMAT_UYVY,
+ DRM_FORMAT_YUYV,
+};
+
struct mtk_disp_ovl_data {
unsigned int addr;
unsigned int gmc_bits;
@@ -73,6 +87,8 @@ struct mtk_disp_ovl_data {
bool fmt_rgb565_is_0;
bool smi_id_en;
bool supports_afbc;
+ const u32 *formats;
+ size_t num_formats;
};
/*
@@ -138,6 +154,20 @@ void mtk_ovl_disable_vblank(struct device *dev)
writel_relaxed(0x0, ovl->regs + DISP_REG_OVL_INTEN);
}
+const u32 *mtk_ovl_get_formats(struct device *dev)
+{
+ struct mtk_disp_ovl *ovl = dev_get_drvdata(dev);
+
+ return ovl->data->formats;
+}
+
+size_t mtk_ovl_get_num_formats(struct device *dev)
+{
+ struct mtk_disp_ovl *ovl = dev_get_drvdata(dev);
+
+ return ovl->data->num_formats;
+}
+
int mtk_ovl_clk_enable(struct device *dev)
{
struct mtk_disp_ovl *ovl = dev_get_drvdata(dev);
@@ -495,6 +525,8 @@ static const struct mtk_disp_ovl_data
mt2701_ovl_driver_data = {
.gmc_bits = 8,
.layer_nr = 4,
.fmt_rgb565_is_0 = false,
+ .formats = mt8173_formats,
+ .num_formats = ARRAY_SIZE(mt8173_formats),
};
static const struct mtk_disp_ovl_data mt8173_ovl_driver_data = {
@@ -502,6 +534,8 @@ static const struct mtk_disp_ovl_data
mt8173_ovl_driver_data = {
.gmc_bits = 8,
.layer_nr = 4,
.fmt_rgb565_is_0 = true,
+ .formats = mt8173_formats,
+ .num_formats = ARRAY_SIZE(mt8173_formats),
};
static const struct mtk_disp_ovl_data mt8183_ovl_driver_data = {
@@ -509,6 +543,8 @@ static const struct mtk_disp_ovl_data
mt8183_ovl_driver_data = {
.gmc_bits = 10,
.layer_nr = 4,
.fmt_rgb565_is_0 = true,
+ .formats = mt8173_formats,
+ .num_formats = ARRAY_SIZE(mt8173_formats),
};
static const struct mtk_disp_ovl_data mt8183_ovl_2l_driver_data = {
@@ -516,6 +552,8 @@ static const struct mtk_disp_ovl_data
mt8183_ovl_2l_driver_data = {
.gmc_bits = 10,
.layer_nr = 2,
.fmt_rgb565_is_0 = true,
+ .formats = mt8173_formats,
+ .num_formats = ARRAY_SIZE(mt8173_formats),
};
static const struct mtk_disp_ovl_data mt8192_ovl_driver_data = {
@@ -524,6 +562,8 @@ static const struct mtk_disp_ovl_data
mt8192_ovl_driver_data = {
.layer_nr = 4,
.fmt_rgb565_is_0 = true,
.smi_id_en = true,
+ .formats = mt8173_formats,
+ .num_formats = ARRAY_SIZE(mt8173_formats),
};
static const struct mtk_disp_ovl_data
