[PATCH v3,2/2] drm: mediatek: Adjust the dpi output format to MT8186

Fri Aug 26 14:13:40 UTC 2022

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 at mediatek.com
> > > > wrote:
> > > > > From: Xinlei Lee <xinlei.lee at 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 at mediatek.com>
> > > > > Signed-off-by: Jitao Shi <jitao.shi at mediatek.com>
> > > > > Signed-off-by: Xinlei Lee <xinlei.lee at 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