[RFC PATCH 1/3] drm: Add support for Amlogic Meson Graphic Controller
Hi Daniel,
On 11/29/2016 09:50 AM, Daniel Vetter wrote:
> Hi Neil,
>
> On Mon, Nov 28, 2016 at 10:34:58AM +0100, Neil Armstrong wrote:
>> On 11/28/2016 09:16 AM, Daniel Vetter wrote:
>>> On Fri, Nov 25, 2016 at 05:03:09PM +0100, Neil Armstrong wrote:
+static void meson_cvbs_encoder_disable(struct drm_encoder *encoder)
+{
+ struct meson_cvbs *meson_cvbs = encoder_to_meson_cvbs(encoder);
+
+ meson_venci_cvbs_disable(meson_cvbs->priv);
+}
+
+static void meson_cvbs_encoder_enable(struct drm_encoder *encoder)
+{
+ struct meson_cvbs *meson_cvbs = encoder_to_meson_cvbs(encoder);
+
+ meson_venci_cvbs_enable(meson_cvbs->priv);
+}
>>>
>>> Personally I'd remove the indirection above, more direct code is easier to
>>> read.
>>
>> I understand, I'll maybe change the meson_venci_cvbs_XXable to be
>> directly added to the ops.
>>
>> I want to keep the registers setup in separate files and keep a clean
>> DRM/HW separation.
>
> I figured this is worth clarifying, and I'm somewhat guessing at your
> motivation here for a clean drm/hw split. There's of course various levels
> of how much you can split the drm side from your hw backend, but in
> general that design approach is really unpopular with upstream. It goes by
> the name of "midlayer", and the trouble with it is that it makes
> subsystem refactoring more complicated.
I totally understand, and I personally would prefer to have an unified drm
source,
but the state of the hardware architecture makes it very hard to map cleanly
over DRM.
I moved all the CRTC and Plane code into the corresponding file ans only kept
the init and common functions in the backend files.
>
> For the driver itself it's nice, because it isolates you a bit from drm
> core. But that exact isolation is the problem when someone wants (or more
> often, needs to) refactor something across the entire subsystem. Then all
> these driver-private little (or sometimes much bigger) abstractions get in
> the way. That's way I suggested to remove it (both here and in the plane
> code), because for upstream the overall subsystem matters more than each
> individual driver. GPUs change fast, we need to be able to adapt fast,
> too.
It totally makes sense and I'm totally ready to refactor when necessary and
match
the DRM/KMS architecture.
>
> Anyway you're driver's pretty small, so personally I don't mind much. I'd
> still think removing the indirection would be better though.
>
> Thanks, Daniel
>
Thanks,
Neil
[RFC PATCH 1/3] drm: Add support for Amlogic Meson Graphic Controller
Hi Neil,
On Mon, Nov 28, 2016 at 10:34:58AM +0100, Neil Armstrong wrote:
> On 11/28/2016 09:16 AM, Daniel Vetter wrote:
> > On Fri, Nov 25, 2016 at 05:03:09PM +0100, Neil Armstrong wrote:
> >> +static void meson_cvbs_encoder_disable(struct drm_encoder *encoder)
> >> +{
> >> + struct meson_cvbs *meson_cvbs = encoder_to_meson_cvbs(encoder);
> >> +
> >> + meson_venci_cvbs_disable(meson_cvbs->priv);
> >> +}
> >> +
> >> +static void meson_cvbs_encoder_enable(struct drm_encoder *encoder)
> >> +{
> >> + struct meson_cvbs *meson_cvbs = encoder_to_meson_cvbs(encoder);
> >> +
> >> + meson_venci_cvbs_enable(meson_cvbs->priv);
> >> +}
> >
> > Personally I'd remove the indirection above, more direct code is easier to
> > read.
>
> I understand, I'll maybe change the meson_venci_cvbs_XXable to be
> directly added to the ops.
>
> I want to keep the registers setup in separate files and keep a clean
> DRM/HW separation.
I figured this is worth clarifying, and I'm somewhat guessing at your
motivation here for a clean drm/hw split. There's of course various levels
of how much you can split the drm side from your hw backend, but in
general that design approach is really unpopular with upstream. It goes by
the name of "midlayer", and the trouble with it is that it makes
subsystem refactoring more complicated.
For the driver itself it's nice, because it isolates you a bit from drm
core. But that exact isolation is the problem when someone wants (or more
often, needs to) refactor something across the entire subsystem. Then all
these driver-private little (or sometimes much bigger) abstractions get in
the way. That's way I suggested to remove it (both here and in the plane
code), because for upstream the overall subsystem matters more than each
individual driver. GPUs change fast, we need to be able to adapt fast,
too.
Anyway you're driver's pretty small, so personally I don't mind much. I'd
still think removing the indirection would be better though.
Thanks, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
[RFC PATCH 1/3] drm: Add support for Amlogic Meson Graphic Controller
Hi Daniel, On 11/28/2016 09:16 AM, Daniel Vetter wrote: > On Fri, Nov 25, 2016 at 05:03:09PM +0100, Neil Armstrong wrote: >> The Amlogic Meson Display controller is composed of several components : >> >> DMC|---VPU (Video Processing >> Unit)|--HHI--| >>| vd1 ___ __ | >> | >> D |---| ||| ||| HDMI PLL >> | >> D | vd2 | VIU || Video Post | | Video Encoders |<---|-VCLK >> | >> R |---| || Processing | ||| >> | >>| osd2 | |||---| Enci >> --||-VDAC--| >> R |---| CSC || Scalers| | Encp >> --||HDMI-TX| >> A | osd1 | || Blenders | | Encl >> --||---| >> M |---|__||| ||| >> | >> ___|__|___| >> >> >> VIU: Video Input Unit >> - >> >> The Video Input Unit is in charge of the pixel scanout from the DDR memory. >> It fetches the frames addresses, stride and parameters from the "Canvas" >> memory. >> This part is also in charge of the CSC (Colorspace Conversion). >> It can handle 2 OSD Planes and 2 Video Planes. >> >> VPP: Video Processing Unit >> -- >> >> The Video Processing Unit is in charge if the scaling and blending of the >> various planes into a single pixel stream. >> There is a special "pre-blending" used by the video planes with a dedicated >> scaler and a "post-blending" to merge with the OSD Planes. >> The OSD planes also have a dedicated scaler for one of the OSD. >> >> VENC: Video Encoders >> >> >> The VENC is composed of the multiple pixel encoders : >> - ENCI : Interlace Video encoder for CVBS and Interlace HDMI >> - ENCP : Progressive Video Encoder for HDMI >> - ENCL : LCD LVDS Encoder >> The VENC Unit gets a Pixel Clocks (VCLK) from a dedicated HDMI PLL and clock >> tree and provides the scanout clock to the VPP and VIU. >> The ENCI is connected to a single VDAC for Composite Output. >> The ENCI and ENCP are connected to an on-chip HDMI Transceiver. >> >> This driver is a DRM/KMS driver using the following DRM components : >> - GEM-CMA >> - PRIME-CMA >> - Atomic Modesetting >> - FBDev-CMA >> >> For the following SoCs : >> - GXBB Family (S905) >> - GXL Family (S905X, S905D) >> - GXM Family (S912) >> >> The current driver only supports the CVBS PAL/NTSC output modes, but the >> CRTC/Planes management should support bigger modes. >> But Advanced Colorspace Conversion, Scaling and HDMI Modes will be added in >> a second time. >> >> The Device Tree bindings makes use of the endpoints video interface >> definitions >> to connect to the optional CVBS and in the future the HDMI Connector nodes. >> >> HDMI Support is planned for a next release. >> >> Signed-off-by: Neil Armstrong > > Few small comments below, but looks reasonable overall. Once you have acks > for the DT part pls submit the entire series as a pull request to Dave > Airlie (with an additional patch to add a MAINTAINERS entry). Thanks for the review. Ok, will add the MAINTAINERS entry. > > Cheers, Daniel > >> --- >> drivers/gpu/drm/Kconfig |2 + >> drivers/gpu/drm/Makefile|1 + >> drivers/gpu/drm/meson/Kconfig |8 + >> drivers/gpu/drm/meson/Makefile |5 + >> drivers/gpu/drm/meson/meson_canvas.c| 96 +++ >> drivers/gpu/drm/meson/meson_canvas.h| 31 + >> drivers/gpu/drm/meson/meson_crtc.c | 176 >> drivers/gpu/drm/meson/meson_crtc.h | 34 + >> drivers/gpu/drm/meson/meson_cvbs.c | 293 +++ >> drivers/gpu/drm/meson/meson_cvbs.h | 32 + >> drivers/gpu/drm/meson/meson_drv.c | 383 + >> drivers/gpu/drm/meson/meson_drv.h | 68 ++ >> drivers/gpu/drm/meson/meson_plane.c | 150 >> drivers/gpu/drm/meson/meson_plane.h | 32 + >> drivers/gpu/drm/meson/meson_registers.h | 1395 >> +++ >> drivers/gpu/drm/meson/meson_vclk.c | 169 >> drivers/gpu/drm/meson/meson_vclk.h | 36 + >> drivers/gpu/drm/meson/meson_venc.c | 286 +++ >> drivers/gpu/drm/meson/meson_venc.h | 77 ++ >> drivers/gpu/drm/meson/meson_viu.c | 497 +++ >> drivers/gpu/drm/meson/meson_viu.h | 37 + >> drivers/gpu/drm/meson/meson_vpp.c | 189 + >> drivers/gpu/drm/meson/meson_vpp.h | 43 + >> 23 files changed, 4040 insertions(+) >> create mode 100644 drivers/gpu/drm/meson/Kconfig >> create mode 100644 drivers/gpu/drm/meson/Makefile >> create mode 100644 drivers/gpu/drm/meson/meson_canvas.c >> create mode 100644 drivers/gpu/drm/meson/meson_canvas.h >> create mode 100644
[RFC PATCH 1/3] drm: Add support for Amlogic Meson Graphic Controller
On Fri, Nov 25, 2016 at 05:03:09PM +0100, Neil Armstrong wrote: > The Amlogic Meson Display controller is composed of several components : > > DMC|---VPU (Video Processing > Unit)|--HHI--| >| vd1 ___ __ | > | > D |---| ||| ||| HDMI PLL > | > D | vd2 | VIU || Video Post | | Video Encoders |<---|-VCLK > | > R |---| || Processing | ||| > | >| osd2 | |||---| Enci > --||-VDAC--| > R |---| CSC || Scalers| | Encp > --||HDMI-TX| > A | osd1 | || Blenders | | Encl > --||---| > M |---|__||| ||| > | > ___|__|___| > > > VIU: Video Input Unit > - > > The Video Input Unit is in charge of the pixel scanout from the DDR memory. > It fetches the frames addresses, stride and parameters from the "Canvas" > memory. > This part is also in charge of the CSC (Colorspace Conversion). > It can handle 2 OSD Planes and 2 Video Planes. > > VPP: Video Processing Unit > -- > > The Video Processing Unit is in charge if the scaling and blending of the > various planes into a single pixel stream. > There is a special "pre-blending" used by the video planes with a dedicated > scaler and a "post-blending" to merge with the OSD Planes. > The OSD planes also have a dedicated scaler for one of the OSD. > > VENC: Video Encoders > > > The VENC is composed of the multiple pixel encoders : > - ENCI : Interlace Video encoder for CVBS and Interlace HDMI > - ENCP : Progressive Video Encoder for HDMI > - ENCL : LCD LVDS Encoder > The VENC Unit gets a Pixel Clocks (VCLK) from a dedicated HDMI PLL and clock > tree and provides the scanout clock to the VPP and VIU. > The ENCI is connected to a single VDAC for Composite Output. > The ENCI and ENCP are connected to an on-chip HDMI Transceiver. > > This driver is a DRM/KMS driver using the following DRM components : > - GEM-CMA > - PRIME-CMA > - Atomic Modesetting > - FBDev-CMA > > For the following SoCs : > - GXBB Family (S905) > - GXL Family (S905X, S905D) > - GXM Family (S912) > > The current driver only supports the CVBS PAL/NTSC output modes, but the > CRTC/Planes management should support bigger modes. > But Advanced Colorspace Conversion, Scaling and HDMI Modes will be added in > a second time. > > The Device Tree bindings makes use of the endpoints video interface > definitions > to connect to the optional CVBS and in the future the HDMI Connector nodes. > > HDMI Support is planned for a next release. > > Signed-off-by: Neil Armstrong Few small comments below, but looks reasonable overall. Once you have acks for the DT part pls submit the entire series as a pull request to Dave Airlie (with an additional patch to add a MAINTAINERS entry). Cheers, Daniel > --- > drivers/gpu/drm/Kconfig |2 + > drivers/gpu/drm/Makefile|1 + > drivers/gpu/drm/meson/Kconfig |8 + > drivers/gpu/drm/meson/Makefile |5 + > drivers/gpu/drm/meson/meson_canvas.c| 96 +++ > drivers/gpu/drm/meson/meson_canvas.h| 31 + > drivers/gpu/drm/meson/meson_crtc.c | 176 > drivers/gpu/drm/meson/meson_crtc.h | 34 + > drivers/gpu/drm/meson/meson_cvbs.c | 293 +++ > drivers/gpu/drm/meson/meson_cvbs.h | 32 + > drivers/gpu/drm/meson/meson_drv.c | 383 + > drivers/gpu/drm/meson/meson_drv.h | 68 ++ > drivers/gpu/drm/meson/meson_plane.c | 150 > drivers/gpu/drm/meson/meson_plane.h | 32 + > drivers/gpu/drm/meson/meson_registers.h | 1395 > +++ > drivers/gpu/drm/meson/meson_vclk.c | 169 > drivers/gpu/drm/meson/meson_vclk.h | 36 + > drivers/gpu/drm/meson/meson_venc.c | 286 +++ > drivers/gpu/drm/meson/meson_venc.h | 77 ++ > drivers/gpu/drm/meson/meson_viu.c | 497 +++ > drivers/gpu/drm/meson/meson_viu.h | 37 + > drivers/gpu/drm/meson/meson_vpp.c | 189 + > drivers/gpu/drm/meson/meson_vpp.h | 43 + > 23 files changed, 4040 insertions(+) > create mode 100644 drivers/gpu/drm/meson/Kconfig > create mode 100644 drivers/gpu/drm/meson/Makefile > create mode 100644 drivers/gpu/drm/meson/meson_canvas.c > create mode 100644 drivers/gpu/drm/meson/meson_canvas.h > create mode 100644 drivers/gpu/drm/meson/meson_crtc.c > create mode 100644 drivers/gpu/drm/meson/meson_crtc.h > create mode 100644 drivers/gpu/drm/meson/meson_cvbs.c > create mode 100644 drivers/gpu/drm/meson/meson_cvbs.h > create mode 100644
[RFC PATCH 1/3] drm: Add support for Amlogic Meson Graphic Controller
The Amlogic Meson Display controller is composed of several components : DMC|---VPU (Video Processing Unit)|--HHI--| | vd1 ___ __ | | D |---| ||| ||| HDMI PLL| D | vd2 | VIU || Video Post | | Video Encoders |<---|-VCLK | R |---| || Processing | ||| | | osd2 | |||---| Enci --||-VDAC--| R |---| CSC || Scalers| | Encp --||HDMI-TX| A | osd1 | || Blenders | | Encl --||---| M |---|__||| ||| | ___|__|___| VIU: Video Input Unit - The Video Input Unit is in charge of the pixel scanout from the DDR memory. It fetches the frames addresses, stride and parameters from the "Canvas" memory. This part is also in charge of the CSC (Colorspace Conversion). It can handle 2 OSD Planes and 2 Video Planes. VPP: Video Processing Unit -- The Video Processing Unit is in charge if the scaling and blending of the various planes into a single pixel stream. There is a special "pre-blending" used by the video planes with a dedicated scaler and a "post-blending" to merge with the OSD Planes. The OSD planes also have a dedicated scaler for one of the OSD. VENC: Video Encoders The VENC is composed of the multiple pixel encoders : - ENCI : Interlace Video encoder for CVBS and Interlace HDMI - ENCP : Progressive Video Encoder for HDMI - ENCL : LCD LVDS Encoder The VENC Unit gets a Pixel Clocks (VCLK) from a dedicated HDMI PLL and clock tree and provides the scanout clock to the VPP and VIU. The ENCI is connected to a single VDAC for Composite Output. The ENCI and ENCP are connected to an on-chip HDMI Transceiver. This driver is a DRM/KMS driver using the following DRM components : - GEM-CMA - PRIME-CMA - Atomic Modesetting - FBDev-CMA For the following SoCs : - GXBB Family (S905) - GXL Family (S905X, S905D) - GXM Family (S912) The current driver only supports the CVBS PAL/NTSC output modes, but the CRTC/Planes management should support bigger modes. But Advanced Colorspace Conversion, Scaling and HDMI Modes will be added in a second time. The Device Tree bindings makes use of the endpoints video interface definitions to connect to the optional CVBS and in the future the HDMI Connector nodes. HDMI Support is planned for a next release. Signed-off-by: Neil Armstrong --- drivers/gpu/drm/Kconfig |2 + drivers/gpu/drm/Makefile|1 + drivers/gpu/drm/meson/Kconfig |8 + drivers/gpu/drm/meson/Makefile |5 + drivers/gpu/drm/meson/meson_canvas.c| 96 +++ drivers/gpu/drm/meson/meson_canvas.h| 31 + drivers/gpu/drm/meson/meson_crtc.c | 176 drivers/gpu/drm/meson/meson_crtc.h | 34 + drivers/gpu/drm/meson/meson_cvbs.c | 293 +++ drivers/gpu/drm/meson/meson_cvbs.h | 32 + drivers/gpu/drm/meson/meson_drv.c | 383 + drivers/gpu/drm/meson/meson_drv.h | 68 ++ drivers/gpu/drm/meson/meson_plane.c | 150 drivers/gpu/drm/meson/meson_plane.h | 32 + drivers/gpu/drm/meson/meson_registers.h | 1395 +++ drivers/gpu/drm/meson/meson_vclk.c | 169 drivers/gpu/drm/meson/meson_vclk.h | 36 + drivers/gpu/drm/meson/meson_venc.c | 286 +++ drivers/gpu/drm/meson/meson_venc.h | 77 ++ drivers/gpu/drm/meson/meson_viu.c | 497 +++ drivers/gpu/drm/meson/meson_viu.h | 37 + drivers/gpu/drm/meson/meson_vpp.c | 189 + drivers/gpu/drm/meson/meson_vpp.h | 43 + 23 files changed, 4040 insertions(+) create mode 100644 drivers/gpu/drm/meson/Kconfig create mode 100644 drivers/gpu/drm/meson/Makefile create mode 100644 drivers/gpu/drm/meson/meson_canvas.c create mode 100644 drivers/gpu/drm/meson/meson_canvas.h create mode 100644 drivers/gpu/drm/meson/meson_crtc.c create mode 100644 drivers/gpu/drm/meson/meson_crtc.h create mode 100644 drivers/gpu/drm/meson/meson_cvbs.c create mode 100644 drivers/gpu/drm/meson/meson_cvbs.h create mode 100644 drivers/gpu/drm/meson/meson_drv.c create mode 100644 drivers/gpu/drm/meson/meson_drv.h create mode 100644 drivers/gpu/drm/meson/meson_plane.c create mode 100644 drivers/gpu/drm/meson/meson_plane.h create mode 100644 drivers/gpu/drm/meson/meson_registers.h create mode 100644 drivers/gpu/drm/meson/meson_vclk.c create mode 100644 drivers/gpu/drm/meson/meson_vclk.h create mode 100644 drivers/gpu/drm/meson/meson_venc.c create mode 100644 drivers/gpu/drm/meson/meson_venc.h create mode 100644 drivers/gpu/drm/meson/meson_viu.c create
