[PATCH v2 0/6] drm: Try to fix encoder possible_clones/crtc

[PATCH v2 0/6] drm: Try to fix encoder possible_clones/crtc

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Remainder of my possible_clones/crtcs cleanup. All the i915 bits and a
few other driver bits got merged already.

Ville Syrjälä (6):
  drm: Include the encoder itself in possible_clones
  drm/gma500: Sanitize possible_clones
  drm/exynos: Use drm_encoder_mask()
  drm/imx: Remove the bogus possible_clones setup
  drm: Validate encoder->possible_clones
  drm: Validate encoder->possible_crtcs

 drivers/gpu/drm/drm_encoder.c           | 63 +++++++++++++++++++++++++
 drivers/gpu/drm/exynos/exynos_drm_drv.c |  5 +-
 drivers/gpu/drm/gma500/framebuffer.c    | 16 +++----
 drivers/gpu/drm/gma500/mdfld_dsi_dpi.c  |  4 +-
 drivers/gpu/drm/imx/imx-drm-core.c      |  2 +-
 5 files changed, 76 insertions(+), 14 deletions(-)

-- 
2.24.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[Intel-gfx] [PATCH v2 0/6] drm: Try to fix encoder possible_clones/crtc

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Remainder of my possible_clones/crtcs cleanup. All the i915 bits and a
few other driver bits got merged already.

Ville Syrjälä (6):
  drm: Include the encoder itself in possible_clones
  drm/gma500: Sanitize possible_clones
  drm/exynos: Use drm_encoder_mask()
  drm/imx: Remove the bogus possible_clones setup
  drm: Validate encoder->possible_clones
  drm: Validate encoder->possible_crtcs

 drivers/gpu/drm/drm_encoder.c           | 63 +++++++++++++++++++++++++
 drivers/gpu/drm/exynos/exynos_drm_drv.c |  5 +-
 drivers/gpu/drm/gma500/framebuffer.c    | 16 +++----
 drivers/gpu/drm/gma500/mdfld_dsi_dpi.c  |  4 +-
 drivers/gpu/drm/imx/imx-drm-core.c      |  2 +-
 5 files changed, 76 insertions(+), 14 deletions(-)

-- 
2.24.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 1/6] drm: Include the encoder itself in possible_clones

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The docs say possible_clones should always include the encoder itself.
Since most drivers don't want to deal with the complexities of cloning
let's allow them to set possible_clones=0 and instead we'll fix that
up in the core.

We can't put this special case into drm_encoder_init() because drivers
will have to fill up possible_clones after adding all the relevant
encoders. Otherwise they wouldn't know the proper encoder indexes to
use. So we'll just do it just before registering the encoders.

TODO: Should we do something similar for possible_crtcs==0?

Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_encoder.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c
index e555281f43d4..f761d9306028 100644
--- a/drivers/gpu/drm/drm_encoder.c
+++ b/drivers/gpu/drm/drm_encoder.c
@@ -66,11 +66,26 @@ static const struct drm_prop_enum_list drm_encoder_enum_list[] = {
 	{ DRM_MODE_ENCODER_DPI, "DPI" },
 };
 
+/*
+ * For some reason we want the encoder itself included in
+ * possible_clones. Make life easy for drivers by allowing them
+ * to leave possible_clones unset if no cloning is possible.
+ */
+static void fixup_possible_clones(struct drm_device *dev)
+{
+	struct drm_encoder *encoder;
+
+	drm_for_each_encoder(encoder, dev)
+		encoder->possible_clones |= drm_encoder_mask(encoder);
+}
+
 int drm_encoder_register_all(struct drm_device *dev)
 {
 	struct drm_encoder *encoder;
 	int ret = 0;
 
+	fixup_possible_clones(dev);
+
 	drm_for_each_encoder(encoder, dev) {
 		if (encoder->funcs->late_register)
 			ret = encoder->funcs->late_register(encoder);
-- 
2.24.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[Intel-gfx] [PATCH v2 1/6] drm: Include the encoder itself in possible_clones

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The docs say possible_clones should always include the encoder itself.
Since most drivers don't want to deal with the complexities of cloning
let's allow them to set possible_clones=0 and instead we'll fix that
up in the core.

We can't put this special case into drm_encoder_init() because drivers
will have to fill up possible_clones after adding all the relevant
encoders. Otherwise they wouldn't know the proper encoder indexes to
use. So we'll just do it just before registering the encoders.

TODO: Should we do something similar for possible_crtcs==0?

Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_encoder.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c
index e555281f43d4..f761d9306028 100644
--- a/drivers/gpu/drm/drm_encoder.c
+++ b/drivers/gpu/drm/drm_encoder.c
@@ -66,11 +66,26 @@ static const struct drm_prop_enum_list drm_encoder_enum_list[] = {
 	{ DRM_MODE_ENCODER_DPI, "DPI" },
 };
 
+/*
+ * For some reason we want the encoder itself included in
+ * possible_clones. Make life easy for drivers by allowing them
+ * to leave possible_clones unset if no cloning is possible.
+ */
+static void fixup_possible_clones(struct drm_device *dev)
+{
+	struct drm_encoder *encoder;
+
+	drm_for_each_encoder(encoder, dev)
+		encoder->possible_clones |= drm_encoder_mask(encoder);
+}
+
 int drm_encoder_register_all(struct drm_device *dev)
 {
 	struct drm_encoder *encoder;
 	int ret = 0;
 
+	fixup_possible_clones(dev);
+
 	drm_for_each_encoder(encoder, dev) {
 		if (encoder->funcs->late_register)
 			ret = encoder->funcs->late_register(encoder);
-- 
2.24.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/6] drm: Include the encoder itself in possible_clones

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 2423 bytes --]

Hi

Am 07.02.20 um 14:59 schrieb Ville Syrjala:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> The docs say possible_clones should always include the encoder itself.
> Since most drivers don't want to deal with the complexities of cloning
> let's allow them to set possible_clones=0 and instead we'll fix that
> up in the core.
> 
> We can't put this special case into drm_encoder_init() because drivers
> will have to fill up possible_clones after adding all the relevant
> encoders. Otherwise they wouldn't know the proper encoder indexes to
> use. So we'll just do it just before registering the encoders.
> 
> TODO: Should we do something similar for possible_crtcs==0?
> 
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

May this fixup function should warn iff possible_clones was set to non-0
by the driver, but the encoder itself is missing. In any case

Acked-by: Thomas Zimmermann <tzimmermann@suse.de>

> ---
>  drivers/gpu/drm/drm_encoder.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c
> index e555281f43d4..f761d9306028 100644
> --- a/drivers/gpu/drm/drm_encoder.c
> +++ b/drivers/gpu/drm/drm_encoder.c
> @@ -66,11 +66,26 @@ static const struct drm_prop_enum_list drm_encoder_enum_list[] = {
>  	{ DRM_MODE_ENCODER_DPI, "DPI" },
>  };
>  
> +/*
> + * For some reason we want the encoder itself included in
> + * possible_clones. Make life easy for drivers by allowing them
> + * to leave possible_clones unset if no cloning is possible.
> + */
> +static void fixup_possible_clones(struct drm_device *dev)
> +{
> +	struct drm_encoder *encoder;
> +
> +	drm_for_each_encoder(encoder, dev)
> +		encoder->possible_clones |= drm_encoder_mask(encoder);
> +}
> +
>  int drm_encoder_register_all(struct drm_device *dev)
>  {
>  	struct drm_encoder *encoder;
>  	int ret = 0;
>  
> +	fixup_possible_clones(dev);
> +
>  	drm_for_each_encoder(encoder, dev) {
>  		if (encoder->funcs->late_register)
>  			ret = encoder->funcs->late_register(encoder);
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH v2 1/6] drm: Include the encoder itself in possible_clones

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 2423 bytes --]

Hi

Am 07.02.20 um 14:59 schrieb Ville Syrjala:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> The docs say possible_clones should always include the encoder itself.
> Since most drivers don't want to deal with the complexities of cloning
> let's allow them to set possible_clones=0 and instead we'll fix that
> up in the core.
> 
> We can't put this special case into drm_encoder_init() because drivers
> will have to fill up possible_clones after adding all the relevant
> encoders. Otherwise they wouldn't know the proper encoder indexes to
> use. So we'll just do it just before registering the encoders.
> 
> TODO: Should we do something similar for possible_crtcs==0?
> 
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

May this fixup function should warn iff possible_clones was set to non-0
by the driver, but the encoder itself is missing. In any case

Acked-by: Thomas Zimmermann <tzimmermann@suse.de>

> ---
>  drivers/gpu/drm/drm_encoder.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c
> index e555281f43d4..f761d9306028 100644
> --- a/drivers/gpu/drm/drm_encoder.c
> +++ b/drivers/gpu/drm/drm_encoder.c
> @@ -66,11 +66,26 @@ static const struct drm_prop_enum_list drm_encoder_enum_list[] = {
>  	{ DRM_MODE_ENCODER_DPI, "DPI" },
>  };
>  
> +/*
> + * For some reason we want the encoder itself included in
> + * possible_clones. Make life easy for drivers by allowing them
> + * to leave possible_clones unset if no cloning is possible.
> + */
> +static void fixup_possible_clones(struct drm_device *dev)
> +{
> +	struct drm_encoder *encoder;
> +
> +	drm_for_each_encoder(encoder, dev)
> +		encoder->possible_clones |= drm_encoder_mask(encoder);
> +}
> +
>  int drm_encoder_register_all(struct drm_device *dev)
>  {
>  	struct drm_encoder *encoder;
>  	int ret = 0;
>  
> +	fixup_possible_clones(dev);
> +
>  	drm_for_each_encoder(encoder, dev) {
>  		if (encoder->funcs->late_register)
>  			ret = encoder->funcs->late_register(encoder);
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/6] drm: Include the encoder itself in possible_clones

[Ville Syrjälä]

On Fri, Feb 07, 2020 at 03:28:35PM +0100, Thomas Zimmermann wrote:
> Hi
> 
> Am 07.02.20 um 14:59 schrieb Ville Syrjala:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > The docs say possible_clones should always include the encoder itself.
> > Since most drivers don't want to deal with the complexities of cloning
> > let's allow them to set possible_clones=0 and instead we'll fix that
> > up in the core.
> > 
> > We can't put this special case into drm_encoder_init() because drivers
> > will have to fill up possible_clones after adding all the relevant
> > encoders. Otherwise they wouldn't know the proper encoder indexes to
> > use. So we'll just do it just before registering the encoders.
> > 
> > TODO: Should we do something similar for possible_crtcs==0?
> > 
> > Cc: Thomas Zimmermann <tzimmermann@suse.de>
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> May this fixup function should warn iff possible_clones was set to non-0
> by the driver, but the encoder itself is missing.

Yeah, I guess we could do that.

> In any case
> 
> Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
> 
> > ---
> >  drivers/gpu/drm/drm_encoder.c | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c
> > index e555281f43d4..f761d9306028 100644
> > --- a/drivers/gpu/drm/drm_encoder.c
> > +++ b/drivers/gpu/drm/drm_encoder.c
> > @@ -66,11 +66,26 @@ static const struct drm_prop_enum_list drm_encoder_enum_list[] = {
> >  	{ DRM_MODE_ENCODER_DPI, "DPI" },
> >  };
> >  
> > +/*
> > + * For some reason we want the encoder itself included in
> > + * possible_clones. Make life easy for drivers by allowing them
> > + * to leave possible_clones unset if no cloning is possible.
> > + */
> > +static void fixup_possible_clones(struct drm_device *dev)
> > +{
> > +	struct drm_encoder *encoder;
> > +
> > +	drm_for_each_encoder(encoder, dev)
> > +		encoder->possible_clones |= drm_encoder_mask(encoder);
> > +}
> > +
> >  int drm_encoder_register_all(struct drm_device *dev)
> >  {
> >  	struct drm_encoder *encoder;
> >  	int ret = 0;
> >  
> > +	fixup_possible_clones(dev);
> > +
> >  	drm_for_each_encoder(encoder, dev) {
> >  		if (encoder->funcs->late_register)
> >  			ret = encoder->funcs->late_register(encoder);
> > 
> 
> -- 
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Felix Imendörffer
> 




-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH v2 1/6] drm: Include the encoder itself in possible_clones

[Ville Syrjälä]

On Fri, Feb 07, 2020 at 03:28:35PM +0100, Thomas Zimmermann wrote:
> Hi
> 
> Am 07.02.20 um 14:59 schrieb Ville Syrjala:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > The docs say possible_clones should always include the encoder itself.
> > Since most drivers don't want to deal with the complexities of cloning
> > let's allow them to set possible_clones=0 and instead we'll fix that
> > up in the core.
> > 
> > We can't put this special case into drm_encoder_init() because drivers
> > will have to fill up possible_clones after adding all the relevant
> > encoders. Otherwise they wouldn't know the proper encoder indexes to
> > use. So we'll just do it just before registering the encoders.
> > 
> > TODO: Should we do something similar for possible_crtcs==0?
> > 
> > Cc: Thomas Zimmermann <tzimmermann@suse.de>
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> May this fixup function should warn iff possible_clones was set to non-0
> by the driver, but the encoder itself is missing.

Yeah, I guess we could do that.

> In any case
> 
> Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
> 
> > ---
> >  drivers/gpu/drm/drm_encoder.c | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c
> > index e555281f43d4..f761d9306028 100644
> > --- a/drivers/gpu/drm/drm_encoder.c
> > +++ b/drivers/gpu/drm/drm_encoder.c
> > @@ -66,11 +66,26 @@ static const struct drm_prop_enum_list drm_encoder_enum_list[] = {
> >  	{ DRM_MODE_ENCODER_DPI, "DPI" },
> >  };
> >  
> > +/*
> > + * For some reason we want the encoder itself included in
> > + * possible_clones. Make life easy for drivers by allowing them
> > + * to leave possible_clones unset if no cloning is possible.
> > + */
> > +static void fixup_possible_clones(struct drm_device *dev)
> > +{
> > +	struct drm_encoder *encoder;
> > +
> > +	drm_for_each_encoder(encoder, dev)
> > +		encoder->possible_clones |= drm_encoder_mask(encoder);
> > +}
> > +
> >  int drm_encoder_register_all(struct drm_device *dev)
> >  {
> >  	struct drm_encoder *encoder;
> >  	int ret = 0;
> >  
> > +	fixup_possible_clones(dev);
> > +
> >  	drm_for_each_encoder(encoder, dev) {
> >  		if (encoder->funcs->late_register)
> >  			ret = encoder->funcs->late_register(encoder);
> > 
> 
> -- 
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Felix Imendörffer
> 




-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 1/6] drm: Include the encoder itself in possible_clones

[Daniel Vetter]

On Fri, Feb 07, 2020 at 04:50:01PM +0200, Ville Syrjälä wrote:
> On Fri, Feb 07, 2020 at 03:28:35PM +0100, Thomas Zimmermann wrote:
> > Hi
> > 
> > Am 07.02.20 um 14:59 schrieb Ville Syrjala:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > The docs say possible_clones should always include the encoder itself.
> > > Since most drivers don't want to deal with the complexities of cloning
> > > let's allow them to set possible_clones=0 and instead we'll fix that
> > > up in the core.
> > > 
> > > We can't put this special case into drm_encoder_init() because drivers
> > > will have to fill up possible_clones after adding all the relevant
> > > encoders. Otherwise they wouldn't know the proper encoder indexes to
> > > use. So we'll just do it just before registering the encoders.
> > > 
> > > TODO: Should we do something similar for possible_crtcs==0?
> > > 
> > > Cc: Thomas Zimmermann <tzimmermann@suse.de>
> > > Cc: Daniel Vetter <daniel@ffwll.ch>
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > May this fixup function should warn iff possible_clones was set to non-0
> > by the driver, but the encoder itself is missing.
> 
> Yeah, I guess we could do that.

+1 on that, should catch some bugs at least.

Also can you pls fix up the kerneldoc for drm_encoder.possible_clones,
defacto this now means that 0 is a totally fine setting.
-Daniel

> 
> > In any case
> > 
> > Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
> > 
> > > ---
> > >  drivers/gpu/drm/drm_encoder.c | 15 +++++++++++++++
> > >  1 file changed, 15 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c
> > > index e555281f43d4..f761d9306028 100644
> > > --- a/drivers/gpu/drm/drm_encoder.c
> > > +++ b/drivers/gpu/drm/drm_encoder.c
> > > @@ -66,11 +66,26 @@ static const struct drm_prop_enum_list drm_encoder_enum_list[] = {
> > >  	{ DRM_MODE_ENCODER_DPI, "DPI" },
> > >  };
> > >  
> > > +/*
> > > + * For some reason we want the encoder itself included in
> > > + * possible_clones. Make life easy for drivers by allowing them
> > > + * to leave possible_clones unset if no cloning is possible.
> > > + */
> > > +static void fixup_possible_clones(struct drm_device *dev)
> > > +{
> > > +	struct drm_encoder *encoder;
> > > +
> > > +	drm_for_each_encoder(encoder, dev)
> > > +		encoder->possible_clones |= drm_encoder_mask(encoder);
> > > +}
> > > +
> > >  int drm_encoder_register_all(struct drm_device *dev)
> > >  {
> > >  	struct drm_encoder *encoder;
> > >  	int ret = 0;
> > >  
> > > +	fixup_possible_clones(dev);
> > > +
> > >  	drm_for_each_encoder(encoder, dev) {
> > >  		if (encoder->funcs->late_register)
> > >  			ret = encoder->funcs->late_register(encoder);
> > > 
> > 
> > -- 
> > Thomas Zimmermann
> > Graphics Driver Developer
> > SUSE Software Solutions Germany GmbH
> > Maxfeldstr. 5, 90409 Nürnberg, Germany
> > (HRB 36809, AG Nürnberg)
> > Geschäftsführer: Felix Imendörffer
> > 
> 
> 
> 
> 
> -- 
> Ville Syrjälä
> Intel
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH v2 1/6] drm: Include the encoder itself in possible_clones

[Daniel Vetter]

On Fri, Feb 07, 2020 at 04:50:01PM +0200, Ville Syrjälä wrote:
> On Fri, Feb 07, 2020 at 03:28:35PM +0100, Thomas Zimmermann wrote:
> > Hi
> > 
> > Am 07.02.20 um 14:59 schrieb Ville Syrjala:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > The docs say possible_clones should always include the encoder itself.
> > > Since most drivers don't want to deal with the complexities of cloning
> > > let's allow them to set possible_clones=0 and instead we'll fix that
> > > up in the core.
> > > 
> > > We can't put this special case into drm_encoder_init() because drivers
> > > will have to fill up possible_clones after adding all the relevant
> > > encoders. Otherwise they wouldn't know the proper encoder indexes to
> > > use. So we'll just do it just before registering the encoders.
> > > 
> > > TODO: Should we do something similar for possible_crtcs==0?
> > > 
> > > Cc: Thomas Zimmermann <tzimmermann@suse.de>
> > > Cc: Daniel Vetter <daniel@ffwll.ch>
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > May this fixup function should warn iff possible_clones was set to non-0
> > by the driver, but the encoder itself is missing.
> 
> Yeah, I guess we could do that.

+1 on that, should catch some bugs at least.

Also can you pls fix up the kerneldoc for drm_encoder.possible_clones,
defacto this now means that 0 is a totally fine setting.
-Daniel

> 
> > In any case
> > 
> > Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
> > 
> > > ---
> > >  drivers/gpu/drm/drm_encoder.c | 15 +++++++++++++++
> > >  1 file changed, 15 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c
> > > index e555281f43d4..f761d9306028 100644
> > > --- a/drivers/gpu/drm/drm_encoder.c
> > > +++ b/drivers/gpu/drm/drm_encoder.c
> > > @@ -66,11 +66,26 @@ static const struct drm_prop_enum_list drm_encoder_enum_list[] = {
> > >  	{ DRM_MODE_ENCODER_DPI, "DPI" },
> > >  };
> > >  
> > > +/*
> > > + * For some reason we want the encoder itself included in
> > > + * possible_clones. Make life easy for drivers by allowing them
> > > + * to leave possible_clones unset if no cloning is possible.
> > > + */
> > > +static void fixup_possible_clones(struct drm_device *dev)
> > > +{
> > > +	struct drm_encoder *encoder;
> > > +
> > > +	drm_for_each_encoder(encoder, dev)
> > > +		encoder->possible_clones |= drm_encoder_mask(encoder);
> > > +}
> > > +
> > >  int drm_encoder_register_all(struct drm_device *dev)
> > >  {
> > >  	struct drm_encoder *encoder;
> > >  	int ret = 0;
> > >  
> > > +	fixup_possible_clones(dev);
> > > +
> > >  	drm_for_each_encoder(encoder, dev) {
> > >  		if (encoder->funcs->late_register)
> > >  			ret = encoder->funcs->late_register(encoder);
> > > 
> > 
> > -- 
> > Thomas Zimmermann
> > Graphics Driver Developer
> > SUSE Software Solutions Germany GmbH
> > Maxfeldstr. 5, 90409 Nürnberg, Germany
> > (HRB 36809, AG Nürnberg)
> > Geschäftsführer: Felix Imendörffer
> > 
> 
> 
> 
> 
> -- 
> Ville Syrjälä
> Intel
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 1/6] drm: Include the encoder itself in possible_clones

[Ville Syrjälä]

On Fri, Feb 07, 2020 at 05:27:51PM +0100, Daniel Vetter wrote:
> On Fri, Feb 07, 2020 at 04:50:01PM +0200, Ville Syrjälä wrote:
> > On Fri, Feb 07, 2020 at 03:28:35PM +0100, Thomas Zimmermann wrote:
> > > Hi
> > > 
> > > Am 07.02.20 um 14:59 schrieb Ville Syrjala:
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > 
> > > > The docs say possible_clones should always include the encoder itself.
> > > > Since most drivers don't want to deal with the complexities of cloning
> > > > let's allow them to set possible_clones=0 and instead we'll fix that
> > > > up in the core.
> > > > 
> > > > We can't put this special case into drm_encoder_init() because drivers
> > > > will have to fill up possible_clones after adding all the relevant
> > > > encoders. Otherwise they wouldn't know the proper encoder indexes to
> > > > use. So we'll just do it just before registering the encoders.
> > > > 
> > > > TODO: Should we do something similar for possible_crtcs==0?
> > > > 
> > > > Cc: Thomas Zimmermann <tzimmermann@suse.de>
> > > > Cc: Daniel Vetter <daniel@ffwll.ch>
> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > May this fixup function should warn iff possible_clones was set to non-0
> > > by the driver, but the encoder itself is missing.
> > 
> > Yeah, I guess we could do that.
> 
> +1 on that, should catch some bugs at least.
> 
> Also can you pls fix up the kerneldoc for drm_encoder.possible_clones,
> defacto this now means that 0 is a totally fine setting.

Sure.

And for possible_crtcs I was thinking similar concept:

for_each_encoder()
	if (possible_crtc == 0)
		possible_crtcs = all_crtc_mask;

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH v2 1/6] drm: Include the encoder itself in possible_clones

[Ville Syrjälä]

On Fri, Feb 07, 2020 at 05:27:51PM +0100, Daniel Vetter wrote:
> On Fri, Feb 07, 2020 at 04:50:01PM +0200, Ville Syrjälä wrote:
> > On Fri, Feb 07, 2020 at 03:28:35PM +0100, Thomas Zimmermann wrote:
> > > Hi
> > > 
> > > Am 07.02.20 um 14:59 schrieb Ville Syrjala:
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > 
> > > > The docs say possible_clones should always include the encoder itself.
> > > > Since most drivers don't want to deal with the complexities of cloning
> > > > let's allow them to set possible_clones=0 and instead we'll fix that
> > > > up in the core.
> > > > 
> > > > We can't put this special case into drm_encoder_init() because drivers
> > > > will have to fill up possible_clones after adding all the relevant
> > > > encoders. Otherwise they wouldn't know the proper encoder indexes to
> > > > use. So we'll just do it just before registering the encoders.
> > > > 
> > > > TODO: Should we do something similar for possible_crtcs==0?
> > > > 
> > > > Cc: Thomas Zimmermann <tzimmermann@suse.de>
> > > > Cc: Daniel Vetter <daniel@ffwll.ch>
> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > May this fixup function should warn iff possible_clones was set to non-0
> > > by the driver, but the encoder itself is missing.
> > 
> > Yeah, I guess we could do that.
> 
> +1 on that, should catch some bugs at least.
> 
> Also can you pls fix up the kerneldoc for drm_encoder.possible_clones,
> defacto this now means that 0 is a totally fine setting.

Sure.

And for possible_crtcs I was thinking similar concept:

for_each_encoder()
	if (possible_crtc == 0)
		possible_crtcs = all_crtc_mask;

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 1/6] drm: Include the encoder itself in possible_clones

[Daniel Vetter]

On Fri, Feb 07, 2020 at 06:34:47PM +0200, Ville Syrjälä wrote:
> On Fri, Feb 07, 2020 at 05:27:51PM +0100, Daniel Vetter wrote:
> > On Fri, Feb 07, 2020 at 04:50:01PM +0200, Ville Syrjälä wrote:
> > > On Fri, Feb 07, 2020 at 03:28:35PM +0100, Thomas Zimmermann wrote:
> > > > Hi
> > > > 
> > > > Am 07.02.20 um 14:59 schrieb Ville Syrjala:
> > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > 
> > > > > The docs say possible_clones should always include the encoder itself.
> > > > > Since most drivers don't want to deal with the complexities of cloning
> > > > > let's allow them to set possible_clones=0 and instead we'll fix that
> > > > > up in the core.
> > > > > 
> > > > > We can't put this special case into drm_encoder_init() because drivers
> > > > > will have to fill up possible_clones after adding all the relevant
> > > > > encoders. Otherwise they wouldn't know the proper encoder indexes to
> > > > > use. So we'll just do it just before registering the encoders.
> > > > > 
> > > > > TODO: Should we do something similar for possible_crtcs==0?
> > > > > 
> > > > > Cc: Thomas Zimmermann <tzimmermann@suse.de>
> > > > > Cc: Daniel Vetter <daniel@ffwll.ch>
> > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > 
> > > > May this fixup function should warn iff possible_clones was set to non-0
> > > > by the driver, but the encoder itself is missing.
> > > 
> > > Yeah, I guess we could do that.
> > 
> > +1 on that, should catch some bugs at least.
> > 
> > Also can you pls fix up the kerneldoc for drm_encoder.possible_clones,
> > defacto this now means that 0 is a totally fine setting.
> 
> Sure.
> 
> And for possible_crtcs I was thinking similar concept:
> 
> for_each_encoder()
> 	if (possible_crtc == 0)
> 		possible_crtcs = all_crtc_mask;

A quick grep shows that I think we can risk enforcing this. If that turns
out to be a misconception we can always go back to the fixup approach if
possible_crtcs is 0. But unlike possible_clones I think for possible_crtcs
the fixup-less approach looks possible at least.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH v2 1/6] drm: Include the encoder itself in possible_clones

[Daniel Vetter]

On Fri, Feb 07, 2020 at 06:34:47PM +0200, Ville Syrjälä wrote:
> On Fri, Feb 07, 2020 at 05:27:51PM +0100, Daniel Vetter wrote:
> > On Fri, Feb 07, 2020 at 04:50:01PM +0200, Ville Syrjälä wrote:
> > > On Fri, Feb 07, 2020 at 03:28:35PM +0100, Thomas Zimmermann wrote:
> > > > Hi
> > > > 
> > > > Am 07.02.20 um 14:59 schrieb Ville Syrjala:
> > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > 
> > > > > The docs say possible_clones should always include the encoder itself.
> > > > > Since most drivers don't want to deal with the complexities of cloning
> > > > > let's allow them to set possible_clones=0 and instead we'll fix that
> > > > > up in the core.
> > > > > 
> > > > > We can't put this special case into drm_encoder_init() because drivers
> > > > > will have to fill up possible_clones after adding all the relevant
> > > > > encoders. Otherwise they wouldn't know the proper encoder indexes to
> > > > > use. So we'll just do it just before registering the encoders.
> > > > > 
> > > > > TODO: Should we do something similar for possible_crtcs==0?
> > > > > 
> > > > > Cc: Thomas Zimmermann <tzimmermann@suse.de>
> > > > > Cc: Daniel Vetter <daniel@ffwll.ch>
> > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > 
> > > > May this fixup function should warn iff possible_clones was set to non-0
> > > > by the driver, but the encoder itself is missing.
> > > 
> > > Yeah, I guess we could do that.
> > 
> > +1 on that, should catch some bugs at least.
> > 
> > Also can you pls fix up the kerneldoc for drm_encoder.possible_clones,
> > defacto this now means that 0 is a totally fine setting.
> 
> Sure.
> 
> And for possible_crtcs I was thinking similar concept:
> 
> for_each_encoder()
> 	if (possible_crtc == 0)
> 		possible_crtcs = all_crtc_mask;

A quick grep shows that I think we can risk enforcing this. If that turns
out to be a misconception we can always go back to the fixup approach if
possible_crtcs is 0. But unlike possible_clones I think for possible_crtcs
the fixup-less approach looks possible at least.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 1/6] drm: Include the encoder itself in possible_clones

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 3760 bytes --]

Hi

Am 07.02.20 um 17:27 schrieb Daniel Vetter:
> On Fri, Feb 07, 2020 at 04:50:01PM +0200, Ville Syrjälä wrote:
>> On Fri, Feb 07, 2020 at 03:28:35PM +0100, Thomas Zimmermann wrote:
>>> Hi
>>>
>>> Am 07.02.20 um 14:59 schrieb Ville Syrjala:
>>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>>
>>>> The docs say possible_clones should always include the encoder itself.
>>>> Since most drivers don't want to deal with the complexities of cloning
>>>> let's allow them to set possible_clones=0 and instead we'll fix that
>>>> up in the core.
>>>>
>>>> We can't put this special case into drm_encoder_init() because drivers
>>>> will have to fill up possible_clones after adding all the relevant
>>>> encoders. Otherwise they wouldn't know the proper encoder indexes to
>>>> use. So we'll just do it just before registering the encoders.
>>>>
>>>> TODO: Should we do something similar for possible_crtcs==0?
>>>>
>>>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
>>>> Cc: Daniel Vetter <daniel@ffwll.ch>
>>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>
>>> May this fixup function should warn iff possible_clones was set to non-0
>>> by the driver, but the encoder itself is missing.
>>
>> Yeah, I guess we could do that.
> 
> +1 on that, should catch some bugs at least.
> 
> Also can you pls fix up the kerneldoc for drm_encoder.possible_clones,
> defacto this now means that 0 is a totally fine setting.

Just a thought: we should encourage, or maybe even require, the use of
such defaults (i.e., 0); unless a driver has a good reason to override
them. So we don't have to fix drivers if the defaults ever change.

Best regards
Thomas

> -Daniel
> 
>>
>>> In any case
>>>
>>> Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>
>>>> ---
>>>>  drivers/gpu/drm/drm_encoder.c | 15 +++++++++++++++
>>>>  1 file changed, 15 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c
>>>> index e555281f43d4..f761d9306028 100644
>>>> --- a/drivers/gpu/drm/drm_encoder.c
>>>> +++ b/drivers/gpu/drm/drm_encoder.c
>>>> @@ -66,11 +66,26 @@ static const struct drm_prop_enum_list drm_encoder_enum_list[] = {
>>>>  	{ DRM_MODE_ENCODER_DPI, "DPI" },
>>>>  };
>>>>  
>>>> +/*
>>>> + * For some reason we want the encoder itself included in
>>>> + * possible_clones. Make life easy for drivers by allowing them
>>>> + * to leave possible_clones unset if no cloning is possible.
>>>> + */
>>>> +static void fixup_possible_clones(struct drm_device *dev)
>>>> +{
>>>> +	struct drm_encoder *encoder;
>>>> +
>>>> +	drm_for_each_encoder(encoder, dev)
>>>> +		encoder->possible_clones |= drm_encoder_mask(encoder);
>>>> +}
>>>> +
>>>>  int drm_encoder_register_all(struct drm_device *dev)
>>>>  {
>>>>  	struct drm_encoder *encoder;
>>>>  	int ret = 0;
>>>>  
>>>> +	fixup_possible_clones(dev);
>>>> +
>>>>  	drm_for_each_encoder(encoder, dev) {
>>>>  		if (encoder->funcs->late_register)
>>>>  			ret = encoder->funcs->late_register(encoder);
>>>>
>>>
>>> -- 
>>> Thomas Zimmermann
>>> Graphics Driver Developer
>>> SUSE Software Solutions Germany GmbH
>>> Maxfeldstr. 5, 90409 Nürnberg, Germany
>>> (HRB 36809, AG Nürnberg)
>>> Geschäftsführer: Felix Imendörffer
>>>
>>
>>
>>
>>
>> -- 
>> Ville Syrjälä
>> Intel
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH v2 1/6] drm: Include the encoder itself in possible_clones

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 3760 bytes --]

Hi

Am 07.02.20 um 17:27 schrieb Daniel Vetter:
> On Fri, Feb 07, 2020 at 04:50:01PM +0200, Ville Syrjälä wrote:
>> On Fri, Feb 07, 2020 at 03:28:35PM +0100, Thomas Zimmermann wrote:
>>> Hi
>>>
>>> Am 07.02.20 um 14:59 schrieb Ville Syrjala:
>>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>>
>>>> The docs say possible_clones should always include the encoder itself.
>>>> Since most drivers don't want to deal with the complexities of cloning
>>>> let's allow them to set possible_clones=0 and instead we'll fix that
>>>> up in the core.
>>>>
>>>> We can't put this special case into drm_encoder_init() because drivers
>>>> will have to fill up possible_clones after adding all the relevant
>>>> encoders. Otherwise they wouldn't know the proper encoder indexes to
>>>> use. So we'll just do it just before registering the encoders.
>>>>
>>>> TODO: Should we do something similar for possible_crtcs==0?
>>>>
>>>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
>>>> Cc: Daniel Vetter <daniel@ffwll.ch>
>>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>
>>> May this fixup function should warn iff possible_clones was set to non-0
>>> by the driver, but the encoder itself is missing.
>>
>> Yeah, I guess we could do that.
> 
> +1 on that, should catch some bugs at least.
> 
> Also can you pls fix up the kerneldoc for drm_encoder.possible_clones,
> defacto this now means that 0 is a totally fine setting.

Just a thought: we should encourage, or maybe even require, the use of
such defaults (i.e., 0); unless a driver has a good reason to override
them. So we don't have to fix drivers if the defaults ever change.

Best regards
Thomas

> -Daniel
> 
>>
>>> In any case
>>>
>>> Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>
>>>> ---
>>>>  drivers/gpu/drm/drm_encoder.c | 15 +++++++++++++++
>>>>  1 file changed, 15 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c
>>>> index e555281f43d4..f761d9306028 100644
>>>> --- a/drivers/gpu/drm/drm_encoder.c
>>>> +++ b/drivers/gpu/drm/drm_encoder.c
>>>> @@ -66,11 +66,26 @@ static const struct drm_prop_enum_list drm_encoder_enum_list[] = {
>>>>  	{ DRM_MODE_ENCODER_DPI, "DPI" },
>>>>  };
>>>>  
>>>> +/*
>>>> + * For some reason we want the encoder itself included in
>>>> + * possible_clones. Make life easy for drivers by allowing them
>>>> + * to leave possible_clones unset if no cloning is possible.
>>>> + */
>>>> +static void fixup_possible_clones(struct drm_device *dev)
>>>> +{
>>>> +	struct drm_encoder *encoder;
>>>> +
>>>> +	drm_for_each_encoder(encoder, dev)
>>>> +		encoder->possible_clones |= drm_encoder_mask(encoder);
>>>> +}
>>>> +
>>>>  int drm_encoder_register_all(struct drm_device *dev)
>>>>  {
>>>>  	struct drm_encoder *encoder;
>>>>  	int ret = 0;
>>>>  
>>>> +	fixup_possible_clones(dev);
>>>> +
>>>>  	drm_for_each_encoder(encoder, dev) {
>>>>  		if (encoder->funcs->late_register)
>>>>  			ret = encoder->funcs->late_register(encoder);
>>>>
>>>
>>> -- 
>>> Thomas Zimmermann
>>> Graphics Driver Developer
>>> SUSE Software Solutions Germany GmbH
>>> Maxfeldstr. 5, 90409 Nürnberg, Germany
>>> (HRB 36809, AG Nürnberg)
>>> Geschäftsführer: Felix Imendörffer
>>>
>>
>>
>>
>>
>> -- 
>> Ville Syrjälä
>> Intel
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/6] drm: Include the encoder itself in possible_clones

[Daniel Vetter]

On Fri, Feb 07, 2020 at 03:59:45PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> The docs say possible_clones should always include the encoder itself.
> Since most drivers don't want to deal with the complexities of cloning
> let's allow them to set possible_clones=0 and instead we'll fix that
> up in the core.
> 
> We can't put this special case into drm_encoder_init() because drivers
> will have to fill up possible_clones after adding all the relevant
> encoders. Otherwise they wouldn't know the proper encoder indexes to
> use. So we'll just do it just before registering the encoders.
> 
> TODO: Should we do something similar for possible_crtcs==0?
> 
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_encoder.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c
> index e555281f43d4..f761d9306028 100644
> --- a/drivers/gpu/drm/drm_encoder.c
> +++ b/drivers/gpu/drm/drm_encoder.c
> @@ -66,11 +66,26 @@ static const struct drm_prop_enum_list drm_encoder_enum_list[] = {
>  	{ DRM_MODE_ENCODER_DPI, "DPI" },
>  };
>  
> +/*
> + * For some reason we want the encoder itself included in
> + * possible_clones. Make life easy for drivers by allowing them
> + * to leave possible_clones unset if no cloning is possible.
> + */
> +static void fixup_possible_clones(struct drm_device *dev)
> +{
> +	struct drm_encoder *encoder;
> +
> +	drm_for_each_encoder(encoder, dev)
> +		encoder->possible_clones |= drm_encoder_mask(encoder);
> +}
> +
>  int drm_encoder_register_all(struct drm_device *dev)
>  {
>  	struct drm_encoder *encoder;
>  	int ret = 0;
>  
> +	fixup_possible_clones(dev);

This is way too late, we've already registered the chardev minors at this
point. I think we need a new drm_mode_config_validate() at the top of
drm_dev_register, but which does _not_ run when the driver has a ->load
callback (which soon will be no driver at all).

Cheers, Daniel


> +
>  	drm_for_each_encoder(encoder, dev) {
>  		if (encoder->funcs->late_register)
>  			ret = encoder->funcs->late_register(encoder);
> -- 
> 2.24.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH v2 1/6] drm: Include the encoder itself in possible_clones

[Daniel Vetter]

On Fri, Feb 07, 2020 at 03:59:45PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> The docs say possible_clones should always include the encoder itself.
> Since most drivers don't want to deal with the complexities of cloning
> let's allow them to set possible_clones=0 and instead we'll fix that
> up in the core.
> 
> We can't put this special case into drm_encoder_init() because drivers
> will have to fill up possible_clones after adding all the relevant
> encoders. Otherwise they wouldn't know the proper encoder indexes to
> use. So we'll just do it just before registering the encoders.
> 
> TODO: Should we do something similar for possible_crtcs==0?
> 
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_encoder.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c
> index e555281f43d4..f761d9306028 100644
> --- a/drivers/gpu/drm/drm_encoder.c
> +++ b/drivers/gpu/drm/drm_encoder.c
> @@ -66,11 +66,26 @@ static const struct drm_prop_enum_list drm_encoder_enum_list[] = {
>  	{ DRM_MODE_ENCODER_DPI, "DPI" },
>  };
>  
> +/*
> + * For some reason we want the encoder itself included in
> + * possible_clones. Make life easy for drivers by allowing them
> + * to leave possible_clones unset if no cloning is possible.
> + */
> +static void fixup_possible_clones(struct drm_device *dev)
> +{
> +	struct drm_encoder *encoder;
> +
> +	drm_for_each_encoder(encoder, dev)
> +		encoder->possible_clones |= drm_encoder_mask(encoder);
> +}
> +
>  int drm_encoder_register_all(struct drm_device *dev)
>  {
>  	struct drm_encoder *encoder;
>  	int ret = 0;
>  
> +	fixup_possible_clones(dev);

This is way too late, we've already registered the chardev minors at this
point. I think we need a new drm_mode_config_validate() at the top of
drm_dev_register, but which does _not_ run when the driver has a ->load
callback (which soon will be no driver at all).

Cheers, Daniel


> +
>  	drm_for_each_encoder(encoder, dev) {
>  		if (encoder->funcs->late_register)
>  			ret = encoder->funcs->late_register(encoder);
> -- 
> 2.24.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 2/6] drm/gma500: Sanitize possible_clones

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

I doubt the DP+DP and SDVO+SDVO cloning works for this driver.
i915 at least doesn't do those. Truthfully there could be some very
specific circumstances where some of them would do doable, but
genereally it's too much pain to deal with so we've chose not to
bother. Let's use the same approach for gma500.

Also the LVDS+LVDS and DSI+DSI cases probably don't really exist as
there is one of each at most.

This does mean we'll now leave possible_clones at 0 for these encoder
types whereas previosuly we included the encoder itself in the bitmask.
But that's fine as the core now treaks 0 as a special case and adds
the encoder itself into the final bitmask reported to userspace.

Cc: Patrik Jakobsson <patrik.r.jakobsson@gmail.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/gma500/framebuffer.c   | 16 ++++++++--------
 drivers/gpu/drm/gma500/mdfld_dsi_dpi.c |  4 ++--
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/gma500/framebuffer.c b/drivers/gpu/drm/gma500/framebuffer.c
index 1459076d1980..6ca4e6ded96c 100644
--- a/drivers/gpu/drm/gma500/framebuffer.c
+++ b/drivers/gpu/drm/gma500/framebuffer.c
@@ -581,31 +581,31 @@ static void psb_setup_outputs(struct drm_device *dev)
 			break;
 		case INTEL_OUTPUT_SDVO:
 			crtc_mask = dev_priv->ops->sdvo_mask;
-			clone_mask = (1 << INTEL_OUTPUT_SDVO);
+			clone_mask = 0;
 			break;
 		case INTEL_OUTPUT_LVDS:
-		        crtc_mask = dev_priv->ops->lvds_mask;
-			clone_mask = (1 << INTEL_OUTPUT_LVDS);
+			crtc_mask = dev_priv->ops->lvds_mask;
+			clone_mask = 0;
 			break;
 		case INTEL_OUTPUT_MIPI:
 			crtc_mask = (1 << 0);
-			clone_mask = (1 << INTEL_OUTPUT_MIPI);
+			clone_mask = 0;
 			break;
 		case INTEL_OUTPUT_MIPI2:
 			crtc_mask = (1 << 2);
-			clone_mask = (1 << INTEL_OUTPUT_MIPI2);
+			clone_mask = 0;
 			break;
 		case INTEL_OUTPUT_HDMI:
-		        crtc_mask = dev_priv->ops->hdmi_mask;
+			crtc_mask = dev_priv->ops->hdmi_mask;
 			clone_mask = (1 << INTEL_OUTPUT_HDMI);
 			break;
 		case INTEL_OUTPUT_DISPLAYPORT:
 			crtc_mask = (1 << 0) | (1 << 1);
-			clone_mask = (1 << INTEL_OUTPUT_DISPLAYPORT);
+			clone_mask = 0;
 			break;
 		case INTEL_OUTPUT_EDP:
 			crtc_mask = (1 << 1);
-			clone_mask = (1 << INTEL_OUTPUT_EDP);
+			clone_mask = 0;
 		}
 		encoder->possible_crtcs = crtc_mask;
 		encoder->possible_clones =
diff --git a/drivers/gpu/drm/gma500/mdfld_dsi_dpi.c b/drivers/gpu/drm/gma500/mdfld_dsi_dpi.c
index d4c65f268922..187817e0c004 100644
--- a/drivers/gpu/drm/gma500/mdfld_dsi_dpi.c
+++ b/drivers/gpu/drm/gma500/mdfld_dsi_dpi.c
@@ -1006,10 +1006,10 @@ struct mdfld_dsi_encoder *mdfld_dsi_dpi_init(struct drm_device *dev,
 	/*set possible crtcs and clones*/
 	if (dsi_connector->pipe) {
 		encoder->possible_crtcs = (1 << 2);
-		encoder->possible_clones = (1 << 1);
+		encoder->possible_clones = 0;
 	} else {
 		encoder->possible_crtcs = (1 << 0);
-		encoder->possible_clones = (1 << 0);
+		encoder->possible_clones = 0;
 	}
 
 	dsi_connector->base.encoder = &dpi_output->base.base;
-- 
2.24.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[Intel-gfx] [PATCH v2 2/6] drm/gma500: Sanitize possible_clones

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

I doubt the DP+DP and SDVO+SDVO cloning works for this driver.
i915 at least doesn't do those. Truthfully there could be some very
specific circumstances where some of them would do doable, but
genereally it's too much pain to deal with so we've chose not to
bother. Let's use the same approach for gma500.

Also the LVDS+LVDS and DSI+DSI cases probably don't really exist as
there is one of each at most.

This does mean we'll now leave possible_clones at 0 for these encoder
types whereas previosuly we included the encoder itself in the bitmask.
But that's fine as the core now treaks 0 as a special case and adds
the encoder itself into the final bitmask reported to userspace.

Cc: Patrik Jakobsson <patrik.r.jakobsson@gmail.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/gma500/framebuffer.c   | 16 ++++++++--------
 drivers/gpu/drm/gma500/mdfld_dsi_dpi.c |  4 ++--
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/gma500/framebuffer.c b/drivers/gpu/drm/gma500/framebuffer.c
index 1459076d1980..6ca4e6ded96c 100644
--- a/drivers/gpu/drm/gma500/framebuffer.c
+++ b/drivers/gpu/drm/gma500/framebuffer.c
@@ -581,31 +581,31 @@ static void psb_setup_outputs(struct drm_device *dev)
 			break;
 		case INTEL_OUTPUT_SDVO:
 			crtc_mask = dev_priv->ops->sdvo_mask;
-			clone_mask = (1 << INTEL_OUTPUT_SDVO);
+			clone_mask = 0;
 			break;
 		case INTEL_OUTPUT_LVDS:
-		        crtc_mask = dev_priv->ops->lvds_mask;
-			clone_mask = (1 << INTEL_OUTPUT_LVDS);
+			crtc_mask = dev_priv->ops->lvds_mask;
+			clone_mask = 0;
 			break;
 		case INTEL_OUTPUT_MIPI:
 			crtc_mask = (1 << 0);
-			clone_mask = (1 << INTEL_OUTPUT_MIPI);
+			clone_mask = 0;
 			break;
 		case INTEL_OUTPUT_MIPI2:
 			crtc_mask = (1 << 2);
-			clone_mask = (1 << INTEL_OUTPUT_MIPI2);
+			clone_mask = 0;
 			break;
 		case INTEL_OUTPUT_HDMI:
-		        crtc_mask = dev_priv->ops->hdmi_mask;
+			crtc_mask = dev_priv->ops->hdmi_mask;
 			clone_mask = (1 << INTEL_OUTPUT_HDMI);
 			break;
 		case INTEL_OUTPUT_DISPLAYPORT:
 			crtc_mask = (1 << 0) | (1 << 1);
-			clone_mask = (1 << INTEL_OUTPUT_DISPLAYPORT);
+			clone_mask = 0;
 			break;
 		case INTEL_OUTPUT_EDP:
 			crtc_mask = (1 << 1);
-			clone_mask = (1 << INTEL_OUTPUT_EDP);
+			clone_mask = 0;
 		}
 		encoder->possible_crtcs = crtc_mask;
 		encoder->possible_clones =
diff --git a/drivers/gpu/drm/gma500/mdfld_dsi_dpi.c b/drivers/gpu/drm/gma500/mdfld_dsi_dpi.c
index d4c65f268922..187817e0c004 100644
--- a/drivers/gpu/drm/gma500/mdfld_dsi_dpi.c
+++ b/drivers/gpu/drm/gma500/mdfld_dsi_dpi.c
@@ -1006,10 +1006,10 @@ struct mdfld_dsi_encoder *mdfld_dsi_dpi_init(struct drm_device *dev,
 	/*set possible crtcs and clones*/
 	if (dsi_connector->pipe) {
 		encoder->possible_crtcs = (1 << 2);
-		encoder->possible_clones = (1 << 1);
+		encoder->possible_clones = 0;
 	} else {
 		encoder->possible_crtcs = (1 << 0);
-		encoder->possible_clones = (1 << 0);
+		encoder->possible_clones = 0;
 	}
 
 	dsi_connector->base.encoder = &dpi_output->base.base;
-- 
2.24.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 2/6] drm/gma500: Sanitize possible_clones

[Daniel Vetter]

On Fri, Feb 07, 2020 at 03:59:46PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> I doubt the DP+DP and SDVO+SDVO cloning works for this driver.
> i915 at least doesn't do those. Truthfully there could be some very
> specific circumstances where some of them would do doable, but
> genereally it's too much pain to deal with so we've chose not to
> bother. Let's use the same approach for gma500.
> 
> Also the LVDS+LVDS and DSI+DSI cases probably don't really exist as
> there is one of each at most.
> 
> This does mean we'll now leave possible_clones at 0 for these encoder
> types whereas previosuly we included the encoder itself in the bitmask.
> But that's fine as the core now treaks 0 as a special case and adds
> the encoder itself into the final bitmask reported to userspace.
> 
> Cc: Patrik Jakobsson <patrik.r.jakobsson@gmail.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Looks reasonable.

Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/gma500/framebuffer.c   | 16 ++++++++--------
>  drivers/gpu/drm/gma500/mdfld_dsi_dpi.c |  4 ++--
>  2 files changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/gma500/framebuffer.c b/drivers/gpu/drm/gma500/framebuffer.c
> index 1459076d1980..6ca4e6ded96c 100644
> --- a/drivers/gpu/drm/gma500/framebuffer.c
> +++ b/drivers/gpu/drm/gma500/framebuffer.c
> @@ -581,31 +581,31 @@ static void psb_setup_outputs(struct drm_device *dev)
>  			break;
>  		case INTEL_OUTPUT_SDVO:
>  			crtc_mask = dev_priv->ops->sdvo_mask;
> -			clone_mask = (1 << INTEL_OUTPUT_SDVO);
> +			clone_mask = 0;
>  			break;
>  		case INTEL_OUTPUT_LVDS:
> -		        crtc_mask = dev_priv->ops->lvds_mask;
> -			clone_mask = (1 << INTEL_OUTPUT_LVDS);
> +			crtc_mask = dev_priv->ops->lvds_mask;
> +			clone_mask = 0;
>  			break;
>  		case INTEL_OUTPUT_MIPI:
>  			crtc_mask = (1 << 0);
> -			clone_mask = (1 << INTEL_OUTPUT_MIPI);
> +			clone_mask = 0;
>  			break;
>  		case INTEL_OUTPUT_MIPI2:
>  			crtc_mask = (1 << 2);
> -			clone_mask = (1 << INTEL_OUTPUT_MIPI2);
> +			clone_mask = 0;
>  			break;
>  		case INTEL_OUTPUT_HDMI:
> -		        crtc_mask = dev_priv->ops->hdmi_mask;
> +			crtc_mask = dev_priv->ops->hdmi_mask;
>  			clone_mask = (1 << INTEL_OUTPUT_HDMI);
>  			break;
>  		case INTEL_OUTPUT_DISPLAYPORT:
>  			crtc_mask = (1 << 0) | (1 << 1);
> -			clone_mask = (1 << INTEL_OUTPUT_DISPLAYPORT);
> +			clone_mask = 0;
>  			break;
>  		case INTEL_OUTPUT_EDP:
>  			crtc_mask = (1 << 1);
> -			clone_mask = (1 << INTEL_OUTPUT_EDP);
> +			clone_mask = 0;
>  		}
>  		encoder->possible_crtcs = crtc_mask;
>  		encoder->possible_clones =
> diff --git a/drivers/gpu/drm/gma500/mdfld_dsi_dpi.c b/drivers/gpu/drm/gma500/mdfld_dsi_dpi.c
> index d4c65f268922..187817e0c004 100644
> --- a/drivers/gpu/drm/gma500/mdfld_dsi_dpi.c
> +++ b/drivers/gpu/drm/gma500/mdfld_dsi_dpi.c
> @@ -1006,10 +1006,10 @@ struct mdfld_dsi_encoder *mdfld_dsi_dpi_init(struct drm_device *dev,
>  	/*set possible crtcs and clones*/
>  	if (dsi_connector->pipe) {
>  		encoder->possible_crtcs = (1 << 2);
> -		encoder->possible_clones = (1 << 1);
> +		encoder->possible_clones = 0;
>  	} else {
>  		encoder->possible_crtcs = (1 << 0);
> -		encoder->possible_clones = (1 << 0);
> +		encoder->possible_clones = 0;
>  	}
>  
>  	dsi_connector->base.encoder = &dpi_output->base.base;
> -- 
> 2.24.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH v2 2/6] drm/gma500: Sanitize possible_clones

[Daniel Vetter]

On Fri, Feb 07, 2020 at 03:59:46PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> I doubt the DP+DP and SDVO+SDVO cloning works for this driver.
> i915 at least doesn't do those. Truthfully there could be some very
> specific circumstances where some of them would do doable, but
> genereally it's too much pain to deal with so we've chose not to
> bother. Let's use the same approach for gma500.
> 
> Also the LVDS+LVDS and DSI+DSI cases probably don't really exist as
> there is one of each at most.
> 
> This does mean we'll now leave possible_clones at 0 for these encoder
> types whereas previosuly we included the encoder itself in the bitmask.
> But that's fine as the core now treaks 0 as a special case and adds
> the encoder itself into the final bitmask reported to userspace.
> 
> Cc: Patrik Jakobsson <patrik.r.jakobsson@gmail.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Looks reasonable.

Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/gma500/framebuffer.c   | 16 ++++++++--------
>  drivers/gpu/drm/gma500/mdfld_dsi_dpi.c |  4 ++--
>  2 files changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/gma500/framebuffer.c b/drivers/gpu/drm/gma500/framebuffer.c
> index 1459076d1980..6ca4e6ded96c 100644
> --- a/drivers/gpu/drm/gma500/framebuffer.c
> +++ b/drivers/gpu/drm/gma500/framebuffer.c
> @@ -581,31 +581,31 @@ static void psb_setup_outputs(struct drm_device *dev)
>  			break;
>  		case INTEL_OUTPUT_SDVO:
>  			crtc_mask = dev_priv->ops->sdvo_mask;
> -			clone_mask = (1 << INTEL_OUTPUT_SDVO);
> +			clone_mask = 0;
>  			break;
>  		case INTEL_OUTPUT_LVDS:
> -		        crtc_mask = dev_priv->ops->lvds_mask;
> -			clone_mask = (1 << INTEL_OUTPUT_LVDS);
> +			crtc_mask = dev_priv->ops->lvds_mask;
> +			clone_mask = 0;
>  			break;
>  		case INTEL_OUTPUT_MIPI:
>  			crtc_mask = (1 << 0);
> -			clone_mask = (1 << INTEL_OUTPUT_MIPI);
> +			clone_mask = 0;
>  			break;
>  		case INTEL_OUTPUT_MIPI2:
>  			crtc_mask = (1 << 2);
> -			clone_mask = (1 << INTEL_OUTPUT_MIPI2);
> +			clone_mask = 0;
>  			break;
>  		case INTEL_OUTPUT_HDMI:
> -		        crtc_mask = dev_priv->ops->hdmi_mask;
> +			crtc_mask = dev_priv->ops->hdmi_mask;
>  			clone_mask = (1 << INTEL_OUTPUT_HDMI);
>  			break;
>  		case INTEL_OUTPUT_DISPLAYPORT:
>  			crtc_mask = (1 << 0) | (1 << 1);
> -			clone_mask = (1 << INTEL_OUTPUT_DISPLAYPORT);
> +			clone_mask = 0;
>  			break;
>  		case INTEL_OUTPUT_EDP:
>  			crtc_mask = (1 << 1);
> -			clone_mask = (1 << INTEL_OUTPUT_EDP);
> +			clone_mask = 0;
>  		}
>  		encoder->possible_crtcs = crtc_mask;
>  		encoder->possible_clones =
> diff --git a/drivers/gpu/drm/gma500/mdfld_dsi_dpi.c b/drivers/gpu/drm/gma500/mdfld_dsi_dpi.c
> index d4c65f268922..187817e0c004 100644
> --- a/drivers/gpu/drm/gma500/mdfld_dsi_dpi.c
> +++ b/drivers/gpu/drm/gma500/mdfld_dsi_dpi.c
> @@ -1006,10 +1006,10 @@ struct mdfld_dsi_encoder *mdfld_dsi_dpi_init(struct drm_device *dev,
>  	/*set possible crtcs and clones*/
>  	if (dsi_connector->pipe) {
>  		encoder->possible_crtcs = (1 << 2);
> -		encoder->possible_clones = (1 << 1);
> +		encoder->possible_clones = 0;
>  	} else {
>  		encoder->possible_crtcs = (1 << 0);
> -		encoder->possible_clones = (1 << 0);
> +		encoder->possible_clones = 0;
>  	}
>  
>  	dsi_connector->base.encoder = &dpi_output->base.base;
> -- 
> 2.24.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 3/6] drm/exynos: Use drm_encoder_mask()

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Replace the hand rolled encoder bitmask thing with drm_encoder_mask()

Cc: Inki Dae <inki.dae@samsung.com>
Cc: Joonyoung Shim <jy0922.shim@samsung.com>
Cc: Seung-Woo Kim <sw0312.kim@samsung.com>
Cc: Kyungmin Park <kyungmin.park@samsung.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/exynos/exynos_drm_drv.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
index ba0f868b2477..57defeb44522 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
@@ -270,7 +270,7 @@ static int exynos_drm_bind(struct device *dev)
 	struct drm_encoder *encoder;
 	struct drm_device *drm;
 	unsigned int clone_mask;
-	int cnt, ret;
+	int ret;
 
 	drm = drm_dev_alloc(&exynos_drm_driver, dev);
 	if (IS_ERR(drm))
@@ -293,10 +293,9 @@ static int exynos_drm_bind(struct device *dev)
 	exynos_drm_mode_config_init(drm);
 
 	/* setup possible_clones. */
-	cnt = 0;
 	clone_mask = 0;
 	list_for_each_entry(encoder, &drm->mode_config.encoder_list, head)
-		clone_mask |= (1 << (cnt++));
+		clone_mask |= drm_encoder_mask(encoder);
 
 	list_for_each_entry(encoder, &drm->mode_config.encoder_list, head)
 		encoder->possible_clones = clone_mask;
-- 
2.24.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[Intel-gfx] [PATCH v2 3/6] drm/exynos: Use drm_encoder_mask()

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Replace the hand rolled encoder bitmask thing with drm_encoder_mask()

Cc: Inki Dae <inki.dae@samsung.com>
Cc: Joonyoung Shim <jy0922.shim@samsung.com>
Cc: Seung-Woo Kim <sw0312.kim@samsung.com>
Cc: Kyungmin Park <kyungmin.park@samsung.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/exynos/exynos_drm_drv.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
index ba0f868b2477..57defeb44522 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
@@ -270,7 +270,7 @@ static int exynos_drm_bind(struct device *dev)
 	struct drm_encoder *encoder;
 	struct drm_device *drm;
 	unsigned int clone_mask;
-	int cnt, ret;
+	int ret;
 
 	drm = drm_dev_alloc(&exynos_drm_driver, dev);
 	if (IS_ERR(drm))
@@ -293,10 +293,9 @@ static int exynos_drm_bind(struct device *dev)
 	exynos_drm_mode_config_init(drm);
 
 	/* setup possible_clones. */
-	cnt = 0;
 	clone_mask = 0;
 	list_for_each_entry(encoder, &drm->mode_config.encoder_list, head)
-		clone_mask |= (1 << (cnt++));
+		clone_mask |= drm_encoder_mask(encoder);
 
 	list_for_each_entry(encoder, &drm->mode_config.encoder_list, head)
 		encoder->possible_clones = clone_mask;
-- 
2.24.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 3/6] drm/exynos: Use drm_encoder_mask()

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 1785 bytes --]



Am 07.02.20 um 14:59 schrieb Ville Syrjala:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Replace the hand rolled encoder bitmask thing with drm_encoder_mask()
> 
> Cc: Inki Dae <inki.dae@samsung.com>
> Cc: Joonyoung Shim <jy0922.shim@samsung.com>
> Cc: Seung-Woo Kim <sw0312.kim@samsung.com>
> Cc: Kyungmin Park <kyungmin.park@samsung.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Acked-by: Thomas Zimmermann <tzimmermann@suse.de>

> ---
>  drivers/gpu/drm/exynos/exynos_drm_drv.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> index ba0f868b2477..57defeb44522 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> @@ -270,7 +270,7 @@ static int exynos_drm_bind(struct device *dev)
>  	struct drm_encoder *encoder;
>  	struct drm_device *drm;
>  	unsigned int clone_mask;
> -	int cnt, ret;
> +	int ret;
>  
>  	drm = drm_dev_alloc(&exynos_drm_driver, dev);
>  	if (IS_ERR(drm))
> @@ -293,10 +293,9 @@ static int exynos_drm_bind(struct device *dev)
>  	exynos_drm_mode_config_init(drm);
>  
>  	/* setup possible_clones. */
> -	cnt = 0;
>  	clone_mask = 0;
>  	list_for_each_entry(encoder, &drm->mode_config.encoder_list, head)
> -		clone_mask |= (1 << (cnt++));
> +		clone_mask |= drm_encoder_mask(encoder);
>  
>  	list_for_each_entry(encoder, &drm->mode_config.encoder_list, head)
>  		encoder->possible_clones = clone_mask;
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH v2 3/6] drm/exynos: Use drm_encoder_mask()

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 1785 bytes --]



Am 07.02.20 um 14:59 schrieb Ville Syrjala:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Replace the hand rolled encoder bitmask thing with drm_encoder_mask()
> 
> Cc: Inki Dae <inki.dae@samsung.com>
> Cc: Joonyoung Shim <jy0922.shim@samsung.com>
> Cc: Seung-Woo Kim <sw0312.kim@samsung.com>
> Cc: Kyungmin Park <kyungmin.park@samsung.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Acked-by: Thomas Zimmermann <tzimmermann@suse.de>

> ---
>  drivers/gpu/drm/exynos/exynos_drm_drv.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> index ba0f868b2477..57defeb44522 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> @@ -270,7 +270,7 @@ static int exynos_drm_bind(struct device *dev)
>  	struct drm_encoder *encoder;
>  	struct drm_device *drm;
>  	unsigned int clone_mask;
> -	int cnt, ret;
> +	int ret;
>  
>  	drm = drm_dev_alloc(&exynos_drm_driver, dev);
>  	if (IS_ERR(drm))
> @@ -293,10 +293,9 @@ static int exynos_drm_bind(struct device *dev)
>  	exynos_drm_mode_config_init(drm);
>  
>  	/* setup possible_clones. */
> -	cnt = 0;
>  	clone_mask = 0;
>  	list_for_each_entry(encoder, &drm->mode_config.encoder_list, head)
> -		clone_mask |= (1 << (cnt++));
> +		clone_mask |= drm_encoder_mask(encoder);
>  
>  	list_for_each_entry(encoder, &drm->mode_config.encoder_list, head)
>  		encoder->possible_clones = clone_mask;
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 4/6] drm/imx: Remove the bogus possible_clones setup

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

It's not at all clear what cloning options this driver supports.
So let's just clear possible_clones instead of setting it to some
bogus value.

Cc: Philipp Zabel <p.zabel@pengutronix.de>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/imx/imx-drm-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
index da87c70e413b..a0a709dfba34 100644
--- a/drivers/gpu/drm/imx/imx-drm-core.c
+++ b/drivers/gpu/drm/imx/imx-drm-core.c
@@ -140,7 +140,7 @@ int imx_drm_encoder_parse_of(struct drm_device *drm,
 	encoder->possible_crtcs = crtc_mask;
 
 	/* FIXME: this is the mask of outputs which can clone this output. */
-	encoder->possible_clones = ~0;
+	encoder->possible_clones = 0;
 
 	return 0;
 }
-- 
2.24.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[Intel-gfx] [PATCH v2 4/6] drm/imx: Remove the bogus possible_clones setup

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

It's not at all clear what cloning options this driver supports.
So let's just clear possible_clones instead of setting it to some
bogus value.

Cc: Philipp Zabel <p.zabel@pengutronix.de>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/imx/imx-drm-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
index da87c70e413b..a0a709dfba34 100644
--- a/drivers/gpu/drm/imx/imx-drm-core.c
+++ b/drivers/gpu/drm/imx/imx-drm-core.c
@@ -140,7 +140,7 @@ int imx_drm_encoder_parse_of(struct drm_device *drm,
 	encoder->possible_crtcs = crtc_mask;
 
 	/* FIXME: this is the mask of outputs which can clone this output. */
-	encoder->possible_clones = ~0;
+	encoder->possible_clones = 0;
 
 	return 0;
 }
-- 
2.24.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 4/6] drm/imx: Remove the bogus possible_clones setup

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 1285 bytes --]

Hi

Am 07.02.20 um 14:59 schrieb Ville Syrjala:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> It's not at all clear what cloning options this driver supports.
> So let's just clear possible_clones instead of setting it to some
> bogus value.
> 
> Cc: Philipp Zabel <p.zabel@pengutronix.de>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/imx/imx-drm-core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
> index da87c70e413b..a0a709dfba34 100644
> --- a/drivers/gpu/drm/imx/imx-drm-core.c
> +++ b/drivers/gpu/drm/imx/imx-drm-core.c
> @@ -140,7 +140,7 @@ int imx_drm_encoder_parse_of(struct drm_device *drm,
>  	encoder->possible_crtcs = crtc_mask;
>  
>  	/* FIXME: this is the mask of outputs which can clone this output. */
> -	encoder->possible_clones = ~0;
> +	encoder->possible_clones = 0;

Maybe remove the comment as well. It's pointless.

Best regards
Thomas

>  
>  	return 0;
>  }
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH v2 4/6] drm/imx: Remove the bogus possible_clones setup

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 1285 bytes --]

Hi

Am 07.02.20 um 14:59 schrieb Ville Syrjala:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> It's not at all clear what cloning options this driver supports.
> So let's just clear possible_clones instead of setting it to some
> bogus value.
> 
> Cc: Philipp Zabel <p.zabel@pengutronix.de>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/imx/imx-drm-core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
> index da87c70e413b..a0a709dfba34 100644
> --- a/drivers/gpu/drm/imx/imx-drm-core.c
> +++ b/drivers/gpu/drm/imx/imx-drm-core.c
> @@ -140,7 +140,7 @@ int imx_drm_encoder_parse_of(struct drm_device *drm,
>  	encoder->possible_crtcs = crtc_mask;
>  
>  	/* FIXME: this is the mask of outputs which can clone this output. */
> -	encoder->possible_clones = ~0;
> +	encoder->possible_clones = 0;

Maybe remove the comment as well. It's pointless.

Best regards
Thomas

>  
>  	return 0;
>  }
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 4/6] drm/imx: Remove the bogus possible_clones setup

[Daniel Vetter]

On Fri, Feb 07, 2020 at 03:20:44PM +0100, Thomas Zimmermann wrote:
> Hi
> 
> Am 07.02.20 um 14:59 schrieb Ville Syrjala:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > It's not at all clear what cloning options this driver supports.
> > So let's just clear possible_clones instead of setting it to some
> > bogus value.
> > 
> > Cc: Philipp Zabel <p.zabel@pengutronix.de>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/imx/imx-drm-core.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
> > index da87c70e413b..a0a709dfba34 100644
> > --- a/drivers/gpu/drm/imx/imx-drm-core.c
> > +++ b/drivers/gpu/drm/imx/imx-drm-core.c
> > @@ -140,7 +140,7 @@ int imx_drm_encoder_parse_of(struct drm_device *drm,
> >  	encoder->possible_crtcs = crtc_mask;
> >  
> >  	/* FIXME: this is the mask of outputs which can clone this output. */
> > -	encoder->possible_clones = ~0;
> > +	encoder->possible_clones = 0;
> 
> Maybe remove the comment as well. It's pointless.

Maybe change it to "FIXME: cloning support not clear, disable it all for
now". With that

Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> Best regards
> Thomas
> 
> >  
> >  	return 0;
> >  }
> > 
> 
> -- 
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Felix Imendörffer
> 




> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx


-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH v2 4/6] drm/imx: Remove the bogus possible_clones setup

[Daniel Vetter]

On Fri, Feb 07, 2020 at 03:20:44PM +0100, Thomas Zimmermann wrote:
> Hi
> 
> Am 07.02.20 um 14:59 schrieb Ville Syrjala:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > It's not at all clear what cloning options this driver supports.
> > So let's just clear possible_clones instead of setting it to some
> > bogus value.
> > 
> > Cc: Philipp Zabel <p.zabel@pengutronix.de>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/imx/imx-drm-core.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
> > index da87c70e413b..a0a709dfba34 100644
> > --- a/drivers/gpu/drm/imx/imx-drm-core.c
> > +++ b/drivers/gpu/drm/imx/imx-drm-core.c
> > @@ -140,7 +140,7 @@ int imx_drm_encoder_parse_of(struct drm_device *drm,
> >  	encoder->possible_crtcs = crtc_mask;
> >  
> >  	/* FIXME: this is the mask of outputs which can clone this output. */
> > -	encoder->possible_clones = ~0;
> > +	encoder->possible_clones = 0;
> 
> Maybe remove the comment as well. It's pointless.

Maybe change it to "FIXME: cloning support not clear, disable it all for
now". With that

Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> Best regards
> Thomas
> 
> >  
> >  	return 0;
> >  }
> > 
> 
> -- 
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Felix Imendörffer
> 




> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx


-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 5/6] drm: Validate encoder->possible_clones

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Many drivers are populating encoder->possible_clones wrong. Let's
persuade them to get it right by adding some loud WARNs.

We'll cross check the bits between any two encoders. So either
both encoders can clone with the other, or neither can.

We'll also complain about effectively empty possible_clones, and
possible_clones containing bits for encoders that don't exist.

TODO: Or should we just silently filter out any bits for non-existing
encoders?

v2: encoder->possible_clones now includes the encoder itelf

Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_encoder.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c
index f761d9306028..bc2246f27e0d 100644
--- a/drivers/gpu/drm/drm_encoder.c
+++ b/drivers/gpu/drm/drm_encoder.c
@@ -79,6 +79,34 @@ static void fixup_possible_clones(struct drm_device *dev)
 		encoder->possible_clones |= drm_encoder_mask(encoder);
 }
 
+static void validate_possible_clones(struct drm_encoder *encoder)
+{
+	struct drm_device *dev = encoder->dev;
+	struct drm_encoder *other;
+	u32 encoder_mask = 0;
+
+	drm_for_each_encoder(other, dev) {
+		encoder_mask |= drm_encoder_mask(other);
+
+		WARN(!(encoder->possible_clones & drm_encoder_mask(other)) !=
+		     !(other->possible_clones & drm_encoder_mask(encoder)),
+		     "possible_clones mismatch: "
+		     "[ENCODER:%d:%s] mask=0x%x possible_clones=0x%x vs. "
+		     "[ENCODER:%d:%s] mask=0x%x possible_clones=0x%x\n",
+		     encoder->base.id, encoder->name,
+		     drm_encoder_mask(encoder), encoder->possible_clones,
+		     other->base.id, other->name,
+		     drm_encoder_mask(other), other->possible_clones);
+	}
+
+	WARN((encoder->possible_clones & drm_encoder_mask(encoder)) == 0 ||
+	     (encoder->possible_clones & ~encoder_mask) != 0,
+	     "Bogus possible_clones: "
+	     "[ENCODER:%d:%s] possible_clones=0x%x (full encoder mask=0x%x)\n",
+	     encoder->base.id, encoder->name,
+	     encoder->possible_clones, encoder_mask);
+}
+
 int drm_encoder_register_all(struct drm_device *dev)
 {
 	struct drm_encoder *encoder;
@@ -87,6 +115,8 @@ int drm_encoder_register_all(struct drm_device *dev)
 	fixup_possible_clones(dev);
 
 	drm_for_each_encoder(encoder, dev) {
+		validate_possible_clones(encoder);
+
 		if (encoder->funcs->late_register)
 			ret = encoder->funcs->late_register(encoder);
 		if (ret)
-- 
2.24.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[Intel-gfx] [PATCH v2 5/6] drm: Validate encoder->possible_clones

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Many drivers are populating encoder->possible_clones wrong. Let's
persuade them to get it right by adding some loud WARNs.

We'll cross check the bits between any two encoders. So either
both encoders can clone with the other, or neither can.

We'll also complain about effectively empty possible_clones, and
possible_clones containing bits for encoders that don't exist.

TODO: Or should we just silently filter out any bits for non-existing
encoders?

v2: encoder->possible_clones now includes the encoder itelf

Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_encoder.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c
index f761d9306028..bc2246f27e0d 100644
--- a/drivers/gpu/drm/drm_encoder.c
+++ b/drivers/gpu/drm/drm_encoder.c
@@ -79,6 +79,34 @@ static void fixup_possible_clones(struct drm_device *dev)
 		encoder->possible_clones |= drm_encoder_mask(encoder);
 }
 
+static void validate_possible_clones(struct drm_encoder *encoder)
+{
+	struct drm_device *dev = encoder->dev;
+	struct drm_encoder *other;
+	u32 encoder_mask = 0;
+
+	drm_for_each_encoder(other, dev) {
+		encoder_mask |= drm_encoder_mask(other);
+
+		WARN(!(encoder->possible_clones & drm_encoder_mask(other)) !=
+		     !(other->possible_clones & drm_encoder_mask(encoder)),
+		     "possible_clones mismatch: "
+		     "[ENCODER:%d:%s] mask=0x%x possible_clones=0x%x vs. "
+		     "[ENCODER:%d:%s] mask=0x%x possible_clones=0x%x\n",
+		     encoder->base.id, encoder->name,
+		     drm_encoder_mask(encoder), encoder->possible_clones,
+		     other->base.id, other->name,
+		     drm_encoder_mask(other), other->possible_clones);
+	}
+
+	WARN((encoder->possible_clones & drm_encoder_mask(encoder)) == 0 ||
+	     (encoder->possible_clones & ~encoder_mask) != 0,
+	     "Bogus possible_clones: "
+	     "[ENCODER:%d:%s] possible_clones=0x%x (full encoder mask=0x%x)\n",
+	     encoder->base.id, encoder->name,
+	     encoder->possible_clones, encoder_mask);
+}
+
 int drm_encoder_register_all(struct drm_device *dev)
 {
 	struct drm_encoder *encoder;
@@ -87,6 +115,8 @@ int drm_encoder_register_all(struct drm_device *dev)
 	fixup_possible_clones(dev);
 
 	drm_for_each_encoder(encoder, dev) {
+		validate_possible_clones(encoder);
+
 		if (encoder->funcs->late_register)
 			ret = encoder->funcs->late_register(encoder);
 		if (ret)
-- 
2.24.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 5/6] drm: Validate encoder->possible_clones

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 3173 bytes --]



Am 07.02.20 um 14:59 schrieb Ville Syrjala:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Many drivers are populating encoder->possible_clones wrong. Let's
> persuade them to get it right by adding some loud WARNs.
> 
> We'll cross check the bits between any two encoders. So either
> both encoders can clone with the other, or neither can.
> 
> We'll also complain about effectively empty possible_clones, and
> possible_clones containing bits for encoders that don't exist.
> 
> TODO: Or should we just silently filter out any bits for non-existing
> encoders?

Please be noisy. Setting these masks incorrectly could be a bug.

> 
> v2: encoder->possible_clones now includes the encoder itelf
> 
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Acked-by: Thomas Zimmermann <tzimmermann@suse.de>

> ---
>  drivers/gpu/drm/drm_encoder.c | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c
> index f761d9306028..bc2246f27e0d 100644
> --- a/drivers/gpu/drm/drm_encoder.c
> +++ b/drivers/gpu/drm/drm_encoder.c
> @@ -79,6 +79,34 @@ static void fixup_possible_clones(struct drm_device *dev)
>  		encoder->possible_clones |= drm_encoder_mask(encoder);
>  }
>  
> +static void validate_possible_clones(struct drm_encoder *encoder)
> +{
> +	struct drm_device *dev = encoder->dev;
> +	struct drm_encoder *other;
> +	u32 encoder_mask = 0;
> +
> +	drm_for_each_encoder(other, dev) {
> +		encoder_mask |= drm_encoder_mask(other);
> +
> +		WARN(!(encoder->possible_clones & drm_encoder_mask(other)) !=
> +		     !(other->possible_clones & drm_encoder_mask(encoder)),
> +		     "possible_clones mismatch: "
> +		     "[ENCODER:%d:%s] mask=0x%x possible_clones=0x%x vs. "
> +		     "[ENCODER:%d:%s] mask=0x%x possible_clones=0x%x\n",
> +		     encoder->base.id, encoder->name,
> +		     drm_encoder_mask(encoder), encoder->possible_clones,
> +		     other->base.id, other->name,
> +		     drm_encoder_mask(other), other->possible_clones);
> +	}
> +
> +	WARN((encoder->possible_clones & drm_encoder_mask(encoder)) == 0 ||
> +	     (encoder->possible_clones & ~encoder_mask) != 0,
> +	     "Bogus possible_clones: "
> +	     "[ENCODER:%d:%s] possible_clones=0x%x (full encoder mask=0x%x)\n",
> +	     encoder->base.id, encoder->name,
> +	     encoder->possible_clones, encoder_mask);
> +}
> +
>  int drm_encoder_register_all(struct drm_device *dev)
>  {
>  	struct drm_encoder *encoder;
> @@ -87,6 +115,8 @@ int drm_encoder_register_all(struct drm_device *dev)
>  	fixup_possible_clones(dev);
>  
>  	drm_for_each_encoder(encoder, dev) {
> +		validate_possible_clones(encoder);
> +
>  		if (encoder->funcs->late_register)
>  			ret = encoder->funcs->late_register(encoder);
>  		if (ret)
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH v2 5/6] drm: Validate encoder->possible_clones

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 3173 bytes --]



Am 07.02.20 um 14:59 schrieb Ville Syrjala:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Many drivers are populating encoder->possible_clones wrong. Let's
> persuade them to get it right by adding some loud WARNs.
> 
> We'll cross check the bits between any two encoders. So either
> both encoders can clone with the other, or neither can.
> 
> We'll also complain about effectively empty possible_clones, and
> possible_clones containing bits for encoders that don't exist.
> 
> TODO: Or should we just silently filter out any bits for non-existing
> encoders?

Please be noisy. Setting these masks incorrectly could be a bug.

> 
> v2: encoder->possible_clones now includes the encoder itelf
> 
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Acked-by: Thomas Zimmermann <tzimmermann@suse.de>

> ---
>  drivers/gpu/drm/drm_encoder.c | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c
> index f761d9306028..bc2246f27e0d 100644
> --- a/drivers/gpu/drm/drm_encoder.c
> +++ b/drivers/gpu/drm/drm_encoder.c
> @@ -79,6 +79,34 @@ static void fixup_possible_clones(struct drm_device *dev)
>  		encoder->possible_clones |= drm_encoder_mask(encoder);
>  }
>  
> +static void validate_possible_clones(struct drm_encoder *encoder)
> +{
> +	struct drm_device *dev = encoder->dev;
> +	struct drm_encoder *other;
> +	u32 encoder_mask = 0;
> +
> +	drm_for_each_encoder(other, dev) {
> +		encoder_mask |= drm_encoder_mask(other);
> +
> +		WARN(!(encoder->possible_clones & drm_encoder_mask(other)) !=
> +		     !(other->possible_clones & drm_encoder_mask(encoder)),
> +		     "possible_clones mismatch: "
> +		     "[ENCODER:%d:%s] mask=0x%x possible_clones=0x%x vs. "
> +		     "[ENCODER:%d:%s] mask=0x%x possible_clones=0x%x\n",
> +		     encoder->base.id, encoder->name,
> +		     drm_encoder_mask(encoder), encoder->possible_clones,
> +		     other->base.id, other->name,
> +		     drm_encoder_mask(other), other->possible_clones);
> +	}
> +
> +	WARN((encoder->possible_clones & drm_encoder_mask(encoder)) == 0 ||
> +	     (encoder->possible_clones & ~encoder_mask) != 0,
> +	     "Bogus possible_clones: "
> +	     "[ENCODER:%d:%s] possible_clones=0x%x (full encoder mask=0x%x)\n",
> +	     encoder->base.id, encoder->name,
> +	     encoder->possible_clones, encoder_mask);
> +}
> +
>  int drm_encoder_register_all(struct drm_device *dev)
>  {
>  	struct drm_encoder *encoder;
> @@ -87,6 +115,8 @@ int drm_encoder_register_all(struct drm_device *dev)
>  	fixup_possible_clones(dev);
>  
>  	drm_for_each_encoder(encoder, dev) {
> +		validate_possible_clones(encoder);
> +
>  		if (encoder->funcs->late_register)
>  			ret = encoder->funcs->late_register(encoder);
>  		if (ret)
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 6/6] drm: Validate encoder->possible_crtcs

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

WARN if the encoder possible_crtcs is effectively empty or contains
bits for non-existing crtcs.

TODO: Or should we perhapst just filter out any bit for a
non-exisiting crtc?

Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_encoder.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c
index bc2246f27e0d..f16b2a2518d7 100644
--- a/drivers/gpu/drm/drm_encoder.c
+++ b/drivers/gpu/drm/drm_encoder.c
@@ -107,6 +107,23 @@ static void validate_possible_clones(struct drm_encoder *encoder)
 	     encoder->possible_clones, encoder_mask);
 }
 
+static void validate_possible_crtcs(struct drm_encoder *encoder)
+{
+	struct drm_device *dev = encoder->dev;
+	struct drm_crtc *crtc;
+	u32 crtc_mask = 0;
+
+	drm_for_each_crtc(crtc, dev)
+		crtc_mask |= drm_crtc_mask(crtc);
+
+	WARN((encoder->possible_crtcs & crtc_mask) == 0 ||
+	     (encoder->possible_crtcs & ~crtc_mask) != 0,
+	     "Bogus possible_crtcs: "
+	     "[ENCODER:%d:%s] possible_crtcs=0x%x (full crtc mask=0x%x)\n",
+	     encoder->base.id, encoder->name,
+	     encoder->possible_crtcs, crtc_mask);
+}
+
 int drm_encoder_register_all(struct drm_device *dev)
 {
 	struct drm_encoder *encoder;
@@ -115,6 +132,7 @@ int drm_encoder_register_all(struct drm_device *dev)
 	fixup_possible_clones(dev);
 
 	drm_for_each_encoder(encoder, dev) {
+		validate_possible_crtcs(encoder);
 		validate_possible_clones(encoder);
 
 		if (encoder->funcs->late_register)
-- 
2.24.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[Intel-gfx] [PATCH v2 6/6] drm: Validate encoder->possible_crtcs

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

WARN if the encoder possible_crtcs is effectively empty or contains
bits for non-existing crtcs.

TODO: Or should we perhapst just filter out any bit for a
non-exisiting crtc?

Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_encoder.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c
index bc2246f27e0d..f16b2a2518d7 100644
--- a/drivers/gpu/drm/drm_encoder.c
+++ b/drivers/gpu/drm/drm_encoder.c
@@ -107,6 +107,23 @@ static void validate_possible_clones(struct drm_encoder *encoder)
 	     encoder->possible_clones, encoder_mask);
 }
 
+static void validate_possible_crtcs(struct drm_encoder *encoder)
+{
+	struct drm_device *dev = encoder->dev;
+	struct drm_crtc *crtc;
+	u32 crtc_mask = 0;
+
+	drm_for_each_crtc(crtc, dev)
+		crtc_mask |= drm_crtc_mask(crtc);
+
+	WARN((encoder->possible_crtcs & crtc_mask) == 0 ||
+	     (encoder->possible_crtcs & ~crtc_mask) != 0,
+	     "Bogus possible_crtcs: "
+	     "[ENCODER:%d:%s] possible_crtcs=0x%x (full crtc mask=0x%x)\n",
+	     encoder->base.id, encoder->name,
+	     encoder->possible_crtcs, crtc_mask);
+}
+
 int drm_encoder_register_all(struct drm_device *dev)
 {
 	struct drm_encoder *encoder;
@@ -115,6 +132,7 @@ int drm_encoder_register_all(struct drm_device *dev)
 	fixup_possible_clones(dev);
 
 	drm_for_each_encoder(encoder, dev) {
+		validate_possible_crtcs(encoder);
 		validate_possible_clones(encoder);
 
 		if (encoder->funcs->late_register)
-- 
2.24.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 6/6] drm: Validate encoder->possible_crtcs

[Daniel Vetter]

On Fri, Feb 07, 2020 at 03:59:50PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> WARN if the encoder possible_crtcs is effectively empty or contains
> bits for non-existing crtcs.
> 
> TODO: Or should we perhapst just filter out any bit for a
> non-exisiting crtc?
> 
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

From a quick grep it looks like at least most drivers seem to get this
right. Worth a shot to find the hold-outs.

Two things:
- Imo also best to move into the drm_mode_config_validate I suggested.
- Please update the kerneldoc for drm_encoder.possible_crtcs to mention
  that this will WARN if you get it wrong (and maybe remove the line that
  most drivers screw this up).

Check itself lgtm.
-Daniel

> ---
>  drivers/gpu/drm/drm_encoder.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c
> index bc2246f27e0d..f16b2a2518d7 100644
> --- a/drivers/gpu/drm/drm_encoder.c
> +++ b/drivers/gpu/drm/drm_encoder.c
> @@ -107,6 +107,23 @@ static void validate_possible_clones(struct drm_encoder *encoder)
>  	     encoder->possible_clones, encoder_mask);
>  }
>  
> +static void validate_possible_crtcs(struct drm_encoder *encoder)
> +{
> +	struct drm_device *dev = encoder->dev;
> +	struct drm_crtc *crtc;
> +	u32 crtc_mask = 0;
> +
> +	drm_for_each_crtc(crtc, dev)
> +		crtc_mask |= drm_crtc_mask(crtc);
> +
> +	WARN((encoder->possible_crtcs & crtc_mask) == 0 ||
> +	     (encoder->possible_crtcs & ~crtc_mask) != 0,
> +	     "Bogus possible_crtcs: "
> +	     "[ENCODER:%d:%s] possible_crtcs=0x%x (full crtc mask=0x%x)\n",
> +	     encoder->base.id, encoder->name,
> +	     encoder->possible_crtcs, crtc_mask);
> +}
> +
>  int drm_encoder_register_all(struct drm_device *dev)
>  {
>  	struct drm_encoder *encoder;
> @@ -115,6 +132,7 @@ int drm_encoder_register_all(struct drm_device *dev)
>  	fixup_possible_clones(dev);
>  
>  	drm_for_each_encoder(encoder, dev) {
> +		validate_possible_crtcs(encoder);
>  		validate_possible_clones(encoder);
>  
>  		if (encoder->funcs->late_register)
> -- 
> 2.24.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH v2 6/6] drm: Validate encoder->possible_crtcs

[Daniel Vetter]

On Fri, Feb 07, 2020 at 03:59:50PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> WARN if the encoder possible_crtcs is effectively empty or contains
> bits for non-existing crtcs.
> 
> TODO: Or should we perhapst just filter out any bit for a
> non-exisiting crtc?
> 
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

From a quick grep it looks like at least most drivers seem to get this
right. Worth a shot to find the hold-outs.

Two things:
- Imo also best to move into the drm_mode_config_validate I suggested.
- Please update the kerneldoc for drm_encoder.possible_crtcs to mention
  that this will WARN if you get it wrong (and maybe remove the line that
  most drivers screw this up).

Check itself lgtm.
-Daniel

> ---
>  drivers/gpu/drm/drm_encoder.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c
> index bc2246f27e0d..f16b2a2518d7 100644
> --- a/drivers/gpu/drm/drm_encoder.c
> +++ b/drivers/gpu/drm/drm_encoder.c
> @@ -107,6 +107,23 @@ static void validate_possible_clones(struct drm_encoder *encoder)
>  	     encoder->possible_clones, encoder_mask);
>  }
>  
> +static void validate_possible_crtcs(struct drm_encoder *encoder)
> +{
> +	struct drm_device *dev = encoder->dev;
> +	struct drm_crtc *crtc;
> +	u32 crtc_mask = 0;
> +
> +	drm_for_each_crtc(crtc, dev)
> +		crtc_mask |= drm_crtc_mask(crtc);
> +
> +	WARN((encoder->possible_crtcs & crtc_mask) == 0 ||
> +	     (encoder->possible_crtcs & ~crtc_mask) != 0,
> +	     "Bogus possible_crtcs: "
> +	     "[ENCODER:%d:%s] possible_crtcs=0x%x (full crtc mask=0x%x)\n",
> +	     encoder->base.id, encoder->name,
> +	     encoder->possible_crtcs, crtc_mask);
> +}
> +
>  int drm_encoder_register_all(struct drm_device *dev)
>  {
>  	struct drm_encoder *encoder;
> @@ -115,6 +132,7 @@ int drm_encoder_register_all(struct drm_device *dev)
>  	fixup_possible_clones(dev);
>  
>  	drm_for_each_encoder(encoder, dev) {
> +		validate_possible_crtcs(encoder);
>  		validate_possible_clones(encoder);
>  
>  		if (encoder->funcs->late_register)
> -- 
> 2.24.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 6/6] drm: Validate encoder->possible_crtcs

[Ville Syrjälä]

On Fri, Feb 07, 2020 at 05:39:26PM +0100, Daniel Vetter wrote:
> On Fri, Feb 07, 2020 at 03:59:50PM +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > WARN if the encoder possible_crtcs is effectively empty or contains
> > bits for non-existing crtcs.
> > 
> > TODO: Or should we perhapst just filter out any bit for a
> > non-exisiting crtc?
> > 
> > Cc: Thomas Zimmermann <tzimmermann@suse.de>
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> >From a quick grep it looks like at least most drivers seem to get this
> right. Worth a shot to find the hold-outs.
> 
> Two things:
> - Imo also best to move into the drm_mode_config_validate I suggested.
> - Please update the kerneldoc for drm_encoder.possible_crtcs to mention
>   that this will WARN if you get it wrong (and maybe remove the line that
>   most drivers screw this up).

ack

> 
> Check itself lgtm.
> -Daniel
> 
> > ---
> >  drivers/gpu/drm/drm_encoder.c | 18 ++++++++++++++++++
> >  1 file changed, 18 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c
> > index bc2246f27e0d..f16b2a2518d7 100644
> > --- a/drivers/gpu/drm/drm_encoder.c
> > +++ b/drivers/gpu/drm/drm_encoder.c
> > @@ -107,6 +107,23 @@ static void validate_possible_clones(struct drm_encoder *encoder)
> >  	     encoder->possible_clones, encoder_mask);
> >  }
> >  
> > +static void validate_possible_crtcs(struct drm_encoder *encoder)
> > +{
> > +	struct drm_device *dev = encoder->dev;
> > +	struct drm_crtc *crtc;
> > +	u32 crtc_mask = 0;
> > +
> > +	drm_for_each_crtc(crtc, dev)
> > +		crtc_mask |= drm_crtc_mask(crtc);
> > +
> > +	WARN((encoder->possible_crtcs & crtc_mask) == 0 ||
> > +	     (encoder->possible_crtcs & ~crtc_mask) != 0,
> > +	     "Bogus possible_crtcs: "
> > +	     "[ENCODER:%d:%s] possible_crtcs=0x%x (full crtc mask=0x%x)\n",
> > +	     encoder->base.id, encoder->name,
> > +	     encoder->possible_crtcs, crtc_mask);
> > +}
> > +
> >  int drm_encoder_register_all(struct drm_device *dev)
> >  {
> >  	struct drm_encoder *encoder;
> > @@ -115,6 +132,7 @@ int drm_encoder_register_all(struct drm_device *dev)
> >  	fixup_possible_clones(dev);
> >  
> >  	drm_for_each_encoder(encoder, dev) {
> > +		validate_possible_crtcs(encoder);
> >  		validate_possible_clones(encoder);
> >  
> >  		if (encoder->funcs->late_register)
> > -- 
> > 2.24.1
> > 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH v2 6/6] drm: Validate encoder->possible_crtcs

[Ville Syrjälä]

On Fri, Feb 07, 2020 at 05:39:26PM +0100, Daniel Vetter wrote:
> On Fri, Feb 07, 2020 at 03:59:50PM +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > WARN if the encoder possible_crtcs is effectively empty or contains
> > bits for non-existing crtcs.
> > 
> > TODO: Or should we perhapst just filter out any bit for a
> > non-exisiting crtc?
> > 
> > Cc: Thomas Zimmermann <tzimmermann@suse.de>
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> >From a quick grep it looks like at least most drivers seem to get this
> right. Worth a shot to find the hold-outs.
> 
> Two things:
> - Imo also best to move into the drm_mode_config_validate I suggested.
> - Please update the kerneldoc for drm_encoder.possible_crtcs to mention
>   that this will WARN if you get it wrong (and maybe remove the line that
>   most drivers screw this up).

ack

> 
> Check itself lgtm.
> -Daniel
> 
> > ---
> >  drivers/gpu/drm/drm_encoder.c | 18 ++++++++++++++++++
> >  1 file changed, 18 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c
> > index bc2246f27e0d..f16b2a2518d7 100644
> > --- a/drivers/gpu/drm/drm_encoder.c
> > +++ b/drivers/gpu/drm/drm_encoder.c
> > @@ -107,6 +107,23 @@ static void validate_possible_clones(struct drm_encoder *encoder)
> >  	     encoder->possible_clones, encoder_mask);
> >  }
> >  
> > +static void validate_possible_crtcs(struct drm_encoder *encoder)
> > +{
> > +	struct drm_device *dev = encoder->dev;
> > +	struct drm_crtc *crtc;
> > +	u32 crtc_mask = 0;
> > +
> > +	drm_for_each_crtc(crtc, dev)
> > +		crtc_mask |= drm_crtc_mask(crtc);
> > +
> > +	WARN((encoder->possible_crtcs & crtc_mask) == 0 ||
> > +	     (encoder->possible_crtcs & ~crtc_mask) != 0,
> > +	     "Bogus possible_crtcs: "
> > +	     "[ENCODER:%d:%s] possible_crtcs=0x%x (full crtc mask=0x%x)\n",
> > +	     encoder->base.id, encoder->name,
> > +	     encoder->possible_crtcs, crtc_mask);
> > +}
> > +
> >  int drm_encoder_register_all(struct drm_device *dev)
> >  {
> >  	struct drm_encoder *encoder;
> > @@ -115,6 +132,7 @@ int drm_encoder_register_all(struct drm_device *dev)
> >  	fixup_possible_clones(dev);
> >  
> >  	drm_for_each_encoder(encoder, dev) {
> > +		validate_possible_crtcs(encoder);
> >  		validate_possible_clones(encoder);
> >  
> >  		if (encoder->funcs->late_register)
> > -- 
> > 2.24.1
> > 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] ✓ Fi.CI.BAT: success for drm: Try to fix encoder possible_clones/crtc (rev2)

[Patchwork]

== Series Details ==

Series: drm: Try to fix encoder possible_clones/crtc (rev2)
URL   : https://patchwork.freedesktop.org/series/63399/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_7886 -> Patchwork_16482
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16482/index.html

Known issues
------------

  Here are the changes found in Patchwork_16482 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_close_race@basic-threads:
    - fi-byt-j1900:       [PASS][1] -> [INCOMPLETE][2] ([i915#45])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7886/fi-byt-j1900/igt@gem_close_race@basic-threads.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16482/fi-byt-j1900/igt@gem_close_race@basic-threads.html

  * igt@i915_selftest@live_blt:
    - fi-hsw-4770r:       [PASS][3] -> [DMESG-FAIL][4] ([i915#725])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7886/fi-hsw-4770r/igt@i915_selftest@live_blt.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16482/fi-hsw-4770r/igt@i915_selftest@live_blt.html

  * igt@kms_chamelium@hdmi-hpd-fast:
    - fi-icl-u2:          [PASS][5] -> [FAIL][6] ([i915#217])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7886/fi-icl-u2/igt@kms_chamelium@hdmi-hpd-fast.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16482/fi-icl-u2/igt@kms_chamelium@hdmi-hpd-fast.html

  * igt@kms_flip@basic-flip-vs-dpms:
    - fi-skl-6770hq:      [PASS][7] -> [SKIP][8] ([fdo#109271]) +27 similar issues
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7886/fi-skl-6770hq/igt@kms_flip@basic-flip-vs-dpms.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16482/fi-skl-6770hq/igt@kms_flip@basic-flip-vs-dpms.html

  
#### Possible fixes ####

  * igt@i915_selftest@live_blt:
    - fi-hsw-4770:        [DMESG-FAIL][9] ([i915#553] / [i915#725]) -> [PASS][10]
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7886/fi-hsw-4770/igt@i915_selftest@live_blt.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16482/fi-hsw-4770/igt@i915_selftest@live_blt.html

  * igt@i915_selftest@live_execlists:
    - fi-icl-y:           [DMESG-FAIL][11] ([fdo#108569]) -> [PASS][12]
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7886/fi-icl-y/igt@i915_selftest@live_execlists.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16482/fi-icl-y/igt@i915_selftest@live_execlists.html

  * igt@i915_selftest@live_gem_contexts:
    - fi-kbl-x1275:       [DMESG-FAIL][13] ([i915#943]) -> [PASS][14]
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7886/fi-kbl-x1275/igt@i915_selftest@live_gem_contexts.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16482/fi-kbl-x1275/igt@i915_selftest@live_gem_contexts.html
    - fi-byt-n2820:       [DMESG-FAIL][15] ([i915#1052]) -> [PASS][16]
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7886/fi-byt-n2820/igt@i915_selftest@live_gem_contexts.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16482/fi-byt-n2820/igt@i915_selftest@live_gem_contexts.html

  * igt@i915_selftest@live_gt_pm:
    - {fi-tgl-dsi}:       [DMESG-FAIL][17] -> [PASS][18]
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7886/fi-tgl-dsi/igt@i915_selftest@live_gt_pm.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16482/fi-tgl-dsi/igt@i915_selftest@live_gt_pm.html

  * igt@i915_selftest@live_gtt:
    - fi-bxt-dsi:         [TIMEOUT][19] ([fdo#112271]) -> [PASS][20]
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7886/fi-bxt-dsi/igt@i915_selftest@live_gtt.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16482/fi-bxt-dsi/igt@i915_selftest@live_gtt.html

  * igt@kms_chamelium@hdmi-hpd-fast:
    - fi-kbl-7500u:       [FAIL][21] ([fdo#111096] / [i915#323]) -> [PASS][22]
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7886/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16482/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html

  
#### Warnings ####

  * igt@gem_exec_parallel@contexts:
    - fi-byt-n2820:       [FAIL][23] ([i915#694]) -> [TIMEOUT][24] ([fdo#112271] / [i915#1084])
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7886/fi-byt-n2820/igt@gem_exec_parallel@contexts.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16482/fi-byt-n2820/igt@gem_exec_parallel@contexts.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#108569]: https://bugs.freedesktop.org/show_bug.cgi?id=108569
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#111096]: https://bugs.freedesktop.org/show_bug.cgi?id=111096
  [fdo#112271]: https://bugs.freedesktop.org/show_bug.cgi?id=112271
  [i915#1052]: https://gitlab.freedesktop.org/drm/intel/issues/1052
  [i915#1084]: https://gitlab.freedesktop.org/drm/intel/issues/1084
  [i915#217]: https://gitlab.freedesktop.org/drm/intel/issues/217
  [i915#323]: https://gitlab.freedesktop.org/drm/intel/issues/323
  [i915#45]: https://gitlab.freedesktop.org/drm/intel/issues/45
  [i915#553]: https://gitlab.freedesktop.org/drm/intel/issues/553
  [i915#694]: https://gitlab.freedesktop.org/drm/intel/issues/694
  [i915#725]: https://gitlab.freedesktop.org/drm/intel/issues/725
  [i915#943]: https://gitlab.freedesktop.org/drm/intel/issues/943


Participating hosts (54 -> 42)
------------------------------

  Missing    (12): fi-ilk-m540 fi-hsw-4200u fi-hsw-peppy fi-byt-squawks fi-bsw-cyan fi-cfl-guc fi-ctg-p8600 fi-ivb-3770 fi-kbl-7560u fi-byt-clapper fi-skl-6700k2 fi-snb-2600 


Build changes
-------------

  * CI: CI-20190529 -> None
  * Linux: CI_DRM_7886 -> Patchwork_16482

  CI-20190529: 20190529
  CI_DRM_7886: c76da740823aa950e340a8e53758511680da79ca @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5425: ad4542ef1adbaa1227bc9ba9e24bb0e0f6dd408d @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_16482: 3e57a6ac85fa82e45de24c0fe193644ff89d96fd @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

3e57a6ac85fa drm: Validate encoder->possible_crtcs
c9052ff0cc3f drm: Validate encoder->possible_clones
2b6316f16a22 drm/imx: Remove the bogus possible_clones setup
d5b660531144 drm/exynos: Use drm_encoder_mask()
20905e49f113 drm/gma500: Sanitize possible_clones
3ead1c5c3666 drm: Include the encoder itself in possible_clones

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16482/index.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] ✓ Fi.CI.IGT: success for drm: Try to fix encoder possible_clones/crtc (rev2)

[Patchwork]

== Series Details ==

Series: drm: Try to fix encoder possible_clones/crtc (rev2)
URL   : https://patchwork.freedesktop.org/series/63399/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_7886_full -> Patchwork_16482_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  

Known issues
------------

  Here are the changes found in Patchwork_16482_full that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_ctx_isolation@rcs0-s3:
    - shard-kbl:          [PASS][1] -> [DMESG-WARN][2] ([i915#180]) +5 similar issues
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7886/shard-kbl3/igt@gem_ctx_isolation@rcs0-s3.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16482/shard-kbl4/igt@gem_ctx_isolation@rcs0-s3.html

  * igt@gem_exec_balancer@smoke:
    - shard-iclb:         [PASS][3] -> [SKIP][4] ([fdo#110854])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7886/shard-iclb1/igt@gem_exec_balancer@smoke.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16482/shard-iclb6/igt@gem_exec_balancer@smoke.html

  * igt@gem_exec_parallel@vcs1-fds:
    - shard-iclb:         [PASS][5] -> [SKIP][6] ([fdo#112080]) +13 similar issues
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7886/shard-iclb4/igt@gem_exec_parallel@vcs1-fds.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16482/shard-iclb5/igt@gem_exec_parallel@vcs1-fds.html

  * igt@gem_exec_schedule@reorder-wide-bsd:
    - shard-iclb:         [PASS][7] -> [SKIP][8] ([fdo#112146]) +6 similar issues
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7886/shard-iclb3/igt@gem_exec_schedule@reorder-wide-bsd.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16482/shard-iclb1/igt@gem_exec_schedule@reorder-wide-bsd.html

  * igt@gem_workarounds@suspend-resume-context:
    - shard-apl:          [PASS][9] -> [DMESG-WARN][10] ([i915#180]) +2 similar issues
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7886/shard-apl8/igt@gem_workarounds@suspend-resume-context.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16482/shard-apl6/igt@gem_workarounds@suspend-resume-context.html

  * igt@gen9_exec_parse@allowed-all:
    - shard-glk:          [PASS][11] -> [DMESG-WARN][12] ([i915#716])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7886/shard-glk6/igt@gen9_exec_parse@allowed-all.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16482/shard-glk4/igt@gen9_exec_parse@allowed-all.html

  * igt@i915_pm_dc@dc5-dpms:
    - shard-iclb:         [PASS][13] -> [FAIL][14] ([i915#447])
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7886/shard-iclb4/igt@i915_pm_dc@dc5-dpms.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16482/shard-iclb3/igt@i915_pm_dc@dc5-dpms.html

  * igt@i915_pm_dc@dc6-psr:
    - shard-iclb:         [PASS][15] -> [FAIL][16] ([i915#454])
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7886/shard-iclb1/igt@i915_pm_dc@dc6-psr.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16482/shard-iclb8/igt@i915_pm_dc@dc6-psr.html

  * igt@kms_plane_lowres@pipe-a-tiling-x:
    - shard-glk:          [PASS][17] -> [FAIL][18] ([i915#899])
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7886/shard-glk2/igt@kms_plane_lowres@pipe-a-tiling-x.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16482/shard-glk9/igt@kms_plane_lowres@pipe-a-tiling-x.html

  * igt@kms_psr@psr2_cursor_mmap_cpu:
    - shard-iclb:         [PASS][19] -> [SKIP][20] ([fdo#109441]) +2 similar issues
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7886/shard-iclb2/igt@kms_psr@psr2_cursor_mmap_cpu.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16482/shard-iclb8/igt@kms_psr@psr2_cursor_mmap_cpu.html

  * igt@prime_vgem@fence-wait-bsd2:
    - shard-iclb:         [PASS][21] -> [SKIP][22] ([fdo#109276]) +26 similar issues
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7886/shard-iclb2/igt@prime_vgem@fence-wait-bsd2.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16482/shard-iclb8/igt@prime_vgem@fence-wait-bsd2.html

  
#### Possible fixes ####

  * igt@gem_ctx_isolation@rcs0-s3:
    - shard-apl:          [DMESG-WARN][23] ([i915#180]) -> [PASS][24] +3 similar issues
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7886/shard-apl1/igt@gem_ctx_isolation@rcs0-s3.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16482/shard-apl6/igt@gem_ctx_isolation@rcs0-s3.html

  * igt@gem_exec_balancer@hang:
    - shard-tglb:         [TIMEOUT][25] ([fdo#112271]) -> [PASS][26]
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7886/shard-tglb8/igt@gem_exec_balancer@hang.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16482/shard-tglb5/igt@gem_exec_balancer@hang.html

  * igt@gem_exec_schedule@in-order-bsd:
    - shard-iclb:         [SKIP][27] ([fdo#112146]) -> [PASS][28] +6 similar issues
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7886/shard-iclb1/igt@gem_exec_schedule@in-order-bsd.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16482/shard-iclb3/igt@gem_exec_schedule@in-order-bsd.html

  * igt@gem_exec_schedule@pi-shared-iova-bsd:
    - shard-iclb:         [SKIP][29] ([i915#677]) -> [PASS][30] +1 similar issue
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7886/shard-iclb2/igt@gem_exec_schedule@pi-shared-iova-bsd.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16482/shard-iclb8/igt@gem_exec_schedule@pi-shared-iova-bsd.html

  * igt@i915_pm_rps@waitboost:
    - shard-iclb:         [FAIL][31] ([i915#413]) -> [PASS][32]
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7886/shard-iclb7/igt@i915_pm_rps@waitboost.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16482/shard-iclb7/igt@i915_pm_rps@waitboost.html

  * igt@kms_cursor_crc@pipe-c-cursor-suspend:
    - shard-kbl:          [DMESG-WARN][33] ([i915#180]) -> [PASS][34] +6 similar issues
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7886/shard-kbl7/igt@kms_cursor_crc@pipe-c-cursor-suspend.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16482/shard-kbl3/igt@kms_cursor_crc@pipe-c-cursor-suspend.html

  * igt@kms_frontbuffer_tracking@psr-1p-primscrn-pri-shrfb-draw-render:
    - shard-tglb:         [SKIP][35] ([i915#668]) -> [PASS][36] +1 similar issue
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7886/shard-tglb5/igt@kms_frontbuffer_tracking@psr-1p-primscrn-pri-shrfb-draw-render.html
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16482/shard-tglb7/igt@kms_frontbuffer_tracking@psr-1p-primscrn-pri-shrfb-draw-render.html

  * igt@kms_plane@plane-panning-bottom-right-suspend-pipe-a-planes:
    - shard-kbl:          [FAIL][37] ([fdo#103375]) -> [PASS][38]
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7886/shard-kbl4/igt@kms_plane@plane-panning-bottom-right-suspend-pipe-a-planes.html
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16482/shard-kbl6/igt@kms_plane@plane-panning-bottom-right-suspend-pipe-a-planes.html

  * igt@kms_psr2_su@page_flip:
    - shard-iclb:         [SKIP][39] ([fdo#109642] / [fdo#111068]) -> [PASS][40]
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7886/shard-iclb5/igt@kms_psr2_su@page_flip.html
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16482/shard-iclb2/igt@kms_psr2_su@page_flip.html

  * igt@kms_psr@psr2_cursor_plane_onoff:
    - shard-iclb:         [SKIP][41] ([fdo#109441]) -> [PASS][42] +1 similar issue
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7886/shard-iclb5/igt@kms_psr@psr2_cursor_plane_onoff.html
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16482/shard-iclb2/igt@kms_psr@psr2_cursor_plane_onoff.html

  * igt@kms_vblank@pipe-a-ts-continuation-dpms-suspend:
    - shard-kbl:          [INCOMPLETE][43] ([fdo#103665]) -> [PASS][44]
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7886/shard-kbl2/igt@kms_vblank@pipe-a-ts-continuation-dpms-suspend.html
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16482/shard-kbl1/igt@kms_vblank@pipe-a-ts-continuation-dpms-suspend.html

  * igt@perf_pmu@busy-no-semaphores-vcs1:
    - shard-iclb:         [SKIP][45] ([fdo#112080]) -> [PASS][46] +15 similar issues
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7886/shard-iclb8/igt@perf_pmu@busy-no-semaphores-vcs1.html
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16482/shard-iclb4/igt@perf_pmu@busy-no-semaphores-vcs1.html

  * igt@prime_busy@hang-bsd2:
    - shard-iclb:         [SKIP][47] ([fdo#109276]) -> [PASS][48] +21 similar issues
   [47]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7886/shard-iclb8/igt@prime_busy@hang-bsd2.html
   [48]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16482/shard-iclb4/igt@prime_busy@hang-bsd2.html

  
#### Warnings ####

  * igt@kms_dp_dsc@basic-dsc-enable-edp:
    - shard-iclb:         [SKIP][49] ([fdo#109349]) -> [DMESG-WARN][50] ([fdo#107724])
   [49]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7886/shard-iclb5/igt@kms_dp_dsc@basic-dsc-enable-edp.html
   [50]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16482/shard-iclb2/igt@kms_dp_dsc@basic-dsc-enable-edp.html

  
  [fdo#103375]: https://bugs.freedesktop.org/show_bug.cgi?id=103375
  [fdo#103665]: https://bugs.freedesktop.org/show_bug.cgi?id=103665
  [fdo#107724]: https://bugs.freedesktop.org/show_bug.cgi?id=107724
  [fdo#109276]: https://bugs.freedesktop.org/show_bug.cgi?id=109276
  [fdo#109349]: https://bugs.freedesktop.org/show_bug.cgi?id=109349
  [fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441
  [fdo#109642]: https://bugs.freedesktop.org/show_bug.cgi?id=109642
  [fdo#110854]: https://bugs.freedesktop.org/show_bug.cgi?id=110854
  [fdo#111068]: https://bugs.freedesktop.org/show_bug.cgi?id=111068
  [fdo#112080]: https://bugs.freedesktop.org/show_bug.cgi?id=112080
  [fdo#112146]: https://bugs.freedesktop.org/show_bug.cgi?id=112146
  [fdo#112271]: https://bugs.freedesktop.org/show_bug.cgi?id=112271
  [i915#180]: https://gitlab.freedesktop.org/drm/intel/issues/180
  [i915#413]: https://gitlab.freedesktop.org/drm/intel/issues/413
  [i915#447]: https://gitlab.freedesktop.org/drm/intel/issues/447
  [i915#454]: https://gitlab.freedesktop.org/drm/intel/issues/454
  [i915#668]: https://gitlab.freedesktop.org/drm/intel/issues/668
  [i915#677]: https://gitlab.freedesktop.org/drm/intel/issues/677
  [i915#716]: https://gitlab.freedesktop.org/drm/intel/issues/716
  [i915#899]: https://gitlab.freedesktop.org/drm/intel/issues/899


Participating hosts (10 -> 10)
------------------------------

  No changes in participating hosts


Build changes
-------------

  * CI: CI-20190529 -> None
  * Linux: CI_DRM_7886 -> Patchwork_16482

  CI-20190529: 20190529
  CI_DRM_7886: c76da740823aa950e340a8e53758511680da79ca @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5425: ad4542ef1adbaa1227bc9ba9e24bb0e0f6dd408d @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_16482: 3e57a6ac85fa82e45de24c0fe193644ff89d96fd @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16482/index.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx