Re: [Intel-gfx] [PATCH v3 1/3] pwm: make it possible to apply pwm changes in atomic context

[Daniel Thompson]

On Sun, Oct 22, 2023 at 11:46:22AM +0100, Sean Young wrote:
> Hi Hans,
>
> On Sat, Oct 21, 2023 at 11:08:22AM +0200, Hans de Goede wrote:
> > On 10/19/23 12:51, Uwe Kleine-König wrote:
> > > On Wed, Oct 18, 2023 at 03:57:48PM +0200, Hans de Goede wrote:
> > >> On 10/17/23 11:17, Sean Young wrote:
> > > I think it's very subjective if you consider this
> > > churn or not.
> >
> > I consider it churn because I don't think adding a postfix
> > for what is the default/expected behavior is a good idea
> > (with GPIOs not sleeping is the expected behavior).
> >
> > I agree that this is very subjective and very much goes
> > into the territory of bikeshedding. So please consider
> > the above my 2 cents on this and lets leave it at that.
>
> You have a valid point. Let's focus on having descriptive function names.

For a couple of days I've been trying to resist the bikeshedding (esp.
given the changes to backlight are tiny) so I'll try to keep it as
brief as I can:

1. I dislike the do_it() and do_it_cansleep() pairing. It is
   difficult to detect when a client driver calls do_it() by mistake.
   In fact a latent bug of this nature can only be detected by runtime
   testing with the small number of PWMs that do not support
   configuration from an atomic context.

   In contrast do_it() and do_it_atomic()[*] means that although
   incorrectly calling do_it() from an atomic context can be pretty
   catastrophic it is also trivially detected (with any PWM driver)
   simply by running with CONFIG_DEBUG_ATOMIC_SLEEP.

   No objections (beyond churn) to fully spelt out pairings such as
   do_it_cansleep() and do_it_atomic()[*]!


2. If there is an API rename can we make sure the patch contains no
   other changes (e.g. don't introduce any new API in the same patch).
   Seperating renames makes the patches easier to review!
   It makes each one smaller and easier to review!


Daniel.


[*] or do_it_nosleep()... etc.

Re: [Intel-gfx] [Kgdb-bugreport] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr

[Daniel Thompson]

On Thu, Mar 03, 2022 at 03:26:57PM +0800, Xiaomeng Tong wrote:
> On Thu, 3 Mar 2022 04:58:23 +, David Laight wrote:
> > on 3 Mar 2022 10:27:29 +0800, Xiaomeng Tong wrote:
> > > The problem is the mis-use of iterator outside the loop on exit, and
> > > the iterator will be the HEAD's container_of pointer which pointers
> > > to a type-confused struct. Sidenote: The *mis-use* here refers to
> > > mistakely access to other members of the struct, instead of the
> > > list_head member which acutally is the valid HEAD.
> >
> > The problem is that the HEAD's container_of pointer should never
> > be calculated at all.
> > This is what is fundamentally broken about the current definition.
> 
> Yes, the rule is "the HEAD's container_of pointer should never be
> calculated at all outside the loop", but how do you make sure everyone
> follows this rule?

Your formulation of the rule is correct: never run container_of() on HEAD
pointer.

However the rule that is introduced by list_for_each_entry_inside() is
*not* this rule. The rule it introduces is: never access the iterator
variable outside the loop.

Making the iterator NULL on loop exit does follow the rule you proposed
but using a different technique: do not allow HEAD to be stored in the
iterator variable after loop exit. This also makes it impossible to run
container_of() on the HEAD pointer.


> Everyone makes mistakes, but we can eliminate them all from the beginning
> with the help of compiler which can catch such use-after-loop things.

Indeed but if we introduce new interfaces then we don't have to worry
about existing usages and silent regressions. Code will have been
written knowing the loop can exit with the iterator set to NULL.

Sure it is still possible for programmers to make mistakes and
dereference the NULL pointer but C programmers are well training w.r.t.
NULL pointer checking so such mistakes are much less likely than with
the current list_for_each_entry() macro. This risk must be offset
against the way a NULLify approach can lead to more elegant code when we
are doing a list search.


Daniel.

Re: [Intel-gfx] [PATCH v13 00/11] Convert PWM period and duty cycle to u64

[Daniel Thompson]

On Wed, Apr 22, 2020 at 04:37:55PM -0700, Guru Das Srinagesh wrote:
> On Wed, Apr 22, 2020 at 09:49:34AM +0100, Daniel Thompson wrote:
> > On Tue, Apr 21, 2020 at 07:57:12PM -0700, Guru Das Srinagesh wrote:
> > > [REQUEST]
> > > 
> > > Would it be possible for the patches that have already received 
> > > Acked-by's in
> > > this series to be accepted and applied to the tree? I lost an Acked-by (in
> > > intel-panel.c) because it had a merge conflict with a new change that 
> > > came in
> > > after I rebased to tip. I wasn't sure earlier about accepting single 
> > > patches as
> > > opposed to the entire series en masse, but this event has got me thinking.
> > 
> > Has there been a positive maintainer review of patch 11 at any point in
> > the thread (most of the people you are asking to commit patches have
> > not been able to see the discussion about the actual feature these
> > patches are preparing for)?
> 
> Yes. Uwe had this to say [1] about a previous patchset (v5) of patch 11
> which is essentially unchanged in this patchset save the dropping of the
> pwm_capture change.
> 
> [1] https://www.spinics.net/lists/linux-pwm/msg11536.html

Excellent. Thanks!


Daniel.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v13 00/11] Convert PWM period and duty cycle to u64

[Daniel Thompson]

On Tue, Apr 21, 2020 at 07:57:12PM -0700, Guru Das Srinagesh wrote:
> [REQUEST]
> 
> Would it be possible for the patches that have already received Acked-by's in
> this series to be accepted and applied to the tree? I lost an Acked-by (in
> intel-panel.c) because it had a merge conflict with a new change that came in
> after I rebased to tip. I wasn't sure earlier about accepting single patches 
> as
> opposed to the entire series en masse, but this event has got me thinking.

Has there been a positive maintainer review of patch 11 at any point in
the thread (most of the people you are asking to commit patches have
not been able to see the discussion about the actual feature these
patches are preparing for)?


Daniel.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v10 00/12] Convert PWM period and duty cycle to u64

[Daniel Thompson]

On Mon, Mar 30, 2020 at 02:00:12PM -0700, Guru Das Srinagesh wrote:
> On Mon, Mar 30, 2020 at 09:26:36PM +0100, Daniel Thompson wrote:
> > On Mon, Mar 30, 2020 at 12:15:07PM -0700, Guru Das Srinagesh wrote:
> > > On Sat, Mar 21, 2020 at 02:47:03PM +0300, Dan Carpenter wrote:
> > > > This is a giant CC list.
> > > 
> > > Yes, this is because I received feedback [1] on an earlier patchset
> > > directing me to add the reviewers of patches to the cover letter as
> > > well so that they get some context for the patch.
> > > ...
> > > [1] https://www.spinics.net/lists/linux-pwm/msg11735.html
> > 
> > Strictly speaking I only asked for backlight maintainers to be Cc:ed.
> > I was fairly careful to be specific since I'm aware there are a variety
> > of differing habits when putting together the Cc: list for covering
> > letters.
> > 
> > With the original patch header the purpose of the patch I was Cc:ed on
> > was impossible to determine without the covering letter.
> 
> I suspect this might be the case for all the other reviewers as well -
> that they also would appreciate context for the specific patch they are
> being added to review.
> 
> I wasn't entirely sure what the convention was, so I applied your
> suggestion to all the files. How do you suggest I handle this in my next
> patchset? I fully agree that such a large CC list does look really
> ungainly.

IHMO there should not be a mechanical convention. Instead your goal
needs to be how to make it as easy as possible to review your patches.

Think about it this way: Each person in the To: of a patch (and maybe
also Cc: depending on how you construct things) is a person you are
asking to review and comment on the patch. If that person will find it
easier to review the patch if they are included in the cover letter then
either they should be included or you should improve the patch
description of the patch itself (sometimes both).

Either way it is about optimizing the patchset for readability. More
people read them than write them.


Daniel.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v10 00/12] Convert PWM period and duty cycle to u64

[Daniel Thompson]

On Mon, Mar 30, 2020 at 12:15:07PM -0700, Guru Das Srinagesh wrote:
> On Sat, Mar 21, 2020 at 02:47:03PM +0300, Dan Carpenter wrote:
> > This is a giant CC list.
> 
> Yes, this is because I received feedback [1] on an earlier patchset
> directing me to add the reviewers of patches to the cover letter as
> well so that they get some context for the patch.
> ...
> [1] https://www.spinics.net/lists/linux-pwm/msg11735.html

Strictly speaking I only asked for backlight maintainers to be Cc:ed.
I was fairly careful to be specific since I'm aware there are a variety
of differing habits when putting together the Cc: list for covering
letters.

With the original patch header the purpose of the patch I was Cc:ed on
was impossible to determine without the covering letter.


Daniel.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 00/33] fbcon notifier begone v3!

[Daniel Thompson]

On Fri, Jun 07, 2019 at 12:07:55PM +0200, Bartlomiej Zolnierkiewicz wrote:
> 
> On 6/6/19 9:38 AM, Daniel Vetter wrote:
> > Hi Bart,
> 
> Hi Daniel,
> 
> > On Tue, May 28, 2019 at 11:02:31AM +0200, Daniel Vetter wrote:
> >> Hi all,
> >>
> >> I think we're slowly getting there. Previous cover letters with more
> >> context:
> >>
> >> https://lists.freedesktop.org/archives/dri-devel/2019-May/218362.html
> >>
> >> tldr; I have a multi-year plan to improve fbcon locking, because the
> >> current thing is a bit a mess.
> >>
> >> Cover letter of this version, where I detail a bit more the details
> >> fixed in this one here:
> >>
> >> https://lists.freedesktop.org/archives/dri-devel/2019-May/218984.html
> >>
> >> Note that the locking plan in this one is already outdated, I overlooked a
> >> few fun issues around any printk() going back to console_lock.
> >>
> >> I think remaining bits:
> >>
> >> - Ack from Daniel Thompson for the backlight bits, he wanted to check the
> >>   big picture.
> > 
> > I think Daniel is still on vacation until next week or so.

Thanks for spotting that. As it happens the e-mail asking for extra detail
was just about the last thing I sent before going on holiday (exactly to
try and avoid round trips this wee ;-) ).


> > 
> >> - Hash out actual merge plan.
> > 
> > I'd like to stuff this into drm.git somehow, I guess topic branch works
> > too.
> 
> I would like to have topic branch for this patchset.

From a backlight perspective its Lee Jones who hoovers up the patches
and worries about hiding merge conflicts from Linus.

I'll let him follow up if needed but I suspect he'd like an immutable
branch to work from also.


Daniel.


> 
> > Long term I think we need to reconsider how we handle fbdev, at least the
> > core/fbcon pieces. Since a few years all the work in that area has been
> > motivated by drm, and pushed by drm contributors. Having that maintained
> > in a separate tree that doesn't regularly integrate imo doesn't make much
> > sense, and we ended up merging almost everything through some drm tree.
> > That one time we didn't (for some panel rotation stuff) it resulted in
> > some good suprises.
> > 
> > I think best solution is if we put the core and fbcon bits into drm-misc,
> > as group maintained infrastructure piece. All the other gfx infra pieces
> > are maintained in there already too. You'd obviously get commit rights.
> > I think that would include
> > - drivers/video/fbdev
> > - drivers/video/*c
> > - drivers/video/console
> 
> Sounds fine to me.
> 
> > I don't really care about what happens with the actual fbdev drivers
> > (aside from the drm one in drm_fb_helper.c, but that's already maintained
> > as part of drm). I guess we could also put those into drm-misc, or as a
> > separate tree, depending what you want.
> > 
> > Thoughts?
> 
> I would like to handle fbdev changes for v5.3 merge window using fbdev
> tree but after that everything (including changes to fbdev drivers) can go
> through drm-misc tree.
> 
> Best regards,
> --
> Bartlomiej Zolnierkiewicz
> Samsung R Institute Poland
> Samsung Electronics
> 
> > Cheers, Daniel
> > 
> > 
> >>
> >> I'm also cc'ing the entire pile to a lot more people on request.
> >>
> >> Thanks, Daniel
> >>
> >> Daniel Vetter (33):
> >>   dummycon: Sprinkle locking checks
> >>   fbdev: locking check for fb_set_suspend
> >>   vt: might_sleep() annotation for do_blank_screen
> >>   vt: More locking checks
> >>   fbdev/sa1100fb: Remove dead code
> >>   fbdev/cyber2000: Remove struct display
> >>   fbdev/aty128fb: Remove dead code
> >>   fbcon: s/struct display/struct fbcon_display/
> >>   fbcon: Remove fbcon_has_exited
> >>   fbcon: call fbcon_fb_(un)registered directly
> >>   fbdev/sh_mobile: remove sh_mobile_lcdc_display_notify
> >>   fbdev/omap: sysfs files can't disappear before the device is gone
> >>   fbdev: sysfs files can't disappear before the device is gone
> >>   staging/olpc: lock_fb_info can't fail
> >>   fbdev/atyfb: lock_fb_info can't fail
> >>   fbdev: lock_fb_info cannot fail
> >>   fbcon: call fbcon_fb_bind directly
> >>   fbdev: make unregister/unlink functions not fail
> >>   fbdev: unify unlink_framebuffer paths
> >>   fbdev/sh_mob: Remove fb notifier callback
> >>   fbdev: directly call fbcon_suspended/re

Re: [Intel-gfx] [PATCH 33/33] backlight: simplify lcd notifier

[Daniel Thompson]

On Tue, May 28, 2019 at 11:03:04AM +0200, Daniel Vetter wrote:
> With all the work I've done on replacing fb notifier calls with direct
> calls into fbcon the backlight/lcd notifier is the only user left.
> 
> It will only receive events now that it cares about, hence we can
> remove this check.
> 
> Signed-off-by: Daniel Vetter 
> Reviewed-by: Sam Ravnborg 
> Reviewed-by: Maarten Lankhorst 
> Cc: Lee Jones 
> Cc: Daniel Thompson 
> Cc: Jingoo Han 

Acked-by: Daniel Thompson 


> ---
>  drivers/video/backlight/lcd.c | 11 ---
>  1 file changed, 11 deletions(-)
> 
> diff --git a/drivers/video/backlight/lcd.c b/drivers/video/backlight/lcd.c
> index ecdda06989d0..d6b653aa4ee9 100644
> --- a/drivers/video/backlight/lcd.c
> +++ b/drivers/video/backlight/lcd.c
> @@ -30,17 +30,6 @@ static int fb_notifier_callback(struct notifier_block 
> *self,
>   struct lcd_device *ld;
>   struct fb_event *evdata = data;
>  
> - /* If we aren't interested in this event, skip it immediately ... */
> - switch (event) {
> - case FB_EVENT_BLANK:
> - case FB_EVENT_MODE_CHANGE:
> - case FB_EARLY_EVENT_BLANK:
> - case FB_R_EARLY_EVENT_BLANK:
> - break;
> - default:
> - return 0;
> - }
> -
>   ld = container_of(self, struct lcd_device, fb_notif);
>   if (!ld->ops)
>   return 0;
> -- 
> 2.20.1
> 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 28/33] fbcon: replace FB_EVENT_MODE_CHANGE/_ALL with direct calls

[Daniel Thompson]

On Tue, May 28, 2019 at 11:02:59AM +0200, Daniel Vetter wrote:
> Create a new wrapper function for this, feels like there's some
> refactoring room here between the two modes.
> 
> v2: backlight notifier is also interested in the mode change event,
> it calls lcd->set_mode, of which there are 3 implementations. Thanks
> to Maarten for spotting this. So we keep that. We can ditch the 
> differentiation
> between mode change and all mode changes (because backlight notifier
> doesn't care), and we can drop the FBINFO_MISC_USEREVENT stuff too,
> because that's just to prevent recursion between fbmem.c and fbcon.c.
> 
> While at it flatten the control flow a bit.
> 
> v3: Need to add a static inline to the dummy function.
> 
> v4: Add missing #include  to sh_mob (Sam).
> 
> Cc: Sam Ravnborg 
> Signed-off-by: Daniel Vetter 
> Reviewed-by: Sam Ravnborg 
> Reviewed-by: Maarten Lankhorst 
> Cc: Maarten Lankhorst 
> Cc: Lee Jones 
> Cc: Daniel Thompson 
> Cc: Jingoo Han 
> Cc: Bartlomiej Zolnierkiewicz 
> Cc: Daniel Vetter 
> Cc: Hans de Goede 
> Cc: Yisheng Xie 
> Cc: "Michał Mirosław" 
> Cc: Peter Rosin 
> Cc: Mikulas Patocka 
> Cc: linux-fb...@vger.kernel.org

Acked-by: Daniel Thompson 


> ---
>  drivers/video/backlight/lcd.c  |  1 -
>  drivers/video/fbdev/core/fbcon.c   | 15 +--
>  drivers/video/fbdev/core/fbmem.c   | 21 ++---
>  drivers/video/fbdev/sh_mobile_lcdcfb.c | 12 ++--
>  include/linux/fb.h |  2 --
>  include/linux/fbcon.h  |  2 ++
>  6 files changed, 23 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/video/backlight/lcd.c b/drivers/video/backlight/lcd.c
> index 151b18776add..ecdda06989d0 100644
> --- a/drivers/video/backlight/lcd.c
> +++ b/drivers/video/backlight/lcd.c
> @@ -34,7 +34,6 @@ static int fb_notifier_callback(struct notifier_block *self,
>   switch (event) {
>   case FB_EVENT_BLANK:
>   case FB_EVENT_MODE_CHANGE:
> - case FB_EVENT_MODE_CHANGE_ALL:
>   case FB_EARLY_EVENT_BLANK:
>   case FB_R_EARLY_EVENT_BLANK:
>   break;
> diff --git a/drivers/video/fbdev/core/fbcon.c 
> b/drivers/video/fbdev/core/fbcon.c
> index b5ee89f16d6c..e98551f96138 100644
> --- a/drivers/video/fbdev/core/fbcon.c
> +++ b/drivers/video/fbdev/core/fbcon.c
> @@ -3009,6 +3009,15 @@ static void fbcon_set_all_vcs(struct fb_info *info)
>   fbcon_modechanged(info);
>  }
>  
> +
> +void fbcon_update_vcs(struct fb_info *info, bool all)
> +{
> + if (all)
> + fbcon_set_all_vcs(info);
> + else
> + fbcon_modechanged(info);
> +}
> +
>  int fbcon_mode_deleted(struct fb_info *info,
>  struct fb_videomode *mode)
>  {
> @@ -3318,12 +3327,6 @@ static int fbcon_event_notify(struct notifier_block 
> *self,
>   int idx, ret = 0;
>  
>   switch(action) {
> - case FB_EVENT_MODE_CHANGE:
> - fbcon_modechanged(info);
> - break;
> - case FB_EVENT_MODE_CHANGE_ALL:
> - fbcon_set_all_vcs(info);
> - break;
>   case FB_EVENT_SET_CONSOLE_MAP:
>   /* called with console lock held */
>   con2fb = event->data;
> diff --git a/drivers/video/fbdev/core/fbmem.c 
> b/drivers/video/fbdev/core/fbmem.c
> index 96805fe85332..dd1a708df1a7 100644
> --- a/drivers/video/fbdev/core/fbmem.c
> +++ b/drivers/video/fbdev/core/fbmem.c
> @@ -957,6 +957,7 @@ fb_set_var(struct fb_info *info, struct fb_var_screeninfo 
> *var)
>   u32 activate;
>   struct fb_var_screeninfo old_var;
>   struct fb_videomode mode;
> + struct fb_event event;
>  
>   if (var->activate & FB_ACTIVATE_INV_MODE) {
>   struct fb_videomode mode1, mode2;
> @@ -1039,19 +1040,17 @@ fb_set_var(struct fb_info *info, struct 
> fb_var_screeninfo *var)
>   !list_empty(>modelist))
>   ret = fb_add_videomode(, >modelist);
>  
> - if (!ret && (flags & FBINFO_MISC_USEREVENT)) {
> - struct fb_event event;
> - int evnt = (activate & FB_ACTIVATE_ALL) ?
> - FB_EVENT_MODE_CHANGE_ALL :
> - FB_EVENT_MODE_CHANGE;
> + if (ret)
> + return ret;
>  
> - info->flags &= ~FBINFO_MISC_USEREVENT;
> - event.info = info;
> - event.data = 
> - fb_notifier_call_chain(evnt, );
> - }
> + event.info = info;
> + event.data = 
> + fb_notifier_call_chain(FB_EVENT_MODE_CHANGE, );
>  
> - return ret;
> + if (flags & FBINFO

Re: [Intel-gfx] [PATCH 24/33] Revert "backlight/fbcon: Add FB_EVENT_CONBLANK"

[Daniel Thompson]

On Tue, May 28, 2019 at 11:02:55AM +0200, Daniel Vetter wrote:
> This reverts commit 994efacdf9a087b52f71e620b58dfa526b0cf928.
> 
> The justification is that if hw blanking fails (i.e. fbops->fb_blank)
> fails, then we still want to shut down the backlight. Which is exactly
> _not_ what fb_blank() does and so rather inconsistent if we end up
> with different behaviour between fbcon and direct fbdev usage. Given
> that the entire notifier maze is getting in the way anyway I figured
> it's simplest to revert this not well justified commit.
> 
> v2: Add static inline to the dummy version.
> 
> Cc: Richard Purdie 
> Signed-off-by: Daniel Vetter 
> Reviewed-by: Sam Ravnborg 
> Reviewed-by: Maarten Lankhorst 
> Cc: Lee Jones 
> Cc: Daniel Thompson 
> Cc: Jingoo Han 
> Cc: Bartlomiej Zolnierkiewicz 
> Cc: Daniel Vetter 
> Cc: Hans de Goede 
> Cc: Yisheng Xie 
> Cc: linux-fb...@vger.kernel.org

This was the main patch where I wanted the bigger picture ;-) and TBH
I'm still in two minds here. I don't personally view fbcon as
inconsistent, more that, as an in-kernel service it might have to do
more that something more complicated than freak out and let userspace
decide what to do next.

However... since I'm struggling to make up my mind, I can't think of
many products that would ship reliant exclusively on fbcon *and* this
patch is more about fbcon than backlight then I figure that, from a
backlight perspective:

Acked-by: Daniel Thompson 


Daniel.


> ---
>  drivers/video/backlight/backlight.c |  2 +-
>  drivers/video/fbdev/core/fbcon.c| 14 +-
>  drivers/video/fbdev/core/fbmem.c|  1 +
>  include/linux/fb.h  |  4 +---
>  include/linux/fbcon.h   |  2 ++
>  5 files changed, 6 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/video/backlight/backlight.c 
> b/drivers/video/backlight/backlight.c
> index 1ef8b6fd62ac..5dc07106a59e 100644
> --- a/drivers/video/backlight/backlight.c
> +++ b/drivers/video/backlight/backlight.c
> @@ -47,7 +47,7 @@ static int fb_notifier_callback(struct notifier_block *self,
>   int fb_blank = 0;
>  
>   /* If we aren't interested in this event, skip it immediately ... */
> - if (event != FB_EVENT_BLANK && event != FB_EVENT_CONBLANK)
> + if (event != FB_EVENT_BLANK)
>   return 0;
>  
>   bd = container_of(self, struct backlight_device, fb_notif);
> diff --git a/drivers/video/fbdev/core/fbcon.c 
> b/drivers/video/fbdev/core/fbcon.c
> index ef69bd4ad343..a4617067ff24 100644
> --- a/drivers/video/fbdev/core/fbcon.c
> +++ b/drivers/video/fbdev/core/fbcon.c
> @@ -2350,8 +2350,6 @@ static int fbcon_switch(struct vc_data *vc)
>  static void fbcon_generic_blank(struct vc_data *vc, struct fb_info *info,
>   int blank)
>  {
> - struct fb_event event;
> -
>   if (blank) {
>   unsigned short charmask = vc->vc_hi_font_mask ?
>   0x1ff : 0xff;
> @@ -2362,13 +2360,6 @@ static void fbcon_generic_blank(struct vc_data *vc, 
> struct fb_info *info,
>   fbcon_clear(vc, 0, 0, vc->vc_rows, vc->vc_cols);
>   vc->vc_video_erase_char = oldc;
>   }
> -
> -
> - lock_fb_info(info);
> - event.info = info;
> - event.data = 
> - fb_notifier_call_chain(FB_EVENT_CONBLANK, );
> - unlock_fb_info(info);
>  }
>  
>  static int fbcon_blank(struct vc_data *vc, int blank, int mode_switch)
> @@ -3240,7 +3231,7 @@ int fbcon_fb_registered(struct fb_info *info)
>   return ret;
>  }
>  
> -static void fbcon_fb_blanked(struct fb_info *info, int blank)
> +void fbcon_fb_blanked(struct fb_info *info, int blank)
>  {
>   struct fbcon_ops *ops = info->fbcon_par;
>   struct vc_data *vc;
> @@ -3344,9 +3335,6 @@ static int fbcon_event_notify(struct notifier_block 
> *self,
>   con2fb = event->data;
>   con2fb->framebuffer = con2fb_map[con2fb->console - 1];
>   break;
> - case FB_EVENT_BLANK:
> - fbcon_fb_blanked(info, *(int *)event->data);
> - break;
>   case FB_EVENT_REMAP_ALL_CONSOLE:
>   idx = info->node;
>   fbcon_remap_all(idx);
> diff --git a/drivers/video/fbdev/core/fbmem.c 
> b/drivers/video/fbdev/core/fbmem.c
> index ddc0c16b8bbf..9366fbe99a58 100644
> --- a/drivers/video/fbdev/core/fbmem.c
> +++ b/drivers/video/fbdev/core/fbmem.c
> @@ -1068,6 +1068,7 @@ fb_blank(struct fb_info *info, int blank)
>   event.data = 
>  
>   early_ret = fb_notifier_call_chain(FB_EARLY_EVENT_BLANK, );
> + fbcon_fb_blanked(info, blank);
>  
>   if (info->fbops->fb_blank)
>

Re: [Intel-gfx] [PATCH 24/33] Revert "backlight/fbcon: Add FB_EVENT_CONBLANK"

[Daniel Thompson]

On Fri, May 24, 2019 at 10:53:45AM +0200, Daniel Vetter wrote:
> This reverts commit 994efacdf9a087b52f71e620b58dfa526b0cf928.
> 
> The justification is that if hw blanking fails (i.e. fbops->fb_blank)
> fails, then we still want to shut down the backlight. Which is exactly
> _not_ what fb_blank() does and so rather inconsistent if we end up
> with different behaviour between fbcon and direct fbdev usage. Given
> that the entire notifier maze is getting in the way anyway I figured
> it's simplest to revert this not well justified commit.
> 
> v2: Add static inline to the dummy version.
> 
> Cc: Richard Purdie 
> Signed-off-by: Daniel Vetter 
> Cc: Lee Jones 
> Cc: Daniel Thompson 
> Cc: Jingoo Han 
> Cc: Bartlomiej Zolnierkiewicz 
> Cc: Daniel Vetter 
> Cc: Hans de Goede 
> Cc: Yisheng Xie 
> Cc: linux-fb...@vger.kernel.org

Hi Daniel

When this goes round again could you add me to the covering letter?

I looked at all three of the patches and no objections on my side but
I'm reluctant to send out acks because I'm not sure I understood the
wider picture well enough.


Daniel.


> ---
>  drivers/video/backlight/backlight.c |  2 +-
>  drivers/video/fbdev/core/fbcon.c| 14 +-
>  drivers/video/fbdev/core/fbmem.c|  1 +
>  include/linux/fb.h  |  4 +---
>  include/linux/fbcon.h   |  2 ++
>  5 files changed, 6 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/video/backlight/backlight.c 
> b/drivers/video/backlight/backlight.c
> index deb824bef6e2..c55590ec0057 100644
> --- a/drivers/video/backlight/backlight.c
> +++ b/drivers/video/backlight/backlight.c
> @@ -46,7 +46,7 @@ static int fb_notifier_callback(struct notifier_block *self,
>   int fb_blank = 0;
>  
>   /* If we aren't interested in this event, skip it immediately ... */
> - if (event != FB_EVENT_BLANK && event != FB_EVENT_CONBLANK)
> + if (event != FB_EVENT_BLANK)
>   return 0;
>  
>   bd = container_of(self, struct backlight_device, fb_notif);
> diff --git a/drivers/video/fbdev/core/fbcon.c 
> b/drivers/video/fbdev/core/fbcon.c
> index 259cdd118475..d9f545f1a81b 100644
> --- a/drivers/video/fbdev/core/fbcon.c
> +++ b/drivers/video/fbdev/core/fbcon.c
> @@ -2350,8 +2350,6 @@ static int fbcon_switch(struct vc_data *vc)
>  static void fbcon_generic_blank(struct vc_data *vc, struct fb_info *info,
>   int blank)
>  {
> - struct fb_event event;
> -
>   if (blank) {
>   unsigned short charmask = vc->vc_hi_font_mask ?
>   0x1ff : 0xff;
> @@ -2362,13 +2360,6 @@ static void fbcon_generic_blank(struct vc_data *vc, 
> struct fb_info *info,
>   fbcon_clear(vc, 0, 0, vc->vc_rows, vc->vc_cols);
>   vc->vc_video_erase_char = oldc;
>   }
> -
> -
> - lock_fb_info(info);
> - event.info = info;
> - event.data = 
> - fb_notifier_call_chain(FB_EVENT_CONBLANK, );
> - unlock_fb_info(info);
>  }
>  
>  static int fbcon_blank(struct vc_data *vc, int blank, int mode_switch)
> @@ -3240,7 +3231,7 @@ int fbcon_fb_registered(struct fb_info *info)
>   return ret;
>  }
>  
> -static void fbcon_fb_blanked(struct fb_info *info, int blank)
> +void fbcon_fb_blanked(struct fb_info *info, int blank)
>  {
>   struct fbcon_ops *ops = info->fbcon_par;
>   struct vc_data *vc;
> @@ -3344,9 +3335,6 @@ static int fbcon_event_notify(struct notifier_block 
> *self,
>   con2fb = event->data;
>   con2fb->framebuffer = con2fb_map[con2fb->console - 1];
>   break;
> - case FB_EVENT_BLANK:
> - fbcon_fb_blanked(info, *(int *)event->data);
> - break;
>   case FB_EVENT_REMAP_ALL_CONSOLE:
>   idx = info->node;
>   fbcon_remap_all(idx);
> diff --git a/drivers/video/fbdev/core/fbmem.c 
> b/drivers/video/fbdev/core/fbmem.c
> index ddc0c16b8bbf..9366fbe99a58 100644
> --- a/drivers/video/fbdev/core/fbmem.c
> +++ b/drivers/video/fbdev/core/fbmem.c
> @@ -1068,6 +1068,7 @@ fb_blank(struct fb_info *info, int blank)
>   event.data = 
>  
>   early_ret = fb_notifier_call_chain(FB_EARLY_EVENT_BLANK, );
> + fbcon_fb_blanked(info, blank);
>  
>   if (info->fbops->fb_blank)
>   ret = info->fbops->fb_blank(blank, info);
> diff --git a/include/linux/fb.h b/include/linux/fb.h
> index 0d86aa31bf8d..1e66fac3124f 100644
> --- a/include/linux/fb.h
> +++ b/include/linux/fb.h
> @@ -137,12 +137,10 @@ struct fb_cursor_user {
>  #define FB_EVENT_GET_CONSOLE_MAP0x07
>  /*  CONSOLE-SPECIFIC: set console to framebuf

Re: [Intel-gfx] [PATCH v3 05/10] pwm: add PWM mode to pwm_config()

[Daniel Thompson]

On Tue, Feb 27, 2018 at 01:40:58PM +0200, Claudiu Beznea wrote:
> On 27.02.2018 12:54, Daniel Thompson wrote:
> > On Mon, Feb 26, 2018 at 04:24:15PM +0200, Claudiu Beznea wrote:
> >> On 26.02.2018 11:57, Jani Nikula wrote:
> >>> On Thu, 22 Feb 2018, Daniel Thompson <daniel.thomp...@linaro.org> wrote:
> >>>> On Thu, Feb 22, 2018 at 02:01:16PM +0200, Claudiu Beznea wrote:
> >>>>> Add PWM mode to pwm_config() function. The drivers which uses 
> >>>>> pwm_config()
> >>>>> were adapted to this change.
> >>>>>
> >>>>> Signed-off-by: Claudiu Beznea <claudiu.bez...@microchip.com>
> >>>>> ---
> >>>>>  arch/arm/mach-s3c24xx/mach-rx1950.c  | 11 +--
> >>>>>  drivers/bus/ts-nbus.c|  2 +-
> >>>>>  drivers/clk/clk-pwm.c|  3 ++-
> >>>>>  drivers/gpu/drm/i915/intel_panel.c   | 17 ++---
> >>>>>  drivers/hwmon/pwm-fan.c  |  2 +-
> >>>>>  drivers/input/misc/max77693-haptic.c |  2 +-
> >>>>>  drivers/input/misc/max8997_haptic.c  |  6 +-
> >>>>>  drivers/leds/leds-pwm.c  |  5 -
> >>>>>  drivers/media/rc/ir-rx51.c   |  5 -
> >>>>>  drivers/media/rc/pwm-ir-tx.c |  5 -
> >>>>>  drivers/video/backlight/lm3630a_bl.c |  4 +++-
> >>>>>  drivers/video/backlight/lp855x_bl.c  |  4 +++-
> >>>>>  drivers/video/backlight/lp8788_bl.c  |  5 -
> >>>>>  drivers/video/backlight/pwm_bl.c | 11 +--
> >>>>>  drivers/video/fbdev/ssd1307fb.c  |  3 ++-
> >>>>>  include/linux/pwm.h  |  6 --
> >>>>>  16 files changed, 70 insertions(+), 21 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/video/backlight/lm3630a_bl.c 
> >>>>> b/drivers/video/backlight/lm3630a_bl.c
> >>>>> index 2030a6b77a09..696fa25dafd2 100644
> >>>>> --- a/drivers/video/backlight/lm3630a_bl.c
> >>>>> +++ b/drivers/video/backlight/lm3630a_bl.c
> >>>>> @@ -165,8 +165,10 @@ static void lm3630a_pwm_ctrl(struct lm3630a_chip 
> >>>>> *pchip, int br, int br_max)
> >>>>>  {
> >>>>> unsigned int period = pchip->pdata->pwm_period;
> >>>>> unsigned int duty = br * period / br_max;
> >>>>> +   struct pwm_caps caps = { };
> >>>>>  
> >>>>> -   pwm_config(pchip->pwmd, duty, period);
> >>>>> +   pwm_get_caps(pchip->pwmd->chip, pchip->pwmd, );
> >>>>> +   pwm_config(pchip->pwmd, duty, period, BIT(ffs(caps.modes) - 1));
> >>>>
> >>>> Well... I admit I've only really looked at the patches that impact 
> >>>> backlight but dispersing this really odd looking bit twiddling 
> >>>> throughout the kernel doesn't strike me a great API design.
> >>>>
> >>>> IMHO callers should not be required to find the first set bit in
> >>>> some specially crafted set of capability bits simply to get sane 
> >>>> default behaviour.
> >>>
> >>> Agreed. IMHO the regular use case becomes rather tedious, ugly, and
> >>> error prone.
> >>
> >> Using simply PWM_MODE(NORMAL) instead of BIT(ffs(caps.modes) - 1) would be 
> >> OK
> >> from your side?
> >>
> >> Or, what about using a function like pwm_mode_first() to get the first 
> >> supported
> >> mode by PWM channel?
> >>
> >> Or, would you prefer to solve this inside pwm_config() function, let's 
> >> say, in
> >> case an invalid mode is passed as argument, to let pwm_config() to choose 
> >> the
> >> first available PWM mode for PWM channel passed as argument?
> > 
> > What is it that actually needs solving?
> > 
> > If a driver requests normal mode and the PWM driver cannot support it
> > why not just return an error an move on.
> Because, simply, I wasn't aware of what these PWM client drivers needs for.

I'm afraid you have confused me here.

Didn't you just *add* the whole concept of PWM caps with your patches?
How could any existing call site expect anything except normal mode.
Until now there has been no possiblity to request anything else.


> > Put another way, what is the use case for secretly adopting a mode the
> > caller didn't want? Under what circumstances is this a good thing?
> No one... But I wasn't aware of what the PWM clients needs for from their PWM
> controllers. At this moment having BIT(ffs(caps.modes)) instead of
> PWM_MODE(NORMAL) is mostly the same since all the driver that has not 
> explicitly
> registered PWM caps will use PWM normal mode.
> 
> I will use PWM_MODE(NORMAL) instead of this in all the cases if this is OK 
> from
> your side.
> 
> Thank you,
> Claudiu Beznea
> > 
> > 
> > Daniel.
> > 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v3 05/10] pwm: add PWM mode to pwm_config()

[Daniel Thompson]

On Mon, Feb 26, 2018 at 04:24:15PM +0200, Claudiu Beznea wrote:
> On 26.02.2018 11:57, Jani Nikula wrote:
> > On Thu, 22 Feb 2018, Daniel Thompson <daniel.thomp...@linaro.org> wrote:
> >> On Thu, Feb 22, 2018 at 02:01:16PM +0200, Claudiu Beznea wrote:
> >>> Add PWM mode to pwm_config() function. The drivers which uses pwm_config()
> >>> were adapted to this change.
> >>>
> >>> Signed-off-by: Claudiu Beznea <claudiu.bez...@microchip.com>
> >>> ---
> >>>  arch/arm/mach-s3c24xx/mach-rx1950.c  | 11 +--
> >>>  drivers/bus/ts-nbus.c|  2 +-
> >>>  drivers/clk/clk-pwm.c|  3 ++-
> >>>  drivers/gpu/drm/i915/intel_panel.c   | 17 ++---
> >>>  drivers/hwmon/pwm-fan.c  |  2 +-
> >>>  drivers/input/misc/max77693-haptic.c |  2 +-
> >>>  drivers/input/misc/max8997_haptic.c  |  6 +-
> >>>  drivers/leds/leds-pwm.c  |  5 -
> >>>  drivers/media/rc/ir-rx51.c   |  5 -
> >>>  drivers/media/rc/pwm-ir-tx.c |  5 -
> >>>  drivers/video/backlight/lm3630a_bl.c |  4 +++-
> >>>  drivers/video/backlight/lp855x_bl.c  |  4 +++-
> >>>  drivers/video/backlight/lp8788_bl.c  |  5 -
> >>>  drivers/video/backlight/pwm_bl.c | 11 +--
> >>>  drivers/video/fbdev/ssd1307fb.c  |  3 ++-
> >>>  include/linux/pwm.h  |  6 --
> >>>  16 files changed, 70 insertions(+), 21 deletions(-)
> >>>
> >>> diff --git a/drivers/video/backlight/lm3630a_bl.c 
> >>> b/drivers/video/backlight/lm3630a_bl.c
> >>> index 2030a6b77a09..696fa25dafd2 100644
> >>> --- a/drivers/video/backlight/lm3630a_bl.c
> >>> +++ b/drivers/video/backlight/lm3630a_bl.c
> >>> @@ -165,8 +165,10 @@ static void lm3630a_pwm_ctrl(struct lm3630a_chip 
> >>> *pchip, int br, int br_max)
> >>>  {
> >>>   unsigned int period = pchip->pdata->pwm_period;
> >>>   unsigned int duty = br * period / br_max;
> >>> + struct pwm_caps caps = { };
> >>>  
> >>> - pwm_config(pchip->pwmd, duty, period);
> >>> + pwm_get_caps(pchip->pwmd->chip, pchip->pwmd, );
> >>> + pwm_config(pchip->pwmd, duty, period, BIT(ffs(caps.modes) - 1));
> >>
> >> Well... I admit I've only really looked at the patches that impact 
> >> backlight but dispersing this really odd looking bit twiddling 
> >> throughout the kernel doesn't strike me a great API design.
> >>
> >> IMHO callers should not be required to find the first set bit in
> >> some specially crafted set of capability bits simply to get sane 
> >> default behaviour.
> > 
> > Agreed. IMHO the regular use case becomes rather tedious, ugly, and
> > error prone.
> 
> Using simply PWM_MODE(NORMAL) instead of BIT(ffs(caps.modes) - 1) would be OK
> from your side?
>
> Or, what about using a function like pwm_mode_first() to get the first 
> supported
> mode by PWM channel?
> 
> Or, would you prefer to solve this inside pwm_config() function, let's say, in
> case an invalid mode is passed as argument, to let pwm_config() to choose the
> first available PWM mode for PWM channel passed as argument?

What is it that actually needs solving?

If a driver requests normal mode and the PWM driver cannot support it
why not just return an error an move on.

Put another way, what is the use case for secretly adopting a mode the
caller didn't want? Under what circumstances is this a good thing?


Daniel.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v3 05/10] pwm: add PWM mode to pwm_config()

[Daniel Thompson]

On Thu, Feb 22, 2018 at 02:01:16PM +0200, Claudiu Beznea wrote:
> Add PWM mode to pwm_config() function. The drivers which uses pwm_config()
> were adapted to this change.
> 
> Signed-off-by: Claudiu Beznea 
> ---
>  arch/arm/mach-s3c24xx/mach-rx1950.c  | 11 +--
>  drivers/bus/ts-nbus.c|  2 +-
>  drivers/clk/clk-pwm.c|  3 ++-
>  drivers/gpu/drm/i915/intel_panel.c   | 17 ++---
>  drivers/hwmon/pwm-fan.c  |  2 +-
>  drivers/input/misc/max77693-haptic.c |  2 +-
>  drivers/input/misc/max8997_haptic.c  |  6 +-
>  drivers/leds/leds-pwm.c  |  5 -
>  drivers/media/rc/ir-rx51.c   |  5 -
>  drivers/media/rc/pwm-ir-tx.c |  5 -
>  drivers/video/backlight/lm3630a_bl.c |  4 +++-
>  drivers/video/backlight/lp855x_bl.c  |  4 +++-
>  drivers/video/backlight/lp8788_bl.c  |  5 -
>  drivers/video/backlight/pwm_bl.c | 11 +--
>  drivers/video/fbdev/ssd1307fb.c  |  3 ++-
>  include/linux/pwm.h  |  6 --
>  16 files changed, 70 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/video/backlight/lm3630a_bl.c 
> b/drivers/video/backlight/lm3630a_bl.c
> index 2030a6b77a09..696fa25dafd2 100644
> --- a/drivers/video/backlight/lm3630a_bl.c
> +++ b/drivers/video/backlight/lm3630a_bl.c
> @@ -165,8 +165,10 @@ static void lm3630a_pwm_ctrl(struct lm3630a_chip *pchip, 
> int br, int br_max)
>  {
>   unsigned int period = pchip->pdata->pwm_period;
>   unsigned int duty = br * period / br_max;
> + struct pwm_caps caps = { };
>  
> - pwm_config(pchip->pwmd, duty, period);
> + pwm_get_caps(pchip->pwmd->chip, pchip->pwmd, );
> + pwm_config(pchip->pwmd, duty, period, BIT(ffs(caps.modes) - 1));

Well... I admit I've only really looked at the patches that impact 
backlight but dispersing this really odd looking bit twiddling 
throughout the kernel doesn't strike me a great API design.

IMHO callers should not be required to find the first set bit in
some specially crafted set of capability bits simply to get sane 
default behaviour.


Daniel.




>   if (duty)
>   pwm_enable(pchip->pwmd);
>   else
> diff --git a/drivers/video/backlight/lp855x_bl.c 
> b/drivers/video/backlight/lp855x_bl.c
> index 939f057836e1..3d274c604862 100644
> --- a/drivers/video/backlight/lp855x_bl.c
> +++ b/drivers/video/backlight/lp855x_bl.c
> @@ -240,6 +240,7 @@ static void lp855x_pwm_ctrl(struct lp855x *lp, int br, 
> int max_br)
>   unsigned int period = lp->pdata->period_ns;
>   unsigned int duty = br * period / max_br;
>   struct pwm_device *pwm;
> + struct pwm_caps caps = { };
>  
>   /* request pwm device with the consumer name */
>   if (!lp->pwm) {
> @@ -256,7 +257,8 @@ static void lp855x_pwm_ctrl(struct lp855x *lp, int br, 
> int max_br)
>   pwm_apply_args(pwm);
>   }
>  
> - pwm_config(lp->pwm, duty, period);
> + pwm_get_caps(lp->pwm->chip, lp->pwm, );
> + pwm_config(lp->pwm, duty, period, BIT(ffs(caps.modes) - 1));
>   if (duty)
>   pwm_enable(lp->pwm);
>   else
> diff --git a/drivers/video/backlight/lp8788_bl.c 
> b/drivers/video/backlight/lp8788_bl.c
> index cf869ec90cce..06de3163650d 100644
> --- a/drivers/video/backlight/lp8788_bl.c
> +++ b/drivers/video/backlight/lp8788_bl.c
> @@ -128,6 +128,7 @@ static void lp8788_pwm_ctrl(struct lp8788_bl *bl, int br, 
> int max_br)
>   unsigned int duty;
>   struct device *dev;
>   struct pwm_device *pwm;
> + struct pwm_caps caps = { };
>  
>   if (!bl->pdata)
>   return;
> @@ -153,7 +154,9 @@ static void lp8788_pwm_ctrl(struct lp8788_bl *bl, int br, 
> int max_br)
>   pwm_apply_args(pwm);
>   }
>  
> - pwm_config(bl->pwm, duty, period);
> + pwm_get_caps(bl->pwm->chip, bl->pwm, );
> +
> + pwm_config(bl->pwm, duty, period, BIT(ffs(caps.modes) - 1));
>   if (duty)
>   pwm_enable(bl->pwm);
>   else
> diff --git a/drivers/video/backlight/pwm_bl.c 
> b/drivers/video/backlight/pwm_bl.c
> index 1c2289ddd555..706a9ab053a7 100644
> --- a/drivers/video/backlight/pwm_bl.c
> +++ b/drivers/video/backlight/pwm_bl.c
> @@ -63,10 +63,14 @@ static void pwm_backlight_power_on(struct pwm_bl_data 
> *pb, int brightness)
>  
>  static void pwm_backlight_power_off(struct pwm_bl_data *pb)
>  {
> + struct pwm_caps caps = { };
> +
>   if (!pb->enabled)
>   return;
>  
> - pwm_config(pb->pwm, 0, pb->period);
> + pwm_get_caps(pb->pwm->chip, pb->pwm, );
> +
> + pwm_config(pb->pwm, 0, pb->period, BIT(ffs(caps.modes) - 1));
>   pwm_disable(pb->pwm);
>  
>   if (pb->enable_gpio)
> @@ -96,6 +100,7 @@ static int pwm_backlight_update_status(struct 
> backlight_device *bl)
>  {
>   struct pwm_bl_data *pb = bl_get_data(bl);
>   int brightness = bl->props.brightness;
> + struct pwm_caps caps = { };
>   int duty_cycle;
>  
>   if 

Re: [Intel-gfx] [PATCH v3] dma-buf: cleanup dma_buf_export() to make it easily extensible

[Daniel Thompson]

On 28/01/15 12:54, Sumit Semwal wrote:
 At present, dma_buf_export() takes a series of parameters, which
 makes it difficult to add any new parameters for exporters, if required.
 
 Make it simpler by moving all these parameters into a struct, and pass
 the struct * as parameter to dma_buf_export().
 
 While at it, unite dma_buf_export_named() with dma_buf_export(), and
 change all callers accordingly.
 
 Signed-off-by: Sumit Semwal sumit.sem...@linaro.org

Sorry, a few more comments. Should have sent these before but at least
the are all related only to documentation. Once that is fixed then:
Reviewed-by: Daniel Thompson daniel.thomp...@linaro.org


 ---
 v3: Daniel Thompson caught the C99 warning issue w/ using {0}; using
 {.exp_name = xxx} instead.
 
 v2: add macro to zero out local struct, and fill KBUILD_MODNAME by default
 
  drivers/dma-buf/dma-buf.c  | 47 
 +-
  drivers/gpu/drm/armada/armada_gem.c| 10 --
  drivers/gpu/drm/drm_prime.c| 12 ---
  drivers/gpu/drm/exynos/exynos_drm_dmabuf.c |  9 +++--
  drivers/gpu/drm/i915/i915_gem_dmabuf.c | 10 --
  drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c  |  9 -
  drivers/gpu/drm/tegra/gem.c| 10 --
  drivers/gpu/drm/ttm/ttm_object.c   |  9 +++--
  drivers/gpu/drm/udl/udl_dmabuf.c   |  9 -
  drivers/media/v4l2-core/videobuf2-dma-contig.c |  8 -
  drivers/media/v4l2-core/videobuf2-dma-sg.c |  8 -
  drivers/media/v4l2-core/videobuf2-vmalloc.c|  8 -
  drivers/staging/android/ion/ion.c  |  9 +++--
  include/linux/dma-buf.h| 34 +++

Documentation/dma-buf-sharing.txt needs updating as a result of these
changes but its not in the diffstat.


  14 files changed, 142 insertions(+), 50 deletions(-)
 
 diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
 index 5be225c2ba98..6d3df3dd9310 100644
 --- a/drivers/dma-buf/dma-buf.c
 +++ b/drivers/dma-buf/dma-buf.c
 @@ -265,7 +265,7 @@ static inline int is_dma_buf_file(struct file *file)
  }
  
  /**
 - * dma_buf_export_named - Creates a new dma_buf, and associates an anon file
 + * dma_buf_export - Creates a new dma_buf, and associates an anon file
   * with this buffer, so it can be exported.
   * Also connect the allocator specific data and ops to the buffer.
   * Additionally, provide a name string for exporter; useful in debugging.
 @@ -277,31 +277,32 @@ static inline int is_dma_buf_file(struct file *file)
   * @exp_name:[in]name of the exporting module - useful for 
 debugging.
   * @resv:[in]reservation-object, NULL to allocate default one.
   *
 + * All the above info comes from struct dma_buf_export_info.
 + *

I'm not at all sure about this. Its a novel trick but won't this make
the HTML docs come out looking a bit weird? Is there any prior art for
double-documenting the structure members like this?


Daniel.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2] dma-buf: cleanup dma_buf_export() to make it easily extensible

[Daniel Thompson]

On 28/01/15 06:00, Sumit Semwal wrote:
 At present, dma_buf_export() takes a series of parameters, which
 makes it difficult to add any new parameters for exporters, if required.
 
 Make it simpler by moving all these parameters into a struct, and pass
 the struct * as parameter to dma_buf_export().
 
 While at it, unite dma_buf_export_named() with dma_buf_export(), and
 change all callers accordingly.
 
 Signed-off-by: Sumit Semwal sumit.sem...@linaro.org
 ---
 v2: add macro to zero out local struct, and fill KBUILD_MODNAME by default
 
  drivers/dma-buf/dma-buf.c  | 47 
 +-
  drivers/gpu/drm/armada/armada_gem.c| 10 --
  drivers/gpu/drm/drm_prime.c| 12 ---
  drivers/gpu/drm/exynos/exynos_drm_dmabuf.c |  9 +++--
  drivers/gpu/drm/i915/i915_gem_dmabuf.c | 10 --
  drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c  |  9 -
  drivers/gpu/drm/tegra/gem.c| 10 --
  drivers/gpu/drm/ttm/ttm_object.c   |  9 +++--
  drivers/gpu/drm/udl/udl_dmabuf.c   |  9 -
  drivers/media/v4l2-core/videobuf2-dma-contig.c |  8 -
  drivers/media/v4l2-core/videobuf2-dma-sg.c |  8 -
  drivers/media/v4l2-core/videobuf2-vmalloc.c|  8 -
  drivers/staging/android/ion/ion.c  |  9 +++--
  include/linux/dma-buf.h| 35 +++
  14 files changed, 143 insertions(+), 50 deletions(-)
 
 diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
 index 5be225c2ba98..6d3df3dd9310 100644
 --- a/drivers/dma-buf/dma-buf.c
 +++ b/drivers/dma-buf/dma-buf.c
 @@ -265,7 +265,7 @@ static inline int is_dma_buf_file(struct file *file)
  }
  
  /**
 - * dma_buf_export_named - Creates a new dma_buf, and associates an anon file
 + * dma_buf_export - Creates a new dma_buf, and associates an anon file
   * with this buffer, so it can be exported.
   * Also connect the allocator specific data and ops to the buffer.
   * Additionally, provide a name string for exporter; useful in debugging.
 @@ -277,31 +277,32 @@ static inline int is_dma_buf_file(struct file *file)
   * @exp_name:[in]name of the exporting module - useful for 
 debugging.
   * @resv:[in]reservation-object, NULL to allocate default one.
   *
 + * All the above info comes from struct dma_buf_export_info.
 + *
   * Returns, on success, a newly created dma_buf object, which wraps the
   * supplied private data and operations for dma_buf_ops. On either missing
   * ops, or error in allocating struct dma_buf, will return negative error.
   *
   */
 -struct dma_buf *dma_buf_export_named(void *priv, const struct dma_buf_ops 
 *ops,
 - size_t size, int flags, const char *exp_name,
 - struct reservation_object *resv)
 +struct dma_buf *dma_buf_export(struct dma_buf_export_info *exp_info)
  {
   struct dma_buf *dmabuf;
   struct file *file;
   size_t alloc_size = sizeof(struct dma_buf);
 - if (!resv)
 + if (!exp_info-resv)
   alloc_size += sizeof(struct reservation_object);
   else
   /* prevent dma_buf[1] == dma_buf-resv */
   alloc_size += 1;
  
 - if (WARN_ON(!priv || !ops
 -   || !ops-map_dma_buf
 -   || !ops-unmap_dma_buf
 -   || !ops-release
 -   || !ops-kmap_atomic
 -   || !ops-kmap
 -   || !ops-mmap)) {
 + if (WARN_ON(!exp_info-priv
 +   || !exp_info-ops
 +   || !exp_info-ops-map_dma_buf
 +   || !exp_info-ops-unmap_dma_buf
 +   || !exp_info-ops-release
 +   || !exp_info-ops-kmap_atomic
 +   || !exp_info-ops-kmap
 +   || !exp_info-ops-mmap)) {
   return ERR_PTR(-EINVAL);
   }
  
 @@ -309,21 +310,22 @@ struct dma_buf *dma_buf_export_named(void *priv, const 
 struct dma_buf_ops *ops,
   if (dmabuf == NULL)
   return ERR_PTR(-ENOMEM);
  
 - dmabuf-priv = priv;
 - dmabuf-ops = ops;
 - dmabuf-size = size;
 - dmabuf-exp_name = exp_name;
 + dmabuf-priv = exp_info-priv;
 + dmabuf-ops = exp_info-ops;
 + dmabuf-size = exp_info-size;
 + dmabuf-exp_name = exp_info-exp_name;
   init_waitqueue_head(dmabuf-poll);
   dmabuf-cb_excl.poll = dmabuf-cb_shared.poll = dmabuf-poll;
   dmabuf-cb_excl.active = dmabuf-cb_shared.active = 0;
  
 - if (!resv) {
 - resv = (struct reservation_object *)dmabuf[1];
 - reservation_object_init(resv);
 + if (!exp_info-resv) {
 + exp_info-resv = (struct reservation_object *)dmabuf[1];
 + reservation_object_init(exp_info-resv);
   }
 - dmabuf-resv = resv;
 + dmabuf-resv = exp_info-resv;
  
 - file = anon_inode_getfile(dmabuf, 

Re: [Intel-gfx] [PATCH] drm/atomic-helper: implement -page_flip

[Daniel Thompson]

On 04/11/14 22:09, Daniel Vetter wrote:
 Currently there is no way to implement async flips using atomic, that
 essentially requires us to be able to cancel pending requests
 mid-flight.
 
 To be able to do that (and I guess we want this since vblank synced
 updates whic opportunistically cancel still pending updates seem to be
 wanted) we'd need to add a mandatory cancellation mode. Depending upon
 the exact semantics we decide upon that could mean that userspace will
 not get completion events, or will get them all stacked up.
 
 So reject async updates for now. Also async updates usually means not
 vblank synced at all, and I guess for drivers which want to support
 this they should simply add a special pageflip handler (since usually
 you need a special flip cmd to achieve this). That kind of async flip
 is pretty much exclusively just used for games and benchmarks where
 dropping just one frame means you'll get a headshot or something bad
 like that ... And so slight amounts of tearing is acceptable.
 
 v2: Fixup kerneldoc, reported by Paulo.
 
 v3: Use the set_crtc_for_plane function to assign the crtc, since
 otherwise the book-keeping is off.
 
 v4: Update crtc-primary-fb since -page_flip is the only driver
 callback where the core won't do this itself. We might want to fix
 this inconsistency eventually.
 
 v5: Use set_crtc_for_connector as suggested by Sean.
 
 Cc: Sean Paul seanp...@chromium.org
 Cc: Paulo Zanoni przan...@gmail.com
 Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch
 ---
  drivers/gpu/drm/drm_atomic_helper.c | 92 
 +
  include/drm/drm_atomic_helper.h |  5 ++
  2 files changed, 97 insertions(+)
 
 diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
 b/drivers/gpu/drm/drm_atomic_helper.c
 index 790edc18b2cb..36ccc42d43db 100644
 --- a/drivers/gpu/drm/drm_atomic_helper.c
 +++ b/drivers/gpu/drm/drm_atomic_helper.c
 @@ -1633,3 +1633,95 @@ backoff:
   goto retry;
  }
  EXPORT_SYMBOL(drm_atomic_helper_connector_set_property);
 +
 +/**
 + * drm_atomic_helper_page_flip - execute a legacy page flip
 + * @crtc: DRM crtc
 + * @fb: DRM framebuffer
 + * @event: optional DRM event to signal upon completion
 + * @flags: flip flags for non-vblank sync'ed updates
 + *
 + * Provides a default page flip implementation using the atomic driver 
 interface.
 + *
 + * Note that for now so called async page flips (i.e. updates which are not
 + * synchronized to vblank) are not supported, since the atomic interfaces 
 have
 + * no provisions for this yet.
 + *
 + * Returns:
 + * Returns 0 on success, negative errno numbers on failure.
 + */
 +int drm_atomic_helper_page_flip(struct drm_crtc *crtc,
 + struct drm_framebuffer *fb,
 + struct drm_pending_vblank_event *event,
 + uint32_t flags)
 +{
 + struct drm_plane *plane = crtc-primary;
 + struct drm_atomic_state *state;
 + struct drm_plane_state *plane_state;
 + struct drm_crtc_state *crtc_state;
 + int ret = 0;
 +
 + if (flags  DRM_MODE_PAGE_FLIP_ASYNC)
 + return -EINVAL;
 +
 + state = drm_atomic_state_alloc(plane-dev);
 + if (!state)
 + return -ENOMEM;
 +
 + state-acquire_ctx = drm_modeset_legacy_acquire_ctx(crtc);
 +retry:
 + crtc_state = drm_atomic_get_crtc_state(state, crtc);
 + if (IS_ERR(crtc_state)) {
 + ret = PTR_ERR(crtc_state);
 + if (ret == -EDEADLK)
 + goto backoff;
 + else
 + goto fail;
 + }
 + crtc_state-event = event;
 +
 + plane_state = drm_atomic_get_plane_state(state, plane);
 + if (IS_ERR(plane_state)) {
 + ret = PTR_ERR(plane_state);
 + if (ret == -EDEADLK)
 + goto backoff;
 + else
 + goto fail;
 + }
 +
 + ret = drm_atomic_set_crtc_for_plane(plane_state, crtc);
 + if (ret == -EDEADLK)
 + goto backoff;

The kerneldoc for this function states that it can return -ENOMEM too
and, even if it didn't, this approach to error handling is potentially
fragile as the kernel changes.


 + plane_state-fb = fb;
 +
 + ret = drm_atomic_async_commit(state);
 + if (ret == -EDEADLK)
 + goto backoff;

A fussy point but this would be easier to read if it followed the same
pattern as the above error checks with a goto fail. The the success
path below can be outdented and made more obvious.


 +
 + /* Driver takes ownership of state on successful async commit. */
 + if (ret == 0) {
 + /* TODO: -page_flip is the only driver callback where the core
 +  * doesn't update plane-fb. For now patch it up here. */
 + plane-fb = plane-state-fb;
 +
 + return 0;
 + }
 +
 +fail:
 + drm_atomic_state_free(state);
 +
 + return ret;
 +backoff:
 + drm_atomic_legacy_backoff(state);
 + 

Re: [Intel-gfx] [PATCH 15/17] drm/atomic-helpers: functions for state duplicate/destroy/reset

[Daniel Thompson]

On 02/11/14 13:19, Daniel Vetter wrote: The atomic users and helpers
assume that there is always a obj-state
 structure around. Which means drivers need to somehow create that at
 driver load time. Also it should obviously reset hardware state, so
 needs to be reset upon resume.

 Finally the destroy/duplicate_state functions are an awful lot of
 boilerplate if the driver doesn't need anything beyond the default
 state objects.

 So add helper functions for all of this.

 v2: Somehow the plane/connector versions got lost in the first
 version.

 v3: Add kerneldoc.

 v4: Make duplicate_state functions a bit more robust, which is useful
 for debugging state tracking issues when transitioning to atomic.

 v5: Clear temporary variables in the crtc state when duplicating it,
 like -mode_changed or -planes_changed. If we don't do this stale
 values for these might pollute the next atomic modeset.

 v6: Also clear crtc_state-event in case the driver didn't (yet) clear
 this out.

 Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch
 ---
  drivers/gpu/drm/drm_atomic_helper.c | 154
+++-
  include/drm/drm_atomic_helper.h |  19 +
  2 files changed, 170 insertions(+), 3 deletions(-)

 diff --git a/drivers/gpu/drm/drm_atomic_helper.c
b/drivers/gpu/drm/drm_atomic_helper.c
 index 70bd67cf86e3..bd38df3cbe55 100644
 --- a/drivers/gpu/drm/drm_atomic_helper.c
 +++ b/drivers/gpu/drm/drm_atomic_helper.c
 @@ -1429,7 +1429,7 @@ EXPORT_SYMBOL(drm_atomic_helper_set_config);
  /**
   * drm_atomic_helper_crtc_set_property - helper for crtc prorties
   * @crtc: DRM crtc
 - * @prorty: DRM property
 + * @property: DRM property

This looks like a bad fixup (should be in patch 11).


   * @val: value of property
   *
   * Provides a default plane disablle handler using the atomic driver
interface.
 @@ -1493,7 +1493,7 @@ EXPORT_SYMBOL(drm_atomic_helper_crtc_set_property);
  /**
   * drm_atomic_helper_plane_set_property - helper for plane prorties
   * @plane: DRM plane
 - * @prorty: DRM property
 + * @property: DRM property

+1


   * @val: value of property
   *
   * Provides a default plane disable handler using the atomic driver
interface.
 @@ -1557,7 +1557,7 @@ EXPORT_SYMBOL(drm_atomic_helper_plane_set_property);
  /**
   * drm_atomic_helper_connector_set_property - helper for connector
prorties
   * @connector: DRM connector
 - * @prorty: DRM property
 + * @property: DRM property

+1


   * @val: value of property
   *
   * Provides a default plane disablle handler using the atomic driver
interface.
 @@ -1707,3 +1707,151 @@ backoff:
   goto retry;
  }
  EXPORT_SYMBOL(drm_atomic_helper_page_flip);
 +
 +/**
 + * drm_atomic_helper_crtc_reset - default -reset hook for CRTCs
 + * @crtc: drm CRTC
 + *
 + * Resets the atomic state for @crtc by freeing the state pointer and
allocating
 + * a new empty state object.
 + */
 +void drm_atomic_helper_crtc_reset(struct drm_crtc *crtc)
 +{
 + kfree(crtc-state);
 + crtc-state = kzalloc(sizeof(*crtc-state), GFP_KERNEL);

This code looks semantically equivalent to a memset() although it may
result in a change to the pointer value. Is this code trying to flush
out uses-after-free?

I can't find this free/alloc pattern in delivered code anywhere else in
the drm code base. Should this need to be replaced with memset() before
merging (or at least commenting)?


 +}
 +EXPORT_SYMBOL(drm_atomic_helper_crtc_reset);
 +
 +/**
 + * drm_atomic_helper_crtc_duplicate_state - default state duplicate hook
 + * @crtc: drm CRTC
 + *
 + * Default CRTC state duplicate hook for drivers which don't have
their own
 + * subclassed CRTC state structure.
 + */
 +struct drm_crtc_state *
 +drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc)
 +{
 + struct drm_crtc_state *state;
 +
 + if (WARN_ON(!crtc-state))
 + return NULL;
 +
 + state = kmemdup(crtc-state, sizeof(*crtc-state), GFP_KERNEL);
 +
 + if (state) {
 + state-mode_changed = false;
 + state-planes_changed = false;
 + state-event = NULL;
 + }
 +
 + return state;
 +}
 +EXPORT_SYMBOL(drm_atomic_helper_crtc_duplicate_state);
 +
 +/**
 + * drm_atomic_helper_crtc_destroy_state - default state destroy hook
 + * @crtc: drm CRTC
 + * @state: CRTC state object to release
 + *
 + * Default CRTC state destroy hook for drivers which don't have their own
 + * subclassed CRTC state structure.
 + */
 +void drm_atomic_helper_crtc_destroy_state(struct drm_crtc *crtc,
 +   struct drm_crtc_state *state)
 +{
 + kfree(state);
 +}
 +EXPORT_SYMBOL(drm_atomic_helper_crtc_destroy_state);
 +
 +/**
 + * drm_atomic_helper_plane_reset - default -reset hook for planes
 + * @plane: drm plane
 + *
 + * Resets the atomic state for @plane by freeing the state pointer and
 + * allocating a new empty state object.
 + */
 +void drm_atomic_helper_plane_reset(struct drm_plane *plane)
 +{
 + kfree(plane-state);
 + plane-state = 

Re: [Intel-gfx] [PATCH 15/17] drm/atomic-helpers: functions for state duplicate/destroy/reset

[Daniel Thompson]

On 03/11/14 14:53, Daniel Vetter wrote:
 On Mon, Nov 03, 2014 at 02:45:28PM +, Daniel Thompson wrote:
 index 70bd67cf86e3..bd38df3cbe55 100644
 --- a/drivers/gpu/drm/drm_atomic_helper.c
 +++ b/drivers/gpu/drm/drm_atomic_helper.c
 @@ -1429,7 +1429,7 @@ EXPORT_SYMBOL(drm_atomic_helper_set_config);
  /**
   * drm_atomic_helper_crtc_set_property - helper for crtc prorties
   * @crtc: DRM crtc
 - * @prorty: DRM property
 + * @property: DRM property

 This looks like a bad fixup (should be in patch 11).
 
 Indeed, will shuffle around.
 
 +void drm_atomic_helper_crtc_reset(struct drm_crtc *crtc)
 +{
 +   kfree(crtc-state);
 +   crtc-state = kzalloc(sizeof(*crtc-state), GFP_KERNEL);

 This code looks semantically equivalent to a memset() although it may
 result in a change to the pointer value. Is this code trying to flush
 out uses-after-free?

 I can't find this free/alloc pattern in delivered code anywhere else in
 the drm code base. Should this need to be replaced with memset() before
 merging (or at least commenting)?
 
 kfree is a nop when the argument is NULL, which is a crucial property of
 this - memset would oops on driver load.

Oops. Missed that (I think I misread who as assuming there was always
obj-state in the patch header).

Do you fancy making the comment by freeing the state pointer and
allocating a new... into by freeing the state pointer (which may be
NULL) and allocating a new

If nothing else that means the documentation is richer than the code...

 Even neglecting this a memset imo doesn't blow up loudly enough if the
 driver subclasses the state structs (by adding more of it's driver private
 state at the end). Whereas underallocating tends to anger the slab
 poisoning code badly.
 
 Finally it's really not just a memset, but a free + realloc. See the plane
 state, which also needs to drop a potential fb reference. Imo the explicit
 kfree+realloc makes that more obvious.

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx