| Message ID | 20190906135436.10622-7-jacopo+renesas@jmondi.org (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | [v4,1/9] dt-bindings: display: renesas,cmm: Add R-Car CMM documentation | expand |
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? > 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 > */ >
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. Would you prefer to have this guarded by an #if IS_ENABLED() ? 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 > > */ > > >
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 >>> */ >>> >>
Hello, On Thu, Sep 12, 2019 at 10:19:30AM +0100, Kieran Bingham wrote: > On 12/09/2019 09:07, Jacopo Mondi wrote: > > On Wed, Sep 11, 2019 at 07:40:27PM +0100, Kieran Bingham wrote: > >> 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. Doesn't this result in probe failure ? > ... 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> > > >>> 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 > >>> */
Hi Laurent, / Jacopo On 19/09/2019 00:23, Laurent Pinchart wrote: > Hello, > > On Thu, Sep 12, 2019 at 10:19:30AM +0100, Kieran Bingham wrote: >> On 12/09/2019 09:07, Jacopo Mondi wrote: >>> On Wed, Sep 11, 2019 at 07:40:27PM +0100, Kieran Bingham wrote: >>>> 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. > > Doesn't this result in probe failure ? I think I mis-spoke above, I didn't mean "if rcar_cmm_init() fails" I meant "if rcar_du_cmm_init() determines there are no connected CMM's or if they are disabled." If rcar_cmm_init() returns a failure, then yes we will fail to probe. But I think it's up to rcar_du_cmm_init() to determine if the CMM exists or not (or is enabled) and if that's not a failure case then it should not prevent the probing of the DU. In fact, I've now seen that if CONFIG_DRM_RCAR_CMM is not enabled, rcar_cmm_init() returns 0, and I think in fact it should return -ENODEV, with an exception on that return value in rcar_du_cmm_init() so that the DU continues with no CMM attached there. > >> ... 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> >> >>>>> 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 >>>>> */ >
Hi Kieran, On Thu, Sep 19, 2019 at 09:08:18AM +0100, Kieran Bingham wrote: > On 19/09/2019 00:23, Laurent Pinchart wrote: > > On Thu, Sep 12, 2019 at 10:19:30AM +0100, Kieran Bingham wrote: > >> On 12/09/2019 09:07, Jacopo Mondi wrote: > >>> On Wed, Sep 11, 2019 at 07:40:27PM +0100, Kieran Bingham wrote: > >>>> 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. > > > > Doesn't this result in probe failure ? > > I think I mis-spoke above, I didn't mean "if rcar_cmm_init() fails" I > meant "if rcar_du_cmm_init() determines there are no connected CMM's or > if they are disabled." > > If rcar_cmm_init() returns a failure, then yes we will fail to probe. > > But I think it's up to rcar_du_cmm_init() to determine if the CMM exists > or not (or is enabled) and if that's not a failure case then it should > not prevent the probing of the DU. > > In fact, I've now seen that if CONFIG_DRM_RCAR_CMM is not enabled, > rcar_cmm_init() returns 0, and I think in fact it should return -ENODEV, > with an exception on that return value in rcar_du_cmm_init() so that the > DU continues with no CMM attached there. I've replied to your other e-mail regarding this, and I agree with you. > >> ... 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> > >> > >>>>> 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 > >>>>> */
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); + } + 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 */