diff mbox

[2/8] drm/udl: Change drm_fb_helper_sys_*() calls to sys_*()

Message ID 1461165929-11344-3-git-send-email-noralf@tronnes.org (mailing list archive)
State New, archived
Headers show

Commit Message

Noralf Trønnes April 20, 2016, 3:25 p.m. UTC
Now that drm_fb_helper gets deferred io support, the
drm_fb_helper_sys_{fillrect,copyarea,imageblit} functions will schedule
the worker that calls the deferred_io callback. This will break this
driver so use the sys_{fillrect,copyarea,imageblit} functions directly.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 drivers/gpu/drm/udl/udl_fb.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Daniel Vetter April 20, 2016, 5:42 p.m. UTC | #1
On Wed, Apr 20, 2016 at 05:25:23PM +0200, Noralf Trønnes wrote:
> Now that drm_fb_helper gets deferred io support, the
> drm_fb_helper_sys_{fillrect,copyarea,imageblit} functions will schedule
> the worker that calls the deferred_io callback. This will break this
> driver so use the sys_{fillrect,copyarea,imageblit} functions directly.
> 
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>

I think this intermediately breaks the build, if you disable fbdev
support. That's now supported in the fbdev helpers core generically across
all drivers.

Not sure how to best fix this up, since the only way would be to squash
these patches, plus generic deferred io plus the conversion patches for
udl/qxl all into one. Tricky.
-Daniel

> ---
>  drivers/gpu/drm/udl/udl_fb.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c
> index fd1eb9d..a52de2f 100644
> --- a/drivers/gpu/drm/udl/udl_fb.c
> +++ b/drivers/gpu/drm/udl/udl_fb.c
> @@ -287,7 +287,7 @@ static void udl_fb_fillrect(struct fb_info *info, const struct fb_fillrect *rect
>  {
>  	struct udl_fbdev *ufbdev = info->par;
>  
> -	drm_fb_helper_sys_fillrect(info, rect);
> +	sys_fillrect(info, rect);
>  
>  	udl_handle_damage(&ufbdev->ufb, rect->dx, rect->dy, rect->width,
>  			  rect->height);
> @@ -297,7 +297,7 @@ static void udl_fb_copyarea(struct fb_info *info, const struct fb_copyarea *regi
>  {
>  	struct udl_fbdev *ufbdev = info->par;
>  
> -	drm_fb_helper_sys_copyarea(info, region);
> +	sys_copyarea(info, region);
>  
>  	udl_handle_damage(&ufbdev->ufb, region->dx, region->dy, region->width,
>  			  region->height);
> @@ -307,7 +307,7 @@ static void udl_fb_imageblit(struct fb_info *info, const struct fb_image *image)
>  {
>  	struct udl_fbdev *ufbdev = info->par;
>  
> -	drm_fb_helper_sys_imageblit(info, image);
> +	sys_imageblit(info, image);
>  
>  	udl_handle_damage(&ufbdev->ufb, image->dx, image->dy, image->width,
>  			  image->height);
> -- 
> 2.2.2
>
Noralf Trønnes April 20, 2016, 6:15 p.m. UTC | #2
Den 20.04.2016 19:42, skrev Daniel Vetter:
> On Wed, Apr 20, 2016 at 05:25:23PM +0200, Noralf Trønnes wrote:
>> Now that drm_fb_helper gets deferred io support, the
>> drm_fb_helper_sys_{fillrect,copyarea,imageblit} functions will schedule
>> the worker that calls the deferred_io callback. This will break this
>> driver so use the sys_{fillrect,copyarea,imageblit} functions directly.
>>
>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> I think this intermediately breaks the build, if you disable fbdev
> support. That's now supported in the fbdev helpers core generically across
> all drivers.
>
> Not sure how to best fix this up, since the only way would be to squash
> these patches, plus generic deferred io plus the conversion patches for
> udl/qxl all into one. Tricky.

Yes you're right, I missed that.
How about this:
#ifdef CONFIG_FB
         sys_fillrect(info, rect);
#endif

The later patch will then remove this ugliness...

> -Daniel
>
>> ---
>>   drivers/gpu/drm/udl/udl_fb.c | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c
>> index fd1eb9d..a52de2f 100644
>> --- a/drivers/gpu/drm/udl/udl_fb.c
>> +++ b/drivers/gpu/drm/udl/udl_fb.c
>> @@ -287,7 +287,7 @@ static void udl_fb_fillrect(struct fb_info *info, const struct fb_fillrect *rect
>>   {
>>   	struct udl_fbdev *ufbdev = info->par;
>>   
>> -	drm_fb_helper_sys_fillrect(info, rect);
>> +	sys_fillrect(info, rect);
>>   
>>   	udl_handle_damage(&ufbdev->ufb, rect->dx, rect->dy, rect->width,
>>   			  rect->height);
>> @@ -297,7 +297,7 @@ static void udl_fb_copyarea(struct fb_info *info, const struct fb_copyarea *regi
>>   {
>>   	struct udl_fbdev *ufbdev = info->par;
>>   
>> -	drm_fb_helper_sys_copyarea(info, region);
>> +	sys_copyarea(info, region);
>>   
>>   	udl_handle_damage(&ufbdev->ufb, region->dx, region->dy, region->width,
>>   			  region->height);
>> @@ -307,7 +307,7 @@ static void udl_fb_imageblit(struct fb_info *info, const struct fb_image *image)
>>   {
>>   	struct udl_fbdev *ufbdev = info->par;
>>   
>> -	drm_fb_helper_sys_imageblit(info, image);
>> +	sys_imageblit(info, image);
>>   
>>   	udl_handle_damage(&ufbdev->ufb, image->dx, image->dy, image->width,
>>   			  image->height);
>> -- 
>> 2.2.2
>>

--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Daniel Vetter April 21, 2016, 7:28 a.m. UTC | #3
On Wed, Apr 20, 2016 at 08:15:30PM +0200, Noralf Trønnes wrote:
> 
> Den 20.04.2016 19:42, skrev Daniel Vetter:
> >On Wed, Apr 20, 2016 at 05:25:23PM +0200, Noralf Trønnes wrote:
> >>Now that drm_fb_helper gets deferred io support, the
> >>drm_fb_helper_sys_{fillrect,copyarea,imageblit} functions will schedule
> >>the worker that calls the deferred_io callback. This will break this
> >>driver so use the sys_{fillrect,copyarea,imageblit} functions directly.
> >>
> >>Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> >I think this intermediately breaks the build, if you disable fbdev
> >support. That's now supported in the fbdev helpers core generically across
> >all drivers.
> >
> >Not sure how to best fix this up, since the only way would be to squash
> >these patches, plus generic deferred io plus the conversion patches for
> >udl/qxl all into one. Tricky.
> 
> Yes you're right, I missed that.
> How about this:
> #ifdef CONFIG_FB
>         sys_fillrect(info, rect);
> #endif
> 
> The later patch will then remove this ugliness...

Yeah I think we have to bite the bullet and take this temporary ugliness
:(

-Daniel
Noralf Trønnes April 21, 2016, 6:18 p.m. UTC | #4
Den 21.04.2016 09:28, skrev Daniel Vetter:
> On Wed, Apr 20, 2016 at 08:15:30PM +0200, Noralf Trønnes wrote:
>> Den 20.04.2016 19:42, skrev Daniel Vetter:
>>> On Wed, Apr 20, 2016 at 05:25:23PM +0200, Noralf Trønnes wrote:
>>>> Now that drm_fb_helper gets deferred io support, the
>>>> drm_fb_helper_sys_{fillrect,copyarea,imageblit} functions will schedule
>>>> the worker that calls the deferred_io callback. This will break this
>>>> driver so use the sys_{fillrect,copyarea,imageblit} functions directly.
>>>>
>>>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>>> I think this intermediately breaks the build, if you disable fbdev
>>> support. That's now supported in the fbdev helpers core generically across
>>> all drivers.
>>>
>>> Not sure how to best fix this up, since the only way would be to squash
>>> these patches, plus generic deferred io plus the conversion patches for
>>> udl/qxl all into one. Tricky.
>> Yes you're right, I missed that.
>> How about this:
>> #ifdef CONFIG_FB
>>          sys_fillrect(info, rect);
>> #endif
>>
>> The later patch will then remove this ugliness...
> Yeah I think we have to bite the bullet and take this temporary ugliness
> :(

Turns out the #ifdef isn't necessary since FB is always selected.

Both udl and qxl have this:
         select DRM_KMS_HELPER
         select DRM_KMS_FB_HELPER

And then we have:

config DRM_KMS_HELPER
         tristate
         depends on DRM

config DRM_KMS_FB_HELPER
         bool
         depends on DRM_KMS_HELPER
         select FB
         ...
         select FB_SYS_FILLRECT
         select FB_SYS_COPYAREA
         select FB_SYS_IMAGEBLIT


Noralf.

--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Daniel Vetter April 22, 2016, 8:24 a.m. UTC | #5
On Thu, Apr 21, 2016 at 08:18:48PM +0200, Noralf Trønnes wrote:
> 
> Den 21.04.2016 09:28, skrev Daniel Vetter:
> >On Wed, Apr 20, 2016 at 08:15:30PM +0200, Noralf Trønnes wrote:
> >>Den 20.04.2016 19:42, skrev Daniel Vetter:
> >>>On Wed, Apr 20, 2016 at 05:25:23PM +0200, Noralf Trønnes wrote:
> >>>>Now that drm_fb_helper gets deferred io support, the
> >>>>drm_fb_helper_sys_{fillrect,copyarea,imageblit} functions will schedule
> >>>>the worker that calls the deferred_io callback. This will break this
> >>>>driver so use the sys_{fillrect,copyarea,imageblit} functions directly.
> >>>>
> >>>>Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> >>>I think this intermediately breaks the build, if you disable fbdev
> >>>support. That's now supported in the fbdev helpers core generically across
> >>>all drivers.
> >>>
> >>>Not sure how to best fix this up, since the only way would be to squash
> >>>these patches, plus generic deferred io plus the conversion patches for
> >>>udl/qxl all into one. Tricky.
> >>Yes you're right, I missed that.
> >>How about this:
> >>#ifdef CONFIG_FB
> >>         sys_fillrect(info, rect);
> >>#endif
> >>
> >>The later patch will then remove this ugliness...
> >Yeah I think we have to bite the bullet and take this temporary ugliness
> >:(
> 
> Turns out the #ifdef isn't necessary since FB is always selected.
> 
> Both udl and qxl have this:
>         select DRM_KMS_HELPER
>         select DRM_KMS_FB_HELPER
> 
> And then we have:
> 
> config DRM_KMS_HELPER
>         tristate
>         depends on DRM
> 
> config DRM_KMS_FB_HELPER
>         bool
>         depends on DRM_KMS_HELPER
>         select FB
>         ...
>         select FB_SYS_FILLRECT
>         select FB_SYS_COPYAREA
>         select FB_SYS_IMAGEBLIT

Hm ... the thing that actually builds fbdev emulation is
DRM_FBDEV_EMULATION, and you can disable that. Otoh the select FB stuff
seems to be at the wrong level and probably should be moved.

But indeed I tried doing this and it's an impossible config. I guess I
need to type a patch to ditch all these selects from drivers ;-)
-Daniel
Emil Velikov April 24, 2016, 10:16 a.m. UTC | #6
On 22 April 2016 at 09:24, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Thu, Apr 21, 2016 at 08:18:48PM +0200, Noralf Trønnes wrote:
>>
>> Den 21.04.2016 09:28, skrev Daniel Vetter:
>> >On Wed, Apr 20, 2016 at 08:15:30PM +0200, Noralf Trønnes wrote:
>> >>Den 20.04.2016 19:42, skrev Daniel Vetter:
>> >>>On Wed, Apr 20, 2016 at 05:25:23PM +0200, Noralf Trønnes wrote:
>> >>>>Now that drm_fb_helper gets deferred io support, the
>> >>>>drm_fb_helper_sys_{fillrect,copyarea,imageblit} functions will schedule
>> >>>>the worker that calls the deferred_io callback. This will break this
>> >>>>driver so use the sys_{fillrect,copyarea,imageblit} functions directly.
>> >>>>
>> >>>>Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>> >>>I think this intermediately breaks the build, if you disable fbdev
>> >>>support. That's now supported in the fbdev helpers core generically across
>> >>>all drivers.
>> >>>
>> >>>Not sure how to best fix this up, since the only way would be to squash
>> >>>these patches, plus generic deferred io plus the conversion patches for
>> >>>udl/qxl all into one. Tricky.
>> >>Yes you're right, I missed that.
>> >>How about this:
>> >>#ifdef CONFIG_FB
>> >>         sys_fillrect(info, rect);
>> >>#endif
>> >>
>> >>The later patch will then remove this ugliness...
>> >Yeah I think we have to bite the bullet and take this temporary ugliness
>> >:(
>>
>> Turns out the #ifdef isn't necessary since FB is always selected.
>>
>> Both udl and qxl have this:
>>         select DRM_KMS_HELPER
>>         select DRM_KMS_FB_HELPER
>>
>> And then we have:
>>
>> config DRM_KMS_HELPER
>>         tristate
>>         depends on DRM
>>
>> config DRM_KMS_FB_HELPER
>>         bool
>>         depends on DRM_KMS_HELPER
>>         select FB
>>         ...
>>         select FB_SYS_FILLRECT
>>         select FB_SYS_COPYAREA
>>         select FB_SYS_IMAGEBLIT
>
> Hm ... the thing that actually builds fbdev emulation is
> DRM_FBDEV_EMULATION, and you can disable that. Otoh the select FB stuff
> seems to be at the wrong level and probably should be moved.
>
> But indeed I tried doing this and it's an impossible config. I guess I
> need to type a patch to ditch all these selects from drivers ;-)
That's a great idea imho.

Note that some drivers still partially use FB directly - radeon comes
to mind. Last time I've looked there was no particular reason for
that, as relevant fb_helper already exists.
In general I'm wondering if one cannot check-in a new cocci script
with each function that get factored out. Even if they don't produce
nice (enough) patches they will serve as a nice warning/sanity check,
right ?

If anyone is wondering "Why should we bother?", mostly because people
(too) often base their work before said functionality/helper has
landed. There's also that we cannot catch everything during review ;-)

Regards,
Emil
--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Daniel Vetter April 25, 2016, 8:31 a.m. UTC | #7
On Sun, Apr 24, 2016 at 11:16:34AM +0100, Emil Velikov wrote:
> On 22 April 2016 at 09:24, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Thu, Apr 21, 2016 at 08:18:48PM +0200, Noralf Trønnes wrote:
> >>
> >> Den 21.04.2016 09:28, skrev Daniel Vetter:
> >> >On Wed, Apr 20, 2016 at 08:15:30PM +0200, Noralf Trønnes wrote:
> >> >>Den 20.04.2016 19:42, skrev Daniel Vetter:
> >> >>>On Wed, Apr 20, 2016 at 05:25:23PM +0200, Noralf Trønnes wrote:
> >> >>>>Now that drm_fb_helper gets deferred io support, the
> >> >>>>drm_fb_helper_sys_{fillrect,copyarea,imageblit} functions will schedule
> >> >>>>the worker that calls the deferred_io callback. This will break this
> >> >>>>driver so use the sys_{fillrect,copyarea,imageblit} functions directly.
> >> >>>>
> >> >>>>Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> >> >>>I think this intermediately breaks the build, if you disable fbdev
> >> >>>support. That's now supported in the fbdev helpers core generically across
> >> >>>all drivers.
> >> >>>
> >> >>>Not sure how to best fix this up, since the only way would be to squash
> >> >>>these patches, plus generic deferred io plus the conversion patches for
> >> >>>udl/qxl all into one. Tricky.
> >> >>Yes you're right, I missed that.
> >> >>How about this:
> >> >>#ifdef CONFIG_FB
> >> >>         sys_fillrect(info, rect);
> >> >>#endif
> >> >>
> >> >>The later patch will then remove this ugliness...
> >> >Yeah I think we have to bite the bullet and take this temporary ugliness
> >> >:(
> >>
> >> Turns out the #ifdef isn't necessary since FB is always selected.
> >>
> >> Both udl and qxl have this:
> >>         select DRM_KMS_HELPER
> >>         select DRM_KMS_FB_HELPER
> >>
> >> And then we have:
> >>
> >> config DRM_KMS_HELPER
> >>         tristate
> >>         depends on DRM
> >>
> >> config DRM_KMS_FB_HELPER
> >>         bool
> >>         depends on DRM_KMS_HELPER
> >>         select FB
> >>         ...
> >>         select FB_SYS_FILLRECT
> >>         select FB_SYS_COPYAREA
> >>         select FB_SYS_IMAGEBLIT
> >
> > Hm ... the thing that actually builds fbdev emulation is
> > DRM_FBDEV_EMULATION, and you can disable that. Otoh the select FB stuff
> > seems to be at the wrong level and probably should be moved.
> >
> > But indeed I tried doing this and it's an impossible config. I guess I
> > need to type a patch to ditch all these selects from drivers ;-)
> That's a great idea imho.

You're volunteered? ;-)

> Note that some drivers still partially use FB directly - radeon comes
> to mind. Last time I've looked there was no particular reason for
> that, as relevant fb_helper already exists.
> In general I'm wondering if one cannot check-in a new cocci script
> with each function that get factored out. Even if they don't produce
> nice (enough) patches they will serve as a nice warning/sanity check,
> right ?
> 
> If anyone is wondering "Why should we bother?", mostly because people
> (too) often base their work before said functionality/helper has
> landed. There's also that we cannot catch everything during review ;-)

Maybe we could even piggy-pack on top of the cocci project. They maintain
a large repo of cocci scripts somewhere, we could use those. Otoh the big
work is in looking at fallout and acting upon it, which needs someone with
copious amounts of free time. I think that's the hard part.

Wrt old stuff slipping through review: Generally when someone does a
subsystem wide refactor that kind of stuff gets caught again. So I'm not
too worried.
-Daniel
diff mbox

Patch

diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c
index fd1eb9d..a52de2f 100644
--- a/drivers/gpu/drm/udl/udl_fb.c
+++ b/drivers/gpu/drm/udl/udl_fb.c
@@ -287,7 +287,7 @@  static void udl_fb_fillrect(struct fb_info *info, const struct fb_fillrect *rect
 {
 	struct udl_fbdev *ufbdev = info->par;
 
-	drm_fb_helper_sys_fillrect(info, rect);
+	sys_fillrect(info, rect);
 
 	udl_handle_damage(&ufbdev->ufb, rect->dx, rect->dy, rect->width,
 			  rect->height);
@@ -297,7 +297,7 @@  static void udl_fb_copyarea(struct fb_info *info, const struct fb_copyarea *regi
 {
 	struct udl_fbdev *ufbdev = info->par;
 
-	drm_fb_helper_sys_copyarea(info, region);
+	sys_copyarea(info, region);
 
 	udl_handle_damage(&ufbdev->ufb, region->dx, region->dy, region->width,
 			  region->height);
@@ -307,7 +307,7 @@  static void udl_fb_imageblit(struct fb_info *info, const struct fb_image *image)
 {
 	struct udl_fbdev *ufbdev = info->par;
 
-	drm_fb_helper_sys_imageblit(info, image);
+	sys_imageblit(info, image);
 
 	udl_handle_damage(&ufbdev->ufb, image->dx, image->dy, image->width,
 			  image->height);