| Message ID | 20200224173901.174016-7-jernej.skrabec@siol.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/sun4i: de2/de3 format fixes and updates | expand |
Hi, On Mon, Feb 24, 2020 at 06:39:00PM +0100, Jernej Skrabec wrote: > Now that de2_fmt_info contains only DRM <-> HW format mapping, it > doesn't make sense to return pointer to structure when searching by DRM > format. Rework that to return only HW format instead. > > This doesn't make any functional change. > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > --- > drivers/gpu/drm/sun4i/sun8i_mixer.c | 15 +++++++++++---- > drivers/gpu/drm/sun4i/sun8i_mixer.h | 7 +------ > drivers/gpu/drm/sun4i/sun8i_ui_layer.c | 10 +++++----- > drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 12 ++++++------ > 4 files changed, 23 insertions(+), 21 deletions(-) > > diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c b/drivers/gpu/drm/sun4i/sun8i_mixer.c > index e078ec96de2d..56cc037fd312 100644 > --- a/drivers/gpu/drm/sun4i/sun8i_mixer.c > +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c > @@ -27,6 +27,11 @@ > #include "sun8i_vi_layer.h" > #include "sunxi_engine.h" > > +struct de2_fmt_info { > + u32 drm_fmt; > + u32 de2_fmt; > +}; > + > static const struct de2_fmt_info de2_formats[] = { > { > .drm_fmt = DRM_FORMAT_ARGB8888, > @@ -230,15 +235,17 @@ static const struct de2_fmt_info de2_formats[] = { > }, > }; > > -const struct de2_fmt_info *sun8i_mixer_format_info(u32 format) > +int sun8i_mixer_drm_format_to_hw(u32 format, u32 *hw_format) > { > unsigned int i; > > for (i = 0; i < ARRAY_SIZE(de2_formats); ++i) > - if (de2_formats[i].drm_fmt == format) > - return &de2_formats[i]; > + if (de2_formats[i].drm_fmt == format) { > + *hw_format = de2_formats[i].de2_fmt; > + return 0; > + } > > - return NULL; > + return -EINVAL; > } I'm not too sure about that one. It breaks the consistency with the other functions, and I don't really see a particular benefit to it? The rest of the series is Acked-by: Maxime Ripard <mripard@kernel.org> Maxime
On Tue, Feb 25, 2020 at 4:35 PM Maxime Ripard <maxime@cerno.tech> wrote: > > Hi, > > On Mon, Feb 24, 2020 at 06:39:00PM +0100, Jernej Skrabec wrote: > > Now that de2_fmt_info contains only DRM <-> HW format mapping, it > > doesn't make sense to return pointer to structure when searching by DRM > > format. Rework that to return only HW format instead. > > > > This doesn't make any functional change. > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > --- > > drivers/gpu/drm/sun4i/sun8i_mixer.c | 15 +++++++++++---- > > drivers/gpu/drm/sun4i/sun8i_mixer.h | 7 +------ > > drivers/gpu/drm/sun4i/sun8i_ui_layer.c | 10 +++++----- > > drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 12 ++++++------ > > 4 files changed, 23 insertions(+), 21 deletions(-) > > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c b/drivers/gpu/drm/sun4i/sun8i_mixer.c > > index e078ec96de2d..56cc037fd312 100644 > > --- a/drivers/gpu/drm/sun4i/sun8i_mixer.c > > +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c > > @@ -27,6 +27,11 @@ > > #include "sun8i_vi_layer.h" > > #include "sunxi_engine.h" > > > > +struct de2_fmt_info { > > + u32 drm_fmt; > > + u32 de2_fmt; > > +}; > > + > > static const struct de2_fmt_info de2_formats[] = { > > { > > .drm_fmt = DRM_FORMAT_ARGB8888, > > @@ -230,15 +235,17 @@ static const struct de2_fmt_info de2_formats[] = { > > }, > > }; > > > > -const struct de2_fmt_info *sun8i_mixer_format_info(u32 format) > > +int sun8i_mixer_drm_format_to_hw(u32 format, u32 *hw_format) > > { > > unsigned int i; > > > > for (i = 0; i < ARRAY_SIZE(de2_formats); ++i) > > - if (de2_formats[i].drm_fmt == format) > > - return &de2_formats[i]; > > + if (de2_formats[i].drm_fmt == format) { > > + *hw_format = de2_formats[i].de2_fmt; > > + return 0; > > + } > > > > - return NULL; > > + return -EINVAL; > > } > > I'm not too sure about that one. It breaks the consistency with the > other functions, and I don't really see a particular benefit to it? I guess we could just define an "invalid" value, and have the function return that if can't find a match? I'm guessing 0x0 is valid, so maybe 0xffffffff or 0xdeadbeef ? That would keep consistency with everything else all the while removing the level of indirection you wanted to. ChenYu > The rest of the series is > Acked-by: Maxime Ripard <mripard@kernel.org> > > Maxime
Hi! Dne torek, 25. februar 2020 ob 09:52:18 CET je Chen-Yu Tsai napisal(a): > On Tue, Feb 25, 2020 at 4:35 PM Maxime Ripard <maxime@cerno.tech> wrote: > > Hi, > > > > On Mon, Feb 24, 2020 at 06:39:00PM +0100, Jernej Skrabec wrote: > > > Now that de2_fmt_info contains only DRM <-> HW format mapping, it > > > doesn't make sense to return pointer to structure when searching by DRM > > > format. Rework that to return only HW format instead. > > > > > > This doesn't make any functional change. > > > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > > --- > > > > > > drivers/gpu/drm/sun4i/sun8i_mixer.c | 15 +++++++++++---- > > > drivers/gpu/drm/sun4i/sun8i_mixer.h | 7 +------ > > > drivers/gpu/drm/sun4i/sun8i_ui_layer.c | 10 +++++----- > > > drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 12 ++++++------ > > > 4 files changed, 23 insertions(+), 21 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c > > > b/drivers/gpu/drm/sun4i/sun8i_mixer.c index e078ec96de2d..56cc037fd312 > > > 100644 > > > --- a/drivers/gpu/drm/sun4i/sun8i_mixer.c > > > +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c > > > @@ -27,6 +27,11 @@ > > > > > > #include "sun8i_vi_layer.h" > > > #include "sunxi_engine.h" > > > > > > +struct de2_fmt_info { > > > + u32 drm_fmt; > > > + u32 de2_fmt; > > > +}; > > > + > > > > > > static const struct de2_fmt_info de2_formats[] = { > > > > > > { > > > > > > .drm_fmt = DRM_FORMAT_ARGB8888, > > > > > > @@ -230,15 +235,17 @@ static const struct de2_fmt_info de2_formats[] = { > > > > > > }, > > > > > > }; > > > > > > -const struct de2_fmt_info *sun8i_mixer_format_info(u32 format) > > > +int sun8i_mixer_drm_format_to_hw(u32 format, u32 *hw_format) > > > > > > { > > > > > > unsigned int i; > > > > > > for (i = 0; i < ARRAY_SIZE(de2_formats); ++i) > > > > > > - if (de2_formats[i].drm_fmt == format) > > > - return &de2_formats[i]; > > > + if (de2_formats[i].drm_fmt == format) { > > > + *hw_format = de2_formats[i].de2_fmt; > > > + return 0; > > > + } > > > > > > - return NULL; > > > + return -EINVAL; > > > > > > } > > > > I'm not too sure about that one. It breaks the consistency with the > > other functions, and I don't really see a particular benefit to it? > I don't have strong opinion about this patch. It can be dropped. > I guess we could just define an "invalid" value, and have the function > return that if can't find a match? I'm guessing 0x0 is valid, so maybe > 0xffffffff or 0xdeadbeef ? > > That would keep consistency with everything else all the while > removing the level of indirection you wanted to. I modeled this after static int sun4i_backend_drm_format_to_layer(u32 format, u32 *mode); from sun4i_backend.c. What consistency do you have in mind? > > ChenYu > > > The rest of the series is > > Acked-by: Maxime Ripard <mripard@kernel.org> Thanks! Best regards, Jernej > > > > Maxime
On Wed, Feb 26, 2020 at 2:50 AM Jernej Škrabec <jernej.skrabec@siol.net> wrote: > > Hi! > > Dne torek, 25. februar 2020 ob 09:52:18 CET je Chen-Yu Tsai napisal(a): > > On Tue, Feb 25, 2020 at 4:35 PM Maxime Ripard <maxime@cerno.tech> wrote: > > > Hi, > > > > > > On Mon, Feb 24, 2020 at 06:39:00PM +0100, Jernej Skrabec wrote: > > > > Now that de2_fmt_info contains only DRM <-> HW format mapping, it > > > > doesn't make sense to return pointer to structure when searching by DRM > > > > format. Rework that to return only HW format instead. > > > > > > > > This doesn't make any functional change. > > > > > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > > > --- > > > > > > > > drivers/gpu/drm/sun4i/sun8i_mixer.c | 15 +++++++++++---- > > > > drivers/gpu/drm/sun4i/sun8i_mixer.h | 7 +------ > > > > drivers/gpu/drm/sun4i/sun8i_ui_layer.c | 10 +++++----- > > > > drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 12 ++++++------ > > > > 4 files changed, 23 insertions(+), 21 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c > > > > b/drivers/gpu/drm/sun4i/sun8i_mixer.c index e078ec96de2d..56cc037fd312 > > > > 100644 > > > > --- a/drivers/gpu/drm/sun4i/sun8i_mixer.c > > > > +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c > > > > @@ -27,6 +27,11 @@ > > > > > > > > #include "sun8i_vi_layer.h" > > > > #include "sunxi_engine.h" > > > > > > > > +struct de2_fmt_info { > > > > + u32 drm_fmt; > > > > + u32 de2_fmt; > > > > +}; > > > > + > > > > > > > > static const struct de2_fmt_info de2_formats[] = { > > > > > > > > { > > > > > > > > .drm_fmt = DRM_FORMAT_ARGB8888, > > > > > > > > @@ -230,15 +235,17 @@ static const struct de2_fmt_info de2_formats[] = { > > > > > > > > }, > > > > > > > > }; > > > > > > > > -const struct de2_fmt_info *sun8i_mixer_format_info(u32 format) > > > > +int sun8i_mixer_drm_format_to_hw(u32 format, u32 *hw_format) > > > > > > > > { > > > > > > > > unsigned int i; > > > > > > > > for (i = 0; i < ARRAY_SIZE(de2_formats); ++i) > > > > > > > > - if (de2_formats[i].drm_fmt == format) > > > > - return &de2_formats[i]; > > > > + if (de2_formats[i].drm_fmt == format) { > > > > + *hw_format = de2_formats[i].de2_fmt; > > > > + return 0; > > > > + } > > > > > > > > - return NULL; > > > > + return -EINVAL; > > > > > > > > } > > > > > > I'm not too sure about that one. It breaks the consistency with the > > > other functions, and I don't really see a particular benefit to it? > > > > I don't have strong opinion about this patch. It can be dropped. > > > I guess we could just define an "invalid" value, and have the function > > return that if can't find a match? I'm guessing 0x0 is valid, so maybe > > 0xffffffff or 0xdeadbeef ? > > > > That would keep consistency with everything else all the while > > removing the level of indirection you wanted to. > > I modeled this after > static int sun4i_backend_drm_format_to_layer(u32 format, u32 *mode); > from sun4i_backend.c. > > What consistency do you have in mind? Directly returning values (or error codes) instead of passing in a pointer for data to be returned. I assumed that was what Maxime was referring to. ChenYu > > > > ChenYu > > > > > The rest of the series is > > > Acked-by: Maxime Ripard <mripard@kernel.org> > > Thanks! > > Best regards, > Jernej > > > > > > > Maxime > > > >
On Tue, Feb 25, 2020 at 07:50:03PM +0100, Jernej Škrabec wrote: > Hi! > > Dne torek, 25. februar 2020 ob 09:52:18 CET je Chen-Yu Tsai napisal(a): > > On Tue, Feb 25, 2020 at 4:35 PM Maxime Ripard <maxime@cerno.tech> wrote: > > > Hi, > > > > > > On Mon, Feb 24, 2020 at 06:39:00PM +0100, Jernej Skrabec wrote: > > > > Now that de2_fmt_info contains only DRM <-> HW format mapping, it > > > > doesn't make sense to return pointer to structure when searching by DRM > > > > format. Rework that to return only HW format instead. > > > > > > > > This doesn't make any functional change. > > > > > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > > > --- > > > > > > > > drivers/gpu/drm/sun4i/sun8i_mixer.c | 15 +++++++++++---- > > > > drivers/gpu/drm/sun4i/sun8i_mixer.h | 7 +------ > > > > drivers/gpu/drm/sun4i/sun8i_ui_layer.c | 10 +++++----- > > > > drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 12 ++++++------ > > > > 4 files changed, 23 insertions(+), 21 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c > > > > b/drivers/gpu/drm/sun4i/sun8i_mixer.c index e078ec96de2d..56cc037fd312 > > > > 100644 > > > > --- a/drivers/gpu/drm/sun4i/sun8i_mixer.c > > > > +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c > > > > @@ -27,6 +27,11 @@ > > > > > > > > #include "sun8i_vi_layer.h" > > > > #include "sunxi_engine.h" > > > > > > > > +struct de2_fmt_info { > > > > + u32 drm_fmt; > > > > + u32 de2_fmt; > > > > +}; > > > > + > > > > > > > > static const struct de2_fmt_info de2_formats[] = { > > > > > > > > { > > > > > > > > .drm_fmt = DRM_FORMAT_ARGB8888, > > > > > > > > @@ -230,15 +235,17 @@ static const struct de2_fmt_info de2_formats[] = { > > > > > > > > }, > > > > > > > > }; > > > > > > > > -const struct de2_fmt_info *sun8i_mixer_format_info(u32 format) > > > > +int sun8i_mixer_drm_format_to_hw(u32 format, u32 *hw_format) > > > > > > > > { > > > > > > > > unsigned int i; > > > > > > > > for (i = 0; i < ARRAY_SIZE(de2_formats); ++i) > > > > > > > > - if (de2_formats[i].drm_fmt == format) > > > > - return &de2_formats[i]; > > > > + if (de2_formats[i].drm_fmt == format) { > > > > + *hw_format = de2_formats[i].de2_fmt; > > > > + return 0; > > > > + } > > > > > > > > - return NULL; > > > > + return -EINVAL; > > > > > > > > } > > > > > > I'm not too sure about that one. It breaks the consistency with the > > > other functions, and I don't really see a particular benefit to it? > > > > I don't have strong opinion about this patch. It can be dropped. > > > I guess we could just define an "invalid" value, and have the function > > return that if can't find a match? I'm guessing 0x0 is valid, so maybe > > 0xffffffff or 0xdeadbeef ? > > > > That would keep consistency with everything else all the while > > removing the level of indirection you wanted to. > > I modeled this after > static int sun4i_backend_drm_format_to_layer(u32 format, u32 *mode); > from sun4i_backend.c. > > What consistency do you have in mind? Well I guess if we're doing that elsewhere it's not really fair to ask you to change this then :) Fine by me Maxime
diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c b/drivers/gpu/drm/sun4i/sun8i_mixer.c index e078ec96de2d..56cc037fd312 100644 --- a/drivers/gpu/drm/sun4i/sun8i_mixer.c +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c @@ -27,6 +27,11 @@ #include "sun8i_vi_layer.h" #include "sunxi_engine.h" +struct de2_fmt_info { + u32 drm_fmt; + u32 de2_fmt; +}; + static const struct de2_fmt_info de2_formats[] = { { .drm_fmt = DRM_FORMAT_ARGB8888, @@ -230,15 +235,17 @@ static const struct de2_fmt_info de2_formats[] = { }, }; -const struct de2_fmt_info *sun8i_mixer_format_info(u32 format) +int sun8i_mixer_drm_format_to_hw(u32 format, u32 *hw_format) { unsigned int i; for (i = 0; i < ARRAY_SIZE(de2_formats); ++i) - if (de2_formats[i].drm_fmt == format) - return &de2_formats[i]; + if (de2_formats[i].drm_fmt == format) { + *hw_format = de2_formats[i].de2_fmt; + return 0; + } - return NULL; + return -EINVAL; } static void sun8i_mixer_commit(struct sunxi_engine *engine) diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.h b/drivers/gpu/drm/sun4i/sun8i_mixer.h index 0dd4a347fa06..7576b523fdbb 100644 --- a/drivers/gpu/drm/sun4i/sun8i_mixer.h +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.h @@ -143,11 +143,6 @@ #define SUN50I_MIXER_CDC0_EN 0xd0000 #define SUN50I_MIXER_CDC1_EN 0xd8000 -struct de2_fmt_info { - u32 drm_fmt; - u32 de2_fmt; -}; - /** * struct sun8i_mixer_cfg - mixer HW configuration * @vi_num: number of VI channels @@ -207,5 +202,5 @@ sun8i_channel_base(struct sun8i_mixer *mixer, int channel) return DE2_CH_BASE + channel * DE2_CH_SIZE; } -const struct de2_fmt_info *sun8i_mixer_format_info(u32 format); +int sun8i_mixer_drm_format_to_hw(u32 format, u32 *hw_format); #endif /* _SUN8I_MIXER_H_ */ diff --git a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c index 99ee19a00415..a64aaea1ba74 100644 --- a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c +++ b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c @@ -174,20 +174,20 @@ static int sun8i_ui_layer_update_formats(struct sun8i_mixer *mixer, int channel, int overlay, struct drm_plane *plane) { struct drm_plane_state *state = plane->state; - const struct de2_fmt_info *fmt_info; const struct drm_format_info *fmt; - u32 val, ch_base; + u32 val, ch_base, hw_fmt; + int ret; ch_base = sun8i_channel_base(mixer, channel); fmt = state->fb->format; - fmt_info = sun8i_mixer_format_info(fmt->format); - if (!fmt_info || fmt->is_yuv) { + ret = sun8i_mixer_drm_format_to_hw(fmt->format, &hw_fmt); + if (ret || fmt->is_yuv) { DRM_DEBUG_DRIVER("Invalid format\n"); return -EINVAL; } - val = fmt_info->de2_fmt << SUN8I_MIXER_CHAN_UI_LAYER_ATTR_FBFMT_OFFSET; + val = hw_fmt << SUN8I_MIXER_CHAN_UI_LAYER_ATTR_FBFMT_OFFSET; regmap_update_bits(mixer->engine.regs, SUN8I_MIXER_CHAN_UI_LAYER_ATTR(ch_base, overlay), SUN8I_MIXER_CHAN_UI_LAYER_ATTR_FBFMT_MASK, val); diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c index d783c2bfc77e..b1e1ba2da663 100644 --- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c +++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c @@ -231,20 +231,20 @@ static int sun8i_vi_layer_update_formats(struct sun8i_mixer *mixer, int channel, int overlay, struct drm_plane *plane) { struct drm_plane_state *state = plane->state; - const struct de2_fmt_info *fmt_info; + u32 val, ch_base, csc_mode, hw_fmt; const struct drm_format_info *fmt; - u32 val, ch_base, csc_mode; + int ret; ch_base = sun8i_channel_base(mixer, channel); fmt = state->fb->format; - fmt_info = sun8i_mixer_format_info(fmt->format); - if (!fmt_info) { + ret = sun8i_mixer_drm_format_to_hw(fmt->format, &hw_fmt); + if (ret) { DRM_DEBUG_DRIVER("Invalid format\n"); - return -EINVAL; + return ret; } - val = fmt_info->de2_fmt << SUN8I_MIXER_CHAN_VI_LAYER_ATTR_FBFMT_OFFSET; + val = hw_fmt << SUN8I_MIXER_CHAN_VI_LAYER_ATTR_FBFMT_OFFSET; regmap_update_bits(mixer->engine.regs, SUN8I_MIXER_CHAN_VI_LAYER_ATTR(ch_base, overlay), SUN8I_MIXER_CHAN_VI_LAYER_ATTR_FBFMT_MASK, val);
Now that de2_fmt_info contains only DRM <-> HW format mapping, it doesn't make sense to return pointer to structure when searching by DRM format. Rework that to return only HW format instead. This doesn't make any functional change. Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> --- drivers/gpu/drm/sun4i/sun8i_mixer.c | 15 +++++++++++---- drivers/gpu/drm/sun4i/sun8i_mixer.h | 7 +------ drivers/gpu/drm/sun4i/sun8i_ui_layer.c | 10 +++++----- drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 12 ++++++------ 4 files changed, 23 insertions(+), 21 deletions(-)