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"?
Best Regards!
Xinlei
