Re: [PATCH 03/22] drm/arc: Use drm_fb_cma_fbdev_init/fini()

[Noralf Trønnes <noralf@xxxxxxxxxxx>]

Den 06.11.2017 10.08, skrev Daniel Vetter:
> On Sat, Nov 04, 2017 at 02:03:57PM +0100, Noralf Trønnes wrote:
> > Use drm_fb_cma_fbdev_init() and drm_fb_cma_fbdev_fini() which relies on
> > the fact that drm_device holds a pointer to the drm_fb_helper structure.
> > This means that the driver doesn't have to keep track of that.
> > Also use the drm_fb_helper functions directly.
> > Remove unused function prototype arcpgu_fbdev_cma_init().
> >
> > Cc: Alexey Brodkin <abrodkin@xxxxxxxxxxxx>
> > Signed-off-by: Noralf Trønnes <noralf@xxxxxxxxxxx>
> > ---
> >   drivers/gpu/drm/arc/arcpgu.h     |  4 ----
> >   drivers/gpu/drm/arc/arcpgu_drv.c | 36 +++++++-----------------------------
> >   2 files changed, 7 insertions(+), 33 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/arc/arcpgu.h b/drivers/gpu/drm/arc/arcpgu.h
> > index e8fcf3ab1d9a..90ef76b19f8a 100644
> > --- a/drivers/gpu/drm/arc/arcpgu.h
> > +++ b/drivers/gpu/drm/arc/arcpgu.h
> > @@ -20,7 +20,6 @@
> >   struct arcpgu_drm_private {
> >   	void __iomem		*regs;
> >   	struct clk		*clk;
> > -	struct drm_fbdev_cma	*fbdev;
> >   	struct drm_framebuffer	*fb;
> >   	struct drm_crtc		crtc;
> >   	struct drm_plane	*plane;
> > @@ -43,8 +42,5 @@ static inline u32 arc_pgu_read(struct arcpgu_drm_private *arcpgu,
> >   int arc_pgu_setup_crtc(struct drm_device *dev);
> >   int arcpgu_drm_hdmi_init(struct drm_device *drm, struct device_node *np);
> >   int arcpgu_drm_sim_init(struct drm_device *drm, struct device_node *np);
> > -struct drm_fbdev_cma *arcpgu_fbdev_cma_init(struct drm_device *dev,
> > -	unsigned int preferred_bpp, unsigned int num_crtc,
> > -	unsigned int max_conn_count);
> >   
> >   #endif
> > diff --git a/drivers/gpu/drm/arc/arcpgu_drv.c b/drivers/gpu/drm/arc/arcpgu_drv.c
> > index 074fd4ea7ece..f54481ee834c 100644
> > --- a/drivers/gpu/drm/arc/arcpgu_drv.c
> > +++ b/drivers/gpu/drm/arc/arcpgu_drv.c
> > @@ -16,6 +16,7 @@
> >   
> >   #include <linux/clk.h>
> >   #include <drm/drm_crtc_helper.h>
> > +#include <drm/drm_fb_helper.h>
> >   #include <drm/drm_fb_cma_helper.h>
> >   #include <drm/drm_gem_cma_helper.h>
> >   #include <drm/drm_gem_framebuffer_helper.h>
> > @@ -25,16 +26,9 @@
> >   #include "arcpgu.h"
> >   #include "arcpgu_regs.h"
> >   
> > -static void arcpgu_fb_output_poll_changed(struct drm_device *dev)
> > -{
> > -	struct arcpgu_drm_private *arcpgu = dev->dev_private;
> > -
> > -	drm_fbdev_cma_hotplug_event(arcpgu->fbdev);
> > -}
> > -
> >   static const struct drm_mode_config_funcs arcpgu_drm_modecfg_funcs = {
> >   	.fb_create  = drm_gem_fb_create,
> > -	.output_poll_changed = arcpgu_fb_output_poll_changed,
> > +	.output_poll_changed = drm_fb_helper_output_poll_changed,
> >   	.atomic_check = drm_atomic_helper_check,
> >   	.atomic_commit = drm_atomic_helper_commit,
> >   };
> > @@ -51,13 +45,6 @@ static void arcpgu_setup_mode_config(struct drm_device *drm)
> >   
> >   DEFINE_DRM_GEM_CMA_FOPS(arcpgu_drm_ops);
> >   
> > -static void arcpgu_lastclose(struct drm_device *drm)
> > -{
> > -	struct arcpgu_drm_private *arcpgu = drm->dev_private;
> > -
> > -	drm_fbdev_cma_restore_mode(arcpgu->fbdev);
> > -}
> > -
> >   static int arcpgu_load(struct drm_device *drm)
> >   {
> >   	struct platform_device *pdev = to_platform_device(drm->dev);
> > @@ -113,13 +100,9 @@ static int arcpgu_load(struct drm_device *drm)
> >   	drm_mode_config_reset(drm);
> >   	drm_kms_helper_poll_init(drm);
> >   
> > -	arcpgu->fbdev = drm_fbdev_cma_init(drm, 16,
> > -					   drm->mode_config.num_connector);
> > -	if (IS_ERR(arcpgu->fbdev)) {
> > -		ret = PTR_ERR(arcpgu->fbdev);
> > -		arcpgu->fbdev = NULL;
> > -		return -ENODEV;
> > -	}
> > +	ret = drm_fb_cma_fbdev_init(drm, 16, 0);
> s/0/NULL/
>
> Probably applies to other patches too.
>
> Also, why did you merge this new change into the old patch with the
> output_poll/lastclose change? It took me a while to figure out what you're
> doing, because the patches looked familiar, but the cover letter didn't
> mention how stuff evolved, nor the patches here.
>
> Also, you've lost the bunch of acks/r-bs you've gathered already. If
> there's no technical reason to merge the 2 patches I'm kinda leaning
> towards merging them separately, at least the ones that have acks already.
> Just to avoid the pitfall of the continually evolving patch series that
> never lands.

Ah, now I understand some of the confusion.

I have one patchset that converts the drivers that don't use the cma helper:
drm/fb-helper: Add .last_close and .output_poll_changed helpers

And in this patchset I remove the use of struct drm_fbdev_cma which means
I move away from the all the drm_fbdev_cma* functions. This means switching
to the .last_close and .output_poll_changed helpers from the previous
patchset.

Not sure how I can un-confuse this.

Noralf.

> -Daniel
>
> > +	if (ret)
> > +		return ret;
> >   
> >   	platform_set_drvdata(pdev, drm);
> >   	return 0;
> > @@ -127,12 +110,7 @@ static int arcpgu_load(struct drm_device *drm)
> >   
> >   static int arcpgu_unload(struct drm_device *drm)
> >   {
> > -	struct arcpgu_drm_private *arcpgu = drm->dev_private;
> > -
> > -	if (arcpgu->fbdev) {
> > -		drm_fbdev_cma_fini(arcpgu->fbdev);
> > -		arcpgu->fbdev = NULL;
> > -	}
> > +	drm_fb_cma_fbdev_fini(drm);
> >   	drm_kms_helper_poll_fini(drm);
> >   	drm_mode_config_cleanup(drm);
> >   
> > @@ -168,7 +146,7 @@ static int arcpgu_debugfs_init(struct drm_minor *minor)
> >   static struct drm_driver arcpgu_drm_driver = {
> >   	.driver_features = DRIVER_MODESET | DRIVER_GEM | DRIVER_PRIME |
> >   			   DRIVER_ATOMIC,
> > -	.lastclose = arcpgu_lastclose,
> > +	.lastclose = drm_fb_helper_lastclose,
> >   	.name = "arcpgu",
> >   	.desc = "ARC PGU Controller",
> >   	.date = "20160219",
> > --
> > 2.14.2
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel

_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel