Re: [PATCH] drm/simple-kms: Standardize arguments for callbacks

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi

Am 22.10.19 um 21:03 schrieb Daniel Vetter:
> On Tue, Oct 22, 2019 at 7:16 PM Thomas Zimmermann <tzimmermann@xxxxxxx> wrote:
>>
>> Hi,
>>
>> there are two types of callbacks in struct
>> drm_simple_display_pipe_funcs: functions that are genuine to simple KMS,
>> and functions that are merely forwarded from another structure (crtc,
>> plane, etc).
>>
>> In the former category are enable(), disable(), check(), and update().
>> Those should probably receive a simple display pipe as their first argument.
> 
> mode_valid _very_ much belongs to this category too, since there's
> mode_valid hooks also on other objects. But simple pipe helper
> condenses that down to one mode_valid hook (we could also put the
> mode_valid onto encoder, wouldn't change anything).
> 
>> In the latter category are mode_valid(), prepare_fb(), cleanup_fb(),
>> enable_vblank(), and disable_vblank(). IMHO those functions should
>> receive a pointer to the original structure as their first argument.
>> This type provides the context in which the operations make sense. (Even
>> their documentation already refers to the original structure.)
> 
> Now on those you can maybe make a case that they only exist in one
> object. But the entire point of simple helpers was to condense the zoo
> of drm types down to one. Only reason you don't also get a
> drm_simple_display_pipe_state is that this one would be a bit more
> work to make work correctly. If we full on leak all the underlying
> objects, then you might as well set them up yourself and set up all
> the hooks, it's just a few more lines of code.
> 
> Imo for simple pipe we should go more into that direction, not less.
> 
>> I admit that not all are as shareable as prepare_fb() and enable_fb().
>> But what else than boiler-plate wrappers do we get from simple display
>> pipe here?
> 
> Boiler plate wrappers is pretty much the entire point of simple pipe
> helpers. Anytime you're interested in the things it abstracts away
> (crtc, plane, encoder) you probably want your own atomic
> implementation.

I was speaking of boiler-plate code in drivers and other helpers (e.g.,
drm_gem_fb_simple_display_pipe_prepare_fb() )

TBH I don't think it is possible to build and use simple pipe without
exposing the underlying primitives (crtc, plane, connector). This would
require a completely separate set of atomic helpers. IMHO the current
simple pipe is a mid-layer and comes with typical mid-layer problems.

Anyway, given your rational for the current design, I'll update my
recent patches for prepare_fb() to support simple pipe.

For this patch

  Acked-By: Thomas Zimmermann <tzimmermann@xxxxxxx>

Best regards
Thomas

> conversion is a good fit, it's not meant to be useful for all small
> drivers. Only for the _really_ simple ones.
> 
> Otherwise if we readd all the bells and whistles to simple pipe
> helpers, then we just end back where we started. That's also why I
> personally think explicit simple wrappers would fit better, instead of
> wrestling the prepare/cleanup_fb functions to match full atomic
> helpers.
> -Daniel
> 
>>
>> Best regards
>> Thomas
>>
>> Am 22.10.19 um 17:55 schrieb Daniel Vetter:
>>> Passing the wrong type feels icky, everywhere else we use the pipe as
>>> the first parameter. Spotted while discussing patches with Thomas
>>> Zimmermann.
>>>
>>> Cc: Thomas Zimmermann <tzimmermann@xxxxxxx>
>>> Cc: Noralf Trønnes <noralf@xxxxxxxxxxx>
>>> Cc: Gerd Hoffmann <kraxel@xxxxxxxxxx>
>>> Cc: Eric Anholt <eric@xxxxxxxxxx>
>>> Cc: Emil Velikov <emil.velikov@xxxxxxxxxxxxx>
>>> Cc: virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
>>> Cc: Linus Walleij <linus.walleij@xxxxxxxxxx>
>>> Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx>
>>> ---
>>>  drivers/gpu/drm/cirrus/cirrus.c         | 2 +-
>>>  drivers/gpu/drm/drm_simple_kms_helper.c | 2 +-
>>>  drivers/gpu/drm/pl111/pl111_display.c   | 4 ++--
>>>  include/drm/drm_simple_kms_helper.h     | 2 +-
>>>  4 files changed, 5 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/cirrus/cirrus.c b/drivers/gpu/drm/cirrus/cirrus.c
>>> index 7d08d067e1a4..248c9f765c45 100644
>>> --- a/drivers/gpu/drm/cirrus/cirrus.c
>>> +++ b/drivers/gpu/drm/cirrus/cirrus.c
>>> @@ -390,7 +390,7 @@ static int cirrus_conn_init(struct cirrus_device *cirrus)
>>>  /* ------------------------------------------------------------------ */
>>>  /* cirrus (simple) display pipe                                            */
>>>
>>> -static enum drm_mode_status cirrus_pipe_mode_valid(struct drm_crtc *crtc,
>>> +static enum drm_mode_status cirrus_pipe_mode_valid(struct drm_simple_display_pipe *pipe,
>>>                                                  const struct drm_display_mode *mode)
>>>  {
>>>       if (cirrus_check_size(mode->hdisplay, mode->vdisplay, NULL) < 0)
>>> diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c
>>> index 046055719245..15fb516ae2d8 100644
>>> --- a/drivers/gpu/drm/drm_simple_kms_helper.c
>>> +++ b/drivers/gpu/drm/drm_simple_kms_helper.c
>>> @@ -43,7 +43,7 @@ drm_simple_kms_crtc_mode_valid(struct drm_crtc *crtc,
>>>               /* Anything goes */
>>>               return MODE_OK;
>>>
>>> -     return pipe->funcs->mode_valid(crtc, mode);
>>> +     return pipe->funcs->mode_valid(pipe, mode);
>>>  }
>>>
>>>  static int drm_simple_kms_crtc_check(struct drm_crtc *crtc,
>>> diff --git a/drivers/gpu/drm/pl111/pl111_display.c b/drivers/gpu/drm/pl111/pl111_display.c
>>> index 024771a4083e..703ddc803c55 100644
>>> --- a/drivers/gpu/drm/pl111/pl111_display.c
>>> +++ b/drivers/gpu/drm/pl111/pl111_display.c
>>> @@ -48,10 +48,10 @@ irqreturn_t pl111_irq(int irq, void *data)
>>>  }
>>>
>>>  static enum drm_mode_status
>>> -pl111_mode_valid(struct drm_crtc *crtc,
>>> +pl111_mode_valid(struct drm_simple_display_pipe *pipe,
>>>                const struct drm_display_mode *mode)
>>>  {
>>> -     struct drm_device *drm = crtc->dev;
>>> +     struct drm_device *drm = pipe->crtc.dev;
>>>       struct pl111_drm_dev_private *priv = drm->dev_private;
>>>       u32 cpp = priv->variant->fb_bpp / 8;
>>>       u64 bw;
>>> diff --git a/include/drm/drm_simple_kms_helper.h b/include/drm/drm_simple_kms_helper.h
>>> index 4d89cd0a60db..15afee9cf049 100644
>>> --- a/include/drm/drm_simple_kms_helper.h
>>> +++ b/include/drm/drm_simple_kms_helper.h
>>> @@ -49,7 +49,7 @@ struct drm_simple_display_pipe_funcs {
>>>        *
>>>        * drm_mode_status Enum
>>>        */
>>> -     enum drm_mode_status (*mode_valid)(struct drm_crtc *crtc,
>>> +     enum drm_mode_status (*mode_valid)(struct drm_simple_display_pipe *pipe,
>>>                                          const struct drm_display_mode *mode);
>>>
>>>       /**
>>>
>>
>> --
>> Thomas Zimmermann
>> Graphics Driver Developer
>> SUSE Software Solutions Germany GmbH
>> Maxfeldstr. 5, 90409 Nürnberg, Germany
>> (HRB 36809, AG Nürnberg)
>> Geschäftsführer: Felix Imendörffer
>>
> 
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer

Attachment: signature.asc
Description: OpenPGP digital signature

_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux