| Message ID | 20220720142732.32041-1-tzimmermann@suse.de |
|---|---|
| Headers | show |
| Series | drm: Add driver for PowerPC OF displays | expand |
On 7/20/22 16:27, Thomas Zimmermann wrote: > Remove the unused mem field from struct simpledrm_device. > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > --- Acked-by: Javier Martinez Canillas <javierm@redhat.com>
Hello Thomas, On 7/20/22 16:27, Thomas Zimmermann wrote: > Inline the helpers for initializing the hardware FB, the memory > management and the modesetting into the device-creation function. > No functional changes. > Could you please elaborate in the commit message why this change is desirable? Without this additional context, this feels like going backwards, since you are dropping few helpers that have quite self contained code and making simpledrm_device_create() much larger.
On 7/20/22 16:27, Thomas Zimmermann wrote: > Replace the remaining uses of the field pdev by upcasts from the Linux > device and remove the field. > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> Much better indeed. Acked-by: Javier Martinez Canillas <javierm@redhat.com>
On 7/20/22 16:27, Thomas Zimmermann wrote: > Replace the simple-KMS helpers with the regular atomic helpers. The > regular helpers are better architectured and therefore allow for easier > code sharing among drivers. No functional changes. > Acked-by: Javier Martinez Canillas <javierm@redhat.com> But I've a question below... > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > --- > drivers/gpu/drm/tiny/simpledrm.c | 283 ++++++++++++++++++++----------- > 1 file changed, 180 insertions(+), 103 deletions(-) [...] > +static void simpledrm_crtc_helper_atomic_enable(struct drm_crtc *crtc, > + struct drm_atomic_state *old_state) > +{ > + /* > + * Always enabled; screen updates are performed by > + * the primary plane's atomic_update function. > + */ > +} > + > +static void simpledrm_crtc_helper_atomic_disable(struct drm_crtc *crtc, > + struct drm_atomic_state *old_state) > +{ > + /* > + * Always enabled; disabling clears the screen in the > + * primary plane's atomic_disable function. > + */ > +} ...do we really need to have these ? Can't we just not set them ? > + > +static const struct drm_crtc_helper_funcs simpledrm_crtc_helper_funcs = { > + .mode_valid = simpledrm_crtc_helper_mode_valid, > + .atomic_check = simpledrm_crtc_helper_atomic_check, > + .atomic_enable = simpledrm_crtc_helper_atomic_enable, > + .atomic_disable = simpledrm_crtc_helper_atomic_disable, > +}; > + looking at https://elixir.bootlin.com/linux/latest/source/include/drm/drm_modeset_helper_vtables.h#L703 that says the .atomic_{en,dis}able handlers are optional.
Hello Thomas, On 7/20/22 16:27, Thomas Zimmermann wrote: > Open Firmware provides basic display output via the 'display' node. > DT platform code already provides a device that represents the node's > framebuffer. Add a DRM driver for the device. The display mode and > color format is pre-initialized by the system's firmware. Runtime > modesetting via DRM is not possible. The display is useful during > early boot stages or as error fallback. > I'm not familiar with OF display but the driver looks good to me. Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> I just have a few questions below. [...] > +static int ofdrm_primary_plane_helper_atomic_check(struct drm_plane *plane, > + struct drm_atomic_state *new_state) > +{ > + struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(new_state, plane); > + struct drm_crtc_state *new_crtc_state; > + int ret; > + > + if (!new_plane_state->fb) > + return 0; > + > + new_crtc_state = drm_atomic_get_new_crtc_state(new_state, new_plane_state->crtc); > + > + ret = drm_atomic_helper_check_plane_state(new_plane_state, new_crtc_state, > + DRM_PLANE_HELPER_NO_SCALING, > + DRM_PLANE_HELPER_NO_SCALING, > + false, false); > + if (ret) > + return ret; > + > + return 0; > +} This seems to be exactly the same check than used in the simpledrm driver. Maybe could be moved to the fwfb helper library too ? [...] > + > +static void ofdrm_crtc_helper_atomic_disable(struct drm_crtc *crtc, > + struct drm_atomic_state *old_state) > +{ > + /* > + * Always enabled; disabling clears the screen in the > + * primary plane's atomic_disable function. > + */ > +} > + Same comment than for simpledrm, are these no-op helpers really needed ? [...] > +static const struct of_device_id ofdrm_of_match_display[] = { > + { .compatible = "display", }, > + { }, > +}; > +MODULE_DEVICE_TABLE(of, ofdrm_of_match_display); > + I don't see a binding for this in Documentation/devicetree/bindings/display. Do we need one or it's that only required for FDT and not Open Firmware DT ?
Hi Am 25.07.22 um 17:01 schrieb Javier Martinez Canillas: > Hello Thomas, > > On 7/20/22 16:27, Thomas Zimmermann wrote: >> Inline the helpers for initializing the hardware FB, the memory >> management and the modesetting into the device-creation function. >> No functional changes. >> > > Could you please elaborate in the commit message why this change is > desirable? Without this additional context, this feels like going > backwards, since you are dropping few helpers that have quite self > contained code and making simpledrm_device_create() much larger. To clarify: I want to make the init code more easy to follow. These old init functions still had to be called in the right order as each possibly depends on settings from the others. It also feels like it's easier to extract common code for ofdrm. And the pipeline is static, so it doesn't require complex chains of helper calls. Having everything in one helper seems beneficial. (It's a trade-off, I know.) Best regards Thomas >
Hi Am 25.07.22 um 17:46 schrieb Javier Martinez Canillas: > On 7/20/22 16:27, Thomas Zimmermann wrote: >> Replace the simple-KMS helpers with the regular atomic helpers. The >> regular helpers are better architectured and therefore allow for easier >> code sharing among drivers. No functional changes. >> > > Acked-by: Javier Martinez Canillas <javierm@redhat.com> > > But I've a question below... > >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >> --- >> drivers/gpu/drm/tiny/simpledrm.c | 283 ++++++++++++++++++++----------- >> 1 file changed, 180 insertions(+), 103 deletions(-) > > [...] > >> +static void simpledrm_crtc_helper_atomic_enable(struct drm_crtc *crtc, >> + struct drm_atomic_state *old_state) >> +{ >> + /* >> + * Always enabled; screen updates are performed by >> + * the primary plane's atomic_update function. >> + */ >> +} >> + >> +static void simpledrm_crtc_helper_atomic_disable(struct drm_crtc *crtc, >> + struct drm_atomic_state *old_state) >> +{ >> + /* >> + * Always enabled; disabling clears the screen in the >> + * primary plane's atomic_disable function. >> + */ >> +} > > ...do we really need to have these ? Can't we just not set them ? > >> + >> +static const struct drm_crtc_helper_funcs simpledrm_crtc_helper_funcs = { >> + .mode_valid = simpledrm_crtc_helper_mode_valid, >> + .atomic_check = simpledrm_crtc_helper_atomic_check, >> + .atomic_enable = simpledrm_crtc_helper_atomic_enable, >> + .atomic_disable = simpledrm_crtc_helper_atomic_disable, >> +}; >> + > looking at https://elixir.bootlin.com/linux/latest/source/include/drm/drm_modeset_helper_vtables.h#L703 > that says the .atomic_{en,dis}able handlers are optional. > The code also looks like we don't need the helpers. I mostly added them for the comments they contain, but I can also add those next to simpledrm_crtc_helper_funcs. Best regards Thomas
Hello Thomas, On 7/27/22 09:50, Thomas Zimmermann wrote: > Hi > > Am 25.07.22 um 17:01 schrieb Javier Martinez Canillas: >> Hello Thomas, >> >> On 7/20/22 16:27, Thomas Zimmermann wrote: >>> Inline the helpers for initializing the hardware FB, the memory >>> management and the modesetting into the device-creation function. >>> No functional changes. >>> >> >> Could you please elaborate in the commit message why this change is >> desirable? Without this additional context, this feels like going >> backwards, since you are dropping few helpers that have quite self >> contained code and making simpledrm_device_create() much larger. > > To clarify: I want to make the init code more easy to follow. These old > init functions still had to be called in the right order as each > possibly depends on settings from the others. It also feels like it's > easier to extract common code for ofdrm. And the pipeline is static, so > it doesn't require complex chains of helper calls. Having everything in > one helper seems beneficial. (It's a trade-off, I know.) > I see. That makes sense to me. Could you please add the explanation to the commit message ? And feel free to add my Acked-by for this one too.
Thomas Zimmermann <tzimmermann@suse.de> writes: > (was: drm: Add driverof PowerPC OF displays) > > PowerPC's Open Firmware offers a simple display buffer for graphics > output. Add ofdrm, a DRM driver for the device. As with the existing > simpledrm driver, the graphics hardware is pre-initialized by the > firmware. The driver only provides blitting, no actual DRM modesetting > is possible. Hi Thomas, I tried to test this on a 32-bit ppc Mac Mini but didn't have much luck. But I'm probably doing something wrong because I'm a graphics noob. The machine normally uses CONFIG_DRM_RADEON, so I turned that off, and turned DRM_OFDRM on. When I boot I get boot messages but only one screen worth, the messages don't scroll at all, which is unusual. But I'm not sure if that's related to ofdrm or something else. The machine does come up, I can login via SSH. Is there some way to start X to exercise the driver from an SSH login? cheers
Hello, On Thu, Jul 28, 2022 at 09:13:59PM +1000, Michael Ellerman wrote: > Thomas Zimmermann <tzimmermann@suse.de> writes: > > (was: drm: Add driverof PowerPC OF displays) > > > > PowerPC's Open Firmware offers a simple display buffer for graphics > > output. Add ofdrm, a DRM driver for the device. As with the existing > > simpledrm driver, the graphics hardware is pre-initialized by the > > firmware. The driver only provides blitting, no actual DRM modesetting > > is possible. > > Hi Thomas, > > I tried to test this on a 32-bit ppc Mac Mini but didn't have much luck. > But I'm probably doing something wrong because I'm a graphics noob. > > The machine normally uses CONFIG_DRM_RADEON, so I turned that off, and > turned DRM_OFDRM on. > > When I boot I get boot messages but only one screen worth, the messages > don't scroll at all, which is unusual. But I'm not sure if that's > related to ofdrm or something else. A somewhat interesting datapoint might be how this works with offb. > The machine does come up, I can login via SSH. Is there some way to > start X to exercise the driver from an SSH login? The startx script provided by distribution usually works. It's basically a very convoluted way to do something like X :0& DISPLAY=:0 xterm& Thanks Michal
Hi Am 26.07.22 um 15:17 schrieb Javier Martinez Canillas: > Hello Thomas, > > On 7/20/22 16:27, Thomas Zimmermann wrote: >> Open Firmware provides basic display output via the 'display' node. >> DT platform code already provides a device that represents the node's >> framebuffer. Add a DRM driver for the device. The display mode and >> color format is pre-initialized by the system's firmware. Runtime >> modesetting via DRM is not possible. The display is useful during >> early boot stages or as error fallback. >> > > I'm not familiar with OF display but the driver looks good to me. > > Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> > > I just have a few questions below. > > [...] > >> +static int ofdrm_primary_plane_helper_atomic_check(struct drm_plane *plane, >> + struct drm_atomic_state *new_state) >> +{ >> + struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(new_state, plane); >> + struct drm_crtc_state *new_crtc_state; >> + int ret; >> + >> + if (!new_plane_state->fb) >> + return 0; >> + >> + new_crtc_state = drm_atomic_get_new_crtc_state(new_state, new_plane_state->crtc); >> + >> + ret = drm_atomic_helper_check_plane_state(new_plane_state, new_crtc_state, >> + DRM_PLANE_HELPER_NO_SCALING, >> + DRM_PLANE_HELPER_NO_SCALING, >> + false, false); >> + if (ret) >> + return ret; >> + >> + return 0; >> +} > > This seems to be exactly the same check than used in the simpledrm driver. > Maybe could be moved to the fwfb helper library too ? I've meanwhile dropped fwfb helpers. Color management requires specific code here, so there's no much to share anyway. > > [...] > >> + >> +static void ofdrm_crtc_helper_atomic_disable(struct drm_crtc *crtc, >> + struct drm_atomic_state *old_state) >> +{ >> + /* >> + * Always enabled; disabling clears the screen in the >> + * primary plane's atomic_disable function. >> + */ >> +} >> + > > Same comment than for simpledrm, are these no-op helpers really needed ? In simpledrm, I've meanwhile removed them. I'll do so here as well. > > [...] > >> +static const struct of_device_id ofdrm_of_match_display[] = { >> + { .compatible = "display", }, >> + { }, >> +}; >> +MODULE_DEVICE_TABLE(of, ofdrm_of_match_display); >> + > > I don't see a binding for this in Documentation/devicetree/bindings/display. > Do we need one or it's that only required for FDT and not Open Firmware DT ? > No idea. The device is being created in drivers/of/platform.c. If offb didn't need these bindings, ofdrm probably won't need them either. Best regards Thomas