Re: [PATCH] drm: simple_kms_helper: Add mode_valid() callback support

[Daniel Vetter]

On Tue, Feb 20, 2018 at 10:52:46AM +0100, Thierry Reding wrote:
> On Tue, Feb 20, 2018 at 08:28:59AM +0100, Linus Walleij wrote:
> > The PL111 needs to filter valid modes based on memory bandwidth.
> > I guess it is a pretty simple operation, so we can still claim
> > the DRM KMS helper pipeline is simple after adding this (optional)
> > vtable callback.
> > 
> > Reviewed-by: Eric Anholt 
> > Reviewed-by: Daniel Vetter 
> > Signed-off-by: Linus Walleij 
> > ---
> > ChangeLog v1->v2:
> > - Fix up the kerneldoc to just refer to the enum.
> > - Collect Eric's and Daniel's review tags.
> > ---
> >  drivers/gpu/drm/drm_simple_kms_helper.c | 15 +++
> >  include/drm/drm_simple_kms_helper.h | 14 ++
> >  2 files changed, 29 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c 
> > b/drivers/gpu/drm/drm_simple_kms_helper.c
> > index 9f3b1c94802b..652cde9a5a6b 100644
> > --- a/drivers/gpu/drm/drm_simple_kms_helper.c
> > +++ b/drivers/gpu/drm/drm_simple_kms_helper.c
> > @@ -34,6 +34,20 @@ static const struct drm_encoder_funcs 
> > drm_simple_kms_encoder_funcs = {
> > .destroy = drm_encoder_cleanup,
> >  };
> >  
> > +static enum drm_mode_status
> > +drm_simple_kms_crtc_mode_valid(struct drm_crtc *crtc,
> > +  const struct drm_display_mode *mode)
> > +{
> > +   struct drm_simple_display_pipe *pipe;
> > +
> > +   pipe = container_of(crtc, struct drm_simple_display_pipe, crtc);
> > +   if (!pipe->funcs || !pipe->funcs->mode_valid)
> > +   /* Anything goes */
> > +   return MODE_OK;
> > +
> > +   return pipe->funcs->mode_valid(crtc, mode);
> > +}
> > +
> >  static int drm_simple_kms_crtc_check(struct drm_crtc *crtc,
> >  struct drm_crtc_state *state)
> >  {
> > @@ -72,6 +86,7 @@ static void drm_simple_kms_crtc_disable(struct drm_crtc 
> > *crtc,
> >  }
> >  
> >  static const struct drm_crtc_helper_funcs drm_simple_kms_crtc_helper_funcs 
> > = {
> > +   .mode_valid = drm_simple_kms_crtc_mode_valid,
> > .atomic_check = drm_simple_kms_crtc_check,
> > .atomic_enable = drm_simple_kms_crtc_enable,
> > .atomic_disable = drm_simple_kms_crtc_disable,
> > diff --git a/include/drm/drm_simple_kms_helper.h 
> > b/include/drm/drm_simple_kms_helper.h
> > index 6d9adbb46293..d9e4c3c3f009 100644
> > --- a/include/drm/drm_simple_kms_helper.h
> > +++ b/include/drm/drm_simple_kms_helper.h
> > @@ -21,6 +21,20 @@ struct drm_simple_display_pipe;
> >   *display pipeline
> >   */
> >  struct drm_simple_display_pipe_funcs {
> > +   /**
> > +* @mode_valid:
> > +*
> > +* This function is called to filter out valid modes from the
> > +* suggestions suggested by the bridge or display. This optional
> 
> This is a little awkward to read. Perhaps something like "... filter out
> valid modes from those suggested by..."?

If you want to respin the kerneldoc I suggest you align more with the one
for drm_crtc_helper_funcs.mode_valid. There's a bunch of things you're
missing here. I think for consistency probably best to just copypaste the
entire thing, with an s/crtc/simple display pipe/ or similar.
-Daniel


> 
> Otherwise looks good:
> 
> Reviewed-by: Thierry Reding 



> ___
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel


-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: simple_kms_helper: Add mode_valid() callback support

[Thierry Reding]

On Tue, Feb 20, 2018 at 08:28:59AM +0100, Linus Walleij wrote:
> The PL111 needs to filter valid modes based on memory bandwidth.
> I guess it is a pretty simple operation, so we can still claim
> the DRM KMS helper pipeline is simple after adding this (optional)
> vtable callback.
> 
> Reviewed-by: Eric Anholt 
> Reviewed-by: Daniel Vetter 
> Signed-off-by: Linus Walleij 
> ---
> ChangeLog v1->v2:
> - Fix up the kerneldoc to just refer to the enum.
> - Collect Eric's and Daniel's review tags.
> ---
>  drivers/gpu/drm/drm_simple_kms_helper.c | 15 +++
>  include/drm/drm_simple_kms_helper.h | 14 ++
>  2 files changed, 29 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c 
> b/drivers/gpu/drm/drm_simple_kms_helper.c
> index 9f3b1c94802b..652cde9a5a6b 100644
> --- a/drivers/gpu/drm/drm_simple_kms_helper.c
> +++ b/drivers/gpu/drm/drm_simple_kms_helper.c
> @@ -34,6 +34,20 @@ static const struct drm_encoder_funcs 
> drm_simple_kms_encoder_funcs = {
>   .destroy = drm_encoder_cleanup,
>  };
>  
> +static enum drm_mode_status
> +drm_simple_kms_crtc_mode_valid(struct drm_crtc *crtc,
> +const struct drm_display_mode *mode)
> +{
> + struct drm_simple_display_pipe *pipe;
> +
> + pipe = container_of(crtc, struct drm_simple_display_pipe, crtc);
> + if (!pipe->funcs || !pipe->funcs->mode_valid)
> + /* Anything goes */
> + return MODE_OK;
> +
> + return pipe->funcs->mode_valid(crtc, mode);
> +}
> +
>  static int drm_simple_kms_crtc_check(struct drm_crtc *crtc,
>struct drm_crtc_state *state)
>  {
> @@ -72,6 +86,7 @@ static void drm_simple_kms_crtc_disable(struct drm_crtc 
> *crtc,
>  }
>  
>  static const struct drm_crtc_helper_funcs drm_simple_kms_crtc_helper_funcs = 
> {
> + .mode_valid = drm_simple_kms_crtc_mode_valid,
>   .atomic_check = drm_simple_kms_crtc_check,
>   .atomic_enable = drm_simple_kms_crtc_enable,
>   .atomic_disable = drm_simple_kms_crtc_disable,
> diff --git a/include/drm/drm_simple_kms_helper.h 
> b/include/drm/drm_simple_kms_helper.h
> index 6d9adbb46293..d9e4c3c3f009 100644
> --- a/include/drm/drm_simple_kms_helper.h
> +++ b/include/drm/drm_simple_kms_helper.h
> @@ -21,6 +21,20 @@ struct drm_simple_display_pipe;
>   *display pipeline
>   */
>  struct drm_simple_display_pipe_funcs {
> + /**
> +  * @mode_valid:
> +  *
> +  * This function is called to filter out valid modes from the
> +  * suggestions suggested by the bridge or display. This optional

This is a little awkward to read. Perhaps something like "... filter out
valid modes from those suggested by..."?

Otherwise looks good:

Reviewed-by: Thierry Reding 


signature.asc
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm: simple_kms_helper: Add mode_valid() callback support

[Linus Walleij]

The PL111 needs to filter valid modes based on memory bandwidth.
I guess it is a pretty simple operation, so we can still claim
the DRM KMS helper pipeline is simple after adding this (optional)
vtable callback.

Reviewed-by: Eric Anholt 
Reviewed-by: Daniel Vetter 
Signed-off-by: Linus Walleij 
---
ChangeLog v1->v2:
- Fix up the kerneldoc to just refer to the enum.
- Collect Eric's and Daniel's review tags.
---
 drivers/gpu/drm/drm_simple_kms_helper.c | 15 +++
 include/drm/drm_simple_kms_helper.h | 14 ++
 2 files changed, 29 insertions(+)

diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c 
b/drivers/gpu/drm/drm_simple_kms_helper.c
index 9f3b1c94802b..652cde9a5a6b 100644
--- a/drivers/gpu/drm/drm_simple_kms_helper.c
+++ b/drivers/gpu/drm/drm_simple_kms_helper.c
@@ -34,6 +34,20 @@ static const struct drm_encoder_funcs 
drm_simple_kms_encoder_funcs = {
.destroy = drm_encoder_cleanup,
 };
 
+static enum drm_mode_status
+drm_simple_kms_crtc_mode_valid(struct drm_crtc *crtc,
+  const struct drm_display_mode *mode)
+{
+   struct drm_simple_display_pipe *pipe;
+
+   pipe = container_of(crtc, struct drm_simple_display_pipe, crtc);
+   if (!pipe->funcs || !pipe->funcs->mode_valid)
+   /* Anything goes */
+   return MODE_OK;
+
+   return pipe->funcs->mode_valid(crtc, mode);
+}
+
 static int drm_simple_kms_crtc_check(struct drm_crtc *crtc,
 struct drm_crtc_state *state)
 {
@@ -72,6 +86,7 @@ static void drm_simple_kms_crtc_disable(struct drm_crtc *crtc,
 }
 
 static const struct drm_crtc_helper_funcs drm_simple_kms_crtc_helper_funcs = {
+   .mode_valid = drm_simple_kms_crtc_mode_valid,
.atomic_check = drm_simple_kms_crtc_check,
.atomic_enable = drm_simple_kms_crtc_enable,
.atomic_disable = drm_simple_kms_crtc_disable,
diff --git a/include/drm/drm_simple_kms_helper.h 
b/include/drm/drm_simple_kms_helper.h
index 6d9adbb46293..d9e4c3c3f009 100644
--- a/include/drm/drm_simple_kms_helper.h
+++ b/include/drm/drm_simple_kms_helper.h
@@ -21,6 +21,20 @@ struct drm_simple_display_pipe;
  *display pipeline
  */
 struct drm_simple_display_pipe_funcs {
+   /**
+* @mode_valid:
+*
+* This function is called to filter out valid modes from the
+* suggestions suggested by the bridge or display. This optional
+* hook is passed in when initializing the pipeline.
+*
+* RETURNS:
+*
+* drm_mode_status Enum
+*/
+   enum drm_mode_status (*mode_valid)(struct drm_crtc *crtc,
+  const struct drm_display_mode *mode);
+
/**
 * @enable:
 *
-- 
2.14.3

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