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@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
Hi Nícolas:
Okay, I'll send out the next edition based on what we communicated.
Best Regards!
Xinlei
