[v1] drm/arm/mali-dp: Disable checking for required pixel clock rate

[v1] drm/arm/mali-dp: Disable checking for required pixel clock rate

[Wen He]

Disable checking for required pixel clock rate if ARCH_LAYERSCPAE
is enable.

Signed-off-by: Alison Wang <alison.wang@nxp.com>
Signed-off-by: Wen He <wen.he_1@nxp.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.
 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,
 
 	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

Re: [v1] drm/arm/mali-dp: Disable checking for required pixel clock rate

[liviu.dudau]

Hi Wen,

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@nxp.com>
> Signed-off-by: Wen He <wen.he_1@nxp.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?

Also, not sure what the paragraph above is meant to be. Should it be part of
the commit message?

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,
>  
>  	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!  /
  ---------------
    ¯\_(ツ)_/¯

Re: [v1] drm/arm/mali-dp: Disable checking for required pixel clock rate

[Robin Murphy]

On 15/05/2019 03:42, Wen He wrote:
> Disable checking for required pixel clock rate if ARCH_LAYERSCPAE
> is enable.
> 
> Signed-off-by: Alison Wang <alison.wang@nxp.com>
> Signed-off-by: Wen He <wen.he_1@nxp.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.
>   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,
>   
>   	if (req_rate) {
>   		rate = clk_round_rate(hwdev->pxlclk, req_rate);
> +#ifndef CONFIG_ARCH_LAYERSCAPE

What about multiplatform builds? The kernel config doesn't tell you what 
hardware you're actually running on.

Robin.

>   		if (rate != req_rate) {
>   			DRM_DEBUG_DRIVER("pxlclk doesn't support %ld Hz\n",
>   					 req_rate);
>   			return MODE_NOCLOCK;
>   		}
> +#endif
>   	}
>   
>   	return MODE_OK;
> 

RE: [EXT] Re: [v1] drm/arm/mali-dp: Disable checking for required pixel clock rate

[Wen He]

> -----Original Message-----
> From: liviu.dudau@arm.com [mailto:liviu.dudau@arm.com]
> Sent: 2019年5月15日 23:46
> To: Wen He <wen.he_1@nxp.com>
> Cc: dri-devel@lists.freedesktop.org; linux-kernel@vger.kernel.org; Leo Li
> <leoyang.li@nxp.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@nxp.com>
> > Signed-off-by: Wen He <wen.he_1@nxp.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.

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!  /
>   ---------------
>     ¯\_(ツ)_/¯

Re: [EXT] Re: [v1] drm/arm/mali-dp: Disable checking for required pixel clock rate

[liviu.dudau]

On Thu, May 16, 2019 at 08:10:21AM +0000, Wen He wrote:
> 
> 
> > -----Original Message-----
> > From: liviu.dudau@arm.com [mailto:liviu.dudau@arm.com]
> > Sent: 2019年5月15日 23:46
> > To: Wen He <wen.he_1@nxp.com>
> > Cc: dri-devel@lists.freedesktop.org; linux-kernel@vger.kernel.org; Leo Li
> > <leoyang.li@nxp.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@nxp.com>
> > > Signed-off-by: Wen He <wen.he_1@nxp.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!  /
  ---------------
    ¯\_(ツ)_/¯

RE: [EXT] Re: [v1] drm/arm/mali-dp: Disable checking for required pixel clock rate

[Wen He]

> -----Original Message-----
> From: Robin Murphy [mailto:robin.murphy@arm.com]
> Sent: 2019年5月16日 1:14
> To: Wen He <wen.he_1@nxp.com>; dri-devel@lists.freedesktop.org;
> linux-kernel@vger.kernel.org; liviu.dudau@arm.com
> Cc: Leo Li <leoyang.li@nxp.com>
> Subject: [EXT] Re: [v1] drm/arm/mali-dp: Disable checking for required pixel
> clock rate
> 
> Caution: EXT Email
> 
> On 15/05/2019 03:42, Wen He wrote:
> > Disable checking for required pixel clock rate if ARCH_LAYERSCPAE is
> > enable.
> >
> > Signed-off-by: Alison Wang <alison.wang@nxp.com>
> > Signed-off-by: Wen He <wen.he_1@nxp.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.
> >   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,
> >
> >       if (req_rate) {
> >               rate = clk_round_rate(hwdev->pxlclk, req_rate);
> > +#ifndef CONFIG_ARCH_LAYERSCAPE
> 
> What about multiplatform builds? The kernel config doesn't tell you what
> hardware you're actually running on.
> 

Hi Robin,

Thanks for your reply.

In fact, Only one platform integrates this IP when CONFIG_ARCH_LAYERSCAPE is set.
Although this are not good ways, but I think it won't be a problem under multiplatform builds.

Best Regards,
Wen

> Robin.
> 
> >               if (rate != req_rate) {
> >                       DRM_DEBUG_DRIVER("pxlclk doesn't support %ld
> Hz\n",
> >                                        req_rate);
> >                       return MODE_NOCLOCK;
> >               }
> > +#endif
> >       }
> >
> >       return MODE_OK;
> >

Re: [EXT] Re: [v1] drm/arm/mali-dp: Disable checking for required pixel clock rate

[Robin Murphy]

On 16/05/2019 10:42, Wen He wrote:
> 
> 
>> -----Original Message-----
>> From: Robin Murphy [mailto:robin.murphy@arm.com]
>> Sent: 2019年5月16日 1:14
>> To: Wen He <wen.he_1@nxp.com>; dri-devel@lists.freedesktop.org;
>> linux-kernel@vger.kernel.org; liviu.dudau@arm.com
>> Cc: Leo Li <leoyang.li@nxp.com>
>> Subject: [EXT] Re: [v1] drm/arm/mali-dp: Disable checking for required pixel
>> clock rate
>>
>> Caution: EXT Email
>>
>> On 15/05/2019 03:42, Wen He wrote:
>>> Disable checking for required pixel clock rate if ARCH_LAYERSCPAE is
>>> enable.
>>>
>>> Signed-off-by: Alison Wang <alison.wang@nxp.com>
>>> Signed-off-by: Wen He <wen.he_1@nxp.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.
>>>    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,
>>>
>>>        if (req_rate) {
>>>                rate = clk_round_rate(hwdev->pxlclk, req_rate);
>>> +#ifndef CONFIG_ARCH_LAYERSCAPE
>>
>> What about multiplatform builds? The kernel config doesn't tell you what
>> hardware you're actually running on.
>>
> 
> Hi Robin,
> 
> Thanks for your reply.
> 
> In fact, Only one platform integrates this IP when CONFIG_ARCH_LAYERSCAPE is set.
> Although this are not good ways, but I think it won't be a problem under multiplatform builds.

My point is that ARCH_LAYERSCAPE is going to be enabled in distribution 
kernels along with everything else, so you're effectively removing this 
check for all other vendors' Mali-DP implementations as well, which is 
probably not OK.

Furthermore, if LS1028A really only supports 4 specific modes as the BSP 
documentation I found claims, then surely you'd want a *more* specific 
check here, rather than no check at all?

Robin.

RE: [EXT] Re: [v1] drm/arm/mali-dp: Disable checking for required pixel clock rate

[Wen He]

> -----Original Message-----
> From: Robin Murphy [mailto:robin.murphy@arm.com]
> Sent: 2019年5月16日 18:45
> To: Wen He <wen.he_1@nxp.com>; dri-devel@lists.freedesktop.org;
> linux-kernel@vger.kernel.org; liviu.dudau@arm.com
> Cc: Leo Li <leoyang.li@nxp.com>
> Subject: Re: [EXT] Re: [v1] drm/arm/mali-dp: Disable checking for required
> pixel clock rate
> 
> 
> On 16/05/2019 10:42, Wen He wrote:
> >
> >
> >> -----Original Message-----
> >> From: Robin Murphy [mailto:robin.murphy@arm.com]
> >> Sent: 2019年5月16日 1:14
> >> To: Wen He <wen.he_1@nxp.com>; dri-devel@lists.freedesktop.org;
> >> linux-kernel@vger.kernel.org; liviu.dudau@arm.com
> >> Cc: Leo Li <leoyang.li@nxp.com>
> >> Subject: [EXT] Re: [v1] drm/arm/mali-dp: Disable checking for
> >> required pixel clock rate
> >>
> >> Caution: EXT Email
> >>
> >> On 15/05/2019 03:42, Wen He wrote:
> >>> Disable checking for required pixel clock rate if ARCH_LAYERSCPAE is
> >>> enable.
> >>>
> >>> Signed-off-by: Alison Wang <alison.wang@nxp.com>
> >>> Signed-off-by: Wen He <wen.he_1@nxp.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.
> >>>    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,
> >>>
> >>>        if (req_rate) {
> >>>                rate = clk_round_rate(hwdev->pxlclk, req_rate);
> >>> +#ifndef CONFIG_ARCH_LAYERSCAPE
> >>
> >> What about multiplatform builds? The kernel config doesn't tell you
> >> what hardware you're actually running on.
> >>
> >
> > Hi Robin,
> >
> > Thanks for your reply.
> >
> > In fact, Only one platform integrates this IP when
> CONFIG_ARCH_LAYERSCAPE is set.
> > Although this are not good ways, but I think it won't be a problem under
> multiplatform builds.
> 
> My point is that ARCH_LAYERSCAPE is going to be enabled in distribution
> kernels along with everything else, so you're effectively removing this check for
> all other vendors' Mali-DP implementations as well, which is probably not OK.
> 
> Furthermore, if LS1028A really only supports 4 specific modes as the BSP
> documentation I found claims, then surely you'd want a *more* specific check
> here, rather than no check at all?

Hi Robin,

Thanks for your comments.

Yes, As you said, now LS1028A only supports 4 modes, and we use three clocks to support
all four modes. In fact, this was really the point.

However, we can only enable one mode to meet the check statement in this place.

For example, If user has a 1080p monitor, we must be set the pixel fixed-clock to 148.5MHz. 
if user want to choice 4k monitor, we also to be change the pixel fixed-clock to 594MHz in
DT node. In reality, We have no way of knowing what kind of monitor the user wants. Right?

Moreover, user cannot to change screen resolution in this case, I don't think this place is
reasonable. we need to supporting Ubuntu , Wayland and other embedded GU, so we need
to switch the resolutions.

Maybe it's that most android device used, and android system always only need one
resolution.

Best Regards,
Wen

> 
> Robin.

RE: [EXT] Re: [v1] drm/arm/mali-dp: Disable checking for required pixel clock rate

[Wen He]

> -----Original Message-----
> From: liviu.dudau@arm.com [mailto:liviu.dudau@arm.com]
> Sent: 2019年5月16日 16:24
> To: Wen He <wen.he_1@nxp.com>
> Cc: dri-devel@lists.freedesktop.org; linux-kernel@vger.kernel.org; Leo Li
> <leoyang.li@nxp.com>
> Subject: Re: [EXT] Re: [v1] drm/arm/mali-dp: Disable checking for required
> pixel clock rate
> 
> Caution: EXT Email
> 
> On Thu, May 16, 2019 at 08:10:21AM +0000, Wen He wrote:
> >
> >
> > > -----Original Message-----
> > > From: liviu.dudau@arm.com [mailto:liviu.dudau@arm.com]
> > > Sent: 2019年5月15日 23:46
> > > To: Wen He <wen.he_1@nxp.com>
> > > Cc: dri-devel@lists.freedesktop.org; linux-kernel@vger.kernel.org;
> > > Leo Li <leoyang.li@nxp.com>
> > > Subject: [EXT] Re: [v1] drm/arm/mali-dp: Disable checking for
> > > required pixel clock rate
> > >
> > >
> > > Hi Wen,
> >
> > Hi Liviu,
> >

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@nxp.com>
> > > > Signed-off-by: Wen He <wen.he_1@nxp.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.
> 

Hi Liviu,

Yes, I think you are right, and even though we didn't implement clocks prepare
/enable/disable interface, but we can notification hardware to change pixel clock
by determining the required pixel clock in each mode once had modeset event
in DP driver.

But the point is how do we meet the conditions for the clock rate check in here? 

As you know, we LS1028A supports 4 modes, every resolution change will to
determine whether the hardware supports the pixel clock required for the resolution
by calling this function malidp_crtc_mode_valid() . assume if we put fixed-clock in DT
node that will can't meet this checking.

Best Regards,
Wen

> 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!  /
>   ---------------
>     ¯\_(ツ)_/¯

Re: [EXT] Re: [v1] drm/arm/mali-dp: Disable checking for required pixel clock rate

[liviu.dudau]

On Fri, May 17, 2019 at 10:37:01AM +0000, Wen He wrote:
> 
> 
> > -----Original Message-----
> > From: Robin Murphy [mailto:robin.murphy@arm.com]
> > Sent: 2019年5月16日 18:45
> > To: Wen He <wen.he_1@nxp.com>; dri-devel@lists.freedesktop.org;
> > linux-kernel@vger.kernel.org; liviu.dudau@arm.com
> > Cc: Leo Li <leoyang.li@nxp.com>
> > Subject: Re: [EXT] Re: [v1] drm/arm/mali-dp: Disable checking for required
> > pixel clock rate
> > 
> > 
> > On 16/05/2019 10:42, Wen He wrote:
> > >
> > >
> > >> -----Original Message-----
> > >> From: Robin Murphy [mailto:robin.murphy@arm.com]
> > >> Sent: 2019年5月16日 1:14
> > >> To: Wen He <wen.he_1@nxp.com>; dri-devel@lists.freedesktop.org;
> > >> linux-kernel@vger.kernel.org; liviu.dudau@arm.com
> > >> Cc: Leo Li <leoyang.li@nxp.com>
> > >> Subject: [EXT] Re: [v1] drm/arm/mali-dp: Disable checking for
> > >> required pixel clock rate
> > >>
> > >> Caution: EXT Email
> > >>
> > >> On 15/05/2019 03:42, Wen He wrote:
> > >>> Disable checking for required pixel clock rate if ARCH_LAYERSCPAE is
> > >>> enable.
> > >>>
> > >>> Signed-off-by: Alison Wang <alison.wang@nxp.com>
> > >>> Signed-off-by: Wen He <wen.he_1@nxp.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.
> > >>>    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,
> > >>>
> > >>>        if (req_rate) {
> > >>>                rate = clk_round_rate(hwdev->pxlclk, req_rate);
> > >>> +#ifndef CONFIG_ARCH_LAYERSCAPE
> > >>
> > >> What about multiplatform builds? The kernel config doesn't tell you
> > >> what hardware you're actually running on.
> > >>
> > >
> > > Hi Robin,
> > >
> > > Thanks for your reply.
> > >
> > > In fact, Only one platform integrates this IP when
> > CONFIG_ARCH_LAYERSCAPE is set.
> > > Although this are not good ways, but I think it won't be a problem under
> > multiplatform builds.
> > 
> > My point is that ARCH_LAYERSCAPE is going to be enabled in distribution
> > kernels along with everything else, so you're effectively removing this check for
> > all other vendors' Mali-DP implementations as well, which is probably not OK.
> > 
> > Furthermore, if LS1028A really only supports 4 specific modes as the BSP
> > documentation I found claims, then surely you'd want a *more* specific check
> > here, rather than no check at all?
> 
> Hi Robin,
> 
> Thanks for your comments.
> 
> Yes, As you said, now LS1028A only supports 4 modes, and we use three clocks to support
> all four modes. In fact, this was really the point.
> 
> However, we can only enable one mode to meet the check statement in this place.
> 
> For example, If user has a 1080p monitor, we must be set the pixel fixed-clock to 148.5MHz. 
> if user want to choice 4k monitor, we also to be change the pixel fixed-clock to 594MHz in
> DT node. In reality, We have no way of knowing what kind of monitor the user wants. Right?

How does your DT know which monitor the user is going to plug in? Like I've
said, if you expose the mechanism you use to set the fixed-clock to a certain
value via the clk provider then you will be able to switch automatically to
that frequency without your patch.

Best regards,
Liviu

> 
> Moreover, user cannot to change screen resolution in this case, I don't think this place is
> reasonable. we need to supporting Ubuntu , Wayland and other embedded GU, so we need
> to switch the resolutions.
> 
> Maybe it's that most android device used, and android system always only need one
> resolution.
> 
> Best Regards,
> Wen
> 
> > 
> > Robin.

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

Re: [EXT] Re: [v1] drm/arm/mali-dp: Disable checking for required pixel clock rate

[liviu.dudau]

On Fri, May 17, 2019 at 10:38:00AM +0000, Wen He wrote:
> 
> 
> > -----Original Message-----
> > From: liviu.dudau@arm.com [mailto:liviu.dudau@arm.com]
> > Sent: 2019年5月16日 16:24
> > To: Wen He <wen.he_1@nxp.com>
> > Cc: dri-devel@lists.freedesktop.org; linux-kernel@vger.kernel.org; Leo Li
> > <leoyang.li@nxp.com>
> > Subject: Re: [EXT] Re: [v1] drm/arm/mali-dp: Disable checking for required
> > pixel clock rate
> > 
> > Caution: EXT Email
> > 
> > On Thu, May 16, 2019 at 08:10:21AM +0000, Wen He wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: liviu.dudau@arm.com [mailto:liviu.dudau@arm.com]
> > > > Sent: 2019年5月15日 23:46
> > > > To: Wen He <wen.he_1@nxp.com>
> > > > Cc: dri-devel@lists.freedesktop.org; linux-kernel@vger.kernel.org;
> > > > Leo Li <leoyang.li@nxp.com>
> > > > Subject: [EXT] Re: [v1] drm/arm/mali-dp: Disable checking for
> > > > required pixel clock rate
> > > >
> > > >
> > > > Hi Wen,
> > >
> > > Hi Liviu,
> > >
> 
> 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@nxp.com>
> > > > > Signed-off-by: Wen He <wen.he_1@nxp.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.
> > 
> 
> Hi Liviu,

Hi Wen,

> 
> Yes, I think you are right, and even though we didn't implement clocks prepare
> /enable/disable interface, but we can notification hardware to change pixel clock
> by determining the required pixel clock in each mode once had modeset event
> in DP driver.
> 
> But the point is how do we meet the conditions for the clock rate check in here? 

You don't need to change anything in this driver, your clock provider will only
set the values it supports. So in malidp_crtc_atomic_enable() we set the
desired pxlclk, but your provider will drop/refuse the change, so in
malidp_crtc_mode_valid() only the 4 supported resolutions will pass, all other
modes will fail.

> 
> As you know,

I don't, I still don't have a LS1028A platform ;)

Best regards,
Liviu


>  we LS1028A supports 4 modes, every resolution change will to
> determine whether the hardware supports the pixel clock required for the resolution
> by calling this function malidp_crtc_mode_valid() . assume if we put fixed-clock in DT
> node that will can't meet this checking.
> 
> Best Regards,
> Wen
> 
> > 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!  /
> >   ---------------
> >     ¯\_(ツ)_/¯

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