[v2,00/15] Improvements and fixes for mxsfb DRM driver

Message

Robert Chiras Aug. 14, 2019, 10:48 a.m. UTC

This patch-set improves the use of eLCDIF block on iMX 8 SoCs (like 8MQ, 8MM
and 8QXP). Following, are the new features added and fixes from this
patch-set:

1. Add support for drm_bridge
On 8MQ and 8MM, the LCDIF block is not directly connected to a parallel
display connector, where an LCD panel can be attached, but instead it is
connected to DSI controller. Since this DSI stands between the display
controller (eLCDIF) and the physical connector, the DSI can be implemented
as a DRM bridge. So, in order to be able to connect the mxsfb driver to
the DSI driver, the support for a drm_bridge was needed in mxsfb DRM
driver (the actual driver for the eLCDIF block).

2. Add support for additional pixel formats
Some of the pixel formats needed by Android were not implemented in this
driver, but they were actually supported. So, add support for them.

3. Add support for horizontal stride
Having support for horizontal stride allows the use of eLCDIF with a GPU
(for example) that can only output resolution sizes multiple of a power of
8. For example, 1080 is not a power of 16, so in order to support 1920x1080
output from GPUs that can produce linear buffers only in sizes multiple to 16,
this feature is needed.

3. Few minor features and bug-fixing
The addition of max-res DT property was actually needed in order to limit
the bandwidth usage of the eLCDIF block. This is need on systems where
multiple display controllers are presend and the memory bandwidth is not
enough to handle all of them at maximum capacity (like it is the case on
8MQ, where there are two display controllers: DCSS and eLCDIF).
The rest of the patches are bug-fixes.

v2:
- Collected Tested-by from Guido
- Split the first patch, which added more than one feature into relevant
  patches, explaining each feature added
- Also split the second patch into more patches, to differentiate between
  the feature itself (additional pixel formats support) and the cleanup
  of the register definitions for a better representation (guido)
- Included a patch submitted by Guido, while he was testing my patch-set

Guido Günther (1):
  drm/mxsfb: Read bus flags from bridge if present

Mirela Rabulea (1):
  drm/mxsfb: Signal mode changed when bpp changed

Robert Chiras (13):
  drm/mxsfb: Update mxsfb to support a bridge
  drm/mxsfb: Add defines for the rest of registers
  drm/mxsfb: Reset vital register for a proper initialization
  drm/mxsfb: Update register definitions using bit manipulation defines
  drm/mxsfb: Update mxsfb with additional pixel formats
  drm/mxsfb: Fix the vblank events
  dt-bindings: display: Add max-res property for mxsfb
  drm/mxsfb: Add max-res property for MXSFB
  drm/mxsfb: Update mxsfb to support LCD reset
  drm/mxsfb: Improve the axi clock usage
  drm/mxsfb: Clear OUTSTANDING_REQS bits
  drm/mxsfb: Add support for horizontal stride
  drm/mxsfb: Add support for live pixel format change

 .../devicetree/bindings/display/mxsfb.txt          |   6 +
 drivers/gpu/drm/mxsfb/mxsfb_crtc.c                 | 287 ++++++++++++++++++---
 drivers/gpu/drm/mxsfb/mxsfb_drv.c                  | 190 +++++++++++---
 drivers/gpu/drm/mxsfb/mxsfb_drv.h                  |  10 +-
 drivers/gpu/drm/mxsfb/mxsfb_out.c                  |  26 +-
 drivers/gpu/drm/mxsfb/mxsfb_regs.h                 | 193 +++++++++-----
 6 files changed, 573 insertions(+), 139 deletions(-)

Comments

Robert Chiras Aug. 14, 2019, 11:38 a.m. UTC | #1

Hi Stefan,

On Mi, 2019-08-14 at 13:11 +0200, Stefan Agner wrote:
> On 2019-08-14 12:48, Robert Chiras wrote:
> > 
> > Some of the regiters need, like LCDC_CTRL and
> > CTRL2_OUTSTANDING_REQS
> Typo in registers, and there is a need to many.
Thanks, will fix this.
> 
> > 
> > needs to be properly cleared and initialized for a better start and
> > stop
> > routine.
> 
> 
> > 
> > 
> > Signed-off-by: Robert Chiras <[e-mail]>
> > ---
> >  drivers/gpu/drm/mxsfb/mxsfb_crtc.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c
> > b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c
> > index b69ace8..5e44f57 100644
> > --- a/drivers/gpu/drm/mxsfb/mxsfb_crtc.c
> > +++ b/drivers/gpu/drm/mxsfb/mxsfb_crtc.c
> > @@ -127,6 +127,10 @@ static void mxsfb_enable_controller(struct
> > mxsfb_drm_private *mxsfb)
> >               clk_prepare_enable(mxsfb->clk_disp_axi);
> >       clk_prepare_enable(mxsfb->clk);
> > 
> > +     if (mxsfb->devdata->ipversion >= 4)
> > +             writel(CTRL2_OUTSTANDING_REQS(REQ_16),
> > +                    mxsfb->base + LCDC_V4_CTRL2 + REG_SET);
> > +
> >       /* If it was disabled, re-enable the mode again */
> >       writel(CTRL_DOTCLK_MODE, mxsfb->base + LCDC_CTRL + REG_SET);
> > 
> > @@ -136,12 +140,19 @@ static void mxsfb_enable_controller(struct
> > mxsfb_drm_private *mxsfb)
> >       writel(reg, mxsfb->base + LCDC_VDCTRL4);
> > 
> >       writel(CTRL_RUN, mxsfb->base + LCDC_CTRL + REG_SET);
> > +     writel(CTRL1_RECOVERY_ON_UNDERFLOW, mxsfb->base + LCDC_CTRL1
> > + REG_SET);
> This seems not to be accounted for in the commit message. Can you do
> this in a separate commit?
> 
> Also I suggest to introduce CTRL1_RECOVERY_ON_UNDERFLOW in that same
> commit.
You are right, I missed this one in the description. I will add this
one too.
> 
> --
> Stefan
> 
> > 
> >  }
> > 
> >  static void mxsfb_disable_controller(struct mxsfb_drm_private
> > *mxsfb)
> >  {
> >       u32 reg;
> > 
> > +     if (mxsfb->devdata->ipversion >= 4)
> > +             writel(CTRL2_OUTSTANDING_REQS(0x7),
> > +                    mxsfb->base + LCDC_V4_CTRL2 + REG_CLR);
> > +
> > +     writel(CTRL_RUN, mxsfb->base + LCDC_CTRL + REG_CLR);
> > +
> >       /*
> >        * Even if we disable the controller here, it will still
> > continue
> >        * until its FIFOs are running out of data
> > @@ -295,6 +306,7 @@ void mxsfb_crtc_enable(struct mxsfb_drm_private
> > *mxsfb)
> >       dma_addr_t paddr;
> > 
> >       mxsfb_enable_axi_clk(mxsfb);
> > +     writel(0, mxsfb->base + LCDC_CTRL);
> >       mxsfb_crtc_mode_set_nofb(mxsfb);
> > 
> >       /* Write cur_buf as well to avoid an initial corrupt frame */

Thanks,
Robert

Robert Chiras Aug. 14, 2019, 12:03 p.m. UTC | #2

Hi Stefann,
On Mi, 2019-08-14 at 13:25 +0200, Stefan Agner wrote:
> On 2019-08-14 13:14, Robert Chiras wrote:
> > 
> > Hi Stefan,
> > On Mi, 2019-08-14 at 13:03 +0200, Stefan Agner wrote:
> > > 
> > > On 2019-08-14 12:48, Robert Chiras wrote:
> > > > 
> > > > 
> > > > Add new optional property 'max-res', to limit the maximum
> > > > supported
> > > > resolution by the MXSFB_DRM driver.
> > > I would also mention the reason why we need this.
> > > 
> > > I guess this needs a vendor prefix as well (fsl,max-res). I also
> > > would
> > > like to have the ack of the device tree folks here.
> > Rob Herring also aked be about this, and I'll copy here the reply,
> > with
> > explanations:
> > 
> > Indeed, this limitation is actually due to bandwidth limitation,
> > but
> > the problem is that this limitation comes on i.MX8M (known as
> > mScale
> > 850D), where the memory bandwidth cannot support: GPU/VPU workload
> > in
> > the same time with both DCSS driving 4k@60 and eLCDIF driving 1080p
> > @60.
> > Since eLCDIF is a secondary display we though to add the posibility
> > to
> > limit it's bandwidth by limiting the resolution.
> > If you say that more details are needed, I can add them in the
> > description.
> Oh sorry I missed that.
> 
> Rob Herring also wrote:
> > 
> > I suppose what you are after is bandwidth limits? IIRC, there's
> > already
> > some bindings expressing such limits. Also, wouldn't you need to
> > account
> > for bpp and using the 2nd plane (IIRC that there is one).
> I guess the binding he refers to is max-memory-bandwidth, which is
> used
> in multiple driver already. It makes sense to reuse this property
> instead of inventing a new set of property which is also not taking
> bpp
> into account...
Actually I saw this binding, but I thought it is strictly related to
that driver, where it is documented. Now, that you say this one is used
in many drivers (and recalling that you suggested for my 'max-res' to
add a prefix), I get it now.
> 
> The pl111 driver implements this property, it should be fairly easy
> to
> adopt that code.
Sure, I will do something similar here, too.
> 
> --
> Stefan
> 
> 
> 
> > 
> > > 
> > > 
> > > --
> > > Stefan
> > > 
> > > > 
> > > > 
> > > > 
> > > > Signed-off-by: Robert Chiras <[e-mail]>
> > > > ---
> > > >  Documentation/devicetree/bindings/display/mxsfb.txt | 6 ++++++
> > > >  1 file changed, 6 insertions(+)
> > > > 
> > > > diff --git
> > > > a/Documentation/devicetree/bindings/display/mxsfb.txt
> > > > b/Documentation/devicetree/bindings/display/mxsfb.txt
> > > > index 472e1ea..55e22ed 100644
> > > > --- a/Documentation/devicetree/bindings/display/mxsfb.txt
> > > > +++ b/Documentation/devicetree/bindings/display/mxsfb.txt
> > > > @@ -17,6 +17,12 @@ Required properties:
> > > >  Required sub-nodes:
> > > >    - port: The connection to an encoder chip.
> > > > 
> > > > +Optional properties:
> > > > +- max-res:   an array with a maximum of two integers,
> > > > representing
> > > > the
> > > > +             maximum supported resolution, in the form of
> > > > +             <maxX>, <maxY>; if one of the item is <0>, the
> > > > default
> > > > +             driver-defined maximum resolution for that axis
> > > > is
> > > > used
> > > > +
> > > >  Example:
> > > > 
> > > >       lcdif1: display-controller@2220000 {
> > Thanks,
> > Robert