Re: [Intel-gfx] [PATCH v3 04/27] drm: Don't test for IRQ support in VBLANK ioctls
On Thu, Jun 24, 2021 at 11:07:57AM +0200, Thomas Zimmermann wrote:
> Hi
>
> Am 24.06.21 um 10:51 schrieb Jani Nikula:
> > On Thu, 24 Jun 2021, Thomas Zimmermann wrote:
> > > Hi
> > >
> > > Am 24.06.21 um 10:06 schrieb Jani Nikula:
> > > > On Thu, 24 Jun 2021, Thomas Zimmermann wrote:
> > > > > diff --git a/drivers/gpu/drm/drm_vblank.c
> > > > > b/drivers/gpu/drm/drm_vblank.c
> > > > > index 3417e1ac7918..10fe16bafcb6 100644
> > > > > --- a/drivers/gpu/drm/drm_vblank.c
> > > > > +++ b/drivers/gpu/drm/drm_vblank.c
> > > > > @@ -1748,8 +1748,16 @@ int drm_wait_vblank_ioctl(struct drm_device
> > > > > *dev, void *data,
> > > > > unsigned int pipe_index;
> > > > > unsigned int flags, pipe, high_pipe;
> > > > > - if (!dev->irq_enabled)
> > > > > - return -EOPNOTSUPP;
> > > > > +#if defined(CONFIG_DRM_LEGACY)
> > > > > + if (unlikely(drm_core_check_feature(dev, DRIVER_LEGACY))) {
> > > > > + if (!dev->irq_enabled)
> > > > > + return -EOPNOTSUPP;
> > > > > + } else /* if DRIVER_MODESET */
> > > > > +#endif
> > > > > + {
> > > > > + if (!drm_dev_has_vblank(dev))
> > > > > + return -EOPNOTSUPP;
> > > > > + }
> > > >
> > > > Sheesh I hate this kind of inline #ifdefs.
> > > >
> > > > Two alternate suggestions that I believe should be as just efficient:
> > >
> > > Or how about:
> > >
> > > static bool drm_wait_vblank_supported(struct drm_device *dev)
> > >
> > > {
> > >
> > > if defined(CONFIG_DRM_LEGACY)
> > > if (unlikely(drm_core_check_feature(dev, DRIVER_LEGACY)))
> > >
> > > return dev->irq_enabled;
> > >
> > > #endif
> > > return drm_dev_has_vblank(dev);
> > >
> > > }
> > >
> > >
> > > ?
> > >
> > > It's inline, but still readable.
> >
> > It's definitely better than the original, but it's unclear to me why
> > you'd prefer this over option 2) below. I guess the only reason I can
> > think of is emphasizing the conditional compilation. However,
> > IS_ENABLED() is widely used in this manner specifically to avoid inline
> > #if, and the compiler optimizes it away.
>
> It's simply more readable to me as the condition is simpler. But option 2 is
> also ok.
Perhaps do something like this, then:
if (IS_ENABLED(CONFIG_DRM_LEGACY)) {
if (unlikely(drm_core_check_feature(dev, DRIVER_LEGACY)))
return dev->irq_enabled;
}
return drm_dev_has_vblank(dev);
That's about just as readable as the variant involving the preprocessor
but has all the benefits of not using the preprocessor.
Thierry
signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3 21/27] drm/tegra: Don't set struct drm_device.irq_enabled
On Thu, Jun 24, 2021 at 09:29:10AM +0200, Thomas Zimmermann wrote: > The field drm_device.irq_enabled is only used by legacy drivers > with userspace modesetting. Don't set it in tegra. > > Signed-off-by: Thomas Zimmermann > Reviewed-by: Laurent Pinchart > Acked-by: Daniel Vetter > --- > drivers/gpu/drm/tegra/drm.c | 7 --- > 1 file changed, 7 deletions(-) Acked-by: Thierry Reding signature.asc Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 02/20] drm/tegra: Don't register DP AUX channels before connectors
On Fri, Mar 26, 2021 at 04:37:49PM -0400, Lyude Paul wrote: > As pointed out by the documentation for drm_dp_aux_register(), > drm_dp_aux_init() should be used in situations where the AUX channel for a > display driver can potentially be registered before it's respective DRM > driver. This is the case with Tegra, since the DP aux channel exists as a > platform device instead of being a grandchild of the DRM device. > > Since we're about to add a backpointer to a DP AUX channel's respective DRM > device, let's fix this so that we don't potentially allow userspace to use > the AUX channel before we've associated it with it's DRM connector. > > Signed-off-by: Lyude Paul > --- > drivers/gpu/drm/tegra/dpaux.c | 11 ++----- > 1 file changed, 6 insertions(+), 5 deletions(-) Acked-by: Thierry Reding signature.asc Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 05/20] drm/dp: Add backpointer to drm_device in drm_dp_aux
On Fri, Mar 26, 2021 at 04:37:52PM -0400, Lyude Paul wrote: > This is something that we've wanted for a while now: the ability to > actually look up the respective drm_device for a given drm_dp_aux struct. > This will also allow us to transition over to using the drm_dbg_*() helpers > for debug message printing, as we'll finally have a drm_device to reference > for doing so. > > Note that there is one limitation with this - because some DP AUX adapters > exist as platform devices which are initialized independently of their > respective DRM devices, one cannot rely on drm_dp_aux->drm_dev to always be > non-NULL until drm_dp_aux_register() has been called. We make sure to point > this out in the documentation for struct drm_dp_aux. > > Signed-off-by: Lyude Paul > --- [...] > drivers/gpu/drm/tegra/dpaux.c | 1 + [...] Acked-by: Thierry Reding signature.asc Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] pwm: Rename pwm_get_state() to better reflect its semantic
On Tue, Apr 06, 2021 at 09:30:36AM +0200, Uwe Kleine-König wrote: > Given that lowlevel drivers usually cannot implement exactly what a > consumer requests with pwm_apply_state() there is some rounding involved. > > pwm_get_state() traditionally returned the setting that was requested most > recently by the consumer (opposed to what was actually implemented in > hardware in reply to the last request). To make this semantic obvious > rename the function. > > Signed-off-by: Uwe Kleine-König > --- > Documentation/driver-api/pwm.rst | 6 +++- > drivers/clk/clk-pwm.c | 2 +- > drivers/gpu/drm/i915/display/intel_panel.c | 4 +-- > drivers/input/misc/da7280.c| 2 +- > drivers/input/misc/pwm-beeper.c| 2 +- > drivers/input/misc/pwm-vibra.c | 4 +-- > drivers/pwm/core.c | 4 +-- > drivers/pwm/pwm-atmel-hlcdc.c | 2 +- > drivers/pwm/pwm-atmel.c| 2 +- > drivers/pwm/pwm-imx27.c| 2 +- > drivers/pwm/pwm-rockchip.c | 2 +- > drivers/pwm/pwm-stm32-lp.c | 4 +-- > drivers/pwm/pwm-sun4i.c| 2 +- > drivers/pwm/sysfs.c| 18 ++-- > drivers/regulator/pwm-regulator.c | 4 +-- > drivers/video/backlight/pwm_bl.c | 10 +++ > include/linux/pwm.h| 34 ++ > 17 files changed, 59 insertions(+), 45 deletions(-) Honestly, I don't think this is worth the churn. If you think people will easily get confused by this then a better solution might be to more explicitly document the pwm_get_state() function to say exactly what it returns. But there's no need to make life difficult for everyone by renaming this to something as cumbersome as this. Thierry signature.asc Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] pwm: Rename pwm_get_state() to better reflect its semantic
On Tue, Apr 06, 2021 at 03:43:56PM +0200, Uwe Kleine-König wrote: > Hello Thierry, > > On Tue, Apr 06, 2021 at 01:16:31PM +0200, Thierry Reding wrote: > > On Tue, Apr 06, 2021 at 09:30:36AM +0200, Uwe Kleine-König wrote: > > > Given that lowlevel drivers usually cannot implement exactly what a > > > consumer requests with pwm_apply_state() there is some rounding involved. > > > > > > pwm_get_state() traditionally returned the setting that was requested most > > > recently by the consumer (opposed to what was actually implemented in > > > hardware in reply to the last request). To make this semantic obvious > > > rename the function. > > > > > > Signed-off-by: Uwe Kleine-König > > > --- > > > Documentation/driver-api/pwm.rst | 6 +++- > > > drivers/clk/clk-pwm.c | 2 +- > > > drivers/gpu/drm/i915/display/intel_panel.c | 4 +-- > > > drivers/input/misc/da7280.c| 2 +- > > > drivers/input/misc/pwm-beeper.c| 2 +- > > > drivers/input/misc/pwm-vibra.c | 4 +-- > > > drivers/pwm/core.c | 4 +-- > > > drivers/pwm/pwm-atmel-hlcdc.c | 2 +- > > > drivers/pwm/pwm-atmel.c| 2 +- > > > drivers/pwm/pwm-imx27.c| 2 +- > > > drivers/pwm/pwm-rockchip.c | 2 +- > > > drivers/pwm/pwm-stm32-lp.c | 4 +-- > > > drivers/pwm/pwm-sun4i.c| 2 +- > > > drivers/pwm/sysfs.c| 18 ++-- > > > drivers/regulator/pwm-regulator.c | 4 +-- > > > drivers/video/backlight/pwm_bl.c | 10 +++ > > > include/linux/pwm.h| 34 ++ > > > 17 files changed, 59 insertions(+), 45 deletions(-) > > > > Honestly, I don't think this is worth the churn. If you think people > > will easily get confused by this then a better solution might be to more > > explicitly document the pwm_get_state() function to say exactly what it > > returns. > > I'm not so optimistic that people become aware of the semantic just > because there is documentation describing it and I strongly believe that > a good name for functions is more important than accurate documentation. > > If you don't agree, what do you think about the updated wording in > Documentation/driver-api/pwm.rst? Yeah, that clarifies this a bit. I can apply that hunk of the patch separately. > > But there's no need to make life difficult for everyone by > > renaming this to something as cumbersome as this. > > I don't expect any merge conflicts (and if still a problem occurs > resolving should be trivial enough). So I obviously don't agree to your > weighing. I wasn't talking about merge conflicts but instead about the extra churn of changing all consumers and having to type all these extra characters for no benefit. Thierry signature.asc Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 10/12] drm/tegra: Don't set allow_fb_modifiers explicitly
On Tue, Apr 13, 2021 at 11:49:01AM +0200, Daniel Vetter wrote:
> Since
>
> commit 890880ddfdbe256083170866e49c87618b706ac7
> Author: Paul Kocialkowski
> Date: Fri Jan 4 09:56:10 2019 +0100
>
> drm: Auto-set allow_fb_modifiers when given modifiers at plane init
>
> this is done automatically as part of plane init, if drivers set the
> modifier list correctly. Which is the case here.
>
> It was slightly inconsistently though, since planes with only linear
> modifier support haven't listed that explicitly. Fix that, and cc:
> stable to allow userspace to rely on this. Again don't backport
> further than where Paul's patch got added.
>
> Cc: sta...@vger.kernel.org # v5.1 +
> Cc: Pekka Paalanen
> Signed-off-by: Daniel Vetter
> Cc: Thierry Reding
> Cc: Jonathan Hunter
> Cc: linux-te...@vger.kernel.org
> ---
> drivers/gpu/drm/tegra/dc.c | 10 --
> drivers/gpu/drm/tegra/drm.c | 2 --
> 2 files changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
> index c9385cfd0fc1..f9845a50f866 100644
> --- a/drivers/gpu/drm/tegra/dc.c
> +++ b/drivers/gpu/drm/tegra/dc.c
> @@ -959,6 +959,11 @@ static const struct drm_plane_helper_funcs
> tegra_cursor_plane_helper_funcs = {
> .atomic_disable = tegra_cursor_atomic_disable,
> };
>
> +static const uint64_t linear_modifiers[] = {
> + DRM_FORMAT_MOD_LINEAR,
> + DRM_FORMAT_MOD_INVALID
> +};
> +
> static struct drm_plane *tegra_dc_cursor_plane_create(struct drm_device *drm,
> struct tegra_dc *dc)
> {
> @@ -987,7 +992,7 @@ static struct drm_plane
> *tegra_dc_cursor_plane_create(struct drm_device *drm,
>
> err = drm_universal_plane_init(drm, &plane->base, possible_crtcs,
> &tegra_plane_funcs, formats,
> -num_formats, NULL,
> +num_formats, linear_modifiers,
> DRM_PLANE_TYPE_CURSOR, NULL);
> if (err < 0) {
> kfree(plane);
> @@ -1106,7 +,8 @@ static struct drm_plane
> *tegra_dc_overlay_plane_create(struct drm_device *drm,
>
> err = drm_universal_plane_init(drm, &plane->base, possible_crtcs,
> &tegra_plane_funcs, formats,
> -num_formats, NULL, type, NULL);
> +num_formats, linear_modifiers,
> + type, NULL);
I think we can do better than linear_modifiers for overlay planes, but
given that this doesn't change existing behaviour, I'll do that in a
separate patch.
Acked-by: Thierry Reding
signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 03/30] drm/tegra: Don't register DP AUX channels before connectors
On Fri, Feb 19, 2021 at 04:52:59PM -0500, Lyude Paul wrote: > As pointed out by the documentation for drm_dp_aux_register(), > drm_dp_aux_init() should be used in situations where the AUX channel for a > display driver can potentially be registered before it's respective DRM > driver. This is the case with Tegra, since the DP aux channel exists as a > platform device instead of being a grandchild of the DRM device. > > Since we're about to add a backpointer to a DP AUX channel's respective DRM > device, let's fix this so that we don't potentially allow userspace to use > the AUX channel before we've associated it with it's DRM connector. > > Signed-off-by: Lyude Paul > --- > drivers/gpu/drm/tegra/dpaux.c | 11 ++- > 1 file changed, 6 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/tegra/dpaux.c b/drivers/gpu/drm/tegra/dpaux.c > index 105fb9cdbb3b..ea56c6ec25e4 100644 > --- a/drivers/gpu/drm/tegra/dpaux.c > +++ b/drivers/gpu/drm/tegra/dpaux.c > @@ -534,9 +534,7 @@ static int tegra_dpaux_probe(struct platform_device *pdev) > dpaux->aux.transfer = tegra_dpaux_transfer; > dpaux->aux.dev = &pdev->dev; > > - err = drm_dp_aux_register(&dpaux->aux); > - if (err < 0) > - return err; > + drm_dp_aux_init(&dpaux->aux); I just noticed that this change causes an error on some setups that I haven't seen before. The problem is that the SOR driver tries to grab a reference to the I2C device to make sure it doesn't go away while it has a pointer to it. However, since now the I2C adapter hasn't been registered yet, I get this: [ 15.013969] kobject: '(null)' (5c903e43): is not initialized, yet kobject_get() is being called. I recall that you wanted to make this change so that a backpointer to the DRM device could be added (I think that's patch 15 of the series), but I didn't see that patch get merged, so it's a bit difficult to try and fix this up. Has the situation changed? Do we no longer need the backpointer? If we still want it, what's the plan for merging the change? Should I work under the assumption that patch will make it in sometime and try to fix this on top of that? I'm thinking that perhaps we can move the I2C adapter registration into drm_dp_aux_init() since that's independent of the DRM device. It would also make a bit more sense from the Tegra driver's point of view where all devices would be created during the ->probe() path, and only during the ->init() path would the connection between DRM device and DRM DP AUX device be established. Thierry signature.asc Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3 03/20] drm/dp: Move i2c init to drm_dp_aux_init, add __must_check and fini
On Thu, Apr 22, 2021 at 01:18:09PM -0400, Lyude Paul wrote: > On Tue, 2021-04-20 at 02:16 +0300, Ville Syrjälä wrote: > > > > The init vs. register split is intentional. Registering the thing > > and allowing userspace access to it before the rest of the driver > > is ready isn't particularly great. For a while now we've tried to > > move towards an architecture where the driver is fully initialzied > > before anything gets exposed to userspace. > > Yeah-thank you for pointing this out. Thierry - do you think there's an > alternate solution we could go with in Tegra to fix the get_device() issue > that wouldn't require us trying to expose the i2c adapter early? I suppose we could do it in a hackish way that grabs a reference to the I2C adapter only upon registration. We can't do that for the regular I2C DDC case where the I2C controller is an external one because by the time we get to registration it could've gone again. This would make both code paths asymmetric, so I'd prefer not to do it. Perhaps it could serve as an stop-gap solution until something better is in place, though. Thierry signature.asc Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3 03/20] drm/dp: Move i2c init to drm_dp_aux_init, add __must_check and fini
On Thu, Apr 22, 2021 at 06:33:44PM -0400, Lyude Paul wrote: > OK - talked with Ville a bit on this and did some of my own research, I > actually think that moving i2c to drm_dp_aux_init() is the right decision for > the time being. The reasoning behind this being that as shown by my previous > work of fixing drivers that call drm_dp_aux_register() too early - it seems > like there's already been drivers that have been working just fine with > setting up the i2c device before DRM registration. > > In the future, it'd probably be better if we can split up i2c_add_adapter() > into an init and register function - but we'll have to talk with the i2c > maintainers to see if this is acceptable w/ them Yeah, that sounds like a better long-term solution. We could leave i2c_add_adapter() in place, since it's already half-way split up into some initialization code and i2c_register_adapter(), so it shouldn't be all that difficult to split out an i2c_init_adapter() so that outside users can do the split setup. Thierry signature.asc Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3 03/20] drm/dp: Move i2c init to drm_dp_aux_init, add __must_check and fini
On Fri, Apr 23, 2021 at 12:11:06AM -0400, Lyude Paul wrote: > On Thu, 2021-04-22 at 18:33 -0400, Lyude Paul wrote: > > OK - talked with Ville a bit on this and did some of my own research, I > > actually think that moving i2c to drm_dp_aux_init() is the right decision > > for > > the time being. The reasoning behind this being that as shown by my previous > > work of fixing drivers that call drm_dp_aux_register() too early - it seems > > like there's already been drivers that have been working just fine with > > setting up the i2c device before DRM registration. > > > > In the future, it'd probably be better if we can split up i2c_add_adapter() > > into an init and register function - but we'll have to talk with the i2c > > maintainers to see if this is acceptable w/ them > > Actually - I think adding the ability to refcount dp aux adapters might be a > better solution so I'm going to try that! I'm curious: how is a DP AUX adapter reference count going to solve the issue of potentially registering devices too early (i.e. before the DRM is registered)? Is it because registering too early could cause a reference count problem if somebody get a hold of the DP AUX adapter before the parent DRM device is around? Thierry signature.asc Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 22/26] drm/qxl: Use drm_fb_helper_fill_info
On Thu, Jan 24, 2019 at 05:58:27PM +0100, Daniel Vetter wrote: > Another driver that didn't set fbinfo->fix.id before. > > Signed-off-by: Daniel Vetter > Cc: Thierry Reding > Cc: Jonathan Hunter > Cc: linux-te...@vger.kernel.org > --- > drivers/gpu/drm/tegra/fb.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) In the subject: drm/qxl -> drm/tegra, with that: Acked-by: Thierry Reding signature.asc Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 26/26] drm/: Don't set FBINFO_(FLAG_)DEFAULT
On Thu, Jan 24, 2019 at 05:58:31PM +0100, Daniel Vetter wrote: > It's 0. > > Signed-off-by: Daniel Vetter > Cc: Inki Dae > Cc: Joonyoung Shim > Cc: Seung-Woo Kim > Cc: Kyungmin Park > Cc: Kukjin Kim > Cc: Krzysztof Kozlowski > Cc: Patrik Jakobsson > Cc: Ben Skeggs > Cc: Sandy Huang > Cc: "Heiko Stübner" > Cc: Thierry Reding > Cc: Jonathan Hunter > Cc: Hans de Goede > Cc: Greg Kroah-Hartman > Cc: Daniel Vetter > Cc: Bartlomiej Zolnierkiewicz > Cc: Alexander Kapshuk > Cc: linux-arm-ker...@lists.infradead.org > Cc: linux-samsung-...@vger.kernel.org > Cc: nouv...@lists.freedesktop.org > Cc: linux-rockc...@lists.infradead.org > Cc: linux-te...@vger.kernel.org > --- > drivers/gpu/drm/exynos/exynos_drm_fbdev.c | 1 - > drivers/gpu/drm/gma500/framebuffer.c | 1 - > drivers/gpu/drm/nouveau/nouveau_fbcon.c | 4 ++-- > drivers/gpu/drm/rockchip/rockchip_drm_fbdev.c | 1 - > drivers/gpu/drm/tegra/fb.c| 1 - > 5 files changed, 2 insertions(+), 6 deletions(-) Acked-by: Thierry Reding signature.asc Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 14/17] drm/tegra: Convert to using __drm_atomic_helper_crtc_reset() for reset.
On Fri, Mar 01, 2019 at 01:56:24PM +0100, Maarten Lankhorst wrote:
> Convert tegra to using __drm_atomic_helper_crtc_reset(), instead of
> writing its own version. Instead of open coding destroy_state(),
> call it directly for freeing the old state.
>
> Signed-off-by: Maarten Lankhorst
> Cc: Thierry Reding
> Cc: Jonathan Hunter
> Cc: linux-te...@vger.kernel.org
> ---
> drivers/gpu/drm/tegra/dc.c | 30 +++---
> 1 file changed, 11 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
> index 607a6ea17ecc..57c88d78cdaa 100644
> --- a/drivers/gpu/drm/tegra/dc.c
> +++ b/drivers/gpu/drm/tegra/dc.c
> @@ -1153,25 +1153,6 @@ static void tegra_dc_destroy(struct drm_crtc *crtc)
> drm_crtc_cleanup(crtc);
> }
>
> -static void tegra_crtc_reset(struct drm_crtc *crtc)
> -{
> - struct tegra_dc_state *state;
> -
> - if (crtc->state)
> - __drm_atomic_helper_crtc_destroy_state(crtc->state);
> -
> - kfree(crtc->state);
> - crtc->state = NULL;
> -
> - state = kzalloc(sizeof(*state), GFP_KERNEL);
> - if (state) {
> - crtc->state = &state->base;
> - crtc->state->crtc = crtc;
> - }
> -
> - drm_crtc_vblank_reset(crtc);
> -}
> -
> static struct drm_crtc_state *
> tegra_crtc_atomic_duplicate_state(struct drm_crtc *crtc)
> {
> @@ -1198,6 +1179,17 @@ static void tegra_crtc_atomic_destroy_state(struct
> drm_crtc *crtc,
> kfree(state);
> }
>
> +static void tegra_crtc_reset(struct drm_crtc *crtc)
> +{
> + struct tegra_dc_state *state = kzalloc(sizeof(*state), GFP_KERNEL);
> +
> + if (crtc->state)
> + tegra_crtc_atomic_destroy_state(crtc, crtc->state);
> +
> + __drm_atomic_helper_crtc_reset(crtc, &state->base);
> + drm_crtc_vblank_reset(crtc);
> +}
> +
I would preferred a predeclaration of tegra_crtc_atomic_destroy_state()
in this case because the implementations are in the same order as their
use in tegra_crtc_funcs, but I think I can live with it, so either way:
Acked-by: Thierry Reding
signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 7/7] drm: Split out drm_probe_helper.h
/gpu/drm/rockchip/rockchip_drm_fbdev.c | 2 +- > drivers/gpu/drm/rockchip/rockchip_drm_psr.c | 2 +- > drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 2 +- > drivers/gpu/drm/rockchip/rockchip_lvds.c | 2 +- > drivers/gpu/drm/rockchip/rockchip_rgb.c | 2 +- > drivers/gpu/drm/sti/sti_crtc.c| 2 +- > drivers/gpu/drm/sti/sti_drv.c | 2 +- > drivers/gpu/drm/sti/sti_dvo.c | 2 +- > drivers/gpu/drm/sti/sti_hda.c | 2 +- > drivers/gpu/drm/sti/sti_hdmi.c| 2 +- > drivers/gpu/drm/sti/sti_tvout.c | 2 +- > drivers/gpu/drm/stm/drv.c | 2 +- > drivers/gpu/drm/stm/ltdc.c| 2 +- > drivers/gpu/drm/sun4i/sun4i_backend.c | 2 +- > drivers/gpu/drm/sun4i/sun4i_crtc.c| 2 +- > drivers/gpu/drm/sun4i/sun4i_drv.c | 2 +- > drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c| 2 +- > drivers/gpu/drm/sun4i/sun4i_lvds.c| 2 +- > drivers/gpu/drm/sun4i/sun4i_rgb.c | 2 +- > drivers/gpu/drm/sun4i/sun4i_tcon.c| 2 +- > drivers/gpu/drm/sun4i/sun4i_tv.c | 2 +- > drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c| 2 +- > drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c | 2 +- > drivers/gpu/drm/sun4i/sun8i_mixer.c | 2 +- > drivers/gpu/drm/sun4i/sun8i_ui_layer.c| 2 +- > drivers/gpu/drm/sun4i/sun8i_vi_layer.c| 2 +- > drivers/gpu/drm/tegra/drm.h | 2 +- > drivers/gpu/drm/tegra/hdmi.c | 2 +- > drivers/gpu/drm/tegra/hub.c | 2 +- > drivers/gpu/drm/tinydrm/core/tinydrm-core.c | 2 +- > drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c | 2 +- > drivers/gpu/drm/tve200/tve200_drv.c | 2 +- > drivers/gpu/drm/udl/udl_connector.c | 1 + > drivers/gpu/drm/udl/udl_drv.c | 1 + > drivers/gpu/drm/udl/udl_main.c| 1 + > drivers/gpu/drm/vc4/vc4_crtc.c| 2 +- > drivers/gpu/drm/vc4/vc4_dpi.c | 2 +- > drivers/gpu/drm/vc4/vc4_dsi.c | 2 +- > drivers/gpu/drm/vc4/vc4_hdmi.c| 2 +- > drivers/gpu/drm/vc4/vc4_kms.c | 2 +- > drivers/gpu/drm/vc4/vc4_txp.c | 2 +- > drivers/gpu/drm/vc4/vc4_vec.c | 2 +- > drivers/gpu/drm/virtio/virtgpu_display.c | 2 +- > drivers/gpu/drm/virtio/virtgpu_drv.h | 2 +- > drivers/gpu/drm/vkms/vkms_crtc.c | 2 +- > drivers/gpu/drm/vkms/vkms_drv.c | 2 +- > drivers/gpu/drm/vkms/vkms_output.c| 2 +- > drivers/gpu/drm/vmwgfx/vmwgfx_kms.h | 2 +- > drivers/gpu/drm/xen/xen_drm_front.c | 2 +- > drivers/gpu/drm/xen/xen_drm_front_conn.c | 2 +- > drivers/gpu/drm/xen/xen_drm_front_gem.c | 2 +- > drivers/gpu/drm/xen/xen_drm_front_kms.c | 2 +- > drivers/gpu/drm/zte/zx_drm_drv.c | 2 +- > drivers/gpu/drm/zte/zx_hdmi.c | 2 +- > drivers/gpu/drm/zte/zx_tvenc.c| 2 +- > drivers/gpu/drm/zte/zx_vga.c | 2 +- > drivers/gpu/drm/zte/zx_vou.c | 2 +- > drivers/staging/vboxvideo/vbox_irq.c | 2 +- > drivers/staging/vboxvideo/vbox_mode.c | 2 +- > include/drm/drm_crtc_helper.h | 16 -- > include/drm/drm_probe_helper.h| 50 +++ > 208 files changed, 256 insertions(+), 200 deletions(-) > create mode 100644 include/drm/drm_probe_helper.h Looks good to me: Acked-by: Thierry Reding signature.asc Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [Nouveau] [PATCH v6 08/17] drm/ttm: use gem vma_node
On Sat, Sep 07, 2019 at 09:58:46PM -0400, Ilia Mirkin wrote:
> On Wed, Aug 21, 2019 at 7:55 AM Thierry Reding
> wrote:
> >
> > On Wed, Aug 21, 2019 at 04:33:58PM +1000, Ben Skeggs wrote:
> > > On Wed, 14 Aug 2019 at 20:14, Gerd Hoffmann wrote:
> > > >
> > > > Hi,
> > > >
> > > > > > Changing the order doesn't look hard. Patch attached (untested,
> > > > > > have no
> > > > > > test hardware). But maybe I missed some detail ...
> > > > >
> > > > > I came up with something very similar by splitting up nouveau_bo_new()
> > > > > into allocation and initialization steps, so that when necessary the
> > > > > GEM
> > > > > object can be initialized in between. I think that's slightly more
> > > > > flexible and easier to understand than a boolean flag.
> > > >
> > > > Yes, that should work too.
> > > >
> > > > Acked-by: Gerd Hoffmann
> > > Acked-by: Ben Skeggs
> >
> > Thanks guys, applied to drm-misc-next.
>
> Hi Thierry,
>
> Initial investigations suggest that this commit currently in drm-next
>
> commit 019cbd4a4feb3aa3a917d78e7110e3011bbff6d5
> Author: Thierry Reding
> Date: Wed Aug 14 11:00:48 2019 +0200
>
> drm/nouveau: Initialize GEM object before TTM object
>
> breaks nouveau userspace which tries to allocate GEM objects with a
> non-page-aligned size. Previously nouveau_gem_new would just call
> nouveau_bo_init which would call nouveau_bo_fixup_align before
> initializing the GEM object. With this change, it is done after. What
> do you think -- OK to just move that bit of logic into the new
> nouveau_bo_alloc() (and make size/align be pointers so that they can
> be fixed up?)
Hi Ilia,
sorry, got side-tracked earlier and forgot to send this out. I'll turn
this into a proper patch, but if you manage to find the time to test
this while I work out the userspace issues that are preventing me from
testing this more thoroughly, that'd be great.
Thierry
--- >8 ---
diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c
b/drivers/gpu/drm/nouveau/nouveau_bo.c
index e918b437af17..7d5ede756711 100644
--- a/drivers/gpu/drm/nouveau/nouveau_bo.c
+++ b/drivers/gpu/drm/nouveau/nouveau_bo.c
@@ -186,8 +186,8 @@ nouveau_bo_fixup_align(struct nouveau_bo *nvbo, u32 flags,
}
struct nouveau_bo *
-nouveau_bo_alloc(struct nouveau_cli *cli, u64 size, u32 flags, u32 tile_mode,
-u32 tile_flags)
+nouveau_bo_alloc(struct nouveau_cli *cli, u64 *size, int *align, u32 flags,
+u32 tile_mode, u32 tile_flags)
{
struct nouveau_drm *drm = cli->drm;
struct nouveau_bo *nvbo;
@@ -195,8 +195,8 @@ nouveau_bo_alloc(struct nouveau_cli *cli, u64 size, u32
flags, u32 tile_mode,
struct nvif_vmm *vmm = cli->svm.cli ? &cli->svm.vmm : &cli->vmm.vmm;
int i, pi = -1;
- if (!size) {
- NV_WARN(drm, "skipped size %016llx\n", size);
+ if (!*size) {
+ NV_WARN(drm, "skipped size %016llx\n", *size);
return ERR_PTR(-EINVAL);
}
@@ -266,7 +266,7 @@ nouveau_bo_alloc(struct nouveau_cli *cli, u64 size, u32
flags, u32 tile_mode,
pi = i;
/* Stop once the buffer is larger than the current page size. */
- if (size >= 1ULL << vmm->page[i].shift)
+ if (*size >= 1ULL << vmm->page[i].shift)
break;
}
@@ -281,6 +281,8 @@ nouveau_bo_alloc(struct nouveau_cli *cli, u64 size, u32
flags, u32 tile_mode,
}
nvbo->page = vmm->page[pi].shift;
+ nouveau_bo_fixup_align(nvbo, flags, align, size);
+
return nvbo;
}
@@ -292,12 +294,11 @@ nouveau_bo_init(struct nouveau_bo *nvbo, u64 size, int
align, u32 flags,
size_t acc_size;
int ret;
- acc_size = ttm_bo_dma_acc_size(nvbo->bo.bdev, size, sizeof(*nvbo));
-
- nouveau_bo_fixup_align(nvbo, flags, &align, &size);
nvbo->bo.mem.num_pages = size >> PAGE_SHIFT;
nouveau_bo_placement_set(nvbo, flags, 0);
+ acc_size = ttm_bo_dma_acc_size(nvbo->bo.bdev, size, sizeof(*nvbo));
+
ret = ttm_bo_init(nvbo->bo.bdev, &nvbo->bo, size, type,
&nvbo->placement, align >> PAGE_SHIFT, false,
acc_size, sg, robj, nouveau_bo_del_ttm);
@@ -318,7 +319,8 @@ nouveau_bo_new(struct nouveau_cli *cli, u64 size, int align,
struct nouveau_bo *nvbo;
int ret;
- nvbo = nouveau_bo_alloc(cli, size, flags, tile_mode, tile_flags);
+ nvbo = nouveau_bo_alloc(cli, &size, &align, flags, t
Re: [Intel-gfx] [PATCH] video/hdmi: Fix AVI bar unpack
On Thu, Sep 19, 2019 at 04:28:53PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä
>
> The bar values are little endian, not big endian. The pack
> function did it right but the unpack got it wrong. Fix it.
>
> Cc: sta...@vger.kernel.org
> Cc: linux-me...@vger.kernel.org
> Cc: Martin Bugge
> Cc: Hans Verkuil
> Cc: Thierry Reding
> Cc: Mauro Carvalho Chehab
> Fixes: 2c676f378edb ("[media] hdmi: added unpack and logging functions for
> InfoFrames")
> Signed-off-by: Ville Syrjälä
> ---
> drivers/video/hdmi.c | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
Reviewed-by: Thierry Reding
signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 00/21] drm/dp: Various helper improvements and cleanups
On Mon, Sep 23, 2019 at 04:52:50PM +0300, Jani Nikula wrote: > On Fri, 20 Sep 2019, Thierry Reding wrote: > > On Mon, Sep 02, 2019 at 01:31:00PM +0200, Thierry Reding wrote: > >> From: Thierry Reding > >> > >> Hi, > >> > >> this series of patches improves the DP helpers a bit and cleans up some > >> inconsistencies along the way. > >> > >> v2 incorporates all review comments add collects Reviewed-bys from v1. > >> > >> Thierry > >> > >> Thierry Reding (21): > >> drm/dp: Sort includes alphabetically > >> drm/dp: Add missing kerneldoc for struct drm_dp_link > >> drm/dp: Add drm_dp_link_reset() implementation > >> drm/dp: Track link capabilities alongside settings > >> drm/dp: Turn link capabilities into booleans > >> drm/dp: Probe link using existing parsing helpers > >> drm/dp: Read fast training capability from link > >> drm/dp: Read TPS3 capability from sink > >> drm/dp: Read channel coding capability from sink > >> drm/dp: Read alternate scrambler reset capability from sink > >> drm/dp: Read eDP version from DPCD > >> drm/dp: Read AUX read interval from DPCD > >> drm/dp: Do not busy-loop during link training > >> drm/dp: Use drm_dp_aux_rd_interval() > >> drm/dp: Add helper to get post-cursor adjustments > >> drm/dp: Set channel coding on link configuration > >> drm/dp: Enable alternate scrambler reset when supported > >> drm/dp: Add drm_dp_link_choose() helper > >> drm/dp: Add support for eDP link rates > >> drm/dp: Remove a gratuituous blank line > >> drm/bridge: tc358767: Use DP nomenclature > > > > Anyone interested in reviewing these? > > Thierry, I don't quite know how to put this nicely, but I also don't > think it's nice to not reply at all. So I'll try to be fair but it'll be > blunt. Fair enough? Fair enough. > I've glanced over the series, already before you pinged for reviews. It > looks like you've put effort into it, and it all looks nice. However, it > does not look like we could use this in i915, without significant effort > both on top of this work and in i915. It does not feel like there's any > incentive for us to review this in detail. > > It also feels like there's an increasing disconnect between "small" and > "big" drivers (*) when it comes to handling DP link and training. It > scares me a bit that this work is being done on the terms of the "small" > drivers, and that later in time this might be considered the One True > Way of handling DP. I'm not sure I understand your concern here. The goal of the series is primarily to extend the existing support for DP. It follows the pattern established by existing helpers and then goes one step further and provides some common way of actually storing the values that are being read from the sink so that they can be used. These are meant to be helpers and in no way should anyone feel obliged to use them. If you've got this all figured out already, great! If you do this already much better in i915, by all means stay away from this. At the same time it seems counter-productive to write all of this code as part of the Tegra DRM driver. In my opinion subsystems should provide generic helpers that can help multiple drivers share code. This is especially true for things that are defined in a specification because there's not a lot of room for interpretation. The helpers in these patches are meant to be that kind of helpers. > One of the technical observations is that you fill the struct > drm_dp_link and struct drm_dp_link_caps from the sink. It's not clear > that the link caps really are an intersection of the source and sink > caps. The eDP 1.4 link rates are the prime example. I think you should > have sets of source and sink rates, and you should intersect those to > find out the available link rates. The max rate is the highest number in > that set. Similarly for many things, like training pattern support. I > think it's only going to get more complicated with DP 2.0. The idea here was to provide only helpers to collect the DPCD data defined by the specification. Anything specific to the source is meant to be handled by display driver. In case of eDP 1.4 link rates the code will only add the rates read from DPCD. It's up to the driver to filter out rates that it doesn't support from that list. I think the fact that things will keep getting more complicated is an argument in favour of sharing code rather than keep doing the same (complicated) thing over and over again in every driver. > Anothe
Re: [Intel-gfx] [PATCH 18/21] drm/tegra: Use drm_fb_helper_fill_info
On Tue, Mar 26, 2019 at 02:20:05PM +0100, Daniel Vetter wrote: > Another driver that didn't set fbinfo->fix.id before. > > v2: Fix subject and rebase > > Acked-by: Thierry Reding > Signed-off-by: Daniel Vetter > Cc: Thierry Reding > Cc: Jonathan Hunter > Cc: linux-te...@vger.kernel.org > --- > drivers/gpu/drm/tegra/fb.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) Acked-by: Thierry Reding signature.asc Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 09/59] drm/prime: Align gem_prime_export with obj_funcs.export
On Fri, Jun 14, 2019 at 10:35:25PM +0200, Daniel Vetter wrote: > The idea is that gem_prime_export is deprecated in favor of > obj_funcs.export. That's much easier to do if both have matching > function signatures. > > Signed-off-by: Daniel Vetter > Cc: Russell King > Cc: Maarten Lankhorst > Cc: Maxime Ripard > Cc: Sean Paul > Cc: David Airlie > Cc: Daniel Vetter > Cc: Zhenyu Wang > Cc: Zhi Wang > Cc: Jani Nikula > Cc: Joonas Lahtinen > Cc: Rodrigo Vivi > Cc: Tomi Valkeinen > Cc: Alex Deucher > Cc: "Christian König" > Cc: "David (ChunMing) Zhou" > Cc: Thierry Reding > Cc: Jonathan Hunter > Cc: Dave Airlie > Cc: Eric Anholt > Cc: "Michel Dänzer" > Cc: Chris Wilson > Cc: Huang Rui > Cc: Felix Kuehling > Cc: Hawking Zhang > Cc: Feifei Xu > Cc: Jim Qu > Cc: Evan Quan > Cc: Matthew Auld > Cc: Mika Kuoppala > Cc: Thomas Zimmermann > Cc: Kate Stewart > Cc: Sumit Semwal > Cc: Jilayne Lovejoy > Cc: Thomas Gleixner > Cc: Mikulas Patocka > Cc: Greg Kroah-Hartman > Cc: Junwei Zhang > Cc: intel-gvt-...@lists.freedesktop.org > Cc: intel-gfx@lists.freedesktop.org > Cc: amd-...@lists.freedesktop.org > Cc: linux-te...@vger.kernel.org > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c | 7 +++ > drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.h | 3 +-- > drivers/gpu/drm/armada/armada_gem.c | 5 ++--- > drivers/gpu/drm/armada/armada_gem.h | 3 +-- > drivers/gpu/drm/drm_prime.c | 9 - > drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 5 ++--- > drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c | 8 > drivers/gpu/drm/i915/gvt/dmabuf.c| 2 +- > drivers/gpu/drm/i915/i915_drv.h | 3 +-- > drivers/gpu/drm/omapdrm/omap_gem.h | 3 +-- > drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c| 5 ++--- > drivers/gpu/drm/radeon/radeon_drv.c | 3 +-- > drivers/gpu/drm/radeon/radeon_prime.c| 5 ++--- > drivers/gpu/drm/tegra/gem.c | 7 +++ > drivers/gpu/drm/tegra/gem.h | 3 +-- > drivers/gpu/drm/udl/udl_dmabuf.c | 5 ++--- > drivers/gpu/drm/udl/udl_drv.h| 3 +-- > drivers/gpu/drm/vc4/vc4_bo.c | 5 ++--- > drivers/gpu/drm/vc4/vc4_drv.h| 3 +-- > drivers/gpu/drm/vgem/vgem_fence.c| 2 +- > include/drm/drm_drv.h| 4 ++-- > include/drm/drm_prime.h | 3 +-- > 22 files changed, 39 insertions(+), 57 deletions(-) Acked-by: Thierry Reding signature.asc Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] drm connectors, tegra, and the web they weave (was Re: [PATCH 58/59] drm/todo: Add new debugfs todo)
On Tue, Jun 18, 2019 at 11:46:56PM +0200, Daniel Vetter wrote: > On Tue, Jun 18, 2019 at 08:01:13PM +0200, Greg Kroah-Hartman wrote: > > On Tue, Jun 18, 2019 at 07:32:20PM +0200, Daniel Vetter wrote: > > > On Tue, Jun 18, 2019 at 5:25 PM Greg Kroah-Hartman > > > wrote: > > > > On Tue, Jun 18, 2019 at 05:19:38PM +0200, Greg Kroah-Hartman wrote: > > > > > I could just "open code" a bunch of calls to debugfs_create_file() for > > > > > these drivers, which would solve this issue, but in a more "non-drm" > > > > > way. Is it worth to just do that instead of overthinking the whole > > > > > thing and trying to squish it into the drm "model" of drm debugfs > > > > > calls? > > > > > > > > An example of "open coding" this is the patch below for the sor.c > > > > driver. > > > > > > I think open-coding is the way to go here. One of the todos is to > > > extend debugfs support for crtc/connectors, but looking at the > > > open-coded version we really don't need a drm-flavoured midlayer here. > > > > There already is debugfs support in the code for crtc/connectors, these > > files are "hanging" off of those locations already. I'll keep that, but > > indent it one more directory so that there's no namespace collisions. > > The todo was to have some drm wrappers here for the boilerplate, but after > looking at your version that's not a good idea. So not just making sure > crtcs/connectors have a debugfs directory made for them, but more. > > Wrt adding a new directory: debugfs isnt uapi, but there's usually a > massive pile of script relying on them, so it's not nice to shuffle paths > around. Plus the lifetimes match anyway (at least if you don't hotplug > connectors, which tegra doesn't do). So, I think you two already covered everything. From a Tegra perspective there's not really a need to care about the exact structure of debugfs because there are only a handful of scripts that use this and they are not exactly widely distributed. At the same time there's really no need to add another level of directories, since the connector really is the SOR, so an sor directory in the connector's directory is just redundant. Cleaning up and lifetime management aren't issues, really, so there are no good arguments for adding it, in my opinion. Historically, the sor.c driver is interesting because it used to be just plain debugfs calls. Only when I added a second debugfs file I decided to go with the built-in DRM infrastructure for this and then later went and converted the first file to it as well, for consistency. I do remember though that I was very unhappy about the fact that I had to do all this kmemdup()'ing just to make debugfs files per-instance (the DRM infrastructure doesn't allow that by default). In retrospect that was a poor decision and I should've just stuck with debugfs and perhaps just spin a couple of helpers around that instead. So I'm happy to see this effort. Thierry signature.asc Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] drm connectors, tegra, and the web they weave (was Re: [PATCH 58/59] drm/todo: Add new debugfs todo)
On Tue, Jun 18, 2019 at 05:19:38PM +0200, Greg Kroah-Hartman wrote: > On Fri, Jun 14, 2019 at 10:36:14PM +0200, Daniel Vetter wrote: > > Greg is busy already, but maybe he won't do everything ... > > > > Cc: Greg Kroah-Hartman > > Signed-off-by: Daniel Vetter > > --- > > Documentation/gpu/todo.rst | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst > > index 9717540ee28f..026e55c517e1 100644 > > --- a/Documentation/gpu/todo.rst > > +++ b/Documentation/gpu/todo.rst > > @@ -375,6 +375,9 @@ There's a bunch of issues with it: > >this (together with the drm_minor->drm_device move) would allow us to > > remove > >debugfs_init. > > > > +- Drop the return code and error checking from all debugfs functions. Greg > > KH is > > + working on this already. > > > Part of this work was to try to delete drm_debugfs_remove_files(). > > There are only 4 files that currently still call this function: > drivers/gpu/drm/tegra/dc.c > drivers/gpu/drm/tegra/dsi.c > drivers/gpu/drm/tegra/hdmi.c > drivers/gpu/drm/tegra/sor.c > > For dc.c, the driver wants to add debugfs files to the struct drm_crtc > debugfs directory. Which is fine, but it has to do some special memory > allocation to get the debugfs callback to point not to the struct > drm_minor pointer, but rather the drm_crtc structure. Actually the reason why the memory allocation is done is because there can be multiple instances of the display controller. In fact, there's always at least two (and up to four in later Tegra generations). The DRM debugfs infrastructure, however, doesn't automatically duplicate the data for each drm_debugfs_create_files() call and at the same time it does not allow you to specify driver-private data other than by embedding it in the drm_info_list structure. So rather than manually create the drm_info_list for each display controller instance, the code creates a template that is then duplicated and for which the driver- private is then set. That way multiple invocations end up with different data. This is because of the extra indirection that the DRM debugfs infrastructure introduces. It's in fact much easier to do this with just plain debugfs function calls. The only downside is the boilerplate required to make that happen. Thierry signature.asc Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] drm connectors, tegra, and the web they weave (was Re: [PATCH 58/59] drm/todo: Add new debugfs todo)
On Tue, Jun 18, 2019 at 07:32:20PM +0200, Daniel Vetter wrote:
> On Tue, Jun 18, 2019 at 5:25 PM Greg Kroah-Hartman
> wrote:
> > On Tue, Jun 18, 2019 at 05:19:38PM +0200, Greg Kroah-Hartman wrote:
> > > On Fri, Jun 14, 2019 at 10:36:14PM +0200, Daniel Vetter wrote:
> > > > Greg is busy already, but maybe he won't do everything ...
> > > >
> > > > Cc: Greg Kroah-Hartman
> > > > Signed-off-by: Daniel Vetter
> > > > ---
> > > > Documentation/gpu/todo.rst | 3 +++
> > > > 1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
> > > > index 9717540ee28f..026e55c517e1 100644
> > > > --- a/Documentation/gpu/todo.rst
> > > > +++ b/Documentation/gpu/todo.rst
> > > > @@ -375,6 +375,9 @@ There's a bunch of issues with it:
> > > >this (together with the drm_minor->drm_device move) would allow us
> > > > to remove
> > > >debugfs_init.
> > > >
> > > > +- Drop the return code and error checking from all debugfs functions.
> > > > Greg KH is
> > > > + working on this already.
> > >
> > >
> > > Part of this work was to try to delete drm_debugfs_remove_files().
> > >
> > > There are only 4 files that currently still call this function:
> > > drivers/gpu/drm/tegra/dc.c
> > > drivers/gpu/drm/tegra/dsi.c
> > > drivers/gpu/drm/tegra/hdmi.c
> > > drivers/gpu/drm/tegra/sor.c
> > >
> > > For dc.c, the driver wants to add debugfs files to the struct drm_crtc
> > > debugfs directory. Which is fine, but it has to do some special memory
> > > allocation to get the debugfs callback to point not to the struct
> > > drm_minor pointer, but rather the drm_crtc structure.
>
> There's already a todo to switch the drm_minor debugfs stuff over to
> drm_device. drm_minor is essentially different uapi flavours (/dev/
> minor nodes, hence the name) sitting on top of the same drm_device.
> Last time I checked all the debugfs files want the drm_device, not the
> minor. I think we even discussed to only register the debugfs files
> for the first minor, and create the other ones as symlinks to the
> first one. But haven't yet gotten around to typing that.
>
> drm_crtc/connector are parts of drm_device with modesetting support,
> so the drm_minor is even worse choice really.
For the connector drivers we already sit on top of the per-connector
debugfs directories. I think the only reason why we don't do that for
the display controller is because drm_crtc didn't have built-in debugfs
support like the connectors have. It looks like that's no longer true,
though it's been there for a while. I think it'd be good to just move
those over as well.
As for passing struct drm_minor, I think that's mostly unnecessary. As
far as I can tell, we only use drm_minor to get at drm_device, which in
turn we only use to check some features flags, and drm_minor itself is
only used to track the list of files that are being added so that they
can later be removed again. Given that we can just tear down everything
debugfs recursively, I don't think we need any of that.
>
> Not exactly sure why we went with this, but probably dates back to the
> *bsd compat layer and a lot of these files hanging out in procfs too
> (we've fixed those mistakes a few years ago, yay!).
>
> > > So, to remove this call, I need to remove this special memory allocation
> > > and to do that, I need to somehow be able to cast from drm_minor back to
> > > the drm_crtc structure being used in this driver. And I can't figure
> > > how they are related at all.
> > >
> > > Any pointers here (pun intended) would be appreciated.
> > >
> > > For the other 3 files, the situation is much the same, but I need to get
> > > from a 'struct drm_minor' pointer to a 'struct drm_connector' pointer.
>
> Ditch the drm_minor, there's no no way to get from that to something
> like drm_connector/crtc, since it's a n:m relationship.
Yeah. That too.
> > > I could just "open code" a bunch of calls to debugfs_create_file() for
> > > these drivers, which would solve this issue, but in a more "non-drm"
> > > way. Is it worth to just do that instead of overthinking the whole
> > > thing and trying to squish it into the drm "model" of drm debugfs calls?
> >
> > An example of "open coding" this is the patch below for the sor.c
> > driver.
>
> I think open-coding is the way to go here. One of the todos is to
> extend debugfs support for crtc/connectors, but looking at the
> open-coded version we really don't need a drm-flavoured midlayer here.
Exactly my thoughts. It'd be nice to have some sort of macro to help
bring the boilerplate down a bit.
Thierry
> > Totally untested, not even built, but you should get the idea here.
> >
> > thanks,
> >
> > greg k-h
> >
> > ---
> >
> > diff --git a/drivers/gpu/drm/tegra/sor.c b/drivers/gpu/drm/tegra/sor.c
> > index 5be5a0817dfe..3216221c77c4 100644
> > --- a/drivers/gpu/drm/tegra/sor.c
> > +++ b/drivers/gpu/drm/tegra/sor.c
> > @@ -414,7 +414,8 @@ struct tegra_sor {
>
Re: [Intel-gfx] drm connectors, tegra, and the web they weave (was Re: [PATCH 58/59] drm/todo: Add new debugfs todo)
On Tue, Jun 18, 2019 at 04:37:16PM +0100, Jon Hunter wrote: > > On 18/06/2019 16:19, Greg Kroah-Hartman wrote: > > On Fri, Jun 14, 2019 at 10:36:14PM +0200, Daniel Vetter wrote: > >> Greg is busy already, but maybe he won't do everything ... > >> > >> Cc: Greg Kroah-Hartman > >> Signed-off-by: Daniel Vetter > >> --- > >> Documentation/gpu/todo.rst | 3 +++ > >> 1 file changed, 3 insertions(+) > >> > >> diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst > >> index 9717540ee28f..026e55c517e1 100644 > >> --- a/Documentation/gpu/todo.rst > >> +++ b/Documentation/gpu/todo.rst > >> @@ -375,6 +375,9 @@ There's a bunch of issues with it: > >>this (together with the drm_minor->drm_device move) would allow us to > >> remove > >>debugfs_init. > >> > >> +- Drop the return code and error checking from all debugfs functions. > >> Greg KH is > >> + working on this already. > > > > > > Part of this work was to try to delete drm_debugfs_remove_files(). > > > > There are only 4 files that currently still call this function: > > drivers/gpu/drm/tegra/dc.c > > drivers/gpu/drm/tegra/dsi.c > > drivers/gpu/drm/tegra/hdmi.c > > drivers/gpu/drm/tegra/sor.c > > > > For dc.c, the driver wants to add debugfs files to the struct drm_crtc > > debugfs directory. Which is fine, but it has to do some special memory > > allocation to get the debugfs callback to point not to the struct > > drm_minor pointer, but rather the drm_crtc structure. > > > > So, to remove this call, I need to remove this special memory allocation > > and to do that, I need to somehow be able to cast from drm_minor back to > > the drm_crtc structure being used in this driver. And I can't figure > > how they are related at all. > > > > Any pointers here (pun intended) would be appreciated. > > > > For the other 3 files, the situation is much the same, but I need to get > > from a 'struct drm_minor' pointer to a 'struct drm_connector' pointer. > > > > I could just "open code" a bunch of calls to debugfs_create_file() for > > these drivers, which would solve this issue, but in a more "non-drm" > > way. Is it worth to just do that instead of overthinking the whole > > thing and trying to squish it into the drm "model" of drm debugfs calls? > > > > Either way, who can test these changes? I can't even build the tegra > > driver without digging up an arm64 cross-compiler, and can't test it as > > I have no hardware at all. > > We can definitely compile and boot test these no problem. In fact > anything that lands in -next we will boot test. However, I can do some > quick sanity if you have something to test. > > Thierry may have more specific Tegra DRM tests. We don't have any automated tests for this yet, unfortunately. Let me work on something. In the meantime I can manually test any of the patches that Greg sends out. These should be fairly trivial to test. It's difficult to check for success/failure on something like the register dump or the CRC, but I think for now we don't really need much more than just validating that things don't crash when we read one of these debugfs files. Thierry signature.asc Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/5] drm/panel: Add attach/detach callbacks
On Tue, Jun 11, 2019 at 05:25:47PM -0700, dbasehore . wrote:
> On Tue, Jun 11, 2019 at 1:57 AM Daniel Vetter wrote:
> >
> > On Mon, Jun 10, 2019 at 09:03:48PM -0700, Derek Basehore wrote:
> > > This adds the attach/detach callbacks. These are for setting up
> > > internal state for the connector/panel pair that can't be done at
> > > probe (since the connector doesn't exist) and which don't need to be
> > > repeatedly done for every get/modes, prepare, or enable callback.
> > > Values such as the panel orientation, and display size can be filled
> > > in for the connector.
> > >
> > > Signed-off-by: Derek Basehore
> > > ---
> > > drivers/gpu/drm/drm_panel.c | 14 ++
> > > include/drm/drm_panel.h | 4
> > > 2 files changed, 18 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
> > > index 3b689ce4a51a..72f67678d9d5 100644
> > > --- a/drivers/gpu/drm/drm_panel.c
> > > +++ b/drivers/gpu/drm/drm_panel.c
> > > @@ -104,12 +104,23 @@ EXPORT_SYMBOL(drm_panel_remove);
> > > */
> > > int drm_panel_attach(struct drm_panel *panel, struct drm_connector
> > > *connector)
> > > {
> > > + int ret;
> > > +
> > > if (panel->connector)
> > > return -EBUSY;
> > >
> > > panel->connector = connector;
> > > panel->drm = connector->dev;
> > >
> > > + if (panel->funcs->attach) {
> > > + ret = panel->funcs->attach(panel);
> > > + if (ret < 0) {
> > > + panel->connector = NULL;
> > > + panel->drm = NULL;
> > > + return ret;
> > > + }
> > > + }
> >
> > Why can't we just implement this in the drm helpers for everyone, by e.g.
> > storing a dt node in drm_panel? Feels a bit overkill to have these new
> > hooks here.
> >
> > Also, my understanding is that this dt stuff is supposed to be
> > standardized, so this should work.
>
> So do you want all of this information added to the drm_panel struct?
> If we do that, we don't necessarily even need the drm helper function.
> We could just copy the values over here in the drm_panel_attach
> function (and clear them in drm_panel_detach).
Yeah, I think we should have all this extra information in the struct
drm_panel. However, I think we need to more carefully split things such
that the DT parsing happens at panel probe time. That way we can catch
errors in DT, or missing entries/resources when we can still do
something about it.
If we start parsing DT and encounter failures, it's going to be very
confusing if that's at panel attach time where code will usually just
assume that everything is already validated and can't fail anymore.
Thierry
signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/panel: make drm_panel.h self-contained
On Thu, Jun 27, 2019 at 02:01:03PM +0300, Jani Nikula wrote:
> Fix build warning if drm_panel.h is built with CONFIG_OF=n or
> CONFIG_DRM_PANEL=n and included without the prerequisite err.h:
>
> ./include/drm/drm_panel.h: In function ‘of_drm_find_panel’:
> ./include/drm/drm_panel.h:203:9: error: implicit declaration of function
> ‘ERR_PTR’ [-Werror=implicit-function-declaration]
> return ERR_PTR(-ENODEV);
> ^~~
> ./include/drm/drm_panel.h:203:9: error: returning ‘int’ from a function with
> return type ‘struct drm_panel *’ makes pointer from integer without a cast
> [-Werror=int-conversion]
> return ERR_PTR(-ENODEV);
> ^~~~
>
> Fixes: 5fa8e4a22182 ("drm/panel: Make of_drm_find_panel() return an ERR_PTR()
> instead of NULL")
> Cc: Boris Brezillon
> Cc: Thierry Reding
> Signed-off-by: Jani Nikula
>
> ---
>
> I don't know if there's a combo where this actually fails, so I'm not
> adding cc: stable. It's just something I hit when playing with other
> code.
> ---
> include/drm/drm_panel.h | 1 +
> 1 file changed, 1 insertion(+)
Acked-by: Thierry Reding
signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 00/21] drm/dp: Various helper improvements and cleanups
On Mon, Sep 23, 2019 at 04:52:02PM +0200, Thierry Reding wrote: > On Mon, Sep 23, 2019 at 04:52:50PM +0300, Jani Nikula wrote: > > On Fri, 20 Sep 2019, Thierry Reding wrote: > > > On Mon, Sep 02, 2019 at 01:31:00PM +0200, Thierry Reding wrote: > > >> From: Thierry Reding > > >> > > >> Hi, > > >> > > >> this series of patches improves the DP helpers a bit and cleans up some > > >> inconsistencies along the way. > > >> > > >> v2 incorporates all review comments add collects Reviewed-bys from v1. > > >> > > >> Thierry > > >> > > >> Thierry Reding (21): > > >> drm/dp: Sort includes alphabetically > > >> drm/dp: Add missing kerneldoc for struct drm_dp_link > > >> drm/dp: Add drm_dp_link_reset() implementation > > >> drm/dp: Track link capabilities alongside settings > > >> drm/dp: Turn link capabilities into booleans > > >> drm/dp: Probe link using existing parsing helpers > > >> drm/dp: Read fast training capability from link > > >> drm/dp: Read TPS3 capability from sink > > >> drm/dp: Read channel coding capability from sink > > >> drm/dp: Read alternate scrambler reset capability from sink > > >> drm/dp: Read eDP version from DPCD > > >> drm/dp: Read AUX read interval from DPCD > > >> drm/dp: Do not busy-loop during link training > > >> drm/dp: Use drm_dp_aux_rd_interval() > > >> drm/dp: Add helper to get post-cursor adjustments > > >> drm/dp: Set channel coding on link configuration > > >> drm/dp: Enable alternate scrambler reset when supported > > >> drm/dp: Add drm_dp_link_choose() helper > > >> drm/dp: Add support for eDP link rates > > >> drm/dp: Remove a gratuituous blank line > > >> drm/bridge: tc358767: Use DP nomenclature > > > > > > Anyone interested in reviewing these? > > > > Thierry, I don't quite know how to put this nicely, but I also don't > > think it's nice to not reply at all. So I'll try to be fair but it'll be > > blunt. Fair enough? > > Fair enough. > > > I've glanced over the series, already before you pinged for reviews. It > > looks like you've put effort into it, and it all looks nice. However, it > > does not look like we could use this in i915, without significant effort > > both on top of this work and in i915. It does not feel like there's any > > incentive for us to review this in detail. > > > > It also feels like there's an increasing disconnect between "small" and > > "big" drivers (*) when it comes to handling DP link and training. It > > scares me a bit that this work is being done on the terms of the "small" > > drivers, and that later in time this might be considered the One True > > Way of handling DP. > > I'm not sure I understand your concern here. The goal of the series is > primarily to extend the existing support for DP. It follows the pattern > established by existing helpers and then goes one step further and > provides some common way of actually storing the values that are being > read from the sink so that they can be used. > > These are meant to be helpers and in no way should anyone feel obliged > to use them. If you've got this all figured out already, great! If you > do this already much better in i915, by all means stay away from this. > > At the same time it seems counter-productive to write all of this code > as part of the Tegra DRM driver. In my opinion subsystems should provide > generic helpers that can help multiple drivers share code. This is > especially true for things that are defined in a specification because > there's not a lot of room for interpretation. The helpers in these > patches are meant to be that kind of helpers. > > > One of the technical observations is that you fill the struct > > drm_dp_link and struct drm_dp_link_caps from the sink. It's not clear > > that the link caps really are an intersection of the source and sink > > caps. The eDP 1.4 link rates are the prime example. I think you should > > have sets of source and sink rates, and you should intersect those to > > find out the available link rates. The max rate is the highest number in > > that set. Similarly for many things, like training pattern support. I > > think it's only going to get more complicated with DP 2.0. > > The idea here was to provide only helpers to collect the DPCD da
Re: [Intel-gfx] [PATCH 01/15] drm/tegra: Map cmdbuf once for reloc processing
On Mon, Nov 25, 2019 at 10:58:56AM +0100, Daniel Vetter wrote:
> On Mon, Nov 18, 2019 at 11:35:22AM +0100, Daniel Vetter wrote:
> > A few reasons to drop kmap:
> >
> > - For native objects all we do is look at obj->vaddr anyway, so might
> > as well not call functions for every page.
> >
> > - Reloc-processing on dma-buf is ... questionable.
> >
> > - Plus most dma-buf that bother kernel cpu mmaps give you at least
> > vmap, much less kmaps. And all the ones relevant for arm-soc are
> > again doing a obj->vaddr game anyway, there's no real kmap going on
> > on arm it seems.
> >
> > Plus this seems to be the only real in-tree user of dma_buf_kmap, and
> > I'd like to get rid of that.
> >
> > Signed-off-by: Daniel Vetter
> > Cc: Thierry Reding
> > Cc: linux-te...@vger.kernel.org
>
> Ping for testing/review on these first 2 tegra patches. They're holding up
> the entire series, and I got acks for all the other bits surprisingly
> fast. So would like to land this rather sooner than later. I'm also
> working on a lot more dma-buf patches ...
Right, I had forgotten about this series. Let me go test it right away.
Thierry
> > ---
> > drivers/gpu/host1x/job.c | 21 +++--
> > 1 file changed, 7 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/gpu/host1x/job.c b/drivers/gpu/host1x/job.c
> > index 25ca54de8fc5..60b2fedd0061 100644
> > --- a/drivers/gpu/host1x/job.c
> > +++ b/drivers/gpu/host1x/job.c
> > @@ -244,8 +244,7 @@ static unsigned int pin_job(struct host1x *host, struct
> > host1x_job *job)
> >
> > static int do_relocs(struct host1x_job *job, struct host1x_job_gather *g)
> > {
> > - u32 last_page = ~0;
> > - void *cmdbuf_page_addr = NULL;
> > + void *cmdbuf_addr = NULL;
> > struct host1x_bo *cmdbuf = g->bo;
> > unsigned int i;
> >
> > @@ -267,28 +266,22 @@ static int do_relocs(struct host1x_job *job, struct
> > host1x_job_gather *g)
> > goto patch_reloc;
> > }
> >
> > - if (last_page != reloc->cmdbuf.offset >> PAGE_SHIFT) {
> > - if (cmdbuf_page_addr)
> > - host1x_bo_kunmap(cmdbuf, last_page,
> > -cmdbuf_page_addr);
> > + if (!cmdbuf_addr) {
> > + cmdbuf_addr = host1x_bo_mmap(cmdbuf);
> >
> > - cmdbuf_page_addr = host1x_bo_kmap(cmdbuf,
> > - reloc->cmdbuf.offset >> PAGE_SHIFT);
> > - last_page = reloc->cmdbuf.offset >> PAGE_SHIFT;
> > -
> > - if (unlikely(!cmdbuf_page_addr)) {
> > + if (unlikely(!cmdbuf_addr)) {
> > pr_err("Could not map cmdbuf for relocation\n");
> > return -ENOMEM;
> > }
> > }
> >
> > - target = cmdbuf_page_addr + (reloc->cmdbuf.offset & ~PAGE_MASK);
> > + target = cmdbuf_addr + reloc->cmdbuf.offset;
> > patch_reloc:
> > *target = reloc_addr;
> > }
> >
> > - if (cmdbuf_page_addr)
> > - host1x_bo_kunmap(cmdbuf, last_page, cmdbuf_page_addr);
> > + if (cmdbuf_addr)
> > + host1x_bo_munmap(cmdbuf, cmdbuf_addr);
> >
> > return 0;
> > }
> > --
> > 2.24.0
> >
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 01/15] drm/tegra: Map cmdbuf once for reloc processing
On Mon, Nov 18, 2019 at 11:35:22AM +0100, Daniel Vetter wrote:
> A few reasons to drop kmap:
>
> - For native objects all we do is look at obj->vaddr anyway, so might
> as well not call functions for every page.
>
> - Reloc-processing on dma-buf is ... questionable.
>
> - Plus most dma-buf that bother kernel cpu mmaps give you at least
> vmap, much less kmaps. And all the ones relevant for arm-soc are
> again doing a obj->vaddr game anyway, there's no real kmap going on
> on arm it seems.
>
> Plus this seems to be the only real in-tree user of dma_buf_kmap, and
> I'd like to get rid of that.
>
> Signed-off-by: Daniel Vetter
> Cc: Thierry Reding
> Cc: linux-te...@vger.kernel.org
> ---
> drivers/gpu/host1x/job.c | 21 +++--
> 1 file changed, 7 insertions(+), 14 deletions(-)
This looks correct to me, and running some of the grate project's tests
against this works just fine, so:
Reviewed-by: Thierry Reding
Tested-by: Thierry Reding
> diff --git a/drivers/gpu/host1x/job.c b/drivers/gpu/host1x/job.c
> index 25ca54de8fc5..60b2fedd0061 100644
> --- a/drivers/gpu/host1x/job.c
> +++ b/drivers/gpu/host1x/job.c
> @@ -244,8 +244,7 @@ static unsigned int pin_job(struct host1x *host, struct
> host1x_job *job)
>
> static int do_relocs(struct host1x_job *job, struct host1x_job_gather *g)
> {
> - u32 last_page = ~0;
> - void *cmdbuf_page_addr = NULL;
> + void *cmdbuf_addr = NULL;
> struct host1x_bo *cmdbuf = g->bo;
> unsigned int i;
>
> @@ -267,28 +266,22 @@ static int do_relocs(struct host1x_job *job, struct
> host1x_job_gather *g)
> goto patch_reloc;
> }
>
> - if (last_page != reloc->cmdbuf.offset >> PAGE_SHIFT) {
> - if (cmdbuf_page_addr)
> - host1x_bo_kunmap(cmdbuf, last_page,
> - cmdbuf_page_addr);
> + if (!cmdbuf_addr) {
> + cmdbuf_addr = host1x_bo_mmap(cmdbuf);
>
> - cmdbuf_page_addr = host1x_bo_kmap(cmdbuf,
> - reloc->cmdbuf.offset >> PAGE_SHIFT);
> - last_page = reloc->cmdbuf.offset >> PAGE_SHIFT;
> -
> - if (unlikely(!cmdbuf_page_addr)) {
> + if (unlikely(!cmdbuf_addr)) {
> pr_err("Could not map cmdbuf for relocation\n");
> return -ENOMEM;
> }
> }
>
> - target = cmdbuf_page_addr + (reloc->cmdbuf.offset & ~PAGE_MASK);
> + target = cmdbuf_addr + reloc->cmdbuf.offset;
> patch_reloc:
> *target = reloc_addr;
> }
>
> - if (cmdbuf_page_addr)
> - host1x_bo_kunmap(cmdbuf, last_page, cmdbuf_page_addr);
> + if (cmdbuf_addr)
> + host1x_bo_munmap(cmdbuf, cmdbuf_addr);
>
> return 0;
> }
> --
> 2.24.0
>
signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 02/15] drm/tegra: Delete host1x_bo_ops->k(un)map
On Mon, Nov 18, 2019 at 11:35:23AM +0100, Daniel Vetter wrote: > It doesn't have any callers anymore. > > Aside: The ->mmap/munmap hooks have a bit a confusing name, they don't > do userspace mmaps, but a kernel vmap. I think most places use vmap > for this, except ttm, which uses kmap for vmap for added confusion. > mmap seems entirely for userspace mappings set up through mmap(2) > syscall. > > Signed-off-by: Daniel Vetter > Cc: Thierry Reding > Cc: Jonathan Hunter > Cc: linux-te...@vger.kernel.org > --- > drivers/gpu/drm/tegra/gem.c | 28 > include/linux/host1x.h | 13 - > 2 files changed, 41 deletions(-) Tested along with the rest of the series and this is obviously right now that the only user is gone, so: Reviewed-by: Thierry Reding Tested-by: Thierry Reding signature.asc Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 08/15] drm/tegra: Remove dma_buf->k(un)map
On Mon, Nov 18, 2019 at 11:35:29AM +0100, Daniel Vetter wrote: > No in-tree users left. > > Signed-off-by: Daniel Vetter > Cc: Thierry Reding > Cc: Jonathan Hunter > Cc: linux-te...@vger.kernel.org > --- > drivers/gpu/drm/tegra/gem.c | 12 > 1 file changed, 12 deletions(-) Same as before, I don't see any regressions when running some of the grate tests, and there's obviously no longer any reason to keep these functions around given that they are no longer used, so: Reviewed-by: Thierry Reding Tested-by: Thierry Reding signature.asc Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/dp: Add function to parse EDID descriptors for adaptive sync limits
On Wed, Oct 23, 2019 at 05:00:41PM -0700, Manasi Navare wrote:
> Adaptive Sync is a VESA feature so add a DRM core helper to parse
> the EDID's detailed descritors to obtain the adaptive sync monitor range.
> Store this info as part fo drm_display_info so it can be used
> across all drivers.
> This part of the code is stripped out of amdgpu's function
> amdgpu_dm_update_freesync_caps() to make it generic and be used
> across all DRM drivers
>
> Cc: Ville Syrjälä
> Cc: Harry Wentland
> Cc: Clinton A Taylor
> Signed-off-by: Manasi Navare
> ---
> drivers/gpu/drm/drm_edid.c | 49 +
> include/drm/drm_connector.h | 25 +++
> include/drm/drm_edid.h | 2 ++
> 3 files changed, 76 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 474ac04d5600..97dd1200773e 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -4707,6 +4707,52 @@ static void drm_parse_cea_ext(struct drm_connector
> *connector,
> }
> }
>
> +void drm_get_adaptive_sync_limits(struct drm_connector *connector,
> + const struct edid *edid)
> +{
> + struct drm_display_info *info = &connector->display_info;
> + const struct detailed_timing *timing;
> + const struct detailed_non_pixel *data;
> + const struct detailed_data_monitor_range *range;
> + int i;
This can be unsigned int.
> +
> + /*
> + * Restrict Adaptive Sync only for dp and edp
> + */
> + if (connector->connector_type != DRM_MODE_CONNECTOR_DisplayPort &&
> + connector->connector_type != DRM_MODE_CONNECTOR_eDP)
> + return;
> +
> + if (edid->version <= 1 && !(edid->version == 1 && edid->revision > 1))
> + return;
> +
> + for (i = 0; i < 4; i++) {
> + timing = &edid->detailed_timings[i];
> + data= &timing->data.other_data;
> + range = &data->data.range;
> + /*
> + * Check if monitor has continuous frequency mode
> + */
> + if (data->type != EDID_DETAIL_MONITOR_RANGE)
> + continue;
> + /*
> + * Check for flag range limits only. If flag == 1 then
> + * no additional timing information provided.
> + * Default GTF, GTF Secondary curve and CVT are not
> + * supported
> + */
> + if (range->flags != 1)
> + continue;
> +
> + info->adaptive_sync.min_vfreq = range->min_vfreq;
> + info->adaptive_sync.max_vfreq = range->max_vfreq;
> + info->adaptive_sync.pixel_clock_mhz =
> + range->pixel_clock_mhz * 10;
> + break;
> + }
> +}
> +EXPORT_SYMBOL(drm_get_adaptive_sync_limits);
> +
> /* A connector has no EDID information, so we've got no EDID to compute
> quirks from. Reset
> * all of the values which would have been set from EDID
> */
> @@ -4728,6 +4774,7 @@ drm_reset_display_info(struct drm_connector *connector)
> memset(&info->hdmi, 0, sizeof(info->hdmi));
>
> info->non_desktop = 0;
> + memset(&info->adaptive_sync, 0, sizeof(info->adaptive_sync));
> }
>
> u32 drm_add_display_info(struct drm_connector *connector, const struct edid
> *edid)
> @@ -4743,6 +4790,8 @@ u32 drm_add_display_info(struct drm_connector
> *connector, const struct edid *edi
>
> info->non_desktop = !!(quirks & EDID_QUIRK_NON_DESKTOP);
>
> + drm_get_adaptive_sync_limits(connector, edid);
> +
> DRM_DEBUG_KMS("non_desktop set to %d\n", info->non_desktop);
>
> if (edid->revision < 3)
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 5f8c3389d46f..a27a84270d8d 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -254,6 +254,26 @@ enum drm_panel_orientation {
> DRM_MODE_PANEL_ORIENTATION_RIGHT_UP,
> };
>
> +/**
> + * struct drm_adaptive_sync_info - Panel's Adaptive Sync capabilities for
> + * &drm_display_info
> + *
> + * This struct is used to store a Panel's Adaptive Sync capabilities
> + * as parsed from EDID's detailed monitor range descriptor block.
> + *
> + * @min_vfreq: This is the min supported refresh rate in Hz from
> + * EDID's detailed monitor range.
> + * @max_vfreq: This is the max supported refresh rate in Hz from
> + * EDID's detailed monitor range
> + * @pixel_clock_mhz: This is the dotclock in MHz from
> + * EDID's detailed monitor range
> + */
> +struct drm_adaptive_sync_info {
> + int min_vfreq;
> + int max_vfreq;
> + int pixel_clock_mhz;
Any reason why these can't be unsigned? Also, does it perhaps make sense
to store the pixel clock as kHz like we do everywhere else?
Thierry
signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.fr
Re: [Intel-gfx] [PATCH 2/2] drm/todo: Add entry to remove load/unload hooks
On Wed, Oct 23, 2019 at 04:49:53PM +0200, Daniel Vetter wrote: > They're midlayer, broken, and because of the old gunk, we can't fix > them. For examples see the various checks in drm_mode_object.c against > dev->registered, which cannot be enforced if the driver still uses the > load hook. > > Unfortunately our biggest driver still uses load/unload, so this would > be really great to get fixed. > > Cc: Alex Deucher > Cc: Harry Wentland > Signed-off-by: Daniel Vetter > --- > Documentation/gpu/todo.rst | 17 + > 1 file changed, 17 insertions(+) Reminds me that I need to still do that for Tegra: Reviewed-by: Thierry Reding signature.asc Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] drm/property: Enforce more lifetime rules
On Wed, Oct 23, 2019 at 04:49:52PM +0200, Daniel Vetter wrote:
> Properties can't be attached after registering, userspace would get
> confused (no one bothers to reprobe really).
>
> - Add kerneldoc
> - Enforce this with some checks. This needs a somewhat ugly check
> since connectors can be added later on, but we still need to attach
> all properties before they go public.
>
> Note that we already enforce that properties themselves are created
> before the entire device is registered.
>
> Cc: Jani Nikula
> Cc: Rajat Jain
> Signed-off-by: Daniel Vetter
> ---
> drivers/gpu/drm/drm_mode_object.c | 14 ++
> 1 file changed, 14 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_mode_object.c
> b/drivers/gpu/drm/drm_mode_object.c
> index 6a23e36ed4fe..35c2719407a8 100644
> --- a/drivers/gpu/drm/drm_mode_object.c
> +++ b/drivers/gpu/drm/drm_mode_object.c
> @@ -224,12 +224,26 @@ EXPORT_SYMBOL(drm_mode_object_get);
> * This attaches the given property to the modeset object with the given
> initial
> * value. Currently this function cannot fail since the properties are
> stored in
> * a statically sized array.
> + *
> + * Note that all properties must be attached before the object itself is
> + * registered and accessible from userspace.
> */
> void drm_object_attach_property(struct drm_mode_object *obj,
> struct drm_property *property,
> uint64_t init_val)
> {
> int count = obj->properties->count;
> + struct drm_device *dev = property->dev;
> +
> +
> + if (obj->type == DRM_MODE_OBJECT_CONNECTOR) {
> + struct drm_connector *connector = obj_to_connector(obj);
> +
> + WARN_ON(!dev->driver->load &&
> + connector->registration_state ==
> DRM_CONNECTOR_REGISTERED);
> + } else {
> + WARN_ON(!dev->driver->load && dev->registered);
> + }
I'm not sure I understand why dev->driver->load needs to be a special
case. Don't the same rules apply for those drivers as well?
Thierry
>
> if (count == DRM_OBJECT_MAX_PROPERTY) {
> WARN(1, "Failed to attach object property (type: 0x%x). Please "
> --
> 2.23.0
>
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] drm/property: Enforce more lifetime rules
On Thu, Oct 24, 2019 at 12:40:55PM +0200, Thierry Reding wrote:
> On Wed, Oct 23, 2019 at 04:49:52PM +0200, Daniel Vetter wrote:
> > Properties can't be attached after registering, userspace would get
> > confused (no one bothers to reprobe really).
> >
> > - Add kerneldoc
> > - Enforce this with some checks. This needs a somewhat ugly check
> > since connectors can be added later on, but we still need to attach
> > all properties before they go public.
> >
> > Note that we already enforce that properties themselves are created
> > before the entire device is registered.
> >
> > Cc: Jani Nikula
> > Cc: Rajat Jain
> > Signed-off-by: Daniel Vetter
> > ---
> > drivers/gpu/drm/drm_mode_object.c | 14 ++
> > 1 file changed, 14 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_mode_object.c
> > b/drivers/gpu/drm/drm_mode_object.c
> > index 6a23e36ed4fe..35c2719407a8 100644
> > --- a/drivers/gpu/drm/drm_mode_object.c
> > +++ b/drivers/gpu/drm/drm_mode_object.c
> > @@ -224,12 +224,26 @@ EXPORT_SYMBOL(drm_mode_object_get);
> > * This attaches the given property to the modeset object with the given
> > initial
> > * value. Currently this function cannot fail since the properties are
> > stored in
> > * a statically sized array.
> > + *
> > + * Note that all properties must be attached before the object itself is
> > + * registered and accessible from userspace.
> > */
> > void drm_object_attach_property(struct drm_mode_object *obj,
> > struct drm_property *property,
> > uint64_t init_val)
> > {
> > int count = obj->properties->count;
> > + struct drm_device *dev = property->dev;
> > +
> > +
> > + if (obj->type == DRM_MODE_OBJECT_CONNECTOR) {
> > + struct drm_connector *connector = obj_to_connector(obj);
> > +
> > + WARN_ON(!dev->driver->load &&
> > + connector->registration_state ==
> > DRM_CONNECTOR_REGISTERED);
> > + } else {
> > + WARN_ON(!dev->driver->load && dev->registered);
> > + }
>
> I'm not sure I understand why dev->driver->load needs to be a special
> case. Don't the same rules apply for those drivers as well?
Nevermind, I just noticed that drm_dev_register() sets dev->registered
to true before calling the driver's ->load() implementation, so makes
sense:
Reviewed-by: Thierry Reding
signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm: Add support for integrated privacy screens
On Tue, Oct 22, 2019 at 05:12:06PM -0700, Rajat Jain wrote:
> Certain laptops now come with panels that have integrated privacy
> screens on them. This patch adds support for such panels by adding
> a privacy-screen property to the drm_connector for the panel, that
> the userspace can then use to control and check the status. The idea
> was discussed here:
>
> https://lkml.org/lkml/2019/10/1/786
>
> ACPI methods are used to identify, query and control privacy screen:
>
> * Identifying an ACPI object corresponding to the panel: The patch
> follows ACPI Spec 6.3 (available at
> https://uefi.org/sites/default/files/resources/ACPI_6_3_final_Jan30.pdf).
> Pages 1119 - 1123 describe what I believe, is a standard way of
> identifying / addressing "display panels" in the ACPI tables, thus
> allowing kernel to attach ACPI nodes to the panel. IMHO, this ability
> to identify and attach ACPI nodes to drm connectors may be useful for
> reasons other privacy-screens, in future.
>
> * Identifying the presence of privacy screen, and controlling it, is done
> via ACPI _DSM methods.
>
> Currently, this is done only for the Intel display ports. But in future,
> this can be done for any other ports if the hardware becomes available
> (e.g. external monitors supporting integrated privacy screens?).
>
> Also, this code can be extended in future to support non-ACPI methods
> (e.g. using a kernel GPIO driver to toggle a gpio that controls the
> privacy-screen).
>
> Signed-off-by: Rajat Jain
> ---
> drivers/gpu/drm/Makefile| 1 +
> drivers/gpu/drm/drm_atomic_uapi.c | 5 +
> drivers/gpu/drm/drm_connector.c | 38 +
> drivers/gpu/drm/drm_privacy_screen.c| 176
> drivers/gpu/drm/i915/display/intel_dp.c | 3 +
> include/drm/drm_connector.h | 18 +++
> include/drm/drm_mode_config.h | 7 +
> include/drm/drm_privacy_screen.h| 33 +
> 8 files changed, 281 insertions(+)
> create mode 100644 drivers/gpu/drm/drm_privacy_screen.c
> create mode 100644 include/drm/drm_privacy_screen.h
I like this much better than the prior proposal to use sysfs. However
the support currently looks a bit tangled. I realize that we only have a
single implementation for this in hardware right now, so there's no use
in over-engineering things, but I think we can do a better job from the
start without getting into too many abstractions. See below.
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index 82ff826b33cc..e1fc33d69bb7 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -19,6 +19,7 @@ drm-y := drm_auth.o drm_cache.o \
> drm_syncobj.o drm_lease.o drm_writeback.o drm_client.o \
> drm_client_modeset.o drm_atomic_uapi.o drm_hdcp.o
>
> +drm-$(CONFIG_ACPI) += drm_privacy_screen.o
> drm-$(CONFIG_DRM_LEGACY) += drm_legacy_misc.o drm_bufs.o drm_context.o
> drm_dma.o drm_scatter.o drm_lock.o
> drm-$(CONFIG_DRM_LIB_RANDOM) += lib/drm_random.o
> drm-$(CONFIG_DRM_VM) += drm_vm.o
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
> b/drivers/gpu/drm/drm_atomic_uapi.c
> index 7a26bfb5329c..44131165e4ea 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -30,6 +30,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
>
> @@ -766,6 +767,8 @@ static int drm_atomic_connector_set_property(struct
> drm_connector *connector,
> fence_ptr);
> } else if (property == connector->max_bpc_property) {
> state->max_requested_bpc = val;
> + } else if (property == config->privacy_screen_property) {
> + drm_privacy_screen_set_val(connector, val);
This doesn't look right. Shouldn't you store the value in the connector
state and then leave it up to the connector driver to set it
appropriately? I think that also has the advantage of untangling this
support a little.
> } else if (connector->funcs->atomic_set_property) {
> return connector->funcs->atomic_set_property(connector,
> state, property, val);
> @@ -842,6 +845,8 @@ drm_atomic_connector_get_property(struct drm_connector
> *connector,
> *val = 0;
> } else if (property == connector->max_bpc_property) {
> *val = state->max_requested_bpc;
> + } else if (property == config->privacy_screen_property) {
> + *val = drm_privacy_screen_get_val(connector);
Similarly, I think this can just return the atomic state's value for
this.
> } else if (connector->funcs->atomic_get_property) {
> return connector->funcs->atomic_get_property(connector,
> state, property, val);
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 4c766624b20d..a31e0382132b 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/
Re: [Intel-gfx] [PATCH] drm/dp: Add function to parse EDID descriptors for adaptive sync limits
On Thu, Oct 24, 2019 at 02:34:00PM +0300, Ville Syrjälä wrote:
> On Thu, Oct 24, 2019 at 12:31:06PM +0200, Thierry Reding wrote:
> > On Wed, Oct 23, 2019 at 05:00:41PM -0700, Manasi Navare wrote:
> > > Adaptive Sync is a VESA feature so add a DRM core helper to parse
> > > the EDID's detailed descritors to obtain the adaptive sync monitor range.
> > > Store this info as part fo drm_display_info so it can be used
> > > across all drivers.
> > > This part of the code is stripped out of amdgpu's function
> > > amdgpu_dm_update_freesync_caps() to make it generic and be used
> > > across all DRM drivers
> > >
> > > Cc: Ville Syrjälä
> > > Cc: Harry Wentland
> > > Cc: Clinton A Taylor
> > > Signed-off-by: Manasi Navare
> > > ---
> > > drivers/gpu/drm/drm_edid.c | 49 +
> > > include/drm/drm_connector.h | 25 +++
> > > include/drm/drm_edid.h | 2 ++
> > > 3 files changed, 76 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > > index 474ac04d5600..97dd1200773e 100644
> > > --- a/drivers/gpu/drm/drm_edid.c
> > > +++ b/drivers/gpu/drm/drm_edid.c
> > > @@ -4707,6 +4707,52 @@ static void drm_parse_cea_ext(struct drm_connector
> > > *connector,
> > > }
> > > }
> > >
> > > +void drm_get_adaptive_sync_limits(struct drm_connector *connector,
> > > + const struct edid *edid)
> > > +{
> > > + struct drm_display_info *info = &connector->display_info;
> > > + const struct detailed_timing *timing;
> > > + const struct detailed_non_pixel *data;
> > > + const struct detailed_data_monitor_range *range;
> > > + int i;
> >
> > This can be unsigned int.
>
> Please no. A loop iterator called 'i' should always be a normal signed int.
What? Where's that rule written down? In my experience it's always
better to use as restrictive a type as possible. It's really annoying
when GCC suddenly starts complaining about comparison between signed and
unsigned. So if a variable can never contain a signed value, why risk
the ambiguity? The value goes from 0 to 4, the sign bit is useless.
> > > + /*
> > > + * Restrict Adaptive Sync only for dp and edp
> > > + */
> > > + if (connector->connector_type != DRM_MODE_CONNECTOR_DisplayPort &&
> > > + connector->connector_type != DRM_MODE_CONNECTOR_eDP)
> > > + return;
> > > +
> > > + if (edid->version <= 1 && !(edid->version == 1 && edid->revision > 1))
> > > + return;
> > > +
> > > + for (i = 0; i < 4; i++) {
> > > + timing = &edid->detailed_timings[i];
> > > + data= &timing->data.other_data;
> > > + range = &data->data.range;
> > > + /*
> > > + * Check if monitor has continuous frequency mode
> > > + */
> > > + if (data->type != EDID_DETAIL_MONITOR_RANGE)
> > > + continue;
> > > + /*
> > > + * Check for flag range limits only. If flag == 1 then
> > > + * no additional timing information provided.
> > > + * Default GTF, GTF Secondary curve and CVT are not
> > > + * supported
> > > + */
> > > + if (range->flags != 1)
> > > + continue;
> > > +
> > > + info->adaptive_sync.min_vfreq = range->min_vfreq;
> > > + info->adaptive_sync.max_vfreq = range->max_vfreq;
> > > + info->adaptive_sync.pixel_clock_mhz =
> > > + range->pixel_clock_mhz * 10;
> > > + break;
> > > + }
> > > +}
> > > +EXPORT_SYMBOL(drm_get_adaptive_sync_limits);
> > > +
> > > /* A connector has no EDID information, so we've got no EDID to compute
> > > quirks from. Reset
> > > * all of the values which would have been set from EDID
> > > */
> > > @@ -4728,6 +4774,7 @@ drm_reset_display_info(struct drm_connector
> > > *connector)
> > > memset(&info->hdmi, 0, sizeof(info->hdmi));
> > >
> > > info->non_desktop = 0;
> > > + memset(&info->adaptive_sync, 0, sizeof(info->adaptive_sync));
> > > }
> > >
> > > u32 drm_add_displa
Re: [Intel-gfx] [PATCH] drm/dp: Add function to parse EDID descriptors for adaptive sync limits
On Thu, Oct 24, 2019 at 05:20:32PM +0300, Ville Syrjälä wrote:
> On Thu, Oct 24, 2019 at 03:54:41PM +0200, Thierry Reding wrote:
> > On Thu, Oct 24, 2019 at 02:34:00PM +0300, Ville Syrjälä wrote:
> > > On Thu, Oct 24, 2019 at 12:31:06PM +0200, Thierry Reding wrote:
> > > > On Wed, Oct 23, 2019 at 05:00:41PM -0700, Manasi Navare wrote:
> > > > > Adaptive Sync is a VESA feature so add a DRM core helper to parse
> > > > > the EDID's detailed descritors to obtain the adaptive sync monitor
> > > > > range.
> > > > > Store this info as part fo drm_display_info so it can be used
> > > > > across all drivers.
> > > > > This part of the code is stripped out of amdgpu's function
> > > > > amdgpu_dm_update_freesync_caps() to make it generic and be used
> > > > > across all DRM drivers
> > > > >
> > > > > Cc: Ville Syrjälä
> > > > > Cc: Harry Wentland
> > > > > Cc: Clinton A Taylor
> > > > > Signed-off-by: Manasi Navare
> > > > > ---
> > > > > drivers/gpu/drm/drm_edid.c | 49
> > > > > +
> > > > > include/drm/drm_connector.h | 25 +++
> > > > > include/drm/drm_edid.h | 2 ++
> > > > > 3 files changed, 76 insertions(+)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > > > > index 474ac04d5600..97dd1200773e 100644
> > > > > --- a/drivers/gpu/drm/drm_edid.c
> > > > > +++ b/drivers/gpu/drm/drm_edid.c
> > > > > @@ -4707,6 +4707,52 @@ static void drm_parse_cea_ext(struct
> > > > > drm_connector *connector,
> > > > > }
> > > > > }
> > > > >
> > > > > +void drm_get_adaptive_sync_limits(struct drm_connector *connector,
> > > > > + const struct edid *edid)
> > > > > +{
> > > > > + struct drm_display_info *info = &connector->display_info;
> > > > > + const struct detailed_timing *timing;
> > > > > + const struct detailed_non_pixel *data;
> > > > > + const struct detailed_data_monitor_range *range;
> > > > > + int i;
> > > >
> > > > This can be unsigned int.
> > >
> > > Please no. A loop iterator called 'i' should always be a normal signed
> > > int.
> >
> > What? Where's that rule written down? In my experience it's always
> > better to use as restrictive a type as possible. It's really annoying
> > when GCC suddenly starts complaining about comparison between signed and
> > unsigned. So if a variable can never contain a signed value, why risk
> > the ambiguity? The value goes from 0 to 4, the sign bit is useless.
>
> Dunno if it's really written down anywhere. It's just something
> experience has taught. IIRC there's also a rant from Linus about this
> somewhere. Hm, can't find that one right now, but Andrew Morton also
> seems to agree: https://lwn.net/Articles/309279/
> Ah, here is one Linus rant about unsigned array indexes:
> https://yarchive.net/comp/linux/gcc.html
It's interesting that none of those actually give a real reason why
unsigned int shouldn't be used for variables called i.
> My opinion: unsigned is an very *dangerous* keyword in C that demands
> your respect. You should never use it without thinking first what the
> ramifications are. You always have to have the promotion rules clear
> in you mind when you do any kind of arithmetic with >= unsigned int
> type. And common idioms such as 'int i' should be respected so as to
> not cause unexpected hair loss to other developers when they decide
> to make the loop iterate backwards.
I would argue that when you do things like make a loop iterate backwards
you better know what variable types you're dealing with.
Anyway, this is clearly very subjective, so feel free to let this be int
if you prefer.
> > > > > + /*
> > > > > + * Restrict Adaptive Sync only for dp and edp
> > > > > + */
> > > > > + if (connector->connector_type != DRM_MODE_CONNECTOR_DisplayPort
> > > > > &&
> > > > > + connector->connector_type != DRM_MODE_CONNECTOR_eDP)
> > > > > + return;
> > > > > +
> > > > >
Re: [Intel-gfx] [PATCH] drm: Add support for integrated privacy screens
On Thu, Oct 24, 2019 at 01:45:16PM -0700, Rajat Jain wrote:
> Hi,
>
> Thanks for your review and comments. Please see inline below.
>
> On Thu, Oct 24, 2019 at 4:20 AM Thierry Reding
> wrote:
> >
> > On Tue, Oct 22, 2019 at 05:12:06PM -0700, Rajat Jain wrote:
> > > Certain laptops now come with panels that have integrated privacy
> > > screens on them. This patch adds support for such panels by adding
> > > a privacy-screen property to the drm_connector for the panel, that
> > > the userspace can then use to control and check the status. The idea
> > > was discussed here:
> > >
> > > https://lkml.org/lkml/2019/10/1/786
> > >
> > > ACPI methods are used to identify, query and control privacy screen:
> > >
> > > * Identifying an ACPI object corresponding to the panel: The patch
> > > follows ACPI Spec 6.3 (available at
> > > https://uefi.org/sites/default/files/resources/ACPI_6_3_final_Jan30.pdf).
> > > Pages 1119 - 1123 describe what I believe, is a standard way of
> > > identifying / addressing "display panels" in the ACPI tables, thus
> > > allowing kernel to attach ACPI nodes to the panel. IMHO, this ability
> > > to identify and attach ACPI nodes to drm connectors may be useful for
> > > reasons other privacy-screens, in future.
> > >
> > > * Identifying the presence of privacy screen, and controlling it, is done
> > > via ACPI _DSM methods.
> > >
> > > Currently, this is done only for the Intel display ports. But in future,
> > > this can be done for any other ports if the hardware becomes available
> > > (e.g. external monitors supporting integrated privacy screens?).
> > >
> > > Also, this code can be extended in future to support non-ACPI methods
> > > (e.g. using a kernel GPIO driver to toggle a gpio that controls the
> > > privacy-screen).
> > >
> > > Signed-off-by: Rajat Jain
> > > ---
> > > drivers/gpu/drm/Makefile| 1 +
> > > drivers/gpu/drm/drm_atomic_uapi.c | 5 +
> > > drivers/gpu/drm/drm_connector.c | 38 +
> > > drivers/gpu/drm/drm_privacy_screen.c| 176
> > > drivers/gpu/drm/i915/display/intel_dp.c | 3 +
> > > include/drm/drm_connector.h | 18 +++
> > > include/drm/drm_mode_config.h | 7 +
> > > include/drm/drm_privacy_screen.h| 33 +
> > > 8 files changed, 281 insertions(+)
> > > create mode 100644 drivers/gpu/drm/drm_privacy_screen.c
> > > create mode 100644 include/drm/drm_privacy_screen.h
> >
> > I like this much better than the prior proposal to use sysfs. However
> > the support currently looks a bit tangled. I realize that we only have a
> > single implementation for this in hardware right now, so there's no use
> > in over-engineering things, but I think we can do a better job from the
> > start without getting into too many abstractions. See below.
> >
> > > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> > > index 82ff826b33cc..e1fc33d69bb7 100644
> > > --- a/drivers/gpu/drm/Makefile
> > > +++ b/drivers/gpu/drm/Makefile
> > > @@ -19,6 +19,7 @@ drm-y := drm_auth.o drm_cache.o \
> > > drm_syncobj.o drm_lease.o drm_writeback.o drm_client.o \
> > > drm_client_modeset.o drm_atomic_uapi.o drm_hdcp.o
> > >
> > > +drm-$(CONFIG_ACPI) += drm_privacy_screen.o
> > > drm-$(CONFIG_DRM_LEGACY) += drm_legacy_misc.o drm_bufs.o drm_context.o
> > > drm_dma.o drm_scatter.o drm_lock.o
> > > drm-$(CONFIG_DRM_LIB_RANDOM) += lib/drm_random.o
> > > drm-$(CONFIG_DRM_VM) += drm_vm.o
> > > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
> > > b/drivers/gpu/drm/drm_atomic_uapi.c
> > > index 7a26bfb5329c..44131165e4ea 100644
> > > --- a/drivers/gpu/drm/drm_atomic_uapi.c
> > > +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> > > @@ -30,6 +30,7 @@
> > > #include
> > > #include
> > > #include
> > > +#include
> > > #include
> > > #include
> > >
> > > @@ -766,6 +767,8 @@ static int drm_atomic_connector_set_property(struct
> > > drm_connector *connector,
> > > fence_ptr);
> > > } else if (property == connector->max_bpc_property) {
> > > state->max_requested_bpc = val;
> > > + } else if (property ==
[Intel-gfx] [RESEND PATCH] drm/dp: Increase link status size
From: Thierry Reding
The current link status contains bytes 0x202 through 0x207, but we also
want to make sure to include the DP_ADJUST_REQUEST_POST_CURSOR2 (0x20c)
so that the post-cursor adjustment can be queried during link training.
Reported-by: coverity-bot
Addresses-Coverity-ID: 1487366 ("Memory - corruptions")
Fixes: 79465e0ffeb9 ("drm/dp: Add helper to get post-cursor adjustments")
Signed-off-by: Thierry Reding
---
include/drm/drm_dp_helper.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index 51ecb5112ef8..9581dec900ba 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -1121,7 +1121,7 @@
#define DP_MST_PHYSICAL_PORT_0 0
#define DP_MST_LOGICAL_PORT_0 8
-#define DP_LINK_STATUS_SIZE 6
+#define DP_LINK_STATUS_SIZE 11
bool drm_dp_channel_eq_ok(const u8 link_status[DP_LINK_STATUS_SIZE],
int lane_count);
bool drm_dp_clock_recovery_ok(const u8 link_status[DP_LINK_STATUS_SIZE],
--
2.23.0
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/dp: Increase link status size
On Tue, Oct 29, 2019 at 03:32:41PM +0200, Jani Nikula wrote: > On Tue, 29 Oct 2019, Thierry Reding wrote: > > From: Thierry Reding > > > > The current link status contains bytes 0x202 through 0x207, but we also > > want to make sure to include the DP_ADJUST_REQUEST_POST_CURSOR2 (0x20c) > > so that the post-cursor adjustment can be queried during link training. > > We don't currently use this in i915 (we probably should), so the impact > here is that we'll just read more DPCD than before. I quickly perused > i915, and this does not appear to directly break anything. I think the > change is probably fine, but at the same time it freaks me out a bit... > > If you don't mind, please resend this with Cc: > intel-gfx@lists.freedesktop.org to have our CI crunch through it across > a number of platforms. Would give me a warm fuzzy feeling. :) > > With the caveat that I didn't look at any other drivers besides i915, > > Reviewed-by: Jani Nikula Done, thanks. Thierry signature.asc Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 1/5] drm/dsi: clean up DSI data type definitions
On Mon, Oct 28, 2019 at 05:00:43PM +0200, Jani Nikula wrote:
> Rename picture parameter set (it's a long packet, not a long write) and
> compression mode (it's not a DCS command) enumerations according to the
> DSI specification. Order the types according to the spec. Use tabs
> instead of spaces for indentation. Use all lower case for hex.
>
> Cc: Vandita Kulkarni
> Reviewed-by: Vandita Kulkarni
> Signed-off-by: Jani Nikula
> ---
> drivers/gpu/drm/drm_mipi_dsi.c | 4 ++--
> include/video/mipi_display.h | 10 +-
> 2 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
> index bd2498bbd74a..f237d80828c3 100644
> --- a/drivers/gpu/drm/drm_mipi_dsi.c
> +++ b/drivers/gpu/drm/drm_mipi_dsi.c
> @@ -373,6 +373,7 @@ bool mipi_dsi_packet_format_is_short(u8 type)
> case MIPI_DSI_V_SYNC_END:
> case MIPI_DSI_H_SYNC_START:
> case MIPI_DSI_H_SYNC_END:
> + case MIPI_DSI_COMPRESSION_MODE:
> case MIPI_DSI_END_OF_TRANSMISSION:
> case MIPI_DSI_COLOR_MODE_OFF:
> case MIPI_DSI_COLOR_MODE_ON:
> @@ -387,7 +388,6 @@ bool mipi_dsi_packet_format_is_short(u8 type)
> case MIPI_DSI_DCS_SHORT_WRITE:
> case MIPI_DSI_DCS_SHORT_WRITE_PARAM:
> case MIPI_DSI_DCS_READ:
> - case MIPI_DSI_DCS_COMPRESSION_MODE:
> case MIPI_DSI_SET_MAXIMUM_RETURN_PACKET_SIZE:
> return true;
> }
> @@ -406,11 +406,11 @@ EXPORT_SYMBOL(mipi_dsi_packet_format_is_short);
> bool mipi_dsi_packet_format_is_long(u8 type)
> {
> switch (type) {
> - case MIPI_DSI_PPS_LONG_WRITE:
> case MIPI_DSI_NULL_PACKET:
> case MIPI_DSI_BLANKING_PACKET:
> case MIPI_DSI_GENERIC_LONG_WRITE:
> case MIPI_DSI_DCS_LONG_WRITE:
> + case MIPI_DSI_PICTURE_PARAMETER_SET:
> case MIPI_DSI_LOOSELY_PACKED_PIXEL_STREAM_YCBCR20:
> case MIPI_DSI_PACKED_PIXEL_STREAM_YCBCR24:
> case MIPI_DSI_PACKED_PIXEL_STREAM_YCBCR16:
> diff --git a/include/video/mipi_display.h b/include/video/mipi_display.h
> index cba57a678daf..79fd71cf4934 100644
> --- a/include/video/mipi_display.h
> +++ b/include/video/mipi_display.h
> @@ -17,6 +17,9 @@ enum {
> MIPI_DSI_H_SYNC_START = 0x21,
> MIPI_DSI_H_SYNC_END = 0x31,
>
> + MIPI_DSI_COMPRESSION_MODE = 0x07,
> + MIPI_DSI_END_OF_TRANSMISSION= 0x08,
> +
> MIPI_DSI_COLOR_MODE_OFF = 0x02,
> MIPI_DSI_COLOR_MODE_ON = 0x12,
> MIPI_DSI_SHUTDOWN_PERIPHERAL= 0x22,
> @@ -35,18 +38,15 @@ enum {
>
> MIPI_DSI_DCS_READ = 0x06,
>
> - MIPI_DSI_DCS_COMPRESSION_MODE = 0x07,
> - MIPI_DSI_PPS_LONG_WRITE = 0x0A,
> -
> MIPI_DSI_SET_MAXIMUM_RETURN_PACKET_SIZE = 0x37,
>
> - MIPI_DSI_END_OF_TRANSMISSION= 0x08,
> -
> MIPI_DSI_NULL_PACKET= 0x09,
> MIPI_DSI_BLANKING_PACKET= 0x19,
> MIPI_DSI_GENERIC_LONG_WRITE = 0x29,
> MIPI_DSI_DCS_LONG_WRITE = 0x39,
>
> + MIPI_DSI_PICTURE_PARAMETER_SET = 0x0a,
> +
> MIPI_DSI_LOOSELY_PACKED_PIXEL_STREAM_YCBCR20= 0x0c,
> MIPI_DSI_PACKED_PIXEL_STREAM_YCBCR24= 0x1c,
> MIPI_DSI_PACKED_PIXEL_STREAM_YCBCR16= 0x2c,
Looks good to me. I haven't specifically checked that the order matches
that in the specification, but given that it's not really ordered in any
sane way in the first place (or perhaps I'm too stupid to see the logic)
I don't really mind about the order.
Took me a while to find the specification. You might want to mention in
the commit message that some of these enumerations are only available in
the DSI 2 specification because I was looking at 1.2 first. Anyway, the
enumerations and names match what's in the spec, so:
Reviewed-by: Thierry Reding
signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 2/5] drm/dsi: add missing DSI data types
On Mon, Oct 28, 2019 at 05:00:44PM +0200, Jani Nikula wrote:
> Add execute queue and compressed pixel stream packet data types for
> completeness.
>
> Cc: Vandita Kulkarni
> Signed-off-by: Jani Nikula
> ---
> drivers/gpu/drm/drm_mipi_dsi.c | 2 ++
> include/video/mipi_display.h | 2 ++
> 2 files changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
> index f237d80828c3..3f33f02571fd 100644
> --- a/drivers/gpu/drm/drm_mipi_dsi.c
> +++ b/drivers/gpu/drm/drm_mipi_dsi.c
> @@ -388,6 +388,7 @@ bool mipi_dsi_packet_format_is_short(u8 type)
> case MIPI_DSI_DCS_SHORT_WRITE:
> case MIPI_DSI_DCS_SHORT_WRITE_PARAM:
> case MIPI_DSI_DCS_READ:
> + case MIPI_DSI_EXECUTE_QUEUE:
> case MIPI_DSI_SET_MAXIMUM_RETURN_PACKET_SIZE:
> return true;
> }
> @@ -411,6 +412,7 @@ bool mipi_dsi_packet_format_is_long(u8 type)
> case MIPI_DSI_GENERIC_LONG_WRITE:
> case MIPI_DSI_DCS_LONG_WRITE:
> case MIPI_DSI_PICTURE_PARAMETER_SET:
> + case MIPI_DSI_COMPRESSED_PIXEL_STREAM:
> case MIPI_DSI_LOOSELY_PACKED_PIXEL_STREAM_YCBCR20:
> case MIPI_DSI_PACKED_PIXEL_STREAM_YCBCR24:
> case MIPI_DSI_PACKED_PIXEL_STREAM_YCBCR16:
> diff --git a/include/video/mipi_display.h b/include/video/mipi_display.h
> index 79fd71cf4934..6b6390dfa203 100644
> --- a/include/video/mipi_display.h
> +++ b/include/video/mipi_display.h
> @@ -37,6 +37,7 @@ enum {
> MIPI_DSI_DCS_SHORT_WRITE_PARAM = 0x15,
>
> MIPI_DSI_DCS_READ = 0x06,
> + MIPI_DSI_EXECUTE_QUEUE = 0x16,
>
> MIPI_DSI_SET_MAXIMUM_RETURN_PACKET_SIZE = 0x37,
>
> @@ -46,6 +47,7 @@ enum {
> MIPI_DSI_DCS_LONG_WRITE = 0x39,
>
> MIPI_DSI_PICTURE_PARAMETER_SET = 0x0a,
> + MIPI_DSI_COMPRESSED_PIXEL_STREAM= 0x0b,
>
> MIPI_DSI_LOOSELY_PACKED_PIXEL_STREAM_YCBCR20= 0x0c,
> MIPI_DSI_PACKED_PIXEL_STREAM_YCBCR24= 0x1c,
Actually, it looks like the ordering is by lowest-significant nibble
first, then by highest-significant nibble, so maybe there's some logic
to this after all.
Hmm... that's mostly true, except for 0x07 and 0x08... anyway, the new
enumeration values and names match the specification, so:
Reviewed-by: Thierry Reding
signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 3/5] drm/dsi: add missing DSI DCS commands
On Mon, Oct 28, 2019 at 05:00:45PM +0200, Jani Nikula wrote:
> Update from the DCS specification.
>
> Cc: Vandita Kulkarni
> Signed-off-by: Jani Nikula
> ---
> include/video/mipi_display.h | 10 ++
> 1 file changed, 10 insertions(+)
>
> diff --git a/include/video/mipi_display.h b/include/video/mipi_display.h
> index 6b6390dfa203..928f8c4b6658 100644
> --- a/include/video/mipi_display.h
> +++ b/include/video/mipi_display.h
> @@ -79,7 +79,9 @@ enum {
> enum {
> MIPI_DCS_NOP= 0x00,
> MIPI_DCS_SOFT_RESET = 0x01,
> + MIPI_DCS_GET_COMPRESSION_MODE = 0x03,
> MIPI_DCS_GET_DISPLAY_ID = 0x04,
> + MIPI_DCS_GET_ERROR_COUNT_ON_DSI = 0x05,
> MIPI_DCS_GET_RED_CHANNEL= 0x06,
> MIPI_DCS_GET_GREEN_CHANNEL = 0x07,
> MIPI_DCS_GET_BLUE_CHANNEL = 0x08,
> @@ -94,6 +96,8 @@ enum {
> MIPI_DCS_EXIT_SLEEP_MODE= 0x11,
> MIPI_DCS_ENTER_PARTIAL_MODE = 0x12,
> MIPI_DCS_ENTER_NORMAL_MODE = 0x13,
> + MIPI_DCS_GET_IMAGE_CHECKSUM_RGB = 0x14,
> + MIPI_DCS_GET_IMAGE_CHECKSUM_CT = 0x15,
> MIPI_DCS_EXIT_INVERT_MODE = 0x20,
> MIPI_DCS_ENTER_INVERT_MODE = 0x21,
> MIPI_DCS_SET_GAMMA_CURVE= 0x26,
> @@ -105,6 +109,7 @@ enum {
> MIPI_DCS_WRITE_LUT = 0x2D,
> MIPI_DCS_READ_MEMORY_START = 0x2E,
> MIPI_DCS_SET_PARTIAL_AREA = 0x30,
> + MIPI_DCS_SET_PARTIAL_COLUMNS= 0x31,
> MIPI_DCS_SET_SCROLL_AREA= 0x33,
> MIPI_DCS_SET_TEAR_OFF = 0x34,
> MIPI_DCS_SET_TEAR_ON= 0x35,
> @@ -114,7 +119,10 @@ enum {
> MIPI_DCS_ENTER_IDLE_MODE= 0x39,
> MIPI_DCS_SET_PIXEL_FORMAT = 0x3A,
> MIPI_DCS_WRITE_MEMORY_CONTINUE = 0x3C,
> + MIPI_DCS_SET_3D_CONTROL = 0x3D,
> MIPI_DCS_READ_MEMORY_CONTINUE = 0x3E,
> + MIPI_DCS_GET_3D_CONTROL = 0x3F,
> + MIPI_DCS_SET_VSYNC_TIMING = 0x40,
> MIPI_DCS_SET_TEAR_SCANLINE = 0x44,
> MIPI_DCS_GET_SCANLINE = 0x45,
> MIPI_DCS_SET_DISPLAY_BRIGHTNESS = 0x51, /* MIPI DCS 1.3 */
> @@ -126,7 +134,9 @@ enum {
> MIPI_DCS_SET_CABC_MIN_BRIGHTNESS = 0x5E,/* MIPI DCS 1.3 */
> MIPI_DCS_GET_CABC_MIN_BRIGHTNESS = 0x5F,/* MIPI DCS 1.3 */
> MIPI_DCS_READ_DDB_START = 0xA1,
> + MIPI_DCS_READ_PPS_START = 0xA2,
> MIPI_DCS_READ_DDB_CONTINUE = 0xA8,
> + MIPI_DCS_READ_PPS_CONTINUE = 0xA9,
> };
>
> /* MIPI DCS pixel formats */
> --
> 2.20.1
I only have a copy of DCS 1.2, which doesn't seem to have these values.
Let me see if I can find a more updated version.
Thierry
signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 3/5] drm/dsi: add missing DSI DCS commands
On Mon, Oct 28, 2019 at 05:00:45PM +0200, Jani Nikula wrote:
> Update from the DCS specification.
>
> Cc: Vandita Kulkarni
> Signed-off-by: Jani Nikula
> ---
> include/video/mipi_display.h | 10 ++
> 1 file changed, 10 insertions(+)
>
> diff --git a/include/video/mipi_display.h b/include/video/mipi_display.h
> index 6b6390dfa203..928f8c4b6658 100644
> --- a/include/video/mipi_display.h
> +++ b/include/video/mipi_display.h
> @@ -79,7 +79,9 @@ enum {
> enum {
> MIPI_DCS_NOP= 0x00,
> MIPI_DCS_SOFT_RESET = 0x01,
> + MIPI_DCS_GET_COMPRESSION_MODE = 0x03,
> MIPI_DCS_GET_DISPLAY_ID = 0x04,
> + MIPI_DCS_GET_ERROR_COUNT_ON_DSI = 0x05,
> MIPI_DCS_GET_RED_CHANNEL= 0x06,
> MIPI_DCS_GET_GREEN_CHANNEL = 0x07,
> MIPI_DCS_GET_BLUE_CHANNEL = 0x08,
> @@ -94,6 +96,8 @@ enum {
> MIPI_DCS_EXIT_SLEEP_MODE= 0x11,
> MIPI_DCS_ENTER_PARTIAL_MODE = 0x12,
> MIPI_DCS_ENTER_NORMAL_MODE = 0x13,
> + MIPI_DCS_GET_IMAGE_CHECKSUM_RGB = 0x14,
> + MIPI_DCS_GET_IMAGE_CHECKSUM_CT = 0x15,
> MIPI_DCS_EXIT_INVERT_MODE = 0x20,
> MIPI_DCS_ENTER_INVERT_MODE = 0x21,
> MIPI_DCS_SET_GAMMA_CURVE= 0x26,
> @@ -105,6 +109,7 @@ enum {
> MIPI_DCS_WRITE_LUT = 0x2D,
> MIPI_DCS_READ_MEMORY_START = 0x2E,
> MIPI_DCS_SET_PARTIAL_AREA = 0x30,
> + MIPI_DCS_SET_PARTIAL_COLUMNS= 0x31,
> MIPI_DCS_SET_SCROLL_AREA= 0x33,
> MIPI_DCS_SET_TEAR_OFF = 0x34,
> MIPI_DCS_SET_TEAR_ON= 0x35,
> @@ -114,7 +119,10 @@ enum {
> MIPI_DCS_ENTER_IDLE_MODE= 0x39,
> MIPI_DCS_SET_PIXEL_FORMAT = 0x3A,
> MIPI_DCS_WRITE_MEMORY_CONTINUE = 0x3C,
> + MIPI_DCS_SET_3D_CONTROL = 0x3D,
> MIPI_DCS_READ_MEMORY_CONTINUE = 0x3E,
> + MIPI_DCS_GET_3D_CONTROL = 0x3F,
> + MIPI_DCS_SET_VSYNC_TIMING = 0x40,
> MIPI_DCS_SET_TEAR_SCANLINE = 0x44,
> MIPI_DCS_GET_SCANLINE = 0x45,
> MIPI_DCS_SET_DISPLAY_BRIGHTNESS = 0x51, /* MIPI DCS 1.3 */
> @@ -126,7 +134,9 @@ enum {
> MIPI_DCS_SET_CABC_MIN_BRIGHTNESS = 0x5E,/* MIPI DCS 1.3 */
> MIPI_DCS_GET_CABC_MIN_BRIGHTNESS = 0x5F,/* MIPI DCS 1.3 */
> MIPI_DCS_READ_DDB_START = 0xA1,
> + MIPI_DCS_READ_PPS_START = 0xA2,
> MIPI_DCS_READ_DDB_CONTINUE = 0xA8,
> + MIPI_DCS_READ_PPS_CONTINUE = 0xA9,
> };
>
> /* MIPI DCS pixel formats */
Okay, found a copy of DCS v1.4 and the above matches the specification,
so:
Reviewed-by: Thierry Reding
Does it perhaps make sense to add comments about the version number that
these were introduced with?
Thierry
signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 4/5] drm/dsi: rename MIPI_DCS_SET_PARTIAL_AREA to MIPI_DCS_SET_PARTIAL_ROWS
On Mon, Oct 28, 2019 at 05:00:46PM +0200, Jani Nikula wrote: > The DCS command has been named SET_PARTIAL_ROWS in the DCS spec since > v1.02, for more than a decade. Rename the enumeration to match the spec. > > v2: add comment about the rename (David Lechner) > > Cc: David Lechner > Cc: Vandita Kulkarni > Signed-off-by: Jani Nikula > --- > drivers/gpu/drm/tiny/st7586.c | 2 +- > include/video/mipi_display.h | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) Reviewed-by: Thierry Reding signature.asc Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 5/5] drm/dsi: add helpers for DSI compression mode and PPS packets
On Mon, Oct 28, 2019 at 05:00:47PM +0200, Jani Nikula wrote: > Add helper functions for sending the DSI compression mode and picture > parameter set data type packets. For the time being, limit the support > to using VESA DSC 1.1 and the default PPS. This may need updating if the > need arises for proprietary compression or non-default PPS, however keep > it simple for starters. > > v2: Add missing EXPORT_SYMBOL > > Cc: Vandita Kulkarni > Signed-off-by: Jani Nikula > --- > drivers/gpu/drm/drm_mipi_dsi.c | 51 ++ > include/drm/drm_mipi_dsi.h | 4 +++ > 2 files changed, 55 insertions(+) Looks good: Reviewed-by: Thierry Reding signature.asc Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v6 08/17] drm/ttm: use gem vma_node
On Mon, Aug 05, 2019 at 04:01:10PM +0200, Gerd Hoffmann wrote:
> Drop vma_node from ttm_buffer_object, use the gem struct
> (base.vma_node) instead.
>
> Signed-off-by: Gerd Hoffmann
> Reviewed-by: Christian König
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_object.h | 2 +-
> drivers/gpu/drm/qxl/qxl_object.h | 2 +-
> drivers/gpu/drm/radeon/radeon_object.h | 2 +-
> drivers/gpu/drm/virtio/virtgpu_drv.h | 2 +-
> include/drm/ttm/ttm_bo_api.h | 4
> drivers/gpu/drm/drm_gem_vram_helper.c | 2 +-
> drivers/gpu/drm/nouveau/nouveau_display.c | 2 +-
> drivers/gpu/drm/nouveau/nouveau_gem.c | 2 +-
> drivers/gpu/drm/ttm/ttm_bo.c | 8
> drivers/gpu/drm/ttm/ttm_bo_util.c | 2 +-
> drivers/gpu/drm/ttm/ttm_bo_vm.c| 9 +
> drivers/gpu/drm/virtio/virtgpu_prime.c | 3 ---
> drivers/gpu/drm/vmwgfx/vmwgfx_bo.c | 4 ++--
> drivers/gpu/drm/vmwgfx/vmwgfx_surface.c| 4 ++--
> 14 files changed, 21 insertions(+), 27 deletions(-)
Hi Gerd,
I've been seeing a regression on Nouveau with recent linux-next releases
and git bisect points at this commit as the first bad one. If I revert
it (there's a tiny conflict with a patch that was merged subsequently),
things are back to normal.
I think the reason for this issue is that Nouveau doesn't use GEM
objects for all buffer objects, and even when it uses GEM objects, the
code will not initialize the GEM object until after the buffer objects
and the backing TTM objects have been created.
I tried to fix that by making sure drm_gem_object_init() gets called by
Nouveau before ttm_bo_init(), but the changes are fairly involved and I
was unable to get the GEM reference counting right. I can look into the
proper fix some more, but it might be worth reverting this patch for
now to get Nouveau working again.
Thierry
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> index 645a189d365c..113fb2feb437 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> @@ -191,7 +191,7 @@ static inline unsigned
> amdgpu_bo_gpu_page_alignment(struct amdgpu_bo *bo)
> */
> static inline u64 amdgpu_bo_mmap_offset(struct amdgpu_bo *bo)
> {
> - return drm_vma_node_offset_addr(&bo->tbo.vma_node);
> + return drm_vma_node_offset_addr(&bo->tbo.base.vma_node);
> }
>
> /**
> diff --git a/drivers/gpu/drm/qxl/qxl_object.h
> b/drivers/gpu/drm/qxl/qxl_object.h
> index b812d4ae9d0d..8ae54ba7857c 100644
> --- a/drivers/gpu/drm/qxl/qxl_object.h
> +++ b/drivers/gpu/drm/qxl/qxl_object.h
> @@ -60,7 +60,7 @@ static inline unsigned long qxl_bo_size(struct qxl_bo *bo)
>
> static inline u64 qxl_bo_mmap_offset(struct qxl_bo *bo)
> {
> - return drm_vma_node_offset_addr(&bo->tbo.vma_node);
> + return drm_vma_node_offset_addr(&bo->tbo.base.vma_node);
> }
>
> static inline int qxl_bo_wait(struct qxl_bo *bo, u32 *mem_type,
> diff --git a/drivers/gpu/drm/radeon/radeon_object.h
> b/drivers/gpu/drm/radeon/radeon_object.h
> index 9ffd8215d38a..e5554bf9140e 100644
> --- a/drivers/gpu/drm/radeon/radeon_object.h
> +++ b/drivers/gpu/drm/radeon/radeon_object.h
> @@ -116,7 +116,7 @@ static inline unsigned
> radeon_bo_gpu_page_alignment(struct radeon_bo *bo)
> */
> static inline u64 radeon_bo_mmap_offset(struct radeon_bo *bo)
> {
> - return drm_vma_node_offset_addr(&bo->tbo.vma_node);
> + return drm_vma_node_offset_addr(&bo->tbo.base.vma_node);
> }
>
> extern int radeon_bo_wait(struct radeon_bo *bo, u32 *mem_type,
> diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h
> b/drivers/gpu/drm/virtio/virtgpu_drv.h
> index f4ecea6054ba..e28829661724 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_drv.h
> +++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
> @@ -396,7 +396,7 @@ static inline void virtio_gpu_object_unref(struct
> virtio_gpu_object **bo)
>
> static inline u64 virtio_gpu_object_mmap_offset(struct virtio_gpu_object *bo)
> {
> - return drm_vma_node_offset_addr(&bo->tbo.vma_node);
> + return drm_vma_node_offset_addr(&bo->tbo.base.vma_node);
> }
>
> static inline int virtio_gpu_object_reserve(struct virtio_gpu_object *bo,
> diff --git a/include/drm/ttm/ttm_bo_api.h b/include/drm/ttm/ttm_bo_api.h
> index fa050f0328ab..7ffc50a3303d 100644
> --- a/include/drm/ttm/ttm_bo_api.h
> +++ b/include/drm/ttm/ttm_bo_api.h
> @@ -152,7 +152,6 @@ struct ttm_tt;
> * @ddestroy: List head for the delayed destroy list.
> * @swap: List head for swap LRU list.
> * @moving: Fence set when BO is moving
> - * @vma_node: Address space manager node.
> * @offset: The current GPU offset, which can have different meanings
> * depending on the memory type. For SYSTEM type memory, it should be 0.
> * @cur_placement: Hint of current placement.
> @@ -219,9 +218,6 @@ struct ttm_buffer_object {
>*/
>
> struct dma_fence *moving;
> -
> - struct drm_vma_offset_node vma_node
Re: [Intel-gfx] [PATCH v6 08/17] drm/ttm: use gem vma_node
On Wed, Aug 14, 2019 at 07:58:27AM +0200, Gerd Hoffmann wrote:
> > Hi Gerd,
> >
> > I've been seeing a regression on Nouveau with recent linux-next releases
> > and git bisect points at this commit as the first bad one. If I revert
> > it (there's a tiny conflict with a patch that was merged subsequently),
> > things are back to normal.
> >
> > I think the reason for this issue is that Nouveau doesn't use GEM
> > objects for all buffer objects,
>
> That shouldn't be a problem ...
>
> > and even when it uses GEM objects, the
> > code will not initialize the GEM object until after the buffer objects
> > and the backing TTM objects have been created.
>
> ... but the initialization order is.
>
> ttm_bo_uses_embedded_gem_object() assumes gem gets initialized first.
>
> drm_gem_object_init() init calling drm_vma_node_reset() again is
> probably the root cause for the breakage.
>
> > I tried to fix that by making sure drm_gem_object_init() gets called by
> > Nouveau before ttm_bo_init(), but the changes are fairly involved and I
> > was unable to get the GEM reference counting right. I can look into the
> > proper fix some more, but it might be worth reverting this patch for
> > now to get Nouveau working again.
>
> Changing the order doesn't look hard. Patch attached (untested, have no
> test hardware). But maybe I missed some detail ...
>
> The other patch attached works around the issue with a flag, to avoid
> drm_vma_node_reset() being called twice.
I came up with something very similar by splitting up nouveau_bo_new()
into allocation and initialization steps, so that when necessary the GEM
object can be initialized in between. I think that's slightly more
flexible and easier to understand than a boolean flag.
Thierry
From a1130a6affcb7c00133e89f3e498cb6757f5bb51 Mon Sep 17 00:00:00 2001
From: Thierry Reding
Date: Wed, 14 Aug 2019 11:00:48 +0200
Subject: [PATCH] drm/nouveau: Initialize GEM object before TTM object
TTM assumes that drivers initialize the embedded GEM object before
calling the ttm_bo_init() function. This is not currently the case
in the Nouveau driver. Fix this by splitting up nouveau_bo_new()
into nouveau_bo_alloc() and nouveau_bo_init() so that the GEM can
be initialized before TTM BO initialization when necessary.
Fixes: b96f3e7c8069 ("drm/ttm: use gem vma_node")
Signed-off-by: Thierry Reding
---
drivers/gpu/drm/nouveau/nouveau_bo.c| 69 -
drivers/gpu/drm/nouveau/nouveau_bo.h| 4 ++
drivers/gpu/drm/nouveau/nouveau_gem.c | 29 ++-
drivers/gpu/drm/nouveau/nouveau_prime.c | 16 --
4 files changed, 77 insertions(+), 41 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c
b/drivers/gpu/drm/nouveau/nouveau_bo.c
index 99e391be9370..b3d3e07de1af 100644
--- a/drivers/gpu/drm/nouveau/nouveau_bo.c
+++ b/drivers/gpu/drm/nouveau/nouveau_bo.c
@@ -185,31 +185,24 @@ nouveau_bo_fixup_align(struct nouveau_bo *nvbo, u32 flags,
*size = roundup_64(*size, PAGE_SIZE);
}
-int
-nouveau_bo_new(struct nouveau_cli *cli, u64 size, int align,
- uint32_t flags, uint32_t tile_mode, uint32_t tile_flags,
- struct sg_table *sg, struct reservation_object *robj,
- struct nouveau_bo **pnvbo)
+struct nouveau_bo *
+nouveau_bo_alloc(struct nouveau_cli *cli, u64 size, u32 flags, u32 tile_mode,
+u32 tile_flags)
{
struct nouveau_drm *drm = cli->drm;
struct nouveau_bo *nvbo;
struct nvif_mmu *mmu = &cli->mmu;
struct nvif_vmm *vmm = cli->svm.cli ? &cli->svm.vmm : &cli->vmm.vmm;
- size_t acc_size;
- int type = ttm_bo_type_device;
- int ret, i, pi = -1;
+ int i, pi = -1;
if (!size) {
NV_WARN(drm, "skipped size %016llx\n", size);
- return -EINVAL;
+ return ERR_PTR(-EINVAL);
}
- if (sg)
- type = ttm_bo_type_sg;
-
nvbo = kzalloc(sizeof(struct nouveau_bo), GFP_KERNEL);
if (!nvbo)
- return -ENOMEM;
+ return ERR_PTR(-ENOMEM);
INIT_LIST_HEAD(&nvbo->head);
INIT_LIST_HEAD(&nvbo->entry);
INIT_LIST_HEAD(&nvbo->vma_list);
@@ -231,7 +224,7 @@ nouveau_bo_new(struct nouveau_cli *cli, u64 size, int align,
nvbo->kind = (tile_flags & 0xff00) >> 8;
if (!nvif_mmu_kind_valid(mmu, nvbo->kind)) {
kfree(nvbo);
- return -EINVAL;
+ return ERR_PTR(-EINVAL);
}
nvbo->comp = mmu->kind[nvbo->kind] != nvbo->kind;
@@ -241,7 +234,7 @@ nouveau_bo_new(struct nouveau_cli *cli, u64 size, int align,
Re: [Intel-gfx] [Nouveau] [PATCH v6 08/17] drm/ttm: use gem vma_node
On Wed, Aug 21, 2019 at 04:33:58PM +1000, Ben Skeggs wrote: > On Wed, 14 Aug 2019 at 20:14, Gerd Hoffmann wrote: > > > > Hi, > > > > > > Changing the order doesn't look hard. Patch attached (untested, have no > > > > test hardware). But maybe I missed some detail ... > > > > > > I came up with something very similar by splitting up nouveau_bo_new() > > > into allocation and initialization steps, so that when necessary the GEM > > > object can be initialized in between. I think that's slightly more > > > flexible and easier to understand than a boolean flag. > > > > Yes, that should work too. > > > > Acked-by: Gerd Hoffmann > Acked-by: Ben Skeggs Thanks guys, applied to drm-misc-next. Thierry signature.asc Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/atomic-helper: reset vblank on crtc reset
On Wed, May 27, 2020 at 11:53:32AM +0200, Daniel Vetter wrote: > Only when vblanks are supported ofc. > > Some drivers do this already, but most unfortunately missed it. This > opens up bugs after driver load, before the crtc is enabled for the > first time. syzbot spotted this when loading vkms as a secondary > output. Given how many drivers are buggy it's best to solve this once > and for all in shared helper code. > > Aside from moving the few existing calls to drm_crtc_vblank_reset into > helpers (i915 doesn't use helpers, so keeps its own) I think the > regression risk is minimal: atomic helpers already rely on drivers > calling drm_crtc_vblank_on/off correctly in their hooks when they > support vblanks. And driver that's failing to handle vblanks after > this is missing those calls already, and vblanks could only work by > accident when enabling a CRTC for the first time right after boot. > > Big thanks to Tetsuo for helping track down what's going wrong here. > > There's only a few drivers which already had the necessary call and > needed some updating: > - komeda, atmel and tidss also needed to be changed to call > __drm_atomic_helper_crtc_reset() intead of open coding it > - tegra and msm even had it in the same place already, just code > motion, and malidp already uses __drm_atomic_helper_crtc_reset(). > > Only call left is in i915, which doesn't use drm_mode_config_reset, > but has its own fastboot infrastructure. So that's the only case where > we actually want this in the driver still. > > I've also reviewed all other drivers which set up vblank support with > drm_vblank_init. After the previous patch fixing mxsfb all atomic > drivers do call drm_crtc_vblank_on/off as they should, the remaining > drivers are either legacy kms or legacy dri1 drivers, so not affected > by this change to atomic helpers. > > v2: Use the drm_dev_has_vblank() helper. > > Link: > https://syzkaller.appspot.com/bug?id=0ba17d70d062b2595e1f061231474800f076c7cb > Reported-by: Tetsuo Handa > Reported-by: syzbot+0871b14ca2e2fb64f...@syzkaller.appspotmail.com > Cc: Tetsuo Handa > Cc: "James (Qian) Wang" > Cc: Liviu Dudau > Cc: Mihail Atanassov > Cc: Brian Starkey > Cc: Sam Ravnborg > Cc: Boris Brezillon > Cc: Nicolas Ferre > Cc: Alexandre Belloni > Cc: Ludovic Desroches > Cc: Maarten Lankhorst > Cc: Maxime Ripard > Cc: Thomas Zimmermann > Cc: David Airlie > Cc: Daniel Vetter > Cc: Thierry Reding > Cc: Jonathan Hunter > Cc: Jyri Sarha > Cc: Tomi Valkeinen > Cc: Rob Clark > Cc: Sean Paul > Cc: Brian Masney > Cc: Emil Velikov > Cc: zhengbin > Cc: Thomas Gleixner > Cc: linux-te...@vger.kernel.org > Signed-off-by: Daniel Vetter > --- > drivers/gpu/drm/arm/display/komeda/komeda_crtc.c | 7 ++- > drivers/gpu/drm/arm/malidp_drv.c | 1 - > drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 7 ++- > drivers/gpu/drm/drm_atomic_state_helper.c| 4 > drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c| 2 -- > drivers/gpu/drm/tegra/dc.c | 1 - > drivers/gpu/drm/tidss/tidss_crtc.c | 3 +-- > drivers/gpu/drm/tidss/tidss_kms.c| 4 > 8 files changed, 9 insertions(+), 20 deletions(-) Looks good, and nice cleanup: Acked-by: Thierry Reding signature.asc Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v5 00/16] acpi/pwm/i915: Convert pwm-crc and i915 driver's PWM code to use the atomic PWM API
On Fri, Jul 17, 2020 at 03:37:37PM +0200, Hans de Goede wrote: > Hi All, > > Here is v5 of my patch series converting the i915 driver's code for > controlling the panel's backlight with an external PWM controller to > use the atomic PWM API. See below for the changelog. > > This series consists of 4 parts: > > 1. acpi_lpss fixes workarounds for Cherry Trail DSTD nastiness > 2. various fixes to the pwm-lpss driver > 3. convert the pwm-crc driver to support the atomic PWM API and > 4. convert the i915 driver's PWM code to use the atomic PWM API > > The involved acpi_lpss and pwm drivers do not see a whole lot of churn, > so the plan is to merge this all through drm-intel-next-queued (dinq) > once all the patches are reviewed / have acks. > > Specifically patches 5-9, 11 still need an Acked- / Reviewed-by > > Andy, can you please take a look at the unreviewed patches? Specifically > patches 5-6 should address your review remarks from v4 of this set > and I've addressed your review remarks on patches 7-9 in v3 already. > A review of patch 11 would also be welcome > > Uwe, can you please take a look at the unreviewed patches? > > Uwe, may I have your Acked-by for merging this series through the > drm-intel-next-queued branch once all PWM patches have an Acked- or > Reviewed-by ? > > This series has been tested (and re-tested after adding various bug-fixes) > extensively. It has been tested on the following devices: > > -Asus T100TA BYT + CRC-PMIC PWM > -Toshiba WT8-A BYT + CRC-PMIC PWM > -Thundersoft TS178 BYT + CRC-PMIC PWM, inverse PWM > -Asus T100HA CHT + CRC-PMIC PWM > -Terra Pad 1061 BYT + LPSS PWM > -Trekstor Twin 10.1 BYT + LPSS PWM > -Asus T101HA CHT + CRC-PMIC PWM > -GPD Pocket CHT + CRC-PMIC PWM > > Changelog: > Changes in v5: > - Dropped the "pwm: lpss: Correct get_state result for base_unit == 0" > patch. The base_unit == 0 condition should never happen and sofar it is > unclear what the proper behavior / correct values to store in the > pwm_state should be when this does happen. Since this patch was added as > an extra pwm-lpss fix in v4 of this patch-set and otherwise is orthogonal > to the of this patch-set just drop it (again). > - "[PATCH 04/16] pwm: lpss: Add range limit check for the base_unit register > value" > - Use clamp_val(... instead of clam_t(unsigned long long, ... > - "[PATCH 05/16] pwm: lpss: Add pwm_lpss_prepare_enable() helper" > - This is a new patch in v5 of this patchset > - [PATCH 06/16] pwm: lpss: Use pwm_lpss_apply() when restoring state on resume > - Use the new pwm_lpss_prepare_enable() helper > > Changes in v4: > - "[PATCH v4 06/16] pwm: lpss: Correct get_state result for base_unit == 0" > - This is a new patch in v4 of this patchset > - "[PATCH v4 12/16] pwm: crc: Implement get_state() method" > - Use DIV_ROUND_UP when calculating the period and duty_cycle values > - "[PATCH v4 16/16] drm/i915: panel: Use atomic PWM API for devs with an > external PWM controller" > - Add a note to the commit message about the changes in > pwm_disable_backlight() > - Use the pwm_set/get_relative_duty_cycle() helpers > > Changes in v3: > - "[PATCH v3 04/15] pwm: lpss: Add range limit check for the base_unit > register value" > - Use base_unit_range - 1 as maximum value for the clamp() > - "[PATCH v3 05/15] pwm: lpss: Use pwm_lpss_apply() when restoring state on > resume" > - This replaces the "pwm: lpss: Set SW_UPDATE bit when enabling the PWM" > patch from previous versions of this patch-set, which really was a hack > working around the resume issue which this patch fixes properly. > - PATCH v3 6 - 11 pwm-crc changes: > - Various small changes resulting from the reviews by Andy and Uwe, > including some refactoring of the patches to reduce the amount of churn > in the patch-set > > Changes in v2: > - Fix coverletter subject > - Drop accidentally included debugging patch > - "[PATCH v3 02/15] ACPI / LPSS: Save Cherry Trail PWM ctx registers only > once ( > - Move #define LPSS_SAVE_CTX_ONCE define to group it with LPSS_SAVE_CTX Hi Hans, I've applied patches 3 through 12 to the PWM tree. I thought it was a bit odd that only a handful of these patches had been reviewed and there were no Tested-bys, but I'm going to trust that you know what you're doing. =) If this breaks things for anyone I'm sure they'll complain. That said I see that Rafael has acked patches 1-2 and Jani did so for patches 13-16. I'm not sure if you expect me to pick those patches up as well. As far as I can tell the ACPI, PWM and DRM parts are all independent, so these patches could be applied to the corresponding subsystem trees. Anyway, if you want me to pick those all up into the PWM tree, I suppose that's something I can do as well. Thierry signature.asc Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v5 00/16] acpi/pwm/i915: Convert pwm-crc and i915 driver's PWM code to use the atomic PWM API
On Wed, Jul 29, 2020 at 11:32:28AM +0200, Hans de Goede wrote:
> cHi,
>
> On 7/29/20 10:23 AM, Andy Shevchenko wrote:
> > On Mon, Jul 27, 2020 at 09:41:20AM +0200, Thierry Reding wrote:
> > > On Fri, Jul 17, 2020 at 03:37:37PM +0200, Hans de Goede wrote:
> >
> > > I've applied patches 3 through 12 to the PWM tree. I thought it was a
> > > bit odd that only a handful of these patches had been reviewed and there
> > > were no Tested-bys, but I'm going to trust that you know what you're
> > > doing. =) If this breaks things for anyone I'm sure they'll complain.
>
> Thank you for picking up these patches, but ...
>
> > Can we postpone a bit?
>
> I have to agree with Andy here, as mentioned my plan was to push the
> entire series through drm-intel-next-queued once the last few PWM
> patches are reviewed.
>
> There are some fixes, to the pwm-crc driver which change behavior in
> a possibly undesirable way, unless combined with the i915 changes.
>
> E.g. there is a fix which makes the pwm-crc driver actually honor
> the requested output frequency (it was not doing this due to a bug)
> and before the i915 changes, the i915 driver was hardcoding an output
> freq, rather then looking at the video-bios-tables as it should.
>
> So having just the pwm-crc fix, will change the output frequency
> which some LCD panels might not like.
>
> Note things are probably fine with the hardcoded output freq, but I
> would like to play it safe here.
>
> Also Andy was still reviewing some of the PWM patches, and has requested
> changes to 1 patch, nothing functional just some code-reshuffling for
> cleaner code, so we could alternatively fix this up with a follow-up patch.
>
> Either way please let us know how you want to proceed.
Okay, that's fine, I'll drop them again.
> > > That said I see that Rafael has acked patches 1-2 and Jani did so for
> > > patches 13-16. I'm not sure if you expect me to pick those patches up as
> > > well. As far as I can tell the ACPI, PWM and DRM parts are all
> > > independent, so these patches could be applied to the corresponding
> > > subsystem trees.
> > >
> > > Anyway, if you want me to pick those all up into the PWM tree, I suppose
> > > that's something I can do as well.
>
> drm-intel-next-queued is usually seeing quite a bit of churn, so the i915
> patches really should go upstream through that branch.
During my build tests I ran into a small issue caused by this series
interacting with the conversion of period and duty-cycle to u64 that
I've queued for v5.9. This causes a build failure on x86.
I have this local diff to fix that:
--- >8 ---
diff --git a/drivers/pwm/pwm-crc.c b/drivers/pwm/pwm-crc.c
index 370ab826a20b..92e838797733 100644
--- a/drivers/pwm/pwm-crc.c
+++ b/drivers/pwm/pwm-crc.c
@@ -76,7 +76,9 @@ static int crc_pwm_apply(struct pwm_chip *chip, struct
pwm_device *pwm,
if (pwm_get_duty_cycle(pwm) != state->duty_cycle ||
pwm_get_period(pwm) != state->period) {
- int level = state->duty_cycle * PWM_MAX_LEVEL / state->period;
+ u64 level = state->duty_cycle * PWM_MAX_LEVEL;
+
+ do_div(level, state->period);
err = regmap_write(crc_pwm->regmap, PWM0_DUTY_CYCLE, level);
if (err) {
@@ -141,10 +143,9 @@ static void crc_pwm_get_state(struct pwm_chip *chip,
struct pwm_device *pwm,
clk_div = (clk_div_reg & ~PWM_OUTPUT_ENABLE) + 1;
- state->period =
- DIV_ROUND_UP(clk_div * NSEC_PER_USEC * 256, PWM_BASE_CLK_MHZ);
- state->duty_cycle =
- DIV_ROUND_UP(duty_cycle_reg * state->period, PWM_MAX_LEVEL);
+ state->period = DIV_ROUND_UP(clk_div * NSEC_PER_USEC * 256,
PWM_BASE_CLK_MHZ);
+ state->duty_cycle = duty_cycle_reg * state->period + PWM_MAX_LEVEL - 1;
+ do_div(state->duty_cycle, PWM_MAX_LEVEL);
state->polarity = PWM_POLARITY_NORMAL;
state->enabled = !!(clk_div_reg & PWM_OUTPUT_ENABLE);
}
--- >8 ---
So perhaps you want to integrate that or something equivalent into your
series.
Also this could result in a tricky dependency between PWM and drm-misc,
although if you're targetting drm-misc it's too late for v5.9 anyway. In
that case you should be able to rebase your series on v5.9-rc1 when it's
out and then you'll get the prerequisite PWM changes for the u64
conversion as part of that. No need to track the dependency explicitly.
Thierry
signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v8 04/17] pwm: lpss: Add range limit check for the base_unit register value
On Sun, Aug 30, 2020 at 02:57:40PM +0200, Hans de Goede wrote:
> When the user requests a high enough period ns value, then the
> calculations in pwm_lpss_prepare() might result in a base_unit value of 0.
>
> But according to the data-sheet the way the PWM controller works is that
> each input clock-cycle the base_unit gets added to a N bit counter and
> that counter overflowing determines the PWM output frequency. Adding 0
> to the counter is a no-op. The data-sheet even explicitly states that
> writing 0 to the base_unit bits will result in the PWM outputting a
> continuous 0 signal.
>
> When the user requestes a low enough period ns value, then the
> calculations in pwm_lpss_prepare() might result in a base_unit value
> which is bigger then base_unit_range - 1. Currently the codes for this
> deals with this by applying a mask:
>
> base_unit &= (base_unit_range - 1);
>
> But this means that we let the value overflow the range, we throw away the
> higher bits and store whatever value is left in the lower bits into the
> register leading to a random output frequency, rather then clamping the
> output frequency to the highest frequency which the hardware can do.
>
> This commit fixes both issues by clamping the base_unit value to be
> between 1 and (base_unit_range - 1).
>
> Fixes: 684309e5043e ("pwm: lpss: Avoid potential overflow of base_unit")
> Reviewed-by: Andy Shevchenko
> Signed-off-by: Hans de Goede
> ---
> Changes in v5:
> - Use clamp_val(... instead of clam_t(unsigned long long, ...
>
> Changes in v3:
> - Change upper limit of clamp to (base_unit_range - 1)
> - Add Fixes tag
> ---
> drivers/pwm/pwm-lpss.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
Acked-by: Thierry Reding
signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v8 03/17] pwm: lpss: Fix off by one error in base_unit math in pwm_lpss_prepare()
On Sun, Aug 30, 2020 at 02:57:39PM +0200, Hans de Goede wrote:
> According to the data-sheet the way the PWM controller works is that
> each input clock-cycle the base_unit gets added to a N bit counter and
> that counter overflowing determines the PWM output frequency.
>
> So assuming e.g. a 16 bit counter this means that if base_unit is set to 1,
> after 65535 input clock-cycles the counter has been increased from 0 to
> 65535 and it will overflow on the next cycle, so it will overflow after
> every 65536 clock cycles and thus the calculations done in
> pwm_lpss_prepare() should use 65536 and not 65535.
>
> This commit fixes this. Note this also aligns the calculations in
> pwm_lpss_prepare() with those in pwm_lpss_get_state().
>
> Note this effectively reverts commit 684309e5043e ("pwm: lpss: Avoid
> potential overflow of base_unit"). The next patch in this series really
> fixes the potential overflow of the base_unit value.
>
> Fixes: 684309e5043e ("pwm: lpss: Avoid potential overflow of base_unit")
> Reviewed-by: Andy Shevchenko
> Acked-by: Uwe Kleine-König
> Signed-off-by: Hans de Goede
> ---
> Changes in v3:
> - Add Fixes tag
> - Add Reviewed-by: Andy Shevchenko tag
> ---
> drivers/pwm/pwm-lpss.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
Acked-by: Thierry Reding
signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v8 05/17] pwm: lpss: Add pwm_lpss_prepare_enable() helper
On Sun, Aug 30, 2020 at 02:57:41PM +0200, Hans de Goede wrote: > In the not-enabled -> enabled path pwm_lpss_apply() needs to get a > runtime-pm reference; and then on any errors it needs to release it > again. > > This leads to somewhat hard to read code. This commit introduces a new > pwm_lpss_prepare_enable() helper and moves all the steps necessary for > the not-enabled -> enabled transition there, so that we can error check > the entire transition in a single place and only have one pm_runtime_put() > on failure call site. > > While working on this I noticed that the enabled -> enabled (update > settings) path was quite similar, so I've added an enable parameter to > the new pwm_lpss_prepare_enable() helper, which allows using it in that > path too. > > Suggested-by: Andy Shevchenko > Reviewed-by: Andy Shevchenko > Signed-off-by: Hans de Goede > --- > drivers/pwm/pwm-lpss.c | 45 ------ > 1 file changed, 26 insertions(+), 19 deletions(-) Acked-by: Thierry Reding signature.asc Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v8 06/17] pwm: lpss: Use pwm_lpss_restore() when restoring state on resume
On Sun, Aug 30, 2020 at 02:57:42PM +0200, Hans de Goede wrote: > Before this commit a suspend + resume of the LPSS PWM controller > would result in the controller being reset to its defaults of > output-freq = clock/256, duty-cycle=100%, until someone changes > to the output-freq and/or duty-cycle are made. > > This problem has been masked so far because the main consumer > (the i915 driver) was always making duty-cycle changes on resume. > With the conversion of the i915 driver to the atomic PWM API the > driver now only disables/enables the PWM on suspend/resume leaving > the output-freq and duty as is, triggering this problem. Doesn't this imply that there's another bug at play here? At the PWM API level you're applying a state and it's up to the driver to ensure that the hardware state after ->apply() is what the software has requested. If you only switch the enable state and that doesn't cause period and duty cycle to be updated it means that your driver isn't writing those registers when it should be. > The LPSS PWM controller has a mechanism where the ctrl register value > and the actual base-unit and on-time-div values used are latched. When > software sets the SW_UPDATE bit then at the end of the current PWM cycle, > the new values from the ctrl-register will be latched into the actual > registers, and the SW_UPDATE bit will be cleared. > > The problem is that before this commit our suspend/resume handling > consisted of simply saving the PWM ctrl register on suspend and > restoring it on resume, without setting the PWM_SW_UPDATE bit. > When the controller has lost its state over a suspend/resume and thus > has been reset to the defaults, just restoring the register is not > enough. We must also set the SW_UPDATE bit to tell the controller to > latch the restored values into the actual registers. > > Fixing this problem is not as simple as just or-ing in the value which > is being restored with SW_UPDATE. If the PWM was enabled before we must > write the new settings + PWM_SW_UPDATE before setting PWM_ENABLE. > We must also wait for PWM_SW_UPDATE to become 0 again and depending on the > model we must do this either before or after the setting of PWM_ENABLE. > > All the necessary logic for doing this is already present inside > pwm_lpss_apply(), so instead of duplicating this inside the resume > handler, this commit adds a new pwm_lpss_restore() helper which mirrors > pwm_lpss_apply() minus the runtime-pm reference handling (which we should > not change on resume). If this is all already implemented in pwm_lpss_apply(), why isn't it working for the suspend/resume case? It shouldn't matter that the consumer only changes the enable/disable state. After ->apply() successfully returns your hardware should be programmed with exactly the state that the consumer requested. Thierry signature.asc Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v8 08/17] pwm: crc: Fix period / duty_cycle times being off by a factor of 256
On Sun, Aug 30, 2020 at 02:57:44PM +0200, Hans de Goede wrote: > While looking into adding atomic-pwm support to the pwm-crc driver I > noticed something odd, there is a PWM_BASE_CLK define of 6 MHz and > there is a clock-divider which divides this with a value between 1-128, > and there are 256 duty-cycle steps. > > The pwm-crc code before this commit assumed that a clock-divider > setting of 1 means that the PWM output is running at 6 MHZ, if that > is true, where do these 256 duty-cycle steps come from? > > This would require an internal frequency of 256 * 6 MHz = 1.5 GHz, that > seems unlikely for a PMIC which is using a silicon process optimized for > power-switching transistors. It is way more likely that there is an 8 > bit counter for the duty cycle which acts as an extra fixed divider > wrt the PWM output frequency. > > The main user of the pwm-crc driver is the i915 GPU driver which uses it > for backlight control. Lets compare the PWM register values set by the > video-BIOS (the GOP), assuming the extra fixed divider is present versus > the PWM frequency specified in the Video-BIOS-Tables: > > Device: PWM Hz set by BIOS PWM Hz specified in VBT > Asus T100TA 200 200 > Asus T100HA 200 200 > Lenovo Miix 2 8 23437 2 > Toshiba WT8-A 23437 2 > > So as we can see if we assume the extra division by 256 then the register > values set by the GOP are an exact match for the VBT values, where as > otherwise the values would be of by a factor of 256. > > This commit fixes the period / duty_cycle calculations to take the > extra division by 256 into account. > > Reviewed-by: Andy Shevchenko > Signed-off-by: Hans de Goede > --- > Changes in v3: > - Use NSEC_PER_USEC instead of adding a new (non-sensical) NSEC_PER_MHZ define > --- > drivers/pwm/pwm-crc.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) Acked-by: Thierry Reding signature.asc Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v8 07/17] pwm: lpss: Always update state and set update bit
On Sun, Aug 30, 2020 at 02:57:43PM +0200, Hans de Goede wrote:
> This commit removes a check where we would skip writing the ctrl register
> and then setting the update bit in case the ctrl register already contains
> the correct values.
>
> In a perfect world skipping the update should be fine in these cases, but
> on Cherry Trail devices the AML code in the GFX0 devices' PS0 and PS3
> methods messes with the PWM controller.
>
> The "ACPI / LPSS: Resume Cherry Trail PWM controller in no-irq phase" patch
> earlier in this series stops the GFX0._PS0 method from messing with the PWM
> controller and on the DSDT-s inspected sofar the _PS3 method only reads
> from the PWM controller (and turns it off before we get a change to do so):
>
> {
> PWMB = PWMC /* \_SB_.PCI0.GFX0.PWMC */
> PSAT |= 0x03
> Local0 = PSAT /* \_SB_.PCI0.GFX0.PSAT */
> }
>
> The PWM controller getting turning off before we do this ourselves is
> a bit annoying but not really an issue.
>
> The problem this patch fixes comes from a new variant of the GFX0._PS3 code
> messing with the PWM controller found on the Acer One 10 S1003 (1):
>
> {
> PWMB = PWMC /* \_SB_.PCI0.GFX0.PWMC */
> PWMT = PWMC /* \_SB_.PCI0.GFX0.PWMC */
> PWMT &= 0xFFFF
> PWMT |= 0xC000
> PWMC = PWMT /* \_SB_.PCI0.GFX0.PWMT */
> PWMT = PWMC /* \_SB_.PCI0.GFX0.PWMC */
> Sleep (0x64)
> PWMB &= 0x3FFF
> PWMC = PWMB /* \_SB_.PCI0.GFX0.PWMB */
> PSAT |= 0x03
> Local0 = PSAT /* \_SB_.PCI0.GFX0.PSAT */
> }
>
> This "beautiful" piece of code clears the base-unit part of the ctrl-reg,
> which effectively disables the controller, and it sets the update flag
> to apply this change. Then after this it restores the original ctrl-reg
> value, so we do not see it has mucked with the controller.
>
> *But* it does not set the update flag when restoring the original value.
> So the check to see if we can skip writing the ctrl register succeeds
> but since the update flag was not set, the old base-unit value of 0 is
> still in use and the PWM controller is effectively disabled.
>
> IOW this PWM controller poking means that we cannot trust the base-unit /
> on-time-div value we read back from the PWM controller since it may not
> have been applied/committed. Thus we must always update the ctrl-register
> and set the update bit.
Doesn't this now make patch 6/17 obsolete?
Thierry
signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v8 08/17] pwm: crc: Fix period / duty_cycle times being off by a factor of 256
On Sun, Aug 30, 2020 at 02:57:44PM +0200, Hans de Goede wrote: > While looking into adding atomic-pwm support to the pwm-crc driver I > noticed something odd, there is a PWM_BASE_CLK define of 6 MHz and > there is a clock-divider which divides this with a value between 1-128, > and there are 256 duty-cycle steps. > > The pwm-crc code before this commit assumed that a clock-divider > setting of 1 means that the PWM output is running at 6 MHZ, if that > is true, where do these 256 duty-cycle steps come from? > > This would require an internal frequency of 256 * 6 MHz = 1.5 GHz, that > seems unlikely for a PMIC which is using a silicon process optimized for > power-switching transistors. It is way more likely that there is an 8 > bit counter for the duty cycle which acts as an extra fixed divider > wrt the PWM output frequency. > > The main user of the pwm-crc driver is the i915 GPU driver which uses it > for backlight control. Lets compare the PWM register values set by the > video-BIOS (the GOP), assuming the extra fixed divider is present versus > the PWM frequency specified in the Video-BIOS-Tables: > > Device: PWM Hz set by BIOS PWM Hz specified in VBT > Asus T100TA 200 200 > Asus T100HA 200 200 > Lenovo Miix 2 8 23437 2 > Toshiba WT8-A 23437 2 > > So as we can see if we assume the extra division by 256 then the register > values set by the GOP are an exact match for the VBT values, where as > otherwise the values would be of by a factor of 256. > > This commit fixes the period / duty_cycle calculations to take the > extra division by 256 into account. > > Reviewed-by: Andy Shevchenko > Signed-off-by: Hans de Goede > --- > Changes in v3: > - Use NSEC_PER_USEC instead of adding a new (non-sensical) NSEC_PER_MHZ define > --- > drivers/pwm/pwm-crc.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) Acked-by: Thierry Reding signature.asc Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v8 09/17] pwm: crc: Fix off-by-one error in the clock-divider calculations
On Sun, Aug 30, 2020 at 02:57:45PM +0200, Hans de Goede wrote: > The CRC PWM controller has a clock-divider which divides the clock with > a value between 1-128. But as can seen from the PWM_DIV_CLK_xxx > defines, this range maps to a register value of 0-127. > > So after calculating the clock-divider we must subtract 1 to get the > register value, unless the requested frequency was so high that the > calculation has already resulted in a (rounded) divider value of 0. > > Note that before this fix, setting a period of PWM_MAX_PERIOD_NS which > corresponds to the max. divider value of 128 could have resulted in a > bug where the code would use 128 as divider-register value which would > have resulted in an actual divider value of 0 (and the enable bit being > set). A rounding error stopped this bug from actually happen. This > same rounding error means that after the subtraction of 1 it is impossible > to set the divider to 128. Also bump PWM_MAX_PERIOD_NS by 1 ns to allow > setting a divider of 128 (register-value 127). > > Reviewed-by: Andy Shevchenko > Signed-off-by: Hans de Goede > --- > Changes in v3: > - Introduce crc_pwm_calc_clk_div() here instead of later in the patch-set > to reduce the amount of churn in the patch-set a bit > --- > drivers/pwm/pwm-crc.c | 17 ++++++--- > 1 file changed, 14 insertions(+), 3 deletions(-) Acked-by: Thierry Reding signature.asc Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v8 10/17] pwm: crc: Fix period changes not having any effect
On Sun, Aug 30, 2020 at 02:57:46PM +0200, Hans de Goede wrote: > The pwm-crc code is using 2 different enable bits: > 1. bit 7 of the PWM0_CLK_DIV (PWM_OUTPUT_ENABLE) > 2. bit 0 of the BACKLIGHT_EN register > > The BACKLIGHT_EN register at address 0x51 really controls a separate > output-only GPIO which is earmarked to be used as output connected to the > backlight-enable pin for LCD panels, this GPO is part of the PMIC's > "Display Panel Control Block." . This pin should probably be moved over > to a GPIO provider driver (and consumers modified accordingly), but that > is something for an(other) patch. > > Enabling / disabling the actual PWM output is controlled by the > PWM_OUTPUT_ENABLE bit of the PWM0_CLK_DIV register. > > As the comment in the old code already indicates we must disable the PWM > before we can change the clock divider. But the crc_pwm_disable() and > crc_pwm_enable() calls the old code make for this only change the > BACKLIGHT_EN register; and the value of that register does not matter for > changing the period / the divider. What does matter is that the > PWM_OUTPUT_ENABLE bit must be cleared before a new value can be written. > > This commit modifies crc_pwm_config() to clear PWM_OUTPUT_ENABLE instead > when changing the period, so that period changes actually work. > > Note this fix will cause a significant behavior change on some devices > using the CRC PWM output to drive their backlight. Before the PWM would > always run with the output frequency configured by the BIOS at boot, now > the period time specified by the i915 driver will actually be honored. > > Reviewed-by: Andy Shevchenko > Signed-off-by: Hans de Goede > --- > drivers/pwm/pwm-crc.c | 7 ++- > 1 file changed, 2 insertions(+), 5 deletions(-) Acked-by: Thierry Reding signature.asc Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v8 11/17] pwm: crc: Enable/disable PWM output on enable/disable
On Sun, Aug 30, 2020 at 02:57:47PM +0200, Hans de Goede wrote: > The pwm-crc code is using 2 different enable bits: > 1. bit 7 of the PWM0_CLK_DIV (PWM_OUTPUT_ENABLE) > 2. bit 0 of the BACKLIGHT_EN register > > So far we've kept the PWM_OUTPUT_ENABLE bit set when disabling the PWM, > this commit makes crc_pwm_disable() clear it on disable and makes > crc_pwm_enable() set it again on re-enable. > > Acked-by: Uwe Kleine-König > Reviewed-by: Andy Shevchenko > Signed-off-by: Hans de Goede > --- > Changes in v3: > - Remove paragraph about tri-stating the output from the commit message, > we don't have a datasheet so this was just an unfounded guess > --- > drivers/pwm/pwm-crc.c | 4 > 1 file changed, 4 insertions(+) Acked-by: Thierry Reding signature.asc Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v8 13/17] pwm: crc: Implement get_state() method
On Sun, Aug 30, 2020 at 02:57:49PM +0200, Hans de Goede wrote: > Implement the pwm_ops.get_state() method to complete the support for the > new atomic PWM API. > > Reviewed-by: Andy Shevchenko > Signed-off-by: Hans de Goede > --- > Changes in v6: > - Rebase on 5.9-rc1 > - Use DIV_ROUND_UP_ULL because pwm_state.period and .duty_cycle are now u64 > > Changes in v5: > - Fix an indentation issue > > Changes in v4: > - Use DIV_ROUND_UP when calculating the period and duty_cycle from the > controller's register values > > Changes in v3: > - Add Andy's Reviewed-by tag > - Remove extra whitespace to align some code after assignments (requested by > Uwe Kleine-König) > --- > drivers/pwm/pwm-crc.c | 31 +++++++ > 1 file changed, 31 insertions(+) Acked-by: Thierry Reding signature.asc Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v8 12/17] pwm: crc: Implement apply() method to support the new atomic PWM API
On Sun, Aug 30, 2020 at 02:57:48PM +0200, Hans de Goede wrote: > Replace the enable, disable and config pwm_ops with an apply op, > to support the new atomic PWM API. > > Reviewed-by: Andy Shevchenko > Signed-off-by: Hans de Goede > --- > Changes in v6: > - Rebase on 5.9-rc1 > - Use do_div when calculating level because pwm_state.period and .duty_cycle > are now u64 > > Changes in v3: > - Keep crc_pwm_calc_clk_div() helper to avoid needless churn > --- > drivers/pwm/pwm-crc.c | 89 ++- > 1 file changed, 54 insertions(+), 35 deletions(-) Acked-by: Thierry Reding signature.asc Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v8 06/17] pwm: lpss: Use pwm_lpss_restore() when restoring state on resume
On Mon, Aug 31, 2020 at 01:46:28PM +0200, Hans de Goede wrote:
> Hi,
>
> On 8/31/20 1:10 PM, Thierry Reding wrote:
> > On Sun, Aug 30, 2020 at 02:57:42PM +0200, Hans de Goede wrote:
> > > Before this commit a suspend + resume of the LPSS PWM controller
> > > would result in the controller being reset to its defaults of
> > > output-freq = clock/256, duty-cycle=100%, until someone changes
> > > to the output-freq and/or duty-cycle are made.
> > >
> > > This problem has been masked so far because the main consumer
> > > (the i915 driver) was always making duty-cycle changes on resume.
> > > With the conversion of the i915 driver to the atomic PWM API the
> > > driver now only disables/enables the PWM on suspend/resume leaving
> > > the output-freq and duty as is, triggering this problem.
> >
> > Doesn't this imply that there's another bug at play here? At the PWM API
> > level you're applying a state and it's up to the driver to ensure that
> > the hardware state after ->apply() is what the software has requested.
> >
> > If you only switch the enable state and that doesn't cause period and
> > duty cycle to be updated it means that your driver isn't writing those
> > registers when it should be.
>
> Right, the driver was not committing those as it should *on resume*,
> that and it skips setting the update bit on the subsequent enable,
> which is an optimization which gets removed in 7/17.
>
> Before switching the i915 driver over to atomic, when the LPSS-PWM
> was used for the backlight we got the following order on suspend/resume
>
> 1. Set duty-cycle to 0%
> 2. Set enabled to 0
> 3. Save ctrl reg
> 4. Power-off PWM controller, it now looses all its state
> 5. Power-on PWM ctrl
> 6. Restore ctrl reg (as a single reg write)
> 7. Set enabled to 1, at this point one would expect the
> duty/freq from the restored ctrl-reg to apply, but:
> a) The resume code never sets the update bit (which this commit fixes); and
> b) On applying the pwm_state with enabled=1 the code applying the
> state does this (before setting the enabled bit in the ctrl reg):
>
> if (orig_ctrl != ctrl) {
> pwm_lpss_write(pwm, ctrl);
> pwm_lpss_write(pwm, ctrl | PWM_SW_UPDATE);
> }
> and since the restore of the ctrl reg set the old duty/freq the
> writes are skipped, so the update bit never gets set.
>
> 8. Set duty-cycle to the pre-suspend value (which is not 0)
> this does cause a change in the ctrl-reg, so now the update flag
> does get set.
>
> Note that 1-2 and 7-8 are both done by the non atomic i915 code,
> when moving the i915 code to atomic I decided that having these
> 2 separate steps here is non-sense, so the new i915 code just
> toggles the enable bit. So in essence the new atomic PWM
> i915 code drops step 1 and 8.
>
> Dropping steps 8 means that the update bit never gets set and we
> end up with the PWM running at its power-on-reset duty cycle.
>
> You are correct in your remark to patch 7/17 that since that removes
> the if (orig_ctrl != ctrl) for the writes that now step 7 will be
> sufficient to get the PWM to work again. But that only takes the i915
> usage into account.
>
> What if the PWM is used through the sysfs userspace API?
> Then only steps 3-6 will happen on suspend-resume and without
> fixing step 6 to properly restore the PWM controller in its
> pre-resume state (this patch) it will once again be running at
> its power-on-reset defaults instead of the values from the
> restored control register.
Actually PWM's sysfs code has suspend/resume callbacks that basically
make sysfs just a regular consumer of PWMs. So they do end up doing a
pwm_apply_state() on the PWM as well on suspend and restore the state
from before suspend on resume.
This was done very specifically because the suspend/resume order can be
unexpected under some circumstances, so for PWM we really want for the
consumer to always have ultimate control over when precisely the PWM is
restored on resume.
The reason why we did this was because people observed weird glitches on
suspend/resume with different severity. In some cases a backlight would
be resumed before the display controller had had a chance to start
sending frames, causing on-screen corruption in some cases (such as
smart displays) and in other cases a PWM-controller regulator would be
resumed too late or too early, which I think was causing some issue with
the CPUs not working properly on resume.
So I'd prefer not to have any PWM driver save and restore its own
context on suspend/resume, because that's inevitably going to cause
unexpected behaviour at some point. If it's a
Re: [Intel-gfx] [PATCH v8 07/17] pwm: lpss: Always update state and set update bit
On Mon, Aug 31, 2020 at 01:26:46PM +0200, Hans de Goede wrote:
> Hi,
>
> On 8/31/20 1:13 PM, Thierry Reding wrote:
> > On Sun, Aug 30, 2020 at 02:57:43PM +0200, Hans de Goede wrote:
> > > This commit removes a check where we would skip writing the ctrl register
> > > and then setting the update bit in case the ctrl register already contains
> > > the correct values.
> > >
> > > In a perfect world skipping the update should be fine in these cases, but
> > > on Cherry Trail devices the AML code in the GFX0 devices' PS0 and PS3
> > > methods messes with the PWM controller.
> > >
> > > The "ACPI / LPSS: Resume Cherry Trail PWM controller in no-irq phase"
> > > patch
> > > earlier in this series stops the GFX0._PS0 method from messing with the
> > > PWM
> > > controller and on the DSDT-s inspected sofar the _PS3 method only reads
> > > from the PWM controller (and turns it off before we get a change to do
> > > so):
> > >
> > > {
> > > PWMB = PWMC /* \_SB_.PCI0.GFX0.PWMC */
> > > PSAT |= 0x03
> > > Local0 = PSAT /* \_SB_.PCI0.GFX0.PSAT */
> > > }
> > >
> > > The PWM controller getting turning off before we do this ourselves is
> > > a bit annoying but not really an issue.
> > >
> > > The problem this patch fixes comes from a new variant of the GFX0._PS3
> > > code
> > > messing with the PWM controller found on the Acer One 10 S1003 (1):
> > >
> > > {
> > > PWMB = PWMC /* \_SB_.PCI0.GFX0.PWMC */
> > > PWMT = PWMC /* \_SB_.PCI0.GFX0.PWMC */
> > > PWMT &= 0xFFFF
> > > PWMT |= 0xC000
> > > PWMC = PWMT /* \_SB_.PCI0.GFX0.PWMT */
> > > PWMT = PWMC /* \_SB_.PCI0.GFX0.PWMC */
> > > Sleep (0x64)
> > > PWMB &= 0x3FFF
> > > PWMC = PWMB /* \_SB_.PCI0.GFX0.PWMB */
> > > PSAT |= 0x03
> > > Local0 = PSAT /* \_SB_.PCI0.GFX0.PSAT */
> > > }
> > >
> > > This "beautiful" piece of code clears the base-unit part of the ctrl-reg,
> > > which effectively disables the controller, and it sets the update flag
> > > to apply this change. Then after this it restores the original ctrl-reg
> > > value, so we do not see it has mucked with the controller.
> > >
> > > *But* it does not set the update flag when restoring the original value.
> > > So the check to see if we can skip writing the ctrl register succeeds
> > > but since the update flag was not set, the old base-unit value of 0 is
> > > still in use and the PWM controller is effectively disabled.
> > >
> > > IOW this PWM controller poking means that we cannot trust the base-unit /
> > > on-time-div value we read back from the PWM controller since it may not
> > > have been applied/committed. Thus we must always update the ctrl-register
> > > and set the update bit.
> >
> > Doesn't this now make patch 6/17 obsolete?
>
> No, there is no guarantee we will get any changes soon after resume,
> so we must restore the state properly on resume, before 5.17
> we were just blindly restoring the old ctrl reg state, but
> if either the freq-div or the duty-cycle changes, we should
> also set the update bit in that case to apply the new freq-div/
> duty-cycle.
Hm... I didn't realize the driver was already saving/restoring context
before this. And from a quick look through the subsystem it looks like
I've done a pretty poor job of enforcing the "no context save/restore
from PWM drivers" rule. There are some cases that have had this support
since before we realized that this is problematic, but I think at least
pwm-img is newer than that and should never have had that code either.
> This actually also helps with that case since patch 6/17 uses
> pwm_lpss_prepare and this makes pwm_lpss_prepare set the
> update but unconditionally.
>
> Also on resume we most do the set the enable bit vs set
> the update bit in the right order, depending on the
> generation of the SoC in which the PWM controller is
> embedded. 6/17 fixes all this by resume, by treating
> resume as a special case of apply() taking all the
> steps apply does.
As I mentioned earlier this works only under the assumption that the
suspend/resume order is correct. And that's possibly true for LPSS. It
won't work in the general case, though, because a backlight could end up
suspending/resuming completely out of sync wit
Re: [Intel-gfx] [PATCH v9 06/17] pwm: lpss: Make pwm_lpss_apply() not rely on existing hardware state
On Thu, Sep 03, 2020 at 12:51:03PM +0200, Hans de Goede wrote: > Before this commit pwm_lpss_apply() was making 2 assuming > 2 pre-conditions were met by the existing hardware state: I think that "making 2" is too much. > > 1. That the base-unit and on-time-div read back from the > control register are those actually in use, so that it > can skip setting the update bit if the read-back value > matches the desired values. > > 2. That the controller is enabled when the cached > pwm_state.enabled says that the controller is enabled. > > As the long history of fixes for subtle (often suspend/resume) > lpss-pwm issues shows, this assumptions are not necessary > always true. > > 1. Specifically is not true on some (*) Cherry Trail devices > with a nasty GFX0._PS3 method which: a. saves the ctrl reg value. > b. sets the base-unit to 0 and writes the update bit to apply/commit > c. restores the original ctrl value without setting the update bit, > so that the 0 base-unit value is still in use. > > 2. Assumption 2. currently is true, but only because of the code which > saves/restores the state on suspend/resume. By convention restoring the > PWM state should be done by the PWM consumer and the presence of this > code in the pmw-lpss driver is a bug. Therefor the save/restore code will > be dropped in the next patch in this series, after which this assumption > also is no longer true. > > This commit changes the pwm_lpss_apply() to make any assumptions about the Did you mean to say "... to _not_ make any assumptions ..."? > state the hardware is in. Instead it makes pwm_lpss_apply() always fully > program the PWM controller, making it much less fragile. > > *) Seen on the Acer One 10 S1003, Lenovo Ideapad Miix 310 and 320 models > and various Medion models. > > Signed-off-by: Hans de Goede > --- > drivers/pwm/pwm-lpss.c | 21 + > 1 file changed, 9 insertions(+), 12 deletions(-) Other than the two small nits, this looks much more idiomatic and true to the atomic API, so: Acked-by: Thierry Reding signature.asc Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v9 07/17] pwm: lpss: Remove suspend/resume handlers
On Thu, Sep 03, 2020 at 12:51:04PM +0200, Hans de Goede wrote: > PWM controller drivers should not restore the PWM state on resume. The > convention is that PWM consumers do this by calling pwm_apply_state(), > so that it can be done at the exact moment when the consumer needs > the state to be stored, avoiding e.g. backlight flickering. > > The only in kernel consumers of the pwm-lpss code, the i915 driver > and the pwm-class sysfs interface code both correctly restore the > state on resume, so there is no need to do this in the pwm-lpss code. > > More-over the removed resume handler is buggy, since it blindly > restores the ctrl-register contents without setting the update > bit, which is necessary to get the controller to actually use/apply > the restored base-unit and on-time-div values. > > Signed-off-by: Hans de Goede > --- > drivers/pwm/pwm-lpss-platform.c | 1 - > drivers/pwm/pwm-lpss.c | 24 > drivers/pwm/pwm-lpss.h | 3 --- > 3 files changed, 28 deletions(-) Nice! Acked-by: Thierry Reding signature.asc Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 14/21] drm/tegra: Introduce GEM object functions
On Tue, Sep 15, 2020 at 04:59:51PM +0200, Thomas Zimmermann wrote: > GEM object functions deprecate several similar callback interfaces in > struct drm_driver. This patch replaces the per-driver callbacks with > per-instance callbacks in tegra. > > Signed-off-by: Thomas Zimmermann > --- > drivers/gpu/drm/tegra/drm.c | 4 > drivers/gpu/drm/tegra/gem.c | 8 > 2 files changed, 8 insertions(+), 4 deletions(-) Acked-by: Thierry Reding signature.asc Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/core: Remove drm_dev_unref() and it's uses
On Thu, Apr 26, 2018 at 03:58:19PM +0530, Vaishali Thakkar wrote:
> It's been a while since we introduced drm_dev{get/put} functions
> to replace reference/unreference in drm subsystem for the
> consistency purpose. So, with this patch, let's just replace
> all current use cases of drm_dev_unref() with drm_dev_put and remove
> the function itself.
>
> Coccinelle was used for mass-patching.
>
> Signed-off-by: Vaishali Thakkar
Yes, please.
Acked-by: Thierry Reding
signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/2] drm/atomic: Create and use __drm_atomic_helper_crtc_reset() everywhere
On Mon, Nov 12, 2018 at 04:01:14PM +0100, Maarten Lankhorst wrote: > We already have __drm_atomic_helper_connector_reset() and > __drm_atomic_helper_plane_reset(), extend this to crtc as well. > > Most drivers already have a gpu reset hook, correct it. > Nouveau already implemented its own __drm_atomic_helper_crtc_reset(), > convert it to the common one. > > Signed-off-by: Maarten Lankhorst > Cc: Harry Wentland > Cc: Leo Li > Cc: Alex Deucher > Cc: "Christian König" > Cc: "David (ChunMing) Zhou" > Cc: David Airlie > Cc: Liviu Dudau > Cc: Brian Starkey > Cc: Mali DP Maintainers > Cc: Boris Brezillon > Cc: Nicolas Ferre > Cc: Alexandre Belloni > Cc: Ludovic Desroches > Cc: Maarten Lankhorst > Cc: Maxime Ripard > Cc: Sean Paul > Cc: Jani Nikula > Cc: Joonas Lahtinen > Cc: Rodrigo Vivi > Cc: Philipp Zabel > Cc: CK Hu > Cc: Matthias Brugger > Cc: Rob Clark > Cc: Ben Skeggs > Cc: Tomi Valkeinen > Cc: Laurent Pinchart > Cc: Kieran Bingham > Cc: Sandy Huang > Cc: "Heiko Stübner" > Cc: Thierry Reding > Cc: Jonathan Hunter > Cc: Eric Anholt > Cc: VMware Graphics > Cc: Sinclair Yeh > Cc: Thomas Hellstrom > Cc: Tony Cheng > Cc: Shirish S > Cc: Mikita Lipski > Cc: Bhawanpreet Lakha > Cc: David Francis > Cc: Anthony Koo > Cc: Jeykumar Sankaran > Cc: Jordan Crouse > Cc: Bruce Wang > Cc: Sravanthi Kollukuduru > Cc: Archit Taneja > Cc: Steve Kowalik > Cc: Carsten Behling > Cc: Haneen Mohammed > Cc: Daniel Vetter > Cc: Rodrigo Siqueira > Cc: Mahesh Kumar > Cc: amd-...@lists.freedesktop.org > Cc: dri-de...@lists.freedesktop.org > Cc: linux-ker...@vger.kernel.org > Cc: linux-arm-ker...@lists.infradead.org > Cc: intel-gfx@lists.freedesktop.org > Cc: linux-media...@lists.infradead.org > Cc: linux-arm-...@vger.kernel.org > Cc: freedr...@lists.freedesktop.org > Cc: nouv...@lists.freedesktop.org > Cc: linux-renesas-...@vger.kernel.org > Cc: linux-rockc...@lists.infradead.org > Cc: linux-te...@vger.kernel.org > --- > .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 4 +-- > drivers/gpu/drm/arm/malidp_crtc.c | 5 +-- > .../gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c| 5 +-- > drivers/gpu/drm/drm_atomic_state_helper.c | 31 --- > drivers/gpu/drm/i915/intel_display.c | 2 +- > drivers/gpu/drm/imx/ipuv3-crtc.c | 5 +-- > drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 5 +-- > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 12 ++- > drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c | 6 +--- > drivers/gpu/drm/nouveau/dispnv50/head.c | 13 ++-- > drivers/gpu/drm/omapdrm/omap_crtc.c | 7 ++--- > drivers/gpu/drm/rcar-du/rcar_du_crtc.c| 4 +-- > drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 7 +++-- > drivers/gpu/drm/tegra/dc.c| 5 +-- > drivers/gpu/drm/vc4/vc4_crtc.c | 8 ++--- > drivers/gpu/drm/vkms/vkms_crtc.c | 7 + > drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 9 +- > include/drm/drm_atomic_state_helper.h | 2 ++ > 18 files changed, 56 insertions(+), 81 deletions(-) Looks good to me: Acked-by: Thierry Reding signature.asc Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/4] drm/edid: Pass connector to AVI inforframe functions
On Tue, Nov 20, 2018 at 06:13:42PM +0200, Ville Syrjala wrote: > From: Ville Syrjälä > > Make life easier for drivers by simply passing the connector > to drm_hdmi_avi_infoframe_from_display_mode() and > drm_hdmi_avi_infoframe_quant_range(). That way drivers don't > need to worry about is_hdmi2_sink mess. > > Cc: Alex Deucher > Cc: "Christian König" > Cc: "David (ChunMing) Zhou" > Cc: Archit Taneja > Cc: Andrzej Hajda > Cc: Laurent Pinchart > Cc: Inki Dae > Cc: Joonyoung Shim > Cc: Seung-Woo Kim > Cc: Kyungmin Park > Cc: Russell King > Cc: CK Hu > Cc: Philipp Zabel > Cc: Rob Clark > Cc: Ben Skeggs > Cc: Tomi Valkeinen > Cc: Sandy Huang > Cc: "Heiko Stübner" > Cc: Benjamin Gaignard > Cc: Vincent Abriou > Cc: Thierry Reding > Cc: Eric Anholt > Cc: Shawn Guo > Cc: Ilia Mirkin > Cc: amd-...@lists.freedesktop.org > Cc: linux-arm-...@vger.kernel.org > Cc: freedr...@lists.freedesktop.org > Cc: nouv...@lists.freedesktop.org > Cc: linux-te...@vger.kernel.org > Signed-off-by: Ville Syrjälä > --- > drivers/gpu/drm/amd/amdgpu/dce_v10_0.c| 2 +- > drivers/gpu/drm/amd/amdgpu/dce_v11_0.c| 2 +- > drivers/gpu/drm/amd/amdgpu/dce_v6_0.c | 3 ++- > drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 2 +- > drivers/gpu/drm/bridge/analogix-anx78xx.c | 5 ++-- > drivers/gpu/drm/bridge/sii902x.c | 3 ++- > drivers/gpu/drm/bridge/sil-sii8620.c | 3 +-- > drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 3 ++- > drivers/gpu/drm/drm_edid.c| 33 ++- > drivers/gpu/drm/exynos/exynos_hdmi.c | 3 ++- > drivers/gpu/drm/i2c/tda998x_drv.c | 3 ++- > drivers/gpu/drm/i915/intel_hdmi.c | 14 +- > drivers/gpu/drm/i915/intel_lspcon.c | 15 ++- > drivers/gpu/drm/i915/intel_sdvo.c | 10 --- > drivers/gpu/drm/mediatek/mtk_hdmi.c | 3 ++- > drivers/gpu/drm/msm/hdmi/hdmi_bridge.c| 3 ++- > drivers/gpu/drm/nouveau/dispnv50/disp.c | 7 +++-- > drivers/gpu/drm/omapdrm/omap_encoder.c| 5 ++-- > drivers/gpu/drm/radeon/radeon_audio.c | 2 +- > drivers/gpu/drm/rockchip/inno_hdmi.c | 4 ++- > drivers/gpu/drm/sti/sti_hdmi.c| 3 ++- > drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c| 3 ++- > drivers/gpu/drm/tegra/hdmi.c | 3 ++- > drivers/gpu/drm/tegra/sor.c | 3 ++- > drivers/gpu/drm/vc4/vc4_hdmi.c | 11 +--- > drivers/gpu/drm/zte/zx_hdmi.c | 4 ++- > include/drm/drm_edid.h| 8 +++--- > 27 files changed, 94 insertions(+), 66 deletions(-) That's actually a lot nicer: Acked-by: Thierry Reding signature.asc Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm: Document code of conduct
On Tue, Apr 11, 2017 at 08:48:15AM +0200, Daniel Vetter wrote: > freedesktop.org has adopted a formal&enforced code of conduct: > > https://www.fooishbar.org/blog/fdo-contributor-covenant/ > https://www.freedesktop.org/wiki/CodeOfConduct/ > > Besides formalizing things a bit more I don't think this changes > anything for us, we've already peer-enforced respectful and > constructive interactions since a long time. But it's good to document > things properly. > > Note: As Daniel Stone mentioned in the announcement fd.o admins > started chatting with the communities their hosting, which includs the > X.org foundation board, to figure out how to fan out enforcement and > allow projects to run things on their own (with fd.o still as the > fallback). So the details of enforcement (and appealing decisions) > might still change, but since this involves the board and lots more > people it'll take a while to get there. For now this is good enough I > think. > > For the text itself I went with the same blurb as the Wayland project, > didn't feel creative yet this early in the morning: > > https://cgit.freedesktop.org/wayland/wayland/commit/?id=0eefe99fe0683ae409b665a8b18cc7eb648c6c0c > > Cc: Daniel Stone > Cc: Keith Packard > Cc: tfh...@err.no > Signed-off-by: Daniel Vetter > --- > Documentation/gpu/introduction.rst | 11 +++ > 1 file changed, 11 insertions(+) Acked-by: Thierry Reding signature.asc Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v2 2/9] drm/fb-helper: Reshuffle code for subsequent patches
From: Thierry Reding
An unlocked version of the drm_fb_helper_add_one_connector() function
will be added in a subsequent patch. Reshuffle the code separately to
make the diff more readable later on.
Signed-off-by: Thierry Reding
---
drivers/gpu/drm/drm_fb_helper.c | 60 -
1 file changed, 30 insertions(+), 30 deletions(-)
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 7945620e7439..301644d12013 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -95,6 +95,36 @@ static LIST_HEAD(kernel_fb_helper_list);
* mmap page writes.
*/
+int drm_fb_helper_add_one_connector(struct drm_fb_helper *fb_helper, struct
drm_connector *connector)
+{
+ struct drm_fb_helper_connector **temp;
+ struct drm_fb_helper_connector *fb_helper_connector;
+
+ if (!drm_fbdev_emulation)
+ return 0;
+
+ WARN_ON(!mutex_is_locked(&fb_helper->dev->mode_config.mutex));
+ if (fb_helper->connector_count + 1 >
fb_helper->connector_info_alloc_count) {
+ temp = krealloc(fb_helper->connector_info, sizeof(struct
drm_fb_helper_connector *) * (fb_helper->connector_count + 1), GFP_KERNEL);
+ if (!temp)
+ return -ENOMEM;
+
+ fb_helper->connector_info_alloc_count =
fb_helper->connector_count + 1;
+ fb_helper->connector_info = temp;
+ }
+
+
+ fb_helper_connector = kzalloc(sizeof(struct drm_fb_helper_connector),
GFP_KERNEL);
+ if (!fb_helper_connector)
+ return -ENOMEM;
+
+ drm_connector_reference(connector);
+ fb_helper_connector->connector = connector;
+ fb_helper->connector_info[fb_helper->connector_count++] =
fb_helper_connector;
+ return 0;
+}
+EXPORT_SYMBOL(drm_fb_helper_add_one_connector);
+
/**
* drm_fb_helper_single_add_all_connectors() - add all connectors to fbdev
*emulation helper
@@ -139,36 +169,6 @@ fail:
}
EXPORT_SYMBOL(drm_fb_helper_single_add_all_connectors);
-int drm_fb_helper_add_one_connector(struct drm_fb_helper *fb_helper, struct
drm_connector *connector)
-{
- struct drm_fb_helper_connector **temp;
- struct drm_fb_helper_connector *fb_helper_connector;
-
- if (!drm_fbdev_emulation)
- return 0;
-
- WARN_ON(!mutex_is_locked(&fb_helper->dev->mode_config.mutex));
- if (fb_helper->connector_count + 1 >
fb_helper->connector_info_alloc_count) {
- temp = krealloc(fb_helper->connector_info, sizeof(struct
drm_fb_helper_connector *) * (fb_helper->connector_count + 1), GFP_KERNEL);
- if (!temp)
- return -ENOMEM;
-
- fb_helper->connector_info_alloc_count =
fb_helper->connector_count + 1;
- fb_helper->connector_info = temp;
- }
-
-
- fb_helper_connector = kzalloc(sizeof(struct drm_fb_helper_connector),
GFP_KERNEL);
- if (!fb_helper_connector)
- return -ENOMEM;
-
- drm_connector_reference(connector);
- fb_helper_connector->connector = connector;
- fb_helper->connector_info[fb_helper->connector_count++] =
fb_helper_connector;
- return 0;
-}
-EXPORT_SYMBOL(drm_fb_helper_add_one_connector);
-
int drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper,
struct drm_connector *connector)
{
--
2.8.3
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v2 7/9] drm/atmel-hlcdc: Remove custom FB helper deferred setup
From: Thierry Reding
The FB helper core now supports deferred setup, so the driver's custom
implementation can be removed.
Signed-off-by: Thierry Reding
---
drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 26 +++---
1 file changed, 15 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
index 8ded7645747e..a8a855910885 100644
--- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
+++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
@@ -431,15 +431,7 @@ static void atmel_hlcdc_fb_output_poll_changed(struct
drm_device *dev)
{
struct atmel_hlcdc_dc *dc = dev->dev_private;
- if (dc->fbdev) {
- drm_fbdev_cma_hotplug_event(dc->fbdev);
- } else {
- dc->fbdev = drm_fbdev_cma_init(dev, 24,
- dev->mode_config.num_crtc,
- dev->mode_config.num_connector);
- if (IS_ERR(dc->fbdev))
- dc->fbdev = NULL;
- }
+ drm_fbdev_cma_hotplug_event(dc->fbdev);
}
struct atmel_hlcdc_dc_commit {
@@ -654,11 +646,23 @@ static int atmel_hlcdc_dc_load(struct drm_device *dev)
drm_kms_helper_poll_init(dev);
- /* force connectors detection */
- drm_helper_hpd_irq_event(dev);
+ dc->fbdev = drm_fbdev_cma_init(dev, 24, dev->mode_config.num_crtc,
+ dev->mode_config.num_connector);
+ if (IS_ERR(dc->fbdev)) {
+ dev_err(dev->dev, "failed to setup fbdev\n");
+ ret = PTR_ERR(dc->fbdev);
+ goto err_cleanup_poll;
+ }
return 0;
+err_cleanup_poll:
+ drm_kms_helper_poll_fini(dev);
+
+ pm_runtime_get_sync(dev->dev);
+ drm_irq_uninstall(dev);
+ pm_runtime_put_sync(dev->dev);
+
err_periph_clk_disable:
pm_runtime_disable(dev->dev);
clk_disable_unprepare(dc->hlcdc->periph_clk);
--
2.8.3
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v2 0/9] drm/fb-helper: Deferred setup support
From: Thierry Reding This set of patches adds support for deferring FB helper setup, which is useful to obtain a sane configuration even when no outputs are available during probe. One example is HDMI, where fbdev will currently fallback to a 1024x786 resolution if no monitor is connected, and will then forever stay that way. With these patches, the FB helpers will take note that it doesn't make sense to setup fbdev yet and will defer until a monitor is connected, at which point the preferred mode will be selected. Thierry Changes in v2: - now with locking Thierry Reding (9): drm/fb-helper: Cleanup checkpatch warnings drm/fb-helper: Reshuffle code for subsequent patches drm/fb-helper: Improve code readability drm/fb-helper: Push down modeset lock into FB helpers drm/fb-helper: Add top-level lock drm/fb-helper: Support deferred setup drm/atmel-hlcdc: Remove custom FB helper deferred setup drm/exynos: Remove custom FB helper deferred setup drm/hisilicon: Remove custom FB helper deferred setup drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c| 26 +-- drivers/gpu/drm/drm_fb_helper.c | 217 +--- drivers/gpu/drm/exynos/exynos_drm_drv.c | 8 +- drivers/gpu/drm/exynos/exynos_drm_fbdev.c | 2 - drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c | 22 +-- drivers/gpu/drm/i915/intel_dp_mst.c | 4 - drivers/gpu/drm/radeon/radeon_dp_mst.c | 7 - include/drm/drm_fb_helper.h | 26 +++ 8 files changed, 217 insertions(+), 95 deletions(-) -- 2.8.3 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v2 6/9] drm/fb-helper: Support deferred setup
From: Thierry Reding
FB helper code falls back to a 1024x768 mode if no outputs are connected
or don't report back any modes upon initialization. This can be annoying
because outputs that are added to FB helper later on can't be used with
FB helper if they don't support a matching mode.
The fallback is in place because VGA connectors can happen to report an
unknown connection status even when they are in fact connected.
Some drivers have custom solutions in place to defer FB helper setup
until at least one output is connected. But the logic behind these
solutions is always the same and there is nothing driver-specific about
it, so a better alterative is to fix the FB helper core and add support
for all drivers automatically.
This patch adds support for deferred FB helper setup. It checks all the
connectors for their connection status, and if all of them report to be
disconnected marks the FB helper as needing deferred setup. Whet setup
is deferred, the FB helper core will automatically retry setup after a
hotplug event, and it will keep trying until it succeeds.
Signed-off-by: Thierry Reding
---
drivers/gpu/drm/drm_fb_helper.c | 36
include/drm/drm_fb_helper.h | 21 +
2 files changed, 57 insertions(+)
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index f7722bcb0064..0911b10c711c 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -487,6 +487,9 @@ int drm_fb_helper_restore_fbdev_mode_unlocked(struct
drm_fb_helper *fb_helper)
if (!drm_fbdev_emulation)
return -ENODEV;
+ if (fb_helper->deferred_setup)
+ return 0;
+
mutex_lock(&fb_helper->lock);
drm_modeset_lock_all(dev);
@@ -1452,6 +1455,23 @@ unlock:
}
EXPORT_SYMBOL(drm_fb_helper_pan_display);
+static bool drm_fb_helper_maybe_connected(struct drm_fb_helper *helper)
+{
+ bool connected = false;
+ unsigned int i;
+
+ for (i = 0; i < helper->connector_count; i++) {
+ struct drm_fb_helper_connector *fb = helper->connector_info[i];
+
+ if (fb->connector->status != connector_status_disconnected) {
+ connected = true;
+ break;
+ }
+ }
+
+ return connected;
+}
+
/*
* Allocates the backing storage and sets up the fbdev info structure through
* the ->fb_probe callback and then registers the fbdev and sets up the panic
@@ -1554,6 +1574,17 @@ static int drm_fb_helper_single_fb_probe(struct
drm_fb_helper *fb_helper,
sizes.fb_height = min_t(u32, desired_mode->vdisplay +
y, sizes.fb_height);
}
+ /*
+* If everything's disconnected, there's no use in attempting to set
+* up fbdev.
+*/
+ if (!drm_fb_helper_maybe_connected(fb_helper)) {
+ DRM_INFO("No outputs connected, deferring setup\n");
+ fb_helper->preferred_bpp = preferred_bpp;
+ fb_helper->deferred_setup = true;
+ return 0;
+ }
+
if (crtc_count == 0 || sizes.fb_width == -1 || sizes.fb_height == -1) {
/* hmm everyone went away - assume VGA cable just fell out
and will come back later. */
@@ -1593,6 +1624,7 @@ static int drm_fb_helper_single_fb_probe(struct
drm_fb_helper *fb_helper,
list_add(&fb_helper->kernel_fb_list, &kernel_fb_helper_list);
+ fb_helper->deferred_setup = false;
return 0;
}
@@ -2294,6 +2326,10 @@ int drm_fb_helper_hotplug_event(struct drm_fb_helper
*fb_helper)
if (!drm_fbdev_emulation)
return 0;
+ if (fb_helper->deferred_setup)
+ return drm_fb_helper_initial_config(fb_helper,
+ fb_helper->preferred_bpp);
+
mutex_lock(&fb_helper->lock);
mutex_lock(&fb_helper->dev->mode_config.mutex);
diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
index 7739be08ebca..dac155ad33f6 100644
--- a/include/drm/drm_fb_helper.h
+++ b/include/drm/drm_fb_helper.h
@@ -219,6 +219,27 @@ struct drm_fb_helper {
bool delayed_hotplug;
/**
+* @deferred_setup:
+*
+* If no outputs are connected (disconnected or unknown) the FB helper
+* code will defer setup until at least one of the outputs shows up.
+* This field keeps track of the status so that setup can be retried
+* at every hotplug event until it succeeds eventually.
+*/
+ bool deferred_setup;
+
+ /**
+* @preferred_bpp:
+*
+* Temporary storage for the driver's preferred BPP setting passed to
+* FB helper initialization. This needs to be tracked so that deferred
+* FB he
[Intel-gfx] [PATCH v2 3/9] drm/fb-helper: Improve code readability
From: Thierry Reding
Add a couple of temporary variables and use shorter names for existing
variables in drm_fb_helper_add_one_connector() for better readability.
Signed-off-by: Thierry Reding
---
drivers/gpu/drm/drm_fb_helper.c | 26 --
1 file changed, 16 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 301644d12013..9afd4208596f 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -95,32 +95,38 @@ static LIST_HEAD(kernel_fb_helper_list);
* mmap page writes.
*/
-int drm_fb_helper_add_one_connector(struct drm_fb_helper *fb_helper, struct
drm_connector *connector)
+int drm_fb_helper_add_one_connector(struct drm_fb_helper *fb_helper,
+ struct drm_connector *connector)
{
struct drm_fb_helper_connector **temp;
- struct drm_fb_helper_connector *fb_helper_connector;
+ struct drm_fb_helper_connector *conn;
+ unsigned int count;
if (!drm_fbdev_emulation)
return 0;
WARN_ON(!mutex_is_locked(&fb_helper->dev->mode_config.mutex));
- if (fb_helper->connector_count + 1 >
fb_helper->connector_info_alloc_count) {
- temp = krealloc(fb_helper->connector_info, sizeof(struct
drm_fb_helper_connector *) * (fb_helper->connector_count + 1), GFP_KERNEL);
+
+ count = fb_helper->connector_count + 1;
+
+ if (count > fb_helper->connector_info_alloc_count) {
+ size_t size = count * sizeof(conn);
+
+ temp = krealloc(fb_helper->connector_info, size, GFP_KERNEL);
if (!temp)
return -ENOMEM;
- fb_helper->connector_info_alloc_count =
fb_helper->connector_count + 1;
+ fb_helper->connector_info_alloc_count = count;
fb_helper->connector_info = temp;
}
-
- fb_helper_connector = kzalloc(sizeof(struct drm_fb_helper_connector),
GFP_KERNEL);
- if (!fb_helper_connector)
+ conn = kzalloc(sizeof(*conn), GFP_KERNEL);
+ if (!conn)
return -ENOMEM;
drm_connector_reference(connector);
- fb_helper_connector->connector = connector;
- fb_helper->connector_info[fb_helper->connector_count++] =
fb_helper_connector;
+ conn->connector = connector;
+ fb_helper->connector_info[fb_helper->connector_count++] = conn;
return 0;
}
EXPORT_SYMBOL(drm_fb_helper_add_one_connector);
--
2.8.3
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v2 8/9] drm/exynos: Remove custom FB helper deferred setup
From: Thierry Reding The FB helper core now supports deferred setup, so the driver's custom implementation can be removed. Signed-off-by: Thierry Reding --- drivers/gpu/drm/exynos/exynos_drm_drv.c | 8 ++-- drivers/gpu/drm/exynos/exynos_drm_fbdev.c | 2 -- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c index 2dd820e23b0c..259c2585c703 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c @@ -215,11 +215,15 @@ static int exynos_drm_load(struct drm_device *dev, unsigned long flags) /* init kms poll for handling hpd */ drm_kms_helper_poll_init(dev); - /* force connectors detection */ - drm_helper_hpd_irq_event(dev); + ret = exynos_drm_fbdev_init(dev); + if (ret) + goto err_cleanup_poll; return 0; +err_cleanup_poll: + drm_kms_helper_poll_fini(dev); + exynos_drm_device_subdrv_remove(dev); err_cleanup_vblank: drm_vblank_cleanup(dev); err_unbind_all: diff --git a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c index 67dcd6831291..83a277946200 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c +++ b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c @@ -319,6 +319,4 @@ void exynos_drm_output_poll_changed(struct drm_device *dev) if (fb_helper) drm_fb_helper_hotplug_event(fb_helper); - else - exynos_drm_fbdev_init(dev); } -- 2.8.3 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v2 5/9] drm/fb-helper: Add top-level lock
From: Thierry Reding
Introduce a new top-level lock for the FB helper code. This will allow
better locking granularity and avoid the need to abuse modeset locking
for this purpose instead.
Signed-off-by: Thierry Reding
---
drivers/gpu/drm/drm_fb_helper.c | 27 ++-
include/drm/drm_fb_helper.h | 5 +
2 files changed, 31 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 252c7557bdb5..f7722bcb0064 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -105,6 +105,7 @@ static int __drm_fb_helper_add_one_connector(struct
drm_fb_helper *fb_helper,
if (!drm_fbdev_emulation)
return 0;
+ WARN_ON(!mutex_is_locked(&fb_helper->lock));
WARN_ON(!mutex_is_locked(&fb_helper->dev->mode_config.mutex));
count = fb_helper->connector_count + 1;
@@ -127,6 +128,7 @@ static int __drm_fb_helper_add_one_connector(struct
drm_fb_helper *fb_helper,
drm_connector_reference(connector);
conn->connector = connector;
fb_helper->connector_info[fb_helper->connector_count++] = conn;
+
return 0;
}
@@ -135,11 +137,13 @@ int drm_fb_helper_add_one_connector(struct drm_fb_helper
*fb_helper,
{
int err;
+ mutex_lock(&fb_helper->lock);
mutex_lock(&fb_helper->dev->mode_config.mutex);
err = __drm_fb_helper_add_one_connector(fb_helper, connector);
mutex_unlock(&fb_helper->dev->mode_config.mutex);
+ mutex_unlock(&fb_helper->lock);
return err;
}
@@ -168,6 +172,7 @@ int drm_fb_helper_single_add_all_connectors(struct
drm_fb_helper *fb_helper)
if (!drm_fbdev_emulation)
return 0;
+ mutex_lock(&fb_helper->lock);
mutex_lock(&dev->mode_config.mutex);
drm_for_each_connector(connector, dev) {
@@ -184,6 +189,7 @@ int drm_fb_helper_single_add_all_connectors(struct
drm_fb_helper *fb_helper)
}
mutex_unlock(&dev->mode_config.mutex);
+ mutex_unlock(&fb_helper->lock);
return ret;
}
@@ -198,6 +204,7 @@ static int __drm_fb_helper_remove_one_connector(struct
drm_fb_helper *fb_helper,
if (!drm_fbdev_emulation)
return 0;
+ WARN_ON(!mutex_is_locked(&fb_helper->lock));
WARN_ON(!mutex_is_locked(&fb_helper->dev->mode_config.mutex));
for (i = 0; i < fb_helper->connector_count; i++) {
@@ -225,11 +232,13 @@ int drm_fb_helper_remove_one_connector(struct
drm_fb_helper *fb_helper,
{
int err;
+ mutex_lock(&fb_helper->lock);
mutex_lock(&fb_helper->dev->mode_config.mutex);
err = __drm_fb_helper_remove_one_connector(fb_helper, connector);
mutex_unlock(&fb_helper->dev->mode_config.mutex);
+ mutex_unlock(&fb_helper->lock);
return err;
}
@@ -478,16 +487,21 @@ int drm_fb_helper_restore_fbdev_mode_unlocked(struct
drm_fb_helper *fb_helper)
if (!drm_fbdev_emulation)
return -ENODEV;
+ mutex_lock(&fb_helper->lock);
drm_modeset_lock_all(dev);
+
ret = restore_fbdev_mode(fb_helper);
do_delayed = fb_helper->delayed_hotplug;
if (do_delayed)
fb_helper->delayed_hotplug = false;
+
drm_modeset_unlock_all(dev);
+ mutex_unlock(&fb_helper->lock);
if (do_delayed)
drm_fb_helper_hotplug_event(fb_helper);
+
return ret;
}
EXPORT_SYMBOL(drm_fb_helper_restore_fbdev_mode_unlocked);
@@ -688,6 +702,7 @@ void drm_fb_helper_prepare(struct drm_device *dev, struct
drm_fb_helper *helper,
spin_lock_init(&helper->dirty_lock);
INIT_WORK(&helper->dirty_work, drm_fb_helper_dirty_work);
helper->dirty_clip.x1 = helper->dirty_clip.y1 = ~0;
+ mutex_init(&helper->lock);
helper->funcs = funcs;
helper->dev = dev;
}
@@ -853,6 +868,7 @@ void drm_fb_helper_fini(struct drm_fb_helper *fb_helper)
unregister_sysrq_key('v',
&sysrq_drm_fb_helper_restore_op);
}
+ mutex_destroy(&fb_helper->lock);
drm_fb_helper_crtc_free(fb_helper);
}
@@ -2273,16 +2289,20 @@ int drm_fb_helper_hotplug_event(struct drm_fb_helper
*fb_helper)
{
struct drm_device *dev = fb_helper->dev;
u32 max_width, max_height;
+ int err = 0;
if (!drm_fbdev_emulation)
return 0;
+ mutex_lock(&fb_helper->lock);
mutex_lock(&fb_helper->dev->mode_config.mutex);
+
if (!fb_helper->fb || !drm_fb_helper_is_bound(fb_helper)) {
fb_helper->delayed_hotplug = true;
mutex_unlock(&fb_helper->dev->mode_config.mutex);
-
[Intel-gfx] [PATCH v2 1/9] drm/fb-helper: Cleanup checkpatch warnings
From: Thierry Reding
Fix up a couple of checkpatch warnings, such as whitespace or coding
style issues.
Signed-off-by: Thierry Reding
---
drivers/gpu/drm/drm_fb_helper.c | 33 +++--
1 file changed, 19 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 7c2eb75db60f..7945620e7439 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -190,9 +190,9 @@ int drm_fb_helper_remove_one_connector(struct drm_fb_helper
*fb_helper,
fb_helper_connector = fb_helper->connector_info[i];
drm_connector_unreference(fb_helper_connector->connector);
- for (j = i + 1; j < fb_helper->connector_count; j++) {
+ for (j = i + 1; j < fb_helper->connector_count; j++)
fb_helper->connector_info[j - 1] = fb_helper->connector_info[j];
- }
+
fb_helper->connector_count--;
kfree(fb_helper_connector);
@@ -290,6 +290,7 @@ int drm_fb_helper_debug_leave(struct fb_info *info)
for (i = 0; i < helper->crtc_count; i++) {
struct drm_mode_set *mode_set = &helper->crtc_info[i].mode_set;
+
crtc = mode_set->crtc;
funcs = crtc->helper_private;
fb = drm_mode_config_fb(crtc);
@@ -317,7 +318,7 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper
*fb_helper)
struct drm_plane *plane;
struct drm_atomic_state *state;
int i, ret;
- unsigned plane_mask;
+ unsigned int plane_mask;
state = drm_atomic_state_alloc(dev);
if (!state)
@@ -349,7 +350,7 @@ retry:
goto fail;
}
- for(i = 0; i < fb_helper->crtc_count; i++) {
+ for (i = 0; i < fb_helper->crtc_count; i++) {
struct drm_mode_set *mode_set =
&fb_helper->crtc_info[i].mode_set;
ret = __drm_atomic_helper_set_config(mode_set, state);
@@ -511,6 +512,7 @@ static bool drm_fb_helper_force_kernel_mode(void)
static void drm_fb_helper_restore_work_fn(struct work_struct *ignored)
{
bool ret;
+
ret = drm_fb_helper_force_kernel_mode();
if (ret == true)
DRM_ERROR("Failed to restore crtc configuration\n");
@@ -812,9 +814,8 @@ void drm_fb_helper_fini(struct drm_fb_helper *fb_helper)
if (!list_empty(&fb_helper->kernel_fb_list)) {
list_del(&fb_helper->kernel_fb_list);
- if (list_empty(&kernel_fb_helper_list)) {
+ if (list_empty(&kernel_fb_helper_list))
unregister_sysrq_key('v',
&sysrq_drm_fb_helper_restore_op);
- }
}
drm_fb_helper_crtc_free(fb_helper);
@@ -1059,6 +1060,7 @@ static int setcolreg(struct drm_crtc *crtc, u16 red, u16
green,
(blue << info->var.blue.offset);
if (info->var.transp.length > 0) {
u32 mask = (1 << info->var.transp.length) - 1;
+
mask <<= info->var.transp.offset;
value |= mask;
}
@@ -1087,6 +1089,7 @@ static int setcolreg(struct drm_crtc *crtc, u16 red, u16
green,
if (fb->depth == 16) {
u16 r, g, b;
int i;
+
if (regno < 32) {
for (i = 0; i < 8; i++)
fb_helper->funcs->gamma_set(crtc, red,
@@ -1298,7 +1301,7 @@ static int pan_display_atomic(struct fb_var_screeninfo
*var,
struct drm_atomic_state *state;
struct drm_plane *plane;
int i, ret;
- unsigned plane_mask;
+ unsigned int plane_mask;
state = drm_atomic_state_alloc(dev);
if (!state)
@@ -1307,7 +1310,7 @@ static int pan_display_atomic(struct fb_var_screeninfo
*var,
state->acquire_ctx = dev->mode_config.acquire_ctx;
retry:
plane_mask = 0;
- for(i = 0; i < fb_helper->crtc_count; i++) {
+ for (i = 0; i < fb_helper->crtc_count; i++) {
struct drm_mode_set *mode_set;
mode_set = &fb_helper->crtc_info[i].mode_set;
@@ -1416,8 +1419,8 @@ static int drm_fb_helper_single_fb_probe(struct
drm_fb_helper *fb_helper,
memset(&sizes, 0, sizeof(struct drm_fb_helper_surface_size));
sizes.surface_depth = 24;
sizes.surface_bpp = 32;
- sizes.fb_width = (unsigned)-1;
- sizes.fb_height = (unsigned)-1;
+ sizes.fb_width = (u32)-1;
+ sizes.fb_height = (u32)-1;
/* if driver picks 8 or 16 by default use that
for both depth/bpp */
@@ -1485,6 +1488,7 @@ static int drm_fb_helper_single_fb_probe(struct
drm_fb_helper *fb_helper,
[Intel-gfx] [PATCH v2 9/9] drm/hisilicon: Remove custom FB helper deferred setup
From: Thierry Reding
The FB helper core now supports deferred setup, so the driver's custom
implementation can be removed.
Signed-off-by: Thierry Reding
---
drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c | 22 +++---
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c
b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c
index 3f94785fbcca..0e0bd77e4499 100644
--- a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c
+++ b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c
@@ -54,15 +54,7 @@ static void kirin_fbdev_output_poll_changed(struct
drm_device *dev)
{
struct kirin_drm_private *priv = dev->dev_private;
- if (priv->fbdev) {
- drm_fbdev_cma_hotplug_event(priv->fbdev);
- } else {
- priv->fbdev = drm_fbdev_cma_init(dev, 32,
- dev->mode_config.num_crtc,
- dev->mode_config.num_connector);
- if (IS_ERR(priv->fbdev))
- priv->fbdev = NULL;
- }
+ drm_fbdev_cma_hotplug_event(priv->fbdev);
}
#endif
@@ -129,11 +121,19 @@ static int kirin_drm_kms_init(struct drm_device *dev)
/* init kms poll for handling hpd */
drm_kms_helper_poll_init(dev);
- /* force detection after connectors init */
- (void)drm_helper_hpd_irq_event(dev);
+ priv->fbdev = drm_fbdev_cma_init(dev, 32, dev->mode_config.num_crtc,
+dev->mode_config.num_connector);
+ if (IS_ERR(priv->fbdev)) {
+ DRM_ERROR("failed to initialize fbdev.\n");
+ ret = PTR_ERR(priv->fbdev);
+ goto err_cleanup_poll;
+ }
return 0;
+err_cleanup_poll:
+ drm_kms_helper_poll_fini(dev);
+ drm_vblank_cleanup(dev);
err_unbind_all:
component_unbind_all(dev->dev, dev);
err_dc_cleanup:
--
2.8.3
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v2 4/9] drm/fb-helper: Push down modeset lock into FB helpers
From: Thierry Reding
Move the modeset locking from drivers into FB helpers.
Signed-off-by: Thierry Reding
---
drivers/gpu/drm/drm_fb_helper.c| 59 +-
drivers/gpu/drm/i915/intel_dp_mst.c| 4 ---
drivers/gpu/drm/radeon/radeon_dp_mst.c | 7
3 files changed, 44 insertions(+), 26 deletions(-)
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 9afd4208596f..252c7557bdb5 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -95,8 +95,8 @@ static LIST_HEAD(kernel_fb_helper_list);
* mmap page writes.
*/
-int drm_fb_helper_add_one_connector(struct drm_fb_helper *fb_helper,
- struct drm_connector *connector)
+static int __drm_fb_helper_add_one_connector(struct drm_fb_helper *fb_helper,
+struct drm_connector *connector)
{
struct drm_fb_helper_connector **temp;
struct drm_fb_helper_connector *conn;
@@ -129,6 +129,20 @@ int drm_fb_helper_add_one_connector(struct drm_fb_helper
*fb_helper,
fb_helper->connector_info[fb_helper->connector_count++] = conn;
return 0;
}
+
+int drm_fb_helper_add_one_connector(struct drm_fb_helper *fb_helper,
+ struct drm_connector *connector)
+{
+ int err;
+
+ mutex_lock(&fb_helper->dev->mode_config.mutex);
+
+ err = __drm_fb_helper_add_one_connector(fb_helper, connector);
+
+ mutex_unlock(&fb_helper->dev->mode_config.mutex);
+
+ return err;
+}
EXPORT_SYMBOL(drm_fb_helper_add_one_connector);
/**
@@ -155,28 +169,28 @@ int drm_fb_helper_single_add_all_connectors(struct
drm_fb_helper *fb_helper)
return 0;
mutex_lock(&dev->mode_config.mutex);
+
drm_for_each_connector(connector, dev) {
- ret = drm_fb_helper_add_one_connector(fb_helper, connector);
+ ret = __drm_fb_helper_add_one_connector(fb_helper, connector);
+ if (ret) {
+ for (i = 0; i < fb_helper->connector_count; i++) {
+ kfree(fb_helper->connector_info[i]);
+ fb_helper->connector_info[i] = NULL;
+ }
- if (ret)
- goto fail;
- }
- mutex_unlock(&dev->mode_config.mutex);
- return 0;
-fail:
- for (i = 0; i < fb_helper->connector_count; i++) {
- kfree(fb_helper->connector_info[i]);
- fb_helper->connector_info[i] = NULL;
+ fb_helper->connector_count = 0;
+ break;
+ }
}
- fb_helper->connector_count = 0;
+
mutex_unlock(&dev->mode_config.mutex);
return ret;
}
EXPORT_SYMBOL(drm_fb_helper_single_add_all_connectors);
-int drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper,
- struct drm_connector *connector)
+static int __drm_fb_helper_remove_one_connector(struct drm_fb_helper
*fb_helper,
+ struct drm_connector *connector)
{
struct drm_fb_helper_connector *fb_helper_connector;
int i, j;
@@ -193,6 +207,7 @@ int drm_fb_helper_remove_one_connector(struct drm_fb_helper
*fb_helper,
if (i == fb_helper->connector_count)
return -EINVAL;
+
fb_helper_connector = fb_helper->connector_info[i];
drm_connector_unreference(fb_helper_connector->connector);
@@ -204,6 +219,20 @@ int drm_fb_helper_remove_one_connector(struct
drm_fb_helper *fb_helper,
return 0;
}
+
+int drm_fb_helper_remove_one_connector(struct drm_fb_helper *fb_helper,
+ struct drm_connector *connector)
+{
+ int err;
+
+ mutex_lock(&fb_helper->dev->mode_config.mutex);
+
+ err = __drm_fb_helper_remove_one_connector(fb_helper, connector);
+
+ mutex_unlock(&fb_helper->dev->mode_config.mutex);
+
+ return err;
+}
EXPORT_SYMBOL(drm_fb_helper_remove_one_connector);
static void drm_fb_helper_save_lut_atomic(struct drm_crtc *crtc, struct
drm_fb_helper *helper)
diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c
b/drivers/gpu/drm/i915/intel_dp_mst.c
index 7a34090cef34..47c3980cb3b9 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -477,9 +477,7 @@ static void intel_dp_register_mst_connector(struct
drm_connector *connector)
{
struct intel_connector *intel_connector = to_intel_connector(connector);
struct drm_device *dev = connector->dev;
- drm_modeset_lock_all(dev);
intel_connector_add_to_fbdev(intel_connector);
- drm_modeset_unlock_all(dev);
drm_connector_register(&intel_connector->base);
}
@@ -492,10 +490,8 @@
Re: [Intel-gfx] [PATCH 15/29] drm/tegra: Use drm_gem_object_unreference_unlocked
On Mon, Nov 23, 2015 at 10:32:48AM +0100, Daniel Vetter wrote: > This only grabs the mutex when really needed, but still has a > might-acquire lockdep check to make sure that's always possible. > With this patch tegra is officially struct_mutex free, yay! > > v2: refernce_unlocked doesn't exist as kbuild spotted. > > Cc: Thierry Reding > Acked-by: Thierry Reding > Signed-off-by: Daniel Vetter > --- > drivers/gpu/drm/tegra/drm.c | 4 +--- > drivers/gpu/drm/tegra/gem.c | 6 +- > 2 files changed, 2 insertions(+), 8 deletions(-) Applied, thanks. Thierry signature.asc Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 14/29] drm/tegra: don't take dev->struct_mutex in mmap offset ioctl
On Mon, Nov 23, 2015 at 10:32:47AM +0100, Daniel Vetter wrote: > Since David Herrmann's mmap vma manager rework we don't need to grab > dev->struct_mutex any more to prevent races when looking up the mmap > offset. Drop it and instead don't forget to use the unref_unlocked > variant (since the drm core still cares). > > v2: Finally get rid of the copypasta from another commit in this > commit message. And convert to _unlocked like we need to (Patrik). > > Cc: Patrik Jakobsson > Cc: Thierry Reding > Acked-by: Thierry Reding > Reviewed-by: Patrik Jakobsson > Signed-off-by: Daniel Vetter > --- > drivers/gpu/drm/tegra/gem.c | 7 +-- > 1 file changed, 1 insertion(+), 6 deletions(-) Applied, thanks. Thierry signature.asc Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 07/29] drm/tegra: Use unlocked gem unreferencing
On Mon, Nov 23, 2015 at 10:32:40AM +0100, Daniel Vetter wrote: > For drm_gem_object_unreference callers are required to hold > dev->struct_mutex, which these paths don't. Enforcing this requirement > has become a bit more strict with > > commit ef4c6270bf2867e2f8032e9614d1a8cfc6c71663 > Author: Daniel Vetter > Date: Thu Oct 15 09:36:25 2015 +0200 > > drm/gem: Check locking in drm_gem_object_unreference > > Cc: Thierry Reding > Signed-off-by: Daniel Vetter > --- > drivers/gpu/drm/tegra/drm.c | 10 +- > 1 file changed, 5 insertions(+), 5 deletions(-) Applied, thanks. Thierry signature.asc Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/9] drm/tegra: Assign conn_state->connector when allocating connector state.
On Tue, Nov 24, 2015 at 10:34:29AM +0100, Maarten Lankhorst wrote: > Signed-off-by: Maarten Lankhorst > --- > drivers/gpu/drm/tegra/dsi.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) Acked-by: Thierry Reding signature.asc Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/9] drm/tegra: Assign conn_state->connector when allocating connector state.
On Tue, Nov 24, 2015 at 11:37:47AM +0100, Daniel Vetter wrote:
> On Tue, Nov 24, 2015 at 10:34:29AM +0100, Maarten Lankhorst wrote:
> > Signed-off-by: Maarten Lankhorst
> > ---
> > drivers/gpu/drm/tegra/dsi.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/tegra/dsi.c b/drivers/gpu/drm/tegra/dsi.c
> > index f0a138ef68ce..6b8ae3d08eeb 100644
> > --- a/drivers/gpu/drm/tegra/dsi.c
> > +++ b/drivers/gpu/drm/tegra/dsi.c
> > @@ -751,8 +751,10 @@ static void tegra_dsi_connector_reset(struct
> > drm_connector *connector)
> > connector->state = NULL;
> >
> > state = kzalloc(sizeof(*state), GFP_KERNEL);
> > - if (state)
> > + if (state) {
> > + state->base.connector = connector;
> > connector->state = &state->base;
> > + }
>
> Hm, we might want __ versions of all the _reset hooks if this becomes more
> common. I do wonder a bit why it isn't since a lot of drivers overwrite
> state structures by now, and then the provided reset functions aren't
> sufficient really.
We already have the __ versions for duplicate_state helpers, but the
problem with reset helpers is that they need to know the allocation
size...
Actually, that's true of the duplicate_state helpers as well, and the __
variants don't actually allocate the memory but rather just copy the
state from old to new. So we could do something just like that for the
reset helpers:
void __drm_atomic_helper_connector_reset(struct drm_connector
*connector,
struct drm_connector_state
*state)
{
state->base.connector = connector;
connector->state = state;
}
and use like this:
static void tegra_dsi_connector_reset(struct drm_connector *connector)
{
struct tegra_dsi_state *state;
...
state = kzalloc(sizeof(*state), GFP_KERNEL);
if (state)
__drm_atomic_helper_connector_reset(connector,
&state->base);
}
I think back at the time when we did this for duplicate_state helpers we
had a discussion about the usefulness of this, but this patchset clearly
shows that this kind of change, which applies to a number of drivers, is
going to happen again and again, so the only way to avoid mistakes or an
extensive set of changes across all drivers is by putting this into a
helper, even if it's only two assignments now.
Thierry
signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/3] drm/atomic: Add __drm_atomic_helper_connector_reset.
On Tue, Nov 24, 2015 at 02:35:33PM +0100, Maarten Lankhorst wrote: > This is useful for drivers that subclass connector_state, like tegra. > > Signed-off-by: Maarten Lankhorst > --- > drivers/gpu/drm/drm_atomic_helper.c | 30 ++ > include/drm/drm_atomic_helper.h | 2 ++ > 2 files changed, 28 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c > b/drivers/gpu/drm/drm_atomic_helper.c > index cee31d43cd1c..5d23f818faec 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -2359,6 +2359,28 @@ void drm_atomic_helper_plane_destroy_state(struct > drm_plane *plane, > EXPORT_SYMBOL(drm_atomic_helper_plane_destroy_state); > > /** > + * __drm_atomic_helper_connector_reset - default ->reset hook for connectors That's misleading because you can't use this as a ->reset() hook as-is. Maybe something like this would be better as a synopsis: __drm_atomic_helper_connector_reset - reset connector state Then the description below gives the details about what it does and when to use it, which looks fine to me. > + * @connector: drm connector > + * @conn_state: connector state to assign > + * > + * Initializes conn_state and assigns it to connector->state, usually > + * required at when initializing the drivers or when called from the s/required at/required/ Thierry signature.asc Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/3] drm/atomic: Add __drm_atomic_helper_connector_reset.
On Tue, Nov 24, 2015 at 02:35:33PM +0100, Maarten Lankhorst wrote: > This is useful for drivers that subclass connector_state, like tegra. One more nit: s/tegra/Tegra/ Thierry signature.asc Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/3] drm/tegra: Use __drm_atomic_helper_reset_connector for subclassing connector state.
On Tue, Nov 24, 2015 at 02:35:34PM +0100, Maarten Lankhorst wrote:
> Signed-off-by: Maarten Lankhorst
> ---
> drivers/gpu/drm/tegra/dsi.c | 9 +++--
> 1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/tegra/dsi.c b/drivers/gpu/drm/tegra/dsi.c
> index f0a138ef68ce..33ad50487f2e 100644
> --- a/drivers/gpu/drm/tegra/dsi.c
> +++ b/drivers/gpu/drm/tegra/dsi.c
> @@ -745,14 +745,11 @@ static void tegra_dsi_soft_reset(struct tegra_dsi *dsi)
>
> static void tegra_dsi_connector_reset(struct drm_connector *connector)
> {
> - struct tegra_dsi_state *state;
> + struct tegra_dsi_state *state =
> + kzalloc(sizeof(*state), GFP_KERNEL);
I think this could use a check just for safety. It's unlikely to ever
happen, but just in case, better allow to fail gracefully than crash.
Thierry
signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 5/9] drm/atomic: add connector mask to drm_crtc_state.
On Tue, Nov 24, 2015 at 10:34:32AM +0100, Maarten Lankhorst wrote: > Signed-off-by: Maarten Lankhorst > --- > drivers/gpu/drm/drm_atomic.c | 11 +++ > include/drm/drm_crtc.h | 3 +++ > 2 files changed, 14 insertions(+) I think this could use a proper commit message. As it is, it's completely unclear as to what this mask will be useful for. Other than that looks good to me. Thierry signature.asc Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 8/9] drm/atomic: Remove drm_atomic_connectors_for_crtc.
On Tue, Nov 24, 2015 at 10:34:35AM +0100, Maarten Lankhorst wrote:
[...]
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> b/drivers/gpu/drm/drm_atomic_helper.c
> index cee31d43cd1c..fb79c54b2aed 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -420,8 +420,6 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
>* crtc only changed its mode but has the same set of connectors.
>*/
> for_each_crtc_in_state(state, crtc, crtc_state, i) {
> - int num_connectors;
> -
> /*
>* We must set ->active_changed after walking connectors for
>* otherwise an update that only changes active would result in
> @@ -449,10 +447,7 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
> if (ret != 0)
> return ret;
>
> - num_connectors = drm_atomic_connectors_for_crtc(state,
> - crtc);
> -
> - if (crtc_state->enable != !!num_connectors) {
> + if (crtc_state->enable != !!crtc_state->connector_mask) {
I have difficulty to doubly negate in my head, so something like this
would be a lot clearer in my opinion:
bool enable = crtc_state->connector_mask != 0;
...
if (crtc_state->enable != enable)
...
Or perhaps even:
bool disable = crtc_state->connector_mask == 0;
...
if (crtc_state->enable == disable)
...
> DRM_DEBUG_ATOMIC("[CRTC:%d] enabled/connectors
> mismatch\n",
>crtc->base.id);
>
> @@ -1668,7 +1663,7 @@ static int update_output_state(struct drm_atomic_state
> *state,
> if (crtc == set->crtc)
> continue;
>
> - if (!drm_atomic_connectors_for_crtc(state, crtc)) {
> + if (!crtc_state->connector_mask) {
Similarly I think it would be more natural to say:
if (crtc->state->connector_mask == 0)
here.
Anyway, this is mostly about personal taste, and the change looks
correct to me (after checking twice that I got the double negation
right), so:
Reviewed-by: Thierry Reding
signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 6/9] drm/i915: Update connector_mask during readout.
On Tue, Nov 24, 2015 at 10:34:33AM +0100, Maarten Lankhorst wrote: > Signed-off-by: Maarten Lankhorst > --- > drivers/gpu/drm/i915/intel_display.c | 11 --- > 1 file changed, 8 insertions(+), 3 deletions(-) Reviewed-by: Thierry Reding signature.asc Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 01/28] drm: Polish fbdev helper struct docs
On Fri, Dec 04, 2015 at 09:45:42AM +0100, Daniel Vetter wrote:
> Mostly this is just adding extensive docs for the callbacks, but also
> a few other additions.
>
> v2: Use FIXME comments to annotate helper hooks that should be
> replaced.
>
> Signed-off-by: Daniel Vetter
> ---
> include/drm/drm_fb_helper.h | 92
> ++---
> 1 file changed, 79 insertions(+), 13 deletions(-)
>
> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
> index 87b090c4b730..5ce033e36039 100644
> --- a/include/drm/drm_fb_helper.h
> +++ b/include/drm/drm_fb_helper.h
> @@ -74,25 +74,76 @@ struct drm_fb_helper_surface_size {
>
> /**
> * struct drm_fb_helper_funcs - driver callbacks for the fbdev emulation
> library
> - * @gamma_set: Set the given gamma lut register on the given crtc.
> - * @gamma_get: Read the given gamma lut register on the given crtc, used to
> - * save the current lut when force-restoring the fbdev for e.g.
> - * kdbg.
> - * @fb_probe: Driver callback to allocate and initialize the fbdev info
> - *structure. Furthermore it also needs to allocate the drm
> - *framebuffer used to back the fbdev.
> - * @initial_config: Setup an initial fbdev display configuration
> *
> * Driver callbacks used by the fbdev emulation helper library.
> */
> struct drm_fb_helper_funcs {
> + /**
> + * @gamma_set:
> + *
> + * Set the given gamma lut register on the given crtc.
> + *
> + * This callback is optional.
> + *
> + * FIXME:
> + *
> + * This callback is functionally redundant with the core gamma table
> + * support and simply exists because the fbdev hasn't yet been
> + * refactored to use the core gamma table interfaces.
> + */
> void (*gamma_set)(struct drm_crtc *crtc, u16 red, u16 green,
> u16 blue, int regno);
Pardon my ignorance, but is there a way to document parameters with this
new syntax?
> + /**
> + * @gamma_get:
> + *
> + * Read the given gamma lut register on the given crtc, used to save the
> + * current lut when force-restoring the fbdev for e.g. kdbg.
> + *
> + * This callback is optional.
> + *
> + * FIXME:
> + *
> + * This callback is functionally redundant with the core gamma table
> + * support and simply exists because the fbdev hasn't yet been
> + * refactored to use the core gamma table interfaces.
> + */
> void (*gamma_get)(struct drm_crtc *crtc, u16 *red, u16 *green,
> u16 *blue, int regno);
Nit: While at it, perhaps (here and in the @gamma_set documentation):
s/lut/LUT/ and s/crtc/CRTC/?
>
> + /**
> + * @fb_probe:
> + *
> + * Driver callback to allocate and initialize the fbdev info structure.
> + * Furthermore it also needs to allocate the drm framebuffer used to
> + * back the fbdev.
> + *
> + * This callback is mandatory.
> + *
> + * RETURNS:
> + *
> + * The driver should return 0 on success and a negative error code on
> + * failure.
> + */
> int (*fb_probe)(struct drm_fb_helper *helper,
> struct drm_fb_helper_surface_size *sizes);
Nit: s/drm/DRM/
> /**
> - * struct drm_fb_helper - helper to emulate fbdev on top of kms
> + * struct drm_fb_helper - main structure to emulate fbdev on top of kms
s/kms/KMS
> * @fb: Scanout framebuffer object
> * @dev: DRM device
There seems to be an extra space between the : and the description. That
was already there, but maybe worth a follow-up.
> * @crtc_count: number of possible CRTCs
> * @crtc_info: per-CRTC helper state (mode, x/y offset, etc)
> * @connector_count: number of connected connectors
> * @connector_info_alloc_count: size of connector_info
> + * @connector_info: array of per-connector information
> * @funcs: driver callbacks for fb helper
> * @fbdev: emulated fbdev device info struct
> * @pseudo_palette: fake palette of 16 colors
> - * @kernel_fb_list: list_head in kernel_fb_helper_list
> - * @delayed_hotplug: was there a hotplug while kms master active?
> + *
> + * This is the main structure used by the fbdev helpers. Drivers supporting
> + * fbdev emulation should embedded this into their overall driver structure.
> + * Drivers must also fill out a struct &drm_fb_helper_funcs with a few
> + * operations.
> */
> struct drm_fb_helper {
> struct drm_framebuffer *fb;
> @@ -129,10 +184,21 @@ struct drm_fb_helper {
> const struct drm_fb_helper_funcs *funcs;
> struct fb_info *fbdev;
> u32 pseudo_palette[17];
> +
> + /**
> + * @kernel_fb_list:
> + *
> + * Entry on the global kernel_fb_helper_list, used for kgdb entry/exit.
> + */
> struct list_head kernel_fb_list;
>
> - /* we got a hotplug but fbdev wasn't running the console
> -delay until next set_par *
Re: [Intel-gfx] [PATCH 03/28] drm: Reorganize helper vtables and their docs
On Fri, Dec 04, 2015 at 09:45:44AM +0100, Daniel Vetter wrote: [...] > diff --git a/drivers/gpu/drm/drm_crtc_helper.c > b/drivers/gpu/drm/drm_crtc_helper.c > index 10d0989db273..077e48d3cac2 100644 > --- a/drivers/gpu/drm/drm_crtc_helper.c > +++ b/drivers/gpu/drm/drm_crtc_helper.c > @@ -62,6 +62,11 @@ > * converting to the plane helpers). New drivers must not use these functions > * but need to implement the atomic interface instead, potentially using the > * atomic helpers for that. > + * > + * The these legacy modeset helpers use the same function table structures as s/The these/The/ > diff --git a/include/drm/drm_modeset_helper_vtables.h > b/include/drm/drm_modeset_helper_vtables.h > new file mode 100644 > index ..35c5a1c4e7ba > --- /dev/null > +++ b/include/drm/drm_modeset_helper_vtables.h > @@ -0,0 +1,252 @@ > +/* > + * Copyright © 2015 Intel Corporation > + * Daniel Vetter Perhaps inherit the copyright statements from the includes that you extracted these from? > +/** > + * DOC: overview > + * > + * The DRM mode setting helper functions are common code for drivers to use > if > + * they wish. Drivers are not forced to use this code in their > + * implementations but it would be useful if the code they do use at least > + * provides a consistent interface and operation to userspace. Therefore it > is > + * highly recommended to use the provided helpers as much as possible. > + * > + * Because there is only one pointer per modeset object to hold a vfunc table > + * for helper libraries they are by necessity shared among the different > + * helpers. > + * > + * To make this clear all the helper vtables are pulled together in this > location here. Perhaps drop the "here" at the end of that sentence. Also maybe wrap the last line because it stands out as much longer than the above. > + */ > + > +enum mode_set_atomic; > + > +/** > + * struct drm_crtc_helper_funcs - helper operations for CRTCs > + * @dpms: set power state > + * @prepare: prepare the CRTC, called before @mode_set > + * @commit: commit changes to CRTC, called after @mode_set > + * @mode_fixup: try to fixup proposed mode for this CRTC > + * @mode_set: set this mode > + * @mode_set_nofb: set mode only (no scanout buffer attached) > + * @mode_set_base: update the scanout buffer > + * @mode_set_base_atomic: non-blocking mode set (used for kgdb support) > + * @load_lut: load color palette > + * @disable: disable CRTC when no longer in use > + * @enable: enable CRTC > + * @atomic_check: check for validity of an atomic state > + * @atomic_begin: begin atomic update > + * @atomic_flush: flush atomic update > + * > + * The helper operations are called by the mid-layer CRTC helper. > + * > + * Note that with atomic helpers @dpms, @prepare and @commit hooks are > + * deprecated. Used @enable and @disable instead exclusively. I /think/ it would be more correct to say: "Use @enable and @disable exclusively instead." > + * > + * With legacy crtc helpers there's a big semantic difference between > @disable s/crtc/CRTC/, there's a couple more places where the casing is inconsistent, I'll refrain from pointing them out explicitly since your editor will be much better at finding them. > +/** > + * struct drm_encoder_helper_funcs - helper operations for encoders > + * @dpms: set power state > + * @save: save connector state > + * @restore: restore connector state > + * @mode_fixup: try to fixup proposed mode for this connector > + * @prepare: part of the disable sequence, called before the CRTC modeset > + * @commit: called after the CRTC modeset > + * @mode_set: set this mode, optional for atomic helpers > + * @get_crtc: return CRTC that the encoder is currently attached to > + * @detect: connection status detection > + * @disable: disable encoder when not in use (overrides DPMS off) > + * @enable: enable encoder > + * @atomic_check: check for validity of an atomic update > + * > + * The helper operations are called by the mid-layer CRTC helper. > + * > + * Note that with atomic helpers @dpms, @prepare and @commit hooks are > + * deprecated. Used @enable and @disable instead exclusively. Same comment as for the CRTC helper functions. Thierry signature.asc Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 02/28] drm: Move LEAVE/ENTER_ATOMIC_MODESET to fbdev helpers
On Fri, Dec 04, 2015 at 09:45:43AM +0100, Daniel Vetter wrote: > This is only used for kgdb (and previously panic) handlers in > the fbdev emulation, so belongs there. > > Note that this means we'll leave behind a forward declaration, but > once all the helper vtables are consolidated (in the next patch) that > will make more sense. > > Signed-off-by: Daniel Vetter > --- > drivers/gpu/drm/nouveau/nv50_display.c | 1 + > include/drm/drm_crtc_helper.h | 5 + > include/drm/drm_fb_helper.h| 5 + > 3 files changed, 7 insertions(+), 4 deletions(-) Reviewed-by: Thierry Reding signature.asc Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 04/28] drm: Make helper vtable pointers type-safe
On Fri, Dec 04, 2015 at 09:45:45AM +0100, Daniel Vetter wrote: > Originally the idea behind void* was to allow different sets of > helpers. But now we have that (with probe, plane, crtc and atomic > helpers) and we still just use the same set of vtables. That's the > only way to make the individual helpers modular and allow drivers to > pick&choose and transition between them. So this flexibility isn't > really needed. Also we have lots of non-vtable data meanwhile in core > structures too, this is not the first one at all. > > Given that the void * is only trouble since gcc can't warn you if you > mix them up. Let's fix that and make them typesafe. > > Signed-off-by: Daniel Vetter > --- > include/drm/drm_crtc.h | 13 + > 1 file changed, 9 insertions(+), 4 deletions(-) Very good: Reviewed-by: Thierry Reding signature.asc Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 05/28] drm: Merge helper docbook into kerneldoc comments
On Fri, Dec 04, 2015 at 09:45:46AM +0100, Daniel Vetter wrote:
[...]
> diff --git a/drivers/gpu/drm/drm_crtc_helper.c
> b/drivers/gpu/drm/drm_crtc_helper.c
> index 077e48d3cac2..c4fbcf8e6664 100644
> --- a/drivers/gpu/drm/drm_crtc_helper.c
> +++ b/drivers/gpu/drm/drm_crtc_helper.c
> @@ -51,6 +51,11 @@
> * the same callbacks which drivers can use to e.g. restore the modeset
> * configuration on resume with drm_helper_resume_force_mode().
> *
> + * Note that this helper library doesn't track the current power state of
> CRTCs
> + * and encoders. It can callbacks like ->dpms() even though the hardware is
Perhaps "It can {call,run,execute} callbacks like ->dpms() ..."
> @@ -450,11 +455,33 @@ drm_crtc_helper_disable(struct drm_crtc *crtc)
> * drm_crtc_helper_set_config - set a new config from userspace
> * @set: mode set configuration
> *
> - * Setup a new configuration, provided by the upper layers (either an ioctl
> call
> - * from userspace or internally e.g. from the fbdev support code) in @set,
> and
> - * enable it. This is the main helper functions for drivers that implement
> - * kernel mode setting with the crtc helper functions and the assorted
> - * ->prepare(), ->modeset() and ->commit() helper callbacks.
> + * The drm_crtc_helper_set_config() helper function implements the set_config
> + * callback of struct &drm_crtc_funcs for drivers using the legacy CRTC
> helpers.
> + *
> + * It first tries to locate the best encoder for each connector by calling
> the
> + * connector best_encoder (struct &drm_connector_helper_funcs) helper
> operation.
Perhaps "->best_encoder()"? Or is the above required to get formatting
right with the new hypertext/markdown additions?
> + *
> + * After locating the appropriate encoders, the helper function will call the
> + * mode_fixup encoder and CRTC helper operations to adjust the requested
> mode,
Again, "->mode_fixup()"?
> + * or reject it completely in which case an error will be returned to the
> + * application. If the new configuration after mode adjustment is identical
> to
> + * the current configuration the helper function will return without
> performing
> + * any other operation.
> + *
> + * If the adjusted mode is identical to the current mode but changes to the
> + * frame buffer need to be applied, the drm_crtc_helper_set_config function
> will
Parentheses after "drm_crtc_helper_set_config" to get it marked up as
function?
> + * call the CRTC mode_set_base (struct &drm_crtc_helper_funcs) helper
> operation.
"->mode_set_base()"?
> + *
> + * If the adjusted mode differs from the current mode, or if the
> mode_set_base
"->mode_set_base()"?
> + * helper operation is not provided, the helper function performs a full mode
> + * set sequence by calling the prepare, mode_set and commit CRTC and encoder
"->prepare(), ->mode_set() and ->commit()"?
> @@ -763,10 +790,18 @@ static int drm_helper_choose_crtc_dpms(struct drm_crtc
> *crtc)
> * @connector: affected connector
> * @mode: DPMS mode
> *
> + * The drm_helper_connector_dpms() helper function implements the dpms
"->dpms()"?
> + * callback of struct &drm_connector_funcs for drivers using the legacy CRTC
> helpers.
> + *
> * This is the main helper function provided by the crtc helper framework for
s/crtc/CRTC/?
> * implementing the DPMS connector attribute. It computes the new desired
> DPMS
> * state for all encoders and crtcs in the output mesh and calls the ->dpms()
s/crtcs/CRTCs/?
> - * callback provided by the driver appropriately.
> + * callbacks provided by the driver in struct &drm_crtc_helper_funcs and
> struct
> + * &drm_encoder_helper_funcs appropriately.
Perhaps s/appropriately./, respectively./?
> diff --git a/drivers/gpu/drm/drm_probe_helper.c
> b/drivers/gpu/drm/drm_probe_helper.c
> index dcd7c0289e03..62889249cbf8 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -272,15 +272,29 @@ prune:
> * @maxX: max width for modes
> * @maxY: max height for modes
> *
> - * Based on the helper callbacks implemented by @connector try to detect all
> - * valid modes. Modes will first be added to the connector's probed_modes
> list,
> - * then culled (based on validity and the @maxX, @maxY parameters) and put
> into
> - * the normal modes list.
> + * Based on the helper callbacks implemented by @connector in struct
> + * &drm_connector_helper_funcs try to detect all valid modes. Modes will
> first
> + * be added to the connector's probed_modes list, then culled (based on
> validity
> + * and the @maxX, @maxY parameters) and put into the normal modes list.
> *
> * Intended to be use as a generic implementation of the ->fill_modes()
s/be use/be used/
> * @connector vfunc for drivers that use the crtc helpers for output mode
s/crtc/CRTC/
> * filtering and detection.
> *
> + * If the helper operation returns no mode, and if the connector status is
> + * connector_status_connected, sta
