| Message ID | 1661235517-23699-3-git-send-email-xinlei.lee@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add dpi output format control for MT8186 | expand |
On Tue, Aug 23, 2022 at 02:18:37PM +0800, xinlei.lee@mediatek.com wrote: > From: Xinlei Lee <xinlei.lee@mediatek.com> > > Dpi output needs to adjust the output format to dual edge for MT8186. > Because MT8186 HW has been modified at that time, SW needs to cooperate. > And the register (MMSYS) reserved for dpi will be used for output > format control (dual_edge/single_edge). > > Co-developed-by: Jitao Shi <jitao.shi@mediatek.com> > Signed-off-by: Jitao Shi <jitao.shi@mediatek.com> > Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com> > > --- [..] > --- a/drivers/gpu/drm/mediatek/mtk_dpi.c > +++ b/drivers/gpu/drm/mediatek/mtk_dpi.c [..] > * @yuv422_en_bit: Enable bit of yuv422. > * @csc_enable_bit: Enable bit of CSC. > * @pixels_per_iter: Quantity of transferred pixels per iteration. > + * @rgb888_dual_enable: Control output format for mt8186. Let's not mention mt8186 in the description to keep the property generic. Also, this description should say what having 'rgb888_dual_enable = true' indicates about the hardware (in this case mt8186) and it currently doesn't. Let's take a step back. What does 'dual enable' mean in this context and how does it relate to 'dual edge' and the dpi output format? By answering those questions we can find a description (and maybe variable name) that makes more sense. > */ [..] > @@ -449,6 +454,9 @@ static void mtk_dpi_dual_edge(struct mtk_dpi *dpi) > mtk_dpi_mask(dpi, DPI_OUTPUT_SETTING, > dpi->output_fmt == MEDIA_BUS_FMT_RGB888_2X12_LE ? > EDGE_SEL : 0, EDGE_SEL); > + if (dpi->conf->rgb888_dual_enable) > + mtk_mmsys_ddp_dpi_fmt_config(dpi->mmsys_dev, DPI_RGB888_DDR_CON, > + DPI_FORMAT_MASK, NULL); This if block should be further indented. > } else { > mtk_dpi_mask(dpi, DPI_DDR_SETTING, DDR_EN | DDR_4PHASE, 0); > } [..] > --- a/drivers/gpu/drm/mediatek/mtk_dpi_regs.h > +++ b/drivers/gpu/drm/mediatek/mtk_dpi_regs.h > @@ -235,4 +235,8 @@ > #define MATRIX_SEL_RGB_TO_JPEG 0 > #define MATRIX_SEL_RGB_TO_BT601 2 > > +#define DPI_FORMAT_MASK 0x1 > +#define DPI_RGB888_DDR_CON BIT(0) > +#define DPI_RGB565_SDR_CON BIT(1) I'm not sure if it would make more sense to have these definitions in the mmsys header since they're configurations of a register in mmsys' iospace... I think we can keep them here but at least add a comment above: /* Values for DPI configuration in MMSYS address space */ Thanks, Nícolas
On Tue, 2022-08-23 at 16:16 -0400, Nícolas F. R. A. Prado wrote: > On Tue, Aug 23, 2022 at 02:18:37PM +0800, xinlei.lee@mediatek.com > wrote: > > From: Xinlei Lee <xinlei.lee@mediatek.com> > > > > Dpi output needs to adjust the output format to dual edge for > > MT8186. > > Because MT8186 HW has been modified at that time, SW needs to > > cooperate. > > And the register (MMSYS) reserved for dpi will be used for output > > format control (dual_edge/single_edge). > > > > Co-developed-by: Jitao Shi <jitao.shi@mediatek.com> > > Signed-off-by: Jitao Shi <jitao.shi@mediatek.com> > > Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com> > > > > --- > > [..] > > --- a/drivers/gpu/drm/mediatek/mtk_dpi.c > > +++ b/drivers/gpu/drm/mediatek/mtk_dpi.c > > [..] > > * @yuv422_en_bit: Enable bit of yuv422. > > * @csc_enable_bit: Enable bit of CSC. > > * @pixels_per_iter: Quantity of transferred pixels per iteration. > > + * @rgb888_dual_enable: Control output format for mt8186. > > Let's not mention mt8186 in the description to keep the property > generic. Also, > this description should say what having 'rgb888_dual_enable = true' > indicates > about the hardware (in this case mt8186) and it currently doesn't. > > Let's take a step back. What does 'dual enable' mean in this context > and how > does it relate to 'dual edge' and the dpi output format? By answering > those > questions we can find a description (and maybe variable name) that > makes more > sense. > > > */ > > [..] > > @@ -449,6 +454,9 @@ static void mtk_dpi_dual_edge(struct mtk_dpi > > *dpi) > > mtk_dpi_mask(dpi, DPI_OUTPUT_SETTING, > > dpi->output_fmt == > > MEDIA_BUS_FMT_RGB888_2X12_LE ? > > EDGE_SEL : 0, EDGE_SEL); > > + if (dpi->conf->rgb888_dual_enable) > > + mtk_mmsys_ddp_dpi_fmt_config(dpi->mmsys_dev, > > DPI_RGB888_DDR_CON, > > + DPI_FORMAT_MASK, NULL); > > This if block should be further indented. > > > } else { > > mtk_dpi_mask(dpi, DPI_DDR_SETTING, DDR_EN | DDR_4PHASE, > > 0); > > } > > [..] > > --- a/drivers/gpu/drm/mediatek/mtk_dpi_regs.h > > +++ b/drivers/gpu/drm/mediatek/mtk_dpi_regs.h > > @@ -235,4 +235,8 @@ > > #define MATRIX_SEL_RGB_TO_JPEG 0 > > #define MATRIX_SEL_RGB_TO_BT601 2 > > > > +#define DPI_FORMAT_MASK 0x1 > > +#define DPI_RGB888_DDR_CON BIT(0) > > +#define DPI_RGB565_SDR_CON BIT(1) > > I'm not sure if it would make more sense to have these definitions in > the mmsys > header since they're configurations of a register in mmsys' > iospace... I think > we can keep them here but at least add a comment above: > > /* Values for DPI configuration in MMSYS address space */ > > Thanks, > Nícolas Hi Nícolas: Thanks for your careful review! I will modify the description of this member variable and add the hardware state corresponding to the software setting. (eg. rgb888_dual_enable = true the hardware output rgb888_dual_edge format data) Your suggestion is very necessary, maybe my name is not accurate enough, this flag is to enable RGB888_dual_edge format output. Would it be better for the variable to be called RGB888_dual_edge_enable then? Also I'll fix the code formatting issues and add descriptions to the mmsys registers in the mtk_dpi_regs.h file. Best Regards! xinlei
On Wed, Aug 24, 2022 at 09:59:21AM +0800, xinlei.lee wrote: > On Tue, 2022-08-23 at 16:16 -0400, Nícolas F. R. A. Prado wrote: > > On Tue, Aug 23, 2022 at 02:18:37PM +0800, xinlei.lee@mediatek.com > > wrote: > > > From: Xinlei Lee <xinlei.lee@mediatek.com> > > > > > > Dpi output needs to adjust the output format to dual edge for > > > MT8186. > > > Because MT8186 HW has been modified at that time, SW needs to > > > cooperate. > > > And the register (MMSYS) reserved for dpi will be used for output > > > format control (dual_edge/single_edge). > > > > > > Co-developed-by: Jitao Shi <jitao.shi@mediatek.com> > > > Signed-off-by: Jitao Shi <jitao.shi@mediatek.com> > > > Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com> > > > > > > --- > > > > [..] > > > --- a/drivers/gpu/drm/mediatek/mtk_dpi.c > > > +++ b/drivers/gpu/drm/mediatek/mtk_dpi.c > > > > [..] > > > * @yuv422_en_bit: Enable bit of yuv422. > > > * @csc_enable_bit: Enable bit of CSC. > > > * @pixels_per_iter: Quantity of transferred pixels per iteration. > > > + * @rgb888_dual_enable: Control output format for mt8186. > > > > Let's not mention mt8186 in the description to keep the property > > generic. Also, > > this description should say what having 'rgb888_dual_enable = true' > > indicates > > about the hardware (in this case mt8186) and it currently doesn't. > > > > Let's take a step back. What does 'dual enable' mean in this context > > and how > > does it relate to 'dual edge' and the dpi output format? By answering > > those > > questions we can find a description (and maybe variable name) that > > makes more > > sense. > > > > > */ > > > > [..] > > > @@ -449,6 +454,9 @@ static void mtk_dpi_dual_edge(struct mtk_dpi > > > *dpi) > > > mtk_dpi_mask(dpi, DPI_OUTPUT_SETTING, > > > dpi->output_fmt == > > > MEDIA_BUS_FMT_RGB888_2X12_LE ? > > > EDGE_SEL : 0, EDGE_SEL); > > > + if (dpi->conf->rgb888_dual_enable) > > > + mtk_mmsys_ddp_dpi_fmt_config(dpi->mmsys_dev, > > > DPI_RGB888_DDR_CON, > > > + DPI_FORMAT_MASK, NULL); > > > > This if block should be further indented. > > > > > } else { > > > mtk_dpi_mask(dpi, DPI_DDR_SETTING, DDR_EN | DDR_4PHASE, > > > 0); > > > } > > > > [..] > > > --- a/drivers/gpu/drm/mediatek/mtk_dpi_regs.h > > > +++ b/drivers/gpu/drm/mediatek/mtk_dpi_regs.h > > > @@ -235,4 +235,8 @@ > > > #define MATRIX_SEL_RGB_TO_JPEG 0 > > > #define MATRIX_SEL_RGB_TO_BT601 2 > > > > > > +#define DPI_FORMAT_MASK 0x1 > > > +#define DPI_RGB888_DDR_CON BIT(0) > > > +#define DPI_RGB565_SDR_CON BIT(1) > > > > I'm not sure if it would make more sense to have these definitions in > > the mmsys > > header since they're configurations of a register in mmsys' > > iospace... I think > > we can keep them here but at least add a comment above: > > > > /* Values for DPI configuration in MMSYS address space */ > > > > Thanks, > > Nícolas > > Hi Nícolas: > Thanks for your careful review! > I will modify the description of this member variable and add the > hardware state corresponding to the software setting. > (eg. rgb888_dual_enable = true the hardware output rgb888_dual_edge > format data) > > Your suggestion is very necessary, maybe my name is not accurate > enough, this flag is to enable RGB888_dual_edge format output. > Would it be better for the variable to be called > RGB888_dual_edge_enable then? The thing is, we also output in rgb888 dual edge format on mt8183 and mt8192, and therefore set DDR_EN in mtk_dpi_dual_edge(), right? But, as you said, we don't need to enable this new rgb888_dual_enable variable on those platforms, only on mt8186. So that's why I don't think the current name/description is suitable. If the variable only needs to be set on mt8186, it should have a name and description that shows what is different between mt8186 and the others. But without containing the "mt8186" name, since this might happen on other SoCs later on. My understanding is that even though both mt8186 and mt8192 output in the rgb888 dual edge format, only mt8186 is able to configure the edge setting in MMSYS (so on mt8192 it would be hardwired to dual edge and not possible to change). So what I propose is Name: edge_cfg_in_mmsys Description: "If the edge configuration for DPI's output needs to be set in MMSYS" But maybe since you know the hardware, you might be able to find an even better name/description. Thanks, Nícolas
On Wed, 2022-08-24 at 11:44 -0400, Nícolas F. R. A. Prado wrote: > On Wed, Aug 24, 2022 at 09:59:21AM +0800, xinlei.lee wrote: > > On Tue, 2022-08-23 at 16:16 -0400, Nícolas F. R. A. Prado wrote: > > > On Tue, Aug 23, 2022 at 02:18:37PM +0800, xinlei.lee@mediatek.com > > > wrote: > > > > From: Xinlei Lee <xinlei.lee@mediatek.com> > > > > > > > > Dpi output needs to adjust the output format to dual edge for > > > > MT8186. > > > > Because MT8186 HW has been modified at that time, SW needs to > > > > cooperate. > > > > And the register (MMSYS) reserved for dpi will be used for > > > > output > > > > format control (dual_edge/single_edge). > > > > > > > > Co-developed-by: Jitao Shi <jitao.shi@mediatek.com> > > > > Signed-off-by: Jitao Shi <jitao.shi@mediatek.com> > > > > Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com> > > > > > > > > --- > > > > > > [..] > > > > --- a/drivers/gpu/drm/mediatek/mtk_dpi.c > > > > +++ b/drivers/gpu/drm/mediatek/mtk_dpi.c > > > > > > [..] > > > > * @yuv422_en_bit: Enable bit of yuv422. > > > > * @csc_enable_bit: Enable bit of CSC. > > > > * @pixels_per_iter: Quantity of transferred pixels per > > > > iteration. > > > > + * @rgb888_dual_enable: Control output format for mt8186. > > > > > > Let's not mention mt8186 in the description to keep the property > > > generic. Also, > > > this description should say what having 'rgb888_dual_enable = > > > true' > > > indicates > > > about the hardware (in this case mt8186) and it currently > > > doesn't. > > > > > > Let's take a step back. What does 'dual enable' mean in this > > > context > > > and how > > > does it relate to 'dual edge' and the dpi output format? By > > > answering > > > those > > > questions we can find a description (and maybe variable name) > > > that > > > makes more > > > sense. > > > > > > > */ > > > > > > [..] > > > > @@ -449,6 +454,9 @@ static void mtk_dpi_dual_edge(struct > > > > mtk_dpi > > > > *dpi) > > > > mtk_dpi_mask(dpi, DPI_OUTPUT_SETTING, > > > > dpi->output_fmt == > > > > MEDIA_BUS_FMT_RGB888_2X12_LE ? > > > > EDGE_SEL : 0, EDGE_SEL); > > > > + if (dpi->conf->rgb888_dual_enable) > > > > + mtk_mmsys_ddp_dpi_fmt_config(dpi->mmsys_dev, > > > > DPI_RGB888_DDR_CON, > > > > + DPI_FORMAT_MASK, > > > > NULL); > > > > > > This if block should be further indented. > > > > > > > } else { > > > > mtk_dpi_mask(dpi, DPI_DDR_SETTING, DDR_EN | > > > > DDR_4PHASE, > > > > 0); > > > > } > > > > > > [..] > > > > --- a/drivers/gpu/drm/mediatek/mtk_dpi_regs.h > > > > +++ b/drivers/gpu/drm/mediatek/mtk_dpi_regs.h > > > > @@ -235,4 +235,8 @@ > > > > #define MATRIX_SEL_RGB_TO_JPEG 0 > > > > #define MATRIX_SEL_RGB_TO_BT601 2 > > > > > > > > +#define DPI_FORMAT_MASK 0x1 > > > > +#define DPI_RGB888_DDR_CON BIT(0) > > > > +#define DPI_RGB565_SDR_CON BIT(1) > > > > > > I'm not sure if it would make more sense to have these > > > definitions in > > > the mmsys > > > header since they're configurations of a register in mmsys' > > > iospace... I think > > > we can keep them here but at least add a comment above: > > > > > > /* Values for DPI configuration in MMSYS address space */ > > > > > > Thanks, > > > Nícolas > > > > Hi Nícolas: > > Thanks for your careful review! > > I will modify the description of this member variable and add the > > hardware state corresponding to the software setting. > > (eg. rgb888_dual_enable = true the hardware output rgb888_dual_edge > > format data) > > > > Your suggestion is very necessary, maybe my name is not accurate > > enough, this flag is to enable RGB888_dual_edge format output. > > Would it be better for the variable to be called > > RGB888_dual_edge_enable then? > > The thing is, we also output in rgb888 dual edge format on mt8183 and > mt8192, > and therefore set DDR_EN in mtk_dpi_dual_edge(), right? But, as you > said, we > don't need to enable this new rgb888_dual_enable variable on those > platforms, > only on mt8186. So that's why I don't think the current > name/description is > suitable. If the variable only needs to be set on mt8186, it should > have a name > and description that shows what is different between mt8186 and the > others. But > without containing the "mt8186" name, since this might happen on > other SoCs > later on. > > My understanding is that even though both mt8186 and mt8192 output in > the rgb888 > dual edge format, only mt8186 is able to configure the edge setting > in MMSYS (so > on mt8192 it would be hardwired to dual edge and not possible to > change). So > what I propose is > > Name: edge_cfg_in_mmsys > > Description: "If the edge configuration for DPI's output needs to be > set in MMSYS" > > But maybe since you know the hardware, you might be able to find an > even better > name/description. > > Thanks, > Nícolas Hi Nícolas: Thanks for your suggestion. At present, it is true that only 8186 needs to set this flag when outputting dual_edge format. If other ICs need to modify the output format, they only need to modify the DPI register. On the 8186, DPI MUX (0x400) is required for synchronous modification. A more detailed explanation of this DPI MUX register is bit[0]: dual_edge enable, bit[1]: rgb565_en. And the priority of bit[1] is higher, the following is the format of different combinations: 00: SDR enable 01: DDR enable 10: RGB565 11: RGB565 The hardware characteristics can be ignored. Based on this situation, what is your opinion if it is changed to "edge_cfg_in_mmsys"? Best Regards! Xinlei
On Fri, Aug 26, 2022 at 11:58:29AM +0800, xinlei.lee wrote: > On Wed, 2022-08-24 at 11:44 -0400, Nícolas F. R. A. Prado wrote: > > On Wed, Aug 24, 2022 at 09:59:21AM +0800, xinlei.lee wrote: > > > On Tue, 2022-08-23 at 16:16 -0400, Nícolas F. R. A. Prado wrote: > > > > On Tue, Aug 23, 2022 at 02:18:37PM +0800, xinlei.lee@mediatek.com > > > > wrote: > > > > > From: Xinlei Lee <xinlei.lee@mediatek.com> > > > > > > > > > > Dpi output needs to adjust the output format to dual edge for > > > > > MT8186. > > > > > Because MT8186 HW has been modified at that time, SW needs to > > > > > cooperate. > > > > > And the register (MMSYS) reserved for dpi will be used for > > > > > output > > > > > format control (dual_edge/single_edge). > > > > > > > > > > Co-developed-by: Jitao Shi <jitao.shi@mediatek.com> > > > > > Signed-off-by: Jitao Shi <jitao.shi@mediatek.com> > > > > > Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com> > > > > > > > > > > --- > > > > > > > > [..] > > > > > --- a/drivers/gpu/drm/mediatek/mtk_dpi.c > > > > > +++ b/drivers/gpu/drm/mediatek/mtk_dpi.c > > > > > > > > [..] > > > > > * @yuv422_en_bit: Enable bit of yuv422. > > > > > * @csc_enable_bit: Enable bit of CSC. > > > > > * @pixels_per_iter: Quantity of transferred pixels per > > > > > iteration. > > > > > + * @rgb888_dual_enable: Control output format for mt8186. > > > > > > > > Let's not mention mt8186 in the description to keep the property > > > > generic. Also, > > > > this description should say what having 'rgb888_dual_enable = > > > > true' > > > > indicates > > > > about the hardware (in this case mt8186) and it currently > > > > doesn't. > > > > > > > > Let's take a step back. What does 'dual enable' mean in this > > > > context > > > > and how > > > > does it relate to 'dual edge' and the dpi output format? By > > > > answering > > > > those > > > > questions we can find a description (and maybe variable name) > > > > that > > > > makes more > > > > sense. > > > > > > > > > */ > > > > > > > > [..] > > > > > @@ -449,6 +454,9 @@ static void mtk_dpi_dual_edge(struct > > > > > mtk_dpi > > > > > *dpi) > > > > > mtk_dpi_mask(dpi, DPI_OUTPUT_SETTING, > > > > > dpi->output_fmt == > > > > > MEDIA_BUS_FMT_RGB888_2X12_LE ? > > > > > EDGE_SEL : 0, EDGE_SEL); > > > > > + if (dpi->conf->rgb888_dual_enable) > > > > > + mtk_mmsys_ddp_dpi_fmt_config(dpi->mmsys_dev, > > > > > DPI_RGB888_DDR_CON, > > > > > + DPI_FORMAT_MASK, > > > > > NULL); > > > > > > > > This if block should be further indented. > > > > > > > > > } else { > > > > > mtk_dpi_mask(dpi, DPI_DDR_SETTING, DDR_EN | > > > > > DDR_4PHASE, > > > > > 0); > > > > > } > > > > > > > > [..] > > > > > --- a/drivers/gpu/drm/mediatek/mtk_dpi_regs.h > > > > > +++ b/drivers/gpu/drm/mediatek/mtk_dpi_regs.h > > > > > @@ -235,4 +235,8 @@ > > > > > #define MATRIX_SEL_RGB_TO_JPEG 0 > > > > > #define MATRIX_SEL_RGB_TO_BT601 2 > > > > > > > > > > +#define DPI_FORMAT_MASK 0x1 > > > > > +#define DPI_RGB888_DDR_CON BIT(0) > > > > > +#define DPI_RGB565_SDR_CON BIT(1) > > > > > > > > I'm not sure if it would make more sense to have these > > > > definitions in > > > > the mmsys > > > > header since they're configurations of a register in mmsys' > > > > iospace... I think > > > > we can keep them here but at least add a comment above: > > > > > > > > /* Values for DPI configuration in MMSYS address space */ > > > > > > > > Thanks, > > > > Nícolas > > > > > > Hi Nícolas: > > > Thanks for your careful review! > > > I will modify the description of this member variable and add the > > > hardware state corresponding to the software setting. > > > (eg. rgb888_dual_enable = true the hardware output rgb888_dual_edge > > > format data) > > > > > > Your suggestion is very necessary, maybe my name is not accurate > > > enough, this flag is to enable RGB888_dual_edge format output. > > > Would it be better for the variable to be called > > > RGB888_dual_edge_enable then? > > > > The thing is, we also output in rgb888 dual edge format on mt8183 and > > mt8192, > > and therefore set DDR_EN in mtk_dpi_dual_edge(), right? But, as you > > said, we > > don't need to enable this new rgb888_dual_enable variable on those > > platforms, > > only on mt8186. So that's why I don't think the current > > name/description is > > suitable. If the variable only needs to be set on mt8186, it should > > have a name > > and description that shows what is different between mt8186 and the > > others. But > > without containing the "mt8186" name, since this might happen on > > other SoCs > > later on. > > > > My understanding is that even though both mt8186 and mt8192 output in > > the rgb888 > > dual edge format, only mt8186 is able to configure the edge setting > > in MMSYS (so > > on mt8192 it would be hardwired to dual edge and not possible to > > change). So > > what I propose is > > > > Name: edge_cfg_in_mmsys > > > > Description: "If the edge configuration for DPI's output needs to be > > set in MMSYS" > > > > But maybe since you know the hardware, you might be able to find an > > even better > > name/description. > > > > Thanks, > > Nícolas > Hi Nícolas: > > Thanks for your suggestion. > > At present, it is true that only 8186 needs to set this flag when > outputting dual_edge format. > If other ICs need to modify the output format, they only need to modify > the DPI register. > On the 8186, DPI MUX (0x400) is required for synchronous modification. > A more detailed explanation of this DPI MUX register is > bit[0]: dual_edge enable, bit[1]: rgb565_en. > And the priority of bit[1] is higher, the following is the format of > different combinations: > 00: SDR enable > 01: DDR enable > 10: RGB565 > 11: RGB565 > > The hardware characteristics can be ignored. Based on this situation, > what is your opinion if it is changed to "edge_cfg_in_mmsys"? Hi Xinlei, thank you for the detailed explanation of the situation. Based on this, I still think the name I suggested, "edge_cfg_in_mmsys", and its description, make sense. If you're also happy with this name and description, then let's go with that. Thanks, Nícolas
On Fri, 2022-08-26 at 10:13 -0400, Nícolas F. R. A. Prado wrote: > On Fri, Aug 26, 2022 at 11:58:29AM +0800, xinlei.lee wrote: > > On Wed, 2022-08-24 at 11:44 -0400, Nícolas F. R. A. Prado wrote: > > > On Wed, Aug 24, 2022 at 09:59:21AM +0800, xinlei.lee wrote: > > > > On Tue, 2022-08-23 at 16:16 -0400, Nícolas F. R. A. Prado > > > > wrote: > > > > > On Tue, Aug 23, 2022 at 02:18:37PM +0800, > > > > > xinlei.lee@mediatek.com > > > > > wrote: > > > > > > From: Xinlei Lee <xinlei.lee@mediatek.com> > > > > > > > > > > > > Dpi output needs to adjust the output format to dual edge > > > > > > for > > > > > > MT8186. > > > > > > Because MT8186 HW has been modified at that time, SW needs > > > > > > to > > > > > > cooperate. > > > > > > And the register (MMSYS) reserved for dpi will be used for > > > > > > output > > > > > > format control (dual_edge/single_edge). > > > > > > > > > > > > Co-developed-by: Jitao Shi <jitao.shi@mediatek.com> > > > > > > Signed-off-by: Jitao Shi <jitao.shi@mediatek.com> > > > > > > Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com> > > > > > > > > > > > > --- > > > > > > > > > > [..] > > > > > > --- a/drivers/gpu/drm/mediatek/mtk_dpi.c > > > > > > +++ b/drivers/gpu/drm/mediatek/mtk_dpi.c > > > > > > > > > > [..] > > > > > > * @yuv422_en_bit: Enable bit of yuv422. > > > > > > * @csc_enable_bit: Enable bit of CSC. > > > > > > * @pixels_per_iter: Quantity of transferred pixels per > > > > > > iteration. > > > > > > + * @rgb888_dual_enable: Control output format for mt8186. > > > > > > > > > > Let's not mention mt8186 in the description to keep the > > > > > property > > > > > generic. Also, > > > > > this description should say what having 'rgb888_dual_enable = > > > > > true' > > > > > indicates > > > > > about the hardware (in this case mt8186) and it currently > > > > > doesn't. > > > > > > > > > > Let's take a step back. What does 'dual enable' mean in this > > > > > context > > > > > and how > > > > > does it relate to 'dual edge' and the dpi output format? By > > > > > answering > > > > > those > > > > > questions we can find a description (and maybe variable name) > > > > > that > > > > > makes more > > > > > sense. > > > > > > > > > > > */ > > > > > > > > > > [..] > > > > > > @@ -449,6 +454,9 @@ static void mtk_dpi_dual_edge(struct > > > > > > mtk_dpi > > > > > > *dpi) > > > > > > mtk_dpi_mask(dpi, DPI_OUTPUT_SETTING, > > > > > > dpi->output_fmt == > > > > > > MEDIA_BUS_FMT_RGB888_2X12_LE ? > > > > > > EDGE_SEL : 0, EDGE_SEL); > > > > > > + if (dpi->conf->rgb888_dual_enable) > > > > > > + mtk_mmsys_ddp_dpi_fmt_config(dpi->mmsys_dev, > > > > > > DPI_RGB888_DDR_CON, > > > > > > + DPI_FORMAT_MASK, > > > > > > NULL); > > > > > > > > > > This if block should be further indented. > > > > > > > > > > > } else { > > > > > > mtk_dpi_mask(dpi, DPI_DDR_SETTING, DDR_EN | > > > > > > DDR_4PHASE, > > > > > > 0); > > > > > > } > > > > > > > > > > [..] > > > > > > --- a/drivers/gpu/drm/mediatek/mtk_dpi_regs.h > > > > > > +++ b/drivers/gpu/drm/mediatek/mtk_dpi_regs.h > > > > > > @@ -235,4 +235,8 @@ > > > > > > #define MATRIX_SEL_RGB_TO_JPEG 0 > > > > > > #define MATRIX_SEL_RGB_TO_BT601 2 > > > > > > > > > > > > +#define DPI_FORMAT_MASK 0x1 > > > > > > +#define DPI_RGB888_DDR_CON BIT(0) > > > > > > +#define DPI_RGB565_SDR_CON BIT(1) > > > > > > > > > > I'm not sure if it would make more sense to have these > > > > > definitions in > > > > > the mmsys > > > > > header since they're configurations of a register in mmsys' > > > > > iospace... I think > > > > > we can keep them here but at least add a comment above: > > > > > > > > > > /* Values for DPI configuration in MMSYS address space */ > > > > > > > > > > Thanks, > > > > > Nícolas > > > > > > > > Hi Nícolas: > > > > Thanks for your careful review! > > > > I will modify the description of this member variable and add > > > > the > > > > hardware state corresponding to the software setting. > > > > (eg. rgb888_dual_enable = true the hardware output > > > > rgb888_dual_edge > > > > format data) > > > > > > > > Your suggestion is very necessary, maybe my name is not > > > > accurate > > > > enough, this flag is to enable RGB888_dual_edge format output. > > > > Would it be better for the variable to be called > > > > RGB888_dual_edge_enable then? > > > > > > The thing is, we also output in rgb888 dual edge format on mt8183 > > > and > > > mt8192, > > > and therefore set DDR_EN in mtk_dpi_dual_edge(), right? But, as > > > you > > > said, we > > > don't need to enable this new rgb888_dual_enable variable on > > > those > > > platforms, > > > only on mt8186. So that's why I don't think the current > > > name/description is > > > suitable. If the variable only needs to be set on mt8186, it > > > should > > > have a name > > > and description that shows what is different between mt8186 and > > > the > > > others. But > > > without containing the "mt8186" name, since this might happen on > > > other SoCs > > > later on. > > > > > > My understanding is that even though both mt8186 and mt8192 > > > output in > > > the rgb888 > > > dual edge format, only mt8186 is able to configure the edge > > > setting > > > in MMSYS (so > > > on mt8192 it would be hardwired to dual edge and not possible to > > > change). So > > > what I propose is > > > > > > Name: edge_cfg_in_mmsys > > > > > > Description: "If the edge configuration for DPI's output needs to > > > be > > > set in MMSYS" > > > > > > But maybe since you know the hardware, you might be able to find > > > an > > > even better > > > name/description. > > > > > > Thanks, > > > Nícolas > > > > Hi Nícolas: > > > > Thanks for your suggestion. > > > > At present, it is true that only 8186 needs to set this flag when > > outputting dual_edge format. > > If other ICs need to modify the output format, they only need to > > modify > > the DPI register. > > On the 8186, DPI MUX (0x400) is required for synchronous > > modification. > > A more detailed explanation of this DPI MUX register is > > bit[0]: dual_edge enable, bit[1]: rgb565_en. > > And the priority of bit[1] is higher, the following is the format > > of > > different combinations: > > 00: SDR enable > > 01: DDR enable > > 10: RGB565 > > 11: RGB565 > > > > The hardware characteristics can be ignored. Based on this > > situation, > > what is your opinion if it is changed to "edge_cfg_in_mmsys"? > > Hi Xinlei, > > thank you for the detailed explanation of the situation. Based on > this, I still > think the name I suggested, "edge_cfg_in_mmsys", and its description, > make > sense. If you're also happy with this name and description, then > let's go with > that. > > Thanks, > Nícolas Hi Nícolas: Okay, I'll send out the next edition based on what we communicated. Best Regards! Xinlei
diff --git a/drivers/gpu/drm/mediatek/mtk_dpi.c b/drivers/gpu/drm/mediatek/mtk_dpi.c index 630a4e301ef6..b66423bd7765 100644 --- a/drivers/gpu/drm/mediatek/mtk_dpi.c +++ b/drivers/gpu/drm/mediatek/mtk_dpi.c @@ -15,6 +15,7 @@ #include <linux/of_graph.h> #include <linux/pinctrl/consumer.h> #include <linux/platform_device.h> +#include <linux/soc/mediatek/mtk-mmsys.h> #include <linux/types.h> #include <video/videomode.h> @@ -30,6 +31,7 @@ #include "mtk_disp_drv.h" #include "mtk_dpi_regs.h" #include "mtk_drm_ddp_comp.h" +#include "mtk_drm_drv.h" enum mtk_dpi_out_bit_num { MTK_DPI_OUT_BIT_NUM_8BITS, @@ -82,6 +84,7 @@ struct mtk_dpi { struct pinctrl_state *pins_dpi; u32 output_fmt; int refcount; + struct device *mmsys_dev; }; static inline struct mtk_dpi *bridge_to_dpi(struct drm_bridge *b) @@ -135,6 +138,7 @@ struct mtk_dpi_yc_limit { * @yuv422_en_bit: Enable bit of yuv422. * @csc_enable_bit: Enable bit of CSC. * @pixels_per_iter: Quantity of transferred pixels per iteration. + * @rgb888_dual_enable: Control output format for mt8186. */ struct mtk_dpi_conf { unsigned int (*cal_factor)(int clock); @@ -153,6 +157,7 @@ struct mtk_dpi_conf { u32 yuv422_en_bit; u32 csc_enable_bit; u32 pixels_per_iter; + bool rgb888_dual_enable; }; static void mtk_dpi_mask(struct mtk_dpi *dpi, u32 offset, u32 val, u32 mask) @@ -449,6 +454,9 @@ static void mtk_dpi_dual_edge(struct mtk_dpi *dpi) mtk_dpi_mask(dpi, DPI_OUTPUT_SETTING, dpi->output_fmt == MEDIA_BUS_FMT_RGB888_2X12_LE ? EDGE_SEL : 0, EDGE_SEL); + if (dpi->conf->rgb888_dual_enable) + mtk_mmsys_ddp_dpi_fmt_config(dpi->mmsys_dev, DPI_RGB888_DDR_CON, + DPI_FORMAT_MASK, NULL); } else { mtk_dpi_mask(dpi, DPI_DDR_SETTING, DDR_EN | DDR_4PHASE, 0); } @@ -778,8 +786,10 @@ static int mtk_dpi_bind(struct device *dev, struct device *master, void *data) { struct mtk_dpi *dpi = dev_get_drvdata(dev); struct drm_device *drm_dev = data; + struct mtk_drm_private *priv = drm_dev->dev_private; int ret; + dpi->mmsys_dev = priv->mmsys_dev; ret = drm_simple_encoder_init(drm_dev, &dpi->encoder, DRM_MODE_ENCODER_TMDS); if (ret) { @@ -930,6 +940,24 @@ static const struct mtk_dpi_conf mt8183_conf = { .csc_enable_bit = CSC_ENABLE, }; +static const struct mtk_dpi_conf mt8186_conf = { + .cal_factor = mt8183_calculate_factor, + .reg_h_fre_con = 0xe0, + .max_clock_khz = 150000, + .output_fmts = mt8183_output_fmts, + .num_output_fmts = ARRAY_SIZE(mt8183_output_fmts), + .rgb888_dual_enable = true, + .pixels_per_iter = 1, + .is_ck_de_pol = true, + .swap_input_support = true, + .support_direct_pin = true, + .dimension_mask = HPW_MASK, + .hvsize_mask = HSIZE_MASK, + .channel_swap_shift = CH_SWAP, + .yuv422_en_bit = YUV422_EN, + .csc_enable_bit = CSC_ENABLE, +}; + static const struct mtk_dpi_conf mt8192_conf = { .cal_factor = mt8183_calculate_factor, .reg_h_fre_con = 0xe0, @@ -1080,6 +1108,9 @@ static const struct of_device_id mtk_dpi_of_ids[] = { { .compatible = "mediatek,mt8183-dpi", .data = &mt8183_conf, }, + { .compatible = "mediatek,mt8186-dpi", + .data = &mt8186_conf, + }, { .compatible = "mediatek,mt8192-dpi", .data = &mt8192_conf, }, diff --git a/drivers/gpu/drm/mediatek/mtk_dpi_regs.h b/drivers/gpu/drm/mediatek/mtk_dpi_regs.h index 62bd4931b344..e5d254a2097b 100644 --- a/drivers/gpu/drm/mediatek/mtk_dpi_regs.h +++ b/drivers/gpu/drm/mediatek/mtk_dpi_regs.h @@ -235,4 +235,8 @@ #define MATRIX_SEL_RGB_TO_JPEG 0 #define MATRIX_SEL_RGB_TO_BT601 2 +#define DPI_FORMAT_MASK 0x1 +#define DPI_RGB888_DDR_CON BIT(0) +#define DPI_RGB565_SDR_CON BIT(1) + #endif /* __MTK_DPI_REGS_H */ diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c index d72263c8a621..6cafbfa25d02 100644 --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c @@ -784,6 +784,8 @@ static const struct of_device_id mtk_ddp_comp_dt_ids[] = { .data = (void *)MTK_DPI }, { .compatible = "mediatek,mt8183-dpi", .data = (void *)MTK_DPI }, + { .compatible = "mediatek,mt8186-dpi", + .data = (void *)MTK_DPI }, { .compatible = "mediatek,mt8192-dpi", .data = (void *)MTK_DPI }, { .compatible = "mediatek,mt8195-dp-intf",