[Intel-gfx] [PATCH v4] drm/i915/fbdev: Implement fb_dirty for intel custom fb helper

[Thomas Zimmermann]

Hi

Am 23.01.23 um 13:20 schrieb Hogander, Jouni:
> On Mon, 2023-01-23 at 12:38 +0100, Thomas Zimmermann wrote:
>> Hi
>>
>> Am 23.01.23 um 08:44 schrieb Jouni Högander:
>>> After disconnecting damage worker from update logic it's left to
>>> fbdev
>>> emulation implementation to have fb_dirty function. Currently intel
>>> fbdev doesn't have it. This is causing problems to features (PSR,
>>> FBC,
>>> DRRS) relying on dirty callback.
>>>
>>> Implement simple fb_dirty callback to deliver notifications about
>>> updates
>>> in fb console.
>>>
>>> v4: Add proper Fixes tag and modify commit message
>>> v3: Check damage clip
>>> v2: Improved commit message and added Fixes tag
>>>
>>> Fixes: f231af498c29 ("drm/fb-helper: Disconnect damage worker from
>>> update logic")
>>> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
>>> Cc: Thomas Zimmermann <tzimmermann at suse.de>
>>> Cc: Jani Nikula <jani.nikula at intel.com>
>>> Signed-off-by: Jouni Högander <jouni.hogander at intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/display/intel_fbdev.c | 12 ++++++++++++
>>>    1 file changed, 12 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c
>>> b/drivers/gpu/drm/i915/display/intel_fbdev.c
>>> index 19f3b5d92a55..d39db8050c69 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_fbdev.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_fbdev.c
>>> @@ -321,8 +321,20 @@ static int intelfb_create(struct drm_fb_helper
>>> *helper,
>>>          return ret;
>>>    }
>>>    
>>> +static int intelfb_dirty(struct drm_fb_helper *helper, struct
>>> drm_clip_rect *clip)
>>> +{
>>> +       if (!(clip->x1 < clip->x2 && clip->y1 < clip->y2))
>>> +               return 0;
>>> +
>>> +       if (helper->fb->funcs->dirty)
>>> +               return helper->fb->funcs->dirty(helper->fb, NULL,
>>> 0, 0, clip, 1);
>>
>> Didn't I nack this approach already? You shouldn't set fb_dirty. The
>> better solution was to implement i915-specific helpers for write,
>> fillarea, copyarea and blit. Those should call intelfb_dirty()
>> directly
>> after performing the output.  And IIRC you already implemented this.
> 
> I have implemented such thing. I didn't took it as a nack back then.
> Last comment from you was:
> 
> "if you go with fb_dirty, please implement the clipping
> test in your callback."
> 
> and v3 was prepared to address that comment. My thinking was that this
> is only for fb console, but Ville Syrjälä commented that there is more
> than console so I sticked in this approach.
> 
> So you think I should just drop idea of setting dirty callback and
> implement those i915-specific helpers?

Ah OK, so I remembered incorrectly.

A few things have changed since my original comment and I worked on 
fbdev helper a bit.  The thing is that fb_dirty will likely go away at 
some point (together with the rest of drm_fb_helper_funcs). IOW at some 
point, you'll need those i915 functions anyways. Not using fb_dirty now 
will safe that work later on.

Best regards
Thomas

> 
>>
>> Best regards
>> Thomas
>>
>>> +
>>> +       return 0;
>>> +}
>>> +
>>>    static const struct drm_fb_helper_funcs intel_fb_helper_funcs = {
>>>          .fb_probe = intelfb_create,
>>> +       .fb_dirty = intelfb_dirty,
>>>    };
>>>    
>>>    static void intel_fbdev_destroy(struct intel_fbdev *ifbdev)
>>
>> -- 
>> Thomas Zimmermann
>> Graphics Driver Developer
>> SUSE Software Solutions Germany GmbH
>> Maxfeldstr. 5, 90409 Nürnberg, Germany
>> (HRB 36809, AG Nürnberg)
>> Geschäftsführer: Ivo Totev
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 840 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/intel-gfx/attachments/20230123/6f0a5139/attachment-0001.sig>