> -----Original Message----- > From: Thierry Reding [mailto:thierry.reding@xxxxxxxxx] > Sent: Thursday, July 16, 2015 7:31 PM > To: Wang Jianwei-B52261 > Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux- > arm-kernel@xxxxxxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; > airlied@xxxxxxxx; daniel.vetter@xxxxxxxx; mark.yao@xxxxxxxxxxxxxx; Wood > Scott-B07421; Wang Huan-B18965; Xiubo Li > Subject: Re: [PATCH v9 1/4] drm/layerscape: Add Freescale DCU DRM driver > > On Thu, Jul 16, 2015 at 11:10:29AM +0000, Wang J.W. wrote: > [...] > > > -----Original Message----- > > > From: Thierry Reding [mailto:thierry.reding@xxxxxxxxx] > [...] > > > On Mon, Jul 13, 2015 at 06:51:29PM +0800, Jianwei Wang wrote: > [...] > > > > DRM DRIVERS FOR NVIDIA TEGRA > > > > M: Thierry Reding <thierry.reding@xxxxxxxxx> > > > > M: Terje Bergström <tbergstrom@xxxxxxxxxx> > > > > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig > > > > index c46ca31..9cfd14e 100644 > > > > --- a/drivers/gpu/drm/Kconfig > > > > +++ b/drivers/gpu/drm/Kconfig > > > > @@ -231,6 +231,8 @@ source "drivers/gpu/drm/virtio/Kconfig" > > > > > > > > source "drivers/gpu/drm/msm/Kconfig" > > > > > > > > +source "drivers/gpu/drm/fsl-dcu/Kconfig" > > > > + > > > > > > As mentioned in a different email, I'm somewhat annoyed by the > > > random placement of these source statements. But we can probably > > > clean that up in one go and insist on proper ordering when there is > one. > > > > > > > Ok, I plan to move it to the last line. Is it ok? > > It doesn't really matter, it will be inconsistent no matter what because > the current ordering isn't consistent. Keep it where it is for now. I'll > prepare a set of patches to get some consistency into this file. > > > > > +int fsl_dcu_drm_connector_create(struct fsl_dcu_drm_device *fsl_dev, > > > > + struct drm_encoder *encoder) { > > > > + struct drm_connector *connector = &fsl_dev- > >connector.connector; > > > > + struct device_node *panel_node; > > > > + int ret; > > > > + > > > > + fsl_dev->connector.encoder = encoder; > > > > > > Why do you need this? The DRM core should set this for you when > > > setting the initial configuration. > > > > > > > This connector is a private structure fsl_dcu_drm_connector. I set it > > for select best encoder. > > That doesn't sound right. Technically the DRM core or userspace is going > to select the encoder for your connector, so it'd be better to derive the > pointer to your driver-private structure from struct drm_encoder *, > otherwise you'll end up getting you private copy of the assignment out of > sync with what the DRM core and/or userspace set up. > Maybe this is a misunderstanding. fsl_dev->connector.encoder = encoder; In this sentence fsl_dev and connector are all driver-private structures Fsl_dev define: 177 struct fsl_dcu_drm_device { 178 struct device *dev; 179 struct device_node *np; 180 struct regmap *regmap; 181 int irq; 182 struct clk *clk; 183 /*protects hardware register*/ 184 spinlock_t irq_lock; 185 struct drm_device *drm; 186 struct drm_fbdev_cma *fbdev; 187 struct drm_crtc crtc; 188 struct drm_encoder encoder; 189 struct fsl_dcu_drm_connector connector; connector is here 190 }; fsl_dcu_drm_connector define: 15 struct fsl_dcu_drm_connector { 16 struct drm_connector base; 17 struct drm_encoder *encoder; 18 struct drm_panel *panel; 19 }; And this will be used in .best_encoder hooker 91 static struct drm_encoder * 92 fsl_dcu_drm_connector_best_encoder(struct drm_connector *connector) 93 { 94 struct fsl_dcu_drm_connector *fsl_con = to_fsl_dcu_connector(connector); 95 96 return fsl_con->encoder; 97 } I see some other also do it like this. Is it ok? > Note that you might not run into problems now because you only have a > single encoder. But if you ever add support for another you're going to > run into problems with this kind of static assignment. > > > > > + dev->irq_enabled = true; > > > > + dev->vblank_disable_allowed = true; > > > > + > > > > + ret = regmap_write(fsl_dev->regmap, DCU_INT_STATUS, 0); > > > > + if (ret) > > > > + dev_err(&pdev->dev, "set DCU_INT_STATUS failed\n"); > > > > + ret = regmap_read(fsl_dev->regmap, DCU_INT_MASK, &int_mask); > > > > + if (ret) > > > > + dev_err(&pdev->dev, "read DCU_INT_MASK failed\n"); > > > > + ret = regmap_write(fsl_dev->regmap, DCU_INT_MASK, int_mask & > > > > + ~DCU_INT_MASK_VBLANK); > > > > + if (ret) > > > > + dev_err(&pdev->dev, "set DCU_INT_MASK failed\n"); > > > > + ret = regmap_write(fsl_dev->regmap, DCU_UPDATE_MODE, > > > > + DCU_UPDATE_MODE_READREG); > > > > > > What's this DCU_UPDATE_MODE_READREG bit? > > > > > > > In fact, Only when setting DCU_UPDATE_MODE_READREG bit, it begin to > > transfer register values. So setting DCU_UPDATE_MODE_READREG bit is a > > must after registers updating. > > Okay, I see. That's going to be convenient for atomic updates. > > > > > diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h > > > > b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h > > > [...] > > > > +#define DCU_DISP_SIZE 0x0018 > > > > +#define DCU_DISP_SIZE_DELTA_Y(x) ((x) << 16) > > > > +/*Regisiter value 1/16 of horizontal resolution*/ > > > > +#define DCU_DISP_SIZE_DELTA_X(x) ((x) >> 4) > > > > > > Does this mean the display controller only supports horizontal > > > resolutions that are a multiple of 16? > > > > > > > Yes. > > You may want to check for this explicitly in the driver and filter out > modes that you don't support. Ok, I'll add mode check in connector_helper_funcs.mode_valid > > > > > +#ifdef CONFIG_SOC_VF610 > > > > +#define DCU_TOTAL_LAYER_NUM 64 > > > > +#define DCU_LAYER_NUM_MAX 6 > > > > +#else > > > > +#define DCU_TOTAL_LAYER_NUM 16 > > > > +#define DCU_LAYER_NUM_MAX 4 > > > > +#endif > > > > > > Should this not be a runtime decision so that the same driver can > > > run on > > > VF610 and LS1021A platforms? > > > > > > > Yes, Both VF610 and LS1021A use DCU as video IP module. > > And there are a bit of differences on each SoCs. > > Yes, I understand. This should be covered by SoC-specific parameters (via > the of_device_id table) so that you can run the same kernel on both > VF610 and LS1021A SoCs. > > As it is, if you build this on a configuration where both VF610 and > LS1021A are enabled you're going to pick up the values for VF610 and cause > LS1021A to not work. Very good idea, I'll do it > > > > > +void fsl_dcu_drm_plane_destroy(struct drm_plane *plane) { > > > > + fsl_dcu_drm_plane_disable(plane); > > > > > > Hmm? This function doesn't do anything, so why not just drop it? > > > > > > > > > I'll implement fsl_dcu_drm_plane_disable(plane) > > > > > > > > > > + drm_plane_cleanup(plane); > > > > +} > > > > > > Also this function and ->atomic_update() should be static. Perhaps > > > make it a habit of running your build tests with C=1 occasionally to > > > get notified of this kind of error. > > > > > > Thierry > > > > > > > > One question: How can I set C=1? > > Just add it to the make command-line along with any other parameters, like > this for example: > > $ make ARCH=arm CROSS_COMPILE=armv7l-unknown-linux-gnueabihf- \ > O=build/vf610 C=1 > > Thierry Thanks Jianwei _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
