* [GIT PULL FOR v3.19] R-Car DU changes
@ 2014-11-07 6:25 Laurent Pinchart
2014-11-07 9:19 ` Daniel Vetter
0 siblings, 1 reply; 11+ messages in thread
From: Laurent Pinchart @ 2014-11-07 6:25 UTC (permalink / raw)
To: dri-devel; +Cc: magnus.damm, Simon Horman
Hi Dave,
Here's a pull request that adds HDMI support to the R-Car DU driver, including
a new slave encoder driver for the adv7511.
The branch is based on a merge of drm-next and Simon's tags/renesas-dt-du-for-
v3.19 available at
git://git.kernel.org/pub/scm/linux/kernel/git/horms/renesas.git, for which a
pull request has been submitted for v3.19 to the ARM SoC maintainers.
I'm leaving now for two weeks of holidays and would like to avoid missing the
merge window. Simon, could you please confirm that tags/renesas-dt-du-for-
v3.19 is stable and ack this pull request (and possibly ping Dave on my behalf
;-)) ?
The following changes since commit 19de43d4dea8712b3574d72d3d8aa16cd17e7fed:
Merge tag 'tags/renesas-dt-du-for-v3.19' into drm/next/adv7511-base
(2014-11-07 08:06:16 +0200)
are available in the git repository at:
git://linuxtv.org/pinchartl/fbdev.git
for you to fetch changes up to cc47927ca851110d2c387a7d9fd7086112abb92e:
drm: Add adv7511 encoder driver (2014-11-07 08:19:06 +0200)
----------------------------------------------------------------
Lars-Peter Clausen (2):
drm: Decouple EDID parsing from I2C adapter
drm: Add adv7511 encoder driver
Laurent Pinchart (6):
drm: rcar-du: Remove platform data support
drm: rcar-du: Pass the encoder DT node to rcar_du_encoder_init()
drm: rcar-du: Replace direct DRM encoder access with cast macro
drm: rcar-du: Replace drm_encoder with drm_slave_encoder
drm: rcar-du: Add HDMI encoder and connector support
video: Add ADV751[13] DT bindings documentation
Documentation/devicetree/bindings/video/adi,adv7511.txt | 88 ++
drivers/gpu/drm/drm_edid.c | 27 +-
drivers/gpu/drm/i2c/Kconfig | 6 +
drivers/gpu/drm/i2c/Makefile | 2 +
drivers/gpu/drm/i2c/adv7511.c | 1010 ++++++++++++++
drivers/gpu/drm/i2c/adv7511.h | 289 +++++++
drivers/gpu/drm/rcar-du/Kconfig | 11 +-
drivers/gpu/drm/rcar-du/Makefile | 2 +
drivers/gpu/drm/rcar-du/rcar_du_crtc.h | 10 +-
drivers/gpu/drm/rcar-du/rcar_du_drv.c | 4 +-
drivers/gpu/drm/rcar-du/rcar_du_drv.h | 2 -
drivers/gpu/drm/rcar-du/rcar_du_encoder.c | 45 +-
drivers/gpu/drm/rcar-du/rcar_du_encoder.h | 23 +-
drivers/gpu/drm/rcar-du/rcar_du_hdmicon.c | 118 +++
drivers/gpu/drm/rcar-du/rcar_du_hdmicon.h | 31 +
drivers/gpu/drm/rcar-du/rcar_du_hdmienc.c | 151 ++++
drivers/gpu/drm/rcar-du/rcar_du_hdmienc.h | 35 +
drivers/gpu/drm/rcar-du/rcar_du_kms.c | 53 +-
drivers/gpu/drm/rcar-du/rcar_du_lvdscon.c | 31 +-
drivers/gpu/drm/rcar-du/rcar_du_lvdscon.h | 2 -
drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.h | 1 -
drivers/gpu/drm/rcar-du/rcar_du_vgacon.c | 5 +-
include/drm/drm_edid.h | 5 +
include/linux/platform_data/rcar-du.h | 74 --
24 files changed, 1845 insertions(+), 180 deletions(-)
create mode 100644 Documentation/devicetree/bindings/video/adi,adv7511.txt
create mode 100644 drivers/gpu/drm/i2c/adv7511.c
create mode 100644 drivers/gpu/drm/i2c/adv7511.h
create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_hdmicon.c
create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_hdmicon.h
create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_hdmienc.c
create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_hdmienc.h
delete mode 100644 include/linux/platform_data/rcar-du.h
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [GIT PULL FOR v3.19] R-Car DU changes 2014-11-07 6:25 [GIT PULL FOR v3.19] R-Car DU changes Laurent Pinchart @ 2014-11-07 9:19 ` Daniel Vetter 2014-11-24 9:46 ` Laurent Pinchart 0 siblings, 1 reply; 11+ messages in thread From: Daniel Vetter @ 2014-11-07 9:19 UTC (permalink / raw) To: Laurent Pinchart; +Cc: Simon Horman, magnus.damm, dri-devel On Fri, Nov 07, 2014 at 08:25:32AM +0200, Laurent Pinchart wrote: > Hi Dave, > > Here's a pull request that adds HDMI support to the R-Car DU driver, including > a new slave encoder driver for the adv7511. > > The branch is based on a merge of drm-next and Simon's tags/renesas-dt-du-for- > v3.19 available at > git://git.kernel.org/pub/scm/linux/kernel/git/horms/renesas.git, for which a > pull request has been submitted for v3.19 to the ARM SoC maintainers. > > I'm leaving now for two weeks of holidays and would like to avoid missing the > merge window. Simon, could you please confirm that tags/renesas-dt-du-for- > v3.19 is stable and ack this pull request (and possibly ping Dave on my behalf > ;-)) ? > > The following changes since commit 19de43d4dea8712b3574d72d3d8aa16cd17e7fed: > > Merge tag 'tags/renesas-dt-du-for-v3.19' into drm/next/adv7511-base > (2014-11-07 08:06:16 +0200) > > are available in the git repository at: > > git://linuxtv.org/pinchartl/fbdev.git > > for you to fetch changes up to cc47927ca851110d2c387a7d9fd7086112abb92e: > > drm: Add adv7511 encoder driver (2014-11-07 08:19:06 +0200) > > ---------------------------------------------------------------- > Lars-Peter Clausen (2): > drm: Decouple EDID parsing from I2C adapter So somehow the pull request seems to lack the branch so I can't actually take a lookc. But this patch here seems to lack review: - kerneldoc for the newly exported function is missing - the interface looks rather backwards: Either this still does i2c reads, and then you'd just need a i2c-over-whatever adapter to make it work. Or you have other magic means to optain an edid block, in which case just do that and then feed the edid drm_add_edid_modes. Nack from me in this form. And can we try not to be quite this sneaky with drm core patches going in through driver trees, please? Thanks, Daniel > drm: Add adv7511 encoder driver > > Laurent Pinchart (6): > drm: rcar-du: Remove platform data support > drm: rcar-du: Pass the encoder DT node to rcar_du_encoder_init() > drm: rcar-du: Replace direct DRM encoder access with cast macro > drm: rcar-du: Replace drm_encoder with drm_slave_encoder > drm: rcar-du: Add HDMI encoder and connector support > video: Add ADV751[13] DT bindings documentation > > Documentation/devicetree/bindings/video/adi,adv7511.txt | 88 ++ > drivers/gpu/drm/drm_edid.c | 27 +- > drivers/gpu/drm/i2c/Kconfig | 6 + > drivers/gpu/drm/i2c/Makefile | 2 + > drivers/gpu/drm/i2c/adv7511.c | 1010 ++++++++++++++ > drivers/gpu/drm/i2c/adv7511.h | 289 +++++++ > drivers/gpu/drm/rcar-du/Kconfig | 11 +- > drivers/gpu/drm/rcar-du/Makefile | 2 + > drivers/gpu/drm/rcar-du/rcar_du_crtc.h | 10 +- > drivers/gpu/drm/rcar-du/rcar_du_drv.c | 4 +- > drivers/gpu/drm/rcar-du/rcar_du_drv.h | 2 - > drivers/gpu/drm/rcar-du/rcar_du_encoder.c | 45 +- > drivers/gpu/drm/rcar-du/rcar_du_encoder.h | 23 +- > drivers/gpu/drm/rcar-du/rcar_du_hdmicon.c | 118 +++ > drivers/gpu/drm/rcar-du/rcar_du_hdmicon.h | 31 + > drivers/gpu/drm/rcar-du/rcar_du_hdmienc.c | 151 ++++ > drivers/gpu/drm/rcar-du/rcar_du_hdmienc.h | 35 + > drivers/gpu/drm/rcar-du/rcar_du_kms.c | 53 +- > drivers/gpu/drm/rcar-du/rcar_du_lvdscon.c | 31 +- > drivers/gpu/drm/rcar-du/rcar_du_lvdscon.h | 2 - > drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.h | 1 - > drivers/gpu/drm/rcar-du/rcar_du_vgacon.c | 5 +- > include/drm/drm_edid.h | 5 + > include/linux/platform_data/rcar-du.h | 74 -- > 24 files changed, 1845 insertions(+), 180 deletions(-) > create mode 100644 Documentation/devicetree/bindings/video/adi,adv7511.txt > create mode 100644 drivers/gpu/drm/i2c/adv7511.c > create mode 100644 drivers/gpu/drm/i2c/adv7511.h > create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_hdmicon.c > create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_hdmicon.h > create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_hdmienc.c > create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_hdmienc.h > delete mode 100644 include/linux/platform_data/rcar-du.h > > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [GIT PULL FOR v3.19] R-Car DU changes 2014-11-07 9:19 ` Daniel Vetter @ 2014-11-24 9:46 ` Laurent Pinchart 2014-11-24 13:09 ` Daniel Vetter 2014-11-25 0:11 ` Simon Horman 0 siblings, 2 replies; 11+ messages in thread From: Laurent Pinchart @ 2014-11-24 9:46 UTC (permalink / raw) To: Daniel Vetter; +Cc: Simon Horman, magnus.damm, dri-devel Hi Daniel, Thank you for the review. On Friday 07 November 2014 10:19:14 Daniel Vetter wrote: > On Fri, Nov 07, 2014 at 08:25:32AM +0200, Laurent Pinchart wrote: > > Hi Dave, > > > > Here's a pull request that adds HDMI support to the R-Car DU driver, > > including a new slave encoder driver for the adv7511. > > > > The branch is based on a merge of drm-next and Simon's > > tags/renesas-dt-du-for- v3.19 available at > > git://git.kernel.org/pub/scm/linux/kernel/git/horms/renesas.git, for which > > a pull request has been submitted for v3.19 to the ARM SoC maintainers. > > > > I'm leaving now for two weeks of holidays and would like to avoid missing > > the merge window. Simon, could you please confirm that > > tags/renesas-dt-du-for- v3.19 is stable and ack this pull request (and > > possibly ping Dave on my behalf ;-)) ? > > > > The following changes since commit 19de43d4dea8712b3574d72d3d8aa16cd17e7fed: > > Merge tag 'tags/renesas-dt-du-for-v3.19' into drm/next/adv7511-base > > > > (2014-11-07 08:06:16 +0200) > > > > are available in the git repository at: > > git://linuxtv.org/pinchartl/fbdev.git > > > > for you to fetch changes up to cc47927ca851110d2c387a7d9fd7086112abb92e: > > drm: Add adv7511 encoder driver (2014-11-07 08:19:06 +0200) > > > > ---------------------------------------------------------------- > > > > Lars-Peter Clausen (2): > > drm: Decouple EDID parsing from I2C adapter > > So somehow the pull request seems to lack the branch so I can't actually > take a lookc. git request-pull is playing trick with me :-/ I'll try to fix that. The branch is available at git://linuxtv.org/pinchartl/fbdev.git drm/next/du (http://git.linuxtv.org/cgit.cgi/pinchartl/fbdev.git/commit/?h=drm/next/du) > But this patch here seems to lack review: > - kerneldoc for the newly exported function is missing I'll fix that. > - the interface looks rather backwards: Either this still does i2c reads, > and then you'd just need a i2c-over-whatever adapter to make it work. Or > you have other magic means to optain an edid block, in which case just > do that and then feed the edid drm_add_edid_modes. I have a magic way to get EDID over I2C :-) Basically the ADV7511 controls the DDC bus, and exposes EDID data over I2C using vendor commands. To read an EDID block I have to write an ADV7511 register over I2C with the block number, wait for an interrupt, read a status register to check whether EDID data is available or whether an error occurred, and then read EDID data from the ADV7511 over I2C in 64-bytes chunks. This needs to be repeated for every block. I thus can't use drm_get_edid() directly. > Nack from me in this form. And can we try not to be quite this sneaky with > drm core patches going in through driver trees, please? > > Thanks, Daniel > > > drm: Add adv7511 encoder driver > > > > Laurent Pinchart (6): > > drm: rcar-du: Remove platform data support > > drm: rcar-du: Pass the encoder DT node to rcar_du_encoder_init() > > drm: rcar-du: Replace direct DRM encoder access with cast macro > > drm: rcar-du: Replace drm_encoder with drm_slave_encoder > > drm: rcar-du: Add HDMI encoder and connector support > > video: Add ADV751[13] DT bindings documentation > > > > Documentation/devicetree/bindings/video/adi,adv7511.txt | 88 ++ > > drivers/gpu/drm/drm_edid.c | 27 +- > > drivers/gpu/drm/i2c/Kconfig | 6 + > > drivers/gpu/drm/i2c/Makefile | 2 + > > drivers/gpu/drm/i2c/adv7511.c | 1010 ++++++++++ > > drivers/gpu/drm/i2c/adv7511.h | 289 +++++++ > > drivers/gpu/drm/rcar-du/Kconfig | 11 +- > > drivers/gpu/drm/rcar-du/Makefile | 2 + > > drivers/gpu/drm/rcar-du/rcar_du_crtc.h | 10 +- > > drivers/gpu/drm/rcar-du/rcar_du_drv.c | 4 +- > > drivers/gpu/drm/rcar-du/rcar_du_drv.h | 2 - > > drivers/gpu/drm/rcar-du/rcar_du_encoder.c | 45 +- > > drivers/gpu/drm/rcar-du/rcar_du_encoder.h | 23 +- > > drivers/gpu/drm/rcar-du/rcar_du_hdmicon.c | 118 +++ > > drivers/gpu/drm/rcar-du/rcar_du_hdmicon.h | 31 + > > drivers/gpu/drm/rcar-du/rcar_du_hdmienc.c | 151 ++++ > > drivers/gpu/drm/rcar-du/rcar_du_hdmienc.h | 35 + > > drivers/gpu/drm/rcar-du/rcar_du_kms.c | 53 +- > > drivers/gpu/drm/rcar-du/rcar_du_lvdscon.c | 31 +- > > drivers/gpu/drm/rcar-du/rcar_du_lvdscon.h | 2 - > > drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.h | 1 - > > drivers/gpu/drm/rcar-du/rcar_du_vgacon.c | 5 +- > > include/drm/drm_edid.h | 5 + > > include/linux/platform_data/rcar-du.h | 74 -- > > 24 files changed, 1845 insertions(+), 180 deletions(-) > > create mode 100644 > > Documentation/devicetree/bindings/video/adi,adv7511.txt > > create mode 100644 drivers/gpu/drm/i2c/adv7511.c > > create mode 100644 drivers/gpu/drm/i2c/adv7511.h > > create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_hdmicon.c > > create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_hdmicon.h > > create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_hdmienc.c > > create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_hdmienc.h > > delete mode 100644 include/linux/platform_data/rcar-du.h -- Regards, Laurent Pinchart _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [GIT PULL FOR v3.19] R-Car DU changes 2014-11-24 9:46 ` Laurent Pinchart @ 2014-11-24 13:09 ` Daniel Vetter 2014-11-24 14:00 ` Laurent Pinchart 2014-11-25 0:11 ` Simon Horman 1 sibling, 1 reply; 11+ messages in thread From: Daniel Vetter @ 2014-11-24 13:09 UTC (permalink / raw) To: Laurent Pinchart; +Cc: Simon Horman, magnus.damm, dri-devel On Mon, Nov 24, 2014 at 11:46:18AM +0200, Laurent Pinchart wrote: > > - the interface looks rather backwards: Either this still does i2c reads, > > and then you'd just need a i2c-over-whatever adapter to make it work. Or > > you have other magic means to optain an edid block, in which case just > > do that and then feed the edid drm_add_edid_modes. > > I have a magic way to get EDID over I2C :-) Basically the ADV7511 controls the > DDC bus, and exposes EDID data over I2C using vendor commands. To read an EDID > block I have to write an ADV7511 register over I2C with the block number, wait > for an interrupt, read a status register to check whether EDID data is > available or whether an error occurred, and then read EDID data from the > ADV7511 over I2C in 64-bytes chunks. This needs to be repeated for every > block. I thus can't use drm_get_edid() directly. Sounds familiar. See the special ddc-over-sdvo i2c bus we register in intel_sdvo.c, specifically look at intel_sdvo_init_ddc_proxy. It is a bit of boilerplate, but in the end just amounts to 3 small functions and one tiny vtable to wire it all up cleanly. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [GIT PULL FOR v3.19] R-Car DU changes 2014-11-24 13:09 ` Daniel Vetter @ 2014-11-24 14:00 ` Laurent Pinchart 2014-11-24 14:18 ` Lars-Peter Clausen 2014-11-24 15:18 ` Daniel Vetter 0 siblings, 2 replies; 11+ messages in thread From: Laurent Pinchart @ 2014-11-24 14:00 UTC (permalink / raw) To: Daniel Vetter; +Cc: Simon Horman, magnus.damm, dri-devel Hi Daniel, (CC'ing Rob Clark and Lars-Peter. As a reminder we're discussing the "drm: Decouple EDID parsing from I2C adapter" patch available at git://linuxtv.org/pinchartl/fbdev.git drm/next/du) On Monday 24 November 2014 14:09:39 Daniel Vetter wrote: > On Mon, Nov 24, 2014 at 11:46:18AM +0200, Laurent Pinchart wrote: > > > - the interface looks rather backwards: Either this still does i2c > > > reads, and then you'd just need a i2c-over-whatever adapter to make it > > > work. Or you have other magic means to optain an edid block, in which > > > case just do that and then feed the edid drm_add_edid_modes. > > > > I have a magic way to get EDID over I2C :-) Basically the ADV7511 controls > > the DDC bus, and exposes EDID data over I2C using vendor commands. To > > read an EDID block I have to write an ADV7511 register over I2C with the > > block number, wait for an interrupt, read a status register to check > > whether EDID data is available or whether an error occurred, and then > > read EDID data from the ADV7511 over I2C in 64-bytes chunks. This needs > > to be repeated for every block. I thus can't use drm_get_edid() directly. > > Sounds familiar. See the special ddc-over-sdvo i2c bus we register in > intel_sdvo.c, specifically look at intel_sdvo_init_ddc_proxy. It is a bit > of boilerplate, but in the end just amounts to 3 small functions and one > tiny vtable to wire it all up cleanly. That's what I would have done as well if I had a device-specific I2C adapter connected to the DDC bus, but in this case the interface exposed by the ADV7511 to the SoC over I2C consists of higher level device-specific I2C commands to read EDID data. There is no low-level I2C read/write primitives available. I would thus need to expose a fake adapter that would receive I2C commands, parse them to detect an EDID block read, retrieve the EDID data and return them from the fake read. That doesn't make much sense to me. -- Regards, Laurent Pinchart _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [GIT PULL FOR v3.19] R-Car DU changes 2014-11-24 14:00 ` Laurent Pinchart @ 2014-11-24 14:18 ` Lars-Peter Clausen 2014-11-24 20:01 ` Dave Airlie 2014-11-24 15:18 ` Daniel Vetter 1 sibling, 1 reply; 11+ messages in thread From: Lars-Peter Clausen @ 2014-11-24 14:18 UTC (permalink / raw) To: Laurent Pinchart, Daniel Vetter; +Cc: Simon Horman, magnus.damm, dri-devel On 11/24/2014 03:00 PM, Laurent Pinchart wrote: > Hi Daniel, > > (CC'ing Rob Clark and Lars-Peter. As a reminder we're discussing the "drm: > Decouple EDID parsing from I2C adapter" patch available at > git://linuxtv.org/pinchartl/fbdev.git drm/next/du) > > On Monday 24 November 2014 14:09:39 Daniel Vetter wrote: >> On Mon, Nov 24, 2014 at 11:46:18AM +0200, Laurent Pinchart wrote: >>>> - the interface looks rather backwards: Either this still does i2c >>>> reads, and then you'd just need a i2c-over-whatever adapter to make it >>>> work. Or you have other magic means to optain an edid block, in which >>>> case just do that and then feed the edid drm_add_edid_modes. >>> >>> I have a magic way to get EDID over I2C :-) Basically the ADV7511 controls >>> the DDC bus, and exposes EDID data over I2C using vendor commands. To >>> read an EDID block I have to write an ADV7511 register over I2C with the >>> block number, wait for an interrupt, read a status register to check >>> whether EDID data is available or whether an error occurred, and then >>> read EDID data from the ADV7511 over I2C in 64-bytes chunks. This needs >>> to be repeated for every block. I thus can't use drm_get_edid() directly. >> >> Sounds familiar. See the special ddc-over-sdvo i2c bus we register in >> intel_sdvo.c, specifically look at intel_sdvo_init_ddc_proxy. It is a bit >> of boilerplate, but in the end just amounts to 3 small functions and one >> tiny vtable to wire it all up cleanly. > > That's what I would have done as well if I had a device-specific I2C adapter > connected to the DDC bus, but in this case the interface exposed by the > ADV7511 to the SoC over I2C consists of higher level device-specific I2C > commands to read EDID data. There is no low-level I2C read/write primitives > available. I would thus need to expose a fake adapter that would receive I2C > commands, parse them to detect an EDID block read, retrieve the EDID data and > return them from the fake read. That doesn't make much sense to me. The intel sdvo looks just like a simple I2C mux which will just pass-through messages from the master to the EDID EEPROM. The ADV7511 is unfortunately a bit different. You tell it to fetch the EDID information, then it will do some magic and then you can read the EDID back. Abstracting this as a this as a I2C controller will, while possible, result in a fair amount of boiler plate code that will not look particularly pretty. - Lars _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [GIT PULL FOR v3.19] R-Car DU changes 2014-11-24 14:18 ` Lars-Peter Clausen @ 2014-11-24 20:01 ` Dave Airlie 2014-11-24 20:29 ` Lars-Peter Clausen 0 siblings, 1 reply; 11+ messages in thread From: Dave Airlie @ 2014-11-24 20:01 UTC (permalink / raw) To: Lars-Peter Clausen; +Cc: dri-devel, Simon Horman, magnus.damm, Laurent Pinchart On 25 November 2014 at 00:18, Lars-Peter Clausen <lars@metafoo.de> wrote: > On 11/24/2014 03:00 PM, Laurent Pinchart wrote: >> >> Hi Daniel, >> >> (CC'ing Rob Clark and Lars-Peter. As a reminder we're discussing the "drm: >> Decouple EDID parsing from I2C adapter" patch available at >> git://linuxtv.org/pinchartl/fbdev.git drm/next/du) >> >> On Monday 24 November 2014 14:09:39 Daniel Vetter wrote: >>> >>> On Mon, Nov 24, 2014 at 11:46:18AM +0200, Laurent Pinchart wrote: >>>>> >>>>> - the interface looks rather backwards: Either this still does i2c >>>>> reads, and then you'd just need a i2c-over-whatever adapter to make >>>>> it >>>>> work. Or you have other magic means to optain an edid block, in >>>>> which >>>>> case just do that and then feed the edid drm_add_edid_modes. >>>> >>>> >>>> I have a magic way to get EDID over I2C :-) Basically the ADV7511 >>>> controls >>>> the DDC bus, and exposes EDID data over I2C using vendor commands. To >>>> read an EDID block I have to write an ADV7511 register over I2C with the >>>> block number, wait for an interrupt, read a status register to check >>>> whether EDID data is available or whether an error occurred, and then >>>> read EDID data from the ADV7511 over I2C in 64-bytes chunks. This needs >>>> to be repeated for every block. I thus can't use drm_get_edid() >>>> directly. >>> >>> >>> Sounds familiar. See the special ddc-over-sdvo i2c bus we register in >>> intel_sdvo.c, specifically look at intel_sdvo_init_ddc_proxy. It is a bit >>> of boilerplate, but in the end just amounts to 3 small functions and one >>> tiny vtable to wire it all up cleanly. >> >> >> That's what I would have done as well if I had a device-specific I2C >> adapter >> connected to the DDC bus, but in this case the interface exposed by the >> ADV7511 to the SoC over I2C consists of higher level device-specific I2C >> commands to read EDID data. There is no low-level I2C read/write >> primitives >> available. I would thus need to expose a fake adapter that would receive >> I2C >> commands, parse them to detect an EDID block read, retrieve the EDID data >> and >> return them from the fake read. That doesn't make much sense to me. > > > The intel sdvo looks just like a simple I2C mux which will just pass-through > messages from the master to the EDID EEPROM. The ADV7511 is unfortunately a > bit different. You tell it to fetch the EDID information, then it will do > some magic and then you can read the EDID back. Abstracting this as a this > as a I2C controller will, while possible, result in a fair amount of boiler > plate code that will not look particularly pretty. It sounds also a bit like DP auxch also, or even how on UDL we get the edid over USB. I'd rather see not pretty code that only one person had to look at though :-) with lots of comments on the hw design that demands ugly. Dave. _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [GIT PULL FOR v3.19] R-Car DU changes 2014-11-24 20:01 ` Dave Airlie @ 2014-11-24 20:29 ` Lars-Peter Clausen 2014-11-24 21:35 ` Rob Clark 0 siblings, 1 reply; 11+ messages in thread From: Lars-Peter Clausen @ 2014-11-24 20:29 UTC (permalink / raw) To: Dave Airlie; +Cc: dri-devel, Simon Horman, magnus.damm, Laurent Pinchart On 11/24/2014 09:01 PM, Dave Airlie wrote: > On 25 November 2014 at 00:18, Lars-Peter Clausen <lars@metafoo.de> wrote: >> On 11/24/2014 03:00 PM, Laurent Pinchart wrote: >>> >>> Hi Daniel, >>> >>> (CC'ing Rob Clark and Lars-Peter. As a reminder we're discussing the "drm: >>> Decouple EDID parsing from I2C adapter" patch available at >>> git://linuxtv.org/pinchartl/fbdev.git drm/next/du) >>> >>> On Monday 24 November 2014 14:09:39 Daniel Vetter wrote: >>>> >>>> On Mon, Nov 24, 2014 at 11:46:18AM +0200, Laurent Pinchart wrote: >>>>>> >>>>>> - the interface looks rather backwards: Either this still does i2c >>>>>> reads, and then you'd just need a i2c-over-whatever adapter to make >>>>>> it >>>>>> work. Or you have other magic means to optain an edid block, in >>>>>> which >>>>>> case just do that and then feed the edid drm_add_edid_modes. >>>>> >>>>> >>>>> I have a magic way to get EDID over I2C :-) Basically the ADV7511 >>>>> controls >>>>> the DDC bus, and exposes EDID data over I2C using vendor commands. To >>>>> read an EDID block I have to write an ADV7511 register over I2C with the >>>>> block number, wait for an interrupt, read a status register to check >>>>> whether EDID data is available or whether an error occurred, and then >>>>> read EDID data from the ADV7511 over I2C in 64-bytes chunks. This needs >>>>> to be repeated for every block. I thus can't use drm_get_edid() >>>>> directly. >>>> >>>> >>>> Sounds familiar. See the special ddc-over-sdvo i2c bus we register in >>>> intel_sdvo.c, specifically look at intel_sdvo_init_ddc_proxy. It is a bit >>>> of boilerplate, but in the end just amounts to 3 small functions and one >>>> tiny vtable to wire it all up cleanly. >>> >>> >>> That's what I would have done as well if I had a device-specific I2C >>> adapter >>> connected to the DDC bus, but in this case the interface exposed by the >>> ADV7511 to the SoC over I2C consists of higher level device-specific I2C >>> commands to read EDID data. There is no low-level I2C read/write >>> primitives >>> available. I would thus need to expose a fake adapter that would receive >>> I2C >>> commands, parse them to detect an EDID block read, retrieve the EDID data >>> and >>> return them from the fake read. That doesn't make much sense to me. >> >> >> The intel sdvo looks just like a simple I2C mux which will just pass-through >> messages from the master to the EDID EEPROM. The ADV7511 is unfortunately a >> bit different. You tell it to fetch the EDID information, then it will do >> some magic and then you can read the EDID back. Abstracting this as a this >> as a I2C controller will, while possible, result in a fair amount of boiler >> plate code that will not look particularly pretty. > > It sounds also a bit like DP auxch also, or even how on UDL we get the edid > over USB. > > I'd rather see not pretty code that only one person had to look at though :-) > with lots of comments on the hw design that demands ugly. I'd rather not see that if I'm the person who has to look at it ;) But looking around it appears as if this is a more common thing among external HDMI encoders though. E.g. the tda998x has the exact same issue and the current driver just copy&pastes drm_do_get_edid() and plugs in its own EDID block retrieval method. On the other hand the patch that makes it possible to use a different backend then a raw I2C adapter to fetch the EDID is extremely trivial and doesn't make the existing code more complex in any way. _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [GIT PULL FOR v3.19] R-Car DU changes 2014-11-24 20:29 ` Lars-Peter Clausen @ 2014-11-24 21:35 ` Rob Clark 0 siblings, 0 replies; 11+ messages in thread From: Rob Clark @ 2014-11-24 21:35 UTC (permalink / raw) To: Lars-Peter Clausen; +Cc: Magnus Damm, Simon Horman, Laurent Pinchart, dri-devel On Mon, Nov 24, 2014 at 3:29 PM, Lars-Peter Clausen <lars@metafoo.de> wrote: > On 11/24/2014 09:01 PM, Dave Airlie wrote: >> >> On 25 November 2014 at 00:18, Lars-Peter Clausen <lars@metafoo.de> wrote: >>> >>> On 11/24/2014 03:00 PM, Laurent Pinchart wrote: >>>> >>>> >>>> Hi Daniel, >>>> >>>> (CC'ing Rob Clark and Lars-Peter. As a reminder we're discussing the >>>> "drm: >>>> Decouple EDID parsing from I2C adapter" patch available at >>>> git://linuxtv.org/pinchartl/fbdev.git drm/next/du) >>>> >>>> On Monday 24 November 2014 14:09:39 Daniel Vetter wrote: >>>>> >>>>> >>>>> On Mon, Nov 24, 2014 at 11:46:18AM +0200, Laurent Pinchart wrote: >>>>>>> >>>>>>> >>>>>>> - the interface looks rather backwards: Either this still does i2c >>>>>>> reads, and then you'd just need a i2c-over-whatever adapter to >>>>>>> make >>>>>>> it >>>>>>> work. Or you have other magic means to optain an edid block, in >>>>>>> which >>>>>>> case just do that and then feed the edid drm_add_edid_modes. >>>>>> >>>>>> >>>>>> >>>>>> I have a magic way to get EDID over I2C :-) Basically the ADV7511 >>>>>> controls >>>>>> the DDC bus, and exposes EDID data over I2C using vendor commands. To >>>>>> read an EDID block I have to write an ADV7511 register over I2C with >>>>>> the >>>>>> block number, wait for an interrupt, read a status register to check >>>>>> whether EDID data is available or whether an error occurred, and then >>>>>> read EDID data from the ADV7511 over I2C in 64-bytes chunks. This >>>>>> needs >>>>>> to be repeated for every block. I thus can't use drm_get_edid() >>>>>> directly. >>>>> >>>>> >>>>> >>>>> Sounds familiar. See the special ddc-over-sdvo i2c bus we register in >>>>> intel_sdvo.c, specifically look at intel_sdvo_init_ddc_proxy. It is a >>>>> bit >>>>> of boilerplate, but in the end just amounts to 3 small functions and >>>>> one >>>>> tiny vtable to wire it all up cleanly. >>>> >>>> >>>> >>>> That's what I would have done as well if I had a device-specific I2C >>>> adapter >>>> connected to the DDC bus, but in this case the interface exposed by the >>>> ADV7511 to the SoC over I2C consists of higher level device-specific I2C >>>> commands to read EDID data. There is no low-level I2C read/write >>>> primitives >>>> available. I would thus need to expose a fake adapter that would receive >>>> I2C >>>> commands, parse them to detect an EDID block read, retrieve the EDID >>>> data >>>> and >>>> return them from the fake read. That doesn't make much sense to me. >>> >>> >>> >>> The intel sdvo looks just like a simple I2C mux which will just >>> pass-through >>> messages from the master to the EDID EEPROM. The ADV7511 is unfortunately >>> a >>> bit different. You tell it to fetch the EDID information, then it will do >>> some magic and then you can read the EDID back. Abstracting this as a >>> this >>> as a I2C controller will, while possible, result in a fair amount of >>> boiler >>> plate code that will not look particularly pretty. >> >> >> It sounds also a bit like DP auxch also, or even how on UDL we get the >> edid >> over USB. >> >> I'd rather see not pretty code that only one person had to look at though >> :-) >> with lots of comments on the hw design that demands ugly. > > > I'd rather not see that if I'm the person who has to look at it ;) > > But looking around it appears as if this is a more common thing among > external HDMI encoders though. E.g. the tda998x has the exact same issue and > the current driver just copy&pastes drm_do_get_edid() and plugs in its own > EDID block retrieval method. > > On the other hand the patch that makes it possible to use a different > backend then a raw I2C adapter to fetch the EDID is extremely trivial and > doesn't make the existing code more complex in any way. > I suppose the only real disadvantage of the patch is it gives driver writers a bad alternative in cases where they really should expose their ddc as an i2c_adapter.. but perhaps we should just handle that via sufficient review ;-) (I have seen enough downstream drivers that could use i2c_adapter but do not..) BR, -R > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [GIT PULL FOR v3.19] R-Car DU changes 2014-11-24 14:00 ` Laurent Pinchart 2014-11-24 14:18 ` Lars-Peter Clausen @ 2014-11-24 15:18 ` Daniel Vetter 1 sibling, 0 replies; 11+ messages in thread From: Daniel Vetter @ 2014-11-24 15:18 UTC (permalink / raw) To: Laurent Pinchart; +Cc: magnus.damm, dri-devel, Simon Horman On Mon, Nov 24, 2014 at 04:00:53PM +0200, Laurent Pinchart wrote: > Hi Daniel, > > (CC'ing Rob Clark and Lars-Peter. As a reminder we're discussing the "drm: > Decouple EDID parsing from I2C adapter" patch available at > git://linuxtv.org/pinchartl/fbdev.git drm/next/du) > > On Monday 24 November 2014 14:09:39 Daniel Vetter wrote: > > On Mon, Nov 24, 2014 at 11:46:18AM +0200, Laurent Pinchart wrote: > > > > - the interface looks rather backwards: Either this still does i2c > > > > reads, and then you'd just need a i2c-over-whatever adapter to make it > > > > work. Or you have other magic means to optain an edid block, in which > > > > case just do that and then feed the edid drm_add_edid_modes. > > > > > > I have a magic way to get EDID over I2C :-) Basically the ADV7511 controls > > > the DDC bus, and exposes EDID data over I2C using vendor commands. To > > > read an EDID block I have to write an ADV7511 register over I2C with the > > > block number, wait for an interrupt, read a status register to check > > > whether EDID data is available or whether an error occurred, and then > > > read EDID data from the ADV7511 over I2C in 64-bytes chunks. This needs > > > to be repeated for every block. I thus can't use drm_get_edid() directly. > > > > Sounds familiar. See the special ddc-over-sdvo i2c bus we register in > > intel_sdvo.c, specifically look at intel_sdvo_init_ddc_proxy. It is a bit > > of boilerplate, but in the end just amounts to 3 small functions and one > > tiny vtable to wire it all up cleanly. > > That's what I would have done as well if I had a device-specific I2C adapter > connected to the DDC bus, but in this case the interface exposed by the > ADV7511 to the SoC over I2C consists of higher level device-specific I2C > commands to read EDID data. There is no low-level I2C read/write primitives > available. I would thus need to expose a fake adapter that would receive I2C > commands, parse them to detect an EDID block read, retrieve the EDID data and > return them from the fake read. That doesn't make much sense to me. Yeah that's a bit annoying indeed. So if you capture that in the commit message + kerneldoc (stating clearly what the other options are and when to use this) the I'm ok with this patch. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [GIT PULL FOR v3.19] R-Car DU changes 2014-11-24 9:46 ` Laurent Pinchart 2014-11-24 13:09 ` Daniel Vetter @ 2014-11-25 0:11 ` Simon Horman 1 sibling, 0 replies; 11+ messages in thread From: Simon Horman @ 2014-11-25 0:11 UTC (permalink / raw) To: Laurent Pinchart; +Cc: magnus.damm, dri-devel Hi Laurent, On Mon, Nov 24, 2014 at 11:46:18AM +0200, Laurent Pinchart wrote: > Hi Daniel, > > Thank you for the review. > > On Friday 07 November 2014 10:19:14 Daniel Vetter wrote: > > On Fri, Nov 07, 2014 at 08:25:32AM +0200, Laurent Pinchart wrote: > > > Hi Dave, > > > > > > Here's a pull request that adds HDMI support to the R-Car DU driver, > > > including a new slave encoder driver for the adv7511. > > > > > > The branch is based on a merge of drm-next and Simon's > > > tags/renesas-dt-du-for- v3.19 available at > > > git://git.kernel.org/pub/scm/linux/kernel/git/horms/renesas.git, for which > > > a pull request has been submitted for v3.19 to the ARM SoC maintainers. > > > > > > I'm leaving now for two weeks of holidays and would like to avoid missing > > > the merge window. Simon, could you please confirm that > > > tags/renesas-dt-du-for- v3.19 is stable and ack this pull request (and > > > possibly ping Dave on my behalf ;-)) ? Welcome back from vacation :) FYI renesas-dt-du-for was accepted by Arnd last week so it is rather unlikely it will change now: it should be a stable base. > > > > > > The following changes since commit > 19de43d4dea8712b3574d72d3d8aa16cd17e7fed: > > > Merge tag 'tags/renesas-dt-du-for-v3.19' into drm/next/adv7511-base > > > > > > (2014-11-07 08:06:16 +0200) > > > > > > are available in the git repository at: > > > git://linuxtv.org/pinchartl/fbdev.git > > > > > > for you to fetch changes up to cc47927ca851110d2c387a7d9fd7086112abb92e: > > > drm: Add adv7511 encoder driver (2014-11-07 08:19:06 +0200) > > > > > > ---------------------------------------------------------------- > > > > > > Lars-Peter Clausen (2): > > > drm: Decouple EDID parsing from I2C adapter > > > > So somehow the pull request seems to lack the branch so I can't actually > > take a lookc. > > git request-pull is playing trick with me :-/ I'll try to fix that. > > The branch is available at > > git://linuxtv.org/pinchartl/fbdev.git drm/next/du > > (http://git.linuxtv.org/cgit.cgi/pinchartl/fbdev.git/commit/?h=drm/next/du) > > > But this patch here seems to lack review: > > - kerneldoc for the newly exported function is missing > > I'll fix that. > > > - the interface looks rather backwards: Either this still does i2c reads, > > and then you'd just need a i2c-over-whatever adapter to make it work. Or > > you have other magic means to optain an edid block, in which case just > > do that and then feed the edid drm_add_edid_modes. > > I have a magic way to get EDID over I2C :-) Basically the ADV7511 controls the > DDC bus, and exposes EDID data over I2C using vendor commands. To read an EDID > block I have to write an ADV7511 register over I2C with the block number, wait > for an interrupt, read a status register to check whether EDID data is > available or whether an error occurred, and then read EDID data from the > ADV7511 over I2C in 64-bytes chunks. This needs to be repeated for every > block. I thus can't use drm_get_edid() directly. > > > Nack from me in this form. And can we try not to be quite this sneaky with > > drm core patches going in through driver trees, please? > > > > Thanks, Daniel > > > > > drm: Add adv7511 encoder driver > > > > > > Laurent Pinchart (6): > > > drm: rcar-du: Remove platform data support > > > drm: rcar-du: Pass the encoder DT node to rcar_du_encoder_init() > > > drm: rcar-du: Replace direct DRM encoder access with cast macro > > > drm: rcar-du: Replace drm_encoder with drm_slave_encoder > > > drm: rcar-du: Add HDMI encoder and connector support > > > video: Add ADV751[13] DT bindings documentation > > > > > > Documentation/devicetree/bindings/video/adi,adv7511.txt | 88 ++ > > > drivers/gpu/drm/drm_edid.c | 27 +- > > > drivers/gpu/drm/i2c/Kconfig | 6 + > > > drivers/gpu/drm/i2c/Makefile | 2 + > > > drivers/gpu/drm/i2c/adv7511.c | 1010 ++++++++++ > > > drivers/gpu/drm/i2c/adv7511.h | 289 +++++++ > > > drivers/gpu/drm/rcar-du/Kconfig | 11 +- > > > drivers/gpu/drm/rcar-du/Makefile | 2 + > > > drivers/gpu/drm/rcar-du/rcar_du_crtc.h | 10 +- > > > drivers/gpu/drm/rcar-du/rcar_du_drv.c | 4 +- > > > drivers/gpu/drm/rcar-du/rcar_du_drv.h | 2 - > > > drivers/gpu/drm/rcar-du/rcar_du_encoder.c | 45 +- > > > drivers/gpu/drm/rcar-du/rcar_du_encoder.h | 23 +- > > > drivers/gpu/drm/rcar-du/rcar_du_hdmicon.c | 118 +++ > > > drivers/gpu/drm/rcar-du/rcar_du_hdmicon.h | 31 + > > > drivers/gpu/drm/rcar-du/rcar_du_hdmienc.c | 151 ++++ > > > drivers/gpu/drm/rcar-du/rcar_du_hdmienc.h | 35 + > > > drivers/gpu/drm/rcar-du/rcar_du_kms.c | 53 +- > > > drivers/gpu/drm/rcar-du/rcar_du_lvdscon.c | 31 +- > > > drivers/gpu/drm/rcar-du/rcar_du_lvdscon.h | 2 - > > > drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.h | 1 - > > > drivers/gpu/drm/rcar-du/rcar_du_vgacon.c | 5 +- > > > include/drm/drm_edid.h | 5 + > > > include/linux/platform_data/rcar-du.h | 74 -- > > > 24 files changed, 1845 insertions(+), 180 deletions(-) > > > create mode 100644 > > > Documentation/devicetree/bindings/video/adi,adv7511.txt > > > create mode 100644 drivers/gpu/drm/i2c/adv7511.c > > > create mode 100644 drivers/gpu/drm/i2c/adv7511.h > > > create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_hdmicon.c > > > create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_hdmicon.h > > > create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_hdmienc.c > > > create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_hdmienc.h > > > delete mode 100644 include/linux/platform_data/rcar-du.h > > -- > Regards, > > Laurent Pinchart > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2014-11-25 0:11 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-11-07 6:25 [GIT PULL FOR v3.19] R-Car DU changes Laurent Pinchart 2014-11-07 9:19 ` Daniel Vetter 2014-11-24 9:46 ` Laurent Pinchart 2014-11-24 13:09 ` Daniel Vetter 2014-11-24 14:00 ` Laurent Pinchart 2014-11-24 14:18 ` Lars-Peter Clausen 2014-11-24 20:01 ` Dave Airlie 2014-11-24 20:29 ` Lars-Peter Clausen 2014-11-24 21:35 ` Rob Clark 2014-11-24 15:18 ` Daniel Vetter 2014-11-25 0:11 ` Simon Horman
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.