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@xxxxxxxxxxxx > > > > wrote: > > > > > From: Xinlei Lee <xinlei.lee@xxxxxxxxxxxx> > > > > > > > > > > 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@xxxxxxxxxxxx> > > > > > Signed-off-by: Jitao Shi <jitao.shi@xxxxxxxxxxxx> > > > > > Signed-off-by: Xinlei Lee <xinlei.lee@xxxxxxxxxxxx> > > > > > > > > > > --- > > > > > > > > [..] > > > > > --- 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