| Message ID | 1531385336-17418-1-git-send-email-alexandru-cosmin.gheorghe@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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.
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;
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(+)