Message-ID: <20190516082350.GB15144@e110455-lin.cambridge.arm.com>
Date:   Thu, 16 May 2019 09:23:50 +0100
From:   "liviu.dudau@....com" <liviu.dudau@....com>
To:     Wen He <wen.he_1@....com>
Cc:     "dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Leo Li <leoyang.li@....com>
Subject: Re: [EXT] Re: [v1] drm/arm/mali-dp: Disable checking for required
 pixel clock rate

On Thu, May 16, 2019 at 08:10:21AM +0000, Wen He wrote:
> 
> 
> > -----Original Message-----
> > From: liviu.dudau@....com [mailto:liviu.dudau@....com]
> > Sent: 2019年5月15日 23:46
> > To: Wen He <wen.he_1@....com>
> > Cc: dri-devel@...ts.freedesktop.org; linux-kernel@...r.kernel.org; Leo Li
> > <leoyang.li@....com>
> > Subject: [EXT] Re: [v1] drm/arm/mali-dp: Disable checking for required pixel
> > clock rate
> > 
> > 
> > Hi Wen,
> 
> Hi Liviu,
> 
> > 
> > On Wed, May 15, 2019 at 02:42:08AM +0000, Wen He wrote:
> > > Disable checking for required pixel clock rate if ARCH_LAYERSCPAE is
> > > enable.
> > >
> > > Signed-off-by: Alison Wang <alison.wang@....com>
> > > Signed-off-by: Wen He <wen.he_1@....com>
> > > ---
> > > change in description:
> > >       - This check that only supported one pixel clock required clock rate
> > >       compare with dts node value. but we have supports 4 pixel clock
> > >       for ls1028a board.
> > 
> > So, your DT says your pixel clock provider is a fixed clock? If you support more
> > than one rate, you should instead use a real provider for it. How do you
> > support the 4 pixel clocks?
> > 
>  
> Yes , the DT node only can provided one pixel clock by using a fixed clock.
> But we Display Port controller support 4 or more resolutions, each of which
> requires a set of pixel clocks to drive, and we hope they can switch any resolution
> we want by some program every times.

That program can't be some userspace application, because it will have to make
changes to the hardware and the kernel will not know that things have changed
under its feet. That leaves the option of the bootloader or some other kernel
module doing the changes.

If you have another kernel module that knows how to change clocks, that should
be implemented using the common clocks infrastructure, at which time you can
put it in the DT as the clock provider for the pixelclock.

If the bootloader does the changes, then the bootloader should edit the DT and
set the correct value for the pixel clock. Regardless, with your change and on
your platform the user can request any resolution and the driver will silently
fail to set that resolution.

One other problem is the one Robin raised, where the kernel is compiled for
multiple platforms, like what various Linux distributions do. That kernel will
either work on other SoC or not, depending on what CONFIG_ARCH_LAYERSCAPE is
set to.

In summary, for this patch, it's a NAK. There are proper ways of achieving what
you need, but this patch is not.

Best regards,
Liviu

> 
> For example, if we set that fixed pixel clock is 27000000 (27Mhz), but user hope can see
> a group 1080p resolution penguins during startup , and hope playing a 4k video once
> system boot up done. 
> Btw, In our board, the 1080p resolution is driven by a 148.5Mhz pixel clock, 4k is driven
> by a 594Mhz. 27Mhz only can drive 480p resolution.
> 
> To meet the above user requirements, I was to setup following steps,
> 1. Add the "video=1920x1080-32@60" to bootargs command line [specify penguins size]
> 2. Play a 4K video with 4k resolution when system boot up done.
> 
> > Also, not sure what the paragraph above is meant to be. Should it be part of
> > the commit message?
> > 
> 
> These comments just want to let you know.
> 
> > Best regards,
> > Liviu
> > 
> > 
> > >  drivers/gpu/drm/arm/malidp_crtc.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/arm/malidp_crtc.c
> > > b/drivers/gpu/drm/arm/malidp_crtc.c
> > > index 56aad288666e..bb79223d9981 100644
> > > --- a/drivers/gpu/drm/arm/malidp_crtc.c
> > > +++ b/drivers/gpu/drm/arm/malidp_crtc.c
> > > @@ -36,11 +36,13 @@ static enum drm_mode_status
> > > malidp_crtc_mode_valid(struct drm_crtc *crtc,
> > >
> 
> According to our pixel configuration above,
> Now the variable req_rate value is 148500000 or 59400000, another variable rate value is
> 27000000, so we will get a warning and display will cannot works well. 
> 
> We're not sure which resolution are user want, and we also can't just offered one resolution
> to user. so I remove this check on our board, maybe it's not good change.
> 
> I want to know do you have other good suggestion? Thanks.
> 
> Best Regards,
> Wen
> 
> > >       if (req_rate) {
> > >               rate = clk_round_rate(hwdev->pxlclk, req_rate);
> > > +#ifndef CONFIG_ARCH_LAYERSCAPE
> > >               if (rate != req_rate) {
> > >                       DRM_DEBUG_DRIVER("pxlclk doesn't support %ld
> > Hz\n",
> > >                                        req_rate);
> > >                       return MODE_NOCLOCK;
> > >               }
> > > +#endif
> > >       }
> > >
> > >       return MODE_OK;
> > > --
> > > 2.17.1
> > >
> > 
> > --
> > ====================
> > | I would like to |
> > | fix the world,  |
> > | but they're not |
> > | giving me the   |
> >  \ source code!  /
> >   ---------------
> >     ¯\_(ツ)_/¯

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

Powered by blists - more mailing lists