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
