| Message ID | 20180812184152.GA22343@ravnborg.org |
|---|---|
| Headers | show |
| Series | add at91sam9 LCDC DRM driver | expand |
The at91sam9263 has a few interesting "GPU" features: - 2D memory addressing Data sheet says: The LCDC can be configured to work on a frame buffer larger than the actual screen size. By changing the values in a few registers, it is easy to move the displayed area along the frame buffer width and height - 2D Graphics controller Data sheet says: The Two D Graphics Controller (TDGC) features a hardware accelerator which highly simplifies drawing tasks and graphic management operations. The hardware accelerator makes it easy to draw lines and complex polygons and to perform block transfers within the frame buffer. The TDGC also features a draw command queue that automatically executes a more complex drawing function that is composed of several register accesses. The datasheet text is from: chapter 43.9 + chapter 44: http://ww1.microchip.com/downloads/en/DeviceDoc/Atmel-6249-32-bit-ARM926EJ-S-Embedded-Microprocessor-SAM9263_Datasheet.pdf (No NDA required) Based on the above, would it be possible to utilise some of these features without any dedicated userspace (mesa) support? Any other driver that has something similar that can be used for inspiration? Or is this in reality a simple GPU that requires a dedicated GPU driver? Sam
On 12/08/2018 at 21:55, Sam Ravnborg wrote: > The at91sam9263 has a few interesting "GPU" features: > > - 2D memory addressing > Data sheet says: > The LCDC can be configured to work on a frame buffer > larger than the actual screen size. By changing the > values in a few registers, it is easy to move the > displayed area along the frame buffer width and height > > - 2D Graphics controller > Data sheet says: > The Two D Graphics Controller (TDGC) features a > hardware accelerator which highly simplifies drawing > tasks and graphic management operations. > The hardware accelerator makes it easy to draw lines > and complex polygons and to perform block transfers > within the frame buffer. > The TDGC also features a draw command queue that > automatically executes a more complex drawing function > that is composed of several register accesses. > > The datasheet text is from: chapter 43.9 + chapter 44: > http://ww1.microchip.com/downloads/en/DeviceDoc/Atmel-6249-32-bit-ARM926EJ-S-Embedded-Microprocessor-SAM9263_Datasheet.pdf > (No NDA required) (old memories) this 2D engine prove itself of being quite limited as the fill polygon and clipping functions are not working. > Based on the above, would it be possible to utilise some > of these features without any dedicated userspace (mesa) support? > Any other driver that has something similar that can be used > for inspiration? This is really an interesting question indeed. > Or is this in reality a simple GPU that requires a > dedicated GPU driver? > > Sam >
On 12/08/2018 at 20:41, Sam Ravnborg wrote: > New DRM based driver for at91sam9 SOC's that uses the > Atmel LCDC IP core. I'm delighted to see this work: Thanks a lot Sam! > This is first version of a patch set that adds > drivers for the Atmel LCDC IP core. > Posted for review as the basics works now. > > The LCDC IP core contains two devices: > - a PWM often used for backlight > - a LCD display controller > > Both devices are supported today by the atmel_lcdfb driver. > For this new set of drivers the compatible strings was > selected to avoid clash with the existing compatible > strings used for the atmel_lcdfb driver to allow them > to co-exist. Would be good to have a plan to phase-out the old atmel_lcdfb fbdev driver when this one addresses some TODO items that make sense. The mfd suffix seems strange to me. What about "atmel,at91sam9263-lcdc-mfd" => "atmel,at91sam9263-lcd" (or "microchip,at91sam9263-lcdc"). > This patchset implements three drivers. > - A MFD driver that include the generic parts. > - A PWM driver. > - A DRM display controller driver. > This is the same split as used for the Atmel hlcdc IP. Good, this is why I would use the same type of compatibility string for the split mfd/pwm/lcd (just without the "h"). > The hlcdc and lcdc has only a few things in common and > trying to share the code for them was not a viable solution. Yes, I agree. > The DRM implementation has a few shortcomings compared to the > existing fbdev based driver: > - STN displays are not supported > Binding support is missing but most of the > STN specific functionality is otherwise ported > from the fbdev driver. > I assume the info should come from the panel > but as I lack HW I have not looked too much > into what is required. Yes, I'm not even sure if STN displays are still available those days... Might not be worth it spending time on this. > - gamma support is missing > The driver utilises drm_simple_kms_helper and > this helper lacks support for setting up gamma. > If this is useful please let me know and I > will extend drm_simple_kms_helper to support this > and update the driver. > - modesetting is not checked (see TODO in file) > Is this required for such a simple setup? > - support for extra modes as applicable (and lcd-wiring-mode) > - support for AVR32 (is it relevant?) No, AVR32 is not relevant anymore as the core and SoC were removed some years ago from Linux. All mention of AT32 or AVR32 can be remove then. > The first patch renames .../drm/atmel-hlcdc to .../drm/atmel > to have a nice home for both drivers. > > The drivers _works_ on a proprietary at91sam9263 based board > and I will eventually migrate the at91sam9263ek over > to use it as well. We'll be able to test it on other SoCs and boards. > Works is maybe a stretch - the following was tested: > modetest shows reasonable output > modetest -s shows some nice colored squares You must see something like this (without the overlay additional black square): http://www.at91.com/linux4sam/bin/view/Linux4SAM/UsingAtmelDRMDriver#Some_modetest_commands > vbltest shows 45.70 Hz (after terminating modetest -s with SIGINT) > drmdevice shows reasonable output > brightness can be controlled > > Anything else that can be recommended for some basic tests? > How to test suspend/resume? > > REVIEW > Please give feedback on general structure/architecture > Please check if the drm framework is used in the optimal way > And then the usual review from spelling errors, names, style etc. > > All feedback (from spelling errors to general structure) appreciated! On my side, it's mostly Nitpicking ;-) Now that we're Microchip for a little bit more than 2 years, I tend to make this name prevalent compared to Atmel for new work around our products... But I know this driver is for older SoCs and that it got inspired by two former drivers that have this "atmel" naming everywhere. MAINTAINERS and Kconfig changes could include Microchip name, so that it's obvious for the newcomers... > Sam > > Sam Ravnborg (7): > atmel-hlcdc: renamed directory to drm/atmel/ > dt-binding: add bindings for Atmel LCDC mfd > mfd: add atmel-lcdc driver > dt-bindings: add bindings for Atmel LCDC pwm > pwm: add pwm-atmel-lcdc driver > dt-bindings: add bindings for Atmel lcdc-display-controller > drm: add Atmel LCDC display controller support > > .../display/atmel/lcdc-display-controller.txt | 40 + > .../devicetree/bindings/mfd/atmel-lcdc.txt | 75 ++ > .../devicetree/bindings/pwm/atmel-lcdc-pwm.txt | 30 + > MAINTAINERS | 9 +- > drivers/gpu/drm/Kconfig | 2 +- > drivers/gpu/drm/Makefile | 2 +- > drivers/gpu/drm/atmel-hlcdc/Kconfig | 10 - > drivers/gpu/drm/atmel/Kconfig | 28 + > drivers/gpu/drm/{atmel-hlcdc => atmel}/Makefile | 2 + > .../drm/{atmel-hlcdc => atmel}/atmel_hlcdc_crtc.c | 0 > .../drm/{atmel-hlcdc => atmel}/atmel_hlcdc_dc.c | 0 > .../drm/{atmel-hlcdc => atmel}/atmel_hlcdc_dc.h | 0 > .../{atmel-hlcdc => atmel}/atmel_hlcdc_output.c | 0 > .../drm/{atmel-hlcdc => atmel}/atmel_hlcdc_plane.c | 0 > drivers/gpu/drm/atmel/atmel_lcdc-dc.c | 1094 ++++++++++++++++++++ > drivers/mfd/Kconfig | 10 + > drivers/mfd/Makefile | 1 + > drivers/mfd/atmel-lcdc.c | 158 +++ > drivers/pwm/Kconfig | 13 + > drivers/pwm/Makefile | 1 + > drivers/pwm/pwm-atmel-lcdc.c | 178 ++++ > include/linux/mfd/atmel-lcdc.h | 184 ++++ > 22 files changed, 1824 insertions(+), 13 deletions(-) >
Hi Nicholas. On Mon, Aug 13, 2018 at 05:54:48PM +0200, Nicolas Ferre wrote: > On 12/08/2018 at 20:41, Sam Ravnborg wrote: > >New DRM based driver for at91sam9 SOC's that uses the > >Atmel LCDC IP core. > > I'm delighted to see this work: Thanks a lot Sam! Glad to hear. I was a bit worried that the response would be "this is waste of time as we have a working driver already". > > >This is first version of a patch set that adds > >drivers for the Atmel LCDC IP core. > >Posted for review as the basics works now. > > > >The LCDC IP core contains two devices: > >- a PWM often used for backlight > >- a LCD display controller > > > >Both devices are supported today by the atmel_lcdfb driver. > >For this new set of drivers the compatible strings was > >selected to avoid clash with the existing compatible > >strings used for the atmel_lcdfb driver to allow them > >to co-exist. > > Would be good to have a plan to phase-out the old atmel_lcdfb fbdev > driver when this one addresses some TODO items that make sense. Agree on this. One approach could be to say that when all in-kernel users of atmel_lcdfb are ported, then the old driver could be dropped after a kernel release. > The mfd suffix seems strange to me. What about "atmel,at91sam9263-lcdc-mfd" > => "atmel,at91sam9263-lcd" (or "microchip,at91sam9263-lcdc"). The "-mfd" suffix was added to avoid clashing with the current compatible string used by the atmel_lcdfb driver. I susggest we do the following: - use the microchip prefix, as this is now owned by microchip - and add the driver to a drm/microchip/ directory (Then we can only hope that microchip do not change name or are purchased by someone else). > > >The DRM implementation has a few shortcomings compared to the > >existing fbdev based driver: > > - STN displays are not supported > > Binding support is missing but most of the > > STN specific functionality is otherwise ported > > from the fbdev driver. > > I assume the info should come from the panel > > but as I lack HW I have not looked too much > > into what is required. > > Yes, I'm not even sure if STN displays are still available those > days... Might not be worth it spending time on this. Then I will delete all the STN bits that was ported over. If we need them later they can be found on the mailign list. > > > - gamma support is missing > > The driver utilises drm_simple_kms_helper and > > this helper lacks support for setting up gamma. > > If this is useful please let me know and I > > will extend drm_simple_kms_helper to support this > > and update the driver. > > - modesetting is not checked (see TODO in file) > > Is this required for such a simple setup? > > - support for extra modes as applicable (and lcd-wiring-mode) > > - support for AVR32 (is it relevant?) > > No, AVR32 is not relevant anymore as the core and SoC were removed > some years ago from Linux. > All mention of AT32 or AVR32 can be remove then. Ok, I will delete these. > >The drivers _works_ on a proprietary at91sam9263 based board > >and I will eventually migrate the at91sam9263ek over > >to use it as well. > > We'll be able to test it on other SoCs and boards. Thanks! > > >Works is maybe a stretch - the following was tested: > > modetest shows reasonable output > > modetest -s shows some nice colored squares > > You must see something like this (without the overlay additional > black square): > http://www.at91.com/linux4sam/bin/view/Linux4SAM/UsingAtmelDRMDriver#Some_modetest_commands I posted a picture to G+ here: https://plus.google.com/+SamRavnborg/posts/YBt4jUGLgWa They look similar but are not equal. For now I assume this is OK. I will do some testing with a Qt app, and will test colors with this too. > >All feedback (from spelling errors to general structure) appreciated! > > On my side, it's mostly Nitpicking ;-) > Now that we're Microchip for a little bit more than 2 years, I tend > to make this name prevalent compared to Atmel for new work around > our products... But I know this driver is for older SoCs and that it > got inspired by two former drivers that have this "atmel" naming > everywhere. MAINTAINERS and Kconfig changes could include Microchip > name, so that it's obvious for the newcomers... Will fix, as described above. Thanks for the inputs! Sam
On Mon, Aug 13, 2018 at 08:18:08PM +0200, Sam Ravnborg wrote: > Hi Nicholas. > > On Mon, Aug 13, 2018 at 05:54:48PM +0200, Nicolas Ferre wrote: > > On 12/08/2018 at 20:41, Sam Ravnborg wrote: > > >New DRM based driver for at91sam9 SOC's that uses the > > >Atmel LCDC IP core. > > > > I'm delighted to see this work: Thanks a lot Sam! > Glad to hear. > I was a bit worried that the response would be "this is > waste of time as we have a working driver already". > > > > > >This is first version of a patch set that adds > > >drivers for the Atmel LCDC IP core. > > >Posted for review as the basics works now. > > > > > >The LCDC IP core contains two devices: > > >- a PWM often used for backlight > > >- a LCD display controller > > > > > >Both devices are supported today by the atmel_lcdfb driver. > > >For this new set of drivers the compatible strings was > > >selected to avoid clash with the existing compatible > > >strings used for the atmel_lcdfb driver to allow them > > >to co-exist. > > > > Would be good to have a plan to phase-out the old atmel_lcdfb fbdev > > driver when this one addresses some TODO items that make sense. > Agree on this. > One approach could be to say that when all in-kernel users of atmel_lcdfb > are ported, then the old driver could be dropped after a kernel release. > > > The mfd suffix seems strange to me. What about "atmel,at91sam9263-lcdc-mfd" > > => "atmel,at91sam9263-lcd" (or "microchip,at91sam9263-lcdc"). > The "-mfd" suffix was added to avoid clashing with the current > compatible string used by the atmel_lcdfb driver. I don't know that 2 registers for a backlight PWM constitute an MFD. A single node can be both an LCD controller and a PWM. The fact that the OS has 2 drivers is irrelevant to the binding and DT is not a way to select drivers. Your choice with Linux is either kconfig or manual module loading. > I susggest we do the following: > - use the microchip prefix, as this is now owned by microchip > - and add the driver to a drm/microchip/ directory > (Then we can only hope that microchip do not change name or > are purchased by someone else). I would not do that. Then you have a dts with a mixture based on when you got around to writing each binding. i.MX has continued using 'fsl' even on new chips today. Probably a good choice had the QCom acq had gone thru. Rob
On Mon, Aug 13, 2018 at 04:47:26PM +0200, Nicolas Ferre wrote: > On 12/08/2018 at 21:55, Sam Ravnborg wrote: > > The at91sam9263 has a few interesting "GPU" features: > > > > - 2D memory addressing > > Data sheet says: > > The LCDC can be configured to work on a frame buffer > > larger than the actual screen size. By changing the > > values in a few registers, it is easy to move the > > displayed area along the frame buffer width and height > > > > - 2D Graphics controller > > Data sheet says: > > The Two D Graphics Controller (TDGC) features a > > hardware accelerator which highly simplifies drawing > > tasks and graphic management operations. > > The hardware accelerator makes it easy to draw lines > > and complex polygons and to perform block transfers > > within the frame buffer. > > The TDGC also features a draw command queue that > > automatically executes a more complex drawing function > > that is composed of several register accesses. > > > > The datasheet text is from: chapter 43.9 + chapter 44: > > http://ww1.microchip.com/downloads/en/DeviceDoc/Atmel-6249-32-bit-ARM926EJ-S-Embedded-Microprocessor-SAM9263_Datasheet.pdf > > (No NDA required) > > (old memories) this 2D engine prove itself of being quite limited as the > fill polygon and clipping functions are not working. > > > Based on the above, would it be possible to utilise some > > of these features without any dedicated userspace (mesa) support? > > Any other driver that has something similar that can be used > > for inspiration? > > This is really an interesting question indeed. Needs userspace like everything else. There's unfortunately no real standard for 2d apis in userspace, which means none of these efforts go very far. Mostly it's just some custom-made X drivers. Adding a generic 2d api to drm is a FAQ, and the answer is "no". -Daniel > > > Or is this in reality a simple GPU that requires a > > dedicated GPU driver? > > > > Sam > > > > > -- > Nicolas Ferre > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On 13/08/2018 20:18:08+0200, Sam Ravnborg wrote: > > Would be good to have a plan to phase-out the old atmel_lcdfb fbdev > > driver when this one addresses some TODO items that make sense. > Agree on this. > One approach could be to say that when all in-kernel users of atmel_lcdfb > are ported, then the old driver could be dropped after a kernel release. > I would drop it only after an LTS release. > > The mfd suffix seems strange to me. What about "atmel,at91sam9263-lcdc-mfd" > > => "atmel,at91sam9263-lcd" (or "microchip,at91sam9263-lcdc"). > The "-mfd" suffix was added to avoid clashing with the current > compatible string used by the atmel_lcdfb driver. > > I susggest we do the following: > - use the microchip prefix, as this is now owned by microchip > - and add the driver to a drm/microchip/ directory > (Then we can only hope that microchip do not change name or > are purchased by someone else). > The compatible string should remain the same but the drivers have to be mutually exclusive in Kconfig.
On Tue, Aug 14, 2018 at 04:36:03PM +0200, Alexandre Belloni wrote: > On 13/08/2018 20:18:08+0200, Sam Ravnborg wrote: > > > Would be good to have a plan to phase-out the old atmel_lcdfb fbdev > > > driver when this one addresses some TODO items that make sense. > > Agree on this. > > One approach could be to say that when all in-kernel users of atmel_lcdfb > > are ported, then the old driver could be dropped after a kernel release. > > > > I would drop it only after an LTS release. Much better, agreed. > > > > The mfd suffix seems strange to me. What about "atmel,at91sam9263-lcdc-mfd" > > > => "atmel,at91sam9263-lcd" (or "microchip,at91sam9263-lcdc"). > > The "-mfd" suffix was added to avoid clashing with the current > > compatible string used by the atmel_lcdfb driver. > > > > I susggest we do the following: > > - use the microchip prefix, as this is now owned by microchip > > - and add the driver to a drm/microchip/ directory > > (Then we can only hope that microchip do not change name or > > are purchased by someone else). > > > > The compatible string should remain the same but the drivers have to be > mutually exclusive in Kconfig. OK, will do so in v2. I had planned to keep both in the DT file but then I will just replace one with the other. Sam
Hi Rob. > I don't know that 2 registers for a backlight PWM constitute an MFD. A > single node can be both an LCD controller and a PWM. Current suggestion from v1 patchset looks like this: lcdc0: lcdc@700000 { compatible = "atmel,at91sam9263-lcdc-mfd"; reg = <0x700000 0x1000>; interrupts = <26 IRQ_TYPE_LEVEL_HIGH 3>; clocks = <&lcd_clk>, <&lcd_clk>; clock-names = "lcdc_clk", "hclk"; lcdc-display-controller { compatible = "atmel,lcdc-display-controller"; lcd-supply = <&lcdc_reg>; port@0 { }; }; lcdc_pwm: lcdc-pwm { compatible = "atmel,lcdc-pwm"; #pwm-cells = <3>; }; }; backlight: backlight { compatible = "pwm-backlight"; pwms = <&lcdc_pwm 0 50000 0>; }; We could have described the full binding in one file, rather than in three files like it is done in v1. But the structure was done so it matched what was done for hlcdc. If I understand the proposal from you correct an example binding would look like this: lcdc0: lcdc@700000 { compatible = "atmel,at91sam9263-lcdc"; reg = <0x700000 0x1000>; interrupts = <26 IRQ_TYPE_LEVEL_HIGH 3>; clocks = <&lcd_clk>, <&lcd_clk>; clock-names = "lcdc_clk", "hclk"; lcd-supply = <&lcdc_reg>; port@0 { }; #pwm-cells = <3>; }; backlight: backlight { compatible = "pwm-backlight"; pwms = <&lcd0 0 50000 0>; }; This is doable, but IMO it is less obvious that the LCDC IP core implements two different features - a PWM and a LCD controller. Right now the preference is to stay with the v1 approach: - It is a mirror of what we do today for hlcdc, so no suprises - It shows in a nice way that the LCDC IP core implments both an LCD controller and a PWM - The pwm functionality is not hiddin inside the lcdc stuff, and it is thus simpler to add good pin-ctrl handles with nice names that matches the usage. (I could always add more pin-ctrl, but it is common to refer to a single pin-ctrl. One DT related Q: The LCD Controller supports BGR565, but as this is less common some HW implmentations exchange R and B, expessed in the old binding as wiring-mode like this: atmel,lcd-wiring-mode: lcd wiring mode "RGB" or "BRG" How can we express this wiring-mode in a generic way, both in DT and in code? Is it something that in DRM belongs to the panel, the encoder, the connector, or? And can any of the exisitng flags be used? Thanks in advance, Sam
On Tue, Aug 14, 2018 at 06:43:43PM +0200, Sam Ravnborg wrote: > Hi Rob. > > > I don't know that 2 registers for a backlight PWM constitute an MFD. A > > single node can be both an LCD controller and a PWM. > > Current suggestion from v1 patchset looks like this: > lcdc0: lcdc@700000 { > compatible = "atmel,at91sam9263-lcdc-mfd"; > reg = <0x700000 0x1000>; > interrupts = <26 IRQ_TYPE_LEVEL_HIGH 3>; > clocks = <&lcd_clk>, <&lcd_clk>; > clock-names = "lcdc_clk", "hclk"; > > lcdc-display-controller { > compatible = "atmel,lcdc-display-controller"; > lcd-supply = <&lcdc_reg>; > > port@0 { > }; > }; > > lcdc_pwm: lcdc-pwm { > compatible = "atmel,lcdc-pwm"; > #pwm-cells = <3>; > }; > > }; > > backlight: backlight { > compatible = "pwm-backlight"; > pwms = <&lcdc_pwm 0 50000 0>; > }; > > > We could have described the full binding in one file, rather than in > three files like it is done in v1. But the structure was done so it matched > what was done for hlcdc. > > > If I understand the proposal from you correct an example binding would > look like this: > > lcdc0: lcdc@700000 { > compatible = "atmel,at91sam9263-lcdc"; > reg = <0x700000 0x1000>; > interrupts = <26 IRQ_TYPE_LEVEL_HIGH 3>; > clocks = <&lcd_clk>, <&lcd_clk>; > clock-names = "lcdc_clk", "hclk"; > > lcd-supply = <&lcdc_reg>; > > port@0 { > }; > > #pwm-cells = <3>; > }; > > backlight: backlight { > compatible = "pwm-backlight"; > pwms = <&lcd0 0 50000 0>; > }; > > > This is doable, but IMO it is less obvious that the LCDC IP core implements > two different features - a PWM and a LCD controller. Features don't equate to nodes. If the sub-blocks can be separately instantiated or have their own resources then sub-nodes make sense. Otherwise, it's a single device (node) with multiple providers. > Right now the preference is to stay with the v1 approach: > - It is a mirror of what we do today for hlcdc, so no suprises Which BTW doesn't appear to have actually been reviewed. Do these IP blocks actually share anything? Consistency is nice, but keeping compatibility with the existing binding by extending things in a backwards compatible way is more important. Implementing a new driver is not license to change the binding. Shall I let u-boot or *BSD developers change the binding too for their driver? > - It shows in a nice way that the LCDC IP core implments both an LCD controller and a PWM > - The pwm functionality is not hiddin inside the lcdc stuff, and it is thus > simpler to add good pin-ctrl handles with nice names that matches the usage. > (I could always add more pin-ctrl, but it is common to refer to a single pin-ctrl. Does anyone actually use this for a non-backlight PWM? I'm not sure the abstraction is worth it. The driver could just register itself as a backlight provider rather than a PWM provider. > One DT related Q: > The LCD Controller supports BGR565, but as this is less common some HW implmentations > exchange R and B, expessed in the old binding as wiring-mode like this: > > atmel,lcd-wiring-mode: lcd wiring mode "RGB" or "BRG" > > How can we express this wiring-mode in a generic way, both in DT and in code? > Is it something that in DRM belongs to the panel, the encoder, the connector, or? > And can any of the exisitng flags be used? I thought we had come up with a common definition, but I guess it didn't make it upstream. It's definitely needed and I've been rejecting anything new that's vendor specific. Rob
Hi Rob. Thanks for the feedback, as I am rather new to this DT stuff a few iterations are no suprise. > > If I understand the proposal from you correct an example binding would > > look like this: > > > > lcdc0: lcdc@700000 { > > compatible = "atmel,at91sam9263-lcdc"; > > reg = <0x700000 0x1000>; > > interrupts = <26 IRQ_TYPE_LEVEL_HIGH 3>; > > clocks = <&lcd_clk>, <&lcd_clk>; > > clock-names = "lcdc_clk", "hclk"; > > > > lcd-supply = <&lcdc_reg>; > > > > port@0 { > > }; > > > > #pwm-cells = <3>; > > }; > > > > backlight: backlight { > > compatible = "pwm-backlight"; > > pwms = <&lcd0 0 50000 0>; > > }; > > > > > > This is doable, but IMO it is less obvious that the LCDC IP core implements > > two different features - a PWM and a LCD controller. > > Features don't equate to nodes. If the sub-blocks can be separately > instantiated or have their own resources then sub-nodes make sense. > Otherwise, it's a single device (node) with multiple providers. > > > Right now the preference is to stay with the v1 approach: > > - It is a mirror of what we do today for hlcdc, so no suprises > > Which BTW doesn't appear to have actually been reviewed. > > Do these IP blocks actually share anything? They have registers in the same memory area - and I think that it. > Consistency is nice, but keeping compatibility with the existing binding > by extending things in a backwards compatible way is more important. > Implementing a new driver is not license to change the binding. Shall I > let u-boot or *BSD developers change the binding too for their driver? So in other words - the existing binding in atmel,lcdc.txt needs to be extended in a backward compatible way. I will take a look at that and post a proposal in v2. > > - It shows in a nice way that the LCDC IP core implments both an LCD controller and a PWM > > - The pwm functionality is not hiddin inside the lcdc stuff, and it is thus > > simpler to add good pin-ctrl handles with nice names that matches the usage. > > (I could always add more pin-ctrl, but it is common to refer to a single pin-ctrl. > > Does anyone actually use this for a non-backlight PWM? I'm not sure the > abstraction is worth it. The driver could just register itself as a > backlight provider rather than a PWM provider. Makes sense. > > > One DT related Q: > > The LCD Controller supports BGR565, but as this is less common some HW implmentations > > exchange R and B, expessed in the old binding as wiring-mode like this: > > > > atmel,lcd-wiring-mode: lcd wiring mode "RGB" or "BRG" > > > > How can we express this wiring-mode in a generic way, both in DT and in code? > > Is it something that in DRM belongs to the panel, the encoder, the connector, or? > > And can any of the exisitng flags be used? > > I thought we had come up with a common definition, but I guess it didn't > make it upstream. It's definitely needed and I've been rejecting > anything new that's vendor specific. I found this: https://patchwork.ozlabs.org/patch/659570/ The suggestion with a boolean seems simple and I will try that. Sam
On Tue, Aug 14, 2018 at 10:48 PM Sam Ravnborg <sam@ravnborg.org> wrote: > [...] > > > One DT related Q: > > > The LCD Controller supports BGR565, but as this is less common some HW implmentations > > > exchange R and B, expessed in the old binding as wiring-mode like this: > > > > > > atmel,lcd-wiring-mode: lcd wiring mode "RGB" or "BRG" > > > > > > How can we express this wiring-mode in a generic way, both in DT and in code? > > > Is it something that in DRM belongs to the panel, the encoder, the connector, or? > > > And can any of the exisitng flags be used? > > > > I thought we had come up with a common definition, but I guess it didn't > > make it upstream. It's definitely needed and I've been rejecting > > anything new that's vendor specific. > I found this: https://patchwork.ozlabs.org/patch/659570/ > The suggestion with a boolean seems simple and I will try that. That's really too simple to be common. There's been a few other attempts[1][2][3] (for Atmel HLCDC in fact) with [3] probably being the closest to merging. The binding looked fine to me, but looks like the DRM implementation had some issues. Rob [1] https://lists.freedesktop.org/archives/dri-devel/2018-April/173089.html [2] https://patchwork.kernel.org/patch/9965079/ [3] http://lists.infradead.org/pipermail/linux-arm-kernel/2018-March/568731.html
On Wed, Aug 15, 2018 at 4:45 PM, Rob Herring <robh@kernel.org> wrote: > On Tue, Aug 14, 2018 at 10:48 PM Sam Ravnborg <sam@ravnborg.org> wrote: >> > > [...] > >> > > One DT related Q: >> > > The LCD Controller supports BGR565, but as this is less common some HW implmentations >> > > exchange R and B, expessed in the old binding as wiring-mode like this: >> > > >> > > atmel,lcd-wiring-mode: lcd wiring mode "RGB" or "BRG" >> > > >> > > How can we express this wiring-mode in a generic way, both in DT and in code? >> > > Is it something that in DRM belongs to the panel, the encoder, the connector, or? >> > > And can any of the exisitng flags be used? >> > >> > I thought we had come up with a common definition, but I guess it didn't >> > make it upstream. It's definitely needed and I've been rejecting >> > anything new that's vendor specific. >> I found this: https://patchwork.ozlabs.org/patch/659570/ >> The suggestion with a boolean seems simple and I will try that. > > That's really too simple to be common. There's been a few other > attempts[1][2][3] (for Atmel HLCDC in fact) with [3] probably being > the closest to merging. The binding looked fine to me, but looks like > the DRM implementation had some issues. > > Rob > > [1] https://lists.freedesktop.org/archives/dri-devel/2018-April/173089.html > [2] https://patchwork.kernel.org/patch/9965079/ > [3] http://lists.infradead.org/pipermail/linux-arm-kernel/2018-March/568731.html There's a standardized bus_format for drm_panel. IIrc there's been discussions to add something similar to drm_bridge, but didn't go all that far for reasons. Lots of drivers just handle this internally in some way. So no idea why this all got stalled. -Daniel
On 2018-08-15 17:04, Daniel Vetter wrote: > On Wed, Aug 15, 2018 at 4:45 PM, Rob Herring <robh@kernel.org> wrote: >> On Tue, Aug 14, 2018 at 10:48 PM Sam Ravnborg <sam@ravnborg.org> wrote: >>> >> >> [...] >> >>>>> One DT related Q: >>>>> The LCD Controller supports BGR565, but as this is less common some HW implmentations >>>>> exchange R and B, expessed in the old binding as wiring-mode like this: >>>>> >>>>> atmel,lcd-wiring-mode: lcd wiring mode "RGB" or "BRG" >>>>> >>>>> How can we express this wiring-mode in a generic way, both in DT and in code? >>>>> Is it something that in DRM belongs to the panel, the encoder, the connector, or? >>>>> And can any of the exisitng flags be used? >>>> >>>> I thought we had come up with a common definition, but I guess it didn't >>>> make it upstream. It's definitely needed and I've been rejecting >>>> anything new that's vendor specific. >>> I found this: https://patchwork.ozlabs.org/patch/659570/ >>> The suggestion with a boolean seems simple and I will try that. >> >> That's really too simple to be common. There's been a few other >> attempts[1][2][3] (for Atmel HLCDC in fact) with [3] probably being >> the closest to merging. The binding looked fine to me, but looks like >> the DRM implementation had some issues. >> >> Rob >> >> [1] https://lists.freedesktop.org/archives/dri-devel/2018-April/173089.html >> [2] https://patchwork.kernel.org/patch/9965079/ >> [3] http://lists.infradead.org/pipermail/linux-arm-kernel/2018-March/568731.html > > There's a standardized bus_format for drm_panel. IIrc there's been > discussions to add something similar to drm_bridge, but didn't go all > that far for reasons. > > Lots of drivers just handle this internally in some way. So no idea > why this all got stalled. I stopped pushing that patch for reasons mentioned in https://lkml.org/lkml/2018/4/9/108 However, since then, the component approach mentioned in that mail was shot down and instead the tda998x driver is now a bridge (or soon, I expect the series from Russell King that to land in 4.19-rc1) which means that the binding from that series is back on the table. At least I guess so? However, in that series the approach is that the bridge states its expected input and the output then adjusts to what is needed. However, the "problem" is really in the atmel-hlcdc output (it moves the MSB for the RGB colors around depending on the output mode) so I no longer subscribe to all ideas in that series and think it is cleaner to state the needed bus format closer to the atmel- hlcdc endpoint as is done e.g. in this latest series: https://lkml.org/lkml/2018/8/10/309 This latest series uses the media/video-interface approach and specifies the bus-width in the endpoint. However, the bus-width is alone obviously not enough to differentiate rgb565 and brg565, so will not help Sam (or is it bgr565 that Sam needs? The above quoted text is ambiguous). I think something like my bus-format binding in [3] is generic enough to also help Sam. Cheers, Peter
Hi Peter. > > I stopped pushing that patch for reasons mentioned in > https://lkml.org/lkml/2018/4/9/108 > > However, since then, the component approach mentioned in that mail was > shot down and instead the tda998x driver is now a bridge (or soon, I expect > the series from Russell King that to land in 4.19-rc1) which means that the > binding from that series is back on the table. At least I guess so? However, > in that series the approach is that the bridge states its expected input and > the output then adjusts to what is needed. However, the "problem" is really in > the atmel-hlcdc output (it moves the MSB for the RGB colors around depending > on the output mode) so I no longer subscribe to all ideas in that series > and think it is cleaner to state the needed bus format closer to the atmel- > hlcdc endpoint as is done e.g. in this latest series: > > https://lkml.org/lkml/2018/8/10/309 > > This latest series uses the media/video-interface approach and specifies > the bus-width in the endpoint. However, the bus-width is alone obviously not > enough to differentiate rgb565 and brg565, so will not help Sam Some digging in diverse threads required. I may take the easy way in v2 and postpone this feature. But you all confirmed that doing something locally in the driver is the wrong approach so that is ruled out. > (or is it bgr565 that Sam needs? The above quoted text is ambiguous). My bad. I need either rgb565 or bgr565 (at lest this is how I understood the application note from Atmel/Microchip - need to re-read this too). Some of the IP cores have an intensify bit which I do not yet understand how to adapt to the supported formats. This is also on the todo list. > I think something like my bus-format binding in [3] is generic enough to > also help Sam. Thanks, if I find time I will look into this, but first priority is to get the bindinns correct, and then the overall structure of the driver. Sam
Hi Daniel. > > > Based on the above, would it be possible to utilise some > > > of these features without any dedicated userspace (mesa) support? > > > Any other driver that has something similar that can be used > > > for inspiration? > > > > This is really an interesting question indeed. > > Needs userspace like everything else. There's unfortunately no real > standard for 2d apis in userspace, which means none of these efforts go > very far. Mostly it's just some custom-made X drivers. > > Adding a generic 2d api to drm is a FAQ, and the answer is "no". And then you went off writing a blog post about it - thanks. The blog post provided a lot of useful answers - great. (Or rather you will write the blog in 5 days, seems to be some time machine involved. (Date is the 27th below the title)). Sam
On Wed, Aug 22, 2018 at 10:12 PM, Sam Ravnborg <sam@ravnborg.org> wrote: > Hi Daniel. > >> > > Based on the above, would it be possible to utilise some >> > > of these features without any dedicated userspace (mesa) support? >> > > Any other driver that has something similar that can be used >> > > for inspiration? >> > >> > This is really an interesting question indeed. >> >> Needs userspace like everything else. There's unfortunately no real >> standard for 2d apis in userspace, which means none of these efforts go >> very far. Mostly it's just some custom-made X drivers. >> >> Adding a generic 2d api to drm is a FAQ, and the answer is "no". > > And then you went off writing a blog post about it - thanks. > The blog post provided a lot of useful answers - great. Yeah I figured more detail than this here would be good, but also wanted to avoid having to retype that ever again. > (Or rather you will write the blog in 5 days, seems to be some time > machine involved. (Date is the 27th below the title)). My blog platform pulled one on me - pushed it out with a future date, with the idea to proof-read a bit more. Didn't work, so everyone read the slightly draft-ish version :-/ In the past this did work, and was very neat. -Daniel
Hi Sam, On Sun, 12 Aug 2018 20:41:52 +0200 Sam Ravnborg <sam@ravnborg.org> wrote: > New DRM based driver for at91sam9 SOC's that uses the > Atmel LCDC IP core. First of all, thanks for this contribution. > > This is first version of a patch set that adds > drivers for the Atmel LCDC IP core. > Posted for review as the basics works now. > > The LCDC IP core contains two devices: > - a PWM often used for backlight > - a LCD display controller > > Both devices are supported today by the atmel_lcdfb driver. > For this new set of drivers the compatible strings was > selected to avoid clash with the existing compatible > strings used for the atmel_lcdfb driver to allow them > to co-exist. Hm, I think Rob commented on that already, but we usually try to stay compatible with the exisiting/old bindings when introducing a new one. Don't know how feasible this is in this particular case though. > > This patchset implements three drivers. > - A MFD driver that include the generic parts. > - A PWM driver. > - A DRM display controller driver. > This is the same split as used for the Atmel hlcdc IP. > > The hlcdc and lcdc has only a few things in common and > trying to share the code for them was not a viable solution. > > The DRM implementation has a few shortcomings compared to the > existing fbdev based driver: > - STN displays are not supported > Binding support is missing but most of the > STN specific functionality is otherwise ported > from the fbdev driver. > I assume the info should come from the panel > but as I lack HW I have not looked too much > into what is required. > - gamma support is missing > The driver utilises drm_simple_kms_helper and > this helper lacks support for setting up gamma. > If this is useful please let me know and I > will extend drm_simple_kms_helper to support this > and update the driver. I guess you can skip that for now. > - modesetting is not checked (see TODO in file) > Is this required for such a simple setup? Well, that's always better if you can check that the requested display mode is supported before trying to apply it. > - support for extra modes as applicable (and lcd-wiring-mode) Peter already suggested something I think. > - support for AVR32 (is it relevant?) It is, AVR32 is no longer supported in mainline. > > The first patch renames .../drm/atmel-hlcdc to .../drm/atmel > to have a nice home for both drivers. Sounds good. Regards, Boris
Hi Boris. > > > > Both devices are supported today by the atmel_lcdfb driver. > > For this new set of drivers the compatible strings was > > selected to avoid clash with the existing compatible > > strings used for the atmel_lcdfb driver to allow them > > to co-exist. > > Hm, I think Rob commented on that already, but we usually try to stay > compatible with the exisiting/old bindings when introducing a new one. > Don't know how feasible this is in this particular case though. I v2 I am working with a backward compatible approach. This is better that what I cam up with initially. > > The DRM implementation has a few shortcomings compared to the > > existing fbdev based driver: > > - STN displays are not supported > > Binding support is missing but most of the > > STN specific functionality is otherwise ported > > from the fbdev driver. > > I assume the info should come from the panel > > but as I lack HW I have not looked too much > > into what is required. > > - gamma support is missing > > The driver utilises drm_simple_kms_helper and > > this helper lacks support for setting up gamma. > > If this is useful please let me know and I > > will extend drm_simple_kms_helper to support this > > and update the driver. > > I guess you can skip that for now. Also based on feedback from Nicholas the STN parts will be dropped in v2. For the gamma stuff this looks feasible with a small extension to drm_simple_kms_helper - but I dunno if this is something that userspace will actually use. So that will wait until there is some good reason to implement it, and I know how to test it too. > > > - modesetting is not checked (see TODO in file) > > Is this required for such a simple setup? > > Well, that's always better if you can check that the requested display > mode is supported before trying to apply it. I will try to cook up something, have learned a little since posting v1. > > > - support for extra modes as applicable (and lcd-wiring-mode) > > Peter already suggested something I think. Yep, plenty of links to read. Sam
New DRM based driver for at91sam9 SOC's that uses the Atmel LCDC IP core. This is first version of a patch set that adds drivers for the Atmel LCDC IP core. Posted for review as the basics works now. The LCDC IP core contains two devices: - a PWM often used for backlight - a LCD display controller Both devices are supported today by the atmel_lcdfb driver. For this new set of drivers the compatible strings was selected to avoid clash with the existing compatible strings used for the atmel_lcdfb driver to allow them to co-exist. This patchset implements three drivers. - A MFD driver that include the generic parts. - A PWM driver. - A DRM display controller driver. This is the same split as used for the Atmel hlcdc IP. The hlcdc and lcdc has only a few things in common and trying to share the code for them was not a viable solution. The DRM implementation has a few shortcomings compared to the existing fbdev based driver: - STN displays are not supported Binding support is missing but most of the STN specific functionality is otherwise ported from the fbdev driver. I assume the info should come from the panel but as I lack HW I have not looked too much into what is required. - gamma support is missing The driver utilises drm_simple_kms_helper and this helper lacks support for setting up gamma. If this is useful please let me know and I will extend drm_simple_kms_helper to support this and update the driver. - modesetting is not checked (see TODO in file) Is this required for such a simple setup? - support for extra modes as applicable (and lcd-wiring-mode) - support for AVR32 (is it relevant?) The first patch renames .../drm/atmel-hlcdc to .../drm/atmel to have a nice home for both drivers. The drivers _works_ on a proprietary at91sam9263 based board and I will eventually migrate the at91sam9263ek over to use it as well. Works is maybe a stretch - the following was tested: modetest shows reasonable output modetest -s shows some nice colored squares vbltest shows 45.70 Hz (after terminating modetest -s with SIGINT) drmdevice shows reasonable output brightness can be controlled Anything else that can be recommended for some basic tests? How to test suspend/resume? REVIEW Please give feedback on general structure/architecture Please check if the drm framework is used in the optimal way And then the usual review from spelling errors, names, style etc. All feedback (from spelling errors to general structure) appreciated! Sam Sam Ravnborg (7): atmel-hlcdc: renamed directory to drm/atmel/ dt-binding: add bindings for Atmel LCDC mfd mfd: add atmel-lcdc driver dt-bindings: add bindings for Atmel LCDC pwm pwm: add pwm-atmel-lcdc driver dt-bindings: add bindings for Atmel lcdc-display-controller drm: add Atmel LCDC display controller support .../display/atmel/lcdc-display-controller.txt | 40 + .../devicetree/bindings/mfd/atmel-lcdc.txt | 75 ++ .../devicetree/bindings/pwm/atmel-lcdc-pwm.txt | 30 + MAINTAINERS | 9 +- drivers/gpu/drm/Kconfig | 2 +- drivers/gpu/drm/Makefile | 2 +- drivers/gpu/drm/atmel-hlcdc/Kconfig | 10 - drivers/gpu/drm/atmel/Kconfig | 28 + drivers/gpu/drm/{atmel-hlcdc => atmel}/Makefile | 2 + .../drm/{atmel-hlcdc => atmel}/atmel_hlcdc_crtc.c | 0 .../drm/{atmel-hlcdc => atmel}/atmel_hlcdc_dc.c | 0 .../drm/{atmel-hlcdc => atmel}/atmel_hlcdc_dc.h | 0 .../{atmel-hlcdc => atmel}/atmel_hlcdc_output.c | 0 .../drm/{atmel-hlcdc => atmel}/atmel_hlcdc_plane.c | 0 drivers/gpu/drm/atmel/atmel_lcdc-dc.c | 1094 ++++++++++++++++++++ drivers/mfd/Kconfig | 10 + drivers/mfd/Makefile | 1 + drivers/mfd/atmel-lcdc.c | 158 +++ drivers/pwm/Kconfig | 13 + drivers/pwm/Makefile | 1 + drivers/pwm/pwm-atmel-lcdc.c | 178 ++++ include/linux/mfd/atmel-lcdc.h | 184 ++++ 22 files changed, 1824 insertions(+), 13 deletions(-)