[PATCH] drm: mali-dp: Set encoder possible_clones

[PATCH] drm: mali-dp: Set encoder possible_clones

[Alexandru Gheorghe]

Set possible_clones field to report that the writeback connector and
the one driving the display could be enabled at the same time.

Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
---
 drivers/gpu/drm/arm/malidp_drv.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c
index 5b72605..3664da9 100644
--- a/drivers/gpu/drm/arm/malidp_drv.c
+++ b/drivers/gpu/drm/arm/malidp_drv.c
@@ -616,6 +616,8 @@ static int malidp_bind(struct device *dev)
 	struct malidp_hw_device *hwdev;
 	struct platform_device *pdev = to_platform_device(dev);
 	struct of_device_id const *dev_id;
+	struct drm_encoder *encoder;
+	int num_encoders = 0;
 	/* number of lines for the R, G and B output */
 	u8 output_width[MAX_OUTPUT_CHANNELS];
 	int ret = 0, i;
@@ -737,6 +739,14 @@ static int malidp_bind(struct device *dev)
 		goto bind_fail;
 	}
 
+	list_for_each_entry(encoder, &drm->mode_config.encoder_list, head) {
+		num_encoders++;
+	}
+
+	list_for_each_entry(encoder, &drm->mode_config.encoder_list, head) {
+		encoder->possible_clones = (1 << num_encoders) -  1;
+	}
+
 	ret = malidp_irq_init(pdev);
 	if (ret < 0)
 		goto irq_init_fail;
-- 
2.7.4

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

Re: [PATCH] drm: mali-dp: Set encoder possible_clones

[Ville Syrjälä]

On Thu, Jul 12, 2018 at 09:48:56AM +0100, Alexandru Gheorghe wrote:
> Set possible_clones field to report that the writeback connector and
> the one driving the display could be enabled at the same time.
> 
> Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
> ---
>  drivers/gpu/drm/arm/malidp_drv.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c
> index 5b72605..3664da9 100644
> --- a/drivers/gpu/drm/arm/malidp_drv.c
> +++ b/drivers/gpu/drm/arm/malidp_drv.c
> @@ -616,6 +616,8 @@ static int malidp_bind(struct device *dev)
>  	struct malidp_hw_device *hwdev;
>  	struct platform_device *pdev = to_platform_device(dev);
>  	struct of_device_id const *dev_id;
> +	struct drm_encoder *encoder;
> +	int num_encoders = 0;
>  	/* number of lines for the R, G and B output */
>  	u8 output_width[MAX_OUTPUT_CHANNELS];
>  	int ret = 0, i;
> @@ -737,6 +739,14 @@ static int malidp_bind(struct device *dev)
>  		goto bind_fail;
>  	}
>  
> +	list_for_each_entry(encoder, &drm->mode_config.encoder_list, head) {
> +		num_encoders++;
> +	}

dev->mode_config.num_encoder ?

> +
> +	list_for_each_entry(encoder, &drm->mode_config.encoder_list, head) {
> +		encoder->possible_clones = (1 << num_encoders) -  1;
> +	}

Your commit message speaks only about two specific encoders but here
you're claiming any encoder can be cloned with any other. I can't
actually figure out how many encoders there can be because the encoder
registration seems to happen by magic means. Some explanation would
be nice.

I'm interested because I'd like to clean up the possible_clones stuff
to actually reflect reality so that userspace could actually use that
information for something. See
https://patchwork.freedesktop.org/series/44849/

> +
>  	ret = malidp_irq_init(pdev);
>  	if (ret < 0)
>  		goto irq_init_fail;
> -- 
> 2.7.4
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

Re: [PATCH] drm: mali-dp: Set encoder possible_clones

[Alexandru-Cosmin Gheorghe]

On Thu, Jul 12, 2018 at 03:37:36PM +0300, Ville Syrjälä wrote:
> On Thu, Jul 12, 2018 at 09:48:56AM +0100, Alexandru Gheorghe wrote:
> > Set possible_clones field to report that the writeback connector and
> > the one driving the display could be enabled at the same time.
> > 
> > Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
> > ---
> >  drivers/gpu/drm/arm/malidp_drv.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c
> > index 5b72605..3664da9 100644
> > --- a/drivers/gpu/drm/arm/malidp_drv.c
> > +++ b/drivers/gpu/drm/arm/malidp_drv.c
> > @@ -616,6 +616,8 @@ static int malidp_bind(struct device *dev)
> >  	struct malidp_hw_device *hwdev;
> >  	struct platform_device *pdev = to_platform_device(dev);
> >  	struct of_device_id const *dev_id;
> > +	struct drm_encoder *encoder;
> > +	int num_encoders = 0;
> >  	/* number of lines for the R, G and B output */
> >  	u8 output_width[MAX_OUTPUT_CHANNELS];
> >  	int ret = 0, i;
> > @@ -737,6 +739,14 @@ static int malidp_bind(struct device *dev)
> >  		goto bind_fail;
> >  	}
> >  
> > +	list_for_each_entry(encoder, &drm->mode_config.encoder_list, head) {
> > +		num_encoders++;
> > +	}
> 
> dev->mode_config.num_encoder ?

Thanks for that, I will use it.

> 
> > +
> > +	list_for_each_entry(encoder, &drm->mode_config.encoder_list, head) {
> > +		encoder->possible_clones = (1 << num_encoders) -  1;
> > +	}
> 
> Your commit message speaks only about two specific encoders but here
> you're claiming any encoder can be cloned with any other. I can't
> actually figure out how many encoders there can be because the encoder
> registration seems to happen by magic means. Some explanation would
> be nice.
> 
> I'm interested because I'd like to clean up the possible_clones stuff
> to actually reflect reality so that userspace could actually use that
> information for something. See
> https://patchwork.freedesktop.org/series/44849/
> 

There are just two encoders, encoder->possible_clones = 0x3 would've
done the job, do you think I should use it to better convey intent?


> > +
> >  	ret = malidp_irq_init(pdev);
> >  	if (ret < 0)
> >  		goto irq_init_fail;
> > -- 
> > 2.7.4
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Ville Syrjälä
> Intel
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

Re: [PATCH] drm: mali-dp: Set encoder possible_clones

[Ville Syrjälä]

On Thu, Jul 12, 2018 at 01:54:07PM +0100, Alexandru-Cosmin Gheorghe wrote:
> On Thu, Jul 12, 2018 at 03:37:36PM +0300, Ville Syrjälä wrote:
> > On Thu, Jul 12, 2018 at 09:48:56AM +0100, Alexandru Gheorghe wrote:
> > > Set possible_clones field to report that the writeback connector and
> > > the one driving the display could be enabled at the same time.
> > > 
> > > Signed-off-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
> > > ---
> > >  drivers/gpu/drm/arm/malidp_drv.c | 10 ++++++++++
> > >  1 file changed, 10 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c
> > > index 5b72605..3664da9 100644
> > > --- a/drivers/gpu/drm/arm/malidp_drv.c
> > > +++ b/drivers/gpu/drm/arm/malidp_drv.c
> > > @@ -616,6 +616,8 @@ static int malidp_bind(struct device *dev)
> > >  	struct malidp_hw_device *hwdev;
> > >  	struct platform_device *pdev = to_platform_device(dev);
> > >  	struct of_device_id const *dev_id;
> > > +	struct drm_encoder *encoder;
> > > +	int num_encoders = 0;
> > >  	/* number of lines for the R, G and B output */
> > >  	u8 output_width[MAX_OUTPUT_CHANNELS];
> > >  	int ret = 0, i;
> > > @@ -737,6 +739,14 @@ static int malidp_bind(struct device *dev)
> > >  		goto bind_fail;
> > >  	}
> > >  
> > > +	list_for_each_entry(encoder, &drm->mode_config.encoder_list, head) {
> > > +		num_encoders++;
> > > +	}
> > 
> > dev->mode_config.num_encoder ?
> 
> Thanks for that, I will use it.
> 
> > 
> > > +
> > > +	list_for_each_entry(encoder, &drm->mode_config.encoder_list, head) {
> > > +		encoder->possible_clones = (1 << num_encoders) -  1;
> > > +	}
> > 
> > Your commit message speaks only about two specific encoders but here
> > you're claiming any encoder can be cloned with any other. I can't
> > actually figure out how many encoders there can be because the encoder
> > registration seems to happen by magic means. Some explanation would
> > be nice.
> > 
> > I'm interested because I'd like to clean up the possible_clones stuff
> > to actually reflect reality so that userspace could actually use that
> > information for something. See
> > https://patchwork.freedesktop.org/series/44849/
> > 
> 
> There are just two encoders, encoder->possible_clones = 0x3 would've
> done the job, do you think I should use it to better convey intent?

Are there any direct pointers to those encoders? If there are I
might recommend a more explicit
 a.possible_clones = drm_encoder_mask(a) | drm_encoder_mask(b);
 b.possible_clones = drm_encodrr_mask(a) | drm_encoder_mask(b);
or something like that.

But if there aren't then I think the num_encoder thing should work
fine. But I would suggest adding a comment to explain the situation,
and maybe even a WARN_ON(num_encoder != 2) so that people will
realize that this code needs to be updated if more encoders show up
in the future.

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