Linux-Renesas-SoC Archive on lore.kernel.org
 help / color / Atom feed
From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
To: Jacopo Mondi <jacopo@jmondi.org>
Cc: Jacopo Mondi <jacopo+renesas@jmondi.org>,
	laurent.pinchart@ideasonboard.com, geert@linux-m68k.org,
	horms@verge.net.au, uli+renesas@fpond.eu,
	VenkataRajesh.Kalakodima@in.bosch.com, airlied@linux.ie,
	daniel@ffwll.ch, koji.matsuoka.xm@renesas.com, muroya@ksk.co.jp,
	Harsha.ManjulaMallikarjun@in.bosch.com,
	linux-renesas-soc@vger.kernel.org,
	dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4 6/9] drm: rcar-du: crtc: Enable and disable CMMs
Date: Thu, 12 Sep 2019 10:19:30 +0100
Message-ID: <a0f0554d-1bb0-fefa-a5d5-a252f7426c36@ideasonboard.com> (raw)
In-Reply-To: <20190912080720.bn7bmu2o3gacrpf4@uno.localdomain>

Hi Jacopo,

On 12/09/2019 09:07, Jacopo Mondi wrote:
> Hi Kieran,
> 
> On Wed, Sep 11, 2019 at 07:40:27PM +0100, Kieran Bingham wrote:
>> Hi Jacopo,
>>
>> On 06/09/2019 14:54, Jacopo Mondi wrote:
>>> Enable/disable the CMM associated with a CRTC at CRTC start and stop
>>> time and enable the CMM unit through the Display Extensional Functions
>>> register at group setup time.
>>>
>>> Reviewed-by: Ulrich Hecht <uli+renesas@fpond.eu>
>>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>>> ---
>>>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c  | 7 +++++++
>>>  drivers/gpu/drm/rcar-du/rcar_du_group.c | 8 ++++++++
>>>  drivers/gpu/drm/rcar-du/rcar_du_regs.h  | 5 +++++
>>>  3 files changed, 20 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
>>> index 23f1d6cc1719..3dac605c3a67 100644
>>> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
>>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
>>> @@ -21,6 +21,7 @@
>>>  #include <drm/drm_plane_helper.h>
>>>  #include <drm/drm_vblank.h>
>>>
>>> +#include "rcar_cmm.h"
>>>  #include "rcar_du_crtc.h"
>>>  #include "rcar_du_drv.h"
>>>  #include "rcar_du_encoder.h"
>>> @@ -619,6 +620,9 @@ static void rcar_du_crtc_stop(struct rcar_du_crtc *rcrtc)
>>>  	if (rcar_du_has(rcrtc->dev, RCAR_DU_FEATURE_VSP1_SOURCE))
>>>  		rcar_du_vsp_disable(rcrtc);
>>>
>>> +	if (rcrtc->cmm)
>>> +		rcar_cmm_disable(rcrtc->cmm);
>>> +
>>>  	/*
>>>  	 * Select switch sync mode. This stops display operation and configures
>>>  	 * the HSYNC and VSYNC signals as inputs.
>>> @@ -686,6 +690,9 @@ static void rcar_du_crtc_atomic_enable(struct drm_crtc *crtc,
>>>  	}
>>>
>>>  	rcar_du_crtc_start(rcrtc);
>>> +
>>> +	if (rcrtc->cmm)
>>> +		rcar_cmm_enable(rcrtc->cmm);
>>>  }
>>>
>>>  static void rcar_du_crtc_atomic_disable(struct drm_crtc *crtc,
>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_group.c b/drivers/gpu/drm/rcar-du/rcar_du_group.c
>>> index 9eee47969e77..25d0fc125d7a 100644
>>> --- a/drivers/gpu/drm/rcar-du/rcar_du_group.c
>>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_group.c
>>> @@ -147,6 +147,14 @@ static void rcar_du_group_setup(struct rcar_du_group *rgrp)
>>>
>>>  	rcar_du_group_setup_pins(rgrp);
>>>
>>> +	if (rcar_du_has(rcdu, RCAR_DU_FEATURE_CMM)) {
>>> +		u32 defr7 = DEFR7_CODE
>>> +			  | (rgrp->cmms_mask & BIT(1) ? DEFR7_CMME1 : 0)
>>> +			  | (rgrp->cmms_mask & BIT(0) ? DEFR7_CMME0 : 0);
>>> +
>>> +		rcar_du_group_write(rgrp, DEFR7, defr7);
>>> +	}
>>> +
>>
>> What's the effect here on platforms with a CMM, but with
>> CONFIG_DRM_RCAR_CMM unset?
>>
>> Will this incorrectly configure the DU ?
>>
>> Will it stall the display if the DU tries to interact with another
>> module which is not enabled?
> 
> I recall I tested that (that's why I had to add stubs for CMM
> functions, as I had linkage errors otherwise) and thing seems to be
> fine as the CMM configuration/enblement resolve to an empty function.

Yes, I see the stubs to allow for linkage, but it's the hardware I'm
concerned about. If it passes the tests and doesn't break then that's
probably ok ... but I'm really weary that we're enabling a hardware
pipeline with a disabled component in the middle.


> Would you prefer to have this guarded by an #if IS_ENABLED() ?

I don't think we need a compile time conditional, but I'd say it
probably needs to be more about whether the CMM has actually probed or not

Aha, and I see that in rcar_du_cmm_init() we already do a
call to rcar_cmm_init(), which if fails will leave rcdu->cmms[i] as
NULL. So that's catered for, which results in the rgrp->cmms_mask being
correctly representative of whether there is a CMM connected or not.

 ... so I think that means the ...
 "if (rcar_du_has(rcdu, RCAR_DU_FEATURE_CMM))" is somewhat redundant:


This could be:

  if (rgrp->cmms_mask) {
	u32 defr7 = DEFR7_CODE
		  | (rgrp->cmms_mask & BIT(1) ? DEFR7_CMME1 : 0)
		  | (rgrp->cmms_mask & BIT(0) ? DEFR7_CMME0 : 0);

  rcar_du_group_write(rgrp, DEFR7, defr7);

Or in fact, if we don't mind writing 0 to DEFR7 when there is no CMM
(which is safe by the looks of things as DEFR7 is available on all
platforms), then we can even remove the outer conditional, and leave
this all up to the ternary operators to write the correct value to the
defr7.


Phew ... net result - your current code *is* safe with the
CONFIG_DRM_RCAR_CMM option disabled. I'll leave it up to you if you want
to simplify the code here and remove the RCAR_DU_FEATURE_CMM.

As this RCAR_DU_FEATURE_CMM flag is only checked here, removing it would
however simplify all of the rcar_du_device_info structures.

So - with or without the _FEATURE_CMM" simplification, this patch looks
functional and safe so:


Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>


> Thanks
>    j
>>
>>
>>>  	if (rcdu->info->gen >= 2) {
>>>  		rcar_du_group_setup_defr8(rgrp);
>>>  		rcar_du_group_setup_didsr(rgrp);
>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_regs.h b/drivers/gpu/drm/rcar-du/rcar_du_regs.h
>>> index bc87f080b170..fb9964949368 100644
>>> --- a/drivers/gpu/drm/rcar-du/rcar_du_regs.h
>>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_regs.h
>>> @@ -197,6 +197,11 @@
>>>  #define DEFR6_MLOS1		(1 << 2)
>>>  #define DEFR6_DEFAULT		(DEFR6_CODE | DEFR6_TCNE1)
>>>
>>> +#define DEFR7			0x000ec
>>> +#define DEFR7_CODE		(0x7779 << 16)
>>> +#define DEFR7_CMME1		BIT(6)
>>> +#define DEFR7_CMME0		BIT(4)
>>> +
>>>  /* -----------------------------------------------------------------------------
>>>   * R8A7790-only Control Registers
>>>   */
>>>
>>


  reply index

Thread overview: 38+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-09-06 13:54 [PATCH v4 0/9 drm: rcar-du: Add Color Management Module (CMM) Jacopo Mondi
2019-09-06 13:54 ` [PATCH v4 1/9] dt-bindings: display: renesas,cmm: Add R-Car CMM documentation Jacopo Mondi
2019-09-11 19:04   ` Kieran Bingham
2019-09-17 17:55   ` Rob Herring
2019-09-18 22:08   ` Laurent Pinchart
2019-09-06 13:54 ` [PATCH v4 2/9] dt-bindings: display, renesas,du: Document cmms property Jacopo Mondi
2019-09-11 16:06   ` Kieran Bingham
2019-09-17 17:58     ` Rob Herring
2019-09-06 13:54 ` [PATCH v4 3/9] drm: rcar-du: Add support for CMM Jacopo Mondi
2019-09-11 15:54   ` Kieran Bingham
2019-09-12  7:59     ` Jacopo Mondi
2019-09-12  9:49       ` Kieran Bingham
2019-09-18 22:55   ` Laurent Pinchart
2019-09-19  8:59     ` Kieran Bingham
2019-09-20 22:43       ` Laurent Pinchart
2019-10-10 17:46     ` Jacopo Mondi
2019-10-14 23:54       ` Laurent Pinchart
2019-09-06 13:54 ` [PATCH v4 4/9] drm: rcar-du: Claim CMM support for Gen3 SoCs Jacopo Mondi
2019-09-11 18:45   ` Kieran Bingham
2019-09-06 13:54 ` [PATCH v4 5/9] drm: rcar-du: kms: Initialize CMM instances Jacopo Mondi
2019-09-12  9:27   ` Kieran Bingham
2019-09-06 13:54 ` [PATCH v4 6/9] drm: rcar-du: crtc: Enable and disable CMMs Jacopo Mondi
2019-09-11 18:40   ` Kieran Bingham
2019-09-12  8:07     ` Jacopo Mondi
2019-09-12  9:19       ` Kieran Bingham [this message]
2019-09-18 23:23         ` Laurent Pinchart
2019-09-19  8:08           ` Kieran Bingham
2019-09-20 22:44             ` Laurent Pinchart
2019-09-06 13:54 ` [PATCH v4 7/9] drm: rcar-du: crtc: Register GAMMA_LUT properties Jacopo Mondi
2019-09-11 18:47   ` Kieran Bingham
2019-09-06 13:54 ` [PATCH v4 8/9] drm: rcar-du: kms: Update CMM in atomic commit tail Jacopo Mondi
2019-09-12  9:51   ` Kieran Bingham
2019-09-20 22:49   ` Laurent Pinchart
2019-09-30 20:53   ` Ezequiel Garcia
2019-10-01 19:20     ` Laurent Pinchart
2019-09-06 13:54 ` [PATCH v4 9/9] arm64: dts: renesas: Add CMM units to Gen3 SoCs Jacopo Mondi
2019-09-11 18:16   ` Kieran Bingham
2019-09-11 19:35     ` Geert Uytterhoeven

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=a0f0554d-1bb0-fefa-a5d5-a252f7426c36@ideasonboard.com \
    --to=kieran.bingham+renesas@ideasonboard.com \
    --cc=Harsha.ManjulaMallikarjun@in.bosch.com \
    --cc=VenkataRajesh.Kalakodima@in.bosch.com \
    --cc=airlied@linux.ie \
    --cc=daniel@ffwll.ch \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=geert@linux-m68k.org \
    --cc=horms@verge.net.au \
    --cc=jacopo+renesas@jmondi.org \
    --cc=jacopo@jmondi.org \
    --cc=koji.matsuoka.xm@renesas.com \
    --cc=laurent.pinchart@ideasonboard.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-renesas-soc@vger.kernel.org \
    --cc=muroya@ksk.co.jp \
    --cc=uli+renesas@fpond.eu \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Renesas-SoC Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-renesas-soc/0 linux-renesas-soc/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-renesas-soc linux-renesas-soc/ https://lore.kernel.org/linux-renesas-soc \
		linux-renesas-soc@vger.kernel.org
	public-inbox-index linux-renesas-soc

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-renesas-soc


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git